diff mbox series

[stable] x86/microcode: do not cache microcode if it will not be used

Message ID 20240115102202.1321115-1-pbonzini@redhat.com
State New
Headers
Series [stable] x86/microcode: do not cache microcode if it will not be used |

Commit Message

Paolo Bonzini Jan. 15, 2024, 10:22 a.m. UTC 
  [ Upstream commit a7939f01672034a58ad3fdbce69bb6c665ce0024 ]

Builtin/initrd microcode will not be used the ucode loader is disabled.
But currently, save_microcode_in_initrd is always performed and it
accesses MSR_IA32_UCODE_REV even if dis_ucode_ldr is true, and in
particular even if X86_FEATURE_HYPERVISOR is set; the TDX module does not
implement the MSR and the result is a call trace at boot for TDX guests.

Mainline Linux fixed this as part of a more complex rework of microcode
caching that went into 6.7 (see in particular commits dd5e3e3ca6,
"x86/microcode/intel: Simplify early loading"; and a7939f0167203,
"x86/microcode/amd: Cache builtin/initrd microcode early").  Do the bare
minimum in stable kernels, setting initrd_gone just like mainline Linux
does in mark_initrd_gone().

Note that save_microcode_in_initrd() is not in the microcode application
path, which runs with paging disabled on 32-bit systems, so it can (and
has to) use dis_ucode_ldr instead of check_loader_disabled_ap().

Cc: stable@vger.kernel.org # v6.6+
Cc: x86@kernel.org # v6.6+
Cc: Borislav Petkov <bp@suse.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kernel/cpu/microcode/core.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Borislav Petkov Jan. 15, 2024, 10:56 a.m. UTC | #1 
On Mon, Jan 15, 2024 at 11:22:02AM +0100, Paolo Bonzini wrote:
> [ Upstream commit a7939f01672034a58ad3fdbce69bb6c665ce0024 ]
> 
> Builtin/initrd microcode will not be used the ucode loader is disabled.
> But currently, save_microcode_in_initrd is always performed and it
> accesses MSR_IA32_UCODE_REV even if dis_ucode_ldr is true, and in
> particular even if X86_FEATURE_HYPERVISOR is set; the TDX module does not
> implement the MSR and the result is a call trace at boot for TDX guests.
> 
> Mainline Linux fixed this as part of a more complex rework of microcode
> caching that went into 6.7 (see in particular commits dd5e3e3ca6,
> "x86/microcode/intel: Simplify early loading"; and a7939f0167203,
> "x86/microcode/amd: Cache builtin/initrd microcode early").  Do the bare
> minimum in stable kernels, setting initrd_gone just like mainline Linux
> does in mark_initrd_gone().
> 
> Note that save_microcode_in_initrd() is not in the microcode application
> path, which runs with paging disabled on 32-bit systems, so it can (and
> has to) use dis_ucode_ldr instead of check_loader_disabled_ap().

.. simply because save_microcode_in_initrd() is a fs_initcall and by
then 32-bit has managed to enable paging.

> Cc: stable@vger.kernel.org # v6.6+
> Cc: x86@kernel.org # v6.6+
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kernel/cpu/microcode/core.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
> index 35d39a13dc90..503b5da56685 100644
> --- a/arch/x86/kernel/cpu/microcode/core.c
> +++ b/arch/x86/kernel/cpu/microcode/core.c
> @@ -214,6 +214,11 @@ static int __init save_microcode_in_initrd(void)
>  	struct cpuinfo_x86 *c = &boot_cpu_data;
>  	int ret = -EINVAL;
>  
> +	if (dis_ucode_ldr) {
> +		ret = 0;
> +		goto out;
> +	}
> +
>  	switch (c->x86_vendor) {
>  	case X86_VENDOR_INTEL:
>  		if (c->x86 >= 6)
> @@ -227,6 +230,7 @@ static int __init save_microcode_in_initrd(void)
>  		break;
>  	}
>  
> +out:
>  	initrd_gone = true;
>  
>  	return ret;
> -- 

Makes sense to me.

Acked-by: Borislav Petkov (AMD) <bp@alien8.de>
Greg KH Jan. 15, 2024, 6:35 p.m. UTC | #2 
On Mon, Jan 15, 2024 at 11:22:02AM +0100, Paolo Bonzini wrote:
> [ Upstream commit a7939f01672034a58ad3fdbce69bb6c665ce0024 ]

