From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 5F7E121E43B49 for ; Fri, 22 Sep 2017 15:48:58 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 5DD8C356C7; Fri, 22 Sep 2017 22:52:06 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 5DD8C356C7 Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-219.rdu2.redhat.com [10.10.120.219]) by smtp.corp.redhat.com (Postfix) with ESMTP id B7EC860BE3; Fri, 22 Sep 2017 22:52:05 +0000 (UTC) To: "Ni, Ruiyu" Cc: edk2-devel-01 From: Laszlo Ersek Message-ID: <3e8c8ffa-761f-1be3-f3b4-0f82d3fb4641@redhat.com> Date: Sat, 23 Sep 2017 00:52:04 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Fri, 22 Sep 2017 22:52:06 +0000 (UTC) Subject: multiple paddings in the same PCI resource tree X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 22 Sep 2017 22:48:58 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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