[PATCH] [x86, msr]: remove code duplication

[PATCH] [x86, msr]: remove code duplication

[Borislav Petkov]

Since rdmsr_on_cpus and wrmsr_on_cpus are almost identical, make them
call into a common __rwmsr_on_cpus helper passing a function pointer arg
to the actual MSR operation.

Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 arch/x86/lib/msr.c |   48 ++++++++++++++++++------------------------------
 1 files changed, 18 insertions(+), 30 deletions(-)

diff --git a/arch/x86/lib/msr.c b/arch/x86/lib/msr.c
index 1440b9c..20b6df5 100644
--- a/arch/x86/lib/msr.c
+++ b/arch/x86/lib/msr.c
@@ -71,14 +71,9 @@ int wrmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 l, u32 h)
 }
 EXPORT_SYMBOL(wrmsr_on_cpu);
 
-/* rdmsr on a bunch of CPUs
- *
- * @mask:       which CPUs
- * @msr_no:     which MSR
- * @msrs:       array of MSR values
- *
- */
-void rdmsr_on_cpus(const cpumask_t *mask, u32 msr_no, struct msr *msrs)
+static inline void __rwmsr_on_cpus(const cpumask_t *mask, u32 msr_no,
+				   struct msr *msrs,
+				   void (*msr_func) (void *info))
 {
 	struct msr_info rv;
 	int this_cpu;
@@ -95,11 +90,22 @@ void rdmsr_on_cpus(const cpumask_t *mask, u32 msr_no, struct msr *msrs)
 	 * smp_call_function_many has been fixed to not skip it.
 	 */
 	this_cpu = raw_smp_processor_id();
-	smp_call_function_single(this_cpu, __rdmsr_on_cpu, &rv, 1);
-
-	smp_call_function_many(mask, __rdmsr_on_cpu, &rv, 1);
+	smp_call_function_single(this_cpu, msr_func, &rv, 1);
+	smp_call_function_many(mask, msr_func, &rv, 1);
 	preempt_enable();
 }
+
+/* rdmsr on a bunch of CPUs
+ *
+ * @mask:       which CPUs
+ * @msr_no:     which MSR
+ * @msrs:       array of MSR values
+ *
+ */
+void rdmsr_on_cpus(const cpumask_t *mask, u32 msr_no, struct msr *msrs)
+{
+	__rwmsr_on_cpus(mask, msr_no, msrs, __rdmsr_on_cpu);
+}
 EXPORT_SYMBOL(rdmsr_on_cpus);
 
 /*
@@ -112,25 +118,7 @@ EXPORT_SYMBOL(rdmsr_on_cpus);
  */
 void wrmsr_on_cpus(const cpumask_t *mask, u32 msr_no, struct msr *msrs)
 {
-	struct msr_info rv;
-	int this_cpu;
-
-	memset(&rv, 0, sizeof(rv));
-
-	rv.off    = cpumask_first(mask);
-	rv.msrs   = msrs;
-	rv.msr_no = msr_no;
-
-	preempt_disable();
-	/*
-	 * FIXME: handle the CPU we're executing on separately for now until
-	 * smp_call_function_many has been fixed to not skip it.
-	 */
-	this_cpu = raw_smp_processor_id();
-	smp_call_function_single(this_cpu, __wrmsr_on_cpu, &rv, 1);
-
-	smp_call_function_many(mask, __wrmsr_on_cpu, &rv, 1);
-	preempt_enable();
+	__rwmsr_on_cpus(mask, msr_no, msrs, __wrmsr_on_cpu);
 }
 EXPORT_SYMBOL(wrmsr_on_cpus);
 
-- 
1.6.3.3

Re: [PATCH] [x86, msr]: remove code duplication

[H. Peter Anvin]

Borislav Petkov wrote:
> -
> -	smp_call_function_many(mask, __rdmsr_on_cpu, &rv, 1);
> +	smp_call_function_single(this_cpu, msr_func, &rv, 1);
> +	smp_call_function_many(mask, msr_func, &rv, 1);
>  	preempt_enable();

Why are you using smp_call_function_single() to execute something *on
your own CPU*?

	-hpa

Re: [PATCH] [x86, msr]: remove code duplication

[Borislav Petkov]

On Mon, Jul 06, 2009 at 02:31:36PM -0700, H. Peter Anvin wrote:
> Borislav Petkov wrote:
> > -
> > -	smp_call_function_many(mask, __rdmsr_on_cpu, &rv, 1);
> > +	smp_call_function_single(this_cpu, msr_func, &rv, 1);
> > +	smp_call_function_many(mask, msr_func, &rv, 1);
> >  	preempt_enable();
> 
> Why are you using smp_call_function_single() to execute something *on
> your own CPU*?

Actually, the more important question is why am I executing anything on
my own CPU without first checking if it is in the cpumask _at_ _all_?!
/me ducks behind the sofa.

The right thing to do should be something like the following:

preempt_disable();
this_cpu = raw_smp_processor_id();

if (cpumask_test_cpu(this_cpu, mask)) {
	local_irq_disable();
	msr_func(&rv);
	local_irq_enable();
}

smp_call_function_many(mask, msr_func, &rv, 1);
preempt_enable();

Patch coming up...

-- 
Regards/Gruss,
Boris.

Operating | Advanced Micro Devices GmbH
  System  | Karl-Hammerschmidt-Str. 34, 85609 Dornach b. München, Germany
 Research | Geschäftsführer: Thomas M. McCoy, Giuliano Meroni
  Center  | Sitz: Dornach, Gemeinde Aschheim, Landkreis München
  (OSRC)  | Registergericht München, HRB Nr. 43632

Re: [PATCH] [x86, msr]: remove code duplication

[H. Peter Anvin]

Borislav Petkov wrote:
> 
> Actually, the more important question is why am I executing anything on
> my own CPU without first checking if it is in the cpumask _at_ _all_?!
> /me ducks behind the sofa.
> 
> The right thing to do should be something like the following:
> 
> preempt_disable();
> this_cpu = raw_smp_processor_id();
> 
> if (cpumask_test_cpu(this_cpu, mask)) {
> 	local_irq_disable();
> 	msr_func(&rv);
> 	local_irq_enable();
> }
> 
> smp_call_function_many(mask, msr_func, &rv, 1);
> preempt_enable();
> 

I don't see why you're disabling local IRQs.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

[PATCH] [x86, msr]: execute on the correct CPU subset (was: Re: [PATCH] [x86, msr]: remove code duplication)

[Borislav Petkov]

On Tue, Jul 07, 2009 at 08:56:00AM -0700, H. Peter Anvin wrote:
> Borislav Petkov wrote:
> > 
> > Actually, the more important question is why am I executing anything on
> > my own CPU without first checking if it is in the cpumask _at_ _all_?!
> > /me ducks behind the sofa.
> > 
> > The right thing to do should be something like the following:
> > 
> > preempt_disable();
> > this_cpu = raw_smp_processor_id();
> > 
> > if (cpumask_test_cpu(this_cpu, mask)) {
> > 	local_irq_disable();
> > 	msr_func(&rv);
> > 	local_irq_enable();
> > }
> > 
> > smp_call_function_many(mask, msr_func, &rv, 1);
> > preempt_enable();
> > 
> 
> I don't see why you're disabling local IRQs.

I guess I was trying to be overly careful but can't seem to think of a
case when this would be appropriate. Hmm...

--
From: Borislav Petkov <borislav.petkov@amd.com>
Date: Mon, 6 Jul 2009 16:08:34 +0200
Subject: [PATCH] [x86, msr]: execute on the correct CPU subset

rdmsr_on_cpus/wrmsr_on_cpus were erroneously executing on the current
CPU even in the case where it wasn't in the supplied bitmask. Add a
check for that and handle accordingly.

While at it, since rdmsr_on_cpus and wrmsr_on_cpus are almost identical,
fold them into a common __rwmsr_on_cpus helper passing a function
pointer arg to the actual MSR operation.

Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 arch/x86/lib/msr.c |   53 +++++++++++++++++++--------------------------------
 1 files changed, 20 insertions(+), 33 deletions(-)

diff --git a/arch/x86/lib/msr.c b/arch/x86/lib/msr.c
index 1440b9c..8242b12 100644
--- a/arch/x86/lib/msr.c
+++ b/arch/x86/lib/msr.c
@@ -71,14 +71,9 @@ int wrmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 l, u32 h)
 }
 EXPORT_SYMBOL(wrmsr_on_cpu);
 
-/* rdmsr on a bunch of CPUs
- *
- * @mask:       which CPUs
- * @msr_no:     which MSR
- * @msrs:       array of MSR values
- *
- */
-void rdmsr_on_cpus(const cpumask_t *mask, u32 msr_no, struct msr *msrs)
+static inline void __rwmsr_on_cpus(const cpumask_t *mask, u32 msr_no,
+				   struct msr *msrs,
+				   void (*msr_func) (void *info))
 {
 	struct msr_info rv;
 	int this_cpu;
@@ -90,16 +85,26 @@ void rdmsr_on_cpus(const cpumask_t *mask, u32 msr_no, struct msr *msrs)
 	rv.msr_no = msr_no;
 
 	preempt_disable();
-	/*
-	 * FIXME: handle the CPU we're executing on separately for now until
-	 * smp_call_function_many has been fixed to not skip it.
-	 */
 	this_cpu = raw_smp_processor_id();
-	smp_call_function_single(this_cpu, __rdmsr_on_cpu, &rv, 1);
 
-	smp_call_function_many(mask, __rdmsr_on_cpu, &rv, 1);
+	if (cpumask_test_cpu(this_cpu, mask))
+		msr_func(&rv);
+
+	smp_call_function_many(mask, msr_func, &rv, 1);
 	preempt_enable();
 }
+
+/* rdmsr on a bunch of CPUs
+ *
+ * @mask:       which CPUs
+ * @msr_no:     which MSR
+ * @msrs:       array of MSR values
+ *
+ */
+void rdmsr_on_cpus(const cpumask_t *mask, u32 msr_no, struct msr *msrs)
+{
+	__rwmsr_on_cpus(mask, msr_no, msrs, __rdmsr_on_cpu);
+}
 EXPORT_SYMBOL(rdmsr_on_cpus);
 
 /*
@@ -112,25 +117,7 @@ EXPORT_SYMBOL(rdmsr_on_cpus);
  */
 void wrmsr_on_cpus(const cpumask_t *mask, u32 msr_no, struct msr *msrs)
 {
-	struct msr_info rv;
-	int this_cpu;
-
-	memset(&rv, 0, sizeof(rv));
-
-	rv.off    = cpumask_first(mask);
-	rv.msrs   = msrs;
-	rv.msr_no = msr_no;
-
-	preempt_disable();
-	/*
-	 * FIXME: handle the CPU we're executing on separately for now until
-	 * smp_call_function_many has been fixed to not skip it.
-	 */
-	this_cpu = raw_smp_processor_id();
-	smp_call_function_single(this_cpu, __wrmsr_on_cpu, &rv, 1);
-
-	smp_call_function_many(mask, __wrmsr_on_cpu, &rv, 1);
-	preempt_enable();
+	__rwmsr_on_cpus(mask, msr_no, msrs, __wrmsr_on_cpu);
 }
 EXPORT_SYMBOL(wrmsr_on_cpus);
 
