Messages in this thread | | | From | Michael Shavit <> | Date | Tue, 5 Sep 2023 21:14:09 +0800 | Subject | Re: [PATCH v1 3/3] iommu/arm-smmu-v3-sva: Remove arm_smmu_bond |
| |
On Tue, Sep 5, 2023 at 8:42 PM Jason Gunthorpe <jgg@nvidia.com> wrote: > > On Tue, Sep 05, 2023 at 07:49:14PM +0800, Michael Shavit wrote: > > Create a new iommu_domain subclass for SVA iommu domains to hold the > > data previously stored in the dynamically allocated arm_smmu_bond. Add a > > simple count of attached SVA domains to arm_smmu_master to replace the > > list of bonds. > > > > Signed-off-by: Michael Shavit <mshavit@google.com> > > --- > > > > .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 70 +++++++------------ > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 1 - > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 2 +- > > 3 files changed, 26 insertions(+), 47 deletions(-) > > > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > > index 9fb6907c5e7d4..0342c0f35d55a 100644 > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > > @@ -24,14 +24,13 @@ struct arm_smmu_mmu_notifier { > > > > #define mn_to_smmu(mn) container_of(mn, struct arm_smmu_mmu_notifier, mn) > > > > -struct arm_smmu_bond { > > - struct mm_struct *mm; > > +struct arm_smmu_sva_domain { > > + struct iommu_domain iommu_domain; > > struct arm_smmu_mmu_notifier *smmu_mn; > > - struct list_head list; > > }; > > > > -#define sva_to_bond(handle) \ > > - container_of(handle, struct arm_smmu_bond, sva) > > +#define to_sva_domain(domain) \ > > + container_of(domain, struct arm_smmu_sva_domain, iommu_domain) > > I'm not sure about this? This seems like a strange direction > > The SVA domain and a UNMANAGED/PAGING domain should be basically the > same thing. Making a sva_domain a completely different type looks like > it would stand in the way of that?
Agreed that's the eventual destination of all these re-works, but the stage isn't fully set for that yet. IMO this is a simpler improvement to get through for now, and I don't see it being an obstacle in the future.
> > @@ -545,12 +526,11 @@ static const struct iommu_domain_ops arm_smmu_sva_domain_ops = { > > > > struct iommu_domain *arm_smmu_sva_domain_alloc(void) > > { > > - struct iommu_domain *domain; > > + struct arm_smmu_sva_domain *sva_domain; > > > > - domain = kzalloc(sizeof(*domain), GFP_KERNEL); > > - if (!domain) > > + sva_domain = kzalloc(sizeof(*sva_domain), GFP_KERNEL); > > + if (!sva_domain) > > return NULL; > > - domain->ops = &arm_smmu_sva_domain_ops; > > - > > - return domain; > > + sva_domain->iommu_domain.ops = &arm_smmu_sva_domain_ops; > > arm_smmu_sva_domain_free() should container_of before freeing, but > gross to assume the iommu_domain is the first member.
Oh good catch I missed updating the free.
| |