On 09/23/17 00:52, Laszlo Ersek wrote:
> Hi Ray,
>
> I ran into an interesting problem, while working on
> "OvmfPkg/PciHotPlugInitDxe":
>
> It is an assertion failure in CalculateResourceAperture(), in
> "MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c":
>
>> VOID
>> CalculateResourceAperture (
>> IN PCI_RESOURCE_NODE *Bridge
>> )
>> {
>> UINT64 Aperture;
>> LIST_ENTRY *CurrentLink;
>> PCI_RESOURCE_NODE *Node;
>> UINT64 PaddingAperture;
>> UINT64 Offset;
>>
>> Aperture = 0;
>> PaddingAperture = 0;
>>
>> if (Bridge == NULL) {
>> return ;
>> }
>>
>> if (Bridge->ResType == PciBarTypeIo16) {
>>
>> CalculateApertureIo16 (Bridge);
>> return ;
>> }
>>
>> //
>> // Assume the bridge is aligned
>> //
>> 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;
>> }
>
> The ASSERT() was added in commit c7e7613e09ff ("MdeModulePkg: Fix a
> PciBusDxe hot plug bug", 2015-11-03), which makes the argument that:
>
> Correct behavior is to reserve the bigger one between the padding
> resource and the actual occupied resource.
>
> While I totally agree with that statement, I don't think the ASSERT() is
> correct. The ASSERT() assumes that every resource tree -- Mem32Node,
> PMem32Node, Mem64Node, PMem64Node, IoNode -- will contain at most one
> padding.
>
> I think this assumption can be broken easily; the DegradeResource()
> function merges resource trees for a number of reasons. For example,
>
> - if the Hot Plug Init Protocol requests, for a bridge,
>
> - both 2MB non-prefetchable 32-bit MMIO reservation
>
> - and 2MB prefetchable 32-bit MMIO reservation,
>
> - and the root bridge reported EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM,
>
> then DegradeResource() will merge the PMem32 padding as well into the
> Mem32 tree (not just BARs of devices behind the bridge), while the Mem32
> tree already has a Mem32 padding. Thus the Mem32 tree will end up with
> two paddings.
>
> Other degradations are possible too, which can lead to the same
> situation:
>
> - For example 64-bit -> 32-bit degradation.
>
> - Or we can imagine a recursive situation where we have two bridges
> down-stream from an upstream bridge, the first downstream bridge wants
> a PMem32 padding, while the second downstream bridge wants a Mem32
> padding. On the level of the upstream bridge, I believe these two
> paddings will be merged into the same resource tree (in case of
> EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM).
>
> My question is if the problem is easy enough for me to fix:
>
> (a) Should I replace the ASSERT() and the assignment with summation?
>
> PaddingAperture += Node->Length;
>
> (b) Should I replace them with a MAX() search?
>
> PaddingAperture = MAX (PaddingAperture, Node->Length);
>
> (c) Is the fix more complicated than (a) and (b)?
>
> I think the correct solution is (b), in the spirit of your commit
> c7e7613e09ff, but I'm not sure if any other parts of PciBusDxe need to
> be updated for (b).
I reported this in <https://bugzilla.tianocore.org/show_bug.cgi?id=720>
so that we can track it in the longer term.
Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
