Re: [PATCH] arm64/io: Don't use WZR in writel

From: Marc Gonzalez <marc.w.gonzalez@free.fr>
To: Robin Murphy <robin.murphy@arm.com>,
	Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Mark Rutland <mark.rutland@arm.com>,
	Marc Zyngier <marc.zyngier@arm.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will.deacon@arm.com>,
	Jeffrey Hugo <jhugo@codeaurora.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: Thu, 2 May 2019 19:27:01 +0200	[thread overview]
Message-ID: <190e3c4f-7486-f56c-b3e1-7ee07da88395@free.fr> (raw)
In-Reply-To: <3757fc2d-0587-be46-8f75-6d79906be8bd@arm.com>

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@gmail.com>
> Reported-by: Marc Gonzalez <marc.w.gonzalez@free.fr>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> 
> 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@free.fr>

and you can throw in my

Reviewed-by: Marc Gonzalez <marc.w.gonzalez@free.fr>

for good measure ;-)

All that's left now is to submit it to Linus during the merge window :-p

Regards.