Will you start to change the OVMF platform to use the core PciHostBridge next week? I am still not quite sure it's the only gap:)
Regards, Ray >-----Original Message----- >From: Laszlo Ersek [mailto:[email protected]] >Sent: Friday, February 26, 2016 8:34 PM >To: Ni, Ruiyu <[email protected]>; [email protected] >Subject: Re: [PATCH v3] MdeModulePkg/PciHostBridge: Don't assume resources are >fully NonExistent > >On 02/26/16 13:15, Ni, Ruiyu wrote: >> Thanks for the quick patch. >> >> Reviewed-by: Ruiyu Ni <[email protected]> > >Thanks a lot! > >Committed & pushed as 6474f1f156eefa6886e594039f4d562e374ba344. > >Laszlo > >> >> >>> -----Original Message----- >>> From: Laszlo Ersek [mailto:[email protected]] >>> Sent: Friday, February 26, 2016 7:16 PM >>> To: [email protected] >>> Cc: Ni, Ruiyu <[email protected]> >>> Subject: [PATCH v3] MdeModulePkg/PciHostBridge: Don't assume resources are >>> fully NonExistent >>> >>> From: Ruiyu Ni <[email protected]> >>> >>> The patch removes the assumption that the resources claimed by root >>> bridges should not exist. Because resources might have been added: >>> 1. by platform modules either in PEI through resource HOB, or in DXE, >>> before the PCI host bridge driver runs. >>> 2. Resources claimed by different root bridges may overlap so that >>> resource adding operation for latter root bridges may fail if >>> we assume the resource should not exist. >>> >>> In real world, this patch is to fit OVMF platform needs because >>> different root bridges in OVMF platform shares the same resources. >>> >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Ruiyu Ni <[email protected]> >>> [[email protected]: intersection-based implementation] >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Laszlo Ersek <[email protected]> >>> Cc: Ruiyu Ni <[email protected]> >>> --- >>> >>> Notes: >>> v3: >>> - replace "completely missing" with "fully NonExistent" in the subject >>> [Ray] >>> >>> - sorting is only necessary for gap checking, but there are no gaps -- >>> drop both gap checking and sorting [Ray] >>> >>> - comment in the code that (IntersectionBase >= IntersectionEnd) means >>> "no overlap" [Ray] >>> >>> - log errors when incompatible descriptors are encountered >>> >>> - log an error when GetIoSpaceMap() / GetMemorySpaceMap() fails >>> >>> - remove useless ASSERT()s on the descriptors returned by >>> GetIoSpaceDescriptor() and GetMemorySpaceDescriptor() [Ray] >>> >>> - eliminate the Remaining, DescriptorLeft and Step variables, >>> simplifying the verification loops [Ray] >>> >>> - the verification loops should assert on CheckStatus, not Status >>> >>> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 305 >>> +++++++++++++++++++- >>> 1 file changed, 294 insertions(+), 11 deletions(-) >>> >>> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c >>> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c >>> index edf042cc2547..ef0c4f2a5e7b 100644 >>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c >>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c >>> @@ -29,6 +29,290 @@ GLOBAL_REMOVE_IF_UNREFERENCED CHAR16 >>> *mPciResourceTypeStr[] = { >>> }; >>> >>> /** >>> + Ensure the compatibility of an IO space descriptor with the IO aperture. >>> + >>> + The IO space descriptor can come from the GCD IO space map, or it can >>> + represent a gap between two neighboring IO space descriptors. In the >>> latter >>> + case, the GcdIoType field is expected to be EfiGcdIoTypeNonExistent. >>> + >>> + If the IO space descriptor already has type EfiGcdIoTypeIo, then no >>> action is >>> + taken -- it is by definition compatible with the aperture. >>> + >>> + Otherwise, the intersection of the IO space descriptor is calculated >>> with the >>> + aperture. If the intersection is the empty set (no overlap), no action is >>> + taken; the IO space descriptor is compatible with the aperture. >>> + >>> + Otherwise, the type of the descriptor is investigated again. If the type >>> is >>> + EfiGcdIoTypeNonExistent (representing a gap, or a genuine descriptor with >>> + such a type), then an attempt is made to add the intersection as IO >>> space to >>> + the GCD IO space map. This ensures continuity for the aperture, and the >>> + descriptor is deemed compatible with the aperture. >>> + >>> + Otherwise, the IO space descriptor is incompatible with the IO aperture. >>> + >>> + @param[in] Base Base address of the aperture. >>> + @param[in] Length Length of the aperture. >>> + @param[in] Descriptor The descriptor to ensure compatibility with the >>> + aperture for. >>> + >>> + @retval EFI_SUCCESS The descriptor is compatible. The GCD IO >>> space >>> + map may have been updated, for continuity >>> + within the aperture. >>> + @retval EFI_INVALID_PARAMETER The descriptor is incompatible. >>> + @return Error codes from gDS->AddIoSpace(). >>> +**/ >>> +EFI_STATUS >>> +IntersectIoDescriptor ( >>> + IN UINT64 Base, >>> + IN UINT64 Length, >>> + IN CONST EFI_GCD_IO_SPACE_DESCRIPTOR *Descriptor >>> + ) >>> +{ >>> + UINT64 IntersectionBase; >>> + UINT64 IntersectionEnd; >>> + EFI_STATUS Status; >>> + >>> + if (Descriptor->GcdIoType == EfiGcdIoTypeIo) { >>> + return EFI_SUCCESS; >>> + } >>> + >>> + IntersectionBase = MAX (Base, Descriptor->BaseAddress); >>> + IntersectionEnd = MIN (Base + Length, >>> + Descriptor->BaseAddress + Descriptor->Length); >>> + if (IntersectionBase >= IntersectionEnd) { >>> + // >>> + // The descriptor and the aperture don't overlap. >>> + // >>> + return EFI_SUCCESS; >>> + } >>> + >>> + if (Descriptor->GcdIoType == EfiGcdIoTypeNonExistent) { >>> + Status = gDS->AddIoSpace (EfiGcdIoTypeIo, IntersectionBase, >>> + IntersectionEnd - IntersectionBase); >>> + >>> + DEBUG ((EFI_ERROR (Status) ? EFI_D_ERROR : EFI_D_VERBOSE, >>> + "%a: %a: add [%Lx, %Lx): %r\n", gEfiCallerBaseName, __FUNCTION__, >>> + IntersectionBase, IntersectionEnd, Status)); >>> + return Status; >>> + } >>> + >>> + DEBUG ((EFI_D_ERROR, "%a: %a: desc [%Lx, %Lx) type %u conflicts with " >>> + "aperture [%Lx, %Lx)\n", gEfiCallerBaseName, __FUNCTION__, >>> + Descriptor->BaseAddress, Descriptor->BaseAddress + Descriptor->Length, >>> + (UINT32)Descriptor->GcdIoType, Base, Base + Length)); >>> + return EFI_INVALID_PARAMETER; >>> +} >>> + >>> +/** >>> + Add IO space to GCD. >>> + The routine checks the GCD database and only adds those which are >>> + not added in the specified range to GCD. >>> + >>> + @param Base Base address of the IO space. >>> + @param Length Length of the IO space. >>> + >>> + @retval EFI_SUCCES The IO space was added successfully. >>> +**/ >>> +EFI_STATUS >>> +AddIoSpace ( >>> + IN UINT64 Base, >>> + IN UINT64 Length >>> + ) >>> +{ >>> + EFI_STATUS Status; >>> + UINTN Index; >>> + UINTN NumberOfDescriptors; >>> + EFI_GCD_IO_SPACE_DESCRIPTOR *IoSpaceMap; >>> + >>> + Status = gDS->GetIoSpaceMap (&NumberOfDescriptors, &IoSpaceMap); >>> + if (EFI_ERROR (Status)) { >>> + DEBUG ((EFI_D_ERROR, "%a: %a: GetIoSpaceMap(): %r\n", >>> + gEfiCallerBaseName, __FUNCTION__, Status)); >>> + return Status; >>> + } >>> + >>> + for (Index = 0; Index < NumberOfDescriptors; Index++) { >>> + Status = IntersectIoDescriptor (Base, Length, &IoSpaceMap[Index]); >>> + if (EFI_ERROR (Status)) { >>> + goto FreeIoSpaceMap; >>> + } >>> + } >>> + >>> + DEBUG_CODE ( >>> + // >>> + // Make sure there are adjacent descriptors covering [Base, Base + >>> Length). >>> + // It is possible that they have not been merged; merging can be >>> prevented >>> + // by allocation. >>> + // >>> + UINT64 CheckBase; >>> + EFI_STATUS CheckStatus; >>> + EFI_GCD_IO_SPACE_DESCRIPTOR Descriptor; >>> + >>> + for (CheckBase = Base; >>> + CheckBase < Base + Length; >>> + CheckBase = Descriptor.BaseAddress + Descriptor.Length) { >>> + CheckStatus = gDS->GetIoSpaceDescriptor (CheckBase, &Descriptor); >>> + ASSERT_EFI_ERROR (CheckStatus); >>> + ASSERT (Descriptor.GcdIoType == EfiGcdIoTypeIo); >>> + } >>> + ); >>> + >>> +FreeIoSpaceMap: >>> + FreePool (IoSpaceMap); >>> + >>> + return Status; >>> +} >>> + >>> +/** >>> + Ensure the compatibility of a memory space descriptor with the MMIO >>> aperture. >>> + >>> + The memory space descriptor can come from the GCD memory space map, or >>> it can >>> + represent a gap between two neighboring memory space descriptors. In the >>> + latter case, the GcdMemoryType field is expected to be >>> + EfiGcdMemoryTypeNonExistent. >>> + >>> + If the memory space descriptor already has type >>> + EfiGcdMemoryTypeMemoryMappedIo, and its capabilities are a superset of >>> the >>> + required capabilities, then no action is taken -- it is by definition >>> + compatible with the aperture. >>> + >>> + Otherwise, the intersection of the memory space descriptor is calculated >>> with >>> + the aperture. If the intersection is the empty set (no overlap), no >>> action is >>> + taken; the memory space descriptor is compatible with the aperture. >>> + >>> + Otherwise, the type of the descriptor is investigated again. If the type >>> is >>> + EfiGcdMemoryTypeNonExistent (representing a gap, or a genuine descriptor >>> with >>> + such a type), then an attempt is made to add the intersection as MMIO >>> space >>> + to the GCD memory space map, with the specified capabilities. This >>> ensures >>> + continuity for the aperture, and the descriptor is deemed compatible >>> with the >>> + aperture. >>> + >>> + Otherwise, the memory space descriptor is incompatible with the MMIO >>> + aperture. >>> + >>> + @param[in] Base Base address of the aperture. >>> + @param[in] Length Length of the aperture. >>> + @param[in] Capabilities Capabilities required by the aperture. >>> + @param[in] Descriptor The descriptor to ensure compatibility with the >>> + aperture for. >>> + >>> + @retval EFI_SUCCESS The descriptor is compatible. The GCD >>> memory >>> + space map may have been updated, for >>> + continuity within the aperture. >>> + @retval EFI_INVALID_PARAMETER The descriptor is incompatible. >>> + @return Error codes from gDS->AddMemorySpace(). >>> +**/ >>> +EFI_STATUS >>> +IntersectMemoryDescriptor ( >>> + IN UINT64 Base, >>> + IN UINT64 Length, >>> + IN UINT64 Capabilities, >>> + IN CONST EFI_GCD_MEMORY_SPACE_DESCRIPTOR *Descriptor >>> + ) >>> +{ >>> + UINT64 IntersectionBase; >>> + UINT64 IntersectionEnd; >>> + EFI_STATUS Status; >>> + >>> + if (Descriptor->GcdMemoryType == EfiGcdMemoryTypeMemoryMappedIo && >>> + (Descriptor->Capabilities & Capabilities) == Capabilities) { >>> + return EFI_SUCCESS; >>> + } >>> + >>> + IntersectionBase = MAX (Base, Descriptor->BaseAddress); >>> + IntersectionEnd = MIN (Base + Length, >>> + Descriptor->BaseAddress + Descriptor->Length); >>> + if (IntersectionBase >= IntersectionEnd) { >>> + // >>> + // The descriptor and the aperture don't overlap. >>> + // >>> + return EFI_SUCCESS; >>> + } >>> + >>> + if (Descriptor->GcdMemoryType == EfiGcdMemoryTypeNonExistent) { >>> + Status = gDS->AddMemorySpace (EfiGcdMemoryTypeMemoryMappedIo, >>> + IntersectionBase, IntersectionEnd - IntersectionBase, >>> + Capabilities); >>> + >>> + DEBUG ((EFI_ERROR (Status) ? EFI_D_ERROR : EFI_D_VERBOSE, >>> + "%a: %a: add [%Lx, %Lx): %r\n", gEfiCallerBaseName, __FUNCTION__, >>> + IntersectionBase, IntersectionEnd, Status)); >>> + return Status; >>> + } >>> + >>> + DEBUG ((EFI_D_ERROR, "%a: %a: desc [%Lx, %Lx) type %u cap %Lx conflicts " >>> + "with aperture [%Lx, %Lx) cap %Lx\n", gEfiCallerBaseName, __FUNCTION__, >>> + Descriptor->BaseAddress, Descriptor->BaseAddress + Descriptor->Length, >>> + (UINT32)Descriptor->GcdMemoryType, Descriptor->Capabilities, >>> + Base, Base + Length, Capabilities)); >>> + return EFI_INVALID_PARAMETER; >>> +} >>> + >>> +/** >>> + Add MMIO space to GCD. >>> + The routine checks the GCD database and only adds those which are >>> + not added in the specified range to GCD. >>> + >>> + @param Base Base address of the MMIO space. >>> + @param Length Length of the MMIO space. >>> + @param Capabilities Capabilities of the MMIO space. >>> + >>> + @retval EFI_SUCCES The MMIO space was added successfully. >>> +**/ >>> +EFI_STATUS >>> +AddMemoryMappedIoSpace ( >>> + IN UINT64 Base, >>> + IN UINT64 Length, >>> + IN UINT64 Capabilities >>> + ) >>> +{ >>> + EFI_STATUS Status; >>> + UINTN Index; >>> + UINTN NumberOfDescriptors; >>> + EFI_GCD_MEMORY_SPACE_DESCRIPTOR *MemorySpaceMap; >>> + >>> + Status = gDS->GetMemorySpaceMap (&NumberOfDescriptors, &MemorySpaceMap); >>> + if (EFI_ERROR (Status)) { >>> + DEBUG ((EFI_D_ERROR, "%a: %a: GetMemorySpaceMap(): %r\n", >>> + gEfiCallerBaseName, __FUNCTION__, Status)); >>> + return Status; >>> + } >>> + >>> + for (Index = 0; Index < NumberOfDescriptors; Index++) { >>> + Status = IntersectMemoryDescriptor (Base, Length, Capabilities, >>> + &MemorySpaceMap[Index]); >>> + if (EFI_ERROR (Status)) { >>> + goto FreeMemorySpaceMap; >>> + } >>> + } >>> + >>> + DEBUG_CODE ( >>> + // >>> + // Make sure there are adjacent descriptors covering [Base, Base + >>> Length). >>> + // It is possible that they have not been merged; merging can be >>> prevented >>> + // by allocation and different capabilities. >>> + // >>> + UINT64 CheckBase; >>> + EFI_STATUS CheckStatus; >>> + EFI_GCD_MEMORY_SPACE_DESCRIPTOR Descriptor; >>> + >>> + for (CheckBase = Base; >>> + CheckBase < Base + Length; >>> + CheckBase = Descriptor.BaseAddress + Descriptor.Length) { >>> + CheckStatus = gDS->GetMemorySpaceDescriptor (CheckBase, &Descriptor); >>> + ASSERT_EFI_ERROR (CheckStatus); >>> + ASSERT (Descriptor.GcdMemoryType == EfiGcdMemoryTypeMemoryMappedIo); >>> + ASSERT ((Descriptor.Capabilities & Capabilities) == Capabilities); >>> + } >>> + ); >>> + >>> +FreeMemorySpaceMap: >>> + FreePool (MemorySpaceMap); >>> + >>> + return Status; >>> +} >>> + >>> +/** >>> >>> Entry point of this driver. >>> >>> @@ -89,6 +373,7 @@ InitializePciHostBridge ( >>> &gEfiPciHostBridgeResourceAllocationProtocolGuid, >>> &HostBridge->ResAlloc, >>> NULL >>> ); >>> + ASSERT_EFI_ERROR (Status); >>> if (EFI_ERROR (Status)) { >>> FreePool (HostBridge); >>> PciHostBridgeFreeRootBridges (RootBridges, RootBridgeCount); >>> @@ -109,11 +394,10 @@ InitializePciHostBridge ( >>> } >>> >>> 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 >>> - ); >>> + Status = AddIoSpace ( >>> + RootBridges[Index].Io.Base, >>> + RootBridges[Index].Io.Limit - RootBridges[Index].Io.Base >>> + 1 >>> + ); >>> ASSERT_EFI_ERROR (Status); >>> } >>> >>> @@ -130,12 +414,11 @@ InitializePciHostBridge ( >>> >>> for (MemApertureIndex = 0; MemApertureIndex < sizeof (MemApertures) / >>> sizeof (MemApertures[0]); >>> MemApertureIndex++) { >>> if (MemApertures[MemApertureIndex]->Limit > >>> MemApertures[MemApertureIndex]->Base) { >>> - Status = gDS->AddMemorySpace ( >>> - EfiGcdMemoryTypeMemoryMappedIo, >>> - MemApertures[MemApertureIndex]->Base, >>> - MemApertures[MemApertureIndex]->Limit - >>> MemApertures[MemApertureIndex]->Base + >1, >>> - EFI_MEMORY_UC >>> - ); >>> + Status = AddMemoryMappedIoSpace ( >>> + MemApertures[MemApertureIndex]->Base, >>> + MemApertures[MemApertureIndex]->Limit - >>> MemApertures[MemApertureIndex]->Base + 1, >>> + EFI_MEMORY_UC >>> + ); >>> ASSERT_EFI_ERROR (Status); >>> Status = gDS->SetMemorySpaceAttributes ( >>> MemApertures[MemApertureIndex]->Base, >>> -- >>> 1.8.3.1 >> _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
