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

[Auger Eric]

Hi Marc,

On 05/01/2017 12:57, Marc Zyngier wrote:
> On 05/01/17 11:29, Auger Eric wrote:
>> Hi Marc,
>>
>> On 05/01/2017 12:25, Marc Zyngier wrote:
>>> On 05/01/17 10:45, Auger Eric wrote:
>>>> Hi Marc,
>>>>
>>>> On 04/01/2017 16:27, Marc Zyngier wrote:
>>>>> 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@xxxxxxxxxx>
>>>>>>>>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> 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?
>>>>
>>>> Don't we need to set the IRQ_DOMAIN_FLAG_MSI flag in
>>>> platform_msi_create_device_domain too (drivers/base/platform-msi.c)?
>> 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
> 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!

Eric
> 
> Thanks,
> 
> 	M.
>