On Sun, 2022-02-20 at 20:19 -0600, Suravee Suthikulpanit wrote:
> When an AVIC-enabled guest switch from APIC to x2APIC mode during runtime,
> the SVM driver needs to
>
> 1. Set the x2APIC mode bit for AVIC in VMCB along with the maximum
> APIC ID support for each mode accodingly.
>
> 2. Disable x2APIC MSRs interception in order to allow the hardware
> to virtualize x2APIC MSRs accesses.
>
> This is currently handled in the svm_refresh_apicv_exec_ctrl().
>
> Note that guest kerenel does not need to disable APIC before swtiching
> to x2APIC. Therefore the WARN_ON in vcpu_load() to check if the vCPU is
> currently running is no longer appropriate.
>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> ---
> arch/x86/kvm/svm/avic.c | 61 +++++++++++++++++++++++++++++++++++++----
> 1 file changed, 55 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 3543b7a4514a..3306b74f1d8b 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -79,6 +79,50 @@ static inline enum avic_modes avic_get_vcpu_apic_mode(struct vcpu_svm *svm)
> return AVIC_MODE_NONE;
> }
>
> +static inline void avic_set_x2apic_msr_interception(struct vcpu_svm *svm, bool disable)
> +{
> + int i;
> +
> + for (i = 0x800; i <= 0x8ff; i++)
> + set_msr_interception(&svm->vcpu, svm->msrpm, i,
> + !disable, !disable);
> +}
> +
> +void avic_activate_vmcb(struct vcpu_svm *svm)
> +{
> + struct vmcb *vmcb = svm->vmcb01.ptr;
> +
> + vmcb->control.int_ctl |= AVIC_ENABLE_MASK;
> +
> + if (svm->x2apic_enabled) {
Use apic_x2apic_mode here as well
> + vmcb->control.int_ctl |= X2APIC_MODE_MASK;
> + vmcb->control.avic_physical_id &= ~X2AVIC_MAX_PHYSICAL_ID;
> + vmcb->control.avic_physical_id |= X2AVIC_MAX_PHYSICAL_ID;
Why not just use
phys_addr_t ppa = __sme_set(page_to_phys(kvm_svm->avic_physical_id_table_page));;
vmcb->control.avic_physical_id = ppa | X2AVIC_MAX_PHYSICAL_ID;
> + /* Disabling MSR intercept for x2APIC registers */
> + avic_set_x2apic_msr_interception(svm, false);
> + } else {
> + vmcb->control.avic_physical_id &= ~AVIC_MAX_PHYSICAL_ID;
> + vmcb->control.avic_physical_id |= AVIC_MAX_PHYSICAL_ID;
Same here....
> + /* Enabling MSR intercept for x2APIC registers */
> + avic_set_x2apic_msr_interception(svm, true);
> + }
> +}
> +
> +void avic_deactivate_vmcb(struct vcpu_svm *svm)
> +{
> + struct vmcb *vmcb = svm->vmcb01.ptr;
> +
> + vmcb->control.int_ctl &= ~(AVIC_ENABLE_MASK | X2APIC_MODE_MASK);
> +
> + if (svm->x2apic_enabled)
> + vmcb->control.avic_physical_id &= ~X2AVIC_MAX_PHYSICAL_ID;
> + else
> + vmcb->control.avic_physical_id &= ~AVIC_MAX_PHYSICAL_ID;
> +
> + /* Enabling MSR intercept for x2APIC registers */
> + avic_set_x2apic_msr_interception(svm, true);
> +}
> +
> /* Note:
> * This function is called from IOMMU driver to notify
> * SVM to schedule in a particular vCPU of a particular VM.
> @@ -195,13 +239,12 @@ void avic_init_vmcb(struct vcpu_svm *svm)
> vmcb->control.avic_backing_page = bpa & AVIC_HPA_MASK;
> vmcb->control.avic_logical_id = lpa & AVIC_HPA_MASK;
> vmcb->control.avic_physical_id = ppa & AVIC_HPA_MASK;
> - vmcb->control.avic_physical_id |= AVIC_MAX_PHYSICAL_ID;
> vmcb->control.avic_vapic_bar = APIC_DEFAULT_PHYS_BASE & VMCB_AVIC_APIC_BAR_MASK;
>
> if (kvm_apicv_activated(svm->vcpu.kvm))
> - vmcb->control.int_ctl |= AVIC_ENABLE_MASK;
> + avic_activate_vmcb(svm);
Why not set AVIC_ENABLE_MASK in avic_activate_vmcb ?
> else
> - vmcb->control.int_ctl &= ~AVIC_ENABLE_MASK;
> + avic_deactivate_vmcb(svm);
> }
>
> static u64 *avic_get_physical_id_entry(struct kvm_vcpu *vcpu,
> @@ -657,6 +700,13 @@ void avic_update_vapic_bar(struct vcpu_svm *svm, u64 data)
> svm->x2apic_enabled ? "x2APIC" : "xAPIC");
> vmcb_mark_dirty(svm->vmcb, VMCB_AVIC);
> kvm_vcpu_update_apicv(&svm->vcpu);
> +
> + /*
> + * The VM could be running w/ AVIC activated switching from APIC
> + * to x2APIC mode. We need to all refresh to make sure that all
> + * x2AVIC configuration are being done.
> + */
> + svm_refresh_apicv_exec_ctrl(&svm->vcpu);
That also should be done in avic_set_virtual_apic_mode instead, but otherwise should be fine.
Also it seems that .avic_set_virtual_apic_mode will cover you on the case when x2apic is disabled
in the guest cpuid - kvm_set_apic_base checks if the guest cpuid has x2apic support and refuses
to enable it if it is not set.
But still a WARN_ON_ONCE won't hurt to see that you are not enabling x2avic when not supported.
> }
>
> void svm_set_virtual_apic_mode(struct kvm_vcpu *vcpu)
> @@ -722,9 +772,9 @@ void svm_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu)
> * accordingly before re-activating.
> */
> avic_post_state_restore(vcpu);
> - vmcb->control.int_ctl |= AVIC_ENABLE_MASK;
> + avic_activate_vmcb(svm);
> } else {
> - vmcb->control.int_ctl &= ~AVIC_ENABLE_MASK;
> + avic_deactivate_vmcb(svm);
> }
> vmcb_mark_dirty(vmcb, VMCB_AVIC);
>
> @@ -1019,7 +1069,6 @@ void avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> return;
>
> entry = READ_ONCE(*(svm->avic_physical_id_cache));
> - WARN_ON(entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK);
Why?
>
> entry &= ~AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK;
> entry |= (h_physical_id & AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK);
Best regards,
Maxim Levitsky
