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; Wed, 03 Jul 2019 07:46:12 -0700 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id DE3F230C31A1; Wed, 3 Jul 2019 14:46:03 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-114.ams2.redhat.com [10.36.116.114]) by smtp.corp.redhat.com (Postfix) with ESMTP id D4A0878907; Wed, 3 Jul 2019 14:46:00 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2] OvmfPkg: Use DxeRuntimeCapsuleLib from DxeCapsuleLibFmp in X64 builds To: "Tomas Pilar (tpilar)" , devel@edk2.groups.io Cc: "jordan.l.justen@intel.com" , Ard Biesheuvel , Michael Kinney References: <6ef09714-fd1f-2f7b-5a1d-fdf5e1a609fb@solarflare.com> <5edb95f8-40bd-bf5e-3238-80d58855724e@redhat.com> <1a255a93-7d7b-b729-2ab8-a50f913a2c5a@solarflare.com> From: "Laszlo Ersek" Message-ID: <7fc62bca-124a-421c-84ee-ee93ba9c6eaa@redhat.com> Date: Wed, 3 Jul 2019 16:45:59 +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: <1a255a93-7d7b-b729-2ab8-a50f913a2c5a@solarflare.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.40]); Wed, 03 Jul 2019 14:46:05 +0000 (UTC) Content-Type: text/plain; charset=windows-1252 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 >>> 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. > 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 >> 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.