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.
