[PATCH v2 0/9] x86/hyperv: Support wakeup mailbox for VTL2 TDX guest

[PATCH v2 0/9] x86/hyperv: Support wakeup mailbox for VTL2 TDX guest

[Yunhong Jiang]

This set of patches add ACPI multiprocessor wakeup support to VTL2 TDX VMs
booting with device tree instead of ACPI.

Historically, x86 platforms have booted secondary processors (APs) using
INIT followed by the start up IPI (SIPI) messages. However, TDX VMs
can't use this protocol because this protocol requires assistance from
VMMs while VMMs are not trusted by TDX guest.

ACPI specification version 6.4 introduced a new wakeup mailbox model to
address this issue. A "Multiprocessor Wakeup Structure" has been added to
an existing ACPI table (MADT). This structure provides the physical of a
"Multiprocessor Wakeup Mailbox Structure". Message written to the mailbox
structure steers the APs to the boot code.

With this new wakeup model, TDX VMs with ACPI support boot the APs
securely. However, TDX VMs with the device tree, like VTL2 TDX hyperv
guests, have no ACPI support and still face the challenge.

To fix this challenge, either a new mechanism from scratch is
introduced, or the TDX VMs with device tree can utilize the ACPI wakeup
model.

By reusing the ACPI wakeup mailbox model, the Multiprocessor Wakeup Mailbox
Structure will be kept and the message mechanism will be the same as ACPI.
This will reduce maintenance effort in the long term.

The first patch moves the madt wakeup implementation to generic code.
The patches 2-3 add the wakeup mailbox support to the device tree.
The patches 4-5 add the wakeup mailbox support to the hyperv guest.
The patches 6-8 update the real mode memory reservation.
The last patch applies the wakeup mailbox support to the VTL2 TDX guest.

v2:
  - 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.

Yunhong Jiang (9):
  x86/acpi: Move ACPI MADT wakeup to generic code
  dt-bindings: x86: Add a binding for x86 wakeup mailbox
  x86/dt: Support the ACPI multiprocessor wakeup for device tree
  x86/hyperv: Parse the ACPI wakeup mailbox
  x86/hyperv: Mark ACPI wakeup mailbox page as private
  x86/realmode: Add memory range support to reserve_real_mode
  x86/hyperv: Move setting the real_mode_header to hv_vtl_init_platform
  x86/hyperv: Set realmode_limit to 4G for VTL2 TDX guest
  x86/hyperv: Use wakeup mailbox for VTL2 guests if available

 .../devicetree/bindings/x86/wakeup.yaml       | 64 +++++++++++++++++++
 MAINTAINERS                                   |  3 +
 arch/x86/Kconfig                              |  2 +-
 arch/x86/hyperv/hv_vtl.c                      | 29 +++++++--
 arch/x86/include/asm/acpi.h                   |  1 -
 arch/x86/include/asm/madt_wakeup.h            | 16 +++++
 arch/x86/include/asm/mshyperv.h               |  3 +
 arch/x86/include/asm/x86_init.h               |  6 ++
 arch/x86/kernel/Makefile                      |  1 +
 arch/x86/kernel/acpi/Makefile                 |  1 -
 arch/x86/kernel/cpu/mshyperv.c                |  2 +
 arch/x86/kernel/{acpi => }/madt_playdead.S    |  0
 arch/x86/kernel/{acpi => }/madt_wakeup.c      | 38 +++++++++++
 arch/x86/kernel/x86_init.c                    |  3 +
 arch/x86/realmode/init.c                      | 14 ++--
 drivers/hv/hv_common.c                        |  8 +++
 16 files changed, 176 insertions(+), 15 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/x86/wakeup.yaml
 create mode 100644 arch/x86/include/asm/madt_wakeup.h
 rename arch/x86/kernel/{acpi => }/madt_playdead.S (100%)
 rename arch/x86/kernel/{acpi => }/madt_wakeup.c (90%)


base-commit: fedb9ddeb1445f85d8f691b20f3faaa6dab8dd3f
-- 
2.25.1

[PATCH v2 1/9] x86/acpi: Move ACPI MADT wakeup to generic code

[Yunhong Jiang]

In order to support the ACPI mailbox wakeup in device tree, move the MADT
wakeup code out of the acpi directory, so that both ACPI and device tree
can use it.

Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
---
 MAINTAINERS                                | 2 ++
 arch/x86/kernel/Makefile                   | 1 +
 arch/x86/kernel/acpi/Makefile              | 1 -
 arch/x86/kernel/{acpi => }/madt_playdead.S | 0
 arch/x86/kernel/{acpi => }/madt_wakeup.c   | 0
 5 files changed, 3 insertions(+), 1 deletion(-)
 rename arch/x86/kernel/{acpi => }/madt_playdead.S (100%)
 rename arch/x86/kernel/{acpi => }/madt_wakeup.c (100%)

diff --git a/MAINTAINERS b/MAINTAINERS
index 969eb9c6e759..5555a3bbac5f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -289,6 +289,8 @@ F:	Documentation/ABI/testing/configfs-acpi
 F:	Documentation/ABI/testing/sysfs-bus-acpi
 F:	Documentation/firmware-guide/acpi/
 F:	arch/x86/kernel/acpi/
