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
