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

["Fan, Jeff" <jeff.fan@intel.com>]

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; edk2-devel@ml01.01.org
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
>>>
>>