diff mbox series

[v4,01/12] vfio/pci: Extract mdev check into an helper

Message ID 20240712114704.8708-2-joao.m.martins@oracle.com (mailing list archive)
State New, archived
Headers show
Series hw/iommufd: IOMMUFD Dirty Tracking | expand

Commit Message

Joao Martins July 12, 2024, 11:46 a.m. UTC 
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.martins@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(-)

Comments

Cédric Le Goater July 16, 2024, 9:21 a.m. UTC | #1 
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.martins@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)

> +{
> +    g_autofree char *tmp = NULL;
> +    char *subsys;

a g_autofree variable is preferable here.

> +    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 ....

> +    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.


Thanks,

C.




>       trace_vfio_mdev(vbasedev->name, is_mdev);
>   
>       if (vbasedev->ram_block_discard_allowed && !is_mdev) {
Joao Martins July 16, 2024, 9:33 a.m. UTC | #2 
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.martins@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) {
>
diff mbox series

Patch

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)
+{
+    g_autofree char *tmp = NULL;
+    char *subsys;
+    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);
+    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;
     trace_vfio_mdev(vbasedev->name, is_mdev);
 
     if (vbasedev->ram_block_discard_allowed && !is_mdev) {