Re: [PATCH v5 08/27] iommu/arm-smmu-v3: Move allocation of the cdtable into arm_smmu_get_cd_ptr()

[Mostafa Saleh <smostafa@google.com>]

Hi Jason,

On Mon, Mar 04, 2024 at 07:43:56PM -0400, Jason Gunthorpe wrote:
> No reason to force callers to do two steps. Make arm_smmu_get_cd_ptr()
> able to return an entry in all cases except OOM

I believe the current code is more clear, as it is explicit about which path
is expected to allocate.

As there are many callers for arm_smmu_get_cd_ptr() directly and indirectly,
and it read-modify-writes the cdtable, it would be a pain to debug not
knowing which one could allocate, and this patch only abstracts one
allocating call, so it is not much code less.

For example, (again I don’t know much about SVA) I think there might be a
race condition as follows:
arm_smmu_attach_dev
	arm_smmu_domain_finalise() => set domain stage
	[....]
	arm_smmu_get_cd_ptr() => RMW master->cd_table

arm_smmu_sva_set_dev_pasid
	__arm_smmu_sva_bind
		Check stage is valid
		[...]
		arm_smmu_write_ctx_desc
			arm_smmu_get_cd_ptr => RMW master->cd_table

If this path is true though, I guess the in current code, we would need some
barriers in arm_smmu_get_cd_ptr(), arm_smmu_get_cd_ptr()

> Tested-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 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 e25dbb982feeee..2dd6cb17112e98 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -106,6 +106,7 @@ static struct arm_smmu_option_prop arm_smmu_options[] = {
>  
>  static int arm_smmu_domain_finalise(struct arm_smmu_domain *smmu_domain,
>  				    struct arm_smmu_device *smmu);
> +static int arm_smmu_alloc_cd_tables(struct arm_smmu_master *master);
>  
>  static void parse_driver_options(struct arm_smmu_device *smmu)
>  {
> @@ -1231,6 +1232,11 @@ struct arm_smmu_cd *arm_smmu_get_cd_ptr(struct arm_smmu_master *master,
>  	struct arm_smmu_device *smmu = master->smmu;
>  	struct arm_smmu_ctx_desc_cfg *cd_table = &master->cd_table;
>  
> +	if (!master->cd_table.cdtab) {
> +		if (arm_smmu_alloc_cd_tables(master))
> +			return NULL;
> +	}
> +
>  	if (cd_table->s1fmt == STRTAB_STE_0_S1FMT_LINEAR)
>  		return (struct arm_smmu_cd *)(cd_table->cdtab +
>  					      ssid * CTXDESC_CD_DWORDS);
> @@ -2719,12 +2725,6 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  		struct arm_smmu_cd target_cd;
>  		struct arm_smmu_cd *cdptr;
>  
> -		if (!master->cd_table.cdtab) {
> -			ret = arm_smmu_alloc_cd_tables(master);
> -			if (ret)
> -				goto out_list_del;
> -		}
> -
>  		cdptr = arm_smmu_get_cd_ptr(master, IOMMU_NO_PASID);
>  		if (!cdptr) {
>  			ret = -ENOMEM;
> -- 
> 2.43.2
>
Thanks,
Mostafa