lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240102152023.GA2821956-robh@kernel.org>
Date: Tue, 2 Jan 2024 08:20:23 -0700
From: Rob Herring <robh@...nel.org>
To: Alexander Graf <graf@...zon.com>
Cc: linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org, linux-mm@...ck.org, devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, kexec@...ts.infradead.org, linux-doc@...r.kernel.org, x86@...nel.org, Eric Biederman <ebiederm@...ssion.com>, "H. Peter Anvin" <hpa@...or.com>, Andy Lutomirski <luto@...nel.org>, Peter Zijlstra <peterz@...radead.org>, Steven Rostedt <rostedt@...dmis.org>, Andrew Morton <akpm@...ux-foundation.org>, Mark Rutland <mark.rutland@....com>, Tom Lendacky <thomas.lendacky@....com>, Ashish Kalra <ashish.kalra@....com>, James Gowans <jgowans@...zon.com>, Stanislav Kinsburskii <skinsburskii@...ux.microsoft.com>, arnd@...db.de, pbonzini@...hat.com, madvenka@...ux.microsoft.com, Anthony Yznaga <anthony.yznaga@...cle.com>, Usama Arif <usama.arif@...edance.com>, David Woodhouse <dwmw@...zon.co.uk>, Benjamin Herrenschmidt <benh@...nel.crashing.org>
Subject: Re: [PATCH v2 17/17] devicetree: Add bindings for ftrace KHO

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@...zon.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@...zon.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@...zon.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@...zon.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
> 
> 
> 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