| Message ID | 1467335453-25967-4-git-send-email-lersek@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 07/04/16 09:38, Ni, Ruiyu wrote: > Adding Kinney. > > Laszlo, > Your patch assumes all PCIE slot are hot plug capable. Yes, it does assume that. > But why PCIE spec 3.0 chapter 7.8.9 Slot Capabilities Register (Offset 14h) > contains a BIT called Hot-Plug Capable? > Does your patch also need to check the Hot-Plug Capable bit as well? I don't know :) I tried to follow the logic that I found in SeaBIOS and in UefiShellDebug1CommandsLib. I think this is a valid question, of course. I'd just like to forward it to Marcel and Alex :) It is perfectly possible, as far as I understand, that QEMU's virtual hardware guarantees a "shortcut" that is not a given with physical hardware. So if the check must be restricted a bit, I'm glad to do it. > BTW, though I am the owner of PciBus driver in EDKII, I am really a newbie to > this area. > Before your patch, I always thought PciHotPlugInit.GetRootHpcList() should > return all hot plug PCIE ports. But after seeing your patch, I went back to > re-read the PI spec and found some words like "root HPCs", "may not be > able to detect these HPCs", which let me agree with your understanding > that GetRootHpcList() only returns these HPCs that cannot be detected by > PciBus. Thank you very much for spending time on this. I stared at the same PI spec pages that you mention for a long time, and ultimately I thought that GetRootHpcList() should return only those hotplug-providing ports that are non-enumerable. In other words, the spec's wording "root Hot Plug Controllers" does not mean "PCIe root ports"; instead it seems to mean "a hotplug controller that cannot be found by way of the usual enumeration (and might need special initialization)". To my knowledge, QEMU has no such thing. (Definitely not in the scope of this series.) Thanks! Laszlo >> -----Original Message----- >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of >> Laszlo Ersek >> Sent: Friday, July 1, 2016 9:11 AM >> To: edk2-devel-01 <edk2-devel@ml01.01.org> >> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Tian, Feng <feng.tian@intel.com>; >> Johnson, Brian J. <bjohnson@sgi.com>; Andrew Fish <afish@apple.com>; >> Justen, Jordan L <jordan.l.justen@intel.com>; Marcel Apfelbaum >> <marcel@redhat.com>; Zeng, Star <star.zeng@intel.com> >> Subject: [edk2] [PATCH 3/4] MdeModulePkg/PciBusDxe: recognize hotplug- >> capable PCIe ports >> >> Section 7.8.2 of the PCI Express specification (r4.0 v0.3), entitled "PCI Express >> Capabilities Register (Offset 02h)", describes the conditions when a PCIe port >> should be considered "supporting hotplug": >> - it should be a root complex port or a switch downstream port, and >> - it should have the "Slot Implemented" bit set. >> >> This logic is already implemented in at least two open source projects I could >> find: >> >> - in SeaBIOS by Marcel Apfelbaum: "hw/pci: reserve IO and mem for pci >> express downstream ports with no devices attached" >> <https://code.coreboot.org/p/seabios/source/commit/3aa31d7d6375>, >> >> - in edk2 itself, in the implementation of the "PCI" UEFI Shell command: >> see the "PcieExplainTypeSlot" case label in function >> PciExplainPciExpress(), file >> "ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c". >> >> PciBusDxe recognizes such PCIe ports as bridges, but it doesn't realize they >> support hotplug. In turn PciBusDxe omits getting any resource padding >> information from the platform's EFI_PCI_HOT_PLUG_INIT_PROTOCOL for >> these >> bridges: >> >> GatherPpbInfo() [PciEnumeratorSupport.c] >> GetResourcePaddingPpb() [PciResourceSupport.c] >> GetResourcePaddingForHpb() [PciHotPlugSupport.c] >> IsPciHotPlugBus() [PciHotPlugSupport.c] >> // >> // returns FALSE >> // >> // >> // the following is not reached: >> // >> gPciHotPlugInit->GetResourcePadding() >> >> Implement a function called SupportsPcieHotplug() for identifying such ports, >> and call it from IsPciHotPlugBus() (after the call to IsSHPC()). >> >> Cc: "Johnson, Brian J." <bjohnson@sgi.com> >> Cc: Alex Williamson <alex.williamson@redhat.com> >> Cc: Andrew Fish <afish@apple.com> >> Cc: Feng Tian <feng.tian@intel.com> >> Cc: Jordan Justen <jordan.l.justen@intel.com> >> Cc: Marcel Apfelbaum <marcel@redhat.com> >> Cc: Ruiyu Ni <ruiyu.ni@intel.com> >> Cc: Star Zeng <star.zeng@intel.com> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >> --- >> MdeModulePkg/Bus/Pci/PciBusDxe/PciHotPlugSupport.h | 19 ++++++ >> MdeModulePkg/Bus/Pci/PciBusDxe/PciHotPlugSupport.c | 71 >> ++++++++++++++++++++ >> 2 files changed, 90 insertions(+) >> >> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciHotPlugSupport.h >> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciHotPlugSupport.h >> index 1fb9ba972091..6e02b8b98bf0 100644 >> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciHotPlugSupport.h >> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciHotPlugSupport.h >> @@ -177,6 +177,25 @@ IsSHPC ( >> ); >> >> /** >> + Check whether PciIoDevice supports PCIe hotplug. >> + >> + This is equivalent to the following condition: >> + - the device is either a PCIe switch downstream port or a root port, >> + - and the device has the SlotImplemented bit set in its PCIe capability >> + register. >> + >> + @param[in] PciIoDevice The device being checked. >> + >> + @retval TRUE PciIoDevice is a PCIe port that accepts a hotplugged device. >> + @retval FALSE Otherwise. >> + >> +**/ >> +BOOLEAN >> +SupportsPcieHotplug ( >> + IN PCI_IO_DEVICE *PciIoDevice >> + ); >> + >> +/** >> Get resource padding if the specified PCI bridge is a hot plug bus. >> >> @param PciIoDevice PCI bridge instance. >> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciHotPlugSupport.c >> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciHotPlugSupport.c >> index ca8766869ae7..19dd97a4528d 100644 >> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciHotPlugSupport.c >> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciHotPlugSupport.c >> @@ -317,6 +317,69 @@ IsSHPC ( >> } >> >> /** >> + Check whether PciIoDevice supports PCIe hotplug. >> + >> + This is equivalent to the following condition: >> + - the device is either a PCIe switch downstream port or a root port, >> + - and the device has the SlotImplemented bit set in its PCIe capability >> + register. >> + >> + @param[in] PciIoDevice The device being checked. >> + >> + @retval TRUE PciIoDevice is a PCIe port that accepts a hotplugged device. >> + @retval FALSE Otherwise. >> + >> +**/ >> +BOOLEAN >> +SupportsPcieHotplug ( >> + IN PCI_IO_DEVICE *PciIoDevice >> + ) >> +{ >> + UINT32 Offset; >> + EFI_STATUS Status; >> + PCI_REG_PCIE_CAPABILITY Capability; >> + >> + if (PciIoDevice == NULL) { >> + return FALSE; >> + } >> + >> + // >> + // Read the PCI Express Capabilities Register // if >> + (!PciIoDevice->IsPciExp) { >> + return FALSE; >> + } >> + Offset = PciIoDevice->PciExpressCapabilityOffset + >> + OFFSET_OF (PCI_CAPABILITY_PCIEXP, Capability); >> + >> + Status = PciIoDevice->PciIo.Pci.Read ( >> + &PciIoDevice->PciIo, >> + EfiPciIoWidthUint16, >> + Offset, >> + 1, >> + &Capability >> + ); >> + if (EFI_ERROR (Status)) { >> + return FALSE; >> + } >> + >> + // >> + // Check the contents of the register >> + // >> + switch (Capability.Bits.DevicePortType) { >> + case PCIE_DEVICE_PORT_TYPE_ROOT_PORT: >> + case PCIE_DEVICE_PORT_TYPE_DOWNSTREAM_PORT: >> + break; >> + default: >> + return FALSE; >> + } >> + if (Capability.Bits.SlotImplemented) { >> + return TRUE; >> + } >> + return FALSE; >> +} >> + >> +/** >> Get resource padding if the specified PCI bridge is a hot plug bus. >> >> @param PciIoDevice PCI bridge instance. >> @@ -382,6 +445,14 @@ IsPciHotPlugBus ( >> return TRUE; >> } >> >> + if (SupportsPcieHotplug (PciIoDevice)) { >> + // >> + // If the PPB is a PCIe root complex port or a switch downstream port, and >> + // implements a slot, then also return TRUE. >> + // >> + return TRUE; >> + } >> + >> // >> // Otherwise, see if it is a Root HPC >> // >> -- >> 1.8.3.1 >> >> >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 07/05/16 12:54, Laszlo Ersek wrote: > On 07/04/16 09:38, Ni, Ruiyu wrote: >> Adding Kinney. >> >> Laszlo, >> Your patch assumes all PCIE slot are hot plug capable. > > Yes, it does assume that. > >> But why PCIE spec 3.0 chapter 7.8.9 Slot Capabilities Register (Offset 14h) >> contains a BIT called Hot-Plug Capable? >> Does your patch also need to check the Hot-Plug Capable bit as well? > > I don't know :) I tried to follow the logic that I found in SeaBIOS and > in UefiShellDebug1CommandsLib. > > I think this is a valid question, of course. I'd just like to forward it > to Marcel and Alex :) It is perfectly possible, as far as I understand, > that QEMU's virtual hardware guarantees a "shortcut" that is not a given > with physical hardware. So if the check must be restricted a bit, I'm > glad to do it. I checked the QEMU implementation in: - hw/pci-bridge/xio3130_downstream.c (downstream port) - hw/pci-bridge/ioh3420.c (root port) They both call pcie_cap_slot_init(). The pcie_cap_slot_init() function [hw/pci/pcie.c] sets the "Hot-Plug Capable" bit (= bit 6) in the Slot Capabilities Register (Offset 14h): uint32_t pos = dev->exp.exp_cap; ... pci_long_test_and_set_mask(dev->config + pos + PCI_EXP_SLTCAP, (slot << PCI_EXP_SLTCAP_PSN_SHIFT) | PCI_EXP_SLTCAP_EIP | PCI_EXP_SLTCAP_HPS | PCI_EXP_SLTCAP_HPC | PCI_EXP_SLTCAP_PIP | PCI_EXP_SLTCAP_AIP | PCI_EXP_SLTCAP_ABP); where the relevant macros are [include/standard-headers/linux/pci_regs.h]: #define PCI_EXP_SLTCAP 20 /* Slot Capabilities */ ... #define PCI_EXP_SLTCAP_HPC 0x00000040 /* Hot-Plug Capable */ Also, from the PCIe spec ("7.8. PCI Express Capability Structure"): [...] The Slot Capabilities register is required if the Slot Implemented bit is Set (see Section 7.8.2). [...] so it shouldn't be hard to implement your suggestion for v2 of this patch; I'll do it. Thanks! Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciHotPlugSupport.h b/MdeModulePkg/Bus/Pci/PciBusDxe/PciHotPlugSupport.h index 1fb9ba972091..6e02b8b98bf0 100644 --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciHotPlugSupport.h +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciHotPlugSupport.h @@ -177,6 +177,25 @@ IsSHPC ( ); /** + Check whether PciIoDevice supports PCIe hotplug. + + This is equivalent to the following condition: + - the device is either a PCIe switch downstream port or a root port, + - and the device has the SlotImplemented bit set in its PCIe capability + register. + + @param[in] PciIoDevice The device being checked. + + @retval TRUE PciIoDevice is a PCIe port that accepts a hotplugged device. + @retval FALSE Otherwise. + +**/ +BOOLEAN +SupportsPcieHotplug ( + IN PCI_IO_DEVICE *PciIoDevice + ); + +/** Get resource padding if the specified PCI bridge is a hot plug bus. @param PciIoDevice PCI bridge instance. diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciHotPlugSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciHotPlugSupport.c index ca8766869ae7..19dd97a4528d 100644 --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciHotPlugSupport.c +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciHotPlugSupport.c @@ -317,6 +317,69 @@ IsSHPC ( } /** + Check whether PciIoDevice supports PCIe hotplug. + + This is equivalent to the following condition: + - the device is either a PCIe switch downstream port or a root port, + - and the device has the SlotImplemented bit set in its PCIe capability + register. + + @param[in] PciIoDevice The device being checked. + + @retval TRUE PciIoDevice is a PCIe port that accepts a hotplugged device. + @retval FALSE Otherwise. + +**/ +BOOLEAN +SupportsPcieHotplug ( + IN PCI_IO_DEVICE *PciIoDevice + ) +{ + UINT32 Offset; + EFI_STATUS Status; + PCI_REG_PCIE_CAPABILITY Capability; + + if (PciIoDevice == NULL) { + return FALSE; + } + + // + // Read the PCI Express Capabilities Register + // + if (!PciIoDevice->IsPciExp) { + return FALSE; + } + Offset = PciIoDevice->PciExpressCapabilityOffset + + OFFSET_OF (PCI_CAPABILITY_PCIEXP, Capability); + + Status = PciIoDevice->PciIo.Pci.Read ( + &PciIoDevice->PciIo, + EfiPciIoWidthUint16, + Offset, + 1, + &Capability + ); + if (EFI_ERROR (Status)) { + return FALSE; + } + + // + // Check the contents of the register + // + switch (Capability.Bits.DevicePortType) { + case PCIE_DEVICE_PORT_TYPE_ROOT_PORT: + case PCIE_DEVICE_PORT_TYPE_DOWNSTREAM_PORT: + break; + default: + return FALSE; + } + if (Capability.Bits.SlotImplemented) { + return TRUE; + } + return FALSE; +} + +/** Get resource padding if the specified PCI bridge is a hot plug bus. @param PciIoDevice PCI bridge instance. @@ -382,6 +445,14 @@ IsPciHotPlugBus ( return TRUE; } + if (SupportsPcieHotplug (PciIoDevice)) { + // + // If the PPB is a PCIe root complex port or a switch downstream port, and + // implements a slot, then also return TRUE. + // + return TRUE; + } + // // Otherwise, see if it is a Root HPC //
Section 7.8.2 of the PCI Express specification (r4.0 v0.3), entitled "PCI Express Capabilities Register (Offset 02h)", describes the conditions when a PCIe port should be considered "supporting hotplug": - it should be a root complex port or a switch downstream port, and - it should have the "Slot Implemented" bit set. This logic is already implemented in at least two open source projects I could find: - in SeaBIOS by Marcel Apfelbaum: "hw/pci: reserve IO and mem for pci express downstream ports with no devices attached" <https://code.coreboot.org/p/seabios/source/commit/3aa31d7d6375>, - in edk2 itself, in the implementation of the "PCI" UEFI Shell command: see the "PcieExplainTypeSlot" case label in function PciExplainPciExpress(), file "ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c". PciBusDxe recognizes such PCIe ports as bridges, but it doesn't realize they support hotplug. In turn PciBusDxe omits getting any resource padding information from the platform's EFI_PCI_HOT_PLUG_INIT_PROTOCOL for these bridges: GatherPpbInfo() [PciEnumeratorSupport.c] GetResourcePaddingPpb() [PciResourceSupport.c] GetResourcePaddingForHpb() [PciHotPlugSupport.c] IsPciHotPlugBus() [PciHotPlugSupport.c] // // returns FALSE // // // the following is not reached: // gPciHotPlugInit->GetResourcePadding() Implement a function called SupportsPcieHotplug() for identifying such ports, and call it from IsPciHotPlugBus() (after the call to IsSHPC()). Cc: "Johnson, Brian J." <bjohnson@sgi.com> Cc: Alex Williamson <alex.williamson@redhat.com> Cc: Andrew Fish <afish@apple.com> Cc: Feng Tian <feng.tian@intel.com> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Marcel Apfelbaum <marcel@redhat.com> Cc: Ruiyu Ni <ruiyu.ni@intel.com> Cc: Star Zeng <star.zeng@intel.com> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- MdeModulePkg/Bus/Pci/PciBusDxe/PciHotPlugSupport.h | 19 ++++++ MdeModulePkg/Bus/Pci/PciBusDxe/PciHotPlugSupport.c | 71 ++++++++++++++++++++ 2 files changed, 90 insertions(+) -- 1.8.3.1 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel