From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org 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 BF69220945BA7 for ; Mon, 25 Sep 2017 06:28:26 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 6B6F82AD61; Mon, 25 Sep 2017 13:31:37 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 6B6F82AD61 Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-161.rdu2.redhat.com [10.10.120.161]) by smtp.corp.redhat.com (Postfix) with ESMTP id 8B3476BF6E; Mon, 25 Sep 2017 13:31:36 +0000 (UTC) From: Laszlo Ersek To: "Ni, Ruiyu" Cc: edk2-devel-01 References: <3e8c8ffa-761f-1be3-f3b4-0f82d3fb4641@redhat.com> Message-ID: <5f08df9f-f2da-8c9f-c447-fcd99733bd9a@redhat.com> Date: Mon, 25 Sep 2017 15:31:35 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <3e8c8ffa-761f-1be3-f3b4-0f82d3fb4641@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Mon, 25 Sep 2017 13:31:37 +0000 (UTC) Subject: Re: 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: Mon, 25 Sep 2017 13:28:26 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 so that we can track it in the longer term. Thanks Laszlo