| 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
| ||
|
Date: Fri, 15 Nov 2019 15:27:22 -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 01/18] firmware: qcom_scm: Rename macros and structures
Quoting Elliot Berman (2019-11-12 13:22:37)
> Rename legacy-specific structures and macros with legacy prefix; rename
> smccc-specific structures and macros with smccc prefix.
Yes that's what's happening in the patch, but there isn't the most
important part in the commit text here, which is _why_ this change is
meaningful. Presumably the reason is to clearly separate the original
SCM calling convention from the newer SCM calling conventions.
>
> Signed-off-by: Elliot Berman <eberman@...eaurora.org>
> ---
> drivers/firmware/qcom_scm-32.c | 70 ++++++++++++++++++++++--------------------
> drivers/firmware/qcom_scm-64.c | 53 ++++++++++++++++----------------
> 2 files changed, 64 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/firmware/qcom_scm-32.c b/drivers/firmware/qcom_scm-32.c
> index bee8729..5d52641 100644
> --- a/drivers/firmware/qcom_scm-32.c
> +++ b/drivers/firmware/qcom_scm-32.c
> @@ -39,22 +39,22 @@ static struct qcom_scm_entry qcom_scm_wb[] = {
> static DEFINE_MUTEX(qcom_scm_lock);
>
> /**
> - * struct qcom_scm_command - one SCM command buffer
> + * struct qcom_scm_legacy_command - one SCM command buffer
> * @len: total available memory for command and response
> * @buf_offset: start of command buffer
> * @resp_hdr_offset: start of response buffer
> * @id: command to be executed
> - * @buf: buffer returned from qcom_scm_get_command_buffer()
> + * @buf: buffer returned from legacy_get_command_buffer()
I'd prefer to keep qcom_ or at least scm_ somewhere in the name. Just
plain legacy_ is too generic.
> *
> * An SCM command is laid out in memory as follows:
> *
> - * ------------------- <--- struct qcom_scm_command
> + * ------------------- <--- struct qcom_scm_legacy_command
> * | command header |
> - * ------------------- <--- qcom_scm_get_command_buffer()
> + * ------------------- <--- legacy_get_command_buffer()
> * | command buffer |
> - * ------------------- <--- struct qcom_scm_response and
> - * | response header | qcom_scm_command_to_response()
> - * ------------------- <--- qcom_scm_get_response_buffer()
> + * ------------------- <--- struct qcom_scm_legacy_response and
> + * | response header | legacy_command_to_response()
> + * ------------------- <--- legacy_get_response_buffer()
> * | response buffer |
> * -------------------
> *
> @@ -62,7 +62,7 @@ static DEFINE_MUTEX(qcom_scm_lock);
> * you should always use the appropriate qcom_scm_get_*_buffer() routines
Shouldn't this comment be updated too to say "legacy"?
> * to access the buffers in a safe manner.
> */
> -struct qcom_scm_command {
> +struct qcom_scm_legacy_command {
Like here, it's called qcom_scm_legacy_<foo> so maybe that should be how
it works.
> __le32 len;
> __le32 buf_offset;
> __le32 resp_hdr_offset;
> @@ -71,52 +71,55 @@ struct qcom_scm_command {
> };
>
> /**
> - * struct qcom_scm_response - one SCM response buffer
> + * struct qcom_scm_legacy_response - one SCM response buffer
> * @len: total available memory for response
> - * @buf_offset: start of response data relative to start of qcom_scm_response
> + * @buf_offset: start of response data relative to start of
> + * qcom_scm_legacy_response
> * @is_complete: indicates if the command has finished processing
> */
> -struct qcom_scm_response {
> +struct qcom_scm_legacy_response {
> __le32 len;
> __le32 buf_offset;
> __le32 is_complete;
> };
>
> /**
> - * qcom_scm_command_to_response() - Get a pointer to a qcom_scm_response
> + * legacy_command_to_response() - Get a pointer to a qcom_scm_legacy_response
> * @cmd: command
> *
> * Returns a pointer to a response for a command.
> */
> -static inline struct qcom_scm_response *qcom_scm_command_to_response(
> - const struct qcom_scm_command *cmd)
> +static inline struct qcom_scm_legacy_response *legacy_command_to_response(
> + const struct qcom_scm_legacy_command *cmd)
> {
> return (void *)cmd + le32_to_cpu(cmd->resp_hdr_offset);
> }
>
> /**
> - * qcom_scm_get_command_buffer() - Get a pointer to a command buffer
> + * legacy_get_command_buffer() - Get a pointer to a command buffer
> * @cmd: command
> *
> * Returns a pointer to the command buffer of a command.
> */
> -static inline void *qcom_scm_get_command_buffer(const struct qcom_scm_command *cmd)
> +static inline void *legacy_get_command_buffer(
> + const struct qcom_scm_legacy_command *cmd)
> {
> return (void *)cmd->buf;
> }
>
> /**
> - * qcom_scm_get_response_buffer() - Get a pointer to a response buffer
> + * legacy_get_response_buffer() - Get a pointer to a response buffer
> * @rsp: response
> *
> * Returns a pointer to a response buffer of a response.
> */
> -static inline void *qcom_scm_get_response_buffer(const struct qcom_scm_response *rsp)
> +static inline void *legacy_get_response_buffer(
> + const struct qcom_scm_legacy_response *rsp)
> {
> return (void *)rsp + le32_to_cpu(rsp->buf_offset);
> }
>
> -static u32 smc(u32 cmd_addr)
> +static u32 __qcom_scm_call_do(u32 cmd_addr)
> {
> int context_id;
> register u32 r0 asm("r0") = 1;
> @@ -164,8 +167,8 @@ static int qcom_scm_call(struct device *dev, u32 svc_id, u32 cmd_id,
> size_t resp_len)
> {
> int ret;
> - struct qcom_scm_command *cmd;
> - struct qcom_scm_response *rsp;
> + struct qcom_scm_legacy_command *cmd;
> + struct qcom_scm_legacy_response *rsp;
> size_t alloc_len = sizeof(*cmd) + cmd_len + sizeof(*rsp) + resp_len;
> dma_addr_t cmd_phys;
>
> @@ -179,9 +182,9 @@ static int qcom_scm_call(struct device *dev, u32 svc_id, u32 cmd_id,
>
> cmd->id = cpu_to_le32((svc_id << 10) | cmd_id);
> if (cmd_buf)
> - memcpy(qcom_scm_get_command_buffer(cmd), cmd_buf, cmd_len);
> + memcpy(legacy_get_command_buffer(cmd), cmd_buf, cmd_len);
>
> - rsp = qcom_scm_command_to_response(cmd);
> + rsp = legacy_command_to_response(cmd);
>
> cmd_phys = dma_map_single(dev, cmd, alloc_len, DMA_TO_DEVICE);
> if (dma_mapping_error(dev, cmd_phys)) {
> @@ -190,7 +193,7 @@ static int qcom_scm_call(struct device *dev, u32 svc_id, u32 cmd_id,
> }
>
> mutex_lock(&qcom_scm_lock);
> - ret = smc(cmd_phys);
> + ret = __qcom_scm_call_do(cmd_phys);
What is this change about? Is it confusing to have a function called
'smc'? Please mention why this should change in the commit text.
> if (ret < 0)
> ret = qcom_scm_remap_error(ret);
> mutex_unlock(&qcom_scm_lock);
> @@ -206,7 +209,7 @@ static int qcom_scm_call(struct device *dev, u32 svc_id, u32 cmd_id,
> dma_sync_single_for_cpu(dev, cmd_phys + sizeof(*cmd) + cmd_len +
> le32_to_cpu(rsp->buf_offset),
> resp_len, DMA_FROM_DEVICE);
> - memcpy(resp_buf, qcom_scm_get_response_buffer(rsp),
> + memcpy(resp_buf, legacy_get_response_buffer(rsp),
> resp_len);
> }
> out:
> @@ -215,11 +218,12 @@ static int qcom_scm_call(struct device *dev, u32 svc_id, u32 cmd_id,
> return ret;
> }
>
> -#define SCM_CLASS_REGISTER (0x2 << 8)
> -#define SCM_MASK_IRQS BIT(5)
> -#define SCM_ATOMIC(svc, cmd, n) (((((svc) << 10)|((cmd) & 0x3ff)) << 12) | \
> - SCM_CLASS_REGISTER | \
> - SCM_MASK_IRQS | \
> +#define LEGACY_CLASS_REGISTER (0x2 << 8)
> +#define LEGACY_MASK_IRQS BIT(5)
> +#define LEGACY_ATOMIC_ID(svc, cmd, n) \
> + (((((svc) << 10)|((cmd) & 0x3ff)) << 12) | \
> + LEGACY_CLASS_REGISTER | \
> + LEGACY_MASK_IRQS | \
> (n & 0xf))
>
> /**
> @@ -235,7 +239,7 @@ static s32 qcom_scm_call_atomic1(u32 svc, u32 cmd, u32 arg1)
> {
> int context_id;
>
> - register u32 r0 asm("r0") = SCM_ATOMIC(svc, cmd, 1);
> + register u32 r0 asm("r0") = LEGACY_ATOMIC_ID(svc, cmd, 1);
> register u32 r1 asm("r1") = (u32)&context_id;
> register u32 r2 asm("r2") = arg1;
>
> @@ -268,7 +272,7 @@ static s32 qcom_scm_call_atomic2(u32 svc, u32 cmd, u32 arg1, u32 arg2)
> {
> int context_id;
>
> - register u32 r0 asm("r0") = SCM_ATOMIC(svc, cmd, 2);
> + register u32 r0 asm("r0") = LEGACY_ATOMIC_ID(svc, cmd, 2);
> register u32 r1 asm("r1") = (u32)&context_id;
> register u32 r2 asm("r2") = arg1;
> register u32 r3 asm("r3") = arg2;
Is this hunk really necessary? The defines are local to the file.
> diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
> index 7686786..8226b94 100644
> --- a/drivers/firmware/qcom_scm-64.c
> +++ b/drivers/firmware/qcom_scm-64.c
> @@ -14,7 +14,7 @@
>
> #include "qcom_scm.h"
>
> -#define QCOM_SCM_FNID(s, c) ((((s) & 0xFF) << 8) | ((c) & 0xFF))
> +#define SMCCC_FUNCNUM(s, c) ((((s) & 0xFF) << 8) | ((c) & 0xFF))
Is this generic? Maybe it should go into the SMCCC file then if it isn't
qcom specific? Otherwise, please leave QCOM in the name.
>
> #define MAX_QCOM_SCM_ARGS 10
> #define MAX_QCOM_SCM_RETS 3
> @@ -58,11 +58,11 @@ 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 N_REGISTER_ARGS (MAX_QCOM_SCM_ARGS - N_EXT_QCOM_SCM_ARGS + 1)
> +#define SMCCC_N_EXT_ARGS 7
> +#define SMCCC_FIRST_EXT_IDX 3
> +#define SMCCC_N_REG_ARGS (MAX_QCOM_SCM_ARGS - SMCCC_N_EXT_ARGS + 1)
>
> -static void __qcom_scm_call_do(const struct qcom_scm_desc *desc,
> +static void __qcom_scm_call_do_quirk(const struct qcom_scm_desc *desc,
> struct arm_smccc_res *res, u32 fn_id,
> u64 x5, u32 type)
> {
>From here....
> @@ -85,22 +85,23 @@ static void __qcom_scm_call_do(const struct qcom_scm_desc *desc,
> } while (res->a0 == QCOM_SCM_INTERRUPTED);
> }
>
> -static void qcom_scm_call_do(const struct qcom_scm_desc *desc,
> +static void qcom_scm_call_do_smccc(const struct qcom_scm_desc *desc,
> struct arm_smccc_res *res, u32 fn_id,
> u64 x5, bool atomic)
> {
> int retry_count = 0;
>
> if (atomic) {
> - __qcom_scm_call_do(desc, res, fn_id, x5, ARM_SMCCC_FAST_CALL);
> + __qcom_scm_call_do_quirk(desc, res, fn_id, x5,
> + ARM_SMCCC_FAST_CALL);
> return;
> }
>
> do {
> mutex_lock(&qcom_scm_lock);
>
> - __qcom_scm_call_do(desc, res, fn_id, x5,
> - ARM_SMCCC_STD_CALL);
> + __qcom_scm_call_do_quirk(desc, res, fn_id, x5,
> + ARM_SMCCC_STD_CALL);
>
> mutex_unlock(&qcom_scm_lock);
>
> @@ -112,21 +113,21 @@ static void qcom_scm_call_do(const struct qcom_scm_desc *desc,
> } while (res->a0 == QCOM_SCM_V2_EBUSY);
> }
>
> -static int ___qcom_scm_call(struct device *dev, u32 svc_id, u32 cmd_id,
> - const struct qcom_scm_desc *desc,
> - struct arm_smccc_res *res, bool atomic)
> +static int ___qcom_scm_call_smccc(struct device *dev, u32 svc_id, u32 cmd_id,
> + const struct qcom_scm_desc *desc,
> + struct arm_smccc_res *res, bool atomic)
> {
> int arglen = desc->arginfo & 0xf;
> int i;
> - u32 fn_id = QCOM_SCM_FNID(svc_id, cmd_id);
> - u64 x5 = desc->args[FIRST_EXT_ARG_IDX];
> + u32 fn_id = SMCCC_FUNCNUM(svc_id, cmd_id);
> + u64 x5 = desc->args[SMCCC_FIRST_EXT_IDX];
> dma_addr_t args_phys = 0;
> void *args_virt = NULL;
> size_t alloc_len;
> gfp_t flag = atomic ? GFP_ATOMIC : GFP_KERNEL;
>
> - if (unlikely(arglen > N_REGISTER_ARGS)) {
> - alloc_len = N_EXT_QCOM_SCM_ARGS * sizeof(u64);
> + if (unlikely(arglen > SMCCC_N_REG_ARGS)) {
> + alloc_len = SMCCC_N_EXT_ARGS * sizeof(u64);
> args_virt = kzalloc(PAGE_ALIGN(alloc_len), flag);
>
> if (!args_virt)
> @@ -135,15 +136,15 @@ static int ___qcom_scm_call(struct device *dev, u32 svc_id, u32 cmd_id,
> if (qcom_smccc_convention == ARM_SMCCC_SMC_32) {
> __le32 *args = args_virt;
>
> - for (i = 0; i < N_EXT_QCOM_SCM_ARGS; i++)
> + for (i = 0; i < SMCCC_N_EXT_ARGS; i++)
> args[i] = cpu_to_le32(desc->args[i +
> - FIRST_EXT_ARG_IDX]);
> + SMCCC_FIRST_EXT_IDX]);
> } else {
> __le64 *args = args_virt;
>
> - for (i = 0; i < N_EXT_QCOM_SCM_ARGS; i++)
> + for (i = 0; i < SMCCC_N_EXT_ARGS; i++)
> args[i] = cpu_to_le64(desc->args[i +
> - FIRST_EXT_ARG_IDX]);
> + SMCCC_FIRST_EXT_IDX]);
> }
>
> args_phys = dma_map_single(dev, args_virt, alloc_len,
> @@ -157,7 +158,7 @@ static int ___qcom_scm_call(struct device *dev, u32 svc_id, u32 cmd_id,
> x5 = args_phys;
> }
>
> - qcom_scm_call_do(desc, res, fn_id, x5, atomic);
> + qcom_scm_call_do_smccc(desc, res, fn_id, x5, atomic);
>
> if (args_virt) {
> dma_unmap_single(dev, args_phys, alloc_len, DMA_TO_DEVICE);
> @@ -185,7 +186,7 @@ static int qcom_scm_call(struct device *dev, u32 svc_id, u32 cmd_id,
> struct arm_smccc_res *res)
> {
> might_sleep();
> - return ___qcom_scm_call(dev, svc_id, cmd_id, desc, res, false);
> + return ___qcom_scm_call_smccc(dev, svc_id, cmd_id, desc, res, false);
> }
>
> /**
> @@ -203,7 +204,7 @@ static int qcom_scm_call_atomic(struct device *dev, u32 svc_id, u32 cmd_id,
> const struct qcom_scm_desc *desc,
> struct arm_smccc_res *res)
> {
> - return ___qcom_scm_call(dev, svc_id, cmd_id, desc, res, true);
> + return ___qcom_scm_call_smccc(dev, svc_id, cmd_id, desc, res, true);
> }
>
> /**
> @@ -253,7 +254,7 @@ int __qcom_scm_is_call_available(struct device *dev, u32 svc_id, u32 cmd_id)
> struct arm_smccc_res res;
>
> desc.arginfo = QCOM_SCM_ARGS(1);
> - desc.args[0] = QCOM_SCM_FNID(svc_id, cmd_id) |
> + desc.args[0] = SMCCC_FUNCNUM(svc_id, cmd_id) |
> (ARM_SMCCC_OWNER_SIP << ARM_SMCCC_OWNER_SHIFT);
>
> ret = qcom_scm_call(dev, QCOM_SCM_SVC_INFO, QCOM_IS_CALL_AVAIL_CMD,
> @@ -295,7 +296,7 @@ void __qcom_scm_init(void)
> {
> u64 cmd;
> struct arm_smccc_res res;
> - u32 function = QCOM_SCM_FNID(QCOM_SCM_SVC_INFO, QCOM_IS_CALL_AVAIL_CMD);
> + u32 function = SMCCC_FUNCNUM(QCOM_SCM_SVC_INFO, QCOM_IS_CALL_AVAIL_CMD);
>
> /* First try a SMC64 call */
> cmd = ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, ARM_SMCCC_SMC_64,
... to here I don't understand why any of it needs to change. It looks
like a bunch of churn and it conflates qcom SCM calls with SMCCC which
is not desirable. Those two concepts are different.
Powered by blists - more mailing lists