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

[Sean Edmond]

On 2023-08-15 10:46 a.m., Sean Edmond wrote:
>
> 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

Sorry about the formatting... let's try that again.

With default (OFNODE_MULTI_TREE is not set):

text: 938013
data: 55368
bss:46752
dec: 1040133
hex: fdf05

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

text: 939016
data: 55368
bss: 46752
dec: 1041136
hex: fe2f0

>
>>
>>>> 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
>>
>>
>
>