Skip to contentSkip to conversation

apic: clean up msr handling

PATCH In review Christoph Egger 3 participants 7 messages Jun 11, 2010

lore
Share & tools
lore

Christoph Egger

author
#0 lore raw

PATCH 1/1 apic: clean up msr handling

Hi!

Attached patch cleans up apic msr-handling. This patch is extracted from the big msr patch.

Signed-off-by Christoph Egger <[email protected]>
Signature · 6 lines
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632
xen/arch/x86/genapic/x2apic.c +10 -4
diff -r 3e1b6559f67d xen/arch/x86/genapic/x2apic.c
@@ -90,6 +90,7 @@ void send_IPI_mask_x2apic_phys(const cpu
 {
     unsigned int cpu, cfg;
     unsigned long flags;
+    uint64_t msr_content;
 
     /*
      * Ensure that any synchronisation data written in program order by this

@@ -107,8 +108,10 @@ void send_IPI_mask_x2apic_phys(const cpu
 
     cfg = APIC_DM_FIXED | 0 /* no shorthand */ | APIC_DEST_PHYSICAL | vector;
     for_each_cpu_mask ( cpu, *cpumask )
-        if ( cpu != smp_processor_id() )
-            apic_wrmsr(APIC_ICR, cfg, cpu_physical_id(cpu));
+        if ( cpu != smp_processor_id() ) {
+            msr_content = cfg | ((uint64_t)cpu_physical_id(cpu) << 32);
+            apic_wrmsr(APIC_ICR, msr_content);
+        }
 
     local_irq_restore(flags);
 }

@@ -117,6 +120,7 @@ void send_IPI_mask_x2apic_cluster(const 
 {
     unsigned int cpu, cfg;
     unsigned long flags;
+    uint64_t msr_content;
 
     mb(); /* see the comment in send_IPI_mask_x2apic_phys() */
 

@@ -124,8 +128,10 @@ void send_IPI_mask_x2apic_cluster(const 
 
     cfg = APIC_DM_FIXED | 0 /* no shorthand */ | APIC_DEST_LOGICAL | vector;
     for_each_cpu_mask ( cpu, *cpumask )
-        if ( cpu != smp_processor_id() )
-            apic_wrmsr(APIC_ICR, cfg, cpu_2_logical_apicid[cpu]);
+        if ( cpu != smp_processor_id() ) {
+            msr_content = cfg | ((uint64_t)cpu_2_logical_apicid[cpu] << 32);
+            apic_wrmsr(APIC_ICR, msr_content);
+        }
 
     local_irq_restore(flags);
 }
xen/include/asm-x86/apic.h +16 -22
diff -r 3e1b6559f67d xen/include/asm-x86/apic.h
@@ -4,7 +4,7 @@
 #include <xen/config.h>
 #include <asm/apicdef.h>
 #include <asm/fixmap.h>
-#include <asm/system.h>
+#include <asm/msr.h>
 
 #define Dprintk(x...)
 

@@ -76,34 +76,31 @@ static __inline u32 apic_mem_read(unsign
  * access the 64-bit ICR register.
  */
 
-static __inline void apic_wrmsr(unsigned long reg, u32 low, u32 high)
+static __inline void apic_wrmsr(unsigned long reg, uint64_t msr_content)
 {
     if (reg == APIC_DFR || reg == APIC_ID || reg == APIC_LDR ||
         reg == APIC_LVR)
         return;
 
-    __asm__ __volatile__("wrmsr"
-            : /* no outputs */
-            : "c" (APIC_MSR_BASE + (reg >> 4)), "a" (low), "d" (high));
+    wrmsrl(APIC_MSR_BASE + (reg >> 4), msr_content);
 }
 
-static __inline void apic_rdmsr(unsigned long reg, u32 *low, u32 *high)
+static __inline uint64_t apic_rdmsr(unsigned long reg)
 {
+    uint64_t msr_content;
+
     if (reg == APIC_DFR)
-    {
-        *low = *high = -1u;
-        return;
-    }
-    __asm__ __volatile__("rdmsr"
-            : "=a" (*low), "=d" (*high)
-            : "c" (APIC_MSR_BASE + (reg >> 4)));
+        return -1u;
+
+    rdmsrl(APIC_MSR_BASE + (reg >> 4), msr_content);
+    return msr_content;
 }
 
 static __inline void apic_write(unsigned long reg, u32 v)
 {
 
     if ( x2apic_enabled )
-        apic_wrmsr(reg, v, 0);
+        apic_wrmsr(reg, v);
     else
         apic_mem_write(reg, v);
 }

@@ -111,20 +108,17 @@ static __inline void apic_write(unsigned
 static __inline void apic_write_atomic(unsigned long reg, u32 v)
 {
     if ( x2apic_enabled )
-        apic_wrmsr(reg, v, 0);
+        apic_wrmsr(reg, v);
     else
         apic_mem_write_atomic(reg, v);
 }
 
 static __inline u32 apic_read(unsigned long reg)
 {
-    u32 lo, hi;
-
     if ( x2apic_enabled )
-        apic_rdmsr(reg, &lo, &hi);
+        return apic_rdmsr(reg);
     else
-        lo = apic_mem_read(reg);
-    return lo;
+        return apic_mem_read(reg);
 }
 
 static __inline u64 apic_icr_read(void)

@@ -132,7 +126,7 @@ static __inline u64 apic_icr_read(void)
     u32 lo, hi;
 
     if ( x2apic_enabled )
-        apic_rdmsr(APIC_ICR, &lo, &hi);
+        return apic_rdmsr(APIC_ICR);
     else
     {
         lo = apic_mem_read(APIC_ICR);

@@ -145,7 +139,7 @@ static __inline u64 apic_icr_read(void)
 static __inline void apic_icr_write(u32 low, u32 dest)
 {
     if ( x2apic_enabled )
-        apic_wrmsr(APIC_ICR, low, dest);
+        apic_wrmsr(APIC_ICR, low | ((uint64_t)dest << 32));
     else
     {
         apic_mem_write(APIC_ICR2, dest << 24);
xen/include/asm-x86/msr.h +12 -20
diff -r 3e1b6559f67d xen/include/asm-x86/msr.h
@@ -5,7 +5,6 @@
 
 #ifndef __ASSEMBLY__
 
-#include <xen/smp.h>
 #include <xen/types.h>
 #include <xen/percpu.h>
 #include <xen/errno.h>

@@ -104,29 +103,22 @@ static inline int wrmsr_safe(unsigned in
 
 DECLARE_PER_CPU(u64, efer);
 
-static inline u64 read_efer(void)
-{
-    return this_cpu(efer);
-}
+#define read_efer() this_cpu(efer)
 
-static inline void write_efer(u64 val)
-{
-    this_cpu(efer) = val;
-    wrmsrl(MSR_EFER, val);
-}
+#define write_efer(val) do { \
+    this_cpu(efer) = val; \
+    wrmsrl(MSR_EFER, val); \
+} while(0)
 
 DECLARE_PER_CPU(u32, ler_msr);
 
-static inline void ler_enable(void)
-{
-    u64 debugctl;
-    
-    if ( !this_cpu(ler_msr) )
-        return;
-
-    rdmsrl(MSR_IA32_DEBUGCTLMSR, debugctl);
-    wrmsrl(MSR_IA32_DEBUGCTLMSR, debugctl | 1);
-}
+#define ler_enable() do { \
+    u64 debugctl; \
+    if ( !this_cpu(ler_msr) ) \
+        return; \
+    rdmsrl(MSR_IA32_DEBUGCTLMSR, debugctl); \
+    wrmsrl(MSR_IA32_DEBUGCTLMSR, debugctl | 1); \
+} while(0)
 
 #endif /* !__ASSEMBLY__ */
xen/include/xen/cpuidle.h +2 -0
diff -r 3e1b6559f67d xen/include/xen/cpuidle.h
@@ -27,6 +27,8 @@
 #ifndef _XEN_CPUIDLE_H
 #define _XEN_CPUIDLE_H
 
+#include <xen/smp.h>
+
 #define ACPI_PROCESSOR_MAX_POWER        8
 #define CPUIDLE_NAME_LEN                16

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

#1 lore raw

xen/include/asm-x86/msr.h static inline int wrmsr_safe(unsigned in · line 111

Quoted 10 lines
Quoted 1 line
Quoted 1 line · Christoph Egger
xen/include/asm-x86/msr.h +4 -5
-static inline void write_efer(u64 val)
-{
-    this_cpu(efer) = val;
-    wrmsrl(MSR_EFER, val);
-}
+#define write_efer(val) do { \
+    this_cpu(efer) = val; \
+    wrmsrl(MSR_EFER, val); \
+} while(0)

This isn't a good change imo: You now require all current and future users of write_efer() to not pass expressions with side effects.

Also, is doesn't really look like a cleanup to me, more like a complication.

Jan

#2 lore raw
Quoted 14 lines · Jan Beulich

xen/include/asm-x86/msr.h static inline int wrmsr_safe(unsigned in · line 111

Quoted 10 lines
Quoted 1 line
Quoted 1 line · Christoph Egger
xen/include/asm-x86/msr.h +4 -5
-static inline void write_efer(u64 val)
-{
-    this_cpu(efer) = val;
-    wrmsrl(MSR_EFER, val);
-}
+#define write_efer(val) do { \
+    this_cpu(efer) = val; \
+    wrmsrl(MSR_EFER, val); \
+} while(0)

This isn't a good change imo: You now require all current and future users of write_efer() to not pass expressions with side effects.

Quoted 2 lines

Also, is doesn't really look like a cleanup to me, more like a complication.

Hm, I didn't spot that one. It's odd since I thought Christoph was changign macros into inlien functions where possible. Maybe he just changes in whichever direction makes his patch bigger? ;-)

-- Keir

author
#3 lore raw
Quoted 16 lines · On Friday 11 June 2010 15:49:39 Keir Fraser wrote:
Quoted 15 lines · Jan Beulich

xen/include/asm-x86/msr.h static inline int wrmsr_safe(unsigned in · line 111

Quoted 11 lines
Quoted 1 line
Quoted 1 line · Christoph Egger
xen/include/asm-x86/msr.h +4 -5
-static inline void write_efer(u64 val)
-{
-    this_cpu(efer) = val;
-    wrmsrl(MSR_EFER, val);
-}
+#define write_efer(val) do { \
+    this_cpu(efer) = val; \
+    wrmsrl(MSR_EFER, val); \
+} while(0)

This isn't a good change imo: You now require all current and future users of write_efer() to not pass expressions with side effects.

Quoted 6 lines
Quoted 2 lines

Also, is doesn't really look like a cleanup to me, more like a complication.

Hm, I didn't spot that one. It's odd since I thought Christoph was changign macros into inlien functions where possible. Maybe he just changes in whichever direction makes his patch bigger? ;-)

No, this change has to do with 'smp_processor_id() undefined'. The root problem is a circular dependency with inclusion of headers. The right fix is to clean up the headers.

Christoph

Signature · 6 lines
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632
#4 lore raw
Quoted 7 lines · Christoph Egger
Quoted 3 lines

Hm, I didn't spot that one. It's odd since I thought Christoph was changign macros into inlien functions where possible. Maybe he just changes in whichever direction makes his patch bigger? ;-)

No, this change has to do with 'smp_processor_id() undefined'. The root problem is a circular dependency with inclusion of headers. The right fix is to clean up the headers.

Yep, that's what I did.

-- Keir

author
#5 lore raw
Quoted 10 lines · On Friday 11 June 2010 17:57:58 Keir Fraser wrote:
Quoted 7 lines · Christoph Egger
Quoted 3 lines

Hm, I didn't spot that one. It's odd since I thought Christoph was changign macros into inlien functions where possible. Maybe he just changes in whichever direction makes his patch bigger? ;-)

No, this change has to do with 'smp_processor_id() undefined'. The root problem is a circular dependency with inclusion of headers. The right fix is to clean up the headers.

Yep, that's what I did.

Yes, I saw that in c/s 21605. Thanks for doing this.

But this is not what I meant by 'clean up the headers'. For example, when you do

touch xen/include/asm-x86/apic.h

then in the next build files that have nothing to do with apic get recompiled because apic.h is indirectly included.

Christoph

Signature · 6 lines
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632
#6 lore raw
Quoted 7 lines · Christoph Egger

But this is not what I meant by 'clean up the headers'. For example, when you do

touch xen/include/asm-x86/apic.h

then in the next build files that have nothing to do with apic get recompiled because apic.h is indirectly included.

Yeah, smp.h should not need to include apic.h. I fixed this up as xen-unstable:21608.

-- Keir