Re: [PATCHv7 21/30] x86/acpi, x86/boot: Add multiprocessor wake-up support

[Kirill A. Shutemov]

On Wed, Mar 30, 2022 at 04:44:13PM -0700, Dave Hansen wrote:
> On 3/30/22 16:16, Kirill A. Shutemov wrote:
> > On Mon, Mar 28, 2022 at 12:17:35PM -0700, Dave Hansen wrote:
> >> Is there anything in the trampoline code or start_secondary() that would
> >> cause damage if it got run later than the kernel expects?
> > 
> > I didn't find anything specific.
> > 
> > But I tried to simulate similar scenario by returning -EIO just after
> > writing wake up command in acpi_wakeup_cpu(). System booted, but there is
> > a warning in the dmesg:
> > 
> > WARNING: CPU: 0 PID: 1 at kernel/irq/chip.c:210 irq_startup+0x2a3/0x2b0
> > 
> > System seems recovered fine, but I'm not sure what will happen if
> > wake up is delayed by VMM.
> 
> That shouldn't be too hard to simulate.  Just add a spin loop at the
> beginning of start_secondary() that can spin for grotesque amounts of
> time and have it get more grotesque with each CPU that boots.

I tried to play a bit with delay on start_secondary() side, but I failed
to find anything new.

> If you're still booting CPUs when userspace comes up, you've done as
> much damage as possible.
> 
> But, I do think there are two relatively discrete problems here:
> 1. How long do we wait for a secondary CPU to actually ack the MADT
>    wakeup?  (the answer might be "don't wait").

"don't wait" actually leads to wait in do_boot_cpu(): successful wake up
call leads to 10s wait for sign of life from the secondary CPU.

> 2. How do we verify that a woken-up CPU actually booted all the way?
> 
> Even if #1 is a loooooong time, it might get stalled before it fully boots.

Right. "booted all the way" hard to define. It can be cut off at any
point by VMM.
> 
> So, for this patch, let's just remove the timeout.  If the boot CPU
> hangs (spins forever) because the VMM is being naughty, I'm OK with it.
>  It's hard to do too much damage when you're spinning.
> 
> #2 seems like a separate issue to tackle.  Maybe it's as simple as
> waiting for the secondary CPU to mark itself as online.

As I mentioned above, do_boot_cpu() already does this. I don't think we
can anything sensible beyond that.

> >> It looks like debugging that you add when you're bringing something up.
> >>  I'm not sure of its utility going forward.  I'd just zap it for now and
> >> bring it back later if it's really needed.
> > 
> > Sathya pointed out that this protection was added based on request[1] by
> > Rafael.
> > 
> > TDX should be safe from re-wakeups as we forbid offlining CPUs, but the
> > wake up method suppose to be generic, not limited to TDX.
> > 
> > [1] https://lore.kernel.org/all/CAJZ5v0jonk2Pb2HUHMoe69vnCV4+EV9XZpo2LwKrnYPF3CxD_A@xxxxxxxxxxxxxx/
> 
> Fair enough.  Please just add that reasoning somewhere.

Looking closer, the protections seems to be provided by caller.

apic->wakeup_secondary_cpu_64() is only called from do_boot_cpu() which is
only called from native_cpu_up(). It checks the CPU against cpu_callin_mask
before trying to wake it up, so we don't need to do this here. 

I'm dropping this protection after all.

Does the patch below look okay?