Re: [PATCH v2] xen/pci: Refactor PCI MSI interrupts related code

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Apr 14, 2021 at 09:49:37AM +0100, Julien Grall wrote:
> Hi Roger,
> 
> On 14/04/2021 09:05, Roger Pau Monné wrote:
> > On Tue, Apr 13, 2021 at 06:12:10PM +0100, Julien Grall wrote:
> > > Hi Roger,
> > > 
> > > On 12/04/2021 11:49, Roger Pau Monné wrote:
> > > > On Fri, Apr 09, 2021 at 05:00:41PM +0100, Rahul Singh wrote:
> > > > > diff --git a/xen/drivers/passthrough/pci.c 
> > > > > b/xen/drivers/passthrough/pci.c
> > > > > index 705137f8be..1b473502a1 100644
> > > > > --- a/xen/drivers/passthrough/pci.c
> > > > > +++ b/xen/drivers/passthrough/pci.c
> > > > > @@ -1303,12 +1279,15 @@ static int __init setup_dump_pcidevs(void)
> > > > >    }
> > > > >    __initcall(setup_dump_pcidevs);
> > > > > +
> > > > > +#ifdef CONFIG_PCI_MSI_INTERCEPT
> > > > >    int iommu_update_ire_from_msi(
> > > > >        struct msi_desc *msi_desc, struct msi_msg *msg)
> > > > >    {
> > > > >        return iommu_intremap
> > > > >               ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, 
> > > > > msg) : 0;
> > > > >    }
> > > > > +#endif
> > > > 
> > > > This is not exactly related to MSI intercepts, the IOMMU interrupt
> > > > remapping table will also be used for interrupt entries for devices
> > > > being used by Xen directly, where no intercept is required.
> > > 
> > > On Arm, this is not tie to the IOMMU. Instead, this handled is a separate
> > > MSI controller (such as the ITS).
> > > 
> > > > 
> > > > And then you also want to gate the hook from iommu_ops itself with
> > > > CONFIG_PCI_MSI_INTERCEPT, if we want to got this route.
> > > 
> > > 
> > > All the callers are in the x86 code. So how about moving the function in 
> > > an
> > > x86 specific file?
> > 
> > Yes, that seems fine. Just place it in asm-x86/iommu.h. I wonder why
> > update_ire_from_msi wasn't moved when the rest of the x86 specific
> > functions where moved there.
> 
> I am guessing it is because the function was protected by CONFIG_HAS_PCI
> rather than CONFIG_X86. So it was deferred until another arch enables
> CONFIG_HAS_PCI (it is easier to know what code should be moved).
> 
> > Was there an aim to use that in other
> > arches?
> 
> In the future we may need to use MSIs in Xen (IIRC some SMMUs only support
> MSI interrupt).

Then I think some of the bits that are moved from
xen/drivers/passthrough/pci.c (alloc_pci_msi, free_pci_msi and
dump_pci_msi) need to be protected by a Kconfig option different than
CONFIG_PCI_MSI_INTERCEPT, as those are not related to MSI interception,
but to MSI handling in general (ie: Xen devices using MSI need
those). The same with the msi_list and msix fields of pci_dev, those
are not only used for MSI interception.

Or at least might be worth mentioning that the functions will be
needed for Xen to be able to use MSI interrupts itself, even if not
for interception purposes.

Thanks, Roger.