Re: [PATCH v5 13/17] irqdomain: irq_domain_check_msi_remap

[Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>]

On 04/01/17 14:11, Auger Eric wrote:
> Hi Marc,
> 
> On 04/01/2017 14:46, Marc Zyngier wrote:
>> Hi Eric,
>>
>> On 04/01/17 13:32, Eric Auger wrote:
>>> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>>
>>> ---
>>>
>>> v4 -> v5:
>>> - Handle DOMAIN_BUS_FSL_MC_MSI domains
>>> - Check parents
>>> ---
>>>  include/linux/irqdomain.h |  1 +
>>>  kernel/irq/irqdomain.c    | 41 +++++++++++++++++++++++++++++++++++++++++
>>>  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,
>>>  					 void *host_data);
>>>  extern struct irq_domain *irq_find_matching_fwspec(struct irq_fwspec *fwspec,
>>>  						   enum irq_domain_bus_token bus_token);
>>> +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 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)
>>> +{
>>> +	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) ||
>>> +		     (h->bus_token & DOMAIN_BUS_FSL_MC_MSI)) &&
>>> +		     !irq_domain_is_msi_remap(h)) {
>>
>> (h->bus_token & DOMAIN_BUS_PCI_MSI) and co looks quite wrong. 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):

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
@@ -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) ||
-		     (h->bus_token & DOMAIN_BUS_FSL_MC_MSI)) &&
-		     !irq_domain_is_msi_remap(h)) {
+		if (irq_domain_is_msi(h) && !irq_domain_is_msi_remap(h)) {
 			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,
 	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, 0, fwnode,
 					   &msi_domain_ops, info);
 }
 


Thoughts?

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