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 CD56321962301 for ; Thu, 13 Sep 2018 06:15:40 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 524E53082E64; Thu, 13 Sep 2018 13:15:40 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-156.rdu2.redhat.com [10.10.120.156]) by smtp.corp.redhat.com (Postfix) with ESMTP id 3558A600C6; Thu, 13 Sep 2018 13:15:39 +0000 (UTC) To: Nikita Leshenko Cc: Liran Alon , edk2-devel@lists.01.org, ard.biesheuvel@linaro.org References: <2fdb6059-86a4-a8d5-a46c-286c62e17864@redhat.com> <8A63AC46-22FF-480A-A109-9902B7076464@oracle.com> From: Laszlo Ersek Message-ID: Date: Thu, 13 Sep 2018 15:15:38 +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: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.46]); Thu, 13 Sep 2018 13:15:40 +0000 (UTC) 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: Thu, 13 Sep 2018 13:15:41 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 09/13/18 14:27, Nikita Leshenko wrote: > > >> On 11 Sep 2018, at 15:34, Laszlo Ersek wrote: >> >> "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 for the detailed explanation. I guess we have no choice > but to copy BaseCachingPciExpressLib (renamed to > DxePciExpressLibCaching) from ArmVirt to OVMF as well. Uhh, hold on for a minute :) , "no choice but" is a bit of a stretch. Where does OVMF enter the picture to begin with? Between OvmfPkg and ArmVirtPkg, we have a sort-of rule that ArmVirtPkg is allowed to consume OvmfPkg content, but not vice versa. So, we could move (and rename) the lib instance from ArmVirtPkg to OvmfPkg, update the reference(s) in ArmVirtPkg accordingly, and then consume the lib instance in OvmfPkg as well... *if* there was any reason to consume the lib instance in OvmfPkg in the first place. (Of course if, by "we", you meant an internal Oracle use case, then I'm not trying to pry. I took "we" as "upstream community". Emails are ambiguous! :) ) > In order to prevent such bugs in the future, what do you think > about changing the PCD reference in BasePciExpressLib.inf (in > MdePkg) from [Pcd] to [FixedPcd]? This way a module that has a > dynamic PcdPciExpressBaseAddress will fail to link with the > default PCIE library. Is it practical to get such change into > MdePkg despite their strict change policy? I certainly think that's a valid approach if the PCD can only be Fixed by design. However, judging by Ray's (as yet unanswered) followup question in the thread: http://mid.mail-archive.com/734D49CCEBEEF84792F5B80ED585239D5BDF99C7@SHSMSX104.ccr.corp.intel.com I believe "by design" might not be a done deal just yet. Perhaps BasePciExpressLib can be fixed to work with a dynamic PCD (and then we should drop BaseCachingPciExpressLib in ArmVirtPkg too, and consume the fixed BasePciExpressLib instance). I suggest that you please file a bug for MdeModulePkg / BasePciExpressLib in the TianoCore Bugzilla , referencing this discussion from the mailing list archive , and request that (a) either the dynamic PCD use case be fixed, or (b) the code own up to the restriction and be explicit about FixedPCD, both in the INF file(s) and the C-language APIs. Thanks! Laszlo