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.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')
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
> >
>
