Re: [PATCH 06/21] x86: microcode: Convert to hotplug state machine

[Sebastian Andrzej Siewior]

On 2016-09-07 13:36:40 [+0200], Borislav Petkov wrote:
> You mean this:
> 
>         /* The CPU refused to come up during a system resume */
>         if (action == CPU_UP_CANCELED_FROZEN)
>                 microcode_fini_cpu(cpu);
correct.
> ?
> 
> It clears internal state, i.e., invalidates the current microcode patch.

it cleans the memory on Intel but not on AMD

> > diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
> > index df04b2d033f6..4fc67b51e22e 100644
> > --- a/arch/x86/kernel/cpu/microcode/core.c
> > +++ b/arch/x86/kernel/cpu/microcode/core.c
> 
> ...
> 
> > -static struct notifier_block mc_cpu_notifier = {
> > -	.notifier_call	= mc_cpu_callback,
> > -};
> > +static int mc_cpu_down_prep(unsigned int cpu)
> > +{
> > +	struct device *dev;
> > +
> > +	dev = get_cpu_device(cpu);
> > +	/* Suspend is in progress, only remove the interface */
> > +	sysfs_remove_group(&dev->kobj, &mc_attr_group);
> > +	pr_debug("CPU%d removed\n", cpu);
> > +	return 0;
> > +}
> > +
> > +static int mc_cpu_dead(unsigned int cpu)
> > +{
> > +#ifdef CONFIG_SMP
> > +	if (cpuhp_tasks_frozen)
> > +		microcode_fini_cpu(cpu);
> > +#endif
> > +	return 0;
> > +}
> 
> If this is corresponding to CPU_DEAD, then I'd like to point to that comment:
> 
>         /*
>          * case CPU_DEAD:
>          *
>          * When a CPU goes offline, don't free up or invalidate the copy of
>          * the microcode in kernel memory, so that we can reuse it when the
>          * CPU comes back online without unnecessarily requesting the userspace
>          * for it again.
>          */
> 
> IOW, you don't need mc_cpu_dead().
Okay. After a second look I would say so, too. On Intel we free memory
in this case but we don't set uci->valid back. Which means if the CPU
did not come up after resume we free the memory. If we try it (manually)
again and for some reason the CPU manages to get up, it will end up in
the ONLINE callback with no memory and uci->valid set.

I will prepare a patch with the DEAD state gone.

Sebastian