[PATCH 0/5] x86/cpu: Rework of X86_VENDOR_* constants

[PATCH 0/5] x86/cpu: Rework of X86_VENDOR_* constants

[Andrew Cooper]

Pu: I'm sorry to keep on breaking the code you're working, but I think the
changes here will simplify your Hygon series somewhat.  See patch 3 for the
best example.

To apologise, I've also rebased your v5 Patch 1 over the changes here (patch 4
in this series), to save you the work.  I'll also fix up any patches in your
v5 series on commit, if there are no further review comments, to save you
sending out a v6.

Andrew Cooper (4):
  x86/cpu: Drop cpu_devs[] and $VENDOR_init_cpu() hooks
  x86/cpu: Introduce x86_cpuid_vendor_to_str() and drop cpu_dev.c_vendor[]
  x86/cpu: Renumber X86_VENDOR_* to form a bitmap
  x86/msr: Fix handling of MSR_AMD_PATCHLEVEL/MSR_IA32_UCODE_REV

Pu Wen (1):
  x86/cpu: Create Hygon Dhyana architecture support file

 tools/tests/cpu-policy/test-cpu-policy.c |   1 +
 xen/arch/x86/acpi/cpufreq/cpufreq.c      |   3 +-
 xen/arch/x86/acpi/suspend.c              |   6 +-
 xen/arch/x86/cpu/Makefile                |   1 +
 xen/arch/x86/cpu/amd.c                   |  11 +---
 xen/arch/x86/cpu/centaur.c               |   9 +--
 xen/arch/x86/cpu/common.c                |  28 ++++----
 xen/arch/x86/cpu/cpu.h                   |  10 +--
 xen/arch/x86/cpu/hygon.c                 | 107 +++++++++++++++++++++++++++++++
 xen/arch/x86/cpu/intel.c                 |  12 +---
 xen/arch/x86/cpu/shanghai.c              |   9 +--
 xen/arch/x86/msr.c                       |  34 ++++++++++
 xen/arch/x86/pv/emul-priv-op.c           |  25 +-------
 xen/arch/x86/x86_64/traps.c              |   3 +-
 xen/include/asm-x86/x86-vendors.h        |  18 ++++--
 xen/include/xen/lib/x86/cpuid.h          |   6 ++
 xen/lib/x86/cpuid.c                      |  19 ++++++
 17 files changed, 203 insertions(+), 99 deletions(-)
 create mode 100644 xen/arch/x86/cpu/hygon.c

-- 
2.1.4


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

[PATCH 1/5] x86/cpu: Drop cpu_devs[] and $VENDOR_init_cpu() hooks

[Andrew Cooper]

These helpers each fill in a single cpu_devs[] pointer, and since c/s
00b4f4d0f "x86/cpuid: Drop get_cpu_vendor() completely", this array is read
exactly once on boot.

Delete the hooks and cpu_devs[], and have early_cpu_detect() pick the
appropriate cpu_dev structure directly.

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: David Wang <davidwang@zhaoxin.com>
CC: Pu Wen <puwen@hygon.cn>
---
 xen/arch/x86/cpu/amd.c      |  8 +-------
 xen/arch/x86/cpu/centaur.c  |  8 +-------
 xen/arch/x86/cpu/common.c   | 16 +++++++---------
 xen/arch/x86/cpu/cpu.h      |  8 ++------
 xen/arch/x86/cpu/intel.c    | 11 +----------
 xen/arch/x86/cpu/shanghai.c |  8 +-------
 6 files changed, 13 insertions(+), 46 deletions(-)

diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index 7a73d62..d58952b 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -792,14 +792,8 @@ static void init_amd(struct cpuinfo_x86 *c)
 	check_syscfg_dram_mod_en();
 }
 
-static const struct cpu_dev amd_cpu_dev = {
+const struct cpu_dev amd_cpu_dev = {
 	.c_vendor	= "AMD",
 	.c_early_init	= early_init_amd,
 	.c_init		= init_amd,
 };
-
-int __init amd_init_cpu(void)
-{
-	cpu_devs[X86_VENDOR_AMD] = &amd_cpu_dev;
-	return 0;
-}
diff --git a/xen/arch/x86/cpu/centaur.c b/xen/arch/x86/cpu/centaur.c
index 71f6503..268f4d4 100644
--- a/xen/arch/x86/cpu/centaur.c
+++ b/xen/arch/x86/cpu/centaur.c
@@ -54,13 +54,7 @@ static void init_centaur(struct cpuinfo_x86 *c)
 		init_c3(c);
 }
 
-static const struct cpu_dev centaur_cpu_dev = {
+const struct cpu_dev centaur_cpu_dev = {
 	.c_vendor	= "Centaur",
 	.c_init		= init_centaur,
 };
-
-int __init centaur_init_cpu(void)
-{
-	cpu_devs[X86_VENDOR_CENTAUR] = &centaur_cpu_dev;
-	return 0;
-}
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index b2249b5..4cf9ec2 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -42,8 +42,6 @@ unsigned int __read_mostly levelling_caps;
 DEFINE_PER_CPU(struct cpuidmasks, cpuidmasks);
 struct cpuidmasks __read_mostly cpuidmask_defaults;
 
-const struct cpu_dev *__read_mostly cpu_devs[X86_VENDOR_NUM] = {};
-
 unsigned int paddr_bits __read_mostly = 36;
 unsigned int hap_paddr_bits __read_mostly = 36;
 unsigned int vaddr_bits __read_mostly = VADDR_BITS;
@@ -284,12 +282,16 @@ static void __init early_cpu_detect(void)
 	*(u32 *)&c->x86_vendor_id[4] = edx;
 
 	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
+	switch (c->x86_vendor) {
+	case X86_VENDOR_INTEL:	  this_cpu = &intel_cpu_dev;    break;
+	case X86_VENDOR_AMD:	  this_cpu = &amd_cpu_dev;      break;
+	case X86_VENDOR_CENTAUR:  this_cpu = &centaur_cpu_dev;  break;
+	case X86_VENDOR_SHANGHAI: this_cpu = &shanghai_cpu_dev; break;
+	default:
 		printk(XENLOG_ERR
 		       "Unrecognised or unsupported CPU vendor '%.12s'\n",
 		       c->x86_vendor_id);
+	}
 
 	cpuid(0x00000001, &eax, &ebx, &ecx, &edx);
 	c->x86 = get_cpu_family(eax, &c->x86_model, &c->x86_mask);
@@ -687,10 +689,6 @@ static cpumask_t cpu_initialized;
 
 void __init early_cpu_init(void)
 {
-	intel_cpu_init();
-	amd_init_cpu();
-	centaur_init_cpu();
-	shanghai_init_cpu();
 	early_cpu_detect();
 }
 
diff --git a/xen/arch/x86/cpu/cpu.h b/xen/arch/x86/cpu/cpu.h
index edc88b1..62e4b03 100644
--- a/xen/arch/x86/cpu/cpu.h
+++ b/xen/arch/x86/cpu/cpu.h
@@ -6,7 +6,8 @@ struct cpu_dev {
 	void		(*c_init)(struct cpuinfo_x86 * c);
 };
 
-extern const struct cpu_dev *cpu_devs[X86_VENDOR_NUM];
+extern const struct cpu_dev intel_cpu_dev, amd_cpu_dev, centaur_cpu_dev,
+    shanghai_cpu_dev;
 
 extern bool_t opt_arat;
 extern unsigned int opt_cpuid_mask_ecx, opt_cpuid_mask_edx;
@@ -15,8 +16,3 @@ extern unsigned int opt_cpuid_mask_ext_ecx, opt_cpuid_mask_ext_edx;
 
 extern int get_model_name(struct cpuinfo_x86 *c);
 extern void display_cacheinfo(struct cpuinfo_x86 *c);
-
-int intel_cpu_init(void);
-int amd_init_cpu(void);
-int centaur_init_cpu(void);
-int shanghai_init_cpu(void);
diff --git a/xen/arch/x86/cpu/intel.c b/xen/arch/x86/cpu/intel.c
index f9c2ec4..fcb3708 100644
--- a/xen/arch/x86/cpu/intel.c
+++ b/xen/arch/x86/cpu/intel.c
@@ -348,17 +348,8 @@ static void init_intel(struct cpuinfo_x86 *c)
 		__set_bit(X86_FEATURE_ARAT, c->x86_capability);
 }
 
