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@xxxxxxxxxx> > > --- > > 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 > >
