From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
Dave Hansen <dave.hansen@linux.intel.com>,
x86@kernel.org, "Rafael J. Wysocki" <rafael@kernel.org>,
Peter Zijlstra <peterz@infradead.org>,
Adrian Hunter <adrian.hunter@intel.com>,
Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com>,
Elena Reshetova <elena.reshetova@intel.com>,
Jun Nakajima <jun.nakajima@intel.com>,
Rick Edgecombe <rick.p.edgecombe@intel.com>,
Tom Lendacky <thomas.lendacky@amd.com>,
"Kalra, Ashish" <ashish.kalra@amd.com>,
Sean Christopherson <seanjc@google.com>,
"Huang, Kai" <kai.huang@intel.com>, Baoquan He <bhe@redhat.com>,
kexec@lists.infradead.org, linux-coco@lists.linux.dev,
linux-kernel@vger.kernel.org
Subject: Re: [PATCHv2 13/13] x86/acpi: Add support for CPU offlining for ACPI MADT wakeup method
Date: Wed, 1 Nov 2023 16:26:16 +0300 [thread overview]
Message-ID: <20231101132616.nviynrbb53acpnv5@box> (raw)
In-Reply-To: <87r0ldz66f.ffs@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.
--
Kiryl Shutsemau / Kirill A. Shutemov
prev parent reply other threads:[~2023-11-01 13:26 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-20 15:12 [PATCHv2 00/13] x86/tdx: Add kexec support Kirill A. Shutemov
2023-10-20 15:12 ` [PATCHv2 01/13] x86/acpi: Extract ACPI MADT wakeup code into a separate file Kirill A. Shutemov
2023-10-20 17:12 ` Kuppuswamy Sathyanarayanan
2023-10-20 15:12 ` [PATCHv2 02/13] kernel/cpu: Add support for declaring CPU offlining not supported Kirill A. Shutemov
2023-10-23 9:30 ` Huang, Kai
2023-10-23 15:31 ` kirill.shutemov
2023-10-23 22:07 ` Huang, Kai
2023-10-28 14:07 ` Thomas Gleixner
2023-10-28 14:12 ` Thomas Gleixner
2023-10-29 14:23 ` Kirill A. Shutemov
2023-10-20 15:12 ` [PATCHv2 03/13] cpu/hotplug, x86/acpi: Disable CPU offlining for ACPI MADT wakeup Kirill A. Shutemov
2023-10-20 15:12 ` [PATCHv2 04/13] x86/kvm: Do not try to disable kvmclock if it was not enabled Kirill A. Shutemov
2023-10-20 15:32 ` Sean Christopherson
2023-10-20 15:41 ` Vitaly Kuznetsov
2023-10-20 17:07 ` Sean Christopherson
2023-10-23 8:45 ` Vitaly Kuznetsov
2023-10-23 14:40 ` Sean Christopherson
2023-10-20 15:12 ` [PATCHv2 05/13] x86/kexec: Keep CR4.MCE set during kexec for TDX guest Kirill A. Shutemov
2023-10-20 15:12 ` [PATCHv2 06/13] x86/mm: Make x86_platform.guest.enc_status_change_*() return errno Kirill A. Shutemov
2023-10-20 15:12 ` [PATCHv2 07/13] x86/mm: Return correct level from lookup_address() if pte is none Kirill A. Shutemov
2023-10-20 15:12 ` [PATCHv2 08/13] x86/tdx: Account shared memory Kirill A. Shutemov
2023-10-20 15:12 ` [PATCHv2 09/13] x86/tdx: Convert shared memory back to private on kexec Kirill A. Shutemov
2023-10-20 15:12 ` [PATCHv2 10/13] x86/mm: Make e820_end_ram_pfn() cover E820_TYPE_ACPI ranges Kirill A. Shutemov
2023-10-20 15:12 ` [PATCHv2 11/13] x86/acpi: Do not attempt to bring up secondary CPUs in kexec case Kirill A. Shutemov
2023-10-24 10:14 ` Huang, Kai
2023-10-24 13:59 ` Kuppuswamy Sathyanarayanan
2023-10-27 13:01 ` Kirill A. Shutemov
2023-10-20 15:12 ` [PATCHv2 12/13] x86/acpi: Rename fields in acpi_madt_multiproc_wakeup structure Kirill A. Shutemov
2023-10-24 10:18 ` Huang, Kai
2023-10-24 12:46 ` Kuppuswamy Sathyanarayanan
2023-10-20 15:12 ` [PATCHv2 13/13] x86/acpi: Add support for CPU offlining for ACPI MADT wakeup method Kirill A. Shutemov
2023-10-24 10:11 ` Huang, Kai
2023-10-25 3:50 ` Huang, Kai
2023-10-27 11:58 ` kirill.shutemov
2023-10-29 17:31 ` Thomas Gleixner
2023-11-01 13:26 ` Kirill A. Shutemov [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20231101132616.nviynrbb53acpnv5@box \
--to=kirill.shutemov@linux.intel.com \
--cc=adrian.hunter@intel.com \
--cc=ashish.kalra@amd.com \
--cc=bhe@redhat.com \
--cc=bp@alien8.de \
--cc=dave.hansen@linux.intel.com \
--cc=elena.reshetova@intel.com \
--cc=jun.nakajima@intel.com \
--cc=kai.huang@intel.com \
--cc=kexec@lists.infradead.org \
--cc=linux-coco@lists.linux.dev \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
--cc=rafael@kernel.org \
--cc=rick.p.edgecombe@intel.com \
--cc=sathyanarayanan.kuppuswamy@linux.intel.com \
--cc=seanjc@google.com \
--cc=tglx@linutronix.de \
--cc=thomas.lendacky@amd.com \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).