| Message ID | 1483533584-8015-11-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 04.01.17 at 13:39, <andrew.cooper3@citrix.com> 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? > --- 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. > + }; > + }; > + 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 ... > @@ -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? Jan
On 04/01/17 15:44, Jan Beulich wrote: >>>> On 04.01.17 at 13:39, <andrew.cooper3@citrix.com> 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
>>> On 04.01.17 at 18:21, <andrew.cooper3@citrix.com> wrote: > On 04/01/17 15:44, Jan Beulich wrote: >>>>> On 04.01.17 at 13:39, <andrew.cooper3@citrix.com> 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. What's wrong with that? It's surely better than having to keep two pieces of code in sync manually. >>> @@ -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. Or have been removed, in the case of pcommit. But my point really was that the commit message might better be relaxed/extended a little in this regard. >>> + }; >>> + }; >>> + 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. Thinking about it another time - what's wrong with using uint32_t instead of bool here, allowing consecutive unknown fields to be folded? Jan
On 05/01/17 08:27, Jan Beulich wrote: >>>> On 04.01.17 at 18:21, <andrew.cooper3@citrix.com> wrote: >> On 04/01/17 15:44, Jan Beulich wrote: >>>>>> On 04.01.17 at 13:39, <andrew.cooper3@citrix.com> 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. > What's wrong with that? My specific dislike of hiding code from tools like grep and cscope. > It's surely better than having to keep two pieces of code in sync manually. True, but that doesn't come with zero cost. Thinking about it, the spanner in the works for easily generating this in an automatic way is MAWAU in leaf 7, which is the first non-bit thing in the feature leaves. >>>> + }; >>>> + }; >>>> + 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. > Thinking about it another time - what's wrong with using uint32_t > instead of bool here, allowing consecutive unknown fields to be > folded? I first tried using uint32_t and had many problems counting bits (although this less of an issue if it was automatically generated). I also wanted to maintain bool properties, but now I think back, I don't forsee any situation where we would make an assignment to one of these named features. ~Andrew
>>> On 05.01.17 at 15:53, <andrew.cooper3@citrix.com> wrote: > On 05/01/17 08:27, Jan Beulich wrote: >>>>> On 04.01.17 at 18:21, <andrew.cooper3@citrix.com> wrote: >>> On 04/01/17 15:44, Jan Beulich wrote: >>>>>>> On 04.01.17 at 13:39, <andrew.cooper3@citrix.com> 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. >> What's wrong with that? > > My specific dislike of hiding code from tools like grep and cscope. > >> It's surely better than having to keep two pieces of code in sync manually. > > True, but that doesn't come with zero cost. > > Thinking about it, the spanner in the works for easily generating this > in an automatic way is MAWAU in leaf 7, which is the first non-bit thing > in the feature leaves. That's a case needing something new anyway, as you can't even express it using XEN_CPUFEATURE(). Jan
diff --git a/xen/include/asm-x86/cpuid.h b/xen/include/asm-x86/cpuid.h index e20b0d2..0371e6e 100644 --- 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, + tsc_deadline:1, aesni:1, xsave:1, /* osxsave */:1, avx:1, f16c:1, rdrand:1, hv:1; + }; + }; + union { + uint32_t _1d; + struct { + bool fpu:1, vme:1, de:1, pse:1, tsc:1, msr:1, pae:1, mce:1, + cx8:1, /* apic */:1, :1, sep:1, mtrr:1, pge:1, mca:1, cmov:1, + pat:1, pse36:1, /* psn */:1, clflush:1, :1, ds:1, acpi:1, mmx:1, + fxsr:1, sse:1, sse2:1, /* ss */:1, htt:1, tm1:1, /* IA-64 */:1, pbe:1; + }; + }; }; }; } basic; @@ -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; + }; + }; + 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; + }; + }; + union { + uint32_t _7d0; + struct { + bool :1, avx512_4vnniw:1, avx512_4fmaps: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, :1; + }; + }; }; }; } feat; @@ -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, + :1, :1, :1, :1, :1, :1, :1, :1, + :1, :1, :1, :1, :1, :1, :1, :1, + :1, :1, :1, :1, :1, :1, :1, :1; + }; + }; + uint32_t :32, xss_low, xss_high; }; }; } xstate; @@ -142,7 +196,25 @@ struct cpuid_policy /* Leaf 0x80000001 - family/model/stepping and features. */ struct { - uint32_t :32, :32, e1c, e1d; + uint32_t :32, :32; + union { + uint32_t e1c; + struct { + bool lahf_lm:1, cmp_legacy:1, svm:1, extapic:1, cr8_legacy:1, abm:1, sse4a:1, misalignsse:1, + _3dnowprefetch:1, osvw: 1, ibs:1, xop:1, skinit:1, wdt:1, :1, lwp:1, + fma4:1, :1, :1, nodeid_msr:1, :1, tbm:1, topoext:1, :1, + :1, :1, dbext:1, :1, :1, monitorx:1, :1, :1; + }; + }; + union { + uint32_t e1d; + struct { + bool :1, :1, :1, :1, :1, :1, :1, :1, + :1, :1, :1, syscall:1, :1, :1, :1, :1, + :1, :1, :1, :1, nx:1, :1, mmxext:1, :1, + :1, ffxsr:1, page1gb:1, rdtscp:1, :1, lm:1, _3dnowext:1, _3dnow:1; + }; + }; }; uint64_t :64, :64; /* Brand string. */ @@ -153,12 +225,31 @@ struct cpuid_policy /* Leaf 0x80000007 - Advanced Power Management. */ struct { - uint32_t :32, :32, :32, e7d; + uint32_t :32, :32, :32; + union { + uint32_t e7d; + struct { + bool :1, :1, :1, :1, :1, :1, :1, :1, + itsc:1, :1, efro:1, :1, :1, :1, :1, :1, + :1, :1, :1, :1, :1, :1, :1, :1, + :1, :1, :1, :1, :1, :1, :1, :1; + }; + }; }; /* Leaf 0x80000008 - Misc addr/feature info. */ struct { - uint32_t :32, e8b, :32, :32; + uint32_t :32; + union { + uint32_t e8b; + struct { + bool clzero: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, :1, :1, :1; + }; + }; + uint32_t :32, :32; }; }; } extd;
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. No functional change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> --- xen/include/asm-x86/cpuid.h | 103 +++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 97 insertions(+), 6 deletions(-)