Re: [PATCH v3 1/2] KVM: SVM: Increase X2AVIC limit to 4096 vcpus

[Sean Christopherson <seanjc@google.com>]

On Thu, Feb 20, 2025, Naveen N Rao (AMD) wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> Newer AMD platforms enhance x2AVIC feature to support up to 4096 vcpus.
> This capatility is detected via CPUID_Fn8000000A_ECX[x2AVIC_EXT].
> 
> Modify the SVM driver to check the capability. If detected, extend bitmask
> for guest max physical APIC ID to 0xFFF, increase maximum vcpu index to
> 4095, and increase the size of the Phyical APIC ID table from 4K to 32K in
> order to accommodate up to 4096 entries.

Kinda silly, but please split this into (at least) two patches.  One to introduce
the variables to replace the macros, and then one to actually implement support
for 4096 entries.  That makes it a _lot_ easier to review each change (I'm having
trouble teasing out what's actually changing for 4k support).

The changelog also needs more info.  Unless I'm misreading the diff, only the
physical table is being expanded?  How does that work?  (I might be able to
figure it out if I think hard, but I shouldn't have to think that hard).

> @@ -182,7 +185,8 @@ void avic_vm_destroy(struct kvm *kvm)
>  	if (kvm_svm->avic_logical_id_table_page)
>  		__free_page(kvm_svm->avic_logical_id_table_page);
>  	if (kvm_svm->avic_physical_id_table_page)
> -		__free_page(kvm_svm->avic_physical_id_table_page);
> +		__free_pages(kvm_svm->avic_physical_id_table_page,
> +			     get_order(sizeof(u64) * (x2avic_max_physical_id + 1)));

The order should be encapsulated in some way, e.g. through a global or a helper.

> @@ -1218,8 +1224,19 @@ bool avic_hardware_setup(void)
>  
>  	/* AVIC is a prerequisite for x2AVIC. */
>  	x2avic_enabled = boot_cpu_has(X86_FEATURE_X2AVIC);
> -	if (x2avic_enabled)
> -		pr_info("x2AVIC enabled\n");
> +	if (x2avic_enabled) {
> +		x2avic_4k_vcpu_supported = !!(cpuid_ecx(0x8000000a) & 0x40);

No, add an X86_FEATURE_xxx for this, don't open code the CPUID lookup.  I think
I'd even be tempted to use helpers instead of  

> +		if (x2avic_4k_vcpu_supported) {
> +			x2avic_max_physical_id = X2AVIC_MAX_PHYSICAL_ID_4K;
> +			avic_physical_max_index_mask = AVIC_PHYSICAL_MAX_INDEX_4K_MASK;
> +		} else {
> +			x2avic_max_physical_id = X2AVIC_MAX_PHYSICAL_ID;
> +			avic_physical_max_index_mask = AVIC_PHYSICAL_MAX_INDEX_MASK;
> +		}
> +
> +		pr_info("x2AVIC enabled%s\n",
> +			x2avic_4k_vcpu_supported ? " (w/ 4K-vcpu)" : "");

Maybe print the max number of vCPUs that are supported?  That way there is clear
signal when 4k *isn't* supported (and communicating the max number of vCPUs in
the !4k case would be helpful too).

> +	}
>  
>  	amd_iommu_register_ga_log_notifier(&avic_ga_log_notifier);
>  
> -- 
> 2.48.1
>