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

[Mukesh Ojha]

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 .

-Mukesh