From: Toshi Kani <toshi.kani@hpe.com>
To: "Luis R. Rodriguez" <mcgrof@kernel.org>,
Julia Lawall <julia.lawall@lip6.fr>
Cc: Ingo Molnar <mingo@kernel.org>, Toshi Kani <toshi.kani@hp.com>,
Paul McKenney <paulmck@linux.vnet.ibm.com>,
Dave Airlie <airlied@redhat.com>,
Benjamin Herrenschmidt <benh@kernel.crashing.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
linux-arch@vger.kernel.org, X86 ML <x86@kernel.org>,
Daniel Vetter <daniel.vetter@intel.com>,
Thomas Gleixner <tglx@linutronix.de>,
"H. Peter Anvin" <hpa@zytor.com>,
Peter Zijlstra <a.p.zijlstra@chello.nl>,
Borislav Petkov <bp@alien8.de>,
Linus Torvalds <torvalds@linux-foundation.org>,
Andrew Morton <akpm@linux-foundation.org>,
Andy Lutomirski <luto@kernel.org>,
Brian Gerst <brgerst@gmail.com>
Subject: Re: Overlapping ioremap() calls, set_memory_*() semantics
Date: Thu, 17 Mar 2016 16:44:53 -0600 [thread overview]
Message-ID: <1458254693.6393.506.camel@hpe.com> (raw)
In-Reply-To: <20160316014548.GK1990@wotan.suse.de>
On Wed, 2016-03-16 at 02:45 +0100, Luis R. Rodriguez wrote:
> On Fri, Mar 11, 2016 at 03:13:52PM -0700, Toshi Kani wrote:
> > On Wed, 2016-03-09 at 10:15 +0100, Ingo Molnar wrote:
> > > * Toshi Kani <toshi.kani@hpe.com> wrote:
> > >
> > > > On Tue, 2016-03-08 at 13:16 +0100, Ingo Molnar wrote:
> > > > > * Toshi Kani <toshi.kani@hpe.com> wrote:
> > > > >
:
> > > > Did you mean 'aliased' or 'aliased with different cache attribute'?
> > > > The former check might be too strict.
> > >
> > > I'd say even 'same attribute' aliasing is probably relatively rare.
> > >
> > > And 'different but compatible cache attribute' is in fact more of a
> > > sign that the driver author does the aliasing for a valid _reason_:
> > > to have two different types of access methods to the same piece of
> > > physical address space...
> >
> > Right. So, if we change to fail ioremap() on aliased cases, it'd be
> > easier to start with the different attribute case first. This case
> > should be rare enough that we can manage to identify such callers and
> > make them use a new API as necessary. If we go ahead to fail any
> > aliased cases, it'd be challenging to manage without a regression or
> > two.
>
> From my experience on the ioremap_wc() crusade, I found that the need for
> aliasing with different cache types would have been needed in only 3
> drivers. For these 3, the atyfb driver I did the proper split in MMIO and
> framebuffer, but that was significant work. I did this work to demo and
> document such work. It wasn't easy. For other two, ivtv and ipath we left
> as requiring "nopat" to be used. The ipath driver is on its way out of
> the kenrel now through staging, and ivtv, well I am not aware of single
> human being claiming to use it. The architecture of ivtv actually
> prohibits us from ever using PAT for write-combining on the framebuffer
> as the firmware is the only one who knows the write-combining area and
> hides it from us.
At glace, there are 863 references to ioremap(), 329 references to
ioremap_nocache(), and only 68 references to ioremap_wc() on x86. There
are many more ioremap callers with UC mappings than WC mappings, and it is
hard to say that they never get aliased.
> We might be able to use tools like Coccinelle to perhaps hunt for
> the use of aliasing on drivers with different cache attribute types
> to do a full assessment but I really think that will be really hard
> to accomplish.
>
> If we can learn anything from the ioremap_wc() crusade I'd say its that
> the need for aliasing with different cache types obviously implies we
> should disable such drivers with PAT as what we'd really need is a proper
> split in maps, but history shows the split can be really hard. It sounded
> like you guys were confirming we currently do not allow for aliasing with
> different attributes on x86, is that the case for all architectures?
>
> If aliasing with different cache attributes is not allowed for x86 and
> if its also rare for other architectures that just leaves the hunt for
> valid aliasing uses. That still may be hard to hunt for, but I also
> suspect it may be rare.
Yes, I'd fail the different cache attribute case if we are to place more
strict check.
:
> >
> > I think the "set_memory_" prefix implies that their target is regular
> > memory only.
>
> I did not find any driver using set_memory_wc() on MMIO, its a good thing
> as that does not work it seems even if it returns no error. I'm not sure
> of the use of other set_memory_*() on MMIO but I would suspect its not
> used. A manual hunt may suffice to rule these out.
It's good to know that you did not find any case on MMIO. The thing is,
set_memory_wc() actually works on MMIO today... This is because __pa()
returns a bogus address, which skips the alias check in the memtype.
> I guess what I'm trying to say is I am not sure we have a need for
> set_cache_attr_*() APIs, unless of course we find such valid use.
>
> > > And at that point we could definitely argue that set_cache_attr_*()
> > > APIs should probably generate a warning for _RAM_, because they
> > > mostly make sense for MMIO type of physical addresses, right? Regular
> > > RAM should always be WB.
> > >
> > > Are there cases where we change the caching attribute of RAM for
> > > valid reasons, outside of legacy quirks?
> >
> > ati_create_page_map() is one example that it gets a RAM page
> > by __get_free_page(), and changes it to UC by calling set_memory_uc().
>
> Should we instead have an API that lets it ask for RAM and of UC type?
> That would seem a bit cleaner. BTW do you happen to know *why* it needs
> UC RAM types?
This RAM page is then shared between graphic card and CPU. I think this is
because graphic card cannot snoop the cache.
> >
:
> > > > - 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.
> > >
> > > That looks like a bug?
> >
> > This is by design since set_memory_xx was introduced for RAM only. If
> > we extend it to MMIO, then we need to change how memtype manages MMIO.
>
> I'd be afraid to *want* to support this on MMIO as I would only expect
> hacks from drivers.
Agreed, with the hope that they are not used on MMIO already...
Thanks,
-Toshi
next prev parent reply other threads:[~2016-03-17 21:52 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-03-03 21:28 Overlapping ioremap() calls, set_memory_*() semantics Luis R. Rodriguez
2016-03-03 21:28 ` Luis R. Rodriguez
2016-03-04 9:44 ` Ingo Molnar
2016-03-04 18:18 ` Toshi Kani
2016-03-04 18:18 ` Toshi Kani
2016-03-04 18:51 ` Luis R. Rodriguez
2016-03-04 21:39 ` Toshi Kani
2016-03-05 11:42 ` Ingo Molnar
2016-03-05 11:40 ` Ingo Molnar
2016-03-07 17:03 ` Toshi Kani
2016-03-07 17:03 ` Toshi Kani
2016-03-08 12:16 ` Ingo Molnar
2016-03-09 0:29 ` Toshi Kani
2016-03-09 9:15 ` Ingo Molnar
2016-03-11 22:13 ` Toshi Kani
2016-03-16 1:45 ` Luis R. Rodriguez
2016-03-16 1:45 ` Luis R. Rodriguez
2016-03-17 22:44 ` Toshi Kani [this message]
2016-04-13 21:16 ` Luis R. Rodriguez
2016-04-15 14:47 ` Toshi Kani
2016-04-15 14:47 ` Toshi Kani
2016-04-16 9:20 ` Ingo Molnar
2016-04-16 9:20 ` Ingo Molnar
2016-03-21 17:38 ` Maciej W. Rozycki
2016-04-13 21:03 ` Luis R. Rodriguez
2016-03-11 6:47 ` Andy Lutomirski
2016-03-11 22:36 ` Toshi Kani
2016-03-13 1:02 ` Andy Lutomirski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1458254693.6393.506.camel@hpe.com \
--to=toshi.kani@hpe.com \
--cc=a.p.zijlstra@chello.nl \
--cc=airlied@redhat.com \
--cc=akpm@linux-foundation.org \
--cc=benh@kernel.crashing.org \
--cc=bp@alien8.de \
--cc=brgerst@gmail.com \
--cc=daniel.vetter@intel.com \
--cc=hpa@zytor.com \
--cc=julia.lawall@lip6.fr \
--cc=linux-arch@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@kernel.org \
--cc=mcgrof@kernel.org \
--cc=mingo@kernel.org \
--cc=paulmck@linux.vnet.ibm.com \
--cc=tglx@linutronix.de \
--cc=torvalds@linux-foundation.org \
--cc=toshi.kani@hp.com \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).