[PATCH v2 0/4] Resolve compatibility issues with EDK2, improve error handling

[Jan Kiszka]

On the one side, EDK2 is stricter than U-Boot and pointed out that our
trick to use the kernel section directly cannot work reliably, see patch
2.

On the other side, it lacks support the for device tree fixup protocol.
But if the DTB contains what the fixup would apply (and if that is
static), we can live without it, see patch 3.

Patch 4 is just an output cleanup.

New in v2 is patch 1. It resolves (hopefully) all cleanup issues when
something goes wrong and the stub or the kernel exit with an error.
Pointed out by Christian during review of v1.

Further changes in v2:
- dropped now unused kernel_pe_headers variable from bg_gen_unified_kernel
- hand over "unfixed" dtb via pool memory (rather than using the section
directly) - API suggests this, so does systemd
- many typo fixes

Jan

Jan Kiszka (4):
kernel-stub: Improve cleanup on errors
kernel-stub: Copy the kernel out of its section into a buffer
kernel-stub: Allow booting without device tree fixup protocol
kernel-stub: Drop incorrect new-line in error output

kernel-stub/fdt.c | 52 ++++++++++++++++----
kernel-stub/kernel-stub.h | 4 +-
kernel-stub/main.c | 96 +++++++++++++++++++++++++++++--------
tools/bg_gen_unified_kernel | 5 +-
4 files changed, 121 insertions(+), 36 deletions(-)

--
2.35.3

[Jan Kiszka]

From: Jan Kiszka <jan.k...@siemens.com>

EDK2 does not support this yet. We can live without it if the user
provides DTBs with the required carve-outs hard-coded. Therefore, issue
a warning and install the embedded DTB directly.

Signed-off-by: Jan Kiszka <jan.k...@siemens.com>
---
kernel-stub/fdt.c | 43 +++++++++++++++++++++++++++++++--------
kernel-stub/kernel-stub.h | 1 +
kernel-stub/main.c | 2 +-
3 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/kernel-stub/fdt.c b/kernel-stub/fdt.c
index b64cd7d..11e8857 100644
--- a/kernel-stub/fdt.c
+++ b/kernel-stub/fdt.c
@@ -152,9 +152,24 @@ BOOLEAN match_fdt(const VOID *fdt, const CHAR8 *compatible)
return strcmpa(compatible, alt_compatible) == 0;
}

