[PATCH] x86/microcode: Allow early loading without initrd

[PATCH] x86/microcode: Allow early loading without initrd

[Alexander Hirsch]

Microcode can be baked into the kernel image via CONFIG_EXTRA_FIRMWARE
and the early loader supports that, but still depended on
BLK_DEV_INITRD.
This dependency is removed.

Signed-off-by: Alexander Hirsch <1zeeky@gmail.com>
---
This patch depends on the "Parse built-in microcode early" patch by Borislav Petkov (https://lkml.org/lkml/2015/4/1/331).
I only tested this on two Intel machines (an i3 M330 and an i5-4690). There is no change in amd_early.c because it compiles fine - only intel_early.c complained without BLK_DEV_INITRD.

 arch/x86/Kconfig                            |  2 +-
 arch/x86/kernel/cpu/microcode/intel_early.c | 30 +++++++++++++++++++++--------
 2 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 226d569..bc7e187 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1144,7 +1144,7 @@ config MICROCODE_AMD_EARLY
 
 config MICROCODE_EARLY
 	bool "Early load microcode"
-	depends on MICROCODE=y && BLK_DEV_INITRD
+	depends on MICROCODE=y
 	select MICROCODE_INTEL_EARLY if MICROCODE_INTEL
 	select MICROCODE_AMD_EARLY if MICROCODE_AMD
 	default y
diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
index 98d320c..3d5906e 100644
--- a/arch/x86/kernel/cpu/microcode/intel_early.c
+++ b/arch/x86/kernel/cpu/microcode/intel_early.c
@@ -691,6 +691,9 @@ int __init save_microcode_in_initrd_intel(void)
 	unsigned int count = mc_saved_data.mc_saved_count;
 	struct microcode_intel *mc_saved[MAX_UCODE_COUNT];
 	int ret = 0;
+#ifndef BLK_DEV_INITRD
+	unsigned long initrd_start = 0;
+#endif
 
 	if (count == 0)
 		return ret;
@@ -728,24 +731,32 @@ _load_ucode_intel_bsp(struct mc_saved_data *mc_saved_data,
 
 void __init load_ucode_intel_bsp(void)
 {
-	u64 start, size;
+	u64 start = 0, size = 0;
+	struct mc_saved_data *mc_saved_data_p;
+	unsigned long *mc_saved_in_initrd_p;
+#ifdef BLK_DEV_INITRD
 #ifdef CONFIG_X86_32
 	struct boot_params *p;
 
 	p	= (struct boot_params *)__pa_nodebug(&boot_params);
 	start	= p->hdr.ramdisk_image;
 	size	= p->hdr.ramdisk_size;
-
-	_load_ucode_intel_bsp(
-			(struct mc_saved_data *)__pa_nodebug(&mc_saved_data),
-			(unsigned long *)__pa_nodebug(&mc_saved_in_initrd),
-			start, size);
 #else
 	start	= boot_params.hdr.ramdisk_image + PAGE_OFFSET;
 	size	= boot_params.hdr.ramdisk_size;
-
-	_load_ucode_intel_bsp(&mc_saved_data, mc_saved_in_initrd, start, size);
 #endif
+#endif
+
+#ifdef CONFIG_X86_32
+	mc_saved_in_initrd_p =
+		(unsigned long *)__pa_nodebug(mc_saved_in_initrd);
+	mc_saved_data_p = (struct mc_saved_data *)__pa_nodebug(&mc_saved_data);
+#else
+	mc_saved_data_p = &mc_saved_data;
+	mc_saved_in_initrd_p = mc_saved_in_initrd;
+#endif
+
+	_load_ucode_intel_bsp(mc_saved_data_p, mc_saved_in_initrd_p, start, size);
 }
 
 void load_ucode_intel_ap(void)
@@ -755,6 +766,9 @@ void load_ucode_intel_ap(void)
 	unsigned long *mc_saved_in_initrd_p;
 	unsigned long initrd_start_addr;
 	enum ucode_state ret;
+#ifndef BLK_DEV_INITRD
+	unsigned long initrd_start = 0;
+#endif
 #ifdef CONFIG_X86_32
 	unsigned long *initrd_start_p;
 
-- 
2.3.6

Re: [PATCH] x86/microcode: Allow early loading without initrd

[Paul Bolle]

On Sun, 2015-04-26 at 17:03 +0200, Alexander Hirsch wrote:
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
 
>  config MICROCODE_EARLY
>  	bool "Early load microcode"
> -	depends on MICROCODE=y && BLK_DEV_INITRD
> +	depends on MICROCODE=y
>  	select MICROCODE_INTEL_EARLY if MICROCODE_INTEL
>  	select MICROCODE_AMD_EARLY if MICROCODE_AMD
>  	default y

> --- a/arch/x86/kernel/cpu/microcode/intel_early.c
> +++ b/arch/x86/kernel/cpu/microcode/intel_early.c
> @@ -691,6 +691,9 @@ int __init save_microcode_in_initrd_intel(void)
>  	unsigned int count = mc_saved_data.mc_saved_count;
>  	struct microcode_intel *mc_saved[MAX_UCODE_COUNT];
>  	int ret = 0;
> +#ifndef BLK_DEV_INITRD

#ifndef CONFIG_BLK_DEV_INITRD?

> +	unsigned long initrd_start = 0;
> +#endif
>  
>  	if (count == 0)
>  		return ret;
> @@ -728,24 +731,32 @@ _load_ucode_intel_bsp(struct mc_saved_data *mc_saved_data,
>  
>  void __init load_ucode_intel_bsp(void)
>  {
> -	u64 start, size;
> +	u64 start = 0, size = 0;
> +	struct mc_saved_data *mc_saved_data_p;
> +	unsigned long *mc_saved_in_initrd_p;
> +#ifdef BLK_DEV_INITRD

Likewise.

>  #ifdef CONFIG_X86_32
>  	struct boot_params *p;
>  
>  	p	= (struct boot_params *)__pa_nodebug(&boot_params);
>  	start	= p->hdr.ramdisk_image;
>  	size	= p->hdr.ramdisk_size;
> -
> -	_load_ucode_intel_bsp(
> -			(struct mc_saved_data *)__pa_nodebug(&mc_saved_data),
> -			(unsigned long *)__pa_nodebug(&mc_saved_in_initrd),
> -			start, size);
>  #else
>  	start	= boot_params.hdr.ramdisk_image + PAGE_OFFSET;
>  	size	= boot_params.hdr.ramdisk_size;
> -
> -	_load_ucode_intel_bsp(&mc_saved_data, mc_saved_in_initrd, start, size);
>  #endif
> +#endif
> +
> +#ifdef CONFIG_X86_32
> +	mc_saved_in_initrd_p =
> +		(unsigned long *)__pa_nodebug(mc_saved_in_initrd);
> +	mc_saved_data_p = (struct mc_saved_data *)__pa_nodebug(&mc_saved_data);
> +#else
> +	mc_saved_data_p = &mc_saved_data;
> +	mc_saved_in_initrd_p = mc_saved_in_initrd;
> +#endif
> +
> +	_load_ucode_intel_bsp(mc_saved_data_p, mc_saved_in_initrd_p, start, size);
>  }
>  
>  void load_ucode_intel_ap(void)
> @@ -755,6 +766,9 @@ void load_ucode_intel_ap(void)
>  	unsigned long *mc_saved_in_initrd_p;
>  	unsigned long initrd_start_addr;
>  	enum ucode_state ret;
> +#ifndef BLK_DEV_INITRD

Likewise.

> +	unsigned long initrd_start = 0;
> +#endif
>  #ifdef CONFIG_X86_32
>  	unsigned long *initrd_start_p;
>  

Thanks,


Paul Bolle

Re: [PATCH] x86/microcode: Allow early loading without initrd

[Alexander Hirsch]

On Sun, 26 Apr 2015 17:31:13 +0200
Paul Bolle <pebolle@tiscali.nl> wrote:

> #ifndef CONFIG_BLK_DEV_INITRD?  

Indeed, thank you. I fixed it.

---
From: Alexander 'z33ky' Hirsch <1zeeky@gmail.com>
Date: Sun, 26 Apr 2015 15:18:18 +0200
Subject: [PATCH] x86, microcode: Allow early loading without initrd

Microcode can be baked into the kernel image via CONFIG_EXTRA_FIRMWARE
and the early loader supports that, but still depended on
BLK_DEV_INITRD.
This dependency is removed.

Signed-off-by: Alexander Hirsch <1zeeky@gmail.com>
---
 arch/x86/Kconfig                            |  2 +-
 arch/x86/kernel/cpu/microcode/intel_early.c | 30 +++++++++++++++++++++--------
 2 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 226d569..bc7e187 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1144,7 +1144,7 @@ config MICROCODE_AMD_EARLY
 
 config MICROCODE_EARLY
 	bool "Early load microcode"
-	depends on MICROCODE=y && BLK_DEV_INITRD
+	depends on MICROCODE=y
 	select MICROCODE_INTEL_EARLY if MICROCODE_INTEL
 	select MICROCODE_AMD_EARLY if MICROCODE_AMD
 	default y
diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
index 98d320c..ad7901f 100644
--- a/arch/x86/kernel/cpu/microcode/intel_early.c
+++ b/arch/x86/kernel/cpu/microcode/intel_early.c
@@ -691,6 +691,9 @@ int __init save_microcode_in_initrd_intel(void)
 	unsigned int count = mc_saved_data.mc_saved_count;
 	struct microcode_intel *mc_saved[MAX_UCODE_COUNT];
 	int ret = 0;
+#ifndef CONFIG_BLK_DEV_INITRD
+	unsigned long initrd_start = 0;
+#endif
 
 	if (count == 0)
 		return ret;
@@ -728,24 +731,32 @@ _load_ucode_intel_bsp(struct mc_saved_data *mc_saved_data,
 
 void __init load_ucode_intel_bsp(void)
 {
-	u64 start, size;
+	u64 start = 0, size = 0;
+	struct mc_saved_data *mc_saved_data_p;
+	unsigned long *mc_saved_in_initrd_p;
+#ifdef CONFIG_BLK_DEV_INITRD
 #ifdef CONFIG_X86_32
 	struct boot_params *p;
 
 	p	= (struct boot_params *)__pa_nodebug(&boot_params);
 	start	= p->hdr.ramdisk_image;
 	size	= p->hdr.ramdisk_size;
-
-	_load_ucode_intel_bsp(
-			(struct mc_saved_data *)__pa_nodebug(&mc_saved_data),
-			(unsigned long *)__pa_nodebug(&mc_saved_in_initrd),
-			start, size);
 #else
 	start	= boot_params.hdr.ramdisk_image + PAGE_OFFSET;
 	size	= boot_params.hdr.ramdisk_size;
-
-	_load_ucode_intel_bsp(&mc_saved_data, mc_saved_in_initrd, start, size);
 #endif
+#endif
+
+#ifdef CONFIG_X86_32
+	mc_saved_in_initrd_p =
+		(unsigned long *)__pa_nodebug(mc_saved_in_initrd);
+	mc_saved_data_p = (struct mc_saved_data *)__pa_nodebug(&mc_saved_data);
+#else
+	mc_saved_data_p = &mc_saved_data;
+	mc_saved_in_initrd_p = mc_saved_in_initrd;
+#endif
+
+	_load_ucode_intel_bsp(mc_saved_data_p, mc_saved_in_initrd_p, start, size);
 }
 
 void load_ucode_intel_ap(void)
@@ -755,6 +766,9 @@ void load_ucode_intel_ap(void)
 	unsigned long *mc_saved_in_initrd_p;
 	unsigned long initrd_start_addr;
 	enum ucode_state ret;
+#ifndef CONFIG_BLK_DEV_INITRD
+	unsigned long initrd_start = 0;
+#endif
 #ifdef CONFIG_X86_32
 	unsigned long *initrd_start_p;
 
-- 
2.3.6

Re: [PATCH] x86/microcode: Allow early loading without initrd

[Borislav Petkov]

On Sun, Apr 26, 2015 at 05:03:14PM +0200, Alexander Hirsch wrote:
> Microcode can be baked into the kernel image via CONFIG_EXTRA_FIRMWARE
> and the early loader supports that, but still depended on
> BLK_DEV_INITRD.
> This dependency is removed.

Yeah, it is removed but it adds a bunch of ugly ifdeffery which makes
the code even more unreadable than it is :-\

> Signed-off-by: Alexander Hirsch <1zeeky@gmail.com>
> ---
>
> This patch depends on the "Parse built-in microcode early" patch by
> Borislav Petkov (https://lkml.org/lkml/2015/4/1/331).

In the future, please CC me on microcode loader patches.

> I only tested this on two Intel machines (an i3 M330 and an i5-4690).
> There is no change in amd_early.c because it compiles fine - only
> intel_early.c complained without BLK_DEV_INITRD.

Right, so I see what you're trying to achieve and I'd guess your
reasoning is that BLK_DEV_INITRD is not really needed if you build your
microcode into the kernel. And that's ok but I'm not persuaded yet:

* why do you even bother - BLK_DEV_INITRD gets enabled on all distro
kernels and almost everything running Linux so why bother? Or do you
have a special use case which doesn't want BLK_DEV_INITRD. I'd be
interested to hear about it.

* the early loader was done with initrd in mind and it was/still is its
main source for microcode blobs early in the boot. So if we want to
make it not-mandatory, then the driver needs to be reorganized so that
builtin blobs and initrd blobs loading paths are cleanly untangled. The
ifdeffery thing might work now but is certainly not future-proof so it
would need to be designed in a cleaner way.

Perhaps something like a microcode cache of patches the AMD loader has,
all decoupled from the loading paths or so... I don't have a good idea
right now. I'll have to think about it.

Thanks.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] x86/microcode: Allow early loading without initrd

[Alexander Hirsch]

On Sun, 26 Apr 2015 18:16:09 +0200
Borislav Petkov <bp@alien8.de> wrote:

> 
> Yeah, it is removed but it adds a bunch of ugly ifdeffery which makes
> the code even more unreadable than it is :-\
> 

I wasn't to happy about the nested ifdefs either, but given my inexperience in the kernel code I didn't want to push things around too much.
It should be possible to better hide the X86_32 pointer wizardry in a macro, but I wouldn't trust myself submitting patches for code I didn't run. Perhaps I should and just ask for testers.

> 
> In the future, please CC me on microcode loader patches.
> 

Will do.

> 
> * why do you even bother - BLK_DEV_INITRD gets enabled on all distro
> kernels and almost everything running Linux so why bother? Or do you
> have a special use case which doesn't want BLK_DEV_INITRD. I'd be
> interested to hear about it.
> 

I trimmed my kernel config down to what I need, think I need or don't trust myself to know if I can disable it.
There is nothing actually hindering me from using an initrd, so this patch, at least for me personally, does not have a high priority to become mainlined. It still would be nice of course.

I had some segfaults due to the broken lock elision features of my CPU and found out that built-in microcode isn't considered (in the stable kernel).
When I saw your patch for loading built-in microcode I simply thought that this would allow me to have early microcode patching without the need of an initrd.

Boot-time might be improved by this and of course a few bytes are saved, but all in all probably not noteworthy. I did it, because (I thought) I can.

> * the early loader was done with initrd in mind and it was/still is its
> main source for microcode blobs early in the boot. So if we want to
> make it not-mandatory, then the driver needs to be reorganized so that
> builtin blobs and initrd blobs loading paths are cleanly untangled. The
> ifdeffery thing might work now but is certainly not future-proof so it
> would need to be designed in a cleaner way.
> 
> Perhaps something like a microcode cache of patches the AMD loader has,
> all decoupled from the loading paths or so... I don't have a good idea
> right now. I'll have to think about it.
> 

It seems logical that the code was written with initrd in mind for early boot.

What would be the downside of a microcode cache?
I didn't follow the whole flow of how the CPUs get to their microcode patches, but I thought the mc_saved* stuff did caching of the microcode, since scan_microcode, the only user of load_builtin_intel_microcode, is only called for the BSP and the other cores thus seem to get it from somewhere else.

Thanks,
Alexander Hirsch

Re: [PATCH] x86/microcode: Allow early loading without initrd

[Borislav Petkov]

On Sun, Apr 26, 2015 at 07:27:56PM +0200, Alexander Hirsch wrote:
> I wasn't to happy about the nested ifdefs either, but given my
> inexperience in the kernel code I didn't want to push things around
> too much.

Right, so this is the current situation: The intel early loader is being
aggressively cleaned up currently.

Then, the second early loading method which gets the builtin microcode,
i.e., not initrd, is not even upstream yet. I did test it a bit and it
worked fine but it needs more testing before it goes into 4.1.

Now, I would like to design the two methods cleanly by having Kconfig
suboptions which select BLK_DEV_INITRD or FW_LOADER and both options
have a good Kconfig help text explaining how one does either methods.

And, most importantly, untangle those two methods so that we can support
either and do that cleanly.

> I trimmed my kernel config down to what I need, think I need or don't
> trust myself to know if I can disable it. There is nothing actually
> hindering me from using an initrd, so this patch, at least for me
> personally, does not have a high priority to become mainlined. It
> still would be nice of course.

Right, so please use the current state with enabling BLK_DEV_INITRD
until this is done right. I have this on my radar and am working on it
so I'll get it done sooner or later.

> I had some segfaults due to the broken lock elision features of my CPU
> and found out that built-in microcode isn't considered (in the stable
> kernel). When I saw your patch for loading built-in microcode I simply
> thought that this would allow me to have early microcode patching
> without the need of an initrd.
>
> Boot-time might be improved by this and of course a few bytes are
> saved, but all in all probably not noteworthy. I did it, because (I
> thought) I can.

No, that's fine - you actually made me realize that we probably should
separate the early loading methods so that was a good thing. Thanks!

> It seems logical that the code was written with initrd in mind for
> early boot.

Oh sure. Perhaps the only advantage of the initrd method I can think of
is that if you want to upgrade microcode, you need to regenerate only
the initrd, not the whole kernel but you would have to reboot anyway to
load it.

For later loading without reboot we have a different method where we
don't need initrd or whatever.

So yeah, if we're going to support more than one early loading method,
I'd like to have those nice and clean and separated from one-another.

> What would be the downside of a microcode cache?

Oh nothing - it just needs to be written and tested :-)

> I didn't follow the whole flow of how the CPUs get to their
> microcode patches, but I thought the mc_saved* stuff did caching
> of the microcode, since scan_microcode, the only user of
> load_builtin_intel_microcode, is only called for the BSP and the other
> cores thus seem to get it from somewhere else.

Yeah, it is a bunch jumping through hoops which needs to be simplified
first :-)

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] x86/microcode: Allow early loading without initrd

