Re: [PATCH v3 3/4] dt-bindings: rng: Add vmgenid support

[Rob Herring]

On Mon, Mar 25, 2024 at 2:53 PM Sudan Landge <sudanl@xxxxxxxxxx> wrote:
>

Please give time for discussions on prior versions to finish and
others to comment. We're not all in one timezone and are busy. I've
replied there too.

> 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.
>
> VMGenID specification http://go.microsoft.com/fwlink/?LinkId=260709 defines
> a mechanism for the BIOS/hypervisors to communicate to the virtual machine
> that it is executed with a different configuration (e.g. snapshot execution
> or creation from a template).
> The guest operating system can use the notification for various purposes
> such as re-initializing its random number generator etc.
>
> As per the specs, hypervisor should provide a globally unique identified,
> or GUID via ACPI.
>
> This patch tries to mimic the mechanism to provide the same functionality
> which is for a hypervisor/BIOS to notify the virtual machine when it is
> executed with a different configuration.
>
> As part of this support the devicetree bindings requires the hypervisors or
> BIOS to provide a memory address which holds the GUID and an IRQ which is
> used to notify when there is a change in the GUID.
> The memory exposed in the DT should follow the rules defined in the
> vmgenid spec mentioned above.
>
> *Reason for this change*:
> Chosing ACPI or devicetree is an intrinsic part of an hypervisor design.
> Without going into details of why a hypervisor would chose DT over ACPI,
> we would like to highlight that the hypervisors that have chose devicetree
> and now want to make use of the vmgenid functionality cannot do so today
> because vmgenid is an ACPI only device.
> This forces these hypervisors to change their design which could have
> undesirable impacts on their use-cases, test-scenarios etc.
>
> The point of vmgenid is to provide a mechanism to discover a GUID when
> the execution state of a virtual machine changes and the simplest
> way to do it is pass a memory location and an interrupt via devicetree.
> It would complicate things unnecessarily if instead of using devicetree,
> we try to implement a new protocol or modify other protocols to somehow
> provide the same functionility.
>
> We believe that adding a devicetree binding for vmgenid is a simpler,
> better alternative to provide the same functionality and will allow
> such hypervisors as mentioned above to continue using devicetree.
>
> More references to vmgenid specs:
>  - https://www.qemu.org/docs/master/specs/vmgenid.html
>  - https://learn.microsoft.com/en-us/windows/win32/hyperv_v2/virtual-
> machine-generation-identifier
>
> Signed-off-by: Sudan Landge <sudanl@xxxxxxxxxx>
> ---
>  .../devicetree/bindings/rng/vmgenid.yaml      | 58 +++++++++++++++++++

Filename should match the compatible, whatever that ends up being.

>  MAINTAINERS                                   |  1 +
>  2 files changed, 59 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/rng/vmgenid.yaml
>
> diff --git a/Documentation/devicetree/bindings/rng/vmgenid.yaml b/Documentation/devicetree/bindings/rng/vmgenid.yaml
> new file mode 100644
> index 000000000000..24643080d6b0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rng/vmgenid.yaml
> @@ -0,0 +1,58 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/rng/vmgenid.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Virtual Machine Generation Counter ID device
> +
> +maintainers:
> +  - Jason A. Donenfeld <Jason@xxxxxxxxx>
> +
> +description:
> +  Firmwares or hypervisors can use this devicetree to describe
> +  interrupts and the shared resources to inject a Virtual Machine Generation
> +  counter.
> +
> +properties:
> +  compatible:
> +    const: virtual,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-map: true

Sigh. What makes this an interrupt-map? Why do you think you need this
and #interrupt-cells? You don't have them in the example.

> +
> +  reg:
> +    description:
> +      The 1st cell specifies the base physical address of the 8-byte aligned
> +      buffer in guest memory space which is guaranteed not to be used by the
> +      operating system.
> +      The 2nd cell specifies the size of the buffer which holds the VMGenID.

I didn't ask for you to explain the purpose of cells in 'reg' as that
is the same for *every* instance of 'reg'. Ignore DTisms and describe
the format of the registers. For example, is it 4 32-bit registers
(hex) or 9 32-bit registers (ascii)?

> +    maxItems: 1
> +
> +  interrupts:
> +    description:
> +      interrupt used to notify that a new VMGenID counter is available.
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    rng@80000000 {
> +      compatible = "virtual,vmgenctr";
> +      reg = <0x80000000 0x1000>;
> +      interrupts = <0x00 0x23 0x01>;
> +    };
> +
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index de6a64b248ae..e295d2f50af4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18461,6 +18461,7 @@ M:      "Theodore Ts'o" <tytso@xxxxxxx>
>  M:     Jason A. Donenfeld <Jason@xxxxxxxxx>
>  S:     Maintained
>  T:     git https://git.kernel.org/pub/scm/linux/kernel/git/crng/random.git
> +F:     Documentation/devicetree/bindings/rng/vmgenid.yaml
>  F:     drivers/char/random.c
>  F:     drivers/virt/vmgenid.c
>
> --
> 2.40.1
>
>