[PATCH] x86: Unbreak early processor microcode loading

[PATCH] x86: Unbreak early processor microcode loading

[Daniel J Blueman]

The changes in 871b72dd "x86: microcode: use smp_call_function_single instead
of set_cpus_allowed, cleanup of synchronization logic" introduced a check
that prevents built-in microcode from being loaded before init starts.

Conditionalise it on early microcode loading, so we get the expected behaviour
when early microcode loading is enabled, and when it is not. This has potential
importance as BIOSes often don't load the current microcode.

Signed-off-by: Daniel J Blueman <daniel@numascale.com>
---
 arch/x86/kernel/cpu/microcode/core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 36a8361..fa7f9fc 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -391,9 +391,11 @@ static enum ucode_state microcode_init_cpu(int cpu, bool refresh_fw)
 	if (collect_cpu_info(cpu))
 		return UCODE_ERROR;
 
+#if !defined(CONFIG_MICROCODE_AMD_EARLY) && !defined(CONFIG_MICROCODE_INTEL_EARLY)
 	/* --dimm. Trigger a delayed update? */
 	if (system_state != SYSTEM_RUNNING)
 		return UCODE_NFOUND;
+#endif
 
 	ustate = microcode_ops->request_microcode_fw(cpu, &microcode_pdev->dev,
 						     refresh_fw);
-- 
2.1.0

Re: [PATCH] x86: Unbreak early processor microcode loading

[Borislav Petkov]

On Tue, Mar 03, 2015 at 11:10:44PM +0800, Daniel J Blueman wrote:
> The changes in 871b72dd "x86: microcode: use smp_call_function_single instead
> of set_cpus_allowed, cleanup of synchronization logic" introduced a check
> that prevents built-in microcode from being loaded before init starts.
> 
> Conditionalise it on early microcode loading, so we get the expected behaviour
> when early microcode loading is enabled, and when it is not. This has potential
> importance as BIOSes often don't load the current microcode.

... probably because they don't have it. Which is also the main reason
for the existence of this microcode loader btw :)

> 
> Signed-off-by: Daniel J Blueman <daniel@numascale.com>
> ---
>  arch/x86/kernel/cpu/microcode/core.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
> index 36a8361..fa7f9fc 100644
> --- a/arch/x86/kernel/cpu/microcode/core.c
> +++ b/arch/x86/kernel/cpu/microcode/core.c
> @@ -391,9 +391,11 @@ static enum ucode_state microcode_init_cpu(int cpu, bool refresh_fw)
>  	if (collect_cpu_info(cpu))
>  		return UCODE_ERROR;
>  
> +#if !defined(CONFIG_MICROCODE_AMD_EARLY) && !defined(CONFIG_MICROCODE_INTEL_EARLY)
>  	/* --dimm. Trigger a delayed update? */
>  	if (system_state != SYSTEM_RUNNING)
>  		return UCODE_NFOUND;
> +#endif

Ok, let me try to understand this correctly: where is this microcode
built in, into the kernel?

If yes, you should consider enabling the early loading
method and build in the microcode into the initrd, see
Documentation/x86/early-microcode.txt

This is the preferred method as we're applying the microcode much
earlier.

Back to you.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH] x86: Unbreak early processor microcode loading

[Daniel J Blueman]

On 04/03/2015 00:38, Borislav Petkov wrote:
> On Tue, Mar 03, 2015 at 11:10:44PM +0800, Daniel J Blueman wrote:
>> The changes in 871b72dd "x86: microcode: use smp_call_function_single instead
>> of set_cpus_allowed, cleanup of synchronization logic" introduced a check
>> that prevents built-in microcode from being loaded before init starts.
>>
>> Conditionalise it on early microcode loading, so we get the expected behaviour
>> when early microcode loading is enabled, and when it is not. This has potential
>> importance as BIOSes often don't load the current microcode.
>
> ... probably because they don't have it. Which is also the main reason
> for the existence of this microcode loader btw :)
>
>>
>> Signed-off-by: Daniel J Blueman <daniel@numascale.com>
>> ---
>>   arch/x86/kernel/cpu/microcode/core.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
>> index 36a8361..fa7f9fc 100644
>> --- a/arch/x86/kernel/cpu/microcode/core.c
>> +++ b/arch/x86/kernel/cpu/microcode/core.c
>> @@ -391,9 +391,11 @@ static enum ucode_state microcode_init_cpu(int cpu, bool refresh_fw)
>>   	if (collect_cpu_info(cpu))
>>   		return UCODE_ERROR;
>>
>> +#if !defined(CONFIG_MICROCODE_AMD_EARLY) && !defined(CONFIG_MICROCODE_INTEL_EARLY)
>>   	/* --dimm. Trigger a delayed update? */
>>   	if (system_state != SYSTEM_RUNNING)
>>   		return UCODE_NFOUND;
>> +#endif
>
> Ok, let me try to understand this correctly: where is this microcode
> built in, into the kernel?
>
> If yes, you should consider enabling the early loading
> method and build in the microcode into the initrd, see
> Documentation/x86/early-microcode.txt
>
> This is the preferred method as we're applying the microcode much
> earlier.
>
> Back to you.

Yes, it's built into the kernel with config:

CONFIG_FIRMWARE_IN_KERNEL=y
CONFIG_EXTRA_FIRMWARE="amd-ucode/microcode_amd.bin 
amd-ucode/microcode_amd_fam15h.bin"
CONFIG_EXTRA_FIRMWARE_DIR="../firmware"

That's as some customer and in-house environments we use are 
initramfs-less and some we don't have direct control over the initramfs.

I don't see why built-in microcode loading shouldn't work, so I guess 
the question is, why was that 'system_state .. RUNNING' check introduced?

If just a cleanup and loading built-in microcode early was overlooked, 
it may be reasonable to conditionalise the check like so.

Thanks,
   Daniel

Re: [PATCH] x86: Unbreak early processor microcode loading

[Borislav Petkov]

On Wed, Mar 04, 2015 at 04:27:12PM +0800, Daniel J Blueman wrote:
> Yes, it's built into the kernel with config:
> 
> CONFIG_FIRMWARE_IN_KERNEL=y
> CONFIG_EXTRA_FIRMWARE="amd-ucode/microcode_amd.bin
> amd-ucode/microcode_amd_fam15h.bin"
> CONFIG_EXTRA_FIRMWARE_DIR="../firmware"
> 
> That's as some customer and in-house environments we use are initramfs-less
> and some we don't have direct control over the initramfs.
> 
> I don't see why built-in microcode loading shouldn't work,

Well, it doesn't matter really. If you need to update microcode, either
with the early loader or with a built-in microcode, you'd still have to
regenerate the initrd *and* the kernel. Let me try to reproduce that
here.

Btw, you could also use the late-update method without rebooting the
machine by putting the microcode in

/lib/firmware/amd-ucode/

and doing

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

as root.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH] x86: Unbreak early processor microcode loading

[Borislav Petkov]

On Wed, Mar 04, 2015 at 10:18:47AM +0100, Borislav Petkov wrote:
> Let me try to reproduce that here.

Well, it works fine on a single-socket box here:

[    1.045426] microcode: updated early to new patch_level=0x010000dc
[    1.060957] microcode: CPU0: patch_level=0x010000dc
[    1.061143] microcode: CPU1: patch_level=0x010000dc
[    1.061310] microcode: CPU2: patch_level=0x010000dc
[    1.061495] microcode: CPU3: patch_level=0x010000dc
[    1.061677] microcode: CPU4: patch_level=0x010000dc
[    1.061863] microcode: CPU5: patch_level=0x010000dc

That's early patching.

$ grep -E "(FIRMWARE|MICROCODE)" .config
CONFIG_MICROCODE=y
CONFIG_MICROCODE_INTEL=y
CONFIG_MICROCODE_AMD=y
CONFIG_MICROCODE_OLD_INTERFACE=y
CONFIG_MICROCODE_INTEL_EARLY=y
CONFIG_MICROCODE_AMD_EARLY=y
CONFIG_MICROCODE_EARLY=y
CONFIG_PREVENT_FIRMWARE_BUILD=y
CONFIG_FIRMWARE_IN_KERNEL=y
CONFIG_EXTRA_FIRMWARE="microcode_amd.bin microcode_amd_fam15h.bin microcode_amd_fam16h.bin"
CONFIG_EXTRA_FIRMWARE_DIR="/lib/firmware/amd-ucode"
# CONFIG_DRM_LOAD_EDID_FIRMWARE is not set
CONFIG_FIRMWARE_EDID=y
CONFIG_FIRMWARE_MEMMAP=y
# CONFIG_GOOGLE_FIRMWARE is not set
# CONFIG_TEST_FIRMWARE is not set

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH] x86: Unbreak early processor microcode loading

[Daniel J Blueman]

On Wed, Mar 4, 2015 at 7:45 PM, Borislav Petkov <bp@alien8.de> wrote:
> On Wed, Mar 04, 2015 at 10:18:47AM +0100, Borislav Petkov wrote:
>>  Let me try to reproduce that here.
> 
> Well, it works fine on a single-socket box here:
> 
> [    1.045426] microcode: updated early to new patch_level=0x010000dc
> [    1.060957] microcode: CPU0: patch_level=0x010000dc
> [    1.061143] microcode: CPU1: patch_level=0x010000dc
> [    1.061310] microcode: CPU2: patch_level=0x010000dc
> [    1.061495] microcode: CPU3: patch_level=0x010000dc
> [    1.061677] microcode: CPU4: patch_level=0x010000dc
> [    1.061863] microcode: CPU5: patch_level=0x010000dc
> 
> That's early patching.
> 
> $ grep -E "(FIRMWARE|MICROCODE)" .config
> CONFIG_MICROCODE=y
> CONFIG_MICROCODE_INTEL=y
> CONFIG_MICROCODE_AMD=y
> CONFIG_MICROCODE_OLD_INTERFACE=y
> CONFIG_MICROCODE_INTEL_EARLY=y
> CONFIG_MICROCODE_AMD_EARLY=y
> CONFIG_MICROCODE_EARLY=y
> CONFIG_PREVENT_FIRMWARE_BUILD=y
> CONFIG_FIRMWARE_IN_KERNEL=y
> CONFIG_EXTRA_FIRMWARE="microcode_amd.bin microcode_amd_fam15h.bin 
> microcode_amd_fam16h.bin"
> CONFIG_EXTRA_FIRMWARE_DIR="/lib/firmware/amd-ucode"
> # CONFIG_DRM_LOAD_EDID_FIRMWARE is not set
> CONFIG_FIRMWARE_EDID=y
> CONFIG_FIRMWARE_MEMMAP=y
> # CONFIG_GOOGLE_FIRMWARE is not set
> # CONFIG_TEST_FIRMWARE is not set

Thanks for checking Boris!

I can't reproduce your early-loading case with this same config; I 
suspect your udev is loading the firmware which is available in the 
initramfs, or it's being loaded later from the rootfs non-early.

In my case, the microcode is in neither, hence we build it into the 
kernel image.

The full console log, config and bzImage are at 
https://resources.numascale.com/telemetry/

I find the changes I posted earlier fix this. Maybe I'm missing 
something still?

Daniel

Re: [PATCH] x86: Unbreak early processor microcode loading

[Borislav Petkov]

On Wed, Mar 18, 2015 at 05:09:12PM +0800, Daniel J Blueman wrote:
> I can't reproduce your early-loading case with this same config; I suspect
> your udev is loading the firmware which is available in the initramfs, or
> it's being loaded later from the rootfs non-early.

Yeah, you're right. I was able to reproduce it after disabling the
postinst script which adds the microcode to the initrd.

> I find the changes I posted earlier fix this. Maybe I'm missing something
> still?

I don't like the ifdeffery in your solution and would like to try to fix
it in a cleaner way. Unless you come up with a better solution first.

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH] x86: Unbreak early processor microcode loading

[Borislav Petkov]

On Wed, Mar 18, 2015 at 11:02:27AM +0100, Borislav Petkov wrote:
> I don't like the ifdeffery in your solution and would like to try to fix
> it in a cleaner way. Unless you come up with a better solution first.

