Re: [PATCH 0/4] ArmVirtPkg: implement basic capsule support

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 2 March 2017 at 17:09, Laszlo Ersek <lersek@redhat.com> wrote:
> On 03/02/17 17:15, Ard Biesheuvel wrote:
>> This wires up the existing basic support for capsules left in memory by
>> the OS across a warm reset. This involves wiring up the PEI phase modules
>> to preserve the capsule images before releasing the memory for normal
>> consumption, and some tweaks to the boot mode and BDS platform routines.
>>
>> As proposed, this allows capsules to be used as a pstore backend, which
>> keeps the pstore payload in memory rather than in EFI variables. For
>> example, something like this is supported when the prerequisite Linux
>> patches have been merged (which are currently under review)
>>
>> - modprobe capsule-pstore
>> - echo 1 > /sys/module/kernel/parameters/panic
>> - echo c > /proc/sysrq-trigger
>> - system reboot...
>> - ls -l /sys/fs/pstore/
>>   -r--r--r-- 1 root root 4386 Feb 12 00:31 console-efi-capsule-0
>>   -r--r--r-- 1 root root 9065 Feb 12 00:29 dmesg-efi-capsule-6250071391647825921
>>   -r--r--r-- 1 root root 9073 Feb 12 00:29 dmesg-efi-capsule-6250071391647825922
>>   -r--r--r-- 1 root root 9096 Feb 12 00:29 dmesg-efi-capsule-6250071391647825923
>>   -r--r--r-- 1 root root 9073 Feb 12 00:29 dmesg-efi-capsule-6250071391647825924
>>   -r--r--r-- 1 root root 9048 Feb 12 00:29 dmesg-efi-capsule-6250071391647825925
>>
>> Updating the firmware image in NOR flash in this way should be feasible as
>> well, but this is something that builds on top of this basic capsule support,
>> and involves ESRT and FMP, which have far too few vowels for me to explain
>> what they entail.
>>
>> Ard Biesheuvel (4):
>>   ArmVirtPkg/ArmVirtPlatformLib: base boot mode on capsule presence
>>   ArmVirtPkg/ArmVirtMemoryInitPeiLib: check for capsules before memory
>>     init
>>   ArmVirtPkg/PlatformBootManagerLib: process pending capsules
>>   ArmVirtPkg/ArmVirtQemu: enable basic capsule support
>>
>>  ArmVirtPkg/ArmVirtQemu.dsc                                             |  7 ++-
>>  ArmVirtPkg/ArmVirtQemu.fdf                                             |  2 +
>>  ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c   | 60 +++++++++++++++++++-
>>  ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf |  9 ++-
>>  ArmVirtPkg/Library/ArmVirtPlatformLib/ArmVirtPlatformLib.inf           |  5 +-
>>  ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c                           |  4 ++
>>  ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c                 | 17 ++++++
>>  ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf   |  2 +
>>  8 files changed, 101 insertions(+), 5 deletions(-)
>>
>
> (1) I think I disagree with this feature. What is the use of
> capsule-pstore over efivar-pstore?

efivar-pstore keeps the logs itself in EFI variables, and is known to
cause problems when it runs out of space.

> I dislike the addition of a new boot
> mode (BOOT_ON_FLASH_UPDATE) -- and all the complications it means for
> PlatformBootManagerLib -- unless there is a *very* good argument for
> capsule-pstore.
>
> Even if there is such a very good argument, a detailed design document
> would be necessary about
>
> - memory regions (address, size, life cycle etc) used for capsule
>   passing from OS to firmware across warm reboot,
>

Yes, I wondered about this: how do we keep the OS from putting the
capsule in our temporary PEI ram. Some AArch64 systems use on-chip
SRAM for temporary PEI RAM, which nicely sidesteps the problem.

> - interplay between existent and newly pulled in modules (for example,
>   what produces the EFI_HOB_TYPE_UEFI_CAPSULE HOB before patch #1 and
>   "ArmPlatformPkg/PlatformPei/PlatformPeim.inf" consume it, what ensures
>   the HOB would be prodced in time, how does the new boot mode affect
>   other modules, why do we need setting / returning the new boot mode
>   in both patches #1 and #2, ...)
>

The one concern I have here is that we currently have no way of
signalling the boot mode at all, and we have to infer it. Ideally, the
UpdateCapsule() and/or ResetSystem() call arguments propagate into
this variable, but how this is implemented is platform specific afaict

> Even if said "very good reason" exists, I think I'd insist on a big red
> switch over all of this, defaulting to "off" -- a build time flag that
> by default excludes modules from the DSC / FDF, and also a parallel
> Feature PCD, which short-circuits all the new code in the modules that
> we build in unconditionally. (Same as we do with SMM_REQUIRE in OVMF.)
>

Fair enough. I think this mainly comes down to our difference in use
cases: for me, ArmVirtQemu is primarily a reference implementation,
whereas your interest is strictly virtualization.

> (2) I most definitely disagree with the idea of firmware executable
> updates within the guest. That turns firmware image updates into a
> multi-master problem (on a virtualization host, you update firmware for
> guests by running a package update, and then shutting down and
> restarting eligible guests -- the host file that backs the pflash chip
> that contains the firmware executable is not even writeable to QEMU).
>

Same point as above. If Linaro members need a reference impementation
of FMP, ArmVirtQemu is the vehicle atm.

> Such a feature might perhaps be useful for testing the capsule thing
> itself, but in production, it's a recipe for disaster. This kind of
> capsule-based firmware update was invented to solve a chicken-egg
> problem on physical hardware, where there's nothing "underneath", but on
> virtual platforms, the problem doesn't exist in the first place.
>
> The implication that this feature -- which I already disagree with,
> without knowing a compelling reason for it -- lays the foundation for
> FMP, makes me *very* nervous.
>

Perhaps it is time to separate the showcase ArmVirtQemu from the
QEMU/KVM ArmVirtQemu. In fact, we are already working on a QEMU
machine model different from mach-virt which matches the enterprise
system that we target more closely, and so we'd need a different
ArmVirtPkg platform for that anyway. Another thing that has been on my
wishlist for a long time is a QEMU machine that has no RAM below 4 GB
(I have local patches for that, but it would be good to have that
upstream as well)

In fact, I am quite pleased that we have reached this point, since it
means the basic functionality is all there. In any case, we will have
plenty of time to discuss these things next week.

Cheers,
Ard.