linux-kernel - Re: [Freedreno] [PATCH v7 6/6] drm/msm: iommu: Replace runtime calls with runtime suppliers

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 Feb 2018 08:30:35 -0500
From:   Rob Clark <robdclark@...il.com>
To:     Tomasz Figa <tfiga@...omium.org>
Cc:     Robin Murphy <robin.murphy@....com>,
        Mark Rutland <mark.rutland@....com>,
        devicetree@...r.kernel.org, Jordan Crouse <jcrouse@...eaurora.org>,
        Linux PM <linux-pm@...r.kernel.org>,
        David Airlie <airlied@...ux.ie>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Joerg Roedel <joro@...tes.org>,
        Will Deacon <will.deacon@....com>,
        "list@....net:IOMMU DRIVERS" <iommu@...ts.linux-foundation.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Vivek Gautam <vivek.gautam@...eaurora.org>,
        Greg KH <gregkh@...uxfoundation.org>,
        freedreno <freedreno@...ts.freedesktop.org>,
        Stephen Boyd <sboyd@...eaurora.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>
Subject: Re: [Freedreno] [PATCH v7 6/6] drm/msm: iommu: Replace runtime calls
 with runtime suppliers

On Thu, Feb 22, 2018 at 3:13 AM, Tomasz Figa <tfiga@...omium.org> wrote:
> On Fri, Feb 16, 2018 at 9:13 AM, Tomasz Figa <tfiga@...omium.org> wrote:
>> On Fri, Feb 16, 2018 at 2:14 AM, Robin Murphy <robin.murphy@....com> wrote:
>>> On 15/02/18 04:17, Tomasz Figa wrote:
>>> [...]
>>>>>
>>>>> Could you elaborate on what kind of locking you are concerned about?
>>>>> As I explained before, the normally happening fast path would lock
>>>>> dev->power_lock only for the brief moment of incrementing the runtime
>>>>> PM usage counter.
>>>>
>>>>
>>>> My bad, that's not even it.
>>>>
>>>> The atomic usage counter is incremented beforehands, without any
>>>> locking [1] and the spinlock is acquired only for the sake of
>>>> validating that device's runtime PM state remained valid indeed [2],
>>>> which would be the case in the fast path of the same driver doing two
>>>> mappings in parallel, with the master powered on (and so the SMMU,
>>>> through device links; if master was not powered on already, powering
>>>> on the SMMU is unavoidable anyway and it would add much more latency
>>>> than the spinlock itself).
>>>
>>>
>>> We now have no locking at all in the map path, and only a per-domain lock
>>> around TLB sync in unmap which is unfortunately necessary for correctness;
>>> the latter isn't too terrible, since in "serious" hardware it should only be
>>> serialising a few cpus serving the same device against each other (e.g. for
>>> multiple queues on a single NIC).
>>>
>>> Putting in a global lock which serialises *all* concurrent map and unmap
>>> calls for *all* unrelated devices makes things worse. Period. Even if the
>>> lock itself were held for the minimum possible time, i.e. trivially
>>> "spin_lock(&lock); spin_unlock(&lock)", the cost of repeatedly bouncing that
>>> one cache line around between 96 CPUs across two sockets is not negligible.
>>
>> Fair enough. Note that we're in a quite interesting situation now:
>>  a) We need to have runtime PM enabled on Qualcomm SoC to have power
>> properly managed,
>>  b) We need to have lock-free map/unmap on such distributed systems,
>>  c) If runtime PM is enabled, we need to call into runtime PM from any
>> code that does hardware accesses, otherwise the IOMMU API (and so DMA
>> API and then any V4L2 driver) becomes unusable.
>>
>> I can see one more way that could potentially let us have all the
>> three. How about enabling runtime PM only on selected implementations
>> (e.g. qcom,smmu) and then having all the runtime PM calls surrounded
>> with if (pm_runtime_enabled()), which is lockless?
>>
>
> Sorry for pinging, but any opinion on this kind of approach?
>

It is ok by me, for whatever that is worth

BR,
-R