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

[Weizhao Ouyang]

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.

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