-static const struct cpu_dev intel_cpu_dev = {
+const struct cpu_dev intel_cpu_dev = {
 	.c_vendor	= "Intel",
 	.c_early_init	= early_init_intel,
 	.c_init		= init_intel,
 };
-
-int __init intel_cpu_init(void)
-{
-	cpu_devs[X86_VENDOR_INTEL] = &intel_cpu_dev;
-	return 0;
-}
-
-// arch_initcall(intel_cpu_init);
-
diff --git a/xen/arch/x86/cpu/shanghai.c b/xen/arch/x86/cpu/shanghai.c
index 24af5c8..189e13e 100644
--- a/xen/arch/x86/cpu/shanghai.c
+++ b/xen/arch/x86/cpu/shanghai.c
@@ -15,13 +15,7 @@ static void init_shanghai(struct cpuinfo_x86 *c)
     init_intel_cacheinfo(c);
 }
 
-static const struct cpu_dev shanghai_cpu_dev = {
+const struct cpu_dev shanghai_cpu_dev = {
     .c_vendor   = "  Shang",
     .c_init     = init_shanghai,
 };
-
-int __init shanghai_init_cpu(void)
-{
-    cpu_devs[X86_VENDOR_SHANGHAI] = &shanghai_cpu_dev;
-    return 0;
-}
-- 
2.1.4


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

[PATCH 2/5] x86/cpu: Introduce x86_cpuid_vendor_to_str() and drop cpu_dev.c_vendor[]

[Andrew Cooper]

cpu_dev.c_vendor[] is a char[8] array which is printed using %s in two
locations.  This leads to subtle lack-of-NUL bugs when using an 8 character
vendor name.

Introduce x86_cpuid_vendor_to_str() to turn an x86_vendor into a printable
string, use it in the two locations that c_vendor is used, and drop c_vendor.

This drops the final user of X86_VENDOR_NUM, so drop that 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: David Wang <davidwang@zhaoxin.com>
CC: Pu Wen <puwen@hygon.cn>

David: Same question as Wei asked.  Presuambly, " Shang" is a mistake, and
"Shanghai" is the correct term to use?
---
 xen/arch/x86/cpu/amd.c            |  1 -
 xen/arch/x86/cpu/centaur.c        |  1 -
 xen/arch/x86/cpu/common.c         | 11 +++--------
 xen/arch/x86/cpu/cpu.h            |  2 --
 xen/arch/x86/cpu/intel.c          |  1 -
 xen/arch/x86/cpu/shanghai.c       |  1 -
 xen/include/asm-x86/x86-vendors.h |  2 --
 xen/include/xen/lib/x86/cpuid.h   |  6 ++++++
 xen/lib/x86/cpuid.c               | 12 ++++++++++++
 9 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index d58952b..e19a5ea 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -793,7 +793,6 @@ static void init_amd(struct cpuinfo_x86 *c)
 }
 
 const struct cpu_dev amd_cpu_dev = {
-	.c_vendor	= "AMD",
 	.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 268f4d4..34a5bfc 100644
--- a/xen/arch/x86/cpu/centaur.c
+++ b/xen/arch/x86/cpu/centaur.c
@@ -55,6 +55,5 @@ static void init_centaur(struct cpuinfo_x86 *c)
 }
 
 const struct cpu_dev centaur_cpu_dev = {
-	.c_vendor	= "Centaur",
 	.c_init		= init_centaur,
 };
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 4cf9ec2..7cc45fe 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -102,7 +102,6 @@ static void default_init(struct cpuinfo_x86 * c)
 
 static const struct cpu_dev default_cpu = {
 	.c_init	= default_init,
-	.c_vendor = "Unknown",
 };
 static const struct cpu_dev *this_cpu = &default_cpu;
 
@@ -306,7 +305,7 @@ static void __init early_cpu_detect(void)
 
 	printk(XENLOG_INFO
 	       "CPU Vendor: %s, Family %u (%#x), Model %u (%#x), Stepping %u (raw %08x)\n",
-	       this_cpu->c_vendor, c->x86, c->x86,
+	       x86_cpuid_vendor_to_str(c->x86_vendor), c->x86, c->x86,
 	       c->x86_model, c->x86_model, c->x86_mask, eax);
 
 	eax = cpuid_eax(0x80000000);
@@ -661,12 +660,8 @@ void print_cpu_info(unsigned int cpu)
 
 	printk("CPU%u: ", cpu);
 
-	if (c->x86_vendor < X86_VENDOR_NUM)
-		vendor = this_cpu->c_vendor;
-	else
-		vendor = c->x86_vendor_id;
-
-	if (vendor && strncmp(c->x86_model_id, vendor, strlen(vendor)))
+	vendor = x86_cpuid_vendor_to_str(c->x86_vendor);
+	if (strncmp(c->x86_model_id, vendor, strlen(vendor)))
 		printk("%s ", vendor);
 
 	if (!c->x86_model_id[0])
diff --git a/xen/arch/x86/cpu/cpu.h b/xen/arch/x86/cpu/cpu.h
index 62e4b03..54bd0d3 100644
--- a/xen/arch/x86/cpu/cpu.h
+++ b/xen/arch/x86/cpu/cpu.h
@@ -1,7 +1,5 @@
 /* attempt to consolidate cpu attributes */
 struct cpu_dev {
-	char	c_vendor[8];
-
 	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 fcb3708..0dd8f98 100644
--- a/xen/arch/x86/cpu/intel.c
+++ b/xen/arch/x86/cpu/intel.c
@@ -349,7 +349,6 @@ static void init_intel(struct cpuinfo_x86 *c)
 }
 
 const struct cpu_dev intel_cpu_dev = {
-	.c_vendor	= "Intel",
 	.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 189e13e..08a81f0 100644
--- a/xen/arch/x86/cpu/shanghai.c
+++ b/xen/arch/x86/cpu/shanghai.c
@@ -16,6 +16,5 @@ static void init_shanghai(struct cpuinfo_x86 *c)
 }
 
 const struct cpu_dev shanghai_cpu_dev = {
-    .c_vendor   = "  Shang",
     .c_init     = init_shanghai,
 };
diff --git a/xen/include/asm-x86/x86-vendors.h b/xen/include/asm-x86/x86-vendors.h
index 774ceac..fca7396 100644
--- a/xen/include/asm-x86/x86-vendors.h
+++ b/xen/include/asm-x86/x86-vendors.h
@@ -30,6 +30,4 @@
 #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 c7a3bff..022757f 100644
--- a/xen/include/xen/lib/x86/cpuid.h
+++ b/xen/include/xen/lib/x86/cpuid.h
@@ -71,6 +71,12 @@ static inline void cpuid_count_leaf(
  */
 unsigned int x86_cpuid_lookup_vendor(uint32_t ebx, uint32_t ecx, uint32_t edx);
 
+/**
+ * Given Xen's internal vendor ID, return a string suitable for printing.
+ * Returns "Unknown" for any unrecognised ID.
+ */
+const char *x86_cpuid_vendor_to_str(unsigned int vendor);
+
 #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 311d19e..23619c7 100644
--- a/xen/lib/x86/cpuid.c
+++ b/xen/lib/x86/cpuid.c
@@ -34,6 +34,18 @@ unsigned int x86_cpuid_lookup_vendor(uint32_t ebx, uint32_t ecx, uint32_t edx)
     return X86_VENDOR_UNKNOWN;
 }
 
