Re: [PATCH] x86/gen-cpuid: Split deep_features by vendor

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.04.2026 18:35, Andrew Cooper wrote:
> AMD CPUs need LKGS depend on NSCB, but this dependency cannot be unconditional
> as it will break FRED on Intel CPUs which don't need to enumerate the absence
> of a bug.
> 
> The deep dependecy logic is formed of two parts; a deep_features bitmap
> indicating which features have dependencies, and deep_deps; the mapping of
> feature to dependent features.  Given that NSCB is an unconnected root of a
> dependency, we can have the row in deep_deps and conditionally exclude it the
> deep_features level.
> 
> Rename INIT_DEEP_FEATURES to INIT_ALL_DEEP_FEATURES and add AMD and INTEL
> forms too.  In both xc_cpuid_apply_policy() and sanitise_featureset(), choose
> the appropriate {amd,intel}_deep_features based on vendor.
> 
> Introduce the NSCB <-> LKGS dependency and exclude the NSCB row from
> intel_deep_features.

This reads as if there was a bi-directional dependency.

> --- a/tools/libs/guest/xg_cpuid_x86.c
> +++ b/tools/libs/guest/xg_cpuid_x86.c
> @@ -650,9 +650,12 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t 
> domid, bool restore,
>  
>      if ( featureset )
>      {
> +        static const uint32_t amd_deep_features[] =  INIT_AMD_DEEP_FEATURES;
> +        static const uint32_t intel_deep_features[] =  
> INIT_INTEL_DEEP_FEATURES;

Nit: One blank too many each (after '=').

> @@ -158,11 +159,21 @@ static void zero_leaves(struct cpuid_leaf *l,
>  
>  static void sanitise_featureset(uint32_t *fs)
>  {
> +    const uint32_t *deep_features;
>      /* bitmap_for_each() uses unsigned longs.  Extend with zeroes. */
>      uint32_t disabled_features[
>          ROUNDUP(FSCAPINTS, sizeof(unsigned long)/sizeof(uint32_t))] = {};
>      unsigned int i;
>  
> +    /*
> +     * At the time of writing, amd_deep_features contains one extra 
> dependency
> +     * over intel for a "hardware no longer has this bug" bit.
> +     */
> +    if ( boot_cpu_data.vendor & (X86_VENDOR_AMD|X86_VENDOR_HYGON) )
> +        deep_features = amd_deep_features;
> +    else
> +        deep_features = intel_deep_features;

Aren't there going to be other somewhat similar features which may want
marking like this (no matter whether right away they have a dependency)?
Deferring the special-casing until a dependency appears is only risking
to forget to add them.

> --- a/xen/tools/gen-cpuid.py
> +++ b/xen/tools/gen-cpuid.py
> @@ -344,6 +344,12 @@ def crunch_numbers(state):
>          # The ARCH_CAPS CPUID bit enumerates the availability of the whole 
> register.
>          ARCH_CAPS: feat_range(RDCL_NO, RDCL_NO + 63),
>  
> +        # AMD-only special case.  NullSelectorClearsBase is really a 
> "hardware
> +        # doesn't have this bug any more" bit.  All FRED-capable hardware has
> +        # NSCB properties, so disallow configurations which would cause LGKS
> +        # to behave unexpectedly.
> +        NSCB: [LKGS],

Along the lines of my reply to your review comments on my LKGS patch, why
is LKGS different from other selector loads? Don't we rather need to
disallow (perhaps not physically, but verbally) the suppressing of NSCB?
And then also for a few other "features", as per above? FDP_EXCP_ONLY and
NO_FPU_SEL clearly would be of that kind, and (for possible dependencies)
AMD doesn't know of them.

Also, nit: "LKGS" (in the comment).

Jan