Re: [PATCH v2 3/9] x86/dt: Support the ACPI multiprocessor wakeup for device tree

[Yunhong Jiang <yunhong.jiang@linux.intel.com>]

On Mon, Sep 02, 2024 at 03:35:06AM +0000, Michael Kelley wrote:
> From: Yunhong Jiang <yunhong.jiang@linux.intel.com> 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@linux.intel.com>
> > ---
> >  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
> > 
>