Re: [PATCH v2 2/2] xen/arm: If the DOM0 zImage as a DTB appended replace it

[Julien Grall <julien.grall@linaro.org>]

On 05/30/2013 03:24 PM, Ian Campbell wrote:

> On Thu, 2013-05-30 at 15:05 +0100, Julien Grall wrote:
>> If a DTB is appended to the DOM0 zImage, it's probably means that the linux
>> decompression stage will replace the DBT register value (r2 on arm32)
>> by the address of the appended DTB.
> 
> I don't think we should do this, it certainly violates the principal of
> least surprise, since no other "bootloader" does anything like this
> AFAIK. Yes that means you occasionally trip over your DTB changes
> appearing not to take affect, but that's true on native and is one of
> the known and understood bad things about appended DTB.


Shall we load the DTB if there is one appended? If yes, I think the user
will be confused because Linux won't use the right device tree.
It's hard for the user to find the issue because there is no log.

> So I think this is a case of "don't do that then". At most we could
> issue a warning, I suppose. But really we shouldn't be making any
> assumptions (good or bad) about what happens to live in the memory just
> past the end of the kernel, it may or may not be an appended DTB.


I can add a warning and also fix the check the line:
if ( addr + end - start + sizeof(dtb_hdr) <= size )
must be replace by:
if ( end - start + sizeof(dtb_hdr) <= size )

>> In this case, to avoid issue with Linux, Xen needs to load the new device tree
>> just after the kernel.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> ---
>>  xen/arch/arm/kernel.c |   26 ++++++++++++--------------
>>  1 file changed, 12 insertions(+), 14 deletions(-)
>>
>> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
>> index f8c8850..1d6c927 100644
>> --- a/xen/arch/arm/kernel.c
>> +++ b/xen/arch/arm/kernel.c
>> @@ -123,20 +123,6 @@ static int kernel_try_zimage_prepare(struct kernel_info *info,
>>      if ( (end - start) > size )
>>          return -EINVAL;
>>  
>> -    /*
>> -     * Check for an appended DTB.
>> -     */
>> -    if ( addr + end - start + sizeof(dtb_hdr) <= size )
>> -    {
>> -        copy_from_paddr(&dtb_hdr, addr + end - start,
>> -                        sizeof(dtb_hdr), DEV_SHARED);
>> -        if (be32_to_cpu(dtb_hdr.magic) == DTB_MAGIC) {
>> -            end += be32_to_cpu(dtb_hdr.total_size);
>> -
>> -            if ( end > addr + size )
>> -                return -EINVAL;
>> -        }
>> -    }
>>  
>>      info->zimage.kernel_addr = addr;
>>  
>> @@ -154,6 +140,18 @@ static int kernel_try_zimage_prepare(struct kernel_info *info,
>>      info->load = kernel_zimage_load;
>>      info->type = KERNEL_ZIMAGE;
>>  
>> +    /*
>> +     * If there is an appended DTB, ask XEN to replace the DTB
>> +     * by the generate one.
>> +     */
>> +    if ( info->zimage.len + sizeof(dtb_hdr) <= size )
>> +    {
>> +        copy_from_paddr(&dtb_hdr, addr + end - start,
>> +                        sizeof(dtb_hdr), DEV_SHARED);
>> +        if (be32_to_cpu(dtb_hdr.magic) == DTB_MAGIC)
>> +            info->dtb_paddr = info->zimage.load_addr + info->zimage.len;
>> +    }
>> +
>>      return 0;
>>  }
>>  
> 
>