[v1,Part2,3/5] x86/microcode: Add a generic mechanism to declare support for minrev

  Intel microcode adds some meta-data to report a minimum required revision
before this new microcode can be safely late loaded. There are no generic
mechanism to declare support for all vendors.

Add generic support to microcode core to declare such support, this allows
late-loading to be permitted in those architectures that report support
for safe late loading.

Late loading has added support for

- New images declaring a required minimum base version before a late-load
  is performed.

Tainting only happens on architectures that don't support minimum required
version reporting.

Add a new variable in microcode_ops to allow an architecture to declare
support for safe microcode late loading.

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Cc: LKML <linux-kernel@vger.kernel.org>
Cc: x86 <x86@kernel.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Reinette Chatre <reinette.chatre@intel.com>
Cc: Thomas Gleixner (Intel) <tglx@linutronix.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Stefan Talpalaru <stefantalpalaru@yahoo.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Rafael J. Wysocki <rafael@kernel.org>
Cc: Peter Zilstra (Intel) <peterz@infradead.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>
---
 arch/x86/include/asm/microcode.h      |  2 ++
 arch/x86/kernel/cpu/microcode/core.c  | 25 ++++++++++++++++++++-----
 arch/x86/kernel/cpu/microcode/intel.c |  1 +
 arch/x86/Kconfig                      |  7 ++++---
 4 files changed, 27 insertions(+), 8 deletions(-)
  

Ashok!

On Fri, Jan 13 2023 at 09:29, Ashok Raj wrote:
> Intel microcode adds some meta-data to report a minimum required revision
> before this new microcode can be safely late loaded. There are no generic

s/this new microcode/a new microcode revision/

Changelogs are not restricted by twitter posting rules.

> mechanism to declare support for all vendors.
>
> Add generic support to microcode core to declare such support, this allows
> late-loading to be permitted in those architectures that report support
> for safe late loading.
>
> Late loading has added support for
>
> - New images declaring a required minimum base version before a late-load
>   is performed.
>
> Tainting only happens on architectures that don't support minimum required
> version reporting.
>
> Add a new variable in microcode_ops to allow an architecture to declare
> support for safe microcode late loading.
> @@ -487,13 +488,22 @@ static ssize_t reload_store(struct device *dev,
>  	if (ret)
>  		goto put;
>  
> +	safe_late_load = microcode_ops->safe_late_load;
> +
> +	/*
> +	 * If safe loading indication isn't present, bail out.
> +	 */
> +	if (!safe_late_load) {
> +		pr_err("Attempting late microcode loading - it is dangerous and taints the kernel.\n");
> +		pr_err("You should switch to early loading, if possible.\n");
> +		ret = -EINVAL;
> +		goto put;
> +	}
> +
>  	tmp_ret = microcode_ops->request_microcode_fw(bsp, &microcode_pdev->dev);
>  	if (tmp_ret != UCODE_NEW)
>  		goto put;
>  
> -	pr_err("Attempting late microcode loading - it is dangerous and taints the kernel.\n");
> -	pr_err("You should switch to early loading, if possible.\n");
> -

Why are you not moving the pr_err()s right away (in 1/5) to the place
where you move it now?

>  	mutex_lock(&microcode_mutex);
>  	ret = microcode_reload_late();
>  	mutex_unlock(&microcode_mutex);
> @@ -501,11 +511,16 @@ static ssize_t reload_store(struct device *dev,
>  put:
>  	cpus_read_unlock();
>  
> +	/*
> +	 * Only taint if a successful load and vendor doesn't support
> +	 * safe_late_load
> +	 */
> +	if (!(ret && safe_late_load))
> +		add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);

The resulting code is undecodable garbage. Whats worse is that the
existing logic in this code is broken already.

#1
	ssize_t ret = 0;

This 'ret = 0' assignment is pointless as ret is immediately overwritten
by the next line:

	ret = kstrtoul(buf, 0, &val);
	if (ret)
		return ret;

	if (val != 1)
		return size;

Now this is really useful. If the value is invalid, i.e. it causes the
function to abort immediately it returns 'size' which means the write
was successful. Oh well.

Now lets look at a few lines further down:

#2

	ssize_t ret = 0;
        ...
        ret = check_online_cpus();
	if (ret)
		goto put;
        ...
put:
        ...
	add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
        ...
        return ret;

Why are we tainting the kernel when there was absolutely ZERO action
done here? All what check_online_cpus() figured out was that not enough
CPUs were online, right? That justfies a error return, but the taint is
bogus, no?

The next bogosity is:

	ssize_t ret = 0;
        ...
        tmp_ret = microcode_ops->request_microcode_fw(bsp, &microcode_pdev->dev);
	if (tmp_ret != UCODE_NEW)
		goto put;
        ...    
put:
        ...
	add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);

	if (ret == 0)
		ret = size;

        return ret;

IOW, the microcode request can fail for whatever reason and the return
value is unconditionally 'size' which means the write to the sysfs file
is successfull.

#3

Not to talk about the completely broken error handling in the actual
microcode loading case in __reload_late()::wait_for_siblings code path.

Maybe more #...

How does any of this make sense and allows sensible scripting of this
interface?

Surely you spent several orders of magnitude more time to stare at this
code than I did during this review, no?

Now instead of noticing and fixing any of this nonsense you are duct
taping this whole safe_late_load handling into that mess to make it even
more incomprehensible.

If you expected an alternative patch here, then I have to disappoint
you.

I'm not presenting you the proper solution this time on a silver tablet
because I'm in the process of taming my 'let me fix this for you' reflex
to prepare for my retirement some years down the road.

But you should have enough hints to fix all of this for real, right?

Thanks,

        tglx
  

On Fri, Jan 20, 2023 at 01:15:04AM +0100, Thomas Gleixner wrote:
> Ashok!

I know I'm in trouble when it starts like this :-(

> 
> On Fri, Jan 13 2023 at 09:29, Ashok Raj wrote:
> > Intel microcode adds some meta-data to report a minimum required revision
> > before this new microcode can be safely late loaded. There are no generic
> 
> s/this new microcode/a new microcode revision/
> 
> Changelogs are not restricted by twitter posting rules.

:-)

