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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
