Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1171727 > unrolled thread

Re: [PATCH v7 5/9] PCI: Add pci_iomap_wc() variants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v7 5/9] PCI: Add pci_iomap_wc() variants "Luis R. Rodriguez" <mcgrof@suse.com> - 2015-06-25 00:30 +0200
    Re: [PATCH v7 5/9] PCI: Add pci_iomap_wc() variants Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2015-06-25 01:50 +0200
      Re: [PATCH v7 5/9] PCI: Add pci_iomap_wc() variants "Luis R. Rodriguez" <mcgrof@suse.com> - 2015-06-25 02:10 +0200
        Re: [Xen-devel] [PATCH v7 5/9] PCI: Add pci_iomap_wc() variants "Luis R. Rodriguez" <mcgrof@suse.com> - 2015-06-25 03:00 +0200
          Re: [Xen-devel] [PATCH v7 5/9] PCI: Add pci_iomap_wc() variants Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2015-06-25 03:20 +0200
        Re: [PATCH v7 5/9] PCI: Add pci_iomap_wc() variants Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2015-06-25 03:00 +0200
      RE: [PATCH v7 5/9] PCI: Add pci_iomap_wc() variants Casey Leedom <leedom@chelsio.com> - 2015-06-25 23:50 +0200
        Re: [PATCH v7 5/9] PCI: Add pci_iomap_wc() variants Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2015-06-26 01:00 +0200
          Re: [Xen-devel] [PATCH v7 5/9] PCI: Add pci_iomap_wc() variants Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2015-06-27 02:20 +0200
        Re: [PATCH v7 5/9] PCI: Add pci_iomap_wc() variants Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2015-06-26 04:50 +0200
          Re: [PATCH v7 5/9] PCI: Add pci_iomap_wc() variants Casey Leedom <leedom@chelsio.com> - 2015-07-03 02:20 +0200

#1171727 — Re: [PATCH v7 5/9] PCI: Add pci_iomap_wc() variants

On Wed, Jun 24, 2015 at 3:05 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Wed, 2015-06-24 at 18:38 +0200, Luis R. Rodriguez wrote:
>> On Wed, Jun 24, 2015 at 08:42:23AM +1000, Benjamin Herrenschmidt wrote:
>> > On Fri, 2015-06-19 at 15:08 -0700, Luis R. Rodriguez wrote:
>> > > From: "Luis R. Rodriguez" <mcgrof@suse.com>
>> > >
>> > > PCI BARs tell us whether prefetching is safe, but they don't say anything
>> > > about write combining (WC).  WC changes ordering rules and allows writes to
>> > > be collapsed, so it's not safe in general to use it on a prefetchable
>> > > region.
>> >
>> > Well, the PCIe spec at least specifies that a prefetchable BAR also
>> > tolerates write merging...
>>
>> How can that be determined and can that be used as a full bullet proof hint
>> to enable wc ? And are you sure? :)
>
> Well, I"m sure the spec says that ;-) But it could be new to PCIe, I
> haven't checked legacy PCI.

OK cool so to be clear from what I gather you are suggesting (or not
and letting me make it) is that we might be able to enforce
write-merging on prefetchable areas, and if we can *ensure* we do this
then automatically enable write-combining behind the scenes?

>> Reason all this was stated was to be
>> apologetic over why we can't automate this behind the scenes. Otherwise
>> we could amend what you stated into the commit log to elaborate on our
>> technical apology. Let me know!
>
> At least on powerpc, for mmap of resource to userspace, we take off the
> garded bit in the PTE for prefetchable BARs. This has the effect
> architecturally of enabling both prefetch and write combine (ie. side
> effect)

That's pretty darn sexy.

> though afaik, the implementations probably don't actually
> prefetch. We've done that for years.

Neat!

> In fact we don't have a way to split the notions, it's either G or no G,
> which carries both meanings.

Interesting.

> Do you have example/case of a device having problems ?