-- 
1.6.3.3


-- 
Regards/Gruss,
Boris.

Operating | Advanced Micro Devices GmbH
  System  | Karl-Hammerschmidt-Str. 34, 85609 Dornach b. München, Germany
 Research | Geschäftsführer: Thomas M. McCoy, Giuliano Meroni
  Center  | Sitz: Dornach, Gemeinde Aschheim, Landkreis München
  (OSRC)  | Registergericht München, HRB Nr. 43632

Re: [PATCH] [x86, msr]: execute on the correct CPU subset (was: Re: [PATCH] [x86, msr]: remove code duplication)

[Borislav Petkov]

Hi,

On Wed, Jul 08, 2009 at 01:19:11PM +0200, Borislav Petkov wrote:
> On Tue, Jul 07, 2009 at 08:56:00AM -0700, H. Peter Anvin wrote:
> > Borislav Petkov wrote:
> > > 
> > > Actually, the more important question is why am I executing anything on
> > > my own CPU without first checking if it is in the cpumask _at_ _all_?!
> > > /me ducks behind the sofa.
> > > 
> > > The right thing to do should be something like the following:
> > > 
> > > preempt_disable();
> > > this_cpu = raw_smp_processor_id();
> > > 
> > > if (cpumask_test_cpu(this_cpu, mask)) {
> > > 	local_irq_disable();
> > > 	msr_func(&rv);
> > > 	local_irq_enable();
> > > }
> > > 
> > > smp_call_function_many(mask, msr_func, &rv, 1);
> > > preempt_enable();
> > > 
> > 
> > I don't see why you're disabling local IRQs.
> 
> I guess I was trying to be overly careful but can't seem to think of a
> case when this would be appropriate. Hmm...
> 
> --
> From: Borislav Petkov <borislav.petkov@amd.com>
> Date: Mon, 6 Jul 2009 16:08:34 +0200
> Subject: [PATCH] [x86, msr]: execute on the correct CPU subset
> 
> rdmsr_on_cpus/wrmsr_on_cpus were erroneously executing on the current
> CPU even in the case where it wasn't in the supplied bitmask. Add a
> check for that and handle accordingly.
> 
> While at it, since rdmsr_on_cpus and wrmsr_on_cpus are almost identical,
> fold them into a common __rwmsr_on_cpus helper passing a function
> pointer arg to the actual MSR operation.
> 
> Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
> ---
>  arch/x86/lib/msr.c |   53 +++++++++++++++++++--------------------------------
>  1 files changed, 20 insertions(+), 33 deletions(-)

any comments on that one, NAK/AK? Since it is a fix and not changing the
interface to external users, it might be a good idea to include it in
2.6.31, IMHO, no?

-- 
Regards/Gruss,
Boris.

Operating | Advanced Micro Devices GmbH
  System  | Karl-Hammerschmidt-Str. 34, 85609 Dornach b. München, Germany
 Research | Geschäftsführer: Thomas M. McCoy, Giuliano Meroni
  Center  | Sitz: Dornach, Gemeinde Aschheim, Landkreis München
  (OSRC)  | Registergericht München, HRB Nr. 43632

Re: [PATCH] [x86, msr]: execute on the correct CPU subset

[H. Peter Anvin]

Borislav Petkov wrote:
> 
> any comments on that one, NAK/AK? Since it is a fix and not changing the
> interface to external users, it might be a good idea to include it in
> 2.6.31, IMHO, no?
> 

Yes, I also don't think it's a rush, so I'll take care of it when I get 
back from OLS.

	-hpa

Re: [PATCH] [x86, msr]: execute on the correct CPU subset

[H. Peter Anvin]

Borislav Petkov wrote:
>  
>  	preempt_disable();
> -	/*
> -	 * FIXME: handle the CPU we're executing on separately for now until
> -	 * smp_call_function_many has been fixed to not skip it.
> -	 */
>  	this_cpu = raw_smp_processor_id();
> -	smp_call_function_single(this_cpu, __rdmsr_on_cpu, &rv, 1);
>  
> -	smp_call_function_many(mask, __rdmsr_on_cpu, &rv, 1);
> +	if (cpumask_test_cpu(this_cpu, mask))
> +		msr_func(&rv);
> +
> +	smp_call_function_many(mask, msr_func, &rv, 1);
>  	preempt_enable();
>  }

Any reason not to use get_cpu() ... put_cpu() instead?

	-hpa

Re: [PATCH] [x86, msr]: execute on the correct CPU subset

[Borislav Petkov]

On Mon, Jul 27, 2009 at 01:46:26PM -0700, H. Peter Anvin wrote:
> Borislav Petkov wrote:
> >  
> >  	preempt_disable();
> > -	/*
> > -	 * FIXME: handle the CPU we're executing on separately for now until
> > -	 * smp_call_function_many has been fixed to not skip it.
> > -	 */
> >  	this_cpu = raw_smp_processor_id();
> > -	smp_call_function_single(this_cpu, __rdmsr_on_cpu, &rv, 1);
> >  
> > -	smp_call_function_many(mask, __rdmsr_on_cpu, &rv, 1);
> > +	if (cpumask_test_cpu(this_cpu, mask))
> > +		msr_func(&rv);
> > +
> > +	smp_call_function_many(mask, msr_func, &rv, 1);
> >  	preempt_enable();
> >  }
> 
> Any reason not to use get_cpu() ... put_cpu() instead?

