On 22.08.20 09:37, Awan, Arsalan wrote: >> On 21.08.20 17:46, Arsalan H. Awan wrote: >>> This adds support for AMD Watchdog in efibootguard. >>> >> >> The driver does not look like it's derived from the authoritative >> linux/drivers/watchdog/sp5100_tco.c, is it? Did you double-check that >> you are not missing anything from that driver? If you are in line with >> Linux, you may also add the other PCI IDs. But I just started to compare. >> >> That linux driver suggests you are implementing the SB800 (and newer >> compatible ones). Maybe avoid such a generic name and use that instead. >> > Arsalan: > No, it's not! This driver is for the latest AMD platforms e.g.: > > * AMD Ryzen Embedded V1000 (V-Series APU) > * AMD Ryzen Embedded R1000 (R-Series APU) > * AMD EPYC Embedded E3000 (E-Series CPU) > > See: https://www.amd.com/en/products/embedded-ryzen-series > See: https://www.amd.com/en/products/embedded-epyc-3000-series > > The driver that you mentioned does not drive the watchdog on these platforms, > and the driver I am submitting
It matches the same PCI ID as your driver (PCI_DEVICE_ID_AMD_KERNCZ_SMBUS vs. PCI_DEVICE_ID_AMD_CARRIZO_SMBUS). It shares a lot of the magic variables. It is the same base. Where do I find the manuals for the implemented logic? > does not exist in the upstream linux kernel. However linux implementation of > this driver was shared by AMD > with us (Mentor). What? It's not upstream? > > See: > https://git.yoctoproject.org/cgit/cgit.cgi/meta-amd/tree/meta-amd-bsp/recipes-kernel/amd-wdt/files > A yocto layer... Folks, properly work towards upstream. This kernel driver apparently sits there for a year. This is no proper way of maintaining such code. If AMD didn't do their homework, fix it. I didn't look into all the details of what is missing/diverging, but I predict from what I've seen they will be minimal. It should take you day to sort it out for good. > ...and I implemented this driver for grub-efi bootloader. > > See: > https://git.yoctoproject.org/cgit/cgit.cgi/meta-amd/tree/common/mentor-swupdate/recipes-bsp/grub/amd-wdt?h=sumo > > ...and I basically ported that over to efibootguard bootloader. > > And yes, you're right. I should change the name to something more specific > targetting these platforms. > But I'd need your suggestion on this for what you'd like for the efibootguard > project. For EFI Boot Guard, I'm fine with a driver that has a similar, modern scope like your downstream kernel module plus what the kernel drives under the very same PCI ID. Give it a name that reflects that scope. > >>> Signed-off-by: Arsalan H. Awan <arsalan_a...@mentor.com> >>> --- >>> Makefile.am | 1 + >>> drivers/watchdog/amd_wdt.c | 296 +++++++++++++++++++++++++++++++++++++ >>> drivers/watchdog/amd_wdt.h | 76 ++++++++++ >>> 3 files changed, 373 insertions(+) >>> create mode 100644 drivers/watchdog/amd_wdt.c >>> create mode 100644 drivers/watchdog/amd_wdt.h >>> >>> diff --git a/Makefile.am b/Makefile.am >>> index 74fa7f0..da4c7c1 100644 >>> --- a/Makefile.am >>> +++ b/Makefile.am >>> @@ -123,6 +123,7 @@ efi_loadername = efibootguard$(MACHINE_TYPE_NAME).efi >>> >>> efi_sources = \ >>> drivers/watchdog/init_array_start.S \ >>> + drivers/watchdog/amd_wdt.c \ >>> drivers/watchdog/i6300esb.c \ >>> drivers/watchdog/atom-quark.c \ >>> drivers/watchdog/itco.c \ >>> diff --git a/drivers/watchdog/amd_wdt.c b/drivers/watchdog/amd_wdt.c >>> new file mode 100644 >>> index 0000000..2e194ea >>> --- /dev/null >>> +++ b/drivers/watchdog/amd_wdt.c >>> @@ -0,0 +1,296 @@ >>> +/* >>> + * EFI Boot Guard >>> + * >>> + * Copyright (C) 2020 Mentor Graphics, A Siemens business >>> + * >>> + * Authors: >>> + * Arsalan H. Awan <arsalan_a...@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 <pci/header.h> >>> + >>> +#include "amd_wdt.h" >> >> Please move the used defines here. >> > Arsalan: > I'm on it! > >>> + >>> +#define PCI_VENDOR_ID_AMD 0x1022 >>> + >>> +/* #define AMD_WDT_DEBUG */ >>> + >>> +static struct >>> +{ >>> + UINT8 fired; >>> + UINTN base; >>> + EFI_PCI_IO *pci_io; >>> +} watchdog; >>> + >>> +static EFI_STATUS >>> +writel (UINT32 val, UINT32 addr) >> >> static EFI_STATUS writel(...) >> >> Please align your coding style to the rest of the project, specifically >> the watchdog drivers. >> >>> +{ >>> +#ifdef AMD_WDT_DEBUG >>> + Print (L"\n- writel (%X, %X) ", val, addr); >>> +#endif >> >> Still needed? If so, wrap properly and have a single ifdef there and a >> DebugPrint or so in the code. But I suppose your code is beyond that >> point already. >> > Arsalan: > I found this extremely helpful in debugging. > >>> + EFI_STATUS status; >>> + >>> + status = uefi_call_wrapper(watchdog.pci_io->Mem.Write, 6, >>> watchdog.pci_io, >>> + EfiPciIoWidthUint32, >>> + EFI_PCI_IO_PASS_THROUGH_BAR, >>> + addr, 1, &val); >>> + >>> + if (EFI_ERROR(status)) { >>> + Print(L"Error while writel (%X, %X): %r", val, addr, status); >>> + } >>> + >>> + return status; >>> +} >>> + >>> +static UINT32 >>> +readl (UINT32 addr) >>> +{ >>> +#ifdef AMD_WDT_DEBUG >>> + Print (L"\n- readl (%X) ", addr); >>> +#endif >>> + UINT32 val = 0; >>> + EFI_STATUS status; >>> + >>> + status = uefi_call_wrapper(watchdog.pci_io->Mem.Read, 6, >>> watchdog.pci_io, >>> + EfiPciIoWidthUint32, >>> + EFI_PCI_IO_PASS_THROUGH_BAR, >>> + addr, 1, &val); >>> + >>> + if (EFI_ERROR(status)) { >>> + Print(L"Error while readl (%X): %r", addr, status); >>> + } >>> + >>> +#ifdef AMD_WDT_DEBUG >>> + Print (L"= %X ", val); >>> +#endif >>> + return val; >>> +} >>> + >>> +static EFI_STATUS >>> +outb (UINT8 val, UINT16 reg) >>> +{ >>> +#ifdef AMD_WDT_DEBUG >>> + Print (L"\n- outb (%X, %X) ", val, reg); >>> +#endif >>> + EFI_STATUS status; >>> + >>> + status = uefi_call_wrapper(watchdog.pci_io->Io.Write, 6, >>> watchdog.pci_io, >>> + EfiPciIoWidthUint8, >>> + EFI_PCI_IO_PASS_THROUGH_BAR, >>> + reg, 1, &val); >>> + if (EFI_ERROR(status)) { >>> + Print(L"Error while outb (%X, %X): %r", val, reg, status); >>> + } >>> + >>> + return status; >>> +} >>> + >>> +static UINT8 >>> +inb (UINT16 reg) >>> +{ >>> +#ifdef AMD_WDT_DEBUG >>> + Print (L"\n- inb (%X) ", reg); >>> +#endif >>> + EFI_STATUS status; >>> + UINT8 val = 0; >>> + >>> + status = uefi_call_wrapper(watchdog.pci_io->Io.Read, 6, >>> watchdog.pci_io, >>> + EfiPciIoWidthUint8, >>> + EFI_PCI_IO_PASS_THROUGH_BAR, >>> + reg, 1, &val); >>> + if (EFI_ERROR(status)) { >>> + Print(L"Error while inb (%X): %r", reg, status); >>> + } >>> + >>> +#ifdef AMD_WDT_DEBUG >>> + Print(L"= %X ", val); >>> +#endif >>> + return val; >>> +} >>> + >>> +static UINT8 >>> +amd_wdt_check_fired (VOID) >>> +{ >>> +#ifdef AMD_WDT_DEBUG >>> + Print (L"\n-- amd_wdt_check_fired() "); >>> +#endif >>> + UINT32 val; >>> + /* Read watchdog fired bit */ >>> + val = readl (AMD_WDT_CONTROL(watchdog.base)); >>> + return val & AMD_WDT_FIRED_BIT; >>> +} >>> + >>> +static EFI_STATUS >>> +amd_wdt_enable (VOID) >>> +{ >>> +#ifdef AMD_WDT_DEBUG >>> + Print (L"\n-- amd_wdt_enable() "); >>> +#endif >>> + UINT8 val; >>> + /* Enable watchdog timer */ >>> + outb(AMD_PM_WATCHDOG_EN_REG, AMD_IO_PM_INDEX_REG); >>> + val = inb(AMD_IO_PM_DATA_REG); >>> + val |= AMD_PM_WATCHDOG_TIMER_EN; >>> + return outb(val, AMD_IO_PM_DATA_REG); >>> +} >>> + >>> +static EFI_STATUS >>> +amd_wdt_set_resolution (UINT8 freq) >>> +{ >>> +#ifdef AMD_WDT_DEBUG >>> + Print (L"\n-- amd_wdt_set_resolution(%d) ", freq); >>> +#endif >>> + UINT8 val; >>> + /* Set the watchdog timer resolution */ >>> + outb(AMD_PM_WATCHDOG_CONFIG_REG, AMD_IO_PM_INDEX_REG); >>> + val = inb(AMD_IO_PM_DATA_REG); >>> + /* Clear the previous frequency setting, if any */ >>> + val &= ~AMD_PM_WATCHDOG_CONFIG_MASK; >>> + /* Set the new frequency value */ >>> + val |= freq; >>> + return outb(val, AMD_IO_PM_DATA_REG); >>> +} >>> + >>> +static EFI_STATUS >>> +amd_wdt_set_timeout_action (CONST CHAR16 * action) >> >> Unneed abstraction with a rather unusual (string-based) interface. We >> only use "reboot". >> >> If you want to factor out a common read-modify-write cycle, look at >> sp5100_tco_update_pm_reg8. >> > Arsalan: > I did what AMD did for their kernel implementation of this watchdog driver. Their driver is for the Linux watchdog subsystem with all its runtime features. We just implement the setup function here. So you can fold a lot of things to make the code more compact. > >>> +{ >>> +#ifdef AMD_WDT_DEBUG >>> + Print (L"\n-- amd_wdt_set_timeout_action(%s) ", action); >>> +#endif >>> + UINT32 val; >>> + val = readl (AMD_WDT_CONTROL(watchdog.base)); >>> + >>> + /* >>> + * Set the watchdog timeout action. >>> + * >>> + * If action is specified anything other than reboot or shutdown, >>> + * we default it to reboot. >>> + */ >>> + if (StrnCmp(action, L"shutdown", 8) == 0) >>> + val |= AMD_WDT_ACTION_RESET_BIT; >>> + else >>> + val &= ~AMD_WDT_ACTION_RESET_BIT; >>> + >>> + return writel (val, AMD_WDT_CONTROL(watchdog.base)); >>> +} >>> + >>> +static EFI_STATUS >>> +amd_wdt_set_time (UINT32 t) >>> +{ >>> +#ifdef AMD_WDT_DEBUG >>> + Print (L"\n-- amd_wdt_set_time(%d) ", t); >>> +#endif >>> + if (t < AMD_WDT_MIN_TIMEOUT) >>> + t = AMD_WDT_MIN_TIMEOUT; >>> + else if (t > AMD_WDT_MAX_TIMEOUT) >>> + t = AMD_WDT_MAX_TIMEOUT; >>> + >>> + /* Write new timeout value to watchdog COUNT register */ >>> + return writel (t, AMD_WDT_COUNT(watchdog.base)); >>> +} >>> + >>> +static EFI_STATUS >>> +amd_wdt_start (VOID) >>> +{ >>> +#ifdef AMD_WDT_DEBUG >>> + Print (L"\n-- amd_wdt_start() "); >>> +#endif >>> + UINT32 val; >>> + /* Start the watchdog timer */ >> >> That comment is not correct. >> > Arsalan: > AMD themselves think that it is. Ah, sorry, I was off the track. > >>> + val = readl (AMD_WDT_CONTROL(watchdog.base)); >>> + val |= AMD_WDT_START_STOP_BIT; >> >> I think the kernel does more, please cross-check. >> > Arsalan: > I did what AMD did in their kernel implementation of this driver. That's why I'm asking for the documentation. > >>> + return writel (val, AMD_WDT_CONTROL(watchdog.base)); >>> +} >>> + >>> +static EFI_STATUS >>> +amd_wdt_ping (VOID) >>> +{ >>> +#ifdef AMD_WDT_DEBUG >>> + Print (L"\n-- amd_wdt_ping() "); >>> +#endif >>> + UINT32 val; >>> + /* Trigger/Ping watchdog timer */ >>> + val = readl (AMD_WDT_CONTROL(watchdog.base)); >>> + val |= AMD_WDT_TRIGGER_BIT; >>> + return writel (val, AMD_WDT_CONTROL(watchdog.base)); >>> +} >>> + >>> +static EFI_STATUS __attribute__((constructor)) >>> +init (EFI_PCI_IO *pci_io, UINT16 pci_vendor_id, UINT16 pci_device_id, >>> + UINTN timeout) >>> +{ >>> + EFI_STATUS status; >>> + >>> + if (!pci_io || pci_vendor_id != PCI_VENDOR_ID_AMD || >>> + pci_device_id != PCI_DEVICE_ID_AMD_CARRIZO_SMBUS) { BTW, you will likely need to check the revision as well. I suspect it will be the key to differentiate from existing EFCH (if there are relevant differences in the end), but that already requires >= 0x49, see tco_reg_layout. >>> + return EFI_UNSUPPORTED; >>> + } >>> + >>> + watchdog.base = AMD_ACPI_MMIO_BASE + AMD_WDT_MEM_MAP_OFFSET; >> >> Ok, this hard-coding actually limits us to embedded FCH chipset. Fine >> with me, but then we have a different scope and should reflect that in >> the driver name (amd-efch?). >> > Arsalan: > Again, It's not for EFCH chipset. And it targets at least 3 different latest > series of AMD chips at the moment It *is* EFCH-derived, just compare the code. > that I have personally tested this driver on, and we know 2 more series of > AMD APUs/CPUs coming soon > that this driver will be potentially applicable to... but I'm not sure. > > So what would you suggest as a name for this driver? Give me the docs and I can likely tell you. > >>> + watchdog.pci_io = pci_io; >>> + >>> + Print(L"Detected AMD Carrizo SMBus Watchdog Timer\n"); >> >> And here you actually call Carrizo, though I'm not sure if that covers >> it better. It's just that "AMD" is too generic. >> > Arsalan: > Please suggest a name. I'm only copying AMD here: ---> #define > PCI_DEVICE_ID_AMD_CARRIZO_SMBUS 0x790B > >>> + >>> + watchdog.fired = amd_wdt_check_fired(); >>> + >>> +#ifdef AMD_WDT_DEBUG >>> + Print(L"\nwatchdog.base = %X\n", watchdog.base); >>> + Print(L"watchdog.fired = %X\n", watchdog.fired); >>> +#endif >> >> Still needed? Wlhat for? If not, drop all the dead fired-related code. >> > Arsalan: > In grub, we used the fired status to allow the user to take many decissions > based on watchdog.fired status in the grub.cfg. > So, it can be useful in the cfg files. Besides I just copied the kernel > implementation of this driver... so... > I can remove this if you guys don't have any plans of having such > functionality. > There is no use case in EFI Boot Guard for it. We robustly track the state internally, and it does not matter if we restarted due to a watchdog timeout or an erroneous reboot/power-cycle - we will always declare an active trial boot failed if that happens. In fact, if you based your state machine in grub solely on the fired bit, you would have a serious bug. So, please drop this dead code. >>> + >>> + status = amd_wdt_enable (); >>> + if (EFI_ERROR(status)) { >>> + Print(L"Error: amd_wdt_enable () failed."); >>> + return status; >>> + } >>> + >>> + status = amd_wdt_set_resolution (AMD_PM_WATCHDOG_1SEC_RES); >>> + if (EFI_ERROR(status)) { >>> + Print(L"Error: amd_wdt_set_resolution (%d) failed.", >>> AMD_PM_WATCHDOG_1SEC_RES); >>> + return status; >>> + } >>> + >>> + status = amd_wdt_set_timeout_action (L"reboot"); >>> + if (EFI_ERROR(status)) { >>> + Print(L"Error: amd_wdt_set_timeout_action ("reboot") >>> failed."); >>> + return status; >>> + } >>> + >>> + status = amd_wdt_set_time (timeout); >>> + if (EFI_ERROR(status)) { >>> + Print(L"Error: amd_wdt_set_time (%d) failed.", timeout); >>> + return status; >>> + } >>> + >>> + status = amd_wdt_start (); >>> + if (EFI_ERROR(status)) { >>> + Print(L"Error: amd_wdt_start () failed."); >>> + return status; >>> + } >>> + >>> + status = amd_wdt_ping (); >> >> Is that ping actually needed? The kernel doesn't issue it. >> > Arsalan: > Well, I'm pretty sure the watchdog does not start the countdown unless it is > pingged after start cmd. > Tested this driver with and without the ping on 3 different AMD platforms... > so... Then this might be a concrete deviation from older EFCH designs. > > Also, here's how AMD did it in their kernel implementation of this driver: > > static int amd_wdt_start(struct watchdog_device *wdt_dev) > { > u32 val; > unsigned long flags; > > /* Enable the watchdog timer */ > spin_lock_irqsave(&wdt_lock, flags); > > val = readl(AMD_WDT_CONTROL(wdtbase)); > val |= AMD_WDT_START_STOP_BIT; > writel(val, AMD_WDT_CONTROL(wdtbase)); > > spin_unlock_irqrestore(&wdt_lock, flags); > > /* Trigger the watchdog timer */ > amd_wdt_ping(wdt_dev); > > return 0; > } > >>> + if (EFI_ERROR(status)) { >>> + Print(L"Error: amd_wdt_ping () failed."); >>> + return status; >>> + } >>> + >>> +/* >>> + * Following is an alternate easy way to set the watchdog >>> + * using the UEFI API. >>> + * This works only if watchdog driver is implemented in the >>> + * UEFI firmware on the machine. >>> + * >>> + * status = uefi_call_wrapper(BS->SetWatchdogTimer, 4, >>> + * timeout, 0, 0, NULL); >>> + */ >> >> What is this? >> > Arsalan: > It's the holy grail of watchdogs in UEFI. We don't need this whole > implemntation of watchdog > drivers if the driver is already implemented in the UEFI firmware... we can > simply do this! > And the UEFI sets the watchdog up for is... I can remove it if you think it's > not useful. WDAT? That would be a different driver for EBG, nothing that belongs here because it is not specific to this hardware. Or is this the UEFI boot services watchdog that terminates early during Linux boot? That's unfortunately a completely useless thing when it comes to A/B update monitoring. Jan -- Siemens AG, Corporate Technology, CT RDA IOT SES-DE 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 efibootguard-dev+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/efibootguard-dev/1997de23-f840-101f-d6cc-99483bf5bdac%40siemens.com.
