Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751650AbcCHXhS (ORCPT ); Tue, 8 Mar 2016 18:37:18 -0500 Received: from g9t5009.houston.hp.com ([15.240.92.67]:35118 "EHLO g9t5009.houston.hp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750888AbcCHXhO (ORCPT ); Tue, 8 Mar 2016 18:37:14 -0500 Message-ID: <1457483385.15454.519.camel@hpe.com> Subject: Re: Overlapping ioremap() calls, set_memory_*() semantics From: Toshi Kani To: Ingo Molnar Cc: "Luis R. Rodriguez" , Toshi Kani , Paul McKenney , Dave Airlie , Benjamin Herrenschmidt , "linux-kernel@vger.kernel.org" , linux-arch@vger.kernel.org, X86 ML , Daniel Vetter , Thomas Gleixner , "H. Peter Anvin" , Peter Zijlstra , Borislav Petkov , Linus Torvalds , Andrew Morton , Andy Lutomirski , Brian Gerst Date: Tue, 08 Mar 2016 17:29:45 -0700 In-Reply-To: <20160308121601.GA6573@gmail.com> References: <20160304094424.GA16228@gmail.com> <1457115514.15454.216.camel@hpe.com> <20160305114012.GA7259@gmail.com> <1457370228.15454.311.camel@hpe.com> <20160308121601.GA6573@gmail.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.18.4 (3.18.4-1.fc23) Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4258 Lines: 98 On Tue, 2016-03-08 at 13:16 +0100, Ingo Molnar wrote: > * Toshi Kani wrote: > > > > So where is the problem? The memtype implementation and hence most > > > ioremap() users are supposed to be safe. set_memory_*() APIs are > > > supposed > > > to be safe as well, as they too go via the memtype API. > > > > Let me try to summarize... > > > > The original issue Luis brought up was that drivers written to work > > with MTRR may create a single ioremap range covering multiple cache > > attributes since MTRR can overwrite cache attribute of a certain range. > >  Converting such drivers with PAT-based ioremap interfaces, i.e. > > ioremap_wc() and ioremap_nocache(), requires a separate ioremap map for > > each cache attribute, which can be challenging as it may result in > > overlapping ioremap ranges (in his term) with different cache > > attributes. > > > > So, Luis asked about 'sematics of overlapping ioremap()' calls.  Hence, > > I responded that aliasing mapping itself is supported, but alias with > > different cache attribute is not.  We have checks in place to detect > > such condition.  Overlapping ioremap calls with a different cache > > attribute either fails or gets redirected to the existing cache > > attribute on x86. > > Ok, fair enough! > > So to go back to the original suggestion from Luis, I've quoted it, but > with a s/overlapping/aliased substitution: > > > I had suggested long ago then that one possible resolution was for us > > to add an API that *enables* aliased ioremap() calls, and only use it > > on select locations in the kernel. This means we only have to convert a > > few users to that call to white list such semantics, and by default > > we'd disable aliased calls. To kick things off -- is this strategy > > agreeable for all other architectures? > > I'd say that since the overwhelming majority of ioremap() calls are not > aliased, ever, thus making it 'harder' to accidentally alias is probably > a good idea. Did you mean 'aliased' or 'aliased with different cache attribute'?  The former check might be too strict. > The memtype infrastructure of phyisical memory ranges in that case acts > as a security measure, to avoid unintended (not just physically > incompatible) aliasing. I suspect it would be helpful during driver > development as well. The memtype infrastructure does not track caller interfaces.  So, it will check against to any map, i.e. kernel & user map.  I assume a kernel map is created before user map, though. > What extra API are you thinking about to enable aliasing in the few cases > where it's justified? I'll defer this for Luis... > the other problem listed: > > > As another problem case, set_memor_*() will not fail on MMIO even > > though set_memor_*() is designed only for RAM. > > So what does this mean exactly? Having WB caching on MMIO area is not > good, but UC, WC and WB sure is still sensible in some cases, right? I responded to Luis in other email that: | Drivers use ioremap family with a right cache type when mapping MMIO | ranges, ex. ioremap_wc().  They do not need to change the type to MMIO. | RAM is different since it's already mapped with WB at boot-time. | set_memory_*() allows us to change the type from WB, and put it back to | WB. > > [...] If the above strategy on avoiding aliasing is agreeable, could > > the next step, or an orthogonal step be to error out on set_memory_*() > > on IO memory? > > Well, do we have drivers that nevertheless change caching attributes on > MMIO areas? Not sure.  We will need to check all callers of set_memory_xx() if we change it to fail on MMIO. > Basically if ioremap_uc() and ioremap_wc() is allowed on MMIO areas, then > I see no reason in principle why it should be invalid to change the area > from UC to WC after it has been ioremap()ed. The current implementation does not support MMIO.  - It does not track cache attribute correctly for MMIO since it uses __pa().  - It only supports attribute transition of {WB -> NewType -> WB} for RAM.  RAM is tracked differently that WB is treated as "no map".  So, this transition does not cause a conflict on RAM.  This will causes a conflict on MMIO when it is tracked correctly.    Thanks, -Toshi