From: John Garry <john.garry@huawei.com>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: <lorenzo.pieralisi@arm.com>, <arnd@arndb.de>,
<linux-pci@vger.kernel.org>, <rjw@rjwysocki.net>,
<linux-arm-kernel@lists.infradead.org>, <will.deacon@arm.com>,
<wangkefeng.wang@huawei.com>, <linuxarm@huawei.com>,
<andriy.shevchenko@linux.intel.com>,
<linux-kernel@vger.kernel.org>, <catalin.marinas@arm.com>
Subject: Re: [PATCH v4 2/3] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions
Date: Thu, 13 Jun 2019 15:09:10 +0100 [thread overview]
Message-ID: <d1ed7c02-9bad-c584-9b0e-1e3fc22ea46e@huawei.com> (raw)
In-Reply-To: <20190613134650.GF13533@google.com>
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
> .
>
next prev parent reply other threads:[~2019-06-13 15:06 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2019-06-11 14:12 ` [PATCH v4 3/3] lib: logic_pio: Fix up a print John Garry
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d1ed7c02-9bad-c584-9b0e-1e3fc22ea46e@huawei.com \
--to=john.garry@huawei.com \
--cc=andriy.shevchenko@linux.intel.com \
--cc=arnd@arndb.de \
--cc=catalin.marinas@arm.com \
--cc=helgaas@kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=linuxarm@huawei.com \
--cc=lorenzo.pieralisi@arm.com \
--cc=rjw@rjwysocki.net \
--cc=wangkefeng.wang@huawei.com \
--cc=will.deacon@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).