[PATCH] drivers/watchdog: add support for WDAT (ACPI) watchdogs

[Cedric Hombourger]

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
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 <Cedric_H...@mentor.com>
---
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 \
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 <Cedric_H...@mentor.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 <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.
+ *
+ ******************************************************************************/
+
+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)
+
+/*******************************************************************************
+ *
+ * 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 */
+} 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;
+
+ if (wdat_table_ptr == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+ *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++) {
+ 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;
+
+ 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)) {
+ rsdp = (EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER *) ect->VendorTable;
+ 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)) {
+ 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;
+ }
+ }
+ 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;
+ }
+ }
+
+ 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;
+}
+
+static inline EFI_STATUS
+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;
+
+ wdat_entry = (ACPI_WDAT_ENTRY *)(wdat_table + 1);
+ for (i=0; i<wdat_table->entries; i++, wdat_entry++) {
+ /* 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:
+ 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;
+ }
+ 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);
+ }
+ 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;
+ }
+
+ /* 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");
+ }
+
+ /* 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;
+ }
+
+ 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;
+ }
+
+ /* 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 {
--
2.30.2

[Jan Kiszka]

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

Where was this taken from? Where is the copyright notice of the source?
Do these comments actually contribute to the readability of this driver?

Anything else in this file that was derived from other projects?

> + ******************************************************************************/
> +
> +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?

Style-wise, pack annotation on the struct itself looks somehow clearer
than this #pragma thingy, but the codebase is already inconsistent in
this regard.

> +
> +/*******************************************************************************
> + *
> + * 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

"i" suggests signed integer. Better use "n" for unsigned counter vars.

> +
> + if (wdat_table_ptr == NULL) {
> + return EFI_INVALID_PARAMETER;
> + }

Over-defensive programming: Caller is local (static...) and doesn't drop
a NULL pointer here.

> + *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++, ...

> + 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"

> +
> + 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

> + 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.

> + 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.

> + }
> + }
> + 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

(Almost) same comments as for read_reg.

> +}
> +
> +static inline EFI_STATUS

Drop the inline, also below. The compile will do that anyway when
appropriate.

i -> n

> +
> + wdat_entry = (ACPI_WDAT_ENTRY *)(wdat_table + 1);

That "+ 1" may deserve a comment.

> + for (i=0; i<wdat_table->entries; i++, wdat_entry++) {

Style

> + /* 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.

> + 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.

> + 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.

> + }
> + 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.

> +
> + /* 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

The kernel driver also calls ACPI_WDAT_SET_STATUS to clear it - do we
have to do that as well?

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?

> +
> + 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!

Jan

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

[Cedric Hombourger]

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 <Cedric_H...@mentor.com>
>> ---
>> 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)

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

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

fixed

>
>> +}
>> +
>> +static inline EFI_STATUS
> Drop the inline, also below. The compile will do that anyway when
> appropriate.

fixed

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

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

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
>

[Cedric Hombourger]

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 dictionary of

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 <Cedric_H...@mentor.com>
---
Makefile.am | 1 +

drivers/watchdog/wdat.c | 425 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 426 insertions(+)

create mode 100644 drivers/watchdog/wdat.c

diff --git a/Makefile.am b/Makefile.am

index 8fb8652..6d61cc4 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -129,6 +129,7 @@ efi_sources = \
drivers/watchdog/i6300esb.c \
drivers/watchdog/atom-quark.c \
drivers/watchdog/ipc4x7e_wdt.c \
+ drivers/watchdog/wdat.c \
drivers/watchdog/itco.c \
drivers/watchdog/init_array_end.S \
env/syspart.c \

diff --git a/drivers/watchdog/wdat.c b/drivers/watchdog/wdat.c
new file mode 100644

index 0000000..8fda156
--- /dev/null
+++ b/drivers/watchdog/wdat.c
@@ -0,0 +1,425 @@

+#define EFI_ACPI_ROOT_SDP_REVISION 0x02

+
+#define ACPI_SIG_RSDP (CHAR8 *)"RSD PTR "
+#define ACPI_SIG_XSDT (CHAR8 *)"XSDT"
+#define ACPI_SIG_WDAT (CHAR8 *)"WDAT"
+

+#pragma pack(1)
+
+/* Root System Description Pointer */
+typedef struct {
+ CHAR8 signature[8];
+ UINT8 checksum;
+ UINT8 oem_id[6];
+ UINT8 revision;
+ UINT32 rsdt_address;
+ UINT32 length;
+ UINT64 xsdt_address;
+ UINT8 extended_checksum;
+ UINT8 reserved[3];
+} EFI_ACPI_ROOT_SDP_HEADER;
+
+/* System Description Table */
+typedef struct {
+ CHAR8 signature[4];
+ UINT32 length;
+ UINT8 revision;
+ UINT8 checksum;
+ CHAR8 oem_id[6];
+ CHAR8 oem_table_id[8];
+ UINT32 oem_revision;
+ UINT32 creator_id;
+ UINT32 creator_revision;
+} EFI_ACPI_SDT_HEADER;
+
+/* Generic Address Structure */

+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 */

+} ACPI_ADDR;
+
+/* Values for space_id field above */
+enum acpi_spaces {
+ ACPI_ADRR_SPACE_SYSTEM_MEMORY = 0,
+ ACPI_ADDR_SPACE_SYSTEM_IO = 1
+};

