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

[dmkhn@xxxxxxxxx]

On Fri, Apr 25, 2025 at 04:35:06PM -0700, Stefano Stabellini wrote:
> From: Andrei Cherechesu <andrei.cherechesu@xxxxxxxxxxx>
> 
> 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@xxxxxxx>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxx>

Reviewed-by: Denis Mukhin <dmukhin@xxxxxxxx>

> ---
> Changes in v7:
> - use $()
> - better description and alphabetical order
> - use heredoc
> 
> 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.
> ---
> 
> 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..73d9600 100755
> --- a/scripts/uboot-script-gen
> +++ b/scripts/uboot-script-gen
> @@ -1,8 +1,11 @@
>  #!/bin/bash
> 
>  offset=$((2*1024*1024))
> +PADDING_MASK=$(printf "0x%X\n" $(($offset - 1)))
> +PADDING_MASK_INV=$(printf "0x%X\n" $((~$PADDING_MASK)))
>  filesize=0
>  prog_req=(mkimage file fdtput mktemp awk od)
> +CALC=""
> 
>  function cleanup_and_return_err()
>  {
> @@ -100,17 +103,40 @@ function dt_set()
>      fi
>  }
> 
> +function dt_set_calc()
> +{
> +    local path=$1
> +    local var=$2
> +    local name_var=$3
> +
> +    local addr_var="$name_var"_addr
> +    local size_var="$name_var"_size
> +
> +    cat >> $UBOOT_SOURCE <<- EOF
> +     setexpr addr_hi \${$addr_var} / 0x100000000
> +     setexpr addr_lo \${$addr_var} \& 0xFFFFFFFF
> +     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 +144,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 +394,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 +407,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 +421,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 +512,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 +552,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 +564,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 +586,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 +673,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 +705,26 @@ function add_size()
>      filesize=$size
>  }
> 
> +function add_size_calculate()
> +{
> +    local fit_scr_name=$1
> +
> +    cat >> $UBOOT_SOURCE <<- EOF
> +     setenv "$fit_scr_name"_addr \${memaddr}
> +     setenv "$fit_scr_name"_size \${filesize}
> +     setexpr memaddr \${memaddr} \+ \${filesize}
> +     setexpr memaddr \${memaddr} \+ $PADDING_MASK
> +     setexpr memaddr \${memaddr} \& $PADDING_MASK_INV
> +     EOF
> +
> +    # 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"
> @@ -1289,13 +1395,14 @@ function print_help
>      echo "   -f - enable generating a FIT image"
>      echo "   PREPEND_PATH - path to be appended before file names to match 
> deploy location within rootfs"
>      echo "   -h - prints out the help message and exits "
> +    echo "   -s - let U-Boot calculate binary images load addresses/sizes 
> dynamically"
>      echo "Defaults:"
>      echo "   CONFIG_FILE=$cfg_file, UBOOT_TYPE=\"LOAD_CMD\" env var, 
> DIRECTORY=$uboot_dir"
>      echo "Example:"
>      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