[v3] x86/acpi: fix panic while AP online later with kernel parameter maxcpus=1

The issue was found on the platform that using "Multiprocessor Wakeup
Structure"[1] to startup secondary CPU, which is usually used by
encrypted guest.  When restrict boot time CPU to 1 with the kernel
parameter "maxcpus=1" and bring other CPUs online later, there will be
a kernel panic.

The variable acpi_mp_wake_mailbox, which holds the virtual address of
the MP Wakeup Structure mailbox, will be set as read-only after init.
If the first AP gets online later, after init, the attempt to update
the variable results in panic.

The memremap() call that initializes the variable cannot be moved into
acpi_parse_mp_wake() because memremap() is not functional at that point
in the boot process.

[1] Details about the MP Wakeup structure can be found in ACPI v6.4, in
    the "Multiprocessor Wakeup Structure" section.

Signed-off-by: Zhiquan Li <zhiquan1.li@intel.com>
Reviewed-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

---

V2: https://lore.kernel.org/all/20240628082119.357735-1-zhiquan1.li@intel.com/

Changes since V2:
- Modify the commit log as suggested by Kirill.
- Add Kirill's Reviewed-by tag.

V1: https://lore.kernel.org/all/20240626073920.176471-1-zhiquan1.li@intel.com/

Changes since V1:
- Amend the commit message as per Kirill's comments.
---
 arch/x86/kernel/acpi/madt_wakeup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


base-commit: ec6574a634db84f25e4eee6698d76fed5649e3bd

On Tue, Jul 02, 2024 at 12:05:38PM +0000, Huang, Kai wrote:
> On Tue, 2024-07-02 at 08:58 +0800, Zhiquan Li wrote:
> > The issue was found on the platform that using "Multiprocessor Wakeup
> > Structure"[1] to startup secondary CPU, which is usually used by
> > encrypted guest.  When restrict boot time CPU to 1 with the kernel
> > parameter "maxcpus=1" and bring other CPUs online later, there will be
> > a kernel panic.
> > 
> > The variable acpi_mp_wake_mailbox, which holds the virtual address of
> > the MP Wakeup Structure mailbox, will be set as read-only after init.
> > If the first AP gets online later, after init, the attempt to update
> > the variable results in panic.
> > 
> > The memremap() call that initializes the variable cannot be moved into
> > acpi_parse_mp_wake() because memremap() is not functional at that point
> > in the boot process.
> > 
> > [1] Details about the MP Wakeup structure can be found in ACPI v6.4, in
> >     the "Multiprocessor Wakeup Structure" section.
> > 
> > Signed-off-by: Zhiquan Li <zhiquan1.li@intel.com>
> > Reviewed-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> 
> Seems this changelog only mentions the problem, but doesn't say how to fix:
> 
>   Remove the __ro_after_init annotation of acpi_mp_wake_mailbox to fix.

Do not talk about *what* the patch is doing in the commit message - that
should be obvious from the diff itself. Rather, concentrate on the *why*
it needs to be done.

Imagine one fine day you're doing git archeology, you find the place in
the code about which you want to find out why it was changed the way it 
is now.

You do git annotate <filename> ... find the line, see the commit id and
you do:

git show <commit id>

You read the commit message and there's just gibberish and nothing's
explaining *why* that change was done. And you start scratching your
head, trying to figure out why. Because the damn commit message is worth
sh*t.

This happens to us maintainers at least once a week. Well, I don't want
that to happen in my tree anymore.

So none of this text above still doesn't explain to me *why* this is
happening.

Why do APs need to update acpi_mp_wake_mailbox?

Which patch is this fixing?

See Documentation/process/submitting-patches.rst

Questions over questions...

On Tue, 2024-07-02 at 14:45 +0200, Borislav Petkov wrote:
> On Tue, Jul 02, 2024 at 12:05:38PM +0000, Huang, Kai wrote:
> > On Tue, 2024-07-02 at 08:58 +0800, Zhiquan Li wrote:
> > > The issue was found on the platform that using "Multiprocessor Wakeup
> > > Structure"[1] to startup secondary CPU, which is usually used by
> > > encrypted guest.  When restrict boot time CPU to 1 with the kernel
> > > parameter "maxcpus=1" and bring other CPUs online later, there will be
> > > a kernel panic.
> > > 
> > > The variable acpi_mp_wake_mailbox, which holds the virtual address of
> > > the MP Wakeup Structure mailbox, will be set as read-only after init.
> > > If the first AP gets online later, after init, the attempt to update
> > > the variable results in panic.
> > > 
> > > The memremap() call that initializes the variable cannot be moved into
> > > acpi_parse_mp_wake() because memremap() is not functional at that point
> > > in the boot process.
> > > 
> > > [1] Details about the MP Wakeup structure can be found in ACPI v6.4, in
> > >     the "Multiprocessor Wakeup Structure" section.
> > > 
> > > Signed-off-by: Zhiquan Li <zhiquan1.li@intel.com>
> > > Reviewed-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > 
> > Seems this changelog only mentions the problem, but doesn't say how to fix:
> > 
> >   Remove the __ro_after_init annotation of acpi_mp_wake_mailbox to fix.
> 
> Do not talk about *what* the patch is doing in the commit message - that
> should be obvious from the diff itself. Rather, concentrate on the *why*
> it needs to be done.
> 
> Imagine one fine day you're doing git archeology, you find the place in
> the code about which you want to find out why it was changed the way it 
> is now.
> 
> You do git annotate <filename> ... find the line, see the commit id and
> you do:
> 
> git show <commit id>
> 
> You read the commit message and there's just gibberish and nothing's
> explaining *why* that change was done. And you start scratching your
> head, trying to figure out why. Because the damn commit message is worth
> sh*t.

Yeah fully agree.  Thanks for saying this again.

> 
> This happens to us maintainers at least once a week. Well, I don't want
> that to happen in my tree anymore.
> 
> So none of this text above still doesn't explain to me *why* this is
> happening.
> 
> Why do APs need to update acpi_mp_wake_mailbox?

They don't need to if acpi_mp_wake_mailbox can be setup before smp_init()
once for all.

But currently the setup of acpi_mp_wake_mailbox is done when the first AP is
brought up because memremap() doesn't work in acpi_parse_mp_wake(), as
mentioned in the changelog of this patch.

I also feel it's not ideal to setup acpi_mp_wake_mailbox when bringing up
the first AP, so I provided my diff.  IIUC, if memremap() works for
acpi_mp_wake_mailbox when bringing up the first AP, then it should also work
in
the early_initcall().

> 
> Which patch is this fixing?

It fiexes below commit AFAICT:

  24dd05da8c79 ("x86/apic: Mark acpi_mp_wake_* variables as
__ro_after_init")

Which didn't consider 'maxvcpus=xx' case.


But I will leave to Kirill to confirm.

On 2024/7/3 07:55, Huang, Kai wrote:
>> This happens to us maintainers at least once a week. Well, I don't want
>> that to happen in my tree anymore.
>>
>> So none of this text above still doesn't explain to me *why* this is
>> happening.
>>
>> Why do APs need to update acpi_mp_wake_mailbox?

Not AP needs to update acpi_mp_wake_mailbox, but BSP might need to
update it after the init stage.  In the encrypted guest CPU hot-plug
scenario, BSP memremap() the acpi_mp_wake_mailbox_paddr, and writes APIC
ID of APs, wakeup vector and the ACPI_MP_WAKE_COMMAND_WAKEUP command
into mailbox.  Firmware will listen on mailbox command address, and once
it receives the wakeup command, the CPU associated with the given apicid
will be booted.

We cannot assume that all APs will be brought up in the init stage.

> They don't need to if acpi_mp_wake_mailbox can be setup before smp_init()
> once for all.
> 
> But currently the setup of acpi_mp_wake_mailbox is done when the first AP is
> brought up because memremap() doesn't work in acpi_parse_mp_wake(), as
> mentioned in the changelog of this patch.
> 
> I also feel it's not ideal to setup acpi_mp_wake_mailbox when bringing up
> the first AP, so I provided my diff.  IIUC, if memremap() works for
> acpi_mp_wake_mailbox when bringing up the first AP, then it should also work
> in
> the early_initcall().

Besides the factor that whether memremap() is functional at the point in
the boot process, another reason I can think of is, if the intention is
just to work with BSP, then the remapping is a redundant step.
Especially in the kexec & kdump case, the capture kernel only needs
single CPU to work usually with the "maxcpus=1" option.

IMHO, the solution that postpone the remapping while really needs to
bring up APs is reasonable, just don't make acpi_mp_wake_mailbox
read-only.  The APs might be brought up later, might be never.


> 
>> Which patch is this fixing?
> It fiexes below commit AFAICT:
> 
>   24dd05da8c79 ("x86/apic: Mark acpi_mp_wake_* variables as
> __ro_after_init")
> 
> Which didn't consider 'maxvcpus=xx' case.
> 

Thanks a lot for checking this, Kai.

> 
> But I will leave to Kirill to confirm.

Best Regards,
Zhiquan

On July 3, 2024 4:39:43 AM GMT+02:00, Zhiquan Li <zhiquan1.li@intel.com> wrote:
>
>On 2024/7/3 07:55, Huang, Kai wrote:
>>> This happens to us maintainers at least once a week. Well, I don't want
>>> that to happen in my tree anymore.
>>>
>>> So none of this text above still doesn't explain to me *why* this is
>>> happening.
>>>
>>> Why do APs need to update acpi_mp_wake_mailbox?
>
>Not AP needs to update acpi_mp_wake_mailbox, but BSP might need to
>update it after the init stage.  In the encrypted guest CPU hot-plug
>scenario, BSP memremap() the acpi_mp_wake_mailbox_paddr, and writes APIC
>ID of APs, wakeup vector and the ACPI_MP_WAKE_COMMAND_WAKEUP command
>into mailbox.  Firmware will listen on mailbox command address, and once
>it receives the wakeup command, the CPU associated with the given apicid
>will be booted.
>
>We cannot assume that all APs will be brought up in the init stage.
>
>> They don't need to if acpi_mp_wake_mailbox can be setup before smp_init()
>> once for all.
>> 
>> But currently the setup of acpi_mp_wake_mailbox is done when the first AP is
>> brought up because memremap() doesn't work in acpi_parse_mp_wake(), as
>> mentioned in the changelog of this patch.
>> 
>> I also feel it's not ideal to setup acpi_mp_wake_mailbox when bringing up
>> the first AP, so I provided my diff.  IIUC, if memremap() works for
>> acpi_mp_wake_mailbox when bringing up the first AP, then it should also work
>> in
>> the early_initcall().
>
>Besides the factor that whether memremap() is functional at the point in
>the boot process, another reason I can think of is, if the intention is
>just to work with BSP, then the remapping is a redundant step.
>Especially in the kexec & kdump case, the capture kernel only needs
>single CPU to work usually with the "maxcpus=1" option.
>
>IMHO, the solution that postpone the remapping while really needs to
>bring up APs is reasonable, just don't make acpi_mp_wake_mailbox
>read-only.  The APs might be brought up later, might be never.
>
>
>> 
>>> Which patch is this fixing?
>> It fiexes below commit AFAICT:
>> 
>>   24dd05da8c79 ("x86/apic: Mark acpi_mp_wake_* variables as
>> __ro_after_init")
>> 
>> Which didn't consider 'maxvcpus=xx' case.
>> 
>
>Thanks a lot for checking this, Kai.
>
>> 
>> But I will leave to Kirill to confirm.
>
>Best Regards,
>Zhiquan

Then please extend the commit message with the "why", add the Fixes tag and resend.

Thx.