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

> .