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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e5ee0dad-3b52-3bf2-a473-f624415cdf67@arm.com>
Date:   Mon, 19 Oct 2020 15:02:04 +0100
From:   Robin Murphy <robin.murphy@....com>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>,
        Will Deacon <will@...nel.org>, Joerg Roedel <joro@...tes.org>,
        Sai Prakash Ranjan <saiprakash.ranjan@...eaurora.org>,
        Jordan Crouse <jcrouse@...eaurora.org>,
        Thierry Reding <treding@...dia.com>,
        Rob Clark <robdclark@...omium.org>
Cc:     linux-arm-kernel@...ts.infradead.org,
        iommu@...ts.linux-foundation.org, linux-kernel@...r.kernel.org,
        linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v4 1/3] iommu/arm-smmu: Allow implementation specific
 write_s2cr

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@...aro.org>
> ---
> 
> 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@....com>

> +	} 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
>