[PATCH 0/2] x86, microcode: Reload ucode only per-system

[PATCH 0/2] x86, microcode: Reload ucode only per-system

[Borislav Petkov]

From: Borislav Petkov <borislav.petkov@amd.com>

Once upon a time there was this microcode reloading interface
/sys/devices/system/cpu/cpuX/microcode/reload, where X is an online
cpu on the system, which allowed the loading of microcode in a per-cpu
manner.

This had problems like having different ucode revisions on an otherwise
homogeneous system and needed O(n^2) overhead when tracking minimum
microcode revision per-core.

So make this interface per-system so that it does microcode reloading on
the whole system only.

Single commit messages have more info too.

The first patch has a stable tag which I'd like to see in stable but
since it is not fixing a direct regression, I'd like to not push it
upstream now but have it get tested in linux-next and go upstream during
the next merge window from where it can trickle slowly to stable.

Patches have been tested on all AMD families, it wouldn't hurt if it saw
some Intel testing too, although it should just work.

Holler if you see regressions/problems with it.

Thanks.

[PATCH 1/2] x86, microcode: Sanitize per-cpu microcode reloading interface

[Borislav Petkov]

From: Borislav Petkov <borislav.petkov@amd.com>

Microcode reloading in a per-core manner is a very bad idea for both
major x86 vendors. And the thing is, we have such interface with which
we can end up with different microcode versions applied on different
cores of an otherwise homogeneous wrt (family,model,stepping) system.

So turn off the possibility of doing that per core and allow it only
system-wide.

This is a minimal fix which we'd like to see in stable too thus the
more-or-less arbitrary decision to allow system-wide reloading only on
the BSP:

$ echo 1 > /sys/devices/system/cpu/cpu0/microcode/reload
...

and disable the interface on the other cores:

$ echo 1 > /sys/devices/system/cpu/cpu23/microcode/reload
-bash: echo: write error: Invalid argument

Also, allowing the reload only from one CPU (the BSP in
that case) doesn't allow the reload procedure to degenerate
into an O(n^2) deal when triggering reloads from all
/sys/devices/system/cpu/cpuX/microcode/reload sysfs nodes
simultaneously.

A more generic fix will follow.

Cc: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: stable@vger.kernel.org
Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 arch/x86/kernel/microcode_core.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
index fbdfc6917180..24b852b61be3 100644
--- a/arch/x86/kernel/microcode_core.c
+++ b/arch/x86/kernel/microcode_core.c
@@ -298,19 +298,31 @@ static ssize_t reload_store(struct device *dev,
 			    const char *buf, size_t size)
 {
 	unsigned long val;
-	int cpu = dev->id;
-	ssize_t ret = 0;
+	int cpu;
+	ssize_t ret = 0, tmp_ret;
+
+	/* allow reload only from the BSP */
+	if (boot_cpu_data.cpu_index != dev->id)
+		return -EINVAL;
 
 	ret = kstrtoul(buf, 0, &val);
 	if (ret)
 		return ret;
 
-	if (val == 1) {
-		get_online_cpus();
-		if (cpu_online(cpu))
-			ret = reload_for_cpu(cpu);
-		put_online_cpus();
+	if (val != 1)
+		return size;
+
+	get_online_cpus();
+	for_each_online_cpu(cpu) {
+		tmp_ret = reload_for_cpu(cpu);
+		if (tmp_ret != 0)
+			pr_warn("Error reloading microcode on CPU %d\n", cpu);
+
+		/* save retval of the first encountered reload error */
+		if (!ret)
+			ret = tmp_ret;
 	}
+	put_online_cpus();
 
 	if (!ret)
 		ret = size;
-- 
1.7.11.rc1

Re: [PATCH 1/2] x86, microcode: Sanitize per-cpu microcode reloading interface

[Henrique de Moraes Holschuh]

On Tue, 19 Jun 2012, Borislav Petkov wrote:
> From: Borislav Petkov <borislav.petkov@amd.com>
> 
> Microcode reloading in a per-core manner is a very bad idea for both
> major x86 vendors. And the thing is, we have such interface with which
> we can end up with different microcode versions applied on different
> cores of an otherwise homogeneous wrt (family,model,stepping) system.
> 
> So turn off the possibility of doing that per core and allow it only
> system-wide.
> 
> This is a minimal fix which we'd like to see in stable too thus the
> more-or-less arbitrary decision to allow system-wide reloading only on
> the BSP:
> 
> $ echo 1 > /sys/devices/system/cpu/cpu0/microcode/reload
> ...
> 
> and disable the interface on the other cores:
> 
> $ echo 1 > /sys/devices/system/cpu/cpu23/microcode/reload
> -bash: echo: write error: Invalid argument
> 
> Also, allowing the reload only from one CPU (the BSP in
> that case) doesn't allow the reload procedure to degenerate
> into an O(n^2) deal when triggering reloads from all
> /sys/devices/system/cpu/cpuX/microcode/reload sysfs nodes
> simultaneously.
> 
> A more generic fix will follow.
> 
> Cc: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: stable@vger.kernel.org
> Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>

Tested-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>

Tested on Intel Xeon X5550.  Tested as a (trivial) 3.0-stable backport.

> ---
>  arch/x86/kernel/microcode_core.c | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
> index fbdfc6917180..24b852b61be3 100644
> --- a/arch/x86/kernel/microcode_core.c
> +++ b/arch/x86/kernel/microcode_core.c
> @@ -298,19 +298,31 @@ static ssize_t reload_store(struct device *dev,
>  			    const char *buf, size_t size)
>  {
>  	unsigned long val;
> -	int cpu = dev->id;
> -	ssize_t ret = 0;
> +	int cpu;
> +	ssize_t ret = 0, tmp_ret;
> +
> +	/* allow reload only from the BSP */
> +	if (boot_cpu_data.cpu_index != dev->id)
> +		return -EINVAL;
>  
>  	ret = kstrtoul(buf, 0, &val);
>  	if (ret)
>  		return ret;
>  
> -	if (val == 1) {
> -		get_online_cpus();
> -		if (cpu_online(cpu))
> -			ret = reload_for_cpu(cpu);
> -		put_online_cpus();
> +	if (val != 1)
> +		return size;
> +
> +	get_online_cpus();
> +	for_each_online_cpu(cpu) {
> +		tmp_ret = reload_for_cpu(cpu);
> +		if (tmp_ret != 0)
> +			pr_warn("Error reloading microcode on CPU %d\n", cpu);
> +
> +		/* save retval of the first encountered reload error */
> +		if (!ret)
> +			ret = tmp_ret;
>  	}
> +	put_online_cpus();
>  
>  	if (!ret)
>  		ret = size;

-- 
  "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 1/2] x86, microcode: Sanitize per-cpu microcode reloading interface

[Yu, Fenghua]

> From: Borislav Petkov <bp@amd64.org>
> Date: Tue, 19 Jun 2012 18:03:30 +0200
> Subject: [PATCH 1/2] x86, microcode: Sanitize per-cpu microcode
> reloading interface
> To: X86-ML <x86@kernel.org>
> Cc: "H. Peter Anvin" <hpa@zytor.com>, Ingo Molnar <mingo@kernel.org>,
> Thomas Gleixner <tglx@linutronix.de>, LKML
> <linux-kernel@vger.kernel.org>, Andreas Herrmann
> <andreas.herrmann3@amd.com>, Borislav Petkov
> <borislav.petkov@amd.com>, Henrique de Moraes Holschuh
> <hmh@hmh.eng.br>, Peter Zijlstra <peterz@infradead.org>,
> stable@vger.kernel.org
> 
> From: Borislav Petkov <borislav.petkov@amd.com>
> 
> Microcode reloading in a per-core manner is a very bad idea for both
> major x86 vendors. And the thing is, we have such interface with which
> we can end up with different microcode versions applied on different
> cores of an otherwise homogeneous wrt (family,model,stepping) system.
> 
> So turn off the possibility of doing that per core and allow it only
> system-wide.
> 
> This is a minimal fix which we'd like to see in stable too thus the
> more-or-less arbitrary decision to allow system-wide reloading only on
> the BSP:
> 
> $ echo 1 > /sys/devices/system/cpu/cpu0/microcode/reload
> ...
> 
> and disable the interface on the other cores:
> 
> $ echo 1 > /sys/devices/system/cpu/cpu23/microcode/reload
> -bash: echo: write error: Invalid argument
> 
> Also, allowing the reload only from one CPU (the BSP in
> that case) doesn't allow the reload procedure to degenerate
> into an O(n^2) deal when triggering reloads from all
> /sys/devices/system/cpu/cpuX/microcode/reload sysfs nodes
> simultaneously.
> 
> A more generic fix will follow.
> 
> Cc: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: stable@vger.kernel.org
> Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
> ---
>  arch/x86/kernel/microcode_core.c | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kernel/microcode_core.c
> b/arch/x86/kernel/microcode_core.c
> index fbdfc6917180..24b852b61be3 100644
> --- a/arch/x86/kernel/microcode_core.c
> +++ b/arch/x86/kernel/microcode_core.c
> @@ -298,19 +298,31 @@ static ssize_t reload_store(struct device *dev,
>  			    const char *buf, size_t size)
>  {
>  	unsigned long val;
> -	int cpu = dev->id;
> -	ssize_t ret = 0;
> +	int cpu;
> +	ssize_t ret = 0, tmp_ret;
> +
> +	/* allow reload only from the BSP */
> +	if (boot_cpu_data.cpu_index != dev->id)
> +		return -EINVAL;

With the /sys/devices/system/cpu/microcode/reload interface in your patch 2/2, this will be broken, right? With the new interface, reload_store() can be executed on any cpu or dev. I think you need to remove this check if working with the patch 2/2.

Thanks.

-Fenghua

Re: [PATCH 1/2] x86, microcode: Sanitize per-cpu microcode reloading interface

[Borislav Petkov]

On Tue, Jun 19, 2012 at 11:15:56PM +0000, Yu, Fenghua wrote:
> > diff --git a/arch/x86/kernel/microcode_core.c
> > b/arch/x86/kernel/microcode_core.c
> > index fbdfc6917180..24b852b61be3 100644
> > --- a/arch/x86/kernel/microcode_core.c
> > +++ b/arch/x86/kernel/microcode_core.c
> > @@ -298,19 +298,31 @@ static ssize_t reload_store(struct device *dev,
> >  			    const char *buf, size_t size)
> >  {
> >  	unsigned long val;
> > -	int cpu = dev->id;
> > -	ssize_t ret = 0;
> > +	int cpu;
> > +	ssize_t ret = 0, tmp_ret;
> > +
> > +	/* allow reload only from the BSP */
> > +	if (boot_cpu_data.cpu_index != dev->id)
> > +		return -EINVAL;
> 
> With the /sys/devices/system/cpu/microcode/reload interface in your
> patch 2/2, this will be broken, right?

Actually it works per chance and passed testing. And I think I know why:
now the reload sysfs node is not per-cpu, this dev->id is always 0 and
thus the check passes.

> With the new interface, reload_store() can be executed on any cpu or
> dev. I think you need to remove this check if working with the patch
> 2/2.

But you're right, will fix.

Thanks for the review.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

[PATCH 2/2] x86, microcode: Make reload interface per system

[Borislav Petkov]

From: Borislav Petkov <borislav.petkov@amd.com>

The reload interface should be per-system so that a full system ucode
reload happens (on each core) when doing

echo 1 > /sys/devices/system/cpu/microcode/reload

Move it to the cpu subsys directory instead of it being per-cpu.

Cc: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Cc: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 arch/x86/kernel/microcode_core.c | 32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
index 24b852b61be3..4c6f3b37ed3c 100644
--- a/arch/x86/kernel/microcode_core.c
+++ b/arch/x86/kernel/microcode_core.c
@@ -351,7 +351,6 @@ static DEVICE_ATTR(version, 0400, version_show, NULL);
 static DEVICE_ATTR(processor_flags, 0400, pf_show, NULL);
 
 static struct attribute *mc_default_attrs[] = {
-	&dev_attr_reload.attr,
 	&dev_attr_version.attr,
 	&dev_attr_processor_flags.attr,
 	NULL
@@ -528,6 +527,16 @@ static const struct x86_cpu_id microcode_id[] = {
 MODULE_DEVICE_TABLE(x86cpu, microcode_id);
 #endif
 
+static struct attribute *cpu_root_microcode_attrs[] = {
+	&dev_attr_reload.attr,
+	NULL
+};
+
+static struct attribute_group cpu_root_microcode_group = {
+	.name  = "microcode",
+	.attrs = cpu_root_microcode_attrs,
+};
+
 static int __init microcode_init(void)
 {
 	struct cpuinfo_x86 *c = &cpu_data(0);
@@ -559,9 +568,17 @@ static int __init microcode_init(void)
 	if (error)
 		goto out_pdev;
 
+	error = sysfs_create_group(&cpu_subsys.dev_root->kobj,
+				   &cpu_root_microcode_group);
+
+	if (error) {
+		pr_err("Error creating microcode group!\n");
+		goto out_driver;
+	}
+
 	error = microcode_dev_init();
 	if (error)
-		goto out_driver;
+		goto out_ucode_group;
 
 	register_syscore_ops(&mc_syscore_ops);
 	register_hotcpu_notifier(&mc_cpu_notifier);
@@ -571,7 +588,11 @@ static int __init microcode_init(void)
 
 	return 0;
 
-out_driver:
+ out_ucode_group:
+	sysfs_remove_group(&cpu_subsys.dev_root->kobj,
+			   &cpu_root_microcode_group);
+
+ out_driver:
 	get_online_cpus();
 	mutex_lock(&microcode_mutex);
 
@@ -580,7 +601,7 @@ out_driver:
 	mutex_unlock(&microcode_mutex);
 	put_online_cpus();
 
-out_pdev:
+ out_pdev:
 	platform_device_unregister(microcode_pdev);
 	return error;
 
@@ -596,6 +617,9 @@ static void __exit microcode_exit(void)
 	unregister_hotcpu_notifier(&mc_cpu_notifier);
 	unregister_syscore_ops(&mc_syscore_ops);
 
+	sysfs_remove_group(&cpu_subsys.dev_root->kobj,
+			   &cpu_root_microcode_group);
+
 	get_online_cpus();
 	mutex_lock(&microcode_mutex);
 
-- 
1.7.11.rc1

Re: [PATCH 2/2] x86, microcode: Make reload interface per system

[Henrique de Moraes Holschuh]

On Tue, 19 Jun 2012, Borislav Petkov wrote:
> From: Borislav Petkov <borislav.petkov@amd.com>
> 
> The reload interface should be per-system so that a full system ucode
> reload happens (on each core) when doing
> 
> echo 1 > /sys/devices/system/cpu/microcode/reload
> 
> Move it to the cpu subsys directory instead of it being per-cpu.
> 
> Cc: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>

Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>

We will use /sys/devices/system/cpu/microcode/reload in Debian.

> ---
>  arch/x86/kernel/microcode_core.c | 32 ++++++++++++++++++++++++++++----
>  1 file changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
> index 24b852b61be3..4c6f3b37ed3c 100644
> --- a/arch/x86/kernel/microcode_core.c
> +++ b/arch/x86/kernel/microcode_core.c
> @@ -351,7 +351,6 @@ static DEVICE_ATTR(version, 0400, version_show, NULL);
>  static DEVICE_ATTR(processor_flags, 0400, pf_show, NULL);
>  
>  static struct attribute *mc_default_attrs[] = {
> -	&dev_attr_reload.attr,
>  	&dev_attr_version.attr,
>  	&dev_attr_processor_flags.attr,
>  	NULL
> @@ -528,6 +527,16 @@ static const struct x86_cpu_id microcode_id[] = {
>  MODULE_DEVICE_TABLE(x86cpu, microcode_id);
>  #endif
>  
> +static struct attribute *cpu_root_microcode_attrs[] = {
> +	&dev_attr_reload.attr,
> +	NULL
> +};
> +
> +static struct attribute_group cpu_root_microcode_group = {
> +	.name  = "microcode",
> +	.attrs = cpu_root_microcode_attrs,
> +};
> +
>  static int __init microcode_init(void)
>  {
>  	struct cpuinfo_x86 *c = &cpu_data(0);
> @@ -559,9 +568,17 @@ static int __init microcode_init(void)
>  	if (error)
>  		goto out_pdev;
>  
> +	error = sysfs_create_group(&cpu_subsys.dev_root->kobj,
> +				   &cpu_root_microcode_group);
> +
> +	if (error) {
> +		pr_err("Error creating microcode group!\n");
> +		goto out_driver;
> +	}
> +
>  	error = microcode_dev_init();
>  	if (error)
> -		goto out_driver;
> +		goto out_ucode_group;
>  
>  	register_syscore_ops(&mc_syscore_ops);
>  	register_hotcpu_notifier(&mc_cpu_notifier);
> @@ -571,7 +588,11 @@ static int __init microcode_init(void)
>  
>  	return 0;
>  
> -out_driver:
> + out_ucode_group:
> +	sysfs_remove_group(&cpu_subsys.dev_root->kobj,
> +			   &cpu_root_microcode_group);
> +
> + out_driver:
>  	get_online_cpus();
>  	mutex_lock(&microcode_mutex);
>  
> @@ -580,7 +601,7 @@ out_driver:
>  	mutex_unlock(&microcode_mutex);
>  	put_online_cpus();
>  
> -out_pdev:
> + out_pdev:
>  	platform_device_unregister(microcode_pdev);
>  	return error;
>  
> @@ -596,6 +617,9 @@ static void __exit microcode_exit(void)
>  	unregister_hotcpu_notifier(&mc_cpu_notifier);
>  	unregister_syscore_ops(&mc_syscore_ops);
>  
> +	sysfs_remove_group(&cpu_subsys.dev_root->kobj,
> +			   &cpu_root_microcode_group);
> +
>  	get_online_cpus();
>  	mutex_lock(&microcode_mutex);
>  

-- 
  "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 2/2] x86, microcode: Make reload interface per system

[Yu, Fenghua]

> From: Borislav Petkov <bp@amd64.org>
> Date: Tue, 19 Jun 2012 18:03:31 +0200
> Subject: [PATCH 2/2] x86, microcode: Make reload interface per system
> To: X86-ML <x86@kernel.org>
> Cc: "H. Peter Anvin" <hpa@zytor.com>, Ingo Molnar <mingo@kernel.org>,
> Thomas Gleixner <tglx@linutronix.de>, LKML
> <linux-kernel@vger.kernel.org>, Andreas Herrmann
> <andreas.herrmann3@amd.com>, Borislav Petkov
> <borislav.petkov@amd.com>, Henrique de Moraes Holschuh
> <hmh@hmh.eng.br>, Peter Zijlstra <peterz@infradead.org>
> 
> From: Borislav Petkov <borislav.petkov@amd.com>
> 
> The reload interface should be per-system so that a full system ucode
> reload happens (on each core) when doing
> 
> echo 1 > /sys/devices/system/cpu/microcode/reload
> 
> Move it to the cpu subsys directory instead of it being per-cpu.
> 
> Cc: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
> ---
>  arch/x86/kernel/microcode_core.c | 32 ++++++++++++++++++++++++++++----
>  1 file changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kernel/microcode_core.c
> b/arch/x86/kernel/microcode_core.c
> index 24b852b61be3..4c6f3b37ed3c 100644
> --- a/arch/x86/kernel/microcode_core.c
> +++ b/arch/x86/kernel/microcode_core.c
> @@ -351,7 +351,6 @@ static DEVICE_ATTR(version, 0400, version_show,
> NULL);
>  static DEVICE_ATTR(processor_flags, 0400, pf_show, NULL);
> 
>  static struct attribute *mc_default_attrs[] = {
> -	&dev_attr_reload.attr,
>  	&dev_attr_version.attr,
>  	&dev_attr_processor_flags.attr,
>  	NULL
> @@ -528,6 +527,16 @@ static const struct x86_cpu_id microcode_id[] = {
>  MODULE_DEVICE_TABLE(x86cpu, microcode_id);
>  #endif
> 
> +static struct attribute *cpu_root_microcode_attrs[] = {
> +	&dev_attr_reload.attr,
> +	NULL
> +};
> +
> +static struct attribute_group cpu_root_microcode_group = {
> +	.name  = "microcode",
> +	.attrs = cpu_root_microcode_attrs,
> +};
> +
>  static int __init microcode_init(void)
>  {
>  	struct cpuinfo_x86 *c = &cpu_data(0);
> @@ -559,9 +568,17 @@ static int __init microcode_init(void)
>  	if (error)
>  		goto out_pdev;
> 
> +	error = sysfs_create_group(&cpu_subsys.dev_root->kobj,
> +				   &cpu_root_microcode_group);
> +
> +	if (error) {
> +		pr_err("Error creating microcode group!\n");
> +		goto out_driver;
> +	}
> +
>  	error = microcode_dev_init();
>  	if (error)
> -		goto out_driver;
> +		goto out_ucode_group;
> 
If you put sysfs_create_group() after microcode_dev_init(), out_ucode_group is unnecessary and code is a bit simpler. Microcode_dev_init() doesn't rely on sysfs_create_group() and it's a NULL function any way.

>  	register_syscore_ops(&mc_syscore_ops);
>  	register_hotcpu_notifier(&mc_cpu_notifier);
> @@ -571,7 +588,11 @@ static int __init microcode_init(void)
> 
>  	return 0;
> 
> -out_driver:
> + out_ucode_group:
> +	sysfs_remove_group(&cpu_subsys.dev_root->kobj,
> +			   &cpu_root_microcode_group);
> +
> + out_driver:
>  	get_online_cpus();
>  	mutex_lock(&microcode_mutex);
> 
> @@ -580,7 +601,7 @@ out_driver:
>  	mutex_unlock(&microcode_mutex);
>  	put_online_cpus();
> 
> -out_pdev:
> + out_pdev:
>  	platform_device_unregister(microcode_pdev);
>  	return error;	
> 
> @@ -596,6 +617,9 @@ static void __exit microcode_exit(void)
>  	unregister_hotcpu_notifier(&mc_cpu_notifier);
>  	unregister_syscore_ops(&mc_syscore_ops);
> 
> +	sysfs_remove_group(&cpu_subsys.dev_root->kobj,
> +			   &cpu_root_microcode_group);
> +
>  	get_online_cpus();
>  	mutex_lock(&microcode_mutex);

Re: [PATCH 2/2] x86, microcode: Make reload interface per system

[Borislav Petkov]

On Tue, Jun 19, 2012 at 11:10:14PM +0000, Yu, Fenghua wrote:
> >  static int __init microcode_init(void)
> >  {
> >  	struct cpuinfo_x86 *c = &cpu_data(0);
> > @@ -559,9 +568,17 @@ static int __init microcode_init(void)
> >  	if (error)
> >  		goto out_pdev;
> > 
> > +	error = sysfs_create_group(&cpu_subsys.dev_root->kobj,
> > +				   &cpu_root_microcode_group);
> > +
> > +	if (error) {
> > +		pr_err("Error creating microcode group!\n");
> > +		goto out_driver;
> > +	}
> > +
> >  	error = microcode_dev_init();
> >  	if (error)
> > -		goto out_driver;
> > +		goto out_ucode_group;
> > 
> If you put sysfs_create_group() after microcode_dev_init(),
> out_ucode_group is unnecessary and code is a bit simpler.
> Microcode_dev_init() doesn't rely on sysfs_create_group() and it's a
> NULL function any way.

I don't think microcode_dev_init() is a NULL function since it depends
on CONFIG_MICROCODE_OLD_INTERFACE and we have it on by default:

config MICROCODE_OLD_INTERFACE
        def_bool y
        depends on MICROCODE

which makes me wonder whether we still need that OLD INTERFACE.
Henrique, any thoughts here?

But you're right, reordering the registration could simplify the code.
I'll take a look at it tomorrow.

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH 2/2] x86, microcode: Make reload interface per system

[Henrique de Moraes Holschuh]

On Wed, 20 Jun 2012, Borislav Petkov wrote:
> which makes me wonder whether we still need that OLD INTERFACE.
> Henrique, any thoughts here?

Well, there are distros out there that only have microcode.ctl, so you
should give them _some_ advanced warning.

As far as I am concerned you can simply deprecate /dev/cpu/microcode for,
say, two kernel releases, and then delete it.

-- 
  "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 2/2] x86, microcode: Make reload interface per system

[Borislav Petkov]

On Wed, Jun 20, 2012 at 12:26:58AM -0300, Henrique de Moraes Holschuh wrote:
> On Wed, 20 Jun 2012, Borislav Petkov wrote:
> > which makes me wonder whether we still need that OLD INTERFACE.
> > Henrique, any thoughts here?
> 
> Well, there are distros out there that only have microcode.ctl, so you
> should give them _some_ advanced warning.

Ok, I think Fedora was one of them.

But yeah, the advanced warning is a good idea.

> As far as I am concerned you can simply deprecate /dev/cpu/microcode for,
> say, two kernel releases, and then delete it.

Cool, will do that.

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH 2/2] x86, microcode: Make reload interface per system

[Henrique de Moraes Holschuh]

On Wed, 20 Jun 2012, Borislav Petkov wrote:
> On Wed, Jun 20, 2012 at 12:26:58AM -0300, Henrique de Moraes Holschuh wrote:
> > On Wed, 20 Jun 2012, Borislav Petkov wrote:
> > > which makes me wonder whether we still need that OLD INTERFACE.
> > > Henrique, any thoughts here?
> > 
> > Well, there are distros out there that only have microcode.ctl, so you
> > should give them _some_ advanced warning.
> 
> Ok, I think Fedora was one of them.

No, Fedora has a .c quick hack that can split Intel .dat files into
/lib/firmware/intel-ucode bin files.  Maybe it is not in their stable
branch yet.

-- 
  "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 2/2] x86, microcode: Make reload interface per system

[Peter Zijlstra]

On Wed, 2012-06-20 at 01:28 +0200, Borislav Petkov wrote:
> 
> which makes me wonder whether we still need that OLD INTERFACE.
> Henrique, any thoughts here? 

Yes I need it, I've no f'ing clue where to put the ucode image so that
the whole firmware mess can find it :-)

Re: [PATCH 2/2] x86, microcode: Make reload interface per system

[Borislav Petkov]

On Wed, Jun 20, 2012 at 10:59:38AM +0200, Peter Zijlstra wrote:
> On Wed, 2012-06-20 at 01:28 +0200, Borislav Petkov wrote:
> > 
> > which makes me wonder whether we still need that OLD INTERFACE.
> > Henrique, any thoughts here? 
> 
> Yes I need it, I've no f'ing clue where to put the ucode image so that
> the whole firmware mess can find it :-)

On AMD:

static enum ucode_state request_microcode_amd(int cpu, struct device *device)
{
        char fw_name[36] = "amd-ucode/microcode_amd.bin";
        const struct firmware *fw;
        enum ucode_state ret = UCODE_NFOUND;
        struct cpuinfo_x86 *c = &cpu_data(cpu);

        if (c->x86 >= 0x15)
                snprintf(fw_name, sizeof(fw_name), "amd-ucode/microcode_amd_fam%.2xh.bin", c->x86);
	...


On Intel:

static enum ucode_state request_microcode_fw(int cpu, struct device *device)
{
        char name[30];
        struct cpuinfo_x86 *c = &cpu_data(cpu);
        const struct firmware *firmware;
        enum ucode_state ret;

        sprintf(name, "intel-ucode/%02x-%02x-%02x",
                c->x86, c->x86_model, c->x86_mask);

	...

And that's all under /lib/firmware/

Phew, now that Peter is converted, we can kill the old interface too :-)

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH 2/2] x86, microcode: Make reload interface per system

[Peter Zijlstra]

On Wed, 2012-06-20 at 11:56 +0200, Borislav Petkov wrote:
>         sprintf(name, "intel-ucode/%02x-%02x-%02x",
>                 c->x86, c->x86_model, c->x86_mask);
> 
> 

but but but, the ucode image actually contains stuff for multiple
things.. so I have to like rename it to match the actual cpu in the
machine?

That seems rather daft..

awk 'BEGIN { FS=":" } /family/ {fam=$2} /model[^ ]/ {mod=$2} /stepping/
{step=$2} END { printf "intel-ucode/%02x-%02x-%02x\n", fam, mod,
step }' /proc/cpuinfo

Will get me the right filename, but surely that's not user-friendly at
all...

So no, that's not an improvement over /dev/microcode

Re: [PATCH 2/2] x86, microcode: Make reload interface per system

[Borislav Petkov]

On Wed, Jun 20, 2012 at 12:08:51PM +0200, Peter Zijlstra wrote:
> On Wed, 2012-06-20 at 11:56 +0200, Borislav Petkov wrote:
> >         sprintf(name, "intel-ucode/%02x-%02x-%02x",
> >                 c->x86, c->x86_model, c->x86_mask);
> > 
> > 
> 
> but but but, the ucode image actually contains stuff for multiple
> things.. so I have to like rename it to match the actual cpu in the
> machine?
> 
> That seems rather daft..
> 
> awk 'BEGIN { FS=":" } /family/ {fam=$2} /model[^ ]/ {mod=$2} /stepping/
> {step=$2} END { printf "intel-ucode/%02x-%02x-%02x\n", fam, mod,
> step }' /proc/cpuinfo
> 
> Will get me the right filename, but surely that's not user-friendly at
> all...

I know, right. Whose idea was it to do it like that I don't know.
AFAICT, Intel delivers ucode as a single blob too, so why split it? The
driver picks out the right blob anyway.

But there was a script somewhere which did that automatically. Henrique
will know more about how this is handled.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH 2/2] x86, microcode: Make reload interface per system

[Peter Zijlstra]

On Wed, 2012-06-20 at 12:19 +0200, Borislav Petkov wrote:
> On Wed, Jun 20, 2012 at 12:08:51PM +0200, Peter Zijlstra wrote:
> > On Wed, 2012-06-20 at 11:56 +0200, Borislav Petkov wrote:
> > >         sprintf(name, "intel-ucode/%02x-%02x-%02x",
> > >                 c->x86, c->x86_model, c->x86_mask);
> > > 
> > > 
> > 
> > but but but, the ucode image actually contains stuff for multiple
> > things.. so I have to like rename it to match the actual cpu in the
> > machine?
> > 
> > That seems rather daft..
> > 
> > awk 'BEGIN { FS=":" } /family/ {fam=$2} /model[^ ]/ {mod=$2} /stepping/
> > {step=$2} END { printf "intel-ucode/%02x-%02x-%02x\n", fam, mod,
> > step }' /proc/cpuinfo
> > 
> > Will get me the right filename, but surely that's not user-friendly at
> > all...
> 
> I know, right. Whose idea was it to do it like that I don't know.
> AFAICT, Intel delivers ucode as a single blob too, so why split it? The
> driver picks out the right blob anyway.

Right, so since we're mucking about with the whole interface anyway,
can't we fix this too?

Re: [PATCH 2/2] x86, microcode: Make reload interface per system

[Borislav Petkov]

On Wed, Jun 20, 2012 at 12:22:00PM +0200, Peter Zijlstra wrote:
> On Wed, 2012-06-20 at 12:19 +0200, Borislav Petkov wrote:
> > On Wed, Jun 20, 2012 at 12:08:51PM +0200, Peter Zijlstra wrote:
> > > On Wed, 2012-06-20 at 11:56 +0200, Borislav Petkov wrote:
> > > >         sprintf(name, "intel-ucode/%02x-%02x-%02x",
> > > >                 c->x86, c->x86_model, c->x86_mask);
> > > > 
> > > > 
> > > 
> > > but but but, the ucode image actually contains stuff for multiple
> > > things.. so I have to like rename it to match the actual cpu in the
> > > machine?
> > > 
> > > That seems rather daft..
> > > 
> > > awk 'BEGIN { FS=":" } /family/ {fam=$2} /model[^ ]/ {mod=$2} /stepping/
> > > {step=$2} END { printf "intel-ucode/%02x-%02x-%02x\n", fam, mod,
> > > step }' /proc/cpuinfo
> > > 
> > > Will get me the right filename, but surely that's not user-friendly at
> > > all...
> > 
> > I know, right. Whose idea was it to do it like that I don't know.
> > AFAICT, Intel delivers ucode as a single blob too, so why split it? The
> > driver picks out the right blob anyway.
> 
> Right, so since we're mucking about with the whole interface anyway,
> can't we fix this too?

Right,

so you can download Intel ucode from here, for example:

http://downloadcenter.intel.com/Detail_Desc.aspx?ProductID=483&DwnldID=14303

and the binary blob is 1.2 MB unpacked. Which could explain why it is
being split - I can see how loading 1.2 MB from userspace could be a
PITA.

But if we had a script which would split it and we had it in tools/,
then all would be fine, right?

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH 2/2] x86, microcode: Make reload interface per system

[Peter Zijlstra]

On Wed, 2012-06-20 at 12:27 +0200, Borislav Petkov wrote:
> 
> But if we had a script which would split it and we had it in tools/,
> then all would be fine, right? 

Yeah.. I guess so, but what is 1.2MB in this day and age.

Do put a comment in microcode_intel.c where to find this tool, even if
its in the kernel source.

Re: [PATCH 2/2] x86, microcode: Make reload interface per system

[Borislav Petkov]

On Wed, Jun 20, 2012 at 12:33:11PM +0200, Peter Zijlstra wrote:
> On Wed, 2012-06-20 at 12:27 +0200, Borislav Petkov wrote:
> > 
> > But if we had a script which would split it and we had it in tools/,
> > then all would be fine, right? 
> 
> Yeah.. I guess so, but what is 1.2MB in this day and age.
> 
> Do put a comment in microcode_intel.c where to find this tool, even if
> its in the kernel source.

Yeah, both should be workable. Let's see what Intel wants to do: hpa,
Fenghua?

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH 2/2] x86, microcode: Make reload interface per system

[H. Peter Anvin]

On 06/20/2012 04:09 AM, Borislav Petkov wrote:
> 
> Yeah, both should be workable. Let's see what Intel wants to do: hpa,
> Fenghua?
> 

What we want to do is to have a unified binary blob (which would be
about half that size) but the format may be affected by Fenghua's
ongoing work so we're not quite ready to rev the format now just to do
it again.

	-hpa

Re: [PATCH 2/2] x86, microcode: Make reload interface per system

[Henrique de Moraes Holschuh]

On Fri, 22 Jun 2012, H. Peter Anvin wrote:
> On 06/20/2012 04:09 AM, Borislav Petkov wrote:
> > Yeah, both should be workable. Let's see what Intel wants to do: hpa,
> > Fenghua?
> 
> What we want to do is to have a unified binary blob (which would be
> about half that size) but the format may be affected by Fenghua's
> ongoing work so we're not quite ready to rev the format now just to do
> it again.

Any reason why you can't just take a single file with one binary microcode
appended back-to-back after the other (i.e. exactly what /dev/cpu/microcode
accepts)?

You'd only be able to release that memory after all cores were brought
online [and any microcode that did get used to update a core was copied
somewhere else for future use], but that's hardly a big problem.

-- 
  "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 2/2] x86, microcode: Make reload interface per system

[H. Peter Anvin]

Not quite.

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

>On Fri, 22 Jun 2012, H. Peter Anvin wrote:
>> On 06/20/2012 04:09 AM, Borislav Petkov wrote:
>> > Yeah, both should be workable. Let's see what Intel wants to do:
>hpa,
>> > Fenghua?
>> 
>> What we want to do is to have a unified binary blob (which would be
>> about half that size) but the format may be affected by Fenghua's
>> ongoing work so we're not quite ready to rev the format now just to
>do
>> it again.
>
>Any reason why you can't just take a single file with one binary
>microcode
>appended back-to-back after the other (i.e. exactly what
>/dev/cpu/microcode
>accepts)?
>
>You'd only be able to release that memory after all cores were brought
>online [and any microcode that did get used to update a core was copied
>somewhere else for future use], but that's hardly a big problem.
>
>-- 
>  "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

-- 
Sent from my mobile phone. Please excuse brevity and lack of formatting.

Re: [PATCH 2/2] x86, microcode: Make reload interface per system

[Henrique de Moraes Holschuh]

On Wed, 20 Jun 2012, Peter Zijlstra wrote:
> On Wed, 2012-06-20 at 12:19 +0200, Borislav Petkov wrote:
> > On Wed, Jun 20, 2012 at 12:08:51PM +0200, Peter Zijlstra wrote:
> > > On Wed, 2012-06-20 at 11:56 +0200, Borislav Petkov wrote:
> > > >         sprintf(name, "intel-ucode/%02x-%02x-%02x",
> > > >                 c->x86, c->x86_model, c->x86_mask);
> > > 
> > > but but but, the ucode image actually contains stuff for multiple
> > > things.. so I have to like rename it to match the actual cpu in the
> > > machine?
> > > 
> > > That seems rather daft..
> > > 
> > > awk 'BEGIN { FS=":" } /family/ {fam=$2} /model[^ ]/ {mod=$2} /stepping/
> > > {step=$2} END { printf "intel-ucode/%02x-%02x-%02x\n", fam, mod,
> > > step }' /proc/cpuinfo
> > > 
> > > Will get me the right filename, but surely that's not user-friendly at
> > > all...
> > 
> > I know, right. Whose idea was it to do it like that I don't know.
> > AFAICT, Intel delivers ucode as a single blob too, so why split it? The
> > driver picks out the right blob anyway.
> 
> Right, so since we're mucking about with the whole interface anyway,
> can't we fix this too?

Please!

Just make it attempt to load a statically named firmware file first, and
fall back to the older variable naming.

If the reason for the weird naming is a size concern, "iucode_tool
--scan-system -w /lib/firmware/intel-ucode/x86.bin" (or whatever you name
it) would create a file with just the microcode required by the (online)
processors in the running system.

-- 
  "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 2/2] x86, microcode: Make reload interface per system

[Borislav Petkov]

On Wed, Jun 20, 2012 at 08:21:17PM -0300, Henrique de Moraes Holschuh wrote:
> > Right, so since we're mucking about with the whole interface anyway,
> > can't we fix this too?
> 
> Please!
> 
> Just make it attempt to load a statically named firmware file first, and
> fall back to the older variable naming.
> 
> If the reason for the weird naming is a size concern, "iucode_tool
> --scan-system -w /lib/firmware/intel-ucode/x86.bin" (or whatever you name
> it) would create a file with just the microcode required by the (online)
> processors in the running system.

But why go to all that trouble? Why not just load the single blob and
let the ucode driver figure out which patch is needed?

The way we do it, btw.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH 2/2] x86, microcode: Make reload interface per system

[Nix]

On 20 Jun 2012, Borislav Petkov uttered the following:
> I know, right. Whose idea was it to do it like that I don't know.
> AFAICT, Intel delivers ucode as a single blob too, so why split it? The
> driver picks out the right blob anyway.

Only if supplied over the old interface. Over the new interface,
we just see

    microcode: error! Bad data in microcode data file

in the log. So clearly the driver doesn't know how to split up the
microcode.bin that Intel provides, and (until Henrique's iucode-tool or
something like it becomes ubiquitous) the old interface, and
microcode_ctl, cannot be removed.

-- 
NULL && (void)

Re: [PATCH 2/2] x86, microcode: Make reload interface per system

[H. Peter Anvin]

On 06/22/2012 09:26 AM, Nix wrote:
> On 20 Jun 2012, Borislav Petkov uttered the following:
>> I know, right. Whose idea was it to do it like that I don't know.
>> AFAICT, Intel delivers ucode as a single blob too, so why split it? The
>> driver picks out the right blob anyway.
> 
> Only if supplied over the old interface. Over the new interface,
> we just see
> 
>     microcode: error! Bad data in microcode data file
> 
> in the log. So clearly the driver doesn't know how to split up the
> microcode.bin that Intel provides, and (until Henrique's iucode-tool or
> something like it becomes ubiquitous) the old interface, and
> microcode_ctl, cannot be removed.
> 

Even more importantly, to do early microcode updates we need to stash
away not just the current CPU's microcode but any compatible CPU's
microcode, so just loading a single one is not going to work...

Oif.

	-hpa

Re: [PATCH 2/2] x86, microcode: Make reload interface per system

[Henrique de Moraes Holschuh]

On Fri, 22 Jun 2012, H. Peter Anvin wrote:
> On 06/22/2012 09:26 AM, Nix wrote:
> > On 20 Jun 2012, Borislav Petkov uttered the following:
> >> I know, right. Whose idea was it to do it like that I don't know.
> >> AFAICT, Intel delivers ucode as a single blob too, so why split it? The
> >> driver picks out the right blob anyway.
> > 
> > Only if supplied over the old interface. Over the new interface,
> > we just see
> > 
> >     microcode: error! Bad data in microcode data file
> > 
> > in the log. So clearly the driver doesn't know how to split up the
> > microcode.bin that Intel provides, and (until Henrique's iucode-tool or
> > something like it becomes ubiquitous) the old interface, and
> > microcode_ctl, cannot be removed.
> 
> Even more importantly, to do early microcode updates we need to stash
> away not just the current CPU's microcode but any compatible CPU's
> microcode, so just loading a single one is not going to work...

Well, THAT is rather difficult with Intel.  You really have no choice
other than packing all microcodes, or just the ones for the current
system.  For all microcodes, it will compresses to around 350KiB with
gzip.

-- 
  "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 2/2] x86, microcode: Make reload interface per system

[Henrique de Moraes Holschuh]

On Wed, 20 Jun 2012, Peter Zijlstra wrote:
> On Wed, 2012-06-20 at 01:28 +0200, Borislav Petkov wrote:
> > which makes me wonder whether we still need that OLD INTERFACE.
> > Henrique, any thoughts here? 
> 
> Yes I need it, I've no f'ing clue where to put the ucode image so that
> the whole firmware mess can find it :-)

iucode_tool -K <name of intel microcode .dat/.bin file(s)> will do it
for you :p

-- 
  "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 2/2] x86, microcode: Make reload interface per system

[H. Peter Anvin]

On 06/20/2012 04:08 PM, Henrique de Moraes Holschuh wrote:
> On Wed, 20 Jun 2012, Peter Zijlstra wrote:
>> On Wed, 2012-06-20 at 01:28 +0200, Borislav Petkov wrote:
>>> which makes me wonder whether we still need that OLD INTERFACE.
>>> Henrique, any thoughts here? 
>>
>> Yes I need it, I've no f'ing clue where to put the ucode image so that
>> the whole firmware mess can find it :-)
> 
> iucode_tool -K <name of intel microcode .dat/.bin file(s)> will do it
> for you :p
> 

I'm wondering about the new format... it seems to potentially make it
harder to do the proper retaining of compatible CPU microcodes.

	-hpa

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

Re: [PATCH 2/2] x86, microcode: Make reload interface per system

[Borislav Petkov]

On Wed, Jun 20, 2012 at 04:10:18PM -0700, H. Peter Anvin wrote:
> I'm wondering about the new format... it seems to potentially make it
> harder to do the proper retaining of compatible CPU microcodes.

.. and if you don't split the blob, you have all the ucode patches in
one package. So are there any advantages in splitting the single blob?

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH 2/2] x86, microcode: Make reload interface per system

[H. Peter Anvin]

On 06/20/2012 04:23 PM, Borislav Petkov wrote:
> On Wed, Jun 20, 2012 at 04:10:18PM -0700, H. Peter Anvin wrote:
>> I'm wondering about the new format... it seems to potentially make it
>> harder to do the proper retaining of compatible CPU microcodes.
> 
> .. and if you don't split the blob, you have all the ucode patches in
> one package. So are there any advantages in splitting the single blob?
> 

Not that I know of.  IMO the driver can, and should, be the one doing
the pruning.

	-hpa

Re: [PATCH 2/2] x86, microcode: Make reload interface per system

[Borislav Petkov]

On Wed, Jun 20, 2012 at 04:27:58PM -0700, H. Peter Anvin wrote:
> On 06/20/2012 04:23 PM, Borislav Petkov wrote:
> > On Wed, Jun 20, 2012 at 04:10:18PM -0700, H. Peter Anvin wrote:
> >> I'm wondering about the new format... it seems to potentially make it
> >> harder to do the proper retaining of compatible CPU microcodes.
> > 
> > .. and if you don't split the blob, you have all the ucode patches in
> > one package. So are there any advantages in splitting the single blob?
> > 
> 
> Not that I know of.  IMO the driver can, and should, be the one doing
> the pruning.

Which means that

static enum ucode_state request_microcode_fw(int cpu, struct device *device)
{
        char name[30];

	...

        sprintf(name, "intel-ucode/%02x-%02x-%02x",
                c->x86, c->x86_model, c->x86_mask);

needs to be changed to the name of the microcode blob you guys use for
distributing and we can drop the OLD INTERFACE and there'll be no need
for userspace tools doing anything with the ucode patches.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH 2/2] x86, microcode: Make reload interface per system

[H. Peter Anvin]

On 06/20/2012 04:32 PM, Borislav Petkov wrote:
> 
> Which means that
> 
> static enum ucode_state request_microcode_fw(int cpu, struct device *device)
> {
>         char name[30];
> 
> 	...
> 
>         sprintf(name, "intel-ucode/%02x-%02x-%02x",
>                 c->x86, c->x86_model, c->x86_mask);
> 
> needs to be changed to the name of the microcode blob you guys use for
> distributing and we can drop the OLD INTERFACE and there'll be no need
> for userspace tools doing anything with the ucode patches.
> 

Except you still need to re-poke it when you have a new microcode
blob... so what was gained by all this churn?

	-hpa

Re: [PATCH 2/2] x86, microcode: Make reload interface per system

[Borislav Petkov]

On Wed, Jun 20, 2012 at 04:34:17PM -0700, H. Peter Anvin wrote:
> On 06/20/2012 04:32 PM, Borislav Petkov wrote:
> > 
> > Which means that
> > 
> > static enum ucode_state request_microcode_fw(int cpu, struct device *device)
> > {
> >         char name[30];
> > 
> > 	...
> > 
> >         sprintf(name, "intel-ucode/%02x-%02x-%02x",
> >                 c->x86, c->x86_model, c->x86_mask);
> > 
> > needs to be changed to the name of the microcode blob you guys use for
> > distributing and we can drop the OLD INTERFACE and there'll be no need
> > for userspace tools doing anything with the ucode patches.
> > 
> 
> Except you still need to re-poke it when you have a new microcode
> blob... so what was gained by all this churn?

We need to do that anyway if new (F,M,S) ucode piece comes along.

The gain is twofold:

* we don't need the userspace tool to split the blob - we have one
single file we load and the driver picks out what it needs.

* as a result, we drop the CONFIG_MICROCODE_OLD_INTERFACE, i.e.
/dev/cpu/microcode which takes the single blob anyway which the driver
picks apart later.

In the end, we have one unified ucode loading procedure:

1. put the blob in /lib/firmware/...
2. echo 1 > /sys/devices/system/cpu/microcode/reload

That's it - it can't be simpler than that.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH 2/2] x86, microcode: Make reload interface per system

[H. Peter Anvin]

On 06/20/2012 04:46 PM, Borislav Petkov wrote:
>>
>> Except you still need to re-poke it when you have a new microcode
>> blob... so what was gained by all this churn?
> 
> We need to do that anyway if new (F,M,S) ucode piece comes along.
> 
> The gain is twofold:
> 
> * we don't need the userspace tool to split the blob - we have one
> single file we load and the driver picks out what it needs.
> 
> * as a result, we drop the CONFIG_MICROCODE_OLD_INTERFACE, i.e.
> /dev/cpu/microcode which takes the single blob anyway which the driver
> picks apart later.
> 
> In the end, we have one unified ucode loading procedure:
> 
> 1. put the blob in /lib/firmware/...
> 2. echo 1 > /sys/devices/system/cpu/microcode/reload
> 
> That's it - it can't be simpler than that.
> 

I really don't get why the whole thing is any simpler than the old
/dev/cpu/microcode interface in the first place, where instead of:

> 1. put the blob in /lib/firmware/...
> 2. echo 1 > /sys/devices/system/cpu/microcode/reload

... you do ...

1. dd if=<microcode blob> of=/dev/cpu/microcode bs=<large>

... as well as set up the early initramfs part, of course.

	-hpa



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

Re: [PATCH 2/2] x86, microcode: Make reload interface per system

[Borislav Petkov]

On Wed, Jun 20, 2012 at 04:48:37PM -0700, H. Peter Anvin wrote:
> I really don't get why the whole thing is any simpler than the old
> /dev/cpu/microcode interface in the first place, where instead of:
> 
> > 1. put the blob in /lib/firmware/...
> > 2. echo 1 > /sys/devices/system/cpu/microcode/reload
> 
> ... you do ...
> 
> 1. dd if=<microcode blob> of=/dev/cpu/microcode bs=<large>

Ok, this is more complex to the normal user than doing "echo 1 >.. "

And "echo 1 >.. " is the best and simplest we can do, and without any
tools needed to do the job - just the shell.

And if we need additional tools like to strip some text files or
whatever, then the whole discussion about what distros distribute it how
and what own patches they apply on the tool, etc, etc, starts.

> ... as well as set up the early initramfs part, of course.

Yeah, we all agree on that.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH 2/2] x86, microcode: Make reload interface per system

[H. Peter Anvin]

On 06/21/2012 03:07 AM, Borislav Petkov wrote:
> 
> And if we need additional tools like to strip some text files or
> whatever, then the whole discussion about what distros distribute it how
> and what own patches they apply on the tool, etc, etc, starts.
> 

That is orthogonal.  Anyway, my only concern about the request_firmware
interface is that the timing will be inherently wrong.

	-hpa

Re: [PATCH 2/2] x86, microcode: Make reload interface per system

[Borislav Petkov]

On Thu, Jun 21, 2012 at 04:27:27PM -0700, H. Peter Anvin wrote:
> On 06/21/2012 03:07 AM, Borislav Petkov wrote:
> > 
> > And if we need additional tools like to strip some text files or
> > whatever, then the whole discussion about what distros distribute it how
> > and what own patches they apply on the tool, etc, etc, starts.
> > 
> 
> That is orthogonal.  Anyway, my only concern about the request_firmware
> interface is that the timing will be inherently wrong.

Timing? Please elaborate.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH 2/2] x86, microcode: Make reload interface per system

[H. Peter Anvin]

On 06/21/2012 07:56 PM, Borislav Petkov wrote:
>>
>> That is orthogonal.  Anyway, my only concern about the request_firmware
>> interface is that the timing will be inherently wrong.
> 
> Timing? Please elaborate.
> 

The request_firmware will happen as soon as the driver is loaded; with
the early microcode blob scheme the driver will need to be built in and
so the request_firmware will happen, redundantly, immediately...

	-hpa

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

Re: [PATCH 2/2] x86, microcode: Make reload interface per system

[Borislav Petkov]

On Thu, Jun 21, 2012 at 08:19:04PM -0700, H. Peter Anvin wrote:
> On 06/21/2012 07:56 PM, Borislav Petkov wrote:
> >>
> >> That is orthogonal.  Anyway, my only concern about the request_firmware
> >> interface is that the timing will be inherently wrong.
> > 
> > Timing? Please elaborate.
> > 
> 
> The request_firmware will happen as soon as the driver is loaded; with
> the early microcode blob scheme the driver will need to be built in and
> so the request_firmware will happen, redundantly, immediately...

I see the problem: we will just have updated the latest ucode from the
early scheme and then shortly after do a request_firmware to find out
that we don't have a newer patch anyway.

Hmm, ok, when I tried compiling in the microcode driver this week, it
didn't wait for 60 seconds because there was no userspace at 3 seconds
within the boot - it simply continued booting.

I think there's a guard for this in microcode_init_cpu:

        if (system_state != SYSTEM_RUNNING)
		return UCODE_NFOUND;

so that we don't call request_firmware that early.

But I haven't verified this yet.

In any case, we should disallow the request_firmware call temporarily
during init when the early scheme is in place.

But there is another problem:

What if BIOS has patch version 1 (numbers are only for showing what I
mean), then early scheme applies patch v2 but there is a newer patch
version 3 in /lib/firmware?

If the ucode driver is built in, we don't get to update to v3
automatically. User has to do it.

The current fix for this situation is have the microcode.ko as module
(and only allow it as M) which then automatically does request_firmware
at module init time and loads v3.

Which doesn't help people who don't build modules...

Hmmm.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH 2/2] x86, microcode: Make reload interface per system

[Markus Trippelsdorf]

On 2012.06.22 at 05:36 +0200, Borislav Petkov wrote:
> On Thu, Jun 21, 2012 at 08:19:04PM -0700, H. Peter Anvin wrote:
> > On 06/21/2012 07:56 PM, Borislav Petkov wrote:
> > >>
> > >> That is orthogonal.  Anyway, my only concern about the request_firmware
> > >> interface is that the timing will be inherently wrong.
> > > 
> > > Timing? Please elaborate.
> > > 
> > 
> > The request_firmware will happen as soon as the driver is loaded; with
> > the early microcode blob scheme the driver will need to be built in and
> > so the request_firmware will happen, redundantly, immediately...
> 
> What if BIOS has patch version 1 (numbers are only for showing what I
> mean), then early scheme applies patch v2 but there is a newer patch
> version 3 in /lib/firmware?
> 
> If the ucode driver is built in, we don't get to update to v3
> automatically. User has to do it.
> 
> The current fix for this situation is have the microcode.ko as module
> (and only allow it as M) which then automatically does request_firmware
> at module init time and loads v3.
> 
> Which doesn't help people who don't build modules...

People who don't use modules have:
 echo -n 1 > /sys/devices/system/cpu/cpu0/microcode/reload
 ...
in their init script already.

That will have to change to a single:
 echo -n 1 >| /sys/devices/system/cpu/microcode/reload
in your new scheme.

Of course the best solution would be to get rid of that manual reload
altogether and to automatically load the firmware during boot (even
without modules)...

-- 
Markus

Re: [PATCH 2/2] x86, microcode: Make reload interface per system

[Henrique de Moraes Holschuh]

On Thu, 21 Jun 2012, Borislav Petkov wrote:
> * we don't need the userspace tool to split the blob - we have one
> single file we load and the driver picks out what it needs.

Yes.  A smarter userspace tool is useful to reduce the size of the
microcode blob that ends up linked to the kernel or added to the
initramfs in distros that generate the initramfs at the target system
when you install a kernel (like Debian does), but it is in no way
*required*.

Typically you go from ~512KiB uncompressed (all binary microcodes) to
8-32KiB uncompressed when you target only the [online] processors of the
current system.  Nice for embedded, I suppose.

> In the end, we have one unified ucode loading procedure:
> 
> 1. put the blob in /lib/firmware/...
> 2. echo 1 > /sys/devices/system/cpu/microcode/reload
> 
> That's it - it can't be simpler than that.

Indeed.  And with an invariant microcode name for the Intel driver, one
can use the standard userspace tooling that detects which firmware needs
to go into the initramfs when no special tooling is installed.

-- 
  "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 2/2] x86, microcode: Make reload interface per system

[Henrique de Moraes Holschuh]

On Thu, 21 Jun 2012, Borislav Petkov wrote:
> Which means that
> 
> static enum ucode_state request_microcode_fw(int cpu, struct device *device)
> {
>         char name[30];
> 
> 	...
> 
>         sprintf(name, "intel-ucode/%02x-%02x-%02x",
>                 c->x86, c->x86_model, c->x86_mask);
> 
> needs to be changed to the name of the microcode blob you guys use for
> distributing and we can drop the OLD INTERFACE and there'll be no need
> for userspace tools doing anything with the ucode patches.

Only if Intel will stop distributing that text crap and distribute the
binary microcode already.

-- 
  "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 2/2] x86, microcode: Make reload interface per system

[Henrique de Moraes Holschuh]

On Wed, 20 Jun 2012, Henrique de Moraes Holschuh wrote:
> On Thu, 21 Jun 2012, Borislav Petkov wrote:
> > Which means that
> > 
> > static enum ucode_state request_microcode_fw(int cpu, struct device *device)
> > {
> >         char name[30];
> > 
> > 	...
> > 
> >         sprintf(name, "intel-ucode/%02x-%02x-%02x",
> >                 c->x86, c->x86_model, c->x86_mask);
> > 
> > needs to be changed to the name of the microcode blob you guys use for
> > distributing and we can drop the OLD INTERFACE and there'll be no need
> > for userspace tools doing anything with the ucode patches.
> 
> Only if Intel will stop distributing that text crap and distribute the
> binary microcode already.

I mean, for as long as Intel continues distributing text files, we will
have use for an userspace tool to convert it to binary and conveniently
package it to users in a binary package for the distro.

-- 
  "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 2/2] x86, microcode: Make reload interface per system

[Peter Zijlstra]

On Wed, 2012-06-20 at 20:08 -0300, Henrique de Moraes Holschuh wrote:
> On Wed, 20 Jun 2012, Peter Zijlstra wrote:
> > On Wed, 2012-06-20 at 01:28 +0200, Borislav Petkov wrote:
> > > which makes me wonder whether we still need that OLD INTERFACE.
> > > Henrique, any thoughts here? 
> > 
> > Yes I need it, I've no f'ing clue where to put the ucode image so that
> > the whole firmware mess can find it :-)
> 
> iucode_tool -K <name of intel microcode .dat/.bin file(s)> will do it
> for you :p

This thing is not installed on my machine, nor would I have know to look
for it. Nor does apt-cache search iucode_tool suggest any package.

IOW fail.

Re: [PATCH 2/2] x86, microcode: Make reload interface per system

[Borislav Petkov]

On Thu, Jun 21, 2012 at 10:24:35AM +0200, Peter Zijlstra wrote:
> This thing is not installed on my machine, nor would I have know
> to look for it. Nor does apt-cache search iucode_tool suggest any
> package.
>
> IOW fail.

FWIW, if we agree that we need a userspace tool for massaging Intel's
ucode, it should be in tools/ and it should be very easy to install even
on a distro which doesn't distribute it.

Emphasis on the initial "if", of course.

I'd very much prefer tool-less ucode application.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH 2/2] x86, microcode: Make reload interface per system

[H. Peter Anvin]

On 06/21/2012 02:58 AM, Borislav Petkov wrote:
> On Thu, Jun 21, 2012 at 10:24:35AM +0200, Peter Zijlstra wrote:
>> This thing is not installed on my machine, nor would I have know
>> to look for it. Nor does apt-cache search iucode_tool suggest any
>> package.
>>
>> IOW fail.
> 
> FWIW, if we agree that we need a userspace tool for massaging Intel's
> ucode, it should be in tools/ and it should be very easy to install even
> on a distro which doesn't distribute it.
> 
> Emphasis on the initial "if", of course.
> 
> I'd very much prefer tool-less ucode application.
> 

We're working on fixing that, however, we want to close on the early
ucode stuff and have it work first before we make any changes.

	-hpa

Re: [PATCH 2/2] x86, microcode: Make reload interface per system

[Henrique de Moraes Holschuh]

On Thu, 21 Jun 2012, Peter Zijlstra wrote:
> On Wed, 2012-06-20 at 20:08 -0300, Henrique de Moraes Holschuh wrote:
> > On Wed, 20 Jun 2012, Peter Zijlstra wrote:
> > > On Wed, 2012-06-20 at 01:28 +0200, Borislav Petkov wrote:
> > > > which makes me wonder whether we still need that OLD INTERFACE.
> > > > Henrique, any thoughts here? 
> > > 
> > > Yes I need it, I've no f'ing clue where to put the ucode image so that
> > > the whole firmware mess can find it :-)
> > 
> > iucode_tool -K <name of intel microcode .dat/.bin file(s)> will do it
> > for you :p
> 
> This thing is not installed on my machine, nor would I have know to look
> for it. Nor does apt-cache search iucode_tool suggest any package.
> 
> IOW fail.

Meh.  I uploaded the first public version of the thing a whole two weeks
ago!  It should have been all over the Internet by now ;-)

It is in Debian Wheezy (soon-to-be-stable) as of yesterday, I think.  It
will eventually end up in Ubuntu.  And if you guys think it is worthwhile to
maintain it upstream either in git.k.o, or as part of tools/, I have nothing
against it.

I posted the URL earlier, I think, but here it is:
http://cdn.debian.net/debian/pool/contrib/i/iucode-tool/iucode-tool_0.8.orig.tar.bz2

But it really exists to help distros do Intel microcode management and
release install-and-forget packages for end users.  End-users should get to
either ignore its existence entirely and get automated updates of microcode
as distro binary packages, or run an "update-intel-microcode" script (not
written yet, but there is one on Debian and Ubuntu's microcode.ctl package)
that downloads the blob from Intel, and does whatever is needed to install
it to /lib/firmware and apply it to the running system (and kernel image,
initramfs, whatever).

-- 
  "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 2/2] x86, microcode: Make reload interface per system

[Borislav Petkov]

On Thu, Jun 21, 2012 at 08:00:59PM -0300, Henrique de Moraes Holschuh wrote:
> Meh.  I uploaded the first public version of the thing a whole two weeks
> ago!  It should have been all over the Internet by now ;-)
> 
> It is in Debian Wheezy (soon-to-be-stable) as of yesterday, I think.  It
> will eventually end up in Ubuntu.  And if you guys think it is worthwhile to
> maintain it upstream either in git.k.o, or as part of tools/, I have nothing
> against it.
> 
> I posted the URL earlier, I think, but here it is:
> http://cdn.debian.net/debian/pool/contrib/i/iucode-tool/iucode-tool_0.8.orig.tar.bz2
> 
> But it really exists to help distros do Intel microcode management and
> release install-and-forget packages for end users.  End-users should get to
> either ignore its existence entirely and get automated updates of microcode
> as distro binary packages, or run an "update-intel-microcode" script (not
> written yet, but there is one on Debian and Ubuntu's microcode.ctl package)
> that downloads the blob from Intel, and does whatever is needed to install
> it to /lib/firmware and apply it to the running system (and kernel image,
> initramfs, whatever).

Yeah, this is all very helpful but let's see whether we are even going
to need a tool, at all. If we are tool-less, we don't depend on any
userspace stuff and ucode application just works like ftrace - no need
for a specific tool but the shell and debugfs (sysfs in our case).

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH 2/2] x86, microcode: Make reload interface per system

