[PATCH] x86, microcode, Fix long microcode load time when firmware file is missing [v2]

[PATCH] x86, microcode, Fix long microcode load time when firmware file is missing [v2]

[Prarit Bhargava]

If no firmware is found on the system that matches the processor, the
microcode module can take hours to load.  For example on recent kernels
when loading the microcode module on an Intel system:

[  239.532116] microcode: CPU0 sig=0x306e4, pf=0x1, revision=0x413
[  299.693447] microcode: CPU1 sig=0x306e4, pf=0x1, revision=0x413
[  359.821972] microcode: CPU2 sig=0x306e4, pf=0x1, revision=0x413
[  419.960263] microcode: CPU3 sig=0x306e4, pf=0x1, revision=0x413
[  480.090024] microcode: CPU4 sig=0x306e4, pf=0x1, revision=0x413
...
[ 2825.151364] microcode: CPU43 sig=0x306e4, pf=0x1, revision=0x413
[ 2885.280863] microcode: CPU44 sig=0x306e4, pf=0x1, revision=0x413
[ 2945.410719] microcode: CPU45 sig=0x306e4, pf=0x1, revision=0x413
[ 3005.540541] microcode: CPU46 sig=0x306e4, pf=0x1, revision=0x413
[ 3065.670405] microcode: CPU47 sig=0x306e4, pf=0x1, revision=0x413
...
etc.

Similarly there is a 60 second "hang" when loading the AMD module, which
isn't as bad as the Intel situation.  This is because the AMD microcode
loader only attempts to look for the firmware on the boot_cpu and, if
found, loads the microcode on each cpu.  This patch does the same for the
Intel microcode, and it obviously peeds up the module load if there is no
microcode update available.

After making this change, the old microcode loading code and the new
loading code can be cleaned up and unified in the core code.

This patch was tested on several systems (both AMD and Intel) with the
microcode in place and without, as well as on several different OSes.

Signed-off-by: Prarit Bhargava <prarit@redhat.com>
Cc: tigran@aivazian.fsnet.co.uk
Cc: x86@kernel.org
Cc: hmh@hmh.eng.br
Cc: andi@firstfloor.org

v2: Update for Andi's and Henrique's comments.

---
 arch/x86/kernel/microcode_core.c  | 142 +++++++++++++++++---------------------
 arch/x86/kernel/microcode_intel.c |  33 +++++++--
 2 files changed, 91 insertions(+), 84 deletions(-)

diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
index 15c9876..608b1bd 100644
--- a/arch/x86/kernel/microcode_core.c
+++ b/arch/x86/kernel/microcode_core.c
@@ -180,30 +180,57 @@ static int apply_microcode_on_target(int cpu)
 	return ret;
 }
 
-#ifdef CONFIG_MICROCODE_OLD_INTERFACE
-static int do_microcode_update(const void __user *buf, size_t size)
+/* fake device for request_firmware */
+static struct platform_device	*microcode_pdev;
+
+static int do_microcode_update(bool user, const void __user *buf, size_t size)
 {
-	int error = 0;
-	int cpu;
+	enum ucode_state ustate;
+	int boot_cpu = boot_cpu_data.cpu_index;
+	int cpu, ret;
+
+	get_online_cpus();
+	mutex_lock(&microcode_mutex);
+
+	if (user)
+		ustate = microcode_ops->request_microcode_user(boot_cpu,
+							       buf, size);
+	else
+		ustate = microcode_ops->request_microcode_fw(boot_cpu,
+							   &microcode_pdev->dev,
+							     true);
+	if (ustate == UCODE_ERROR) {
+		pr_warn("Error firmware request failed\n");
+		ret = -EINVAL;
+		goto out;
+	}
 
 	for_each_online_cpu(cpu) {
-		struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
-		enum ucode_state ustate;
-
-		if (!uci->valid)
-			continue;
-
-		ustate = microcode_ops->request_microcode_user(cpu, buf, size);
-		if (ustate == UCODE_ERROR) {
-			error = -1;
-			break;
-		} else if (ustate == UCODE_OK)
-			apply_microcode_on_target(cpu);
+		ret = collect_cpu_info(cpu);
+		if (ret) {
+			pr_warn("Error collecting info on CPU %d\n", cpu);
+			ret = -ENODEV;
+			goto out;
+		}
+
+		ret = apply_microcode_on_target(cpu);
+		if (ret) {
+			pr_warn("Error reloading microcode on CPU %d\n", cpu);
+			ret = -EINVAL;
+			goto out;
+		}
 	}
 
-	return error;
+	perf_check_microcode();
+out:
+	mutex_unlock(&microcode_mutex);
+	put_online_cpus();
+
+	return ret;
 }
 
+#ifdef CONFIG_MICROCODE_OLD_INTERFACE
+
 static int microcode_open(struct inode *inode, struct file *file)
 {
 	return capable(CAP_SYS_RAWIO) ? nonseekable_open(inode, file) : -EPERM;
@@ -219,18 +246,10 @@ static ssize_t microcode_write(struct file *file, const char __user *buf,
 		return ret;
 	}
 
-	get_online_cpus();
-	mutex_lock(&microcode_mutex);
-
-	if (do_microcode_update(buf, len) == 0)
+	ret = do_microcode_update(true, buf, len);
+	if (!ret)
 		ret = (ssize_t)len;
 
-	if (ret > 0)
-		perf_check_microcode();
-
-	mutex_unlock(&microcode_mutex);
-	put_online_cpus();
-
 	return ret;
 }
 
