diff mbox

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

Message ID 1426793399-6283-30-git-send-email-julien.grall@linaro.org
State New
Headers show

Commit Message

Julien Grall March 19, 2015, 7:29 p.m. UTC
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 +
 4 files changed, 183 insertions(+)

Comments

Julien Grall March 31, 2015, 12:55 p.m. UTC | #1
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:

"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,
diff mbox

Patch

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.
+
 =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
+
+static int check_partial_fdt(libxl__gc *gc, void *fdt, size_t size)
+{
+    int r;
+
+    if (size < FDT_V17_SIZE) {
+        LOG(ERROR, "Partial FDT is too small");
+        return ERROR_FAIL;
+    }
+
+    if (fdt_magic(fdt) != FDT_MAGIC) {
+        LOG(ERROR, "Partial FDT is not a valid Flat Device Tree");
+        return ERROR_FAIL;
+    }
+
+    if (fdt_version(fdt) != FDT_REQUIRED_VERSION) {
+        LOG(ERROR, "Partial FDT version not supported. Required 0x%x got 0x%x",
+            FDT_REQUIRED_VERSION, fdt_version(fdt));
+        return ERROR_FAIL;
+    }
+
+    r = fdt_check_header(fdt);
+    if (r) {
+        LOG(ERROR, "Failed to check the partial FDT (%d)", r);
+        return ERROR_FAIL;
+    }
+
+    if (fdt_totalsize(fdt) > size) {
+        LOG(ERROR, "Partial FDT totalsize is too big");
+        return ERROR_FAIL;
+    }
+
+    return 0;
+}
+
+static int copy_properties(libxl__gc *gc, void *fdt, void *pfdt,
+                           int nodeoff)
+{
+    int propoff, nameoff, r;
+    const struct fdt_property *prop;
+
+    for (propoff = fdt_first_property_offset(pfdt, nodeoff);
+         propoff >= 0;
+         propoff = fdt_next_property_offset(pfdt, propoff)) {
+
+        if (!(prop = fdt_get_property_by_offset(pfdt, propoff, NULL))) {
+            return -FDT_ERR_INTERNAL;
+        }
+
+        nameoff = fdt32_to_cpu(prop->nameoff);
+        r = fdt_property(fdt, fdt_string(pfdt, nameoff),
+                         prop->data, fdt32_to_cpu(prop->len));
+        if (r) return r;
+    }
+
+    /* FDT_ERR_NOTFOUND => There is no more properties for this node */
+    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);
+
+/* Copy a node from the partial device tree to the guest device tree */
+static int copy_node(libxl__gc *gc, void *fdt, void *pfdt,
+                     int nodeoff, int depth)
+{
+    int r;
+
+    r = fdt_begin_node(fdt, fdt_get_name(pfdt, nodeoff, NULL));
+    if (r) return r;
+
+    r = copy_properties(gc, fdt, pfdt, nodeoff);
+    if (r) return r;
+
+    for (nodeoff = fdt_first_subnode(pfdt, nodeoff);
+         nodeoff >= 0;
+         nodeoff = fdt_next_subnode(pfdt, nodeoff)) {
+        r = copy_node(gc, fdt, pfdt, nodeoff, depth + 1);
+        if (r) return r;
+    }
+
+    if (nodeoff != -FDT_ERR_NOTFOUND)
+        return nodeoff;
+
+    r = fdt_end_node(fdt);
+    if (r) return r;
+
+    return 0;
+}
+
+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 a node doesn't take into account the
+     * unit (i.e anything after @) when search by name. Check if the
+     * name exactly matches.
+     */
+    nodeoff = fdt_path_offset(pfdt, path);
+    if (nodeoff < 0)
+        return nodeoff;
+
+    if (strcmp(fdt_get_name(pfdt, nodeoff, NULL), name))
+        return -FDT_ERR_NOTFOUND;
+
+    r = copy_node(gc, fdt, pfdt, nodeoff, 0);
+    if (r) return r;
+
+    return 0;
+}
+
+/*
+ * The partial device tree is not copied entirely. Only the relevant bits are
+ * copied to the guest device tree:
+ *  - /passthrough node
+ *  - /aliases node
+ */
+static int copy_partial_fdt(libxl__gc *gc, void *fdt, void *pfdt)
+{
+    int r;
+
+    r = copy_node_by_path(gc, "/passthrough", fdt, pfdt);
+    if (r < 0) {
+        LOG(ERROR, "Can't copy the node \"/passthrough\" from the partial FDT");
+        return r;
+    }
+
+    r = copy_node_by_path(gc, "/aliases", fdt, pfdt);
+    if (r < 0 && r != -FDT_ERR_NOTFOUND) {
+        LOG(ERROR, "Can't copy the node \"/aliases\" from the partial FDT");
+        return r;
+    }
+
+    return 0;
+}
+
 #define FDT_MAX_SIZE (1<<20)
 
 int libxl__arch_domain_init_hw_description(libxl__gc *gc,
@@ -550,8 +700,10 @@  int libxl__arch_domain_init_hw_description(libxl__gc *gc,
                                            struct xc_dom_image *dom)
 {
     void *fdt = NULL;
+    void *pfdt = NULL;
     int rc, res;
     size_t fdt_size = 0;
+    int pfdt_size = 0;
 
     const libxl_version_info *vers;
     const struct arch_info *ainfo;
@@ -571,6 +723,22 @@  int libxl__arch_domain_init_hw_description(libxl__gc *gc,
         vers->xen_version_major, vers->xen_version_minor);
     LOG(DEBUG, " - vGIC version: %s\n", gicv_to_string(xc_config->gic_version));
 
+    if (info->device_tree) {
+        LOG(DEBUG, " - Partial device tree provided: %s", info->device_tree);
+
+        rc = libxl_read_file_contents(CTX, info->device_tree,
+                                      &pfdt, &pfdt_size);
+        if (rc) {
+            LOGEV(ERROR, rc, "failed to read the partial device file %s",
+                  info->device_tree);
+            return ERROR_FAIL;
+        }
+        libxl__ptr_add(gc, pfdt);
+
+        if (check_partial_fdt(gc, pfdt, pfdt_size))
+            return ERROR_FAIL;
+    }
+
 /*
  * Call "call" handling FDT_ERR_*. Will either:
  * - loop back to retry_resize
@@ -637,6 +805,9 @@  next_resize:
         FDT( make_timer_node(gc, fdt, ainfo) );
         FDT( make_hypervisor_node(gc, fdt, vers) );
 
+        if (pfdt)
+            FDT( copy_partial_fdt(gc, fdt, pfdt) );
+
         FDT( fdt_end_node(fdt) );
 
         FDT( fdt_finish(fdt) );
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 0866433..c97e6ed 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -411,6 +411,7 @@  libxl_domain_build_info = Struct("domain_build_info",[
     ("kernel",           string),
     ("cmdline",          string),
     ("ramdisk",          string),
+    ("device_tree",      string),
     ("u", KeyedUnion(None, libxl_domain_type, "type",
                 [("hvm", Struct(None, [("firmware",         string),
                                        ("bios",             libxl_bios_type),
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 394b55d..c2415ba 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1379,6 +1379,7 @@  static void parse_config_data(const char *config_source,
 
     xlu_cfg_replace_string (config, "kernel", &b_info->kernel, 0);
     xlu_cfg_replace_string (config, "ramdisk", &b_info->ramdisk, 0);
+    xlu_cfg_replace_string (config, "device_tree", &b_info->device_tree, 0);
     b_info->cmdline = parse_cmdline(config);
 
     xlu_cfg_get_defbool(config, "driver_domain", &c_info->driver_domain, 0);