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

[Bjorn Andersson]

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.

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