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

[Markus Armbruster]

Marc-André Lureau <address@hidden> writes:

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

For me, working code trumps spec unless the code is clearly flawed.
Other software doesn't interface with the spec, it interfaces with the
code.

However, I guess we can follow the spec in this case.  Two reasons:

* We can't permit arbitrary values, because value 0 could break things
  (I think).

* If I read the code correctly, the value we read here should come from
  a peer's ivshmem device model.  The device model writes it with
  event_notifier_set(), which writes 1.  To get any other value, you
  need to get creative.  So the code agrees with the spec, unless you
  get creative.

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

Yes, but I'm not familiar enough with this stuff to do a real review
with reasonable effot.

The hamfisted way to "encourage" real review is to withhold my R-by for
this patch.  Isn't 100% right, because I *did* look over it (and liked
what I saw), but it's less wrong than merging this without real review.