Re: [Patch v5 48/48] UefiCpuPkg/PiSmmCpuDxeSmm: Add gEfiVariableArchProtocolGuid dependency

["Zeng, Star" <star.zeng@intel.com>]

[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
>>>
>>