| Message ID | 20231222195144.24532-12-graf@amazon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On Fri, 22 Dec 2023 19:51:44 +0000, Alexander Graf wrote: > With ftrace in KHO, we are creating an ABI between old kernel and new > kernel about the state that they transfer. To ensure that we document > that state and catch any breaking change, let's add its schema to the > common devicetree bindings. This way, we can quickly reason about the > state that gets passed. > > Signed-off-by: Alexander Graf <graf@amazon.com> > --- > .../bindings/kho/ftrace/ftrace-array.yaml | 46 +++++++++++++++ > .../bindings/kho/ftrace/ftrace-cpu.yaml | 56 +++++++++++++++++++ > .../bindings/kho/ftrace/ftrace.yaml | 48 ++++++++++++++++ > 3 files changed, 150 insertions(+) > create mode 100644 Documentation/devicetree/bindings/kho/ftrace/ftrace-array.yaml > create mode 100644 Documentation/devicetree/bindings/kho/ftrace/ftrace-cpu.yaml > create mode 100644 Documentation/devicetree/bindings/kho/ftrace/ftrace.yaml > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: ./Documentation/devicetree/bindings/kho/ftrace/ftrace-array.yaml:43:111: [warning] line too long (117 > 110 characters) (line-length) ./Documentation/devicetree/bindings/kho/ftrace/ftrace-cpu.yaml:53:111: [warning] line too long (117 > 110 characters) (line-length) ./Documentation/devicetree/bindings/kho/ftrace/ftrace.yaml:45:111: [warning] line too long (117 > 110 characters) (line-length) dtschema/dtc warnings/errors: WARNING: Documentation/devicetree/bindings/kho/ftrace/ftrace-array.example.dts:29.25-39: Value 0x0000000101000000 truncated to 0x01000000 WARNING: Documentation/devicetree/bindings/kho/ftrace/ftrace-array.example.dts:29.48-62: Value 0x0000000101000100 truncated to 0x01000100 WARNING: Documentation/devicetree/bindings/kho/ftrace/ftrace-array.example.dts:29.73-87: Value 0x0000000101000038 truncated to 0x01000038 WARNING: Documentation/devicetree/bindings/kho/ftrace/ftrace-array.example.dts:29.96-110: Value 0x0000000101002000 truncated to 0x01002000 WARNING: Documentation/devicetree/bindings/kho/ftrace/ftrace-cpu.example.dts:29.25-39: Value 0x0000000101000000 truncated to 0x01000000 WARNING: Documentation/devicetree/bindings/kho/ftrace/ftrace-cpu.example.dts:29.48-62: Value 0x0000000101000100 truncated to 0x01000100 WARNING: Documentation/devicetree/bindings/kho/ftrace/ftrace-cpu.example.dts:29.73-87: Value 0x0000000101000038 truncated to 0x01000038 WARNING: Documentation/devicetree/bindings/kho/ftrace/ftrace-cpu.example.dts:29.96-110: Value 0x0000000101002000 truncated to 0x01002000 WARNING: Documentation/devicetree/bindings/kho/ftrace/ftrace.example.dts:29.25-39: Value 0x0000000101000000 truncated to 0x01000000 WARNING: Documentation/devicetree/bindings/kho/ftrace/ftrace.example.dts:29.48-62: Value 0x0000000101000100 truncated to 0x01000100 WARNING: Documentation/devicetree/bindings/kho/ftrace/ftrace.example.dts:29.73-87: Value 0x0000000101000038 truncated to 0x01000038 WARNING: Documentation/devicetree/bindings/kho/ftrace/ftrace.example.dts:29.96-110: Value 0x0000000101002000 truncated to 0x01002000 doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231222195144.24532-12-graf@amazon.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On 22/12/2023 20:51, Alexander Graf wrote: > With ftrace in KHO, we are creating an ABI between old kernel and new > kernel about the state that they transfer. To ensure that we document > that state and catch any breaking change, let's add its schema to the > common devicetree bindings. This way, we can quickly reason about the > state that gets passed. Please use scripts/get_maintainers.pl to get a list of necessary people and lists to CC (and consider --no-git-fallback argument). It might happen, that command when run on an older kernel, gives you outdated entries. Therefore please be sure you base your patches on recent Linux kernel. Please use subject prefixes matching the subsystem. You can get them for example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory your patch is touching. A nit, subject: drop second/last, redundant "bindings". The "dt-bindings" prefix is already stating that these are bindings. > > Signed-off-by: Alexander Graf <graf@amazon.com> > --- > .../bindings/kho/ftrace/ftrace-array.yaml | 46 +++++++++++++++ > .../bindings/kho/ftrace/ftrace-cpu.yaml | 56 +++++++++++++++++++ > .../bindings/kho/ftrace/ftrace.yaml | 48 ++++++++++++++++ > 3 files changed, 150 insertions(+) > create mode 100644 Documentation/devicetree/bindings/kho/ftrace/ftrace-array.yaml > create mode 100644 Documentation/devicetree/bindings/kho/ftrace/ftrace-cpu.yaml > create mode 100644 Documentation/devicetree/bindings/kho/ftrace/ftrace.yaml > > diff --git a/Documentation/devicetree/bindings/kho/ftrace/ftrace-array.yaml b/Documentation/devicetree/bindings/kho/ftrace/ftrace-array.yaml > new file mode 100644 > index 000000000000..9960fefc292d > --- /dev/null > +++ b/Documentation/devicetree/bindings/kho/ftrace/ftrace-array.yaml > @@ -0,0 +1,46 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/kho/ftrace/ftrace-array.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Ftrace trace array > + Missing description. Commit msg also does not tell me much. This must stand on its own and must describe the hardware. Whatever you have in cover letter, does not matter, especially that you did not Cc us on it. > +maintainers: > + - Alexander Graf <graf@amazon.com> > + > +properties: > + compatible: > + enum: > + - ftrace,array-v1 > + > + trace_flags: Underscores are not allowed. Does not look like generic property. > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: > + Bitmap of all the trace flags that were enabled in the trace array at the > + point of serialization. > + > +# Subnodes will be of type "ftrace,cpu-v1", one each per CPU > +additionalProperties: true No, this must be false. And it goes after required: > + > +required: > + - compatible > + - trace_flags > + > +examples: > + - | > + ftrace { > + compatible = "ftrace-v1"; > + events = <1 1 2 2 3 3>; > + > + global_trace { Again, no underscores. > + compatible = "ftrace,array-v1"; > + trace_flags = < 0x3354601 >; > + > + cpu0 { > + compatible = "ftrace,cpu-v1"; > + cpu = < 0x00 >; Drop redundant spaces. > + mem = < 0x101000000ULL 0x38ULL 0x101000100ULL 0x1000ULL 0x101000038ULL 0x38ULL 0x101002000ULL 0x1000ULL>; ? Do you see any of such syntax in DTS? Best regards, Krzysztof
Hi Krzysztof! Thanks a lot for the fast review! On 23.12.23 15:30, Krzysztof Kozlowski wrote: > On 22/12/2023 20:51, Alexander Graf wrote: >> With ftrace in KHO, we are creating an ABI between old kernel and new >> kernel about the state that they transfer. To ensure that we document >> that state and catch any breaking change, let's add its schema to the >> common devicetree bindings. This way, we can quickly reason about the >> state that gets passed. > Please use scripts/get_maintainers.pl to get a list of necessary people > and lists to CC (and consider --no-git-fallback argument). It might > happen, that command when run on an older kernel, gives you outdated > entries. Therefore please be sure you base your patches on recent Linux > kernel. Ah, this is about directly CC'ing maintainers? I was slightly picky on CCs since the CC list is already a bit long for this patch set, so I limited the CC list to mailing lists and people that I know were directly interested. Happy to CC you next time. > > Please use subject prefixes matching the subsystem. You can get them for > example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory > your patch is touching. > > A nit, subject: drop second/last, redundant "bindings". The > "dt-bindings" prefix is already stating that these are bindings. Happy to fix up for v3 :) > >> Signed-off-by: Alexander Graf <graf@amazon.com> >> --- >> .../bindings/kho/ftrace/ftrace-array.yaml | 46 +++++++++++++++ >> .../bindings/kho/ftrace/ftrace-cpu.yaml | 56 +++++++++++++++++++ >> .../bindings/kho/ftrace/ftrace.yaml | 48 ++++++++++++++++ >> 3 files changed, 150 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/kho/ftrace/ftrace-array.yaml >> create mode 100644 Documentation/devicetree/bindings/kho/ftrace/ftrace-cpu.yaml >> create mode 100644 Documentation/devicetree/bindings/kho/ftrace/ftrace.yaml >> >> diff --git a/Documentation/devicetree/bindings/kho/ftrace/ftrace-array.yaml b/Documentation/devicetree/bindings/kho/ftrace/ftrace-array.yaml >> new file mode 100644 >> index 000000000000..9960fefc292d >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/kho/ftrace/ftrace-array.yaml >> @@ -0,0 +1,46 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/kho/ftrace/ftrace-array.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Ftrace trace array >> + > Missing description. Commit msg also does not tell me much. This must > stand on its own and must describe the hardware. Whatever you have in > cover letter, does not matter, especially that you did not Cc us on it. Alrighty, I'll add descriptions and make the commit message stand on its own. For quick reference: KHO is a new mechanism this patch set introduces which allows Linux to pass arbitrary memory and metadata between kernels on kexec. I'm reusing FDTs to implement the hand over protocol, as Linux-to-Linux boot communication holds very similar properties to firmware-to-Linux boot communication. So this binding is not about hardware; it's about preserving Linux subsystem state across kexec. For more details, please refer to the KHO documentation which is part of patch 7 of this patch set: https://lore.kernel.org/lkml/20231222195144.24532-2-graf@amazon.com/ > >> +maintainers: >> + - Alexander Graf <graf@amazon.com> >> + >> +properties: >> + compatible: >> + enum: >> + - ftrace,array-v1 >> + >> + trace_flags: > Underscores are not allowed. Does not look like generic property. Let me make it "trace-flags" to not have underscores. Could you please elaborate on what you mean by generic property? > > >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + description: >> + Bitmap of all the trace flags that were enabled in the trace array at the >> + point of serialization. >> + >> +# Subnodes will be of type "ftrace,cpu-v1", one each per CPU >> +additionalProperties: true > No, this must be false. And it goes after required: Ok, making it false and adding pattern matches instead for subnodes. > > >> + >> +required: >> + - compatible >> + - trace_flags >> + >> +examples: >> + - | >> + ftrace { >> + compatible = "ftrace-v1"; >> + events = <1 1 2 2 3 3>; >> + >> + global_trace { > Again, no underscores. Ok :) > >> + compatible = "ftrace,array-v1"; >> + trace_flags = < 0x3354601 >; >> + >> + cpu0 { >> + compatible = "ftrace,cpu-v1"; >> + cpu = < 0x00 >; > Drop redundant spaces. I don't understand what you're referring to as redundant spaces? Double checking, I believe indentation is off for every line below "ftrace {". Is that what you're referring to? Fixing :) > >> + mem = < 0x101000000ULL 0x38ULL 0x101000100ULL 0x1000ULL 0x101000038ULL 0x38ULL 0x101002000ULL 0x1000ULL>; > ? Do you see any of such syntax in DTS? I was trying to make it easy to reason to readers about 64bit numbers - and then potentially extend dtc to consume that new syntax. KHO DTs are native/little endian, so dtc already has some difficulties interpreting it which I'll need to fix up with patches to it eventually :). I'll change it to something that looks more 32bit'y for now. Alex Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
On 24/12/2023 00:20, Alexander Graf wrote: >>> new file mode 100644 >>> index 000000000000..9960fefc292d >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/kho/ftrace/ftrace-array.yaml >>> @@ -0,0 +1,46 @@ >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/kho/ftrace/ftrace-array.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: Ftrace trace array >>> + >> Missing description. Commit msg also does not tell me much. This must >> stand on its own and must describe the hardware. Whatever you have in >> cover letter, does not matter, especially that you did not Cc us on it. > > > Alrighty, I'll add descriptions and make the commit message stand on its > own. > > For quick reference: KHO is a new mechanism this patch set introduces > which allows Linux to pass arbitrary memory and metadata between kernels > on kexec. I'm reusing FDTs to implement the hand over protocol, as > Linux-to-Linux boot communication holds very similar properties to > firmware-to-Linux boot communication. So this binding is not about > hardware; it's about preserving Linux subsystem state across kexec. Devicetree is for non-discoverable systems and their hardware, not for passing arbitrary data between kernels. For me this does not suit DT at all, please use other ways. > > For more details, please refer to the KHO documentation which is part of > patch 7 of this patch set: > https://lore.kernel.org/lkml/20231222195144.24532-2-graf@amazon.com/ > > >> >>> +maintainers: >>> + - Alexander Graf <graf@amazon.com> >>> + >>> +properties: >>> + compatible: >>> + enum: >>> + - ftrace,array-v1 >>> + >>> + trace_flags: >> Underscores are not allowed. Does not look like generic property. > > > Let me make it "trace-flags" to not have underscores. Could you please > elaborate on what you mean by generic property? Generic property, so one without vendor prefix, is shared and common to a group of devices. > > >> >> >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + description: >>> + Bitmap of all the trace flags that were enabled in the trace array at the >>> + point of serialization. >>> + >>> +# Subnodes will be of type "ftrace,cpu-v1", one each per CPU >>> +additionalProperties: true >> No, this must be false. And it goes after required: > > > Ok, making it false and adding pattern matches instead for subnodes. > > >> >> >>> + >>> +required: >>> + - compatible >>> + - trace_flags >>> + >>> +examples: >>> + - | >>> + ftrace { >>> + compatible = "ftrace-v1"; >>> + events = <1 1 2 2 3 3>; >>> + >>> + global_trace { >> Again, no underscores. > > > Ok :) > > >> >>> + compatible = "ftrace,array-v1"; >>> + trace_flags = < 0x3354601 >; >>> + >>> + cpu0 { >>> + compatible = "ftrace,cpu-v1"; >>> + cpu = < 0x00 >; >> Drop redundant spaces. > > > I don't understand what you're referring to as redundant spaces? Double > checking, I believe indentation is off for every line below "ftrace {". > Is that what you're referring to? Fixing :) Open DTS, some recent, arm64 like Qualcomm. Do you see spaces around <>? Or open the coding style document... Please do not introduce different coding style. > > >> >>> + mem = < 0x101000000ULL 0x38ULL 0x101000100ULL 0x1000ULL 0x101000038ULL 0x38ULL 0x101002000ULL 0x1000ULL>; >> ? Do you see any of such syntax in DTS? > > > I was trying to make it easy to reason to readers about 64bit numbers - 64bit numbers are not a problem for DTS reading. Above syntax is. > and then potentially extend dtc to consume that new syntax. KHO DTs are > native/little endian, so dtc already has some difficulties interpreting > it which I'll need to fix up with patches to it eventually :). I'll > change it to something that looks more 32bit'y for now. > Best regards, Krzysztof
On Sun, Dec 24, 2023 at 12:20:17AM +0100, Alexander Graf wrote: > Hi Krzysztof! > > Thanks a lot for the fast review! > > On 23.12.23 15:30, Krzysztof Kozlowski wrote: > > On 22/12/2023 20:51, Alexander Graf wrote: > > > With ftrace in KHO, we are creating an ABI between old kernel and new > > > kernel about the state that they transfer. To ensure that we document > > > that state and catch any breaking change, let's add its schema to the > > > common devicetree bindings. This way, we can quickly reason about the > > > state that gets passed. > > Please use scripts/get_maintainers.pl to get a list of necessary people > > and lists to CC (and consider --no-git-fallback argument). It might > > happen, that command when run on an older kernel, gives you outdated > > entries. Therefore please be sure you base your patches on recent Linux > > kernel. [...] > > > > > + mem = < 0x101000000ULL 0x38ULL 0x101000100ULL 0x1000ULL 0x101000038ULL 0x38ULL 0x101002000ULL 0x1000ULL>; > > ? Do you see any of such syntax in DTS? > > > I was trying to make it easy to reason to readers about 64bit numbers - and > then potentially extend dtc to consume that new syntax. KHO DTs are > native/little endian, so dtc already has some difficulties interpreting it > which I'll need to fix up with patches to it eventually :). I'll change it > to something that looks more 32bit'y for now. "/bits/ 64 <0x0 ...>" is what you are looking for. Rob
On Fri, Dec 22, 2023 at 07:51:44PM +0000, Alexander Graf wrote: > With ftrace in KHO, we are creating an ABI between old kernel and new > kernel about the state that they transfer. To ensure that we document > that state and catch any breaking change, let's add its schema to the > common devicetree bindings. This way, we can quickly reason about the > state that gets passed. Why so much data in DT rather than putting all this information into memory in your own data structure and DT just has a single property pointing to that? That's what is done with every other blob of data passed by kexec. > > Signed-off-by: Alexander Graf <graf@amazon.com> > --- > .../bindings/kho/ftrace/ftrace-array.yaml | 46 +++++++++++++++ > .../bindings/kho/ftrace/ftrace-cpu.yaml | 56 +++++++++++++++++++ > .../bindings/kho/ftrace/ftrace.yaml | 48 ++++++++++++++++ > 3 files changed, 150 insertions(+) > create mode 100644 Documentation/devicetree/bindings/kho/ftrace/ftrace-array.yaml > create mode 100644 Documentation/devicetree/bindings/kho/ftrace/ftrace-cpu.yaml > create mode 100644 Documentation/devicetree/bindings/kho/ftrace/ftrace.yaml > > diff --git a/Documentation/devicetree/bindings/kho/ftrace/ftrace-array.yaml b/Documentation/devicetree/bindings/kho/ftrace/ftrace-array.yaml > new file mode 100644 > index 000000000000..9960fefc292d > --- /dev/null > +++ b/Documentation/devicetree/bindings/kho/ftrace/ftrace-array.yaml > @@ -0,0 +1,46 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/kho/ftrace/ftrace-array.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Ftrace trace array > + > +maintainers: > + - Alexander Graf <graf@amazon.com> > + > +properties: > + compatible: > + enum: > + - ftrace,array-v1 > + > + trace_flags: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: > + Bitmap of all the trace flags that were enabled in the trace array at the > + point of serialization. > + > +# Subnodes will be of type "ftrace,cpu-v1", one each per CPU This can be expressed as a schema. > +additionalProperties: true > + > +required: > + - compatible > + - trace_flags > + > +examples: > + - | > + ftrace { > + compatible = "ftrace-v1"; > + events = <1 1 2 2 3 3>; > + > + global_trace { > + compatible = "ftrace,array-v1"; > + trace_flags = < 0x3354601 >; > + > + cpu0 { > + compatible = "ftrace,cpu-v1"; > + cpu = < 0x00 >; > + mem = < 0x101000000ULL 0x38ULL 0x101000100ULL 0x1000ULL 0x101000038ULL 0x38ULL 0x101002000ULL 0x1000ULL>; > + }; > + }; > + }; > diff --git a/Documentation/devicetree/bindings/kho/ftrace/ftrace-cpu.yaml b/Documentation/devicetree/bindings/kho/ftrace/ftrace-cpu.yaml > new file mode 100644 > index 000000000000..58c715e93f37 > --- /dev/null > +++ b/Documentation/devicetree/bindings/kho/ftrace/ftrace-cpu.yaml > @@ -0,0 +1,56 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/kho/ftrace/ftrace-cpu.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Ftrace per-CPU ring buffer contents > + > +maintainers: > + - Alexander Graf <graf@amazon.com> > + > +properties: > + compatible: > + enum: > + - ftrace,cpu-v1 > + > + cpu: > + $ref: /schemas/types.yaml#/definitions/uint32 'cpu' is already a defined property of type 'phandle'. While we can have multiple types for a given property name, best practice is to avoid that. The normal way to refer to a CPU would be a phandle to the CPU node, but I can see that might not make sense here. "CPU numbers" on arm64 are 64-bit values as well as they are the CPU's MPIDR value. > + description: > + CPU number of the CPU that this ring buffer belonged to when it was > + serialized. > + > + mem: Too vague. Make the property name indicate what's in the memory. > + $ref: /schemas/types.yaml#/definitions/uint32-array > + description: > + Array of { u64 phys_addr, u64 len } elements that describe a list of ring > + buffer pages. Each page consists of two elements. The first element > + describes the location of the struct buffer_page that contains metadata > + for a given ring buffer page, such as the ring's head indicator. The > + second element points to the ring buffer data page which contains the raw > + trace data. > + > +additionalProperties: false > + > +required: > + - compatible > + - cpu > + - mem > + > +examples: > + - | > + ftrace { > + compatible = "ftrace-v1"; > + events = <1 1 2 2 3 3>; > + > + global_trace { > + compatible = "ftrace,array-v1"; > + trace_flags = < 0x3354601 >; > + > + cpu0 { > + compatible = "ftrace,cpu-v1"; > + cpu = < 0x00 >; > + mem = < 0x101000000ULL 0x38ULL 0x101000100ULL 0x1000ULL 0x101000038ULL 0x38ULL 0x101002000ULL 0x1000ULL>; > + }; > + }; > + }; > diff --git a/Documentation/devicetree/bindings/kho/ftrace/ftrace.yaml b/Documentation/devicetree/bindings/kho/ftrace/ftrace.yaml > new file mode 100644 > index 000000000000..b87a64843af3 > --- /dev/null > +++ b/Documentation/devicetree/bindings/kho/ftrace/ftrace.yaml > @@ -0,0 +1,48 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/kho/ftrace/ftrace.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Ftrace core data > + > +maintainers: > + - Alexander Graf <graf@amazon.com> > + > +properties: > + compatible: > + enum: > + - ftrace-v1 > + > + events: Again, too vague. > + $ref: /schemas/types.yaml#/definitions/uint32-array > + description: > + Array of { u32 crc, u32 type } elements. Each element contains a unique > + identifier for an event, followed by the identifier that this event had > + in the previous kernel's trace buffers. > + > +# Other child nodes will be of type "ftrace,array-v1". Each of which describe > +# a trace buffer > +additionalProperties: true > + > +required: > + - compatible > + - events > + > +examples: > + - | > + ftrace { This should go under /chosen. Show that here. Start the example with '/{' to do that and not add the usual boilerplate we add when extracting the examples. Also, we don't need 3 examples. Just do 1 complete example here. > + compatible = "ftrace-v1"; > + events = <1 1 2 2 3 3>; > + > + global_trace { > + compatible = "ftrace,array-v1"; > + trace_flags = < 0x3354601 >; > + > + cpu0 { > + compatible = "ftrace,cpu-v1"; > + cpu = < 0x00 >; > + mem = < 0x101000000ULL 0x38ULL 0x101000100ULL 0x1000ULL 0x101000038ULL 0x38ULL 0x101002000ULL 0x1000ULL>; > + }; > + }; > + }; > -- > 2.40.1 > > > > > Amazon Development Center Germany GmbH > Krausenstr. 38 > 10117 Berlin > Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss > Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B > Sitz: Berlin > Ust-ID: DE 289 237 879 > > >
Hey Rob, Thanks a lot for taking the time to review! On 02.01.24 16:20, Rob Herring wrote: > On Fri, Dec 22, 2023 at 07:51:44PM +0000, Alexander Graf wrote: >> With ftrace in KHO, we are creating an ABI between old kernel and new >> kernel about the state that they transfer. To ensure that we document >> that state and catch any breaking change, let's add its schema to the >> common devicetree bindings. This way, we can quickly reason about the >> state that gets passed. > Why so much data in DT rather than putting all this information into > memory in your own data structure and DT just has a single property > pointing to that? That's what is done with every other blob of data > passed by kexec. This is our own data structure for KHO that just happens to again contain a DT structure. The reason is simple: I want a unified, versioned, introspectable data format that is cross platform so you don't need to touch every architecture specific boot passing logic every time you want to add a tiny piece of data. > >> Signed-off-by: Alexander Graf <graf@amazon.com> >> --- >> .../bindings/kho/ftrace/ftrace-array.yaml | 46 +++++++++++++++ >> .../bindings/kho/ftrace/ftrace-cpu.yaml | 56 +++++++++++++++++++ >> .../bindings/kho/ftrace/ftrace.yaml | 48 ++++++++++++++++ >> 3 files changed, 150 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/kho/ftrace/ftrace-array.yaml >> create mode 100644 Documentation/devicetree/bindings/kho/ftrace/ftrace-cpu.yaml >> create mode 100644 Documentation/devicetree/bindings/kho/ftrace/ftrace.yaml >> >> diff --git a/Documentation/devicetree/bindings/kho/ftrace/ftrace-array.yaml b/Documentation/devicetree/bindings/kho/ftrace/ftrace-array.yaml >> new file mode 100644 >> index 000000000000..9960fefc292d >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/kho/ftrace/ftrace-array.yaml >> @@ -0,0 +1,46 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/kho/ftrace/ftrace-array.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Ftrace trace array >> + >> +maintainers: >> + - Alexander Graf <graf@amazon.com> >> + >> +properties: >> + compatible: >> + enum: >> + - ftrace,array-v1 >> + >> + trace_flags: >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + description: >> + Bitmap of all the trace flags that were enabled in the trace array at the >> + point of serialization. >> + >> +# Subnodes will be of type "ftrace,cpu-v1", one each per CPU > This can be expressed as a schema. Could you please give me a few more hints here? I'm not sure I understand how :) > >> +additionalProperties: true >> + >> +required: >> + - compatible >> + - trace_flags >> + >> +examples: >> + - | >> + ftrace { >> + compatible = "ftrace-v1"; >> + events = <1 1 2 2 3 3>; >> + >> + global_trace { >> + compatible = "ftrace,array-v1"; >> + trace_flags = < 0x3354601 >; >> + >> + cpu0 { >> + compatible = "ftrace,cpu-v1"; >> + cpu = < 0x00 >; >> + mem = < 0x101000000ULL 0x38ULL 0x101000100ULL 0x1000ULL 0x101000038ULL 0x38ULL 0x101002000ULL 0x1000ULL>; >> + }; >> + }; >> + }; >> diff --git a/Documentation/devicetree/bindings/kho/ftrace/ftrace-cpu.yaml b/Documentation/devicetree/bindings/kho/ftrace/ftrace-cpu.yaml >> new file mode 100644 >> index 000000000000..58c715e93f37 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/kho/ftrace/ftrace-cpu.yaml >> @@ -0,0 +1,56 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/kho/ftrace/ftrace-cpu.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Ftrace per-CPU ring buffer contents >> + >> +maintainers: >> + - Alexander Graf <graf@amazon.com> >> + >> +properties: >> + compatible: >> + enum: >> + - ftrace,cpu-v1 >> + >> + cpu: >> + $ref: /schemas/types.yaml#/definitions/uint32 > 'cpu' is already a defined property of type 'phandle'. While we can have > multiple types for a given property name, best practice is to avoid > that. The normal way to refer to a CPU would be a phandle to the CPU > node, but I can see that might not make sense here. > > "CPU numbers" on arm64 are 64-bit values as well as they are the > CPU's MPIDR value. Here we're looking at the Linux internal CPU numbering which I believe does not have to use the MIDR value? > >> + description: >> + CPU number of the CPU that this ring buffer belonged to when it was >> + serialized. >> + >> + mem: > Too vague. Make the property name indicate what's in the memory. "mem" is a generic property for every node in the KHO DT that contains a <u64 phys_start, u64 size> array that describes memory to pass over. I use it in generic code so that we don't need to do memory reservations individually per node. That means I can't change the name here. > >> + $ref: /schemas/types.yaml#/definitions/uint32-array >> + description: >> + Array of { u64 phys_addr, u64 len } elements that describe a list of ring >> + buffer pages. Each page consists of two elements. The first element >> + describes the location of the struct buffer_page that contains metadata >> + for a given ring buffer page, such as the ring's head indicator. The >> + second element points to the ring buffer data page which contains the raw >> + trace data. >> + >> +additionalProperties: false >> + >> +required: >> + - compatible >> + - cpu >> + - mem >> + >> +examples: >> + - | >> + ftrace { >> + compatible = "ftrace-v1"; >> + events = <1 1 2 2 3 3>; >> + >> + global_trace { >> + compatible = "ftrace,array-v1"; >> + trace_flags = < 0x3354601 >; >> + >> + cpu0 { >> + compatible = "ftrace,cpu-v1"; >> + cpu = < 0x00 >; >> + mem = < 0x101000000ULL 0x38ULL 0x101000100ULL 0x1000ULL 0x101000038ULL 0x38ULL 0x101002000ULL 0x1000ULL>; >> + }; >> + }; >> + }; >> diff --git a/Documentation/devicetree/bindings/kho/ftrace/ftrace.yaml b/Documentation/devicetree/bindings/kho/ftrace/ftrace.yaml >> new file mode 100644 >> index 000000000000..b87a64843af3 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/kho/ftrace/ftrace.yaml >> @@ -0,0 +1,48 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/kho/ftrace/ftrace.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Ftrace core data >> + >> +maintainers: >> + - Alexander Graf <graf@amazon.com> >> + >> +properties: >> + compatible: >> + enum: >> + - ftrace-v1 >> + >> + events: > Again, too vague. > >> + $ref: /schemas/types.yaml#/definitions/uint32-array >> + description: >> + Array of { u32 crc, u32 type } elements. Each element contains a unique >> + identifier for an event, followed by the identifier that this event had >> + in the previous kernel's trace buffers. >> + >> +# Other child nodes will be of type "ftrace,array-v1". Each of which describe >> +# a trace buffer >> +additionalProperties: true >> + >> +required: >> + - compatible >> + - events >> + >> +examples: >> + - | >> + ftrace { > This should go under /chosen. Show that here. Start the example with It can't go under /chosen because x86 doesn't have /chosen :). This is not a device DT, it's a KHO DT. > '/{' to do that and not add the usual boilerplate we add when extracting > the examples. What exact difference does /{ and ftrace { make? > > Also, we don't need 3 examples. Just do 1 complete example here. Great idea :) > > >> + compatible = "ftrace-v1"; >> + events = <1 1 2 2 3 3>; >> + >> + global_trace { >> + compatible = "ftrace,array-v1"; >> + trace_flags = < 0x3354601 >; >> + >> + cpu0 { >> + compatible = "ftrace,cpu-v1"; >> + cpu = < 0x00 >; >> + mem = < 0x101000000ULL 0x38ULL 0x101000100ULL 0x1000ULL 0x101000038ULL 0x38ULL 0x101002000ULL 0x1000ULL>; >> + }; >> + }; >> + }; >> -- >> 2.40.1 >> Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
diff --git a/Documentation/devicetree/bindings/kho/ftrace/ftrace-array.yaml b/Documentation/devicetree/bindings/kho/ftrace/ftrace-array.yaml new file mode 100644 index 000000000000..9960fefc292d --- /dev/null +++ b/Documentation/devicetree/bindings/kho/ftrace/ftrace-array.yaml @@ -0,0 +1,46 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/kho/ftrace/ftrace-array.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Ftrace trace array + +maintainers: + - Alexander Graf <graf@amazon.com> + +properties: + compatible: + enum: + - ftrace,array-v1 + + trace_flags: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + Bitmap of all the trace flags that were enabled in the trace array at the + point of serialization. + +# Subnodes will be of type "ftrace,cpu-v1", one each per CPU +additionalProperties: true + +required: + - compatible + - trace_flags + +examples: + - | + ftrace { + compatible = "ftrace-v1"; + events = <1 1 2 2 3 3>; + + global_trace { + compatible = "ftrace,array-v1"; + trace_flags = < 0x3354601 >; + + cpu0 { + compatible = "ftrace,cpu-v1"; + cpu = < 0x00 >; + mem = < 0x101000000ULL 0x38ULL 0x101000100ULL 0x1000ULL 0x101000038ULL 0x38ULL 0x101002000ULL 0x1000ULL>; + }; + }; + }; diff --git a/Documentation/devicetree/bindings/kho/ftrace/ftrace-cpu.yaml b/Documentation/devicetree/bindings/kho/ftrace/ftrace-cpu.yaml new file mode 100644 index 000000000000..58c715e93f37 --- /dev/null +++ b/Documentation/devicetree/bindings/kho/ftrace/ftrace-cpu.yaml @@ -0,0 +1,56 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/kho/ftrace/ftrace-cpu.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Ftrace per-CPU ring buffer contents + +maintainers: + - Alexander Graf <graf@amazon.com> + +properties: + compatible: + enum: + - ftrace,cpu-v1 + + cpu: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + CPU number of the CPU that this ring buffer belonged to when it was + serialized. + + mem: + $ref: /schemas/types.yaml#/definitions/uint32-array + description: + Array of { u64 phys_addr, u64 len } elements that describe a list of ring + buffer pages. Each page consists of two elements. The first element + describes the location of the struct buffer_page that contains metadata + for a given ring buffer page, such as the ring's head indicator. The + second element points to the ring buffer data page which contains the raw + trace data. + +additionalProperties: false + +required: + - compatible + - cpu + - mem + +examples: + - | + ftrace { + compatible = "ftrace-v1"; + events = <1 1 2 2 3 3>; + + global_trace { + compatible = "ftrace,array-v1"; + trace_flags = < 0x3354601 >; + + cpu0 { + compatible = "ftrace,cpu-v1"; + cpu = < 0x00 >; + mem = < 0x101000000ULL 0x38ULL 0x101000100ULL 0x1000ULL 0x101000038ULL 0x38ULL 0x101002000ULL 0x1000ULL>; + }; + }; + }; diff --git a/Documentation/devicetree/bindings/kho/ftrace/ftrace.yaml b/Documentation/devicetree/bindings/kho/ftrace/ftrace.yaml new file mode 100644 index 000000000000..b87a64843af3 --- /dev/null +++ b/Documentation/devicetree/bindings/kho/ftrace/ftrace.yaml @@ -0,0 +1,48 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/kho/ftrace/ftrace.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Ftrace core data + +maintainers: + - Alexander Graf <graf@amazon.com> + +properties: + compatible: + enum: + - ftrace-v1 + + events: + $ref: /schemas/types.yaml#/definitions/uint32-array + description: + Array of { u32 crc, u32 type } elements. Each element contains a unique + identifier for an event, followed by the identifier that this event had + in the previous kernel's trace buffers. + +# Other child nodes will be of type "ftrace,array-v1". Each of which describe +# a trace buffer +additionalProperties: true + +required: + - compatible + - events + +examples: + - | + ftrace { + compatible = "ftrace-v1"; + events = <1 1 2 2 3 3>; + + global_trace { + compatible = "ftrace,array-v1"; + trace_flags = < 0x3354601 >; + + cpu0 { + compatible = "ftrace,cpu-v1"; + cpu = < 0x00 >; + mem = < 0x101000000ULL 0x38ULL 0x101000100ULL 0x1000ULL 0x101000038ULL 0x38ULL 0x101002000ULL 0x1000ULL>; + }; + }; + };
With ftrace in KHO, we are creating an ABI between old kernel and new kernel about the state that they transfer. To ensure that we document that state and catch any breaking change, let's add its schema to the common devicetree bindings. This way, we can quickly reason about the state that gets passed. Signed-off-by: Alexander Graf <graf@amazon.com> --- .../bindings/kho/ftrace/ftrace-array.yaml | 46 +++++++++++++++ .../bindings/kho/ftrace/ftrace-cpu.yaml | 56 +++++++++++++++++++ .../bindings/kho/ftrace/ftrace.yaml | 48 ++++++++++++++++ 3 files changed, 150 insertions(+) create mode 100644 Documentation/devicetree/bindings/kho/ftrace/ftrace-array.yaml create mode 100644 Documentation/devicetree/bindings/kho/ftrace/ftrace-cpu.yaml create mode 100644 Documentation/devicetree/bindings/kho/ftrace/ftrace.yaml