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

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

Hi,

I'll just remove this patch (and amend the next) in the interest of having
it committed early. That said...

On Fri, Oct 13, 2023 at 10:14:45AM +0800, Andrew Cooper wrote:
> 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.
As it stands, it's a matter of time before a bug of this form creeps up. 
Particularly
because it reads very innocent.

   if (is_zen1_uarch())
      fun1();
   else if (is_zen2_uarch())
      fun2();
   else if (is_zen3_uarch())
      fun3();
   else if (is_zen4_uarch())
      fun4();

Particularly if the bodies of each conditional are big enough that you lose
track of the family you're dealing with.

> 
> It's bogus to do a family check without a vendor check.
I can get behind that. I didn't include the vendor check because by and
large these macros are used in vendor-specific areas. Would that appease
your concerns? Whenever the macros are used we're in glacially cold paths
anyway.

> 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.
IMO, It's no different from separating the heuristic from the vendor/family
check. What causes definite cognitive load is (as a reader) having to
remember what particular fields must be read off boot_cpu_data in order to
discriminate zenN, which of them are co-familiar and so on.

> 
> > 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 ?
The same argument that a Hygon zen2 supports STIBP. Having seen neither HW
nor docs all that's left is divination skills :)

> 
> ~Andrew

As I said, let's move this discussion away from the errata fix (I'll remove
this patch and adapt the next one to remove the family check), as it's
unrelated and can be done later if I manage to convince you.

Thanks,
Alejandro