Re: [RFC PATCH v1 0/6] KVM/VFIO: guest_memfd support for device MMIO resources

[Aneesh Kumar K . V]

Ackerley Tng <ackerleytng@xxxxxxxxxx> writes:

> Aneesh Kumar K.V <aneesh.kumar@xxxxxxxxxx> writes:
>
>> Ackerley Tng <ackerleytng@xxxxxxxxxx> writes:
>>
>>> "Aneesh Kumar K.V (Arm)" <aneesh.kumar@xxxxxxxxxx> writes:
>>>

 [ ... 65 lines skipped ... ] 

>
>>>
>>>> and GUEST_MEMFD_FLAG_INIT_SHARED. Eligible BAR
>>>> intervals are registered as guest_memfd memslots. File offsets use the
>>>> existing VFIO region-offset namespace, allowing one guest_memfd to cover
>>>> multiple BAR intervals. Device-backed guest_memfd rejects mmap and
>>>> fallocate; shared guest faults obtain bare BAR PFNs from the provider
>>>> instead of resolving the memslot HVA or allocating folios.
>>>>
>>>
>>> The model I have in mind is kind of opposite from above. Instead of the
>>> guest_memfd rejecting mmap(), I'm thinking that the VFIO fd should be
>>> the one that rejects mmap(), read() and write() once guest_memfd
>>> attaches to it.
>>>
>>> If userspace needed to mmap() the VFIO fd, it should instead mmap() the
>>> guest_memfd fd. guest_memfd doesn't support read() and write() now, but
>>> support can be added. The read/write for guest_memfd just forwards to
>>> vfio if the offset range is shared and blocks if the range is
>>> private.
>>>
>>> For mmap(), for regular memory, mmap() is always allowed, but faulting
>>> dependsd on shared/private status. It would be nice if the same logic
>>> could apply to guest_memfds that wrap VFIO.
>>>
>>> I prefer the userspace accesses to go through guest_memfd to VFIO so
>>> that the shared/private-ness can be enforced from guest_memfd.
>>>
>>
>> Sure, I can implement this. I guess read(), write(), and ioeventfd will
>> continue to go through the VFIO fd?
>>
>
> If you agree to having mmap() go through guest_memfd, then read() and
> write() should also go through guest_memfd imo. I think it makes sense
> since guest_memfd gates accesses based on shared/private status.
>
> guest_memfd already had plans to support write() [4], so I think
> supporting read() should be fine too.
>
> [4] https://lore.kernel.org/all/20251020161352.69257-1-kalyazin@xxxxxxxxxx/
>
>
> I'm not super familiar with ioeventfd, let me know if this makes sense:
> ioeventfd is a kind of config, like mmap() is a config, but faulting is
> where the actual access can happen.
>
> guest_memfd always allows mmap(), but only allows faults on shared
> offset ranges.
>
> If userspace sets up some link between a private range and some eventfd,
> I think that's ok, since it's not actually doing accesses. When the
> signalling actually happens, then guest_memfd needs to gate that.
>
> Going by that, I think if the ioeventfd ioctls are just setting up
> links/mapping, then it can go through the VFIO fd.
>
> Between going via the VFIO fd and guest_memfd, I think if "everything is
> wrapped by guest_memfd" a simpler mental model to go with, then maybe we
> could just go with swapping the entire f_ops with guest_memfd's
> intercepted version?
>
>

I omitted the rest of the email so that we can focus on the above. While
implementing mmap redirection through guest_memfd, I found that it
results in code duplication in both the VMM and the kernel. The VMM
already uses the VFIO fd for these operations. I think we should allow
it to continue using the existing flow, with additional checks to ensure
that it accesses only shared regions. Requiring the VMM to use
guest_memfd as the file descriptor for accessing CoCo guest resources
introduces unnecessary duplication.

We cannot move all provider fd ioctls to guest_memfd. Providers may have
different ioctls that access shared regions, and supporting all of them
through guest_memfd would not be practical. VFIO_DEVICE_IOEVENTFD is one
example. If ioctls cannot be moved, the provider code will need access
checks anyway. For example, for VFIO I do:

modified   drivers/vfio/pci/vfio_pci_rdwr.c
@@ -44,7 +44,9 @@ int vfio_pci_core_iowrite##size(struct vfio_pci_core_device *vdev,	\
 {									\
 	if (test_mem) {							\
 		down_read(&vdev->memory_lock);				\
-		if (!__vfio_pci_memory_enabled(vdev)) {			\
+		if (!__vfio_pci_memory_enabled(vdev) ||			\
+		    !vfio_pci_gmem_io_access_allowed(vdev, io,		\
+						     sizeof(u##size))) {	\
 			up_read(&vdev->memory_lock);			\
 			return -EIO;					\
 		}							\
@@ -70,7 +72,9 @@ int vfio_pci_core_ioread##size(struct vfio_pci_core_device *vdev,	\
 {									\
 	if (test_mem) {							\
 		down_read(&vdev->memory_lock);				\
-		if (!__vfio_pci_memory_enabled(vdev)) {			\
+		if (!__vfio_pci_memory_enabled(vdev) ||			\
+		    !vfio_pci_gmem_io_access_allowed(vdev, io,		\
+						     sizeof(u##size))) {	\
 			up_read(&vdev->memory_lock);			\
 			return -EIO;					\
 		}							\
@@ -380,7 +384,10 @@ static int vfio_pci_ioeventfd_handler(void *opaque, void *unused)
 	if (ioeventfd->test_mem) {
 		if (!down_read_trylock(&vdev->memory_lock))
 			return 1; /* Lock contended, use thread */
-		if (!__vfio_pci_memory_enabled(vdev)) {
+		if (!__vfio_pci_memory_enabled(vdev) ||
+		    !vfio_pci_gmem_access_allowed(vdev,
+				VFIO_PCI_INDEX_TO_OFFSET(ioeventfd->bar) +
+				ioeventfd->pos, ioeventfd->count)) {
 			up_read(&vdev->memory_lock);
 			return 0;
 		}


If we are going to do this, it should also be OK to perform the check on
the fault path. For VFIO, that is much simpler.

@@ -1766,6 +1764,14 @@ vm_fault_t vfio_pci_vmf_insert_pfn(struct vfio_pci_core_device *vdev,
 	if (vdev->pm_runtime_engaged || !__vfio_pci_memory_enabled(vdev))
 		return VM_FAULT_SIGBUS;
 
+	/* Check each page separately while guest_memfd controls BAR access. */
+	if (order && vdev->gmem)
+		return VM_FAULT_FALLBACK;
+	/* memory_lock keeps revocation from racing admission and PTE insertion. */
+	if (!vfio_pci_gmem_access_allowed(vdev,
+				(u64)vmf->pgoff << PAGE_SHIFT, PAGE_SIZE))
+		return VM_FAULT_SIGBUS;
+
 	if (!order)
 		return vmf_insert_pfn(vmf->vma, vmf->address, pfn);
 

Given the above, it seems much simpler to allow mmap(), read(), write(),
and ioctls through the VFIO fd. Attach should require
GUEST_MEMFD_FLAG_MMAP, and mmap() on the VFIO fd should be allowed only
when that flag is set.

I can post a series demonstrating the implementation duplication if that
would help.

-aneesh