Re: [PATCH v6 02/25] iommu: Add IOMMU_DOMAIN_PLATFORM

[Jason Gunthorpe <jgg@nvidia.com>]

On Sun, Aug 13, 2023 at 08:11:30PM +0800, Baolu Lu wrote:
> On 2023/8/12 19:28, Jason Gunthorpe wrote:
> > On Sat, Aug 12, 2023 at 09:36:33AM +0800, Baolu Lu wrote:
> > > > @@ -290,6 +295,7 @@ struct iommu_ops {
> > > >    	unsigned long pgsize_bitmap;
> > > >    	struct module *owner;
> > > >    	struct iommu_domain *identity_domain;
> > > > +	struct iommu_domain *default_domain;
> > > 
> > > I am imaging whether we can merge above two pointers into a single one.
> > > It is either an IDENTITY or PLATFORM domain and the core will choose it
> > > as the default domain of a group if iommu_group_alloc_default_domain()
> > > fails to allocate one through the iommu dev_ops.
> > 
> > I think that would be the wrong direction..
> > 
> > identity_domain is a pointer that is always, ALWAYS an identity
> > domain. It is the shortcut for drivers (and all drivers should do
> > this) that implement a global static identity domain.
> 
> I see. I originally thought this was special for arm32.

No, everything should use it eventually. Eg you would convert the
Intel driver too. It makes the function always available in all error
unwind paths.

> > default_domain is a shortcut to avoid implementing the entire flow
> > around def_domain_type/domain_alloc for special cases. For this patch
> > the specialc ase is the IOMMU_DOMAIN_PLATFORM.
> 
> I think this is special for drivers like s390. You don't want it to be
> used beyond those special drivers, right?

It ended up here:

arch/powerpc/kernel/iommu.c:    .default_domain = &spapr_tce_platform_domain,
drivers/iommu/fsl_pamu_domain.c:        .default_domain = &fsl_pamu_platform_domain,
drivers/iommu/iommufd/selftest.c:       .default_domain = &mock_blocking_domain,
drivers/iommu/s390-iommu.c:     .default_domain = &s390_iommu_platform_domain,
drivers/iommu/tegra-smmu.c:     .default_domain = &tegra_smmu_identity_domain,

And I have to try to remember why tegra-smmu had it.. So yes special
places only.

> If so, the naming of default_domain seems to be a bit generic. I can't
> think of a better one, hence I am fine if you keep as it-is. After all,
> the comment for this field has already explained it very clearly.

Me too

Thanks,
Jason