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

[Markus Armbruster]

Markus Armbruster <address@hidden> writes:

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

I'm afraid I didn't read the code correctly.  Yes, the peer writes 1
unless someone got creative.  And yes, if we're using the pipe
emulation, we actually read what the peer wrote.  But if we're using
eventfd(2), we read the sum of what the peer wrote since our last read.
If that sum is zero modulo 256, the interrupt is lost.  Fortunately, we
should be using KVM ioeventfd in practice, which should bypass the
flawed code.

So this is actually a bug fix, but it's much too late to update the
commit message.

I told you I want review from someone who actually understands this
stuff :)