KVM: VMX: check flexpriority module param when setting virt apic mode
diff mbox series

Message ID 20181003174207.26418-1-sean.j.christopherson@intel.com
State New
Headers show
Series
  • KVM: VMX: check flexpriority module param when setting virt apic mode
Related show

Commit Message

Sean Christopherson Oct. 3, 2018, 5:42 p.m. UTC
Query the flexpriority module param instead of simply checking if
flexpriority is supported by the cpu when checking whether or not VMCS
fields need to be updated in response to the guest changing its
APIC_BASE MSR control bits.  This avoids multiple unnecessary VMREADs
and a VMWRITE if flexpriority is disabled via its module param and the
cpu doesn't support X2APIC virtualization.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---

This is basically fixup for commit 76e97cc35522 ("KVM: VMX: check for
existence of secondary exec controls before accessing") in
kvm.git/queue.  The aforementioned commit came from a series with a
prior patch that removed the module param, but the prior patch was
ultimately rejected resulting in this partially wrong code.

 arch/x86/kvm/vmx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jim Mattson Oct. 3, 2018, 6:32 p.m. UTC | #1
On Wed, Oct 3, 2018 at 10:42 AM, Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
> Query the flexpriority module param instead of simply checking if
> flexpriority is supported by the cpu when checking whether or not VMCS
> fields need to be updated in response to the guest changing its
> APIC_BASE MSR control bits.  This avoids multiple unnecessary VMREADs
> and a VMWRITE if flexpriority is disabled via its module param and the
> cpu doesn't support X2APIC virtualization.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>
> This is basically fixup for commit 76e97cc35522 ("KVM: VMX: check for
> existence of secondary exec controls before accessing") in
> kvm.git/queue.  The aforementioned commit came from a series with a
> prior patch that removed the module param, but the prior patch was
> ultimately rejected resulting in this partially wrong code.
>
>  arch/x86/kvm/vmx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index c7ae8ea87bc4..612fd17be635 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -10223,7 +10223,7 @@ static void vmx_set_virtual_apic_mode(struct kvm_vcpu *vcpu)
>         if (!lapic_in_kernel(vcpu))
>                 return;
>
> -       if (!cpu_has_vmx_flexpriority() &&
> +       if (!flexpriority_enabled &&
>             !cpu_has_vmx_virtualize_x2apic_mode())
>                 return;

I think it would be better if cpu_has_vmx_flexpriority() returned
false when flexpriority_enabled is false.
Sean Christopherson Oct. 3, 2018, 6:39 p.m. UTC | #2
On Wed, Oct 03, 2018 at 11:32:33AM -0700, Jim Mattson wrote:
> On Wed, Oct 3, 2018 at 10:42 AM, Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> > Query the flexpriority module param instead of simply checking if
> > flexpriority is supported by the cpu when checking whether or not VMCS
> > fields need to be updated in response to the guest changing its
> > APIC_BASE MSR control bits.  This avoids multiple unnecessary VMREADs
> > and a VMWRITE if flexpriority is disabled via its module param and the
> > cpu doesn't support X2APIC virtualization.
> >
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > ---
> >
> > This is basically fixup for commit 76e97cc35522 ("KVM: VMX: check for
> > existence of secondary exec controls before accessing") in
> > kvm.git/queue.  The aforementioned commit came from a series with a
> > prior patch that removed the module param, but the prior patch was
> > ultimately rejected resulting in this partially wrong code.
> >
> >  arch/x86/kvm/vmx.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > index c7ae8ea87bc4..612fd17be635 100644
> > --- a/arch/x86/kvm/vmx.c
> > +++ b/arch/x86/kvm/vmx.c
> > @@ -10223,7 +10223,7 @@ static void vmx_set_virtual_apic_mode(struct kvm_vcpu *vcpu)
> >         if (!lapic_in_kernel(vcpu))
> >                 return;
> >
> > -       if (!cpu_has_vmx_flexpriority() &&
> > +       if (!flexpriority_enabled &&
> >             !cpu_has_vmx_virtualize_x2apic_mode())
> >                 return;
> 
> I think it would be better if cpu_has_vmx_flexpriority() returned
> false when flexpriority_enabled is false.

Ha, I had that all coded up but decided against it because the
convention used by other module params is to not adjust cpu_has_*().
Paolo Bonzini Oct. 4, 2018, 11:58 a.m. UTC | #3
On 03/10/2018 20:39, Sean Christopherson wrote:
>>> -       if (!cpu_has_vmx_flexpriority() &&
>>> +       if (!flexpriority_enabled &&
>>>             !cpu_has_vmx_virtualize_x2apic_mode())
>>>                 return;
>> I think it would be better if cpu_has_vmx_flexpriority() returned
>> false when flexpriority_enabled is false.
> Ha, I had that all coded up but decided against it because the
> convention used by other module params is to not adjust cpu_has_*().

Yeah, for now let's stick to the minimum.  It's a common idiom to adjust
the variable instead of the cpu_has_*() because then the processor
capabilities show up in /sys/kernel/module/kvm_intel/parameters.  It
requires more care but it's also more user friendly.

Paolo

Patch
diff mbox series

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index c7ae8ea87bc4..612fd17be635 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -10223,7 +10223,7 @@  static void vmx_set_virtual_apic_mode(struct kvm_vcpu *vcpu)
 	if (!lapic_in_kernel(vcpu))
 		return;
 
-	if (!cpu_has_vmx_flexpriority() &&
+	if (!flexpriority_enabled &&
 	    !cpu_has_vmx_virtualize_x2apic_mode())
 		return;