Re: [Xen-devel] [PATCH 10/27] x86/cpuid: Introduce named feature bitmaps

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 04/01/17 15:44, Jan Beulich wrote:
>>>> On 04.01.17 at 13:39, <andrew.cooper3@xxxxxxxxxx> wrote:
>> Use anonymous unions to access the feature leaves as complete words, and by
>> named individual feature.
>>
>> A feature name is introduced for every architectural X86_FEATURE_*, other 
>> than
>> the dynamically calculated values such as APIC, OSXSAVE and OSPKE.
> A rationale for this change would be nice to have here, as the
> redundancy with public/arch-x86/cpufeatureset.h means any
> addition will now need to change two places. Would it be possible
> for gen-cpuid.py to generate these bitfield declarations?

Hmm.  I hadn't considered that as an option.

Thinking about it however, I'd ideally prefer not to hide the
declarations behind a macro.

>
>> --- a/xen/include/asm-x86/cpuid.h
>> +++ b/xen/include/asm-x86/cpuid.h
>> @@ -103,7 +103,25 @@ struct cpuid_policy
>>  
>>              /* Leaf 0x1 - family/model/stepping and features. */
>>              struct {
>> -                uint32_t :32, :32, _1c, _1d;
>> +                uint32_t :32, :32;
>> +                union {
>> +                    uint32_t _1c;
>> +                    struct {
>> +                        bool sse3:1, pclmulqdq:1, dtes64:1, monitor:1, 
>> dscpl:1, vmx:1, smx:1, eist:1,
>> +                             tm2:1, ssse3:1, /* cid */:1, /* sdbg */:1, 
>> fma:1, cx16:1, xtpr:1, pcdm:1,
>> +                             :1, pcid:1, dca:1, sse4_1:1, sse4_2:1, 
>> x2apic:1, movebe:1, popcnt:1,
> movbe
>
>> +                             tsc_deadline:1, aesni:1, xsave:1, /* osxsave 
>> */:1, avx:1, f16c:1, rdrand:1, hv:1;
> hypervisor (please make sure the name match up with X86_FEATURE_*).
>
>> @@ -113,7 +131,34 @@ struct cpuid_policy
>>          struct cpuid_leaf raw[CPUID_GUEST_NR_FEAT];
>>          struct {
>>              struct {
>> -                uint32_t max_subleaf, _7b0, _7c0, _7d0;
>> +                uint32_t max_subleaf;
>> +                union {
>> +                    uint32_t _7b0;
>> +                    struct {
>> +                        bool fsgsbase:1, tsc_adjust:1, sgx:1, bmi1:1, 
>> hle:1, avx2:1, fdp_excp_only:1, smep:1,
>> +                             bmi2:1, erms:1, invpcid:1, rtm:1, pqm:1, 
>> no_fpu_sel:1, mpx:1, pqe:1,
>> +                             avx512f:1, avx512dq:1, rdseed:1, adx:1, 
>> smap:1, avx512ifma:1, /* pcommit */:1, clflushopt:1,
>> +                             clwb:1, /* pt */:1, avx512pf:1, avx512er:1, 
>> avx512cd:1, sha:1, avx512bw:1, avx512vl:1;
> The commented out entries here don't match the commit message.

Well - they are not yet implemented.

>
>> +                    };
>> +                };
>> +                union {
>> +                    uint32_t _7c0;
>> +                    struct {
>> +                        bool prefetchwt1:1, avx512vbmi:1, :1, pku: 1, :1, 
>> :1, :1, :1,
>> +                             :1, :1, :1, :1, :1, :1, :1, :1,
>> +                             :1, :1, :1, :1, :1, :1, :1, :1,
>> +                             :1, :1, :1, :1, :1, :1, :1, :1;
> This is ugly, but I remember you saying (on irc?) the compiler
> doesn't allow bitfields wider than one bit for bool ...

Correct.  I was quite surprised by this, but I can understand that bool
foo:2 is quite meaningless when foo can strictly only take a binary value.

>
>> @@ -126,7 +171,16 @@ struct cpuid_policy
>>                  uint32_t xcr0_low, :32, :32, xcr0_high;
>>              };
>>              struct {
>> -                uint32_t Da1, :32, xss_low, xss_high;
>> +                union {
>> +                    uint32_t Da1;
>> +                    struct {
>> +                        bool xsaveopt: 1, xsavec: 1, xgetbv1: 1, xsaves: 1, 
>> :1, :1, :1, :1,
> Why the blanks after the colons?

Older formatting choice.  I will fix up.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel