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

["Huang, Kai" <kai.huang@intel.com>]

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.

[...]

> 
>  /* Virtual address of the Multiprocessor Wakeup Structure mailbox */
> -static struct acpi_madt_multiproc_wakeup_mailbox *acpi_mp_wake_mailbox __ro_after_init;
> +static struct acpi_madt_multiproc_wakeup_mailbox *acpi_mp_wake_mailbox;
>  
> 

But if memremap() isn't available in acpi_parse_mp_wake() is the concern, could
we change to do it before smp_init() but after memremap() is available?

E.g, we should be able to use early_initcall() (only compile tested):

diff --git a/arch/x86/kernel/acpi/madt_wakeup.c
b/arch/x86/kernel/acpi/madt_wakeup.c
index 6cfe762be28b..ca0aea0832ac 100644
--- a/arch/x86/kernel/acpi/madt_wakeup.c
+++ b/arch/x86/kernel/acpi/madt_wakeup.c
@@ -176,18 +176,6 @@ static int acpi_wakeup_cpu(u32 apicid, unsigned long
start_ip)
                return -EOPNOTSUPP;
        }
 
-       /*
-        * Remap mailbox memory only for the first call to acpi_wakeup_cpu().
-        *
-        * Wakeup of secondary CPUs is fully serialized in the core code.
-        * No need to protect acpi_mp_wake_mailbox from concurrent accesses.
-        */
-       if (!acpi_mp_wake_mailbox) {
-               acpi_mp_wake_mailbox = memremap(acpi_mp_wake_mailbox_paddr,
-                                               sizeof(*acpi_mp_wake_mailbox),
-                                               MEMREMAP_WB);
-       }
-
        /*
         * Mailbox memory is shared between the firmware and OS. Firmware will
         * listen on mailbox command address, and once it receives the wakeup
@@ -290,3 +278,29 @@ int __init acpi_parse_mp_wake(union acpi_subtable_headers
*header,
 
        return 0;
 }
+
+static int __init acpi_mp_wake_map_mailbox(void)
+{
+       /*
+        * Nothing to do if the "Multiprocessor Wakeup Structure" is
+        * not present.  acpi_mp_wake_mailbox_paddr could also be 0
+        * if the kernel is from kexec.
+        */
+       if (!acpi_mp_wake_mailbox_paddr)
+               return 0;
+
+       /* memremap() isn't available in acpi_parse_mp_wake() */
+       acpi_mp_wake_mailbox = memremap(acpi_mp_wake_mailbox_paddr,
+                                       sizeof(*acpi_mp_wake_mailbox),
+                                       MEMREMAP_WB);
+       /*
+        * When memremap() fails, clear acpi_mp_wake_mailbox_paddr
+        * to prevent NULL dereference of acpi_mp_wake_mailbox
+        * when bringing up secondary CPUs.
+        */
+       if (WARN_ON(!acpi_mp_wake_mailbox))
+               acpi_mp_wake_mailbox_paddr = 0;
+
+       return 0;
+}
+early_initcall(acpi_mp_wake_map_mailbox);