| Message ID | 20230926113255.1177834-8-zhenzhong.duan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Prerequisite change for IOMMUFD support | expand |
Hi, On 9/26/23 13:32, Zhenzhong Duan wrote: > From: Eric Auger <eric.auger@redhat.com> > > Let the vfio-platform device use vfio_attach_device() and > vfio_detach_device(), hence hiding the details of the used > IOMMU backend. > > Drop the trace event for vfio-platform as we have similar > one in vfio_attach_device. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- > hw/vfio/platform.c | 43 +++---------------------------------------- > hw/vfio/trace-events | 1 - > 2 files changed, 3 insertions(+), 41 deletions(-) > > diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c > index 5af73f9287..8e3d4ac458 100644 > --- a/hw/vfio/platform.c > +++ b/hw/vfio/platform.c > @@ -529,12 +529,7 @@ static VFIODeviceOps vfio_platform_ops = { > */ > static int vfio_base_device_init(VFIODevice *vbasedev, Error **errp) > { > - VFIOGroup *group; > - VFIODevice *vbasedev_iter; > - char *tmp, group_path[PATH_MAX], *group_name; > - ssize_t len; > struct stat st; > - int groupid; > int ret; > > /* @sysfsdev takes precedence over @host */ > @@ -557,47 +552,15 @@ static int vfio_base_device_init(VFIODevice *vbasedev, Error **errp) > return -errno; > } > > - tmp = g_strdup_printf("%s/iommu_group", vbasedev->sysfsdev); > - len = readlink(tmp, group_path, sizeof(group_path)); > - g_free(tmp); > - > - if (len < 0 || len >= sizeof(group_path)) { > - ret = len < 0 ? -errno : -ENAMETOOLONG; > - error_setg_errno(errp, -ret, "no iommu_group found"); > - return ret; > - } > - > - group_path[len] = 0; > - > - group_name = basename(group_path); > - if (sscanf(group_name, "%d", &groupid) != 1) { > - error_setg_errno(errp, errno, "failed to read %s", group_path); > - return -errno; > - } > - Here also on error path we are leaking both vbasedev->name and vbasedev->sysfsdev. This is independent on this patch care must be taken because vdev->vbasedev.name is used in the caller (vfio_platform_realize) to output the error msg deallocation could happen there? Eric > - trace_vfio_platform_base_device_init(vbasedev->name, groupid); > - > - group = vfio_get_group(groupid, &address_space_memory, errp); > - if (!group) { > - return -ENOENT; > - } > - > - QLIST_FOREACH(vbasedev_iter, &group->device_list, next) { > - if (strcmp(vbasedev_iter->name, vbasedev->name) == 0) { > - error_setg(errp, "device is already attached"); > - vfio_put_group(group); > - return -EBUSY; > - } > - } > - ret = vfio_get_device(group, vbasedev->name, vbasedev, errp); > + ret = vfio_attach_device(vbasedev->name, vbasedev, > + &address_space_memory, errp); > if (ret) { > - vfio_put_group(group); > return ret; > } > > ret = vfio_populate_device(vbasedev, errp); > if (ret) { > - vfio_put_group(group); > + vfio_detach_device(vbasedev); > } > > return ret; > diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events > index e710026a73..3ac914854b 100644 > --- a/hw/vfio/trace-events > +++ b/hw/vfio/trace-events > @@ -120,7 +120,6 @@ vfio_get_dirty_bitmap(int fd, uint64_t iova, uint64_t size, uint64_t bitmap_size > vfio_iommu_map_dirty_notify(uint64_t iova_start, uint64_t iova_end) "iommu dirty @ 0x%"PRIx64" - 0x%"PRIx64 > > # platform.c > -vfio_platform_base_device_init(char *name, int groupid) "%s belongs to group #%d" > vfio_platform_realize(char *name, char *compat) "vfio device %s, compat = %s" > vfio_platform_eoi(int pin, int fd) "EOI IRQ pin %d (fd=%d)" > vfio_platform_intp_mmap_enable(int pin) "IRQ #%d still active, stay in slow path"
>-----Original Message----- >From: Eric Auger <eric.auger@redhat.com> >Sent: Wednesday, September 27, 2023 5:11 PM >Subject: Re: [PATCH v2 07/12] vfio/platform: Use vfio_[attach/detach]_device > >Hi, > >On 9/26/23 13:32, Zhenzhong Duan wrote: >> From: Eric Auger <eric.auger@redhat.com> >> >> Let the vfio-platform device use vfio_attach_device() and >> vfio_detach_device(), hence hiding the details of the used >> IOMMU backend. >> >> Drop the trace event for vfio-platform as we have similar >> one in vfio_attach_device. >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> --- >> hw/vfio/platform.c | 43 +++---------------------------------------- >> hw/vfio/trace-events | 1 - >> 2 files changed, 3 insertions(+), 41 deletions(-) >> >> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c >> index 5af73f9287..8e3d4ac458 100644 >> --- a/hw/vfio/platform.c >> +++ b/hw/vfio/platform.c >> @@ -529,12 +529,7 @@ static VFIODeviceOps vfio_platform_ops = { >> */ >> static int vfio_base_device_init(VFIODevice *vbasedev, Error **errp) >> { >> - VFIOGroup *group; >> - VFIODevice *vbasedev_iter; >> - char *tmp, group_path[PATH_MAX], *group_name; >> - ssize_t len; >> struct stat st; >> - int groupid; >> int ret; >> >> /* @sysfsdev takes precedence over @host */ >> @@ -557,47 +552,15 @@ static int vfio_base_device_init(VFIODevice >*vbasedev, Error **errp) >> return -errno; >> } >> >> - tmp = g_strdup_printf("%s/iommu_group", vbasedev->sysfsdev); >> - len = readlink(tmp, group_path, sizeof(group_path)); >> - g_free(tmp); >> - >> - if (len < 0 || len >= sizeof(group_path)) { >> - ret = len < 0 ? -errno : -ENAMETOOLONG; >> - error_setg_errno(errp, -ret, "no iommu_group found"); >> - return ret; >> - } >> - >> - group_path[len] = 0; >> - >> - group_name = basename(group_path); >> - if (sscanf(group_name, "%d", &groupid) != 1) { >> - error_setg_errno(errp, errno, "failed to read %s", group_path); >> - return -errno; >> - } >> - >Here also on error path we are leaking both vbasedev->name and >vbasedev->sysfsdev. This is independent on this patch >care must be taken because vdev->vbasedev.name is used in the caller >(vfio_platform_realize) to output the error msg >deallocation could happen there? Both are device property, I think they are freed by QOM subsystem. Please fix me if I'm wrong. static Property vfio_platform_dev_properties[] = { DEFINE_PROP_STRING("host", VFIOPlatformDevice, vbasedev.name), DEFINE_PROP_STRING("sysfsdev", VFIOPlatformDevice, vbasedev.sysfsdev), Thanks Zhenzhong
diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c index 5af73f9287..8e3d4ac458 100644 --- a/hw/vfio/platform.c +++ b/hw/vfio/platform.c @@ -529,12 +529,7 @@ static VFIODeviceOps vfio_platform_ops = { */ static int vfio_base_device_init(VFIODevice *vbasedev, Error **errp) { - VFIOGroup *group; - VFIODevice *vbasedev_iter; - char *tmp, group_path[PATH_MAX], *group_name; - ssize_t len; struct stat st; - int groupid; int ret; /* @sysfsdev takes precedence over @host */ @@ -557,47 +552,15 @@ static int vfio_base_device_init(VFIODevice *vbasedev, Error **errp) return -errno; } - tmp = g_strdup_printf("%s/iommu_group", vbasedev->sysfsdev); - len = readlink(tmp, group_path, sizeof(group_path)); - g_free(tmp); - - if (len < 0 || len >= sizeof(group_path)) { - ret = len < 0 ? -errno : -ENAMETOOLONG; - error_setg_errno(errp, -ret, "no iommu_group found"); - return ret; - } - - group_path[len] = 0; - - group_name = basename(group_path); - if (sscanf(group_name, "%d", &groupid) != 1) { - error_setg_errno(errp, errno, "failed to read %s", group_path); - return -errno; - } - - trace_vfio_platform_base_device_init(vbasedev->name, groupid); - - group = vfio_get_group(groupid, &address_space_memory, errp); - if (!group) { - return -ENOENT; - } - - QLIST_FOREACH(vbasedev_iter, &group->device_list, next) { - if (strcmp(vbasedev_iter->name, vbasedev->name) == 0) { - error_setg(errp, "device is already attached"); - vfio_put_group(group); - return -EBUSY; - } - } - ret = vfio_get_device(group, vbasedev->name, vbasedev, errp); + ret = vfio_attach_device(vbasedev->name, vbasedev, + &address_space_memory, errp); if (ret) { - vfio_put_group(group); return ret; } ret = vfio_populate_device(vbasedev, errp); if (ret) { - vfio_put_group(group); + vfio_detach_device(vbasedev); } return ret; diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events index e710026a73..3ac914854b 100644 --- a/hw/vfio/trace-events +++ b/hw/vfio/trace-events @@ -120,7 +120,6 @@ vfio_get_dirty_bitmap(int fd, uint64_t iova, uint64_t size, uint64_t bitmap_size vfio_iommu_map_dirty_notify(uint64_t iova_start, uint64_t iova_end) "iommu dirty @ 0x%"PRIx64" - 0x%"PRIx64 # platform.c -vfio_platform_base_device_init(char *name, int groupid) "%s belongs to group #%d" vfio_platform_realize(char *name, char *compat) "vfio device %s, compat = %s" vfio_platform_eoi(int pin, int fd) "EOI IRQ pin %d (fd=%d)" vfio_platform_intp_mmap_enable(int pin) "IRQ #%d still active, stay in slow path"