[Borislav Petkov]

On Wed, Jun 20, 2012 at 01:28:57AM +0200, Borislav Petkov wrote:
> But you're right, reordering the registration could simplify the code.
> I'll take a look at it tomorrow.

Actually, IINM, the code doesn't become simpler - just different because
if sysfs_create_group() fails, then you have to do microcode_dev_exit().
Pasting here the whole snippet:

...
        error = microcode_dev_init();
        if (error)
                goto out_driver;

        error = sysfs_create_group(&cpu_subsys.dev_root->kobj,
                                   &cpu_root_microcode_group);

        if (error) {
                pr_err("Error creating microcode group!\n");
                goto out_dev_misc;				<--- we need to undo what microcode_dev_init() did here...
        }

        register_syscore_ops(&mc_syscore_ops);
        register_hotcpu_notifier(&mc_cpu_notifier);

        pr_info("Microcode Update Driver: v" MICROCODE_VERSION
                " <tigran@aivazian.fsnet.co.uk>, Peter Oruba\n");

        return 0;

 out_dev_misc:
        microcode_dev_exit();					<--- and we do it here

 out_driver:
...

Now, if we get to remove the crappy misc dev interface, then we can
simplify the whole thing. But for that I need a statement from Intel
people how they want to load ucode and whether they want to split the
firmware blob or keep it together and load the whole 1.2MB at once.

So I'll keep the error unwind as it was in the original patch.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551