by Kirill A. Shutemov

[permalink] [raw]

Subject: Re: [PATCHv2 13/13] x86/acpi: Add support for CPU offlining for ACPI MADT wakeup method

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.

--
Kiryl Shutsemau / Kirill A. Shutemov