From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v3 21/24] tools/(lib)xl: Add partial device tree support for ARM Date: Mon, 23 Feb 2015 17:06:11 +0000 Message-ID: <54EB5E03.2010800@linaro.org> References: <1421159133-31526-1-git-send-email-julien.grall@linaro.org> <1421159133-31526-22-git-send-email-julien.grall@linaro.org> <1424691989.27930.41.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YPwT9-00031Y-Gr for xen-devel@lists.xenproject.org; Mon, 23 Feb 2015 17:06:55 +0000 Received: by mail-wi0-f177.google.com with SMTP id bs8so19130693wib.4 for ; Mon, 23 Feb 2015 09:06:38 -0800 (PST) In-Reply-To: <1424691989.27930.41.camel@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: xen-devel@lists.xenproject.org, tim@xen.org, Ian Jackson , stefano.stabellini@citrix.com, Wei Liu List-Id: xen-devel@lists.xenproject.org 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 = > 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 >> Cc: Ian Jackson >> Cc: Wei Liu >> >> --- >> 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 >> + >> +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, -- Julien Grall