+
+/* 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
+};
+

+/* Watchdog Action Table */

+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_ROOT_SDP_HEADER *rsdp, ACPI_TABLE_WDAT **wdat_table_ptr) {

+ EFI_ACPI_SDT_HEADER *xsdt, *entry;
+ UINT64 *entry_ptr;

+ UINT64 n, count;
+

+ *wdat_table_ptr = NULL;
+

+ if (rsdp->revision < EFI_ACPI_ROOT_SDP_REVISION) {

+ return EFI_NOT_FOUND;
+ }
+

+ xsdt = (EFI_ACPI_SDT_HEADER *)(rsdp->xsdt_address);
+ 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 (n = 0; n < count; n++, entry_ptr++) {

+ 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_ROOT_SDP_HEADER *rsdp;

+ EFI_GUID acpi_table_guid = ACPI_TABLE_GUID;
+ EFI_GUID acpi2_table_guid = ACPI_20_TABLE_GUID;

+ UINTN n;
+
+ for (n = 0; n < ST->NumberOfTableEntries; n++) {

+ if ((CompareGuid (&ect->VendorGuid, &acpi_table_guid)) ||
+ (CompareGuid (&ect->VendorGuid, &acpi2_table_guid))) {
+ if (!strncmpa(ACPI_SIG_RSDP, (CHAR8 *)(ect->VendorTable), 8)) {

+ rsdp = (EFI_ACPI_ROOT_SDP_HEADER *)ect->VendorTable;

+ 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) {

+ 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;

+ }
+ }
+ else {
+ switch (addr->access_width) {
+ case 1:

+ value = readb(addr->address);
+ break;
+ case 2:
+ value = readw(addr->address);
+ break;
+ case 3:
+ value = readl(addr->address);
+ break;

+ }
+ }
+
+ 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;

+}
+
+static EFI_STATUS

+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 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 EFI_STATUS

+ UINTN n;
+
+ /* ACPI_TABLE_WDAT is immediately followed by multiple ACPI_WDAT_ENTRY tables,
+ * the former tells us how many (via ->entries). */

+ wdat_entry = (ACPI_WDAT_ENTRY *)(wdat_table + 1);

+ for (n = 0; n < wdat_table->entries; n++, wdat_entry++) {

+ /* 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:

+ status = read_value(addr, value, mask, retval);

+ break;
+ case ACPI_WDAT_READ_COUNTDOWN:
+ status = read_countdown(addr, value, mask, retval);

+ break;
+ case ACPI_WDAT_WRITE_VALUE:
+ status = write_value(addr, value, mask, preserve);

+ break;
+ case ACPI_WDAT_WRITE_COUNTDOWN:
+ status = write_value(addr, param, mask, preserve);

+ break;
+ default:
+ ERROR(L"Unsupported WDAT instruction %x!\n", flags);
+ return EFI_UNSUPPORTED;
+ }
+ /* Stop on first error */
+ if (EFI_ERROR(status)) {
+ return status;
+ }
+ }

+ 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;
+ }

+
+ /* 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 by watchdog\n");
+ }
+

+ return EFI_SUCCESS;
+}
--
2.30.2

[Cedric Hombourger]

Changes in v2:
- addressed stylistic review comments from Jan
- (TYPE *)var instead of (TYPE *) var
- do not indent "case" in switch statements
- remove over-defensive if statements
- let the compiler decide when to inline

- move REBOOT action up
- no longer print a warning for unsupported actions (let callers do
that)
- removed changes to main.c
- moved wdat.c before itco.c so it takes precedence at runtime
- call readl() (as expected) for width == 3

New revision of this patch tested on SIMATIC IPC127E.

Cedric Hombourger (1):
drivers/watchdog: add support for WDAT (ACPI) watchdogs

Makefile.am | 1 +
drivers/watchdog/wdat.c | 425 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 426 insertions(+)
create mode 100644 drivers/watchdog/wdat.c

--
2.30.2

[Jan Kiszka]

On 02.07.21 11:02, 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 dictionary of
> 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 <Cedric_H...@mentor.com>
> ---
> Makefile.am | 1 +
> drivers/watchdog/wdat.c | 425 ++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 426 insertions(+)
> create mode 100644 drivers/watchdog/wdat.c
>
> diff --git a/Makefile.am b/Makefile.am
> index 8fb8652..6d61cc4 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -129,6 +129,7 @@ efi_sources = \
> drivers/watchdog/i6300esb.c \
> drivers/watchdog/atom-quark.c \
> drivers/watchdog/ipc4x7e_wdt.c \
> + drivers/watchdog/wdat.c \

You should leave a comment like for the ipc4x7e_wdt.c on the required
ordering.

And I would actually restore your original prioritization of WDAT over
anything else, i.e. move this right after init_array_start.S. Or do you
see any reason we should generally prefer the specific PCI drivers over
WDAT? Quirks could still be encoded by adding some excludion to the WDAT
driver - if that should ever be needed.

> drivers/watchdog/itco.c \
> drivers/watchdog/init_array_end.S \
> env/syspart.c \
> diff --git a/drivers/watchdog/wdat.c b/drivers/watchdog/wdat.c
> new file mode 100644
> index 0000000..8fda156
> --- /dev/null
> +++ b/drivers/watchdog/wdat.c
> @@ -0,0 +1,425 @@
> +/*
> + * EFI Boot Guard
> + *
> + * Copyright (C) 2021 Mentor Graphics, A Siemens business
> + *

Again, you need to add copyright notices for the origin of these structs
with their comments. Could be linux/include/acpi/actbl.h.

You didn't incorporate my refactoring suggestion.

[Cedric Hombourger]

Changes in v3:
- place wdat.c first and note its precedence as done for ipc4x7e_wdt.c

Changes in v2:
- addressed stylistic review comments from Jan
- (TYPE *)var instead of (TYPE *) var
- do not indent "case" in switch statements
- remove over-defensive if statements
- let the compiler decide when to inline

- move REBOOT action up
- no longer print a warning for unsupported actions (let callers do
that)
- removed changes to main.c
- moved wdat.c before itco.c so it takes precedence at runtime
- call readl() (as expected) for width == 3

New revision of this patch tested on SIMATIC IPC127E.


Cedric Hombourger (1):
drivers/watchdog: add support for WDAT (ACPI) watchdogs

Makefile.am | 2 +
drivers/watchdog/wdat.c | 425 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 427 insertions(+)
create mode 100644 drivers/watchdog/wdat.c

--
2.30.2

[Cedric Hombourger]

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 dictionary of
actions as we may just parse the tables on the go to perform our

SET_COUNTDOWN and SET_RUNNING actions. Note that this driver is
tried before any others (the BIOS is expected to expose ACPI tables
with valid information). This driver was tested on SIMATIC IPC127E.

Signed-off-by: Cedric Hombourger <Cedric_H...@mentor.com>
---

Makefile.am | 2 +
drivers/watchdog/wdat.c | 425 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 427 insertions(+)
create mode 100644 drivers/watchdog/wdat.c

diff --git a/Makefile.am b/Makefile.am
index 8fb8652..8347749 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -122,9 +122,11 @@ install-exec-hook:
#
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 = \
drivers/watchdog/init_array_start.S \
+ drivers/watchdog/wdat.c \
drivers/watchdog/amdfch_wdt.c \
drivers/watchdog/i6300esb.c \
drivers/watchdog/atom-quark.c \

diff --git a/drivers/watchdog/wdat.c b/drivers/watchdog/wdat.c
new file mode 100644
index 0000000..8fda156
--- /dev/null
+++ b/drivers/watchdog/wdat.c
@@ -0,0 +1,425 @@
+/*
+ * EFI Boot Guard
+ *
+ * Copyright (C) 2021 Mentor Graphics, A Siemens business
+ *

--
2.30.2

[Jan Kiszka]

On 05.07.21 10:22, Cedric Hombourger wrote:
> Changes in v3:
> - place wdat.c first and note its precedence as done for ipc4x7e_wdt.c
>

Thoroughness trumps speed: Please read and address ALL my remarks.

Jan

> Changes in v2:
> - addressed stylistic review comments from Jan
> - (TYPE *)var instead of (TYPE *) var
> - do not indent "case" in switch statements
> - remove over-defensive if statements
> - let the compiler decide when to inline
>
> - move REBOOT action up
> - no longer print a warning for unsupported actions (let callers do
> that)
> - removed changes to main.c
> - moved wdat.c before itco.c so it takes precedence at runtime
> - call readl() (as expected) for width == 3
>
> New revision of this patch tested on SIMATIC IPC127E.
>
>
> Cedric Hombourger (1):
> drivers/watchdog: add support for WDAT (ACPI) watchdogs
>
> Makefile.am | 2 +
> drivers/watchdog/wdat.c | 425 ++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 427 insertions(+)
> create mode 100644 drivers/watchdog/wdat.c
>

--

[Cedric Hombourger]

Changes in v4:
- drop check for pci_io/pci_vendor_id/pci_device_id since init will
be called with these being non-zero when a PCI bus is detected
(we may still ignore these)
- refactor lookup for RSDP as suggested by Jan (merge 2 "if" clauses
into one
- add reference to Linux WDAT structures that were imported into this
driver
- add references to ACPI specs for structs we use (RSDP, XSDP, GAS)

Changes in v3:
- place wdat.c first and note its precedence as done for ipc4x7e_wdt.c

Changes in v2:
- addressed stylistic review comments from Jan
- (TYPE *)var instead of (TYPE *) var
- do not indent "case" in switch statements
- remove over-defensive if statements
- let the compiler decide when to inline

- move REBOOT action up
- no longer print a warning for unsupported actions (let callers do
that)
- removed changes to main.c
- moved wdat.c before itco.c so it takes precedence at runtime
- call readl() (as expected) for width == 3

New revision of this patch tested on SIMATIC IPC127E.

Cedric Hombourger (1):
drivers/watchdog: add support for WDAT (ACPI) watchdogs

Makefile.am | 2 +

drivers/watchdog/wdat.c | 439 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 441 insertions(+)

create mode 100644 drivers/watchdog/wdat.c

--

2.30.2

[Cedric Hombourger]

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 dictionary of
actions as we may just parse the tables on the go to perform our
SET_COUNTDOWN and SET_RUNNING actions. Note that this driver is
tried before any others (the BIOS is expected to expose ACPI tables
with valid information). This driver was tested on SIMATIC IPC127E.

Signed-off-by: Cedric Hombourger <Cedric_H...@mentor.com>
---
Makefile.am | 2 +

drivers/watchdog/wdat.c | 442 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 444 insertions(+)
create mode 100644 drivers/watchdog/wdat.c

diff --git a/Makefile.am b/Makefile.am
index 8fb8652..8347749 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -122,9 +122,11 @@ install-exec-hook:
#
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 = \
drivers/watchdog/init_array_start.S \
+ drivers/watchdog/wdat.c \
drivers/watchdog/amdfch_wdt.c \
drivers/watchdog/i6300esb.c \
drivers/watchdog/atom-quark.c \
diff --git a/drivers/watchdog/wdat.c b/drivers/watchdog/wdat.c
new file mode 100644

index 0000000..fe5d8a7
--- /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 <Cedric_H...@mentor.com>
+ *

+ * This driver includes WDAT structs from linux/include/acpi/actbl.h (GPL-2.0)
+ * ACPI structs were created from https://uefi.org/specs/ACPI/6.4/index.html

+/* --------------------------------------------------------------------------
+ * ACPI structures
+ * Reference: https://uefi.org/specs/ACPI/6.4/index.html
+ * --------------------------------------------------------------------------
+ */
+
+/* Root System Description Pointer (ACPI section 5.2.5.3) */

+typedef struct {
+ CHAR8 signature[8];
+ UINT8 checksum;
+ UINT8 oem_id[6];
+ UINT8 revision;
+ UINT32 rsdt_address;
+ UINT32 length;
+ UINT64 xsdt_address;
+ UINT8 extended_checksum;
+ UINT8 reserved[3];
+} EFI_ACPI_ROOT_SDP_HEADER;
+

+/* System Description Table (ACPI section 5.2.6) */

+typedef struct {
+ CHAR8 signature[4];
+ UINT32 length;
+ UINT8 revision;
+ UINT8 checksum;
+ CHAR8 oem_id[6];
+ CHAR8 oem_table_id[8];
+ UINT32 oem_revision;
+ UINT32 creator_id;
+ UINT32 creator_revision;
+} EFI_ACPI_SDT_HEADER;
+

+/* Generic Address Structure (ACPI section 5.2.3.2) */

+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 */
+} ACPI_ADDR;
+
+/* Values for space_id field above */
+enum acpi_spaces {
+ ACPI_ADRR_SPACE_SYSTEM_MEMORY = 0,
+ ACPI_ADDR_SPACE_SYSTEM_IO = 1
+};
+

