* [PATCH v2 01/20] vfio/display: Fix error path in call site of ramfb_setup()
2024-05-22 4:39 [PATCH v2 00/20] VFIO: misc cleanups part2 Zhenzhong Duan
@ 2024-05-22 4:39 ` Zhenzhong Duan
2024-05-22 4:39 ` [PATCH v2 02/20] vfio/display: Make vfio_display_*() return bool Zhenzhong Duan
` (19 subsequent siblings)
20 siblings, 0 replies; 32+ messages in thread
From: Zhenzhong Duan @ 2024-05-22 4:39 UTC (permalink / raw)
To: qemu-devel
Cc: alex.williamson, clg, eric.auger, chao.p.peng, Zhenzhong Duan,
Gerd Hoffmann
vfio_display_dmabuf_init() and vfio_display_region_init() calls
ramfb_setup() without checking its return value.
So we may run into a situation that vfio_display_probe() succeed
but errp is set. This is risky and may lead to assert failure in
error_setv().
Cc: Gerd Hoffmann <kraxel@redhat.com>
Fixes: b290659fc3d ("hw/vfio/display: add ramfb support")
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
---
hw/vfio/display.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/hw/vfio/display.c b/hw/vfio/display.c
index fe624a6c9b..d28b724102 100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -361,6 +361,9 @@ static int vfio_display_dmabuf_init(VFIOPCIDevice *vdev, Error **errp)
vdev);
if (vdev->enable_ramfb) {
vdev->dpy->ramfb = ramfb_setup(errp);
+ if (!vdev->dpy->ramfb) {
+ return -EINVAL;
+ }
}
vfio_display_edid_init(vdev);
return 0;
@@ -488,6 +491,9 @@ static int vfio_display_region_init(VFIOPCIDevice *vdev, Error **errp)
vdev);
if (vdev->enable_ramfb) {
vdev->dpy->ramfb = ramfb_setup(errp);
+ if (!vdev->dpy->ramfb) {
+ return -EINVAL;
+ }
}
return 0;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH v2 02/20] vfio/display: Make vfio_display_*() return bool
2024-05-22 4:39 [PATCH v2 00/20] VFIO: misc cleanups part2 Zhenzhong Duan
2024-05-22 4:39 ` [PATCH v2 01/20] vfio/display: Fix error path in call site of ramfb_setup() Zhenzhong Duan
@ 2024-05-22 4:39 ` Zhenzhong Duan
2024-05-22 4:39 ` [PATCH v2 03/20] vfio/helpers: Use g_autofree in vfio_set_irq_signaling() Zhenzhong Duan
` (18 subsequent siblings)
20 siblings, 0 replies; 32+ messages in thread
From: Zhenzhong Duan @ 2024-05-22 4:39 UTC (permalink / raw)
To: qemu-devel; +Cc: alex.williamson, clg, eric.auger, chao.p.peng, Zhenzhong Duan
This is to follow the coding standand in qapi/error.h to return bool
for bool-valued functions.
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>
---
hw/vfio/pci.h | 2 +-
hw/vfio/display.c | 20 ++++++++++----------
hw/vfio/pci.c | 3 +--
3 files changed, 12 insertions(+), 13 deletions(-)
diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index 92cd62d115..a5ac9efd4b 100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -232,7 +232,7 @@ int vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev,
Error **errp);
void vfio_display_reset(VFIOPCIDevice *vdev);
-int vfio_display_probe(VFIOPCIDevice *vdev, Error **errp);
+bool vfio_display_probe(VFIOPCIDevice *vdev, Error **errp);
void vfio_display_finalize(VFIOPCIDevice *vdev);
extern const VMStateDescription vfio_display_vmstate;
diff --git a/hw/vfio/display.c b/hw/vfio/display.c
index d28b724102..661e921616 100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -348,11 +348,11 @@ static const GraphicHwOps vfio_display_dmabuf_ops = {
.ui_info = vfio_display_edid_ui_info,
};
-static int vfio_display_dmabuf_init(VFIOPCIDevice *vdev, Error **errp)
+static bool vfio_display_dmabuf_init(VFIOPCIDevice *vdev, Error **errp)
{
if (!display_opengl) {
error_setg(errp, "vfio-display-dmabuf: opengl not available");
- return -1;
+ return false;
}
vdev->dpy = g_new0(VFIODisplay, 1);
@@ -362,11 +362,11 @@ static int vfio_display_dmabuf_init(VFIOPCIDevice *vdev, Error **errp)
if (vdev->enable_ramfb) {
vdev->dpy->ramfb = ramfb_setup(errp);
if (!vdev->dpy->ramfb) {
- return -EINVAL;
+ return false;
}
}
vfio_display_edid_init(vdev);
- return 0;
+ return true;
}
static void vfio_display_dmabuf_exit(VFIODisplay *dpy)
@@ -483,7 +483,7 @@ static const GraphicHwOps vfio_display_region_ops = {
.gfx_update = vfio_display_region_update,
};
-static int vfio_display_region_init(VFIOPCIDevice *vdev, Error **errp)
+static bool vfio_display_region_init(VFIOPCIDevice *vdev, Error **errp)
{
vdev->dpy = g_new0(VFIODisplay, 1);
vdev->dpy->con = graphic_console_init(DEVICE(vdev), 0,
@@ -492,10 +492,10 @@ static int vfio_display_region_init(VFIOPCIDevice *vdev, Error **errp)
if (vdev->enable_ramfb) {
vdev->dpy->ramfb = ramfb_setup(errp);
if (!vdev->dpy->ramfb) {
- return -EINVAL;
+ return false;
}
}
- return 0;
+ return true;
}
static void vfio_display_region_exit(VFIODisplay *dpy)
@@ -510,7 +510,7 @@ static void vfio_display_region_exit(VFIODisplay *dpy)
/* ---------------------------------------------------------------------- */
-int vfio_display_probe(VFIOPCIDevice *vdev, Error **errp)
+bool vfio_display_probe(VFIOPCIDevice *vdev, Error **errp)
{
struct vfio_device_gfx_plane_info probe;
int ret;
@@ -533,11 +533,11 @@ int vfio_display_probe(VFIOPCIDevice *vdev, Error **errp)
if (vdev->display == ON_OFF_AUTO_AUTO) {
/* not an error in automatic mode */
- return 0;
+ return true;
}
error_setg(errp, "vfio: device doesn't support any (known) display method");
- return -1;
+ return false;
}
void vfio_display_finalize(VFIOPCIDevice *vdev)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index c1adef5cf8..a447013a1d 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3200,8 +3200,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
}
if (vdev->display != ON_OFF_AUTO_OFF) {
- ret = vfio_display_probe(vdev, errp);
- if (ret) {
+ if (!vfio_display_probe(vdev, errp)) {
goto out_deregister;
}
}
--
2.34.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH v2 03/20] vfio/helpers: Use g_autofree in vfio_set_irq_signaling()
2024-05-22 4:39 [PATCH v2 00/20] VFIO: misc cleanups part2 Zhenzhong Duan
2024-05-22 4:39 ` [PATCH v2 01/20] vfio/display: Fix error path in call site of ramfb_setup() Zhenzhong Duan
2024-05-22 4:39 ` [PATCH v2 02/20] vfio/display: Make vfio_display_*() return bool Zhenzhong Duan
@ 2024-05-22 4:39 ` Zhenzhong Duan
2024-05-22 5:59 ` Cédric Le Goater
2024-05-22 4:39 ` [PATCH v2 04/20] vfio/helpers: Make vfio_set_irq_signaling() return bool Zhenzhong Duan
` (17 subsequent siblings)
20 siblings, 1 reply; 32+ messages in thread
From: Zhenzhong Duan @ 2024-05-22 4:39 UTC (permalink / raw)
To: qemu-devel; +Cc: alex.williamson, clg, eric.auger, chao.p.peng, Zhenzhong Duan
Local pointer irq_set is freed before return from
vfio_set_irq_signaling().
Use 'g_autofree' to avoid the g_free() calls.
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
hw/vfio/helpers.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/hw/vfio/helpers.c b/hw/vfio/helpers.c
index 47b4096c05..1f3bdd9bf0 100644
--- a/hw/vfio/helpers.c
+++ b/hw/vfio/helpers.c
@@ -111,7 +111,7 @@ int vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex,
int action, int fd, Error **errp)
{
ERRP_GUARD();
- struct vfio_irq_set *irq_set;
+ g_autofree struct vfio_irq_set *irq_set = NULL;
int argsz, ret = 0;
const char *name;
int32_t *pfd;
@@ -130,7 +130,6 @@ int vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex,
if (ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, irq_set)) {
ret = -errno;
}
- g_free(irq_set);
if (!ret) {
return 0;
--
2.34.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH v2 03/20] vfio/helpers: Use g_autofree in vfio_set_irq_signaling()
2024-05-22 4:39 ` [PATCH v2 03/20] vfio/helpers: Use g_autofree in vfio_set_irq_signaling() Zhenzhong Duan
@ 2024-05-22 5:59 ` Cédric Le Goater
0 siblings, 0 replies; 32+ messages in thread
From: Cédric Le Goater @ 2024-05-22 5:59 UTC (permalink / raw)
To: Zhenzhong Duan, qemu-devel; +Cc: alex.williamson, eric.auger, chao.p.peng
On 5/22/24 06:39, Zhenzhong Duan wrote:
> Local pointer irq_set is freed before return from
> vfio_set_irq_signaling().
>
> 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/helpers.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/hw/vfio/helpers.c b/hw/vfio/helpers.c
> index 47b4096c05..1f3bdd9bf0 100644
> --- a/hw/vfio/helpers.c
> +++ b/hw/vfio/helpers.c
> @@ -111,7 +111,7 @@ int vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex,
> int action, int fd, Error **errp)
> {
> ERRP_GUARD();
> - struct vfio_irq_set *irq_set;
> + g_autofree struct vfio_irq_set *irq_set = NULL;
> int argsz, ret = 0;
> const char *name;
> int32_t *pfd;
> @@ -130,7 +130,6 @@ int vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex,
> if (ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, irq_set)) {
> ret = -errno;
> }
> - g_free(irq_set);
>
> if (!ret) {
> return 0;
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v2 04/20] vfio/helpers: Make vfio_set_irq_signaling() return bool
2024-05-22 4:39 [PATCH v2 00/20] VFIO: misc cleanups part2 Zhenzhong Duan
` (2 preceding siblings ...)
2024-05-22 4:39 ` [PATCH v2 03/20] vfio/helpers: Use g_autofree in vfio_set_irq_signaling() Zhenzhong Duan
@ 2024-05-22 4:39 ` Zhenzhong Duan
2024-05-22 4:40 ` [PATCH v2 05/20] vfio/helpers: Make vfio_device_get_name() " Zhenzhong Duan
` (16 subsequent siblings)
20 siblings, 0 replies; 32+ messages in thread
From: Zhenzhong Duan @ 2024-05-22 4:39 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
This is to follow the coding standand in qapi/error.h to return bool
for bool-valued functions.
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-common.h | 4 ++--
hw/vfio/ap.c | 8 +++----
hw/vfio/ccw.c | 8 +++----
hw/vfio/helpers.c | 18 ++++++----------
hw/vfio/pci.c | 40 ++++++++++++++++++-----------------
hw/vfio/platform.c | 18 +++++++---------
6 files changed, 46 insertions(+), 50 deletions(-)
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index b7bb4f5304..b712799caf 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -207,8 +207,8 @@ void vfio_spapr_container_deinit(VFIOContainer *container);
void vfio_disable_irqindex(VFIODevice *vbasedev, int index);
void vfio_unmask_single_irqindex(VFIODevice *vbasedev, int index);
void vfio_mask_single_irqindex(VFIODevice *vbasedev, int index);
-int vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex,
- int action, int fd, Error **errp);
+bool vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex,
+ int action, int fd, Error **errp);
void vfio_region_write(void *opaque, hwaddr addr,
uint64_t data, unsigned size);
uint64_t vfio_region_read(void *opaque,
diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
index ba653ef70f..d8a9615fee 100644
--- a/hw/vfio/ap.c
+++ b/hw/vfio/ap.c
@@ -117,8 +117,8 @@ static bool vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
fd = event_notifier_get_fd(notifier);
qemu_set_fd_handler(fd, fd_read, NULL, vapdev);
- if (vfio_set_irq_signaling(vdev, irq, 0, VFIO_IRQ_SET_ACTION_TRIGGER, fd,
- errp)) {
+ if (!vfio_set_irq_signaling(vdev, irq, 0, VFIO_IRQ_SET_ACTION_TRIGGER, fd,
+ errp)) {
qemu_set_fd_handler(fd, NULL, NULL, vapdev);
event_notifier_cleanup(notifier);
}
@@ -141,8 +141,8 @@ static void vfio_ap_unregister_irq_notifier(VFIOAPDevice *vapdev,
return;
}
- if (vfio_set_irq_signaling(&vapdev->vdev, irq, 0,
- VFIO_IRQ_SET_ACTION_TRIGGER, -1, &err)) {
+ if (!vfio_set_irq_signaling(&vapdev->vdev, irq, 0,
+ VFIO_IRQ_SET_ACTION_TRIGGER, -1, &err)) {
warn_reportf_err(err, VFIO_MSG_PREFIX, vapdev->vdev.name);
}
diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index 89bb980167..1f578a3c75 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -434,8 +434,8 @@ static bool vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev,
fd = event_notifier_get_fd(notifier);
qemu_set_fd_handler(fd, fd_read, NULL, vcdev);
- if (vfio_set_irq_signaling(vdev, irq, 0,
- VFIO_IRQ_SET_ACTION_TRIGGER, fd, errp)) {
+ if (!vfio_set_irq_signaling(vdev, irq, 0,
+ VFIO_IRQ_SET_ACTION_TRIGGER, fd, errp)) {
qemu_set_fd_handler(fd, NULL, NULL, vcdev);
event_notifier_cleanup(notifier);
}
@@ -464,8 +464,8 @@ static void vfio_ccw_unregister_irq_notifier(VFIOCCWDevice *vcdev,
return;
}
- if (vfio_set_irq_signaling(&vcdev->vdev, irq, 0,
- VFIO_IRQ_SET_ACTION_TRIGGER, -1, &err)) {
+ if (!vfio_set_irq_signaling(&vcdev->vdev, irq, 0,
+ VFIO_IRQ_SET_ACTION_TRIGGER, -1, &err)) {
warn_reportf_err(err, VFIO_MSG_PREFIX, vcdev->vdev.name);
}
diff --git a/hw/vfio/helpers.c b/hw/vfio/helpers.c
index 1f3bdd9bf0..9edbc96688 100644
--- a/hw/vfio/helpers.c
+++ b/hw/vfio/helpers.c
@@ -107,12 +107,12 @@ static const char *index_to_str(VFIODevice *vbasedev, int index)
}
}
-int vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex,
- int action, int fd, Error **errp)
+bool vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex,
+ int action, int fd, Error **errp)
{
ERRP_GUARD();
g_autofree struct vfio_irq_set *irq_set = NULL;
- int argsz, ret = 0;
+ int argsz;
const char *name;
int32_t *pfd;
@@ -127,15 +127,11 @@ int vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex,
pfd = (int32_t *)&irq_set->data;
*pfd = fd;
- if (ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, irq_set)) {
- ret = -errno;
+ if (!ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, irq_set)) {
+ return true;
}
- if (!ret) {
- return 0;
- }
-
- error_setg_errno(errp, -ret, "VFIO_DEVICE_SET_IRQS failure");
+ error_setg_errno(errp, errno, "VFIO_DEVICE_SET_IRQS failure");
name = index_to_str(vbasedev, index);
if (name) {
@@ -146,7 +142,7 @@ int vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex,
error_prepend(errp,
"Failed to %s %s eventfd signaling for interrupt ",
fd < 0 ? "tear down" : "set up", action_to_str(action));
- return ret;
+ return false;
}
/*
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index a447013a1d..358da4497b 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -147,10 +147,10 @@ static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp)
goto fail_irqfd;
}
- if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_INTX_IRQ_INDEX, 0,
- VFIO_IRQ_SET_ACTION_UNMASK,
- event_notifier_get_fd(&vdev->intx.unmask),
- errp)) {
+ if (!vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_INTX_IRQ_INDEX, 0,
+ VFIO_IRQ_SET_ACTION_UNMASK,
+ event_notifier_get_fd(&vdev->intx.unmask),
+ errp)) {
goto fail_vfio;
}
@@ -295,8 +295,8 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
fd = event_notifier_get_fd(&vdev->intx.interrupt);
qemu_set_fd_handler(fd, vfio_intx_interrupt, NULL, vdev);
- if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_INTX_IRQ_INDEX, 0,
- VFIO_IRQ_SET_ACTION_TRIGGER, fd, errp)) {
+ if (!vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_INTX_IRQ_INDEX, 0,
+ VFIO_IRQ_SET_ACTION_TRIGGER, fd, errp)) {
qemu_set_fd_handler(fd, NULL, NULL, vdev);
event_notifier_cleanup(&vdev->intx.interrupt);
return -errno;
@@ -590,9 +590,10 @@ static int vfio_msix_vector_do_use(PCIDevice *pdev, unsigned int nr,
fd = event_notifier_get_fd(&vector->interrupt);
}
- if (vfio_set_irq_signaling(&vdev->vbasedev,
- VFIO_PCI_MSIX_IRQ_INDEX, nr,
- VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
+ if (!vfio_set_irq_signaling(&vdev->vbasedev,
+ VFIO_PCI_MSIX_IRQ_INDEX, nr,
+ VFIO_IRQ_SET_ACTION_TRIGGER, fd,
+ &err)) {
error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
}
}
@@ -634,8 +635,9 @@ static void vfio_msix_vector_release(PCIDevice *pdev, unsigned int nr)
int32_t fd = event_notifier_get_fd(&vector->interrupt);
Error *err = NULL;
- if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX, nr,
- VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
+ if (!vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX,
+ nr, VFIO_IRQ_SET_ACTION_TRIGGER, fd,
+ &err)) {
error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
}
}
@@ -2873,8 +2875,8 @@ static void vfio_register_err_notifier(VFIOPCIDevice *vdev)
fd = event_notifier_get_fd(&vdev->err_notifier);
qemu_set_fd_handler(fd, vfio_err_notifier_handler, NULL, vdev);
- if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_ERR_IRQ_INDEX, 0,
- VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
+ if (!vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_ERR_IRQ_INDEX, 0,
+ VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
qemu_set_fd_handler(fd, NULL, NULL, vdev);
event_notifier_cleanup(&vdev->err_notifier);
@@ -2890,8 +2892,8 @@ static void vfio_unregister_err_notifier(VFIOPCIDevice *vdev)
return;
}
- if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_ERR_IRQ_INDEX, 0,
- VFIO_IRQ_SET_ACTION_TRIGGER, -1, &err)) {
+ if (!vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_ERR_IRQ_INDEX, 0,
+ VFIO_IRQ_SET_ACTION_TRIGGER, -1, &err)) {
error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
}
qemu_set_fd_handler(event_notifier_get_fd(&vdev->err_notifier),
@@ -2938,8 +2940,8 @@ static void vfio_register_req_notifier(VFIOPCIDevice *vdev)
fd = event_notifier_get_fd(&vdev->req_notifier);
qemu_set_fd_handler(fd, vfio_req_notifier_handler, NULL, vdev);
- if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_REQ_IRQ_INDEX, 0,
- VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
+ if (!vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_REQ_IRQ_INDEX, 0,
+ VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
qemu_set_fd_handler(fd, NULL, NULL, vdev);
event_notifier_cleanup(&vdev->req_notifier);
@@ -2956,8 +2958,8 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
return;
}
- if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_REQ_IRQ_INDEX, 0,
- VFIO_IRQ_SET_ACTION_TRIGGER, -1, &err)) {
+ if (!vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_REQ_IRQ_INDEX, 0,
+ VFIO_IRQ_SET_ACTION_TRIGGER, -1, &err)) {
error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
}
qemu_set_fd_handler(event_notifier_get_fd(&vdev->req_notifier),
diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index 2bd16096bb..3233ca8691 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -115,18 +115,17 @@ static int vfio_set_trigger_eventfd(VFIOINTp *intp,
VFIODevice *vbasedev = &intp->vdev->vbasedev;
int32_t fd = event_notifier_get_fd(intp->interrupt);
Error *err = NULL;
- int ret;
qemu_set_fd_handler(fd, (IOHandler *)handler, NULL, intp);
- ret = vfio_set_irq_signaling(vbasedev, intp->pin, 0,
- VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err);
- if (ret) {
+ if (!vfio_set_irq_signaling(vbasedev, intp->pin, 0,
+ VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
error_reportf_err(err, VFIO_MSG_PREFIX, vbasedev->name);
qemu_set_fd_handler(fd, NULL, NULL, NULL);
+ return -EINVAL;
}
- return ret;
+ return 0;
}
/*
@@ -355,15 +354,14 @@ static int vfio_set_resample_eventfd(VFIOINTp *intp)
int32_t fd = event_notifier_get_fd(intp->unmask);
VFIODevice *vbasedev = &intp->vdev->vbasedev;
Error *err = NULL;
- int ret;
qemu_set_fd_handler(fd, NULL, NULL, NULL);
- ret = vfio_set_irq_signaling(vbasedev, intp->pin, 0,
- VFIO_IRQ_SET_ACTION_UNMASK, fd, &err);
- if (ret) {
+ if (!vfio_set_irq_signaling(vbasedev, intp->pin, 0,
+ VFIO_IRQ_SET_ACTION_UNMASK, fd, &err)) {
error_reportf_err(err, VFIO_MSG_PREFIX, vbasedev->name);
+ return -EINVAL;
}
- return ret;
+ return 0;
}
/**
--
2.34.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH v2 05/20] vfio/helpers: Make vfio_device_get_name() return bool
2024-05-22 4:39 [PATCH v2 00/20] VFIO: misc cleanups part2 Zhenzhong Duan
` (3 preceding siblings ...)
2024-05-22 4:39 ` [PATCH v2 04/20] vfio/helpers: Make vfio_set_irq_signaling() return bool Zhenzhong Duan
@ 2024-05-22 4:40 ` Zhenzhong Duan
2024-05-22 4:40 ` [PATCH v2 06/20] vfio/platform: Make vfio_populate_device() and vfio_base_device_init() " Zhenzhong Duan
` (15 subsequent siblings)
20 siblings, 0 replies; 32+ messages in thread
From: Zhenzhong Duan @ 2024-05-22 4:40 UTC (permalink / raw)
To: qemu-devel
Cc: alex.williamson, clg, eric.auger, chao.p.peng, Zhenzhong Duan,
Thomas Huth, Tony Krowiak, Halil Pasic, Jason Herne, Eric Farman,
Matthew Rosato, open list:S390 general arch...
This is to follow the coding standand in qapi/error.h to return bool
for bool-valued functions.
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-common.h | 2 +-
hw/vfio/ap.c | 2 +-
hw/vfio/ccw.c | 2 +-
hw/vfio/helpers.c | 8 ++++----
hw/vfio/pci.c | 2 +-
hw/vfio/platform.c | 5 ++---
6 files changed, 10 insertions(+), 11 deletions(-)
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index b712799caf..4cb1ab8645 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -279,7 +279,7 @@ int vfio_get_dirty_bitmap(const VFIOContainerBase *bcontainer, uint64_t iova,
uint64_t size, ram_addr_t ram_addr, Error **errp);
/* Returns 0 on success, or a negative errno. */
-int vfio_device_get_name(VFIODevice *vbasedev, Error **errp);
+bool vfio_device_get_name(VFIODevice *vbasedev, Error **errp);
void vfio_device_set_fd(VFIODevice *vbasedev, const char *str, Error **errp);
void vfio_device_init(VFIODevice *vbasedev, int type, VFIODeviceOps *ops,
DeviceState *dev, bool ram_discard);
diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
index d8a9615fee..c12531a788 100644
--- a/hw/vfio/ap.c
+++ b/hw/vfio/ap.c
@@ -158,7 +158,7 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
VFIOAPDevice *vapdev = VFIO_AP_DEVICE(dev);
VFIODevice *vbasedev = &vapdev->vdev;
- if (vfio_device_get_name(vbasedev, errp) < 0) {
+ if (!vfio_device_get_name(vbasedev, errp)) {
return;
}
diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index 1f578a3c75..8850ca17c8 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -588,7 +588,7 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
}
}
- if (vfio_device_get_name(vbasedev, errp) < 0) {
+ if (!vfio_device_get_name(vbasedev, errp)) {
return;
}
diff --git a/hw/vfio/helpers.c b/hw/vfio/helpers.c
index 9edbc96688..4b079dc383 100644
--- a/hw/vfio/helpers.c
+++ b/hw/vfio/helpers.c
@@ -607,7 +607,7 @@ bool vfio_has_region_cap(VFIODevice *vbasedev, int region, uint16_t cap_type)
return ret;
}
-int vfio_device_get_name(VFIODevice *vbasedev, Error **errp)
+bool vfio_device_get_name(VFIODevice *vbasedev, Error **errp)
{
ERRP_GUARD();
struct stat st;
@@ -616,7 +616,7 @@ int vfio_device_get_name(VFIODevice *vbasedev, Error **errp)
if (stat(vbasedev->sysfsdev, &st) < 0) {
error_setg_errno(errp, errno, "no such host device");
error_prepend(errp, VFIO_MSG_PREFIX, vbasedev->sysfsdev);
- return -errno;
+ return false;
}
/* User may specify a name, e.g: VFIO platform device */
if (!vbasedev->name) {
@@ -625,7 +625,7 @@ int vfio_device_get_name(VFIODevice *vbasedev, Error **errp)
} else {
if (!vbasedev->iommufd) {
error_setg(errp, "Use FD passing only with iommufd backend");
- return -EINVAL;
+ return false;
}
/*
* Give a name with fd so any function printing out vbasedev->name
@@ -636,7 +636,7 @@ int vfio_device_get_name(VFIODevice *vbasedev, Error **errp)
}
}
- return 0;
+ return true;
}
void vfio_device_set_fd(VFIODevice *vbasedev, const char *str, Error **errp)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 358da4497b..aad012c348 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2999,7 +2999,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
vdev->host.slot, vdev->host.function);
}
- if (vfio_device_get_name(vbasedev, errp) < 0) {
+ if (!vfio_device_get_name(vbasedev, errp)) {
return;
}
diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index 3233ca8691..e1a32863d9 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -545,9 +545,8 @@ static int vfio_base_device_init(VFIODevice *vbasedev, Error **errp)
vbasedev->name);
}
- ret = vfio_device_get_name(vbasedev, errp);
- if (ret) {
- return ret;
+ if (!vfio_device_get_name(vbasedev, errp)) {
+ return -EINVAL;
}
if (!vfio_attach_device(vbasedev->name, vbasedev,
--
2.34.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH v2 06/20] vfio/platform: Make vfio_populate_device() and vfio_base_device_init() return bool
2024-05-22 4:39 [PATCH v2 00/20] VFIO: misc cleanups part2 Zhenzhong Duan
` (4 preceding siblings ...)
2024-05-22 4:40 ` [PATCH v2 05/20] vfio/helpers: Make vfio_device_get_name() " Zhenzhong Duan
@ 2024-05-22 4:40 ` Zhenzhong Duan
2024-05-22 4:40 ` [PATCH v2 07/20] vfio/ccw: Make vfio_ccw_get_region() return a bool Zhenzhong Duan
` (14 subsequent siblings)
20 siblings, 0 replies; 32+ messages in thread
From: Zhenzhong Duan @ 2024-05-22 4:40 UTC (permalink / raw)
To: qemu-devel; +Cc: alex.williamson, clg, eric.auger, chao.p.peng, Zhenzhong Duan
This is to follow the coding standand in qapi/error.h to return bool
for bool-valued functions.
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>
---
hw/vfio/platform.c | 40 +++++++++++++++++-----------------------
1 file changed, 17 insertions(+), 23 deletions(-)
diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index e1a32863d9..a85c199c76 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -441,7 +441,7 @@ static int vfio_platform_hot_reset_multi(VFIODevice *vbasedev)
* @errp: error object
*
*/
-static int vfio_populate_device(VFIODevice *vbasedev, Error **errp)
+static bool vfio_populate_device(VFIODevice *vbasedev, Error **errp)
{
VFIOINTp *intp, *tmp;
int i, ret = -1;
@@ -450,7 +450,7 @@ static int vfio_populate_device(VFIODevice *vbasedev, Error **errp)
if (!(vbasedev->flags & VFIO_DEVICE_FLAGS_PLATFORM)) {
error_setg(errp, "this isn't a platform device");
- return ret;
+ return false;
}
vdev->regions = g_new0(VFIORegion *, vbasedev->num_regions);
@@ -487,12 +487,11 @@ static int vfio_populate_device(VFIODevice *vbasedev, Error **errp)
irq.flags);
intp = vfio_init_intp(vbasedev, irq, errp);
if (!intp) {
- ret = -1;
goto irq_err;
}
}
}
- return 0;
+ return true;
irq_err:
timer_del(vdev->mmap_timer);
QLIST_FOREACH_SAFE(intp, &vdev->intp_list, next, tmp) {
@@ -507,7 +506,7 @@ reg_error:
g_free(vdev->regions[i]);
}
g_free(vdev->regions);
- return ret;
+ return false;
}
/* specialized functions for VFIO Platform devices */
@@ -527,10 +526,8 @@ static VFIODeviceOps vfio_platform_ops = {
* fd retrieval, resource query.
* Precondition: the device name must be initialized
*/
-static int vfio_base_device_init(VFIODevice *vbasedev, Error **errp)
+static bool vfio_base_device_init(VFIODevice *vbasedev, Error **errp)
{
- int ret;
-
/* @fd takes precedence over @sysfsdev which takes precedence over @host */
if (vbasedev->fd < 0 && vbasedev->sysfsdev) {
g_free(vbasedev->name);
@@ -538,7 +535,7 @@ static int vfio_base_device_init(VFIODevice *vbasedev, Error **errp)
} else if (vbasedev->fd < 0) {
if (!vbasedev->name || strchr(vbasedev->name, '/')) {
error_setg(errp, "wrong host device name");
- return -EINVAL;
+ return false;
}
vbasedev->sysfsdev = g_strdup_printf("/sys/bus/platform/devices/%s",
@@ -546,20 +543,20 @@ static int vfio_base_device_init(VFIODevice *vbasedev, Error **errp)
}
if (!vfio_device_get_name(vbasedev, errp)) {
- return -EINVAL;
+ return false;
}
if (!vfio_attach_device(vbasedev->name, vbasedev,
&address_space_memory, errp)) {
- return -EINVAL;
+ return false;
}
- ret = vfio_populate_device(vbasedev, errp);
- if (ret) {
- vfio_detach_device(vbasedev);
+ if (vfio_populate_device(vbasedev, errp)) {
+ return true;
}
- return ret;
+ vfio_detach_device(vbasedev);
+ return false;
}
/**
@@ -576,7 +573,7 @@ static void vfio_platform_realize(DeviceState *dev, Error **errp)
VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(dev);
SysBusDevice *sbdev = SYS_BUS_DEVICE(dev);
VFIODevice *vbasedev = &vdev->vbasedev;
- int i, ret;
+ int i;
qemu_mutex_init(&vdev->intp_mutex);
@@ -584,9 +581,8 @@ static void vfio_platform_realize(DeviceState *dev, Error **errp)
vbasedev->sysfsdev : vbasedev->name,
vdev->compat);
- ret = vfio_base_device_init(vbasedev, errp);
- if (ret) {
- goto out;
+ if (!vfio_base_device_init(vbasedev, errp)) {
+ goto init_err;
}
if (!vdev->compat) {
@@ -618,11 +614,9 @@ static void vfio_platform_realize(DeviceState *dev, Error **errp)
}
sysbus_init_mmio(sbdev, vdev->regions[i]->mem);
}
-out:
- if (!ret) {
- return;
- }
+ return;
+init_err:
if (vdev->vbasedev.name) {
error_prepend(errp, VFIO_MSG_PREFIX, vdev->vbasedev.name);
} else {
--
2.34.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH v2 07/20] vfio/ccw: Make vfio_ccw_get_region() return a bool
2024-05-22 4:39 [PATCH v2 00/20] VFIO: misc cleanups part2 Zhenzhong Duan
` (5 preceding siblings ...)
2024-05-22 4:40 ` [PATCH v2 06/20] vfio/platform: Make vfio_populate_device() and vfio_base_device_init() " Zhenzhong Duan
@ 2024-05-22 4:40 ` Zhenzhong Duan
2024-05-22 4:40 ` [PATCH v2 08/20] vfio/pci: Make vfio_intx_enable_kvm() " Zhenzhong Duan
` (13 subsequent siblings)
20 siblings, 0 replies; 32+ messages in thread
From: Zhenzhong Duan @ 2024-05-22 4:40 UTC (permalink / raw)
To: qemu-devel
Cc: alex.williamson, clg, eric.auger, chao.p.peng, Zhenzhong Duan,
Eric Farman, Matthew Rosato, Thomas Huth, open list:vfio-ccw
Since vfio_populate_device() takes an 'Error **' argument,
best practices suggest to return a bool. See the qapi/error.h
Rules section.
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
---
hw/vfio/ccw.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index 8850ca17c8..2600e62e37 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -474,7 +474,7 @@ static void vfio_ccw_unregister_irq_notifier(VFIOCCWDevice *vcdev,
event_notifier_cleanup(notifier);
}
-static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error **errp)
+static bool vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error **errp)
{
VFIODevice *vdev = &vcdev->vdev;
struct vfio_region_info *info;
@@ -483,7 +483,7 @@ static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error **errp)
/* Sanity check device */
if (!(vdev->flags & VFIO_DEVICE_FLAGS_CCW)) {
error_setg(errp, "vfio: Um, this isn't a vfio-ccw device");
- return;
+ return false;
}
/*
@@ -493,13 +493,13 @@ static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error **errp)
if (vdev->num_regions < VFIO_CCW_CONFIG_REGION_INDEX + 1) {
error_setg(errp, "vfio: too few regions (%u), expected at least %u",
vdev->num_regions, VFIO_CCW_CONFIG_REGION_INDEX + 1);
- return;
+ return false;
}
ret = vfio_get_region_info(vdev, VFIO_CCW_CONFIG_REGION_INDEX, &info);
if (ret) {
error_setg_errno(errp, -ret, "vfio: Error getting config info");
- return;
+ return false;
}
vcdev->io_region_size = info->size;
@@ -553,7 +553,7 @@ static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error **errp)
g_free(info);
}
- return;
+ return true;
out_err:
g_free(vcdev->crw_region);
@@ -561,7 +561,7 @@ out_err:
g_free(vcdev->async_cmd_region);
g_free(vcdev->io_region);
g_free(info);
- return;
+ return false;
}
static void vfio_ccw_put_region(VFIOCCWDevice *vcdev)
@@ -597,8 +597,7 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
goto out_attach_dev_err;
}
- vfio_ccw_get_region(vcdev, &err);
- if (err) {
+ if (!vfio_ccw_get_region(vcdev, &err)) {
goto out_region_err;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH v2 08/20] vfio/pci: Make vfio_intx_enable_kvm() return a bool
2024-05-22 4:39 [PATCH v2 00/20] VFIO: misc cleanups part2 Zhenzhong Duan
` (6 preceding siblings ...)
2024-05-22 4:40 ` [PATCH v2 07/20] vfio/ccw: Make vfio_ccw_get_region() return a bool Zhenzhong Duan
@ 2024-05-22 4:40 ` Zhenzhong Duan
2024-05-22 4:40 ` [PATCH v2 09/20] vfio/pci: Make vfio_pci_relocate_msix() and vfio_msix_early_setup() " Zhenzhong Duan
` (12 subsequent siblings)
20 siblings, 0 replies; 32+ messages in thread
From: Zhenzhong Duan @ 2024-05-22 4:40 UTC (permalink / raw)
To: qemu-devel; +Cc: alex.williamson, clg, eric.auger, chao.p.peng, Zhenzhong Duan
Since vfio_intx_enable_kvm() takes an 'Error **' argument,
best practices suggest to return a bool. See the qapi/error.h
Rules section.
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
---
hw/vfio/pci.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index aad012c348..12fb534d79 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -116,7 +116,7 @@ static void vfio_intx_eoi(VFIODevice *vbasedev)
vfio_unmask_single_irqindex(vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
}
-static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp)
+static bool vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp)
{
#ifdef CONFIG_KVM
int irq_fd = event_notifier_get_fd(&vdev->intx.interrupt);
@@ -124,7 +124,7 @@ static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp)
if (vdev->no_kvm_intx || !kvm_irqfds_enabled() ||
vdev->intx.route.mode != PCI_INTX_ENABLED ||
!kvm_resamplefds_enabled()) {
- return;
+ return true;
}
/* Get to a known interrupt state */
@@ -161,7 +161,7 @@ static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp)
trace_vfio_intx_enable_kvm(vdev->vbasedev.name);
- return;
+ return true;
fail_vfio:
kvm_irqchip_remove_irqfd_notifier_gsi(kvm_state, &vdev->intx.interrupt,
@@ -171,6 +171,9 @@ fail_irqfd:
fail:
qemu_set_fd_handler(irq_fd, vfio_intx_interrupt, NULL, vdev);
vfio_unmask_single_irqindex(&vdev->vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
+ return false;
+#else
+ return true;
#endif
}
@@ -226,8 +229,7 @@ static void vfio_intx_update(VFIOPCIDevice *vdev, PCIINTxRoute *route)
return;
}
- vfio_intx_enable_kvm(vdev, &err);
- if (err) {
+ if (!vfio_intx_enable_kvm(vdev, &err)) {
warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
}
@@ -302,8 +304,7 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
return -errno;
}
- vfio_intx_enable_kvm(vdev, &err);
- if (err) {
+ if (!vfio_intx_enable_kvm(vdev, &err)) {
warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH v2 09/20] vfio/pci: Make vfio_pci_relocate_msix() and vfio_msix_early_setup() return a bool
2024-05-22 4:39 [PATCH v2 00/20] VFIO: misc cleanups part2 Zhenzhong Duan
` (7 preceding siblings ...)
2024-05-22 4:40 ` [PATCH v2 08/20] vfio/pci: Make vfio_intx_enable_kvm() " Zhenzhong Duan
@ 2024-05-22 4:40 ` Zhenzhong Duan
2024-05-22 6:06 ` Cédric Le Goater
2024-05-22 4:40 ` [PATCH v2 10/20] vfio/pci: Make vfio_populate_device() " Zhenzhong Duan
` (11 subsequent siblings)
20 siblings, 1 reply; 32+ messages in thread
From: Zhenzhong Duan @ 2024-05-22 4:40 UTC (permalink / raw)
To: qemu-devel; +Cc: alex.williamson, clg, eric.auger, chao.p.peng, Zhenzhong Duan
Since vfio_pci_relocate_msix() and vfio_msix_early_setup() takes
an 'Error **' argument, best practices suggest to return a bool.
See the qapi/error.h Rules section.
By this chance, pass errp directly to vfio_msix_early_setup() to avoid
calling error_propagate().
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
hw/vfio/pci.c | 33 ++++++++++++++++-----------------
1 file changed, 16 insertions(+), 17 deletions(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 12fb534d79..4fb5fd0c9f 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1450,13 +1450,13 @@ static void vfio_pci_fixup_msix_region(VFIOPCIDevice *vdev)
}
}
-static void vfio_pci_relocate_msix(VFIOPCIDevice *vdev, Error **errp)
+static bool vfio_pci_relocate_msix(VFIOPCIDevice *vdev, Error **errp)
{
int target_bar = -1;
size_t msix_sz;
if (!vdev->msix || vdev->msix_relo == OFF_AUTOPCIBAR_OFF) {
- return;
+ return true;
}
/* The actual minimum size of MSI-X structures */
@@ -1479,7 +1479,7 @@ static void vfio_pci_relocate_msix(VFIOPCIDevice *vdev, Error **errp)
if (target_bar < 0) {
error_setg(errp, "No automatic MSI-X relocation available for "
"device %04x:%04x", vdev->vendor_id, vdev->device_id);
- return;
+ return false;
}
} else {
target_bar = (int)(vdev->msix_relo - OFF_AUTOPCIBAR_BAR0);
@@ -1489,7 +1489,7 @@ static void vfio_pci_relocate_msix(VFIOPCIDevice *vdev, Error **errp)
if (vdev->bars[target_bar].ioport) {
error_setg(errp, "Invalid MSI-X relocation BAR %d, "
"I/O port BAR", target_bar);
- return;
+ return false;
}
/* Cannot use a BAR in the "shadow" of a 64-bit BAR */
@@ -1497,7 +1497,7 @@ static void vfio_pci_relocate_msix(VFIOPCIDevice *vdev, Error **errp)
target_bar > 0 && vdev->bars[target_bar - 1].mem64) {
error_setg(errp, "Invalid MSI-X relocation BAR %d, "
"consumed by 64-bit BAR %d", target_bar, target_bar - 1);
- return;
+ return false;
}
/* 2GB max size for 32-bit BARs, cannot double if already > 1G */
@@ -1505,7 +1505,7 @@ static void vfio_pci_relocate_msix(VFIOPCIDevice *vdev, Error **errp)
!vdev->bars[target_bar].mem64) {
error_setg(errp, "Invalid MSI-X relocation BAR %d, "
"no space to extend 32-bit BAR", target_bar);
- return;
+ return false;
}
/*
@@ -1540,6 +1540,7 @@ static void vfio_pci_relocate_msix(VFIOPCIDevice *vdev, Error **errp)
trace_vfio_msix_relo(vdev->vbasedev.name,
vdev->msix->table_bar, vdev->msix->table_offset);
+ return true;
}
/*
@@ -1550,7 +1551,7 @@ static void vfio_pci_relocate_msix(VFIOPCIDevice *vdev, Error **errp)
* need to first look for where the MSI-X table lives. So we
* unfortunately split MSI-X setup across two functions.
*/
-static void vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
+static bool vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
{
uint8_t pos;
uint16_t ctrl;
@@ -1562,25 +1563,25 @@ static void vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
pos = pci_find_capability(&vdev->pdev, PCI_CAP_ID_MSIX);
if (!pos) {
- return;
+ return true;
}
if (pread(fd, &ctrl, sizeof(ctrl),
vdev->config_offset + pos + PCI_MSIX_FLAGS) != sizeof(ctrl)) {
error_setg_errno(errp, errno, "failed to read PCI MSIX FLAGS");
- return;
+ return false;
}
if (pread(fd, &table, sizeof(table),
vdev->config_offset + pos + PCI_MSIX_TABLE) != sizeof(table)) {
error_setg_errno(errp, errno, "failed to read PCI MSIX TABLE");
- return;
+ return false;
}
if (pread(fd, &pba, sizeof(pba),
vdev->config_offset + pos + PCI_MSIX_PBA) != sizeof(pba)) {
error_setg_errno(errp, errno, "failed to read PCI MSIX PBA");
- return;
+ return false;
}
ctrl = le16_to_cpu(ctrl);
@@ -1598,7 +1599,7 @@ static void vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
if (ret < 0) {
error_setg_errno(errp, -ret, "failed to get MSI-X irq info");
g_free(msix);
- return;
+ return false;
}
msix->noresize = !!(irq_info.flags & VFIO_IRQ_INFO_NORESIZE);
@@ -1630,7 +1631,7 @@ static void vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
error_setg(errp, "hardware reports invalid configuration, "
"MSIX PBA outside of specified BAR");
g_free(msix);
- return;
+ return false;
}
}
@@ -1641,7 +1642,7 @@ static void vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
vfio_pci_fixup_msix_region(vdev);
- vfio_pci_relocate_msix(vdev, errp);
+ return vfio_pci_relocate_msix(vdev, errp);
}
static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
@@ -3130,9 +3131,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
vfio_bars_prepare(vdev);
- vfio_msix_early_setup(vdev, &err);
- if (err) {
- error_propagate(errp, err);
+ if (!vfio_msix_early_setup(vdev, errp)) {
goto error;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH v2 09/20] vfio/pci: Make vfio_pci_relocate_msix() and vfio_msix_early_setup() return a bool
2024-05-22 4:40 ` [PATCH v2 09/20] vfio/pci: Make vfio_pci_relocate_msix() and vfio_msix_early_setup() " Zhenzhong Duan
@ 2024-05-22 6:06 ` Cédric Le Goater
0 siblings, 0 replies; 32+ messages in thread
From: Cédric Le Goater @ 2024-05-22 6:06 UTC (permalink / raw)
To: Zhenzhong Duan, qemu-devel; +Cc: alex.williamson, eric.auger, chao.p.peng
On 5/22/24 06:40, Zhenzhong Duan wrote:
> Since vfio_pci_relocate_msix() and vfio_msix_early_setup() takes
> an 'Error **' argument, best practices suggest to return a bool.
> See the qapi/error.h Rules section.
>
> By this chance, pass errp directly to vfio_msix_early_setup() to avoid
> calling error_propagate().
>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
Thanks,
C.
> ---
> hw/vfio/pci.c | 33 ++++++++++++++++-----------------
> 1 file changed, 16 insertions(+), 17 deletions(-)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 12fb534d79..4fb5fd0c9f 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -1450,13 +1450,13 @@ static void vfio_pci_fixup_msix_region(VFIOPCIDevice *vdev)
> }
> }
>
> -static void vfio_pci_relocate_msix(VFIOPCIDevice *vdev, Error **errp)
> +static bool vfio_pci_relocate_msix(VFIOPCIDevice *vdev, Error **errp)
> {
> int target_bar = -1;
> size_t msix_sz;
>
> if (!vdev->msix || vdev->msix_relo == OFF_AUTOPCIBAR_OFF) {
> - return;
> + return true;
> }
>
> /* The actual minimum size of MSI-X structures */
> @@ -1479,7 +1479,7 @@ static void vfio_pci_relocate_msix(VFIOPCIDevice *vdev, Error **errp)
> if (target_bar < 0) {
> error_setg(errp, "No automatic MSI-X relocation available for "
> "device %04x:%04x", vdev->vendor_id, vdev->device_id);
> - return;
> + return false;
> }
> } else {
> target_bar = (int)(vdev->msix_relo - OFF_AUTOPCIBAR_BAR0);
> @@ -1489,7 +1489,7 @@ static void vfio_pci_relocate_msix(VFIOPCIDevice *vdev, Error **errp)
> if (vdev->bars[target_bar].ioport) {
> error_setg(errp, "Invalid MSI-X relocation BAR %d, "
> "I/O port BAR", target_bar);
> - return;
> + return false;
> }
>
> /* Cannot use a BAR in the "shadow" of a 64-bit BAR */
> @@ -1497,7 +1497,7 @@ static void vfio_pci_relocate_msix(VFIOPCIDevice *vdev, Error **errp)
> target_bar > 0 && vdev->bars[target_bar - 1].mem64) {
> error_setg(errp, "Invalid MSI-X relocation BAR %d, "
> "consumed by 64-bit BAR %d", target_bar, target_bar - 1);
> - return;
> + return false;
> }
>
> /* 2GB max size for 32-bit BARs, cannot double if already > 1G */
> @@ -1505,7 +1505,7 @@ static void vfio_pci_relocate_msix(VFIOPCIDevice *vdev, Error **errp)
> !vdev->bars[target_bar].mem64) {
> error_setg(errp, "Invalid MSI-X relocation BAR %d, "
> "no space to extend 32-bit BAR", target_bar);
> - return;
> + return false;
> }
>
> /*
> @@ -1540,6 +1540,7 @@ static void vfio_pci_relocate_msix(VFIOPCIDevice *vdev, Error **errp)
>
> trace_vfio_msix_relo(vdev->vbasedev.name,
> vdev->msix->table_bar, vdev->msix->table_offset);
> + return true;
> }
>
> /*
> @@ -1550,7 +1551,7 @@ static void vfio_pci_relocate_msix(VFIOPCIDevice *vdev, Error **errp)
> * need to first look for where the MSI-X table lives. So we
> * unfortunately split MSI-X setup across two functions.
> */
> -static void vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
> +static bool vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
> {
> uint8_t pos;
> uint16_t ctrl;
> @@ -1562,25 +1563,25 @@ static void vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
>
> pos = pci_find_capability(&vdev->pdev, PCI_CAP_ID_MSIX);
> if (!pos) {
> - return;
> + return true;
> }
>
> if (pread(fd, &ctrl, sizeof(ctrl),
> vdev->config_offset + pos + PCI_MSIX_FLAGS) != sizeof(ctrl)) {
> error_setg_errno(errp, errno, "failed to read PCI MSIX FLAGS");
> - return;
> + return false;
> }
>
> if (pread(fd, &table, sizeof(table),
> vdev->config_offset + pos + PCI_MSIX_TABLE) != sizeof(table)) {
> error_setg_errno(errp, errno, "failed to read PCI MSIX TABLE");
> - return;
> + return false;
> }
>
> if (pread(fd, &pba, sizeof(pba),
> vdev->config_offset + pos + PCI_MSIX_PBA) != sizeof(pba)) {
> error_setg_errno(errp, errno, "failed to read PCI MSIX PBA");
> - return;
> + return false;
> }
>
> ctrl = le16_to_cpu(ctrl);
> @@ -1598,7 +1599,7 @@ static void vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
> if (ret < 0) {
> error_setg_errno(errp, -ret, "failed to get MSI-X irq info");
> g_free(msix);
> - return;
> + return false;
> }
>
> msix->noresize = !!(irq_info.flags & VFIO_IRQ_INFO_NORESIZE);
> @@ -1630,7 +1631,7 @@ static void vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
> error_setg(errp, "hardware reports invalid configuration, "
> "MSIX PBA outside of specified BAR");
> g_free(msix);
> - return;
> + return false;
> }
> }
>
> @@ -1641,7 +1642,7 @@ static void vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
>
> vfio_pci_fixup_msix_region(vdev);
>
> - vfio_pci_relocate_msix(vdev, errp);
> + return vfio_pci_relocate_msix(vdev, errp);
> }
>
> static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
> @@ -3130,9 +3131,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>
> vfio_bars_prepare(vdev);
>
> - vfio_msix_early_setup(vdev, &err);
> - if (err) {
> - error_propagate(errp, err);
> + if (!vfio_msix_early_setup(vdev, errp)) {
> goto error;
> }
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v2 10/20] vfio/pci: Make vfio_populate_device() return a bool
2024-05-22 4:39 [PATCH v2 00/20] VFIO: misc cleanups part2 Zhenzhong Duan
` (8 preceding siblings ...)
2024-05-22 4:40 ` [PATCH v2 09/20] vfio/pci: Make vfio_pci_relocate_msix() and vfio_msix_early_setup() " Zhenzhong Duan
@ 2024-05-22 4:40 ` Zhenzhong Duan
2024-05-22 6:06 ` Cédric Le Goater
2024-05-22 4:40 ` [PATCH v2 11/20] vfio/pci: Make vfio_intx_enable() return bool Zhenzhong Duan
` (10 subsequent siblings)
20 siblings, 1 reply; 32+ messages in thread
From: Zhenzhong Duan @ 2024-05-22 4:40 UTC (permalink / raw)
To: qemu-devel; +Cc: alex.williamson, clg, eric.auger, chao.p.peng, Zhenzhong Duan
Since vfio_populate_device() takes an 'Error **' argument,
best practices suggest to return a bool. See the qapi/error.h
Rules section.
By this chance, pass errp directly to vfio_populate_device() to
avoid calling error_propagate().
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
hw/vfio/pci.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 4fb5fd0c9f..46d3c61859 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2740,7 +2740,7 @@ int vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp)
return 0;
}
-static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
+static bool vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
{
VFIODevice *vbasedev = &vdev->vbasedev;
struct vfio_region_info *reg_info;
@@ -2750,18 +2750,18 @@ static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
/* Sanity check device */
if (!(vbasedev->flags & VFIO_DEVICE_FLAGS_PCI)) {
error_setg(errp, "this isn't a PCI device");
- return;
+ return false;
}
if (vbasedev->num_regions < VFIO_PCI_CONFIG_REGION_INDEX + 1) {
error_setg(errp, "unexpected number of io regions %u",
vbasedev->num_regions);
- return;
+ return false;
}
if (vbasedev->num_irqs < VFIO_PCI_MSIX_IRQ_INDEX + 1) {
error_setg(errp, "unexpected number of irqs %u", vbasedev->num_irqs);
- return;
+ return false;
}
for (i = VFIO_PCI_BAR0_REGION_INDEX; i < VFIO_PCI_ROM_REGION_INDEX; i++) {
@@ -2773,7 +2773,7 @@ static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
if (ret) {
error_setg_errno(errp, -ret, "failed to get region %d info", i);
- return;
+ return false;
}
QLIST_INIT(&vdev->bars[i].quirks);
@@ -2783,7 +2783,7 @@ static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
VFIO_PCI_CONFIG_REGION_INDEX, ®_info);
if (ret) {
error_setg_errno(errp, -ret, "failed to get config info");
- return;
+ return false;
}
trace_vfio_populate_device_config(vdev->vbasedev.name,
@@ -2804,7 +2804,7 @@ static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
if (ret) {
error_append_hint(errp, "device does not support "
"requested feature x-vga\n");
- return;
+ return false;
}
}
@@ -2821,6 +2821,8 @@ static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
"Could not enable error recovery for the device",
vbasedev->name);
}
+
+ return true;
}
static void vfio_pci_put_device(VFIOPCIDevice *vdev)
@@ -2977,7 +2979,6 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
VFIOPCIDevice *vdev = VFIO_PCI(pdev);
VFIODevice *vbasedev = &vdev->vbasedev;
char *subsys;
- Error *err = NULL;
int i, ret;
bool is_mdev;
char uuid[UUID_STR_LEN];
@@ -3036,9 +3037,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
goto error;
}
- vfio_populate_device(vdev, &err);
- if (err) {
- error_propagate(errp, err);
+ if (!vfio_populate_device(vdev, errp)) {
goto error;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH v2 10/20] vfio/pci: Make vfio_populate_device() return a bool
2024-05-22 4:40 ` [PATCH v2 10/20] vfio/pci: Make vfio_populate_device() " Zhenzhong Duan
@ 2024-05-22 6:06 ` Cédric Le Goater
0 siblings, 0 replies; 32+ messages in thread
From: Cédric Le Goater @ 2024-05-22 6:06 UTC (permalink / raw)
To: Zhenzhong Duan, qemu-devel; +Cc: alex.williamson, eric.auger, chao.p.peng
On 5/22/24 06:40, Zhenzhong Duan wrote:
> Since vfio_populate_device() takes an 'Error **' argument,
> best practices suggest to return a bool. See the qapi/error.h
> Rules section.
>
> By this chance, pass errp directly to vfio_populate_device() to
> avoid calling error_propagate().
>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
Thanks,
C.
> ---
> hw/vfio/pci.c | 21 ++++++++++-----------
> 1 file changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 4fb5fd0c9f..46d3c61859 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2740,7 +2740,7 @@ int vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp)
> return 0;
> }
>
> -static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
> +static bool vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
> {
> VFIODevice *vbasedev = &vdev->vbasedev;
> struct vfio_region_info *reg_info;
> @@ -2750,18 +2750,18 @@ static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
> /* Sanity check device */
> if (!(vbasedev->flags & VFIO_DEVICE_FLAGS_PCI)) {
> error_setg(errp, "this isn't a PCI device");
> - return;
> + return false;
> }
>
> if (vbasedev->num_regions < VFIO_PCI_CONFIG_REGION_INDEX + 1) {
> error_setg(errp, "unexpected number of io regions %u",
> vbasedev->num_regions);
> - return;
> + return false;
> }
>
> if (vbasedev->num_irqs < VFIO_PCI_MSIX_IRQ_INDEX + 1) {
> error_setg(errp, "unexpected number of irqs %u", vbasedev->num_irqs);
> - return;
> + return false;
> }
>
> for (i = VFIO_PCI_BAR0_REGION_INDEX; i < VFIO_PCI_ROM_REGION_INDEX; i++) {
> @@ -2773,7 +2773,7 @@ static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
>
> if (ret) {
> error_setg_errno(errp, -ret, "failed to get region %d info", i);
> - return;
> + return false;
> }
>
> QLIST_INIT(&vdev->bars[i].quirks);
> @@ -2783,7 +2783,7 @@ static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
> VFIO_PCI_CONFIG_REGION_INDEX, ®_info);
> if (ret) {
> error_setg_errno(errp, -ret, "failed to get config info");
> - return;
> + return false;
> }
>
> trace_vfio_populate_device_config(vdev->vbasedev.name,
> @@ -2804,7 +2804,7 @@ static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
> if (ret) {
> error_append_hint(errp, "device does not support "
> "requested feature x-vga\n");
> - return;
> + return false;
> }
> }
>
> @@ -2821,6 +2821,8 @@ static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
> "Could not enable error recovery for the device",
> vbasedev->name);
> }
> +
> + return true;
> }
>
> static void vfio_pci_put_device(VFIOPCIDevice *vdev)
> @@ -2977,7 +2979,6 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
> VFIOPCIDevice *vdev = VFIO_PCI(pdev);
> VFIODevice *vbasedev = &vdev->vbasedev;
> char *subsys;
> - Error *err = NULL;
> int i, ret;
> bool is_mdev;
> char uuid[UUID_STR_LEN];
> @@ -3036,9 +3037,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
> goto error;
> }
>
> - vfio_populate_device(vdev, &err);
> - if (err) {
> - error_propagate(errp, err);
> + if (!vfio_populate_device(vdev, errp)) {
> goto error;
> }
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v2 11/20] vfio/pci: Make vfio_intx_enable() return bool
2024-05-22 4:39 [PATCH v2 00/20] VFIO: misc cleanups part2 Zhenzhong Duan
` (9 preceding siblings ...)
2024-05-22 4:40 ` [PATCH v2 10/20] vfio/pci: Make vfio_populate_device() " Zhenzhong Duan
@ 2024-05-22 4:40 ` Zhenzhong Duan
2024-05-22 4:40 ` [PATCH v2 12/20] vfio/pci: Make vfio_populate_vga() " Zhenzhong Duan
` (9 subsequent siblings)
20 siblings, 0 replies; 32+ messages in thread
From: Zhenzhong Duan @ 2024-05-22 4:40 UTC (permalink / raw)
To: qemu-devel; +Cc: alex.williamson, clg, eric.auger, chao.p.peng, Zhenzhong Duan
This is to follow the coding standand in qapi/error.h to return bool
for bool-valued functions.
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>
---
hw/vfio/pci.c | 19 ++++++++-----------
1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 46d3c61859..7f35cb8a29 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -261,7 +261,7 @@ static void vfio_irqchip_change(Notifier *notify, void *data)
vfio_intx_update(vdev, &vdev->intx.route);
}
-static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
+static bool vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
{
uint8_t pin = vfio_pci_read_config(&vdev->pdev, PCI_INTERRUPT_PIN, 1);
Error *err = NULL;
@@ -270,7 +270,7 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
if (!pin) {
- return 0;
+ return true;
}
vfio_disable_interrupts(vdev);
@@ -292,7 +292,7 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
ret = event_notifier_init(&vdev->intx.interrupt, 0);
if (ret) {
error_setg_errno(errp, -ret, "event_notifier_init failed");
- return ret;
+ return false;
}
fd = event_notifier_get_fd(&vdev->intx.interrupt);
qemu_set_fd_handler(fd, vfio_intx_interrupt, NULL, vdev);
@@ -301,7 +301,7 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
VFIO_IRQ_SET_ACTION_TRIGGER, fd, errp)) {
qemu_set_fd_handler(fd, NULL, NULL, vdev);
event_notifier_cleanup(&vdev->intx.interrupt);
- return -errno;
+ return false;
}
if (!vfio_intx_enable_kvm(vdev, &err)) {
@@ -311,7 +311,7 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
vdev->interrupt = VFIO_INT_INTx;
trace_vfio_intx_enable(vdev->vbasedev.name);
- return 0;
+ return true;
}
static void vfio_intx_disable(VFIOPCIDevice *vdev)
@@ -836,8 +836,7 @@ static void vfio_msix_disable(VFIOPCIDevice *vdev)
vfio_disable_irqindex(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX);
vfio_msi_disable_common(vdev);
- vfio_intx_enable(vdev, &err);
- if (err) {
+ if (!vfio_intx_enable(vdev, &err)) {
error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
}
@@ -2450,8 +2449,7 @@ void vfio_pci_post_reset(VFIOPCIDevice *vdev)
Error *err = NULL;
int nr;
- vfio_intx_enable(vdev, &err);
- if (err) {
+ if (!vfio_intx_enable(vdev, &err)) {
error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
}
@@ -3194,8 +3192,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
vfio_intx_routing_notifier);
vdev->irqchip_change_notifier.notify = vfio_irqchip_change;
kvm_irqchip_add_change_notifier(&vdev->irqchip_change_notifier);
- ret = vfio_intx_enable(vdev, errp);
- if (ret) {
+ if (!vfio_intx_enable(vdev, errp)) {
goto out_deregister;
}
}
--
2.34.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH v2 12/20] vfio/pci: Make vfio_populate_vga() return bool
2024-05-22 4:39 [PATCH v2 00/20] VFIO: misc cleanups part2 Zhenzhong Duan
` (10 preceding siblings ...)
2024-05-22 4:40 ` [PATCH v2 11/20] vfio/pci: Make vfio_intx_enable() return bool Zhenzhong Duan
@ 2024-05-22 4:40 ` Zhenzhong Duan
2024-05-22 4:40 ` [PATCH v2 13/20] vfio/pci: Make capability related functions " Zhenzhong Duan
` (8 subsequent siblings)
20 siblings, 0 replies; 32+ messages in thread
From: Zhenzhong Duan @ 2024-05-22 4:40 UTC (permalink / raw)
To: qemu-devel; +Cc: alex.williamson, clg, eric.auger, chao.p.peng, Zhenzhong Duan
This is to follow the coding standand in qapi/error.h to return bool
for bool-valued functions.
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>
---
hw/vfio/pci.h | 2 +-
hw/vfio/igd.c | 2 +-
hw/vfio/pci.c | 11 +++++------
3 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index a5ac9efd4b..7914f019d5 100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -225,7 +225,7 @@ bool vfio_pci_host_match(PCIHostDeviceAddress *addr, const char *name);
int vfio_pci_get_pci_hot_reset_info(VFIOPCIDevice *vdev,
struct vfio_pci_hot_reset_info **info_p);
-int vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp);
+bool vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp);
int vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev,
struct vfio_region_info *info,
diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
index b31ee79c60..ffe57c5954 100644
--- a/hw/vfio/igd.c
+++ b/hw/vfio/igd.c
@@ -478,7 +478,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
* try to enable it. Probably shouldn't be using legacy mode without VGA,
* but also no point in us enabling VGA if disabled in hardware.
*/
- if (!(gmch & 0x2) && !vdev->vga && vfio_populate_vga(vdev, &err)) {
+ if (!(gmch & 0x2) && !vdev->vga && !vfio_populate_vga(vdev, &err)) {
error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
error_report("IGD device %s failed to enable VGA access, "
"legacy mode disabled", vdev->vbasedev.name);
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 7f35cb8a29..ab8f74299e 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2670,7 +2670,7 @@ static VFIODeviceOps vfio_pci_ops = {
.vfio_load_config = vfio_pci_load_config,
};
-int vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp)
+bool vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp)
{
VFIODevice *vbasedev = &vdev->vbasedev;
struct vfio_region_info *reg_info;
@@ -2681,7 +2681,7 @@ int vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp)
error_setg_errno(errp, -ret,
"failed getting region info for VGA region index %d",
VFIO_PCI_VGA_REGION_INDEX);
- return ret;
+ return false;
}
if (!(reg_info->flags & VFIO_REGION_INFO_FLAG_READ) ||
@@ -2691,7 +2691,7 @@ int vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp)
(unsigned long)reg_info->flags,
(unsigned long)reg_info->size);
g_free(reg_info);
- return -EINVAL;
+ return false;
}
vdev->vga = g_new0(VFIOVGA, 1);
@@ -2735,7 +2735,7 @@ int vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp)
&vdev->vga->region[QEMU_PCI_VGA_IO_LO].mem,
&vdev->vga->region[QEMU_PCI_VGA_IO_HI].mem);
- return 0;
+ return true;
}
static bool vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
@@ -2798,8 +2798,7 @@ static bool vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
g_free(reg_info);
if (vdev->features & VFIO_FEATURE_ENABLE_VGA) {
- ret = vfio_populate_vga(vdev, errp);
- if (ret) {
+ if (!vfio_populate_vga(vdev, errp)) {
error_append_hint(errp, "device does not support "
"requested feature x-vga\n");
return false;
--
2.34.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH v2 13/20] vfio/pci: Make capability related functions return bool
2024-05-22 4:39 [PATCH v2 00/20] VFIO: misc cleanups part2 Zhenzhong Duan
` (11 preceding siblings ...)
2024-05-22 4:40 ` [PATCH v2 12/20] vfio/pci: Make vfio_populate_vga() " Zhenzhong Duan
@ 2024-05-22 4:40 ` Zhenzhong Duan
2024-05-22 4:40 ` [PATCH v2 14/20] vfio/pci: Use g_autofree for vfio_region_info pointer Zhenzhong Duan
` (7 subsequent siblings)
20 siblings, 0 replies; 32+ messages in thread
From: Zhenzhong Duan @ 2024-05-22 4:40 UTC (permalink / raw)
To: qemu-devel; +Cc: alex.williamson, clg, eric.auger, chao.p.peng, Zhenzhong Duan
The functions operating on capability don't have a consistent return style.
Below functions are in bool-valued functions style:
vfio_msi_setup()
vfio_msix_setup()
vfio_add_std_cap()
vfio_add_capabilities()
Below two are integer-valued functions:
vfio_add_vendor_specific_cap()
vfio_setup_pcie_cap()
But the returned integer is only used for check succeed/failure.
Change them all to return bool so now all capability related
functions follow the coding standand in qapi/error.h to return
bool.
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
---
hw/vfio/pci.c | 77 ++++++++++++++++++++++++---------------------------
1 file changed, 36 insertions(+), 41 deletions(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index ab8f74299e..c3323912dd 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1339,7 +1339,7 @@ static void vfio_disable_interrupts(VFIOPCIDevice *vdev)
}
}
-static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
+static bool vfio_msi_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
{
uint16_t ctrl;
bool msi_64bit, msi_maskbit;
@@ -1349,7 +1349,7 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
if (pread(vdev->vbasedev.fd, &ctrl, sizeof(ctrl),
vdev->config_offset + pos + PCI_CAP_FLAGS) != sizeof(ctrl)) {
error_setg_errno(errp, errno, "failed reading MSI PCI_CAP_FLAGS");
- return -errno;
+ return false;
}
ctrl = le16_to_cpu(ctrl);
@@ -1362,14 +1362,14 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
ret = msi_init(&vdev->pdev, pos, entries, msi_64bit, msi_maskbit, &err);
if (ret < 0) {
if (ret == -ENOTSUP) {
- return 0;
+ return true;
}
error_propagate_prepend(errp, err, "msi_init failed: ");
- return ret;
+ return false;
}
vdev->msi_cap_size = 0xa + (msi_maskbit ? 0xa : 0) + (msi_64bit ? 0x4 : 0);
- return 0;
+ return true;
}
static void vfio_pci_fixup_msix_region(VFIOPCIDevice *vdev)
@@ -1644,7 +1644,7 @@ static bool vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
return vfio_pci_relocate_msix(vdev, errp);
}
-static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
+static bool vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
{
int ret;
Error *err = NULL;
@@ -1660,11 +1660,11 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
if (ret < 0) {
if (ret == -ENOTSUP) {
warn_report_err(err);
- return 0;
+ return true;
}
error_propagate(errp, err);
- return ret;
+ return false;
}
/*
@@ -1698,7 +1698,7 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
memory_region_set_enabled(&vdev->pdev.msix_table_mmio, false);
}
- return 0;
+ return true;
}
static void vfio_teardown_msi(VFIOPCIDevice *vdev)
@@ -1977,8 +1977,8 @@ static void vfio_pci_disable_rp_atomics(VFIOPCIDevice *vdev)
}
}
-static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size,
- Error **errp)
+static bool vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size,
+ Error **errp)
{
uint16_t flags;
uint8_t type;
@@ -1992,7 +1992,7 @@ static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size,
error_setg(errp, "assignment of PCIe type 0x%x "
"devices is not currently supported", type);
- return -EINVAL;
+ return false;
}
if (!pci_bus_is_express(pci_get_bus(&vdev->pdev))) {
@@ -2025,7 +2025,7 @@ static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size,
}
if (pci_bus_is_express(bus)) {
- return 0;
+ return true;
}
} else if (pci_bus_is_root(pci_get_bus(&vdev->pdev))) {
@@ -2063,7 +2063,7 @@ static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size,
* Legacy endpoints don't belong on the root complex. Windows
* seems to be happier with devices if we skip the capability.
*/
- return 0;
+ return true;
}
} else {
@@ -2099,12 +2099,12 @@ static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size,
pos = pci_add_capability(&vdev->pdev, PCI_CAP_ID_EXP, pos, size,
errp);
if (pos < 0) {
- return pos;
+ return false;
}
vdev->pdev.exp.exp_cap = pos;
- return pos;
+ return true;
}
static void vfio_check_pcie_flr(VFIOPCIDevice *vdev, uint8_t pos)
@@ -2137,14 +2137,14 @@ static void vfio_check_af_flr(VFIOPCIDevice *vdev, uint8_t pos)
}
}
-static int vfio_add_vendor_specific_cap(VFIOPCIDevice *vdev, int pos,
- uint8_t size, Error **errp)
+static bool vfio_add_vendor_specific_cap(VFIOPCIDevice *vdev, int pos,
+ uint8_t size, Error **errp)
{
PCIDevice *pdev = &vdev->pdev;
pos = pci_add_capability(pdev, PCI_CAP_ID_VNDR, pos, size, errp);
if (pos < 0) {
- return pos;
+ return false;
}
/*
@@ -2156,15 +2156,15 @@ static int vfio_add_vendor_specific_cap(VFIOPCIDevice *vdev, int pos,
memset(pdev->cmask + pos + 3, 0, size - 3);
}
- return pos;
+ return true;
}
-static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos, Error **errp)
+static bool vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos, Error **errp)
{
ERRP_GUARD();
PCIDevice *pdev = &vdev->pdev;
uint8_t cap_id, next, size;
- int ret;
+ bool ret;
cap_id = pdev->config[pos];
next = pdev->config[pos + PCI_CAP_LIST_NEXT];
@@ -2185,9 +2185,8 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos, Error **errp)
* will be changed as we unwind the stack.
*/
if (next) {
- ret = vfio_add_std_cap(vdev, next, errp);
- if (ret) {
- return ret;
+ if (!vfio_add_std_cap(vdev, next, errp)) {
+ return false;
}
} else {
/* Begin the rebuild, use QEMU emulated list bits */
@@ -2197,7 +2196,7 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos, Error **errp)
ret = vfio_add_virt_caps(vdev, errp);
if (ret) {
- return ret;
+ return false;
}
}
@@ -2221,28 +2220,27 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos, Error **errp)
case PCI_CAP_ID_PM:
vfio_check_pm_reset(vdev, pos);
vdev->pm_cap = pos;
- ret = pci_add_capability(pdev, cap_id, pos, size, errp);
+ ret = pci_add_capability(pdev, cap_id, pos, size, errp) >= 0;
break;
case PCI_CAP_ID_AF:
vfio_check_af_flr(vdev, pos);
- ret = pci_add_capability(pdev, cap_id, pos, size, errp);
+ ret = pci_add_capability(pdev, cap_id, pos, size, errp) >= 0;
break;
case PCI_CAP_ID_VNDR:
ret = vfio_add_vendor_specific_cap(vdev, pos, size, errp);
break;
default:
- ret = pci_add_capability(pdev, cap_id, pos, size, errp);
+ ret = pci_add_capability(pdev, cap_id, pos, size, errp) >= 0;
break;
}
- if (ret < 0) {
+ if (!ret) {
error_prepend(errp,
"failed to add PCI capability 0x%x[0x%x]@0x%x: ",
cap_id, size, pos);
- return ret;
}
- return 0;
+ return ret;
}
static int vfio_setup_rebar_ecap(VFIOPCIDevice *vdev, uint16_t pos)
@@ -2388,23 +2386,21 @@ static void vfio_add_ext_cap(VFIOPCIDevice *vdev)
return;
}
-static int vfio_add_capabilities(VFIOPCIDevice *vdev, Error **errp)
+static bool vfio_add_capabilities(VFIOPCIDevice *vdev, Error **errp)
{
PCIDevice *pdev = &vdev->pdev;
- int ret;
if (!(pdev->config[PCI_STATUS] & PCI_STATUS_CAP_LIST) ||
!pdev->config[PCI_CAPABILITY_LIST]) {
- return 0; /* Nothing to add */
+ return true; /* Nothing to add */
}
- ret = vfio_add_std_cap(vdev, pdev->config[PCI_CAPABILITY_LIST], errp);
- if (ret) {
- return ret;
+ if (!vfio_add_std_cap(vdev, pdev->config[PCI_CAPABILITY_LIST], errp)) {
+ return false;
}
vfio_add_ext_cap(vdev);
- return 0;
+ return true;
}
void vfio_pci_pre_reset(VFIOPCIDevice *vdev)
@@ -3133,8 +3129,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
vfio_bars_register(vdev);
- ret = vfio_add_capabilities(vdev, errp);
- if (ret) {
+ if (!vfio_add_capabilities(vdev, errp)) {
goto out_teardown;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH v2 14/20] vfio/pci: Use g_autofree for vfio_region_info pointer
2024-05-22 4:39 [PATCH v2 00/20] VFIO: misc cleanups part2 Zhenzhong Duan
` (12 preceding siblings ...)
2024-05-22 4:40 ` [PATCH v2 13/20] vfio/pci: Make capability related functions " Zhenzhong Duan
@ 2024-05-22 4:40 ` Zhenzhong Duan
2024-05-22 4:40 ` [PATCH v2 15/20] vfio/pci-quirks: Make vfio_pci_igd_opregion_init() return bool Zhenzhong Duan
` (6 subsequent siblings)
20 siblings, 0 replies; 32+ messages in thread
From: Zhenzhong Duan @ 2024-05-22 4:40 UTC (permalink / raw)
To: qemu-devel; +Cc: alex.williamson, clg, eric.auger, chao.p.peng, Zhenzhong Duan
Pointer opregion is freed after vfio_pci_igd_opregion_init().
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>
---
hw/vfio/pci.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index c3323912dd..8379d2284a 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3143,7 +3143,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
if (!vdev->igd_opregion &&
vdev->features & VFIO_FEATURE_ENABLE_IGD_OPREGION) {
- struct vfio_region_info *opregion;
+ g_autofree struct vfio_region_info *opregion = NULL;
if (vdev->pdev.qdev.hotplugged) {
error_setg(errp,
@@ -3162,7 +3162,6 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
}
ret = vfio_pci_igd_opregion_init(vdev, opregion, errp);
- g_free(opregion);
if (ret) {
goto out_teardown;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH v2 15/20] vfio/pci-quirks: Make vfio_pci_igd_opregion_init() return bool
2024-05-22 4:39 [PATCH v2 00/20] VFIO: misc cleanups part2 Zhenzhong Duan
` (13 preceding siblings ...)
2024-05-22 4:40 ` [PATCH v2 14/20] vfio/pci: Use g_autofree for vfio_region_info pointer Zhenzhong Duan
@ 2024-05-22 4:40 ` Zhenzhong Duan
2024-05-22 4:40 ` [PATCH v2 16/20] vfio/pci-quirks: Make vfio_add_*_cap() " Zhenzhong Duan
` (5 subsequent siblings)
20 siblings, 0 replies; 32+ messages in thread
From: Zhenzhong Duan @ 2024-05-22 4:40 UTC (permalink / raw)
To: qemu-devel; +Cc: alex.williamson, clg, eric.auger, chao.p.peng, Zhenzhong Duan
This is to follow the coding standand in qapi/error.h to return bool
for bool-valued functions.
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>
---
hw/vfio/pci.h | 6 +++---
hw/vfio/igd.c | 3 +--
hw/vfio/pci-quirks.c | 8 ++++----
hw/vfio/pci.c | 3 +--
4 files changed, 9 insertions(+), 11 deletions(-)
diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index 7914f019d5..f158681072 100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -227,9 +227,9 @@ int vfio_pci_get_pci_hot_reset_info(VFIOPCIDevice *vdev,
bool vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp);
-int vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev,
- struct vfio_region_info *info,
- Error **errp);
+bool vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev,
+ struct vfio_region_info *info,
+ Error **errp);
void vfio_display_reset(VFIOPCIDevice *vdev);
bool vfio_display_probe(VFIOPCIDevice *vdev, Error **errp);
diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
index ffe57c5954..402fc5ce1d 100644
--- a/hw/vfio/igd.c
+++ b/hw/vfio/igd.c
@@ -502,8 +502,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
}
/* Setup OpRegion access */
- ret = vfio_pci_igd_opregion_init(vdev, opregion, &err);
- if (ret) {
+ if (!vfio_pci_igd_opregion_init(vdev, opregion, &err)) {
error_append_hint(&err, "IGD legacy mode disabled\n");
error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
goto out;
diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
index 496fd1ee86..ca27917159 100644
--- a/hw/vfio/pci-quirks.c
+++ b/hw/vfio/pci-quirks.c
@@ -1169,8 +1169,8 @@ static void vfio_probe_rtl8168_bar2_quirk(VFIOPCIDevice *vdev, int nr)
* the table and to write the base address of that memory to the ASLS register
* of the IGD device.
*/
-int vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev,
- struct vfio_region_info *info, Error **errp)
+bool vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev,
+ struct vfio_region_info *info, Error **errp)
{
int ret;
@@ -1181,7 +1181,7 @@ int vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev,
error_setg(errp, "failed to read IGD OpRegion");
g_free(vdev->igd_opregion);
vdev->igd_opregion = NULL;
- return -EINVAL;
+ return false;
}
/*
@@ -1206,7 +1206,7 @@ int vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev,
pci_set_long(vdev->pdev.wmask + IGD_ASLS, ~0);
pci_set_long(vdev->emulated_config_bits + IGD_ASLS, ~0);
- return 0;
+ return true;
}
/*
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 8379d2284a..76a3931dba 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3161,8 +3161,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
goto out_teardown;
}
- ret = vfio_pci_igd_opregion_init(vdev, opregion, errp);
- if (ret) {
+ if (!vfio_pci_igd_opregion_init(vdev, opregion, errp)) {
goto out_teardown;
}
}
--
2.34.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH v2 16/20] vfio/pci-quirks: Make vfio_add_*_cap() return bool
2024-05-22 4:39 [PATCH v2 00/20] VFIO: misc cleanups part2 Zhenzhong Duan
` (14 preceding siblings ...)
2024-05-22 4:40 ` [PATCH v2 15/20] vfio/pci-quirks: Make vfio_pci_igd_opregion_init() return bool Zhenzhong Duan
@ 2024-05-22 4:40 ` Zhenzhong Duan
2024-05-22 4:40 ` [PATCH v2 17/20] vfio: Use g_autofree in all call site of vfio_get_region_info() Zhenzhong Duan
` (4 subsequent siblings)
20 siblings, 0 replies; 32+ messages in thread
From: Zhenzhong Duan @ 2024-05-22 4:40 UTC (permalink / raw)
To: qemu-devel; +Cc: alex.williamson, clg, eric.auger, chao.p.peng, Zhenzhong Duan
This is to follow the coding standand in qapi/error.h to return bool
for bool-valued functions.
Include below functions:
vfio_add_virt_caps()
vfio_add_nv_gpudirect_cap()
vfio_add_vmd_shadow_cap()
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>
---
hw/vfio/pci.h | 2 +-
hw/vfio/pci-quirks.c | 42 +++++++++++++++++++-----------------------
hw/vfio/pci.c | 3 +--
3 files changed, 21 insertions(+), 26 deletions(-)
diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index f158681072..bf67df2fbc 100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -212,7 +212,7 @@ void vfio_bar_quirk_setup(VFIOPCIDevice *vdev, int nr);
void vfio_bar_quirk_exit(VFIOPCIDevice *vdev, int nr);
void vfio_bar_quirk_finalize(VFIOPCIDevice *vdev, int nr);
void vfio_setup_resetfn_quirk(VFIOPCIDevice *vdev);
-int vfio_add_virt_caps(VFIOPCIDevice *vdev, Error **errp);
+bool vfio_add_virt_caps(VFIOPCIDevice *vdev, Error **errp);
void vfio_quirk_reset(VFIOPCIDevice *vdev);
VFIOQuirk *vfio_quirk_alloc(int nr_mem);
void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr);
diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
index ca27917159..39dae72497 100644
--- a/hw/vfio/pci-quirks.c
+++ b/hw/vfio/pci-quirks.c
@@ -1536,7 +1536,7 @@ static bool is_valid_std_cap_offset(uint8_t pos)
pos <= (PCI_CFG_SPACE_SIZE - PCI_CAP_SIZEOF));
}
-static int vfio_add_nv_gpudirect_cap(VFIOPCIDevice *vdev, Error **errp)
+static bool vfio_add_nv_gpudirect_cap(VFIOPCIDevice *vdev, Error **errp)
{
ERRP_GUARD();
PCIDevice *pdev = &vdev->pdev;
@@ -1545,18 +1545,18 @@ static int vfio_add_nv_gpudirect_cap(VFIOPCIDevice *vdev, Error **errp)
uint8_t tmp;
if (vdev->nv_gpudirect_clique == 0xFF) {
- return 0;
+ return true;
}
if (!vfio_pci_is(vdev, PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID)) {
error_setg(errp, "NVIDIA GPUDirect Clique ID: invalid device vendor");
- return -EINVAL;
+ return false;
}
if (pci_get_byte(pdev->config + PCI_CLASS_DEVICE + 1) !=
PCI_BASE_CLASS_DISPLAY) {
error_setg(errp, "NVIDIA GPUDirect Clique ID: unsupported PCI class");
- return -EINVAL;
+ return false;
}
/*
@@ -1572,7 +1572,7 @@ static int vfio_add_nv_gpudirect_cap(VFIOPCIDevice *vdev, Error **errp)
vdev->config_offset + PCI_CAPABILITY_LIST);
if (ret != 1 || !is_valid_std_cap_offset(tmp)) {
error_setg(errp, "NVIDIA GPUDirect Clique ID: error getting cap list");
- return -EINVAL;
+ return false;
}
do {
@@ -1590,13 +1590,13 @@ static int vfio_add_nv_gpudirect_cap(VFIOPCIDevice *vdev, Error **errp)
pos = 0xD4;
} else {
error_setg(errp, "NVIDIA GPUDirect Clique ID: invalid config space");
- return -EINVAL;
+ return false;
}
ret = pci_add_capability(pdev, PCI_CAP_ID_VNDR, pos, 8, errp);
if (ret < 0) {
error_prepend(errp, "Failed to add NVIDIA GPUDirect cap: ");
- return ret;
+ return false;
}
memset(vdev->emulated_config_bits + pos, 0xFF, 8);
@@ -1608,7 +1608,7 @@ static int vfio_add_nv_gpudirect_cap(VFIOPCIDevice *vdev, Error **errp)
pci_set_byte(pdev->config + pos++, vdev->nv_gpudirect_clique << 3);
pci_set_byte(pdev->config + pos, 0);
- return 0;
+ return true;
}
/*
@@ -1629,7 +1629,7 @@ static int vfio_add_nv_gpudirect_cap(VFIOPCIDevice *vdev, Error **errp)
*/
#define VMD_SHADOW_CAP_VER 1
#define VMD_SHADOW_CAP_LEN 24
-static int vfio_add_vmd_shadow_cap(VFIOPCIDevice *vdev, Error **errp)
+static bool vfio_add_vmd_shadow_cap(VFIOPCIDevice *vdev, Error **errp)
{
ERRP_GUARD();
uint8_t membar_phys[16];
@@ -1639,7 +1639,7 @@ static int vfio_add_vmd_shadow_cap(VFIOPCIDevice *vdev, Error **errp)
vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, 0x467F) ||
vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, 0x4C3D) ||
vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, 0x9A0B))) {
- return 0;
+ return true;
}
ret = pread(vdev->vbasedev.fd, membar_phys, 16,
@@ -1647,14 +1647,14 @@ static int vfio_add_vmd_shadow_cap(VFIOPCIDevice *vdev, Error **errp)
if (ret != 16) {
error_report("VMD %s cannot read MEMBARs (%d)",
vdev->vbasedev.name, ret);
- return -EFAULT;
+ return false;
}
ret = pci_add_capability(&vdev->pdev, PCI_CAP_ID_VNDR, pos,
VMD_SHADOW_CAP_LEN, errp);
if (ret < 0) {
error_prepend(errp, "Failed to add VMD MEMBAR Shadow cap: ");
- return ret;
+ return false;
}
memset(vdev->emulated_config_bits + pos, 0xFF, VMD_SHADOW_CAP_LEN);
@@ -1664,22 +1664,18 @@ static int vfio_add_vmd_shadow_cap(VFIOPCIDevice *vdev, Error **errp)
pci_set_long(vdev->pdev.config + pos, 0x53484457); /* SHDW */
memcpy(vdev->pdev.config + pos + 4, membar_phys, 16);
- return 0;
+ return true;
}
-int vfio_add_virt_caps(VFIOPCIDevice *vdev, Error **errp)
+bool vfio_add_virt_caps(VFIOPCIDevice *vdev, Error **errp)
{
- int ret;
-
- ret = vfio_add_nv_gpudirect_cap(vdev, errp);
- if (ret) {
- return ret;
+ if (!vfio_add_nv_gpudirect_cap(vdev, errp)) {
+ return false;
}
- ret = vfio_add_vmd_shadow_cap(vdev, errp);
- if (ret) {
- return ret;
+ if (!vfio_add_vmd_shadow_cap(vdev, errp)) {
+ return false;
}
- return 0;
+ return true;
}
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 76a3931dba..35ad9b582f 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2194,8 +2194,7 @@ static bool vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos, Error **errp)
vdev->emulated_config_bits[PCI_CAPABILITY_LIST] = 0xff;
vdev->emulated_config_bits[PCI_STATUS] |= PCI_STATUS_CAP_LIST;
- ret = vfio_add_virt_caps(vdev, errp);
- if (ret) {
+ if (!vfio_add_virt_caps(vdev, errp)) {
return false;
}
}
--
2.34.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH v2 17/20] vfio: Use g_autofree in all call site of vfio_get_region_info()
2024-05-22 4:39 [PATCH v2 00/20] VFIO: misc cleanups part2 Zhenzhong Duan
` (15 preceding siblings ...)
2024-05-22 4:40 ` [PATCH v2 16/20] vfio/pci-quirks: Make vfio_add_*_cap() " Zhenzhong Duan
@ 2024-05-22 4:40 ` Zhenzhong Duan
2024-05-22 7:20 ` Cédric Le Goater
2024-05-22 4:40 ` [PATCH v2 18/20] vfio/igd: Use g_autofree in vfio_probe_igd_bar4_quirk() Zhenzhong Duan
` (3 subsequent siblings)
20 siblings, 1 reply; 32+ messages in thread
From: Zhenzhong Duan @ 2024-05-22 4:40 UTC (permalink / raw)
To: qemu-devel; +Cc: alex.williamson, clg, eric.auger, chao.p.peng, Zhenzhong Duan
There are some exceptions when pointer to vfio_region_info is reused.
In that case, the pointed memory is freed manually.
Suggested-by: Cédric Le Goater <clg@redhat.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
hw/vfio/helpers.c | 7 ++-----
hw/vfio/igd.c | 5 ++---
hw/vfio/pci.c | 13 +++----------
3 files changed, 7 insertions(+), 18 deletions(-)
diff --git a/hw/vfio/helpers.c b/hw/vfio/helpers.c
index 4b079dc383..27ea26aa48 100644
--- a/hw/vfio/helpers.c
+++ b/hw/vfio/helpers.c
@@ -343,7 +343,7 @@ static int vfio_setup_region_sparse_mmaps(VFIORegion *region,
int vfio_region_setup(Object *obj, VFIODevice *vbasedev, VFIORegion *region,
int index, const char *name)
{
- struct vfio_region_info *info;
+ g_autofree struct vfio_region_info *info = NULL;
int ret;
ret = vfio_get_region_info(vbasedev, index, &info);
@@ -376,8 +376,6 @@ int vfio_region_setup(Object *obj, VFIODevice *vbasedev, VFIORegion *region,
}
}
- g_free(info);
-
trace_vfio_region_setup(vbasedev->name, index, name,
region->flags, region->fd_offset, region->size);
return 0;
@@ -594,14 +592,13 @@ int vfio_get_dev_region_info(VFIODevice *vbasedev, uint32_t type,
bool vfio_has_region_cap(VFIODevice *vbasedev, int region, uint16_t cap_type)
{
- struct vfio_region_info *info = NULL;
+ g_autofree struct vfio_region_info *info = NULL;
bool ret = false;
if (!vfio_get_region_info(vbasedev, region, &info)) {
if (vfio_get_region_info_cap(info, cap_type)) {
ret = true;
}
- g_free(info);
}
return ret;
diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
index 402fc5ce1d..1e79202f2b 100644
--- a/hw/vfio/igd.c
+++ b/hw/vfio/igd.c
@@ -367,8 +367,8 @@ static const MemoryRegionOps vfio_igd_index_quirk = {
void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
{
- struct vfio_region_info *rom = NULL, *opregion = NULL,
- *host = NULL, *lpc = NULL;
+ g_autofree struct vfio_region_info *rom = NULL;
+ struct vfio_region_info *opregion = NULL, *host = NULL, *lpc = NULL;
VFIOQuirk *quirk;
VFIOIGDQuirk *igd;
PCIDevice *lpc_bridge;
@@ -609,7 +609,6 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
trace_vfio_pci_igd_bdsm_enabled(vdev->vbasedev.name, ggms_mb + gms_mb);
out:
- g_free(rom);
g_free(opregion);
g_free(host);
g_free(lpc);
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 35ad9b582f..74a79bdf61 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -879,7 +879,7 @@ static void vfio_update_msi(VFIOPCIDevice *vdev)
static void vfio_pci_load_rom(VFIOPCIDevice *vdev)
{
- struct vfio_region_info *reg_info;
+ g_autofree struct vfio_region_info *reg_info = NULL;
uint64_t size;
off_t off = 0;
ssize_t bytes;
@@ -897,8 +897,6 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev)
vdev->rom_size = size = reg_info->size;
vdev->rom_offset = reg_info->offset;
- g_free(reg_info);
-
if (!vdev->rom_size) {
vdev->rom_read_failed = true;
error_report("vfio-pci: Cannot read device rom at "
@@ -2668,7 +2666,7 @@ static VFIODeviceOps vfio_pci_ops = {
bool vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp)
{
VFIODevice *vbasedev = &vdev->vbasedev;
- struct vfio_region_info *reg_info;
+ g_autofree struct vfio_region_info *reg_info = NULL;
int ret;
ret = vfio_get_region_info(vbasedev, VFIO_PCI_VGA_REGION_INDEX, ®_info);
@@ -2685,7 +2683,6 @@ bool vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp)
error_setg(errp, "unexpected VGA info, flags 0x%lx, size 0x%lx",
(unsigned long)reg_info->flags,
(unsigned long)reg_info->size);
- g_free(reg_info);
return false;
}
@@ -2694,8 +2691,6 @@ bool vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp)
vdev->vga->fd_offset = reg_info->offset;
vdev->vga->fd = vdev->vbasedev.fd;
- g_free(reg_info);
-
vdev->vga->region[QEMU_PCI_VGA_MEM].offset = QEMU_PCI_VGA_MEM_BASE;
vdev->vga->region[QEMU_PCI_VGA_MEM].nr = QEMU_PCI_VGA_MEM;
QLIST_INIT(&vdev->vga->region[QEMU_PCI_VGA_MEM].quirks);
@@ -2736,7 +2731,7 @@ bool vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp)
static bool vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
{
VFIODevice *vbasedev = &vdev->vbasedev;
- struct vfio_region_info *reg_info;
+ g_autofree struct vfio_region_info *reg_info = NULL;
struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info) };
int i, ret = -1;
@@ -2790,8 +2785,6 @@ static bool vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
}
vdev->config_offset = reg_info->offset;
- g_free(reg_info);
-
if (vdev->features & VFIO_FEATURE_ENABLE_VGA) {
if (!vfio_populate_vga(vdev, errp)) {
error_append_hint(errp, "device does not support "
--
2.34.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH v2 17/20] vfio: Use g_autofree in all call site of vfio_get_region_info()
2024-05-22 4:40 ` [PATCH v2 17/20] vfio: Use g_autofree in all call site of vfio_get_region_info() Zhenzhong Duan
@ 2024-05-22 7:20 ` Cédric Le Goater
0 siblings, 0 replies; 32+ messages in thread
From: Cédric Le Goater @ 2024-05-22 7:20 UTC (permalink / raw)
To: Zhenzhong Duan, qemu-devel; +Cc: alex.williamson, eric.auger, chao.p.peng
On 5/22/24 06:40, Zhenzhong Duan wrote:
> There are some exceptions when pointer to vfio_region_info is reused.
> In that case, the pointed memory is freed manually.
>
> 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/helpers.c | 7 ++-----
> hw/vfio/igd.c | 5 ++---
> hw/vfio/pci.c | 13 +++----------
> 3 files changed, 7 insertions(+), 18 deletions(-)
>
> diff --git a/hw/vfio/helpers.c b/hw/vfio/helpers.c
> index 4b079dc383..27ea26aa48 100644
> --- a/hw/vfio/helpers.c
> +++ b/hw/vfio/helpers.c
> @@ -343,7 +343,7 @@ static int vfio_setup_region_sparse_mmaps(VFIORegion *region,
> int vfio_region_setup(Object *obj, VFIODevice *vbasedev, VFIORegion *region,
> int index, const char *name)
> {
> - struct vfio_region_info *info;
> + g_autofree struct vfio_region_info *info = NULL;
> int ret;
>
> ret = vfio_get_region_info(vbasedev, index, &info);
> @@ -376,8 +376,6 @@ int vfio_region_setup(Object *obj, VFIODevice *vbasedev, VFIORegion *region,
> }
> }
>
> - g_free(info);
> -
> trace_vfio_region_setup(vbasedev->name, index, name,
> region->flags, region->fd_offset, region->size);
> return 0;
> @@ -594,14 +592,13 @@ int vfio_get_dev_region_info(VFIODevice *vbasedev, uint32_t type,
>
> bool vfio_has_region_cap(VFIODevice *vbasedev, int region, uint16_t cap_type)
> {
> - struct vfio_region_info *info = NULL;
> + g_autofree struct vfio_region_info *info = NULL;
> bool ret = false;
>
> if (!vfio_get_region_info(vbasedev, region, &info)) {
> if (vfio_get_region_info_cap(info, cap_type)) {
> ret = true;
> }
> - g_free(info);
> }
>
> return ret;
> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
> index 402fc5ce1d..1e79202f2b 100644
> --- a/hw/vfio/igd.c
> +++ b/hw/vfio/igd.c
> @@ -367,8 +367,8 @@ static const MemoryRegionOps vfio_igd_index_quirk = {
>
> void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
> {
> - struct vfio_region_info *rom = NULL, *opregion = NULL,
> - *host = NULL, *lpc = NULL;
> + g_autofree struct vfio_region_info *rom = NULL;
> + struct vfio_region_info *opregion = NULL, *host = NULL, *lpc = NULL;
> VFIOQuirk *quirk;
> VFIOIGDQuirk *igd;
> PCIDevice *lpc_bridge;
> @@ -609,7 +609,6 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
> trace_vfio_pci_igd_bdsm_enabled(vdev->vbasedev.name, ggms_mb + gms_mb);
>
> out:
> - g_free(rom);
> g_free(opregion);
> g_free(host);
> g_free(lpc);
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 35ad9b582f..74a79bdf61 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -879,7 +879,7 @@ static void vfio_update_msi(VFIOPCIDevice *vdev)
>
> static void vfio_pci_load_rom(VFIOPCIDevice *vdev)
> {
> - struct vfio_region_info *reg_info;
> + g_autofree struct vfio_region_info *reg_info = NULL;
> uint64_t size;
> off_t off = 0;
> ssize_t bytes;
> @@ -897,8 +897,6 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev)
> vdev->rom_size = size = reg_info->size;
> vdev->rom_offset = reg_info->offset;
>
> - g_free(reg_info);
> -
> if (!vdev->rom_size) {
> vdev->rom_read_failed = true;
> error_report("vfio-pci: Cannot read device rom at "
> @@ -2668,7 +2666,7 @@ static VFIODeviceOps vfio_pci_ops = {
> bool vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp)
> {
> VFIODevice *vbasedev = &vdev->vbasedev;
> - struct vfio_region_info *reg_info;
> + g_autofree struct vfio_region_info *reg_info = NULL;
> int ret;
>
> ret = vfio_get_region_info(vbasedev, VFIO_PCI_VGA_REGION_INDEX, ®_info);
> @@ -2685,7 +2683,6 @@ bool vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp)
> error_setg(errp, "unexpected VGA info, flags 0x%lx, size 0x%lx",
> (unsigned long)reg_info->flags,
> (unsigned long)reg_info->size);
> - g_free(reg_info);
> return false;
> }
>
> @@ -2694,8 +2691,6 @@ bool vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp)
> vdev->vga->fd_offset = reg_info->offset;
> vdev->vga->fd = vdev->vbasedev.fd;
>
> - g_free(reg_info);
> -
> vdev->vga->region[QEMU_PCI_VGA_MEM].offset = QEMU_PCI_VGA_MEM_BASE;
> vdev->vga->region[QEMU_PCI_VGA_MEM].nr = QEMU_PCI_VGA_MEM;
> QLIST_INIT(&vdev->vga->region[QEMU_PCI_VGA_MEM].quirks);
> @@ -2736,7 +2731,7 @@ bool vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp)
> static bool vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
> {
> VFIODevice *vbasedev = &vdev->vbasedev;
> - struct vfio_region_info *reg_info;
> + g_autofree struct vfio_region_info *reg_info = NULL;
> struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info) };
> int i, ret = -1;
>
> @@ -2790,8 +2785,6 @@ static bool vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
> }
> vdev->config_offset = reg_info->offset;
>
> - g_free(reg_info);
> -
> if (vdev->features & VFIO_FEATURE_ENABLE_VGA) {
> if (!vfio_populate_vga(vdev, errp)) {
> error_append_hint(errp, "device does not support "
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v2 18/20] vfio/igd: Use g_autofree in vfio_probe_igd_bar4_quirk()
2024-05-22 4:39 [PATCH v2 00/20] VFIO: misc cleanups part2 Zhenzhong Duan
` (16 preceding siblings ...)
2024-05-22 4:40 ` [PATCH v2 17/20] vfio: Use g_autofree in all call site of vfio_get_region_info() Zhenzhong Duan
@ 2024-05-22 4:40 ` Zhenzhong Duan
2024-05-22 7:14 ` Cédric Le Goater
2024-05-22 4:40 ` [PATCH v2 19/20] vfio/ccw: Drop local @err in vfio_ccw_realize() Zhenzhong Duan
` (2 subsequent siblings)
20 siblings, 1 reply; 32+ messages in thread
From: Zhenzhong Duan @ 2024-05-22 4:40 UTC (permalink / raw)
To: qemu-devel; +Cc: alex.williamson, clg, eric.auger, chao.p.peng, Zhenzhong Duan
Pointer opregion, host and lpc are allocated and freed in
vfio_probe_igd_bar4_quirk(). Use g_autofree to automatically
free them.
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
hw/vfio/igd.c | 27 ++++++++++++---------------
1 file changed, 12 insertions(+), 15 deletions(-)
diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
index 1e79202f2b..d320d032a7 100644
--- a/hw/vfio/igd.c
+++ b/hw/vfio/igd.c
@@ -368,7 +368,9 @@ static const MemoryRegionOps vfio_igd_index_quirk = {
void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
{
g_autofree struct vfio_region_info *rom = NULL;
- struct vfio_region_info *opregion = NULL, *host = NULL, *lpc = NULL;
+ g_autofree struct vfio_region_info *opregion = NULL;
+ g_autofree struct vfio_region_info *host = NULL;
+ g_autofree struct vfio_region_info *lpc = NULL;
VFIOQuirk *quirk;
VFIOIGDQuirk *igd;
PCIDevice *lpc_bridge;
@@ -426,7 +428,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
if ((ret || !rom->size) && !vdev->pdev.romfile) {
error_report("IGD device %s has no ROM, legacy mode disabled",
vdev->vbasedev.name);
- goto out;
+ return;
}
/*
@@ -437,7 +439,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
error_report("IGD device %s hotplugged, ROM disabled, "
"legacy mode disabled", vdev->vbasedev.name);
vdev->rom_read_failed = true;
- goto out;
+ return;
}
/*
@@ -450,7 +452,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
if (ret) {
error_report("IGD device %s does not support OpRegion access,"
"legacy mode disabled", vdev->vbasedev.name);
- goto out;
+ return;
}
ret = vfio_get_dev_region_info(&vdev->vbasedev,
@@ -459,7 +461,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
if (ret) {
error_report("IGD device %s does not support host bridge access,"
"legacy mode disabled", vdev->vbasedev.name);
- goto out;
+ return;
}
ret = vfio_get_dev_region_info(&vdev->vbasedev,
@@ -468,7 +470,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
if (ret) {
error_report("IGD device %s does not support LPC bridge access,"
"legacy mode disabled", vdev->vbasedev.name);
- goto out;
+ return;
}
gmch = vfio_pci_read_config(&vdev->pdev, IGD_GMCH, 4);
@@ -482,7 +484,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
error_report("IGD device %s failed to enable VGA access, "
"legacy mode disabled", vdev->vbasedev.name);
- goto out;
+ return;
}
/* Create our LPC/ISA bridge */
@@ -490,7 +492,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
if (ret) {
error_report("IGD device %s failed to create LPC bridge, "
"legacy mode disabled", vdev->vbasedev.name);
- goto out;
+ return;
}
/* Stuff some host values into the VM PCI host bridge */
@@ -498,14 +500,14 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
if (ret) {
error_report("IGD device %s failed to modify host bridge, "
"legacy mode disabled", vdev->vbasedev.name);
- goto out;
+ return;
}
/* Setup OpRegion access */
if (!vfio_pci_igd_opregion_init(vdev, opregion, &err)) {
error_append_hint(&err, "IGD legacy mode disabled\n");
error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
- goto out;
+ return;
}
/* Setup our quirk to munge GTT addresses to the VM allocated buffer */
@@ -607,9 +609,4 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
}
trace_vfio_pci_igd_bdsm_enabled(vdev->vbasedev.name, ggms_mb + gms_mb);
-
-out:
- g_free(opregion);
- g_free(host);
- g_free(lpc);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH v2 18/20] vfio/igd: Use g_autofree in vfio_probe_igd_bar4_quirk()
2024-05-22 4:40 ` [PATCH v2 18/20] vfio/igd: Use g_autofree in vfio_probe_igd_bar4_quirk() Zhenzhong Duan
@ 2024-05-22 7:14 ` Cédric Le Goater
0 siblings, 0 replies; 32+ messages in thread
From: Cédric Le Goater @ 2024-05-22 7:14 UTC (permalink / raw)
To: Zhenzhong Duan, qemu-devel; +Cc: alex.williamson, eric.auger, chao.p.peng
On 5/22/24 06:40, Zhenzhong Duan wrote:
> Pointer opregion, host and lpc are allocated and freed in
> vfio_probe_igd_bar4_quirk(). Use g_autofree to automatically
> free them.
>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
Thanks,
C.
> ---
> hw/vfio/igd.c | 27 ++++++++++++---------------
> 1 file changed, 12 insertions(+), 15 deletions(-)
>
> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
> index 1e79202f2b..d320d032a7 100644
> --- a/hw/vfio/igd.c
> +++ b/hw/vfio/igd.c
> @@ -368,7 +368,9 @@ static const MemoryRegionOps vfio_igd_index_quirk = {
> void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
> {
> g_autofree struct vfio_region_info *rom = NULL;
> - struct vfio_region_info *opregion = NULL, *host = NULL, *lpc = NULL;
> + g_autofree struct vfio_region_info *opregion = NULL;
> + g_autofree struct vfio_region_info *host = NULL;
> + g_autofree struct vfio_region_info *lpc = NULL;
> VFIOQuirk *quirk;
> VFIOIGDQuirk *igd;
> PCIDevice *lpc_bridge;
> @@ -426,7 +428,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
> if ((ret || !rom->size) && !vdev->pdev.romfile) {
> error_report("IGD device %s has no ROM, legacy mode disabled",
> vdev->vbasedev.name);
> - goto out;
> + return;
> }
>
> /*
> @@ -437,7 +439,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
> error_report("IGD device %s hotplugged, ROM disabled, "
> "legacy mode disabled", vdev->vbasedev.name);
> vdev->rom_read_failed = true;
> - goto out;
> + return;
> }
>
> /*
> @@ -450,7 +452,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
> if (ret) {
> error_report("IGD device %s does not support OpRegion access,"
> "legacy mode disabled", vdev->vbasedev.name);
> - goto out;
> + return;
> }
>
> ret = vfio_get_dev_region_info(&vdev->vbasedev,
> @@ -459,7 +461,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
> if (ret) {
> error_report("IGD device %s does not support host bridge access,"
> "legacy mode disabled", vdev->vbasedev.name);
> - goto out;
> + return;
> }
>
> ret = vfio_get_dev_region_info(&vdev->vbasedev,
> @@ -468,7 +470,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
> if (ret) {
> error_report("IGD device %s does not support LPC bridge access,"
> "legacy mode disabled", vdev->vbasedev.name);
> - goto out;
> + return;
> }
>
> gmch = vfio_pci_read_config(&vdev->pdev, IGD_GMCH, 4);
> @@ -482,7 +484,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
> error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> error_report("IGD device %s failed to enable VGA access, "
> "legacy mode disabled", vdev->vbasedev.name);
> - goto out;
> + return;
> }
>
> /* Create our LPC/ISA bridge */
> @@ -490,7 +492,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
> if (ret) {
> error_report("IGD device %s failed to create LPC bridge, "
> "legacy mode disabled", vdev->vbasedev.name);
> - goto out;
> + return;
> }
>
> /* Stuff some host values into the VM PCI host bridge */
> @@ -498,14 +500,14 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
> if (ret) {
> error_report("IGD device %s failed to modify host bridge, "
> "legacy mode disabled", vdev->vbasedev.name);
> - goto out;
> + return;
> }
>
> /* Setup OpRegion access */
> if (!vfio_pci_igd_opregion_init(vdev, opregion, &err)) {
> error_append_hint(&err, "IGD legacy mode disabled\n");
> error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> - goto out;
> + return;
> }
>
> /* Setup our quirk to munge GTT addresses to the VM allocated buffer */
> @@ -607,9 +609,4 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
> }
>
> trace_vfio_pci_igd_bdsm_enabled(vdev->vbasedev.name, ggms_mb + gms_mb);
> -
> -out:
> - g_free(opregion);
> - g_free(host);
> - g_free(lpc);
> }
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v2 19/20] vfio/ccw: Drop local @err in vfio_ccw_realize()
2024-05-22 4:39 [PATCH v2 00/20] VFIO: misc cleanups part2 Zhenzhong Duan
` (17 preceding siblings ...)
2024-05-22 4:40 ` [PATCH v2 18/20] vfio/igd: Use g_autofree in vfio_probe_igd_bar4_quirk() Zhenzhong Duan
@ 2024-05-22 4:40 ` Zhenzhong Duan
2024-05-22 7:50 ` Cédric Le Goater
2024-05-22 4:40 ` [PATCH v2 20/20] vfio/ccw: Fix the missed unrealize() call in error path Zhenzhong Duan
2024-05-22 9:32 ` [PATCH v2 00/20] VFIO: misc cleanups part2 Cédric Le Goater
20 siblings, 1 reply; 32+ messages in thread
From: Zhenzhong Duan @ 2024-05-22 4:40 UTC (permalink / raw)
To: qemu-devel
Cc: alex.williamson, clg, eric.auger, chao.p.peng, Zhenzhong Duan,
Eric Farman, Matthew Rosato, Thomas Huth, open list:vfio-ccw
Use @errp to fetch error information directly and drop the local
variable @err.
Suggested-by: Cédric Le Goater <clg@redhat.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
hw/vfio/ccw.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index 2600e62e37..168c9e5973 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -574,17 +574,17 @@ static void vfio_ccw_put_region(VFIOCCWDevice *vcdev)
static void vfio_ccw_realize(DeviceState *dev, Error **errp)
{
+ ERRP_GUARD();
S390CCWDevice *cdev = S390_CCW_DEVICE(dev);
VFIOCCWDevice *vcdev = VFIO_CCW(cdev);
S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev);
VFIODevice *vbasedev = &vcdev->vdev;
- Error *err = NULL;
/* Call the class init function for subchannel. */
if (cdc->realize) {
- cdc->realize(cdev, vcdev->vdev.sysfsdev, &err);
- if (err) {
- goto out_err_propagate;
+ cdc->realize(cdev, vcdev->vdev.sysfsdev, errp);
+ if (*errp) {
+ return;
}
}
@@ -597,27 +597,28 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
goto out_attach_dev_err;
}
- if (!vfio_ccw_get_region(vcdev, &err)) {
+ if (!vfio_ccw_get_region(vcdev, errp)) {
goto out_region_err;
}
- if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX, &err)) {
+ if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX, errp)) {
goto out_io_notifier_err;
}
if (vcdev->crw_region) {
if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_CRW_IRQ_INDEX,
- &err)) {
+ errp)) {
goto out_irq_notifier_err;
}
}
- if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_REQ_IRQ_INDEX, &err)) {
+ if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_REQ_IRQ_INDEX, errp)) {
/*
* Report this error, but do not make it a failing condition.
* Lack of this IRQ in the host does not prevent normal operation.
*/
- error_report_err(err);
+ error_report_err(*errp);
+ *errp = NULL;
}
return;
@@ -635,8 +636,6 @@ out_attach_dev_err:
if (cdc->unrealize) {
cdc->unrealize(cdev);
}
-out_err_propagate:
- error_propagate(errp, err);
}
static void vfio_ccw_unrealize(DeviceState *dev)
--
2.34.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH v2 19/20] vfio/ccw: Drop local @err in vfio_ccw_realize()
2024-05-22 4:40 ` [PATCH v2 19/20] vfio/ccw: Drop local @err in vfio_ccw_realize() Zhenzhong Duan
@ 2024-05-22 7:50 ` Cédric Le Goater
2024-05-22 8:01 ` Duan, Zhenzhong
0 siblings, 1 reply; 32+ messages in thread
From: Cédric Le Goater @ 2024-05-22 7:50 UTC (permalink / raw)
To: Zhenzhong Duan, qemu-devel
Cc: alex.williamson, eric.auger, chao.p.peng, Eric Farman,
Matthew Rosato, Thomas Huth, open list:vfio-ccw
On 5/22/24 06:40, Zhenzhong Duan wrote:
> Use @errp to fetch error information directly and drop the local
> variable @err.
>
> Suggested-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
> hw/vfio/ccw.c | 21 ++++++++++-----------
> 1 file changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index 2600e62e37..168c9e5973 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c
> @@ -574,17 +574,17 @@ static void vfio_ccw_put_region(VFIOCCWDevice *vcdev)
>
> static void vfio_ccw_realize(DeviceState *dev, Error **errp)
> {
> + ERRP_GUARD();
> S390CCWDevice *cdev = S390_CCW_DEVICE(dev);
> VFIOCCWDevice *vcdev = VFIO_CCW(cdev);
> S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev);
> VFIODevice *vbasedev = &vcdev->vdev;
> - Error *err = NULL;
>
> /* Call the class init function for subchannel. */
> if (cdc->realize) {
> - cdc->realize(cdev, vcdev->vdev.sysfsdev, &err);
> - if (err) {
> - goto out_err_propagate;
> + cdc->realize(cdev, vcdev->vdev.sysfsdev, errp);
> + if (*errp) {
> + return;
We should change the realize() return value to bool also. this is more
work and it should be addressed in its own patchset I think and ...
> }
> }
>
> @@ -597,27 +597,28 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
> goto out_attach_dev_err;
> }
>
> - if (!vfio_ccw_get_region(vcdev, &err)) {
> + if (!vfio_ccw_get_region(vcdev, errp)) {
> goto out_region_err;
> }
>
> - if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX, &err)) {
> + if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX, errp)) {
> goto out_io_notifier_err;
> }
>
> if (vcdev->crw_region) {
> if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_CRW_IRQ_INDEX,
> - &err)) {
> + errp)) {
> goto out_irq_notifier_err;
> }
> }
>
> - if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_REQ_IRQ_INDEX, &err)) {
> + if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_REQ_IRQ_INDEX, errp)) {
> /*
> * Report this error, but do not make it a failing condition.
> * Lack of this IRQ in the host does not prevent normal operation.
> */
> - error_report_err(err);
> + error_report_err(*errp);
This should use a local Error variable and be a warn_report_err instead.
Let's address these changes in another series. I can take care of it
later if no one does.
Thanks,
C.
> + *errp = NULL;
> }
>
> return;
> @@ -635,8 +636,6 @@ out_attach_dev_err:
> if (cdc->unrealize) {
> cdc->unrealize(cdev);
> }
> -out_err_propagate:
> - error_propagate(errp, err);
> }
>
> static void vfio_ccw_unrealize(DeviceState *dev)
^ permalink raw reply [flat|nested] 32+ messages in thread* RE: [PATCH v2 19/20] vfio/ccw: Drop local @err in vfio_ccw_realize()
2024-05-22 7:50 ` Cédric Le Goater
@ 2024-05-22 8:01 ` Duan, Zhenzhong
0 siblings, 0 replies; 32+ messages in thread
From: Duan, Zhenzhong @ 2024-05-22 8:01 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel@nongnu.org
Cc: alex.williamson@redhat.com, eric.auger@redhat.com, Peng, Chao P,
Eric Farman, Matthew Rosato, Thomas Huth, open list:vfio-ccw
>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Subject: Re: [PATCH v2 19/20] vfio/ccw: Drop local @err in vfio_ccw_realize()
>
>On 5/22/24 06:40, Zhenzhong Duan wrote:
>> Use @errp to fetch error information directly and drop the local
>> variable @err.
>>
>> Suggested-by: Cédric Le Goater <clg@redhat.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>> hw/vfio/ccw.c | 21 ++++++++++-----------
>> 1 file changed, 10 insertions(+), 11 deletions(-)
>>
>> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
>> index 2600e62e37..168c9e5973 100644
>> --- a/hw/vfio/ccw.c
>> +++ b/hw/vfio/ccw.c
>> @@ -574,17 +574,17 @@ static void
>vfio_ccw_put_region(VFIOCCWDevice *vcdev)
>>
>> static void vfio_ccw_realize(DeviceState *dev, Error **errp)
>> {
>> + ERRP_GUARD();
>> S390CCWDevice *cdev = S390_CCW_DEVICE(dev);
>> VFIOCCWDevice *vcdev = VFIO_CCW(cdev);
>> S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev);
>> VFIODevice *vbasedev = &vcdev->vdev;
>> - Error *err = NULL;
>>
>> /* Call the class init function for subchannel. */
>> if (cdc->realize) {
>> - cdc->realize(cdev, vcdev->vdev.sysfsdev, &err);
>> - if (err) {
>> - goto out_err_propagate;
>> + cdc->realize(cdev, vcdev->vdev.sysfsdev, errp);
>> + if (*errp) {
>> + return;
>
>We should change the realize() return value to bool also. this is more
>work and it should be addressed in its own patchset I think and ...
>
>> }
>> }
>>
>> @@ -597,27 +597,28 @@ static void vfio_ccw_realize(DeviceState *dev,
>Error **errp)
>> goto out_attach_dev_err;
>> }
>>
>> - if (!vfio_ccw_get_region(vcdev, &err)) {
>> + if (!vfio_ccw_get_region(vcdev, errp)) {
>> goto out_region_err;
>> }
>>
>> - if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX,
>&err)) {
>> + if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX,
>errp)) {
>> goto out_io_notifier_err;
>> }
>>
>> if (vcdev->crw_region) {
>> if (!vfio_ccw_register_irq_notifier(vcdev,
>VFIO_CCW_CRW_IRQ_INDEX,
>> - &err)) {
>> + errp)) {
>> goto out_irq_notifier_err;
>> }
>> }
>>
>> - if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_REQ_IRQ_INDEX,
>&err)) {
>> + if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_REQ_IRQ_INDEX,
>errp)) {
>> /*
>> * Report this error, but do not make it a failing condition.
>> * Lack of this IRQ in the host does not prevent normal operation.
>> */
>> - error_report_err(err);
>> + error_report_err(*errp);
>
>This should use a local Error variable and be a warn_report_err instead.
Yes.
>
>Let's address these changes in another series. I can take care of it
>later if no one does.
OK, leave it to you😊
Thanks
Zhenzhong
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v2 20/20] vfio/ccw: Fix the missed unrealize() call in error path
2024-05-22 4:39 [PATCH v2 00/20] VFIO: misc cleanups part2 Zhenzhong Duan
` (18 preceding siblings ...)
2024-05-22 4:40 ` [PATCH v2 19/20] vfio/ccw: Drop local @err in vfio_ccw_realize() Zhenzhong Duan
@ 2024-05-22 4:40 ` Zhenzhong Duan
2024-05-22 7:51 ` Cédric Le Goater
2024-05-22 9:32 ` [PATCH v2 00/20] VFIO: misc cleanups part2 Cédric Le Goater
20 siblings, 1 reply; 32+ messages in thread
From: Zhenzhong Duan @ 2024-05-22 4:40 UTC (permalink / raw)
To: qemu-devel
Cc: alex.williamson, clg, eric.auger, chao.p.peng, Zhenzhong Duan,
Eric Farman, Matthew Rosato, Thomas Huth, open list:vfio-ccw
When get name failed, we should call unrealize() so that
vfio_ccw_realize() is self contained.
Fixes: 909a6254eda ("vfio/ccw: Make vfio cdev pre-openable by passing a file handle")
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
hw/vfio/ccw.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index 168c9e5973..161704cd7b 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -589,7 +589,7 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
}
if (!vfio_device_get_name(vbasedev, errp)) {
- return;
+ goto out_unrealize;
}
if (!vfio_attach_device(cdev->mdevid, vbasedev,
@@ -633,6 +633,7 @@ out_region_err:
vfio_detach_device(vbasedev);
out_attach_dev_err:
g_free(vbasedev->name);
+out_unrealize:
if (cdc->unrealize) {
cdc->unrealize(cdev);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH v2 20/20] vfio/ccw: Fix the missed unrealize() call in error path
2024-05-22 4:40 ` [PATCH v2 20/20] vfio/ccw: Fix the missed unrealize() call in error path Zhenzhong Duan
@ 2024-05-22 7:51 ` Cédric Le Goater
2024-05-22 8:05 ` Duan, Zhenzhong
0 siblings, 1 reply; 32+ messages in thread
From: Cédric Le Goater @ 2024-05-22 7:51 UTC (permalink / raw)
To: Zhenzhong Duan, qemu-devel
Cc: alex.williamson, eric.auger, chao.p.peng, Eric Farman,
Matthew Rosato, Thomas Huth, open list:vfio-ccw
On 5/22/24 06:40, Zhenzhong Duan wrote:
> When get name failed, we should call unrealize() so that
> vfio_ccw_realize() is self contained.
>
> Fixes: 909a6254eda ("vfio/ccw: Make vfio cdev pre-openable by passing a file handle")
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
If the realize handler fails, the unrealize handler should be called.
See device_set_realized(). We should be fine without IMO.
Thanks,
C.
> ---
> hw/vfio/ccw.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index 168c9e5973..161704cd7b 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c
> @@ -589,7 +589,7 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
> }
>
> if (!vfio_device_get_name(vbasedev, errp)) {
> - return;
> + goto out_unrealize;
> }
>
> if (!vfio_attach_device(cdev->mdevid, vbasedev,
> @@ -633,6 +633,7 @@ out_region_err:
> vfio_detach_device(vbasedev);
> out_attach_dev_err:
> g_free(vbasedev->name);
> +out_unrealize:
> if (cdc->unrealize) {
> cdc->unrealize(cdev);
> }
^ permalink raw reply [flat|nested] 32+ messages in thread* RE: [PATCH v2 20/20] vfio/ccw: Fix the missed unrealize() call in error path
2024-05-22 7:51 ` Cédric Le Goater
@ 2024-05-22 8:05 ` Duan, Zhenzhong
2024-05-22 8:20 ` Cédric Le Goater
0 siblings, 1 reply; 32+ messages in thread
From: Duan, Zhenzhong @ 2024-05-22 8:05 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel@nongnu.org
Cc: alex.williamson@redhat.com, eric.auger@redhat.com, Peng, Chao P,
Eric Farman, Matthew Rosato, Thomas Huth, open list:vfio-ccw
Hi Cédric,
>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Sent: Wednesday, May 22, 2024 3:52 PM
>To: Duan, Zhenzhong <zhenzhong.duan@intel.com>; qemu-
>devel@nongnu.org
>Cc: alex.williamson@redhat.com; eric.auger@redhat.com; Peng, Chao P
><chao.p.peng@intel.com>; Eric Farman <farman@linux.ibm.com>; Matthew
>Rosato <mjrosato@linux.ibm.com>; Thomas Huth <thuth@redhat.com>;
>open list:vfio-ccw <qemu-s390x@nongnu.org>
>Subject: Re: [PATCH v2 20/20] vfio/ccw: Fix the missed unrealize() call in
>error path
>
>On 5/22/24 06:40, Zhenzhong Duan wrote:
>> When get name failed, we should call unrealize() so that
>> vfio_ccw_realize() is self contained.
>>
>> Fixes: 909a6254eda ("vfio/ccw: Make vfio cdev pre-openable by passing a
>file handle")
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>
>If the realize handler fails, the unrealize handler should be called.
>See device_set_realized(). We should be fine without IMO.
Do you mean when vfio_ccw_realize() fails, vfio_ccw_unrealize() will be called?
Looked into device_set_realized(), I didn't see where vfio_ccw_unrealize() was called.
Do I misunderstand?
Thanks
Zhenzhong
>
>
>Thanks,
>
>C.
>
>
>
>> ---
>> hw/vfio/ccw.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
>> index 168c9e5973..161704cd7b 100644
>> --- a/hw/vfio/ccw.c
>> +++ b/hw/vfio/ccw.c
>> @@ -589,7 +589,7 @@ static void vfio_ccw_realize(DeviceState *dev,
>Error **errp)
>> }
>>
>> if (!vfio_device_get_name(vbasedev, errp)) {
>> - return;
>> + goto out_unrealize;
>> }
>>
>> if (!vfio_attach_device(cdev->mdevid, vbasedev,
>> @@ -633,6 +633,7 @@ out_region_err:
>> vfio_detach_device(vbasedev);
>> out_attach_dev_err:
>> g_free(vbasedev->name);
>> +out_unrealize:
>> if (cdc->unrealize) {
>> cdc->unrealize(cdev);
>> }
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH v2 20/20] vfio/ccw: Fix the missed unrealize() call in error path
2024-05-22 8:05 ` Duan, Zhenzhong
@ 2024-05-22 8:20 ` Cédric Le Goater
0 siblings, 0 replies; 32+ messages in thread
From: Cédric Le Goater @ 2024-05-22 8:20 UTC (permalink / raw)
To: Duan, Zhenzhong, qemu-devel@nongnu.org
Cc: alex.williamson@redhat.com, eric.auger@redhat.com, Peng, Chao P,
Eric Farman, Matthew Rosato, Thomas Huth, open list:vfio-ccw
On 5/22/24 10:05, Duan, Zhenzhong wrote:
> Hi Cédric,
>
>> -----Original Message-----
>> From: Cédric Le Goater <clg@redhat.com>
>> Sent: Wednesday, May 22, 2024 3:52 PM
>> To: Duan, Zhenzhong <zhenzhong.duan@intel.com>; qemu-
>> devel@nongnu.org
>> Cc: alex.williamson@redhat.com; eric.auger@redhat.com; Peng, Chao P
>> <chao.p.peng@intel.com>; Eric Farman <farman@linux.ibm.com>; Matthew
>> Rosato <mjrosato@linux.ibm.com>; Thomas Huth <thuth@redhat.com>;
>> open list:vfio-ccw <qemu-s390x@nongnu.org>
>> Subject: Re: [PATCH v2 20/20] vfio/ccw: Fix the missed unrealize() call in
>> error path
>>
>> On 5/22/24 06:40, Zhenzhong Duan wrote:
>>> When get name failed, we should call unrealize() so that
>>> vfio_ccw_realize() is self contained.
>>>
>>> Fixes: 909a6254eda ("vfio/ccw: Make vfio cdev pre-openable by passing a
>> file handle")
>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>
>> If the realize handler fails, the unrealize handler should be called.
>> See device_set_realized(). We should be fine without IMO.
>
> Do you mean when vfio_ccw_realize() fails, vfio_ccw_unrealize() will be called?
> Looked into device_set_realized(), I didn't see where vfio_ccw_unrealize() was called.
> Do I misunderstand?
no, it's me. I thought it was called in the failure path :/ Anyhow, let's keep
this patch for a ccw series.
Thanks,
C.
>
> Thanks
> Zhenzhong
>
>>
>>
>> Thanks,
>>
>> C.
>>
>>
>>
>>> ---
>>> hw/vfio/ccw.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
>>> index 168c9e5973..161704cd7b 100644
>>> --- a/hw/vfio/ccw.c
>>> +++ b/hw/vfio/ccw.c
>>> @@ -589,7 +589,7 @@ static void vfio_ccw_realize(DeviceState *dev,
>> Error **errp)
>>> }
>>>
>>> if (!vfio_device_get_name(vbasedev, errp)) {
>>> - return;
>>> + goto out_unrealize;
>>> }
>>>
>>> if (!vfio_attach_device(cdev->mdevid, vbasedev,
>>> @@ -633,6 +633,7 @@ out_region_err:
>>> vfio_detach_device(vbasedev);
>>> out_attach_dev_err:
>>> g_free(vbasedev->name);
>>> +out_unrealize:
>>> if (cdc->unrealize) {
>>> cdc->unrealize(cdev);
>>> }
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2 00/20] VFIO: misc cleanups part2
2024-05-22 4:39 [PATCH v2 00/20] VFIO: misc cleanups part2 Zhenzhong Duan
` (19 preceding siblings ...)
2024-05-22 4:40 ` [PATCH v2 20/20] vfio/ccw: Fix the missed unrealize() call in error path Zhenzhong Duan
@ 2024-05-22 9:32 ` Cédric Le Goater
20 siblings, 0 replies; 32+ messages in thread
From: Cédric Le Goater @ 2024-05-22 9:32 UTC (permalink / raw)
To: Zhenzhong Duan, qemu-devel; +Cc: alex.williamson, eric.auger, chao.p.peng
On 5/22/24 06:39, Zhenzhong Duan wrote:
> Hi
>
> This is the last round of cleanup series to change functions in hw/vfio/
> to return bool when the error is passed through errp parameter.
>
> The first round is at
> https://lists.gnu.org/archive/html/qemu-devel/2024-05/msg01147.html
>
> I see Cédric is also working on some migration stuff cleanup,
> so didn't touch migration.c, but all other files in hw/vfio/ are cleanup now.
>
> Patch1 and patch20 are fix patch, all others are cleanup patches.
>
> Test done on x86 platform:
> vfio device hotplug/unplug with different backend
> reboot
>
> This series is rebased to https://github.com/legoater/qemu/tree/vfio-next
Patches 01-18 applied to vfio-next.
Thanks,
C.
^ permalink raw reply [flat|nested] 32+ messages in thread