[PATCH v4 0/3] Fix ARM64 crash for accessing unmapped IO port regions

[PATCH v4 0/3] Fix ARM64 crash for accessing unmapped IO port regions

[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

[PATCH v4 1/3] lib: logic_pio: Use logical PIO low-level accessors for !CONFIG_INDIRECT_PIO

[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

Re: [PATCH v4 1/3] lib: logic_pio: Use logical PIO low-level accessors for !CONFIG_INDIRECT_PIO

[Bjorn Helgaas]

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
> 

Re: [PATCH v4 1/3] lib: logic_pio: Use logical PIO low-level accessors for !CONFIG_INDIRECT_PIO

[John Garry]

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

Re: [PATCH v4 1/3] lib: logic_pio: Use logical PIO low-level accessors for !CONFIG_INDIRECT_PIO

[Bjorn Helgaas]

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

Re: [PATCH v4 1/3] lib: logic_pio: Use logical PIO low-level accessors for !CONFIG_INDIRECT_PIO

[John Garry]

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
>
> .
>

Re: [PATCH v4 1/3] lib: logic_pio: Use logical PIO low-level accessors for !CONFIG_INDIRECT_PIO

[Bjorn Helgaas]

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

Re: [PATCH v4 1/3] lib: logic_pio: Use logical PIO low-level accessors for !CONFIG_INDIRECT_PIO

[John Garry]

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

Re: [PATCH v4 1/3] lib: logic_pio: Use logical PIO low-level accessors for !CONFIG_INDIRECT_PIO

[Bjorn Helgaas]

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

Re: [PATCH v4 1/3] lib: logic_pio: Use logical PIO low-level accessors for !CONFIG_INDIRECT_PIO

[John Garry]

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
>
> .
>

[PATCH v4 2/3] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions

[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

Re: [PATCH v4 2/3] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions

[Bjorn Helgaas]

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
> 

Re: [PATCH v4 2/3] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions

[Arnd Bergmann]

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

Re: [PATCH v4 2/3] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions

[John Garry]

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

Re: [PATCH v4 2/3] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions

[Bjorn Helgaas]

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

Re: [PATCH v4 2/3] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions

[John Garry]

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

> .
>

[PATCH v4 3/3] lib: logic_pio: Fix up a print

[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