Re: [edk2-devel] [PATCH v2] OvmfPkg: Use DxeRuntimeCapsuleLib from DxeCapsuleLibFmp in X64 builds

["Tomas Pilar (tpilar)" <tpilar@solarflare.com>]

On 24/06/2019 22:28, Laszlo Ersek wrote:
> (+Mike)
>
> On 06/24/19 17:53, Tomas Pilar (tpilar) wrote:
>> Switching to this library enables capsule support for FMP devices.
>> This will allow testing of FMP for PCI devices using OVMF and PCI
>> passthrough as well as software parts of the FMP API.
>>
>> Simple tests show that a capsule with an embedded driver now
>> updates using CapsuleApp.
>>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Signed-off-by: Tomas Pilar <tpilar@solarflare.com>
>> ---
>>  OvmfPkg/OvmfPkgX64.dsc | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>> index 39ac791565..4c41e59a75 100644
>> --- a/OvmfPkg/OvmfPkgX64.dsc
>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>> @@ -125,7 +125,7 @@
>>    UefiBootManagerLib|MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf
>>    BootLogoLib|MdeModulePkg/Library/BootLogoLib/BootLogoLib.inf
>>    FileExplorerLib|MdeModulePkg/Library/FileExplorerLib/FileExplorerLib.inf
>> -  CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
>> +  CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeRuntimeCapsuleLib.inf
>>    DxeServicesLib|MdePkg/Library/DxeServicesLib/DxeServicesLib.inf
>>    DxeServicesTableLib|MdePkg/Library/DxeServicesTableLib/DxeServicesTableLib.inf
>>    PeCoffGetEntryPointLib|MdePkg/Library/BasePeCoffGetEntryPointLib/BasePeCoffGetEntryPointLib.inf
>>
> (I couldn't respond in time to the v1 posting, so I'm responding here.)
>
> (1) I'd like the commit message to be (even) more comprehensive. (Yes, I
> realize v2 is already an improvement in that direction, due to Ard's
> comments on v1.)
>
> In particular, I'd like to see
> "MdeModulePkg/Universal/CapsuleRuntimeDxe" being mentioned, as the
> implementation for the capsule runtime services, for which CapsuleLib
> provides the back-end.
>
> If there are other drivers affected, please list those as well (they can
> be collected from the OVMF build report file (--report-file=...). The
> pre-patch code was added in commit 49ba9447c92d ("Add initial version of
> Open Virtual Machine Firmware (OVMF) platform.", 2009-05-27), so this
> isn't exactly an oft-visited part of the DSC file(s) -- more explanation
> is welcome.
Best I can tell based on the report, only CapsuleRuntimeDxe consumes the
CapsuleLib in the Ovmf platform build.
>
> (2) I see this change as part of a much larger feature, "capsule
> support". Multiple people have expressed interest in that (Mike had even
> run some WIP patches by me earlier, off-list). Ultimately it would aim
> at updating the platform firmware flash too, from the inside. (Which in
> turn would require us to solve
> <https://bugzilla.tianocore.org/show_bug.cgi?id=386> as well -- but
> that's just a minuscule part of the whole.)
It is quite out of scope for me to try and solve the problem of platform flash update.
If the platform firmware publishes FMP instance then this library should Just Work
unless there is a problem with flash locking that requires capsule in memory processed
during SEC or PEI (correct me if I am wrong).
> If I'm mistaken in this regard (that is, regarding feature size), please
> correct me. If I'm right (or sort-of-right), then please make this lib
> class resolution dependent on a new build flag (such as CAPSULE_ENABLE
> or similar). The default value should be FALSE.
The size increase due to including this library over the Null library is 0x4c1000 -> 0x4c6000
for the DXEFV. Seems fairly trivial to me.
>
>
> (3) I think the separate build flag (default FALSE) is even more
> desirable because with capsule updates supported for add-on devices, you
> can screw up an assigned *physical* device for good, with a botched
> firmware update. That's a "feature" we shouldn't enable lightly.
I am not sure this is really necessary. If you configure your VM with PCI passthrough,
which requires you to correctly configure IOMMU and the host virtualization support
you are giving the VM the full, unqualified control over that device - that is what PCI
passthrough means. If that's the case, you can brick your device in many different
ways of which firmware update is just one.

Similarly, users performing a flash update already know all the dangers - do not turn
off the computer, do not do stupid things.

It seems somewhat unnecessary to include the extra flag that amounts to "If you give
the VM unqualified control over your device do you want the VM to be able to do a
firmware update".
>
>
> (4) All of the OvmfPkg/OvmfPkg*.dsc files should be modified in sync.
>
>
> Thanks
> Laszlo
>
> 
>