linux-kernel - Re: [PATCH v2 4/5] firmware: qcom: scm: Add wait-queue helper functions

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220811030022.GA18104@quicinc.com>
Date:   Wed, 10 Aug 2022 20:00:24 -0700
From:   Guru Das Srinagesh <quic_gurus@...cinc.com>
To:     Rajendra Nayak <quic_rjendra@...cinc.com>
CC:     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>,
        "David Heidelberg" <david@...t.cz>,
        Robert Marko <robimarko@...il.com>,
        Elliot Berman <quic_eberman@...cinc.com>
Subject: Re: [PATCH v2 4/5] firmware: qcom: scm: Add wait-queue helper
 functions

Hey Rajendra,

Sorry for the delay in response. Needed to clarify with internal team members
on these questions before responding.

On Aug 02 2022 17:07, Rajendra Nayak wrote:
> 
> On 7/23/2022 4:07 AM, Guru Das Srinagesh wrote:
> >When the firmware (FW) supports multiple requests per VM, and the VM
> >also supports it via the `allow-multi-call` device tree flag, the
> >floodgates are thrown open for them to all reach the firmware at the
> >same time.

[...]

> >   2) SCM_WAITQ_WAKE:
> >
> >   	When an SCM call receives this return value instead of success
> >   	or error, FW wishes to signal HLOS to wake up a (different)
> >   	previously sleeping call.
> >
> >   	FW tells HLOS which call to wake up via the additional return
> >   	values `wq_ctx`, `smc_call_ctx` and `flags`. The first two have
> >   	already been explained above.
> >
> >   	`flags` can be either WAKE_ONE or WAKE_ALL. Meaning, wake either
> >   	one, or all, of the SCM calls that HLOS is associating with the
> >   	given `wq_ctx`.
> >
> >A sleeping SCM call can be woken up by either an interrupt that FW
> >raises, or via a SCM_WAITQ_WAKE return value for a new SCM call.
> 
> Do you know why the FW was not designed to always use an interrupt?
> That would have made the handling of this in kernel a lot less complicated.

Because:

1. Our firmware in TrustZone cannot raise interrupts on its own - it needs the
hypervisor to do that.

2. Thus, in platforms where there is no hypervisor, there is no interrupt
possible - only SMC_WAITQ_WAKE.

Therefore, relying only on an interrupt would render the driver unable to
support platforms without a hypervisor, which we didn't want to do.

> >The handshake mechanism that HLOS uses to talk to FW about wait-queue
> >operations involves three new SMC calls. These are:
> >

[...]

> >+static void scm_irq_work(struct work_struct *work)
> >+{
> >+	int ret;
> >+	u32 wq_ctx, flags, more_pending = 0;
> >+	struct completion *wq_to_wake;
> >+	struct qcom_scm_waitq *w = container_of(work, struct qcom_scm_waitq, scm_irq_work);
> >+	struct qcom_scm *scm = container_of(w, struct qcom_scm, waitq);
> >+
> >+	do {
> >+		ret = scm_get_wq_ctx(&wq_ctx, &flags, &more_pending);
> >+		if (ret) {
> >+			pr_err("GET_WQ_CTX SMC call failed: %d\n", ret);
> >+			return;
> >+		}
> >+
> >+		wq_to_wake = qcom_scm_lookup_wq(scm, wq_ctx);
> >+		if (IS_ERR_OR_NULL(wq_to_wake)) {
> >+			pr_err("No waitqueue found for wq_ctx %d: %ld\n",
> >+					wq_ctx, PTR_ERR(wq_to_wake));
> >+			return;
> 
> What happens if at this point 'more_pending' was true? will the FW raise
> another interrupt?

Hmm. At this point, the interrupt handler is early-exiting without waking up a
sleeping call via the flag_handler() because firmware has goofed up and given
it an invalid wq_ctx. We have bigger problems than `more_pending` being true.

> 
> >+		}
> >+
> >+		scm_waitq_flag_handler(wq_to_wake, flags);
> >+	} while (more_pending);
> >+}

Thank you.

Guru Das.