Re: [RFC PATCH kvmtool 03/10] vfio: Create new file legacy.c

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

Mostafa Saleh <smostafa@google.com> writes:

> On Sun, May 25, 2025 at 01:19:09PM +0530, Aneesh Kumar K.V (Arm) wrote:
>> Move legacy vfio config methodology to legacy.c. Also add helper
>> vfio_map/unmap_mem_range which will be switched to function pointers in
>> the later patch.
>> 
>> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>
>> ---
>>  Makefile           |   1 +
>>  include/kvm/vfio.h |  14 ++
>>  vfio/core.c        | 342 ++------------------------------------------
>>  vfio/legacy.c      | 347 +++++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 372 insertions(+), 332 deletions(-)
>>  create mode 100644 vfio/legacy.c
>> 
>> diff --git a/Makefile b/Makefile
>> index 60e551fd0c2a..8b2720f73386 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -65,6 +65,7 @@ OBJS	+= pci.o
>>  OBJS	+= term.o
>>  OBJS	+= vfio/core.o
>>  OBJS	+= vfio/pci.o
>> +OBJS	+= vfio/legacy.o
>>  OBJS	+= virtio/blk.o
>>  OBJS	+= virtio/scsi.o
>>  OBJS	+= virtio/console.o
>> diff --git a/include/kvm/vfio.h b/include/kvm/vfio.h
>> index ac7b6226239a..67a528f18d33 100644
>> --- a/include/kvm/vfio.h
>> +++ b/include/kvm/vfio.h
>> @@ -126,4 +126,18 @@ void vfio_unmap_region(struct kvm *kvm, struct vfio_region *region);
>>  int vfio_pci_setup_device(struct kvm *kvm, struct vfio_device *device);
>>  void vfio_pci_teardown_device(struct kvm *kvm, struct vfio_device *vdev);
>>  
>> +int vfio_map_mem_range(struct kvm *kvm, __u64 host_addr, __u64 iova, __u64 size);
>> +int vfio_unmap_mem_range(struct kvm *kvm, __u64 iova, __u64 size);
>> +
>> +struct kvm_mem_bank;
>> +int vfio_map_mem_bank(struct kvm *kvm, struct kvm_mem_bank *bank, void *data);
>> +int vfio_unmap_mem_bank(struct kvm *kvm, struct kvm_mem_bank *bank, void *data);
>> +int vfio_configure_reserved_regions(struct kvm *kvm, struct vfio_group *group);
>> +int legacy_vfio__init(struct kvm *kvm);
>> +int legacy_vfio__exit(struct kvm *kvm);
>> +
>> +extern int kvm_vfio_device;
>> +extern struct list_head vfio_groups;
>> +extern struct vfio_device *vfio_devices;
>> +
>>  #endif /* KVM__VFIO_H */
>> diff --git a/vfio/core.c b/vfio/core.c
>> index 424dc4ed3aef..2af30df3b2b9 100644
>> --- a/vfio/core.c
>> +++ b/vfio/core.c
>> @@ -4,14 +4,11 @@
>>  
>>  #include <linux/list.h>
>>  
>> -#define VFIO_DEV_DIR		"/dev/vfio"
>> -#define VFIO_DEV_NODE		VFIO_DEV_DIR "/vfio"
>>  #define IOMMU_GROUP_DIR		"/sys/kernel/iommu_groups"
>>  
>> -static int vfio_container;
>> -static int kvm_vfio_device;
>> -static LIST_HEAD(vfio_groups);
>> -static struct vfio_device *vfio_devices;
>> +int kvm_vfio_device;
>
> kvm_vfio_device shouldn’t be VFIO/IOMMUFD specific, so that leads to
> duplication in both files, I suggest move it’s management to the vfio/core.c
> (and don’t extern the fd) And either export a function to add devices or maybe,
> better doing it once from vfio__init()
>
>> +LIST_HEAD(vfio_groups);
> “vfio_groups” seems not to be used by the core code, maybe it’s better to have a
> static version in each file?
> Also, as that is not really used for IOMMUFD, it seems to move group logic into
> legacy file. Instead of making iommufd populating groups so the core code handle
> the group exit.
>

I am also using the groups for reserved region configuration.

static int iommufd_configure_reserved_mem(struct kvm *kvm)
{
	int ret;
	struct vfio_group *group;

	list_for_each_entry(group, &vfio_groups, list) {
		ret = vfio_configure_reserved_regions(kvm, group);
		if (ret)
			return ret;
	}
	return 0;
}

An updated version of these patches can be found at
https://gitlab.arm.com/linux-arm/kvmtool-cca/-/tree/cca/tdisp-upstream-post-v1

>
>> +struct vfio_device *vfio_devices;
>>  
>
> Similarly for “vfio_devices”, it’s only allocated/freed in core code, but never used.
> But no strong opinion about that.
>
> Thanks,
> Mostafa
>

-aneesh