Re: [RFC PATCH v1 2/9] KVM: guest_memfd: Add guest_memfd support to kvm_(read|/write)_guest_page()

[Fuad Tabba <tabba@google.com>]

On Thu, 23 Jan 2025 at 14:21, David Hildenbrand <david@redhat.com> wrote:
>
> On 23.01.25 14:57, Patrick Roy wrote:
> >
> >
> > On Thu, 2025-01-23 at 12:28 +0000, Fuad Tabba wrote:
> >> Hi Patrick,
> >>
> >> On Thu, 23 Jan 2025 at 11:57, Patrick Roy <roypat@amazon.co.uk> wrote:
> >>>
> >>>
> >>>
> >>> On Thu, 2025-01-23 at 11:39 +0000, David Hildenbrand wrote:
> >>>> On 23.01.25 10:48, Fuad Tabba wrote:
> >>>>> On Wed, 22 Jan 2025 at 22:10, David Hildenbrand <david@redhat.com> wrote:
> >>>>>>
> >>>>>> On 22.01.25 16:27, Fuad Tabba wrote:
> >>>>>>> Make kvm_(read|/write)_guest_page() capable of accessing guest
> >>>>>>> memory for slots that don't have a userspace address, but only if
> >>>>>>> the memory is mappable, which also indicates that it is
> >>>>>>> accessible by the host.
> >>>>>>
> >>>>>> Interesting. So far my assumption was that, for shared memory, user
> >>>>>> space would simply mmap() guest_memdd and pass it as userspace address
> >>>>>> to the same memslot that has this guest_memfd for private memory.
> >>>>>>
> >>>>>> Wouldn't that be easier in the first shot? (IOW, not require this patch
> >>>>>> with the cost of faulting the shared page into the page table on access)
> >>>>>
> >>>>
> >>>> In light of:
> >>>>
> >>>> https://lkml.kernel.org/r/20250117190938.93793-4-imbrenda@linux.ibm.com
> >>>>
> >>>> there can, in theory, be memslots that start at address 0 and have a
> >>>> "valid" mapping. This case is done from the kernel (and on special s390x
> >>>> hardware), though, so it does not apply here at all so far.
> >>>>
> >>>> In practice, getting address 0 as a valid address is unlikely, because
> >>>> the default:
> >>>>
> >>>> $ sysctl  vm.mmap_min_addr
> >>>> vm.mmap_min_addr = 65536
> >>>>
> >>>> usually prohibits it for good reason.
> >>>>
> >>>>> This has to do more with the ABI I had for pkvm and shared memory
> >>>>> implementations, in which you don't need to specify the userspace
> >>>>> address for memory in a guestmem memslot. The issue is there is no
> >>>>> obvious address to map it to. This would be the case in kvm:arm64 for
> >>>>> tracking paravirtualized time, which the userspace doesn't necessarily
> >>>>> need to interact with, but kvm does.
> >>>>
> >>>> So I understand correctly: userspace wouldn't have to mmap it because it
> >>>> is not interested in accessing it, but there is nothing speaking against
> >>>> mmaping it, at least in the first shot.
> >>>>
> >>>> I assume it would not be a private memslot (so far, my understanding is
> >>>> that internal memslots never have a guest_memfd attached).
> >>>> kvm_gmem_create() is only called via KVM_CREATE_GUEST_MEMFD, to be set
> >>>> on user-created memslots.
> >>>>
> >>>>>
> >>>>> That said, we could always have a userspace address dedicated to
> >>>>> mapping shared locations, and use that address when the necessity
> >>>>> arises. Or we could always require that memslots have a userspace
> >>>>> address, even if not used. I don't really have a strong preference.
> >>>>
> >>>> So, the simpler version where user space would simply mmap guest_memfd
> >>>> to provide the address via userspace_addr would at least work for the
> >>>> use case of paravirtualized time?
> >>>
> >>> fwiw, I'm currently prototyping something like this for x86 (although
> >>> not by putting the gmem address into userspace_addr, but by adding a new
> >>> field to memslots, so that memory attributes continue working), based on
> >>> what we talked about at the last guest_memfd sync meeting (the whole
> >>> "how to get MMIO emulation working for non-CoCo VMs in guest_memfd"
> >>> story). So I guess if we're going down this route for x86, maybe it
> >>> makes sense to do the same on ARM, for consistency?
> >>>
> >>>> It would get rid of the immediate need for this patch and patch #4 to
> >>>> get it flying.
> >>>>
> >>>>
> >>>> One interesting question is: when would you want shared memory in
> >>>> guest_memfd and *not* provide it as part of the same memslot.
> >>>
> >>> In my testing of non-CoCo gmem VMs on ARM, I've been able to get quite
> >>> far without giving KVM a way to internally access shared parts of gmem -
> >>> it's why I was probing Fuad for this simplified series, because
> >>> KVM_SW_PROTECTED_VM + mmap (for loading guest kernel) is enough to get a
> >>> working non-CoCo VM on ARM (although I admittedly never looked at clocks
> >>> inside the guest - maybe that's one thing that breaks if KVM can't
> >>> access gmem. How to guest and host agree on the guest memory range
> >>> used to exchange paravirtual timekeeping information? Could that exchange
> >>> be intercepted in userspace, and set to shared via memory attributes (e.g.
> >>> placed outside gmem)? That's the route I'm going down the paravirtual
> >>> time on x86).
> >>
> >> For an idea of what it looks like on arm64, here's how kvmtool handles it:
> >> https://github.com/kvmtool/kvmtool/blob/master/arm/aarch64/pvtime.c
> >>
> >> Cheers,
> >> /fuad
> >
> > Thanks! In that example, kvmtool actually allocates a separate memslot for
> > the pvclock stuff, so I guess it's always possible to simply put it into
> > a non-gmem memslot, which indeed sidesteps this issue as you mention in
> > your reply to David :D
>
> Does that work on CC where all memory defaults to private first, and the
> VM explicitly has to opt into marking it shared first, or how exactly
> would the flow of operations be in the cases of the non-gmem ("good
> old") memslot?

We use a normal memslot, without the KVM_MEM_GUEST_MEMFD flag, and
consider that kind of slot to be shared by default.

Cheers,
/fuad

> --
> Cheers,
>
> David / dhildenb
>