[PATCH] KVM: SVM: Inhibit APIC virtualization for X2APIC guest

[PATCH] KVM: SVM: Inhibit APIC virtualization for X2APIC guest

[Oliver Upton]

The AVIC does not support guest use of the x2APIC interface. Currently,
KVM simply chooses to squash the x2APIC feature in the guest's CPUID
If the AVIC is enabled. Doing so prevents KVM from running a guest
with greater than 255 vCPUs, as such a guest necessitates the use
of the x2APIC interface.

Instead, inhibit AVIC enablement on a per-VM basis whenever the x2APIC
feature is set in the guest's CPUID. Since this changes the behavior of
KVM as seen by userspace, add a module parameter, avic_per_vm, to opt-in
for the new behavior. If this parameter is set, report x2APIC as
available on KVM_GET_SUPPORTED_CPUID. Without opt-in, continue to
suppress x2APIC from the guest's CPUID.

Signed-off-by: Oliver Upton <oupton@google.com>
Reviewed-by: Jim Mattson <jmattson@google.com>
---

 Parent commit: a93236fcbe1d ("KVM: s390: rstify new ioctls in api.rst")

 arch/x86/include/asm/kvm_host.h |  1 +
 arch/x86/kvm/svm.c              | 19 ++++++++++++++++---
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 98959e8cd448..9d40132a3ae2 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -890,6 +890,7 @@ enum kvm_irqchip_mode {
 #define APICV_INHIBIT_REASON_NESTED     2
 #define APICV_INHIBIT_REASON_IRQWIN     3
 #define APICV_INHIBIT_REASON_PIT_REINJ  4
+#define APICV_INHIBIT_REASON_X2APIC	5
 
 struct kvm_arch {
 	unsigned long n_used_mmu_pages;
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index ad3f5b178a03..95c03c75f51a 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -382,6 +382,10 @@ module_param(sev, int, 0444);
 static bool __read_mostly dump_invalid_vmcb = 0;
 module_param(dump_invalid_vmcb, bool, 0644);
 
+/* enable/disable opportunistic use of the AVIC on a per-VM basis */
+static bool __read_mostly avic_per_vm;
+module_param(avic_per_vm, bool, 0444);
+
 static u8 rsm_ins_bytes[] = "\x0f\xaa";
 
 static void svm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0);
@@ -6027,7 +6031,15 @@ static void svm_cpuid_update(struct kvm_vcpu *vcpu)
 	if (!kvm_vcpu_apicv_active(vcpu))
 		return;
 
-	guest_cpuid_clear(vcpu, X86_FEATURE_X2APIC);
+	/*
+	 * AVIC does not work with an x2APIC mode guest. If the X2APIC feature
+	 * is exposed to the guest, disable AVIC.
+	 */
+	if (avic_per_vm && guest_cpuid_has(vcpu, X86_FEATURE_X2APIC))
+		kvm_request_apicv_update(vcpu->kvm, false,
+					 APICV_INHIBIT_REASON_X2APIC);
+	else
+		guest_cpuid_clear(vcpu, X86_FEATURE_X2APIC);
 
 	/*
 	 * Currently, AVIC does not work with nested virtualization.
@@ -6044,7 +6056,7 @@ static void svm_set_supported_cpuid(u32 func, struct kvm_cpuid_entry2 *entry)
 {
 	switch (func) {
 	case 0x1:
-		if (avic)
+		if (avic && !avic_per_vm)
 			entry->ecx &= ~F(X2APIC);
 		break;
 	case 0x80000001:
@@ -7370,7 +7382,8 @@ static bool svm_check_apicv_inhibit_reasons(ulong bit)
 			  BIT(APICV_INHIBIT_REASON_HYPERV) |
 			  BIT(APICV_INHIBIT_REASON_NESTED) |
 			  BIT(APICV_INHIBIT_REASON_IRQWIN) |
-			  BIT(APICV_INHIBIT_REASON_PIT_REINJ);
+			  BIT(APICV_INHIBIT_REASON_PIT_REINJ) |
+			  BIT(APICV_INHIBIT_REASON_X2APIC);
 
 	return supported & BIT(bit);
 }
-- 
2.25.1.481.gfbce0eb801-goog

Re: [PATCH] KVM: SVM: Inhibit APIC virtualization for X2APIC guest

[Paolo Bonzini]

On 28/02/20 01:35, Oliver Upton wrote:
> The AVIC does not support guest use of the x2APIC interface. Currently,
> KVM simply chooses to squash the x2APIC feature in the guest's CPUID
> If the AVIC is enabled. Doing so prevents KVM from running a guest
> with greater than 255 vCPUs, as such a guest necessitates the use
> of the x2APIC interface.
> 
> Instead, inhibit AVIC enablement on a per-VM basis whenever the x2APIC
> feature is set in the guest's CPUID. Since this changes the behavior of
> KVM as seen by userspace, add a module parameter, avic_per_vm, to opt-in
> for the new behavior. If this parameter is set, report x2APIC as
> available on KVM_GET_SUPPORTED_CPUID. Without opt-in, continue to
> suppress x2APIC from the guest's CPUID.
> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> Reviewed-by: Jim Mattson <jmattson@google.com>

Since AVIC is not enabled by default, let's do this always and flip the
default to avic=1 in 5.7.  People using avic=1 will have to disable
x2apic manually instead but the default will be the same in practice
(AVIC not enabled).  And then we can figure out:

- how to do emulation of x2apic when avic is enabled (so it will cause
vmexits but still use the AVIC for e.g. assigned devices)

- a PV CPUID leaf to tell <=255 vCPUs on AMD virtualization _not_ to use
x2apic.

Paolo

> ---
> 
>  Parent commit: a93236fcbe1d ("KVM: s390: rstify new ioctls in api.rst")
> 
>  arch/x86/include/asm/kvm_host.h |  1 +
>  arch/x86/kvm/svm.c              | 19 ++++++++++++++++---
>  2 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 98959e8cd448..9d40132a3ae2 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -890,6 +890,7 @@ enum kvm_irqchip_mode {
>  #define APICV_INHIBIT_REASON_NESTED     2
>  #define APICV_INHIBIT_REASON_IRQWIN     3
>  #define APICV_INHIBIT_REASON_PIT_REINJ  4
> +#define APICV_INHIBIT_REASON_X2APIC	5
>  
>  struct kvm_arch {
>  	unsigned long n_used_mmu_pages;
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index ad3f5b178a03..95c03c75f51a 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -382,6 +382,10 @@ module_param(sev, int, 0444);
>  static bool __read_mostly dump_invalid_vmcb = 0;
>  module_param(dump_invalid_vmcb, bool, 0644);
>  
> +/* enable/disable opportunistic use of the AVIC on a per-VM basis */
> +static bool __read_mostly avic_per_vm;
> +module_param(avic_per_vm, bool, 0444);
> +
>  static u8 rsm_ins_bytes[] = "\x0f\xaa";
>  
>  static void svm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0);
> @@ -6027,7 +6031,15 @@ static void svm_cpuid_update(struct kvm_vcpu *vcpu)
>  	if (!kvm_vcpu_apicv_active(vcpu))
>  		return;
>  
> -	guest_cpuid_clear(vcpu, X86_FEATURE_X2APIC);
> +	/*
> +	 * AVIC does not work with an x2APIC mode guest. If the X2APIC feature
> +	 * is exposed to the guest, disable AVIC.
> +	 */
> +	if (avic_per_vm && guest_cpuid_has(vcpu, X86_FEATURE_X2APIC))
> +		kvm_request_apicv_update(vcpu->kvm, false,
> +					 APICV_INHIBIT_REASON_X2APIC);
> +	else
> +		guest_cpuid_clear(vcpu, X86_FEATURE_X2APIC);
>  
>  	/*
>  	 * Currently, AVIC does not work with nested virtualization.
> @@ -6044,7 +6056,7 @@ static void svm_set_supported_cpuid(u32 func, struct kvm_cpuid_entry2 *entry)
>  {
>  	switch (func) {
>  	case 0x1:
> -		if (avic)
> +		if (avic && !avic_per_vm)
>  			entry->ecx &= ~F(X2APIC);
>  		break;
>  	case 0x80000001:
> @@ -7370,7 +7382,8 @@ static bool svm_check_apicv_inhibit_reasons(ulong bit)
>  			  BIT(APICV_INHIBIT_REASON_HYPERV) |
>  			  BIT(APICV_INHIBIT_REASON_NESTED) |
>  			  BIT(APICV_INHIBIT_REASON_IRQWIN) |
> -			  BIT(APICV_INHIBIT_REASON_PIT_REINJ);
> +			  BIT(APICV_INHIBIT_REASON_PIT_REINJ) |
> +			  BIT(APICV_INHIBIT_REASON_X2APIC);
>  
>  	return supported & BIT(bit);
>  }
> 

Re: [PATCH] KVM: SVM: Inhibit APIC virtualization for X2APIC guest

[Oliver Upton]

Hi Paolo,

On Fri, Feb 28, 2020 at 09:01:11AM +0100, Paolo Bonzini wrote:
> On 28/02/20 01:35, Oliver Upton wrote:
> > The AVIC does not support guest use of the x2APIC interface. Currently,
> > KVM simply chooses to squash the x2APIC feature in the guest's CPUID
> > If the AVIC is enabled. Doing so prevents KVM from running a guest
> > with greater than 255 vCPUs, as such a guest necessitates the use
> > of the x2APIC interface.
> > 
> > Instead, inhibit AVIC enablement on a per-VM basis whenever the x2APIC
> > feature is set in the guest's CPUID. Since this changes the behavior of
> > KVM as seen by userspace, add a module parameter, avic_per_vm, to opt-in
> > for the new behavior. If this parameter is set, report x2APIC as
> > available on KVM_GET_SUPPORTED_CPUID. Without opt-in, continue to
> > suppress x2APIC from the guest's CPUID.
> > 
> > Signed-off-by: Oliver Upton <oupton@google.com>
> > Reviewed-by: Jim Mattson <jmattson@google.com>
> 
> Since AVIC is not enabled by default, let's do this always and flip the
> default to avic=1 in 5.7.  People using avic=1 will have to disable
> x2apic manually instead but the default will be the same in practice
> (AVIC not enabled).  And then we can figure out:
> 

I'll drop the new module param in v2 and adopt this suggested behavior.

> - how to do emulation of x2apic when avic is enabled (so it will cause
> vmexits but still use the AVIC for e.g. assigned devices)
>
> - a PV CPUID leaf to tell <=255 vCPUs on AMD virtualization _not_ to use
> x2apic.
>

If a VMM didn't want the guest to use x2APIC in the first place, shouldn't
it instead omit x2APIC from the guest CPUID? I can see a point for this
if folks are inclined to use the same CPUID for VMs regardless of shape.
Just a thought to tackle later down the road :)

Thanks for the review, I'll send a new patch out shortly.

--
Thanks,
Oliver

> Paolo
> 
> > ---
> > 
> >  Parent commit: a93236fcbe1d ("KVM: s390: rstify new ioctls in api.rst")
> > 
> >  arch/x86/include/asm/kvm_host.h |  1 +
> >  arch/x86/kvm/svm.c              | 19 ++++++++++++++++---
> >  2 files changed, 17 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index 98959e8cd448..9d40132a3ae2 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -890,6 +890,7 @@ enum kvm_irqchip_mode {
> >  #define APICV_INHIBIT_REASON_NESTED     2
> >  #define APICV_INHIBIT_REASON_IRQWIN     3
> >  #define APICV_INHIBIT_REASON_PIT_REINJ  4
> > +#define APICV_INHIBIT_REASON_X2APIC	5
> >  
> >  struct kvm_arch {
> >  	unsigned long n_used_mmu_pages;
> > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> > index ad3f5b178a03..95c03c75f51a 100644
> > --- a/arch/x86/kvm/svm.c
> > +++ b/arch/x86/kvm/svm.c
> > @@ -382,6 +382,10 @@ module_param(sev, int, 0444);
> >  static bool __read_mostly dump_invalid_vmcb = 0;
> >  module_param(dump_invalid_vmcb, bool, 0644);
> >  
> > +/* enable/disable opportunistic use of the AVIC on a per-VM basis */
> > +static bool __read_mostly avic_per_vm;
> > +module_param(avic_per_vm, bool, 0444);
> > +
> >  static u8 rsm_ins_bytes[] = "\x0f\xaa";
> >  
> >  static void svm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0);
> > @@ -6027,7 +6031,15 @@ static void svm_cpuid_update(struct kvm_vcpu *vcpu)
> >  	if (!kvm_vcpu_apicv_active(vcpu))
> >  		return;
> >  
> > -	guest_cpuid_clear(vcpu, X86_FEATURE_X2APIC);
> > +	/*
> > +	 * AVIC does not work with an x2APIC mode guest. If the X2APIC feature
> > +	 * is exposed to the guest, disable AVIC.
> > +	 */
> > +	if (avic_per_vm && guest_cpuid_has(vcpu, X86_FEATURE_X2APIC))
> > +		kvm_request_apicv_update(vcpu->kvm, false,
> > +					 APICV_INHIBIT_REASON_X2APIC);
> > +	else
> > +		guest_cpuid_clear(vcpu, X86_FEATURE_X2APIC);
> >  
> >  	/*
> >  	 * Currently, AVIC does not work with nested virtualization.
> > @@ -6044,7 +6056,7 @@ static void svm_set_supported_cpuid(u32 func, struct kvm_cpuid_entry2 *entry)
> >  {
> >  	switch (func) {
> >  	case 0x1:
> > -		if (avic)
> > +		if (avic && !avic_per_vm)
> >  			entry->ecx &= ~F(X2APIC);
> >  		break;
> >  	case 0x80000001:
> > @@ -7370,7 +7382,8 @@ static bool svm_check_apicv_inhibit_reasons(ulong bit)
> >  			  BIT(APICV_INHIBIT_REASON_HYPERV) |
> >  			  BIT(APICV_INHIBIT_REASON_NESTED) |
> >  			  BIT(APICV_INHIBIT_REASON_IRQWIN) |
> > -			  BIT(APICV_INHIBIT_REASON_PIT_REINJ);
> > +			  BIT(APICV_INHIBIT_REASON_PIT_REINJ) |
> > +			  BIT(APICV_INHIBIT_REASON_X2APIC);
> >  
> >  	return supported & BIT(bit);
> >  }
> > 
> 

Re: [PATCH] KVM: SVM: Inhibit APIC virtualization for X2APIC guest

[Paolo Bonzini]

On 28/02/20 09:45, Oliver Upton wrote:
>> - a PV CPUID leaf to tell <=255 vCPUs on AMD virtualization _not_ to use
>> x2apic.
>>
> If a VMM didn't want the guest to use x2APIC in the first place, shouldn't
> it instead omit x2APIC from the guest CPUID? I can see a point for this
> if folks are inclined to use the same CPUID for VMs regardless of shape.
> Just a thought to tackle later down the road :)

Yes, it's mostly for simplicity of configuration.

Paolo