From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: solarflare.com, ip: 148.163.129.52, mailfrom: tpilar@solarflare.com) Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [148.163.129.52]) by groups.io with SMTP; Wed, 03 Jul 2019 08:28:44 -0700 X-Virus-Scanned: Proofpoint Essentials engine Received: from webmail.solarflare.com (uk.solarflare.com [193.34.186.16]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by mx1-us1.ppe-hosted.com (PPE Hosted ESMTP Server) with ESMTPS id EBB31940060; Wed, 3 Jul 2019 15:28:42 +0000 (UTC) Received: from ukex01.SolarFlarecom.com (10.17.10.4) by ukex01.SolarFlarecom.com (10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Wed, 3 Jul 2019 16:28:35 +0100 Received: from ukex01.SolarFlarecom.com ([fe80::40de:58ad:e2bb:6942]) by ukex01.SolarFlarecom.com ([fe80::40de:58ad:e2bb:6942%14]) with mapi id 15.00.1395.000; Wed, 3 Jul 2019 16:28:35 +0100 From: "Tomas Pilar (tpilar)" To: Laszlo Ersek , Devel EDK2 CC: "jordan.l.justen@intel.com" , Ard Biesheuvel , Michael Kinney Subject: Re: [edk2-devel] [PATCH v2] OvmfPkg: Use DxeRuntimeCapsuleLib from DxeCapsuleLibFmp in X64 builds Thread-Topic: [edk2-devel] [PATCH v2] OvmfPkg: Use DxeRuntimeCapsuleLib from DxeCapsuleLibFmp in X64 builds Thread-Index: AQHVKqUCpHRIy8ihbkq+LeDvyFUmaaarx6EAgA0IvYCAACWigIAAHJQw Date: Wed, 3 Jul 2019 15:28:34 +0000 Message-ID: <39ab9b5fa7ca4de882e4419a98d75f41@ukex01.SolarFlarecom.com> References: <6ef09714-fd1f-2f7b-5a1d-fdf5e1a609fb@solarflare.com> <5edb95f8-40bd-bf5e-3238-80d58855724e@redhat.com> <1a255a93-7d7b-b729-2ab8-a50f913a2c5a@solarflare.com> <7fc62bca-124a-421c-84ee-ee93ba9c6eaa@redhat.com> In-Reply-To: <7fc62bca-124a-421c-84ee-ee93ba9c6eaa@redhat.com> Accept-Language: en-GB, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [10.17.25.143] x-tm-as-product-ver: SMEX-12.5.0.1300-8.5.1010-24736.003 x-tm-as-result: No-22.009300-8.000000-10 x-tmase-matchedrid: 8HTFlOrbAtEikhyG+/kfahvgzEPRJaDEWw/S0HB7eoMTiSW9r3PknEfj d+aXilyP4YpJxB0tJOmKiPY+pyEfRXx8EDQqTtoD+LORUvwlJdvDcG244XPgEcWdygDqK8LXZNK ZtQKCl+eXxXpkoQkDT0aHaPfxl4UYPg9yfk4YC0bn11MLDEWdofLfqX4QTDm8cPafKCoXYH/G2o CtWQTpF3lL7yhd9GmgwcCje8eOa7qg437Gg9fTIS/MuWzsdN8ZSWg+u4ir2NM7e2YyTwFSIlMe5 Blkpry7rdoLblq9S5pX0/uHm06FyHwKvvjL1fDqolVO7uyOCDUIOKDcZUYipFc/CedjlcvkPDAD fLdL5BoAp53fkkmWxN6bmObKUOT9AD+KppHhFfAYteHAndhXo+xvW9tsEBVzfyjNXlbBvnRIeBR Uygi/YVGrOrjJwonApvrTBFZ2EvGBANQIBoQOqW6HurDH4PpPqV3VmuIFNEuEWB8iuyGqCYOebE P+AgB4GgTuoj2pdm04LxgJSVfCdRKovmG69iNrcFe8RUQnnIcSUsfu0Jv3Su9FCyScBaYa9VlGB jCDnchCifD1DIfHmcanDVrxAGqxxOu/L8NfUw+dVNZaI2n6/+/kl6iUzyK7dz3bnI4leYW/6/cy CTWmI0uTYzkZpCwjZ6QuuqcA0dYPkUPAqVjHpiXTnAZkhKfl6e1dE1dnasnkMnUVL5d0E0WmZqY rA0q9dXhkBccop9uIvMccV+eyN+TSNUWBxp8VIAjxomarSPDqobkz1A0A7ey9vsxhLmzegG8l8V uw4XbQRc3h7tg2If523T32Fq9yg3ZnjImxGAeeAiCmPx4NwFkMvWAuahr8+gD2vYtOFhgqtq5d3 cxkNQwWxr7XDKH8lExlQIQeRG0= x-tm-as-user-approved-sender: Yes x-tm-as-user-blocked-sender: No x-tmase-result: 10--22.009300-8.000000 x-tmase-version: SMEX-12.5.0.1300-8.5.1010-24736.003 MIME-Version: 1.0 X-MDID: 1562167724-oFYgATs4x5Ux Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Fair enough, I'll spin a new patch. -----Original Message----- From: Laszlo Ersek =20 Sent: 03 July 2019 15:46 To: Tomas Pilar ; Devel EDK2 Cc: jordan.l.justen@intel.com; Ard Biesheuvel ; = Michael Kinney 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=20 >>> passthrough as well as software parts of the FMP API. >>> >>> Simple tests show that a capsule with an embedded driver now updates=20 >>> 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=20 >>> 39ac791565..4c41e59a75 100644 >>> --- a/OvmfPkg/OvmfPkgX64.dsc >>> +++ b/OvmfPkg/OvmfPkgX64.dsc >>> @@ -125,7 +125,7 @@ >>> UefiBootManagerLib|MdeModulePkg/Library/UefiBootManagerLib/UefiBootM= anagerLib.inf >>> BootLogoLib|MdeModulePkg/Library/BootLogoLib/BootLogoLib.inf >>> =20 >>> FileExplorerLib|MdeModulePkg/Library/FileExplorerLib/FileExplorerLib >>> .inf >>> - =20 >>> CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull. >>> inf >>> + =20 >>> + CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeRuntimeCapsule >>> + Lib.inf >>> DxeServicesLib|MdePkg/Library/DxeServicesLib/DxeServicesLib.inf >>> DxeServicesTableLib|MdePkg/Library/DxeServicesTableLib/DxeServicesTa= bleLib.inf >>> =20 >>> PeCoffGetEntryPointLib|MdePkg/Library/BasePeCoffGetEntryPointLib/Bas >>> ePeCoffGetEntryPointLib.inf >>> >> (I couldn't respond in time to the v1 posting, so I'm responding=20 >> here.) >> >> (1) I'd like the commit message to be (even) more comprehensive.=20 >> (Yes, I realize v2 is already an improvement in that direction, due=20 >> to Ard's comments on v1.) >> >> In particular, I'd like to see >> "MdeModulePkg/Universal/CapsuleRuntimeDxe" being mentioned, as the=20 >> implementation for the capsule runtime services, for which CapsuleLib=20 >> provides the back-end. >> >> If there are other drivers affected, please list those as well (they=20 >> can be collected from the OVMF build report file (--report-file=3D...).= =20 >> The pre-patch code was added in commit 49ba9447c92d ("Add initial=20 >> version of Open Virtual Machine Firmware (OVMF) platform.",=20 >> 2009-05-27), so this isn't exactly an oft-visited part of the DSC=20 >> file(s) -- more explanation is welcome. > Best I can tell based on the report, only CapsuleRuntimeDxe consumes=20 > 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=20 >> support". Multiple people have expressed interest in that (Mike had=20 >> even run some WIP patches by me earlier, off-list). Ultimately it=20 >> would aim at updating the platform firmware flash too, from the=20 >> inside. (Which in turn would require us to solve=20 >> as well -- but=20 >> 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=20 > should Just Work unless there is a problem with flash locking that=20 > requires capsule in memory processed during SEC or PEI (correct me if I a= m wrong). >> If I'm mistaken in this regard (that is, regarding feature size),=20 >> please correct me. If I'm right (or sort-of-right), then please make=20 >> this lib class resolution dependent on a new build flag (such as=20 >> CAPSULE_ENABLE or similar). The default value should be FALSE. > The size increase due to including this library over the Null library=20 > 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 increas= e in the firmware binary size. I should have written "scope", rather than "size". So, to clarify, I see th= is feature fall under the same larger scope as "platform flash update", an= d that scope is large enough to deserve a new "-D" flag, even if the curren= t change is just a tiny sub-feature of that scope. >> (3) I think the separate build flag (default FALSE) is even more=20 >> desirable because with capsule updates supported for add-on devices,=20 >> you can screw up an assigned *physical* device for good, with a=20 >> 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=20 > PCI passthrough, which requires you to correctly configure IOMMU and=20 > the host virtualization support you are giving the VM the full,=20 > unqualified control over that device - that is what PCI passthrough=20 > means. If that's the case, you can brick your device in many different wa= ys of which firmware update is just one. I'm not sure I agree with you. For sake of discussion, just remove the enti= re VM / device assignment concept from the picture -- assume a card is simp= ly plugged into a normal physical system, and a user runs a normal OS on th= e physical system. Do we really think that the user can brick their device in many different w= ays (just by virtue of this run-of-the-mill physical setup), of which firmw= are update is just one way? I'd opine a physical system user would never br= ick their card, *unless* they attempted a firmware upgrade on it. > Similarly, users performing a flash update already know all the=20 > dangers - do not turn off the computer, do not do stupid things. >=20 > It seems somewhat unnecessary to include the extra flag that amounts=20 > to "If you give the VM unqualified control over your device do you=20 > 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 sa= me thing) matter. In my opinion, it is a lot harder for a user to unintenti= onally 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.