[Qemu-devel] [PATCH 0/2] hw/vfio/platform: irqfd setup changes

[Qemu-devel] [PATCH 0/2] hw/vfio/platform: irqfd setup changes

[Eric Auger]

This series fixes a bug related to irqfd setup for edge sensitive IRQs
and proposes a new startup sequence for irqfd signaling.

The current startup sequence brings some issues with respect to the
oncoming ARM IRQ forwarding support. The new startup sequence starts
either irqfd signaling or eventfd signaling and there is no risk the
IRQ is active nor masked when irqfd/IRQ forwarding is setup.

Eric Auger (2):
  hw/vfio/platform: irqfd setup sequence update
  hw/vfio/platform: do not set resamplefd for edge-sensitive IRQS

 hw/vfio/platform.c | 55 ++++++++++++++++++++++++++++++++----------------------
 1 file changed, 33 insertions(+), 22 deletions(-)

-- 
1.8.3.2

[Qemu-devel] [PATCH 1/2] hw/vfio/platform: irqfd setup sequence update

[Eric Auger]

With current implementation, eventfd VFIO signaling is first set up and
then irqfd is setup, if supported and allowed.

This start sequence causes several issues with IRQ forwarding setup
which, if supported, is transparently attempted on irqfd setup:
IRQ forwarding setup is likely to fail if the IRQ is detected as under
injection into the guest (active at irqchip level or VFIO masked).

This currently always happens because the current sequence explicitly
VFIO-masks the IRQ before setting irqfd.

Even if that masking were removed, we couldn't prevent the case where
the IRQ is under injection into the guest.

So the simpler solution is to remove this 2-step startup and directly
attempt irqfd setup. This is what this patch does.

Also in case the eventfd setup fails, there is no reason to go farther:
let's abort.

Signed-off-by: Eric Auger <eric.auger@linaro.org>
---
 hw/vfio/platform.c | 51 +++++++++++++++++++++++++++++----------------------
 1 file changed, 29 insertions(+), 22 deletions(-)

diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index de4ec52..38eaccf 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -310,18 +310,29 @@ static void vfio_platform_eoi(VFIODevice *vbasedev)
 /**
  * vfio_start_eventfd_injection - starts the virtual IRQ injection using
  * user-side handled eventfds
- * @intp: the IRQ struct pointer
+ * @sbdev: the sysbus device handle
+ * @irq: the qemu irq handle
  */
 
-static int vfio_start_eventfd_injection(VFIOINTp *intp)
+static void vfio_start_eventfd_injection(SysBusDevice *sbdev, qemu_irq irq)
 {
     int ret;
+    VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
+    VFIOINTp *intp;
+
+    QLIST_FOREACH(intp, &vdev->intp_list, next) {
+        if (intp->qemuirq == irq) {
+            break;
+        }
+    }
+    assert(intp);
 
     ret = vfio_set_trigger_eventfd(intp, vfio_intp_interrupt);
     if (ret) {
-        error_report("vfio: Error: Failed to pass IRQ fd to the driver: %m");
+        error_report("vfio: failed to start eventfd signaling for IRQ %d: %m",
+                     intp->pin);
+        abort();
     }
-    return ret;
 }
 
 /*
@@ -359,6 +370,15 @@ static int vfio_set_resample_eventfd(VFIOINTp *intp)
     return ret;
 }
 
+/**
+ * vfio_start_irqfd_injection - starts the virtual IRQ injection using
+ * irqfd
+ *
+ * @sbdev: the sysbus device handle
+ * @irq: the qemu irq handle
+ *
+ * In case the irqfd setup fails, we fallback to userspace handled eventfd
+ */
 static void vfio_start_irqfd_injection(SysBusDevice *sbdev, qemu_irq irq)
 {
     VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
@@ -366,7 +386,7 @@ static void vfio_start_irqfd_injection(SysBusDevice *sbdev, qemu_irq irq)
 
     if (!kvm_irqfds_enabled() || !kvm_resamplefds_enabled() ||
         !vdev->irqfd_allowed) {
-        return;
+        goto fail_irqfd;
     }
 
     QLIST_FOREACH(intp, &vdev->intp_list, next) {
@@ -376,13 +396,6 @@ static void vfio_start_irqfd_injection(SysBusDevice *sbdev, qemu_irq irq)
     }
     assert(intp);
 
-    /* Get to a known interrupt state */
-    qemu_set_fd_handler(event_notifier_get_fd(&intp->interrupt),
-                        NULL, NULL, vdev);
-
-    vfio_mask_single_irqindex(&vdev->vbasedev, intp->pin);
-    qemu_set_irq(intp->qemuirq, 0);
-
     if (kvm_irqchip_add_irqfd_notifier(kvm_state, &intp->interrupt,
                                    &intp->unmask, irq) < 0) {
         goto fail_irqfd;
@@ -395,9 +408,6 @@ static void vfio_start_irqfd_injection(SysBusDevice *sbdev, qemu_irq irq)
         goto fail_vfio;
     }
 
-    /* Let's resume injection with irqfd setup */
-    vfio_unmask_single_irqindex(&vdev->vbasedev, intp->pin);
-
     intp->kvm_accel = true;
 
     trace_vfio_platform_start_irqfd_injection(intp->pin,
@@ -406,9 +416,11 @@ static void vfio_start_irqfd_injection(SysBusDevice *sbdev, qemu_irq irq)
     return;
 fail_vfio:
     kvm_irqchip_remove_irqfd_notifier(kvm_state, &intp->interrupt, irq);
+    error_report("vfio: failed to start eventfd signaling for IRQ %d: %m",
+                 intp->pin);
+    abort();
 fail_irqfd:
-    vfio_start_eventfd_injection(intp);
-    vfio_unmask_single_irqindex(&vdev->vbasedev, intp->pin);
+    vfio_start_eventfd_injection(sbdev, irq);
     return;
 }
 
@@ -646,7 +658,6 @@ static void vfio_platform_realize(DeviceState *dev, Error **errp)
     VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(dev);
     SysBusDevice *sbdev = SYS_BUS_DEVICE(dev);
     VFIODevice *vbasedev = &vdev->vbasedev;
-    VFIOINTp *intp;
     int i, ret;
 
     vbasedev->type = VFIO_DEVICE_TYPE_PLATFORM;
@@ -665,10 +676,6 @@ static void vfio_platform_realize(DeviceState *dev, Error **errp)
         vfio_map_region(vdev, i);
         sysbus_init_mmio(sbdev, &vdev->regions[i]->mem);
     }
