[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [ImageBuilder] uboot-script-gen: handle reserved memory regions


  • To: Stefano Stabellini <sstabellini@xxxxxxxxxx>
  • From: "Miccio, Luca" <luca.miccio@xxxxxxx>
  • Date: Tue, 11 Mar 2025 11:11:21 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass (sender ip is 165.204.84.17) smtp.rcpttodomain=kernel.org smtp.mailfrom=amd.com; dmarc=pass (p=quarantine sp=quarantine pct=100) action=none header.from=amd.com; dkim=none (message not signed); arc=none (0)
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector10001; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=E79lLws45ZBEe6luJS6Y165zfndfyoNjgEp6F3PC4J0=; b=HdFWPvf+MeaaFLmhxspzG8g57le2LVRq4D2G+vjxYMBMlytB7sJ7AjoONsnPVAK9SZYhAUdO1TO+lj16aT8HD72HhvIdaR5CUn2wmHeK6o/93/sHqJtbThgLPJqGq8ULXfze8kEl1jL378wkfGv7atOIDNzu+vtUm3HwZSostJbWHY9JT0aYo0OTrH9KMlMx8C9+if5cVIgeAKXK/YHMhwWl48IqQnKgTWwVN7jGNgglBrlpAKIbe0+N7qq4eKPfjvsrz6YJrSLXZPMX57W9858oeNGW1WQLKLxXhIsPT3OFCZYo+NAiEwi8a/qsVFuILO+s3DW0ABbXkpvKxKPuFA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector10001; d=microsoft.com; cv=none; b=M2GEFBettdIK/3/RehVR+q7KGmUsOSGfC08uBPO8GYyGroCcIYzbXRQOueX7MzmJKHrmC3AIn9m8Zie7M0LqTuPzvBfSYJWfohBlWxHQYhTwWmOcSE47RblZC5b87GkPLuKv37fl7s7iB2qzk2osKP+RC0gQuBC9R6G205a1BSYrEToOneszeLC+B6kmX1pl5ZKBOxN137Y07w+TTAWBcB8Q+0bFXUC97drfPRV1jm16pB1PRORzCmeDV6Rg7D8LcfSTt8PkWIhA+jEck+sZ8Ej0uq90CAVb+l24qIFQ3m6KKvjsuNPsIufIzgskPozgSdR/kP74o9oCGEEqEIXEbA==
  • Cc: <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Tue, 11 Mar 2025 10:11:37 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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@xxxxxxx>
>>
>> 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
>>>
>>




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.