| 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, 1 Jul 2022 16:32:28 +0530
From: Rajendra Nayak <quic_rjendra@...cinc.com>
To: Guru Das Srinagesh <quic_gurus@...cinc.com>,
Andy Gross <agross@...nel.org>,
Bjorn Andersson <bjorn.andersson@...aro.org>,
"Philipp Zabel" <p.zabel@...gutronix.de>,
<linux-arm-msm@...r.kernel.org>, <linux-kernel@...r.kernel.org>
CC: David Heidelberg <david@...t.cz>,
Robert Marko <robimarko@...il.com>,
Elliot Berman <quic_eberman@...cinc.com>
Subject: Re: [PATCH 5/5] firmware: qcom: scm: Add wait-queue handling logic
On 6/28/2022 1:14 AM, Guru Das Srinagesh wrote:
> Add logic to handle QCOM_SCM_WAITQ_SLEEP or QCOM_SCM_WAITQ_WAKE return
> codes.
>
> Scenario 1: Requests made by 2 different VMs:
>
> VM_1 VM_2 Firmware
> │ │ │
> │ │ │
> │ │ │
> │ │ │
> │ REQUEST_1 │ │
> ├────────────────────────┼─────────────────────────────────┤
> │ │ │
> │ │ ┌──┼──┐
> │ │ │ │ │
> │ │ REQUEST_2 │ │ │
> │ ├──────────────────────────────┼──┤ │
> │ │ │ │ │Resource
> │ │ │ │ │is busy
> │ │ {WQ_SLEEP} │ │ │
> │ │◄─────────────────────────────┼──┤ │
> │ │ wq_ctx, smc_call_ctx │ │ │
> │ │ └──┼──┘
> │ REQUEST_1 COMPLETE │ │
> │◄───────────────────────┼─────────────────────────────────┤
> │ │ │
> │ │ IRQ │
> │ │◄─-------------------------------│
> │ │ │
> │ │ get_wq_ctx() │
> │ ├────────────────────────────────►│
> │ │ │
> │ │ │
> │ │◄────────────────────────────────┤
> │ │ wq_ctx, flags, and │
> │ │ more_pending │
> │ │ │
> │ │ │
> │ │ wq_resume(smc_call_ctx) │
> │ ├────────────────────────────────►│
> │ │ │
> │ │ │
> │ │ REQUEST_2 COMPLETE │
> │ │◄────────────────────────────────┤
> │ │ │
> │ │ │
>
> Scenario 2: Two Requests coming in from same VM:
>
> VM_1 Firmware
> │ │
> │ │
> │ │
> │ │
> │ REQUEST_1 │
> ├──────────────────────────────────────────────────────────┤
> │ │
> │ ┌────┼───┐
> │ │ │ │
> │ │ │ │
> │ │ │ │
> │ REQUEST_2 │ │ │
> ├─────────────────────────────────────────────────────┼───►│ │
> │ │ │ │Resource
> │ │ │ │is busy
> │ {WQ_SLEEP} │ │ │
> │◄────────────────────────────────────────────────────┼────┤ │
> │ wq_ctx, req2_smc_call_ctx │ │ │
> │ │ │ │
> │ └────┼───┘
> │ │
> │ {WQ_WAKE} │
> │◄─────────────────────────────────────────────────────────┤
> │ wq_ctx, req1_smc_call_ctx, flags │
This is perhaps the same thing I asked on the previous patch,
I am guessing {WQ_WAKE} is returned in respone to REQUEST_1?
How do you know in this case if REQUEST_1 was a success or failure?
> │ │
> │ │
> │ wq_wake_ack(req1_smc_call_ctx) │
> ├─────────────────────────────────────────────────────────►│
> │ │
> │ REQUEST_1 COMPLETE │
> │◄─────────────────────────────────────────────────────────┤
> │ │
> │ │
> │ wq_resume(req_2_smc_call_ctx) │
> ├─────────────────────────────────────────────────────────►│
> │ │
> │ REQUEST_2 COMPLETE │
> │◄─────────────────────────────────────────────────────────┤
> │ │
>
> With the exception of get_wq_ctx(), the other two newly-introduced SMC
> calls, wq_ack() and wq_resume() can themselves return WQ_SLEEP (these
> nested rounds of WQ_SLEEP are not shown in the above diagram for the
> sake of simplicity). Therefore, introduce a new do-while loop to handle
> multiple WQ_SLEEP return values for the same parent SCM call.
>
> Request Completion in the above diagram refers to either a success
> return value (zero) or error (and not SMC_WAITQ_SLEEP or
> SMC_WAITQ_WAKE).
>
> Signed-off-by: Guru Das Srinagesh <quic_gurus@...cinc.com>
> ---
> drivers/firmware/qcom_scm-smc.c | 79 +++++++++++++++++++++++++++++++++++++----
> 1 file changed, 72 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/firmware/qcom_scm-smc.c b/drivers/firmware/qcom_scm-smc.c
> index 4150da1..fe95cc3 100644
> --- a/drivers/firmware/qcom_scm-smc.c
> +++ b/drivers/firmware/qcom_scm-smc.c
> @@ -53,6 +53,9 @@ static void __scm_smc_do_quirk(const struct arm_smccc_args *smc,
> } while (res->a0 == QCOM_SCM_INTERRUPTED);
> }
>
> +#define IS_WAITQ_SLEEP_OR_WAKE(res) \
> + (res->a0 == QCOM_SCM_WAITQ_SLEEP || res->a0 == QCOM_SCM_WAITQ_WAKE)
> +
> static void fill_wq_resume_args(struct arm_smccc_args *resume, u32 smc_call_ctx)
> {
> memset(resume->args, 0, ARRAY_SIZE(resume->args));
> @@ -109,25 +112,80 @@ int scm_get_wq_ctx(u32 *wq_ctx, u32 *flags, u32 *more_pending)
> return 0;
> }
>
> -static void __scm_smc_do(const struct arm_smccc_args *smc,
> +static int scm_smc_do_quirk(struct device *dev, struct arm_smccc_args *smc,
> + struct arm_smccc_res *res)
> +{
> + struct completion *wq = NULL;
> + struct qcom_scm *qscm;
> + u32 wq_ctx, smc_call_ctx, flags;
> +
> + do {
> + __scm_smc_do_quirk(smc, res);
> +
> + if (IS_WAITQ_SLEEP_OR_WAKE(res)) {
> + wq_ctx = res->a1;
> + smc_call_ctx = res->a2;
> + flags = res->a3;
> +
> + if (!dev)
> + return -EPROBE_DEFER;
> +
> + qscm = dev_get_drvdata(dev);
> + wq = qcom_scm_lookup_wq(qscm, wq_ctx);
> + if (IS_ERR_OR_NULL(wq)) {
> + pr_err("No waitqueue found for wq_ctx %d: %ld\n",
> + wq_ctx, PTR_ERR(wq));
> + return PTR_ERR(wq);
> + }
> +
> + if (res->a0 == QCOM_SCM_WAITQ_SLEEP) {
> + wait_for_completion(wq);
> + fill_wq_resume_args(smc, smc_call_ctx);
> + wq = NULL;
> + continue;
> + } else {
> + fill_wq_wake_ack_args(smc, smc_call_ctx);
> + continue;
> + }
> + } else if ((long)res->a0 < 0) {
> + /* Error, simply return to caller */
> + break;
> + } else {
> + /*
> + * Success.
> + * wq will be set only if a prior WAKE happened.
> + * Its value will be the one from the prior WAKE.
> + */
> + if (wq)
> + scm_waitq_flag_handler(wq, flags);
> + break;
> + }
> + } while (IS_WAITQ_SLEEP_OR_WAKE(res));
> +
> + return 0;
> +}
> +
> +static int __scm_smc_do(struct device *dev, struct arm_smccc_args *smc,
> struct arm_smccc_res *res, bool atomic)
> {
> - int retry_count = 0;
> + int ret, retry_count = 0;
>
> if (atomic) {
> __scm_smc_do_quirk(smc, res);
> - return;
> + return 0;
> }
>
> do {
> if (!qcom_scm_allow_multicall)
> mutex_lock(&qcom_scm_lock);
>
> - __scm_smc_do_quirk(smc, res);
> + ret = scm_smc_do_quirk(dev, smc, res);
>
> if (!qcom_scm_allow_multicall)
> mutex_unlock(&qcom_scm_lock);
>
> + if (ret)
> + return ret;
>
> if (res->a0 == QCOM_SCM_V2_EBUSY) {
> if (retry_count++ > QCOM_SCM_EBUSY_MAX_RETRY)
> @@ -135,6 +193,8 @@ static void __scm_smc_do(const struct arm_smccc_args *smc,
> msleep(QCOM_SCM_EBUSY_WAIT_MS);
> }
> } while (res->a0 == QCOM_SCM_V2_EBUSY);
> +
> + return 0;
> }
>
>
> @@ -143,7 +203,7 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
> struct qcom_scm_res *res, bool atomic)
> {
> int arglen = desc->arginfo & 0xf;
> - int i;
> + int i, ret;
> dma_addr_t args_phys = 0;
> void *args_virt = NULL;
> size_t alloc_len;
> @@ -195,19 +255,24 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
> smc.args[SCM_SMC_LAST_REG_IDX] = args_phys;
> }
>
> - __scm_smc_do(&smc, &smc_res, atomic);
> + ret = __scm_smc_do(dev, &smc, &smc_res, atomic);
> + /* ret error check follows after args_virt cleanup*/
>
> if (args_virt) {
> dma_unmap_single(dev, args_phys, alloc_len, DMA_TO_DEVICE);
> kfree(args_virt);
> }
>
> + if (ret)
> + return ret;
> +
> if (res) {
> res->result[0] = smc_res.a1;
> res->result[1] = smc_res.a2;
> res->result[2] = smc_res.a3;
> }
>
> - return (long)smc_res.a0 ? qcom_scm_remap_error(smc_res.a0) : 0;
> + ret = (long)smc_res.a0 ? qcom_scm_remap_error(smc_res.a0) : 0;
>
> + return ret;
> }
Powered by blists - more mailing lists