[PATCH v5 13/17] irqdomain: irq_domain_check_msi_remap

From: Marc Zyngier <hidden>
Date: 2017-01-06 09:52:11
Also in: kvm, linux-iommu, lkml

On 06/01/17 04:27, Bharat Bhushan wrote:

Hi Mark,

quoted

-----Original Message-----
From: Auger Eric [mailto:eric.auger at redhat.com]
Sent: Thursday, January 05, 2017 5:39 PM
To: Marc Zyngier <redacted>; eric.auger.pro at gmail.com;
christoffer.dall at linaro.org; robin.murphy at arm.com;
alex.williamson at redhat.com; will.deacon at arm.com; joro at 8bytes.org;
tglx at linutronix.de; jason at lakedaemon.net; linux-arm-
kernel at lists.infradead.org
Cc: drjones at redhat.com; kvm at vger.kernel.org; punit.agrawal at arm.com;
linux-kernel at vger.kernel.org; geethasowjanya.akula at gmail.com; Diana
Madalina Craciun [off-list ref]; iommu at lists.linux-
foundation.org; pranav.sawargaonkar at gmail.com; Bharat Bhushan
[off-list ref]; shankerd at codeaurora.org;
gpkulkarni at gmail.com
Subject: Re: [PATCH v5 13/17] irqdomain: irq_domain_check_msi_remap

Hi Marc,

On 05/01/2017 12:57, Marc Zyngier wrote:

quoted

On 05/01/17 11:29, Auger Eric wrote:

quoted

Hi Marc,

On 05/01/2017 12:25, Marc Zyngier wrote:

quoted

On 05/01/17 10:45, Auger Eric wrote:

quoted

Hi Marc,

On 04/01/2017 16:27, Marc Zyngier wrote:

quoted

On 04/01/17 14:11, Auger Eric wrote:

quoted

Hi Marc,

On 04/01/2017 14:46, Marc Zyngier wrote:

quoted

Hi Eric,

On 04/01/17 13:32, Eric Auger wrote:

quoted

This new function checks whether all platform and PCI MSI
domains implement IRQ remapping. This is useful to understand
whether VFIO passthrough is safe with respect to interrupts.

On ARM typically an MSI controller can sit downstream to the
IOMMU without preventing VFIO passthrough.
As such any assigned device can write into the MSI doorbell.
In case the MSI controller implements IRQ remapping, assigned
devices will not be able to trigger interrupts towards the
host. On the contrary, the assignment must be emphasized as
unsafe with respect to interrupts.

Signed-off-by: Eric Auger <eric.auger@redhat.com>

---

v4 -> v5:
- Handle DOMAIN_BUS_FSL_MC_MSI domains
- Check parents
---
 include/linux/irqdomain.h |  1 +
 kernel/irq/irqdomain.c    | 41

+++++++++++++++++++++++++++++++++++++++++

quoted

 2 files changed, 42 insertions(+)

diff --git a/include/linux/irqdomain.h

b/include/linux/irqdomain.h index ab017b2..281a40f 100644

--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h

@@ -219,6 +219,7 @@ struct irq_domain

*irq_domain_add_legacy(struct device_node *of_node,

quoted

 					 void *host_data);
 extern struct irq_domain *irq_find_matching_fwspec(struct

irq_fwspec *fwspec,

quoted

quoted
quoted
quoted
quoted
quoted
quoted
quoted
 						   enum

irq_domain_bus_token bus_token);

quoted

+extern bool irq_domain_check_msi_remap(void);
 extern void irq_set_default_host(struct irq_domain *host);
