[PATCH 0/2] IPMI watchdog support

[Henning Schild]

These patches add support for IPMI watchdogs typically found in
server-class hardware. Machines having IPMI will in any version have a
watchdog as well.

The Linux iTCO driver has special vendor logic for Supermicro, where the
iTCO does not work in favour of IPMI. So we place that one before itco
like done with the IPC 427E.

This driver has error handling which was kind of hard to test, but the
code can be easily placed into a user-space application with iopl(3) and
"no action". Where ipmitool and other helpers can be used.
The error handling also means we could end up in the unlikely and
unfortunate situation where we would have to boot without the watchdog
being actually armed. For testing i modified the code to cause some
errors, and the retries and recovery worked. So i hope it would be
highly unlikely that we fail to arm.

p1 is unrelated, just a drive-by finding

Henning Schild (3):
gitignore: add pkgconf wildcard
Move smbios helper function into utils.
watchdog: ipmi: Add driver for machines with IPMI

.gitignore | 1 +
Makefile.am | 4 +-
drivers/watchdog/.ipmi_wdt.c.swn | Bin 0 -> 12288 bytes
drivers/watchdog/ipc4x7e_wdt.c | 25 ----
drivers/watchdog/ipmi_wdt.c | 193 +++++++++++++++++++++++++++++++
include/utils.h | 4 +
utils.c | 25 ++++
7 files changed, 226 insertions(+), 26 deletions(-)
create mode 100644 drivers/watchdog/.ipmi_wdt.c.swn
create mode 100644 drivers/watchdog/ipmi_wdt.c

--
2.39.3

[Henning Schild]

The code can also prove useful for other use-cases where smbios
information needs to be accessed. Move it to a central place to allow
for such future users.

Signed-off-by: Henning Schild <henning...@siemens.com>
---
drivers/watchdog/ipc4x7e_wdt.c | 25 -------------------------
include/utils.h | 4 ++++
utils.c | 25 +++++++++++++++++++++++++
3 files changed, 29 insertions(+), 25 deletions(-)

diff --git a/drivers/watchdog/ipc4x7e_wdt.c b/drivers/watchdog/ipc4x7e_wdt.c
index 6a5c0f0f4632..f7e5e6a2ef9b 100644
--- a/drivers/watchdog/ipc4x7e_wdt.c
+++ b/drivers/watchdog/ipc4x7e_wdt.c
@@ -80,31 +80,6 @@ static UINT32 get_station_id(SMBIOS_STRUCTURE_POINTER oem_strct)
return 0;
}

-static SMBIOS_STRUCTURE_POINTER
-smbios_find_struct(SMBIOS_STRUCTURE_TABLE *table, UINT16 type)
-{
- SMBIOS_STRUCTURE_POINTER strct;
- UINT8 *str;
- UINTN n;
-
- strct.Raw = (UINT8 *)(uintptr_t)table->TableAddress;
-
- for (n = 0; n < table->NumberOfSmbiosStructures; n++) {
- if (strct.Hdr->Type == type) {
- return strct;
- }
- /* Read over any appended strings. */
- str = strct.Raw + strct.Hdr->Length;
- while (str[0] != 0 || str[1] != 0) {
- str++;
- }
- strct.Raw = str + 2;
- }
-
- strct.Raw = NULL;
- return strct;
-}
-
static UINTN mmcfg_address(UINTN bus, UINTN device, UINTN function,
UINTN offset)
{
diff --git a/include/utils.h b/include/utils.h
index 084796e23222..2610e8f0cb31 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -56,3 +56,7 @@ VOID PrintC(const UINT8 color, const CHAR16 *fmt, ...);

#define INFO(fmt, ...) \
PrintC(EFI_LIGHTGRAY, fmt, ##__VA_ARGS__)
+
+
+SMBIOS_STRUCTURE_POINTER smbios_find_struct(SMBIOS_STRUCTURE_TABLE *table,
+ UINT16 type);
diff --git a/utils.c b/utils.c
index 0ae5a5e0b8dc..e065b6f5d800 100644
--- a/utils.c
+++ b/utils.c
@@ -295,3 +295,28 @@ CHAR16 *GetBootMediumPath(CHAR16 *input)

return dst;
}
+
+SMBIOS_STRUCTURE_POINTER smbios_find_struct(SMBIOS_STRUCTURE_TABLE *table,
+ UINT16 type)
+{
+ SMBIOS_STRUCTURE_POINTER strct;
+ UINT8 *str;
+ UINTN n;
+
+ strct.Raw = (UINT8 *)(uintptr_t)table->TableAddress;
+
+ for (n = 0; n < table->NumberOfSmbiosStructures; n++) {
+ if (strct.Hdr->Type == type) {
+ return strct;
+ }
+ /* Read over any appended strings. */
+ str = strct.Raw + strct.Hdr->Length;
+ while (str[0] != 0 || str[1] != 0) {
+ str++;
+ }
+ strct.Raw = str + 2;
+ }
+
+ strct.Raw = NULL;
+ return strct;
+}
--
2.39.3

[Henning Schild]

[Henning Schild]

Tested on a Supermicro X11 series (Simatic IPC 1047E) and on some other
random Supermicro X9 series machine. Should work for any board that has
IPMI in any version. When an IPMI watchdog is present, iTCO is likely
not working. The Linux driver would detect that and has special code to
deal with especially Supermicro, here we use probe order.

Developed using the IPMI Spec 2.0 sections 9 and 27.

Signed-off-by: Henning Schild <henning...@siemens.com>
---

Makefile.am | 4 +-
drivers/watchdog/.ipmi_wdt.c.swn | Bin 0 -> 12288 bytes

drivers/watchdog/ipmi_wdt.c | 193 +++++++++++++++++++++++++++++++
3 files changed, 196 insertions(+), 1 deletion(-)

create mode 100644 drivers/watchdog/.ipmi_wdt.c.swn
create mode 100644 drivers/watchdog/ipmi_wdt.c

diff --git a/Makefile.am b/Makefile.am
index 48c560f72f0c..88d9829a74fd 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -159,14 +159,16 @@ if BOOTLOADER
if ARCH_IS_X86
# NOTE: wdat.c is placed first so it is tried before any other drivers
# NOTE: ipc4x7e_wdt.c must be *before* itco.c
+# NOTE: ipmi_wdt.c must be *before* itco.c
efi_sources_watchdogs = \
drivers/watchdog/wdat.c \
drivers/watchdog/amdfch_wdt.c \
drivers/watchdog/i6300esb.c \
drivers/watchdog/atom-quark.c \
drivers/watchdog/ipc4x7e_wdt.c \
+ drivers/watchdog/ipmi_wdt.c \
drivers/watchdog/itco.c \
- drivers/watchdog/hpwdt.c
+ drivers/watchdog/hpwdt.c \
else
efi_sources_watchdogs =
endif
diff --git a/drivers/watchdog/.ipmi_wdt.c.swn b/drivers/watchdog/.ipmi_wdt.c.swn
new file mode 100644
index 0000000000000000000000000000000000000000..d5e0f93dd94236c02d30c96b348641f981fd9e59
GIT binary patch
literal 12288
zcmeI2&yOQV6~}9XfHg?~!388F-cFQJ+w1X<+0|;u>_m8M&ul9m+qm0VLRhuh?((>_
zcK4{eJU_D9<&c$fOGG&GQwWJjkq~eQ7cNmCH&FfqK;jT7CxTFjL=?VN?(P{+X0v-F
zqAh*K?&??Xz53Rxs$OjmDo>j`^o>%5;qyht1pkiruV3B%+G}?h8*|?m{;s<7v7f0|
zea>AQ4L{ZhH}Av_%bqLUG9QUS5Xjw`8+v6g6#G1k$_K6-j=f;FEcT|tIq+oZzFzMa
zHM^fFkSV}sa<sDcD7&;;)l2=cr3>`sFZDBFG6gaPG6gaPG6gaPG6gaPG6gaP{*M%p
zd*|5qFy4paG4I9S=T3dcm)TdQK&C*ZK&C*ZK&C*ZK&C*ZK&C*ZK&C*ZK&HSaPyyFt
z?C?{Jbw7>d@&EtHzyCk@0%Pxhx51m>4e&eg0=Ns-!NcG@`1|J>`z!bh_%nD7{2aUl
zUIbIH1)6}s*TK7wGWJLC2k;Z{V{ibz1s(_QKf>6*!F%8x@H221JP!^)18jm4cnC1?
z;lqsm6Z{tZ8oUBt2Hyug@Kx{?a2~w<ImTWGKLi1|3C3U-6u^f$#y$Z50e=EN0s>qC
zOMt-p=NWqo{2sgtegN)*=fO3Q2L^Zqy!%<k{sCSAzXUIUDVTtDunf+D_ddhe-@u#T
zRqzY&GI$BR2(AMIoCj}V&VB=a3Ix~%J)r0E6IhE`%9#TH%L?3Pxsc0Q=u>mcbgX{8
z-n6WBmSeey`<|m;S$Ytg;nXuk;0)Y|7s>4In9lWv?bLS~POoV-?IN}NtyYmLdH6mU
ziwP%VFmW$l%+oEn&yoH_;yE6SjEGD1nv|nnsmiMk$En;qj1F?LXOZO!QcQU;lg_?7
znQ_{n_?ik-t-$Z;+eY7P+tn*n$S>qT7wNJ}!)$MLjMh?vOFop`)5E0X?Jja9ndJ^W
ze)wH~sYtOF=*DttfVsGVmN)2570z;e5^;6og#7)s)$evYJ-gXZ7P)enc6Q7T`GF)c
z-J9?!_YoMk#E6UwiQu$M)qEatNcp5&>i&ExFl0R!Pc`aJqq$Y<w`|AkY}TwM{3{=+
z-3F#c^-LS6U}!n6T-I_-2O@}^NQSeaEcM(2XtJi}2C?or02LtmNXLy0T?=`1UAT!{
z<Li+?<;_DS{iQpYps=i+O|xS;_A}k4lML)Zl8~;PMRZ}~WRa@`^v8I*V>YNz$S=q7
zbucyI(Y)UVwZveXOY10-d|D6uk=UJut`vc9tGtZmd0zTrdnUXhT&q%ob|~@<HQh;N
zEtFi`4zpNxH0Wh$*X+JU^YCbkhi%!te%<c(nohR^@6BE!*j%f#4DH%xt4W2E16yae
zm|krvrFOC;^7A&wait8!U?#ca7{<(>Mcm8hDNcWHC>)RPiy?Q^D7Bjh=(3NkBnWjE
z;R|WptBV?Yw{D_Dsh~oMpvaU6VmP0;xh$u84+DQb{vgC6U|4K7hmnI-KQJb4BnL+l
zg%j(W{y_Ow)eNVm^J=PFdds@rGwr6+Y&RC{7cO+wsfDc<cj`uMS<i|ascHHn378uN
zA(kDAy|PY%u9E68;XbWU6%BergxAy|HoVkF`XhCg3=r;g<P*+rV03B$#!YH*pEcZe
z)84Y2R==(G>oh?6BZ-vebB0cqUF#ipRXt93XSw0n4e3O-7?#S<$G)cOadCr>i!Gfq
z-B}Bve-+e_5CWZ(!6j9qR<bUE_=pkyz=&kD7lbm8T{2DHvMtBl+?pRgx>WQDQ6z1t
zGD-)ml3AnpcCrlMub8Zf#Q$Or>PV{`Rw_91)ZgQux7RV+<MZv=8<@LWIx%%%;uLfG
zR@3QNtxgT)w{W9Yx6Mx5xmN2Xr)m;Y>4K+>$2`1%>~p$LH>6a(hwX7}y|(eEa(F4V
zO|wzwI#Nw+a+Sl?%V%5Z(|>NKtX7YcOebTZ1c_i)T|2`}#Y`7w5?SSUWpy$1QwytD
zS7%}l<LVi%<8nx>RHCP?K5~HOU^R4)L!Wgt<#LzQNQ6I}%sfs@GbtufX}omYbTvAP
z$|5L@pFGiqt*<=huE)d0c1*5nU{ojrvbv3Lue7ii`w?F;Jv<tSk>KGIx$SOiWfgC~
zQ$d-@F{gT``;6J%rjdH^625Me!QRS*@AC<HBE$nt5FVA3y*(BY9R%S`!uMqE-lLh1
zbmskdIE^S6>5J`lpAfD{>P>f?hqPKEvN(@p)3y2Q_GAt5%;(Y7_-*-8q_4jj3H3-5
zm4?CeNfo;`lj9(ao`}2GgT13r?2aWF!#r7YW2$WzX|=L?DK^5PL7RB#p0?Fn_pDrC
F{{`k&sS^MI

literal 0
HcmV?d00001

diff --git a/drivers/watchdog/ipmi_wdt.c b/drivers/watchdog/ipmi_wdt.c
new file mode 100644
index 000000000000..fead8d9afd50
--- /dev/null
+++ b/drivers/watchdog/ipmi_wdt.c
@@ -0,0 +1,193 @@
+/*
+ * EFI Boot Guard
+ *
+ * Copyright (c) Siemens AG, 2023
+ *
+ * Authors:
+ * Henning Schild <henning...@siemens.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See
+ * the COPYING file in the top-level directory.
+ *
+ * SPDX-License-Identifier: GPL-2.0
+ */
+
+#include <efi.h>
+#include <pci/header.h>
+#include <sys/io.h>
+#include "utils.h"
+
+#define SMBIOS_TYPE_IPMI_KCS 38
+#define IPMI_KCS_DEFAULT_IOBASE 0xca2
+
+#define IPMI_KCS_STS_OBF 0x1
+#define IPMI_KCS_STS_IBF 0x2
+
+#define IPMI_KCS_CMD_ABORT 0x60
+#define IPMI_KCS_CMD_WRITE_START 0x61
+#define IPMI_KCS_CMD_WRITE_END 0x62
+
+#define IPMI_KCS_NETFS_LUN_WDT 0x18
+
+#define IPMI_WDT_CMD_RESET 0x22
+#define IPMI_WDT_CMD_SET 0x24
+#define IPMI_WDT_SET_USE_OSLOAD 0x3
+#define IPMI_WDT_SET_ACTION_HARD_RESET 0x1
+
+
+#define kcs_sts_is_error(sts) (((sts >> 6 ) & 0x3) == 0x3)
+
+static char
+set_wdt_data[] = {IPMI_WDT_SET_USE_OSLOAD, IPMI_WDT_SET_ACTION_HARD_RESET,
+ 0x00, 0x00,0x00, 0x00};
+
+static EFI_STATUS
+kcs_wait_iobf(UINT16 io_base, char iobf)
+{
+ char sts;
+
+ while (1) {
+ sts = inb(io_base + 1);
+ if (kcs_sts_is_error(sts))
+ return EFI_DEVICE_ERROR;
+ if (iobf == IPMI_KCS_STS_IBF) {
+ // IBF we wait for clear
+ if (!(sts & IPMI_KCS_STS_IBF))
+ break;
+ } else {
+ // OBF we wait for set
+ if (sts & IPMI_KCS_STS_OBF)
+ break;
+ }
+ }
+
+ return EFI_SUCCESS;
+}
+
+static EFI_STATUS
+kcs_outb(UINT8 value, UINT16 io_base, UINT16 port)
+{
+ EFI_STATUS status;
+
+ status = kcs_wait_iobf(io_base, IPMI_KCS_STS_IBF);
+ if (status)
+ return status;
+
+ outb(value, io_base + port);
+ // dummy read
+ inb(io_base);
+
+ return EFI_SUCCESS;
+}
+
+static EFI_STATUS
+_send_ipmi_cmd(UINT16 io_base, char cmd, char *data, int datalen)
+{
+ int i, err = 0;
+ char lastbyte = cmd;
+
+ err += kcs_outb(IPMI_KCS_CMD_WRITE_START, io_base, 1);
+ err += kcs_outb(IPMI_KCS_NETFS_LUN_WDT, io_base, 0);
+
+ if (datalen) {
+ lastbyte = data[datalen - 1];
+ err += kcs_outb(cmd, io_base, 0);
+ for (i = 0; i < datalen - 1; i++)
+ err += kcs_outb(data[i], io_base, 0);
+ }
+
+ err += kcs_outb(IPMI_KCS_CMD_WRITE_END, io_base, 1);
+ err += kcs_outb(lastbyte, io_base, 0);
+
+ if (err)
+ return EFI_DEVICE_ERROR;
+
+ return kcs_wait_iobf(io_base, IPMI_KCS_STS_OBF);
+}
+
+static VOID
+handle_ipmi_error(UINT16 io_base)
+{
+ WARNING(L"Handling Error Status 0x%x\n", inb(io_base + 1));
+
+ outb(IPMI_KCS_CMD_ABORT, io_base + 1);
+
+ if (kcs_wait_iobf(io_base, IPMI_KCS_STS_IBF))
+ // stuck in error state, hopefully does not happen
+ return;
+
+ if (inb((io_base + 1) & IPMI_KCS_STS_OBF))
+ inb(io_base);
+ outb(0x0, io_base);
+
+ if (kcs_wait_iobf(io_base, IPMI_KCS_STS_IBF))
+ // stuck in error state, hopefully does not happen
+ return;
+}
+
+static EFI_STATUS
+send_ipmi_cmd(UINT16 io_base, char cmd, char *data, int datalen)
+{
+ EFI_STATUS status;
+ int i;
+
+ for (i = 0; i < 5; i++) {
+ status = _send_ipmi_cmd(io_base, cmd, data, datalen);
+ if (status == EFI_SUCCESS)
+ return status;
+ handle_ipmi_error(io_base);
+ }
+
+ return status;
+}
+
+static EFI_STATUS __attribute__((constructor))
+init(__attribute__((unused)) EFI_PCI_IO *pci_io,
+ __attribute__((unused)) UINT16 pci_vendor_id,
+ __attribute__((unused)) UINT16 pci_device_id,
+ UINTN timeout)
+{
+ SMBIOS_STRUCTURE_TABLE *smbios_table;
+ SMBIOS_STRUCTURE_POINTER smbios_struct;
+ EFI_STATUS status;
+ UINT64 io_base;
+ UINT16 *timeout_value;
+
+ status = LibGetSystemConfigurationTable(&SMBIOSTableGuid,
+ (VOID **)&smbios_table);
+
+ if (status != EFI_SUCCESS)
+ return EFI_UNSUPPORTED;
+
+ smbios_struct = smbios_find_struct(smbios_table, SMBIOS_TYPE_IPMI_KCS);
+
+ if (smbios_struct.Raw == NULL)
+ return EFI_UNSUPPORTED;
+
+ io_base = *((UINT64 *)(smbios_struct.Raw + 8));
+ if (io_base == 0) {
+ io_base = IPMI_KCS_DEFAULT_IOBASE;
+ } else {
+ if (!(io_base & 1))
+ // MMIO not implemented
+ return EFI_UNSUPPORTED;
+
+ io_base &= ~1;
+ }
+
+ INFO(L"Detected IPMI watchdog at I/O 0x%x\n", io_base);
+ timeout_value = (UINT16 *)(set_wdt_data + 4);
+ *timeout_value = timeout * 10;
+
+ status = send_ipmi_cmd(io_base, IPMI_WDT_CMD_SET, set_wdt_data,
+ sizeof(set_wdt_data));
+ if (status == EFI_SUCCESS)
+ status = send_ipmi_cmd(io_base, IPMI_WDT_CMD_RESET, NULL, 0);
+
+ if (status != EFI_SUCCESS) {
+ WARNING(L"Watchdog device repeatedly reported errors. " \
+ "Failed to arm the watchdog, but still booting up.\n");
+ }
+
+ return EFI_SUCCESS;
+}
--
2.39.3

[Henning Schild]

Tested on a Supermicro X11 series (Simatic IPC 1047E) and on some other
random Supermicro X9 series machine. Should work for any board that has
IPMI in any version.

Developed using the IPMI Spec 2.0 sections 9 and 27. Error handling and
recovery tested to some degree. Best way to work turned out to be a
user-space application with iopl(3) and setting "no action".

Signed-off-by: Henning Schild <henning...@siemens.com>
---

.gitignore | 1 +
Makefile.am | 3 +-

drivers/watchdog/.ipmi_wdt.c.swn | Bin 0 -> 12288 bytes

drivers/watchdog/ipmi_wdt.c | 193 +++++++++++++++++++++++++++++++
4 files changed, 196 insertions(+), 1 deletion(-)

create mode 100644 drivers/watchdog/.ipmi_wdt.c.swn
create mode 100644 drivers/watchdog/ipmi_wdt.c

diff --git a/.gitignore b/.gitignore
index ae0d682be947..0dfbde702618 100644
--- a/.gitignore
+++ b/.gitignore
@@ -85,6 +85,7 @@ m4/lt*
version.h
libtool
.libs
+*.pc

# Test binaries
test_bgenv_init_retval
diff --git a/Makefile.am b/Makefile.am
index 48c560f72f0c..07f059a5b373 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -166,7 +166,8 @@ efi_sources_watchdogs = \
drivers/watchdog/atom-quark.c \
drivers/watchdog/ipc4x7e_wdt.c \

drivers/watchdog/itco.c \
- drivers/watchdog/hpwdt.c
+ drivers/watchdog/hpwdt.c \

+ drivers/watchdog/ipmi_wdt.c

[Henning Schild]

please ignore this and the other x/2

Am Mon, 15 May 2023 20:08:48 +0200
schrieb Henning Schild <henning...@siemens.com>:

[Henning Schild]

please ignore this and the other x/2

Am Mon, 15 May 2023 20:08:50 +0200
schrieb Henning Schild <henning...@siemens.com>:

[Henning Schild]

Am Mon, 15 May 2023 20:08:51 +0200
schrieb Henning Schild <henning...@siemens.com>:

this should not have been part of the patch, stay tuned for a v2 of the
whole series

Henning

[Henning Schild]

currently covering only "libebgenv.pc"

Signed-off-by: Henning Schild <henning...@siemens.com>
---

.gitignore | 1 +
1 file changed, 1 insertion(+)

diff --git a/.gitignore b/.gitignore
index ae0d682be947..0dfbde702618 100644
--- a/.gitignore
+++ b/.gitignore
@@ -85,6 +85,7 @@ m4/lt*
version.h
libtool
.libs
+*.pc

# Test binaries
test_bgenv_init_retval

--
2.39.3

[Henning Schild]

The code can also prove useful for other use-cases where smbios
information needs to be accessed. Move it to a central place to allow
for such future users.

Signed-off-by: Henning Schild <henning...@siemens.com>
---

--
2.39.3

[Henning Schild]

changes since v1:
- whitespace fix
- remove temporary editor file from p3

These patches add support for IPMI watchdogs typically found in
server-class hardware. Machines having IPMI will in any version have a
watchdog as well.

The Linux iTCO driver has special vendor logic for Supermicro, where the
iTCO does not work in favour of IPMI. So we place that one before itco
like done with the IPC 427E.

This driver has error handling which was kind of hard to test, but the
code can be easily placed into a user-space application with iopl(3) and
"no action". Where ipmitool and other helpers can be used.
The error handling also means we could end up in the unlikely and
unfortunate situation where we would have to boot without the watchdog
being actually armed. For testing i modified the code to cause some
errors, and the retries and recovery worked. So i hope it would be
highly unlikely that we fail to arm.

p1 is unrelated, just a drive-by finding

Henning Schild (3):
gitignore: add pkgconf wildcard
Move smbios helper function into utils.
watchdog: ipmi: Add driver for machines with IPMI

.gitignore | 1 +

Makefile.am | 2 +
drivers/watchdog/ipc4x7e_wdt.c | 25 -----
drivers/watchdog/ipmi_wdt.c | 193 +++++++++++++++++++++++++++++++++
include/utils.h | 4 +
utils.c | 25 +++++
6 files changed, 225 insertions(+), 25 deletions(-)

[Henning Schild]

Tested on a Supermicro X11 series (Simatic IPC 1047E) and on some other
random Supermicro X9 series machine. Should work for any board that has
IPMI in any version. When an IPMI watchdog is present, iTCO is likely
not working. The Linux driver would detect that and has special code to
deal with especially Supermicro, here we use probe order.

Developed using the IPMI Spec 2.0 sections 9 and 27.

Signed-off-by: Henning Schild <henning...@siemens.com>
---

Makefile.am | 2 +
drivers/watchdog/ipmi_wdt.c | 193 ++++++++++++++++++++++++++++++++++++
2 files changed, 195 insertions(+)

create mode 100644 drivers/watchdog/ipmi_wdt.c

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

index 48c560f72f0c..ba2440025a0c 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -159,12 +159,14 @@ if BOOTLOADER

if ARCH_IS_X86
# NOTE: wdat.c is placed first so it is tried before any other drivers
# NOTE: ipc4x7e_wdt.c must be *before* itco.c
+# NOTE: ipmi_wdt.c must be *before* itco.c
efi_sources_watchdogs = \
drivers/watchdog/wdat.c \
drivers/watchdog/amdfch_wdt.c \
drivers/watchdog/i6300esb.c \
drivers/watchdog/atom-quark.c \
drivers/watchdog/ipc4x7e_wdt.c \
+ drivers/watchdog/ipmi_wdt.c \
drivers/watchdog/itco.c \

drivers/watchdog/hpwdt.c
else

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

index 000000000000..af55e57c5a0e

--
2.39.3

[Jan Kiszka]

Extra newline.

Is that loop guaranteed to eventually terminate? Or do we have to set a
waiting limit? What does the kernel do?

> +
> + return EFI_SUCCESS;
> +}
> +
> +static EFI_STATUS
> +kcs_outb(UINT8 value, UINT16 io_base, UINT16 port)
> +{
> + EFI_STATUS status;
> +
> + status = kcs_wait_iobf(io_base, IPMI_KCS_STS_IBF);
> + if (status)
> + return status;
> +
> + outb(value, io_base + port);
> + // dummy read

Why? As delay or because the BMC expects a read in order to continue?

Reads a bit like "we are lost when we get here", which is not completely
true as we have an error message on the console and an error return code
from the caller, send_ipmi_cmd. What we then do with the error code is a
different story, see below.

That's deviating for other watchdog drivers. What makes this watchdog
different to justify it? If nothing, we can still argue about the best
behavior at EBG level. But, IIRC, a failing EFI application would cause
a reboot attempt of the firmware.

Jan

> + }
> +
> + return EFI_SUCCESS;
> +}

--

Siemens AG, Technology
Competence Center Embedded Linux

[Jan Kiszka]

On 15.05.23 20:20, 'Henning Schild' via EFI Boot Guard wrote:

Thanks, applied.

Jan

[Jan Kiszka]

On 15.05.23 20:20, 'Henning Schild' via EFI Boot Guard wrote:

Extra newline.

[Henning Schild]

Am Wed, 17 May 2023 12:52:31 +0200
schrieb Jan Kiszka <jan.k...@siemens.com>:

Not sure what the kernel really does, did not dig that deep since the
kernel code is hard to read and was not even looked at.

The loop is expected to eventually end. We wait for input to become
available, or output to be received by the BMC. Or we should see an
error.

But thinking about it again, i think it might indeed be better to put a
timer around all of that. I will have a look at the kernel and freeipmi
(userspace implementation) and kind of expect endless retry loops
guarded with timers that use multiples of 5s (number coming from the
spec).

> > +
> > + return EFI_SUCCESS;
> > +}
> > +
> > +static EFI_STATUS
> > +kcs_outb(UINT8 value, UINT16 io_base, UINT16 port)
> > +{
> > + EFI_STATUS status;
> > +
> > + status = kcs_wait_iobf(io_base, IPMI_KCS_STS_IBF);
> > + if (status)
> > + return status;
> > +
> > + outb(value, io_base + port);
> > + // dummy read
>
> Why? As delay or because the BMC expects a read in order to continue?

BMC expects that, that is written in the SPEC and also called "dummy
read" there.

I will remove the comments. We come here when we are in error-state and
hope to transition out of it by sending an abort command. If that all
goes well we can try sending our real command again ... but if we are
stuck in error we eventually have to give up.

This watchdog driver needs to have error-handling built-in the actual
arming of the watchdog. Because we talk to a potentially multi-user
device and there can be races.

But you are totally right, i never thought about what would happen if i
just returned the error to the firmware. I played with all the error
handling in an UEFI shell ... where an error code looks like a "stuck
in boot".

I think i will indeed remove much of the error handling and
retry/abort logic and simply pass the error to the firmware. And for
the "breaking up endless waiting" simply use a 5s timer. We manage to
arm in less than 5 seconds, or we tell the firmware there was an error.

Henning