Re: multiple paddings in the same PCI resource tree

[Laszlo Ersek <lersek@redhat.com>]

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