[PATCH 1/3] fdt: common API to populate kaslr seed

[Sean Edmond]

On 2023-08-15 7:44 a.m., Simon Glass wrote:
> Hi Sean,
>
> On Mon, 14 Aug 2023 at 13:12, Sean Edmond
> <seanedmond at linux.microsoft.com> wrote:
>>
>> On 2023-08-12 6:09 a.m., Simon Glass wrote:
>>> Hi Sean,
>>>
>>> On Fri, 11 Aug 2023 at 11:14, Sean Edmond <seanedmond at linux.microsoft.com>
>>> wrote:
>>>> On 2023-08-09 6:49 p.m., Simon Glass wrote:
>>>>> Hi Sean,
>>>>>
>>>>> On Wed, 9 Aug 2023 at 16:35, Sean Edmond <seanedmond at linux.microsoft.com>
>>> wrote:
>>>>>> On 2023-08-08 7:03 p.m., Simon Glass wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On Fri, 4 Aug 2023 at 17:34, <seanedmond at linux.microsoft.com> wrote:
>>>>>>>> From: Dhananjay Phadke <dphadke at linux.microsoft.com>
>>>>>>>>
>>>>>>>> fdt_fixup_kaslr_seed() will update given FDT with random seed value.
>>>>>>>> Source for random seed can be TPM or RNG driver in u-boot or sec
>>>>>>>> firmware (ARM).
>>>>>>>>
>>>>>>>> Signed-off-by: Dhananjay Phadke <dphadke at linux.microsoft.com>
>>>>>>>> ---
>>>>>>>>      arch/arm/cpu/armv8/sec_firmware.c | 32
>>> +++++++------------------------
>>>>>>>>      common/fdt_support.c              | 31
>>> ++++++++++++++++++++++++++++++
>>>>>>>>      include/fdt_support.h             |  3 +++
>>>>>>>>      3 files changed, 41 insertions(+), 25 deletions(-)
>>>>>>> We need to find a way to use the ofnode API here.
>>>>>>>
>>>>>>>> diff --git a/arch/arm/cpu/armv8/sec_firmware.c
>>> b/arch/arm/cpu/armv8/sec_firmware.c
>>>>>>>> index c0e8726346..84ba49924e 100644
>>>>>>>> --- a/arch/arm/cpu/armv8/sec_firmware.c
>>>>>>>> +++ b/arch/arm/cpu/armv8/sec_firmware.c
>>>>>>>> @@ -411,46 +411,28 @@ int sec_firmware_init(const void
>>> *sec_firmware_img,
>>>>>>>>      /*
>>>>>>>>       * fdt_fix_kaslr - Add kalsr-seed node in Device tree
>>>>>>>>       * @fdt:               Device tree
>>>>>>>> - * @eret:              0 in case of error, 1 for success
>>>>>>>> + * @eret:              0 for success
>>>>>>>>       */
>>>>>>>>      int fdt_fixup_kaslr(void *fdt)
>>>>>>> You could pass an oftree to this function, e.g. obtained with:
>>>>>>>
>>>>>>> oftree_from_fdt(fdt)
>>>>>> The common API I added is fdt_fixup_kaslr_seed(), which was added to
>>>>>> "common/fdt_support.c".
>>>>>>
>>>>>> There are 3 callers:
>>>>>> sec_firmware_init()->fdt_fixup_kaslr_seed()
>>>>>> do_kaslr_seed()->fdt_fixup_kaslr_seed()
>>>>>> image_setup_libfdt()->fdt_tpm_kaslr_seed->fdt_fixup_kaslr_seed()
>>>>>>
>>>>>> I think the ask is to create a common API that uses the ofnode API.
>>> So,
>>>>>> instead of fdt_fixup_kaslr_seed() I can create
>>>>>> ofnode_fixup_kaslr_seed()?  Where should it live?
>>>>> If you like you could add common/ofnode_support.c ?
>>>>>
>>>>> But it is OK to have it in the same file, I think.
>>>>>
>>>>>> Are you also wanting
>>>>>> the callers (eg. fdt_tpm_kaslr_seed, fdt_fixup_kaslr) to take oftree as
>>>>>> input too?
>>>>> So far as you can go, yes. Also you may want to pass an ofnode (the
>>>>> root node) so that things can deal with adding their stuff to any
>>>>> node.
>>>>>
>>>>> Regards,
>>>>> Simon
>>>> I re-worked the API to use the ofnode API and tested it on our board.  I
>>>> was required to explicitly enable CONFIG_OFNODE_MULTI_TREE in order for
>>>> it to work.
>>>>
>>>> I have concerns this will create a breaking change for users of the
>>>> kaslr fdt touch up.  In our case, if CONFIG_OFNODE_MULTI_TREE isn't set,
>>>> the control FDT gets touched up, not the kernel FDT as required.
>>>> Everything runs to completion, but "/proc/device-tree/chosen/kaslr-seed"
>>>> isn't present after boot.
>>>>
>>>> Am I missing something?  Perhaps there's a way to modify the default
>>>> value for CONFIG_OFNODE_MULTI_TREE to ensure this works out-of-the-box?
>>>>
>>> Yes, perhaps we should enable this when fixups are used? Is there a way to
>>> tell?
>> I don't think there's a way to tell unfortunately.  Fixups are always
>> called if OF_LIBFDT is enabled, and if an FDT is found during bootm.
>>
>> I'm having trouble understanding the intention of the current default
>> for OFNODE_MULTI_TREE:
>>       default y if EVENT && !DM_DEV_READ_INLINE && !DM_INLINE_OFNODE
>> Could we simplify to this?
>>           default y if !OF_LIVE
> I don't think it will build if inlining is used, but I can't remember...
I wasn't able to break this by turning off DM_DEV_READ_INLINE, and 
enabling OFNODE_MULTI_TREE/SPL_DM_INLINE_OFNODE/TPL_DM_INLINE_OFNODE. 
Perhaps things have changes since this was created.
> The EVENT thing is because there is an of-fixup event, which was the
> original thing using ofnode fixups.
>
> I wonder what sort of size increment this will create with !OF_LIVE ?

With default (OFNODE_MULTI_TREE is not set):

textdatabssdechex filename

9380135536846752 1040133fdf05 u-boot/u-boot/u-boot


With !OF_LIVE (OFNODE_MULTI_TREE is set with OFNODE_MULTI_TREE_MAX=4):

seanedmond at ovlvm106:~/code/QEMU/qemu_uboot$ sizeu-boot/u-boot/u-boot

textdatabssdechex filename

9390165536846752 1041136fe2f0 u-boot/u-boot/u-boot

>
>>> Also, we should make it return an error when attempting to use a tree
>>> without that option enabled. I would expect oftree_ensure() to provide that?
>> I'll add a check.
> OK thanks.
>
> Regards,
> Simon
>
>