On Wed, 2022-08-31 at 00:34 +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);
>
> 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 +++++++++++++++----------------
> 1 file changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index b1ade555e8d0..f3a74c8284cb 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -741,18 +741,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;
> @@ -1094,17 +1082,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_set_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
> @@ -1118,6 +1107,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_set_virtual_apic_mode(vcpu);
This call is misleading - this will usually be called
when avic mode didn't change - I think we need a better name for
avic_set_virtual_apic_mode.
Other than that this makes sense.
Best regards,
Maxim Levitsky
>
> if (activated)
> avic_vcpu_load(vcpu, vcpu->cpu);