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

From: "Landge, Sudan" <sudanl@amazon.co.uk>
To: Rob Herring <robh@kernel.org>, Sudan Landge <sudanl@amazon.com>
Cc: <tytso@mit.edu>, <Jason@zx2c4.com>,
	<krzysztof.kozlowski+dt@linaro.org>, <conor+dt@kernel.org>,
	<sathyanarayanan.kuppuswamy@linux.intel.com>,
	<thomas.lendacky@amd.com>, <dan.j.williams@intel.com>,
	<devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<graf@amazon.de>, <dwmw@amazon.co.uk>, <bchalios@amazon.es>,
	<xmarcalx@amazon.co.uk>
Subject: Re: [PATCH v2 3/4] dt-bindings: rng: Add vmgenid support
Date: Mon, 25 Mar 2024 20:11:33 +0000	[thread overview]
Message-ID: <51403072-f5ca-450e-9206-19ca627ead11@amazon.co.uk> (raw)
In-Reply-To: <20240325150609.GA3477574-robh@kernel.org>


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@amazon.com>
>> ---
>>   .../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@zx2c4.com>
>> +
>> +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@mit.edu>
>>   M:   Jason A. Donenfeld <Jason@zx2c4.com>
>>   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.