RE: [RFC 2/4] irqchip, gicv3-its:Workaround for HiSilicon erratum 161010801

From: Shameerali Kolothum Thodi
Date: Tue Jan 24 2017 - 11:15:35 EST

Next message: Tejun Heo: "Re: [PATCH] ata: pata_of_platform: using of_property_read_u32() helper"
Previous message: Mathieu Poirier: "Re: [PATCH v5 3/3] watchdog: zx2967: add watchdog controller driver for ZTE's zx2967 family"
In reply to: Marc Zyngier: "Re: [RFC 2/4] irqchip, gicv3-its:Workaround for HiSilicon erratum 161010801"
Next in thread: Marc Zyngier: "Re: [RFC 2/4] irqchip, gicv3-its:Workaround for HiSilicon erratum 161010801"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> -----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.

Thanks,
Shameer

[1] https://github.com/eauger/linux/commit/df2a17bd8cc73b5a381ed1570870a2aeb6f7e068

Next message: Tejun Heo: "Re: [PATCH] ata: pata_of_platform: using of_property_read_u32() helper"
Previous message: Mathieu Poirier: "Re: [PATCH v5 3/3] watchdog: zx2967: add watchdog controller driver for ZTE's zx2967 family"
In reply to: Marc Zyngier: "Re: [RFC 2/4] irqchip, gicv3-its:Workaround for HiSilicon erratum 161010801"
Next in thread: Marc Zyngier: "Re: [RFC 2/4] irqchip, gicv3-its:Workaround for HiSilicon erratum 161010801"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]