* [PATCH 01/16] vfio/display: Fix error path in call site of ramfb_setup()
2024-05-15 8:20 [PATCH 00/16] VFIO: misc cleanups part2 Zhenzhong Duan
@ 2024-05-15 8:20 ` Zhenzhong Duan
2024-05-21 12:09 ` Cédric Le Goater
2024-05-15 8:20 ` [PATCH 02/16] vfio/display: Make vfio_display_*() return bool Zhenzhong Duan
` (15 subsequent siblings)
16 siblings, 1 reply; 39+ messages in thread
From: Zhenzhong Duan @ 2024-05-15 8:20 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>
---
hw/vfio/display.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/hw/vfio/display.c b/hw/vfio/display.c
index 1aa440c663..57c5ae0b2a 100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -359,6 +359,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;
@@ -486,6 +489,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] 39+ messages in thread* Re: [PATCH 01/16] vfio/display: Fix error path in call site of ramfb_setup()
2024-05-15 8:20 ` [PATCH 01/16] vfio/display: Fix error path in call site of ramfb_setup() Zhenzhong Duan
@ 2024-05-21 12:09 ` Cédric Le Goater
0 siblings, 0 replies; 39+ messages in thread
From: Cédric Le Goater @ 2024-05-21 12:09 UTC (permalink / raw)
To: Zhenzhong Duan, qemu-devel
Cc: alex.williamson, eric.auger, chao.p.peng, Gerd Hoffmann
On 5/15/24 10:20, Zhenzhong Duan wrote:
> 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>
Thanks,
C.
> ---
> hw/vfio/display.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/hw/vfio/display.c b/hw/vfio/display.c
> index 1aa440c663..57c5ae0b2a 100644
> --- a/hw/vfio/display.c
> +++ b/hw/vfio/display.c
> @@ -359,6 +359,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;
> @@ -486,6 +489,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;
> }
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 02/16] vfio/display: Make vfio_display_*() return bool
2024-05-15 8:20 [PATCH 00/16] VFIO: misc cleanups part2 Zhenzhong Duan
2024-05-15 8:20 ` [PATCH 01/16] vfio/display: Fix error path in call site of ramfb_setup() Zhenzhong Duan
@ 2024-05-15 8:20 ` Zhenzhong Duan
2024-05-21 12:14 ` Cédric Le Goater
2024-05-15 8:20 ` [PATCH 03/16] vfio/helpers: Use g_autofree in hw/vfio/helpers.c Zhenzhong Duan
` (14 subsequent siblings)
16 siblings, 1 reply; 39+ messages in thread
From: Zhenzhong Duan @ 2024-05-15 8:20 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>
---
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 57c5ae0b2a..b562f4be74 100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -346,11 +346,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);
@@ -360,11 +360,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)
@@ -481,7 +481,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,
@@ -490,10 +490,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)
@@ -508,7 +508,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;
@@ -531,11 +531,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] 39+ messages in thread* Re: [PATCH 02/16] vfio/display: Make vfio_display_*() return bool
2024-05-15 8:20 ` [PATCH 02/16] vfio/display: Make vfio_display_*() return bool Zhenzhong Duan
@ 2024-05-21 12:14 ` Cédric Le Goater
2024-05-22 4:45 ` Duan, Zhenzhong
0 siblings, 1 reply; 39+ messages in thread
From: Cédric Le Goater @ 2024-05-21 12:14 UTC (permalink / raw)
To: Zhenzhong Duan, qemu-devel; +Cc: alex.williamson, eric.auger, chao.p.peng
On 5/15/24 10:20, Zhenzhong Duan wrote:
> 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>
One comment below,
> ---
> 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 57c5ae0b2a..b562f4be74 100644
> --- a/hw/vfio/display.c
> +++ b/hw/vfio/display.c
> @@ -346,11 +346,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);
> @@ -360,11 +360,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);
vfio_display_edid_init() can fail for many reasons and does it silently.
It would be good to report the error in a future patch.
Thanks,
C.
> - return 0;
> + return true;
> }
>
> static void vfio_display_dmabuf_exit(VFIODisplay *dpy)
> @@ -481,7 +481,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,
> @@ -490,10 +490,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)
> @@ -508,7 +508,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;
> @@ -531,11 +531,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;
> }
> }
^ permalink raw reply [flat|nested] 39+ messages in thread* RE: [PATCH 02/16] vfio/display: Make vfio_display_*() return bool
2024-05-21 12:14 ` Cédric Le Goater
@ 2024-05-22 4:45 ` Duan, Zhenzhong
0 siblings, 0 replies; 39+ messages in thread
From: Duan, Zhenzhong @ 2024-05-22 4:45 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel@nongnu.org
Cc: alex.williamson@redhat.com, eric.auger@redhat.com, Peng, Chao P
>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Subject: Re: [PATCH 02/16] vfio/display: Make vfio_display_*() return bool
>
>On 5/15/24 10:20, Zhenzhong Duan wrote:
>> 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>
>
>One comment below,
>
>> ---
>> 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 57c5ae0b2a..b562f4be74 100644
>> --- a/hw/vfio/display.c
>> +++ b/hw/vfio/display.c
>> @@ -346,11 +346,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);
>> @@ -360,11 +360,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);
>
>vfio_display_edid_init() can fail for many reasons and does it silently.
>It would be good to report the error in a future patch.
Yes, that deserve a fix. Will address it with a future patch.
Thanks
Zhenzhong
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 03/16] vfio/helpers: Use g_autofree in hw/vfio/helpers.c
2024-05-15 8:20 [PATCH 00/16] VFIO: misc cleanups part2 Zhenzhong Duan
2024-05-15 8:20 ` [PATCH 01/16] vfio/display: Fix error path in call site of ramfb_setup() Zhenzhong Duan
2024-05-15 8:20 ` [PATCH 02/16] vfio/display: Make vfio_display_*() return bool Zhenzhong Duan
@ 2024-05-15 8:20 ` Zhenzhong Duan
2024-05-21 12:19 ` Cédric Le Goater
2024-05-15 8:20 ` [PATCH 04/16] vfio/helpers: Make vfio_set_irq_signaling() return bool Zhenzhong Duan
` (13 subsequent siblings)
16 siblings, 1 reply; 39+ messages in thread
From: Zhenzhong Duan @ 2024-05-15 8:20 UTC (permalink / raw)
To: qemu-devel; +Cc: alex.williamson, clg, eric.auger, chao.p.peng, Zhenzhong Duan
Changed functions include vfio_set_irq_signaling() and vfio_region_setup().
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
hw/vfio/helpers.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/hw/vfio/helpers.c b/hw/vfio/helpers.c
index 47b4096c05..0bb7b40a6a 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;
@@ -348,7 +347,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);
@@ -381,8 +380,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;
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH 03/16] vfio/helpers: Use g_autofree in hw/vfio/helpers.c
2024-05-15 8:20 ` [PATCH 03/16] vfio/helpers: Use g_autofree in hw/vfio/helpers.c Zhenzhong Duan
@ 2024-05-21 12:19 ` Cédric Le Goater
0 siblings, 0 replies; 39+ messages in thread
From: Cédric Le Goater @ 2024-05-21 12:19 UTC (permalink / raw)
To: Zhenzhong Duan, qemu-devel; +Cc: alex.williamson, eric.auger, chao.p.peng
On 5/15/24 10:20, Zhenzhong Duan wrote:
> Changed functions include vfio_set_irq_signaling()
this change looks fine
> and vfio_region_setup().
I would prefer all users of vfio_get_region_info() to be changed.
Thanks,
C.
>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
> hw/vfio/helpers.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/hw/vfio/helpers.c b/hw/vfio/helpers.c
> index 47b4096c05..0bb7b40a6a 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;
> @@ -348,7 +347,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);
> @@ -381,8 +380,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;
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 04/16] vfio/helpers: Make vfio_set_irq_signaling() return bool
2024-05-15 8:20 [PATCH 00/16] VFIO: misc cleanups part2 Zhenzhong Duan
` (2 preceding siblings ...)
2024-05-15 8:20 ` [PATCH 03/16] vfio/helpers: Use g_autofree in hw/vfio/helpers.c Zhenzhong Duan
@ 2024-05-15 8:20 ` Zhenzhong Duan
2024-05-21 12:24 ` Cédric Le Goater
2024-05-24 13:16 ` Anthony Krowiak
2024-05-15 8:20 ` [PATCH 05/16] vfio/helpers: Make vfio_device_get_name() " Zhenzhong Duan
` (12 subsequent siblings)
16 siblings, 2 replies; 39+ messages in thread
From: Zhenzhong Duan @ 2024-05-15 8:20 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>
---
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 2d8da32df4..fdce13f0f2 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 0bb7b40a6a..93e6fef6de 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] 39+ messages in thread* Re: [PATCH 04/16] vfio/helpers: Make vfio_set_irq_signaling() return bool
2024-05-15 8:20 ` [PATCH 04/16] vfio/helpers: Make vfio_set_irq_signaling() return bool Zhenzhong Duan
@ 2024-05-21 12:24 ` Cédric Le Goater
2024-05-24 13:16 ` Anthony Krowiak
1 sibling, 0 replies; 39+ messages in thread
From: Cédric Le Goater @ 2024-05-21 12:24 UTC (permalink / raw)
To: Zhenzhong Duan, qemu-devel
Cc: alex.williamson, eric.auger, chao.p.peng, Thomas Huth,
Tony Krowiak, Halil Pasic, Jason Herne, Eric Farman,
Matthew Rosato, open list:S390 general arch...
On 5/15/24 10:20, Zhenzhong Duan wrote:
> 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>
Thanks,
C.
> ---
> 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 2d8da32df4..fdce13f0f2 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 0bb7b40a6a..93e6fef6de 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;
> }
>
> /**
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH 04/16] vfio/helpers: Make vfio_set_irq_signaling() return bool
2024-05-15 8:20 ` [PATCH 04/16] vfio/helpers: Make vfio_set_irq_signaling() return bool Zhenzhong Duan
2024-05-21 12:24 ` Cédric Le Goater
@ 2024-05-24 13:16 ` Anthony Krowiak
2024-05-27 3:18 ` Duan, Zhenzhong
1 sibling, 1 reply; 39+ messages in thread
From: Anthony Krowiak @ 2024-05-24 13:16 UTC (permalink / raw)
To: Zhenzhong Duan, qemu-devel
Cc: alex.williamson, clg, eric.auger, chao.p.peng, Thomas Huth,
Halil Pasic, Jason Herne, Eric Farman, Matthew Rosato,
open list:S390 general arch...
On 5/15/24 4:20 AM, Zhenzhong Duan wrote:
> 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>
> ---
> 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 2d8da32df4..fdce13f0f2 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 0bb7b40a6a..93e6fef6de 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;
With this change, I don't see where the allocation of irq_set is is freed.
g_free(irq_set);
What am I missing?
> }
>
> - 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;
> }
>
> /**
^ permalink raw reply [flat|nested] 39+ messages in thread* RE: [PATCH 04/16] vfio/helpers: Make vfio_set_irq_signaling() return bool
2024-05-24 13:16 ` Anthony Krowiak
@ 2024-05-27 3:18 ` Duan, Zhenzhong
0 siblings, 0 replies; 39+ messages in thread
From: Duan, Zhenzhong @ 2024-05-27 3:18 UTC (permalink / raw)
To: Anthony Krowiak, qemu-devel@nongnu.org
Cc: alex.williamson@redhat.com, clg@redhat.com, eric.auger@redhat.com,
Peng, Chao P, Thomas Huth, Halil Pasic, Jason Herne, Eric Farman,
Matthew Rosato, open list:S390 general arch...
>-----Original Message-----
>From: Anthony Krowiak <akrowiak@linux.ibm.com>
>Subject: Re: [PATCH 04/16] vfio/helpers: Make vfio_set_irq_signaling()
>return bool
>
>
>On 5/15/24 4:20 AM, Zhenzhong Duan wrote:
>> 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>
>> ---
>> 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 2d8da32df4..fdce13f0f2 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 0bb7b40a6a..93e6fef6de 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;
>
>
>With this change, I don't see where the allocation of irq_set is is freed.
It's g_autofree type and freed implicitly.
Thanks
Zhenzhong
>
>g_free(irq_set);
>
>What am I missing?
>
>
>> }
>>
>> - 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;
>> }
>>
>> /**
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 05/16] vfio/helpers: Make vfio_device_get_name() return bool
2024-05-15 8:20 [PATCH 00/16] VFIO: misc cleanups part2 Zhenzhong Duan
` (3 preceding siblings ...)
2024-05-15 8:20 ` [PATCH 04/16] vfio/helpers: Make vfio_set_irq_signaling() return bool Zhenzhong Duan
@ 2024-05-15 8:20 ` Zhenzhong Duan
2024-05-21 12:25 ` Cédric Le Goater
2024-05-24 13:16 ` Anthony Krowiak
2024-05-15 8:20 ` [PATCH 06/16] vfio/platform: Make vfio_populate_device() and vfio_base_device_init() " Zhenzhong Duan
` (11 subsequent siblings)
16 siblings, 2 replies; 39+ messages in thread
From: Zhenzhong Duan @ 2024-05-15 8:20 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>
---
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 fdce13f0f2..d9891c796f 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -280,7 +280,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 93e6fef6de..a69b4411e5 100644
--- a/hw/vfio/helpers.c
+++ b/hw/vfio/helpers.c
@@ -605,7 +605,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;
@@ -614,7 +614,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) {
@@ -623,7 +623,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
@@ -634,7 +634,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] 39+ messages in thread* Re: [PATCH 05/16] vfio/helpers: Make vfio_device_get_name() return bool
2024-05-15 8:20 ` [PATCH 05/16] vfio/helpers: Make vfio_device_get_name() " Zhenzhong Duan
@ 2024-05-21 12:25 ` Cédric Le Goater
2024-05-24 13:16 ` Anthony Krowiak
1 sibling, 0 replies; 39+ messages in thread
From: Cédric Le Goater @ 2024-05-21 12:25 UTC (permalink / raw)
To: Zhenzhong Duan, qemu-devel
Cc: alex.williamson, eric.auger, chao.p.peng, Thomas Huth,
Tony Krowiak, Halil Pasic, Jason Herne, Eric Farman,
Matthew Rosato, open list:S390 general arch...
On 5/15/24 10:20, Zhenzhong Duan wrote:
> 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>
Thanks,
C.
> ---
> 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 fdce13f0f2..d9891c796f 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -280,7 +280,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 93e6fef6de..a69b4411e5 100644
> --- a/hw/vfio/helpers.c
> +++ b/hw/vfio/helpers.c
> @@ -605,7 +605,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;
> @@ -614,7 +614,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) {
> @@ -623,7 +623,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
> @@ -634,7 +634,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,
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH 05/16] vfio/helpers: Make vfio_device_get_name() return bool
2024-05-15 8:20 ` [PATCH 05/16] vfio/helpers: Make vfio_device_get_name() " Zhenzhong Duan
2024-05-21 12:25 ` Cédric Le Goater
@ 2024-05-24 13:16 ` Anthony Krowiak
1 sibling, 0 replies; 39+ messages in thread
From: Anthony Krowiak @ 2024-05-24 13:16 UTC (permalink / raw)
To: Zhenzhong Duan, qemu-devel
Cc: alex.williamson, clg, eric.auger, chao.p.peng, Thomas Huth,
Halil Pasic, Jason Herne, Eric Farman, Matthew Rosato,
open list:S390 general arch...
On 5/15/24 4:20 AM, Zhenzhong Duan wrote:
> 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>
> ---
> 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 fdce13f0f2..d9891c796f 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -280,7 +280,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;
> }
snip ...
>
> diff --git a/hw/vfio/helpers.c b/hw/vfio/helpers.c
> index 93e6fef6de..a69b4411e5 100644
> --- a/hw/vfio/helpers.c
> +++ b/hw/vfio/helpers.c
> @@ -605,7 +605,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;
> @@ -614,7 +614,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) {
> @@ -623,7 +623,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
> @@ -634,7 +634,7 @@ int vfio_device_get_name(VFIODevice *vbasedev, Error **errp)
> }
> }
>
> - return 0;
> + return true;
> }
For the two functions above:
Reviewed-by: Anthony Krowiak <akrowiak@linux.ibm.com>
>
snip ...
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 06/16] vfio/platform: Make vfio_populate_device() and vfio_base_device_init() return bool
2024-05-15 8:20 [PATCH 00/16] VFIO: misc cleanups part2 Zhenzhong Duan
` (4 preceding siblings ...)
2024-05-15 8:20 ` [PATCH 05/16] vfio/helpers: Make vfio_device_get_name() " Zhenzhong Duan
@ 2024-05-15 8:20 ` Zhenzhong Duan
2024-05-21 12:27 ` Cédric Le Goater
2024-05-15 8:20 ` [PATCH 07/16] vfio/ccw: Make vfio_ccw_get_region() return a bool Zhenzhong Duan
` (10 subsequent siblings)
16 siblings, 1 reply; 39+ messages in thread
From: Zhenzhong Duan @ 2024-05-15 8:20 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>
---
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] 39+ messages in thread* Re: [PATCH 06/16] vfio/platform: Make vfio_populate_device() and vfio_base_device_init() return bool
2024-05-15 8:20 ` [PATCH 06/16] vfio/platform: Make vfio_populate_device() and vfio_base_device_init() " Zhenzhong Duan
@ 2024-05-21 12:27 ` Cédric Le Goater
0 siblings, 0 replies; 39+ messages in thread
From: Cédric Le Goater @ 2024-05-21 12:27 UTC (permalink / raw)
To: Zhenzhong Duan, qemu-devel; +Cc: alex.williamson, eric.auger, chao.p.peng
On 5/15/24 10:20, Zhenzhong Duan wrote:
> 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>
Thanks,
C.
> ---
> 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 {
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 07/16] vfio/ccw: Make vfio_ccw_get_region() return a bool
2024-05-15 8:20 [PATCH 00/16] VFIO: misc cleanups part2 Zhenzhong Duan
` (5 preceding siblings ...)
2024-05-15 8:20 ` [PATCH 06/16] vfio/platform: Make vfio_populate_device() and vfio_base_device_init() " Zhenzhong Duan
@ 2024-05-15 8:20 ` Zhenzhong Duan
2024-05-21 12:34 ` Cédric Le Goater
2024-05-15 8:20 ` [PATCH 08/16] vfio/pci: Make vfio_intx_enable_kvm() " Zhenzhong Duan
` (9 subsequent siblings)
16 siblings, 1 reply; 39+ messages in thread
From: Zhenzhong Duan @ 2024-05-15 8:20 UTC (permalink / raw)
To: qemu-devel
Cc: alex.williamson, clg, eric.auger, chao.p.peng, Zhenzhong Duan,
Thomas Huth, Eric Farman, Matthew Rosato,
open list:S390 general arch...
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>
---
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] 39+ messages in thread* Re: [PATCH 07/16] vfio/ccw: Make vfio_ccw_get_region() return a bool
2024-05-15 8:20 ` [PATCH 07/16] vfio/ccw: Make vfio_ccw_get_region() return a bool Zhenzhong Duan
@ 2024-05-21 12:34 ` Cédric Le Goater
0 siblings, 0 replies; 39+ messages in thread
From: Cédric Le Goater @ 2024-05-21 12:34 UTC (permalink / raw)
To: Zhenzhong Duan, qemu-devel
Cc: alex.williamson, eric.auger, chao.p.peng, Thomas Huth,
Eric Farman, Matthew Rosato, open list:S390 general arch...
On 5/15/24 10:20, 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.
>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
One comment below
> ---
> 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)
vfio_ccw_realize() needs a little cleanup to avoid using the local
'Error *err' variable and the error_propagate() call at the end.
Thanks,
C.
> goto out_attach_dev_err;
> }
>
> - vfio_ccw_get_region(vcdev, &err);
> - if (err) {
> + if (!vfio_ccw_get_region(vcdev, &err)) {
> goto out_region_err;
> }
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 08/16] vfio/pci: Make vfio_intx_enable_kvm() return a bool
2024-05-15 8:20 [PATCH 00/16] VFIO: misc cleanups part2 Zhenzhong Duan
` (6 preceding siblings ...)
2024-05-15 8:20 ` [PATCH 07/16] vfio/ccw: Make vfio_ccw_get_region() return a bool Zhenzhong Duan
@ 2024-05-15 8:20 ` Zhenzhong Duan
2024-05-21 12:36 ` Cédric Le Goater
2024-05-15 8:20 ` [PATCH 09/16] vfio/pci: Make vfio_pci_relocate_msix() and vfio_msix_early_setup() " Zhenzhong Duan
` (8 subsequent siblings)
16 siblings, 1 reply; 39+ messages in thread
From: Zhenzhong Duan @ 2024-05-15 8:20 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>
---
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] 39+ messages in thread* Re: [PATCH 08/16] vfio/pci: Make vfio_intx_enable_kvm() return a bool
2024-05-15 8:20 ` [PATCH 08/16] vfio/pci: Make vfio_intx_enable_kvm() " Zhenzhong Duan
@ 2024-05-21 12:36 ` Cédric Le Goater
0 siblings, 0 replies; 39+ messages in thread
From: Cédric Le Goater @ 2024-05-21 12:36 UTC (permalink / raw)
To: Zhenzhong Duan, qemu-devel; +Cc: alex.williamson, eric.auger, chao.p.peng
On 5/15/24 10:20, Zhenzhong Duan wrote:
> 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>
Thanks,
C.
> ---
> 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);
> }
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 09/16] vfio/pci: Make vfio_pci_relocate_msix() and vfio_msix_early_setup() return a bool
2024-05-15 8:20 [PATCH 00/16] VFIO: misc cleanups part2 Zhenzhong Duan
` (7 preceding siblings ...)
2024-05-15 8:20 ` [PATCH 08/16] vfio/pci: Make vfio_intx_enable_kvm() " Zhenzhong Duan
@ 2024-05-15 8:20 ` Zhenzhong Duan
2024-05-21 12:38 ` Cédric Le Goater
2024-05-15 8:20 ` [PATCH 10/16] vfio/pci: Make vfio_populate_device() " Zhenzhong Duan
` (7 subsequent siblings)
16 siblings, 1 reply; 39+ messages in thread
From: Zhenzhong Duan @ 2024-05-15 8:20 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.
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
hw/vfio/pci.c | 32 ++++++++++++++++----------------
1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 12fb534d79..379cbad757 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,8 +3131,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
vfio_bars_prepare(vdev);
- vfio_msix_early_setup(vdev, &err);
- if (err) {
+ if (!vfio_msix_early_setup(vdev, &err)) {
error_propagate(errp, err);
goto error;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH 09/16] vfio/pci: Make vfio_pci_relocate_msix() and vfio_msix_early_setup() return a bool
2024-05-15 8:20 ` [PATCH 09/16] vfio/pci: Make vfio_pci_relocate_msix() and vfio_msix_early_setup() " Zhenzhong Duan
@ 2024-05-21 12:38 ` Cédric Le Goater
0 siblings, 0 replies; 39+ messages in thread
From: Cédric Le Goater @ 2024-05-21 12:38 UTC (permalink / raw)
To: Zhenzhong Duan, qemu-devel; +Cc: alex.williamson, eric.auger, chao.p.peng
On 5/15/24 10:20, 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.
>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
> hw/vfio/pci.c | 32 ++++++++++++++++----------------
> 1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 12fb534d79..379cbad757 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,8 +3131,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>
> vfio_bars_prepare(vdev);
>
> - vfio_msix_early_setup(vdev, &err);
> - if (err) {
> + if (!vfio_msix_early_setup(vdev, &err)) {
why not pass errp directly and avoid error_propagate() ?
Thanks,
C.
> error_propagate(errp, err);
> goto error;
> }
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 10/16] vfio/pci: Make vfio_populate_device() return a bool
2024-05-15 8:20 [PATCH 00/16] VFIO: misc cleanups part2 Zhenzhong Duan
` (8 preceding siblings ...)
2024-05-15 8:20 ` [PATCH 09/16] vfio/pci: Make vfio_pci_relocate_msix() and vfio_msix_early_setup() " Zhenzhong Duan
@ 2024-05-15 8:20 ` Zhenzhong Duan
2024-05-21 12:39 ` Cédric Le Goater
2024-05-15 8:20 ` [PATCH 11/16] vfio/pci: Make vfio_intx_enable() return bool Zhenzhong Duan
` (6 subsequent siblings)
16 siblings, 1 reply; 39+ messages in thread
From: Zhenzhong Duan @ 2024-05-15 8:20 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.
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
hw/vfio/pci.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 379cbad757..c091d21adf 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)
@@ -3036,8 +3038,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
goto error;
}
- vfio_populate_device(vdev, &err);
- if (err) {
+ if (!vfio_populate_device(vdev, &err)) {
error_propagate(errp, err);
goto error;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH 10/16] vfio/pci: Make vfio_populate_device() return a bool
2024-05-15 8:20 ` [PATCH 10/16] vfio/pci: Make vfio_populate_device() " Zhenzhong Duan
@ 2024-05-21 12:39 ` Cédric Le Goater
0 siblings, 0 replies; 39+ messages in thread
From: Cédric Le Goater @ 2024-05-21 12:39 UTC (permalink / raw)
To: Zhenzhong Duan, qemu-devel; +Cc: alex.williamson, eric.auger, chao.p.peng
On 5/15/24 10:20, 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.
>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
> hw/vfio/pci.c | 19 ++++++++++---------
> 1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 379cbad757..c091d21adf 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)
> @@ -3036,8 +3038,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
> goto error;
> }
>
> - vfio_populate_device(vdev, &err);
> - if (err) {
> + if (!vfio_populate_device(vdev, &err)) {
why not pass errp directly and avoid error_propagate() ?
Thanks,
C.
> error_propagate(errp, err);
> goto error;
> }
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 11/16] vfio/pci: Make vfio_intx_enable() return bool
2024-05-15 8:20 [PATCH 00/16] VFIO: misc cleanups part2 Zhenzhong Duan
` (9 preceding siblings ...)
2024-05-15 8:20 ` [PATCH 10/16] vfio/pci: Make vfio_populate_device() " Zhenzhong Duan
@ 2024-05-15 8:20 ` Zhenzhong Duan
2024-05-21 12:42 ` Cédric Le Goater
2024-05-15 8:20 ` [PATCH 12/16] vfio/pci: Make vfio_populate_vga() " Zhenzhong Duan
` (5 subsequent siblings)
16 siblings, 1 reply; 39+ messages in thread
From: Zhenzhong Duan @ 2024-05-15 8:20 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>
---
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 c091d21adf..e2ca4507f8 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);
}
@@ -3197,8 +3195,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] 39+ messages in thread* Re: [PATCH 11/16] vfio/pci: Make vfio_intx_enable() return bool
2024-05-15 8:20 ` [PATCH 11/16] vfio/pci: Make vfio_intx_enable() return bool Zhenzhong Duan
@ 2024-05-21 12:42 ` Cédric Le Goater
0 siblings, 0 replies; 39+ messages in thread
From: Cédric Le Goater @ 2024-05-21 12:42 UTC (permalink / raw)
To: Zhenzhong Duan, qemu-devel; +Cc: alex.williamson, eric.auger, chao.p.peng
On 5/15/24 10:20, Zhenzhong Duan wrote:
> 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>
Thanks,
C.
> ---
> 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 c091d21adf..e2ca4507f8 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);
> }
>
> @@ -3197,8 +3195,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;
> }
> }
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 12/16] vfio/pci: Make vfio_populate_vga() return bool
2024-05-15 8:20 [PATCH 00/16] VFIO: misc cleanups part2 Zhenzhong Duan
` (10 preceding siblings ...)
2024-05-15 8:20 ` [PATCH 11/16] vfio/pci: Make vfio_intx_enable() return bool Zhenzhong Duan
@ 2024-05-15 8:20 ` Zhenzhong Duan
2024-05-21 12:44 ` Cédric Le Goater
2024-05-15 8:20 ` [PATCH 13/16] vfio/pci: Make capability related functions " Zhenzhong Duan
` (4 subsequent siblings)
16 siblings, 1 reply; 39+ messages in thread
From: Zhenzhong Duan @ 2024-05-15 8:20 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>
---
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 e2ca4507f8..1922593253 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] 39+ messages in thread* Re: [PATCH 12/16] vfio/pci: Make vfio_populate_vga() return bool
2024-05-15 8:20 ` [PATCH 12/16] vfio/pci: Make vfio_populate_vga() " Zhenzhong Duan
@ 2024-05-21 12:44 ` Cédric Le Goater
0 siblings, 0 replies; 39+ messages in thread
From: Cédric Le Goater @ 2024-05-21 12:44 UTC (permalink / raw)
To: Zhenzhong Duan, qemu-devel; +Cc: alex.williamson, eric.auger, chao.p.peng
On 5/15/24 10:20, Zhenzhong Duan wrote:
> 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>
Thanks,
C.
> ---
> 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 e2ca4507f8..1922593253 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;
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 13/16] vfio/pci: Make capability related functions return bool
2024-05-15 8:20 [PATCH 00/16] VFIO: misc cleanups part2 Zhenzhong Duan
` (11 preceding siblings ...)
2024-05-15 8:20 ` [PATCH 12/16] vfio/pci: Make vfio_populate_vga() " Zhenzhong Duan
@ 2024-05-15 8:20 ` Zhenzhong Duan
2024-05-21 12:54 ` Cédric Le Goater
2024-05-15 8:20 ` [PATCH 14/16] vfio/pci: Use g_autofree for vfio_region_info pointer Zhenzhong Duan
` (3 subsequent siblings)
16 siblings, 1 reply; 39+ messages in thread
From: Zhenzhong Duan @ 2024-05-15 8:20 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>
---
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 1922593253..ecfbb9619f 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)
@@ -3136,8 +3132,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] 39+ messages in thread* Re: [PATCH 13/16] vfio/pci: Make capability related functions return bool
2024-05-15 8:20 ` [PATCH 13/16] vfio/pci: Make capability related functions " Zhenzhong Duan
@ 2024-05-21 12:54 ` Cédric Le Goater
0 siblings, 0 replies; 39+ messages in thread
From: Cédric Le Goater @ 2024-05-21 12:54 UTC (permalink / raw)
To: Zhenzhong Duan, qemu-devel; +Cc: alex.williamson, eric.auger, chao.p.peng
On 5/15/24 10:20, Zhenzhong Duan wrote:
> 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>
Thanks,
C.
> ---
> 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 1922593253..ecfbb9619f 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)
> @@ -3136,8 +3132,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;
> }
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 14/16] vfio/pci: Use g_autofree for vfio_region_info pointer
2024-05-15 8:20 [PATCH 00/16] VFIO: misc cleanups part2 Zhenzhong Duan
` (12 preceding siblings ...)
2024-05-15 8:20 ` [PATCH 13/16] vfio/pci: Make capability related functions " Zhenzhong Duan
@ 2024-05-15 8:20 ` Zhenzhong Duan
2024-05-21 12:48 ` Cédric Le Goater
2024-05-15 8:20 ` [PATCH 15/16] vfio/pci-quirks: Make vfio_pci_igd_opregion_init() return bool Zhenzhong Duan
` (2 subsequent siblings)
16 siblings, 1 reply; 39+ messages in thread
From: Zhenzhong Duan @ 2024-05-15 8:20 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>
---
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 ecfbb9619f..be87478716 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3146,7 +3146,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,
@@ -3165,7 +3165,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] 39+ messages in thread* Re: [PATCH 14/16] vfio/pci: Use g_autofree for vfio_region_info pointer
2024-05-15 8:20 ` [PATCH 14/16] vfio/pci: Use g_autofree for vfio_region_info pointer Zhenzhong Duan
@ 2024-05-21 12:48 ` Cédric Le Goater
0 siblings, 0 replies; 39+ messages in thread
From: Cédric Le Goater @ 2024-05-21 12:48 UTC (permalink / raw)
To: Zhenzhong Duan, qemu-devel; +Cc: alex.williamson, eric.auger, chao.p.peng
On 5/15/24 10:20, Zhenzhong Duan wrote:
> 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>
Thanks,
C.
> ---
> 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 ecfbb9619f..be87478716 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3146,7 +3146,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,
> @@ -3165,7 +3165,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;
> }
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 15/16] vfio/pci-quirks: Make vfio_pci_igd_opregion_init() return bool
2024-05-15 8:20 [PATCH 00/16] VFIO: misc cleanups part2 Zhenzhong Duan
` (13 preceding siblings ...)
2024-05-15 8:20 ` [PATCH 14/16] vfio/pci: Use g_autofree for vfio_region_info pointer Zhenzhong Duan
@ 2024-05-15 8:20 ` Zhenzhong Duan
2024-05-21 12:48 ` Cédric Le Goater
2024-05-15 8:20 ` [PATCH 16/16] vfio/pci-quirks: Make vfio_add_*_cap() " Zhenzhong Duan
2024-05-16 16:48 ` [PATCH 00/16] VFIO: misc cleanups part2 Cédric Le Goater
16 siblings, 1 reply; 39+ messages in thread
From: Zhenzhong Duan @ 2024-05-15 8:20 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>
---
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 be87478716..15823c359a 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3164,8 +3164,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] 39+ messages in thread* Re: [PATCH 15/16] vfio/pci-quirks: Make vfio_pci_igd_opregion_init() return bool
2024-05-15 8:20 ` [PATCH 15/16] vfio/pci-quirks: Make vfio_pci_igd_opregion_init() return bool Zhenzhong Duan
@ 2024-05-21 12:48 ` Cédric Le Goater
0 siblings, 0 replies; 39+ messages in thread
From: Cédric Le Goater @ 2024-05-21 12:48 UTC (permalink / raw)
To: Zhenzhong Duan, qemu-devel; +Cc: alex.williamson, eric.auger, chao.p.peng
On 5/15/24 10:20, Zhenzhong Duan wrote:
> 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>
Thanks,
C.
> ---
> 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 be87478716..15823c359a 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3164,8 +3164,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;
> }
> }
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 16/16] vfio/pci-quirks: Make vfio_add_*_cap() return bool
2024-05-15 8:20 [PATCH 00/16] VFIO: misc cleanups part2 Zhenzhong Duan
` (14 preceding siblings ...)
2024-05-15 8:20 ` [PATCH 15/16] vfio/pci-quirks: Make vfio_pci_igd_opregion_init() return bool Zhenzhong Duan
@ 2024-05-15 8:20 ` Zhenzhong Duan
2024-05-21 12:54 ` Cédric Le Goater
2024-05-16 16:48 ` [PATCH 00/16] VFIO: misc cleanups part2 Cédric Le Goater
16 siblings, 1 reply; 39+ messages in thread
From: Zhenzhong Duan @ 2024-05-15 8:20 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>
---
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 15823c359a..1254650d4a 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] 39+ messages in thread* Re: [PATCH 16/16] vfio/pci-quirks: Make vfio_add_*_cap() return bool
2024-05-15 8:20 ` [PATCH 16/16] vfio/pci-quirks: Make vfio_add_*_cap() " Zhenzhong Duan
@ 2024-05-21 12:54 ` Cédric Le Goater
0 siblings, 0 replies; 39+ messages in thread
From: Cédric Le Goater @ 2024-05-21 12:54 UTC (permalink / raw)
To: Zhenzhong Duan, qemu-devel; +Cc: alex.williamson, eric.auger, chao.p.peng
On 5/15/24 10:20, Zhenzhong Duan wrote:
> 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>
Thanks,
C.
> ---
> 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 15823c359a..1254650d4a 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;
> }
> }
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 00/16] VFIO: misc cleanups part2
2024-05-15 8:20 [PATCH 00/16] VFIO: misc cleanups part2 Zhenzhong Duan
` (15 preceding siblings ...)
2024-05-15 8:20 ` [PATCH 16/16] vfio/pci-quirks: Make vfio_add_*_cap() " Zhenzhong Duan
@ 2024-05-16 16:48 ` Cédric Le Goater
2024-05-17 9:25 ` Duan, Zhenzhong
16 siblings, 1 reply; 39+ messages in thread
From: Cédric Le Goater @ 2024-05-16 16:48 UTC (permalink / raw)
To: Zhenzhong Duan, qemu-devel; +Cc: alex.williamson, eric.auger, chao.p.peng
Hello Zhenzhong,
On 5/15/24 10:20, 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 is a 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
I queued part 1 in vfio-next with other changes. part 2 is in vfio-9.1
for now and should reach vfio-next after reviews next week.
Then, we have to work on your v5 [1] which should have all my attention
again after the next vfio PR. You, Joao and Eric have followups series
that need a resync on top of v5, possibly others [2] and [3], not sent
AFAICT. Anyhow, we will need inputs from these people and IOMMU
stakeholders/maintainers.
Thanks,
C.
[1] [PATCH v5 00/19] Add a host IOMMU device abstraction to check with vIOMMU
https://lore.kernel.org/qemu-devel/20240507092043.1172717-1-zhenzhong.duan@intel.com/
[2] [PATCH ats_vtd v2 00/25] ATS support for VT-d
https://lore.kernel.org/all/20240515071057.33990-1-clement.mathieu--drif@eviden.com/
[3] Add Tegra241 (Grace) CMDQV Support
https://lore.kernel.org/all/cover.1712978212.git.nicolinc@nvidia.com/
https://github.com/nicolinc/qemu/commits/wip/iommufd_vcmdq/
>
> Thanks
> Zhenzhong
>
> Zhenzhong Duan (16):
> vfio/display: Fix error path in call site of ramfb_setup()
> vfio/display: Make vfio_display_*() return bool
> vfio/helpers: Use g_autofree in hw/vfio/helpers.c
> vfio/helpers: Make vfio_set_irq_signaling() return bool
> vfio/helpers: Make vfio_device_get_name() return bool
> vfio/platform: Make vfio_populate_device() and vfio_base_device_init()
> return bool
> vfio/ccw: Make vfio_ccw_get_region() return a bool
> vfio/pci: Make vfio_intx_enable_kvm() return a bool
> vfio/pci: Make vfio_pci_relocate_msix() and vfio_msix_early_setup()
> return a bool
> vfio/pci: Make vfio_populate_device() return a bool
> vfio/pci: Make vfio_intx_enable() return bool
> vfio/pci: Make vfio_populate_vga() return bool
> vfio/pci: Make capability related functions return bool
> vfio/pci: Use g_autofree for vfio_region_info pointer
> vfio/pci-quirks: Make vfio_pci_igd_opregion_init() return bool
> vfio/pci-quirks: Make vfio_add_*_cap() return bool
>
> hw/vfio/pci.h | 12 +-
> include/hw/vfio/vfio-common.h | 6 +-
> hw/vfio/ap.c | 10 +-
> hw/vfio/ccw.c | 25 ++--
> hw/vfio/display.c | 22 ++--
> hw/vfio/helpers.c | 33 ++---
> hw/vfio/igd.c | 5 +-
> hw/vfio/pci-quirks.c | 50 ++++----
> hw/vfio/pci.c | 227 ++++++++++++++++------------------
> hw/vfio/platform.c | 61 ++++-----
> 10 files changed, 213 insertions(+), 238 deletions(-)
>
^ permalink raw reply [flat|nested] 39+ messages in thread* RE: [PATCH 00/16] VFIO: misc cleanups part2
2024-05-16 16:48 ` [PATCH 00/16] VFIO: misc cleanups part2 Cédric Le Goater
@ 2024-05-17 9:25 ` Duan, Zhenzhong
0 siblings, 0 replies; 39+ messages in thread
From: Duan, Zhenzhong @ 2024-05-17 9:25 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel@nongnu.org
Cc: alex.williamson@redhat.com, eric.auger@redhat.com, Peng, Chao P,
Joao Martins, Eric Auger, jasonwang@redhat.com,
Michael S. Tsirkin, nicolinc@nvidia.com, CLEMENT MATHIEU--DRIF
Hi Cédric,
>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Sent: Friday, May 17, 2024 12:48 AM
>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>
>Subject: Re: [PATCH 00/16] VFIO: misc cleanups part2
>
>Hello Zhenzhong,
>
>On 5/15/24 10:20, 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 is a 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
>
>I queued part 1 in vfio-next with other changes. part 2 is in vfio-9.1
>for now and should reach vfio-next after reviews next week.
>
>Then, we have to work on your v5 [1] which should have all my attention
>again after the next vfio PR. You, Joao and Eric have followups series
>that need a resync on top of v5, possibly others [2] and [3], not sent
>AFAICT. Anyhow, we will need inputs from these people and IOMMU
>stakeholders/maintainers.
Thanks for sharing the plan.
+Joao, Eric, Michael, Jason, Nicolin, Clement for their awareness.
On my side, I have rebased nesting series on top of v5[1],
the newest patches at https://github.com/yiliu1765/qemu/commits/zhenzhong/iommufd_nesting_rfcv2/
is under internal review, FYI.
Thanks
Zhenzhong
>
>Thanks,
>
>C.
>
>[1] [PATCH v5 00/19] Add a host IOMMU device abstraction to check with
>vIOMMU
> https://lore.kernel.org/qemu-devel/20240507092043.1172717-1-
>zhenzhong.duan@intel.com/
>
>[2] [PATCH ats_vtd v2 00/25] ATS support for VT-d
> https://lore.kernel.org/all/20240515071057.33990-1-clement.mathieu--
>drif@eviden.com/
>
>[3] Add Tegra241 (Grace) CMDQV Support
> https://lore.kernel.org/all/cover.1712978212.git.nicolinc@nvidia.com/
> https://github.com/nicolinc/qemu/commits/wip/iommufd_vcmdq/
>
>
>
>>
>> Thanks
>> Zhenzhong
>>
>> Zhenzhong Duan (16):
>> vfio/display: Fix error path in call site of ramfb_setup()
>> vfio/display: Make vfio_display_*() return bool
>> vfio/helpers: Use g_autofree in hw/vfio/helpers.c
>> vfio/helpers: Make vfio_set_irq_signaling() return bool
>> vfio/helpers: Make vfio_device_get_name() return bool
>> vfio/platform: Make vfio_populate_device() and vfio_base_device_init()
>> return bool
>> vfio/ccw: Make vfio_ccw_get_region() return a bool
>> vfio/pci: Make vfio_intx_enable_kvm() return a bool
>> vfio/pci: Make vfio_pci_relocate_msix() and vfio_msix_early_setup()
>> return a bool
>> vfio/pci: Make vfio_populate_device() return a bool
>> vfio/pci: Make vfio_intx_enable() return bool
>> vfio/pci: Make vfio_populate_vga() return bool
>> vfio/pci: Make capability related functions return bool
>> vfio/pci: Use g_autofree for vfio_region_info pointer
>> vfio/pci-quirks: Make vfio_pci_igd_opregion_init() return bool
>> vfio/pci-quirks: Make vfio_add_*_cap() return bool
>>
>> hw/vfio/pci.h | 12 +-
>> include/hw/vfio/vfio-common.h | 6 +-
>> hw/vfio/ap.c | 10 +-
>> hw/vfio/ccw.c | 25 ++--
>> hw/vfio/display.c | 22 ++--
>> hw/vfio/helpers.c | 33 ++---
>> hw/vfio/igd.c | 5 +-
>> hw/vfio/pci-quirks.c | 50 ++++----
>> hw/vfio/pci.c | 227 ++++++++++++++++------------------
>> hw/vfio/platform.c | 61 ++++-----
>> 10 files changed, 213 insertions(+), 238 deletions(-)
>>
^ permalink raw reply [flat|nested] 39+ messages in thread