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 9520920352A8A for ; Fri, 1 Dec 2017 03:37:23 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 7A1CA61479; Fri, 1 Dec 2017 11:41:49 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-33.rdu2.redhat.com [10.10.120.33]) by smtp.corp.redhat.com (Postfix) with ESMTP id 5DE3F5D9C8; Fri, 1 Dec 2017 11:41:48 +0000 (UTC) To: Ard Biesheuvel Cc: Anthony Perard , Jordan Justen , edk2-devel-01 References: <20171130163029.19743-1-lersek@redhat.com> <20171130163029.19743-4-lersek@redhat.com> From: Laszlo Ersek Message-ID: Date: Fri, 1 Dec 2017 12:41:47 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Fri, 01 Dec 2017 11:41:49 +0000 (UTC) Subject: Re: [PATCH v2 3/8] OvmfPkg: conditionally disable reserved memory varstore emulation at build 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, 01 Dec 2017 11:37:23 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 12/01/17 11:53, Ard Biesheuvel wrote: > On 1 December 2017 at 10:52, Laszlo Ersek wrote: >> On 12/01/17 09:51, Ard Biesheuvel wrote: >>> On 30 November 2017 at 16:30, Laszlo Ersek wrote: >>>> (All of the below is only relevant for SMM_REQUIRE=FALSE.) >>>> >>>> For the emulated variable store, PlatformPei allocates reserved memory (as >>>> early as possible, so that the address remains the same during reboot), >>>> and PcdEmuVariableNvStoreReserved carries the address to >>>> EmuVariableFvbRuntimeDxe. >>>> >>>> In addition, QemuFlashFvbServicesRuntimeDxe is always launched before >>>> EmuVariableFvbRuntimeDxe, so that if flash variables are available, >>>> QemuFlashFvbServicesRuntimeDxe can set PcdFlashNvStorageVariableBase64 >>>> first, and EmuVariableFvbRuntimeDxe can exit early. This ordering is >>>> currently enforced by adding QemuFlashFvbServicesRuntimeDxe to the APRIORI >>>> DXE file. >>>> >>>> All of this is unnecessary when MEM_VARSTORE_EMU_ENABLE is set to FALSE. >>>> In such a build, >>>> >>>> - (almost) remove the dynamic default for PcdEmuVariableNvStoreReserved >>>> (we can't really do this because the PcdSet64() in >>>> ReserveEmuVariableNvStore() wouldn't compile), >>>> >>> >>> If that is the only concern, and the value is irrelevant, you could >>> make it a patchable PCD instead >> >> This sounds interesting; I've never used patchable PCDs. Can you please >> elaborate? >> >> Do you mean that for (SMM_REQUIRE==TRUE || >> MEM_VARSTORE_EMU_ENABLE==FALSE), the DSC file should list >> PcdEmuVariableNvStoreReserved somewhere else? >> > > Yes. > >> Does PcdSet work on patchable PCDs? >> > > Yes. It's a bit icky because the value does not actually propagate to > other modules, but that doesn't matter in this case. Hm, this sounds like a recipe. We have two sets of affected PCDs: - PcdEmuVariableNvStoreReserved --> in the "flash required" builds, make it patchable, don't bother setting a good value at build time, and the PcdSet calls will compile (but won't ever be reached due to code logic) - PcdFlashNvStorageVariableBase64, PcdFlashNvStorageFtwWorkingBase, PcdFlashNvStorageFtwSpareBase: in the "flash required" builds, make them patchable, *try* to set the right default values at build time, with SET commands, and the PcdSet calls will compile (but won't ever be reached due to code logic) That could let us drop FlashNvStorageAddressLib. (On patch #6, only the commit message would change.) Thanks, Laszlo