On 23/07/2024 08: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_IOMMUFD));
>> @@ -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?
Oh, yes, I thought I was doing that, but I am not :( Thanks for catching that
Your comment is spot on but in the wrong place.
vfio_legacy_attach_device() can just return false as it's at the top of the
function but here (...)
>> + }
>> +
>> 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;
>> + }
>> +
(...) we definitely need a goto err_alloc_ioas here.
Snip below:
@@ -482,7 +483,7 @@ 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;
+ goto err_alloc_ioas;
}
/* try to attach to an existing container in this space */
