Re: [PATCH] x86: Remove x86 prefixed names from mcheck code

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 02/03/2026 7:19 pm, Kevin Lampis wrote:
> struct cpuinfo_x86
>   .x86        => .family
>   .x86_vendor => .vendor
>   .x86_model  => .model
>   .x86_mask   => .stepping
>
> No functional change.
>
> This work is part of making Xen safe for Intel family 18/19.
>
> Signed-off-by: Kevin Lampis <kevin.lampis@xxxxxxxxxx>
> ---
>
> In xen/arch/x86/cpu/mcheck/mce.c: mcheck_init(...)
> Xen only calls `intel_mcheck_init(...)` if family is 6 or 15.
>
> Linux only calls `mce_intel_feature_init(...)` if family != 5.
>
> Do we need to do something like extend this switch statement in
> `mcheck_init(...)` to include family 18 and 19?

Yes, we will want to include 18/19.

The Machine Check infrastructure in the Pentium was very primitive, but
everything 6 and later should be fine.  I think we'll want to switch to
the Linux form here.

>
>
> In xen/arch/x86/cpu/mcheck/mce.c: mce_firstbank(...)
> The check
>   c->family == 6 && c->vendor == X86_VENDOR_INTEL && c->model < 0x1a
>
> could be re-written as
>   c->vfm >= INTEL_PENTIUM_PRO && c->vfm < INTEL_NEHALEM_EP
>
> I don't know if that would be better.

There's a subtlety.  This suggestion is for ...

> diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c
> index 9a91807cfb..10e826e3a6 100644
> --- a/xen/arch/x86/cpu/mcheck/mce.c
> +++ b/xen/arch/x86/cpu/mcheck/mce.c
> @@ -564,8 +564,8 @@ bool mce_available(const struct cpuinfo_x86 *c)
>   */
>  unsigned int mce_firstbank(struct cpuinfo_x86 *c)
>  {
> -    return c->x86 == 6 &&
> -           c->x86_vendor == X86_VENDOR_INTEL && c->x86_model < 0x1a;
> +    return c->family == 6 &&
> +           c->vendor == X86_VENDOR_INTEL && c->model < 0x1a;

... here, but you've made a related change ...

> diff --git a/xen/arch/x86/cpu/mcheck/mce_intel.c 
> b/xen/arch/x86/cpu/mcheck/mce_intel.c
> index 839a0e5ba9..9100ce0f6c 100644
> --- a/xen/arch/x86/cpu/mcheck/mce_intel.c
> +++ b/xen/arch/x86/cpu/mcheck/mce_intel.c
> @@ -711,10 +711,7 @@ static bool mce_is_broadcast(struct cpuinfo_x86 *c)
>       * DisplayFamily_DisplayModel encoding of 06H_EH and above,
>       * a MCA signal is broadcast to all logical processors in the system
>       */
> -    if ( c->x86_vendor == X86_VENDOR_INTEL && c->x86 == 6 &&
> -         c->x86_model >= 0xe )
> -        return true;
> -    return false;
> +    return c->vfm >= INTEL_CORE_YONAH;

... here.

It's not safe to have a single inequality like this, unless you're
absolutely certain you're inside a c->vendor==$ME region.

In both cases here you're replacing a vendor check, suggesting that we
might not be within a suitably guarded region.  mce_firstbank() does
seem to be used from common code.  mce_is_broadcast() OTOH does look to
be fully within an Intel-only region.


Otherwise, The vendor encoding in the higher bits will make a false
positive/negative match on one side of the inequality.

It is safe to use c->vfm >= FOO && v->vfm <= BAR, because constants of
another vendor will not be captured.

~Andrew