From: Robin Murphy <robin.murphy@arm.com> To: Marc Gonzalez <marc.w.gonzalez@free.fr>, Marc Zyngier <marc.zyngier@arm.com>, Will Deacon <will.deacon@arm.com> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>, AngeloGioacchino Del Regno <kholk11@gmail.com>, Jens Axboe <axboe@kernel.dk>, Catalin Marinas <catalin.marinas@arm.com>, LKML <linux-kernel@vger.kernel.org>, Linux ARM <linux-arm-kernel@lists.infradead.org>, MSM <linux-arm-msm@vger.kernel.org>, Jeffrey Hugo <jhugo@codeaurora.org> Subject: Re: [PATCH] arm64/io: Don't use WZR in writel Date: Mon, 18 Mar 2019 16:04:03 +0000 [thread overview] Message-ID: <8eb4f446-6152-ffb6-9529-77fb0bcc307f@arm.com> (raw) In-Reply-To: <33d765b5-1807-fa6c-1ceb-99f09f7c8d5a@free.fr> On 12/03/2019 12:36, Marc Gonzalez wrote: > On 24/02/2019 04:53, Bjorn Andersson wrote: > >> On Sat 23 Feb 10:37 PST 2019, Marc Zyngier wrote: >> >>> On Sat, 23 Feb 2019 18:12:54 +0000, Bjorn Andersson wrote: >>>> >>>> On Mon 11 Feb 06:59 PST 2019, Marc Zyngier wrote: >>>> >>>>> On 11/02/2019 14:29, AngeloGioacchino Del Regno wrote: >>>>> >>>>>> Also, just one more thing: yes this thing is going ARM64-wide and >>>>>> - from my findings - it's targeting certain Qualcomm SoCs, but... >>>>>> I'm not sure that only QC is affected by that, others may as well >>>>>> have the same stupid bug. >>>>> >>>>> At the moment, only QC SoCs seem to be affected, probably because >>>>> everyone else has debugged their hypervisor (or most likely doesn't >>>>> bother with shipping one). >>>>> >>>>> In all honesty, we need some information from QC here: which SoCs are >>>>> affected, what is the exact nature of the bug, can it be triggered from >>>>> EL0. Randomly papering over symptoms is not something I really like >>>>> doing, and is likely to generate problems on unaffected systems. >>>> >>>> The bug at hand is that the XZR is not deemed a valid source in the >>>> virtualization of the SMMU registers. It was identified and fixed for >>>> all platforms that are shipping kernels based on v4.9 or later. >>> >>> When you say "fixed": Do you mean fixed in the firmware? Or by adding >>> a workaround in the shipped kernel? >> >> I mean that it's fixed in the firmware. >> >>> If the former, is this part of an official QC statement, with an >>> associated erratum number? >> >> I don't know, will get back to you on this one. >> >>> Is this really limited to the SMMU accesses? >> >> Yes. >> >>>> As such Angelo's list of affected platforms covers the high-profile >>>> ones. In particular MSM8996 and MSM8998 is getting pretty good support >>>> upstream, if we can figure out a way around this issue. >>> >>> We'd need an exhaustive list of the affected SoCs, and work out if we >>> can limit the hack to the SMMU driver (cc'ing Robin, who's the one >>> who'd know about it). >> >> I will try to compose a list. > > FWIW, I have just been bitten by this issue. I needed to enable an SMMU to > filter PCIe EP accesses to system RAM (or something). I'm using an APQ8098 > MEDIABOX dev board. My system hangs in arm_smmu_device_reset() doing: > > /* Invalidate the TLB, just in case */ > writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLH); > writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLNSNH); > > > With the 'Z' constraint, gcc generates: > > str wzr, [x0] > > without the 'Z' constraint, gcc generates: > > mov w1, 0 > str w1, [x0] > > > I can work around the problem using the following patch: > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c > index 045d93884164..93117519aed8 100644 > --- a/drivers/iommu/arm-smmu.c > +++ b/drivers/iommu/arm-smmu.c > @@ -59,6 +59,11 @@ > > #include "arm-smmu-regs.h" > > +static inline void qcom_writel(u32 val, volatile void __iomem *addr) > +{ > + asm volatile("str %w0, [%1]" : : "r" (val), "r" (addr)); > +} > + > #define ARM_MMU500_ACTLR_CPRE (1 << 1) > > #define ARM_MMU500_ACR_CACHE_LOCK (1 << 26) > @@ -422,7 +427,7 @@ static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu, > { > unsigned int spin_cnt, delay; > > - writel_relaxed(0, sync); > + qcom_writel(0, sync); > for (delay = 1; delay < TLB_LOOP_TIMEOUT; delay *= 2) { > for (spin_cnt = TLB_SPIN_COUNT; spin_cnt > 0; spin_cnt--) { > if (!(readl_relaxed(status) & sTLBGSTATUS_GSACTIVE)) > @@ -1760,8 +1765,8 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu) > } > > /* Invalidate the TLB, just in case */ > - writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLH); > - writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLNSNH); > + qcom_writel(0, gr0_base + ARM_SMMU_GR0_TLBIALLH); > + qcom_writel(0, gr0_base + ARM_SMMU_GR0_TLBIALLNSNH); > > reg = readl_relaxed(ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0); > > > > > Can a quirk be used to work around the issue? > Or can we just "pessimize" the 3 writes for everybody? > (Might be cheaper than a test anyway) If it really is just the SMMU driver which is affected, we can work around it for free (not counting the 'cost' of slightly-weird-looking code, of course). If the diff below works as expected, I'll write it up properly. Robin. ----->8----- diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index 045d93884164..7ff29e33298f 100644 --- a/drivers/iommu/arm-smmu.c +++ b/drivers/iommu/arm-smmu.c @@ -422,7 +422,7 @@ static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu, { unsigned int spin_cnt, delay; - writel_relaxed(0, sync); + writel_relaxed((unsigned long)sync, sync); for (delay = 1; delay < TLB_LOOP_TIMEOUT; delay *= 2) { for (spin_cnt = TLB_SPIN_COUNT; spin_cnt > 0; spin_cnt--) { if (!(readl_relaxed(status) & sTLBGSTATUS_GSACTIVE)) @@ -681,7 +681,12 @@ static void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx) /* Unassigned context banks only need disabling */ if (!cfg) { - writel_relaxed(0, cb_base + ARM_SMMU_CB_SCTLR); + /* + * For Qualcomm reasons, we want to guarantee that we write a + * zero from a register which is not WZR. Fortunately, the cfg + * logic here plays right into our hands... + */ + writel_relaxed((unsigned long)cfg, cb_base + ARM_SMMU_CB_SCTLR); return; } @@ -1760,8 +1765,8 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu) } /* Invalidate the TLB, just in case */ - writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLH); - writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLNSNH); + writel_relaxed(reg, gr0_base + ARM_SMMU_GR0_TLBIALLH); + writel_relaxed(reg, gr0_base + ARM_SMMU_GR0_TLBIALLNSNH); reg = readl_relaxed(ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
WARNING: multiple messages have this Message-ID (diff)
From: Robin Murphy <robin.murphy@arm.com> To: Marc Gonzalez <marc.w.gonzalez@free.fr>, Marc Zyngier <marc.zyngier@arm.com>, Will Deacon <will.deacon@arm.com> Cc: Jens Axboe <axboe@kernel.dk>, Jeffrey Hugo <jhugo@codeaurora.org>, Catalin Marinas <catalin.marinas@arm.com>, LKML <linux-kernel@vger.kernel.org>, Bjorn Andersson <bjorn.andersson@linaro.org>, MSM <linux-arm-msm@vger.kernel.org>, AngeloGioacchino Del Regno <kholk11@gmail.com>, Linux ARM <linux-arm-kernel@lists.infradead.org> Subject: Re: [PATCH] arm64/io: Don't use WZR in writel Date: Mon, 18 Mar 2019 16:04:03 +0000 [thread overview] Message-ID: <8eb4f446-6152-ffb6-9529-77fb0bcc307f@arm.com> (raw) In-Reply-To: <33d765b5-1807-fa6c-1ceb-99f09f7c8d5a@free.fr> On 12/03/2019 12:36, Marc Gonzalez wrote: > On 24/02/2019 04:53, Bjorn Andersson wrote: > >> On Sat 23 Feb 10:37 PST 2019, Marc Zyngier wrote: >> >>> On Sat, 23 Feb 2019 18:12:54 +0000, Bjorn Andersson wrote: >>>> >>>> On Mon 11 Feb 06:59 PST 2019, Marc Zyngier wrote: >>>> >>>>> On 11/02/2019 14:29, AngeloGioacchino Del Regno wrote: >>>>> >>>>>> Also, just one more thing: yes this thing is going ARM64-wide and >>>>>> - from my findings - it's targeting certain Qualcomm SoCs, but... >>>>>> I'm not sure that only QC is affected by that, others may as well >>>>>> have the same stupid bug. >>>>> >>>>> At the moment, only QC SoCs seem to be affected, probably because >>>>> everyone else has debugged their hypervisor (or most likely doesn't >>>>> bother with shipping one). >>>>> >>>>> In all honesty, we need some information from QC here: which SoCs are >>>>> affected, what is the exact nature of the bug, can it be triggered from >>>>> EL0. Randomly papering over symptoms is not something I really like >>>>> doing, and is likely to generate problems on unaffected systems. >>>> >>>> The bug at hand is that the XZR is not deemed a valid source in the >>>> virtualization of the SMMU registers. It was identified and fixed for >>>> all platforms that are shipping kernels based on v4.9 or later. >>> >>> When you say "fixed": Do you mean fixed in the firmware? Or by adding >>> a workaround in the shipped kernel? >> >> I mean that it's fixed in the firmware. >> >>> If the former, is this part of an official QC statement, with an >>> associated erratum number? >> >> I don't know, will get back to you on this one. >> >>> Is this really limited to the SMMU accesses? >> >> Yes. >> >>>> As such Angelo's list of affected platforms covers the high-profile >>>> ones. In particular MSM8996 and MSM8998 is getting pretty good support >>>> upstream, if we can figure out a way around this issue. >>> >>> We'd need an exhaustive list of the affected SoCs, and work out if we >>> can limit the hack to the SMMU driver (cc'ing Robin, who's the one >>> who'd know about it). >> >> I will try to compose a list. > > FWIW, I have just been bitten by this issue. I needed to enable an SMMU to > filter PCIe EP accesses to system RAM (or something). I'm using an APQ8098 > MEDIABOX dev board. My system hangs in arm_smmu_device_reset() doing: > > /* Invalidate the TLB, just in case */ > writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLH); > writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLNSNH); > > > With the 'Z' constraint, gcc generates: > > str wzr, [x0] > > without the 'Z' constraint, gcc generates: > > mov w1, 0 > str w1, [x0] > > > I can work around the problem using the following patch: > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c > index 045d93884164..93117519aed8 100644 > --- a/drivers/iommu/arm-smmu.c > +++ b/drivers/iommu/arm-smmu.c > @@ -59,6 +59,11 @@ > > #include "arm-smmu-regs.h" > > +static inline void qcom_writel(u32 val, volatile void __iomem *addr) > +{ > + asm volatile("str %w0, [%1]" : : "r" (val), "r" (addr)); > +} > + > #define ARM_MMU500_ACTLR_CPRE (1 << 1) > > #define ARM_MMU500_ACR_CACHE_LOCK (1 << 26) > @@ -422,7 +427,7 @@ static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu, > { > unsigned int spin_cnt, delay; > > - writel_relaxed(0, sync); > + qcom_writel(0, sync); > for (delay = 1; delay < TLB_LOOP_TIMEOUT; delay *= 2) { > for (spin_cnt = TLB_SPIN_COUNT; spin_cnt > 0; spin_cnt--) { > if (!(readl_relaxed(status) & sTLBGSTATUS_GSACTIVE)) > @@ -1760,8 +1765,8 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu) > } > > /* Invalidate the TLB, just in case */ > - writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLH); > - writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLNSNH); > + qcom_writel(0, gr0_base + ARM_SMMU_GR0_TLBIALLH); > + qcom_writel(0, gr0_base + ARM_SMMU_GR0_TLBIALLNSNH); > > reg = readl_relaxed(ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0); > > > > > Can a quirk be used to work around the issue? > Or can we just "pessimize" the 3 writes for everybody? > (Might be cheaper than a test anyway) If it really is just the SMMU driver which is affected, we can work around it for free (not counting the 'cost' of slightly-weird-looking code, of course). If the diff below works as expected, I'll write it up properly. Robin. ----->8----- diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index 045d93884164..7ff29e33298f 100644 --- a/drivers/iommu/arm-smmu.c +++ b/drivers/iommu/arm-smmu.c @@ -422,7 +422,7 @@ static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu, { unsigned int spin_cnt, delay; - writel_relaxed(0, sync); + writel_relaxed((unsigned long)sync, sync); for (delay = 1; delay < TLB_LOOP_TIMEOUT; delay *= 2) { for (spin_cnt = TLB_SPIN_COUNT; spin_cnt > 0; spin_cnt--) { if (!(readl_relaxed(status) & sTLBGSTATUS_GSACTIVE)) @@ -681,7 +681,12 @@ static void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx) /* Unassigned context banks only need disabling */ if (!cfg) { - writel_relaxed(0, cb_base + ARM_SMMU_CB_SCTLR); + /* + * For Qualcomm reasons, we want to guarantee that we write a + * zero from a register which is not WZR. Fortunately, the cfg + * logic here plays right into our hands... + */ + writel_relaxed((unsigned long)cfg, cb_base + ARM_SMMU_CB_SCTLR); return; } @@ -1760,8 +1765,8 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu) } /* Invalidate the TLB, just in case */ - writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLH); - writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLNSNH); + writel_relaxed(reg, gr0_base + ARM_SMMU_GR0_TLBIALLH); + writel_relaxed(reg, gr0_base + ARM_SMMU_GR0_TLBIALLNSNH); reg = readl_relaxed(ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0); _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-03-18 16:04 UTC|newest] Thread overview: 74+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-02-09 18:34 [PATCH] arm64/io: Don't use WZR in writel AngeloGioacchino Del Regno 2019-02-09 18:34 ` AngeloGioacchino Del Regno 2019-02-09 18:34 ` AngeloGioacchino Del Regno 2019-02-11 10:57 ` Will Deacon 2019-02-11 10:57 ` Will Deacon 2019-02-11 11:52 ` Marc Zyngier 2019-02-11 11:52 ` Marc Zyngier 2019-02-11 14:29 ` AngeloGioacchino Del Regno 2019-02-11 14:29 ` AngeloGioacchino Del Regno 2019-02-11 14:59 ` Marc Zyngier 2019-02-11 14:59 ` Marc Zyngier 2019-02-11 16:15 ` AngeloGioacchino Del Regno 2019-02-11 16:15 ` AngeloGioacchino Del Regno 2019-02-11 16:37 ` Robin Murphy 2019-02-11 16:37 ` Robin Murphy 2019-02-23 18:12 ` Bjorn Andersson 2019-02-23 18:12 ` Bjorn Andersson 2019-02-23 18:37 ` Marc Zyngier 2019-02-23 18:37 ` Marc Zyngier 2019-02-23 18:37 ` Marc Zyngier 2019-02-24 3:53 ` Bjorn Andersson 2019-02-24 3:53 ` Bjorn Andersson 2019-03-12 12:36 ` Marc Gonzalez 2019-03-12 12:36 ` Marc Gonzalez 2019-03-18 16:04 ` Robin Murphy [this message] 2019-03-18 16:04 ` Robin Murphy 2019-03-18 17:00 ` Russell King - ARM Linux admin 2019-03-18 17:00 ` Russell King - ARM Linux admin 2019-03-18 17:11 ` Ard Biesheuvel 2019-03-18 17:11 ` Ard Biesheuvel 2019-03-18 17:19 ` Robin Murphy 2019-03-18 17:19 ` Robin Murphy 2019-03-18 17:24 ` Robin Murphy 2019-03-18 17:24 ` Robin Murphy 2019-03-19 11:45 ` Robin Murphy 2019-03-19 11:45 ` Robin Murphy 2019-03-18 17:30 ` Marc Gonzalez 2019-03-18 17:30 ` Marc Gonzalez 2019-03-18 17:59 ` Robin Murphy 2019-03-18 17:59 ` Robin Murphy 2019-05-02 16:05 ` Marc Gonzalez 2019-05-02 16:05 ` Marc Gonzalez 2019-05-02 16:05 ` Marc Gonzalez 2019-05-02 16:33 ` Robin Murphy 2019-05-02 16:33 ` Robin Murphy 2019-05-02 16:33 ` Robin Murphy 2019-05-02 16:50 ` Marc Gonzalez 2019-05-02 16:50 ` Marc Gonzalez 2019-05-02 16:50 ` Marc Gonzalez 2019-05-03 11:36 ` Marc Gonzalez 2019-05-03 11:36 ` Marc Gonzalez 2019-05-03 11:36 ` Marc Gonzalez 2019-05-03 12:48 ` Robin Murphy 2019-05-03 12:48 ` Robin Murphy 2019-05-03 13:07 ` Marc Gonzalez 2019-05-03 13:07 ` Marc Gonzalez 2019-05-03 13:07 ` Marc Gonzalez 2019-05-04 13:35 ` AngeloGioacchino Del Regno 2019-05-04 13:35 ` AngeloGioacchino Del Regno 2019-05-04 13:35 ` AngeloGioacchino Del Regno 2019-05-05 18:05 ` AngeloGioacchino Del Regno 2019-05-05 18:05 ` AngeloGioacchino Del Regno 2019-05-05 18:05 ` AngeloGioacchino Del Regno 2019-05-20 15:05 ` Marc Gonzalez 2019-05-20 15:05 ` Marc Gonzalez 2019-05-02 17:27 ` Marc Gonzalez 2019-05-02 17:27 ` Marc Gonzalez 2019-05-02 17:27 ` Marc Gonzalez 2019-05-03 0:38 ` Bjorn Andersson 2019-05-03 0:38 ` Bjorn Andersson 2019-05-03 0:38 ` Bjorn Andersson -- strict thread matches above, loose matches on Subject: below -- 2019-02-09 18:30 AngeloGioacchino Del Regno 2019-02-09 18:30 ` AngeloGioacchino Del Regno 2019-02-09 18:30 ` AngeloGioacchino Del Regno
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=8eb4f446-6152-ffb6-9529-77fb0bcc307f@arm.com \ --to=robin.murphy@arm.com \ --cc=axboe@kernel.dk \ --cc=bjorn.andersson@linaro.org \ --cc=catalin.marinas@arm.com \ --cc=jhugo@codeaurora.org \ --cc=kholk11@gmail.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=marc.w.gonzalez@free.fr \ --cc=marc.zyngier@arm.com \ --cc=will.deacon@arm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.