linux-kernel - Re: [PATCH v4 2/3] iommu/arm-smmu-qcom: Read back stream mappings

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]

Date:   Mon, 19 Oct 2020 15:03:15 +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 2/3] iommu/arm-smmu-qcom: Read back stream mappings

On 2020-10-17 05:39, Bjorn Andersson wrote:
> The Qualcomm boot loader configures stream mapping for the peripherals
> that it accesses and in particular it sets up the stream mapping for the
> display controller to be allowed to scan out a splash screen or EFI
> framebuffer.
> 
> Read back the stream mappings during initialization and make the
> arm-smmu driver maintain the streams in bypass mode.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@...aro.org>
> ---
> 
> Changes since v3:
> - Extracted from different patch in v3.
> - Now configures the stream as BYPASS, rather than translate, which should work
>    for platforms with working S2CR handling as well.
> 
>   drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 24 ++++++++++++++++++++++
>   1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> index be4318044f96..0089048342dd 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> @@ -23,6 +23,29 @@ static const struct of_device_id qcom_smmu_client_of_match[] __maybe_unused = {
>   	{ }
>   };
>   
> +static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
> +{
> +	u32 smr;
> +	int i;
> +
> +	for (i = 0; i < smmu->num_mapping_groups; i++) {
> +		smr = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_SMR(i));
> +
> +		if (FIELD_GET(ARM_SMMU_SMR_VALID, smr)) {
> +			smmu->smrs[i].id = FIELD_GET(ARM_SMMU_SMR_ID, smr);
> +			smmu->smrs[i].mask = FIELD_GET(ARM_SMMU_SMR_MASK, smr);
> +			smmu->smrs[i].valid = true;
> +
> +			smmu->s2crs[i].type = S2CR_TYPE_BYPASS;
> +			smmu->s2crs[i].privcfg = S2CR_PRIVCFG_DEFAULT;
> +			smmu->s2crs[i].cbndx = 0xff;
> +			smmu->s2crs[i].count++;

FWIW I don't think you actually need to adjust the count here - the SMR 
being valid should already prevent the whole SME from being reassigned 
until the display probes, at which point it should "take over" the SMR 
based on matching values and claim the "initial" refcount. After that 
you're back into the standard flow. It might be a little unintuitive to 
have something in a valid but "unused" state, but arguably it's entirely 
accurate in terms of the software abstraction here.

Otherwise, you end up making boot-time SMRs - so potentially all SMRs 
after a kexec - effectively immutable, since even after Linux has taken 
control of the whole system such that they *could* be reassigned safely, 
there's still this undroppable refcount hanging around preventing it.

That said, for a mobile SoC use-case if you have enough SMRs for all 
your stream IDs and don't have any kind of device hotplug, that 
restriction shouldn't make much difference in practice, so I'm not too 
concerned either way. Otherwise this is as nice and tidy as I'd hoped :)

Robin.

> +		}
> +	}
> +
> +	return 0;
> +}
> +
>   static int qcom_smmu_def_domain_type(struct device *dev)
>   {
>   	const struct of_device_id *match =
> @@ -61,6 +84,7 @@ static int qcom_smmu500_reset(struct arm_smmu_device *smmu)
>   }
>   
>   static const struct arm_smmu_impl qcom_smmu_impl = {
> +	.cfg_probe = qcom_smmu_cfg_probe,
>   	.def_domain_type = qcom_smmu_def_domain_type,
>   	.reset = qcom_smmu500_reset,
>   };
>