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
