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

[Michael Shavit <mshavit@google.com>]

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>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel