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
