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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 28 Apr 2025, Andrei Cherechesu wrote:
> 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)

Both great suggestions, I'll do that


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

I'll send v8 with your R-b, please have a quick look that everything
checks out, thanks!