2023-06-03 20:12:14

by Thomas Gleixner

Subject: [patch 5/6] x86/smp: Split sending INIT IPI out into a helper function

Putting CPUs into INIT is a safer place during kexec() to park CPUs.

Split the INIT assert/deassert sequence out so it can be reused.

Signed-off-by: Thomas Gleixner <[email protected]>
---
arch/x86/kernel/smpboot.c | 51 +++++++++++++++++++---------------------------
1 file changed, 22 insertions(+), 29 deletions(-)

--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -853,47 +853,40 @@ wakeup_secondary_cpu_via_nmi(int apicid,
return (send_status | accept_status);
}

-static int
-wakeup_secondary_cpu_via_init(int phys_apicid, unsigned long start_eip)
+static void send_init_sequence(int phys_apicid)
{
- unsigned long send_status = 0, accept_status = 0;
- int maxlvt, num_starts, j;
-
- maxlvt = lapic_get_maxlvt();
+ int maxlvt = lapic_get_maxlvt();

- /*
- * Be paranoid about clearing APIC errors.
- */
+ /* Be paranoid about clearing APIC errors. */
if (APIC_INTEGRATED(boot_cpu_apic_version)) {
- if (maxlvt > 3) /* Due to the Pentium erratum 3AP. */
+ /* Due to the Pentium erratum 3AP. */
+ if (maxlvt > 3)
apic_write(APIC_ESR, 0);
apic_read(APIC_ESR);
}

- pr_debug("Asserting INIT\n");
-
- /*
- * Turn INIT on target chip
- */
- /*
- * Send IPI
- */
- apic_icr_write(APIC_INT_LEVELTRIG | APIC_INT_ASSERT | APIC_DM_INIT,
- phys_apicid);
-
- pr_debug("Waiting for send to finish...\n");
- send_status = safe_apic_wait_icr_idle();
+ /* Assert INIT on the target CPU */
+ apic_icr_write(APIC_INT_LEVELTRIG | APIC_INT_ASSERT | APIC_DM_INIT, phys_apicid);
+ safe_apic_wait_icr_idle();

udelay(init_udelay);

- pr_debug("Deasserting INIT\n");
-
- /* Target chip */
- /* Send IPI */
+ /* Deassert INIT on the target CPU */
apic_icr_write(APIC_INT_LEVELTRIG | APIC_DM_INIT, phys_apicid);
+ safe_apic_wait_icr_idle();
+}
+
+/*
+ * Wake up AP by INIT, INIT, STARTUP sequence.
+ */
+static int wakeup_secondary_cpu_via_init(int phys_apicid, unsigned long start_eip)
+{
+ unsigned long send_status = 0, accept_status = 0;
+ int maxlvt, num_starts, j;
+
+ preempt_disable();

- pr_debug("Waiting for send to finish...\n");
- send_status = safe_apic_wait_icr_idle();
+ send_init_sequence(phys_apicid);

mb();

2023-06-04 04:45:02

by Mika Penttilä

Subject: Re: [patch 5/6] x86/smp: Split sending INIT IPI out into a helper function

Hi,

On 3.6.2023 23.07, Thomas Gleixner wrote:
> Putting CPUs into INIT is a safer place during kexec() to park CPUs.
>
> Split the INIT assert/deassert sequence out so it can be reused.
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> ---
> arch/x86/kernel/smpboot.c | 51 +++++++++++++++++++---------------------------
> 1 file changed, 22 insertions(+), 29 deletions(-)
>
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -853,47 +853,40 @@ wakeup_secondary_cpu_via_nmi(int apicid,
> return (send_status | accept_status);
> }
>
> -static int
> -wakeup_secondary_cpu_via_init(int phys_apicid, unsigned long start_eip)
> +static void send_init_sequence(int phys_apicid)
> {
> - unsigned long send_status = 0, accept_status = 0;
> - int maxlvt, num_starts, j;
> -
> - maxlvt = lapic_get_maxlvt();
> + int maxlvt = lapic_get_maxlvt();
>
> - /*
> - * Be paranoid about clearing APIC errors.
> - */
> + /* Be paranoid about clearing APIC errors. */
> if (APIC_INTEGRATED(boot_cpu_apic_version)) {
> - if (maxlvt > 3) /* Due to the Pentium erratum 3AP. */
> + /* Due to the Pentium erratum 3AP. */
> + if (maxlvt > 3)
> apic_write(APIC_ESR, 0);
> apic_read(APIC_ESR);
> }
>
> - pr_debug("Asserting INIT\n");
> -
> - /*
> - * Turn INIT on target chip
> - */
> - /*
> - * Send IPI
> - */
> - apic_icr_write(APIC_INT_LEVELTRIG | APIC_INT_ASSERT | APIC_DM_INIT,
> - phys_apicid);
> -
> - pr_debug("Waiting for send to finish...\n");
> - send_status = safe_apic_wait_icr_idle();
> + /* Assert INIT on the target CPU */
> + apic_icr_write(APIC_INT_LEVELTRIG | APIC_INT_ASSERT | APIC_DM_INIT, phys_apicid);
> + safe_apic_wait_icr_idle();
>
> udelay(init_udelay);
>
> - pr_debug("Deasserting INIT\n");
> -
> - /* Target chip */
> - /* Send IPI */
> + /* Deassert INIT on the target CPU */
> apic_icr_write(APIC_INT_LEVELTRIG | APIC_DM_INIT, phys_apicid);
> + safe_apic_wait_icr_idle();
> +}
> +
> +/*
> + * Wake up AP by INIT, INIT, STARTUP sequence.
> + */
> +static int wakeup_secondary_cpu_via_init(int phys_apicid, unsigned long start_eip)
> +{
> + unsigned long send_status = 0, accept_status = 0;
> + int maxlvt, num_starts, j;
> +
> + preempt_disable();

This seems like an unbalanced preempt disable..

>
> - pr_debug("Waiting for send to finish...\n");
> - send_status = safe_apic_wait_icr_idle();
> + send_init_sequence(phys_apicid);
>
> mb();
>
>


--Mika

2023-06-04 10:59:03

by Ashok Raj

Subject: Re: [patch 5/6] x86/smp: Split sending INIT IPI out into a helper function

Hi

On Sun, Jun 04, 2023 at 07:02:33AM +0300, Mika Penttil? wrote:
> Hi,
>
> On 3.6.2023 23.07, Thomas Gleixner wrote:
> > Putting CPUs into INIT is a safer place during kexec() to park CPUs.
> >
> > Split the INIT assert/deassert sequence out so it can be reused.
> >
> > Signed-off-by: Thomas Gleixner <[email protected]>
> > ---
> > arch/x86/kernel/smpboot.c | 51 +++++++++++++++++++---------------------------
> > 1 file changed, 22 insertions(+), 29 deletions(-)
> >
> > --- a/arch/x86/kernel/smpboot.c
> > +++ b/arch/x86/kernel/smpboot.c
> > @@ -853,47 +853,40 @@ wakeup_secondary_cpu_via_nmi(int apicid,
> > return (send_status | accept_status);
> > }
> > -static int
> > -wakeup_secondary_cpu_via_init(int phys_apicid, unsigned long start_eip)
> > +static void send_init_sequence(int phys_apicid)
> > {
> > - unsigned long send_status = 0, accept_status = 0;
> > - int maxlvt, num_starts, j;
> > -
> > - maxlvt = lapic_get_maxlvt();
> > + int maxlvt = lapic_get_maxlvt();
> > - /*
> > - * Be paranoid about clearing APIC errors.
> > - */
> > + /* Be paranoid about clearing APIC errors. */
> > if (APIC_INTEGRATED(boot_cpu_apic_version)) {
> > - if (maxlvt > 3) /* Due to the Pentium erratum 3AP. */
> > + /* Due to the Pentium erratum 3AP. */
> > + if (maxlvt > 3)
> > apic_write(APIC_ESR, 0);
> > apic_read(APIC_ESR);
> > }
> > - pr_debug("Asserting INIT\n");
> > -
> > - /*
> > - * Turn INIT on target chip
> > - */
> > - /*
> > - * Send IPI
> > - */
> > - apic_icr_write(APIC_INT_LEVELTRIG | APIC_INT_ASSERT | APIC_DM_INIT,
> > - phys_apicid);
> > -
> > - pr_debug("Waiting for send to finish...\n");
> > - send_status = safe_apic_wait_icr_idle();
> > + /* Assert INIT on the target CPU */
> > + apic_icr_write(APIC_INT_LEVELTRIG | APIC_INT_ASSERT | APIC_DM_INIT, phys_apicid);
> > + safe_apic_wait_icr_idle();
> > udelay(init_udelay);
> > - pr_debug("Deasserting INIT\n");
> > -
> > - /* Target chip */
> > - /* Send IPI */
> > + /* Deassert INIT on the target CPU */
> > apic_icr_write(APIC_INT_LEVELTRIG | APIC_DM_INIT, phys_apicid);
> > + safe_apic_wait_icr_idle();
> > +}
> > +
> > +/*
> > + * Wake up AP by INIT, INIT, STARTUP sequence.
> > + */
> > +static int wakeup_secondary_cpu_via_init(int phys_apicid, unsigned long start_eip)
> > +{
> > + unsigned long send_status = 0, accept_status = 0;
> > + int maxlvt, num_starts, j;

Also maxlvt used uninitialized.

> > +
> > + preempt_disable();
>
> This seems like an unbalanced preempt disable..
>
> > - pr_debug("Waiting for send to finish...\n");
> > - send_status = safe_apic_wait_icr_idle();
> > + send_init_sequence(phys_apicid);
> > mb();
> >
>
>
> --Mika
>

2023-06-05 08:04:58

by Thomas Gleixner

Subject: Re: [patch 5/6] x86/smp: Split sending INIT IPI out into a helper function

On Sun, Jun 04 2023 at 07:02, Mika Penttilä wrote:
> On 3.6.2023 23.07, Thomas Gleixner wrote:
>> +/*
>> + * Wake up AP by INIT, INIT, STARTUP sequence.
>> + */
>> +static int wakeup_secondary_cpu_via_init(int phys_apicid, unsigned long start_eip)
>> +{
>> + unsigned long send_status = 0, accept_status = 0;
>> + int maxlvt, num_starts, j;
>> +
>> + preempt_disable();
>
> This seems like an unbalanced preempt disable..

Right. I screwed up a rebase completely....

2023-06-05 08:26:58

by Thomas Gleixner

Subject: [patch v2 5/6] x86/smp: Split sending INIT IPI out into a helper function

Subject: x86/smp: Split sending INIT IPI out into a helper function
From: Thomas Gleixner <[email protected]>
Date: Fri, 02 Jun 2023 15:04:10 +0200

Putting CPUs into INIT is a safer place during kexec() to park CPUs.

Split the INIT assert/deassert sequence out so it can be reused.

Signed-off-by: Thomas Gleixner <[email protected]>
---
V2: Fix rebase screwup
---
arch/x86/kernel/smpboot.c | 49 ++++++++++++++++++----------------------------
1 file changed, 20 insertions(+), 29 deletions(-)

--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -853,47 +853,38 @@ wakeup_secondary_cpu_via_nmi(int apicid,
return (send_status | accept_status);
}

-static int
-wakeup_secondary_cpu_via_init(int phys_apicid, unsigned long start_eip)
+static void send_init_sequence(int phys_apicid)
{
- unsigned long send_status = 0, accept_status = 0;
- int maxlvt, num_starts, j;
-
- maxlvt = lapic_get_maxlvt();
+ int maxlvt = lapic_get_maxlvt();

- /*
- * Be paranoid about clearing APIC errors.
- */
+ /* Be paranoid about clearing APIC errors. */
if (APIC_INTEGRATED(boot_cpu_apic_version)) {
- if (maxlvt > 3) /* Due to the Pentium erratum 3AP. */
+ /* Due to the Pentium erratum 3AP. */
+ if (maxlvt > 3)
apic_write(APIC_ESR, 0);
apic_read(APIC_ESR);
}

- pr_debug("Asserting INIT\n");
-
- /*
- * Turn INIT on target chip
- */
- /*
- * Send IPI
- */
- apic_icr_write(APIC_INT_LEVELTRIG | APIC_INT_ASSERT | APIC_DM_INIT,
- phys_apicid);
-
- pr_debug("Waiting for send to finish...\n");
- send_status = safe_apic_wait_icr_idle();
+ /* Assert INIT on the target CPU */
+ apic_icr_write(APIC_INT_LEVELTRIG | APIC_INT_ASSERT | APIC_DM_INIT, phys_apicid);
+ safe_apic_wait_icr_idle();

udelay(init_udelay);

- pr_debug("Deasserting INIT\n");
-
- /* Target chip */
- /* Send IPI */
+ /* Deassert INIT on the target CPU */
apic_icr_write(APIC_INT_LEVELTRIG | APIC_DM_INIT, phys_apicid);
+ safe_apic_wait_icr_idle();
+}
+
+/*
+ * Wake up AP by INIT, INIT, STARTUP sequence.
+ */
+static int wakeup_secondary_cpu_via_init(int phys_apicid, unsigned long start_eip)
+{
+ unsigned long send_status = 0, accept_status = 0;
+ int num_starts, j, maxlvt = lapic_get_maxlvt();

- pr_debug("Waiting for send to finish...\n");
- send_status = safe_apic_wait_icr_idle();
+ send_init_sequence(phys_apicid);

mb();