Nope but at least what made me squint at this being a possible
"feature" was that in practice when reviewing all of the kernels
pending device drivers using MTRR (potential write-combine candidates)
I encountered a slew of them which had the architectural unfortunate
practice of combining PCI bars for MMIO and their respective
write-combined desirable area (framebuffer for video, PIO buffers for
infiniband, etc). Now, to me that read more as a practice for old
school devices when such things were likely still being evaluated,
more modern devices seem to adhere to sticking a full PCI bar with
write-combining or not. Did you not encounter such mismatch splits on
powerpc ? Was such possibility addressed?

If what you are implying here is applicable to the x86 world I'm all
for enabling this as we'd have less code to maintain but I'll note
that getting a clarification alone on that prefetchable !=
write-combining was in and of itself hard, I'd be surprised if we
could get full architectural buy-in to this as an immediate automatic
feature. Because of this and because PAT did have some errata as well,
I would not be surprised if some PCI bridges / devices would end up
finding corner cases, as such if we can really do what you're saying
and unless we can get some super sane certainty over it across the
board, I'd be inclined to leave such things as a part of a new API.
Maybe have some folks test using the new API for all calls and after
some sanity of testing / releases consider a full switch.

That is, unless of course you're sure all this is sane and would wager
all-in on it from the get-go.

 Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1171743

On Wed, 2015-06-24 at 15:29 -0700, Luis R. Rodriguez wrote:

> Nope but at least what made me squint at this being a possible
> "feature" was that in practice when reviewing all of the kernels
> pending device drivers using MTRR (potential write-combine candidates)
> I encountered a slew of them which had the architectural unfortunate
> practice of combining PCI bars for MMIO and their respective
> write-combined desirable area (framebuffer for video, PIO buffers for
> infiniband, etc). Now, to me that read more as a practice for old
> school devices when such things were likely still being evaluated,
> more modern devices seem to adhere to sticking a full PCI bar with
> write-combining or not. Did you not encounter such mismatch splits on
> powerpc ? Was such possibility addressed?

Yes, I remember we dealt with some networking (or maybe IB) stuff back
in the day. We dealt with it by using a WC mapping and explicit barriers
to prevent combine when not wanted.

It is to be noted that on powerpc at least, writel() and co will never
combine due to the memory barriers in them. Only "normal" stores (or
__raw_writel) will.

On Intel things I different I assume...

The problem I see is that architectures can provide widely different
mechanisms and semantics in those areas and it's hard to define a
generic driver interface.

> If what you are implying here is applicable to the x86 world I'm all
> for enabling this as we'd have less code to maintain but I'll note
> that getting a clarification alone on that prefetchable !=
> write-combining was in and of itself hard, I'd be surprised if we
> could get full architectural buy-in to this as an immediate automatic
> feature.

I'm happy not to make it automatic for kernel space. As for user
mappings, maybe the right thing to do is to let us do what we do by
default with a quirk that can set a flag in pci_dev to disable that
behaviour (maybe on a per BAR basis ?). I think the common case is
that WC works.

>  Because of this and because PAT did have some errata as well,
> I would not be surprised if some PCI bridges / devices would end up
> finding corner cases, as such if we can really do what you're saying
> and unless we can get some super sane certainty over it across the
> board, I'd be inclined to leave such things as a part of a new API.
> Maybe have some folks test using the new API for all calls and after
> some sanity of testing / releases consider a full switch.
> 
> That is, unless of course you're sure all this is sane and would wager
> all-in on it from the get-go.

Cheers,
Ben.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1171755

On Thu, Jun 25, 2015 at 09:38:01AM +1000, Benjamin Herrenschmidt wrote:
> On Wed, 2015-06-24 at 15:29 -0700, Luis R. Rodriguez wrote:
> 
> > Nope but at least what made me squint at this being a possible
> > "feature" was that in practice when reviewing all of the kernels
> > pending device drivers using MTRR (potential write-combine candidates)
> > I encountered a slew of them which had the architectural unfortunate
> > practice of combining PCI bars for MMIO and their respective
> > write-combined desirable area (framebuffer for video, PIO buffers for
> > infiniband, etc). Now, to me that read more as a practice for old
> > school devices when such things were likely still being evaluated,
> > more modern devices seem to adhere to sticking a full PCI bar with
> > write-combining or not. Did you not encounter such mismatch splits on
> > powerpc ? Was such possibility addressed?
> 
> Yes, I remember we dealt with some networking (or maybe IB) stuff back
> in the day. We dealt with it by using a WC mapping and explicit barriers
> to prevent combine when not wanted.
> 
> It is to be noted that on powerpc at least, writel() and co will never
> combine due to the memory barriers in them. Only "normal" stores (or
> __raw_writel) will.
> 
> On Intel things I different I assume...

