From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (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 BE5FD21A07A80 for ; Tue, 11 Sep 2018 06:34:21 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E713E4075166; Tue, 11 Sep 2018 13:34:20 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-36.rdu2.redhat.com [10.10.120.36]) by smtp.corp.redhat.com (Postfix) with ESMTP id 2565E2156701; Tue, 11 Sep 2018 13:34:19 +0000 (UTC) To: Liran Alon Cc: edk2-devel@lists.01.org, Nikita Leshenko References: <2fdb6059-86a4-a8d5-a46c-286c62e17864@redhat.com> <8A63AC46-22FF-480A-A109-9902B7076464@oracle.com> From: Laszlo Ersek Message-ID: Date: Tue, 11 Sep 2018 15:34:19 +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: <8A63AC46-22FF-480A-A109-9902B7076464@oracle.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.6 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Tue, 11 Sep 2018 13:34:20 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Tue, 11 Sep 2018 13:34:20 +0000 (UTC) for IP:'10.11.54.6' DOMAIN:'int-mx06.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: PciBusDxe: PCI-Express bug with dynamic PcdPciExpressBaseAddress 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: Tue, 11 Sep 2018 13:34:23 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit On 09/07/18 19:01, Liran Alon wrote: > > >> On 7 Sep 2018, at 11:44, Laszlo Ersek wrote: >> >> (+Ard) >> >> On 09/06/18 21:08, Nikita Leshenko wrote: >>> Hi, >>> >>> We ran into a bug in EDK2 relating to PCI-Express in PciBusDxe. Here's the flow >>> of the bug: >>> >>> 1. PciBusDxe/PciEnumeratorSupport.c: Function BarExisted probes a BAR. It raises >>> TPL to TPL_HIGH_LEVEL to avoid timer interrupts while probing the BAR and >>> calls PciIo->Pci.Write. >>> 2. BasePciExpressLib/PciExpressLib.c: The write reaches PciExpressWrite32, which >>> calls GetPciExpressBaseAddress. >>> 3. GetPciExpressBaseAddress retrieves the address from PcdPciExpressBaseAddress. >>> 4. Reading the PCD calls DxePcdGet64 -> GetWorker -> >>> EfiAcquireLock(&mPcdDatabaseLock), which is at TPL_NOTIFY level. This crashes >>> the firmware because step 1 raised the TPL to TPL_HIGH_LEVEL. >>> >>> This doesn't happen when PcdPciExpressBaseAddress is fixed at build (because >>> then the read is optimized to a static global variable), but when the PCD is >>> dynamic PCI-Express is broken. >>> >>> Does anybody have a suggestion for fixing it? >>> >>> Options we thought about: >>> - Change mPcdDatabaseLock.Tpl to TPL_HIGH_LEVEL >>> - Don't use a PCD for the base address, put it in a static global variable and >>> create functions to set and retrieve it. >> >> In the ArmVirtPkg platforms, we also set "PcdPciExpressBaseAddress" >> dynamically. And, we implemented your second option above; see: >> >> ArmVirtPkg/Library/BaseCachingPciExpressLib/ >> >> Relevant commits: >> >> - ad3359eb43a9 ("ArmVirtualizationPkg: clone BasePciExpressLib, cache >> PCIe config base", 2015-02-23) >> - a06d0bb58eb9 ("ArmVirtPkg/BaseCachingPciExpressLib: depend on >> PciPcdProducerLib", 2016-04-12) >> >> (In fact, commit ad3359eb43a9 documents the exact issue you report here.) >> >> Thanks >> Laszlo > > Thanks Lazlo. Weren’t aware of this solution in the ArmVirtPkg platforms. > > However, I wonder why the solution was to clone the MdePkg/Library/BasePciExpressLib rather than > change the original library itself? > > That is, what was the reason for not just adding a library constructor to MdePkg/Library/BasePciExpressLib > to cache PcdPciExpressBaseAddress in a global var? This seem to solve the issues for all platforms. There are two reasons, one related to project governance, and another technical. (1) The reason related to project management is that modifying core edk2 libraries and drivers (such that live in MdePkg and MdeModulePkg) is usually difficult. The requirement that the code be correct and pass technical review is the easy part; the hard part is that you can sell your use case enough for your change to be accepted for MdePkg / MdeModulePkg. I'm pretty unhappy about this state of affairs myself, but it is what it is. (I do sympathize with the MdePkg / MdeModulePkg maintainers as well, though! Their responsibility is big.) So, in many cases, it is a lot easier to enable your platform by cloning a library instance and modifying it. After all, that's what library *classes* were invented for -- to have multiple instances, accommodating different platforms. (2) The technical reason is that the library instance in question is called "BasePciExpressLib". Given a library class called "FoobarLib", an instance that implements the class is usually named - with a prefix that identifies the firmware phase / module type that the lib instance (= implementation) targets, i.e. those that the lib instance is usable within, - with an optional suffix that hints at the implementation details / behavior of the library instance. "BasePciExpressLib" has the prefix "Base", meaning that it is supposed to be usable in all types of firmware modules, even in SEC and PEIMs -- which may not have access to writeable memory except stack (i.e. writeable global variables). Therefore modifying the original lib instance so that it depend on writeable globals wouldn't have been right. We could have attempted to add the new library instance under MdePkg, and not ArmVirtPkg, but that's just a (more difficult) special case of point (1). (Obviously if you try to apply the nomenclature I describe above to "BaseCachingPciExpressLib" as well, you'll see that it doesn't match. And that's because, when I invented the name for that lib instance, in 2015, I didn't know about the naming rules myself. :) In reality the lib instance should be called "DxePciExpressLibCaching" -- with "Dxe" for prefix, and "Caching" for suffix.) Thanks Laszlo > If PcdPciExpressBaseAddress is fixed and doesn’t change dynamically, then the caching of the value in a global var > should be harmless (besides adding an extra read from global-var). If it does change dynamically, > MdePkg/Library/BasePciExpressLib have the issue discussed in this email thread. > > Thanks, > -Liran