| Message ID | 1421159133-31526-22-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 29/01/15 11:03, Stefano Stabellini wrote: > On Tue, 13 Jan 2015, Julien Grall wrote: >> Let the user to pass additional nodes to the guest device tree. For this >> purpose, everything in the node /passthrough from the partial device tree will >> be copied into the guest device tree. >> >> The node /aliases will be also copied to allow the user to define aliases >> which can be used by the guest kernel. >> >> A simple partial device tree will look like: >> >> /dts-v1/; >> >> / { >> #address-cells = <2>; >> #size-cells = <2>; >> >> passthrough { >> compatible = "simple-bus"; >> ranges; >> #address-cells = <2>; >> #size-cells = <2>; >> >> /* List of your nodes */ >> } >> }; > > It would be nice to have an example of this under tools/examples. Ok. I will add one. [..] >> +/* >> + * Check if a string stored the strings block section is correctly >> + * nul-terminated. >> + * off_dt_strings and size_dt_strings fields have been validity-check >> + * earlier, so it's safe to use them here. >> + */ >> +static bool check_string(void *fdt, int nameoffset) >> +{ >> + const char *str = fdt_string(fdt, nameoffset); >> + >> + for (; nameoffset < fdt_size_dt_strings(fdt); nameoffset++, str++) { >> + if (*str == '\0') >> + return true; >> + } >> + >> + return false; >> +} > > strnlen? I could but it would not tell us directly if the string is NULL terminated or not. What about memchr? [..] >> +static int copy_node_by_path(libxl__gc *gc, const char *path, >> + void *fdt, void *pfdt) >> +{ >> + int nodeoff, r; >> + const char *name = strrchr(path, '/'); >> + >> + if (!name) >> + return -FDT_ERR_INTERNAL; >> + >> + name++; >> + >> + /* The FDT function to look at node doesn't take into account the >> + * unit (i.e anything after @) when search by name. Check if the >> + * name exactly match. >> + */ >> + nodeoff = fdt_path_offset(pfdt, path); >> + if (nodeoff < 0) >> + return nodeoff; >> + >> + if (strcmp(fdt_get_name(pfdt, nodeoff, NULL), name)) >> + return -FDT_ERR_NOTFOUND; > > Are we sure that the string returned by fdt_get_name is NULL terminated? Yes, libfdt does some sanity check on it (see fdt_next_tag case FDT_BEGIN_NODE). I tried to fix all the possible security flaw in libfdt (and there is quite a lot). If we don't trust the rest of libfdt, then we have to import our own and fix it. Regards,
On 23/02/15 11:46, Ian Campbell wrote: > On Tue, 2015-01-13 at 14:25 +0000, Julien Grall wrote: >> Let the user to pass additional nodes to the guest device tree. For this >> purpose, everything in the node /passthrough from the partial device tree will >> be copied into the guest device tree. >> >> The node /aliases will be also copied to allow the user to define aliases >> which can be used by the guest kernel. >> >> A simple partial device tree will look like: >> >> /dts-v1/; >> >> / { >> #address-cells = <2>; >> #size-cells = <2>; > > Are these mandatory/required as implied below, or only the ones inside > the passthrough node (which is what I would expect). It's to make DTC quiet. >> >> passthrough { >> compatible = "simple-bus"; >> ranges; >> #address-cells = <2>; >> #size-cells = <2>; >> >> /* List of your nodes */ >> } >> }; >> >> Note that: >> * The interrupt-parent proporties will be added by the toolstack in > > "properties" > >> the root node >> * The properties compatible, ranges, #address-cells and #size-cells >> in /passthrough are mandatory. > > Does ranges need to be the empty form? I think ranges = <stuff stuff> > would be illegal? It's not illegal as long as you correctly use it in the inner "reg". Also, I admit that the "ranges" is confusing to read. >> >> Signed-off-by: Julien Grall <julien.grall@linaro.org> >> Cc: Ian Jackson <ian.jackson@eu.citrix.com> >> Cc: Wei Liu <wei.liu2@citrix.com> >> >> --- >> Changes in v3: >> - Patch added >> --- >> docs/man/xl.cfg.pod.5 | 7 ++ >> tools/libxl/libxl_arm.c | 253 ++++++++++++++++++++++++++++++++++++++++++++ >> tools/libxl/libxl_types.idl | 1 + >> tools/libxl/xl_cmdimpl.c | 1 + >> 4 files changed, 262 insertions(+) >> >> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5 >> index e2f91fc..225b782 100644 >> --- a/docs/man/xl.cfg.pod.5 >> +++ b/docs/man/xl.cfg.pod.5 >> @@ -398,6 +398,13 @@ not emulated. >> Specify that this domain is a driver domain. This enables certain >> features needed in order to run a driver domain. >> >> +=item B<device_tree=PATH> >> + >> +Specify a partial device tree (compiled via the Device Tree Compiler). >> +Everything under the node "/passthrough" will be copied into the guest >> +device tree. For convenience, the node "/aliases" is also copied to allow >> +the user to defined aliases which can be used by the guest kernel. >> + >> =back >> >> =head2 Devices >> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c >> index 53177eb..619458b 100644 >> --- a/tools/libxl/libxl_arm.c >> +++ b/tools/libxl/libxl_arm.c >> @@ -540,6 +540,238 @@ out: >> } >> } >> >> +static bool check_overrun(uint64_t a, uint64_t b, uint32_t max) >> +{ >> + return ((a + b) > UINT_MAX || (a + b) > max); > > Both halves here will fail if e.g. a == UINT64_MAX-1 and b == 2, so e..g > a+b <= UINT_MAX and < max. Oops right. > To avoid this you should check that a and b are both less than some > fraction of UINT64_MAX before the other checks, which would ensure the > overflow can't happen, perhaps even UINT32_MAX would be acceptable for > this use, depending on the input types involved. max is an uint32_t so a and b should be inferior to UINT32_MAX. What about a < UINT_MAX && b < UINT_MAX && (a + b) < UINT_MAX > >> +/* >> + * Based on fdt_first_subnode and fdt_next_subnode. >> + * We can't use the one helpers provided by libfdt because: >> + * - It has been introduced in 2013 => Doesn't work on wheezy >> + * - The prototypes exist but the functions are not exported. Don't >> + * ask why... >> + * - There is no version in the header (might be better to use >> + * configure for this purpose?) >> + */ >> +static int _fdt_first_subnode(const void *fdt, int offset) > > The _ namespace is for something other than applications (POSIX or the > compiler, I forget which is _ and which __) > > Using configure to conditionally compile our own versions with the usual > names would be better I think. I will give a look. > The copyright and licensing of these functions should be included > somewhere either just above their definitions, or maybe you want to put > these into a separate .c file for convenience. Ok. >> +{ >> + int depth = 0; >> + >> + offset = fdt_next_node(fdt, offset, &depth); >> + if (offset < 0 || depth != 1) >> + return -FDT_ERR_NOTFOUND; >> + >> + return offset; >> +} >> + >> +static int _fdt_next_subnode(const void *fdt, int offset) >> +{ >> + int depth = 1; >> + >> + /* >> + * With respect to the parent, the depth of the next subnode will be >> + * the same as the last. >> + */ >> + do { >> + offset = fdt_next_node(fdt, offset, &depth); >> + if (offset < 0 || depth < 1) >> + return -FDT_ERR_NOTFOUND; >> + } while (depth > 1); >> + >> + return offset; >> +} >> + >> +/* Limit the maxixum depth of a node because of the recursion */ > > "maximum". > >> +#define MAX_DEPTH 8 > > This restriction ought to be in the docs I think. ok. >> +static int copy_node_by_path(libxl__gc *gc, const char *path, >> + void *fdt, void *pfdt) >> +{ >> + int nodeoff, r; >> + const char *name = strrchr(path, '/'); >> + >> + if (!name) >> + return -FDT_ERR_INTERNAL; >> + >> + name++; >> + >> + /* The FDT function to look at node doesn't take into account the > ^a > >> + * unit (i.e anything after @) when search by name. Check if the >> + * name exactly match. > > "exactly matches" or "names exactly match" (I think the second?) We check only one name at the time. So "exactly matches" seems better. >> +/* >> + * The partial device tree is not copied entirely. Only the relevant bits are >> + * copied to the guest device tree: >> + * - /passthrough node >> + * - /aliases node > > Would it be easy to add a check for other top-level nodes and > error/warn? That would require to browse the device tree entirely, which is not done there. >> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl >> index 1214d2e..5651110 100644 >> --- a/tools/libxl/libxl_types.idl >> +++ b/tools/libxl/libxl_types.idl >> @@ -399,6 +399,7 @@ libxl_domain_build_info = Struct("domain_build_info",[ >> ("kernel", string), >> ("cmdline", string), >> ("ramdisk", string), >> + ("device_tree", string), > > Needs a #define LIBXL_HAVE... in libxl.h Hmmm why? This will be set to empty when libxl_domain_build_info is initialized. Regards,
Hi Ian, On 23/02/15 12:03, Ian Jackson wrote: > Julien Grall writes ("[PATCH v3 21/24] tools/(lib)xl: Add partial device tree support for ARM"): >> Let the user to pass additional nodes to the guest device tree. For this >> purpose, everything in the node /passthrough from the partial device tree \ > will >> be copied into the guest device tree. > > Please wrap your commit messages to 70, not 80. I though commit message has to be wrapped to 80. I will change it. > >> +=item B<device_tree=PATH> >> + >> +Specify a partial device tree (compiled via the Device Tree Compiler). >> +Everything under the node "/passthrough" will be copied into the guest >> +device tree. For convenience, the node "/aliases" is also copied to allow >> +the user to defined aliases which can be used by the guest kernel. > > This is rather odd. The config option is `device_tree' but apparently > it is only relevant for passthrough and nothing else can be set with > it. I had to chose a name for the node and "/passthrough" was the best one and it won't collapse with the device tree generated by the toolstack. Although, you can put pretty much everything in the "/passthrough" node. >> +static int check_partial_fdt(libxl__gc *gc, void *fdt, size_t size) >> +{ > ... >> + /* Check if the *size and off* fields doesn't overrun the totalsize >> + * of the partial FDT. >> + */ >> + if (fdt_totalsize(fdt) > size) { >> + LOG(ERROR, "Partial FDT totalsize is too big"); >> + return ERROR_FAIL; >> + } > > There's lots and lots of this very fragile binary parsing code. > > Is this facility supposed to take untrusted or partially-trusted > partial device trees ? It may take untrusted device tree. I review the libfdt code and try to fix all possible security issue in the toolstack. > If so then I suspect we need a different approach. It might be easer > to rewrite this whole functionality in a programming language which is > less fragile in the face of programming errors, than to try to make > this whole thing secure (and review it). > > I'm definitely having XSA-55 flashbacks. It's not my plan to have an XSA-55 like :). As discussed IRL, we can mark this option "unsafe". So any device tree pass to libxl should be trusted. I will add an item in the description. Regards,
Hi Ian, Sorry for the late answer. On 23/02/15 17:22, Ian Campbell wrote: > On Mon, 2015-02-23 at 17:06 +0000, Julien Grall wrote: >> On 23/02/15 11:46, Ian Campbell wrote: >>> On Tue, 2015-01-13 at 14:25 +0000, Julien Grall wrote: >>>> Let the user to pass additional nodes to the guest device tree. For this >>>> purpose, everything in the node /passthrough from the partial device tree will >>>> be copied into the guest device tree. >>>> >>>> The node /aliases will be also copied to allow the user to define aliases >>>> which can be used by the guest kernel. >>>> >>>> A simple partial device tree will look like: >>>> >>>> /dts-v1/; >>>> >>>> / { >>>> #address-cells = <2>; >>>> #size-cells = <2>; >>> >>> Are these mandatory/required as implied below, or only the ones inside >>> the passthrough node (which is what I would expect). >> >> It's to make DTC quiet. > > Maybe add /* Keep DTC happy */ to both lines? > >> >>>> >>>> passthrough { >>>> compatible = "simple-bus"; >>>> ranges; >>>> #address-cells = <2>; >>>> #size-cells = <2>; >>>> >>>> /* List of your nodes */ >>>> } >>>> }; >>>> >>>> Note that: >>>> * The interrupt-parent proporties will be added by the toolstack in >>> >>> "properties" >>> >>>> the root node >>>> * The properties compatible, ranges, #address-cells and #size-cells >>>> in /passthrough are mandatory. >>> >>> Does ranges need to be the empty form? I think ranges = <stuff stuff> >>> would be illegal? >> >> It's not illegal as long as you correctly use it in the inner "reg". > > OK. This could be explained in some more complete documentaiton I think. > (It's a doc day on Wednesday ;-)) > >> >> Also, I admit that the "ranges" is confusing to read. >> >>>> >>>> Signed-off-by: Julien Grall <julien.grall@linaro.org> >>>> Cc: Ian Jackson <ian.jackson@eu.citrix.com> >>>> Cc: Wei Liu <wei.liu2@citrix.com> >>>> >>>> --- >>>> Changes in v3: >>>> - Patch added >>>> --- >>>> docs/man/xl.cfg.pod.5 | 7 ++ >>>> tools/libxl/libxl_arm.c | 253 ++++++++++++++++++++++++++++++++++++++++++++ >>>> tools/libxl/libxl_types.idl | 1 + >>>> tools/libxl/xl_cmdimpl.c | 1 + >>>> 4 files changed, 262 insertions(+) >>>> >>>> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5 >>>> index e2f91fc..225b782 100644 >>>> --- a/docs/man/xl.cfg.pod.5 >>>> +++ b/docs/man/xl.cfg.pod.5 >>>> @@ -398,6 +398,13 @@ not emulated. >>>> Specify that this domain is a driver domain. This enables certain >>>> features needed in order to run a driver domain. >>>> >>>> +=item B<device_tree=PATH> >>>> + >>>> +Specify a partial device tree (compiled via the Device Tree Compiler). >>>> +Everything under the node "/passthrough" will be copied into the guest >>>> +device tree. For convenience, the node "/aliases" is also copied to allow >>>> +the user to defined aliases which can be used by the guest kernel. >>>> + >>>> =back >>>> >>>> =head2 Devices >>>> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c >>>> index 53177eb..619458b 100644 >>>> --- a/tools/libxl/libxl_arm.c >>>> +++ b/tools/libxl/libxl_arm.c >>>> @@ -540,6 +540,238 @@ out: >>>> } >>>> } >>>> >>>> +static bool check_overrun(uint64_t a, uint64_t b, uint32_t max) >>>> +{ >>>> + return ((a + b) > UINT_MAX || (a + b) > max); >>> >>> Both halves here will fail if e.g. a == UINT64_MAX-1 and b == 2, so e..g >>> a+b <= UINT_MAX and < max. >> >> Oops right. >> >>> To avoid this you should check that a and b are both less than some >>> fraction of UINT64_MAX before the other checks, which would ensure the >>> overflow can't happen, perhaps even UINT32_MAX would be acceptable for >>> this use, depending on the input types involved. >> >> max is an uint32_t so a and b should be inferior to UINT32_MAX. > > by "inferior to" do you mean less than? Or something to do with type > promotion/demotion rules? I meant less than. >> >> What about >> >> a < UINT_MAX && b < UINT_MAX && (a + b) < UINT_MAX > > Isn't that inverted from the sense which the function name requires? > > Given the complexity in reasoning about this I think a series of > individual if and return statements which check each precondition one at > a time and return failure if necessary wuold be clearer to read and > reason about than trying to encode it all in one expression. Given that we will mark the option unsafe. I'm thinking to drop this check and some others. This would make the code less complex and avoid to check on half of the FDT. > >>>> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl >>>> index 1214d2e..5651110 100644 >>>> --- a/tools/libxl/libxl_types.idl >>>> +++ b/tools/libxl/libxl_types.idl >>>> @@ -399,6 +399,7 @@ libxl_domain_build_info = Struct("domain_build_info",[ >>>> ("kernel", string), >>>> ("cmdline", string), >>>> ("ramdisk", string), >>>> + ("device_tree", string), >>> >>> Needs a #define LIBXL_HAVE... in libxl.h >> >> Hmmm why? This will be set to empty when libxl_domain_build_info is >> initialized. > > So that applications which use libxl can take advantage of the new > feature when built against versions of the library which support it, > without breaking when built against versions which do not. Hmmm right. I will add the LIBXL_HAVE_PARTIAL_DEVICE_TREE. Regards,
diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5 index e2f91fc..225b782 100644 --- a/docs/man/xl.cfg.pod.5 +++ b/docs/man/xl.cfg.pod.5 @@ -398,6 +398,13 @@ not emulated. Specify that this domain is a driver domain. This enables certain features needed in order to run a driver domain. +=item B<device_tree=PATH> + +Specify a partial device tree (compiled via the Device Tree Compiler). +Everything under the node "/passthrough" will be copied into the guest +device tree. For convenience, the node "/aliases" is also copied to allow +the user to defined aliases which can be used by the guest kernel. + =back =head2 Devices diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c index 53177eb..619458b 100644 --- a/tools/libxl/libxl_arm.c +++ b/tools/libxl/libxl_arm.c @@ -540,6 +540,238 @@ out: } } +static bool check_overrun(uint64_t a, uint64_t b, uint32_t max) +{ + return ((a + b) > UINT_MAX || (a + b) > max); +} + +/* Only FDT v17 is supported */ +#define FDT_REQUIRED_VERSION 0x11 + +static int check_partial_fdt(libxl__gc *gc, void *fdt, size_t size) +{ + int r; + + if (size < FDT_V17_SIZE) { + LOG(ERROR, "Partial FDT is too small"); + return ERROR_FAIL; + } + + if (fdt_magic(fdt) != FDT_MAGIC) { + LOG(ERROR, "Partial FDT is not a valid Flat Device Tree"); + return ERROR_FAIL; + } + + if (fdt_version(fdt) != FDT_REQUIRED_VERSION) { + LOG(ERROR, "Partial FDT version not supported. Required 0x%x got 0x%x", + FDT_REQUIRED_VERSION, fdt_version(fdt)); + return ERROR_FAIL; + } + + r = fdt_check_header(fdt); + if (r) { + LOG(ERROR, "Failed to check the partial FDT (%d)", r); + return ERROR_FAIL; + } + + /* Check if the *size and off* fields doesn't overrun the totalsize + * of the partial FDT. + */ + if (fdt_totalsize(fdt) > size) { + LOG(ERROR, "Partial FDT totalsize is too big"); + return ERROR_FAIL; + } + + size = fdt_totalsize(fdt); + if (fdt_off_dt_struct(fdt) > size || + fdt_off_dt_strings(fdt) > size || + check_overrun(fdt_off_dt_struct(fdt), fdt_size_dt_struct(fdt), size) || + check_overrun(fdt_off_dt_strings(fdt), fdt_size_dt_strings(fdt), size)) { + LOG(ERROR, "Failed to validate the header of the partial FDT"); + return ERROR_FAIL; + } + + return 0; +} + +/* + * Check if a string stored the strings block section is correctly + * nul-terminated. + * off_dt_strings and size_dt_strings fields have been validity-check + * earlier, so it's safe to use them here. + */ +static bool check_string(void *fdt, int nameoffset) +{ + const char *str = fdt_string(fdt, nameoffset); + + for (; nameoffset < fdt_size_dt_strings(fdt); nameoffset++, str++) { + if (*str == '\0') + return true; + } + + return false; +} + +static int copy_properties(libxl__gc *gc, void *fdt, void *pfdt, + int nodeoff) +{ + int propoff, nameoff, r; + const struct fdt_property *prop; + + for (propoff = fdt_first_property_offset(pfdt, nodeoff); + propoff >= 0; + propoff = fdt_next_property_offset(pfdt, propoff)) { + + if (!(prop = fdt_get_property_by_offset(pfdt, propoff, NULL))) { + return -FDT_ERR_INTERNAL; + } + + /* + * Libfdt doesn't perform any check on the validity of a string + * stored in the strings block section. As the property name is + * stored there, check it. + */ + nameoff = fdt32_to_cpu(prop->nameoff); + if (!check_string(pfdt, nameoff)) { + LOG(ERROR, "The strings block section of the partial FDT is malformed"); + return -FDT_ERR_BADSTRUCTURE; + } + + r = fdt_property(fdt, fdt_string(pfdt, nameoff), + prop->data, fdt32_to_cpu(prop->len)); + if (r) return r; + } + + /* FDT_ERR_NOTFOUND => There is no more properties for this node */ + return (propoff != -FDT_ERR_NOTFOUND)? propoff : 0; +} + +/* + * Based on fdt_first_subnode and fdt_next_subnode. + * We can't use the one helpers provided by libfdt because: + * - It has been introduced in 2013 => Doesn't work on wheezy + * - The prototypes exist but the functions are not exported. Don't + * ask why... + * - There is no version in the header (might be better to use + * configure for this purpose?) + */ +static int _fdt_first_subnode(const void *fdt, int offset) +{ + int depth = 0; + + offset = fdt_next_node(fdt, offset, &depth); + if (offset < 0 || depth != 1) + return -FDT_ERR_NOTFOUND; + + return offset; +} + +static int _fdt_next_subnode(const void *fdt, int offset) +{ + int depth = 1; + + /* + * With respect to the parent, the depth of the next subnode will be + * the same as the last. + */ + do { + offset = fdt_next_node(fdt, offset, &depth); + if (offset < 0 || depth < 1) + return -FDT_ERR_NOTFOUND; + } while (depth > 1); + + return offset; +} + +/* Limit the maxixum depth of a node because of the recursion */ +#define MAX_DEPTH 8 + +/* Copy a node from the partial device tree to the guest device tree */ +static int copy_node(libxl__gc *gc, void *fdt, void *pfdt, + int nodeoff, int depth) +{ + int r; + + if (depth >= MAX_DEPTH) { + LOG(ERROR, "Partial FDT contains a too depth node"); + return -FDT_ERR_INTERNAL; + } + + r = fdt_begin_node(fdt, fdt_get_name(pfdt, nodeoff, NULL)); + if (r) return r; + + r = copy_properties(gc, fdt, pfdt, nodeoff); + if (r) return r; + + for (nodeoff = _fdt_first_subnode(pfdt, nodeoff); + nodeoff >= 0; + nodeoff = _fdt_next_subnode(pfdt, nodeoff)) { + r = copy_node(gc, fdt, pfdt, nodeoff, depth + 1); + if (r) return r; + } + + if (nodeoff != -FDT_ERR_NOTFOUND) + return nodeoff; + + r = fdt_end_node(fdt); + if (r) return r; + + return 0; +} + +static int copy_node_by_path(libxl__gc *gc, const char *path, + void *fdt, void *pfdt) +{ + int nodeoff, r; + const char *name = strrchr(path, '/'); + + if (!name) + return -FDT_ERR_INTERNAL; + + name++; + + /* The FDT function to look at node doesn't take into account the + * unit (i.e anything after @) when search by name. Check if the + * name exactly match. + */ + nodeoff = fdt_path_offset(pfdt, path); + if (nodeoff < 0) + return nodeoff; + + if (strcmp(fdt_get_name(pfdt, nodeoff, NULL), name)) + return -FDT_ERR_NOTFOUND; + + r = copy_node(gc, fdt, pfdt, nodeoff, 0); + if (r) return r; + + return 0; +} + +/* + * The partial device tree is not copied entirely. Only the relevant bits are + * copied to the guest device tree: + * - /passthrough node + * - /aliases node + */ +static int copy_partial_fdt(libxl__gc *gc, void *fdt, void *pfdt) +{ + int r; + + r = copy_node_by_path(gc, "/passthrough", fdt, pfdt); + if (r < 0) { + LOG(ERROR, "Can't copy the node \"/passthrough\" from the partial FDT"); + return r; + } + + r = copy_node_by_path(gc, "/aliases", fdt, pfdt); + if (r < 0 && r != -FDT_ERR_NOTFOUND) { + LOG(ERROR, "Can't copy the node \"/aliases\" from the partial FDT"); + return r; + } + + return 0; +} + #define FDT_MAX_SIZE (1<<20) int libxl__arch_domain_init_hw_description(libxl__gc *gc, @@ -548,8 +780,10 @@ int libxl__arch_domain_init_hw_description(libxl__gc *gc, struct xc_dom_image *dom) { void *fdt = NULL; + void *pfdt = NULL; int rc, res; size_t fdt_size = 0; + int pfdt_size = 0; const libxl_version_info *vers; const struct arch_info *ainfo; @@ -569,6 +803,22 @@ int libxl__arch_domain_init_hw_description(libxl__gc *gc, vers->xen_version_major, vers->xen_version_minor); LOG(DEBUG, " - vGIC version: %s\n", gicv_to_string(xc_config->gic_version)); + if (info->device_tree) { + LOG(DEBUG, " - Partial device tree provided: %s", info->device_tree); + + rc = libxl_read_file_contents(CTX, info->device_tree, + &pfdt, &pfdt_size); + if (rc) { + LOGEV(ERROR, rc, "failed to read the partial device file %s", + info->device_tree); + return ERROR_FAIL; + } + libxl__ptr_add(gc, pfdt); + + if (check_partial_fdt(gc, pfdt, pfdt_size)) + return ERROR_FAIL; + } + /* * Call "call" handling FDT_ERR_*. Will either: * - loop back to retry_resize @@ -635,6 +885,9 @@ next_resize: FDT( make_timer_node(gc, fdt, ainfo) ); FDT( make_hypervisor_node(gc, fdt, vers) ); + if (pfdt) + FDT( copy_partial_fdt(gc, fdt, pfdt) ); + FDT( fdt_end_node(fdt) ); FDT( fdt_finish(fdt) ); diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl index 1214d2e..5651110 100644 --- a/tools/libxl/libxl_types.idl +++ b/tools/libxl/libxl_types.idl @@ -399,6 +399,7 @@ libxl_domain_build_info = Struct("domain_build_info",[ ("kernel", string), ("cmdline", string), ("ramdisk", string), + ("device_tree", string), ("u", KeyedUnion(None, libxl_domain_type, "type", [("hvm", Struct(None, [("firmware", string), ("bios", libxl_bios_type), diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c index 0b02a6c..31e89e8 100644 --- a/tools/libxl/xl_cmdimpl.c +++ b/tools/libxl/xl_cmdimpl.c @@ -1184,6 +1184,7 @@ static void parse_config_data(const char *config_source, xlu_cfg_replace_string (config, "kernel", &b_info->kernel, 0); xlu_cfg_replace_string (config, "ramdisk", &b_info->ramdisk, 0); + xlu_cfg_replace_string (config, "device_tree", &b_info->device_tree, 0); b_info->cmdline = parse_cmdline(config); xlu_cfg_get_defbool(config, "driver_domain", &c_info->driver_domain, 0);
Let the user to pass additional nodes to the guest device tree. For this purpose, everything in the node /passthrough from the partial device tree will be copied into the guest device tree. The node /aliases will be also copied to allow the user to define aliases which can be used by the guest kernel. A simple partial device tree will look like: /dts-v1/; / { #address-cells = <2>; #size-cells = <2>; passthrough { compatible = "simple-bus"; ranges; #address-cells = <2>; #size-cells = <2>; /* List of your nodes */ } }; Note that: * The interrupt-parent proporties will be added by the toolstack in the root node * The properties compatible, ranges, #address-cells and #size-cells in /passthrough are mandatory. Signed-off-by: Julien Grall <julien.grall@linaro.org> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Wei Liu <wei.liu2@citrix.com> --- Changes in v3: - Patch added --- docs/man/xl.cfg.pod.5 | 7 ++ tools/libxl/libxl_arm.c | 253 ++++++++++++++++++++++++++++++++++++++++++++ tools/libxl/libxl_types.idl | 1 + tools/libxl/xl_cmdimpl.c | 1 + 4 files changed, 262 insertions(+)