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

[Simon Glass]

Hi Sean,

On Thu, 17 Aug 2023 at 10:03, Sean Edmond <seanedmond at linux.microsoft.com>
wrote:
>
>
> 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.

Something odd about this email, but anyway, please consider whether (with
your changes) there is any point in still having the inline options. If
not, we should remove them in the same series, but need to explain the
code-size cost.

Regards,
Simon