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
