On 02/26/16 13:36, Ni, Ruiyu wrote: > 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:)
I'm going to resume that work (well, hobby, but don't tell my manager! :)) today. Thanks! Laszlo > > 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
