Re: [edk2-devel] [PATCH v3 3/6] OvmfPkg/AmdSev: add Grub Firmware Volume Package

["Laszlo Ersek" <lersek@redhat.com>]

On 11/30/20 21:28, James Bottomley wrote:
> This is used to package up the grub bootloader into a firmware volume
> where it can be executed as a shell like the UEFI Shell.  Grub itself
> is built as a minimal entity into a Fv and then added as a boot
> option.  By default the UEFI shell isn't built but for debugging
> purposes it can be enabled and will then be presented as a boot option
> (This should never be allowed for secure boot in an external data
> centre but may be useful for local debugging).  Finally all other boot
> options except grub and possibly the shell are stripped and the boot
> timeout forced to 0 so the system will not enter a setup menu and will
> only boot to grub.  This is done by copying the
> Library/PlatformBootManagerLib into Library/PlatformBootManagerLibGrub
> and then customizing it.
> 
> Boot failure is fatal to try to prevent secret theft.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3077
> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
> 
> ---
> 
> v2: strip out s3 and qemu boot contacts make grub script robust and
>     don't build grub.efi each time
> v3: add copyrights, untabify grub.sh, strip more from PlatformBootLibGrub.
> ---
>  OvmfPkg/OvmfPkg.dec                           |    1 +
>  OvmfPkg/AmdSev/AmdSevX64.dsc                  |   21 +-
>  OvmfPkg/AmdSev/AmdSevX64.fdf                  |    7 +-
>  OvmfPkg/AmdSev/Grub/Grub.inf                  |   39 +
>  .../PlatformBootManagerLibGrub.inf            |   71 +
>  .../PlatformBootManagerLibGrub/BdsPlatform.h  |  175 ++
>  .../PlatformBootManagerLibGrub/BdsPlatform.c  | 1482 +++++++++++++++++
>  .../PlatformBootManagerLibGrub/PlatformData.c |  214 +++
>  OvmfPkg/AmdSev/Grub/.gitignore                |    1 +
>  OvmfPkg/AmdSev/Grub/grub.cfg                  |   46 +
>  OvmfPkg/AmdSev/Grub/grub.sh                   |   93 ++
>  11 files changed, 2141 insertions(+), 9 deletions(-)
>  create mode 100644 OvmfPkg/AmdSev/Grub/Grub.inf
>  create mode 100644 OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.inf
>  create mode 100644 OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h
>  create mode 100644 OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c
>  create mode 100644 OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformData.c
>  create mode 100644 OvmfPkg/AmdSev/Grub/.gitignore
>  create mode 100644 OvmfPkg/AmdSev/Grub/grub.cfg
>  create mode 100644 OvmfPkg/AmdSev/Grub/grub.sh

[...]

> +VOID
> +EFIAPI
> +PlatformBootManagerBeforeConsole (
> +  VOID
> +  )
> +{
> +  EFI_HANDLE    Handle;
> +  EFI_STATUS    Status;
> +  UINT16 FrontPageTimeout = 0;

(1) initialization of local variables is not permitted in the edk2
coding style; but we can fix this up at merge time.

(also now I understand why you kept gEfiGlobalVariableGuid and
UefiRuntimeServicesTableLib)

> +
> +  DEBUG ((DEBUG_INFO, "PlatformBootManagerBeforeConsole\n"));
> +  InstallDevicePathCallback ();
> +
> +  VisitAllInstancesOfProtocol (&gEfiPciRootBridgeIoProtocolGuid,
> +    ConnectRootBridge, NULL);
> +
> +  //
> +  // Signal the ACPI platform driver that it can download QEMU ACPI tables.
> +  //
> +  EfiEventGroupSignal (&gRootBridgesConnectedEventGroupGuid);
> +
> +  //
> +  // We can't signal End-of-Dxe earlier than this. Namely, End-of-Dxe triggers
> +  // the preparation of S3 system information. That logic has a hard dependency
> +  // on the presence of the FACS ACPI table. Since our ACPI tables are only
> +  // installed after PCI enumeration completes, we must not trigger the S3 save
> +  // earlier, hence we can't signal End-of-Dxe earlier.
> +  //
> +  EfiEventGroupSignal (&gEfiEndOfDxeEventGroupGuid);
> +
> +  //
> +  // Prevent further changes to LockBoxes or SMRAM.
> +  //
> +  Handle = NULL;
> +  Status = gBS->InstallProtocolInterface (&Handle,
> +                  &gEfiDxeSmmReadyToLockProtocolGuid, EFI_NATIVE_INTERFACE,
> +                  NULL);
> +  ASSERT_EFI_ERROR (Status);
> +
> +  //
> +  // Dispatch deferred images after EndOfDxe event and ReadyToLock
> +  // installation.
> +  //
> +  EfiBootManagerDispatchDeferredImages ();
> +
> +  PlatformInitializeConsole (gPlatformConsole);
> +
> +  Status = gRT->SetVariable (
> +                  EFI_TIME_OUT_VARIABLE_NAME,
> +                  &gEfiGlobalVariableGuid,
> +                  (EFI_VARIABLE_NON_VOLATILE |
> +                   EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +                   EFI_VARIABLE_RUNTIME_ACCESS),
> +                  sizeof FrontPageTimeout,
> +                  &FrontPageTimeout
> +                  );
> +  //
> +  // Install both VIRTIO_DEVICE_PROTOCOL and (dependent) EFI_RNG_PROTOCOL
> +  // instances on Virtio PCI RNG devices.
> +  //
> +  VisitAllInstancesOfProtocol (&gEfiPciIoProtocolGuid, ConnectVirtioPciRng,
> +    NULL);
> +}

[...]

with (1) fixed:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo