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
