Re: [PATCH 07/11] vfio/platform: Remove dead assignment in vfio_intp_interrupt()

[Auger Eric]

Hi Stefan,

On 8/18/20 2:54 PM, Stefan Hajnoczi wrote:
> On Thu, Aug 13, 2020 at 09:18:59PM +0200, Auger Eric wrote:
>> Hi Alex,
>>
>> On 8/13/20 9:15 PM, Alex Williamson wrote:
>>> On Thu, 13 Aug 2020 20:02:45 +0200
>>> Auger Eric <eric.auger@redhat.com> wrote:
>>>
>>>> Hi Alex,
>>>>
>>>> On 8/13/20 6:59 PM, Alex Williamson wrote:
>>>>> On Thu, 13 Aug 2020 15:37:08 +0800
>>>>> Chen Qun <kuhn.chenqun@huawei.com> wrote:
>>>>>   
>>>>>> Clang static code analyzer show warning:
>>>>>> hw/vfio/platform.c:239:9: warning: Value stored to 'ret' is never read
>>>>>>         ret = event_notifier_test_and_clear(intp->interrupt);
>>>>>>         ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>>
>>>>>> Reported-by: Euler Robot <euler.robot@huawei.com>
>>>>>> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
>>>>>> ---
>>>>>> Cc: Alex Williamson <alex.williamson@redhat.com>
>>>>>> Cc: Eric Auger <eric.auger@redhat.com>
>>>>>> ---
>>>>>>  hw/vfio/platform.c | 2 +-
>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
>>>>>> index ac2cefc9b1..869ed2c39d 100644
>>>>>> --- a/hw/vfio/platform.c
>>>>>> +++ b/hw/vfio/platform.c
>>>>>> @@ -236,7 +236,7 @@ static void vfio_intp_interrupt(VFIOINTp *intp)
>>>>>>          trace_vfio_intp_interrupt_set_pending(intp->pin);
>>>>>>          QSIMPLEQ_INSERT_TAIL(&vdev->pending_intp_queue,
>>>>>>                               intp, pqnext);
>>>>>> -        ret = event_notifier_test_and_clear(intp->interrupt);
>>>>>> +        event_notifier_test_and_clear(intp->interrupt);
>>>>>>          return;
>>>>>>      }  
>>>>>
>>>>> Testing that an event is pending in our notifier is generally a
>>>>> prerequisite to doing anything in the interrupt handler, I don't
>>>>> understand why we're just consuming it and ignoring the return value.
>>>>> The above is in the delayed handling branch of the function, but the
>>>>> normal non-delayed path would only go on to error_report() if the
>>>>> notifier is not pending and then inject an interrupt anyway.  This all
>>>>> seems rather suspicious and it's a unique pattern among the vfio
>>>>> callers of this function.  Is there a more fundamental bug that this
>>>>> function should perform this test once and return without doing
>>>>> anything if it's called spuriously, ie. without a notifier pending?
>>>>> Thanks,  
>>>>
>>>> Hum that's correct that other VFIO call sites do the check. My
>>>> understanding was that this could not fail in this case as, if we
>>>> entered the handler there was something to be cleared. In which
>>>> situation can this fail?
>>>
>>> I'm not sure what the right answer is, I see examples either way
>>> looking outside of vfio code.  On one hand, maybe we never get called
>>> spuriously, on the other if it's the callee's responsibility to drain
>>> events from the fd and we have it readily accessible whether there were
>>> any events pending, why would we inject an interrupt if the result that
>>> we have in hand shows no pending events?  The overhead of returning
>>> based on that result is minuscule.
>>
>> I agree
>>>
>>> qemu_set_fd_handler() is a wrapper for aio_set_fd_handler().  Stefan is
>>> a possible defacto maintainer of some of the aio code.  Stefan, do you
>>> have thoughts on whether callbacks from event notifier fds should
>>> consider spurious events?  Thanks,
>>
>> Indeed I saw that for instance block/nvme.c nvme_handle_event is not
>> checking the result.
>>
>> Let's wait for Stefan's answer ...
> 
> vfio_intp_interrupt() will always read a non-zero eventfd value, based
> on these assumptions:
> 
> intp->interrupt is "readable" since vfio_intp_interrupt() is called by
> the AioContext (event loop). "readable" does not guarantee that data can
> actually be read because it also includes error events:
> 
>   new_node->pfd.events = (io_read ? G_IO_IN | G_IO_HUP | G_IO_ERR : 0);
> 
> However, I think we can exclude the error case for the VFIO interrupt
> eventfds because there are no error cases for eventfds (unlike socket
> disconnection, for example).
> 
> The other important assumption is that only one thread on the host is
> monitoring the eventfd for activity.

Thank for your the confirmation. So this patch should be safe.

Best Regards

Eric
> 
> Stefan
>