Re: [Xen-devel] [PATCH v4 7/9] tools: arm: prepare guest FDT building for multiple RAM banks

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2014-05-13 at 13:35 +0100, Julien Grall wrote:
> Hi Ian,
> 
> On 05/13/2014 12:29 PM, Ian Campbell wrote:
> > This required exposing the sizes of the banks determined by the domain 
> > builder
> > up to libxl via xc_dom_image.
> > 
> > Since the domain build needs to know the size of the DTB we create 
> > placeholder
> > nodes for each possible bank and when we finialise the DTB we fill in the 
> > ones
> 
> finalise
> 
> > which are actually populated and NOP out the rest.
> 
> It's annoying that we can't dissociate bank setup (i.e defining the size
> of each banks) and placing modules (kernel, DTB,...).
> 
> It would avoid to have 2 step to create memory node.

We have to have 2 steps for the initrd case (that's unavoidable), given
that having something similar for the memory is not the end of the
world.

> Anyway, I'm fine with this solution for now.
> 
> [..]
> 
> >  int libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
> >                                                 libxl_domain_build_info 
> > *info,
> >                                                 struct xc_dom_image *dom)
> >  {
> >      void *fdt = dom->devicetree_blob;
> > +    int i;
> > +    const uint64_t bankbase[GUEST_RAM_BANKS] = {
> > +        GUEST_RAM0_BASE
> > +    };
> 
> You've duplicate this variable in multiple place (twice in libxl and
> once in libxc).
> 
> I think it's better if we can avoid duplicating this variable, maybe by
> storing in xc_dom_image?

I'll take a look. Since these are currently constants I might just
#define GUEST_RAM_BASES { GUEST_RAM0_BASE , GUEST_RAM1_BASE }
So that these become const uunt64_t bankbase[] = GUEST_RAM_BASES;

> 
> >  
> >      const struct xc_dom_seg *ramdisk = dom->ramdisk_blob ?
> >          &dom->ramdisk_seg : NULL;
> > @@ -552,9 +588,16 @@ int 
> > libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
> >          assert(!res);
> >  
> >          val = cpu_to_fdt64(ramdisk->vend);
> > -        res = fdt_setprop_inplace(fdt, chosen,PROP_INITRD_END,
> > +        res = fdt_setprop_inplace(fdt, chosen, PROP_INITRD_END,
> >                                    &val, sizeof(val));
> 
> Spurious change?

It was deliberate, I just spotted the coding style error while I was in
the region. Naughty to slip it in here I know. I'll mention it in the
commit log.

Ian.


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