Hi Ray, I have a work-in-progress series to convert OvmfPkg to use this driver. While testing it, I ran into a failed assertion:
On 01/13/16 09:00, Ruiyu Ni wrote: > This driver links to PciHostBridgeLib provided by platform/silicon to > produce PciRootBridgeIo and PciHostBridgeResourceAllocation protocol. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ruiyu Ni <[email protected]> > Cc: Jeff Fan <[email protected]> > Cc: Feng Tian <[email protected]> > --- > .../Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 1134 ++++++++++++++ > .../Bus/Pci/PciHostBridgeDxe/PciHostBridge.h | 252 ++++ > .../Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf | 55 + > .../Bus/Pci/PciHostBridgeDxe/PciHostResource.h | 47 + > .../Bus/Pci/PciHostBridgeDxe/PciRootBridge.h | 568 +++++++ > .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 1561 > ++++++++++++++++++++ > MdeModulePkg/MdeModulePkg.dsc | 2 + > 7 files changed, 3619 insertions(+) > create mode 100644 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c > create mode 100644 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.h > create mode 100644 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf > create mode 100644 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h > create mode 100644 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h > create mode 100644 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c > > diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c > b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c > new file mode 100644 > index 0000000..08285d8 > --- /dev/null > +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c > @@ -0,0 +1,1134 @@ > +/** @file > + > + Provides the basic interfaces to abstract a PCI Host Bridge Resource > Allocation. > + > +Copyright (c) 1999 - 2016, Intel Corporation. All rights reserved.<BR> > +This program and the accompanying materials > +are licensed and made available under the terms and conditions of the BSD > License > +which accompanies this distribution. The full text of the license may be > found at > +http://opensource.org/licenses/bsd-license.php > + > +THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > + > +**/ > + > +#include "PciHostBridge.h" > +#include "PciRootBridge.h" > +#include "PciHostResource.h" > + > + > +EFI_METRONOME_ARCH_PROTOCOL *mMetronome; > +EFI_CPU_IO2_PROTOCOL *mCpuIo; > + > +GLOBAL_REMOVE_IF_UNREFERENCED CHAR16 *mAcpiAddressSpaceTypeStr[] = { > + L"Mem", L"I/O", L"Bus" > +}; > +GLOBAL_REMOVE_IF_UNREFERENCED CHAR16 *mPciResourceTypeStr[] = { > + L"I/O", L"Mem", L"PMem", L"Mem64", L"PMem64", L"Bus" > +}; > + > +/** > + > + Entry point of this driver. > + > + @param ImageHandle Image handle of this driver. > + @param SystemTable Pointer to standard EFI system table. > + > + @retval EFI_SUCCESS Succeed. > + @retval EFI_DEVICE_ERROR Fail to install PCI_ROOT_BRIDGE_IO protocol. > + > +**/ > +EFI_STATUS > +EFIAPI > +InitializePciHostBridge ( > + IN EFI_HANDLE ImageHandle, > + IN EFI_SYSTEM_TABLE *SystemTable > + ) > +{ > + EFI_STATUS Status; > + PCI_HOST_BRIDGE_INSTANCE *HostBridge; > + PCI_ROOT_BRIDGE_INSTANCE *RootBridge; > + PCI_ROOT_BRIDGE *RootBridges; > + UINTN RootBridgeCount; > + UINTN Index; > + PCI_ROOT_BRIDGE_APERTURE *MemApertures[4]; > + UINTN MemApertureIndex; > + > + RootBridges = PciHostBridgeGetRootBridges (&RootBridgeCount); > + if ((RootBridges == NULL) || (RootBridgeCount == 0)) { > + return EFI_UNSUPPORTED; > + } > + > + Status = gBS->LocateProtocol (&gEfiMetronomeArchProtocolGuid, NULL, (VOID > **) &mMetronome); > + ASSERT_EFI_ERROR (Status); > + Status = gBS->LocateProtocol (&gEfiCpuIo2ProtocolGuid, NULL, (VOID **) > &mCpuIo); > + ASSERT_EFI_ERROR (Status); > + > + // > + // Most systems in the world including complex servers have only one Host > Bridge. > + // > + HostBridge = AllocateZeroPool (sizeof (PCI_HOST_BRIDGE_INSTANCE)); > + ASSERT (HostBridge != NULL); > + > + HostBridge->Signature = PCI_HOST_BRIDGE_SIGNATURE; > + HostBridge->CanRestarted = TRUE; > + InitializeListHead (&HostBridge->RootBridges); > + > + HostBridge->ResAlloc.NotifyPhase = NotifyPhase; > + HostBridge->ResAlloc.GetNextRootBridge = GetNextRootBridge; > + HostBridge->ResAlloc.GetAllocAttributes = GetAttributes; > + HostBridge->ResAlloc.StartBusEnumeration = StartBusEnumeration; > + HostBridge->ResAlloc.SetBusNumbers = SetBusNumbers; > + HostBridge->ResAlloc.SubmitResources = SubmitResources; > + HostBridge->ResAlloc.GetProposedResources = GetProposedResources; > + HostBridge->ResAlloc.PreprocessController = PreprocessController; > + > + Status = gBS->InstallMultipleProtocolInterfaces ( > + &HostBridge->Handle, > + &gEfiPciHostBridgeResourceAllocationProtocolGuid, > &HostBridge->ResAlloc, > + NULL > + ); > + if (EFI_ERROR (Status)) { > + FreePool (HostBridge); > + PciHostBridgeFreeRootBridges (RootBridges, RootBridgeCount); > + return Status; > + } > + > + // > + // Create Root Bridge Device Handle in this Host Bridge > + // > + for (Index = 0; Index < RootBridgeCount; Index++) { > + // > + // Create Root Bridge Handle Instance > + // > + RootBridge = CreateRootBridge (&RootBridges[Index], HostBridge->Handle); > + ASSERT (RootBridge != NULL); > + if (RootBridge == NULL) { > + continue; > + } > + > + if (RootBridges[Index].Io.Limit > RootBridges[Index].Io.Base) { > + Status = gDS->AddIoSpace ( > + EfiGcdIoTypeIo, > + RootBridges[Index].Io.Base, > + RootBridges[Index].Io.Limit - > RootBridges[Index].Io.Base + 1 > + ); > + ASSERT_EFI_ERROR (Status); > + } This assertion fails here with with "Access Denied". The reason is that we add IO and MMIO space still in PEI, as resource descriptor HOBs, and then the DXE core primes the GCD memory space map from them. Adding the same IO space here with the appropriate DXE service runs into a conflict justifiedly, and it is rejected. My question is if we could make the GCD memory space map manipulation optional in this driver: The PCI_ROOT_BRIDGE structure could be extended with a UINT32 flag. For each of Io, Mem, MemAbove4G, PMem, PMemAbove4G, the PciHostBridgeLib instance producing the PCI_ROOT_BRIDGE structure could set a separate bit, indicating whether PciHostBridgeDxe should add (and set the attributes of) the given aperture to the GCD memory space map. Since this is a generic driver, I think it should accommodate the case when the relevant apertures are described during PEI. (Not just because that's what OVMF does, but because said practice is valid, as far as I understand / recall the PI specs.) In my (limited) experience it varies between PCI host bridge drivers whether they install the apertures themselves, or rely on them already being present. In ArmVirtPkg/PciHostBridgeDxe/, the driver installs the (emulated) IO and (real) MMIO space itself; while in OvmfPkg (and the original PcAtChipsetPkg driver), this does not happen. If you agree with the idea, I can try writing a patch for it. Alternatively, you could require from the PciHostBridgeGetRootBridges() API that on return, all the necessary GCD memory space map entries be in place. At the moment, both approaches would work for me. ... Hm, I was too curious, and wrote the patch just now. OVMF (ported to PciHostBridgeDxe) works well with it; I retested the multiple root bridges case too, and compared the OVMF debug log & the Linux guest dmesg before<->after. The resource assignments are the same. I also regression tested a Windows 8.1 guest, with 6GB RAM. So, what do you think of the attached patch? If you are okay with it, I can submit it as part of my upcoming OVMF series. Thanks! Laszlo [...]
From 939c8cf13bce4c66a5dde158343f6cda91b1378d Mon Sep 17 00:00:00 2001 From: Laszlo Ersek <[email protected]> Date: Tue, 26 Jan 2016 02:46:28 +0100 Subject: [PATCH] MdeModulePkg: PciHostBridgeDxe: allow PciHostBridgeLib to prime the GCD map Introduce the PCI_ROOT_BRIDGE.Flags member, which is a bitmap of 32 bits. The least significant 5 bits are now assigned so that PciHostBridgeLib can ask PciHostBridgeDxe *not* to add the various root bridge apertures to the GCD IO and memory space map. This is motivated by the case when some other module installs the same (or larger) apertures in the GCD memory space map earlier, and PciHostBridgeDxe's attempt to install overlapping entries would fail. For example, the DXE core primes the GCD map from the IO and MMIO resource descriptor HOBs that the HOB procuder (in practice: PEI) phase creates. Cc: Ruiyu Ni <[email protected] Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <[email protected]> --- MdeModulePkg/Include/Library/PciHostBridgeLib.h | 13 +++++++++++++ MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 9 +++++++-- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/MdeModulePkg/Include/Library/PciHostBridgeLib.h b/MdeModulePkg/Include/Library/PciHostBridgeLib.h index b1dba0f..e35e0b1 100644 --- a/MdeModulePkg/Include/Library/PciHostBridgeLib.h +++ b/MdeModulePkg/Include/Library/PciHostBridgeLib.h @@ -24,6 +24,18 @@ typedef struct { UINT64 Limit; } PCI_ROOT_BRIDGE_APERTURE; +// +// Bits for the PCI_ROOT_BRIDGE.Flags field. When a given bit is clear, the +// host bridge driver will attempt to add the corresponding aperture to the GCD +// memory space map, and set its attributes, using DXE services. When a bit is +// set, the aperture is assumed to be present in the GCD memory space map. +// +#define PCI_ROOT_BRIDGE_APERTURE_PRESENT_IO BIT0 +#define PCI_ROOT_BRIDGE_APERTURE_PRESENT_MEM32 BIT1 +#define PCI_ROOT_BRIDGE_APERTURE_PRESENT_MEM64 BIT2 +#define PCI_ROOT_BRIDGE_APERTURE_PRESENT_PMEM32 BIT3 +#define PCI_ROOT_BRIDGE_APERTURE_PRESENT_PMEM64 BIT4 + typedef struct { UINT32 Segment; ///< Segment number. UINT64 Supports; ///< Supported attributes. @@ -44,6 +56,7 @@ typedef struct { PCI_ROOT_BRIDGE_APERTURE MemAbove4G; ///< MMIO aperture above 4GB which can be used by the root bridge. PCI_ROOT_BRIDGE_APERTURE PMem; ///< Prefetchable MMIO aperture below 4GB which can be used by the root bridge. PCI_ROOT_BRIDGE_APERTURE PMemAbove4G; ///< Prefetchable MMIO aperture above 4GB which can be used by the root bridge. + UINT32 Flags; ///< See PCI_ROOT_BRIDGE_APERTURE_PRESENT_xxx above. EFI_DEVICE_PATH_PROTOCOL *DevicePath; ///< Device path. } PCI_ROOT_BRIDGE; diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c index 08285d8..9c810e5 100644 --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c @@ -54,6 +54,7 @@ InitializePciHostBridge ( UINTN Index; PCI_ROOT_BRIDGE_APERTURE *MemApertures[4]; UINTN MemApertureIndex; + UINT32 AperturePresentMask; RootBridges = PciHostBridgeGetRootBridges (&RootBridgeCount); if ((RootBridges == NULL) || (RootBridgeCount == 0)) { @@ -108,7 +109,9 @@ InitializePciHostBridge ( continue; } - if (RootBridges[Index].Io.Limit > RootBridges[Index].Io.Base) { + AperturePresentMask = PCI_ROOT_BRIDGE_APERTURE_PRESENT_IO; + if (RootBridges[Index].Io.Limit > RootBridges[Index].Io.Base && + (RootBridges[Index].Flags & AperturePresentMask) == 0) { Status = gDS->AddIoSpace ( EfiGcdIoTypeIo, RootBridges[Index].Io.Base, @@ -129,7 +132,9 @@ InitializePciHostBridge ( MemApertures[3] = &RootBridges[Index].PMemAbove4G; for (MemApertureIndex = 0; MemApertureIndex < sizeof (MemApertures) / sizeof (MemApertures[0]); MemApertureIndex++) { - if (MemApertures[MemApertureIndex]->Limit > MemApertures[MemApertureIndex]->Base) { + AperturePresentMask <<= 1; + if (MemApertures[MemApertureIndex]->Limit > MemApertures[MemApertureIndex]->Base && + (RootBridges[Index].Flags & AperturePresentMask) == 0) { Status = gDS->AddMemorySpace ( EfiGcdMemoryTypeMemoryMappedIo, MemApertures[MemApertureIndex]->Base, -- 1.8.3.1
_______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
