[PATCH 0/4] x86/cpuid: Handling of synthetic cpuid_policy fields

[PATCH 0/4] x86/cpuid: Handling of synthetic cpuid_policy fields

[Andrew Cooper]

This is the next piece of CPUID work, and associated cleanup for the existing
logic.

Andrew Cooper (4):
  libx86: Introduce x86_cpuid_lookup_vendor()
  x86/cpuid: Drop get_cpu_vendor() completely
  tools/libxc: Use x86_cpuid_lookup_vendor() rather than opencoding the logic
  libx86: Recalculate synthesised cpuid_policy fields when appropriate

 tools/libxc/xc_cpuid_x86.c               | 28 ++++++--------------
 tools/tests/cpu-policy/test-cpu-policy.c | 37 ++++++++++++++++++++++++++
 xen/arch/x86/cpu/amd.c                   |  1 -
 xen/arch/x86/cpu/centaur.c               |  1 -
 xen/arch/x86/cpu/common.c                | 45 +++++++++-----------------------
 xen/arch/x86/cpu/cpu.h                   |  1 -
 xen/arch/x86/cpu/intel.c                 |  1 -
 xen/arch/x86/cpu/shanghai.c              |  1 -
 xen/arch/x86/cpuid.c                     |  7 ++---
 xen/include/asm-x86/processor.h          |  7 -----
 xen/include/asm-x86/x86-vendors.h        | 21 +++++++++++++++
 xen/include/xen/lib/x86/cpuid.h          | 14 +++++++---
 xen/lib/x86/cpuid.c                      | 43 ++++++++++++++++++++++++++++++
 xen/lib/x86/private.h                    |  1 +
 14 files changed, 138 insertions(+), 70 deletions(-)

-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 1/4] libx86: Introduce x86_cpuid_lookup_vendor()

[Andrew Cooper]

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(+)

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>
 
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 2/4] x86/cpuid: Drop get_cpu_vendor() completely

[Andrew Cooper]

get_cpu_vendor() tries to do a number of things, and ends up doing none of
them well.

For calculating the vendor itself, use x86_cpuid_lookup_vendor() which is
implemented in a far more efficient manner than looping over cpu_devs[].

For setting up this_cpu, set it up once on the BSP only, rather than
latest-takes-precident across the APs.  Such a system is probably not going to
boot, but this feels like a less dangerous course of action.  Adjust the
printed errors to be more clear in the mismatch case.

This removes the only user of cpu_dev->c_ident[], so drop that field as well.

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>
---
 xen/arch/x86/cpu/amd.c          |  1 -
 xen/arch/x86/cpu/centaur.c      |  1 -
 xen/arch/x86/cpu/common.c       | 45 ++++++++++++-----------------------------
 xen/arch/x86/cpu/cpu.h          |  1 -
 xen/arch/x86/cpu/intel.c        |  1 -
 xen/arch/x86/cpu/shanghai.c     |  1 -
 xen/arch/x86/cpuid.c            |  4 ++--
 xen/include/asm-x86/processor.h |  7 -------
 8 files changed, 15 insertions(+), 46 deletions(-)

diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index c790416..7a73d62 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -794,7 +794,6 @@ static void init_amd(struct cpuinfo_x86 *c)
 
 static const struct cpu_dev amd_cpu_dev = {
 	.c_vendor	= "AMD",
-	.c_ident 	= { "AuthenticAMD" },
 	.c_early_init	= early_init_amd,
 	.c_init		= init_amd,
 };
diff --git a/xen/arch/x86/cpu/centaur.c b/xen/arch/x86/cpu/centaur.c
index 1c760be..71f6503 100644
--- a/xen/arch/x86/cpu/centaur.c
+++ b/xen/arch/x86/cpu/centaur.c
@@ -56,7 +56,6 @@ static void init_centaur(struct cpuinfo_x86 *c)
 
 static const struct cpu_dev centaur_cpu_dev = {
 	.c_vendor	= "Centaur",
-	.c_ident	= { "CentaurHauls" },
 	.c_init		= init_centaur,
 };
 
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 53bb0a9..c69c996 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -247,36 +247,6 @@ void display_cacheinfo(struct cpuinfo_x86 *c)
 		       l2size, ecx & 0xFF);
 }
 
-int get_cpu_vendor(uint32_t b, uint32_t c, uint32_t d, enum get_cpu_vendor mode)
-{
-	int i;
-	static int printed;
-
-	for (i = 0; i < X86_VENDOR_NUM; i++) {
-		if (cpu_devs[i]) {
-			struct {
-				uint32_t b, d, c;
-			} *ptr = (void *)cpu_devs[i]->c_ident;
-
-			if (ptr->b == b && ptr->c == c && ptr->d == d) {
-				if (mode == gcv_host)
-					this_cpu = cpu_devs[i];
-				return i;
-			}
-		}
-	}
-	if (mode == gcv_guest)
-		return X86_VENDOR_UNKNOWN;
-	if (!printed) {
-		printed++;
-		printk(KERN_ERR "CPU: Vendor unknown, using generic init.\n");
-		printk(KERN_ERR "CPU: Your system may be unstable.\n");
-	}
-	this_cpu = &default_cpu;
-
-	return X86_VENDOR_UNKNOWN;
-}
-
 static inline u32 _phys_pkg_id(u32 cpuid_apic, int index_msb)
 {
 	return cpuid_apic >> index_msb;
@@ -313,7 +283,13 @@ static void __init early_cpu_detect(void)
 	*(u32 *)&c->x86_vendor_id[8] = ecx;
 	*(u32 *)&c->x86_vendor_id[4] = edx;
 
-	c->x86_vendor = get_cpu_vendor(ebx, ecx, edx, gcv_host);
+	c->x86_vendor = x86_cpuid_lookup_vendor(ebx, ecx, edx);
+	if (c->x86_vendor < ARRAY_SIZE(cpu_devs) && cpu_devs[c->x86_vendor])
+		this_cpu = cpu_devs[c->x86_vendor];
+	else
+		printk(XENLOG_ERR
+		       "Unrecognised or unsupported CPU vendor '%s'\n",
+		       c->x86_vendor_id);
 
 	cpuid(0x00000001, &eax, &ebx, &ecx, &edx);
 	c->x86 = get_cpu_family(eax, &c->x86_model, &c->x86_mask);
@@ -361,7 +337,12 @@ static void generic_identify(struct cpuinfo_x86 *c)
 	*(u32 *)&c->x86_vendor_id[8] = ecx;
 	*(u32 *)&c->x86_vendor_id[4] = edx;
 
-	c->x86_vendor = get_cpu_vendor(ebx, ecx, edx, gcv_host);
+	c->x86_vendor = x86_cpuid_lookup_vendor(ebx, ecx, edx);
+	if (boot_cpu_data.x86_vendor != c->x86_vendor)
+		printk(XENLOG_ERR "CPU%u vendor %u mismatch against BSP %u\n",
+		       smp_processor_id(), c->x86_vendor,
+		       boot_cpu_data.x86_vendor);
+
 	/* Initialize the standard set of capabilities */
 	/* Note that the vendor-specific code below might override */
 
diff --git a/xen/arch/x86/cpu/cpu.h b/xen/arch/x86/cpu/cpu.h
index 2fcb931..edc88b1 100644
--- a/xen/arch/x86/cpu/cpu.h
+++ b/xen/arch/x86/cpu/cpu.h
@@ -1,7 +1,6 @@
 /* attempt to consolidate cpu attributes */
 struct cpu_dev {
 	char	c_vendor[8];
-	char	c_ident[13];
 
 	void		(*c_early_init)(struct cpuinfo_x86 *c);
 	void		(*c_init)(struct cpuinfo_x86 * c);
diff --git a/xen/arch/x86/cpu/intel.c b/xen/arch/x86/cpu/intel.c
index 29c6b87..f9c2ec4 100644
--- a/xen/arch/x86/cpu/intel.c
+++ b/xen/arch/x86/cpu/intel.c
@@ -350,7 +350,6 @@ static void init_intel(struct cpuinfo_x86 *c)
 
 static const struct cpu_dev intel_cpu_dev = {
 	.c_vendor	= "Intel",
-	.c_ident 	= { "GenuineIntel" },
 	.c_early_init	= early_init_intel,
 	.c_init		= init_intel,
 };
diff --git a/xen/arch/x86/cpu/shanghai.c b/xen/arch/x86/cpu/shanghai.c
index 9156c85..24af5c8 100644
--- a/xen/arch/x86/cpu/shanghai.c
+++ b/xen/arch/x86/cpu/shanghai.c
@@ -17,7 +17,6 @@ static void init_shanghai(struct cpuinfo_x86 *c)
 
 static const struct cpu_dev shanghai_cpu_dev = {
     .c_vendor   = "  Shang",
-    .c_ident    = {"  Shanghai  "},
     .c_init     = init_shanghai,
 };
 
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index ab0aab6..b5eb584 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -459,8 +459,8 @@ void recalculate_cpuid_policy(struct domain *d)
     uint32_t fs[FSCAPINTS], max_fs[FSCAPINTS];
     unsigned int i;
 
-    p->x86_vendor = get_cpu_vendor(p->basic.vendor_ebx, p->basic.vendor_ecx,
-                                   p->basic.vendor_edx, gcv_guest);
+    p->x86_vendor = x86_cpuid_lookup_vendor(
+        p->basic.vendor_ebx, p->basic.vendor_ecx, p->basic.vendor_edx);
 
     p->basic.max_leaf   = min(p->basic.max_leaf,   max->basic.max_leaf);
     p->feat.max_subleaf = min(p->feat.max_subleaf, max->feat.max_subleaf);
diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index f3275ca..cef3ffb 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -579,13 +579,6 @@ int early_microcode_init(void);
 int microcode_init_intel(void);
 int microcode_init_amd(void);
 
-enum get_cpu_vendor {
-    gcv_host,
-    gcv_guest,
-};
-
-int get_cpu_vendor(uint32_t b, uint32_t c, uint32_t d, enum get_cpu_vendor mode);
-
 static inline uint8_t get_cpu_family(uint32_t raw, uint8_t *model,
                                      uint8_t *stepping)
 {
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 3/4] tools/libxc: Use x86_cpuid_lookup_vendor() rather than opencoding the logic

[Andrew Cooper]

This doesn't address any of the assumptions that "anything which isn't AMD is
Intel".  This logic is expected to be replaced wholesale with libx86 in the
longterm.

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/libxc/xc_cpuid_x86.c | 28 ++++++++--------------------
 1 file changed, 8 insertions(+), 20 deletions(-)

diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index 098affe..71e1ee7 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -32,6 +32,8 @@ enum {
 #include <xen/arch-x86/cpufeatureset.h>
 };
 
+#include <xen/asm/x86-vendors.h>
+
 #include <xen/lib/x86/cpuid.h>
 #include <xen/lib/x86/msr.h>
 
@@ -229,12 +231,7 @@ int xc_get_domain_cpu_policy(xc_interface *xch, uint32_t domid,
 
 struct cpuid_domain_info
 {
-    enum
-    {
-        VENDOR_UNKNOWN,
-        VENDOR_INTEL,
-        VENDOR_AMD,
-    } vendor;
+    unsigned int vendor; /* X86_VENDOR_* */
 
     bool hvm;
     uint64_t xfeature_mask;
@@ -296,16 +293,7 @@ static int get_cpuid_domain_info(xc_interface *xch, uint32_t domid,
     int rc;
 
     cpuid(in, regs);
-    if ( regs[1] == 0x756e6547U &&      /* "GenuineIntel" */
-         regs[2] == 0x6c65746eU &&
-         regs[3] == 0x49656e69U )
-        info->vendor = VENDOR_INTEL;
-    else if ( regs[1] == 0x68747541U && /* "AuthenticAMD" */
-              regs[2] == 0x444d4163U &&
-              regs[3] == 0x69746e65U )
-        info->vendor = VENDOR_AMD;
-    else
-        info->vendor = VENDOR_UNKNOWN;
+    info->vendor = x86_cpuid_lookup_vendor(regs[1], regs[2], regs[3]);
 
     if ( xc_domain_getinfo(xch, domid, 1, &di) != 1 ||
          di.domid != domid )
@@ -568,7 +556,7 @@ static void xc_cpuid_hvm_policy(const struct cpuid_domain_info *info,
         break;
     }
 
-    if ( info->vendor == VENDOR_AMD )
+    if ( info->vendor == X86_VENDOR_AMD )
         amd_xc_cpuid_policy(info, input, regs);
     else
         intel_xc_cpuid_policy(info, input, regs);
@@ -630,7 +618,7 @@ static void xc_cpuid_pv_policy(const struct cpuid_domain_info *info,
 
     case 0x80000000:
     {
-        unsigned int max = info->vendor == VENDOR_AMD
+        unsigned int max = info->vendor == X86_VENDOR_AMD
             ? DEF_MAX_AMDEXT : DEF_MAX_INTELEXT;
 
         if ( regs[0] > max )
@@ -736,7 +724,7 @@ static void sanitise_featureset(struct cpuid_domain_info *info)
         if ( !info->pv64 )
         {
             clear_bit(X86_FEATURE_LM, info->featureset);
-            if ( info->vendor != VENDOR_AMD )
+            if ( info->vendor != X86_VENDOR_AMD )
                 clear_bit(X86_FEATURE_SYSCALL, info->featureset);
         }
 
@@ -787,7 +775,7 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid,
     input[0] = 0x80000000;
     cpuid(input, regs);
 
-    if ( info.vendor == VENDOR_AMD )
+    if ( info.vendor == X86_VENDOR_AMD )
         ext_max = (regs[0] <= DEF_MAX_AMDEXT) ? regs[0] : DEF_MAX_AMDEXT;
     else
         ext_max = (regs[0] <= DEF_MAX_INTELEXT) ? regs[0] : DEF_MAX_INTELEXT;
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 4/4] libx86: Recalculate synthesised cpuid_policy fields when appropriate

[Andrew Cooper]

When filling a policy, either from CPUID or an incomming leaf stream,
recalculate the synthesised vendor value.  All callers are expected to want
this behaviour.

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>
---
 xen/arch/x86/cpuid.c            |  3 ++-
 xen/include/xen/lib/x86/cpuid.h |  8 +++++---
 xen/lib/x86/cpuid.c             | 11 +++++++++++
 3 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index b5eb584..cb170ac 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -272,7 +272,8 @@ static void __init calculate_raw_policy(void)
 
     x86_cpuid_policy_fill_native(p);
 
-    p->x86_vendor = boot_cpu_data.x86_vendor;
+    /* Nothing good will come from Xen and libx86 disagreeing on vendor. */
+    ASSERT(p->x86_vendor == boot_cpu_data.x86_vendor);
 }
 
 static void __init calculate_host_policy(void)
diff --git a/xen/include/xen/lib/x86/cpuid.h b/xen/include/xen/lib/x86/cpuid.h
index f392c78..c7a3bff 100644
--- a/xen/include/xen/lib/x86/cpuid.h
+++ b/xen/include/xen/lib/x86/cpuid.h
@@ -297,8 +297,9 @@ const uint32_t *x86_cpuid_lookup_deep_deps(uint32_t feature);
 /**
  * Fill a CPUID policy using the native CPUID instruction.
  *
- * No sanitisation is performed.  Values may be influenced by a hypervisor or
- * from masking/faulting configuration.
+ * No sanitisation is performed, but synthesised values are calculated.
+ * Values may be influenced by a hypervisor or from masking/faulting
+ * configuration.
  */
 void x86_cpuid_policy_fill_native(struct cpuid_policy *p);
 
@@ -339,7 +340,8 @@ int x86_cpuid_copy_to_buffer(const struct cpuid_policy *policy,
  * incoming leaf is out of range of cpuid_policy, in which case the optional
  * err_* pointers are filled to aid diagnostics.
  *
- * No content validation of in-range leaves is performed.
+ * No content validation of in-range leaves is performed.  Synthesised data is
+ * recalculated.
  */
 int x86_cpuid_copy_from_buffer(struct cpuid_policy *policy,
                                const cpuid_leaf_buffer_t leaves,
diff --git a/xen/lib/x86/cpuid.c b/xen/lib/x86/cpuid.c
index 104a867..311d19e 100644
--- a/xen/lib/x86/cpuid.c
+++ b/xen/lib/x86/cpuid.c
@@ -34,6 +34,13 @@ unsigned int x86_cpuid_lookup_vendor(uint32_t ebx, uint32_t ecx, uint32_t edx)
     return X86_VENDOR_UNKNOWN;
 }
 
+/* Recalculate the content in a CPUID policy which is derived from raw data. */
+static void recalculate_synth(struct cpuid_policy *p)
+{
+    p->x86_vendor = x86_cpuid_lookup_vendor(
+        p->basic.vendor_ebx, p->basic.vendor_ecx, p->basic.vendor_edx);
+}
+
 void x86_cpuid_policy_fill_native(struct cpuid_policy *p)
 {
     unsigned int i;
@@ -141,6 +148,8 @@ void x86_cpuid_policy_fill_native(struct cpuid_policy *p)
     for ( i = 1; i < min_t(unsigned int, ARRAY_SIZE(p->extd.raw),
                            p->extd.max_leaf + 1 - 0x80000000); ++i )
         cpuid_leaf(0x80000000 + i, &p->extd.raw[i]);
+
+    recalculate_synth(p);
 }
 
 const uint32_t *x86_cpuid_lookup_deep_deps(uint32_t feature)
@@ -363,6 +372,8 @@ int x86_cpuid_copy_from_buffer(struct cpuid_policy *p,
         }
     }
 
+    recalculate_synth(p);
+
     return 0;
 
  out_of_range:
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/4] libx86: Introduce x86_cpuid_lookup_vendor()

[Jan Beulich]

>>> 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



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/4] x86/cpuid: Drop get_cpu_vendor() completely

[Jan Beulich]

>>> On 21.03.19 at 13:21, <andrew.cooper3@citrix.com> wrote:
> get_cpu_vendor() tries to do a number of things, and ends up doing none of
> them well.
> 
> For calculating the vendor itself, use x86_cpuid_lookup_vendor() which is
> implemented in a far more efficient manner than looping over cpu_devs[].

Well, yes, the new library function is more efficient. The downside is
that the vendor specific information no longer lives in a central place
(struct cpu_dev).

> @@ -313,7 +283,13 @@ static void __init early_cpu_detect(void)
>  	*(u32 *)&c->x86_vendor_id[8] = ecx;
>  	*(u32 *)&c->x86_vendor_id[4] = edx;
>  
> -	c->x86_vendor = get_cpu_vendor(ebx, ecx, edx, gcv_host);
> +	c->x86_vendor = x86_cpuid_lookup_vendor(ebx, ecx, edx);
> +	if (c->x86_vendor < ARRAY_SIZE(cpu_devs) && cpu_devs[c->x86_vendor])
> +		this_cpu = cpu_devs[c->x86_vendor];
> +	else
> +		printk(XENLOG_ERR
> +		       "Unrecognised or unsupported CPU vendor '%s'\n",
> +		       c->x86_vendor_id);

%s happens to work because x86_vendor_id is an array of 16
characters. I'd prefer if the code here wasn't dependent on
that property plus the fact that the last 4 characters start out
as zeros and never get written to, by using %.*s instead.

Preferably with this taken care of
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Then again I wonder if we wouldn't better drop x86_vendor_id[]
altogether. It's close to unused, yet takes up NR_CPUS*16 bytes
of storage. The few uses could be replaced by a reverse function
to x86_cpuid_lookup_vendor(). I don't think we care much to be
able to "correctly" reverse X86_VENDOR_UNKNOWN.

> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -459,8 +459,8 @@ void recalculate_cpuid_policy(struct domain *d)
>      uint32_t fs[FSCAPINTS], max_fs[FSCAPINTS];
>      unsigned int i;
>  
> -    p->x86_vendor = get_cpu_vendor(p->basic.vendor_ebx, p->basic.vendor_ecx,
> -                                   p->basic.vendor_edx, gcv_guest);
> +    p->x86_vendor = x86_cpuid_lookup_vendor(
> +        p->basic.vendor_ebx, p->basic.vendor_ecx, p->basic.vendor_edx);

Personally I dislike this style of line wrapping, but I know
./CODING_STYLE doesn't say anything as to what style to use, so
your choice is going to be it here.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/4] tools/libxc: Use x86_cpuid_lookup_vendor() rather than opencoding the logic

[Jan Beulich]

>>> On 21.03.19 at 13:21, <andrew.cooper3@citrix.com> wrote:
> This doesn't address any of the assumptions that "anything which isn't AMD is
> Intel".  This logic is expected to be replaced wholesale with libx86 in the
> longterm.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/4] libx86: Recalculate synthesised cpuid_policy fields when appropriate

[Jan Beulich]

>>> On 21.03.19 at 13:21, <andrew.cooper3@citrix.com> wrote:
> When filling a policy, either from CPUID or an incomming leaf stream,
> recalculate the synthesised vendor value.  All callers are expected to want
> this behaviour.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with one question:

> @@ -141,6 +148,8 @@ void x86_cpuid_policy_fill_native(struct cpuid_policy *p)
>      for ( i = 1; i < min_t(unsigned int, ARRAY_SIZE(p->extd.raw),
>                             p->extd.max_leaf + 1 - 0x80000000); ++i )
>          cpuid_leaf(0x80000000 + i, &p->extd.raw[i]);
> +
> +    recalculate_synth(p);
>  }
>  
>  const uint32_t *x86_cpuid_lookup_deep_deps(uint32_t feature)
> @@ -363,6 +372,8 @@ int x86_cpuid_copy_from_buffer(struct cpuid_policy *p,
>          }
>      }
>  
> +    recalculate_synth(p);
> +
>      return 0;
>  
>   out_of_range:

