[PATCH RESEND] x86: kernel/microcode_core.c simple_strtoul cleanup

[PATCH RESEND] x86: kernel/microcode_core.c simple_strtoul cleanup

[Shuah Khan]

Change reload_for_cpu() in kernel/microcode_core.c to call kstrtoul()
instead of calling obsoleted simple_strtoul().

Signed-off-by: Shuah Khan <shuahkhan@gmail.com>
---
 arch/x86/kernel/microcode_core.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
index c9bda6d..fbdfc69 100644
--- a/arch/x86/kernel/microcode_core.c
+++ b/arch/x86/kernel/microcode_core.c
@@ -299,12 +299,11 @@ static ssize_t reload_store(struct device *dev,
 {
 	unsigned long val;
 	int cpu = dev->id;
-	int ret = 0;
-	char *end;
+	ssize_t ret = 0;
 
-	val = simple_strtoul(buf, &end, 0);
-	if (end == buf)
-		return -EINVAL;
+	ret = kstrtoul(buf, 0, &val);
+	if (ret)
+		return ret;
 
 	if (val == 1) {
 		get_online_cpus();
-- 
1.7.9.5

Re: [PATCH RESEND] x86: kernel/microcode_core.c simple_strtoul cleanup

[Borislav Petkov]

On Sun, May 06, 2012 at 11:11:04AM -0600, Shuah Khan wrote:
> Change reload_for_cpu() in kernel/microcode_core.c to call kstrtoul()
> instead of calling obsoleted simple_strtoul().
> 
> Signed-off-by: Shuah Khan <shuahkhan@gmail.com>

Ok, much better, thanks.

@Ingo: I'll pick this one up unless you want to do that :-).

> ---
>  arch/x86/kernel/microcode_core.c |    9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
> index c9bda6d..fbdfc69 100644
> --- a/arch/x86/kernel/microcode_core.c
> +++ b/arch/x86/kernel/microcode_core.c
> @@ -299,12 +299,11 @@ static ssize_t reload_store(struct device *dev,
>  {
>  	unsigned long val;
>  	int cpu = dev->id;
> -	int ret = 0;
> -	char *end;
> +	ssize_t ret = 0;
>  
> -	val = simple_strtoul(buf, &end, 0);
> -	if (end == buf)
> -		return -EINVAL;
> +	ret = kstrtoul(buf, 0, &val);
> +	if (ret)
> +		return ret;
>  
>  	if (val == 1) {
>  		get_online_cpus();
> -- 
> 1.7.9.5

-- 
Regards/Gruss,
Boris.

Re: [PATCH RESEND] x86: kernel/microcode_core.c simple_strtoul cleanup

[H. Peter Anvin]

On 05/07/2012 03:49 AM, Borislav Petkov wrote:
> On Sun, May 06, 2012 at 11:11:04AM -0600, Shuah Khan wrote:
>> Change reload_for_cpu() in kernel/microcode_core.c to call kstrtoul()
>> instead of calling obsoleted simple_strtoul().
>>
>> Signed-off-by: Shuah Khan <shuahkhan@gmail.com>
> 
> Ok, much better, thanks.
> 
> @Ingo: I'll pick this one up unless you want to do that :-).
> 

I'll pick it up.  I presume I have your ack on it?

	-hpa

Re: [PATCH RESEND] x86: kernel/microcode_core.c simple_strtoul cleanup

[Borislav Petkov]

On Mon, May 07, 2012 at 11:35:15AM -0700, H. Peter Anvin wrote:
> I'll pick it up.  I presume I have your ack on it?

Yep, sure.

Btw, while at it, I gave the whole sysfs reload thing a critical look
and whether it is at all that useful - this thing gives you microcode
reloading on a single CPU. And what you actually wanna do is reload the
microcode on the whole system, i.e. all cores in succession.

And we don't use the reload thing on AMD, so I was wondering, if you
guys don't find it useful on Intel hw, maybe we can remove it completely
in favor of

$ rmmod microcode; modprobe microcode

which reloads the ucode on each core.

Of course, one can iterate over each core in a shell-loop and write into
the reload file to reload ucode after having updated the ucode image in
/lib/firmware but removing and then modprobing the module is shorter :-)

Hmm?

-- 
Regards/Gruss,
    Boris.

Re: [PATCH RESEND] x86: kernel/microcode_core.c simple_strtoul cleanup

[Henrique de Moraes Holschuh]

On Mon, 07 May 2012, Borislav Petkov wrote:
> On Mon, May 07, 2012 at 11:35:15AM -0700, H. Peter Anvin wrote:
> > I'll pick it up.  I presume I have your ack on it?
> 
> Yep, sure.
> 
> Btw, while at it, I gave the whole sysfs reload thing a critical look
> and whether it is at all that useful - this thing gives you microcode
> reloading on a single CPU. And what you actually wanna do is reload the
> microcode on the whole system, i.e. all cores in succession.
> 
> And we don't use the reload thing on AMD, so I was wondering, if you
> guys don't find it useful on Intel hw, maybe we can remove it completely
> in favor of
> 
> $ rmmod microcode; modprobe microcode
> 
> which reloads the ucode on each core.
> 
> Of course, one can iterate over each core in a shell-loop and write into
> the reload file to reload ucode after having updated the ucode image in
> /lib/firmware but removing and then modprobing the module is shorter :-)

Can we PLEASE fix it properly by adding a new node (which is _not_ per-cpu)
that requests the microcode core to refresh all cpus?  Preferably by
invalidating the microcode cache, THEN fetching each required microcode just
once for the first core that needs it, and caching it for use the other
cores.  You can leave the (IMHO mostly useless) per-cpu sysfs nodes alone,
so as to not break ABI, or deprecate them for an year or something.

I am speaking this with my userland maintainer hat.  I *do NOT* want to
rmmod crap in a production server to update microcode.  And I want to be
able to support static-compiled microcode.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH RESEND] x86: kernel/microcode_core.c simple_strtoul cleanup

[Ingo Molnar]

* Henrique de Moraes Holschuh <hmh@hmh.eng.br> wrote:

> > Of course, one can iterate over each core in a shell-loop 
> > and write into the reload file to reload ucode after having 
> > updated the ucode image in /lib/firmware but removing and 
> > then modprobing the module is shorter :-)
> 
> Can we PLEASE fix it properly by adding a new node (which is 
> _not_ per-cpu) that requests the microcode core to refresh all 
> cpus?  Preferably by invalidating the microcode cache, THEN 
> fetching each required microcode just once for the first core 
> that needs it, and caching it for use the other cores.  You 
> can leave the (IMHO mostly useless) per-cpu sysfs nodes alone, 
> so as to not break ABI, or deprecate them for an year or 
> something.
> 
> I am speaking this with my userland maintainer hat.  I *do 
> NOT* want to rmmod crap in a production server to update 
> microcode.  And I want to be able to support static-compiled 
> microcode.

Seconded. There's also the ability to disable module unloading.

Thanks,

	Ingo

Re: [PATCH RESEND] x86: kernel/microcode_core.c simple_strtoul cleanup

[Shuah Khan]

On Tue, 2012-05-08 at 06:00 +0200, Ingo Molnar wrote:
> * Henrique de Moraes Holschuh <hmh@hmh.eng.br> wrote:
> 
> > > Of course, one can iterate over each core in a shell-loop 
> > > and write into the reload file to reload ucode after having 
> > > updated the ucode image in /lib/firmware but removing and 
> > > then modprobing the module is shorter :-)
> > 
> > Can we PLEASE fix it properly by adding a new node (which is 
> > _not_ per-cpu) that requests the microcode core to refresh all 
> > cpus?  Preferably by invalidating the microcode cache, THEN 
> > fetching each required microcode just once for the first core 
> > that needs it, and caching it for use the other cores.  You 
> > can leave the (IMHO mostly useless) per-cpu sysfs nodes alone, 
> > so as to not break ABI, or deprecate them for an year or 
> > something.
> > 
> > I am speaking this with my userland maintainer hat.  I *do 
> > NOT* want to rmmod crap in a production server to update 
> > microcode.  And I want to be able to support static-compiled 
> > microcode.
> 
> Seconded. There's also the ability to disable module unloading.

I will take a stab at the disabling module unloading if I may. The rest
of the tasks are a bit out of my league at this time. :)

Thanks,
-- Shuah

Re: [PATCH RESEND] x86: kernel/microcode_core.c simple_strtoul cleanup

[Borislav Petkov]

On Tue, May 08, 2012 at 06:00:10AM +0200, Ingo Molnar wrote:
> 
> * Henrique de Moraes Holschuh <hmh@hmh.eng.br> wrote:
> 
> > > Of course, one can iterate over each core in a shell-loop 
> > > and write into the reload file to reload ucode after having 
> > > updated the ucode image in /lib/firmware but removing and 
> > > then modprobing the module is shorter :-)
> > 
> > Can we PLEASE fix it properly by adding a new node (which is 
> > _not_ per-cpu) that requests the microcode core to refresh all 
> > cpus?

Ok, where do you want to have it:

$ find /sys -iname "microcode"
/sys/bus/platform/devices/microcode
/sys/devices/system/cpu/cpu0/microcode
/sys/devices/system/cpu/cpu1/microcode
/sys/devices/virtual/misc/microcode
/sys/devices/platform/microcode
/sys/class/misc/microcode
/sys/module/microcode

I'm all for the level at

/sys/devices/system/cpu/

where CPU-related stuff can go in - not per-CPU but per whole system.
Ingo?

> > Preferably by invalidating the microcode cache, THEN fetching each
> > required microcode just once for the first core that needs it, and
> > caching it for use the other cores.

That should be easy, the most part is already there.

> > You can leave the (IMHO mostly useless) per-cpu sysfs nodes alone,
> > so as to not break ABI, or deprecate them for an year or something.

Yeah, about that, what is not breaking the ABI? Having the sysfs node but
reading/writing it returns -E<something>, having the sysfs node and
reading/writing it does nothing, or...?

> > I *do NOT* want to rmmod crap in a production server to update
> > microcode. And I want to be able to support static-compiled
> > microcode.

I'd need this more explained, why?

If by "static-compiled microcode" you mean the microcode.ko module
should be built-in that can't fly because for request_firmware we need
userspace to actually find the ucode image.

Thanks.

-- 
Regards/Gruss,
    Boris.

[tip:x86/microcode] x86, microcode: microcode_core. c simple_strtoul cleanup

[tip-bot for Shuah Khan]

Commit-ID:  e826abd523913f63eb03b59746ffb16153c53dc4
Gitweb:     http://git.kernel.org/tip/e826abd523913f63eb03b59746ffb16153c53dc4
Author:     Shuah Khan <shuahkhan@gmail.com>
AuthorDate: Sun, 6 May 2012 11:11:04 -0600
Committer:  H. Peter Anvin <hpa@linux.intel.com>
CommitDate: Mon, 7 May 2012 11:36:49 -0700

x86, microcode: microcode_core.c simple_strtoul cleanup

Change reload_for_cpu() in kernel/microcode_core.c to call kstrtoul()
instead of calling obsoleted simple_strtoul().

Signed-off-by: Shuah Khan <shuahkhan@gmail.com>
Reviewed-by: Borislav Petkov <bp@alien8.de>
Link: http://lkml.kernel.org/r/1336324264.2897.9.camel@lorien2
Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
---
 arch/x86/kernel/microcode_core.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
index c9bda6d..fbdfc69 100644
--- a/arch/x86/kernel/microcode_core.c
+++ b/arch/x86/kernel/microcode_core.c
@@ -299,12 +299,11 @@ static ssize_t reload_store(struct device *dev,
 {
 	unsigned long val;
 	int cpu = dev->id;
-	int ret = 0;
-	char *end;
+	ssize_t ret = 0;
 
-	val = simple_strtoul(buf, &end, 0);
-	if (end == buf)
-		return -EINVAL;
+	ret = kstrtoul(buf, 0, &val);
+	if (ret)
+		return ret;
 
 	if (val == 1) {
 		get_online_cpus();