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

[Laszlo Ersek <lersek@redhat.com>]

On 03/02/17 18:22, Ard Biesheuvel wrote:
> 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 don't think I can debate that, but for VMs, virtio-pstore (an upcoming
QEMU feature IIRC) would be a better alternative then.

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

Well, one solution would be to reserve both the temporary and the
permanent PEI RAM regions used during "flash update boot" as Reserved or
AcpiNVS memory from the OS, assuming the firmware feature is enabled.
Then the OS would stay away.

In OVMF, we do exactly this if the S3 feature is enabled on the QEMU
command line -- during first boot, we reserve
- the small, boot mode-independent RAM area that is used as temporary
SEC/PEI heap & stack, and
- the larger, S3 resume-specific RAM area that is used as permanent PEI
RAM during S3 resume. (The permanent PEI RAM used during normal boot is
larger and separate.)

Another idea is to reserve yet another small, separate area, and when
gRT->UpdateCapsule() is called, coalesce the input (the capsules)
immediately to that reserved area, rather than just keep pointers. (This
is somewhat similar to the effects of
CAPSULE_FLAGS_POPULATE_SYSTEM_TABLE, although the coalescing would occur
immediately.)

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

Yeah, in OVMF we only distinguish "boot with full config" from "s3
resume", and the basis for that is a CMOS register value that is set by
QEMU itself -- see rtc_notify_suspend() in "hw/timer/mc146818rtc.c" --,
not by the guest OS.

On ArmVirtQemu, gRT->ResetSystem() lands in
"ArmVirtPkg/Library/ArmVirtPsciResetSystemLib", to my knowledge. The
handling of the ResetType parameter does not consider a "capsule update"
reset type just yet.

So I think ArmVirtQemu's gRT->QueryCapsuleCapabilities() implementation
should dedicate a value to this reset type, and then
ArmVirtPsciResetSystemLib should both recognize that value, *and* store
the fact in some register of the "virt" QEMU board, such that it
survives the actual reset. (Or, I guess, this fact could again be
stashed in a pre-reserved memory area -- a small one --, because guest
memory itself would survive the reset.)

This would only work of course if the guest OS rebooted the VM via the
appropriate gRT->ResetSystem() call.

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

True.

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

Good point. I think these different use cases do justify the build flag
/ feature PCD.

I guess we could agree that patches & code that affect capsule handling
would fall under your maintenance (like Xen and a bunch of other parts
already do, in effect!), and I would check such patches against
regressing the capsule-less use case, and for general (higher level) sanity.

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

Wow, that's a larger change than I expected :)

I agree that if the new QEMU board type is sufficiently different, that
could be good justification for a separate DSC. In OVMF we try to handle
i440fx, Q35, and Xen dynamically, and that causes a lot of pain. A few
months back the Xen community started developing a new domain type (PVH2
if I remember correctly? not entirely sure), and for me that was the
breaking point in OVMF's dynamic platform support -- I asked for a
separate DSC. (And, in the longer term, to separate out even the current
Xen code to that new platform.)

We can do some dynamism in the firmware, but PI doesn't really optimize
for the same platform drivers (PEIMs and DXE (runtime) drivers) to
execute on seriously different platforms. UEFI drivers/apps are
different, of course (and they are not the problem now).

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

Right. Let's hope I can shed this flu sufficiently until then.

CC'ing Drew for his information.

Thanks!
Laszlo