Ok, how about this below? It is more involved but finds and loads the
microcode built-in into the kernel in the early loader which is when
you want to load microcode anyway. Only AMD for now but that should be
enough for testing.

---
From: Borislav Petkov <bp@suse.de>
Date: Wed, 18 Mar 2015 19:28:56 +0100
Subject: [RFC PATCH] x86/microcode: Parse built-in microcode early

Only AMD for now, WIP.

Not-yet-signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/include/asm/microcode.h           |  8 +++++++-
 arch/x86/include/asm/microcode_amd.h       |  4 ++--
 arch/x86/kernel/cpu/microcode/amd_early.c  | 13 ++++++++++---
 arch/x86/kernel/cpu/microcode/core_early.c | 23 ++++++++++++++++++++++-
 4 files changed, 41 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index 2fb20d6f7e23..8924b9a65603 100644
--- a/arch/x86/include/asm/microcode.h
+++ b/arch/x86/include/asm/microcode.h
@@ -1,6 +1,8 @@
 #ifndef _ASM_X86_MICROCODE_H
 #define _ASM_X86_MICROCODE_H
 
+#include <linux/earlycpio.h>
+
 #define native_rdmsr(msr, val1, val2)			\
 do {							\
 	u64 __val = native_read_msr((msr));		\
@@ -152,6 +154,7 @@ extern void __init load_ucode_bsp(void);
 extern void load_ucode_ap(void);
 extern int __init save_microcode_in_initrd(void);
 void reload_early_microcode(void);
+extern bool get_builtin_firmware(struct cpio_data *cd, const char *name);
 #else
 static inline void __init load_ucode_bsp(void) {}
 static inline void load_ucode_ap(void) {}
@@ -160,6 +163,9 @@ static inline int __init save_microcode_in_initrd(void)
 	return 0;
 }
 static inline void reload_early_microcode(void) {}
+static bool get_builtin_firmware(struct cpio_data *cd, const char *name)
+{
+	return false;
+}
 #endif
-
 #endif /* _ASM_X86_MICROCODE_H */
diff --git a/arch/x86/include/asm/microcode_amd.h b/arch/x86/include/asm/microcode_amd.h
index af935397e053..b8438543f340 100644
--- a/arch/x86/include/asm/microcode_amd.h
+++ b/arch/x86/include/asm/microcode_amd.h
@@ -65,12 +65,12 @@ extern enum ucode_state load_microcode_amd(int cpu, u8 family, const u8 *data, s
 extern u8 amd_ucode_patch[PATCH_MAX_SIZE];
 
 #ifdef CONFIG_MICROCODE_AMD_EARLY
-extern void __init load_ucode_amd_bsp(void);
+extern void __init load_ucode_amd_bsp(int family);
 extern void load_ucode_amd_ap(void);
 extern int __init save_microcode_in_initrd_amd(void);
 void reload_ucode_amd(void);
 #else
-static inline void __init load_ucode_amd_bsp(void) {}
+static inline void __init load_ucode_amd_bsp(int family) {}
 static inline void load_ucode_amd_ap(void) {}
 static inline int __init save_microcode_in_initrd_amd(void) { return -EINVAL; }
 void reload_ucode_amd(void) {}
diff --git a/arch/x86/kernel/cpu/microcode/amd_early.c b/arch/x86/kernel/cpu/microcode/amd_early.c
index 737737edbd1e..10d9068ca4f6 100644
--- a/arch/x86/kernel/cpu/microcode/amd_early.c
+++ b/arch/x86/kernel/cpu/microcode/amd_early.c
@@ -228,8 +228,9 @@ static void apply_ucode_in_initrd(void *ucode, size_t size, bool save_patch)
 	}
 }
 
