[Xen-devel] [PATCH] x86/cpuid: Don't use volatile asm statements

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

Common use of the CPUID instruction operates without side effects.  Let the
compiler better optimise code by dropping the volatile qualifier.

The only place where order matters is for Intel microcode loading, where
executing a CPUID instruction is used for its side effect of updating
MSR_IA32_UCODE_REV.

The existing logic is buggy because GCC has been seen to reorder independent
asm volatile statements.  Opencode the two cases, with a compiler barrier to
enforce the correct ordering of operations.

While here, fix the comment, which isn't correct.  The SDM doesn't state that
a read of leaf 1 is required - just that a CPUID instruction is required.
Using leaf 0 results in better code generation, following the write of 0 to
MSR_IA32_UCODE_REV.

Suggested-by: Jan Beulich <JBeulich@xxxxxxxx>
Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
---
CC: Jan Beulich <JBeulich@xxxxxxxx>
CC: Wei Liu <wei.liu2@xxxxxxxxxx>
CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
---
 xen/arch/x86/microcode_intel.c  | 13 ++++++++-----
 xen/include/asm-x86/processor.h | 12 ++++++------
 2 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/xen/arch/x86/microcode_intel.c b/xen/arch/x86/microcode_intel.c
index 22fdeca..088cfa2 100644
--- a/xen/arch/x86/microcode_intel.c
+++ b/xen/arch/x86/microcode_intel.c
@@ -99,6 +99,7 @@ static DEFINE_SPINLOCK(microcode_update_lock);
 static int collect_cpu_info(unsigned int cpu_num, struct cpu_signature *csig)
 {
     struct cpuinfo_x86 *c = &cpu_data[cpu_num];
+    unsigned long tmp;
     uint64_t msr_content;
 
     BUG_ON(cpu_num != smp_processor_id());
@@ -122,8 +123,9 @@ static int collect_cpu_info(unsigned int cpu_num, struct 
cpu_signature *csig)
     }
 
     wrmsrl(MSR_IA32_UCODE_REV, 0x0ULL);
-    /* As documented in the SDM: Do a CPUID 1 here */
-    cpuid_eax(1);
+    /* As documented in the SDM: Do a CPUID here. */
+    asm volatile ( "cpuid" : "=a" (tmp) : "a" (0)
+                   : "bx", "cx", "dx", "memory" );
 
     /* get the current revision from MSR 0x8B */
     rdmsrl(MSR_IA32_UCODE_REV, msr_content);
@@ -286,7 +288,7 @@ static int get_matching_microcode(const void *mc, unsigned 
int cpu)
 
 static int apply_microcode(unsigned int cpu)
 {
-    unsigned long flags;
+    unsigned long flags, tmp;
     uint64_t msr_content;
     unsigned int val[2];
     unsigned int cpu_num = raw_smp_processor_id();
@@ -305,8 +307,9 @@ static int apply_microcode(unsigned int cpu)
     wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)uci->mc.mc_intel->bits);
     wrmsrl(MSR_IA32_UCODE_REV, 0x0ULL);
 
-    /* As documented in the SDM: Do a CPUID 1 here */
-    cpuid_eax(1);
+    /* As documented in the SDM: Do a CPUID here. */
+    asm volatile ( "cpuid" : "=a" (tmp) : "a" (0)
+                   : "bx", "cx", "dx", "memory" );
 
     /* get the current revision from MSR 0x8B */
     rdmsrl(MSR_IA32_UCODE_REV, msr_content);
diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index cef3ffb..3c76e7e 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -185,7 +185,7 @@ unsigned int apicid_to_socket(unsigned int);
  * resulting in stale register contents being returned.
  */
 #define cpuid(_op,_eax,_ebx,_ecx,_edx)          \
-    asm volatile ( "cpuid"                      \
+    asm ( "cpuid"                               \
           : "=a" (*(int *)(_eax)),              \
             "=b" (*(int *)(_ebx)),              \
             "=c" (*(int *)(_ecx)),              \
@@ -201,7 +201,7 @@ static inline void cpuid_count(
     unsigned int *ecx,
     unsigned int *edx)
 {
-    asm volatile ( "cpuid"
+    asm ( "cpuid"
           : "=a" (*eax), "=b" (*ebx), "=c" (*ecx), "=d" (*edx)
           : "0" (op), "c" (count) );
 }
@@ -213,7 +213,7 @@ static always_inline unsigned int cpuid_eax(unsigned int op)
 {
     unsigned int eax;
 
-    asm volatile ( "cpuid"
+    asm ( "cpuid"
           : "=a" (eax)
           : "0" (op)
           : "bx", "cx", "dx" );
@@ -224,7 +224,7 @@ static always_inline unsigned int cpuid_ebx(unsigned int op)
 {
     unsigned int eax, ebx;
 
-    asm volatile ( "cpuid"
+    asm ( "cpuid"
           : "=a" (eax), "=b" (ebx)
           : "0" (op)
           : "cx", "dx" );
@@ -235,7 +235,7 @@ static always_inline unsigned int cpuid_ecx(unsigned int op)
 {
     unsigned int eax, ecx;
 
-    asm volatile ( "cpuid"
+    asm ( "cpuid"
           : "=a" (eax), "=c" (ecx)
           : "0" (op)
           : "bx", "dx" );
@@ -246,7 +246,7 @@ static always_inline unsigned int cpuid_edx(unsigned int op)
 {
     unsigned int eax, edx;
 
-    asm volatile ( "cpuid"
+    asm ( "cpuid"
           : "=a" (eax), "=d" (edx)
           : "0" (op)
           : "bx", "cx" );
-- 
2.1.4


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