On Tue, 2 Jul 2019 14:32:26 +0200 Auger Eric <[email protected]> wrote:
> Hi Peter, > > On 7/2/19 12:37 PM, Peter Maydell wrote: > > On Thu, 13 Jun 2019 at 22:51, Alex Williamson > > <[email protected]> wrote: > >> > >> From: Eric Auger <[email protected]> > >> > >> 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 <[email protected]> > >> Reviewed-by: Cornelia Huck <[email protected]> > >> Reviewed-by: Li Qiang <[email protected]> > >> Signed-off-by: Alex Williamson <[email protected]> > > > > 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
