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.
Cc: stable@vger.kernel.org Fixes: 9a434cee773a ("firmware: qcom_scm: Dynamically support SMCCC and legacy conventions") Signed-off-by: Kathiravan T quic_kathirav@quicinc.com --- 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);
On 6/7/2023 10:23 AM, 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.
Gentle Reminder...
Cc: stable@vger.kernel.org Fixes: 9a434cee773a ("firmware: qcom_scm: Dynamically support SMCCC and legacy conventions") Signed-off-by: Kathiravan T quic_kathirav@quicinc.com
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);
On 6/20/2023 11:43 AM, Kathiravan T wrote:
On 6/7/2023 10:23 AM, 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.
Gentle Reminder...
Bjorn,
Can you share your thoughts on this patch?
Thanks, Kathiravan T.
Cc: stable@vger.kernel.org Fixes: 9a434cee773a ("firmware: qcom_scm: Dynamically support SMCCC and legacy conventions") Signed-off-by: Kathiravan T quic_kathirav@quicinc.com
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);
On 7/14/2023 7:28 PM, Kathiravan T wrote:
On 6/20/2023 11:43 AM, Kathiravan T wrote:
On 6/7/2023 10:23 AM, 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.
Gentle Reminder...
Bjorn,
Can you share your thoughts on this patch?
Thanks, Kathiravan T.
Gentle Reminder...
Cc: stable@vger.kernel.org Fixes: 9a434cee773a ("firmware: qcom_scm: Dynamically support SMCCC and legacy conventions") Signed-off-by: Kathiravan T quic_kathirav@quicinc.com
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);
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.
Regards, Bjorn
Cc: stable@vger.kernel.org Fixes: 9a434cee773a ("firmware: qcom_scm: Dynamically support SMCCC and legacy conventions") Signed-off-by: Kathiravan T quic_kathirav@quicinc.com
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
On Fri, 15 Sept 2023 at 18:17, Bjorn Andersson andersson@kernel.org 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.
Regards, Bjorn
Cc: stable@vger.kernel.org Fixes: 9a434cee773a ("firmware: qcom_scm: Dynamically support SMCCC and legacy conventions") Signed-off-by: Kathiravan T quic_kathirav@quicinc.com
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
On Fri, Sep 15, 2023 at 10:10:32PM +0300, Dmitry Baryshkov wrote:
On Fri, 15 Sept 2023 at 18:17, Bjorn Andersson andersson@kernel.org 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.
Regards, Bjorn
Regards, Bjorn
Cc: stable@vger.kernel.org Fixes: 9a434cee773a ("firmware: qcom_scm: Dynamically support SMCCC and legacy conventions") Signed-off-by: Kathiravan T quic_kathirav@quicinc.com
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
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@kernel.org 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@quicinc.com
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.
Regards, Bjorn
Regards, Bjorn
Cc: stable@vger.kernel.org Fixes: 9a434cee773a ("firmware: qcom_scm: Dynamically support SMCCC and legacy conventions") Signed-off-by: Kathiravan T quic_kathirav@quicinc.com
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
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@kernel.org 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@quicinc.com
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@vger.kernel.org Fixes: 9a434cee773a ("firmware: qcom_scm: Dynamically support SMCCC and legacy conventions") Signed-off-by: Kathiravan T quic_kathirav@quicinc.com
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
linux-stable-mirror@lists.linaro.org
-
Bjorn Andersson -
Dmitry Baryshkov -
Elliot Berman -
Kathiravan T -
Kathiravan Thirumoorthy