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