Re: [PATCH v6] firmware: qcom: scm: Add support for ARM64 SoCs

[Rob Clark]

On Fri, May 29, 2015 at 5:34 AM, Mark Rutland <mark.rutland@xxxxxxx> wrote:
> Hi,
>
> On Thu, May 28, 2015 at 05:11:20PM +0100, Kumar Gala wrote:
>> Add an implementation of the SCM interface that works on ARM64 SoCs.  This
>> is used by things like determine if we have HDCP support or not on the
>> system.
>
> Which drivers will be calling this code?

jfyi, for the HDCP part, that would be used by drm/msm..

BR,
-R

>>
>> Signed-off-by: Kumar Gala <galak@xxxxxxxxxxxxxx>
>> ---
>> * v6:
>> - Added comment about HDCP usage
>> - Folded in HDCP SCM call
>> - implement boot interfaces as -EINVAL
>>
>> * v5:
>> - use common error defines from qcom_scm.h
>> - removed R*_STR defines
>>
>> * v4:
>> - Folded in change to qcom_scm_cpu_power_down to remove HOTPLUG flag
>>   from Lina.
>>
>>  arch/arm64/Kconfig             |   1 +
>>  drivers/firmware/Makefile      |   4 +
>>  drivers/firmware/qcom_scm-64.c | 406 +++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 411 insertions(+)
>>  create mode 100644 drivers/firmware/qcom_scm-64.c
>>
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index 4269dba..8878800 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -190,6 +190,7 @@ config ARCH_MEDIATEK
>>  config ARCH_QCOM
>>         bool "Qualcomm Platforms"
>>         select PINCTRL
>> +       select QCOM_SCM
>>         help
>>           This enables support for the ARMv8 based Qualcomm chipsets.
>>
>> diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
>> index 3001f1a..c79751a 100644
>> --- a/drivers/firmware/Makefile
>> +++ b/drivers/firmware/Makefile
>> @@ -12,7 +12,11 @@ obj-$(CONFIG_ISCSI_IBFT_FIND)        += iscsi_ibft_find.o
>>  obj-$(CONFIG_ISCSI_IBFT)       += iscsi_ibft.o
>>  obj-$(CONFIG_FIRMWARE_MEMMAP)  += memmap.o
>>  obj-$(CONFIG_QCOM_SCM)         += qcom_scm.o
>> +ifdef CONFIG_64BIT
>> +obj-$(CONFIG_QCOM_SCM)         += qcom_scm-64.o
>> +else
>>  obj-$(CONFIG_QCOM_SCM)         += qcom_scm-32.o
>> +endif
>>  CFLAGS_qcom_scm-32.o :=$(call as-instr,.arch_extension sec,-DREQUIRES_SEC=1)
>>
>>  obj-$(CONFIG_GOOGLE_FIRMWARE)  += google/
>> diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
>> new file mode 100644
>> index 0000000..90e0fa3
>> --- /dev/null
>> +++ b/drivers/firmware/qcom_scm-64.c
>> @@ -0,0 +1,406 @@
>> +/* Copyright (c) 2014-2015, The Linux Foundation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
>> + * 02110-1301, USA.
>> + */
>> +
>> +#include <linux/cpumask.h>
>> +#include <linux/delay.h>
>> +#include <linux/mutex.h>
>> +#include <linux/slab.h>
>> +#include <linux/types.h>
>> +#include <linux/qcom_scm.h>
>> +
>> +#include <asm/cacheflush.h>
>> +#include <asm/compiler.h>
>> +#include <asm/smp_plat.h>
>> +
>> +#include "qcom_scm.h"
>> +
>> +#define QCOM_SCM_SIP_FNID(s, c) (((((s) & 0xFF) << 8) | ((c) & 0xFF)) | 0x02000000)
>> +
>> +#define MAX_QCOM_SCM_ARGS 10
>> +#define MAX_QCOM_SCM_RETS 3
>> +
>> +#define QCOM_SCM_ARGS_IMPL(num, a, b, c, d, e, f, g, h, i, j, ...) (\
>> +                       (((a) & 0xff) << 4) | \
>> +                       (((b) & 0xff) << 6) | \
>> +                       (((c) & 0xff) << 8) | \
>> +                       (((d) & 0xff) << 10) | \
>> +                       (((e) & 0xff) << 12) | \
>> +                       (((f) & 0xff) << 14) | \
>> +                       (((g) & 0xff) << 16) | \
>> +                       (((h) & 0xff) << 18) | \
>> +                       (((i) & 0xff) << 20) | \
>> +                       (((j) & 0xff) << 22) | \
>> +                       (num & 0xffff))
>> +
>> +#define QCOM_SCM_ARGS(...) QCOM_SCM_ARGS_IMPL(__VA_ARGS__, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0)
>> +
>> +/**
>> + * struct qcom_scm_desc
>> + * @arginfo: Metadata describing the arguments in args[]
>> + * @args: The array of arguments for the secure syscall
>> + * @ret: The values returned by the secure syscall
>> + * @extra_arg_buf: The buffer containing extra arguments
>> +                  (that don't fit in available registers)
>> + * @x5: The 4rd argument to the secure syscall or physical address of
>> +       extra_arg_buf
>
> Nit: 4th
>
> Why not just fold x5 into the args array? It's still an argument either
> way...
>
>> + */
>> +struct qcom_scm_desc {
>> +       u32 arginfo;
>> +       u64 args[MAX_QCOM_SCM_ARGS];
>> +       u64 ret[MAX_QCOM_SCM_RETS];
>> +
>> +       /* private */
>> +       void *extra_arg_buf;
>
> Shouldn't this be a qcom_scm_extra_arg* ?
>
>> +       u64 x5;
>> +};
>> +
>> +
>> +#define QCOM_SCM_EBUSY         -55
>> +#define QCOM_SCM_V2_EBUSY      -12
>> +
>> +static DEFINE_MUTEX(qcom_scm_lock);
>> +
>> +#define QCOM_SCM_EBUSY_WAIT_MS 30
>> +#define QCOM_SCM_EBUSY_MAX_RETRY 20
>> +
>> +#define N_EXT_QCOM_SCM_ARGS 7
>> +#define FIRST_EXT_ARG_IDX 3
>> +#define SMC_ATOMIC_SYSCALL 31
>
> Unused define?
>
>> +#define N_REGISTER_ARGS (MAX_QCOM_SCM_ARGS - N_EXT_QCOM_SCM_ARGS + 1)
>> +#define SMC64_MASK 0x40000000
>> +#define SMC_ATOMIC_MASK 0x80000000
>
> These look like SMC Calling Conventions values? I think we need an
> arm_smccc.h header so this can be shared with other users (e.g. the
> generic TEE client code).
>
>> +
>> +static int qcom_scm_remap_error(int err)
>> +{
>> +       switch (err) {
>> +       case QCOM_SCM_ERROR:
>> +               return -EIO;
>> +       case QCOM_SCM_EINVAL_ADDR:
>> +       case QCOM_SCM_EINVAL_ARG:
>> +               return -EINVAL;
>> +       case QCOM_SCM_EOPNOTSUPP:
>> +               return -EOPNOTSUPP;
>> +       case QCOM_SCM_ENOMEM:
>> +               return -ENOMEM;
>> +       case QCOM_SCM_EBUSY:
>> +               return QCOM_SCM_EBUSY;
>> +       case QCOM_SCM_V2_EBUSY:
>> +               return QCOM_SCM_V2_EBUSY;
>> +       }
>> +       return -EINVAL;
>> +}
>> +
>> +int __qcom_scm_call_armv8_64(u64 x0, u64 x1, u64 x2, u64 x3, u64 x4, u64 x5,
>> +                               u64 *ret1, u64 *ret2, u64 *ret3)
>> +{
>> +       register u64 r0 asm("r0") = x0;
>> +       register u64 r1 asm("r1") = x1;
>> +       register u64 r2 asm("r2") = x2;
>> +       register u64 r3 asm("r3") = x3;
>> +       register u64 r4 asm("r4") = x4;
>> +       register u64 r5 asm("r5") = x5;
>> +
>> +       do {
>> +               asm volatile(
>> +                       __asmeq("%0", "x0")
>> +                       __asmeq("%1", "x1")
>> +                       __asmeq("%2", "x2")
>> +                       __asmeq("%3", "x3")
>> +                       __asmeq("%4", "x0")
>> +                       __asmeq("%5", "x1")
>> +                       __asmeq("%6", "x2")
>> +                       __asmeq("%7", "x3")
>> +                       __asmeq("%8", "x4")
>> +                       __asmeq("%9", "x5")
>> +#ifdef REQUIRES_SEC
>> +                       ".arch_extension sec\n"
>> +#endif
>
> In the Makefile REQUIRES_SEC only seems to get defined for the 32-bit
> scm client code. Is this necessary? We didn't seem to need it for
> psci-call.S.
>
> Likewise in __qcom_scm_call_armv8_32.
>
>> +                       "smc    #0\n"
>> +                       : "=r" (r0), "=r" (r1), "=r" (r2), "=r" (r3)
>> +                       : "r" (r0), "r" (r1), "r" (r2), "r" (r3), "r" (r4),
>> +                         "r" (r5)
>> +                       : "x6", "x7", "x8", "x9", "x10", "x11", "x12", "x13",
>> +                         "x14", "x15", "x16", "x17");
>
> I wonder if this asm will break similarly to the way the PSCI invocation
> code did (see f5e0a12ca2d939e4).
>
>> +       } while (r0 == QCOM_SCM_INTERRUPTED);
>
> This looks like it could livelock rather easily. Is this called in
> non-preemptible context?
>
>> +
>> +       if (ret1)
>> +               *ret1 = r1;
>> +       if (ret2)
>> +               *ret2 = r2;
>> +       if (ret3)
>> +               *ret3 = r3;
>> +
>> +       return r0;
>> +}
>> +
>> +int __qcom_scm_call_armv8_32(u32 w0, u32 w1, u32 w2, u32 w3, u32 w4, u32 w5,
>> +                               u64 *ret1, u64 *ret2, u64 *ret3)
>> +{
>> +       register u32 r0 asm("r0") = w0;
>> +       register u32 r1 asm("r1") = w1;
>> +       register u32 r2 asm("r2") = w2;
>> +       register u32 r3 asm("r3") = w3;
>> +       register u32 r4 asm("r4") = w4;
>> +       register u32 r5 asm("r5") = w5;
>> +
>> +       do {
>> +               asm volatile(
>> +                       __asmeq("%0", "x0")
>> +                       __asmeq("%1", "x1")
>> +                       __asmeq("%2", "x2")
>> +                       __asmeq("%3", "x3")
>> +                       __asmeq("%4", "x0")
>> +                       __asmeq("%5", "x1")
>> +                       __asmeq("%6", "x2")
>> +                       __asmeq("%7", "x3")
>> +                       __asmeq("%8", "x4")
>> +                       __asmeq("%9", "x5")
>> +#ifdef REQUIRES_SEC
>> +                       ".arch_extension sec\n"
>> +#endif
>> +                       "smc    #0\n"
>> +                       : "=r" (r0), "=r" (r1), "=r" (r2), "=r" (r3)
>> +                       : "r" (r0), "r" (r1), "r" (r2), "r" (r3), "r" (r4),
>> +                         "r" (r5)
>> +                       : "x6", "x7", "x8", "x9", "x10", "x11", "x12", "x13",
>> +                       "x14", "x15", "x16", "x17");
>> +
>> +       } while (r0 == QCOM_SCM_INTERRUPTED);
>> +
>> +       if (ret1)
>> +               *ret1 = r1;
>> +       if (ret2)
>> +               *ret2 = r2;
>> +       if (ret3)
>> +               *ret3 = r3;
>> +
>> +       return r0;
>> +}
>
> Why not just always use the smc64 asm and copy the results per required
> register width?
>
>> +
>> +struct qcom_scm_extra_arg {
>> +       union {
>> +               u32 args32[N_EXT_QCOM_SCM_ARGS];
>> +               u64 args64[N_EXT_QCOM_SCM_ARGS];
>> +       };
>> +};
>> +
>> +static enum qcom_scm_interface_version {
>> +       QCOM_SCM_UNKNOWN,
>> +       QCOM_SCM_LEGACY,
>> +       QCOM_SCM_ARMV8_32,
>> +       QCOM_SCM_ARMV8_64,
>> +} qcom_scm_version = QCOM_SCM_UNKNOWN;
>> +
>> +/* This will be set to specify SMC32 or SMC64 */
>> +static u32 qcom_scm_version_mask;
>> +
>> +/*
>> + * If there are more than N_REGISTER_ARGS, allocate a buffer and place
>> + * the additional arguments in it. The extra argument buffer will be
>> + * pointed to by X5.
>> + */
>> +static int allocate_extra_arg_buffer(struct qcom_scm_desc *desc, gfp_t flags)
>
> Is there any need for this to take the flags parameter? There's only one
> caller, which always passes GFP_KERNEL.
>
>> +{
>> +       int i, j;
>> +       struct qcom_scm_extra_arg *argbuf;
>> +       int arglen = desc->arginfo & 0xf;
>> +       size_t argbuflen = PAGE_ALIGN(sizeof(struct qcom_scm_extra_arg));
>
> PAGE_ALIGN(sizeof(*argbuf)) ?
>
>> +
>> +       desc->x5 = desc->args[FIRST_EXT_ARG_IDX];
>> +
>> +       if (likely(arglen <= N_REGISTER_ARGS)) {
>> +               desc->extra_arg_buf = NULL;
>> +               return 0;
>> +       }
>> +
>> +       argbuf = kzalloc(argbuflen, flags);
>
> get_zeroed_page(flags) ?
>
> You could do BUILD_BUG_ON(sizeof(*argbuf) > PAGE_SIZE) if you ever
> expect argbuf to grow.
>
>> +       if (!argbuf) {
>> +               pr_err("qcom_scm_call: failed to alloc mem for extended argument buffer\n");
>> +               return -ENOMEM;
>> +       }
>> +
>> +       desc->extra_arg_buf = argbuf;
>> +
>> +       j = FIRST_EXT_ARG_IDX;
>> +       if (qcom_scm_version == QCOM_SCM_ARMV8_64)
>> +               for (i = 0; i < N_EXT_QCOM_SCM_ARGS; i++)
>> +                       argbuf->args64[i] = desc->args[j++];
>> +       else
>> +               for (i = 0; i < N_EXT_QCOM_SCM_ARGS; i++)
>> +                       argbuf->args32[i] = desc->args[j++];
>> +       desc->x5 = virt_to_phys(argbuf);
>> +       __flush_dcache_area(argbuf, argbuflen);
>> +
>> +       return 0;
>> +}
>> +
>> +/**
>> + * qcom_scm_call() - Invoke a syscall in the secure world
>> + * @svc_id: service identifier
>> + * @cmd_id: command identifier
>> + * @fn_id: The function ID for this syscall
>> + * @desc: Descriptor structure containing arguments and return values
>> + *
>> + * Sends a command to the SCM and waits for the command to finish processing.
>> + * This should *only* be called in pre-emptible context.
>> + *
>> + * A note on cache maintenance:
>> + * Note that any buffers that are expected to be accessed by the secure world
>> + * must be flushed before invoking qcom_scm_call and invalidated in the cache
>> + * immediately after qcom_scm_call returns.
>
> Please use the architecturally-defined terms; "flush" can mean several
> distinct things w.r.t. cache maintenance. So s/flushed/cleaned/, please.
>
> As far as I can see you only need to ensure data is visible to the
> secure world (presumably it's using a non-cacheable mapping?), for which
> cleaning is sufficient.
>
>> An important point that must be noted
>> + * is that on ARMV8 architectures, invalidation actually also causes a dirty
>> + * cache line to be cleaned (flushed + unset-dirty-bit). Therefore it is of
>> + * paramount importance that the buffer be flushed before invoking qcom_scm_call,
>> + * even if you don't care about the contents of that buffer.
>
> This is misleading.
>
> Your problem isn't that an invalidate implies a clean, but rather that a
> dirty cacheline may be evicted naturally at any time. Before calling the
> secure world you need to ensure that buffers the secure world will use
> are clean or invalid in the caches to avoid races with natural eviction.
>
> Per the architecture, invalidating a cache line does not guarantee that
> said cache line will be written back. Some CPUs may be convinced to
> upgrade invalidate to clean+invalidate by some IMPLEMENTATION DEFINED
> mechanism, but that's not an architectual guarantee.
>
>> + *
>> + * Note that cache maintenance on the argument buffer (desc->args) is taken care
>> + * of by qcom_scm_call; however, callers are responsible for any other cached
>> + * buffers passed over to the secure world.
>> +*/
>> +static int qcom_scm_call(u32 svc_id, u32 cmd_id, struct qcom_scm_desc *desc)
>> +{
>> +       int arglen = desc->arginfo & 0xf;
>> +       int ret, retry_count = 0;
>> +       u32 fn_id = QCOM_SCM_SIP_FNID(svc_id, cmd_id);
>> +       u64 x0;
>> +
>> +       ret = allocate_extra_arg_buffer(desc, GFP_KERNEL);
>> +       if (ret)
>> +               return ret;
>> +
>> +       x0 = fn_id | qcom_scm_version_mask;
>> +
>> +       do {
>> +               mutex_lock(&qcom_scm_lock);
>> +
>> +               desc->ret[0] = desc->ret[1] = desc->ret[2] = 0;
>> +
>> +               pr_debug("qcom_scm_call: func id %#llx, args: %#x, %#llx, %#llx, %#llx, %#llx\n",
>> +                       x0, desc->arginfo, desc->args[0], desc->args[1],
>> +                       desc->args[2], desc->x5);
>> +
>> +               if (qcom_scm_version == QCOM_SCM_ARMV8_64)
>> +                       ret = __qcom_scm_call_armv8_64(x0, desc->arginfo,
>> +                                                 desc->args[0], desc->args[1],
>> +                                                 desc->args[2], desc->x5,
>> +                                                 &desc->ret[0], &desc->ret[1],
>> +                                                 &desc->ret[2]);
>> +               else
>> +                       ret = __qcom_scm_call_armv8_32(x0, desc->arginfo,
>> +                                                 desc->args[0], desc->args[1],
>> +                                                 desc->args[2], desc->x5,
>> +                                                 &desc->ret[0], &desc->ret[1],
>> +                                                 &desc->ret[2]);
>> +               mutex_unlock(&qcom_scm_lock);
>> +
>> +               if (ret == QCOM_SCM_V2_EBUSY)
>> +                       msleep(QCOM_SCM_EBUSY_WAIT_MS);
>> +       }  while (ret == QCOM_SCM_V2_EBUSY && (retry_count++ < QCOM_SCM_EBUSY_MAX_RETRY));
>> +
>> +       if (ret < 0)
>> +               pr_err("qcom_scm_call failed: func id %#llx, arginfo: %#x, args: %#llx, %#llx, %#llx, %#llx, ret: %d, syscall returns: %#llx, %#llx, %#llx\n",
>> +                       x0, desc->arginfo, desc->args[0], desc->args[1],
>> +                       desc->args[2], desc->x5, ret, desc->ret[0],
>> +                       desc->ret[1], desc->ret[2]);
>> +
>> +       if (arglen > N_REGISTER_ARGS)
>> +               kfree(desc->extra_arg_buf);
>> +       if (ret < 0)
>> +               return qcom_scm_remap_error(ret);
>> +       return 0;
>> +}
>> +
>> +int __qcom_scm_set_cold_boot_addr(void *entry, const cpumask_t *cpus)
>> +{
>> +       return -EINVAL;
>> +}
>> +
>> +int __qcom_scm_set_warm_boot_addr(void *entry, const cpumask_t *cpus)
>> +{
>> +       return -EINVAL;
>> +}
>> +
>> +void __qcom_scm_cpu_power_down(u32 flags)
>> +{
>> +       return;
>> +}
>> +
>> +int __qcom_scm_is_call_available(u32 svc_id, u32 cmd_id)
>> +{
>> +       int ret;
>> +       struct qcom_scm_desc desc = {0};
>> +
>> +       desc.arginfo = QCOM_SCM_ARGS(1);
>> +       desc.args[0] = QCOM_SCM_SIP_FNID(svc_id, cmd_id);
>> +
>> +       ret = qcom_scm_call(QCOM_SCM_SVC_INFO, QCOM_IS_CALL_AVAIL_CMD, &desc);
>> +
>> +       if (ret)
>> +               return ret;
>> +
>> +       return desc.ret[0];
>> +}
>> +
>> +int __qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt, u32 *resp)
>> +{
>> +       int ret, i, j;
>> +       struct qcom_scm_desc desc = {0};
>> +
>> +       if (req_cnt > QCOM_SCM_HDCP_MAX_REQ_CNT)
>> +               return -ERANGE;
>> +
>> +       for (i = 0, j = 0; i < req_cnt; i++) {
>> +               desc.args[j++] = req[i].addr;
>> +               desc.args[j++] = req[i].val;
>> +       }
>> +       desc.arginfo = QCOM_SCM_ARGS(j);
>> +
>> +       ret = qcom_scm_call(QCOM_SCM_SVC_HDCP, QCOM_SCM_CMD_HDCP, &desc);
>> +       *resp = desc.ret[0];
>> +
>> +       return ret;
>> +}
>> +
>> +static int __init qcom_scm_init(void)
>> +{
>> +       int ret;
>> +       u64 ret1 = 0, x0;
>> +
>> +       /* First try a SMC64 call */
>> +       qcom_scm_version = QCOM_SCM_ARMV8_64;
>> +       x0 = QCOM_SCM_SIP_FNID(QCOM_SCM_SVC_INFO, QCOM_IS_CALL_AVAIL_CMD);
>> +       x0 |= SMC_ATOMIC_MASK;
>> +       ret = __qcom_scm_call_armv8_64(x0 | SMC64_MASK, QCOM_SCM_ARGS(1), x0, 0, 0, 0,
>> +                                 &ret1, NULL, NULL);
>> +       if (ret || !ret1) {
>> +               /* Try SMC32 call */
>
> This looks fragile. Why the fallback?
>
> Surely you should know which interface to call?
>
>> +               ret1 = 0;
>> +               ret = __qcom_scm_call_armv8_32(x0, QCOM_SCM_ARGS(1), x0, 0, 0,
>> +                                               0, &ret1, NULL, NULL);
>> +               if (ret || !ret1)
>> +                       qcom_scm_version = QCOM_SCM_LEGACY;
>> +               else
>> +                       qcom_scm_version = QCOM_SCM_ARMV8_32;
>> +       } else
>> +               qcom_scm_version_mask = SMC64_MASK;
>> +
>> +       pr_debug("qcom_scm_call: qcom_scm version is %x, mask is %x\n",
>> +                qcom_scm_version, qcom_scm_version_mask);
>> +
>> +       return 0;
>> +}
>> +early_initcall(qcom_scm_init);
>
> So we'll just blindly issue SMCs on every platform?
>
> You'll need a DT binding to tell the kernel when this interface is
> present.
>
> Thanks,
> Mark.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/