On 07.09.20 11:44, Jan Kiszka wrote:
> On 07.09.20 11:08, Awan, Arsalan wrote:
>>
>>>>> OK, just missed my first review comment. But we can sort out read/in
>>>>> error handling while factoring out those helpers in-tree.
>>>>>
>>>>> Applied to next.
>>>>>
>>>> Arsalan:
>>>> Appreciate that! Thanks!
>>>>
>>>
>>> Some update after trying out the driver on my new R1505G. The good news:
>>>
>>> - efibootguard detects and configures the watchdog correctly
>>>
>>> - the upstream kernel driver picks up the watchdog - if not, the system
>>> resets after the timeout
>>>
>> Arsalan:
>> I noticed that the other day. The kernel was picking sp5100 driver + our
>> amd_wdt kernel module to get things working.
>> Without the amd_wdt kernel module OR the amdfch_wdt efibootguard driver, the
>> WDT won't work.
>>
>
> The difference must be in the watchdog enabling bits. I didn't look into
> the details, but they can't be many.
>
>>> The bad ones:
>>>
>>> - there is no lock-down of the watchdog, and a distro kernel will
>>> simply turn it off until some watchdog service may pick it up again
>>>
>> Arsalan:
>> Yes you're right.
>> However, the kernel driver sp5100 has this nowayout functionality
>> implemented though. Probably a software implementation. So user should be
>> able to enable nowayout using kernel module args. Right?
>
> The generic nowayout mode of the watchdog subsystem only works for the
> case of opening the device from userspace. Once that happened, the
> kernel prevents in software that the watchdog is disabled again.
>
> However, this has nothing to do with hardware-assisted locking and also
> the default behavior when loading the driver, i.e. before the first
> userspace access. Here, the given kernel driver disables the hardware
> unconditionally.
>
>>
>>> - the upstream driver only works after efibootguard enabled the
>>> watchdog - likely fixable, though, and likely the reason why the
>>> misunderstanding arose that the kernel does not support this
>>> hardware
>>>
>>> Did you check the specifications for a potential hw-assisted locking /
>>> no-way-out mode? Not having that significantly decreases the value of
>>> this watchdog unfortunately.
>>>
>> Arsalan:
>> I had a conversation with AMD requesting from them the WDT docs, WDT name
>> and upstream status. They said that their team is working on upstreaming
>> this amdfch_wdt driver as we speak. They will not be able to share the docs.
>> They suggested this name "amdfch_wdt" for this WDT. They said that you can
>> use the current code for the driver that AMD has shared with Mentor, and you
>> can merge the latest code once publicly available. The code is expected to
>> be available in Q4 2020.
>
> I doubt they will be successful with proposing a new driver, rather than
> adding the three missing lines to the existing one. From my experience,
> the wdt maintainer is very accurate.
>
All what is missing is a single(!) line:
diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
index 85e9664318c9..5482154fde42 100644
--- a/drivers/watchdog/sp5100_tco.c
+++ b/drivers/watchdog/sp5100_tco.c
@@ -193,7 +193,8 @@ static void tco_timer_enable(struct sp5100_tco *tco)
/* Set the Watchdog timer resolution to 1 sec and enable */
sp5100_tco_update_pm_reg8(EFCH_PM_DECODEEN3,
~EFCH_PM_WATCHDOG_DISABLE,
- EFCH_PM_DECODEEN_SECOND_RES);
+ EFCH_PM_DECODEEN_SECOND_RES |
+ EFCH_PM_DECODEEN_WDT_TMREN);
break;
}
}
[ 11.957371] sp5100_tco: SP5100/SB800 TCO WatchDog Timer Driver
[ 11.957473] sp5100-tco sp5100-tco: Using 0xfed80b00 for watchdog MMIO address
[ 11.957555] sp5100-tco sp5100-tco: initialized. heartbeat=60 sec (nowayout=0)
(while EBG was disabled)
I'm not sure if that is unconditionally correct, also for the older
chipsets supported by this driver, but maybe you can poke your AMD
contacts. Will also save them "a lot" of their time. Then I can propose
the result for upstream.
Jan
>>
>> Since I could not get hands on the design docs or manuals for this WDT,
>> unfortunately we would not be able to implement the HW-assisted nowayout at
>> the moment. But workarounds exist! And having this amdfch_wdt implementation
>> in the efibootguard has enough value for now, than not having it at all.
>>
>>> It could be partially mitigated by changing the kernel driver to detect
>>> a running watchdog and avoid turning it off unless explicitly requested.
>>> That what w83627hf_wdt.c does e.g. I have that one on my board as well,
>>> and it also lacks any lock-down support.
>>>
>> Arsalan:
>> I did fix this by removing that line in the amd_wdt kernel module that stops
>> the wdt. With that workaround, things worked fine!
>>
>
> To make that effective (unless the hardware has some hidden lock-down
> mode), it needs to go upstream.
>
> Jan
>
--
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/fd3b9d0a-e78d-fd49-d383-5c906349db2e%40web.de.
