| Message ID | 20240823232327.2408869-3-yunhong.jiang@linux.intel.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | x86/hyperv: Support wakeup mailbox for VTL2 TDX guest | expand |
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
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 >
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 > > > >
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
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 > >
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 > > > > >
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
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
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
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