And the people who really know seem to be eaten by volcanoes or not have time.

> The problem I see is that architectures can provide widely different
> mechanisms and semantics in those areas and it's hard to define a
> generic driver interface.

Provided asm generic helpers are defined this should work though. The question
is just if there is enough motivation. Doesn't sound like it or as you note
maybe for userspace there might be. My position is that if it was too late for
PCIE or if this was too ambigious for PCIE perhaps the next generation bus
archicture or ammendments (I have no clue if this would would be possible) will
make this part of future device negotiation clear and fully expected, not a
wonderful side effect.

> > If what you are implying here is applicable to the x86 world I'm all
> > for enabling this as we'd have less code to maintain but I'll note
> > that getting a clarification alone on that prefetchable !=
> > write-combining was in and of itself hard, I'd be surprised if we
> > could get full architectural buy-in to this as an immediate automatic
> > feature.
> 
> I'm happy not to make it automatic for kernel space.

OK thanks I'll proceed with these patches then.

> As for user mappings,

Which APIs were you considering in this regard BTW?

> maybe the right thing to do is to let us do what we do by
> default with a quirk that can set a flag in pci_dev to disable that
> behaviour (maybe on a per BAR basis ?).

That might mean it could restrict userspace WC to require devices
to have WC parts on a full PCI BAR. Although this is restrictive
having reviewed most WC uses in the kernel I'd think this would be
a fair compromise to make, but again, if things are still murky
perhaps best we kiss this idea good bye for now and hope for it
to come in on future buses or ammendments (if that's even possible?).

> I think the common case is that WC works.

If WC does not I will note one hack which migh be worth mentioning -- just for
the record, this was devised as a shortcoming of a device where they failed to
split things properly and that *without* WC performance suffered quite a bit so
they made one full PCI BAR WC and as a work around this:

http://lkml.kernel.org/r/20150416041837.GA5712@hykim-PC

That is for registers that needed it:

write; wmb;

Then if they wanted to wait till the NIC has seen the write, they did:

write; wmb; read;

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1171791 — Re: [Xen-devel] [PATCH v7 5/9] PCI: Add pci_iomap_wc() variants

On Wed, Jun 24, 2015 at 5:52 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Thu, 2015-06-25 at 02:08 +0200, Luis R. Rodriguez wrote:
>>
>> OK thanks I'll proceed with these patches then.
>>
>> > As for user mappings,
>>
>> Which APIs were you considering in this regard BTW?
>
> mmap of the generic /sys/bus/pci/.../resource*

Like? Got a demo patch in mind ? :)

 Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1171798 — Re: [Xen-devel] [PATCH v7 5/9] PCI: Add pci_iomap_wc() variants

On Wed, 2015-06-24 at 17:58 -0700, Luis R. Rodriguez wrote:
> On Wed, Jun 24, 2015 at 5:52 PM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > On Thu, 2015-06-25 at 02:08 +0200, Luis R. Rodriguez wrote:
> >>
> >> OK thanks I'll proceed with these patches then.
> >>
> >> > As for user mappings,
> >>
> >> Which APIs were you considering in this regard BTW?
> >
> > mmap of the generic /sys/bus/pci/.../resource*
> 
> Like? Got a demo patch in mind ? :)

Nope. I was just thinking out loud. Today I have yet to see a problem
with what we do so ...

Cheers,
Ben.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1171792

On Thu, 2015-06-25 at 02:08 +0200, Luis R. Rodriguez wrote:
> 
> OK thanks I'll proceed with these patches then.
> 
> > As for user mappings,
> 
> Which APIs were you considering in this regard BTW?

mmap of the generic /sys/bus/pci/.../resource*