@@ -273,34 +292,12 @@ MODULE_ALIAS("devname:cpu/microcode");
 #define microcode_dev_exit()	do { } while (0)
 #endif
 
-/* fake device for request_firmware */
-static struct platform_device	*microcode_pdev;
-
-static int reload_for_cpu(int cpu)
-{
-	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
-	enum ucode_state ustate;
-	int err = 0;
-
-	if (!uci->valid)
-		return err;
-
-	ustate = microcode_ops->request_microcode_fw(cpu, &microcode_pdev->dev, true);
-	if (ustate == UCODE_OK)
-		apply_microcode_on_target(cpu);
-	else
-		if (ustate == UCODE_ERROR)
-			err = -EINVAL;
-	return err;
-}
-
 static ssize_t reload_store(struct device *dev,
 			    struct device_attribute *attr,
 			    const char *buf, size_t size)
 {
 	unsigned long val;
-	int cpu;
-	ssize_t ret = 0, tmp_ret;
+	ssize_t ret;
 
 	ret = kstrtoul(buf, 0, &val);
 	if (ret)
@@ -309,22 +306,7 @@ static ssize_t reload_store(struct device *dev,
 	if (val != 1)
 		return size;
 
-	get_online_cpus();
-	mutex_lock(&microcode_mutex);
-	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;
-	}
-	if (!ret)
-		perf_check_microcode();
-	mutex_unlock(&microcode_mutex);
-	put_online_cpus();
-
+	ret = do_microcode_update(false, NULL, 0);
 	if (!ret)
 		ret = size;
 
@@ -380,26 +362,32 @@ static enum ucode_state microcode_resume_cpu(int cpu)
 static enum ucode_state microcode_init_cpu(int cpu, bool refresh_fw)
 {
 	enum ucode_state ustate;
-	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
-
-	if (uci && uci->valid)
-		return UCODE_OK;
-
-	if (collect_cpu_info(cpu))
-		return UCODE_ERROR;
+	int ret;
 
 	/* --dimm. Trigger a delayed update? */
 	if (system_state != SYSTEM_RUNNING)
 		return UCODE_NFOUND;
 
+	ret = collect_cpu_info(cpu);
+	if (ret) {
+		pr_warn("Error collecting info on CPU %d\n", cpu);
+		return UCODE_ERROR;
+	}
+
 	ustate = microcode_ops->request_microcode_fw(cpu, &microcode_pdev->dev,
 						     refresh_fw);
-
-	if (ustate == UCODE_OK) {
-		pr_debug("CPU%d updated upon init\n", cpu);
-		apply_microcode_on_target(cpu);
+	if (ustate == UCODE_NFOUND) {
+		pr_info("Processor microcode update not found\n");
+		return ustate;
 	}
 
+	if (ustate != UCODE_OK)
+		pr_warn("Error microcode request failed on CPU %d\n", cpu);
+
+	ret = apply_microcode_on_target(cpu);
+	if (ret)
+		pr_warn("Error applying microcode on CPU %d\n", cpu);
+
 	return ustate;
 }
 
diff --git a/arch/x86/kernel/microcode_intel.c b/arch/x86/kernel/microcode_intel.c
index 5fb2ceb..f39b677 100644
--- a/arch/x86/kernel/microcode_intel.c
+++ b/arch/x86/kernel/microcode_intel.c
@@ -87,9 +87,12 @@ MODULE_DESCRIPTION("Microcode Update Driver");
 MODULE_AUTHOR("Tigran Aivazian <tigran@aivazian.fsnet.co.uk>");
 MODULE_LICENSE("GPL");
 
+static void *master_mc; /* global copy of the microcode */
+
 static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
 {
 	struct cpuinfo_x86 *c = &cpu_data(cpu_num);
+	struct ucode_cpu_info *uci = ucode_cpu_info + cpu_num;
 	unsigned int val[2];
 
 	memset(csig, 0, sizeof(*csig));
@@ -102,9 +105,10 @@ static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
 		csig->pf = 1 << ((val[1] >> 18) & 7);
 	}
 
+	if (master_mc)
+		uci->mc = master_mc;
+
 	csig->rev = c->microcode;
-	pr_info("CPU%d sig=0x%x, pf=0x%x, revision=0x%x\n",
-		cpu_num, csig->sig, csig->pf, csig->rev);
 
 	return 0;
 }
@@ -124,6 +128,9 @@ static int get_matching_mc(struct microcode_intel *mc_intel, int cpu)
 	cpf = cpu_sig.pf;
 	crev = cpu_sig.rev;
 
+	pr_info("CPU%d sig=0x%x, pf=0x%x, revision=0x%x\n",
+		cpu, csig, cpf, crev);
+
 	return get_matching_microcode(csig, cpf, mc_intel, crev);
 }
 
@@ -136,12 +143,12 @@ int apply_microcode(int cpu)
 	struct cpuinfo_x86 *c = &cpu_data(cpu_num);
 
 	uci = ucode_cpu_info + cpu;
-	mc_intel = uci->mc;
+	mc_intel = master_mc;
 
 	/* We should bind the task to the CPU */
 	BUG_ON(cpu_num != cpu);
 
