This function is meant to replace all instances of the following
patterns in CPU policies and boot_cpu_data:

  - x->x86_vendor == X86_VENDOR_FOO
  - x->x86_vendor != X86_VENDOR_FOO
  - x->x86_vendor & (X86_VENDOR_FOO | X86_VENDOR_BAR)

The secret sauce is that all branches inside the helper resolve at
compile time, so for the all-vendors-compiled-in case the function
resolves to equivalent code as that without the helper and you get
progressively more aggressive DCE as you disable vendors. The function
folds into a constant once you remove the fallback CPU vendor setting.

While at this, move an include out of place so they sort alphabetically.

Not a functional change.

Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com>
---
 xen/arch/x86/include/asm/cpuid.h | 49 +++++++++++++++++++++++++++++++-
 1 file changed, 48 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/include/asm/cpuid.h b/xen/arch/x86/include/asm/cpuid.h
index bf1c635cdd..a4280d1b0d 100644
--- a/xen/arch/x86/include/asm/cpuid.h
+++ b/xen/arch/x86/include/asm/cpuid.h
@@ -2,10 +2,12 @@
 #define __X86_CPUID_H__
 
 #include <asm/cpufeatureset.h>
+#include <asm/x86-vendors.h>
 
-#include <xen/types.h>
+#include <xen/compiler.h>
 #include <xen/kernel.h>
 #include <xen/percpu.h>
+#include <xen/types.h>
 
 #include <public/sysctl.h>
 
@@ -56,6 +58,51 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
      (IS_ENABLED(CONFIG_SHANGHAI) ? X86_VENDOR_SHANGHAI : 0) | \
      (IS_ENABLED(CONFIG_HYGON)    ? X86_VENDOR_HYGON    : 0))
 
+/*
+ * When compiling Xen for a single vendor with no fallback vendor there's no
+ * need no check the candidate. `vendor` is always a compile-time constant,
+ * which means this all can fold into a constant boolean.
+ *
+ * A runtime check at the time of CPUID probing guarantees we never run on
+ * wrong hardware and another check when loading CPU policies guarantees we
+ * never run policies for a vendor in another vendor's silicon.
+ *
+ * By the same token, the same folding can happen when no vendor is compiled
+ * in and the fallback path is present.
+ */
+static always_inline bool x86_vendor_is(uint8_t candidate, uint8_t vendor)
+{
+    uint8_t filtered_vendor = vendor & X86_ENABLED_VENDORS;
+
+    if ( vendor == X86_VENDOR_UNKNOWN )
+    {
+        if ( IS_ENABLED(CONFIG_UNKNOWN_CPU) )
+            /* no-vendor optimisation */
+            return X86_ENABLED_VENDORS ? vendor == candidate : true;
+
+        /* unknown-vendor-elimination optimisation */
+        return false;
+    }
+
+    /* single-vendor optimisation */
+    if ( !IS_ENABLED(CONFIG_UNKNOWN_CPU) &&
+         (ISOLATE_LSB(X86_ENABLED_VENDORS) == X86_ENABLED_VENDORS) )
+        return filtered_vendor == X86_ENABLED_VENDORS;
+
+    /* compiled-out-vendor-elimination optimisation */
+    if ( !filtered_vendor )
+        return false;
+
+    /*
+     * When checking against a single vendor, perform an equality check, as
+     * it yields (marginally) better codegen
+     */
+    if ( ISOLATE_LSB(filtered_vendor) == filtered_vendor )
+        return filtered_vendor == candidate ;
+
+    return filtered_vendor & candidate;
+}
+
 #endif /* !__X86_CPUID_H__ */
 
 /*
-- 
2.43.0

On 26.11.2025 17:44, Alejandro Vallejo wrote:
> This function is meant to replace all instances of the following
> patterns in CPU policies and boot_cpu_data:
> 
>   - x->x86_vendor == X86_VENDOR_FOO
>   - x->x86_vendor != X86_VENDOR_FOO
>   - x->x86_vendor & (X86_VENDOR_FOO | X86_VENDOR_BAR)
> 
> The secret sauce is that all branches inside the helper resolve at
> compile time, so for the all-vendors-compiled-in case the function
> resolves to equivalent code as that without the helper and you get
> progressively more aggressive DCE as you disable vendors. The function
> folds into a constant once you remove the fallback CPU vendor setting.

Here and below in the comment, "fallback CPU vendor" wants clarifying. I
don't view it as obvious that what's presently named UNKNOWN_CPU is that
"fallback" (as imo that really isn't any kind of fallback, but merely a
placeholder).

> While at this, move an include out of place so they sort alphabetically.

I'd rather suggest to simply ...

> --- a/xen/arch/x86/include/asm/cpuid.h
> +++ b/xen/arch/x86/include/asm/cpuid.h
> @@ -2,10 +2,12 @@
>  #define __X86_CPUID_H__
>  
>  #include <asm/cpufeatureset.h>
> +#include <asm/x86-vendors.h>
>  
> -#include <xen/types.h>
> +#include <xen/compiler.h>
>  #include <xen/kernel.h>
>  #include <xen/percpu.h>
> +#include <xen/types.h>

... drop it. xen/kernel.h, for example, already gets it for you anyway.

> @@ -56,6 +58,51 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
>       (IS_ENABLED(CONFIG_SHANGHAI) ? X86_VENDOR_SHANGHAI : 0) | \
>       (IS_ENABLED(CONFIG_HYGON)    ? X86_VENDOR_HYGON    : 0))
>  
> +/*
> + * When compiling Xen for a single vendor with no fallback vendor there's no
> + * need no check the candidate. `vendor` is always a compile-time constant,
> + * which means this all can fold into a constant boolean.

DYM "`vendor` is always supposed to be a compile-time constant, ..." ?

> + * A runtime check at the time of CPUID probing guarantees we never run on
> + * wrong hardware and another check when loading CPU policies guarantees we
> + * never run policies for a vendor in another vendor's silicon.
> + *
> + * By the same token, the same folding can happen when no vendor is compiled
> + * in and the fallback path is present.
> + */
> +static always_inline bool x86_vendor_is(uint8_t candidate, uint8_t vendor)

