Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Date:   Tue, 10 Oct 2023 15:48:34 -0500
From:   Andrew Halaney <ahalaney@redhat.com>
To:     Bartosz Golaszewski <brgl@bgdev.pl>
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>
Subject: Re: [PATCH v3 04/15] firmware: qcom: add a dedicated TrustZone
 buffer allocator
Message-ID: <pmmetoy5pb6khrjzsg6cd565cw3eowvs5i67465ne4xlqkrqgk@befdv3vfran3>
References: <20231009153427.20951-1-brgl@bgdev.pl>
 <20231009153427.20951-5-brgl@bgdev.pl>
 <f6jspdoeyv6ntcrl6qndy2ud3mcdkoxxcnzqm3qpbtcd3ztdpi@7iw5f5og7is2>
 <CAMRc=MchBQrpSqHHs-cD0RmOdSoKt2SLd76a97E8mSmHYGUCUg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAMRc=MchBQrpSqHHs-cD0RmOdSoKt2SLd76a97E8mSmHYGUCUg@mail.gmail.com>
Precedence: bulk

On Tue, Oct 10, 2023 at 10:26:34AM +0200, Bartosz Golaszewski wrote:
> On Mon, Oct 9, 2023 at 11:28 PM 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=arm64 CROSS_COMPILE=aarch64-linux-gnu- make W=1 C=2 drivers/firmware/qcom/
> >     drivers/firmware/qcom/qcom_tzmem.c:137: warning: Function parameter or 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] __rcu **
> >     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] __rcu **
> >     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] __rcu **
> >     drivers/firmware/qcom/qcom_tzmem.c:339:13: warning: context imbalance in 'qcom_tzmem_to_phys' - wrong count at exit
> 
> I fixed the other ones but this one I think comes from CHECK not
> dealing correctly with the spinlock guard.
> 
> >
> >
> > 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/qcom/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 = 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, &iter, 0) {
> >     -                       chunk = *slot;
> >     +                       chunk = radix_tree_deref_slot_protected(slot, &qcom_tzmem_chunks_lock);
> 
> We need to keep the lock taken for the duration of the looping so we
> can use the regular radix_tree_deref_slot().

IIUC, using the protected version is preferable since you already
have the lock in hand: https://www.kernel.org/doc/html/latest/RCU/whatisRCU.html#id2

Quote:
    The variant rcu_dereference_protected() can be used outside of an RCU
    read-side critical section as long as the usage is protected by locks
    acquired by the update-side code. This variant avoids the lockdep warning
    that would happen when using (for example) rcu_dereference() without
    rcu_read_lock() protection. Using rcu_dereference_protected() also has
    the advantage of permitting compiler optimizations that rcu_dereference()
    must prohibit. The rcu_dereference_protected() variant takes a lockdep
    expression to indicate which locks must be acquired by the caller.
    If the indicated protection is not provided, a lockdep splat is emitted.

Thanks,
Andrew


> 
> Bart
> 
> >
> >                             if (chunk->owner == pool)
> >                                     non_empty = true;
> >
> >
> > Still planning on reviewing/testing the rest, but got tripped up there
> > so thought I'd highlight it before doing the rest.
> >
> > Thanks,
> > Andrew
> >
>