On Mon, Sep 02, 2024 at 03:35:06AM +0000, Michael Kelley wrote:
> From: Yunhong Jiang <yunhong.jiang@xxxxxxxxxxxxxxx> Sent: Friday, August 23, 2024 4:23 PM
> >
> > When a TDX guest boots with the device tree instead of ACPI, it can
> > reuse the ACPI multiprocessor wakeup mechanism to wake up application
> > processors(AP), without introducing a new mechanism from scrach.
> >
> > In the ACPI spec, two structures are defined to wake up the APs: the
> > multiprocessor wakeup structure and the multiprocessor wakeup mailbox
> > structure. The multiprocessor wakeup structure is passed to OS through a
> > Multiple APIC Description Table(MADT), one field specifying the physical
> > address of the multiprocessor wakeup mailbox structure. The OS sends
> > a message to firmware through the multiprocessor wakeup mailbox
> > structure, to bring up the APs.
> >
> > In device tree environment, the multiprocessor wakeup structure is not
> > used, to reduce the dependency on the generic ACPI table. The
> > information defined in this structure is defined in the properties of
> > cpus node in the device tree. The "wakeup-mailbox-addr" property
> > specifies the physical address of the multiprocessor wakeup mailbox
> > structure. The OS will follow the ACPI spec to send the message to the
> > firmware to bring up the APs.
> >
> > Signed-off-by: Yunhong Jiang <yunhong.jiang@xxxxxxxxxxxxxxx>
> > ---
> > MAINTAINERS | 1 +
> > arch/x86/Kconfig | 2 +-
> > arch/x86/include/asm/acpi.h | 1 -
> > arch/x86/include/asm/madt_wakeup.h | 16 +++++++++++++
> > arch/x86/kernel/madt_wakeup.c | 38 ++++++++++++++++++++++++++++++
> > 5 files changed, 56 insertions(+), 2 deletions(-)
> > create mode 100644 arch/x86/include/asm/madt_wakeup.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 5555a3bbac5f..900de6501eba 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -288,6 +288,7 @@ T: git
> > git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm
> > F: Documentation/ABI/testing/configfs-acpi
> > F: Documentation/ABI/testing/sysfs-bus-acpi
> > F: Documentation/firmware-guide/acpi/
> > +F: arch/x86/include/asm/madt_wakeup.h
> > F: arch/x86/kernel/acpi/
> > F: arch/x86/kernel/madt_playdead.S
> > F: arch/x86/kernel/madt_wakeup.c
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index d422247b2882..dba46dd30049 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -1123,7 +1123,7 @@ config X86_LOCAL_APIC
> > config ACPI_MADT_WAKEUP
> > def_bool y
> > depends on X86_64
> > - depends on ACPI
> > + depends on ACPI || OF
> > depends on SMP
> > depends on X86_LOCAL_APIC
> >
> > diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h
> > index 21bc53f5ed0c..0e082303ca26 100644
> > --- a/arch/x86/include/asm/acpi.h
> > +++ b/arch/x86/include/asm/acpi.h
> > @@ -83,7 +83,6 @@ union acpi_subtable_headers;
> > int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
> > const unsigned long end);
> >
> > -void asm_acpi_mp_play_dead(u64 reset_vector, u64 pgd_pa);
> >
> > /*
> > * Check if the CPU can handle C2 and deeper
> > diff --git a/arch/x86/include/asm/madt_wakeup.h
> > b/arch/x86/include/asm/madt_wakeup.h
> > new file mode 100644
> > index 000000000000..a8cd50af581a
> > --- /dev/null
> > +++ b/arch/x86/include/asm/madt_wakeup.h
> > @@ -0,0 +1,16 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __ASM_X86_MADT_WAKEUP_H
> > +#define __ASM_X86_MADT_WAKEUP_H
> > +
> > +void asm_acpi_mp_play_dead(u64 reset_vector, u64 pgd_pa);
> > +
> > +#if defined(CONFIG_OF) && defined(CONFIG_ACPI_MADT_WAKEUP)
> > +u64 dtb_parse_mp_wake(void);
> > +#else
> > +static inline u64 dtb_parse_mp_wake(void)
> > +{
> > + return 0;
> > +}
> > +#endif
> > +
> > +#endif /* __ASM_X86_MADT_WAKEUP_H */
> > diff --git a/arch/x86/kernel/madt_wakeup.c b/arch/x86/kernel/madt_wakeup.c
> > index d5ef6215583b..7257e7484569 100644
> > --- a/arch/x86/kernel/madt_wakeup.c
> > +++ b/arch/x86/kernel/madt_wakeup.c
> > @@ -14,6 +14,8 @@
> > #include <asm/nmi.h>
> > #include <asm/processor.h>
> > #include <asm/reboot.h>
> > +#include <asm/madt_wakeup.h>
> > +#include <asm/prom.h>
> >
> > /* Physical address of the Multiprocessor Wakeup Structure mailbox */
> > static u64 acpi_mp_wake_mailbox_paddr __ro_after_init;
> > @@ -122,6 +124,7 @@ static int __init init_transition_pgtable(pgd_t *pgd)
> > return 0;
> > }
> >
> > +#ifdef CONFIG_ACPI
> > static int __init acpi_mp_setup_reset(u64 reset_vector)
> > {
> > struct x86_mapping_info info = {
> > @@ -168,6 +171,7 @@ static int __init acpi_mp_setup_reset(u64 reset_vector)
> >
> > return 0;
> > }
> > +#endif
>
> When acpi_mp_setup_reset() is #ifdef'ed out because of CONFIG_ACPI
> not being set, doesn't this generate compile warnings about
> init_transition_pgtable(), alloc_pgt_page(), free_pgt_page(), etc. being
> unused?
>
> It appears that the only code in madt_wakeup.c that is shared between
> the ACPI and OF cases is acpi_wakeup_cpu()? Is that correct?
Yes, the acpi_wakeup_cpu() is the only code. Interestingly that I don't see
compilation warning for the functions you listed like
alloc_pgt_page()/free_pgt_page() etc when built with CONFIG_ACPI disabled.
Will check in deep and figure out the reason.
>
> >
> > static int acpi_wakeup_cpu(u32 apicid, unsigned long start_ip)
> > {
> > @@ -226,6 +230,7 @@ static int acpi_wakeup_cpu(u32 apicid, unsigned long start_ip)
> > return 0;
> > }
> >
> > +#ifdef CONFIG_ACPI
> > static void acpi_mp_disable_offlining(struct acpi_madt_multiproc_wakeup *mp_wake)
> > {
> > cpu_hotplug_disable_offlining();
> > @@ -290,3 +295,36 @@ int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
> >
> > return 0;
> > }
> > +#endif /* CONFIG_ACPI */
> > +
> > +#ifdef CONFIG_OF
> > +u64 __init dtb_parse_mp_wake(void)
> > +{
> > + struct device_node *node;
> > + u64 mailaddr = 0;
> > +
> > + node = of_find_node_by_path("/cpus");
> > + if (!node)
> > + return 0;
> > +
> > + if (of_property_match_string(node, "enable-method", "acpi-wakeup-mailbox") < 0) {
> > + pr_err("No acpi wakeup mailbox enable-method\n");
> > + goto done;
>
> Patch 4 of this series unconditionally calls dtb_parse_mp_wake(),
> so it will be called in normal VMs, as a well as SEV-SNP and TDX
> kernels built for VTL 2 (assuming CONFIG_OF is set). Normal VMs
> presumably won't be using DT and won't have the "/cpus" node,
> so this function will silently do nothing, which is fine. But do you
> expect the DT "/cpus" node to be present in an SEV-SNP VTL 2
> environment? If so, this function will either output some spurious
> error messages, or SEV-SNP will use the ACPI wakeup mailbox
> method, which is probably not what you want.
>
> Michael
Yes, will fix the spurios error messages. Thank you for pointing out this.
Thanks
--jyh
>
> > + }
> > +
> > + if (of_property_read_u64(node, "wakeup-mailbox-addr", &mailaddr)) {
> > + pr_err("Invalid wakeup mailbox addr\n");
> > + goto done;
> > + }
> > + acpi_mp_wake_mailbox_paddr = mailaddr;
> > + pr_info("dt wakeup-mailbox: addr 0x%llx\n", mailaddr);
> > +
> > + /* No support for the MADT reset vector yet */
> > + cpu_hotplug_disable_offlining();
> > + apic_update_callback(wakeup_secondary_cpu_64, acpi_wakeup_cpu);
> > +
> > +done:
> > + of_node_put(node);
> > + return mailaddr;
> > +}
> > +#endif /* CONFIG_OF */
> > --
> > 2.25.1
> >
>
