Re: [PATCH] firmware: qcom_scm: Give page_aligned size for dma api's

[Bjorn Andersson]

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@xxxxxxxxxxx>
> > > ---
> > >   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