On 16/07/2024 10:21, Cédric Le Goater wrote:
> Hello Joao
>
> On 7/12/24 13:46, Joao Martins wrote:
>> In preparation to skip initialization of the HostIOMMUDevice for mdev,
>> extract the checks that validate if a device is an mdev into helpers.
>>
>> A vfio_set_mdev() is created, and subsystems consult VFIODevice::mdev
>> to check if it's mdev or not.
>>
>> Signed-off-by: Joao Martins <joao.m.mart...@oracle.com>
>> ---
>> include/hw/vfio/vfio-common.h | 2 ++
>> hw/vfio/helpers.c | 18 ++++++++++++++++++
>> hw/vfio/pci.c | 9 ++-------
>> 3 files changed, 22 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>> index e8ddf92bb185..7419466bca92 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -116,6 +116,7 @@ typedef struct VFIODevice {
>> DeviceState *dev;
>> int fd;
>> int type;
>> + bool mdev;
>> bool reset_works;
>> bool needs_reset;
>> bool no_mmap;
>> @@ -231,6 +232,7 @@ void vfio_region_exit(VFIORegion *region);
>> void vfio_region_finalize(VFIORegion *region);
>> void vfio_reset_handler(void *opaque);
>> struct vfio_device_info *vfio_get_device_info(int fd);
>> +void vfio_set_mdev(VFIODevice *vbasedev);
>> bool vfio_attach_device(char *name, VFIODevice *vbasedev,
>> AddressSpace *as, Error **errp);
>> void vfio_detach_device(VFIODevice *vbasedev);
>> diff --git a/hw/vfio/helpers.c b/hw/vfio/helpers.c
>> index b14edd46edc9..bace0e788a09 100644
>> --- a/hw/vfio/helpers.c
>> +++ b/hw/vfio/helpers.c
>> @@ -675,3 +675,21 @@ int vfio_device_get_aw_bits(VFIODevice *vdev)
>> return HOST_IOMMU_DEVICE_CAP_AW_BITS_MAX;
>> }
>> +
>> +void vfio_set_mdev(VFIODevice *vbasedev)
>
> Could you please change this routine to :
>
> bool vfio_device_is_mdev(VFIODevice *vbasedev)
>
OK
>> +{
>> + g_autofree char *tmp = NULL;
>> + char *subsys;
>
> a g_autofree variable is preferable here.
>
It was a code move hence why I kept the exact same structure
I will change to your suggestion
>> + bool is_mdev;
>> +
>> + if (!vbasedev->sysfsdev) {
>> + return;
>> + }
>> +
>> + tmp = g_strdup_printf("%s/subsystem", vbasedev->sysfsdev);
>> + subsys = realpath(tmp, NULL);
>> + is_mdev = subsys && (strcmp(subsys, "/sys/bus/mdev") == 0);
>
> simply return is the result here and ....
>
OK
>> + free(subsys);
>> +
>> + vbasedev->mdev = is_mdev;
>> +}
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index e03d9f3ba546..585f23a18406 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -2963,12 +2963,10 @@ static void vfio_realize(PCIDevice *pdev, Error
>> **errp)
>> ERRP_GUARD();
>> VFIOPCIDevice *vdev = VFIO_PCI(pdev);
>> VFIODevice *vbasedev = &vdev->vbasedev;
>> - char *subsys;
>> int i, ret;
>> bool is_mdev;
>> char uuid[UUID_STR_LEN];
>> g_autofree char *name = NULL;
>> - g_autofree char *tmp = NULL;
>> if (vbasedev->fd < 0 && !vbasedev->sysfsdev) {
>> if (!(~vdev->host.domain || ~vdev->host.bus ||
>> @@ -2997,11 +2995,8 @@ static void vfio_realize(PCIDevice *pdev, Error
>> **errp)
>> * stays in sync with the active working set of the guest driver.
>> Prevent
>> * the x-balloon-allowed option unless this is minimally an mdev
>> device.
>> */
>> - tmp = g_strdup_printf("%s/subsystem", vbasedev->sysfsdev);
>> - subsys = realpath(tmp, NULL);
>> - is_mdev = subsys && (strcmp(subsys, "/sys/bus/mdev") == 0);
>> - free(subsys);
>> -
>> + vfio_set_mdev(vbasedev);
>> + is_mdev = vbasedev->mdev;
>
> replace with :
>
> vbasedev->mdev = vfio_device_is_mdev(vbasedev);
>
> and use vbasedev->mdev instead of is_mdev where needed.
>
OK
>
> Thanks,
>
> C.
>
>
>
>
>> trace_vfio_mdev(vbasedev->name, is_mdev);
>> if (vbasedev->ram_block_discard_allowed && !is_mdev) {
>
