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

[Marc-André Lureau]

Hi

On Fri, Jan 29, 2016 at 5:23 PM, Markus Armbruster <address@hidden> wrote:
> 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."

I didn't noticed that (it has been like this from initial commit), I
think we should follow the spec.

>> -}
>> -
>>  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.

Ok

>
>> +    }
>>  }
>>
>>  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?

It's really not much, qemu_chr_open_eventfd() was introduced for
ivshmem to watch eventfd with qemu_chr_add_handlers(), but we can
simply use EventNotifier + qemu_set_fd_handler() alone.

-- 
Marc-André Lureau