lkml.org 
[lkml]   [2023]   [Oct]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH 01/13] x86/acpi: Extract ACPI MADT wakeup code into a separate file
    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@linux.intel.com>
    > > ---
    > > 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

    \
     
     \ /
      Last update: 2023-10-06 14:01    [W:3.190 / U:0.032 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site