Re: [ImageBuilder] Add config option to use separate load commands for Xen, DOM0 and DOMU binaries

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

Hi Stefano,

On 11/08/2025 22:38, Stefano Stabellini wrote:
> On Wed, 6 Aug 2025, Ayan Kumar Halder wrote:
> > Introduce the following options :-
> > 1. XEN_LOAD - This specifies command to load xen hypervisor binary and device
> > tree.
> > 2. DOM0_LOAD - This specifies command to load Dom0 binaries.
> > 3. DOMU_LOAD[] - This specifies command to load DomU binaries.
> >
> > There can be situations where Xen, Dom0 and DomU binaries are stored in
> > different partitions. Thus, imagebuilder should provide a way the binaries
> > using different commands.
> >
> > If any of the above options are not specified, LOAD_CMD is used by default.
> >
> > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> The patch is correct and the new config options look good. Two things.
>
> The following check should be removed or, better, modified to account
> for the new options:
>
>      if test ! "$LOAD_CMD"
>      then
>          echo "LOAD_CMD not set, either specify it in the config or set it with the 
> -t option"
>          exit 1
>      fi
>
> Thanks to this patch, it shouldn't be necessary to specify LOAD_CMD any
> longer.
Actually, we should keep this check when Linux is loaded (without Xen). IOW,

@@ -1557,6 +1551,11 @@ then
 elif test "$LINUX"
 then
     os="linux"
+    if test ! "$LOAD_CMD"
+    then
+        echo "LOAD_CMD not set, either specify it in the config or set 
it with the -t option"
+        exit 1
+    fi
     linux_config

> find_root_dev and fit are the only two remaining users of LOAD_CMD.
> While I think it makes sense to keep LOAD_CMD for fit, find_root_dev
> should probably use DOM0_LOAD instead.
yes.
> This incremental change (untested) should work. What do you think?
>
>
>
> diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen
> index 9e97944..867876f 100755
> --- a/scripts/uboot-script-gen
> +++ b/scripts/uboot-script-gen
> @@ -824,10 +824,10 @@ function check_compressed_file_type()
>   
>   function find_root_dev()
>   {
> -
> -    local dev=${LOAD_CMD%:*}
> +    local load_cmd=$1
> +    local dev=${load_cmd%:*}
>       dev=${dev##* }
> -    local par=${LOAD_CMD#*:}
> +    local par=${load_cmd#*:}
>   
>       if [ -z "$dev" ] || [ -z "$par" ]
>       then
> @@ -838,10 +838,10 @@ function find_root_dev()
>   
>       par=$((par + 1))
>   
> -    if [[ $LOAD_CMD =~ mmc ]]
> +    if [[ $load_cmd =~ mmc ]]
>       then
>           root_dev="/dev/mmcblk${dev}p${par}"
> -    elif [[ $LOAD_CMD =~ scsi ]]
> +    elif [[ $load_cmd =~ scsi ]]
>       then
>           # converts number to a scsi device character
>           dev=$((dev + 97))
> @@ -912,7 +912,7 @@ function xen_config()
>           then
>               DOM0_CMD="$DOM0_CMD root=/dev/ram0"
>           else
> -            find_root_dev
> +            find_root_dev "$DOM0_LOAD"
>               # $root_dev is set by find_root_dev
>               DOM0_CMD="$DOM0_CMD root=$root_dev"
>           fi
> @@ -960,7 +960,7 @@ function linux_config()
>           then
>               LINUX_CMD="$LINUX_CMD root=/dev/ram0"
>           else
> -            find_root_dev
> +            find_root_dev "$LOAD_CMD"
>               # $root_dev is set by find_root_dev
>               LINUX_CMD="$LINUX_CMD root=$root_dev"
>           fi
Till here the change looks ok.
> @@ -990,10 +990,6 @@ generate_uboot_images()
>   
>   xen_file_loading()
>   {
> -    if test -z "$DOM0_LOAD"
> -    then
> -        DOM0_LOAD="$LOAD_CMD"
> -    fi
This and
>       if test "$DOM0_KERNEL"
>       then
>           check_compressed_file_type $DOM0_KERNEL "executable\|uImage"
> @@ -1065,11 +1061,6 @@ xen_file_loading()
>           generate_uboot_images
>       fi
>   
> -    if test -z "${XEN_LOAD}"
> -    then
> -        XEN_LOAD="$LOAD_CMD"
> -    fi
> -
this, I have a concern about moving the changes out of xen_file_loading()

In xen_file_loading(), we set DOM0_LOAD, XEN_LOAD and DOMU_LOAD[i]. With 
this change, we set DOM0_LOAD , XEN_LOAD at the beginning of the script 
and DOMU_LOAD[i] in the function. This looks a bit odd to me.
Further DOM0_LOAD and XEN_LOAD should be set only when "$XEN" is set.

I can leave the change as it is in the current patch or I can move them 
to xen_config().
Please let me know your thoughts.

I have sent "[ImageBuilder v2] Add config option to use separate load 
commands for Xen, DOM0 and DOMU binaries" so that it becomes a bit clear.
- Ayan

>       kernel_addr=$memaddr
>       kernel_path=$XEN
>       load_file "$XEN" "host_kernel" "$XEN_LOAD"
> @@ -1518,12 +1509,22 @@ then
>       FIT="${UBOOT_SOURCE%.source}.fit"
>   fi
>   
> -if test ! "$LOAD_CMD"
> +if test ! "$LOAD_CMD" && ! test "$XEN_LOAD"
>   then
>       echo "LOAD_CMD not set, either specify it in the config or set it with the -t 
> option"
>       exit 1
>   fi
>   
> +if test -z "$DOM0_LOAD"
> +then
> +    DOM0_LOAD="$LOAD_CMD"
> +fi
> +
> +if test -z "${XEN_LOAD}"
> +then
> +    XEN_LOAD="$LOAD_CMD"
> +fi
> +
>   if test ! "$BOOT_CMD"
>   then
>       BOOT_CMD="booti"