* [PATCH v4 0/3] Fix ARM64 crash for accessing unmapped IO port regions @ 2019-06-11 14:12 John Garry 2019-06-11 14:12 ` [PATCH v4 1/3] lib: logic_pio: Use logical PIO low-level accessors for !CONFIG_INDIRECT_PIO John Garry ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: John Garry @ 2019-06-11 14:12 UTC (permalink / raw) To: bhelgaas, lorenzo.pieralisi, arnd Cc: linux-pci, rjw, linux-arm-kernel, will.deacon, wangkefeng.wang, linuxarm, andriy.shevchenko, linux-kernel, catalin.marinas, John Garry It was reported some time ago that arm64 systems will crash if a driver attempts to access IO port addresses when the PCI IO port region has not been mapped [1]. More recently, a similar crash is where the system PCI host probe fails, and the IPMI driver crashes the system while attempting to do some IO port accesses [2]. This patchset attempts to keep the kernel alive in such situations by rejecting logical PIO access to PCI IO regions until PCI IO port regions have been mapped. Accesses to unmapped regions fail silently, mimicking x86 behaviour. The previous versions of this patchset also included a patch to reject IO port resource requests until PCI IO port regions have been mapped (in a pci_remap_iospace() call). However I decided to drop it since the validity of the patch was strongly in doubt - [3]. 1. https://lore.kernel.org/linux-pci/56F209A9.4040304@huawei.com 2. https://lore.kernel.org/linux-arm-kernel/e6995b4a-184a-d8d4-f4d4-9ce75d8f47c0@huawei.com/ 3. https://lore.kernel.org/linux-pci/20190326224810.GY24180@google.com/ Differences to v3 patchset: https://lkml.org/lkml/2019/4/4/1294 - Drop patch to reject IO port requests - Make unmapped IO port accesses silent, mimicking x86 Differences to v2 patchset: https://lkml.org/lkml/2019/3/20/788 - Add a patch to use logical PIO accessors for !CONFIG_INDIRECT_PIO - Some tidy-up according to Andy's review Differences to v1 patchset: https://lkml.org/lkml/2019/3/14/630 - Drop f71805f fix - it can be done in a separate patchset - Change implementation in resource.c patch to check if parent of region is ioport_resource - Add patch to fix some logic_pio.c prints John Garry (3): lib: logic_pio: Use logical PIO low-level accessors for !CONFIG_INDIRECT_PIO lib: logic_pio: Reject accesses to unregistered CPU MMIO regions lib: logic_pio: Fix up a print include/linux/logic_pio.h | 7 ++- lib/logic_pio.c | 116 ++++++++++++++++++++++++++++---------- 2 files changed, 90 insertions(+), 33 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v4 1/3] lib: logic_pio: Use logical PIO low-level accessors for !CONFIG_INDIRECT_PIO 2019-06-11 14:12 [PATCH v4 0/3] Fix ARM64 crash for accessing unmapped IO port regions John Garry @ 2019-06-11 14:12 ` John Garry 2019-06-13 2:39 ` Bjorn Helgaas 2019-06-13 13:58 ` Bjorn Helgaas 2019-06-11 14:12 ` [PATCH v4 2/3] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions John Garry 2019-06-11 14:12 ` [PATCH v4 3/3] lib: logic_pio: Fix up a print John Garry 2 siblings, 2 replies; 17+ messages in thread From: John Garry @ 2019-06-11 14:12 UTC (permalink / raw) To: bhelgaas, lorenzo.pieralisi, arnd Cc: linux-pci, rjw, linux-arm-kernel, will.deacon, wangkefeng.wang, linuxarm, andriy.shevchenko, linux-kernel, catalin.marinas, John Garry Currently we only use logical PIO low-level accessors for when CONFIG_INDIRECT_PIO is enabled. Otherwise we just use inb/out et all directly. It is useful to now use logical PIO accessors for all cases, so we can add legality checks to accesses. Such a check would be for ensuring that the PCI IO port has been IO remapped prior to the access. Using the logical PIO accesses will add a little processing overhead, but that's ok as IO port accesses are relatively slow anyway. Some changes are also made to stop spilling so many lines under CONFIG_INDIRECT_PIO. Signed-off-by: John Garry <john.garry@huawei.com> --- include/linux/logic_pio.h | 7 ++-- lib/logic_pio.c | 83 ++++++++++++++++++++++++++++----------- 2 files changed, 63 insertions(+), 27 deletions(-) diff --git a/include/linux/logic_pio.h b/include/linux/logic_pio.h index cbd9d8495690..06d22b2ec99f 100644 --- a/include/linux/logic_pio.h +++ b/include/linux/logic_pio.h @@ -37,7 +37,7 @@ struct logic_pio_host_ops { size_t dwidth, unsigned int count); }; -#ifdef CONFIG_INDIRECT_PIO +#if defined(PCI_IOBASE) u8 logic_inb(unsigned long addr); void logic_outb(u8 value, unsigned long addr); void logic_outw(u16 value, unsigned long addr); @@ -102,6 +102,7 @@ void logic_outsl(unsigned long addr, const void *buffer, unsigned int count); #define outsl logic_outsl #endif +#if defined(CONFIG_INDIRECT_PIO) /* * We reserve 0x4000 bytes for Indirect IO as so far this library is only * used by the HiSilicon LPC Host. If needed, we can reserve a wider IO @@ -109,10 +110,10 @@ void logic_outsl(unsigned long addr, const void *buffer, unsigned int count); */ #define PIO_INDIRECT_SIZE 0x4000 #define MMIO_UPPER_LIMIT (IO_SPACE_LIMIT - PIO_INDIRECT_SIZE) -#else +#else /* CONFIG_INDIRECT_PIO */ #define MMIO_UPPER_LIMIT IO_SPACE_LIMIT #endif /* CONFIG_INDIRECT_PIO */ - +#endif /* PCI_IOBASE */ struct logic_pio_hwaddr *find_io_range_by_fwnode(struct fwnode_handle *fwnode); unsigned long logic_pio_trans_hwaddr(struct fwnode_handle *fwnode, resource_size_t hw_addr, resource_size_t size); diff --git a/lib/logic_pio.c b/lib/logic_pio.c index feea48fd1a0d..40d9428010e1 100644 --- a/lib/logic_pio.c +++ b/lib/logic_pio.c @@ -191,7 +191,8 @@ unsigned long logic_pio_trans_cpuaddr(resource_size_t addr) return ~0UL; } -#if defined(CONFIG_INDIRECT_PIO) && defined(PCI_IOBASE) +#if defined(PCI_IOBASE) +#if defined(CONFIG_INDIRECT_PIO) #define BUILD_LOGIC_IO(bw, type) \ type logic_in##bw(unsigned long addr) \ { \ @@ -200,11 +201,11 @@ type logic_in##bw(unsigned long addr) \ if (addr < MMIO_UPPER_LIMIT) { \ ret = read##bw(PCI_IOBASE + addr); \ } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { \ - struct logic_pio_hwaddr *entry = find_io_range(addr); \ + struct logic_pio_hwaddr *range = find_io_range(addr); \ + size_t sz = sizeof(type); \ \ - if (entry && entry->ops) \ - ret = entry->ops->in(entry->hostdata, \ - addr, sizeof(type)); \ + if (range && range->ops) \ + ret = range->ops->in(range->hostdata, addr, sz);\ else \ WARN_ON_ONCE(1); \ } \ @@ -216,49 +217,83 @@ void logic_out##bw(type value, unsigned long addr) \ if (addr < MMIO_UPPER_LIMIT) { \ write##bw(value, PCI_IOBASE + addr); \ } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { \ - struct logic_pio_hwaddr *entry = find_io_range(addr); \ + struct logic_pio_hwaddr *range = find_io_range(addr); \ + size_t sz = sizeof(type); \ \ - if (entry && entry->ops) \ - entry->ops->out(entry->hostdata, \ - addr, value, sizeof(type)); \ + if (range && range->ops) \ + range->ops->out(range->hostdata, \ + addr, value, sz); \ else \ WARN_ON_ONCE(1); \ } \ } \ \ -void logic_ins##bw(unsigned long addr, void *buffer, \ - unsigned int count) \ +void logic_ins##bw(unsigned long addr, void *buf, unsigned int cnt) \ { \ if (addr < MMIO_UPPER_LIMIT) { \ - reads##bw(PCI_IOBASE + addr, buffer, count); \ + reads##bw(PCI_IOBASE + addr, buf, cnt); \ } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { \ - struct logic_pio_hwaddr *entry = find_io_range(addr); \ + struct logic_pio_hwaddr *range = find_io_range(addr); \ + size_t sz = sizeof(type); \ \ - if (entry && entry->ops) \ - entry->ops->ins(entry->hostdata, \ - addr, buffer, sizeof(type), count); \ + if (range && range->ops) \ + range->ops->ins(range->hostdata, \ + addr, buf, sz, cnt); \ else \ WARN_ON_ONCE(1); \ } \ \ } \ \ -void logic_outs##bw(unsigned long addr, const void *buffer, \ - unsigned int count) \ +void logic_outs##bw(unsigned long addr, const void *buf, \ + unsigned int cnt) \ { \ if (addr < MMIO_UPPER_LIMIT) { \ - writes##bw(PCI_IOBASE + addr, buffer, count); \ + writes##bw(PCI_IOBASE + addr, buf, cnt); \ } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { \ - struct logic_pio_hwaddr *entry = find_io_range(addr); \ + struct logic_pio_hwaddr *range = find_io_range(addr); \ + size_t sz = sizeof(type); \ \ - if (entry && entry->ops) \ - entry->ops->outs(entry->hostdata, \ - addr, buffer, sizeof(type), count); \ + if (range && range->ops) \ + range->ops->outs(range->hostdata, \ + addr, buf, sz, cnt); \ else \ WARN_ON_ONCE(1); \ } \ } +#else /* CONFIG_INDIRECT_PIO */ + +#define BUILD_LOGIC_IO(bw, type) \ +type logic_in##bw(unsigned long addr) \ +{ \ + type ret = (type)~0; \ + \ + if (addr < MMIO_UPPER_LIMIT) \ + ret = read##bw(PCI_IOBASE + addr); \ + return ret; \ +} \ + \ +void logic_out##bw(type value, unsigned long addr) \ +{ \ + if (addr < MMIO_UPPER_LIMIT) \ + write##bw(value, PCI_IOBASE + addr); \ +} \ + \ +void logic_ins##bw(unsigned long addr, void *buf, unsigned int cnt) \ +{ \ + if (addr < MMIO_UPPER_LIMIT) \ + reads##bw(PCI_IOBASE + addr, buf, cnt); \ +} \ + \ +void logic_outs##bw(unsigned long addr, const void *buf, \ + unsigned int cnt) \ +{ \ + if (addr < MMIO_UPPER_LIMIT) \ + writes##bw(PCI_IOBASE + addr, buf, cnt); \ +} +#endif /* CONFIG_INDIRECT_PIO */ + BUILD_LOGIC_IO(b, u8) EXPORT_SYMBOL(logic_inb); EXPORT_SYMBOL(logic_insb); @@ -277,4 +312,4 @@ EXPORT_SYMBOL(logic_insl); EXPORT_SYMBOL(logic_outl); EXPORT_SYMBOL(logic_outsl); -#endif /* CONFIG_INDIRECT_PIO && PCI_IOBASE */ +#endif /* PCI_IOBASE */ -- 2.17.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/3] lib: logic_pio: Use logical PIO low-level accessors for !CONFIG_INDIRECT_PIO 2019-06-11 14:12 ` [PATCH v4 1/3] lib: logic_pio: Use logical PIO low-level accessors for !CONFIG_INDIRECT_PIO John Garry @ 2019-06-13 2:39 ` Bjorn Helgaas 2019-06-13 9:39 ` John Garry 2019-06-13 13:58 ` Bjorn Helgaas 1 sibling, 1 reply; 17+ messages in thread From: Bjorn Helgaas @ 2019-06-13 2:39 UTC (permalink / raw) To: John Garry Cc: lorenzo.pieralisi, arnd, linux-pci, rjw, linux-arm-kernel, will.deacon, wangkefeng.wang, linuxarm, andriy.shevchenko, linux-kernel, catalin.marinas On Tue, Jun 11, 2019 at 10:12:52PM +0800, John Garry wrote: > Currently we only use logical PIO low-level accessors for when > CONFIG_INDIRECT_PIO is enabled. > > Otherwise we just use inb/out et all directly. > > It is useful to now use logical PIO accessors for all cases, so we can add > legality checks to accesses. Such a check would be for ensuring that the > PCI IO port has been IO remapped prior to the access. IIUC, *this* patch doesn't actually add any additional checks, so no need to mention that in this commit log. One thing this patch *does* do is "#define inb logic_inb" whenever PCI_IOBASE is defined (we used to do that #define only when CONFIG_INDIRECT_PIO was defined). That's a pretty important change and needs to be very clear in the commit log. I'm not sure it's even safe, because CONFIG_INDIRECT_PIO depends on ARM64, but PCI_IOBASE is defined on most arches via asm-generic/io.h, so this potentially affects arches other than ARM64. If possible, split out the cleanup patches as below and make the patch that does this PCI_IOBASE change as small as possible so we can evaluate that change by itself. > Using the logical PIO accesses will add a little processing overhead, but > that's ok as IO port accesses are relatively slow anyway. > > Some changes are also made to stop spilling so many lines under > CONFIG_INDIRECT_PIO. "Some changes are also made" is a good hint to me that this patch might be able to be split up :) > Signed-off-by: John Garry <john.garry@huawei.com> > --- > include/linux/logic_pio.h | 7 ++-- > lib/logic_pio.c | 83 ++++++++++++++++++++++++++++----------- > 2 files changed, 63 insertions(+), 27 deletions(-) > > diff --git a/include/linux/logic_pio.h b/include/linux/logic_pio.h > index cbd9d8495690..06d22b2ec99f 100644 > --- a/include/linux/logic_pio.h > +++ b/include/linux/logic_pio.h > @@ -37,7 +37,7 @@ struct logic_pio_host_ops { > size_t dwidth, unsigned int count); > }; > > -#ifdef CONFIG_INDIRECT_PIO > +#if defined(PCI_IOBASE) Why change the #ifdef style? I understand these are equivalent, but unless there's a movement to change from "#ifdef X" to "#if defined(X)" I wouldn't bother. > u8 logic_inb(unsigned long addr); > void logic_outb(u8 value, unsigned long addr); > void logic_outw(u16 value, unsigned long addr); > @@ -102,6 +102,7 @@ void logic_outsl(unsigned long addr, const void *buffer, unsigned int count); > #define outsl logic_outsl > #endif > > +#if defined(CONFIG_INDIRECT_PIO) > /* > * We reserve 0x4000 bytes for Indirect IO as so far this library is only > * used by the HiSilicon LPC Host. If needed, we can reserve a wider IO > @@ -109,10 +110,10 @@ void logic_outsl(unsigned long addr, const void *buffer, unsigned int count); > */ > #define PIO_INDIRECT_SIZE 0x4000 > #define MMIO_UPPER_LIMIT (IO_SPACE_LIMIT - PIO_INDIRECT_SIZE) > -#else > +#else /* CONFIG_INDIRECT_PIO */ > #define MMIO_UPPER_LIMIT IO_SPACE_LIMIT > #endif /* CONFIG_INDIRECT_PIO */ > - > +#endif /* PCI_IOBASE */ > struct logic_pio_hwaddr *find_io_range_by_fwnode(struct fwnode_handle *fwnode); > unsigned long logic_pio_trans_hwaddr(struct fwnode_handle *fwnode, > resource_size_t hw_addr, resource_size_t size); > diff --git a/lib/logic_pio.c b/lib/logic_pio.c > index feea48fd1a0d..40d9428010e1 100644 > --- a/lib/logic_pio.c > +++ b/lib/logic_pio.c > @@ -191,7 +191,8 @@ unsigned long logic_pio_trans_cpuaddr(resource_size_t addr) > return ~0UL; > } > > -#if defined(CONFIG_INDIRECT_PIO) && defined(PCI_IOBASE) > +#if defined(PCI_IOBASE) > +#if defined(CONFIG_INDIRECT_PIO) > #define BUILD_LOGIC_IO(bw, type) \ > type logic_in##bw(unsigned long addr) \ > { \ > @@ -200,11 +201,11 @@ type logic_in##bw(unsigned long addr) \ > if (addr < MMIO_UPPER_LIMIT) { \ > ret = read##bw(PCI_IOBASE + addr); \ > } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { \ > - struct logic_pio_hwaddr *entry = find_io_range(addr); \ > + struct logic_pio_hwaddr *range = find_io_range(addr); \ > + size_t sz = sizeof(type); \ I don't mind changing "entry" to "range" and adding "sz". But that could be done in a separate "no functional change" patch that is trivial to review, which would make *this* patch smaller and easier to review. Another "no functional change" simplification patch would be to replace this: type ret = (type)~0; if (addr < MMIO_UPPER_LIMIT) { ret = read##bw(...); } else if (...) { if (range && range->ops) ret = range->ops->in(...); else WARN_ON_ONCE(); } return ret; with this: if (addr < MMIO_UPPER_LIMIT) return read##bw(...); if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { if (range && range->ops) return range->ops->in(...); else WARN_ON_ONCE(); } return (type)~0; Finally, I think the end result would be a little easier to read if you restructured the #ifdefs like this: #ifdef PCI_IOBASE #define BUILD_LOGIC_IO(...) type logic_in##bw(...) { if (addr < MMIO_UPPER_LIMIT) return read##bw(...); #ifdef CONFIG_INDIRECT_PIO if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { if (range && range->ops) return range->ops->in(...); else WARN_ON_ONCE(); } #endif return (type)~0; } That does mean a CONFIG_INDIRECT_PIO #ifdef in each in/out/ins/outs builder, but it's more localized so I think it's easier to understand that INDIRECT_PIO is just adding a new case to the default path. > - if (entry && entry->ops) \ > - ret = entry->ops->in(entry->hostdata, \ > - addr, sizeof(type)); \ > + if (range && range->ops) \ > + ret = range->ops->in(range->hostdata, addr, sz);\ > else \ > WARN_ON_ONCE(1); \ > } \ > @@ -216,49 +217,83 @@ void logic_out##bw(type value, unsigned long addr) \ > if (addr < MMIO_UPPER_LIMIT) { \ > write##bw(value, PCI_IOBASE + addr); \ > } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { \ > - struct logic_pio_hwaddr *entry = find_io_range(addr); \ > + struct logic_pio_hwaddr *range = find_io_range(addr); \ > + size_t sz = sizeof(type); \ > \ > - if (entry && entry->ops) \ > - entry->ops->out(entry->hostdata, \ > - addr, value, sizeof(type)); \ > + if (range && range->ops) \ > + range->ops->out(range->hostdata, \ > + addr, value, sz); \ > else \ > WARN_ON_ONCE(1); \ > } \ > } \ > \ > -void logic_ins##bw(unsigned long addr, void *buffer, \ > - unsigned int count) \ > +void logic_ins##bw(unsigned long addr, void *buf, unsigned int cnt) \ > { \ > if (addr < MMIO_UPPER_LIMIT) { \ > - reads##bw(PCI_IOBASE + addr, buffer, count); \ > + reads##bw(PCI_IOBASE + addr, buf, cnt); \ > } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { \ > - struct logic_pio_hwaddr *entry = find_io_range(addr); \ > + struct logic_pio_hwaddr *range = find_io_range(addr); \ > + size_t sz = sizeof(type); \ > \ > - if (entry && entry->ops) \ > - entry->ops->ins(entry->hostdata, \ > - addr, buffer, sizeof(type), count); \ > + if (range && range->ops) \ > + range->ops->ins(range->hostdata, \ > + addr, buf, sz, cnt); \ > else \ > WARN_ON_ONCE(1); \ > } \ > \ > } \ > \ > -void logic_outs##bw(unsigned long addr, const void *buffer, \ > - unsigned int count) \ > +void logic_outs##bw(unsigned long addr, const void *buf, \ > + unsigned int cnt) \ > { \ > if (addr < MMIO_UPPER_LIMIT) { \ > - writes##bw(PCI_IOBASE + addr, buffer, count); \ > + writes##bw(PCI_IOBASE + addr, buf, cnt); \ > } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { \ > - struct logic_pio_hwaddr *entry = find_io_range(addr); \ > + struct logic_pio_hwaddr *range = find_io_range(addr); \ > + size_t sz = sizeof(type); \ > \ > - if (entry && entry->ops) \ > - entry->ops->outs(entry->hostdata, \ > - addr, buffer, sizeof(type), count); \ > + if (range && range->ops) \ > + range->ops->outs(range->hostdata, \ > + addr, buf, sz, cnt); \ > else \ > WARN_ON_ONCE(1); \ > } \ > } > > +#else /* CONFIG_INDIRECT_PIO */ > + > +#define BUILD_LOGIC_IO(bw, type) \ > +type logic_in##bw(unsigned long addr) \ > +{ \ > + type ret = (type)~0; \ > + \ > + if (addr < MMIO_UPPER_LIMIT) \ > + ret = read##bw(PCI_IOBASE + addr); \ > + return ret; \ > +} \ > + \ > +void logic_out##bw(type value, unsigned long addr) \ > +{ \ > + if (addr < MMIO_UPPER_LIMIT) \ > + write##bw(value, PCI_IOBASE + addr); \ > +} \ > + \ > +void logic_ins##bw(unsigned long addr, void *buf, unsigned int cnt) \ > +{ \ > + if (addr < MMIO_UPPER_LIMIT) \ > + reads##bw(PCI_IOBASE + addr, buf, cnt); \ > +} \ > + \ > +void logic_outs##bw(unsigned long addr, const void *buf, \ > + unsigned int cnt) \ > +{ \ > + if (addr < MMIO_UPPER_LIMIT) \ > + writes##bw(PCI_IOBASE + addr, buf, cnt); \ > +} > +#endif /* CONFIG_INDIRECT_PIO */ > + > BUILD_LOGIC_IO(b, u8) > EXPORT_SYMBOL(logic_inb); > EXPORT_SYMBOL(logic_insb); > @@ -277,4 +312,4 @@ EXPORT_SYMBOL(logic_insl); > EXPORT_SYMBOL(logic_outl); > EXPORT_SYMBOL(logic_outsl); > > -#endif /* CONFIG_INDIRECT_PIO && PCI_IOBASE */ > +#endif /* PCI_IOBASE */ > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/3] lib: logic_pio: Use logical PIO low-level accessors for !CONFIG_INDIRECT_PIO 2019-06-13 2:39 ` Bjorn Helgaas @ 2019-06-13 9:39 ` John Garry 2019-06-13 20:09 ` Bjorn Helgaas 0 siblings, 1 reply; 17+ messages in thread From: John Garry @ 2019-06-13 9:39 UTC (permalink / raw) To: Bjorn Helgaas Cc: lorenzo.pieralisi, arnd, linux-pci, rjw, linux-arm-kernel, will.deacon, wangkefeng.wang, linuxarm, andriy.shevchenko, linux-kernel, catalin.marinas On 13/06/2019 03:39, Bjorn Helgaas wrote: > On Tue, Jun 11, 2019 at 10:12:52PM +0800, John Garry wrote: >> Currently we only use logical PIO low-level accessors for when >> CONFIG_INDIRECT_PIO is enabled. >> >> Otherwise we just use inb/out et all directly. >> Hi Bjorn, thanks for checking this. >> It is useful to now use logical PIO accessors for all cases, so we can add >> legality checks to accesses. Such a check would be for ensuring that the >> PCI IO port has been IO remapped prior to the access. > > IIUC, *this* patch doesn't actually add any ok, fine. I suppose that the subsequent patches in the series describe the motivation. additional checks, so no > need to mention that in this commit log. > > One thing this patch *does* do is "#define inb logic_inb" whenever > PCI_IOBASE is defined (we used to do that #define only when > CONFIG_INDIRECT_PIO was defined). Yes, right. > That's a pretty important change and needs to be very clear in the commit log. ok > > I'm not sure it's even safe, because CONFIG_INDIRECT_PIO depends on > ARM64, but PCI_IOBASE is defined on most arches via asm-generic/io.h, > so this potentially affects arches other than ARM64. It would do. It would affect any arch which defines PCI_IOBASE and does not have arch-specific definition of inb et all. > > If possible, split out the cleanup patches as below and make the patch > that does this PCI_IOBASE change as small as possible so we can > evaluate that change by itself. > Fine >> Using the logical PIO accesses will add a little processing overhead, but >> that's ok as IO port accesses are relatively slow anyway. >> >> Some changes are also made to stop spilling so many lines under >> CONFIG_INDIRECT_PIO. > > "Some changes are also made" is a good hint to me that this patch > might be able to be split up :) > >> Signed-off-by: John Garry <john.garry@huawei.com> >> --- >> include/linux/logic_pio.h | 7 ++-- >> lib/logic_pio.c | 83 ++++++++++++++++++++++++++++----------- >> 2 files changed, 63 insertions(+), 27 deletions(-) >> >> diff --git a/include/linux/logic_pio.h b/include/linux/logic_pio.h >> index cbd9d8495690..06d22b2ec99f 100644 >> --- a/include/linux/logic_pio.h >> +++ b/include/linux/logic_pio.h >> @@ -37,7 +37,7 @@ struct logic_pio_host_ops { >> size_t dwidth, unsigned int count); >> }; >> >> -#ifdef CONFIG_INDIRECT_PIO >> +#if defined(PCI_IOBASE) > > Why change the #ifdef style? I understand these are equivalent, but > unless there's a movement to change from "#ifdef X" to "#if defined(X)" > I wouldn't bother. Not intentional. I can keep this style. > >> u8 logic_inb(unsigned long addr); >> void logic_outb(u8 value, unsigned long addr); >> void logic_outw(u16 value, unsigned long addr); >> @@ -102,6 +102,7 @@ void logic_outsl(unsigned long addr, const void *buffer, unsigned int count); >> #define outsl logic_outsl >> #endif >> >> +#if defined(CONFIG_INDIRECT_PIO) >> /* >> * We reserve 0x4000 bytes for Indirect IO as so far this library is only >> * used by the HiSilicon LPC Host. If needed, we can reserve a wider IO >> @@ -109,10 +110,10 @@ void logic_outsl(unsigned long addr, const void *buffer, unsigned int count); >> */ >> #define PIO_INDIRECT_SIZE 0x4000 >> #define MMIO_UPPER_LIMIT (IO_SPACE_LIMIT - PIO_INDIRECT_SIZE) >> -#else >> +#else /* CONFIG_INDIRECT_PIO */ >> #define MMIO_UPPER_LIMIT IO_SPACE_LIMIT >> #endif /* CONFIG_INDIRECT_PIO */ >> - >> +#endif /* PCI_IOBASE */ >> struct logic_pio_hwaddr *find_io_range_by_fwnode(struct fwnode_handle *fwnode); >> unsigned long logic_pio_trans_hwaddr(struct fwnode_handle *fwnode, >> resource_size_t hw_addr, resource_size_t size); >> diff --git a/lib/logic_pio.c b/lib/logic_pio.c >> index feea48fd1a0d..40d9428010e1 100644 >> --- a/lib/logic_pio.c >> +++ b/lib/logic_pio.c >> @@ -191,7 +191,8 @@ unsigned long logic_pio_trans_cpuaddr(resource_size_t addr) >> return ~0UL; >> } >> >> -#if defined(CONFIG_INDIRECT_PIO) && defined(PCI_IOBASE) >> +#if defined(PCI_IOBASE) >> +#if defined(CONFIG_INDIRECT_PIO) >> #define BUILD_LOGIC_IO(bw, type) \ >> type logic_in##bw(unsigned long addr) \ >> { \ >> @@ -200,11 +201,11 @@ type logic_in##bw(unsigned long addr) \ >> if (addr < MMIO_UPPER_LIMIT) { \ >> ret = read##bw(PCI_IOBASE + addr); \ >> } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { \ >> - struct logic_pio_hwaddr *entry = find_io_range(addr); \ >> + struct logic_pio_hwaddr *range = find_io_range(addr); \ >> + size_t sz = sizeof(type); \ > > I don't mind changing "entry" to "range" and adding "sz". But that > could be done in a separate "no functional change" patch that is > trivial to review, which would make *this* patch smaller and easier to > review. ok > > Another "no functional change" simplification patch would be to > replace this: > > type ret = (type)~0; > > if (addr < MMIO_UPPER_LIMIT) { > ret = read##bw(...); > } else if (...) { > if (range && range->ops) > ret = range->ops->in(...); > else > WARN_ON_ONCE(); > } > return ret; > > with this: > > if (addr < MMIO_UPPER_LIMIT) > return read##bw(...); > > if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { > if (range && range->ops) > return range->ops->in(...); > else > WARN_ON_ONCE(); > } > > return (type)~0; > > Finally, I think the end result would be a little easier to read if > you restructured the #ifdefs like this: > > #ifdef PCI_IOBASE > #define BUILD_LOGIC_IO(...) > type logic_in##bw(...) > { > if (addr < MMIO_UPPER_LIMIT) > return read##bw(...); > > #ifdef CONFIG_INDIRECT_PIO I get your idea, but I don't think that that we can have an ifdef in macros (BUILD_LOGIC_IO) like this. > if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { > if (range && range->ops) > return range->ops->in(...); > else > WARN_ON_ONCE(); > } > #endif > > return (type)~0; > } > > That does mean a CONFIG_INDIRECT_PIO #ifdef in each in/out/ins/outs > builder, but it's more localized so I think it's easier to understand > that INDIRECT_PIO is just adding a new case to the default path. I'll see what I can do to improve the flow. But any change would also depend on your idea in response to patch v2, to unify the 2 types of logic_inb. > >> - if (entry && entry->ops) \ >> - ret = entry->ops->in(entry->hostdata, \ >> - addr, sizeof(type)); \ >> + if (range && range->ops) \ >> + ret = range->ops->in(range->hostdata, addr, sz);\ >> else \ >> WARN_ON_ONCE(1); \ >> } \ >> @@ -216,49 +217,83 @@ void logic_out##bw(type value, unsigned long addr) \ >> if (addr < MMIO_UPPER_LIMIT) { \ >> write##bw(value, PCI_IOBASE + addr); \ thanks again ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/3] lib: logic_pio: Use logical PIO low-level accessors for !CONFIG_INDIRECT_PIO 2019-06-13 9:39 ` John Garry @ 2019-06-13 20:09 ` Bjorn Helgaas 2019-06-14 9:02 ` John Garry 0 siblings, 1 reply; 17+ messages in thread From: Bjorn Helgaas @ 2019-06-13 20:09 UTC (permalink / raw) To: John Garry Cc: lorenzo.pieralisi, arnd, linux-pci, rjw, linux-arm-kernel, will.deacon, wangkefeng.wang, linuxarm, andriy.shevchenko, linux-kernel, catalin.marinas On Thu, Jun 13, 2019 at 10:39:12AM +0100, John Garry wrote: > On 13/06/2019 03:39, Bjorn Helgaas wrote: > > I'm not sure it's even safe, because CONFIG_INDIRECT_PIO depends on > > ARM64, but PCI_IOBASE is defined on most arches via asm-generic/io.h, > > so this potentially affects arches other than ARM64. > > It would do. It would affect any arch which defines PCI_IOBASE and > does not have arch-specific definition of inb et all. What's the reason for testing PCI_IOBASE instead of CONFIG_INDIRECT_PIO? If there's a reason it's needed, that's fine, but it does make this much more complicated to review. Bjorn ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/3] lib: logic_pio: Use logical PIO low-level accessors for !CONFIG_INDIRECT_PIO 2019-06-13 20:09 ` Bjorn Helgaas @ 2019-06-14 9:02 ` John Garry 2019-06-14 11:50 ` Bjorn Helgaas 0 siblings, 1 reply; 17+ messages in thread From: John Garry @ 2019-06-14 9:02 UTC (permalink / raw) To: Bjorn Helgaas Cc: lorenzo.pieralisi, arnd, linux-pci, rjw, linux-arm-kernel, will.deacon, wangkefeng.wang, linuxarm, andriy.shevchenko, linux-kernel, catalin.marinas On 13/06/2019 21:09, Bjorn Helgaas wrote: > On Thu, Jun 13, 2019 at 10:39:12AM +0100, John Garry wrote: >> On 13/06/2019 03:39, Bjorn Helgaas wrote: >>> I'm not sure it's even safe, because CONFIG_INDIRECT_PIO depends on >>> ARM64, but PCI_IOBASE is defined on most arches via asm-generic/io.h, >>> so this potentially affects arches other than ARM64. >> >> It would do. It would affect any arch which defines PCI_IOBASE and >> does not have arch-specific definition of inb et all. > Hi Bjorn, > What's the reason for testing PCI_IOBASE instead of > CONFIG_INDIRECT_PIO? If there's a reason it's needed, that's fine, > but it does make this much more complicated to review. For ARM64, we have PCI_IOBASE defined but may not have CONFIG_INDIRECT_PIO defined. Currently CONFIG_INDIRECT_PIO is only selected by CONFIG_HISILICON_LPC. As such, we should make this change also for when CONFIG_INDIRECT_PIO is not defined. Thanks, John > > Bjorn > > . > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/3] lib: logic_pio: Use logical PIO low-level accessors for !CONFIG_INDIRECT_PIO 2019-06-14 9:02 ` John Garry @ 2019-06-14 11:50 ` Bjorn Helgaas 2019-06-14 12:22 ` John Garry 0 siblings, 1 reply; 17+ messages in thread From: Bjorn Helgaas @ 2019-06-14 11:50 UTC (permalink / raw) To: John Garry Cc: lorenzo.pieralisi, arnd, linux-pci, rjw, linux-arm-kernel, will.deacon, wangkefeng.wang, linuxarm, andriy.shevchenko, linux-kernel, catalin.marinas On Fri, Jun 14, 2019 at 10:02:52AM +0100, John Garry wrote: > On 13/06/2019 21:09, Bjorn Helgaas wrote: > > On Thu, Jun 13, 2019 at 10:39:12AM +0100, John Garry wrote: > > > On 13/06/2019 03:39, Bjorn Helgaas wrote: > > > > I'm not sure it's even safe, because CONFIG_INDIRECT_PIO depends on > > > > ARM64, but PCI_IOBASE is defined on most arches via asm-generic/io.h, > > > > so this potentially affects arches other than ARM64. > > > > > > It would do. It would affect any arch which defines PCI_IOBASE and > > > does not have arch-specific definition of inb et all. > > > What's the reason for testing PCI_IOBASE instead of > > CONFIG_INDIRECT_PIO? If there's a reason it's needed, that's fine, > > but it does make this much more complicated to review. > > For ARM64, we have PCI_IOBASE defined but may not have > CONFIG_INDIRECT_PIO defined. Currently CONFIG_INDIRECT_PIO is only > selected by CONFIG_HISILICON_LPC. > > As such, we should make this change also for when > CONFIG_INDIRECT_PIO is not defined. OK. This is all very important for the commit log -- we need to understand what arches are affected and the reason they need it. Since the goal of this series is to fix an ARM64-specific issue, and the typical port I/O model is for each arch to #define its own inb(), maybe it would make sense to move the "#define inb logic_inb" from linux/logic_pio.h to arm64/include/asm/io.h? The "#ifndef inb" arrangement gets pretty complicated when it occurs more than one place (asm-generic/io.h and logic_pio.h) and we have to start worrying about the ordering of #includes. Bjorn ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/3] lib: logic_pio: Use logical PIO low-level accessors for !CONFIG_INDIRECT_PIO 2019-06-14 11:50 ` Bjorn Helgaas @ 2019-06-14 12:22 ` John Garry 0 siblings, 0 replies; 17+ messages in thread From: John Garry @ 2019-06-14 12:22 UTC (permalink / raw) To: Bjorn Helgaas Cc: rjw, wangkefeng.wang, lorenzo.pieralisi, arnd, linux-pci, will.deacon, linuxarm, linux-kernel, catalin.marinas, andriy.shevchenko, linux-arm-kernel On 14/06/2019 12:50, Bjorn Helgaas wrote: > On Fri, Jun 14, 2019 at 10:02:52AM +0100, John Garry wrote: >> On 13/06/2019 21:09, Bjorn Helgaas wrote: >>> On Thu, Jun 13, 2019 at 10:39:12AM +0100, John Garry wrote: >>>> On 13/06/2019 03:39, Bjorn Helgaas wrote: >>>>> I'm not sure it's even safe, because CONFIG_INDIRECT_PIO depends on >>>>> ARM64, but PCI_IOBASE is defined on most arches via asm-generic/io.h, >>>>> so this potentially affects arches other than ARM64. >>>> >>>> It would do. It would affect any arch which defines PCI_IOBASE and >>>> does not have arch-specific definition of inb et all. >> >>> What's the reason for testing PCI_IOBASE instead of >>> CONFIG_INDIRECT_PIO? If there's a reason it's needed, that's fine, >>> but it does make this much more complicated to review. >> >> For ARM64, we have PCI_IOBASE defined but may not have >> CONFIG_INDIRECT_PIO defined. Currently CONFIG_INDIRECT_PIO is only >> selected by CONFIG_HISILICON_LPC. >> >> As such, we should make this change also for when >> CONFIG_INDIRECT_PIO is not defined. Hi Bjorn, > > OK. This is all very important for the commit log -- we need to > understand what arches are affected and the reason they need it. Right, and to repeat, this would affect other archs which define PCI_IOBASE and don't have custom IO port accessors definitions. There are a few remaining even after the recent arch clear out . I have it at arm64, microblaze, and unicore32. Arch m68k defines PCI_IOBASE but seems to have its own IO port accessors. Same again for some arm machines. At least I should cc those arch maintainers. > > Since the goal of this series is to fix an ARM64-specific issue, "ARM64" was in the headline banner, but it would apply to other archs, as mentioned above. I should have made that clearer. and > the typical port I/O model is for each arch to #define its own inb(), > maybe it would make sense to move the "#define inb logic_inb" from > linux/logic_pio.h to arm64/include/asm/io.h? > CONFIG_INDIRECT_PIO has been indirectly enabled in ARM64 defconfig for some time, so I think that it's ok for ARM64 arch Kconfig to select it at this stage. From that, we could make the change suggested. And, in addition to that, we can make the change in this series just for CONFIG_INDIRECT_PIO. But I think that the other archs, above, could benefit from the changes in this series, so it would be shame to omit them. > The "#ifndef inb" arrangement gets pretty complicated when it occurs > more than one place (asm-generic/io.h and logic_pio.h) and we have to > start worrying about the ordering of #includes. I agree on that. Cheers, John > > Bjorn > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/3] lib: logic_pio: Use logical PIO low-level accessors for !CONFIG_INDIRECT_PIO 2019-06-11 14:12 ` [PATCH v4 1/3] lib: logic_pio: Use logical PIO low-level accessors for !CONFIG_INDIRECT_PIO John Garry 2019-06-13 2:39 ` Bjorn Helgaas @ 2019-06-13 13:58 ` Bjorn Helgaas 2019-06-13 15:21 ` John Garry 1 sibling, 1 reply; 17+ messages in thread From: Bjorn Helgaas @ 2019-06-13 13:58 UTC (permalink / raw) To: John Garry Cc: lorenzo.pieralisi, arnd, linux-pci, rjw, linux-arm-kernel, will.deacon, wangkefeng.wang, linuxarm, andriy.shevchenko, linux-kernel, catalin.marinas On Tue, Jun 11, 2019 at 10:12:52PM +0800, John Garry wrote: Another thought here: > if (addr < MMIO_UPPER_LIMIT) { \ > ret = read##bw(PCI_IOBASE + addr); \ > } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { \ > - struct logic_pio_hwaddr *entry = find_io_range(addr); \ > + struct logic_pio_hwaddr *range = find_io_range(addr); \ > + size_t sz = sizeof(type); \ > \ > - if (entry && entry->ops) \ > - ret = entry->ops->in(entry->hostdata, \ > - addr, sizeof(type)); \ > + if (range && range->ops) \ > + ret = range->ops->in(range->hostdata, addr, sz);\ > else \ > WARN_ON_ONCE(1); \ Could this be simplified a little by requiring callers to set range->ops for LOGIC_PIO_INDIRECT ranges *before* calling logic_pio_register_range()? E.g., hisi_lpc_probe(...) { range = devm_kzalloc(...); range->flags = LOGIC_PIO_INDIRECT; range->ops = &hisi_lpc_ops; logic_pio_register_range(range); ... and logic_pio_register_range(struct logic_pio_hwaddr *new_range) { if (new_range->flags == LOGIC_PIO_INDIRECT && !new_range->ops) return -EINVAL; ... Then maybe you wouldn't need to check range->ops in the accessors. Bjorn ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/3] lib: logic_pio: Use logical PIO low-level accessors for !CONFIG_INDIRECT_PIO 2019-06-13 13:58 ` Bjorn Helgaas @ 2019-06-13 15:21 ` John Garry 0 siblings, 0 replies; 17+ messages in thread From: John Garry @ 2019-06-13 15:21 UTC (permalink / raw) To: Bjorn Helgaas Cc: lorenzo.pieralisi, arnd, linux-pci, rjw, linux-arm-kernel, will.deacon, wangkefeng.wang, linuxarm, andriy.shevchenko, linux-kernel, catalin.marinas On 13/06/2019 14:58, Bjorn Helgaas wrote: > On Tue, Jun 11, 2019 at 10:12:52PM +0800, John Garry wrote: > Another thought here: > >> if (addr < MMIO_UPPER_LIMIT) { \ >> ret = read##bw(PCI_IOBASE + addr); \ >> } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { \ >> - struct logic_pio_hwaddr *entry = find_io_range(addr); \ >> + struct logic_pio_hwaddr *range = find_io_range(addr); \ >> + size_t sz = sizeof(type); \ >> \ >> - if (entry && entry->ops) \ >> - ret = entry->ops->in(entry->hostdata, \ >> - addr, sizeof(type)); \ >> + if (range && range->ops) \ >> + ret = range->ops->in(range->hostdata, addr, sz);\ >> else \ >> WARN_ON_ONCE(1); Hi Bjorn, \ > > Could this be simplified a little by requiring callers to set > range->ops for LOGIC_PIO_INDIRECT ranges *before* calling > logic_pio_register_range()? E.g., > > hisi_lpc_probe(...) > { > range = devm_kzalloc(...); > range->flags = LOGIC_PIO_INDIRECT; > range->ops = &hisi_lpc_ops; > logic_pio_register_range(range); > ... > > and > > logic_pio_register_range(struct logic_pio_hwaddr *new_range) > { > if (new_range->flags == LOGIC_PIO_INDIRECT && !new_range->ops) > return -EINVAL; > ... > > Then maybe you wouldn't need to check range->ops in the accessors. > I think I know the reason why it was done this way. So currently there is no method to unregister a logical PIO region (the old code leaked ranges as well). As such, if hisi_lpc_probe() fails after we register the logical PIO range, there would be a range registered but no actual host backing it. So we set the ops at the point at which the probe cannot fail to avoid a potential problem. And now I realise that there is a bug in the code - range is allocated with devm_kzalloc and is passed to logic_pio_register_range(). As such, if the hisi_lpc_probe() goes on to fail, then this memory would be free'd and we have an issue. PCI code should be ok as it uses kzalloc(). The simplest solution is to not change the logical PIO API to allocate this memory itself, but rather make hisi_lpc_probe() use kzalloc(). And, if we go this way, we can use your idea to set the ops. I'll spin a separate patch for this. Thanks, John > Bjorn > > . > ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v4 2/3] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions 2019-06-11 14:12 [PATCH v4 0/3] Fix ARM64 crash for accessing unmapped IO port regions John Garry 2019-06-11 14:12 ` [PATCH v4 1/3] lib: logic_pio: Use logical PIO low-level accessors for !CONFIG_INDIRECT_PIO John Garry @ 2019-06-11 14:12 ` John Garry 2019-06-13 3:20 ` Bjorn Helgaas 2019-06-11 14:12 ` [PATCH v4 3/3] lib: logic_pio: Fix up a print John Garry 2 siblings, 1 reply; 17+ messages in thread From: John Garry @ 2019-06-11 14:12 UTC (permalink / raw) To: bhelgaas, lorenzo.pieralisi, arnd Cc: linux-pci, rjw, linux-arm-kernel, will.deacon, wangkefeng.wang, linuxarm, andriy.shevchenko, linux-kernel, catalin.marinas, John Garry Currently when accessing logical indirect PIO addresses in logic_{in, out}{,s}, we first ensure that the region is registered. However, no such check exists for CPU MMIO regions. The CPU MMIO regions would be registered by the PCI host (when PCI_IOBASE is defined) in pci_register_io_range(). We have seen scenarios when systems which don't have a PCI host, or they do but the PCI host probe fails, certain drivers attempts to still attempt to access PCI IO ports; examples are in [1] and [2]. Such is a case on an ARM64 system without a PCI host: root@(none)$ insmod hwmon/f71805f.ko Unable to handle kernel paging request at virtual address ffff7dfffee0002e Mem abort info: ESR = 0x96000046 Exception class = DABT (current EL), IL = 32 bits SET = 0, FnV = 0 EA = 0, S1PTW = 0 Data abort info: ISV = 0, ISS = 0x00000046 CM = 0, WnR = 1 swapper pgtable: 4k pages, 48-bit VAs, pgdp = (____ptrval____) [ffff7dfffee0002e] pgd=000000000141c003, pud=000000000141d003, pmd=0000000000000000 Internal error: Oops: 96000046 [#1] PREEMPT SMP Modules linked in: f71805f(+) CPU: 20 PID: 2736 Comm: insmod Not tainted 5.1.0-rc1-00003-g6f1bfec2a620-dirty #99 Hardware name: Huawei Taishan 2280 /D05, BIOS Hisilicon D05 IT21 Nemo 2.0 RC0 04/18/2018 pstate: 80000005 (Nzcv daif -PAN -UAO) pc : logic_outb+0x54/0xb8 lr : f71805f_find+0x2c/0x1b8 [f71805f] sp : ffff000025fbba90 x29: ffff000025fbba90 x28: ffff000008b944d0 x27: ffff000025fbbdf0 x26: 0000000000000100 x25: ffff801f8c270580 x24: ffff000011420000 x23: ffff000025fbbb3e x22: ffff000025fbbb40 x21: ffff000008b991b8 x20: 0000000000000087 x19: 000000000000002e x18: ffffffffffffffff x17: 0000000000000000 x16: 0000000000000000 x15: ffff00001127d6c8 x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000000 x11: 0000000000010820 x10: 0000841fdac40000 x9 : 0000000000000001 x8 : 0000000040000000 x7 : 0000000000210d00 x6 : 0000000000000000 x5 : ffff801fb6a46040 x4 : ffff841febeaeda0 x3 : 0000000000ffbffe x2 : ffff000025fbbb40 x1 : ffff7dfffee0002e x0 : ffff7dfffee00000 Process insmod (pid: 2736, stack limit = 0x(____ptrval____)) Call trace: logic_outb+0x54/0xb8 f71805f_find+0x2c/0x1b8 [f71805f] f71805f_init+0x38/0xe48 [f71805f] do_one_initcall+0x5c/0x198 do_init_module+0x54/0x1b0 load_module+0x1dc4/0x2158 __se_sys_init_module+0x14c/0x1e8 __arm64_sys_init_module+0x18/0x20 el0_svc_common+0x5c/0x100 el0_svc_handler+0x2c/0x80 el0_svc+0x8/0xc Code: d2bfdc00 f2cfbfe0 f2ffffe0 8b000021 (39000034) ---[ end trace 10ea80bde051bbfc ]--- root@(none)$ Well-behaved drivers call request_{muxed_}region() to grab the IO port region, but success here still doesn't actually mean that there is some IO port mapped in this region. This patch adds a check to ensure that the CPU MMIO region is registered prior to accessing the PCI IO ports. Any failed checks silently return. [1] https://lore.kernel.org/linux-pci/56F209A9.4040304@huawei.com [2] https://lore.kernel.org/linux-arm-kernel/e6995b4a-184a-d8d4-f4d4-9ce75d8f47c0@huawei.com/ Signed-off-by: John Garry <john.garry@huawei.com> --- lib/logic_pio.c | 60 +++++++++++++++++++++++++++++++++---------------- 1 file changed, 41 insertions(+), 19 deletions(-) diff --git a/lib/logic_pio.c b/lib/logic_pio.c index 40d9428010e1..47d24f428908 100644 --- a/lib/logic_pio.c +++ b/lib/logic_pio.c @@ -126,7 +126,7 @@ static struct logic_pio_hwaddr *find_io_range(unsigned long pio) if (in_range(pio, range->io_start, range->size)) return range; } - pr_err("PIO entry token %lx invalid\n", pio); + return NULL; } @@ -197,11 +197,12 @@ unsigned long logic_pio_trans_cpuaddr(resource_size_t addr) type logic_in##bw(unsigned long addr) \ { \ type ret = (type)~0; \ + struct logic_pio_hwaddr *range = find_io_range(addr); \ \ if (addr < MMIO_UPPER_LIMIT) { \ - ret = read##bw(PCI_IOBASE + addr); \ + if (range) \ + ret = read##bw(PCI_IOBASE + addr); \ } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { \ - struct logic_pio_hwaddr *range = find_io_range(addr); \ size_t sz = sizeof(type); \ \ if (range && range->ops) \ @@ -214,10 +215,12 @@ type logic_in##bw(unsigned long addr) \ \ void logic_out##bw(type value, unsigned long addr) \ { \ + struct logic_pio_hwaddr *range = find_io_range(addr); \ + \ if (addr < MMIO_UPPER_LIMIT) { \ - write##bw(value, PCI_IOBASE + addr); \ + if (range) \ + write##bw(value, PCI_IOBASE + addr); \ } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { \ - struct logic_pio_hwaddr *range = find_io_range(addr); \ size_t sz = sizeof(type); \ \ if (range && range->ops) \ @@ -230,10 +233,12 @@ void logic_out##bw(type value, unsigned long addr) \ \ void logic_ins##bw(unsigned long addr, void *buf, unsigned int cnt) \ { \ + struct logic_pio_hwaddr *range = find_io_range(addr); \ + \ if (addr < MMIO_UPPER_LIMIT) { \ - reads##bw(PCI_IOBASE + addr, buf, cnt); \ + if (range) \ + reads##bw(PCI_IOBASE + addr, buf, cnt); \ } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { \ - struct logic_pio_hwaddr *range = find_io_range(addr); \ size_t sz = sizeof(type); \ \ if (range && range->ops) \ @@ -242,16 +247,17 @@ void logic_ins##bw(unsigned long addr, void *buf, unsigned int cnt) \ else \ WARN_ON_ONCE(1); \ } \ - \ } \ \ void logic_outs##bw(unsigned long addr, const void *buf, \ unsigned int cnt) \ { \ + struct logic_pio_hwaddr *range = find_io_range(addr); \ + \ if (addr < MMIO_UPPER_LIMIT) { \ - writes##bw(PCI_IOBASE + addr, buf, cnt); \ + if (range) \ + writes##bw(PCI_IOBASE + addr, buf, cnt); \ } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { \ - struct logic_pio_hwaddr *range = find_io_range(addr); \ size_t sz = sizeof(type); \ \ if (range && range->ops) \ @@ -269,28 +275,44 @@ type logic_in##bw(unsigned long addr) \ { \ type ret = (type)~0; \ \ - if (addr < MMIO_UPPER_LIMIT) \ - ret = read##bw(PCI_IOBASE + addr); \ + if (addr < MMIO_UPPER_LIMIT) { \ + struct logic_pio_hwaddr *range = find_io_range(addr); \ + \ + if (range) \ + ret = read##bw(PCI_IOBASE + addr); \ + } \ return ret; \ } \ \ -void logic_out##bw(type value, unsigned long addr) \ +void logic_out##bw(type val, unsigned long addr) \ { \ - if (addr < MMIO_UPPER_LIMIT) \ - write##bw(value, PCI_IOBASE + addr); \ + if (addr < MMIO_UPPER_LIMIT) { \ + struct logic_pio_hwaddr *range = find_io_range(addr); \ + \ + if (range) \ + write##bw(val, PCI_IOBASE + addr); \ + } \ } \ \ void logic_ins##bw(unsigned long addr, void *buf, unsigned int cnt) \ { \ - if (addr < MMIO_UPPER_LIMIT) \ - reads##bw(PCI_IOBASE + addr, buf, cnt); \ + if (addr < MMIO_UPPER_LIMIT) { \ + struct logic_pio_hwaddr *range = find_io_range(addr); \ + \ + if (range) \ + reads##bw(PCI_IOBASE + addr, buf, cnt); \ + } \ } \ \ void logic_outs##bw(unsigned long addr, const void *buf, \ unsigned int cnt) \ { \ - if (addr < MMIO_UPPER_LIMIT) \ - writes##bw(PCI_IOBASE + addr, buf, cnt); \ + if (addr < MMIO_UPPER_LIMIT) { \ + struct logic_pio_hwaddr *range = find_io_range(addr); \ + \ + if (range) \ + writes##bw(PCI_IOBASE + addr, buf, cnt); \ + } \ } #endif /* CONFIG_INDIRECT_PIO */ -- 2.17.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v4 2/3] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions 2019-06-11 14:12 ` [PATCH v4 2/3] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions John Garry @ 2019-06-13 3:20 ` Bjorn Helgaas 2019-06-13 7:47 ` Arnd Bergmann 2019-06-13 10:17 ` John Garry 0 siblings, 2 replies; 17+ messages in thread From: Bjorn Helgaas @ 2019-06-13 3:20 UTC (permalink / raw) To: John Garry Cc: lorenzo.pieralisi, arnd, linux-pci, rjw, linux-arm-kernel, will.deacon, wangkefeng.wang, linuxarm, andriy.shevchenko, linux-kernel, catalin.marinas On Tue, Jun 11, 2019 at 10:12:53PM +0800, John Garry wrote: > Currently when accessing logical indirect PIO addresses in > logic_{in, out}{,s}, we first ensure that the region is registered. I think logic_pio is specifically concerned with I/O port space, so it's a little bit unfortunate that we named this "PIO". PIO is a general term for "Programmed I/O", which just means the CPU is involved in each transfer, as opposed to DMA. The transfers can be to either MMIO or I/O port space. So this ends up being a little confusing because I think you mean "Port I/O", not "Programmed I/O". > However, no such check exists for CPU MMIO regions. The CPU MMIO regions > would be registered by the PCI host (when PCI_IOBASE is defined) in > pci_register_io_range(). IIUC this "CPU MMIO region" is an MMIO region where a memory load or store from the CPU is converted by a PCI host bridge into an I/O port transaction on PCI. Again IIUC, logic_pio supports two kinds of I/O port space accesses: 1) The simple "bridge converts loads/stores to an MMIO region to PCI I/O port transactions" kind, and 2) The more complicated "somebody supplies logic_pio_host_ops functions that do arbitrary magic to generate I/O port transactions on some bus. And this patch is making the first kind smarter. Previously it would perform the memory access whenever "addr < MMIO_UPPER_LIMIT", but after this patch it will only do it if find_io_range() succeeds. Right? Sorry for restating what probably should have been obvious to me. I have two observations here: 1) The simple "bridge converts CPU MMIO space to PCI I/O port space" flavor is essentially identical to what ia64 (and probably other architectures) does. This should really be combined somehow. 2) If you made a default set of logic_pio_host_ops that merely did loads/stores and maybe added a couple fields in the struct logic_pio_hwaddr, I bet you could unify the two kinds so logic_inb() would look something like this: u8 logic_inb(unsigned long addr) { struct logic_pio_hwaddr *range = find_io_range(addr); if (!range) return (u8) ~0; return (u8) range->ops->in(range->hostdata, addr, sz); } > We have seen scenarios when systems which don't have a PCI host, or they > do but the PCI host probe fails, certain drivers attempts to still attempt > to access PCI IO ports; examples are in [1] and [2]. > > Such is a case on an ARM64 system without a PCI host: > > root@(none)$ insmod hwmon/f71805f.ko > Unable to handle kernel paging request at virtual address ffff7dfffee0002e > Mem abort info: > ESR = 0x96000046 > Exception class = DABT (current EL), IL = 32 bits > SET = 0, FnV = 0 > EA = 0, S1PTW = 0 > Data abort info: > ISV = 0, ISS = 0x00000046 > CM = 0, WnR = 1 > swapper pgtable: 4k pages, 48-bit VAs, pgdp = (____ptrval____) > [ffff7dfffee0002e] pgd=000000000141c003, pud=000000000141d003, pmd=0000000000000000 > Internal error: Oops: 96000046 [#1] PREEMPT SMP > Modules linked in: f71805f(+) > CPU: 20 PID: 2736 Comm: insmod Not tainted 5.1.0-rc1-00003-g6f1bfec2a620-dirty #99 > Hardware name: Huawei Taishan 2280 /D05, BIOS Hisilicon D05 IT21 Nemo 2.0 RC0 04/18/2018 > pstate: 80000005 (Nzcv daif -PAN -UAO) > pc : logic_outb+0x54/0xb8 > lr : f71805f_find+0x2c/0x1b8 [f71805f] > sp : ffff000025fbba90 > x29: ffff000025fbba90 x28: ffff000008b944d0 > x27: ffff000025fbbdf0 x26: 0000000000000100 > x25: ffff801f8c270580 x24: ffff000011420000 > x23: ffff000025fbbb3e x22: ffff000025fbbb40 > x21: ffff000008b991b8 x20: 0000000000000087 > x19: 000000000000002e x18: ffffffffffffffff > x17: 0000000000000000 x16: 0000000000000000 > x15: ffff00001127d6c8 x14: 0000000000000000 > x13: 0000000000000000 x12: 0000000000000000 > x11: 0000000000010820 x10: 0000841fdac40000 > x9 : 0000000000000001 x8 : 0000000040000000 > x7 : 0000000000210d00 x6 : 0000000000000000 > x5 : ffff801fb6a46040 x4 : ffff841febeaeda0 > x3 : 0000000000ffbffe x2 : ffff000025fbbb40 > x1 : ffff7dfffee0002e x0 : ffff7dfffee00000 > Process insmod (pid: 2736, stack limit = 0x(____ptrval____)) > Call trace: > logic_outb+0x54/0xb8 > f71805f_find+0x2c/0x1b8 [f71805f] > f71805f_init+0x38/0xe48 [f71805f] > do_one_initcall+0x5c/0x198 > do_init_module+0x54/0x1b0 > load_module+0x1dc4/0x2158 > __se_sys_init_module+0x14c/0x1e8 > __arm64_sys_init_module+0x18/0x20 > el0_svc_common+0x5c/0x100 > el0_svc_handler+0x2c/0x80 > el0_svc+0x8/0xc > Code: d2bfdc00 f2cfbfe0 f2ffffe0 8b000021 (39000034) > ---[ end trace 10ea80bde051bbfc ]--- > root@(none)$ > > Well-behaved drivers call request_{muxed_}region() to grab the IO port > region, but success here still doesn't actually mean that there is some IO > port mapped in this region. > > This patch adds a check to ensure that the CPU MMIO region is registered > prior to accessing the PCI IO ports. > > Any failed checks silently return. I *think* what you're doing here is making inb/outb/etc work the same as on x86, i.e., if no device responds to an inb(), the caller gets ~0, and if no device claims an outb() the data gets dropped. That should be explicit in the commit log. > [1] https://lore.kernel.org/linux-pci/56F209A9.4040304@huawei.com > [2] https://lore.kernel.org/linux-arm-kernel/e6995b4a-184a-d8d4-f4d4-9ce75d8f47c0@huawei.com/ > > Signed-off-by: John Garry <john.garry@huawei.com> > --- > lib/logic_pio.c | 60 +++++++++++++++++++++++++++++++++---------------- > 1 file changed, 41 insertions(+), 19 deletions(-) > > diff --git a/lib/logic_pio.c b/lib/logic_pio.c > index 40d9428010e1..47d24f428908 100644 > --- a/lib/logic_pio.c > +++ b/lib/logic_pio.c > @@ -126,7 +126,7 @@ static struct logic_pio_hwaddr *find_io_range(unsigned long pio) > if (in_range(pio, range->io_start, range->size)) > return range; > } > - pr_err("PIO entry token %lx invalid\n", pio); > + > return NULL; > } > > @@ -197,11 +197,12 @@ unsigned long logic_pio_trans_cpuaddr(resource_size_t addr) > type logic_in##bw(unsigned long addr) \ > { \ > type ret = (type)~0; \ > + struct logic_pio_hwaddr *range = find_io_range(addr); \ > \ > if (addr < MMIO_UPPER_LIMIT) { \ > - ret = read##bw(PCI_IOBASE + addr); \ > + if (range) \ > + ret = read##bw(PCI_IOBASE + addr); \ > } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { \ > - struct logic_pio_hwaddr *range = find_io_range(addr); \ > size_t sz = sizeof(type); \ > \ > if (range && range->ops) \ > @@ -214,10 +215,12 @@ type logic_in##bw(unsigned long addr) \ > \ > void logic_out##bw(type value, unsigned long addr) \ > { \ > + struct logic_pio_hwaddr *range = find_io_range(addr); \ > + \ > if (addr < MMIO_UPPER_LIMIT) { \ > - write##bw(value, PCI_IOBASE + addr); \ > + if (range) \ > + write##bw(value, PCI_IOBASE + addr); \ > } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { \ > - struct logic_pio_hwaddr *range = find_io_range(addr); \ > size_t sz = sizeof(type); \ > \ > if (range && range->ops) \ > @@ -230,10 +233,12 @@ void logic_out##bw(type value, unsigned long addr) \ > \ > void logic_ins##bw(unsigned long addr, void *buf, unsigned int cnt) \ > { \ > + struct logic_pio_hwaddr *range = find_io_range(addr); \ > + \ > if (addr < MMIO_UPPER_LIMIT) { \ > - reads##bw(PCI_IOBASE + addr, buf, cnt); \ > + if (range) \ > + reads##bw(PCI_IOBASE + addr, buf, cnt); \ > } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { \ > - struct logic_pio_hwaddr *range = find_io_range(addr); \ > size_t sz = sizeof(type); \ > \ > if (range && range->ops) \ > @@ -242,16 +247,17 @@ void logic_ins##bw(unsigned long addr, void *buf, unsigned int cnt) \ > else \ > WARN_ON_ONCE(1); \ > } \ > - \ > } \ > \ > void logic_outs##bw(unsigned long addr, const void *buf, \ > unsigned int cnt) \ > { \ > + struct logic_pio_hwaddr *range = find_io_range(addr); \ > + \ > if (addr < MMIO_UPPER_LIMIT) { \ > - writes##bw(PCI_IOBASE + addr, buf, cnt); \ > + if (range) \ > + writes##bw(PCI_IOBASE + addr, buf, cnt); \ > } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { \ > - struct logic_pio_hwaddr *range = find_io_range(addr); \ > size_t sz = sizeof(type); \ > \ > if (range && range->ops) \ > @@ -269,28 +275,44 @@ type logic_in##bw(unsigned long addr) \ > { \ > type ret = (type)~0; \ > \ > - if (addr < MMIO_UPPER_LIMIT) \ > - ret = read##bw(PCI_IOBASE + addr); \ > + if (addr < MMIO_UPPER_LIMIT) { \ > + struct logic_pio_hwaddr *range = find_io_range(addr); \ > + \ > + if (range) \ > + ret = read##bw(PCI_IOBASE + addr); \ > + } \ > return ret; \ > } \ > \ > -void logic_out##bw(type value, unsigned long addr) \ > +void logic_out##bw(type val, unsigned long addr) \ > { \ > - if (addr < MMIO_UPPER_LIMIT) \ > - write##bw(value, PCI_IOBASE + addr); \ > + if (addr < MMIO_UPPER_LIMIT) { \ > + struct logic_pio_hwaddr *range = find_io_range(addr); \ > + \ > + if (range) \ > + write##bw(val, PCI_IOBASE + addr); \ > + } \ > } \ > \ > void logic_ins##bw(unsigned long addr, void *buf, unsigned int cnt) \ > { \ > - if (addr < MMIO_UPPER_LIMIT) \ > - reads##bw(PCI_IOBASE + addr, buf, cnt); \ > + if (addr < MMIO_UPPER_LIMIT) { \ > + struct logic_pio_hwaddr *range = find_io_range(addr); \ > + \ > + if (range) \ > + reads##bw(PCI_IOBASE + addr, buf, cnt); \ > + } \ > } \ > \ > void logic_outs##bw(unsigned long addr, const void *buf, \ > unsigned int cnt) \ > { \ > - if (addr < MMIO_UPPER_LIMIT) \ > - writes##bw(PCI_IOBASE + addr, buf, cnt); \ > + if (addr < MMIO_UPPER_LIMIT) { \ > + struct logic_pio_hwaddr *range = find_io_range(addr); \ > + \ > + if (range) \ > + writes##bw(PCI_IOBASE + addr, buf, cnt); \ > + } \ > } > #endif /* CONFIG_INDIRECT_PIO */ > > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 2/3] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions 2019-06-13 3:20 ` Bjorn Helgaas @ 2019-06-13 7:47 ` Arnd Bergmann 2019-06-13 10:17 ` John Garry 1 sibling, 0 replies; 17+ messages in thread From: Arnd Bergmann @ 2019-06-13 7:47 UTC (permalink / raw) To: Bjorn Helgaas Cc: John Garry, Lorenzo Pieralisi, linux-pci, Rafael J. Wysocki, Linux ARM, Will Deacon, Kefeng Wang, Linuxarm, Andy Shevchenko, Linux Kernel Mailing List, Catalin Marinas On Thu, Jun 13, 2019 at 5:20 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Tue, Jun 11, 2019 at 10:12:53PM +0800, John Garry wrote: > > Currently when accessing logical indirect PIO addresses in > > logic_{in, out}{,s}, we first ensure that the region is registered. > > I think logic_pio is specifically concerned with I/O port space, so > it's a little bit unfortunate that we named this "PIO". > > PIO is a general term for "Programmed I/O", which just means the CPU > is involved in each transfer, as opposed to DMA. The transfers can be > to either MMIO or I/O port space. > > So this ends up being a little confusing because I think you mean > "Port I/O", not "Programmed I/O". I think the terms that John uses are more common: I would also assume that "PIO" (regardless of whether you expand it as Port or Programmed I/O) refers only to inb/outb and PCI/ISA/LPC I/O space, and is distinct from "MMIO", which refers to the readl/writel accessors and PCI memory space. That is consistent with the usage across at least the x86, powerpc and ia64 architectures when they refer to PIO. Arnd ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 2/3] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions 2019-06-13 3:20 ` Bjorn Helgaas 2019-06-13 7:47 ` Arnd Bergmann @ 2019-06-13 10:17 ` John Garry 2019-06-13 13:46 ` Bjorn Helgaas 1 sibling, 1 reply; 17+ messages in thread From: John Garry @ 2019-06-13 10:17 UTC (permalink / raw) To: Bjorn Helgaas Cc: lorenzo.pieralisi, arnd, linux-pci, rjw, linux-arm-kernel, will.deacon, wangkefeng.wang, linuxarm, andriy.shevchenko, linux-kernel, catalin.marinas On 13/06/2019 04:20, Bjorn Helgaas wrote: > On Tue, Jun 11, 2019 at 10:12:53PM +0800, John Garry wrote: >> Currently when accessing logical indirect PIO addresses in >> logic_{in, out}{,s}, we first ensure that the region is registered. > Hi Bjorn, > I think logic_pio is specifically concerned with I/O port space, so > it's a little bit unfortunate that we named this "PIO". > > PIO is a general term for "Programmed I/O", which just means the CPU > is involved in each transfer, as opposed to DMA. The transfers can be > to either MMIO or I/O port space. > > So this ends up being a little confusing because I think you mean > "Port I/O", not "Programmed I/O". > Personally I agree that the naming isn't great. But then Arnd does think that "PIO" is appropriate. There were many different names along the way to this support merged, and I think that the naming became almost irrelevant in the end. >> However, no such check exists for CPU MMIO regions. The CPU MMIO regions >> would be registered by the PCI host (when PCI_IOBASE is defined) in >> pci_register_io_range(). > > IIUC this "CPU MMIO region" is an MMIO region where a memory load or > store from the CPU is converted by a PCI host bridge into an I/O port > transaction on PCI. > Right > Again IIUC, logic_pio supports two kinds of I/O port space accesses: > > 1) The simple "bridge converts loads/stores to an MMIO region to PCI > I/O port transactions" kind, and > > 2) The more complicated "somebody supplies logic_pio_host_ops > functions that do arbitrary magic to generate I/O port > transactions on some bus. Right > > And this patch is making the first kind smarter. Previously it would > perform the memory access whenever "addr < MMIO_UPPER_LIMIT", but > after this patch it will only do it if find_io_range() succeeds. > > Right? Sorry for restating what probably should have been obvious to > me. > Yes, right. A logical PIO range is registered for a PCI MMIO region in pci_register_io_range(). As such, if no range is registered, we can assume that no PCI MMIO region has been mapped and discard any attempted access. > I have two observations here: > > 1) The simple "bridge converts CPU MMIO space to PCI I/O port space" > flavor is essentially identical to what ia64 (and probably other > architectures) does. This should really be combined somehow. > Maybe. For ia64, it seems to have some "platform" versions of IO port accessors, and then also accessors need a fence barrier. I'm not sure how well that would fit with logical PIO. It would need further analysis. IIRC, PPC did have its own custom version of "indirect IO". So we could look to factor that out. > 2) If you made a default set of logic_pio_host_ops that merely did > loads/stores and maybe added a couple fields in the struct > logic_pio_hwaddr, I bet you could unify the two kinds so > logic_inb() would look something like this: > Yeah, I did consider this. We do not provide host operators for PCI MMIO ranges. We could simply provide regular versions of inb et al for this. A small obstacle for this is that we redefine inb et al, so would need "direct" versions also. It would be strange. So I'm not sure on the value of this. > u8 logic_inb(unsigned long addr) > { > struct logic_pio_hwaddr *range = find_io_range(addr); > > if (!range) > return (u8) ~0; > > return (u8) range->ops->in(range->hostdata, addr, sz); > } > >> We have seen scenarios when systems which don't have a PCI host, or they >> do but the PCI host probe fails, certain drivers attempts to still attempt >> to access PCI IO ports; examples are in [1] and [2]. >> >> Such is a case on an ARM64 system without a PCI host: >> >> root@(none)$ insmod hwmon/f71805f.ko >> Unable to handle kernel paging request at virtual address ffff7dfffee0002e >> Mem abort info: >> ESR = 0x96000046 >> Exception class = DABT (current EL), IL = 32 bits >> SET = 0, FnV = 0 >> EA = 0, S1PTW = 0 >> Data abort info: >> ISV = 0, ISS = 0x00000046 >> CM = 0, WnR = 1 >> swapper pgtable: 4k pages, 48-bit VAs, pgdp = (____ptrval____) >> [ffff7dfffee0002e] pgd=000000000141c003, pud=000000000141d003, pmd=0000000000000000 >> Internal error: Oops: 96000046 [#1] PREEMPT SMP >> Modules linked in: f71805f(+) >> CPU: 20 PID: 2736 Comm: insmod Not tainted 5.1.0-rc1-00003-g6f1bfec2a620-dirty #99 >> Hardware name: Huawei Taishan 2280 /D05, BIOS Hisilicon D05 IT21 Nemo 2.0 RC0 04/18/2018 >> pstate: 80000005 (Nzcv daif -PAN -UAO) >> pc : logic_outb+0x54/0xb8 >> lr : f71805f_find+0x2c/0x1b8 [f71805f] >> sp : ffff000025fbba90 >> x29: ffff000025fbba90 x28: ffff000008b944d0 >> x27: ffff000025fbbdf0 x26: 0000000000000100 >> x25: ffff801f8c270580 x24: ffff000011420000 >> x23: ffff000025fbbb3e x22: ffff000025fbbb40 >> x21: ffff000008b991b8 x20: 0000000000000087 >> x19: 000000000000002e x18: ffffffffffffffff >> x17: 0000000000000000 x16: 0000000000000000 >> x15: ffff00001127d6c8 x14: 0000000000000000 >> x13: 0000000000000000 x12: 0000000000000000 >> x11: 0000000000010820 x10: 0000841fdac40000 >> x9 : 0000000000000001 x8 : 0000000040000000 >> x7 : 0000000000210d00 x6 : 0000000000000000 >> x5 : ffff801fb6a46040 x4 : ffff841febeaeda0 >> x3 : 0000000000ffbffe x2 : ffff000025fbbb40 >> x1 : ffff7dfffee0002e x0 : ffff7dfffee00000 >> Process insmod (pid: 2736, stack limit = 0x(____ptrval____)) >> Call trace: >> logic_outb+0x54/0xb8 >> f71805f_find+0x2c/0x1b8 [f71805f] >> f71805f_init+0x38/0xe48 [f71805f] >> do_one_initcall+0x5c/0x198 >> do_init_module+0x54/0x1b0 >> load_module+0x1dc4/0x2158 >> __se_sys_init_module+0x14c/0x1e8 >> __arm64_sys_init_module+0x18/0x20 >> el0_svc_common+0x5c/0x100 >> el0_svc_handler+0x2c/0x80 >> el0_svc+0x8/0xc >> Code: d2bfdc00 f2cfbfe0 f2ffffe0 8b000021 (39000034) >> ---[ end trace 10ea80bde051bbfc ]--- >> root@(none)$ >> >> Well-behaved drivers call request_{muxed_}region() to grab the IO port >> region, but success here still doesn't actually mean that there is some IO >> port mapped in this region. >> >> This patch adds a check to ensure that the CPU MMIO region is registered >> prior to accessing the PCI IO ports. >> >> Any failed checks silently return. > > I *think* what you're doing here is making inb/outb/etc work the same > as on x86, i.e., if no device responds to an inb(), the caller gets > ~0, and if no device claims an outb() the data gets dropped. > Correct, but with a caveat: when you say no device responds, this means that - for arm64 case - no PCI MMIO region is mapped. > That should be explicit in the commit log. > >> [1] https://lore.kernel.org/linux-pci/56F209A9.4040304@huawei.com >> [2] https://lore.kernel.org/linux-arm-kernel/e6995b4a-184a-d8d4-f4d4-9ce75d8f47c0@huawei.com/ >> Thanks ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 2/3] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions 2019-06-13 10:17 ` John Garry @ 2019-06-13 13:46 ` Bjorn Helgaas 2019-06-13 14:09 ` John Garry 0 siblings, 1 reply; 17+ messages in thread From: Bjorn Helgaas @ 2019-06-13 13:46 UTC (permalink / raw) To: John Garry Cc: lorenzo.pieralisi, arnd, linux-pci, rjw, linux-arm-kernel, will.deacon, wangkefeng.wang, linuxarm, andriy.shevchenko, linux-kernel, catalin.marinas On Thu, Jun 13, 2019 at 11:17:37AM +0100, John Garry wrote: > On 13/06/2019 04:20, Bjorn Helgaas wrote: > > On Tue, Jun 11, 2019 at 10:12:53PM +0800, John Garry wrote: > > > Currently when accessing logical indirect PIO addresses in > > > logic_{in, out}{,s}, we first ensure that the region is registered. > > > I think logic_pio is specifically concerned with I/O port space, so > > it's a little bit unfortunate that we named this "PIO". > > > > PIO is a general term for "Programmed I/O", which just means the CPU > > is involved in each transfer, as opposed to DMA. The transfers can be > > to either MMIO or I/O port space. > > > > So this ends up being a little confusing because I think you mean > > "Port I/O", not "Programmed I/O". > > Personally I agree that the naming isn't great. But then Arnd does think > that "PIO" is appropriate. > > There were many different names along the way to this support merged, and I > think that the naming became almost irrelevant in the end. Yep, Arnd is right. The "PIO" name contributed a little to my confusion, but I think the bigger piece was that I read the "indirect PIO addresses" above as being parallel to the "CPU MMIO regions" below, when in fact, they are not. The arguments to logic_inb() are always port addresses, never CPU MMIO addresses, but in some cases logic_inb() internally references a CPU MMIO region that corresponds to the port address. Possible commit log text: The logic_{in,out}*() functions access two regions of I/O port addresses: 1) [0, MMIO_UPPER_LIMIT): these are assumed to be LOGIC_PIO_CPU_MMIO regions, where a bridge converts CPU loads and stores to MMIO space on its primary side into I/O port transactions on its secondary side. 2) [MMIO_UPPER_LIMIT, IO_SPACE_LIMIT): these are assumed to be LOGIC_PIO_INDIRECT regions, where we verify that the region was registered by logic_pio_register_range() before calling the logic_pio_host_ops functions to perform the access. Previously there was no requirement that accesses to the LOGIC_PIO_CPU_MMIO area matched anything registered by logic_pio_register_range(), and accesses to unregistered I/O ports could cause exceptions like the one below. Verify that accesses to ports in the LOGIC_PIO_CPU_MMIO area correspond to registered ranges. Accesses to ports outside those registered ranges fail (logic_in*() returns ~0 data and logic_out*() does nothing). This matches the x86 behavior where in*() returns ~0 if no device responds, and out*() is dropped if no device claims it. > > 1) The simple "bridge converts CPU MMIO space to PCI I/O port space" > > flavor is essentially identical to what ia64 (and probably other > > architectures) does. This should really be combined somehow. > > Maybe. For ia64, it seems to have some "platform" versions of IO port > accessors, and then also accessors need a fence barrier. I'm not sure how > well that would fit with logical PIO. It would need further analysis. Right. That shouldn't be part of this series, but I think it would be nice to someday unify the ia64 add_io_space() path with the pci_register_io_range() path. There might have to be ia64-specific accessors at the bottom for the fences, but I think the top side could be unified because it's conceptually the same thing -- an MMIO region that is translated by a bridge to an I/O port region. > > 2) If you made a default set of logic_pio_host_ops that merely did > > loads/stores and maybe added a couple fields in the struct > > logic_pio_hwaddr, I bet you could unify the two kinds so > > logic_inb() would look something like this: > > Yeah, I did consider this. We do not provide host operators for PCI MMIO > ranges. We could simply provide regular versions of inb et al for this. A > small obstacle for this is that we redefine inb et al, so would need > "direct" versions also. It would be strange. Yeah, just a thought, maybe it wouldn't work out. > > > Any failed checks silently return. > > > > I *think* what you're doing here is making inb/outb/etc work the same > > as on x86, i.e., if no device responds to an inb(), the caller gets > > ~0, and if no device claims an outb() the data gets dropped. > > Correct, but with a caveat: when you say no device responds, this means that > - for arm64 case - no PCI MMIO region is mapped. Yep. I was describing the x86 behavior, where we don't do any mapping and all we can say is that no device responded. Bjorn ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 2/3] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions 2019-06-13 13:46 ` Bjorn Helgaas @ 2019-06-13 14:09 ` John Garry 0 siblings, 0 replies; 17+ messages in thread From: John Garry @ 2019-06-13 14:09 UTC (permalink / raw) To: Bjorn Helgaas Cc: lorenzo.pieralisi, arnd, linux-pci, rjw, linux-arm-kernel, will.deacon, wangkefeng.wang, linuxarm, andriy.shevchenko, linux-kernel, catalin.marinas Hi Bjorn, >> There were many different names along the way to this support merged, and I >> think that the naming became almost irrelevant in the end. > > Yep, Arnd is right. The "PIO" name contributed a little to my > confusion, but I think the bigger piece was that I read the "indirect > PIO addresses" above as being parallel to the "CPU MMIO regions" > below, when in fact, they are not. The arguments to logic_inb() are > always port addresses, never CPU MMIO addresses, but in some cases > logic_inb() internally references a CPU MMIO region that corresponds > to the port address. Right > > Possible commit log text: > > The logic_{in,out}*() functions access two regions of I/O port > addresses: > > 1) [0, MMIO_UPPER_LIMIT): these are assumed to be > LOGIC_PIO_CPU_MMIO regions, where a bridge converts CPU loads > and stores to MMIO space on its primary side into I/O port > transactions on its secondary side. > > 2) [MMIO_UPPER_LIMIT, IO_SPACE_LIMIT): these are assumed to be > LOGIC_PIO_INDIRECT regions, where we verify that the region was > registered by logic_pio_register_range() before calling the > logic_pio_host_ops functions to perform the access. > > Previously there was no requirement that accesses to the > LOGIC_PIO_CPU_MMIO area matched anything registered by > logic_pio_register_range(), and accesses to unregistered I/O ports > could cause exceptions like the one below. > > Verify that accesses to ports in the LOGIC_PIO_CPU_MMIO area > correspond to registered ranges. Accesses to ports outside those > registered ranges fail (logic_in*() returns ~0 data and logic_out*() > does nothing). > > This matches the x86 behavior where in*() returns ~0 if no device > responds, and out*() is dropped if no device claims it. It reads quite well so I can incorporate it. I'd still like to mention about request_{muxed_}region(), and how this does not protect against accesses to unregistered regions. > >>> 1) The simple "bridge converts CPU MMIO space to PCI I/O port space" >>> flavor is essentially identical to what ia64 (and probably other >>> architectures) does. This should really be combined somehow. >> >> Maybe. For ia64, it seems to have some "platform" versions of IO port >> accessors, and then also accessors need a fence barrier. I'm not sure how >> well that would fit with logical PIO. It would need further analysis. > > Right. That shouldn't be part of this series, but I think it would be > nice to someday unify the ia64 add_io_space() path with the > pci_register_io_range() path. There might have to be ia64-specific > accessors at the bottom for the fences, but I think the top side could > be unified because it's conceptually the same thing -- an MMIO region > that is translated by a bridge to an I/O port region. Yes, it would be good to move any arch-specific port IO function to this common framework. To mention it again, what's under CONFIG_PPC_INDIRECT_PIO seems an obvious candidate. > >>> 2) If you made a default set of logic_pio_host_ops that merely did >>> loads/stores and maybe added a couple fields in the struct >>> logic_pio_hwaddr, I bet you could unify the two kinds so >>> logic_inb() would look something like this: >> >> Yeah, I did consider this. We do not provide host operators for PCI MMIO >> ranges. We could simply provide regular versions of inb et al for this. A >> small obstacle for this is that we redefine inb et al, so would need >> "direct" versions also. It would be strange. > > Yeah, just a thought, maybe it wouldn't work out. > >>>> Any failed checks silently return. >>> >>> I *think* what you're doing here is making inb/outb/etc work the same >>> as on x86, i.e., if no device responds to an inb(), the caller gets >>> ~0, and if no device claims an outb() the data gets dropped. >> >> Correct, but with a caveat: when you say no device responds, this means that >> - for arm64 case - no PCI MMIO region is mapped. > > Yep. I was describing the x86 behavior, where we don't do any mapping > and all we can say is that no device responded. > > Bjorn > Thanks, John > . > ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v4 3/3] lib: logic_pio: Fix up a print 2019-06-11 14:12 [PATCH v4 0/3] Fix ARM64 crash for accessing unmapped IO port regions John Garry 2019-06-11 14:12 ` [PATCH v4 1/3] lib: logic_pio: Use logical PIO low-level accessors for !CONFIG_INDIRECT_PIO John Garry 2019-06-11 14:12 ` [PATCH v4 2/3] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions John Garry @ 2019-06-11 14:12 ` John Garry 2 siblings, 0 replies; 17+ messages in thread From: John Garry @ 2019-06-11 14:12 UTC (permalink / raw) To: bhelgaas, lorenzo.pieralisi, arnd Cc: linux-pci, rjw, linux-arm-kernel, will.deacon, wangkefeng.wang, linuxarm, andriy.shevchenko, linux-kernel, catalin.marinas, John Garry For the print in logic_pio_trans_cpuaddr(), don't cast the value to unsigned long long, and just print the resource_size_t type directly. Signed-off-by: John Garry <john.garry@huawei.com> --- lib/logic_pio.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/logic_pio.c b/lib/logic_pio.c index 47d24f428908..030708ce7bb6 100644 --- a/lib/logic_pio.c +++ b/lib/logic_pio.c @@ -186,8 +186,7 @@ unsigned long logic_pio_trans_cpuaddr(resource_size_t addr) if (in_range(addr, range->hw_start, range->size)) return addr - range->hw_start + range->io_start; } - pr_err("addr %llx not registered in io_range_list\n", - (unsigned long long) addr); + pr_err("addr %pa not registered in io_range_list\n", &addr); return ~0UL; } -- 2.17.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
end of thread, other threads:[~2019-06-14 12:22 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-11 14:12 [PATCH v4 0/3] Fix ARM64 crash for accessing unmapped IO port regions John Garry 2019-06-11 14:12 ` [PATCH v4 1/3] lib: logic_pio: Use logical PIO low-level accessors for !CONFIG_INDIRECT_PIO John Garry 2019-06-13 2:39 ` Bjorn Helgaas 2019-06-13 9:39 ` John Garry 2019-06-13 20:09 ` Bjorn Helgaas 2019-06-14 9:02 ` John Garry 2019-06-14 11:50 ` Bjorn Helgaas 2019-06-14 12:22 ` John Garry 2019-06-13 13:58 ` Bjorn Helgaas 2019-06-13 15:21 ` John Garry 2019-06-11 14:12 ` [PATCH v4 2/3] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions John Garry 2019-06-13 3:20 ` Bjorn Helgaas 2019-06-13 7:47 ` Arnd Bergmann 2019-06-13 10:17 ` John Garry 2019-06-13 13:46 ` Bjorn Helgaas 2019-06-13 14:09 ` John Garry 2019-06-11 14:12 ` [PATCH v4 3/3] lib: logic_pio: Fix up a print John Garry
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).