[PATCH v3 00/13] x86/hyperv/hv_vtl: Use a wakeup mailbox to boot secondary CPUs

[PATCH v3 00/13] x86/hyperv/hv_vtl: Use a wakeup mailbox to boot secondary CPUs

[Ricardo Neri]

Hi,

I have taken over this work from Yunhong Jiang [1]. I have implemented all
the feedback received in his last submission. I think that the acpi,
smpboot, and hyperv portions are in good shape and ready for review by the
x86 maintainers. I did major rework on the DeviceTree bindings and in my
opinion are also ready for review by the maintainer too.

Thanks in advance for your feedback!

---

This patchset adds functionality to use a wakeup mailbox to boot secondary
CPUs in Hyper-V VTL level 2 TDX guests with virtual firmware that describes
hardware using a DeviceTree graph. Although this is the target use case,
the use of the mailbox depends solely on it being enumerated in the
DeviceTree.

On x86 platforms, secondary CPUs are typically booted using INIT assert,
de-assert followed by Start-Up IPI messages. Virtual machines can also make
hypercalls to bring up secondary CPUs to a desired execution state. These
two mechanisms require support from the hypervisor. Confidential computing
VMs in a TDX environment cannot use this mechanism because the hypervisor
is considered an untrusted entity.

Linux already supports the ACPI Multiprocessor Wakeup Structure in which
the guest platform firmware boots the secondary CPUs and transfers control
to the kernel using a mailbox. This mechanism does not need involvement
of the VMM. It can be used in a Hyper-V VTL level 2 TDX guest.

Currently, this mechanism can only be used on x86 platforms with firmware
that supports ACPI. There are platforms that use DeviceTree (e.g., OpenHCL
[2]) instead of ACPI to describe the hardware.

Provided that a Wakeup Mailbox defined in the DeviceTree is compatible in
structure and operation with the ACPI Multiprocessor Wakeup Structure, the
kernel can use common code for both.

This patcheset is structured as follows:

   * Relocate portions of the ACPI Multiprocessor Wakeup Structure code to
     to a common location. (patches 1-3)
   * Add DeviceTree schema and bindings to define a Wakeup Mailbox for
     Intel processors that is compatible with the ACPI Multiprocessor
     Wakeup Structure as well as a new enable-method property for cpu@N
     nodes (patches 4, 6).
   * Add support to parse the enable-method property in the cpu@N nodes of
     DeviceTree graphs for x86 and enable the Wakeup Mailbox if available.
     (patches 5, 7)
   * Prepare Hyper-V VTL2 TDX guests to use the Wakeup Mailbox to boot
     secondary CPUs when available. (patches 8-13)

I have tested this patchset on a Hyper-V host with VTL2 OpenHCL, QEMU, and
physical hardware.

Thanks and BR,
Ricardo

Changes since v2:
  - Only move out of the acpi directory acpi_wakeup_cpu() and its
    accessory variables. Use helper functions to access the mailbox as
    needed. This also fixed the warnings about unused code with CONFIG_
    ACPI=n that Michael reported.
  - Major rework of the DeviceTree bindings and schema. Now there is a
    reserved-memory binding for the mailbox as well as a new x86 CPU
    bindings. Both have `compatible` properties.
  - Rework of the code parsing the DeviceTree bindings for the mailbox.
    Now configuring the mailbox depends solely on its enumeration in the
    DeviceTree and not on Hyper-V VTL2 TDX guest.
  - Do not make reserving the first 1MB of memory optional. It is not
    needed and may introduce bugs.
  - Prepare Hyper-V VTL2 guests to unconditionally use the mailbox in TDX
    environments. If the mailbox is not available, booting secondary CPUs
    will fail gracefully.

Changes since v1:
  - Fix the cover letter's summary phrase.
  - Fix the DT binding document to pass validation.
  - Change the DT binding document to be ACPI independent.
  - Move ACPI-only functions into the #ifdef CONFIG_ACPI.
  - Change dtb_parse_mp_wake() to return mailbox physical address.
  - Rework the hv_is_private_mmio_tdx().
  - Remove unrelated real mode change from the patch that marks mailbox
    page private.
  - Check hv_isolation_type_tdx() instead of wakeup_mailbox_addr in
    hv_vtl_init_platform() because wakeup_mailbox_addr is not parsed yet.
  - Add memory range support to reserve_real_mode.
  - Remove realmode_reserve callback and use the memory range.
  - Move setting the real_mode_header to hv_vtl_init_platform.
  - Update comments and commit messages.
  - Minor style changes.

[1]. https://lore.kernel.org/lkml/20240823232327.2408869-7-yunhong.jiang@linux.intel.com/T/#ma1f56fc7eee585b777829fa7e8bd39cd3e780fe0
[2]. https://openvmm.dev/guide/user_guide/openhcl.html

Ricardo Neri (9):
  x86/acpi: Add a helper function to setup the wakeup mailbox
  x86/acpi: Add a helper function to get a pointer to the wakeup mailbox
  x86/acpi: Move acpi_wakeup_cpu() and helpers to smpboot.c
  dt-bindings: x86: Add CPU bindings for x86
  x86/dt: Parse the `enable-method` property of CPU nodes
  dt-bindings: reserved-memory: Wakeup Mailbox for Intel processors
  x86/dt: Parse the Wakeup Mailbox for Intel processors
  x86/smpboot: Add a helper get the address of the wakeup mailbox
  x86/hyperv/vtl: Use the wakeup mailbox to boot secondary CPUs

Yunhong Jiang (4):
  x86/hyperv/vtl: Set real_mode_header in hv_vtl_init_platform()
  x86/realmode: Make the location of the trampoline configurable
  x86/hyperv/vtl: Setup the 64-bit trampoline for TDX guests
  x86/hyperv/vtl: Mark the wakeup mailbox page as private

 .../reserved-memory/intel,wakeup-mailbox.yaml |  87 +++++++++++
 .../devicetree/bindings/x86/cpus.yaml         |  80 ++++++++++
 arch/x86/hyperv/hv_vtl.c                      |  35 ++++-
 arch/x86/include/asm/smp.h                    |   6 +
 arch/x86/include/asm/x86_init.h               |   3 +
 arch/x86/kernel/acpi/madt_wakeup.c            |  75 +---------
 arch/x86/kernel/devicetree.c                  | 141 +++++++++++++++++-
 arch/x86/kernel/smpboot.c                     |  83 +++++++++++
 arch/x86/kernel/x86_init.c                    |   3 +
 arch/x86/realmode/init.c                      |   7 +-
 10 files changed, 440 insertions(+), 80 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/reserved-memory/intel,wakeup-mailbox.yaml
 create mode 100644 Documentation/devicetree/bindings/x86/cpus.yaml

-- 
2.43.0

[PATCH v3 01/13] x86/acpi: Add a helper function to setup the wakeup mailbox

[Ricardo Neri]

In preparation to move the functionality to wake secondary CPUs up out of
the ACPI code, add a helper function that stores the physical address of
the mailbox and updates the wakeup_secondary_cpu_64() APIC callback.

There is a slight change in behavior: now the APIC callback is updated
before configuring CPU hotplug offline behavior. This is fine as the APIC
callback continues to be updated unconditionally, regardless of the
restriction on CPU offlining.

The wakeup mailbox is only supported for CONFIG_X86_64 and needed only with
CONFIG_SMP=y.

Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
---
Changes since v2:
 - Introduced this patch.

Changes since v1:
 - N/A
---
 arch/x86/include/asm/smp.h         |  4 ++++
 arch/x86/kernel/acpi/madt_wakeup.c | 10 +++++++---
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index 0c1c68039d6f..3622951d2ee0 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -146,6 +146,10 @@ static inline struct cpumask *cpu_l2c_shared_mask(int cpu)
 	return per_cpu(cpu_l2c_shared_map, cpu);
 }
 
+#ifdef CONFIG_X86_64
+void setup_mp_wakeup_mailbox(u64 addr);
+#endif
+
 #else /* !CONFIG_SMP */
 #define wbinvd_on_cpu(cpu)     wbinvd()
 static inline int wbinvd_on_all_cpus(void)
diff --git a/arch/x86/kernel/acpi/madt_wakeup.c b/arch/x86/kernel/acpi/madt_wakeup.c
index f36f28405dcc..04de3db307de 100644
--- a/arch/x86/kernel/acpi/madt_wakeup.c
+++ b/arch/x86/kernel/acpi/madt_wakeup.c
@@ -227,7 +227,7 @@ int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
 
 	acpi_table_print_madt_entry(&header->common);
 
-	acpi_mp_wake_mailbox_paddr = mp_wake->mailbox_address;
+	setup_mp_wakeup_mailbox(mp_wake->mailbox_address);
 
 	if (mp_wake->version >= ACPI_MADT_MP_WAKEUP_VERSION_V1 &&
 	    mp_wake->header.length >= ACPI_MADT_MP_WAKEUP_SIZE_V1) {
@@ -243,7 +243,11 @@ int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
 		acpi_mp_disable_offlining(mp_wake);
 	}
 
-	apic_update_callback(wakeup_secondary_cpu_64, acpi_wakeup_cpu);
-
 	return 0;
 }
+
+void __init setup_mp_wakeup_mailbox(u64 mailbox_paddr)
+{
+	acpi_mp_wake_mailbox_paddr = mailbox_paddr;
+	apic_update_callback(wakeup_secondary_cpu_64, acpi_wakeup_cpu);
+}
-- 
2.43.0

[PATCH v3 02/13] x86/acpi: Add a helper function to get a pointer to the wakeup mailbox

[Ricardo Neri]

In preparation to move the functionality to wake secondary CPUs up out
of the ACPI code, add a helper function to get a pointer to the mailbox.

Use this helper function only in the portions of the code for which the
variable acpi_mp_wake_mailbox will be out of scope once it is relocated
out of the ACPI directory.

The wakeup mailbox is only supported for CONFIG_X86_64 and needed only
with CONFIG_SMP.

Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
---
Changes since v2:
 - Introduced this patch.

Changes since v1:
 - N/A
---
 arch/x86/include/asm/smp.h         |  1 +
 arch/x86/kernel/acpi/madt_wakeup.c | 12 +++++++++---
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index 3622951d2ee0..97bfbd0d24d4 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -148,6 +148,7 @@ static inline struct cpumask *cpu_l2c_shared_mask(int cpu)
 
 #ifdef CONFIG_X86_64
 void setup_mp_wakeup_mailbox(u64 addr);
+struct acpi_madt_multiproc_wakeup_mailbox *get_mp_wakeup_mailbox(void);
 #endif
 
 #else /* !CONFIG_SMP */
diff --git a/arch/x86/kernel/acpi/madt_wakeup.c b/arch/x86/kernel/acpi/madt_wakeup.c
index 04de3db307de..6b9e41a24574 100644
--- a/arch/x86/kernel/acpi/madt_wakeup.c
+++ b/arch/x86/kernel/acpi/madt_wakeup.c
@@ -37,6 +37,7 @@ static void acpi_mp_play_dead(void)
 
 static void acpi_mp_cpu_die(unsigned int cpu)
 {
+	struct acpi_madt_multiproc_wakeup_mailbox *mailbox = get_mp_wakeup_mailbox();
 	u32 apicid = per_cpu(x86_cpu_to_apicid, cpu);
 	unsigned long timeout;
 
@@ -46,13 +47,13 @@ static void acpi_mp_cpu_die(unsigned int cpu)
 	 *
 	 * BIOS has to clear 'command' field of the mailbox.
 	 */
-	acpi_mp_wake_mailbox->apic_id = apicid;
-	smp_store_release(&acpi_mp_wake_mailbox->command,
+	mailbox->apic_id = apicid;
+	smp_store_release(&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)
+	while (READ_ONCE(mailbox->command) && --timeout)
 		udelay(1);
 
 	if (!timeout)
@@ -251,3 +252,8 @@ void __init setup_mp_wakeup_mailbox(u64 mailbox_paddr)
 	acpi_mp_wake_mailbox_paddr = mailbox_paddr;
 	apic_update_callback(wakeup_secondary_cpu_64, acpi_wakeup_cpu);
 }
+
+struct acpi_madt_multiproc_wakeup_mailbox *get_mp_wakeup_mailbox(void)
+{
+	return acpi_mp_wake_mailbox;
+}
-- 
2.43.0

[PATCH v3 03/13] x86/acpi: Move acpi_wakeup_cpu() and helpers to smpboot.c

[Ricardo Neri]

The bootstrap processor uses acpi_wakeup_cpu() to indicate to firmware that
it wants to boot a secondary CPU using a mailbox as described in the
Multiprocessor Wakeup Structure of the ACPI specification.

The wakeup mailbox does not strictly require support from ACPI. The
platform firmware can implement a mailbox compatible in structure and
operation and enumerate it using other mechanisms such a DeviceTree graph.

Move the code used to setup and use the mailbox out of the ACPI
directory to use it when support for ACPI is not available or needed.

No functional changes are intended.

Originally-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
---
Changes since v2:
 - Only move to smpboot.c the portions of the code that configure and
   use the mailbox. This also resolved the compile warnings about unused
   functions that Michael Kelley reported.
 - Edited the commit message for clarity.

Changes since v1:
 - None.
---
 arch/x86/kernel/acpi/madt_wakeup.c | 75 ----------------------------
 arch/x86/kernel/smpboot.c          | 78 ++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+), 75 deletions(-)

diff --git a/arch/x86/kernel/acpi/madt_wakeup.c b/arch/x86/kernel/acpi/madt_wakeup.c
index 6b9e41a24574..15627f63f9f5 100644
--- a/arch/x86/kernel/acpi/madt_wakeup.c
+++ b/arch/x86/kernel/acpi/madt_wakeup.c
@@ -2,7 +2,6 @@
 #include <linux/acpi.h>
 #include <linux/cpu.h>
 #include <linux/delay.h>
-#include <linux/io.h>
 #include <linux/kexec.h>
 #include <linux/memblock.h>
 #include <linux/pgtable.h>
@@ -15,12 +14,6 @@
 #include <asm/processor.h>
 #include <asm/reboot.h>
 
-/* Physical address of the Multiprocessor Wakeup Structure mailbox */
-static u64 acpi_mp_wake_mailbox_paddr __ro_after_init;
-
-/* Virtual address of the Multiprocessor Wakeup Structure mailbox */
-static struct acpi_madt_multiproc_wakeup_mailbox *acpi_mp_wake_mailbox;
-
 static u64 acpi_mp_pgd __ro_after_init;
 static u64 acpi_mp_reset_vector_paddr __ro_after_init;
 
@@ -127,63 +120,6 @@ static int __init acpi_mp_setup_reset(u64 reset_vector)
 	return 0;
 }
 
-static int acpi_wakeup_cpu(u32 apicid, unsigned long start_ip)
-{
-	if (!acpi_mp_wake_mailbox_paddr) {
-		pr_warn_once("No MADT mailbox: cannot bringup secondary CPUs. Booting with kexec?\n");
-		return -EOPNOTSUPP;
-	}
-
-	/*
-	 * Remap mailbox memory only for the first call to acpi_wakeup_cpu().
-	 *
-	 * Wakeup of secondary CPUs is fully serialized in the core code.
-	 * No need to protect acpi_mp_wake_mailbox from concurrent accesses.
-	 */
-	if (!acpi_mp_wake_mailbox) {
-		acpi_mp_wake_mailbox = memremap(acpi_mp_wake_mailbox_paddr,
-						sizeof(*acpi_mp_wake_mailbox),
-						MEMREMAP_WB);
-	}
-
-	/*
-	 * Mailbox memory is shared between the firmware and OS. Firmware will
-	 * listen on mailbox command address, and once it receives the wakeup
-	 * command, the CPU associated with the given apicid will be booted.
-	 *
-	 * The value of 'apic_id' and 'wakeup_vector' must be visible to the
-	 * firmware before the wakeup command is visible.  smp_store_release()
-	 * ensures ordering and visibility.
-	 */
-	acpi_mp_wake_mailbox->apic_id	    = apicid;
-	acpi_mp_wake_mailbox->wakeup_vector = start_ip;
-	smp_store_release(&acpi_mp_wake_mailbox->command,
-			  ACPI_MP_WAKE_COMMAND_WAKEUP);
-
-	/*
-	 * Wait for the CPU to wake up.
-	 *
-	 * The CPU being woken up is essentially in a spin loop waiting to be
-	 * woken up. It should not take long for it wake up and acknowledge by
-	 * zeroing out ->command.
-	 *
-	 * ACPI specification doesn't provide any guidance on how long kernel
-	 * has to wait for a wake up acknowledgment. It also doesn't provide
-	 * a way to cancel a wake up request if it takes too long.
-	 *
-	 * In TDX environment, the VMM has control over how long it takes to
-	 * wake up secondary. It can postpone scheduling secondary vCPU
-	 * indefinitely. Giving up on wake up request and reporting error opens
-	 * possible attack vector for VMM: it can wake up a secondary CPU when
-	 * kernel doesn't expect it. Wait until positive result of the wake up
-	 * request.
-	 */
-	while (READ_ONCE(acpi_mp_wake_mailbox->command))
-		cpu_relax();
-
-	return 0;
-}
-
 static void acpi_mp_disable_offlining(struct acpi_madt_multiproc_wakeup *mp_wake)
 {
 	cpu_hotplug_disable_offlining();
@@ -246,14 +182,3 @@ int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
 
 	return 0;
 }
-
-void __init setup_mp_wakeup_mailbox(u64 mailbox_paddr)
-{
-	acpi_mp_wake_mailbox_paddr = mailbox_paddr;
-	apic_update_callback(wakeup_secondary_cpu_64, acpi_wakeup_cpu);
-}
-
-struct acpi_madt_multiproc_wakeup_mailbox *get_mp_wakeup_mailbox(void)
-{
-	return acpi_mp_wake_mailbox;
-}
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index d6cf1e23c2a3..6f39ebe4d192 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -61,7 +61,9 @@
 #include <linux/cpuhotplug.h>
 #include <linux/mc146818rtc.h>
 #include <linux/acpi.h>
+#include <linux/io.h>
 
+#include <asm/barrier.h>
 #include <asm/acpi.h>
 #include <asm/cacheinfo.h>
 #include <asm/cpuid.h>
@@ -1354,3 +1356,79 @@ void native_play_dead(void)
 }
 
 #endif
+
+#ifdef CONFIG_X86_64
+/* Physical address of the Multiprocessor Wakeup Structure mailbox */
+static u64 acpi_mp_wake_mailbox_paddr __ro_after_init;
+
+/* Virtual address of the Multiprocessor Wakeup Structure mailbox */
+static struct acpi_madt_multiproc_wakeup_mailbox *acpi_mp_wake_mailbox;
+
+static int acpi_wakeup_cpu(u32 apicid, unsigned long start_ip)
+{
+	if (!acpi_mp_wake_mailbox_paddr) {
+		pr_warn_once("No MADT mailbox: cannot bringup secondary CPUs. Booting with kexec?\n");
+		return -EOPNOTSUPP;
+	}
+
+	/*
+	 * Remap mailbox memory only for the first call to acpi_wakeup_cpu().
+	 *
+	 * Wakeup of secondary CPUs is fully serialized in the core code.
+	 * No need to protect acpi_mp_wake_mailbox from concurrent accesses.
+	 */
+	if (!acpi_mp_wake_mailbox) {
+		acpi_mp_wake_mailbox = memremap(acpi_mp_wake_mailbox_paddr,
+						sizeof(*acpi_mp_wake_mailbox),
+						MEMREMAP_WB);
+	}
+
+	/*
+	 * Mailbox memory is shared between the firmware and OS. Firmware will
+	 * listen on mailbox command address, and once it receives the wakeup
+	 * command, the CPU associated with the given apicid will be booted.
+	 *
+	 * The value of 'apic_id' and 'wakeup_vector' must be visible to the
+	 * firmware before the wakeup command is visible.  smp_store_release()
+	 * ensures ordering and visibility.
+	 */
+	acpi_mp_wake_mailbox->apic_id	    = apicid;
+	acpi_mp_wake_mailbox->wakeup_vector = start_ip;
+	smp_store_release(&acpi_mp_wake_mailbox->command,
+			  ACPI_MP_WAKE_COMMAND_WAKEUP);
+
+	/*
+	 * Wait for the CPU to wake up.
+	 *
+	 * The CPU being woken up is essentially in a spin loop waiting to be
+	 * woken up. It should not take long for it wake up and acknowledge by
+	 * zeroing out ->command.
+	 *
+	 * ACPI specification doesn't provide any guidance on how long kernel
+	 * has to wait for a wake up acknowledgment. It also doesn't provide
+	 * a way to cancel a wake up request if it takes too long.
+	 *
+	 * In TDX environment, the VMM has control over how long it takes to
+	 * wake up secondary. It can postpone scheduling secondary vCPU
+	 * indefinitely. Giving up on wake up request and reporting error opens
+	 * possible attack vector for VMM: it can wake up a secondary CPU when
+	 * kernel doesn't expect it. Wait until positive result of the wake up
+	 * request.
+	 */
+	while (READ_ONCE(acpi_mp_wake_mailbox->command))
+		cpu_relax();
+
+	return 0;
+}
+
+void __init setup_mp_wakeup_mailbox(u64 mailbox_paddr)
+{
+	acpi_mp_wake_mailbox_paddr = mailbox_paddr;
+	apic_update_callback(wakeup_secondary_cpu_64, acpi_wakeup_cpu);
+}
+
+struct acpi_madt_multiproc_wakeup_mailbox *get_mp_wakeup_mailbox(void)
+{
+	return acpi_mp_wake_mailbox;
+}
+#endif
-- 
2.43.0

[PATCH v3 04/13] dt-bindings: x86: Add CPU bindings for x86

[Ricardo Neri]

Add bindings for CPUs in x86 architecture. Start by defining the `reg` and
`enable-method` properties and their relationship to x86 APIC ID and the
available mechanisms to boot secondary CPUs.

Start defining bindings for Intel processors. Bindings for other vendors
can be added later as needed.

Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
---
 .../devicetree/bindings/x86/cpus.yaml         | 80 +++++++++++++++++++
 1 file changed, 80 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/x86/cpus.yaml

diff --git a/Documentation/devicetree/bindings/x86/cpus.yaml b/Documentation/devicetree/bindings/x86/cpus.yaml
new file mode 100644
index 000000000000..108b3ad64aea
--- /dev/null
+++ b/Documentation/devicetree/bindings/x86/cpus.yaml
@@ -0,0 +1,80 @@
+# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/x86/cpus.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: x86 CPUs
+
+maintainers:
+  - Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
+
+description: |
+  Description of x86 CPUs in a system through the "cpus" node.
+
+  Detailed information about the CPU architecture can be found in the Intel
+  Software Developer's Manual:
+    https://intel.com/sdm
+
+properties:
+  compatible:
+    enum:
+      - intel,x86
+
+  reg:
+    description: |
+      Local APIC ID of the CPU. If the CPU has more than one execution thread,
+      then the property is an array with one element per thread.
+
+  enable-method:
+    $ref: /schemas/types.yaml#/definitions/string
+    description: |
+      The method used to wake up secondary CPUs. This property is not needed if
+      the secondary processors are booted using INIT assert, de-assert followed
+      by Start-Up IPI messages as described in the Volume 3, Section 11.4 of
+      Intel Software Developer's Manual.
+
+      It is also optional for the bootstrap CPU.
+
+    oneOf:
+      - items:
+          - const: intel,wakeup-mailbox
+            description: |
+              CPUs are woken up using the mailbox mechanism. The platform
+              firmware boots the secondary CPUs and puts them in a state
+              to check the mailbox for a wakeup command from the operating
+              system.
+
+required:
+  - reg
+  - compatible
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    /*
+     * A system with two CPUs. cpu@0 is the bootstrap CPU and its status is
+     * "okay". It does not have the enable-method property. cpu@1 is a
+     * secondary CPU. Its status is "disabled" and defines the enable-method
+     * property.
+     */
+
+    cpus {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      cpu@0 {
+        reg = <0x0 0x1>;
+        compatible = "intel,x86";
+        status = "okay";
+      };
+
+      cpu@1 {
+        reg = <0x0 0x1>;
+        compatible = "intel,x86";
+        status = "disabled";
+        enable-method = "intel,wakeup-mailbox";
+      };
+    };
+
-- 
2.43.0

[PATCH v3 05/13] x86/dt: Parse the `enable-method` property of CPU nodes

[Ricardo Neri]

Add functionality to parse and validate the `enable-method` property for
platforms that use alternative methods to wakeup secondary CPUs (e.g., a
wakeup mailbox).

Most x86 platforms boot secondary CPUs using INIT assert, de-assert
followed by a Start-Up IPI messages. These systems do no need to specify an
`enable-method` property in the cpu@N nodes of the DeviceTree.

Although it is possible to specify a different `enable-method` for each
secondary CPU, the existing functionality relies on using the
APIC wakeup_secondary_cpu{ (), _64()} callback to wake up all CPUs. Ensure
that either all CPUs specify the same `enable-method` or none at all.

Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
---
Changes since v2:
 - Introduced this patch.

Changes since v1:
 - N/A
---
 arch/x86/kernel/devicetree.c | 88 +++++++++++++++++++++++++++++++++++-
 1 file changed, 86 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/devicetree.c b/arch/x86/kernel/devicetree.c
index dd8748c45529..5835afc74acd 100644
--- a/arch/x86/kernel/devicetree.c
+++ b/arch/x86/kernel/devicetree.c
@@ -127,8 +127,59 @@ static void __init dtb_setup_hpet(void)
 
 #ifdef CONFIG_X86_LOCAL_APIC
 
+#ifdef CONFIG_SMP
+static const char *dtb_supported_enable_methods[] __initconst = { };
+
+static bool __init dtb_enable_method_is_valid(const char *enable_method_a,
+					      const char *enable_method_b)
+{
+	int i;
+
+	if (!enable_method_a && !enable_method_b)
+		return true;
+
+	if (strcmp(enable_method_a, enable_method_b))
+		return false;
+
+	for (i = 0; i < ARRAY_SIZE(dtb_supported_enable_methods); i++) {
+		if (!strcmp(enable_method_a, dtb_supported_enable_methods[i]))
+			return true;
+	}
+
+	return false;
+}
+
+static int __init dtb_configure_enable_method(const char *enable_method)
+{
+	/* Nothing to do for a missing enable-method or if the system has one CPU */
+	if (!enable_method || IS_ERR(enable_method))
+		return 0;
+
+	return -ENOTSUPP;
+}
+#else /* !CONFIG_SMP */
+static inline bool dtb_enable_method_is_valid(const char *enable_method_a,
+					      const char *enable_method_b)
+{
+	/* No secondary CPUs. We do not care about the enable-method. */
+	return true;
+}
+
+static inline int dtb_configure_enable_method(const char *enable_method)
+{
+	return 0;
+}
+#endif /* CONFIG_SMP */
+
+static void __init dtb_register_apic_id(u32 apic_id, struct device_node *dn)
+{
+	topology_register_apic(apic_id, CPU_ACPIID_INVALID, true);
+	set_apicid_to_node(apic_id, of_node_to_nid(dn));
+}
+
 static void __init dtb_cpu_setup(void)
 {
+	const char *enable_method = ERR_PTR(-EINVAL), *this_em;
 	struct device_node *dn;
 	u32 apic_id;
 
@@ -138,9 +189,42 @@ static void __init dtb_cpu_setup(void)
 			pr_warn("%pOF: missing local APIC ID\n", dn);
 			continue;
 		}
-		topology_register_apic(apic_id, CPU_ACPIID_INVALID, true);
-		set_apicid_to_node(apic_id, of_node_to_nid(dn));
+
+		/*
+		 * Also check the enable-method of the secondary CPUs, if present.
+		 *
+		 * Systems that use the INIT-!INIT-StartUp IPI sequence to boot
+		 * secondary CPUs do not need to define an enable-method.
+		 *
+		 * All CPUs must have the same enable-method. The enable-method
+		 * must be supported. If absent in one secondary CPU, it must be
+		 * absent for all CPUs.
+		 *
+		 * Compare the first secondary CPU with the rest. We do not care
+		 * about the boot CPU, as it is enabled already.
+		 */
+
+		if (apic_id == boot_cpu_physical_apicid) {
+			dtb_register_apic_id(apic_id, dn);
+			continue;
+		}
+
+		this_em = of_get_property(dn, "enable-method", NULL);
+
+		if (IS_ERR(enable_method)) {
+			enable_method = this_em;
+			dtb_register_apic_id(apic_id, dn);
+			continue;
+		}
+
+		if (!dtb_enable_method_is_valid(enable_method, this_em))
+			continue;
+
+		dtb_register_apic_id(apic_id, dn);
 	}
+
+	if (dtb_configure_enable_method(enable_method))
+		pr_err("enable-method '%s' needed but not configured\n", enable_method);
 }
 
 static void __init dtb_lapic_setup(void)
-- 
2.43.0

[PATCH v3 06/13] dt-bindings: reserved-memory: Wakeup Mailbox for Intel processors

[Ricardo Neri]

Add DeviceTree bindings for the wakeup mailbox used on Intel processors.

x86 platforms commonly boot secondary CPUs using an INIT assert, de-assert
followed by Start-Up IPI messages. The wakeup mailbox can be used when this
mechanism unavailable.

The wakeup mailbox offers more control to the operating system to boot
secondary CPUs than a spin-table. It allows the reuse of same wakeup vector
for all CPUs while maintaining control over which CPUs to boot and when.
While it is possible to achieve the same level of control using a spin-
table, it would require to specify a separate cpu-release-addr for each
secondary CPU.

Originally-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
---
Changes since v2:
 - Implemented the mailbox as a reserved-memory node. Add to it a
   `compatible` property. (Krzysztof)
 - Explained the relationship between the mailbox and the `enable-mehod`
   property of the CPU nodes.
 - Expanded the documentation of the binding.

Changes since v1:
 - Added more details to the description of the binding.
 - Added requirement a new requirement for cpu@N nodes to add an
   `enable-method`.
---
 .../reserved-memory/intel,wakeup-mailbox.yaml | 87 +++++++++++++++++++
 1 file changed, 87 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reserved-memory/intel,wakeup-mailbox.yaml

diff --git a/Documentation/devicetree/bindings/reserved-memory/intel,wakeup-mailbox.yaml b/Documentation/devicetree/bindings/reserved-memory/intel,wakeup-mailbox.yaml
new file mode 100644
index 000000000000..d97755b4673d
--- /dev/null
+++ b/Documentation/devicetree/bindings/reserved-memory/intel,wakeup-mailbox.yaml
@@ -0,0 +1,87 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/reserved-memory/intel,wakeup-mailbox.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Wakeup Mailbox for Intel processors
+
+description: |
+  The Wakeup Mailbox provides a mechanism for the operating system to wake up
+  secondary CPUs on Intel processors. It is an alternative to the INIT-!INIT-
+  SIPI sequence used on most x86 systems.
+
+  Firmware must define the enable-method property in the CPU nodes as
+  "intel,wakeup-mailbox" to use the mailbox.
+
+  Firmware implements the wakeup mailbox as a 4KB-aligned memory region of size
+  of 4KB. It is memory that the firmware reserves so that each secondary CPU can
+  have the operating system send a single message to them. The firmware is
+  responsible for putting the secondary CPUs in a state to check the mailbox.
+
+  The structure of the mailbox is as follows:
+
+  Field           Byte   Byte  Description
+                 Length Offset
+  ------------------------------------------------------------------------------
+  Command          2      0    Command to wake up the secondary CPU:
+                                        0: Noop
+                                        1: Wakeup: Jump to the wakeup_vector
+                                        2-0xFFFF: Reserved:
+  Reserved         2      2    Must be 0.
+  APIC_ID          4      4    APIC ID of the secondary CPU to wake up.
+  Wakeup_Vector    8      8    The wakeup address for the secondary CPU.
+  ReservedForOs 2032     16    Reserved for OS use.
+  ReservedForFW 2048   2048    Reserved for firmware use.
+  ------------------------------------------------------------------------------
+
+  To wake up a secondary CPU, the operating system 1) prepares the wakeup
+  routine; 2) populates the address of the wakeup routine address into the
+  Wakeup_Vector field; 3) populates the APIC_ID field with the APIC ID of the
+  secondary CPU; 4) writes Wakeup in the Command field. Upon receiving the
+  Wakeup command, the secondary CPU acknowledges the command by writing Noop in
+  the Command field and jumps to the Wakeup_Vector. The operating system can
+  send the next command only after the Command field is changed to Noop.
+
+  The secondary CPU will no longer check the mailbox after waking up. The
+  secondary CPU must ignore the command if its APIC_ID written in the mailbox
+  does not match its own.
+
+  When entering the Wakeup_Vector, interrupts must be disabled and 64-bit
+  addressing mode must be enabled. Paging mode must be enabled. The virtual
+  address of the Wakeup_Vector page must be equal to its physical address.
+  Segment selectors are not used.
+
+maintainers:
+  - Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
+
+allOf:
+  - $ref: reserved-memory.yaml
+
+properties:
+  compatible:
+    const: intel,wakeup-mailbox
+
+  alignment:
+    description: The mailbox must be 4KB-aligned.
+    const: 0x1000
+
+required:
+  - compatible
+  - alignment
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    reserved-memory {
+        #address-cells = <2>;
+        #size-cells = <1>;
+
+        wakeup-mailbox@12340000 {
+            compatible = "intel,wakeup-mailbox";
+            alignment = <0x1000>;
+            reg = <0x0 0x12340000 0x1000>;
+        };
+    };
-- 
2.43.0

[PATCH v3 07/13] x86/dt: Parse the Wakeup Mailbox for Intel processors

[Ricardo Neri]

The Wakeup Mailbox is a mechanism to boot secondary CPUs used on systems
that do not want or cannot use the INIT + StartUp IPI messages.

Add `intel,wakeup-mailbox` to the set of supported enable methods. Also add
functionality to find and parse the parameters of the mailbox from the
DeviceTree from the platform firmware.

The operation and structure of the Wakeup Mailbox are described in the
corresponding DeviceTree schema that accompanies the documentation of the
Linux kernel.

The Wakeup Mailbox is compatible with the Multiprocessor Wakeup Mailbox
Structure described in the ACPI specification. Reuse the existing
functionality to set the memory location of the mailbox and update the
wakeup_secondary_cpu_64() APIC callback.

do_boot_cpu() uses wakeup_secondary_cpu_64() when set. If a wakeup mailbox
is found (enumerated via an ACPI table or a DeviceTree node) it will be
used unconditionally. For cases in which this behavior is not desired, this
APIC callback can be updated later during boot using platform-specific
hooks.

Originally-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
---
Changes since v2:
 - Added extra sanity checks when parsing the mailbox node.
 - Probe the mailbox using its `compatible` property
 - Setup the Wakeup Mailbox if the `enable-method` is found in the CPU
   nodes.
 - Cleaned up unneeded ifdeffery.
 - Clarified the mechanisms used to override the wakeup_secondary_64()
   callback to not use the mailbox when not desired. (Michael)
 - Edited the commit message for clarity.

Changes since v1:
 - Disabled CPU offlining.
 - Modified dtb_parse_mp_wake() to return the address of the mailbox.
---
 arch/x86/kernel/devicetree.c | 57 ++++++++++++++++++++++++++++++++++--
 1 file changed, 55 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/devicetree.c b/arch/x86/kernel/devicetree.c
index 5835afc74acd..d03d9e23c693 100644
--- a/arch/x86/kernel/devicetree.c
+++ b/arch/x86/kernel/devicetree.c
@@ -17,6 +17,7 @@
 #include <linux/pci.h>
 #include <linux/of_pci.h>
 #include <linux/initrd.h>
+#include <linux/smp.h>
 
 #include <asm/irqdomain.h>
 #include <asm/hpet.h>
@@ -128,7 +129,59 @@ static void __init dtb_setup_hpet(void)
 #ifdef CONFIG_X86_LOCAL_APIC
 
 #ifdef CONFIG_SMP
-static const char *dtb_supported_enable_methods[] __initconst = { };
+
+#ifdef CONFIG_X86_64
+
+#define WAKEUP_MAILBOX_SIZE	0x1000
+#define WAKEUP_MAILBOX_ALIGN	0x1000
+
+static int __init dtb_parse_wakeup_mailbox(const char *enable_method)
+{
+	struct device_node *node;
+	struct resource res;
+	int ret = 0;
+
+	if (strcmp(enable_method, "intel,wakeup-mailbox"))
+		return -EINVAL;
+
+	node = of_find_compatible_node(NULL, NULL, "intel,wakeup-mailbox");
+	if (!node)
+		return -ENODEV;
+
+	ret = of_address_to_resource(node, 0, &res);
+	if (ret)
+		goto done;
+
+	/* The mailbox is a 4KB-aligned region.*/
+	if (res.start & (WAKEUP_MAILBOX_ALIGN - 1)) {
+		ret = -EINVAL;
+		goto done;
+	}
+
+	/* The mailbox has a size of 4KB. */
+	if (res.end - res.start + 1 != WAKEUP_MAILBOX_SIZE) {
+		ret = -EINVAL;
+		goto done;
+	}
+
+	/* Not supported when the mailbox is used. */
+	cpu_hotplug_disable_offlining();
+
+	setup_mp_wakeup_mailbox(res.start);
+done:
+	of_node_put(node);
+	return ret;
+}
+#else /* !CONFIG_X86_64 */
+static inline int dtb_parse_wakeup_mailbox(const char *enable_method)
+{
+	return -ENOTSUPP;
+}
+#endif /* CONFIG_X86_64 */
+
+static const char *dtb_supported_enable_methods[] __initconst = {
+	"intel,wakeup-mailbox"
+};
 
 static bool __init dtb_enable_method_is_valid(const char *enable_method_a,
 					      const char *enable_method_b)
@@ -155,7 +208,7 @@ static int __init dtb_configure_enable_method(const char *enable_method)
 	if (!enable_method || IS_ERR(enable_method))
 		return 0;
 
-	return -ENOTSUPP;
+	return dtb_parse_wakeup_mailbox(enable_method);
 }
 #else /* !CONFIG_SMP */
 static inline bool dtb_enable_method_is_valid(const char *enable_method_a,
-- 
2.43.0

[PATCH v3 08/13] x86/hyperv/vtl: Set real_mode_header in hv_vtl_init_platform()

[Ricardo Neri]

From: Yunhong Jiang <yunhong.jiang@linux.intel.com>

Hyper-V VTL clears x86_platform.realmode_{init(), reserve()} in
hv_vtl_platform_init() whereas it sets real_mode_header later in
hv_vtl_early_init(). There is no need to deal with the real mode memory
in two places: x86_platform.realmode_init() is invoked much later via an
early_initcall.

Set real_mode_header in hv_vtl_init_platform() to keep all code dealing
with memory for the real mode trampoline in one place. Besides making the
code more readable, it prepares it for a subsequent changeset in which the
behavior needs to change to support Hyper-V VTL guests in TDX environment.

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
---
Changes since v2:
 - Edited the commit message for clarity.

Changes since v1:
 - Introduced this patch.
---
 arch/x86/hyperv/hv_vtl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/hyperv/hv_vtl.c b/arch/x86/hyperv/hv_vtl.c
index 4580936dcb03..6bd183ee484f 100644
--- a/arch/x86/hyperv/hv_vtl.c
+++ b/arch/x86/hyperv/hv_vtl.c
@@ -60,6 +60,7 @@ void __init hv_vtl_init_platform(void)
 
 	x86_platform.realmode_reserve = x86_init_noop;
 	x86_platform.realmode_init = x86_init_noop;
+	real_mode_header = &hv_vtl_real_mode_header;
 	x86_init.irqs.pre_vector_init = x86_init_noop;
 	x86_init.timers.timer_init = x86_init_noop;
 	x86_init.resources.probe_roms = x86_init_noop;
@@ -279,7 +280,6 @@ int __init hv_vtl_early_init(void)
 		panic("XSAVE has to be disabled as it is not supported by this module.\n"
 			  "Please add 'noxsave' to the kernel command line.\n");
 
-	real_mode_header = &hv_vtl_real_mode_header;
 	apic_update_callback(wakeup_secondary_cpu_64, hv_vtl_wakeup_secondary_cpu);
 
 	return 0;
-- 
2.43.0

[PATCH v3 09/13] x86/realmode: Make the location of the trampoline configurable

[Ricardo Neri]

From: Yunhong Jiang <yunhong.jiang@linux.intel.com>

x86 CPUs boot in real mode. This mode uses 20-bit memory addresses (16-bit
registers plus 4-bit segment selectors). This implies that the trampoline
must reside under the 1MB memory boundary.

There are platforms in which the firmware boots the secondary CPUs,
switches them to long mode and transfers control to the kernel. An example
of such mechanism is the ACPI Multiprocessor Wakeup Structure.

In this scenario there is no restriction to locate the trampoline under 1MB
memory. Moreover, certain platforms (for example, Hyper-V VTL guests) may
not have memory available for allocation under 1MB.

Add a new member to struct x86_init_resources to specify the upper bound
for the location of the trampoline memory. Keep the default upper bound of
1MB to conserve the current behavior.

Originally-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
---
Changes since v2:
 - Edited the commit message for clarity.
 - Minor tweaks to comments.
 - Removed the option to not reserve the first 1MB of memory as it is
   not needed.

Changes since v1:
 - Added this patch using code that Thomas suggested:
   https://lore.kernel.org/lkml/87a5ho2q6x.ffs@tglx/
---
 arch/x86/include/asm/x86_init.h | 3 +++
 arch/x86/kernel/x86_init.c      | 3 +++
 arch/x86/realmode/init.c        | 7 +++----
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index 36698cc9fb44..e770ce507a87 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -31,12 +31,15 @@ struct x86_init_mpparse {
  *				platform
  * @memory_setup:		platform specific memory setup
  * @dmi_setup:			platform specific DMI setup
+ * @realmode_limit:		platform specific address limit for the real mode trampoline
+ *				(default 1M)
  */
 struct x86_init_resources {
 	void (*probe_roms)(void);
 	void (*reserve_resources)(void);
 	char *(*memory_setup)(void);
 	void (*dmi_setup)(void);
+	unsigned long realmode_limit;
 };
 
 /**
diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
index 0a2bbd674a6d..a25fd7282811 100644
--- a/arch/x86/kernel/x86_init.c
+++ b/arch/x86/kernel/x86_init.c
@@ -9,6 +9,7 @@
 #include <linux/export.h>
 #include <linux/pci.h>
 #include <linux/acpi.h>
+#include <linux/sizes.h>
 
 #include <asm/acpi.h>
 #include <asm/bios_ebda.h>
@@ -69,6 +70,8 @@ struct x86_init_ops x86_init __initdata = {
 		.reserve_resources	= reserve_standard_io_resources,
 		.memory_setup		= e820__memory_setup_default,
 		.dmi_setup		= dmi_setup,
+		/* Has to be under 1M so we can execute real-mode AP code. */
+		.realmode_limit		= SZ_1M,
 	},
 
 	.mpparse = {
diff --git a/arch/x86/realmode/init.c b/arch/x86/realmode/init.c
index ed5c63c0b4e5..01155f995b2b 100644
--- a/arch/x86/realmode/init.c
+++ b/arch/x86/realmode/init.c
@@ -46,7 +46,7 @@ void load_trampoline_pgtable(void)
 
 void __init reserve_real_mode(void)
 {
-	phys_addr_t mem;
+	phys_addr_t mem, limit = x86_init.resources.realmode_limit;
 	size_t size = real_mode_size_needed();
 
 	if (!size)
@@ -54,10 +54,9 @@ void __init reserve_real_mode(void)
 
 	WARN_ON(slab_is_available());
 
-	/* Has to be under 1M so we can execute real-mode AP code. */
-	mem = memblock_phys_alloc_range(size, PAGE_SIZE, 0, 1<<20);
+	mem = memblock_phys_alloc_range(size, PAGE_SIZE, 0, limit);
 	if (!mem)
-		pr_info("No sub-1M memory is available for the trampoline\n");
+		pr_info("No memory below %pa for the real-mode trampoline\n", &limit);
 	else
 		set_real_mode_mem(mem);
 
-- 
2.43.0

[PATCH v3 10/13] x86/hyperv/vtl: Setup the 64-bit trampoline for TDX guests

[Ricardo Neri]

From: Yunhong Jiang <yunhong.jiang@linux.intel.com>

The hypervisor is an untrusted entity for TDX guests. It cannot be used
to boot secondary CPUs - neither via hypercalls not the INIT assert,
de-assert plus Start-Up IPI messages.

Instead, the platform virtual firmware boots the secondary CPUs and
puts them in a state to transfer control to the kernel. This mechanism uses
the wakeup mailbox described in the Multiprocessor Wakeup Structure of the
ACPI specification. The entry point to the kernel is trampoline_start64.

Allocate and setup the trampoline using the default x86_platform callbacks.

The platform firmware configures the secondary CPUs in long mode. It is no
longer necessary to locate the trampoline under 1MB memory. After handoff
from firmware, the trampoline code switches briefly to 32-bit addressing
mode, which has an addressing limit of 4GB. Set the upper bound of the
trampoline memory accordingly.

Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
---
Changes since v2:
 - Added a note regarding there is no need to check for a present
   paravisor.
 - Edited commit message for clarity.

Changes since v1:
 - Dropped the function hv_reserve_real_mode(). Instead, used the new
   members realmode_limit and reserve_bios members of x86_init to
   set the upper bound of the trampoline memory. (Thomas)
---
 arch/x86/hyperv/hv_vtl.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/arch/x86/hyperv/hv_vtl.c b/arch/x86/hyperv/hv_vtl.c
index 6bd183ee484f..8b497c8292d3 100644
--- a/arch/x86/hyperv/hv_vtl.c
+++ b/arch/x86/hyperv/hv_vtl.c
@@ -58,9 +58,14 @@ void __init hv_vtl_init_platform(void)
 {
 	pr_info("Linux runs in Hyper-V Virtual Trust Level\n");
 
-	x86_platform.realmode_reserve = x86_init_noop;
-	x86_platform.realmode_init = x86_init_noop;
-	real_mode_header = &hv_vtl_real_mode_header;
+	/* There is no paravisor present if we are here. */
+	if (hv_isolation_type_tdx()) {
+		x86_init.resources.realmode_limit = SZ_4G;
+	} else {
+		x86_platform.realmode_reserve = x86_init_noop;
+		x86_platform.realmode_init = x86_init_noop;
+		real_mode_header = &hv_vtl_real_mode_header;
+	}
 	x86_init.irqs.pre_vector_init = x86_init_noop;
 	x86_init.timers.timer_init = x86_init_noop;
 	x86_init.resources.probe_roms = x86_init_noop;
-- 
2.43.0

[PATCH v3 11/13] x86/smpboot: Add a helper get the address of the wakeup mailbox

[Ricardo Neri]

A Hyper-V VTL level 2 guest on a TDX environment needs to map the
physical page of the ACPI Multiprocessor Wakeup Structure as private
(encrypted). It needs to know the physical address of this structure.
Add a helper function.

Suggested-by: Michael Kelley <mhklinux@outlook.com>
Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
---
Changes since v2:
 - Introduced this patch

Changes since v1:
 - N/A
---
 arch/x86/include/asm/smp.h | 1 +
 arch/x86/kernel/smpboot.c  | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index 97bfbd0d24d4..18003453569a 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -149,6 +149,7 @@ static inline struct cpumask *cpu_l2c_shared_mask(int cpu)
 #ifdef CONFIG_X86_64
 void setup_mp_wakeup_mailbox(u64 addr);
 struct acpi_madt_multiproc_wakeup_mailbox *get_mp_wakeup_mailbox(void);
+u64 get_mp_wakeup_mailbox_paddr(void);
 #endif
 
 #else /* !CONFIG_SMP */
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 6f39ebe4d192..1e211c78c1d3 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1431,4 +1431,9 @@ struct acpi_madt_multiproc_wakeup_mailbox *get_mp_wakeup_mailbox(void)
 {
 	return acpi_mp_wake_mailbox;
 }
+
+u64 get_mp_wakeup_mailbox_paddr(void)
+{
+	return acpi_mp_wake_mailbox_paddr;
+}
 #endif
-- 
2.43.0

[PATCH v3 12/13] x86/hyperv/vtl: Mark the wakeup mailbox page as private

[Ricardo Neri]

From: Yunhong Jiang <yunhong.jiang@linux.intel.com>

The current code maps MMIO devices as shared (decrypted) by default in a
confidential computing VM.

In a TDX environment, secondary CPUs are booted using the Multiprocessor
Wakeup Structure defined in the ACPI specification. The virtual firmware
and the operating system function in the guest context, without
intervention from the VMM. Map the physical memory of the mailbox as
private. Use the is_private_mmio() callback.

Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
---
Changes since v2:
 - Use the new helper function get_mp_wakeup_mailbox_paddr().
 - Edited the commit message for clarity.

Changes since v1:
 - Added the helper function within_page() to improve readability
 - Override the is_private_mmio() callback when detecting a TDX
   environment. The address of the mailbox is checked in
   hv_is_private_mmio_tdx().
---
 arch/x86/hyperv/hv_vtl.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/x86/hyperv/hv_vtl.c b/arch/x86/hyperv/hv_vtl.c
index 8b497c8292d3..cd48bedd21f0 100644
--- a/arch/x86/hyperv/hv_vtl.c
+++ b/arch/x86/hyperv/hv_vtl.c
@@ -54,6 +54,18 @@ static void  __noreturn hv_vtl_restart(char __maybe_unused *cmd)
 	hv_vtl_emergency_restart();
 }
 
+static inline bool within_page(u64 addr, u64 start)
+{
+	return addr >= start && addr < (start + PAGE_SIZE);
+}
+
+static bool hv_vtl_is_private_mmio_tdx(u64 addr)
+{
+	u64 mb_addr = get_mp_wakeup_mailbox_paddr();
+
+	return mb_addr && within_page(addr, mb_addr);
+}
+
 void __init hv_vtl_init_platform(void)
 {
 	pr_info("Linux runs in Hyper-V Virtual Trust Level\n");
@@ -61,6 +73,8 @@ void __init hv_vtl_init_platform(void)
 	/* There is no paravisor present if we are here. */
 	if (hv_isolation_type_tdx()) {
 		x86_init.resources.realmode_limit = SZ_4G;
+		x86_platform.hyper.is_private_mmio = hv_vtl_is_private_mmio_tdx;
+
 	} else {
 		x86_platform.realmode_reserve = x86_init_noop;
 		x86_platform.realmode_init = x86_init_noop;
-- 
2.43.0

[PATCH v3 13/13] x86/hyperv/vtl: Use the wakeup mailbox to boot secondary CPUs

[Ricardo Neri]

The hypervisor is an untrusted entity for TDX guests. It cannot be used
to boot secondary CPUs. The function hv_vtl_wakeup_secondary_cpu() cannot
be used.

Instead, the virtual firmware boots the secondary CPUs and places them in
a state to transfer control to the kernel using the wakeup mailbox.

The kernel updates the APIC callback wakeup_secondary_cpu_64() to use
the mailbox if detected early during boot (enumerated via either an ACPI
table or a DeviceTree node).

Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
---
Changes since v2:
 - Unconditionally use the wakeup mailbox in a TDX confidential VM.
   (Michael).
 - Edited the commit message for clarity.

Changes since v1:
 - None
---
 arch/x86/hyperv/hv_vtl.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/arch/x86/hyperv/hv_vtl.c b/arch/x86/hyperv/hv_vtl.c
index cd48bedd21f0..30a5a0c156c1 100644
--- a/arch/x86/hyperv/hv_vtl.c
+++ b/arch/x86/hyperv/hv_vtl.c
@@ -299,7 +299,15 @@ int __init hv_vtl_early_init(void)
 		panic("XSAVE has to be disabled as it is not supported by this module.\n"
 			  "Please add 'noxsave' to the kernel command line.\n");
 
-	apic_update_callback(wakeup_secondary_cpu_64, hv_vtl_wakeup_secondary_cpu);
+	/*
+	 * TDX confidential VMs do not trust the hypervisor and cannot use it to
+	 * boot secondary CPUs. Instead, they will be booted using the wakeup
+	 * mailbox if detected during boot. See setup_arch().
+	 *
+	 * There is no paravisor present if we are here.
+	 */
+	if (!hv_isolation_type_tdx())
+		apic_update_callback(wakeup_secondary_cpu_64, hv_vtl_wakeup_secondary_cpu);
 
 	return 0;
 }
-- 
2.43.0

Re: [PATCH v3 04/13] dt-bindings: x86: Add CPU bindings for x86

[Rob Herring (Arm)]

On Sat, 03 May 2025 12:15:06 -0700, Ricardo Neri wrote:
> Add bindings for CPUs in x86 architecture. Start by defining the `reg` and
> `enable-method` properties and their relationship to x86 APIC ID and the
> available mechanisms to boot secondary CPUs.
> 
> Start defining bindings for Intel processors. Bindings for other vendors
> can be added later as needed.
> 
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> ---
>  .../devicetree/bindings/x86/cpus.yaml         | 80 +++++++++++++++++++
>  1 file changed, 80 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/x86/cpus.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/x86/cpus.example.dtb: cpus: cpu@0: 'cache-level' is a required property
	from schema $id: http://devicetree.org/schemas/cpus.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250503191515.24041-5-ricardo.neri-calderon@linux.intel.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v3 04/13] dt-bindings: x86: Add CPU bindings for x86

[Krzysztof Kozlowski]

On Sat, May 03, 2025 at 12:15:06PM GMT, Ricardo Neri wrote:
> Add bindings for CPUs in x86 architecture. Start by defining the `reg` and

What for?

> `enable-method` properties and their relationship to x86 APIC ID and the
> available mechanisms to boot secondary CPUs.
> 
> Start defining bindings for Intel processors. Bindings for other vendors
> can be added later as needed.
> 
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> ---

Not really tested so only limited review follows.

>  .../devicetree/bindings/x86/cpus.yaml         | 80 +++++++++++++++++++
>  1 file changed, 80 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/x86/cpus.yaml
> 
> diff --git a/Documentation/devicetree/bindings/x86/cpus.yaml b/Documentation/devicetree/bindings/x86/cpus.yaml
> new file mode 100644
> index 000000000000..108b3ad64aea
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/x86/cpus.yaml
> @@ -0,0 +1,80 @@
> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/x86/cpus.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: x86 CPUs
> +
> +maintainers:
> +  - Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> +
> +description: |
> +  Description of x86 CPUs in a system through the "cpus" node.
> +
> +  Detailed information about the CPU architecture can be found in the Intel
> +  Software Developer's Manual:
> +    https://intel.com/sdm
> +
> +properties:
> +  compatible:
> +    enum:
> +      - intel,x86

That's architecture, not a CPU. CPUs are like 80286, 80386, so that's
not even specific instruction set. I don't get what you need it for.

> +
> +  reg:

Missing constraints.

> +    description: |

Do not need '|' unless you need to preserve formatting.

> +      Local APIC ID of the CPU. If the CPU has more than one execution thread,
> +      then the property is an array with one element per thread.
> +
> +  enable-method:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    description: |
> +      The method used to wake up secondary CPUs. This property is not needed if
> +      the secondary processors are booted using INIT assert, de-assert followed
> +      by Start-Up IPI messages as described in the Volume 3, Section 11.4 of
> +      Intel Software Developer's Manual.
> +
> +      It is also optional for the bootstrap CPU.
> +
> +    oneOf:

I see only one entry, so didn't you want an enum?

> +      - items:

Not a list

> +          - const: intel,wakeup-mailbox

So every vendor is supposed to come with different name for the same
feature? Or is wakeup-mailnox really intel specific, but then specific
to which processors?


> +            description: |
> +              CPUs are woken up using the mailbox mechanism. The platform
> +              firmware boots the secondary CPUs and puts them in a state
> +              to check the mailbox for a wakeup command from the operating
> +              system.
> +
> +required:
> +  - reg
> +  - compatible
> +
> +unevaluatedProperties: false

Missing ref in top-level or this is supposed to be additionalProps. See
example-schema.

> +
> +examples:
> +  - |
> +    /*
> +     * A system with two CPUs. cpu@0 is the bootstrap CPU and its status is
> +     * "okay". It does not have the enable-method property. cpu@1 is a
> +     * secondary CPU. Its status is "disabled" and defines the enable-method
> +     * property.
> +     */
> +
> +    cpus {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      cpu@0 {
> +        reg = <0x0 0x1>;
> +        compatible = "intel,x86";
> +        status = "okay";

Drop

> +      };
> +
> +      cpu@1 {
> +        reg = <0x0 0x1>;
> +        compatible = "intel,x86";
> +        status = "disabled";

Why?

Best regards,
Krzysztof

Re: [PATCH v3 06/13] dt-bindings: reserved-memory: Wakeup Mailbox for Intel processors

[Krzysztof Kozlowski]

On Sat, May 03, 2025 at 12:15:08PM GMT, Ricardo Neri wrote:
> Add DeviceTree bindings for the wakeup mailbox used on Intel processors.
> 

Start using b4, so your cover letter will have proper lore links to
previous versions.

> x86 platforms commonly boot secondary CPUs using an INIT assert, de-assert
> followed by Start-Up IPI messages. The wakeup mailbox can be used when this
> mechanism unavailable.
> 
> The wakeup mailbox offers more control to the operating system to boot
> secondary CPUs than a spin-table. It allows the reuse of same wakeup vector
> for all CPUs while maintaining control over which CPUs to boot and when.
> While it is possible to achieve the same level of control using a spin-
> table, it would require to specify a separate cpu-release-addr for each
> secondary CPU.
> 
> Originally-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>

What does this tag mean? Why you cannot use standard tags - SoB and
co-developed?

> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> ---
> Changes since v2:
>  - Implemented the mailbox as a reserved-memory node. Add to it a
>    `compatible` property. (Krzysztof)
>  - Explained the relationship between the mailbox and the `enable-mehod`
>    property of the CPU nodes.
>  - Expanded the documentation of the binding.
> 
> Changes since v1:
>  - Added more details to the description of the binding.
>  - Added requirement a new requirement for cpu@N nodes to add an
>    `enable-method`.
> ---
>  .../reserved-memory/intel,wakeup-mailbox.yaml | 87 +++++++++++++++++++
>  1 file changed, 87 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reserved-memory/intel,wakeup-mailbox.yaml
> 
> diff --git a/Documentation/devicetree/bindings/reserved-memory/intel,wakeup-mailbox.yaml b/Documentation/devicetree/bindings/reserved-memory/intel,wakeup-mailbox.yaml
> new file mode 100644
> index 000000000000..d97755b4673d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reserved-memory/intel,wakeup-mailbox.yaml
> @@ -0,0 +1,87 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/reserved-memory/intel,wakeup-mailbox.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Wakeup Mailbox for Intel processors
> +
> +description: |
> +  The Wakeup Mailbox provides a mechanism for the operating system to wake up
> +  secondary CPUs on Intel processors. It is an alternative to the INIT-!INIT-
> +  SIPI sequence used on most x86 systems.
> +
> +  Firmware must define the enable-method property in the CPU nodes as
> +  "intel,wakeup-mailbox" to use the mailbox.
> +
> +  Firmware implements the wakeup mailbox as a 4KB-aligned memory region of size
> +  of 4KB. It is memory that the firmware reserves so that each secondary CPU can
> +  have the operating system send a single message to them. The firmware is
> +  responsible for putting the secondary CPUs in a state to check the mailbox.
> +
> +  The structure of the mailbox is as follows:
> +
> +  Field           Byte   Byte  Description
> +                 Length Offset
> +  ------------------------------------------------------------------------------
> +  Command          2      0    Command to wake up the secondary CPU:
> +                                        0: Noop
> +                                        1: Wakeup: Jump to the wakeup_vector
> +                                        2-0xFFFF: Reserved:
> +  Reserved         2      2    Must be 0.
> +  APIC_ID          4      4    APIC ID of the secondary CPU to wake up.
> +  Wakeup_Vector    8      8    The wakeup address for the secondary CPU.
> +  ReservedForOs 2032     16    Reserved for OS use.
> +  ReservedForFW 2048   2048    Reserved for firmware use.
> +  ------------------------------------------------------------------------------
> +
> +  To wake up a secondary CPU, the operating system 1) prepares the wakeup
> +  routine; 2) populates the address of the wakeup routine address into the
> +  Wakeup_Vector field; 3) populates the APIC_ID field with the APIC ID of the
> +  secondary CPU; 4) writes Wakeup in the Command field. Upon receiving the
> +  Wakeup command, the secondary CPU acknowledges the command by writing Noop in
> +  the Command field and jumps to the Wakeup_Vector. The operating system can
> +  send the next command only after the Command field is changed to Noop.
> +
> +  The secondary CPU will no longer check the mailbox after waking up. The
> +  secondary CPU must ignore the command if its APIC_ID written in the mailbox
> +  does not match its own.
> +
> +  When entering the Wakeup_Vector, interrupts must be disabled and 64-bit
> +  addressing mode must be enabled. Paging mode must be enabled. The virtual
> +  address of the Wakeup_Vector page must be equal to its physical address.
> +  Segment selectors are not used.
> +
> +maintainers:
> +  - Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> +
> +allOf:
> +  - $ref: reserved-memory.yaml
> +
> +properties:
> +  compatible:
> +    const: intel,wakeup-mailbox

If this is a device, then compatibles specific to devices. You do not
get different rules than all other bindings... or this does not have to
be binding at all. Why standard reserved-memory does not work for here?

Why do you need compatible in the first place?


> +
> +  alignment:
> +    description: The mailbox must be 4KB-aligned.

Then drop it because it is implied by compatible.

> +    const: 0x1000
> +
> +required:
> +  - compatible
> +  - alignment
> +  - reg
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    reserved-memory {
> +        #address-cells = <2>;
> +        #size-cells = <1>;
> +
> +        wakeup-mailbox@12340000 {

Please use real addresses from real DT.

> +            compatible = "intel,wakeup-mailbox";
> +            alignment = <0x1000>;
> +            reg = <0x0 0x12340000 0x1000>;

reg is always the second property. See DTS coding style.

Best regards,
Krzysztof

Re: [PATCH v3 01/13] x86/acpi: Add a helper function to setup the wakeup mailbox

[Rafael J. Wysocki]

On Sat, May 3, 2025 at 9:10 PM Ricardo Neri
<ricardo.neri-calderon@linux.intel.com> wrote:
>
> In preparation to move the functionality to wake secondary CPUs up out of
> the ACPI code, add a helper function that stores the physical address of
> the mailbox and updates the wakeup_secondary_cpu_64() APIC callback.
>
> There is a slight change in behavior: now the APIC callback is updated
> before configuring CPU hotplug offline behavior. This is fine as the APIC
> callback continues to be updated unconditionally, regardless of the
> restriction on CPU offlining.
>
> The wakeup mailbox is only supported for CONFIG_X86_64 and needed only with
> CONFIG_SMP=y.
>
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> ---
> Changes since v2:
>  - Introduced this patch.
>
> Changes since v1:
>  - N/A
> ---
>  arch/x86/include/asm/smp.h         |  4 ++++
>  arch/x86/kernel/acpi/madt_wakeup.c | 10 +++++++---
>  2 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
> index 0c1c68039d6f..3622951d2ee0 100644
> --- a/arch/x86/include/asm/smp.h
> +++ b/arch/x86/include/asm/smp.h
> @@ -146,6 +146,10 @@ static inline struct cpumask *cpu_l2c_shared_mask(int cpu)
>         return per_cpu(cpu_l2c_shared_map, cpu);
>  }
>
> +#ifdef CONFIG_X86_64
> +void setup_mp_wakeup_mailbox(u64 addr);
> +#endif

The #ifdef is only necessary if you are going to provide an
alternative for builds in which the symbol is unset.

> +
>  #else /* !CONFIG_SMP */
>  #define wbinvd_on_cpu(cpu)     wbinvd()
>  static inline int wbinvd_on_all_cpus(void)
> diff --git a/arch/x86/kernel/acpi/madt_wakeup.c b/arch/x86/kernel/acpi/madt_wakeup.c
> index f36f28405dcc..04de3db307de 100644
> --- a/arch/x86/kernel/acpi/madt_wakeup.c
> +++ b/arch/x86/kernel/acpi/madt_wakeup.c
> @@ -227,7 +227,7 @@ int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
>
>         acpi_table_print_madt_entry(&header->common);
>
> -       acpi_mp_wake_mailbox_paddr = mp_wake->mailbox_address;
> +       setup_mp_wakeup_mailbox(mp_wake->mailbox_address);

I'd prefer acpi_setup_mp_wakeup_mailbox().

>
>         if (mp_wake->version >= ACPI_MADT_MP_WAKEUP_VERSION_V1 &&
>             mp_wake->header.length >= ACPI_MADT_MP_WAKEUP_SIZE_V1) {
> @@ -243,7 +243,11 @@ int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
>                 acpi_mp_disable_offlining(mp_wake);
>         }
>
> -       apic_update_callback(wakeup_secondary_cpu_64, acpi_wakeup_cpu);
> -
>         return 0;
>  }
> +
> +void __init setup_mp_wakeup_mailbox(u64 mailbox_paddr)
> +{
> +       acpi_mp_wake_mailbox_paddr = mailbox_paddr;
> +       apic_update_callback(wakeup_secondary_cpu_64, acpi_wakeup_cpu);
> +}
> --
> 2.43.0
>
>

Re: [PATCH v3 02/13] x86/acpi: Add a helper function to get a pointer to the wakeup mailbox

[Rafael J. Wysocki]

On Sat, May 3, 2025 at 9:10 PM Ricardo Neri
<ricardo.neri-calderon@linux.intel.com> wrote:
>
> In preparation to move the functionality to wake secondary CPUs up out
> of the ACPI code, add a helper function to get a pointer to the mailbox.
>
> Use this helper function only in the portions of the code for which the
> variable acpi_mp_wake_mailbox will be out of scope once it is relocated
> out of the ACPI directory.
>
> The wakeup mailbox is only supported for CONFIG_X86_64 and needed only
> with CONFIG_SMP.
>
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> ---
> Changes since v2:
>  - Introduced this patch.

Have you considered merging it with the previous patch?  They both do
analogous things.

> Changes since v1:
>  - N/A
> ---
>  arch/x86/include/asm/smp.h         |  1 +
>  arch/x86/kernel/acpi/madt_wakeup.c | 12 +++++++++---
>  2 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
> index 3622951d2ee0..97bfbd0d24d4 100644
> --- a/arch/x86/include/asm/smp.h
> +++ b/arch/x86/include/asm/smp.h
> @@ -148,6 +148,7 @@ static inline struct cpumask *cpu_l2c_shared_mask(int cpu)
>
>  #ifdef CONFIG_X86_64
>  void setup_mp_wakeup_mailbox(u64 addr);
> +struct acpi_madt_multiproc_wakeup_mailbox *get_mp_wakeup_mailbox(void);
>  #endif
>
>  #else /* !CONFIG_SMP */
> diff --git a/arch/x86/kernel/acpi/madt_wakeup.c b/arch/x86/kernel/acpi/madt_wakeup.c
> index 04de3db307de..6b9e41a24574 100644
> --- a/arch/x86/kernel/acpi/madt_wakeup.c
> +++ b/arch/x86/kernel/acpi/madt_wakeup.c
> @@ -37,6 +37,7 @@ static void acpi_mp_play_dead(void)
>
>  static void acpi_mp_cpu_die(unsigned int cpu)
>  {
> +       struct acpi_madt_multiproc_wakeup_mailbox *mailbox = get_mp_wakeup_mailbox();

I'd prefer acpi_get_mp_wakeup_mailbox().

>         u32 apicid = per_cpu(x86_cpu_to_apicid, cpu);
>         unsigned long timeout;
>
> @@ -46,13 +47,13 @@ static void acpi_mp_cpu_die(unsigned int cpu)
>          *
>          * BIOS has to clear 'command' field of the mailbox.
>          */
> -       acpi_mp_wake_mailbox->apic_id = apicid;
> -       smp_store_release(&acpi_mp_wake_mailbox->command,
> +       mailbox->apic_id = apicid;
> +       smp_store_release(&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)
> +       while (READ_ONCE(mailbox->command) && --timeout)
>                 udelay(1);
>
>         if (!timeout)
> @@ -251,3 +252,8 @@ void __init setup_mp_wakeup_mailbox(u64 mailbox_paddr)
>         acpi_mp_wake_mailbox_paddr = mailbox_paddr;
>         apic_update_callback(wakeup_secondary_cpu_64, acpi_wakeup_cpu);
>  }
> +
> +struct acpi_madt_multiproc_wakeup_mailbox *get_mp_wakeup_mailbox(void)
> +{
> +       return acpi_mp_wake_mailbox;
> +}
> --

Re: [PATCH v3 03/13] x86/acpi: Move acpi_wakeup_cpu() and helpers to smpboot.c

[Rafael J. Wysocki]

On Sat, May 3, 2025 at 9:10 PM Ricardo Neri
<ricardo.neri-calderon@linux.intel.com> wrote:
>
> The bootstrap processor uses acpi_wakeup_cpu() to indicate to firmware that
> it wants to boot a secondary CPU using a mailbox as described in the
> Multiprocessor Wakeup Structure of the ACPI specification.
>
> The wakeup mailbox does not strictly require support from ACPI.

Well, except that it is defined by the ACPI specification.

> The platform firmware can implement a mailbox compatible in structure and
> operation and enumerate it using other mechanisms such a DeviceTree graph.

So is there a specification defining this mechanism?

It is generally not sufficient to put the code and DT bindings
unilaterally into the OS and expect the firmware to follow suit.

> Move the code used to setup and use the mailbox out of the ACPI
> directory to use it when support for ACPI is not available or needed.

I think that the code implementing interfaces defined by the ACPI
specification is not generic and so it should not be built when the
kernel is configured without ACPI support.

> No functional changes are intended.
>
> Originally-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> ---
> Changes since v2:
>  - Only move to smpboot.c the portions of the code that configure and
>    use the mailbox. This also resolved the compile warnings about unused
>    functions that Michael Kelley reported.
>  - Edited the commit message for clarity.
>
> Changes since v1:
>  - None.
> ---
>  arch/x86/kernel/acpi/madt_wakeup.c | 75 ----------------------------
>  arch/x86/kernel/smpboot.c          | 78 ++++++++++++++++++++++++++++++
>  2 files changed, 78 insertions(+), 75 deletions(-)
>
> diff --git a/arch/x86/kernel/acpi/madt_wakeup.c b/arch/x86/kernel/acpi/madt_wakeup.c
> index 6b9e41a24574..15627f63f9f5 100644
> --- a/arch/x86/kernel/acpi/madt_wakeup.c
> +++ b/arch/x86/kernel/acpi/madt_wakeup.c
> @@ -2,7 +2,6 @@
>  #include <linux/acpi.h>
>  #include <linux/cpu.h>
>  #include <linux/delay.h>
> -#include <linux/io.h>
>  #include <linux/kexec.h>
>  #include <linux/memblock.h>
>  #include <linux/pgtable.h>
> @@ -15,12 +14,6 @@
>  #include <asm/processor.h>
>  #include <asm/reboot.h>
>
> -/* Physical address of the Multiprocessor Wakeup Structure mailbox */
> -static u64 acpi_mp_wake_mailbox_paddr __ro_after_init;
> -
> -/* Virtual address of the Multiprocessor Wakeup Structure mailbox */
> -static struct acpi_madt_multiproc_wakeup_mailbox *acpi_mp_wake_mailbox;
> -
>  static u64 acpi_mp_pgd __ro_after_init;
>  static u64 acpi_mp_reset_vector_paddr __ro_after_init;
>
> @@ -127,63 +120,6 @@ static int __init acpi_mp_setup_reset(u64 reset_vector)
>         return 0;
>  }
>
> -static int acpi_wakeup_cpu(u32 apicid, unsigned long start_ip)
> -{
> -       if (!acpi_mp_wake_mailbox_paddr) {
> -               pr_warn_once("No MADT mailbox: cannot bringup secondary CPUs. Booting with kexec?\n");
> -               return -EOPNOTSUPP;
> -       }
> -
> -       /*
> -        * Remap mailbox memory only for the first call to acpi_wakeup_cpu().
> -        *
> -        * Wakeup of secondary CPUs is fully serialized in the core code.
> -        * No need to protect acpi_mp_wake_mailbox from concurrent accesses.
> -        */
> -       if (!acpi_mp_wake_mailbox) {
> -               acpi_mp_wake_mailbox = memremap(acpi_mp_wake_mailbox_paddr,
> -                                               sizeof(*acpi_mp_wake_mailbox),
> -                                               MEMREMAP_WB);
> -       }
> -
> -       /*
> -        * Mailbox memory is shared between the firmware and OS. Firmware will
> -        * listen on mailbox command address, and once it receives the wakeup
> -        * command, the CPU associated with the given apicid will be booted.
> -        *
> -        * The value of 'apic_id' and 'wakeup_vector' must be visible to the
> -        * firmware before the wakeup command is visible.  smp_store_release()
> -        * ensures ordering and visibility.
> -        */
> -       acpi_mp_wake_mailbox->apic_id       = apicid;
> -       acpi_mp_wake_mailbox->wakeup_vector = start_ip;
> -       smp_store_release(&acpi_mp_wake_mailbox->command,
> -                         ACPI_MP_WAKE_COMMAND_WAKEUP);
> -
> -       /*
> -        * Wait for the CPU to wake up.
> -        *
> -        * The CPU being woken up is essentially in a spin loop waiting to be
> -        * woken up. It should not take long for it wake up and acknowledge by
> -        * zeroing out ->command.
> -        *
> -        * ACPI specification doesn't provide any guidance on how long kernel
> -        * has to wait for a wake up acknowledgment. It also doesn't provide
> -        * a way to cancel a wake up request if it takes too long.
> -        *
> -        * In TDX environment, the VMM has control over how long it takes to
> -        * wake up secondary. It can postpone scheduling secondary vCPU
> -        * indefinitely. Giving up on wake up request and reporting error opens
> -        * possible attack vector for VMM: it can wake up a secondary CPU when
> -        * kernel doesn't expect it. Wait until positive result of the wake up
> -        * request.
> -        */
> -       while (READ_ONCE(acpi_mp_wake_mailbox->command))
> -               cpu_relax();
> -
> -       return 0;
> -}
> -
>  static void acpi_mp_disable_offlining(struct acpi_madt_multiproc_wakeup *mp_wake)
>  {
>         cpu_hotplug_disable_offlining();
> @@ -246,14 +182,3 @@ int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
>
>         return 0;
>  }
> -
> -void __init setup_mp_wakeup_mailbox(u64 mailbox_paddr)
> -{
> -       acpi_mp_wake_mailbox_paddr = mailbox_paddr;
> -       apic_update_callback(wakeup_secondary_cpu_64, acpi_wakeup_cpu);
> -}
> -
> -struct acpi_madt_multiproc_wakeup_mailbox *get_mp_wakeup_mailbox(void)
> -{
> -       return acpi_mp_wake_mailbox;
> -}
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index d6cf1e23c2a3..6f39ebe4d192 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -61,7 +61,9 @@
>  #include <linux/cpuhotplug.h>
>  #include <linux/mc146818rtc.h>
>  #include <linux/acpi.h>
> +#include <linux/io.h>
>
> +#include <asm/barrier.h>
>  #include <asm/acpi.h>
>  #include <asm/cacheinfo.h>
>  #include <asm/cpuid.h>
> @@ -1354,3 +1356,79 @@ void native_play_dead(void)
>  }
>
>  #endif
> +
> +#ifdef CONFIG_X86_64
> +/* Physical address of the Multiprocessor Wakeup Structure mailbox */
> +static u64 acpi_mp_wake_mailbox_paddr __ro_after_init;
> +
> +/* Virtual address of the Multiprocessor Wakeup Structure mailbox */
> +static struct acpi_madt_multiproc_wakeup_mailbox *acpi_mp_wake_mailbox;
> +
> +static int acpi_wakeup_cpu(u32 apicid, unsigned long start_ip)
> +{
> +       if (!acpi_mp_wake_mailbox_paddr) {
> +               pr_warn_once("No MADT mailbox: cannot bringup secondary CPUs. Booting with kexec?\n");
> +               return -EOPNOTSUPP;
> +       }
> +
> +       /*
> +        * Remap mailbox memory only for the first call to acpi_wakeup_cpu().
> +        *
> +        * Wakeup of secondary CPUs is fully serialized in the core code.
> +        * No need to protect acpi_mp_wake_mailbox from concurrent accesses.
> +        */
> +       if (!acpi_mp_wake_mailbox) {
> +               acpi_mp_wake_mailbox = memremap(acpi_mp_wake_mailbox_paddr,
> +                                               sizeof(*acpi_mp_wake_mailbox),
> +                                               MEMREMAP_WB);
> +       }
> +
> +       /*
> +        * Mailbox memory is shared between the firmware and OS. Firmware will
> +        * listen on mailbox command address, and once it receives the wakeup
> +        * command, the CPU associated with the given apicid will be booted.
> +        *
> +        * The value of 'apic_id' and 'wakeup_vector' must be visible to the
> +        * firmware before the wakeup command is visible.  smp_store_release()
> +        * ensures ordering and visibility.
> +        */
> +       acpi_mp_wake_mailbox->apic_id       = apicid;
> +       acpi_mp_wake_mailbox->wakeup_vector = start_ip;
> +       smp_store_release(&acpi_mp_wake_mailbox->command,
> +                         ACPI_MP_WAKE_COMMAND_WAKEUP);
> +
> +       /*
> +        * Wait for the CPU to wake up.
> +        *
> +        * The CPU being woken up is essentially in a spin loop waiting to be
> +        * woken up. It should not take long for it wake up and acknowledge by
> +        * zeroing out ->command.
> +        *
> +        * ACPI specification doesn't provide any guidance on how long kernel
> +        * has to wait for a wake up acknowledgment. It also doesn't provide
> +        * a way to cancel a wake up request if it takes too long.
> +        *
> +        * In TDX environment, the VMM has control over how long it takes to
> +        * wake up secondary. It can postpone scheduling secondary vCPU
> +        * indefinitely. Giving up on wake up request and reporting error opens
> +        * possible attack vector for VMM: it can wake up a secondary CPU when
> +        * kernel doesn't expect it. Wait until positive result of the wake up
> +        * request.
> +        */
> +       while (READ_ONCE(acpi_mp_wake_mailbox->command))
> +               cpu_relax();
> +
> +       return 0;
> +}
> +
> +void __init setup_mp_wakeup_mailbox(u64 mailbox_paddr)
> +{
> +       acpi_mp_wake_mailbox_paddr = mailbox_paddr;
> +       apic_update_callback(wakeup_secondary_cpu_64, acpi_wakeup_cpu);
> +}
> +
> +struct acpi_madt_multiproc_wakeup_mailbox *get_mp_wakeup_mailbox(void)
> +{
> +       return acpi_mp_wake_mailbox;
> +}
> +#endif
> --
> 2.43.0
>
>

Re: [PATCH v3 06/13] dt-bindings: reserved-memory: Wakeup Mailbox for Intel processors

[Rafael J. Wysocki]

On Sat, May 3, 2025 at 9:10 PM Ricardo Neri
<ricardo.neri-calderon@linux.intel.com> wrote:
>
> Add DeviceTree bindings for the wakeup mailbox used on Intel processors.
>
> x86 platforms commonly boot secondary CPUs using an INIT assert, de-assert
> followed by Start-Up IPI messages. The wakeup mailbox can be used when this
> mechanism unavailable.
>
> The wakeup mailbox offers more control to the operating system to boot
> secondary CPUs than a spin-table. It allows the reuse of same wakeup vector
> for all CPUs while maintaining control over which CPUs to boot and when.
> While it is possible to achieve the same level of control using a spin-
> table, it would require to specify a separate cpu-release-addr for each
> secondary CPU.
>
> Originally-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> ---
> Changes since v2:
>  - Implemented the mailbox as a reserved-memory node. Add to it a
>    `compatible` property. (Krzysztof)
>  - Explained the relationship between the mailbox and the `enable-mehod`
>    property of the CPU nodes.
>  - Expanded the documentation of the binding.
>
> Changes since v1:
>  - Added more details to the description of the binding.
>  - Added requirement a new requirement for cpu@N nodes to add an
>    `enable-method`.
> ---
>  .../reserved-memory/intel,wakeup-mailbox.yaml | 87 +++++++++++++++++++
>  1 file changed, 87 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reserved-memory/intel,wakeup-mailbox.yaml
>
> diff --git a/Documentation/devicetree/bindings/reserved-memory/intel,wakeup-mailbox.yaml b/Documentation/devicetree/bindings/reserved-memory/intel,wakeup-mailbox.yaml
> new file mode 100644
> index 000000000000..d97755b4673d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reserved-memory/intel,wakeup-mailbox.yaml
> @@ -0,0 +1,87 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/reserved-memory/intel,wakeup-mailbox.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Wakeup Mailbox for Intel processors
> +
> +description: |
> +  The Wakeup Mailbox provides a mechanism for the operating system to wake up
> +  secondary CPUs on Intel processors. It is an alternative to the INIT-!INIT-
> +  SIPI sequence used on most x86 systems.
> +
> +  Firmware must define the enable-method property in the CPU nodes as
> +  "intel,wakeup-mailbox" to use the mailbox.
> +
> +  Firmware implements the wakeup mailbox as a 4KB-aligned memory region of size
> +  of 4KB. It is memory that the firmware reserves so that each secondary CPU can
> +  have the operating system send a single message to them. The firmware is
> +  responsible for putting the secondary CPUs in a state to check the mailbox.
> +
> +  The structure of the mailbox is as follows:
> +
> +  Field           Byte   Byte  Description
> +                 Length Offset
> +  ------------------------------------------------------------------------------
> +  Command          2      0    Command to wake up the secondary CPU:
> +                                        0: Noop
> +                                        1: Wakeup: Jump to the wakeup_vector
> +                                        2-0xFFFF: Reserved:
> +  Reserved         2      2    Must be 0.
> +  APIC_ID          4      4    APIC ID of the secondary CPU to wake up.
> +  Wakeup_Vector    8      8    The wakeup address for the secondary CPU.
> +  ReservedForOs 2032     16    Reserved for OS use.
> +  ReservedForFW 2048   2048    Reserved for firmware use.
> +  ------------------------------------------------------------------------------
> +
> +  To wake up a secondary CPU, the operating system 1) prepares the wakeup
> +  routine; 2) populates the address of the wakeup routine address into the
> +  Wakeup_Vector field; 3) populates the APIC_ID field with the APIC ID of the
> +  secondary CPU; 4) writes Wakeup in the Command field. Upon receiving the
> +  Wakeup command, the secondary CPU acknowledges the command by writing Noop in
> +  the Command field and jumps to the Wakeup_Vector. The operating system can
> +  send the next command only after the Command field is changed to Noop.
> +
> +  The secondary CPU will no longer check the mailbox after waking up. The
> +  secondary CPU must ignore the command if its APIC_ID written in the mailbox
> +  does not match its own.
> +
> +  When entering the Wakeup_Vector, interrupts must be disabled and 64-bit
> +  addressing mode must be enabled. Paging mode must be enabled. The virtual
> +  address of the Wakeup_Vector page must be equal to its physical address.
> +  Segment selectors are not used.

This interface is defined in the ACPI specification and all of the
above information is present there.

Why are you copying it without acknowledging the source of it instead
of just saying where this interface is defined and pointing to its
definition?

> +
> +maintainers:
> +  - Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> +
> +allOf:
> +  - $ref: reserved-memory.yaml
> +
> +properties:
> +  compatible:
> +    const: intel,wakeup-mailbox
> +
> +  alignment:
> +    description: The mailbox must be 4KB-aligned.
> +    const: 0x1000
> +
> +required:
> +  - compatible
> +  - alignment

Why do you need the "alignment" property if the alignment is always the same?

> +  - reg
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    reserved-memory {
> +        #address-cells = <2>;
> +        #size-cells = <1>;
> +
> +        wakeup-mailbox@12340000 {
> +            compatible = "intel,wakeup-mailbox";
> +            alignment = <0x1000>;
> +            reg = <0x0 0x12340000 0x1000>;
> +        };
> +    };
> --

Re: [PATCH v3 04/13] dt-bindings: x86: Add CPU bindings for x86

[Ricardo Neri]

On Sun, May 04, 2025 at 06:45:59PM +0200, Krzysztof Kozlowski wrote:
> On Sat, May 03, 2025 at 12:15:06PM GMT, Ricardo Neri wrote:
> > Add bindings for CPUs in x86 architecture. Start by defining the `reg` and
> 
> What for?

Thank you for your quick feedback, Krzysztof!

Do you mean for what reason I want to start bindings for x86 CPUs? Or only
the `reg` property? If the former, it is to add an enable-method property to
x86 CPUs. If the latter, is to show the relationship between APIC and `reg`.

> 
> > `enable-method` properties and their relationship to x86 APIC ID and the
> > available mechanisms to boot secondary CPUs.
> > 
> > Start defining bindings for Intel processors. Bindings for other vendors
> > can be added later as needed.
> > 
> > Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> > ---
> 
> Not really tested so only limited review follows.

Sorry, I ran make dt_binding_check but only on this schema. I missed the
reported error.

> 
> >  .../devicetree/bindings/x86/cpus.yaml         | 80 +++++++++++++++++++
> >  1 file changed, 80 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/x86/cpus.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/x86/cpus.yaml b/Documentation/devicetree/bindings/x86/cpus.yaml
> > new file mode 100644
> > index 000000000000..108b3ad64aea
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/x86/cpus.yaml
> > @@ -0,0 +1,80 @@
> > +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/x86/cpus.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: x86 CPUs
> > +
> > +maintainers:
> > +  - Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> > +
> > +description: |
> > +  Description of x86 CPUs in a system through the "cpus" node.
> > +
> > +  Detailed information about the CPU architecture can be found in the Intel
> > +  Software Developer's Manual:
> > +    https://intel.com/sdm
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - intel,x86
> 
> That's architecture, not a CPU. CPUs are like 80286, 80386, so that's
> not even specific instruction set. I don't get what you need it for.

Am I to understand the the `compatible` property is not needed if the
bindings apply to any x86 CPU?

> 
> > +
> > +  reg:
> 
> Missing constraints.

I could add minItems. For maxItems, there is no limit to the number of
threads.

> 
> > +    description: |
> 
> Do not need '|' unless you need to preserve formatting.

OK.

> 
> > +      Local APIC ID of the CPU. If the CPU has more than one execution thread,
> > +      then the property is an array with one element per thread.
> > +
> > +  enable-method:
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    description: |
> > +      The method used to wake up secondary CPUs. This property is not needed if
> > +      the secondary processors are booted using INIT assert, de-assert followed
> > +      by Start-Up IPI messages as described in the Volume 3, Section 11.4 of
> > +      Intel Software Developer's Manual.
> > +
> > +      It is also optional for the bootstrap CPU.
> > +
> > +    oneOf:
> 
> I see only one entry, so didn't you want an enum?

Indeed, enum would be more appropriate.

> 
> > +      - items:
> 
> Not a list
> 
> > +          - const: intel,wakeup-mailbox
> 
> So every vendor is supposed to come with different name for the same
> feature? Or is wakeup-mailnox really intel specific, but then specific
> to which processors?

It would not be necessary for every vendor to provide a different name for
the same feature. I saw, however that the Devicetree specification requires
a [vendor],[method] stringlist.

Also, platform firmware for any processor could implement the wakeup
mailbox.

> 
> 
> > +            description: |
> > +              CPUs are woken up using the mailbox mechanism. The platform
> > +              firmware boots the secondary CPUs and puts them in a state
> > +              to check the mailbox for a wakeup command from the operating
> > +              system.
> > +
> > +required:
> > +  - reg
> > +  - compatible
> > +
> > +unevaluatedProperties: false
> 
> Missing ref in top-level or this is supposed to be additionalProps. See
> example-schema.

I will check.

> 
> > +
> > +examples:
> > +  - |
> > +    /*
> > +     * A system with two CPUs. cpu@0 is the bootstrap CPU and its status is
> > +     * "okay". It does not have the enable-method property. cpu@1 is a
> > +     * secondary CPU. Its status is "disabled" and defines the enable-method
> > +     * property.
> > +     */
> > +
> > +    cpus {
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +
> > +      cpu@0 {
> > +        reg = <0x0 0x1>;
> > +        compatible = "intel,x86";
> > +        status = "okay";
> 
> Drop

I will drop status = "okay"

> 
> > +      };
> > +
> > +      cpu@1 {
> > +        reg = <0x0 0x1>;
> > +        compatible = "intel,x86";
> > +        status = "disabled";
> 
> Why?

Because this is a secondary CPU that the operating system will enable using
the method specified in the `enable-method` property.

Thanks and BR,
Ricardo

Re: [PATCH v3 06/13] dt-bindings: reserved-memory: Wakeup Mailbox for Intel processors

[Ricardo Neri]

On Sun, May 04, 2025 at 06:51:17PM +0200, Krzysztof Kozlowski wrote:
> On Sat, May 03, 2025 at 12:15:08PM GMT, Ricardo Neri wrote:
> > Add DeviceTree bindings for the wakeup mailbox used on Intel processors.
> > 
> 
> Start using b4, so your cover letter will have proper lore links to
> previous versions.

Sure. Thanks for this suggestion I will follow it.

> 
> > x86 platforms commonly boot secondary CPUs using an INIT assert, de-assert
> > followed by Start-Up IPI messages. The wakeup mailbox can be used when this
> > mechanism unavailable.
> > 
> > The wakeup mailbox offers more control to the operating system to boot
> > secondary CPUs than a spin-table. It allows the reuse of same wakeup vector
> > for all CPUs while maintaining control over which CPUs to boot and when.
> > While it is possible to achieve the same level of control using a spin-
> > table, it would require to specify a separate cpu-release-addr for each
> > secondary CPU.
> > 
> > Originally-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
> 
> What does this tag mean? Why you cannot use standard tags - SoB and
> co-developed?

Yunhong handed this work to me. We did not work in it concurrently. But
yes, SoB and Co-developed-by also looks appropriate as two individuals
worked on this.

> 
> > Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> > ---
> > Changes since v2:
> >  - Implemented the mailbox as a reserved-memory node. Add to it a
> >    `compatible` property. (Krzysztof)
> >  - Explained the relationship between the mailbox and the `enable-mehod`
> >    property of the CPU nodes.
> >  - Expanded the documentation of the binding.
> > 
> > Changes since v1:
> >  - Added more details to the description of the binding.
> >  - Added requirement a new requirement for cpu@N nodes to add an
> >    `enable-method`.
> > ---
> >  .../reserved-memory/intel,wakeup-mailbox.yaml | 87 +++++++++++++++++++
> >  1 file changed, 87 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/reserved-memory/intel,wakeup-mailbox.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/reserved-memory/intel,wakeup-mailbox.yaml b/Documentation/devicetree/bindings/reserved-memory/intel,wakeup-mailbox.yaml
> > new file mode 100644
> > index 000000000000..d97755b4673d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/reserved-memory/intel,wakeup-mailbox.yaml
> > @@ -0,0 +1,87 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/reserved-memory/intel,wakeup-mailbox.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Wakeup Mailbox for Intel processors
> > +
> > +description: |
> > +  The Wakeup Mailbox provides a mechanism for the operating system to wake up
> > +  secondary CPUs on Intel processors. It is an alternative to the INIT-!INIT-
> > +  SIPI sequence used on most x86 systems.
> > +
> > +  Firmware must define the enable-method property in the CPU nodes as
> > +  "intel,wakeup-mailbox" to use the mailbox.
> > +
> > +  Firmware implements the wakeup mailbox as a 4KB-aligned memory region of size
> > +  of 4KB. It is memory that the firmware reserves so that each secondary CPU can
> > +  have the operating system send a single message to them. The firmware is
> > +  responsible for putting the secondary CPUs in a state to check the mailbox.
> > +
> > +  The structure of the mailbox is as follows:
> > +
> > +  Field           Byte   Byte  Description
> > +                 Length Offset
> > +  ------------------------------------------------------------------------------
> > +  Command          2      0    Command to wake up the secondary CPU:
> > +                                        0: Noop
> > +                                        1: Wakeup: Jump to the wakeup_vector
> > +                                        2-0xFFFF: Reserved:
> > +  Reserved         2      2    Must be 0.
> > +  APIC_ID          4      4    APIC ID of the secondary CPU to wake up.
> > +  Wakeup_Vector    8      8    The wakeup address for the secondary CPU.
> > +  ReservedForOs 2032     16    Reserved for OS use.
> > +  ReservedForFW 2048   2048    Reserved for firmware use.
> > +  ------------------------------------------------------------------------------
> > +
> > +  To wake up a secondary CPU, the operating system 1) prepares the wakeup
> > +  routine; 2) populates the address of the wakeup routine address into the
> > +  Wakeup_Vector field; 3) populates the APIC_ID field with the APIC ID of the
> > +  secondary CPU; 4) writes Wakeup in the Command field. Upon receiving the
> > +  Wakeup command, the secondary CPU acknowledges the command by writing Noop in
> > +  the Command field and jumps to the Wakeup_Vector. The operating system can
> > +  send the next command only after the Command field is changed to Noop.
> > +
> > +  The secondary CPU will no longer check the mailbox after waking up. The
> > +  secondary CPU must ignore the command if its APIC_ID written in the mailbox
> > +  does not match its own.
> > +
> > +  When entering the Wakeup_Vector, interrupts must be disabled and 64-bit
> > +  addressing mode must be enabled. Paging mode must be enabled. The virtual
> > +  address of the Wakeup_Vector page must be equal to its physical address.
> > +  Segment selectors are not used.
> > +
> > +maintainers:
> > +  - Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> > +
> > +allOf:
> > +  - $ref: reserved-memory.yaml
> > +
> > +properties:
> > +  compatible:
> > +    const: intel,wakeup-mailbox
> 
> If this is a device, then compatibles specific to devices. You do not
> get different rules than all other bindings... or this does not have to
> be binding at all. Why standard reserved-memory does not work for here?
> 
> Why do you need compatible in the first place?

Are you suggesting something like this?

reserved-memory {
	# address-cells = <2>;
	# size-cells = <1>;

	wakeup_mailbox: wakeupmb@fff000 {
		reg = < 0x0 0xfff000 0x1000>
	}

and then reference to the reserved memory using the wakeup_mailbox
phandle?

If this is the case, IIUC, this schema is not needed at all.

}
> 
> 
> > +
> > +  alignment:
> > +    description: The mailbox must be 4KB-aligned.
> 
> Then drop it because it is implied by compatible.

Sure I can drop it.

