Re: [PATCH 2/9] util: Add cpuinfo-i386.c

[Richard Henderson <richard.henderson@linaro.org>]

On 5/18/23 02:35, Juan Quintela wrote:
> Richard Henderson <richard.henderson@linaro.org> wrote:
>> Add cpuinfo.h for i386 and x86_64, and the initialization
>> for that in util/.  Populate that with a slightly altered
>> copy of the tcg host probing code.  Other uses of cpuid.h
>> will be adjusted one patch at a time.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> 
> For what is worth my vote O:-)
> 
>> +#define CPUINFO_ALWAYS          (1u << 0)  /* so cpuinfo is nonzero */
>> +#define CPUINFO_CMOV            (1u << 1)
>> +#define CPUINFO_MOVBE           (1u << 2)
>> +#define CPUINFO_LZCNT           (1u << 3)
>> +#define CPUINFO_POPCNT          (1u << 4)
>> +#define CPUINFO_BMI1            (1u << 5)
>> +#define CPUINFO_BMI2            (1u << 6)
>> +#define CPUINFO_SSE2            (1u << 7)
>> +#define CPUINFO_SSE4            (1u << 8)
>> +#define CPUINFO_AVX1            (1u << 9)
>> +#define CPUINFO_AVX2            (1u << 10)
>> +#define CPUINFO_AVX512F         (1u << 11)
>> +#define CPUINFO_AVX512VL        (1u << 12)
>> +#define CPUINFO_AVX512BW        (1u << 13)
>> +#define CPUINFO_AVX512DQ        (1u << 14)
>> +#define CPUINFO_AVX512VBMI2     (1u << 15)
>> +#define CPUINFO_ATOMIC_VMOVDQA  (1u << 16)
>> +
>> +/* Initialized with a constructor. */
>> +extern unsigned cpuinfo;
> 
> On one hand, it is weird having a flags variable that is only 32bit.  I
> am so user to put 64 bit flags. Future proof, blah, blah, ...
> 
> On the other hand, if tcg has survived for so long with only 16 bits, it
> is inside posibility that 32bits are more than enough.

Indeed.  Nor is this an public abi that needs future-proofing -- in the event we need more 
bits, we change it.

>> +/* Called both as constructor and (possibly) via other constructors. */
>> +unsigned __attribute__((constructor)) cpuinfo_init(void)
>> +{
>> +    unsigned info = cpuinfo;
>> +
>> +    if (info) {
>> +        return info;
>> +    }
> 
> Have to look several times to this, because info "needed to be"" a
> static variable, right? O:-)

:-)


r~