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

[Landge, Sudan]

On 25/03/2024 15:06, Rob Herring wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>
>
>
> On Thu, Mar 21, 2024 at 02:51:04AM +0000, 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.
> >
> > 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      | 57 +++++++++++++++++++
> >   MAINTAINERS                                   |  1 +
> >   2 files changed, 58 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..4b6ab62cc2ae
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/rng/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/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: linux,vmgenctr
>
> Why 'linux'? It should be named for a particular host implementation
> (and that implementation's bugs/quirks). However, this thing is simple
> enough we can perhaps avoid that here. As the interface is defined by
> Microsoft, then perhaps they should be the vendor here.
We chose "linux" because the current implementation and usage of 
devicetree was Linux specific. However, I think "virtual" would be a 
better choice than "Microsoft" since this is a generic virtual device 
that could be configured by any hypervisor or firmware not owned or 
related to Microsoft. I have updated this as part of the new version if 
it looks good. I don't have a strong opinion for "virtual" though so if 
that is the right choice as per you I can update it.

> > +
> > +  "#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
>
> Why is this device an interrupt controller?
My devicetree references I took initially were incorrect which led to 
the addition of this, I have removed this in the next version. Sorry 
about that.

> > +
> > +  reg:
> > +    description: |
> > +      specifies the base physical address and
> > +      size of the regions in memory which holds the VMGenID counter.
>
> Odd wrapping, but drop unless you have something specific to say about
> region like perhaps the layout of the registers. Or maybe thats defined
> somewhere else?
>
> Does the spec say anything about endianness or access size? DT assumes
> native endianness by default. We have properties to deal these, but
> would be better to be explicit if that's defined already.
The spec doesn't mention anything about the endianness but, I have 
updated the description with some more data.

> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    description: |
>
> Don't need '|' if no formatting.
>
> > +      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:
> > +  - |
> > +    rng@80000000 {
> > +      compatible = "linux,vmgenctr";
> > +      reg = <0x80000000 0x1000>;
> > +      interrupts = <0x00 0x23 0x01>;
> > +    };
> > +
> > +...
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 43b39956694a..cf4b2e10fb49 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -18431,6 +18431,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
Thank you for the feedback, I have pushed a new version of the patch to 
address the review comments.