Re: [Xen-devel] [PATCH 1/2] xen/arm: Add support to load initrd in dom0

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2013-09-25 at 16:23 +0100, Julien Grall wrote:
> On 09/25/2013 04:15 PM, Ian Campbell wrote:
> > On Mon, 2013-09-16 at 16:20 +0100, Julien Grall wrote:
> >> @@ -806,7 +821,8 @@ static int prepare_dtb(struct domain *d, struct 
> >> kernel_info *kinfo)
> >>          goto err;
> >>  
> >>      /* Actual new size */
> >> -    new_size = fdt_totalsize(kinfo->fdt);
> >> +    initrd_len = early_info.modules.module[MOD_INITRD].size;
> > 
> > I think you need to check nr_modules here and in write_properties (which
> > I already trimmed by mistake.
> > 
> 
> Even if we check nr_modules, we can't assume MOD_INITRD is the last
> modules in the array, so it's possible to have nr_modules greater than
> MOD_INITRD but the module is not set. That's why I only choose to rely
> on size.

Hrm that's true.

> > .size may not be initialised otherwise. (in reality it's probably
> > in .bss not sure I want to rely on that though)
> 
> It's in .bss, on common/device_tree.c we already rely that this
> structure is zeroed (nr_modules is never initialized to 0).

OK, I guess its fine then.

> 
> 
> > 
> >> +    new_size = fdt_totalsize(kinfo->fdt) + initrd_len;
> >>  
> >>      /*
> >>       * DTB must be loaded such that it does not conflict with the
> >> @@ -815,15 +831,20 @@ static int prepare_dtb(struct domain *d, struct 
> >> kernel_info *kinfo)
> >>       * the recommendation in Documentation/arm64/booting.txt is below
> >>       * 512MB. Place at 128MB, (or, if we have less RAM, as high as
> >>       * possible) in order to satisfy both.
> >> +     * If the bootloader provides an initrd, it will be loaded just
> >> +     * after the DTB.
> >>       */
> >>      end = kinfo->mem.bank[0].start + kinfo->mem.bank[0].size;
> >>      end = MIN(kinfo->mem.bank[0].start + (128<<20) + new_size, end);
> >>  
> >> -    kinfo->dtb_paddr = end - fdt_totalsize(kinfo->fdt);
> >> +    kinfo->initrd_paddr = end - initrd_len;
> >> +    kinfo->initrd_paddr &= ~((1 << 20) - 1);
> > 
> > 1MB aligned, why not 2 like most things?
> 
> A mistake, I will fix it in the next patch series.

Actually, this extra alignment makes me think that maybe new_size needs
to account for the slop too? Does it?

Otherwise start + 128M + new_size doesn't make it such that
{dtb,initrd}_paddr are actually above 128M?

Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel