Leif, I saw your feedbacks as your opinion if you create patches. I don't think your opinion is the *only* right solution. I can also split 3/4 in your patch serials to two patches: one is to just change DumpBridgeResource(), the other is for the remaining changes. So I think my splitting way is good enough. OK I will give response to your questions firstly.
Regards, Ray -----Original Message----- From: Leif Lindholm [mailto:leif.lindh...@linaro.org] Sent: Tuesday, November 3, 2015 6:45 PM To: Ni, Ruiyu <ruiyu...@intel.com> Cc: Tian, Feng <feng.t...@intel.com>; Kinney, Michael D <michael.d.kin...@intel.com>; edk2-devel@lists.01.org; Fan, Jeff <jeff....@intel.com> Subject: Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug Hi guys, I never saw a response to this feedback I sent on the revised patch set, but I see it has now been committed. Could you let me know why my feedback was ignored? Regards, Leif On 30 October 2015 at 18:02, Leif Lindholm <leif.lindh...@linaro.org> wrote: > 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
