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

[Laszlo Ersek <lersek@redhat.com>]

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? 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,

- 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, ...)

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.)

(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).

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.

Laszlo