| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20231201155739.rxo6l5om6mdw54rs@box.shutemov.name>
Date: Fri, 1 Dec 2023 18:57:39 +0300
From: "kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>
To: "Huang, Kai" <kai.huang@...el.com>
Cc: "tglx@...utronix.de" <tglx@...utronix.de>,
"mingo@...hat.com" <mingo@...hat.com>,
"x86@...nel.org" <x86@...nel.org>, "bp@...en8.de" <bp@...en8.de>,
"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
"Edgecombe, Rick P" <rick.p.edgecombe@...el.com>,
"Reshetova, Elena" <elena.reshetova@...el.com>,
"Nakajima, Jun" <jun.nakajima@...el.com>,
"rafael@...nel.org" <rafael@...nel.org>,
"peterz@...radead.org" <peterz@...radead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"sathyanarayanan.kuppuswamy@...ux.intel.com"
<sathyanarayanan.kuppuswamy@...ux.intel.com>,
"Hunter, Adrian" <adrian.hunter@...el.com>,
"thomas.lendacky@....com" <thomas.lendacky@....com>,
"ashish.kalra@....com" <ashish.kalra@....com>,
"kexec@...ts.infradead.org" <kexec@...ts.infradead.org>,
"seanjc@...gle.com" <seanjc@...gle.com>,
"bhe@...hat.com" <bhe@...hat.com>,
"linux-coco@...ts.linux.dev" <linux-coco@...ts.linux.dev>
Subject: Re: [PATCHv3 14/14] x86/acpi: Add support for CPU offlining for ACPI
MADT wakeup method
On Thu, Nov 23, 2023 at 09:38:13AM +0000, Huang, Kai wrote:
>
> > diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
> > index 171d86fe71ef..602b5d3982ff 100644
> > --- a/arch/x86/kernel/acpi/boot.c
> > +++ b/arch/x86/kernel/acpi/boot.c
> > @@ -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>
>
> The above two are leftovers I believe?
>
> [...]
>
Right.
> > +
> > +static atomic_t waiting_for_crash_ipi;
> > +
> > +void asm_acpi_mp_play_dead(u64 reset_vector, u64 pgd_pa);
> > +
> > +static void acpi_mp_play_dead(void)
> > +{
> > + play_dead_common();
> > + asm_acpi_mp_play_dead(acpi_mp_reset_vector_paddr,
> > + acpi_mp_pgd);
> > +}
> > +
> > +static void acpi_mp_cpu_die(unsigned int cpu)
> > +{
> > + u32 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 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();
> > +
> > +
> > + 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);
> > +
> > + /* Don't wait longer than a second. */
> > + timeout = USEC_PER_SEC;
> > + while (atomic_read(&waiting_for_crash_ipi) && timeout--)
> > + udelay(1);
> > +}
> > +
> >
>
> [...]
>
> > + smp_ops.stop_other_cpus = acpi_mp_stop_other_cpus;
> > + smp_ops.crash_stop_other_cpus = acpi_mp_crash_stop_other_cpus;
> > +
> >
>
> The above acpi_mp_crash_stop_other_cpus() and crash_nmi_callback() etc are kinda
> duplicated code with the normal crash kexec() in reboot.c.
>
> I am not expert here but spent some time looking into the code, and it appears
> the main reason preventing us from reusing that code should be TDX guest doesn't
> play nicely with mwait/halt staff when putting the cpu to offline.
>
> I am thinking if we skip/replace them with the asm_acpi_mp_play_dead(), we
> should be able to just reuse the existing smp_ops.stop_other_cpus() and
> smp_ops.crash_stop_other_cpus()?
Okay, fair enough. See fixup below.
> > +
> > int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
> > const unsigned long end)
> > {
> > 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;
>
> I think you can keep the BAD_MADT_ENTRY() check as a standard check, and ...
No. BAD_MADT_ENTRY() will fail if the struct version is V0 because the
size will be smaller than sizeof(struct acpi_madt_multiproc_wakeup).
diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index 4fab2ed454f3..3c8efba86d5c 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -38,6 +38,7 @@ struct smp_ops {
int (*cpu_disable)(void);
void (*cpu_die)(unsigned int cpu);
void (*play_dead)(void);
+ void (*crash_play_dead)(void);
void (*send_call_func_ipi)(const struct cpumask *mask);
void (*send_call_func_single_ipi)(int cpu);
diff --git a/arch/x86/kernel/acpi/madt_wakeup.c b/arch/x86/kernel/acpi/madt_wakeup.c
index 782fe8fd533c..a801f773f9f1 100644
--- a/arch/x86/kernel/acpi/madt_wakeup.c
+++ b/arch/x86/kernel/acpi/madt_wakeup.c
@@ -25,6 +25,12 @@ static u64 acpi_mp_reset_vector_paddr __ro_after_init;
void asm_acpi_mp_play_dead(u64 reset_vector, u64 pgd_pa);
+static void crash_acpi_mp_play_dead(void)
+{
+ asm_acpi_mp_play_dead(acpi_mp_reset_vector_paddr,
+ acpi_mp_pgd);
+}
+
static void acpi_mp_play_dead(void)
{
play_dead_common();
@@ -58,57 +64,6 @@ static void acpi_mp_stop_other_cpus(int wait)
smp_shutdown_nonboot_cpus(smp_processor_id());
}
-#ifdef CONFIG_KEXEC_CORE
-static atomic_t waiting_for_crash_ipi;
-
-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();
-
-
- 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);
-
- /* Don't wait longer than a second. */
- timeout = USEC_PER_SEC;
- while (atomic_read(&waiting_for_crash_ipi) && timeout--)
- udelay(1);
-}
-#endif
-
/* The argument is required to match type of x86_mapping_info::alloc_pgt_page */
static void __init *alloc_pgt_page(void *dummy)
{
@@ -277,11 +232,9 @@ static int __init acpi_mp_setup_reset(u64 reset_vector)
}
smp_ops.play_dead = acpi_mp_play_dead;
+ smp_ops.crash_play_dead = crash_acpi_mp_play_dead;
smp_ops.cpu_die = acpi_mp_cpu_die;
smp_ops.stop_other_cpus = acpi_mp_stop_other_cpus;
-#ifdef CONFIG_KEXEC_CORE
- smp_ops.crash_stop_other_cpus = acpi_mp_crash_stop_other_cpus;
-#endif
acpi_mp_reset_vector_paddr = reset_vector;
acpi_mp_pgd = __pa(pgd);
diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index c81afffaa954..99e6ab552da0 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -878,10 +878,14 @@ static int crash_nmi_callback(unsigned int val, struct pt_regs *regs)
cpu_emergency_disable_virtualization();
atomic_dec(&waiting_for_crash_ipi);
- /* Assume hlt works */
- halt();
- for (;;)
- cpu_relax();
+
+ if (smp_ops.crash_play_dead) {
+ smp_ops.crash_play_dead();
+ } else {
+ halt();
+ for (;;)
+ cpu_relax();
+ }
return NMI_HANDLED;
}
--
Kiryl Shutsemau / Kirill A. Shutemov
Powered by blists - more mailing lists