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

[Sean Edmond <seanedmond@linux.microsoft.com>]

On 2023-08-12 6:09 a.m., Simon Glass wrote:
> Hi Sean,
>
> On Fri, 11 Aug 2023 at 11:14, Sean Edmond <seanedmond@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@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@linux.microsoft.com> wrote:
>>>>>> From: Dhananjay Phadke <dphadke@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@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
>
> 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.
>
> Regards,
> Simon
>
>