Fair enough, I'll spin a new patch.
-----Original Message-----
From: Laszlo Ersek <ler...@redhat.com>
Sent: 03 July 2019 15:46
To: Tomas Pilar <tpi...@solarflare.com>; Devel EDK2 <devel@edk2.groups.io>
Cc: jordan.l.jus...@intel.com; Ard Biesheuvel <ard.biesheu...@linaro.org>;
Michael Kinney <michael.d.kin...@intel.com>
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
>>> 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.jus...@intel.com>
>>> Cc: Laszlo Ersek <ler...@redhat.com>
>>> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
>>> Signed-off-by: Tomas Pilar <tpi...@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/DxeRuntimeCapsule
>>> + Lib.inf
>>> DxeServicesLib|MdePkg/Library/DxeServicesLib/DxeServicesLib.inf
>>>
>>> DxeServicesTableLib|MdePkg/Library/DxeServicesTableLib/DxeServicesTableLib.inf
>>>
>>> PeCoffGetEntryPointLib|MdePkg/Library/BasePeCoffGetEntryPointLib/Bas
>>> ePeCoffGetEntryPointLib.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
>> <https://bugzilla.tianocore.org/show_bug.cgi?id=386> 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.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#43221): https://edk2.groups.io/g/devel/message/43221
Mute This Topic: https://groups.io/mt/32193560/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
