Received-SPF: pass (google.com: domain of linux-kernel+bounces-185385-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 21 May 2024 12:31:53 -0700
From: Bjorn Andersson <quic_bjorande@quicinc.com>
To: Mukesh Ojha <quic_mojha@quicinc.com>
CC: Elliot Berman <quic_eberman@quicinc.com>, <andersson@kernel.org>,
        <konrad.dybcio@linaro.org>, <linux-arm-msm@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] firmware: qcom_scm: Give page_aligned size for dma api's
Message-ID: <Zkz2qRr9QnjL1Yg5@hu-bjorande-lv.qualcomm.com>
References: <1715887976-1288-1-git-send-email-quic_mojha@quicinc.com>
 <20240516131759140-0700.eberman@hu-eberman-lv.qualcomm.com>
 <a39b3b85-8abe-52fd-e4b9-81720ffcd6a1@quicinc.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <a39b3b85-8abe-52fd-e4b9-81720ffcd6a1@quicinc.com>

On Mon, May 20, 2024 at 05:55:49PM +0530, Mukesh Ojha wrote:
> Thanks for the review..
> 
> On 5/17/2024 1:58 AM, Elliot Berman wrote:
> > On Fri, May 17, 2024 at 01:02:56AM +0530, Mukesh Ojha wrote:
> > > If we disable CONFIG_ZONE_DMA32 to make the selection of DMA
> > > memory from higher 4GB range. dma_alloc_coherant() api usage
> >                                  dma_alloc_coherent()
> > > inside qcom_scm_pas_init_image() which usage scm 32bit device
> > > will fail for size of data passed less than PAGE_SIZE and
> > > it will fallback to buddy pool to allocate memory from which
> > > will fail.
> > 
> > I interpreted this as:
> > 
> > When CONFIG_ZONE_DMA32 is disabled, dma_alloc_coherent() fails when size
> > is < PAGE_SIZE. qcom_scm_pas_init_image() will fail to allocate using > dma_alloc_coherent() and incorrectly fall back to buddy pool.
> > 
> > This justification seems incorrect to me. None of the other
> > dma_alloc_coherent() users are page-aligning their requests in scm
> > driver. Is something else going on?
> 
> For SCM protection, memory allocation should be physically contiguous, 4K
> aligned and non-cacheable to avoid XPU violations as that is the
> granularity of protection to be applied from secure world also what if,
> there is a 32-bit secure peripheral is going to access this memory which
> for some SoCs and some not.
> 
> So, we wanted to keep this common and align across multiple SoCs to do
> the allocation from CMA and add a pad to the memory passed to secure world
> Also, this also enables us to keep CONFIG_ZONE_{DMA|DMA32} disabled which is
> a significant overhead.
> 
> > 
> > > 
> > > Convert the size to aligned to PAGE_SIZE before it gets pass
> > > to dma_alloc_coherant(), so that it gets coherant memory in
> >       dma_alloc_coherent                    coherent
> > > lower 4GB from linux cma region.
> > > 
> > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> > > ---
> > >   drivers/firmware/qcom/qcom_scm.c | 8 +++++---
> > >   1 file changed, 5 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> > > index 029ee5edbea6..6616048f1c33 100644
> > > --- a/drivers/firmware/qcom/qcom_scm.c
> > > +++ b/drivers/firmware/qcom/qcom_scm.c
> > > @@ -562,6 +562,7 @@ static void qcom_scm_set_download_mode(u32 dload_mode)
> > >   int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
> > >   			    struct qcom_scm_pas_metadata *ctx)
> > >   {
> > > +	size_t page_aligned_size;
> > >   	dma_addr_t mdata_phys;
> > >   	void *mdata_buf;
> > >   	int ret;
> > > @@ -579,7 +580,8 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
> > >   	 * data blob, so make sure it's physically contiguous, 4K aligned and
> > >   	 * non-cachable to avoid XPU violations.
> > >   	 */
> > > -	mdata_buf = dma_alloc_coherent(__scm->dev, size, &mdata_phys,
> > > +	page_aligned_size = PAGE_ALIGN(size + PAGE_SIZE);
> > 
> > Isn't PAGE_ALIGN(size) good enough? Why do you need to round up to the
> > 2nd page? Maybe you thought PAGE_ALIGN was PAGE_ALIGN_DOWN ?
> 
> No, this was intentional as there is a check inside
> dma_alloc_contiguous() call for a size <= PAGE_SIZE .
> 

Sure, but as Elliot points out PAGE_ALIGN(x) for X <= PAGE_SIZE is
PAGE_SIZE, so you wouldn't pass a value < PAGE_SIZE to the function.

That said, here you say <= PAGE_SIZE and in commit message you say "less
than PAGE_SIZE"...


If you need this to be 2 pages, it needs to be vary clearly documented,
to avoid having future readers fixing what looks like a bug.

Regards,
Bjorn