Re: [Qemu-devel] [PATCH v2 04/12] vfio/pci: Pass an error object to vfio_intx_enable

[Markus Armbruster]

Eric Auger <address@hidden> writes:

> Pass an error object to prepare for migration to VFIO-PCI realize.
>
> The error object is propagated downto vfio_intx_enable_kvm

down to vfio_intx_enable_kvm().

(feel free to omit the () I automatically add after a function name)

> vfio_intx_update which calls vfio_intx_enable_kvm and
> vfio_msi_disable_common/vfio_pci_post_reset which calls vfio_intx_enable

Suggest: The three other callers vfio_intx_update,
vfio_msi_disable_common() and vfio_pci_post_reset()

> do not propagate the error and simply call error_reportf_err with the
> ERR_PREFIX formatting.
>
> Signed-off-by: Eric Auger <address@hidden>
>
> ---
>
> v2: creation
> ---
>  hw/vfio/pci.c | 39 +++++++++++++++++++++++++++------------
>  1 file changed, 27 insertions(+), 12 deletions(-)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index b35925a..f67eec4 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -100,7 +100,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)
> +static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp)
>  {
>  #ifdef CONFIG_KVM
>      struct kvm_irqfd irqfd = {
> @@ -126,7 +126,7 @@ static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev)
>  
>      /* Get an eventfd for resample/unmask */
>      if (event_notifier_init(&vdev->intx.unmask, 0)) {
> -        error_report("vfio: Error: event_notifier_init failed eoi");
> +        error_setg(errp, "event_notifier_init failed eoi");
>          goto fail;
>      }
>  
> @@ -134,7 +134,7 @@ static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev)
>      irqfd.resamplefd = event_notifier_get_fd(&vdev->intx.unmask);
>  
>      if (kvm_vm_ioctl(kvm_state, KVM_IRQFD, &irqfd)) {
> -        error_report("vfio: Error: Failed to setup resample irqfd: %m");
> +        error_setg_errno(errp, errno, "failed to setup resample irqfd");
>          goto fail_irqfd;
>      }
>  
> @@ -153,7 +153,7 @@ static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev)
>      ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
>      g_free(irq_set);
>      if (ret) {
> -        error_report("vfio: Error: Failed to setup INTx unmask fd: %m");
> +        error_setg_errno(errp, -ret, "failed to setup INTx unmask fd");
>          goto fail_vfio;
>      }
>  
> @@ -222,6 +222,7 @@ static void vfio_intx_update(PCIDevice *pdev)
>  {
>      VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
>      PCIINTxRoute route;
> +    Error *err = NULL;
>  
>      if (vdev->interrupt != VFIO_INT_INTx) {
>          return;
> @@ -244,18 +245,22 @@ static void vfio_intx_update(PCIDevice *pdev)
>          return;
>      }
>  
> -    vfio_intx_enable_kvm(vdev);
> +    vfio_intx_enable_kvm(vdev, &err);
> +    if (err) {
> +        error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name);
> +    }
>  
>      /* Re-enable the interrupt in cased we missed an EOI */
>      vfio_intx_eoi(&vdev->vbasedev);
>  }

Nate that this function now permits callers to detect failure.

>  
> -static int vfio_intx_enable(VFIOPCIDevice *vdev)
> +static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
>  {
>      uint8_t pin = vfio_pci_read_config(&vdev->pdev, PCI_INTERRUPT_PIN, 1);
>      int ret, argsz;
>      struct vfio_irq_set *irq_set;
>      int32_t *pfd;
> +    Error *err = NULL;
>  
>      if (!pin) {
>          return 0;
> @@ -279,7 +284,7 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev)
>  
>      ret = event_notifier_init(&vdev->intx.interrupt, 0);
>      if (ret) {
> -        error_report("vfio: Error: event_notifier_init failed");
> +        error_setg_errno(errp, -ret, "event_notifier_init failed");
>          return ret;
>      }
>  
> @@ -299,13 +304,14 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev)
>      ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
>      g_free(irq_set);
>      if (ret) {
> -        error_report("vfio: Error: Failed to setup INTx fd: %m");
> +        error_setg_errno(errp, -ret, "failed to setup INTx fd");
>          qemu_set_fd_handler(*pfd, NULL, NULL, vdev);
>          event_notifier_cleanup(&vdev->intx.interrupt);
>          return -errno;
>      }
>  
> -    vfio_intx_enable_kvm(vdev);
> +    vfio_intx_enable_kvm(vdev, &err);
> +    error_propagate(errp, err);

This wasn't an error before.  Bug fix or regression?

Since you don't examine err, you should simply

       vfio_intx_enable_kvm(errp)

>      vdev->interrupt = VFIO_INT_INTx;
>  
> @@ -707,6 +713,7 @@ retry:
>  
>  static void vfio_msi_disable_common(VFIOPCIDevice *vdev)
>  {
> +    Error *err = NULL;
>      int i;
>  
>      for (i = 0; i < vdev->nr_vectors; i++) {
> @@ -726,7 +733,10 @@ static void vfio_msi_disable_common(VFIOPCIDevice *vdev)
>      vdev->nr_vectors = 0;
>      vdev->interrupt = VFIO_INT_NONE;
>  
> -    vfio_intx_enable(vdev);
> +    vfio_intx_enable(vdev, &err);
> +    if (err) {
> +        error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name);
> +    }
>  }
>  
>  static void vfio_msix_disable(VFIOPCIDevice *vdev)
> @@ -1908,7 +1918,12 @@ static void vfio_pci_pre_reset(VFIOPCIDevice *vdev)
>  
>  static void vfio_pci_post_reset(VFIOPCIDevice *vdev)
>  {
> -    vfio_intx_enable(vdev);
> +    Error *err = NULL;
> +
> +    vfio_intx_enable(vdev, &err);
> +    if (err) {
> +        error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name);
> +    }
>  }
>  
>  static bool vfio_pci_host_match(PCIHostDeviceAddress *addr, const char *name)
> @@ -2722,7 +2737,7 @@ static int vfio_initfn(PCIDevice *pdev)
>          vdev->intx.mmap_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
>                                                    vfio_intx_mmap_enable, 
> vdev);
>          pci_device_set_intx_routing_notifier(&vdev->pdev, vfio_intx_update);
> -        ret = vfio_intx_enable(vdev);
> +        ret = vfio_intx_enable(vdev, &err);
>          if (ret) {
>              goto out_teardown;
>          }