Hi Ray, Many thanks for this.
On Fri, Oct 30, 2015 at 04:53:54AM +0000, Ni, Ruiyu wrote: > Ok I will revert the two patches I committed (big patch + small > patch). But I want to clarify one thing that not every big patch can > be easily understood by just splitting to small patches. Certainly, but it is always helpful to split as much as is possible. The shorter a patch is to review, the more likely reviewers are to spot mistakes - especially when reviewing code they are not already very familiar with. As an example, I have split your set up a little bit further by breaking 2/3 into two separate patches. https://git.linaro.org/people/leif.lindholm/edk2.git/shortlog/refs/heads/ray-pci I have also tried to remove modifications completely unrelated to this fix - resulting in a difference between the tree with your current patch set and my variant as included below. The changes in your set are all valid changes, but they are not related to the fix. The method behind all of this madness is to make the output of "git blame" as relevant as possible, as well as making automated "git bisect" sessions for tracking down which specific change caused an issue more useful. Best Regards, Leif Diff between Ray's v2 and my version of it: diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c index 12647be..523c698 100644 --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c @@ -281,13 +281,13 @@ DumpResourceMap ( IN UINTN ResourceCount ) { - EFI_STATUS Status; - LIST_ENTRY *Link; - PCI_IO_DEVICE *Device; - UINTN Index; - CHAR16 *Str; - PCI_RESOURCE_NODE **ChildResources; - UINTN ChildResourceCount; + EFI_STATUS Status;^M + LIST_ENTRY *Link;^M + PCI_IO_DEVICE *Device;^M + UINTN Index;^M + CHAR16 *Str;^M + PCI_RESOURCE_NODE **ChildResources;^M + UINTN ChildResourceCount;^M DEBUG ((EFI_D_INFO, "PciBus: Resource Map for ")); @@ -976,7 +976,7 @@ PciScanBus ( UINT8 Device; UINT8 Func; UINT64 Address; - UINT8 SecondBus; + UINTN SecondBus;^M UINT8 PaddedSubBus; UINT16 Register; UINTN HpIndex; @@ -1211,7 +1211,7 @@ PciScanBus ( Status = PciScanBus ( PciDevice, - SecondBus, + (UINT8) (SecondBus),^M SubBusNumber, PaddedBusRange ); @@ -1227,7 +1227,7 @@ PciScanBus ( if ((Attributes == EfiPaddingPciRootBridge) && (State & EFI_HPC_STATE_ENABLED) != 0 && (State & EFI_HPC_STATE_INITIALIZED) != 0) { - *PaddedBusRange = (UINT8) ((UINT8) (BusRange) + *PaddedBusRange); + *PaddedBusRange = (UINT8) ((UINT8) (BusRange) +*PaddedBusRange);^M } else { // // Reserve the larger one between the actual occupied bus // number and padded bus number diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c index e12d59f..fb6270b 100644 --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c @@ -245,6 +245,7 @@ CalculateApertureIo16 ( ) { Node = RESOURCE_NODE_FROM_LINK (CurrentLink); +^M if (Node->ResourceUsage == PciResUsagePadding) { ASSERT (PaddingAperture == 0); PaddingAperture = Node->Length; @@ -303,7 +304,8 @@ CalculateApertureIo16 ( } // - // Adjust the aperture with the bridge's alignment + // At last, adjust the aperture with the bridge's^M + // alignment^M // Offset = Aperture & (Bridge->Alignment); @@ -346,6 +348,7 @@ CalculateResourceAperture ( UINT64 Aperture; LIST_ENTRY *CurrentLink; PCI_RESOURCE_NODE *Node; +^M UINT64 PaddingAperture; UINT64 Offset; @@ -419,7 +422,7 @@ CalculateResourceAperture ( } // - // Adjust the bridge's alignment to the first child's alignment + // At last, adjust the bridge's alignment to the first child's // alignment^M // if the bridge has at least one child // CurrentLink = Bridge->ChildList.ForwardLink; > > Thanks, > Ray > > -----Original Message----- > From: Tian, Feng > Sent: Friday, October 30, 2015 9:57 AM > To: Leif Lindholm <leif.lindh...@linaro.org> > Cc: Ni, Ruiyu <ruiyu...@intel.com>; Kinney, Michael D > <michael.d.kin...@intel.com>; edk2-devel@lists.01.org; Fan, Jeff > <jeff....@intel.com>; Tian, Feng <feng.t...@intel.com> > Subject: RE: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug > > Leif, > > Thanks for raising this issue. > > I agree with you that the patch should be split to small ones and make it > more readable. I also agree we should give community more time to review > those big patch before getting committed in. > > Ray, who is the module owner, will follow up your suggestions. > > Thanks > Feng > > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Leif > Lindholm > Sent: Thursday, October 29, 2015 19:55 > To: Tian, Feng > Cc: Ni, Ruiyu; Kinney, Michael D; edk2-devel@lists.01.org; Fan, Jeff > Subject: Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug > > Hi Feng, > > On Thu, Oct 29, 2015 at 12:23:04AM +0000, Tian, Feng wrote: > > Ray has sent the patch for review, you may ignore it. > > (http://article.gmane.org/gmane.comp.bios.edk2.devel/3484) > > Yes, I see it has now been committed - which solves the known bug. > > But this still leaves us with a clearly not sufficiently reviewed/tested > invasive patch in core PCI code, and a jumbled commit history. > > Since I have not received any response when asking previously, I will ask > again: Can we please revert SVN r18658 and introduce this changeset in a more > reviewable form, with proper review/testing? > > Regards, > > Leif > > > -----Original Message----- > > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of > > Leif Lindholm > > Sent: Wednesday, October 28, 2015 22:51 > > To: Tian, Feng > > Cc: Ni, Ruiyu; edk2-devel@lists.01.org; Fan, Jeff > > Subject: Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug > > > > Feng, > > > > Any update on the below? > > The hard crash bug is still in SVN r18690. > > > > Regards, > > > > Leif > > > > On Mon, Oct 26, 2015 at 03:35:15PM +0000, Leif Lindholm wrote: > > > Hi Ray, > > > > > > On Mon, Oct 26, 2015 at 03:17:22PM +0000, Ni, Ruiyu wrote: > > > > Can you add a null pointer check to > > > > PciIoDevice->ResourcePaddingDescriptors before calling > > > > DumpPpbPaddingResource? Does it help? > > > > > > It removes the delay and the crash - thanks! > > > > > > But it does nothing for the commit history ;) > > > > > > Regards, > > > > > > Leif > > > > > > > > 在 2015年10月26日，21:13，Leif Lindholm <leif.lindh...@linaro.org> 写道： > > > > > > > > > > Hi Ruiyu, Feng, > > > > > > > > > > I am currently tracking down an issue on (at least) one of my > > > > > platforms - that happens with this (now committed) patch, but > > > > > not without it. > > > > > > > > > > Symptoms are a _long_ delay, followed by an unaligned access in > > > > > (I > > > > > think) DumpPpbPaddingResource. > > > > > > > > > > Anyway, there could be other things playing in here - I'm > > > > > testing a new card in a platform I haven't previously tested cards. > > > > > > > > > > But this is one large patch, which could have been split up into > > > > > multiple ones to make the introduced changes more reviewable: > > > > > - There are both functional changes and whitespace fixups. > > > > > - There are (text-only) changes to existing messages. > > > > > - There is refactoring of internal APIs. > > > > > > > > > > It is certainly too invasive a change to be committed ~32 > > > > > minutes after having first been posted to the list. > > > > > > > > > > Any chance we can revert this one and introduce it in smaller > > > > > portions, making the individual change sets more reviewable? > > > > > > > > > > Regards, > > > > > > > > > > Leif > > > > > > > > > >> On Fri, Oct 23, 2015 at 03:57:40PM +0800, Ruiyu Ni wrote: > > > > >> For a hot plug bridge with device attached, PciBusDxe driver > > > > >> reserves the resources which equal to the total amount of > > > > >> padding resource returned from HotPlug->GetResourcePadding() > > > > >> and the actual occupied resource by the attached device. The > > > > >> behavior is incorrect. > > > > >> Correct behavior is to reserve the bigger one between the > > > > >> padding resource and the actual occupied resource. > > > > >> > > > > >> Contributed-under: TianoCore Contribution Agreement 1.0 > > > > >> Signed-off-by: Ruiyu Ni <ruiyu...@intel.com> > > > > >> Cc: Jeff Fan <jeff....@intel.com> > > > > >> Cc: Feng Tian <feng.t...@intel.com> > > > > >> --- > > > > >> .../Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 76 +++++++++- > > > > >> .../Bus/Pci/PciBusDxe/PciEnumeratorSupport.h | 13 ++ > > > > >> MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c | 159 > > > > >> ++++++++++----------- > > > > >> .../Bus/Pci/PciBusDxe/PciResourceSupport.c | 58 +++++--- > > > > >> 4 files changed, 204 insertions(+), 102 deletions(-) > > > > >> > > > > >> diff --git > > > > >> a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > > > > >> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > > > > >> index f7aea4f..030ef42 100644 > > > > >> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > > > > >> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > > > > >> @@ -325,6 +325,77 @@ PciSearchDevice ( } > > > > >> > > > > >> /** > > > > >> + Dump the PPB padding resource information. > > > > >> + > > > > >> + @param PciIoDevice PCI IO instance. > > > > >> + @param ResourceType The desired resource type to dump. > > > > >> + PciBarTypeUnknown means to dump all types > > > > >> of resources. > > > > >> +**/ > > > > >> +VOID > > > > >> +DumpPpbPaddingResource ( > > > > >> + IN PCI_IO_DEVICE *PciIoDevice, > > > > >> + IN PCI_BAR_TYPE ResourceType > > > > >> + ) > > > > >> +{ > > > > >> + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Descriptor; > > > > >> + PCI_BAR_TYPE Type; > > > > >> + > > > > >> + if (ResourceType == PciBarTypeIo16 || ResourceType == > > > > >> PciBarTypeIo32) { > > > > >> + ResourceType = PciBarTypeIo; } > > > > >> + > > > > >> + for (Descriptor = PciIoDevice->ResourcePaddingDescriptors; > > > > >> + Descriptor->Desc != ACPI_END_TAG_DESCRIPTOR; Descriptor++) { > > > > >> + > > > > >> + Type = PciBarTypeUnknown; > > > > >> + if (Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR && > > > > >> Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_IO) { > > > > >> + Type = PciBarTypeIo; > > > > >> + } else if (Descriptor->Desc == > > > > >> + ACPI_ADDRESS_SPACE_DESCRIPTOR && Descriptor->ResType == > > > > >> + ACPI_ADDRESS_SPACE_TYPE_MEM) { > > > > >> + > > > > >> + if (Descriptor->AddrSpaceGranularity == 32) { > > > > >> + // > > > > >> + // prefechable > > > > >> + // > > > > >> + if (Descriptor->SpecificFlag == > > > > >> EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) { > > > > >> + Type = PciBarTypePMem32; > > > > >> + } > > > > >> + > > > > >> + // > > > > >> + // Non-prefechable > > > > >> + // > > > > >> + if (Descriptor->SpecificFlag == 0) { > > > > >> + Type = PciBarTypeMem32; > > > > >> + } > > > > >> + } > > > > >> + > > > > >> + if (Descriptor->AddrSpaceGranularity == 64) { > > > > >> + // > > > > >> + // prefechable > > > > >> + // > > > > >> + if (Descriptor->SpecificFlag == > > > > >> EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) { > > > > >> + Type = PciBarTypePMem64; > > > > >> + } > > > > >> + > > > > >> + // > > > > >> + // Non-prefechable > > > > >> + // > > > > >> + if (Descriptor->SpecificFlag == 0) { > > > > >> + Type = PciBarTypeMem64; > > > > >> + } > > > > >> + } > > > > >> + } > > > > >> + > > > > >> + if ((Type != PciBarTypeUnknown) && ((ResourceType == > > > > >> PciBarTypeUnknown) || (ResourceType == Type))) { > > > > >> + DEBUG (( > > > > >> + EFI_D_INFO, > > > > >> + " Padding: Type = %s; Alignment = 0x%lx;\tLength = > > > > >> 0x%lx\n", > > > > >> + mBarTypeStr[Type], Descriptor->AddrRangeMax, > > > > >> Descriptor->AddrLen > > > > >> + )); > > > > >> + } > > > > >> + } > > > > >> + > > > > >> +} > > > > >> + > > > > >> +/** > > > > >> Dump the PCI BAR information. > > > > >> > > > > >> @param PciIoDevice PCI IO instance. > > > > >> @@ -586,7 +657,10 @@ GatherPpbInfo ( > > > > >> > > > > >> GetResourcePaddingPpb (PciIoDevice); > > > > >> > > > > >> - DEBUG_CODE (DumpPciBars (PciIoDevice);); > > > > >> + DEBUG_CODE ( > > > > >> + DumpPpbPaddingResource (PciIoDevice, PciBarTypeUnknown); > > > > >> + DumpPciBars (PciIoDevice); ); > > > > >> > > > > >> return PciIoDevice; > > > > >> } > > > > >> diff --git > > > > >> a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.h > > > > >> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.h > > > > >> index a4489b8..4d7b3b7 100644 > > > > >> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.h > > > > >> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.h > > > > >> @@ -460,4 +460,17 @@ ResetAllPpbBusNumber ( > > > > >> IN UINT8 StartBusNumber > > > > >> ); > > > > >> > > > > >> +/** > > > > >> + Dump the PPB padding resource information. > > > > >> + > > > > >> + @param PciIoDevice PCI IO instance. > > > > >> + @param ResourceType The desired resource type to dump. > > > > >> + PciBarTypeUnknown means to dump all types > > > > >> of resources. > > > > >> +**/ > > > > >> +VOID > > > > >> +DumpPpbPaddingResource ( > > > > >> + IN PCI_IO_DEVICE *PciIoDevice, > > > > >> + IN PCI_BAR_TYPE ResourceType > > > > >> + ); > > > > >> + > > > > >> #endif > > > > >> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > > > > >> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > > > > >> index 3e275e3..f4b6ebf 100644 > > > > >> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > > > > >> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > > > > >> @@ -188,19 +188,21 @@ DumpBridgeResource ( > > > > >> > > > > >> BridgeResource->PciDev->PciBar[BridgeResource->Bar].BaseAddress, > > > > >> BridgeResource->Length, BridgeResource->Alignment > > > > >> )); > > > > >> - for ( Link = BridgeResource->ChildList.ForwardLink > > > > >> - ; Link != &BridgeResource->ChildList > > > > >> - ; Link = Link->ForwardLink > > > > >> + for ( Link = GetFirstNode (&BridgeResource->ChildList) > > > > >> + ; !IsNull (&BridgeResource->ChildList, Link) > > > > >> + ; Link = GetNextNode (&BridgeResource->ChildList, > > > > >> + Link) > > > > >> ) { > > > > >> Resource = RESOURCE_NODE_FROM_LINK (Link); > > > > >> if (Resource->ResourceUsage == PciResUsageTypical) { > > > > >> Bar = Resource->Virtual ? Resource->PciDev->VfPciBar : > > > > >> Resource->PciDev->PciBar; > > > > >> DEBUG (( > > > > >> - EFI_D_INFO, " Base = 0x%lx;\tLength = 0x%lx;\tAlignment = > > > > >> 0x%lx;\tOwner = %s ", > > > > >> + EFI_D_INFO, " Base = 0x%lx;\tLength = 0x%lx;\tAlignment > > > > >> = 0x%lx;\tOwner = %s [%02x|%02x|%02x:", > > > > >> Bar[Resource->Bar].BaseAddress, Resource->Length, > > > > >> Resource->Alignment, > > > > >> IS_PCI_BRIDGE (&Resource->PciDev->Pci) ? L"PPB" : > > > > >> IS_CARDBUS_BRIDGE (&Resource->PciDev->Pci) ? L"P2C" : > > > > >> - L"PCI" > > > > >> + L"PCI", > > > > >> + Resource->PciDev->BusNumber, > > > > >> Resource->PciDev->DeviceNumber, > > > > >> + Resource->PciDev->FunctionNumber > > > > >> )); > > > > >> > > > > >> if ((!IS_PCI_BRIDGE (&Resource->PciDev->Pci) && > > > > >> !IS_CARDBUS_BRIDGE (&Resource->PciDev->Pci)) || @@ -210,24 +212,20 > > > > >> @@ DumpBridgeResource ( > > > > >> // > > > > >> // The resource requirement comes from the device itself. > > > > >> // > > > > >> - DEBUG (( > > > > >> - EFI_D_INFO, " [%02x|%02x|%02x:%02x]\n", > > > > >> - Resource->PciDev->BusNumber, > > > > >> Resource->PciDev->DeviceNumber, > > > > >> - Resource->PciDev->FunctionNumber, > > > > >> Bar[Resource->Bar].Offset > > > > >> - )); > > > > >> + DEBUG ((EFI_D_INFO, "%02x]", > > > > >> + Bar[Resource->Bar].Offset)); > > > > >> } else { > > > > >> // > > > > >> // The resource requirement comes from the subordinate > > > > >> devices. > > > > >> // > > > > >> - DEBUG (( > > > > >> - EFI_D_INFO, " [%02x|%02x|%02x:**]\n", > > > > >> - Resource->PciDev->BusNumber, > > > > >> Resource->PciDev->DeviceNumber, > > > > >> - Resource->PciDev->FunctionNumber > > > > >> - )); > > > > >> + DEBUG ((EFI_D_INFO, "**]")); > > > > >> } > > > > >> } else { > > > > >> - DEBUG ((EFI_D_INFO, " Padding:Length = 0x%lx;\tAlignment = > > > > >> 0x%lx\n", Resource->Length, Resource->Alignment)); > > > > >> + DEBUG ((EFI_D_INFO, " Base = Padding;\tLength = > > > > >> 0x%lx;\tAlignment = 0x%lx", Resource->Length, Resource->Alignment)); > > > > >> } > > > > >> + if (BridgeResource->ResType != Resource->ResType) { > > > > >> + DEBUG ((EFI_D_INFO, "; Type = %s", mBarTypeStr[MIN > > > > >> (Resource->ResType, PciBarTypeMaxType)])); > > > > >> + } > > > > >> + DEBUG ((EFI_D_INFO, "\n")); > > > > >> } > > > > >> } > > > > >> } > > > > >> @@ -235,63 +233,61 @@ DumpBridgeResource ( > > > > >> /** > > > > >> Find the corresponding resource node for the Device in child list > > > > >> of BridgeResource. > > > > >> > > > > >> - @param[in] Device Pointer to PCI_IO_DEVICE. > > > > >> - @param[in] BridgeResource Pointer to PCI_RESOURCE_NODE. > > > > >> + @param[in] Device Pointer to PCI_IO_DEVICE. > > > > >> + @param[in] BridgeResource Pointer to PCI_RESOURCE_NODE. > > > > >> + @param[out] DeviceResources Pointer to a buffer to receive > > > > >> resources for the Device. > > > > >> > > > > >> - @return !NULL The corresponding resource node for the Device. > > > > >> - @return NULL No corresponding resource node for the Device. > > > > >> + @return Count of the resource descriptors returned. > > > > >> **/ > > > > >> -PCI_RESOURCE_NODE * > > > > >> +UINTN > > > > >> FindResourceNode ( > > > > >> - IN PCI_IO_DEVICE *Device, > > > > >> - IN PCI_RESOURCE_NODE *BridgeResource > > > > >> + IN PCI_IO_DEVICE *Device, > > > > >> + IN PCI_RESOURCE_NODE *BridgeResource, OUT > > > > >> + PCI_RESOURCE_NODE **DeviceResources OPTIONAL > > > > >> ) > > > > >> { > > > > >> LIST_ENTRY *Link; > > > > >> PCI_RESOURCE_NODE *Resource; > > > > >> + UINTN Count; > > > > >> > > > > >> + Count = 0; > > > > >> for ( Link = BridgeResource->ChildList.ForwardLink > > > > >> ; Link != &BridgeResource->ChildList > > > > >> ; Link = Link->ForwardLink > > > > >> ) { > > > > >> Resource = RESOURCE_NODE_FROM_LINK (Link); > > > > >> if (Resource->PciDev == Device) { > > > > >> - return Resource; > > > > >> + if (DeviceResources != NULL) { > > > > >> + DeviceResources[Count] = Resource; > > > > >> + } > > > > >> + Count++; > > > > >> } > > > > >> } > > > > >> > > > > >> - return NULL; > > > > >> + return Count; > > > > >> } > > > > >> > > > > >> /** > > > > >> Dump the resource map of all the devices under Bridge. > > > > >> > > > > >> - @param[in] Bridge Bridge device instance. > > > > >> - @param[in] IoNode IO resource descriptor for the bridge > > > > >> device. > > > > >> - @param[in] Mem32Node Mem32 resource descriptor for the bridge > > > > >> device. > > > > >> - @param[in] PMem32Node PMem32 resource descriptor for the bridge > > > > >> device. > > > > >> - @param[in] Mem64Node Mem64 resource descriptor for the bridge > > > > >> device. > > > > >> - @param[in] PMem64Node PMem64 resource descriptor for the bridge > > > > >> device. > > > > >> + @param[in] Bridge Bridge device instance. > > > > >> + @param[in] Resources Resource descriptors for the bridge > > > > >> device. > > > > >> + @param[in] ResourceCount Count of resource descriptors. > > > > >> **/ > > > > >> VOID > > > > >> DumpResourceMap ( > > > > >> IN PCI_IO_DEVICE *Bridge, > > > > >> - IN PCI_RESOURCE_NODE *IoNode, > > > > >> - IN PCI_RESOURCE_NODE *Mem32Node, > > > > >> - IN PCI_RESOURCE_NODE *PMem32Node, > > > > >> - IN PCI_RESOURCE_NODE *Mem64Node, > > > > >> - IN PCI_RESOURCE_NODE *PMem64Node > > > > >> + IN PCI_RESOURCE_NODE **Resources, > > > > >> + IN UINTN ResourceCount > > > > >> ) > > > > >> { > > > > >> - EFI_STATUS Status; > > > > >> - LIST_ENTRY *Link; > > > > >> - PCI_IO_DEVICE *Device; > > > > >> - PCI_RESOURCE_NODE *ChildIoNode; > > > > >> - PCI_RESOURCE_NODE *ChildMem32Node; > > > > >> - PCI_RESOURCE_NODE *ChildPMem32Node; > > > > >> - PCI_RESOURCE_NODE *ChildMem64Node; > > > > >> - PCI_RESOURCE_NODE *ChildPMem64Node; > > > > >> - CHAR16 *Str; > > > > >> + EFI_STATUS Status; > > > > >> + LIST_ENTRY *Link; > > > > >> + PCI_IO_DEVICE *Device; > > > > >> + UINTN Index; > > > > >> + CHAR16 *Str; > > > > >> + PCI_RESOURCE_NODE **ChildResources; > > > > >> + UINTN ChildResourceCount; > > > > >> > > > > >> DEBUG ((EFI_D_INFO, "PciBus: Resource Map for ")); > > > > >> > > > > >> @@ -320,11 +316,9 @@ DumpResourceMap ( > > > > >> } > > > > >> } > > > > >> > > > > >> - DumpBridgeResource (IoNode); > > > > >> - DumpBridgeResource (Mem32Node); > > > > >> - DumpBridgeResource (PMem32Node); > > > > >> - DumpBridgeResource (Mem64Node); > > > > >> - DumpBridgeResource (PMem64Node); > > > > >> + for (Index = 0; Index < ResourceCount; Index++) { > > > > >> + DumpBridgeResource (Resources[Index]); } > > > > >> DEBUG ((EFI_D_INFO, "\n")); > > > > >> > > > > >> for ( Link = Bridge->ChildList.ForwardLink @@ -334,20 +328,19 > > > > >> @@ DumpResourceMap ( > > > > >> Device = PCI_IO_DEVICE_FROM_LINK (Link); > > > > >> if (IS_PCI_BRIDGE (&Device->Pci)) { > > > > >> > > > > >> - ChildIoNode = (IoNode == NULL ? NULL : > > > > >> FindResourceNode (Device, IoNode)); > > > > >> - ChildMem32Node = (Mem32Node == NULL ? NULL : > > > > >> FindResourceNode (Device, Mem32Node)); > > > > >> - ChildPMem32Node = (PMem32Node == NULL ? NULL : > > > > >> FindResourceNode (Device, PMem32Node)); > > > > >> - ChildMem64Node = (Mem64Node == NULL ? NULL : > > > > >> FindResourceNode (Device, Mem64Node)); > > > > >> - ChildPMem64Node = (PMem64Node == NULL ? NULL : > > > > >> FindResourceNode (Device, PMem64Node)); > > > > >> - > > > > >> - DumpResourceMap ( > > > > >> - Device, > > > > >> - ChildIoNode, > > > > >> - ChildMem32Node, > > > > >> - ChildPMem32Node, > > > > >> - ChildMem64Node, > > > > >> - ChildPMem64Node > > > > >> - ); > > > > >> + ChildResourceCount = 0; > > > > >> + for (Index = 0; Index < ResourceCount; Index++) { > > > > >> + ChildResourceCount += FindResourceNode (Device, > > > > >> Resources[Index], NULL); > > > > >> + } > > > > >> + ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * > > > > >> ChildResourceCount); > > > > >> + ASSERT (ChildResources != NULL); > > > > >> + ChildResourceCount = 0; > > > > >> + for (Index = 0; Index < ResourceCount; Index++) { > > > > >> + ChildResourceCount += FindResourceNode (Device, > > > > >> Resources[Index], &ChildResources[ChildResourceCount]); > > > > >> + } > > > > >> + > > > > >> + DumpResourceMap (Device, ChildResources, ChildResourceCount); > > > > >> + FreePool (ChildResources); > > > > >> } > > > > >> } > > > > >> } > > > > >> @@ -807,11 +800,11 @@ PciHostBridgeResourceAllocator ( > > > > >> // Create the entire system resource map from the information > > > > >> collected by > > > > >> // enumerator. Several resource tree was created > > > > >> // > > > > >> - IoBridge = FindResourceNode (RootBridgeDev, &IoPool); > > > > >> - Mem32Bridge = FindResourceNode (RootBridgeDev, &Mem32Pool); > > > > >> - PMem32Bridge = FindResourceNode (RootBridgeDev, &PMem32Pool); > > > > >> - Mem64Bridge = FindResourceNode (RootBridgeDev, &Mem64Pool); > > > > >> - PMem64Bridge = FindResourceNode (RootBridgeDev, &PMem64Pool); > > > > >> + FindResourceNode (RootBridgeDev, &IoPool, &IoBridge); > > > > >> + FindResourceNode (RootBridgeDev, &Mem32Pool, &Mem32Bridge); > > > > >> + FindResourceNode (RootBridgeDev, &PMem32Pool, &PMem32Bridge); > > > > >> + FindResourceNode (RootBridgeDev, &Mem64Pool, &Mem64Bridge); > > > > >> + FindResourceNode (RootBridgeDev, &PMem64Pool, > > > > >> + &PMem64Bridge); > > > > >> > > > > >> ASSERT (IoBridge != NULL); > > > > >> ASSERT (Mem32Bridge != NULL); @@ -869,14 +862,13 @@ > > > > >> PciHostBridgeResourceAllocator ( > > > > >> // Dump the resource map for current root bridge > > > > >> // > > > > >> DEBUG_CODE ( > > > > >> - DumpResourceMap ( > > > > >> - RootBridgeDev, > > > > >> - IoBridge, > > > > >> - Mem32Bridge, > > > > >> - PMem32Bridge, > > > > >> - Mem64Bridge, > > > > >> - PMem64Bridge > > > > >> - ); > > > > >> + PCI_RESOURCE_NODE *Resources[5]; > > > > >> + Resources[0] = IoBridge; > > > > >> + Resources[1] = Mem32Bridge; > > > > >> + Resources[2] = PMem32Bridge; > > > > >> + Resources[3] = Mem64Bridge; > > > > >> + Resources[4] = PMem64Bridge; > > > > >> + DumpResourceMap (RootBridgeDev, Resources, sizeof > > > > >> + (Resources) / sizeof (Resources[0])); > > > > >> ); > > > > >> > > > > >> FreePool (AcpiConfig); > > > > >> @@ -984,7 +976,8 @@ PciScanBus ( > > > > >> UINT8 Device; > > > > >> UINT8 Func; > > > > >> UINT64 Address; > > > > >> - UINTN SecondBus; > > > > >> + UINT8 SecondBus; > > > > >> + UINT8 PaddedSubBus; > > > > >> UINT16 Register; > > > > >> UINTN HpIndex; > > > > >> PCI_IO_DEVICE *PciDevice; > > > > >> @@ -1218,7 +1211,7 @@ PciScanBus ( > > > > >> > > > > >> Status = PciScanBus ( > > > > >> PciDevice, > > > > >> - (UINT8) (SecondBus), > > > > >> + SecondBus, > > > > >> SubBusNumber, > > > > >> PaddedBusRange > > > > >> ); > > > > >> @@ -1234,12 +1227,16 @@ PciScanBus ( > > > > >> if ((Attributes == EfiPaddingPciRootBridge) && > > > > >> (State & EFI_HPC_STATE_ENABLED) != 0 && > > > > >> (State & EFI_HPC_STATE_INITIALIZED) != 0) { > > > > >> - *PaddedBusRange = (UINT8) ((UINT8) (BusRange) > > > > >> +*PaddedBusRange); > > > > >> + *PaddedBusRange = (UINT8) ((UINT8) (BusRange) + > > > > >> + *PaddedBusRange); > > > > >> } else { > > > > >> - Status = PciAllocateBusNumber (PciDevice, > > > > >> *SubBusNumber, (UINT8) (BusRange), SubBusNumber); > > > > >> + // > > > > >> + // Reserve the larger one between the actual occupied > > > > >> bus number and padded bus number > > > > >> + // > > > > >> + Status = PciAllocateBusNumber (PciDevice, > > > > >> + StartBusNumber, (UINT8) (BusRange), &PaddedSubBus); > > > > >> if (EFI_ERROR (Status)) { > > > > >> return Status; > > > > >> } > > > > >> + *SubBusNumber = MAX (PaddedSubBus, *SubBusNumber); > > > > >> } > > > > >> } > > > > >> > > > > >> diff --git > > > > >> a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c > > > > >> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c > > > > >> index d8d988c..b106abe 100644 > > > > >> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c > > > > >> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c > > > > >> @@ -196,6 +196,7 @@ CalculateApertureIo16 ( > > > > >> PCI_RESOURCE_NODE *Node; > > > > >> UINT64 Offset; > > > > >> EFI_PCI_PLATFORM_POLICY PciPolicy; > > > > >> + UINT64 PaddingAperture; > > > > >> > > > > >> if (!mPolicyDetermined) { > > > > >> // > > > > >> @@ -228,21 +229,27 @@ CalculateApertureIo16 ( > > > > >> mPolicyDetermined = TRUE; > > > > >> } > > > > >> > > > > >> - Aperture = 0; > > > > >> + Aperture = 0; > > > > >> + PaddingAperture = 0; > > > > >> > > > > >> if (Bridge == NULL) { > > > > >> return ; > > > > >> } > > > > >> > > > > >> - CurrentLink = Bridge->ChildList.ForwardLink; > > > > >> - > > > > >> // > > > > >> // Assume the bridge is aligned > > > > >> // > > > > >> - while (CurrentLink != &Bridge->ChildList) { > > > > >> + for ( CurrentLink = GetFirstNode (&Bridge->ChildList) > > > > >> + ; !IsNull (&Bridge->ChildList, CurrentLink) > > > > >> + ; CurrentLink = GetNextNode (&Bridge->ChildList, CurrentLink) > > > > >> + ) { > > > > >> > > > > >> Node = RESOURCE_NODE_FROM_LINK (CurrentLink); > > > > >> - > > > > >> + if (Node->ResourceUsage == PciResUsagePadding) { > > > > >> + ASSERT (PaddingAperture == 0); > > > > >> + PaddingAperture = Node->Length; > > > > >> + continue; > > > > >> + } > > > > >> // > > > > >> // Consider the aperture alignment > > > > >> // > > > > >> @@ -293,13 +300,10 @@ CalculateApertureIo16 ( > > > > >> // Increment aperture by the length of node > > > > >> // > > > > >> Aperture += Node->Length; > > > > >> - > > > > >> - CurrentLink = CurrentLink->ForwardLink; > > > > >> } > > > > >> > > > > >> // > > > > >> - // At last, adjust the aperture with the bridge's > > > > >> - // alignment > > > > >> + // Adjust the aperture with the bridge's alignment > > > > >> // > > > > >> Offset = Aperture & (Bridge->Alignment); > > > > >> > > > > >> @@ -319,6 +323,12 @@ CalculateApertureIo16 ( > > > > >> Bridge->Alignment = Node->Alignment; > > > > >> } > > > > >> } > > > > >> + > > > > >> + // > > > > >> + // Hotplug controller needs padding resources. > > > > >> + // Use the larger one between the padding resource and actual > > > > >> occupied resource. > > > > >> + // > > > > >> + Bridge->Length = MAX (Bridge->Length, PaddingAperture); > > > > >> } > > > > >> > > > > >> /** > > > > >> @@ -336,10 +346,11 @@ CalculateResourceAperture ( > > > > >> UINT64 Aperture; > > > > >> LIST_ENTRY *CurrentLink; > > > > >> PCI_RESOURCE_NODE *Node; > > > > >> - > > > > >> + UINT64 PaddingAperture; > > > > >> UINT64 Offset; > > > > >> > > > > >> - Aperture = 0; > > > > >> + Aperture = 0; > > > > >> + PaddingAperture = 0; > > > > >> > > > > >> if (Bridge == NULL) { > > > > >> return ; > > > > >> @@ -351,14 +362,20 @@ CalculateResourceAperture ( > > > > >> return ; > > > > >> } > > > > >> > > > > >> - CurrentLink = Bridge->ChildList.ForwardLink; > > > > >> - > > > > >> // > > > > >> // Assume the bridge is aligned > > > > >> // > > > > >> - while (CurrentLink != &Bridge->ChildList) { > > > > >> + for ( CurrentLink = GetFirstNode (&Bridge->ChildList) > > > > >> + ; !IsNull (&Bridge->ChildList, CurrentLink) > > > > >> + ; CurrentLink = GetNextNode (&Bridge->ChildList, CurrentLink) > > > > >> + ) { > > > > >> > > > > >> Node = RESOURCE_NODE_FROM_LINK (CurrentLink); > > > > >> + if (Node->ResourceUsage == PciResUsagePadding) { > > > > >> + ASSERT (PaddingAperture == 0); > > > > >> + PaddingAperture = Node->Length; > > > > >> + continue; > > > > >> + } > > > > >> > > > > >> // > > > > >> // Apply padding resource if available @@ -381,11 +398,6 @@ > > > > >> CalculateResourceAperture ( > > > > >> // Increment aperture by the length of node > > > > >> // > > > > >> Aperture += Node->Length; > > > > >> - > > > > >> - // > > > > >> - // Consider the aperture alignment > > > > >> - // > > > > >> - CurrentLink = CurrentLink->ForwardLink; > > > > >> } > > > > >> > > > > >> // > > > > >> @@ -407,7 +419,7 @@ CalculateResourceAperture ( > > > > >> } > > > > >> > > > > >> // > > > > >> - // At last, adjust the bridge's alignment to the first > > > > >> child's alignment > > > > >> + // Adjust the bridge's alignment to the first child's > > > > >> + alignment > > > > >> // if the bridge has at least one child > > > > >> // > > > > >> CurrentLink = Bridge->ChildList.ForwardLink; @@ -417,6 > > > > >> +429,12 @@ CalculateResourceAperture ( > > > > >> Bridge->Alignment = Node->Alignment; > > > > >> } > > > > >> } > > > > >> + > > > > >> + // > > > > >> + // Hotplug controller needs padding resources. > > > > >> + // Use the larger one between the padding resource and actual > > > > >> occupied resource. > > > > >> + // > > > > >> + Bridge->Length = MAX (Bridge->Length, PaddingAperture); > > > > >> } > > > > >> > > > > >> /** > > > > >> -- > > > > >> 1.9.5.msysgit.1 > > > > >> > > > > >> _______________________________________________ > > > > >> 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 > _______________________________________________ > 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
