Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
MIME-Version: 1.0
References: <20231009153427.20951-1-brgl@bgdev.pl> <20231009153427.20951-5-brgl@bgdev.pl>
 <f6jspdoeyv6ntcrl6qndy2ud3mcdkoxxcnzqm3qpbtcd3ztdpi@7iw5f5og7is2>
In-Reply-To: <f6jspdoeyv6ntcrl6qndy2ud3mcdkoxxcnzqm3qpbtcd3ztdpi@7iw5f5og7is2>
From:   Bartosz Golaszewski <brgl@bgdev.pl>
Date:   Tue, 10 Oct 2023 08:42:50 +0200
Message-ID: <CAMRc=Mdj6fkHaig131=LG2BtzS67mX6o-ornA=Bzo8oBA1UT9w@mail.gmail.com>
Subject: Re: [PATCH v3 04/15] firmware: qcom: add a dedicated TrustZone buffer allocator
To:     Andrew Halaney <ahalaney@redhat.com>
Cc:     Andy Gross <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        Elliot Berman <quic_eberman@quicinc.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        Guru Das Srinagesh <quic_gurus@quicinc.com>,
        Maximilian Luz <luzmaximilian@gmail.com>,
        Alex Elder <elder@linaro.org>,
        Srini Kandagatla <srinivas.kandagatla@linaro.org>,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, kernel@quicinc.com,
        Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Oct 9, 2023 at 11:28=E2=80=AFPM Andrew Halaney <ahalaney@redhat.com=
> wrote:
>
> On Mon, Oct 09, 2023 at 05:34:16PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > We have several SCM calls that require passing buffers to the TrustZone
> > on top of the SMC core which allocates memory for calls that require
> > more than 4 arguments.
> >
> > Currently every user does their own thing which leads to code
> > duplication. Many users call dma_alloc_coherent() for every call which
> > is terribly unperformant (speed- and size-wise).
> >
> > Provide a set of library functions for creating and managing pool of
> > memory which is suitable for sharing with the TrustZone, that is:
> > page-aligned, contiguous and non-cachable as well as provides a way of
> > mapping of kernel virtual addresses to physical space.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---

[snip]

>
> I got these warnings with this series:
>
>     ahalaney@fedora ~/git/linux-next (git)-[7204cc6c3d73] % ARCH=3Darm64 =
CROSS_COMPILE=3Daarch64-linux-gnu- make W=3D1 C=3D2 drivers/firmware/qcom/
>     drivers/firmware/qcom/qcom_tzmem.c:137: warning: Function parameter o=
r member 'size' not described in 'qcom_tzmem_pool_new'
>       CHECK   drivers/firmware/qcom/qcom_tzmem.c
>     drivers/firmware/qcom/qcom_tzmem.c:204:17: warning: incorrect type in=
 assignment (different address spaces)
>     drivers/firmware/qcom/qcom_tzmem.c:204:17:    expected void **slot
>     drivers/firmware/qcom/qcom_tzmem.c:204:17:    got void [noderef] __rc=
u **
>     drivers/firmware/qcom/qcom_tzmem.c:204:17: warning: incorrect type in=
 assignment (different address spaces)
>     drivers/firmware/qcom/qcom_tzmem.c:204:17:    expected void **slot
>     drivers/firmware/qcom/qcom_tzmem.c:204:17:    got void [noderef] __rc=
u **
>     drivers/firmware/qcom/qcom_tzmem.c:204:17: warning: incorrect type in=
 argument 1 (different address spaces)
>     drivers/firmware/qcom/qcom_tzmem.c:204:17:    expected void [noderef]=
 __rcu **slot
>     drivers/firmware/qcom/qcom_tzmem.c:204:17:    got void **slot
>     drivers/firmware/qcom/qcom_tzmem.c:204:17: warning: incorrect type in=
 assignment (different address spaces)
>     drivers/firmware/qcom/qcom_tzmem.c:204:17:    expected void **slot
>     drivers/firmware/qcom/qcom_tzmem.c:204:17:    got void [noderef] __rc=
u **
>     drivers/firmware/qcom/qcom_tzmem.c:339:13: warning: context imbalance=
 in 'qcom_tzmem_to_phys' - wrong count at exit
>
>
> All are confusing me, size seems described, I don't know much about
> radix tree usage / rcu, and the locking in qcom_tzmem_to_phys seems sane
> to me but I'm still grappling with the new syntax.
>
> For the one address space one, I _think_ maybe a diff like this is in
> order?
>
>     diff --git a/drivers/firmware/qcom/qcom_tzmem.c b/drivers/firmware/qc=
om/qcom_tzmem.c
>     index b3137844fe43..5b409615198d 100644
>     --- a/drivers/firmware/qcom/qcom_tzmem.c
>     +++ b/drivers/firmware/qcom/qcom_tzmem.c
>     @@ -193,7 +193,7 @@ void qcom_tzmem_pool_free(struct qcom_tzmem_pool =
*pool)
>             struct qcom_tzmem_chunk *chunk;
>             struct radix_tree_iter iter;
>             bool non_empty =3D false;
>     -       void **slot;
>     +       void __rcu **slot;
>
>             if (!pool)
>                     return;
>     @@ -202,7 +202,7 @@ void qcom_tzmem_pool_free(struct qcom_tzmem_pool =
*pool)
>
>             scoped_guard(spinlock_irqsave, &qcom_tzmem_chunks_lock) {
>                     radix_tree_for_each_slot(slot, &qcom_tzmem_chunks, &i=
ter, 0) {
>     -                       chunk =3D *slot;
>     +                       chunk =3D radix_tree_deref_slot_protected(slo=
t, &qcom_tzmem_chunks_lock);
>
>                             if (chunk->owner =3D=3D pool)
>                                     non_empty =3D true;
>

Ah, I was thinking about it but then figured that I already use a
spinlock and I didn't see these errors on my side so decided to
dereference it normally.

I'll check it again.

Bart

>
> Still planning on reviewing/testing the rest, but got tripped up there
> so thought I'd highlight it before doing the rest.
>
> Thanks,
> Andrew
>