Re: [PATCH v1 11/14] iommu/arm-smmu-v3: Add arm_smmu_domain_alloc_user

From: Eric Auger
Date: Fri Mar 24 2023 - 11:34:51 EST




On 3/9/23 11:53, Nicolin Chen wrote:
> The arm_smmu_domain_alloc_user callback function is used for userspace to
> allocate iommu_domains, such as standalone stage-1 domain, nested stage-1
> domain, and nested stage-2 domain. The input user_data is in the type of
> struct iommu_hwpt_arm_smmuv3 that contains the configurations of a nested
> stage-1 or a nested stage-2 iommu_domain. A NULL user_data will just opt
> in a standalone stage-1 domain allocation.
>
> Add a constitutive function __arm_smmu_domain_alloc to support that.
>
> Since ops->domain_alloc_user has a valid dev pointer, the master pointer
> is available when calling __arm_smmu_domain_alloc() in this case, meaning
> that arm_smmu_domain_finalise() can be done at the allocation stage. This
> allows IOMMUFD to initialize the hw_pagetable for the domain.
>
> Signed-off-by: Nicolin Chen <nicolinc@xxxxxxxxxx>
> ---
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 95 ++++++++++++++-------
> 1 file changed, 65 insertions(+), 30 deletions(-)
>
> 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 2d29f7320570..5ff74edfbd68 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2053,36 +2053,6 @@ static void *arm_smmu_hw_info(struct device *dev, u32 *length)
> return info;
> }
>
> -static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
> -{
> - struct arm_smmu_domain *smmu_domain;
> -
> - 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 do anything meaningful until we've added a
> - * master.
> - */
> - smmu_domain = kzalloc(sizeof(*smmu_domain), GFP_KERNEL);
> - if (!smmu_domain)
> - return NULL;
> -
> - mutex_init(&smmu_domain->init_mutex);
> - INIT_LIST_HEAD(&smmu_domain->devices);
> - spin_lock_init(&smmu_domain->devices_lock);
> - INIT_LIST_HEAD(&smmu_domain->mmu_notifiers);
> -
> - return &smmu_domain->domain;
> -}
> -
> static struct iommu_domain *arm_smmu_get_unmanaged_domain(struct device *dev)
> {
> struct arm_smmu_master *master = dev_iommu_priv_get(dev);
> @@ -2893,10 +2863,75 @@ static void arm_smmu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
> arm_smmu_sva_remove_dev_pasid(domain, dev, pasid);
> }
>
> +static struct iommu_domain *
> +__arm_smmu_domain_alloc(unsigned type,
> + struct arm_smmu_domain *s2,
I think you should rather introduce s2 param in "iommu/arm-smmu-v3:
Support IOMMU_DOMAIN_NESTED type of allocations" because it is not use
at all in this patch nor really explained in the commit msg

Thanks

Eric
> + 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;
> +
> + mutex_init(&smmu_domain->init_mutex);
> + INIT_LIST_HEAD(&smmu_domain->devices);
> + spin_lock_init(&smmu_domain->devices_lock);
> + INIT_LIST_HEAD(&smmu_domain->mmu_notifiers);
> +
> + if (master) {
> + smmu_domain->smmu = master->smmu;
> + ret = arm_smmu_domain_finalise(domain, master, user_cfg);
> + if (ret) {
> + kfree(smmu_domain);
> + return NULL;
> + }
> + }
> +
> + return domain;
> +}
> +
> +static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
> +{
> + return __arm_smmu_domain_alloc(type, NULL, NULL, NULL);
> +}
> +
> +static struct iommu_domain *
> +arm_smmu_domain_alloc_user(struct device *dev, struct iommu_domain *parent,
> + const void *user_data)
> +{
> + const struct iommu_hwpt_arm_smmuv3 *user_cfg = user_data;
> + struct arm_smmu_master *master = dev_iommu_priv_get(dev);
> + unsigned type = IOMMU_DOMAIN_UNMANAGED;
> +
> + return __arm_smmu_domain_alloc(type, NULL, master, user_cfg);
> +}
> +
> static struct iommu_ops arm_smmu_ops = {
> .capable = arm_smmu_capable,
> .hw_info = arm_smmu_hw_info,
> .domain_alloc = arm_smmu_domain_alloc,
> + .domain_alloc_user = arm_smmu_domain_alloc_user,
> .get_unmanaged_domain = arm_smmu_get_unmanaged_domain,
> .probe_device = arm_smmu_probe_device,
> .release_device = arm_smmu_release_device,