[Henrique de Moraes Holschuh]

On Sun, Apr 26, 2015, at 13:16, Borislav Petkov wrote:
> * the early loader was done with initrd in mind and it was/still is its
> main source for microcode blobs early in the boot. So if we want to
> make it not-mandatory, then the driver needs to be reorganized so that
> builtin blobs and initrd blobs loading paths are cleanly untangled. The
> ifdeffery thing might work now but is certainly not future-proof so it
> would need to be designed in a cleaner way.
> 
> Perhaps something like a microcode cache of patches the AMD loader has,
> all decoupled from the loading paths or so... I don't have a good idea
> right now. I'll have to think about it.

Well, the *early* Intel driver does have a cache of sorts. It could use (a lot of) love, though...

The "cache" in the early intel microcode update driver is implemented by mc_saved_in_initrd[].  It is currently sub-optimal, in that it has MAX_UCODE_COUNT slots (128 slots).

The worst-case real-world fill of this cache is, currently, 7 slots (processor signature 0x6FB). The typical fill would be 1-4 slots.

If we change the code to store at maximum one microcode per pfmask bit plus some other details to handle the "processor does not support pfmask, so it is all zeroes" special case, we can safely change MAX_UCODE_COUNT to either 8 or 9, depending on implementation details.

-- 
  "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