Re: [PATCH] arm64: Enable PCI write-combine resources under sysfs

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Tue, 2020-09-15 at 20:40 -0300, Jason Gunthorpe wrote:
> Not quite, upstream kernel will never use WC on those
> devices. DEVICE_GRE is not supported in upstream,
> arch_can_pci_mmap_wc() is always false and the WC tester will always
> fail.
> 
> > With the patch, those device will now use MT_DEVICE_NC.
> 
> Which doesn't do WC at all on some ARM implementations.

Lovely... this is arm64 btw, still the case ?

Also we could make this a variable rather than a constant and choose
a more appropriate set of flags at boot time....

> > Why would that be a regression ? 
> 
> Using the WC submission flow when it doesn't work costs something like
> 10% performance vs using the non-WC flow.

You mean the driver uses a different path to the HW which ahs that
overhead, not that MMIOs have that overhead right ?

> Like I said, the case where the driver can't self test probably
> doesn't intersect with the ARM implementations that can't do write
> combining, and if it did, the users probably run the out of tree
> driver that has the hacky stuff to make it use DEVICE_GRE.

Ok. So you are saying to go for it and ignore that Mellanox case then ?
:-)

> > BTW. Lorenzo, why don't we use MT_DEVICE_GRE for pgprot_writecombine ?
> > Its not supported on some chips ?
> 
> It has alignment requirements drivers don't meet. We need a new
> concept of "write combining and I promise to do aligned access"

Ah yes, I remember. Right, we would need to provide new/better
accessors for these kind of things. It's going to be a mess to find a
common set that works for all archs.

> > What on earth is pgprot_device() ? This is new ? On ARM it will be
> > MT_DEVICE_nGnRE, so it allows posted write. It seems to match what
> > ioremap does. Should then ioremap use it as well ?
> > 
> > But it's only ever used for PCI mmap. Why is it different from
> > pgprot_noncached() which disables posted writes (nE) ?
> > 
> > Because a whole lot of drivers will use pgprot_noncached() explicitly
> > in either mmap or vmap, with the expectation that it's somewhat the
> > same as what ioremap does...
> 
> *boggle*
> 
> Only sysfs uses pci_mmap_resource_range() any other driver exposing
> BAR pages, like VFIO dies not. Makes no sense at all it is different.
> 
> Delete the ill defined pgprot_device() ? Nobody has complained
> something is wrong with VFIO in the 6 years since it was added...

I was wondering what it was, that's it ... 

Cheers,
Ben.