Re: [PATCH] ImageBuilder: Add support for 64-bit addresses

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Stefano,

On 07/10/2022 00:34, Stefano Stabellini wrote:
> 
> 
> +xen-devel
> 
> On Thu, 6 Oct 2022, Michal Orzel wrote:
>> At the moment, ImageBuilder assumes that all addresses/sizes are
>> 32-bit max. It sets #{address,size}-cells to 0x2 and puts 0x0 as the
>> value for the first cell. Because of that, we cannot specify MEMORY_START
>> and MEMORY_END to be above 32-bits (e.g. to place the images in the
>> upper memory bank).
>>
>> Add support to properly handle 64-bit addresses:
>>  - add function split_into_halves to split the value passed as a first
>>    argument into upper and lower halves. These are then set as values for
>>    variables passed respetively as the second and third argument,
>>  - whenever there is a variable storing the full 64-bit number with
>>    "addr" or "size" in name, introduce two additional variables with
>>    "addr1,addr2"/"size1,size2" in name to store the halves. These are
>>    then used to properly set cells.
>>
>> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
>>
>> ---
>>  scripts/uboot-script-gen | 60 +++++++++++++++++++++++++++++++++++-----
>>  1 file changed, 53 insertions(+), 7 deletions(-)
>>
>> diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen
>> index 16269f02f1e7..4c6525a910f3 100755
>> --- a/scripts/uboot-script-gen
>> +++ b/scripts/uboot-script-gen
>> @@ -25,6 +25,14 @@ function dt_mknode()
>>      fi
>>  }
>>
>> +# Usage:
>> +# split_into_halves <value> <variable_to_store_upper> 
>> <variable_to_store_lower>
>> +function split_into_halves()
>> +{
>> +    eval "$2=$(printf "0x%X\n" $(($1 >> 32)))"
>> +    eval "$3=$(printf "0x%X\n" $(($1 & 0xFFFFFFFF)))"
>> +}
> 
> I know it is the same thing, but I would prefer the following version
> because it makes it easier to read:
> 
> # Usage:
> # split_into_halves <value> <variable_to_store_upper> 
> <variable_to_store_lower>
> function split_into_halves()
> {
>     local value=$1
>     local upper=$2
>     local lower=$3
> 
>     eval "$upper=$(printf "0x%X\n" $(($value >> 32)))"
>     eval "$lower=$(printf "0x%X\n" $(($value & 0xFFFFFFFF)))"
> }
That is ok for me.

> 
> 
>> +
>>  # data_type is either
>>  #   int
>>  #   hex
>> @@ -41,10 +49,14 @@ function dt_set()
>>
>>      if test $data_type = "var"
>>      then
>> -        eval data_addr_var="$data"_addr
>> -        eval data_addr=\$"$data_addr_var"
>> -        eval data_size_var="$data"_size
>> -        eval data_size=\$"$data_size_var"
>> +        eval data_addr1_var="$data"_addr1
>> +        eval data_addr2_var="$data"_addr2
>> +        eval data_addr1=\$"$data_addr1_var"
>> +        eval data_addr2=\$"$data_addr2_var"
>> +        eval data_size1_var="$data"_size1
>> +        eval data_size2_var="$data"_size2
>> +        eval data_size1=\$"$data_size1_var"
>> +        eval data_size2=\$"$data_size2_var"
> 
> To avoid making the code more complex, is it possible to stick with just
> a single data_addr variable in u-boot and calculate the upper and lower
> 32-bit using u-boot commands?
The reason why we need these extra variables is to add them into respective
cells under different nodes. In dt_set we need to put the variable names
for dynamic assignment and variable values for static assignment. We cannot
do this having a single pair data_addr_var,data_addr. These evals corresponds
to variables from xen_file_loading. dt_set and add_size are two different
functions. The former is used to create the nodes and the latter is used to
set values for the environment variables.

Example:
dt_set "/chosen/dom0" "reg" "var" "dom0_linux"
- this will create a reg property for dom0 kernel. We need to insert the upper
and lower halves into this property (so we need separate variables for that)
e.g.
reg <0x${dom0_linux_addr1} 0x${dom0_linux_addr2} 0x${dom0_linux_size1} 
0x${dom0_linux_size2}>

load_file $DOM0_KERNEL "dom0_linux" calling add_size
- this will set values for upper and lower halves into u-boot env variables
that corresponds to variables we placed previously in reg property,
e.g.
setenv dom0_linux_addr1 ${memaddr1}
setenv dom0_linux_addr2 ${memaddr2}
setenv dom0_linux_size1 ${filesize1}
setenv dom0_linux_size2 ${filesize2}

FWICS, we cannot achieve this using a single pair.

~Michal