Date:   Fri, 17 Mar 2023 17:01:49 -0600
From:   Alex Williamson <alex.williamson@redhat.com>
To:     Reinette Chatre <reinette.chatre@intel.com>
Cc:     <jgg@nvidia.com>, <yishaih@nvidia.com>,
        <shameerali.kolothum.thodi@huawei.com>, <kevin.tian@intel.com>,
        <tglx@linutronix.de>, <darwi@linutronix.de>, <kvm@vger.kernel.org>,
        <dave.jiang@intel.com>, <jing2.liu@intel.com>,
        <ashok.raj@intel.com>, <fenghua.yu@intel.com>,
        <tom.zanussi@linux.intel.com>, <linux-kernel@vger.kernel.org>
Subject: Re: [RFC PATCH 8/8] vfio/pci: Clear VFIO_IRQ_INFO_NORESIZE for
 MSI-X
Message-ID: <20230317170149.2be79d5a.alex.williamson@redhat.com>
In-Reply-To: <61296e93-6268-05cd-e876-680e07645a16@intel.com>
References: <cover.1678911529.git.reinette.chatre@intel.com>
        <549e6300c0ea011cdce9a2712d49de4efd3a06b7.1678911529.git.reinette.chatre@intel.com>
        <20230317150554.6bf92337.alex.williamson@redhat.com>
        <61296e93-6268-05cd-e876-680e07645a16@intel.com>
Organization: Red Hat
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

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