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