diff mbox series

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

Message ID 20230607045345.25049-1-quic_kathirav@quicinc.com
State New
Headers show
Series [V2] firmware: qcom_scm: use the SCM_CONVENTION based on ARM / ARM64 | expand

Commit Message

Kathiravan Thirumoorthy June 7, 2023, 4:53 a.m. UTC
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(+)

Comments

Kathiravan Thirumoorthy June 20, 2023, 6:13 a.m. UTC | #1
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);
Kathiravan Thirumoorthy July 14, 2023, 1:58 p.m. UTC | #2
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);
Kathiravan Thirumoorthy Aug. 3, 2023, 9:16 a.m. UTC | #3
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);
Bjorn Andersson Sept. 15, 2023, 3:21 p.m. UTC | #4
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
>
Dmitry Baryshkov Sept. 15, 2023, 7:10 p.m. UTC | #5
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
> >
Bjorn Andersson Sept. 19, 2023, 3:29 p.m. UTC | #6
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
Elliot Berman Sept. 25, 2023, 2:53 a.m. UTC | #7
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
Kathiravan Thirumoorthy Sept. 25, 2023, 4:02 a.m. UTC | #8
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
diff mbox series

Patch

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);