Re: [PATCH v1 0/4] virt: vmgenid: Add devicetree bindings support

[Rob Herring]

On Fri, Mar 22, 2024 at 3:21 AM David Woodhouse <dwmw2@xxxxxxxxxxxxx> wrote:
>
> On Fri, 2024-03-22 at 06:40 +0100, Krzysztof Kozlowski wrote:
> > On 21/03/2024 18:39, Landge, Sudan wrote:
> > >
> > >
> > > On 21/03/2024 13:32, 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 Wed, Mar 20, 2024 at 04:55:45PM +0000, David Woodhouse wrote:
> > > > > On Wed, 2024-03-20 at 11:15 -0500, Rob Herring wrote:
> > > > > > On Wed, Mar 20, 2024 at 01:50:43PM +0000, David Woodhouse wrote:
> > > > > > > On Tue, 2024-03-19 at 16:24 +0100, Krzysztof Kozlowski wrote:
> > > > > > > > On 19/03/2024 15:32, Sudan Landge wrote:
> > > > > > > > > This small series of patches aims to add devicetree bindings support for
> > > > > > > > > the Virtual Machine Generation ID (vmgenid) driver.
> > > > > > > > >
> > > > > > > > > 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.
> > > > > > > > > We would like to extend vmgenid to support devicetree bindings because:
> > > > > > > > > 1. A device should not be defined as an ACPI or DT only device.
> > > > > >
> > > > > > This (and the binding patch) tells me nothing about what "Virtual
> > > > > > Machine Generation ID driver" is and isn't really justification for
> > > > > > "why".
> > > > >
> > > > > It's a reference to a memory area which the OS can use to tell whether
> > > > > it's been snapshotted and restored (or 'forked'). A future submission
> > > > > should have a reference to something like
> > > > > https://www.qemu.org/docs/master/specs/vmgenid.html or the Microsoft
> > > > > doc which is linked from there.
> > > >
> > > > That doc mentions fw_cfg for which we already have a binding. Why can't
> > > > it be used/extended here?
> > > QEMU has support for vmgenid but even they do not pass vmgenid directly
> > > to the guest kernel using fw_cfg. QEMU passes the vmgenid/UUID via
> > > fw_cfg to an intermediate UEFI firmware. This UEFI firmware, running as
> > > a guest in QEMU, reads the UUID from fw_cfg and creates ACPI tables for
> > > it. The UEFI firmware then passes the UUID information to the guest
> > > kernel via ACPI.
> > > This approach requires yet another intermediary which is UEFI firmware
> > > and adds more complexity than ACPI for minimalist hypervisors that do
> > > not have an intermediate bootloader today.
> >
> > What stops you from passing fw_cfg not to UEFI FW? BTW, no actual VM
> > name was used in your posting, but now suddenly it is a talk about QEMU.
>
> That would be possible. But not ideal.

Why not ideal?

To rephrase the question, why is it fine for UEFI to read the vmgenid
from fw_cfg, but the kernel can't use the same mechanism? The response
that you'd have to use UEFI to use fw_cfg makes no sense to me. The
only reason I can think of is just being lazy and wanting to have
minimal changes to some existing driver. It looks to me like you could
implement this entirely in userspace already with zero kernel or
binding changes. From a quick look, we already have a fw_cfg driver
exposing UUID (that's the same thing as vmgenid AIUI) to userspace,
and you can feed that back into the random pool.

I am concerned that we already have a mechanism and you want to add a
second way. When do we ever think that's a good idea? What happens on
the next piece of fw_cfg data? We add yet another binding?

> Just as exposing it via PCI
> would be possible, but not ideal. Or forcing ACPI onto the guests in
> question, and various other less efficient options.
>
> If what we're really looking at here is a hostile takeover of the DT
> bindings repository, with a blanket "No, DT is dead. Go use something
> else, preferably ACPI", than all those other options are possible. We
> *never* have to add a new binding to DT ever again. Let's just set the
> existing bindings in stone and move on.

I'll refrain from all my snarky replies to this that aren't helpful to
the discussion.

Rob