Messages in this thread | | | Date | Fri, 24 Mar 2023 11:00:05 -0700 | From | Nicolin Chen <> | Subject | Re: [PATCH v1 11/14] iommu/arm-smmu-v3: Add arm_smmu_domain_alloc_user |
| |
On Fri, Mar 24, 2023 at 02:50:42PM -0300, Jason Gunthorpe wrote: > 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.
Thanks for the suggestion. Will add a patch doing that in v2.
Nicolin
| |