| Message ID | 20170227151436.18698-4-lorenzo.pieralisi@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
[+cc Luis] On Mon, Feb 27, 2017 at 03:14:14PM +0000, Lorenzo Pieralisi wrote: > The PCI specifications (Rev 3.0, 3.2.5 "Transaction Ordering and > Posting") mandate non-posted configuration transactions. As further > highlighted in the PCIe specifications (4.0 - Rev0.3, "Ordering > Considerations for the Enhanced Configuration Access Mechanism"), > through ECAM and ECAM-derivative configuration mechanism, the memory > mapped transactions from the host CPU into Configuration Requests on the > PCI express fabric may create ordering problems for software because > writes to memory address are typically posted transactions (unless the > architecture can enforce through virtual address mapping non-posted > write transactions behaviour) but writes to Configuration Space are not > posted on the PCI express fabric. > > Current DT and ACPI host bridge controllers map PCI configuration space > (ECAM and ECAM-derivative) into the virtual address space through > ioremap() calls, that are non-cacheable device accesses on most > architectures, but may provide "bufferable" or "posted" write semantics > in architecture like eg ARM/ARM64 that allow ioremap'ed regions writes > to be buffered in the bus connecting the host CPU to the PCI fabric; > this behaviour, as underlined in the PCIe specifications, may trigger > transactions ordering rules and must be prevented. > > Introduce a new generic and explicit API to create a memory > mapping for ECAM and ECAM-derivative config space area that > defaults to ioremap_nocache() (which should provide a sane default > behaviour) but still allowing architectures on which ioremap_nocache() > results in posted write transactions to override the function > call with an arch specific implementation that complies with > the PCI specifications for configuration transactions. > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Will Deacon <will.deacon@arm.com> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: Russell King <linux@armlinux.org.uk> > Cc: Catalin Marinas <catalin.marinas@arm.com> > --- > include/asm-generic/io.h | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h > index 7ef015e..52dda81 100644 > --- a/include/asm-generic/io.h > +++ b/include/asm-generic/io.h > @@ -915,6 +915,15 @@ extern void ioport_unmap(void __iomem *p); > #endif /* CONFIG_GENERIC_IOMAP */ > #endif /* CONFIG_HAS_IOPORT_MAP */ > > +#ifndef pci_remap_cfgspace > +#define pci_remap_cfgspace pci_remap_cfgspace > +static inline void __iomem *pci_remap_cfgspace(phys_addr_t offset, > + size_t size) > +{ > + return ioremap_nocache(offset, size); > +} I'm fine with this conceptually, but I think it would make more sense if the name weren't specific to PCI or config space, e.g., ioremap_nopost() or something. > +#endif > + > #ifndef xlate_dev_kmem_ptr > #define xlate_dev_kmem_ptr xlate_dev_kmem_ptr > static inline void *xlate_dev_kmem_ptr(void *addr) > -- > 2.10.0 >
On Thu, Mar 16, 2017 at 04:12:43PM -0500, Bjorn Helgaas wrote: > [+cc Luis] > > On Mon, Feb 27, 2017 at 03:14:14PM +0000, Lorenzo Pieralisi wrote: > > The PCI specifications (Rev 3.0, 3.2.5 "Transaction Ordering and > > Posting") mandate non-posted configuration transactions. As further > > highlighted in the PCIe specifications (4.0 - Rev0.3, "Ordering > > Considerations for the Enhanced Configuration Access Mechanism"), > > through ECAM and ECAM-derivative configuration mechanism, the memory > > mapped transactions from the host CPU into Configuration Requests on the > > PCI express fabric may create ordering problems for software because > > writes to memory address are typically posted transactions (unless the > > architecture can enforce through virtual address mapping non-posted > > write transactions behaviour) but writes to Configuration Space are not > > posted on the PCI express fabric. > > > > Current DT and ACPI host bridge controllers map PCI configuration space > > (ECAM and ECAM-derivative) into the virtual address space through > > ioremap() calls, that are non-cacheable device accesses on most > > architectures, but may provide "bufferable" or "posted" write semantics > > in architecture like eg ARM/ARM64 that allow ioremap'ed regions writes > > to be buffered in the bus connecting the host CPU to the PCI fabric; > > this behaviour, as underlined in the PCIe specifications, may trigger > > transactions ordering rules and must be prevented. > > > > Introduce a new generic and explicit API to create a memory > > mapping for ECAM and ECAM-derivative config space area that > > defaults to ioremap_nocache() (which should provide a sane default > > behaviour) but still allowing architectures on which ioremap_nocache() > > results in posted write transactions to override the function > > call with an arch specific implementation that complies with > > the PCI specifications for configuration transactions. So... I take it this is actually fixing a series of odd issues also, do we at least have some reports or actual issues ? > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > Cc: Arnd Bergmann <arnd@arndb.de> > > Cc: Will Deacon <will.deacon@arm.com> > > Cc: Bjorn Helgaas <bhelgaas@google.com> > > Cc: Russell King <linux@armlinux.org.uk> > > Cc: Catalin Marinas <catalin.marinas@arm.com> > > --- > > include/asm-generic/io.h | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h > > index 7ef015e..52dda81 100644 > > --- a/include/asm-generic/io.h > > +++ b/include/asm-generic/io.h > > @@ -915,6 +915,15 @@ extern void ioport_unmap(void __iomem *p); > > #endif /* CONFIG_GENERIC_IOMAP */ > > #endif /* CONFIG_HAS_IOPORT_MAP */ > > > > +#ifndef pci_remap_cfgspace > > +#define pci_remap_cfgspace pci_remap_cfgspace > > +static inline void __iomem *pci_remap_cfgspace(phys_addr_t offset, > > + size_t size) > > +{ > > + return ioremap_nocache(offset, size); > > +} > > I'm fine with this conceptually, but I think it would make more sense > if the name weren't specific to PCI or config space, e.g., > ioremap_nopost() or something. Seems reasonable to me -- but are there other buses that could use this already as well ? Wouldn't these other buses also run into similar issues ? Can someone also bounce me a copy of the patches that use this ? While at it, please add some documentation too, the above commit log is huge, and yet for the person eyeballing the code they won't have any clue why this was added exactly. Since this is about helping with picking the right ioremap due to certain semantics / requirements on the PCI config space, we should clarify then what exactly are the expectations here. The clearer you are the less in trouble we can get later. Luis
On 17/03/2017 00:08, Luis R. Rodriguez wrote: > On Thu, Mar 16, 2017 at 04:12:43PM -0500, Bjorn Helgaas wrote: >> [+cc Luis] >> >> On Mon, Feb 27, 2017 at 03:14:14PM +0000, Lorenzo Pieralisi wrote: >>> The PCI specifications (Rev 3.0, 3.2.5 "Transaction Ordering and >>> Posting") mandate non-posted configuration transactions. As further >>> highlighted in the PCIe specifications (4.0 - Rev0.3, "Ordering >>> Considerations for the Enhanced Configuration Access Mechanism"), >>> through ECAM and ECAM-derivative configuration mechanism, the memory >>> mapped transactions from the host CPU into Configuration Requests on the >>> PCI express fabric may create ordering problems for software because >>> writes to memory address are typically posted transactions (unless the >>> architecture can enforce through virtual address mapping non-posted >>> write transactions behaviour) but writes to Configuration Space are not >>> posted on the PCI express fabric. >>> >>> Current DT and ACPI host bridge controllers map PCI configuration space >>> (ECAM and ECAM-derivative) into the virtual address space through >>> ioremap() calls, that are non-cacheable device accesses on most >>> architectures, but may provide "bufferable" or "posted" write semantics >>> in architecture like eg ARM/ARM64 that allow ioremap'ed regions writes >>> to be buffered in the bus connecting the host CPU to the PCI fabric; >>> this behaviour, as underlined in the PCIe specifications, may trigger >>> transactions ordering rules and must be prevented. >>> >>> Introduce a new generic and explicit API to create a memory >>> mapping for ECAM and ECAM-derivative config space area that >>> defaults to ioremap_nocache() (which should provide a sane default >>> behaviour) but still allowing architectures on which ioremap_nocache() >>> results in posted write transactions to override the function >>> call with an arch specific implementation that complies with >>> the PCI specifications for configuration transactions. > > So... I take it this is actually fixing a series of odd issues also, > do we at least have some reports or actual issues ? > We (Huawei) originally raised the concern [1], but have not actually experienced any related issue. Thanks, John [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2017-January/477353.html >>> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> >>> Cc: Arnd Bergmann <arnd@arndb.de> >>> Cc: Will Deacon <will.deacon@arm.com> >>> Cc: Bjorn Helgaas <bhelgaas@google.com> >>> Cc: Russell King <linux@armlinux.org.uk> >>> Cc: Catalin Marinas <catalin.marinas@arm.com> >>> --- >>> include/asm-generic/io.h | 9 +++++++++ >>> 1 file changed, 9 insertions(+) >>> >>> diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h >>> index 7ef015e..52dda81 100644 >>> --- a/include/asm-generic/io.h >>> +++ b/include/asm-generic/io.h >>> @@ -915,6 +915,15 @@ extern void ioport_unmap(void __iomem *p); >>> #endif /* CONFIG_GENERIC_IOMAP */ >>> #endif /* CONFIG_HAS_IOPORT_MAP */ >>> >>> +#ifndef pci_remap_cfgspace >>> +#define pci_remap_cfgspace pci_remap_cfgspace >>> +static inline void __iomem *pci_remap_cfgspace(phys_addr_t offset, >>> + size_t size) >>> +{ >>> + return ioremap_nocache(offset, size); >>> +} >> >> I'm fine with this conceptually, but I think it would make more sense >> if the name weren't specific to PCI or config space, e.g., >> ioremap_nopost() or something. > > Seems reasonable to me -- but are there other buses that could use this already > as well ? Wouldn't these other buses also run into similar issues ? Can someone > also bounce me a copy of the patches that use this ? > > While at it, please add some documentation too, the above commit log is huge, > and yet for the person eyeballing the code they won't have any clue why this > was added exactly. Since this is about helping with picking the right > ioremap due to certain semantics / requirements on the PCI config space, > we should clarify then what exactly are the expectations here. The clearer > you are the less in trouble we can get later. > > Luis > > . >
On Fri, Mar 17, 2017 at 01:08:03AM +0100, Luis R. Rodriguez wrote: > On Thu, Mar 16, 2017 at 04:12:43PM -0500, Bjorn Helgaas wrote: > > [+cc Luis] > > > > On Mon, Feb 27, 2017 at 03:14:14PM +0000, Lorenzo Pieralisi wrote: > > > The PCI specifications (Rev 3.0, 3.2.5 "Transaction Ordering and > > > Posting") mandate non-posted configuration transactions. As further > > > highlighted in the PCIe specifications (4.0 - Rev0.3, "Ordering > > > Considerations for the Enhanced Configuration Access Mechanism"), > > > through ECAM and ECAM-derivative configuration mechanism, the memory > > > mapped transactions from the host CPU into Configuration Requests on the > > > PCI express fabric may create ordering problems for software because > > > writes to memory address are typically posted transactions (unless the > > > architecture can enforce through virtual address mapping non-posted > > > write transactions behaviour) but writes to Configuration Space are not > > > posted on the PCI express fabric. > > > > > > Current DT and ACPI host bridge controllers map PCI configuration space > > > (ECAM and ECAM-derivative) into the virtual address space through > > > ioremap() calls, that are non-cacheable device accesses on most > > > architectures, but may provide "bufferable" or "posted" write semantics > > > in architecture like eg ARM/ARM64 that allow ioremap'ed regions writes > > > to be buffered in the bus connecting the host CPU to the PCI fabric; > > > this behaviour, as underlined in the PCIe specifications, may trigger > > > transactions ordering rules and must be prevented. > > > > > > Introduce a new generic and explicit API to create a memory > > > mapping for ECAM and ECAM-derivative config space area that > > > defaults to ioremap_nocache() (which should provide a sane default > > > behaviour) but still allowing architectures on which ioremap_nocache() > > > results in posted write transactions to override the function > > > call with an arch specific implementation that complies with > > > the PCI specifications for configuration transactions. > > ... > > > +#ifndef pci_remap_cfgspace > > > +#define pci_remap_cfgspace pci_remap_cfgspace > > > +static inline void __iomem *pci_remap_cfgspace(phys_addr_t offset, > > > + size_t size) > > > +{ > > > + return ioremap_nocache(offset, size); > > > +} > > > > I'm fine with this conceptually, but I think it would make more sense > > if the name weren't specific to PCI or config space, e.g., > > ioremap_nopost() or something. > > Seems reasonable to me -- but are there other buses that could use > this already as well ? Wouldn't these other buses also run into > similar issues ? Can someone also bounce me a copy of the patches > that use this ? I forwarded a copy of the initial posting of all 20 patches to you. > While at it, please add some documentation too, the above commit log > is huge, and yet for the person eyeballing the code they won't have > any clue why this was added exactly. Since this is about helping > with picking the right ioremap due to certain semantics / > requirements on the PCI config space, we should clarify then what > exactly are the expectations here. The clearer you are the less in > trouble we can get later. I think the documentation above does contain the critical facts that: - Accesses to PCI config space and PCI I/O port space should be non-posted - ARM64 currently maps them as posted, which may cause ordering problems It's possible the changelog could be made clearer by *removing* text, but I'm not sure what Lorenzo could *add* to make it better. This particular patch is generic (not ARM64-specific), so maybe all we need to say here is that PCI requires non-posted mappings in some cases, and we currently don't have a generic ioremap() to make them, so we're adding a way for an arch to provide such an interface. Bjorn
On Thu, Mar 16, 2017 at 04:12:43PM -0500, Bjorn Helgaas wrote: > [+cc Luis] > > On Mon, Feb 27, 2017 at 03:14:14PM +0000, Lorenzo Pieralisi wrote: > > The PCI specifications (Rev 3.0, 3.2.5 "Transaction Ordering and > > Posting") mandate non-posted configuration transactions. As further > > highlighted in the PCIe specifications (4.0 - Rev0.3, "Ordering > > Considerations for the Enhanced Configuration Access Mechanism"), > > through ECAM and ECAM-derivative configuration mechanism, the memory > > mapped transactions from the host CPU into Configuration Requests on the > > PCI express fabric may create ordering problems for software because > > writes to memory address are typically posted transactions (unless the > > architecture can enforce through virtual address mapping non-posted > > write transactions behaviour) but writes to Configuration Space are not > > posted on the PCI express fabric. > > > > Current DT and ACPI host bridge controllers map PCI configuration space > > (ECAM and ECAM-derivative) into the virtual address space through > > ioremap() calls, that are non-cacheable device accesses on most > > architectures, but may provide "bufferable" or "posted" write semantics > > in architecture like eg ARM/ARM64 that allow ioremap'ed regions writes > > to be buffered in the bus connecting the host CPU to the PCI fabric; > > this behaviour, as underlined in the PCIe specifications, may trigger > > transactions ordering rules and must be prevented. > > > > Introduce a new generic and explicit API to create a memory > > mapping for ECAM and ECAM-derivative config space area that > > defaults to ioremap_nocache() (which should provide a sane default > > behaviour) but still allowing architectures on which ioremap_nocache() > > results in posted write transactions to override the function > > call with an arch specific implementation that complies with > > the PCI specifications for configuration transactions. > > > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > Cc: Arnd Bergmann <arnd@arndb.de> > > Cc: Will Deacon <will.deacon@arm.com> > > Cc: Bjorn Helgaas <bhelgaas@google.com> > > Cc: Russell King <linux@armlinux.org.uk> > > Cc: Catalin Marinas <catalin.marinas@arm.com> > > --- > > include/asm-generic/io.h | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h > > index 7ef015e..52dda81 100644 > > --- a/include/asm-generic/io.h > > +++ b/include/asm-generic/io.h > > @@ -915,6 +915,15 @@ extern void ioport_unmap(void __iomem *p); > > #endif /* CONFIG_GENERIC_IOMAP */ > > #endif /* CONFIG_HAS_IOPORT_MAP */ > > > > +#ifndef pci_remap_cfgspace > > +#define pci_remap_cfgspace pci_remap_cfgspace > > +static inline void __iomem *pci_remap_cfgspace(phys_addr_t offset, > > + size_t size) > > +{ > > + return ioremap_nocache(offset, size); > > +} > > I'm fine with this conceptually, but I think it would make more sense > if the name weren't specific to PCI or config space, e.g., > ioremap_nopost() or something. I agree with you since there is not much PCI specific in the ioremap implementation we are introducing (and it may be used more by other subsystems too), I suspect it is all about finding a reasonable ioremap_ naming extension (ioremap_nowb() ?). On the other hand having an explicit pci_remap_cfgspace() interface would simplify preventing drivers to sneak into the kernel with the wrong ioremap API for config space (even though when all existing ones are converted new drivers would just be based on existing ones so it should be fine). Suggestions welcome. Thanks ! Lorenzo
Hi Bjorn, Arnd, On Thu, Mar 16, 2017 at 04:12:43PM -0500, Bjorn Helgaas wrote: > [+cc Luis] > > On Mon, Feb 27, 2017 at 03:14:14PM +0000, Lorenzo Pieralisi wrote: > > The PCI specifications (Rev 3.0, 3.2.5 "Transaction Ordering and > > Posting") mandate non-posted configuration transactions. As further > > highlighted in the PCIe specifications (4.0 - Rev0.3, "Ordering > > Considerations for the Enhanced Configuration Access Mechanism"), > > through ECAM and ECAM-derivative configuration mechanism, the memory > > mapped transactions from the host CPU into Configuration Requests on the > > PCI express fabric may create ordering problems for software because > > writes to memory address are typically posted transactions (unless the > > architecture can enforce through virtual address mapping non-posted > > write transactions behaviour) but writes to Configuration Space are not > > posted on the PCI express fabric. > > > > Current DT and ACPI host bridge controllers map PCI configuration space > > (ECAM and ECAM-derivative) into the virtual address space through > > ioremap() calls, that are non-cacheable device accesses on most > > architectures, but may provide "bufferable" or "posted" write semantics > > in architecture like eg ARM/ARM64 that allow ioremap'ed regions writes > > to be buffered in the bus connecting the host CPU to the PCI fabric; > > this behaviour, as underlined in the PCIe specifications, may trigger > > transactions ordering rules and must be prevented. > > > > Introduce a new generic and explicit API to create a memory > > mapping for ECAM and ECAM-derivative config space area that > > defaults to ioremap_nocache() (which should provide a sane default > > behaviour) but still allowing architectures on which ioremap_nocache() > > results in posted write transactions to override the function > > call with an arch specific implementation that complies with > > the PCI specifications for configuration transactions. > > > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > Cc: Arnd Bergmann <arnd@arndb.de> > > Cc: Will Deacon <will.deacon@arm.com> > > Cc: Bjorn Helgaas <bhelgaas@google.com> > > Cc: Russell King <linux@armlinux.org.uk> > > Cc: Catalin Marinas <catalin.marinas@arm.com> > > --- > > include/asm-generic/io.h | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h > > index 7ef015e..52dda81 100644 > > --- a/include/asm-generic/io.h > > +++ b/include/asm-generic/io.h > > @@ -915,6 +915,15 @@ extern void ioport_unmap(void __iomem *p); > > #endif /* CONFIG_GENERIC_IOMAP */ > > #endif /* CONFIG_HAS_IOPORT_MAP */ > > > > +#ifndef pci_remap_cfgspace > > +#define pci_remap_cfgspace pci_remap_cfgspace > > +static inline void __iomem *pci_remap_cfgspace(phys_addr_t offset, > > + size_t size) > > +{ > > + return ioremap_nocache(offset, size); > > +} > > I'm fine with this conceptually, but I think it would make more sense > if the name weren't specific to PCI or config space, e.g., > ioremap_nopost() or something. I would like to respin shortly so I have to make a decision on the interface, either: (1) I keep it a PCI only interface (which means I can even move it to include/linux/pci.h and arch*/include/asm/pci.h to override it) or (2) We add it as a generic ioremap_* interface (I am not sure though that's really useful other than to map PCI config space, actually the reasoning behind the naming was to limit its usage to PCI config space mappings) I take it as Bjorn is keener on (2), just running a final check before putting v2 together to make progress. Thanks ! Lorenzo
On Wed, Mar 22, 2017 at 4:04 PM, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > Hi Bjorn, Arnd, > > On Thu, Mar 16, 2017 at 04:12:43PM -0500, Bjorn Helgaas wrote: >> [+cc Luis] >> >> On Mon, Feb 27, 2017 at 03:14:14PM +0000, Lorenzo Pieralisi wrote: >> > The PCI specifications (Rev 3.0, 3.2.5 "Transaction Ordering and >> > Posting") mandate non-posted configuration transactions. As further >> > highlighted in the PCIe specifications (4.0 - Rev0.3, "Ordering >> > Considerations for the Enhanced Configuration Access Mechanism"), >> > through ECAM and ECAM-derivative configuration mechanism, the memory >> > mapped transactions from the host CPU into Configuration Requests on the >> > PCI express fabric may create ordering problems for software because >> > writes to memory address are typically posted transactions (unless the >> > architecture can enforce through virtual address mapping non-posted >> > write transactions behaviour) but writes to Configuration Space are not >> > posted on the PCI express fabric. >> > >> > Current DT and ACPI host bridge controllers map PCI configuration space >> > (ECAM and ECAM-derivative) into the virtual address space through >> > ioremap() calls, that are non-cacheable device accesses on most >> > architectures, but may provide "bufferable" or "posted" write semantics >> > in architecture like eg ARM/ARM64 that allow ioremap'ed regions writes >> > to be buffered in the bus connecting the host CPU to the PCI fabric; >> > this behaviour, as underlined in the PCIe specifications, may trigger >> > transactions ordering rules and must be prevented. >> > >> > Introduce a new generic and explicit API to create a memory >> > mapping for ECAM and ECAM-derivative config space area that >> > defaults to ioremap_nocache() (which should provide a sane default >> > behaviour) but still allowing architectures on which ioremap_nocache() >> > results in posted write transactions to override the function >> > call with an arch specific implementation that complies with >> > the PCI specifications for configuration transactions. >> > >> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> >> > Cc: Arnd Bergmann <arnd@arndb.de> >> > Cc: Will Deacon <will.deacon@arm.com> >> > Cc: Bjorn Helgaas <bhelgaas@google.com> >> > Cc: Russell King <linux@armlinux.org.uk> >> > Cc: Catalin Marinas <catalin.marinas@arm.com> >> > --- >> > include/asm-generic/io.h | 9 +++++++++ >> > 1 file changed, 9 insertions(+) >> > >> > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h >> > index 7ef015e..52dda81 100644 >> > --- a/include/asm-generic/io.h >> > +++ b/include/asm-generic/io.h >> > @@ -915,6 +915,15 @@ extern void ioport_unmap(void __iomem *p); >> > #endif /* CONFIG_GENERIC_IOMAP */ >> > #endif /* CONFIG_HAS_IOPORT_MAP */ >> > >> > +#ifndef pci_remap_cfgspace >> > +#define pci_remap_cfgspace pci_remap_cfgspace >> > +static inline void __iomem *pci_remap_cfgspace(phys_addr_t offset, >> > + size_t size) >> > +{ >> > + return ioremap_nocache(offset, size); >> > +} >> >> I'm fine with this conceptually, but I think it would make more sense >> if the name weren't specific to PCI or config space, e.g., >> ioremap_nopost() or something. > > I would like to respin shortly so I have to make a decision on the > interface, either: > > (1) I keep it a PCI only interface (which means I can even move it to > include/linux/pci.h and arch*/include/asm/pci.h to override it) > > or > > (2) We add it as a generic ioremap_* interface (I am not sure though > that's really useful other than to map PCI config space, actually > the reasoning behind the naming was to limit its usage to PCI > config space mappings) > > I take it as Bjorn is keener on (2), just running a final check before > putting v2 together to make progress. I'm fine with either way. Arnd
On Wed, Mar 22, 2017 at 03:04:03PM +0000, Lorenzo Pieralisi wrote: > Hi Bjorn, Arnd, > > On Thu, Mar 16, 2017 at 04:12:43PM -0500, Bjorn Helgaas wrote: > > [+cc Luis] > > > > On Mon, Feb 27, 2017 at 03:14:14PM +0000, Lorenzo Pieralisi wrote: > > > The PCI specifications (Rev 3.0, 3.2.5 "Transaction Ordering and > > > Posting") mandate non-posted configuration transactions. As further > > > highlighted in the PCIe specifications (4.0 - Rev0.3, "Ordering > > > Considerations for the Enhanced Configuration Access Mechanism"), > > > through ECAM and ECAM-derivative configuration mechanism, the memory > > > mapped transactions from the host CPU into Configuration Requests on the > > > PCI express fabric may create ordering problems for software because > > > writes to memory address are typically posted transactions (unless the > > > architecture can enforce through virtual address mapping non-posted > > > write transactions behaviour) but writes to Configuration Space are not > > > posted on the PCI express fabric. > > > > > > Current DT and ACPI host bridge controllers map PCI configuration space > > > (ECAM and ECAM-derivative) into the virtual address space through > > > ioremap() calls, that are non-cacheable device accesses on most > > > architectures, but may provide "bufferable" or "posted" write semantics > > > in architecture like eg ARM/ARM64 that allow ioremap'ed regions writes > > > to be buffered in the bus connecting the host CPU to the PCI fabric; > > > this behaviour, as underlined in the PCIe specifications, may trigger > > > transactions ordering rules and must be prevented. > > > > > > Introduce a new generic and explicit API to create a memory > > > mapping for ECAM and ECAM-derivative config space area that > > > defaults to ioremap_nocache() (which should provide a sane default > > > behaviour) but still allowing architectures on which ioremap_nocache() > > > results in posted write transactions to override the function > > > call with an arch specific implementation that complies with > > > the PCI specifications for configuration transactions. > > > > > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > > Cc: Arnd Bergmann <arnd@arndb.de> > > > Cc: Will Deacon <will.deacon@arm.com> > > > Cc: Bjorn Helgaas <bhelgaas@google.com> > > > Cc: Russell King <linux@armlinux.org.uk> > > > Cc: Catalin Marinas <catalin.marinas@arm.com> > > > --- > > > include/asm-generic/io.h | 9 +++++++++ > > > 1 file changed, 9 insertions(+) > > > > > > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h > > > index 7ef015e..52dda81 100644 > > > --- a/include/asm-generic/io.h > > > +++ b/include/asm-generic/io.h > > > @@ -915,6 +915,15 @@ extern void ioport_unmap(void __iomem *p); > > > #endif /* CONFIG_GENERIC_IOMAP */ > > > #endif /* CONFIG_HAS_IOPORT_MAP */ > > > > > > +#ifndef pci_remap_cfgspace > > > +#define pci_remap_cfgspace pci_remap_cfgspace > > > +static inline void __iomem *pci_remap_cfgspace(phys_addr_t offset, > > > + size_t size) > > > +{ > > > + return ioremap_nocache(offset, size); > > > +} > > > > I'm fine with this conceptually, but I think it would make more sense > > if the name weren't specific to PCI or config space, e.g., > > ioremap_nopost() or something. > > I would like to respin shortly so I have to make a decision on the > interface, either: > > (1) I keep it a PCI only interface (which means I can even move it to > include/linux/pci.h and arch*/include/asm/pci.h to override it) > > or > > (2) We add it as a generic ioremap_* interface (I am not sure though > that's really useful other than to map PCI config space, actually > the reasoning behind the naming was to limit its usage to PCI > config space mappings) > > I take it as Bjorn is keener on (2), just running a final check before > putting v2 together to make progress. The point of this seems to be to use non-posted mappings for both I/O port space and ECAM (config) space. So I think the most readable way would be to have generic definitions like this: #ifndef ioremap_nopost #define ioremap_nopost ioremap_nocache #endif #ifndef pgprot_nonposted #define pgprot_nonposted(prot) pgprot_noncached(prot) #endif and let ARM/ARM64 implement their own versions of those. Then the devm code might fit naturally in lib/devres.c, e.g., void __iomem *devm_ioremap_nopost(...) { ... } and we probably wouldn't need the pci_remap_cfgspace() wrapper, e.g., we could do: return ioremap_page_range(..., pgprot_nonposted(PAGE_KERNEL)); cfg->win = ioremap_nopost(...); But I might be oversimplifying things. BTW, I tried to apply the v1 series on my "master" branch (v4.11-rc1) and patch 19 didn't apply cleanly. It's trivial and I can fix it up by hand, so not really a problem, just FYI. Bjorn
diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h index 7ef015e..52dda81 100644 --- a/include/asm-generic/io.h +++ b/include/asm-generic/io.h @@ -915,6 +915,15 @@ extern void ioport_unmap(void __iomem *p); #endif /* CONFIG_GENERIC_IOMAP */ #endif /* CONFIG_HAS_IOPORT_MAP */ +#ifndef pci_remap_cfgspace +#define pci_remap_cfgspace pci_remap_cfgspace +static inline void __iomem *pci_remap_cfgspace(phys_addr_t offset, + size_t size) +{ + return ioremap_nocache(offset, size); +} +#endif + #ifndef xlate_dev_kmem_ptr #define xlate_dev_kmem_ptr xlate_dev_kmem_ptr static inline void *xlate_dev_kmem_ptr(void *addr)
The PCI specifications (Rev 3.0, 3.2.5 "Transaction Ordering and Posting") mandate non-posted configuration transactions. As further highlighted in the PCIe specifications (4.0 - Rev0.3, "Ordering Considerations for the Enhanced Configuration Access Mechanism"), through ECAM and ECAM-derivative configuration mechanism, the memory mapped transactions from the host CPU into Configuration Requests on the PCI express fabric may create ordering problems for software because writes to memory address are typically posted transactions (unless the architecture can enforce through virtual address mapping non-posted write transactions behaviour) but writes to Configuration Space are not posted on the PCI express fabric. Current DT and ACPI host bridge controllers map PCI configuration space (ECAM and ECAM-derivative) into the virtual address space through ioremap() calls, that are non-cacheable device accesses on most architectures, but may provide "bufferable" or "posted" write semantics in architecture like eg ARM/ARM64 that allow ioremap'ed regions writes to be buffered in the bus connecting the host CPU to the PCI fabric; this behaviour, as underlined in the PCIe specifications, may trigger transactions ordering rules and must be prevented. Introduce a new generic and explicit API to create a memory mapping for ECAM and ECAM-derivative config space area that defaults to ioremap_nocache() (which should provide a sane default behaviour) but still allowing architectures on which ioremap_nocache() results in posted write transactions to override the function call with an arch specific implementation that complies with the PCI specifications for configuration transactions. Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Will Deacon <will.deacon@arm.com> Cc: Bjorn Helgaas <bhelgaas@google.com> Cc: Russell King <linux@armlinux.org.uk> Cc: Catalin Marinas <catalin.marinas@arm.com> --- include/asm-generic/io.h | 9 +++++++++ 1 file changed, 9 insertions(+)