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
