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

[Ian Campbell <ian.campbell@citrix.com>]

On Tue, 2015-03-31 at 13:55 +0100, Julien Grall wrote:
> 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@linaro.org>
> >> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> >> Cc: Wei Liu <wei.liu2@citrix.com>
> >>
> >> ---
> >>     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:

Oops, I couldn't remember v3 so I didn't read the changelog and missed
this, sorry.

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

I think it should be in both, libxl/IDL for developers and xl docs for
users.

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

OK.

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