Re: [RFC PATCH v4 06/25] KVM: guest_memfd: Support device-backed PFN providers

[Aneesh Kumar K.V <aneesh.kumar@kernel.org>]

Fred Griffoul <griffoul@gmail.com> writes:

> "Aneesh Kumar K.V (Arm)" <aneesh.kumar@kernel.org> writes:
>
>> Device resources such as PCI BARs supply PFNs directly and cannot use
>> the folio allocation path. Store the mapping owner and device ID in the
>> core context during attach(), and use them when preparing device
>> memslots. Keep device memory separate from ordinary PFN-backed RAM.
>
> The device rules you wrote down in api.rst and locking.rst helped a
> lot. If I read v4 right, the device backing differs from the RAM PFN
> backing in these places:
>
>   1. PFN mode calls RAM arch hooks: kvm_arch_gmem_make_shared() at
>      conversion, kvm_arch_gmem_make_private() at fault, and
>      kvm_arch_gmem_reclaim() on revoke and release. The device path
>      has to skip them.
>

I would say that the VFIO provider expects the conversion to complete at
conversion time, rather than being triggered by a fault. This is also
required because, when an MMIO validation request completes, the guest
expects the range to be mapped. The current flow is that the MMIO
validation request exits to the VMM, which uses set_memory_attributes()
to convert the range to private.

>   2. lookup_pfn_range() only feeds those hooks, so the device path
>      does not need it.
>   3. The host mmap and stage-2 need a device memory type.
>


Do you mean this code? vm_page_prot is used for the host mapping created
by mmap() on the guest_memfd, and its setup follows
vfio_pci_core_mmap(). I think the only device-specific difference here
should be pgprot_noncached(). Shouldn't the rest of the protection
remain unchanged even for device PFNs?


+	if (gmem_provider_is_device(gi)) {
+		vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
+		vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
+		vm_flags_set(vma, VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP);
+		vma->vm_ops = &kvm_gmem_device_vm_ops;
+		return 0;
+	}
+


For stage-2, the architecture-specific fault handler should handle this.


>   4. In PFN mode, a conversion to private only updates the attributes,
>      and the guest fault maps the page later. CCA has to map device
>      memory at conversion, because the guest validates the mapping
>      before it touches it.
>

correct 

>   5. The device provider needs lock()/unlock(), the private range
>      summary and prepare_memory_region().'
>

The TSM checks that there are no private pages before issuing the unlock
request.

>   6. The provider gets the VM and the vDEVICE binding at attach.
>
> Would these changes to PFN mode be enough to drop the device table?
>
>   1. The arch hooks take the frame attributes. For RAM the arch does
>      the RMP work. For MMIO it skips it, or does the RMM work instead.
>

IIUC, this is what the patch already does. Or are you suggesting
something else?

static int kvm_gmem_convert_attributes(struct inode *inode, pgoff_t start,
		pgoff_t end, u64 attrs)
{
	const struct kvm_gmem_backing_ops *ops = GMEM_I(inode)->backing_ops;

	if (ops->convert_attributes)
		return ops->convert_attributes(inode, start, end, attrs);

	return 0;
}



>   2. lookup_pfn_range() stays required. For vfio-pci it is the BAR
>      start plus the offset.
>

I have to admit that I did not understand the need for lookup_pfn_range.
It was called from the reclaim path, but we did not have reclaim
support.

>   3. get_pfn() and lookup_pfn_range() report a memory type (MMIO) in
>      their attributes, next to read only. The core mmap and the KVM
>      fault path use it.
>

Yes, this should work.

>   4. A conversion calls an arch hook in both directions, and the hook
>      can fail. CCA maps the private device range there.
>

As kvm_gmem_convert_attributes() already does in the current patch?

>   5. lock()/unlock(), the private range summary and
>      prepare_memory_region() become optional ops for any PFN provider.
>      A provider that moves RAM between guests needs them too: it has
>      to order its moves with conversions and leave private pages
>      alone.
>

Isn't that what the patch already does?

static void gmem_provider_lock(struct gmem_inode *gi)
{
	if (gi->provider_ops && gi->provider_ops->lock)
		gi->provider_ops->lock(gi->provider);
}

static void gmem_provider_unlock(struct gmem_inode *gi)
{
	if (gi->provider_ops && gi->provider_ops->unlock)
		gi->provider_ops->unlock(gi->provider);
}

if (gi->provider_ops->prepare_memory_region) {
	u32 flags = slot->flags & (KVM_MEM_LOG_DIRTY_PAGES | KVM_MEM_READONLY);

	ret = gi->provider_ops->prepare_memory_region(gi->provider, offset,
						      size, flags);
	if (ret) {
		fput(file);
		return ret;
	}
}



>
> I'm less sure about 6. Could KVM ask the provider for the vDEVICE when
> the memslot is created, passing the VM? The provider would then not
> need to keep a VM pointer.
>

OK, so you don't want struct gmem_inode to contain struct
guest_memfd_device, or is the concern limited to struct kvm? I'll check
whether we can move it out.

-aneesh