-EFI_STATUS replace_fdt(const VOID *fdt)
+static VOID *clone_fdt(const VOID *fdt, UINTN size)
{
const FDT_HEADER *header = fdt;
+ VOID *clone;
+
+ clone = AllocatePool(size);
+ if (!clone) {
+ error(L"Error allocating device tree buffer",
+ EFI_OUT_OF_RESOURCES);
+ return NULL;
+ }
+
+ CopyMem(clone, fdt, BE32_TO_HOST(header->TotalSize));
+ return clone;
+}
+
+EFI_STATUS replace_fdt(const VOID *fdt)
+{
EFI_DT_FIXUP_PROTOCOL *protocol;
EFI_STATUS status;
VOID *fdt_buffer;
@@ -162,27 +177,39 @@ EFI_STATUS replace_fdt(const VOID *fdt)

status = LibLocateProtocol(&EfiDtFixupProtocol, (VOID **)&protocol);
if (EFI_ERROR(status)) {
- error(L"Did not find device tree fixup protocol", status);
+ const FDT_HEADER *header = fdt;
+
+ info(L"Firmware does not provide device tree fixup protocol");
+
+ fdt_buffer = clone_fdt(fdt, BE32_TO_HOST(header->TotalSize));
+ if (!fdt_buffer) {
+ return EFI_OUT_OF_RESOURCES;
+ }
+
+ status = BS->InstallConfigurationTable(&EfiDtbTableGuid,
+ fdt_buffer);
+ if (EFI_ERROR(status)) {
+ FreePool(fdt_buffer);
+ error(L"Failed to install alternative device tree",
+ status);
+ }
return status;
}

/* Find out which size we need */
size = 0;
- status = protocol->Fixup(protocol, (VOID *)fdt, &size,
+ status = protocol->Fixup(protocol, (VOID *) fdt, &size,
EFI_DT_APPLY_FIXUPS);
if (status != EFI_BUFFER_TOO_SMALL) {
error(L"Device tree fixup: unexpected error", status);
return status;
}

- fdt_buffer = AllocatePool(size);
+ fdt_buffer = clone_fdt(fdt, size);
if (!fdt_buffer) {
- status = EFI_OUT_OF_RESOURCES;
- error(L"Error allocating device tree buffer", status);
- return status;
+ return EFI_OUT_OF_RESOURCES;
}

- CopyMem(fdt_buffer, fdt, BE32_TO_HOST(header->TotalSize));
status = protocol->Fixup(protocol, fdt_buffer, &size,
EFI_DT_APPLY_FIXUPS | EFI_DT_RESERVE_MEMORY |
EFI_DT_INSTALL_TABLE);
diff --git a/kernel-stub/kernel-stub.h b/kernel-stub/kernel-stub.h
index c0bd542..6bf9d88 100644
--- a/kernel-stub/kernel-stub.h
+++ b/kernel-stub/kernel-stub.h
@@ -16,6 +16,7 @@

VOID error(CHAR16 *message, EFI_STATUS status);
VOID __attribute__((noreturn)) error_exit(CHAR16 *message, EFI_STATUS status);
+VOID info(CHAR16 *message);

const VOID *get_fdt_compatible(VOID);
BOOLEAN match_fdt(const VOID *fdt, const CHAR8 *compatible);
diff --git a/kernel-stub/main.c b/kernel-stub/main.c
index 5cf06ac..c0be1f6 100644
--- a/kernel-stub/main.c
+++ b/kernel-stub/main.c
@@ -63,7 +63,7 @@ EFI_PHYSICAL_ADDRESS align_addr(EFI_PHYSICAL_ADDRESS ptr,
return (ptr + align - 1) & ~(align - 1);
}

-static VOID info(CHAR16 *message)
+VOID info(CHAR16 *message)
{
Print(L"Unified kernel stub: %s\n", message);
}
--
2.35.3

[Jan Kiszka]

From: Jan Kiszka <jan.k...@siemens.com>

Make sure we always roll back resource allocations and protocol
registrations on errors so that nothing is leaked and nothing could
break when running a different UEFI binary after the stub has failed.
Requires to let replace_fdt return an error rather than terminating
the stub.

Signed-off-by: Jan Kiszka <jan.k...@siemens.com>
---

kernel-stub/fdt.c | 17 ++++++++++------
kernel-stub/kernel-stub.h | 3 ++-
kernel-stub/main.c | 41 ++++++++++++++++++++++++++-------------
3 files changed, 40 insertions(+), 21 deletions(-)

diff --git a/kernel-stub/fdt.c b/kernel-stub/fdt.c
index 513aa76..b64cd7d 100644
--- a/kernel-stub/fdt.c
+++ b/kernel-stub/fdt.c
@@ -152,7 +152,7 @@ BOOLEAN match_fdt(const VOID *fdt, const CHAR8 *compatible)

return strcmpa(compatible, alt_compatible) == 0;
}

-VOID replace_fdt(const VOID *fdt)
+EFI_STATUS replace_fdt(const VOID *fdt)

{
const FDT_HEADER *header = fdt;

EFI_DT_FIXUP_PROTOCOL *protocol;
@@ -162,7 +162,8 @@ VOID replace_fdt(const VOID *fdt)

status = LibLocateProtocol(&EfiDtFixupProtocol, (VOID **)&protocol);
if (EFI_ERROR(status)) {

- error_exit(L"Did not find device tree fixup protocol", status);
+ error(L"Did not find device tree fixup protocol", status);

+ return status;
}

/* Find out which size we need */

@@ -170,13 +171,15 @@ VOID replace_fdt(const VOID *fdt)

status = protocol->Fixup(protocol, (VOID *)fdt, &size,

EFI_DT_APPLY_FIXUPS);
if (status != EFI_BUFFER_TOO_SMALL) {

- error_exit(L"Device tree fixup: unexpected error", status);
+ error(L"Device tree fixup: unexpected error", status);
+ return status;
}

fdt_buffer = AllocatePool(size);
if (!fdt_buffer) {
- error_exit(L"Error allocating device tree buffer",
- EFI_OUT_OF_RESOURCES);
+ status = EFI_OUT_OF_RESOURCES;
+ error(L"Error allocating device tree buffer", status);
+ return status;
}

CopyMem(fdt_buffer, fdt, BE32_TO_HOST(header->TotalSize));
@@ -185,6 +188,8 @@ VOID replace_fdt(const VOID *fdt)
EFI_DT_INSTALL_TABLE);
if (EFI_ERROR(status)) {
FreePool(fdt_buffer);
- error_exit(L"Device tree fixup failed", status);
+ error(L"Device tree fixup failed", status);
}
+
+ return status;
}
diff --git a/kernel-stub/kernel-stub.h b/kernel-stub/kernel-stub.h
index 4944355..c0bd542 100644
--- a/kernel-stub/kernel-stub.h
+++ b/kernel-stub/kernel-stub.h
@@ -14,11 +14,12 @@

