Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <3fd0aabb6288a5703760da854fd6b09a485a2d69.camel@redhat.com>
Subject: Re: [PATCH v2 11/12] KVM: SVM: Do not inhibit APICv when x2APIC is
 present
From:   Maxim Levitsky <mlevitsk@redhat.com>
To:     Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>,
        linux-kernel@vger.kernel.org, kvm@vger.kernel.org
Cc:     pbonzini@redhat.com, seanjc@google.com, joro@8bytes.org,
        jon.grimm@amd.com, wei.huang2@amd.com, terry.bowman@amd.com
Date:   Tue, 19 Apr 2022 16:29:18 +0300
In-Reply-To: <20220412115822.14351-12-suravee.suthikulpanit@amd.com>
References: <20220412115822.14351-1-suravee.suthikulpanit@amd.com>
         <20220412115822.14351-12-suravee.suthikulpanit@amd.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.36.5 (3.36.5-2.fc32) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue, 2022-04-12 at 06:58 -0500, Suravee Suthikulpanit wrote:
> Currently, AVIC is inhibited when booting a VM w/ x2APIC support.
> This is because AVIC cannot virtualize x2APIC mode in the VM.
> With x2AVIC support, the APICV_INHIBIT_REASON_X2APIC is
> no longer enforced.
> 
> Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  arch/x86/kvm/svm/avic.c | 19 +++++++++++++++++++
>  arch/x86/kvm/svm/svm.c  | 17 ++---------------
>  arch/x86/kvm/svm/svm.h  |  1 +
>  3 files changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 085a82e95cb0..abcf761c0c53 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -21,6 +21,7 @@
>  
>  #include <asm/irq_remapping.h>
>  
> +#include "cpuid.h"
>  #include "trace.h"
>  #include "lapic.h"
>  #include "x86.h"
> @@ -159,6 +160,24 @@ void avic_vm_destroy(struct kvm *kvm)
>  	spin_unlock_irqrestore(&svm_vm_data_hash_lock, flags);
>  }
>  
> +void avic_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu, int nested)
> +{
> +	/*
> +	 * If the X2APIC feature is exposed to the guest,
> +	 * disable AVIC unless X2AVIC mode is enabled.
> +	 */
> +	if (avic_mode == AVIC_MODE_X1 &&
> +	    guest_cpuid_has(vcpu, X86_FEATURE_X2APIC))
> +		kvm_set_apicv_inhibit(vcpu->kvm, APICV_INHIBIT_REASON_X2APIC);
> +
> +	/*
> +	 * Currently, AVIC does not work with nested virtualization.
> +	 * So, we disable AVIC when cpuid for SVM is set in the L1 guest.
> +	 */
> +	if (nested && guest_cpuid_has(vcpu, X86_FEATURE_SVM))
> +		kvm_set_apicv_inhibit(vcpu->kvm, APICV_INHIBIT_REASON_NESTED);
> +}
> +
>  int avic_vm_init(struct kvm *kvm)
>  {
>  	unsigned long flags;
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index b7dbd8bb2c0a..931998d1d8c4 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3961,7 +3961,6 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>  {
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  	struct kvm_cpuid_entry2 *best;
> -	struct kvm *kvm = vcpu->kvm;
>  
>  	vcpu->arch.xsaves_enabled = guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) &&
>  				    boot_cpu_has(X86_FEATURE_XSAVE) &&
> @@ -3982,21 +3981,9 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>  			vcpu->arch.reserved_gpa_bits &= ~(1UL << (best->ebx & 0x3f));
>  	}
>  
> -	if (kvm_vcpu_apicv_active(vcpu)) {
> -		/*
> -		 * AVIC does not work with an x2APIC mode guest. If the X2APIC feature
> -		 * is exposed to the guest, disable AVIC.
> -		 */
> -		if (guest_cpuid_has(vcpu, X86_FEATURE_X2APIC))
> -			kvm_set_apicv_inhibit(kvm, APICV_INHIBIT_REASON_X2APIC);
> +	if (kvm_vcpu_apicv_active(vcpu))
> +		avic_vcpu_after_set_cpuid(vcpu, nested);
>  
> -		/*
> -		 * Currently, AVIC does not work with nested virtualization.
> -		 * So, we disable AVIC when cpuid for SVM is set in the L1 guest.
> -		 */
> -		if (nested && guest_cpuid_has(vcpu, X86_FEATURE_SVM))
> -			kvm_set_apicv_inhibit(kvm, APICV_INHIBIT_REASON_NESTED);
> -	}
>  	init_vmcb_after_set_cpuid(vcpu);
>  }
>  
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index e340c86941be..0312eec7c7f5 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -583,6 +583,7 @@ int avic_init_vcpu(struct vcpu_svm *svm);
>  void __avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu);
>  void __avic_vcpu_put(struct kvm_vcpu *vcpu);
>  void avic_apicv_post_state_restore(struct kvm_vcpu *vcpu);
> +void avic_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu, int nested);
>  void avic_set_virtual_apic_mode(struct kvm_vcpu *vcpu);
>  void avic_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu);
>  bool avic_check_apicv_inhibit_reasons(enum kvm_apicv_inhibit reason);

Hi!


I just got an idea, while writing a kvm selftest that would use AVIC,
and finding out that selftest code uploads the '-host' cpuid right away
which has x2apic enabled and that inhibits AVIC, and later clearing x2apic
in the cpuid doesn't un-inhibit it.
 
That can be fixed in few ways but that got me thinking:
 
Why do we inhibit AVIC when the guest uses x2apic, even without X2AVIC?
I think that if we didn't it would just work, and even work faster than
pure software x2apic.
 
My thinking is:
 
- when a vcpu itself uses its x2apic, even if its avic is not inhibited, 
the guest will write x2apic msrs which kvm intercepts and will correctly emulate a proper x2apic.
 
- vcpu peers will also use x2apic msrs and again it will work correctly 
(even when there are more than 256 vcpus).
 
- and the host + iommu will still be able to use AVIC's doorbell to send interrupts to the guest
and that doesn't need apic ids or anything, it should work just fine. 

Also AVIC should have no issues scanning IRR and injecting interrupts on VM entry, 
x2apic mode doesn't matter for that.
 
AVIC mmio can still be though discovered by the guest which is technically against x86 spec
(in x2apic mode, mmio supposed to not work) but that can be fixed easily by disabing
the AVIC memslot if any of the vCPUs are in x2apic mode, or this can be ignored since
it should not cause any issues.
We seem to have a quirk for that KVM_X86_QUIRK_LAPIC_MMIO_HOLE.
 
On top of all this, removing this inhibit will also allow to test AVIC with guest
which does have x2apic in the CPUID but doesn't use it (e.g kvm unit test, or
linux booted with nox2apic, which is also nice IMHO)
 
What do you think?

Best regards,
	Maxim Levitsky