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

Rafael J. Wysocki
2025-05-06T14:01:28Z

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