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

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

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


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

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.


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


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


Thanks
Laszlo