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

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

 



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.

Thanks

Eric
> 
> Well, platform_msi_create_irq_domain does call msi_create_irq_domain,
> doesn't it? That's one of the benefits of the generic MSI
> infrastructure: it is the only function that performs the creation of an
> MSI domain for any bus type.
> 
> Or am I missing something completely obvious (which is perfectly
> possible since I only had a couple of cups of the brown stuff...)?
> 
> Thanks,
> 
> 	M.
> 
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux