Re: [RFC PATCH 11/13] KVM: SVM: Add logic to switch between APIC and x2APIC virtualization mode

[Maxim Levitsky <mlevitsk@redhat.com>]

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
>