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

[Auger Eric]

Markus,

On 22/09/2016 18:27, Markus Armbruster wrote:
> 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
I took into account the rewording suggestions.
> 
> 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.
There was error_report before in vfio_intx_enable_kvm but indeed testing
vfio_intx_enable_kvm()'s *errp now makes possible to detect issues.
> 
>>  
>> -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?
Thanks for catching this. It is a regression since in case KVM setup
fails we revert to userspace handled eventfds.

I replaced that by a warning instead.

Thanks!

Eric
> 
> 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;
>>          }
>