| Message ID | 20190927214523.3376-5-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: nVMX GUEST_CR3 bug fix, and then some... | expand |
Sean Christopherson <sean.j.christopherson@intel.com> writes: > Rework vmx_set_rflags() to avoid the extra code need to handle emulation > of real mode and invalid state when unrestricted guest is disabled. The > primary reason for doing so is to avoid the call to vmx_get_rflags(), > which will incur a VMREAD when RFLAGS is not already available. When > running nested VMs, the majority of calls to vmx_set_rflags() will occur > without an associated vmx_get_rflags(), i.e. when stuffing GUEST_RFLAGS > during transitions between vmcs01 and vmcs02. > > Note, vmx_get_rflags() guarantees RFLAGS is marked available. > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > arch/x86/kvm/vmx/vmx.c | 28 ++++++++++++++++++---------- > 1 file changed, 18 insertions(+), 10 deletions(-) > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index 83fe8b02b732..814d3e6d0264 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -1426,18 +1426,26 @@ unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu) > void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags) > { > struct vcpu_vmx *vmx = to_vmx(vcpu); > - unsigned long old_rflags = vmx_get_rflags(vcpu); > + unsigned long old_rflags; > > - __set_bit(VCPU_EXREG_RFLAGS, (ulong *)&vcpu->arch.regs_avail); > - vmx->rflags = rflags; > - if (vmx->rmode.vm86_active) { > - vmx->rmode.save_rflags = rflags; > - rflags |= X86_EFLAGS_IOPL | X86_EFLAGS_VM; > + if (enable_unrestricted_guest) { > + __set_bit(VCPU_EXREG_RFLAGS, (ulong *)&vcpu->arch.regs_avail); > + > + vmx->rflags = rflags; > + vmcs_writel(GUEST_RFLAGS, rflags); > + } else { > + old_rflags = vmx_get_rflags(vcpu); > + > + vmx->rflags = rflags; > + if (vmx->rmode.vm86_active) { > + vmx->rmode.save_rflags = rflags; > + rflags |= X86_EFLAGS_IOPL | X86_EFLAGS_VM; > + } > + vmcs_writel(GUEST_RFLAGS, rflags); > + > + if ((old_rflags ^ vmx->rflags) & X86_EFLAGS_VM) > + vmx->emulation_required = emulation_required(vcpu); > } > - vmcs_writel(GUEST_RFLAGS, rflags); We're doing vmcs_writel() in both branches so it could've stayed here, right? > - > - if ((old_rflags ^ vmx->rflags) & X86_EFLAGS_VM) > - vmx->emulation_required = emulation_required(vcpu); > } > > u32 vmx_get_interrupt_shadow(struct kvm_vcpu *vcpu) Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
On Mon, Sep 30, 2019 at 10:57:17AM +0200, Vitaly Kuznetsov wrote: > Sean Christopherson <sean.j.christopherson@intel.com> writes: > > > Rework vmx_set_rflags() to avoid the extra code need to handle emulation > > of real mode and invalid state when unrestricted guest is disabled. The > > primary reason for doing so is to avoid the call to vmx_get_rflags(), > > which will incur a VMREAD when RFLAGS is not already available. When > > running nested VMs, the majority of calls to vmx_set_rflags() will occur > > without an associated vmx_get_rflags(), i.e. when stuffing GUEST_RFLAGS > > during transitions between vmcs01 and vmcs02. > > > > Note, vmx_get_rflags() guarantees RFLAGS is marked available. > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > --- > > arch/x86/kvm/vmx/vmx.c | 28 ++++++++++++++++++---------- > > 1 file changed, 18 insertions(+), 10 deletions(-) > > > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > > index 83fe8b02b732..814d3e6d0264 100644 > > --- a/arch/x86/kvm/vmx/vmx.c > > +++ b/arch/x86/kvm/vmx/vmx.c > > @@ -1426,18 +1426,26 @@ unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu) > > void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags) > > { > > struct vcpu_vmx *vmx = to_vmx(vcpu); > > - unsigned long old_rflags = vmx_get_rflags(vcpu); > > + unsigned long old_rflags; > > > > - __set_bit(VCPU_EXREG_RFLAGS, (ulong *)&vcpu->arch.regs_avail); > > - vmx->rflags = rflags; > > - if (vmx->rmode.vm86_active) { > > - vmx->rmode.save_rflags = rflags; > > - rflags |= X86_EFLAGS_IOPL | X86_EFLAGS_VM; > > + if (enable_unrestricted_guest) { > > + __set_bit(VCPU_EXREG_RFLAGS, (ulong *)&vcpu->arch.regs_avail); > > + > > + vmx->rflags = rflags; > > + vmcs_writel(GUEST_RFLAGS, rflags); > > + } else { > > + old_rflags = vmx_get_rflags(vcpu); > > + > > + vmx->rflags = rflags; > > + if (vmx->rmode.vm86_active) { > > + vmx->rmode.save_rflags = rflags; > > + rflags |= X86_EFLAGS_IOPL | X86_EFLAGS_VM; > > + } > > + vmcs_writel(GUEST_RFLAGS, rflags); > > + > > + if ((old_rflags ^ vmx->rflags) & X86_EFLAGS_VM) > > + vmx->emulation_required = emulation_required(vcpu); > > } > > - vmcs_writel(GUEST_RFLAGS, rflags); > > We're doing vmcs_writel() in both branches so it could've stayed here, right? Yes, but the resulting code is a bit ugly. emulation_required() consumes vmcs.GUEST_RFLAGS, i.e. the if statement that reads old_rflags would also need to be outside of the else{} case. This isn't too bad: if (!enable_unrestricted_guest && ((old_rflags ^ vmx->rflags) & X86_EFLAGS_VM)) vmx->emulation_required = emulation_required(vcpu); but gcc isn't smart enough to understand old_rflags won't be used if enable_unrestricted_guest, so old_rflags either needs to be tagged with uninitialized_var() or explicitly initialized in the if(){} case. Duplicating a small amount of code felt like the lesser of two evils. > > - > > - if ((old_rflags ^ vmx->rflags) & X86_EFLAGS_VM) > > - vmx->emulation_required = emulation_required(vcpu); > > } > > > > u32 vmx_get_interrupt_shadow(struct kvm_vcpu *vcpu) > > Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com> > > -- > Vitaly
Sean Christopherson <sean.j.christopherson@intel.com> writes: > On Mon, Sep 30, 2019 at 10:57:17AM +0200, Vitaly Kuznetsov wrote: >> Sean Christopherson <sean.j.christopherson@intel.com> writes: >> >> > Rework vmx_set_rflags() to avoid the extra code need to handle emulation >> > of real mode and invalid state when unrestricted guest is disabled. The >> > primary reason for doing so is to avoid the call to vmx_get_rflags(), >> > which will incur a VMREAD when RFLAGS is not already available. When >> > running nested VMs, the majority of calls to vmx_set_rflags() will occur >> > without an associated vmx_get_rflags(), i.e. when stuffing GUEST_RFLAGS >> > during transitions between vmcs01 and vmcs02. >> > >> > Note, vmx_get_rflags() guarantees RFLAGS is marked available. >> > >> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> >> > --- >> > arch/x86/kvm/vmx/vmx.c | 28 ++++++++++++++++++---------- >> > 1 file changed, 18 insertions(+), 10 deletions(-) >> > >> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c >> > index 83fe8b02b732..814d3e6d0264 100644 >> > --- a/arch/x86/kvm/vmx/vmx.c >> > +++ b/arch/x86/kvm/vmx/vmx.c >> > @@ -1426,18 +1426,26 @@ unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu) >> > void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags) >> > { >> > struct vcpu_vmx *vmx = to_vmx(vcpu); >> > - unsigned long old_rflags = vmx_get_rflags(vcpu); >> > + unsigned long old_rflags; >> > >> > - __set_bit(VCPU_EXREG_RFLAGS, (ulong *)&vcpu->arch.regs_avail); >> > - vmx->rflags = rflags; >> > - if (vmx->rmode.vm86_active) { >> > - vmx->rmode.save_rflags = rflags; >> > - rflags |= X86_EFLAGS_IOPL | X86_EFLAGS_VM; >> > + if (enable_unrestricted_guest) { >> > + __set_bit(VCPU_EXREG_RFLAGS, (ulong *)&vcpu->arch.regs_avail); >> > + >> > + vmx->rflags = rflags; >> > + vmcs_writel(GUEST_RFLAGS, rflags); >> > + } else { >> > + old_rflags = vmx_get_rflags(vcpu); >> > + >> > + vmx->rflags = rflags; >> > + if (vmx->rmode.vm86_active) { >> > + vmx->rmode.save_rflags = rflags; >> > + rflags |= X86_EFLAGS_IOPL | X86_EFLAGS_VM; >> > + } >> > + vmcs_writel(GUEST_RFLAGS, rflags); >> > + >> > + if ((old_rflags ^ vmx->rflags) & X86_EFLAGS_VM) >> > + vmx->emulation_required = emulation_required(vcpu); >> > } >> > - vmcs_writel(GUEST_RFLAGS, rflags); >> >> We're doing vmcs_writel() in both branches so it could've stayed here, right? > > Yes, but the resulting code is a bit ugly. emulation_required() consumes > vmcs.GUEST_RFLAGS, i.e. the if statement that reads old_rflags would also > need to be outside of the else{} case. > > This isn't too bad: > > if (!enable_unrestricted_guest && > ((old_rflags ^ vmx->rflags) & X86_EFLAGS_VM)) > vmx->emulation_required = emulation_required(vcpu); > > but gcc isn't smart enough to understand old_rflags won't be used if > enable_unrestricted_guest, so old_rflags either needs to be tagged with > uninitialized_var() or explicitly initialized in the if(){} case. > > Duplicating a small amount of code felt like the lesser of two evils. > I see, thanks for these additional details!
On 27/09/19 23:45, Sean Christopherson wrote: > Rework vmx_set_rflags() to avoid the extra code need to handle emulation > of real mode and invalid state when unrestricted guest is disabled. The > primary reason for doing so is to avoid the call to vmx_get_rflags(), > which will incur a VMREAD when RFLAGS is not already available. When > running nested VMs, the majority of calls to vmx_set_rflags() will occur > without an associated vmx_get_rflags(), i.e. when stuffing GUEST_RFLAGS > during transitions between vmcs01 and vmcs02. > > Note, vmx_get_rflags() guarantees RFLAGS is marked available. Slightly nicer this way: diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 8de9853d7ab6..62ab19d65efd 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -1431,9 +1431,17 @@ unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu) void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags) { struct vcpu_vmx *vmx = to_vmx(vcpu); - unsigned long old_rflags = vmx_get_rflags(vcpu); + unsigned long old_rflags; + + if (enable_unrestricted_guest) { + __set_bit(VCPU_EXREG_RFLAGS, (ulong *)&vcpu->arch.regs_avail); + vmx->rflags = rflags; + vmcs_writel(GUEST_RFLAGS, rflags); + return; + } + + old_rflags = vmx_get_rflags(vcpu); - __set_bit(VCPU_EXREG_RFLAGS, (ulong *)&vcpu->arch.regs_avail); vmx->rflags = rflags; if (vmx->rmode.vm86_active) { vmx->rmode.save_rflags = rflags; Paolo
On Wed, Oct 09, 2019 at 12:40:53PM +0200, Paolo Bonzini wrote: > On 27/09/19 23:45, Sean Christopherson wrote: > > Rework vmx_set_rflags() to avoid the extra code need to handle emulation > > of real mode and invalid state when unrestricted guest is disabled. The > > primary reason for doing so is to avoid the call to vmx_get_rflags(), > > which will incur a VMREAD when RFLAGS is not already available. When > > running nested VMs, the majority of calls to vmx_set_rflags() will occur > > without an associated vmx_get_rflags(), i.e. when stuffing GUEST_RFLAGS > > during transitions between vmcs01 and vmcs02. > > > > Note, vmx_get_rflags() guarantees RFLAGS is marked available. > > Slightly nicer this way: > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index 8de9853d7ab6..62ab19d65efd 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -1431,9 +1431,17 @@ unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu) > void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags) > { > struct vcpu_vmx *vmx = to_vmx(vcpu); > - unsigned long old_rflags = vmx_get_rflags(vcpu); > + unsigned long old_rflags; > + > + if (enable_unrestricted_guest) { > + __set_bit(VCPU_EXREG_RFLAGS, (ulong *)&vcpu->arch.regs_avail); > + vmx->rflags = rflags; > + vmcs_writel(GUEST_RFLAGS, rflags); > + return; > + } > + > + old_rflags = vmx_get_rflags(vcpu); > > - __set_bit(VCPU_EXREG_RFLAGS, (ulong *)&vcpu->arch.regs_avail); > vmx->rflags = rflags; > if (vmx->rmode.vm86_active) { > vmx->rmode.save_rflags = rflags; Works for me. Do you want me to spin a v3 to incorporate this and remove the open coding of the RIP/RSP accessors? Or are you planning on squashing the changes as you apply?
On 09/10/19 18:38, Sean Christopherson wrote: > On Wed, Oct 09, 2019 at 12:40:53PM +0200, Paolo Bonzini wrote: >> On 27/09/19 23:45, Sean Christopherson wrote: >>> Rework vmx_set_rflags() to avoid the extra code need to handle emulation >>> of real mode and invalid state when unrestricted guest is disabled. The >>> primary reason for doing so is to avoid the call to vmx_get_rflags(), >>> which will incur a VMREAD when RFLAGS is not already available. When >>> running nested VMs, the majority of calls to vmx_set_rflags() will occur >>> without an associated vmx_get_rflags(), i.e. when stuffing GUEST_RFLAGS >>> during transitions between vmcs01 and vmcs02. >>> >>> Note, vmx_get_rflags() guarantees RFLAGS is marked available. >> >> Slightly nicer this way: >> >> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c >> index 8de9853d7ab6..62ab19d65efd 100644 >> --- a/arch/x86/kvm/vmx/vmx.c >> +++ b/arch/x86/kvm/vmx/vmx.c >> @@ -1431,9 +1431,17 @@ unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu) >> void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags) >> { >> struct vcpu_vmx *vmx = to_vmx(vcpu); >> - unsigned long old_rflags = vmx_get_rflags(vcpu); >> + unsigned long old_rflags; >> + >> + if (enable_unrestricted_guest) { >> + __set_bit(VCPU_EXREG_RFLAGS, (ulong *)&vcpu->arch.regs_avail); >> + vmx->rflags = rflags; >> + vmcs_writel(GUEST_RFLAGS, rflags); >> + return; >> + } >> + >> + old_rflags = vmx_get_rflags(vcpu); >> >> - __set_bit(VCPU_EXREG_RFLAGS, (ulong *)&vcpu->arch.regs_avail); >> vmx->rflags = rflags; >> if (vmx->rmode.vm86_active) { >> vmx->rmode.save_rflags = rflags; > > Works for me. Do you want me to spin a v3 to incorporate this and remove > the open coding of the RIP/RSP accessors? Or are you planning on squashing > the changes as you apply? If it's okay for you I can squash it. Paolo
On Wed, Oct 09, 2019 at 10:59:10PM +0200, Paolo Bonzini wrote: > On 09/10/19 18:38, Sean Christopherson wrote: > > On Wed, Oct 09, 2019 at 12:40:53PM +0200, Paolo Bonzini wrote: > >> On 27/09/19 23:45, Sean Christopherson wrote: > >>> Rework vmx_set_rflags() to avoid the extra code need to handle emulation > >>> of real mode and invalid state when unrestricted guest is disabled. The > >>> primary reason for doing so is to avoid the call to vmx_get_rflags(), > >>> which will incur a VMREAD when RFLAGS is not already available. When > >>> running nested VMs, the majority of calls to vmx_set_rflags() will occur > >>> without an associated vmx_get_rflags(), i.e. when stuffing GUEST_RFLAGS > >>> during transitions between vmcs01 and vmcs02. > >>> > >>> Note, vmx_get_rflags() guarantees RFLAGS is marked available. > >> > >> Slightly nicer this way: > >> > >> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > >> index 8de9853d7ab6..62ab19d65efd 100644 > >> --- a/arch/x86/kvm/vmx/vmx.c > >> +++ b/arch/x86/kvm/vmx/vmx.c > >> @@ -1431,9 +1431,17 @@ unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu) > >> void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags) > >> { > >> struct vcpu_vmx *vmx = to_vmx(vcpu); > >> - unsigned long old_rflags = vmx_get_rflags(vcpu); > >> + unsigned long old_rflags; > >> + > >> + if (enable_unrestricted_guest) { > >> + __set_bit(VCPU_EXREG_RFLAGS, (ulong *)&vcpu->arch.regs_avail); > >> + vmx->rflags = rflags; > >> + vmcs_writel(GUEST_RFLAGS, rflags); > >> + return; > >> + } > >> + > >> + old_rflags = vmx_get_rflags(vcpu); > >> > >> - __set_bit(VCPU_EXREG_RFLAGS, (ulong *)&vcpu->arch.regs_avail); > >> vmx->rflags = rflags; > >> if (vmx->rmode.vm86_active) { > >> vmx->rmode.save_rflags = rflags; > > > > Works for me. Do you want me to spin a v3 to incorporate this and remove > > the open coding of the RIP/RSP accessors? Or are you planning on squashing > > the changes as you apply? > > If it's okay for you I can squash it. Squash away.
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 83fe8b02b732..814d3e6d0264 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -1426,18 +1426,26 @@ unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu) void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags) { struct vcpu_vmx *vmx = to_vmx(vcpu); - unsigned long old_rflags = vmx_get_rflags(vcpu); + unsigned long old_rflags; - __set_bit(VCPU_EXREG_RFLAGS, (ulong *)&vcpu->arch.regs_avail); - vmx->rflags = rflags; - if (vmx->rmode.vm86_active) { - vmx->rmode.save_rflags = rflags; - rflags |= X86_EFLAGS_IOPL | X86_EFLAGS_VM; + if (enable_unrestricted_guest) { + __set_bit(VCPU_EXREG_RFLAGS, (ulong *)&vcpu->arch.regs_avail); + + vmx->rflags = rflags; + vmcs_writel(GUEST_RFLAGS, rflags); + } else { + old_rflags = vmx_get_rflags(vcpu); + + vmx->rflags = rflags; + if (vmx->rmode.vm86_active) { + vmx->rmode.save_rflags = rflags; + rflags |= X86_EFLAGS_IOPL | X86_EFLAGS_VM; + } + vmcs_writel(GUEST_RFLAGS, rflags); + + if ((old_rflags ^ vmx->rflags) & X86_EFLAGS_VM) + vmx->emulation_required = emulation_required(vcpu); } - vmcs_writel(GUEST_RFLAGS, rflags); - - if ((old_rflags ^ vmx->rflags) & X86_EFLAGS_VM) - vmx->emulation_required = emulation_required(vcpu); } u32 vmx_get_interrupt_shadow(struct kvm_vcpu *vcpu)
Rework vmx_set_rflags() to avoid the extra code need to handle emulation of real mode and invalid state when unrestricted guest is disabled. The primary reason for doing so is to avoid the call to vmx_get_rflags(), which will incur a VMREAD when RFLAGS is not already available. When running nested VMs, the majority of calls to vmx_set_rflags() will occur without an associated vmx_get_rflags(), i.e. when stuffing GUEST_RFLAGS during transitions between vmcs01 and vmcs02. Note, vmx_get_rflags() guarantees RFLAGS is marked available. Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/kvm/vmx/vmx.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-)