I fear the comment, no matter that it's pretty large already, doesn't make
clear how this function is to be used, i.e. how for this being an "is"
predicate the two arguments should be chosen. My typical expectation would be
for "is" predicates to apply to a single property, with other parameters (if
any) only being auxiliary ones. Maybe it would already help if the first
parameter wasn't named "candidate" but e.g. "actual" (from looking at just
the next patch). Or maybe (depending on the number of possible different
inputs for the first parameter) there want to be a few wrappers, so the
"single property" aspect would be achieved at use sites.

Then I see no reason for the parameters to be other than unsigned int. (Same
for the local variable then, obviously.)

I'm further uncertain this is a good place for the function. In the old days
it may have been, but cpuid.[ch] now are only about guest exposure of CPUID,
when this predicate is intended to be used for both host and guest. (As I
realize only now, this also applies to the addition patch 1 does.) One
might think processor.h might be a good home, but we're actually trying to
slim that one down. So one of cpufeature.h and cpufeatures.h, I guess. (Maybe
other x86 maintainers also have thoughts here.)

> +{
> +    uint8_t filtered_vendor = vendor & X86_ENABLED_VENDORS;
> +
> +    if ( vendor == X86_VENDOR_UNKNOWN )
> +    {
> +        if ( IS_ENABLED(CONFIG_UNKNOWN_CPU) )
> +            /* no-vendor optimisation */

Nit: Comment style (also again below).

> +            return X86_ENABLED_VENDORS ? vendor == candidate : true;

With the surrounding if() this effectively (and more explicitly) is

            return X86_ENABLED_VENDORS ? candidate == X86_VENDOR_UNKNOWN : true;

First: Would one ever pass X86_VENDOR_UNKNOWN for "vendor"? The next patch,
for example, specifically doesn't. And then why not shorter as

            return !X86_ENABLED_VENDORS || candidate == X86_VENDOR_UNKNOWN;

Which raises the next question: Should we even allow a hypervisor to be built
with X86_ENABLED_VENDORS == 0? Plus, question more on patch 1, what's the
(useful) difference between a build with all vendors set to N and
(a) UNKNOWN_CPU=n vs (b) UNKNOWN_CPU=y? With all vendor support explicitly
turned off, all CPUs are going to be "unknown".

> +
> +        /* unknown-vendor-elimination optimisation */
> +        return false;
> +    }
> +
> +    /* single-vendor optimisation */
> +    if ( !IS_ENABLED(CONFIG_UNKNOWN_CPU) &&
> +         (ISOLATE_LSB(X86_ENABLED_VENDORS) == X86_ENABLED_VENDORS) )
> +        return filtered_vendor == X86_ENABLED_VENDORS;
> +
> +    /* compiled-out-vendor-elimination optimisation */
> +    if ( !filtered_vendor )
> +        return false;
> +
> +    /*
> +     * When checking against a single vendor, perform an equality check, as
> +     * it yields (marginally) better codegen
> +     */
> +    if ( ISOLATE_LSB(filtered_vendor) == filtered_vendor )

So one may pass a combination of multiple vendors for "vendor"? Is so, why
is the parameter name singular?

> +        return filtered_vendor == candidate ;

Nit: Stray blank.

Jan

Hi,

On Thu Nov 27, 2025 at 11:46 AM CET, Jan Beulich wrote:
> On 26.11.2025 17:44, Alejandro Vallejo wrote:
>> This function is meant to replace all instances of the following
>> patterns in CPU policies and boot_cpu_data:
>> 
>>   - x->x86_vendor == X86_VENDOR_FOO
>>   - x->x86_vendor != X86_VENDOR_FOO
>>   - x->x86_vendor & (X86_VENDOR_FOO | X86_VENDOR_BAR)
>> 
>> The secret sauce is that all branches inside the helper resolve at
>> compile time, so for the all-vendors-compiled-in case the function
>> resolves to equivalent code as that without the helper and you get
>> progressively more aggressive DCE as you disable vendors. The function
>> folds into a constant once you remove the fallback CPU vendor setting.
>
> Here and below in the comment, "fallback CPU vendor" wants clarifying. I
> don't view it as obvious that what's presently named UNKNOWN_CPU is that
> "fallback" (as imo that really isn't any kind of fallback, but merely a
> placeholder).

I'll rename all fallback references in commits/comments to "generic vendor".
Though do note there's a fallback behaviour. It's introduced in patch 1 due
to the ANDing of the x86_vendor with the mask of compiled-in vendors.

We can trivially get rid of this behaviour, but I assumed booting in untuned
mode is preferable to panicking. And if you _do_ care about panicking when you
don't know about a CPU you're better off setting UNKNOWN_CPU=n and getting that
exact behaviour.

>
>> While at this, move an include out of place so they sort alphabetically.
>
> I'd rather suggest to simply ...
>
>> --- a/xen/arch/x86/include/asm/cpuid.h
>> +++ b/xen/arch/x86/include/asm/cpuid.h
>> @@ -2,10 +2,12 @@
>>  #define __X86_CPUID_H__
>>  
>>  #include <asm/cpufeatureset.h>
>> +#include <asm/x86-vendors.h>
>>  
>> -#include <xen/types.h>
>> +#include <xen/compiler.h>
>>  #include <xen/kernel.h>
>>  #include <xen/percpu.h>
>> +#include <xen/types.h>
>
> ... drop it. xen/kernel.h, for example, already gets it for you anyway.

Good call.

>
>> @@ -56,6 +58,51 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
>>       (IS_ENABLED(CONFIG_SHANGHAI) ? X86_VENDOR_SHANGHAI : 0) | \
>>       (IS_ENABLED(CONFIG_HYGON)    ? X86_VENDOR_HYGON    : 0))
>>  
>> +/*
>> + * When compiling Xen for a single vendor with no fallback vendor there's no
>> + * need no check the candidate. `vendor` is always a compile-time constant,
>> + * which means this all can fold into a constant boolean.
>
> DYM "`vendor` is always supposed to be a compile-time constant, ..." ?

Yes. We could guard against it not being so by having an initial branch where:

if ( !__builtin_constant_p(vendor) )
    return candidate & filtered_vendor;
>
>> + * A runtime check at the time of CPUID probing guarantees we never run on
>> + * wrong hardware and another check when loading CPU policies guarantees we
>> + * never run policies for a vendor in another vendor's silicon.
>> + *
>> + * By the same token, the same folding can happen when no vendor is compiled
>> + * in and the fallback path is present.
>> + */
>> +static always_inline bool x86_vendor_is(uint8_t candidate, uint8_t vendor)
>
> I fear the comment, no matter that it's pretty large already, doesn't make
> clear how this function is to be used, i.e. how for this being an "is"
> predicate the two arguments should be chosen. My typical expectation would be
> for "is" predicates to apply to a single property, with other parameters (if
> any) only being auxiliary ones. Maybe it would already help if the first
> parameter wasn't named "candidate" but e.g. "actual" (from looking at just
> the next patch). Or maybe (depending on the number of possible different
> inputs for the first parameter) there want to be a few wrappers, so the
> "single property" aspect would be achieved at use sites.
>
> Then I see no reason for the parameters to be other than unsigned int. (Same
> for the local variable then, obviously.)

