Hi Ray, On Tue, Nov 03, 2015 at 02:27:29PM +0000, Ni, Ruiyu wrote: > Leif, > I saw your feedbacks as your opinion if you create patches. I don't > think your opinion is the *only* right solution.
Oh, certainly. I was only expecting some sort of response before the patches were committed again. > 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. And I will always argue for splitting more :) > OK I will give response to your questions firstly. Thanks. Regards, Leif > 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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
