Hi Mark, > -----Original Message----- > From: Auger Eric [mailto:eric.auger@xxxxxxxxxx] > Sent: Thursday, January 05, 2017 5:39 PM > To: Marc Zyngier <marc.zyngier@xxxxxxx>; eric.auger.pro@xxxxxxxxx; > christoffer.dall@xxxxxxxxxx; robin.murphy@xxxxxxx; > alex.williamson@xxxxxxxxxx; will.deacon@xxxxxxx; joro@xxxxxxxxxx; > tglx@xxxxxxxxxxxxx; jason@xxxxxxxxxxxxxx; linux-arm- > kernel@xxxxxxxxxxxxxxxxxxx > Cc: drjones@xxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; punit.agrawal@xxxxxxx; > linux-kernel@xxxxxxxxxxxxxxx; geethasowjanya.akula@xxxxxxxxx; Diana > Madalina Craciun <diana.craciun@xxxxxxx>; iommu@lists.linux- > foundation.org; pranav.sawargaonkar@xxxxxxxxx; Bharat Bhushan > <bharat.bhushan@xxxxxxx>; shankerd@xxxxxxxxxxxxxx; > gpkulkarni@xxxxxxxxx > 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@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! 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? Thanks -Bharat > > Eric > > > > 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
