linux-kernel - Re: [PATCH v1 3/4] dt-bindings: Add bindings for vmgenid

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: <ba8418ab-2829-416c-8e20-414f7818cab9@linaro.org>
Date: Tue, 19 Mar 2024 16:28:12 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Sudan Landge <sudanl@...zon.com>, tytso@....edu, Jason@...c4.com,
 robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
 sathyanarayanan.kuppuswamy@...ux.intel.com, thomas.lendacky@....com,
 dan.j.williams@...el.com, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org
Cc: graf@...zon.de, dwmw@...zon.co.uk, bchalios@...zon.es,
 xmarcalx@...zon.co.uk
Subject: Re: [PATCH v1 3/4] dt-bindings: Add bindings for vmgenid

On 19/03/2024 15:32, Sudan Landge wrote:
> Virtual Machine Generation ID driver was introduced in commit af6b54e2b5ba
> ("virt: vmgenid: notify RNG of VM fork and supply generation ID"), as an
> ACPI only device.

That's not a valid rationale. Second today... we do not add things to
bindings just because someone added some crazy or not crazy idea to Linux.

Bindings represent the hardware.

Please come with real rationale. Even if this is accepted, above reason
is just wrong and will be used as an excuse to promote more crap into
bindings.

A nit, subject: drop second/last, redundant "bindings". The
"dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18

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.

> 
> Add a devicetree binding support for vmgenid so that hypervisors
> can support vmgenid without the need to support ACPI.

Devicetree is not for virtual platforms. Virtual platform can define
whatever interface they want (virtio, ACPI, "VTree" (just invented now)).

> 
> Signed-off-by: Sudan Landge <sudanl@...zon.com>
> ---
>  .../devicetree/bindings/vmgenid/vmgenid.yaml  | 57 +++++++++++++++++++

No, you do not get your own hardware subsystem. Use existing ones.

>  MAINTAINERS                                   |  1 +
>  2 files changed, 58 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/vmgenid/vmgenid.yaml
> 
> diff --git a/Documentation/devicetree/bindings/vmgenid/vmgenid.yaml b/Documentation/devicetree/bindings/vmgenid/vmgenid.yaml
> new file mode 100644
> index 000000000000..17773aa96f8b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/vmgenid/vmgenid.yaml
> @@ -0,0 +1,57 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/vmgenid/vmgenid.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Virtual Machine Generation Counter ID device.

Titles are not sentences. Drop full stop.

> +
> +maintainers:
> +  - Jason A. Donenfeld <Jason@...c4.com>
> +
> +description: |+

Drop |+

> +  Firmwares or hypervisors can use this devicetree to describe
> +  interrupts and the shared resources to inject a Virtual Machine Generation
> +  counter.
> +
> +properties:
> +  compatible:
> +    const: linux,vmgenctr
> +
> +  "#interrupt-cells":
> +    const: 3
> +    description: |
> +      The 1st cell is the interrupt type.
> +      The 2nd cell contains the interrupt number for the interrupt type.
> +      The 3rd cell is for trigger type and level flags.
> +
> +  interrupt-controller: true
> +
> +  reg:
> +    description: |
> +      specifies the base physical address and
> +      size of the regions in memory which holds the VMGenID counter.
> +    maxItems: 1
> +
> +  interrupts:
> +    description: |
> +      interrupt used to notify that a new VMGenID counter is available.
> +      The interrupt should be Edge triggered.
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    vmgenid@...00000 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

No generic name? Kind of, because *it is not a real thing*.

Best regards,
Krzysztof