Messages in this thread | | | Subject | Re: [PATCH v4 2/3] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions | From | John Garry <> | Date | Thu, 13 Jun 2019 15:09:10 +0100 |
| |
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
> . >
| |