+const char *x86_cpuid_vendor_to_str(unsigned int vendor)
+{
+    switch ( vendor )
+    {
+    case X86_VENDOR_INTEL:    return "Intel";
+    case X86_VENDOR_AMD:      return "AMD";
+    case X86_VENDOR_CENTAUR:  return "Centaur";
+    case X86_VENDOR_SHANGHAI: return "Shanghai";
+    default:                  return "Unknown";
+    }
+}
+
 /* Recalculate the content in a CPUID policy which is derived from raw data. */
 static void recalculate_synth(struct cpuid_policy *p)
 {
-- 
2.1.4


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

[PATCH 3/5] x86/cpu: Renumber X86_VENDOR_* to form a bitmap

[Andrew Cooper]

CPUs from different vendors sometimes share characteristics.  All users of
X86_VENDOR_* are now direct equal/not-equal comparisons.  By expressing the
X86_VENDOR_* constants in a bitmap fashon, we can more concicely and
efficiently test whether a vendor is one of a group.

Update all parts of the code which can already benefit from this improvement.

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: David Wang <davidwang@zhaoxin.com>
CC: Pu Wen <puwen@hygon.cn>

The 3 locations which are INTEL | CENTAUR are all to do with SYSENTER MSR
handling, which I expect should include SHANGHAI.  I expect this is also true
of !hvm_long_mode_active() handling of X86_FEATURE_SYSCALL.
---
 xen/arch/x86/acpi/cpufreq/cpufreq.c |  3 +--
 xen/arch/x86/acpi/suspend.c         |  6 ++----
 xen/arch/x86/pv/emul-priv-op.c      |  3 +--
 xen/arch/x86/x86_64/traps.c         |  3 +--
 xen/include/asm-x86/x86-vendors.h   | 13 +++++++------
 5 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/xen/arch/x86/acpi/cpufreq/cpufreq.c b/xen/arch/x86/acpi/cpufreq/cpufreq.c
index 844ab85..f4e13e1 100644
--- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
+++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
@@ -661,8 +661,7 @@ int cpufreq_cpu_init(unsigned int cpuid)
     int ret;
 
     /* Currently we only handle Intel and AMD processor */
-    if ( (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL ) ||
-         (boot_cpu_data.x86_vendor == X86_VENDOR_AMD ) )
+    if ( boot_cpu_data.x86_vendor & (X86_VENDOR_INTEL | X86_VENDOR_AMD) )
         ret = cpufreq_add_cpu(cpuid);
     else
         ret = -EFAULT;
diff --git a/xen/arch/x86/acpi/suspend.c b/xen/arch/x86/acpi/suspend.c
index 00e6012..9e69bf2 100644
--- a/xen/arch/x86/acpi/suspend.c
+++ b/xen/arch/x86/acpi/suspend.c
@@ -27,8 +27,7 @@ void save_rest_processor_state(void)
     rdmsrl(MSR_SHADOW_GS_BASE, saved_kernel_gs_base);
     rdmsrl(MSR_CSTAR, saved_cstar);
     rdmsrl(MSR_LSTAR, saved_lstar);
-    if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL ||
-         boot_cpu_data.x86_vendor == X86_VENDOR_CENTAUR )
+    if ( boot_cpu_data.x86_vendor & (X86_VENDOR_INTEL | X86_VENDOR_CENTAUR) )
     {
         rdmsrl(MSR_IA32_SYSENTER_ESP, saved_sysenter_esp);
         rdmsrl(MSR_IA32_SYSENTER_EIP, saved_sysenter_eip);
@@ -52,8 +51,7 @@ void restore_rest_processor_state(void)
     wrgsbase(saved_gs_base);
     wrmsrl(MSR_SHADOW_GS_BASE, saved_kernel_gs_base);
 
-    if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL ||
-         boot_cpu_data.x86_vendor == X86_VENDOR_CENTAUR )
+    if ( boot_cpu_data.x86_vendor & (X86_VENDOR_INTEL | X86_VENDOR_CENTAUR) )
     {
         /* Recover sysenter MSRs */
         wrmsrl(MSR_IA32_SYSENTER_ESP, saved_sysenter_esp);
diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index 84ce67c..f594065 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -1066,8 +1066,7 @@ static int write_msr(unsigned int reg, uint64_t val,
 
     case MSR_IA32_MPERF:
     case MSR_IA32_APERF:
-        if ( (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL) &&
-             (boot_cpu_data.x86_vendor != X86_VENDOR_AMD) )
+        if ( !(boot_cpu_data.x86_vendor & (X86_VENDOR_INTEL | X86_VENDOR_AMD)) )
             break;
         if ( likely(!is_cpufreq_controller(currd)) ||
              wrmsr_safe(reg, val) == 0 )
diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
index bf7870e..44af765 100644
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -334,8 +334,7 @@ void subarch_percpu_traps_init(void)
                                    (unsigned long)lstar_enter);
     stub_va += offset;
 
-    if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL ||
-         boot_cpu_data.x86_vendor == X86_VENDOR_CENTAUR )
+    if ( boot_cpu_data.x86_vendor & (X86_VENDOR_INTEL | X86_VENDOR_CENTAUR) )
     {
         /* SYSENTER entry. */
         wrmsrl(MSR_IA32_SYSENTER_ESP, stack_bottom);
diff --git a/xen/include/asm-x86/x86-vendors.h b/xen/include/asm-x86/x86-vendors.h
index fca7396..1ecb934 100644
--- a/xen/include/asm-x86/x86-vendors.h
+++ b/xen/include/asm-x86/x86-vendors.h
@@ -4,28 +4,29 @@
 /*
  * CPU vendor IDs
  *
- * - X86_VENDOR_* are Xen-internal identifiers.  Values and order are
- *   arbitrary.
+ * - X86_VENDOR_* are Xen-internal identifiers.  The order is arbitrary, but
+ *   values form a bitmap so vendor checks can be made against multiple
+ *   vendors at once.
  * - 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 (1 << 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 (1 << 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 (1 << 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 (1 << 4)
 #define X86_VENDOR_SHANGHAI_EBX 0x68532020U /* "  Shanghai  " */
 #define X86_VENDOR_SHANGHAI_ECX 0x20206961U
 #define X86_VENDOR_SHANGHAI_EDX 0x68676e61U
-- 
2.1.4


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

[PATCH 4/5] x86/cpu: Create Hygon Dhyana architecture support file

[Andrew Cooper]

From: Pu Wen <puwen@hygon.cn>

Add x86 architecture support for a new processor: Hygon Dhyana Family
18h. To make Hygon initialization flow more clear, carve out code from
amd.c into a separate file hygon.c, and remove unnecessary code for
Hygon Dhyana.

To identify Hygon Dhyana CPU, add a new vendor type X86_VENDOR_HYGON
and vendor ID "HygonGenuine" for system recognition, and fit the new
x86 vendor lookup mechanism.

Hygon can fully use the function early_init_amd(), so make this common
function non-static and direct call it from Hygon code.

Add a separate hygon_get_topology(), which calculate phys_proc_id from
AcpiId[6](see reference [1]).

Reference:
[1] https://git.kernel.org/tip/e0ceeae708cebf22c990c3d703a4ca187dc837f5

Signed-off-by: Pu Wen <puwen@hygon.cn>
---
 tools/tests/cpu-policy/test-cpu-policy.c |   1 +
 xen/arch/x86/cpu/Makefile                |   1 +
 xen/arch/x86/cpu/amd.c                   |   2 +-
 xen/arch/x86/cpu/common.c                |   1 +
 xen/arch/x86/cpu/cpu.h                   |   4 +-
 xen/arch/x86/cpu/hygon.c                 | 107 +++++++++++++++++++++++++++++++
 xen/include/asm-x86/x86-vendors.h        |   5 ++
 xen/lib/x86/cpuid.c                      |   7 ++
 8 files changed, 126 insertions(+), 2 deletions(-)
 create mode 100644 xen/arch/x86/cpu/hygon.c

diff --git a/tools/tests/cpu-policy/test-cpu-policy.c b/tools/tests/cpu-policy/test-cpu-policy.c
index beced5e..88f5121 100644
--- a/tools/tests/cpu-policy/test-cpu-policy.c
+++ b/tools/tests/cpu-policy/test-cpu-policy.c
@@ -35,6 +35,7 @@ static void test_vendor_identification(void)
         { { "AuthenticAMD" }, X86_VENDOR_AMD },
         { { "CentaurHauls" }, X86_VENDOR_CENTAUR },
         { { "  Shanghai  " }, X86_VENDOR_SHANGHAI },
+        { { "HygonGenuine" }, X86_VENDOR_HYGON },
 
         { { ""             }, X86_VENDOR_UNKNOWN },
         { { "            " }, X86_VENDOR_UNKNOWN },
diff --git a/xen/arch/x86/cpu/Makefile b/xen/arch/x86/cpu/Makefile
index 34a01ca..466acc8 100644
--- a/xen/arch/x86/cpu/Makefile
+++ b/xen/arch/x86/cpu/Makefile
@@ -4,6 +4,7 @@ subdir-y += mtrr
 obj-y += amd.o
 obj-y += centaur.o
 obj-y += common.o
+obj-y += hygon.o
 obj-y += intel.o
 obj-y += intel_cacheinfo.o
 obj-y += mwait-idle.o
diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index e19a5ea..3966560 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -526,7 +526,7 @@ static void amd_get_topology(struct cpuinfo_x86 *c)
                                                           : c->cpu_core_id);
 }
 
