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

From: Clint Sbisa <csbisa@amazon.com>
To: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: <linux-pci@vger.kernel.org>, Bjorn Helgaas <helgaas@kernel.org>,
	<benh@kernel.crashing.org>
Subject: Re: [PATCH] arm64: Enable PCI write-combine resources under sysfs
Date: Wed, 2 Sep 2020 14:29:22 +0000	[thread overview]
Message-ID: <20200902142922.xc4x6m33unkzewuh@amazon.com> (raw)
In-Reply-To: <20200902113207.GA27676@e121166-lin.cambridge.arm.com>

On Wed, Sep 02, 2020 at 12:32:07PM +0100, Lorenzo Pieralisi wrote:
> 
> On Mon, Aug 31, 2020 at 03:18:27PM +0000, Clint Sbisa wrote:
> > Using write-combine is crucial for performance of PCI devices where
> > significant amounts of transactions go over PCI BARs.
> 
> Write-combine is an x86ism that means nothing on ARM64 platforms
> so this should be rewritten to say what you actually mean, namely,
> you want to allow prefetchable resources to be mapped with
> "write combine" semantics (which means normal non-cacheable
> memory on arm64) through proc/sysfs.
> 
> This is an outright can of worms and the PCI specs don't help in this
> respect, since we may end up mapping resources that have read
> side-effects with normal NC mappings (ie that's what "write combine" is
> in arm64 - pgprot_writecombine() and that's speculative memory).
> 
> I am referring to "Additional Guidance on the Prefetchable Bit
> in Memory Space BARs" in the PCI specifications - it does not make
> any sense and must be removed because people use it to design
> endpoints.
> 
> True - this is a problem even in kernel drivers but at least there
> the ioremap_ semantics is in the driver and can be vetted.
> 
> This patch would make it user space ABI so I am a little nervous
> about merging this code TBH.
> 

User space applications are utilizing WC already. You can see DPDK code using
resourceX_wc over the usual resourceX file at
https://github.com/DPDK/dpdk/blob/main/drivers/bus/pci/linux/pci_uio.c#L312
(commit https://github.com/DPDK/dpdk/commit/4a928ef9f61).

Given that write-combine support was added in 2008 for x86 (and is also enabled
for powerpc and ia64), I'm not sure if there'd be a downside to enabling it on
arm64 as well given how prevalent it is. Lorenzo, do you still have particular
concerns about exposing this to userspace?

I understand that my commit message is outright wrong after your explanation,
so I'll rewrite that.

> > arm64 supports write-combine PCI mappings, so the appropriate define
> > has been added which will expose write-combine mappings under sysfs
> > for prefetchable PCI resources.
> >
> > Signed-off-by: Clint Sbisa <csbisa@amazon.com>
> > ---
> >  arch/arm64/include/asm/pci.h | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h
> > index 70b323cf8300..b33ca260e3c9 100644
> > --- a/arch/arm64/include/asm/pci.h
> > +++ b/arch/arm64/include/asm/pci.h
> > @@ -17,6 +17,7 @@
> >  #define pcibios_assign_all_busses() \
> >       (pci_has_flag(PCI_REASSIGN_ALL_BUS))
> >
> > +#define arch_can_pci_mmap_wc() 1
> 
> I am not comfortable with this blanket enable. Some existing drivers,
> eg:
> 
> drivers/infiniband/hw/mlx5
> 
> use this macro to detect WC capability which again, it is x86 specific,
> on arm64 it means nothing and can have consequences on the driver
> operations.

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.

It is otherwise only used by pci-sysfs to determine if a resourceX_wc file
should be exposed.

Thanks,
Clint

> 
> Thanks,
> Lorenzo
> 
> >  #define ARCH_GENERIC_PCI_MMAP_RESOURCE       1
> >
> >  extern int isa_dma_bridge_buggy;
> > --
> > 2.23.3
> >