None, patch updated.

--
From: Borislav Petkov <borislav.petkov@amd.com>
Date: Mon, 6 Jul 2009 16:08:34 +0200
Subject: [PATCH] [x86, msr]: execute on the correct CPU subset

rdmsr_on_cpus/wrmsr_on_cpus were erroneously executing on the current
CPU even in the case where it wasn't in the supplied bitmask. Add a
check for that and handle accordingly.

While at it, since rdmsr_on_cpus and wrmsr_on_cpus are almost identical,
fold them call into a common __rwmsr_on_cpus helper passing a function
pointer arg to the actual MSR operation.

Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 arch/x86/lib/msr.c |   58 +++++++++++++++++++--------------------------------
 1 files changed, 22 insertions(+), 36 deletions(-)

diff --git a/arch/x86/lib/msr.c b/arch/x86/lib/msr.c
index 1440b9c..cf879f0 100644
--- a/arch/x86/lib/msr.c
+++ b/arch/x86/lib/msr.c
@@ -71,14 +71,9 @@ int wrmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 l, u32 h)
 }
 EXPORT_SYMBOL(wrmsr_on_cpu);
 
-/* rdmsr on a bunch of CPUs
- *
- * @mask:       which CPUs
- * @msr_no:     which MSR
- * @msrs:       array of MSR values
- *
- */
-void rdmsr_on_cpus(const cpumask_t *mask, u32 msr_no, struct msr *msrs)
+static inline void __rwmsr_on_cpus(const cpumask_t *mask, u32 msr_no,
+				   struct msr *msrs,
+				   void (*msr_func) (void *info))
 {
 	struct msr_info rv;
 	int this_cpu;
@@ -89,16 +84,25 @@ void rdmsr_on_cpus(const cpumask_t *mask, u32 msr_no, struct msr *msrs)
 	rv.msrs	  = msrs;
 	rv.msr_no = msr_no;
 
-	preempt_disable();
-	/*
-	 * FIXME: handle the CPU we're executing on separately for now until
-	 * smp_call_function_many has been fixed to not skip it.
-	 */
-	this_cpu = raw_smp_processor_id();
-	smp_call_function_single(this_cpu, __rdmsr_on_cpu, &rv, 1);
+	this_cpu = get_cpu();
 
-	smp_call_function_many(mask, __rdmsr_on_cpu, &rv, 1);
-	preempt_enable();
+	if (cpumask_test_cpu(this_cpu, mask))
+		msr_func(&rv);
+
+	smp_call_function_many(mask, msr_func, &rv, 1);
+	put_cpu();
+}
+
+/* rdmsr on a bunch of CPUs
+ *
+ * @mask:       which CPUs
+ * @msr_no:     which MSR
+ * @msrs:       array of MSR values
+ *
+ */
+void rdmsr_on_cpus(const cpumask_t *mask, u32 msr_no, struct msr *msrs)
+{
+	__rwmsr_on_cpus(mask, msr_no, msrs, __rdmsr_on_cpu);
 }
 EXPORT_SYMBOL(rdmsr_on_cpus);
 
@@ -112,25 +116,7 @@ EXPORT_SYMBOL(rdmsr_on_cpus);
  */
 void wrmsr_on_cpus(const cpumask_t *mask, u32 msr_no, struct msr *msrs)
 {
-	struct msr_info rv;
-	int this_cpu;
-
-	memset(&rv, 0, sizeof(rv));
-
-	rv.off    = cpumask_first(mask);
-	rv.msrs   = msrs;
-	rv.msr_no = msr_no;
-
-	preempt_disable();
-	/*
-	 * FIXME: handle the CPU we're executing on separately for now until
-	 * smp_call_function_many has been fixed to not skip it.
-	 */
-	this_cpu = raw_smp_processor_id();
-	smp_call_function_single(this_cpu, __wrmsr_on_cpu, &rv, 1);
-
-	smp_call_function_many(mask, __wrmsr_on_cpu, &rv, 1);
-	preempt_enable();
+	__rwmsr_on_cpus(mask, msr_no, msrs, __wrmsr_on_cpu);
 }
 EXPORT_SYMBOL(wrmsr_on_cpus);
 
-- 
1.6.3.3


-- 
Regards/Gruss,
Boris.

Operating | Advanced Micro Devices GmbH
  System  | Karl-Hammerschmidt-Str. 34, 85609 Dornach b. München, Germany
 Research | Geschäftsführer: Thomas M. McCoy, Giuliano Meroni
  Center  | Sitz: Dornach, Gemeinde Aschheim, Landkreis München
  (OSRC)  | Registergericht München, HRB Nr. 43632

Re: [PATCH] [x86, msr]: execute on the correct CPU subset

[Borislav Petkov]

On Mon, Jul 27, 2009 at 01:46:26PM -0700, H. Peter Anvin wrote:
> Any reason not to use get_cpu() ... put_cpu() instead?

In case you haven't applied the one from yesterday yet, here's a better
one which uses struct cpumask * instead of cpumask_t.

