diff mbox series

[v3,3/3] x86/acpi, x86/boot: Add multiprocessor wake-up support

Message ID 20210426023941.729334-4-sathyanarayanan.kuppuswamy@linux.intel.com (mailing list archive)
State Changes Requested, archived
Headers show
Series Add multiprocessor wake-up support | expand

Commit Message

Kuppuswamy Sathyanarayanan April 26, 2021, 2:39 a.m. UTC 
As per ACPI specification r6.4, sec 5.2.12.19, a new sub
structure – multiprocessor wake-up structure - is added to the
ACPI Multiple APIC Description Table (MADT) to describe the
information of the mailbox. If a platform firmware produces the
multiprocessor wake-up structure, then OS may use this new
mailbox-based mechanism to wake up the APs.

Add ACPI MADT wake table parsing support for x86 platform and if
MADT wake table is present, update apic->wakeup_secondary_cpu with
new API which uses MADT wake mailbox to wake-up CPU.

Co-developed-by: Sean Christopherson <sean.j.christopherson@intel.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Reviewed-by: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/include/asm/apic.h |  3 ++
 arch/x86/kernel/acpi/boot.c | 79 +++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/apic/apic.c |  8 ++++
 3 files changed, 90 insertions(+)

Comments

Kuppuswamy Sathyanarayanan May 10, 2021, 4:32 p.m. UTC | #1 
Hi Rafael/Thomas,

On 4/25/21 7:39 PM, Kuppuswamy Sathyanarayanan wrote:
> As per ACPI specification r6.4, sec 5.2.12.19, a new sub
> structure – multiprocessor wake-up structure - is added to the
> ACPI Multiple APIC Description Table (MADT) to describe the
> information of the mailbox. If a platform firmware produces the
> multiprocessor wake-up structure, then OS may use this new
> mailbox-based mechanism to wake up the APs.
> 
> Add ACPI MADT wake table parsing support for x86 platform and if
> MADT wake table is present, update apic->wakeup_secondary_cpu with
> new API which uses MADT wake mailbox to wake-up CPU.

Gentle ping. Any comments on this patch? I think I have addressed
the concerns raised by you in previous version.