+F:	arch/x86/kernel/madt_playdead.S
+F:	arch/x86/kernel/madt_wakeup.c
 F:	arch/x86/pci/acpi.c
 F:	drivers/acpi/
 F:	drivers/pci/*/*acpi*
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index f7918980667a..0823e5ba3aea 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -163,4 +163,5 @@ ifeq ($(CONFIG_X86_64),y)
 
 	obj-$(CONFIG_MMCONF_FAM10H)	+= mmconf-fam10h_64.o
 	obj-y				+= vsmp_64.o
+	obj-$(CONFIG_ACPI_MADT_WAKEUP)	+= madt_wakeup.o madt_playdead.o
 endif
diff --git a/arch/x86/kernel/acpi/Makefile b/arch/x86/kernel/acpi/Makefile
index 842a5f449404..fc17b3f136fe 100644
--- a/arch/x86/kernel/acpi/Makefile
+++ b/arch/x86/kernel/acpi/Makefile
@@ -4,7 +4,6 @@ obj-$(CONFIG_ACPI)		+= boot.o
 obj-$(CONFIG_ACPI_SLEEP)	+= sleep.o wakeup_$(BITS).o
 obj-$(CONFIG_ACPI_APEI)		+= apei.o
 obj-$(CONFIG_ACPI_CPPC_LIB)	+= cppc.o
-obj-$(CONFIG_ACPI_MADT_WAKEUP)	+= madt_wakeup.o madt_playdead.o
 
 ifneq ($(CONFIG_ACPI_PROCESSOR),)
 obj-y				+= cstate.o
diff --git a/arch/x86/kernel/acpi/madt_playdead.S b/arch/x86/kernel/madt_playdead.S
similarity index 100%
rename from arch/x86/kernel/acpi/madt_playdead.S
rename to arch/x86/kernel/madt_playdead.S
diff --git a/arch/x86/kernel/acpi/madt_wakeup.c b/arch/x86/kernel/madt_wakeup.c
similarity index 100%
rename from arch/x86/kernel/acpi/madt_wakeup.c
rename to arch/x86/kernel/madt_wakeup.c
-- 
2.25.1

[PATCH v2 2/9] dt-bindings: x86: Add a binding for x86 wakeup mailbox

[Yunhong Jiang]

Add the binding to use mailbox wakeup mechanism to bringup APs.

Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
---
 .../devicetree/bindings/x86/wakeup.yaml       | 64 +++++++++++++++++++
 1 file changed, 64 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/x86/wakeup.yaml

diff --git a/Documentation/devicetree/bindings/x86/wakeup.yaml b/Documentation/devicetree/bindings/x86/wakeup.yaml
new file mode 100644
index 000000000000..cb84e2756bca
--- /dev/null
+++ b/Documentation/devicetree/bindings/x86/wakeup.yaml
@@ -0,0 +1,64 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) 2024 Intel Corporation
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/x86/wakeup.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: x86 mailbox wakeup
+maintainers:
+  - Yunhong Jiang <yunhong.jiang@linux.intel.com>
+
+description: |
+  The x86 mailbox wakeup mechanism defines a mechanism to let the bootstrap
+  processor (BSP) to wake up application processors (APs) through a wakeup
+  mailbox.
+
+  The "wakeup-mailbox-addr" property specifies the wakeup mailbox address. The
+  wakeup mailbox is a 4K-aligned 4K-size memory block allocated in the reserved
+  memory.
+
+  The wakeup mailbox structure is defined as follows.
+
+    uint16_t command;
+    uint16_t reserved;
+    uint32_t apic_id;
+    uint64_t wakeup_vector;
+    uint8_t  reservedForOs[2032];
+
+  The memory after reservedForOs field is reserved and OS should not touch it.
+
+  To wakes up a AP, the BSP prepares the wakeup routine, fills the wakeup
+  routine's address into the wakeup_vector field, fill the apic_id field with
+  the target AP's APIC_ID, and write 1 to the command field. After receiving the
+  wakeup command, the target AP will jump to the wakeup routine.
+
+  For each AP, the mailbox can be used only once for the wakeup command. After
+  the AP jumps to the wakeup routine, the mailbox will no longer be checked by
+  this AP.
+
+  The wakeup mailbox structure and the wakeup process is the same as
+  the Multiprocessor Wakeup Mailbox Structure defined in ACPI spec version 6.5,
+  section 5.2.12.19 [1].
+
+  References:
+
+  [1] https://uefi.org/specs/ACPI/6.5/05_ACPI_Software_Programming_Model.html
+
+select: false
+
+properties:
+  wakeup-mailbox-addr:
+    $ref: /schemas/types.yaml#/definitions/uint64
+    description: |
+      The physical address of the wakeup mailbox data structure. The address
+      must be 4K bytes aligned 4k-size memory and it should be in the reserved
+      memory.
+
+      This requires the "enable-method" property in the cpus node is set to
+      "acpi-wakeup-mailbox".
+
+required:
+  - wakeup-mailbox-addr
+
+additionalProperties: false
-- 
2.25.1

Re: [PATCH v2 2/9] dt-bindings: x86: Add a binding for x86 wakeup mailbox

[Krzysztof Kozlowski]

On Fri, Aug 23, 2024 at 04:23:20PM -0700, Yunhong Jiang wrote:
> Add the binding to use mailbox wakeup mechanism to bringup APs.
> 
> Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
> ---
>  .../devicetree/bindings/x86/wakeup.yaml       | 64 +++++++++++++++++++
>  1 file changed, 64 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/x86/wakeup.yaml
> 
> diff --git a/Documentation/devicetree/bindings/x86/wakeup.yaml b/Documentation/devicetree/bindings/x86/wakeup.yaml
> new file mode 100644
> index 000000000000..cb84e2756bca
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/x86/wakeup.yaml
> @@ -0,0 +1,64 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) 2024 Intel Corporation
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/x86/wakeup.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: x86 mailbox wakeup
> +maintainers:
> +  - Yunhong Jiang <yunhong.jiang@linux.intel.com>
> +
> +description: |
> +  The x86 mailbox wakeup mechanism defines a mechanism to let the bootstrap
> +  processor (BSP) to wake up application processors (APs) through a wakeup
> +  mailbox.
> +
> +  The "wakeup-mailbox-addr" property specifies the wakeup mailbox address. The
> +  wakeup mailbox is a 4K-aligned 4K-size memory block allocated in the reserved
> +  memory.
> +
> +  The wakeup mailbox structure is defined as follows.
> +
> +    uint16_t command;
> +    uint16_t reserved;
> +    uint32_t apic_id;
> +    uint64_t wakeup_vector;
> +    uint8_t  reservedForOs[2032];
> +
> +  The memory after reservedForOs field is reserved and OS should not touch it.
> +
> +  To wakes up a AP, the BSP prepares the wakeup routine, fills the wakeup
> +  routine's address into the wakeup_vector field, fill the apic_id field with
> +  the target AP's APIC_ID, and write 1 to the command field. After receiving the
> +  wakeup command, the target AP will jump to the wakeup routine.
> +
> +  For each AP, the mailbox can be used only once for the wakeup command. After
> +  the AP jumps to the wakeup routine, the mailbox will no longer be checked by
> +  this AP.
> +
> +  The wakeup mailbox structure and the wakeup process is the same as
> +  the Multiprocessor Wakeup Mailbox Structure defined in ACPI spec version 6.5,
> +  section 5.2.12.19 [1].
> +
> +  References:
> +
> +  [1] https://uefi.org/specs/ACPI/6.5/05_ACPI_Software_Programming_Model.html
> +
> +select: false

This schema is still a no-op because of this false.

What is the point of defining one property if it is not placed anywhere?
Every device node can have it? Seems wrong...

You need to come with proper schema. Lack of an example is another thing
- this cannot be even validated by the tools. 

Best regards,
Krzysztof

Re: [PATCH v2 2/9] dt-bindings: x86: Add a binding for x86 wakeup mailbox

[Yunhong Jiang]

On Sun, Aug 25, 2024 at 09:10:01AM +0200, Krzysztof Kozlowski wrote:
> On Fri, Aug 23, 2024 at 04:23:20PM -0700, Yunhong Jiang wrote:
> > Add the binding to use mailbox wakeup mechanism to bringup APs.
> > 
> > Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
> > ---
> >  .../devicetree/bindings/x86/wakeup.yaml       | 64 +++++++++++++++++++
> >  1 file changed, 64 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/x86/wakeup.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/x86/wakeup.yaml b/Documentation/devicetree/bindings/x86/wakeup.yaml
> > new file mode 100644
> > index 000000000000..cb84e2756bca
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/x86/wakeup.yaml
> > @@ -0,0 +1,64 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +# Copyright (C) 2024 Intel Corporation
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/x86/wakeup.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: x86 mailbox wakeup
> > +maintainers:
> > +  - Yunhong Jiang <yunhong.jiang@linux.intel.com>
> > +
> > +description: |
> > +  The x86 mailbox wakeup mechanism defines a mechanism to let the bootstrap
> > +  processor (BSP) to wake up application processors (APs) through a wakeup
> > +  mailbox.
> > +
> > +  The "wakeup-mailbox-addr" property specifies the wakeup mailbox address. The
> > +  wakeup mailbox is a 4K-aligned 4K-size memory block allocated in the reserved
> > +  memory.
> > +
> > +  The wakeup mailbox structure is defined as follows.
> > +
> > +    uint16_t command;
> > +    uint16_t reserved;
> > +    uint32_t apic_id;
> > +    uint64_t wakeup_vector;
> > +    uint8_t  reservedForOs[2032];
> > +
> > +  The memory after reservedForOs field is reserved and OS should not touch it.
> > +
> > +  To wakes up a AP, the BSP prepares the wakeup routine, fills the wakeup
> > +  routine's address into the wakeup_vector field, fill the apic_id field with
> > +  the target AP's APIC_ID, and write 1 to the command field. After receiving the
> > +  wakeup command, the target AP will jump to the wakeup routine.
> > +
> > +  For each AP, the mailbox can be used only once for the wakeup command. After
> > +  the AP jumps to the wakeup routine, the mailbox will no longer be checked by
> > +  this AP.
> > +
> > +  The wakeup mailbox structure and the wakeup process is the same as
> > +  the Multiprocessor Wakeup Mailbox Structure defined in ACPI spec version 6.5,
> > +  section 5.2.12.19 [1].
> > +
> > +  References:
> > +
> > +  [1] https://uefi.org/specs/ACPI/6.5/05_ACPI_Software_Programming_Model.html
> > +
> > +select: false
> 
> This schema is still a no-op because of this false.
> 
> What is the point of defining one property if it is not placed anywhere?
> Every device node can have it? Seems wrong...
> 
> You need to come with proper schema. Lack of an example is another thing
> - this cannot be even validated by the tools. 
> 
> Best regards,
> Krzysztof

Thank you for the feedback. Will update the schema file on next round
submission.

Thanks
--jyh

> 

Re: [PATCH v2 2/9] dt-bindings: x86: Add a binding for x86 wakeup mailbox

[Yunhong Jiang]

On Tue, Aug 27, 2024 at 01:45:49PM -0700, Yunhong Jiang wrote:
> On Sun, Aug 25, 2024 at 09:10:01AM +0200, Krzysztof Kozlowski wrote:
> > On Fri, Aug 23, 2024 at 04:23:20PM -0700, Yunhong Jiang wrote:
> > > Add the binding to use mailbox wakeup mechanism to bringup APs.
> > > 
> > > Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
> > > ---
> > >  .../devicetree/bindings/x86/wakeup.yaml       | 64 +++++++++++++++++++
> > >  1 file changed, 64 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/x86/wakeup.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/x86/wakeup.yaml b/Documentation/devicetree/bindings/x86/wakeup.yaml
> > > new file mode 100644
> > > index 000000000000..cb84e2756bca
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/x86/wakeup.yaml
> > > @@ -0,0 +1,64 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +# Copyright (C) 2024 Intel Corporation
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/x86/wakeup.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: x86 mailbox wakeup
> > > +maintainers:
> > > +  - Yunhong Jiang <yunhong.jiang@linux.intel.com>
> > > +
> > > +description: |
> > > +  The x86 mailbox wakeup mechanism defines a mechanism to let the bootstrap
> > > +  processor (BSP) to wake up application processors (APs) through a wakeup
> > > +  mailbox.
> > > +
> > > +  The "wakeup-mailbox-addr" property specifies the wakeup mailbox address. The
> > > +  wakeup mailbox is a 4K-aligned 4K-size memory block allocated in the reserved
> > > +  memory.
> > > +
> > > +  The wakeup mailbox structure is defined as follows.
> > > +
> > > +    uint16_t command;
> > > +    uint16_t reserved;
> > > +    uint32_t apic_id;
> > > +    uint64_t wakeup_vector;
> > > +    uint8_t  reservedForOs[2032];
> > > +
> > > +  The memory after reservedForOs field is reserved and OS should not touch it.
> > > +
> > > +  To wakes up a AP, the BSP prepares the wakeup routine, fills the wakeup
> > > +  routine's address into the wakeup_vector field, fill the apic_id field with
> > > +  the target AP's APIC_ID, and write 1 to the command field. After receiving the
> > > +  wakeup command, the target AP will jump to the wakeup routine.
> > > +
> > > +  For each AP, the mailbox can be used only once for the wakeup command. After
> > > +  the AP jumps to the wakeup routine, the mailbox will no longer be checked by
> > > +  this AP.
> > > +
> > > +  The wakeup mailbox structure and the wakeup process is the same as
> > > +  the Multiprocessor Wakeup Mailbox Structure defined in ACPI spec version 6.5,
> > > +  section 5.2.12.19 [1].
> > > +
> > > +  References:
> > > +
> > > +  [1] https://uefi.org/specs/ACPI/6.5/05_ACPI_Software_Programming_Model.html
> > > +
> > > +select: false
> > 
> > This schema is still a no-op because of this false.
> > 
> > What is the point of defining one property if it is not placed anywhere?
> > Every device node can have it? Seems wrong...
> > 
> > You need to come with proper schema. Lack of an example is another thing
> > - this cannot be even validated by the tools. 
> > 
> > Best regards,
> > Krzysztof

Hi, Krzysztof, I'm working to address your comments and have some questions.
Hope to get help/guide from your side.

For the select, the writing-schema.rst describes it as "A json-schema used to
match nodes for applying the schema" but I'm a bit confused. In my case, should
it be "cpus" node? Is there any code/tools that uses this property, so that I
can have a better understanding?

For your "validated by the tools", can you please share the tools you used to
validate the schema? I used "make dt_binding_check" per the
submitting-patches.rst but I think your comments is about another tool.

Sorry for the bothering. I read the DT spec and the
Documentation/devicetree/bindings documents and still not sure.

Than you
--jyh

> 
> Thank you for the feedback. Will update the schema file on next round
> submission.
> 
> Thanks
> --jyh
> 
> > 
> 

Re: [PATCH v2 2/9] dt-bindings: x86: Add a binding for x86 wakeup mailbox

[Krzysztof Kozlowski]

On 10/09/2024 08:13, Yunhong Jiang wrote:
> On Tue, Aug 27, 2024 at 01:45:49PM -0700, Yunhong Jiang wrote:
>> On Sun, Aug 25, 2024 at 09:10:01AM +0200, Krzysztof Kozlowski wrote:
>>> On Fri, Aug 23, 2024 at 04:23:20PM -0700, Yunhong Jiang wrote:
>>>> Add the binding to use mailbox wakeup mechanism to bringup APs.
>>>>
>>>> Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
>>>> ---
>>>>  .../devicetree/bindings/x86/wakeup.yaml       | 64 +++++++++++++++++++
>>>>  1 file changed, 64 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/x86/wakeup.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/x86/wakeup.yaml b/Documentation/devicetree/bindings/x86/wakeup.yaml
>>>> new file mode 100644
>>>> index 000000000000..cb84e2756bca
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/x86/wakeup.yaml
>>>> @@ -0,0 +1,64 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +# Copyright (C) 2024 Intel Corporation
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/x86/wakeup.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: x86 mailbox wakeup
>>>> +maintainers:
>>>> +  - Yunhong Jiang <yunhong.jiang@linux.intel.com>
>>>> +
>>>> +description: |
>>>> +  The x86 mailbox wakeup mechanism defines a mechanism to let the bootstrap
>>>> +  processor (BSP) to wake up application processors (APs) through a wakeup
>>>> +  mailbox.
>>>> +
>>>> +  The "wakeup-mailbox-addr" property specifies the wakeup mailbox address. The
>>>> +  wakeup mailbox is a 4K-aligned 4K-size memory block allocated in the reserved
>>>> +  memory.
>>>> +
>>>> +  The wakeup mailbox structure is defined as follows.
>>>> +
>>>> +    uint16_t command;
>>>> +    uint16_t reserved;
>>>> +    uint32_t apic_id;
>>>> +    uint64_t wakeup_vector;
>>>> +    uint8_t  reservedForOs[2032];
>>>> +
>>>> +  The memory after reservedForOs field is reserved and OS should not touch it.
>>>> +
>>>> +  To wakes up a AP, the BSP prepares the wakeup routine, fills the wakeup
>>>> +  routine's address into the wakeup_vector field, fill the apic_id field with
>>>> +  the target AP's APIC_ID, and write 1 to the command field. After receiving the
>>>> +  wakeup command, the target AP will jump to the wakeup routine.
>>>> +
>>>> +  For each AP, the mailbox can be used only once for the wakeup command. After
>>>> +  the AP jumps to the wakeup routine, the mailbox will no longer be checked by
>>>> +  this AP.
>>>> +
>>>> +  The wakeup mailbox structure and the wakeup process is the same as
>>>> +  the Multiprocessor Wakeup Mailbox Structure defined in ACPI spec version 6.5,
>>>> +  section 5.2.12.19 [1].
>>>> +
>>>> +  References:
>>>> +
>>>> +  [1] https://uefi.org/specs/ACPI/6.5/05_ACPI_Software_Programming_Model.html
>>>> +
>>>> +select: false
>>>
>>> This schema is still a no-op because of this false.
>>>
>>> What is the point of defining one property if it is not placed anywhere?
>>> Every device node can have it? Seems wrong...
>>>
>>> You need to come with proper schema. Lack of an example is another thing
>>> - this cannot be even validated by the tools. 
>>>
>>> Best regards,
>>> Krzysztof
> 
> Hi, Krzysztof, I'm working to address your comments and have some questions.
> Hope to get help/guide from your side.
> 
> For the select, the writing-schema.rst describes it as "A json-schema used to
> match nodes for applying the schema" but I'm a bit confused. In my case, should
> it be "cpus" node? Is there any code/tools that uses this property, so that I
> can have a better understanding?

Usually we expect matching by compatible, but it does not seem suitable
here because it is not related to any specific device, right? That is
the problem with all this DT-reuse-for-virtual-stuff work. It just does
not follow usual expectations and guidelines - you do not describe a device.

You can still match by nodes. See all top-level bindings.

> 
> For your "validated by the tools", can you please share the tools you used to
> validate the schema? I used "make dt_binding_check" per the
> submitting-patches.rst but I think your comments is about another tool.

See writing-schema document.


Best regards,
Krzysztof

Re: [PATCH v2 2/9] dt-bindings: x86: Add a binding for x86 wakeup mailbox

[Yunhong Jiang]

On Mon, Sep 16, 2024 at 10:56:38AM +0200, Krzysztof Kozlowski wrote:
> On 10/09/2024 08:13, Yunhong Jiang wrote:
> > On Tue, Aug 27, 2024 at 01:45:49PM -0700, Yunhong Jiang wrote:
> >> On Sun, Aug 25, 2024 at 09:10:01AM +0200, Krzysztof Kozlowski wrote:
> >>> On Fri, Aug 23, 2024 at 04:23:20PM -0700, Yunhong Jiang wrote:
> >>>> Add the binding to use mailbox wakeup mechanism to bringup APs.
> >>>>
> >>>> Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
> >>>> ---
> >>>>  .../devicetree/bindings/x86/wakeup.yaml       | 64 +++++++++++++++++++
> >>>>  1 file changed, 64 insertions(+)
> >>>>  create mode 100644 Documentation/devicetree/bindings/x86/wakeup.yaml
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/x86/wakeup.yaml b/Documentation/devicetree/bindings/x86/wakeup.yaml
> >>>> new file mode 100644
> >>>> index 000000000000..cb84e2756bca
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/x86/wakeup.yaml
> >>>> @@ -0,0 +1,64 @@
> >>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>>> +# Copyright (C) 2024 Intel Corporation
> >>>> +%YAML 1.2
> >>>> +---
> >>>> +$id: http://devicetree.org/schemas/x86/wakeup.yaml#
> >>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>> +
> >>>> +title: x86 mailbox wakeup
> >>>> +maintainers:
> >>>> +  - Yunhong Jiang <yunhong.jiang@linux.intel.com>
> >>>> +
> >>>> +description: |
> >>>> +  The x86 mailbox wakeup mechanism defines a mechanism to let the bootstrap
> >>>> +  processor (BSP) to wake up application processors (APs) through a wakeup
> >>>> +  mailbox.
> >>>> +
> >>>> +  The "wakeup-mailbox-addr" property specifies the wakeup mailbox address. The
> >>>> +  wakeup mailbox is a 4K-aligned 4K-size memory block allocated in the reserved
> >>>> +  memory.
> >>>> +
> >>>> +  The wakeup mailbox structure is defined as follows.
> >>>> +
> >>>> +    uint16_t command;
> >>>> +    uint16_t reserved;
> >>>> +    uint32_t apic_id;
> >>>> +    uint64_t wakeup_vector;
> >>>> +    uint8_t  reservedForOs[2032];
> >>>> +
> >>>> +  The memory after reservedForOs field is reserved and OS should not touch it.
> >>>> +
> >>>> +  To wakes up a AP, the BSP prepares the wakeup routine, fills the wakeup
> >>>> +  routine's address into the wakeup_vector field, fill the apic_id field with
> >>>> +  the target AP's APIC_ID, and write 1 to the command field. After receiving the
> >>>> +  wakeup command, the target AP will jump to the wakeup routine.
> >>>> +
> >>>> +  For each AP, the mailbox can be used only once for the wakeup command. After
> >>>> +  the AP jumps to the wakeup routine, the mailbox will no longer be checked by
> >>>> +  this AP.
> >>>> +
> >>>> +  The wakeup mailbox structure and the wakeup process is the same as
> >>>> +  the Multiprocessor Wakeup Mailbox Structure defined in ACPI spec version 6.5,
> >>>> +  section 5.2.12.19 [1].
> >>>> +
> >>>> +  References:
> >>>> +
> >>>> +  [1] https://uefi.org/specs/ACPI/6.5/05_ACPI_Software_Programming_Model.html
> >>>> +
> >>>> +select: false
> >>>
> >>> This schema is still a no-op because of this false.
> >>>
> >>> What is the point of defining one property if it is not placed anywhere?
> >>> Every device node can have it? Seems wrong...
> >>>
> >>> You need to come with proper schema. Lack of an example is another thing
> >>> - this cannot be even validated by the tools. 
> >>>
> >>> Best regards,
> >>> Krzysztof
> > 
> > Hi, Krzysztof, I'm working to address your comments and have some questions.
> > Hope to get help/guide from your side.
> > 
> > For the select, the writing-schema.rst describes it as "A json-schema used to
> > match nodes for applying the schema" but I'm a bit confused. In my case, should
> > it be "cpus" node? Is there any code/tools that uses this property, so that I
> > can have a better understanding?
> 
> Usually we expect matching by compatible, but it does not seem suitable
> here because it is not related to any specific device, right? That is
> the problem with all this DT-reuse-for-virtual-stuff work. It just does
> not follow usual expectations and guidelines - you do not describe a device.

Thank you for the reply.

I'm a bit confused on your "do not describe a device".
I think VM is also a device, it's just a virtual device, but I don't see much
difference of the virtual and physical device from DT point of view, possibly I
missed some point.
 
> 
> You can still match by nodes. See all top-level bindings.

After checking the code at
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/validator.py,
seems the 'select' is translated to 'if'/'then'.

Do you have any example of "top-level bindings"? I tried to check binding for
enable-methods like arm/cpu-enable-method/nuvoton,npcm750-smp or
cpu/idle-states.yaml, but they are either not schema file, or quite different.

I have been struggling on this device binding document for a while. I
reconsidered what this binding is for. This binding means, if the cpus node has
"enable-method" as "acpi-wakeup-mailbox", then the device should have property
"wakeup-mailbox-addr" with uint64 type.

In that case, I'm considering to set the "select" to be true so that it will
apply to any potential device, and add if/then keyword to check the
enable-method. But seems it does not work and I'm still trying to figure out the
reason (I'm new to the json/json schema and is still learning).

I received followed error:
cpus: '#address-cells', '#size-cells', 'cpu@0', 'enable-method' do not match any of the regexes: 'pinctrl-[0-9]+'
        from schema $id: http://devicetree.org/schemas/x86/wakeup.yaml#
cpu@0: 'device_type', 'reg' do not match any of the regexes: 'pinctrl-[0-9]+'
        from schema $id: http://devicetree.org/schemas/x86/wakeup.yaml#

With the followed yaml file (I delete some description).

$ cat Documentation/devicetree/bindings/x86/wakeup.yaml
# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
# Copyright (C) 2024 Intel Corporation
%YAML 1.2
---
$id: http://devicetree.org/schemas/x86/wakeup.yaml#
$schema: http://devicetree.org/meta-schemas/core.yaml#

title: x86 mailbox wakeup
maintainers:
  - Yunhong Jiang <yunhong.jiang@linux.intel.com>

description: |
  ......
  Removed to save space.

properties:
  wakeup-mailbox-addr:
    $ref: /schemas/types.yaml#/definitions/uint32
    description: |
      ......
      Removed to save space.

select: true

if:
  properties:
    enable-method:
      contains:
        const: acpi-wakeup-mailbox
  required:
    - enable-method

then:
  required:
    - wakeup-mailbox-addr

additionalProperties: false

examples:
  - |
    cpus {
      #address-cells = <1>;
      #size-cells = <0>;
      enable-method = "acpi-wakeup-mailbox";
      wakeup-mailbox-addr = <0x1c000500>;
      cpu@0 {
        device_type = "cpu";
        reg = <0x1>;
      };
    };
...

> 
> > 
> > For your "validated by the tools", can you please share the tools you used to
> > validate the schema? I used "make dt_binding_check" per the
> > submitting-patches.rst but I think your comments is about another tool.
> 
> See writing-schema document.
Yes, I figured out in the end that the validate tools means the dt-schema tools.

Thank you
--jyh

> 
> 
> Best regards,
> Krzysztof
> 
> 

Re: [PATCH v2 2/9] dt-bindings: x86: Add a binding for x86 wakeup mailbox

[Yunhong Jiang]

On Thu, Sep 19, 2024 at 12:19:01PM -0700, Yunhong Jiang wrote:
> On Mon, Sep 16, 2024 at 10:56:38AM +0200, Krzysztof Kozlowski wrote:
> > On 10/09/2024 08:13, Yunhong Jiang wrote:
> > > On Tue, Aug 27, 2024 at 01:45:49PM -0700, Yunhong Jiang wrote:
> > >> On Sun, Aug 25, 2024 at 09:10:01AM +0200, Krzysztof Kozlowski wrote:
> > >>> On Fri, Aug 23, 2024 at 04:23:20PM -0700, Yunhong Jiang wrote:
> > >>>> Add the binding to use mailbox wakeup mechanism to bringup APs.
> > >>>>
> > >>>> Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
> > >>>> ---
> > >>>>  .../devicetree/bindings/x86/wakeup.yaml       | 64 +++++++++++++++++++
> > >>>>  1 file changed, 64 insertions(+)
> > >>>>  create mode 100644 Documentation/devicetree/bindings/x86/wakeup.yaml
> > >>>>
> > >>>> diff --git a/Documentation/devicetree/bindings/x86/wakeup.yaml b/Documentation/devicetree/bindings/x86/wakeup.yaml
> > >>>> new file mode 100644
> > >>>> index 000000000000..cb84e2756bca
> > >>>> --- /dev/null
> > >>>> +++ b/Documentation/devicetree/bindings/x86/wakeup.yaml
> > >>>> @@ -0,0 +1,64 @@
> > >>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > >>>> +# Copyright (C) 2024 Intel Corporation
> > >>>> +%YAML 1.2
> > >>>> +---
> > >>>> +$id: http://devicetree.org/schemas/x86/wakeup.yaml#
> > >>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > >>>> +
> > >>>> +title: x86 mailbox wakeup
> > >>>> +maintainers:
> > >>>> +  - Yunhong Jiang <yunhong.jiang@linux.intel.com>
> > >>>> +
> > >>>> +description: |
> > >>>> +  The x86 mailbox wakeup mechanism defines a mechanism to let the bootstrap
> > >>>> +  processor (BSP) to wake up application processors (APs) through a wakeup
> > >>>> +  mailbox.
> > >>>> +
> > >>>> +  The "wakeup-mailbox-addr" property specifies the wakeup mailbox address. The
> > >>>> +  wakeup mailbox is a 4K-aligned 4K-size memory block allocated in the reserved
> > >>>> +  memory.
> > >>>> +
> > >>>> +  The wakeup mailbox structure is defined as follows.
> > >>>> +
> > >>>> +    uint16_t command;
> > >>>> +    uint16_t reserved;
> > >>>> +    uint32_t apic_id;
> > >>>> +    uint64_t wakeup_vector;
> > >>>> +    uint8_t  reservedForOs[2032];
> > >>>> +
> > >>>> +  The memory after reservedForOs field is reserved and OS should not touch it.
> > >>>> +
> > >>>> +  To wakes up a AP, the BSP prepares the wakeup routine, fills the wakeup
> > >>>> +  routine's address into the wakeup_vector field, fill the apic_id field with
> > >>>> +  the target AP's APIC_ID, and write 1 to the command field. After receiving the
> > >>>> +  wakeup command, the target AP will jump to the wakeup routine.
> > >>>> +
> > >>>> +  For each AP, the mailbox can be used only once for the wakeup command. After
> > >>>> +  the AP jumps to the wakeup routine, the mailbox will no longer be checked by
> > >>>> +  this AP.
> > >>>> +
> > >>>> +  The wakeup mailbox structure and the wakeup process is the same as
> > >>>> +  the Multiprocessor Wakeup Mailbox Structure defined in ACPI spec version 6.5,
> > >>>> +  section 5.2.12.19 [1].
> > >>>> +
> > >>>> +  References:
> > >>>> +
> > >>>> +  [1] https://uefi.org/specs/ACPI/6.5/05_ACPI_Software_Programming_Model.html
> > >>>> +
> > >>>> +select: false
> > >>>
> > >>> This schema is still a no-op because of this false.
> > >>>
> > >>> What is the point of defining one property if it is not placed anywhere?
> > >>> Every device node can have it? Seems wrong...
> > >>>
> > >>> You need to come with proper schema. Lack of an example is another thing
> > >>> - this cannot be even validated by the tools. 
> > >>>
> > >>> Best regards,
> > >>> Krzysztof
> > > 
> > > Hi, Krzysztof, I'm working to address your comments and have some questions.
> > > Hope to get help/guide from your side.
> > > 
> > > For the select, the writing-schema.rst describes it as "A json-schema used to
> > > match nodes for applying the schema" but I'm a bit confused. In my case, should
> > > it be "cpus" node? Is there any code/tools that uses this property, so that I
> > > can have a better understanding?
> > 
> > Usually we expect matching by compatible, but it does not seem suitable
> > here because it is not related to any specific device, right? That is
> > the problem with all this DT-reuse-for-virtual-stuff work. It just does
> > not follow usual expectations and guidelines - you do not describe a device.
> 
> Thank you for the reply.
> 
> I'm a bit confused on your "do not describe a device".
> I think VM is also a device, it's just a virtual device, but I don't see much
> difference of the virtual and physical device from DT point of view, possibly I
> missed some point.
>  
> > 
> > You can still match by nodes. See all top-level bindings.
> 
> After checking the code at
> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/validator.py,
> seems the 'select' is translated to 'if'/'then'.
> 
> Do you have any example of "top-level bindings"? I tried to check binding for
> enable-methods like arm/cpu-enable-method/nuvoton,npcm750-smp or
> cpu/idle-states.yaml, but they are either not schema file, or quite different.
> 
> I have been struggling on this device binding document for a while. I
> reconsidered what this binding is for. This binding means, if the cpus node has
> "enable-method" as "acpi-wakeup-mailbox", then the device should have property
> "wakeup-mailbox-addr" with uint64 type.
> 
> In that case, I'm considering to set the "select" to be true so that it will
> apply to any potential device, and add if/then keyword to check the
> enable-method. But seems it does not work and I'm still trying to figure out the
> reason (I'm new to the json/json schema and is still learning).
> 
> I received followed error:
> cpus: '#address-cells', '#size-cells', 'cpu@0', 'enable-method' do not match any of the regexes: 'pinctrl-[0-9]+'
>         from schema $id: http://devicetree.org/schemas/x86/wakeup.yaml#
> cpu@0: 'device_type', 'reg' do not match any of the regexes: 'pinctrl-[0-9]+'
>         from schema $id: http://devicetree.org/schemas/x86/wakeup.yaml#
> 
> With the followed yaml file (I delete some description).
> 
> $ cat Documentation/devicetree/bindings/x86/wakeup.yaml
> # SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> # Copyright (C) 2024 Intel Corporation
> %YAML 1.2
> ---
> $id: http://devicetree.org/schemas/x86/wakeup.yaml#
> $schema: http://devicetree.org/meta-schemas/core.yaml#
> 
> title: x86 mailbox wakeup
> maintainers:
>   - Yunhong Jiang <yunhong.jiang@linux.intel.com>
> 
> description: |
>   ......
>   Removed to save space.
> 
> properties:
>   wakeup-mailbox-addr:
>     $ref: /schemas/types.yaml#/definitions/uint32
>     description: |
>       ......
>       Removed to save space.
> 
> select: true
> 
> if:
>   properties:
>     enable-method:
>       contains:
>         const: acpi-wakeup-mailbox
>   required:
>     - enable-method
> 
> then:
>   required:
>     - wakeup-mailbox-addr
> 
> additionalProperties: false
> 
> examples:
>   - |
>     cpus {
>       #address-cells = <1>;
>       #size-cells = <0>;
>       enable-method = "acpi-wakeup-mailbox";
>       wakeup-mailbox-addr = <0x1c000500>;
>       cpu@0 {
>         device_type = "cpu";
>         reg = <0x1>;
>       };
>     };
> ...
> 
> > 
> > > 
> > > For your "validated by the tools", can you please share the tools you used to
> > > validate the schema? I used "make dt_binding_check" per the
> > > submitting-patches.rst but I think your comments is about another tool.
> > 
> > See writing-schema document.
> Yes, I figured out in the end that the validate tools means the dt-schema tools.
> 
> Thank you
> --jyh

One thing just come to my mind. The
Documentation/devicetree/bindings/x86/wakeup.yaml only adds one property,
wakeup-mailbox-addr, to the cpus node, and it does not specify any new object.
Per my understanding, the json schema file normally specifies a new object.
Is my change a supported scenario in current Documentation/devicetree?

I checked similar scenario like arm/cpu-enable-method/nuvoton and
npcm750-smp/cpu-enable-method/al,alpine-smp, they don't present as a json
schema file. Wonder if it's because the same reason.

> 
> > 
> > 
> > Best regards,
> > Krzysztof
> > 
> > 
> 

Re: [PATCH v2 2/9] dt-bindings: x86: Add a binding for x86 wakeup mailbox

[Krzysztof Kozlowski]

On 20/09/2024 00:15, Yunhong Jiang wrote:
>>>
>>>>
>>>> For your "validated by the tools", can you please share the tools you used to
>>>> validate the schema? I used "make dt_binding_check" per the
>>>> submitting-patches.rst but I think your comments is about another tool.
>>>
>>> See writing-schema document.
>> Yes, I figured out in the end that the validate tools means the dt-schema tools.
>>
>> Thank you
>> --jyh
> 
> One thing just come to my mind. The
> Documentation/devicetree/bindings/x86/wakeup.yaml only adds one property,
> wakeup-mailbox-addr, to the cpus node, and it does not specify any new object.
> Per my understanding, the json schema file normally specifies a new object.
> Is my change a supported scenario in current Documentation/devicetree?

I think that's not related. You can add properties per specific devices,
but (as I said) you do not have a compatible. Compatible represents a
specific device. You do not have it. Look at the cpus.yaml schema.

> 
> I checked similar scenario like arm/cpu-enable-method/nuvoton and
> npcm750-smp/cpu-enable-method/al,alpine-smp, they don't present as a json
> schema file. Wonder if it's because the same reason.

Rather no one cares about that hardware.


Best regards,
Krzysztof

Re: [PATCH v2 2/9] dt-bindings: x86: Add a binding for x86 wakeup mailbox

[Krzysztof Kozlowski]

On 19/09/2024 21:19, Yunhong Jiang wrote:
> On Mon, Sep 16, 2024 at 10:56:38AM +0200, Krzysztof Kozlowski wrote:
>> On 10/09/2024 08:13, Yunhong Jiang wrote:
>>> On Tue, Aug 27, 2024 at 01:45:49PM -0700, Yunhong Jiang wrote:
>>>> On Sun, Aug 25, 2024 at 09:10:01AM +0200, Krzysztof Kozlowski wrote:
>>>>> On Fri, Aug 23, 2024 at 04:23:20PM -0700, Yunhong Jiang wrote:
>>>>>> Add the binding to use mailbox wakeup mechanism to bringup APs.
>>>>>>
>>>>>> Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
>>>>>> ---
>>>>>>  .../devicetree/bindings/x86/wakeup.yaml       | 64 +++++++++++++++++++
>>>>>>  1 file changed, 64 insertions(+)
>>>>>>  create mode 100644 Documentation/devicetree/bindings/x86/wakeup.yaml
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/x86/wakeup.yaml b/Documentation/devicetree/bindings/x86/wakeup.yaml
>>>>>> new file mode 100644
>>>>>> index 000000000000..cb84e2756bca
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/x86/wakeup.yaml
>>>>>> @@ -0,0 +1,64 @@
>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>>> +# Copyright (C) 2024 Intel Corporation
>>>>>> +%YAML 1.2
>>>>>> +---
>>>>>> +$id: http://devicetree.org/schemas/x86/wakeup.yaml#
>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>> +
>>>>>> +title: x86 mailbox wakeup
>>>>>> +maintainers:
>>>>>> +  - Yunhong Jiang <yunhong.jiang@linux.intel.com>
>>>>>> +
>>>>>> +description: |
>>>>>> +  The x86 mailbox wakeup mechanism defines a mechanism to let the bootstrap
>>>>>> +  processor (BSP) to wake up application processors (APs) through a wakeup
>>>>>> +  mailbox.
>>>>>> +
>>>>>> +  The "wakeup-mailbox-addr" property specifies the wakeup mailbox address. The
>>>>>> +  wakeup mailbox is a 4K-aligned 4K-size memory block allocated in the reserved
>>>>>> +  memory.
>>>>>> +
>>>>>> +  The wakeup mailbox structure is defined as follows.
>>>>>> +
>>>>>> +    uint16_t command;
>>>>>> +    uint16_t reserved;
>>>>>> +    uint32_t apic_id;
>>>>>> +    uint64_t wakeup_vector;
>>>>>> +    uint8_t  reservedForOs[2032];
>>>>>> +
>>>>>> +  The memory after reservedForOs field is reserved and OS should not touch it.
>>>>>> +
>>>>>> +  To wakes up a AP, the BSP prepares the wakeup routine, fills the wakeup
>>>>>> +  routine's address into the wakeup_vector field, fill the apic_id field with
>>>>>> +  the target AP's APIC_ID, and write 1 to the command field. After receiving the
>>>>>> +  wakeup command, the target AP will jump to the wakeup routine.
>>>>>> +
>>>>>> +  For each AP, the mailbox can be used only once for the wakeup command. After
>>>>>> +  the AP jumps to the wakeup routine, the mailbox will no longer be checked by
>>>>>> +  this AP.
>>>>>> +
>>>>>> +  The wakeup mailbox structure and the wakeup process is the same as
>>>>>> +  the Multiprocessor Wakeup Mailbox Structure defined in ACPI spec version 6.5,
>>>>>> +  section 5.2.12.19 [1].
>>>>>> +
>>>>>> +  References:
>>>>>> +
>>>>>> +  [1] https://uefi.org/specs/ACPI/6.5/05_ACPI_Software_Programming_Model.html
>>>>>> +
>>>>>> +select: false
>>>>>
>>>>> This schema is still a no-op because of this false.
>>>>>
>>>>> What is the point of defining one property if it is not placed anywhere?
>>>>> Every device node can have it? Seems wrong...
>>>>>
>>>>> You need to come with proper schema. Lack of an example is another thing
>>>>> - this cannot be even validated by the tools. 
>>>>>
>>>>> Best regards,
>>>>> Krzysztof
>>>
>>> Hi, Krzysztof, I'm working to address your comments and have some questions.
>>> Hope to get help/guide from your side.
>>>
>>> For the select, the writing-schema.rst describes it as "A json-schema used to
>>> match nodes for applying the schema" but I'm a bit confused. In my case, should
>>> it be "cpus" node? Is there any code/tools that uses this property, so that I
>>> can have a better understanding?
>>
>> Usually we expect matching by compatible, but it does not seem suitable
>> here because it is not related to any specific device, right? That is
>> the problem with all this DT-reuse-for-virtual-stuff work. It just does
>> not follow usual expectations and guidelines - you do not describe a device.
> 
> Thank you for the reply.
> 
> I'm a bit confused on your "do not describe a device".
> I think VM is also a device, it's just a virtual device, but I don't see much
> difference of the virtual and physical device from DT point of view, possibly I
> missed some point.

VM is purely a software construct, so it is not a device. But regardless
of terminology, you did not describe here VM or its part, either.

>  
>>
>> You can still match by nodes. See all top-level bindings.
> 
> After checking the code at
> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/validator.py,
> seems the 'select' is translated to 'if'/'then'.
> 
> Do you have any example of "top-level bindings"? I tried to check binding for
> enable-methods like arm/cpu-enable-method/nuvoton,npcm750-smp or
> cpu/idle-states.yaml, but they are either not schema file, or quite different.

Most of board schemas, so for example in arm directory.

> 
> I have been struggling on this device binding document for a while. I
> reconsidered what this binding is for. This binding means, if the cpus node has
> "enable-method" as "acpi-wakeup-mailbox", then the device should have property
> "wakeup-mailbox-addr" with uint64 type.
> 
> In that case, I'm considering to set the "select" to be true so that it will
> apply to any potential device, and add if/then keyword to check the
> enable-method. But seems it does not work and I'm still trying to figure out the
> reason (I'm new to the json/json schema and is still learning).
> 
> I received followed error:
> cpus: '#address-cells', '#size-cells', 'cpu@0', 'enable-method' do not match any of the regexes: 'pinctrl-[0-9]+'
>         from schema $id: http://devicetree.org/schemas/x86/wakeup.yaml#
> cpu@0: 'device_type', 'reg' do not match any of the regexes: 'pinctrl-[0-9]+'
>         from schema $id: http://devicetree.org/schemas/x86/wakeup.yaml#
> 
> With the followed yaml file (I delete some description).
> 
> $ cat Documentation/devicetree/bindings/x86/wakeup.yaml
> # SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> # Copyright (C) 2024 Intel Corporation
> %YAML 1.2
> ---
> $id: http://devicetree.org/schemas/x86/wakeup.yaml#
> $schema: http://devicetree.org/meta-schemas/core.yaml#
> 
> title: x86 mailbox wakeup
> maintainers:
>   - Yunhong Jiang <yunhong.jiang@linux.intel.com>
> 
> description: |
>   ......
>   Removed to save space.
> 
> properties:
>   wakeup-mailbox-addr:
>     $ref: /schemas/types.yaml#/definitions/uint32
>     description: |
>       ......
>       Removed to save space.
> 
> select: true
> 
> if:
>   properties:
>     enable-method:
>       contains:
>         const: acpi-wakeup-mailbox
>   required:
>     - enable-method

enable-method is part of CPUs, so you probably should match the CPUs...
I am not sure, I don't have the big picture here.

Maybe if companies want to push more of bindings for purely virtual
systems, then they should first get involved more, instead of relying on
us. Provide reviews for your virtual stuff, provide guidance. There is
resistance in accepting bindings for such cases for a reason - I don't
even know what exactly is this and judging/reviewing based on my
practices will no be accurate.


> 
> then:
>   required:
>     - wakeup-mailbox-addr
> 
> additionalProperties: false
> 
> examples:
>   - |
>     cpus {
>       #address-cells = <1>;
>       #size-cells = <0>;
>       enable-method = "acpi-wakeup-mailbox";
>       wakeup-mailbox-addr = <0x1c000500>;
>       cpu@0 {
>         device_type = "cpu";
>         reg = <0x1>;
>       };
>     };
> ...
> 
>>
>>>
>>> For your "validated by the tools", can you please share the tools you used to
>>> validate the schema? I used "make dt_binding_check" per the
>>> submitting-patches.rst but I think your comments is about another tool.
>>
>> See writing-schema document.
> Yes, I figured out in the end that the validate tools means the dt-schema tools.
> 
> Thank you
> --jyh
> 
>>
>>
>> Best regards,
>> Krzysztof
>>
>>

Best regards,
Krzysztof

Re: [PATCH v2 2/9] dt-bindings: x86: Add a binding for x86 wakeup mailbox

[Ricardo Neri]

On Fri, Sep 20, 2024 at 01:15:41PM +0200, Krzysztof Kozlowski wrote:

[...]
 
> enable-method is part of CPUs, so you probably should match the CPUs...
> I am not sure, I don't have the big picture here.
> 
> Maybe if companies want to push more of bindings for purely virtual
> systems, then they should first get involved more, instead of relying on
> us. Provide reviews for your virtual stuff, provide guidance. There is
> resistance in accepting bindings for such cases for a reason - I don't
> even know what exactly is this and judging/reviewing based on my
> practices will no be accurate.

Hi Krzysztof,

I am taking over this work from Yunhong.

First of all, I apologize for the late reply. I will make sure
communications are timely in the future.

Our goal is to describe in the device tree a mechanism or artifact to boot
secondary CPUs.

In our setup, the firmware puts secondary CPUs to monitor a memory location
(i.e., the wakeup mailbox) while spinning. From the boot CPU, the OS writes
in the mailbox the wakeup vector and the ID of the secondary CPU it wants
to boot. When a secondary CPU sees its own ID it will jump to the wakeup
vector.

This is similar to the spin-table described in the Device Tree
specification. The key difference is that with the spin-table CPUs spin
until a non-zero value is written in `cpu-release-addr`. The wakeup mailbox
uses CPU IDs.

You raised the issue of the lack of a `compatible` property, and the fact
that we are not describing an actual device.

I took your suggestion of matching by node and I came up with the binding
below. I see these advantages in this approach:

  * I define a new node with a `compatible` property.
  * There is precedent: the psci node. In the `cpus` node, each cpu@n has
    an `enable-method` property that specify `psci`.
  * The mailbox is a device as it is located in a reserved memory region.
    This true regardless of the device tree describing bare-metal or
    virtualized machines.

Thanks in advance for your feedback!

Best,
Ricardo

(only the relevant sections of the binding are shown for brevity)

properties:
  $nodename:
    const: wakeup-mailbox

  compatible:
    const: x86,wakeup-mailbox

  mailbox-addr:
    $ref: /schemas/types.yaml#/definitions/uint64

required:
  - compatible
  - mailbox-addr

additionalProperties: false

examples:
  - |
    wakeup-mailbox {
      compatible = "x86,wakeup-mailbox";
      mailbox-addr = <0 0x1c000500>;
    };

    cpus {
        #address-cells = <1>;
        #size-cells = <0>;

        cpu@0 {
            device_type = "cpu";
            reg = <0x00>;
            enable-method = "wakeup-mailbox";
        };
    };
...

Re: [PATCH v2 2/9] dt-bindings: x86: Add a binding for x86 wakeup mailbox

[Krzysztof Kozlowski]

On 03/03/2025 23:21, Ricardo Neri wrote:
> On Fri, Sep 20, 2024 at 01:15:41PM +0200, Krzysztof Kozlowski wrote:
> 
> [...]
>  
>> enable-method is part of CPUs, so you probably should match the CPUs...
>> I am not sure, I don't have the big picture here.
>>
>> Maybe if companies want to push more of bindings for purely virtual
>> systems, then they should first get involved more, instead of relying on
>> us. Provide reviews for your virtual stuff, provide guidance. There is
>> resistance in accepting bindings for such cases for a reason - I don't
>> even know what exactly is this and judging/reviewing based on my
>> practices will no be accurate.
> 
> Hi Krzysztof,
> 
> I am taking over this work from Yunhong.
> 
> First of all, I apologize for the late reply. I will make sure
> communications are timely in the future.
> 
> Our goal is to describe in the device tree a mechanism or artifact to boot
> secondary CPUs.
> 
> In our setup, the firmware puts secondary CPUs to monitor a memory location
> (i.e., the wakeup mailbox) while spinning. From the boot CPU, the OS writes
> in the mailbox the wakeup vector and the ID of the secondary CPU it wants
> to boot. When a secondary CPU sees its own ID it will jump to the wakeup
> vector.
> 
> This is similar to the spin-table described in the Device Tree
> specification. The key difference is that with the spin-table CPUs spin
> until a non-zero value is written in `cpu-release-addr`. The wakeup mailbox
> uses CPU IDs.
> 
> You raised the issue of the lack of a `compatible` property, and the fact
> that we are not describing an actual device.
> 
> I took your suggestion of matching by node and I came up with the binding
> below. I see these advantages in this approach:
> 
>   * I define a new node with a `compatible` property.
>   * There is precedent: the psci node. In the `cpus` node, each cpu@n has

psci is a standard. If you are documenting here a standard, clearly
express it and provide reference to the specification.


>     an `enable-method` property that specify `psci`.
>   * The mailbox is a device as it is located in a reserved memory region.
>     This true regardless of the device tree describing bare-metal or
>     virtualized machines.
> 
> Thanks in advance for your feedback!
> 
> Best,
> Ricardo
> 
> (only the relevant sections of the binding are shown for brevity)
> 
> properties:
>   $nodename:
>     const: wakeup-mailbox
> 
>   compatible:
>     const: x86,wakeup-mailbox

You need vendor prefix for this particular device. If I pointed out lack
of device and specific compatible, then adding random compatible does
not solve it. I understand it solves for you, but not from the bindings
point of view.

> 
>   mailbox-addr:
>     $ref: /schemas/types.yaml#/definitions/uint64

So is this some sort of reserved memory? Mailbox needs mbox-cells, so
maybe that's not mailbox.



Best regards,
Krzysztof

Re: [PATCH v2 2/9] dt-bindings: x86: Add a binding for x86 wakeup mailbox

[Ricardo Neri]

On Tue, Mar 11, 2025 at 11:01:28AM +0100, Krzysztof Kozlowski wrote:
> On 03/03/2025 23:21, Ricardo Neri wrote:
> > On Fri, Sep 20, 2024 at 01:15:41PM +0200, Krzysztof Kozlowski wrote:
> > 
> > [...]
> >  
> >> enable-method is part of CPUs, so you probably should match the CPUs...
> >> I am not sure, I don't have the big picture here.
> >>
> >> Maybe if companies want to push more of bindings for purely virtual
> >> systems, then they should first get involved more, instead of relying on
> >> us. Provide reviews for your virtual stuff, provide guidance. There is
> >> resistance in accepting bindings for such cases for a reason - I don't
> >> even know what exactly is this and judging/reviewing based on my
> >> practices will no be accurate.
> > 
> > Hi Krzysztof,
> > 
> > I am taking over this work from Yunhong.
> > 
> > First of all, I apologize for the late reply. I will make sure
> > communications are timely in the future.
> > 
> > Our goal is to describe in the device tree a mechanism or artifact to boot
> > secondary CPUs.
> > 
> > In our setup, the firmware puts secondary CPUs to monitor a memory location
> > (i.e., the wakeup mailbox) while spinning. From the boot CPU, the OS writes
> > in the mailbox the wakeup vector and the ID of the secondary CPU it wants
> > to boot. When a secondary CPU sees its own ID it will jump to the wakeup
> > vector.
> > 
> > This is similar to the spin-table described in the Device Tree
> > specification. The key difference is that with the spin-table CPUs spin
> > until a non-zero value is written in `cpu-release-addr`. The wakeup mailbox
> > uses CPU IDs.
> > 
> > You raised the issue of the lack of a `compatible` property, and the fact
> > that we are not describing an actual device.
> > 
> > I took your suggestion of matching by node and I came up with the binding
> > below. I see these advantages in this approach:
> > 
> >   * I define a new node with a `compatible` property.
> >   * There is precedent: the psci node. In the `cpus` node, each cpu@n has
> 

Thanks for your feedback!

> psci is a standard. If you are documenting here a standard, clearly
> express it and provide reference to the specification.

It is not really a standard, but this mailbox behaves indentically to the
wakeup mailbox described in the ACPI spec [1]. I am happy reference the
spec in the documentation of the binding... or describe in full the
mechanism of mailbox without referring to ACPI. You had indicated you don't
care about what ACPI does [2].

In a nutshell, the wakeup mailbox is similar to the spin table used in ARM
boards: it is reserved memory region that secondary CPUs monitor while
spinning.

> 
> 
> >     an `enable-method` property that specify `psci`.
> >   * The mailbox is a device as it is located in a reserved memory region.
> >     This true regardless of the device tree describing bare-metal or
> >     virtualized machines.
> > 
> > Thanks in advance for your feedback!
> > 
> > Best,
> > Ricardo
> > 
> > (only the relevant sections of the binding are shown for brevity)
> > 
> > properties:
> >   $nodename:
> >     const: wakeup-mailbox
> > 
> >   compatible:
> >     const: x86,wakeup-mailbox
> 
> You need vendor prefix for this particular device. If I pointed out lack
> of device and specific compatible, then adding random compatible does
> not solve it. I understand it solves for you, but not from the bindings
> point of view.

I see. Platform firmware will implement the mailbox. It would not be any
specific hardware from Intel. Perhaps `intel,wakeup-mailbox`?

> 
> > 
> >   mailbox-addr:
> >     $ref: /schemas/types.yaml#/definitions/uint64
> 
> So is this some sort of reserved memory?

Yes, the mailbox is located in reserved memory.

> Mailbox needs mbox-cells, so
> maybe that's not mailbox.

Your comment got me to look under Documentation/devicetree/bindings/mailbox.
Maybe I am ovethinking this and I can describe the mailbox as others
vendors do (see below).

Thanks and BR,
Ricardo

[1]. https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/05_ACPI_Software_Programming_Model/ACPI_Software_Programming_Model.html#multiprocessor-wakeup-structure
if that is OK.
[2]. https://lore.kernel.org/lkml/624e1985-7dd2-4abe-a918-78cb43556967@kernel.oirg

description: |
  [Removed for brevity]

properties:
  compatible:
    const: intel,wakeup-mailbox

  reg:
    maxItems: 1

  '#mbox-cells':
    const: 1

required:
  - compatible
  - reg

additionalProperties: false

examples:
  - |
    wakeupmailbox: mailbox@1c000500 {
      compatible = "intel,wakeup-mailbox";
      reg = <0x1c000500 0x100>;
      #mbox-cells = <1>;
    };

    cpus {
        #address-cells = <1>;
        #size-cells = <0>;

        cpu@0 {
            device_type = "cpu";
            reg = <0x00>;
            enable-method = "wakeup-mailbox";
        };
    };
...

Re: [PATCH v2 2/9] dt-bindings: x86: Add a binding for x86 wakeup mailbox

[Krzysztof Kozlowski]

On 12/03/2025 06:51, Ricardo Neri wrote:
> On Tue, Mar 11, 2025 at 11:01:28AM +0100, Krzysztof Kozlowski wrote:
>> On 03/03/2025 23:21, Ricardo Neri wrote:
>>> On Fri, Sep 20, 2024 at 01:15:41PM +0200, Krzysztof Kozlowski wrote:
>>>
>>> [...]
>>>  
>>>> enable-method is part of CPUs, so you probably should match the CPUs...
>>>> I am not sure, I don't have the big picture here.
>>>>
>>>> Maybe if companies want to push more of bindings for purely virtual
>>>> systems, then they should first get involved more, instead of relying on
>>>> us. Provide reviews for your virtual stuff, provide guidance. There is
>>>> resistance in accepting bindings for such cases for a reason - I don't
>>>> even know what exactly is this and judging/reviewing based on my
>>>> practices will no be accurate.
>>>
>>> Hi Krzysztof,
>>>
>>> I am taking over this work from Yunhong.
>>>
>>> First of all, I apologize for the late reply. I will make sure
>>> communications are timely in the future.
>>>
>>> Our goal is to describe in the device tree a mechanism or artifact to boot
>>> secondary CPUs.
>>>
>>> In our setup, the firmware puts secondary CPUs to monitor a memory location
>>> (i.e., the wakeup mailbox) while spinning. From the boot CPU, the OS writes
>>> in the mailbox the wakeup vector and the ID of the secondary CPU it wants
>>> to boot. When a secondary CPU sees its own ID it will jump to the wakeup
>>> vector.
>>>
>>> This is similar to the spin-table described in the Device Tree
>>> specification. The key difference is that with the spin-table CPUs spin
>>> until a non-zero value is written in `cpu-release-addr`. The wakeup mailbox
>>> uses CPU IDs.
>>>
>>> You raised the issue of the lack of a `compatible` property, and the fact
>>> that we are not describing an actual device.
>>>
>>> I took your suggestion of matching by node and I came up with the binding
>>> below. I see these advantages in this approach:
>>>
>>>   * I define a new node with a `compatible` property.
>>>   * There is precedent: the psci node. In the `cpus` node, each cpu@n has
>>
> 
> Thanks for your feedback!
> 
>> psci is a standard. If you are documenting here a standard, clearly
>> express it and provide reference to the specification.
> 
> It is not really a standard, but this mailbox behaves indentically to the
> wakeup mailbox described in the ACPI spec [1]. I am happy reference the
> spec in the documentation of the binding... or describe in full the
> mechanism of mailbox without referring to ACPI. You had indicated you don't
> care about what ACPI does [2].

Behaving like ACPI and implementing a spec are two different things. The
question is whether you need to implement it like that and I believe
answer is: no.

> 
> In a nutshell, the wakeup mailbox is similar to the spin table used in ARM
> boards: it is reserved memory region that secondary CPUs monitor while
> spinning.
> 
>>
>>
>>>     an `enable-method` property that specify `psci`.
>>>   * The mailbox is a device as it is located in a reserved memory region.
>>>     This true regardless of the device tree describing bare-metal or
>>>     virtualized machines.
>>>
>>> Thanks in advance for your feedback!
>>>
>>> Best,
>>> Ricardo
>>>
>>> (only the relevant sections of the binding are shown for brevity)
>>>
>>> properties:
>>>   $nodename:
>>>     const: wakeup-mailbox
>>>
>>>   compatible:
>>>     const: x86,wakeup-mailbox
>>
>> You need vendor prefix for this particular device. If I pointed out lack
>> of device and specific compatible, then adding random compatible does
>> not solve it. I understand it solves for you, but not from the bindings
>> point of view.
> 
> I see. Platform firmware will implement the mailbox. It would not be any
> specific hardware from Intel. Perhaps `intel,wakeup-mailbox`?
> 
>>
>>>
>>>   mailbox-addr:
>>>     $ref: /schemas/types.yaml#/definitions/uint64
>>
>> So is this some sort of reserved memory?
> 
> Yes, the mailbox is located in reserved memory.


Then why reserved memory bindings are not working?

Anyway this was half a year ago. None of the emails are in my inbox.
None of the context is in my head.

It's the second or third email this month someone responds to my email
from 2024.

Frankly, that's a neat trick. I won't remember anything, but it would be
impolite to say just "no" without arguments. So now you will resend the
same code leading to the same discussions we had half a year ago. Or
ignoring that discussions.

I don't understand why this should be reviewers problem, so no, that's
just unfair.


Best regards,
Krzysztof

Re: [PATCH v2 2/9] dt-bindings: x86: Add a binding for x86 wakeup mailbox

[Ricardo Neri]

On Wed, Mar 19, 2025 at 08:52:37AM +0100, Krzysztof Kozlowski wrote:
> On 12/03/2025 06:51, Ricardo Neri wrote:
> > On Tue, Mar 11, 2025 at 11:01:28AM +0100, Krzysztof Kozlowski wrote:
> >> On 03/03/2025 23:21, Ricardo Neri wrote:
> >>> On Fri, Sep 20, 2024 at 01:15:41PM +0200, Krzysztof Kozlowski wrote:
> >>>
> >>> [...]
> >>>  
> >>>> enable-method is part of CPUs, so you probably should match the CPUs...
> >>>> I am not sure, I don't have the big picture here.
> >>>>
> >>>> Maybe if companies want to push more of bindings for purely virtual
> >>>> systems, then they should first get involved more, instead of relying on
> >>>> us. Provide reviews for your virtual stuff, provide guidance. There is
> >>>> resistance in accepting bindings for such cases for a reason - I don't
> >>>> even know what exactly is this and judging/reviewing based on my
> >>>> practices will no be accurate.
> >>>
> >>> Hi Krzysztof,
> >>>
> >>> I am taking over this work from Yunhong.
> >>>
> >>> First of all, I apologize for the late reply. I will make sure
> >>> communications are timely in the future.
> >>>
> >>> Our goal is to describe in the device tree a mechanism or artifact to boot
> >>> secondary CPUs.
> >>>
> >>> In our setup, the firmware puts secondary CPUs to monitor a memory location
> >>> (i.e., the wakeup mailbox) while spinning. From the boot CPU, the OS writes
> >>> in the mailbox the wakeup vector and the ID of the secondary CPU it wants
> >>> to boot. When a secondary CPU sees its own ID it will jump to the wakeup
> >>> vector.
> >>>
> >>> This is similar to the spin-table described in the Device Tree
> >>> specification. The key difference is that with the spin-table CPUs spin
> >>> until a non-zero value is written in `cpu-release-addr`. The wakeup mailbox
> >>> uses CPU IDs.
> >>>
> >>> You raised the issue of the lack of a `compatible` property, and the fact
> >>> that we are not describing an actual device.
> >>>
> >>> I took your suggestion of matching by node and I came up with the binding
> >>> below. I see these advantages in this approach:
> >>>
> >>>   * I define a new node with a `compatible` property.
> >>>   * There is precedent: the psci node. In the `cpus` node, each cpu@n has
> >>
> > 
> > Thanks for your feedback!
> > 
> >> psci is a standard. If you are documenting here a standard, clearly
> >> express it and provide reference to the specification.
> > 
> > It is not really a standard, but this mailbox behaves indentically to the
> > wakeup mailbox described in the ACPI spec [1]. I am happy reference the
> > spec in the documentation of the binding... or describe in full the
> > mechanism of mailbox without referring to ACPI. You had indicated you don't
> > care about what ACPI does [2].
> 
> Behaving like ACPI and implementing a spec are two different things. The
> question is whether you need to implement it like that and I believe
> answer is: no.
> 
> > 
> > In a nutshell, the wakeup mailbox is similar to the spin table used in ARM
> > boards: it is reserved memory region that secondary CPUs monitor while
> > spinning.
> > 
> >>
> >>
> >>>     an `enable-method` property that specify `psci`.
> >>>   * The mailbox is a device as it is located in a reserved memory region.
> >>>     This true regardless of the device tree describing bare-metal or
> >>>     virtualized machines.
> >>>
> >>> Thanks in advance for your feedback!
> >>>
> >>> Best,
> >>> Ricardo
> >>>
> >>> (only the relevant sections of the binding are shown for brevity)
> >>>
> >>> properties:
> >>>   $nodename:
> >>>     const: wakeup-mailbox
> >>>
> >>>   compatible:
> >>>     const: x86,wakeup-mailbox
> >>
> >> You need vendor prefix for this particular device. If I pointed out lack
> >> of device and specific compatible, then adding random compatible does
> >> not solve it. I understand it solves for you, but not from the bindings
> >> point of view.
> > 
> > I see. Platform firmware will implement the mailbox. It would not be any
> > specific hardware from Intel. Perhaps `intel,wakeup-mailbox`?
> > 
> >>
> >>>
> >>>   mailbox-addr:
> >>>     $ref: /schemas/types.yaml#/definitions/uint64
> >>
> >> So is this some sort of reserved memory?
> > 
> > Yes, the mailbox is located in reserved memory.
> 
> 
> Then why reserved memory bindings are not working?
> 
> Anyway this was half a year ago. None of the emails are in my inbox.
> None of the context is in my head.
> 
> It's the second or third email this month someone responds to my email
> from 2024.
> 
> Frankly, that's a neat trick. I won't remember anything, but it would be
> impolite to say just "no" without arguments. So now you will resend the
> same code leading to the same discussions we had half a year ago. Or
> ignoring that discussions.
> 
> I don't understand why this should be reviewers problem, so no, that's
> just unfair.
> 

Thank you again for your feedback. I will send a third version of the
patchset. This will give the full context. I will incorporate your feedback
in such version.

Thanks and BR,
Ricardo

[PATCH v2 3/9] x86/dt: Support the ACPI multiprocessor wakeup for device tree

[Yunhong Jiang]

When a TDX guest boots with the device tree instead of ACPI, it can
reuse the ACPI multiprocessor wakeup mechanism to wake up application
processors(AP), without introducing a new mechanism from scrach.

In the ACPI spec, two structures are defined to wake up the APs: the
multiprocessor wakeup structure and the multiprocessor wakeup mailbox
structure. The multiprocessor wakeup structure is passed to OS through a
Multiple APIC Description Table(MADT), one field specifying the physical
address of the multiprocessor wakeup mailbox structure. The OS sends
a message to firmware through the multiprocessor wakeup mailbox
structure, to bring up the APs.

In device tree environment, the multiprocessor wakeup structure is not
used, to reduce the dependency on the generic ACPI table. The
information defined in this structure is defined in the properties of
cpus node in the device tree. The "wakeup-mailbox-addr" property
specifies the physical address of the multiprocessor wakeup mailbox
structure. The OS will follow the ACPI spec to send the message to the
firmware to bring up the APs.

Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
---
 MAINTAINERS                        |  1 +
 arch/x86/Kconfig                   |  2 +-
 arch/x86/include/asm/acpi.h        |  1 -
 arch/x86/include/asm/madt_wakeup.h | 16 +++++++++++++
 arch/x86/kernel/madt_wakeup.c      | 38 ++++++++++++++++++++++++++++++
 5 files changed, 56 insertions(+), 2 deletions(-)
 create mode 100644 arch/x86/include/asm/madt_wakeup.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 5555a3bbac5f..900de6501eba 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -288,6 +288,7 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm
 F:	Documentation/ABI/testing/configfs-acpi
 F:	Documentation/ABI/testing/sysfs-bus-acpi
 F:	Documentation/firmware-guide/acpi/
+F:	arch/x86/include/asm/madt_wakeup.h
 F:	arch/x86/kernel/acpi/
 F:	arch/x86/kernel/madt_playdead.S
 F:	arch/x86/kernel/madt_wakeup.c
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index d422247b2882..dba46dd30049 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1123,7 +1123,7 @@ config X86_LOCAL_APIC
 config ACPI_MADT_WAKEUP
 	def_bool y
 	depends on X86_64
-	depends on ACPI
+	depends on ACPI || OF
 	depends on SMP
 	depends on X86_LOCAL_APIC
 
diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h
index 21bc53f5ed0c..0e082303ca26 100644
--- a/arch/x86/include/asm/acpi.h
+++ b/arch/x86/include/asm/acpi.h
@@ -83,7 +83,6 @@ union acpi_subtable_headers;
 int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
 			      const unsigned long end);
 
-void asm_acpi_mp_play_dead(u64 reset_vector, u64 pgd_pa);
 
 /*
  * Check if the CPU can handle C2 and deeper
diff --git a/arch/x86/include/asm/madt_wakeup.h b/arch/x86/include/asm/madt_wakeup.h
new file mode 100644
index 000000000000..a8cd50af581a
--- /dev/null
+++ b/arch/x86/include/asm/madt_wakeup.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __ASM_X86_MADT_WAKEUP_H
+#define __ASM_X86_MADT_WAKEUP_H
+
+void asm_acpi_mp_play_dead(u64 reset_vector, u64 pgd_pa);
+
+#if defined(CONFIG_OF) && defined(CONFIG_ACPI_MADT_WAKEUP)
+u64 dtb_parse_mp_wake(void);
+#else
+static inline u64 dtb_parse_mp_wake(void)
+{
+	return 0;
+}
+#endif
+
+#endif /* __ASM_X86_MADT_WAKEUP_H */
diff --git a/arch/x86/kernel/madt_wakeup.c b/arch/x86/kernel/madt_wakeup.c
index d5ef6215583b..7257e7484569 100644
--- a/arch/x86/kernel/madt_wakeup.c
+++ b/arch/x86/kernel/madt_wakeup.c
@@ -14,6 +14,8 @@
 #include <asm/nmi.h>
 #include <asm/processor.h>
 #include <asm/reboot.h>
