Re: [Qemu-devel] [PULL 3/3] vfio/common: Introduce vfio_set_irq_signaling helper

[Alex Williamson <alex.williamson@redhat.com>]

On Tue, 2 Jul 2019 14:32:26 +0200
Auger Eric <eric.auger@redhat.com> wrote:

> Hi Peter,
> 
> On 7/2/19 12:37 PM, Peter Maydell wrote:
> > On Thu, 13 Jun 2019 at 22:51, Alex Williamson
> > <alex.williamson@redhat.com> wrote:  
> >>
> >> From: Eric Auger <eric.auger@redhat.com>
> >>
> >> The code used to assign an interrupt index/subindex to an
> >> eventfd is duplicated many times. Let's introduce an helper that
> >> allows to set/unset the signaling for an ACTION_TRIGGER,
> >> ACTION_MASK or ACTION_UNMASK action.
> >>
> >> In the error message, we now use errno in case of any
> >> VFIO_DEVICE_SET_IRQS ioctl failure.
> >>
> >> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> >> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> >> Reviewed-by: Li Qiang <liq3ea@gmail.com>
> >> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>  
> > 
> > Hi; coverity reports (CID 1402196) a possible unchecked return value
> > in this code:
> > 
> >   
> >> @@ -592,26 +550,10 @@ static void vfio_msix_vector_release(PCIDevice *pdev, unsigned int nr)
> >>       * be re-asserted on unmask.  Nothing to do if already using QEMU mode.
> >>       */
> >>      if (vector->virq >= 0) {
> >> -        int argsz;
> >> -        struct vfio_irq_set *irq_set;
> >> -        int32_t *pfd;
> >> -
> >> -        argsz = sizeof(*irq_set) + sizeof(*pfd);
> >> -
> >> -        irq_set = g_malloc0(argsz);
> >> -        irq_set->argsz = argsz;
> >> -        irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
> >> -                         VFIO_IRQ_SET_ACTION_TRIGGER;
> >> -        irq_set->index = VFIO_PCI_MSIX_IRQ_INDEX;
> >> -        irq_set->start = nr;
> >> -        irq_set->count = 1;
> >> -        pfd = (int32_t *)&irq_set->data;
> >> +        int32_t fd = event_notifier_get_fd(&vector->interrupt);
> >>
> >> -        *pfd = event_notifier_get_fd(&vector->interrupt);
> >> -
> >> -        ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
> >> -
> >> -        g_free(irq_set);
> >> +        vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX, nr,
> >> +                               VFIO_IRQ_SET_ACTION_TRIGGER, fd, NULL);  
> > 
> > In vfp_msix_vector_release() we call vfio_set_irq_signaling(),
> > but we don't check the returned error value, whereas in the other
> > 7 places we call the function we do check. Is there some missing
> > error handling here ?  
> 
> the difference with the other calls is that we pass a NULL errp here so
> we don't need to consume a potential error. Before the introduction of
> vfio_set_irq_signaling we had an ioctl call whose returned value was not
> tested either. So I think it properly translates what was done before.
> It seems we are willingly not producing any error message in that case.
> Alex, can you confirm?

When we're emulating writes to the MSI-X vector table we have no
failure path up to the guest.  Real hardware cannot fail to enable a
vector that's available in hardware, thus we can either log the issue,
ignore the issue, or fault.  I guess Coverity is simply noting that
other cases are tested while this is not, therefore we should either
explicitly ignore the return value with a cast to void or take this as
an opportunity to log the fault, which might be useful in debugging a
device that isn't working properly.  Thanks,

Alex