diff mbox series

[v5,18/27] iommu/arm-smmu-v3: Make SVA allocate a normal arm_smmu_domain

Message ID 18-v5-9a37e0c884ce+31e3-smmuv3_newapi_p2_jgg@nvidia.com (mailing list archive)
State New, archived
Headers show
Series Update SMMUv3 to the modern iommu API (part 2/3) | expand

Commit Message

Jason Gunthorpe March 4, 2024, 11:44 p.m. UTC 
Currently the SVA domain is a naked struct iommu_domain, allocate a struct
arm_smmu_domain instead.

This is necessary to be able to use the struct arm_master_domain
mechanism.

Tested-by: Nicolin Chen <nicolinc@nvidia.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   | 19 ++++++-----
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 34 +++++++++++--------
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  4 ++-
 3 files changed, 33 insertions(+), 24 deletions(-)

Comments

Michael Shavit March 19, 2024, 2:52 p.m. UTC | #1 
On Tue, Mar 5, 2024 at 7:44 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> Currently the SVA domain is a naked struct iommu_domain, allocate a struct
> arm_smmu_domain instead.
>
> This is necessary to be able to use the struct arm_master_domain
> mechanism.
>
> Tested-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   | 19 ++++++-----
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 34 +++++++++++--------
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  4 ++-
>  3 files changed, 33 insertions(+), 24 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 82b9c4d4061c3d..d633316f2e45bc 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
> @@ -654,7 +654,7 @@ static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain,
>
>  static void arm_smmu_sva_domain_free(struct iommu_domain *domain)
>  {
> -       kfree(domain);
> +       kfree(to_smmu_domain(domain));
>  }
>
>  static const struct iommu_domain_ops arm_smmu_sva_domain_ops = {
> @@ -662,14 +662,17 @@ static const struct iommu_domain_ops arm_smmu_sva_domain_ops = {
>         .free                   = arm_smmu_sva_domain_free
>  };
>
> -struct iommu_domain *arm_smmu_sva_domain_alloc(void)
> +struct iommu_domain *arm_smmu_sva_domain_alloc(unsigned type)
>  {
> -       struct iommu_domain *domain;
> +       struct arm_smmu_domain *smmu_domain;
>
> -       domain = kzalloc(sizeof(*domain), GFP_KERNEL);
> -       if (!domain)
> -               return NULL;
> -       domain->ops = &arm_smmu_sva_domain_ops;
> +       if (type != IOMMU_DOMAIN_SVA)
> +               return ERR_PTR(-EOPNOTSUPP);
>
> -       return domain;
> +       smmu_domain = arm_smmu_domain_alloc();
> +       if (IS_ERR(smmu_domain))
> +               return ERR_CAST(smmu_domain);
> +       smmu_domain->domain.ops = &arm_smmu_sva_domain_ops;
> +
> +       return &smmu_domain->domain;
>  }
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index dd7f841cd19b3c..2db2b822292a87 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2291,23 +2291,10 @@ static bool arm_smmu_capable(struct device *dev, enum iommu_cap cap)
>         }
>  }
>
> -static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
> -{
> -
> -       if (type == IOMMU_DOMAIN_SVA)
> -               return arm_smmu_sva_domain_alloc();
> -       return ERR_PTR(-EOPNOTSUPP);
> -}
> -
> -static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev)
> +struct arm_smmu_domain *arm_smmu_domain_alloc(void)

Consider renaming arm_smmu_domain_free as well since there's asymmetry
between arm_smmu_domain_alloc and arm_smmu_domain_free that could be a
little confusing:
1. arm_smmu_domain_alloc is shared between arm_smmu_sva_domain_alloc
and arm_smmu_domain_alloc_paging
2. arm_smmu_domain_free is only used by paging domains, with SVA
domains freed through arm_smmu_sva_domain_free.


>  {
>         struct arm_smmu_domain *smmu_domain;
>
> -       /*
> -        * Allocate the domain and initialise some of its data structures.
> -        * We can't really do anything meaningful until we've added a
> -        * master.
> -        */
>         smmu_domain = kzalloc(sizeof(*smmu_domain), GFP_KERNEL);
>         if (!smmu_domain)
>                 return ERR_PTR(-ENOMEM);
> @@ -2317,6 +2304,23 @@ static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev)
>         spin_lock_init(&smmu_domain->devices_lock);
>         INIT_LIST_HEAD(&smmu_domain->mmu_notifiers);
>
> +       return smmu_domain;
> +}
> +
> +static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev)
> +{
> +       struct arm_smmu_domain *smmu_domain;
> +
> +       smmu_domain = arm_smmu_domain_alloc();
> +       if (IS_ERR(smmu_domain))
> +               return ERR_CAST(smmu_domain);
> +
> +       /*
> +        * Allocate the domain and initialise some of its data structures.
> +        * We can't really do anything meaningful until we've added a
> +        * master.
> +        */
> +
>         if (dev) {
>                 struct arm_smmu_master *master = dev_iommu_priv_get(dev);
>                 int ret;
> @@ -3288,7 +3292,7 @@ static struct iommu_ops arm_smmu_ops = {
>         .identity_domain        = &arm_smmu_identity_domain,
>         .blocked_domain         = &arm_smmu_blocked_domain,
>         .capable                = arm_smmu_capable,
> -       .domain_alloc           = arm_smmu_domain_alloc,
> +       .domain_alloc           = arm_smmu_sva_domain_alloc,
>         .domain_alloc_paging    = arm_smmu_domain_alloc_paging,
>         .probe_device           = arm_smmu_probe_device,
>         .release_device         = arm_smmu_release_device,
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> index 7e1f6af4ce4e79..c47e07d695bef2 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -759,6 +759,8 @@ static inline struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
>  extern struct xarray arm_smmu_asid_xa;
>  extern struct mutex arm_smmu_asid_lock;
>
> +struct arm_smmu_domain *arm_smmu_domain_alloc(void);
> +
>  void arm_smmu_clear_cd(struct arm_smmu_master *master, ioasid_t ssid);
>  struct arm_smmu_cd *arm_smmu_get_cd_ptr(struct arm_smmu_master *master,
>                                         u32 ssid);
> @@ -791,7 +793,7 @@ int arm_smmu_master_enable_sva(struct arm_smmu_master *master);
>  int arm_smmu_master_disable_sva(struct arm_smmu_master *master);
>  bool arm_smmu_master_iopf_supported(struct arm_smmu_master *master);
>  void arm_smmu_sva_notifier_synchronize(void);
> -struct iommu_domain *arm_smmu_sva_domain_alloc(void);
> +struct iommu_domain *arm_smmu_sva_domain_alloc(unsigned int type);
>  void arm_smmu_sva_remove_dev_pasid(struct iommu_domain *domain,
>                                    struct device *dev, ioasid_t id);
>  #else /* CONFIG_ARM_SMMU_V3_SVA */
> --
> 2.43.2
>
>
Reviewed-by: Michael Shavit <mshavit@google.com>
Jason Gunthorpe March 20, 2024, 11:20 p.m. UTC | #2 
On Tue, Mar 19, 2024 at 10:52:11PM +0800, Michael Shavit wrote:
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > index dd7f841cd19b3c..2db2b822292a87 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -2291,23 +2291,10 @@ static bool arm_smmu_capable(struct device *dev, enum iommu_cap cap)
> >         }
> >  }
> >
> > -static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
> > -{
> > -
> > -       if (type == IOMMU_DOMAIN_SVA)
> > -               return arm_smmu_sva_domain_alloc();
> > -       return ERR_PTR(-EOPNOTSUPP);
> > -}
> > -
> > -static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev)
> > +struct arm_smmu_domain *arm_smmu_domain_alloc(void)
> 
> Consider renaming arm_smmu_domain_free as well since there's asymmetry
> between arm_smmu_domain_alloc and arm_smmu_domain_free that could be a
> little confusing:
> 1. arm_smmu_domain_alloc is shared between arm_smmu_sva_domain_alloc
> and arm_smmu_domain_alloc_paging
> 2. arm_smmu_domain_free is only used by paging domains, with SVA
> domains freed through arm_smmu_sva_domain_free.

Yeah, that is a good idea, done

Thanks,
Jason
diff mbox series

Patch

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 82b9c4d4061c3d..d633316f2e45bc 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
@@ -654,7 +654,7 @@  static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain,
 
 static void arm_smmu_sva_domain_free(struct iommu_domain *domain)
 {
-	kfree(domain);
+	kfree(to_smmu_domain(domain));
 }
 
 static const struct iommu_domain_ops arm_smmu_sva_domain_ops = {
@@ -662,14 +662,17 @@  static const struct iommu_domain_ops arm_smmu_sva_domain_ops = {
 	.free			= arm_smmu_sva_domain_free
 };
 
-struct iommu_domain *arm_smmu_sva_domain_alloc(void)
+struct iommu_domain *arm_smmu_sva_domain_alloc(unsigned type)
 {
-	struct iommu_domain *domain;
+	struct arm_smmu_domain *smmu_domain;
 
-	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
-	if (!domain)
-		return NULL;
-	domain->ops = &arm_smmu_sva_domain_ops;
+	if (type != IOMMU_DOMAIN_SVA)
+		return ERR_PTR(-EOPNOTSUPP);
 
-	return domain;
+	smmu_domain = arm_smmu_domain_alloc();
+	if (IS_ERR(smmu_domain))
+		return ERR_CAST(smmu_domain);
+	smmu_domain->domain.ops = &arm_smmu_sva_domain_ops;
+
+	return &smmu_domain->domain;
 }
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index dd7f841cd19b3c..2db2b822292a87 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2291,23 +2291,10 @@  static bool arm_smmu_capable(struct device *dev, enum iommu_cap cap)
 	}
 }
 
-static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
-{
-
-	if (type == IOMMU_DOMAIN_SVA)
-		return arm_smmu_sva_domain_alloc();
-	return ERR_PTR(-EOPNOTSUPP);
-}
-
-static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev)
+struct arm_smmu_domain *arm_smmu_domain_alloc(void)
 {
 	struct arm_smmu_domain *smmu_domain;
 
-	/*
-	 * Allocate the domain and initialise some of its data structures.
-	 * We can't really do anything meaningful until we've added a
-	 * master.
-	 */
 	smmu_domain = kzalloc(sizeof(*smmu_domain), GFP_KERNEL);
 	if (!smmu_domain)
 		return ERR_PTR(-ENOMEM);
@@ -2317,6 +2304,23 @@  static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev)
 	spin_lock_init(&smmu_domain->devices_lock);
 	INIT_LIST_HEAD(&smmu_domain->mmu_notifiers);
 
+	return smmu_domain;
+}
+
+static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev)
+{
+	struct arm_smmu_domain *smmu_domain;
+
+	smmu_domain = arm_smmu_domain_alloc();
+	if (IS_ERR(smmu_domain))
+		return ERR_CAST(smmu_domain);
+
+	/*
+	 * Allocate the domain and initialise some of its data structures.
+	 * We can't really do anything meaningful until we've added a
+	 * master.
+	 */
+
 	if (dev) {
 		struct arm_smmu_master *master = dev_iommu_priv_get(dev);
 		int ret;
@@ -3288,7 +3292,7 @@  static struct iommu_ops arm_smmu_ops = {
 	.identity_domain	= &arm_smmu_identity_domain,
 	.blocked_domain		= &arm_smmu_blocked_domain,
 	.capable		= arm_smmu_capable,
-	.domain_alloc		= arm_smmu_domain_alloc,
+	.domain_alloc		= arm_smmu_sva_domain_alloc,
 	.domain_alloc_paging    = arm_smmu_domain_alloc_paging,
 	.probe_device		= arm_smmu_probe_device,
 	.release_device		= arm_smmu_release_device,
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index 7e1f6af4ce4e79..c47e07d695bef2 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -759,6 +759,8 @@  static inline struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
 extern struct xarray arm_smmu_asid_xa;
 extern struct mutex arm_smmu_asid_lock;
 
+struct arm_smmu_domain *arm_smmu_domain_alloc(void);
+
 void arm_smmu_clear_cd(struct arm_smmu_master *master, ioasid_t ssid);
 struct arm_smmu_cd *arm_smmu_get_cd_ptr(struct arm_smmu_master *master,
 					u32 ssid);
@@ -791,7 +793,7 @@  int arm_smmu_master_enable_sva(struct arm_smmu_master *master);
 int arm_smmu_master_disable_sva(struct arm_smmu_master *master);
 bool arm_smmu_master_iopf_supported(struct arm_smmu_master *master);
 void arm_smmu_sva_notifier_synchronize(void);
-struct iommu_domain *arm_smmu_sva_domain_alloc(void);
+struct iommu_domain *arm_smmu_sva_domain_alloc(unsigned int type);
 void arm_smmu_sva_remove_dev_pasid(struct iommu_domain *domain,
 				   struct device *dev, ioasid_t id);
 #else /* CONFIG_ARM_SMMU_V3_SVA */