lkml.org 
[lkml]   [2017]   [Jan]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [RFC 2/4] irqchip, gicv3-its:Workaround for HiSilicon erratum 161010801
Date


> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy@arm.com]
> Sent: Tuesday, January 24, 2017 4:30 PM
> To: Shameerali Kolothum Thodi; Marc Zyngier; mark.rutland@arm.com;
> will.deacon@arm.com; eric.auger@redhat.com
> Cc: linux-arm-kernel@lists.infradead.org; Linuxarm; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org; John Garry;
> Guohanjun (Hanjun Guo)
> Subject: Re: [RFC 2/4] irqchip, gicv3-its:Workaround for HiSilicon
> erratum 161010801
>
> On 24/01/17 16:14, Shameerali Kolothum Thodi wrote:
> >
> >
> >> -----Original Message-----
> >> From: Marc Zyngier [mailto:marc.zyngier@arm.com]
> >> Sent: Tuesday, January 24, 2017 3:50 PM
> >> To: Shameerali Kolothum Thodi; Robin Murphy; mark.rutland@arm.com;
> >> will.deacon@arm.com; eric.auger@redhat.com
> >> Cc: linux-arm-kernel@lists.infradead.org; Linuxarm; linux-
> >> kernel@vger.kernel.org; devicetree@vger.kernel.org; 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@arm.com]
> >>>> Sent: Tuesday, January 24, 2017 2:42 PM
> >>>> To: Marc Zyngier; Shameerali Kolothum Thodi; mark.rutland@arm.com;
> >>>> will.deacon@arm.com
> >>>> Cc: linux-arm-kernel@lists.infradead.org; Linuxarm; linux-
> >>>> kernel@vger.kernel.org; devicetree@vger.kernel.org; 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@huawei.com>
> >>>>>> ---
> >>>>>> 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.

Ok. Got it. Since the PCIe RC tags the transaction with the device ID,
the ITS is able to perform IRQ isolation and is safe.

Thanks,
Shameer

\
 
 \ /
  Last update: 2017-01-24 17:42    [W:1.274 / U:1.236 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site