| Message ID | 549e6300c0ea011cdce9a2712d49de4efd3a06b7.1678911529.git.reinette.chatre@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio/pci: Support dynamic allocation of MSI-X interrupts | expand |
On Wed, 15 Mar 2023 13:59:28 -0700 Reinette Chatre <reinette.chatre@intel.com> wrote: > Dynamic MSI-X is supported. Clear VFIO_IRQ_INFO_NORESIZE > to provide guidance to user space. > > Signed-off-by: Reinette Chatre <reinette.chatre@intel.com> > --- > drivers/vfio/pci/vfio_pci_core.c | 2 +- > include/uapi/linux/vfio.h | 3 +++ > 2 files changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c > index ae0e161c7fc9..1d071ee212a7 100644 > --- a/drivers/vfio/pci/vfio_pci_core.c > +++ b/drivers/vfio/pci/vfio_pci_core.c > @@ -1111,7 +1111,7 @@ static int vfio_pci_ioctl_get_irq_info(struct vfio_pci_core_device *vdev, > if (info.index == VFIO_PCI_INTX_IRQ_INDEX) > info.flags |= > (VFIO_IRQ_INFO_MASKABLE | VFIO_IRQ_INFO_AUTOMASKED); > - else > + else if (info.index != VFIO_PCI_MSIX_IRQ_INDEX) > info.flags |= VFIO_IRQ_INFO_NORESIZE; > I think we need to check pci_msix_can_alloc_dyn(), right? Thanks, Alex > return copy_to_user(arg, &info, minsz) ? -EFAULT : 0; > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h > index 0552e8dcf0cb..1a36134cae5c 100644 > --- a/include/uapi/linux/vfio.h > +++ b/include/uapi/linux/vfio.h > @@ -511,6 +511,9 @@ struct vfio_region_info_cap_nvlink2_lnkspd { > * then add and unmask vectors, it's up to userspace to make the decision > * whether to allocate the maximum supported number of vectors or tear > * down setup and incrementally increase the vectors as each is enabled. > + * Absence of the NORESIZE flag indicates that vectors can be enabled > + * and disabled dynamically without impacting other vectors within the > + * index. > */ > struct vfio_irq_info { > __u32 argsz;
Hi Alex, On 3/17/2023 2:05 PM, Alex Williamson wrote: > On Wed, 15 Mar 2023 13:59:28 -0700 > Reinette Chatre <reinette.chatre@intel.com> wrote: > >> Dynamic MSI-X is supported. Clear VFIO_IRQ_INFO_NORESIZE >> to provide guidance to user space. >> >> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com> >> --- >> drivers/vfio/pci/vfio_pci_core.c | 2 +- >> include/uapi/linux/vfio.h | 3 +++ >> 2 files changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c >> index ae0e161c7fc9..1d071ee212a7 100644 >> --- a/drivers/vfio/pci/vfio_pci_core.c >> +++ b/drivers/vfio/pci/vfio_pci_core.c >> @@ -1111,7 +1111,7 @@ static int vfio_pci_ioctl_get_irq_info(struct vfio_pci_core_device *vdev, >> if (info.index == VFIO_PCI_INTX_IRQ_INDEX) >> info.flags |= >> (VFIO_IRQ_INFO_MASKABLE | VFIO_IRQ_INFO_AUTOMASKED); >> - else >> + else if (info.index != VFIO_PCI_MSIX_IRQ_INDEX) >> info.flags |= VFIO_IRQ_INFO_NORESIZE; >> > > I think we need to check pci_msix_can_alloc_dyn(), right? Thanks, Can pci_msix_can_alloc_dyn() ever return false? I cannot see how pci_msix_can_alloc_dyn() can return false when considering the VFIO PCI MSI-X flow: vfio_msi_enable(..., ..., msix == true) pci_alloc_irq_vectors(..., ..., ..., flag == PCI_IRQ_MSIX) pci_alloc_irq_vectors_affinity() __pci_enable_msix_range() pci_setup_msix_device_domain() pci_create_device_domain(..., &pci_msix_template, ...) The template used above, pci_msix_template, has MSI_FLAG_PCI_MSIX_ALLOC_DYN hardcoded. This is the flag that pci_msix_can_alloc_dyn() tests for. If indeed pci_msix_can_alloc_dyn() can return false in the VFIO PCI MSI-X usage then this series needs to be reworked to continue supporting existing allocation mechanism as well as dynamic MSI-X allocation. Which allocation mechanism to use would be depend on pci_msix_can_alloc_dyn(). Alternatively, if you agree that VFIO PCI MSI-X can expect pci_msix_can_alloc_dyn() to always return true then I should perhaps add a test in vfio_msi_enable() that confirms this is the case. This would cause vfio_msi_enable() to fail if dynamic MSI-X is not possible, perhaps even complain loudly with a WARN. Reinette
On Fri, 17 Mar 2023 15:21:09 -0700 Reinette Chatre <reinette.chatre@intel.com> wrote: > Hi Alex, > > On 3/17/2023 2:05 PM, Alex Williamson wrote: > > On Wed, 15 Mar 2023 13:59:28 -0700 > > Reinette Chatre <reinette.chatre@intel.com> wrote: > > > >> Dynamic MSI-X is supported. Clear VFIO_IRQ_INFO_NORESIZE > >> to provide guidance to user space. > >> > >> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com> > >> --- > >> drivers/vfio/pci/vfio_pci_core.c | 2 +- > >> include/uapi/linux/vfio.h | 3 +++ > >> 2 files changed, 4 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c > >> index ae0e161c7fc9..1d071ee212a7 100644 > >> --- a/drivers/vfio/pci/vfio_pci_core.c > >> +++ b/drivers/vfio/pci/vfio_pci_core.c > >> @@ -1111,7 +1111,7 @@ static int vfio_pci_ioctl_get_irq_info(struct vfio_pci_core_device *vdev, > >> if (info.index == VFIO_PCI_INTX_IRQ_INDEX) > >> info.flags |= > >> (VFIO_IRQ_INFO_MASKABLE | VFIO_IRQ_INFO_AUTOMASKED); > >> - else > >> + else if (info.index != VFIO_PCI_MSIX_IRQ_INDEX) > >> info.flags |= VFIO_IRQ_INFO_NORESIZE; > >> > > > > I think we need to check pci_msix_can_alloc_dyn(), right? Thanks, > > Can pci_msix_can_alloc_dyn() ever return false? > > I cannot see how pci_msix_can_alloc_dyn() can return false when > considering the VFIO PCI MSI-X flow: > > vfio_msi_enable(..., ..., msix == true) > pci_alloc_irq_vectors(..., ..., ..., flag == PCI_IRQ_MSIX) > pci_alloc_irq_vectors_affinity() > __pci_enable_msix_range() > pci_setup_msix_device_domain() > pci_create_device_domain(..., &pci_msix_template, ...) > > The template used above, pci_msix_template, has MSI_FLAG_PCI_MSIX_ALLOC_DYN > hardcoded. This is the flag that pci_msix_can_alloc_dyn() tests for. > > If indeed pci_msix_can_alloc_dyn() can return false in the VFIO PCI MSI-X > usage then this series needs to be reworked to continue supporting > existing allocation mechanism as well as dynamic MSI-X allocation. Which > allocation mechanism to use would be depend on pci_msix_can_alloc_dyn(). > > Alternatively, if you agree that VFIO PCI MSI-X can expect > pci_msix_can_alloc_dyn() to always return true then I should perhaps > add a test in vfio_msi_enable() that confirms this is the case. This would > cause vfio_msi_enable() to fail if dynamic MSI-X is not possible, perhaps even > complain loudly with a WARN. pci_setup_msix_device_domain() says it returns true if: * True when: * - The device does not have a MSI parent irq domain associated, * which keeps the legacy architecture specific and the global * PCI/MSI domain models working * - The MSI-X domain exists already * - The MSI-X domain was successfully allocated That first one seems concerning, dynamic allocation only works on irq domain configurations. What does that exclude and do we care about any of them for vfio-pci? Minimally this suggests a dependency on IRQ_DOMAIN, which we don't currently have, but I'm not sure if supporting irq domains is the same as having irq domains. Thanks, Alex
Hi Alex, On 3/17/2023 4:01 PM, Alex Williamson wrote: > On Fri, 17 Mar 2023 15:21:09 -0700 > Reinette Chatre <reinette.chatre@intel.com> wrote: > >> Hi Alex, >> >> On 3/17/2023 2:05 PM, Alex Williamson wrote: >>> On Wed, 15 Mar 2023 13:59:28 -0700 >>> Reinette Chatre <reinette.chatre@intel.com> wrote: >>> >>>> Dynamic MSI-X is supported. Clear VFIO_IRQ_INFO_NORESIZE >>>> to provide guidance to user space. >>>> >>>> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com> >>>> --- >>>> drivers/vfio/pci/vfio_pci_core.c | 2 +- >>>> include/uapi/linux/vfio.h | 3 +++ >>>> 2 files changed, 4 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c >>>> index ae0e161c7fc9..1d071ee212a7 100644 >>>> --- a/drivers/vfio/pci/vfio_pci_core.c >>>> +++ b/drivers/vfio/pci/vfio_pci_core.c >>>> @@ -1111,7 +1111,7 @@ static int vfio_pci_ioctl_get_irq_info(struct vfio_pci_core_device *vdev, >>>> if (info.index == VFIO_PCI_INTX_IRQ_INDEX) >>>> info.flags |= >>>> (VFIO_IRQ_INFO_MASKABLE | VFIO_IRQ_INFO_AUTOMASKED); >>>> - else >>>> + else if (info.index != VFIO_PCI_MSIX_IRQ_INDEX) >>>> info.flags |= VFIO_IRQ_INFO_NORESIZE; >>>> >>> >>> I think we need to check pci_msix_can_alloc_dyn(), right? Thanks, >> >> Can pci_msix_can_alloc_dyn() ever return false? >> >> I cannot see how pci_msix_can_alloc_dyn() can return false when >> considering the VFIO PCI MSI-X flow: >> >> vfio_msi_enable(..., ..., msix == true) >> pci_alloc_irq_vectors(..., ..., ..., flag == PCI_IRQ_MSIX) >> pci_alloc_irq_vectors_affinity() >> __pci_enable_msix_range() >> pci_setup_msix_device_domain() >> pci_create_device_domain(..., &pci_msix_template, ...) >> >> The template used above, pci_msix_template, has MSI_FLAG_PCI_MSIX_ALLOC_DYN >> hardcoded. This is the flag that pci_msix_can_alloc_dyn() tests for. >> >> If indeed pci_msix_can_alloc_dyn() can return false in the VFIO PCI MSI-X >> usage then this series needs to be reworked to continue supporting >> existing allocation mechanism as well as dynamic MSI-X allocation. Which >> allocation mechanism to use would be depend on pci_msix_can_alloc_dyn(). >> >> Alternatively, if you agree that VFIO PCI MSI-X can expect >> pci_msix_can_alloc_dyn() to always return true then I should perhaps >> add a test in vfio_msi_enable() that confirms this is the case. This would >> cause vfio_msi_enable() to fail if dynamic MSI-X is not possible, perhaps even >> complain loudly with a WARN. > > pci_setup_msix_device_domain() says it returns true if: > > * True when: > * - The device does not have a MSI parent irq domain associated, > * which keeps the legacy architecture specific and the global > * PCI/MSI domain models working > * - The MSI-X domain exists already > * - The MSI-X domain was successfully allocated > > That first one seems concerning, dynamic allocation only works on irq > domain configurations. What does that exclude and do we care about any > of them for vfio-pci? Minimally this suggests a dependency on > IRQ_DOMAIN, which we don't currently have, but I'm not sure if > supporting irq domains is the same as having irq domains. Thanks, Just to confirm, as I mentioned in [1] I do plan to rework this solution to support both allocation mechanisms, using pci_msix_can_alloc_dyn() to pick which one to use. Thank you very much for pointing out this gap to me. Reinette [1] https://lore.kernel.org/lkml/e2d3f5a6-0a36-f19d-8f19-748197c3308d@intel.com/
On Fri, Mar 17, 2023 at 05:01:49PM -0600, Alex Williamson wrote: > pci_setup_msix_device_domain() says it returns true if: > > * True when: > * - The device does not have a MSI parent irq domain associated, > * which keeps the legacy architecture specific and the global > * PCI/MSI domain models working > * - The MSI-X domain exists already > * - The MSI-X domain was successfully allocated > > That first one seems concerning, dynamic allocation only works on irq > domain configurations. What does that exclude and do we care about any > of them for vfio-pci? Several archs and other weird things override the MSI-X programming. Eg by turning it into a hypervisor call or something. These were not converted to dynamic mode. So at the VFIO level you can get end up with MSI support but done through legacy paths that don't support dynamic allocation. Eg on POWER, xen, etc. Jason
diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c index ae0e161c7fc9..1d071ee212a7 100644 --- a/drivers/vfio/pci/vfio_pci_core.c +++ b/drivers/vfio/pci/vfio_pci_core.c @@ -1111,7 +1111,7 @@ static int vfio_pci_ioctl_get_irq_info(struct vfio_pci_core_device *vdev, if (info.index == VFIO_PCI_INTX_IRQ_INDEX) info.flags |= (VFIO_IRQ_INFO_MASKABLE | VFIO_IRQ_INFO_AUTOMASKED); - else + else if (info.index != VFIO_PCI_MSIX_IRQ_INDEX) info.flags |= VFIO_IRQ_INFO_NORESIZE; return copy_to_user(arg, &info, minsz) ? -EFAULT : 0; diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h index 0552e8dcf0cb..1a36134cae5c 100644 --- a/include/uapi/linux/vfio.h +++ b/include/uapi/linux/vfio.h @@ -511,6 +511,9 @@ struct vfio_region_info_cap_nvlink2_lnkspd { * then add and unmask vectors, it's up to userspace to make the decision * whether to allocate the maximum supported number of vectors or tear * down setup and incrementally increase the vectors as each is enabled. + * Absence of the NORESIZE flag indicates that vectors can be enabled + * and disabled dynamically without impacting other vectors within the + * index. */ struct vfio_irq_info { __u32 argsz;
Dynamic MSI-X is supported. Clear VFIO_IRQ_INFO_NORESIZE to provide guidance to user space. Signed-off-by: Reinette Chatre <reinette.chatre@intel.com> --- drivers/vfio/pci/vfio_pci_core.c | 2 +- include/uapi/linux/vfio.h | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-)