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

[Julien Grall <julien@xxxxxxx>]

Hi Roger,

On 15/04/2021 14:26, Roger Pau Monné wrote:
> 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).
Well... x86-specific devices yes. We don't know in what form MSI will be 
added on for other arch-specific devices.
 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.
My preference would be to comment in the commit message (although, there 
is no promise they will ever get re-used). We can then modify the #ifdef 
once we introduce any use.
Cheers,

--
Julien Grall