Re: [PATCH 01/13] x86/acpi: Extract ACPI MADT wakeup code into a separate file

[kirill.shutemov@xxxxxxxxxxxxxxx]

On Fri, Oct 06, 2023 at 10:22:54AM +0000, Huang, Kai wrote:
> On Thu, 2023-10-05 at 16:13 +0300, Kirill A. Shutemov wrote:
> > In order to prepare for the expansion of support for the ACPI MADT
> > wakeup method, the relevant code has been moved into a separate file.
> > A new configuration option has been introduced to clearly indicate
> > dependencies without the use of ifdefs.
> 
> Use imperative mood?  
> 
> - Move the relevant code into ...
> - Introduce a new configuration option to ...

Okay.

> > There have been no functional changes.
> > 
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
> > ---
> >  arch/x86/Kconfig                   |  7 +++
> >  arch/x86/include/asm/acpi.h        |  5 ++
> >  arch/x86/kernel/acpi/Makefile      | 11 ++--
> >  arch/x86/kernel/acpi/boot.c        | 86 +-----------------------------
> >  arch/x86/kernel/acpi/madt_wakeup.c | 80 +++++++++++++++++++++++++++
> >  5 files changed, 99 insertions(+), 90 deletions(-)
> >  create mode 100644 arch/x86/kernel/acpi/madt_wakeup.c
> > 
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index 3154dbc49cf5..7368d254d01f 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -1108,6 +1108,13 @@ config X86_LOCAL_APIC
> >  	depends on X86_64 || SMP || X86_32_NON_STANDARD || X86_UP_APIC || PCI_MSI
> >  	select IRQ_DOMAIN_HIERARCHY
> >  
> > +config X86_ACPI_MADT_WAKEUP
> > +	def_bool y
> > +	depends on X86_64
> > +	depends on ACPI
> > +	depends on SMP
> > +	depends on X86_LOCAL_APIC
> > +
> >  config X86_IO_APIC
> >  	def_bool y
> >  	depends on X86_LOCAL_APIC || X86_UP_IOAPIC
> > diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h
> > index c8a7fc23f63c..b536b5a6a57b 100644
> > --- a/arch/x86/include/asm/acpi.h
> > +++ b/arch/x86/include/asm/acpi.h
> > @@ -73,6 +73,11 @@ static inline bool acpi_skip_set_wakeup_address(void)
> >  
> >  #define acpi_skip_set_wakeup_address acpi_skip_set_wakeup_address
> >  
> > +union acpi_subtable_headers;
> > +
> > +int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
> > +			      const unsigned long end);
> > +
> >  /*
> >   * Check if the CPU can handle C2 and deeper
> >   */
> > diff --git a/arch/x86/kernel/acpi/Makefile b/arch/x86/kernel/acpi/Makefile
> > index fc17b3f136fe..8c7329c88a75 100644
> > --- a/arch/x86/kernel/acpi/Makefile
> > +++ b/arch/x86/kernel/acpi/Makefile
> > @@ -1,11 +1,12 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  
> > -obj-$(CONFIG_ACPI)		+= boot.o
> > -obj-$(CONFIG_ACPI_SLEEP)	+= sleep.o wakeup_$(BITS).o
> > -obj-$(CONFIG_ACPI_APEI)		+= apei.o
> > -obj-$(CONFIG_ACPI_CPPC_LIB)	+= cppc.o
> > +obj-$(CONFIG_ACPI)			+= boot.o
> > +obj-$(CONFIG_ACPI_SLEEP)		+= sleep.o wakeup_$(BITS).o
> > +obj-$(CONFIG_ACPI_APEI)			+= apei.o
> > +obj-$(CONFIG_ACPI_CPPC_LIB)		+= cppc.o
> > +obj-$(CONFIG_X86_ACPI_MADT_WAKEUP)	+= madt_wakeup.o
> >  
> >  ifneq ($(CONFIG_ACPI_PROCESSOR),)
> > -obj-y				+= cstate.o
> > +obj-y					+= cstate.o
> >  endif
> 
> unintended code change?

No. It keeps += aligned as they are before the patch.
> 
> [...]
> 
> > 
> > diff --git a/arch/x86/kernel/acpi/madt_wakeup.c b/arch/x86/kernel/acpi/madt_wakeup.c
> > new file mode 100644
> > index 000000000000..1b9747bfd5b9
> > --- /dev/null
> > +++ b/arch/x86/kernel/acpi/madt_wakeup.c
> > @@ -0,0 +1,80 @@
> > +#include <linux/acpi.h>
> > +#include <asm/apic.h>
> 
> Functions like memremap(), smp_store_release() and cpu_relax() are used in this
> file.  Should we explicitly include the relevant headers?

Okay, will do.

> > +
> > +/* Physical address of the Multiprocessor Wakeup Structure mailbox */
> > +static u64 acpi_mp_wake_mailbox_paddr;
> > +/* Virtual address of the Multiprocessor Wakeup Structure mailbox */
> > +static struct acpi_madt_multiproc_wakeup_mailbox *acpi_mp_wake_mailbox;
> > +
> > +static int acpi_wakeup_cpu(int apicid, unsigned long start_ip)
> > +{
> > +	/*
> > +	 * 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
> > +	 * command, the CPU associated with the given apicid will be booted.
> > +	 *
> > +	 * The value of 'apic_id' and 'wakeup_vector' must be visible to the
> > +	 * firmware before the wakeup command is visible.  smp_store_release()
> > +	 * ensures ordering and visibility.
> > +	 */
> > +	acpi_mp_wake_mailbox->apic_id	    = apicid;
> > +	acpi_mp_wake_mailbox->wakeup_vector = start_ip;
> > +	smp_store_release(&acpi_mp_wake_mailbox->command,
> > +			  ACPI_MP_WAKE_COMMAND_WAKEUP);
> > +
> > +	/*
> > +	 * Wait for the CPU to wake up.
> > +	 *
> > +	 * The CPU being woken up is essentially in a spin loop waiting to be
> > +	 * woken up. It should not take long for it wake up and acknowledge by
> > +	 * zeroing out ->command.
> > +	 *
> > +	 * ACPI specification doesn't provide any guidance on how long kernel
> > +	 * has to wait for a wake up acknowledgement. It also doesn't provide
> > +	 * a way to cancel a wake up request if it takes too long.
> > +	 *
> > +	 * In TDX environment, the VMM has control over how long it takes to
> > +	 * wake up secondary. It can postpone scheduling secondary vCPU
> > +	 * indefinitely. Giving up on wake up request and reporting error opens
> > +	 * possible attack vector for VMM: it can wake up a secondary CPU when
> > +	 * kernel doesn't expect it. Wait until positive result of the wake up
> > +	 * request.
> > +	 */
> > +	while (READ_ONCE(acpi_mp_wake_mailbox->command))
> > +		cpu_relax();
> > +
> > +	return 0;
> > +}
> > +
> > +int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
> > +			      const unsigned long end)
> > +{
> > +	struct acpi_madt_multiproc_wakeup *mp_wake;
> > +
> > +	if (!IS_ENABLED(CONFIG_SMP))
> > +		return -ENODEV;
> 
> Now you have made X86_ACPI_MADT_WAKEUP depend on SMP, and this file will only be
> compiled when  X86_ACPI_MADT_WAKEUP is turned on.  IIUC this essentially means
> IS_ENABLED(CONFIG_SMP) will always report true?

Good catch. I didn't have 'depends SMP' initially, but it triggered build
issue, so I added it, but forgot to drop IS_ENABLED().

> > +
> > +	mp_wake = (struct acpi_madt_multiproc_wakeup *)header;
> > +	if (BAD_MADT_ENTRY(mp_wake, end))
> > +		return -EINVAL;
> > +
> > +	acpi_table_print_madt_entry(&header->common);
> > +
> > +	acpi_mp_wake_mailbox_paddr = mp_wake->base_address;
> > +
> > +	apic_update_callback(wakeup_secondary_cpu_64, acpi_wakeup_cpu);
> > +
> > +	return 0;
> > +}
> 

-- 
  Kiryl Shutsemau / Kirill A. Shutemov