linux-kernel - Re: [PATCH 2/6] arm64: dts: qcom: sm8250: add apps

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, 25 May 2020 07:08:04 -0400
From:   Jonathan Marek <jonathan@...ek.ca>
To:     Sai Prakash Ranjan <saiprakash.ranjan@...eaurora.org>
Cc:     linux-arm-msm@...r.kernel.org, Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Rob Herring <robh+dt@...nel.org>, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-msm-owner@...r.kernel.org
Subject: Re: [PATCH 2/6] arm64: dts: qcom: sm8250: add apps_smmu node

On 5/25/20 6:54 AM, Sai Prakash Ranjan wrote:
> On 2020-05-25 15:39, Jonathan Marek wrote:
>> Hi,
>>
>> On 5/25/20 5:42 AM, Sai Prakash Ranjan wrote:
>>> Hi Jonathan,
>>>
>>> On 2020-05-24 08:08, Jonathan Marek wrote:
>>>> Add the apps_smmu node for sm8250. Note that adding the iommus field 
>>>> for
>>>> UFS is required because initializing the iommu removes the bypass 
>>>> mapping
>>>> that created by the bootloader.
>>>>
>>>
>>> This statement doesn't seem right, you can just say since the bypass 
>>> is disabled
>>> by default now, we need to add this property to enable translation 
>>> and avoid global faults.
>>>
>>
>> If I use this patch [1] then the UFS iommu property isn't needed. In
>> downstream, the identity (bypass?) stream mapping is inherited from
>> the bootloader, and UFS is used without any iommu property. Setting
>> ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=n doesn't make it work on its own
>> (without the UFS iommu property), so there's more to it than just
>> "bypass is disabled by default now".
>>
>> https://patchwork.kernel.org/patch/11310757/
>>
> 
> "iommus" property is not about inheriting stream mapping from bootloader,
> it is used to enable SMMU address translation for the corresponding
> master when specified. So when you have disabled bypass, i.e.,
> ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=y or via cmdline 
> "arm-smmu.disable_bypass=1"
> and iommus property with SID and mask is not specified, then it will result
> in SMMU global faults.
> 
> Downstream has bypass enabled(ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=n),so you
> won't see any global faults if you do not have iommus property.
> 
> Patch in your link is for display because of the usecase for splash screen
> on android and some other devices where the bootloader will configure SMMU,
> it has not yet merged and not likely to get merged in the current state.
> 
> So yes "there is *not* much more to it than bypass is disabled by 
> default now"
> and you have to specify "iommus" for the master devices or you should 
> enable bypass,
> i.e., ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=n or arm-smmu.disable_bypass=n
> 
> Try without the patch in the link and without iommus for UFS and
> ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=y and you will see.
> 
> -Sai

I know that the "iommus" property is not about inheriting stream 
mapping. Probing the iommu removes the stream mapping created by the 
bootloader, the iommus property is added so that new mappings are 
created to replace what was removed.

You seem to be under the impression that the SM8150/SM8250 bootloader 
does not configure SMMU. It does, for both UFS and SDHC, just like it 
does for display/splash screen on some devices.

With either value of ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT, it will not 
work without the iommus property.