On July 3, 2024 4:39:43 AM GMT+02:00, Zhiquan Li <zhiquan1.li@xxxxxxxxx> 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.
--
Sent from a small device: formatting sucks and brevity is inevitable.
