Hi
On Fri, Jan 29, 2016 at 5:23 PM, Markus Armbruster <arm...@redhat.com> wrote:
> marcandre.lur...@redhat.com writes:
>
>> From: Marc-André Lureau <marcandre.lur...@redhat.com>
>>
>> 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 <marcandre.lur...@redhat.com>
>> ---
>> 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
