[PATCH v3 1/3] firmware: psci: Fix bind_smccc_features psci check

[Igor Opaniuk]

Hello Weizhao,

On Fri, Feb 2, 2024 at 4:43 AM Weizhao Ouyang <o451686892 at gmail.com> wrote:
>
> Hi Igor,
>
> On Thu, Feb 1, 2024 at 10:36 PM Igor Opaniuk <igor.opaniuk at foundries.io> wrote:
> >
> > Hello Weizhao,
> >
> > On Wed, Jan 31, 2024 at 3:15 PM Weizhao Ouyang <o451686892 at gmail.com> wrote:
> > >
> > > According to PSCI specification DEN0022F, PSCI_FEATURES is used to check
> > > whether the SMCCC is implemented by discovering SMCCC_VERSION.
> > >
> > Could you please add more details to your commit message or as a comment
> > explaining what exact steps should be done for a full discovery sequence of Arm
> > Architecture Service functions, so people don't need to search for
> > that information explicitly?
> >
> > For instance:
> > Step 1: Determine if SMCCC_VERSION is implemented
> > - Use firmware data, device tree PSCI node, or ACPI FADT PSCI flag, to
> > determine whether a
> > PSCI implementation is present.
> > - Use PSCI_VERSION to learn whether PSCI_FEATURES is provided. PSCI_FEATURES is
> > mandatory from version 1.0 of PSCI
> > Step 2. etc.
> >
> > I would just pull that info from the latest SMC Calling Convention version 1.5,
> > from 9 Appendix B: Discovery of Arm Architecture Service functions.
> >
> > Thank you!
> > > Signed-off-by: Weizhao Ouyang <o451686892 at gmail.com>
> > > ---
> > > v3: remove fallback smc call
> > > v2: check SMCCC_ARCH_FEATURES
> > > ---
> > >  drivers/firmware/psci.c   | 5 ++++-
> > >  include/linux/arm-smccc.h | 6 ++++++
> > >  2 files changed, 10 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
> > > index c6b9efab41..03544d76ed 100644
> > > --- a/drivers/firmware/psci.c
> > > +++ b/drivers/firmware/psci.c
> > > @@ -135,10 +135,13 @@ static int bind_smccc_features(struct udevice *dev, int psci_method)
> > >             PSCI_VERSION_MAJOR(psci_0_2_get_version()) == 0)
> > >                 return 0;
> > >
> > > -       if (request_psci_features(ARM_SMCCC_ARCH_FEATURES) ==
> > > +       if (request_psci_features(ARM_SMCCC_VERSION) ==
> > >             PSCI_RET_NOT_SUPPORTED)
> > >                 return 0;
> > >
> > > +       if (invoke_psci_fn(ARM_SMCCC_VERSION, 0, 0, 0) < ARM_SMCCC_VERSION_1_1)
> > > +               return 0;
> > > +
> > IMO, to fully comply with SMC Calling Convention version 1.5
> > we should also check for SMCCC_ARCH_WORKAROUND_1:
> >
> > From 9 Appendix B: Discovery of Arm Architecture Service functions,
> > Step 2: Determine if Arm Architecture Service function is implemented
> > - Use SMCCC_VERSION to learn whether the calling convention complies
> > to version 1.1 or above.
> > - Use SMCCC_ARCH_FEATURES to query whether the Arm Architecture
> > Service function is implemented
> > on this system <--- we lack of this step
>
> Thanks for your review. The 9 Appendix B describes an approach to
> discovery the maximize ability without causing unsafe behavior on
> existing platforms. Regarding the second step, it is just using
> SMCCC_ARCH_WORKAROUND_1 as an example to test SMCCC_ARCH_FEATURES.
>
> For the U-Boot case, we can revisit this from two perspectives:
> 1. SMCCC_ARCH_FEATURES is MANDATORY from SMCCC v1.1.
> 2. SMCCC_VERSION is OPTIONAL for SMCCC v1.0, so we should consider a
> safe behavior.
> 3. U-Boot is using CONFIG_ARM_SMCCC_FEATURES to probe and bind SMCCC
> service driver if this driver is reported as supported.
> 4. U-Boot SMCCC service driver can embed its discovery process in
> is_supported() callback.
>
> So now we can choose the following approach in U-Boot:
> - Use firmware data (check "arm,psci-1.0") to determine whether a PSCI
> implementation is present.
> - Use PSCI_VERSION (psci_0_2_get_version() major version >= 0) to learn
> whether PSCI_FEATURES is provided.
> - Use PSCI_FEATURES with the SMCCC_VERSION Function Identifier as a
> parameter to determine that SMCCC_VERSION is implemented.
> - Use SMCCC_VERSION to learn whether the calling convention complies to
> version 1.1 or above.
> - Trying to probe and bind SMCCC service driver.

Thanks for the detailed explanation!
Reviewed-by: Igor Opaniuk <igor.opaniuk at foundries.io>

>
> BR,
> Weizhao
>
> >
> > >         if (psci_method == PSCI_METHOD_HVC)
> > >                 pdata->invoke_fn = smccc_invoke_hvc;
> > >         else
> > > diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
> > > index f44e9e8f93..da3d29aabe 100644
> > > --- a/include/linux/arm-smccc.h
> > > +++ b/include/linux/arm-smccc.h
> > > @@ -55,8 +55,14 @@
> > >  #define ARM_SMCCC_QUIRK_NONE           0
> > >  #define ARM_SMCCC_QUIRK_QCOM_A6                1 /* Save/restore register a6 */
> > >
> > > +#define ARM_SMCCC_VERSION              0x80000000
> > >  #define ARM_SMCCC_ARCH_FEATURES                0x80000001
> > >
> > > +#define ARM_SMCCC_VERSION_1_0          0x10000
> > > +#define ARM_SMCCC_VERSION_1_1          0x10001
> > > +#define ARM_SMCCC_VERSION_1_2          0x10002
> > > +#define ARM_SMCCC_VERSION_1_3          0x10003
> > > +
> > >  #define ARM_SMCCC_RET_NOT_SUPPORTED    ((unsigned long)-1)
> > >
> > >  #ifndef __ASSEMBLY__
> > > --
> > > 2.39.2
> > >
> >
> >
> > --
> > Best regards - Freundliche Grüsse - Meilleures salutations
> >
> > Igor Opaniuk
> > Senior Software Engineer, Embedded & Security
> > E: igor.opaniuk at foundries.io
> > W: www.foundries.io



-- 
Best regards - Freundliche Grüsse - Meilleures salutations

Igor Opaniuk
Senior Software Engineer, Embedded & Security
E: igor.opaniuk at foundries.io
W: www.foundries.io