-void __init load_ucode_amd_bsp(void)
+void __init load_ucode_amd_bsp(int family)
 {
+	char fw_name[26] = "microcode_amd.bin";
 	struct cpio_data cp;
 	void **data;
 	size_t *size;
@@ -243,8 +244,14 @@ void __init load_ucode_amd_bsp(void)
 #endif
 
 	cp = find_ucode_in_initrd();
-	if (!cp.data)
-		return;
+	if (!cp.data) {
+		if (family >= 0x15)
+			snprintf(fw_name, sizeof(fw_name),
+				 "microcode_amd_fam%.2xh.bin", family);
+
+		if (!get_builtin_firmware(&cp, fw_name))
+			return;
+	}
 
 	*data = cp.data;
 	*size = cp.size;
diff --git a/arch/x86/kernel/cpu/microcode/core_early.c b/arch/x86/kernel/cpu/microcode/core_early.c
index a413a69cbd74..beda0ea2409e 100644
--- a/arch/x86/kernel/cpu/microcode/core_early.c
+++ b/arch/x86/kernel/cpu/microcode/core_early.c
@@ -3,6 +3,7 @@
  *
  *	Copyright (C) 2012 Fenghua Yu <fenghua.yu@intel.com>
  *			   H Peter Anvin" <hpa@zytor.com>
+ *		  (C) 2015 Borislav Petkov <bp@suse.de>
  *
  *	This driver allows to early upgrade microcode on Intel processors
  *	belonging to IA-32 family - PentiumPro, Pentium II,
@@ -17,6 +18,7 @@
  *	2 of the License, or (at your option) any later version.
  */
 #include <linux/module.h>
+#include <linux/firmware.h>
 #include <asm/microcode.h>
 #include <asm/microcode_intel.h>
 #include <asm/microcode_amd.h>
@@ -43,6 +45,25 @@ static bool __init check_loader_disabled_bsp(void)
 	return *res;
 }
 
+extern struct builtin_fw __start_builtin_fw[];
+extern struct builtin_fw __end_builtin_fw[];
+
+bool get_builtin_firmware(struct cpio_data *cd, const char *name)
+{
+#ifdef CONFIG_FW_LOADER
+	struct builtin_fw *b_fw;
+
+	for (b_fw = __start_builtin_fw; b_fw != __end_builtin_fw; b_fw++) {
+		if (!strcmp(name, b_fw->name)) {
+			cd->size = b_fw->size;
+			cd->data = b_fw->data;
+			return true;
+		}
+	}
+#endif
+	return false;
+}
+
 void __init load_ucode_bsp(void)
 {
 	int vendor, family;
@@ -63,7 +84,7 @@ void __init load_ucode_bsp(void)
 		break;
 	case X86_VENDOR_AMD:
 		if (family >= 0x10)
-			load_ucode_amd_bsp();
+			load_ucode_amd_bsp(family);
 		break;
 	default:
 		break;
-- 
2.3.3

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH] x86: Unbreak early processor microcode loading

[Daniel J Blueman]

On Thu, Mar 19, 2015 at 2:42 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Wed, Mar 18, 2015 at 11:02:27AM +0100, Borislav Petkov wrote:
>>  I don't like the ifdeffery in your solution and would like to try 
>> to fix
>>  it in a cleaner way. Unless you come up with a better solution 
>> first.
> 
> Ok, how about this below? It is more involved but finds and loads the
> microcode built-in into the kernel in the early loader which is when
> you want to load microcode anyway. Only AMD for now but that should be
> enough for testing.
[]

Neat; I added the 'amd-ucode/' directory prefix to the string and 
adjusted the buffer size, and the microcode is loaded, but we see 
consistent hangs when onlining core 32 on the first server.

Further investigation shows the BIOS in this platform isn't loading the 
microcode after the Hypertransport reconfig warm-boot (though other 
platforms do), so it's a better solution I engage with the vendor to 
fix their BIOS, rather than working around it in the kernel at this 
time.

Many thanks!
  Daniel

Re: [PATCH] x86: Unbreak early processor microcode loading

[Borislav Petkov]

On Thu, Mar 19, 2015 at 03:30:23PM +0800, Daniel J Blueman wrote:
> Neat; I added the 'amd-ucode/' directory prefix to the string and adjusted
> the buffer size, and the microcode is loaded,

We shouldn't need to do that though for obvious reasons.

IOW, you need to type in only the filenames of the microcode blobs in Kconfig
option:

CONFIG_EXTRA_FIRMWARE="microcode_amd.bin microcode_amd_fam15h.bin microcode_amd_fam16h.bin"

and then add the absolute path to them with:

