* [PATCH 0/3] Cleanup VFIOIOMMUClass callback return with bool
@ 2024-05-06 8:33 Zhenzhong Duan
2024-05-06 8:33 ` [PATCH 1/3] vfio: Make VFIOIOMMUClass::attach_device() and its wrapper return bool Zhenzhong Duan
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Zhenzhong Duan @ 2024-05-06 8:33 UTC (permalink / raw)
To: qemu-devel; +Cc: alex.williamson, clg, eric.auger, chao.p.peng, Zhenzhong Duan
Hi
This is a cleanup series to change VFIOIOMMUClass callbacks to return
bool when the error is passed through errp parameter.
See discussion at https://lists.gnu.org/archive/html/qemu-devel/2024-04/msg04782.html
It looks many functions in VFIO sub-system need same change,
so this can be a very first series.
Test done on x86 platform:
vfio device hotplug/unplug with different backend
reboot
Thanks
Zhenzhong
Zhenzhong Duan (3):
vfio: Make VFIOIOMMUClass::attach_device() and its wrapper return bool
vfio: Make VFIOIOMMUClass::setup() return bool
vfio: Make VFIOIOMMUClass::add_window() and its wrapper return bool
include/hw/vfio/vfio-common.h | 4 ++--
include/hw/vfio/vfio-container-base.h | 18 ++++++++---------
hw/vfio/ap.c | 6 ++----
hw/vfio/ccw.c | 6 ++----
hw/vfio/common.c | 6 +++---
hw/vfio/container-base.c | 8 ++++----
hw/vfio/container.c | 24 +++++++++++------------
hw/vfio/iommufd.c | 11 +++++------
hw/vfio/pci.c | 8 +++-----
hw/vfio/platform.c | 7 +++----
hw/vfio/spapr.c | 28 +++++++++++++--------------
11 files changed, 58 insertions(+), 68 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1/3] vfio: Make VFIOIOMMUClass::attach_device() and its wrapper return bool 2024-05-06 8:33 [PATCH 0/3] Cleanup VFIOIOMMUClass callback return with bool Zhenzhong Duan @ 2024-05-06 8:33 ` Zhenzhong Duan 2024-05-06 11:59 ` Cédric Le Goater 2024-05-06 8:33 ` [PATCH 2/3] vfio: Make VFIOIOMMUClass::setup() " Zhenzhong Duan 2024-05-06 8:33 ` [PATCH 3/3] vfio: Make VFIOIOMMUClass::add_window() and its wrapper " Zhenzhong Duan 2 siblings, 1 reply; 10+ messages in thread From: Zhenzhong Duan @ 2024-05-06 8:33 UTC (permalink / raw) To: qemu-devel Cc: alex.williamson, clg, eric.auger, chao.p.peng, Zhenzhong Duan, Tony Krowiak, Halil Pasic, Jason Herne, Thomas Huth, Eric Farman, Matthew Rosato, open list:vfio-ap Make VFIOIOMMUClass::attach_device() and its wrapper function vfio_attach_device() return bool. This is to follow the coding standand to return bool if 'Error **' is used to pass error. Suggested-by: Cédric Le Goater <clg@redhat.com> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> --- include/hw/vfio/vfio-common.h | 4 ++-- include/hw/vfio/vfio-container-base.h | 4 ++-- hw/vfio/ap.c | 6 ++---- hw/vfio/ccw.c | 6 ++---- hw/vfio/common.c | 4 ++-- hw/vfio/container.c | 14 +++++++------- hw/vfio/iommufd.c | 11 +++++------ hw/vfio/pci.c | 8 +++----- hw/vfio/platform.c | 7 +++---- 9 files changed, 28 insertions(+), 36 deletions(-) diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h index b9da6c08ef..a7b6fc8f46 100644 --- a/include/hw/vfio/vfio-common.h +++ b/include/hw/vfio/vfio-common.h @@ -198,8 +198,8 @@ void vfio_region_exit(VFIORegion *region); void vfio_region_finalize(VFIORegion *region); void vfio_reset_handler(void *opaque); struct vfio_device_info *vfio_get_device_info(int fd); -int vfio_attach_device(char *name, VFIODevice *vbasedev, - AddressSpace *as, Error **errp); +bool vfio_attach_device(char *name, VFIODevice *vbasedev, + AddressSpace *as, Error **errp); void vfio_detach_device(VFIODevice *vbasedev); int vfio_kvm_device_add_fd(int fd, Error **errp); diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h index 3582d5f97a..c839cfd9cb 100644 --- a/include/hw/vfio/vfio-container-base.h +++ b/include/hw/vfio/vfio-container-base.h @@ -118,8 +118,8 @@ struct VFIOIOMMUClass { int (*dma_unmap)(const VFIOContainerBase *bcontainer, hwaddr iova, ram_addr_t size, IOMMUTLBEntry *iotlb); - int (*attach_device)(const char *name, VFIODevice *vbasedev, - AddressSpace *as, Error **errp); + bool (*attach_device)(const char *name, VFIODevice *vbasedev, + AddressSpace *as, Error **errp); void (*detach_device)(VFIODevice *vbasedev); /* migration feature */ int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer, diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c index 7c4caa5938..d50600b702 100644 --- a/hw/vfio/ap.c +++ b/hw/vfio/ap.c @@ -156,7 +156,6 @@ static void vfio_ap_unregister_irq_notifier(VFIOAPDevice *vapdev, static void vfio_ap_realize(DeviceState *dev, Error **errp) { ERRP_GUARD(); - int ret; Error *err = NULL; VFIOAPDevice *vapdev = VFIO_AP_DEVICE(dev); VFIODevice *vbasedev = &vapdev->vdev; @@ -165,9 +164,8 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp) return; } - ret = vfio_attach_device(vbasedev->name, vbasedev, - &address_space_memory, errp); - if (ret) { + if (!vfio_attach_device(vbasedev->name, vbasedev, + &address_space_memory, errp)) { goto error; } diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c index 90e4a53437..782bd4bed7 100644 --- a/hw/vfio/ccw.c +++ b/hw/vfio/ccw.c @@ -580,7 +580,6 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp) S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev); VFIODevice *vbasedev = &vcdev->vdev; Error *err = NULL; - int ret; /* Call the class init function for subchannel. */ if (cdc->realize) { @@ -594,9 +593,8 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp) return; } - ret = vfio_attach_device(cdev->mdevid, vbasedev, - &address_space_memory, errp); - if (ret) { + if (!vfio_attach_device(cdev->mdevid, vbasedev, + &address_space_memory, errp)) { goto out_attach_dev_err; } diff --git a/hw/vfio/common.c b/hw/vfio/common.c index 8f9cbdc026..890d30910e 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -1492,8 +1492,8 @@ retry: return info; } -int vfio_attach_device(char *name, VFIODevice *vbasedev, - AddressSpace *as, Error **errp) +bool vfio_attach_device(char *name, VFIODevice *vbasedev, + AddressSpace *as, Error **errp) { const VFIOIOMMUClass *ops = VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_LEGACY)); diff --git a/hw/vfio/container.c b/hw/vfio/container.c index 77bdec276e..ea3b145913 100644 --- a/hw/vfio/container.c +++ b/hw/vfio/container.c @@ -908,8 +908,8 @@ static int vfio_device_groupid(VFIODevice *vbasedev, Error **errp) * @name and @vbasedev->name are likely to be different depending * on the type of the device, hence the need for passing @name */ -static int vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev, - AddressSpace *as, Error **errp) +static bool vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev, + AddressSpace *as, Error **errp) { int groupid = vfio_device_groupid(vbasedev, errp); VFIODevice *vbasedev_iter; @@ -918,27 +918,27 @@ static int vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev, int ret; if (groupid < 0) { - return groupid; + return false; } trace_vfio_attach_device(vbasedev->name, groupid); group = vfio_get_group(groupid, as, errp); if (!group) { - return -ENOENT; + return false; } 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; + return false; } } ret = vfio_get_device(group, name, vbasedev, errp); if (ret) { vfio_put_group(group); - return ret; + return false; } bcontainer = &group->container->bcontainer; @@ -946,7 +946,7 @@ static int vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev, QLIST_INSERT_HEAD(&bcontainer->device_list, vbasedev, container_next); QLIST_INSERT_HEAD(&vfio_device_list, vbasedev, global_next); - return ret; + return true; } static void vfio_legacy_detach_device(VFIODevice *vbasedev) diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c index 8827ffe636..9aa0dd6d8e 100644 --- a/hw/vfio/iommufd.c +++ b/hw/vfio/iommufd.c @@ -301,8 +301,8 @@ error: return ret; } -static int iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, - AddressSpace *as, Error **errp) +static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, + AddressSpace *as, Error **errp) { VFIOContainerBase *bcontainer; VFIOIOMMUFDContainer *container; @@ -317,7 +317,7 @@ static int iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, if (vbasedev->fd < 0) { devfd = iommufd_cdev_getfd(vbasedev->sysfsdev, errp); if (devfd < 0) { - return devfd; + return false; } vbasedev->fd = devfd; } else { @@ -394,7 +394,6 @@ static int iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, memory_listener_register(&bcontainer->listener, bcontainer->space->as); if (bcontainer->error) { - ret = -1; error_propagate_prepend(errp, bcontainer->error, "memory listener initialization failed: "); goto err_listener_register; @@ -433,7 +432,7 @@ found_container: trace_iommufd_cdev_device_info(vbasedev->name, devfd, vbasedev->num_irqs, vbasedev->num_regions, vbasedev->flags); - return 0; + return true; err_listener_register: iommufd_cdev_ram_block_discard_disable(false); @@ -446,7 +445,7 @@ err_alloc_ioas: iommufd_cdev_unbind_and_disconnect(vbasedev); err_connect_bind: close(vbasedev->fd); - return ret; + return false; } static void iommufd_cdev_detach(VFIODevice *vbasedev) diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 64780d1b79..952e4b1a25 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -2951,7 +2951,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) int i, ret; bool is_mdev; char uuid[UUID_STR_LEN]; - char *name; + g_autofree char *name = NULL; if (vbasedev->fd < 0 && !vbasedev->sysfsdev) { if (!(~vdev->host.domain || ~vdev->host.bus || @@ -3001,10 +3001,8 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) name = g_strdup(vbasedev->name); } - ret = vfio_attach_device(name, vbasedev, - pci_device_iommu_address_space(pdev), errp); - g_free(name); - if (ret) { + if (!vfio_attach_device(name, vbasedev, + pci_device_iommu_address_space(pdev), errp)) { goto error; } diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c index dcd2365fb3..2bd16096bb 100644 --- a/hw/vfio/platform.c +++ b/hw/vfio/platform.c @@ -552,10 +552,9 @@ static int vfio_base_device_init(VFIODevice *vbasedev, Error **errp) return ret; } - ret = vfio_attach_device(vbasedev->name, vbasedev, - &address_space_memory, errp); - if (ret) { - return ret; + if (!vfio_attach_device(vbasedev->name, vbasedev, + &address_space_memory, errp)) { + return -EINVAL; } ret = vfio_populate_device(vbasedev, errp); -- 2.34.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] vfio: Make VFIOIOMMUClass::attach_device() and its wrapper return bool 2024-05-06 8:33 ` [PATCH 1/3] vfio: Make VFIOIOMMUClass::attach_device() and its wrapper return bool Zhenzhong Duan @ 2024-05-06 11:59 ` Cédric Le Goater 2024-05-07 2:09 ` Duan, Zhenzhong 0 siblings, 1 reply; 10+ messages in thread From: Cédric Le Goater @ 2024-05-06 11:59 UTC (permalink / raw) To: Zhenzhong Duan, qemu-devel Cc: alex.williamson, eric.auger, chao.p.peng, Tony Krowiak, Halil Pasic, Jason Herne, Thomas Huth, Eric Farman, Matthew Rosato, open list:vfio-ap On 5/6/24 10:33, Zhenzhong Duan wrote: > Make VFIOIOMMUClass::attach_device() and its wrapper function > vfio_attach_device() return bool. > > This is to follow the coding standand to return bool if 'Error **' > is used to pass error. > > Suggested-by: Cédric Le Goater <clg@redhat.com> > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- > include/hw/vfio/vfio-common.h | 4 ++-- > include/hw/vfio/vfio-container-base.h | 4 ++-- > hw/vfio/ap.c | 6 ++---- > hw/vfio/ccw.c | 6 ++---- > hw/vfio/common.c | 4 ++-- > hw/vfio/container.c | 14 +++++++------- > hw/vfio/iommufd.c | 11 +++++------ > hw/vfio/pci.c | 8 +++----- > hw/vfio/platform.c | 7 +++---- > 9 files changed, 28 insertions(+), 36 deletions(-) > > diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h > index b9da6c08ef..a7b6fc8f46 100644 > --- a/include/hw/vfio/vfio-common.h > +++ b/include/hw/vfio/vfio-common.h > @@ -198,8 +198,8 @@ void vfio_region_exit(VFIORegion *region); > void vfio_region_finalize(VFIORegion *region); > void vfio_reset_handler(void *opaque); > struct vfio_device_info *vfio_get_device_info(int fd); > -int vfio_attach_device(char *name, VFIODevice *vbasedev, > - AddressSpace *as, Error **errp); > +bool vfio_attach_device(char *name, VFIODevice *vbasedev, > + AddressSpace *as, Error **errp); > void vfio_detach_device(VFIODevice *vbasedev); > > int vfio_kvm_device_add_fd(int fd, Error **errp); > diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h > index 3582d5f97a..c839cfd9cb 100644 > --- a/include/hw/vfio/vfio-container-base.h > +++ b/include/hw/vfio/vfio-container-base.h > @@ -118,8 +118,8 @@ struct VFIOIOMMUClass { > int (*dma_unmap)(const VFIOContainerBase *bcontainer, > hwaddr iova, ram_addr_t size, > IOMMUTLBEntry *iotlb); > - int (*attach_device)(const char *name, VFIODevice *vbasedev, > - AddressSpace *as, Error **errp); > + bool (*attach_device)(const char *name, VFIODevice *vbasedev, > + AddressSpace *as, Error **errp); > void (*detach_device)(VFIODevice *vbasedev); > /* migration feature */ > int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer, > diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c > index 7c4caa5938..d50600b702 100644 > --- a/hw/vfio/ap.c > +++ b/hw/vfio/ap.c > @@ -156,7 +156,6 @@ static void vfio_ap_unregister_irq_notifier(VFIOAPDevice *vapdev, > static void vfio_ap_realize(DeviceState *dev, Error **errp) > { > ERRP_GUARD(); > - int ret; > Error *err = NULL; > VFIOAPDevice *vapdev = VFIO_AP_DEVICE(dev); > VFIODevice *vbasedev = &vapdev->vdev; > @@ -165,9 +164,8 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp) > return; > } > > - ret = vfio_attach_device(vbasedev->name, vbasedev, > - &address_space_memory, errp); > - if (ret) { > + if (!vfio_attach_device(vbasedev->name, vbasedev, > + &address_space_memory, errp)) { > goto error; > } > > diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c > index 90e4a53437..782bd4bed7 100644 > --- a/hw/vfio/ccw.c > +++ b/hw/vfio/ccw.c > @@ -580,7 +580,6 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp) > S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev); > VFIODevice *vbasedev = &vcdev->vdev; > Error *err = NULL; > - int ret; > > /* Call the class init function for subchannel. */ > if (cdc->realize) { > @@ -594,9 +593,8 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp) > return; > } > > - ret = vfio_attach_device(cdev->mdevid, vbasedev, > - &address_space_memory, errp); > - if (ret) { > + if (!vfio_attach_device(cdev->mdevid, vbasedev, > + &address_space_memory, errp)) { > goto out_attach_dev_err; > } > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index 8f9cbdc026..890d30910e 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -1492,8 +1492,8 @@ retry: > return info; > } > > -int vfio_attach_device(char *name, VFIODevice *vbasedev, > - AddressSpace *as, Error **errp) > +bool vfio_attach_device(char *name, VFIODevice *vbasedev, > + AddressSpace *as, Error **errp) > { > const VFIOIOMMUClass *ops = > VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_LEGACY)); I think vfio_attach_device() can be cleaned up a little further : ret = ops->attach_device(name, vbasedev, as, errp); if (ret < 0) { return ret; } > diff --git a/hw/vfio/container.c b/hw/vfio/container.c > index 77bdec276e..ea3b145913 100644 > --- a/hw/vfio/container.c > +++ b/hw/vfio/container.c > @@ -908,8 +908,8 @@ static int vfio_device_groupid(VFIODevice *vbasedev, Error **errp) > * @name and @vbasedev->name are likely to be different depending > * on the type of the device, hence the need for passing @name > */ > -static int vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev, > - AddressSpace *as, Error **errp) > +static bool vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev, > + AddressSpace *as, Error **errp) > { > int groupid = vfio_device_groupid(vbasedev, errp); > VFIODevice *vbasedev_iter; > @@ -918,27 +918,27 @@ static int vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev, > int ret; > > if (groupid < 0) { > - return groupid; > + return false; > } > > trace_vfio_attach_device(vbasedev->name, groupid); > > group = vfio_get_group(groupid, as, errp); > if (!group) { > - return -ENOENT; > + return false; > } > > 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; > + return false; > } > } > ret = vfio_get_device(group, name, vbasedev, errp); > if (ret) { vfio_get_device() would be the next candidate for cleanup. > vfio_put_group(group); > - return ret; > + return false; > } > > bcontainer = &group->container->bcontainer; > @@ -946,7 +946,7 @@ static int vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev, > QLIST_INSERT_HEAD(&bcontainer->device_list, vbasedev, container_next); > QLIST_INSERT_HEAD(&vfio_device_list, vbasedev, global_next); > > - return ret; > + return true; > } > > static void vfio_legacy_detach_device(VFIODevice *vbasedev) > diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c > index 8827ffe636..9aa0dd6d8e 100644 > --- a/hw/vfio/iommufd.c > +++ b/hw/vfio/iommufd.c > @@ -301,8 +301,8 @@ error: > return ret; > } > > -static int iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, > - AddressSpace *as, Error **errp) > +static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, > + AddressSpace *as, Error **errp) > { > VFIOContainerBase *bcontainer; > VFIOIOMMUFDContainer *container; > @@ -317,7 +317,7 @@ static int iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, > if (vbasedev->fd < 0) { > devfd = iommufd_cdev_getfd(vbasedev->sysfsdev, errp); > if (devfd < 0) { > - return devfd; > + return false; > } > vbasedev->fd = devfd; > } else { > @@ -394,7 +394,6 @@ static int iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, > memory_listener_register(&bcontainer->listener, bcontainer->space->as); > > if (bcontainer->error) { > - ret = -1; > error_propagate_prepend(errp, bcontainer->error, > "memory listener initialization failed: "); > goto err_listener_register; > @@ -433,7 +432,7 @@ found_container: > > trace_iommufd_cdev_device_info(vbasedev->name, devfd, vbasedev->num_irqs, > vbasedev->num_regions, vbasedev->flags); > - return 0; > + return true; > > err_listener_register: > iommufd_cdev_ram_block_discard_disable(false); > @@ -446,7 +445,7 @@ err_alloc_ioas: > iommufd_cdev_unbind_and_disconnect(vbasedev); > err_connect_bind: > close(vbasedev->fd); > - return ret; > + return false; > } > > static void iommufd_cdev_detach(VFIODevice *vbasedev) > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index 64780d1b79..952e4b1a25 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -2951,7 +2951,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > int i, ret; > bool is_mdev; > char uuid[UUID_STR_LEN]; > - char *name; > + g_autofree char *name = NULL; > > if (vbasedev->fd < 0 && !vbasedev->sysfsdev) { > if (!(~vdev->host.domain || ~vdev->host.bus || > @@ -3001,10 +3001,8 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > name = g_strdup(vbasedev->name); > } > > - ret = vfio_attach_device(name, vbasedev, > - pci_device_iommu_address_space(pdev), errp); > - g_free(name); This change would deserve another patch. Thanks, C. > - if (ret) { > + if (!vfio_attach_device(name, vbasedev, > + pci_device_iommu_address_space(pdev), errp)) { > goto error; > } > > diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c > index dcd2365fb3..2bd16096bb 100644 > --- a/hw/vfio/platform.c > +++ b/hw/vfio/platform.c > @@ -552,10 +552,9 @@ static int vfio_base_device_init(VFIODevice *vbasedev, Error **errp) > return ret; > } > > - ret = vfio_attach_device(vbasedev->name, vbasedev, > - &address_space_memory, errp); > - if (ret) { > - return ret; > + if (!vfio_attach_device(vbasedev->name, vbasedev, > + &address_space_memory, errp)) { > + return -EINVAL; > } > > ret = vfio_populate_device(vbasedev, errp); ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH 1/3] vfio: Make VFIOIOMMUClass::attach_device() and its wrapper return bool 2024-05-06 11:59 ` Cédric Le Goater @ 2024-05-07 2:09 ` Duan, Zhenzhong 2024-05-07 5:57 ` Cédric Le Goater 0 siblings, 1 reply; 10+ messages in thread From: Duan, Zhenzhong @ 2024-05-07 2:09 UTC (permalink / raw) To: Cédric Le Goater, qemu-devel@nongnu.org Cc: alex.williamson@redhat.com, eric.auger@redhat.com, Peng, Chao P, Tony Krowiak, Halil Pasic, Jason Herne, Thomas Huth, Eric Farman, Matthew Rosato, open list:vfio-ap >-----Original Message----- >From: Cédric Le Goater <clg@redhat.com> >Subject: Re: [PATCH 1/3] vfio: Make VFIOIOMMUClass::attach_device() and >its wrapper return bool > >On 5/6/24 10:33, Zhenzhong Duan wrote: >> Make VFIOIOMMUClass::attach_device() and its wrapper function >> vfio_attach_device() return bool. >> >> This is to follow the coding standand to return bool if 'Error **' >> is used to pass error. >> >> Suggested-by: Cédric Le Goater <clg@redhat.com> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> --- >> include/hw/vfio/vfio-common.h | 4 ++-- >> include/hw/vfio/vfio-container-base.h | 4 ++-- >> hw/vfio/ap.c | 6 ++---- >> hw/vfio/ccw.c | 6 ++---- >> hw/vfio/common.c | 4 ++-- >> hw/vfio/container.c | 14 +++++++------- >> hw/vfio/iommufd.c | 11 +++++------ >> hw/vfio/pci.c | 8 +++----- >> hw/vfio/platform.c | 7 +++---- >> 9 files changed, 28 insertions(+), 36 deletions(-) >> >> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio- >common.h >> index b9da6c08ef..a7b6fc8f46 100644 >> --- a/include/hw/vfio/vfio-common.h >> +++ b/include/hw/vfio/vfio-common.h >> @@ -198,8 +198,8 @@ void vfio_region_exit(VFIORegion *region); >> void vfio_region_finalize(VFIORegion *region); >> void vfio_reset_handler(void *opaque); >> struct vfio_device_info *vfio_get_device_info(int fd); >> -int vfio_attach_device(char *name, VFIODevice *vbasedev, >> - AddressSpace *as, Error **errp); >> +bool vfio_attach_device(char *name, VFIODevice *vbasedev, >> + AddressSpace *as, Error **errp); >> void vfio_detach_device(VFIODevice *vbasedev); >> >> int vfio_kvm_device_add_fd(int fd, Error **errp); >> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio- >container-base.h >> index 3582d5f97a..c839cfd9cb 100644 >> --- a/include/hw/vfio/vfio-container-base.h >> +++ b/include/hw/vfio/vfio-container-base.h >> @@ -118,8 +118,8 @@ struct VFIOIOMMUClass { >> int (*dma_unmap)(const VFIOContainerBase *bcontainer, >> hwaddr iova, ram_addr_t size, >> IOMMUTLBEntry *iotlb); >> - int (*attach_device)(const char *name, VFIODevice *vbasedev, >> - AddressSpace *as, Error **errp); >> + bool (*attach_device)(const char *name, VFIODevice *vbasedev, >> + AddressSpace *as, Error **errp); >> void (*detach_device)(VFIODevice *vbasedev); >> /* migration feature */ >> int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer, >> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c >> index 7c4caa5938..d50600b702 100644 >> --- a/hw/vfio/ap.c >> +++ b/hw/vfio/ap.c >> @@ -156,7 +156,6 @@ static void >vfio_ap_unregister_irq_notifier(VFIOAPDevice *vapdev, >> static void vfio_ap_realize(DeviceState *dev, Error **errp) >> { >> ERRP_GUARD(); >> - int ret; >> Error *err = NULL; >> VFIOAPDevice *vapdev = VFIO_AP_DEVICE(dev); >> VFIODevice *vbasedev = &vapdev->vdev; >> @@ -165,9 +164,8 @@ static void vfio_ap_realize(DeviceState *dev, Error >**errp) >> return; >> } >> >> - ret = vfio_attach_device(vbasedev->name, vbasedev, >> - &address_space_memory, errp); >> - if (ret) { >> + if (!vfio_attach_device(vbasedev->name, vbasedev, >> + &address_space_memory, errp)) { >> goto error; >> } >> >> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c >> index 90e4a53437..782bd4bed7 100644 >> --- a/hw/vfio/ccw.c >> +++ b/hw/vfio/ccw.c >> @@ -580,7 +580,6 @@ static void vfio_ccw_realize(DeviceState *dev, >Error **errp) >> S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev); >> VFIODevice *vbasedev = &vcdev->vdev; >> Error *err = NULL; >> - int ret; >> >> /* Call the class init function for subchannel. */ >> if (cdc->realize) { >> @@ -594,9 +593,8 @@ static void vfio_ccw_realize(DeviceState *dev, >Error **errp) >> return; >> } >> >> - ret = vfio_attach_device(cdev->mdevid, vbasedev, >> - &address_space_memory, errp); >> - if (ret) { >> + if (!vfio_attach_device(cdev->mdevid, vbasedev, >> + &address_space_memory, errp)) { >> goto out_attach_dev_err; >> } >> >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >> index 8f9cbdc026..890d30910e 100644 >> --- a/hw/vfio/common.c >> +++ b/hw/vfio/common.c >> @@ -1492,8 +1492,8 @@ retry: >> return info; >> } >> >> -int vfio_attach_device(char *name, VFIODevice *vbasedev, >> - AddressSpace *as, Error **errp) >> +bool vfio_attach_device(char *name, VFIODevice *vbasedev, >> + AddressSpace *as, Error **errp) >> { >> const VFIOIOMMUClass *ops = >> >VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_LEGACY)); > > >I think vfio_attach_device() can be cleaned up a little further : > > ret = ops->attach_device(name, vbasedev, as, errp); > if (ret < 0) { > return ret; > } Not understand this. I have both ops->attach_device() and vfio_attach_device() return bool in this patch. Do you mean drop the change for ops->attach_device()? Or split the two changes? > > >> diff --git a/hw/vfio/container.c b/hw/vfio/container.c >> index 77bdec276e..ea3b145913 100644 >> --- a/hw/vfio/container.c >> +++ b/hw/vfio/container.c >> @@ -908,8 +908,8 @@ static int vfio_device_groupid(VFIODevice >*vbasedev, Error **errp) >> * @name and @vbasedev->name are likely to be different depending >> * on the type of the device, hence the need for passing @name >> */ >> -static int vfio_legacy_attach_device(const char *name, VFIODevice >*vbasedev, >> - AddressSpace *as, Error **errp) >> +static bool vfio_legacy_attach_device(const char *name, VFIODevice >*vbasedev, >> + AddressSpace *as, Error **errp) >> { >> int groupid = vfio_device_groupid(vbasedev, errp); >> VFIODevice *vbasedev_iter; >> @@ -918,27 +918,27 @@ static int vfio_legacy_attach_device(const char >*name, VFIODevice *vbasedev, >> int ret; >> >> if (groupid < 0) { >> - return groupid; >> + return false; >> } >> >> trace_vfio_attach_device(vbasedev->name, groupid); >> >> group = vfio_get_group(groupid, as, errp); >> if (!group) { >> - return -ENOENT; >> + return false; >> } >> >> 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; >> + return false; >> } >> } >> ret = vfio_get_device(group, name, vbasedev, errp); >> if (ret) { > >vfio_get_device() would be the next candidate for cleanup. Yes. > >> vfio_put_group(group); >> - return ret; >> + return false; >> } >> >> bcontainer = &group->container->bcontainer; >> @@ -946,7 +946,7 @@ static int vfio_legacy_attach_device(const char >*name, VFIODevice *vbasedev, >> QLIST_INSERT_HEAD(&bcontainer->device_list, vbasedev, >container_next); >> QLIST_INSERT_HEAD(&vfio_device_list, vbasedev, global_next); >> >> - return ret; >> + return true; >> } >> >> static void vfio_legacy_detach_device(VFIODevice *vbasedev) >> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c >> index 8827ffe636..9aa0dd6d8e 100644 >> --- a/hw/vfio/iommufd.c >> +++ b/hw/vfio/iommufd.c >> @@ -301,8 +301,8 @@ error: >> return ret; >> } >> >> -static int iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, >> - AddressSpace *as, Error **errp) >> +static bool iommufd_cdev_attach(const char *name, VFIODevice >*vbasedev, >> + AddressSpace *as, Error **errp) >> { >> VFIOContainerBase *bcontainer; >> VFIOIOMMUFDContainer *container; >> @@ -317,7 +317,7 @@ static int iommufd_cdev_attach(const char *name, >VFIODevice *vbasedev, >> if (vbasedev->fd < 0) { >> devfd = iommufd_cdev_getfd(vbasedev->sysfsdev, errp); >> if (devfd < 0) { >> - return devfd; >> + return false; >> } >> vbasedev->fd = devfd; >> } else { >> @@ -394,7 +394,6 @@ static int iommufd_cdev_attach(const char *name, >VFIODevice *vbasedev, >> memory_listener_register(&bcontainer->listener, bcontainer->space- >>as); >> >> if (bcontainer->error) { >> - ret = -1; >> error_propagate_prepend(errp, bcontainer->error, >> "memory listener initialization failed: "); >> goto err_listener_register; >> @@ -433,7 +432,7 @@ found_container: >> >> trace_iommufd_cdev_device_info(vbasedev->name, devfd, vbasedev- >>num_irqs, >> vbasedev->num_regions, vbasedev->flags); >> - return 0; >> + return true; >> >> err_listener_register: >> iommufd_cdev_ram_block_discard_disable(false); >> @@ -446,7 +445,7 @@ err_alloc_ioas: >> iommufd_cdev_unbind_and_disconnect(vbasedev); >> err_connect_bind: >> close(vbasedev->fd); >> - return ret; >> + return false; >> } >> >> static void iommufd_cdev_detach(VFIODevice *vbasedev) >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >> index 64780d1b79..952e4b1a25 100644 >> --- a/hw/vfio/pci.c >> +++ b/hw/vfio/pci.c >> @@ -2951,7 +2951,7 @@ static void vfio_realize(PCIDevice *pdev, Error >**errp) >> int i, ret; >> bool is_mdev; >> char uuid[UUID_STR_LEN]; >> - char *name; >> + g_autofree char *name = NULL; >> >> if (vbasedev->fd < 0 && !vbasedev->sysfsdev) { >> if (!(~vdev->host.domain || ~vdev->host.bus || >> @@ -3001,10 +3001,8 @@ static void vfio_realize(PCIDevice *pdev, Error >**errp) >> name = g_strdup(vbasedev->name); >> } >> >> - ret = vfio_attach_device(name, vbasedev, >> - pci_device_iommu_address_space(pdev), errp); >> - g_free(name); > >This change would deserve another patch. Will do. Thanks Zhenzhong ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] vfio: Make VFIOIOMMUClass::attach_device() and its wrapper return bool 2024-05-07 2:09 ` Duan, Zhenzhong @ 2024-05-07 5:57 ` Cédric Le Goater 2024-05-07 6:09 ` Duan, Zhenzhong 0 siblings, 1 reply; 10+ messages in thread From: Cédric Le Goater @ 2024-05-07 5:57 UTC (permalink / raw) To: Duan, Zhenzhong, qemu-devel@nongnu.org Cc: alex.williamson@redhat.com, eric.auger@redhat.com, Peng, Chao P, Tony Krowiak, Halil Pasic, Jason Herne, Thomas Huth, Eric Farman, Matthew Rosato, open list:vfio-ap On 5/7/24 04:09, Duan, Zhenzhong wrote: > > >> -----Original Message----- >> From: Cédric Le Goater <clg@redhat.com> >> Subject: Re: [PATCH 1/3] vfio: Make VFIOIOMMUClass::attach_device() and >> its wrapper return bool >> >> On 5/6/24 10:33, Zhenzhong Duan wrote: >>> Make VFIOIOMMUClass::attach_device() and its wrapper function >>> vfio_attach_device() return bool. >>> >>> This is to follow the coding standand to return bool if 'Error **' >>> is used to pass error. >>> >>> Suggested-by: Cédric Le Goater <clg@redhat.com> >>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>> --- >>> include/hw/vfio/vfio-common.h | 4 ++-- >>> include/hw/vfio/vfio-container-base.h | 4 ++-- >>> hw/vfio/ap.c | 6 ++---- >>> hw/vfio/ccw.c | 6 ++---- >>> hw/vfio/common.c | 4 ++-- >>> hw/vfio/container.c | 14 +++++++------- >>> hw/vfio/iommufd.c | 11 +++++------ >>> hw/vfio/pci.c | 8 +++----- >>> hw/vfio/platform.c | 7 +++---- >>> 9 files changed, 28 insertions(+), 36 deletions(-) >>> >>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio- >> common.h >>> index b9da6c08ef..a7b6fc8f46 100644 >>> --- a/include/hw/vfio/vfio-common.h >>> +++ b/include/hw/vfio/vfio-common.h >>> @@ -198,8 +198,8 @@ void vfio_region_exit(VFIORegion *region); >>> void vfio_region_finalize(VFIORegion *region); >>> void vfio_reset_handler(void *opaque); >>> struct vfio_device_info *vfio_get_device_info(int fd); >>> -int vfio_attach_device(char *name, VFIODevice *vbasedev, >>> - AddressSpace *as, Error **errp); >>> +bool vfio_attach_device(char *name, VFIODevice *vbasedev, >>> + AddressSpace *as, Error **errp); >>> void vfio_detach_device(VFIODevice *vbasedev); >>> >>> int vfio_kvm_device_add_fd(int fd, Error **errp); >>> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio- >> container-base.h >>> index 3582d5f97a..c839cfd9cb 100644 >>> --- a/include/hw/vfio/vfio-container-base.h >>> +++ b/include/hw/vfio/vfio-container-base.h >>> @@ -118,8 +118,8 @@ struct VFIOIOMMUClass { >>> int (*dma_unmap)(const VFIOContainerBase *bcontainer, >>> hwaddr iova, ram_addr_t size, >>> IOMMUTLBEntry *iotlb); >>> - int (*attach_device)(const char *name, VFIODevice *vbasedev, >>> - AddressSpace *as, Error **errp); >>> + bool (*attach_device)(const char *name, VFIODevice *vbasedev, >>> + AddressSpace *as, Error **errp); >>> void (*detach_device)(VFIODevice *vbasedev); >>> /* migration feature */ >>> int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer, >>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c >>> index 7c4caa5938..d50600b702 100644 >>> --- a/hw/vfio/ap.c >>> +++ b/hw/vfio/ap.c >>> @@ -156,7 +156,6 @@ static void >> vfio_ap_unregister_irq_notifier(VFIOAPDevice *vapdev, >>> static void vfio_ap_realize(DeviceState *dev, Error **errp) >>> { >>> ERRP_GUARD(); >>> - int ret; >>> Error *err = NULL; >>> VFIOAPDevice *vapdev = VFIO_AP_DEVICE(dev); >>> VFIODevice *vbasedev = &vapdev->vdev; >>> @@ -165,9 +164,8 @@ static void vfio_ap_realize(DeviceState *dev, Error >> **errp) >>> return; >>> } >>> >>> - ret = vfio_attach_device(vbasedev->name, vbasedev, >>> - &address_space_memory, errp); >>> - if (ret) { >>> + if (!vfio_attach_device(vbasedev->name, vbasedev, >>> + &address_space_memory, errp)) { >>> goto error; >>> } >>> >>> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c >>> index 90e4a53437..782bd4bed7 100644 >>> --- a/hw/vfio/ccw.c >>> +++ b/hw/vfio/ccw.c >>> @@ -580,7 +580,6 @@ static void vfio_ccw_realize(DeviceState *dev, >> Error **errp) >>> S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev); >>> VFIODevice *vbasedev = &vcdev->vdev; >>> Error *err = NULL; >>> - int ret; >>> >>> /* Call the class init function for subchannel. */ >>> if (cdc->realize) { >>> @@ -594,9 +593,8 @@ static void vfio_ccw_realize(DeviceState *dev, >> Error **errp) >>> return; >>> } >>> >>> - ret = vfio_attach_device(cdev->mdevid, vbasedev, >>> - &address_space_memory, errp); >>> - if (ret) { >>> + if (!vfio_attach_device(cdev->mdevid, vbasedev, >>> + &address_space_memory, errp)) { >>> goto out_attach_dev_err; >>> } >>> >>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >>> index 8f9cbdc026..890d30910e 100644 >>> --- a/hw/vfio/common.c >>> +++ b/hw/vfio/common.c >>> @@ -1492,8 +1492,8 @@ retry: >>> return info; >>> } >>> >>> -int vfio_attach_device(char *name, VFIODevice *vbasedev, >>> - AddressSpace *as, Error **errp) >>> +bool vfio_attach_device(char *name, VFIODevice *vbasedev, >>> + AddressSpace *as, Error **errp) >>> { >>> const VFIOIOMMUClass *ops = >>> >> VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_LEGACY)); >> >> >> I think vfio_attach_device() can be cleaned up a little further : >> >> ret = ops->attach_device(name, vbasedev, as, errp); >> if (ret < 0) { >> return ret; >> } > > Not understand this. > I have both ops->attach_device() and vfio_attach_device() return bool > in this patch. Do you mean drop the change for ops->attach_device()? > Or split the two changes? The return value of ops->attach_device() should be tested as a bool now and not as an int, as it is still currently done. See the fix here : https://gitlab.com/legoater/qemu/-/commit/abae88dc28e723745fe2ff506ea9f0adeb47afdb Please simply fold the changes in this patch. Thanks, C. >> >> >>> diff --git a/hw/vfio/container.c b/hw/vfio/container.c >>> index 77bdec276e..ea3b145913 100644 >>> --- a/hw/vfio/container.c >>> +++ b/hw/vfio/container.c >>> @@ -908,8 +908,8 @@ static int vfio_device_groupid(VFIODevice >> *vbasedev, Error **errp) >>> * @name and @vbasedev->name are likely to be different depending >>> * on the type of the device, hence the need for passing @name >>> */ >>> -static int vfio_legacy_attach_device(const char *name, VFIODevice >> *vbasedev, >>> - AddressSpace *as, Error **errp) >>> +static bool vfio_legacy_attach_device(const char *name, VFIODevice >> *vbasedev, >>> + AddressSpace *as, Error **errp) >>> { >>> int groupid = vfio_device_groupid(vbasedev, errp); >>> VFIODevice *vbasedev_iter; >>> @@ -918,27 +918,27 @@ static int vfio_legacy_attach_device(const char >> *name, VFIODevice *vbasedev, >>> int ret; >>> >>> if (groupid < 0) { >>> - return groupid; >>> + return false; >>> } >>> >>> trace_vfio_attach_device(vbasedev->name, groupid); >>> >>> group = vfio_get_group(groupid, as, errp); >>> if (!group) { >>> - return -ENOENT; >>> + return false; >>> } >>> >>> 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; >>> + return false; >>> } >>> } >>> ret = vfio_get_device(group, name, vbasedev, errp); >>> if (ret) { >> >> vfio_get_device() would be the next candidate for cleanup. > > Yes. > >> >>> vfio_put_group(group); >>> - return ret; >>> + return false; >>> } >>> >>> bcontainer = &group->container->bcontainer; >>> @@ -946,7 +946,7 @@ static int vfio_legacy_attach_device(const char >> *name, VFIODevice *vbasedev, >>> QLIST_INSERT_HEAD(&bcontainer->device_list, vbasedev, >> container_next); >>> QLIST_INSERT_HEAD(&vfio_device_list, vbasedev, global_next); >>> >>> - return ret; >>> + return true; >>> } >>> >>> static void vfio_legacy_detach_device(VFIODevice *vbasedev) >>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c >>> index 8827ffe636..9aa0dd6d8e 100644 >>> --- a/hw/vfio/iommufd.c >>> +++ b/hw/vfio/iommufd.c >>> @@ -301,8 +301,8 @@ error: >>> return ret; >>> } >>> >>> -static int iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, >>> - AddressSpace *as, Error **errp) >>> +static bool iommufd_cdev_attach(const char *name, VFIODevice >> *vbasedev, >>> + AddressSpace *as, Error **errp) >>> { >>> VFIOContainerBase *bcontainer; >>> VFIOIOMMUFDContainer *container; >>> @@ -317,7 +317,7 @@ static int iommufd_cdev_attach(const char *name, >> VFIODevice *vbasedev, >>> if (vbasedev->fd < 0) { >>> devfd = iommufd_cdev_getfd(vbasedev->sysfsdev, errp); >>> if (devfd < 0) { >>> - return devfd; >>> + return false; >>> } >>> vbasedev->fd = devfd; >>> } else { >>> @@ -394,7 +394,6 @@ static int iommufd_cdev_attach(const char *name, >> VFIODevice *vbasedev, >>> memory_listener_register(&bcontainer->listener, bcontainer->space- >>> as); >>> >>> if (bcontainer->error) { >>> - ret = -1; >>> error_propagate_prepend(errp, bcontainer->error, >>> "memory listener initialization failed: "); >>> goto err_listener_register; >>> @@ -433,7 +432,7 @@ found_container: >>> >>> trace_iommufd_cdev_device_info(vbasedev->name, devfd, vbasedev- >>> num_irqs, >>> vbasedev->num_regions, vbasedev->flags); >>> - return 0; >>> + return true; >>> >>> err_listener_register: >>> iommufd_cdev_ram_block_discard_disable(false); >>> @@ -446,7 +445,7 @@ err_alloc_ioas: >>> iommufd_cdev_unbind_and_disconnect(vbasedev); >>> err_connect_bind: >>> close(vbasedev->fd); >>> - return ret; >>> + return false; >>> } >>> >>> static void iommufd_cdev_detach(VFIODevice *vbasedev) >>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >>> index 64780d1b79..952e4b1a25 100644 >>> --- a/hw/vfio/pci.c >>> +++ b/hw/vfio/pci.c >>> @@ -2951,7 +2951,7 @@ static void vfio_realize(PCIDevice *pdev, Error >> **errp) >>> int i, ret; >>> bool is_mdev; >>> char uuid[UUID_STR_LEN]; >>> - char *name; >>> + g_autofree char *name = NULL; >>> >>> if (vbasedev->fd < 0 && !vbasedev->sysfsdev) { >>> if (!(~vdev->host.domain || ~vdev->host.bus || >>> @@ -3001,10 +3001,8 @@ static void vfio_realize(PCIDevice *pdev, Error >> **errp) >>> name = g_strdup(vbasedev->name); >>> } >>> >>> - ret = vfio_attach_device(name, vbasedev, >>> - pci_device_iommu_address_space(pdev), errp); >>> - g_free(name); >> >> This change would deserve another patch. > > Will do. > > Thanks > Zhenzhong ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH 1/3] vfio: Make VFIOIOMMUClass::attach_device() and its wrapper return bool 2024-05-07 5:57 ` Cédric Le Goater @ 2024-05-07 6:09 ` Duan, Zhenzhong 0 siblings, 0 replies; 10+ messages in thread From: Duan, Zhenzhong @ 2024-05-07 6:09 UTC (permalink / raw) To: Cédric Le Goater, qemu-devel@nongnu.org Cc: alex.williamson@redhat.com, eric.auger@redhat.com, Peng, Chao P, Tony Krowiak, Halil Pasic, Jason Herne, Thomas Huth, Eric Farman, Matthew Rosato, open list:vfio-ap >-----Original Message----- >From: Cédric Le Goater <clg@redhat.com> >Subject: Re: [PATCH 1/3] vfio: Make VFIOIOMMUClass::attach_device() and >its wrapper return bool > >On 5/7/24 04:09, Duan, Zhenzhong wrote: >> >> >>> -----Original Message----- >>> From: Cédric Le Goater <clg@redhat.com> >>> Subject: Re: [PATCH 1/3] vfio: Make VFIOIOMMUClass::attach_device() >and >>> its wrapper return bool >>> >>> On 5/6/24 10:33, Zhenzhong Duan wrote: >>>> Make VFIOIOMMUClass::attach_device() and its wrapper function >>>> vfio_attach_device() return bool. >>>> >>>> This is to follow the coding standand to return bool if 'Error **' >>>> is used to pass error. >>>> >>>> Suggested-by: Cédric Le Goater <clg@redhat.com> >>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>>> --- >>>> include/hw/vfio/vfio-common.h | 4 ++-- >>>> include/hw/vfio/vfio-container-base.h | 4 ++-- >>>> hw/vfio/ap.c | 6 ++---- >>>> hw/vfio/ccw.c | 6 ++---- >>>> hw/vfio/common.c | 4 ++-- >>>> hw/vfio/container.c | 14 +++++++------- >>>> hw/vfio/iommufd.c | 11 +++++------ >>>> hw/vfio/pci.c | 8 +++----- >>>> hw/vfio/platform.c | 7 +++---- >>>> 9 files changed, 28 insertions(+), 36 deletions(-) >>>> >>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio- >>> common.h >>>> index b9da6c08ef..a7b6fc8f46 100644 >>>> --- a/include/hw/vfio/vfio-common.h >>>> +++ b/include/hw/vfio/vfio-common.h >>>> @@ -198,8 +198,8 @@ void vfio_region_exit(VFIORegion *region); >>>> void vfio_region_finalize(VFIORegion *region); >>>> void vfio_reset_handler(void *opaque); >>>> struct vfio_device_info *vfio_get_device_info(int fd); >>>> -int vfio_attach_device(char *name, VFIODevice *vbasedev, >>>> - AddressSpace *as, Error **errp); >>>> +bool vfio_attach_device(char *name, VFIODevice *vbasedev, >>>> + AddressSpace *as, Error **errp); >>>> void vfio_detach_device(VFIODevice *vbasedev); >>>> >>>> int vfio_kvm_device_add_fd(int fd, Error **errp); >>>> diff --git a/include/hw/vfio/vfio-container-base.h >b/include/hw/vfio/vfio- >>> container-base.h >>>> index 3582d5f97a..c839cfd9cb 100644 >>>> --- a/include/hw/vfio/vfio-container-base.h >>>> +++ b/include/hw/vfio/vfio-container-base.h >>>> @@ -118,8 +118,8 @@ struct VFIOIOMMUClass { >>>> int (*dma_unmap)(const VFIOContainerBase *bcontainer, >>>> hwaddr iova, ram_addr_t size, >>>> IOMMUTLBEntry *iotlb); >>>> - int (*attach_device)(const char *name, VFIODevice *vbasedev, >>>> - AddressSpace *as, Error **errp); >>>> + bool (*attach_device)(const char *name, VFIODevice *vbasedev, >>>> + AddressSpace *as, Error **errp); >>>> void (*detach_device)(VFIODevice *vbasedev); >>>> /* migration feature */ >>>> int (*set_dirty_page_tracking)(const VFIOContainerBase >*bcontainer, >>>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c >>>> index 7c4caa5938..d50600b702 100644 >>>> --- a/hw/vfio/ap.c >>>> +++ b/hw/vfio/ap.c >>>> @@ -156,7 +156,6 @@ static void >>> vfio_ap_unregister_irq_notifier(VFIOAPDevice *vapdev, >>>> static void vfio_ap_realize(DeviceState *dev, Error **errp) >>>> { >>>> ERRP_GUARD(); >>>> - int ret; >>>> Error *err = NULL; >>>> VFIOAPDevice *vapdev = VFIO_AP_DEVICE(dev); >>>> VFIODevice *vbasedev = &vapdev->vdev; >>>> @@ -165,9 +164,8 @@ static void vfio_ap_realize(DeviceState *dev, >Error >>> **errp) >>>> return; >>>> } >>>> >>>> - ret = vfio_attach_device(vbasedev->name, vbasedev, >>>> - &address_space_memory, errp); >>>> - if (ret) { >>>> + if (!vfio_attach_device(vbasedev->name, vbasedev, >>>> + &address_space_memory, errp)) { >>>> goto error; >>>> } >>>> >>>> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c >>>> index 90e4a53437..782bd4bed7 100644 >>>> --- a/hw/vfio/ccw.c >>>> +++ b/hw/vfio/ccw.c >>>> @@ -580,7 +580,6 @@ static void vfio_ccw_realize(DeviceState *dev, >>> Error **errp) >>>> S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev); >>>> VFIODevice *vbasedev = &vcdev->vdev; >>>> Error *err = NULL; >>>> - int ret; >>>> >>>> /* Call the class init function for subchannel. */ >>>> if (cdc->realize) { >>>> @@ -594,9 +593,8 @@ static void vfio_ccw_realize(DeviceState *dev, >>> Error **errp) >>>> return; >>>> } >>>> >>>> - ret = vfio_attach_device(cdev->mdevid, vbasedev, >>>> - &address_space_memory, errp); >>>> - if (ret) { >>>> + if (!vfio_attach_device(cdev->mdevid, vbasedev, >>>> + &address_space_memory, errp)) { >>>> goto out_attach_dev_err; >>>> } >>>> >>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >>>> index 8f9cbdc026..890d30910e 100644 >>>> --- a/hw/vfio/common.c >>>> +++ b/hw/vfio/common.c >>>> @@ -1492,8 +1492,8 @@ retry: >>>> return info; >>>> } >>>> >>>> -int vfio_attach_device(char *name, VFIODevice *vbasedev, >>>> - AddressSpace *as, Error **errp) >>>> +bool vfio_attach_device(char *name, VFIODevice *vbasedev, >>>> + AddressSpace *as, Error **errp) >>>> { >>>> const VFIOIOMMUClass *ops = >>>> >>> >VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_LEGACY)); >>> >>> >>> I think vfio_attach_device() can be cleaned up a little further : >>> >>> ret = ops->attach_device(name, vbasedev, as, errp); >>> if (ret < 0) { >>> return ret; >>> } >> >> Not understand this. >> I have both ops->attach_device() and vfio_attach_device() return bool >> in this patch. Do you mean drop the change for ops->attach_device()? >> Or split the two changes? > >The return value of ops->attach_device() should be tested as a bool now >and not as an int, as it is still currently done. > >See the fix here : > https://gitlab.com/legoater/qemu/- >/commit/abae88dc28e723745fe2ff506ea9f0adeb47afdb > >Please simply fold the changes in this patch. Clear, I'll fold it in hiod v4 series. Thanks Zhenzhong ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/3] vfio: Make VFIOIOMMUClass::setup() return bool 2024-05-06 8:33 [PATCH 0/3] Cleanup VFIOIOMMUClass callback return with bool Zhenzhong Duan 2024-05-06 8:33 ` [PATCH 1/3] vfio: Make VFIOIOMMUClass::attach_device() and its wrapper return bool Zhenzhong Duan @ 2024-05-06 8:33 ` Zhenzhong Duan 2024-05-06 12:02 ` Cédric Le Goater 2024-05-06 8:33 ` [PATCH 3/3] vfio: Make VFIOIOMMUClass::add_window() and its wrapper " Zhenzhong Duan 2 siblings, 1 reply; 10+ messages in thread From: Zhenzhong Duan @ 2024-05-06 8:33 UTC (permalink / raw) To: qemu-devel Cc: alex.williamson, clg, eric.auger, chao.p.peng, Zhenzhong Duan, Nicholas Piggin, Daniel Henrique Barboza, David Gibson, Harsh Prateek Bora, open list:sPAPR (pseries) This is to follow the coding standand to return bool if 'Error **' is used to pass error. Suggested-by: Cédric Le Goater <clg@redhat.com> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> --- include/hw/vfio/vfio-container-base.h | 2 +- hw/vfio/container.c | 10 +++++----- hw/vfio/spapr.c | 12 +++++------- 3 files changed, 11 insertions(+), 13 deletions(-) diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h index c839cfd9cb..68539e3bed 100644 --- a/include/hw/vfio/vfio-container-base.h +++ b/include/hw/vfio/vfio-container-base.h @@ -111,7 +111,7 @@ struct VFIOIOMMUClass { InterfaceClass parent_class; /* basic feature */ - int (*setup)(VFIOContainerBase *bcontainer, Error **errp); + bool (*setup)(VFIOContainerBase *bcontainer, Error **errp); int (*dma_map)(const VFIOContainerBase *bcontainer, hwaddr iova, ram_addr_t size, void *vaddr, bool readonly); diff --git a/hw/vfio/container.c b/hw/vfio/container.c index ea3b145913..85a8a369dc 100644 --- a/hw/vfio/container.c +++ b/hw/vfio/container.c @@ -505,7 +505,7 @@ static void vfio_get_iommu_info_migration(VFIOContainer *container, } } -static int vfio_legacy_setup(VFIOContainerBase *bcontainer, Error **errp) +static bool vfio_legacy_setup(VFIOContainerBase *bcontainer, Error **errp) { VFIOContainer *container = container_of(bcontainer, VFIOContainer, bcontainer); @@ -515,7 +515,7 @@ static int vfio_legacy_setup(VFIOContainerBase *bcontainer, Error **errp) ret = vfio_get_iommu_info(container, &info); if (ret) { error_setg_errno(errp, -ret, "Failed to get VFIO IOMMU info"); - return ret; + return false; } if (info->flags & VFIO_IOMMU_INFO_PGSIZES) { @@ -531,7 +531,7 @@ static int vfio_legacy_setup(VFIOContainerBase *bcontainer, Error **errp) vfio_get_info_iova_range(info, bcontainer); vfio_get_iommu_info_migration(container, info); - return 0; + return true; } static int vfio_connect_container(VFIOGroup *group, AddressSpace *as, @@ -633,8 +633,8 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as, assert(bcontainer->ops->setup); - ret = bcontainer->ops->setup(bcontainer, errp); - if (ret) { + if (!bcontainer->ops->setup(bcontainer, errp)) { + ret = -EINVAL; goto enable_discards_exit; } diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c index 0d949bb728..148b257c9c 100644 --- a/hw/vfio/spapr.c +++ b/hw/vfio/spapr.c @@ -458,8 +458,8 @@ static void vfio_spapr_container_release(VFIOContainerBase *bcontainer) } } -static int vfio_spapr_container_setup(VFIOContainerBase *bcontainer, - Error **errp) +static bool vfio_spapr_container_setup(VFIOContainerBase *bcontainer, + Error **errp) { VFIOContainer *container = container_of(bcontainer, VFIOContainer, bcontainer); @@ -480,7 +480,7 @@ static int vfio_spapr_container_setup(VFIOContainerBase *bcontainer, ret = ioctl(fd, VFIO_IOMMU_ENABLE); if (ret) { error_setg_errno(errp, errno, "failed to enable container"); - return -errno; + return false; } } else { scontainer->prereg_listener = vfio_prereg_listener; @@ -488,7 +488,6 @@ static int vfio_spapr_container_setup(VFIOContainerBase *bcontainer, memory_listener_register(&scontainer->prereg_listener, &address_space_memory); if (bcontainer->error) { - ret = -1; error_propagate_prepend(errp, bcontainer->error, "RAM memory listener initialization failed: "); goto listener_unregister_exit; @@ -500,7 +499,6 @@ static int vfio_spapr_container_setup(VFIOContainerBase *bcontainer, if (ret) { error_setg_errno(errp, errno, "VFIO_IOMMU_SPAPR_TCE_GET_INFO failed"); - ret = -errno; goto listener_unregister_exit; } @@ -527,13 +525,13 @@ static int vfio_spapr_container_setup(VFIOContainerBase *bcontainer, 0x1000); } - return 0; + return true; listener_unregister_exit: if (v2) { memory_listener_unregister(&scontainer->prereg_listener); } - return ret; + return false; } static void vfio_iommu_spapr_class_init(ObjectClass *klass, void *data) -- 2.34.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] vfio: Make VFIOIOMMUClass::setup() return bool 2024-05-06 8:33 ` [PATCH 2/3] vfio: Make VFIOIOMMUClass::setup() " Zhenzhong Duan @ 2024-05-06 12:02 ` Cédric Le Goater 0 siblings, 0 replies; 10+ messages in thread From: Cédric Le Goater @ 2024-05-06 12:02 UTC (permalink / raw) To: Zhenzhong Duan, qemu-devel Cc: alex.williamson, eric.auger, chao.p.peng, Nicholas Piggin, Daniel Henrique Barboza, David Gibson, Harsh Prateek Bora, open list:sPAPR (pseries) On 5/6/24 10:33, Zhenzhong Duan wrote: > This is to follow the coding standand to return bool if 'Error **' > is used to pass error. > > Suggested-by: Cédric Le Goater <clg@redhat.com> > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> Reviewed-by: Cédric Le Goater <clg@redhat.com> Thanks, C. > --- > include/hw/vfio/vfio-container-base.h | 2 +- > hw/vfio/container.c | 10 +++++----- > hw/vfio/spapr.c | 12 +++++------- > 3 files changed, 11 insertions(+), 13 deletions(-) > > diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h > index c839cfd9cb..68539e3bed 100644 > --- a/include/hw/vfio/vfio-container-base.h > +++ b/include/hw/vfio/vfio-container-base.h > @@ -111,7 +111,7 @@ struct VFIOIOMMUClass { > InterfaceClass parent_class; > > /* basic feature */ > - int (*setup)(VFIOContainerBase *bcontainer, Error **errp); > + bool (*setup)(VFIOContainerBase *bcontainer, Error **errp); > int (*dma_map)(const VFIOContainerBase *bcontainer, > hwaddr iova, ram_addr_t size, > void *vaddr, bool readonly); > diff --git a/hw/vfio/container.c b/hw/vfio/container.c > index ea3b145913..85a8a369dc 100644 > --- a/hw/vfio/container.c > +++ b/hw/vfio/container.c > @@ -505,7 +505,7 @@ static void vfio_get_iommu_info_migration(VFIOContainer *container, > } > } > > -static int vfio_legacy_setup(VFIOContainerBase *bcontainer, Error **errp) > +static bool vfio_legacy_setup(VFIOContainerBase *bcontainer, Error **errp) > { > VFIOContainer *container = container_of(bcontainer, VFIOContainer, > bcontainer); > @@ -515,7 +515,7 @@ static int vfio_legacy_setup(VFIOContainerBase *bcontainer, Error **errp) > ret = vfio_get_iommu_info(container, &info); > if (ret) { > error_setg_errno(errp, -ret, "Failed to get VFIO IOMMU info"); > - return ret; > + return false; > } > > if (info->flags & VFIO_IOMMU_INFO_PGSIZES) { > @@ -531,7 +531,7 @@ static int vfio_legacy_setup(VFIOContainerBase *bcontainer, Error **errp) > vfio_get_info_iova_range(info, bcontainer); > > vfio_get_iommu_info_migration(container, info); > - return 0; > + return true; > } > > static int vfio_connect_container(VFIOGroup *group, AddressSpace *as, > @@ -633,8 +633,8 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as, > > assert(bcontainer->ops->setup); > > - ret = bcontainer->ops->setup(bcontainer, errp); > - if (ret) { > + if (!bcontainer->ops->setup(bcontainer, errp)) { > + ret = -EINVAL; > goto enable_discards_exit; > } > > diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c > index 0d949bb728..148b257c9c 100644 > --- a/hw/vfio/spapr.c > +++ b/hw/vfio/spapr.c > @@ -458,8 +458,8 @@ static void vfio_spapr_container_release(VFIOContainerBase *bcontainer) > } > } > > -static int vfio_spapr_container_setup(VFIOContainerBase *bcontainer, > - Error **errp) > +static bool vfio_spapr_container_setup(VFIOContainerBase *bcontainer, > + Error **errp) > { > VFIOContainer *container = container_of(bcontainer, VFIOContainer, > bcontainer); > @@ -480,7 +480,7 @@ static int vfio_spapr_container_setup(VFIOContainerBase *bcontainer, > ret = ioctl(fd, VFIO_IOMMU_ENABLE); > if (ret) { > error_setg_errno(errp, errno, "failed to enable container"); > - return -errno; > + return false; > } > } else { > scontainer->prereg_listener = vfio_prereg_listener; > @@ -488,7 +488,6 @@ static int vfio_spapr_container_setup(VFIOContainerBase *bcontainer, > memory_listener_register(&scontainer->prereg_listener, > &address_space_memory); > if (bcontainer->error) { > - ret = -1; > error_propagate_prepend(errp, bcontainer->error, > "RAM memory listener initialization failed: "); > goto listener_unregister_exit; > @@ -500,7 +499,6 @@ static int vfio_spapr_container_setup(VFIOContainerBase *bcontainer, > if (ret) { > error_setg_errno(errp, errno, > "VFIO_IOMMU_SPAPR_TCE_GET_INFO failed"); > - ret = -errno; > goto listener_unregister_exit; > } > > @@ -527,13 +525,13 @@ static int vfio_spapr_container_setup(VFIOContainerBase *bcontainer, > 0x1000); > } > > - return 0; > + return true; > > listener_unregister_exit: > if (v2) { > memory_listener_unregister(&scontainer->prereg_listener); > } > - return ret; > + return false; > } > > static void vfio_iommu_spapr_class_init(ObjectClass *klass, void *data) ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/3] vfio: Make VFIOIOMMUClass::add_window() and its wrapper return bool 2024-05-06 8:33 [PATCH 0/3] Cleanup VFIOIOMMUClass callback return with bool Zhenzhong Duan 2024-05-06 8:33 ` [PATCH 1/3] vfio: Make VFIOIOMMUClass::attach_device() and its wrapper return bool Zhenzhong Duan 2024-05-06 8:33 ` [PATCH 2/3] vfio: Make VFIOIOMMUClass::setup() " Zhenzhong Duan @ 2024-05-06 8:33 ` Zhenzhong Duan 2024-05-06 12:06 ` Cédric Le Goater 2 siblings, 1 reply; 10+ messages in thread From: Zhenzhong Duan @ 2024-05-06 8:33 UTC (permalink / raw) To: qemu-devel Cc: alex.williamson, clg, eric.auger, chao.p.peng, Zhenzhong Duan, Nicholas Piggin, Daniel Henrique Barboza, David Gibson, Harsh Prateek Bora, open list:sPAPR (pseries) Make VFIOIOMMUClass::add_window() and its wrapper function vfio_container_add_section_window() return bool. This is to follow the coding standand to return bool if 'Error **' is used to pass error. Suggested-by: Cédric Le Goater <clg@redhat.com> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> --- include/hw/vfio/vfio-container-base.h | 12 ++++++------ hw/vfio/common.c | 2 +- hw/vfio/container-base.c | 8 ++++---- hw/vfio/spapr.c | 16 ++++++++-------- 4 files changed, 19 insertions(+), 19 deletions(-) diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h index 68539e3bed..e96cda78c8 100644 --- a/include/hw/vfio/vfio-container-base.h +++ b/include/hw/vfio/vfio-container-base.h @@ -76,9 +76,9 @@ int vfio_container_dma_map(VFIOContainerBase *bcontainer, int vfio_container_dma_unmap(VFIOContainerBase *bcontainer, hwaddr iova, ram_addr_t size, IOMMUTLBEntry *iotlb); -int vfio_container_add_section_window(VFIOContainerBase *bcontainer, - MemoryRegionSection *section, - Error **errp); +bool vfio_container_add_section_window(VFIOContainerBase *bcontainer, + MemoryRegionSection *section, + Error **errp); void vfio_container_del_section_window(VFIOContainerBase *bcontainer, MemoryRegionSection *section); int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer, @@ -131,9 +131,9 @@ struct VFIOIOMMUClass { int (*pci_hot_reset)(VFIODevice *vbasedev, bool single); /* SPAPR specific */ - int (*add_window)(VFIOContainerBase *bcontainer, - MemoryRegionSection *section, - Error **errp); + bool (*add_window)(VFIOContainerBase *bcontainer, + MemoryRegionSection *section, + Error **errp); void (*del_window)(VFIOContainerBase *bcontainer, MemoryRegionSection *section); void (*release)(VFIOContainerBase *bcontainer); diff --git a/hw/vfio/common.c b/hw/vfio/common.c index 890d30910e..9f1f2e19f7 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -585,7 +585,7 @@ static void vfio_listener_region_add(MemoryListener *listener, return; } - if (vfio_container_add_section_window(bcontainer, section, &err)) { + if (!vfio_container_add_section_window(bcontainer, section, &err)) { goto fail; } diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c index 913ae49077..98d71b3144 100644 --- a/hw/vfio/container-base.c +++ b/hw/vfio/container-base.c @@ -31,12 +31,12 @@ int vfio_container_dma_unmap(VFIOContainerBase *bcontainer, return bcontainer->ops->dma_unmap(bcontainer, iova, size, iotlb); } -int vfio_container_add_section_window(VFIOContainerBase *bcontainer, - MemoryRegionSection *section, - Error **errp) +bool vfio_container_add_section_window(VFIOContainerBase *bcontainer, + MemoryRegionSection *section, + Error **errp) { if (!bcontainer->ops->add_window) { - return 0; + return true; } return bcontainer->ops->add_window(bcontainer, section, errp); diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c index 148b257c9c..47b040f1bc 100644 --- a/hw/vfio/spapr.c +++ b/hw/vfio/spapr.c @@ -323,7 +323,7 @@ static int vfio_spapr_create_window(VFIOContainer *container, return 0; } -static int +static bool vfio_spapr_container_add_section_window(VFIOContainerBase *bcontainer, MemoryRegionSection *section, Error **errp) @@ -351,13 +351,13 @@ vfio_spapr_container_add_section_window(VFIOContainerBase *bcontainer, error_setg(errp, "Container %p can't map guest IOVA region" " 0x%"HWADDR_PRIx"..0x%"HWADDR_PRIx, container, iova, end); - return -EINVAL; + return false; } - return 0; + return true; } if (container->iommu_type != VFIO_SPAPR_TCE_v2_IOMMU) { - return 0; + return true; } /* For now intersections are not allowed, we may relax this later */ @@ -373,14 +373,14 @@ vfio_spapr_container_add_section_window(VFIOContainerBase *bcontainer, section->offset_within_address_space + int128_get64(section->size) - 1, hostwin->min_iova, hostwin->max_iova); - return -EINVAL; + return false; } } ret = vfio_spapr_create_window(container, section, &pgsize); if (ret) { error_setg_errno(errp, -ret, "Failed to create SPAPR window"); - return ret; + return false; } vfio_host_win_add(scontainer, section->offset_within_address_space, @@ -406,14 +406,14 @@ vfio_spapr_container_add_section_window(VFIOContainerBase *bcontainer, "vfio: failed GROUP_SET_SPAPR_TCE for " "KVM VFIO device %d and group fd %d", param.tablefd, param.groupfd); - return -errno; + return false; } trace_vfio_spapr_group_attach(param.groupfd, param.tablefd); } } } #endif - return 0; + return true; } static void -- 2.34.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] vfio: Make VFIOIOMMUClass::add_window() and its wrapper return bool 2024-05-06 8:33 ` [PATCH 3/3] vfio: Make VFIOIOMMUClass::add_window() and its wrapper " Zhenzhong Duan @ 2024-05-06 12:06 ` Cédric Le Goater 0 siblings, 0 replies; 10+ messages in thread From: Cédric Le Goater @ 2024-05-06 12:06 UTC (permalink / raw) To: Zhenzhong Duan, qemu-devel Cc: alex.williamson, eric.auger, chao.p.peng, Nicholas Piggin, Daniel Henrique Barboza, David Gibson, Harsh Prateek Bora, open list:sPAPR (pseries) On 5/6/24 10:33, Zhenzhong Duan wrote: > Make VFIOIOMMUClass::add_window() and its wrapper function > vfio_container_add_section_window() return bool. > > This is to follow the coding standand to return bool if 'Error **' > is used to pass error. > > Suggested-by: Cédric Le Goater <clg@redhat.com> > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> Reviewed-by: Cédric Le Goater <clg@redhat.com> One comment below for the spapr maintainers, > --- > include/hw/vfio/vfio-container-base.h | 12 ++++++------ > hw/vfio/common.c | 2 +- > hw/vfio/container-base.c | 8 ++++---- > hw/vfio/spapr.c | 16 ++++++++-------- > 4 files changed, 19 insertions(+), 19 deletions(-) > > diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h > index 68539e3bed..e96cda78c8 100644 > --- a/include/hw/vfio/vfio-container-base.h > +++ b/include/hw/vfio/vfio-container-base.h > @@ -76,9 +76,9 @@ int vfio_container_dma_map(VFIOContainerBase *bcontainer, > int vfio_container_dma_unmap(VFIOContainerBase *bcontainer, > hwaddr iova, ram_addr_t size, > IOMMUTLBEntry *iotlb); > -int vfio_container_add_section_window(VFIOContainerBase *bcontainer, > - MemoryRegionSection *section, > - Error **errp); > +bool vfio_container_add_section_window(VFIOContainerBase *bcontainer, > + MemoryRegionSection *section, > + Error **errp); > void vfio_container_del_section_window(VFIOContainerBase *bcontainer, > MemoryRegionSection *section); > int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer, > @@ -131,9 +131,9 @@ struct VFIOIOMMUClass { > int (*pci_hot_reset)(VFIODevice *vbasedev, bool single); > > /* SPAPR specific */ > - int (*add_window)(VFIOContainerBase *bcontainer, > - MemoryRegionSection *section, > - Error **errp); > + bool (*add_window)(VFIOContainerBase *bcontainer, > + MemoryRegionSection *section, > + Error **errp); > void (*del_window)(VFIOContainerBase *bcontainer, > MemoryRegionSection *section); > void (*release)(VFIOContainerBase *bcontainer); > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index 890d30910e..9f1f2e19f7 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -585,7 +585,7 @@ static void vfio_listener_region_add(MemoryListener *listener, > return; > } > > - if (vfio_container_add_section_window(bcontainer, section, &err)) { > + if (!vfio_container_add_section_window(bcontainer, section, &err)) { > goto fail; > } > > diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c > index 913ae49077..98d71b3144 100644 > --- a/hw/vfio/container-base.c > +++ b/hw/vfio/container-base.c > @@ -31,12 +31,12 @@ int vfio_container_dma_unmap(VFIOContainerBase *bcontainer, > return bcontainer->ops->dma_unmap(bcontainer, iova, size, iotlb); > } > > -int vfio_container_add_section_window(VFIOContainerBase *bcontainer, > - MemoryRegionSection *section, > - Error **errp) > +bool vfio_container_add_section_window(VFIOContainerBase *bcontainer, > + MemoryRegionSection *section, > + Error **errp) > { > if (!bcontainer->ops->add_window) { > - return 0; > + return true; > } > > return bcontainer->ops->add_window(bcontainer, section, errp); > diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c > index 148b257c9c..47b040f1bc 100644 > --- a/hw/vfio/spapr.c > +++ b/hw/vfio/spapr.c > @@ -323,7 +323,7 @@ static int vfio_spapr_create_window(VFIOContainer *container, > return 0; > } > > -static int > +static bool > vfio_spapr_container_add_section_window(VFIOContainerBase *bcontainer, > MemoryRegionSection *section, > Error **errp) > @@ -351,13 +351,13 @@ vfio_spapr_container_add_section_window(VFIOContainerBase *bcontainer, > error_setg(errp, "Container %p can't map guest IOVA region" > " 0x%"HWADDR_PRIx"..0x%"HWADDR_PRIx, container, > iova, end); > - return -EINVAL; > + return false; > } > - return 0; > + return true; > } > > if (container->iommu_type != VFIO_SPAPR_TCE_v2_IOMMU) { > - return 0; > + return true; > } > > /* For now intersections are not allowed, we may relax this later */ > @@ -373,14 +373,14 @@ vfio_spapr_container_add_section_window(VFIOContainerBase *bcontainer, > section->offset_within_address_space + > int128_get64(section->size) - 1, > hostwin->min_iova, hostwin->max_iova); > - return -EINVAL; > + return false; > } > } > > ret = vfio_spapr_create_window(container, section, &pgsize); vfio_spapr_create_window() contains several calls to error_report() which would be good to replace with error_setg(). Thanks, C. > if (ret) { > error_setg_errno(errp, -ret, "Failed to create SPAPR window"); > - return ret; > + return false; > } > > vfio_host_win_add(scontainer, section->offset_within_address_space, > @@ -406,14 +406,14 @@ vfio_spapr_container_add_section_window(VFIOContainerBase *bcontainer, > "vfio: failed GROUP_SET_SPAPR_TCE for " > "KVM VFIO device %d and group fd %d", > param.tablefd, param.groupfd); > - return -errno; > + return false; > } > trace_vfio_spapr_group_attach(param.groupfd, param.tablefd); > } > } > } > #endif > - return 0; > + return true; > } > > static void ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-05-07 6:10 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-05-06 8:33 [PATCH 0/3] Cleanup VFIOIOMMUClass callback return with bool Zhenzhong Duan 2024-05-06 8:33 ` [PATCH 1/3] vfio: Make VFIOIOMMUClass::attach_device() and its wrapper return bool Zhenzhong Duan 2024-05-06 11:59 ` Cédric Le Goater 2024-05-07 2:09 ` Duan, Zhenzhong 2024-05-07 5:57 ` Cédric Le Goater 2024-05-07 6:09 ` Duan, Zhenzhong 2024-05-06 8:33 ` [PATCH 2/3] vfio: Make VFIOIOMMUClass::setup() " Zhenzhong Duan 2024-05-06 12:02 ` Cédric Le Goater 2024-05-06 8:33 ` [PATCH 3/3] vfio: Make VFIOIOMMUClass::add_window() and its wrapper " Zhenzhong Duan 2024-05-06 12:06 ` Cédric Le Goater
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).