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

[Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>]

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