On Mon, Aug 05, 2024, mlevitsk@xxxxxxxxxx wrote: > У пт, 2024-08-02 у 08:53 -0700, Sean Christopherson пише: > > > > > > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > > > > > > > > index a6968eadd418..3582f0bb7644 100644 > > > > > > > > > --- a/arch/x86/kvm/x86.c > > > > > > > > > +++ b/arch/x86/kvm/x86.c > > > > > > > > > @@ -1844,7 +1844,16 @@ static int __kvm_set_msr(struct kvm_vcpu *vcpu, u32 index, u64 data, > > > > > > > > > case MSR_KERNEL_GS_BASE: > > > > > > > > > case MSR_CSTAR: > > > > > > > > > case MSR_LSTAR: > > > > > > > > > - if (is_noncanonical_address(data, vcpu)) > > > > > > > > > + > > > > > > > > > + /* > > > > > > > > > + * Both AMD and Intel cpus allow values which > > > > > > > > > + * are canonical in the 5 level paging mode but are not > > > > > > > > > + * canonical in the 4 level paging mode to be written > > > > > > > > > + * to the above MSRs, as long as the host CPU supports > > > > > > > > > + * 5 level paging, regardless of the state of the CR4.LA57. > > > > > > > > > + */ > > > > > > > > > + if (!__is_canonical_address(data, > > > > > > > > > + kvm_cpu_cap_has(X86_FEATURE_LA57) ? 57 : 48)) > > > > > > > > > > Please align indentation. > > > > > > > > > > Checking kvm_cpu_cap_has() is wrong. What the _host_ supports is irrelevant, > > > > > what matters is what the guest CPU supports, i.e. this should check guest CPUID. > > > > > Ah, but for safety, KVM also needs to check kvm_cpu_cap_has() to prevent faulting > > > > > on a bad load into hardware. Which means adding a "governed" feature until my > > > > > CPUID rework lands. > > Well the problem is that we passthrough these MSRs, and that means that the guest > can modify them at will, and only ucode can prevent it from doing so. > > So even if the 5 level paging is disabled in the guest's CPUID, but host supports it, > nothing will prevent the guest to write non canonical value to one of those MSRs, > and later KVM during migration or just KVM_SET_SREGS will fail. Ahh, and now I recall the discussions around the virtualization holes with LA57. > Thus I used kvm_cpu_cap_has on purpose to make KVM follow the actual ucode > behavior. I'm leaning towards having KVM do the right thing when emulation happens to be triggered. If KVM checks kvm_cpu_cap_has() instead of guest_cpu_cap_has() (looking at the future), then KVM will extend the virtualization hole to MSRs that are never passed through, and also to the nested VMX checks. Or I suppose we could add separate helpers for passthrough MSRs vs. non-passthrough, but that seems like it'd add very little value and a lot of maintenance burden. Practically speaking, outside of tests, I can't imagine the guest will ever care if there is inconsistent behavior with respect to loading non-canonical values into MSRs.
