Hi Sean, On Mon, 14 Aug 2023 at 13:12, Sean Edmond <seanedm...@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 <seanedm...@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 <seanedm...@linux.microsoft.com> > > wrote: > >>>> On 2023-08-08 7:03 p.m., Simon Glass wrote: > >>>>> Hi, > >>>>> > >>>>> On Fri, 4 Aug 2023 at 17:34, <seanedm...@linux.microsoft.com> wrote: > >>>>>> From: Dhananjay Phadke <dpha...@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 <dpha...@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... 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_LlIVE ? > > > > 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
