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

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

Fair enough, I'll spin a new patch.

-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com> 
Sent: 03 July 2019 15:46
To: Tomas Pilar <tpilar@solarflare.com>; Devel EDK2 <devel@edk2.groups.io>
Cc: jordan.l.justen@intel.com; Ard Biesheuvel <ard.biesheuvel@linaro.org>; Michael Kinney <michael.d.kinney@intel.com>
Subject: Re: [edk2-devel] [PATCH v2] OvmfPkg: Use DxeRuntimeCapsuleLib from DxeCapsuleLibFmp in X64 builds

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/DxeRuntimeCapsule
>>> + Lib.inf
>>>    DxeServicesLib|MdePkg/Library/DxeServicesLib/DxeServicesLib.inf
>>>    DxeServicesTableLib|MdePkg/Library/DxeServicesTableLib/DxeServicesTableLib.inf
>>>    
>>> PeCoffGetEntryPointLib|MdePkg/Library/BasePeCoffGetEntryPointLib/Bas
>>> ePeCoffGetEntryPointLib.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.