[PATCH] apic: clean up msr handling

[PATCH] apic: clean up msr handling

[Christoph Egger]

[-- Attachment #1: Type: text/plain, Size: 440 bytes --]


Hi!

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

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>


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

[-- Attachment #2: xen_msr_apic.diff --]
[-- Type: text/x-diff, Size: 5920 bytes --]

diff -r 3e1b6559f67d xen/arch/x86/genapic/x2apic.c
--- a/xen/arch/x86/genapic/x2apic.c	Fri Jun 11 13:58:40 2010 +0100
+++ b/xen/arch/x86/genapic/x2apic.c	Fri Jun 11 15:13:26 2010 +0200
@@ -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);
 }
diff -r 3e1b6559f67d xen/include/asm-x86/apic.h
--- a/xen/include/asm-x86/apic.h	Fri Jun 11 13:58:40 2010 +0100
+++ b/xen/include/asm-x86/apic.h	Fri Jun 11 15:13:26 2010 +0200
@@ -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);
diff -r 3e1b6559f67d xen/include/asm-x86/msr.h
--- a/xen/include/asm-x86/msr.h	Fri Jun 11 13:58:40 2010 +0100
+++ b/xen/include/asm-x86/msr.h	Fri Jun 11 15:13:26 2010 +0200
@@ -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__ */
 
diff -r 3e1b6559f67d xen/include/xen/cpuidle.h
--- a/xen/include/xen/cpuidle.h	Fri Jun 11 13:58:40 2010 +0100
+++ b/xen/include/xen/cpuidle.h	Fri Jun 11 15:13:26 2010 +0200
@@ -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
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] apic: clean up msr handling

[Jan Beulich]

>>> On 11.06.10 at 15:22, Christoph Egger <Christoph.Egger@amd.com> wrote:
>-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

Re: [PATCH] apic: clean up msr handling

[Keir Fraser]

On 11/06/2010 14:40, "Jan Beulich" <JBeulich@novell.com> wrote:

>>>> On 11.06.10 at 15:22, Christoph Egger <Christoph.Egger@amd.com> wrote:
>> -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.

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

Re: [PATCH] apic: clean up msr handling

[Christoph Egger]

On Friday 11 June 2010 15:49:39 Keir Fraser wrote:
> On 11/06/2010 14:40, "Jan Beulich" <JBeulich@novell.com> wrote:
> >>>> On 11.06.10 at 15:22, Christoph Egger <Christoph.Egger@amd.com> wrote:
> >>
> >> -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.
>
> 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

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

Re: [PATCH] apic: clean up msr handling

[Keir Fraser]

On 11/06/2010 16:51, "Christoph Egger" <Christoph.Egger@amd.com> wrote:

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

Re: [PATCH] apic: clean up msr handling

[Christoph Egger]

On Friday 11 June 2010 17:57:58 Keir Fraser wrote:
> On 11/06/2010 16:51, "Christoph Egger" <Christoph.Egger@amd.com> wrote:
> >> 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


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

Re: [PATCH] apic: clean up msr handling

[Keir Fraser]

On 11/06/2010 17:10, "Christoph Egger" <Christoph.Egger@amd.com> wrote:

> 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