+/* --------------------------------------------------------------------------
+ * WDAT structures
+ * Reference: linux/include/acpi/actbl.h (GPL-2.0)
+ * --------------------------------------------------------------------------
+ */

+/* --------------------------------------------------------------------------
+ * Parsing of ACPI/WDAT structures for efibootguard
+ * --------------------------------------------------------------------------
+ */

+ CompareGuid(&ect->VendorGuid, &acpi2_table_guid)) &&
+ !strncmpa(ACPI_SIG_RSDP, (CHAR8 *)(ect->VendorTable), 8)) {

+ /* unused */
+ pci_io = pci_io; pci_vendor_id = pci_vendor_id; pci_device_id = pci_device_id;

[Jan Kiszka]

Applied to next, with the the following cppcheck warning [1] fixes:

diff --git a/drivers/watchdog/wdat.c b/drivers/watchdog/wdat.c
index fe5d8a7..4bb7362 100644
--- a/drivers/watchdog/wdat.c
+++ b/drivers/watchdog/wdat.c
@@ -141,7 +141,7 @@ typedef struct {

static EFI_STATUS
parse_rsdp(EFI_ACPI_ROOT_SDP_HEADER *rsdp, ACPI_TABLE_WDAT **wdat_table_ptr) {
- EFI_ACPI_SDT_HEADER *xsdt, *entry;
+ EFI_ACPI_SDT_HEADER *xsdt;
UINT64 *entry_ptr;
UINT64 n, count;

@@ -159,7 +159,8 @@ parse_rsdp(EFI_ACPI_ROOT_SDP_HEADER *rsdp, ACPI_TABLE_WDAT **wdat_table_ptr) {

entry_ptr = (UINT64 *)(xsdt + 1);

count = (xsdt->length - sizeof (EFI_ACPI_SDT_HEADER)) / sizeof(UINT64);

for (n = 0; n < count; n++, entry_ptr++) {

- entry = (EFI_ACPI_SDT_HEADER *)((UINTN)(*entry_ptr));
+ EFI_ACPI_SDT_HEADER *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;
@@ -287,12 +288,11 @@ read_value(ACPI_ADDR *addr, UINT32 value, UINT32 mask, UINT32 *retval)
}

static EFI_STATUS
-read_countdown(ACPI_ADDR *addr, UINT32 value, UINT32 mask, UINT32 *retval)
+read_countdown(ACPI_ADDR *addr, UINT32 mask, UINT32 *retval)
{
EFI_STATUS status;
UINT32 x;

- value = value; /* unused */
status = read_reg(addr, &x);
if (EFI_ERROR(status)) {
return status;
@@ -308,13 +308,12 @@ read_countdown(ACPI_ADDR *addr, UINT32 value, UINT32 mask, UINT32 *retval)
static 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);
+ EFI_STATUS status = read_reg(addr, &y);
if (EFI_ERROR(status)) {
return status;
}
@@ -356,7 +355,7 @@ run_action(ACPI_TABLE_WDAT *wdat_table, UINT8 action, UINT32 param, UINT32 *retv

status = read_value(addr, value, mask, retval);

break;
case ACPI_WDAT_READ_COUNTDOWN:
- status = read_countdown(addr, value, mask, retval);
+ status = read_countdown(addr, mask, retval);
break;
case ACPI_WDAT_WRITE_VALUE:

status = write_value(addr, value, mask, preserve);

@@ -377,7 +376,9 @@ run_action(ACPI_TABLE_WDAT *wdat_table, UINT8 action, UINT32 param, UINT32 *retv
}

static EFI_STATUS __attribute__((constructor))
-init(EFI_PCI_IO *pci_io, UINT16 pci_vendor_id, UINT16 pci_device_id,
+init(EFI_PCI_IO __attribute__((unused)) *pci_io,
+ UINT16 __attribute__((unused)) pci_vendor_id,
+ UINT16 __attribute__((unused)) pci_device_id,
UINTN timeout)
{
ACPI_TABLE_WDAT *wdat_table;
@@ -385,9 +386,6 @@ init(EFI_PCI_IO *pci_io, UINT16 pci_vendor_id, UINT16 pci_device_id,
UINT32 boot_status;
UINTN n;

- /* unused */
- pci_io = pci_io; pci_vendor_id = pci_vendor_id; pci_device_id = pci_device_id;
-

/* Locate WDAT in ACPI tables */

status = locate_and_parse_rsdp(&wdat_table);
if (EFI_ERROR(status)) {

Thanks,
Jan

[1] https://travis-ci.com/github/siemens/efibootguard/jobs/521956855

[Cedric Hombourger]

All look good to me, thank you Jan