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

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
To: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Catalin Marinas <catalin.marinas@arm.com>
Cc: Clint Sbisa <csbisa@amazon.com>,
	linux-pci@vger.kernel.org, Bjorn Helgaas <helgaas@kernel.org>,
	linux-arm-kernel@lists.infradead.org, will@kernel.org
Subject: Re: [PATCH] arm64: Enable PCI write-combine resources under sysfs
Date: Thu, 03 Sep 2020 09:03:32 +1000	[thread overview]
Message-ID: <1fa18cf358eb6570f919c2b03c193e856c72705b.camel@kernel.crashing.org> (raw)
In-Reply-To: <20200902175445.GA31706@e121166-lin.cambridge.arm.com>

On Wed, 2020-09-02 at 18:54 +0100, Lorenzo Pieralisi wrote:
> > > > If that driver is fixed to check what it actually wants to check, would that
> > > > address your concern about the blanket enable? I don't see any other references
> > > > to this in kernel drivers and I think the documentation at
> > > > `filesystems/sysfs-pci.rst` outlines it pretty explicitly:
> > > > 
> > > >    Platforms which support write-combining maps of PCI resources must define
> > > >    arch_can_pci_mmap_wc() which shall evaluate to non-zero at runtime when
> > > >    write-combining is permitted.
> > > 
> > > That's exactly the problem. I am asking you: what does "write-combining
> > > maps of PCI resources" mean ?
> > > 
> > > I understand we do want weak ordering for prefetchable BAR mappings
> > > but my worry is that by exposing the resources as WC to user space
> > > we are giving user space the impression that those mappings mirror
> > > x86 WC mappings behaviour that is not true on ARM64.
> > 
> > Would Device_GRE be close to the x86 WC better? It won't allow unaligned
> > accesses and that can be problematic for the user. OTOH, it doesn't
> > speculate reads, so it's safer from the hardware perspective.
> 
> Thanks Catalin for chiming in, it may yes but I need to figure out
> the precise semantics of WC on x86 first.

We never got to the bottom of that with powerpc... semantics of "WC"
are subtly different all over the archs. They key idea I think is for
us to state that a WC mapping drops all ordering guarantees :-)

That said, the goal here is to expose the sysfs _wc files, without
which, mapping of "no-side-effect" memory such as frame buffers etc...
produces something very very slow.

> Actually *if* I read x86 specs correctly WC mappings allow speculative
> reads, which then would shift the issue on the PCI specs that allow
> marking read side effects BARs as prefetchable;

Yes.

>  in other words if
> an endpoint is designed with a prefetchable BAR that has read side
> effects this is already an issue on x86 in the current kernel.

An powerpc. We remove the "G" bit. Same deal.

> There is that, plus the usage of arch_can_pci_mmap_wc() in mellanox
> drivers which I suspect it is yet another interpretation of x86 write
> combine - I don't know what happens if we let arch_can_pci_mmap_wc() == 1
> on both normalNC or deviceGRE mappings for pgprot_writecombine.
> 
> I think it is worth getting to the bottom of this before applying
> this patch.

I think it basically boils down to mapping things without side effect
and ordering guarantees but that still cannot be cached.

Cheers,
Ben.