-
-    QLIST_FOREACH(intp, &vdev->intp_list, next) {
-        vfio_start_eventfd_injection(intp);
-    }
 }
 
 static const VMStateDescription vfio_platform_vmstate = {
-- 
1.8.3.2

[Qemu-devel] [PATCH 2/2] hw/vfio/platform: do not set resamplefd for edge-sensitive IRQS

[Eric Auger]

In irqfd mode, current code attempts to set a resamplefd whatever
the type of the IRQ. For an edge-sensitive IRQ this attempt fails
and as a consequence the whole irqfd setup fails and we fall back
to the slow mode. This patch bypasses the resamplefd setting for
non level-sentive IRQs.

Signed-off-by: Eric Auger <eric.auger@linaro.org>
---
 hw/vfio/platform.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index 38eaccf..2c91650 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -352,6 +352,10 @@ static int vfio_set_resample_eventfd(VFIOINTp *intp)
     int argsz, ret;
     int32_t *pfd;
 
+    if (!(intp->flags & VFIO_IRQ_INFO_AUTOMASKED)) {
+        return 0;
+    }
+
     argsz = sizeof(*irq_set) + sizeof(*pfd);
     irq_set = g_malloc0(argsz);
     irq_set->argsz = argsz;
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH 2/2] hw/vfio/platform: do not set resamplefd for edge-sensitive IRQS

[Alex Williamson]

On Wed, 2015-09-23 at 00:00 +0100, Eric Auger wrote:
> In irqfd mode, current code attempts to set a resamplefd whatever
> the type of the IRQ. For an edge-sensitive IRQ this attempt fails
> and as a consequence the whole irqfd setup fails and we fall back
> to the slow mode. This patch bypasses the resamplefd setting for
> non level-sentive IRQs.
> 
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> ---
>  hw/vfio/platform.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
> index 38eaccf..2c91650 100644
> --- a/hw/vfio/platform.c
> +++ b/hw/vfio/platform.c
> @@ -352,6 +352,10 @@ static int vfio_set_resample_eventfd(VFIOINTp *intp)
>      int argsz, ret;
>      int32_t *pfd;
>  
> +    if (!(intp->flags & VFIO_IRQ_INFO_AUTOMASKED)) {
> +        return 0;
> +    }
> +
>      argsz = sizeof(*irq_set) + sizeof(*pfd);
>      irq_set = g_malloc0(argsz);
>      irq_set->argsz = argsz;

Aren't we also wasting a file descriptor for intp->unmask on edge IRQs
as well?  I'd be tempted to make a helper function that does this test
and use it both to avoid calling this in vfio_start_irqfd_injection()
and also to avoid creating the unused eventfd in vfio_init_intp().
Thanks,

Alex

Re: [Qemu-devel] [PATCH 2/2] hw/vfio/platform: do not set resamplefd for edge-sensitive IRQS

[Eric Auger]

On 09/23/2015 09:28 PM, Alex Williamson wrote:
> On Wed, 2015-09-23 at 00:00 +0100, Eric Auger wrote:
>> In irqfd mode, current code attempts to set a resamplefd whatever
>> the type of the IRQ. For an edge-sensitive IRQ this attempt fails
>> and as a consequence the whole irqfd setup fails and we fall back
>> to the slow mode. This patch bypasses the resamplefd setting for
>> non level-sentive IRQs.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>> ---
>>  hw/vfio/platform.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
>> index 38eaccf..2c91650 100644
>> --- a/hw/vfio/platform.c
>> +++ b/hw/vfio/platform.c
>> @@ -352,6 +352,10 @@ static int vfio_set_resample_eventfd(VFIOINTp *intp)
>>      int argsz, ret;
>>      int32_t *pfd;
>>  
>> +    if (!(intp->flags & VFIO_IRQ_INFO_AUTOMASKED)) {
>> +        return 0;
>> +    }
>> +
>>      argsz = sizeof(*irq_set) + sizeof(*pfd);
>>      irq_set = g_malloc0(argsz);
>>      irq_set->argsz = argsz;
> 
> Aren't we also wasting a file descriptor for intp->unmask on edge IRQs
> as well?  I'd be tempted to make a helper function that does this test
> and use it both to avoid calling this in vfio_start_irqfd_injection()
> and also to avoid creating the unused eventfd in vfio_init_intp().
> Thanks,

Makes sense

Thanks

Eric
> 
> Alex
>