| Message ID | 1553170866-23812-2-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/cpuid: Handling of synthetic cpuid_policy fields | expand |
>>> On 21.03.19 at 13:21, <andrew.cooper3@citrix.com> wrote: > Also introduce constants for the vendor strings in CPUID leaf 0. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> albeit I'd appreciate if this was committed together with an actual user (other than the testsuite one) of the new function, and despite ... > --- a/xen/include/xen/lib/x86/cpuid.h > +++ b/xen/include/xen/lib/x86/cpuid.h > @@ -65,6 +65,12 @@ static inline void cpuid_count_leaf( > #undef BX_CON > #undef XCHG > > +/** > + * Given the vendor id from CPUID leaf 0, look up Xen's internal integer > + * vendor ID. Returns X86_VENDOR_UNKNOWN for any unknown vendor. > + */ > +unsigned int x86_cpuid_lookup_vendor(uint32_t ebx, uint32_t ecx, uint32_t edx); ... the undesirable (imo; I think I know you think otherwise) use of fixed width types here. Jan
On 26/03/2019 11:52, Jan Beulich wrote: >>>> On 21.03.19 at 13:21, <andrew.cooper3@citrix.com> wrote: >> Also introduce constants for the vendor strings in CPUID leaf 0. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> > albeit I'd appreciate if this was committed together with an actual > user (other than the testsuite one) of the new function, and > despite ... > >> --- a/xen/include/xen/lib/x86/cpuid.h >> +++ b/xen/include/xen/lib/x86/cpuid.h >> @@ -65,6 +65,12 @@ static inline void cpuid_count_leaf( >> #undef BX_CON >> #undef XCHG >> >> +/** >> + * Given the vendor id from CPUID leaf 0, look up Xen's internal integer >> + * vendor ID. Returns X86_VENDOR_UNKNOWN for any unknown vendor. >> + */ >> +unsigned int x86_cpuid_lookup_vendor(uint32_t ebx, uint32_t ecx, uint32_t edx); > ... the undesirable (imo; I think I know you think otherwise) use of > fixed width types here. Please, for the benefit of everyone, stop making snide remarks like this. It comes across as rude, and is off-putting to contributors. You are complaining that I didn't write code in way you would have done. Just because you dislike-but-don't-object-to how the code look doesn't make the code wrong, or worthy of comment. Your judgement of when to use which types is, in my opinion, very inconsistent. By my judgement, I am conforming to your expectation of using fixed width types when the ABI calls for it, which is the case here - the ABI is that of the CPUID instruction. If you feel strongly, then please draft a coherent and simple set of rules for CODING_STYLE. ~Andrew (Who is clearly very irritated this morning, but this does need saying.)
>>> On 26.03.19 at 14:11, <andrew.cooper3@citrix.com> wrote: > On 26/03/2019 11:52, Jan Beulich wrote: >>>>> On 21.03.19 at 13:21, <andrew.cooper3@citrix.com> wrote: >>> Also introduce constants for the vendor strings in CPUID leaf 0. >>> >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> Reviewed-by: Jan Beulich <jbeulich@suse.com> >> albeit I'd appreciate if this was committed together with an actual >> user (other than the testsuite one) of the new function, and >> despite ... >> >>> --- a/xen/include/xen/lib/x86/cpuid.h >>> +++ b/xen/include/xen/lib/x86/cpuid.h >>> @@ -65,6 +65,12 @@ static inline void cpuid_count_leaf( >>> #undef BX_CON >>> #undef XCHG >>> >>> +/** >>> + * Given the vendor id from CPUID leaf 0, look up Xen's internal integer >>> + * vendor ID. Returns X86_VENDOR_UNKNOWN for any unknown vendor. >>> + */ >>> +unsigned int x86_cpuid_lookup_vendor(uint32_t ebx, uint32_t ecx, uint32_t edx); >> ... the undesirable (imo; I think I know you think otherwise) use of >> fixed width types here. > > Please, for the benefit of everyone, stop making snide remarks like > this. It comes across as rude, and is off-putting to contributors. It coming across as rude was certainly not my intention - apologies. > You are complaining that I didn't write code in way you would have > done. Just because you dislike-but-don't-object-to how the code look > doesn't make the code wrong, or worthy of comment. That's your way of looking at it. My basic desire for consistency in how code overall looks like still made me think it was worthwhile to point this out once again (and I'm afraid I'm not going to be willing to be uniformly silent on such matters). This is because if you yourself follow what you wrote above, you'd not complain if a patch of mine was introducing a sibling function using all unsigned int (I probably wouldn't, again for consistency's sake, but I might in a somewhat more remote area of code). The end result would be a total mixture of fixed width types and basic ones, which no- one could make sense of by looking at, or even by looking at just some recent commits (in an attempt to get a feel for where we're trying to move). > Your judgement of when to use which types is, in my opinion, very > inconsistent. By my judgement, I am conforming to your expectation of > using fixed width types when the ABI calls for it, which is the case > here - the ABI is that of the CPUID instruction. I don't think I've ever said anything like this, and we've had the same dispute over CPUID in the past. Instead I think I've been pretty consistently asking to use fixed width types only where strictly needed (or where e.g. improving generated code quality). In all cases where (following the example here) unsigned int is fine, it should be preferred over uint32_t (due to our assumption that sizeof(unsigned int) >= 4). The only ABI relevance I can see here is wrt the public interface - there fixed width types should indeed be used (almost) everywhere, to make the interfaces sufficiently portable. > If you feel strongly, then please draft a coherent and simple set of > rules for CODING_STYLE. https://lists.xenproject.org/archives/html/xen-devel/2018-11/msg02853.html Note how the half sentence in parentheses is already something I added without being wholeheartedly convinced: I don't think values read from or written to registers strongly have such a requirement. This may be needed for the variables directly handed to asm()-s, but not for values which have originally come from a register (like CPUID output), but then get handed on. I've also made attempts in other directions. They've all been either completely ignored (like the one above) or turned down. Seeing how we disagree here, I don't think it's worth my time making another attempt, just to see you veto or everyone ignore it. Jan
On 26/03/2019 15:07, Jan Beulich wrote: >>>> On 26.03.19 at 14:11, <andrew.cooper3@citrix.com> wrote: >> On 26/03/2019 11:52, Jan Beulich wrote: >>>>>> On 21.03.19 at 13:21, <andrew.cooper3@citrix.com> wrote: >>>> Also introduce constants for the vendor strings in CPUID leaf 0. >>>> >>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> Reviewed-by: Jan Beulich <jbeulich@suse.com> >>> albeit I'd appreciate if this was committed together with an actual >>> user (other than the testsuite one) of the new function, and >>> despite ... >>> >>>> --- a/xen/include/xen/lib/x86/cpuid.h >>>> +++ b/xen/include/xen/lib/x86/cpuid.h >>>> @@ -65,6 +65,12 @@ static inline void cpuid_count_leaf( >>>> #undef BX_CON >>>> #undef XCHG >>>> >>>> +/** >>>> + * Given the vendor id from CPUID leaf 0, look up Xen's internal integer >>>> + * vendor ID. Returns X86_VENDOR_UNKNOWN for any unknown vendor. >>>> + */ >>>> +unsigned int x86_cpuid_lookup_vendor(uint32_t ebx, uint32_t ecx, uint32_t edx); >>> ... the undesirable (imo; I think I know you think otherwise) use of >>> fixed width types here. >> >> Please, for the benefit of everyone, stop making snide remarks like >> this. It comes across as rude, and is off-putting to contributors. > > It coming across as rude was certainly not my intention - apologies. > >> You are complaining that I didn't write code in way you would have >> done. Just because you dislike-but-don't-object-to how the code look >> doesn't make the code wrong, or worthy of comment. > > That's your way of looking at it. My basic desire for consistency in > how code overall looks like still made me think it was worthwhile to > point this out once again (and I'm afraid I'm not going to be willing > to be uniformly silent on such matters). This is because if you > yourself follow what you wrote above, you'd not complain if a > patch of mine was introducing a sibling function using all unsigned > int (I probably wouldn't, again for consistency's sake, but I might > in a somewhat more remote area of code). The end result would > be a total mixture of fixed width types and basic ones, which no- > one could make sense of by looking at, or even by looking at just > some recent commits (in an attempt to get a feel for where we're > trying to move). > >> Your judgement of when to use which types is, in my opinion, very >> inconsistent. By my judgement, I am conforming to your expectation of >> using fixed width types when the ABI calls for it, which is the case >> here - the ABI is that of the CPUID instruction. > > I don't think I've ever said anything like this, and we've had the same > dispute over CPUID in the past. Instead I think I've been pretty > consistently asking to use fixed width types only where strictly > needed (or where e.g. improving generated code quality). In all cases > where (following the example here) unsigned int is fine, it should be > preferred over uint32_t (due to our assumption that > sizeof(unsigned int) >= 4). The only ABI relevance I can see here is > wrt the public interface - there fixed width types should indeed be > used (almost) everywhere, to make the interfaces sufficiently portable. IMO especially in the CPUID case it is desirable to explicitly specify the width of the data. Looking at nodes 0x80000002 and following this should be rather clear (and I even think get_model_name() should be modified to use a pointer to uint32_t instead of unsigned int). Using a type with size >= 4 doesn't fit really well. You want size == 4. Juergen
>>> On 26.03.19 at 15:23, <jgross@suse.com> wrote: > IMO especially in the CPUID case it is desirable to explicitly specify > the width of the data. Looking at nodes 0x80000002 and following this > should be rather clear (and I even think get_model_name() should be > modified to use a pointer to uint32_t instead of unsigned int). Using > a type with size >= 4 doesn't fit really well. You want size == 4. Why? Fixed width types only introduce unnecessary restrictions when wanting to re-use code in other environments. And I don't see why CPUID nodes 0x8000000[234] would be any better (or worse) as an example here. If anything they tell us that neither uint32_t nor unsigned int are right, and it should be char[4] or uint8_t[4] instead (depending on whether we want to tie ourselves to CHAR_BIT == 8, which clearly is more restrictive than sizeof(int) >= 4, but otoh is also less likely to get in the way). Jan
On 26/03/2019 15:39, Jan Beulich wrote: >>>> On 26.03.19 at 15:23, <jgross@suse.com> wrote: >> IMO especially in the CPUID case it is desirable to explicitly specify >> the width of the data. Looking at nodes 0x80000002 and following this >> should be rather clear (and I even think get_model_name() should be >> modified to use a pointer to uint32_t instead of unsigned int). Using >> a type with size >= 4 doesn't fit really well. You want size == 4. > > Why? Fixed width types only introduce unnecessary restrictions > when wanting to re-use code in other environments. And I don't > see why CPUID nodes 0x8000000[234] would be any better (or > worse) as an example here. If anything they tell us that neither > uint32_t nor unsigned int are right, and it should be char[4] or > uint8_t[4] instead (depending on whether we want to tie > ourselves to CHAR_BIT == 8, which clearly is more restrictive > than sizeof(int) >= 4, but otoh is also less likely to get in the > way). You are using a little endian specific point of view. That's the only reason the current code would work with a non-fixed width specification. The char[4] variant would work only with a union where the other member would need to be a fixed width type like uint32_t. Juergen
>>> On 26.03.19 at 15:47, <jgross@suse.com> wrote: > On 26/03/2019 15:39, Jan Beulich wrote: >>>>> On 26.03.19 at 15:23, <jgross@suse.com> wrote: >>> IMO especially in the CPUID case it is desirable to explicitly specify >>> the width of the data. Looking at nodes 0x80000002 and following this >>> should be rather clear (and I even think get_model_name() should be >>> modified to use a pointer to uint32_t instead of unsigned int). Using >>> a type with size >= 4 doesn't fit really well. You want size == 4. >> >> Why? Fixed width types only introduce unnecessary restrictions >> when wanting to re-use code in other environments. And I don't >> see why CPUID nodes 0x8000000[234] would be any better (or >> worse) as an example here. If anything they tell us that neither >> uint32_t nor unsigned int are right, and it should be char[4] or >> uint8_t[4] instead (depending on whether we want to tie >> ourselves to CHAR_BIT == 8, which clearly is more restrictive >> than sizeof(int) >= 4, but otoh is also less likely to get in the >> way). > > You are using a little endian specific point of view. Of course - it's x86 code when we talk about x86 CPUID. For the architecture independent aspect I agree. > That's the only > reason the current code would work with a non-fixed width specification. > > The char[4] variant would work only with a union where the other member > would need to be a fixed width type like uint32_t. I think we're talking of slightly different things (and I agree that get_model_name() would need switching to uint32_t, if we wanted to allow for sizeof(int) > 4). My original point though was towards cases where values (not pointers to values) obtained from CPUID get passed around as function arguments. I don't see why the respective function parameters would need to be uint32_t, irrespective of endianness. For functions taking pointers moving away from uint32_t could be doable as well, but might then require more care at certain call sites (like get_model_name()), e.g. by needing to go through intermediate variables. Whether that's beneficial or harmful to code overall would need to be determined on a case by case basis, unless maximum consistency was the primary goal. Jan
On 26/03/2019 14:39, Jan Beulich wrote: >>>> On 26.03.19 at 15:23, <jgross@suse.com> wrote: >> IMO especially in the CPUID case it is desirable to explicitly specify >> the width of the data. Looking at nodes 0x80000002 and following this >> should be rather clear (and I even think get_model_name() should be >> modified to use a pointer to uint32_t instead of unsigned int). Using >> a type with size >= 4 doesn't fit really well. You want size == 4. > Why? Fixed width types only introduce unnecessary restrictions > when wanting to re-use code in other environments. And I don't > see why CPUID nodes 0x8000000[234] would be any better (or > worse) as an example here. If anything they tell us that neither > uint32_t nor unsigned int are right, and it should be char[4] or > uint8_t[4] instead (depending on whether we want to tie > ourselves to CHAR_BIT == 8, which clearly is more restrictive > than sizeof(int) >= 4, but otoh is also less likely to get in the > way). The ABI of CPUID really is 2x uint32_t input, 4x uint32_t output. It is only by current convention that the data is also valid ASCII, and there is absolutely nothing which prevents a new vendor choosing to put non-ASCII content in their id string. Given the recent developments from China, I'm slightly surprised that we haven't seen any UTF-8 (or UTF-16 for that matter) yet. ~Andrew
diff --git a/tools/tests/cpu-policy/test-cpu-policy.c b/tools/tests/cpu-policy/test-cpu-policy.c index d13963e..beced5e 100644 --- a/tools/tests/cpu-policy/test-cpu-policy.c +++ b/tools/tests/cpu-policy/test-cpu-policy.c @@ -8,6 +8,7 @@ #include <err.h> #include <xen-tools/libs.h> +#include <xen/asm/x86-vendors.h> #include <xen/lib/x86/cpuid.h> #include <xen/lib/x86/msr.h> #include <xen/domctl.h> @@ -19,6 +20,40 @@ static unsigned int nr_failures; printf(fmt, ##__VA_ARGS__); \ }) +static void test_vendor_identification(void) +{ + static const struct test { + union { + char ident[12]; + struct { + uint32_t b, d, c; + }; + }; + unsigned int vendor; + } tests[] = { + { { "GenuineIntel" }, X86_VENDOR_INTEL }, + { { "AuthenticAMD" }, X86_VENDOR_AMD }, + { { "CentaurHauls" }, X86_VENDOR_CENTAUR }, + { { " Shanghai " }, X86_VENDOR_SHANGHAI }, + + { { "" }, X86_VENDOR_UNKNOWN }, + { { " " }, X86_VENDOR_UNKNOWN }, + { { "xxxxxxxxxxxx" }, X86_VENDOR_UNKNOWN }, + }; + + printf("Testing CPU vendor identification:\n"); + + for ( size_t i = 0; i < ARRAY_SIZE(tests); ++i ) + { + const struct test *t = &tests[i]; + unsigned int vendor = x86_cpuid_lookup_vendor(t->b, t->c, t->d); + + if ( vendor != t->vendor ) + fail(" Test '%.12s', expected vendor %u, got %u\n", + t->ident, t->vendor, vendor); + } +} + static void test_cpuid_serialise_success(void) { static const struct test { @@ -243,6 +278,8 @@ int main(int argc, char **argv) { printf("CPU Policy unit tests\n"); + test_vendor_identification(); + test_cpuid_serialise_success(); test_msr_serialise_success(); diff --git a/xen/include/asm-x86/x86-vendors.h b/xen/include/asm-x86/x86-vendors.h index 38a81c3..774ceac 100644 --- a/xen/include/asm-x86/x86-vendors.h +++ b/xen/include/asm-x86/x86-vendors.h @@ -3,12 +3,33 @@ /* * CPU vendor IDs + * + * - X86_VENDOR_* are Xen-internal identifiers. Values and order are + * arbitrary. + * - X86_VENDOR_*_E?X are architectural information from CPUID leaf 0 */ #define X86_VENDOR_UNKNOWN 0 + #define X86_VENDOR_INTEL 1 +#define X86_VENDOR_INTEL_EBX 0x756e6547U /* "GenuineIntel" */ +#define X86_VENDOR_INTEL_ECX 0x6c65746eU +#define X86_VENDOR_INTEL_EDX 0x49656e69U + #define X86_VENDOR_AMD 2 +#define X86_VENDOR_AMD_EBX 0x68747541U /* "AuthenticAMD" */ +#define X86_VENDOR_AMD_ECX 0x444d4163U +#define X86_VENDOR_AMD_EDX 0x69746e65U + #define X86_VENDOR_CENTAUR 3 +#define X86_VENDOR_CENTAUR_EBX 0x746e6543U /* "CentaurHauls" */ +#define X86_VENDOR_CENTAUR_ECX 0x736c7561U +#define X86_VENDOR_CENTAUR_EDX 0x48727561U + #define X86_VENDOR_SHANGHAI 4 +#define X86_VENDOR_SHANGHAI_EBX 0x68532020U /* " Shanghai " */ +#define X86_VENDOR_SHANGHAI_ECX 0x20206961U +#define X86_VENDOR_SHANGHAI_EDX 0x68676e61U + #define X86_VENDOR_NUM 5 #endif /* __XEN_X86_VENDORS_H__ */ diff --git a/xen/include/xen/lib/x86/cpuid.h b/xen/include/xen/lib/x86/cpuid.h index 95b37b6..f392c78 100644 --- a/xen/include/xen/lib/x86/cpuid.h +++ b/xen/include/xen/lib/x86/cpuid.h @@ -65,6 +65,12 @@ static inline void cpuid_count_leaf( #undef BX_CON #undef XCHG +/** + * Given the vendor id from CPUID leaf 0, look up Xen's internal integer + * vendor ID. Returns X86_VENDOR_UNKNOWN for any unknown vendor. + */ +unsigned int x86_cpuid_lookup_vendor(uint32_t ebx, uint32_t ecx, uint32_t edx); + #define CPUID_GUEST_NR_BASIC (0xdu + 1) #define CPUID_GUEST_NR_CACHE (5u + 1) #define CPUID_GUEST_NR_FEAT (0u + 1) diff --git a/xen/lib/x86/cpuid.c b/xen/lib/x86/cpuid.c index 6c60ba8..104a867 100644 --- a/xen/lib/x86/cpuid.c +++ b/xen/lib/x86/cpuid.c @@ -2,6 +2,38 @@ #include <xen/lib/x86/cpuid.h> +unsigned int x86_cpuid_lookup_vendor(uint32_t ebx, uint32_t ecx, uint32_t edx) +{ + switch ( ebx ) + { + case X86_VENDOR_INTEL_EBX: + if ( ecx == X86_VENDOR_INTEL_ECX && + edx == X86_VENDOR_INTEL_EDX ) + return X86_VENDOR_INTEL; + break; + + case X86_VENDOR_AMD_EBX: + if ( ecx == X86_VENDOR_AMD_ECX && + edx == X86_VENDOR_AMD_EDX ) + return X86_VENDOR_AMD; + break; + + case X86_VENDOR_CENTAUR_EBX: + if ( ecx == X86_VENDOR_CENTAUR_ECX && + edx == X86_VENDOR_CENTAUR_EDX ) + return X86_VENDOR_CENTAUR; + break; + + case X86_VENDOR_SHANGHAI_EBX: + if ( ecx == X86_VENDOR_SHANGHAI_ECX && + edx == X86_VENDOR_SHANGHAI_EDX ) + return X86_VENDOR_SHANGHAI; + break; + } + + return X86_VENDOR_UNKNOWN; +} + void x86_cpuid_policy_fill_native(struct cpuid_policy *p) { unsigned int i; diff --git a/xen/lib/x86/private.h b/xen/lib/x86/private.h index 6fb5022..f5b195e 100644 --- a/xen/lib/x86/private.h +++ b/xen/lib/x86/private.h @@ -23,6 +23,7 @@ #include <stddef.h> #include <xen/asm/msr-index.h> +#include <xen/asm/x86-vendors.h> #include <xen-tools/libs.h>
Also introduce constants for the vendor strings in CPUID leaf 0. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wei.liu2@citrix.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Sergey Dyasli <sergey.dyasli@citrix.com> --- tools/tests/cpu-policy/test-cpu-policy.c | 37 ++++++++++++++++++++++++++++++++ xen/include/asm-x86/x86-vendors.h | 21 ++++++++++++++++++ xen/include/xen/lib/x86/cpuid.h | 6 ++++++ xen/lib/x86/cpuid.c | 32 +++++++++++++++++++++++++++ xen/lib/x86/private.h | 1 + 5 files changed, 97 insertions(+)