On Fri, 2022-03-04 at 18:22 +0700, Suravee Suthikulpanit wrote:
> Maxim,
>
> On 2/25/22 3:03 AM, Maxim Levitsky wrote:
> > On Sun, 2022-02-20 at 20:19 -0600, Suravee Suthikulpanit wrote:
> > > ....
> > >
> > > 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
>
> Okay
>
> > > + 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;
> >
>
> Sorry, I don't quiet understand this part. We just want to update certain bits in the VMCB register.
It seems a bit cleaner to me to create that field again instead of erasing bits like that.
But honestly I don't mind it that much.
>
> > > ...
> > > +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 ?
>
> It's already doing "vmcb->control.int_ctl |= X2APIC_MODE_MASK;" in avic_activate_vmcb().
Yes, but why not also to set/clear AVIC_ENABLE_MASK there as well?
>
> > > 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.
>
> Agree, and will be updated to use svm_set_virtual_apic_mode() in v2.
>
> > 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.
>
> Not sure if we need this. The logic for activating x2AVIC in VMCB already
> check if the guest x2APIC mode is set, which can only happen if x2APIC CPUID
> is set.
I don't mind that much, just a suggestion.
>
> > > }
> > >
> > > 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?
>
> For AVIC, this WARN_ON is designed to catch the scenario when the vCPU is calling
> avic_vcpu_load() while it is already running. However, with x2AVIC support,
> the vCPU can switch from xAPIC to x2APIC mode while in running state
> (i.e. the AVIC is_running is set). This warning is currently observed due to
> the call from svm_refresh_apicv_exec_ctrl().
Ah, understand you now!
Best regards,
Thanks,
Maxim Levitsky
>
> Regards,
> Suravee
>
