[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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). But I think the naming would be misleading as the IOMMU will not be used for the remapping.

So most likely, we would want a more generic name (maybe "arch_register_msi()"). This could call iommu_update_ire_from_msi() on x86 and the ITS on Arm.

I don't know how RISCv and PowerPC remap the interrupt. But if they are using the IOMMU then we could provide a generic helper protected by CONFIG_HAS_IOMMU_INTERRUPT_REMAP (or a similar name).


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

+1

Cheers,

--
Julien Grall



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.