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
