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

[Andrei Cherechesu <andrei.cherechesu@xxxxxxxxxxx>]

Hi Stefano,

On 26/04/2025 02:35, 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>
> ---
> 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.

I'm very glad that you're finding it useful too now!

The original problem for us was not related to building with
Imagebuilder from Yocto, but this was rather done to help users
have more flexibility with the actual binaries they're
deploying on target (more specifically, with their sizes).

In other words, as long as the same file names are used, their
corresponding sizes do not need to be hard-coded too in the
script, since we can dynamically figure them out and write them
to /chosen. As such, the binaries (xen, kernel image, fdt,
ramdisks) can be replaced without regenerating the boot script
and caring about their sizes.

Regarding building from Yocto, in our case, we're invoking
Imagebuilder from some separate recipe after everything has
been built. The only situation that comes to mind where it's a
bit more tricky to wait for the artifacts to be ready is for the
ramdisk/initramfs ones, but it can be done too. So it can work
both with precomputed and dynamic addresses.

>
> The changes I made to the original patch are purely to make it simpler to 
> maintain.

Thank you for taking the time to refactor and resend it!
Please also see my comments below.

> ---
>
> 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}

The quotes here should also be removed, since quotes are literals
in heredoc:
    setenv ${fit_scr_name}_addr \${memaddr}
    setenv ${fit_scr_name}_size \${filesize}

Otherwise, we'll be getting this in the actual boot script
    setenv "dom0_linux"_addr ${memaddr}
    setenv "dom0_linux"_size ${filesize}

instead of
    setenv dom0_linux_addr ${memaddr}
    setenv dom0_linux_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))

I know you want to make this patch less complicated and I agree
with that, since there was originally some more logic here which
was not necessarily needed and was complicating things.

But even though this variable is only needed for 'module@..."
node names, I think we should at least convert it to hex, to
avoid getting confusing node names, like:
    fdt mknod /chosen/domU0 module@2239758336

Keeping this here should be enough:
    memaddr=$(printf "0x%X\n" $memaddr)

> +    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

With the above changes,

Reviewed-by: Andrei Cherechesu <andrei.cherechesu@xxxxxxx>


Thank you!

Regards,
Andrei Cherechesu