The issue was found on the platform that using "Multiprocessor Wakeup
Structure"[1] to startup secondary CPU, which is usually used by
encrypted guest.  Before waking up the APs, BSP should memremap() the
physical address of the MP Wakeup Structure mailbox to the variable
acpi_mp_wake_mailbox, which holds the virtual address of mailbox.  When
BSP needs to wake up the APs, it writes the APIC ID of APs, wakeup
vector, and the ACPI_MP_WAKE_COMMAND_WAKEUP command into the mailbox.

Current implementation doesn't consider the case that restricts boot
time CPU to 1 with the kernel parameter "maxcpus=1" and brings other
CPUs online later, the variable acpi_mp_wake_mailbox will be set as
read-only after init.  So when the first AP gets online 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.  Moreover, the APs might never be brought up, keep
the memremap() call in acpi_wakeup_cpu() so that the operation only
takes place on demand.

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

Fixes: 24dd05da8c79 ("x86/apic: Mark acpi_mp_wake_* variables as __ro_after_init")
Signed-off-by: Zhiquan Li <zhiquan1.li@intel.com>
Reviewed-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

---

V4 RESEND note:
- No changes on this, just rebasing as v6.11-rc1 is out.

V3: https://lore.kernel.org/all/20240702005800.622910-1-zhiquan1.li@intel.com/

Changes since V3:
- Add Fixes tag for the commit found by Kai.
- Extend the commit message for the root cause and solution.

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.
- Remove the oops message.
---
 arch/x86/kernel/acpi/madt_wakeup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/acpi/madt_wakeup.c b/arch/x86/kernel/acpi/madt_wakeup.c
index 6cfe762be28b..d5ef6215583b 100644
--- a/arch/x86/kernel/acpi/madt_wakeup.c
+++ b/arch/x86/kernel/acpi/madt_wakeup.c
@@ -19,7 +19,7 @@
 static u64 acpi_mp_wake_mailbox_paddr __ro_after_init;
 
 /* 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;
 
 static u64 acpi_mp_pgd __ro_after_init;
 static u64 acpi_mp_reset_vector_paddr __ro_after_init;

base-commit: 435dfff07e5b71ceecc35954645740ab91090fbb
-- 
2.25.1

On Mon, Aug 5, 2024 at 12:03 PM Zhiquan Li <zhiquan1.li@intel.com> 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.  Before waking up the APs, BSP should memremap() the
> physical address of the MP Wakeup Structure mailbox to the variable
> acpi_mp_wake_mailbox, which holds the virtual address of mailbox.  When
> BSP needs to wake up the APs, it writes the APIC ID of APs, wakeup
> vector, and the ACPI_MP_WAKE_COMMAND_WAKEUP command into the mailbox.
>
> Current implementation doesn't consider the case that restricts boot
> time CPU to 1 with the kernel parameter "maxcpus=1" and brings other
> CPUs online later, the variable acpi_mp_wake_mailbox will be set as
> read-only after init.  So when the first AP gets online 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.  Moreover, the APs might never be brought up, keep
> the memremap() call in acpi_wakeup_cpu() so that the operation only
> takes place on demand.
>
> [1] Details about the MP Wakeup structure can be found in ACPI v6.4, in
>     the "Multiprocessor Wakeup Structure" section.
>
> Fixes: 24dd05da8c79 ("x86/apic: Mark acpi_mp_wake_* variables as __ro_after_init")
> Signed-off-by: Zhiquan Li <zhiquan1.li@intel.com>
> Reviewed-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>
> V4 RESEND note:
> - No changes on this, just rebasing as v6.11-rc1 is out.
>
> V3: https://lore.kernel.org/all/20240702005800.622910-1-zhiquan1.li@intel.com/
>
> Changes since V3:
> - Add Fixes tag for the commit found by Kai.
> - Extend the commit message for the root cause and solution.
>
> 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.
> - Remove the oops message.
> ---
>  arch/x86/kernel/acpi/madt_wakeup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/acpi/madt_wakeup.c b/arch/x86/kernel/acpi/madt_wakeup.c
> index 6cfe762be28b..d5ef6215583b 100644
> --- a/arch/x86/kernel/acpi/madt_wakeup.c
> +++ b/arch/x86/kernel/acpi/madt_wakeup.c
> @@ -19,7 +19,7 @@
>  static u64 acpi_mp_wake_mailbox_paddr __ro_after_init;
>
>  /* 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;
>
>  static u64 acpi_mp_pgd __ro_after_init;
>  static u64 acpi_mp_reset_vector_paddr __ro_after_init;
>
> base-commit: 435dfff07e5b71ceecc35954645740ab91090fbb
> --
> 2.25.1
>

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     ab84ba647f2c94ac4d0c3fc6951c49f08aa1fcf7
Gitweb:        https://git.kernel.org/tip/ab84ba647f2c94ac4d0c3fc6951c49f08aa1fcf7
Author:        Zhiquan Li <zhiquan1.li@intel.com>
AuthorDate:    Mon, 05 Aug 2024 18:35:31 +08:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Wed, 07 Aug 2024 20:04:38 +02:00

x86/acpi: Remove __ro_after_init from acpi_mp_wake_mailbox

On a platform using the "Multiprocessor Wakeup Structure"[1] to startup
secondary CPUs the control processor needs to memremap() the physical
address of the MP Wakeup Structure mailbox to the variable
acpi_mp_wake_mailbox, which holds the virtual address of mailbox.

To wake up the AP the control processor writes the APIC ID of AP, the
wakeup vector and the ACPI_MP_WAKE_COMMAND_WAKEUP command into the mailbox.

Current implementation doesn't consider the case which restricts boot time
CPU bringup to 1 with the kernel parameter "maxcpus=1" and brings other
CPUs online later from user space as it sets acpi_mp_wake_mailbox to
read-only after init.  So when the first AP is tried to brought online
after init, the attempt to update the variable results in a kernel 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. Also as the APs might never be brought up, keep the
memremap() call in acpi_wakeup_cpu() so that the operation only takes place
when needed.

Fixes: 24dd05da8c79 ("x86/apic: Mark acpi_mp_wake_* variables as __ro_after_init")
Signed-off-by: Zhiquan Li <zhiquan1.li@intel.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Link: https://lore.kernel.org/all/20240805103531.1230635-1-zhiquan1.li@intel.com
---
 arch/x86/kernel/acpi/madt_wakeup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/acpi/madt_wakeup.c b/arch/x86/kernel/acpi/madt_wakeup.c
index 6cfe762..d5ef621 100644
--- a/arch/x86/kernel/acpi/madt_wakeup.c
+++ b/arch/x86/kernel/acpi/madt_wakeup.c
@@ -19,7 +19,7 @@
 static u64 acpi_mp_wake_mailbox_paddr __ro_after_init;
 
 /* 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;
 
 static u64 acpi_mp_pgd __ro_after_init;
 static u64 acpi_mp_reset_vector_paddr __ro_after_init;