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

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

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. Was there an aim to use that in other
arches?

The hook in iommu_ops also need to be moved inside the x86 region.
Please do this iommu change in a separate patch.

> > > diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
> > > index 9f5b5d52e1..a6b12717b1 100644
> > > --- a/xen/include/xen/vpci.h
> > > +++ b/xen/include/xen/vpci.h
> > > @@ -91,6 +91,7 @@ struct vpci {
> > >           /* FIXME: currently there's no support for SR-IOV. */
> > >       } header;
> > > +#ifdef CONFIG_PCI_MSI_INTERCEPT
> > >       /* MSI data. */
> > >       struct vpci_msi {
> > >         /* Address. */
> > > @@ -136,6 +137,7 @@ struct vpci {
> > >               struct vpci_arch_msix_entry arch;
> > >           } entries[];
> > >       } *msix;
> > > +#endif /* CONFIG_PCI_MSI_INTERCEPT */
> > 
> > Note that here you just remove two pointers from the struct, not that
> > I'm opposed to it, but it's not that much space that's saved anyway.
> > Ie: it might also be fine to just leave them as NULL unconditionally
> > on Arm.
> 
> Can the two pointers be NULL on x86?

Yes, they can be NULL on x86.

> If not, then I would prefer if they
> disappear on Arm so there is less chance to make any mistake (such as
> unconditionally accessing the pointer in common code).

Any access to them needs to be protected anyway, or else we would be
in trouble. I don't think Xen ever accesses them based on the PCI
capabilities of the device.

Thanks, Roger.