| 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
| ||
|
Message-ID: <ad5e875d-e5d7-3b6f-4915-98ba08df8c5d@quicinc.com>
Date: Fri, 1 Jul 2022 16:51:00 +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 7/1/2022 4:32 PM, Rajendra Nayak wrote:
>
>
> 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?
>
Ok looking at this some more, I think what we are saying is that the FW returns
{WQ_WAKE} to REQUEST_1, we then call wq_wake_ack and the return of
*that* will tell if REQUEST_1 was success or failure?
Did I get it right?
>
>> │ │
>> │ │
>> │ 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;
if my understanding above is correct, shouldn't we do a
>> + if (wq)
>> + scm_waitq_flag_handler(wq, flags);
in the error case also?
Also why no just scm_waitq_flag_handler(wq, flags); before fill_wq_wake_ack_args(smc, smc_call_ctx);?
>> + } 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