Re: [PATCH v1 11/14] iommu/arm-smmu-v3: Add arm_smmu_domain_alloc_user

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Mar 24, 2023 at 10:40:46AM -0700, Nicolin Chen wrote:
> Hi Eirc,
> 
> Thanks for the review.
> 
> On Fri, Mar 24, 2023 at 04:28:26PM +0100, Eric Auger wrote:
> 
> > > +static struct iommu_domain *
> > > +__arm_smmu_domain_alloc(unsigned type,
> > > +                     struct arm_smmu_domain *s2,
> > > +                     struct arm_smmu_master *master,
> > > +                     const struct iommu_hwpt_arm_smmuv3 *user_cfg)
> > > +{
> > > +     struct arm_smmu_domain *smmu_domain;
> > > +     struct iommu_domain *domain;
> > > +     int ret = 0;
> > > +
> > > +     if (type == IOMMU_DOMAIN_SVA)
> > > +             return arm_smmu_sva_domain_alloc();
> > > +
> > > +     if (type != IOMMU_DOMAIN_UNMANAGED &&
> > > +         type != IOMMU_DOMAIN_DMA &&
> > > +         type != IOMMU_DOMAIN_DMA_FQ &&
> > > +         type != IOMMU_DOMAIN_IDENTITY)
> > > +             return NULL;
> > > +
> > > +     /*
> > > +      * Allocate the domain and initialise some of its data structures.
> > > +      * We can't really finalise the domain unless a master is given.
> > > +      */
> > > +     smmu_domain = kzalloc(sizeof(*smmu_domain), GFP_KERNEL);
> > > +     if (!smmu_domain)
> > > +             return NULL;
> > > +     domain = &smmu_domain->domain;
> > > +
> > > +     domain->type = type;
> > > +     domain->ops = arm_smmu_ops.default_domain_ops;
> > Compared to the original code, that's something new. Please can you
> > explain why this is added in this patch?
> 
> Yea, I probably should have mentioned in the commit message that
> this function via ops->domain_alloc_user() is called by IOMMUFD
> directly without a wrapper, v.s. the other callers all go with
> the __iommu_domain_alloc() helper in the iommu core where an ops
> pointer gets setup.
> 
> So, this is something new, in order to work with IOMMUFD.

But using default_domain_ops is not great, the ops should be set based
on the domain type being created and the various different flavours
should have their own types and ops.

So name the existing ops something logical like 'unmanaged_domain_ops'
and move it out of the inline initializer.

Make another ops for identity like shown here to get the ball rolling:

 https://lore.kernel.org/r/20230324111127.221640-1-steven.price@arm.com

There is a whole bunch of tidying here to make things follow the op
per type design.

Jason