Regards, Ray
>-----Original Message----- >From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo >Ersek >Sent: Friday, February 26, 2016 1:31 AM >To: Ni, Ruiyu <ruiyu...@intel.com> >Cc: edk2-de...@ml01.01.org >Subject: Re: [edk2] [Patch] MdeModulePkg/PciHostBridge: Don't assume resources >are added already > >Hi Ray, > >thanks a lot for the quick patch. I have comments: > >(1) I think the subject is misleading. It should say: > > Don't assume resources are completely missing Or don't assume resources are completely nonexistent. Background: The initialization of GCD database will create a single range [0, 2^36) with type nonexistent if the CPU address line is maximally 36. > >Because that's what the current code does -- it assumes resources are >missing, and therefore it insist on adding them. > >On 02/25/16 10:13, Ruiyu Ni wrote: >> The patch removes the assumption that the resources claimed by root >> bridges should not exist. Because resources might have been added: > >Yes, this wording is correct. > >> 1. by platform modules either in PEI through resource HOB or earlier >> in DXE. > >(2) "earlier" is misleading here, I think. It seems to imply that "in >DXE" is earlier than "in PEI". > >I understand that you mean "or in DXE, earlier than dispatching the PCI >host bridge driver". Please clarify the wording. Agree. > >> 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. > >Great. I have some code comments as well: > >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ruiyu Ni <ruiyu...@intel.com> >> Cc: Laszlo Ersek <ler...@redhat.com> >> --- >> .../Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 186 >> +++++++++++++++++++-- >> .../Bus/Pci/PciHostBridgeDxe/PciHostBridge.h | 1 + >> .../Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf | 1 + >> 3 files changed, 177 insertions(+), 11 deletions(-) >> >> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c >b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c >> index edf042c..3365b81 100644 >> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c >> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c >> @@ -29,6 +29,171 @@ 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. >> + @return -1 Left is less than Right. >> + @return 1 Left is greater than Right. >> +**/ >> +INTN >> +EFIAPI >> +DescriptorComparator ( >> + IN CONST EFI_GCD_MEMORY_SPACE_DESCRIPTOR *Left, >> + IN CONST EFI_GCD_MEMORY_SPACE_DESCRIPTOR *Right >> + ) >> +{ >> + if (Left->BaseAddress < Right->BaseAddress) { >> + return -1; >> + } else if (Left->BaseAddress == Right->BaseAddress) { >> + return 0; >> + } else { >> + return 1; >> + } >> +} > >(3) The function prototype for comparators seems to be > >typedef >INTN >(EFIAPI *SORT_COMPARE)( > IN CONST VOID *Buffer1, > IN CONST VOID *Buffer2 > ); > >The above doesn't match it on the language level. In addition, I propose >that instead of casting the pointer-to-const-void to >pointer-to-descriptor-structure; we should cast the pointer-to-void to >pointer-to-EFI_PHYSICAL_ADDRESS. This will remain compliant with both >structures even on the language level, not just in practice. > Yes I guess the GCC build may break though VS2015 build pass. >> + >> +/** >> + 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 ( >> + UINT64 Base, >> + UINT64 Length >> + ) >> +{ >> + EFI_STATUS Status; >> + UINTN Index; >> + UINTN NumberOfDescriptors; >> + EFI_GCD_IO_SPACE_DESCRIPTOR *IoSpaceMap; >> + UINT64 DescriptorLimit; >> + UINT64 FreeBase; >> + >> + Status = gDS->GetIoSpaceMap (&NumberOfDescriptors, &IoSpaceMap); >> + ASSERT_EFI_ERROR (Status); >> + >> + PerformQuickSort (IoSpaceMap, NumberOfDescriptors, sizeof >> (EFI_GCD_IO_SPACE_DESCRIPTOR), >DescriptorComparator); >> + >> + for (Index = 0, FreeBase = Base; Index < NumberOfDescriptors && FreeBase >> < Base + Length; Index++) { >> + DescriptorLimit = IoSpaceMap[Index].BaseAddress + >> IoSpaceMap[Index].Length; > >(4) Just a side note: I think we should rather call this DescriptorEnd. >Limit is usually meant as an inclusive bound, but here we have an >exclusive bound. Anyway, this is not too important. > Agree. >> + if (((DescriptorLimit >= FreeBase) && (DescriptorLimit < Base + >> Length)) || >> + ((IoSpaceMap[Index].BaseAddress >= FreeBase) && >> (IoSpaceMap[Index].BaseAddress < Base + Length)) >> + ) { >> + // >> + // For descriptors which overlap with [Base, Base + Length), we need >> to make sure >> + // it's IO type. >> + // Assertion is enough because failed to start PCI Host Bridge is a >> fatal error. >> + // >> + if (IoSpaceMap[Index].GcdIoType != EfiGcdIoTypeNonExistent) { >> + ASSERT (IoSpaceMap[Index].GcdIoType == EfiGcdIoTypeIo); >> + if (IoSpaceMap[Index].BaseAddress > FreeBase) { >> + Status = gDS->AddIoSpace (EfiGcdIoTypeIo, FreeBase, >> IoSpaceMap[Index].BaseAddress - FreeBase); >> + DEBUG ((EFI_D_INFO, "[PciHostBridge]: Add IO Space [%lx, %lx)\n", >> FreeBase, IoSpaceMap[Index].BaseAddress)); >> + ASSERT_EFI_ERROR (Status); >> + } >> + FreeBase = DescriptorLimit; >> + } >> + } >> + } > >After thinking a lot about this function :), I think I understand it. I >can see two problems with it. > >(5) The overlap check itself. It seems to investigate whether the base >of the descriptor falls into the remaining interval, or the end of the >descriptor falls within the remaining interval. > >The relation operators don't seem to be correct: > >- (DescriptorLimit >= FreeBase) is not a good comparison, because >DescriptorLimit is exclusive, while FreeBase is inclusive. So, if they >are equal (satisfying the condition), that means they do *not* overlap. > >- (DescriptorLimit < Base + Length) is also not a good comparison. Both >of these are exclusive limits, so when they are equal (falsifying the >condiiotn), then the overlap *does* exist. Yes >= should be > while < should be <=. > >Etc. > >Instead of fixing the relation operators, I will have a different >proposal, please read on. > >(6) Because, even if we assume that the overlap check is fixed, I see >another problem with the algorithm. Consider the following scenario: > > ................ > IO descriptor #2 > ---------------- > ........... Base+Length > Nonexistent desc > > ................ > IO descriptor #1 > ----------- Base ---------------- > >On the left side, we have the aperture we'd like to cover. On the right >side, we have two IO descriptors that already exist. (And a nonexistent >type descriptor in the middle, optionally.) nonexistent descriptor in the middle is not optionally. Please see the GCD background I mentioned in the very above. > >Assume that IO descriptor #1 starts exactly at Base, and ends somewhere >in the middle. > >Assume that IO descriptor #2 starts strictly above Base+Length. Do you mean IO#2_Base == Base + Length? The overlap check for IO#2 is FALSE. > >In this case, the algorithm will see that IO desc #1 starts exactly at >FreeBase, so no AddIoSpace() is made. FreeBase is then advanced to the >top of IO#1. > >In the next iteration, the nonexistent type desc (if it exists in the >array) is skipped; FreeBase is left unchanged. > >In the next iteration, the range between FreeBase (= the top of IO#1) >and the bottom of IO#2 is added to the IO space map. However, the top of >IO#2 exceeds Base+Length -- it overshoots the requested aperture. If >there is anything else already above Base+Length, the addition will >fail, and the ASSERT() will be triggered. yes I see the problem. But the problem is not what you described here. The problem is the range between [IO#1_End, Base + Length) is not added to GCD. Because the IO#2 cannot meet the overlap check. So my code actually needs additional code to add [IO#1_End, Base+Length) when for-loop ends but FreeBase < Base + Length. > > >** Now consider a simple modification of the above example, for a >different failure mode -- remove IO#2 from the picture. In this case, >the range between the top of IO#1 and Base+Length will never be covered. >The algorithm will report success, but the aperture will not be complete. > The 2nd example is the same as the 1st example I think. > >(7) Here's what I propose instead. The sorting of the descriptors at the >beginning is fine. We should then loop over the descriptors. If there is >a *gap* in the descriptor list (that is, the end of the previous >descriptor does not exactly equal the start of the next descriptor), >then we shall *simulate* a descriptor that has type "nonexistent". With >this out the way, let's see what we will do in the loop body: > >Calculate the *intersection* of the descriptor and the aperture. This is >done by taking the maximum of the minima, and the minimum of the maxima: > >IntersectionBase = MAX (Descriptor->Base, ApertureBase); >IntersectionEnd = MIN (Descriptor->Base + Descriptor->Length, > ApertureBase + ApertureLength); > >(Note that IntersectionBase is inclusive, and IntersectionEnd is exclusive.) > >Now, if the intersection is the null set: > > (IntersectionBase >= IntersectionEnd) > >then there is no overlap between the aperture and the descriptor -- great. > >If there is overlap between them, then the intersection gives us >*exactly* that overlap. So we just have to investigate the type: > >- IO: that's fine already, do nothing. >- Nonexistent: either explicitly, or by way of the simulation above: > add it as IO -- it *must* succeed >- otherwise: ABORT. > >Actually, we wouldn't even have to sort the list of descriptors, if we >were sure that there are no gaps in it. Because there may be gaps, we >must simulate nonexistent descriptors, and for that simulation we need >to adjacency check, hence the sorting. There is no gap. Every byte is covered by a descriptor. So your algorithm can just check every range which overlaps with [Base, Base + Length). I have checked your proposed patch. I think with the assumption that every byte is covered by a descriptor. Sorting can be avoided and your patch can be simplified further, not only removing sorting, but also others. > >For MMIO, the type investigation is a bit different (stolen from your >code below): >- MMIO with a superset of capabilities: already okay, skip it >- nonexistent: either explicitly or by way of simulation (i.e., gap): > add it as MMIO with the right capabilities -- it must succeed >- otherwise: ABORT > >> + >> + FreePool (IoSpaceMap); >> + >> + DEBUG_CODE ( >> + // >> + // Make sure there is a descriptor covering [Base, Base + Length). >> + // >> + EFI_GCD_IO_SPACE_DESCRIPTOR Descriptor; >> + Status = gDS->GetIoSpaceDescriptor (Base, &Descriptor); >> + ASSERT_EFI_ERROR (Status); >> + ASSERT (Descriptor.BaseAddress <= Base && Descriptor.BaseAddress + >> Descriptor.Length >= Base + Length); >> + ASSERT (Descriptor.GcdIoType == EfiGcdIoTypeIo); >> + ); >> + return EFI_SUCCESS; >> +} > >This looks good. > >Ray, are you okay if I modify your patch to show what I mean, and I post >it? I will keep your Signed-off-by in the first position. > >Thanks >Laszlo > >> + >> +/** >> + 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 ( >> + UINT64 Base, >> + UINT64 Length, >> + UINT64 Capabilities >> + ) >> +{ >> + EFI_STATUS Status; >> + UINTN Index; >> + UINTN NumberOfDescriptors; >> + EFI_GCD_MEMORY_SPACE_DESCRIPTOR *MemorySpaceMap; >> + UINT64 DescriptorLimit; >> + UINT64 FreeBase; >> + >> + Status = gDS->GetMemorySpaceMap (&NumberOfDescriptors, &MemorySpaceMap); >> + ASSERT_EFI_ERROR (Status); >> + >> + PerformQuickSort (MemorySpaceMap, NumberOfDescriptors, sizeof >> (EFI_GCD_MEMORY_SPACE_DESCRIPTOR), >DescriptorComparator); >> + >> + for (Index = 0, FreeBase = Base; Index < NumberOfDescriptors && FreeBase >> < Base + Length; Index++) { >> + DescriptorLimit = MemorySpaceMap[Index].BaseAddress + >> MemorySpaceMap[Index].Length; >> + if (((DescriptorLimit >= FreeBase) && (DescriptorLimit < Base + >> Length)) || >> + ((MemorySpaceMap[Index].BaseAddress >= FreeBase) && >> (MemorySpaceMap[Index].BaseAddress < Base + >Length)) >> + ) { >> + // >> + // For descriptors which overlap with [Base, Base + Length), we need >> to make sure >> + // it's MMIO type and its capabilities matches what we want. >> + // Assertion is enough because failed to start PCI Host Bridge is a >> fatal error. >> + // >> + if (MemorySpaceMap[Index].GcdMemoryType != >> EfiGcdMemoryTypeNonExistent) { >> + ASSERT (MemorySpaceMap[Index].GcdMemoryType == >> EfiGcdMemoryTypeMemoryMappedIo); >> + ASSERT ((MemorySpaceMap[Index].Capabilities & Capabilities) == >> Capabilities); >> + if (MemorySpaceMap[Index].BaseAddress > FreeBase) { >> + Status = gDS->AddMemorySpace (EfiGcdMemoryTypeMemoryMappedIo, >> FreeBase, >MemorySpaceMap[Index].BaseAddress - FreeBase, Capabilities); >> + DEBUG ((EFI_D_INFO, "[PciHostBridge]: Add MMIO Space [%lx, >> %lx)\n", FreeBase, >MemorySpaceMap[Index].BaseAddress)); >> + ASSERT_EFI_ERROR (Status); >> + } >> + FreeBase = DescriptorLimit; >> + } >> + } >> + } >> + >> + FreePool (MemorySpaceMap); >> + >> + DEBUG_CODE ( >> + // >> + // Make sure there is a descriptor covering [Base, Base + Length). >> + // >> + EFI_GCD_MEMORY_SPACE_DESCRIPTOR Descriptor; >> + Status = gDS->GetMemorySpaceDescriptor (Base, &Descriptor); >> + ASSERT_EFI_ERROR (Status); >> + ASSERT (Descriptor.BaseAddress <= Base && Descriptor.BaseAddress + >> Descriptor.Length >= Base + Length); >> + ASSERT (Descriptor.GcdMemoryType == EfiGcdMemoryTypeMemoryMappedIo); >> + ASSERT ((Descriptor.Capabilities & Capabilities) == Capabilities); >> + ); >> + >> + return Status; >> +} >> + >> +/** >> >> Entry point of this driver. >> >> @@ -89,6 +254,7 @@ InitializePciHostBridge ( >> &gEfiPciHostBridgeResourceAllocationProtocolGuid, >> &HostBridge->ResAlloc, >> NULL >> ); >> + ASSERT_EFI_ERROR (Status); >> if (EFI_ERROR (Status)) { >> FreePool (HostBridge); >> PciHostBridgeFreeRootBridges (RootBridges, RootBridgeCount); >> @@ -109,11 +275,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 +295,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, >> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.h >b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.h >> index 9a8ca21..6a9bb0a 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/PciHostBridgeDxe.inf >b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf >> index ab5d87e..a11f5a4 100644 >> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf >> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf >> @@ -42,6 +42,7 @@ >> BaseLib >> PciSegmentLib >> PciHostBridgeLib >> + SortLib >> >> [Protocols] >> gEfiMetronomeArchProtocolGuid ## CONSUMES >> > >_______________________________________________ >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
