[v2,04/12] vfio/pci: Pass an error object to vfio_intx_enable
diff mbox

Message ID 1474404352-28958-5-git-send-email-eric.auger@redhat.com
State New
Headers show

Commit Message

Auger Eric Sept. 20, 2016, 8:45 p.m. UTC
Pass an error object to prepare for migration to VFIO-PCI realize.

The error object is propagated downto vfio_intx_enable_kvm

vfio_intx_update which calls vfio_intx_enable_kvm and
vfio_msi_disable_common/vfio_pci_post_reset which calls vfio_intx_enable
do not propagate the error and simply call error_reportf_err with the
ERR_PREFIX formatting.

Signed-off-by: Eric Auger <eric.auger@redhat.com>

---

v2: creation
---
 hw/vfio/pci.c | 39 +++++++++++++++++++++++++++------------
 1 file changed, 27 insertions(+), 12 deletions(-)

Comments

Markus Armbruster Sept. 22, 2016, 4:27 p.m. UTC | #1
Eric Auger <eric.auger@redhat.com> 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 <eric.auger@redhat.com>
>
> ---
>
> 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;
>          }
Auger Eric Sept. 30, 2016, 1:33 p.m. UTC | #2
Markus,

On 22/09/2016 18:27, Markus Armbruster wrote:
> Eric Auger <eric.auger@redhat.com> 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 <eric.auger@redhat.com>
>>
>> ---
>>
>> 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;
>>          }
>

Patch
diff mbox

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