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.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? > > Don't we need to set the IRQ_DOMAIN_FLAG_MSI flag in > platform_msi_create_device_domain too (drivers/base/platform-msi.c)?
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. -- Jazz is not dead. It just smells funny... _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu