Re: [RFC PATCH 03/11] x86: Add x86_vendor_is() by itself before using it

[Jan Beulich <jbeulich@xxxxxxxx>]

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