> 
> > +    const: 0x1000
> > +
> > +required:
> > +  - compatible
> > +  - alignment
> > +  - reg
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    reserved-memory {
> > +        #address-cells = <2>;
> > +        #size-cells = <1>;
> > +
> > +        wakeup-mailbox@12340000 {
> 
> Please use real addresses from real DT.
> 
> > +            compatible = "intel,wakeup-mailbox";
> > +            alignment = <0x1000>;
> > +            reg = <0x0 0x12340000 0x1000>;
> 
> reg is always the second property. See DTS coding style.

Sorry, I missed this convention.

Thanks and BR,
Ricardo 

Re: [PATCH v3 01/13] x86/acpi: Add a helper function to setup the wakeup mailbox

[Ricardo Neri]

On Mon, May 05, 2025 at 11:50:08AM +0200, Rafael J. Wysocki wrote:
> On Sat, May 3, 2025 at 9:10 PM Ricardo Neri
> <ricardo.neri-calderon@linux.intel.com> wrote:
> >
> > In preparation to move the functionality to wake secondary CPUs up out of
> > the ACPI code, add a helper function that stores the physical address of
> > the mailbox and updates the wakeup_secondary_cpu_64() APIC callback.
> >
> > There is a slight change in behavior: now the APIC callback is updated
> > before configuring CPU hotplug offline behavior. This is fine as the APIC
> > callback continues to be updated unconditionally, regardless of the
> > restriction on CPU offlining.
> >
> > The wakeup mailbox is only supported for CONFIG_X86_64 and needed only with
> > CONFIG_SMP=y.
> >
> > Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> > ---
> > Changes since v2:
> >  - Introduced this patch.
> >
> > Changes since v1:
> >  - N/A
> > ---
> >  arch/x86/include/asm/smp.h         |  4 ++++
> >  arch/x86/kernel/acpi/madt_wakeup.c | 10 +++++++---
> >  2 files changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
> > index 0c1c68039d6f..3622951d2ee0 100644
> > --- a/arch/x86/include/asm/smp.h
> > +++ b/arch/x86/include/asm/smp.h
> > @@ -146,6 +146,10 @@ static inline struct cpumask *cpu_l2c_shared_mask(int cpu)
> >         return per_cpu(cpu_l2c_shared_map, cpu);
> >  }
> >
> > +#ifdef CONFIG_X86_64
> > +void setup_mp_wakeup_mailbox(u64 addr);
> > +#endif

Thank you for your feedback, Rafael!

> 
> The #ifdef is only necessary if you are going to provide an
> alternative for builds in which the symbol is unset.

I see. All callers will be built only with CONFIG_X86_64. I will remove
this #ifdef.

> 
> > +
> >  #else /* !CONFIG_SMP */
> >  #define wbinvd_on_cpu(cpu)     wbinvd()
> >  static inline int wbinvd_on_all_cpus(void)
> > diff --git a/arch/x86/kernel/acpi/madt_wakeup.c b/arch/x86/kernel/acpi/madt_wakeup.c
> > index f36f28405dcc..04de3db307de 100644
> > --- a/arch/x86/kernel/acpi/madt_wakeup.c
> > +++ b/arch/x86/kernel/acpi/madt_wakeup.c
> > @@ -227,7 +227,7 @@ int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
> >
> >         acpi_table_print_madt_entry(&header->common);
> >
> > -       acpi_mp_wake_mailbox_paddr = mp_wake->mailbox_address;
> > +       setup_mp_wakeup_mailbox(mp_wake->mailbox_address);
> 
> I'd prefer acpi_setup_mp_wakeup_mailbox().

Sure. This looks like a more appropriate name.

Thanks and BR,
Ricardo

Re: [PATCH v3 02/13] x86/acpi: Add a helper function to get a pointer to the wakeup mailbox

[Ricardo Neri]

On Mon, May 05, 2025 at 11:55:03AM +0200, Rafael J. Wysocki wrote:
> On Sat, May 3, 2025 at 9:10 PM Ricardo Neri
> <ricardo.neri-calderon@linux.intel.com> wrote:
> >
> > In preparation to move the functionality to wake secondary CPUs up out
> > of the ACPI code, add a helper function to get a pointer to the mailbox.
> >
> > Use this helper function only in the portions of the code for which the
> > variable acpi_mp_wake_mailbox will be out of scope once it is relocated
> > out of the ACPI directory.
> >
> > The wakeup mailbox is only supported for CONFIG_X86_64 and needed only
> > with CONFIG_SMP.
> >
> > Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> > ---
> > Changes since v2:
> >  - Introduced this patch.
> 
> Have you considered merging it with the previous patch?  They both do
> analogous things.

Indeed, I can merge these two patches.

Thanks and BR,
Ricardo

Re: [PATCH v3 03/13] x86/acpi: Move acpi_wakeup_cpu() and helpers to smpboot.c

[Ricardo Neri]

On Mon, May 05, 2025 at 12:03:13PM +0200, Rafael J. Wysocki wrote:
> On Sat, May 3, 2025 at 9:10 PM Ricardo Neri
> <ricardo.neri-calderon@linux.intel.com> wrote:
> >
> > The bootstrap processor uses acpi_wakeup_cpu() to indicate to firmware that
> > it wants to boot a secondary CPU using a mailbox as described in the
> > Multiprocessor Wakeup Structure of the ACPI specification.
> >
> > The wakeup mailbox does not strictly require support from ACPI.
> 
> Well, except that it is defined by the ACPI specification.

That is true.

> 
> > The platform firmware can implement a mailbox compatible in structure and
> > operation and enumerate it using other mechanisms such a DeviceTree graph.
> 
> So is there a specification defining this mechanism?
> 
> It is generally not sufficient to put the code and DT bindings
> unilaterally into the OS and expect the firmware to follow suit.
> 

This mechanism is described in the section 4.3.5 of the Intel TDX
Virtual Firmware Design Guide [1], but it refeers to the ACPI
specification for the interface.

> > Move the code used to setup and use the mailbox out of the ACPI
> > directory to use it when support for ACPI is not available or needed.
> 
> I think that the code implementing interfaces defined by the ACPI
> specification is not generic and so it should not be built when the
> kernel is configured without ACPI support.

Support for ACPI would not be used on systems describing hardware using
a DeviceTree graph. My goal is to have a common interface that both
DT and ACPI can use. I think what is missing is that common interface.

Would it be preferred to have a separate implementation that is
functionally equivalent?

Thanks and BR,
Ricardo


[1]. https://cdrdv2-public.intel.com/733585/tdx-virtual-firmware-design-guide-rev-004-20231206.pdf

Re: [PATCH v3 06/13] dt-bindings: reserved-memory: Wakeup Mailbox for Intel processors

[Ricardo Neri]

On Mon, May 05, 2025 at 03:07:43PM +0200, Rafael J. Wysocki wrote:
> On Sat, May 3, 2025 at 9:10 PM Ricardo Neri
> <ricardo.neri-calderon@linux.intel.com> wrote:
> >
> > Add DeviceTree bindings for the wakeup mailbox used on Intel processors.
> >
> > x86 platforms commonly boot secondary CPUs using an INIT assert, de-assert
> > followed by Start-Up IPI messages. The wakeup mailbox can be used when this
> > mechanism unavailable.
> >
> > The wakeup mailbox offers more control to the operating system to boot
> > secondary CPUs than a spin-table. It allows the reuse of same wakeup vector
> > for all CPUs while maintaining control over which CPUs to boot and when.
> > While it is possible to achieve the same level of control using a spin-
> > table, it would require to specify a separate cpu-release-addr for each
> > secondary CPU.
> >
> > Originally-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
> > Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> > ---
> > Changes since v2:
> >  - Implemented the mailbox as a reserved-memory node. Add to it a
> >    `compatible` property. (Krzysztof)
> >  - Explained the relationship between the mailbox and the `enable-mehod`
> >    property of the CPU nodes.
> >  - Expanded the documentation of the binding.
> >
> > Changes since v1:
> >  - Added more details to the description of the binding.
> >  - Added requirement a new requirement for cpu@N nodes to add an
> >    `enable-method`.
> > ---
> >  .../reserved-memory/intel,wakeup-mailbox.yaml | 87 +++++++++++++++++++
> >  1 file changed, 87 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/reserved-memory/intel,wakeup-mailbox.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/reserved-memory/intel,wakeup-mailbox.yaml b/Documentation/devicetree/bindings/reserved-memory/intel,wakeup-mailbox.yaml
> > new file mode 100644
> > index 000000000000..d97755b4673d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/reserved-memory/intel,wakeup-mailbox.yaml
> > @@ -0,0 +1,87 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/reserved-memory/intel,wakeup-mailbox.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Wakeup Mailbox for Intel processors
> > +
> > +description: |
> > +  The Wakeup Mailbox provides a mechanism for the operating system to wake up
> > +  secondary CPUs on Intel processors. It is an alternative to the INIT-!INIT-
> > +  SIPI sequence used on most x86 systems.
> > +
> > +  Firmware must define the enable-method property in the CPU nodes as
> > +  "intel,wakeup-mailbox" to use the mailbox.
> > +
> > +  Firmware implements the wakeup mailbox as a 4KB-aligned memory region of size
> > +  of 4KB. It is memory that the firmware reserves so that each secondary CPU can
> > +  have the operating system send a single message to them. The firmware is
> > +  responsible for putting the secondary CPUs in a state to check the mailbox.
> > +
> > +  The structure of the mailbox is as follows:
> > +
> > +  Field           Byte   Byte  Description
> > +                 Length Offset
> > +  ------------------------------------------------------------------------------
> > +  Command          2      0    Command to wake up the secondary CPU:
> > +                                        0: Noop
> > +                                        1: Wakeup: Jump to the wakeup_vector
> > +                                        2-0xFFFF: Reserved:
> > +  Reserved         2      2    Must be 0.
> > +  APIC_ID          4      4    APIC ID of the secondary CPU to wake up.
> > +  Wakeup_Vector    8      8    The wakeup address for the secondary CPU.
> > +  ReservedForOs 2032     16    Reserved for OS use.
> > +  ReservedForFW 2048   2048    Reserved for firmware use.
> > +  ------------------------------------------------------------------------------
> > +
> > +  To wake up a secondary CPU, the operating system 1) prepares the wakeup
> > +  routine; 2) populates the address of the wakeup routine address into the
> > +  Wakeup_Vector field; 3) populates the APIC_ID field with the APIC ID of the
> > +  secondary CPU; 4) writes Wakeup in the Command field. Upon receiving the
> > +  Wakeup command, the secondary CPU acknowledges the command by writing Noop in
> > +  the Command field and jumps to the Wakeup_Vector. The operating system can
> > +  send the next command only after the Command field is changed to Noop.
> > +
> > +  The secondary CPU will no longer check the mailbox after waking up. The
> > +  secondary CPU must ignore the command if its APIC_ID written in the mailbox
> > +  does not match its own.
> > +
> > +  When entering the Wakeup_Vector, interrupts must be disabled and 64-bit
> > +  addressing mode must be enabled. Paging mode must be enabled. The virtual
> > +  address of the Wakeup_Vector page must be equal to its physical address.
> > +  Segment selectors are not used.
> 
> This interface is defined in the ACPI specification and all of the
> above information is present there.
> 
> Why are you copying it without acknowledging the source of it instead
> of just saying where this interface is defined and pointing to its
> definition?

There was a discussion in the past about preferring a full description of
the mailbox instead of references to ACPI [1]. I am happy to acknowledge
the source in the changeset description. I explicitly acknowledge the ACPI
specification in the cover letter.

[1]. https://lore.kernel.org/all/20240809232928.GB25056@yjiang5-mobl.amr.corp.intel.com/

> 
> > +
> > +maintainers:
> > +  - Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> > +
> > +allOf:
> > +  - $ref: reserved-memory.yaml
> > +
> > +properties:
> > +  compatible:
> > +    const: intel,wakeup-mailbox
> > +
> > +  alignment:
> > +    description: The mailbox must be 4KB-aligned.
> > +    const: 0x1000
> > +
> > +required:
> > +  - compatible
> > +  - alignment
> 
> Why do you need the "alignment" property if the alignment is always the same?

I want to enforce a 4KB alignment. It can also be inferred from the
address of the mailbox.

Thanks and BR,
Ricardo

Re: [PATCH v3 06/13] dt-bindings: reserved-memory: Wakeup Mailbox for Intel processors

[Krzysztof Kozlowski]

On Mon, May 05, 2025 at 10:16:10PM GMT, Ricardo Neri wrote:
> > If this is a device, then compatibles specific to devices. You do not
> > get different rules than all other bindings... or this does not have to
> > be binding at all. Why standard reserved-memory does not work for here?
> > 
> > Why do you need compatible in the first place?
> 
> Are you suggesting something like this?
> 
> reserved-memory {
> 	# address-cells = <2>;
> 	# size-cells = <1>;
> 
> 	wakeup_mailbox: wakeupmb@fff000 {
> 		reg = < 0x0 0xfff000 0x1000>
> 	}
> 
> and then reference to the reserved memory using the wakeup_mailbox
> phandle?

Yes just like every other, typical reserved memory block.

Best regards,
Krzysztof

Re: [PATCH v3 04/13] dt-bindings: x86: Add CPU bindings for x86

[Krzysztof Kozlowski]

On Mon, May 05, 2025 at 09:52:35PM GMT, Ricardo Neri wrote:
> On Sun, May 04, 2025 at 06:45:59PM +0200, Krzysztof Kozlowski wrote:
> > On Sat, May 03, 2025 at 12:15:06PM GMT, Ricardo Neri wrote:
> > > Add bindings for CPUs in x86 architecture. Start by defining the `reg` and
> > 
> > What for?
> 
> Thank you for your quick feedback, Krzysztof!
> 
> Do you mean for what reason I want to start bindings for x86 CPUs? Or only

Yes. For which devices, what purpose.

> the `reg` property? If the former, it is to add an enable-method property to
> x86 CPUs. If the latter, is to show the relationship between APIC and `reg`.
> 
> > 
> > > `enable-method` properties and their relationship to x86 APIC ID and the
> > > available mechanisms to boot secondary CPUs.
> > > 
> > > Start defining bindings for Intel processors. Bindings for other vendors
> > > can be added later as needed.
> > > 
> > > Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> > > ---
> > 
> > Not really tested so only limited review follows.
> 
> Sorry, I ran make dt_binding_check but only on this schema. I missed the
> reported error.
> 
> > 
> > >  .../devicetree/bindings/x86/cpus.yaml         | 80 +++++++++++++++++++
> > >  1 file changed, 80 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/x86/cpus.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/x86/cpus.yaml b/Documentation/devicetree/bindings/x86/cpus.yaml
> > > new file mode 100644
> > > index 000000000000..108b3ad64aea
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/x86/cpus.yaml
> > > @@ -0,0 +1,80 @@
> > > +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/x86/cpus.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: x86 CPUs
> > > +
> > > +maintainers:
> > > +  - Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> > > +
> > > +description: |
> > > +  Description of x86 CPUs in a system through the "cpus" node.
> > > +
> > > +  Detailed information about the CPU architecture can be found in the Intel
> > > +  Software Developer's Manual:
> > > +    https://intel.com/sdm
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - intel,x86
> > 
> > That's architecture, not a CPU. CPUs are like 80286, 80386, so that's
> > not even specific instruction set. I don't get what you need it for.
> 
> Am I to understand the the `compatible` property is not needed if the
> bindings apply to any x86 CPU?

Every device needs compatible. Its meaning is explained:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#compatible

If you add here a device representing CPU, then look at existing
bindings for CPUs how they do it.

It again feels like you add DT for platform which is not a real thing.
If you use DT, you do not get different rules, therefore read all
standard guides and tutorials (there were many, quite comprehensive).


> 
> > 
> > > +
> > > +  reg:
> > 
> > Missing constraints.
> 
> I could add minItems. For maxItems, there is no limit to the number of
> threads.

I am pretty sure that any given CPU, e.g. 80486 has a fixed number of
threads...

> 
> > 
> > > +    description: |
> > 
> > Do not need '|' unless you need to preserve formatting.
> 
> OK.
> 
> > 
> > > +      Local APIC ID of the CPU. If the CPU has more than one execution thread,
> > > +      then the property is an array with one element per thread.
> > > +
> > > +  enable-method:
> > > +    $ref: /schemas/types.yaml#/definitions/string
> > > +    description: |
> > > +      The method used to wake up secondary CPUs. This property is not needed if
> > > +      the secondary processors are booted using INIT assert, de-assert followed
> > > +      by Start-Up IPI messages as described in the Volume 3, Section 11.4 of
> > > +      Intel Software Developer's Manual.
> > > +
> > > +      It is also optional for the bootstrap CPU.
> > > +
> > > +    oneOf:
> > 
> > I see only one entry, so didn't you want an enum?
> 
> Indeed, enum would be more appropriate.
> 
> > 
> > > +      - items:
> > 
> > Not a list
> > 
> > > +          - const: intel,wakeup-mailbox
> > 
> > So every vendor is supposed to come with different name for the same
> > feature? Or is wakeup-mailnox really intel specific, but then specific
> > to which processors?
> 
> It would not be necessary for every vendor to provide a different name for
> the same feature. I saw, however that the Devicetree specification requires
> a [vendor],[method] stringlist.

Indeed, it's fine then.

> 
> Also, platform firmware for any processor could implement the wakeup
> mailbox.
> 
> > 
> > 
> > > +            description: |
> > > +              CPUs are woken up using the mailbox mechanism. The platform
> > > +              firmware boots the secondary CPUs and puts them in a state
> > > +              to check the mailbox for a wakeup command from the operating
> > > +              system.
> > > +
> > > +required:
> > > +  - reg
> > > +  - compatible
> > > +
> > > +unevaluatedProperties: false
> > 
> > Missing ref in top-level or this is supposed to be additionalProps. See
> > example-schema.
> 
> I will check.
> 
> > 
> > > +
> > > +examples:
> > > +  - |
> > > +    /*
> > > +     * A system with two CPUs. cpu@0 is the bootstrap CPU and its status is
> > > +     * "okay". It does not have the enable-method property. cpu@1 is a
> > > +     * secondary CPU. Its status is "disabled" and defines the enable-method
> > > +     * property.
> > > +     */
> > > +
> > > +    cpus {
> > > +      #address-cells = <1>;
> > > +      #size-cells = <0>;
> > > +
> > > +      cpu@0 {
> > > +        reg = <0x0 0x1>;
> > > +        compatible = "intel,x86";
> > > +        status = "okay";
> > 
> > Drop
> 
> I will drop status = "okay"
> 
> > 
> > > +      };
> > > +
> > > +      cpu@1 {
> > > +        reg = <0x0 0x1>;
> > > +        compatible = "intel,x86";
> > > +        status = "disabled";
> > 
> > Why?
> 
> Because this is a secondary CPU that the operating system will enable using
> the method specified in the `enable-method` property.

OK, so this was intentional to express it is in quiescent state.

Best regards,
Krzysztof

Re: [PATCH v3 06/13] dt-bindings: reserved-memory: Wakeup Mailbox for Intel processors

[Rafael J. Wysocki]

On Tue, May 6, 2025 at 7:46 AM Ricardo Neri
<ricardo.neri-calderon@linux.intel.com> wrote:
>
> On Mon, May 05, 2025 at 03:07:43PM +0200, Rafael J. Wysocki wrote:
> > On Sat, May 3, 2025 at 9:10 PM Ricardo Neri
> > <ricardo.neri-calderon@linux.intel.com> wrote:
> > >
> > > Add DeviceTree bindings for the wakeup mailbox used on Intel processors.
> > >
> > > x86 platforms commonly boot secondary CPUs using an INIT assert, de-assert
> > > followed by Start-Up IPI messages. The wakeup mailbox can be used when this
> > > mechanism unavailable.
> > >
> > > The wakeup mailbox offers more control to the operating system to boot
> > > secondary CPUs than a spin-table. It allows the reuse of same wakeup vector
> > > for all CPUs while maintaining control over which CPUs to boot and when.
> > > While it is possible to achieve the same level of control using a spin-
> > > table, it would require to specify a separate cpu-release-addr for each
> > > secondary CPU.
> > >
> > > Originally-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
> > > Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> > > ---
> > > Changes since v2:
> > >  - Implemented the mailbox as a reserved-memory node. Add to it a
> > >    `compatible` property. (Krzysztof)
> > >  - Explained the relationship between the mailbox and the `enable-mehod`
> > >    property of the CPU nodes.
> > >  - Expanded the documentation of the binding.
> > >
> > > Changes since v1:
> > >  - Added more details to the description of the binding.
> > >  - Added requirement a new requirement for cpu@N nodes to add an
> > >    `enable-method`.
> > > ---
> > >  .../reserved-memory/intel,wakeup-mailbox.yaml | 87 +++++++++++++++++++
> > >  1 file changed, 87 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/reserved-memory/intel,wakeup-mailbox.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/reserved-memory/intel,wakeup-mailbox.yaml b/Documentation/devicetree/bindings/reserved-memory/intel,wakeup-mailbox.yaml
> > > new file mode 100644
> > > index 000000000000..d97755b4673d
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/reserved-memory/intel,wakeup-mailbox.yaml
> > > @@ -0,0 +1,87 @@
> > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/reserved-memory/intel,wakeup-mailbox.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Wakeup Mailbox for Intel processors
> > > +
> > > +description: |
> > > +  The Wakeup Mailbox provides a mechanism for the operating system to wake up
> > > +  secondary CPUs on Intel processors. It is an alternative to the INIT-!INIT-
> > > +  SIPI sequence used on most x86 systems.
> > > +
> > > +  Firmware must define the enable-method property in the CPU nodes as
> > > +  "intel,wakeup-mailbox" to use the mailbox.
> > > +
> > > +  Firmware implements the wakeup mailbox as a 4KB-aligned memory region of size
> > > +  of 4KB. It is memory that the firmware reserves so that each secondary CPU can
> > > +  have the operating system send a single message to them. The firmware is
> > > +  responsible for putting the secondary CPUs in a state to check the mailbox.
> > > +
> > > +  The structure of the mailbox is as follows:
> > > +
> > > +  Field           Byte   Byte  Description
> > > +                 Length Offset
> > > +  ------------------------------------------------------------------------------
> > > +  Command          2      0    Command to wake up the secondary CPU:
> > > +                                        0: Noop
> > > +                                        1: Wakeup: Jump to the wakeup_vector
> > > +                                        2-0xFFFF: Reserved:
> > > +  Reserved         2      2    Must be 0.
> > > +  APIC_ID          4      4    APIC ID of the secondary CPU to wake up.
> > > +  Wakeup_Vector    8      8    The wakeup address for the secondary CPU.
> > > +  ReservedForOs 2032     16    Reserved for OS use.
> > > +  ReservedForFW 2048   2048    Reserved for firmware use.
> > > +  ------------------------------------------------------------------------------
> > > +
> > > +  To wake up a secondary CPU, the operating system 1) prepares the wakeup
> > > +  routine; 2) populates the address of the wakeup routine address into the
> > > +  Wakeup_Vector field; 3) populates the APIC_ID field with the APIC ID of the
> > > +  secondary CPU; 4) writes Wakeup in the Command field. Upon receiving the
> > > +  Wakeup command, the secondary CPU acknowledges the command by writing Noop in
> > > +  the Command field and jumps to the Wakeup_Vector. The operating system can
> > > +  send the next command only after the Command field is changed to Noop.
> > > +
> > > +  The secondary CPU will no longer check the mailbox after waking up. The
> > > +  secondary CPU must ignore the command if its APIC_ID written in the mailbox
> > > +  does not match its own.
> > > +
> > > +  When entering the Wakeup_Vector, interrupts must be disabled and 64-bit
> > > +  addressing mode must be enabled. Paging mode must be enabled. The virtual
> > > +  address of the Wakeup_Vector page must be equal to its physical address.
> > > +  Segment selectors are not used.
> >
> > This interface is defined in the ACPI specification and all of the
> > above information is present there.
> >
> > Why are you copying it without acknowledging the source of it instead
> > of just saying where this interface is defined and pointing to its
> > definition?
>
> There was a discussion in the past about preferring a full description of
> the mailbox instead of references to ACPI [1]. I am happy to acknowledge
> the source in the changeset description. I explicitly acknowledge the ACPI
> specification in the cover letter.
>
> [1]. https://lore.kernel.org/all/20240809232928.GB25056@yjiang5-mobl.amr.corp.intel.com/

In which I clearly was not involved.

Acknowledgement in the cover letter is fine, but insufficient.

Also, there is the question regarding what happens when the ASWG
decides to update this part of the ACPI specification.  Is the DT
binding going to be updated too?

> >
> > > +
> > > +maintainers:
> > > +  - Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> > > +
> > > +allOf:
> > > +  - $ref: reserved-memory.yaml
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: intel,wakeup-mailbox
> > > +
> > > +  alignment:
> > > +    description: The mailbox must be 4KB-aligned.
> > > +    const: 0x1000
> > > +
> > > +required:
> > > +  - compatible
> > > +  - alignment
> >
> > Why do you need the "alignment" property if the alignment is always the same?
>
> I want to enforce a 4KB alignment. It can also be inferred from the
> address of the mailbox.
>
> Thanks and BR,
> Ricardo

Re: [PATCH v3 03/13] x86/acpi: Move acpi_wakeup_cpu() and helpers to smpboot.c

[Rafael J. Wysocki]

On Tue, May 6, 2025 at 7:32 AM Ricardo Neri
<ricardo.neri-calderon@linux.intel.com> wrote:
>
> On Mon, May 05, 2025 at 12:03:13PM +0200, Rafael J. Wysocki wrote:
> > On Sat, May 3, 2025 at 9:10 PM Ricardo Neri
> > <ricardo.neri-calderon@linux.intel.com> wrote:
> > >
> > > The bootstrap processor uses acpi_wakeup_cpu() to indicate to firmware that
> > > it wants to boot a secondary CPU using a mailbox as described in the
> > > Multiprocessor Wakeup Structure of the ACPI specification.
> > >
> > > The wakeup mailbox does not strictly require support from ACPI.
> >
> > Well, except that it is defined by the ACPI specification.
>
> That is true.
>
> >
> > > The platform firmware can implement a mailbox compatible in structure and
> > > operation and enumerate it using other mechanisms such a DeviceTree graph.
> >
> > So is there a specification defining this mechanism?
> >
> > It is generally not sufficient to put the code and DT bindings
> > unilaterally into the OS and expect the firmware to follow suit.
> >
>
> This mechanism is described in the section 4.3.5 of the Intel TDX
> Virtual Firmware Design Guide [1], but it refeers to the ACPI
> specification for the interface.

Yes, it does.

> > > Move the code used to setup and use the mailbox out of the ACPI
> > > directory to use it when support for ACPI is not available or needed.
> >
> > I think that the code implementing interfaces defined by the ACPI
> > specification is not generic and so it should not be built when the
> > kernel is configured without ACPI support.
>
> Support for ACPI would not be used on systems describing hardware using
> a DeviceTree graph. My goal is to have a common interface that both
> DT and ACPI can use. I think what is missing is that common interface.

I get the general idea. :-)

> Would it be preferred to have a separate implementation that is
> functionally equivalent?

Functionally equivalent on the OS side, that is.

It would be better, but honestly I'm not sure if this is going to be
sufficient to eliminate the dependency on the ACPI spec.  It is just
as you want the firmware to implement this tiny bit of the ACPI spec
even though it is not implementing the rest of it.

> [1]. https://cdrdv2-public.intel.com/733585/tdx-virtual-firmware-design-guide-rev-004-20231206.pdf

Re: [PATCH v3 06/13] dt-bindings: reserved-memory: Wakeup Mailbox for Intel processors

[Ricardo Neri]

On Tue, May 06, 2025 at 09:10:22AM +0200, Krzysztof Kozlowski wrote:
> On Mon, May 05, 2025 at 10:16:10PM GMT, Ricardo Neri wrote:
> > > If this is a device, then compatibles specific to devices. You do not
> > > get different rules than all other bindings... or this does not have to
> > > be binding at all. Why standard reserved-memory does not work for here?
> > > 
> > > Why do you need compatible in the first place?
> > 
> > Are you suggesting something like this?
> > 
> > reserved-memory {
> > 	# address-cells = <2>;
> > 	# size-cells = <1>;
> > 
> > 	wakeup_mailbox: wakeupmb@fff000 {
> > 		reg = < 0x0 0xfff000 0x1000>
> > 	}
> > 
> > and then reference to the reserved memory using the wakeup_mailbox
> > phandle?
> 
> Yes just like every other, typical reserved memory block.

Thanks! I will take this approach and drop this patch.

BR,
Ricardo 

Re: [PATCH v3 03/13] x86/acpi: Move acpi_wakeup_cpu() and helpers to smpboot.c

[Ricardo Neri]

On Tue, May 06, 2025 at 07:26:01PM +0200, Rafael J. Wysocki wrote:
> On Tue, May 6, 2025 at 7:32 AM Ricardo Neri
> <ricardo.neri-calderon@linux.intel.com> wrote:
> >
> > On Mon, May 05, 2025 at 12:03:13PM +0200, Rafael J. Wysocki wrote:
> > > On Sat, May 3, 2025 at 9:10 PM Ricardo Neri
> > > <ricardo.neri-calderon@linux.intel.com> wrote:
> > > >
> > > > The bootstrap processor uses acpi_wakeup_cpu() to indicate to firmware that
> > > > it wants to boot a secondary CPU using a mailbox as described in the
> > > > Multiprocessor Wakeup Structure of the ACPI specification.
> > > >
> > > > The wakeup mailbox does not strictly require support from ACPI.
> > >
> > > Well, except that it is defined by the ACPI specification.
> >
> > That is true.
> >
> > >
> > > > The platform firmware can implement a mailbox compatible in structure and
> > > > operation and enumerate it using other mechanisms such a DeviceTree graph.
> > >
> > > So is there a specification defining this mechanism?
> > >
> > > It is generally not sufficient to put the code and DT bindings
> > > unilaterally into the OS and expect the firmware to follow suit.
> > >
> >
> > This mechanism is described in the section 4.3.5 of the Intel TDX
> > Virtual Firmware Design Guide [1], but it refeers to the ACPI
> > specification for the interface.
> 
> Yes, it does.
> 
> > > > Move the code used to setup and use the mailbox out of the ACPI
> > > > directory to use it when support for ACPI is not available or needed.
> > >
> > > I think that the code implementing interfaces defined by the ACPI
> > > specification is not generic and so it should not be built when the
> > > kernel is configured without ACPI support.
> >
> > Support for ACPI would not be used on systems describing hardware using
> > a DeviceTree graph. My goal is to have a common interface that both
> > DT and ACPI can use. I think what is missing is that common interface.
> 
> I get the general idea. :-)
> 
> > Would it be preferred to have a separate implementation that is
> > functionally equivalent?
> 
> Functionally equivalent on the OS side, that is.
> 
> It would be better, but honestly I'm not sure if this is going to be
> sufficient to eliminate the dependency on the ACPI spec.  It is just
> as you want the firmware to implement this tiny bit of the ACPI spec
> even though it is not implementing the rest of it.

