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: <fe7cd5012445a941cf55ea82871ea51157490aaa.camel@redhat.com>
Subject: Re: [PATCH v3 11/14] KVM: SVM: Introduce hybrid-AVIC mode
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:   Wed, 04 May 2022 15:32:57 +0300
In-Reply-To: <20220504073128.12031-12-suravee.suthikulpanit@amd.com>
References: <20220504073128.12031-1-suravee.suthikulpanit@amd.com>
         <20220504073128.12031-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 Wed, 2022-05-04 at 02:31 -0500, Suravee Suthikulpanit wrote:
> Currently, AVIC is inhibited when booting a VM w/ x2APIC support.
> because AVIC cannot virtualize x2APIC MSR register accesses.
> However, the AVIC doorbell can be used to accelerate interrupt
> injection into a running vCPU, while all guest accesses to x2APIC MSRs
> will be intercepted and emulated by KVM.
> 
> With hybrid-AVIC support, the APICV_INHIBIT_REASON_X2APIC is
> no longer enforced.
> 
> Suggested-by: Maxim Levitsky <mlevitsk@redhat.com>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  arch/x86/kvm/svm/avic.c | 10 +++++++++-
>  arch/x86/kvm/svm/svm.c  |  9 ---------
>  2 files changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index d07c58f06bed..3b6a96043633 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -92,12 +92,20 @@ static void avic_activate_vmcb(struct vcpu_svm *svm)
>  	vmcb->control.avic_physical_id &= ~AVIC_PHYSICAL_MAX_INDEX_MASK;
>  
>  	vmcb->control.int_ctl |= AVIC_ENABLE_MASK;
> -	if (apic_x2apic_mode(svm->vcpu.arch.apic)) {
> +
> +	/* Note:
> +	 * KVM can support hybrid-x2AVIC mode, where KVM emulates x2APIC
> +	 * MSR accesses, while interrupt injection to a running vCPU
> +	 * can be achieve using AVIC doorbell.
> +	 */
> +	if (apic_x2apic_mode(svm->vcpu.arch.apic) &&
> +	    (avic_mode == AVIC_MODE_X2)) {
>  		vmcb->control.int_ctl |= X2APIC_MODE_MASK;
>  		vmcb->control.avic_physical_id |= X2AVIC_MAX_PHYSICAL_ID;
>  		/* Disabling MSR intercept for x2APIC registers */
>  		avic_set_x2apic_msr_interception(svm, false);
>  	} else {
> +		/* For xAVIC and hybrid-x2AVIC modes */
>  		vmcb->control.avic_physical_id |= AVIC_MAX_PHYSICAL_ID;
>  		/* Enabling MSR intercept for x2APIC registers */
>  		avic_set_x2apic_msr_interception(svm, true);
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 96a1fc1a1d1b..c0a3d4a1f3dc 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -4041,7 +4041,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) &&
> @@ -4073,14 +4072,6 @@ 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);
> -	}
>  	init_vmcb_after_set_cpuid(vcpu);
>  }
>  


Well strictly speaking, another thing that has to be done, other that removing the inhibit,
is to 'hide' the AVIC's private memslot if one of vCPUs is in x2apic mode, 
although not doing this doesn't cause any harm as the guest is not supposed to poke at xAPIC
mmio even when uses x2apic, and if it does it will get the normal AVIC acceleration,
so probably it is better to not add any more complexity and leave it like that.

Besides that my only note on this is that you forgot the most satisfying part of this,
removing the APICV_INHIBIT_REASON_X2APIC value ;-)

So besides the removal of the APICV_INHIBIT_REASON_X2APIC:

Reviewed-by: Maxim Levitsky <mlevisk@redhat.com>


BTW, hardware wise, does 'X2APIC_MODE' keeps the emulation of the AVIC mmio, or
not?

Best regards,
	Maxim Levitsky