Re: [RFC PATCH 03/13] KVM: SVM: Detect X2APIC virtualization (x2AVIC) support

[Suravee Suthikulpanit]

Hi Maxim,

On 2/24/22 11:52 PM, 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 472445aaaf42..abde08ca23ab 100644
> > --- a/arch/x86/kvm/svm/avic.c
> > +++ b/arch/x86/kvm/svm/avic.c
> > @@ -40,6 +40,15 @@
> >   #define AVIC_GATAG_TO_VMID(x)		((x >> AVIC_VCPU_ID_BITS) & AVIC_VM_ID_MASK)
> >   #define AVIC_GATAG_TO_VCPUID(x)		(x & AVIC_VCPU_ID_MASK)
> >   
> > +#define IS_AVIC_MODE_X1(x)		(avic_get_vcpu_apic_mode(x) == AVIC_MODE_X1)
> > +#define IS_AVIC_MODE_X2(x)		(avic_get_vcpu_apic_mode(x) == AVIC_MODE_X2)
> > +
> > +enum avic_modes {
> > +	AVIC_MODE_NONE = 0,
> > +	AVIC_MODE_X1,
> > +	AVIC_MODE_X2,
> > +};
> > +
> >   /* Note:
> >    * This hash table is used to map VM_ID to a struct kvm_svm,
> >    * when handling AMD IOMMU GALOG notification to schedule in
> > @@ -50,6 +59,7 @@ static DEFINE_HASHTABLE(svm_vm_data_hash, SVM_VM_DATA_HASH_BITS);
> >   static u32 next_vm_id = 0;
> >   static bool next_vm_id_wrapped = 0;
> >   static DEFINE_SPINLOCK(svm_vm_data_hash_lock);
> > +static enum avic_modes avic_mode;
> >   
> >   /*
> >    * This is a wrapper of struct amd_iommu_ir_data.
> > @@ -59,6 +69,15 @@ struct amd_svm_iommu_ir {
> >   	void *data;		/* Storing pointer to struct amd_ir_data */
> >   };
> >   
> > +static inline enum avic_modes avic_get_vcpu_apic_mode(struct vcpu_svm *svm)
> > +{
> > +	if (svm->vmcb->control.int_ctl & X2APIC_MODE_MASK)
> > +		return AVIC_MODE_X2;
> > +	else if (svm->vmcb->control.int_ctl & AVIC_ENABLE_MASK)
> > +		return AVIC_MODE_X1;
> > +	else
> > +		return AVIC_MODE_NONE;
> > +}
> I a bit don't like it.
>
> By definition if a vCPU has x2apic, it will use x2avic and if it is in
> xapic mode it will use plain avic, unless avic is inhibited,
> which will also be the case when vCPU is in x2apic mode but hardware
> doesn't support x2avic.
>
> But I might have beeing mistaken here - anyway this function should
> be added when it is used so it will be clear how and why it is needed.

I will remove this part.

> >   
> >   /* Note:
> >    * This function is called from IOMMU driver to notify
> > @@ -1016,3 +1035,28 @@ void avic_vcpu_unblocking(struct kvm_vcpu *vcpu)
> >   
> >   	put_cpu();
> >   }
> > +
> > +/*
> > + * Note:
> > + * - The module param avic enable both xAPIC and x2APIC mode.
> > + * - Hypervisor can support both xAVIC and x2AVIC in the same guest.
> > + * - The mode can be switched at run-time.
> > + */
> > +bool avic_hardware_setup(struct kvm_x86_ops *x86_ops)
> > +{
> > +	if (!npt_enabled)
> > +		return false;
> > +
> > +	if (boot_cpu_has(X86_FEATURE_AVIC)) {
> > +		avic_mode = AVIC_MODE_X1;
> > +		pr_info("AVIC enabled\n");
> > +	}
> > +
> > +	if (boot_cpu_has(X86_FEATURE_X2AVIC)) {
> > +		avic_mode = AVIC_MODE_X2;
> > +		pr_info("x2AVIC enabled\n");
> > +	}
> > +
> > +	amd_iommu_register_ga_log_notifier(&avic_ga_log_notifier);
> If AVIC is not enabled, I guess no need to register GA log notifier?

GA log is only used when AVIC is enabled. I'll restore the AVIC-enabled check.

Regards,
Suravee