> > maybe the right thing to do is to let us do what we do by
> > default with a quirk that can set a flag in pci_dev to disable that
> > behaviour (maybe on a per BAR basis ?).
> 
> That might mean it could restrict userspace WC to require devices
> to have WC parts on a full PCI BAR. Although this is restrictive
> having reviewed most WC uses in the kernel I'd think this would be
> a fair compromise to make, but again, if things are still murky
> perhaps best we kiss this idea good bye for now and hope for it
> to come in on future buses or ammendments (if that's even possible?).
> 
> > I think the common case is that WC works.
> 
> If WC does not I will note one hack which migh be worth mentioning --
> just for
> the record, this was devised as a shortcoming of a device where they
> failed to
> split things properly and that *without* WC performance suffered quite
> a bit so
> they made one full PCI BAR WC and as a work around this:
> 
> http://lkml.kernel.org/r/20150416041837.GA5712@hykim-PC
> 
> That is for registers that needed it:
> 
> write; wmb;
> 
> Then if they wanted to wait till the NIC has seen the write, they did:
> 
> write; wmb; read;
> 

Right, and as I mentioned, on some archs like powerpc (and possibly
more), writel() and co contains an implicit mb()

>   Luis--


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1172481

| From: Arnd Bergmann [arnd@arndb.de]
| Sent: Thursday, June 25, 2015 1:44 PM
| 
| On Thursday 25 June 2015 15:01:56 Casey Leedom wrote:
| >
| >   Is there a reference I can read on this so I can understand
| > when and where we can use the __raw_*() APIs?  Can these
| > Raw Read/Write operations be reordered with respect to
| > each other or are the use of the various flavors of SYNC
| > instructions just to maintain order between Cached Memory
| > Accesses and I/O Instructions?
| 
| The interpretation is not consistent across architectures.
| 
| My best description would be that the __raw_*() accessors should
| only be used for accessing RAM areas that are known to have no
| side-effects and can be read in any size (8-bit to 64-bit wide),
| any alignment, and do not have a specific endianess.
| 
| If you are dealing with MMIO registers that have a fixed endianess
| and size, the correct accessor would be readl_relaxed(), which
| is like readl() but lacks the barriers on certain architectures.

Ah, thanks.  I see now that the __raw_*() APIs don't do any of the
Endian Swizzling.  Unfortunately the *_relaxed() APIs on PowerPC
are just defined as the normal *() routines.  From
arch/powerpc/include/asm/io.h:

    /*
     * We don't do relaxed operations yet, at least not with this semantic
     */
    #define readb_relaxed(addr)     readb(addr)
    #define readw_relaxed(addr)     readw(addr)
    #define readl_relaxed(addr)     readl(addr)
    #define readq_relaxed(addr)     readq(addr)
    #define writeb_relaxed(v, addr) writeb(v, addr)
    #define writew_relaxed(v, addr) writew(v, addr)
    #define writel_relaxed(v, addr) writel(v, addr)
    #define writeq_relaxed(v, addr) writeq(v, addr)

    (And in fact, this is true for most of the architectures.)

So we could go ahead and use these but for now there wouldn't be
any effect.

Hhmmm, so what do PowerPC Drivers do when they want to take
advantage of Write Combining?  Do their own Endian Swizzling
with the __raw_*() APIs?

Casey--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1172506

On Thu, 2015-06-25 at 21:40 +0000, Casey Leedom wrote:
> Hhmmm, so what do PowerPC Drivers do when they want to take
> advantage of Write Combining?  Do their own Endian Swizzling
> with the __raw_*() APIs?

Yeah either, we need to fix our relaxed implementation (patch
welcome :-)

Cheers,
Ben.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1173184 — Re: [Xen-devel] [PATCH v7 5/9] PCI: Add pci_iomap_wc() variants

On Fri, 2015-06-26 at 15:41 -0700, Luis R. Rodriguez wrote:

> > It wasn't nullified for the main user at the time, the fb. And I
> > mentioned an IB adapter or two for which the code had been hand
> tuned.
> 
> This still means there could be some affected drivers when used on
> powerpc, no?

Yes. In fact what about things like ARM who also have barriers in their
writel() ? Won't they also break WC ?

I'm trying to work with the architect and designers here to figure out
exactly where we stand and what we can do. As spelled out by our
architecture, things don't look great, because basically, we only have
attribute bit (garded) which when not set implies both WC and out of
order (& prefetch), and unclear barrier semantics in that case as well.

I *think* we might be able to settle with something along the lines of
"writel_relaxed() will allow combine on a WC mapping" but how I'm going
to get there is TBD.

It would be interesting to clarify the semantics of using the relaxed
accessors in combination with WC anyway. I wouldn't mind if the
definition involved also relaxing general ordering :-) It would
definitely make my life easier.

Cheers,
Ben.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1172569

On Thu, 2015-06-25 at 21:40 +0000, Casey Leedom wrote:
> 
> Ah, thanks.  I see now that the __raw_*() APIs don't do any of the
> Endian Swizzling.  Unfortunately the *_relaxed() APIs on PowerPC
> are just defined as the normal *() routines.  From
> arch/powerpc/include/asm/io.h:
> 
>     /*
>      * We don't do relaxed operations yet, at least not with this
> semantic
>      */

Yes so I was looking at this but there are some difficulties.
Architecturally, even with I=1 G=1 mappings (normal ioremap), we have no
guarantee of ordering of load vs. store unless I misunderstood
something. I think all current implementations provide some of that but
without barriers in the accessors, we aren't architecturally correct.

However, having those barriers will cause issues with G=0 (write
combine). It's unclear whether eieio() will provide the required
ordering for I=1 G=0 mappings and it will probably break write combine.

I'm looking into it with our HW guys and will try to come up with a
solution for power, but it doesn't help that our memory model conflates
write combining with other relaxations and that all our barriers also
prevent write combine.

Maybe we can bias the relaxed accessors toward write, by having no
barriers in it, and putting extra ones in reads.

Cheers,
Ben.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1176274

> On Jul 2, 2015, at 11:49 AM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> 
> On Sat, Jun 27, 2015 at 08:00:48AM +1000, Benjamin Herrenschmidt wrote:
>> On Fri, 2015-06-26 at 16:24 +0000, Casey Leedom wrote:
>>>  Thanks for looking into this Ben.  As it stands now, it seems as
>>> if Write Combined mappings simply aren't supported and/or all
>>> driver writers trying to utilize Write Combined mappings have to
>>> "hand roll" their own solutions which really isn't supportable.
>>> 
>>>  One thing that might be considered is simply to treat the desire
>>> to utilize the Write Combining hardware as a separate issue and
>>> develop writel_wc(), writeq_wc(), etc.
> 
> That seems rather sloppy and cumbersome, its best to just provide the
> infrastructure for initial mapping for an area and let the hardware do it for
> you. With the current design drivers would just use regular read/write on all
> areas and the only thing that will set them apart will be the mapping.  With
> what you propose we'd end up having to shift a whole bunch of reads/writes for
> just the purpose of adding WC to an area that didn't have wc before. That's
> a waste of code and time.
> 
>>> Those could be defined
>>> as legal only for Write Combined Mappings and would "do the
>>> right thing" for each architecture.  
> 
> Yuck.

  Yeah, probably.  But it sounds like from Ben’s response, we should really use write_relaxed() as an already existing API for this.  But most of the architectures just define this as writel() so some work would need to be done to get it to work.

>> The question then is what is "the right thing". In the powerpc case,
>> we'll have a non-garded mapping, which means we also get no ordering
>> between load and stores.
> 
> I don't follow, you *ordering* between load and stores for WC? We should
> not need that for WC, its why WC is used for only very specific things
> such as framebuffer and PIO (which BTw I still don't quite get all this
> use case for infiniband to be honest, and I will note I do see some
> proprietary hardware extensions like bursts but nothing covering all
> this in a general doc, I think I think it all just has to do that this
> is a hardware hack in reality, which we sell as a feature).

  I was talking about the work that our drivers (cxgb4, cxgb4vf, etc.) do to ensure correct ordering between writes to memory and I/O space.  For instance, issuing a wmb() between writes to a DMA buffer and the write to a register telling the hardware that the data is available.  This isn’t necessary on the Strongly Ordered Intel architectures but is necessary on other architectures.

Casey--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web