On Wed, Mar 4, 2026 at 5:50 AM John Naylor <[email protected]> wrote:
> -#if defined(HAVE__GET_CPUID) || defined(HAVE__GET_CPUID_COUNT)
> -#include <cpuid.h>
> -#endif
> -
> -#if defined(HAVE__CPUID) || defined(HAVE__CPUIDEX)
> +#if defined(HAVE__GET_CPUID) || defined(HAVE__GET_CPUID_COUNT) ||
> defined(HAVE__CPUIDEX)
> +#if defined(_MSC_VER)
> #include <intrin.h>
> +#else
> +#include <cpuid.h>
> +#endif /* defined(_MSC_VER) */
>
> What if HAVE__CPUID is defined but not HAVE__CPUIDEX? Is that possible?
Good point - I think technically our configure/meson checks allow the
case where only HAVE__CPUID is set.
> v10-0004:
>
> + pg_cpuid(1, exx);
>
> Since some leaves later on use hex values, so maybe they should all be
> hex for consistency.
Yep, agreed, will adjust.
>
> - /* second cpuid call on leaf 7 to check extended AVX-512 support */
> -
> - memset(exx, 0, 4 * sizeof(exx[0]));
> + /* Use leaf 7 to check TSC_ADJUST and extended AVX-512 support */
> + memset(exx, 0, 4 * sizeof(exx[0]));
>
> It wasn't previously stated outright, but I think the important point
> here is we need to memset before this call so that platforms without
> extended cpuid don't have stale values. It's not important at this
> point what the goal of the call is. [thinking] Actually it's worse,
> see below.
Hm, yeah, I misread the logic there - I think using more helper
functions will help here.
>
> Speaking of initialization, I noticed we historically used "foo =
> {0,0,0,0}" (lack of C99?), but "foo = {0}" is more concise. Not this
> patch's responsibility, but it does add a few more of this pattern. It
> would look nicer if we changed all those at some point.
Agreed, done in the next iteration.
> + X86Features[PG_RDTSCP] = (exx[3] & (1 << 27)) != 0;
>
> The style I've used elsewhere for setting features is
>
> X86Features[PG_SSE4_2] = exx[2] >> 20 & 1;
>
> But maybe it'd actually be better to have a function that can be called like
>
> x86_set_feature(PG_RDTSCP, EDX, 27);
I think your existing style is fine, I'll adjust my additions for consistency.
> ...with the appropriate register name #define's. Again, not this
> patches responsibility. Either way, it should be consistent.
>
> Elsewhere in the patch series there are comments that refer to EBX etc
> while the code looks like exx[1]. It hasn't been a problem up to now
> since there were only a couple places that used cpuid. But this
> patchset adds quite a few more, so now seems like a good time to make
> it more readable with register name symbols.
I'm not a fan of using macros for this, but what if we define
ourselves a struct like this:
typedef struct CPUIDResult
{
unsigned int eax;
unsigned int ebx;
unsigned int ecx;
unsigned int edx;
} CPUIDResult;
Then the code reads like:
pg_cpuid(0x80000001, &r);
X86Features[PG_RDTSCP] = r.edx >> 27 & 1;
>
> - /* All these features depend on OSXSAVE */
> ...
>
> + /* All relevant AVX-512 features depend on OSXSAVE */
>
> This comment was deliberately general, since other things can depend
> on it as well (e.g. AVX2 will be committed soon). Since this patch is
> adding some calls with different leaves, maybe we can say something
> like "/* leaf 7 features that depend on OSXSAVE */"?
Yeah, good point, will adjust.
>
> + if (exx[2] & (1 << 27))
> + {
> + uint32 xcr0_val = 0;
> +
>
> By moving the call to leaf 7 up, the results of leaf 1 just got blown
> away, so isn't OXSAVE support now broken? Maybe we actually want 2
> separate arrays? (Like "regs" and "ext_regs") That'll also avoid
> having to memset.
Yeah, I think using two result variables make sense here.
Alternatively we could save the result of the OXSAVE check in a
boolean.
> + level_type = (exx[2] >> 8) & 0xff;
>
> The comment says "ECX[15:8]". This looks like ECX[7:0] but maybe I'm
> missing something.
I think the code is right - bits 15 to 8 define the level type, and so
we shift that right by 8 bits, and then apply 0xff to drop anything to
the left of that.
> Looking at the above, the following could be in a separate patch(es)
> to reduce the footprint and increase readability of 0004:
>
> * add pg_cpuid
> * call leaf 7 on separate array before checking OSXSAVE
> * possibly the other cleanups mentioned above.
Yeah, makes sense to do that in an earlier patch - will post an
updated patch set later today that does that in a preparatory patch.
Thanks,
Lukas
--
Lukas Fittl
