From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) (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 D184C1A1E2F for ; Wed, 24 Aug 2016 06:42:39 -0700 (PDT) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga105.jf.intel.com with ESMTP; 24 Aug 2016 06:42:39 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.28,570,1464678000"; d="scan'208";a="752930172" Received: from shzintpr01.sh.intel.com (HELO [10.253.24.29]) ([10.239.4.80]) by FMSMGA003.fm.intel.com with ESMTP; 24 Aug 2016 06:42:37 -0700 To: Laszlo Ersek , "Fan, Jeff" , "edk2-devel@ml01.01.org" References: <1470128388-17960-1-git-send-email-jeff.fan@intel.com> <1470128388-17960-49-git-send-email-jeff.fan@intel.com> <4f61b2b4-eeb4-8435-412f-20848347c88e@redhat.com> <542CF652F8836A4AB8DBFAAD40ED192A143D9CFD@shsmsx102.ccr.corp.intel.com> <795dd4fe-cd16-c0f1-7f04-f78601e2c7a8@redhat.com> <82c6b5c9-dcab-f3c1-5ffe-20fb27ddb1af@intel.com> <6cd44902-8c07-8f76-1204-d79c85559ae5@intel.com> <7d39ce29-88ce-c5f2-af61-7fd964acb5c0@intel.com> <801f810c-4cd7-d657-f536-a8471a37802c@redhat.com> Cc: "Kinney, Michael D" , "Jordan Justen (Intel address)" , "Tian, Feng" From: "Zeng, Star" Message-ID: <8c9e77d3-14d2-4a92-2407-eb80cd255bb4@intel.com> Date: Wed, 24 Aug 2016 21:42:07 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: Subject: Re: [Patch v5 48/48] UefiCpuPkg/PiSmmCpuDxeSmm: Add gEfiVariableArchProtocolGuid dependency X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 24 Aug 2016 13:42:40 -0000 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit [Snipped] >>>> >>>> I am not so clear about "the pristine "OVMF_VARS.fd" varstore template >>>> that falls right out of the OVMF build". >>>> >>>> Variable driver depends on PcdFlashNvStorageVariableBase(64) be set >>>> correctly to produce gEfiVariableArchProtocolGuid protocol. >>>> >>>> After PiSmmCpuDxeSmm adds gEfiVariableArchProtocolGuid dependency and >>>> FvbServicesSmm adds gEfiSmmCpuProtocolGuid dependency, there will be a >>>> dependency circle below. >>>> - PiSmmCpuDxeSmm depends on Variable driver to produce >>>> gEfiVariableArchProtocolGuid protocol. >>>> - FvbServicesSmm depends on PiSmmCpuDxeSmm to produce >>>> gEfiSmmCpuProtocolGuid protocol. >>>> - Variable driver depends on FvbServicesSmm to set >>>> PcdFlashNvStorageVariableBase(64) PCD. >>> >>> I understand, thank you for the explanation. The 64-bit PCD that you >>> mention is set at the end of the entry point function. >>> >>>> Are below approaches possible in OVMF? >>>> 1. Do InitializeVariableFvHeader () and >>>> PcdFlashNvStorageVariableBase(64) PCD set at PEI phase. >>> >>> InitializeVariableFvHeader() writes to the pflash chip, which is only >>> possible if the code runs in SMM (under the use case we are discussing >>> now). So running it as part of a PEIM would not be right. >>> >>>> 2. Do InitializeVariableFvHeader () and >>>> PcdFlashNvStorageVariableBase(64) PCD set in entrypoint with dependency >>>> = TRUE, and do other part of code in FvbInitialize() in another >>>> gEfiSmmCpuProtocolGuid notification function? >>> >>> That's again not good, because it would allow the entry point function >>> to exit with success (and to produce the FVB protocol interface) even if >>> the pflash configuration on the QEMU command line is incorrect (that is, >>> a -D SMM_REQUIRE OVMF build is being run with "-bios", rather than the >>> pflash chips). >> >> I am a little curious about what makes "writing to the pflash chip is >> only possible if the code runs in SMM" in OVMF? > > QEMU does that. > > That is the way QEMU protects the pflash chip from direct writes by the > guest OS. If you pass > > -global driver=cfi.pflash01,property=secure,value=on > > to QEMU (which we do), then all write accesses to the chip will be > rejected, unless the code doing the write access runs in SMM. > >> I meant the code flow for approach 2) I mentioned is like below, I am >> not sure if it works or not. :) >> >> FvbInitialize() - This may could be done at PEI phase. >> InitializeVariableFvHeader() >> if ValidateFvHeader () returns error status # mark1 >> allocate buffer to hold data from GetFvbInfo() # mark2 >> endif >> return PcdOvmfFlashNvStorageVariableBase or the buffer >> (allocated at mark2) pointer >> set PcdFlashNvStorageVariableBase64/ >> PcdFlashNvStorageFtwWorkingBase/ >> PcdFlashNvStorageFtwSpareBase >> according to the return from InitializeVariableFvHeader() >> >> FvbWriteInitialize() - Notification function on gEfiSmmCpuProtocolGuid >> QemuFlashInitialize() >> ... >> Free buffer(allocated at mark2) >> set PcdFlashNvStorageVariableBase64/ >> PcdFlashNvStorageFtwWorkingBase/ >> PcdFlashNvStorageFtwSpareBase >> again if mark1 returned success >> Install FVB protocol >> ... > > I don't understand the purpose of the buffer allocated at mark2. > > Also, currently InitializeVariableFvHeader() erases blocks, after it > prints "Variable FV header is not valid. It will be reinitialized". As I > said, that part cannot be done in PEI. > > .... Anyway, I just tried to set PcdFlashNvStorageVariableBase64 / > PcdFlashNvStorageFtwWorkingBase / PcdFlashNvStorageFtwSpareBase right in > the FDF file, at build time -- that is, even earlier than in PEI --, to > the correct values, as dynamic defaults. > > (I even verified those values in the build report file.) > > It doesn't work: I get the exact same assertion failure. In other words, > even if those PCDs are set to the correct values, VariableSmm blows up > with "Firmware Volume for Variable Store is corrupted". > > ..... Ahh, I realized why this happens! QEMU prevents even *read > accesses* if they don't come from code running in SMM. > > > Independently, I have further thoughts about the following: > >>> The "other part" that you mention is about exiting the FVB driver as >>> early as possible, if the varstore is not backed by a pflash chip: >>> - in the SMM_REQUIRE build, this is actually a fatal error, >>> - in the SMM-less build, this allows the EmuVariableFvbRuntimeDxe driver >>> to run. >>> >>> Let me turn the question around: can PiSmmCpuDxeSmm consume its settings >>> from HOBs? The PEI phase has read-only access to variables, and some >>> PEIM could produce those HOBs, either directly, or from reading >>> variables. (Also, it would be nice to know exactly what aspects of >>> PiSmmCpuDxeSmm are planned to be controlled dynamically.) >>> >>> Alternatively, if Jordan agrees, we could break the cycle like this: >>> >>> (1) Drop EmuVariableFvbRuntimeDxe and its dependencies completely. > > Unfortunately, this won't work -- I just realized it --, because Xen has > no pflash chips, and removing EmuVariableFvbRuntimeDxe would break OVMF > on Xen. > > Can we research the HOB (or dynamic PCD) avenue for PiSmmCpuDxeSmm a > bit? Because, it seems that on QEMU, *both* FvbServicesSmm *and* > VariableSmm can only be dispatched after PiSmmCpuDxeSmm enters SMM. > > In other words, if gEfiVariableArchProtocolGuid is added as a depex to > PiSmmCpuDxeSmm, then we have a much tighter cycle than you described > earlier, on QEMU: > > - PiSmmCpuDxeSmm wants read-only variable access, but > - VariableSmm must be dispatched in SMM (= after PiSmmCpuDxeSmm runs), > on QEMU. Seemingly, PiSmmCpuDxeSmm could not simply include gEfiVariableArchProtocolGuid dependency. I will have some discussion with Jeff. Thanks, Star > > In fact, this makes me wonder if PEI-phase read-only variable access is > possible at all on QEMU. (OVMF has never used > MdeModulePkg/Universal/Variable/Pei/.) > > Thanks > Laszlo > >>> This would eliminate OVMF's reuse of PcdFlashNvStorageVariableBase64 for >>> arbitrating between QemuFlashFvbServicesRuntimeDxe and >>> EmuVariableFvbRuntimeDxe. (For more background on this, please see >>> commits 4313b26de098b and 182eb4562731f.) >>> >>> Dropping EmuVariableFvbRuntimeDxe would also make pflash chips mandatory >>> for OVMF, and prevent it from running with the "-bios" option (at long >>> last). I'd strongly support this. >>> >>> (2) Set PcdFlashNvStorageVariableBase64 and the other PCDs that the >>> variable driver consumes directly in the OVMF DSC or FDF files, matching >>> the PcdOvmfFlashNvStorageXXX settings that >>> QemuFlashFvbServicesRuntimeDxe already uses as sorce of information. >>> This would allow the variable driver to proceed without >>> QemuFlashFvbServicesRuntimeDxe's assistance (for read-only access only). >>> This would make perfect sense, because for read-only access, the >>> variable driver doesn't need FVB, it only needs to know where to read >>> (with direct memory references). >>> >>> (3) QemuFlashFvbServicesRuntimeDxe would keep its structure in other >>> aspects; for example it would get a new depex on gEfiSmmCpuProtocolGuid. >>> This would ensure its entry point function was executed in SMM. >>> >>> Thanks >>> Laszlo >>> >>