extern int irq_domain_alloc_descs(int virq, unsigned int nr_irqs,
 				  irq_hw_number_t hwirq, int node,

diff --git

quoted

a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c index
8c0a0ae..700caea 100644

--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c

@@ -278,6 +278,47 @@ struct irq_domain

*irq_find_matching_fwspec(struct irq_fwspec *fwspec,
EXPORT_SYMBOL_GPL(irq_find_matching_fwspec);

 /**
+ * irq_domain_is_msi_remap - Check if @domain or any parent
+ * has MSI remapping support
+ * @domain: domain pointer
+ */
+static bool irq_domain_is_msi_remap(struct irq_domain

*domain)

quoted

+{
+	struct irq_domain *h = domain;
+
+	for (; h; h = h->parent) {
+		if (h->flags & IRQ_DOMAIN_FLAG_MSI_REMAP)
+			return true;
+	}
+	return false;
+}
+
+/**
+ * irq_domain_check_msi_remap() - Checks whether all MSI
+ * irq domains implement IRQ remapping  */ bool
+irq_domain_check_msi_remap(void) {
+	struct irq_domain *h;
+	bool ret = true;
+
+	mutex_lock(&irq_domain_mutex);
+	list_for_each_entry(h, &irq_domain_list, link) {
+		if (((h->bus_token & DOMAIN_BUS_PCI_MSI) ||
+		     (h->bus_token & DOMAIN_BUS_PLATFORM_MSI)

||

quoted

quoted
quoted
quoted
quoted
quoted
quoted
quoted
+		     (h->bus_token & DOMAIN_BUS_FSL_MC_MSI))

&&

quoted

quoted
quoted
quoted
quoted
quoted
quoted
quoted
+		     !irq_domain_is_msi_remap(h)) {
(h->bus_token & DOMAIN_BUS_PCI_MSI) and co looks quite

wrong.

quoted

bus_token is not a bitmap, and DOMAIN_BUS_* not a single bit
value (see enum irq_domain_bus_token). Surely this should read
(h->bus_token == DOMAIN_BUS_PCI_MSI).

Oh I did not notice that. Thanks.

Any other comments on the irqdomain side? Do you think the
current approach consisting in looking at those bus tokens and
their parents looks good?

To be completely honest, I don't like it much, as having to
enumerate all the bus types can come up with could become quite a
burden in the long run. I'd rather be able to identify MSI capable
domains by construction. I came up with the following approach (fully

untested):

quoted

diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 281a40f..7779796 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h

@@ -183,8 +183,11 @@ enum {
 	/* Irq domain is an IPI domain with single virq */
 	IRQ_DOMAIN_FLAG_IPI_SINGLE	= (1 << 3),

+	/* Irq domain implements MSIs */
+	IRQ_DOMAIN_FLAG_MSI		= (1 << 4),
+
 	/* Irq domain is MSI remapping capable */
-	IRQ_DOMAIN_FLAG_MSI_REMAP	= (1 << 4),
+	IRQ_DOMAIN_FLAG_MSI_REMAP	= (1 << 5),

 	/*
 	 * Flags starting from IRQ_DOMAIN_FLAG_NONCORE are reserved

@@

quoted

-450,6 +453,11 @@ static inline bool
irq_domain_is_ipi_single(struct irq_domain *domain)  {
 	return domain->flags & IRQ_DOMAIN_FLAG_IPI_SINGLE;  }
+
+static inline bool irq_domain_is_msi(struct irq_domain *domain) {
+	return domain->flags & IRQ_DOMAIN_FLAG_MSI; }
 #else	/* CONFIG_IRQ_DOMAIN_HIERARCHY */
 static inline void irq_domain_activate_irq(struct irq_data *data)
{ }  static inline void irq_domain_deactivate_irq(struct irq_data
*data) { } @@ -481,6 +489,11 @@ static inline bool
irq_domain_is_ipi_single(struct irq_domain *domain)  {
 	return false;
 }
+
+static inline bool irq_domain_is_msi(struct irq_domain *domain) {
+	return false;
+}
 #endif	/* CONFIG_IRQ_DOMAIN_HIERARCHY */

 #else /* CONFIG_IRQ_DOMAIN */

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c index

700caea..33b6921 100644

--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c

@@ -304,10 +304,7 @@ bool irq_domain_check_msi_remap(void)

 	mutex_lock(&irq_domain_mutex);
 	list_for_each_entry(h, &irq_domain_list, link) {
-		if (((h->bus_token & DOMAIN_BUS_PCI_MSI) ||
-		     (h->bus_token & DOMAIN_BUS_PLATFORM_MSI)

||

quoted

quoted
quoted
quoted
quoted
-		     (h->bus_token & DOMAIN_BUS_FSL_MC_MSI))

&&

quoted

-		     !irq_domain_is_msi_remap(h)) {
+		if (irq_domain_is_msi(h) &&

!irq_domain_is_msi_remap(h)) {

quoted

 			ret = false;
 			goto out;
 		}

diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c index

ee23006..b637263 100644

--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c

@@ -270,7 +270,7 @@ struct irq_domain

*msi_create_irq_domain(struct fwnode_handle *fwnode,

quoted

 	if (info->flags & MSI_FLAG_USE_DEF_CHIP_OPS)
 		msi_domain_update_chip_ops(info);

-	return irq_domain_create_hierarchy(parent, 0, 0, fwnode,
+	return irq_domain_create_hierarchy(parent,

IRQ_DOMAIN_FLAG_MSI,

quoted

+0, fwnode,
 					   &msi_domain_ops, info);
 }



Thoughts?

Don't we need to set the IRQ_DOMAIN_FLAG_MSI flag in
platform_msi_create_device_domain too (drivers/base/platform-

msi.c)?

quoted

was mentioning platform_msi_create_*device*_domain.
it calls irq_domain_create_hierarchy and looks to be MSI irq domain
related. But I don't have a full understanding of the whole irq
domain hierarchy.

Ah, sorry - I blame the ARM coffee.

This function builds a domain for a single device on top of the MSI
domain that has been already created (see the dev->msi_domain passed
to irq_domain_create_hierarchy). The structure looks like this:

device-domain -> platform MSI domain -> HW MSI domain -> whatever

So what we're *really* interested in is the platform MSI domain, which
is going to carry the IRQ_DOMAIN_FLAG_MSI flag. The device-domain only
describes a portion of it, and can safely be ignored.

In the end, what matters for this patch is that we can prove that from
any domain carrying the IRQ_DOMAIN_FLAG_MSI flag, we can find a

domain

quoted

carrying the IRQ_DOMAIN_FLAG_MSI_REMAP flag. If that property holds,
we're safe. Otherwise, we disable the Guest MSI feature.

Does it make sense?

Yes it makes sense. Thank you for the explanation!

If I understood correctly then the domain hierarchy is 

 -> "gic-irq-domain"
	 -> "gic-its-irq-domain"
		-> "platform-msi-domain"
		-> "pci-msi-domain"
		->  "fsl-mc-msi-domain"

"gic-its-irq-domain" carries IRQ_DOMAIN_FLAG_MSI_REMAP

So we need to look for the IRQ_DOMAIN_FLAG_MSI_REMAP flag in
"gic-its-irq-domain" when doing safety check for
"platform/pci/fsl-mc"-msi-irqdomain,  is this what you mentioned?

Can we can pass this flags from "gic-its-irq-domain" to
"platform/pci/fsl-mc"-msi-irqdomain during domain creation?

No, that's a very bad idea. It is not that domain that provides the
isolation, as it merely provides a software abstraction for a bus. On
the other hand, the underlying domain represents the HW that does
provide such isolation. Only that domain can claim that isolation will
be enforced.

So we only set the REMAP flag on the ITS domain, and place the MSI flag
on the top level MSI domains. And if we can prove that *all* MSI domains
have a parent implementing remapping, we're safe. Any other
configuration is unsafe.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help