[PATCHv2,13/13] x86/acpi: Add support for CPU offlining for ACPI MADT wakeup method
Commit Message
MADT Multiprocessor Wakeup structure version 1 brings support of CPU
offlining: BIOS provides a reset vector where the CPU has to jump to
offline itself. The new TEST mailbox command can be used to test the CPU
offlined successfully and BIOS has control over it.
Add CPU offling support for ACPI MADT wakeup method by implementing
custom cpu_die, play_dead and stop_other_cpus SMP operations.
CPU offlining makes possible to hand over secondary CPUs over kexec, not
limiting the second kernel with single CPU.
The change conforms to the approved ACPI spec change proposal. See the
Link.
Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Link: https://lore.kernel.org/all/13356251.uLZWGnKmhe@kreacher
---
arch/x86/kernel/acpi/Makefile | 2 +-
arch/x86/kernel/acpi/boot.c | 2 +
arch/x86/kernel/acpi/madt.S | 24 ++++
arch/x86/kernel/acpi/madt_wakeup.c | 197 ++++++++++++++++++++++++++---
include/acpi/actbl2.h | 15 ++-
5 files changed, 220 insertions(+), 20 deletions(-)
create mode 100644 arch/x86/kernel/acpi/madt.S
Comments
> --- /dev/null
> +++ b/arch/x86/kernel/acpi/madt.S
I think the name 'madt.S' is too generic. How about something be more specific
such as madt_reset.S, or madt_playdead.S, etc?
> @@ -0,0 +1,24 @@
> +#include <linux/linkage.h>
> +#include <asm/nospec-branch.h>
> +#include <asm/page_types.h>
> +#include <asm/processor-flags.h>
> +
> + .text
> + .align PAGE_SIZE
> +SYM_FUNC_START(asm_acpi_mp_play_dead)
> + /* Load address of reset vector into RCX to jump when kernel is ready */
> + movq acpi_mp_reset_vector_paddr(%rip), %rcx
> +
> + /* Turn off global entries. Following CR3 write will flush them. */
> + movq %cr4, %rdx
> + andq $~(X86_CR4_PGE), %rdx
> + movq %rdx, %cr4
> +
> + /* Switch to identity mapping */
> + movq acpi_mp_pgd(%rip), %rax
> + movq %rax, %cr3
Do we need to switch back to kernel direct-map page table after CPU is wake up
again? We do support normal CPU offline/online, but not limited to kexec,
right?
> +
> + /* Jump to reset vector */
> + ANNOTATE_RETPOLINE_SAFE
> + jmp *%rcx
> +SYM_FUNC_END(asm_acpi_mp_play_dead)
> diff --git a/arch/x86/kernel/acpi/madt_wakeup.c b/arch/x86/kernel/acpi/madt_wakeup.c
> index ad170def2367..f9ff14ee2892 100644
> --- a/arch/x86/kernel/acpi/madt_wakeup.c
> +++ b/arch/x86/kernel/acpi/madt_wakeup.c
> @@ -1,8 +1,13 @@
> #include <linux/acpi.h>
> #include <linux/cpu.h>
> +#include <linux/delay.h>
> #include <linux/io.h>
> +#include <linux/memblock.h>
> +#include <linux/pgtable.h>
> +#include <linux/sched/hotplug.h>
> #include <asm/apic.h>
> #include <asm/barrier.h>
> +#include <asm/init.h>
> #include <asm/processor.h>
>
> /* Physical address of the Multiprocessor Wakeup Structure mailbox */
> @@ -11,6 +16,150 @@ 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;
>
> +u64 acpi_mp_pgd;
> +u64 acpi_mp_reset_vector_paddr;
> +
> +void asm_acpi_mp_play_dead(void);
> +
> +static void __init *alloc_pgt_page(void *context)
> +{
> + return memblock_alloc(PAGE_SIZE, PAGE_SIZE);
> +}
> +
> +/*
> + * Make sure asm_acpi_mp_play_dead() is present in the identity mapping at
> + * the same place as in the kernel page tables. The function switches to
> + * the identity mapping
>
This function itself doesn't switch to the identity mapping. It just creates
the kernel mapping for asm_acpi_mp_play_dead() in the identify mapping page
table.
> and has be present at the same spot in before and
> + * after transition.
This part doesn't parse to me. I guess the whole comment can be:
asm_acpi_mp_play_dead() is accessed both before and after switching to
the identity mapping. Also map it at the kernel virtual address in
the identity mapping table.
Or perhaps even better, put the above comments to the place where this function
is called?
> + */
> +static int __init init_transition_pgtable(pgd_t *pgd)
> +{
> + pgprot_t prot = PAGE_KERNEL_EXEC_NOENC;
> + unsigned long vaddr, paddr;
> + int result = -ENOMEM;
> + p4d_t *p4d;
> + pud_t *pud;
> + pmd_t *pmd;
> + pte_t *pte;
> +
> + vaddr = (unsigned long)asm_acpi_mp_play_dead;
> + pgd += pgd_index(vaddr);
> + if (!pgd_present(*pgd)) {
> + p4d = (p4d_t *)alloc_pgt_page(NULL);
> + if (!p4d)
> + goto err;
> + set_pgd(pgd, __pgd(__pa(p4d) | _KERNPG_TABLE));
> + }
> + p4d = p4d_offset(pgd, vaddr);
> + if (!p4d_present(*p4d)) {
> + pud = (pud_t *)alloc_pgt_page(NULL);
> + if (!pud)
> + goto err;
> + set_p4d(p4d, __p4d(__pa(pud) | _KERNPG_TABLE));
> + }
> + pud = pud_offset(p4d, vaddr);
> + if (!pud_present(*pud)) {
> + pmd = (pmd_t *)alloc_pgt_page(NULL);
> + if (!pmd)
> + goto err;
> + set_pud(pud, __pud(__pa(pmd) | _KERNPG_TABLE));
> + }
> + pmd = pmd_offset(pud, vaddr);
> + if (!pmd_present(*pmd)) {
> + pte = (pte_t *)alloc_pgt_page(NULL);
> + if (!pte)
> + goto err;
> + set_pmd(pmd, __pmd(__pa(pte) | _KERNPG_TABLE));
> + }
> + pte = pte_offset_kernel(pmd, vaddr);
> +
> + paddr = __pa(vaddr);
> + set_pte(pte, pfn_pte(paddr >> PAGE_SHIFT, prot));
> +
> + return 0;
> +err:
> + return result;
> +}
> +
> +static void acpi_mp_play_dead(void)
> +{
> + play_dead_common();
> + asm_acpi_mp_play_dead();
> +}
> +
> +static void acpi_mp_cpu_die(unsigned int cpu)
> +{
> + int apicid = per_cpu(x86_cpu_to_apicid, cpu);
> + unsigned long timeout;
> +
> + /*
> + * Use TEST mailbox command to prove that BIOS got control over
> + * the CPU before declaring it dead.
> + *
> + * BIOS has to clear 'command' field of the mailbox.
> + */
> + acpi_mp_wake_mailbox->apic_id = apicid;
> + smp_store_release(&acpi_mp_wake_mailbox->command,
> + ACPI_MP_WAKE_COMMAND_TEST);
> +
> + /* Don't wait longer than a second. */
> + timeout = USEC_PER_SEC;
> + while (READ_ONCE(acpi_mp_wake_mailbox->command) && timeout--)
> + udelay(1);
> +}
> +
> +static void acpi_mp_stop_other_cpus(int wait)
> +{
> + smp_shutdown_nonboot_cpus(smp_processor_id());
> +}
> +
> +static void acpi_mp_crash_stop_other_cpus(void)
> +{
> + smp_shutdown_nonboot_cpus(smp_processor_id());
> +
> + /* The kernel is broken so disable interrupts */
> + local_irq_disable();
> +}
> +
> +static int __init acpi_mp_setup_reset(u64 reset_vector)
> +{
> + pgd_t *pgd;
> + struct x86_mapping_info info = {
> + .alloc_pgt_page = alloc_pgt_page,
> + .page_flag = __PAGE_KERNEL_LARGE_EXEC,
> + .kernpg_flag = _KERNPG_TABLE_NOENC,
> + };
> +
> + pgd = alloc_pgt_page(NULL);
> +
> + for (int i = 0; i < nr_pfn_mapped; i++) {
> + unsigned long mstart, mend;
> + mstart = pfn_mapped[i].start << PAGE_SHIFT;
> + mend = pfn_mapped[i].end << PAGE_SHIFT;
> + if (kernel_ident_mapping_init(&info, pgd, mstart, mend))
> + return -ENOMEM;
> + }
This is for kexec() IIUC. Add a comment?
If we consider normal CPU offline/online case, then I don't think we need the
identity mapping for all memory?
> +
> + if (kernel_ident_mapping_init(&info, pgd,
> + PAGE_ALIGN_DOWN(reset_vector),
> + PAGE_ALIGN(reset_vector + 1))) {
> + return -ENOMEM;
> + }
> +
> + if (init_transition_pgtable(pgd))
> + return -ENOMEM;
> +
> + smp_ops.play_dead = acpi_mp_play_dead;
> + smp_ops.cpu_die = acpi_mp_cpu_die;
> + smp_ops.stop_other_cpus = acpi_mp_stop_other_cpus;
> + smp_ops.crash_stop_other_cpus = acpi_mp_crash_stop_other_cpus;
> +
> + acpi_mp_reset_vector_paddr = reset_vector;
> + acpi_mp_pgd = __pa(pgd);
> +
> + return 0;
> +}
> +
> static int acpi_wakeup_cpu(int apicid, unsigned long start_ip)
> {
> if (!acpi_mp_wake_mailbox_paddr) {
> @@ -74,31 +223,43 @@ int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
> struct acpi_madt_multiproc_wakeup *mp_wake;
>
> mp_wake = (struct acpi_madt_multiproc_wakeup *)header;
> - if (BAD_MADT_ENTRY(mp_wake, end))
> + if (!mp_wake)
> + return -EINVAL;
> +
> + if (end - (unsigned long)mp_wake < ACPI_MADT_MP_WAKEUP_SIZE_V0)
> + return -EINVAL;
> + if (mp_wake->header.length < ACPI_MADT_MP_WAKEUP_SIZE_V0)
> return -EINVAL;
>
> acpi_table_print_madt_entry(&header->common);
>
> acpi_mp_wake_mailbox_paddr = mp_wake->mailbox_address;
>
> - cpu_hotplug_disable_offlining();
> + if (mp_wake->version >= ACPI_MADT_MP_WAKEUP_VERSION_V1 &&
> + mp_wake->header.length >= ACPI_MADT_MP_WAKEUP_SIZE_V1) {
> + acpi_mp_setup_reset(mp_wake->reset_vector);
It's better to fallback to "disable offline" if this function fails.
> > + .text
> > + .align PAGE_SIZE
> > +SYM_FUNC_START(asm_acpi_mp_play_dead)
> > + /* Load address of reset vector into RCX to jump when kernel is ready */
> > + movq acpi_mp_reset_vector_paddr(%rip), %rcx
> > +
> > + /* Turn off global entries. Following CR3 write will flush them. */
> > + movq %cr4, %rdx
> > + andq $~(X86_CR4_PGE), %rdx
> > + movq %rdx, %cr4
> > +
> > + /* Switch to identity mapping */
> > + movq acpi_mp_pgd(%rip), %rax
> > + movq %rax, %cr3
>
> Do we need to switch back to kernel direct-map page table after CPU is wake up
> again? We do support normal CPU offline/online, but not limited to kexec,
> right?
Please ignore this. I found if I am reading right even for TDX guest the new
online cpu will start with trampoline_start64 assembly, so it will load kernel
page table anyway. Sorry for the noise.
[...]
> > + for (int i = 0; i < nr_pfn_mapped; i++) {
> > + unsigned long mstart, mend;
> > + mstart = pfn_mapped[i].start << PAGE_SHIFT;
> > + mend = pfn_mapped[i].end << PAGE_SHIFT;
> > + if (kernel_ident_mapping_init(&info, pgd, mstart, mend))
> > + return -ENOMEM;
> > + }
>
> This is for kexec() IIUC. Add a comment?
>
> If we consider normal CPU offline/online case, then I don't think we need the
> identity mapping for all memory?
>
Also this one. :-)
>
On Tue, Oct 24, 2023 at 10:11:58AM +0000, Huang, Kai wrote:
>
> > --- /dev/null
> > +++ b/arch/x86/kernel/acpi/madt.S
>
> I think the name 'madt.S' is too generic. How about something be more specific
> such as madt_reset.S, or madt_playdead.S, etc?
Okay, madt_playdead.S sounds good.
> > @@ -11,6 +16,150 @@ 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;
> >
> > +u64 acpi_mp_pgd;
> > +u64 acpi_mp_reset_vector_paddr;
> > +
> > +void asm_acpi_mp_play_dead(void);
> > +
> > +static void __init *alloc_pgt_page(void *context)
> > +{
> > + return memblock_alloc(PAGE_SIZE, PAGE_SIZE);
> > +}
> > +
> > +/*
> > + * Make sure asm_acpi_mp_play_dead() is present in the identity mapping at
> > + * the same place as in the kernel page tables. The function switches to
> > + * the identity mapping
> >
>
> This function itself doesn't switch to the identity mapping. It just creates
> the kernel mapping for asm_acpi_mp_play_dead() in the identify mapping page
> table.
By "The function" I meant asm_acpi_mp_play_dead(). Yeah, it is not clear.
Will so s/The function/asm_acpi_mp_play_dead()/
> > - cpu_hotplug_disable_offlining();
> > + if (mp_wake->version >= ACPI_MADT_MP_WAKEUP_VERSION_V1 &&
> > + mp_wake->header.length >= ACPI_MADT_MP_WAKEUP_SIZE_V1) {
> > + acpi_mp_setup_reset(mp_wake->reset_vector);
>
> It's better to fallback to "disable offline" if this function fails.
>
Okay, will warn to disable offlining.
On Fri, Oct 20 2023 at 18:12, Kirill A. Shutemov wrote:
> MADT Multiprocessor Wakeup structure version 1 brings support of CPU
> offlining: BIOS provides a reset vector where the CPU has to jump to
> offline itself. The new TEST mailbox command can be used to test the CPU
> offlined successfully and BIOS has control over it.
>
> Add CPU offling support for ACPI MADT wakeup method by implementing
> custom cpu_die, play_dead and stop_other_cpus SMP operations.
>
> CPU offlining makes possible to hand over secondary CPUs over kexec, not
makes it possible
> limiting the second kernel with single CPU.
s/with/to/
> The change conforms to the approved ACPI spec change proposal. See the
> +SYM_FUNC_START(asm_acpi_mp_play_dead)
> + /* Load address of reset vector into RCX to jump when kernel is ready */
> + movq acpi_mp_reset_vector_paddr(%rip), %rcx
> +
> + /* Turn off global entries. Following CR3 write will flush them. */
> + movq %cr4, %rdx
> + andq $~(X86_CR4_PGE), %rdx
> + movq %rdx, %cr4
> +
> + /* Switch to identity mapping */
> + movq acpi_mp_pgd(%rip), %rax
> + movq %rax, %cr3
You can just make this function:
asm_acpi_mp_play_dead(u64 reset_vector, u64 pgd_pa);
then you have the reset vector in RDI and the pgd in RSI and spare the
global variables.
>
> /* Physical address of the Multiprocessor Wakeup Structure mailbox */
> @@ -11,6 +16,150 @@ 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;
>
> +u64 acpi_mp_pgd;
> +u64 acpi_mp_reset_vector_paddr;
See above (static) and __ro_after_init please
> +
> +void asm_acpi_mp_play_dead(void);
> +
> +static void __init *alloc_pgt_page(void *context)
> +{
What's the purpose of the context argument?
> + return memblock_alloc(PAGE_SIZE, PAGE_SIZE);
> +}
> +
> +/*
> + * Make sure asm_acpi_mp_play_dead() is present in the identity mapping at
> + * the same place as in the kernel page tables. The function switches to
> + * the identity mapping and has be present at the same spot in before and
> + * after transition.
Why does it need to be there after the CPU jumped to the reset vector?
> + */
> +static int __init init_transition_pgtable(pgd_t *pgd)
> +{
> + pgprot_t prot = PAGE_KERNEL_EXEC_NOENC;
> + unsigned long vaddr, paddr;
> + int result = -ENOMEM;
> + p4d_t *p4d;
> + pud_t *pud;
> + pmd_t *pmd;
> + pte_t *pte;
> +
> + vaddr = (unsigned long)asm_acpi_mp_play_dead;
> + pgd += pgd_index(vaddr);
> + if (!pgd_present(*pgd)) {
> + p4d = (p4d_t *)alloc_pgt_page(NULL);
> + if (!p4d)
> + goto err;
return -ENOMEM?
the error labels is pretty silly without an actual cleanup, right?
> + set_pgd(pgd, __pgd(__pa(p4d) | _KERNPG_TABLE));
> + }
> + p4d = p4d_offset(pgd, vaddr);
> + if (!p4d_present(*p4d)) {
> + pud = (pud_t *)alloc_pgt_page(NULL);
> + if (!pud)
> + goto err;
Ditto. But what mops up the already allocated page above?
> + set_pte(pte, pfn_pte(paddr >> PAGE_SHIFT, prot));
> +
> + return 0;
> +err:
> + return result;
> +}
Can you please move that function to the place where it is used?
> +
> +static void acpi_mp_play_dead(void)
> +{
> + play_dead_common();
> + asm_acpi_mp_play_dead();
> +}
> +
> +static void acpi_mp_cpu_die(unsigned int cpu)
> +{
> + int apicid = per_cpu(x86_cpu_to_apicid, cpu);
u32 apicid
> + unsigned long timeout;
> +
> + /*
> + * Use TEST mailbox command to prove that BIOS got control over
> + * the CPU before declaring it dead.
> + *
> + * BIOS has to clear 'command' field of the mailbox.
> + */
> + acpi_mp_wake_mailbox->apic_id = apicid;
> + smp_store_release(&acpi_mp_wake_mailbox->command,
> + ACPI_MP_WAKE_COMMAND_TEST);
> +
> + /* Don't wait longer than a second. */
> + timeout = USEC_PER_SEC;
> + while (READ_ONCE(acpi_mp_wake_mailbox->command) && timeout--)
> + udelay(1);
> +}
> +
> +static void acpi_mp_stop_other_cpus(int wait)
> +{
> + smp_shutdown_nonboot_cpus(smp_processor_id());
This clearly was never tested with lockdep. At the point where
stop_other_cpus() is invoked the invoking CPU has interrupts disabled...
> +}
> +
> +static void acpi_mp_crash_stop_other_cpus(void)
> +{
> + smp_shutdown_nonboot_cpus(smp_processor_id());
Yuck. Crash can happen at arbitrary places. So you really cannot invoke
the whole CPU hotplug state machine from here.
There is a reason why the other implementation just kick CPUs into some
"safe" state.
> + /* The kernel is broken so disable interrupts */
> + local_irq_disable();
> +}
> +
> +static int __init acpi_mp_setup_reset(u64 reset_vector)
> +{
> + pgd_t *pgd;
> + struct x86_mapping_info info = {
> + .alloc_pgt_page = alloc_pgt_page,
> + .page_flag = __PAGE_KERNEL_LARGE_EXEC,
> + .kernpg_flag = _KERNPG_TABLE_NOENC,
> + };
> +
> + pgd = alloc_pgt_page(NULL);
> +
> + for (int i = 0; i < nr_pfn_mapped; i++) {
> + unsigned long mstart, mend;
Missing newline
> + mstart = pfn_mapped[i].start << PAGE_SHIFT;
> + mend = pfn_mapped[i].end << PAGE_SHIFT;
> + if (kernel_ident_mapping_init(&info, pgd, mstart, mend))
> + return -ENOMEM;
> + }
> +
> + if (kernel_ident_mapping_init(&info, pgd,
> + PAGE_ALIGN_DOWN(reset_vector),
> + PAGE_ALIGN(reset_vector + 1))) {
> + return -ENOMEM;
> + }
> +
> + if (init_transition_pgtable(pgd))
> + return -ENOMEM;
> +
> + smp_ops.play_dead = acpi_mp_play_dead;
> + smp_ops.cpu_die = acpi_mp_cpu_die;
> + smp_ops.stop_other_cpus = acpi_mp_stop_other_cpus;
> + smp_ops.crash_stop_other_cpus = acpi_mp_crash_stop_other_cpus;
> +
> + acpi_mp_reset_vector_paddr = reset_vector;
> + acpi_mp_pgd = __pa(pgd);
> +
> + return 0;
> +}
> +
> static int acpi_wakeup_cpu(int apicid, unsigned long start_ip)
> {
> if (!acpi_mp_wake_mailbox_paddr) {
> @@ -74,31 +223,43 @@ int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
> struct acpi_madt_multiproc_wakeup *mp_wake;
>
> mp_wake = (struct acpi_madt_multiproc_wakeup *)header;
> - if (BAD_MADT_ENTRY(mp_wake, end))
> + if (!mp_wake)
> + return -EINVAL;
> +
> + if (end - (unsigned long)mp_wake < ACPI_MADT_MP_WAKEUP_SIZE_V0)
> + return -EINVAL;
> + if (mp_wake->header.length < ACPI_MADT_MP_WAKEUP_SIZE_V0)
> return -EINVAL;
>
> acpi_table_print_madt_entry(&header->common);
>
> acpi_mp_wake_mailbox_paddr = mp_wake->mailbox_address;
>
> - cpu_hotplug_disable_offlining();
> + if (mp_wake->version >= ACPI_MADT_MP_WAKEUP_VERSION_V1 &&
> + mp_wake->header.length >= ACPI_MADT_MP_WAKEUP_SIZE_V1) {
> + acpi_mp_setup_reset(mp_wake->reset_vector);
> + } else {
> + cpu_hotplug_disable_offlining();
>
> - /*
> - * ACPI MADT doesn't allow to offline CPU after it got woke up.
> - * It limits kexec: the second kernel won't be able to use more than
> - * one CPU.
> - *
> - * Now acpi_mp_wake_mailbox_paddr already has the mailbox address.
> - * The acpi_wakeup_cpu() will use it to bring up secondary cpus.
> - *
> - * Zero out mailbox address in the ACPI MADT wakeup structure to
> - * indicate that the mailbox is not usable. This prevents the
> - * kexec()-ed kernel from reading a vaild mailbox, which in turn
> - * makes the kexec()-ed kernel only be able to use the boot CPU.
> - *
> - * This is Linux-specific protocol and not reflected in ACPI spec.
> - */
> - mp_wake->mailbox_address = 0;
> + /*
> + * ACPI MADT doesn't allow to offline CPU after it got woke up.
This is not longer accurate as V1 allows that ....
> + * It limits kexec: the second kernel won't be able to use more
> + * than one CPU.
Thanks,
tglx
On Sun, Oct 29, 2023 at 06:31:36PM +0100, Thomas Gleixner wrote:
> On Fri, Oct 20 2023 at 18:12, Kirill A. Shutemov wrote:
>
> > MADT Multiprocessor Wakeup structure version 1 brings support of CPU
> > offlining: BIOS provides a reset vector where the CPU has to jump to
> > offline itself. The new TEST mailbox command can be used to test the CPU
> > offlined successfully and BIOS has control over it.
> >
> > Add CPU offling support for ACPI MADT wakeup method by implementing
> > custom cpu_die, play_dead and stop_other_cpus SMP operations.
> >
> > CPU offlining makes possible to hand over secondary CPUs over kexec, not
>
> makes it possible
>
> > limiting the second kernel with single CPU.
>
> s/with/to/
Okay.
> > The change conforms to the approved ACPI spec change proposal. See the
> > +SYM_FUNC_START(asm_acpi_mp_play_dead)
> > + /* Load address of reset vector into RCX to jump when kernel is ready */
> > + movq acpi_mp_reset_vector_paddr(%rip), %rcx
> > +
> > + /* Turn off global entries. Following CR3 write will flush them. */
> > + movq %cr4, %rdx
> > + andq $~(X86_CR4_PGE), %rdx
> > + movq %rdx, %cr4
> > +
> > + /* Switch to identity mapping */
> > + movq acpi_mp_pgd(%rip), %rax
> > + movq %rax, %cr3
>
> You can just make this function:
>
> asm_acpi_mp_play_dead(u64 reset_vector, u64 pgd_pa);
>
> then you have the reset vector in RDI and the pgd in RSI and spare the
> global variables.
Yeah, it is better. Thanks.
> > /* Physical address of the Multiprocessor Wakeup Structure mailbox */
> > @@ -11,6 +16,150 @@ 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;
> >
> > +u64 acpi_mp_pgd;
> > +u64 acpi_mp_reset_vector_paddr;
>
> See above (static) and __ro_after_init please
>
> > +
> > +void asm_acpi_mp_play_dead(void);
> > +
> > +static void __init *alloc_pgt_page(void *context)
> > +{
>
> What's the purpose of the context argument?
To conform to x86_mapping_info::alloc_pgt_page type.
I will rename the argument to 'dummy' and add comment.
> > + return memblock_alloc(PAGE_SIZE, PAGE_SIZE);
> > +}
> > +
> > +/*
> > + * Make sure asm_acpi_mp_play_dead() is present in the identity mapping at
> > + * the same place as in the kernel page tables. The function switches to
> > + * the identity mapping and has be present at the same spot in before and
> > + * after transition.
>
> Why does it need to be there after the CPU jumped to the reset vector?
After transition to the identity mapping, not after jumping to reset
vector. I will adjust the comment.
> > + */
> > +static int __init init_transition_pgtable(pgd_t *pgd)
> > +{
> > + pgprot_t prot = PAGE_KERNEL_EXEC_NOENC;
> > + unsigned long vaddr, paddr;
> > + int result = -ENOMEM;
> > + p4d_t *p4d;
> > + pud_t *pud;
> > + pmd_t *pmd;
> > + pte_t *pte;
> > +
> > + vaddr = (unsigned long)asm_acpi_mp_play_dead;
> > + pgd += pgd_index(vaddr);
> > + if (!pgd_present(*pgd)) {
> > + p4d = (p4d_t *)alloc_pgt_page(NULL);
> > + if (!p4d)
> > + goto err;
>
> return -ENOMEM?
>
> the error labels is pretty silly without an actual cleanup, right?
Right.
> > + set_pgd(pgd, __pgd(__pa(p4d) | _KERNPG_TABLE));
> > + }
> > + p4d = p4d_offset(pgd, vaddr);
> > + if (!p4d_present(*p4d)) {
> > + pud = (pud_t *)alloc_pgt_page(NULL);
> > + if (!pud)
> > + goto err;
>
> Ditto. But what mops up the already allocated page above?
Oops. I will add cleanup in acpi_mp_setup_reset() if
kernel_ident_mapping_init() or init_transition_pgtable() fails.
> > + set_pte(pte, pfn_pte(paddr >> PAGE_SHIFT, prot));
> > +
> > + return 0;
> > +err:
> > + return result;
> > +}
>
> Can you please move that function to the place where it is used?
Sure.
> > +
> > +static void acpi_mp_play_dead(void)
> > +{
> > + play_dead_common();
> > + asm_acpi_mp_play_dead();
> > +}
> > +
> > +static void acpi_mp_cpu_die(unsigned int cpu)
> > +{
> > + int apicid = per_cpu(x86_cpu_to_apicid, cpu);
>
> u32 apicid
Okay.
> > + unsigned long timeout;
> > +
> > + /*
> > + * Use TEST mailbox command to prove that BIOS got control over
> > + * the CPU before declaring it dead.
> > + *
> > + * BIOS has to clear 'command' field of the mailbox.
> > + */
> > + acpi_mp_wake_mailbox->apic_id = apicid;
> > + smp_store_release(&acpi_mp_wake_mailbox->command,
> > + ACPI_MP_WAKE_COMMAND_TEST);
> > +
> > + /* Don't wait longer than a second. */
> > + timeout = USEC_PER_SEC;
> > + while (READ_ONCE(acpi_mp_wake_mailbox->command) && timeout--)
> > + udelay(1);
> > +}
> > +
> > +static void acpi_mp_stop_other_cpus(int wait)
> > +{
> > + smp_shutdown_nonboot_cpus(smp_processor_id());
>
> This clearly was never tested with lockdep. At the point where
> stop_other_cpus() is invoked the invoking CPU has interrupts disabled...
Hm. I do have lockdep enabled and there's no reported.
And I am not sure why it should complain. At this point we are running on
reboot cpu (after migrate_to_reboot_cpu()).
Could you elaborate on what the problem here?
> > +}
> > +
> > +static void acpi_mp_crash_stop_other_cpus(void)
> > +{
> > + smp_shutdown_nonboot_cpus(smp_processor_id());
>
> Yuck. Crash can happen at arbitrary places. So you really cannot invoke
> the whole CPU hotplug state machine from here.
>
> There is a reason why the other implementation just kick CPUs into some
> "safe" state.
Yeah, fair enough.
What about the implementation below? It is heavily inspired by
nmi_shootdown_cpus().
BTW, I don't understand why nmi_shootdown_cpus() needs 'crashing_cpu' if
it uses apic_send_IPI_allbutself(). 'crashing_cpu' == self, no?
static atomic_t waiting_for_crash_ipi;
static bool crash_ipi_issued;
static int crash_nmi_callback(unsigned int val, struct pt_regs *regs)
{
local_irq_disable();
crash_save_cpu(regs, raw_smp_processor_id());
cpu_emergency_stop_pt();
disable_local_APIC();
/*
* Prepare the CPU for reboot _after_ invoking the callback so that the
* callback can safely use virtualization instructions, e.g. VMCLEAR.
*/
cpu_emergency_disable_virtualization();
atomic_dec(&waiting_for_crash_ipi);
asm_acpi_mp_play_dead(acpi_mp_reset_vector_paddr,
acpi_mp_pgd);
return NMI_HANDLED;
}
static void acpi_mp_crash_stop_other_cpus(void)
{
unsigned long timeout;
/* The kernel is broken so disable interrupts */
local_irq_disable();
/*
* Avoid certain doom if a shootdown already occurred; re-registering
* the NMI handler will cause list corruption, modifying the callback
* will do who knows what, etc...
*/
if (WARN_ON_ONCE(crash_ipi_issued))
return;
atomic_set(&waiting_for_crash_ipi, num_online_cpus() - 1);
/* Would it be better to replace the trap vector here? */
if (register_nmi_handler(NMI_LOCAL, crash_nmi_callback,
NMI_FLAG_FIRST, "crash"))
return; /* Return what? */
apic_send_IPI_allbutself(NMI_VECTOR);
WRITE_ONCE(crash_ipi_issued, 1);
/* Don't wait longer than a second. */
timeout = USEC_PER_SEC;
while (atomic_read(&waiting_for_crash_ipi) && timeout--)
udelay(1);
}
> > + /* The kernel is broken so disable interrupts */
> > + local_irq_disable();
> > +}
> > +
> > +static int __init acpi_mp_setup_reset(u64 reset_vector)
> > +{
> > + pgd_t *pgd;
> > + struct x86_mapping_info info = {
> > + .alloc_pgt_page = alloc_pgt_page,
> > + .page_flag = __PAGE_KERNEL_LARGE_EXEC,
> > + .kernpg_flag = _KERNPG_TABLE_NOENC,
> > + };
> > +
> > + pgd = alloc_pgt_page(NULL);
> > +
> > + for (int i = 0; i < nr_pfn_mapped; i++) {
> > + unsigned long mstart, mend;
>
> Missing newline
>
Okay.
> >
> > - /*
> > - * ACPI MADT doesn't allow to offline CPU after it got woke up.
> > - * It limits kexec: the second kernel won't be able to use more than
> > - * one CPU.
> > - *
> > - * Now acpi_mp_wake_mailbox_paddr already has the mailbox address.
> > - * The acpi_wakeup_cpu() will use it to bring up secondary cpus.
> > - *
> > - * Zero out mailbox address in the ACPI MADT wakeup structure to
> > - * indicate that the mailbox is not usable. This prevents the
> > - * kexec()-ed kernel from reading a vaild mailbox, which in turn
> > - * makes the kexec()-ed kernel only be able to use the boot CPU.
> > - *
> > - * This is Linux-specific protocol and not reflected in ACPI spec.
> > - */
> > - mp_wake->mailbox_address = 0;
> > + /*
> > + * ACPI MADT doesn't allow to offline CPU after it got woke up.
>
> This is not longer accurate as V1 allows that ....
>
Will fix.
@@ -4,7 +4,7 @@ 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
+obj-$(CONFIG_X86_ACPI_MADT_WAKEUP) += madt_wakeup.o madt.o
ifneq ($(CONFIG_ACPI_PROCESSOR),)
obj-y += cstate.o
@@ -22,6 +22,7 @@
#include <linux/efi-bgrt.h>
#include <linux/serial_core.h>
#include <linux/pgtable.h>
+#include <linux/sched/hotplug.h>
#include <asm/e820/api.h>
#include <asm/irqdomain.h>
@@ -33,6 +34,7 @@
#include <asm/smp.h>
#include <asm/i8259.h>
#include <asm/setup.h>
+#include <asm/init.h>
#include "sleep.h" /* To include x86_acpi_suspend_lowlevel */
static int __initdata acpi_force = 0;
new file mode 100644
@@ -0,0 +1,24 @@
+#include <linux/linkage.h>
+#include <asm/nospec-branch.h>
+#include <asm/page_types.h>
+#include <asm/processor-flags.h>
+
+ .text
+ .align PAGE_SIZE
+SYM_FUNC_START(asm_acpi_mp_play_dead)
+ /* Load address of reset vector into RCX to jump when kernel is ready */
+ movq acpi_mp_reset_vector_paddr(%rip), %rcx
+
+ /* Turn off global entries. Following CR3 write will flush them. */
+ movq %cr4, %rdx
+ andq $~(X86_CR4_PGE), %rdx
+ movq %rdx, %cr4
+
+ /* Switch to identity mapping */
+ movq acpi_mp_pgd(%rip), %rax
+ movq %rax, %cr3
+
+ /* Jump to reset vector */
+ ANNOTATE_RETPOLINE_SAFE
+ jmp *%rcx
+SYM_FUNC_END(asm_acpi_mp_play_dead)
@@ -1,8 +1,13 @@
#include <linux/acpi.h>
#include <linux/cpu.h>
+#include <linux/delay.h>
#include <linux/io.h>
+#include <linux/memblock.h>
+#include <linux/pgtable.h>
+#include <linux/sched/hotplug.h>
#include <asm/apic.h>
#include <asm/barrier.h>
+#include <asm/init.h>
#include <asm/processor.h>
/* Physical address of the Multiprocessor Wakeup Structure mailbox */
@@ -11,6 +16,150 @@ 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;
+u64 acpi_mp_pgd;
+u64 acpi_mp_reset_vector_paddr;
+
+void asm_acpi_mp_play_dead(void);
+
+static void __init *alloc_pgt_page(void *context)
+{
+ return memblock_alloc(PAGE_SIZE, PAGE_SIZE);
+}
+
+/*
+ * Make sure asm_acpi_mp_play_dead() is present in the identity mapping at
+ * the same place as in the kernel page tables. The function switches to
+ * the identity mapping and has be present at the same spot in before and
+ * after transition.
+ */
+static int __init init_transition_pgtable(pgd_t *pgd)
+{
+ pgprot_t prot = PAGE_KERNEL_EXEC_NOENC;
+ unsigned long vaddr, paddr;
+ int result = -ENOMEM;
+ p4d_t *p4d;
+ pud_t *pud;
+ pmd_t *pmd;
+ pte_t *pte;
+
+ vaddr = (unsigned long)asm_acpi_mp_play_dead;
+ pgd += pgd_index(vaddr);
+ if (!pgd_present(*pgd)) {
+ p4d = (p4d_t *)alloc_pgt_page(NULL);
+ if (!p4d)
+ goto err;
+ set_pgd(pgd, __pgd(__pa(p4d) | _KERNPG_TABLE));
+ }
+ p4d = p4d_offset(pgd, vaddr);
+ if (!p4d_present(*p4d)) {
+ pud = (pud_t *)alloc_pgt_page(NULL);
+ if (!pud)
+ goto err;
+ set_p4d(p4d, __p4d(__pa(pud) | _KERNPG_TABLE));
+ }
+ pud = pud_offset(p4d, vaddr);
+ if (!pud_present(*pud)) {
+ pmd = (pmd_t *)alloc_pgt_page(NULL);
+ if (!pmd)
+ goto err;
+ set_pud(pud, __pud(__pa(pmd) | _KERNPG_TABLE));
+ }
+ pmd = pmd_offset(pud, vaddr);
+ if (!pmd_present(*pmd)) {
+ pte = (pte_t *)alloc_pgt_page(NULL);
+ if (!pte)
+ goto err;
+ set_pmd(pmd, __pmd(__pa(pte) | _KERNPG_TABLE));
+ }
+ pte = pte_offset_kernel(pmd, vaddr);
+
+ paddr = __pa(vaddr);
+ set_pte(pte, pfn_pte(paddr >> PAGE_SHIFT, prot));
+
+ return 0;
+err:
+ return result;
+}
+
+static void acpi_mp_play_dead(void)
+{
+ play_dead_common();
+ asm_acpi_mp_play_dead();
+}
+
+static void acpi_mp_cpu_die(unsigned int cpu)
+{
+ int apicid = per_cpu(x86_cpu_to_apicid, cpu);
+ unsigned long timeout;
+
+ /*
+ * Use TEST mailbox command to prove that BIOS got control over
+ * the CPU before declaring it dead.
+ *
+ * BIOS has to clear 'command' field of the mailbox.
+ */
+ acpi_mp_wake_mailbox->apic_id = apicid;
+ smp_store_release(&acpi_mp_wake_mailbox->command,
+ ACPI_MP_WAKE_COMMAND_TEST);
+
+ /* Don't wait longer than a second. */
+ timeout = USEC_PER_SEC;
+ while (READ_ONCE(acpi_mp_wake_mailbox->command) && timeout--)
+ udelay(1);
+}
+
+static void acpi_mp_stop_other_cpus(int wait)
+{
+ smp_shutdown_nonboot_cpus(smp_processor_id());
+}
+
+static void acpi_mp_crash_stop_other_cpus(void)
+{
+ smp_shutdown_nonboot_cpus(smp_processor_id());
+
+ /* The kernel is broken so disable interrupts */
+ local_irq_disable();
+}
+
+static int __init acpi_mp_setup_reset(u64 reset_vector)
+{
+ pgd_t *pgd;
+ struct x86_mapping_info info = {
+ .alloc_pgt_page = alloc_pgt_page,
+ .page_flag = __PAGE_KERNEL_LARGE_EXEC,
+ .kernpg_flag = _KERNPG_TABLE_NOENC,
+ };
+
+ pgd = alloc_pgt_page(NULL);
+
+ for (int i = 0; i < nr_pfn_mapped; i++) {
+ unsigned long mstart, mend;
+ mstart = pfn_mapped[i].start << PAGE_SHIFT;
+ mend = pfn_mapped[i].end << PAGE_SHIFT;
+ if (kernel_ident_mapping_init(&info, pgd, mstart, mend))
+ return -ENOMEM;
+ }
+
+ if (kernel_ident_mapping_init(&info, pgd,
+ PAGE_ALIGN_DOWN(reset_vector),
+ PAGE_ALIGN(reset_vector + 1))) {
+ return -ENOMEM;
+ }
+
+ if (init_transition_pgtable(pgd))
+ return -ENOMEM;
+
+ smp_ops.play_dead = acpi_mp_play_dead;
+ smp_ops.cpu_die = acpi_mp_cpu_die;
+ smp_ops.stop_other_cpus = acpi_mp_stop_other_cpus;
+ smp_ops.crash_stop_other_cpus = acpi_mp_crash_stop_other_cpus;
+
+ acpi_mp_reset_vector_paddr = reset_vector;
+ acpi_mp_pgd = __pa(pgd);
+
+ return 0;
+}
+
static int acpi_wakeup_cpu(int apicid, unsigned long start_ip)
{
if (!acpi_mp_wake_mailbox_paddr) {
@@ -74,31 +223,43 @@ int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
struct acpi_madt_multiproc_wakeup *mp_wake;
mp_wake = (struct acpi_madt_multiproc_wakeup *)header;
- if (BAD_MADT_ENTRY(mp_wake, end))
+ if (!mp_wake)
+ return -EINVAL;
+
+ if (end - (unsigned long)mp_wake < ACPI_MADT_MP_WAKEUP_SIZE_V0)
+ return -EINVAL;
+ if (mp_wake->header.length < ACPI_MADT_MP_WAKEUP_SIZE_V0)
return -EINVAL;
acpi_table_print_madt_entry(&header->common);
acpi_mp_wake_mailbox_paddr = mp_wake->mailbox_address;
- cpu_hotplug_disable_offlining();
+ if (mp_wake->version >= ACPI_MADT_MP_WAKEUP_VERSION_V1 &&
+ mp_wake->header.length >= ACPI_MADT_MP_WAKEUP_SIZE_V1) {
+ acpi_mp_setup_reset(mp_wake->reset_vector);
+ } else {
+ cpu_hotplug_disable_offlining();
- /*
- * ACPI MADT doesn't allow to offline CPU after it got woke up.
- * It limits kexec: the second kernel won't be able to use more than
- * one CPU.
- *
- * Now acpi_mp_wake_mailbox_paddr already has the mailbox address.
- * The acpi_wakeup_cpu() will use it to bring up secondary cpus.
- *
- * Zero out mailbox address in the ACPI MADT wakeup structure to
- * indicate that the mailbox is not usable. This prevents the
- * kexec()-ed kernel from reading a vaild mailbox, which in turn
- * makes the kexec()-ed kernel only be able to use the boot CPU.
- *
- * This is Linux-specific protocol and not reflected in ACPI spec.
- */
- mp_wake->mailbox_address = 0;
+ /*
+ * ACPI MADT doesn't allow to offline CPU after it got woke up.
+ * It limits kexec: the second kernel won't be able to use more
+ * than one CPU.
+ *
+ * Now acpi_mp_wake_mailbox_paddr already has the mailbox
+ * address. The acpi_wakeup_cpu() will use it to bring up
+ * secondary cpus.
+ *
+ * Zero out mailbox address in the ACPI MADT wakeup structure
+ * to indicate that the mailbox is not usable. This prevents
+ * the kexec()-ed kernel from reading a vaild mailbox, which in
+ * turn makes the kexec()-ed kernel only be able to use the boot
+ * CPU.
+ *
+ * This is Linux-specific protocol and not reflected in ACPI spec.
+ */
+ mp_wake->mailbox_address = 0;
+ }
apic_update_callback(wakeup_secondary_cpu_64, acpi_wakeup_cpu);
@@ -1112,8 +1112,20 @@ struct acpi_madt_multiproc_wakeup {
u16 version;
u32 reserved; /* reserved - must be zero */
u64 mailbox_address;
+ u64 reset_vector;
};
+/* Values for Version field above */
+
+enum acpi_madt_multiproc_wakeup_version {
+ ACPI_MADT_MP_WAKEUP_VERSION_NONE = 0,
+ ACPI_MADT_MP_WAKEUP_VERSION_V1 = 1,
+ ACPI_MADT_MP_WAKEUP_VERSION_RESERVED = 2, /* 2 and greater are reserved */
+};
+
+#define ACPI_MADT_MP_WAKEUP_SIZE_V0 16
+#define ACPI_MADT_MP_WAKEUP_SIZE_V1 24
+
#define ACPI_MULTIPROC_WAKEUP_MB_OS_SIZE 2032
#define ACPI_MULTIPROC_WAKEUP_MB_FIRMWARE_SIZE 2048
@@ -1126,7 +1138,8 @@ struct acpi_madt_multiproc_wakeup_mailbox {
u8 reserved_firmware[ACPI_MULTIPROC_WAKEUP_MB_FIRMWARE_SIZE]; /* reserved for firmware use */
};
-#define ACPI_MP_WAKE_COMMAND_WAKEUP 1
+#define ACPI_MP_WAKE_COMMAND_WAKEUP 1
+#define ACPI_MP_WAKE_COMMAND_TEST 2
/* 17: CPU Core Interrupt Controller (ACPI 6.5) */