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

[Kathiravan Thirumoorthy]

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