I could also call it x86_vendor_in(), to mean it's a set membership check,
leaving its prototype as:

bool x86_vendor_in(unsigned int actual, unsigned int bitmap);

bitmap is a special kind because literal 0 has a special meaning (unknown). So

I'd expect x86_vendor_in(X86_VENDOR_UNKNOWN, X86_VENDOR_UNKNOWN) to return true
when UNKNOWN_CPU=y. One way to alleviate complexity would be to promote the
unknown case to a first-class bit. It's not like it's zero for any good reason.

As for the what goes in the first parameter, it's invariably the x86_vendor
field of cpuinfo_x86 (for boot_cpu_data), or of any cpu_policy.

This is meant to replace checks on vendors, so a natural (and universally used)
pattern across the codebase is to have a runtime variable checked against a
constant. Here's a longer list of patterns and expected transformations.

  from: cp->x86_vendor == X86_VENDOR_AMD
    to: x86_vendor_is(c->x86_vendor, X86_VENDOR_AMD)

  from: cp->x86_vendor != X86_VENDOR_AMD
    to: !x86_vendor_is(c->x86_vendor, X86_VENDOR_AMD)

  from: cp->x86_vendor & X86_VENDOR_AMD
    to: x86_vendor_is(c->x86_vendor, X86_VENDOR_AMD)

  from: cp->x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)
    to: x86_vendor_is(c->x86_vendor, X86_VENDOR_AMD | X86_VENDOR_HYGON)

  from: !(cp->x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON))
    to: !x86_vendor_is(cp->x86_vendor, X86_VENDOR_AMD | X86_VENDOR_HYGON)

  from: cp->x86_vendor == X86_VENDOR_UNKNOWN
    to: x86_vendor_is(cp->x86_vendor, X86_VENDOR_UNKNOWN)

And switch statements converted to if-elseif chains.

With the second argument alaways being a constant there's j


> I'm further uncertain this is a good place for the function. In the old days
> it may have been, but cpuid.[ch] now are only about guest exposure of CPUID,

This function would be used for both cpuinfo_x86 and cpu policies. Either here
or cpufeature.h works. I used to have it in x86-vendors.h, but that's exposed
to the toolstack and nothing would come out of doing that.

> when this predicate is intended to be used for both host and guest. (As I
> realize only now, this also applies to the addition patch 1 does.) One
> might think processor.h might be a good home, but we're actually trying to
> slim that one down. So one of cpufeature.h and cpufeatures.h, I guess. (Maybe
> other x86 maintainers also have thoughts here.)
>
>> +{
>> +    uint8_t filtered_vendor = vendor & X86_ENABLED_VENDORS;
>> +
>> +    if ( vendor == X86_VENDOR_UNKNOWN )
>> +    {
>> +        if ( IS_ENABLED(CONFIG_UNKNOWN_CPU) )
>> +            /* no-vendor optimisation */
>
> Nit: Comment style (also again below).
>
>> +            return X86_ENABLED_VENDORS ? vendor == candidate : true;
>
> With the surrounding if() this effectively (and more explicitly) is
>
>             return X86_ENABLED_VENDORS ? candidate == X86_VENDOR_UNKNOWN : true;

Sure.

>
> First: Would one ever pass X86_VENDOR_UNKNOWN for "vendor"? The next patch,
> for example, specifically doesn't.

I don't think so. There's definitely not any existing case atm. Still, I think
it's better to preserve the invariant that the follwing transformation:

  from: cp->x86_vendor == X86_VENDOR_X
    to: x86_vendor_is(cp->x86_vendor, X86_VENDOR_X)

holds for every vendor variant in the "everything compiled-in" case.

> And then why not shorter as
>             return !X86_ENABLED_VENDORS || candidate == X86_VENDOR_UNKNOWN;

Because I didn't think of it. I like your version better.

>
> Which raises the next question: Should we even allow a hypervisor to be built
> with X86_ENABLED_VENDORS == 0?

That's the most extreme case of "should we boot on a CPU known CPU vendor that 
has been compiled out?", the current code in the RFC uses the unknown vendor
as fallback. We could also panic. We could be trying to exercise the
"no assumptions about the vendor" paths.

It's a policy decision for you (x86 mantainers) to take. I personally think the
default path is silly in this day and age and we could get rid of it entirely.
Without it X86_ENABLED_VENDORS=0 would be indeed illegal. On that topic...

> Plus, question more on patch 1, what's the (useful) difference between a build with all vendors set to N and
> (a) UNKNOWN_CPU=n vs (b) UNKNOWN_CPU=y? With all vendor support explicitly
> turned off, all CPUs are going to be "unknown".

... (a) causes a panic because all vendor go on the unknown path that leads to
such panic, (b) has them run as if they belonged to a new unknown vendor FOOBAR.

You could do (b) to exercise the paths on unknown vendors. Or we could get rid
of it altogether and panic on compiled-out vendors. Kconfig is a middle-ground
where the default CPU path serves a not-so-theoretical purpose.

>
>> +
>> +        /* unknown-vendor-elimination optimisation */
>> +        return false;
>> +    }
>> +
>> +    /* single-vendor optimisation */
>> +    if ( !IS_ENABLED(CONFIG_UNKNOWN_CPU) &&
>> +         (ISOLATE_LSB(X86_ENABLED_VENDORS) == X86_ENABLED_VENDORS) )
>> +        return filtered_vendor == X86_ENABLED_VENDORS;
>> +
>> +    /* compiled-out-vendor-elimination optimisation */
>> +    if ( !filtered_vendor )
>> +        return false;
>> +
>> +    /*
>> +     * When checking against a single vendor, perform an equality check, as
>> +     * it yields (marginally) better codegen
>> +     */
>> +    if ( ISOLATE_LSB(filtered_vendor) == filtered_vendor )
>
> So one may pass a combination of multiple vendors for "vendor"? Is so, why
> is the parameter name singular?

Yes, it's a bitmap. Parameter could be in fact "bitmap", except the 0 case is
a special.

>
>> +        return filtered_vendor == candidate ;
>
> Nit: Stray blank.

Oops.

Cheers,
Alejandro

On 27/11/2025 1:15 pm, Alejandro Vallejo wrote:
>> Which raises the next question: Should we even allow a hypervisor to be built
>> with X86_ENABLED_VENDORS == 0?
> That's the most extreme case of "should we boot on a CPU known CPU vendor that 
> has been compiled out?", the current code in the RFC uses the unknown vendor
> as fallback. We could also panic. We could be trying to exercise the
> "no assumptions about the vendor" paths.
>
> It's a policy decision for you (x86 mantainers) to take. I personally think the
> default path is silly in this day and age and we could get rid of it entirely.
> Without it X86_ENABLED_VENDORS=0 would be indeed illegal. On that topic...

We allow compiling out both PV and HVM, so Randconfig can search for
broken corners of the abstraction.  The same principle applies here.

For running such a hypervisor, I can't see anything that would
fundamentally interfere with PV guests; PV guests are all architectural x86.

I don't like there being an explicit Kconfig option for UNKNOWN. 
UNKNOWN should simply be "didn't match anything we compiled in".

~Andrew

On Thu Nov 27, 2025 at 9:36 PM CET, Andrew Cooper wrote:
> On 27/11/2025 1:15 pm, Alejandro Vallejo wrote:
>>> Which raises the next question: Should we even allow a hypervisor to be built
>>> with X86_ENABLED_VENDORS == 0?
>> That's the most extreme case of "should we boot on a CPU known CPU vendor that 
>> has been compiled out?", the current code in the RFC uses the unknown vendor
>> as fallback. We could also panic. We could be trying to exercise the
>> "no assumptions about the vendor" paths.
>>
>> It's a policy decision for you (x86 mantainers) to take. I personally think the
>> default path is silly in this day and age and we could get rid of it entirely.
>> Without it X86_ENABLED_VENDORS=0 would be indeed illegal. On that topic...
>
> We allow compiling out both PV and HVM, so Randconfig can search for
> broken corners of the abstraction.  The same principle applies here.

That answers how to prevent that configuration from appearing in randconfig,
not whether that configuration should be valid or not. That's what I meant is a
policy decision.

As the RFC stands:

           CONFIG                      POLICY
   X86_ENABLED_VENDORS=0 | all boots use the default CPU
   UNKNOWN_CPU=y         |

   X86_ENABLED_VENDORS=0 | panic on boot
   UNKNOWN_CPU=n         |

   X86_ENABLED_VENDORS=<all> | current all-vendors-compiled-in
   UNKNOWN_CPU=y             |

   X86_ENABLED_VENDORS=<all> | Marginal code removal in early_cpu_init() of
   UNKNOWN_CPU=n             | the default CPU branch.

   X86_ENABLED_VENDORS=<more-than-1> | x86_vendor_is() folds into false when
   UNKNOWN_CPU=<any>                 | checking against all-compiled-out vendors
                                     | Also, some &-checks turn into ==-checks.

   X86_ENABLED_VENDORS=<single> | Same as the one above.
   UNKNOWN_CPU=y                |

   X86_ENABLED_VENDORS=<single> | x86_vendor_is() folds into constant true/false
   UNKNOWN_CPU=n                |

Does this sound like a credible approach? What could be different?

  - Should we remove the "default" case altogether"? That removes the Kconfig
    and simplifies matters somewhat.
  - If the default case is to stay, should we allow running with no-explicit
    vendors set?

>
> For running such a hypervisor, I can't see anything that would
> fundamentally interfere with PV guests; PV guests are all architectural x86.

You could also run HVM, seeing how SVM and VMX don't intrinsically require AMD
and INTEL respectively, but it wouldn't be pleasant, secure or safe. It would
be wonky at best. In general it's a path I wouldn't like to fallback to on the
basis that it's completely untested. I don't believe OSSTest ever exercised it
(I wouldn't even know how), nor GitlabCI, nor XenRT.

>
> I don't like there being an explicit Kconfig option for UNKNOWN. 
> UNKNOWN should simply be "didn't match anything we compiled in".

What I require for the optimisation to fully work out is precisely the removal
of that the "didn't match anything we compiled in" as a plausible successful
boot scenario in order for all x86_vendor_is(x, X86_VENDOR_AMD) to fold into
"true". I could reverse the polarity and have a REQUIRE_KNOWN_VENDOR, but that's
a non additive option that clashes with allyesconfig/allnoconfig.

>
> ~Andrew

Cheers,
Alejandro

On 27.11.2025 14:15, Alejandro Vallejo wrote:
> On Thu Nov 27, 2025 at 11:46 AM CET, Jan Beulich wrote:
>> On 26.11.2025 17:44, Alejandro Vallejo wrote:
>>> + * A runtime check at the time of CPUID probing guarantees we never run on
>>> + * wrong hardware and another check when loading CPU policies guarantees we
>>> + * never run policies for a vendor in another vendor's silicon.
>>> + *
>>> + * By the same token, the same folding can happen when no vendor is compiled
>>> + * in and the fallback path is present.
>>> + */
>>> +static always_inline bool x86_vendor_is(uint8_t candidate, uint8_t vendor)
>>
>> I fear the comment, no matter that it's pretty large already, doesn't make
>> clear how this function is to be used, i.e. how for this being an "is"
>> predicate the two arguments should be chosen. My typical expectation would be
>> for "is" predicates to apply to a single property, with other parameters (if
>> any) only being auxiliary ones. Maybe it would already help if the first
>> parameter wasn't named "candidate" but e.g. "actual" (from looking at just
>> the next patch). Or maybe (depending on the number of possible different
>> inputs for the first parameter) there want to be a few wrappers, so the
>> "single property" aspect would be achieved at use sites.
>>
>> Then I see no reason for the parameters to be other than unsigned int. (Same
>> for the local variable then, obviously.)
> 
> I could also call it x86_vendor_in(), to mean it's a set membership check,
> leaving its prototype as:
> 
> bool x86_vendor_in(unsigned int actual, unsigned int bitmap);
> 
> bitmap is a special kind because literal 0 has a special meaning (unknown). So
> 
> I'd expect x86_vendor_in(X86_VENDOR_UNKNOWN, X86_VENDOR_UNKNOWN) to return true
> when UNKNOWN_CPU=y. One way to alleviate complexity would be to promote the
> unknown case to a first-class bit. It's not like it's zero for any good reason.
> 
> As for the what goes in the first parameter, it's invariably the x86_vendor
> field of cpuinfo_x86 (for boot_cpu_data), or of any cpu_policy.
> 
> This is meant to replace checks on vendors, so a natural (and universally used)
> pattern across the codebase is to have a runtime variable checked against a
> constant. Here's a longer list of patterns and expected transformations.
> 
>   from: cp->x86_vendor == X86_VENDOR_AMD
>     to: x86_vendor_is(c->x86_vendor, X86_VENDOR_AMD)
> 
>   from: cp->x86_vendor != X86_VENDOR_AMD
>     to: !x86_vendor_is(c->x86_vendor, X86_VENDOR_AMD)
> 
>   from: cp->x86_vendor & X86_VENDOR_AMD
>     to: x86_vendor_is(c->x86_vendor, X86_VENDOR_AMD)
> 
>   from: cp->x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)
>     to: x86_vendor_is(c->x86_vendor, X86_VENDOR_AMD | X86_VENDOR_HYGON)