Yes, that is all I need: the address of the mailbox and firmware that
implements the mailbox interface as described in the ACPI Multiprocessor
Structure.

Thanks and BR,
Ricardo
> 

Re: [PATCH v3 06/13] dt-bindings: reserved-memory: Wakeup Mailbox for Intel processors

[Ricardo Neri]

On Tue, May 06, 2025 at 04:00:56PM +0200, Rafael J. Wysocki wrote:
> On Tue, May 6, 2025 at 7:46 AM Ricardo Neri
> <ricardo.neri-calderon@linux.intel.com> wrote:
> >
> > On Mon, May 05, 2025 at 03:07:43PM +0200, Rafael J. Wysocki wrote:
> > > On Sat, May 3, 2025 at 9:10 PM Ricardo Neri
> > > <ricardo.neri-calderon@linux.intel.com> wrote:
> > > >
> > > > Add DeviceTree bindings for the wakeup mailbox used on Intel processors.
> > > >
> > > > x86 platforms commonly boot secondary CPUs using an INIT assert, de-assert
> > > > followed by Start-Up IPI messages. The wakeup mailbox can be used when this
> > > > mechanism unavailable.
> > > >
> > > > The wakeup mailbox offers more control to the operating system to boot
> > > > secondary CPUs than a spin-table. It allows the reuse of same wakeup vector
> > > > for all CPUs while maintaining control over which CPUs to boot and when.
> > > > While it is possible to achieve the same level of control using a spin-
> > > > table, it would require to specify a separate cpu-release-addr for each
> > > > secondary CPU.
> > > >
> > > > Originally-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
> > > > Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> > > > ---
> > > > Changes since v2:
> > > >  - Implemented the mailbox as a reserved-memory node. Add to it a
> > > >    `compatible` property. (Krzysztof)
> > > >  - Explained the relationship between the mailbox and the `enable-mehod`
> > > >    property of the CPU nodes.
> > > >  - Expanded the documentation of the binding.
> > > >
> > > > Changes since v1:
> > > >  - Added more details to the description of the binding.
> > > >  - Added requirement a new requirement for cpu@N nodes to add an
> > > >    `enable-method`.
> > > > ---
> > > >  .../reserved-memory/intel,wakeup-mailbox.yaml | 87 +++++++++++++++++++
> > > >  1 file changed, 87 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/reserved-memory/intel,wakeup-mailbox.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/reserved-memory/intel,wakeup-mailbox.yaml b/Documentation/devicetree/bindings/reserved-memory/intel,wakeup-mailbox.yaml
> > > > new file mode 100644
> > > > index 000000000000..d97755b4673d
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/reserved-memory/intel,wakeup-mailbox.yaml
> > > > @@ -0,0 +1,87 @@
> > > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/reserved-memory/intel,wakeup-mailbox.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Wakeup Mailbox for Intel processors
> > > > +
> > > > +description: |
> > > > +  The Wakeup Mailbox provides a mechanism for the operating system to wake up
> > > > +  secondary CPUs on Intel processors. It is an alternative to the INIT-!INIT-
> > > > +  SIPI sequence used on most x86 systems.
> > > > +
> > > > +  Firmware must define the enable-method property in the CPU nodes as
> > > > +  "intel,wakeup-mailbox" to use the mailbox.
> > > > +
> > > > +  Firmware implements the wakeup mailbox as a 4KB-aligned memory region of size
> > > > +  of 4KB. It is memory that the firmware reserves so that each secondary CPU can
> > > > +  have the operating system send a single message to them. The firmware is
> > > > +  responsible for putting the secondary CPUs in a state to check the mailbox.
> > > > +
> > > > +  The structure of the mailbox is as follows:
> > > > +
> > > > +  Field           Byte   Byte  Description
> > > > +                 Length Offset
> > > > +  ------------------------------------------------------------------------------
> > > > +  Command          2      0    Command to wake up the secondary CPU:
> > > > +                                        0: Noop
> > > > +                                        1: Wakeup: Jump to the wakeup_vector
> > > > +                                        2-0xFFFF: Reserved:
> > > > +  Reserved         2      2    Must be 0.
> > > > +  APIC_ID          4      4    APIC ID of the secondary CPU to wake up.
> > > > +  Wakeup_Vector    8      8    The wakeup address for the secondary CPU.
> > > > +  ReservedForOs 2032     16    Reserved for OS use.
> > > > +  ReservedForFW 2048   2048    Reserved for firmware use.
> > > > +  ------------------------------------------------------------------------------
> > > > +
> > > > +  To wake up a secondary CPU, the operating system 1) prepares the wakeup
> > > > +  routine; 2) populates the address of the wakeup routine address into the
> > > > +  Wakeup_Vector field; 3) populates the APIC_ID field with the APIC ID of the
> > > > +  secondary CPU; 4) writes Wakeup in the Command field. Upon receiving the
> > > > +  Wakeup command, the secondary CPU acknowledges the command by writing Noop in
> > > > +  the Command field and jumps to the Wakeup_Vector. The operating system can
> > > > +  send the next command only after the Command field is changed to Noop.
> > > > +
> > > > +  The secondary CPU will no longer check the mailbox after waking up. The
> > > > +  secondary CPU must ignore the command if its APIC_ID written in the mailbox
> > > > +  does not match its own.
> > > > +
> > > > +  When entering the Wakeup_Vector, interrupts must be disabled and 64-bit
> > > > +  addressing mode must be enabled. Paging mode must be enabled. The virtual
> > > > +  address of the Wakeup_Vector page must be equal to its physical address.
> > > > +  Segment selectors are not used.
> > >
> > > This interface is defined in the ACPI specification and all of the
> > > above information is present there.
> > >
> > > Why are you copying it without acknowledging the source of it instead
> > > of just saying where this interface is defined and pointing to its
> > > definition?
> >
> > There was a discussion in the past about preferring a full description of
> > the mailbox instead of references to ACPI [1]. I am happy to acknowledge
> > the source in the changeset description. I explicitly acknowledge the ACPI
> > specification in the cover letter.
> >
> > [1]. https://lore.kernel.org/all/20240809232928.GB25056@yjiang5-mobl.amr.corp.intel.com/
> 
> In which I clearly was not involved.
> 
> Acknowledgement in the cover letter is fine, but insufficient.

I see.

> 
> Also, there is the question regarding what happens when the ASWG
> decides to update this part of the ACPI specification.  Is the DT
> binding going to be updated too?

That was my plan. The ACPI Multiprocessor Wakeup Structure has a version
field that I could have used.

But, it looks like these bindings will not be needed after all.

I will drop this patch.

Thanks and BR,
Ricardo

Re: [PATCH v3 04/13] dt-bindings: x86: Add CPU bindings for x86

[Ricardo Neri]

On Tue, May 06, 2025 at 09:25:59AM +0200, Krzysztof Kozlowski wrote:
> On Mon, May 05, 2025 at 09:52:35PM GMT, Ricardo Neri wrote:
> > On Sun, May 04, 2025 at 06:45:59PM +0200, Krzysztof Kozlowski wrote:
> > > On Sat, May 03, 2025 at 12:15:06PM GMT, Ricardo Neri wrote:
> > > > Add bindings for CPUs in x86 architecture. Start by defining the `reg` and
> > > 
> > > What for?
> > 
> > Thank you for your quick feedback, Krzysztof!
> > 
> > Do you mean for what reason I want to start bindings for x86 CPUs? Or only
> 
> Yes. For which devices, what purpose.

Sure, I could expand on this.

> 
> > the `reg` property? If the former, it is to add an enable-method property to
> > x86 CPUs. If the latter, is to show the relationship between APIC and `reg`.
> > 
> > > 
> > > > `enable-method` properties and their relationship to x86 APIC ID and the
> > > > available mechanisms to boot secondary CPUs.
> > > > 
> > > > Start defining bindings for Intel processors. Bindings for other vendors
> > > > can be added later as needed.
> > > > 
> > > > Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> > > > ---
> > > 
> > > Not really tested so only limited review follows.
> > 
> > Sorry, I ran make dt_binding_check but only on this schema. I missed the
> > reported error.
> > 
> > > 
> > > >  .../devicetree/bindings/x86/cpus.yaml         | 80 +++++++++++++++++++
> > > >  1 file changed, 80 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/x86/cpus.yaml
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/x86/cpus.yaml b/Documentation/devicetree/bindings/x86/cpus.yaml
> > > > new file mode 100644
> > > > index 000000000000..108b3ad64aea
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/x86/cpus.yaml
> > > > @@ -0,0 +1,80 @@
> > > > +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/x86/cpus.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: x86 CPUs
> > > > +
> > > > +maintainers:
> > > > +  - Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> > > > +
> > > > +description: |
> > > > +  Description of x86 CPUs in a system through the "cpus" node.
> > > > +
> > > > +  Detailed information about the CPU architecture can be found in the Intel
> > > > +  Software Developer's Manual:
> > > > +    https://intel.com/sdm
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    enum:
> > > > +      - intel,x86
> > > 
> > > That's architecture, not a CPU. CPUs are like 80286, 80386, so that's
> > > not even specific instruction set. I don't get what you need it for.
> > 
> > Am I to understand the the `compatible` property is not needed if the
> > bindings apply to any x86 CPU?
> 
> Every device needs compatible. Its meaning is explained:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#compatible
> 
> If you add here a device representing CPU, then look at existing
> bindings for CPUs how they do it.
> 
> It again feels like you add DT for platform which is not a real thing.

That is correct. I struggle to enumerate specific CPUs because the `intel,
wakeup-mailbox` enable method is implemented in the platform firmware and
is not tied to a given processor model as required by the rules of the
`compatible` property.

> If you use DT, you do not get different rules, therefore read all
> standard guides and tutorials (there were many, quite comprehensive).

I went through various materials. Perhaps I needed to understand the rules
better.

I realize now the DeviceTree is about describing hardware not firmware and
DT bindings are a suitable vehicle for this.

Thanks for the time you spent reviewing this patchset!

BR,
Ricardo

Re: [PATCH v3 06/13] dt-bindings: reserved-memory: Wakeup Mailbox for Intel processors

[Rob Herring]