--
From: Borislav Petkov <borislav.petkov@amd.com>
Date: Mon, 6 Jul 2009 16:08:34 +0200
Subject: [PATCH] [x86, msr]: execute on the correct CPU subset

rdmsr_on_cpus/wrmsr_on_cpus were erroneously executing on the current
CPU even in the case where it wasn't in the supplied bitmask. Add a
check for that and handle accordingly.

While at it, since rdmsr_on_cpus and wrmsr_on_cpus are almost identical,
fold them call into a common __rwmsr_on_cpus helper passing a function
pointer arg to the actual MSR operation.

Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 arch/x86/include/asm/msr.h |    4 +-
 arch/x86/lib/msr.c         |   60 +++++++++++++++++---------------------------
 2 files changed, 25 insertions(+), 39 deletions(-)

diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
index 48ad9d2..988b067 100644
--- a/arch/x86/include/asm/msr.h
+++ b/arch/x86/include/asm/msr.h
@@ -224,8 +224,8 @@ do {                                                            \
 #ifdef CONFIG_SMP
 int rdmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 *l, u32 *h);
 int wrmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 l, u32 h);
-void rdmsr_on_cpus(const cpumask_t *mask, u32 msr_no, struct msr *msrs);
-void wrmsr_on_cpus(const cpumask_t *mask, u32 msr_no, struct msr *msrs);
+void rdmsr_on_cpus(struct cpumask *mask, u32 msr_no, struct msr *msrs);
+void wrmsr_on_cpus(struct cpumask *mask, u32 msr_no, struct msr *msrs);
 int rdmsr_safe_on_cpu(unsigned int cpu, u32 msr_no, u32 *l, u32 *h);
 int wrmsr_safe_on_cpu(unsigned int cpu, u32 msr_no, u32 l, u32 h);
 #else  /*  CONFIG_SMP  */
diff --git a/arch/x86/lib/msr.c b/arch/x86/lib/msr.c
index 1440b9c..da156b8 100644
--- a/arch/x86/lib/msr.c
+++ b/arch/x86/lib/msr.c
@@ -71,14 +71,9 @@ int wrmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 l, u32 h)
 }
 EXPORT_SYMBOL(wrmsr_on_cpu);
 
-/* rdmsr on a bunch of CPUs
- *
- * @mask:       which CPUs
- * @msr_no:     which MSR
- * @msrs:       array of MSR values
- *
- */
-void rdmsr_on_cpus(const cpumask_t *mask, u32 msr_no, struct msr *msrs)
+static inline void __rwmsr_on_cpus(struct cpumask *mask, u32 msr_no,
+				   struct msr *msrs,
+				   void (*msr_func) (void *info))
 {
 	struct msr_info rv;
 	int this_cpu;
@@ -89,16 +84,25 @@ void rdmsr_on_cpus(const cpumask_t *mask, u32 msr_no, struct msr *msrs)
 	rv.msrs	  = msrs;
 	rv.msr_no = msr_no;
 
-	preempt_disable();
-	/*
-	 * FIXME: handle the CPU we're executing on separately for now until
-	 * smp_call_function_many has been fixed to not skip it.
-	 */
-	this_cpu = raw_smp_processor_id();
-	smp_call_function_single(this_cpu, __rdmsr_on_cpu, &rv, 1);
+	this_cpu = get_cpu();
 
-	smp_call_function_many(mask, __rdmsr_on_cpu, &rv, 1);
-	preempt_enable();
+	if (cpumask_test_cpu(this_cpu, mask))
+		msr_func(&rv);
+
+	smp_call_function_many(mask, msr_func, &rv, 1);
+	put_cpu();
+}
+
+/* rdmsr on a bunch of CPUs
+ *
+ * @mask:       which CPUs
+ * @msr_no:     which MSR
+ * @msrs:       array of MSR values
+ *
+ */
+void rdmsr_on_cpus(struct cpumask *mask, u32 msr_no, struct msr *msrs)
+{
+	__rwmsr_on_cpus(mask, msr_no, msrs, __rdmsr_on_cpu);
 }
 EXPORT_SYMBOL(rdmsr_on_cpus);
 
@@ -110,27 +114,9 @@ EXPORT_SYMBOL(rdmsr_on_cpus);
  * @msrs:       array of MSR values
  *
  */
-void wrmsr_on_cpus(const cpumask_t *mask, u32 msr_no, struct msr *msrs)
+void wrmsr_on_cpus(struct cpumask *mask, u32 msr_no, struct msr *msrs)
 {
-	struct msr_info rv;
-	int this_cpu;
-
-	memset(&rv, 0, sizeof(rv));
-
-	rv.off    = cpumask_first(mask);
-	rv.msrs   = msrs;
-	rv.msr_no = msr_no;
-
-	preempt_disable();
-	/*
-	 * FIXME: handle the CPU we're executing on separately for now until
-	 * smp_call_function_many has been fixed to not skip it.
-	 */
-	this_cpu = raw_smp_processor_id();
-	smp_call_function_single(this_cpu, __wrmsr_on_cpu, &rv, 1);
-
-	smp_call_function_many(mask, __wrmsr_on_cpu, &rv, 1);
-	preempt_enable();
+	__rwmsr_on_cpus(mask, msr_no, msrs, __wrmsr_on_cpu);
 }
 EXPORT_SYMBOL(wrmsr_on_cpus);
 
-- 
1.6.3.3

-- 
Regards/Gruss,
Boris.

