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 A9E68202E534A for ; Fri, 21 Sep 2018 04:06:15 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.25]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1FF3B3091D54; Fri, 21 Sep 2018 11:06:15 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-8.rdu2.redhat.com [10.10.120.8]) by smtp.corp.redhat.com (Postfix) with ESMTP id EFE6F2010D95; Fri, 21 Sep 2018 11:06:13 +0000 (UTC) To: Ruiyu Ni , edk2-devel@lists.01.org Cc: Garrett , Star Zeng , Kirkendall@ml01.01.org References: <20180921072539.268068-1-ruiyu.ni@intel.com> <20180921072539.268068-3-ruiyu.ni@intel.com> From: Laszlo Ersek Message-ID: <95b75c62-43e6-e92e-3de8-04e3fdd9cd7b@redhat.com> Date: Fri, 21 Sep 2018 13:06:12 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20180921072539.268068-3-ruiyu.ni@intel.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.25 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.42]); Fri, 21 Sep 2018 11:06:15 +0000 (UTC) Subject: Re: [PATCH 2/3] MdeModulePkg/PciHostBridge: Fix a bug that prevents PMEM access X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 21 Sep 2018 11:06:15 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 09/21/18 09:25, Ruiyu Ni wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1196 > > RootBridgeIoCheckParameter() verifies that the requested MMIO access > can fit in any of the MEM/PMEM 32/64 ranges. But today's logic > somehow only checks the requested access against MEM 32/64 ranges. > > It should also check the requested access against PMEM 32/64 ranges. > > The patch fixes this issue. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ruiyu Ni > Cc: Star Zeng > Cc: Kirkendall, Garrett > --- > MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c > index 0b6b56f846..f6234b5d11 100644 > --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c > +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c > @@ -411,12 +411,18 @@ RootBridgeIoCheckParameter ( > // By comparing the Address against Limit we know which range to be used > // for checking > // > - if (Address + Length <= RootBridge->Mem.Limit + 1) { > - Base = RootBridge->Mem.Base; > + if ((Address >= RootBridge->Mem.Base) && (Address + Length <= RootBridge->Mem.Limit + 1)) { > + Base = RootBridge->Mem.Base; > Limit = RootBridge->Mem.Limit; > - } else { > - Base = RootBridge->MemAbove4G.Base; > + } else if ((Address >= RootBridge->PMem.Base) && (Address + Length <= RootBridge->PMem.Limit + 1)) { > + Base = RootBridge->PMem.Base; > + Limit = RootBridge->PMem.Limit; > + } else if ((Address >= RootBridge->MemAbove4G.Base) && (Address + Length <= RootBridge->MemAbove4G.Limit + 1)) { > + Base = RootBridge->MemAbove4G.Base; > Limit = RootBridge->MemAbove4G.Limit; > + } else { > + Base = RootBridge->PMemAbove4G.Base; > + Limit = RootBridge->PMemAbove4G.Limit; > } > } else { > PciRbAddr = (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS*) &Address; > The interesting thing about this patch is that, if any one of the first three branches is taken, then the final checks will automatically pass. That's because, on the first three branches, we select the base & the limit *because* the access falls between them. Therefore, in the end, when we check whether the access falls between base and end, they miraculously happen to do so. :) Reviewed-by: Laszlo Ersek Thanks Laszlo