Re: [PATCH V1 10/26] vfio-pci: refactor for cpr

[Steven Sistare <steven.sistare@oracle.com>]

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 <steven.sistare@oracle.com>
>> ---
>>   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-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 @@ static void vfio_add_kvm_msi_virq(VFIOPCIDevice *vdev, VFIOMSIVector *vector,
>>                                                vector_n, &vdev->pdev);
>>   }
>> -static void vfio_connect_kvm_msi_virq(VFIOMSIVector *vector)
>> +static void vfio_connect_kvm_msi_virq(VFIOMSIVector *vector, int nr)
> 
> This change belongs to another patch.

OK.  It will be tiny, though -- add nr in the declaration and at 2 call sites.
It also runs counter to your advice above, which is do not add a parameter
until it is needed.

>>   {
>> +    const char *name = "kvm_interrupt";
>> +
>>       if (vector->virq < 0) {
>>           return;
>>       }
>> -    if (event_notifier_init(&vector->kvm_interrupt, 0)) {
>> +    if (vfio_notifier_init(vector->vdev, &vector->kvm_interrupt, name, nr)) {
>>           goto fail_notifier;
>>       }
>> @@ -489,19 +514,20 @@ static void vfio_connect_kvm_msi_virq(VFIOMSIVector *vector)
>>       return;
>>   fail_kvm:
>> -    event_notifier_cleanup(&vector->kvm_interrupt);
>> +    vfio_notifier_cleanup(vector->vdev, &vector->kvm_interrupt, name, nr);
>>   fail_notifier:
>>       kvm_irqchip_release_virq(kvm_state, vector->virq);
>>       vector->virq = -1;
>>   }
>> -static void vfio_remove_kvm_msi_virq(VFIOMSIVector *vector)
>> +static void vfio_remove_kvm_msi_virq(VFIOPCIDevice *vdev, VFIOMSIVector *vector,
>> +                                     int nr)
>>   {
>>       kvm_irqchip_remove_irqfd_notifier_gsi(kvm_state, &vector->kvm_interrupt,
>>                                             vector->virq);
>>       kvm_irqchip_release_virq(kvm_state, vector->virq);
>>       vector->virq = -1;
>> -    event_notifier_cleanup(&vector->kvm_interrupt);
>> +    vfio_notifier_cleanup(vdev, &vector->kvm_interrupt, "kvm_interrupt", nr);
>>   }
>>   static void vfio_update_kvm_msi_virq(VFIOMSIVector *vector, MSIMessage msg,
>> @@ -511,6 +537,20 @@ static void vfio_update_kvm_msi_virq(VFIOMSIVector *vector, MSIMessage msg,
>>       kvm_irqchip_commit_routes(kvm_state);
>>   }
>> +static void vfio_vector_init(VFIOPCIDevice *vdev, int nr)
>> +{
>> +    VFIOMSIVector *vector = &vdev->msi_vectors[nr];
>> +    PCIDevice *pdev = &vdev->pdev;
>> +
>> +    vector->vdev = vdev;
>> +    vector->virq = -1;
>> +    vfio_notifier_init(vdev, &vector->interrupt, "interrupt", nr);
>> +    vector->use = true;
>> +    if (vdev->interrupt == VFIO_INT_MSIX) {
>> +        msix_vector_use(pdev, nr);
>> +    }
>> +}
> 
> This change belongs to another patch.

OK.

- Steve

>>   static int vfio_msix_vector_do_use(PCIDevice *pdev, unsigned int nr,
>>                                      MSIMessage *msg, IOHandler *handler)
>>   {
>> @@ -524,13 +564,7 @@ static int vfio_msix_vector_do_use(PCIDevice *pdev, unsigned int nr,
>>       vector = &vdev->msi_vectors[nr];
>>       if (!vector->use) {
>> -        vector->vdev = vdev;
>> -        vector->virq = -1;
>> -        if (event_notifier_init(&vector->interrupt, 0)) {
>> -            error_report("vfio: Error: event_notifier_init failed");
>> -        }
>> -        vector->use = true;
>> -        msix_vector_use(pdev, nr);
>> +        vfio_vector_init(vdev, nr);
>>       }
>>       qemu_set_fd_handler(event_notifier_get_fd(&vector->interrupt),
>> @@ -542,7 +576,7 @@ static int vfio_msix_vector_do_use(PCIDevice *pdev, unsigned int nr,
>>        */
>>       if (vector->virq >= 0) {
>>           if (!msg) {
>> -            vfio_remove_kvm_msi_virq(vector);
>> +            vfio_remove_kvm_msi_virq(vdev, vector, nr);
>>           } else {
>>               vfio_update_kvm_msi_virq(vector, *msg, pdev);
>>           }
>> @@ -554,7 +588,7 @@ static int vfio_msix_vector_do_use(PCIDevice *pdev, unsigned int nr,
>>                   vfio_route_change = kvm_irqchip_begin_route_changes(kvm_state);
>>                   vfio_add_kvm_msi_virq(vdev, vector, nr, true);
>>                   kvm_irqchip_commit_route_changes(&vfio_route_change);
>> -                vfio_connect_kvm_msi_virq(vector);
>> +                vfio_connect_kvm_msi_virq(vector, nr);
>>               }
>>           }
>>       }
>> @@ -661,7 +695,7 @@ static void vfio_commit_kvm_msi_virq_batch(VFIOPCIDevice *vdev)
>>       kvm_irqchip_commit_route_changes(&vfio_route_change);
>>       for (i = 0; i < vdev->nr_vectors; i++) {
>> -        vfio_connect_kvm_msi_virq(&vdev->msi_vectors[i]);
>> +        vfio_connect_kvm_msi_virq(&vdev->msi_vectors[i], i);
>>       }
>>   }
>> @@ -741,9 +775,7 @@ retry:
>>           vector->virq = -1;
>>           vector->use = true;
>> -        if (event_notifier_init(&vector->interrupt, 0)) {
>> -            error_report("vfio: Error: event_notifier_init failed");
>> -        }
>> +        vfio_notifier_init(vdev, &vector->interrupt, "interrupt", i);
>>           qemu_set_fd_handler(event_notifier_get_fd(&vector->interrupt),
>>                               vfio_msi_interrupt, NULL, vector);
>> @@ -797,11 +829,11 @@ static void vfio_msi_disable_common(VFIOPCIDevice *vdev)
>>           VFIOMSIVector *vector = &vdev->msi_vectors[i];
>>           if (vdev->msi_vectors[i].use) {
>>               if (vector->virq >= 0) {
>> -                vfio_remove_kvm_msi_virq(vector);
>> +                vfio_remove_kvm_msi_virq(vdev, vector, i);
>>               }
>>               qemu_set_fd_handler(event_notifier_get_fd(&vector->interrupt),
>>                                   NULL, NULL, NULL);
>> -            event_notifier_cleanup(&vector->interrupt);
>> +            vfio_notifier_cleanup(vdev, &vector->interrupt, "interrupt", i);
>>           }
>>       }
>> @@ -2854,8 +2886,7 @@ static void vfio_register_err_notifier(VFIOPCIDevice *vdev)
>>           return;
>>       }
>> -    if (event_notifier_init(&vdev->err_notifier, 0)) {
>> -        error_report("vfio: Unable to init event notifier for error detection");
>> +    if (vfio_notifier_init(vdev, &vdev->err_notifier, "err_notifier", 0)) {
>>           vdev->pci_aer = false;
>>           return;
>>       }
>> @@ -2867,7 +2898,7 @@ static void vfio_register_err_notifier(VFIOPCIDevice *vdev)
>>                                   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);
>> +        vfio_notifier_cleanup(vdev, &vdev->err_notifier, "err_notifier", 0);
>>           vdev->pci_aer = false;
>>       }
>>   }
>> @@ -2886,7 +2917,7 @@ static void vfio_unregister_err_notifier(VFIOPCIDevice *vdev)
>>       }
>>       qemu_set_fd_handler(event_notifier_get_fd(&vdev->err_notifier),
>>                           NULL, NULL, vdev);
>> -    event_notifier_cleanup(&vdev->err_notifier);
>> +    vfio_notifier_cleanup(vdev, &vdev->err_notifier, "err_notifier", 0);
>>   }
>>   static void vfio_req_notifier_handler(void *opaque)
>> @@ -2920,8 +2951,7 @@ static void vfio_register_req_notifier(VFIOPCIDevice *vdev)
>>           return;
>>       }
>> -    if (event_notifier_init(&vdev->req_notifier, 0)) {
>> -        error_report("vfio: Unable to init event notifier for device request");
>> +    if (vfio_notifier_init(vdev, &vdev->req_notifier, "req_notifier", 0)) {
>>           return;
>>       }
>> @@ -2932,7 +2962,7 @@ static void vfio_register_req_notifier(VFIOPCIDevice *vdev)
>>                                   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);
>> +        vfio_notifier_cleanup(vdev, &vdev->req_notifier, "req_notifier", 0);
>>       } else {
>>           vdev->req_enabled = true;
>>       }
>> @@ -2952,7 +2982,7 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
>>       }
>>       qemu_set_fd_handler(event_notifier_get_fd(&vdev->req_notifier),
>>                           NULL, NULL, vdev);
>> -    event_notifier_cleanup(&vdev->req_notifier);
>> +    vfio_notifier_cleanup(vdev, &vdev->req_notifier, "req_notifier", 0);
>>       vdev->req_enabled = false;
>>   }
>