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

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

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.

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?

>  
>      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?

Regards,

-- 
Julien Grall