Re: [PATCH v4 1/5] pci: add pci_iomap_wc() variants

On Thu, Apr 30, 2015 at 9:52 AM, Luis R. Rodriguez [off-list ref] wrote:

On Thu, Apr 30, 2015 at 10:59:17AM -0500, Bjorn Helgaas wrote:

quoted

[+cc linux-pci]

Hi Luis,

On Wed, Apr 29, 2015 at 02:36:08PM -0700, Luis R. Rodriguez wrote:

quoted

From: "Luis R. Rodriguez" <redacted>

This allows drivers to take advantage of write-combining
when possible. Ideally we'd have pci_read_bases() just
peg an IORESOURCE_WC flag for us

This makes it sound like pci_read_bases() could do a better job
if we just tried harder, but I don't think that's the case.  All
pci_read_bases() can do is look at the bits in the BAR.  For
memory BARs, there's a "prefetchable" bit and a "64-bit" bit.

If you just want to complain that the PCI spec didn't define a
way for software to discover whether a BAR can be mapped with WC,
that's fine, but it's misleading to suggest that pci_read_bases()
could figure out WC without some help from the spec.

You're right sorry about that, in my original patch this was more
of a question and I did not have a full answer for but mst had
clarified before the spec doesn't allow for this [0] and you are
confirming this now as well.

[0] https://lkml.org/lkml/2015/4/21/714

I'll update the patch and at least document we did think about
this and that its a shortcoming of the spec.

quoted

quoted

but where exactly
video devices memory lie varies *largely* and at times things
are mixed with MMIO registers, sometimes we can address
the changes in drivers, other times the change requires
intrusive changes.

Although there is also arch_phys_wc_add() that makes use of
architecture specific write-combining alternatives (MTRR on
x86 when a system does not have PAT) we void polluting
pci_iomap() space with it and force drivers and subsystems
that want to use it to be explicit.

I'm not quite sure I understand the point you're making here
about not polluting pci_iomap_wc() with arch_phys_wc_add().  I
think the choice is for a driver to do either this:

  info->screen_base = pci_iomap_wc(dev, 0, 0);

or this:

  info->screen_base = pci_iomap_wc(dev, 0, 0);
  par->wc_cookie = arch_phys_wc_add(pci_resource_start(dev, 0),
                                  pci_resource_len(dev, 0));

The driver is *already* being explicit because it calls
pci_iomap_wc() instead of pci_iomap().

It seems like it would be ideal if ioremap_wc() could call
arch_phys_wc_add() internally.

Indeed, that's what I was alluding to.

quoted

Doesn't any caller of
arch_phys_wc_add() have to also do some sort of ioremap()
beforehand?

This is not a requirement as the physical address is used,
not the virtual address.

quoted

I assume there's some reason for separating them,

Well a full sweep to change to arch_phys_wc_add() was never done,
consider this part of the last effort to do so. In retrospect now
that I've covered all other drivers in 12 different series of patches
I think its perhaps best to not mesh them together as we're phasing
out MTRR and the only reason to have arch_phys_wc_add() is for MTRR
which is legacy.

I would say it much more strongly.

Drivers for new hardware SHOULD NOT call arch_phys_wc_add, directly or
otherwise.  MTRRs are crap.  They have nasty alignment requirements,
they are a very limited and unpredictable resource, and the interact
poorly with BIOS.  They should really only be used for old video
framebuffers and such.

Anything new should use PAT (it's been available for a long time) and
possibly streaming memory writes.  Even fancy server gear (myri10ge,
for example) should stay far away from MTRRs and such: it's very easy
to put enough devices in a server board that you simply run out of
MTRRs and arch_phys_wc_add will stop working.

If we make ioremap_wc and similar start automatically adding MTRRs,
then performance will vary wildly with the order of driver loading,
because we'll run out of MTRRs part-way through bootup.

ioremap_wc via PAT, on the other hand, is 100% reliable on newer hardware.

Maybe I should have called it arch_phys_wc_add_awful_legacy_hack.

--Andy