On Tue, May 06, 2025 at 08:23:39PM -0700, Ricardo Neri wrote:
> On Tue, May 06, 2025 at 09:10:22AM +0200, Krzysztof Kozlowski wrote:
> > On Mon, May 05, 2025 at 10:16:10PM GMT, Ricardo Neri wrote:
> > > > If this is a device, then compatibles specific to devices. You do not
> > > > get different rules than all other bindings... or this does not have to
> > > > be binding at all. Why standard reserved-memory does not work for here?
> > > > 
> > > > Why do you need compatible in the first place?
> > > 
> > > Are you suggesting something like this?
> > > 
> > > reserved-memory {
> > > 	# address-cells = <2>;
> > > 	# size-cells = <1>;
> > > 
> > > 	wakeup_mailbox: wakeupmb@fff000 {
> > > 		reg = < 0x0 0xfff000 0x1000>
> > > 	}
> > > 
> > > and then reference to the reserved memory using the wakeup_mailbox
> > > phandle?
> > 
> > Yes just like every other, typical reserved memory block.
> 
> Thanks! I will take this approach and drop this patch.

If there is nothing else to this other than the reserved region, then 
don't do this. Keep it like you had. There's no need for 2 nodes.

Rob

Re: [PATCH v3 05/13] x86/dt: Parse the `enable-method` property of CPU nodes

[Rob Herring]

On Sat, May 03, 2025 at 12:15:07PM -0700, Ricardo Neri wrote:
> Add functionality to parse and validate the `enable-method` property for
> platforms that use alternative methods to wakeup secondary CPUs (e.g., a
> wakeup mailbox).
> 
> Most x86 platforms boot secondary CPUs using INIT assert, de-assert
> followed by a Start-Up IPI messages. These systems do no need to specify an
> `enable-method` property in the cpu@N nodes of the DeviceTree.
> 
> Although it is possible to specify a different `enable-method` for each
> secondary CPU, the existing functionality relies on using the
> APIC wakeup_secondary_cpu{ (), _64()} callback to wake up all CPUs. Ensure
> that either all CPUs specify the same `enable-method` or none at all.
> 
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> ---
> Changes since v2:
>  - Introduced this patch.
> 
> Changes since v1:
>  - N/A
> ---
>  arch/x86/kernel/devicetree.c | 88 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 86 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/devicetree.c b/arch/x86/kernel/devicetree.c
> index dd8748c45529..5835afc74acd 100644
> --- a/arch/x86/kernel/devicetree.c
> +++ b/arch/x86/kernel/devicetree.c
> @@ -127,8 +127,59 @@ static void __init dtb_setup_hpet(void)
>  
>  #ifdef CONFIG_X86_LOCAL_APIC
>  
> +#ifdef CONFIG_SMP
> +static const char *dtb_supported_enable_methods[] __initconst = { };

If you expect this list to grow, I would say the firmware should support 
"spin-table" enable-method and let's stop the list before it starts. 
Look at the mess that's arm32 enable-methods... Considering you have no 
interrupts, I imagine what you have is not much different from a 
spin-table (write a jump address to an address)? Maybe it would already 
work as long as jump table is reserved (And in that case you don't need 
the compatible or any binding other than for cpu nodes).

OTOH, as the wakeup-mailbox seems to be defined by ACPI, that seems 
fine to add, but I would simplify the code here to not invite more 
entries. Further ones should be rejected IMO.

> +
> +static bool __init dtb_enable_method_is_valid(const char *enable_method_a,
> +					      const char *enable_method_b)
> +{
> +	int i;
> +
> +	if (!enable_method_a && !enable_method_b)
> +		return true;
> +
> +	if (strcmp(enable_method_a, enable_method_b))
> +		return false;
> +
> +	for (i = 0; i < ARRAY_SIZE(dtb_supported_enable_methods); i++) {
> +		if (!strcmp(enable_method_a, dtb_supported_enable_methods[i]))
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +static int __init dtb_configure_enable_method(const char *enable_method)
> +{
> +	/* Nothing to do for a missing enable-method or if the system has one CPU */
> +	if (!enable_method || IS_ERR(enable_method))
> +		return 0;
> +
> +	return -ENOTSUPP;
> +}
> +#else /* !CONFIG_SMP */
> +static inline bool dtb_enable_method_is_valid(const char *enable_method_a,
> +					      const char *enable_method_b)
> +{
> +	/* No secondary CPUs. We do not care about the enable-method. */
> +	return true;
> +}
> +
> +static inline int dtb_configure_enable_method(const char *enable_method)
> +{
> +	return 0;
> +}
> +#endif /* CONFIG_SMP */
> +
> +static void __init dtb_register_apic_id(u32 apic_id, struct device_node *dn)
> +{
> +	topology_register_apic(apic_id, CPU_ACPIID_INVALID, true);
> +	set_apicid_to_node(apic_id, of_node_to_nid(dn));
> +}
> +
>  static void __init dtb_cpu_setup(void)
>  {
> +	const char *enable_method = ERR_PTR(-EINVAL), *this_em;
>  	struct device_node *dn;
>  	u32 apic_id;
>  
> @@ -138,9 +189,42 @@ static void __init dtb_cpu_setup(void)
>  			pr_warn("%pOF: missing local APIC ID\n", dn);
>  			continue;
>  		}
> -		topology_register_apic(apic_id, CPU_ACPIID_INVALID, true);
> -		set_apicid_to_node(apic_id, of_node_to_nid(dn));
> +
> +		/*
> +		 * Also check the enable-method of the secondary CPUs, if present.
> +		 *
> +		 * Systems that use the INIT-!INIT-StartUp IPI sequence to boot
> +		 * secondary CPUs do not need to define an enable-method.
> +		 *
> +		 * All CPUs must have the same enable-method. The enable-method
> +		 * must be supported. If absent in one secondary CPU, it must be
> +		 * absent for all CPUs.
> +		 *
> +		 * Compare the first secondary CPU with the rest. We do not care
> +		 * about the boot CPU, as it is enabled already.
> +		 */
> +
> +		if (apic_id == boot_cpu_physical_apicid) {
> +			dtb_register_apic_id(apic_id, dn);
> +			continue;
> +		}
> +
> +		this_em = of_get_property(dn, "enable-method", NULL);

Use typed accessors. of_property_match_string() would be good here. 
There's some desire to avoid more of_property_read_string() calls too 
because that leaks un-refcounted DT data to the caller (really only an 
issue in overlays).

> +
> +		if (IS_ERR(enable_method)) {
> +			enable_method = this_em;
> +			dtb_register_apic_id(apic_id, dn);
> +			continue;
> +		}
> +
> +		if (!dtb_enable_method_is_valid(enable_method, this_em))
> +			continue;
> +
> +		dtb_register_apic_id(apic_id, dn);
>  	}
> +
> +	if (dtb_configure_enable_method(enable_method))
> +		pr_err("enable-method '%s' needed but not configured\n", enable_method);
>  }
>  
>  static void __init dtb_lapic_setup(void)
> -- 
> 2.43.0
> 

Re: [PATCH v3 06/13] dt-bindings: reserved-memory: Wakeup Mailbox for Intel processors

[Ricardo Neri]

On Mon, May 12, 2025 at 10:32:24AM -0500, Rob Herring wrote:
> On Tue, May 06, 2025 at 08:23:39PM -0700, Ricardo Neri wrote:
> > On Tue, May 06, 2025 at 09:10:22AM +0200, Krzysztof Kozlowski wrote:
> > > On Mon, May 05, 2025 at 10:16:10PM GMT, Ricardo Neri wrote:
> > > > > If this is a device, then compatibles specific to devices. You do not
> > > > > get different rules than all other bindings... or this does not have to
> > > > > be binding at all. Why standard reserved-memory does not work for here?
> > > > > 
> > > > > Why do you need compatible in the first place?
> > > > 
> > > > Are you suggesting something like this?
> > > > 
> > > > reserved-memory {
> > > > 	# address-cells = <2>;
> > > > 	# size-cells = <1>;
> > > > 
> > > > 	wakeup_mailbox: wakeupmb@fff000 {
> > > > 		reg = < 0x0 0xfff000 0x1000>
> > > > 	}
> > > > 
> > > > and then reference to the reserved memory using the wakeup_mailbox
> > > > phandle?
> > > 
> > > Yes just like every other, typical reserved memory block.
> > 
> > Thanks! I will take this approach and drop this patch.
> 
> If there is nothing else to this other than the reserved region, then 
> don't do this. Keep it like you had. There's no need for 2 nodes.

Thank you for your feedback!

I was planning to use one reserved-memory node and inside of it a child
node to with a `reg` property to specify the location and size of the
mailbox. I would reference to that subnode from the kernel code.

IIUC, the reserved-memory node is only the container and the actual memory
regions are expressed as child nodes.

I had it like that before, but with a `compatible` property that I did not
need.

Am I missing anything?

Thanks and BR,
Ricardo

Re: [PATCH v3 05/13] x86/dt: Parse the `enable-method` property of CPU nodes

[Ricardo Neri]

On Mon, May 12, 2025 at 10:54:15AM -0500, Rob Herring wrote:
> On Sat, May 03, 2025 at 12:15:07PM -0700, Ricardo Neri wrote:
> > Add functionality to parse and validate the `enable-method` property for
> > platforms that use alternative methods to wakeup secondary CPUs (e.g., a
> > wakeup mailbox).
> > 
> > Most x86 platforms boot secondary CPUs using INIT assert, de-assert
> > followed by a Start-Up IPI messages. These systems do no need to specify an
> > `enable-method` property in the cpu@N nodes of the DeviceTree.
> > 
> > Although it is possible to specify a different `enable-method` for each
> > secondary CPU, the existing functionality relies on using the
> > APIC wakeup_secondary_cpu{ (), _64()} callback to wake up all CPUs. Ensure
> > that either all CPUs specify the same `enable-method` or none at all.
> > 
> > Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> > ---
> > Changes since v2:
> >  - Introduced this patch.
> > 
> > Changes since v1:
> >  - N/A
> > ---
> >  arch/x86/kernel/devicetree.c | 88 +++++++++++++++++++++++++++++++++++-
> >  1 file changed, 86 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/devicetree.c b/arch/x86/kernel/devicetree.c
> > index dd8748c45529..5835afc74acd 100644
> > --- a/arch/x86/kernel/devicetree.c
> > +++ b/arch/x86/kernel/devicetree.c
> > @@ -127,8 +127,59 @@ static void __init dtb_setup_hpet(void)
> >  
> >  #ifdef CONFIG_X86_LOCAL_APIC
> >  
> > +#ifdef CONFIG_SMP
> > +static const char *dtb_supported_enable_methods[] __initconst = { };
> 
> If you expect this list to grow, I would say the firmware should support 
> "spin-table" enable-method and let's stop the list before it starts. 

Actually, I was thinking on dropping this patch altogether. It does not
seem to be needed: if there is a reserved-memory region for the mailbox,
use it. Otherwise, keep using the INIT-!INIT-SIPI messages. No need to
add extra complexity and maintainance burden with checks for an `enable-
method`.

> Look at the mess that's arm32 enable-methods... Considering you have no 
> interrupts, I imagine what you have is not much different from a 
> spin-table (write a jump address to an address)? Maybe it would already 
> work as long as jump table is reserved (And in that case you don't need 
> the compatible or any binding other than for cpu nodes).

Correct, the spin-table is similar to the ACPI mailbox but there are
differences: the latter lets you send a command to control when, if ever,
secondary CPUs are booted.

> 
> OTOH, as the wakeup-mailbox seems to be defined by ACPI, that seems 
> fine to add,

Yes, and Linux for x86 already supports the ACPI mailbox and that code can
be reused.

> but I would simplify the code here to not invite more 
> entries. Further ones should be rejected IMO.

Unconditionally checking for the presence of mailbox works in this sense
too.

> 
> > +
> > +static bool __init dtb_enable_method_is_valid(const char *enable_method_a,
> > +					      const char *enable_method_b)
> > +{
> > +	int i;
> > +
> > +	if (!enable_method_a && !enable_method_b)
> > +		return true;
> > +
> > +	if (strcmp(enable_method_a, enable_method_b))
> > +		return false;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(dtb_supported_enable_methods); i++) {
> > +		if (!strcmp(enable_method_a, dtb_supported_enable_methods[i]))
> > +			return true;
> > +	}
> > +
> > +	return false;
> > +}
> > +
> > +static int __init dtb_configure_enable_method(const char *enable_method)
> > +{
> > +	/* Nothing to do for a missing enable-method or if the system has one CPU */
> > +	if (!enable_method || IS_ERR(enable_method))
> > +		return 0;
> > +
> > +	return -ENOTSUPP;
> > +}
> > +#else /* !CONFIG_SMP */
> > +static inline bool dtb_enable_method_is_valid(const char *enable_method_a,
> > +					      const char *enable_method_b)
> > +{
> > +	/* No secondary CPUs. We do not care about the enable-method. */
> > +	return true;
> > +}
> > +
> > +static inline int dtb_configure_enable_method(const char *enable_method)
> > +{
> > +	return 0;
> > +}
> > +#endif /* CONFIG_SMP */
> > +
> > +static void __init dtb_register_apic_id(u32 apic_id, struct device_node *dn)
> > +{
> > +	topology_register_apic(apic_id, CPU_ACPIID_INVALID, true);
> > +	set_apicid_to_node(apic_id, of_node_to_nid(dn));
> > +}
> > +
> >  static void __init dtb_cpu_setup(void)
> >  {
> > +	const char *enable_method = ERR_PTR(-EINVAL), *this_em;
> >  	struct device_node *dn;
> >  	u32 apic_id;
> >  
> > @@ -138,9 +189,42 @@ static void __init dtb_cpu_setup(void)
> >  			pr_warn("%pOF: missing local APIC ID\n", dn);
> >  			continue;
> >  		}
> > -		topology_register_apic(apic_id, CPU_ACPIID_INVALID, true);
> > -		set_apicid_to_node(apic_id, of_node_to_nid(dn));
> > +
> > +		/*
> > +		 * Also check the enable-method of the secondary CPUs, if present.
> > +		 *
> > +		 * Systems that use the INIT-!INIT-StartUp IPI sequence to boot
> > +		 * secondary CPUs do not need to define an enable-method.
> > +		 *
> > +		 * All CPUs must have the same enable-method. The enable-method
> > +		 * must be supported. If absent in one secondary CPU, it must be
> > +		 * absent for all CPUs.
> > +		 *
> > +		 * Compare the first secondary CPU with the rest. We do not care
> > +		 * about the boot CPU, as it is enabled already.
> > +		 */
> > +
> > +		if (apic_id == boot_cpu_physical_apicid) {
> > +			dtb_register_apic_id(apic_id, dn);
> > +			continue;
> > +		}
> > +
> > +		this_em = of_get_property(dn, "enable-method", NULL);
> 
> Use typed accessors. of_property_match_string() would be good here. 
> There's some desire to avoid more of_property_read_string() calls too 
> because that leaks un-refcounted DT data to the caller (really only an 
> issue in overlays).

Thanks for this information! However, I plan to scrap this code and
unconditionally use the mailbox if detected.

I would still like to get your inputs on the next submission with updated
code to use the mailbox if you agree.

BR,
Ricardo

Re: [PATCH v3 06/13] dt-bindings: reserved-memory: Wakeup Mailbox for Intel processors

[Rob Herring]

On Tue, May 13, 2025 at 03:14:56PM -0700, Ricardo Neri wrote:
> On Mon, May 12, 2025 at 10:32:24AM -0500, Rob Herring wrote:
> > On Tue, May 06, 2025 at 08:23:39PM -0700, Ricardo Neri wrote:
> > > On Tue, May 06, 2025 at 09:10:22AM +0200, Krzysztof Kozlowski wrote:
> > > > On Mon, May 05, 2025 at 10:16:10PM GMT, Ricardo Neri wrote:
> > > > > > If this is a device, then compatibles specific to devices. You do not
> > > > > > get different rules than all other bindings... or this does not have to
> > > > > > be binding at all. Why standard reserved-memory does not work for here?
> > > > > > 
> > > > > > Why do you need compatible in the first place?
> > > > > 
> > > > > Are you suggesting something like this?
> > > > > 
> > > > > reserved-memory {
> > > > > 	# address-cells = <2>;
> > > > > 	# size-cells = <1>;
> > > > > 
> > > > > 	wakeup_mailbox: wakeupmb@fff000 {
> > > > > 		reg = < 0x0 0xfff000 0x1000>
> > > > > 	}
> > > > > 
> > > > > and then reference to the reserved memory using the wakeup_mailbox
> > > > > phandle?
> > > > 
> > > > Yes just like every other, typical reserved memory block.
> > > 
> > > Thanks! I will take this approach and drop this patch.
> > 
> > If there is nothing else to this other than the reserved region, then 
> > don't do this. Keep it like you had. There's no need for 2 nodes.
> 
> Thank you for your feedback!
> 
> I was planning to use one reserved-memory node and inside of it a child
> node to with a `reg` property to specify the location and size of the
> mailbox. I would reference to that subnode from the kernel code.
> 
> IIUC, the reserved-memory node is only the container and the actual memory
> regions are expressed as child nodes.
> 
> I had it like that before, but with a `compatible` property that I did not
> need.
> 
> Am I missing anything?

Without a compatible, how do you identify which reserved region is the 
wakeup mailbox? Before you say node name, those are supposed to be 
generic though we failed to enforce anything for /reserved-memory child 
nodes.

Rob

Re: [PATCH v3 06/13] dt-bindings: reserved-memory: Wakeup Mailbox for Intel processors

[Ricardo Neri]

On Wed, May 14, 2025 at 10:42:48AM -0500, Rob Herring wrote:
> On Tue, May 13, 2025 at 03:14:56PM -0700, Ricardo Neri wrote:
> > On Mon, May 12, 2025 at 10:32:24AM -0500, Rob Herring wrote:
> > > On Tue, May 06, 2025 at 08:23:39PM -0700, Ricardo Neri wrote:
> > > > On Tue, May 06, 2025 at 09:10:22AM +0200, Krzysztof Kozlowski wrote:
> > > > > On Mon, May 05, 2025 at 10:16:10PM GMT, Ricardo Neri wrote:
> > > > > > > If this is a device, then compatibles specific to devices. You do not
> > > > > > > get different rules than all other bindings... or this does not have to
> > > > > > > be binding at all. Why standard reserved-memory does not work for here?
> > > > > > > 
> > > > > > > Why do you need compatible in the first place?
> > > > > > 
> > > > > > Are you suggesting something like this?
> > > > > > 
> > > > > > reserved-memory {
> > > > > > 	# address-cells = <2>;
> > > > > > 	# size-cells = <1>;
> > > > > > 
> > > > > > 	wakeup_mailbox: wakeupmb@fff000 {
> > > > > > 		reg = < 0x0 0xfff000 0x1000>
> > > > > > 	}
> > > > > > 
> > > > > > and then reference to the reserved memory using the wakeup_mailbox
> > > > > > phandle?
> > > > > 
> > > > > Yes just like every other, typical reserved memory block.
> > > > 
> > > > Thanks! I will take this approach and drop this patch.
> > > 
> > > If there is nothing else to this other than the reserved region, then 
> > > don't do this. Keep it like you had. There's no need for 2 nodes.
> > 
> > Thank you for your feedback!
> > 
> > I was planning to use one reserved-memory node and inside of it a child
> > node to with a `reg` property to specify the location and size of the
> > mailbox. I would reference to that subnode from the kernel code.
> > 
> > IIUC, the reserved-memory node is only the container and the actual memory
> > regions are expressed as child nodes.
> > 
> > I had it like that before, but with a `compatible` property that I did not
> > need.
> > 
> > Am I missing anything?
> 
> Without a compatible, how do you identify which reserved region is the 
> wakeup mailbox?

I thought using a phandle to the wakeup_mailbox. Then I realized that the
device nodes using the mailbox would be CPUs. They would need a `memory-
region` property. This does not look right to me.

> Before you say node name, those are supposed to be 
> generic though we failed to enforce anything for /reserved-memory child 
> nodes.

I see. Thanks for preventing me from doing this.

Then the `compatible` property seems the way to go after all.

This what motivated this patch in the first place. On further analysis,
IIUC, defining bindings and schema is not needed, IMO, since the mailbox
is already defined in the ACPI spec. No need to redefine.

Ricardo

Re: [PATCH v3 06/13] dt-bindings: reserved-memory: Wakeup Mailbox for Intel processors

[Rob Herring]

On Wed, May 14, 2025 at 08:53:38PM -0700, Ricardo Neri wrote:
> On Wed, May 14, 2025 at 10:42:48AM -0500, Rob Herring wrote:
> > On Tue, May 13, 2025 at 03:14:56PM -0700, Ricardo Neri wrote:
> > > On Mon, May 12, 2025 at 10:32:24AM -0500, Rob Herring wrote:
> > > > On Tue, May 06, 2025 at 08:23:39PM -0700, Ricardo Neri wrote:
> > > > > On Tue, May 06, 2025 at 09:10:22AM +0200, Krzysztof Kozlowski wrote:
> > > > > > On Mon, May 05, 2025 at 10:16:10PM GMT, Ricardo Neri wrote:
> > > > > > > > If this is a device, then compatibles specific to devices. You do not
> > > > > > > > get different rules than all other bindings... or this does not have to
> > > > > > > > be binding at all. Why standard reserved-memory does not work for here?
> > > > > > > > 
> > > > > > > > Why do you need compatible in the first place?
> > > > > > > 
> > > > > > > Are you suggesting something like this?
> > > > > > > 
> > > > > > > reserved-memory {
> > > > > > > 	# address-cells = <2>;
> > > > > > > 	# size-cells = <1>;
> > > > > > > 
> > > > > > > 	wakeup_mailbox: wakeupmb@fff000 {
> > > > > > > 		reg = < 0x0 0xfff000 0x1000>
> > > > > > > 	}
> > > > > > > 
> > > > > > > and then reference to the reserved memory using the wakeup_mailbox
> > > > > > > phandle?
> > > > > > 
> > > > > > Yes just like every other, typical reserved memory block.
> > > > > 
> > > > > Thanks! I will take this approach and drop this patch.
> > > > 
> > > > If there is nothing else to this other than the reserved region, then 
> > > > don't do this. Keep it like you had. There's no need for 2 nodes.
> > > 
> > > Thank you for your feedback!
> > > 
> > > I was planning to use one reserved-memory node and inside of it a child
> > > node to with a `reg` property to specify the location and size of the
> > > mailbox. I would reference to that subnode from the kernel code.
> > > 
> > > IIUC, the reserved-memory node is only the container and the actual memory
> > > regions are expressed as child nodes.
> > > 
> > > I had it like that before, but with a `compatible` property that I did not
> > > need.
> > > 
> > > Am I missing anything?
> > 
> > Without a compatible, how do you identify which reserved region is the 
> > wakeup mailbox?
> 
> I thought using a phandle to the wakeup_mailbox. Then I realized that the
> device nodes using the mailbox would be CPUs. They would need a `memory-
> region` property. This does not look right to me.

That doesn't really make sense unless it's a memory region per CPU.


> > Before you say node name, those are supposed to be 
> > generic though we failed to enforce anything for /reserved-memory child 
> > nodes.
> 
> I see. Thanks for preventing me from doing this.
> 
> Then the `compatible` property seems the way to go after all.
> 
> This what motivated this patch in the first place. On further analysis,
> IIUC, defining bindings and schema is not needed, IMO, since the mailbox
> is already defined in the ACPI spec. No need to redefine.

You lost me...

You don't need to redefine the layout of the memory region as that's 
defined already somewhere, but you do need to define where it is for DT. 
And for that, you need a compatible. Do you know where it is in this 
case? 

Rob

Re: [PATCH v3 06/13] dt-bindings: reserved-memory: Wakeup Mailbox for Intel processors

[Ricardo Neri]

On Mon, May 19, 2025 at 10:29:37AM -0500, Rob Herring wrote:
> On Wed, May 14, 2025 at 08:53:38PM -0700, Ricardo Neri wrote:
> > On Wed, May 14, 2025 at 10:42:48AM -0500, Rob Herring wrote:
> > > On Tue, May 13, 2025 at 03:14:56PM -0700, Ricardo Neri wrote:
> > > > On Mon, May 12, 2025 at 10:32:24AM -0500, Rob Herring wrote:
> > > > > On Tue, May 06, 2025 at 08:23:39PM -0700, Ricardo Neri wrote:
> > > > > > On Tue, May 06, 2025 at 09:10:22AM +0200, Krzysztof Kozlowski wrote:
> > > > > > > On Mon, May 05, 2025 at 10:16:10PM GMT, Ricardo Neri wrote:
> > > > > > > > > If this is a device, then compatibles specific to devices. You do not
> > > > > > > > > get different rules than all other bindings... or this does not have to
> > > > > > > > > be binding at all. Why standard reserved-memory does not work for here?
> > > > > > > > > 
> > > > > > > > > Why do you need compatible in the first place?
> > > > > > > > 
> > > > > > > > Are you suggesting something like this?
> > > > > > > > 
> > > > > > > > reserved-memory {
> > > > > > > > 	# address-cells = <2>;
> > > > > > > > 	# size-cells = <1>;
> > > > > > > > 
> > > > > > > > 	wakeup_mailbox: wakeupmb@fff000 {
> > > > > > > > 		reg = < 0x0 0xfff000 0x1000>
> > > > > > > > 	}
> > > > > > > > 
> > > > > > > > and then reference to the reserved memory using the wakeup_mailbox
> > > > > > > > phandle?
> > > > > > > 
> > > > > > > Yes just like every other, typical reserved memory block.
> > > > > > 
> > > > > > Thanks! I will take this approach and drop this patch.
> > > > > 
> > > > > If there is nothing else to this other than the reserved region, then 
> > > > > don't do this. Keep it like you had. There's no need for 2 nodes.
> > > > 
> > > > Thank you for your feedback!
> > > > 
> > > > I was planning to use one reserved-memory node and inside of it a child
> > > > node to with a `reg` property to specify the location and size of the
> > > > mailbox. I would reference to that subnode from the kernel code.
> > > > 
> > > > IIUC, the reserved-memory node is only the container and the actual memory
> > > > regions are expressed as child nodes.
> > > > 
> > > > I had it like that before, but with a `compatible` property that I did not
> > > > need.
> > > > 
> > > > Am I missing anything?
> > > 
> > > Without a compatible, how do you identify which reserved region is the 
> > > wakeup mailbox?
> > 
> > I thought using a phandle to the wakeup_mailbox. Then I realized that the
> > device nodes using the mailbox would be CPUs. They would need a `memory-
> > region` property. This does not look right to me.
> 
> That doesn't really make sense unless it's a memory region per CPU.

Agreed.

> 
> 
> > > Before you say node name, those are supposed to be 
> > > generic though we failed to enforce anything for /reserved-memory child 
> > > nodes.
> > 
> > I see. Thanks for preventing me from doing this.
> > 
> > Then the `compatible` property seems the way to go after all.
> > 
> > This what motivated this patch in the first place. On further analysis,
> > IIUC, defining bindings and schema is not needed, IMO, since the mailbox
> > is already defined in the ACPI spec. No need to redefine.
> 
> You lost me...
> 
> You don't need to redefine the layout of the memory region as that's 
> defined already somewhere,

Great!

> but you do need to define where it is for DT. 
> And for that, you need a compatible. Do you know where it is in this 
> case?

The compatible is not defined anywhere yet. Is a DT schema needed to
document it? If yes, I am usure what to put in the description. We tried
to not redefine the mailbox and refer to the ACPI spec. That was a NAK
from Krzysztof [1].

Thanks and BR,
Ricardo

[1]. https://lore.kernel.org/r/624e1985-7dd2-4abe-a918-78cb43556967@kernel.org

RE: [PATCH v3 08/13] x86/hyperv/vtl: Set real_mode_header in hv_vtl_init_platform()

[Michael Kelley]

From: Ricardo Neri <ricardo.neri-calderon@linux.intel.com> Sent: Saturday, May 3, 2025 12:15 PM
> From: Yunhong Jiang <yunhong.jiang@linux.intel.com>
> 
> Hyper-V VTL clears x86_platform.realmode_{init(), reserve()} in
> hv_vtl_platform_init() whereas it sets real_mode_header later in
> hv_vtl_early_init(). There is no need to deal with the real mode memory
> in two places: x86_platform.realmode_init() is invoked much later via an
> early_initcall.
> 
> Set real_mode_header in hv_vtl_init_platform() to keep all code dealing
> with memory for the real mode trampoline in one place. Besides making the
> code more readable, it prepares it for a subsequent changeset in which the
> behavior needs to change to support Hyper-V VTL guests in TDX environment.
> 
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> ---
> Changes since v2:
>  - Edited the commit message for clarity.
> 
> Changes since v1:
>  - Introduced this patch.
> ---
>  arch/x86/hyperv/hv_vtl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/hyperv/hv_vtl.c b/arch/x86/hyperv/hv_vtl.c
> index 4580936dcb03..6bd183ee484f 100644
> --- a/arch/x86/hyperv/hv_vtl.c
> +++ b/arch/x86/hyperv/hv_vtl.c
> @@ -60,6 +60,7 @@ void __init hv_vtl_init_platform(void)
> 
>  	x86_platform.realmode_reserve = x86_init_noop;
>  	x86_platform.realmode_init = x86_init_noop;
> +	real_mode_header = &hv_vtl_real_mode_header;
>  	x86_init.irqs.pre_vector_init = x86_init_noop;
>  	x86_init.timers.timer_init = x86_init_noop;
>  	x86_init.resources.probe_roms = x86_init_noop;
> @@ -279,7 +280,6 @@ int __init hv_vtl_early_init(void)
>  		panic("XSAVE has to be disabled as it is not supported by this module.\n"
>  			  "Please add 'noxsave' to the kernel command line.\n");
> 
> -	real_mode_header = &hv_vtl_real_mode_header;
>  	apic_update_callback(wakeup_secondary_cpu_64, hv_vtl_wakeup_secondary_cpu);
> 
>  	return 0;
> --
> 2.43.0

Reviewed-by: Michael Kelley <mhklinux@outlook.com>

RE: [PATCH v3 09/13] x86/realmode: Make the location of the trampoline configurable

[Michael Kelley]

From: Ricardo Neri <ricardo.neri-calderon@linux.intel.com> Sent: Saturday, May 3, 2025 12:15 PM
> 
> From: Yunhong Jiang <yunhong.jiang@linux.intel.com>
> 
> x86 CPUs boot in real mode. This mode uses 20-bit memory addresses (16-bit
> registers plus 4-bit segment selectors). This implies that the trampoline
> must reside under the 1MB memory boundary.
> 
> There are platforms in which the firmware boots the secondary CPUs,
> switches them to long mode and transfers control to the kernel. An example
> of such mechanism is the ACPI Multiprocessor Wakeup Structure.
> 
> In this scenario there is no restriction to locate the trampoline under 1MB
> memory. Moreover, certain platforms (for example, Hyper-V VTL guests) may
> not have memory available for allocation under 1MB.
> 
> Add a new member to struct x86_init_resources to specify the upper bound
> for the location of the trampoline memory. Keep the default upper bound of
> 1MB to conserve the current behavior.
> 
> Originally-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> ---
> Changes since v2:
>  - Edited the commit message for clarity.
>  - Minor tweaks to comments.
>  - Removed the option to not reserve the first 1MB of memory as it is
>    not needed.
> 
> Changes since v1:
>  - Added this patch using code that Thomas suggested:
>    https://lore.kernel.org/lkml/87a5ho2q6x.ffs@tglx/
> ---
>  arch/x86/include/asm/x86_init.h | 3 +++
>  arch/x86/kernel/x86_init.c      | 3 +++
>  arch/x86/realmode/init.c        | 7 +++----
>  3 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
> index 36698cc9fb44..e770ce507a87 100644
> --- a/arch/x86/include/asm/x86_init.h
> +++ b/arch/x86/include/asm/x86_init.h
> @@ -31,12 +31,15 @@ struct x86_init_mpparse {
>   *				platform
>   * @memory_setup:		platform specific memory setup
>   * @dmi_setup:			platform specific DMI setup
> + * @realmode_limit:		platform specific address limit for the real mode trampoline
> + *				(default 1M)
>   */
>  struct x86_init_resources {
>  	void (*probe_roms)(void);
>  	void (*reserve_resources)(void);
>  	char *(*memory_setup)(void);
>  	void (*dmi_setup)(void);
> +	unsigned long realmode_limit;
>  };
> 
>  /**
> diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
> index 0a2bbd674a6d..a25fd7282811 100644
> --- a/arch/x86/kernel/x86_init.c
> +++ b/arch/x86/kernel/x86_init.c
> @@ -9,6 +9,7 @@
>  #include <linux/export.h>
>  #include <linux/pci.h>
>  #include <linux/acpi.h>
> +#include <linux/sizes.h>
> 
>  #include <asm/acpi.h>
>  #include <asm/bios_ebda.h>
> @@ -69,6 +70,8 @@ struct x86_init_ops x86_init __initdata = {
>  		.reserve_resources	= reserve_standard_io_resources,
>  		.memory_setup		= e820__memory_setup_default,
>  		.dmi_setup		= dmi_setup,
> +		/* Has to be under 1M so we can execute real-mode AP code. */
> +		.realmode_limit		= SZ_1M,
>  	},
> 
>  	.mpparse = {
> diff --git a/arch/x86/realmode/init.c b/arch/x86/realmode/init.c
> index ed5c63c0b4e5..01155f995b2b 100644
> --- a/arch/x86/realmode/init.c
> +++ b/arch/x86/realmode/init.c
> @@ -46,7 +46,7 @@ void load_trampoline_pgtable(void)
> 
>  void __init reserve_real_mode(void)
>  {
> -	phys_addr_t mem;
> +	phys_addr_t mem, limit = x86_init.resources.realmode_limit;
>  	size_t size = real_mode_size_needed();
> 
>  	if (!size)
> @@ -54,10 +54,9 @@ void __init reserve_real_mode(void)
> 
>  	WARN_ON(slab_is_available());
> 
> -	/* Has to be under 1M so we can execute real-mode AP code. */
> -	mem = memblock_phys_alloc_range(size, PAGE_SIZE, 0, 1<<20);
> +	mem = memblock_phys_alloc_range(size, PAGE_SIZE, 0, limit);
>  	if (!mem)
> -		pr_info("No sub-1M memory is available for the trampoline\n");
> +		pr_info("No memory below %pa for the real-mode trampoline\n", &limit);
>  	else
>  		set_real_mode_mem(mem);
> 
> --
> 2.43.0

Reviewed-by: Michael Kelley <mhklinux@outlook.com>

RE: [PATCH v3 10/13] x86/hyperv/vtl: Setup the 64-bit trampoline for TDX guests

[Michael Kelley]

From: Ricardo Neri <ricardo.neri-calderon@linux.intel.com> Sent: Saturday, May 3, 2025 12:15 PM

> 
> From: Yunhong Jiang <yunhong.jiang@linux.intel.com>
> 
> The hypervisor is an untrusted entity for TDX guests. It cannot be used
> to boot secondary CPUs - neither via hypercalls not the INIT assert,
> de-assert plus Start-Up IPI messages.
> 
> Instead, the platform virtual firmware boots the secondary CPUs and
> puts them in a state to transfer control to the kernel. This mechanism uses
> the wakeup mailbox described in the Multiprocessor Wakeup Structure of the
> ACPI specification. The entry point to the kernel is trampoline_start64.
> 
> Allocate and setup the trampoline using the default x86_platform callbacks.
> 
> The platform firmware configures the secondary CPUs in long mode. It is no
> longer necessary to locate the trampoline under 1MB memory. After handoff
> from firmware, the trampoline code switches briefly to 32-bit addressing
> mode, which has an addressing limit of 4GB. Set the upper bound of the
> trampoline memory accordingly.
> 
> Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> ---
> Changes since v2:
>  - Added a note regarding there is no need to check for a present
>    paravisor.
>  - Edited commit message for clarity.
> 
> Changes since v1:
>  - Dropped the function hv_reserve_real_mode(). Instead, used the new
>    members realmode_limit and reserve_bios members of x86_init to
>    set the upper bound of the trampoline memory. (Thomas)
> ---
>  arch/x86/hyperv/hv_vtl.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/hyperv/hv_vtl.c b/arch/x86/hyperv/hv_vtl.c
> index 6bd183ee484f..8b497c8292d3 100644
> --- a/arch/x86/hyperv/hv_vtl.c
> +++ b/arch/x86/hyperv/hv_vtl.c
> @@ -58,9 +58,14 @@ void __init hv_vtl_init_platform(void)
>  {
>  	pr_info("Linux runs in Hyper-V Virtual Trust Level\n");
> 
> -	x86_platform.realmode_reserve = x86_init_noop;
> -	x86_platform.realmode_init = x86_init_noop;
> -	real_mode_header = &hv_vtl_real_mode_header;
> +	/* There is no paravisor present if we are here. */
> +	if (hv_isolation_type_tdx()) {
> +		x86_init.resources.realmode_limit = SZ_4G;
> +	} else {
> +		x86_platform.realmode_reserve = x86_init_noop;
> +		x86_platform.realmode_init = x86_init_noop;
> +		real_mode_header = &hv_vtl_real_mode_header;
> +	}
>  	x86_init.irqs.pre_vector_init = x86_init_noop;
>  	x86_init.timers.timer_init = x86_init_noop;
>  	x86_init.resources.probe_roms = x86_init_noop;
> --
> 2.43.0

Reviewed-by: Michael Kelley <mhklinux@outlook.com>

RE: [PATCH v3 11/13] x86/smpboot: Add a helper get the address of the wakeup mailbox

[Michael Kelley]

From: Ricardo Neri <ricardo.neri-calderon@linux.intel.com> Sent: Saturday, May 3, 2025 12:15 PM
> 
> A Hyper-V VTL level 2 guest on a TDX environment needs to map the
> physical page of the ACPI Multiprocessor Wakeup Structure as private
> (encrypted). It needs to know the physical address of this structure.
> Add a helper function.
> 
> Suggested-by: Michael Kelley <mhklinux@outlook.com>
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> ---
> Changes since v2:
>  - Introduced this patch
> 
> Changes since v1:
>  - N/A
> ---
>  arch/x86/include/asm/smp.h | 1 +
>  arch/x86/kernel/smpboot.c  | 5 +++++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
> index 97bfbd0d24d4..18003453569a 100644
> --- a/arch/x86/include/asm/smp.h
> +++ b/arch/x86/include/asm/smp.h
> @@ -149,6 +149,7 @@ static inline struct cpumask *cpu_l2c_shared_mask(int cpu)
>  #ifdef CONFIG_X86_64
>  void setup_mp_wakeup_mailbox(u64 addr);
>  struct acpi_madt_multiproc_wakeup_mailbox *get_mp_wakeup_mailbox(void);
> +u64 get_mp_wakeup_mailbox_paddr(void);
>  #endif
> 
>  #else /* !CONFIG_SMP */
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 6f39ebe4d192..1e211c78c1d3 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1431,4 +1431,9 @@ struct acpi_madt_multiproc_wakeup_mailbox
> *get_mp_wakeup_mailbox(void)
>  {
>  	return acpi_mp_wake_mailbox;
>  }
> +
> +u64 get_mp_wakeup_mailbox_paddr(void)
> +{
> +	return acpi_mp_wake_mailbox_paddr;
> +}
>  #endif
> --
> 2.43.0

Reviewed-by: Michael Kelley <mhklinux@outlook.com>

RE: [PATCH v3 12/13] x86/hyperv/vtl: Mark the wakeup mailbox page as private

[Michael Kelley]

From: Ricardo Neri <ricardo.neri-calderon@linux.intel.com> Sent: Saturday, May 3, 2025 12:15 PM
> 
> From: Yunhong Jiang <yunhong.jiang@linux.intel.com>
> 
> The current code maps MMIO devices as shared (decrypted) by default in a
> confidential computing VM.
> 
> In a TDX environment, secondary CPUs are booted using the Multiprocessor
> Wakeup Structure defined in the ACPI specification. The virtual firmware
> and the operating system function in the guest context, without
> intervention from the VMM. Map the physical memory of the mailbox as
> private. Use the is_private_mmio() callback.
> 
> Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> ---
> Changes since v2:
>  - Use the new helper function get_mp_wakeup_mailbox_paddr().
>  - Edited the commit message for clarity.
> 
> Changes since v1:
>  - Added the helper function within_page() to improve readability
>  - Override the is_private_mmio() callback when detecting a TDX
>    environment. The address of the mailbox is checked in
>    hv_is_private_mmio_tdx().
> ---
>  arch/x86/hyperv/hv_vtl.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/arch/x86/hyperv/hv_vtl.c b/arch/x86/hyperv/hv_vtl.c
> index 8b497c8292d3..cd48bedd21f0 100644
> --- a/arch/x86/hyperv/hv_vtl.c
> +++ b/arch/x86/hyperv/hv_vtl.c
> @@ -54,6 +54,18 @@ static void  __noreturn hv_vtl_restart(char __maybe_unused *cmd)
>  	hv_vtl_emergency_restart();
>  }
> 
> +static inline bool within_page(u64 addr, u64 start)
> +{
> +	return addr >= start && addr < (start + PAGE_SIZE);
> +}
> +
> +static bool hv_vtl_is_private_mmio_tdx(u64 addr)
> +{
> +	u64 mb_addr = get_mp_wakeup_mailbox_paddr();
> +
> +	return mb_addr && within_page(addr, mb_addr);
> +}
> +
>  void __init hv_vtl_init_platform(void)
>  {
>  	pr_info("Linux runs in Hyper-V Virtual Trust Level\n");
> @@ -61,6 +73,8 @@ void __init hv_vtl_init_platform(void)
>  	/* There is no paravisor present if we are here. */
>  	if (hv_isolation_type_tdx()) {
>  		x86_init.resources.realmode_limit = SZ_4G;
> +		x86_platform.hyper.is_private_mmio = hv_vtl_is_private_mmio_tdx;
> +
>  	} else {
>  		x86_platform.realmode_reserve = x86_init_noop;
>  		x86_platform.realmode_init = x86_init_noop;
> --
> 2.43.0

Reviewed-by: Michael Kelley <mhklinux@outlook.com>

RE: [PATCH v3 13/13] x86/hyperv/vtl: Use the wakeup mailbox to boot secondary CPUs

[Michael Kelley]

From: Ricardo Neri <ricardo.neri-calderon@linux.intel.com> Sent: Saturday, May 3, 2025 12:15 PM
> 
> The hypervisor is an untrusted entity for TDX guests. It cannot be used
> to boot secondary CPUs. The function hv_vtl_wakeup_secondary_cpu() cannot
> be used.
> 
> Instead, the virtual firmware boots the secondary CPUs and places them in
> a state to transfer control to the kernel using the wakeup mailbox.
> 
> The kernel updates the APIC callback wakeup_secondary_cpu_64() to use
> the mailbox if detected early during boot (enumerated via either an ACPI
> table or a DeviceTree node).
> 
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> ---
> Changes since v2:
>  - Unconditionally use the wakeup mailbox in a TDX confidential VM.
>    (Michael).
>  - Edited the commit message for clarity.
> 
> Changes since v1:
>  - None
> ---
>  arch/x86/hyperv/hv_vtl.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/hyperv/hv_vtl.c b/arch/x86/hyperv/hv_vtl.c
> index cd48bedd21f0..30a5a0c156c1 100644
> --- a/arch/x86/hyperv/hv_vtl.c
> +++ b/arch/x86/hyperv/hv_vtl.c
> @@ -299,7 +299,15 @@ int __init hv_vtl_early_init(void)
>  		panic("XSAVE has to be disabled as it is not supported by this module.\n"
>  			  "Please add 'noxsave' to the kernel command line.\n");
> 
> -	apic_update_callback(wakeup_secondary_cpu_64, hv_vtl_wakeup_secondary_cpu);
> +	/*
> +	 * TDX confidential VMs do not trust the hypervisor and cannot use it to
> +	 * boot secondary CPUs. Instead, they will be booted using the wakeup
> +	 * mailbox if detected during boot. See setup_arch().
> +	 *
> +	 * There is no paravisor present if we are here.
> +	 */
> +	if (!hv_isolation_type_tdx())
> +		apic_update_callback(wakeup_secondary_cpu_64, hv_vtl_wakeup_secondary_cpu);
> 
>  	return 0;
>  }
> --
> 2.43.0

Reviewed-by: Michael Kelley <mhklinux@outlook.com>

Re: [PATCH v3 13/13] x86/hyperv/vtl: Use the wakeup mailbox to boot secondary CPUs

[Ricardo Neri]

On Tue, May 20, 2025 at 01:35:02AM +0000, Michael Kelley wrote:
> From: Ricardo Neri <ricardo.neri-calderon@linux.intel.com> Sent: Saturday, May 3, 2025 12:15 PM
> > 
> > The hypervisor is an untrusted entity for TDX guests. It cannot be used
> > to boot secondary CPUs. The function hv_vtl_wakeup_secondary_cpu() cannot
> > be used.
> > 
> > Instead, the virtual firmware boots the secondary CPUs and places them in
> > a state to transfer control to the kernel using the wakeup mailbox.
> > 
> > The kernel updates the APIC callback wakeup_secondary_cpu_64() to use
> > the mailbox if detected early during boot (enumerated via either an ACPI
> > table or a DeviceTree node).
> > 
> > Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> > ---
> > Changes since v2:
> >  - Unconditionally use the wakeup mailbox in a TDX confidential VM.
> >    (Michael).
> >  - Edited the commit message for clarity.
> > 
> > Changes since v1:
> >  - None
> > ---
> >  arch/x86/hyperv/hv_vtl.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/hyperv/hv_vtl.c b/arch/x86/hyperv/hv_vtl.c
> > index cd48bedd21f0..30a5a0c156c1 100644
> > --- a/arch/x86/hyperv/hv_vtl.c
> > +++ b/arch/x86/hyperv/hv_vtl.c
> > @@ -299,7 +299,15 @@ int __init hv_vtl_early_init(void)
> >  		panic("XSAVE has to be disabled as it is not supported by this module.\n"
> >  			  "Please add 'noxsave' to the kernel command line.\n");
> > 
> > -	apic_update_callback(wakeup_secondary_cpu_64, hv_vtl_wakeup_secondary_cpu);
> > +	/*
> > +	 * TDX confidential VMs do not trust the hypervisor and cannot use it to
> > +	 * boot secondary CPUs. Instead, they will be booted using the wakeup
> > +	 * mailbox if detected during boot. See setup_arch().
> > +	 *
> > +	 * There is no paravisor present if we are here.
> > +	 */
> > +	if (!hv_isolation_type_tdx())
> > +		apic_update_callback(wakeup_secondary_cpu_64, hv_vtl_wakeup_secondary_cpu);
> > 
> >  	return 0;
> >  }
> > --
> > 2.43.0
> 
> Reviewed-by: Michael Kelley <mhklinux@outlook.com>

Thank you very much for your review!

Re: [PATCH v3 06/13] dt-bindings: reserved-memory: Wakeup Mailbox for Intel processors

[Ricardo Neri]

On Mon, May 19, 2025 at 10:56:06AM -0700, Ricardo Neri wrote:
> On Mon, May 19, 2025 at 10:29:37AM -0500, Rob Herring wrote:
> > On Wed, May 14, 2025 at 08:53:38PM -0700, Ricardo Neri wrote:
> > > On Wed, May 14, 2025 at 10:42:48AM -0500, Rob Herring wrote:
> > > > On Tue, May 13, 2025 at 03:14:56PM -0700, Ricardo Neri wrote:
> > > > > On Mon, May 12, 2025 at 10:32:24AM -0500, Rob Herring wrote:
> > > > > > On Tue, May 06, 2025 at 08:23:39PM -0700, Ricardo Neri wrote:
> > > > > > > On Tue, May 06, 2025 at 09:10:22AM +0200, Krzysztof Kozlowski wrote:
> > > > > > > > On Mon, May 05, 2025 at 10:16:10PM GMT, Ricardo Neri wrote:
> > > > > > > > > > If this is a device, then compatibles specific to devices. You do not
> > > > > > > > > > get different rules than all other bindings... or this does not have to
> > > > > > > > > > be binding at all. Why standard reserved-memory does not work for here?
> > > > > > > > > > 
> > > > > > > > > > Why do you need compatible in the first place?
> > > > > > > > > 
> > > > > > > > > Are you suggesting something like this?
> > > > > > > > > 
> > > > > > > > > reserved-memory {
> > > > > > > > > 	# address-cells = <2>;
> > > > > > > > > 	# size-cells = <1>;
> > > > > > > > > 
> > > > > > > > > 	wakeup_mailbox: wakeupmb@fff000 {
> > > > > > > > > 		reg = < 0x0 0xfff000 0x1000>
> > > > > > > > > 	}
> > > > > > > > > 
> > > > > > > > > and then reference to the reserved memory using the wakeup_mailbox
> > > > > > > > > phandle?
> > > > > > > > 
> > > > > > > > Yes just like every other, typical reserved memory block.
> > > > > > > 
> > > > > > > Thanks! I will take this approach and drop this patch.
> > > > > > 
> > > > > > If there is nothing else to this other than the reserved region, then 
> > > > > > don't do this. Keep it like you had. There's no need for 2 nodes.
> > > > > 
> > > > > Thank you for your feedback!
> > > > > 
> > > > > I was planning to use one reserved-memory node and inside of it a child
> > > > > node to with a `reg` property to specify the location and size of the
> > > > > mailbox. I would reference to that subnode from the kernel code.
> > > > > 
> > > > > IIUC, the reserved-memory node is only the container and the actual memory
> > > > > regions are expressed as child nodes.
> > > > > 
> > > > > I had it like that before, but with a `compatible` property that I did not
> > > > > need.
> > > > > 
> > > > > Am I missing anything?
> > > > 
> > > > Without a compatible, how do you identify which reserved region is the 
> > > > wakeup mailbox?
> > > 
> > > I thought using a phandle to the wakeup_mailbox. Then I realized that the
> > > device nodes using the mailbox would be CPUs. They would need a `memory-
> > > region` property. This does not look right to me.
> > 
> > That doesn't really make sense unless it's a memory region per CPU.
> 
> Agreed.
> 
> > 
> > 
> > > > Before you say node name, those are supposed to be 
> > > > generic though we failed to enforce anything for /reserved-memory child 
> > > > nodes.
> > > 
> > > I see. Thanks for preventing me from doing this.
> > > 
> > > Then the `compatible` property seems the way to go after all.
> > > 
> > > This what motivated this patch in the first place. On further analysis,
> > > IIUC, defining bindings and schema is not needed, IMO, since the mailbox
> > > is already defined in the ACPI spec. No need to redefine.
> > 
> > You lost me...
> > 
> > You don't need to redefine the layout of the memory region as that's 
> > defined already somewhere,
> 
> Great!
> 
> > but you do need to define where it is for DT. 
> > And for that, you need a compatible. Do you know where it is in this 
> > case?
> 
> The compatible is not defined anywhere yet. Is a DT schema needed to
> document it? If yes, I am usure what to put in the description. We tried
> to not redefine the mailbox and refer to the ACPI spec. That was a NAK
> from Krzysztof [1].
> 
> [1]. https://lore.kernel.org/r/624e1985-7dd2-4abe-a918-78cb43556967@kernel.org

In summary, documenting the `compatible` property for the mailbox is
necessary. There is no need to redefine the malbox on a schema but
referring to the ACPI spec is not acceptable.

What about referring in the schema to the Intel TDX Virtual Firmware Design
Guide[2]? It describes how firmware should implement the mailbox the section
4.3.5.

A mailbox with compatible = "intel,wakeup-mailbox" is implemented after the
guide that Intel published.

Rob, Krzysztof, would that be acceptable?

Thanks and BR,
Ricardo

[2]. https://www.intel.com/content/www/us/en/content-details/733585/intel-tdx-virtual-firmware-design-guide.html

Re: [PATCH v3 06/13] dt-bindings: reserved-memory: Wakeup Mailbox for Intel processors

[Rob Herring]

On Sat, May 24, 2025 at 08:56:50AM -0700, Ricardo Neri wrote:
> On Mon, May 19, 2025 at 10:56:06AM -0700, Ricardo Neri wrote:
> > On Mon, May 19, 2025 at 10:29:37AM -0500, Rob Herring wrote:
> > > On Wed, May 14, 2025 at 08:53:38PM -0700, Ricardo Neri wrote:
> > > > On Wed, May 14, 2025 at 10:42:48AM -0500, Rob Herring wrote:
> > > > > On Tue, May 13, 2025 at 03:14:56PM -0700, Ricardo Neri wrote:
> > > > > > On Mon, May 12, 2025 at 10:32:24AM -0500, Rob Herring wrote:
> > > > > > > On Tue, May 06, 2025 at 08:23:39PM -0700, Ricardo Neri wrote:
> > > > > > > > On Tue, May 06, 2025 at 09:10:22AM +0200, Krzysztof Kozlowski wrote:
> > > > > > > > > On Mon, May 05, 2025 at 10:16:10PM GMT, Ricardo Neri wrote:
> > > > > > > > > > > If this is a device, then compatibles specific to devices. You do not
> > > > > > > > > > > get different rules than all other bindings... or this does not have to
> > > > > > > > > > > be binding at all. Why standard reserved-memory does not work for here?
> > > > > > > > > > > 
> > > > > > > > > > > Why do you need compatible in the first place?
> > > > > > > > > > 
> > > > > > > > > > Are you suggesting something like this?
> > > > > > > > > > 
> > > > > > > > > > reserved-memory {
> > > > > > > > > > 	# address-cells = <2>;
> > > > > > > > > > 	# size-cells = <1>;
> > > > > > > > > > 
> > > > > > > > > > 	wakeup_mailbox: wakeupmb@fff000 {
> > > > > > > > > > 		reg = < 0x0 0xfff000 0x1000>
> > > > > > > > > > 	}
> > > > > > > > > > 
> > > > > > > > > > and then reference to the reserved memory using the wakeup_mailbox
> > > > > > > > > > phandle?
> > > > > > > > > 
> > > > > > > > > Yes just like every other, typical reserved memory block.
> > > > > > > > 
> > > > > > > > Thanks! I will take this approach and drop this patch.
> > > > > > > 
> > > > > > > If there is nothing else to this other than the reserved region, then 
> > > > > > > don't do this. Keep it like you had. There's no need for 2 nodes.
> > > > > > 
> > > > > > Thank you for your feedback!
> > > > > > 
> > > > > > I was planning to use one reserved-memory node and inside of it a child
> > > > > > node to with a `reg` property to specify the location and size of the
> > > > > > mailbox. I would reference to that subnode from the kernel code.
> > > > > > 
> > > > > > IIUC, the reserved-memory node is only the container and the actual memory
> > > > > > regions are expressed as child nodes.
> > > > > > 
> > > > > > I had it like that before, but with a `compatible` property that I did not
> > > > > > need.
> > > > > > 
> > > > > > Am I missing anything?
> > > > > 
> > > > > Without a compatible, how do you identify which reserved region is the 
> > > > > wakeup mailbox?
> > > > 
> > > > I thought using a phandle to the wakeup_mailbox. Then I realized that the
> > > > device nodes using the mailbox would be CPUs. They would need a `memory-
> > > > region` property. This does not look right to me.
> > > 
> > > That doesn't really make sense unless it's a memory region per CPU.
> > 
> > Agreed.
> > 
> > > 
> > > 
> > > > > Before you say node name, those are supposed to be 
> > > > > generic though we failed to enforce anything for /reserved-memory child 
> > > > > nodes.
> > > > 
> > > > I see. Thanks for preventing me from doing this.
> > > > 
> > > > Then the `compatible` property seems the way to go after all.
> > > > 
> > > > This what motivated this patch in the first place. On further analysis,
> > > > IIUC, defining bindings and schema is not needed, IMO, since the mailbox
> > > > is already defined in the ACPI spec. No need to redefine.
> > > 
> > > You lost me...
> > > 
> > > You don't need to redefine the layout of the memory region as that's 
> > > defined already somewhere,
> > 
> > Great!
> > 
> > > but you do need to define where it is for DT. 
> > > And for that, you need a compatible. Do you know where it is in this 
> > > case?
> > 
> > The compatible is not defined anywhere yet. Is a DT schema needed to
> > document it? If yes, I am usure what to put in the description. We tried
> > to not redefine the mailbox and refer to the ACPI spec. That was a NAK
> > from Krzysztof [1].
> > 
> > [1]. https://lore.kernel.org/r/624e1985-7dd2-4abe-a918-78cb43556967@kernel.org
> 
> In summary, documenting the `compatible` property for the mailbox is
> necessary. There is no need to redefine the malbox on a schema but
> referring to the ACPI spec is not acceptable.

There's the whole "DT bindings in ACPI systems" where ACPI tables 
contain compatibles and DT properties which I think is what 
Krzysztof was objecting to (and I do too). But this is a DT based system 
that implements a mailbox region defined in an ACPI spec. That is 
perfectly fine to refer to.

> 
> What about referring in the schema to the Intel TDX Virtual Firmware Design
> Guide[2]? It describes how firmware should implement the mailbox the section
> 4.3.5.
> 
> A mailbox with compatible = "intel,wakeup-mailbox" is implemented after the
> guide that Intel published.

Use whatever you think best describes the programming model of the 
region.

Rob

Re: [PATCH v3 06/13] dt-bindings: reserved-memory: Wakeup Mailbox for Intel processors

[Ricardo Neri]

On Thu, May 29, 2025 at 08:16:34AM -0500, Rob Herring wrote:
> On Sat, May 24, 2025 at 08:56:50AM -0700, Ricardo Neri wrote:
> > On Mon, May 19, 2025 at 10:56:06AM -0700, Ricardo Neri wrote:
> > > On Mon, May 19, 2025 at 10:29:37AM -0500, Rob Herring wrote:
> > > > On Wed, May 14, 2025 at 08:53:38PM -0700, Ricardo Neri wrote:
> > > > > On Wed, May 14, 2025 at 10:42:48AM -0500, Rob Herring wrote:
> > > > > > On Tue, May 13, 2025 at 03:14:56PM -0700, Ricardo Neri wrote:
> > > > > > > On Mon, May 12, 2025 at 10:32:24AM -0500, Rob Herring wrote:
> > > > > > > > On Tue, May 06, 2025 at 08:23:39PM -0700, Ricardo Neri wrote:
> > > > > > > > > On Tue, May 06, 2025 at 09:10:22AM +0200, Krzysztof Kozlowski wrote:
> > > > > > > > > > On Mon, May 05, 2025 at 10:16:10PM GMT, Ricardo Neri wrote:
> > > > > > > > > > > > If this is a device, then compatibles specific to devices. You do not
> > > > > > > > > > > > get different rules than all other bindings... or this does not have to
> > > > > > > > > > > > be binding at all. Why standard reserved-memory does not work for here?
> > > > > > > > > > > > 
> > > > > > > > > > > > Why do you need compatible in the first place?
> > > > > > > > > > > 
> > > > > > > > > > > Are you suggesting something like this?
> > > > > > > > > > > 
> > > > > > > > > > > reserved-memory {
> > > > > > > > > > > 	# address-cells = <2>;
> > > > > > > > > > > 	# size-cells = <1>;
> > > > > > > > > > > 
> > > > > > > > > > > 	wakeup_mailbox: wakeupmb@fff000 {
> > > > > > > > > > > 		reg = < 0x0 0xfff000 0x1000>
> > > > > > > > > > > 	}
> > > > > > > > > > > 
> > > > > > > > > > > and then reference to the reserved memory using the wakeup_mailbox
> > > > > > > > > > > phandle?
> > > > > > > > > > 
> > > > > > > > > > Yes just like every other, typical reserved memory block.
> > > > > > > > > 
> > > > > > > > > Thanks! I will take this approach and drop this patch.
> > > > > > > > 
> > > > > > > > If there is nothing else to this other than the reserved region, then 
> > > > > > > > don't do this. Keep it like you had. There's no need for 2 nodes.
> > > > > > > 
> > > > > > > Thank you for your feedback!
> > > > > > > 
> > > > > > > I was planning to use one reserved-memory node and inside of it a child
> > > > > > > node to with a `reg` property to specify the location and size of the
> > > > > > > mailbox. I would reference to that subnode from the kernel code.
> > > > > > > 
> > > > > > > IIUC, the reserved-memory node is only the container and the actual memory
> > > > > > > regions are expressed as child nodes.
> > > > > > > 
> > > > > > > I had it like that before, but with a `compatible` property that I did not
> > > > > > > need.
> > > > > > > 
> > > > > > > Am I missing anything?
> > > > > > 
> > > > > > Without a compatible, how do you identify which reserved region is the 
> > > > > > wakeup mailbox?
> > > > > 
> > > > > I thought using a phandle to the wakeup_mailbox. Then I realized that the
> > > > > device nodes using the mailbox would be CPUs. They would need a `memory-
> > > > > region` property. This does not look right to me.
> > > > 
> > > > That doesn't really make sense unless it's a memory region per CPU.
> > > 
> > > Agreed.
> > > 
> > > > 
> > > > 
> > > > > > Before you say node name, those are supposed to be 
> > > > > > generic though we failed to enforce anything for /reserved-memory child 
> > > > > > nodes.
> > > > > 
> > > > > I see. Thanks for preventing me from doing this.
> > > > > 
> > > > > Then the `compatible` property seems the way to go after all.
> > > > > 
> > > > > This what motivated this patch in the first place. On further analysis,
> > > > > IIUC, defining bindings and schema is not needed, IMO, since the mailbox
> > > > > is already defined in the ACPI spec. No need to redefine.
> > > > 
> > > > You lost me...
> > > > 
> > > > You don't need to redefine the layout of the memory region as that's 
> > > > defined already somewhere,
> > > 
> > > Great!
> > > 
> > > > but you do need to define where it is for DT. 
> > > > And for that, you need a compatible. Do you know where it is in this 
> > > > case?
> > > 
> > > The compatible is not defined anywhere yet. Is a DT schema needed to
> > > document it? If yes, I am usure what to put in the description. We tried
> > > to not redefine the mailbox and refer to the ACPI spec. That was a NAK
> > > from Krzysztof [1].
> > > 
> > > [1]. https://lore.kernel.org/r/624e1985-7dd2-4abe-a918-78cb43556967@kernel.org
> > 
> > In summary, documenting the `compatible` property for the mailbox is
> > necessary. There is no need to redefine the malbox on a schema but
> > referring to the ACPI spec is not acceptable.
> 
> There's the whole "DT bindings in ACPI systems" where ACPI tables 
> contain compatibles and DT properties which I think is what 
> Krzysztof was objecting to (and I do too). But this is a DT based system 
> that implements a mailbox region defined in an ACPI spec. That is 
> perfectly fine to refer to.

That is correct. It is a DT-based system.

Great! I will refer to the ACPI spec in my schema in my next version.

> 
> > 
> > What about referring in the schema to the Intel TDX Virtual Firmware Design
> > Guide[2]? It describes how firmware should implement the mailbox the section
> > 4.3.5.
> > 
> > A mailbox with compatible = "intel,wakeup-mailbox" is implemented after the
> > guide that Intel published.
> 
> Use whatever you think best describes the programming model of the 
> region.

Understood.

Thanks and BR,
Ricardo