Re: [PATCH 1/1] OvmfPkg/AmdSev: introduce EMBED_GRUB=FALSE to skip including Grub image


James Bottomley
 

On Wed, 2021-07-07 at 10:42 +0000, Dov Murik wrote:
The AmdSevX64 target includes an embedded Grub image to support
secure
(measured) boot of confidential guests with encrypted root images.

However, it is sometimes convenient to build this target without an
embedded Grub. We introduce the EMBED_GRUB setting (defaults to
TRUE),
which conditions the generation (grub.sh) and inclusion of the Grub
image. Now building AmdSevX64 with -DEMBED_GRUB=FALSE allows it.

Cc: Laszlo Ersek <lersek@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Jordan Justen <jordan.l.justen@...>
Cc: Ashish Kalra <ashish.kalra@...>
Cc: Brijesh Singh <brijesh.singh@...>
Cc: Erdem Aktas <erdemaktas@...>
Cc: James Bottomley <jejb@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Min Xu <min.m.xu@...>
Cc: Tom Lendacky <thomas.lendacky@...>
Cc: Tobin Feldman-Fitzthum <tobin@...>
Signed-off-by: Dov Murik <dovmurik@...>
---
OvmfPkg/AmdSev/AmdSevX64.dsc | 16 +++++++++++++++-
OvmfPkg/AmdSev/AmdSevX64.fdf | 2 ++
2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc
b/OvmfPkg/AmdSev/AmdSevX64.dsc
index 1d487befae08..ba7d6fe6b749 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.dsc
+++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
@@ -25,7 +25,6 @@ [Defines]
BUILD_TARGETS = NOOPT|DEBUG|RELEASE
SKUID_IDENTIFIER = DEFAULT
FLASH_DEFINITION = OvmfPkg/AmdSev/AmdSevX64.fdf
- PREBUILD = sh OvmfPkg/AmdSev/Grub/grub.sh

#
# Defines for default states. These can be changed on the command
line.
@@ -40,6 +39,19 @@ [Defines]
#
DEFINE BUILD_SHELL = FALSE

+ #
+ # Embed Grub into the OVMF image so they are measured together
when launching
+ # confidential guest
+ #
+ DEFINE EMBED_GRUB = TRUE
+
+!if $(EMBED_GRUB) == TRUE
+ #
+ # This step builds the grub.efi binary image if needed
+ #
+ PREBUILD = sh OvmfPkg/AmdSev/Grub/grub.sh
+!endif
+
#
# Device drivers
#
@@ -784,7 +796,9 @@ [Components]
}
!endif
OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
+!if $(EMBED_GRUB) == TRUE
OvmfPkg/AmdSev/Grub/Grub.inf
+!endif
!if $(BUILD_SHELL) == TRUE
ShellPkg/Application/Shell/Shell.inf {
<LibraryClasses>
diff --git a/OvmfPkg/AmdSev/AmdSevX64.fdf
b/OvmfPkg/AmdSev/AmdSevX64.fdf
index 9977b0f00a18..ee3d96bb813f 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.fdf
+++ b/OvmfPkg/AmdSev/AmdSevX64.fdf
@@ -270,7 +270,9 @@ [FV.DXEFV]
INF OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellC
ommand.inf
!endif
INF OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
+!if $(EMBED_GRUB) == TRUE
INF OvmfPkg/AmdSev/Grub/Grub.inf
+!endif
!if $(BUILD_SHELL) == TRUE
INF ShellPkg/Application/Shell/Shell.inf
!endif
This likely isn't enough: the boot pathway of the AmdSev package is
stripped down and designed to fail if grub won't boot, so if you set
EMBED_GRUB = false, you'll likely build a system that won't boot. This
would still work for the Kata use case, if the kernel and initrd are
plumbed back in, but it won't work for the generic use case. I think
the change log needs to describe the use cases so we don't end up
getting a load of annoyed people building systems that won't work for
them.

There's also the broader question of whether this should all be
integrated back into OvmfX64Pkg with more determination done at
runtime, so we can build fewer separate binaries?

James

Join devel@edk2.groups.io to automatically receive all group messages.