Re: [PATCH] arm64: dts: qcom: sc7180: Mark SCM as dma-coherent for trogdor

From: Stephan Gerhold <stephan@gerhold.net>
To: Konrad Dybcio <konrad.dybcio@linaro.org>
Cc: Douglas Anderson <dianders@chromium.org>,
	andersson@kernel.org,
	Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
	konrad.dybcio@somainline.org, sumit.semwal@linaro.org,
	Will Deacon <will@kernel.org>,
	amit.pundir@linaro.org, Sibi Sankar <quic_sibis@quicinc.com>,
	linux-arm-msm@vger.kernel.org, Stephen Boyd <swboyd@chromium.org>,
	linux-arm-kernel@lists.infradead.org,
	Andy Gross <agross@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Rob Clark <robdclark@chromium.org>,
	Rob Herring <robh+dt@kernel.org>,
	cros-qcom-dts-watchers@chromium.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, Nikita Travkin <nikita@trvn.ru>
Subject: Re: [PATCH] arm64: dts: qcom: sc7180: Mark SCM as dma-coherent for trogdor
Date: Fri, 16 Jun 2023 16:54:03 +0200	[thread overview]
Message-ID: <ZIx3i3qmv-aM-RdB@gerhold.net> (raw)
In-Reply-To: <a5605da0-5b6a-cfeb-f095-bbf963551c08@linaro.org>

On Fri, Jun 16, 2023 at 01:20:09PM +0200, Konrad Dybcio wrote:
> On 16.06.2023 09:10, Stephan Gerhold wrote:
> > On Thu, Jun 15, 2023 at 02:52:54PM -0700, Douglas Anderson wrote:
> >> Trogdor devices use firmware backed by TF-A instead of Qualcomm's
> >> normal TZ. On TF-A we end up mapping memory as cachable. Specifically,
> >> you can see in Trogdor's TF-A code [1] in qti_sip_mem_assign() that we
> >> call qti_mmap_add_dynamic_region() with MT_RO_DATA. This translates
> >> down to MT_MEMORY instead of MT_NON_CACHEABLE or MT_DEVICE.
> >>
> >> **Apparently Qualcomm's normal TZ implementation maps the memory as
> >> non-cachable.**
> > 
> > Are you sure about this? From the discussion in the chat the conclusion
> > was that we can check easily for TF-A, but we have absolutely no idea
> > what Qualcomm's firmware implementation does. It might be "broken" the
> > same way and we just have not noticed it yet.
> Nikita (+CC) was able to boot Venus (which uses that memory reservation
> call, I believe) on next-20230615 with a WP firmware, so it should be okay..

Unfortunately we cannot draw any conclusions from a working case. Doug
mentioned this happens only with CONFIG_INIT_ON_ALLOC_DEFAULT_ON=y [1].
But even with that in theory there could be side effects that invalidate
or evict the cache line, making it look like it's described correctly
but it will just explode at some later point.

It's more reliable to draw conclusions from a failing case: I asked
Nikita to test with "dma-coherent" added to SCM on the WoA firmware.
This fails with similar errors Doug had without the property ("Assign
memory protection call failed -22"). Clearly the firmware did not read
the proper values were just written into the cache. So it's indeed
reasonable to assume that Qcom's implementation maps the memory as
non-cacheable.

Feel free to disregard my original comment then and keep the sentence. :)

Thanks,
Stephan

[1]: https://lore.kernel.org/linux-arm-msm/20230614165904.1.I279773c37e2c1ed8fbb622ca6d1397aea0023526@changeid/

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel