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
