Re: [Xen-devel] [PATCH v4 29/33] tools/(lib)xl: Add partial device tree support for ARM

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Ian,

On 31/03/15 12:41, Ian Campbell wrote:
> On Thu, 2015-03-19 at 19:29 +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>;
>>
>>         passthrough {
>>             compatible = "simple-bus";
>>             ranges;
>>             #address-cells = <2>;
>>             #size-cells = <2>;
>>
>>             /* List of your nodes */
>>         }
>> };
>>
>> Note that:
>>     * The interrupt-parent property will be added by the toolstack in
>>     the root node
>>     * The properties compatible, ranges, #address-cells and #size-cells
>>     in /passthrough are mandatory.
>>
>> The helpers provided by the libfdt don't perform all the necessary
>> security check on a given device tree. Therefore, only trusted device
>> tree should be used.
>>
>> Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
>> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
>> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
>>
>> ---
>>     An example of the partial device tree, as long as how to passthrough
>>     a non-pci device will be added to the tree in a follow-up patch.
>>
>>     A new LIBXL_HAVE_* will be added in the patch which add support for
>>     non-PCI passthrough as both are tight.
>>
>>     Changes in v4:
>>         - Mark the option as unsafe
>>         - The _fdt_* helpers has been moved in a separate patch/file.
>>         Only the prototype is declared
>>         - The partial DT is considered valid. Remove some security check
>>         which make the code cleaner
>>         - Typoes
>>
>>     Changes in v3:
>>         - Patch added
>> ---
>>  docs/man/xl.cfg.pod.5       |  10 +++
>>  tools/libxl/libxl_arm.c     | 171 
>> ++++++++++++++++++++++++++++++++++++++++++++
>>  tools/libxl/libxl_types.idl |   1 +
>>  tools/libxl/xl_cmdimpl.c    |   1 +
> 
> Needs a #define LIBXL_HAVE in libxl.h for 3rd party users of the
> library.

As said below the commit message:

"A new LIBXL_HAVE_* will be added in the patch which add support for
non-PCI passthrough as both are tight."

Having the define in the patch #31.

>>  4 files changed, 183 insertions(+)
>>
>> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
>> index 93cd7d2..bcbc277 100644
>> --- a/docs/man/xl.cfg.pod.5
>> +++ b/docs/man/xl.cfg.pod.5
>> @@ -453,6 +453,16 @@ 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.
>> +
>> +Given the complexity of verifying the validity of a device tree, this
>> +option should only be used with trusted device tree.
> 
> This warning should be in the libxl API docs (i.e. the header or IDL)
> somewhere too, so that 3rd party toolstacks know about it. In that
> context it should perhaps be a lot more prominent and scarier sounding
> too.

I will add it to the IDL. Should I keep there too for the xl documentation?

>> +
>>  =back
>>  
>>  =head2 Devices
>> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
>> index 06e940b..54d197b 100644
>> --- a/tools/libxl/libxl_arm.c
>> +++ b/tools/libxl/libxl_arm.c
>> @@ -542,6 +542,156 @@ out:
>>      }
>>  }
>>  
>> +/* Only FDT v17 is supported */
>> +#define FDT_REQUIRED_VERSION    0x11
> 
> Are versions >v17 broken? Or is there some other problem with being more
> forward compatible?
> (TBH, I've no idea how often this value changes, maybe it's unlikely to
> now?)

It was necessary on the previous version of this patch because I was
doing some security check specific to the v17.

I think this check is useless now. So I will drop it.

> 
>> +    return (propoff != -FDT_ERR_NOTFOUND)? propoff : 0;
>> +}
>> +
>> +/*
>> + * These functions are defined by libfdt or libxl_fdt.c if it's not
>> + * present on the former.
>> + */
>> +int fdt_next_subnode(const void *fdt, int offset);
>> +int fdt_first_subnode(const void *fdt, int offset);
> 
> Better to use the prototype from the libfdt header if it is present,
> i.e. wrap these in the associated ifdef-s.

The prototype may be defined in the libfdt header but the function not
exported.

I didn't wrap them into ifdef in order to make sure that the compiler
will complain if the 2 prototypes are different.

> 
> I'm in two minds about suggesting putting all that into
> libxl_libfdt_compat.h, up to you.

I didn't want to bother adding a new header file.

Regards,

-- 
Julien Grall

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