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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 7 Oct 2022, Michal Orzel wrote:
> 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.

OK. In that case please rebase the patch on the "master-next" branch.
We'll figure out how to handle the dynamic address calculation at a
later time.