Message-ID: <2ecfc189-92fd-4575-a9e4-c8a5cf2923fa@amazon.co.uk>
Date: Tue, 9 Apr 2024 19:08:54 +0100
From: "Landge, Sudan" <sudanl@...zon.co.uk>
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 v4 0/5] virt: vmgenid: Add devicetree bindings support



On 09/04/2024 18:01, Sudan Landge wrote:
> This small series of patches aims to add devicetree bindings support for
> the Virtual Machine Generation ID (vmgenid).
> 
> Virtual Machine Generation ID 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.
> 
> 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
> 
> *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 choose DT over ACPI,
> we would like to highlight that the hypervisors that have chosen 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.
> 
> vmgenid exposes to the guest a 16-byte cryptographically random number,
> the value of which changes every time it starts executing from a new
> configuration (snapshot, backup, etc.). During initialization, the device
> exposes to the guest the address of the generation ID and
> an interrupt number, which the device will use to notify the guest when
> the generation ID changes.
> These attributes can be trivially communicated via device tree bindings.
> 
> We believe that adding a devicetree binding for vmgenid is a simpler
> alternative way to expose the device to the guest than forcing the
> hypervisors to implement ACPI.
> 
> Addtional notes:
> While adding the devicetree support we considered re-using existing
> structures/code to avoid duplicating code and reduce maintenance; so,
> we used the same driver to be configured either by ACPI or by DT.
> This also meant reimplementing the existing vmgenid ACPI bus driver as a
> platform driver and making it discoverable using `driver.of_match_table`
> and `driver.acpi_match_table`.
> 
> There is no user impact or change in vmgenid functionality when used
> with ACPI. We verified ACPI support of these patches on X86 and DT
> support on ARM using Firecracker hypervisor
> https://github.com/firecracker-microvm/firecracker.
> 
> To check schema and syntax errors, the bindings file is verified with:
> ```
>    make dt_binding_check \
>    DT_SCHEMA_FILES=\
>    Documentation/devicetree/bindings/rng/microsoft,vmgenid.yaml
> ```
> and the patches were verified with:
> `scripts/checkpatch.pl --strict v4-000*`.
> 
> Changelog with respect to version 3:
> - Changed the compatible string from "virtual,vmgenctr" to
>    "microsoft,vmgenid" as per review comments.
> - Renamed vmgenid.yaml to follow DT file naming convention.
> - Updated the description of properties and example in vmgenid yaml file.
> - Addressed the review comments to remove all ifdefs in vmgenid.c with one
>    exception which still needs to be under CONFIG_ACPI.
> - reformated the code with clang-format.
> - Tested code with W=1, Sparse, Smatch and Coccinelle tools.
> 
> Changelog with respect to version 2:
> - As per review comments, used platform apis instead of "of_*" APIs,
>    removed unnecessary #include and used IF_ENABLED instead of ifdef.
> - Added more info for vmgenid buffer address and corrected the formatting.
> - Replaced the compatible string from "linux,*" to "virtual,*" because,
>    the device does not have a vendor.
> 
> Changelog with respect to version 1:
> - Moved vmgenid.yaml bindings to the more related "rng" folder.
> - Removed `vmgenid_remove` to since it is unrelated to the
>    current goal of the patch.
> - Updated the cover letter and bindings commit
>    "[PATCH v2 3/4] dt-bindings: rng: Add vmgenid support" to
>    provide more information on vmgenid.
> - Compiled with and without CONFIG_OF/CONFIG_ACPI and fixed
>    compilers errors/warnings.
> 
> 
> Sudan Landge (5):
>    virt: vmgenid: rearrange code to make review easier
>    virt: vmgenid: change implementation to use a platform driver
>    virt: vmgenid: enable driver regardless of ACPI config
>    dt-bindings: rng: Add vmgenid support
>    virt: vmgenid: add support for devicetree bindings
> 
>   .../bindings/rng/microsoft,vmgenid.yaml       |  49 +++++
>   MAINTAINERS                                   |   1 +
>   drivers/virt/Kconfig                          |   1 -
>   drivers/virt/vmgenid.c                        | 168 ++++++++++++++----
>   4 files changed, 180 insertions(+), 39 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/rng/microsoft,vmgenid.yaml
> 
> 
> base-commit: 20cb38a7af88dc40095da7c2c9094da3873fea23

Due to a mail server error the last patch of this series, "Re: [PATCH v4 
5/5] virt: vmgenid: add support for devicetree bindings", got sent 
separately. Apologies for this. I will resend the patches to correct 
this issue so, I request reviewers to please ignore this patch.

Powered by blists - more mailing lists