While this takes care of libx86, wouldn't it be desirable to mirror this
behavior into update_domain_cpuid_info() right away?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/4] libx86: Recalculate synthesised cpuid_policy fields when appropriate

[Andrew Cooper]

On 26/03/2019 12:20, Jan Beulich wrote:
>>>> On 21.03.19 at 13:21, <andrew.cooper3@citrix.com> wrote:
>> When filling a policy, either from CPUID or an incomming leaf stream,
>> recalculate the synthesised vendor value.  All callers are expected to want
>> this behaviour.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with one question:
>
>> @@ -141,6 +148,8 @@ void x86_cpuid_policy_fill_native(struct cpuid_policy *p)
>>      for ( i = 1; i < min_t(unsigned int, ARRAY_SIZE(p->extd.raw),
>>                             p->extd.max_leaf + 1 - 0x80000000); ++i )
>>          cpuid_leaf(0x80000000 + i, &p->extd.raw[i]);
>> +
>> +    recalculate_synth(p);
>>  }
>>  
>>  const uint32_t *x86_cpuid_lookup_deep_deps(uint32_t feature)
>> @@ -363,6 +372,8 @@ int x86_cpuid_copy_from_buffer(struct cpuid_policy *p,
>>          }
>>      }
>>  
>> +    recalculate_synth(p);
>> +
>>      return 0;
>>  
>>   out_of_range:
> While this takes care of libx86, wouldn't it be desirable to mirror this
> behavior into update_domain_cpuid_info() right away?

That is already done by patch 2 by virtue of the modification to
recalculate_cpuid_policy()

None of this code is going to survive to the end of my CPUID/MSR work.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/4] libx86: Introduce x86_cpuid_lookup_vendor()

[Andrew Cooper]

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.)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/4] libx86: Introduce x86_cpuid_lookup_vendor()

[Jan Beulich]

>>> 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



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/4] libx86: Introduce x86_cpuid_lookup_vendor()

[Juergen Gross]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/4] libx86: Introduce x86_cpuid_lookup_vendor()

[Jan Beulich]

>>> 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



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/4] libx86: Introduce x86_cpuid_lookup_vendor()

[Juergen Gross]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/4] libx86: Introduce x86_cpuid_lookup_vendor()

[Jan Beulich]

>>> 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



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/4] x86/cpuid: Drop get_cpu_vendor() completely

[Andrew Cooper]

On 26/03/2019 12:08, Jan Beulich wrote:
>>>> On 21.03.19 at 13:21, <andrew.cooper3@citrix.com> wrote:
>> get_cpu_vendor() tries to do a number of things, and ends up doing none of
>> them well.
>>
>> For calculating the vendor itself, use x86_cpuid_lookup_vendor() which is
>> implemented in a far more efficient manner than looping over cpu_devs[].
> Well, yes, the new library function is more efficient. The downside is
> that the vendor specific information no longer lives in a central place
> (struct cpu_dev).

This is intentional.  The set of CPUs which Xen is compiled to support
is logically unrelated to what libx86 needs for vendor-specific safety.

I'm expecting almost all of cpu_dev to disappear in the long term.

>
>> @@ -313,7 +283,13 @@ static void __init early_cpu_detect(void)
>>  	*(u32 *)&c->x86_vendor_id[8] = ecx;
>>  	*(u32 *)&c->x86_vendor_id[4] = edx;
>>  
>> -	c->x86_vendor = get_cpu_vendor(ebx, ecx, edx, gcv_host);
>> +	c->x86_vendor = x86_cpuid_lookup_vendor(ebx, ecx, edx);
>> +	if (c->x86_vendor < ARRAY_SIZE(cpu_devs) && cpu_devs[c->x86_vendor])
>> +		this_cpu = cpu_devs[c->x86_vendor];
>> +	else
>> +		printk(XENLOG_ERR
>> +		       "Unrecognised or unsupported CPU vendor '%s'\n",
>> +		       c->x86_vendor_id);
> %s happens to work because x86_vendor_id is an array of 16
> characters. I'd prefer if the code here wasn't dependent on
> that property plus the fact that the last 4 characters start out
> as zeros and never get written to, by using %.*s instead.

Yeah - I spotted all of that.  I chose not to fix everything wrong with
Xen's cpu handling in this patch.

I'll switch to %.12s here, but...

>
> Preferably with this taken care of
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> Then again I wonder if we wouldn't better drop x86_vendor_id[]
> altogether. It's close to unused, yet takes up NR_CPUS*16 bytes
> of storage. The few uses could be replaced by a reverse function
> to x86_cpuid_lookup_vendor(). I don't think we care much to be
> able to "correctly" reverse X86_VENDOR_UNKNOWN.

... this wants deferring to a later change.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/4] libx86: Introduce x86_cpuid_lookup_vendor()

[Andrew Cooper]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel