From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: solarflare.com, ip: 67.231.154.164, mailfrom: tpilar@solarflare.com) Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [67.231.154.164]) by groups.io with SMTP; Wed, 03 Jul 2019 04:31:26 -0700 X-Virus-Scanned: Proofpoint Essentials engine Received: from webmail.solarflare.com (webmail.solarflare.com [12.187.104.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by mx1-us3.ppe-hosted.com (PPE Hosted ESMTP Server) with ESMTPS id 1C7D46C006D; Wed, 3 Jul 2019 11:31:25 +0000 (UTC) Received: from tp-desktop.uk.solarflarecom.com (10.17.20.51) by ocex03.SolarFlarecom.com (10.20.40.36) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Wed, 3 Jul 2019 04:31:19 -0700 Subject: Re: [edk2-devel] [PATCH v2] OvmfPkg: Use DxeRuntimeCapsuleLib from DxeCapsuleLibFmp in X64 builds To: , 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> From: "Tomas Pilar (tpilar)" Message-ID: <1a255a93-7d7b-b729-2ab8-a50f913a2c5a@solarflare.com> Date: Wed, 3 Jul 2019 12:31:17 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.0 MIME-Version: 1.0 In-Reply-To: <5edb95f8-40bd-bf5e-3238-80d58855724e@redhat.com> X-Originating-IP: [10.17.20.51] X-TM-AS-Product-Ver: SMEX-12.5.0.1300-8.5.1010-24736.005 X-TM-AS-Result: No-21.858600-4.000000-10 X-TMASE-MatchedRID: IeZYkn8zfFoikhyG+/kfavZvT2zYoYOwofZV/2Xa0cJJQMTAl5EpeTr2 APfsc8GEAQmhgeWBaVs3yqkyXbPNBSlNwXDXu2RwjoyKzEmtrEcQtuqs6BbPJ7Hp3ZY7gq3dMet NxSaZpPYZXabqA8iZcpbJ5jQ0RmtRbfcPDSAqJcl8nLwuiNHSSEtc8DbogbSEwj10jtt9j+/Y1F zRn/FVbV20g8l/6lH8xJFQot0gC4VhjejNb4SeB2fd6M+N3X1xMf5Pdi+0fLaVEMx1gSjhQJztI wOsovzch+PUa3Ietirrwh4etcaYhB73mRhARXgp4h8r8l3l4eb5awEvkHdlMQzvg1/q1MH2F+j6 hUTY8i94FTWzKCjc1f0rTXULhsSH5/PHRWHr9Zb8OMEMU7OyZpkzJITaerEFl7ish7ki03zjuX6 D7uzctkg13P5/cYsMvTpxMAzYlG+35J/JantqGvRUId35VCIetD/Lx+LfvEs5SENiHpd6kjCsAm e4bhohzZv3vk1ir9FZ+tHRNgRyAujXv52nBh9BugvJpUjzDZcZSo6PM4Lsigv1OPvvDLzsVf5LE sgZ39fZd84/M2aHB/VtQSt0vE56zq1TWjOPjtfBLypRtAo4yCSF0LR+PMPIR2YNIFh+clGMgJhH xZFePpfgnY+sOxTI6hvCtINQVAeBvVdJKkNrkGWnA2xO92Up/VQgrKHFj2qRjx4hNpIk+Hvibgi OTC8x4M6xWCQejudmV/5hw3TNOMUVv4aG71/lth8ie/2LVJW/yN2q8U674lIxScKXZnK0Vo+jRD KA7KKE/evKcfsXHtM4vbOVEn8OBmjPEm4v5DOeAiCmPx4NwLTrdaH1ZWqCeVl+oyOKCVeh5DOWG PdqWY2j49Ftap9EkGUtrowrXLg= X-TM-AS-User-Approved-Sender: No X-TM-AS-User-Blocked-Sender: No X-TMASE-Result: 10--21.858600-4.000000 X-TMASE-Version: SMEX-12.5.0.1300-8.5.1010-24736.005 X-MDID: 1562153485-vdgYdh70wrcv Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Content-Language: en-US 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. > > (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. > > > (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. 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". > > > (4) All of the OvmfPkg/OvmfPkg*.dsc files should be modified in sync. > > > Thanks > Laszlo > > >