Re: [PATCH V2] firmware: qcom_scm: use the SCM_CONVENTION based on ARM / ARM64

[Elliot Berman]

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.

> 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