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

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

[Ard Biesheuvel]

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

-- 
2.7.4

[PATCH 1/4] ArmVirtPkg/ArmVirtPlatformLib: base boot mode on capsule presence

[Ard Biesheuvel]

Instead of unconditionally returning BOOT_WITH_FULL_CONFIGURATION when
enquiring the platform about the boot mode, let's return enable the use
of capsules by returning BOOT_ON_FLASH_UPDATE when a capsule HOB is
detected.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmVirtPkg/Library/ArmVirtPlatformLib/ArmVirtPlatformLib.inf | 5 +++--
 ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c                 | 4 ++++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/ArmVirtPkg/Library/ArmVirtPlatformLib/ArmVirtPlatformLib.inf b/ArmVirtPkg/Library/ArmVirtPlatformLib/ArmVirtPlatformLib.inf
index 3cb3fb1f3aea..dbbe0fbee21a 100644
--- a/ArmVirtPkg/Library/ArmVirtPlatformLib/ArmVirtPlatformLib.inf
+++ b/ArmVirtPkg/Library/ArmVirtPlatformLib/ArmVirtPlatformLib.inf
@@ -29,11 +29,12 @@ [Packages]
   ArmVirtPkg/ArmVirtPkg.dec
 
 [LibraryClasses]
+  ArmLib
+  FdtLib
+  HobLib
   IoLib
   MemoryAllocationLib
-  ArmLib
   PrintLib
-  FdtLib
 
 [Sources.common]
   Virt.c
diff --git a/ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c b/ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c
index fcaf3c681a97..58175110ab43 100644
--- a/ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c
+++ b/ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c
@@ -18,6 +18,7 @@
 #include <Library/IoLib.h>
 #include <Library/ArmPlatformLib.h>
 #include <Library/DebugLib.h>
+#include <Library/HobLib.h>
 #include <Library/PcdLib.h>
 #include <ArmPlatform.h>
 #include <libfdt.h>
@@ -38,6 +39,9 @@ ArmPlatformGetBootMode (
   VOID
   )
 {
+  if (GetFirstHob (EFI_HOB_TYPE_UEFI_CAPSULE) != NULL) {
+    return BOOT_ON_FLASH_UPDATE;
+  }
   return BOOT_WITH_FULL_CONFIGURATION;
 }
 
-- 
2.7.4

[PATCH 2/4] ArmVirtPkg/ArmVirtMemoryInitPeiLib: check for capsules before memory init

[Ard Biesheuvel]

Look for any capsules left in memory by the OS across reset before
releasing the memory for normal use, so that they can be preserved and
processed later.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c   | 60 +++++++++++++++++++-
 ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf |  9 ++-
 2 files changed, 67 insertions(+), 2 deletions(-)

diff --git a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
index 6f3e54b7afcb..7f55f634e4e5 100644
--- a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
+++ b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
@@ -17,11 +17,15 @@
 
 #include <Library/ArmMmuLib.h>
 #include <Library/ArmPlatformLib.h>
+#include <Library/CacheMaintenanceLib.h>
 #include <Library/DebugLib.h>
 #include <Library/HobLib.h>
 #include <Library/MemoryAllocationLib.h>
 #include <Library/PcdLib.h>
-#include <Library/CacheMaintenanceLib.h>
+#include <Library/PeiServicesLib.h>
+#include <Library/PeiServicesTablePointerLib.h>
+
+#include <Ppi/Capsule.h>
 
 VOID
 BuildMemoryTypeInformationHob (
@@ -49,6 +53,58 @@ InitMmu (
   }
 }
 
+STATIC
+VOID
+CheckCapsule (
+  IN EFI_PHYSICAL_ADDRESS               UefiMemoryBase,
+  IN UINT64                             UefiMemorySize
+  )
+{
+  EFI_STATUS         Status;
+  EFI_PEI_SERVICES   **PeiServices;
+  PEI_CAPSULE_PPI    *Capsule;
+  VOID               *CapsuleBuffer;
+  UINTN              CapsuleBufferLength;
+
+  PeiServices = (EFI_PEI_SERVICES **) GetPeiServicesTablePointer ();
+  ASSERT (PeiServices != NULL);
+
+  //
+  // Check for persistent capsules
+  //
+  Status = PeiServicesLocatePpi (&gPeiCapsulePpiGuid, 0, NULL,
+             (VOID **)&Capsule);
+  if (Status == EFI_SUCCESS) {
+    Status = Capsule->CheckCapsuleUpdate (PeiServices);
+    if (Status == EFI_SUCCESS) {
+
+      CapsuleBuffer = (VOID *)((UINTN)FixedPcdGet32 (PcdCPUCoresStackBase) +
+                               FixedPcdGet32 (PcdCPUCorePrimaryStackSize));
+      CapsuleBufferLength = (UINTN)UefiMemoryBase - (UINTN)CapsuleBuffer;
+
+      PeiServicesSetBootMode (BOOT_ON_FLASH_UPDATE);
+      Status = Capsule->Coalesce (PeiServices, &CapsuleBuffer,
+                          &CapsuleBufferLength);
+      if (!EFI_ERROR (Status)) {
+        DEBUG ((DEBUG_INFO, "%a: Coalesced capsule @ %p (0x%lx) capsule\n",
+                __FUNCTION__, CapsuleBuffer, CapsuleBufferLength));
+      } else {
+        DEBUG ((DEBUG_WARN, "%a: failed to coalesce() capsule (Status == %r)\n",
+                __FUNCTION__, Status));
+        return;
+      }
+
+      Status = Capsule->CreateState (PeiServices, CapsuleBuffer,
+                          CapsuleBufferLength);
+
+      if (EFI_ERROR (Status)) {
+        DEBUG ((DEBUG_WARN, "%a: Capsule->CreateState failed (Status == %r)\n",
+                __FUNCTION__, Status));
+      }
+    }
+  }
+}
+
 EFI_STATUS
 EFIAPI
 MemoryPeim (
@@ -109,6 +165,8 @@ MemoryPeim (
   // Build Memory Allocation Hob
   InitMmu ();
 
+  CheckCapsule (UefiMemoryBase, UefiMemorySize);
+
   if (FeaturePcdGet (PcdPrePiProduceMemoryTypeInformationHob)) {
     // Optional feature that helps prevent EFI memory map fragmentation.
     BuildMemoryTypeInformationHob ();
diff --git a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf
index 028d6fb5ac28..4524afd2c7ed 100644
--- a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf
+++ b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf
@@ -37,6 +37,8 @@ [LibraryClasses]
   ArmMmuLib
   ArmPlatformLib
   CacheMaintenanceLib
+  PeiServicesLib
+  PeiServicesTablePointerLib
 
 [Guids]
   gEfiMemoryTypeInformationGuid
@@ -48,6 +50,8 @@ [FixedPcd]
   gArmTokenSpaceGuid.PcdFdSize
 
   gArmPlatformTokenSpaceGuid.PcdSystemMemoryUefiRegionSize
+  gArmPlatformTokenSpaceGuid.PcdCPUCoresStackBase
+  gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize
 
   gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory
   gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS
@@ -64,5 +68,8 @@ [Pcd]
   gArmTokenSpaceGuid.PcdSystemMemorySize
   gArmTokenSpaceGuid.PcdFdBaseAddress
 
+[Ppis]
+  gPeiCapsulePpiGuid
+
 [Depex]
-  TRUE
+   gPeiCapsulePpiGuid
-- 
2.7.4

[PATCH 3/4] ArmVirtPkg/PlatformBootManagerLib: process pending capsules

[Ard Biesheuvel]

Process any capsule HOBs that were left for us by CapsulePei. This
involves calling ProcessCapsules() twice, as explained in the comment
in DxeCapsuleLibFmp.

1) The first call must be before EndOfDxe. The system capsules is processed.
   If device capsule FMP protocols are exposted at this time and device FMP
   capsule has zero EmbeddedDriverCount, the device capsules are processed.
   Each individual capsule result is recorded in capsule record variable.
   System may reset in this function, if reset is required by capsule and
   all capsules are processed.
   If not all capsules are processed, reset will be defered to second call.

2) The second call must be after EndOfDxe and after ConnectAll, so that all
   device capsule FMP protocols are exposed.
   The system capsules are skipped. If the device capsules are NOT processed
   in first call, they are processed here.
   Each individual capsule result is recorded in capsule record variable.
   System may reset in this function, if reset is required by capsule
   processed in first call and second call.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c               | 17 +++++++++++++++++
 ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf |  2 ++
 2 files changed, 19 insertions(+)

diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
index 94da51ad49f1..1ebfecd992f7 100644
--- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
+++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
@@ -17,7 +17,9 @@
 
 #include <IndustryStandard/Pci22.h>
 #include <Library/BootLogoLib.h>
+#include <Library/CapsuleLib.h>
 #include <Library/DevicePathLib.h>
+#include <Library/HobLib.h>
 #include <Library/PcdLib.h>
 #include <Library/QemuBootOrderLib.h>
 #include <Library/UefiBootManagerLib.h>
@@ -579,6 +581,13 @@ PlatformBootManagerBeforeConsole (
   )
 {
   RETURN_STATUS PcdStatus;
+  EFI_STATUS    Status;
+
+  if (GetBootModeHob() == BOOT_ON_FLASH_UPDATE) {
+    DEBUG((DEBUG_INFO, "ProcessCapsules Before EndOfDxe ......\n"));
+    Status = ProcessCapsules ();
+    DEBUG((DEBUG_INFO, "ProcessCapsules %r\n", Status));
+  }
 
   //
   // Signal EndOfDxe PI Event
@@ -663,6 +672,8 @@ PlatformBootManagerAfterConsole (
   VOID
   )
 {
+  EFI_STATUS    Status;
+
   //
   // Show the splash screen.
   //
@@ -673,6 +684,12 @@ PlatformBootManagerAfterConsole (
   //
   EfiBootManagerConnectAll ();
 
+  if (GetBootModeHob() == BOOT_ON_FLASH_UPDATE) {
+    DEBUG((DEBUG_INFO, "ProcessCapsules After EndOfDxe ......\n"));
+    Status = ProcessCapsules ();
+    DEBUG((DEBUG_INFO, "ProcessCapsules %r\n", Status));
+  }
+
   //
   // Process QEMU's -kernel command line option. Note that the kernel booted
   // this way should receive ACPI tables, which is why we connect all devices
diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
index 1f162c663fc1..4d218097a420 100644
--- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
+++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
@@ -45,8 +45,10 @@ [LibraryClasses]
   BaseLib
   BaseMemoryLib
   BootLogoLib
+  CapsuleLib
   DebugLib
   DevicePathLib
+  HobLib
   MemoryAllocationLib
   PcdLib
   PrintLib
-- 
2.7.4

[PATCH 4/4] ArmVirtPkg/ArmVirtQemu: enable basic capsule support

[Ard Biesheuvel]

This wires up the existing code for processing capsule: it enables
CapsulePei, which preserves capsules left in memory by the OS, and
combined with the PlatformBootManagerLib and other changes in previous
patches, this will ensure that capsules are handed back to the OS
via the system table if it requested so. This enables features like
the capsule-pstore for Linux, which is currently under review (sadly,
Gmane nor marc.info archive the linux-efi mailing list)

Implementing the firmware management protocol on top of this should
certainly be doable, but has not been attempted yet.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmVirtPkg/ArmVirtQemu.dsc | 7 ++++++-
 ArmVirtPkg/ArmVirtQemu.fdf | 2 ++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index 477dfdcfc764..b44b3c82abac 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -59,7 +59,7 @@ [LibraryClasses.common]
   TimerLib|ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.inf
   NorFlashPlatformLib|ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf
 
-  CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
+  CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.inf
   BootLogoLib|MdeModulePkg/Library/BootLogoLib/BootLogoLib.inf
   PlatformBootManagerLib|ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
   CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.inf
@@ -98,6 +98,9 @@ [PcdsFeatureFlag.common]
   gArmVirtTokenSpaceGuid.PcdPureAcpiBoot|TRUE
 !endif
 
+  gEfiMdeModulePkgTokenSpaceGuid.PcdSupportUpdateCapsuleReset|TRUE
+
+
 [PcdsFixedAtBuild.common]
   gArmPlatformTokenSpaceGuid.PcdCoreCount|1
 !if $(ARCH) == AARCH64
@@ -234,7 +237,9 @@ [Components.common]
   ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf
   ArmPkg/Drivers/CpuPei/CpuPei.inf
 
+  MdeModulePkg/Universal/FaultTolerantWritePei/FaultTolerantWritePei.inf
   MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
+  MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
 
   MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf {
     <LibraryClasses>
diff --git a/ArmVirtPkg/ArmVirtQemu.fdf b/ArmVirtPkg/ArmVirtQemu.fdf
index c6a22dc018f3..28d10f2e85d6 100644
--- a/ArmVirtPkg/ArmVirtQemu.fdf
+++ b/ArmVirtPkg/ArmVirtQemu.fdf
@@ -108,7 +108,9 @@ [FV.FVMAIN_COMPACT]
   INF ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf
   INF ArmPkg/Drivers/CpuPei/CpuPei.inf
   INF MdeModulePkg/Universal/PCD/Pei/Pcd.inf
+  INF MdeModulePkg/Universal/FaultTolerantWritePei/FaultTolerantWritePei.inf
   INF MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
+  INF MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
   INF MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
 
   FILE FV_IMAGE = 9E21FD93-9C72-4c15-8C4B-E77F1DB2D792 {
-- 
2.7.4

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

[Laszlo Ersek]

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

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

[Ard Biesheuvel]

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.

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

[Laszlo Ersek]

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