| Message ID | 20250503191515.24041-5-ricardo.neri-calderon@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | x86/hyperv/hv_vtl: Use a wakeup mailbox to boot secondary CPUs | expand |
On Sun, May 04, 2025 at 06:45:59PM +0200, Krzysztof Kozlowski wrote: > On Sat, May 03, 2025 at 12:15:06PM GMT, Ricardo Neri wrote: > > Add bindings for CPUs in x86 architecture. Start by defining the `reg` and > > What for? Thank you for your quick feedback, Krzysztof! Do you mean for what reason I want to start bindings for x86 CPUs? Or only the `reg` property? If the former, it is to add an enable-method property to x86 CPUs. If the latter, is to show the relationship between APIC and `reg`. > > > `enable-method` properties and their relationship to x86 APIC ID and the > > available mechanisms to boot secondary CPUs. > > > > Start defining bindings for Intel processors. Bindings for other vendors > > can be added later as needed. > > > > Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com> > > --- > > Not really tested so only limited review follows. Sorry, I ran make dt_binding_check but only on this schema. I missed the reported error. > > > .../devicetree/bindings/x86/cpus.yaml | 80 +++++++++++++++++++ > > 1 file changed, 80 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/x86/cpus.yaml > > > > diff --git a/Documentation/devicetree/bindings/x86/cpus.yaml b/Documentation/devicetree/bindings/x86/cpus.yaml > > new file mode 100644 > > index 000000000000..108b3ad64aea > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/x86/cpus.yaml > > @@ -0,0 +1,80 @@ > > +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/x86/cpus.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: x86 CPUs > > + > > +maintainers: > > + - Ricardo Neri <ricardo.neri-calderon@linux.intel.com> > > + > > +description: | > > + Description of x86 CPUs in a system through the "cpus" node. > > + > > + Detailed information about the CPU architecture can be found in the Intel > > + Software Developer's Manual: > > + https://intel.com/sdm > > + > > +properties: > > + compatible: > > + enum: > > + - intel,x86 > > That's architecture, not a CPU. CPUs are like 80286, 80386, so that's > not even specific instruction set. I don't get what you need it for. Am I to understand the the `compatible` property is not needed if the bindings apply to any x86 CPU? > > > + > > + reg: > > Missing constraints. I could add minItems. For maxItems, there is no limit to the number of threads. > > > + description: | > > Do not need '|' unless you need to preserve formatting. OK. > > > + Local APIC ID of the CPU. If the CPU has more than one execution thread, > > + then the property is an array with one element per thread. > > + > > + enable-method: > > + $ref: /schemas/types.yaml#/definitions/string > > + description: | > > + The method used to wake up secondary CPUs. This property is not needed if > > + the secondary processors are booted using INIT assert, de-assert followed > > + by Start-Up IPI messages as described in the Volume 3, Section 11.4 of > > + Intel Software Developer's Manual. > > + > > + It is also optional for the bootstrap CPU. > > + > > + oneOf: > > I see only one entry, so didn't you want an enum? Indeed, enum would be more appropriate. > > > + - items: > > Not a list > > > + - const: intel,wakeup-mailbox > > So every vendor is supposed to come with different name for the same > feature? Or is wakeup-mailnox really intel specific, but then specific > to which processors? It would not be necessary for every vendor to provide a different name for the same feature. I saw, however that the Devicetree specification requires a [vendor],[method] stringlist. Also, platform firmware for any processor could implement the wakeup mailbox. > > > > + description: | > > + CPUs are woken up using the mailbox mechanism. The platform > > + firmware boots the secondary CPUs and puts them in a state > > + to check the mailbox for a wakeup command from the operating > > + system. > > + > > +required: > > + - reg > > + - compatible > > + > > +unevaluatedProperties: false > > Missing ref in top-level or this is supposed to be additionalProps. See > example-schema. I will check. > > > + > > +examples: > > + - | > > + /* > > + * A system with two CPUs. cpu@0 is the bootstrap CPU and its status is > > + * "okay". It does not have the enable-method property. cpu@1 is a > > + * secondary CPU. Its status is "disabled" and defines the enable-method > > + * property. > > + */ > > + > > + cpus { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + cpu@0 { > > + reg = <0x0 0x1>; > > + compatible = "intel,x86"; > > + status = "okay"; > > Drop I will drop status = "okay" > > > + }; > > + > > + cpu@1 { > > + reg = <0x0 0x1>; > > + compatible = "intel,x86"; > > + status = "disabled"; > > Why? Because this is a secondary CPU that the operating system will enable using the method specified in the `enable-method` property. Thanks and BR, Ricardo
On Mon, May 05, 2025 at 09:52:35PM GMT, Ricardo Neri wrote: > On Sun, May 04, 2025 at 06:45:59PM +0200, Krzysztof Kozlowski wrote: > > On Sat, May 03, 2025 at 12:15:06PM GMT, Ricardo Neri wrote: > > > Add bindings for CPUs in x86 architecture. Start by defining the `reg` and > > > > What for? > > Thank you for your quick feedback, Krzysztof! > > Do you mean for what reason I want to start bindings for x86 CPUs? Or only Yes. For which devices, what purpose. > the `reg` property? If the former, it is to add an enable-method property to > x86 CPUs. If the latter, is to show the relationship between APIC and `reg`. > > > > > > `enable-method` properties and their relationship to x86 APIC ID and the > > > available mechanisms to boot secondary CPUs. > > > > > > Start defining bindings for Intel processors. Bindings for other vendors > > > can be added later as needed. > > > > > > Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com> > > > --- > > > > Not really tested so only limited review follows. > > Sorry, I ran make dt_binding_check but only on this schema. I missed the > reported error. > > > > > > .../devicetree/bindings/x86/cpus.yaml | 80 +++++++++++++++++++ > > > 1 file changed, 80 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/x86/cpus.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/x86/cpus.yaml b/Documentation/devicetree/bindings/x86/cpus.yaml > > > new file mode 100644 > > > index 000000000000..108b3ad64aea > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/x86/cpus.yaml > > > @@ -0,0 +1,80 @@ > > > +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/x86/cpus.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: x86 CPUs > > > + > > > +maintainers: > > > + - Ricardo Neri <ricardo.neri-calderon@linux.intel.com> > > > + > > > +description: | > > > + Description of x86 CPUs in a system through the "cpus" node. > > > + > > > + Detailed information about the CPU architecture can be found in the Intel > > > + Software Developer's Manual: > > > + https://intel.com/sdm > > > + > > > +properties: > > > + compatible: > > > + enum: > > > + - intel,x86 > > > > That's architecture, not a CPU. CPUs are like 80286, 80386, so that's > > not even specific instruction set. I don't get what you need it for. > > Am I to understand the the `compatible` property is not needed if the > bindings apply to any x86 CPU? Every device needs compatible. Its meaning is explained: https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#compatible If you add here a device representing CPU, then look at existing bindings for CPUs how they do it. It again feels like you add DT for platform which is not a real thing. If you use DT, you do not get different rules, therefore read all standard guides and tutorials (there were many, quite comprehensive). > > > > > > + > > > + reg: > > > > Missing constraints. > > I could add minItems. For maxItems, there is no limit to the number of > threads. I am pretty sure that any given CPU, e.g. 80486 has a fixed number of threads... > > > > > > + description: | > > > > Do not need '|' unless you need to preserve formatting. > > OK. > > > > > > + Local APIC ID of the CPU. If the CPU has more than one execution thread, > > > + then the property is an array with one element per thread. > > > + > > > + enable-method: > > > + $ref: /schemas/types.yaml#/definitions/string > > > + description: | > > > + The method used to wake up secondary CPUs. This property is not needed if > > > + the secondary processors are booted using INIT assert, de-assert followed > > > + by Start-Up IPI messages as described in the Volume 3, Section 11.4 of > > > + Intel Software Developer's Manual. > > > + > > > + It is also optional for the bootstrap CPU. > > > + > > > + oneOf: > > > > I see only one entry, so didn't you want an enum? > > Indeed, enum would be more appropriate. > > > > > > + - items: > > > > Not a list > > > > > + - const: intel,wakeup-mailbox > > > > So every vendor is supposed to come with different name for the same > > feature? Or is wakeup-mailnox really intel specific, but then specific > > to which processors? > > It would not be necessary for every vendor to provide a different name for > the same feature. I saw, however that the Devicetree specification requires > a [vendor],[method] stringlist. Indeed, it's fine then. > > Also, platform firmware for any processor could implement the wakeup > mailbox. > > > > > > > > + description: | > > > + CPUs are woken up using the mailbox mechanism. The platform > > > + firmware boots the secondary CPUs and puts them in a state > > > + to check the mailbox for a wakeup command from the operating > > > + system. > > > + > > > +required: > > > + - reg > > > + - compatible > > > + > > > +unevaluatedProperties: false > > > > Missing ref in top-level or this is supposed to be additionalProps. See > > example-schema. > > I will check. > > > > > > + > > > +examples: > > > + - | > > > + /* > > > + * A system with two CPUs. cpu@0 is the bootstrap CPU and its status is > > > + * "okay". It does not have the enable-method property. cpu@1 is a > > > + * secondary CPU. Its status is "disabled" and defines the enable-method > > > + * property. > > > + */ > > > + > > > + cpus { > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + > > > + cpu@0 { > > > + reg = <0x0 0x1>; > > > + compatible = "intel,x86"; > > > + status = "okay"; > > > > Drop > > I will drop status = "okay" > > > > > > + }; > > > + > > > + cpu@1 { > > > + reg = <0x0 0x1>; > > > + compatible = "intel,x86"; > > > + status = "disabled"; > > > > Why? > > Because this is a secondary CPU that the operating system will enable using > the method specified in the `enable-method` property. OK, so this was intentional to express it is in quiescent state. Best regards, Krzysztof
On Tue, May 06, 2025 at 09:25:59AM +0200, Krzysztof Kozlowski wrote: > On Mon, May 05, 2025 at 09:52:35PM GMT, Ricardo Neri wrote: > > On Sun, May 04, 2025 at 06:45:59PM +0200, Krzysztof Kozlowski wrote: > > > On Sat, May 03, 2025 at 12:15:06PM GMT, Ricardo Neri wrote: > > > > Add bindings for CPUs in x86 architecture. Start by defining the `reg` and > > > > > > What for? > > > > Thank you for your quick feedback, Krzysztof! > > > > Do you mean for what reason I want to start bindings for x86 CPUs? Or only > > Yes. For which devices, what purpose. Sure, I could expand on this. > > > the `reg` property? If the former, it is to add an enable-method property to > > x86 CPUs. If the latter, is to show the relationship between APIC and `reg`. > > > > > > > > > `enable-method` properties and their relationship to x86 APIC ID and the > > > > available mechanisms to boot secondary CPUs. > > > > > > > > Start defining bindings for Intel processors. Bindings for other vendors > > > > can be added later as needed. > > > > > > > > Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com> > > > > --- > > > > > > Not really tested so only limited review follows. > > > > Sorry, I ran make dt_binding_check but only on this schema. I missed the > > reported error. > > > > > > > > > .../devicetree/bindings/x86/cpus.yaml | 80 +++++++++++++++++++ > > > > 1 file changed, 80 insertions(+) > > > > create mode 100644 Documentation/devicetree/bindings/x86/cpus.yaml > > > > > > > > diff --git a/Documentation/devicetree/bindings/x86/cpus.yaml b/Documentation/devicetree/bindings/x86/cpus.yaml > > > > new file mode 100644 > > > > index 000000000000..108b3ad64aea > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/x86/cpus.yaml > > > > @@ -0,0 +1,80 @@ > > > > +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause > > > > +%YAML 1.2 > > > > +--- > > > > +$id: http://devicetree.org/schemas/x86/cpus.yaml# > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > + > > > > +title: x86 CPUs > > > > + > > > > +maintainers: > > > > + - Ricardo Neri <ricardo.neri-calderon@linux.intel.com> > > > > + > > > > +description: | > > > > + Description of x86 CPUs in a system through the "cpus" node. > > > > + > > > > + Detailed information about the CPU architecture can be found in the Intel > > > > + Software Developer's Manual: > > > > + https://intel.com/sdm > > > > + > > > > +properties: > > > > + compatible: > > > > + enum: > > > > + - intel,x86 > > > > > > That's architecture, not a CPU. CPUs are like 80286, 80386, so that's > > > not even specific instruction set. I don't get what you need it for. > > > > Am I to understand the the `compatible` property is not needed if the > > bindings apply to any x86 CPU? > > Every device needs compatible. Its meaning is explained: > https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#compatible > > If you add here a device representing CPU, then look at existing > bindings for CPUs how they do it. > > It again feels like you add DT for platform which is not a real thing. That is correct. I struggle to enumerate specific CPUs because the `intel, wakeup-mailbox` enable method is implemented in the platform firmware and is not tied to a given processor model as required by the rules of the `compatible` property. > If you use DT, you do not get different rules, therefore read all > standard guides and tutorials (there were many, quite comprehensive). I went through various materials. Perhaps I needed to understand the rules better. I realize now the DeviceTree is about describing hardware not firmware and DT bindings are a suitable vehicle for this. Thanks for the time you spent reviewing this patchset! BR, Ricardo
diff --git a/Documentation/devicetree/bindings/x86/cpus.yaml b/Documentation/devicetree/bindings/x86/cpus.yaml new file mode 100644 index 000000000000..108b3ad64aea --- /dev/null +++ b/Documentation/devicetree/bindings/x86/cpus.yaml @@ -0,0 +1,80 @@ +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/x86/cpus.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: x86 CPUs + +maintainers: + - Ricardo Neri <ricardo.neri-calderon@linux.intel.com> + +description: | + Description of x86 CPUs in a system through the "cpus" node. + + Detailed information about the CPU architecture can be found in the Intel + Software Developer's Manual: + https://intel.com/sdm + +properties: + compatible: + enum: + - intel,x86 + + reg: + description: | + Local APIC ID of the CPU. If the CPU has more than one execution thread, + then the property is an array with one element per thread. + + enable-method: + $ref: /schemas/types.yaml#/definitions/string + description: | + The method used to wake up secondary CPUs. This property is not needed if + the secondary processors are booted using INIT assert, de-assert followed + by Start-Up IPI messages as described in the Volume 3, Section 11.4 of + Intel Software Developer's Manual. + + It is also optional for the bootstrap CPU. + + oneOf: + - items: + - const: intel,wakeup-mailbox + description: | + CPUs are woken up using the mailbox mechanism. The platform + firmware boots the secondary CPUs and puts them in a state + to check the mailbox for a wakeup command from the operating + system. + +required: + - reg + - compatible + +unevaluatedProperties: false + +examples: + - | + /* + * A system with two CPUs. cpu@0 is the bootstrap CPU and its status is + * "okay". It does not have the enable-method property. cpu@1 is a + * secondary CPU. Its status is "disabled" and defines the enable-method + * property. + */ + + cpus { + #address-cells = <1>; + #size-cells = <0>; + + cpu@0 { + reg = <0x0 0x1>; + compatible = "intel,x86"; + status = "okay"; + }; + + cpu@1 { + reg = <0x0 0x1>; + compatible = "intel,x86"; + status = "disabled"; + enable-method = "intel,wakeup-mailbox"; + }; + }; +
Add bindings for CPUs in x86 architecture. Start by defining the `reg` and `enable-method` properties and their relationship to x86 APIC ID and the available mechanisms to boot secondary CPUs. Start defining bindings for Intel processors. Bindings for other vendors can be added later as needed. Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com> --- .../devicetree/bindings/x86/cpus.yaml | 80 +++++++++++++++++++ 1 file changed, 80 insertions(+) create mode 100644 Documentation/devicetree/bindings/x86/cpus.yaml