[PATCH for-9.0 03/10] vfio/container: Initialize VFIOIOMMUOps under vfio_init_container()

Cédric Le Goater posted 10 patches 11 months, 3 weeks ago
There is a newer version of this series
[PATCH for-9.0 03/10] vfio/container: Initialize VFIOIOMMUOps under vfio_init_container()
Posted by Cédric Le Goater 11 months, 3 weeks ago
vfio_init_container() already defines the IOMMU type of the container.
Do the same for the VFIOIOMMUOps struct. This prepares ground for the
following patches that will deduce the associated VFIOIOMMUOps struct
from the IOMMU type.

Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 hw/vfio/container.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index afcfe8048805c58291d1104ff0ef20bdc457f99c..f4a0434a5239bfb6a17b91c8879cb98e686afccc 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -370,7 +370,7 @@ static int vfio_get_iommu_type(VFIOContainer *container,
 }
 
 static int vfio_init_container(VFIOContainer *container, int group_fd,
-                               Error **errp)
+                               VFIOAddressSpace *space, Error **errp)
 {
     int iommu_type, ret;
 
@@ -401,6 +401,7 @@ static int vfio_init_container(VFIOContainer *container, int group_fd,
     }
 
     container->iommu_type = iommu_type;
+    vfio_container_init(&container->bcontainer, space, &vfio_legacy_ops);
     return 0;
 }
 
@@ -583,9 +584,8 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
     container = g_malloc0(sizeof(*container));
     container->fd = fd;
     bcontainer = &container->bcontainer;
-    vfio_container_init(bcontainer, space, &vfio_legacy_ops);
 
-    ret = vfio_init_container(container, group->fd, errp);
+    ret = vfio_init_container(container, group->fd, space, errp);
     if (ret) {
         goto free_container_exit;
     }
-- 
2.43.0