> 
> Co-developed-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Reviewed-by: Andi Kleen <ak@linux.intel.com>
> ---
>   arch/x86/include/asm/apic.h |  3 ++
>   arch/x86/kernel/acpi/boot.c | 79 +++++++++++++++++++++++++++++++++++++
>   arch/x86/kernel/apic/apic.c |  8 ++++
>   3 files changed, 90 insertions(+)
> 
> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
> index 412b51e059c8..3e94e1f402ea 100644
> --- a/arch/x86/include/asm/apic.h
> +++ b/arch/x86/include/asm/apic.h
> @@ -487,6 +487,9 @@ static inline unsigned int read_apic_id(void)
>   	return apic->get_apic_id(reg);
>   }
>   
> +typedef int (*wakeup_cpu_handler)(int apicid, unsigned long start_eip);
> +extern void acpi_wake_cpu_handler_update(wakeup_cpu_handler handler);
> +
>   extern int default_apic_id_valid(u32 apicid);
>   extern int default_acpi_madt_oem_check(char *, char *);
>   extern void default_setup_apic_routing(void);
> diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
> index 14cd3186dc77..fce2aa7d718f 100644
> --- a/arch/x86/kernel/acpi/boot.c
> +++ b/arch/x86/kernel/acpi/boot.c
> @@ -65,6 +65,9 @@ int acpi_fix_pin2_polarity __initdata;
>   static u64 acpi_lapic_addr __initdata = APIC_DEFAULT_PHYS_BASE;
>   #endif
>   
> +static struct acpi_madt_multiproc_wakeup_mailbox *acpi_mp_wake_mailbox;
> +static u64 acpi_mp_wake_mailbox_paddr;
> +
>   #ifdef CONFIG_X86_IO_APIC
>   /*
>    * Locks related to IOAPIC hotplug
> @@ -329,6 +332,52 @@ acpi_parse_lapic_nmi(union acpi_subtable_headers * header, const unsigned long e
>   	return 0;
>   }
>   
> +static void acpi_mp_wake_mailbox_init(void)
> +{
> +	if (acpi_mp_wake_mailbox)
> +		return;
> +
> +	acpi_mp_wake_mailbox = memremap(acpi_mp_wake_mailbox_paddr,
> +			sizeof(*acpi_mp_wake_mailbox), MEMREMAP_WB);
> +}
> +
> +static int acpi_wakeup_cpu(int apicid, unsigned long start_ip)
> +{
> +	u8 timeout = 0xFF;
> +
> +	acpi_mp_wake_mailbox_init();
> +
> +	if (!acpi_mp_wake_mailbox)
> +		return -EINVAL;
> +
> +	/*
> +	 * Mailbox memory is shared between firmware and OS. Firmware will
> +	 * listen on mailbox command address, and once it receives the wakeup
> +	 * command, CPU associated with the given apicid will be booted. So,
> +	 * the value of apic_id and wakeup_vector has to be set before updating
> +	 * the wakeup command. So use WRITE_ONCE to let the compiler know about
> +	 * it and preserve the order of writes.
> +	 */
> +	WRITE_ONCE(acpi_mp_wake_mailbox->apic_id, apicid);
> +	WRITE_ONCE(acpi_mp_wake_mailbox->wakeup_vector, start_ip);
> +	WRITE_ONCE(acpi_mp_wake_mailbox->command, ACPI_MP_WAKE_COMMAND_WAKEUP);
> +
> +	/*
> +	 * After writing wakeup command, wait for maximum timeout of 0xFF
> +	 * for firmware to reset the command address back zero to indicate
> +	 * the successful reception of command.
> +	 * NOTE: 255 as timeout value is decided based on our experiments.
> +	 *
> +	 * XXX: Change the timeout once ACPI specification comes up with
> +	 *      standard maximum timeout value.
> +	 */
> +	while (READ_ONCE(acpi_mp_wake_mailbox->command) && timeout--)
> +		cpu_relax();
> +
> +	/* If timedout, return error */
> +	return timeout ? 0 : -EIO;
> +}
> +
>   #endif				/*CONFIG_X86_LOCAL_APIC */
>   
>   #ifdef CONFIG_X86_IO_APIC
> @@ -1086,6 +1135,30 @@ static int __init acpi_parse_madt_lapic_entries(void)
>   	}
>   	return 0;
>   }
> +
> +static int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
> +				      const unsigned long end)
> +{
> +	struct acpi_madt_multiproc_wakeup *mp_wake;
> +
> +	if (acpi_mp_wake_mailbox)
> +		return -EINVAL;
> +
> +	if (!IS_ENABLED(CONFIG_SMP))
> +		return -ENODEV;
> +
> +	mp_wake = (struct acpi_madt_multiproc_wakeup *) header;
> +	if (BAD_MADT_ENTRY(mp_wake, end))
> +		return -EINVAL;
> +
> +	acpi_table_print_madt_entry(&header->common);
> +
> +	acpi_mp_wake_mailbox_paddr = mp_wake->base_address;
> +
> +	acpi_wake_cpu_handler_update(acpi_wakeup_cpu);
> +
> +	return 0;
> +}
>   #endif				/* CONFIG_X86_LOCAL_APIC */
>   
>   #ifdef	CONFIG_X86_IO_APIC
> @@ -1284,6 +1357,12 @@ static void __init acpi_process_madt(void)
>   
>   				smp_found_config = 1;
>   			}
> +
> +			/*
> +			 * Parse MADT MP Wake entry.
> +			 */
> +			acpi_table_parse_madt(ACPI_MADT_TYPE_MULTIPROC_WAKEUP,
> +					      acpi_parse_mp_wake, 1);
>   		}
>   		if (error == -EINVAL) {
>   			/*
> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> index 4f26700f314d..f1b90a4b89e8 100644
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -2554,6 +2554,14 @@ u32 x86_msi_msg_get_destid(struct msi_msg *msg, bool extid)
>   }
>   EXPORT_SYMBOL_GPL(x86_msi_msg_get_destid);
>   
> +void __init acpi_wake_cpu_handler_update(wakeup_cpu_handler handler)
> +{
> +	struct apic **drv;
> +
> +	for (drv = __apicdrivers; drv < __apicdrivers_end; drv++)
> +		(*drv)->wakeup_secondary_cpu = handler;
> +}
> +
>   /*
>    * Override the generic EOI implementation with an optimized version.
>    * Only called during early boot when only one CPU is active and with
>
Rafael J. Wysocki May 10, 2021, 4:55 p.m. UTC | #2 
On Mon, May 10, 2021 at 6:33 PM Kuppuswamy, Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>
> Hi Rafael/Thomas,
>
> On 4/25/21 7:39 PM, Kuppuswamy Sathyanarayanan wrote:
> > As per ACPI specification r6.4, sec 5.2.12.19, a new sub
> > structure – multiprocessor wake-up structure - is added to the
> > ACPI Multiple APIC Description Table (MADT) to describe the
> > information of the mailbox. If a platform firmware produces the
> > multiprocessor wake-up structure, then OS may use this new
> > mailbox-based mechanism to wake up the APs.
> >
> > Add ACPI MADT wake table parsing support for x86 platform and if
> > MADT wake table is present, update apic->wakeup_secondary_cpu with
> > new API which uses MADT wake mailbox to wake-up CPU.
>
> Gentle ping. Any comments on this patch? I think I have addressed
> the concerns raised by you in previous version.
>
> >
> > Co-developed-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > Reviewed-by: Andi Kleen <ak@linux.intel.com>
> > ---
> >   arch/x86/include/asm/apic.h |  3 ++
> >   arch/x86/kernel/acpi/boot.c | 79 +++++++++++++++++++++++++++++++++++++
> >   arch/x86/kernel/apic/apic.c |  8 ++++
> >   3 files changed, 90 insertions(+)
> >
> > diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
> > index 412b51e059c8..3e94e1f402ea 100644
> > --- a/arch/x86/include/asm/apic.h
> > +++ b/arch/x86/include/asm/apic.h
> > @@ -487,6 +487,9 @@ static inline unsigned int read_apic_id(void)
> >       return apic->get_apic_id(reg);
> >   }
> >
> > +typedef int (*wakeup_cpu_handler)(int apicid, unsigned long start_eip);
> > +extern void acpi_wake_cpu_handler_update(wakeup_cpu_handler handler);
> > +
> >   extern int default_apic_id_valid(u32 apicid);
> >   extern int default_acpi_madt_oem_check(char *, char *);
> >   extern void default_setup_apic_routing(void);
> > diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
> > index 14cd3186dc77..fce2aa7d718f 100644
> > --- a/arch/x86/kernel/acpi/boot.c
> > +++ b/arch/x86/kernel/acpi/boot.c
> > @@ -65,6 +65,9 @@ int acpi_fix_pin2_polarity __initdata;
> >   static u64 acpi_lapic_addr __initdata = APIC_DEFAULT_PHYS_BASE;
> >   #endif
> >
> > +static struct acpi_madt_multiproc_wakeup_mailbox *acpi_mp_wake_mailbox;
> > +static u64 acpi_mp_wake_mailbox_paddr;
> > +
> >   #ifdef CONFIG_X86_IO_APIC
> >   /*
> >    * Locks related to IOAPIC hotplug
> > @@ -329,6 +332,52 @@ acpi_parse_lapic_nmi(union acpi_subtable_headers * header, const unsigned long e
> >       return 0;
> >   }
> >
> > +static void acpi_mp_wake_mailbox_init(void)
> > +{
> > +     if (acpi_mp_wake_mailbox)
> > +             return;
> > +
> > +     acpi_mp_wake_mailbox = memremap(acpi_mp_wake_mailbox_paddr,
> > +                     sizeof(*acpi_mp_wake_mailbox), MEMREMAP_WB);
> > +}
> > +
> > +static int acpi_wakeup_cpu(int apicid, unsigned long start_ip)
> > +{
> > +     u8 timeout = 0xFF;
> > +
> > +     acpi_mp_wake_mailbox_init();
> > +
> > +     if (!acpi_mp_wake_mailbox)
> > +             return -EINVAL;
> > +
> > +     /*
> > +      * Mailbox memory is shared between firmware and OS. Firmware will
> > +      * listen on mailbox command address, and once it receives the wakeup
> > +      * command, CPU associated with the given apicid will be booted. So,
> > +      * the value of apic_id and wakeup_vector has to be set before updating
> > +      * the wakeup command. So use WRITE_ONCE to let the compiler know about
> > +      * it and preserve the order of writes.
> > +      */
> > +     WRITE_ONCE(acpi_mp_wake_mailbox->apic_id, apicid);
> > +     WRITE_ONCE(acpi_mp_wake_mailbox->wakeup_vector, start_ip);
> > +     WRITE_ONCE(acpi_mp_wake_mailbox->command, ACPI_MP_WAKE_COMMAND_WAKEUP);
> > +
> > +     /*
> > +      * After writing wakeup command, wait for maximum timeout of 0xFF
> > +      * for firmware to reset the command address back zero to indicate
> > +      * the successful reception of command.
> > +      * NOTE: 255 as timeout value is decided based on our experiments.
> > +      *
> > +      * XXX: Change the timeout once ACPI specification comes up with
> > +      *      standard maximum timeout value.
> > +      */
> > +     while (READ_ONCE(acpi_mp_wake_mailbox->command) && timeout--)
> > +             cpu_relax();
> > +
> > +     /* If timedout, return error */
> > +     return timeout ? 0 : -EIO;

I'm not sure how my comment regarding the fact that for a given CPU
this function is only usable once has been addressed.

While it may not be a practical concern in the use case that you are
after (TDX), this is a generic mechanism and it needs to cover other
possible usage scenarios.

> > +}
> > +
> >   #endif                              /*CONFIG_X86_LOCAL_APIC */
> >
> >   #ifdef CONFIG_X86_IO_APIC
> > @@ -1086,6 +1135,30 @@ static int __init acpi_parse_madt_lapic_entries(void)
> >       }
> >       return 0;
> >   }
> > +
> > +static int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
> > +                                   const unsigned long end)
> > +{
> > +     struct acpi_madt_multiproc_wakeup *mp_wake;
> > +
> > +     if (acpi_mp_wake_mailbox)
> > +             return -EINVAL;
> > +
> > +     if (!IS_ENABLED(CONFIG_SMP))
> > +             return -ENODEV;
> > +
> > +     mp_wake = (struct acpi_madt_multiproc_wakeup *) header;
> > +     if (BAD_MADT_ENTRY(mp_wake, end))
> > +             return -EINVAL;
> > +
> > +     acpi_table_print_madt_entry(&header->common);
> > +
> > +     acpi_mp_wake_mailbox_paddr = mp_wake->base_address;
> > +
> > +     acpi_wake_cpu_handler_update(acpi_wakeup_cpu);
> > +
> > +     return 0;
> > +}
> >   #endif                              /* CONFIG_X86_LOCAL_APIC */
> >
> >   #ifdef      CONFIG_X86_IO_APIC
> > @@ -1284,6 +1357,12 @@ static void __init acpi_process_madt(void)
> >
> >                               smp_found_config = 1;
> >                       }
> > +
> > +                     /*
> > +                      * Parse MADT MP Wake entry.
> > +                      */
> > +                     acpi_table_parse_madt(ACPI_MADT_TYPE_MULTIPROC_WAKEUP,
> > +                                           acpi_parse_mp_wake, 1);
> >               }
> >               if (error == -EINVAL) {
> >                       /*
> > diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> > index 4f26700f314d..f1b90a4b89e8 100644
> > --- a/arch/x86/kernel/apic/apic.c
> > +++ b/arch/x86/kernel/apic/apic.c
> > @@ -2554,6 +2554,14 @@ u32 x86_msi_msg_get_destid(struct msi_msg *msg, bool extid)
> >   }
> >   EXPORT_SYMBOL_GPL(x86_msi_msg_get_destid);
> >
> > +void __init acpi_wake_cpu_handler_update(wakeup_cpu_handler handler)
> > +{
> > +     struct apic **drv;
> > +
> > +     for (drv = __apicdrivers; drv < __apicdrivers_end; drv++)
> > +             (*drv)->wakeup_secondary_cpu = handler;
> > +}
> > +
> >   /*
> >    * Override the generic EOI implementation with an optimized version.
> >    * Only called during early boot when only one CPU is active and with
> >
>
> --
> Sathyanarayanan Kuppuswamy
> Linux Kernel Developer
Kuppuswamy Sathyanarayanan May 10, 2021, 5:10 p.m. UTC | #3 
On 5/10/21 9:55 AM, Rafael J. Wysocki wrote:
> I'm not sure how my comment regarding the fact that for a given CPU
> this function is only usable once has been addressed.
> 
> While it may not be a practical concern in the use case that you are
> after (TDX), this is a generic mechanism and it needs to cover other
> possible usage scenarios.

For the same CPU, if we try to use mailbox again, firmware will not
respond to it. So the command will timeout and return error.
Andi Kleen May 10, 2021, 5:22 p.m. UTC | #4 
On Mon, May 10, 2021 at 10:10:24AM -0700, Kuppuswamy, Sathyanarayanan wrote:
> 
> 
> On 5/10/21 9:55 AM, Rafael J. Wysocki wrote:
> > I'm not sure how my comment regarding the fact that for a given CPU
> > this function is only usable once has been addressed.
> > 
> > While it may not be a practical concern in the use case that you are
> > after (TDX), this is a generic mechanism and it needs to cover other
> > possible usage scenarios.
> 
> For the same CPU, if we try to use mailbox again, firmware will not
> respond to it. So the command will timeout and return error.

Right because the firmware code doesn't run anymore.

The only possibility would be for Linux to put back some code that spins
and waits again, but that would be quite pointless and wasteful.

-Andi
Rafael J. Wysocki May 10, 2021, 5:23 p.m. UTC | #5 
On Mon, May 10, 2021 at 7:10 PM Kuppuswamy, Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>
>
>
> On 5/10/21 9:55 AM, Rafael J. Wysocki wrote:
> > I'm not sure how my comment regarding the fact that for a given CPU
> > this function is only usable once has been addressed.
> >
> > While it may not be a practical concern in the use case that you are
> > after (TDX), this is a generic mechanism and it needs to cover other
> > possible usage scenarios.
>
> For the same CPU, if we try to use mailbox again, firmware will not
> respond to it.

Well, theoretically, but what if it checks the mailbox every time for
all CPUs?  Or similar?

> So the command will timeout and return error.

If the firmware behaves as expected, but what if it doesn't?
Rafael J. Wysocki May 10, 2021, 5:24 p.m. UTC | #6 
On Mon, May 10, 2021 at 7:22 PM Andi Kleen <ak@linux.intel.com> wrote:
>
> On Mon, May 10, 2021 at 10:10:24AM -0700, Kuppuswamy, Sathyanarayanan wrote:
> >
> >
> > On 5/10/21 9:55 AM, Rafael J. Wysocki wrote:
> > > I'm not sure how my comment regarding the fact that for a given CPU
> > > this function is only usable once has been addressed.
> > >
> > > While it may not be a practical concern in the use case that you are
> > > after (TDX), this is a generic mechanism and it needs to cover other
> > > possible usage scenarios.
> >
> > For the same CPU, if we try to use mailbox again, firmware will not
> > respond to it. So the command will timeout and return error.
>
> Right because the firmware code doesn't run anymore.
>
> The only possibility would be for Linux to put back some code that spins
> and waits again, but that would be quite pointless and wasteful.

The wakeup function can return an error when it is called for the
second time on the same CPU.
Kuppuswamy Sathyanarayanan May 10, 2021, 9:15 p.m. UTC | #7 
On 5/10/21 10:24 AM, Rafael J. Wysocki wrote:
> The wakeup function can return an error when it is called for the
> second time on the same CPU.

To do this, we can only maintain the wakeup status of the CPUs. Can
you check whether following physid_mask based status maintenance is
acceptable?

--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -67,6 +67,7 @@ static u64 acpi_lapic_addr __initdata = APIC_DEFAULT_PHYS_BASE;

  static struct acpi_madt_multiproc_wakeup_mailbox *acpi_mp_wake_mailbox;
  static u64 acpi_mp_wake_mailbox_paddr;
+static physid_mask_t apic_id_wakemap = PHYSID_MASK_NONE;

  #ifdef CONFIG_X86_IO_APIC
  /*
@@ -347,6 +348,13 @@ static int acpi_wakeup_cpu(int apicid, unsigned long start_ip)

         acpi_mp_wake_mailbox_init();

+       /* Check if the given CPU (apicid) is already awake */
+       if (physid_isset(apicid, apic_id_wakemap)) {
+               pr_err("APIC ID %x is already awake, so failed to wakeup\n",
+                               apicid);
+               return -EINVAL;
+       }
+
         if (!acpi_mp_wake_mailbox)
                 return -EINVAL;

@@ -374,8 +382,18 @@ static int acpi_wakeup_cpu(int apicid, unsigned long start_ip)
         while (READ_ONCE(acpi_mp_wake_mailbox->command) && timeout--)
                 cpu_relax();

-       /* If timedout, return error */
-       return timeout ? 0 : -EIO;
+       if (timeout) {
+               /*
+                * If the CPU wakeup process is successful, store the
+                * status in apic_id_wakemap to prevent re-wakeup
+                * requests.
+                */
+               physid_set(apicid, apic_id_wakemap);
+               return 0;
+       }
+
+       /* If timed out (timeout == 0), return error */
+       return -EIO;
  }
Rafael J. Wysocki May 11, 2021, 10:45 a.m. UTC | #8 
On Mon, May 10, 2021 at 11:15 PM Kuppuswamy, Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>
>
>
> On 5/10/21 10:24 AM, Rafael J. Wysocki wrote:
> > The wakeup function can return an error when it is called for the
> > second time on the same CPU.
>
> To do this, we can only maintain the wakeup status of the CPUs. Can
> you check whether following physid_mask based status maintenance is
> acceptable?

It would work for me except for a couple of nits below.

> --- a/arch/x86/kernel/acpi/boot.c
> +++ b/arch/x86/kernel/acpi/boot.c
> @@ -67,6 +67,7 @@ static u64 acpi_lapic_addr __initdata = APIC_DEFAULT_PHYS_BASE;
>
>   static struct acpi_madt_multiproc_wakeup_mailbox *acpi_mp_wake_mailbox;
>   static u64 acpi_mp_wake_mailbox_paddr;
> +static physid_mask_t apic_id_wakemap = PHYSID_MASK_NONE;
>
>   #ifdef CONFIG_X86_IO_APIC
>   /*
> @@ -347,6 +348,13 @@ static int acpi_wakeup_cpu(int apicid, unsigned long start_ip)
>
>          acpi_mp_wake_mailbox_init();
>
> +       /* Check if the given CPU (apicid) is already awake */

The reason why is that the wakeup mechanism used here is only usable
once per CPU by the spec, so I would add this information to the
comment.  Maybe something like "According to the ACPI specification
(ACPI 6.4, Section ...), the mailbox-based wakeup mechanism cannot be
used more than once for the same CPU, so avoid doing that."

> +       if (physid_isset(apicid, apic_id_wakemap)) {
> +               pr_err("APIC ID %x is already awake, so failed to wakeup\n",
> +                               apicid);

And I would reword the message like this "CPU already awake (APIC ID
%x), skipping wakeup\n".

> +               return -EINVAL;
> +       }
> +
>          if (!acpi_mp_wake_mailbox)

Note, though, that instead of having this additional flag, you may as
well create a mask that is fully populated initially and clear the IDs
of the woken-up CPUs in it.  Then, you'd only need one check here
instead of two.

>                  return -EINVAL;
>
> @@ -374,8 +382,18 @@ static int acpi_wakeup_cpu(int apicid, unsigned long start_ip)
>          while (READ_ONCE(acpi_mp_wake_mailbox->command) && timeout--)
>                  cpu_relax();
>
> -       /* If timedout, return error */
> -       return timeout ? 0 : -EIO;
> +       if (timeout) {
> +               /*
> +                * If the CPU wakeup process is successful, store the
> +                * status in apic_id_wakemap to prevent re-wakeup
> +                * requests.
> +                */
> +               physid_set(apicid, apic_id_wakemap);
> +               return 0;
> +       }
> +
> +       /* If timed out (timeout == 0), return error */
> +       return -EIO;
>   }
>
>
> --
> Sathyanarayanan Kuppuswamy
> Linux Kernel Developer
diff mbox series

