multiple paddings in the same PCI resource tree

multiple paddings in the same PCI resource tree

[Laszlo Ersek]

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).

Thanks!
Laszlo

Re: multiple paddings in the same PCI resource tree

[Laszlo Ersek]

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