There's a mix of c and cp up from here, but I hope the distinction isn't
relevant in this context. What is relevant though is that you shouldn't
be using struct cpuinfo_x86's x86_vendor field anymore. See the struct
definition.

>   from: !(cp->x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON))
>     to: !x86_vendor_is(cp->x86_vendor, X86_VENDOR_AMD | X86_VENDOR_HYGON)
> 
>   from: cp->x86_vendor == X86_VENDOR_UNKNOWN
>     to: x86_vendor_is(cp->x86_vendor, X86_VENDOR_UNKNOWN)

For it to be clear what the "is" applies to, all of the above would imo
better be x86_vendor_is(c, X86_VENDOR_...) or
x86_vendor_is(cp, X86_VENDOR_...) at the call sites. The c / cp are what
I called "auxiliary data" elsewhere, and the property checked clearly is
the 2nd argument. To achieve this you could introduce a wrapper macro,
which would do the de-ref of the ->vendor field. (As a prereq, struct
cpu_policy would then also need to gain a "vendor" alias of the present
"x86_vendor" field.)

> And switch statements converted to if-elseif chains.

I've voiced concern towards this elsewhere.

>> First: Would one ever pass X86_VENDOR_UNKNOWN for "vendor"? The next patch,
>> for example, specifically doesn't.
> 
> I don't think so. There's definitely not any existing case atm. Still, I think
> it's better to preserve the invariant that the follwing transformation:
> 
>   from: cp->x86_vendor == X86_VENDOR_X
>     to: x86_vendor_is(cp->x86_vendor, X86_VENDOR_X)
> 
> holds for every vendor variant in the "everything compiled-in" case.

Otoh the code could be simplified if you simply rejected the passing of
X86_VENDOR_UNKNOWN.

>>> +    /* single-vendor optimisation */
>>> +    if ( !IS_ENABLED(CONFIG_UNKNOWN_CPU) &&
>>> +         (ISOLATE_LSB(X86_ENABLED_VENDORS) == X86_ENABLED_VENDORS) )
>>> +        return filtered_vendor == X86_ENABLED_VENDORS;
>>> +
>>> +    /* compiled-out-vendor-elimination optimisation */
>>> +    if ( !filtered_vendor )
>>> +        return false;
>>> +
>>> +    /*
>>> +     * When checking against a single vendor, perform an equality check, as
>>> +     * it yields (marginally) better codegen
>>> +     */
>>> +    if ( ISOLATE_LSB(filtered_vendor) == filtered_vendor )
>>
>> So one may pass a combination of multiple vendors for "vendor"? Is so, why
>> is the parameter name singular?
> 
> Yes, it's a bitmap. Parameter could be in fact "bitmap", except the 0 case is
> a special.

We have empty bitmaps elsewhere, as a more or less special case, so this doesn't
look overly concerning.

Jan

On Thu Nov 27, 2025 at 2:37 PM CET, Jan Beulich wrote:
> On 27.11.2025 14:15, Alejandro Vallejo wrote:
>> On Thu Nov 27, 2025 at 11:46 AM CET, Jan Beulich wrote:
>>> On 26.11.2025 17:44, Alejandro Vallejo wrote:
>>>> + * A runtime check at the time of CPUID probing guarantees we never run on
>>>> + * wrong hardware and another check when loading CPU policies guarantees we
>>>> + * never run policies for a vendor in another vendor's silicon.
>>>> + *
>>>> + * By the same token, the same folding can happen when no vendor is compiled
>>>> + * in and the fallback path is present.
>>>> + */
>>>> +static always_inline bool x86_vendor_is(uint8_t candidate, uint8_t vendor)
>>>
>>> I fear the comment, no matter that it's pretty large already, doesn't make
>>> clear how this function is to be used, i.e. how for this being an "is"
>>> predicate the two arguments should be chosen. My typical expectation would be
>>> for "is" predicates to apply to a single property, with other parameters (if
>>> any) only being auxiliary ones. Maybe it would already help if the first
>>> parameter wasn't named "candidate" but e.g. "actual" (from looking at just
>>> the next patch). Or maybe (depending on the number of possible different
>>> inputs for the first parameter) there want to be a few wrappers, so the
>>> "single property" aspect would be achieved at use sites.
>>>
>>> Then I see no reason for the parameters to be other than unsigned int. (Same
>>> for the local variable then, obviously.)
>> 
>> I could also call it x86_vendor_in(), to mean it's a set membership check,
>> leaving its prototype as:
>> 
>> bool x86_vendor_in(unsigned int actual, unsigned int bitmap);
>> 
>> bitmap is a special kind because literal 0 has a special meaning (unknown). So
>> 
>> I'd expect x86_vendor_in(X86_VENDOR_UNKNOWN, X86_VENDOR_UNKNOWN) to return true
>> when UNKNOWN_CPU=y. One way to alleviate complexity would be to promote the
>> unknown case to a first-class bit. It's not like it's zero for any good reason.
>> 
>> As for the what goes in the first parameter, it's invariably the x86_vendor
>> field of cpuinfo_x86 (for boot_cpu_data), or of any cpu_policy.
>> 
>> This is meant to replace checks on vendors, so a natural (and universally used)
>> pattern across the codebase is to have a runtime variable checked against a
>> constant. Here's a longer list of patterns and expected transformations.
>> 
>>   from: cp->x86_vendor == X86_VENDOR_AMD
>>     to: x86_vendor_is(c->x86_vendor, X86_VENDOR_AMD)
>> 
>>   from: cp->x86_vendor != X86_VENDOR_AMD
>>     to: !x86_vendor_is(c->x86_vendor, X86_VENDOR_AMD)
>> 
>>   from: cp->x86_vendor & X86_VENDOR_AMD
>>     to: x86_vendor_is(c->x86_vendor, X86_VENDOR_AMD)
>> 
>>   from: cp->x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)
>>     to: x86_vendor_is(c->x86_vendor, X86_VENDOR_AMD | X86_VENDOR_HYGON)
>
> There's a mix of c and cp up from here, but I hope the distinction isn't
> relevant in this context. What is relevant though is that you shouldn't
> be using struct cpuinfo_x86's x86_vendor field anymore. See the struct
> definition.

