On 9/25/2023 8:23 AM, Elliot Berman wrote:
On 9/19/2023 8:29 AM, Bjorn Andersson wrote:
On Fri, Sep 15, 2023 at 10:10:32PM +0300, Dmitry Baryshkov wrote:
On Fri, 15 Sept 2023 at 18:17, Bjorn Andersson <andersson@xxxxxxxxxx> wrote:
On Wed, Jun 07, 2023 at 10:23:45AM +0530, Kathiravan T wrote:
During SCM probe, to identify the SCM convention, scm call is made with
SMC_CONVENTION_ARM_64 followed by SMC_CONVENTION_ARM_32. Based on the
result what convention to be used is decided.
IPQ chipsets starting from IPQ807x, supports both 32bit and 64bit kernel
variants, however TZ firmware runs in 64bit mode. When running on 32bit
kernel, scm call is made with SMC_CONVENTION_ARM_64 is causing the
system crash, due to the difference in the register sets between ARM and
AARCH64, which is accessed by the TZ.
To avoid this, use SMC_CONVENTION_ARM_64 only on ARM64 builds.
My memory of this is cloudy, but I feel the logic is complicated because
early 64-bit boards all used 32-bit TZ. So, I really would like Elliot's
input before picking this change.
But this codepath is not changed by this patch. Only the 32-bit
codepath is altered.
Ohh, you're right, sorry about that. Would still be nice to see some
feedback from the team here...
The commit message is talking about the convention check crashing the
system, the only part of the convention checker that seems to matter to
me is the "calling convention" bit in the smc call.
Per the "SMC calling convention specification", the 64-bit calling
convention bit can only be used when the client is 64-bit. So perhaps
this is the actual problem?
Beyond that, another practical problem I can see is if we pass more than
4 arguments to a call the layout of the extra arguments will not match
between the two worlds (as Linux will pass an array of unsigned long).
With this in mind, I'd like the commit message to be more specific.
Afaict, this is not an issue with the convention detection, but rather
the invalid to call __scm_smc_call() with 64-bit convention on a 32-bit
system. Working around this by having an undocumented #if ARM64 in
another part of the driver isn't clear enough, IMHO.
Moving the check to __scm_smc_call(), or at least documenting the
behavior there (and next to the #if) seems reasonable.
In terms of disallowing 64-bit convention to be probed on a 32-bit kernel:
Reviewed-By: Elliot Berman <quic_eberman@xxxxxxxxxxx>
I first thought moving the check to __scm_smc_call() would be better but
then I realized we would be adding an extra runtime check for each SCM call
that either always passes or always fails. I think the current #if is best
as-is, although it would be good to add some comments explaining why as
Bjorn mentioned.
Thanks everyone for taking time to review the patch! Will add a comment
above the #if block and post the next version.
Regards,
Bjorn
Regards,
Bjorn
Cc: stable@xxxxxxxxxxxxxxx
Fixes: 9a434cee773a ("firmware: qcom_scm: Dynamically support SMCCC and legacy conventions")
Signed-off-by: Kathiravan T <quic_kathirav@xxxxxxxxxxx>
---
Changes in V2:
- Added the Fixes tag and cc'd stable mailing list
drivers/firmware/qcom_scm.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index fde33acd46b7..db6754db48a0 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -171,6 +171,7 @@ static enum qcom_scm_convention __get_convention(void)
if (likely(qcom_scm_convention != SMC_CONVENTION_UNKNOWN))
return qcom_scm_convention;
+#if IS_ENABLED(CONFIG_ARM64)
/*
* Device isn't required as there is only one argument - no device
* needed to dma_map_single to secure world
@@ -191,6 +192,7 @@ static enum qcom_scm_convention __get_convention(void)
forced = true;
goto found;
}
+#endif
probed_convention = SMC_CONVENTION_ARM_32;
ret = __scm_smc_call(NULL, &desc, probed_convention, &res, true);
--
2.17.1
--
With best wishes
Dmitry
