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

[Julien Grall <julien.grall@linaro.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 = <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,

-- 
Julien Grall