linux-kernel - Re: [PATCH 1/2] iommu/exynos: Abstract getting the fault info

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:   Thu, 22 Dec 2022 14:20:16 +0100
From:   Marek Szyprowski <m.szyprowski@...sung.com>
To:     Sam Protsenko <semen.protsenko@...aro.org>
Cc:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>,
        Robin Murphy <robin.murphy@....com>,
        Janghyuck Kim <janghyuck.kim@...sung.com>,
        Cho KyongHo <pullip.cho@...sung.com>,
        Daniel Mentz <danielmentz@...gle.com>,
        David Virag <virag.david003@...il.com>,
        Sumit Semwal <sumit.semwal@...aro.org>, iommu@...ts.linux.dev,
        linux-arm-kernel@...ts.infradead.org,
        linux-samsung-soc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] iommu/exynos: Abstract getting the fault info

On 21.12.2022 22:32, Sam Protsenko wrote:
> Hi Marek,
>
> On Mon, 24 Oct 2022 at 09:43, Sam Protsenko <semen.protsenko@...aro.org> wrote:
>> Hi Marek,
>>
>> On Fri, 12 Aug 2022 at 14:25, Marek Szyprowski <m.szyprowski@...sung.com> wrote:
>>> Hi Sam,
>> [snip]
>>
>>>> Signed-off-by: Sam Protsenko <semen.protsenko@...aro.org>
>>> I'm not very happy with converting the sysmmu_fault_info arrays into the
>>> decoding functions. If I got the code right, adding v7 is still possible
>>> with the current approach. The main advantage of the array-based
>>> approach is readability and keeping all the information together in a
>>> single place.
>>>
>>> I agree for the items listed above as 'minor functional changes',
>>> though. Those sysmmu_fault_info arrays might be a part of sysmmu hw
>>> variant to avoid decoding hw version for each fault.
>>>
>>> I'm not sure that the linear scan is so problematic with regards to the
>>> performance. You really don't want your drivers to trigger IOMMU fault
>>> so often during normal operation. It is just a way to get some debugging
>>> information or handle some exception.
>>>
>>> You mentioned that the transaction type is read from the separate
>>> register in case of v7, but your code (here and in second patch) still
>>> relies on the reported interrupt bits.
>>>
>>> Could you try to rework all your changes in a such way, that the
>>> sysmmu_fault_info arrays are still used? V7 is really very similar to
>>> the v5 already supported by the current driver.
>>>
>> That's actually how I implemented this patch on my first attempt.
>> Really didn't like it, because a half of existing sysmmu_fault_info
>> structure doesn't make sense for v7, and some functionality of v7 has
>> to be implemented separately from that structure. I'd argue that
>> previous abstraction is just broken, and doesn't work for all SysMMU
>> versions anymore. It's easy to see how much difference between v5 and
>> v7, just by looking at corresponding get_fault_info() functions I
>> implemented. For example, the transaction type is probed from
>> different registers using different version, etc. There is also the
>> need to handle new VM/non-VM registers on v7. Also there is some extra
>> functionality that will be added later, like multiple translation
>> domains support, which is also quite different from how things done
>> for v5.
>>
>> I'd show more specifics to demonstrate my statements above, but alas I
>> already deleted my initial implementation (which was exactly what you
>> suggest). This callback-style HAL seems to be a perfect choice, and I
>> spent several days just experimenting with different approaches and
>> seeing all pros and cons. And from my point of view, this way is the
>> best for providing actual solid abstraction, which doesn't require
>> adding any workarounds on top of that. I understand that my patch
>> changes the very conception of how IRQ is handled in this driver, but
>> I'm still convinced it's a proper way to do that for all v1/v5/v7,
>> especially w.r.t. further v7 additions, to keep the abstraction solid.
>> Not that I'm lazy and don't want to rework things :) But in this
>> particular case I'd go with unchanged patches.
>>
>> Do you think it's reasonable to take this series as is? I can try and
>> collect more particular code snippets to demonstrate my point, if you
>> like.
>>
>> Thanks!
> So, what do you think about this?

Okay, go ahead with your approach. If I find a better way, I will rework 
it then. I would just like to have the code for fault handling for hw 
v1, v5 and v7 similar as much as possible.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland