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 > 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 \ > 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? 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 > +} 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. > + > + 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 > + } > + } > + > + 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. > +} > + > +static inline EFI_STATUS Drop the inline, also below. The compile will do that anyway when appropriate. > +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. > + 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? > + } > + > + /* 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? > + > + 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 -- 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/bee41f6d-e2cb-003f-f770-2c439fea196c%40siemens.com.