-	if (mc_intel == NULL)
+	if ((mc_intel == NULL) || (!uci->valid))
 		return 0;
 
 	/*
@@ -246,7 +253,8 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
 	}
 
 	vfree(uci->mc);
-	uci->mc = (struct microcode_intel *)new_mc;
+	master_mc = (struct microcode_intel *)new_mc;
+	uci->mc = master_mc;
 
 	/*
 	 * If early loading microcode is supported, save this mc into
@@ -275,11 +283,14 @@ static enum ucode_state request_microcode_fw(int cpu, struct device *device,
 	const struct firmware *firmware;
 	enum ucode_state ret;
 
+	if (!refresh_fw || (c->cpu_index != boot_cpu_data.cpu_index))
+		return UCODE_OK;
+
 	sprintf(name, "intel-ucode/%02x-%02x-%02x",
 		c->x86, c->x86_model, c->x86_mask);
 
 	if (request_firmware(&firmware, name, device)) {
-		pr_debug("data file %s load failed\n", name);
+		pr_warn("data file %s load failed\n", name);
 		return UCODE_NFOUND;
 	}
 
@@ -299,15 +310,23 @@ static int get_ucode_user(void *to, const void *from, size_t n)
 static enum ucode_state
 request_microcode_user(int cpu, const void __user *buf, size_t size)
 {
-	return generic_load_microcode(cpu, (void *)buf, size, &get_ucode_user);
+	int boot_cpu = boot_cpu_data.cpu_index;
+
+	return generic_load_microcode(boot_cpu, (void *)buf, size,
+				      &get_ucode_user);
 }
 
 static void microcode_fini_cpu(int cpu)
 {
+	struct cpuinfo_x86 *c = &cpu_data(cpu);
 	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
 
+	if (c->cpu_index != boot_cpu_data.cpu_index)
+		return;
+
 	vfree(uci->mc);
 	uci->mc = NULL;
+	master_mc = NULL;
 }
 
 static struct microcode_ops microcode_intel_ops = {
-- 
1.8.3.1

Re: [PATCH] x86, microcode, Fix long microcode load time when firmware file is missing [v2]

[Borislav Petkov]

On Mon, Oct 28, 2013 at 08:06:08AM -0400, Prarit Bhargava wrote:
> If no firmware is found on the system that matches the processor, the
> microcode module can take hours to load.  For example on recent kernels
> when loading the microcode module on an Intel system:
> 
> [  239.532116] microcode: CPU0 sig=0x306e4, pf=0x1, revision=0x413
> [  299.693447] microcode: CPU1 sig=0x306e4, pf=0x1, revision=0x413
> [  359.821972] microcode: CPU2 sig=0x306e4, pf=0x1, revision=0x413
> [  419.960263] microcode: CPU3 sig=0x306e4, pf=0x1, revision=0x413
> [  480.090024] microcode: CPU4 sig=0x306e4, pf=0x1, revision=0x413
> ...
> [ 2825.151364] microcode: CPU43 sig=0x306e4, pf=0x1, revision=0x413
> [ 2885.280863] microcode: CPU44 sig=0x306e4, pf=0x1, revision=0x413
> [ 2945.410719] microcode: CPU45 sig=0x306e4, pf=0x1, revision=0x413
> [ 3005.540541] microcode: CPU46 sig=0x306e4, pf=0x1, revision=0x413
> [ 3065.670405] microcode: CPU47 sig=0x306e4, pf=0x1, revision=0x413
> ...
> etc.
> 
> Similarly there is a 60 second "hang" when loading the AMD module, which
> isn't as bad as the Intel situation.  This is because the AMD microcode
> loader only attempts to look for the firmware on the boot_cpu and, if
> found, loads the microcode on each cpu.  This patch does the same for the
> Intel microcode, and it obviously peeds up the module load if there is no
> microcode update available.
> 
> After making this change, the old microcode loading code and the new
> loading code can be cleaned up and unified in the core code.
> 
> This patch was tested on several systems (both AMD and Intel) with the
> microcode in place and without, as well as on several different OSes.
> 
> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
> Cc: tigran@aivazian.fsnet.co.uk
> Cc: x86@kernel.org
> Cc: hmh@hmh.eng.br
> Cc: andi@firstfloor.org
> 
> v2: Update for Andi's and Henrique's comments.

This patch is adding a bunch of new printk's on the error path while
the vendor code already complains when application fails or the error
message is simply N/A.

Let me show you:


> ---
>  arch/x86/kernel/microcode_core.c  | 142 +++++++++++++++++---------------------
>  arch/x86/kernel/microcode_intel.c |  33 +++++++--
>  2 files changed, 91 insertions(+), 84 deletions(-)
> 
> diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
> index 15c9876..608b1bd 100644
> --- a/arch/x86/kernel/microcode_core.c
> +++ b/arch/x86/kernel/microcode_core.c
> @@ -180,30 +180,57 @@ static int apply_microcode_on_target(int cpu)
>  	return ret;
>  }
>  
> -#ifdef CONFIG_MICROCODE_OLD_INTERFACE
> -static int do_microcode_update(const void __user *buf, size_t size)
> +/* fake device for request_firmware */
> +static struct platform_device	*microcode_pdev;
> +
> +static int do_microcode_update(bool user, const void __user *buf, size_t size)
>  {
> -	int error = 0;
> -	int cpu;
> +	enum ucode_state ustate;
> +	int boot_cpu = boot_cpu_data.cpu_index;
> +	int cpu, ret;
> +
> +	get_online_cpus();
> +	mutex_lock(&microcode_mutex);
> +
> +	if (user)
> +		ustate = microcode_ops->request_microcode_user(boot_cpu,
> +							       buf, size);
> +	else
> +		ustate = microcode_ops->request_microcode_fw(boot_cpu,
> +							   &microcode_pdev->dev,
> +							     true);
> +	if (ustate == UCODE_ERROR) {
> +		pr_warn("Error firmware request failed\n");
> +		ret = -EINVAL;
> +		goto out;
> +	}
>  
>  	for_each_online_cpu(cpu) {
> -		struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
> -		enum ucode_state ustate;
> -
> -		if (!uci->valid)
> -			continue;
> -
> -		ustate = microcode_ops->request_microcode_user(cpu, buf, size);
> -		if (ustate == UCODE_ERROR) {
> -			error = -1;
> -			break;
> -		} else if (ustate == UCODE_OK)
> -			apply_microcode_on_target(cpu);
> +		ret = collect_cpu_info(cpu);

This one always succeeds. No need for checking retval

> +		if (ret) {
> +			pr_warn("Error collecting info on CPU %d\n", cpu);
> +			ret = -ENODEV;
> +			goto out;
> +		}
> +
> +		ret = apply_microcode_on_target(cpu);

Vendor drivers already scream.

> +		if (ret) {
> +			pr_warn("Error reloading microcode on CPU %d\n", cpu);
> +			ret = -EINVAL;
> +			goto out;
> +		}
>  	}
>  
> -	return error;
> +	perf_check_microcode();
> +out:
> +	mutex_unlock(&microcode_mutex);
> +	put_online_cpus();
> +
> +	return ret;
>  }
>  
> +#ifdef CONFIG_MICROCODE_OLD_INTERFACE
> +
>  static int microcode_open(struct inode *inode, struct file *file)
>  {
>  	return capable(CAP_SYS_RAWIO) ? nonseekable_open(inode, file) : -EPERM;
> @@ -219,18 +246,10 @@ static ssize_t microcode_write(struct file *file, const char __user *buf,
>  		return ret;
>  	}
>  
> -	get_online_cpus();
> -	mutex_lock(&microcode_mutex);
> -
> -	if (do_microcode_update(buf, len) == 0)
> +	ret = do_microcode_update(true, buf, len);
> +	if (!ret)
>  		ret = (ssize_t)len;
>  
> -	if (ret > 0)
> -		perf_check_microcode();
> -
> -	mutex_unlock(&microcode_mutex);
> -	put_online_cpus();
> -
>  	return ret;
>  }
>  
> @@ -273,34 +292,12 @@ MODULE_ALIAS("devname:cpu/microcode");
>  #define microcode_dev_exit()	do { } while (0)
>  #endif
>  
> -/* fake device for request_firmware */
> -static struct platform_device	*microcode_pdev;
> -
> -static int reload_for_cpu(int cpu)
> -{
> -	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
> -	enum ucode_state ustate;
> -	int err = 0;
> -
> -	if (!uci->valid)
> -		return err;
> -
> -	ustate = microcode_ops->request_microcode_fw(cpu, &microcode_pdev->dev, true);
> -	if (ustate == UCODE_OK)
> -		apply_microcode_on_target(cpu);
> -	else
> -		if (ustate == UCODE_ERROR)
> -			err = -EINVAL;
> -	return err;
> -}
> -
>  static ssize_t reload_store(struct device *dev,
>  			    struct device_attribute *attr,
>  			    const char *buf, size_t size)
>  {
>  	unsigned long val;
> -	int cpu;
> -	ssize_t ret = 0, tmp_ret;
> +	ssize_t ret;
>  
>  	ret = kstrtoul(buf, 0, &val);
>  	if (ret)
> @@ -309,22 +306,7 @@ static ssize_t reload_store(struct device *dev,
>  	if (val != 1)
>  		return size;
>  
> -	get_online_cpus();
> -	mutex_lock(&microcode_mutex);
> -	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;
> -	}
> -	if (!ret)
> -		perf_check_microcode();
> -	mutex_unlock(&microcode_mutex);
> -	put_online_cpus();
> -
> +	ret = do_microcode_update(false, NULL, 0);
>  	if (!ret)
>  		ret = size;
>  
> @@ -380,26 +362,32 @@ static enum ucode_state microcode_resume_cpu(int cpu)
>  static enum ucode_state microcode_init_cpu(int cpu, bool refresh_fw)
>  {
>  	enum ucode_state ustate;
> -	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
> -
> -	if (uci && uci->valid)
> -		return UCODE_OK;
> -
> -	if (collect_cpu_info(cpu))
> -		return UCODE_ERROR;
> +	int ret;
>  
>  	/* --dimm. Trigger a delayed update? */
>  	if (system_state != SYSTEM_RUNNING)
>  		return UCODE_NFOUND;
>  
> +	ret = collect_cpu_info(cpu);

Ditto.

> +	if (ret) {
> +		pr_warn("Error collecting info on CPU %d\n", cpu);
> +		return UCODE_ERROR;
> +	}
> +
>  	ustate = microcode_ops->request_microcode_fw(cpu, &microcode_pdev->dev,
>  						     refresh_fw);
> -
> -	if (ustate == UCODE_OK) {
> -		pr_debug("CPU%d updated upon init\n", cpu);
> -		apply_microcode_on_target(cpu);
> +	if (ustate == UCODE_NFOUND) {
> +		pr_info("Processor microcode update not found\n");

The AMD one already complains, you need to add an error printk to the
Intel driver and drop this one here.

> +		return ustate;
>  	}
>  
> +	if (ustate != UCODE_OK)
> +		pr_warn("Error microcode request failed on CPU %d\n", cpu);
> +
> +	ret = apply_microcode_on_target(cpu);

Ditto.

> +	if (ret)
> +		pr_warn("Error applying microcode on CPU %d\n", cpu);

> +
>  	return ustate;
>  }
>  
> diff --git a/arch/x86/kernel/microcode_intel.c b/arch/x86/kernel/microcode_intel.c
> index 5fb2ceb..f39b677 100644
> --- a/arch/x86/kernel/microcode_intel.c
> +++ b/arch/x86/kernel/microcode_intel.c
> @@ -87,9 +87,12 @@ MODULE_DESCRIPTION("Microcode Update Driver");
>  MODULE_AUTHOR("Tigran Aivazian <tigran@aivazian.fsnet.co.uk>");
>  MODULE_LICENSE("GPL");
>  
> +static void *master_mc; /* global copy of the microcode */

Yeah, maybe we should move the microcode patch cache to microcode_core.c
so that we can save us the trouble here. Oh well, when there's time...

So Prarit, please split this patch into changes which *directly* address
the issue and other cleanups ontop. This will simplify review immensely
as having one single bulky patch is not easy on the eyes.

Then, make sure to audit the lowlevel drivers whether they're already
issuing output on the error path before adding new printks arbitrarily.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] x86, microcode, Fix long microcode load time when firmware file is missing [v2]

[Takashi Iwai]

At Mon, 28 Oct 2013 08:06:08 -0400,
Prarit Bhargava wrote:
> 
> If no firmware is found on the system that matches the processor, the
> microcode module can take hours to load.  For example on recent kernels
> when loading the microcode module on an Intel system:
> 
> [  239.532116] microcode: CPU0 sig=0x306e4, pf=0x1, revision=0x413
> [  299.693447] microcode: CPU1 sig=0x306e4, pf=0x1, revision=0x413
> [  359.821972] microcode: CPU2 sig=0x306e4, pf=0x1, revision=0x413
> [  419.960263] microcode: CPU3 sig=0x306e4, pf=0x1, revision=0x413
> [  480.090024] microcode: CPU4 sig=0x306e4, pf=0x1, revision=0x413
> ...
> [ 2825.151364] microcode: CPU43 sig=0x306e4, pf=0x1, revision=0x413
> [ 2885.280863] microcode: CPU44 sig=0x306e4, pf=0x1, revision=0x413
> [ 2945.410719] microcode: CPU45 sig=0x306e4, pf=0x1, revision=0x413
> [ 3005.540541] microcode: CPU46 sig=0x306e4, pf=0x1, revision=0x413
> [ 3065.670405] microcode: CPU47 sig=0x306e4, pf=0x1, revision=0x413
> ...
> etc.
> 
> Similarly there is a 60 second "hang" when loading the AMD module, which
> isn't as bad as the Intel situation.  This is because the AMD microcode
> loader only attempts to look for the firmware on the boot_cpu and, if
> found, loads the microcode on each cpu.  This patch does the same for the
> Intel microcode, and it obviously peeds up the module load if there is no
> microcode update available.
> 
> After making this change, the old microcode loading code and the new
> loading code can be cleaned up and unified in the core code.
> 
> This patch was tested on several systems (both AMD and Intel) with the
> microcode in place and without, as well as on several different OSes.

Does simply disabling CONFIG_FW_LOADER_USER_HELPER work without your
patch?

If yes, it's an infamous udev issue.  An easier solution would be to
create a function that does only the direct f/w loading without
usermode helper, and call it in the microcode driver.

An untested quick fix patch is attached below.


Takashi

---
diff --git a/arch/x86/kernel/microcode_amd.c b/arch/x86/kernel/microcode_amd.c
index af99f71..539a01a 100644
--- a/arch/x86/kernel/microcode_amd.c
+++ b/arch/x86/kernel/microcode_amd.c
@@ -430,7 +430,7 @@ static enum ucode_state request_microcode_amd(int cpu, struct device *device,
 	if (c->x86 >= 0x15)
 		snprintf(fw_name, sizeof(fw_name), "amd-ucode/microcode_amd_fam%.2xh.bin", c->x86);
 
-	if (request_firmware(&fw, (const char *)fw_name, device)) {
+	if (request_firmware_direct(&fw, (const char *)fw_name, device)) {
 		pr_err("failed to load file %s\n", fw_name);
 		goto out;
 	}
diff --git a/arch/x86/kernel/microcode_intel.c b/arch/x86/kernel/microcode_intel.c
index 5fb2ceb..a276fa7 100644
--- a/arch/x86/kernel/microcode_intel.c
+++ b/arch/x86/kernel/microcode_intel.c
@@ -278,7 +278,7 @@ static enum ucode_state request_microcode_fw(int cpu, struct device *device,
 	sprintf(name, "intel-ucode/%02x-%02x-%02x",
 		c->x86, c->x86_model, c->x86_mask);
 
-	if (request_firmware(&firmware, name, device)) {
+	if (request_firmware_direct(&firmware, name, device)) {
 		pr_debug("data file %s load failed\n", name);
 		return UCODE_NFOUND;
 	}
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 10a4467..f30f381 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -1054,7 +1054,7 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device,
 /* called from request_firmware() and request_firmware_work_func() */
 static int
 _request_firmware(const struct firmware **firmware_p, const char *name,
-		  struct device *device, bool uevent, bool nowait)
+		  struct device *device, bool uevent, bool nowait, bool fallback)
 {
 	struct firmware *fw;
 	long timeout;
@@ -1086,7 +1086,7 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 		}
 	}
 
-	if (!fw_get_filesystem_firmware(device, fw->priv))
+	if (!fw_get_filesystem_firmware(device, fw->priv) && fallback)
 		ret = fw_load_from_user_helper(fw, name, device,
 					       uevent, nowait, timeout);
 
@@ -1134,12 +1134,25 @@ request_firmware(const struct firmware **firmware_p, const char *name,
 
 	/* Need to pin this module until return */
 	__module_get(THIS_MODULE);
-	ret = _request_firmware(firmware_p, name, device, true, false);
+	ret = _request_firmware(firmware_p, name, device, true, false, true);
 	module_put(THIS_MODULE);
 	return ret;
 }
 EXPORT_SYMBOL(request_firmware);
 
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+int request_firmware_direct(const struct firmware **firmware_p,
+			    const char *name, struct device *device)
+{
+	int ret;
+	__module_get(THIS_MODULE);
+	ret = _request_firmware(firmware_p, name, device, true, false, false);
+	module_put(THIS_MODULE);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(request_firmware_direct);
+#endif
+
 /**
  * release_firmware: - release the resource associated with a firmware image
  * @fw: firmware resource to release
@@ -1173,7 +1186,7 @@ static void request_firmware_work_func(struct work_struct *work)
 	fw_work = container_of(work, struct firmware_work, work);
 
 	_request_firmware(&fw, fw_work->name, fw_work->device,
-			  fw_work->uevent, true);
+			  fw_work->uevent, true, true);
 	fw_work->cont(fw, fw_work->context);
 	put_device(fw_work->device); /* taken in request_firmware_nowait() */
 
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index e154c10..5952933 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -68,4 +68,11 @@ static inline void release_firmware(const struct firmware *fw)
 
 #endif
 
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+int request_firmware_direct(const struct firmware **fw, const char *name,
+			    struct device *device);
+#else
+#define request_firmware_direct	request_firmware
+#endif
+
 #endif

Re: [PATCH] x86, microcode, Fix long microcode load time when firmware file is missing [v2]

[Henrique de Moraes Holschuh]

On Mon, 28 Oct 2013, Borislav Petkov wrote:
> So Prarit, please split this patch into changes which *directly* address
> the issue and other cleanups ontop. This will simplify review immensely
> as having one single bulky patch is not easy on the eyes.
> 
> Then, make sure to audit the lowlevel drivers whether they're already
> issuing output on the error path before adding new printks arbitrarily.

Something else I couldn't check just from the description (and I apologise,
but I did not look at your patch closely enough to check how you implemented
the functionality on Intel): in the general case, it is NOT acceptable to
bail out if you cannot find the firmware for the first processor.
Mixed-stepping systems do exist, and you might need to update the microcode
of, e.g, just the third processor.

AMD can get away with a half-done implementation of negative caching (or an
"optimised one" depending on your PoV :) ) because they have per-family
firmware files, so even mixed-stepping systems will require only the same
file.  This is *not* true for Intel, which is really annoying.

-- 
  "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] x86, microcode, Fix long microcode load time when firmware file is missing [v2]

[Prarit Bhargava]

On 10/28/2013 11:06 AM, Henrique de Moraes Holschuh wrote:
> On Mon, 28 Oct 2013, Borislav Petkov wrote:
>> So Prarit, please split this patch into changes which *directly* address
>> the issue and other cleanups ontop. This will simplify review immensely
>> as having one single bulky patch is not easy on the eyes.
>>
>> Then, make sure to audit the lowlevel drivers whether they're already
>> issuing output on the error path before adding new printks arbitrarily.
> 
> Something else I couldn't check just from the description (and I apologise,
> but I did not look at your patch closely enough to check how you implemented
> the functionality on Intel): in the general case, it is NOT acceptable to
> bail out if you cannot find the firmware for the first processor.
> Mixed-stepping systems do exist, and you might need to update the microcode
> of, e.g, just the third processor.
> 
> AMD can get away with a half-done implementation of negative caching (or an
> "optimised one" depending on your PoV :) ) because they have per-family
> firmware files, so even mixed-stepping systems will require only the same
> file.  This is *not* true for Intel, which is really annoying.

Is that right? :(  I took Andi's comment to imply otherwise ...  If that's the
case then, yeah, back to the drawing board with this.

Andi (or anyone from Intel) -- care to offer a comment?

P.
> 

Re: [PATCH] x86, microcode, Fix long microcode load time when firmware file is missing [v2]

[Prarit Bhargava]

On 10/28/2013 11:05 AM, Takashi Iwai wrote:
> At Mon, 28 Oct 2013 08:06:08 -0400,
> Prarit Bhargava wrote:
>>
>> If no firmware is found on the system that matches the processor, the
>> microcode module can take hours to load.  For example on recent kernels
>> when loading the microcode module on an Intel system:
>>
>> [  239.532116] microcode: CPU0 sig=0x306e4, pf=0x1, revision=0x413
>> [  299.693447] microcode: CPU1 sig=0x306e4, pf=0x1, revision=0x413
>> [  359.821972] microcode: CPU2 sig=0x306e4, pf=0x1, revision=0x413
>> [  419.960263] microcode: CPU3 sig=0x306e4, pf=0x1, revision=0x413
>> [  480.090024] microcode: CPU4 sig=0x306e4, pf=0x1, revision=0x413
>> ...
>> [ 2825.151364] microcode: CPU43 sig=0x306e4, pf=0x1, revision=0x413
>> [ 2885.280863] microcode: CPU44 sig=0x306e4, pf=0x1, revision=0x413
>> [ 2945.410719] microcode: CPU45 sig=0x306e4, pf=0x1, revision=0x413
>> [ 3005.540541] microcode: CPU46 sig=0x306e4, pf=0x1, revision=0x413
>> [ 3065.670405] microcode: CPU47 sig=0x306e4, pf=0x1, revision=0x413
>> ...
>> etc.
>>
>> Similarly there is a 60 second "hang" when loading the AMD module, which
>> isn't as bad as the Intel situation.  This is because the AMD microcode
>> loader only attempts to look for the firmware on the boot_cpu and, if
>> found, loads the microcode on each cpu.  This patch does the same for the
>> Intel microcode, and it obviously peeds up the module load if there is no
>> microcode update available.
>>
>> After making this change, the old microcode loading code and the new
>> loading code can be cleaned up and unified in the core code.
>>
>> This patch was tested on several systems (both AMD and Intel) with the
>> microcode in place and without, as well as on several different OSes.
> 
> Does simply disabling CONFIG_FW_LOADER_USER_HELPER work without your
> patch?
> 
> If yes, it's an infamous udev issue.  An easier solution would be to
> create a function that does only the direct f/w loading without
> usermode helper, and call it in the microcode driver.

Thanks Takashi, I thought about doing something similar but didn't know if
!waiting was a good idea here.  I'll give this a shot.

P.

> 
> An untested quick fix patch is attached below.
> 
> 
> Takashi
> 
> ---
> diff --git a/arch/x86/kernel/microcode_amd.c b/arch/x86/kernel/microcode_amd.c
> index af99f71..539a01a 100644
> --- a/arch/x86/kernel/microcode_amd.c
> +++ b/arch/x86/kernel/microcode_amd.c
> @@ -430,7 +430,7 @@ static enum ucode_state request_microcode_amd(int cpu, struct device *device,
>  	if (c->x86 >= 0x15)
>  		snprintf(fw_name, sizeof(fw_name), "amd-ucode/microcode_amd_fam%.2xh.bin", c->x86);
>  
> -	if (request_firmware(&fw, (const char *)fw_name, device)) {
> +	if (request_firmware_direct(&fw, (const char *)fw_name, device)) {
>  		pr_err("failed to load file %s\n", fw_name);
>  		goto out;
>  	}
> diff --git a/arch/x86/kernel/microcode_intel.c b/arch/x86/kernel/microcode_intel.c
> index 5fb2ceb..a276fa7 100644
> --- a/arch/x86/kernel/microcode_intel.c
> +++ b/arch/x86/kernel/microcode_intel.c
> @@ -278,7 +278,7 @@ static enum ucode_state request_microcode_fw(int cpu, struct device *device,
>  	sprintf(name, "intel-ucode/%02x-%02x-%02x",
>  		c->x86, c->x86_model, c->x86_mask);
>  
> -	if (request_firmware(&firmware, name, device)) {
> +	if (request_firmware_direct(&firmware, name, device)) {
>  		pr_debug("data file %s load failed\n", name);
>  		return UCODE_NFOUND;
>  	}
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 10a4467..f30f381 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -1054,7 +1054,7 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device,
>  /* called from request_firmware() and request_firmware_work_func() */
>  static int
>  _request_firmware(const struct firmware **firmware_p, const char *name,
> -		  struct device *device, bool uevent, bool nowait)
> +		  struct device *device, bool uevent, bool nowait, bool fallback)
>  {
>  	struct firmware *fw;
>  	long timeout;
> @@ -1086,7 +1086,7 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>  		}
>  	}
>  
> -	if (!fw_get_filesystem_firmware(device, fw->priv))
> +	if (!fw_get_filesystem_firmware(device, fw->priv) && fallback)
>  		ret = fw_load_from_user_helper(fw, name, device,
>  					       uevent, nowait, timeout);
>  
> @@ -1134,12 +1134,25 @@ request_firmware(const struct firmware **firmware_p, const char *name,
>  
>  	/* Need to pin this module until return */
>  	__module_get(THIS_MODULE);
> -	ret = _request_firmware(firmware_p, name, device, true, false);
> +	ret = _request_firmware(firmware_p, name, device, true, false, true);
>  	module_put(THIS_MODULE);
>  	return ret;
>  }
>  EXPORT_SYMBOL(request_firmware);
>  
> +#ifdef CONFIG_FW_LOADER_USER_HELPER
> +int request_firmware_direct(const struct firmware **firmware_p,
> +			    const char *name, struct device *device)
> +{
> +	int ret;
> +	__module_get(THIS_MODULE);
> +	ret = _request_firmware(firmware_p, name, device, true, false, false);
> +	module_put(THIS_MODULE);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(request_firmware_direct);
> +#endif
> +
>  /**
>   * release_firmware: - release the resource associated with a firmware image
>   * @fw: firmware resource to release
> @@ -1173,7 +1186,7 @@ static void request_firmware_work_func(struct work_struct *work)
>  	fw_work = container_of(work, struct firmware_work, work);
>  
>  	_request_firmware(&fw, fw_work->name, fw_work->device,
> -			  fw_work->uevent, true);
> +			  fw_work->uevent, true, true);
>  	fw_work->cont(fw, fw_work->context);
>  	put_device(fw_work->device); /* taken in request_firmware_nowait() */
>  
> diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> index e154c10..5952933 100644
> --- a/include/linux/firmware.h
> +++ b/include/linux/firmware.h
> @@ -68,4 +68,11 @@ static inline void release_firmware(const struct firmware *fw)
>  
>  #endif
>  
> +#ifdef CONFIG_FW_LOADER_USER_HELPER
> +int request_firmware_direct(const struct firmware **fw, const char *name,
> +			    struct device *device);
> +#else
> +#define request_firmware_direct	request_firmware
> +#endif
> +
>  #endif

Re: [PATCH] x86, microcode, Fix long microcode load time when firmware file is missing [v2]

[Borislav Petkov]

On Mon, Oct 28, 2013 at 01:06:56PM -0200, Henrique de Moraes Holschuh wrote:
> AMD can get away with a half-done implementation of negative caching
> (or an "optimised one" depending on your PoV :) ) because they have
> per-family firmware files, so even mixed-stepping systems will require
> only the same file. This is *not* true for Intel, which is really
> annoying.

Not per family but per-a-couple-of-families and starting with F15h we
did a separate container because of some obscure reason we had to dance
around at the time.

But per-family is enough because I hardly doubt any x86 vendor
would do mixed-setups of CPUs from different families. It is called
mixed-steppings for a reason: you can mix the same family and model CPUs
but with different steppings. And I think AMD had a K8 system which
sported that.

Mixing different families (and models for that matter) would be pretty
PITA and you're better off simply buying a bunch of identical CPUs :-)

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] x86, microcode, Fix long microcode load time when firmware file is missing [v2]

[Prarit Bhargava]

On 10/28/2013 11:05 AM, Takashi Iwai wrote:
> At Mon, 28 Oct 2013 08:06:08 -0400,
> Prarit Bhargava wrote:
>>
>> If no firmware is found on the system that matches the processor, the
>> microcode module can take hours to load.  For example on recent kernels
>> when loading the microcode module on an Intel system:
>>
>> [  239.532116] microcode: CPU0 sig=0x306e4, pf=0x1, revision=0x413
>> [  299.693447] microcode: CPU1 sig=0x306e4, pf=0x1, revision=0x413
>> [  359.821972] microcode: CPU2 sig=0x306e4, pf=0x1, revision=0x413
>> [  419.960263] microcode: CPU3 sig=0x306e4, pf=0x1, revision=0x413
>> [  480.090024] microcode: CPU4 sig=0x306e4, pf=0x1, revision=0x413
>> ...
>> [ 2825.151364] microcode: CPU43 sig=0x306e4, pf=0x1, revision=0x413
>> [ 2885.280863] microcode: CPU44 sig=0x306e4, pf=0x1, revision=0x413
>> [ 2945.410719] microcode: CPU45 sig=0x306e4, pf=0x1, revision=0x413
>> [ 3005.540541] microcode: CPU46 sig=0x306e4, pf=0x1, revision=0x413
>> [ 3065.670405] microcode: CPU47 sig=0x306e4, pf=0x1, revision=0x413
>> ...
>> etc.
>>
>> Similarly there is a 60 second "hang" when loading the AMD module, which
>> isn't as bad as the Intel situation.  This is because the AMD microcode
>> loader only attempts to look for the firmware on the boot_cpu and, if
>> found, loads the microcode on each cpu.  This patch does the same for the
>> Intel microcode, and it obviously peeds up the module load if there is no
>> microcode update available.
>>
>> After making this change, the old microcode loading code and the new
>> loading code can be cleaned up and unified in the core code.
>>
>> This patch was tested on several systems (both AMD and Intel) with the
>> microcode in place and without, as well as on several different OSes.
> 
> Does simply disabling CONFIG_FW_LOADER_USER_HELPER work without your
> patch?
> 
> If yes, it's an infamous udev issue.  An easier solution would be to
> create a function that does only the direct f/w loading without
> usermode helper, and call it in the microcode driver.
> 
> An untested quick fix patch is attached below.
> 
> 

Takashi, this patch works AFAICT.

I wish I had thought of doing it this way to begin with ...

If you choose to submit, can you add a Tested-by: Prarit Bhargava
<prarit@redhat.com>

Thanks,

P.