linux-kernel - Re: [RFT PATCH v3 08/27] asm-generic/io.h: Add a non-posted variant of ioremap()

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: <03f75e1f-3b3a-95a7-0298-c616dfed54ec@marcan.st>
Date:   Sat, 6 Mar 2021 00:19:34 +0900
From:   Hector Martin <marcan@...can.st>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     linux-arm Mailing List <linux-arm-kernel@...ts.infradead.org>,
        Marc Zyngier <maz@...nel.org>, Rob Herring <robh@...nel.org>,
        Arnd Bergmann <arnd@...nel.org>,
        Olof Johansson <olof@...om.net>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Mark Kettenis <mark.kettenis@...all.nl>,
        Tony Lindgren <tony@...mide.com>,
        Mohamed Mediouni <mohamed.mediouni@...amail.com>,
        Stan Skowronek <stan@...ellium.com>,
        Alexander Graf <graf@...zon.com>,
        Will Deacon <will@...nel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Mark Rutland <mark.rutland@....com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jonathan Corbet <corbet@....net>,
        Catalin Marinas <catalin.marinas@....com>,
        Christoph Hellwig <hch@...radead.org>,
        "David S. Miller" <davem@...emloft.net>,
        devicetree <devicetree@...r.kernel.org>,
        "open list:SERIAL DRIVERS" <linux-serial@...r.kernel.org>,
        Linux Documentation List <linux-doc@...r.kernel.org>,
        Linux Samsung SOC <linux-samsung-soc@...r.kernel.org>,
        Linux-Arch <linux-arch@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [RFT PATCH v3 08/27] asm-generic/io.h: Add a non-posted variant
 of ioremap()

On 05/03/2021 23.45, Andy Shevchenko wrote:
> On Thu, Mar 4, 2021 at 11:40 PM Hector Martin <marcan@...can.st> wrote:
>>
>> ARM64 currently defaults to posted MMIO (nGnRnE), but some devices
>> require the use of non-posted MMIO (nGnRE). Introduce a new ioremap()
>> variant to handle this case. ioremap_np() is aliased to ioremap() by
>> default on arches that do not implement this variant.
> 
> Hmm... But isn't it basically a requirement to those device drivers to
> use readX()/writeX() instead of readX_relaxed() / writeX_relaxed()?

No, the write ops used do not matter. It's just that on these Apple SoCs 
the fabric requires the mappings to be nGnRnE, else it just throws 
SErrors on all writes and ignores them.

The difference between _relaxed and not is barrier behavior with regards 
to DMA/memory accesses; this applies regardless of whether the writes 
are E or nE. You can have relaxed accesses with nGnRnE and then you 
would still have race conditions if you do not have a barrier between 
the MMIO and accessing DMA memory. What nGnRnE buys you (on 
platforms/buses where it works properly) is that you do need a dummy 
read after a write to ensure completion.

All of this is to some extent moot on these SoCs; it's not that we need 
the drivers to use nGnRnE for some correctness reason, it's that the 
SoCs force us to use it or else everything breaks, which was the 
motivation for this change. But since on most other SoCs both are valid 
options, this does allow some other drivers/platforms to opt into nGnRnE 
if they have a good reason to do so.

Though you just made me notice two mistakes in the commit description: 
first, it describes the old v2 version, for v3 I made ioremap_np() just 
return NULL on arches that don't implement it. Second, nGnRnE and nGnRE 
are backwards. Oops. I'll fix it for the next version.

>>   #define IORESOURCE_MEM_32BIT           (3<<3)
>>   #define IORESOURCE_MEM_SHADOWABLE      (1<<5)  /* dup: IORESOURCE_SHADOWABLE */
>>   #define IORESOURCE_MEM_EXPANSIONROM    (1<<6)
>> +#define IORESOURCE_MEM_NONPOSTED       (1<<7)
> 
> Not sure it's the right location (in a bit field) for this flag.

Do you have a better suggestion? It seemed logical to put it here, as a 
flag on memory-type I/O resources.

-- 
Hector Martin (marcan@...can.st)
Public Key: https://mrcn.st/pub