This really isn't this commit id, sorry.

> Builtin/initrd microcode will not be used the ucode loader is disabled.
> But currently, save_microcode_in_initrd is always performed and it
> accesses MSR_IA32_UCODE_REV even if dis_ucode_ldr is true, and in
> particular even if X86_FEATURE_HYPERVISOR is set; the TDX module does not
> implement the MSR and the result is a call trace at boot for TDX guests.
> 
> Mainline Linux fixed this as part of a more complex rework of microcode
> caching that went into 6.7 (see in particular commits dd5e3e3ca6,
> "x86/microcode/intel: Simplify early loading"; and a7939f0167203,
> "x86/microcode/amd: Cache builtin/initrd microcode early").  Do the bare
> minimum in stable kernels, setting initrd_gone just like mainline Linux
> does in mark_initrd_gone().

Why can't we take the changes in 6.7?  Doing a one-off almost always
causes problems :(

What exact commits are needed?

thanks,

greg
Paolo Bonzini Jan. 15, 2024, 6:54 p.m. UTC | #3 
On Mon, Jan 15, 2024 at 7:35 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Jan 15, 2024 at 11:22:02AM +0100, Paolo Bonzini wrote:
> > [ Upstream commit a7939f01672034a58ad3fdbce69bb6c665ce0024 ]
>
> This really isn't this commit id, sorry.

True, that's the point of the mainline kernel where the logic most
closely resembles the patch. stable-kernel-rules.rst does not quite
say what to do in this case.

> > Builtin/initrd microcode will not be used the ucode loader is disabled.
> > But currently, save_microcode_in_initrd is always performed and it
> > accesses MSR_IA32_UCODE_REV even if dis_ucode_ldr is true, and in
> > particular even if X86_FEATURE_HYPERVISOR is set; the TDX module does not
> > implement the MSR and the result is a call trace at boot for TDX guests.
> >
> > Mainline Linux fixed this as part of a more complex rework of microcode
> > caching that went into 6.7 (see in particular commits dd5e3e3ca6,
> > "x86/microcode/intel: Simplify early loading"; and a7939f0167203,
> > "x86/microcode/amd: Cache builtin/initrd microcode early").  Do the bare
> > minimum in stable kernels, setting initrd_gone just like mainline Linux
> > does in mark_initrd_gone().
>
> Why can't we take the changes in 6.7?  Doing a one-off almost always
> causes problems :(

The series is
https://lore.kernel.org/all/20231002115506.217091296@linutronix.de/

+ fixes at
https://lore.kernel.org/lkml/20231010150702.495139089@linutronix.de/T/

for a total of 35 patches and 800 lines changed.

There is no individual patch that fixes the bug, because it wasn't
really intentional in those patches; it just came out naturally as a
consequence of cleaning up unnecessary code. It is fixed as of patch
7, so one possibility would be to apply patches 1-7 in the series
above but, especially for older kernels, that would be way more scary
than this three line patch. It literally says "if microcode won't be
loaded in the processor don't keep it in memory either".

Paolo

> What exact commits are needed?
>
> thanks,
>
> greg
>
Greg KH Jan. 15, 2024, 8:59 p.m. UTC | #4 
On Mon, Jan 15, 2024 at 07:54:59PM +0100, Paolo Bonzini wrote:
> On Mon, Jan 15, 2024 at 7:35 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Mon, Jan 15, 2024 at 11:22:02AM +0100, Paolo Bonzini wrote:
> > > [ Upstream commit a7939f01672034a58ad3fdbce69bb6c665ce0024 ]
> >
> > This really isn't this commit id, sorry.
> 
> True, that's the point of the mainline kernel where the logic most
> closely resembles the patch. stable-kernel-rules.rst does not quite
> say what to do in this case.

Ok, then just say, "this is not upstream" and the rest of your changelog
is good.  I'll edit it up tomorrow and apply it, thanks.

greg k-h
Greg KH Jan. 17, 2024, 11 a.m. UTC | #5 
On Mon, Jan 15, 2024 at 09:59:23PM +0100, Greg KH wrote:
> On Mon, Jan 15, 2024 at 07:54:59PM +0100, Paolo Bonzini wrote:
> > On Mon, Jan 15, 2024 at 7:35 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Mon, Jan 15, 2024 at 11:22:02AM +0100, Paolo Bonzini wrote:
> > > > [ Upstream commit a7939f01672034a58ad3fdbce69bb6c665ce0024 ]
> > >
> > > This really isn't this commit id, sorry.
> > 
> > True, that's the point of the mainline kernel where the logic most
> > closely resembles the patch. stable-kernel-rules.rst does not quite
> > say what to do in this case.
> 
> Ok, then just say, "this is not upstream" and the rest of your changelog
> is good.  I'll edit it up tomorrow and apply it, thanks.

Ok, now queued up for 6.6.y, but what about older kernel versions?  Is
this relevant for them as well?  I will note that it does seem to apply
to them properly and build, is it not relevant?

thanks,

greg k-h
Paolo Bonzini Jan. 17, 2024, 11:57 a.m. UTC | #6 
On Wed, Jan 17, 2024 at 12:00 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Jan 15, 2024 at 09:59:23PM +0100, Greg KH wrote:
> > On Mon, Jan 15, 2024 at 07:54:59PM +0100, Paolo Bonzini wrote:
> > > On Mon, Jan 15, 2024 at 7:35 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Mon, Jan 15, 2024 at 11:22:02AM +0100, Paolo Bonzini wrote:
> > > > > [ Upstream commit a7939f01672034a58ad3fdbce69bb6c665ce0024 ]
> > > >
> > > > This really isn't this commit id, sorry.
> > >
> > > True, that's the point of the mainline kernel where the logic most
> > > closely resembles the patch. stable-kernel-rules.rst does not quite
> > > say what to do in this case.
> >
> > Ok, then just say, "this is not upstream" and the rest of your changelog
> > is good.  I'll edit it up tomorrow and apply it, thanks.
>
> Ok, now queued up for 6.6.y, but what about older kernel versions?

6.6 is where I tested that it works, and I didn't want to put an old
kernel version in the "Cc" line, without even testing that a
non-upstream patch applies there.

The benefit would be absolutely marginal. People playing with Intel
TDX are not going to use old kernels (6.1 counts as old) anyway, for
example support for lazy acceptance of memory went into 6.5.

Paolo
Greg KH Jan. 17, 2024, 12:08 p.m. UTC | #7 
On Wed, Jan 17, 2024 at 12:57:40PM +0100, Paolo Bonzini wrote:
> On Wed, Jan 17, 2024 at 12:00 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Mon, Jan 15, 2024 at 09:59:23PM +0100, Greg KH wrote:
> > > On Mon, Jan 15, 2024 at 07:54:59PM +0100, Paolo Bonzini wrote:
> > > > On Mon, Jan 15, 2024 at 7:35 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Mon, Jan 15, 2024 at 11:22:02AM +0100, Paolo Bonzini wrote:
> > > > > > [ Upstream commit a7939f01672034a58ad3fdbce69bb6c665ce0024 ]
> > > > >
> > > > > This really isn't this commit id, sorry.
> > > >
> > > > True, that's the point of the mainline kernel where the logic most
> > > > closely resembles the patch. stable-kernel-rules.rst does not quite
> > > > say what to do in this case.
> > >
> > > Ok, then just say, "this is not upstream" and the rest of your changelog
> > > is good.  I'll edit it up tomorrow and apply it, thanks.
> >
> > Ok, now queued up for 6.6.y, but what about older kernel versions?
> 
> 6.6 is where I tested that it works, and I didn't want to put an old
> kernel version in the "Cc" line, without even testing that a
> non-upstream patch applies there.
> 
> The benefit would be absolutely marginal. People playing with Intel
> TDX are not going to use old kernels (6.1 counts as old) anyway, for
> example support for lazy acceptance of memory went into 6.5.

Fair enough, thanks for the explaination, I'll leave it alone then.

greg k-h
diff mbox series

Patch

diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 35d39a13dc90..503b5da56685 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -214,6 +214,11 @@  static int __init save_microcode_in_initrd(void)
 	struct cpuinfo_x86 *c = &boot_cpu_data;
 	int ret = -EINVAL;
 
+	if (dis_ucode_ldr) {
+		ret = 0;
+		goto out;
+	}
+
 	switch (c->x86_vendor) {
 	case X86_VENDOR_INTEL:
 		if (c->x86 >= 6)
@@ -227,6 +230,7 @@  static int __init save_microcode_in_initrd(void)
 		break;
 	}
 
+out:
 	initrd_gone = true;
 
 	return ret;