Re: [Qemu-devel] [PATCH 7/8] ivshmem: use a single eventfd callback, get rid of CharDriver

[Markus Armbruster]

address@hidden writes:

> From: Marc-André Lureau <address@hidden>
>
> Simplify the interrupt handling by having a single callback on irq&msi
> cases. Remove usage of CharDriver, replace it with
> qemu_set_fd_handler(). Use event_notifier_test_and_clear() to read the
> eventfd.
>
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  hw/misc/ivshmem.c | 55 
> ++++++++++++++++++-------------------------------------
>  1 file changed, 18 insertions(+), 37 deletions(-)
>
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index 11780b1..9eb8a81 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -263,15 +263,6 @@ static const MemoryRegionOps ivshmem_mmio_ops = {
>      },
>  };
>  
> -static void ivshmem_receive(void *opaque, const uint8_t *buf, int size)
> -{
> -    IVShmemState *s = opaque;
> -
> -    IVSHMEM_DPRINTF("ivshmem_receive 0x%02x size: %d\n", *buf, size);
> -
> -    ivshmem_IntrStatus_write(s, *buf);

Before your patch, we write the first byte received to s->intrstatus.
This is odd; ivshmem_device_spec.txt says "The status register is set to
1 when an interrupt occurs."

> -}
> -
>  static int ivshmem_can_receive(void * opaque)
>  {
>      return sizeof(int64_t);
> @@ -282,15 +273,24 @@ static void ivshmem_event(void *opaque, int event)
>      IVSHMEM_DPRINTF("ivshmem_event %d\n", event);
>  }
>  
> -static void fake_irqfd(void *opaque, const uint8_t *buf, int size) {
> -
> +static void ivshmem_vector_notify(void *opaque)
> +{
>      MSIVector *entry = opaque;
>      PCIDevice *pdev = entry->pdev;
>      IVShmemState *s = IVSHMEM(pdev);
>      int vector = entry - s->msi_vectors;
> +    EventNotifier *n = &s->peers[s->vm_id].eventfds[vector];
> +
> +    if (!event_notifier_test_and_clear(n)) {
> +        return;
> +    }
>  
>      IVSHMEM_DPRINTF("interrupt on vector %p %d\n", pdev, vector);
> -    msix_notify(pdev, vector);
> +    if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
> +        msix_notify(pdev, vector);
> +    } else {
> +        ivshmem_IntrStatus_write(s, 1);

After the patch, we write 1 to s->intrstatus.  May well be an
improvement, or even a bug fix, but it needs to be explained in the
commit message.

> +    }
>  }
>  
>  static int ivshmem_vector_unmask(PCIDevice *dev, unsigned vector,
> @@ -350,35 +350,16 @@ static void ivshmem_vector_poll(PCIDevice *dev,
>      }
>  }
>  
> -static CharDriverState* create_eventfd_chr_device(IVShmemState *s,
> -                                                  EventNotifier *n,
> -                                                  int vector)
> +static void watch_vector_notifier(IVShmemState *s, EventNotifier *n,
> +                                 int vector)
>  {
> -    /* create a event character device based on the passed eventfd */
>      int eventfd = event_notifier_get_fd(n);
> -    CharDriverState *chr;
> -
> -    chr = qemu_chr_open_eventfd(eventfd);
> -
> -    if (chr == NULL) {
> -        error_report("creating chardriver for eventfd %d failed", eventfd);
> -        return NULL;
> -    }
> -    qemu_chr_fe_claim_no_fail(chr);
>  
>      /* if MSI is supported we need multiple interrupts */
> -    if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
> -        s->msi_vectors[vector].pdev = PCI_DEVICE(s);
> -
> -        qemu_chr_add_handlers(chr, ivshmem_can_receive, fake_irqfd,
> -                      ivshmem_event, &s->msi_vectors[vector]);
> -    } else {
> -        qemu_chr_add_handlers(chr, ivshmem_can_receive, ivshmem_receive,
> -                      ivshmem_event, s);
> -    }
> -
> -    return chr;
> +    s->msi_vectors[vector].pdev = PCI_DEVICE(s);
>  
> +    qemu_set_fd_handler(eventfd, ivshmem_vector_notify,
> +                        NULL, &s->msi_vectors[vector]);
>  }
>  
>  static int check_shm_size(IVShmemState *s, int fd, Error **errp)
> @@ -587,7 +568,7 @@ static void setup_interrupt(IVShmemState *s, int vector)
>  
>      if (!with_irqfd) {
>          IVSHMEM_DPRINTF("with eventfd");
> -        s->eventfd_chr[vector] = create_eventfd_chr_device(s, n, vector);
> +        watch_vector_notifier(s, n, vector);
>      } else if (msix_enabled(pdev)) {
>          IVSHMEM_DPRINTF("with irqfd");
>          if (ivshmem_add_kvm_msi_virq(s, vector) < 0) {

I like the looks of it, not least because it enables removal of
qemu_chr_open_eventfd() in the next patch.  But I recommend to get an
R-by from someone who actually understands this chardev stuff.  Paolo,
perhaps?