| Message ID | 20211221181526.53798-4-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | platform/x86: introduce p2sb_bar() helper | expand |
On Tue, Dec 21, 2021 at 08:15:21PM +0200, Andy Shevchenko wrote: > From: Jonathan Yong <jonathan.yong@intel.com> > > There are already two and at least one more user is coming which > require an access to Primary to Sideband (P2SB) bridge in order > to get IO or MMIO BAR hidden by BIOS. The fact that BIOS hid the BAR is a good hint that BIOS doesn't want us to touch it. > Create a library to access P2SB for x86 devices. > > Background information > ====================== > Note, the term "bridge" is used in the documentation and it has nothing > to do with a PCI (host) bridge as per the PCI specifications. > > The P2SB is an interesting device by it's nature and hardware design. s/it's/its/ > First of all, it has several devices in the hardware behind it. These > devices may or may not be represented as ACPI devices by a firmware. > > It also has a hardwired (to 0s) the least significant part of the > address space which is represented by the only 64-bit BAR0. It means > that OS mustn't reallocate the BAR. You say the "least significant part of the *address space*" -- I don't know what that would be, unless you mean the least-significant bits of a *BAR*. The general rule is that the OS is allowed to reassign BARs unless the firmware tells us otherwise via the "Preserve PCI Boot Configuration" _DSM. I'm not familiar with the rule that the least-significant bits of a BAR being hardwired to zero means the OS must not reallocate the BAR. Per spec, the least-significant bits being hardwired to zero is what tells us the *size* of the BAR. > On top of that in some cases P2SB is represented by function 0 on PCI > slot (in terms of B:D.F) and according to the PCI specification any > other function can't be seen until function 0 is present and visible. This doesn't sound interesting; it sounds like standard PCI behavior. Per PCIe r5.0, sec 7.5.1.1.9, "When [Multi-Function Device is] Clear, software must not probe for Functions other than Function 0 unless explicitly indicated by another mechanism, such as an ARI or SR-IOV Extended Capability structure." So software can't probe for functions other than 0 unless function 0 is present and has the Multi-Function Device bit set. Is this P2SB thing function 0? > In the PCI configuration space of P2SB device the full 32-bit register > is allocated for the only purpose of hiding the entire P2SB device. > > 3.1.39 P2SB Control (P2SBC)—Offset E0h > > Hide Device (HIDE): When this bit is set, the P2SB will return 1s on > any PCI Configuration Read on IOSF-P. All other transactions including > PCI Configuration Writes on IOSF-P are unaffected by this. This does > not affect reads performed on the IOSF-SB interface. Are you saying it works like this? pci_read_config_word (p2sb_dev, PCI_VENDOR_ID, &id); # id = 0x8086 pci_write_config_dword(p2sb_dev, 0xe0, HIDE); pci_read_config_word (p2sb_dev, PCI_VENDOR_ID, &id); # id = 0xffff pci_write_config_dword(p2sb_dev, 0xe0, ~HIDE); pci_read_config_word (p2sb_dev, PCI_VENDOR_ID, &id); # id = 0x8086 > This doesn't prevent MMIO accesses though. In order to prevent OS from > the assignment to these addresses, the firmware on the affected platforms > marks the region as unusable (by cutting it off from the PCI host bridge > resources) as depicted in the Apollo Lake example below: "In order to prevent OS from the assignment to these addresses" doesn't read quite right. Is it supposed to say something about "preventing the OS from assigning these addresses"? When assigning space to PCI devices, the OS is only allowed to use space from the host bridge _CRS. Is the above supposed to say that the firmware omits a region from the host bridge _CRS to prevent the OS from using it? That's the standard behavior, of course. And, of course, if the OS cannot enumerate a PCI device, obviously it cannot reassign any BARs on this device it doesn't know about. > PCI host bridge to bus 0000:00 > pci_bus 0000:00: root bus resource [io 0x0070-0x0077] > pci_bus 0000:00: root bus resource [io 0x0000-0x006f window] > pci_bus 0000:00: root bus resource [io 0x0078-0x0cf7 window] > pci_bus 0000:00: root bus resource [io 0x0d00-0xffff window] > pci_bus 0000:00: root bus resource [mem 0x7c000001-0x7fffffff window] > pci_bus 0000:00: root bus resource [mem 0x7b800001-0x7bffffff window] > pci_bus 0000:00: root bus resource [mem 0x80000000-0xcfffffff window] > pci_bus 0000:00: root bus resource [mem 0xe0000000-0xefffffff window] > pci_bus 0000:00: root bus resource [bus 00-ff] > > The P2SB 16MB BAR is located at 0xd0000000-0xd0ffffff memory window. Ah, OK, maybe this is seeping in. Tell me if I'm understanding correctly: - P2SB is a device connected via PCI that has one BAR - Firmware programs the BAR to [mem 0xd0000000-0xd0ffffff] - Firmware sets the P2SB HIDE bit - P2SB now returns ~0 to config reads, handles config writes normally, and handles MMIO reads/writes normally - Firmware ensures [mem 0xd0000000-0xd0ffffff] is not available to the OS by removing it from _CRS and marking it "RESERVED" in e820 or EFI memory map - P2SB returns ~0 to subsequent config reads - Therefore, OS cannot enumerate P2SB Now you want to know the BAR value (0xd0000000) so you can do something with it, so you clear the HIDE bit, read the BAR, and set the HIDE bit again. > The generic solution > ==================== > The generic solution for all cases when we need to access to the information > behind P2SB device is a library code where users ask for necessary resources > by demand and hence those users take care of not being run on the systems > where this access is not required. > > The library provides the p2sb_bar() API to retrieve the MMIO of the BAR0 of > the device from P2SB device slot. > > P2SB unconditional unhiding awareness > ===================================== > Technically it's possible to unhinde the P2SB device and devices on > the same PCI slot and access them at any time as needed. But there are > several potential issues with that: s/unhinde/unhide/ > - the systems were never tested against such configuration and hence > nobody knows what kind of bugs it may bring, especially when we talk > about SPI NOR case which contains IFWI code (including BIOS) and > already known to be problematic in the past for end users No clue what "IFWI" means. The hardware and BIOS went to some trouble to hide this MMIO space from the OS, but now the OS wants to use it. I'm pretty sure the systems were never tested against *that* configuration either :) And the fact that they went to all this trouble to hide it means the BIOS is likely using it for its own purposes and the OS may cause conflicts if it also uses it. The way the BIOS has this set up, P2SB is logically not a PCI device. It is not enumerable. The MMIO space it uses is not in the _CRS of a PCI host bridge. That means it's now a platform device. Hopefully P2SB is not behind a PCI-to-PCI bridge that *is* under OS control, because the OS might change the bridge apertures so the MMIO space is no longer reachable. The correct way to use this would be as an ACPI device so the OS can enumerate it and the firmware can mediate access to it. Going behind the back of the firmware does not sound advisable to me. If you want to hack something in, I think it might be easier to treat this purely as a platform device instead of a PCI device. You can hack up the config accesses you need, discover the MMIO address, plug that in as a resource of the platform device, and go wild. I don't think the PCI core needs to be involved at all. > - the PCI by it's nature is a hotpluggable bus and in case somebody > attaches a driver to the functions of a P2SB slot device(s) the > end user experience and system behaviour can be unpredictable s/it's/its/ > - the kernel code would need some ugly hacks (or code looking as an > ugly hack) under arch/x86/pci in order to enable these devices on > only selected platforms (which may include CPU ID table followed by > a potentially growing number of DMI strings > > The future improvements > ======================= > The future improvements with this code may go in order to gain some kind > of cache, if it's possible at all, to prevent unhiding and hiding to many > times to take static information that may be saved once per boot. s/to many/too many/ or even better, s/to many/many/ > Links > ===== > [1]: https://lab.whitequark.org/notes/2017-11-08/accessing-intel-ich-pch-gpios/ Reverse engineering notes? Nice, but not really what I would expect to see in patches coming from INTEL to enable some INTEL hardware. > [2]: https://lab.whitequark.org/files/gpioke/Intel-332690-004EN.pdf An Intel datasheet (good) but from a non-Intel site (not so good). > [3]: https://lab.whitequark.org/files/gpioke/Intel-332691-002EN.pdf Again? Use an intel.com link if you can. If these support something in the commit log above, can you make the connection a little clearer? I guess one of these has a section 3.1.39? > [4]: https://medium.com/@jacksonchen_43335/bios-gpio-p2sb-70e9b829b403 Are these notes on reverse engineering this thing? Doesn't really seem like useful information in this one. Bjorn
On Thu, Jan 06, 2022 at 07:03:05PM -0600, Bjorn Helgaas wrote: > On Tue, Dec 21, 2021 at 08:15:21PM +0200, Andy Shevchenko wrote: Thanks for review, my answers below. > > There are already two and at least one more user is coming which > > require an access to Primary to Sideband (P2SB) bridge in order > > to get IO or MMIO BAR hidden by BIOS. > > The fact that BIOS hid the BAR is a good hint that BIOS doesn't want > us to touch it. The main reason is to avoid yellow bang in Windows. But the consequences of this are the untested configurations in case it's always enabled and unhidden. > > Create a library to access P2SB for x86 devices. > > > > Background information > > ====================== > > Note, the term "bridge" is used in the documentation and it has nothing > > to do with a PCI (host) bridge as per the PCI specifications. > > > > The P2SB is an interesting device by it's nature and hardware design. > > s/it's/its/ Fixed locally. > > First of all, it has several devices in the hardware behind it. These > > devices may or may not be represented as ACPI devices by a firmware. > > > > It also has a hardwired (to 0s) the least significant part of the > > address space which is represented by the only 64-bit BAR0. It means > > that OS mustn't reallocate the BAR. > > You say the "least significant part of the *address space*" -- I don't > know what that would be, unless you mean the least-significant bits of > a *BAR*. Yeah, I rephrased this as "...the least significant bits of the base address register which is represented by the only 64-bit BAR0." > The general rule is that the OS is allowed to reassign BARs unless the > firmware tells us otherwise via the "Preserve PCI Boot Configuration" > _DSM. > > I'm not familiar with the rule that the least-significant bits of a > BAR being hardwired to zero means the OS must not reallocate the BAR. > Per spec, the least-significant bits being hardwired to zero is what > tells us the *size* of the BAR. I believe more about this is discussed below (1). > > On top of that in some cases P2SB is represented by function 0 on PCI > > slot (in terms of B:D.F) and according to the PCI specification any > > other function can't be seen until function 0 is present and visible. > > This doesn't sound interesting; it sounds like standard PCI behavior. > Per PCIe r5.0, sec 7.5.1.1.9, "When [Multi-Function Device is] Clear, > software must not probe for Functions other than Function 0 unless > explicitly indicated by another mechanism, such as an ARI or SR-IOV > Extended Capability structure." > > So software can't probe for functions other than 0 unless function 0 > is present and has the Multi-Function Device bit set. So, you are repeating what I told you when you asked me why we read a BAR of another device. I put it here to avoid again the same question from you or anyone else (who may well be not familiar with the spec). What do you want me to do here? > Is this P2SB thing function 0? It depends. > > In the PCI configuration space of P2SB device the full 32-bit register > > is allocated for the only purpose of hiding the entire P2SB device. > > > > 3.1.39 P2SB Control (P2SBC)—Offset E0h > > > > Hide Device (HIDE): When this bit is set, the P2SB will return 1s on > > any PCI Configuration Read on IOSF-P. All other transactions including > > PCI Configuration Writes on IOSF-P are unaffected by this. This does > > not affect reads performed on the IOSF-SB interface. > > Are you saying it works like this? > > pci_read_config_word (p2sb_dev, PCI_VENDOR_ID, &id); # id = 0x8086 > pci_write_config_dword(p2sb_dev, 0xe0, HIDE); > pci_read_config_word (p2sb_dev, PCI_VENDOR_ID, &id); # id = 0xffff > pci_write_config_dword(p2sb_dev, 0xe0, ~HIDE); > pci_read_config_word (p2sb_dev, PCI_VENDOR_ID, &id); # id = 0x8086 It's not me, the documentation, but yes, something like you provided above is what it does. > > This doesn't prevent MMIO accesses though. In order to prevent OS from > > the assignment to these addresses, the firmware on the affected platforms > > marks the region as unusable (by cutting it off from the PCI host bridge > > resources) as depicted in the Apollo Lake example below: > > "In order to prevent OS from the assignment to these addresses" > doesn't read quite right. Is it supposed to say something about > "preventing the OS from assigning these addresses"? Yes, thanks, I fixed as suggested. > When assigning space to PCI devices, the OS is only allowed to use > space from the host bridge _CRS. (1) Correct and the P2SB area is not included there. > Is the above supposed to say that the firmware omits a region from the > host bridge _CRS to prevent the OS from using it? That's the standard > behavior, of course. Yes, and again the purpose of this paragraph is to document the background of all these as requested by you in previous round. It might be I misread what you wanted that time. > And, of course, if the OS cannot enumerate a PCI device, obviously it > cannot reassign any BARs on this device it doesn't know about. Exactly. And I believe this explains why the region is excluded from _CRS and why we mustn't reallocate it. It probably will work (again, no-one have broadly tested this), but it will consume resources which can be used by others (like Thunderbolt). > > PCI host bridge to bus 0000:00 > > pci_bus 0000:00: root bus resource [io 0x0070-0x0077] > > pci_bus 0000:00: root bus resource [io 0x0000-0x006f window] > > pci_bus 0000:00: root bus resource [io 0x0078-0x0cf7 window] > > pci_bus 0000:00: root bus resource [io 0x0d00-0xffff window] > > pci_bus 0000:00: root bus resource [mem 0x7c000001-0x7fffffff window] > > pci_bus 0000:00: root bus resource [mem 0x7b800001-0x7bffffff window] > > pci_bus 0000:00: root bus resource [mem 0x80000000-0xcfffffff window] > > pci_bus 0000:00: root bus resource [mem 0xe0000000-0xefffffff window] > > pci_bus 0000:00: root bus resource [bus 00-ff] > > > > The P2SB 16MB BAR is located at 0xd0000000-0xd0ffffff memory window. > > Ah, OK, maybe this is seeping in. Tell me if I'm understanding > correctly: > > - P2SB is a device connected via PCI that has one BAR Yes. > - Firmware programs the BAR to [mem 0xd0000000-0xd0ffffff] Yes. > - Firmware sets the P2SB HIDE bit Yes. > - P2SB now returns ~0 to config reads, Yes. > handles config writes normally, Only to 0xe0. The rest is skipped. > and handles MMIO reads/writes normally Yes. > - Firmware ensures [mem 0xd0000000-0xd0ffffff] is not available to > the OS by removing it from _CRS and marking it "RESERVED" in e820 > or EFI memory map Yes. > - P2SB returns ~0 to subsequent config reads > - Therefore, OS cannot enumerate P2SB Correct. > Now you want to know the BAR value (0xd0000000) so you can do > something with it, so you clear the HIDE bit, read the BAR, > and set the HIDE bit again. Correct. > > The generic solution > > ==================== > > The generic solution for all cases when we need to access to the information > > behind P2SB device is a library code where users ask for necessary resources > > by demand and hence those users take care of not being run on the systems > > where this access is not required. > > > > The library provides the p2sb_bar() API to retrieve the MMIO of the BAR0 of > > the device from P2SB device slot. > > > > P2SB unconditional unhiding awareness > > ===================================== > > Technically it's possible to unhinde the P2SB device and devices on > > the same PCI slot and access them at any time as needed. But there are > > several potential issues with that: > > s/unhinde/unhide/ Fixed locally. > > - the systems were never tested against such configuration and hence > > nobody knows what kind of bugs it may bring, especially when we talk > > about SPI NOR case which contains IFWI code (including BIOS) and > > already known to be problematic in the past for end users > > No clue what "IFWI" means. Intel FirmWare Image, I will spell it in full. > The hardware and BIOS went to some trouble to hide this MMIO space > from the OS, but now the OS wants to use it. I'm pretty sure the > systems were never tested against *that* configuration either :) What do you mean? The unhide/hide back has been tested and we have already users in the kernel (they have other issues though with the PCI rescan lock, but it doesn't mean it wasn't ever tested). > And the fact that they went to all this trouble to hide it means > the BIOS is likely using it for its own purposes and the OS may cause > conflicts if it also uses it. What purposes do you have in mind? > The way the BIOS has this set up, P2SB is logically not a PCI device. > It is not enumerable. The MMIO space it uses is not in the _CRS of a > PCI host bridge. That means it's now a platform device. I do not follow what you are implying here. As you see the code, it's not a driver, it's a library that reuses PCI functions because the hardware is represented by an IP inside PCI hierarchy and with PCI programming interface. > Hopefully P2SB is not behind a PCI-to-PCI bridge that *is* under OS > control, because the OS might change the bridge apertures so the MMIO > space is no longer reachable. No it's not (at least on all platforms what I know of). We are good here. > The correct way to use this would be as an ACPI device so the OS can > enumerate it and the firmware can mediate access to it. Going behind > the back of the firmware does not sound advisable to me. Are you going to fix all firmwares and devices on the market? We have it's done like this and unfortunately we can't fix what's is done due to users who won't update their firmwares by one or another reason. > If you want to hack something in, I think it might be easier to treat > this purely as a platform device instead of a PCI device. You can > hack up the config accesses you need, discover the MMIO address, plug > that in as a resource of the platform device, and go wild. I don't > think the PCI core needs to be involved at all. Sorry, I do not follow you. The device is PCI, but it's taken out of PCI subsystem control by this hardware trick. > > - the PCI by it's nature is a hotpluggable bus and in case somebody > > attaches a driver to the functions of a P2SB slot device(s) the > > end user experience and system behaviour can be unpredictable > s/it's/its/ Fixed locally. > > - the kernel code would need some ugly hacks (or code looking as an > > ugly hack) under arch/x86/pci in order to enable these devices on > > only selected platforms (which may include CPU ID table followed by > > a potentially growing number of DMI strings > > > > The future improvements > > ======================= > > The future improvements with this code may go in order to gain some kind > > of cache, if it's possible at all, to prevent unhiding and hiding to many > > times to take static information that may be saved once per boot. > > s/to many/too many/ or even better, s/to many/many/ Fixed locally. > > Links > > ===== > > [1]: https://lab.whitequark.org/notes/2017-11-08/accessing-intel-ich-pch-gpios/ > > Reverse engineering notes? Nice, but not really what I would expect > to see in patches coming from INTEL to enable some INTEL hardware. > > > [2]: https://lab.whitequark.org/files/gpioke/Intel-332690-004EN.pdf > > An Intel datasheet (good) but from a non-Intel site (not so good). > > > [3]: https://lab.whitequark.org/files/gpioke/Intel-332691-002EN.pdf > > Again? Use an intel.com link if you can. There are document numbers that make sense. I believe that [2]: https://cdrdv2.intel.com/v1/dl/getContent/332690?wapkw=332690 [3]: https://cdrdv2.intel.com/v1/dl/getContent/332691?wapkw=332691 work for you. Tell me if not (Meanwhile I have changed locally) > If these support something in the commit log above, can you make the > connection a little clearer? I guess one of these has a section > 3.1.39? Done. > > [4]: https://medium.com/@jacksonchen_43335/bios-gpio-p2sb-70e9b829b403 > > Are these notes on reverse engineering this thing? Doesn't really > seem like useful information in this one.
On Fri, Jan 07, 2022 at 04:56:42PM +0200, Andy Shevchenko wrote: > On Thu, Jan 06, 2022 at 07:03:05PM -0600, Bjorn Helgaas wrote: > > On Tue, Dec 21, 2021 at 08:15:21PM +0200, Andy Shevchenko wrote: > > And, of course, if the OS cannot enumerate a PCI device, obviously > > it cannot reassign any BARs on this device it doesn't know about. > > Exactly. And I believe this explains why the region is excluded from > _CRS and why we mustn't reallocate it. It probably will work (again, > no-one have broadly tested this), but it will consume resources > which can be used by others (like Thunderbolt). Firmware excluded it from _CRS because it knows the region is in use. If it *were* in _CRS, the OS would see that no device uses it, so it could assign it to some PCI device, and an MMIO read would get two responses. If you manually reassign that BAR to some unused address space elsewhere, we have no idea what would happen. Since the device is not described anywhere, we have no idea what address space even *reaches* the device. And if firmware is using the device, changing the BAR will likely break whatever firmware is doing. > > The hardware and BIOS went to some trouble to hide this MMIO space > > from the OS, but now the OS wants to use it. I'm pretty sure the > > systems were never tested against *that* configuration either :) > > What do you mean? The unhide/hide back has been tested and we have > already users in the kernel (they have other issues though with the > PCI rescan lock, but it doesn't mean it wasn't ever tested). Does the firmware team that hid this device sign off on the OS unhiding and using it? How do we know that BIOS is not using the device? > > And the fact that they went to all this trouble to hide it means > > the BIOS is likely using it for its own purposes and the OS may > > cause conflicts if it also uses it. > > What purposes do you have in mind? The functionality implemented in the P2SB MMIO space is not specified, so I have no idea what it does or whether BIOS could be using it. But here's a hypothetical example: some platform firmware logs errors to NVRAM. That NVRAM could exist on a device like the P2SB, where the firmware assigns the MMIO address and hides the device from the OS. The firmware legitimately assumes it has exclusive control of the device and the OS will never touch it. If the OS unhides the device and also uses that NVRAM, the platform error logging no longer works. My point is that the unhide is architecturally messed up. The OS runs on the platform as described by ACPI. Devices that cannot be enumerated are described in the ACPI namespace. If the OS goes outside that ACPI-described platform and pokes at things it "knows" should be there, the architectural model falls apart. The OS relies on things the firmware didn't guarantee, and the firmware can't rely on non-interference from the OS. If you want to go outside the ACPI model, that's up to you, but I don't think we should tweak the PCI core to work with things that the BIOS has explicitly arranged to *not* be PCI devices. > > The way the BIOS has this set up, P2SB is logically not a PCI > > device. It is not enumerable. The MMIO space it uses is not in > > the _CRS of a PCI host bridge. That means it's now a platform > > device. > > I do not follow what you are implying here. On an ACPI system, the way we enumerate PCI devices is to find all the PCI host bridges (ACPI PNP0A03 devices), and scan config space to find the PCI devices below them. That doesn't find P2SB, so from a software point of view, it is not a PCI device. Platform devices are by definition non-enumerable, and they have to be described via ACPI, DT, or some kind of platform-specific code. P2SB is not enumerable, so I think a platform device is the most natural way to handle it. > As you see the code, it's not a driver, it's a library that reuses > PCI functions because the hardware is represented by an IP inside > PCI hierarchy and with PCI programming interface. Yes, it's a PCI programming interface at the hardware level, but at the software level, it's not part of PCI. This series does quite a lot of work in the PCI core to read that one register in a device the PCI core doesn't know about. I think it will be simpler overall if instead of wedging this into PCI, we make p2sb.c start with the ECAM base, ioremap() it, compute the register address, readl() the MMIO address, and be done with it. No need to deal with pci_find_bus(), pci_lock_rescan_remove(), change the core's BAR sizing code, etc. > > The correct way to use this would be as an ACPI device so the OS > > can enumerate it and the firmware can mediate access to it. Going > > behind the back of the firmware does not sound advisable to me. > > Are you going to fix all firmwares and devices on the market? We > have it's done like this and unfortunately we can't fix what's is > done due to users who won't update their firmwares by one or another > reason. I just mean that from a platform design standpoint, an ACPI device would be the right way to do this. Obviously it's not practical to add that to systems in the field. You could create a platform_device manually now, and if there ever is an ACPI device, ACPI can create a platform_device for you. > > If you want to hack something in, I think it might be easier to > > treat this purely as a platform device instead of a PCI device. > > You can hack up the config accesses you need, discover the MMIO > > address, plug that in as a resource of the platform device, and go > > wild. I don't think the PCI core needs to be involved at all. > > Sorry, I do not follow you. The device is PCI, but it's taken out of > PCI subsystem control by this hardware trick. The electrical connection might be PCI, but from the software point of view, it's only a PCI device if it can be enumerated by the mechanism specified by the spec, namely, reading the Vendor ID of each potential device. Yes, doing it as a platform device would involve some code in p2sb.c that looks sort of like code in the PCI core. But I don't think it's actually very much, and I think it would be less confusing than trying to pretend that this device sometimes behaves like a PCI device and sometimes not. > There are document numbers that make sense. > I believe that > > [2]: https://cdrdv2.intel.com/v1/dl/getContent/332690?wapkw=332690 > [3]: https://cdrdv2.intel.com/v1/dl/getContent/332691?wapkw=332691 > > work for you. Tell me if not (Meanwhile I have changed locally) Great, thanks. The links work for me (currently). I think a proper citation would also include the document title and document number, since I doubt Intel guarantees those URLs will work forever. Bjorn
On Fri, Jan 07, 2022 at 11:11:08AM -0600, Bjorn Helgaas wrote: > On Fri, Jan 07, 2022 at 04:56:42PM +0200, Andy Shevchenko wrote: > > On Thu, Jan 06, 2022 at 07:03:05PM -0600, Bjorn Helgaas wrote: > > > On Tue, Dec 21, 2021 at 08:15:21PM +0200, Andy Shevchenko wrote: ... > > The unhide/hide back has been tested and we have > > already users in the kernel (they have other issues though with the > > PCI rescan lock, but it doesn't mean it wasn't ever tested). > > Does the firmware team that hid this device sign off on the OS > unhiding and using it? How do we know that BIOS is not using the > device? BIOS might use the device via OperationRegion() in ACPI, but that means that _CRS needs to have that region available. It seems not the case. And as far I as see in the internal documentation the hide / unhide approach is not forbidden for OS side. Moreover, we have already this approach in the 3 device drivers on different platforms. If you not agree with it, probably you can send a removal to that drivers. In the terms of use this code doesn't change the status quo. What it does is the concentration of the p2sb code in one place as a library on obvious (?) purposes, e.g. maintenance. > > > And the fact that they went to all this trouble to hide it means > > > the BIOS is likely using it for its own purposes and the OS may > > > cause conflicts if it also uses it. > > > > What purposes do you have in mind? > > The functionality implemented in the P2SB MMIO space is not specified, > so I have no idea what it does or whether BIOS could be using it. It's specified based on how MMIO address is encoded. The third byte (bits [23:16]) representing the port ID on IOSF that belongs to the certain IPs, such as GPIO. > But here's a hypothetical example: some platform firmware logs errors > to NVRAM. That NVRAM could exist on a device like the P2SB, where the > firmware assigns the MMIO address and hides the device from the OS. > The firmware legitimately assumes it has exclusive control of the > device and the OS will never touch it. If the OS unhides the device > and also uses that NVRAM, the platform error logging no longer works. > > My point is that the unhide is architecturally messed up. The OS runs > on the platform as described by ACPI. Devices that cannot be > enumerated are described in the ACPI namespace. This device may or may not be _partially_ or _fully_ (due to being multifunctional) described in ACPI. I agree, that ideally the devices in question it has behind should be represented properly by firmware. However, the firmwares in the wild for selected products / devices don't do that. We need to solve (work around) it in the software. This is already done for a few devices. This series consolidates that and enables it for very known GPIO IPs. > If the OS goes outside that ACPI-described platform and pokes at > things it "knows" should be there, the architectural model falls > apart. The OS relies on things the firmware didn't guarantee, and > the firmware can't rely on non-interference from the OS. > > If you want to go outside the ACPI model, that's up to you, but I > don't think we should tweak the PCI core to work with things that > the BIOS has explicitly arranged to *not* be PCI devices. PCI core just provides a code that is very similar to what we need here. Are you specifically suggesting that we have to copy'n'paste that rather long function and maintain in parallel with PCI? > > > The way the BIOS has this set up, P2SB is logically not a PCI > > > device. It is not enumerable. The MMIO space it uses is not in > > > the _CRS of a PCI host bridge. That means it's now a platform > > > device. > > > > I do not follow what you are implying here. > > On an ACPI system, the way we enumerate PCI devices is to find all the > PCI host bridges (ACPI PNP0A03 devices), and scan config space to find > the PCI devices below them. That doesn't find P2SB, so from a > software point of view, it is not a PCI device. It's a PCI device that has a PCI programming interface but it has some tricks behind. Do you mean that those tricks automatically make it non-PCI (software speaking) compatible? > Platform devices are by definition non-enumerable, and they have to be > described via ACPI, DT, or some kind of platform-specific code. P2SB > is not enumerable, so I think a platform device is the most natural > way to handle it. How does it fit the proposed library model? Are you suggesting to create a hundreds of LOCs in order just to have some platform device which does what? I do not follow here the design you are proposing, sorry. > > As you see the code, it's not a driver, it's a library that reuses > > PCI functions because the hardware is represented by an IP inside > > PCI hierarchy and with PCI programming interface. > > Yes, it's a PCI programming interface at the hardware level, but at > the software level, it's not part of PCI. Why? > This series does quite a lot of work in the PCI core to read that one > register in a device the PCI core doesn't know about. I think it will > be simpler overall if instead of wedging this into PCI, we make p2sb.c > start with the ECAM base, ioremap() it, compute the register address, > readl() the MMIO address, and be done with it. No need to deal with > pci_find_bus(), pci_lock_rescan_remove(), change the core's BAR sizing > code, etc. So, you are suggesting to write a (simplified) PCI core for the certain device, did I get you right? Would it have good long-term maintenance perspective? > > > The correct way to use this would be as an ACPI device so the OS > > > can enumerate it and the firmware can mediate access to it. Going > > > behind the back of the firmware does not sound advisable to me. > > > > Are you going to fix all firmwares and devices on the market? We > > have it's done like this and unfortunately we can't fix what's is > > done due to users who won't update their firmwares by one or another > > reason. > > I just mean that from a platform design standpoint, an ACPI device > would be the right way to do this. Obviously it's not practical to > add that to systems in the field. You could create a platform_device > manually now, and if there ever is an ACPI device, ACPI can create a > platform_device for you. Why do I need that device? What for? I really don't see a point here. > > > If you want to hack something in, I think it might be easier to > > > treat this purely as a platform device instead of a PCI device. > > > You can hack up the config accesses you need, discover the MMIO > > > address, plug that in as a resource of the platform device, and go > > > wild. I don't think the PCI core needs to be involved at all. > > > > Sorry, I do not follow you. The device is PCI, but it's taken out of > > PCI subsystem control by this hardware trick. > > The electrical connection might be PCI, but from the software point of > view, it's only a PCI device if it can be enumerated by the mechanism > specified by the spec, namely, reading the Vendor ID of each potential > device. > > Yes, doing it as a platform device would involve some code in p2sb.c > that looks sort of like code in the PCI core. But I don't think it's > actually very much, and I think it would be less confusing than trying > to pretend that this device sometimes behaves like a PCI device and > sometimes not. So, duplicating code is good, right? Why do we have libraries in the code? > > There are document numbers that make sense. > > I believe that > > > > [2]: https://cdrdv2.intel.com/v1/dl/getContent/332690?wapkw=332690 > > [3]: https://cdrdv2.intel.com/v1/dl/getContent/332691?wapkw=332691 > > > > work for you. Tell me if not (Meanwhile I have changed locally) > > Great, thanks. The links work for me (currently). I think a proper > citation would also include the document title and document number, > since I doubt Intel guarantees those URLs will work forever.
On Fri, Jan 28, 2022 at 08:30:20PM +0200, Andy Shevchenko wrote: > On Fri, Jan 07, 2022 at 11:11:08AM -0600, Bjorn Helgaas wrote: > > On Fri, Jan 07, 2022 at 04:56:42PM +0200, Andy Shevchenko wrote: > > ... > > > > The unhide/hide back has been tested and we have already users > > > in the kernel (they have other issues though with the PCI rescan > > > lock, but it doesn't mean it wasn't ever tested). > > > > Does the firmware team that hid this device sign off on the OS > > unhiding and using it? How do we know that BIOS is not using the > > device? > > BIOS might use the device via OperationRegion() in ACPI, but that > means that _CRS needs to have that region available. It seems not > the case. > > And as far I as see in the internal documentation the hide / unhide > approach is not forbidden for OS side. Unhiding is device-specific behavior, so generic PCI enumeration cannot use it. We have to know there's a P2SB device at some address before we can safely do a config write to it. PCI enumeration would learn there's a P2SB device at an address by reading a Vendor/Device ID. > > My point is that the unhide is architecturally messed up. The OS > > runs on the platform as described by ACPI. Devices that cannot be > > enumerated are described in the ACPI namespace. > > This device may or may not be _partially_ or _fully_ (due to being > multifunctional) described in ACPI. I agree, that ideally the > devices in question it has behind should be represented properly by > firmware. However, the firmwares in the wild for selected products > / devices don't do that. We need to solve (work around) it in the > software. > > This is already done for a few devices. This series consolidates > that and enables it for very known GPIO IPs. Consolidating the code to unhide the device and size the BAR is fine. I would prefer the PCI core to be involved as little as possible because we're violating some key assumptions and we could trip over those later. We're assuming the existence of P2SB based on the fact that we found some *other* device, we're assuming firmware isn't using P2SB (may be true now, but impossible to verify), we're assuming the P2SB BAR contains a valid address that's not used elsewhere but also won't be assigned to anything. > PCI core just provides a code that is very similar to what we need > here. Are you specifically suggesting that we have to copy'n'paste > that rather long function and maintain in parallel with PCI? I think we're talking about __pci_read_base(), which is currently an internal PCI interface. This series adds pci_bus_info/warn/etc(), reworks __pci_read_base() to operate on a struct pci_bus *, exports it, and uses it via #include <../../../pci/pci.h>. __pci_read_base() is fairly long, but you apparently don't need all the functionality there because the core of the patch is this: - pci_bus_read_config_dword(bus, spi, PCI_BASE_ADDRESS_0, - &spi_base); - if (spi_base != ~0) { - res->start = spi_base & 0xfffffff0; - res->end = res->start + SPIBASE_APL_SZ - 1; - } + __pci_bus_read_base(bus, devfn, pci_bar_unknown, mem, PCI_BASE_ADDRESS_0, true) I don't think it's worth all the __pci_read_base() changes to do that. What if you made a library function that looks like this? int p2sb_bar(...) { pci_bus_write_config_byte(bus, devfn_p2sb, P2SBC_HIDE_BYTE, 0); pci_bus_read_config_dword(bus, devfn, PCI_BASE_ADDRESS_0, &orig); if (orig) { pci_bus_write_config_dword(bus, devfn, PCI_BASE_ADDRESS_0, ~0); pci_bus_read_config_dword(bus, devfn, PCI_BASE_ADDRESS_0, &val); pci_bus_write_config_dword(bus, devfn, PCI_BASE_ADDRESS_0, orig); res->start = orig; res->end = res->start + (~val + 1); } pci_bus_write_config_byte(bus, devfn, P2SBC_HIDE_BYTE, P2SBC_HIDE_BIT); }
On Tue, Feb 01, 2022 at 12:14:01PM -0600, Bjorn Helgaas wrote: > On Fri, Jan 28, 2022 at 08:30:20PM +0200, Andy Shevchenko wrote: > > On Fri, Jan 07, 2022 at 11:11:08AM -0600, Bjorn Helgaas wrote: > > > On Fri, Jan 07, 2022 at 04:56:42PM +0200, Andy Shevchenko wrote: > > > > ... > > > > > > The unhide/hide back has been tested and we have already users > > > > in the kernel (they have other issues though with the PCI rescan > > > > lock, but it doesn't mean it wasn't ever tested). > > > > > > Does the firmware team that hid this device sign off on the OS > > > unhiding and using it? How do we know that BIOS is not using the > > > device? > > > > BIOS might use the device via OperationRegion() in ACPI, but that > > means that _CRS needs to have that region available. It seems not > > the case. > > > > And as far I as see in the internal documentation the hide / unhide > > approach is not forbidden for OS side. > > Unhiding is device-specific behavior, so generic PCI enumeration > cannot use it. We have to know there's a P2SB device at some address > before we can safely do a config write to it. PCI enumeration would > learn there's a P2SB device at an address by reading a Vendor/Device > ID. > > > > My point is that the unhide is architecturally messed up. The OS > > > runs on the platform as described by ACPI. Devices that cannot be > > > enumerated are described in the ACPI namespace. > > > > This device may or may not be _partially_ or _fully_ (due to being > > multifunctional) described in ACPI. I agree, that ideally the > > devices in question it has behind should be represented properly by > > firmware. However, the firmwares in the wild for selected products > > / devices don't do that. We need to solve (work around) it in the > > software. > > > > This is already done for a few devices. This series consolidates > > that and enables it for very known GPIO IPs. > > Consolidating the code to unhide the device and size the BAR is fine. > > I would prefer the PCI core to be involved as little as possible > because we're violating some key assumptions and we could trip over > those later. We're assuming the existence of P2SB based on the fact > that we found some *other* device, we're assuming firmware isn't using > P2SB (may be true now, but impossible to verify), we're assuming the > P2SB BAR contains a valid address that's not used elsewhere but also > won't be assigned to anything. > > > PCI core just provides a code that is very similar to what we need > > here. Are you specifically suggesting that we have to copy'n'paste > > that rather long function and maintain in parallel with PCI? > > I think we're talking about __pci_read_base(), which is currently an > internal PCI interface. This series adds pci_bus_info/warn/etc(), The patch that adds those macros is good on its own, if you think so... I tried to submit it separately, but it was no response, so I don't know. > reworks __pci_read_base() to operate on a struct pci_bus *, exports > it, and uses it via #include <../../../pci/pci.h>. Yes, which allows at least to have the same code, doing same things to be in one copy in one place. > __pci_read_base() is fairly long, but you apparently don't need all > the functionality there because the core of the patch is this: > > - pci_bus_read_config_dword(bus, spi, PCI_BASE_ADDRESS_0, > - &spi_base); > - if (spi_base != ~0) { > - res->start = spi_base & 0xfffffff0; > - res->end = res->start + SPIBASE_APL_SZ - 1; > - } > + __pci_bus_read_base(bus, devfn, pci_bar_unknown, mem, PCI_BASE_ADDRESS_0, true) You probably took the least pleasant (to me) example, because it's buggy in a few ways: - it misses 64-bit handling code - it misses PCI rescan lock (in case PCI code decides to change addresses, previously ones will be invalid, while other drivers may still use that MMIO space - it doesn't check if (for a new version Hans suggested me to add this check as it's done in one out of 3 cases) It also useful to have some messages to be printed just in cases of errors or success in a standard (PCI core provided) way. > I don't think it's worth all the __pci_read_base() changes to do that. > What if you made a library function that looks like this? > > int p2sb_bar(...) > { > pci_bus_write_config_byte(bus, devfn_p2sb, P2SBC_HIDE_BYTE, 0); > pci_bus_read_config_dword(bus, devfn, PCI_BASE_ADDRESS_0, &orig); > if (orig) { > pci_bus_write_config_dword(bus, devfn, PCI_BASE_ADDRESS_0, ~0); > pci_bus_read_config_dword(bus, devfn, PCI_BASE_ADDRESS_0, &val); > pci_bus_write_config_dword(bus, devfn, PCI_BASE_ADDRESS_0, orig); > res->start = orig; > res->end = res->start + (~val + 1); > } > pci_bus_write_config_byte(bus, devfn, P2SBC_HIDE_BYTE, P2SBC_HIDE_BIT); > } It seems simple, but with the above mentioned adjustments, it will become closer to the size of the original __pci_read_base().
On Tue, Feb 01, 2022 at 08:52:22PM +0200, Andy Shevchenko wrote: > On Tue, Feb 01, 2022 at 12:14:01PM -0600, Bjorn Helgaas wrote: > > On Fri, Jan 28, 2022 at 08:30:20PM +0200, Andy Shevchenko wrote: > > > On Fri, Jan 07, 2022 at 11:11:08AM -0600, Bjorn Helgaas wrote: > > > > On Fri, Jan 07, 2022 at 04:56:42PM +0200, Andy Shevchenko wrote: > > > > > > ... > > > > > > > My point is that the unhide is architecturally messed up. The OS > > > > runs on the platform as described by ACPI. Devices that cannot be > > > > enumerated are described in the ACPI namespace. > > > > > > This device may or may not be _partially_ or _fully_ (due to being > > > multifunctional) described in ACPI. I agree, that ideally the > > > devices in question it has behind should be represented properly by > > > firmware. However, the firmwares in the wild for selected products > > > / devices don't do that. We need to solve (work around) it in the > > > software. > > > > > > This is already done for a few devices. This series consolidates > > > that and enables it for very known GPIO IPs. > > > > Consolidating the code to unhide the device and size the BAR is fine. > > > > I would prefer the PCI core to be involved as little as possible > > because we're violating some key assumptions and we could trip over > > those later. We're assuming the existence of P2SB based on the fact > > that we found some *other* device, we're assuming firmware isn't using > > P2SB (may be true now, but impossible to verify), we're assuming the > > P2SB BAR contains a valid address that's not used elsewhere but also > > won't be assigned to anything. > > > > > PCI core just provides a code that is very similar to what we need > > > here. Are you specifically suggesting that we have to copy'n'paste > > > that rather long function and maintain in parallel with PCI? > > > > I think we're talking about __pci_read_base(), which is currently an > > internal PCI interface. This series adds pci_bus_info/warn/etc(), > > The patch that adds those macros is good on its own, if you think so... > I tried to submit it separately, but it was no response, so I don't know. I'd rather not add pci_bus_info(), etc. There are only a couple places that could use it, and if we cared enough, I think those places could avoid it by doing pci_alloc_dev() first. What if you used pci_alloc_dev() and then called the current __pci_read_base() on the pci_dev *? The caller would still have the ugly #include path, but I guess you're OK with that. Since the P2SB BAR is not included in the host bridge _CRS, the pcibios_bus_to_resource() done by __pci_read_base() won't work correctly, so this only "works" on host bridges with no address translation. But that's also the case with your current series. This is an example of one of the PCI core assumptions being violated, so things can break. Bjorn
diff --git a/drivers/platform/x86/intel/Kconfig b/drivers/platform/x86/intel/Kconfig index 38ce3e344589..e0cc64dcf72c 100644 --- a/drivers/platform/x86/intel/Kconfig +++ b/drivers/platform/x86/intel/Kconfig @@ -81,6 +81,18 @@ config INTEL_OAKTRAIL enable/disable the Camera, WiFi, BT etc. devices. If in doubt, say Y here; it will only load on supported platforms. +config P2SB + bool "Primary to Sideband (P2SB) bridge access support" + depends on PCI + help + The Primary to Sideband (P2SB) bridge is an interface to some + PCI devices connected through it. In particular, SPI NOR controller + in Intel Apollo Lake SoC is one of such devices. + + The main purpose of this library is to unhide P2SB device in case + firmware kept it hidden on some platforms in order to access devices + behind it. + config INTEL_BXTWC_PMIC_TMU tristate "Intel Broxton Whiskey Cove TMU Driver" depends on INTEL_SOC_PMIC_BXTWC diff --git a/drivers/platform/x86/intel/Makefile b/drivers/platform/x86/intel/Makefile index 7c24be2423d8..b1f74b3f9c29 100644 --- a/drivers/platform/x86/intel/Makefile +++ b/drivers/platform/x86/intel/Makefile @@ -26,6 +26,8 @@ intel_int0002_vgpio-y := int0002_vgpio.o obj-$(CONFIG_INTEL_INT0002_VGPIO) += intel_int0002_vgpio.o intel_oaktrail-y := oaktrail.o obj-$(CONFIG_INTEL_OAKTRAIL) += intel_oaktrail.o +intel_p2sb-y := p2sb.o +obj-$(CONFIG_P2SB) += intel_p2sb.o # Intel PMIC / PMC / P-Unit drivers intel_bxtwc_tmu-y := bxtwc_tmu.o diff --git a/drivers/platform/x86/intel/p2sb.c b/drivers/platform/x86/intel/p2sb.c new file mode 100644 index 000000000000..b47517572310 --- /dev/null +++ b/drivers/platform/x86/intel/p2sb.c @@ -0,0 +1,93 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Primary to Sideband (P2SB) bridge access support + * + * Copyright (c) 2017, 2021 Intel Corporation. + * + * Authors: Andy Shevchenko <andriy.shevchenko@linux.intel.com> + * Jonathan Yong <jonathan.yong@intel.com> + */ + +#include <linux/bitops.h> +#include <linux/export.h> +#include <linux/pci.h> +#include <linux/platform_data/x86/p2sb.h> + +/* For __pci_bus_read_base(), which is available for the PCI subsystem */ +#include <../../../pci/pci.h> + +#include <asm/cpu_device_id.h> +#include <asm/intel-family.h> + +#define P2SBC_HIDE_BYTE 0xe1 +#define P2SBC_HIDE_BIT BIT(0) + +static const struct x86_cpu_id p2sb_cpu_ids[] = { + X86_MATCH_INTEL_FAM6_MODEL(ATOM_GOLDMONT, PCI_DEVFN(13, 0)), + {} +}; + +static int p2sb_get_devfn(unsigned int *devfn) +{ + const struct x86_cpu_id *id; + + id = x86_match_cpu(p2sb_cpu_ids); + if (!id) + return -ENODEV; + + *devfn = (unsigned int)id->driver_data; + return 0; +} + +/** + * p2sb_bar - Get Primary to Sideband (P2SB) bridge device BAR + * @bus: PCI bus to communicate with + * @devfn: PCI slot and function to communicate with + * @mem: memory resource to be filled in + * + * The BIOS prevents the P2SB device from being enumerated by the PCI + * subsystem, so we need to unhide and hide it back to lookup the BAR. + * + * if @bus is NULL, the bus 0 in domain 0 will be in use. + * If @devfn is 0, it will be replaced by devfn of the P2SB device. + * + * Caller must provide a valid pointer to @mem. + * + * Locking is handled by pci_rescan_remove_lock mutex. + * + * Return: + * 0 on success or appropriate errno value on error. + */ +int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem) +{ + unsigned int devfn_p2sb; + int ret; + + /* Get devfn for P2SB device itself */ + ret = p2sb_get_devfn(&devfn_p2sb); + if (ret) + return ret; + + /* if @pdev is NULL, use bus 0 in domain 0 */ + bus = bus ?: pci_find_bus(0, 0); + + /* If @devfn is 0, replace it with devfn of P2SB device itself */ + devfn = devfn ?: devfn_p2sb; + + pci_lock_rescan_remove(); + + /* Unhide the P2SB device */ + pci_bus_write_config_byte(bus, devfn_p2sb, P2SBC_HIDE_BYTE, 0); + + /* Read the first BAR of the device in question */ + __pci_bus_read_base(bus, devfn, pci_bar_unknown, mem, PCI_BASE_ADDRESS_0, true); + + /* Hide the P2SB device */ + pci_bus_write_config_byte(bus, devfn_p2sb, P2SBC_HIDE_BYTE, P2SBC_HIDE_BIT); + + pci_unlock_rescan_remove(); + + pci_bus_info(bus, devfn, "BAR: %pR\n", mem); + return 0; +} +EXPORT_SYMBOL_GPL(p2sb_bar); diff --git a/include/linux/platform_data/x86/p2sb.h b/include/linux/platform_data/x86/p2sb.h new file mode 100644 index 000000000000..2f71de65aee4 --- /dev/null +++ b/include/linux/platform_data/x86/p2sb.h @@ -0,0 +1,27 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Primary to Sideband (P2SB) bridge access support + */ + +#ifndef _PLATFORM_DATA_X86_P2SB_H +#define _PLATFORM_DATA_X86_P2SB_H + +#include <linux/errno.h> + +struct pci_bus; +struct resource; + +#if IS_BUILTIN(CONFIG_P2SB) + +int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem); + +#else /* CONFIG_P2SB */ + +static inline int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem) +{ + return -ENODEV; +} + +#endif /* CONFIG_P2SB is not set */ + +#endif /* _PLATFORM_DATA_X86_P2SB_H */