Re: [PATCH V3 09/10] vfio/pci: Support dynamic MSI-X

[Reinette Chatre]

Hi Alex,

On 4/18/2023 3:38 PM, Alex Williamson wrote:
> On Tue, 18 Apr 2023 10:29:20 -0700
> Reinette Chatre <reinette.chatre@xxxxxxxxx> wrote:
> 
>> Recently introduced pci_msix_alloc_irq_at() and pci_msix_free_irq()
>> enables an individual MSI-X interrupt to be allocated and freed after
>> MSI-X enabling.
>>
>> Use dynamic MSI-X (if supported by the device) to allocate an interrupt
>> after MSI-X is enabled. An MSI-X interrupt is dynamically allocated at
>> the time a valid eventfd is assigned. This is different behavior from
>> a range provided during MSI-X enabling where interrupts are allocated
>> for the entire range whether a valid eventfd is provided for each
>> interrupt or not.
>>
>> Do not dynamically free interrupts, leave that to when MSI-X is
>> disabled.
> 
> But we do, sometimes, even if essentially only on the error path.  Is
> that worthwhile?  It seems like we could entirely remove
> vfio_msi_free_irq() and rely only on pci_free_irq_vectors() on MSI/X
> teardown.

Yes, it is only on the error path where dynamic MSI-X interrupts are
removed. I do not know how to determine if it is worthwhile. On the
kernel side failure seems unlikely since it would mean memory cannot
be allocated or request_irq() failed. In these cases it may not be
worthwhile since user space may try again and having the interrupt
already allocated would be helpful. The remaining error seems to be
if user space provided an invalid eventfd. An allocation in response
to wrong user input is a concern to me. Should we consider
buggy/malicious users? I am uncertain here so would defer to your
guidance.

> I'd probably also add a comment in the commit log about the theory
> behind not dynamically freeing irqs, ie. latency, reliability, and
> whatever else we used to justify it.  Thanks,

Sure. How about something like below to replace the final sentence of
the changelog:

"When a guest disables an interrupt, user space (Qemu) does not
disable the interrupt but instead assigns it a different trigger. A
common flow is thus for the VFIO_DEVICE_SET_IRQS ioctl() to be 
used to assign a new eventfd to an already enabled interrupt. Freeing
and re-allocating an interrupt in this scenario would add unnecessary
latency as well as uncertainty since the re-allocation may fail. Do
not dynamically free interrupts when an interrupt is disabled, instead
support a subsequent re-enable to draw from the initial allocation when
possible. Leave freeing of interrupts to when MSI-X is disabled."

Reinette