[PATCH v2 4/5] watchdog: rti_wdt: Add support for loading firmware
Jan Kiszka
jan.kiszka at siemens.com
Tue Jul 20 18:14:31 CEST 2021
On 20.07.21 14:57, Jan Kiszka wrote:
> On 14.07.21 16:15, Simon Glass wrote:
>> Hi Jan,
>>
>> On Wed, 14 Jul 2021 at 03:53, Jan Kiszka <jan.kiszka at siemens.com> wrote:
>>>
>>> On 05.07.21 17:29, Simon Glass wrote:
>>>> Hi Jan,
>>>>
>>>> On Sun, 27 Jun 2021 at 23:40, Jan Kiszka <jan.kiszka at siemens.com> wrote:
>>>>>
>>>>> On 27.06.21 20:18, Simon Glass wrote:
>>>>>> Hi Jan,
>>>>>>
>>>>>> On Sun, 27 Jun 2021 at 12:01, Jan Kiszka <jan.kiszka at siemens.com> wrote:
>>>>>>>
>>>>>>> On 26.06.21 20:29, Simon Glass wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On Fri, 11 Jun 2021 at 08:08, Tom Rini <trini at konsulko.com> wrote:
>>>>>>>>>
>>>>>>>>> On Fri, Jun 11, 2021 at 07:14:21PM +0530, Lokesh Vutla wrote:
>>>>>>>>>> Hi Tom,
>>>>>>>>>>
>>>>>>>>>> On 09/06/21 6:47 pm, Jan Kiszka wrote:
>>>>>>>>>>> On 07.06.21 13:44, Jan Kiszka wrote:
>>>>>>>>>>>> On 07.06.21 13:40, Tom Rini wrote:
>>>>>>>>>>>>> On Mon, Jun 07, 2021 at 03:33:52PM +0530, Lokesh Vutla wrote:
>>>>>>>>>>>>>> +Tom,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hi Tom,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 02/06/21 3:07 pm, Jan Kiszka wrote:
>>>>>>>>>>>>>>> From: Jan Kiszka <jan.kiszka at siemens.com>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> To avoid the need of extra boot scripting on AM65x for loading a
>>>>>>>>>>>>>>> watchdog firmware, add the required rproc init and loading logic for the
>>>>>>>>>>>>>>> first R5F core to the watchdog start handler. In case the R5F cluster is
>>>>>>>>>>>>>>> in lock-step mode, also initialize the second core. The firmware itself
>>>>>>>>>>>>>>> is embedded into U-Boot binary to ease access to it and ensure it is
>>>>>>>>>>>>>>> properly hashed in case of secure boot.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> One possible firmware source is https://github.com/siemens/k3-rti-wdt.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka at siemens.com>
>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>> drivers/watchdog/Kconfig | 20 ++++++++++++
>>>>>>>>>>>>>>> drivers/watchdog/Makefile | 5 +++
>>>>>>>>>>>>>>> drivers/watchdog/rti_wdt.c | 58 ++++++++++++++++++++++++++++++++++-
>>>>>>>>>>>>>>> drivers/watchdog/rti_wdt_fw.S | 20 ++++++++++++
>>>>>>>>>>>>>>> 4 files changed, 102 insertions(+), 1 deletion(-)
>>>>>>>>>>>>>>> create mode 100644 drivers/watchdog/rti_wdt_fw.S
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>>>>>>>>>>>>>> index f0ff2612a6..1a1fddfe9f 100644
>>>>>>>>>>>>>>> --- a/drivers/watchdog/Kconfig
>>>>>>>>>>>>>>> +++ b/drivers/watchdog/Kconfig
>>>>>>>>>>>>>>> @@ -209,6 +209,26 @@ config WDT_K3_RTI
>>>>>>>>>>>>>>> Say Y here if you want to include support for the K3 watchdog
>>>>>>>>>>>>>>> timer (RTI module) available in the K3 generation of processors.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> +if WDT_K3_RTI
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> +config WDT_K3_RTI_LOAD_FW
>>>>>>>>>>>>>>> + bool "Load watchdog firmware"
>>>>>>>>>>>>>>> + depends on REMOTEPROC
>>>>>>>>>>>>>>> + help
>>>>>>>>>>>>>>> + Automatically load the specified firmware image into the MCU R5F
>>>>>>>>>>>>>>> + core 0. On the AM65x, this firmware is supposed to handle the expiry
>>>>>>>>>>>>>>> + of the watchdog timer, typically by resetting the system.
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> +config WDT_K3_RTI_FW_FILE
>>>>>>>>>>>>>>> + string "Watchdog firmware image file"
>>>>>>>>>>>>>>> + default "k3-rti-wdt.fw"
>>>>>>>>>>>>>>> + depends on WDT_K3_RTI_LOAD_FW
>>>>>>>>>>>>>>> + help
>>>>>>>>>>>>>>> + Firmware image to be embedded into U-Boot and loaded on watchdog
>>>>>>>>>>>>>>> + start.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I need your input on this proach. Is it okay to include the linker file unders
>>>>>>>>>>>>>> drivers?
>>>>>>>>>>>>>
>>>>>>>>>>>>> Maybe? I suppose the first thing that springs to mind is why aren't we
>>>>>>>>>>>>> using binman and including this blob (which I happily see is GPLv2)
>>>>>>>>>>>>> similar to how we do things with x86 for one example.
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> See https://www.mail-archive.com/u-boot@lists.denx.de/msg377894.html
>>>>>>>>>>>>
>>>>>>>>>>>> Jan
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Did this help to answer open questions? Otherwise, please let me know.
>>>>>>>>>>>
>>>>>>>>>>> I'd also like to avoid that his patch alone blocks 1-3 of the series
>>>>>>>>>>> needless - but I would also not mind getting everything in at once.
>>>>>>>>>>
>>>>>>>>>> Can you provide your reviewed-by if you are okay with this approach?
>>>>>>>>>
>>>>>>>>> I was kind of hoping Simon would chime in here on binman usage. So,
>>>>>>>>> re-re-reading the above URL, yes, fsloader wouldn't be the right choice
>>>>>>>>> for watchdog firmware. But I think binman_entry_find() and related
>>>>>>>>> could work, in general, for this case of "need firmware blob embedded in
>>>>>>>>> to image". That said, this isn't just any firmware blob, it's the
>>>>>>>>> watchdog firmware. The less reliance on other things the safer it is.
>>>>>>>>> That means this would be an exception to the general firmware blob
>>>>>>>>> loading rule and yeah, OK, we can do it this way. Sorry for the delay.
>>>>>>>>
>>>>>>>> Yes I've been a little tied up recently. But I think this should use
>>>>>>>> binman. We really don't want to be building binary firmware into
>>>>>>>> U-Boot itself!
>>>>>>>>
>>>>>>>> Also Tom says, see x86 for a load of binaries of different types and
>>>>>>>> how they are accessed at runttime. This is what binman is for.
>>>>>>>>
>>>>>>>
>>>>>>> Please take the time and study my arguments. I'm open for better
>>>>>>> proposals, but they need to be concrete and addressing my points.
>>>>>>
>>>>>> Do you mean 'properly hashed' and 'easy access', or something else?
>>>>>> What can binman not do?
>>>>>
>>>>> Binman itself can stick things into binary images. But that is at most
>>>>> half of the tasks needed here. I would need concrete guidance how to
>>>>>
>>>>> - access that binary from u-boot proper in a reasonably simple way
>>>>
>>>> I thought you wanted to access it from SPL. For that you would use
>>>> linker symbols. See 'Access to binman entry offsets at run time
>>>> (symbols)' in the binman docs for that.
>>>>
>>>> But for U-Boot proper, the section is 'Access to binman entry offsets
>>>> at run time (fdt)'. There is no mention of the runtime library that
>>>> now exists (binman.h) so I will send a patch for that. But basically
>>>> you call binman_entry_find("name", &entry) and it returns the offset
>>>> and size for you.
>>>>
>>>>> - make sure the binary can be signed and the signature is evaluated
>>>>> before using it
>>>>
>>>> Do you mean signed or hashed? I think you mean hashed. If you trust
>>>> the U-Boot binary then presumably you can put the firmware in a
>>>> similar place do you can trust that as well. After all, you seem happy
>>>> enough to link it into U-Boot.
>>>>
>>>> If not, you can ask binman to add a hash:
>>>>
>>>> my-firmware {
>>>> type = "blob";
>>>> hash {
>>>> algo = "sha256";
>>>> };
>>>> };
>>>>
>>>> Then you can add support for that to some sort of helper function in
>>>> binman.c, e.g.:
>>>>
>>>> ofnode node, hashnode;
>>>> const u8 *hash;
>>>> int size;
>>>>
>>>> node = binman_section_find_node("name");
>>>> if (!ofnode_valid(node))
>>>> ...return error
>>>> hashnode = ofnode_read_prop(node, "hash");
>>>> if (!ofnode_valid(hashnode))
>>>> ...return error
>>>> hash = ofnode_read_prop(hashnode, "value", &size);
>>>>
>>>> /* Hash the firmware...need to read it from flash into fwdata/fwlen
>>>> using binman_entry_find() ...then: */
>>>> u8 digest[SHA256_SUM_LEN];
>>>> ret = hash_block("sha256", fwdata, fwlen, digest, sizeof(digest);
>>>> if (ret)
>>>> return log_msg_ret("hash", ret);
>>>>
>>>> /* compare the hashes */
>>>> if (size != sizeof(digest) || memcmp(hash, digest))
>>>> return log_msg_ret("cmp", ret);
>>>>
>>>
>>> Yes, it will likely be a hash that will be signed, and all that will be
>>> checked by SPL when loading proper. The infrastructure for that should
>>> be there, just not yet plugged for the way of loading things like we do
>>> (one of my todos).
>>>
>>> Obviously, what would be simplest for that is to have the watchdog blob
>>> embedded, rather than separately hashed, as that would Just Work. I
>>> didn't fully grasp yet what you propose, but it seems right now it will
>>> complicate things. I'm not saying it will make it impossible, just harder.
>>
>> I'm not even sure that is true. In binman, add the entry:
>>
>> watchdog-firmware {
>> type = "blob";
>> filename = "...";
>> };
>>
>> In the C code, declare a symbol that will get the image position of the entry:
>>
>> binman_sym_declare(unsigned long, watchdog_firmware, image_pos);
>>
>> then read that value when you need it:
>>
>> int check_it(..)
>> {
>> ulong pos = binman_sym(ulong, watchdog_firmware, image_pos);
>>
>> // read from flash offset @pos into a buffer
>>
>> // check the hash
>> };
>
> This function is the extra boilerplate code that is not needed when the
> blob is simply part of the U-Boot binary that is loaded and checked by
> SPL. The worst part of this: It requires special handling of different
> storage media. We currently only have OSPI for out board, but you may
> also imagine versions that load U-Boot from filesystems (the TI EVM does
> that). So this is the extra complexity without extra value I'm always
> talking about.
>
> But I'm happy to take concrete suggestions where to add the firmware
> into our board and where to add the necessary code in a simple way.
>
> What we do so far: U-Boot proper and DTBs go into a fit image that SPL
> will load (and later also validate). It would be simple to do
>
> diff --git a/arch/arm/dts/k3-am65-iot2050-boot-image.dtsi b/arch/arm/dts/k3-am65-iot2050-boot-image.dtsi
> index 96647719df..d3242af70c 100644
> --- a/arch/arm/dts/k3-am65-iot2050-boot-image.dtsi
> +++ b/arch/arm/dts/k3-am65-iot2050-boot-image.dtsi
> @@ -60,6 +60,11 @@
> filename = "arch/arm/dts/k3-am6548-iot2050-advanced.dtb";
> };
> };
> +
> + k3-rti-firmware {
> + type = "blob";
> + filename = CONFIG_WDT_K3_RTI_FW_FILE;
> + };
> };
>
> configurations {
>
> in [1] (used by [2]).
>
> But now: How do I communicate that blob address from SPL to U-Boot
> proper so that I can skip the extra medium-specific loading part and
> also reuse the fit image validation of SPL? If there is a simple way for
> that, I could indeed switch the model.
>
OK, I think I found a trick (outside of binman): Making the firmware an
additional loadable in the u-boot fit image, and then picking its
location up from /fit-images/k3-rti-wdt-firmware in rti_wdt_load_fw().
That should be nicer then object embedded - without requiring the
complexity of separate image loading and validating.
Jan
--
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux
More information about the U-Boot
mailing list