On 02/05/2019 18:33, Robin Murphy wrote: > Both Angelo's and your reports strongly imply that the previous > constant-folding debate was a red herring and the trivial fix[1] should > still be sufficient, but nobody's given me actual confirmation of > whether it is or isn't :( > > Robin. > > [1] > http://linux-arm.org/git?p=linux-rm.git;a=commitdiff;h=a13e3239f0c543f1f61ce5f7f5c06320e521701c > > > Apparently some Qualcomm arm64 platforms which appear to expose their I'd write qcom. I don't think they deserve to be named & capitalized :'p > SMMU global register space are still in fact using a hypervisor to > mediate it by trapping and emulating register accesses. Sadly, some > deployed versions of said trapping code have bugs wherein they go > horribly wrong for stores using r31 (i.e. XZR/WZR) as the source > register. > > While this can be mitigated for GCC today by tweaking the constraints > for the implementation of writel_relaxed(), to avoid any potential arms > race with future compilers compilers more aggressively optimising "compilers compilers" ... typo? > register allocation the simple way is to just remove all the problematic > constant zeros. For the write-only TLB operations, the actual value is > irrelevant anyway and any old nearby variable will provide a suitable > GPR to encode. The one point at which we really do need a zero to clear > a context bank happens before any of the TLB maintenance where hangs > have been reported, so is apparently not a problem... :/ > > Reported-by: Angelo G. Del Regno <kholk11@xxxxxxxxx> > Reported-by: Marc Gonzalez <marc.w.gonzalez@xxxxxxx> > Signed-off-by: Robin Murphy <robin.murphy@xxxxxxx> > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c > index 045d938..80bf29e 100644 (file) > --- 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); You don't think this might deserve a comment explaining that the value is irrelevant? (On top of the commit message, I mean.) > 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 +1760,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); Same here? Anyway, your solution works on msm8998, therefore you have my Tested-by: Marc Gonzalez <marc.w.gonzalez@xxxxxxx> and you can throw in my Reviewed-by: Marc Gonzalez <marc.w.gonzalez@xxxxxxx> for good measure ;-) All that's left now is to submit it to Linus during the merge window :-p Regards.
