On Wed, Sep 02, 2020 at 06:21:57PM +0100, Catalin Marinas wrote: > On Wed, Sep 02, 2020 at 05:47:02PM +0100, Lorenzo Pieralisi wrote: > > On Wed, Sep 02, 2020 at 02:29:22PM +0000, Clint Sbisa wrote: > > > 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: > > > > > 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. > > > > 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. 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; 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. 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. Thanks, Lorenzo
