Re: [PATCH v1 05/11] vfio: Introduce host_iommu_device_create callback
Hi Zhenzhong,
On 3/19/24 06:44, Duan, Zhenzhong wrote:
>
>> -Original Message-
>> From: Eric Auger
>> Subject: Re: [PATCH v1 05/11] vfio: Introduce host_iommu_device_create
>> callback
>>
>>
>>
>> On 2/28/24 04:58, Zhenzhong Duan wrote:
>>> Introduce host_iommu_device_create callback and a wrapper for it.
>>>
>>> This callback is used to allocate a host iommu device instance and
>>> initialize it based on type.
>>>
>>> Signed-off-by: Zhenzhong Duan
>>> ---
>>> include/hw/vfio/vfio-common.h | 1 +
>>> include/hw/vfio/vfio-container-base.h | 1 +
>>> hw/vfio/common.c | 8
>>> 3 files changed, 10 insertions(+)
>>>
>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
>> common.h
>>> index b6676c9f79..9fefea4b89 100644
>>> --- a/include/hw/vfio/vfio-common.h
>>> +++ b/include/hw/vfio/vfio-common.h
>>> @@ -208,6 +208,7 @@ struct vfio_device_info *vfio_get_device_info(int
>> fd);
>>> int vfio_attach_device(char *name, VFIODevice *vbasedev,
>>> AddressSpace *as, Error **errp);
>>> void vfio_detach_device(VFIODevice *vbasedev);
>>> +void host_iommu_device_create(VFIODevice *vbasedev);
>>>
>>> int vfio_kvm_device_add_fd(int fd, Error **errp);
>>> int vfio_kvm_device_del_fd(int fd, Error **errp);
>>> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-
>> container-base.h
>>> index b2813b0c11..dc003f6eb2 100644
>>> --- a/include/hw/vfio/vfio-container-base.h
>>> +++ b/include/hw/vfio/vfio-container-base.h
>>> @@ -120,6 +120,7 @@ struct VFIOIOMMUClass {
>>> int (*attach_device)(const char *name, VFIODevice *vbasedev,
>>> AddressSpace *as, Error **errp);
>>> void (*detach_device)(VFIODevice *vbasedev);
>>> +void (*host_iommu_device_create)(VFIODevice *vbasedev);
>> Maybe return an int instead. It is common the allocation can fail and
>> the deallocation cannot. While at it I would also pass an errp in case
>> it fails
> Currently host_iommu_device_create implementation only calls g_malloc0,
> so never fails, so I returned void.
>
> I'm fine to return an int, will be like below, take iommufd for example:
>
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -651,7 +651,7 @@ static IOMMUFDDeviceOps vfio_iommufd_device_ops = {
> .detach_hwpt = vfio_iommufd_device_detach_hwpt,
> };
>
> -static void vfio_cdev_host_iommu_device_create(VFIODevice *vbasedev)
> +static int vfio_cdev_host_iommu_device_create(VFIODevice *vbasedev, Error
> **errp)
> {
> IOMMUFDDevice *idev = g_malloc0(sizeof(IOMMUFDDevice));
> VFIOIOMMUFDContainer *container = container_of(vbasedev->bcontainer,
> @@ -661,6 +661,8 @@ static void vfio_cdev_host_iommu_device_create(VFIODevice
> *vbasedev)
>
> iommufd_device_init(idev, vbasedev->iommufd, vbasedev->devid,
> container->ioas_id, vbasedev,
> _iommufd_device_ops);
> +
> +return 0;
> }
I think the rationale behind introducing an error handle is to propagate
it down to the callbacks. This is were I anticipated errors to occur.
But if you don't foresee any maybe we can just wait and postpone this
addition.
Thanks
Eric
>
> Thanks
> Zhenzhong
>
>> Eric
>>> /* migration feature */
>>> int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer,
>>> bool start);
>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>>> index 059bfdc07a..41e9031c59 100644
>>> --- a/hw/vfio/common.c
>>> +++ b/hw/vfio/common.c
>>> @@ -1521,3 +1521,11 @@ void vfio_detach_device(VFIODevice
>> *vbasedev)
>>> }
>>> vbasedev->bcontainer->ops->detach_device(vbasedev);
>>> }
>>> +
>>> +void host_iommu_device_create(VFIODevice *vbasedev)
>>> +{
>>> +const VFIOIOMMUClass *ops = vbasedev->bcontainer->ops;
>>> +
>>> +assert(ops->host_iommu_device_create);
>>> +ops->host_iommu_device_create(vbasedev);
>>> +}
RE: [PATCH v1 05/11] vfio: Introduce host_iommu_device_create callback
>-Original Message-
>From: Eric Auger
>Subject: Re: [PATCH v1 05/11] vfio: Introduce host_iommu_device_create
>callback
>
>
>
>On 2/28/24 04:58, Zhenzhong Duan wrote:
>> Introduce host_iommu_device_create callback and a wrapper for it.
>>
>> This callback is used to allocate a host iommu device instance and
>> initialize it based on type.
>>
>> Signed-off-by: Zhenzhong Duan
>> ---
>> include/hw/vfio/vfio-common.h | 1 +
>> include/hw/vfio/vfio-container-base.h | 1 +
>> hw/vfio/common.c | 8
>> 3 files changed, 10 insertions(+)
>>
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
>common.h
>> index b6676c9f79..9fefea4b89 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -208,6 +208,7 @@ struct vfio_device_info *vfio_get_device_info(int
>fd);
>> int vfio_attach_device(char *name, VFIODevice *vbasedev,
>> AddressSpace *as, Error **errp);
>> void vfio_detach_device(VFIODevice *vbasedev);
>> +void host_iommu_device_create(VFIODevice *vbasedev);
>>
>> int vfio_kvm_device_add_fd(int fd, Error **errp);
>> int vfio_kvm_device_del_fd(int fd, Error **errp);
>> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-
>container-base.h
>> index b2813b0c11..dc003f6eb2 100644
>> --- a/include/hw/vfio/vfio-container-base.h
>> +++ b/include/hw/vfio/vfio-container-base.h
>> @@ -120,6 +120,7 @@ struct VFIOIOMMUClass {
>> int (*attach_device)(const char *name, VFIODevice *vbasedev,
>> AddressSpace *as, Error **errp);
>> void (*detach_device)(VFIODevice *vbasedev);
>> +void (*host_iommu_device_create)(VFIODevice *vbasedev);
>Maybe return an int instead. It is common the allocation can fail and
>the deallocation cannot. While at it I would also pass an errp in case
>it fails
Currently host_iommu_device_create implementation only calls g_malloc0,
so never fails, so I returned void.
I'm fine to return an int, will be like below, take iommufd for example:
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -651,7 +651,7 @@ static IOMMUFDDeviceOps vfio_iommufd_device_ops = {
.detach_hwpt = vfio_iommufd_device_detach_hwpt,
};
-static void vfio_cdev_host_iommu_device_create(VFIODevice *vbasedev)
+static int vfio_cdev_host_iommu_device_create(VFIODevice *vbasedev, Error
**errp)
{
IOMMUFDDevice *idev = g_malloc0(sizeof(IOMMUFDDevice));
VFIOIOMMUFDContainer *container = container_of(vbasedev->bcontainer,
@@ -661,6 +661,8 @@ static void vfio_cdev_host_iommu_device_create(VFIODevice
*vbasedev)
iommufd_device_init(idev, vbasedev->iommufd, vbasedev->devid,
container->ioas_id, vbasedev,
_iommufd_device_ops);
+
+return 0;
}
Thanks
Zhenzhong
>
>Eric
>> /* migration feature */
>> int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer,
>> bool start);
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 059bfdc07a..41e9031c59 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -1521,3 +1521,11 @@ void vfio_detach_device(VFIODevice
>*vbasedev)
>> }
>> vbasedev->bcontainer->ops->detach_device(vbasedev);
>> }
>> +
>> +void host_iommu_device_create(VFIODevice *vbasedev)
>> +{
>> +const VFIOIOMMUClass *ops = vbasedev->bcontainer->ops;
>> +
>> +assert(ops->host_iommu_device_create);
>> +ops->host_iommu_device_create(vbasedev);
>> +}
RE: [PATCH v1 05/11] vfio: Introduce host_iommu_device_create callback
>-Original Message-
>From: Eric Auger
>Subject: Re: [PATCH v1 05/11] vfio: Introduce host_iommu_device_create
>callback
>
>
>
>On 3/18/24 14:52, Eric Auger wrote:
>> Hi ZHenzhong,
>>
>> On 2/28/24 04:58, Zhenzhong Duan wrote:
>>> Introduce host_iommu_device_create callback and a wrapper for it.
>>>
>>> This callback is used to allocate a host iommu device instance and
>>> initialize it based on type.
>>>
>>> Signed-off-by: Zhenzhong Duan
>>> ---
>>> include/hw/vfio/vfio-common.h | 1 +
>>> include/hw/vfio/vfio-container-base.h | 1 +
>>> hw/vfio/common.c | 8
>>> 3 files changed, 10 insertions(+)
>>>
>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
>common.h
>>> index b6676c9f79..9fefea4b89 100644
>>> --- a/include/hw/vfio/vfio-common.h
>>> +++ b/include/hw/vfio/vfio-common.h
>>> @@ -208,6 +208,7 @@ struct vfio_device_info *vfio_get_device_info(int
>fd);
>>> int vfio_attach_device(char *name, VFIODevice *vbasedev,
>>> AddressSpace *as, Error **errp);
>>> void vfio_detach_device(VFIODevice *vbasedev);
>>> +void host_iommu_device_create(VFIODevice *vbasedev);
>>>
>>> int vfio_kvm_device_add_fd(int fd, Error **errp);
>>> int vfio_kvm_device_del_fd(int fd, Error **errp);
>>> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-
>container-base.h
>>> index b2813b0c11..dc003f6eb2 100644
>>> --- a/include/hw/vfio/vfio-container-base.h
>>> +++ b/include/hw/vfio/vfio-container-base.h
>>> @@ -120,6 +120,7 @@ struct VFIOIOMMUClass {
>>> int (*attach_device)(const char *name, VFIODevice *vbasedev,
>>> AddressSpace *as, Error **errp);
>>> void (*detach_device)(VFIODevice *vbasedev);
>>> +void (*host_iommu_device_create)(VFIODevice *vbasedev);
>>> /* migration feature */
>>> int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer,
>>> bool start);
>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>>> index 059bfdc07a..41e9031c59 100644
>>> --- a/hw/vfio/common.c
>>> +++ b/hw/vfio/common.c
>>> @@ -1521,3 +1521,11 @@ void vfio_detach_device(VFIODevice
>*vbasedev)
>>> }
>>> vbasedev->bcontainer->ops->detach_device(vbasedev);
>>> }
>>> +
>>> +void host_iommu_device_create(VFIODevice *vbasedev)
>>> +{
>>> +const VFIOIOMMUClass *ops = vbasedev->bcontainer->ops;
>>> +
>>> +assert(ops->host_iommu_device_create);
>> at this stage ops actual implementation do not exist yet so this will
>> break the bisection
>
>Sorry it is OK at the function only is called in
>[PATCH v1 08/11] vfio/pci: Allocate and initialize HostIOMMUDevice after
>attachment
>
>Sorry for the noise
Ah, send too quickly. No problem.
Thanks
Zhenzhong
RE: [PATCH v1 05/11] vfio: Introduce host_iommu_device_create callback
Hi Eric,
>-Original Message-
>From: Eric Auger
>Subject: Re: [PATCH v1 05/11] vfio: Introduce host_iommu_device_create
>callback
>
>Hi ZHenzhong,
>
>On 2/28/24 04:58, Zhenzhong Duan wrote:
>> Introduce host_iommu_device_create callback and a wrapper for it.
>>
>> This callback is used to allocate a host iommu device instance and
>> initialize it based on type.
>>
>> Signed-off-by: Zhenzhong Duan
>> ---
>> include/hw/vfio/vfio-common.h | 1 +
>> include/hw/vfio/vfio-container-base.h | 1 +
>> hw/vfio/common.c | 8
>> 3 files changed, 10 insertions(+)
>>
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
>common.h
>> index b6676c9f79..9fefea4b89 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -208,6 +208,7 @@ struct vfio_device_info *vfio_get_device_info(int
>fd);
>> int vfio_attach_device(char *name, VFIODevice *vbasedev,
>> AddressSpace *as, Error **errp);
>> void vfio_detach_device(VFIODevice *vbasedev);
>> +void host_iommu_device_create(VFIODevice *vbasedev);
>>
>> int vfio_kvm_device_add_fd(int fd, Error **errp);
>> int vfio_kvm_device_del_fd(int fd, Error **errp);
>> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-
>container-base.h
>> index b2813b0c11..dc003f6eb2 100644
>> --- a/include/hw/vfio/vfio-container-base.h
>> +++ b/include/hw/vfio/vfio-container-base.h
>> @@ -120,6 +120,7 @@ struct VFIOIOMMUClass {
>> int (*attach_device)(const char *name, VFIODevice *vbasedev,
>> AddressSpace *as, Error **errp);
>> void (*detach_device)(VFIODevice *vbasedev);
>> +void (*host_iommu_device_create)(VFIODevice *vbasedev);
>> /* migration feature */
>> int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer,
>> bool start);
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 059bfdc07a..41e9031c59 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -1521,3 +1521,11 @@ void vfio_detach_device(VFIODevice
>*vbasedev)
>> }
>> vbasedev->bcontainer->ops->detach_device(vbasedev);
>> }
>> +
>> +void host_iommu_device_create(VFIODevice *vbasedev)
>> +{
>> +const VFIOIOMMUClass *ops = vbasedev->bcontainer->ops;
>> +
>> +assert(ops->host_iommu_device_create);
>at this stage ops actual implementation do not exist yet so this will
>break the bisection
This patch only introcudes host_iommu_device_create but no one call
into it. Patch6-7 implement callback for different backend,
patch8 call host_iommu_device_create(), so I think the order is ok.
Let me know if I missed your points.
Thanks
Zhenzhong
>
>Eric
>> +ops->host_iommu_device_create(vbasedev);
>> +}
Re: [PATCH v1 05/11] vfio: Introduce host_iommu_device_create callback
On 2/28/24 04:58, Zhenzhong Duan wrote:
> Introduce host_iommu_device_create callback and a wrapper for it.
>
> This callback is used to allocate a host iommu device instance and
> initialize it based on type.
>
> Signed-off-by: Zhenzhong Duan
> ---
> include/hw/vfio/vfio-common.h | 1 +
> include/hw/vfio/vfio-container-base.h | 1 +
> hw/vfio/common.c | 8
> 3 files changed, 10 insertions(+)
>
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index b6676c9f79..9fefea4b89 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -208,6 +208,7 @@ struct vfio_device_info *vfio_get_device_info(int fd);
> int vfio_attach_device(char *name, VFIODevice *vbasedev,
> AddressSpace *as, Error **errp);
> void vfio_detach_device(VFIODevice *vbasedev);
> +void host_iommu_device_create(VFIODevice *vbasedev);
>
> int vfio_kvm_device_add_fd(int fd, Error **errp);
> int vfio_kvm_device_del_fd(int fd, Error **errp);
> diff --git a/include/hw/vfio/vfio-container-base.h
> b/include/hw/vfio/vfio-container-base.h
> index b2813b0c11..dc003f6eb2 100644
> --- a/include/hw/vfio/vfio-container-base.h
> +++ b/include/hw/vfio/vfio-container-base.h
> @@ -120,6 +120,7 @@ struct VFIOIOMMUClass {
> int (*attach_device)(const char *name, VFIODevice *vbasedev,
> AddressSpace *as, Error **errp);
> void (*detach_device)(VFIODevice *vbasedev);
> +void (*host_iommu_device_create)(VFIODevice *vbasedev);
Maybe return an int instead. It is common the allocation can fail and
the deallocation cannot. While at it I would also pass an errp in case
it fails
Eric
> /* migration feature */
> int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer,
> bool start);
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 059bfdc07a..41e9031c59 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -1521,3 +1521,11 @@ void vfio_detach_device(VFIODevice *vbasedev)
> }
> vbasedev->bcontainer->ops->detach_device(vbasedev);
> }
> +
> +void host_iommu_device_create(VFIODevice *vbasedev)
> +{
> +const VFIOIOMMUClass *ops = vbasedev->bcontainer->ops;
> +
> +assert(ops->host_iommu_device_create);
> +ops->host_iommu_device_create(vbasedev);
> +}
Re: [PATCH v1 05/11] vfio: Introduce host_iommu_device_create callback
On 3/18/24 14:52, Eric Auger wrote:
> Hi ZHenzhong,
>
> On 2/28/24 04:58, Zhenzhong Duan wrote:
>> Introduce host_iommu_device_create callback and a wrapper for it.
>>
>> This callback is used to allocate a host iommu device instance and
>> initialize it based on type.
>>
>> Signed-off-by: Zhenzhong Duan
>> ---
>> include/hw/vfio/vfio-common.h | 1 +
>> include/hw/vfio/vfio-container-base.h | 1 +
>> hw/vfio/common.c | 8
>> 3 files changed, 10 insertions(+)
>>
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>> index b6676c9f79..9fefea4b89 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -208,6 +208,7 @@ struct vfio_device_info *vfio_get_device_info(int fd);
>> int vfio_attach_device(char *name, VFIODevice *vbasedev,
>> AddressSpace *as, Error **errp);
>> void vfio_detach_device(VFIODevice *vbasedev);
>> +void host_iommu_device_create(VFIODevice *vbasedev);
>>
>> int vfio_kvm_device_add_fd(int fd, Error **errp);
>> int vfio_kvm_device_del_fd(int fd, Error **errp);
>> diff --git a/include/hw/vfio/vfio-container-base.h
>> b/include/hw/vfio/vfio-container-base.h
>> index b2813b0c11..dc003f6eb2 100644
>> --- a/include/hw/vfio/vfio-container-base.h
>> +++ b/include/hw/vfio/vfio-container-base.h
>> @@ -120,6 +120,7 @@ struct VFIOIOMMUClass {
>> int (*attach_device)(const char *name, VFIODevice *vbasedev,
>> AddressSpace *as, Error **errp);
>> void (*detach_device)(VFIODevice *vbasedev);
>> +void (*host_iommu_device_create)(VFIODevice *vbasedev);
>> /* migration feature */
>> int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer,
>> bool start);
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 059bfdc07a..41e9031c59 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -1521,3 +1521,11 @@ void vfio_detach_device(VFIODevice *vbasedev)
>> }
>> vbasedev->bcontainer->ops->detach_device(vbasedev);
>> }
>> +
>> +void host_iommu_device_create(VFIODevice *vbasedev)
>> +{
>> +const VFIOIOMMUClass *ops = vbasedev->bcontainer->ops;
>> +
>> +assert(ops->host_iommu_device_create);
> at this stage ops actual implementation do not exist yet so this will
> break the bisection
Sorry it is OK at the function only is called in
[PATCH v1 08/11] vfio/pci: Allocate and initialize HostIOMMUDevice after
attachment
Sorry for the noise
Eric
>
> Eric
>> +ops->host_iommu_device_create(vbasedev);
>> +}
>
Re: [PATCH v1 05/11] vfio: Introduce host_iommu_device_create callback
Hi ZHenzhong,
On 2/28/24 04:58, Zhenzhong Duan wrote:
> Introduce host_iommu_device_create callback and a wrapper for it.
>
> This callback is used to allocate a host iommu device instance and
> initialize it based on type.
>
> Signed-off-by: Zhenzhong Duan
> ---
> include/hw/vfio/vfio-common.h | 1 +
> include/hw/vfio/vfio-container-base.h | 1 +
> hw/vfio/common.c | 8
> 3 files changed, 10 insertions(+)
>
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index b6676c9f79..9fefea4b89 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -208,6 +208,7 @@ struct vfio_device_info *vfio_get_device_info(int fd);
> int vfio_attach_device(char *name, VFIODevice *vbasedev,
> AddressSpace *as, Error **errp);
> void vfio_detach_device(VFIODevice *vbasedev);
> +void host_iommu_device_create(VFIODevice *vbasedev);
>
> int vfio_kvm_device_add_fd(int fd, Error **errp);
> int vfio_kvm_device_del_fd(int fd, Error **errp);
> diff --git a/include/hw/vfio/vfio-container-base.h
> b/include/hw/vfio/vfio-container-base.h
> index b2813b0c11..dc003f6eb2 100644
> --- a/include/hw/vfio/vfio-container-base.h
> +++ b/include/hw/vfio/vfio-container-base.h
> @@ -120,6 +120,7 @@ struct VFIOIOMMUClass {
> int (*attach_device)(const char *name, VFIODevice *vbasedev,
> AddressSpace *as, Error **errp);
> void (*detach_device)(VFIODevice *vbasedev);
> +void (*host_iommu_device_create)(VFIODevice *vbasedev);
> /* migration feature */
> int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer,
> bool start);
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 059bfdc07a..41e9031c59 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -1521,3 +1521,11 @@ void vfio_detach_device(VFIODevice *vbasedev)
> }
> vbasedev->bcontainer->ops->detach_device(vbasedev);
> }
> +
> +void host_iommu_device_create(VFIODevice *vbasedev)
> +{
> +const VFIOIOMMUClass *ops = vbasedev->bcontainer->ops;
> +
> +assert(ops->host_iommu_device_create);
at this stage ops actual implementation do not exist yet so this will
break the bisection
Eric
> +ops->host_iommu_device_create(vbasedev);
> +}
