linux-kernel - Re: [PATCH v2 14/18] firmware: qcom_scm-32: Create common legacy atomic call

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5dd4688d.1c69fb81.77385.33e9@mx.google.com>
Date:   Tue, 19 Nov 2019 14:11:24 -0800
From:   Stephen Boyd <swboyd@...omium.org>
To:     Elliot Berman <eberman@...eaurora.org>, agross@...nel.org,
        bjorn.andersson@...aro.org, saiprakash.ranjan@...eaurora.org
Cc:     Elliot Berman <eberman@...eaurora.org>, tsoni@...eaurora.org,
        sidgup@...eaurora.org, psodagud@...eaurora.org,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 14/18] firmware: qcom_scm-32: Create common legacy atomic call

Quoting Elliot Berman (2019-11-12 13:22:50)
> Per [1], legacy calling convention supports up to 5 arguments and
> 3 return values. Create one function to support this combination.

And remove the other functions in its place?

It would be nice to have some motivation here in the commit text.

> 
> [1]: https://source.codeaurora.org/quic/la/kernel/msm-4.9/tree/drivers/soc/qcom/scm.c?h=kernel.lnx.4.9.r28-rel#n1024
> 
> diff --git a/drivers/firmware/qcom_scm-32.c b/drivers/firmware/qcom_scm-32.c
> index 913a77c..eca18e1 100644
> --- a/drivers/firmware/qcom_scm-32.c
> +++ b/drivers/firmware/qcom_scm-32.c
> @@ -252,6 +252,8 @@ static int qcom_scm_call(struct device *dev, struct qcom_scm_desc *desc)
>         return ret;
>  }
>  
> +#define LEGACY_ATOMIC_N_REG_ARGS       5
> +#define LEGACY_ATOMIC_FIRST_REG_IDX    2
>  #define LEGACY_CLASS_REGISTER          (0x2 << 8)
>  #define LEGACY_MASK_IRQS               BIT(5)
>  #define LEGACY_ATOMIC_ID(svc, cmd, n) \
> @@ -261,52 +263,34 @@ static int qcom_scm_call(struct device *dev, struct qcom_scm_desc *desc)
>                                 (n & 0xf))
>  
>  /**
> - * qcom_scm_call_atomic1() - Send an atomic SCM command with one argument
> - * @svc_id: service identifier
> - * @cmd_id: command identifier
> - * @arg1: first argument
> + * qcom_scm_call_atomic() - Send an atomic SCM command with up to 5 arguments
> + * and 3 return values
>   *

Please document arguments.

>   * This shall only be used with commands that are guaranteed to be
>   * uninterruptable, atomic and SMP safe.
>   */
> -static s32 qcom_scm_call_atomic1(u32 svc, u32 cmd, u32 arg1)
> +static int qcom_scm_call_atomic(struct qcom_scm_desc *desc)

Can desc be const?

>  {
>         int context_id;
>         struct arm_smccc_args smc = {0};
>         struct arm_smccc_res res;
> +       size_t i, arglen = desc->arginfo & 0xf;
>  
> -       smc.a[0] = LEGACY_ATOMIC_ID(svc, cmd, 1);
> -       smc.a[1] = (unsigned long)&context_id;
> -       smc.a[2] = arg1;
> -       arm_smccc_smc(smc.a[0], smc.a[1], smc.a[2], smc.a[3],
> -                     smc.a[4], smc.a[5], smc.a[6], smc.a[7], &res);
> +       BUG_ON(arglen > LEGACY_ATOMIC_N_REG_ARGS);
>  
> -       return res.a0;
> -}
> +       smc.a[0] = LEGACY_ATOMIC_ID(desc->svc, desc->cmd, arglen);
> +       smc.a[1] = (unsigned long)&context_id;
>  
[...]
>  int __qcom_scm_io_writel(struct device *dev, phys_addr_t addr, unsigned int val)
>  {
> -       return qcom_scm_call_atomic2(QCOM_SCM_SVC_IO, QCOM_SCM_IO_WRITE,
> -                                    addr, val);
> +       struct qcom_scm_desc desc = {
> +               .svc = QCOM_SCM_SVC_IO,
> +               .cmd = QCOM_SCM_IO_WRITE,
> +               .owner = ARM_SMCCC_OWNER_SIP,
> +       };
> +
> +       desc.args[0] = addr;
> +       desc.args[1] = val;
> +       desc.arginfo = QCOM_SCM_ARGS(2);
> +
> +       return qcom_scm_call_atomic(&desc);

So what is the benefit of this conversion? Now callers have to construct
a descriptor on the stack and call the function that would otherwise
accept some number of arguments. Are we going to be adding more register
based APIs? It would seem simpler to just have a similar interface that
smccc has that takes some fixed number of registers and then suffer the
few extra register moves of some random value like 0 when they're not
used by the secure world.