| Message ID | 20181113154446.124812-1-liran.alon@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: nVMX: vmcs12 revision_id is always VMCS12_REVISION even when copied from eVMCS | expand |
Liran Alon <liran.alon@oracle.com> writes: > vmcs12 represents the per-CPU cache of L1 active vmcs12. > > This cache can be loaded by one of the following: > 1) Guest making a vmcs12 active by exeucting VMPTRLD > 2) Guest specifying eVMCS in VP assist page and executing > VMLAUNCH/VMRESUME. > > Either way, vmcs12 should have revision_id of VMCS12_REVISION. > Which is not equal to eVMCS revision_id which specifies used > VersionNumber of eVMCS struct (e.g. KVM_EVMCS_VERSION). > > Specifically, this causes an issue in restoring a nested VM state > because vmx_set_nested_state() verifies that vmcs12->revision_id > is equal to VMCS12_REVISION which was not true in case vmcs12 > was populated from an eVMCS by vmx_get_nested_state() which calls > copy_enlightened_to_vmcs12(). > > Reviewed-by: Darren Kenny <darren.kenny@oracle.com> > Signed-off-by: Liran Alon <liran.alon@oracle.com> > --- > arch/x86/kvm/vmx.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 4555077d69ce..6e92624fe8d6 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -8664,8 +8664,6 @@ static int copy_enlightened_to_vmcs12(struct vcpu_vmx *vmx) > struct vmcs12 *vmcs12 = vmx->nested.cached_vmcs12; > struct hv_enlightened_vmcs *evmcs = vmx->nested.hv_evmcs; > > - vmcs12->hdr.revision_id = evmcs->revision_id; > - > /* HV_VMX_ENLIGHTENED_CLEAN_FIELD_NONE */ > vmcs12->tpr_threshold = evmcs->tpr_threshold; > vmcs12->guest_rip = evmcs->guest_rip; > @@ -9390,9 +9388,11 @@ static int nested_vmx_handle_enlightened_vmptrld(struct kvm_vcpu *vcpu, > * present in struct hv_enlightened_vmcs, ...). Make sure there > * are no leftovers. > */ > - if (from_launch) > - memset(vmx->nested.cached_vmcs12, 0, > - sizeof(*vmx->nested.cached_vmcs12)); > + if (from_launch) { > + struct vmcs12 *vmcs12 = get_vmcs12(vcpu); > + memset(vmcs12, 0, sizeof(*vmcs12)); > + vmcs12->hdr.revision_id = VMCS12_REVISION; > + } > > } > return 1; I was wondering how evmcs selftest was passing before this but then I remembered the Hyper-V bug (which puts VMCS revision from IA32_VMX_BASIC_MSR - where we have VMCS12_REVISION and not KVM_EVMCS_VERSION) so 'evmcs->revision_id' in this context is actually VMCS12_REVISION and there's no change post-patch. We, however, will need to fix the selftest after we start allowing KVM_EVMCS_VERSION as it currently mocks erroneous Hyper-V behavior :-) Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com> Thanks!
On 13/11/18 17:37, Vitaly Kuznetsov wrote: > I was wondering how evmcs selftest was passing before this but then I > remembered the Hyper-V bug (which puts VMCS revision from > IA32_VMX_BASIC_MSR - where we have VMCS12_REVISION and not > KVM_EVMCS_VERSION) so 'evmcs->revision_id' in this context is actually > VMCS12_REVISION and there's no change post-patch. > > We, however, will need to fix the selftest after we start allowing > KVM_EVMCS_VERSION as it currently mocks erroneous Hyper-V behavior :-) Is this a Hyper-V bug, or a TLFS mistake? Queuing the patch, anyway. Thanks! Paolo
> On 25 Nov 2018, at 19:59, Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 13/11/18 17:37, Vitaly Kuznetsov wrote: >> I was wondering how evmcs selftest was passing before this but then I >> remembered the Hyper-V bug (which puts VMCS revision from >> IA32_VMX_BASIC_MSR - where we have VMCS12_REVISION and not >> KVM_EVMCS_VERSION) so 'evmcs->revision_id' in this context is actually >> VMCS12_REVISION and there's no change post-patch. >> >> We, however, will need to fix the selftest after we start allowing >> KVM_EVMCS_VERSION as it currently mocks erroneous Hyper-V behavior :-) > > Is this a Hyper-V bug, or a TLFS mistake? > > Queuing the patch, anyway. Thanks! > > Paolo We have later discussed this with Microsoft folks. They have confirmed this is a Hyper-V bug. They plan to fix this and told us that we can detect if guest is buggy by examining HV_X64_MSR_GUEST_OS_ID and check if guest is Windows NT (VendorId=1 and OsId=4) and BuildNumber<18000. -Liran
On 25/11/18 22:44, Liran Alon wrote: >> Is this a Hyper-V bug, or a TLFS mistake? >> >> Queuing the patch, anyway. Thanks! >> >> Paolo > We have later discussed this with Microsoft folks. They have confirmed this is a Hyper-V bug. > They plan to fix this and told us that we can detect if guest is buggy by examining > HV_X64_MSR_GUEST_OS_ID and check if guest is Windows NT (VendorId=1 and OsId=4) and BuildNumber<18000. Yup, it's all clear now after I went through the whole queue. Thanks, Paolo
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 4555077d69ce..6e92624fe8d6 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -8664,8 +8664,6 @@ static int copy_enlightened_to_vmcs12(struct vcpu_vmx *vmx) struct vmcs12 *vmcs12 = vmx->nested.cached_vmcs12; struct hv_enlightened_vmcs *evmcs = vmx->nested.hv_evmcs; - vmcs12->hdr.revision_id = evmcs->revision_id; - /* HV_VMX_ENLIGHTENED_CLEAN_FIELD_NONE */ vmcs12->tpr_threshold = evmcs->tpr_threshold; vmcs12->guest_rip = evmcs->guest_rip; @@ -9390,9 +9388,11 @@ static int nested_vmx_handle_enlightened_vmptrld(struct kvm_vcpu *vcpu, * present in struct hv_enlightened_vmcs, ...). Make sure there * are no leftovers. */ - if (from_launch) - memset(vmx->nested.cached_vmcs12, 0, - sizeof(*vmx->nested.cached_vmcs12)); + if (from_launch) { + struct vmcs12 *vmcs12 = get_vmcs12(vcpu); + memset(vmcs12, 0, sizeof(*vmcs12)); + vmcs12->hdr.revision_id = VMCS12_REVISION; + } } return 1;