>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? > Arsalan: Let me check if I can get hands on the docs.
>> 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. > Arsalan: Well that's not all... anyway, there is no point in discussing that... >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. > Arsalan: I can only submit what I have worked on... something I can test on a physical machine. I don't know EFCH or SB800 or SP5100... and I know EFCH driver does *not* work on the platforms I mentioned earlier. >> ...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. > Arsalan: I'll try to talk to AMD to find out what they call this watchdog, but in case I can't, would it be OK if I renamed it to "amd_carrizo_smbus_wdt"? >> >>>> Signed-off-by: Arsalan H. Awan <[email protected]> >>>> --- >>>> 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 <[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 <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. > Arsalan: If you look at the code, you'll see a lot of calls to inb, outb, readl and writel functions. If I remove the functions and use the code inside instead, the code will get only bigger. Everything else is necessary apart from watchdog.fired bits. I'll remove that. It may not be as compact as it can get, but it is clean. >> >>>> +{ >>>> +#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. > Arsalan: How do I access the revision in efibootguard watchdog driver init (constructor) to compare against? >>>> + 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. > Arsalan: Sure! But I don't know EFCH and I know that that driver does not work on these latest platforms. And I don't want to write a driver for an EFCH because I cannot test it. >> 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. > Arsalan: Working on it... >> >>>> + 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. > Arsalan: Consider it done! >>>> + >>>> + 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. > Arsalan: I guess... >> >> 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. > Arsalan: Consider it gone! >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. > Arsalan: I'm not sure if I exactly got you here, but I want to share my experience with this method that the underlying hardware wdt (at least for the platforms I mentioned) that got configured was the same. So whether efibootguard configures the wdt, or UEFI configures the wdt and then dies, the hardware wdt gets configured... and it will pop! It does! I've tested it! >Jan > >-- >Siemens AG, Corporate Technology, CT RDA IOT SES-DE >Corporate Competence Center Embedded Linux - Arsalan Mentor, a Siemens business. Mentor 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/1598119519964.20700%40mentor.com.