CONFIG_EXTRA_FIRMWARE_DIR="/lib/firmware/amd-ucode"

That worked here yesterday.

> but we see consistent hangs when onlining core 32 on the first server.

That's some system boundary, i.e. 4*8.

> Further investigation shows the BIOS in this platform isn't loading the
> microcode after the Hypertransport reconfig warm-boot (though other
> platforms do), so it's a better solution I engage with the vendor to fix
> their BIOS, rather than working around it in the kernel at this time.

Oh, most definitely!

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH] x86: Unbreak early processor microcode loading

[Henrique de Moraes Holschuh]

On Thu, 19 Mar 2015, Borislav Petkov wrote:
> On Thu, Mar 19, 2015 at 03:30:23PM +0800, Daniel J Blueman wrote:
> > Neat; I added the 'amd-ucode/' directory prefix to the string and adjusted
> > the buffer size, and the microcode is loaded,
> 
> We shouldn't need to do that though for obvious reasons.
> 
> IOW, you need to type in only the filenames of the microcode blobs in Kconfig
> option:
> 
> CONFIG_EXTRA_FIRMWARE="microcode_amd.bin microcode_amd_fam15h.bin microcode_amd_fam16h.bin"
> 
> and then add the absolute path to them with:
> 
> CONFIG_EXTRA_FIRMWARE_DIR="/lib/firmware/amd-ucode"
> 
> That worked here yesterday.

I apologise for the late reply.  Please disregard it if it is outdated.

That would obviously work, but people often have other firmware
built-in, so the likehood of CONFIG_EXTRA_FIRMWARE_DIR pointing to the
root of a linux-firmware work tree or to "/lib/firmware" is not low at
all.

In fact, it is natural to expect that CONFIG_EXTRA_FIRMWARE should point
to something that will result in the same filenames as the kernel would
want to use for regular firmware loading.  The current text of the
Kconfig help for CONFIG_EXTRA_FIRMWARE even says so.

So, FWIW, I do think we should always use the same path for builtin and
regular firmware requests, based on the least-surprise principle.

-- 
  "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: Unbreak early processor microcode loading

[Borislav Petkov]

On Wed, Apr 29, 2015 at 03:23:57PM -0300, Henrique de Moraes Holschuh wrote:
> That would obviously work, but people often have other firmware
> built-in, so the likehood of CONFIG_EXTRA_FIRMWARE_DIR pointing to the
> root of a linux-firmware work tree or to "/lib/firmware" is not low at
> all.
> 
> In fact, it is natural to expect that CONFIG_EXTRA_FIRMWARE should point
> to something that will result in the same filenames as the kernel would
> want to use for regular firmware loading.  The current text of the
> Kconfig help for CONFIG_EXTRA_FIRMWARE even says so.

You mean that:

	  "These files should exist under
          the directory specified by the EXTRA_FIRMWARE_DIR option, which is
          by default the firmware subdirectory of the kernel source tree.

          For example, you might set CONFIG_EXTRA_FIRMWARE="usb8388.bin", copy
          the usb8388.bin file into the firmware directory, and build the kernel.
          Then any request_firmware("usb8388.bin") will be satisfied internally
          without needing to call out to userspace."

So people with lotsa firmware should put it all under one directory and
all should just work.

> So, FWIW, I do think we should always use the same path for builtin and
> regular firmware requests, based on the least-surprise principle.

We don't hardcode the path - only the name. What I gave was an example
only.

The regular microcode updates, i.e. the late ones, use firmware class
which has a bunch of hardcoded paths:

static const char * const fw_path[] = {
        fw_path_para,
        "/lib/firmware/updates/" UTS_RELEASE,
        "/lib/firmware/updates",
        "/lib/firmware/" UTS_RELEASE,
        "/lib/firmware"
};

and that's different from CONFIG_FIRMWARE_DIR.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH] x86: Unbreak early processor microcode loading

[Henrique de Moraes Holschuh]