Operating | Advanced Micro Devices GmbH
  System  | Karl-Hammerschmidt-Str. 34, 85609 Dornach b. München, Germany
 Research | Geschäftsführer: Thomas M. McCoy, Giuliano Meroni
  Center  | Sitz: Dornach, Gemeinde Aschheim, Landkreis München
  (OSRC)  | Registergericht München, HRB Nr. 43632

Re: [PATCH] [x86, msr]: execute on the correct CPU subset

[H. Peter Anvin]

On 07/29/2009 09:49 AM, Borislav Petkov wrote:
> On Mon, Jul 27, 2009 at 01:46:26PM -0700, H. Peter Anvin wrote:
>> Any reason not to use get_cpu() ... put_cpu() instead?
> 
> In case you haven't applied the one from yesterday yet, here's a better
> one which uses struct cpumask * instead of cpumask_t.
> 

Mangled patch (quoted-printable).

Furthermore, are there any users for this interface at this time?

	-hpa

Re: [PATCH] [x86, msr]: execute on the correct CPU subset

[Borislav Petkov]

On Wed, Jul 29, 2009 at 10:30:45AM -0700, H. Peter Anvin wrote:
> On 07/29/2009 09:49 AM, Borislav Petkov wrote:
> > On Mon, Jul 27, 2009 at 01:46:26PM -0700, H. Peter Anvin wrote:
> >> Any reason not to use get_cpu() ... put_cpu() instead?
> > 
> > In case you haven't applied the one from yesterday yet, here's a better
> > one which uses struct cpumask * instead of cpumask_t.
> > 
> 
> Mangled patch (quoted-printable).

Sorry about that, will resend tomorrow.
 
> Furthermore, are there any users for this interface at this time?

Yep, drivers/edac/amd64_edac.c.

-- 
Regards/Gruss,
    Boris.

Re: [PATCH] [x86, msr]: execute on the correct CPU subset

[H. Peter Anvin]

On 07/29/2009 11:02 AM, Borislav Petkov wrote:
>  
>> Furthermore, are there any users for this interface at this time?
> 
> Yep, drivers/edac/amd64_edac.c.
> 

So let's see... you're asking to change an in-use interface late in the
-rc series when it has active users?

	-hpa

Re: [PATCH] [x86, msr]: execute on the correct CPU subset

[Borislav Petkov]

On Wed, Jul 29, 2009 at 11:05:41AM -0700, H. Peter Anvin wrote:
> On 07/29/2009 11:02 AM, Borislav Petkov wrote:
> >  
> >> Furthermore, are there any users for this interface at this time?
> > 
> > Yep, drivers/edac/amd64_edac.c.
> > 
> 
> So let's see... you're asking to change an in-use interface late in the
> -rc series when it has active users?

Ok, I could create a minimal patch with only the bugfix and send
the remaining changes later during the next merge window, if that's
preferred?

-- 
Regards/Gruss,
    Boris.

Re: [PATCH] [x86, msr]: execute on the correct CPU subset

[H. Peter Anvin]

On 07/29/2009 11:46 AM, Borislav Petkov wrote:
> On Wed, Jul 29, 2009 at 11:05:41AM -0700, H. Peter Anvin wrote:
>> On 07/29/2009 11:02 AM, Borislav Petkov wrote:
>>>  
>>>> Furthermore, are there any users for this interface at this time?
>>> Yep, drivers/edac/amd64_edac.c.
>>>
>> So let's see... you're asking to change an in-use interface late in the
>> -rc series when it has active users?
> 
> Ok, I could create a minimal patch with only the bugfix and send
> the remaining changes later during the next merge window, if that's
> preferred?

Send the remaining *incremental* changes now, I will put them into the
queue for .32.

	-hpa

[PATCH] x86, msr: execute on the correct CPU subset

[Borislav Petkov]

Make rdmsr_on_cpus/wrmsr_on_cpus execute on the current CPU only if it
is in the supplied bitmask.

Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 arch/x86/lib/msr.c |   26 ++++++++++----------------
 1 files changed, 10 insertions(+), 16 deletions(-)

diff --git a/arch/x86/lib/msr.c b/arch/x86/lib/msr.c
index 1440b9c..caa24ac 100644
--- a/arch/x86/lib/msr.c
+++ b/arch/x86/lib/msr.c
@@ -89,16 +89,13 @@ void rdmsr_on_cpus(const cpumask_t *mask, u32 msr_no, struct msr *msrs)
 	rv.msrs	  = msrs;
 	rv.msr_no = msr_no;
 
-	preempt_disable();
-	/*
-	 * FIXME: handle the CPU we're executing on separately for now until
-	 * smp_call_function_many has been fixed to not skip it.
-	 */
-	this_cpu = raw_smp_processor_id();
-	smp_call_function_single(this_cpu, __rdmsr_on_cpu, &rv, 1);
+	this_cpu = get_cpu();
+
+	if (cpumask_test_cpu(this_cpu, mask))
+		__rdmsr_on_cpu(&rv);
 
 	smp_call_function_many(mask, __rdmsr_on_cpu, &rv, 1);
-	preempt_enable();
+	put_cpu();
 }
 EXPORT_SYMBOL(rdmsr_on_cpus);
 
@@ -121,16 +118,13 @@ void wrmsr_on_cpus(const cpumask_t *mask, u32 msr_no, struct msr *msrs)
 	rv.msrs   = msrs;
 	rv.msr_no = msr_no;
 
