Laszlo, Given the fact that every byte is covered by a descriptor in GCD, the sorting can be avoided.
I added detailed comments (11 in total) in below. Please check. I am learning how to write mail in the style you are using. It's a totally different style:) like talking. Regards, Ray >-----Original Message----- >From: Laszlo Ersek [mailto:ler...@redhat.com] >Sent: Friday, February 26, 2016 8:26 AM >To: edk2-devel-01 <edk2-de...@ml01.01.org> >Cc: Ni, Ruiyu <ruiyu...@intel.com> >Subject: [RFC PATCH v2] MdeModulePkg/PciHostBridge: Don't assume resources are >completely missing > >From: Ruiyu Ni <ruiyu...@intel.com> > >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 <ruiyu...@intel.com> >[ler...@redhat.com: intersection-based implementation] >Contributed-under: TianoCore Contribution Agreement 1.0 >Signed-off-by: Laszlo Ersek <ler...@redhat.com> >Cc: Ruiyu Ni <ruiyu...@intel.com> >--- > >Notes: > Untested (only build tested). I will have to complete the rebase the > OVMF PCI host bridge / root bridge driver on top of PciHostBridgeDxe to > test this patch. > > But, I think it's worth posting; we can discuss it, and Ray could even > test it sooner. Thanks! > > MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf | 1 + > MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.h | 1 + > MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 428 > +++++++++++++++++++- > 3 files changed, 419 insertions(+), 11 deletions(-) > >diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf >b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf >index ab5d87e1cf0a..a11f5a4b01fa 100644 >--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf >+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf >@@ -42,6 +42,7 @@ [LibraryClasses] > BaseLib > PciSegmentLib > PciHostBridgeLib >+ SortLib > > [Protocols] > gEfiMetronomeArchProtocolGuid ## CONSUMES >diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.h >b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.h >index 9a8ca21f4819..6a9bb0a2ee1c 100644 >--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.h >+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.h >@@ -22,6 +22,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER >EXPRESS OR IMPLIED. > #include <Library/UefiDriverEntryPoint.h> > #include <Library/MemoryAllocationLib.h> > #include <Library/PciHostBridgeLib.h> >+#include <Library/SortLib.h> > #include <Protocol/PciHostBridgeResourceAllocation.h> > > #include "PciRootBridge.h" >diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c >b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c >index edf042cc2547..6fe41eac25a8 100644 >--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c >+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c >@@ -29,6 +29,413 @@ GLOBAL_REMOVE_IF_UNREFERENCED CHAR16 >*mPciResourceTypeStr[] = { > }; > > /** >+ Comparator for memory or IO space descriptor. >+ Because the comparator only compares BaseAddress and both >+ EFI_GCD_MEMORY_SPACE_DESCRIPTOR and EFI_GCD_IO_SPACE_DESCRIPTOR >+ put the BaseAddress in the same offset (0), this comparator can be >+ used by both structures. >+ >+ @param Left Pointer to the memory or IO space descriptor. >+ @param Right Pointer to the memory or IO space descriptor. >+ >+ @retval 0 Left equal to Right. >+ @retval -1 Left is less than Right. >+ @retval 1 Left is greater than Right. >+**/ >+INTN >+EFIAPI >+DescriptorComparator ( >+ IN CONST VOID *Left, >+ IN CONST VOID *Right >+ ) >+{ >+ CONST EFI_PHYSICAL_ADDRESS *LeftBaseAddress; >+ CONST EFI_PHYSICAL_ADDRESS *RightBaseAddress; >+ >+ LeftBaseAddress = Left; >+ RightBaseAddress = Right; >+ >+ if (*LeftBaseAddress < *RightBaseAddress) { >+ return -1; >+ } else if (*LeftBaseAddress == *RightBaseAddress) { >+ return 0; >+ } else { >+ return 1; >+ } >+} >+ >+/** >+ 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); 1). Better to add comments here to say the two ranges doesn't overlap. >+ if (IntersectionBase >= IntersectionEnd) { >+ return EFI_SUCCESS; >+ } >+ 2). Can just use ASSERT (Descriptor->GcdIoType == EfiGcdIoTypeNonExistent). because failing to add resource is a fatal error. >+ 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)); 3). Good to know gEfiCallerBaseName:) >+ return Status; >+ } >+ >+ 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; >+ CONST EFI_GCD_IO_SPACE_DESCRIPTOR *Descriptor; >+ UINT64 PriorDescriptorEnd; >+ EFI_GCD_IO_SPACE_DESCRIPTOR Gap; >+ >+ Status = gDS->GetIoSpaceMap (&NumberOfDescriptors, &IoSpaceMap); >+ if (EFI_ERROR (Status)) { >+ return Status; >+ } 4). ASSERT_EFI_ERROR(Status) is enough. >+ >+ PerformQuickSort (IoSpaceMap, NumberOfDescriptors, sizeof >(EFI_GCD_IO_SPACE_DESCRIPTOR), >DescriptorComparator); >+ 5). No sort is needed. >+ PriorDescriptorEnd = 0; >+ >+ Gap.GcdIoType = EfiGcdIoTypeNonExistent; >+ Gap.ImageHandle = NULL; >+ Gap.DeviceHandle = NULL; >+ 6). So no need to use PriorDescriptorEnd and Gap as well. >+ for (Index = 0; Index < NumberOfDescriptors; Index++) { >+ Descriptor = &IoSpaceMap[Index]; >+ >+ // >+ // If there is a gap between the prior descriptor and the current one, >+ // check it. Note that this covers the case when the first descriptor >+ // begins above the start of the aperture. >+ // >+ if (PriorDescriptorEnd < Descriptor->BaseAddress) { >+ Gap.BaseAddress = PriorDescriptorEnd; >+ Gap.Length = Descriptor->BaseAddress - PriorDescriptorEnd; >+ >+ Status = IntersectIoDescriptor (Base, Length, &Gap); >+ if (EFI_ERROR (Status)) { >+ goto FreeIoSpaceMap; >+ } >+ } >+ 7). the above logic is not needed as well. >+ Status = IntersectIoDescriptor (Base, Length, Descriptor); >+ if (EFI_ERROR (Status)) { >+ goto FreeIoSpaceMap; >+ } 8). ASSERT is enough. >+ PriorDescriptorEnd = Descriptor->BaseAddress + Descriptor->Length; >+ } >+ >+ // >+ // Add the gap after the last descriptor. >+ // >+ Gap.BaseAddress = PriorDescriptorEnd; >+ Gap.Length = MAX_UINT64 - PriorDescriptorEnd; >+ Status = IntersectIoDescriptor (Base, Length, &Gap); >+ 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; >+ UINT64 Remaining; >+ EFI_STATUS CheckStatus; >+ EFI_GCD_IO_SPACE_DESCRIPTOR Descriptor; >+ UINT64 DescriptorLeft; >+ UINT64 Step; >+ >+ CheckBase = Base; >+ Remaining = Length; >+ while (Remaining > 0) { >+ CheckStatus = gDS->GetIoSpaceDescriptor (CheckBase, &Descriptor); >+ ASSERT_EFI_ERROR (Status); >+ >+ ASSERT (Descriptor.BaseAddress <= CheckBase); >+ ASSERT (CheckBase < Descriptor.BaseAddress + Descriptor.Length); 9). above two assertion is not necessary. GetIoSpaceDescriptor() API follows spec. >+ DescriptorLeft = Descriptor.Length - >+ (CheckBase - Descriptor.BaseAddress); >+ >+ ASSERT (Descriptor.GcdIoType == EfiGcdIoTypeIo); >+ >+ Step = MIN (DescriptorLeft, Remaining); >+ CheckBase += Step; >+ Remaining -= Step; 10). We can just let CheckBase = Descriptor.BaseAddress + Descriptor.Length since there is no gap in GCD. So the while(Remaining > 0) can be while (CheckBase < Base + Length). >+ } >+ ); >+ >+FreeIoSpaceMap: >+ FreePool (IoSpaceMap); >+ >+ return Status; >+} >+ 11). Similar comments for the MMIO operation in below. >+/** >+ 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) { >+ 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; >+ } >+ >+ 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; >+ CONST EFI_GCD_MEMORY_SPACE_DESCRIPTOR *Descriptor; >+ UINT64 PriorDescriptorEnd; >+ EFI_GCD_MEMORY_SPACE_DESCRIPTOR Gap; >+ >+ Status = gDS->GetMemorySpaceMap (&NumberOfDescriptors, &MemorySpaceMap); >+ if (EFI_ERROR (Status)) { >+ return Status; >+ } >+ >+ PerformQuickSort (MemorySpaceMap, NumberOfDescriptors, sizeof >(EFI_GCD_MEMORY_SPACE_DESCRIPTOR), >DescriptorComparator); >+ >+ PriorDescriptorEnd = 0; >+ >+ Gap.Capabilities = 0; >+ Gap.Attributes = 0; >+ Gap.GcdMemoryType = EfiGcdMemoryTypeNonExistent; >+ Gap.ImageHandle = NULL; >+ Gap.DeviceHandle = NULL; >+ >+ for (Index = 0; Index < NumberOfDescriptors; Index++) { >+ Descriptor = &MemorySpaceMap[Index]; >+ >+ // >+ // If there is a gap between the prior descriptor and the current one, >+ // check it. Note that this covers the case when the first descriptor >+ // begins above the start of the aperture. >+ // >+ if (PriorDescriptorEnd < Descriptor->BaseAddress) { >+ Gap.BaseAddress = PriorDescriptorEnd; >+ Gap.Length = Descriptor->BaseAddress - PriorDescriptorEnd; >+ >+ Status = IntersectMemoryDescriptor (Base, Length, Capabilities, &Gap); >+ if (EFI_ERROR (Status)) { >+ goto FreeMemorySpaceMap; >+ } >+ } >+ >+ Status = IntersectMemoryDescriptor (Base, Length, Capabilities, >+ Descriptor); >+ if (EFI_ERROR (Status)) { >+ goto FreeMemorySpaceMap; >+ } >+ PriorDescriptorEnd = Descriptor->BaseAddress + Descriptor->Length; >+ } >+ >+ // >+ // Add the gap after the last descriptor. >+ // >+ Gap.BaseAddress = PriorDescriptorEnd; >+ Gap.Length = MAX_UINT64 - PriorDescriptorEnd; >+ Status = IntersectMemoryDescriptor (Base, Length, Capabilities, &Gap); >+ 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; >+ UINT64 Remaining; >+ EFI_STATUS CheckStatus; >+ EFI_GCD_MEMORY_SPACE_DESCRIPTOR Descriptor; >+ UINT64 DescriptorLeft; >+ UINT64 Step; >+ >+ CheckBase = Base; >+ Remaining = Length; >+ while (Remaining > 0) { >+ CheckStatus = gDS->GetMemorySpaceDescriptor (CheckBase, &Descriptor); >+ ASSERT_EFI_ERROR (Status); >+ >+ ASSERT (Descriptor.BaseAddress <= CheckBase); >+ ASSERT (CheckBase < Descriptor.BaseAddress + Descriptor.Length); >+ DescriptorLeft = Descriptor.Length - >+ (CheckBase - Descriptor.BaseAddress); >+ >+ ASSERT (Descriptor.GcdMemoryType == EfiGcdMemoryTypeMemoryMappedIo); >+ ASSERT ((Descriptor.Capabilities & Capabilities) == Capabilities); >+ >+ Step = MIN (DescriptorLeft, Remaining); >+ CheckBase += Step; >+ Remaining -= Step; >+ } >+ ); >+ >+FreeMemorySpaceMap: >+ FreePool (MemorySpaceMap); >+ >+ return Status; >+} >+ >+/** > > Entry point of this driver. > >@@ -89,6 +496,7 @@ InitializePciHostBridge ( > &gEfiPciHostBridgeResourceAllocationProtocolGuid, > &HostBridge->ResAlloc, > NULL > ); >+ ASSERT_EFI_ERROR (Status); > if (EFI_ERROR (Status)) { > FreePool (HostBridge); > PciHostBridgeFreeRootBridges (RootBridges, RootBridgeCount); >@@ -109,11 +517,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 +537,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 edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