Patch

diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index 412b51e059c8..3e94e1f402ea 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -487,6 +487,9 @@  static inline unsigned int read_apic_id(void)
 	return apic->get_apic_id(reg);
 }
 
+typedef int (*wakeup_cpu_handler)(int apicid, unsigned long start_eip);
+extern void acpi_wake_cpu_handler_update(wakeup_cpu_handler handler);
+
 extern int default_apic_id_valid(u32 apicid);
 extern int default_acpi_madt_oem_check(char *, char *);
 extern void default_setup_apic_routing(void);
diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 14cd3186dc77..fce2aa7d718f 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -65,6 +65,9 @@  int acpi_fix_pin2_polarity __initdata;
 static u64 acpi_lapic_addr __initdata = APIC_DEFAULT_PHYS_BASE;
 #endif
 
+static struct acpi_madt_multiproc_wakeup_mailbox *acpi_mp_wake_mailbox;
+static u64 acpi_mp_wake_mailbox_paddr;
+
 #ifdef CONFIG_X86_IO_APIC
 /*
  * Locks related to IOAPIC hotplug
@@ -329,6 +332,52 @@  acpi_parse_lapic_nmi(union acpi_subtable_headers * header, const unsigned long e
 	return 0;
 }
 
+static void acpi_mp_wake_mailbox_init(void)
+{
+	if (acpi_mp_wake_mailbox)
+		return;
+
+	acpi_mp_wake_mailbox = memremap(acpi_mp_wake_mailbox_paddr,
+			sizeof(*acpi_mp_wake_mailbox), MEMREMAP_WB);
+}
+
+static int acpi_wakeup_cpu(int apicid, unsigned long start_ip)
+{
+	u8 timeout = 0xFF;
+
+	acpi_mp_wake_mailbox_init();
+
+	if (!acpi_mp_wake_mailbox)
+		return -EINVAL;
+
+	/*
+	 * Mailbox memory is shared between firmware and OS. Firmware will
+	 * listen on mailbox command address, and once it receives the wakeup
+	 * command, CPU associated with the given apicid will be booted. So,
+	 * the value of apic_id and wakeup_vector has to be set before updating
+	 * the wakeup command. So use WRITE_ONCE to let the compiler know about
+	 * it and preserve the order of writes.
+	 */
+	WRITE_ONCE(acpi_mp_wake_mailbox->apic_id, apicid);
+	WRITE_ONCE(acpi_mp_wake_mailbox->wakeup_vector, start_ip);
+	WRITE_ONCE(acpi_mp_wake_mailbox->command, ACPI_MP_WAKE_COMMAND_WAKEUP);
+
+	/*
+	 * After writing wakeup command, wait for maximum timeout of 0xFF
+	 * for firmware to reset the command address back zero to indicate
+	 * the successful reception of command.
+	 * NOTE: 255 as timeout value is decided based on our experiments.
+	 *
+	 * XXX: Change the timeout once ACPI specification comes up with
+	 *      standard maximum timeout value.
+	 */
+	while (READ_ONCE(acpi_mp_wake_mailbox->command) && timeout--)
+		cpu_relax();
+
+	/* If timedout, return error */
+	return timeout ? 0 : -EIO;
+}
+
 #endif				/*CONFIG_X86_LOCAL_APIC */
 
 #ifdef CONFIG_X86_IO_APIC
@@ -1086,6 +1135,30 @@  static int __init acpi_parse_madt_lapic_entries(void)
 	}
 	return 0;
 }
+
+static int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
+				      const unsigned long end)
+{
+	struct acpi_madt_multiproc_wakeup *mp_wake;
+
+	if (acpi_mp_wake_mailbox)
+		return -EINVAL;
+
+	if (!IS_ENABLED(CONFIG_SMP))
+		return -ENODEV;
+
+	mp_wake = (struct acpi_madt_multiproc_wakeup *) header;
+	if (BAD_MADT_ENTRY(mp_wake, end))
+		return -EINVAL;
+
+	acpi_table_print_madt_entry(&header->common);
+
+	acpi_mp_wake_mailbox_paddr = mp_wake->base_address;
+
+	acpi_wake_cpu_handler_update(acpi_wakeup_cpu);
+
+	return 0;
+}
 #endif				/* CONFIG_X86_LOCAL_APIC */
 
 #ifdef	CONFIG_X86_IO_APIC
@@ -1284,6 +1357,12 @@  static void __init acpi_process_madt(void)
 
 				smp_found_config = 1;
 			}
+
+			/*
+			 * Parse MADT MP Wake entry.
+			 */
+			acpi_table_parse_madt(ACPI_MADT_TYPE_MULTIPROC_WAKEUP,
+					      acpi_parse_mp_wake, 1);
 		}
 		if (error == -EINVAL) {
 			/*
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 4f26700f314d..f1b90a4b89e8 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -2554,6 +2554,14 @@  u32 x86_msi_msg_get_destid(struct msi_msg *msg, bool extid)
 }
 EXPORT_SYMBOL_GPL(x86_msi_msg_get_destid);
 
+void __init acpi_wake_cpu_handler_update(wakeup_cpu_handler handler)
+{
+	struct apic **drv;
+
+	for (drv = __apicdrivers; drv < __apicdrivers_end; drv++)
+		(*drv)->wakeup_secondary_cpu = handler;
+}
+
 /*
  * Override the generic EOI implementation with an optimized version.
  * Only called during early boot when only one CPU is active and with