[PATCH 1/2] Carve out watchdog arming

[Christian Storm]

From: Christian Storm <christi...@siemens.com>

Carve out EFI Boot Guard's watchdog arming part in preparation to
enable aarch64 support -- which is then naturally restricted to
the switching logics only -- as the watchdog drivers are specific
to x86. For x86, there's no change in behavior.

Signed-off-by: Christian Storm <christi...@siemens.com>
---
Makefile.am | 2 +
include/watchdog.h | 27 +++++++++++++
main.c | 73 +---------------------------------
watchdog.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 127 insertions(+), 72 deletions(-)
create mode 100644 include/watchdog.h
create mode 100644 watchdog.c

diff --git a/Makefile.am b/Makefile.am
index 8347749..91bcf46 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -133,6 +133,7 @@ efi_sources = \
drivers/watchdog/ipc4x7e_wdt.c \
drivers/watchdog/itco.c \
drivers/watchdog/init_array_end.S \
+ watchdog.c \
env/syspart.c \
env/fatvars.c \
utils.c \
@@ -166,6 +167,7 @@ efi_cflags = \
if ARCH_X86_64
efi_cflags += \
-DEFI_FUNCTION_WRAPPER \
+ -DWATCHDOG_DRIVERS \
-mno-red-zone
endif

diff --git a/include/watchdog.h b/include/watchdog.h
new file mode 100644
index 0000000..0f4cc56
--- /dev/null
+++ b/include/watchdog.h
@@ -0,0 +1,27 @@
+/*
+ * EFI Boot Guard
+ *
+ * Copyright (c) Siemens AG, 2021
+ *
+ * Authors:
+ * Christian Storm <christi...@siemens.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See
+ * the COPYING file in the top-level directory.
+ *
+ * SPDX-License-Identifier: GPL-2.0
+ */
+
+#pragma once
+
+#include <efi.h>
+
+#if defined(WATCHDOG_DRIVERS)
+EFI_STATUS scan_devices(EFI_LOADED_IMAGE *loaded_image, UINTN timeout);
+#else
+EFI_STATUS scan_devices(EFI_LOADED_IMAGE __attribute__((__unused__)) *loaded_image,
+ UINTN __attribute__((__unused__)) timeout)
+{
+ return EFI_SUCCESS;
+}
+#endif
diff --git a/main.c b/main.c
index 0bec838..8e03186 100644
--- a/main.c
+++ b/main.c
@@ -20,83 +20,12 @@
#include <pci/header.h>
#include <bootguard.h>
#include <configuration.h>
+#include <watchdog.h>
#include "version.h"
#include "utils.h"

-extern const unsigned long init_array_start[];
-extern const unsigned long init_array_end[];
extern CHAR16 *boot_medium_path;

-static EFI_STATUS probe_watchdog(EFI_LOADED_IMAGE *loaded_image,
- EFI_PCI_IO *pci_io, UINT16 pci_vendor_id,
- UINT16 pci_device_id, UINTN timeout)
-{
- const unsigned long *entry;
-
- for (entry = init_array_start; entry < init_array_end; entry++) {
- EFI_STATUS (*probe)(EFI_PCI_IO *, UINT16, UINT16, UINTN);
-
- probe = loaded_image->ImageBase + *entry;
- if (probe(pci_io, pci_vendor_id, pci_device_id, timeout) ==
- EFI_SUCCESS) {
- return EFI_SUCCESS;
- }
- }
-
- return EFI_UNSUPPORTED;
-}
-
-static EFI_STATUS scan_devices(EFI_LOADED_IMAGE *loaded_image, UINTN timeout)
-{
- EFI_HANDLE devices[1000];
- UINTN count, size = sizeof(devices);
- EFI_PCI_IO *pci_io;
- EFI_STATUS status;
- UINT32 value;
-
- status = uefi_call_wrapper(BS->LocateHandle, 5, ByProtocol,
- &PciIoProtocol, NULL, &size, devices);
- if (EFI_ERROR(status)) {
- return status;
- }
-
- count = size / sizeof(EFI_HANDLE);
- if (count == 0) {
- return probe_watchdog(loaded_image, NULL, 0, 0, timeout);
- }
-
- do {
- EFI_HANDLE device = devices[count - 1];
-
- count--;
-
- status = uefi_call_wrapper(BS->OpenProtocol, 6, device,
- &PciIoProtocol, (VOID **)&pci_io,
- this_image, NULL,
- EFI_OPEN_PROTOCOL_GET_PROTOCOL);
- if (EFI_ERROR(status)) {
- error_exit(L"Cannot open PciIoProtocol while probing watchdogs",
- status);
- }
-
- status = uefi_call_wrapper(pci_io->Pci.Read, 5, pci_io,
- EfiPciIoWidthUint32, PCI_VENDOR_ID,
- 1, &value);
- if (EFI_ERROR(status)) {
- error_exit(L"Cannot read from PCI device while probing watchdogs",
- status);
- }
-
- status = probe_watchdog(loaded_image, pci_io, (UINT16)value,
- value >> 16, timeout);
-
- uefi_call_wrapper(BS->CloseProtocol, 4, device, &PciIoProtocol,
- this_image, NULL);
- } while (status != EFI_SUCCESS && count > 0);
-
- return status;
-}
-
EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)
{
EFI_DEVICE_PATH *payload_dev_path;
diff --git a/watchdog.c b/watchdog.c
new file mode 100644
index 0000000..60fba03
--- /dev/null
+++ b/watchdog.c
@@ -0,0 +1,97 @@
+/*
+ * EFI Boot Guard
+ *
+ * Copyright (c) Siemens AG, 2017
+ *
+ * Authors:
+ * Jan Kiszka <jan.k...@siemens.com>
+ * Andreas Reichel <andreas.r...@siemens.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See
+ * the COPYING file in the top-level directory.
+ *
+ * SPDX-License-Identifier: GPL-2.0
+ */
+
+#include <efi.h>
+#include <efilib.h>
+#include <efiprot.h>
+#include <efipciio.h>
+#include <pci/header.h>
+#include <bootguard.h>
+#include <watchdog.h>
+#include "utils.h"
+
+extern const unsigned long init_array_start[];
+extern const unsigned long init_array_end[];
+
+static EFI_STATUS probe_watchdog(EFI_LOADED_IMAGE *loaded_image,
+ EFI_PCI_IO *pci_io, UINT16 pci_vendor_id,
+ UINT16 pci_device_id, UINTN timeout)
+{
+ const unsigned long *entry;
+
+ for (entry = init_array_start; entry < init_array_end; entry++) {
+ EFI_STATUS (*probe)(EFI_PCI_IO *, UINT16, UINT16, UINTN);
+
+ probe = loaded_image->ImageBase + *entry;
+ if (probe(pci_io, pci_vendor_id, pci_device_id, timeout) ==
+ EFI_SUCCESS) {
+ return EFI_SUCCESS;
+ }
+ }
+
+ return EFI_UNSUPPORTED;
+}
+
+EFI_STATUS scan_devices(EFI_LOADED_IMAGE *loaded_image, UINTN timeout)
+{
+ EFI_HANDLE devices[1000];
+ UINTN count, size = sizeof(devices);
+ EFI_PCI_IO *pci_io;
+ EFI_STATUS status;
+ UINT32 value;
+
+ status = uefi_call_wrapper(BS->LocateHandle, 5, ByProtocol,
+ &PciIoProtocol, NULL, &size, devices);
+ if (EFI_ERROR(status)) {
+ return status;
+ }
+
+ count = size / sizeof(EFI_HANDLE);
+ if (count == 0) {
+ return probe_watchdog(loaded_image, NULL, 0, 0, timeout);
+ }
+
+ do {
+ EFI_HANDLE device = devices[count - 1];
+
+ count--;
+
+ status = uefi_call_wrapper(BS->OpenProtocol, 6, device,
+ &PciIoProtocol, (VOID **)&pci_io,
+ this_image, NULL,
+ EFI_OPEN_PROTOCOL_GET_PROTOCOL);
+ if (EFI_ERROR(status)) {
+ error_exit(L"Cannot open PciIoProtocol while probing watchdogs",
+ status);
+ }
+
+ status = uefi_call_wrapper(pci_io->Pci.Read, 5, pci_io,
+ EfiPciIoWidthUint32, PCI_VENDOR_ID,
+ 1, &value);
+ if (EFI_ERROR(status)) {
+ error_exit(L"Cannot read from PCI device while probing watchdogs",
+ status);
+ }
+
+ status = probe_watchdog(loaded_image, pci_io, (UINT16)value,
+ value >> 16, timeout);
+
+ uefi_call_wrapper(BS->CloseProtocol, 4, device, &PciIoProtocol,
+ this_image, NULL);
+ } while (status != EFI_SUCCESS && count > 0);
+
+ return status;
+}
+
--
2.32.0

[Christian Storm]

From: Christian Storm <christi...@siemens.com>

Adapt the build system to also support aarch64 builds of EFI Boot
Guard without the (x86-specific) watchdog arming part, leaving the
switching logics part only on this platform.
The watchdog arming part on aarch64 has to be provided externally
for now, e.g, by U-Boot (also providing EFI on aarch64).

Signed-off-by: Christian Storm <christi...@siemens.com>
---

Makefile.am | 32 +++++++++++++++++++++++++++-----
configure.ac | 4 ++++
2 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 91bcf46..d27272d 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -124,7 +124,7 @@ efi_loadername = efibootguard$(MACHINE_TYPE_NAME).efi

# NOTE: wdat.c is placed first so it is tried before any other drivers
# NOTE: ipc4x7e_wdt.c must be *before* itco.c
-efi_sources = \
+efi_sources_watchdogs = \
drivers/watchdog/init_array_start.S \
drivers/watchdog/wdat.c \
drivers/watchdog/amdfch_wdt.c \
@@ -133,7 +133,9 @@ efi_sources = \
drivers/watchdog/ipc4x7e_wdt.c \
drivers/watchdog/itco.c \
drivers/watchdog/init_array_end.S \
- watchdog.c \
+ watchdog.c
+
+efi_sources_main = \
env/syspart.c \
env/fatvars.c \
utils.c \
@@ -160,12 +162,12 @@ efi_cflags = \
-fno-strict-aliasing \
-fno-stack-protector \
-Wsign-compare \
- -mno-sse \
- -mno-mmx \
$(CFLAGS)

if ARCH_X86_64
efi_cflags += \
+ -mno-sse \
+ -mno-mmx \
-DEFI_FUNCTION_WRAPPER \
-DWATCHDOG_DRIVERS \
-mno-red-zone
@@ -183,6 +185,26 @@ efi_ldflags = \
-L $(GNUEFI_LIB_DIR) \
$(GNUEFI_LIB_DIR)/crt0-efi-$(ARCH).o

+if ARCH_AARCH64
+# aarch64's objcopy doesn't understand --target efi-[app|bsdrv|rtdrv],
+# hence set subsystem 0xa (EFI application) and binary format.
+objcopy_format = -O binary
+efi_ldflags += --defsym=EFI_SUBSYSTEM=0xa
+else
+objcopy_format = --target=efi-app-$(ARCH)
+endif
+
+# Don't build EFI Boot Guard's watchdog drivers for non-x86 systems.
+if ARCH_X86_64
+efi_sources = \
+ $(efi_sources_watchdogs) \
+ $(efi_sources_main)
+efi_cflags += -DWATCHDOG_DRIVERS
+else
+efi_sources = \
+ $(efi_sources_main)
+endif
+
efi_objects_pre1 = $(efi_sources:.c=.o)
efi_objects_pre2 = $(efi_objects_pre1:.S=.o)
efi_objects = $(addprefix $(top_builddir)/,$(efi_objects_pre2))
@@ -219,7 +241,7 @@ $(efi_solib): $(efi_objects)
$(efi_loadername): $(efi_solib)
$(AM_V_GEN) $(OBJCOPY) -j .text -j .sdata -j .data -j .dynamic \
-j .dynsym -j .rel -j .rela -j .reloc -j .init_array \
- --target=efi-app-$(ARCH) $< $@
+ -j .rela.got -j .rela.data $(objcopy_format) $< $@

$(top_builddir)/tools/bg_setenv-bg_setenv.o: $(GEN_VERSION_H)

diff --git a/configure.ac b/configure.ac
index b8d2b1c..d7a7451 100644
--- a/configure.ac
+++ b/configure.ac
@@ -82,6 +82,7 @@ SET_ARCH(I586, i586*)
SET_ARCH(I686, i686*)
SET_ARCH(X86_64, x86_64*)
SET_ARCH(IA64, ia64*)
+SET_ARCH(AARCH64, aarch64*)

ARCH=$(echo $host | sed "s/\(-\).*$//")

@@ -96,6 +97,9 @@ AM_COND_IF(ARCH_I586, [
AM_COND_IF(ARCH_X86_64, [
MACHINE_TYPE_NAME=x64])

+AM_COND_IF(ARCH_AARCH64, [
+ MACHINE_TYPE_NAME=aarch64])
+
AC_SUBST([ARCH])
AC_SUBST([MACHINE_TYPE_NAME])

--
2.32.0

[Jan Kiszka]

On 18.08.21 16:04, Christian Storm wrote:
> From: Christian Storm <christi...@siemens.com>
>
> Carve out EFI Boot Guard's watchdog arming part in preparation to
> enable aarch64 support -- which is then naturally restricted to
> the switching logics only -- as the watchdog drivers are specific
> to x86. For x86, there's no change in behavior.
>

This is pragmatic but not future-proof. Eventually, we will be
confronted with non-U-Boot UEFI firmware that probably will not have
proper watchdog support.

Just build an empty driver list for now but excluding the x86-targeting
drivers from the build. Should also be simpler.

Jan

--
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

[Christian Storm]

Hi Jan,

> > Carve out EFI Boot Guard's watchdog arming part in preparation to
> > enable aarch64 support -- which is then naturally restricted to
> > the switching logics only -- as the watchdog drivers are specific
> > to x86. For x86, there's no change in behavior.
> >
>
> This is pragmatic but not future-proof. Eventually, we will be
> confronted with non-U-Boot UEFI firmware that probably will not have
> proper watchdog support.
>
> Just build an empty driver list for now but excluding the x86-targeting
> drivers from the build. Should also be simpler.

Well, the probing part in scan_devices() may as well be specific to
the platform then. So you may probably end up with multiple/different
implementations of scan_devices() depending on the platform ― besides
the platform-specific hard-wired list of watchdog drivers.
As we don't have this now, I opted for this to not introduce boilerplate.

Either way, your suggestion is then to have an empty watchdog driver list for
aarch64 and still call scan_devices() which in turn calls probe_watchdog()
for each successfully enumerated PCI device, knowing that this is void?


Aside, scan_devices() is and has been a misnomer as it (mediately) does
more than just scanning devices...


Kind regards,
Christian

--
Dr. Christian Storm
Siemens AG, Technology, T RDA IOT SES-DE
Otto-Hahn-Ring 6, 81739 München, Germany

[Jan Kiszka]

On 18.08.21 18:08, Christian Storm wrote:
> Hi Jan,
>
>>> Carve out EFI Boot Guard's watchdog arming part in preparation to
>>> enable aarch64 support -- which is then naturally restricted to
>>> the switching logics only -- as the watchdog drivers are specific
>>> to x86. For x86, there's no change in behavior.
>>>
>>
>> This is pragmatic but not future-proof. Eventually, we will be
>> confronted with non-U-Boot UEFI firmware that probably will not have
>> proper watchdog support.
>>
>> Just build an empty driver list for now but excluding the x86-targeting
>> drivers from the build. Should also be simpler.
>
> Well, the probing part in scan_devices() may as well be specific to
> the platform then. So you may probably end up with multiple/different
> implementations of scan_devices() depending on the platform ― besides
> the platform-specific hard-wired list of watchdog drivers.
> As we don't have this now, I opted for this to not introduce boilerplate.
>
> Either way, your suggestion is then to have an empty watchdog driver list for
> aarch64 and still call scan_devices() which in turn calls probe_watchdog()
> for each successfully enumerated PCI device, knowing that this is void?
>

Exactly, as that is much simpler and will not require moving back all
the code when adding the first driver to aarch64, which will likely not
be PCI based for a while, but that does not matter.

I suspect the PCI part of the probing will run rather quickly on most
devices as PCI is not supported, at least not by the firmware. THAT may
actually require some additional change in order to handle a potentially
missing PCI protocol gracefully.

>
> Aside, scan_devices() is and has been a misnomer as it (mediately) does
> more than just scanning devices...
>

Feel free to rename it to probe_devices.

[Jan Kiszka]

Oh, and another reason to not go your path: watchdog timeout != 0 must
keep the semantic "I expect a watchdog to be there" and must error-out
if that is not the case.

Jan

[Christian Storm]

From: Christian Storm <christi...@siemens.com>

Adapt the build system to also support aarch64 builds of EFI Boot
Guard without the (x86-specific) watchdog arming part, leaving the
switching logics part only on this platform.
The watchdog arming part on aarch64 has to be provided externally
for now, e.g, by U-Boot (also providing EFI on aarch64).

Signed-off-by: Christian Storm <christi...@siemens.com>
---

Makefile.am | 28 ++++++++++++++++++++++------
configure.ac | 4 ++++
drivers/watchdog/init_array_start.S | 6 +++++-
3 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 8347749..121c7ed 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -122,16 +122,23 @@ install-exec-hook:
#
efi_loadername = efibootguard$(MACHINE_TYPE_NAME).efi

+if ARCH_AARCH64
+efi_sources_watchdogs =
+else

# NOTE: wdat.c is placed first so it is tried before any other drivers
# NOTE: ipc4x7e_wdt.c must be *before* itco.c
-efi_sources = \

- drivers/watchdog/init_array_start.S \
+efi_sources_watchdogs = \
drivers/watchdog/wdat.c \
drivers/watchdog/amdfch_wdt.c \
drivers/watchdog/i6300esb.c \
drivers/watchdog/atom-quark.c \
drivers/watchdog/ipc4x7e_wdt.c \
- drivers/watchdog/itco.c \
+ drivers/watchdog/itco.c
+endif
+
+efi_sources = \
+ drivers/watchdog/init_array_start.S \
+ $(efi_sources_watchdogs) \
drivers/watchdog/init_array_end.S \
env/syspart.c \
env/fatvars.c \
@@ -159,12 +166,12 @@ efi_cflags = \

-fno-strict-aliasing \
-fno-stack-protector \
-Wsign-compare \
- -mno-sse \
- -mno-mmx \
$(CFLAGS)

if ARCH_X86_64
efi_cflags += \
+ -mno-sse \
+ -mno-mmx \
-DEFI_FUNCTION_WRAPPER \

-mno-red-zone
endif
@@ -181,6 +188,15 @@ efi_ldflags = \

-L $(GNUEFI_LIB_DIR) \
$(GNUEFI_LIB_DIR)/crt0-efi-$(ARCH).o

+if ARCH_AARCH64
+# aarch64's objcopy doesn't understand --target efi-[app|bsdrv|rtdrv],
+# hence set subsystem 0xa (EFI application) and binary format.
+objcopy_format = -O binary
+efi_ldflags += --defsym=EFI_SUBSYSTEM=0xa
+else
+objcopy_format = --target=efi-app-$(ARCH)
+endif
+

efi_objects_pre1 = $(efi_sources:.c=.o)
efi_objects_pre2 = $(efi_objects_pre1:.S=.o)
efi_objects = $(addprefix $(top_builddir)/,$(efi_objects_pre2))

@@ -217,7 +233,7 @@ $(efi_solib): $(efi_objects)

$(efi_loadername): $(efi_solib)
$(AM_V_GEN) $(OBJCOPY) -j .text -j .sdata -j .data -j .dynamic \
-j .dynsym -j .rel -j .rela -j .reloc -j .init_array \
- --target=efi-app-$(ARCH) $< $@
+ -j .rela.got -j .rela.data $(objcopy_format) $< $@

$(top_builddir)/tools/bg_setenv-bg_setenv.o: $(GEN_VERSION_H)

diff --git a/configure.ac b/configure.ac
index b8d2b1c..d7a7451 100644
--- a/configure.ac
+++ b/configure.ac
@@ -82,6 +82,7 @@ SET_ARCH(I586, i586*)
SET_ARCH(I686, i686*)
SET_ARCH(X86_64, x86_64*)
SET_ARCH(IA64, ia64*)
+SET_ARCH(AARCH64, aarch64*)

ARCH=$(echo $host | sed "s/\(-\).*$//")

@@ -96,6 +97,9 @@ AM_COND_IF(ARCH_I586, [
AM_COND_IF(ARCH_X86_64, [
MACHINE_TYPE_NAME=x64])

+AM_COND_IF(ARCH_AARCH64, [
+ MACHINE_TYPE_NAME=aarch64])
+
AC_SUBST([ARCH])
AC_SUBST([MACHINE_TYPE_NAME])

diff --git a/drivers/watchdog/init_array_start.S b/drivers/watchdog/init_array_start.S
index 61a6780..6d4b7cd 100644
--- a/drivers/watchdog/init_array_start.S
+++ b/drivers/watchdog/init_array_start.S
@@ -13,7 +13,11 @@
*/

.section .init_array
+/* Dummy value to avoid zero-size .init_array section linker error on
+ * architectures having no watchdog drivers */
+.word 0x5343
/* align to 4K so that signing tools will not stumble over this section */
-.align 0x1000
+/* Note: Explicitly use .b(yte-)align as .align takes power of 2 on ARM */
+.balign 0x1000
.global init_array_start
init_array_start:
--
2.33.0

[Jan Kiszka]

Thanks, applied.

I quickly wanted to add CI support as well but first stumbled over
Ubuntu cross-compilation complexity, then tried arm64 native build, just
to find out that we have no OSS credits on travis anymore. Seems
migration to github, including coverity, is no longer avoidable.