Re: [PATCH v7 5/9] iommu/arm-smmu-v3: Make arm_smmu_alloc_cd_ptr()

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, Apr 29, 2024 at 02:47:26PM +0000, Mostafa Saleh wrote:

> > > IMO, arm_smmu_alloc_cd_ptr() should only allocate leafs. And inside
> > > arm_smmu_attach_dev() it calls arm_smmu_alloc_cd_tables().
> > > This makes it clear which path is expected to allocate the L1 table.
> > 
> > The PASID path sometimes has to allocate the L1 table too, why
> > duplicate the allocation code?
> > 
> > What is different about the L1 vs L2 that it should be open coded?
> 
> I don’t think it is a big problem, but my main concern is robustness,
> for example a small erroneous code change might trigger allocation for
> L1 table from a path that shouldn’t,

A few patches more we add a lockdep, so a wrongly placed allocation is
*very* likely to hit the lockdep. If the lockdep satisfies then it is
not going to cause a functional problem.

> and that might go unnoticed as
> this function will allow it, leading to memory leaks, 

Any cd table memory allocated by arm_smmu_alloc_cd_ptr() is reliably
freed in the arm_smmu_release_device().

> or other issues that might be harder to triage later, instead with
> limiting which path allocates which level, would return a NULL in
> that case and fail immediately.

All cases that need to allocate a leaf need to allocate the L1 too, it
is artifical to make a distinction between them.

Jason