public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* multiple paddings in the same PCI resource tree
@ 2017-09-22 22:52 Laszlo Ersek
  2017-09-25 13:31 ` Laszlo Ersek
  0 siblings, 1 reply; 2+ messages in thread
From: Laszlo Ersek @ 2017-09-22 22:52 UTC (permalink / raw)
  To: Ni, Ruiyu; +Cc: edk2-devel-01

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


^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: multiple paddings in the same PCI resource tree
  2017-09-22 22:52 multiple paddings in the same PCI resource tree Laszlo Ersek
@ 2017-09-25 13:31 ` Laszlo Ersek
  0 siblings, 0 replies; 2+ messages in thread
From: Laszlo Ersek @ 2017-09-25 13:31 UTC (permalink / raw)
  To: Ni, Ruiyu; +Cc: edk2-devel-01

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


^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2017-09-25 13:28 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-09-22 22:52 multiple paddings in the same PCI resource tree Laszlo Ersek
2017-09-25 13:31 ` Laszlo Ersek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox