>-----Original Message-----
>From: Joao Martins <joao.m.mart...@oracle.com>
>Subject: Re: [PATCH v6 4/9] vfio/{iommufd,container}: Invoke
>HostIOMMUDevice::realize() during attach_device()
>
>On 23/07/2024 08:55, Eric Auger wrote:
>>
>>
>> On 7/23/24 09:44, Cédric Le Goater wrote:
>>> On 7/23/24 09:38, Eric Auger wrote:
>>>> Hi Joao,
>>>>
>>>> On 7/22/24 23:13, Joao Martins wrote:
>>>>> Move the HostIOMMUDevice::realize() to be invoked during the attach
>>>>> of the device
>>>>> before we allocate IOMMUFD hardware pagetable objects (HWPT). This
>>>>> allows the use
>>>>> of the hw_caps obtained by IOMMU_GET_HW_INFO that essentially
>tell
>>>>> if the IOMMU
>>>>> behind the device supports dirty tracking.
>>>>>
>>>>> Note: The HostIOMMUDevice data from legacy backend is static and
>>>>> doesn't
>>>>> need any information from the (type1-iommu) backend to be
>initialized.
>>>>> In contrast however, the IOMMUFD HostIOMMUDevice data requires
>the
>>>>> iommufd FD to be connected and having a devid to be able to
>>>>> successfully
>>>> Nit: maybe this comment shall be also added in iommufd.c before the
>call
>>>> to vfio_device_hiod_realize() to avoid someone else to move that call
>>>> earlier at some point
>>>>> GET_HW_INFO. This means vfio_device_hiod_realize() is called in
>>>>> different places within the backend .attach_device() implementation.
>>>>>
>>>>> Suggested-by: Cédric Le Goater <c...@redhat.cm>
>>>>> Signed-off-by: Joao Martins <joao.m.mart...@oracle.com>
>>>>> Reviewed-by: Zhenzhong Duan <zhenzhong.d...@intel.com>
>>>>> ---
>>>>> include/hw/vfio/vfio-common.h | 1 +
>>>>> hw/vfio/common.c | 16 ++++++----------
>>>>> hw/vfio/container.c | 4 ++++
>>>>> hw/vfio/helpers.c | 11 +++++++++++
>>>>> hw/vfio/iommufd.c | 4 ++++
>>>>> 5 files changed, 26 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/include/hw/vfio/vfio-common.h
>>>>> b/include/hw/vfio/vfio-common.h
>>>>> index 1a96678f8c38..4e44b26d3c45 100644
>>>>> --- a/include/hw/vfio/vfio-common.h
>>>>> +++ b/include/hw/vfio/vfio-common.h
>>>>> @@ -242,6 +242,7 @@ void vfio_region_finalize(VFIORegion *region);
>>>>> void vfio_reset_handler(void *opaque);
>>>>> struct vfio_device_info *vfio_get_device_info(int fd);
>>>>> bool vfio_device_is_mdev(VFIODevice *vbasedev);
>>>>> +bool vfio_device_hiod_realize(VFIODevice *vbasedev, Error **errp);
>>>>> bool vfio_attach_device(char *name, VFIODevice *vbasedev,
>>>>> AddressSpace *as, Error **errp);
>>>>> void vfio_detach_device(VFIODevice *vbasedev);
>>>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>>>>> index 784e266e6aab..da12cbd56408 100644
>>>>> --- a/hw/vfio/common.c
>>>>> +++ b/hw/vfio/common.c
>>>>> @@ -1537,7 +1537,7 @@ bool vfio_attach_device(char *name,
>VFIODevice
>>>>> *vbasedev,
>>>>> {
>>>>> const VFIOIOMMUClass *ops =
>>>>>
>>>>>
>VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_LEGACY));
>>>>> - HostIOMMUDevice *hiod;
>>>>> + HostIOMMUDevice *hiod = NULL;
>>>>> if (vbasedev->iommufd) {
>>>>> ops =
>>>>>
>VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUF
>D));
>>>>> @@ -1545,21 +1545,17 @@ bool vfio_attach_device(char *name,
>>>>> VFIODevice *vbasedev,
>>>>> assert(ops);
>>>>> - if (!ops->attach_device(name, vbasedev, as, errp)) {
>>>>> - return false;
>>>>> - }
>>>>> - if (vbasedev->mdev) {
>>>>> - return true;
>>>>> + if (!vbasedev->mdev) {
>>>>> + hiod = HOST_IOMMU_DEVICE(object_new(ops-
>>hiod_typename));
>>>>> + vbasedev->hiod = hiod;
>>>>> }
>>>>> - hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename));
>>>>> - if (!HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod,
>vbasedev,
>>>>> errp)) {
>>>>> + if (!ops->attach_device(name, vbasedev, as, errp)) {
>>>>> object_unref(hiod);
>>>>> - ops->detach_device(vbasedev);
>>>>> + vbasedev->hiod = NULL;
>>>>> return false;
>>>>> }
>>>>> - vbasedev->hiod = hiod;
>>>>> return true;
>>>>> }
>>>>> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>>>>> index 10cb4b4320ac..9ccdb639ac84 100644
>>>>> --- a/hw/vfio/container.c
>>>>> +++ b/hw/vfio/container.c
>>>>> @@ -914,6 +914,10 @@ static bool vfio_legacy_attach_device(const
>>>>> char *name, VFIODevice *vbasedev,
>>>>> trace_vfio_attach_device(vbasedev->name, groupid);
>>>>> + if (!vfio_device_hiod_realize(vbasedev, errp)) {
>>>>> + return false;
>>>> don't you want to go to err_alloc_ioas instead?
>>>
>>> hmm, the err_alloc_ioas label is in a different function
>>> iommufd_cdev_attach().
>>>
>>> may be you meant the comment for routine iommufd_cdev_attach() and
>>> label err_connect_bind ?
>>>
>>>
>>> Thanks,
>>>
>>> C.
>>>
>>>
>>>>> + }
>>>>> +
>>>>> group = vfio_get_group(groupid, as, errp);
>>>>> if (!group) {
>>>>> return false;
>>>>> diff --git a/hw/vfio/helpers.c b/hw/vfio/helpers.c
>>>>> index 7e23e9080c9d..ea15c79db0a3 100644
>>>>> --- a/hw/vfio/helpers.c
>>>>> +++ b/hw/vfio/helpers.c
>>>>> @@ -689,3 +689,14 @@ bool vfio_device_is_mdev(VFIODevice
>*vbasedev)
>>>>> subsys = realpath(tmp, NULL);
>>>>> return subsys && (strcmp(subsys, "/sys/bus/mdev") == 0);
>>>>> }
>>>>> +
>>>>> +bool vfio_device_hiod_realize(VFIODevice *vbasedev, Error **errp)
>>>>> +{
>>>>> + HostIOMMUDevice *hiod = vbasedev->hiod;
>>>>> +
>>>>> + if (!hiod) {
>>>>> + return true;
>>>>> + }
>>>>> +
>>>>> + return HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod,
>>>>> vbasedev, errp);
>>>>> +}
>>>>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>>>>> index 5e2fc1ce089d..2324bf892c56 100644
>>>>> --- a/hw/vfio/iommufd.c
>>>>> +++ b/hw/vfio/iommufd.c
>>>>> @@ -403,6 +403,10 @@ static bool iommufd_cdev_attach(const char
>>>>> *name, VFIODevice *vbasedev,
>>>>> space = vfio_get_address_space(as);
>>>>> + if (!vfio_device_hiod_realize(vbasedev, errp)) {
>>>>> + return false;
>> Hum sorry my previous comment was targetting that place. I think
>> unrolling is needed up to put_address_space
>>
>> so effectively this does not match err_alloc_ioas but I guess we would
>> need another label
>>
>
>You're right. We haven't yet attached rthe device and that's what
>err_alloc_ioas
>would do. Adding another label not sure would make things cleaner given
>the
>ordering requirement. So maybe this instead?
>
>@@ -482,7 +483,8 @@ static bool iommufd_cdev_attach(const char *name,
>VFIODevice
>*vbasedev,
> space = vfio_get_address_space(as);
>
> if (!vfio_device_hiod_realize(vbasedev, errp)) {
>- return false;
>+ vfio_put_address_space(space);
>+ goto err_connect_bind;
> }
>
> /* try to attach to an existing container in this space */
I was confused though Cedric and Eric both ACK this change. Don't we miss the
iommufd_cdev_unbind_and_disconnect() call?
