Hi Bharat, On 06/01/2017 05:27, Bharat Bhushan wrote: > 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? 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@xxxxxxxxxxxxxxxxxxx > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > -- 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