-static void early_init_amd(struct cpuinfo_x86 *c)
+void early_init_amd(struct cpuinfo_x86 *c)
 {
 	if (c == &boot_cpu_data)
 		amd_init_levelling();
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 7cc45fe..89d3a7b 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -286,6 +286,7 @@ static void __init early_cpu_detect(void)
 	case X86_VENDOR_AMD:	  this_cpu = &amd_cpu_dev;      break;
 	case X86_VENDOR_CENTAUR:  this_cpu = &centaur_cpu_dev;  break;
 	case X86_VENDOR_SHANGHAI: this_cpu = &shanghai_cpu_dev; break;
+	case X86_VENDOR_HYGON:    this_cpu = &hygon_cpu_dev;    break;
 	default:
 		printk(XENLOG_ERR
 		       "Unrecognised or unsupported CPU vendor '%.12s'\n",
diff --git a/xen/arch/x86/cpu/cpu.h b/xen/arch/x86/cpu/cpu.h
index 54bd0d3..30cd3a8 100644
--- a/xen/arch/x86/cpu/cpu.h
+++ b/xen/arch/x86/cpu/cpu.h
@@ -5,7 +5,7 @@ struct cpu_dev {
 };
 
 extern const struct cpu_dev intel_cpu_dev, amd_cpu_dev, centaur_cpu_dev,
-    shanghai_cpu_dev;
+    shanghai_cpu_dev, hygon_cpu_dev;
 
 extern bool_t opt_arat;
 extern unsigned int opt_cpuid_mask_ecx, opt_cpuid_mask_edx;
@@ -14,3 +14,5 @@ extern unsigned int opt_cpuid_mask_ext_ecx, opt_cpuid_mask_ext_edx;
 
 extern int get_model_name(struct cpuinfo_x86 *c);
 extern void display_cacheinfo(struct cpuinfo_x86 *c);
+
+void early_init_amd(struct cpuinfo_x86 *c);
diff --git a/xen/arch/x86/cpu/hygon.c b/xen/arch/x86/cpu/hygon.c
new file mode 100644
index 0000000..9ab7aa8
--- /dev/null
+++ b/xen/arch/x86/cpu/hygon.c
@@ -0,0 +1,107 @@
+#include <xen/init.h>
+#include <asm/processor.h>
+#include <asm/hvm/support.h>
+#include <asm/spec_ctrl.h>
+
+#include "cpu.h"
+
+#define APICID_SOCKET_ID_BIT 6
+
+static void hygon_get_topology(struct cpuinfo_x86 *c)
+{
+	unsigned int ebx;
+
+	if (c->x86_max_cores <= 1)
+		return;
+
+	/* Socket ID is ApicId[6] for Hygon processors. */
+	c->phys_proc_id >>= APICID_SOCKET_ID_BIT;
+
+	ebx = cpuid_ebx(0x8000001e);
+	c->x86_num_siblings = ((ebx >> 8) & 0x3) + 1;
+	c->x86_max_cores /= c->x86_num_siblings;
+	c->cpu_core_id = ebx & 0xff;
+
+	if (opt_cpu_info)
+	        printk("CPU %d(%d) -> Processor %d, Core %d\n",
+	                smp_processor_id(), c->x86_max_cores,
+	                        c->phys_proc_id, c->cpu_core_id);
+}
+
+static void init_hygon(struct cpuinfo_x86 *c)
+{
+	unsigned long long value;
+
+	/*
+	 * Attempt to set lfence to be Dispatch Serialising.  This MSR almost
+	 * certainly isn't virtualised (and Xen at least will leak the real
+	 * value in but silently discard writes), as well as being per-core
+	 * rather than per-thread, so do a full safe read/write/readback cycle
+	 * in the worst case.
+	 */
+	if (rdmsr_safe(MSR_AMD64_DE_CFG, value))
+		/* Unable to read.  Assume the safer default. */
+		__clear_bit(X86_FEATURE_LFENCE_DISPATCH,
+			    c->x86_capability);
+	else if (value & AMD64_DE_CFG_LFENCE_SERIALISE)
+		/* Already dispatch serialising. */
+		__set_bit(X86_FEATURE_LFENCE_DISPATCH,
+			  c->x86_capability);
+	else if (wrmsr_safe(MSR_AMD64_DE_CFG,
+			    value | AMD64_DE_CFG_LFENCE_SERIALISE) ||
+		 rdmsr_safe(MSR_AMD64_DE_CFG, value) ||
+		 !(value & AMD64_DE_CFG_LFENCE_SERIALISE))
+		/* Attempt to set failed.  Assume the safer default. */
+		__clear_bit(X86_FEATURE_LFENCE_DISPATCH,
+			    c->x86_capability);
+	else
+		/* Successfully enabled! */
+		__set_bit(X86_FEATURE_LFENCE_DISPATCH,
+			  c->x86_capability);
+
+	/*
+	 * If the user has explicitly chosen to disable Memory Disambiguation
+	 * to mitigiate Speculative Store Bypass, poke the appropriate MSR.
+	 */
+	if (opt_ssbd && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) {
+		value |= 1ull << 10;
+		wrmsr_safe(MSR_AMD64_LS_CFG, value);
+	}
+
+	/* MFENCE stops RDTSC speculation */
+	if (!cpu_has_lfence_dispatch)
+		__set_bit(X86_FEATURE_MFENCE_RDTSC, c->x86_capability);
+
+	display_cacheinfo(c);
+
+	if (c->extended_cpuid_level >= 0x80000008)
+		c->x86_max_cores = (cpuid_ecx(0x80000008) & 0xff) + 1;
+
+	if (c->extended_cpuid_level >= 0x80000007) {
+		if (cpu_has(c, X86_FEATURE_ITSC)) {
+			__set_bit(X86_FEATURE_CONSTANT_TSC, c->x86_capability);
+			__set_bit(X86_FEATURE_NONSTOP_TSC, c->x86_capability);
+			__set_bit(X86_FEATURE_TSC_RELIABLE, c->x86_capability);
+		}
+	}
+
+	hygon_get_topology(c);
+
+	/* Hygon CPUs do not support SYSENTER outside of legacy mode. */
+	__clear_bit(X86_FEATURE_SEP, c->x86_capability);
+
+	/* Hygon processors have APIC timer running in deep C states. */
+	if (opt_arat)
+		__set_bit(X86_FEATURE_ARAT, c->x86_capability);
+
+	if (cpu_has(c, X86_FEATURE_EFRO)) {
+		rdmsrl(MSR_K7_HWCR, value);
+		value |= (1 << 27); /* Enable read-only APERF/MPERF bit */
+		wrmsrl(MSR_K7_HWCR, value);
+	}
+}
+
+const struct cpu_dev hygon_cpu_dev = {
+	.c_early_init	= early_init_amd,
+	.c_init		= init_hygon,
+};
diff --git a/xen/include/asm-x86/x86-vendors.h b/xen/include/asm-x86/x86-vendors.h
index 1ecb934..5414753 100644
--- a/xen/include/asm-x86/x86-vendors.h
+++ b/xen/include/asm-x86/x86-vendors.h
@@ -31,4 +31,9 @@
 #define X86_VENDOR_SHANGHAI_ECX 0x20206961U
 #define X86_VENDOR_SHANGHAI_EDX 0x68676e61U
 
+#define X86_VENDOR_HYGON (1 << 5)
+#define X86_VENDOR_HYGON_EBX 0x6f677948 /* "HygonGenuine" */
+#define X86_VENDOR_HYGON_ECX 0x656e6975
+#define X86_VENDOR_HYGON_EDX 0x6e65476e
+
 #endif	/* __XEN_X86_VENDORS_H__ */
diff --git a/xen/lib/x86/cpuid.c b/xen/lib/x86/cpuid.c
index 23619c7..876ade6 100644
--- a/xen/lib/x86/cpuid.c
+++ b/xen/lib/x86/cpuid.c
@@ -29,6 +29,12 @@ unsigned int x86_cpuid_lookup_vendor(uint32_t ebx, uint32_t ecx, uint32_t edx)
              edx == X86_VENDOR_SHANGHAI_EDX )
             return X86_VENDOR_SHANGHAI;
         break;
+
+    case X86_VENDOR_HYGON_EBX:
+        if ( ecx == X86_VENDOR_HYGON_ECX &&
+             edx == X86_VENDOR_HYGON_EDX )
+            return X86_VENDOR_HYGON;
+        break;
     }
 
     return X86_VENDOR_UNKNOWN;
@@ -42,6 +48,7 @@ const char *x86_cpuid_vendor_to_str(unsigned int vendor)
     case X86_VENDOR_AMD:      return "AMD";
     case X86_VENDOR_CENTAUR:  return "Centaur";
     case X86_VENDOR_SHANGHAI: return "Shanghai";
+    case X86_VENDOR_HYGON:    return "Hygon";
     default:                  return "Unknown";
     }
 }
-- 
2.1.4


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

[PATCH 5/5] x86/msr: Fix handling of MSR_AMD_PATCHLEVEL/MSR_IA32_UCODE_REV

[Andrew Cooper]

There are a number of bugs.  There are no read/write hooks on the HVM side, so
guest accesses fall into the "read/write-discard" defaults, which bypass the
correct faulting behaviour and the Intel special case.

For the PV side, writes are discarded (again, bypassing proper faulting),
except for a pinned dom0, which is permitted to actually write the values
other than 0.  This is pointless with read hook implementing the Intel special
case.

However, implementing the Intel special case is itself pointless.  First of
all, OS software can't guarentee to read back 0 in the first place, because a)
this behaviour isn't guarenteed in the SDM, and b) there are SMM handlers
which use the CPUID instruction.  Secondly, when a guest executes CPUID, this
doesn't typically result in Xen executing a CPUID instruction in practice.

With the dom0 special case removed, there are now no writes to this MSR other
than Xen's microcode loading facilities, which means that the value held in
the MSR will be properly up-to-date.  Forward it directly, without jumping
through any hoops.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>

v2:
 * Rebase over X86_VENDOR_* bitmap series, to simplify the guest_rdmsr()
   expression.

The migration case is complicated.  A guest which re-evaluates its idea of the
world may find something completely different, but this is probably less bad
letting it see the wrong microcode version.  The cross-vendor case is even
worse, because Intel and AMD report the version in different 32bit words in
this MSR.  The result here is fairly close to current behaviour.
---
 xen/arch/x86/msr.c             | 34 ++++++++++++++++++++++++++++++++++
 xen/arch/x86/pv/emul-priv-op.c | 22 ----------------------
 2 files changed, 34 insertions(+), 22 deletions(-)

diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 4df4a59..3f299af 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -135,6 +135,27 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
         /* Not offered to guests. */
         goto gp_fault;
 
+    case MSR_AMD_PATCHLEVEL:
+        BUILD_BUG_ON(MSR_IA32_UCODE_REV != MSR_AMD_PATCHLEVEL);
+        /*
+         * AMD and Intel use the same MSR for the current microcode version.
+         *
+         * There is no need to jump through the SDM-provided hoops for Intel.
+         * A guest might itself perform the "write 0, CPUID, read" sequence,
+         * but servicing the CPUID for the guest typically wont result in
+         * actually executing a CPUID instruction.
+         *
+         * As a guest can't influence the value of this MSR, the value will be
+         * from Xen's last microcode load, which can be forwarded straight to
+         * the guest.
+         */
+        if ( !(cp->x86_vendor & (X86_VENDOR_INTEL |X86_VENDOR_AMD)) ||
+             !(boot_cpu_data.x86_vendor &
+               (X86_VENDOR_INTEL | X86_VENDOR_AMD)) ||
+             rdmsr_safe(MSR_AMD_PATCHLEVEL, *val) )
+            goto gp_fault;
+        break;
+
     case MSR_SPEC_CTRL:
         if ( !cp->feat.ibrsb )
             goto gp_fault;
@@ -236,6 +257,19 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
         /* Not offered to guests. */
         goto gp_fault;
 
+    case MSR_AMD_PATCHLEVEL:
+        BUILD_BUG_ON(MSR_IA32_UCODE_REV != MSR_AMD_PATCHLEVEL);
+        /*
+         * AMD and Intel use the same MSR for the current microcode version.
+         *
+         * Both document it as read-only.  However Intel also document that,
+         * for backwards compatiblity, the OS should write 0 to it before
+         * trying to access the current microcode version.
+         */
+        if ( d->arch.cpuid->x86_vendor != X86_VENDOR_INTEL || val != 0 )
+            goto gp_fault;
+        break;
+
     case MSR_AMD_PATCHLOADER:
         /*
          * See note on MSR_IA32_UCODE_WRITE below, which may or may not apply
diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index f594065..a55a400 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -893,17 +893,6 @@ static int read_msr(unsigned int reg, uint64_t *val,
         *val = 0;
         return X86EMUL_OKAY;
 
-    case MSR_IA32_UCODE_REV:
-        BUILD_BUG_ON(MSR_IA32_UCODE_REV != MSR_AMD_PATCHLEVEL);
-        if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL )
-        {
-            if ( wrmsr_safe(MSR_IA32_UCODE_REV, 0) )
-                break;
-            /* As documented in the SDM: Do a CPUID 1 here */
-            cpuid_eax(1);
-        }
-        goto normal;
-
     case MSR_IA32_MISC_ENABLE:
         rdmsrl(reg, *val);
         *val = guest_misc_enable(*val);
@@ -1047,17 +1036,6 @@ static int write_msr(unsigned int reg, uint64_t val,
             return X86EMUL_OKAY;
         break;
 
-    case MSR_IA32_UCODE_REV:
-        if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL )
-            break;
-        if ( !is_hwdom_pinned_vcpu(curr) )
-            return X86EMUL_OKAY;
-        if ( rdmsr_safe(reg, temp) )
-            break;
-        if ( val )
-            goto invalid;
-        return X86EMUL_OKAY;
-
     case MSR_IA32_MISC_ENABLE:
         rdmsrl(reg, temp);
         if ( val != guest_misc_enable(temp) )
-- 
2.1.4


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

Re: [PATCH 1/5] x86/cpu: Drop cpu_devs[] and $VENDOR_init_cpu() hooks

[Jan Beulich]

>>> On 04.04.19 at 22:26, <andrew.cooper3@citrix.com> wrote:
> These helpers each fill in a single cpu_devs[] pointer, and since c/s
> 00b4f4d0f "x86/cpuid: Drop get_cpu_vendor() completely", this array is read
> exactly once on boot.
> 
> Delete the hooks and cpu_devs[], and have early_cpu_detect() pick the
> appropriate cpu_dev structure directly.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

> @@ -687,10 +689,6 @@ static cpumask_t cpu_initialized;
>  
>  void __init early_cpu_init(void)
>  {
> -	intel_cpu_init();
> -	amd_init_cpu();
> -	centaur_init_cpu();
> -	shanghai_init_cpu();
>  	early_cpu_detect();
>  }

I think this function should then go away as well, unless you have
plans to put new stuff into it. To avoid modifying the caller,
perhaps best to simply renamed early_cpu_detect()? My R-b
stands with this change, if you decide to fold it in.

Jan



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

Re: [PATCH 2/5] x86/cpu: Introduce x86_cpuid_vendor_to_str() and drop cpu_dev.c_vendor[]

[Jan Beulich]

>>> On 04.04.19 at 22:26, <andrew.cooper3@citrix.com> wrote:
> cpu_dev.c_vendor[] is a char[8] array which is printed using %s in two
> locations.  This leads to subtle lack-of-NUL bugs when using an 8 character
> vendor name.
> 
> Introduce x86_cpuid_vendor_to_str() to turn an x86_vendor into a printable
> string, use it in the two locations that c_vendor is used, and drop 
> c_vendor.
> 
> This drops the final user of X86_VENDOR_NUM, so drop that as well.
> 
> 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 3/5] x86/cpu: Renumber X86_VENDOR_* to form a bitmap

[Jan Beulich]

>>> On 04.04.19 at 22:26, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/include/asm-x86/x86-vendors.h
> +++ b/xen/include/asm-x86/x86-vendors.h
> @@ -4,28 +4,29 @@
>  /*
>   * CPU vendor IDs
>   *
> - * - X86_VENDOR_* are Xen-internal identifiers.  Values and order are
> - *   arbitrary.
> + * - X86_VENDOR_* are Xen-internal identifiers.  The order is arbitrary, but
> + *   values form a bitmap so vendor checks can be made against multiple
> + *   vendors at once.
>   * - 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 (1 << 1)

Any reason you start from a shift count of 1, instead of 0, when
"unknown" simply is no bit set? We're not about to run out of bits,
but preferably with this corrected
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan



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

Re: [PATCH 4/5] x86/cpu: Create Hygon Dhyana architecture support file

[Jan Beulich]

>>> On 04.04.19 at 22:26, <andrew.cooper3@citrix.com> wrote:
> +static void init_hygon(struct cpuinfo_x86 *c)
> +{
> +	unsigned long long value;
> +
> +	/*
> +	 * Attempt to set lfence to be Dispatch Serialising.  This MSR almost
> +	 * certainly isn't virtualised (and Xen at least will leak the real
> +	 * value in but silently discard writes), as well as being per-core
> +	 * rather than per-thread, so do a full safe read/write/readback cycle
> +	 * in the worst case.
> +	 */
> +	if (rdmsr_safe(MSR_AMD64_DE_CFG, value))
> +		/* Unable to read.  Assume the safer default. */
> +		__clear_bit(X86_FEATURE_LFENCE_DISPATCH,
> +			    c->x86_capability);
> +	else if (value & AMD64_DE_CFG_LFENCE_SERIALISE)
> +		/* Already dispatch serialising. */
> +		__set_bit(X86_FEATURE_LFENCE_DISPATCH,
> +			  c->x86_capability);
> +	else if (wrmsr_safe(MSR_AMD64_DE_CFG,
> +			    value | AMD64_DE_CFG_LFENCE_SERIALISE) ||
> +		 rdmsr_safe(MSR_AMD64_DE_CFG, value) ||
> +		 !(value & AMD64_DE_CFG_LFENCE_SERIALISE))
> +		/* Attempt to set failed.  Assume the safer default. */
> +		__clear_bit(X86_FEATURE_LFENCE_DISPATCH,
> +			    c->x86_capability);
> +	else
> +		/* Successfully enabled! */
> +		__set_bit(X86_FEATURE_LFENCE_DISPATCH,
> +			  c->x86_capability);

Down the road we may want to make this another helper function
shared by AMD any Hygon code.

> +	/*
> +	 * If the user has explicitly chosen to disable Memory Disambiguation
> +	 * to mitigiate Speculative Store Bypass, poke the appropriate MSR.
> +	 */
> +	if (opt_ssbd && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) {
> +		value |= 1ull << 10;
> +		wrmsr_safe(MSR_AMD64_LS_CFG, value);
> +	}

Like the above, this lacks a model check. Is it really expected for
all future Hygon models to supports both in exactly the same
fashion? Even if there's just a small chance of this not being the
case, rather than fiddling with MSRs which have an entirely
different meaning in future models, I'd prefer if model checks
were added in cases like these.

I do realize that in the former case there's effectively an "all
models except a few" logic already in the original AMD code,
which I would too question whether it's really desirable. After
all we've learned recently that MSRs indeed can go away
(albeit in that case it was a change to the MSR simply becoming
meaningless, rather than obtaining a new meaning). Based on
this I could accept just the SSBD logic to gain a model check.

Jan



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

Re: [PATCH 5/5] x86/msr: Fix handling of MSR_AMD_PATCHLEVEL/MSR_IA32_UCODE_REV

