[v3] Prerequisite changes for IOMMUFD support

[PATCH v3 13/15] vfio/common: Store the parent container in VFIODevice

Posted by Eric Auger 2 years, 4 months ago

From: Zhenzhong Duan <zhenzhong.duan@intel.com>

let's store the parent contaienr within the VFIODevice.
This simplifies the logic in vfio_viommu_preset() and
brings the benefice to hide the group specificity which
is useful for IOMMUFD migration.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 include/hw/vfio/vfio-common.h | 1 +
 hw/vfio/common.c              | 8 +++++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 8ca70dd821..bf12e40667 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -132,6 +132,7 @@ typedef struct VFIODevice {
     QLIST_ENTRY(VFIODevice) next;
     QLIST_ENTRY(VFIODevice) container_next;
     struct VFIOGroup *group;
+    VFIOContainer *container;
     char *sysfsdev;
     char *name;
     DeviceState *dev;
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index ef9dc7c747..55f8a113ea 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -184,7 +184,7 @@ void vfio_unblock_multiple_devices_migration(void)
 
 bool vfio_viommu_preset(VFIODevice *vbasedev)
 {
-    return vbasedev->group->container->space->as != &address_space_memory;
+    return vbasedev->container->space->as != &address_space_memory;
 }
 
 static void vfio_set_migration_error(int err)
@@ -2655,6 +2655,7 @@ int vfio_attach_device(char *name, VFIODevice *vbasedev,
     }
 
     container = group->container;
+    vbasedev->container = container;
     QLIST_INSERT_HEAD(&container->device_list, vbasedev, container_next);
 
     return ret;
@@ -2664,7 +2665,12 @@ void vfio_detach_device(VFIODevice *vbasedev)
 {
     VFIOGroup *group = vbasedev->group;
 
+    if (!vbasedev->container) {
+        return;
+    }
+
     QLIST_REMOVE(vbasedev, container_next);
+    vbasedev->container = NULL;
     trace_vfio_detach_device(vbasedev->name, group->groupid);
     vfio_put_base_device(vbasedev);
     vfio_put_group(group);
-- 
2.41.0

Re: [PATCH v3 13/15] vfio/common: Store the parent container in VFIODevice

Posted by Cédric Le Goater 2 years, 4 months ago

On 10/3/23 12:14, Eric Auger wrote:
> From: Zhenzhong Duan <zhenzhong.duan@intel.com>
> 
> let's store the parent contaienr within the VFIODevice.
> This simplifies the logic in vfio_viommu_preset() and
> brings the benefice to hide the group specificity which
> is useful for IOMMUFD migration.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>   include/hw/vfio/vfio-common.h | 1 +
>   hw/vfio/common.c              | 8 +++++++-
>   2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 8ca70dd821..bf12e40667 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -132,6 +132,7 @@ typedef struct VFIODevice {
>       QLIST_ENTRY(VFIODevice) next;
>       QLIST_ENTRY(VFIODevice) container_next;
>       struct VFIOGroup *group;
> +    VFIOContainer *container;
>       char *sysfsdev;
>       char *name;
>       DeviceState *dev;
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index ef9dc7c747..55f8a113ea 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -184,7 +184,7 @@ void vfio_unblock_multiple_devices_migration(void)
>   
>   bool vfio_viommu_preset(VFIODevice *vbasedev)
>   {
> -    return vbasedev->group->container->space->as != &address_space_memory;
> +    return vbasedev->container->space->as != &address_space_memory;
>   }
>   
>   static void vfio_set_migration_error(int err)
> @@ -2655,6 +2655,7 @@ int vfio_attach_device(char *name, VFIODevice *vbasedev,
>       }
>   
>       container = group->container;
> +    vbasedev->container = container;
>       QLIST_INSERT_HEAD(&container->device_list, vbasedev, container_next);
>   
>       return ret;
> @@ -2664,7 +2665,12 @@ void vfio_detach_device(VFIODevice *vbasedev)
>   {
>       VFIOGroup *group = vbasedev->group;
>   
> +    if (!vbasedev->container) {
> +        return;
> +    }

Can this happen ? Should it be an assert ?

Thanks,

C.


> +
>       QLIST_REMOVE(vbasedev, container_next);
> +    vbasedev->container = NULL;
>       trace_vfio_detach_device(vbasedev->name, group->groupid);
>       vfio_put_base_device(vbasedev);
>       vfio_put_group(group);

Re: [PATCH v3 13/15] vfio/common: Store the parent container in VFIODevice

Posted by Eric Auger 2 years, 4 months ago

Hi Cédric,

On 10/3/23 17:59, Cédric Le Goater wrote:
> On 10/3/23 12:14, Eric Auger wrote:
>> From: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>
>> let's store the parent contaienr within the VFIODevice.
>> This simplifies the logic in vfio_viommu_preset() and
>> brings the benefice to hide the group specificity which
>> is useful for IOMMUFD migration.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>   include/hw/vfio/vfio-common.h | 1 +
>>   hw/vfio/common.c              | 8 +++++++-
>>   2 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/hw/vfio/vfio-common.h
>> b/include/hw/vfio/vfio-common.h
>> index 8ca70dd821..bf12e40667 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -132,6 +132,7 @@ typedef struct VFIODevice {
>>       QLIST_ENTRY(VFIODevice) next;
>>       QLIST_ENTRY(VFIODevice) container_next;
>>       struct VFIOGroup *group;
>> +    VFIOContainer *container;
>>       char *sysfsdev;
>>       char *name;
>>       DeviceState *dev;
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index ef9dc7c747..55f8a113ea 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -184,7 +184,7 @@ void vfio_unblock_multiple_devices_migration(void)
>>     bool vfio_viommu_preset(VFIODevice *vbasedev)
>>   {
>> -    return vbasedev->group->container->space->as !=
>> &address_space_memory;
>> +    return vbasedev->container->space->as != &address_space_memory;
>>   }
>>     static void vfio_set_migration_error(int err)
>> @@ -2655,6 +2655,7 @@ int vfio_attach_device(char *name, VFIODevice
>> *vbasedev,
>>       }
>>         container = group->container;
>> +    vbasedev->container = container;
>>       QLIST_INSERT_HEAD(&container->device_list, vbasedev,
>> container_next);
>>         return ret;
>> @@ -2664,7 +2665,12 @@ void vfio_detach_device(VFIODevice *vbasedev)
>>   {
>>       VFIOGroup *group = vbasedev->group;
>>   +    if (!vbasedev->container) {
>> +        return;
>> +    }
>
> Can this happen ? Should it be an assert ?
I don't think so. Let me simply drop the check.

Thanks

Eric
>
> Thanks,
>
> C.
>
>
>> +
>>       QLIST_REMOVE(vbasedev, container_next);
>> +    vbasedev->container = NULL;
>>       trace_vfio_detach_device(vbasedev->name, group->groupid);
>>       vfio_put_base_device(vbasedev);
>>       vfio_put_group(group);
>

Re: [PATCH v3 13/15] vfio/common: Store the parent container in VFIODevice

Posted by Cédric Le Goater 2 years, 4 months ago

Hello Eric,


>>> @@ -2664,7 +2665,12 @@ void vfio_detach_device(VFIODevice *vbasedev)
>>>    {
>>>        VFIOGroup *group = vbasedev->group;
>>>    +    if (!vbasedev->container) {
>>> +        return;
>>> +    }
>>
>> Can this happen ? Should it be an assert ?
>
> I don't think so. Let me simply drop the check.

the device-introspect-test needs it. No need to resend a v5, I can add it
back in v4 if you are ok with that.


Thanks,

C.

Re: [PATCH v3 13/15] vfio/common: Store the parent container in VFIODevice

Posted by Eric Auger 2 years, 4 months ago

Hi Cédric,

On 10/4/23 18:55, Cédric Le Goater wrote:
> the device-introspect-test needs it. No need to resend a v5, I can add it
> back in v4 if you are ok with that. 

Ah OK. I am definitively OK for you to restore it if nothing else shows
up inbetween

Eric