> 
> > mechanism to declare support for all vendors.
> >
> > Add generic support to microcode core to declare such support, this allows
> > late-loading to be permitted in those architectures that report support
> > for safe late loading.
> >
> > Late loading has added support for
> >
> > - New images declaring a required minimum base version before a late-load
> >   is performed.
> >
> > Tainting only happens on architectures that don't support minimum required
> > version reporting.
> >
> > Add a new variable in microcode_ops to allow an architecture to declare
> > support for safe microcode late loading.
> > @@ -487,13 +488,22 @@ static ssize_t reload_store(struct device *dev,
> >  	if (ret)
> >  		goto put;
> >  
> > +	safe_late_load = microcode_ops->safe_late_load;
> > +
> > +	/*
> > +	 * If safe loading indication isn't present, bail out.
> > +	 */
> > +	if (!safe_late_load) {
> > +		pr_err("Attempting late microcode loading - it is dangerous and taints the kernel.\n");
> > +		pr_err("You should switch to early loading, if possible.\n");
> > +		ret = -EINVAL;
> > +		goto put;
> > +	}
> > +
> >  	tmp_ret = microcode_ops->request_microcode_fw(bsp, &microcode_pdev->dev);
> >  	if (tmp_ret != UCODE_NEW)
> >  		goto put;
> >  
> > -	pr_err("Attempting late microcode loading - it is dangerous and taints the kernel.\n");
> > -	pr_err("You should switch to early loading, if possible.\n");
> > -
> 
> Why are you not moving the pr_err()s right away (in 1/5) to the place
> where you move it now?

Could have, didn't occur then. But I can move them to the proper place in
patch1.

> 
> >  	mutex_lock(&microcode_mutex);
> >  	ret = microcode_reload_late();
> >  	mutex_unlock(&microcode_mutex);
> > @@ -501,11 +511,16 @@ static ssize_t reload_store(struct device *dev,
> >  put:
> >  	cpus_read_unlock();
> >  
> > +	/*
> > +	 * Only taint if a successful load and vendor doesn't support
> > +	 * safe_late_load
> > +	 */
> > +	if (!(ret && safe_late_load))
> > +		add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
> 
> The resulting code is undecodable garbage. Whats worse is that the
> existing logic in this code is broken already.

Yes, I agree, its hard to comprehend. I'll open it up a little to it makes
sense.

if successfully loaded, and !safe_late_load
       add_taint()


> 
> #1
> 	ssize_t ret = 0;
> 
> This 'ret = 0' assignment is pointless as ret is immediately overwritten
> by the next line:

This was existing code, but I can certainly remove the unneeded
initialization.

> 
> 	ret = kstrtoul(buf, 0, &val);
> 	if (ret)
> 		return ret;
> 
> 	if (val != 1)
> 		return size;
> 
> Now this is really useful. If the value is invalid, i.e. it causes the
> function to abort immediately it returns 'size' which means the write
> was successful. Oh well.

Yes, its a bit awkward. This is how its been forever. 

I wasn't sure if the purpose was values other than 1 don't throw error, so
it could be used to accommodate some extended functionality say "echo X"
in the future.

I'm not suggesting such use :-), but thought that maybe the reason to not
report error. 

If its acceptable to return like -EINVAL or something we could do that, so
there is some error user can catch in user space.

> 
> Now lets look at a few lines further down:
> 
> #2
> 
> 	ssize_t ret = 0;
>         ...
>         ret = check_online_cpus();
> 	if (ret)
> 		goto put;
>         ...
> put:
>         ...
> 	add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
>         ...
>         return ret;
> 
> Why are we tainting the kernel when there was absolutely ZERO action
> done here? All what check_online_cpus() figured out was that not enough
> CPUs were online, right? That justfies a error return, but the taint is
> bogus, no?

Agree!

This was the code that was introduced in 5.19 when we turned off
late-loading in the kernel. We try to fix it here, i.e only taint if the
loading was successful and safe_late_load wasn't set.

It should change after this patch? Or maybe you meant fix it to not taint
always before doing this change? 

> 
> The next bogosity is:
> 
> 	ssize_t ret = 0;
>         ...
>         tmp_ret = microcode_ops->request_microcode_fw(bsp, &microcode_pdev->dev);
> 	if (tmp_ret != UCODE_NEW)
> 		goto put;
>         ...    
> put:
>         ...
> 	add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
> 
> 	if (ret == 0)
> 		ret = size;
> 
>         return ret;
> 
> IOW, the microcode request can fail for whatever reason and the return
> value is unconditionally 'size' which means the write to the sysfs file
> is successfull.

Loading can fail for some known reasons

- No file found
- File is either same or older rev than loaded

Should we return proper codes? Certainly can, but since this has been
around all this time, I'm worried someone who depends on this working this
way will now see failures when it didn't in the past.

> 
> #3
> 
> Not to talk about the completely broken error handling in the actual
> microcode loading case in __reload_late()::wait_for_siblings code path.
> 
> Maybe more #...

I'll need to stare at it more than I have .. 

If its busted, its not popping out.

It's a path that all CPUs go through for the exit rendezvous.

We let the secondary also do an apply_microcode(), just to update the
revision in the per-cpu structures. I could be wrong, but if we didn't
update the per-cpu rev, /proc/cpuinfo was reporting the old values. I
remember doing this way back in 2018 Spectre time.

Guess a multiple choice might be useful :-).. I'll keep looking though!

> 
> How does any of this make sense and allows sensible scripting of this
> interface?
> 
> Surely you spent several orders of magnitude more time to stare at this
> code than I did during this review, no?

Sadly yes!

> 
> Now instead of noticing and fixing any of this nonsense you are duct
> taping this whole safe_late_load handling into that mess to make it even
> more incomprehensible.

safe_late_loading didn't change any of the old algorithms for late-loading
itself. All I used it was a mechanism to inform the core that the vendor
supports some way to tell the minrev is comprehended. This doesn't change
any of the code paths we take for late-load.

When safe_late_load is supported, 

- we don't issue a warning, or taint the kernel.
- Vendor provides a way to check if the new microcode has the proper
  meta-data and honor that.

Did you have something more in mind?

> 
> If you expected an alternative patch here, then I have to disappoint
> you.

Disappointed .. No.. I'm glad this is coming up after 4 years. The next
Part3 that has the NMI handling sort of has something similar to hold HT
siblings in NMI before the update completes in primary. Better now than
late .. 

Thanks for all the direction.
> 
> I'm not presenting you the proper solution this time on a silver tablet
> because I'm in the process of taming my 'let me fix this for you' reflex
> to prepare for my retirement some years down the road.
> 
> But you should have enough hints to fix all of this for real, right?

Yes, once i can spot all those holes :-)

Cheers,
Ashok
  

Hi Thomas and Boris,

This mini series is in response to Thomas's feedback here[1].

I hope this addresses all/most of your concerns you raised in this thread.

Sorry if I missed any, drop more hints and I'll fix them up.

Patch3 needs an AMD change as well. I wasn't confident looking at the
source that was returning the patchid vs reading fromt he real MSR. I'll
check with Boris to confirm before I submit this again.

[1] https://lore.kernel.org/lkml/87y1pygiyf.ffs@tglx/

Now that the other patches are now staged in tip/x86/microcode,
this series applies on top of that.

Cheers,
Ashok

Ashok Raj (4):
  x86/microcode: Taint kernel only if microcode loading was successful
  x86/microcode: Report invalid writes to reload sysfs file
  x86/microcode/intel: Fix collect_cpu_info() to reflect current
    microcode
  x86/microcode: Do not call apply_microde() on sibling threads

 arch/x86/kernel/cpu/microcode/core.c  | 28 ++++++++++++++-------------
 arch/x86/kernel/cpu/microcode/intel.c | 10 +++++++++-
 2 files changed, 24 insertions(+), 14 deletions(-)

Cc: LKML <linux-kernel@vger.kernel.org>
Cc: x86 <x86@kernel.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Reinette Chatre <reinette.chatre@intel.com>
Cc: Thomas Gleixner (Intel) <tglx@linutronix.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Stefan Talpalaru <stefantalpalaru@yahoo.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Rafael J. Wysocki <rafael@kernel.org>
Cc: Peter Zilstra (Intel) <peterz@infradead.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Martin Pohlack <mpohlack@amazon.de>
  

Hi Thomas,

On Sat, Jan 21, 2023 at 01:35:08PM -0800, Ashok Raj wrote:
> Hi Thomas and Boris,
> 
> This mini series is in response to Thomas's feedback here[1].
> 
> I hope this addresses all/most of your concerns you raised in this thread.
> 
> Sorry if I missed any, drop more hints and I'll fix them up.
> 
> Patch3 needs an AMD change as well. I wasn't confident looking at the
> source that was returning the patchid vs reading fromt he real MSR. I'll
> check with Boris to confirm before I submit this again.
> 
> [1] https://lore.kernel.org/lkml/87y1pygiyf.ffs@tglx/

I can send you a new series with these cleanups. If you think I left
anything out please let me know.

Boris: Do you have any feedback on this Part2 of the path series? I can
wrap those as well with these that address Thomas's feedback if possible.

Thanks,
Ashok
  

On Thu, Jan 26, 2023 at 08:52:30AM -0800, Ashok Raj wrote:
> Boris: Do you have any feedback on this Part2 of the path series? I can
> wrap those as well with these that address Thomas's feedback if possible.

I have no clue what I need to look at first - the part 2 or the cleanup? And
what goes where.

I think you should send a proper, tested(!), new revision of the whole set for
review.

Thx.