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.au...@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.chen...@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.ro...@huawei.com> >>>>>> Signed-off-by: Chen Qun <kuhn.chen...@huawei.com> >>>>>> --- >>>>>> Cc: Alex Williamson <alex.william...@redhat.com> >>>>>> Cc: Eric Auger <eric.au...@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 >
