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:[email protected]] On Behalf Of Leif Lindholm Sent: Thursday, October 29, 2015 19:55 To: Tian, Feng Cc: Ni, Ruiyu; Kinney, Michael D; [email protected]; 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:[email protected]] On Behalf Of > Leif Lindholm > Sent: Wednesday, October 28, 2015 22:51 > To: Tian, Feng > Cc: Ni, Ruiyu; [email protected]; 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 <[email protected]> 写道： > > > > > > > > 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 <[email protected]> > > > >> Cc: Jeff Fan <[email protected]> > > > >> Cc: Feng Tian <[email protected]> > > > >> --- > > > >> .../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 > > > >> [email protected] > > > >> https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ > edk2-devel mailing list > [email protected] > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