On Wed, 29 Apr 2015, Borislav Petkov wrote:
> On Wed, Apr 29, 2015 at 03:23:57PM -0300, Henrique de Moraes Holschuh wrote:
> > That would obviously work, but people often have other firmware
> > built-in, so the likehood of CONFIG_EXTRA_FIRMWARE_DIR pointing to the
> > root of a linux-firmware work tree or to "/lib/firmware" is not low at
> > all.
> > 
> > In fact, it is natural to expect that CONFIG_EXTRA_FIRMWARE should point
> > to something that will result in the same filenames as the kernel would
> > want to use for regular firmware loading.  The current text of the
> > Kconfig help for CONFIG_EXTRA_FIRMWARE even says so.
> 
> You mean that:
> 
> 	  "These files should exist under
>           the directory specified by the EXTRA_FIRMWARE_DIR option, which is
>           by default the firmware subdirectory of the kernel source tree.
> 
>           For example, you might set CONFIG_EXTRA_FIRMWARE="usb8388.bin", copy
>           the usb8388.bin file into the firmware directory, and build the kernel.
>           Then any request_firmware("usb8388.bin") will be satisfied internally
>           without needing to call out to userspace."
> 
> So people with lotsa firmware should put it all under one directory and
> all should just work.

That's one possible reading, yes.  Only, it will break for some drivers.

The full Kconfig text of the two most-relevant kconfig defines are:

config EXTRA_FIRMWARE
        This option allows firmware to be built into the kernel for the
        case where the user either cannot or doesn't want to provide it
        from userspace at runtime (for example, when the firmware in
        question is required for accessing the boot device, and the user
        doesn't want to use an initrd).

        This option is a string and takes the (space-separated) names of
        the firmware files -- the same names that appear in
        MODULE_FIRMWARE() and request_firmware() in the source. These
        files should exist under the directory specified by the
        EXTRA_FIRMWARE_DIR option, which is by default the firmware
        subdirectory of the kernel source tree.

        For example, you might set CONFIG_EXTRA_FIRMWARE="usb8388.bin",
        copy the usb8388.bin file into the firmware directory, and build
        the kernel.  Then any request_firmware("usb8388.bin") will be
        satisfied internally without needing to call out to userspace.

        WARNING: If you include additional firmware files into your
        binary kernel image that are not available under the terms of
        the GPL, then it may be a violation of the GPL to distribute the
        resulting image since it combines both GPL and non-GPL work. You
        should consult a lawyer of your own before distributing such an
        image.

config EXTRA_FIRMWARE_DIR
        This option controls the directory in which the kernel build
        system looks for the firmware files listed in the EXTRA_FIRMWARE
        option.  The default is firmware/ in the kernel source tree, but
        by changing this option you can point it elsewhere, such as
        /lib/firmware/ or some other directory containing the firmware
        files.

Note the explicit mention of /lib/firmware as a possibility for
CONFIG_EXTRA_FIRMWARE_DIR, as well as the second paragraph in the
CONFIG_EXTRA_FIRMWARE help text.

This help text needs some love...

Things only work for the drivers that do something like
MODULE_FIRMWARE("foo/bar.fw") when you consider that by "names of the
firmware files" the help text actually means "relative path to the
firmware files".

For the drivers do MODULE_FIRMWARE("foo/bar.fw"), should you place the
firmware file directly in the root of EXTRA_FIRMWARE_DIR (built-in case)
or in the root of the firmware search path for the non-builtin case
(i.e., if drop the "foo/"), it is not going to be found by
request_firmware().

That's why IMHO the least-suprise path would be for builtin microcode to
be searched with the same relative path as it would for late loading.
Because that's how regular drivers that do request_firmware() behave.

FWIW, my preference would be for the early microcode core to look for
builtin firmware using the same names used for the early initramfs
(including the kernel/x86/microcode/ prefix), since it is doing an early
update.  But that would certainly require documentation.

-- 
  "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: Unbreak early processor microcode loading

[Borislav Petkov]

On Wed, Apr 29, 2015 at 05:54:29PM -0300, Henrique de Moraes Holschuh wrote:
> That's why IMHO the least-suprise path would be for builtin microcode to
> be searched with the same relative path as it would for late loading.
> Because that's how regular drivers that do request_firmware() behave.

Ok, I'll add "amd-ucode/" and "intel-ucode/" as prefixes to the built-in
microcode loading path.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--