| 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
| ||
|
Message-ID: <48241e64-953d-4aeb-b4cf-f7eb0e1bffbe@amazon.com>
Date: Wed, 17 Jan 2024 14:56:52 +0100
From: Alexander Graf <graf@...zon.com>
To: Rob Herring <robh@...nel.org>
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
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@...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.
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@...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.
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@...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
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
Powered by blists - more mailing lists