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

[Mostafa Saleh <smostafa@google.com>]

On Mon, Apr 29, 2024 at 11:01:37AM -0300, Jason Gunthorpe wrote:
> On Sat, Apr 27, 2024 at 10:19:37PM +0000, Mostafa Saleh wrote:
> 
> > > I'm not sure what you are asking? We have two versions. One is called
> > > alloc and one is called get. That have different locking requirements
> > > on the caller so they have different names. I would not call them both
> > > get?
> > > 
> > 
> > My point is that arm_smmu_alloc_cd_ptr() doesn’t only allocate the leaf,
> > but also the L1 through arm_smmu_alloc_cd_tables()
> 
> Sure, it is called alloc, it allocs everything to make the CD table
> entry usable.

Maybe if it’s called alloc_leaf, it only allocates leafs :)

> 
> > 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, and that might go unnoticed as
this function will allow it, leading to memory leaks, 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.

Thanks,
Mostafa
> Jason