Re: [PATCH v6] uboot-script-gen: Dynamically compute addr and size when loading binaries

[dmkhn@proton.me]

On Fri, Apr 25, 2025 at 12:40:40PM -0700, Stefano Stabellini wrote:
> From: Andrei Cherechesu <andrei.cherechesu@oss.nxp.com>
> 
> Normally, the Imagebuilder would precompute the sizes of the loaded
> binaries and addresses where they are loaded before generating the
> script, and the sizes and addresses that needed to be provided to
> Xen via /chosen would be hardcoded in the boot script.
> 
> Added an option via "-s" parameter to avoid hardcoding any
> address in the boot script, and dynamically compute the
> loading addresses for binaries. The first loading address is based
> on the MEMORY_START parameter and after loading each binary,
> the loading address and the size of the binary are stored in
> variables with corresponding names. Then, the loading address
> for the next binary is computed and aligned to 0x200000.
> 
> If the "-s" parameter is not used, the normal flow is executed,
> where the loading addresses and sizes for each binaries are
> precomputed and hardcoded inside the script, but the loading
> addresses and sizes for each binary are now also stored for eventual
> later use.
> 
> Reserved memory regions are left TBD in the -s case.
> 
> Link: https://lists.xenproject.org/archives/html/xen-devel/2022-09/msg01862.html
> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> ---
> Changes in v6:
> - make initial ${memaddr} setting depending on CALC
> 
> This patch adds quite a bit of complexity which is the reason why I
> didn't originally commit it. Now that we are enabling ImageBuilder in
> Yocto, it turns out this patch is required because Yocto invokes
> ImageBuilder before all the binaries are ready and available.
> 
> Andrei, sorry for taking so long to realize why it is essential, but we
> are getting there now.
> 
> The changes I made to the original patch are purely to make it simpler
> to maintain.
> ---
>  README.md                |   2 +
>  scripts/uboot-script-gen | 180 ++++++++++++++++++++++++++++++++-------
>  2 files changed, 151 insertions(+), 31 deletions(-)
> 
> diff --git a/README.md b/README.md
> index f8039ec..28c9e6b 100644
> --- a/README.md
> +++ b/README.md
> @@ -356,6 +356,8 @@ Where:\
>     can only be used  in combination with the -k option.  This adds the
>     public key into the dtb.  Then one can add this dtb back into the
>     u-boot bin or elf.\
> +-s addresses and sizes are calculated dynamically from U-Boot, hence
> +   binaries don't need to be available at the time of invocation.\
> 
>  ### Signed FIT images
> 
> diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen
> index 638154a..9ec0e20 100755
> --- a/scripts/uboot-script-gen
> +++ b/scripts/uboot-script-gen
> @@ -1,8 +1,11 @@
>  #!/bin/bash

Unrelated, but I think it is worth adding `set -e` and `set -o pipefail`
to the script.

> 
>  offset=$((2*1024*1024))
> +PADDING_MASK=`printf "0x%X\n" $(($offset - 1))`
> +PADDING_MASK_INV=`printf "0x%X\n" $((~$PADDING_MASK))`

I would use $(...) form for command substitution instead of `...` for
readability, although the script uses both styles already.

>  filesize=0
>  prog_req=(mkimage file fdtput mktemp awk od)
> +CALC=""
> 
>  function cleanup_and_return_err()
>  {
> @@ -100,17 +103,42 @@ function dt_set()
>      fi
>  }
> 
> +function dt_set_calc()
> +{
> +    local path=$1
> +    local var=$2
> +    local name_var=$3
> +
> +    addr_var="$name_var"_addr
> +    size_var="$name_var"_size

       ^ I think these two variables should be `local` too.

> +
> +    # Split data_addr_var into high/low
> +    echo "setexpr addr_hi \${"$addr_var"} / 0x100000000" >> $UBOOT_SOURCE
> +    echo "setexpr addr_lo \${"$addr_var"} \& 0xFFFFFFFF" >> $UBOOT_SOURCE
> +
> +    # Split data_size_var into high/low
> +    echo "setexpr size_hi \${"$size_var"} / 0x100000000" >> $UBOOT_SOURCE
> +    echo "setexpr size_lo \${"$size_var"} \& 0xFFFFFFFF" >> $UBOOT_SOURCE
> +
> +    echo "fdt set $path $var <0x\${addr_hi} 0x\${addr_lo} 0x\${size_hi} 0x\${size_lo}>" >> $UBOOT_SOURCE

I think using heredoc will simplify the code a bit, e.g.:

       cat >> $UBOOT_SOURCE <<EOF
# Split data_addr_var into high/low
setexpr addr_hi \${"$addr_var"} / 0x100000000
setexpr addr_lo \${"$addr_var"} \& 0xFFFFFFFF

# Split data_size_var into high/low
setexpr size_hi \${"$size_var"} / 0x100000000
setexpr size_lo \${"$size_var"} \& 0xFFFFFFFF

fdt set $path $var <0x\${addr_hi} 0x\${addr_lo} 0x\${size_hi} 0x\${size_lo}>
EOF

> +}
> +
>  function add_device_tree_kernel()
>  {
>      local path=$1
> -    local addr=$2
> -    local size=$3
> -    local bootargs=$4
> +    local name=$2
> +    local addr=$3
> +    local size=$4
> +    local bootargs=$5
>      local node_name="module@${addr#0x}"
> 
>      dt_mknode "$path" "$node_name"
>      dt_set "$path/$node_name" "compatible" "str_a" "multiboot,kernel multiboot,module"
> -    dt_set "$path/$node_name" "reg" "hex"  "$(split_addr_size $addr $size)"
> +    if test "$CALC"; then
> +        dt_set_calc "$path/$node_name" "reg" $name
> +    else
> +        dt_set "$path/$node_name" "reg" "hex"  "$(split_addr_size $addr $size)"
> +    fi
>      dt_set "$path/$node_name" "bootargs" "str" "$bootargs"
>  }
> 
> @@ -118,26 +146,36 @@ function add_device_tree_kernel()
>  function add_device_tree_ramdisk()
>  {
>      local path=$1
> -    local addr=$2
> -    local size=$3
> +    local name=$2
> +    local addr=$3
> +    local size=$4
>      local node_name="module@${addr#0x}"
> 
>      dt_mknode "$path"  "$node_name"
>      dt_set "$path/$node_name" "compatible" "str_a" "multiboot,ramdisk multiboot,module"
> -    dt_set "$path/$node_name" "reg" "hex"  "$(split_addr_size $addr $size)"
> +    if test "$CALC"; then
> +        dt_set_calc "$path/$node_name" "reg" $name
> +    else
> +        dt_set "$path/$node_name" "reg" "hex"  "$(split_addr_size $addr $size)"
> +    fi
>  }
> 
> 
>  function add_device_tree_passthrough()
>  {
>      local path=$1
> -    local addr=$2
> -    local size=$3
> +    local name=$2
> +    local addr=$3
> +    local size=$4
>      local node_name="module@${addr#0x}"
> 
>      dt_mknode "$path"  "$node_name"
>      dt_set "$path/$node_name" "compatible" "str_a" "multiboot,device-tree multiboot,module"
> -    dt_set "$path/$node_name" "reg" "hex"  "$(split_addr_size $addr $size)"
> +    if test "$CALC"; then
> +        dt_set_calc "$path/$node_name" "reg" $name
> +    else
> +        dt_set "$path/$node_name" "reg" "hex"  "$(split_addr_size $addr $size)"
> +    fi
>  }
> 
>  function add_device_tree_mem()
> @@ -358,7 +396,11 @@ function xen_device_tree_editing()
> 
>          dt_mknode "/chosen" "$node_name"
>          dt_set "/chosen/$node_name" "compatible" "str_a" "xen,xsm-policy xen,multiboot-module multiboot,module"
> -        dt_set "/chosen/$node_name" "reg" "hex" "$(split_addr_size $xen_policy_addr $xen_policy_size)"
> +        if test "$CALC"; then
> +            dt_set_calc "/chosen/$node_name" "reg" "xen_policy"
> +        else
> +            dt_set "/chosen/$node_name" "reg" "hex" "$(split_addr_size $xen_policy_addr $xen_policy_size)"
> +        fi
>      fi
> 
>      if test "$DOM0_KERNEL"
> @@ -367,7 +409,11 @@ function xen_device_tree_editing()
> 
>          dt_mknode "/chosen" "$node_name"
>          dt_set "/chosen/$node_name" "compatible" "str_a" "xen,linux-zimage xen,multiboot-module multiboot,module"
> -        dt_set "/chosen/$node_name" "reg" "hex" "$(split_addr_size $dom0_kernel_addr $dom0_kernel_size)"
> +        if test "$CALC"; then
> +            dt_set_calc "/chosen/$node_name" "reg" "dom0_linux"
> +        else
> +            dt_set "/chosen/$node_name" "reg" "hex" "$(split_addr_size $dom0_kernel_addr $dom0_kernel_size)"
> +        fi
>          dt_set "/chosen" "xen,dom0-bootargs" "str" "$DOM0_CMD"
>      fi
> 
> @@ -377,7 +423,11 @@ function xen_device_tree_editing()
> 
>          dt_mknode "/chosen" "$node_name"
>          dt_set "/chosen/$node_name" "compatible" "str_a" "xen,linux-initrd xen,multiboot-module multiboot,module"
> -        dt_set "/chosen/$node_name" "reg" "hex" "$(split_addr_size $ramdisk_addr $ramdisk_size)"
> +        if test "$CALC"; then
> +            dt_set_calc "/chosen/$node_name" "reg" "dom0_ramdisk"
> +        else
> +            dt_set "/chosen/$node_name" "reg" "hex" "$(split_addr_size $ramdisk_addr $ramdisk_size)"
> +        fi
>      fi
> 
>      i=0
> @@ -464,14 +514,14 @@ function xen_device_tree_editing()
> 
>          xen_dt_domu_add_vcpu_nodes "/chosen/domU$i" $i ${DOMU_VCPUS[$i]}
> 
> -        add_device_tree_kernel "/chosen/domU$i" ${domU_kernel_addr[$i]} ${domU_kernel_size[$i]} "${DOMU_CMD[$i]}"
> +        add_device_tree_kernel "/chosen/domU$i" "domU${i}_kernel" ${domU_kernel_addr[$i]} ${domU_kernel_size[$i]} "${DOMU_CMD[$i]}"
>          if test "${domU_ramdisk_addr[$i]}"
>          then
> -            add_device_tree_ramdisk "/chosen/domU$i" ${domU_ramdisk_addr[$i]} ${domU_ramdisk_size[$i]}
> +            add_device_tree_ramdisk "/chosen/domU$i" "domU${i}_ramdisk" ${domU_ramdisk_addr[$i]} ${domU_ramdisk_size[$i]}
>          fi
>          if test "${domU_passthrough_dtb_addr[$i]}"
>          then
> -            add_device_tree_passthrough "/chosen/domU$i" ${domU_passthrough_dtb_addr[$i]} ${domU_passthrough_dtb_size[$i]}
> +            add_device_tree_passthrough "/chosen/domU$i" "domU${i}_fdt" ${domU_passthrough_dtb_addr[$i]} ${domU_passthrough_dtb_size[$i]}
>          fi
>          i=$(( $i + 1 ))
>      done
> @@ -504,7 +554,11 @@ function device_tree_editing()
> 
>      if test $UBOOT_SOURCE
>      then
> -        echo "fdt addr $device_tree_addr" >> $UBOOT_SOURCE
> +        if test "$CALC"; then
> +            echo "fdt addr \${host_fdt_addr}" >> $UBOOT_SOURCE
> +        else
> +            echo "fdt addr $device_tree_addr" >> $UBOOT_SOURCE
> +        fi
>          echo "fdt resize 1024" >> $UBOOT_SOURCE
> 
>          if test $NUM_DT_OVERLAY && test $NUM_DT_OVERLAY -gt 0
> @@ -512,7 +566,11 @@ function device_tree_editing()
>              i=0
>              while test $i -lt $NUM_DT_OVERLAY
>              do
> -                echo "fdt apply ${dt_overlay_addr[$i]}" >> $UBOOT_SOURCE
> +                if test "$CALC"; then
> +                    echo "fdt apply \${host_fdto${i}_addr}" >> $UBOOT_SOURCE
> +                else
> +                    echo "fdt apply ${dt_overlay_addr[$i]}" >> $UBOOT_SOURCE
> +                fi
>                  i=$(( $i + 1 ))
>              done
>          fi
> @@ -530,8 +588,12 @@ function fill_reserved_spaces_from_dtb()
>  {
>      if [ ! -f $DEVICE_TREE ]
>      then
> -        echo "File $DEVICE_TREE doesn't exist, exiting";
> -        cleanup_and_return_err
> +        if test "$CALC"; then
> +            return
> +        else
> +            echo "File $DEVICE_TREE doesn't exist, exiting";
> +            cleanup_and_return_err
> +        fi
>      fi
> 
>      # Check if reserved-memory node exists
> @@ -613,7 +675,7 @@ function get_image_size()
>      printf "%u" $effective_size
>  }
> 
> -function add_size()
> +function add_size_from_file()
>  {
>      local filename=$1
>      local size=`stat -L --printf="%s" $filename`
> @@ -645,6 +707,24 @@ function add_size()
>      filesize=$size
>  }
> 
> +function add_size_calculate()
> +{
> +    local fit_scr_name=$1
> +
> +    echo "setenv "$fit_scr_name"_addr \${memaddr}" >> $UBOOT_SOURCE
> +    echo "setenv "$fit_scr_name"_size \${filesize}" >> $UBOOT_SOURCE
> +    echo "setexpr memaddr \${memaddr} \+ \${filesize}" >> $UBOOT_SOURCE
> +    echo "setexpr memaddr \${memaddr} \+ $PADDING_MASK" >> $UBOOT_SOURCE
> +    echo "setexpr memaddr \${memaddr} \& $PADDING_MASK_INV" >> $UBOOT_SOURCE
> +
> +    # TODO: missing ${RESERVED_MEM_SPACES[@]} check
> +
> +    # The following are updated to avoid collisions in node names, but
> +    # they are not actively used.
> +    memaddr=$((memaddr + offset))
> +    filesize=$offset
> +}
> +
>  function load_file()
>  {
>      local filename=$1
> @@ -657,10 +737,16 @@ function load_file()
>      if test "$FIT"
>      then
>          echo "imxtract \$fit_addr $fit_scr_name $memaddr" >> $UBOOT_SOURCE
> +        add_size_from_file $filename
>      else
> -        echo "$LOAD_CMD $memaddr ${prepend_path:+$prepend_path/}$relative_path" >> $UBOOT_SOURCE
> +        if test "$CALC"; then
> +            echo "$LOAD_CMD \${memaddr} ${prepend_path:+$prepend_path/}$relative_path" >> $UBOOT_SOURCE
> +            add_size_calculate $fit_scr_name
> +        else
> +            echo "$LOAD_CMD $memaddr ${prepend_path:+$prepend_path/}$relative_path" >> $UBOOT_SOURCE
> +            add_size_from_file $filename
> +        fi
>      fi
> -    add_size $filename
>  }
> 
>  function check_file_type()
> @@ -670,8 +756,13 @@ function check_file_type()
> 
>      if [ ! -f $filename ]
>      then
> -        echo "File $filename doesn't exist, exiting";
> -        cleanup_and_return_err
> +        if test "$CALC"
> +        then
> +            return
> +        else
> +            echo "File $filename doesn't exist, exiting";
> +            cleanup_and_return_err
> +        fi
>      fi
> 
>      # if file doesn't know what it is, it outputs data, so include that
> @@ -705,8 +796,13 @@ function check_compressed_file_type()
> 
>      if [ ! -f $filename ]
>      then
> -        echo "File $filename doesn't exist, exiting";
> -        cleanup_and_return_err
> +        if test "$CALC"
> +        then
> +            return
> +        else
> +            echo "File $filename doesn't exist, exiting";
> +            cleanup_and_return_err
> +        fi
>      fi
> 
>      file_type=$( file -L $filename )
> @@ -872,6 +968,12 @@ function linux_config()
>  generate_uboot_images()
>  {
>      local arch=$(file -L $XEN | grep -E 'ARM64|Aarch64')
> +
> +    if test "$CALC"
> +    then
> +        echo "bootm is not compatible with -s"
> +        cleanup_and_return_err
> +    fi
> 
>      if test "$arch"
>      then
> @@ -997,7 +1099,11 @@ bitstream_load_and_config()
>          if test "$UBOOT_SOURCE"
>          then
>              # we assume the FPGA device is 0 here
> -            echo "fpga load 0 $bitstream_addr $bitstream_size" >> "$UBOOT_SOURCE"
> +            if test "$CALC"; then
> +                echo "fpga load 0 \${fpga_bitstream_addr} \${fpga_bitstream_size}" >> "$UBOOT_SOURCE"
> +            else
> +                echo "fpga load 0 $bitstream_addr $bitstream_size" >> "$UBOOT_SOURCE"
> +            fi
>          fi
>      fi
>  }
> @@ -1271,7 +1377,7 @@ function print_help
>  {
>      script=`basename "$0"`
>      echo "usage:"
> -    echo "	$script -c CONFIG_FILE -d DIRECTORY [-t LOAD_CMD] [-o FILE] [-k KEY_DIR/HINT [-u U-BOOT_DTB]] [-e] [-f] [-p PREPEND_PATH]"
> +    echo "	$script -c CONFIG_FILE -d DIRECTORY [-t LOAD_CMD] [-o FILE] [-k KEY_DIR/HINT [-u U-BOOT_DTB]] [-e] [-f] [-p PREPEND_PATH] [-s]"
>      echo "	$script -h"
>      echo "where:"
>      echo "	CONFIG_FILE - configuration file"
> @@ -1288,6 +1394,7 @@ function print_help
>      echo "	U-BOOT_DTB - u-boot control dtb so that the public key gets added to it"
>      echo "	-f - enable generating a FIT image"
>      echo "	PREPEND_PATH - path to be appended before file names to match deploy location within rootfs"
> +    echo "	-s - enable dynamic loading of binaries by storing their addresses and sizes u-boot env variables"

I would update description to something like:
  
  let U-Boot calculate binary images load addresses/sizes dynamically

?

Also, I would make all options sorted alphabetically.

>      echo "	-h - prints out the help message and exits "
>      echo "Defaults:"
>      echo "	CONFIG_FILE=$cfg_file, UBOOT_TYPE=\"LOAD_CMD\" env var, DIRECTORY=$uboot_dir"
> @@ -1295,7 +1402,7 @@ function print_help
>      echo "	$script -c ../config -d ./build42 -t \"scsi load 1:1\""
>  }
> 
> -while getopts ":c:t:d:ho:k:u:fp:" opt; do
> +while getopts ":c:t:d:ho:k:u:fp:s" opt; do
>      case ${opt} in
>      t )
>          case $OPTARG in
> @@ -1340,6 +1447,9 @@ while getopts ":c:t:d:ho:k:u:fp:" opt; do
>      p )
>          prepend_path="$OPTARG"
>          ;;
> +    s )
> +        CALC=y
> +        ;;
>      h )
>          print_help
>          exit 0
> @@ -1533,6 +1643,10 @@ uboot_addr=$memaddr
>  # 2MB are enough for a uboot script
>  memaddr=$(( $memaddr + $offset ))
>  memaddr=`printf "0x%X\n" $memaddr`
> +if test "$CALC"
> +then
> +    echo "setenv memaddr $memaddr" >> $UBOOT_SOURCE
> +fi
> 
>  fill_reserved_spaces_from_dtb
> 
> @@ -1583,7 +1697,11 @@ fi
> 
>  if [ "$BOOT_CMD" != "none" ]
>  then
> -    echo "$BOOT_CMD $kernel_addr $([ "$BOOT_CMD" = "bootefi" ] || echo '-') $device_tree_addr" >> $UBOOT_SOURCE
> +    if test "$CALC"; then
> +        echo "$BOOT_CMD \${host_kernel_addr} $([ "$BOOT_CMD" = "bootefi" ] || echo '-') \${host_fdt_addr}" >> $UBOOT_SOURCE
> +    else
> +        echo "$BOOT_CMD $kernel_addr $([ "$BOOT_CMD" = "bootefi" ] || echo '-') $device_tree_addr" >> $UBOOT_SOURCE
> +    fi
>  else
>      # skip boot command but store load addresses to be used later
>      echo "setenv host_kernel_addr $kernel_addr" >> $UBOOT_SOURCE
> --
> 2.25.1
> 
>