Re: [Xen-devel] [PATCH] tools: implement initial ramdisk support for ARM.

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Ian Campbell writes ("[PATCH] tools: implement initial ramdisk support
for ARM."):
> [stuff]

Thanks.  This will be useful.  I have some fairly minor comments:

> Furthermore if the ramdisk has been explicitly placed then 
> xc_dom_build_image()
> assumes that it is not to be decompressed (since that would muck up the 
> sizings
> used on placement).

This strikes me as rather subtle.  Perhaps it should be documented in
the header file.

> @@ -326,7 +364,6 @@ int xc_dom_feature_translated(struct xc_dom_image *dom)
>   * mode: C
>   * c-file-style: "BSD"
>   * c-basic-offset: 4
> - * tab-width: 4

Not mentioned in your commit message, but fair enough.

> +/* setup arch specific hardware description, i.e. DTB on ARM */
> +int libxl__arch_domain_init_hw_description(libxl__gc *gc,
> +                                           libxl_domain_build_info *info,
> +                                           struct xc_dom_image *dom);
> +/* finalize arch specific hardware description. */
> +int libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
> +                                      libxl_domain_build_info *info,
> +                                      struct xc_dom_image *dom);

These comments mostly simply repeat the function name.  I would delete
the second one (but won't object if you want to keep it ...)

> @@ -475,7 +488,7 @@ next_resize:
>          FDT( fdt_begin_node(fdt, "") );
>  
>          FDT( make_root_properties(gc, vers, fdt) );
> -        FDT( make_chosen_node(gc, fdt, info) );
> +        FDT( make_chosen_node(gc, fdt, !!dom->ramdisk_blob, info) );

The !! are redundant when converting an expression to a bool.  But you
can leave them in if you feel it's clearer.

> +    if (ramdisk) {
> +        int chosen, res = fdt_path_offset(fdt, "/chosen");

That's rather confusing.  At the very least can we have these as two
lines so it doesn't look like a multiple-values-bind to Lisp
programmers :-) ?  But I'm not sure of the need for chosen to be
separate from res.

This whole function is rather flabby, with two near-idential 9-line
stanzas.

How can fdt_setprop_inplace fail ?  Can you justify assert() ?

Thanks,
Ian.

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