| Message ID | 20250228150733.3945774-1-luca.miccio@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ImageBuilder] uboot-script-gen: handle reserved memory regions | expand |
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.miccio@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 >
On Thu, 6 Mar 2025, Stefano Stabellini wrote: > 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.miccio@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') One more comment. If the DT doesn't have any reserved memory regions: Error at '/reserved-memory': FDT_ERR_NOTFOUND Error at '/reserved-memory': FDT_ERR_NOTFOUND Error at '/reserved-memory': FDT_ERR_NOTFOUND It would be best to silence these errors > 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 > > >
Hi Stefano, On 3/7/2025 1:49 AM, Stefano Stabellini wrote: > On Thu, 6 Mar 2025, Stefano Stabellini wrote: >> 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.miccio@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') > > One more comment. If the DT doesn't have any reserved memory regions: > > Error at '/reserved-memory': FDT_ERR_NOTFOUND > Error at '/reserved-memory': FDT_ERR_NOTFOUND > Error at '/reserved-memory': FDT_ERR_NOTFOUND > > It would be best to silence these errors > > >> 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. >> You're right. I would go for the numeric check and avoid duplicates. Thanks, Luca >> >>> + 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 >>> >>
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') + for node in $(fdtget -l $DEVICE_TREE /reserved-memory); do + reg_values=($(fdtget -t x $DEVICE_TREE /reserved-memory/$node reg)) + + for ((i=0; i<${#reg_values[@]}; i+=addr_cells+size_cells)); do + addr=0 + size=0 + + 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 + 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
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.miccio@amd.com> --- scripts/uboot-script-gen | 59 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 56 insertions(+), 3 deletions(-)