On 9/10/20 6:29 PM, Alex Williamson wrote:
> On Thu, 10 Sep 2020 17:29:25 +0200
> Philippe Mathieu-Daudé <phi...@redhat.com> wrote:
>
>> Hi Stefan, Alex.
>>
>> On 9/10/20 12:44 PM, Stefan Hajnoczi wrote:
>>> On Wed, Sep 09, 2020 at 04:23:53PM +0200, Philippe Mathieu-Daudé wrote:
>>>> +/**
>>>> + * Initialize device MSIX IRQs and register event notifiers.
>>>> + * @irq_count: pointer to number of MSIX IRQs to initialize
>>>> + * @notifier: Array of @irq_count notifiers (each corresponding to a MSIX
>>>> IRQ)
>>>> +
>>>> + * If the number of IRQs requested exceeds the available on the device,
>>>> + * store the number of available IRQs in @irq_count and return -EOVERFLOW.
>>>> + */
>>>> +int qemu_vfio_pci_init_msix_irqs(QEMUVFIOState *s, EventNotifier
>>>> *notifier,
>>>> + unsigned *irq_count, Error **errp)
>>>> +{
>>>> + int r;
>>>> + size_t irq_set_size;
>>>> + struct vfio_irq_set *irq_set;
>>>> + struct vfio_irq_info irq_info = {
>>>> + .argsz = sizeof(irq_info),
>>>> + .index = VFIO_PCI_MSIX_IRQ_INDEX
>>>> + };
>>>> +
>>>> + if (ioctl(s->device, VFIO_DEVICE_GET_IRQ_INFO, &irq_info)) {
>>>> + error_setg_errno(errp, errno, "Failed to get device interrupt
>>>> info");
>>>> + return -errno;
>>>> + }
>>>> + if (irq_info.count < *irq_count) {
>>>> + error_setg(errp, "Not enough device interrupts available");
>>>> + *irq_count = irq_info.count;
>>>> + return -EOVERFLOW;
>>>> + }
>>>> + if (!(irq_info.flags & VFIO_IRQ_INFO_EVENTFD)) {
>>>> + error_setg(errp, "Device interrupt doesn't support eventfd");
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + irq_set_size = sizeof(*irq_set) + *irq_count * sizeof(int32_t);
>>>> + irq_set = g_malloc0(irq_set_size);
>>>> +
>>>> + /* Get to a known IRQ state */
>>>> + *irq_set = (struct vfio_irq_set) {
>>>> + .argsz = irq_set_size,
>>>> + .flags = VFIO_IRQ_SET_DATA_EVENTFD | VFIO_IRQ_SET_ACTION_TRIGGER,
>>>> + .index = irq_info.index,
>>>> + .start = 0,
>>>> + .count = *irq_count,
>>>> + };
>>>> +
>>>> + for (unsigned i = 0; i < *irq_count; i++) {
>>>> + ((int32_t *)&irq_set->data)[i] =
>>>> event_notifier_get_fd(¬ifier[i]);
>>>> + }
>>>> + r = ioctl(s->device, VFIO_DEVICE_SET_IRQS, irq_set);
>>>> + g_free(irq_set);
>>>> + if (r <= 0) {
>>>> + error_setg_errno(errp, errno, "Failed to setup device
>>>> interrupts");
>>>> + return -errno;
>>>> + } else if (r < *irq_count) {
>>>> + error_setg(errp, "Not enough device interrupts available");
>>>> + *irq_count = r;
>>>> + return -EOVERFLOW;
>>>> + }
>>>
>>> EOVERFLOW can occur in two cases: VFIO_DEVICE_GET_IRQ_INFO and
>>> VFIO_DEVICE_SET_IRQS.
>>
>> Yes.
>>
>>>
>>> If it happens in the second case the notifier[] array has been
>>> registered successfully.
>>
>> No, I don't think so:
>>
>> vfio_pci_set_msi_trigger() register the notifier only if
>> vfio_msi_enable() succeeded (returned 0). If vfio_msi_enable()
>> failed it returns the number of vectors available but do
>> not register the notifiers.
>>
>> Alex, do you confirm?
>
> Yes, if we can't setup what the user requested we don't setup anything.
> However, I think we return zero on success, which seems to fall into
> your error condition. Has this been tested? Thanks,
Not v6 as I didn't have the testing setup handy, and thought
v5 -> v6 change was trivial enough :S
Good news: my next task is to add a test :)
>
> Alex
>
>>> The caller has no way of distinguishing the two cases. Therefore the
>>> caller doesn't know if the eventfds will be used by the kernel after
>>> EOVERFLOW.
>>>
>>> If the second case can ever happen then this function should probably
>>> call VFIO_DEVICE_SET_IRQS again with VFIO_IRQ_SET_DATA_NONE to
>>> unregister the eventfds before returning EOVERFLOW.
>>>
>>> STefan
>>>
>>
>
