[PATCH 0/3] Add multiprocessor wake-up support

[PATCH 0/3] Add multiprocessor wake-up support

[Kuppuswamy Sathyanarayanan]

Add multiprocessor wakeup support using MADT ACPI table for x86
platforms. It uses mailbox based mechanism to wake up the APs. You
can get more details about the ACPI table and mailbox protocol in
Guest-Host-Communication Interface (GHCI) for Intel Trust Domain
Extensions (Intel TDX) specification document (sec 4.1)

https://software.intel.com/content/dam/develop/external/us/en/documents/intel-tdx-guest-hypervisor-communication-interface.pdf

Kuppuswamy Sathyanarayanan (3):
  ACPICA: ACPI 6.4: MADT: add Multiprocessor Wakeup Mailbox Structure
  ACPI/table: Print MADT Wake table information
  x86/acpi, x86/boot: Add multiprocessor wake-up support

 arch/x86/include/asm/apic.h     |  3 ++
 arch/x86/kernel/acpi/boot.c     | 56 +++++++++++++++++++++++++++++++++
 arch/x86/kernel/apic/probe_32.c |  8 +++++
 arch/x86/kernel/apic/probe_64.c |  8 +++++
 drivers/acpi/tables.c           | 11 +++++++
 include/acpi/actbl2.h           | 14 +++++++++
 6 files changed, 100 insertions(+)

-- 
2.25.1

[PATCH 1/3] ACPICA: ACPI 6.4: MADT: add Multiprocessor Wakeup Mailbox Structure

[Kuppuswamy Sathyanarayanan]

ACPICA commit f1ee04207a212f6c519441e7e25397649ebc4cea

Add Multiprocessor Wakeup Mailbox Structure definition. It is useful
in parsing MADT Wake table.

Link: https://github.com/acpica/acpica/commit/f1ee0420
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Signed-off-by: Bob Moore <robert.moore@intel.com>
Signed-off-by: Erik Kaneda <erik.kaneda@intel.com>
---
 include/acpi/actbl2.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
index b2362600b9ff..7dce422f6119 100644
--- a/include/acpi/actbl2.h
+++ b/include/acpi/actbl2.h
@@ -733,6 +733,20 @@ struct acpi_madt_multiproc_wakeup {
 	u64 base_address;
 };
 
+#define ACPI_MULTIPROC_WAKEUP_MB_OS_SIZE	2032
+#define ACPI_MULTIPROC_WAKEUP_MB_FIRMWARE_SIZE	2048
+
+struct acpi_madt_multiproc_wakeup_mailbox {
+	u16 command;
+	u16 reserved;		/* reserved - must be zero */
+	u32 apic_id;
+	u64 wakeup_vector;
+	u8 reserved_os[ACPI_MULTIPROC_WAKEUP_MB_OS_SIZE];	/* reserved for OS use */
+	u8 reserved_firmware[ACPI_MULTIPROC_WAKEUP_MB_FIRMWARE_SIZE];	/* reserved for firmware use */
+};
+
+#define ACPI_MP_WAKE_COMMAND_WAKEUP    1
+
 /*
  * Common flags fields for MADT subtables
  */
-- 
2.25.1

Re: [PATCH 1/3] ACPICA: ACPI 6.4: MADT: add Multiprocessor Wakeup Mailbox Structure

[Dave Hansen]

On 4/22/21 12:24 PM, Kuppuswamy Sathyanarayanan wrote:
> ACPICA commit f1ee04207a212f6c519441e7e25397649ebc4cea
> 
> Add Multiprocessor Wakeup Mailbox Structure definition. It is useful
> in parsing MADT Wake table.
> 
> Link: https://github.com/acpica/acpica/commit/f1ee0420
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Signed-off-by: Bob Moore <robert.moore@intel.com>
> Signed-off-by: Erik Kaneda <erik.kaneda@intel.com>

This SoB chain doesn't look right.  This is what it would have been if
You sent it to Bob who sent it to Erik, who submitted it.

Re: [PATCH 1/3] ACPICA: ACPI 6.4: MADT: add Multiprocessor Wakeup Mailbox Structure

[Kuppuswamy, Sathyanarayanan]

On 4/22/21 12:37 PM, Dave Hansen wrote:
> On 4/22/21 12:24 PM, Kuppuswamy Sathyanarayanan wrote:
>> ACPICA commit f1ee04207a212f6c519441e7e25397649ebc4cea
>>
>> Add Multiprocessor Wakeup Mailbox Structure definition. It is useful
>> in parsing MADT Wake table.
>>
>> Link: https://github.com/acpica/acpica/commit/f1ee0420
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> Signed-off-by: Bob Moore <robert.moore@intel.com>
>> Signed-off-by: Erik Kaneda <erik.kaneda@intel.com>
> 
> This SoB chain doesn't look right.  This is what it would have been if
> You sent it to Bob who sent it to Erik, who submitted it.
Internally, its submitted to Bob and Erik for ACPICA merge.
I think Sign-off is added to track it.
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH 1/3] ACPICA: ACPI 6.4: MADT: add Multiprocessor Wakeup Mailbox Structure

[Borislav Petkov]

On Thu, Apr 22, 2021 at 12:46:13PM -0700, Kuppuswamy, Sathyanarayanan wrote:
> > This SoB chain doesn't look right.  This is what it would have been if
> > You sent it to Bob who sent it to Erik, who submitted it.
> Internally, its submitted to Bob and Erik for ACPICA merge.
> I think Sign-off is added to track it.

This is not how this works - when Erik/Bob merge it, *then* they add
their SOB. Right now it should have only your SOB.

Documentation/process/submitting-patches.rst, section "Sign your work -
the Developer's Certificate of Origin"

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

RE: [PATCH 1/3] ACPICA: ACPI 6.4: MADT: add Multiprocessor Wakeup Mailbox Structure

[Kaneda, Erik]

> -----Original Message-----
> From: Borislav Petkov <bp@alien8.de>
> Sent: Thursday, April 22, 2021 12:56 PM
> To: Kuppuswamy, Sathyanarayanan
> <sathyanarayanan.kuppuswamy@linux.intel.com>
> Cc: Hansen, Dave <dave.hansen@intel.com>; Rafael J Wysocki
> <rjw@rjwysocki.net>; Thomas Gleixner <tglx@linutronix.de>; Ingo Molnar
> <mingo@redhat.com>; H . Peter Anvin <hpa@zytor.com>; Peter Zijlstra
> <peterz@infradead.org>; Len Brown <lenb@kernel.org>; Moore, Robert
> <robert.moore@intel.com>; Kaneda, Erik <erik.kaneda@intel.com>; linux-
> acpi@vger.kernel.org; devel@acpica.org; linux-kernel@vger.kernel.org;
> x86@kernel.org
> Subject: Re: [PATCH 1/3] ACPICA: ACPI 6.4: MADT: add Multiprocessor
> Wakeup Mailbox Structure
> 
> On Thu, Apr 22, 2021 at 12:46:13PM -0700, Kuppuswamy, Sathyanarayanan
> wrote:
> > > This SoB chain doesn't look right.  This is what it would have been if
> > > You sent it to Bob who sent it to Erik, who submitted it.
> > Internally, its submitted to Bob and Erik for ACPICA merge.
> > I think Sign-off is added to track it.
> 
> This is not how this works - when Erik/Bob merge it, *then* they add
> their SOB. Right now it should have only your SOB.

Sorry about that. The script to format the ACPICA upstream to Linux ACPICA automatically adds signed off-by tags from me and Bob to the patch. This would work if we go through the normal process of running the Linux script after we do an ACPICA release. We decided to submit this earlier to meet Sathya's deadline.

Sathya, Please drop these lines from this patch and the SVKL patch.

Erik

> 
> Documentation/process/submitting-patches.rst, section "Sign your work -
> the Developer's Certificate of Origin"
> 
> Thx.
> 
> --
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 1/3] ACPICA: ACPI 6.4: MADT: add Multiprocessor Wakeup Mailbox Structure

[Kuppuswamy, Sathyanarayanan]

On 4/22/21 1:51 PM, Kaneda, Erik wrote:
>> This is not how this works - when Erik/Bob merge it,*then*  they add
>> their SOB. Right now it should have only your SOB.
> Sorry about that. The script to format the ACPICA upstream to Linux ACPICA automatically adds signed off-by tags from me and Bob to the patch. This would work if we go through the normal process of running the Linux script after we do an ACPICA release. We decided to submit this earlier to meet Sathya's deadline.
> 
> Sathya, Please drop these lines from this patch and the SVKL patch.

Will remove it.

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

[PATCH 2/3] ACPI/table: Print MADT Wake table information

[Kuppuswamy Sathyanarayanan]

When MADT is parsed, print MADT Wake table information as
debug message. It will be useful to debug CPU boot issues
related to MADT wake table.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/acpi/tables.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index 9d581045acff..206df4ad8b2b 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -207,6 +207,17 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
 		}
 		break;
 
+	case ACPI_MADT_TYPE_MULTIPROC_WAKEUP:
+		{
+			struct acpi_madt_multiproc_wakeup *p;
+
+			p = (struct acpi_madt_multiproc_wakeup *) header;
+
+			pr_debug("MP Wake (Mailbox version[%d] base_address[%llx])\n",
+				 p->mailbox_version, p->base_address);
+		}
+		break;
+
 	default:
 		pr_warn("Found unsupported MADT entry (type = 0x%x)\n",
 			header->type);
-- 
2.25.1

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

[Kuppuswamy Sathyanarayanan]

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     | 56 +++++++++++++++++++++++++++++++++
 arch/x86/kernel/apic/probe_32.c |  8 +++++
 arch/x86/kernel/apic/probe_64.c |  8 +++++
 4 files changed, 75 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..a4a6b97910e1 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,29 @@ 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)
+{
+	acpi_mp_wake_mailbox_init();
+
+	if (!acpi_mp_wake_mailbox)
+		return -EINVAL;
+
+	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);
+
+	return 0;
+}
+
 #endif				/*CONFIG_X86_LOCAL_APIC */
 
 #ifdef CONFIG_X86_IO_APIC
@@ -1086,6 +1112,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 +1334,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/probe_32.c b/arch/x86/kernel/apic/probe_32.c
index a61f642b1b90..d450014841b2 100644
--- a/arch/x86/kernel/apic/probe_32.c
+++ b/arch/x86/kernel/apic/probe_32.c
@@ -207,3 +207,11 @@ int __init default_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
 	}
 	return 0;
 }
+
+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;
+}
diff --git a/arch/x86/kernel/apic/probe_64.c b/arch/x86/kernel/apic/probe_64.c
index c46720f185c0..986dbb68d3c4 100644
--- a/arch/x86/kernel/apic/probe_64.c
+++ b/arch/x86/kernel/apic/probe_64.c
@@ -50,3 +50,11 @@ int __init default_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
 	}
 	return 0;
 }
+
+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;
+}
-- 
2.25.1

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

[Thomas Gleixner]

Kuppuswamy!

On Thu, Apr 22 2021 at 12:24, Kuppuswamy Sathyanarayanan wrote:
> +static int acpi_wakeup_cpu(int apicid, unsigned long start_ip)
> +{
> +	acpi_mp_wake_mailbox_init();
> +
> +	if (!acpi_mp_wake_mailbox)
> +		return -EINVAL;
> +
> +	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);

What's the point of using WRITE_ONCE() here? Where is the required
READ_ONCE() counterpart and the required documentation in form of a
comment?

> +static int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
> +				      const unsigned long end)
> +{
...
> +	acpi_wake_cpu_handler_update(acpi_wakeup_cpu);
...

> +++ b/arch/x86/kernel/apic/probe_32.c
> @@ -207,3 +207,11 @@ int __init default_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
>  	}
>  	return 0;
>  }
> +
> +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;
> +}
> diff --git a/arch/x86/kernel/apic/probe_64.c b/arch/x86/kernel/apic/probe_64.c
> index c46720f185c0..986dbb68d3c4 100644
> --- a/arch/x86/kernel/apic/probe_64.c
> +++ b/arch/x86/kernel/apic/probe_64.c
> @@ -50,3 +50,11 @@ int __init default_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
>  	}
>  	return 0;
>  }
> +
> +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;
> +}

What's the reason for having two verbatim copies of the same function
which has no dependency on CONFIG_*_32/64 at all?

Thanks,

        tglx

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

[Kuppuswamy, Sathyanarayanan]

On 4/22/21 4:04 PM, Thomas Gleixner wrote:
> Kuppuswamy!
> 
> On Thu, Apr 22 2021 at 12:24, Kuppuswamy Sathyanarayanan wrote:
>> +static int acpi_wakeup_cpu(int apicid, unsigned long start_ip)
>> +{
>> +	acpi_mp_wake_mailbox_init();
>> +
>> +	if (!acpi_mp_wake_mailbox)
>> +		return -EINVAL;
>> +
>> +	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);
> 
> What's the point of using WRITE_ONCE() here? Where is the required
> READ_ONCE() counterpart and the required documentation in form of a
> comment?

mailbox memory is shared between firmware and OS. We use WRITE_ONCE to notify
compiler about it, and also to preserve the order of writes to these
addresses. As per MADT Wake protocol, once OS update the mailbox command
address with ACPI_MP_*_WAKEUP command, firmware will be bring the AP out sleep
state and trigger the boot process.
Since its a one way communication, we don't need to read the mailbox address
again. So there is no corresponding READ_ONCE() call.

I will add some comments about it in next version.

> 
>> +static int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
>> +				      const unsigned long end)
>> +{
> ...
>> +	acpi_wake_cpu_handler_update(acpi_wakeup_cpu);
> ...
> 
>> +++ b/arch/x86/kernel/apic/probe_32.c
>> @@ -207,3 +207,11 @@ int __init default_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
>>   	}
>>   	return 0;
>>   }
>> +
>> +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;
>> +}
>> diff --git a/arch/x86/kernel/apic/probe_64.c b/arch/x86/kernel/apic/probe_64.c
>> index c46720f185c0..986dbb68d3c4 100644
>> --- a/arch/x86/kernel/apic/probe_64.c
>> +++ b/arch/x86/kernel/apic/probe_64.c
>> @@ -50,3 +50,11 @@ int __init default_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
>>   	}
>>   	return 0;
>>   }
>> +
>> +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;
>> +}
> 
> What's the reason for having two verbatim copies of the same function
> which has no dependency on CONFIG_*_32/64 at all?

Initially I thought all ACPI related functions are grouped in probe_*.c.
After some review, now I know its incorrect. I will move the function
definition to apic.c

> 
> Thanks,
> 
>          tglx
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer