| Message ID | 20180913124319.49663-3-liran.alon@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | : KVM: nVMX: Fix bugs in MPX VMX controls emulation | expand |
On Thu, Sep 13, 2018 at 03:43:19PM +0300, Liran Alon wrote: > L2 IA32_BNDCFGS should be updated with vmcs12->guest_bndcfgs only > when VM_ENTRY_LOAD_BNDCFGS is specified in vmcs12->vm_entry_controls. > > Otherwise, L2 IA32_BNDCFGS should be set to vmcs01->guest_bndcfgs which > is L1 IA32_BNDCFGS. > > Reviewed-by: Nikita Leshchenko <nikita.leshchenko@oracle.com> > Reviewed-by: Darren Kenny <darren.kenny@oracle.com> > Signed-off-by: Liran Alon <liran.alon@oracle.com> > --- > arch/x86/kvm/vmx.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 6a82e603f2c5..3259775814d0 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -856,6 +856,7 @@ struct nested_vmx { > > /* to migrate it to L2 if VM_ENTRY_LOAD_DEBUG_CONTROLS is off */ > u64 vmcs01_debugctl; > + u64 vmcs01_guest_bndcfgs; > > u16 vpid02; > u16 last_vpid; > @@ -12028,8 +12029,13 @@ static void prepare_vmcs02_full(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12) > > set_cr4_guest_host_mask(vmx); > > - if (vmx_mpx_supported()) > - vmcs_write64(GUEST_BNDCFGS, vmcs12->guest_bndcfgs); > + if (vmx_mpx_supported()) { > + if (vmx->nested.nested_run_pending && > + (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_BNDCFGS)) > + vmcs_write64(GUEST_BNDCFGS, vmcs12->guest_bndcfgs); > + else > + vmcs_write64(GUEST_BNDCFGS, vmx->nested.vmcs01_guest_bndcfgs); > + } > > if (enable_vpid) { > u16 vmcs02_vpid; > @@ -12597,6 +12603,8 @@ static int enter_vmx_non_root_mode(struct kvm_vcpu *vcpu, u32 *exit_qual) > > if (!(vmcs12->vm_entry_controls & VM_ENTRY_LOAD_DEBUG_CONTROLS)) > vmx->nested.vmcs01_debugctl = vmcs_read64(GUEST_IA32_DEBUGCTL); > + if (!(vmcs12->vm_entry_controls & VM_ENTRY_LOAD_BNDCFGS)) > + vmx->nested.vmcs01_guest_bndcfgs = vmcs_read64(GUEST_BNDCFGS); This needs to be wrapped with vmx_mpx_supported() else you'll VMREAD a non-existent field. > > vmx_switch_vmcs(vcpu, &vmx->nested.vmcs02); > vmx_segment_cache_clear(vmx); > -- > 2.16.1 >
> On 13 Sep 2018, at 16:41, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > On Thu, Sep 13, 2018 at 03:43:19PM +0300, Liran Alon wrote: >> L2 IA32_BNDCFGS should be updated with vmcs12->guest_bndcfgs only >> when VM_ENTRY_LOAD_BNDCFGS is specified in vmcs12->vm_entry_controls. >> >> Otherwise, L2 IA32_BNDCFGS should be set to vmcs01->guest_bndcfgs which >> is L1 IA32_BNDCFGS. >> >> Reviewed-by: Nikita Leshchenko <nikita.leshchenko@oracle.com> >> Reviewed-by: Darren Kenny <darren.kenny@oracle.com> >> Signed-off-by: Liran Alon <liran.alon@oracle.com> >> --- >> arch/x86/kvm/vmx.c | 12 ++++++++++-- >> 1 file changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >> index 6a82e603f2c5..3259775814d0 100644 >> --- a/arch/x86/kvm/vmx.c >> +++ b/arch/x86/kvm/vmx.c >> @@ -856,6 +856,7 @@ struct nested_vmx { >> >> /* to migrate it to L2 if VM_ENTRY_LOAD_DEBUG_CONTROLS is off */ >> u64 vmcs01_debugctl; >> + u64 vmcs01_guest_bndcfgs; >> >> u16 vpid02; >> u16 last_vpid; >> @@ -12028,8 +12029,13 @@ static void prepare_vmcs02_full(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12) >> >> set_cr4_guest_host_mask(vmx); >> >> - if (vmx_mpx_supported()) >> - vmcs_write64(GUEST_BNDCFGS, vmcs12->guest_bndcfgs); >> + if (vmx_mpx_supported()) { >> + if (vmx->nested.nested_run_pending && >> + (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_BNDCFGS)) >> + vmcs_write64(GUEST_BNDCFGS, vmcs12->guest_bndcfgs); >> + else >> + vmcs_write64(GUEST_BNDCFGS, vmx->nested.vmcs01_guest_bndcfgs); >> + } >> >> if (enable_vpid) { >> u16 vmcs02_vpid; >> @@ -12597,6 +12603,8 @@ static int enter_vmx_non_root_mode(struct kvm_vcpu *vcpu, u32 *exit_qual) >> >> if (!(vmcs12->vm_entry_controls & VM_ENTRY_LOAD_DEBUG_CONTROLS)) >> vmx->nested.vmcs01_debugctl = vmcs_read64(GUEST_IA32_DEBUGCTL); >> + if (!(vmcs12->vm_entry_controls & VM_ENTRY_LOAD_BNDCFGS)) >> + vmx->nested.vmcs01_guest_bndcfgs = vmcs_read64(GUEST_BNDCFGS); > > This needs to be wrapped with vmx_mpx_supported() else you'll VMREAD a > non-existent field. Oops. You are right. Will fix in v2. Thanks, -Liran > >> >> vmx_switch_vmcs(vcpu, &vmx->nested.vmcs02); >> vmx_segment_cache_clear(vmx); >> -- >> 2.16.1 >>
On Thu, Sep 13, 2018 at 04:45:01PM +0300, Liran Alon wrote: > > > > On 13 Sep 2018, at 16:41, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > > > On Thu, Sep 13, 2018 at 03:43:19PM +0300, Liran Alon wrote: > >> L2 IA32_BNDCFGS should be updated with vmcs12->guest_bndcfgs only > >> when VM_ENTRY_LOAD_BNDCFGS is specified in vmcs12->vm_entry_controls. > >> > >> Otherwise, L2 IA32_BNDCFGS should be set to vmcs01->guest_bndcfgs which > >> is L1 IA32_BNDCFGS. > >> > >> Reviewed-by: Nikita Leshchenko <nikita.leshchenko@oracle.com> > >> Reviewed-by: Darren Kenny <darren.kenny@oracle.com> > >> Signed-off-by: Liran Alon <liran.alon@oracle.com> > >> --- > >> arch/x86/kvm/vmx.c | 12 ++++++++++-- > >> 1 file changed, 10 insertions(+), 2 deletions(-) > >> > >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > >> index 6a82e603f2c5..3259775814d0 100644 > >> --- a/arch/x86/kvm/vmx.c > >> +++ b/arch/x86/kvm/vmx.c > >> @@ -856,6 +856,7 @@ struct nested_vmx { > >> > >> /* to migrate it to L2 if VM_ENTRY_LOAD_DEBUG_CONTROLS is off */ > >> u64 vmcs01_debugctl; > >> + u64 vmcs01_guest_bndcfgs; > >> > >> u16 vpid02; > >> u16 last_vpid; > >> @@ -12028,8 +12029,13 @@ static void prepare_vmcs02_full(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12) > >> > >> set_cr4_guest_host_mask(vmx); > >> > >> - if (vmx_mpx_supported()) > >> - vmcs_write64(GUEST_BNDCFGS, vmcs12->guest_bndcfgs); > >> + if (vmx_mpx_supported()) { > >> + if (vmx->nested.nested_run_pending && > >> + (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_BNDCFGS)) > >> + vmcs_write64(GUEST_BNDCFGS, vmcs12->guest_bndcfgs); > >> + else > >> + vmcs_write64(GUEST_BNDCFGS, vmx->nested.vmcs01_guest_bndcfgs); > >> + } > >> > >> if (enable_vpid) { > >> u16 vmcs02_vpid; > >> @@ -12597,6 +12603,8 @@ static int enter_vmx_non_root_mode(struct kvm_vcpu *vcpu, u32 *exit_qual) > >> > >> if (!(vmcs12->vm_entry_controls & VM_ENTRY_LOAD_DEBUG_CONTROLS)) > >> vmx->nested.vmcs01_debugctl = vmcs_read64(GUEST_IA32_DEBUGCTL); > >> + if (!(vmcs12->vm_entry_controls & VM_ENTRY_LOAD_BNDCFGS)) > >> + vmx->nested.vmcs01_guest_bndcfgs = vmcs_read64(GUEST_BNDCFGS); > > > > This needs to be wrapped with vmx_mpx_supported() else you'll VMREAD a > > non-existent field. > > Oops. You are right. Will fix in v2. After reading through the existing code a bit more, I think all calls to vmx_mpx_supported() should be replaced with kvm_mpx_supported(). The KVM version returns true iff MPX is enabled in the host, and was added by commit a87036add092 ("KVM: x86: disable MPX if host did not enable MPX XSAVE features").
> On 13 Sep 2018, at 16:59, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > After reading through the existing code a bit more, I think all calls > to vmx_mpx_supported() should be replaced with kvm_mpx_supported(). > The KVM version returns true iff MPX is enabled in the host, and was > added by commit a87036add092 ("KVM: x86: disable MPX if host did not > enable MPX XSAVE features"). I agree. This should also include the call kvm_x86_ops->mpx_supported() inside kvm_init_msr_list(). Do you think it is preferable to do your suggested patch before or after the patch discussed in this email thread? -Liran
On Fri, Sep 14, 2018 at 02:14:49AM +0300, Liran Alon wrote: > > > > On 13 Sep 2018, at 16:59, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > > > After reading through the existing code a bit more, I think all calls > > to vmx_mpx_supported() should be replaced with kvm_mpx_supported(). > > The KVM version returns true iff MPX is enabled in the host, and was > > added by commit a87036add092 ("KVM: x86: disable MPX if host did not > > enable MPX XSAVE features"). > > I agree. > This should also include the call kvm_x86_ops->mpx_supported() inside kvm_init_msr_list(). > Do you think it is preferable to do your suggested patch before or after the patch discussed in this email thread? Probably before? Only because it seems weird to knowingly do the wrong thing.
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 6a82e603f2c5..3259775814d0 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -856,6 +856,7 @@ struct nested_vmx { /* to migrate it to L2 if VM_ENTRY_LOAD_DEBUG_CONTROLS is off */ u64 vmcs01_debugctl; + u64 vmcs01_guest_bndcfgs; u16 vpid02; u16 last_vpid; @@ -12028,8 +12029,13 @@ static void prepare_vmcs02_full(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12) set_cr4_guest_host_mask(vmx); - if (vmx_mpx_supported()) - vmcs_write64(GUEST_BNDCFGS, vmcs12->guest_bndcfgs); + if (vmx_mpx_supported()) { + if (vmx->nested.nested_run_pending && + (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_BNDCFGS)) + vmcs_write64(GUEST_BNDCFGS, vmcs12->guest_bndcfgs); + else + vmcs_write64(GUEST_BNDCFGS, vmx->nested.vmcs01_guest_bndcfgs); + } if (enable_vpid) { u16 vmcs02_vpid; @@ -12597,6 +12603,8 @@ static int enter_vmx_non_root_mode(struct kvm_vcpu *vcpu, u32 *exit_qual) if (!(vmcs12->vm_entry_controls & VM_ENTRY_LOAD_DEBUG_CONTROLS)) vmx->nested.vmcs01_debugctl = vmcs_read64(GUEST_IA32_DEBUGCTL); + if (!(vmcs12->vm_entry_controls & VM_ENTRY_LOAD_BNDCFGS)) + vmx->nested.vmcs01_guest_bndcfgs = vmcs_read64(GUEST_BNDCFGS); vmx_switch_vmcs(vcpu, &vmx->nested.vmcs02); vmx_segment_cache_clear(vmx);