RE: [PATCH for-9.0 03/10] vfio/container: Initialize VFIOIOMMUOps under vfio_init_container()
Posted by Duan, Zhenzhong 11 months, 3 weeks ago

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Sent: Friday, December 8, 2023 4:46 PM
>Subject: [PATCH for-9.0 03/10] vfio/container: Initialize VFIOIOMMUOps
>under vfio_init_container()
>
>vfio_init_container() already defines the IOMMU type of the container.
>Do the same for the VFIOIOMMUOps struct. This prepares ground for the
>following patches that will deduce the associated VFIOIOMMUOps struct
>from the IOMMU type.
>
>Signed-off-by: Cédric Le Goater <clg@redhat.com>
>---
> hw/vfio/container.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
>diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>index
>afcfe8048805c58291d1104ff0ef20bdc457f99c..f4a0434a5239bfb6a17b91c8
>879cb98e686afccc 100644
>--- a/hw/vfio/container.c
>+++ b/hw/vfio/container.c
>@@ -370,7 +370,7 @@ static int vfio_get_iommu_type(VFIOContainer
>*container,
> }
>
> static int vfio_init_container(VFIOContainer *container, int group_fd,
>-                               Error **errp)
>+                               VFIOAddressSpace *space, Error **errp)
> {
>     int iommu_type, ret;
>
>@@ -401,6 +401,7 @@ static int vfio_init_container(VFIOContainer
>*container, int group_fd,
>     }
>
>     container->iommu_type = iommu_type;
>+    vfio_container_init(&container->bcontainer, space, &vfio_legacy_ops);

Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>

Not related to this patch, not clear if it's deserved to rename
vfio_container_init as vfio_bcontainer_init to distinguish
with vfio_init_container.

Thanks
Zhenzhong

>     return 0;
> }
>
>@@ -583,9 +584,8 @@ static int vfio_connect_container(VFIOGroup *group,
>AddressSpace *as,
>     container = g_malloc0(sizeof(*container));
>     container->fd = fd;
>     bcontainer = &container->bcontainer;
>-    vfio_container_init(bcontainer, space, &vfio_legacy_ops);
>
>-    ret = vfio_init_container(container, group->fd, errp);
>+    ret = vfio_init_container(container, group->fd, space, errp);
>     if (ret) {
>         goto free_container_exit;
>     }
>--
>2.43.0

Re: [PATCH for-9.0 03/10] vfio/container: Initialize VFIOIOMMUOps under vfio_init_container()
Posted by Cédric Le Goater 11 months, 3 weeks ago
On 12/11/23 06:59, Duan, Zhenzhong wrote:
> 
> 
>> -----Original Message-----
>> From: Cédric Le Goater <clg@redhat.com>
>> Sent: Friday, December 8, 2023 4:46 PM
>> Subject: [PATCH for-9.0 03/10] vfio/container: Initialize VFIOIOMMUOps
>> under vfio_init_container()
>>
>> vfio_init_container() already defines the IOMMU type of the container.
>> Do the same for the VFIOIOMMUOps struct. This prepares ground for the
>> following patches that will deduce the associated VFIOIOMMUOps struct
>>from the IOMMU type.
>>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>> hw/vfio/container.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>> index
>> afcfe8048805c58291d1104ff0ef20bdc457f99c..f4a0434a5239bfb6a17b91c8
>> 879cb98e686afccc 100644
>> --- a/hw/vfio/container.c
>> +++ b/hw/vfio/container.c
>> @@ -370,7 +370,7 @@ static int vfio_get_iommu_type(VFIOContainer
>> *container,
>> }
>>
>> static int vfio_init_container(VFIOContainer *container, int group_fd,
>> -                               Error **errp)
>> +                               VFIOAddressSpace *space, Error **errp)
>> {
>>      int iommu_type, ret;
>>
>> @@ -401,6 +401,7 @@ static int vfio_init_container(VFIOContainer
>> *container, int group_fd,
>>      }
>>
>>      container->iommu_type = iommu_type;
>> +    vfio_container_init(&container->bcontainer, space, &vfio_legacy_ops);
> 
> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> 
> Not related to this patch, not clear if it's deserved to rename
> vfio_container_init as vfio_bcontainer_init to distinguish
> with vfio_init_container.

I agree, the vfio_container_init() and vfio_init_container() names
are confusing. I would keep vfio_container_init() because it is
consistent with all routines handling 'VFIOContainerBase *' ops.

I would be tempted to rename vfio_init_container() to vfio_set_iommu() ?

Also, I will introduce a vfio_connect_setup() helper in v2 doing the
assert as the other routines.

Thanks,

C.





> 
> Thanks
> Zhenzhong
> 
>>      return 0;
>> }
>>
>> @@ -583,9 +584,8 @@ static int vfio_connect_container(VFIOGroup *group,
>> AddressSpace *as,
>>      container = g_malloc0(sizeof(*container));
>>      container->fd = fd;
>>      bcontainer = &container->bcontainer;
>> -    vfio_container_init(bcontainer, space, &vfio_legacy_ops);
>>
>> -    ret = vfio_init_container(container, group->fd, errp);
>> +    ret = vfio_init_container(container, group->fd, space, errp);
>>      if (ret) {
>>          goto free_container_exit;
>>      }
>> --
>> 2.43.0
> 


RE: [PATCH for-9.0 03/10] vfio/container: Initialize VFIOIOMMUOps under vfio_init_container()
Posted by Duan, Zhenzhong 11 months, 2 weeks ago

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Subject: Re: [PATCH for-9.0 03/10] vfio/container: Initialize VFIOIOMMUOps
>under vfio_init_container()
>
>On 12/11/23 06:59, Duan, Zhenzhong wrote:
>>
>>
>>> -----Original Message-----
>>> From: Cédric Le Goater <clg@redhat.com>
>>> Sent: Friday, December 8, 2023 4:46 PM
>>> Subject: [PATCH for-9.0 03/10] vfio/container: Initialize VFIOIOMMUOps
>>> under vfio_init_container()
>>>
>>> vfio_init_container() already defines the IOMMU type of the container.
>>> Do the same for the VFIOIOMMUOps struct. This prepares ground for the
>>> following patches that will deduce the associated VFIOIOMMUOps struct
>>>from the IOMMU type.
>>>
>>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>>> ---
>>> hw/vfio/container.c | 6 +++---
>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>>> index
>>>
>afcfe8048805c58291d1104ff0ef20bdc457f99c..f4a0434a5239bfb6a17b91c8
>>> 879cb98e686afccc 100644
>>> --- a/hw/vfio/container.c
>>> +++ b/hw/vfio/container.c
>>> @@ -370,7 +370,7 @@ static int vfio_get_iommu_type(VFIOContainer
>>> *container,
>>> }
>>>
>>> static int vfio_init_container(VFIOContainer *container, int group_fd,
>>> -                               Error **errp)
>>> +                               VFIOAddressSpace *space, Error **errp)
>>> {
>>>      int iommu_type, ret;
>>>
>>> @@ -401,6 +401,7 @@ static int vfio_init_container(VFIOContainer
>>> *container, int group_fd,
>>>      }
>>>
>>>      container->iommu_type = iommu_type;
>>> +    vfio_container_init(&container->bcontainer, space, &vfio_legacy_ops);
>>
>> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>
>> Not related to this patch, not clear if it's deserved to rename
>> vfio_container_init as vfio_bcontainer_init to distinguish
>> with vfio_init_container.
>
>I agree, the vfio_container_init() and vfio_init_container() names
>are confusing. I would keep vfio_container_init() because it is
>consistent with all routines handling 'VFIOContainerBase *' ops.

Oh, yes, that's better.

>
>I would be tempted to rename vfio_init_container() to vfio_set_iommu() ?

Sounds good.

Thanks
Zhenzhong

>
>Also, I will introduce a vfio_connect_setup() helper in v2 doing the
>assert as the other routines.
>
>Thanks,
>
>C.
>
>
>
>
>
>>
>> Thanks
>> Zhenzhong
>>
>>>      return 0;
>>> }
>>>
>>> @@ -583,9 +584,8 @@ static int vfio_connect_container(VFIOGroup
>*group,
>>> AddressSpace *as,
>>>      container = g_malloc0(sizeof(*container));
>>>      container->fd = fd;
>>>      bcontainer = &container->bcontainer;
>>> -    vfio_container_init(bcontainer, space, &vfio_legacy_ops);
>>>
>>> -    ret = vfio_init_container(container, group->fd, errp);
>>> +    ret = vfio_init_container(container, group->fd, space, errp);
>>>      if (ret) {
>>>          goto free_container_exit;
>>>      }
>>> --
>>> 2.43.0
>>