+#include <asm/madt_wakeup.h>
+#include <asm/prom.h>
 
 /* Physical address of the Multiprocessor Wakeup Structure mailbox */
 static u64 acpi_mp_wake_mailbox_paddr __ro_after_init;
@@ -122,6 +124,7 @@ static int __init init_transition_pgtable(pgd_t *pgd)
 	return 0;
 }
 
+#ifdef CONFIG_ACPI
 static int __init acpi_mp_setup_reset(u64 reset_vector)
 {
 	struct x86_mapping_info info = {
@@ -168,6 +171,7 @@ static int __init acpi_mp_setup_reset(u64 reset_vector)
 
 	return 0;
 }
+#endif
 
 static int acpi_wakeup_cpu(u32 apicid, unsigned long start_ip)
 {
@@ -226,6 +230,7 @@ static int acpi_wakeup_cpu(u32 apicid, unsigned long start_ip)
 	return 0;
 }
 
+#ifdef CONFIG_ACPI
 static void acpi_mp_disable_offlining(struct acpi_madt_multiproc_wakeup *mp_wake)
 {
 	cpu_hotplug_disable_offlining();
@@ -290,3 +295,36 @@ int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
 
 	return 0;
 }
+#endif /* CONFIG_ACPI */
+
+#ifdef CONFIG_OF
+u64 __init dtb_parse_mp_wake(void)
+{
+	struct device_node *node;
+	u64 mailaddr = 0;
+
+	node = of_find_node_by_path("/cpus");
+	if (!node)
+		return 0;
+
+	if (of_property_match_string(node, "enable-method", "acpi-wakeup-mailbox") < 0) {
+		pr_err("No acpi wakeup mailbox enable-method\n");
+		goto done;
+	}
+
+	if (of_property_read_u64(node, "wakeup-mailbox-addr", &mailaddr)) {
+		pr_err("Invalid wakeup mailbox addr\n");
+		goto done;
+	}
+	acpi_mp_wake_mailbox_paddr = mailaddr;
+	pr_info("dt wakeup-mailbox: addr 0x%llx\n", mailaddr);
+
+	/* No support for the MADT reset vector yet */
+	cpu_hotplug_disable_offlining();
+	apic_update_callback(wakeup_secondary_cpu_64, acpi_wakeup_cpu);
+
+done:
+	of_node_put(node);
+	return mailaddr;
+}
+#endif /* CONFIG_OF */
-- 
2.25.1

RE: [PATCH v2 3/9] x86/dt: Support the ACPI multiprocessor wakeup for device tree

[Michael Kelley]

From: Yunhong Jiang <yunhong.jiang@linux.intel.com> Sent: Friday, August 23, 2024 4:23 PM
> 
> When a TDX guest boots with the device tree instead of ACPI, it can
> reuse the ACPI multiprocessor wakeup mechanism to wake up application
> processors(AP), without introducing a new mechanism from scrach.
> 
> In the ACPI spec, two structures are defined to wake up the APs: the
> multiprocessor wakeup structure and the multiprocessor wakeup mailbox
> structure. The multiprocessor wakeup structure is passed to OS through a
> Multiple APIC Description Table(MADT), one field specifying the physical
> address of the multiprocessor wakeup mailbox structure. The OS sends
> a message to firmware through the multiprocessor wakeup mailbox
> structure, to bring up the APs.
> 
> In device tree environment, the multiprocessor wakeup structure is not
> used, to reduce the dependency on the generic ACPI table. The
> information defined in this structure is defined in the properties of
> cpus node in the device tree. The "wakeup-mailbox-addr" property
> specifies the physical address of the multiprocessor wakeup mailbox
> structure. The OS will follow the ACPI spec to send the message to the
> firmware to bring up the APs.
> 
> Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
> ---
>  MAINTAINERS                        |  1 +
>  arch/x86/Kconfig                   |  2 +-
>  arch/x86/include/asm/acpi.h        |  1 -
>  arch/x86/include/asm/madt_wakeup.h | 16 +++++++++++++
>  arch/x86/kernel/madt_wakeup.c      | 38 ++++++++++++++++++++++++++++++
>  5 files changed, 56 insertions(+), 2 deletions(-)
>  create mode 100644 arch/x86/include/asm/madt_wakeup.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5555a3bbac5f..900de6501eba 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -288,6 +288,7 @@ T:	git
> git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm
>  F:	Documentation/ABI/testing/configfs-acpi
>  F:	Documentation/ABI/testing/sysfs-bus-acpi
>  F:	Documentation/firmware-guide/acpi/
> +F:	arch/x86/include/asm/madt_wakeup.h
>  F:	arch/x86/kernel/acpi/
>  F:	arch/x86/kernel/madt_playdead.S
>  F:	arch/x86/kernel/madt_wakeup.c
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index d422247b2882..dba46dd30049 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1123,7 +1123,7 @@ config X86_LOCAL_APIC
>  config ACPI_MADT_WAKEUP
>  	def_bool y
>  	depends on X86_64
> -	depends on ACPI
> +	depends on ACPI || OF
>  	depends on SMP
>  	depends on X86_LOCAL_APIC
> 
> diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h
> index 21bc53f5ed0c..0e082303ca26 100644
> --- a/arch/x86/include/asm/acpi.h
> +++ b/arch/x86/include/asm/acpi.h
> @@ -83,7 +83,6 @@ union acpi_subtable_headers;
>  int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
>  			      const unsigned long end);
> 
> -void asm_acpi_mp_play_dead(u64 reset_vector, u64 pgd_pa);
> 
>  /*
>   * Check if the CPU can handle C2 and deeper
> diff --git a/arch/x86/include/asm/madt_wakeup.h
> b/arch/x86/include/asm/madt_wakeup.h
> new file mode 100644
> index 000000000000..a8cd50af581a
> --- /dev/null
> +++ b/arch/x86/include/asm/madt_wakeup.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __ASM_X86_MADT_WAKEUP_H
> +#define __ASM_X86_MADT_WAKEUP_H
> +
> +void asm_acpi_mp_play_dead(u64 reset_vector, u64 pgd_pa);
> +
> +#if defined(CONFIG_OF) && defined(CONFIG_ACPI_MADT_WAKEUP)
> +u64 dtb_parse_mp_wake(void);
> +#else
> +static inline u64 dtb_parse_mp_wake(void)
> +{
> +	return 0;
> +}
> +#endif
> +
> +#endif /* __ASM_X86_MADT_WAKEUP_H */
> diff --git a/arch/x86/kernel/madt_wakeup.c b/arch/x86/kernel/madt_wakeup.c
> index d5ef6215583b..7257e7484569 100644
> --- a/arch/x86/kernel/madt_wakeup.c
> +++ b/arch/x86/kernel/madt_wakeup.c
> @@ -14,6 +14,8 @@
>  #include <asm/nmi.h>
>  #include <asm/processor.h>
>  #include <asm/reboot.h>
> +#include <asm/madt_wakeup.h>
> +#include <asm/prom.h>
> 
>  /* Physical address of the Multiprocessor Wakeup Structure mailbox */
>  static u64 acpi_mp_wake_mailbox_paddr __ro_after_init;
> @@ -122,6 +124,7 @@ static int __init init_transition_pgtable(pgd_t *pgd)
>  	return 0;
>  }
> 
> +#ifdef CONFIG_ACPI
>  static int __init acpi_mp_setup_reset(u64 reset_vector)
>  {
>  	struct x86_mapping_info info = {
> @@ -168,6 +171,7 @@ static int __init acpi_mp_setup_reset(u64 reset_vector)
> 
>  	return 0;
>  }
> +#endif

When acpi_mp_setup_reset() is #ifdef'ed out because of CONFIG_ACPI
not being set, doesn't this generate compile warnings about
init_transition_pgtable(), alloc_pgt_page(), free_pgt_page(), etc. being
unused?

It appears that the only code in madt_wakeup.c that is shared between
the ACPI and OF cases is acpi_wakeup_cpu()? Is that correct? 

> 
>  static int acpi_wakeup_cpu(u32 apicid, unsigned long start_ip)
>  {
> @@ -226,6 +230,7 @@ static int acpi_wakeup_cpu(u32 apicid, unsigned long start_ip)
>  	return 0;
>  }
> 
> +#ifdef CONFIG_ACPI
>  static void acpi_mp_disable_offlining(struct acpi_madt_multiproc_wakeup *mp_wake)
>  {
>  	cpu_hotplug_disable_offlining();
> @@ -290,3 +295,36 @@ int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
> 
>  	return 0;
>  }
> +#endif /* CONFIG_ACPI */
> +
> +#ifdef CONFIG_OF
> +u64 __init dtb_parse_mp_wake(void)
> +{
> +	struct device_node *node;
> +	u64 mailaddr = 0;
> +
> +	node = of_find_node_by_path("/cpus");
> +	if (!node)
> +		return 0;
> +
> +	if (of_property_match_string(node, "enable-method", "acpi-wakeup-mailbox") < 0) {
> +		pr_err("No acpi wakeup mailbox enable-method\n");
> +		goto done;

Patch 4 of this series unconditionally calls dtb_parse_mp_wake(),
so it will be called in normal VMs, as a well as SEV-SNP and TDX
kernels built for VTL 2 (assuming CONFIG_OF is set). Normal VMs
presumably won't be using DT and won't have the "/cpus" node,
so this function will silently do nothing, which is fine. But do you
expect the DT "/cpus" node to be present in an SEV-SNP VTL 2
environment? If so, this function will either output some spurious
error messages, or SEV-SNP will use the ACPI wakeup mailbox
method, which is probably not what you want.

Michael

> +	}
> +
> +	if (of_property_read_u64(node, "wakeup-mailbox-addr", &mailaddr)) {
> +		pr_err("Invalid wakeup mailbox addr\n");
> +		goto done;
> +	}
> +	acpi_mp_wake_mailbox_paddr = mailaddr;
> +	pr_info("dt wakeup-mailbox: addr 0x%llx\n", mailaddr);
> +
> +	/* No support for the MADT reset vector yet */
> +	cpu_hotplug_disable_offlining();
> +	apic_update_callback(wakeup_secondary_cpu_64, acpi_wakeup_cpu);
> +
> +done:
> +	of_node_put(node);
> +	return mailaddr;
> +}
> +#endif /* CONFIG_OF */
> --
> 2.25.1
> 

Re: [PATCH v2 3/9] x86/dt: Support the ACPI multiprocessor wakeup for device tree

[Yunhong Jiang]

On Mon, Sep 02, 2024 at 03:35:06AM +0000, Michael Kelley wrote:
> From: Yunhong Jiang <yunhong.jiang@linux.intel.com> Sent: Friday, August 23, 2024 4:23 PM
> > 
> > When a TDX guest boots with the device tree instead of ACPI, it can
> > reuse the ACPI multiprocessor wakeup mechanism to wake up application
> > processors(AP), without introducing a new mechanism from scrach.
> > 
> > In the ACPI spec, two structures are defined to wake up the APs: the
> > multiprocessor wakeup structure and the multiprocessor wakeup mailbox
> > structure. The multiprocessor wakeup structure is passed to OS through a
> > Multiple APIC Description Table(MADT), one field specifying the physical
> > address of the multiprocessor wakeup mailbox structure. The OS sends
> > a message to firmware through the multiprocessor wakeup mailbox
> > structure, to bring up the APs.
> > 
> > In device tree environment, the multiprocessor wakeup structure is not
> > used, to reduce the dependency on the generic ACPI table. The
> > information defined in this structure is defined in the properties of
> > cpus node in the device tree. The "wakeup-mailbox-addr" property
> > specifies the physical address of the multiprocessor wakeup mailbox
> > structure. The OS will follow the ACPI spec to send the message to the
> > firmware to bring up the APs.
> > 
> > Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
> > ---
> >  MAINTAINERS                        |  1 +
> >  arch/x86/Kconfig                   |  2 +-
> >  arch/x86/include/asm/acpi.h        |  1 -
> >  arch/x86/include/asm/madt_wakeup.h | 16 +++++++++++++
> >  arch/x86/kernel/madt_wakeup.c      | 38 ++++++++++++++++++++++++++++++
> >  5 files changed, 56 insertions(+), 2 deletions(-)
> >  create mode 100644 arch/x86/include/asm/madt_wakeup.h
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 5555a3bbac5f..900de6501eba 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -288,6 +288,7 @@ T:	git
> > git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm
> >  F:	Documentation/ABI/testing/configfs-acpi
> >  F:	Documentation/ABI/testing/sysfs-bus-acpi
> >  F:	Documentation/firmware-guide/acpi/
> > +F:	arch/x86/include/asm/madt_wakeup.h
> >  F:	arch/x86/kernel/acpi/
> >  F:	arch/x86/kernel/madt_playdead.S
> >  F:	arch/x86/kernel/madt_wakeup.c
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index d422247b2882..dba46dd30049 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -1123,7 +1123,7 @@ config X86_LOCAL_APIC
> >  config ACPI_MADT_WAKEUP
> >  	def_bool y
> >  	depends on X86_64
> > -	depends on ACPI
> > +	depends on ACPI || OF
> >  	depends on SMP
> >  	depends on X86_LOCAL_APIC
> > 
> > diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h
> > index 21bc53f5ed0c..0e082303ca26 100644
> > --- a/arch/x86/include/asm/acpi.h
> > +++ b/arch/x86/include/asm/acpi.h
> > @@ -83,7 +83,6 @@ union acpi_subtable_headers;
> >  int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
> >  			      const unsigned long end);
> > 
> > -void asm_acpi_mp_play_dead(u64 reset_vector, u64 pgd_pa);
> > 
> >  /*
> >   * Check if the CPU can handle C2 and deeper
> > diff --git a/arch/x86/include/asm/madt_wakeup.h
> > b/arch/x86/include/asm/madt_wakeup.h
> > new file mode 100644
> > index 000000000000..a8cd50af581a
> > --- /dev/null
> > +++ b/arch/x86/include/asm/madt_wakeup.h
> > @@ -0,0 +1,16 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __ASM_X86_MADT_WAKEUP_H
> > +#define __ASM_X86_MADT_WAKEUP_H
> > +
> > +void asm_acpi_mp_play_dead(u64 reset_vector, u64 pgd_pa);
> > +
> > +#if defined(CONFIG_OF) && defined(CONFIG_ACPI_MADT_WAKEUP)
> > +u64 dtb_parse_mp_wake(void);
> > +#else
> > +static inline u64 dtb_parse_mp_wake(void)
> > +{
> > +	return 0;
> > +}
> > +#endif
> > +
> > +#endif /* __ASM_X86_MADT_WAKEUP_H */
> > diff --git a/arch/x86/kernel/madt_wakeup.c b/arch/x86/kernel/madt_wakeup.c
> > index d5ef6215583b..7257e7484569 100644
> > --- a/arch/x86/kernel/madt_wakeup.c
> > +++ b/arch/x86/kernel/madt_wakeup.c
> > @@ -14,6 +14,8 @@
> >  #include <asm/nmi.h>
> >  #include <asm/processor.h>
> >  #include <asm/reboot.h>
> > +#include <asm/madt_wakeup.h>
> > +#include <asm/prom.h>
> > 
> >  /* Physical address of the Multiprocessor Wakeup Structure mailbox */
> >  static u64 acpi_mp_wake_mailbox_paddr __ro_after_init;
> > @@ -122,6 +124,7 @@ static int __init init_transition_pgtable(pgd_t *pgd)
> >  	return 0;
> >  }
> > 
> > +#ifdef CONFIG_ACPI
> >  static int __init acpi_mp_setup_reset(u64 reset_vector)
> >  {
> >  	struct x86_mapping_info info = {
> > @@ -168,6 +171,7 @@ static int __init acpi_mp_setup_reset(u64 reset_vector)
> > 
> >  	return 0;
> >  }
> > +#endif
> 
> When acpi_mp_setup_reset() is #ifdef'ed out because of CONFIG_ACPI
> not being set, doesn't this generate compile warnings about
> init_transition_pgtable(), alloc_pgt_page(), free_pgt_page(), etc. being
> unused?
> 
> It appears that the only code in madt_wakeup.c that is shared between
> the ACPI and OF cases is acpi_wakeup_cpu()? Is that correct?

Yes, the acpi_wakeup_cpu() is the only code. Interestingly that I don't see
compilation warning for the functions you listed like
alloc_pgt_page()/free_pgt_page() etc when built with CONFIG_ACPI disabled.
Will check in deep and figure out the reason.
> 
> > 
> >  static int acpi_wakeup_cpu(u32 apicid, unsigned long start_ip)
> >  {
> > @@ -226,6 +230,7 @@ static int acpi_wakeup_cpu(u32 apicid, unsigned long start_ip)
> >  	return 0;
> >  }
> > 
> > +#ifdef CONFIG_ACPI
> >  static void acpi_mp_disable_offlining(struct acpi_madt_multiproc_wakeup *mp_wake)
> >  {
> >  	cpu_hotplug_disable_offlining();
> > @@ -290,3 +295,36 @@ int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
> > 
> >  	return 0;
> >  }
> > +#endif /* CONFIG_ACPI */
> > +
> > +#ifdef CONFIG_OF
> > +u64 __init dtb_parse_mp_wake(void)
> > +{
> > +	struct device_node *node;
> > +	u64 mailaddr = 0;
> > +
> > +	node = of_find_node_by_path("/cpus");
> > +	if (!node)
> > +		return 0;
> > +
> > +	if (of_property_match_string(node, "enable-method", "acpi-wakeup-mailbox") < 0) {
> > +		pr_err("No acpi wakeup mailbox enable-method\n");
> > +		goto done;
> 
> Patch 4 of this series unconditionally calls dtb_parse_mp_wake(),
> so it will be called in normal VMs, as a well as SEV-SNP and TDX
> kernels built for VTL 2 (assuming CONFIG_OF is set). Normal VMs
> presumably won't be using DT and won't have the "/cpus" node,
> so this function will silently do nothing, which is fine. But do you
> expect the DT "/cpus" node to be present in an SEV-SNP VTL 2
> environment? If so, this function will either output some spurious
> error messages, or SEV-SNP will use the ACPI wakeup mailbox
> method, which is probably not what you want.
> 
> Michael

Yes, will fix the spurios error messages. Thank you for pointing out this.

Thanks
--jyh
> 
> > +	}
> > +
> > +	if (of_property_read_u64(node, "wakeup-mailbox-addr", &mailaddr)) {
> > +		pr_err("Invalid wakeup mailbox addr\n");
> > +		goto done;
> > +	}
> > +	acpi_mp_wake_mailbox_paddr = mailaddr;
> > +	pr_info("dt wakeup-mailbox: addr 0x%llx\n", mailaddr);
> > +
> > +	/* No support for the MADT reset vector yet */
> > +	cpu_hotplug_disable_offlining();
> > +	apic_update_callback(wakeup_secondary_cpu_64, acpi_wakeup_cpu);
> > +
> > +done:
> > +	of_node_put(node);
> > +	return mailaddr;
> > +}
> > +#endif /* CONFIG_OF */
> > --
> > 2.25.1
> > 
> 

[PATCH v2 4/9] x86/hyperv: Parse the ACPI wakeup mailbox

[Yunhong Jiang]

Parse the wakeup mailbox VTL2 TDX guest. Put it to the guest_late_init, so
that it will be invoked before hyperv_init() where the mailbox address is
checked.

Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
---
 arch/x86/include/asm/mshyperv.h | 3 +++
 arch/x86/kernel/cpu/mshyperv.c  | 2 ++
 drivers/hv/hv_common.c          | 8 ++++++++
 3 files changed, 13 insertions(+)

diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
index 390c4d13956d..5178b96c7fc9 100644
--- a/arch/x86/include/asm/mshyperv.h
+++ b/arch/x86/include/asm/mshyperv.h
@@ -10,6 +10,7 @@
 #include <asm/nospec-branch.h>
 #include <asm/paravirt.h>
 #include <asm/mshyperv.h>
+#include <asm/madt_wakeup.h>
 
 /*
  * Hyper-V always provides a single IO-APIC at this MMIO address.
@@ -49,6 +50,8 @@ extern u64 hv_current_partition_id;
 
 extern union hv_ghcb * __percpu *hv_ghcb_pg;
 
+extern u64 wakeup_mailbox_addr;
+
 bool hv_isolation_type_snp(void);
 bool hv_isolation_type_tdx(void);
 u64 hv_tdx_hypercall(u64 control, u64 param1, u64 param2);
diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
index 3d4237f27569..f6b727b4bd0b 100644
--- a/arch/x86/kernel/cpu/mshyperv.c
+++ b/arch/x86/kernel/cpu/mshyperv.c
@@ -43,6 +43,8 @@ struct ms_hyperv_info ms_hyperv;
 bool hyperv_paravisor_present __ro_after_init;
 EXPORT_SYMBOL_GPL(hyperv_paravisor_present);
 
+u64 wakeup_mailbox_addr;
+
 #if IS_ENABLED(CONFIG_HYPERV)
 static inline unsigned int hv_get_nested_msr(unsigned int reg)
 {
diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
index 9c452bfbd571..14b005b6270f 100644
--- a/drivers/hv/hv_common.c
+++ b/drivers/hv/hv_common.c
@@ -365,6 +365,14 @@ void __init ms_hyperv_late_init(void)
 	u8 *randomdata;
 	u32 length, i;
 
+	/*
+	 * Parse the ACPI wakeup structure information from device tree.
+	 * Currently VTL2 TDX guest only.
+	 */
+#ifdef CONFIG_X86_64
+	wakeup_mailbox_addr = dtb_parse_mp_wake();
+#endif
+
 	/*
 	 * Seed the Linux random number generator with entropy provided by
 	 * the Hyper-V host in ACPI table OEM0.
-- 
2.25.1

RE: [PATCH v2 4/9] x86/hyperv: Parse the ACPI wakeup mailbox

[Michael Kelley]

From: Yunhong Jiang <yunhong.jiang@linux.intel.com>
> 
> Parse the wakeup mailbox VTL2 TDX guest. Put it to the guest_late_init, so
> that it will be invoked before hyperv_init() where the mailbox address is
> checked.

Could you elaborate on the choice to set the wakeup_mailbox_address
in ms_hyperv_late_init()? The code in hv_common.c is intended to be
code that is architecture neutral (see the comment at the top of the module),
so it's a red flag to see #ifdef CONFIG_X86_64. Couldn't the
wakeup_mailbox_address be set in the x86 version of hyperv_init()
before it is needed?

> 
> Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
> ---
>  arch/x86/include/asm/mshyperv.h | 3 +++
>  arch/x86/kernel/cpu/mshyperv.c  | 2 ++
>  drivers/hv/hv_common.c          | 8 ++++++++
>  3 files changed, 13 insertions(+)
> 
> diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
> index 390c4d13956d..5178b96c7fc9 100644
> --- a/arch/x86/include/asm/mshyperv.h
> +++ b/arch/x86/include/asm/mshyperv.h
> @@ -10,6 +10,7 @@
>  #include <asm/nospec-branch.h>
>  #include <asm/paravirt.h>
>  #include <asm/mshyperv.h>
> +#include <asm/madt_wakeup.h>
> 
>  /*
>   * Hyper-V always provides a single IO-APIC at this MMIO address.
> @@ -49,6 +50,8 @@ extern u64 hv_current_partition_id;
> 
>  extern union hv_ghcb * __percpu *hv_ghcb_pg;
> 
> +extern u64 wakeup_mailbox_addr;
> +
>  bool hv_isolation_type_snp(void);
>  bool hv_isolation_type_tdx(void);
>  u64 hv_tdx_hypercall(u64 control, u64 param1, u64 param2);
> diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> index 3d4237f27569..f6b727b4bd0b 100644
> --- a/arch/x86/kernel/cpu/mshyperv.c
> +++ b/arch/x86/kernel/cpu/mshyperv.c
> @@ -43,6 +43,8 @@ struct ms_hyperv_info ms_hyperv;
>  bool hyperv_paravisor_present __ro_after_init;
>  EXPORT_SYMBOL_GPL(hyperv_paravisor_present);
> 
> +u64 wakeup_mailbox_addr;

This value duplicates acpi_mp_wake_mailbox_paddr in
madt_wakeup.c. It looks like the duplicate value is used
for two things:

1) In hv_is_private_mmio_tdx() to control the encrypted
vs. decrypted mapping (Patch 5 of this series)

2) As a boolean in hv_vtl_early_init() to avoid overwriting
the wakeup_secondary_cpu_64 value when
dtb_parse_mp_wake() has set it to acpi_wakeup_cpu().
(Patch 9 of this series).

Having a duplicate value is messy, and I'm wondering if
it can be avoided. For (1), hv_private_mmio_tdx() could call
into a function added to madt_wakeup.c to make the
check.  For (2), the check should probably be based on
hv_isolation_type_tdx() instead of whether the wakeup
mailbox address is set.  I'll note that Patch 5 of this series
is using hv_isolation_type_tdx(), so there's a bit of an
inconsistency in testing the wakeup_mailbox_addr in
Patch 9.

This is just a suggestion, as I haven't worked out all
the details. If you think it ends up being messier than
the duplicate value, then I'm OK with it.

Michael

> +
>  #if IS_ENABLED(CONFIG_HYPERV)
>  static inline unsigned int hv_get_nested_msr(unsigned int reg)
>  {
> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
> index 9c452bfbd571..14b005b6270f 100644
> --- a/drivers/hv/hv_common.c
> +++ b/drivers/hv/hv_common.c
> @@ -365,6 +365,14 @@ void __init ms_hyperv_late_init(void)
>  	u8 *randomdata;
>  	u32 length, i;
> 
> +	/*
> +	 * Parse the ACPI wakeup structure information from device tree.
> +	 * Currently VTL2 TDX guest only.
> +	 */
> +#ifdef CONFIG_X86_64
> +	wakeup_mailbox_addr = dtb_parse_mp_wake();
> +#endif
> +
>  	/*
>  	 * Seed the Linux random number generator with entropy provided by
>  	 * the Hyper-V host in ACPI table OEM0.
> --
> 2.25.1
> 

Re: [PATCH v2 4/9] x86/hyperv: Parse the ACPI wakeup mailbox

[Yunhong Jiang]

On Mon, Sep 02, 2024 at 03:35:13AM +0000, Michael Kelley wrote:
> From: Yunhong Jiang <yunhong.jiang@linux.intel.com>
> > 
> > Parse the wakeup mailbox VTL2 TDX guest. Put it to the guest_late_init, so
> > that it will be invoked before hyperv_init() where the mailbox address is
> > checked.
> 
> Could you elaborate on the choice to set the wakeup_mailbox_address
> in ms_hyperv_late_init()? The code in hv_common.c is intended to be
> code that is architecture neutral (see the comment at the top of the module),
> so it's a red flag to see #ifdef CONFIG_X86_64. Couldn't the
> wakeup_mailbox_address be set in the x86 version of hyperv_init()
> before it is needed?

Sure, will try to put it in hyperv_init() before it's needed.
> 
> > 
> > Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
> > ---
> >  arch/x86/include/asm/mshyperv.h | 3 +++
> >  arch/x86/kernel/cpu/mshyperv.c  | 2 ++
> >  drivers/hv/hv_common.c          | 8 ++++++++
> >  3 files changed, 13 insertions(+)
> > 
> > diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
> > index 390c4d13956d..5178b96c7fc9 100644
> > --- a/arch/x86/include/asm/mshyperv.h
> > +++ b/arch/x86/include/asm/mshyperv.h
> > @@ -10,6 +10,7 @@
> >  #include <asm/nospec-branch.h>
> >  #include <asm/paravirt.h>
> >  #include <asm/mshyperv.h>
> > +#include <asm/madt_wakeup.h>
> > 
> >  /*
> >   * Hyper-V always provides a single IO-APIC at this MMIO address.
> > @@ -49,6 +50,8 @@ extern u64 hv_current_partition_id;
> > 
> >  extern union hv_ghcb * __percpu *hv_ghcb_pg;
> > 
> > +extern u64 wakeup_mailbox_addr;
> > +
> >  bool hv_isolation_type_snp(void);
> >  bool hv_isolation_type_tdx(void);
> >  u64 hv_tdx_hypercall(u64 control, u64 param1, u64 param2);
> > diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> > index 3d4237f27569..f6b727b4bd0b 100644
> > --- a/arch/x86/kernel/cpu/mshyperv.c
> > +++ b/arch/x86/kernel/cpu/mshyperv.c
> > @@ -43,6 +43,8 @@ struct ms_hyperv_info ms_hyperv;
> >  bool hyperv_paravisor_present __ro_after_init;
> >  EXPORT_SYMBOL_GPL(hyperv_paravisor_present);
> > 
> > +u64 wakeup_mailbox_addr;
> 
> This value duplicates acpi_mp_wake_mailbox_paddr in
> madt_wakeup.c. It looks like the duplicate value is used
> for two things:
> 
> 1) In hv_is_private_mmio_tdx() to control the encrypted
> vs. decrypted mapping (Patch 5 of this series)
> 
> 2) As a boolean in hv_vtl_early_init() to avoid overwriting
> the wakeup_secondary_cpu_64 value when
> dtb_parse_mp_wake() has set it to acpi_wakeup_cpu().
> (Patch 9 of this series).
> 
> Having a duplicate value is messy, and I'm wondering if
> it can be avoided. For (1), hv_private_mmio_tdx() could call
> into a function added to madt_wakeup.c to make the
> check.  For (2), the check should probably be based on
> hv_isolation_type_tdx() instead of whether the wakeup
> mailbox address is set.  I'll note that Patch 5 of this series
> is using hv_isolation_type_tdx(), so there's a bit of an
> inconsistency in testing the wakeup_mailbox_addr in
> Patch 9.

I think your comment includes two points, the duplicated variables and the
incosistency in the testing.

Thank you for pointing out the duplication of wakeup_mailbox_addr with
acpi_mp_wake_mailbox_paddr. I didn't realize it. Yes, such duplication should be
avoided and will fix it in next submission.

Agree the inconsistency in testing wakeup_mailbox_addr and
hv_isolation_type_tdx() is not good. IMHO, the wakeup_mailbox_addr (or the new
function you proposed) is better than hv_isolation_type_tdx(), since the
wakeup_mailbox_addr is more directly related.  But hv_vtl_init_platform()
happens before DT parse, thus I have to use the hv_isolation_type_tdx() in it. I
don't have a good idea on how to fix this.

Thanks
--jyh

> 
> This is just a suggestion, as I haven't worked out all
> the details. If you think it ends up being messier than
> the duplicate value, then I'm OK with it.
> 
> Michael
> 
> > +
> >  #if IS_ENABLED(CONFIG_HYPERV)
> >  static inline unsigned int hv_get_nested_msr(unsigned int reg)
> >  {
> > diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
> > index 9c452bfbd571..14b005b6270f 100644
> > --- a/drivers/hv/hv_common.c
> > +++ b/drivers/hv/hv_common.c
> > @@ -365,6 +365,14 @@ void __init ms_hyperv_late_init(void)
> >  	u8 *randomdata;
> >  	u32 length, i;
> > 
> > +	/*
> > +	 * Parse the ACPI wakeup structure information from device tree.
> > +	 * Currently VTL2 TDX guest only.
> > +	 */
> > +#ifdef CONFIG_X86_64
> > +	wakeup_mailbox_addr = dtb_parse_mp_wake();
> > +#endif
> > +
> >  	/*
> >  	 * Seed the Linux random number generator with entropy provided by
> >  	 * the Hyper-V host in ACPI table OEM0.
> > --
> > 2.25.1
> > 
> 

RE: [PATCH v2 4/9] x86/hyperv: Parse the ACPI wakeup mailbox

[Michael Kelley]

From: Yunhong Jiang <yunhong.jiang@linux.intel.com> Sent: Tuesday, September 3, 2024 1:19 PM
> 
> On Mon, Sep 02, 2024 at 03:35:13AM +0000, Michael Kelley wrote:
> > From: Yunhong Jiang <yunhong.jiang@linux.intel.com>
> > >
> > > Parse the wakeup mailbox VTL2 TDX guest. Put it to the guest_late_init, so
> > > that it will be invoked before hyperv_init() where the mailbox address is
> > > checked.
> >
> > Could you elaborate on the choice to set the wakeup_mailbox_address
> > in ms_hyperv_late_init()? The code in hv_common.c is intended to be
> > code that is architecture neutral (see the comment at the top of the module),
> > so it's a red flag to see #ifdef CONFIG_X86_64. Couldn't the
> > wakeup_mailbox_address be set in the x86 version of hyperv_init()
> > before it is needed?
> 
> Sure, will try to put it in hyperv_init() before it's needed.
> >
> > >
> > > Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
> > > ---
> > >  arch/x86/include/asm/mshyperv.h | 3 +++
> > >  arch/x86/kernel/cpu/mshyperv.c  | 2 ++
> > >  drivers/hv/hv_common.c          | 8 ++++++++
> > >  3 files changed, 13 insertions(+)
> > >
> > > diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
> > > index 390c4d13956d..5178b96c7fc9 100644
> > > --- a/arch/x86/include/asm/mshyperv.h
> > > +++ b/arch/x86/include/asm/mshyperv.h
> > > @@ -10,6 +10,7 @@
> > >  #include <asm/nospec-branch.h>
> > >  #include <asm/paravirt.h>
> > >  #include <asm/mshyperv.h>
> > > +#include <asm/madt_wakeup.h>
> > >
> > >  /*
> > >   * Hyper-V always provides a single IO-APIC at this MMIO address.
> > > @@ -49,6 +50,8 @@ extern u64 hv_current_partition_id;
> > >
> > >  extern union hv_ghcb * __percpu *hv_ghcb_pg;
> > >
> > > +extern u64 wakeup_mailbox_addr;
> > > +
> > >  bool hv_isolation_type_snp(void);
> > >  bool hv_isolation_type_tdx(void);
> > >  u64 hv_tdx_hypercall(u64 control, u64 param1, u64 param2);
> > > diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> > > index 3d4237f27569..f6b727b4bd0b 100644
> > > --- a/arch/x86/kernel/cpu/mshyperv.c
> > > +++ b/arch/x86/kernel/cpu/mshyperv.c
> > > @@ -43,6 +43,8 @@ struct ms_hyperv_info ms_hyperv;
> > >  bool hyperv_paravisor_present __ro_after_init;
> > >  EXPORT_SYMBOL_GPL(hyperv_paravisor_present);
> > >
> > > +u64 wakeup_mailbox_addr;
> >
> > This value duplicates acpi_mp_wake_mailbox_paddr in
> > madt_wakeup.c. It looks like the duplicate value is used
> > for two things:
> >
> > 1) In hv_is_private_mmio_tdx() to control the encrypted
> > vs. decrypted mapping (Patch 5 of this series)
> >
> > 2) As a boolean in hv_vtl_early_init() to avoid overwriting
> > the wakeup_secondary_cpu_64 value when
> > dtb_parse_mp_wake() has set it to acpi_wakeup_cpu().
> > (Patch 9 of this series).
> >
> > Having a duplicate value is messy, and I'm wondering if
> > it can be avoided. For (1), hv_private_mmio_tdx() could call
> > into a function added to madt_wakeup.c to make the
> > check.  For (2), the check should probably be based on
> > hv_isolation_type_tdx() instead of whether the wakeup
> > mailbox address is set.  I'll note that Patch 5 of this series
> > is using hv_isolation_type_tdx(), so there's a bit of an
> > inconsistency in testing the wakeup_mailbox_addr in
> > Patch 9.
> 
> I think your comment includes two points, the duplicated variables and the
> incosistency in the testing.
> 
> Thank you for pointing out the duplication of wakeup_mailbox_addr with
> acpi_mp_wake_mailbox_paddr. I didn't realize it. Yes, such duplication should be
> avoided and will fix it in next submission.
> 
> Agree the inconsistency in testing wakeup_mailbox_addr and
> hv_isolation_type_tdx() is not good. IMHO, the wakeup_mailbox_addr (or the new
> function you proposed) is better than hv_isolation_type_tdx(), since the
> wakeup_mailbox_addr is more directly related.  But hv_vtl_init_platform()
> happens before DT parse, thus I have to use the hv_isolation_type_tdx() in it. I
> don't have a good idea on how to fix this.
> 
> Thanks
> --jyh
> 

I don't think there's a requirement to set the "is_private_mmio"
function in hv_vtl_init_platform(). It just needs to be set before
acpi_wakeup_cpu() is called, which does the memremap() that will
invoke the "is_private_mmio" function to decide whether to map
as encrypted or decrypted.

So maybe setting the "is_private_mmio" function could be
done after dtb_parse_mp_wake() is called in its new location, and
you know you have a valid wake mailbox address? Again, I haven't
worked out all the details, so this approach might be just as messy,
but in a different way. Use your judgment ... :-)

Michael

Re: [PATCH v2 4/9] x86/hyperv: Parse the ACPI wakeup mailbox

[Yunhong Jiang]

On Wed, Sep 04, 2024 at 02:56:49PM +0000, Michael Kelley wrote:
> From: Yunhong Jiang <yunhong.jiang@linux.intel.com> Sent: Tuesday, September 3, 2024 1:19 PM
> > 
> > On Mon, Sep 02, 2024 at 03:35:13AM +0000, Michael Kelley wrote:
> > > From: Yunhong Jiang <yunhong.jiang@linux.intel.com>
> > > >
> > > > Parse the wakeup mailbox VTL2 TDX guest. Put it to the guest_late_init, so
> > > > that it will be invoked before hyperv_init() where the mailbox address is
> > > > checked.
> > >
> > > Could you elaborate on the choice to set the wakeup_mailbox_address
> > > in ms_hyperv_late_init()? The code in hv_common.c is intended to be
> > > code that is architecture neutral (see the comment at the top of the module),
> > > so it's a red flag to see #ifdef CONFIG_X86_64. Couldn't the
> > > wakeup_mailbox_address be set in the x86 version of hyperv_init()
> > > before it is needed?
> > 
> > Sure, will try to put it in hyperv_init() before it's needed.
> > >
> > > >
> > > > Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
> > > > ---
> > > >  arch/x86/include/asm/mshyperv.h | 3 +++
> > > >  arch/x86/kernel/cpu/mshyperv.c  | 2 ++
> > > >  drivers/hv/hv_common.c          | 8 ++++++++
> > > >  3 files changed, 13 insertions(+)
> > > >
> > > > diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
> > > > index 390c4d13956d..5178b96c7fc9 100644
> > > > --- a/arch/x86/include/asm/mshyperv.h
> > > > +++ b/arch/x86/include/asm/mshyperv.h
> > > > @@ -10,6 +10,7 @@
> > > >  #include <asm/nospec-branch.h>
> > > >  #include <asm/paravirt.h>
> > > >  #include <asm/mshyperv.h>
> > > > +#include <asm/madt_wakeup.h>
> > > >
> > > >  /*
> > > >   * Hyper-V always provides a single IO-APIC at this MMIO address.
> > > > @@ -49,6 +50,8 @@ extern u64 hv_current_partition_id;
> > > >
> > > >  extern union hv_ghcb * __percpu *hv_ghcb_pg;
> > > >
> > > > +extern u64 wakeup_mailbox_addr;
> > > > +
> > > >  bool hv_isolation_type_snp(void);
> > > >  bool hv_isolation_type_tdx(void);
> > > >  u64 hv_tdx_hypercall(u64 control, u64 param1, u64 param2);
> > > > diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> > > > index 3d4237f27569..f6b727b4bd0b 100644
> > > > --- a/arch/x86/kernel/cpu/mshyperv.c
> > > > +++ b/arch/x86/kernel/cpu/mshyperv.c
> > > > @@ -43,6 +43,8 @@ struct ms_hyperv_info ms_hyperv;
> > > >  bool hyperv_paravisor_present __ro_after_init;
> > > >  EXPORT_SYMBOL_GPL(hyperv_paravisor_present);
> > > >
> > > > +u64 wakeup_mailbox_addr;
> > >
> > > This value duplicates acpi_mp_wake_mailbox_paddr in
> > > madt_wakeup.c. It looks like the duplicate value is used
> > > for two things:
> > >
> > > 1) In hv_is_private_mmio_tdx() to control the encrypted
> > > vs. decrypted mapping (Patch 5 of this series)
> > >
> > > 2) As a boolean in hv_vtl_early_init() to avoid overwriting
> > > the wakeup_secondary_cpu_64 value when
> > > dtb_parse_mp_wake() has set it to acpi_wakeup_cpu().
> > > (Patch 9 of this series).
> > >
> > > Having a duplicate value is messy, and I'm wondering if
> > > it can be avoided. For (1), hv_private_mmio_tdx() could call
> > > into a function added to madt_wakeup.c to make the
> > > check.  For (2), the check should probably be based on
> > > hv_isolation_type_tdx() instead of whether the wakeup
> > > mailbox address is set.  I'll note that Patch 5 of this series
> > > is using hv_isolation_type_tdx(), so there's a bit of an
> > > inconsistency in testing the wakeup_mailbox_addr in
> > > Patch 9.
> > 
> > I think your comment includes two points, the duplicated variables and the
> > incosistency in the testing.
> > 
> > Thank you for pointing out the duplication of wakeup_mailbox_addr with
> > acpi_mp_wake_mailbox_paddr. I didn't realize it. Yes, such duplication should be
> > avoided and will fix it in next submission.
> > 
> > Agree the inconsistency in testing wakeup_mailbox_addr and
> > hv_isolation_type_tdx() is not good. IMHO, the wakeup_mailbox_addr (or the new
> > function you proposed) is better than hv_isolation_type_tdx(), since the
> > wakeup_mailbox_addr is more directly related.  But hv_vtl_init_platform()
> > happens before DT parse, thus I have to use the hv_isolation_type_tdx() in it. I
> > don't have a good idea on how to fix this.
> > 
> > Thanks
> > --jyh
> > 
> 
> I don't think there's a requirement to set the "is_private_mmio"
> function in hv_vtl_init_platform(). It just needs to be set before
> acpi_wakeup_cpu() is called, which does the memremap() that will
> invoke the "is_private_mmio" function to decide whether to map
> as encrypted or decrypted.
> 
> So maybe setting the "is_private_mmio" function could be
> done after dtb_parse_mp_wake() is called in its new location, and
> you know you have a valid wake mailbox address? Again, I haven't
> worked out all the details, so this approach might be just as messy,
> but in a different way. Use your judgment ... :-)

Sorry that I didn't explain clearly. The testing in hv_vtl_init_platform() is
not only for the is_private_mmio, but also for the realmode_reserve(), which
happens before the DT parse.

BTW, I don't know why the trampoline_64.S is put into the real mode blob. I
don't find any specific requirement in the code, but I'm not sure if I missed
anything. If this dependency is removed, all the TDX guest will benefit.

Thank you
--jyh

> 
> Michael

[PATCH v2 5/9] x86/hyperv: Mark ACPI wakeup mailbox page as private

[Yunhong Jiang]

Current code maps MMIO devices as shared (decrypted) by default in a
confidential computing VM. However, the wakeup mailbox must be accessed
as private (encrypted) because it's accessed by the OS and the firmware,
both are in the guest's context and encrypted. Set the wakeup mailbox
range as private explicitly.

Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
---
 arch/x86/hyperv/hv_vtl.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/arch/x86/hyperv/hv_vtl.c b/arch/x86/hyperv/hv_vtl.c
index 04775346369c..987a6a1200b0 100644
--- a/arch/x86/hyperv/hv_vtl.c
+++ b/arch/x86/hyperv/hv_vtl.c
@@ -22,10 +22,26 @@ static bool __init hv_vtl_msi_ext_dest_id(void)
 	return true;
 }
 
+static inline bool within_page(u64 addr, u64 start)
+{
+	return addr >= start && addr < (start + PAGE_SIZE);
+}
+
+/*
+ * The ACPI wakeup mailbox are accessed by the OS and the BIOS, both are in the
+ * guest's context, instead of the hypervisor/VMM context.
+ */
+static bool hv_is_private_mmio_tdx(u64 addr)
+{
+	return wakeup_mailbox_addr && within_page(addr, wakeup_mailbox_addr);
+}
+
 void __init hv_vtl_init_platform(void)
 {
 	pr_info("Linux runs in Hyper-V Virtual Trust Level\n");
 
+	if (hv_isolation_type_tdx())
+		x86_platform.hyper.is_private_mmio = hv_is_private_mmio_tdx;
 	x86_platform.realmode_reserve = x86_init_noop;
 	x86_platform.realmode_init = x86_init_noop;
 	x86_init.irqs.pre_vector_init = x86_init_noop;
-- 
2.25.1

RE: [PATCH v2 5/9] x86/hyperv: Mark ACPI wakeup mailbox page as private

[Michael Kelley]

From: Yunhong Jiang <yunhong.jiang@linux.intel.com> Sent: Friday, August 23, 2024 4:23 PM
> 
> Current code maps MMIO devices as shared (decrypted) by default in a
> confidential computing VM. However, the wakeup mailbox must be accessed
> as private (encrypted) because it's accessed by the OS and the firmware,
> both are in the guest's context and encrypted. Set the wakeup mailbox
> range as private explicitly.
> 
> Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
> ---
>  arch/x86/hyperv/hv_vtl.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/arch/x86/hyperv/hv_vtl.c b/arch/x86/hyperv/hv_vtl.c
> index 04775346369c..987a6a1200b0 100644
> --- a/arch/x86/hyperv/hv_vtl.c
> +++ b/arch/x86/hyperv/hv_vtl.c
> @@ -22,10 +22,26 @@ static bool __init hv_vtl_msi_ext_dest_id(void)
>  	return true;
>  }
> 
> +static inline bool within_page(u64 addr, u64 start)
> +{
> +	return addr >= start && addr < (start + PAGE_SIZE);
> +}
> +
> +/*
> + * The ACPI wakeup mailbox are accessed by the OS and the BIOS, both are in the
> + * guest's context, instead of the hypervisor/VMM context.
> + */
> +static bool hv_is_private_mmio_tdx(u64 addr)
> +{
> +	return wakeup_mailbox_addr && within_page(addr, wakeup_mailbox_addr);
> +}
> +
>  void __init hv_vtl_init_platform(void)
>  {
>  	pr_info("Linux runs in Hyper-V Virtual Trust Level\n");
> 
> +	if (hv_isolation_type_tdx())
> +		x86_platform.hyper.is_private_mmio = hv_is_private_mmio_tdx;

hv_vtl_init_platform() is unconditionally called in
ms_hyperv_init_platform(). So in the case of a normal TDX guest
running with a paravisor on Hyper-V, the above code will overwrite
the is_private_mmio function that was set in hv_vtom_init(). Then
the mapping of the emulated IOAPIC and TPM provided by the
paravisor won't be correct.

Michael

>  	x86_platform.realmode_reserve = x86_init_noop;
>  	x86_platform.realmode_init = x86_init_noop;
>  	x86_init.irqs.pre_vector_init = x86_init_noop;
> --
> 2.25.1
> 

Re: [PATCH v2 5/9] x86/hyperv: Mark ACPI wakeup mailbox page as private

[Saurabh Singh Sengar]

On Mon, Sep 02, 2024 at 03:35:18AM +0000, Michael Kelley wrote:
> From: Yunhong Jiang <yunhong.jiang@linux.intel.com> Sent: Friday, August 23, 2024 4:23 PM
> > 
> > Current code maps MMIO devices as shared (decrypted) by default in a
> > confidential computing VM. However, the wakeup mailbox must be accessed
> > as private (encrypted) because it's accessed by the OS and the firmware,
> > both are in the guest's context and encrypted. Set the wakeup mailbox
> > range as private explicitly.
> > 
> > Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
> > ---
> >  arch/x86/hyperv/hv_vtl.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/arch/x86/hyperv/hv_vtl.c b/arch/x86/hyperv/hv_vtl.c
> > index 04775346369c..987a6a1200b0 100644
> > --- a/arch/x86/hyperv/hv_vtl.c
> > +++ b/arch/x86/hyperv/hv_vtl.c
> > @@ -22,10 +22,26 @@ static bool __init hv_vtl_msi_ext_dest_id(void)
> >  	return true;
> >  }
> > 
> > +static inline bool within_page(u64 addr, u64 start)
> > +{
> > +	return addr >= start && addr < (start + PAGE_SIZE);
> > +}
> > +
> > +/*
> > + * The ACPI wakeup mailbox are accessed by the OS and the BIOS, both are in the
> > + * guest's context, instead of the hypervisor/VMM context.
> > + */
> > +static bool hv_is_private_mmio_tdx(u64 addr)
> > +{
> > +	return wakeup_mailbox_addr && within_page(addr, wakeup_mailbox_addr);
> > +}
> > +
> >  void __init hv_vtl_init_platform(void)
> >  {
> >  	pr_info("Linux runs in Hyper-V Virtual Trust Level\n");
> > 
> > +	if (hv_isolation_type_tdx())
> > +		x86_platform.hyper.is_private_mmio = hv_is_private_mmio_tdx;
> 
> hv_vtl_init_platform() is unconditionally called in
> ms_hyperv_init_platform(). So in the case of a normal TDX guest
> running with a paravisor on Hyper-V, the above code will overwrite
> the is_private_mmio function that was set in hv_vtom_init(). Then
> the mapping of the emulated IOAPIC and TPM provided by the
> paravisor won't be correct.
> 
> Michael

non-VTL Hyper-V platforms are expected to disable CONFIG_HYPERV_VTL_MODE,
that means for a normal TDX guest hv_vtl_init_platform will be an empty
stub. Have I missed anything ?

- Saurabh

RE: [PATCH v2 5/9] x86/hyperv: Mark ACPI wakeup mailbox page as private

[Michael Kelley]

From: Saurabh Singh Sengar <ssengar@linux.microsoft.com> Sent: Monday, September 2, 2024 11:39 AM
> 
> On Mon, Sep 02, 2024 at 03:35:18AM +0000, Michael Kelley wrote:
> > From: Yunhong Jiang <yunhong.jiang@linux.intel.com> Sent: Friday, August 23, 2024
> 4:23 PM
> > >
> > > Current code maps MMIO devices as shared (decrypted) by default in a
> > > confidential computing VM. However, the wakeup mailbox must be accessed
> > > as private (encrypted) because it's accessed by the OS and the firmware,
> > > both are in the guest's context and encrypted. Set the wakeup mailbox
> > > range as private explicitly.
> > >
> > > Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
> > > ---
> > >  arch/x86/hyperv/hv_vtl.c | 16 ++++++++++++++++
> > >  1 file changed, 16 insertions(+)
> > >
> > > diff --git a/arch/x86/hyperv/hv_vtl.c b/arch/x86/hyperv/hv_vtl.c
> > > index 04775346369c..987a6a1200b0 100644
> > > --- a/arch/x86/hyperv/hv_vtl.c
> > > +++ b/arch/x86/hyperv/hv_vtl.c
> > > @@ -22,10 +22,26 @@ static bool __init hv_vtl_msi_ext_dest_id(void)
> > >  	return true;
> > >  }
> > >
> > > +static inline bool within_page(u64 addr, u64 start)
> > > +{
> > > +	return addr >= start && addr < (start + PAGE_SIZE);
> > > +}
> > > +
> > > +/*
> > > + * The ACPI wakeup mailbox are accessed by the OS and the BIOS, both are in the
> > > + * guest's context, instead of the hypervisor/VMM context.
> > > + */
> > > +static bool hv_is_private_mmio_tdx(u64 addr)
> > > +{
> > > +	return wakeup_mailbox_addr && within_page(addr, wakeup_mailbox_addr);
> > > +}
> > > +
> > >  void __init hv_vtl_init_platform(void)
> > >  {
> > >  	pr_info("Linux runs in Hyper-V Virtual Trust Level\n");
> > >
> > > +	if (hv_isolation_type_tdx())
> > > +		x86_platform.hyper.is_private_mmio = hv_is_private_mmio_tdx;
> >
> > hv_vtl_init_platform() is unconditionally called in
> > ms_hyperv_init_platform(). So in the case of a normal TDX guest
> > running with a paravisor on Hyper-V, the above code will overwrite
> > the is_private_mmio function that was set in hv_vtom_init(). Then
> > the mapping of the emulated IOAPIC and TPM provided by the
> > paravisor won't be correct.
> >
> > Michael
> 
> non-VTL Hyper-V platforms are expected to disable CONFIG_HYPERV_VTL_MODE,
> that means for a normal TDX guest hv_vtl_init_platform will be an empty
> stub. Have I missed anything ?
> 

Ah, you are right. My concern is misplaced and can be ignored. :-)

Michael

[PATCH v2 6/9] x86/realmode: Add memory range support to reserve_real_mode

[Yunhong Jiang]

Currently the reserve_real_mode() always allocates memory from below 1M
range, although some real mode blob code can execute above 1M.

The VTL2 TDX hyperv guest may have no memory available below 1M, but it
needs to invoke some real mode blob code that can execute above 1M memory.

Instead of providing a platform specific realmode_reserve callback, the
reserve_real_mode() is updated to support flexible memory range to meet
this requirement.

Originally-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://lore.kernel.org/lkml/87a5ho2q6x.ffs@tglx/

Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
---
 arch/x86/include/asm/x86_init.h |  6 ++++++
 arch/x86/kernel/x86_init.c      |  3 +++
 arch/x86/realmode/init.c        | 14 ++++++--------
 3 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index 213cf5379a5a..9e3198bfe56e 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -31,12 +31,18 @@ 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 realmode trampoline
+ *				(default 1M)
+ * @reserve_bios:		platform specific address limit for reserving the BIOS area
+ *				(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;
+	unsigned long reserve_bios;
 };
 
 /**
diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
index 82b128d3f309..f3226aa77bfb 100644
--- a/arch/x86/kernel/x86_init.c
+++ b/arch/x86/kernel/x86_init.c
@@ -8,6 +8,7 @@
 #include <linux/ioport.h>
 #include <linux/export.h>
 #include <linux/pci.h>
+#include <linux/sizes.h>
 
 #include <asm/acpi.h>
 #include <asm/bios_ebda.h>
@@ -68,6 +69,8 @@ struct x86_init_ops x86_init __initdata = {
 		.reserve_resources	= reserve_standard_io_resources,
 		.memory_setup		= e820__memory_setup_default,
 		.dmi_setup		= dmi_setup,
+		.realmode_limit		= SZ_1M,
+		.reserve_bios		= SZ_1M,
 	},
 
 	.mpparse = {
diff --git a/arch/x86/realmode/init.c b/arch/x86/realmode/init.c
index f9bc444a3064..6d658ad8c0f4 100644
--- a/arch/x86/realmode/init.c
+++ b/arch/x86/realmode/init.c
@@ -45,7 +45,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,17 +54,15 @@ 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 %lluM for the real-mode trampoline\n", limit >> 20);
 	else
 		set_real_mode_mem(mem);
 
-	/*
-	 * Unconditionally reserve the entire first 1M, see comment in
-	 * setup_arch().
-	 */
-	memblock_reserve(0, SZ_1M);
+	/* Reserve the entire first 1M, if enabled. See comment in setup_arch(). */
+	if (x86_init.resources.reserve_bios)
+		memblock_reserve(0, x86_init.resources.reserve_bios);
 }
 
 static void __init sme_sev_setup_real_mode(struct trampoline_header *th)
-- 
2.25.1

[PATCH v2 7/9] x86/hyperv: Move setting the real_mode_header to hv_vtl_init_platform

[Yunhong Jiang]

For the VTL2 hyperv guest, currently the hv_vtl_init_platform() clears
x86_platform.realmode_reserve/init while the hv_vtl_early_init() sets the
real_mode_header.

Set the real_mode_header together with x86_platform.realmode_reserve/init
in hv_vtl_init_platform(). This is ok because x86_platform.realmode_init()
is invoked from an early initcall while hv_vtl_init_platform() is called
during early boot.

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://lore.kernel.org/lkml/87a5ho2q6x.ffs@tglx/
Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
---
 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 987a6a1200b0..e5aa2688cdd0 100644
--- a/arch/x86/hyperv/hv_vtl.c
+++ b/arch/x86/hyperv/hv_vtl.c
@@ -44,6 +44,7 @@ void __init hv_vtl_init_platform(void)
 		x86_platform.hyper.is_private_mmio = hv_is_private_mmio_tdx;
 	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;
 
@@ -259,7 +260,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.25.1

[PATCH v2 8/9] x86/hyperv: Set realmode_limit to 4G for VTL2 TDX guest

[Yunhong Jiang]

The VTL2 TDX guest may have no sub-1M memory available, but it needs to
invoke trampoline_start64 to wake up the APs through the wakeup mailbox
mechanism. Set realmode_limit to 4G for the VTL2 TDX guest, so that
reserve_real_mode allocae memory under 4G.

Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
---
 arch/x86/hyperv/hv_vtl.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/x86/hyperv/hv_vtl.c b/arch/x86/hyperv/hv_vtl.c
