Re: [PATCH v5 09/17] iommu/arm-smmu-v3: Put writing the context descriptor in the right order

From: Jason Gunthorpe <jgg@nvidia.com>
To: Mostafa Saleh <smostafa@google.com>
Cc: iommu@lists.linux.dev, Joerg Roedel <joro@8bytes.org>,
	linux-arm-kernel@lists.infradead.org,
	Robin Murphy <robin.murphy@arm.com>,
	Will Deacon <will@kernel.org>,
	Lu Baolu <baolu.lu@linux.intel.com>,
	Jean-Philippe Brucker <jean-philippe@linaro.org>,
	Joerg Roedel <jroedel@suse.de>, Moritz Fischer <mdf@kernel.org>,
	Moritz Fischer <moritzf@google.com>,
	Michael Shavit <mshavit@google.com>,
	Nicolin Chen <nicolinc@nvidia.com>,
	patches@lists.linux.dev,
	Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>,
	Zhangfei Gao <zhangfei.gao@linaro.org>
Subject: Re: [PATCH v5 09/17] iommu/arm-smmu-v3: Put writing the context descriptor in the right order
Date: Tue, 13 Feb 2024 13:50:42 -0400	[thread overview]
Message-ID: <20240213175042.GF1088888@nvidia.com> (raw)
In-Reply-To: <ZcuN_YZBzxJ2wvAg@google.com>

On Tue, Feb 13, 2024 at 03:42:53PM +0000, Mostafa Saleh wrote:
> > @@ -2659,6 +2651,17 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
> >  				master->domain = NULL;
> >  				goto out_list_del;
> >  			}
> > +		} else {
> > +			/*
> > +			 * arm_smmu_write_ctx_desc() relies on the entry being
> > +			 * invalid to work, clear any existing entry.
> > +			 */
> > +			ret = arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID,
> > +						      NULL);
> > +			if (ret) {
> > +				master->domain = NULL;
> > +				goto out_list_del;
> > +			}
> 
> Instead of having duplicate
>            if (ret) {
>                master->domain = NULL;
>                goto out_list_del;
>            }
> 
> In the if and the else, we can just move it outside.

Stylistically I often try to avoid shifting the error path from its
statement, but it is OK either way..

However, part 2 removes the need for error handling here entirely, so
let's leave it.

> > @@ -2668,15 +2671,23 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
> >  		}
> >  
> >  		arm_smmu_make_cdtable_ste(&target, master);
> > +		arm_smmu_install_ste_for_dev(master, &target);
> >  		break;
> >  	case ARM_SMMU_DOMAIN_S2:
> >  		arm_smmu_make_s2_domain_ste(&target, master, smmu_domain);
> > +		arm_smmu_install_ste_for_dev(master, &target);
> > +		if (master->cd_table.cdtab)
> > +			arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID,
> > +						      NULL);
> >  		break;
> >  	case ARM_SMMU_DOMAIN_BYPASS:
> >  		arm_smmu_make_bypass_ste(&target);
> > +		arm_smmu_install_ste_for_dev(master, &target);
> > +		if (master->cd_table.cdtab)
> > +			arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID,
> > +						      NULL);
> >  		break;
> >  	}
> This invalidates the CD while the STE is in bypass/S2 which is a new behavior
> to the driver, 

Yes

> I don’t see anything from the user manual about this, so I
> believe that is fine.

Nor do I. Nor can I see any reason why HW would care. We also
invalidate ASID's and VMID's after their tables have been removed from
the STE/CD too.

There are other options here if this is found out to be a trouble but
they are convoluted enough to not do them without a concrete reason.

Thanks,
Jason