Re: [PATCH v5 11/17] iommu/arm-smmu-v3: Remove arm_smmu_master->domain

Mostafa Saleh
2024-02-13T15:45:56Z

Hi Jason,

On Tue, Feb 06, 2024 at 11:12:48AM -0400, Jason Gunthorpe wrote:
> Introducing global statics which are of type struct iommu_domain, not
> struct arm_smmu_domain makes it difficult to retain
> arm_smmu_master->domain, as it can no longer point to an IDENTITY or
> BLOCKED domain.
>
> The only place that uses the value is arm_smmu_detach_dev(). Change things
> to work like other drivers and call iommu_get_domain_for_dev() to obtain
> the current domain.
>
> The master->domain is subtly protecting the domain_head against being
> unused, change the domain_head to be INIT'd when the master is not
> attached to a domain instead of garbage/zero.

I don't this the problem here, neither the reason for initialising the
domain_head, can you please clarify the issue?

> Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> Tested-by: Nicolin Chen <nicolinc@nvidia.com>
> Tested-by: Moritz Fischer <moritzf@google.com>
> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 26 ++++++++-------------
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 -
> 2 files changed, 10 insertions(+), 17 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 133f13f33df124..a98707cd1efccb 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2560,19 +2560,20 @@ static void arm_smmu_disable_pasid(struct arm_smmu_master *master)
>
> static void arm_smmu_detach_dev(struct arm_smmu_master *master)
> {
> + struct iommu_domain *domain = iommu_get_domain_for_dev(master->dev);
> + struct arm_smmu_domain *smmu_domain;
> unsigned long flags;
> - struct arm_smmu_domain *smmu_domain = master->domain;
>
> - if (!smmu_domain)
> + if (!domain)
> return;
>
> + smmu_domain = to_smmu_domain(domain);
> arm_smmu_disable_ats(master, smmu_domain);
>
> spin_lock_irqsave(&smmu_domain->devices_lock, flags);
> - list_del(&master->domain_head);
> + list_del_init(&master->domain_head);
> spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
>
> - master->domain = NULL;
> master->ats_enabled = false;
> }
>
> @@ -2626,8 +2627,6 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>
> arm_smmu_detach_dev(master);
>
> - master->domain = smmu_domain;
> -
> /*
> * The SMMU does not support enabling ATS with bypass. When the STE is
> * in bypass (STE.Config[2:0] == 0b100), ATS Translation Requests and
> @@ -2646,10 +2645,8 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
> case ARM_SMMU_DOMAIN_S1:
> if (!master->cd_table.cdtab) {
> ret = arm_smmu_alloc_cd_tables(master);
> - if (ret) {
> - master->domain = NULL;
> + if (ret)
> goto out_list_del;
> - }
> } else {
> /*
> * arm_smmu_write_ctx_desc() relies on the entry being
> @@ -2657,17 +2654,13 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
> */
> ret = arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID,
> NULL);
> - if (ret) {
> - master->domain = NULL;
> + if (ret)
> goto out_list_del;
> - }
> }
>
> ret = arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID, &smmu_domain->cd);
> - if (ret) {
> - master->domain = NULL;
> + if (ret)
> goto out_list_del;
> - }
>
> arm_smmu_make_cdtable_ste(&target, master);
> arm_smmu_install_ste_for_dev(master, &target);
> @@ -2693,7 +2686,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>
> out_list_del:
> spin_lock_irqsave(&smmu_domain->devices_lock, flags);
> - list_del(&master->domain_head);
> + list_del_init(&master->domain_head);
> spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
>
> out_unlock:
> @@ -2894,6 +2887,7 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev)
> master->dev = dev;
> master->smmu = smmu;
> INIT_LIST_HEAD(&master->bonds);
> + INIT_LIST_HEAD(&master->domain_head);
> dev_iommu_priv_set(dev, master);
>
> ret = arm_smmu_insert_master(smmu, master);
> 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 cbf4b57719b7b9..587f99701ad30f 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -696,7 +696,6 @@ struct arm_smmu_stream {
> struct arm_smmu_master {
> struct arm_smmu_device *smmu;
> struct device *dev;
> - struct arm_smmu_domain *domain;
> struct list_head domain_head;
> struct arm_smmu_stream *streams;
> /* Locked by the iommu core using the group mutex */
> --
> 2.43.0
>
Thanks,
Mostafa