It indeed isn't relevant. Fair enough, I was misled by its use in cpu/common.c

>
>>   from: !(cp->x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON))
>>     to: !x86_vendor_is(cp->x86_vendor, X86_VENDOR_AMD | X86_VENDOR_HYGON)
>> 
>>   from: cp->x86_vendor == X86_VENDOR_UNKNOWN
>>     to: x86_vendor_is(cp->x86_vendor, X86_VENDOR_UNKNOWN)
>
> For it to be clear what the "is" applies to, all of the above would imo
> better be x86_vendor_is(c, X86_VENDOR_...) or
> x86_vendor_is(cp, X86_VENDOR_...) at the call sites. The c / cp are what
> I called "auxiliary data" elsewhere, and the property checked clearly is
> the 2nd argument. To achieve this you could introduce a wrapper macro,
> which would do the de-ref of the ->vendor field. (As a prereq, struct
> cpu_policy would then also need to gain a "vendor" alias of the present
> "x86_vendor" field.)

I'd be fine with that. It'd also trim the diffstat substantially by allowing
a number of cases to become single-line checks rather than splitting

>
>> And switch statements converted to if-elseif chains.
>
> I've voiced concern towards this elsewhere.
>
>>> First: Would one ever pass X86_VENDOR_UNKNOWN for "vendor"? The next patch,
>>> for example, specifically doesn't.
>> 
>> I don't think so. There's definitely not any existing case atm. Still, I think
>> it's better to preserve the invariant that the follwing transformation:
>> 
>>   from: cp->x86_vendor == X86_VENDOR_X
>>     to: x86_vendor_is(cp->x86_vendor, X86_VENDOR_X)
>> 
>> holds for every vendor variant in the "everything compiled-in" case.
>
> Otoh the code could be simplified if you simply rejected the passing of
> X86_VENDOR_UNKNOWN.

How would this rejection go? Something like this at the top?

  if ( vendor == X86_VENDOR_UNKNOWN )
      BUG();

I'd rather have something that causes a compile-time error, but I'm not sure
how to cause a compile time failure when a constant matches FOO.

Surely there must be a way...

>
>>>> +    /* single-vendor optimisation */
>>>> +    if ( !IS_ENABLED(CONFIG_UNKNOWN_CPU) &&
>>>> +         (ISOLATE_LSB(X86_ENABLED_VENDORS) == X86_ENABLED_VENDORS) )
>>>> +        return filtered_vendor == X86_ENABLED_VENDORS;
>>>> +
>>>> +    /* compiled-out-vendor-elimination optimisation */
>>>> +    if ( !filtered_vendor )
>>>> +        return false;
>>>> +
>>>> +    /*
>>>> +     * When checking against a single vendor, perform an equality check, as
>>>> +     * it yields (marginally) better codegen
>>>> +     */
>>>> +    if ( ISOLATE_LSB(filtered_vendor) == filtered_vendor )
>>>
>>> So one may pass a combination of multiple vendors for "vendor"? Is so, why
>>> is the parameter name singular?
>> 
>> Yes, it's a bitmap. Parameter could be in fact "bitmap", except the 0 case is
>> a special.
>
> We have empty bitmaps elsewhere, as a more or less special case, so this doesn't
> look overly concerning.
>
> Jan

Ack,

Cheers,
Alejandro

On 27.11.2025 15:06, Alejandro Vallejo wrote:
> On Thu Nov 27, 2025 at 2:37 PM CET, Jan Beulich wrote:
>> On 27.11.2025 14:15, Alejandro Vallejo wrote:
>>> On Thu Nov 27, 2025 at 11:46 AM CET, Jan Beulich wrote:
>>>> First: Would one ever pass X86_VENDOR_UNKNOWN for "vendor"? The next patch,
>>>> for example, specifically doesn't.
>>>
>>> I don't think so. There's definitely not any existing case atm. Still, I think
>>> it's better to preserve the invariant that the follwing transformation:
>>>
>>>   from: cp->x86_vendor == X86_VENDOR_X
>>>     to: x86_vendor_is(cp->x86_vendor, X86_VENDOR_X)
>>>
>>> holds for every vendor variant in the "everything compiled-in" case.
>>
>> Otoh the code could be simplified if you simply rejected the passing of
>> X86_VENDOR_UNKNOWN.
> 
> How would this rejection go? Something like this at the top?
> 
>   if ( vendor == X86_VENDOR_UNKNOWN )
>       BUG();
> 
> I'd rather have something that causes a compile-time error, but I'm not sure
> how to cause a compile time failure when a constant matches FOO.
> 
> Surely there must be a way...

Take a look at BUILD_ERROR(), which I think may fit here.

Jan