On 24/01/17 16:14, Shameerali Kolothum Thodi wrote: > > >> -----Original Message----- >> From: Marc Zyngier [mailto:marc.zyngier@xxxxxxx] >> Sent: Tuesday, January 24, 2017 3:50 PM >> To: Shameerali Kolothum Thodi; Robin Murphy; mark.rutland@xxxxxxx; >> will.deacon@xxxxxxx; eric.auger@xxxxxxxxxx >> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; Linuxarm; linux- >> kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; John Garry; >> Guohanjun (Hanjun Guo) >> Subject: Re: [RFC 2/4] irqchip, gicv3-its:Workaround for HiSilicon >> erratum 161010801 >> >> On 24/01/17 15:39, Shameerali Kolothum Thodi wrote: >>> +Eric >>> >>>> -----Original Message----- >>>> From: Robin Murphy [mailto:robin.murphy@xxxxxxx] >>>> Sent: Tuesday, January 24, 2017 2:42 PM >>>> To: Marc Zyngier; Shameerali Kolothum Thodi; mark.rutland@xxxxxxx; >>>> will.deacon@xxxxxxx >>>> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; Linuxarm; linux- >>>> kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; John Garry; >>>> Guohanjun (Hanjun Guo) >>>> Subject: Re: [RFC 2/4] irqchip, gicv3-its:Workaround for HiSilicon >>>> erratum 161010801 >>>> >>>> On 24/01/17 14:11, Marc Zyngier wrote: >>>>> + Robin, >>>>> >>>>> On 24/01/17 13:47, Shameerali Kolothum Thodi wrote: >>>>>> The HiSilicon erratum 161010801 describes the limitation of >> certain >>>>>> HiSilicon platforms to support the SMMU mappings for MSI >>>> transactions. >>>>>> >>>>>> On these platforms GICv3 ITS translator is presented with the >>>> deviceID >>>>>> by extending the MSI payload data to 64 bits to include the >>>> deviceID. >>>>>> Hence, the PCIe controller on this platforms has to differentiate >>>> the >>>>>> MSI payload against other DMA payload and has to modify the MSI >>>> payload. >>>>>> This basically makes it difficult for this platforms to have a >> SMMU >>>>>> translation for MSI. Also these platforms doesn't have a proper >>>>>> IIDR register to use the existing IIDR based quirk mechanism. >>>>>> >>>>>> This workaround based on the devicetree binding property, supports >>>>>> bypassing the SMMU for the MSI transactions on this platforms. >>>>>> >>>>>> Signed-off-by: shameer <shameerali.kolothum.thodi@xxxxxxxxxx> >>>>>> --- >>>>>> arch/arm64/Kconfig | 15 ++++++++++++ >>>>>> drivers/irqchip/irq-gic-common.h | 1 + >>>>>> drivers/irqchip/irq-gic-v3-its.c | 52 >>>> +++++++++++++++++++++++++++++++++++++++- >>>>>> 3 files changed, 67 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index >>>>>> 0ae0427..8d600b0 100644 >>>>>> --- a/arch/arm64/Kconfig >>>>>> +++ b/arch/arm64/Kconfig >>>>>> @@ -485,6 +485,21 @@ config CAVIUM_ERRATUM_27456 >>>>>> >>>>>> If unsure, say Y. >>>>>> >>>>>> +config HISILICON_ERRATUM_161010801 >>>>>> + bool "HiSilicon erratum 161010801" >>>>>> + default y >>>>>> + help >>>>>> + Enable workaround for erratum 161010801. >>>>>> + >>>>>> + This implements a gicv3-its errata workaround for >> HiSilicon >>>>>> + platforms Hip05/Hip07. These platforms cannot support the >> MSI >>>>>> + interrupt remapping and MSI transaction has to be >> bypassed by >>>> SMMU. >>>>>> + >>>>>> + The fix is to avoid calling the remapping hook into the >> SMMU >>>>>> + driver from the its_irq_compose_msi_msg(). >>>>>> + >>>>>> + If unsure, say Y. >>>>>> + >>>>>> endmenu >>>>>> >>>>>> >>>>>> diff --git a/drivers/irqchip/irq-gic-common.h >>>>>> b/drivers/irqchip/irq- >>>> gic-common.h >>>>>> index 205e5fd..de0385a 100644 >>>>>> --- a/drivers/irqchip/irq-gic-common.h >>>>>> +++ b/drivers/irqchip/irq-gic-common.h >>>>>> @@ -26,6 +26,7 @@ struct gic_quirk { >>>>>> void (*init)(void *data); >>>>>> u32 iidr; >>>>>> u32 mask; >>>>>> + const char *erratum; >>>>>> }; >>>>>> >>>>>> int gic_configure_irq(unsigned int irq, unsigned int type, diff >>>>>> --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq- >>>> gic-v3-its.c >>>>>> index f471939..0a326f6 100644 >>>>>> --- a/drivers/irqchip/irq-gic-v3-its.c >>>>>> +++ b/drivers/irqchip/irq-gic-v3-its.c >>>>>> @@ -44,6 +44,7 @@ >>>>>> #define ITS_FLAGS_CMDQ_NEEDS_FLUSHING (1ULL << 0) >>>>>> #define ITS_FLAGS_WORKAROUND_CAVIUM_22375 (1ULL << 1) >>>>>> #define ITS_FLAGS_WORKAROUND_CAVIUM_23144 (1ULL << 2) >>>>>> +#define ITS_FLAGS_WORKAROUND_HISILICON_161010801 (1ULL << 3) >>>>>> >>>>>> #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING (1 << 0) >>>>>> >>>>>> @@ -659,7 +660,8 @@ static void its_irq_compose_msi_msg(struct >>>> irq_data *d, struct msi_msg *msg) >>>>>> msg->address_hi = upper_32_bits(addr); >>>>>> msg->data = its_get_event_id(d); >>>>>> >>>>>> - iommu_dma_map_msi_msg(d->irq, msg); >>>>>> + if (!(its->flags & >> ITS_FLAGS_WORKAROUND_HISILICON_161010801)) >>>>>> + iommu_dma_map_msi_msg(d->irq, msg); >>>>> >>>>> Let's contemplate this for a moment. If we're on the affected ITS, >>>> we're >>>>> using the physical address of the GITS_TRANSLATER register. What >>>>> guarantees that this is not going to conflict with an IOVA that DMA >>>> is >>>>> going to use? From looking at these patches, my feeling is "not >>>> much". >>>>> >>>>> So if I'm right, you're opening the door to some interesting memory >>>>> corruption if the two regions ever intersect. >>>>> >>>>> Robin, what do you think? >>>> >>>> Yup. Unless the ITS physical address is actually reserved from the >>>> IOVA domain, it's still free to be allocated for DMA mappings, and >> if >>>> that ever happens then you'll get odd bits of data landing in the >> ITS >>>> instead of RAM, and maybe even locked-up devices or worse if the >>>> doorbell gives back decode errors on read attempts. It's essentially >>>> the exact same problem as we have with memory-mapped PCI windows, >> and >>>> needs to be solved in the same fashion, i.e. between the SMMU and >> the >>>> IOMMU-DMA code. >>> >>> Is this something that can incorporated in Eric's latest patch >> series[1]? >>> It does mentions reserved regions can be: >>> - directly mapped regions >>> - regions that cannot be iommu mapped (PCI host bridge windows, ...) >>> - MSI regions (because they belong to another address space or >> because >>> they are not translated by the IOMMU and need special handling) >>> >>> Though I am not clear our case comes under "the MSI regions that are >>> not translated by the IOMMU and need special handling" or not. >> >> Well, given that in your case, the IOMMU never sees the MSI write, it >> definitely falls into the "not translated" category. >> >> As for handling it on top of Eric's series, that's probably the most >> reasonable thing to do, which also means that none of this should >> appear in the ITS driver. Robin seems to have an idea on how to >> approach this. > > Ok. Thanks for that Marc/Robin. > > But I am not sure we can get away with ITS driver. Because current vfio > patch series[1] treats GICV3 ITS as irq safe and is setting > IRQ_DOMAIN_FLAG_MSI_REMAP in ITS driver. But this is not the case with > our ITS. As far as I understand, it *is* the case, and largely the reason we're here in the first place. "MSI remapping" is an x86-specific term for the part of their IOMMU which intercepts writes to the architectural MSI region and can "remap" a given device's interrupts to different targets on the CPU side - i.e. more or less the same thing the ITS does with the device ID data which your implementation apparently needs this address-matching widget to append to the write. If your ITS couldn't actually distinguish between requesters (like a GICv2m) we'd have a *much* bigger problem. Robin. > > Thanks, > Shameer > > [1] https://github.com/eauger/linux/commit/df2a17bd8cc73b5a381ed1570870a2aeb6f7e068 > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
