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

[Zhiquan Li <zhiquan1.li@intel.com>]

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