Re: [PATCH v2 01/29] x86: treewide: Introduce x86_vendor_amd_or_hygon()

[H. Peter Anvin]

On March 18, 2025 4:07:51 AM PDT, "Ahmed S. Darwish" <darwi@xxxxxxxxxxxxx> wrote:
>Hi,
>
>On Mon, 17 Mar 2025, Borislav Petkov wrote:
>>
>> On Mon, Mar 17, 2025, Ahmed S. Darwish wrote:
>> > The pattern to check if an x86 vendor is AMD or HYGON (or not both) is
>> > pretty common.  Introduce x86_vendor_amd_or_hygon() at <asm/processor.h>
>>
>> So if we need to check "intel too", we do
>>
>> x86_vendor_amd_or_hygon_or_intel?
>>
>> Nah, this is silly.
>>
>
>I needed this while refactoring the cacheinfo.c leaf 0x8000001d code at
>patch 11/29 ("x86/cacheinfo: Consolidate AMD/Hygon leaf 0x8000001d
>calls") as the combined check was done multiple times.
>
>Then I found that there are 28 other cases in the x86 tree where the
>AMD/Hygon CPU vendor check is also combined.  So I did that macro and it
>also made a number the affected sites more succinct; e.g.:
>
>|  diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
>|  index 43dcd8c7badc..13df4917d7d8 100644
>|  --- a/arch/x86/xen/enlighten.c
>|  +++ b/arch/x86/xen/enlighten.c
>|  @@ -82,11 +82,9 @@ void xen_hypercall_setfunc(void)
>|   	if (static_call_query(xen_hypercall) != xen_hypercall_hvm)
>|   		return;
>|
>|  -	if ((boot_cpu_data.x86_vendor == X86_VENDOR_AMD ||
>|  -	     boot_cpu_data.x86_vendor == X86_VENDOR_HYGON))
>|  -		static_call_update(xen_hypercall, xen_hypercall_amd);
>|  -	else
>|  -		static_call_update(xen_hypercall, xen_hypercall_intel);
>|  +	static_call_update(xen_hypercall,
>|  +			   x86_vendor_amd_or_hygon(boot_cpu_data.x86_vendor) ?
>|  +			   xen_hypercall_amd : xen_hypercall_intel);
>|   }
>|
>|   /*
>|  @@ -118,11 +116,8 @@ noinstr void *__xen_hypercall_setfunc(void)
>|   	if (!boot_cpu_has(X86_FEATURE_CPUID))
>|   		xen_get_vendor();
>|
>|  -	if ((boot_cpu_data.x86_vendor == X86_VENDOR_AMD ||
>|  -	     boot_cpu_data.x86_vendor == X86_VENDOR_HYGON))
>|  -		func = xen_hypercall_amd;
>|  -	else
>|  -		func = xen_hypercall_intel;
>|  +	func = x86_vendor_amd_or_hygon(boot_cpu_data.x86_vendor) ?
>|  +		xen_hypercall_amd : xen_hypercall_intel;
>|
>|   	static_call_update_early(xen_hypercall, func);
>
>Nonetheless, I've seen your other emails in the thread, and I'll drop the
>patch.
>
>Thanks!
>
>--
>Ahmed S. Darwish
>Linutronix GmbH

I would agree that abstracting this into something higher level makes sense, but have you considered whether or not it is actually necessary to do this in the first place? In the case of level 0x8000001d for example, that should be handled by the end bracket from leaf 0x80000000.

In general, VFMS checks are not a good thing.