* [PATCH v2 01/11] vfio/pci: Use g_autofree in vfio_realize
2024-05-07 6:42 [PATCH v2 00/11] VFIO: misc cleanups Zhenzhong Duan
@ 2024-05-07 6:42 ` Zhenzhong Duan
2024-05-14 15:45 ` Cédric Le Goater
2024-05-07 6:42 ` [PATCH v2 02/11] vfio/pci: Use g_autofree in iommufd_cdev_get_info_iova_range() Zhenzhong Duan
` (11 subsequent siblings)
12 siblings, 1 reply; 25+ messages in thread
From: Zhenzhong Duan @ 2024-05-07 6:42 UTC (permalink / raw)
To: qemu-devel; +Cc: alex.williamson, clg, eric.auger, chao.p.peng, Zhenzhong Duan
Local pointer name is allocated before vfio_attach_device() call
and freed after the call.
Same for tmp when calling realpath().
Use 'g_autofree' to avoid the g_free() calls.
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
hw/vfio/pci.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 64780d1b79..576b21e2bb 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2946,12 +2946,13 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
ERRP_GUARD();
VFIOPCIDevice *vdev = VFIO_PCI(pdev);
VFIODevice *vbasedev = &vdev->vbasedev;
- char *tmp, *subsys;
+ char *subsys;
Error *err = NULL;
int i, ret;
bool is_mdev;
char uuid[UUID_STR_LEN];
- char *name;
+ g_autofree char *name = NULL;
+ g_autofree char *tmp = NULL;
if (vbasedev->fd < 0 && !vbasedev->sysfsdev) {
if (!(~vdev->host.domain || ~vdev->host.bus ||
@@ -2982,7 +2983,6 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
*/
tmp = g_strdup_printf("%s/subsystem", vbasedev->sysfsdev);
subsys = realpath(tmp, NULL);
- g_free(tmp);
is_mdev = subsys && (strcmp(subsys, "/sys/bus/mdev") == 0);
free(subsys);
@@ -3003,7 +3003,6 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
ret = vfio_attach_device(name, vbasedev,
pci_device_iommu_address_space(pdev), errp);
- g_free(name);
if (ret) {
goto error;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v2 01/11] vfio/pci: Use g_autofree in vfio_realize
2024-05-07 6:42 ` [PATCH v2 01/11] vfio/pci: Use g_autofree in vfio_realize Zhenzhong Duan
@ 2024-05-14 15:45 ` Cédric Le Goater
0 siblings, 0 replies; 25+ messages in thread
From: Cédric Le Goater @ 2024-05-14 15:45 UTC (permalink / raw)
To: Zhenzhong Duan, qemu-devel; +Cc: alex.williamson, eric.auger, chao.p.peng
On 5/7/24 08:42, Zhenzhong Duan wrote:
> Local pointer name is allocated before vfio_attach_device() call
> and freed after the call.
>
> Same for tmp when calling realpath().
>
> Use 'g_autofree' to avoid the g_free() calls.
>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
Thanks,
C.
> ---
> hw/vfio/pci.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 64780d1b79..576b21e2bb 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2946,12 +2946,13 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
> ERRP_GUARD();
> VFIOPCIDevice *vdev = VFIO_PCI(pdev);
> VFIODevice *vbasedev = &vdev->vbasedev;
> - char *tmp, *subsys;
> + char *subsys;
> Error *err = NULL;
> int i, ret;
> bool is_mdev;
> char uuid[UUID_STR_LEN];
> - char *name;
> + g_autofree char *name = NULL;
> + g_autofree char *tmp = NULL;
>
> if (vbasedev->fd < 0 && !vbasedev->sysfsdev) {
> if (!(~vdev->host.domain || ~vdev->host.bus ||
> @@ -2982,7 +2983,6 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
> */
> tmp = g_strdup_printf("%s/subsystem", vbasedev->sysfsdev);
> subsys = realpath(tmp, NULL);
> - g_free(tmp);
> is_mdev = subsys && (strcmp(subsys, "/sys/bus/mdev") == 0);
> free(subsys);
>
> @@ -3003,7 +3003,6 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>
> ret = vfio_attach_device(name, vbasedev,
> pci_device_iommu_address_space(pdev), errp);
> - g_free(name);
> if (ret) {
> goto error;
> }
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v2 02/11] vfio/pci: Use g_autofree in iommufd_cdev_get_info_iova_range()
2024-05-07 6:42 [PATCH v2 00/11] VFIO: misc cleanups Zhenzhong Duan
2024-05-07 6:42 ` [PATCH v2 01/11] vfio/pci: Use g_autofree in vfio_realize Zhenzhong Duan
@ 2024-05-07 6:42 ` Zhenzhong Duan
2024-05-14 15:45 ` Cédric Le Goater
2024-05-07 6:42 ` [PATCH v2 03/11] vfio: Make VFIOIOMMUClass::attach_device() and its wrapper return bool Zhenzhong Duan
` (10 subsequent siblings)
12 siblings, 1 reply; 25+ messages in thread
From: Zhenzhong Duan @ 2024-05-07 6:42 UTC (permalink / raw)
To: qemu-devel; +Cc: alex.williamson, clg, eric.auger, chao.p.peng, Zhenzhong Duan
Local pointer info is freed before return from
iommufd_cdev_get_info_iova_range().
Use 'g_autofree' to avoid the g_free() calls.
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
hw/vfio/iommufd.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index 8827ffe636..c644127972 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -258,7 +258,7 @@ static int iommufd_cdev_get_info_iova_range(VFIOIOMMUFDContainer *container,
uint32_t ioas_id, Error **errp)
{
VFIOContainerBase *bcontainer = &container->bcontainer;
- struct iommu_ioas_iova_ranges *info;
+ g_autofree struct iommu_ioas_iova_ranges *info = NULL;
struct iommu_iova_range *iova_ranges;
int ret, sz, fd = container->be->fd;
@@ -291,12 +291,10 @@ static int iommufd_cdev_get_info_iova_range(VFIOIOMMUFDContainer *container,
}
bcontainer->pgsizes = info->out_iova_alignment;
- g_free(info);
return 0;
error:
ret = -errno;
- g_free(info);
error_setg_errno(errp, errno, "Cannot get IOVA ranges");
return ret;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v2 02/11] vfio/pci: Use g_autofree in iommufd_cdev_get_info_iova_range()
2024-05-07 6:42 ` [PATCH v2 02/11] vfio/pci: Use g_autofree in iommufd_cdev_get_info_iova_range() Zhenzhong Duan
@ 2024-05-14 15:45 ` Cédric Le Goater
0 siblings, 0 replies; 25+ messages in thread
From: Cédric Le Goater @ 2024-05-14 15:45 UTC (permalink / raw)
To: Zhenzhong Duan, qemu-devel; +Cc: alex.williamson, eric.auger, chao.p.peng
On 5/7/24 08:42, Zhenzhong Duan wrote:
> Local pointer info is freed before return from
> iommufd_cdev_get_info_iova_range().
>
> Use 'g_autofree' to avoid the g_free() calls.
>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
Thanks,
C.
> ---
> hw/vfio/iommufd.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> index 8827ffe636..c644127972 100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -258,7 +258,7 @@ static int iommufd_cdev_get_info_iova_range(VFIOIOMMUFDContainer *container,
> uint32_t ioas_id, Error **errp)
> {
> VFIOContainerBase *bcontainer = &container->bcontainer;
> - struct iommu_ioas_iova_ranges *info;
> + g_autofree struct iommu_ioas_iova_ranges *info = NULL;
> struct iommu_iova_range *iova_ranges;
> int ret, sz, fd = container->be->fd;
>
> @@ -291,12 +291,10 @@ static int iommufd_cdev_get_info_iova_range(VFIOIOMMUFDContainer *container,
> }
> bcontainer->pgsizes = info->out_iova_alignment;
>
> - g_free(info);
> return 0;
>
> error:
> ret = -errno;
> - g_free(info);
> error_setg_errno(errp, errno, "Cannot get IOVA ranges");
> return ret;
> }
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v2 03/11] vfio: Make VFIOIOMMUClass::attach_device() and its wrapper return bool
2024-05-07 6:42 [PATCH v2 00/11] VFIO: misc cleanups Zhenzhong Duan
2024-05-07 6:42 ` [PATCH v2 01/11] vfio/pci: Use g_autofree in vfio_realize Zhenzhong Duan
2024-05-07 6:42 ` [PATCH v2 02/11] vfio/pci: Use g_autofree in iommufd_cdev_get_info_iova_range() Zhenzhong Duan
@ 2024-05-07 6:42 ` Zhenzhong Duan
2024-05-07 7:27 ` Cédric Le Goater
2024-05-07 6:42 ` [PATCH v2 04/11] vfio: Make VFIOIOMMUClass::setup() " Zhenzhong Duan
` (9 subsequent siblings)
12 siblings, 1 reply; 25+ messages in thread
From: Zhenzhong Duan @ 2024-05-07 6:42 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 | 5 ++---
hw/vfio/platform.c | 7 +++----
9 files changed, 27 insertions(+), 34 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 c644127972..4c6992fca1 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -299,8 +299,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;
@@ -315,7 +315,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 {
@@ -392,7 +392,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;
@@ -431,7 +430,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);
@@ -444,7 +443,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 576b21e2bb..d3beb15514 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3001,9 +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);
- 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] 25+ messages in thread* Re: [PATCH v2 03/11] vfio: Make VFIOIOMMUClass::attach_device() and its wrapper return bool
2024-05-07 6:42 ` [PATCH v2 03/11] vfio: Make VFIOIOMMUClass::attach_device() and its wrapper return bool Zhenzhong Duan
@ 2024-05-07 7:27 ` Cédric Le Goater
2024-05-07 7:34 ` Duan, Zhenzhong
0 siblings, 1 reply; 25+ messages in thread
From: Cédric Le Goater @ 2024-05-07 7:27 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/7/24 08:42, 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 | 5 ++---
> hw/vfio/platform.c | 7 +++----
> 9 files changed, 27 insertions(+), 34 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));
This is still broken. No need to resend. I will update the code.
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_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 c644127972..4c6992fca1 100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -299,8 +299,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;
> @@ -315,7 +315,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 {
> @@ -392,7 +392,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;
> @@ -431,7 +430,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);
> @@ -444,7 +443,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 576b21e2bb..d3beb15514 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3001,9 +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);
> - 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] 25+ messages in thread* RE: [PATCH v2 03/11] vfio: Make VFIOIOMMUClass::attach_device() and its wrapper return bool
2024-05-07 7:27 ` Cédric Le Goater
@ 2024-05-07 7:34 ` Duan, Zhenzhong
2024-05-15 16:28 ` Cédric Le Goater
0 siblings, 1 reply; 25+ messages in thread
From: Duan, Zhenzhong @ 2024-05-07 7:34 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 v2 03/11] vfio: Make VFIOIOMMUClass::attach_device()
>and its wrapper return bool
>
>On 5/7/24 08:42, 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 | 5 ++---
>> hw/vfio/platform.c | 7 +++----
>> 9 files changed, 27 insertions(+), 34 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));
>
>This is still broken. No need to resend. I will update the code.
I put this series before preq v4, so you don't see that change.
See https://github.com/yiliu1765/qemu/commits/zhenzhong/iommufd_nesting_rfcv2_preq_v4_has_vfio_cleanup/
That change is in https://github.com/yiliu1765/qemu/commit/0bec499456a9aa5a079ed2335ce1581d79e2850d
Thanks
Zhenzhong
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v2 03/11] vfio: Make VFIOIOMMUClass::attach_device() and its wrapper return bool
2024-05-07 7:34 ` Duan, Zhenzhong
@ 2024-05-15 16:28 ` Cédric Le Goater
0 siblings, 0 replies; 25+ messages in thread
From: Cédric Le Goater @ 2024-05-15 16:28 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 09:34, Duan, Zhenzhong wrote:
>
>
>> -----Original Message-----
>> From: Cédric Le Goater <clg@redhat.com>
>> Subject: Re: [PATCH v2 03/11] vfio: Make VFIOIOMMUClass::attach_device()
>> and its wrapper return bool
>>
>> On 5/7/24 08:42, 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 | 5 ++---
>>> hw/vfio/platform.c | 7 +++----
>>> 9 files changed, 27 insertions(+), 34 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));
>>
>> This is still broken. No need to resend. I will update the code.
>
> I put this series before preq v4, so you don't see that change.
> See https://github.com/yiliu1765/qemu/commits/zhenzhong/iommufd_nesting_rfcv2_preq_v4_has_vfio_cleanup/
>
> That change is in https://github.com/yiliu1765/qemu/commit/0bec499456a9aa5a079ed2335ce1581d79e2850d
OK looks good.
Reviewed-by: Cédric Le Goater <clg@redhat.com>
Thanks,
C.
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v2 04/11] vfio: Make VFIOIOMMUClass::setup() return bool
2024-05-07 6:42 [PATCH v2 00/11] VFIO: misc cleanups Zhenzhong Duan
` (2 preceding siblings ...)
2024-05-07 6:42 ` [PATCH v2 03/11] vfio: Make VFIOIOMMUClass::attach_device() and its wrapper return bool Zhenzhong Duan
@ 2024-05-07 6:42 ` Zhenzhong Duan
2024-05-07 6:42 ` [PATCH v2 05/11] vfio: Make VFIOIOMMUClass::add_window() and its wrapper " Zhenzhong Duan
` (8 subsequent siblings)
12 siblings, 0 replies; 25+ messages in thread
From: Zhenzhong Duan @ 2024-05-07 6:42 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>
Reviewed-by: Cédric Le Goater <clg@redhat.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] 25+ messages in thread* [PATCH v2 05/11] vfio: Make VFIOIOMMUClass::add_window() and its wrapper return bool
2024-05-07 6:42 [PATCH v2 00/11] VFIO: misc cleanups Zhenzhong Duan
` (3 preceding siblings ...)
2024-05-07 6:42 ` [PATCH v2 04/11] vfio: Make VFIOIOMMUClass::setup() " Zhenzhong Duan
@ 2024-05-07 6:42 ` Zhenzhong Duan
2024-05-07 6:42 ` [PATCH v2 06/11] vfio/container: Make vfio_connect_container() " Zhenzhong Duan
` (7 subsequent siblings)
12 siblings, 0 replies; 25+ messages in thread
From: Zhenzhong Duan @ 2024-05-07 6:42 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>
Reviewed-by: Cédric Le Goater <clg@redhat.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] 25+ messages in thread* [PATCH v2 06/11] vfio/container: Make vfio_connect_container() return bool
2024-05-07 6:42 [PATCH v2 00/11] VFIO: misc cleanups Zhenzhong Duan
` (4 preceding siblings ...)
2024-05-07 6:42 ` [PATCH v2 05/11] vfio: Make VFIOIOMMUClass::add_window() and its wrapper " Zhenzhong Duan
@ 2024-05-07 6:42 ` Zhenzhong Duan
2024-05-14 16:00 ` Cédric Le Goater
2024-05-07 6:42 ` [PATCH v2 07/11] vfio/container: Make vfio_set_iommu() " Zhenzhong Duan
` (6 subsequent siblings)
12 siblings, 1 reply; 25+ messages in thread
From: Zhenzhong Duan @ 2024-05-07 6:42 UTC (permalink / raw)
To: qemu-devel; +Cc: alex.williamson, clg, eric.auger, chao.p.peng, Zhenzhong Duan
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>
---
hw/vfio/container.c | 18 +++++++-----------
1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index 85a8a369dc..0a7edfcc43 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -534,8 +534,8 @@ static bool vfio_legacy_setup(VFIOContainerBase *bcontainer, Error **errp)
return true;
}
-static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
- Error **errp)
+static bool vfio_connect_container(VFIOGroup *group, AddressSpace *as,
+ Error **errp)
{
VFIOContainer *container;
VFIOContainerBase *bcontainer;
@@ -587,19 +587,18 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
error_report("vfio: error disconnecting group %d from"
" container", group->groupid);
}
- return ret;
+ return false;
}
group->container = container;
QLIST_INSERT_HEAD(&container->group_list, group, container_next);
vfio_kvm_device_add_group(group);
- return 0;
+ return true;
}
}
fd = qemu_open_old("/dev/vfio/vfio", O_RDWR);
if (fd < 0) {
error_setg_errno(errp, errno, "failed to open /dev/vfio/vfio");
- ret = -errno;
goto put_space_exit;
}
@@ -607,7 +606,6 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
if (ret != VFIO_API_VERSION) {
error_setg(errp, "supported vfio version: %d, "
"reported version: %d", VFIO_API_VERSION, ret);
- ret = -EINVAL;
goto close_fd_exit;
}
@@ -634,7 +632,6 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
assert(bcontainer->ops->setup);
if (!bcontainer->ops->setup(bcontainer, errp)) {
- ret = -EINVAL;
goto enable_discards_exit;
}
@@ -650,7 +647,6 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
memory_listener_register(&bcontainer->listener, bcontainer->space->as);
if (bcontainer->error) {
- ret = -1;
error_propagate_prepend(errp, bcontainer->error,
"memory listener initialization failed: ");
goto listener_release_exit;
@@ -658,7 +654,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
bcontainer->initialized = true;
- return 0;
+ return true;
listener_release_exit:
QLIST_REMOVE(group, container_next);
QLIST_REMOVE(bcontainer, next);
@@ -683,7 +679,7 @@ close_fd_exit:
put_space_exit:
vfio_put_address_space(space);
- return ret;
+ return false;
}
static void vfio_disconnect_container(VFIOGroup *group)
@@ -770,7 +766,7 @@ static VFIOGroup *vfio_get_group(int groupid, AddressSpace *as, Error **errp)
group->groupid = groupid;
QLIST_INIT(&group->device_list);
- if (vfio_connect_container(group, as, errp)) {
+ if (!vfio_connect_container(group, as, errp)) {
error_prepend(errp, "failed to setup container for group %d: ",
groupid);
goto close_fd_exit;
--
2.34.1
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v2 06/11] vfio/container: Make vfio_connect_container() return bool
2024-05-07 6:42 ` [PATCH v2 06/11] vfio/container: Make vfio_connect_container() " Zhenzhong Duan
@ 2024-05-14 16:00 ` Cédric Le Goater
0 siblings, 0 replies; 25+ messages in thread
From: Cédric Le Goater @ 2024-05-14 16:00 UTC (permalink / raw)
To: Zhenzhong Duan, qemu-devel; +Cc: alex.williamson, eric.auger, chao.p.peng
On 5/7/24 08:42, 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.
> ---
> hw/vfio/container.c | 18 +++++++-----------
> 1 file changed, 7 insertions(+), 11 deletions(-)
>
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index 85a8a369dc..0a7edfcc43 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -534,8 +534,8 @@ static bool vfio_legacy_setup(VFIOContainerBase *bcontainer, Error **errp)
> return true;
> }
>
> -static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
> - Error **errp)
> +static bool vfio_connect_container(VFIOGroup *group, AddressSpace *as,
> + Error **errp)
> {
> VFIOContainer *container;
> VFIOContainerBase *bcontainer;
> @@ -587,19 +587,18 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
> error_report("vfio: error disconnecting group %d from"
> " container", group->groupid);
> }
> - return ret;
> + return false;
> }
> group->container = container;
> QLIST_INSERT_HEAD(&container->group_list, group, container_next);
> vfio_kvm_device_add_group(group);
> - return 0;
> + return true;
> }
> }
>
> fd = qemu_open_old("/dev/vfio/vfio", O_RDWR);
> if (fd < 0) {
> error_setg_errno(errp, errno, "failed to open /dev/vfio/vfio");
> - ret = -errno;
> goto put_space_exit;
> }
>
> @@ -607,7 +606,6 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
> if (ret != VFIO_API_VERSION) {
> error_setg(errp, "supported vfio version: %d, "
> "reported version: %d", VFIO_API_VERSION, ret);
> - ret = -EINVAL;
> goto close_fd_exit;
> }
>
> @@ -634,7 +632,6 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
> assert(bcontainer->ops->setup);
>
> if (!bcontainer->ops->setup(bcontainer, errp)) {
> - ret = -EINVAL;
> goto enable_discards_exit;
> }
>
> @@ -650,7 +647,6 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
> memory_listener_register(&bcontainer->listener, bcontainer->space->as);
>
> if (bcontainer->error) {
> - ret = -1;
> error_propagate_prepend(errp, bcontainer->error,
> "memory listener initialization failed: ");
> goto listener_release_exit;
> @@ -658,7 +654,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>
> bcontainer->initialized = true;
>
> - return 0;
> + return true;
> listener_release_exit:
> QLIST_REMOVE(group, container_next);
> QLIST_REMOVE(bcontainer, next);
> @@ -683,7 +679,7 @@ close_fd_exit:
> put_space_exit:
> vfio_put_address_space(space);
>
> - return ret;
> + return false;
> }
>
> static void vfio_disconnect_container(VFIOGroup *group)
> @@ -770,7 +766,7 @@ static VFIOGroup *vfio_get_group(int groupid, AddressSpace *as, Error **errp)
> group->groupid = groupid;
> QLIST_INIT(&group->device_list);
>
> - if (vfio_connect_container(group, as, errp)) {
> + if (!vfio_connect_container(group, as, errp)) {
> error_prepend(errp, "failed to setup container for group %d: ",
> groupid);
> goto close_fd_exit;
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v2 07/11] vfio/container: Make vfio_set_iommu() return bool
2024-05-07 6:42 [PATCH v2 00/11] VFIO: misc cleanups Zhenzhong Duan
` (5 preceding siblings ...)
2024-05-07 6:42 ` [PATCH v2 06/11] vfio/container: Make vfio_connect_container() " Zhenzhong Duan
@ 2024-05-07 6:42 ` Zhenzhong Duan
2024-05-14 16:00 ` Cédric Le Goater
2024-05-07 6:42 ` [PATCH v2 08/11] vfio/container: Make vfio_get_device() " Zhenzhong Duan
` (5 subsequent siblings)
12 siblings, 1 reply; 25+ messages in thread
From: Zhenzhong Duan @ 2024-05-07 6:42 UTC (permalink / raw)
To: qemu-devel; +Cc: alex.williamson, clg, eric.auger, chao.p.peng, Zhenzhong Duan
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>
---
hw/vfio/container.c | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index 0a7edfcc43..5fb4bee082 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -391,21 +391,20 @@ static const VFIOIOMMUClass *vfio_get_iommu_class(int iommu_type, Error **errp)
return VFIO_IOMMU_CLASS(klass);
}
-static int vfio_set_iommu(VFIOContainer *container, int group_fd,
- VFIOAddressSpace *space, Error **errp)
+static bool vfio_set_iommu(VFIOContainer *container, int group_fd,
+ VFIOAddressSpace *space, Error **errp)
{
- int iommu_type, ret;
+ int iommu_type;
const VFIOIOMMUClass *vioc;
iommu_type = vfio_get_iommu_type(container, errp);
if (iommu_type < 0) {
- return iommu_type;
+ return false;
}
- ret = ioctl(group_fd, VFIO_GROUP_SET_CONTAINER, &container->fd);
- if (ret) {
+ if (ioctl(group_fd, VFIO_GROUP_SET_CONTAINER, &container->fd)) {
error_setg_errno(errp, errno, "Failed to set group container");
- return -errno;
+ return false;
}
while (ioctl(container->fd, VFIO_SET_IOMMU, iommu_type)) {
@@ -420,7 +419,7 @@ static int vfio_set_iommu(VFIOContainer *container, int group_fd,
continue;
}
error_setg_errno(errp, errno, "Failed to set iommu for container");
- return -errno;
+ return false;
}
container->iommu_type = iommu_type;
@@ -428,11 +427,11 @@ static int vfio_set_iommu(VFIOContainer *container, int group_fd,
vioc = vfio_get_iommu_class(iommu_type, errp);
if (!vioc) {
error_setg(errp, "No available IOMMU models");
- return -EINVAL;
+ return false;
}
vfio_container_init(&container->bcontainer, space, vioc);
- return 0;
+ return true;
}
static int vfio_get_iommu_info(VFIOContainer *container,
@@ -613,8 +612,7 @@ static bool vfio_connect_container(VFIOGroup *group, AddressSpace *as,
container->fd = fd;
bcontainer = &container->bcontainer;
- ret = vfio_set_iommu(container, group->fd, space, errp);
- if (ret) {
+ if (!vfio_set_iommu(container, group->fd, space, errp)) {
goto free_container_exit;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v2 07/11] vfio/container: Make vfio_set_iommu() return bool
2024-05-07 6:42 ` [PATCH v2 07/11] vfio/container: Make vfio_set_iommu() " Zhenzhong Duan
@ 2024-05-14 16:00 ` Cédric Le Goater
0 siblings, 0 replies; 25+ messages in thread
From: Cédric Le Goater @ 2024-05-14 16:00 UTC (permalink / raw)
To: Zhenzhong Duan, qemu-devel; +Cc: alex.williamson, eric.auger, chao.p.peng
On 5/7/24 08:42, 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.
> ---
> hw/vfio/container.c | 22 ++++++++++------------
> 1 file changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index 0a7edfcc43..5fb4bee082 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -391,21 +391,20 @@ static const VFIOIOMMUClass *vfio_get_iommu_class(int iommu_type, Error **errp)
> return VFIO_IOMMU_CLASS(klass);
> }
>
> -static int vfio_set_iommu(VFIOContainer *container, int group_fd,
> - VFIOAddressSpace *space, Error **errp)
> +static bool vfio_set_iommu(VFIOContainer *container, int group_fd,
> + VFIOAddressSpace *space, Error **errp)
> {
> - int iommu_type, ret;
> + int iommu_type;
> const VFIOIOMMUClass *vioc;
>
> iommu_type = vfio_get_iommu_type(container, errp);
> if (iommu_type < 0) {
> - return iommu_type;
> + return false;
> }
>
> - ret = ioctl(group_fd, VFIO_GROUP_SET_CONTAINER, &container->fd);
> - if (ret) {
> + if (ioctl(group_fd, VFIO_GROUP_SET_CONTAINER, &container->fd)) {
> error_setg_errno(errp, errno, "Failed to set group container");
> - return -errno;
> + return false;
> }
>
> while (ioctl(container->fd, VFIO_SET_IOMMU, iommu_type)) {
> @@ -420,7 +419,7 @@ static int vfio_set_iommu(VFIOContainer *container, int group_fd,
> continue;
> }
> error_setg_errno(errp, errno, "Failed to set iommu for container");
> - return -errno;
> + return false;
> }
>
> container->iommu_type = iommu_type;
> @@ -428,11 +427,11 @@ static int vfio_set_iommu(VFIOContainer *container, int group_fd,
> vioc = vfio_get_iommu_class(iommu_type, errp);
> if (!vioc) {
> error_setg(errp, "No available IOMMU models");
> - return -EINVAL;
> + return false;
> }
>
> vfio_container_init(&container->bcontainer, space, vioc);
> - return 0;
> + return true;
> }
>
> static int vfio_get_iommu_info(VFIOContainer *container,
> @@ -613,8 +612,7 @@ static bool vfio_connect_container(VFIOGroup *group, AddressSpace *as,
> container->fd = fd;
> bcontainer = &container->bcontainer;
>
> - ret = vfio_set_iommu(container, group->fd, space, errp);
> - if (ret) {
> + if (!vfio_set_iommu(container, group->fd, space, errp)) {
> goto free_container_exit;
> }
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v2 08/11] vfio/container: Make vfio_get_device() return bool
2024-05-07 6:42 [PATCH v2 00/11] VFIO: misc cleanups Zhenzhong Duan
` (6 preceding siblings ...)
2024-05-07 6:42 ` [PATCH v2 07/11] vfio/container: Make vfio_set_iommu() " Zhenzhong Duan
@ 2024-05-07 6:42 ` Zhenzhong Duan
2024-05-14 16:01 ` Cédric Le Goater
2024-05-07 6:42 ` [PATCH v2 09/11] vfio/iommufd: Make iommufd_cdev_*() " Zhenzhong Duan
` (4 subsequent siblings)
12 siblings, 1 reply; 25+ messages in thread
From: Zhenzhong Duan @ 2024-05-07 6:42 UTC (permalink / raw)
To: qemu-devel; +Cc: alex.williamson, clg, eric.auger, chao.p.peng, Zhenzhong Duan
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>
---
hw/vfio/container.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index 5fb4bee082..b02583ea16 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -800,8 +800,8 @@ static void vfio_put_group(VFIOGroup *group)
g_free(group);
}
-static int vfio_get_device(VFIOGroup *group, const char *name,
- VFIODevice *vbasedev, Error **errp)
+static bool vfio_get_device(VFIOGroup *group, const char *name,
+ VFIODevice *vbasedev, Error **errp)
{
g_autofree struct vfio_device_info *info = NULL;
int fd;
@@ -813,14 +813,14 @@ static int vfio_get_device(VFIOGroup *group, const char *name,
error_append_hint(errp,
"Verify all devices in group %d are bound to vfio-<bus> "
"or pci-stub and not already in use\n", group->groupid);
- return fd;
+ return false;
}
info = vfio_get_device_info(fd);
if (!info) {
error_setg_errno(errp, errno, "error getting device info");
close(fd);
- return -1;
+ return false;
}
/*
@@ -835,7 +835,7 @@ static int vfio_get_device(VFIOGroup *group, const char *name,
error_setg(errp, "Inconsistent setting of support for discarding "
"RAM (e.g., balloon) within group");
close(fd);
- return -1;
+ return false;
}
if (!group->ram_block_discard_allowed) {
@@ -856,7 +856,7 @@ static int vfio_get_device(VFIOGroup *group, const char *name,
vbasedev->reset_works = !!(info->flags & VFIO_DEVICE_FLAGS_RESET);
- return 0;
+ return true;
}
static void vfio_put_base_device(VFIODevice *vbasedev)
@@ -909,7 +909,6 @@ static bool vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev,
VFIODevice *vbasedev_iter;
VFIOGroup *group;
VFIOContainerBase *bcontainer;
- int ret;
if (groupid < 0) {
return false;
@@ -929,8 +928,7 @@ static bool vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev,
return false;
}
}
- ret = vfio_get_device(group, name, vbasedev, errp);
- if (ret) {
+ if (!vfio_get_device(group, name, vbasedev, errp)) {
vfio_put_group(group);
return false;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v2 08/11] vfio/container: Make vfio_get_device() return bool
2024-05-07 6:42 ` [PATCH v2 08/11] vfio/container: Make vfio_get_device() " Zhenzhong Duan
@ 2024-05-14 16:01 ` Cédric Le Goater
0 siblings, 0 replies; 25+ messages in thread
From: Cédric Le Goater @ 2024-05-14 16:01 UTC (permalink / raw)
To: Zhenzhong Duan, qemu-devel; +Cc: alex.williamson, eric.auger, chao.p.peng
On 5/7/24 08:42, 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.
> ---
> hw/vfio/container.c | 16 +++++++---------
> 1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index 5fb4bee082..b02583ea16 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -800,8 +800,8 @@ static void vfio_put_group(VFIOGroup *group)
> g_free(group);
> }
>
> -static int vfio_get_device(VFIOGroup *group, const char *name,
> - VFIODevice *vbasedev, Error **errp)
> +static bool vfio_get_device(VFIOGroup *group, const char *name,
> + VFIODevice *vbasedev, Error **errp)
> {
> g_autofree struct vfio_device_info *info = NULL;
> int fd;
> @@ -813,14 +813,14 @@ static int vfio_get_device(VFIOGroup *group, const char *name,
> error_append_hint(errp,
> "Verify all devices in group %d are bound to vfio-<bus> "
> "or pci-stub and not already in use\n", group->groupid);
> - return fd;
> + return false;
> }
>
> info = vfio_get_device_info(fd);
> if (!info) {
> error_setg_errno(errp, errno, "error getting device info");
> close(fd);
> - return -1;
> + return false;
> }
>
> /*
> @@ -835,7 +835,7 @@ static int vfio_get_device(VFIOGroup *group, const char *name,
> error_setg(errp, "Inconsistent setting of support for discarding "
> "RAM (e.g., balloon) within group");
> close(fd);
> - return -1;
> + return false;
> }
>
> if (!group->ram_block_discard_allowed) {
> @@ -856,7 +856,7 @@ static int vfio_get_device(VFIOGroup *group, const char *name,
>
> vbasedev->reset_works = !!(info->flags & VFIO_DEVICE_FLAGS_RESET);
>
> - return 0;
> + return true;
> }
>
> static void vfio_put_base_device(VFIODevice *vbasedev)
> @@ -909,7 +909,6 @@ static bool vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev,
> VFIODevice *vbasedev_iter;
> VFIOGroup *group;
> VFIOContainerBase *bcontainer;
> - int ret;
>
> if (groupid < 0) {
> return false;
> @@ -929,8 +928,7 @@ static bool vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev,
> return false;
> }
> }
> - ret = vfio_get_device(group, name, vbasedev, errp);
> - if (ret) {
> + if (!vfio_get_device(group, name, vbasedev, errp)) {
> vfio_put_group(group);
> return false;
> }
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v2 09/11] vfio/iommufd: Make iommufd_cdev_*() return bool
2024-05-07 6:42 [PATCH v2 00/11] VFIO: misc cleanups Zhenzhong Duan
` (7 preceding siblings ...)
2024-05-07 6:42 ` [PATCH v2 08/11] vfio/container: Make vfio_get_device() " Zhenzhong Duan
@ 2024-05-07 6:42 ` Zhenzhong Duan
2024-05-14 16:02 ` Cédric Le Goater
2024-05-07 6:42 ` [PATCH v2 10/11] vfio/cpr: Make vfio_cpr_register_container() " Zhenzhong Duan
` (3 subsequent siblings)
12 siblings, 1 reply; 25+ messages in thread
From: Zhenzhong Duan @ 2024-05-07 6:42 UTC (permalink / raw)
To: qemu-devel; +Cc: alex.williamson, clg, eric.auger, chao.p.peng, Zhenzhong Duan
This is to follow the coding standand to return bool if 'Error **'
is used to pass error.
The changed functions include:
iommufd_cdev_kvm_device_add
iommufd_cdev_connect_and_bind
iommufd_cdev_attach_ioas_hwpt
iommufd_cdev_detach_ioas_hwpt
iommufd_cdev_attach_container
iommufd_cdev_get_info_iova_range
After the change, all functions in hw/vfio/iommufd.c follows the
standand.
Suggested-by: Cédric Le Goater <clg@redhat.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
hw/vfio/iommufd.c | 88 +++++++++++++++++++++--------------------------
1 file changed, 39 insertions(+), 49 deletions(-)
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index 4c6992fca1..84c86b970e 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -49,9 +49,9 @@ static int iommufd_cdev_unmap(const VFIOContainerBase *bcontainer,
container->ioas_id, iova, size);
}
-static int iommufd_cdev_kvm_device_add(VFIODevice *vbasedev, Error **errp)
+static bool iommufd_cdev_kvm_device_add(VFIODevice *vbasedev, Error **errp)
{
- return vfio_kvm_device_add_fd(vbasedev->fd, errp);
+ return !vfio_kvm_device_add_fd(vbasedev->fd, errp);
}
static void iommufd_cdev_kvm_device_del(VFIODevice *vbasedev)
@@ -63,18 +63,16 @@ static void iommufd_cdev_kvm_device_del(VFIODevice *vbasedev)
}
}
-static int iommufd_cdev_connect_and_bind(VFIODevice *vbasedev, Error **errp)
+static bool iommufd_cdev_connect_and_bind(VFIODevice *vbasedev, Error **errp)
{
IOMMUFDBackend *iommufd = vbasedev->iommufd;
struct vfio_device_bind_iommufd bind = {
.argsz = sizeof(bind),
.flags = 0,
};
- int ret;
- ret = iommufd_backend_connect(iommufd, errp);
- if (ret) {
- return ret;
+ if (iommufd_backend_connect(iommufd, errp)) {
+ return false;
}
/*
@@ -82,15 +80,13 @@ static int iommufd_cdev_connect_and_bind(VFIODevice *vbasedev, Error **errp)
* in KVM. Especially for some emulated devices, it requires
* to have kvm information in the device open.
*/
- ret = iommufd_cdev_kvm_device_add(vbasedev, errp);
- if (ret) {
+ if (!iommufd_cdev_kvm_device_add(vbasedev, errp)) {
goto err_kvm_device_add;
}
/* Bind device to iommufd */
bind.iommufd = iommufd->fd;
- ret = ioctl(vbasedev->fd, VFIO_DEVICE_BIND_IOMMUFD, &bind);
- if (ret) {
+ if (ioctl(vbasedev->fd, VFIO_DEVICE_BIND_IOMMUFD, &bind)) {
error_setg_errno(errp, errno, "error bind device fd=%d to iommufd=%d",
vbasedev->fd, bind.iommufd);
goto err_bind;
@@ -99,12 +95,12 @@ static int iommufd_cdev_connect_and_bind(VFIODevice *vbasedev, Error **errp)
vbasedev->devid = bind.out_devid;
trace_iommufd_cdev_connect_and_bind(bind.iommufd, vbasedev->name,
vbasedev->fd, vbasedev->devid);
- return ret;
+ return true;
err_bind:
iommufd_cdev_kvm_device_del(vbasedev);
err_kvm_device_add:
iommufd_backend_disconnect(iommufd);
- return ret;
+ return false;
}
static void iommufd_cdev_unbind_and_disconnect(VFIODevice *vbasedev)
@@ -176,10 +172,10 @@ out:
return ret;
}
-static int iommufd_cdev_attach_ioas_hwpt(VFIODevice *vbasedev, uint32_t id,
+static bool iommufd_cdev_attach_ioas_hwpt(VFIODevice *vbasedev, uint32_t id,
Error **errp)
{
- int ret, iommufd = vbasedev->iommufd->fd;
+ int iommufd = vbasedev->iommufd->fd;
struct vfio_device_attach_iommufd_pt attach_data = {
.argsz = sizeof(attach_data),
.flags = 0,
@@ -187,38 +183,38 @@ static int iommufd_cdev_attach_ioas_hwpt(VFIODevice *vbasedev, uint32_t id,
};
/* Attach device to an IOAS or hwpt within iommufd */
- ret = ioctl(vbasedev->fd, VFIO_DEVICE_ATTACH_IOMMUFD_PT, &attach_data);
- if (ret) {
+ if (ioctl(vbasedev->fd, VFIO_DEVICE_ATTACH_IOMMUFD_PT, &attach_data)) {
error_setg_errno(errp, errno,
"[iommufd=%d] error attach %s (%d) to id=%d",
iommufd, vbasedev->name, vbasedev->fd, id);
- } else {
- trace_iommufd_cdev_attach_ioas_hwpt(iommufd, vbasedev->name,
- vbasedev->fd, id);
+ return false;
}
- return ret;
+
+ trace_iommufd_cdev_attach_ioas_hwpt(iommufd, vbasedev->name,
+ vbasedev->fd, id);
+ return true;
}
-static int iommufd_cdev_detach_ioas_hwpt(VFIODevice *vbasedev, Error **errp)
+static bool iommufd_cdev_detach_ioas_hwpt(VFIODevice *vbasedev, Error **errp)
{
- int ret, iommufd = vbasedev->iommufd->fd;
+ int iommufd = vbasedev->iommufd->fd;
struct vfio_device_detach_iommufd_pt detach_data = {
.argsz = sizeof(detach_data),
.flags = 0,
};
- ret = ioctl(vbasedev->fd, VFIO_DEVICE_DETACH_IOMMUFD_PT, &detach_data);
- if (ret) {
+ if (ioctl(vbasedev->fd, VFIO_DEVICE_DETACH_IOMMUFD_PT, &detach_data)) {
error_setg_errno(errp, errno, "detach %s failed", vbasedev->name);
- } else {
- trace_iommufd_cdev_detach_ioas_hwpt(iommufd, vbasedev->name);
+ return false;
}
- return ret;
+
+ trace_iommufd_cdev_detach_ioas_hwpt(iommufd, vbasedev->name);
+ return true;
}
-static int iommufd_cdev_attach_container(VFIODevice *vbasedev,
- VFIOIOMMUFDContainer *container,
- Error **errp)
+static bool iommufd_cdev_attach_container(VFIODevice *vbasedev,
+ VFIOIOMMUFDContainer *container,
+ Error **errp)
{
return iommufd_cdev_attach_ioas_hwpt(vbasedev, container->ioas_id, errp);
}
@@ -228,7 +224,7 @@ static void iommufd_cdev_detach_container(VFIODevice *vbasedev,
{
Error *err = NULL;
- if (iommufd_cdev_detach_ioas_hwpt(vbasedev, &err)) {
+ if (!iommufd_cdev_detach_ioas_hwpt(vbasedev, &err)) {
error_report_err(err);
}
}
@@ -254,20 +250,19 @@ static int iommufd_cdev_ram_block_discard_disable(bool state)
return ram_block_uncoordinated_discard_disable(state);
}
-static int iommufd_cdev_get_info_iova_range(VFIOIOMMUFDContainer *container,
- uint32_t ioas_id, Error **errp)
+static bool iommufd_cdev_get_info_iova_range(VFIOIOMMUFDContainer *container,
+ uint32_t ioas_id, Error **errp)
{
VFIOContainerBase *bcontainer = &container->bcontainer;
g_autofree struct iommu_ioas_iova_ranges *info = NULL;
struct iommu_iova_range *iova_ranges;
- int ret, sz, fd = container->be->fd;
+ int sz, fd = container->be->fd;
info = g_malloc0(sizeof(*info));
info->size = sizeof(*info);
info->ioas_id = ioas_id;
- ret = ioctl(fd, IOMMU_IOAS_IOVA_RANGES, info);
- if (ret && errno != EMSGSIZE) {
+ if (ioctl(fd, IOMMU_IOAS_IOVA_RANGES, info) && errno != EMSGSIZE) {
goto error;
}
@@ -275,8 +270,7 @@ static int iommufd_cdev_get_info_iova_range(VFIOIOMMUFDContainer *container,
info = g_realloc(info, sizeof(*info) + sz);
info->allowed_iovas = (uintptr_t)(info + 1);
- ret = ioctl(fd, IOMMU_IOAS_IOVA_RANGES, info);
- if (ret) {
+ if (ioctl(fd, IOMMU_IOAS_IOVA_RANGES, info)) {
goto error;
}
@@ -291,12 +285,11 @@ static int iommufd_cdev_get_info_iova_range(VFIOIOMMUFDContainer *container,
}
bcontainer->pgsizes = info->out_iova_alignment;
- return 0;
+ return true;
error:
- ret = -errno;
error_setg_errno(errp, errno, "Cannot get IOVA ranges");
- return ret;
+ return false;
}
static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
@@ -322,8 +315,7 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
devfd = vbasedev->fd;
}
- ret = iommufd_cdev_connect_and_bind(vbasedev, errp);
- if (ret) {
+ if (!iommufd_cdev_connect_and_bind(vbasedev, errp)) {
goto err_connect_bind;
}
@@ -336,7 +328,7 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
vbasedev->iommufd != container->be) {
continue;
}
- if (iommufd_cdev_attach_container(vbasedev, container, &err)) {
+ if (!iommufd_cdev_attach_container(vbasedev, container, &err)) {
const char *msg = error_get_pretty(err);
trace_iommufd_cdev_fail_attach_existing_container(msg);
@@ -369,8 +361,7 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
vfio_container_init(bcontainer, space, iommufd_vioc);
QLIST_INSERT_HEAD(&space->containers, bcontainer, next);
- ret = iommufd_cdev_attach_container(vbasedev, container, errp);
- if (ret) {
+ if (!iommufd_cdev_attach_container(vbasedev, container, errp)) {
goto err_attach_container;
}
@@ -379,8 +370,7 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
goto err_discard_disable;
}
- ret = iommufd_cdev_get_info_iova_range(container, ioas_id, &err);
- if (ret) {
+ if (!iommufd_cdev_get_info_iova_range(container, ioas_id, &err)) {
error_append_hint(&err,
"Fallback to default 64bit IOVA range and 4K page size\n");
warn_report_err(err);
--
2.34.1
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v2 09/11] vfio/iommufd: Make iommufd_cdev_*() return bool
2024-05-07 6:42 ` [PATCH v2 09/11] vfio/iommufd: Make iommufd_cdev_*() " Zhenzhong Duan
@ 2024-05-14 16:02 ` Cédric Le Goater
0 siblings, 0 replies; 25+ messages in thread
From: Cédric Le Goater @ 2024-05-14 16:02 UTC (permalink / raw)
To: Zhenzhong Duan, qemu-devel; +Cc: alex.williamson, eric.auger, chao.p.peng
On 5/7/24 08:42, Zhenzhong Duan wrote:
> This is to follow the coding standand to return bool if 'Error **'
> is used to pass error.
>
> The changed functions include:
>
> iommufd_cdev_kvm_device_add
> iommufd_cdev_connect_and_bind
> iommufd_cdev_attach_ioas_hwpt
> iommufd_cdev_detach_ioas_hwpt
> iommufd_cdev_attach_container
> iommufd_cdev_get_info_iova_range
>
> After the change, all functions in hw/vfio/iommufd.c follows the
> standand.
>
> 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.
> ---
> hw/vfio/iommufd.c | 88 +++++++++++++++++++++--------------------------
> 1 file changed, 39 insertions(+), 49 deletions(-)
>
> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> index 4c6992fca1..84c86b970e 100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -49,9 +49,9 @@ static int iommufd_cdev_unmap(const VFIOContainerBase *bcontainer,
> container->ioas_id, iova, size);
> }
>
> -static int iommufd_cdev_kvm_device_add(VFIODevice *vbasedev, Error **errp)
> +static bool iommufd_cdev_kvm_device_add(VFIODevice *vbasedev, Error **errp)
> {
> - return vfio_kvm_device_add_fd(vbasedev->fd, errp);
> + return !vfio_kvm_device_add_fd(vbasedev->fd, errp);
> }
>
> static void iommufd_cdev_kvm_device_del(VFIODevice *vbasedev)
> @@ -63,18 +63,16 @@ static void iommufd_cdev_kvm_device_del(VFIODevice *vbasedev)
> }
> }
>
> -static int iommufd_cdev_connect_and_bind(VFIODevice *vbasedev, Error **errp)
> +static bool iommufd_cdev_connect_and_bind(VFIODevice *vbasedev, Error **errp)
> {
> IOMMUFDBackend *iommufd = vbasedev->iommufd;
> struct vfio_device_bind_iommufd bind = {
> .argsz = sizeof(bind),
> .flags = 0,
> };
> - int ret;
>
> - ret = iommufd_backend_connect(iommufd, errp);
> - if (ret) {
> - return ret;
> + if (iommufd_backend_connect(iommufd, errp)) {
> + return false;
> }
>
> /*
> @@ -82,15 +80,13 @@ static int iommufd_cdev_connect_and_bind(VFIODevice *vbasedev, Error **errp)
> * in KVM. Especially for some emulated devices, it requires
> * to have kvm information in the device open.
> */
> - ret = iommufd_cdev_kvm_device_add(vbasedev, errp);
> - if (ret) {
> + if (!iommufd_cdev_kvm_device_add(vbasedev, errp)) {
> goto err_kvm_device_add;
> }
>
> /* Bind device to iommufd */
> bind.iommufd = iommufd->fd;
> - ret = ioctl(vbasedev->fd, VFIO_DEVICE_BIND_IOMMUFD, &bind);
> - if (ret) {
> + if (ioctl(vbasedev->fd, VFIO_DEVICE_BIND_IOMMUFD, &bind)) {
> error_setg_errno(errp, errno, "error bind device fd=%d to iommufd=%d",
> vbasedev->fd, bind.iommufd);
> goto err_bind;
> @@ -99,12 +95,12 @@ static int iommufd_cdev_connect_and_bind(VFIODevice *vbasedev, Error **errp)
> vbasedev->devid = bind.out_devid;
> trace_iommufd_cdev_connect_and_bind(bind.iommufd, vbasedev->name,
> vbasedev->fd, vbasedev->devid);
> - return ret;
> + return true;
> err_bind:
> iommufd_cdev_kvm_device_del(vbasedev);
> err_kvm_device_add:
> iommufd_backend_disconnect(iommufd);
> - return ret;
> + return false;
> }
>
> static void iommufd_cdev_unbind_and_disconnect(VFIODevice *vbasedev)
> @@ -176,10 +172,10 @@ out:
> return ret;
> }
>
> -static int iommufd_cdev_attach_ioas_hwpt(VFIODevice *vbasedev, uint32_t id,
> +static bool iommufd_cdev_attach_ioas_hwpt(VFIODevice *vbasedev, uint32_t id,
> Error **errp)
> {
> - int ret, iommufd = vbasedev->iommufd->fd;
> + int iommufd = vbasedev->iommufd->fd;
> struct vfio_device_attach_iommufd_pt attach_data = {
> .argsz = sizeof(attach_data),
> .flags = 0,
> @@ -187,38 +183,38 @@ static int iommufd_cdev_attach_ioas_hwpt(VFIODevice *vbasedev, uint32_t id,
> };
>
> /* Attach device to an IOAS or hwpt within iommufd */
> - ret = ioctl(vbasedev->fd, VFIO_DEVICE_ATTACH_IOMMUFD_PT, &attach_data);
> - if (ret) {
> + if (ioctl(vbasedev->fd, VFIO_DEVICE_ATTACH_IOMMUFD_PT, &attach_data)) {
> error_setg_errno(errp, errno,
> "[iommufd=%d] error attach %s (%d) to id=%d",
> iommufd, vbasedev->name, vbasedev->fd, id);
> - } else {
> - trace_iommufd_cdev_attach_ioas_hwpt(iommufd, vbasedev->name,
> - vbasedev->fd, id);
> + return false;
> }
> - return ret;
> +
> + trace_iommufd_cdev_attach_ioas_hwpt(iommufd, vbasedev->name,
> + vbasedev->fd, id);
> + return true;
> }
>
> -static int iommufd_cdev_detach_ioas_hwpt(VFIODevice *vbasedev, Error **errp)
> +static bool iommufd_cdev_detach_ioas_hwpt(VFIODevice *vbasedev, Error **errp)
> {
> - int ret, iommufd = vbasedev->iommufd->fd;
> + int iommufd = vbasedev->iommufd->fd;
> struct vfio_device_detach_iommufd_pt detach_data = {
> .argsz = sizeof(detach_data),
> .flags = 0,
> };
>
> - ret = ioctl(vbasedev->fd, VFIO_DEVICE_DETACH_IOMMUFD_PT, &detach_data);
> - if (ret) {
> + if (ioctl(vbasedev->fd, VFIO_DEVICE_DETACH_IOMMUFD_PT, &detach_data)) {
> error_setg_errno(errp, errno, "detach %s failed", vbasedev->name);
> - } else {
> - trace_iommufd_cdev_detach_ioas_hwpt(iommufd, vbasedev->name);
> + return false;
> }
> - return ret;
> +
> + trace_iommufd_cdev_detach_ioas_hwpt(iommufd, vbasedev->name);
> + return true;
> }
>
> -static int iommufd_cdev_attach_container(VFIODevice *vbasedev,
> - VFIOIOMMUFDContainer *container,
> - Error **errp)
> +static bool iommufd_cdev_attach_container(VFIODevice *vbasedev,
> + VFIOIOMMUFDContainer *container,
> + Error **errp)
> {
> return iommufd_cdev_attach_ioas_hwpt(vbasedev, container->ioas_id, errp);
> }
> @@ -228,7 +224,7 @@ static void iommufd_cdev_detach_container(VFIODevice *vbasedev,
> {
> Error *err = NULL;
>
> - if (iommufd_cdev_detach_ioas_hwpt(vbasedev, &err)) {
> + if (!iommufd_cdev_detach_ioas_hwpt(vbasedev, &err)) {
> error_report_err(err);
> }
> }
> @@ -254,20 +250,19 @@ static int iommufd_cdev_ram_block_discard_disable(bool state)
> return ram_block_uncoordinated_discard_disable(state);
> }
>
> -static int iommufd_cdev_get_info_iova_range(VFIOIOMMUFDContainer *container,
> - uint32_t ioas_id, Error **errp)
> +static bool iommufd_cdev_get_info_iova_range(VFIOIOMMUFDContainer *container,
> + uint32_t ioas_id, Error **errp)
> {
> VFIOContainerBase *bcontainer = &container->bcontainer;
> g_autofree struct iommu_ioas_iova_ranges *info = NULL;
> struct iommu_iova_range *iova_ranges;
> - int ret, sz, fd = container->be->fd;
> + int sz, fd = container->be->fd;
>
> info = g_malloc0(sizeof(*info));
> info->size = sizeof(*info);
> info->ioas_id = ioas_id;
>
> - ret = ioctl(fd, IOMMU_IOAS_IOVA_RANGES, info);
> - if (ret && errno != EMSGSIZE) {
> + if (ioctl(fd, IOMMU_IOAS_IOVA_RANGES, info) && errno != EMSGSIZE) {
> goto error;
> }
>
> @@ -275,8 +270,7 @@ static int iommufd_cdev_get_info_iova_range(VFIOIOMMUFDContainer *container,
> info = g_realloc(info, sizeof(*info) + sz);
> info->allowed_iovas = (uintptr_t)(info + 1);
>
> - ret = ioctl(fd, IOMMU_IOAS_IOVA_RANGES, info);
> - if (ret) {
> + if (ioctl(fd, IOMMU_IOAS_IOVA_RANGES, info)) {
> goto error;
> }
>
> @@ -291,12 +285,11 @@ static int iommufd_cdev_get_info_iova_range(VFIOIOMMUFDContainer *container,
> }
> bcontainer->pgsizes = info->out_iova_alignment;
>
> - return 0;
> + return true;
>
> error:
> - ret = -errno;
> error_setg_errno(errp, errno, "Cannot get IOVA ranges");
> - return ret;
> + return false;
> }
>
> static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
> @@ -322,8 +315,7 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
> devfd = vbasedev->fd;
> }
>
> - ret = iommufd_cdev_connect_and_bind(vbasedev, errp);
> - if (ret) {
> + if (!iommufd_cdev_connect_and_bind(vbasedev, errp)) {
> goto err_connect_bind;
> }
>
> @@ -336,7 +328,7 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
> vbasedev->iommufd != container->be) {
> continue;
> }
> - if (iommufd_cdev_attach_container(vbasedev, container, &err)) {
> + if (!iommufd_cdev_attach_container(vbasedev, container, &err)) {
> const char *msg = error_get_pretty(err);
>
> trace_iommufd_cdev_fail_attach_existing_container(msg);
> @@ -369,8 +361,7 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
> vfio_container_init(bcontainer, space, iommufd_vioc);
> QLIST_INSERT_HEAD(&space->containers, bcontainer, next);
>
> - ret = iommufd_cdev_attach_container(vbasedev, container, errp);
> - if (ret) {
> + if (!iommufd_cdev_attach_container(vbasedev, container, errp)) {
> goto err_attach_container;
> }
>
> @@ -379,8 +370,7 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
> goto err_discard_disable;
> }
>
> - ret = iommufd_cdev_get_info_iova_range(container, ioas_id, &err);
> - if (ret) {
> + if (!iommufd_cdev_get_info_iova_range(container, ioas_id, &err)) {
> error_append_hint(&err,
> "Fallback to default 64bit IOVA range and 4K page size\n");
> warn_report_err(err);
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v2 10/11] vfio/cpr: Make vfio_cpr_register_container() return bool
2024-05-07 6:42 [PATCH v2 00/11] VFIO: misc cleanups Zhenzhong Duan
` (8 preceding siblings ...)
2024-05-07 6:42 ` [PATCH v2 09/11] vfio/iommufd: Make iommufd_cdev_*() " Zhenzhong Duan
@ 2024-05-07 6:42 ` Zhenzhong Duan
2024-05-14 16:02 ` Cédric Le Goater
2024-05-07 6:42 ` [PATCH v2 11/11] backends/iommufd: Make iommufd_backend_*() " Zhenzhong Duan
` (2 subsequent siblings)
12 siblings, 1 reply; 25+ messages in thread
From: Zhenzhong Duan @ 2024-05-07 6:42 UTC (permalink / raw)
To: qemu-devel; +Cc: alex.williamson, clg, eric.auger, chao.p.peng, Zhenzhong Duan
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 | 2 +-
hw/vfio/container.c | 3 +--
hw/vfio/cpr.c | 4 ++--
hw/vfio/iommufd.c | 3 +--
4 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index a7b6fc8f46..e4c60374fa 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -205,7 +205,7 @@ void vfio_detach_device(VFIODevice *vbasedev);
int vfio_kvm_device_add_fd(int fd, Error **errp);
int vfio_kvm_device_del_fd(int fd, Error **errp);
-int vfio_cpr_register_container(VFIOContainerBase *bcontainer, Error **errp);
+bool vfio_cpr_register_container(VFIOContainerBase *bcontainer, Error **errp);
void vfio_cpr_unregister_container(VFIOContainerBase *bcontainer);
extern const MemoryRegionOps vfio_region_ops;
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index b02583ea16..86266f3b83 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -616,8 +616,7 @@ static bool vfio_connect_container(VFIOGroup *group, AddressSpace *as,
goto free_container_exit;
}
- ret = vfio_cpr_register_container(bcontainer, errp);
- if (ret) {
+ if (!vfio_cpr_register_container(bcontainer, errp)) {
goto free_container_exit;
}
diff --git a/hw/vfio/cpr.c b/hw/vfio/cpr.c
index 392c2dd95d..87e51fcee1 100644
--- a/hw/vfio/cpr.c
+++ b/hw/vfio/cpr.c
@@ -25,12 +25,12 @@ static int vfio_cpr_reboot_notifier(NotifierWithReturn *notifier,
return 0;
}
-int vfio_cpr_register_container(VFIOContainerBase *bcontainer, Error **errp)
+bool vfio_cpr_register_container(VFIOContainerBase *bcontainer, Error **errp)
{
migration_add_notifier_mode(&bcontainer->cpr_reboot_notifier,
vfio_cpr_reboot_notifier,
MIG_MODE_CPR_REBOOT);
- return 0;
+ return true;
}
void vfio_cpr_unregister_container(VFIOContainerBase *bcontainer)
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index 84c86b970e..6a446b16dc 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -396,8 +396,7 @@ found_container:
goto err_listener_register;
}
- ret = vfio_cpr_register_container(bcontainer, errp);
- if (ret) {
+ if (!vfio_cpr_register_container(bcontainer, errp)) {
goto err_listener_register;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v2 10/11] vfio/cpr: Make vfio_cpr_register_container() return bool
2024-05-07 6:42 ` [PATCH v2 10/11] vfio/cpr: Make vfio_cpr_register_container() " Zhenzhong Duan
@ 2024-05-14 16:02 ` Cédric Le Goater
0 siblings, 0 replies; 25+ messages in thread
From: Cédric Le Goater @ 2024-05-14 16:02 UTC (permalink / raw)
To: Zhenzhong Duan, qemu-devel; +Cc: alex.williamson, eric.auger, chao.p.peng
On 5/7/24 08:42, 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-common.h | 2 +-
> hw/vfio/container.c | 3 +--
> hw/vfio/cpr.c | 4 ++--
> hw/vfio/iommufd.c | 3 +--
> 4 files changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index a7b6fc8f46..e4c60374fa 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -205,7 +205,7 @@ void vfio_detach_device(VFIODevice *vbasedev);
> int vfio_kvm_device_add_fd(int fd, Error **errp);
> int vfio_kvm_device_del_fd(int fd, Error **errp);
>
> -int vfio_cpr_register_container(VFIOContainerBase *bcontainer, Error **errp);
> +bool vfio_cpr_register_container(VFIOContainerBase *bcontainer, Error **errp);
> void vfio_cpr_unregister_container(VFIOContainerBase *bcontainer);
>
> extern const MemoryRegionOps vfio_region_ops;
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index b02583ea16..86266f3b83 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -616,8 +616,7 @@ static bool vfio_connect_container(VFIOGroup *group, AddressSpace *as,
> goto free_container_exit;
> }
>
> - ret = vfio_cpr_register_container(bcontainer, errp);
> - if (ret) {
> + if (!vfio_cpr_register_container(bcontainer, errp)) {
> goto free_container_exit;
> }
>
> diff --git a/hw/vfio/cpr.c b/hw/vfio/cpr.c
> index 392c2dd95d..87e51fcee1 100644
> --- a/hw/vfio/cpr.c
> +++ b/hw/vfio/cpr.c
> @@ -25,12 +25,12 @@ static int vfio_cpr_reboot_notifier(NotifierWithReturn *notifier,
> return 0;
> }
>
> -int vfio_cpr_register_container(VFIOContainerBase *bcontainer, Error **errp)
> +bool vfio_cpr_register_container(VFIOContainerBase *bcontainer, Error **errp)
> {
> migration_add_notifier_mode(&bcontainer->cpr_reboot_notifier,
> vfio_cpr_reboot_notifier,
> MIG_MODE_CPR_REBOOT);
> - return 0;
> + return true;
> }
>
> void vfio_cpr_unregister_container(VFIOContainerBase *bcontainer)
> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> index 84c86b970e..6a446b16dc 100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -396,8 +396,7 @@ found_container:
> goto err_listener_register;
> }
>
> - ret = vfio_cpr_register_container(bcontainer, errp);
> - if (ret) {
> + if (!vfio_cpr_register_container(bcontainer, errp)) {
> goto err_listener_register;
> }
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v2 11/11] backends/iommufd: Make iommufd_backend_*() return bool
2024-05-07 6:42 [PATCH v2 00/11] VFIO: misc cleanups Zhenzhong Duan
` (9 preceding siblings ...)
2024-05-07 6:42 ` [PATCH v2 10/11] vfio/cpr: Make vfio_cpr_register_container() " Zhenzhong Duan
@ 2024-05-07 6:42 ` Zhenzhong Duan
2024-05-14 16:03 ` Cédric Le Goater
2024-05-14 3:46 ` [PATCH v2 00/11] VFIO: misc cleanups Duan, Zhenzhong
2024-05-16 16:30 ` Cédric Le Goater
12 siblings, 1 reply; 25+ messages in thread
From: Zhenzhong Duan @ 2024-05-07 6:42 UTC (permalink / raw)
To: qemu-devel
Cc: alex.williamson, clg, eric.auger, chao.p.peng, Zhenzhong Duan,
Yi Liu
This is to follow the coding standand to return bool if 'Error **'
is used to pass error.
The changed functions include:
iommufd_backend_connect
iommufd_backend_alloc_ioas
By this chance, simplify the functions a bit by avoiding duplicate
recordings, e.g., log through either error interface or trace, not
both.
Suggested-by: Cédric Le Goater <clg@redhat.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
include/sysemu/iommufd.h | 6 +++---
backends/iommufd.c | 29 +++++++++++++----------------
hw/vfio/iommufd.c | 5 ++---
backends/trace-events | 4 ++--
4 files changed, 20 insertions(+), 24 deletions(-)
diff --git a/include/sysemu/iommufd.h b/include/sysemu/iommufd.h
index 9af27ebd6c..293bfbe967 100644
--- a/include/sysemu/iommufd.h
+++ b/include/sysemu/iommufd.h
@@ -23,11 +23,11 @@ struct IOMMUFDBackend {
/*< public >*/
};
-int iommufd_backend_connect(IOMMUFDBackend *be, Error **errp);
+bool iommufd_backend_connect(IOMMUFDBackend *be, Error **errp);
void iommufd_backend_disconnect(IOMMUFDBackend *be);
-int iommufd_backend_alloc_ioas(IOMMUFDBackend *be, uint32_t *ioas_id,
- Error **errp);
+bool iommufd_backend_alloc_ioas(IOMMUFDBackend *be, uint32_t *ioas_id,
+ Error **errp);
void iommufd_backend_free_id(IOMMUFDBackend *be, uint32_t id);
int iommufd_backend_map_dma(IOMMUFDBackend *be, uint32_t ioas_id, hwaddr iova,
ram_addr_t size, void *vaddr, bool readonly);
diff --git a/backends/iommufd.c b/backends/iommufd.c
index 76a0204852..c506afbdac 100644
--- a/backends/iommufd.c
+++ b/backends/iommufd.c
@@ -72,24 +72,22 @@ static void iommufd_backend_class_init(ObjectClass *oc, void *data)
object_class_property_add_str(oc, "fd", NULL, iommufd_backend_set_fd);
}
-int iommufd_backend_connect(IOMMUFDBackend *be, Error **errp)
+bool iommufd_backend_connect(IOMMUFDBackend *be, Error **errp)
{
- int fd, ret = 0;
+ int fd;
if (be->owned && !be->users) {
fd = qemu_open_old("/dev/iommu", O_RDWR);
if (fd < 0) {
error_setg_errno(errp, errno, "/dev/iommu opening failed");
- ret = fd;
- goto out;
+ return false;
}
be->fd = fd;
}
be->users++;
-out:
- trace_iommufd_backend_connect(be->fd, be->owned,
- be->users, ret);
- return ret;
+
+ trace_iommufd_backend_connect(be->fd, be->owned, be->users);
+ return true;
}
void iommufd_backend_disconnect(IOMMUFDBackend *be)
@@ -106,25 +104,24 @@ out:
trace_iommufd_backend_disconnect(be->fd, be->users);
}
-int iommufd_backend_alloc_ioas(IOMMUFDBackend *be, uint32_t *ioas_id,
- Error **errp)
+bool iommufd_backend_alloc_ioas(IOMMUFDBackend *be, uint32_t *ioas_id,
+ Error **errp)
{
- int ret, fd = be->fd;
+ int fd = be->fd;
struct iommu_ioas_alloc alloc_data = {
.size = sizeof(alloc_data),
.flags = 0,
};
- ret = ioctl(fd, IOMMU_IOAS_ALLOC, &alloc_data);
- if (ret) {
+ if (ioctl(fd, IOMMU_IOAS_ALLOC, &alloc_data)) {
error_setg_errno(errp, errno, "Failed to allocate ioas");
- return ret;
+ return false;
}
*ioas_id = alloc_data.out_ioas_id;
- trace_iommufd_backend_alloc_ioas(fd, *ioas_id, ret);
+ trace_iommufd_backend_alloc_ioas(fd, *ioas_id);
- return ret;
+ return true;
}
void iommufd_backend_free_id(IOMMUFDBackend *be, uint32_t id)
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index 6a446b16dc..554f9a6292 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -71,7 +71,7 @@ static bool iommufd_cdev_connect_and_bind(VFIODevice *vbasedev, Error **errp)
.flags = 0,
};
- if (iommufd_backend_connect(iommufd, errp)) {
+ if (!iommufd_backend_connect(iommufd, errp)) {
return false;
}
@@ -346,8 +346,7 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
}
/* Need to allocate a new dedicated container */
- ret = iommufd_backend_alloc_ioas(vbasedev->iommufd, &ioas_id, errp);
- if (ret < 0) {
+ if (!iommufd_backend_alloc_ioas(vbasedev->iommufd, &ioas_id, errp)) {
goto err_alloc_ioas;
}
diff --git a/backends/trace-events b/backends/trace-events
index d45c6e31a6..211e6f374a 100644
--- a/backends/trace-events
+++ b/backends/trace-events
@@ -7,11 +7,11 @@ dbus_vmstate_loading(const char *id) "id: %s"
dbus_vmstate_saving(const char *id) "id: %s"
# iommufd.c
-iommufd_backend_connect(int fd, bool owned, uint32_t users, int ret) "fd=%d owned=%d users=%d (%d)"
+iommufd_backend_connect(int fd, bool owned, uint32_t users) "fd=%d owned=%d users=%d"
iommufd_backend_disconnect(int fd, uint32_t users) "fd=%d users=%d"
iommu_backend_set_fd(int fd) "pre-opened /dev/iommu fd=%d"
iommufd_backend_map_dma(int iommufd, uint32_t ioas, uint64_t iova, uint64_t size, void *vaddr, bool readonly, int ret) " iommufd=%d ioas=%d iova=0x%"PRIx64" size=0x%"PRIx64" addr=%p readonly=%d (%d)"
iommufd_backend_unmap_dma_non_exist(int iommufd, uint32_t ioas, uint64_t iova, uint64_t size, int ret) " Unmap nonexistent mapping: iommufd=%d ioas=%d iova=0x%"PRIx64" size=0x%"PRIx64" (%d)"
iommufd_backend_unmap_dma(int iommufd, uint32_t ioas, uint64_t iova, uint64_t size, int ret) " iommufd=%d ioas=%d iova=0x%"PRIx64" size=0x%"PRIx64" (%d)"
-iommufd_backend_alloc_ioas(int iommufd, uint32_t ioas, int ret) " iommufd=%d ioas=%d (%d)"
+iommufd_backend_alloc_ioas(int iommufd, uint32_t ioas) " iommufd=%d ioas=%d"
iommufd_backend_free_id(int iommufd, uint32_t id, int ret) " iommufd=%d id=%d (%d)"
--
2.34.1
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v2 11/11] backends/iommufd: Make iommufd_backend_*() return bool
2024-05-07 6:42 ` [PATCH v2 11/11] backends/iommufd: Make iommufd_backend_*() " Zhenzhong Duan
@ 2024-05-14 16:03 ` Cédric Le Goater
0 siblings, 0 replies; 25+ messages in thread
From: Cédric Le Goater @ 2024-05-14 16:03 UTC (permalink / raw)
To: Zhenzhong Duan, qemu-devel
Cc: alex.williamson, eric.auger, chao.p.peng, Yi Liu
On 5/7/24 08:42, Zhenzhong Duan wrote:
> This is to follow the coding standand to return bool if 'Error **'
> is used to pass error.
>
> The changed functions include:
>
> iommufd_backend_connect
> iommufd_backend_alloc_ioas
>
> By this chance, simplify the functions a bit by avoiding duplicate
> recordings, e.g., log through either error interface or trace, not
> both.
>
> 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/sysemu/iommufd.h | 6 +++---
> backends/iommufd.c | 29 +++++++++++++----------------
> hw/vfio/iommufd.c | 5 ++---
> backends/trace-events | 4 ++--
> 4 files changed, 20 insertions(+), 24 deletions(-)
>
> diff --git a/include/sysemu/iommufd.h b/include/sysemu/iommufd.h
> index 9af27ebd6c..293bfbe967 100644
> --- a/include/sysemu/iommufd.h
> +++ b/include/sysemu/iommufd.h
> @@ -23,11 +23,11 @@ struct IOMMUFDBackend {
> /*< public >*/
> };
>
> -int iommufd_backend_connect(IOMMUFDBackend *be, Error **errp);
> +bool iommufd_backend_connect(IOMMUFDBackend *be, Error **errp);
> void iommufd_backend_disconnect(IOMMUFDBackend *be);
>
> -int iommufd_backend_alloc_ioas(IOMMUFDBackend *be, uint32_t *ioas_id,
> - Error **errp);
> +bool iommufd_backend_alloc_ioas(IOMMUFDBackend *be, uint32_t *ioas_id,
> + Error **errp);
> void iommufd_backend_free_id(IOMMUFDBackend *be, uint32_t id);
> int iommufd_backend_map_dma(IOMMUFDBackend *be, uint32_t ioas_id, hwaddr iova,
> ram_addr_t size, void *vaddr, bool readonly);
> diff --git a/backends/iommufd.c b/backends/iommufd.c
> index 76a0204852..c506afbdac 100644
> --- a/backends/iommufd.c
> +++ b/backends/iommufd.c
> @@ -72,24 +72,22 @@ static void iommufd_backend_class_init(ObjectClass *oc, void *data)
> object_class_property_add_str(oc, "fd", NULL, iommufd_backend_set_fd);
> }
>
> -int iommufd_backend_connect(IOMMUFDBackend *be, Error **errp)
> +bool iommufd_backend_connect(IOMMUFDBackend *be, Error **errp)
> {
> - int fd, ret = 0;
> + int fd;
>
> if (be->owned && !be->users) {
> fd = qemu_open_old("/dev/iommu", O_RDWR);
> if (fd < 0) {
> error_setg_errno(errp, errno, "/dev/iommu opening failed");
> - ret = fd;
> - goto out;
> + return false;
> }
> be->fd = fd;
> }
> be->users++;
> -out:
> - trace_iommufd_backend_connect(be->fd, be->owned,
> - be->users, ret);
> - return ret;
> +
> + trace_iommufd_backend_connect(be->fd, be->owned, be->users);
> + return true;
> }
>
> void iommufd_backend_disconnect(IOMMUFDBackend *be)
> @@ -106,25 +104,24 @@ out:
> trace_iommufd_backend_disconnect(be->fd, be->users);
> }
>
> -int iommufd_backend_alloc_ioas(IOMMUFDBackend *be, uint32_t *ioas_id,
> - Error **errp)
> +bool iommufd_backend_alloc_ioas(IOMMUFDBackend *be, uint32_t *ioas_id,
> + Error **errp)
> {
> - int ret, fd = be->fd;
> + int fd = be->fd;
> struct iommu_ioas_alloc alloc_data = {
> .size = sizeof(alloc_data),
> .flags = 0,
> };
>
> - ret = ioctl(fd, IOMMU_IOAS_ALLOC, &alloc_data);
> - if (ret) {
> + if (ioctl(fd, IOMMU_IOAS_ALLOC, &alloc_data)) {
> error_setg_errno(errp, errno, "Failed to allocate ioas");
> - return ret;
> + return false;
> }
>
> *ioas_id = alloc_data.out_ioas_id;
> - trace_iommufd_backend_alloc_ioas(fd, *ioas_id, ret);
> + trace_iommufd_backend_alloc_ioas(fd, *ioas_id);
>
> - return ret;
> + return true;
> }
>
> void iommufd_backend_free_id(IOMMUFDBackend *be, uint32_t id)
> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> index 6a446b16dc..554f9a6292 100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -71,7 +71,7 @@ static bool iommufd_cdev_connect_and_bind(VFIODevice *vbasedev, Error **errp)
> .flags = 0,
> };
>
> - if (iommufd_backend_connect(iommufd, errp)) {
> + if (!iommufd_backend_connect(iommufd, errp)) {
> return false;
> }
>
> @@ -346,8 +346,7 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
> }
>
> /* Need to allocate a new dedicated container */
> - ret = iommufd_backend_alloc_ioas(vbasedev->iommufd, &ioas_id, errp);
> - if (ret < 0) {
> + if (!iommufd_backend_alloc_ioas(vbasedev->iommufd, &ioas_id, errp)) {
> goto err_alloc_ioas;
> }
>
> diff --git a/backends/trace-events b/backends/trace-events
> index d45c6e31a6..211e6f374a 100644
> --- a/backends/trace-events
> +++ b/backends/trace-events
> @@ -7,11 +7,11 @@ dbus_vmstate_loading(const char *id) "id: %s"
> dbus_vmstate_saving(const char *id) "id: %s"
>
> # iommufd.c
> -iommufd_backend_connect(int fd, bool owned, uint32_t users, int ret) "fd=%d owned=%d users=%d (%d)"
> +iommufd_backend_connect(int fd, bool owned, uint32_t users) "fd=%d owned=%d users=%d"
> iommufd_backend_disconnect(int fd, uint32_t users) "fd=%d users=%d"
> iommu_backend_set_fd(int fd) "pre-opened /dev/iommu fd=%d"
> iommufd_backend_map_dma(int iommufd, uint32_t ioas, uint64_t iova, uint64_t size, void *vaddr, bool readonly, int ret) " iommufd=%d ioas=%d iova=0x%"PRIx64" size=0x%"PRIx64" addr=%p readonly=%d (%d)"
> iommufd_backend_unmap_dma_non_exist(int iommufd, uint32_t ioas, uint64_t iova, uint64_t size, int ret) " Unmap nonexistent mapping: iommufd=%d ioas=%d iova=0x%"PRIx64" size=0x%"PRIx64" (%d)"
> iommufd_backend_unmap_dma(int iommufd, uint32_t ioas, uint64_t iova, uint64_t size, int ret) " iommufd=%d ioas=%d iova=0x%"PRIx64" size=0x%"PRIx64" (%d)"
> -iommufd_backend_alloc_ioas(int iommufd, uint32_t ioas, int ret) " iommufd=%d ioas=%d (%d)"
> +iommufd_backend_alloc_ioas(int iommufd, uint32_t ioas) " iommufd=%d ioas=%d"
> iommufd_backend_free_id(int iommufd, uint32_t id, int ret) " iommufd=%d id=%d (%d)"
^ permalink raw reply [flat|nested] 25+ messages in thread
* RE: [PATCH v2 00/11] VFIO: misc cleanups
2024-05-07 6:42 [PATCH v2 00/11] VFIO: misc cleanups Zhenzhong Duan
` (10 preceding siblings ...)
2024-05-07 6:42 ` [PATCH v2 11/11] backends/iommufd: Make iommufd_backend_*() " Zhenzhong Duan
@ 2024-05-14 3:46 ` Duan, Zhenzhong
2024-05-16 16:30 ` Cédric Le Goater
12 siblings, 0 replies; 25+ messages in thread
From: Duan, Zhenzhong @ 2024-05-14 3:46 UTC (permalink / raw)
To: qemu-devel@nongnu.org
Cc: alex.williamson@redhat.com, clg@redhat.com, eric.auger@redhat.com,
Peng, Chao P, Markus Armbruster
Hi All,
When I looked into more functions passing 'Error **',
I see many are in "int testfunc(..., Error **errp)" format. I was a bit confused.
The qapi/error.h suggests:
* - Whenever practical, also return a value that indicates success /
* failure. This can make the error checking more concise, and can
* avoid useless error object creation and destruction. Note that
* we still have many functions returning void. We recommend
* • bool-valued functions return true on success / false on failure,
* • pointer-valued functions return non-null / null pointer, and
* • integer-valued functions return non-negative / negative.
There are some functions like:
int testfunc(..., Error **errp)
{
If (succeed) {
return 0;
} else {
return -EINVAL;
}
}
Does testfunc() follow 'integer-valued functions' as above or it should be changed to 'bool-valued functions'?
Is there a clear rule in which case to change 'int testfunc(... Error **errp)' to ' bool testfunc(... Error **errp)'?
Thanks
Zhenzhong
>-----Original Message-----
>From: Duan, Zhenzhong <zhenzhong.duan@intel.com>
><zhenzhong.duan@intel.com>
>Subject: [PATCH v2 00/11] VFIO: misc cleanups
>
>Hi
>
>This is a cleanup series to change functions in hw/vfio/ to return bool
>when the error is passed through errp parameter, also some cleanup
>with g_autofree.
>
>See discussion at https://lists.gnu.org/archive/html/qemu-devel/2024-
>04/msg04782.html
>
>This series processed below files:
>hw/vfio/container.c
>hw/vfio/iommufd.c
>hw/vfio/cpr.c
>backends/iommufd.c
>
>So above files are clean now, there are still other files need processing
>in hw/vfio.
>
>Test done on x86 platform:
>vfio device hotplug/unplug with different backend
>reboot
>
>Thanks
>Zhenzhong
>
>Changelog:
>v2:
>- split out g_autofree code as a patch (Cédric)
>- add processing for more files
>
>Zhenzhong Duan (11):
> vfio/pci: Use g_autofree in vfio_realize
> vfio/pci: Use g_autofree in iommufd_cdev_get_info_iova_range()
> 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
> vfio/container: Make vfio_connect_container() return bool
> vfio/container: Make vfio_set_iommu() return bool
> vfio/container: Make vfio_get_device() return bool
> vfio/iommufd: Make iommufd_cdev_*() return bool
> vfio/cpr: Make vfio_cpr_register_container() return bool
> backends/iommufd: Make iommufd_backend_*() return bool
>
> include/hw/vfio/vfio-common.h | 6 +-
> include/hw/vfio/vfio-container-base.h | 18 ++---
> include/sysemu/iommufd.h | 6 +-
> backends/iommufd.c | 29 +++----
> 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 | 81 +++++++++----------
> hw/vfio/cpr.c | 4 +-
> hw/vfio/iommufd.c | 109 +++++++++++---------------
> hw/vfio/pci.c | 12 ++-
> hw/vfio/platform.c | 7 +-
> hw/vfio/spapr.c | 28 +++----
> backends/trace-events | 4 +-
> 15 files changed, 147 insertions(+), 183 deletions(-)
>
>--
>2.34.1
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v2 00/11] VFIO: misc cleanups
2024-05-07 6:42 [PATCH v2 00/11] VFIO: misc cleanups Zhenzhong Duan
` (11 preceding siblings ...)
2024-05-14 3:46 ` [PATCH v2 00/11] VFIO: misc cleanups Duan, Zhenzhong
@ 2024-05-16 16:30 ` Cédric Le Goater
12 siblings, 0 replies; 25+ messages in thread
From: Cédric Le Goater @ 2024-05-16 16:30 UTC (permalink / raw)
To: Zhenzhong Duan, qemu-devel; +Cc: alex.williamson, eric.auger, chao.p.peng
On 5/7/24 08:42, Zhenzhong Duan wrote:
> Hi
>
> This is a cleanup series to change functions in hw/vfio/ to return bool
> when the error is passed through errp parameter, also some cleanup
> with g_autofree.
>
> See discussion at https://lists.gnu.org/archive/html/qemu-devel/2024-04/msg04782.html
>
> This series processed below files:
> hw/vfio/container.c
> hw/vfio/iommufd.c
> hw/vfio/cpr.c
> backends/iommufd.c
>
> So above files are clean now, there are still other files need processing
> in hw/vfio.
>
> Test done on x86 platform:
> vfio device hotplug/unplug with different backend
> reboot
>
> Thanks
> Zhenzhong
>
> Changelog:
> v2:
> - split out g_autofree code as a patch (Cédric)
> - add processing for more files
>
> Zhenzhong Duan (11):
> vfio/pci: Use g_autofree in vfio_realize
> vfio/pci: Use g_autofree in iommufd_cdev_get_info_iova_range()
> 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
> vfio/container: Make vfio_connect_container() return bool
> vfio/container: Make vfio_set_iommu() return bool
> vfio/container: Make vfio_get_device() return bool
> vfio/iommufd: Make iommufd_cdev_*() return bool
> vfio/cpr: Make vfio_cpr_register_container() return bool
> backends/iommufd: Make iommufd_backend_*() return bool
Applied to vfio-next.
Thanks,
C.
^ permalink raw reply [flat|nested] 25+ messages in thread