#include <efi.h>

+VOID error(CHAR16 *message, EFI_STATUS status);

VOID __attribute__((noreturn)) error_exit(CHAR16 *message, EFI_STATUS status);

const VOID *get_fdt_compatible(VOID);
BOOLEAN match_fdt(const VOID *fdt, const CHAR8 *compatible);

-VOID replace_fdt(const VOID *fdt);
+EFI_STATUS replace_fdt(const VOID *fdt);

VOID install_initrd_loader(VOID *initrd, UINTN initrd_size);
VOID uninstall_initrd_loader(VOID);
diff --git a/kernel-stub/main.c b/kernel-stub/main.c
index 95a9593..3858b24 100644
--- a/kernel-stub/main.c
+++ b/kernel-stub/main.c
@@ -53,17 +53,22 @@ typedef struct {
static EFI_HANDLE this_image;
static EFI_LOADED_IMAGE kernel_image;

-VOID __attribute__((noreturn)) error_exit(CHAR16 *message, EFI_STATUS status)
+static VOID info(CHAR16 *message)
+{
+ Print(L"Unified kernel stub: %s\n", message);
+}
+
+VOID error(CHAR16 *message, EFI_STATUS status)
{
Print(L"Unified kernel stub: %s (%r).\n", message, status);
(VOID) BS->Stall(3 * 1000 * 1000);
- (VOID) BS->Exit(this_image, status, 0, NULL);
- __builtin_unreachable();
}

-static VOID info(CHAR16 *message)
+VOID __attribute__((noreturn)) error_exit(CHAR16 *message, EFI_STATUS status)
{
- Print(L"Unified kernel stub: %s\n", message);
+ error(message, status);
+ (VOID) BS->Exit(this_image, status, 0, NULL);
+ __builtin_unreachable();
}

static const PE_HEADER *get_pe_header(const VOID *image)
@@ -93,7 +98,7 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)
EFI_LOADED_IMAGE *stub_image;
const PE_HEADER *pe_header;
const SECTION *section;
- EFI_STATUS status, kernel_status;
+ EFI_STATUS status, cleanup_status;
UINTN n;

this_image = image_handle;
@@ -161,12 +166,15 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)
&kernel_handle, &LoadedImageProtocol, &kernel_image,
NULL);
if (EFI_ERROR(status)) {
- uninstall_initrd_loader();
- error_exit(L"Error registering kernel image", status);
+ error(L"Error registering kernel image", status);
+ goto cleanup_initrd;
}

if (alt_fdt) {
- replace_fdt(alt_fdt);
+ status = replace_fdt(alt_fdt);
+ if (EFI_ERROR(status)) {
+ goto cleanup_protocols;
+ }
info(L"Using matched embedded device tree");
} else if (fdt_compatible) {
if (has_dtbs) {
@@ -180,15 +188,20 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)
((UINT8 *) kernel_image.ImageBase +
pe_header->Opt.AddressOfEntryPoint);

- kernel_status = kernel_entry(kernel_handle, system_table);
+ status = kernel_entry(kernel_handle, system_table);

- status = BS->UninstallMultipleProtocolInterfaces(
+cleanup_protocols:
+ cleanup_status = BS->UninstallMultipleProtocolInterfaces(
kernel_handle, &LoadedImageProtocol, &kernel_image,
NULL);
- if (EFI_ERROR(status)) {
- error_exit(L"Error unregistering kernel image", status);
+ if (EFI_ERROR(cleanup_status)) {
+ error(L"Error unregistering kernel image", status);
+ if (!EFI_ERROR(status)) {
+ status = cleanup_status;
+ }
}
+cleanup_initrd:
uninstall_initrd_loader();

- return kernel_status;
+ return status;
}
--
2.35.3

[Jan Kiszka]

From: Jan Kiszka <jan.k...@siemens.com>

error_exit takes care of that.

Signed-off-by: Jan Kiszka <jan.k...@siemens.com>
---

kernel-stub/fdt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel-stub/fdt.c b/kernel-stub/fdt.c
index 11e8857..a832660 100644
--- a/kernel-stub/fdt.c
+++ b/kernel-stub/fdt.c
@@ -140,7 +140,7 @@ BOOLEAN match_fdt(const VOID *fdt, const CHAR8 *compatible)
const CHAR8 *alt_compatible;

if (!compatible) {
- error_exit(L"Found .dtb section but no firmware DTB\n",
+ error_exit(L"Found .dtb section but no firmware DTB",
EFI_NOT_FOUND);
}

--
2.35.3