Re: [for-4.18][PATCH 1/2] xen/x86: Add family guards to the is_zen[12]_uarch() macros

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 13/10/2023 1:26 am, Alejandro Vallejo wrote:
> It slightly simplifies the code that uses them at no real cost because the
> code is very rarely executed. This makes it harder to confuse zen uarches
> due to missing or mistaken family checks.

I'm afraid I disagree.

It's bogus to do a family check without a vendor check.  By making this
change, you're separating (spatially in code, and therefore cognitively)
the two checks that it's important to be able to cross-check.

> diff --git a/xen/arch/x86/include/asm/amd.h b/xen/arch/x86/include/asm/amd.h
> index d862cb7972..5a40bcc2ba 100644
> --- a/xen/arch/x86/include/asm/amd.h
> +++ b/xen/arch/x86/include/asm/amd.h
> @@ -145,11 +145,12 @@
>   * Hygon (Fam18h) but without simple model number rules.  Instead, use STIBP
>   * as a heuristic that distinguishes the two.
>   *
> - * The caller is required to perform the appropriate vendor/family checks
> - * first.
> + * The caller is required to perform the appropriate vendor check first.
>   */
> -#define is_zen1_uarch() (!boot_cpu_has(X86_FEATURE_AMD_STIBP))
> -#define is_zen2_uarch()   boot_cpu_has(X86_FEATURE_AMD_STIBP)
> +#define is_zen1_uarch() ((boot_cpu_data.x86 == 0x17 || boot_cpu_data.x86 == 
> 0x18) && \
> +                         !boot_cpu_has(X86_FEATURE_AMD_STIBP))
> +#define is_zen2_uarch() (boot_cpu_data.x86 == 0x17 && \
> +                         boot_cpu_has(X86_FEATURE_AMD_STIBP))

What leads you to believe there aren't Hygon Zen2's ?

~Andrew