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

[Ian Campbell <Ian.Campbell@citrix.com>]

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

>  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;
> >>  }
> >>  
> > 
> > 
> 
>