[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PULL 3/3] vfio/common: Introduce vfio_set_irq_signalin
From: |
Alex Williamson |
Subject: |
Re: [Qemu-devel] [PULL 3/3] vfio/common: Introduce vfio_set_irq_signaling helper |
Date: |
Tue, 2 Jul 2019 09:55:46 -0600 |
On Tue, 2 Jul 2019 14:32:26 +0200
Auger Eric <address@hidden> wrote:
> Hi Peter,
>
> On 7/2/19 12:37 PM, Peter Maydell wrote:
> > On Thu, 13 Jun 2019 at 22:51, Alex Williamson
> > <address@hidden> wrote:
> >>
> >> From: Eric Auger <address@hidden>
> >>
> >> 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 <address@hidden>
> >> Reviewed-by: Cornelia Huck <address@hidden>
> >> Reviewed-by: Li Qiang <address@hidden>
> >> Signed-off-by: Alex Williamson <address@hidden>
> >
> > 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