Hi Jan,
Many thanks for your prompt review and for your guidance
On 7/1/2021 10:29 PM, Jan Kiszka wrote:
On 01.07.21 14:25, Cedric Hombourger wrote:
This driver parses ACPI tables and looks for a "WDAT entry. It
then programs the watchdog using the actions/instructions specified
by the BIOS (where register addresses and values are specified).
Unlike Linux, we not need to create (allocate) a dictionnary of
dictionary
fixed in cover letter
actions as we may just parse the tables on the go to perform our
SET_COUNTDOWN and SET_RUNNING actions. This driver was tested
on SIMATIC IPC127E.
Signed-off-by: Cedric Hombourger <[email protected]>
---
Makefile.am | 1 +
drivers/watchdog/wdat.c | 442 ++++++++++++++++++++++++++++++++++++++++
main.c | 10 +-
3 files changed, 452 insertions(+), 1 deletion(-)
create mode 100644 drivers/watchdog/wdat.c
diff --git a/Makefile.am b/Makefile.am
index 8fb8652..9aae4b4 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -130,6 +130,7 @@ efi_sources = \
drivers/watchdog/atom-quark.c \
drivers/watchdog/ipc4x7e_wdt.c \
drivers/watchdog/itco.c \
+ drivers/watchdog/wdat.c \
Moved above itco.c for wdat.c to take precedence (as you suggested below)
drivers/watchdog/init_array_end.S \
env/syspart.c \
env/fatvars.c \
diff --git a/drivers/watchdog/wdat.c b/drivers/watchdog/wdat.c
new file mode 100644
index 0000000..669c94e
--- /dev/null
+++ b/drivers/watchdog/wdat.c
@@ -0,0 +1,442 @@
+/*
+ * EFI Boot Guard
+ *
+ * Copyright (C) 2021 Mentor Graphics, A Siemens business
+ *
+ * Authors:
+ * Cedric Hombourger <[email protected]>
+ *
+ * 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 <mmio.h>
+#include <sys/io.h>
+#include "utils.h"
+
+#define EFI_ACPI_TABLE_GUID \
+ { 0xeb9d2d30, 0x2d88, 0x11d3, {0x9a, 0x16, 0x0, 0x90, 0x27, 0x3f, 0xc1,
0x4d }}
+#define EFI_ACPI_20_TABLE_GUID \
+ { 0x8868e871, 0xe4f1, 0x11d3, {0xbc, 0x22, 0x0, 0x80, 0xc7, 0x3c, 0x88,
0x81 }}
+
+#define EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER_REVISION 0x02
+
+#define ACPI_SIG_RSDP (CHAR8 *)"RSD PTR "
+#define ACPI_SIG_XSDT (CHAR8 *)"XSDT"
+#define ACPI_SIG_WDAT (CHAR8 *)"WDAT"
+
+typedef struct {
+ CHAR8 Signature[8];
+ UINT8 Checksum;
+ UINT8 OemId[6];
+ UINT8 Revision;
+ UINT32 RsdtAddress;
+ UINT32 Length;
+ UINT64 XsdtAddress;
+ UINT8 ExtendedChecksum;
+ UINT8 Reserved[3];
+} EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER;
+
+
+/*******************************************************************************
+ *
+ * XSDT is the main System Description Table.
+ *
+ * There are many kinds of SDT. An SDT may be split into two parts:
+ * A common header and a data section which is different for each table.
+ *
Where was this taken from? Where is the copyright notice of the source?
I found a description of ACPI tables in a blog (as I did not want or had
the time to go through the Linux kernel code and understand how they do it).
https://blog.fpmurphy.com/2015/01/list-acpi-tables-from-uefi-shell.html
The code provided as example is BSD but the structs are documented in
various specs (e.g.
https://uefi.org/specs/ACPI/6.4/05_ACPI_Software_Programming_Model/ACPI_Software_Programming_Model.html#root-system-description-table-fields-rsdt)
I noticed that most structs in the efibootguard do not use camel case
(the specs do) so I'll go ahead and match the style found in existing
headers (if you are OK)
Do these comments actually contribute to the readability of this driver?
Probably not. I'll remove them
Anything else in this file that was derived from other projects?
I only took the struct definitions from the aforementioned blog but
created our own code around them since we have specific goals (locate
the WDAT tables and parse them)
+
******************************************************************************/
+
+typedef struct {
+ CHAR8 Signature[4];
+ UINT32 Length;
+ UINT8 Revision;
+ UINT8 Checksum;
+ CHAR8 OemId[6];
+ CHAR8 OemTableId[8];
+ UINT32 OemRevision;
+ UINT32 CreatorId;
+ UINT32 CreatorRevision;
+} EFI_ACPI_SDT_HEADER;
+
+#pragma pack(1)
Why not pack the struct above as well?
Probably should
Style-wise, pack annotation on the struct itself looks somehow clearer
than this #pragma thingy, but the codebase is already inconsistent in
this regard.
Ok. I had indeed found that .e.g include/ebgpart.h also used a single
pragma pack for multiple structs. In fact, I did not find a single
occurrence of __attribute__((packed)) in the code base. Are you still
wanting me to switch to per-struct packed constraints? (I am happy
either way)
+
+/*******************************************************************************
+ *
+ * GAS - Generic Address Structure (ACPI 2.0+)
+ *
+ * Note: Since this structure is used in the ACPI tables, it is byte aligned.
+ * If misaligned access is not supported by the hardware, accesses to the
+ * 64-bit Address field must be performed with care.
+ *
+
******************************************************************************/
+
+#define ACPI_ADRR_SPACE_SYSTEM_MEMORY 0
+#define ACPI_ADDR_SPACE_SYSTEM_IO 1
+
+typedef struct {
+ UINT8 space_id; /* Address space where struct or register
exists */
+ UINT8 bit_width; /* Size in bits of given register */
+ UINT8 bit_offset; /* Bit offset within the register */
+ UINT8 access_width; /* Minimum Access size (ACPI 3.0) */
+ UINT64 address; /* 64-bit address of struct or register */
inconsistent indention
fixed
+} ACPI_ADDR;
+
+/* WDAT Instruction Entries (actions) */
+
+typedef struct {
+ UINT8 action;
+ UINT8 instruction;
+ UINT16 reserved;
+ ACPI_ADDR register_region;
+ UINT32 value; /* Value used with Read/Write register */
+ UINT32 mask; /* Bitmask required for this register
instruction */
+} ACPI_WDAT_ENTRY;
+
+/* Values for Action field above */
+
+enum acpi_wdat_actions {
+ ACPI_WDAT_RESET = 1,
+ ACPI_WDAT_SET_COUNTDOWN = 6,
+ ACPI_WDAT_SET_RUNNING_STATE = 9,
+ ACPI_WDAT_SET_REBOOT = 17,
+ ACPI_WDAT_GET_STATUS = 32
+};
+
+/* Values for Instruction field above */
+
+enum acpi_wdat_instructions {
+ ACPI_WDAT_READ_VALUE = 0,
+ ACPI_WDAT_READ_COUNTDOWN = 1,
+ ACPI_WDAT_WRITE_VALUE = 2,
+ ACPI_WDAT_WRITE_COUNTDOWN = 3,
+ ACPI_WDAT_PRESERVE_REGISTER = 0x80
+};
+
+/*******************************************************************************
+ *
+ * WDAT - Watchdog Action Table Version 1
+ *
+ * Conforms to "Hardware Watchdog Timers Design Specification",
+ * Copyright for the specification: 2006 Microsoft Corporation.
+ *
+
******************************************************************************/
+
+typedef struct {
+ EFI_ACPI_SDT_HEADER header; /* Common ACPI table header */
+ UINT32 header_length; /* Watchdog Header Length */
+ UINT16 pci_segment; /* PCI Segment number */
+ UINT8 pci_bus; /* PCI Bus number */
+ UINT8 pci_device; /* PCI Device number */
+ UINT8 pci_function; /* PCI Function number */
+ UINT8 reserved[3];
+ UINT32 timer_period; /* Period of one timer count (msec) */
+ UINT32 max_count; /* Maximum counter value supported */
+ UINT32 min_count; /* Minimum counter value */
+ UINT8 flags;
+ UINT8 reserved2[3];
+ UINT32 entries; /* Number of watchdog entries that follow */
+} ACPI_TABLE_WDAT;
+
+#pragma pack()
+
+static EFI_STATUS
+parse_rsdp(EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER *rsdp, ACPI_TABLE_WDAT
**wdat_table_ptr) {
+ EFI_ACPI_SDT_HEADER *xsdt, *entry;
+ UINT64 *entry_ptr;
+ UINT64 i, count;
"i" suggests signed integer. Better use "n" for unsigned counter vars.
fixed
+
+ if (wdat_table_ptr == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
Over-defensive programming: Caller is local (static...) and doesn't drop
a NULL pointer here.
fixed
+ *wdat_table_ptr = NULL;
+
+ if (rsdp->Revision <
EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER_REVISION) {
+ return EFI_NOT_FOUND;
+ }
+
+ xsdt = (EFI_ACPI_SDT_HEADER *)(rsdp->XsdtAddress);
+ if (strncmpa(ACPI_SIG_XSDT, (CHAR8 *)(VOID *)(xsdt->Signature), 4)) {
+ return EFI_INCOMPATIBLE_VERSION;
+ }
+
+ entry_ptr = (UINT64 *)(xsdt + 1);
+ count = (xsdt->Length - sizeof (EFI_ACPI_SDT_HEADER)) / sizeof(UINT64);
+ for (i=0; i<count; i++, entry_ptr++) {
n = 0; n < count; n++, ...
added spaces around operators
+ entry = (EFI_ACPI_SDT_HEADER *)((UINTN)(*entry_ptr));
+ if (!strncmpa(ACPI_SIG_WDAT, entry->Signature, 4)) {
+ *wdat_table_ptr = (ACPI_TABLE_WDAT *) entry;
+ return EFI_SUCCESS;
+ }
+ }
+ return EFI_NOT_FOUND;
+}
+
+static EFI_STATUS
+locate_and_parse_rsdp(ACPI_TABLE_WDAT **wdat_table_ptr) {
+ EFI_CONFIGURATION_TABLE *ect = ST->ConfigurationTable;
+ EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER *rsdp;
+ EFI_GUID acpi_table_guid = ACPI_TABLE_GUID;
+ EFI_GUID acpi2_table_guid = ACPI_20_TABLE_GUID;
+ UINTN i;
"i" -> "n"
fixed
+
+ for (i=0; i<ST->NumberOfTableEntries; i++) {
+ if ((CompareGuid (&ect->VendorGuid, &acpi_table_guid)) ||
+ (CompareGuid (&ect->VendorGuid, &acpi2_table_guid))) {
+ if (!strncmpa(ACPI_SIG_RSDP, (CHAR8
*)(ect->VendorTable), 8)) {
if ((CompareGuid(&ect->VendorGuid, &acpi_table_guid) ||
CompareGuid(&ect->VendorGuid, &acpi2_table_guid)) &&
!strncmpa(ACPI_SIG_RSDP, (CHAR8 *)(ect->VendorTable), 8)) {
+ rsdp =
(EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER *) ect->VendorTable;
(type *)variable
fixed
+ return parse_rsdp(rsdp, wdat_table_ptr);
+ }
+ }
+ ect++;
+ }
+ return EFI_NOT_FOUND;
+}
+
+static EFI_STATUS
+read_reg(ACPI_ADDR *addr, UINT32 *value_ptr)
+{
+ UINT32 value;
+
+ if ((addr->access_width < 1) || (addr->access_width > 3)) {
Unneeded inner parenthesis.
fixed
+ ERROR(L"invalid width for WDAT read operation!\n");
+ return EFI_UNSUPPORTED;
+ }
+
+ if (addr->space_id == ACPI_ADDR_SPACE_SYSTEM_IO) {
+ switch (addr->access_width) {
+ case 1: value = inb(addr->address); break;
+ case 2: value = inw(addr->address); break;
+ case 3: value = inl(addr->address); break;
Kernel style, please, including no multiple statements per line.
fixed
+ }
+ }
+ else {
+ switch (addr->access_width) {
+ case 1: value = readb(addr->address); break;
+ case 2: value = readw(addr->address); break;
+ case 3: value = readw(addr->address); break;
readl
good catch, fixed
+ }
+ }
+
+ if (value_ptr) {
+ *value_ptr = value;
+ }
+ return EFI_SUCCESS;
+}
+
+static EFI_STATUS
+write_reg(ACPI_ADDR *addr, UINT32 value)
+{
+ if ((addr->access_width < 1) || (addr->access_width > 3)) {
+ ERROR(L"invalid width for WDAT write operation!\n");
+ return EFI_UNSUPPORTED;
+ }
+
+ if (addr->space_id == ACPI_ADDR_SPACE_SYSTEM_IO) {
+ switch (addr->access_width) {
+ case 1: outb(value, addr->address); break;
+ case 2: outw(value, addr->address); break;
+ case 3: outl(value, addr->address); break;
+ }
+ }
+ else {
+ switch (addr->access_width) {
+ case 1: writeb(value, addr->address); break;
+ case 2: writew(value, addr->address); break;
+ case 3: writel(value, addr->address); break;
+ }
+ }
+ return EFI_SUCCESS;
(Almost) same comments as for read_reg.
fixed
+}
+
+static inline EFI_STATUS
Drop the inline, also below. The compile will do that anyway when
appropriate.
fixed
+read_value(ACPI_ADDR *addr, UINT32 value, UINT32 mask, UINT32 *retval)
+{
+ EFI_STATUS status;
+ UINT32 x;
+
+ status = read_reg(addr, &x);
+ if (EFI_ERROR(status)) {
+ return status;
+ }
+ x >>= addr->bit_offset;
+ x &= mask;
+ if (retval) {
+ *retval = (x == value);
+ }
+ return status;
+}
+
+static inline EFI_STATUS
+read_countdown(ACPI_ADDR *addr, UINT32 value, UINT32 mask, UINT32 *retval)
+{
+ EFI_STATUS status;
+ UINT32 x;
+
+ value = value; /* unused */
+ status = read_reg(addr, &x);
+ if (EFI_ERROR(status)) {
+ return status;
+ }
+ x >>= addr->bit_offset;
+ x &= mask;
+ if (retval) {
+ *retval = x;
+ }
+ return status;
+}
+
+static inline EFI_STATUS
+write_value(ACPI_ADDR *addr, UINT32 value, UINT32 mask, BOOLEAN preserve)
+{
+ EFI_STATUS status;
+ UINT32 x, y;
+
+ x = value & mask;
+ x <<= addr->bit_offset;
+ if (preserve) {
+ status = read_reg(addr, &y);
+ if (EFI_ERROR(status)) {
+ return status;
+ }
+ y = y & ~(mask << addr->bit_offset);
+ x |= y;
+ }
+ return write_reg(addr, x);
+}
+
+static EFI_STATUS
+run_action(ACPI_TABLE_WDAT *wdat_table, UINT8 action, UINT32 param, UINT32
*retval)
+{
+ ACPI_WDAT_ENTRY *wdat_entry;
+ ACPI_ADDR *addr;
+ EFI_STATUS status = EFI_UNSUPPORTED;
+ BOOLEAN preserve;
+ UINT32 flags, value, mask;
+ UINTN i;
i -> n
+
+ wdat_entry = (ACPI_WDAT_ENTRY *)(wdat_table + 1);
That "+ 1" may deserve a comment.
Added
/* ACPI_TABLE_WDAT is immediately followed by multiple ACPI_WDAT_ENTRY
tables,
* the former tells us how many (via ->entries) */
+ for (i=0; i<wdat_table->entries; i++, wdat_entry++) {
Style
fixed
+ /* Check if this is the action we have requested */
+ if (wdat_entry->action != action) {
+ continue;
+ }
+
+ /* Decode the action */
+ preserve = (wdat_entry->instruction &
ACPI_WDAT_PRESERVE_REGISTER) != 0;
+ flags = wdat_entry->instruction & ~ACPI_WDAT_PRESERVE_REGISTER;
+ value = wdat_entry->value;
+ mask = wdat_entry->mask;
+ addr = &wdat_entry->register_region;
+
+ /* Operation */
+ switch (flags) {
+ case ACPI_WDAT_READ_VALUE:
One indention level too much.
Applied recommended style to all switch statements
+ status = read_value(addr, value, mask, retval);
+ if (EFI_ERROR(status)) {
+ return status;
+ }
+ break;
+ case ACPI_WDAT_READ_COUNTDOWN:
+ status = read_countdown(addr, value, mask,
retval);
+ if (EFI_ERROR(status)) {
+ return status;
+ }
+ break;
+ case ACPI_WDAT_WRITE_VALUE:
+ status = write_value(addr, value, mask,
preserve);
+ if (EFI_ERROR(status)) {
+ return status;
+ }
+ break;
+ case ACPI_WDAT_WRITE_COUNTDOWN:
+ status = write_value(addr, param, mask,
preserve);
+ if (EFI_ERROR(status)) {
+ return status;
+ }
All EFI_ERROR(status) checks are redundant to the check after the
switch-case - drop them.
indeed - dropped them all!
+ break;
+ default:
+ ERROR(L"Unsupported WDAT instruction %x!\n",
flags);
+ return EFI_UNSUPPORTED;
+ }
+ /* Stop on first error */
+ if (EFI_ERROR(status)) {
+ return status;
+ }
+ }
+ if (status == EFI_UNSUPPORTED) {
+ WARNING(L"action %x isn't supported!\n", action);
WARNING or rather ERROR? We can't and won't continue in that case.
The REBOOT action is not always implemented/found in the ACPI tables.
Going to remove the warning. Our calls to run_action() will log an error
if they want/need to
+ }
+ return status;
+}
+
+static EFI_STATUS __attribute__((constructor))
+init(EFI_PCI_IO *pci_io, UINT16 pci_vendor_id, UINT16 pci_device_id,
+ UINTN timeout)
+{
+ ACPI_TABLE_WDAT *wdat_table;
+ EFI_STATUS status;
+ UINT32 boot_status;
+ UINTN n;
+
+ /* do not probe when scanning PCI devices */
+ if (pci_io || pci_vendor_id || pci_device_id) {
+ return EFI_UNSUPPORTED;
+ }
Move this driver to the front of the driver list, like we did with the
4x7E driver, and then you can drop this.
Thanks for the hint!
+
+ /* Locate WDAT in ACPI tables */
+ status = locate_and_parse_rsdp(&wdat_table);
+ if (EFI_ERROR(status)) {
+ return status;
+ }
+ INFO(L"Detected WDAT watchdog\n");
+
+ /* Check if the boot was caused by the watchdog */
+ status = run_action(wdat_table, ACPI_WDAT_GET_STATUS, 0, &boot_status);
+ if ((status == EFI_SUCCESS) && (boot_status != 0)) {
+ INFO(L"Boot caused watchdog\n");
Boot caused by watchdog
fixed
The kernel driver also calls ACPI_WDAT_SET_STATUS to clear it - do we
have to do that as well?
I don't think so or the kernel will always read 0 and report it to
userspace (via its bootstatus file in sysfs) as a normal boot
+ }
+
+ /* Check period */
+ if (wdat_table->timer_period == 0) {
+ ERROR(L"Invalid WDAT period in ACPI tables!\n");
+ return EFI_INVALID_PARAMETER;
+ }
+
+ /* Compute timeout in periods */
+ n = (timeout * 1000) / wdat_table->timer_period;
+
+ /* Program countdown */
+ status = run_action(wdat_table, ACPI_WDAT_SET_COUNTDOWN, n, NULL);
+ if (EFI_ERROR(status)) {
+ ERROR(L"Could not change WDAT timeout!\n");
+ return status;
+ }
+
+ /* Initial ping with specified timeout */
+ status = run_action(wdat_table, ACPI_WDAT_RESET, n, NULL);
+ if (EFI_ERROR(status)) {
+ ERROR(L"Could not reset WDAT!\n");
+ return status;
+ }
+
+ /* Enable watchdog */
+ status = run_action(wdat_table, ACPI_WDAT_SET_RUNNING_STATE, 0, NULL);
+ if (EFI_ERROR(status)) {
+ ERROR(L"Could not change WDAT to RUNNING state!\n");
+ return status;
+ }
+
+ /* Enable reboot */
+ status = run_action(wdat_table, ACPI_WDAT_SET_REBOOT, 0, NULL);
+ if (EFI_ERROR(status) && (status != EFI_UNSUPPORTED)) {
+ ERROR(L"Could not enable REBOOT for WDAT!\n");
+ return status;
+ }
Where does this initial programming sequence originate from? I didn't
find a word on it in the WDAT doc, and the kernel driver uses a
different one. Are we really free, e.g. in setting ACPI_WDAT_SET_REBOOT
that late?
I also did not find any specs/documents mandating a particular sequence.
I did not think we would need to call RESET (I thought that
SET_COUNTDOWN would suffice)
I should probably move up SET_REBOOT
+
+ return EFI_SUCCESS;
+}
diff --git a/main.c b/main.c
index 7949218..bc4d34e 100644
--- a/main.c
+++ b/main.c
@@ -54,6 +54,14 @@ static EFI_STATUS scan_devices(EFI_LOADED_IMAGE
*loaded_image, UINTN timeout)
EFI_STATUS status;
UINT32 value;
+ /* First try without PCI */
+ status = probe_watchdog(loaded_image, NULL, 0, 0, timeout);
+ if (status == EFI_SUCCESS) {
+ /* Stop if a watchdog was already found */
+ return status;
+ }
+
Not needed when ordering the drivers, as pointed out above. If it were
needed, it had to come as separate patch.
+ /* Check watchdog devices on the PCI bus */
status = uefi_call_wrapper(BS->LocateHandle, 5, ByProtocol,
&PciIoProtocol, NULL, &size, devices);
if (EFI_ERROR(status)) {
@@ -62,7 +70,7 @@ static EFI_STATUS scan_devices(EFI_LOADED_IMAGE
*loaded_image, UINTN timeout)
count = size / sizeof(EFI_HANDLE);
if (count == 0) {
- return probe_watchdog(loaded_image, NULL, 0, 0, timeout);
+ return EFI_NOT_FOUND;
}
do {
Thanks, this is surely a valuable extension of our driver set!
Very welcome. It's been a good exercise and I wanted to show my team
that it isn't that hard (well with coding no longer being my daily job,
there were stylistic / coding issues that I should have caught so thank
you for your patience/review).
I am going to build/test the changes you suggested and post a v2
(hopefully later today)
Jan
--
You received this message because you are subscribed to the Google Groups "EFI Boot
Guard" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/efibootguard-dev/a9a54015-3237-3fc6-686a-1ab6c5962545%40mentor.com.
