[QUESTION] Early Write Acknowledge for PCIe configuration space

[Gabriele Paoloni]

Hi Lorenzo

Many thanks to look into this

> -----Original Message-----
> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi at arm.com]
> Sent: 08 February 2017 18:36
> To: Will Deacon
> Cc: Arnd Bergmann; linux-arm-kernel at lists.infradead.org; John Garry;
> catalin.marinas at arm.com; Gabriele Paoloni; Linuxarm; xuwei (O);
> Wangzhou (B); Shameerali Kolothum Thodi; Guohanjun (Hanjun Guo)
> Subject: Re: [QUESTION] Early Write Acknowledge for PCIe configuration
> space
> 
> On Fri, Jan 06, 2017 at 11:42:32AM +0000, Will Deacon wrote:
> > On Fri, Jan 06, 2017 at 12:24:25PM +0100, Arnd Bergmann wrote:
> > > On Friday, January 6, 2017 11:15:22 AM CET John Garry wrote:
> > > > [apologies if this has been queried before]
> > > >
> > > > Hi ARM guys,
> > > >
> > > > I have a question about the device memory attributes we assign
> for PCIe
> > > > config space for arm64. Currently we use ioremap to map in the
> config
> > > > space; this uses nGnRE, which means we enable Early Write
> Acknowledge.
> > > >
> > > > The ARMv8 ARM states that "ARM recommend that No Early Write
> Acknowledge
> > > > Hint is used for PCIe configuration writes".
> > > >
> > > > I understand a problem with using E is in that configuration
> write is a
> > > > non-post operation, which means the RP requires to get the
> completion
> > > > ack from the EP The problem here is if CPU writes data to ECAM by
> E,
> > > > complete will go back to CPU directly, and maybe at this point
> the write
> > > > has not reached the EP.
> > > >
> > > > I believe that this may cause ordering issues in PCI read/write.
> In
> > > > practice we use non-relaxed readl/writel to access config space,
> which
> > > > include the synchronization barriers, which, *as I understand*,
> even if
> > > > for full system domain, may be negated by the E attribute for
> PCIe.
> > >
> > > I don't think the barriers in readl/writel are enough here, in
> particular
> > > the write barrier is *before* the access to synchronize DMAs
> > > on RAM with MMIO accesses, which is a bit different from what you
> > > have here.
> > >
> > > > So a question: why is the recommendation in the ARMv8 ARM
> ignored?
> > >
> > > Probably nobody thought about this properly in the Linux drivers.
> The
> > > ARMv8 ARM sounds correct here.
> >
> > The ARMv8 ARM also says that the E attribute is a hint, so there's no
> > guarantee that it's actually honoured by the implementation. However,
> > now that it explicitly mentions PCI config space, the intention is
> clearly
> > that nE *is* honoured for systems using PCIe, so I agree that we
> should make
> > this change. I don't want to use the nE type for all ioremap
> invocations,
> > though.
> 
> I suspect this is a potential issue on ARM 32-bit systems too(?).
> Fixing
> IO space should be reasonably simple, we just have to change the
> pgprot_device() in pci_remap_iospace() to pgprot_noncached() (they are
> the same attributes on ARM 32-bit already if I am not mistaken).

Agreed on this. Actually I noticed that pci_remap_iospace() is __weak
but no other definitions are present...

> 
> What do we want to do for config space ? Implement ioremap_uc() for
> ARM/ARM64 (and add a devm_ioremap_uc() call to use it in basically all
> drivers/pci/host implementations to map config space - or we just patch
> the ioremap calls in drivers/pci/ecam.c and we assume the other hosts
> controllers have purportedly called devm_ioremap() because on those
> platforms it just works ok till further notice ?)

Well if my understanding is correct we are 100% positive that this issue
affects controllers mounted on ARM and ARM64, right?
If this is correct I would look at drivers/pci/host/Kconfig and see which
controllers depend on ARM or ARM64; for these controllers I would replace
devm_ioremap() with devm_ioremap_uc().
Obviously I would also replace the calls in drivers/pci/ecam.c...

What do you think?

Ciao
Gab


> 
> Thanks,
> Lorenzo