Re: [PATCH V1 10/26] vfio-pci: refactor for cpr
On 2/4/2025 9:39 AM, Cédric Le Goater wrote:
On 1/29/25 15:43, Steve Sistare wrote:
Refactor vector use into a helper vfio_vector_init.
Add vfio_notifier_init and vfio_notifier_cleanup for named notifiers,
and pass additional arguments to vfio_remove_kvm_msi_virq.
All for use by CPR in a subsequent patch. No functional change.
Signed-off-by: Steve Sistare
---
hw/vfio/pci.c | 106 +-
1 file changed, 68 insertions(+), 38 deletions(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index ab17a98..24ebd69 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -54,6 +54,32 @@ static void vfio_disable_interrupts(VFIOPCIDevice *vdev);
static void vfio_mmap_set_enabled(VFIOPCIDevice *vdev, bool enabled);
static void vfio_msi_disable_common(VFIOPCIDevice *vdev);
+/* Create new or reuse existing eventfd */
+static int vfio_notifier_init(VFIOPCIDevice *vdev, EventNotifier *e,
+ const char *name, int nr)
+{
+ int fd = -1; /* placeholder until a subsequent patch */
+ int ret = 0;
+
+ if (fd >= 0) {
+ event_notifier_init_fd(e, fd);
Could you please first introduce the vfio_notifier_init() routine,
which can me merged quickly, and then, in a subsequent patch, modify
vfio_notifier_init() for CPR support.
OK.
+ } else {
+ ret = event_notifier_init(e, 0);
+ if (ret) {
+ Error *err = NULL;
+ error_setg_errno(&err, -ret, "vfio_notifier_init %s failed", name);
I don't think "name" is useful if the caller calls error_prepend() to
extend the error message.
I don't follow. The new code is strictly more informative than the old.
Some of the call sites before this patch printed generic messages such as
"event_notifier_init failed". The new code identifies the notifier that
failed.
+ error_report_err(err);
Nope. We should propagate the error with 'Error **' parameter and return
bool.
OK. Some call sites will simply report that error, though. And some ignore
errors. I intend to be bug-for-bug compatible with the old code, and not
introduce new behaviors.
+ }
+ }
+ return ret;
+}
+
+static void vfio_notifier_cleanup(VFIOPCIDevice *vdev, EventNotifier *e,
+ const char *name, int nr)
That's a lot of unused parameters which should be introduces when required.
OK.
+{
+ event_notifier_cleanup(e);
+}
+
/*
* Disabling BAR mmaping can be slow, but toggling it around INTx can
* also be a huge overhead. We try to get the best of both worlds by
@@ -134,8 +160,8 @@ static bool vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error
**errp)
pci_irq_deassert(&vdev->pdev);
/* Get an eventfd for resample/unmask */
- if (event_notifier_init(&vdev->intx.unmask, 0)) {
- error_setg(errp, "event_notifier_init failed eoi");
+ if (vfio_notifier_init(vdev, &vdev->intx.unmask, "intx-unmask", 0)) {
+ error_setg(errp, "vfio_notifier_init intx-unmask failed");
goto fail;
}
@@ -167,7 +193,7 @@ fail_vfio:
kvm_irqchip_remove_irqfd_notifier_gsi(kvm_state, &vdev->intx.interrupt,
vdev->intx.route.irq);
fail_irqfd:
- event_notifier_cleanup(&vdev->intx.unmask);
+ vfio_notifier_cleanup(vdev, &vdev->intx.unmask, "intx-unmask", 0);
fail:
qemu_set_fd_handler(irq_fd, vfio_intx_interrupt, NULL, vdev);
vfio_unmask_single_irqindex(&vdev->vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
@@ -199,7 +225,7 @@ static void vfio_intx_disable_kvm(VFIOPCIDevice *vdev)
}
/* We only need to close the eventfd for VFIO to cleanup the kernel side
*/
- event_notifier_cleanup(&vdev->intx.unmask);
+ vfio_notifier_cleanup(vdev, &vdev->intx.unmask, "intx-unmask", 0);
/* QEMU starts listening for interrupt events. */
qemu_set_fd_handler(event_notifier_get_fd(&vdev->intx.interrupt),
@@ -266,7 +292,6 @@ 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;
int32_t fd;
- int ret;
if (!pin) {
@@ -289,9 +314,7 @@ static bool vfio_intx_enable(VFIOPCIDevice *vdev, Error
**errp)
}
#endif
- ret = event_notifier_init(&vdev->intx.interrupt, 0);
- if (ret) {
- error_setg_errno(errp, -ret, "event_notifier_init failed");
+ if (vfio_notifier_init(vdev, &vdev->intx.interrupt, "intx-interrupt", 0)) {
return false;
}
fd = event_notifier_get_fd(&vdev->intx.interrupt);
@@ -300,7 +323,7 @@ static bool vfio_intx_enable(VFIOPCIDevice *vdev, Error
**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);
+ vfio_notifier_cleanup(vdev, &vdev->intx.interrupt, "intx-
Re: [PATCH V1 10/26] vfio-pci: refactor for cpr
On 1/29/25 15:43, Steve Sistare wrote:
Refactor vector use into a helper vfio_vector_init.
Add vfio_notifier_init and vfio_notifier_cleanup for named notifiers,
and pass additional arguments to vfio_remove_kvm_msi_virq.
All for use by CPR in a subsequent patch. No functional change.
Signed-off-by: Steve Sistare
---
hw/vfio/pci.c | 106 +-
1 file changed, 68 insertions(+), 38 deletions(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index ab17a98..24ebd69 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -54,6 +54,32 @@ static void vfio_disable_interrupts(VFIOPCIDevice *vdev);
static void vfio_mmap_set_enabled(VFIOPCIDevice *vdev, bool enabled);
static void vfio_msi_disable_common(VFIOPCIDevice *vdev);
+/* Create new or reuse existing eventfd */
+static int vfio_notifier_init(VFIOPCIDevice *vdev, EventNotifier *e,
+ const char *name, int nr)
+{
+int fd = -1; /* placeholder until a subsequent patch */
+int ret = 0;
+
+if (fd >= 0) {
+event_notifier_init_fd(e, fd);
Could you please first introduce the vfio_notifier_init() routine,
which can me merged quickly, and then, in a subsequent patch, modify
vfio_notifier_init() for CPR support.
+} else {
+ret = event_notifier_init(e, 0);
+if (ret) {
+Error *err = NULL;
+error_setg_errno(&err, -ret, "vfio_notifier_init %s failed", name);
I don't think "name" is useful if the caller calls error_prepend() to
extend the error message.
+error_report_err(err);
Nope. We should propagate the error with 'Error **' parameter and return
bool.
+}
+}
+return ret;
+}
+
+static void vfio_notifier_cleanup(VFIOPCIDevice *vdev, EventNotifier *e,
+ const char *name, int nr)
That's a lot of unused parameters which should be introduces when required.
+{
+event_notifier_cleanup(e);
+}
+
/*
* Disabling BAR mmaping can be slow, but toggling it around INTx can
* also be a huge overhead. We try to get the best of both worlds by
@@ -134,8 +160,8 @@ static bool vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error
**errp)
pci_irq_deassert(&vdev->pdev);
/* Get an eventfd for resample/unmask */
-if (event_notifier_init(&vdev->intx.unmask, 0)) {
-error_setg(errp, "event_notifier_init failed eoi");
+if (vfio_notifier_init(vdev, &vdev->intx.unmask, "intx-unmask", 0)) {
+error_setg(errp, "vfio_notifier_init intx-unmask failed");
goto fail;
}
@@ -167,7 +193,7 @@ fail_vfio:
kvm_irqchip_remove_irqfd_notifier_gsi(kvm_state, &vdev->intx.interrupt,
vdev->intx.route.irq);
fail_irqfd:
-event_notifier_cleanup(&vdev->intx.unmask);
+vfio_notifier_cleanup(vdev, &vdev->intx.unmask, "intx-unmask", 0);
fail:
qemu_set_fd_handler(irq_fd, vfio_intx_interrupt, NULL, vdev);
vfio_unmask_single_irqindex(&vdev->vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
@@ -199,7 +225,7 @@ static void vfio_intx_disable_kvm(VFIOPCIDevice *vdev)
}
/* We only need to close the eventfd for VFIO to cleanup the kernel side */
-event_notifier_cleanup(&vdev->intx.unmask);
+vfio_notifier_cleanup(vdev, &vdev->intx.unmask, "intx-unmask", 0);
/* QEMU starts listening for interrupt events. */
qemu_set_fd_handler(event_notifier_get_fd(&vdev->intx.interrupt),
@@ -266,7 +292,6 @@ 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;
int32_t fd;
-int ret;
if (!pin) {
@@ -289,9 +314,7 @@ static bool vfio_intx_enable(VFIOPCIDevice *vdev, Error
**errp)
}
#endif
-ret = event_notifier_init(&vdev->intx.interrupt, 0);
-if (ret) {
-error_setg_errno(errp, -ret, "event_notifier_init failed");
+if (vfio_notifier_init(vdev, &vdev->intx.interrupt, "intx-interrupt", 0)) {
return false;
}
fd = event_notifier_get_fd(&vdev->intx.interrupt);
@@ -300,7 +323,7 @@ static bool vfio_intx_enable(VFIOPCIDevice *vdev, Error
**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);
+vfio_notifier_cleanup(vdev, &vdev->intx.interrupt, "intx-interrupt",
0);
return false;
}
@@ -327,7 +350,7 @@ static void vfio_intx_disable(VFIOPCIDevice *vdev)
fd = event_notifier_get_fd(&vdev->intx.interrupt);
qemu_set_fd_handler(fd, NULL, NULL, vdev);
-event_notifier_cleanup(&vdev->intx.interrupt);
+vfio_notifier_cleanup(vdev, &vdev->intx.interrupt, "intx-interrupt", 0);
vdev->interrupt = VFIO_INT_NONE;
@@ -471,13 +494,15 @@ stati
