| 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: <CAMRc=MeDcMbH=xFZnr=_NZPQ2X9eOfRoRHnS1LJ6M54KwHMVRQ@mail.gmail.com>
Date: Tue, 4 Mar 2025 13:49:12 +0100
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Unnathi Chalicheemala <unnathi.chalicheemala@....qualcomm.com>
Cc: Bjorn Andersson <andersson@...nel.org>, Konrad Dybcio <konradybcio@...nel.org>,
linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
kernel@....qualcomm.com, Bartosz Golaszewski <bartosz.golaszewski@...aro.org>,
Prasad Sodagudi <prasad.sodagudi@....qualcomm.com>,
Satya Durga Srinivasu Prabhala <quic_satyap@...cinc.com>, Trilok Soni <quic_tsoni@...cinc.com>
Subject: Re: [PATCH v5 2/2] firmware: qcom_scm: Support multiple waitq contexts
On Fri, Feb 28, 2025 at 6:40 AM Unnathi Chalicheemala
<unnathi.chalicheemala@....qualcomm.com> wrote:
>
> Currently, only a single waitqueue context exists, with waitqueue id zero.
> Multi-waitqueue mechanism is added in firmware to support the case when
> multiple VMs make SMC calls or single VM making multiple calls on same CPU.
>
> When VMs make SMC call, firmware will allocate waitqueue context assuming
> the SMC call to be a blocking call. SMC calls that cannot acquire resources
> are returned to sleep in the calling VM. When resource is available, VM
> will be notified to wake sleeping thread and resume SMC call.
> SM8650 firmware can allocate two such waitq contexts so create these two
> waitqueue contexts.
>
> Unique waitqueue contexts are supported by a dynamically sized array where
> each unique wq_ctx is associated with a struct completion variable for easy
> lookup. To get the number of waitqueue contexts directly from firmware,
> qcom_scm_query_waitq_cnt() is introduced. On older targets which support
Seems like it's actually called qcom_scm_query_waitq_count
> only a single waitqueue, wq_cnt is set to 1 as SCM call for
> query_waitq_cnt() is not implemented for single waitqueue case.
>
> Signed-off-by: Unnathi Chalicheemala <unnathi.chalicheemala@....qualcomm.com>
> ---
> drivers/firmware/qcom/qcom_scm.c | 75 ++++++++++++++++++++++++++++------------
> 1 file changed, 53 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 1aa42685640da8a14191557896fbb49423697a10..ec139380ce5ba6d11f1023258e1d36edcf3d9d45 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -47,7 +47,7 @@ struct qcom_scm {
> struct clk *iface_clk;
> struct clk *bus_clk;
> struct icc_path *path;
> - struct completion waitq_comp;
> + struct completion *waitq;
> struct reset_controller_dev reset;
>
> /* control access to the interconnect path */
> @@ -57,6 +57,7 @@ struct qcom_scm {
> u64 dload_mode_addr;
>
> struct qcom_tzmem_pool *mempool;
> + unsigned int wq_cnt;
> };
>
> struct qcom_scm_current_perm_info {
> @@ -2118,6 +2119,25 @@ static int qcom_scm_fill_irq_fwspec_params(struct irq_fwspec *fwspec, u32 virq)
> return 0;
> }
>
> +static int qcom_scm_query_waitq_count(struct qcom_scm *scm)
> +{
> + int ret;
> + struct qcom_scm_desc desc = {
> + .svc = QCOM_SCM_SVC_WAITQ,
> + .cmd = QCOM_SCM_WAITQ_GET_INFO,
> + .owner = ARM_SMCCC_OWNER_SIP
> + };
> + struct qcom_scm_res res;
> +
> + ret = qcom_scm_call_atomic(scm->dev, &desc, &res);
This can fail for a multitude of reasons - some of which we may want
to propagate to the caller, how about being more fine-grained and
using __qcom_scm_is_call_available() to check if
QCOM_SCM_WAITQ_GET_INFO is available first?
> + if (ret) {
> + dev_err(scm->dev, "Multi-waitqueue support unavailable\n");
Is this an error though? From the commit message it seems it's normal
operation on older platforms?
Bartosz
> + return 1;
> + }
> +
> + return res.result[0] & GENMASK(7, 0);
> +}
> +
> static int qcom_scm_get_waitq_irq(void)
> {
> int ret;
> @@ -2149,42 +2169,40 @@ static int qcom_scm_get_waitq_irq(void)
> return ret;
> }
>
> -static int qcom_scm_assert_valid_wq_ctx(u32 wq_ctx)
> +static struct completion *qcom_scm_get_completion(u32 wq_ctx)
> {
> - /* FW currently only supports a single wq_ctx (zero).
> - * TODO: Update this logic to include dynamic allocation and lookup of
> - * completion structs when FW supports more wq_ctx values.
> - */
> - if (wq_ctx != 0) {
> - dev_err(__scm->dev, "Firmware unexpectedly passed non-zero wq_ctx\n");
> - return -EINVAL;
> - }
> + struct completion *wq;
>
> - return 0;
> + if (WARN_ON_ONCE(wq_ctx >= __scm->wq_cnt))
> + return ERR_PTR(-EINVAL);
> +
> + wq = &__scm->waitq[wq_ctx];
> +
> + return wq;
> }
>
> int qcom_scm_wait_for_wq_completion(u32 wq_ctx)
> {
> - int ret;
> + struct completion *wq;
>
> - ret = qcom_scm_assert_valid_wq_ctx(wq_ctx);
> - if (ret)
> - return ret;
> + wq = qcom_scm_get_completion(wq_ctx);
> + if (IS_ERR(wq))
> + return PTR_ERR(wq);
>
> - wait_for_completion(&__scm->waitq_comp);
> + wait_for_completion(wq);
>
> return 0;
> }
>
> static int qcom_scm_waitq_wakeup(unsigned int wq_ctx)
> {
> - int ret;
> + struct completion *wq;
>
> - ret = qcom_scm_assert_valid_wq_ctx(wq_ctx);
> - if (ret)
> - return ret;
> + wq = qcom_scm_get_completion(wq_ctx);
> + if (IS_ERR(wq))
> + return PTR_ERR(wq);
>
> - complete(&__scm->waitq_comp);
> + complete(wq);
>
> return 0;
> }
> @@ -2260,6 +2278,7 @@ static int qcom_scm_probe(struct platform_device *pdev)
> struct qcom_tzmem_pool_config pool_config;
> struct qcom_scm *scm;
> int irq, ret;
> + int i;
>
> scm = devm_kzalloc(&pdev->dev, sizeof(*scm), GFP_KERNEL);
> if (!scm)
> @@ -2270,7 +2289,19 @@ static int qcom_scm_probe(struct platform_device *pdev)
> if (ret < 0)
> return ret;
>
> - init_completion(&scm->waitq_comp);
> + ret = qcom_scm_query_waitq_count(scm);
> + if (ret < 0)
> + return ret;
> +
> + scm->wq_cnt = ret;
> +
> + scm->waitq = devm_kcalloc(&pdev->dev, scm->wq_cnt, sizeof(*scm->waitq), GFP_KERNEL);
> + if (!scm->waitq)
> + return -ENOMEM;
> +
> + for (i = 0; i < scm->wq_cnt; i++)
> + init_completion(&scm->waitq[i]);
> +
> mutex_init(&scm->scm_bw_lock);
>
> scm->path = devm_of_icc_get(&pdev->dev, NULL);
>
> --
> 2.34.1
>
>
Powered by blists - more mailing lists