index e5aa2688cdd0..5829aac74f80 100644
--- a/arch/x86/hyperv/hv_vtl.c
+++ b/arch/x86/hyperv/hv_vtl.c
@@ -40,11 +40,15 @@ void __init hv_vtl_init_platform(void)
 {
 	pr_info("Linux runs in Hyper-V Virtual Trust Level\n");
 
-	if (hv_isolation_type_tdx())
+	if (hv_isolation_type_tdx()) {
 		x86_platform.hyper.is_private_mmio = hv_is_private_mmio_tdx;
-	x86_platform.realmode_reserve = x86_init_noop;
-	x86_platform.realmode_init = x86_init_noop;
-	real_mode_header = &hv_vtl_real_mode_header;
+		x86_init.resources.realmode_limit = SZ_4G;
+		x86_init.resources.reserve_bios = 0;
+	} 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;
 
-- 
2.25.1

RE: [PATCH v2 8/9] x86/hyperv: Set realmode_limit to 4G for VTL2 TDX guest

[Michael Kelley]

From: Yunhong Jiang <yunhong.jiang@linux.intel.com>
> 
> The VTL2 TDX guest may have no sub-1M memory available, but it needs to
> invoke trampoline_start64 to wake up the APs through the wakeup mailbox
> mechanism. Set realmode_limit to 4G for the VTL2 TDX guest, so that
> reserve_real_mode allocae memory under 4G.

s/allocate/allocate/

Michael

> 
> Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
> ---
>  arch/x86/hyperv/hv_vtl.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/hyperv/hv_vtl.c b/arch/x86/hyperv/hv_vtl.c
> index e5aa2688cdd0..5829aac74f80 100644
> --- a/arch/x86/hyperv/hv_vtl.c
> +++ b/arch/x86/hyperv/hv_vtl.c
> @@ -40,11 +40,15 @@ void __init hv_vtl_init_platform(void)
>  {
>  	pr_info("Linux runs in Hyper-V Virtual Trust Level\n");
> 
> -	if (hv_isolation_type_tdx())
> +	if (hv_isolation_type_tdx()) {
>  		x86_platform.hyper.is_private_mmio = hv_is_private_mmio_tdx;
> -	x86_platform.realmode_reserve = x86_init_noop;
> -	x86_platform.realmode_init = x86_init_noop;
> -	real_mode_header = &hv_vtl_real_mode_header;
> +		x86_init.resources.realmode_limit = SZ_4G;
> +		x86_init.resources.reserve_bios = 0;
> +	} 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;
> 
> --
> 2.25.1
> 

[PATCH v2 9/9] x86/hyperv: Use wakeup mailbox for VTL2 guests if available

[Yunhong Jiang]

For VTL2 hyperv guest with wakeup mailbox in device tree, don't
overwrite wakeup_secondary_cpu_64 so that the acpi_wakeup_cpu will be
used to bring up the APs.

Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
---
 arch/x86/hyperv/hv_vtl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/hyperv/hv_vtl.c b/arch/x86/hyperv/hv_vtl.c
index 5829aac74f80..09a7410200ba 100644
--- a/arch/x86/hyperv/hv_vtl.c
+++ b/arch/x86/hyperv/hv_vtl.c
@@ -264,7 +264,8 @@ 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);
+	if (!wakeup_mailbox_addr)
+		apic_update_callback(wakeup_secondary_cpu_64, hv_vtl_wakeup_secondary_cpu);
 
 	return 0;
 }
-- 
2.25.1