Laszlo, I now understand your problem. Can you tell me why OVMF needs multiple root bridges support? My understanding to OVMF is it's a firmware which can be used in a guest VM environment to boot OS. Multiple root bridges requirement currently mainly comes from high-end servers. Do you mean that the VM guest needs to be like a high-end server? This may help me to think about the possible solution to your problem.
Regards, Ray -----Original Message----- From: Laszlo Ersek [mailto:[email protected]] Sent: Tuesday, January 26, 2016 11:10 AM To: Ni, Ruiyu <[email protected]> Cc: [email protected]; Tian, Feng <[email protected]>; Fan, Jeff <[email protected]>; Justen, Jordan L <[email protected]>; Marcel Apfelbaum <[email protected]> Subject: Re: [edk2] [Patch V4 4/4] MdeModulePkg: Add generic PciHostBridgeDxe driver. On 01/26/16 03:42, Ni, Ruiyu wrote: > Laszlo, > Thanks for the detailed explanation and I agrees with your idea that a generic > driver should consider the case that part of the IO/MMIO resource may be > already added by platform, no matter by PEI or by DXE. > > So I think we may not need the flag you proposed here. To achieve the maximum > flexibility, we can use gDS.GetMemorySpaceMap/GetIoSpaceMap to retrieve all > the current MMIO/IO space map and only add those ranges that are not added. > > And to be more robust, we can check that the range specified to the root > bridge > should not be allocated by anyone (CAN be added by someone). > > What's your opinion? Very good, but painful questions. :) Until now, OVMF has relied upon PlatformPei producing the HOBs in question (where the base of the 32-bit PCI MMIO window varies in the HOB, based on guest RAM size), *and* OVMF's PCI host bridge driver has never even tried to match the root bridges' 32-bit MMIO apertures to anything at all. All of those apertures are set as [2GB, 4GB). The idea being, the gDS->AllocateMemorySpace() calls can be satisfied from whatever room is left from the range that the HOB identified. Especially in the case of multiple root bridges, I wouldn't know *how* to divide up the MMIO address space between them. (Dividing up the bus range was hard enough.) Same for IO ports -- all root bridges share the same [0xC000, 0xFFFF] port range. Therefore your idea above would immediately break OVMF, functionally :) (I suspect that OVMF is kinda broken already, in this aspect, but it happens to work.) I know precious little about PCI resource allocation. This is one of the areas we intend to look into seriously in the future, with Marcel (CC'd). Thus, if porting OVMF to the generic PciHostBridgeDxe driver requires incompatible changes at once, then I think we should postpone the port until Marcel returns from his vacation. To reiterate, my current problem is ultimately the lack of an algorithm for dynamically dividing up the IO and MMIO space between root bridges. Do you have an idea for that? (The algorithm, if there is one, could very well be specific to QEMU -- which too is where we'll need Marcel's input.) Thanks! Laszlo > > > Regards, > Ray > > > -----Original Message----- > From: Laszlo Ersek [mailto:[email protected]] > Sent: Tuesday, January 26, 2016 10:20 AM > To: Ni, Ruiyu <[email protected]> > Cc: [email protected]; Tian, Feng <[email protected]>; Fan, Jeff > <[email protected]>; Justen, Jordan L <[email protected]> > Subject: Re: [edk2] [Patch V4 4/4] MdeModulePkg: Add generic PciHostBridgeDxe > driver. > > 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 > > [...] > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
