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

From: Mark Rutland
Date: Fri May 29 2015 - 05:34:39 EST

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?

>
> 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-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/