On Fri, 28 Feb 2025, Luca Miccio wrote:
> Currently, the uboot-script-gen does not account for reserved memory
> regions in the device tree. This oversight can lead to scenarios where
> one or more boot modules overlap with a reserved region. As a result,
> Xen will always crash upon detecting this overlap. However, the crash
> will be silent (without output) if earlyprintk is not enabled, which is
> the default setting at the moment.
>
> To address this issue, add a function that iterates over the
> reserved-memory nodes and populates an array. This array will be used
> later to calculate the load address for any given file.
>
> Signed-off-by: Luca Miccio <luca.mic...@amd.com>
Hi Luca,
Thanks for the nice patch! I was waiting for the 4.21 development window
to open.
> ---
> scripts/uboot-script-gen | 59 ++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 56 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen
> index db2c011..cd0d202 100755
> --- a/scripts/uboot-script-gen
> +++ b/scripts/uboot-script-gen
> @@ -468,6 +468,42 @@ function device_tree_editing()
> fi
> }
>
> +function fill_reserved_spaces_from_dtb()
> +{
> + if [ ! -f $DEVICE_TREE ]
> + then
> + echo "File $DEVICE_TREE doesn't exist, exiting";
> + cleanup_and_return_err
> + fi
> +
> + addr_cells=$(fdtget -t x $DEVICE_TREE /reserved-memory '#address-cells')
> + size_cells=$(fdtget -t x $DEVICE_TREE /reserved-memory '#size-cells')
missing "local" for both variables
> + for node in $(fdtget -l $DEVICE_TREE /reserved-memory); do
> + reg_values=($(fdtget -t x $DEVICE_TREE /reserved-memory/$node reg))
missing "local"
> + for ((i=0; i<${#reg_values[@]}; i+=addr_cells+size_cells)); do
> + addr=0
> + size=0
missing "local" for addr and size, and also i and j
> + for ((j=0; j<addr_cells; j++)); do
> + addr=$((addr << 32 | 0x${reg_values[i+j]}))
> + done
> +
> + for ((j=0; j<size_cells; j++)); do
> + size=$((size << 32 | 0x${reg_values[i+addr_cells+j]}))
> + done
> +
> + addr=$(printf "0x%X" $addr)
> + size=$(printf "0x%X" $size)
> + done
> +
> + # Add the reserved space to the list and avoid duplicates
> + if [[ ! " ${RESERVED_MEM_SPACES[@]} " =~ " ${addr},${size} " ]]; then
I think this is too imprecise as a check because it would match with a
similar element of the array with a higher number of zeros. If I read
this right:
0x1000,0x1000 would match 0x1000,0x10000
I would either remove this check, as it might be OK to have duplicates,
or I would turn it into a proper numeric check, one item at a time in
the list.
> + RESERVED_MEM_SPACES+=("${addr},${size}")
> + fi
> + done
> +}
> +
> # Read effective image size from a header, which may be larger than the
> filesize
> # due to noload sections, e.g. bss.
> function get_image_size()
> @@ -505,9 +541,24 @@ function add_size()
> size=${image_size}
> fi
>
> - memaddr=$(( $memaddr + $size + $offset - 1))
> - memaddr=$(( $memaddr & ~($offset - 1) ))
> - memaddr=`printf "0x%X\n" $memaddr`
> + # Try to place the file at the first available space...
> + local new_memaddr=$(( (memaddr + size + offset - 1) & ~(offset - 1) ))
> +
> + # ...then check if it overlaps with any reserved space
> + for reserved_space in "${RESERVED_MEM_SPACES[@]}"; do
> + local reserved_start=${reserved_space%,*}
> + local reserved_size=${reserved_space#*,}
> + local reserved_end=$((reserved_start + reserved_size))
> +
> + if [[ $new_memaddr -le $reserved_end ]] && \
> + [[ $((new_memaddr + size)) -ge $reserved_start ]]; then
> + # In that case, place the file at the next available space
> + # after the reserved one
> + new_memaddr=$(( (reserved_end + offset) & ~(offset - 1) ))
> + fi
> + done
> +
> + memaddr=$(printf "0x%X\n" $new_memaddr)
> filesize=$size
> }
>
> @@ -1373,6 +1424,8 @@ uboot_addr=$memaddr
> memaddr=$(( $memaddr + $offset ))
> memaddr=`printf "0x%X\n" $memaddr`
>
> +fill_reserved_spaces_from_dtb
> +
> if test "$os" = "xen"
> then
> xen_file_loading
> --
> 2.25.1
>
