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.au...@redhat.com>
>>>>
>>>> ---
>>>>
>>>> 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?
No objection from my side. I will respin and test.
Thanks
Eric
>
> M.
>
