Re: [PATCH v4 08/32] KVM: SVM: Don't put/load AVIC when setting virtual APIC mode

From: Maxim Levitsky
Date: Thu Dec 08 2022 - 16:55:17 EST

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@xxxxxxxxxxxxxxx
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> Cc: Maxim Levitsky <mlevitsk@xxxxxxxxxx>
> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>

Best regards,
Maxim Levitsky