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:52 PM, Ian Campbell wrote:

> On Thu, 2013-05-30 at 15:47 +0100, Julien Grall wrote:
>> 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?
> 
> We shouldn't make any assumptions based on whether we suspect there
> might be an appended DTB. We should just carry on.
> 
> The presence or absence of an appended DTB is purely the guests internal
> concern, it is none of our business, even if it breaks things

Ok. I will rework the patch and send it back alone.

>>  If yes, I think the user
>> will be confused because Linux won't use the right device tree.
> 
> Yes. Take a look at the Kconfig help text for the kernel option. This is
> a known shortcoming of APPEND_DTB, it's a hack and shouldn't be used.
> 
>> 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;
>>>>  }
>>>>  
>>>
>>>
>>
>>
> 
>