[PATCH v2] OvmfPkg: Use DxeRuntimeCapsuleLib from DxeCapsuleLibFmp in X64 builds

[PATCH v2] OvmfPkg: Use DxeRuntimeCapsuleLib from DxeCapsuleLibFmp in X64 builds

[Tomas Pilar (tpilar)]

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.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Tomas Pilar <tpilar@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/DxeRuntimeCapsuleLib.inf
   DxeServicesLib|MdePkg/Library/DxeServicesLib/DxeServicesLib.inf
   DxeServicesTableLib|MdePkg/Library/DxeServicesTableLib/DxeServicesTableLib.inf
   PeCoffGetEntryPointLib|MdePkg/Library/BasePeCoffGetEntryPointLib/BasePeCoffGetEntryPointLib.inf
-- 
2.17.2

Re: [PATCH v2] OvmfPkg: Use DxeRuntimeCapsuleLib from DxeCapsuleLibFmp in X64 builds

[Laszlo Ersek]

(+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.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Tomas Pilar <tpilar@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/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
<https://bugzilla.tianocore.org/show_bug.cgi?id=386> 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

Re: [edk2-devel] [PATCH v2] OvmfPkg: Use DxeRuntimeCapsuleLib from DxeCapsuleLibFmp in X64 builds

[Philippe Mathieu-Daudé]

Hi Tomas,

On 6/24/19 5:53 PM, 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.

How can I test this?

Thanks,

Phil.

> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Tomas Pilar <tpilar@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/DxeRuntimeCapsuleLib.inf
>    DxeServicesLib|MdePkg/Library/DxeServicesLib/DxeServicesLib.inf
>    DxeServicesTableLib|MdePkg/Library/DxeServicesTableLib/DxeServicesTableLib.inf
>    PeCoffGetEntryPointLib|MdePkg/Library/BasePeCoffGetEntryPointLib/BasePeCoffGetEntryPointLib.inf
> 

Re: [edk2-devel] [PATCH v2] OvmfPkg: Use DxeRuntimeCapsuleLib from DxeCapsuleLibFmp in X64 builds

[Tomas Pilar (tpilar)]

I mean you need a PCI device with a UEFI driver that implements FMP to test this. Our Solarflare driver currently does, so I can test this, I've sent two of our NICs to Peter Jones @ RedHat (If you are interested, I can get you a sample of our adapter along with drivers).

Once you have your PCI device and a driver, you make a capsule using GenerateCapsule, adding the driver alongside your payload. 

With newish qemu and ovmf you can use PCI passthrough (also called hostdev) to pass the PCI device entirely into the VM. Then you run CapsuleApp.efi (built from MdeModulePkg) in UEFI shell in that VM and pass the capsule as first parameter. That's all there is to it.

I can share my libvirt XML containing the hostdev configuration if you'd like.

Cheers,
Tom

-----Original Message-----
From: Philippe Mathieu-Daudé <philmd@redhat.com> 
Sent: 25 June 2019 15:52
To: Devel EDK2 <devel@edk2.groups.io>; Tomas Pilar <tpilar@solarflare.com>
Cc: jordan.l.justen@intel.com; Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: Re: [edk2-devel] [PATCH v2] OvmfPkg: Use DxeRuntimeCapsuleLib from DxeCapsuleLibFmp in X64 builds

Hi Tomas,

On 6/24/19 5:53 PM, 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.

How can I test this?

Thanks,

Phil.

> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Tomas Pilar <tpilar@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.i
> nf
> -  
> CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.in
> f
> +  
> + CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeRuntimeCapsuleLi
> + b.inf
>    DxeServicesLib|MdePkg/Library/DxeServicesLib/DxeServicesLib.inf
>    DxeServicesTableLib|MdePkg/Library/DxeServicesTableLib/DxeServicesTableLib.inf
>    
> PeCoffGetEntryPointLib|MdePkg/Library/BasePeCoffGetEntryPointLib/BaseP
> eCoffGetEntryPointLib.inf
> 

Re: [edk2-devel] [PATCH v2] OvmfPkg: Use DxeRuntimeCapsuleLib from DxeCapsuleLibFmp in X64 builds

[Tomas Pilar (tpilar)]

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.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Signed-off-by: Tomas Pilar <tpilar@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/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
> <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.
>
>
> (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
>
> 
>

Re: [edk2-devel] [PATCH v2] OvmfPkg: Use DxeRuntimeCapsuleLib from DxeCapsuleLibFmp in X64 builds

[Laszlo Ersek]

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.justen@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Signed-off-by: Tomas Pilar <tpilar@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/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
>> <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.

Re: [edk2-devel] [PATCH v2] OvmfPkg: Use DxeRuntimeCapsuleLib from DxeCapsuleLibFmp in X64 builds

[Tomas Pilar (tpilar)]

Fair enough, I'll spin a new patch.

-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com> 
Sent: 03 July 2019 15:46
To: Tomas Pilar <tpilar@solarflare.com>; Devel EDK2 <devel@edk2.groups.io>
Cc: jordan.l.justen@intel.com; Ard Biesheuvel <ard.biesheuvel@linaro.org>; Michael Kinney <michael.d.kinney@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.justen@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Signed-off-by: Tomas Pilar <tpilar@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.