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

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

Ackerley Tng <ackerleytng@google.com> writes:

> Aneesh Kumar K.V <aneesh.kumar@kernel.org> writes:
>
>> 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.
>
> guest_memfd currently does .gmem_make_shared() eagerly and
> .gmem_make_private() lazily (at fault time).
>
> Technically conversion time spans everything from guest exit to request
> a conversion, including the guest_memfd conversion ioctl, then till the
> vCPU run completion hook.
>
> Can we move the mapping out of the conversion ioctl and into some
> dedicated CCA-specific mapping ioctl, kind of like KVM_ARM_RMI_POPULATE
> but doesn't actually perform writes, just does mapping?
>
> The mapping ioctl should check the necessary prereqs, same for the
> vcpu_run completion hook.
>
> Here are some advantages I can think of:
>
> 1. Decouples mapping of VFIO addresses from guest_memfd and provide more
>    understandable errors for userspace, since the mapping step has a
>    clear and separate error (conversion is complicated enough with many
>    failure points)
>
> 2. Better aligned conversions in general where marking private at the
>    platform level is lazily performed. In this case, doing it outside of
>    the conversions ioctl is not much lazier, but slightly :)
>
>    Not quite mapping, but Suzuki did bring up that for regular memory
>    (not VFIO), ARM CCA requires vCPU context for SMC_RMI_RTT_SET_RIPAS
>    in .gmem_make_private(), hence that's done in the vcpu_run completion
>    hook, so that's kind of lazier, relative to doing
>    SMC_RMI_RTT_SET_RIPAS in the conversion ioctl.
>


Is there a reason why setting private attributes must not create new KVM
mappings? Is it because a guest_memfd can be bound to multiple VMs, and a
set_memory_attributes() operation should not affect the stage-2 mappings of
other VMs?

But yes, we can move the mapping to a separate operation. I am trying to
make device memory behave more like normal guest memory in this respect.
With VFIO passthrough, normal guest memory requires a private mapping
before the guest accesses it because a device can start DMA to the region
without any prior guest access. I currently use KVM's prefault path for
this and can extend it to handle device MMIO regions.

For an MMIO validation request, VMM would set the guest_memfd range's
memory attributes to private, prefault the protected IPA range and then
approve the request. Prefaulting would perform DEV_MAP.

This means that we need to track private device mappings as part of the
Realm. MMIO validation requires the device PFNs to be mapped already;
that work cannot be deferred until a subsequent guest fault. Before
issuing DEV_VALIDATE, the MMIO validation completion path would need to
check both the Realm's private mapping state and the guest_memfd memory
attributes.


>
>
>> 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;
>> +	}
>> +
>>
>
> What do you think of just forwarding .mmap to VFIO to figure out? This
> makes VFIO less special of a provider, since I imagine different
> providers may want to have different page protections or VMA flags.
>
> guest_memfd just enforces VM_PFNMAP for PFN-mode providers.
>

I have reworked the patch set to drop backing_ops and consolidate
everything under provider ops. The generic guest_memfd path now contains
only provider callbacks; the folio, shmem, RAM PFN, and device PFN
differences are handled by those callbacks.

We might still want the generic code to apply:

	pgprot_noncached(vma->vm_page_prot)

These prot bits are exposed to VMM mappings, and if the attribute is not
reflected in vma->vm_page_prot, mprotect() may set the PTE protection bits
incorrectly.

I will send a v5 of the reworked patch series soon.

-aneesh