[Jan Beulich]

>>> On 04.04.19 at 22:26, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -135,6 +135,27 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
>          /* Not offered to guests. */
>          goto gp_fault;
>  
> +    case MSR_AMD_PATCHLEVEL:
> +        BUILD_BUG_ON(MSR_IA32_UCODE_REV != MSR_AMD_PATCHLEVEL);
> +        /*
> +         * AMD and Intel use the same MSR for the current microcode version.
> +         *
> +         * There is no need to jump through the SDM-provided hoops for Intel.
> +         * A guest might itself perform the "write 0, CPUID, read" sequence,
> +         * but servicing the CPUID for the guest typically wont result in
> +         * actually executing a CPUID instruction.
> +         *
> +         * As a guest can't influence the value of this MSR, the value will be
> +         * from Xen's last microcode load, which can be forwarded straight to
> +         * the guest.
> +         */
> +        if ( !(cp->x86_vendor & (X86_VENDOR_INTEL |X86_VENDOR_AMD)) ||

Nit: There's a missing blank after the | here.

Jan



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

Re: [PATCH 1/5] x86/cpu: Drop cpu_devs[] and $VENDOR_init_cpu() hooks

[Andrew Cooper]

On 05/04/2019 09:57, Jan Beulich wrote:
>>>> On 04.04.19 at 22:26, <andrew.cooper3@citrix.com> wrote:
>> These helpers each fill in a single cpu_devs[] pointer, and since c/s
>> 00b4f4d0f "x86/cpuid: Drop get_cpu_vendor() completely", this array is read
>> exactly once on boot.
>>
>> Delete the hooks and cpu_devs[], and have early_cpu_detect() pick the
>> appropriate cpu_dev structure directly.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with one remark:
>
>> @@ -687,10 +689,6 @@ static cpumask_t cpu_initialized;
>>  
>>  void __init early_cpu_init(void)
>>  {
>> -	intel_cpu_init();
>> -	amd_init_cpu();
>> -	centaur_init_cpu();
>> -	shanghai_init_cpu();
>>  	early_cpu_detect();
>>  }
> I think this function should then go away as well, unless you have
> plans to put new stuff into it. To avoid modifying the caller,
> perhaps best to simply renamed early_cpu_detect()? My R-b
> stands with this change, if you decide to fold it in.

I was considering whether to do that, so I will.

~Andrew

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

Re: [PATCH 3/5] x86/cpu: Renumber X86_VENDOR_* to form a bitmap

[Andrew Cooper]

On 05/04/2019 10:03, Jan Beulich wrote:
>>>> On 04.04.19 at 22:26, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/include/asm-x86/x86-vendors.h
>> +++ b/xen/include/asm-x86/x86-vendors.h
>> @@ -4,28 +4,29 @@
>>  /*
>>   * CPU vendor IDs
>>   *
>> - * - X86_VENDOR_* are Xen-internal identifiers.  Values and order are
>> - *   arbitrary.
>> + * - X86_VENDOR_* are Xen-internal identifiers.  The order is arbitrary, but
>> + *   values form a bitmap so vendor checks can be made against multiple
>> + *   vendors at once.
>>   * - 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 (1 << 1)
> Any reason you start from a shift count of 1, instead of 0, when
> "unknown" simply is no bit set? We're not about to run out of bits,
> but preferably with this corrected
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

No - that wasn't intentional.  Will fix.

~Andrew

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

Re: [PATCH 4/5] x86/cpu: Create Hygon Dhyana architecture support file

[Pu Wen]

On 2019/4/5 17:38, Jan Beulich wrote:
> On 04.04.19 at 22:26, <andrew.cooper3@citrix.com> wrote:
>> +	else
>> +		/* Successfully enabled! */
>> +		__set_bit(X86_FEATURE_LFENCE_DISPATCH,
>> +			  c->x86_capability);
> 
> Down the road we may want to make this another helper function
> shared by AMD any Hygon code.

It sounds good.

>> +	/*
>> +	 * If the user has explicitly chosen to disable Memory Disambiguation
>> +	 * to mitigiate Speculative Store Bypass, poke the appropriate MSR.
>> +	 */
>> +	if (opt_ssbd && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) {
>> +		value |= 1ull << 10;
>> +		wrmsr_safe(MSR_AMD64_LS_CFG, value);
>> +	}
> 
> Like the above, this lacks a model check. Is it really expected for
> all future Hygon models to supports both in exactly the same

For current Hygon family 18h, all models will have the same meaning.

> fashion? Even if there's just a small chance of this not being the
> case, rather than fiddling with MSRs which have an entirely
> different meaning in future models, I'd prefer if model checks
> were added in cases like these.

In future for some other Hygon CPU families(such as maybe family 20h ?), 
the bits definition of this MSR may have different meaning. But I think 
it should be dealt with by then, since there would be some other 
features to be adjusted by the way for those families.

> I do realize that in the former case there's effectively an "all
> models except a few" logic already in the original AMD code,
> which I would too question whether it's really desirable. After
> all we've learned recently that MSRs indeed can go away

It's a good thing that MSRs can go away.

> (albeit in that case it was a change to the MSR simply becoming
> meaningless, rather than obtaining a new meaning). Based on
> this I could accept just the SSBD logic to gain a model check.

-- 
Regards,
Pu Wen

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

Re: [PATCH 4/5] x86/cpu: Create Hygon Dhyana architecture support file

[Jan Beulich]

>>> On 05.04.19 at 17:30, <puwen@hygon.cn> wrote:
> On 2019/4/5 17:38, Jan Beulich wrote:
>> On 04.04.19 at 22:26, <andrew.cooper3@citrix.com> wrote:
>>> +	/*
>>> +	 * If the user has explicitly chosen to disable Memory Disambiguation
>>> +	 * to mitigiate Speculative Store Bypass, poke the appropriate MSR.
>>> +	 */
>>> +	if (opt_ssbd && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) {
>>> +		value |= 1ull << 10;
>>> +		wrmsr_safe(MSR_AMD64_LS_CFG, value);
>>> +	}
>> 
>> Like the above, this lacks a model check. Is it really expected for
>> all future Hygon models to supports both in exactly the same
> 
> For current Hygon family 18h, all models will have the same meaning.

Oh, I'm sorry - I meant "families", not "models" in my earlier reply
(throughout). I'm frequently mixing this up when switching between
"Intel" and "AMD" modes, unfortunately.

Jan



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