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.hal...@amd.com>
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"

Reply via email to