From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) (using TLSv1 with cipher CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id A9C0B1A1E2B for ; Thu, 25 Aug 2016 01:00:34 -0700 (PDT) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga102.jf.intel.com with ESMTP; 25 Aug 2016 01:00:35 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.28,574,1464678000"; d="scan'208";a="870848993" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga003.jf.intel.com with ESMTP; 25 Aug 2016 01:00:32 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.248.2; Thu, 25 Aug 2016 01:00:30 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.147]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.107]) with mapi id 14.03.0248.002; Thu, 25 Aug 2016 16:00:28 +0800 From: "Fan, Jeff" To: "Zeng, Star" , Laszlo Ersek , "edk2-devel@ml01.01.org" CC: "Kinney, Michael D" , "Justen, Jordan L" , "Tian, Feng" Thread-Topic: [edk2] [Patch v5 48/48] UefiCpuPkg/PiSmmCpuDxeSmm: Add gEfiVariableArchProtocolGuid dependency Thread-Index: AQHR7Jy4Gz8Pd1vRVUq8pvPSUJUViKBPEBCAgACPpPD//4MCAIAABX8AgAADOQCABxy8AIAAudsAgAANcACAACBagIAAbTsAgAAePICAAbZq0A== Date: Thu, 25 Aug 2016 08:00:27 +0000 Message-ID: <542CF652F8836A4AB8DBFAAD40ED192A143DEF4E@shsmsx102.ccr.corp.intel.com> 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> <8c9e77d3-14d2-4a92-2407-eb80cd255bb4@intel.com> In-Reply-To: <8c9e77d3-14d2-4a92-2407-eb80cd255bb4@intel.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYzFjY2FkNjctNTE1Ny00ZmE4LWI5YzgtMmZkMzU1MGI4OGRlIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6Ik5YREpQTWp6eTlyVGUrN2lBaXBkMjlDVGg0Q2cxbDRiaHJZNlNjR1wvR1ZNPSJ9 x-ctpclassification: CTP_IC x-originating-ip: [10.239.127.40] MIME-Version: 1.0 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: Thu, 25 Aug 2016 08:00:34 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Laszlo, After discussed with Star, I understood OVMF's circle dependency on Variabl= e Arch protocol and SMM CPU driver. I will defer to check-in this patch till found the better solution. Before the proper fix adopted, our platforms might not set the HII types dy= namic PCDs used by PiSmmCpuDxeSmm driver. Thanks! Jeff -----Original Message----- From: Zeng, Star=20 Sent: Wednesday, August 24, 2016 9:42 PM To: Laszlo Ersek; Fan, Jeff; edk2-devel@ml01.01.org Cc: Kinney, Michael D; Justen, Jordan L; Tian, Feng Subject: Re: [edk2] [Patch v5 48/48] UefiCpuPkg/PiSmmCpuDxeSmm: Add gEfiVar= iableArchProtocolGuid dependency [Snipped] >>>> >>>> I am not so clear about "the pristine "OVMF_VARS.fd" varstore=20 >>>> template that falls right out of the OVMF build". >>>> >>>> Variable driver depends on PcdFlashNvStorageVariableBase(64) be set=20 >>>> correctly to produce gEfiVariableArchProtocolGuid protocol. >>>> >>>> After PiSmmCpuDxeSmm adds gEfiVariableArchProtocolGuid dependency=20 >>>> and FvbServicesSmm adds gEfiSmmCpuProtocolGuid dependency, there=20 >>>> will be a dependency circle below. >>>> - PiSmmCpuDxeSmm depends on Variable driver to produce=20 >>>> gEfiVariableArchProtocolGuid protocol. >>>> - FvbServicesSmm depends on PiSmmCpuDxeSmm to produce=20 >>>> 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=20 >>> 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=20 >>> only possible if the code runs in SMM (under the use case we are=20 >>> 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=20 >>>> dependency =3D TRUE, and do other part of code in FvbInitialize() in=20 >>>> another gEfiSmmCpuProtocolGuid notification function? >>> >>> That's again not good, because it would allow the entry point=20 >>> function to exit with success (and to produce the FVB protocol=20 >>> interface) even if the pflash configuration on the QEMU command line=20 >>> is incorrect (that is, a -D SMM_REQUIRE OVMF build is being run with=20 >>> "-bios", rather than the pflash chips). >> >> I am a little curious about what makes "writing to the pflash chip is=20 >> 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=20 > the guest OS. If you pass > > -global driver=3Dcfi.pflash01,property=3Dsecure,value=3Don > > to QEMU (which we do), then all write accesses to the chip will be=20 > 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=20 >> 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=20 > prints "Variable FV header is not valid. It will be reinitialized". As=20 > I said, that part cannot be done in PEI. > > .... Anyway, I just tried to set PcdFlashNvStorageVariableBase64 /=20 > PcdFlashNvStorageFtwWorkingBase / PcdFlashNvStorageFtwSpareBase right=20 > in the FDF file, at build time -- that is, even earlier than in PEI=20 > --, 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=20 > words, even if those PCDs are set to the correct values, VariableSmm=20 > 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=20 >>> 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=20 >>> driver to run. >>> >>> Let me turn the question around: can PiSmmCpuDxeSmm consume its=20 >>> settings from HOBs? The PEI phase has read-only access to variables,=20 >>> and some PEIM could produce those HOBs, either directly, or from=20 >>> reading variables. (Also, it would be nice to know exactly what=20 >>> 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=20 > has no pflash chips, and removing EmuVariableFvbRuntimeDxe would break=20 > OVMF on Xen. > > Can we research the HOB (or dynamic PCD) avenue for PiSmmCpuDxeSmm a=20 > bit? Because, it seems that on QEMU, *both* FvbServicesSmm *and*=20 > VariableSmm can only be dispatched after PiSmmCpuDxeSmm enters SMM. > > In other words, if gEfiVariableArchProtocolGuid is added as a depex to=20 > PiSmmCpuDxeSmm, then we have a much tighter cycle than you described=20 > earlier, on QEMU: > > - PiSmmCpuDxeSmm wants read-only variable access, but > - VariableSmm must be dispatched in SMM (=3D after PiSmmCpuDxeSmm runs),= =20 > on QEMU. Seemingly, PiSmmCpuDxeSmm could not simply include gEfiVariableArchProtocol= Guid dependency. I will have some discussion with Jeff. Thanks, Star > > In fact, this makes me wonder if PEI-phase read-only variable access=20 > is possible at all on QEMU. (OVMF has never used > MdeModulePkg/Universal/Variable/Pei/.) > > Thanks > Laszlo > >>> This would eliminate OVMF's reuse of PcdFlashNvStorageVariableBase64=20 >>> for arbitrating between QemuFlashFvbServicesRuntimeDxe and=20 >>> EmuVariableFvbRuntimeDxe. (For more background on this, please see=20 >>> commits 4313b26de098b and 182eb4562731f.) >>> >>> Dropping EmuVariableFvbRuntimeDxe would also make pflash chips=20 >>> mandatory for OVMF, and prevent it from running with the "-bios"=20 >>> option (at long last). I'd strongly support this. >>> >>> (2) Set PcdFlashNvStorageVariableBase64 and the other PCDs that the=20 >>> variable driver consumes directly in the OVMF DSC or FDF files,=20 >>> matching the PcdOvmfFlashNvStorageXXX settings that=20 >>> QemuFlashFvbServicesRuntimeDxe already uses as sorce of information. >>> This would allow the variable driver to proceed without=20 >>> QemuFlashFvbServicesRuntimeDxe's assistance (for read-only access only)= . >>> This would make perfect sense, because for read-only access, the=20 >>> variable driver doesn't need FVB, it only needs to know where to=20 >>> read (with direct memory references). >>> >>> (3) QemuFlashFvbServicesRuntimeDxe would keep its structure in other=20 >>> aspects; for example it would get a new depex on gEfiSmmCpuProtocolGuid= . >>> This would ensure its entry point function was executed in SMM. >>> >>> Thanks >>> Laszlo >>> >>