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

From: Clint Sbisa <csbisa@amazon.com>
To: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Yishai Hadas <yishaih@mellanox.com>,
	Guy Levy <guyle@mellanox.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>,
	<linux-pci@vger.kernel.org>, Bjorn Helgaas <helgaas@kernel.org>,
	<linux-arm-kernel@lists.infradead.org>, <will@kernel.org>,
	<catalin.marinas@arm.com>
Subject: Re: [PATCH] arm64: Enable PCI write-combine resources under sysfs
Date: Thu, 3 Sep 2020 14:36:09 +0000	[thread overview]
Message-ID: <20200903143609.didfh6jwt7etm7he@amazon.com> (raw)
In-Reply-To: <20200903110844.GB11284@e121166-lin.cambridge.arm.com>

Adding some Mellanox folks to comment on their usage of arch_can_pci_mmap_wc().

On Thu, Sep 03, 2020 at 12:08:44PM +0100, Lorenzo Pieralisi wrote:
> On Thu, Sep 03, 2020 at 09:07:00AM +1000, Benjamin Herrenschmidt wrote:
> > On Wed, 2020-09-02 at 17:47 +0100, Lorenzo Pieralisi wrote:
> > > Yes I do and I expressed them.
> > >
> > > The first concern is the WC ambiguity on non-x86 systems, it looks
> > > like write combinining means everything and nothing at the same time
> > > on != x86 arches.
> > >
> > > On x86 prefetchable BAR == WC mapping (still conditional on arch
> > > features ie PAT, not a blanket enable). On ARM64 WC mapping currently
> > > corresponds to normal NC memory and the PCIe specs allow read
> > > side-effects BAR to be marked as prefetchable, I need to force PCI
> > > sig
> > > to remove the section I mentioned from the specifications because
> > > there
> > > is NO way it can be detected if a prefetchable BAR maps to read
> > > side-effects memory.
> >
> > Im not sure I understand your sentence. It's been a long accepted rule
> > in PCI land that "prefetchable" BARs means "no side effects" and in
> > fact allows much more than just prefetching :-)
> 
> I am referring to the nefarious:
> 
> "Additional Guidance on the Prefetchable Bit in Memory Space BARs"
> 
> I read it 100 times and I still have no idea how it can be implemented,
> it sorts of acknowledges that read side-effects memory can be marked
> as a prefetchable BAR *if* the system meets some criteria.
> 
> As if endpoint designers knew the system where their endpoint is
> plugged into (+ bit (3) in a BAR is read-only).
> 
> I think that that implementation note must be removed from the
> specifications - if anyone dares to follow it this whole
> WC resource mapping can trigger trouble.
> 
> Good news is that it would be trouble for all arches :)
> 
> > > A kernel device driver would (hopefully) know, sysfs code that just
> > > checks the prefetchable attribute and exports resource_WC does not.
> > >
> > > As I mentioned, if the mapping is done in a device specific driver it
> > > can be vetted and there are not many drivers mapping BARs as
> > > ioremap_wc().
> >
> > It's been what other architectures have been doing for mroe than a
> > decade without significant issues... I don't think you should worry too
> > much about this.
> 
> Minus what I wrote above, I agree with you. I'd still be able to
> understand what this patch changes in the mellanox driver HW
> handling though - not sure what they expect from arch_can_pci_mmap_wc()
> returning 1.

This seems to have been added broadly for x86, PPC, and ARM as part of initial
WC support in the driver (37aa5c36 "IB/mlx5: Add UARs write-combining and
non-cached mapping"). It was updated to use `arch_can_pci_mmap_wc()` later
(1f3db161 "IB/mlx5: Generally use the WC auto detection test result").

Guy, Yishai, there are some concerns about difference the technical definition
of WC and how WC is actually used. Can you comment on the usage of WC in mlx5
and which definition of WC the driver utilizes? We're unsure if a blanket
enable for arm64 is safe in light of the driver's use of this function.

Thanks,
Clint

> 
> Thanks,
> Lorenzo