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 > . >
WARNING: multiple messages have this Message-ID (diff)
From: John Garry <john.garry@huawei.com> To: Bjorn Helgaas <helgaas@kernel.org> Cc: rjw@rjwysocki.net, wangkefeng.wang@huawei.com, lorenzo.pieralisi@arm.com, arnd@arndb.de, linux-pci@vger.kernel.org, will.deacon@arm.com, linuxarm@huawei.com, linux-kernel@vger.kernel.org, catalin.marinas@arm.com, andriy.shevchenko@linux.intel.com, linux-arm-kernel@lists.infradead.org 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 > . > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-06-13 15:06 UTC|newest] Thread overview: 34+ 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 ` 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-11 14:12 ` John Garry 2019-06-13 2:39 ` Bjorn Helgaas 2019-06-13 2:39 ` Bjorn Helgaas 2019-06-13 9:39 ` John Garry 2019-06-13 9:39 ` John Garry 2019-06-13 20:09 ` Bjorn Helgaas 2019-06-13 20:09 ` Bjorn Helgaas 2019-06-14 9:02 ` John Garry 2019-06-14 9:02 ` John Garry 2019-06-14 11:50 ` Bjorn Helgaas 2019-06-14 11:50 ` Bjorn Helgaas 2019-06-14 12:22 ` John Garry 2019-06-14 12:22 ` John Garry 2019-06-13 13:58 ` Bjorn Helgaas 2019-06-13 13:58 ` Bjorn Helgaas 2019-06-13 15:21 ` John Garry 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-11 14:12 ` John Garry 2019-06-13 3:20 ` Bjorn Helgaas 2019-06-13 3:20 ` Bjorn Helgaas 2019-06-13 7:47 ` Arnd Bergmann 2019-06-13 7:47 ` Arnd Bergmann 2019-06-13 10:17 ` John Garry 2019-06-13 10:17 ` John Garry 2019-06-13 13:46 ` Bjorn Helgaas 2019-06-13 13:46 ` Bjorn Helgaas 2019-06-13 14:09 ` John Garry [this message] 2019-06-13 14:09 ` John Garry 2019-06-11 14:12 ` [PATCH v4 3/3] lib: logic_pio: Fix up a print John Garry 2019-06-11 14:12 ` 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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.