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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

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.

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

> +
>  =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?)

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

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


Ian.


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