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

["Laszlo Ersek" <lersek@redhat.com>]

On 07/03/19 13:31, Tomas Pilar (tpilar) wrote:
> 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.

OK, thanks. So please name CapsuleRuntimeDxe, and the runtime service(s)
it's responsible for.


>> (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.

Sorry, I must have been unclear. First, I definitely don't suggest that
you take on platform flash update. Second, I wasn't concerned about an
increase in the firmware binary size.

I should have written "scope", rather than "size". So, to clarify, I see
this feature fall under the same larger scope as "platform  flash
update", and that scope is large enough to deserve a new "-D" flag, even
if the current change is just a tiny sub-feature of that scope.


>> (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.

I'm not sure I agree with you. For sake of discussion, just remove the
entire VM / device assignment concept from the picture -- assume a card
is simply plugged into a normal physical system, and a user runs a
normal OS on the physical system.

Do we really think that the user can brick their device in many
different ways (just by virtue of this run-of-the-mill physical setup),
of which firmware update is just one way? I'd opine a physical system
user would never brick their card, *unless* they attempted a firmware
upgrade on it.


> 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".

I don't have evidence that you are wrong, and you could even be right,
in a strict technical sense. However, "vectors" (avenues for arriving at
the same thing) matter. In my opinion, it is a lot harder for a user to
unintentionally shoot themselves in the foot if this feature is off by
default.

Thanks
Laszlo

>> (4) All of the OvmfPkg/OvmfPkg*.dsc files should be modified in sync.