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

[Auger Eric <eric.auger-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

Hi Bharat,

On 06/01/2017 05:27, Bharat Bhushan wrote:
> Hi Mark,
> 
>> -----Original Message-----
>> From: Auger Eric [mailto:eric.auger-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org]
>> Sent: Thursday, January 05, 2017 5:39 PM
>> To: Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>; eric.auger.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org;
>> christoffer.dall-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org; robin.murphy-5wv7dgnIgG8@public.gmane.org;
>> alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org; will.deacon-5wv7dgnIgG8@public.gmane.org; joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org;
>> tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org; jason-NLaQJdtUoK4Be96aLqz0jA@public.gmane.org; linux-arm-
>> kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
>> Cc: drjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org; kvm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; punit.agrawal-5wv7dgnIgG8@public.gmane.org;
>> linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; geethasowjanya.akula-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org; Diana
>> Madalina Craciun <diana.craciun-3arQi8VN3Tc@public.gmane.org>; iommu-cunTk1MwBs/ROKNJybVBZg@public.gmane.org
>> foundation.org; pranav.sawargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org; Bharat Bhushan
>> <bharat.bhushan-3arQi8VN3Tc@public.gmane.org>; shankerd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org;
>> gpkulkarni-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
>> Subject: Re: [PATCH v5 13/17] irqdomain: irq_domain_check_msi_remap
>>
>> 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-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?
>>>>>>
>>>>>> 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!
> 
> 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?

fsl_mc_msi_create_irq_domain (drivers/staging/fsl-mc/bus/fsl-mc-msi.c)
calls msi_create_irq_domain. So the associated domain carries the
IRQ_DOMAIN_FLAG_MSI flag. The code will check whether any parent carries
the IRQ_DOMAIN_FLAG_MSI flag. This will be the case (gic-its-irq-domain).

Does it answer your question?

Thanks

Eric
> 
> Thanks
> -Bharat
> 
>>
>> Eric
>>>
>>> Thanks,
>>>
>>> 	M.
>>>
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>