Re: [RFC PATCH v2 04/51] KVM: guest_memfd: Introduce KVM_GMEM_CONVERT_SHARED/PRIVATE ioctls

[Sean Christopherson <seanjc@google.com>]

On Wed, May 21, 2025, Fuad Tabba wrote:
> On Wed, 21 May 2025 at 16:51, Vishal Annapurve <vannapurve@google.com> wrote:
> > On Wed, May 21, 2025 at 8:22 AM Fuad Tabba <tabba@google.com> wrote:
> > > On Wed, 21 May 2025 at 15:42, Vishal Annapurve <vannapurve@google.com> wrote:
> > > > On Wed, May 21, 2025 at 5:36 AM Fuad Tabba <tabba@google.com> wrote:
> > > > There are a bunch of complexities here, reboot sequence on x86 can be
> > > > triggered using multiple ways that I don't fully understand, but few
> > > > of them include reading/writing to "reset register" in MMIO/PCI config
> > > > space that are emulated by the host userspace directly. Host has to
> > > > know when the guest is shutting down to manage it's lifecycle.
> > >
> > > In that case, I think we need to fully understand these complexities
> > > before adding new IOCTLs. It could be that once we understand these
> > > issues, we find that we don't need these IOCTLs. It's hard to justify
> > > adding an IOCTL for something we don't understand.
> > >
> >
> > I don't understand all the ways x86 guest can trigger reboot but I do
> > know that x86 CoCo linux guest kernel triggers reset using MMIO/PCI
> > config register write that is emulated by host userspace.
> >
> > > > x86 CoCo VM firmwares don't support warm/soft reboot and even if it
> > > > does in future, guest kernel can choose a different reboot mechanism.
> > > > So guest reboot needs to be emulated by always starting from scratch.
> > > > This sequence needs initial guest firmware payload to be installed
> > > > into private ranges of guest_memfd.
> > > >
> > > > >
> > > > > Either the host doesn't (or cannot even) know that the guest is
> > > > > rebooting, in which case I don't see how having an IOCTL would help.
> > > >
> > > > Host does know that the guest is rebooting.
> > >
> > > In that case, that (i.e., the host finding out that the guest is
> > > rebooting) could trigger the conversion back to private. No need for an
> > > IOCTL.
> >
> > In the reboot scenarios, it's the host userspace finding out that the guest
> > kernel wants to reboot.
> 
> How does the host userspace find that out? If the host userspace is capable
> of finding that out, then surely KVM is also capable of finding out the same.

Nope, not on x86.  Well, not without userspace invoking a new ioctl, which would
defeat the purpose of adding these ioctls.

KVM is only responsible for emulating/virtualizing the "CPU".  The chipset, e.g.
the PCI config space, is fully owned by userspace.  KVM doesn't even know whether
or not PCI exists for the VM.  And reboot may be emulated by simply creating a
new KVM instance, i.e. even if KVM was somehow aware of the reboot request, the
change in state would happen in an entirely new struct kvm.

That said, Vishal and Ackerley, this patch is a bit lacking on the documentation
front.  The changelog asserts that:

  A guest_memfd ioctl is used because shareability is a property of the memory,
  and this property should be modifiable independently of the attached struct kvm

but then follows with a very weak and IMO largely irrelevant justification of:

  This allows shareability to be modified even if the memory is not yet bound
  using memslots.

Allowing userspace to change shareability without memslots is one relatively minor
flow in one very specific use case.

The real justification for these ioctls is that fundamentally, shareability for
in-place conversions is a property of a guest_memfd instance and not a struct kvm
instance, and so needs to owned by guest_memfd.

I.e. focus on justifying the change from a design and conceptual perspective,
not from a mechanical perspective of a flow that likely's somewhat unique to our
specific environment.  Y'all are getting deep into the weeds on a random aspect
of x86 platform architecture, instead of focusing on the overall design.

The other issue that's likely making this more confusing than it needs to be is
that this series is actually two completely different series bundled into one,
with very little explanation.  Moving shared vs. private ownership into
guest_memfd isn't a requirement for 1GiB support, it's a requirement for in-place
shared/private conversion in guest_memfd.

For the current guest_memfd implementation, shared vs. private is tracked in the
VM via memory attributes, because a guest_memfd instance is *only* private.  I.e.
shared vs. private is a property of the VM, not of the guest_memfd instance.  But
when in-place conversion support comes along, ownership of that particular
attribute needs to shift to the guest_memfd instance.

I know I gave feedback on earlier posting about there being too series flying
around, but shoving two distinct concepts into a single series is not the answer.
My complaints about too much noise wasn't that there were multiple series, it was
that there was very little coordination and lots of chaos.

If you split this series in two, which should be trivial since you've already
organized the patches as a split, then sans the selftests (thank you for those!),
in-place conversion support will be its own (much smaller!) series that can focus
on that specific aspect of the design, and can provide a cover letter that
expounds on the design goals and uAPI.

  KVM: guest_memfd: Add CAP KVM_CAP_GMEM_CONVERSION
  KVM: Query guest_memfd for private/shared status
  KVM: guest_memfd: Skip LRU for guest_memfd folios
  KVM: guest_memfd: Introduce KVM_GMEM_CONVERT_SHARED/PRIVATE ioctls
  KVM: guest_memfd: Introduce and use shareability to guard faulting
  KVM: guest_memfd: Make guest mem use guest mem inodes instead of anonymous inodes

And then you can post the 1GiB series separately.  So long as you provide pointers
to dependencies along with a link to a repo+branch with the kitchen sink, I won't
complain about things being too chaotic :-)