Re: [Xen-devel] [PATCH v3 21/24] tools/(lib)xl: Add partial device tree support for ARM

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

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).

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

> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> 
> ---
>     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.

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.


> +/*
> + * 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.

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.

> +{
> +    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.
> +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?)

> +/*
> + * 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?

> 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

Ian.


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