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: <d24692562a38998931b1546b6b02c6aac560afa5.camel@redhat.com>
Subject: Re: [PATCH v4 08/32] KVM: SVM: Don't put/load AVIC when setting
 virtual APIC mode
From:   Maxim Levitsky <mlevitsk@redhat.com>
To:     Sean Christopherson <seanjc@google.com>,
        Paolo Bonzini <pbonzini@redhat.com>
Cc:     kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        Alejandro Jimenez <alejandro.j.jimenez@oracle.com>,
        Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>,
        Li RongQing <lirongqing@baidu.com>
Date:   Thu, 08 Dec 2022 23:53:56 +0200
In-Reply-To: <20221001005915.2041642-9-seanjc@google.com>
References: <20221001005915.2041642-1-seanjc@google.com>
         <20221001005915.2041642-9-seanjc@google.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 Sat, 2022-10-01 at 00:58 +0000, Sean Christopherson wrote:
> Move the VMCB updates from avic_refresh_apicv_exec_ctrl() into
> avic_set_virtual_apic_mode() and invert the dependency being said
> functions to avoid calling avic_vcpu_{load,put}() and
> avic_set_pi_irte_mode() when "only" setting the virtual APIC mode.
> 
> avic_set_virtual_apic_mode() is invoked from common x86 with preemption
> enabled, which makes avic_vcpu_{load,put}() unhappy.  Luckily, calling
> those and updating IRTE stuff is unnecessary as the only reason
> avic_set_virtual_apic_mode() is called is to handle transitions between
> xAPIC and x2APIC that don't also toggle APICv activation.  And if
> activation doesn't change, there's no need to fiddle with the physical
> APIC ID table or update IRTE.
> 
> The "full" refresh is guaranteed to be called if activation changes in
> this case as the only call to the "set" path is:
> 
> 	kvm_vcpu_update_apicv(vcpu);
> 	static_call_cond(kvm_x86_set_virtual_apic_mode)(vcpu);
> 
> and kvm_vcpu_update_apicv() invokes the refresh if activation changes:
> 
> 	if (apic->apicv_active == activate)
> 		goto out;
> 
> 	apic->apicv_active = activate;
> 	kvm_apic_update_apicv(vcpu);
> 	static_call(kvm_x86_refresh_apicv_exec_ctrl)(vcpu);
> 
> Rename the helper to reflect that it is also called during "refresh".
> 
>   WARNING: CPU: 183 PID: 49186 at arch/x86/kvm/svm/avic.c:1081 avic_vcpu_put+0xde/0xf0 [kvm_amd]
>   CPU: 183 PID: 49186 Comm: stable Tainted: G           O       6.0.0-smp--fcddbca45f0a-sink #34
>   Hardware name: Google, Inc. Arcadia_IT_80/Arcadia_IT_80, BIOS 10.48.0 01/27/2022
>   RIP: 0010:avic_vcpu_put+0xde/0xf0 [kvm_amd]
>    avic_refresh_apicv_exec_ctrl+0x142/0x1c0 [kvm_amd]
>    avic_set_virtual_apic_mode+0x5a/0x70 [kvm_amd]
>    kvm_lapic_set_base+0x149/0x1a0 [kvm]
>    kvm_set_apic_base+0x8f/0xd0 [kvm]
>    kvm_set_msr_common+0xa3a/0xdc0 [kvm]
>    svm_set_msr+0x364/0x6b0 [kvm_amd]
>    __kvm_set_msr+0xb8/0x1c0 [kvm]
>    kvm_emulate_wrmsr+0x58/0x1d0 [kvm]
>    msr_interception+0x1c/0x30 [kvm_amd]
>    svm_invoke_exit_handler+0x31/0x100 [kvm_amd]
>    svm_handle_exit+0xfc/0x160 [kvm_amd]
>    vcpu_enter_guest+0x21bb/0x23e0 [kvm]
>    vcpu_run+0x92/0x450 [kvm]
>    kvm_arch_vcpu_ioctl_run+0x43e/0x6e0 [kvm]
>    kvm_vcpu_ioctl+0x559/0x620 [kvm]
> 
> Fixes: 05c4fe8c1bd9 ("KVM: SVM: Refresh AVIC configuration when changing APIC mode")
> Cc: stable@vger.kernel.org
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Cc: Maxim Levitsky <mlevitsk@redhat.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/svm/avic.c | 31 +++++++++++++++----------------
>  arch/x86/kvm/svm/svm.c  |  2 +-
>  arch/x86/kvm/svm/svm.h  |  2 +-
>  3 files changed, 17 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 3b2c88b168ba..97ad0661f963 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -747,18 +747,6 @@ void avic_apicv_post_state_restore(struct kvm_vcpu *vcpu)
>  	avic_handle_ldr_update(vcpu);
>  }
>  
> -void avic_set_virtual_apic_mode(struct kvm_vcpu *vcpu)
> -{
> -	if (!lapic_in_kernel(vcpu) || avic_mode == AVIC_MODE_NONE)
> -		return;
> -
> -	if (kvm_get_apic_mode(vcpu) == LAPIC_MODE_INVALID) {
> -		WARN_ONCE(true, "Invalid local APIC state (vcpu_id=%d)", vcpu->vcpu_id);
> -		return;
> -	}
> -	avic_refresh_apicv_exec_ctrl(vcpu);
> -}
> -
>  static int avic_set_pi_irte_mode(struct kvm_vcpu *vcpu, bool activate)
>  {
>  	int ret = 0;
> @@ -1100,17 +1088,18 @@ void avic_vcpu_put(struct kvm_vcpu *vcpu)
>  	WRITE_ONCE(*(svm->avic_physical_id_cache), entry);
>  }
>  
> -
> -void avic_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu)
> +void avic_refresh_virtual_apic_mode(struct kvm_vcpu *vcpu)
>  {
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  	struct vmcb *vmcb = svm->vmcb01.ptr;
> -	bool activated = kvm_vcpu_apicv_active(vcpu);
> +
> +	if (!lapic_in_kernel(vcpu) || avic_mode == AVIC_MODE_NONE)
> +		return;
>  
>  	if (!enable_apicv)
>  		return;
>  
> -	if (activated) {
> +	if (kvm_vcpu_apicv_active(vcpu)) {
>  		/**
>  		 * During AVIC temporary deactivation, guest could update
>  		 * APIC ID, DFR and LDR registers, which would not be trapped
> @@ -1124,6 +1113,16 @@ void avic_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu)
>  		avic_deactivate_vmcb(svm);
>  	}
>  	vmcb_mark_dirty(vmcb, VMCB_AVIC);
> +}
> +
> +void avic_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu)
> +{
> +	bool activated = kvm_vcpu_apicv_active(vcpu);
> +
> +	if (!enable_apicv)
> +		return;
> +
> +	avic_refresh_virtual_apic_mode(vcpu);
>  
>  	if (activated)
>  		avic_vcpu_load(vcpu, vcpu->cpu);
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 58f0077d9357..afae97ea9a06 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -4798,7 +4798,7 @@ static struct kvm_x86_ops svm_x86_ops __initdata = {
>  	.enable_nmi_window = svm_enable_nmi_window,
>  	.enable_irq_window = svm_enable_irq_window,
>  	.update_cr8_intercept = svm_update_cr8_intercept,
> -	.set_virtual_apic_mode = avic_set_virtual_apic_mode,
> +	.set_virtual_apic_mode = avic_refresh_virtual_apic_mode,
>  	.refresh_apicv_exec_ctrl = avic_refresh_apicv_exec_ctrl,
>  	.check_apicv_inhibit_reasons = avic_check_apicv_inhibit_reasons,
>  	.apicv_post_state_restore = avic_apicv_post_state_restore,
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 6a7686bf6900..7a95f50e80e7 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -646,7 +646,7 @@ void avic_vcpu_blocking(struct kvm_vcpu *vcpu);
>  void avic_vcpu_unblocking(struct kvm_vcpu *vcpu);
>  void avic_ring_doorbell(struct kvm_vcpu *vcpu);
>  unsigned long avic_vcpu_get_apicv_inhibit_reasons(struct kvm_vcpu *vcpu);
> -void avic_set_virtual_apic_mode(struct kvm_vcpu *vcpu);
> +void avic_refresh_virtual_apic_mode(struct kvm_vcpu *vcpu);
>  
>  
>  /* sev.c */

Makes sense. I missed that during x2avic review. That warning about preemption is a good one.


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

Best regards,
	Maxim Levitsky