Re: [PATCH v4 1/3] iommu/arm-smmu: Allow implementation specific write_s2cr

[Robin Murphy]

On 2020-10-17 05:39, Bjorn Andersson wrote:
> The firmware found in some Qualcomm platforms intercepts writes to the
> S2CR register in order to replace the BYPASS type with FAULT. Further
> more it treats faults at this level as catastrophic and restarts the
> device.
>
> Add support for providing implementation specific versions of the S2CR
> write function, to allow the Qualcomm driver to work around this
> behavior.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> ---
>
> Changes since v3:
> - New patch
>
>   drivers/iommu/arm/arm-smmu/arm-smmu.c | 22 ++++++++++++++--------
>   drivers/iommu/arm/arm-smmu/arm-smmu.h |  1 +
>   2 files changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> index dad7fa86fbd4..ed3f0428c110 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> @@ -929,14 +929,20 @@ static void arm_smmu_write_smr(struct arm_smmu_device *smmu, int idx)
>   static void arm_smmu_write_s2cr(struct arm_smmu_device *smmu, int idx)
>   {
>   	struct arm_smmu_s2cr *s2cr = smmu->s2crs + idx;
> -	u32 reg = FIELD_PREP(ARM_SMMU_S2CR_TYPE, s2cr->type) |
> -		  FIELD_PREP(ARM_SMMU_S2CR_CBNDX, s2cr->cbndx) |
> -		  FIELD_PREP(ARM_SMMU_S2CR_PRIVCFG, s2cr->privcfg);
> -
> -	if (smmu->features & ARM_SMMU_FEAT_EXIDS && smmu->smrs &&
> -	    smmu->smrs[idx].valid)
> -		reg |= ARM_SMMU_S2CR_EXIDVALID;
> -	arm_smmu_gr0_write(smmu, ARM_SMMU_GR0_S2CR(idx), reg);
> +	u32 reg;
> +
> +	if (smmu->impl && smmu->impl->write_s2cr) {
> +		smmu->impl->write_s2cr(smmu, idx);

Nit: just add an early return here to avoid reindenting the whole 
function. Otherwise this looks like a reasonable level of abstraction to 
me - we'll still have plenty of flexibility to adjust things in future 
if necessary.

With that change,

Reviewed-by: Robin Murphy <robin.murphy@xxxxxxx>

> +	} else {
> +		reg = FIELD_PREP(ARM_SMMU_S2CR_TYPE, s2cr->type) |
> +		      FIELD_PREP(ARM_SMMU_S2CR_CBNDX, s2cr->cbndx) |
> +		      FIELD_PREP(ARM_SMMU_S2CR_PRIVCFG, s2cr->privcfg);
> +
> +		if (smmu->features & ARM_SMMU_FEAT_EXIDS && smmu->smrs &&
> +		    smmu->smrs[idx].valid)
> +			reg |= ARM_SMMU_S2CR_EXIDVALID;
> +		arm_smmu_gr0_write(smmu, ARM_SMMU_GR0_S2CR(idx), reg);
> +	}
>   }
>   
>   static void arm_smmu_write_sme(struct arm_smmu_device *smmu, int idx)
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.h b/drivers/iommu/arm/arm-smmu/arm-smmu.h
> index 1a746476927c..b71647eaa319 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.h
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h
> @@ -436,6 +436,7 @@ struct arm_smmu_impl {
>   	int (*alloc_context_bank)(struct arm_smmu_domain *smmu_domain,
>   				  struct arm_smmu_device *smmu,
>   				  struct device *dev, int start);
> +	void (*write_s2cr)(struct arm_smmu_device *smmu, int idx);
>   };
>   
>   #define INVALID_SMENDX			-1