Laszlo, After discussed with Star, I understood OVMF's circle dependency on Variable 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 dynamic PCDs used by PiSmmCpuDxeSmm driver. Thanks! Jeff -----Original Message----- From: Zeng, Star Sent: Wednesday, August 24, 2016 9:42 PM To: Laszlo Ersek; Fan, Jeff; [email protected] Cc: Kinney, Michael D; Justen, Jordan L; Tian, Feng Subject: Re: [edk2] [Patch v5 48/48] UefiCpuPkg/PiSmmCpuDxeSmm: Add gEfiVariableArchProtocolGuid dependency [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. > <http://git.qemu.org/?p=qemu.git;a=commitdiff;h=f71e42a5c987 I am surprised by the read access prevention. > > > 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 >>> >> _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