-	preempt_disable();
-	/*
-	 * FIXME: handle the CPU we're executing on separately for now until
-	 * smp_call_function_many has been fixed to not skip it.
-	 */
-	this_cpu = raw_smp_processor_id();
-	smp_call_function_single(this_cpu, __wrmsr_on_cpu, &rv, 1);
+	this_cpu = get_cpu();
+
+	if (cpumask_test_cpu(this_cpu, mask))
+		__wrmsr_on_cpu(&rv);
 
 	smp_call_function_many(mask, __wrmsr_on_cpu, &rv, 1);
-	preempt_enable();
+	put_cpu();
 }
 EXPORT_SYMBOL(wrmsr_on_cpus);
 
-- 
1.6.3.3

[PATCH] x86, msr: unify rdmsr_on_cpus/wrmsr_on_cpus

[Borislav Petkov]

Since rdmsr_on_cpus and wrmsr_on_cpus are almost identical, unify them
into a common __rwmsr_on_cpus helper thus avoiding code duplication.

While at it, convert cpumask_t's to const struct cpumask *.

Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---

This one is .32 material.

 arch/x86/include/asm/msr.h |    4 +-
 arch/x86/lib/msr.c         |   46 ++++++++++++++++++-------------------------
 2 files changed, 21 insertions(+), 29 deletions(-)

diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
index 48ad9d2..f2f4309 100644
--- a/arch/x86/include/asm/msr.h
+++ b/arch/x86/include/asm/msr.h
@@ -224,8 +224,8 @@ do {                                                            \
 #ifdef CONFIG_SMP
 int rdmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 *l, u32 *h);
 int wrmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 l, u32 h);
-void rdmsr_on_cpus(const cpumask_t *mask, u32 msr_no, struct msr *msrs);
-void wrmsr_on_cpus(const cpumask_t *mask, u32 msr_no, struct msr *msrs);
+void rdmsr_on_cpus(const struct cpumask *mask, u32 msr_no, struct msr *msrs);
+void wrmsr_on_cpus(const struct cpumask *mask, u32 msr_no, struct msr *msrs);
 int rdmsr_safe_on_cpu(unsigned int cpu, u32 msr_no, u32 *l, u32 *h);
 int wrmsr_safe_on_cpu(unsigned int cpu, u32 msr_no, u32 l, u32 h);
 #else  /*  CONFIG_SMP  */
diff --git a/arch/x86/lib/msr.c b/arch/x86/lib/msr.c
index caa24ac..2f26cf4 100644
--- a/arch/x86/lib/msr.c
+++ b/arch/x86/lib/msr.c
@@ -71,14 +71,9 @@ int wrmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 l, u32 h)
 }
 EXPORT_SYMBOL(wrmsr_on_cpu);
 
-/* rdmsr on a bunch of CPUs
- *
- * @mask:       which CPUs
- * @msr_no:     which MSR
- * @msrs:       array of MSR values
- *
- */
-void rdmsr_on_cpus(const cpumask_t *mask, u32 msr_no, struct msr *msrs)
+static void __rwmsr_on_cpus(const struct cpumask *mask, u32 msr_no,
+			    struct msr *msrs,
+			    void (*msr_func) (void *info))
 {
 	struct msr_info rv;
 	int this_cpu;
@@ -92,11 +87,23 @@ void rdmsr_on_cpus(const cpumask_t *mask, u32 msr_no, struct msr *msrs)
 	this_cpu = get_cpu();
 
 	if (cpumask_test_cpu(this_cpu, mask))
-		__rdmsr_on_cpu(&rv);
+		msr_func(&rv);
 
-	smp_call_function_many(mask, __rdmsr_on_cpu, &rv, 1);
+	smp_call_function_many(mask, msr_func, &rv, 1);
 	put_cpu();
 }
+
+/* rdmsr on a bunch of CPUs
+ *
+ * @mask:       which CPUs
+ * @msr_no:     which MSR
+ * @msrs:       array of MSR values
+ *
+ */
+void rdmsr_on_cpus(const struct cpumask *mask, u32 msr_no, struct msr *msrs)
+{
+	__rwmsr_on_cpus(mask, msr_no, msrs, __rdmsr_on_cpu);
+}
 EXPORT_SYMBOL(rdmsr_on_cpus);
 
 /*
@@ -107,24 +114,9 @@ EXPORT_SYMBOL(rdmsr_on_cpus);
  * @msrs:       array of MSR values
  *
  */
-void wrmsr_on_cpus(const cpumask_t *mask, u32 msr_no, struct msr *msrs)
+void wrmsr_on_cpus(const struct cpumask *mask, u32 msr_no, struct msr *msrs)
 {
-	struct msr_info rv;
-	int this_cpu;
-
-	memset(&rv, 0, sizeof(rv));
-
-	rv.off    = cpumask_first(mask);
-	rv.msrs   = msrs;
-	rv.msr_no = msr_no;
-
-	this_cpu = get_cpu();
-
-	if (cpumask_test_cpu(this_cpu, mask))
-		__wrmsr_on_cpu(&rv);
-
-	smp_call_function_many(mask, __wrmsr_on_cpu, &rv, 1);
-	put_cpu();
+	__rwmsr_on_cpus(mask, msr_no, msrs, __wrmsr_on_cpu);
 }
 EXPORT_SYMBOL(wrmsr_on_cpus);
 
-- 
1.6.3.3

Re: [PATCH] x86, msr: unify rdmsr_on_cpus/wrmsr_on_cpus

[Borislav Petkov]

Hi Peter,

care to pick up the one below too while the merge window is still open?

On Thu, Jul 30, 2009 at 11:10:02AM +0200, Borislav Petkov wrote:
> Since rdmsr_on_cpus and wrmsr_on_cpus are almost identical, unify them
> into a common __rwmsr_on_cpus helper thus avoiding code duplication.
> 
> While at it, convert cpumask_t's to const struct cpumask *.
> 
> Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
> ---
> 
> This one is .32 material.
> 
>  arch/x86/include/asm/msr.h |    4 +-
>  arch/x86/lib/msr.c         |   46 ++++++++++++++++++-------------------------
>  2 files changed, 21 insertions(+), 29 deletions(-)
> 
> diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
> index 48ad9d2..f2f4309 100644
> --- a/arch/x86/include/asm/msr.h
> +++ b/arch/x86/include/asm/msr.h
> @@ -224,8 +224,8 @@ do {                                                            \
>  #ifdef CONFIG_SMP
>  int rdmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 *l, u32 *h);
>  int wrmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 l, u32 h);
> -void rdmsr_on_cpus(const cpumask_t *mask, u32 msr_no, struct msr *msrs);
> -void wrmsr_on_cpus(const cpumask_t *mask, u32 msr_no, struct msr *msrs);
> +void rdmsr_on_cpus(const struct cpumask *mask, u32 msr_no, struct msr *msrs);
> +void wrmsr_on_cpus(const struct cpumask *mask, u32 msr_no, struct msr *msrs);
>  int rdmsr_safe_on_cpu(unsigned int cpu, u32 msr_no, u32 *l, u32 *h);
>  int wrmsr_safe_on_cpu(unsigned int cpu, u32 msr_no, u32 l, u32 h);
>  #else  /*  CONFIG_SMP  */
> diff --git a/arch/x86/lib/msr.c b/arch/x86/lib/msr.c
> index caa24ac..2f26cf4 100644
> --- a/arch/x86/lib/msr.c
> +++ b/arch/x86/lib/msr.c
> @@ -71,14 +71,9 @@ int wrmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 l, u32 h)
>  }
>  EXPORT_SYMBOL(wrmsr_on_cpu);
>  
> -/* rdmsr on a bunch of CPUs
> - *
> - * @mask:       which CPUs
> - * @msr_no:     which MSR
> - * @msrs:       array of MSR values
> - *
> - */
> -void rdmsr_on_cpus(const cpumask_t *mask, u32 msr_no, struct msr *msrs)
> +static void __rwmsr_on_cpus(const struct cpumask *mask, u32 msr_no,
> +			    struct msr *msrs,
> +			    void (*msr_func) (void *info))
>  {
>  	struct msr_info rv;
>  	int this_cpu;
> @@ -92,11 +87,23 @@ void rdmsr_on_cpus(const cpumask_t *mask, u32 msr_no, struct msr *msrs)
>  	this_cpu = get_cpu();
>  
>  	if (cpumask_test_cpu(this_cpu, mask))
> -		__rdmsr_on_cpu(&rv);
> +		msr_func(&rv);
>  
> -	smp_call_function_many(mask, __rdmsr_on_cpu, &rv, 1);
> +	smp_call_function_many(mask, msr_func, &rv, 1);
>  	put_cpu();
>  }
> +
> +/* rdmsr on a bunch of CPUs
> + *
> + * @mask:       which CPUs
> + * @msr_no:     which MSR
> + * @msrs:       array of MSR values
> + *
> + */
> +void rdmsr_on_cpus(const struct cpumask *mask, u32 msr_no, struct msr *msrs)
> +{
> +	__rwmsr_on_cpus(mask, msr_no, msrs, __rdmsr_on_cpu);
> +}
>  EXPORT_SYMBOL(rdmsr_on_cpus);
>  
>  /*
> @@ -107,24 +114,9 @@ EXPORT_SYMBOL(rdmsr_on_cpus);
>   * @msrs:       array of MSR values
>   *
>   */
> -void wrmsr_on_cpus(const cpumask_t *mask, u32 msr_no, struct msr *msrs)
> +void wrmsr_on_cpus(const struct cpumask *mask, u32 msr_no, struct msr *msrs)
>  {
> -	struct msr_info rv;
> -	int this_cpu;
> -
> -	memset(&rv, 0, sizeof(rv));
> -
> -	rv.off    = cpumask_first(mask);
> -	rv.msrs   = msrs;
> -	rv.msr_no = msr_no;
> -
> -	this_cpu = get_cpu();
> -
> -	if (cpumask_test_cpu(this_cpu, mask))
> -		__wrmsr_on_cpu(&rv);
> -
> -	smp_call_function_many(mask, __wrmsr_on_cpu, &rv, 1);
> -	put_cpu();
> +	__rwmsr_on_cpus(mask, msr_no, msrs, __wrmsr_on_cpu);
>  }
>  EXPORT_SYMBOL(wrmsr_on_cpus);
>  
> -- 
> 1.6.3.3
> 
> _______________________________________________
> osrc-patches mailing list
> osrc-patches@elbe.amd.com
> https://ddcwww.amd.com/mailman/listinfo/osrc-patches

-- 
Regards/Gruss,
Boris.

Operating | Advanced Micro Devices GmbH
  System  | Karl-Hammerschmidt-Str. 34, 85609 Dornach b. München, Germany
 Research | Geschäftsführer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
  Center  | Sitz: Dornach, Gemeinde Aschheim, Landkreis München
  (OSRC)  | Registergericht München, HRB Nr. 43632