[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: |
Auger Eric |
Subject: |
Re: [Qemu-devel] [PULL 3/3] vfio/common: Introduce vfio_set_irq_signaling helper |
Date: |
Tue, 2 Jul 2019 14:32:26 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 |
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?
Thanks
Eric
>
> thanks
> -- PMM
>