[PATCH 1/1] firmware: provide space for firmware fixups

Thu Oct 1 10:15:42 EDT 2020

On 01.10.20 14:08, Anup Patel wrote:
>
>
>> -----Original Message-----
>> From: Heinrich Schuchardt <xypron.glpk at gmx.de>
>> Sent: 01 October 2020 16:55
>> To: Anup Patel <Anup.Patel at wdc.com>; Atish Patra
>> <Atish.Patra at wdc.com>
>> Cc: OpenSBI List <opensbi at lists.infradead.org>; Atish Patra
>> <atishp at atishpatra.org>
>> Subject: Re: [PATCH 1/1] firmware: provide space for firmware fixups
>>
>> On 01.10.20 12:56, Anup Patel wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: opensbi <opensbi-bounces at lists.infradead.org> On Behalf Of
>>>> Heinrich Schuchardt
>>>> Sent: 01 October 2020 15:59
>>>> To: Atish Patra <Atish.Patra at wdc.com>
>>>> Cc: Heinrich Schuchardt <xypron.glpk at gmx.de>; OpenSBI List
>>>> <opensbi at lists.infradead.org>; Atish Patra <atishp at atishpatra.org>
>>>> Subject: [PATCH 1/1] firmware: provide space for firmware fixups
>>>>
>>>> We expand the provided device tree by up to 1056 bytes in fixup
>> operations.
>>>> This space must be allocated in the firmware binary.
>>>>
>>>> An alternative approach would be to allocate RAM for a copy of the
>>>> device tree but we do not have library functions for dynamic memory
>> allocation yet.
>>>>
>>>> Without this patch devicetree fixups lead to memory corruption.
>>>>
>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>>>> ---
>>>>  firmware/fw_payload.S | 2 ++
>>>>  scripts/d2c.sh        | 3 +++
>>>>  2 files changed, 5 insertions(+)
>>>>
>>>> diff --git a/firmware/fw_payload.S b/firmware/fw_payload.S index
>>>> 9805d8c..fdd082f 100644
>>>> --- a/firmware/fw_payload.S
>>>> +++ b/firmware/fw_payload.S
>>>> @@ -108,6 +108,8 @@ fw_options:
>>>>  	.globl fdt_bin
>>>>  fdt_bin:
>>>>  	.incbin	FW_PAYLOAD_FDT_PATH
>>>> +	/* space for device tree updates */
>>>> +	. = . + 0x420;
>>>>  #endif
>>>
>>> We should not add arbitrary padding like this. Instead we should have
>>> separate FW_PAYLOAD_FDT_PADDING options for config.mk.
>>>
>>>>
>>>>  	.section .payload, "ax", %progbits
>>>> diff --git a/scripts/d2c.sh b/scripts/d2c.sh index 821a995..1c53a71
>>>> 100755
>>>> --- a/scripts/d2c.sh
>>>> +++ b/scripts/d2c.sh
>>>> @@ -62,6 +62,9 @@ printf "const char __attribute__((aligned(%s)))
>>>> %s_start[] = {\n" "${OUTPUT_C_AL
>>>>
>>>>  od -v -t x1 -An ${INPUT_PATH} | awk '{for (i=1; i<=NF; i++) printf "
>>>> 0x%s,", $i; printf "\n"; }'
>>>>
>>>> +# Add 1056 bytes for device tree fixups echo dummy | awk '{for (j=1;
>>>> +j<=0x42; j++) {for (i=1; i<=0x10; i++) printf " 0x00,"; printf "\n"}; }'
>>>> +
>>>
>>> Here again, we should not have arbitrary padding. Instead:
>>> 1) Add optional parameter to d2c.sh for amount of padding required
>>> 2) In platform/kendryte/k210/objects.mk, add line "platform-padding-
>> k210.o = 1056"
>>
>> This numbers 1024 and 32 is defined by lib/utils/fdt/fdt_fixup.c. I understand
>> why defining constants makes sense. These constants may be used on
>> platform level but should not be defined there but in an include or common
>> Makefile.
>
> The FDT fixups will keep changing over-time so common makefile variable
> for amount of padding in lib/utils/fdt/objects.mk is fine.
>
>>
>>> 3) Extend "compile_d2c" and make rule to convert .dtb to .c file in
>>> toplevel Makfile
>>>
>>>>  printf "};\n"
>>>>
>>>>  printf "const unsigned long %s_size = sizeof(%s_start);\n"
>>>> "${OUTPUT_C_PREFIX}" "${OUTPUT_C_PREFIX}"
>>
>> We already have firmware/fw_payload.S including
>> FW_PAYLOAD_FDT_PATH.
>> Shouldn't we unify the way device trees are included in the firmware. We
>> could define a new variable defined by FW_PAYLOAD_FDT_PATH with a
>> fallback to a platform device tree.
>
> FW_PAYLOAD_FDT_PATH has limitations. The d2c.sh approach is more
> flexible and allows having multiple built-in DTBs whereas we can only include
> one DTB with FW_PAYLOAD_FDT_PATH. Also, the FW_PAYLOAD_FDT_PATH

Do you have an example with multiple dtbs? Will each of the dtbs need a
fixup? Is the idea that OpenSBI will determine the relevant dtb at runtime?

Best regards

Heinrich

> works only for FW_PAYLOAD and does not work for other firmwares whereas
> d2c.sh based built-in DTBs work for all OpenSBI firmwares.
>
> I certainly agree that we should have unified way of built-in DTBs.
>
> I prefer removing FW_PAYLOAD_FDT_PATH but first we have to figure-out
> how we can link external DTB to generic platform firmwares in absence of
> FW_PAYLOAD_FDT_PATH.
>
>>
>> This sort of patch should go in first before addressing the fixup issue.
>
> Agree.
>
>>
>> Best regards
>>
>> Heinrich
>>
>>>> --
>>>> 2.28.0
>>>>
>>>>
>>>> --
>>>> opensbi mailing list
>>>> opensbi at lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/opensbi
>>>
>>> Overall, this needs to be broken down into more patches:
>>> 1) Patch for adding FW_PAYLOAD_FDT_PADDING option
>>> 2) Patch for documenting FW_PAYLOAD_FDT_PADDING
>>> 3) Patch for optional padding parameter to d2c.sh
>>> 4) Patch to extend "compile_d2c" and make rule in top-level makefile
>>> 5) Patch to update platform/kendryte/k210/objects.mk
>>>
>>> Regards,
>>> Anup
>>>
>
> Regards,
> Anup
>