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

[Julien Grall <julien.grall@linaro.org>]

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,

-- 
Julien Grall