On Tue, Mar 10, 2026 at 5:04 PM Lukas Fittl <[email protected]> wrote:
> > 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;
One objection is that __cpuid() takes an array of 4 integers as an
argument. I think it would technically happen to work to pass a
pointer to this struct, but it seems the wrong thing to do. If you're
not a fan of macros, the other way would be an enum of indices (and
named with all caps).
> > + 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.
Actually I like the idea of a boolean better, since that would be less
churn, and maybe simpler to reason about. 0005 overwrites one of the
variables in subsequent calls, so it could be confusing for future
additions as to which one to use.
> > + 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.
Yes, I must have missed the shift, thanks.
v11-0001:
+static inline bool
+pg_cpuid_subleaf(int leaf, int subleaf, CPUIDResult *r)
+{
+#if defined(HAVE__GET_CPUID_COUNT)
+ return __get_cpuid_count(leaf, subleaf, &r->eax, &r->ebx, &r->ecx,
&r->edx) == 1;
+#elif defined(HAVE__CPUIDEX)
+ __cpuidex((int *) r, leaf, subleaf);
+ return true;
+#else
+ memset(r, 0, sizeof(CPUIDResult));
+ return false;
+#endif
+}
This needs a comment to explain the return value.
--
John Naylor
Amazon Web Services
