| Message ID | 1396521943-30825-1-git-send-email-ian.campbell@citrix.com |
|---|---|
| State | New |
| Headers | show |
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.
On Thu, 2014-04-03 at 15:41 +0100, Ian Jackson wrote: > 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. I did mention it in the comment in the code, I could add it to the header too. Would you put it next to the struct defining ramdisk_seg or the xc_dom_build_image? Neither of them are well documented right now :-( > > @@ -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. It was: The emacs magic in tools/libxc/xc_dom_arm.c was wrong, so I corrected it while I was there. > > > +/* 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 ...) I mostly added them for the "i.e. DTB On ARM bit" on the first one, and having added that I thought I should say something on the other one too. I'm inclined to leave them, but to make my spelling of finalise be consistent. > > @@ -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 :-) ? Sure. > But I'm not sure of the need for chosen to be separate from res. Later on I reuse res but still need chosen in my hand. I thought it might be clearer to use res for the fdt_path_offset result while I was still treating it as a potential error code and launder it into chosen (a node offset) once it was shown to be fine. I suppose the fact you queried it means I was wrong. I'll just use chosen here and reserve res for the setprop_inplace_calls later on > This whole function is rather flabby, with two near-idential 9-line > stanzas. > > How can fdt_setprop_inplace fail ? Can you justify assert() ? * -FDT_ERR_NOSPACE, if len is not equal to the property's current length * -FDT_ERR_NOTFOUND, node does not have the named property * -FDT_ERR_BADOFFSET, nodeoffset did not point to FDT_BEGIN_NODE tag * -FDT_ERR_BADMAGIC, * -FDT_ERR_BADVERSION, * -FDT_ERR_BADSTATE, * -FDT_ERR_BADSTRUCTURE, * -FDT_ERR_TRUNCATED, standard meanings All of which I think would indicate a previous coding error in this file. Since the property must certainly exist and have the expected length etc. I think I can justify an assert. Ian.
Ian Campbell writes ("Re: [PATCH] tools: implement initial ramdisk support for ARM."):
> On Thu, 2014-04-03 at 15:41 +0100, Ian Jackson wrote:
> > This strikes me as rather subtle. Perhaps it should be documented in
> > the header file.
>
> I did mention it in the comment in the code, I could add it to the
> header too. Would you put it next to the struct defining ramdisk_seg or
> the xc_dom_build_image? Neither of them are well documented right
> now :-(
Either of those places. I spotted the comment in the code, but I
think it's part of the specification, not just the implmementation.
Moving the comment out to the header would do fine.
> > Not mentioned in your commit message, but fair enough.
>
> It was:
>
> The emacs magic in tools/libxc/xc_dom_arm.c was wrong, so I
> corrected it while I was there.
Oh, sorry.
> > These comments mostly simply repeat the function name. I would delete
> > the second one (but won't object if you want to keep it ...)
>
> I mostly added them for the "i.e. DTB On ARM bit" on the first one, and
> having added that I thought I should say something on the other one too.
> I'm inclined to leave them, but to make my spelling of finalise be
> consistent.
Heh. Fair enough.
> > > + 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 :-) ?
>
> Sure.
>
> > But I'm not sure of the need for chosen to be separate from res.
>
> Later on I reuse res but still need chosen in my hand.
>
> I thought it might be clearer to use res for the fdt_path_offset result
> while I was still treating it as a potential error code and launder it
> into chosen (a node offset) once it was shown to be fine. I suppose the
> fact you queried it means I was wrong.
Heh, everyone has a different idea about these things.
> I'll just use chosen here and reserve res for the setprop_inplace_calls
> later on
Fair enough.
> > This whole function is rather flabby, with two near-idential 9-line
> > stanzas.
> >
> > How can fdt_setprop_inplace fail ? Can you justify assert() ?
...
> All of which I think would indicate a previous coding error in this
> file. Since the property must certainly exist and have the expected
> length etc.
>
> I think I can justify an assert.
Excellent.
Thanks,
Ian.
diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c index a40e04d..3330f12 100644 --- a/tools/libxc/xc_dom_arm.c +++ b/tools/libxc/xc_dom_arm.c @@ -249,6 +249,18 @@ int arch_setup_meminit(struct xc_dom_image *dom) { int rc; xen_pfn_t pfn, allocsz, i; + uint64_t modbase; + + /* Convenient */ + const uint64_t rambase = dom->rambase_pfn << XC_PAGE_SHIFT; + const uint64_t ramend = rambase + ( dom->total_pages << XC_PAGE_SHIFT ); + const uint64_t kernend = ROUNDUP(dom->kernel_seg.vend, 21/*2MB*/); + const uint64_t dtb_size = dom->devicetree_blob ? + ROUNDUP(dom->devicetree_size, XC_PAGE_SHIFT) : 0; + const uint64_t ramdisk_size = dom->ramdisk_blob ? + ROUNDUP(dom->ramdisk_size, XC_PAGE_SHIFT) : 0; + const uint64_t modsize = dtb_size + ramdisk_size; + const uint64_t ram128mb = rambase + (128<<20); rc = set_mode(dom->xch, dom->guest_domid, dom->guest_type); if ( rc ) @@ -278,23 +290,49 @@ int arch_setup_meminit(struct xc_dom_image *dom) 0, 0, &dom->p2m_host[i]); } - if ( dom->devicetree_blob ) + + /* + * Place boot modules at 128MB into RAM if there is enough RAM and + * the kernel does not overlap. Otherwise place them immediately + * after the kernel. If there is no space after the kernel then + * there is insufficient RAM and we fail. + */ + if ( ramend >= ram128mb + modsize && kernend < ram128mb ) + modbase = ram128mb; + else if ( ramend >= kernend + modsize ) + modbase = kernend; + else + return -1; + + DOMPRINTF("%s: placing boot modules at 0x%" PRIx64, __FUNCTION__, modbase); + + /* + * Must map DTB *after* initrd, to satisfy order of calls to + * xc_dom_alloc_segment in xc_dom_build_image, which must map + * things at monotonolically increasing addresses. + */ + if ( ramdisk_size ) { - const uint64_t rambase = dom->rambase_pfn << XC_PAGE_SHIFT; - const uint64_t ramend = rambase + ( dom->total_pages << XC_PAGE_SHIFT ); - const uint64_t dtbsize = ROUNDUP(dom->devicetree_size, XC_PAGE_SHIFT); - - /* Place at 128MB if there is sufficient RAM */ - if ( ramend >= rambase + 128*1024*1024 + dtbsize ) - dom->devicetree_seg.vstart = rambase + 128*1024*1024; - else /* otherwise at top of RAM */ - dom->devicetree_seg.vstart = ramend - dtbsize; - - dom->devicetree_seg.vend = - dom->devicetree_seg.vstart + dom->devicetree_size; + dom->ramdisk_seg.vstart = modbase; + dom->ramdisk_seg.vend = modbase + ramdisk_size; + + DOMPRINTF("%s: ramdisk: 0x%" PRIx64 " -> 0x%" PRIx64 "", + __FUNCTION__, + dom->ramdisk_seg.vstart, dom->ramdisk_seg.vend); + + modbase += ramdisk_size; + } + + if ( dtb_size ) + { + dom->devicetree_seg.vstart = modbase; + dom->devicetree_seg.vend = modbase + dtb_size; + DOMPRINTF("%s: devicetree: 0x%" PRIx64 " -> 0x%" PRIx64 "", __FUNCTION__, dom->devicetree_seg.vstart, dom->devicetree_seg.vend); + + modbase += dtb_size; } return 0; @@ -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 * indent-tabs-mode: nil * End: */ diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c index b9d1015..a5cdbcc 100644 --- a/tools/libxc/xc_dom_core.c +++ b/tools/libxc/xc_dom_core.c @@ -955,13 +955,25 @@ int xc_dom_build_image(struct xc_dom_image *dom) size_t unziplen, ramdisklen; void *ramdiskmap; - unziplen = xc_dom_check_gzip(dom->xch, dom->ramdisk_blob, dom->ramdisk_size); - if ( xc_dom_ramdisk_check_size(dom, unziplen) != 0 ) + /* Assume that if something has explicitly placed the ramdisk + * then they would have uncompressed it if they wanted + * to. Otherwise decompressing risks undoing their careful + * placement. + */ + if ( !dom->ramdisk_seg.vstart ) + { + unziplen = xc_dom_check_gzip(dom->xch, + dom->ramdisk_blob, dom->ramdisk_size); + if ( xc_dom_ramdisk_check_size(dom, unziplen) != 0 ) + unziplen = 0; + } + else unziplen = 0; ramdisklen = unziplen ? unziplen : dom->ramdisk_size; - if ( xc_dom_alloc_segment(dom, &dom->ramdisk_seg, "ramdisk", 0, + if ( xc_dom_alloc_segment(dom, &dom->ramdisk_seg, "ramdisk", + dom->ramdisk_seg.vstart, ramdisklen) != 0 ) goto err; ramdiskmap = xc_dom_seg_to_ptr(dom, &dom->ramdisk_seg); diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h index aee0a91..d3bc136 100644 --- a/tools/libxl/libxl_arch.h +++ b/tools/libxl/libxl_arch.h @@ -19,7 +19,12 @@ int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config, uint32_t domid); -int libxl__arch_domain_configure(libxl__gc *gc, - libxl_domain_build_info *info, - struct xc_dom_image *dom); +/* 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); #endif diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c index 0cfd0cf..159ab73 100644 --- a/tools/libxl/libxl_arm.c +++ b/tools/libxl/libxl_arm.c @@ -2,6 +2,7 @@ #include "libxl_arch.h" #include <xc_dom.h> +#include <stdbool.h> #include <libfdt.h> #include <assert.h> @@ -31,6 +32,9 @@ typedef be32 gic_interrupt[3]; #define ROOT_ADDRESS_CELLS 2 #define ROOT_SIZE_CELLS 2 +#define PROP_INITRD_START "linux,initrd-start" +#define PROP_INITRD_END "linux,initrd-end" + static void set_cell(be32 **cellp, int size, uint64_t val) { int cells = size; @@ -155,7 +159,7 @@ static int make_root_properties(libxl__gc *gc, return 0; } -static int make_chosen_node(libxl__gc *gc, void *fdt, +static int make_chosen_node(libxl__gc *gc, void *fdt, bool ramdisk, const libxl_domain_build_info *info) { int res; @@ -169,6 +173,15 @@ static int make_chosen_node(libxl__gc *gc, void *fdt, if (res) return res; } + if (ramdisk) { + uint64_t dummy = 0; + LOG(DEBUG, "/chosen adding placeholder linux,initrd properties"); + res = fdt_property(fdt, PROP_INITRD_START, &dummy, sizeof(dummy)); + if (res) return res; + res = fdt_property(fdt, PROP_INITRD_END, &dummy, sizeof(dummy)); + if (res) return res; + } + res = fdt_end_node(fdt); if (res) return res; @@ -412,9 +425,9 @@ out: #define FDT_MAX_SIZE (1<<20) -int libxl__arch_domain_configure(libxl__gc *gc, - libxl_domain_build_info *info, - struct xc_dom_image *dom) +int libxl__arch_domain_init_hw_description(libxl__gc *gc, + libxl_domain_build_info *info, + struct xc_dom_image *dom) { void *fdt = NULL; int rc, res; @@ -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) ); FDT( make_cpus_node(gc, fdt, info->max_vcpus, ainfo) ); FDT( make_psci_node(gc, fdt) ); @@ -505,6 +518,58 @@ next_resize: goto out; } + rc = 0; + +out: + return rc; +} + +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 rc = ERROR_FAIL; + + const struct xc_dom_seg *ramdisk = dom->ramdisk_blob ? + &dom->ramdisk_seg : NULL; + + if (ramdisk) { + int chosen, res = fdt_path_offset(fdt, "/chosen"); + uint64_t val; + if (res < 0) { + LOG(ERROR, "FDT: fdt_path_offset of /chosen failed: %d = %s", + res, fdt_strerror(res)); + rc = ERROR_FAIL; + goto out; + } + chosen = res; + + LOG(DEBUG, "/chosen updating initrd properties to cover " + "%"PRIx64"-%"PRIx64, + ramdisk->vstart, ramdisk->vend); + + val = cpu_to_fdt64(ramdisk->vstart); + res = fdt_setprop_inplace(fdt, chosen, PROP_INITRD_START, + &val, sizeof(val)); + if (res) { + LOG(ERROR, + "FDT: setting " PROP_INITRD_START " property failed: %d = %s", + res, fdt_strerror(res)); + goto out; + } + + val = cpu_to_fdt64(ramdisk->vend); + res = fdt_setprop_inplace(fdt, chosen,PROP_INITRD_END, + &val, sizeof(val)); + if (res) { + LOG(ERROR, + "FDT: setting " PROP_INITRD_END " property failed: %d = %s", + res, fdt_strerror(res)); + goto out; + } + } + debug_dump_fdt(gc, fdt); rc = 0; diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c index 36e70b5..c3fa439 100644 --- a/tools/libxl/libxl_dom.c +++ b/tools/libxl/libxl_dom.c @@ -407,8 +407,8 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid, LOGE(ERROR, "xc_dom_parse_image failed"); goto out; } - if ( (ret = libxl__arch_domain_configure(gc, info, dom)) != 0 ) { - LOGE(ERROR, "libxl__arch_domain_configure failed"); + if ( (ret = libxl__arch_domain_init_hw_description(gc, info, dom)) != 0 ) { + LOGE(ERROR, "libxl__arch_domain_init_hw_description failed"); goto out; } if ( (ret = xc_dom_mem_init(dom, info->target_memkb / 1024)) != 0 ) { @@ -419,6 +419,10 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid, LOGE(ERROR, "xc_dom_boot_mem_init failed"); goto out; } + if ( (ret = libxl__arch_domain_finalise_hw_description(gc, info, dom)) != 0 ) { + LOGE(ERROR, "libxl__arch_domain_finalise_hw_description failed"); + goto out; + } if ( (ret = xc_dom_build_image(dom)) != 0 ) { LOGE(ERROR, "xc_dom_build_image failed"); goto out; diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c index b11d036..7589060 100644 --- a/tools/libxl/libxl_x86.c +++ b/tools/libxl/libxl_x86.c @@ -311,9 +311,16 @@ int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config, return ret; } -int libxl__arch_domain_configure(libxl__gc *gc, - libxl_domain_build_info *info, - struct xc_dom_image *dom) +int libxl__arch_domain_init_hw_description(libxl__gc *gc, + libxl_domain_build_info *info, + struct xc_dom_image *dom) +{ + return 0; +} + +int libxl__arch_domain_finalise_hw_description(libxl__gc *gc, + libxl_domain_build_info *info, + struct xc_dom_image *dom) { return 0; }
The ramdisk is passed to the kernel as a property in the chosen node of the device tree. This is somewhat tricky since in order to place the ramdisk and dtb in ram we first need to know the size of the dtb. So we initially create a DTB with placeholders for the ramdisk and finalise the value (which doesn't change the size) once we know where everything is. Rename libxl__arch_domain_configure to xl__arch_domain_init_hw_description to better reflect its use and to be consistent with the new libxl__arch_domain_finalise_hw_description. The common xc_dom_build_image() function did not support explicit placement of the ramdisk, instead passing 0 to xc_dom_alloc_segment, meaning "pick somewhere". This change instead passes ramdisk_seg.vstart. If nothing has set vstart then it will be zero because the entire dom struct is zeroed on allocation in xc_dom_allocate(). Therefore there is no change to the behaviour on x86. This is also consistent with how other segments (kernel, dtb) are handled. 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). With all that I'm able to boot a domain using the current Debian Jessie armhf installer initrd and have it complete successfully. The emacs magic in tools/libxc/xc_dom_arm.c was wrong, so I corrected it while I was there. Signed-off-by: Ian Campbell <ian.campbell@citrix.com> Cc: Ian Jackson <ian.jackson@citrix.com> --- I'm considering proposing this for backport to 4.4 since lack of initrd support is at the very least rather inconvenient for many use cases. Also, should we consider (separate from this change) making libxc not decompress the consider ramdisk by default? These days Linux at least can do the decompress itself and supports more algorithmns, like xz. --- tools/libxc/xc_dom_arm.c | 65 ++++++++++++++++++++++++++++++--------- tools/libxc/xc_dom_core.c | 18 +++++++++-- tools/libxl/libxl_arch.h | 11 +++++-- tools/libxl/libxl_arm.c | 75 ++++++++++++++++++++++++++++++++++++++++++--- tools/libxl/libxl_dom.c | 8 +++-- tools/libxl/libxl_x86.c | 13 ++++++-- 6 files changed, 160 insertions(+), 30 deletions(-)