From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Mon, 24 Jun 2019 14:28:43 -0700 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 5B677308404B; Mon, 24 Jun 2019 21:28:42 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-226.ams2.redhat.com [10.36.116.226]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0618510013D9; Mon, 24 Jun 2019 21:28:38 +0000 (UTC) Subject: Re: [PATCH v2] OvmfPkg: Use DxeRuntimeCapsuleLib from DxeCapsuleLibFmp in X64 builds To: "Tomas Pilar (tpilar)" , Devel EDK2 Cc: "jordan.l.justen@intel.com" , Ard Biesheuvel , Michael Kinney References: <6ef09714-fd1f-2f7b-5a1d-fdf5e1a609fb@solarflare.com> From: "Laszlo Ersek" Message-ID: <5edb95f8-40bd-bf5e-3238-80d58855724e@redhat.com> Date: Mon, 24 Jun 2019 23:28:38 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <6ef09714-fd1f-2f7b-5a1d-fdf5e1a609fb@solarflare.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.40]); Mon, 24 Jun 2019 21:28:42 +0000 (UTC) Content-Type: text/plain; charset=windows-1252 Content-Language: en-US Content-Transfer-Encoding: 7bit (+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 > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Signed-off-by: Tomas Pilar > --- > 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 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