[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 2/5] device tree, arm: supply a flat device tree to dom0



On Thu, 2012-03-22 at 19:17 +0000, David Vrabel wrote:
> From: David Vrabel <david.vrabel@xxxxxxxxxx>
> 
> Build a flat device tree for dom0 based on the one supplied to Xen.
> The following changes are made:
> 
>   * In the /chosen node, the xen,dom0-bootargs parameter is renamed to
>     bootargs.
> 
>   * In all memory nodes, the reg parameters are adjusted to reflect
>     the amount of memory dom0 can use.  The p2m is updated using this
>     info.
> 
> Support for passing ATAGS to dom0 is removed.
> 
> Signed-off-by: David Vrabel <david.vrabel@xxxxxxxxxx>
> ---
>  xen/arch/arm/domain_build.c         |  257 
> +++++++++++++++++++++++++++++------
>  xen/arch/arm/kernel.c               |    2 +-
>  xen/arch/arm/kernel.h               |    8 +-
>  xen/common/device_tree.c            |   47 ++++--
>  xen/include/xen/device_tree.h       |    8 +
>  xen/include/xen/libfdt/libfdt_env.h |    3 +
>  6 files changed, 265 insertions(+), 60 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 15632f7..b4c0452 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -6,6 +6,10 @@
>  #include <xen/sched.h>
>  #include <asm/irq.h>
>  #include <asm/regs.h>
> +#include <xen/errno.h>
> +#include <xen/device_tree.h>
> +#include <xen/libfdt/libfdt.h>
> +#include <xen/guest_access.h>
> 
>  #include "gic.h"
>  #include "kernel.h"
> @@ -13,6 +17,13 @@
>  static unsigned int __initdata opt_dom0_max_vcpus;
>  integer_param("dom0_max_vcpus", opt_dom0_max_vcpus);
> 
> +/*
> + * Amount of extra space required to dom0's device tree.  No new nodes
> + * are added (yet) but one terminating reserve map entry (16 bytes) is
> + * added.
> + */
> +#define DOM0_FDT_EXTRA_SIZE (sizeof(struct fdt_reserve_entry))
> +
>  struct vcpu *__init alloc_dom0_vcpu0(void)
>  {
>      if ( opt_dom0_max_vcpus == 0 )
> @@ -28,43 +39,210 @@ struct vcpu *__init alloc_dom0_vcpu0(void)
>      return alloc_vcpu(dom0, 0, 0);
>  }
> 
> -extern void guest_mode_entry(void);
> +static void set_memory_reg(struct domain *d, struct kernel_info *kinfo,
> +                           const void *fdt,
> +                           const u32 *cell, int address_cells, int 
> size_cells,
> +                           u32 *new_cell, int *len)
> +{
> +    int reg_size = (address_cells + size_cells) * sizeof(*cell);
> +    int l;
> +    u64 start;
> +    u64 size;
> +
> +    l = *len;

> +
> +    while ( kinfo->unassigned_mem > 0 && l >= reg_size
> +            && kinfo->mem.nr_banks < NR_MEM_BANKS )
> +    {
> +        device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
> +        if ( size > kinfo->unassigned_mem )
> +            size = kinfo->unassigned_mem;
> +
> +        device_tree_set_reg(&new_cell, address_cells, size_cells, start, 
> size);

This assumes/requires that address_cells and size_cells a 1, right? If
they end up being 2 or more then device_tree_get_reg will trample on the
stack via &start and &size.

> +
> +        printk("Populate P2M %#llx->%#llx\n", start, start + size);
> +        p2m_populate_ram(d, start, start + size);
> +        kinfo->mem.bank[kinfo->mem.nr_banks].start = start;
> +        kinfo->mem.bank[kinfo->mem.nr_banks].size = size;
> +        kinfo->mem.nr_banks++;
> +        kinfo->unassigned_mem -= size;
> +
> +        l -= reg_size;
> +    }
> +
> +    *len -= l;

I had a bit of trouble following the logic wrt l and the updating of
*len in this function.

AIUI *len is initially the size of the original cell. The loop is
looping over the length of the cell, using l as the iterator, and
duplicating each entry into the new cell, while also populating the p2m
and the bank list as it goes. So far so good.

But then *len is updated by subtracting whatever l remained of the
original cells length. It seems that the caller is using it as the
length of the new cell, and the maths does seem to work out that way but
it seems rather convoluted compared with e.g. just returning the length
of the new cell.

> +}
> +
[...]
> +static void set_val(u32 **cell, u32 cells, u64 val)
> +{
> +    u32 c = cells;
> +
> +    while ( c-- )
> +    {
> +        (*cell)[c] = cpu_to_fdt32(val);
> +        val >>= 32;

I guess cells is never more than 2 then?

> +    }
> +    (*cell) += cells;
> +}
> +
> +void device_tree_set_reg(u32 **cell, u32 address_cells, u32 size_cells,
> +                         u64 start, u64 size)
> +{
> +    set_val(cell, address_cells, start);
> +    set_val(cell, size_cells, size);
> +}
> +
> +u32 device_tree_get_u32(const void *fdt, int node, const char *prop_name)
>  {
>      const struct fdt_property *prop;
> 
> @@ -68,8 +85,6 @@ static u32 __init prop_by_name_u32(const void *fdt, int 
> node,
>      return fdt32_to_cpu(*(uint32_t*)prop->data);
>  }
> 
> -#define MAX_DEPTH 16
> -
>  /**
>   * device_tree_for_each_node - iterate over all device tree nodes
>   * @fdt: flat device tree.
> @@ -81,19 +96,19 @@ int device_tree_for_each_node(const void *fdt,
>  {
>      int node;
>      int depth;
> -    u32 address_cells[MAX_DEPTH];
> -    u32 size_cells[MAX_DEPTH];
> +    u32 address_cells[DEVICE_TREE_MAX_DEPTH];
> +    u32 size_cells[DEVICE_TREE_MAX_DEPTH];
>      int ret;
> 
>      for ( node = 0, depth = 0;
>            node >=0 && depth >= 0;
>            node = fdt_next_node(fdt, node, &depth) )
>      {
> -        if ( depth >= MAX_DEPTH )
> +        if ( depth >= DEVICE_TREE_MAX_DEPTH )
>              continue;
> 
> -        address_cells[depth] = prop_by_name_u32(fdt, node, "#address-cells");
> -        size_cells[depth] = prop_by_name_u32(fdt, node, "#size-cells");
> +        address_cells[depth] = device_tree_get_u32(fdt, node, 
> "#address-cells");
> +        size_cells[depth] = device_tree_get_u32(fdt, node, "#size-cells");
> 
>          ret = func(fdt, node, fdt_get_name(fdt, node, NULL), depth,
>                     address_cells[depth-1], size_cells[depth-1], data);
> @@ -106,7 +121,7 @@ int device_tree_for_each_node(const void *fdt,
>  static int dump_node(const void *fdt, int node, const char *name, int depth,
>                       u32 address_cells, u32 size_cells, void *data)
>  {
> -    char prefix[2*MAX_DEPTH + 1] = "";
> +    char prefix[2*DEVICE_TREE_MAX_DEPTH + 1] = "";
>      int i;
>      int prop;
> 
> @@ -172,7 +187,7 @@ static void __init process_memory_node(const void *fdt, 
> int node,
> 
>      for ( i = 0; i < banks && early_info.mem.nr_banks < NR_MEM_BANKS; i++ )
>      {
> -        get_register(&cell, address_cells, size_cells, &start, &size);
> +        device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
>          early_info.mem.bank[early_info.mem.nr_banks].start = start;
>          early_info.mem.bank[early_info.mem.nr_banks].size = size;
>          early_info.mem.nr_banks++;
> @@ -184,7 +199,7 @@ static int __init early_scan_node(const void *fdt,
>                                    u32 address_cells, u32 size_cells,
>                                    void *data)
>  {
> -    if ( node_matches(fdt, node, "memory") )
> +    if ( device_tree_node_matches(fdt, node, "memory") )
>          process_memory_node(fdt, node, name, address_cells, size_cells);
> 
>      return 0;
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index b91b39f..510b5b4 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -12,6 +12,8 @@
> 
>  #include <xen/types.h>
> 
> +#define DEVICE_TREE_MAX_DEPTH 16
> +
>  #define NR_MEM_BANKS 8
> 
>  struct membank {
> @@ -39,6 +41,12 @@ extern void *device_tree_flattened;
>  size_t device_tree_early_init(const void *fdt);
>  paddr_t device_tree_get_xen_paddr(void);
> 
> +void device_tree_get_reg(const u32 **cell, u32 address_cells, u32 size_cells,
> +                         u64 *start, u64 *size);
> +void device_tree_set_reg(u32 **cell, u32 address_cells, u32 size_cells,
> +                         u64 start, u64 size);
> +u32 device_tree_get_u32(const void *fdt, int node, const char *prop_name);
> +bool_t device_tree_node_matches(const void *fdt, int node, const char 
> *match);
>  int device_tree_for_each_node(const void *fdt,
>                                device_tree_node_func func, void *data);
>  void device_tree_dump(const void *fdt);
> diff --git a/xen/include/xen/libfdt/libfdt_env.h 
> b/xen/include/xen/libfdt/libfdt_env.h
> index 8c0c030..2f1b03c 100644
> --- a/xen/include/xen/libfdt/libfdt_env.h
> +++ b/xen/include/xen/libfdt/libfdt_env.h
> @@ -13,4 +13,7 @@
>  #define fdt64_to_cpu(x) be64_to_cpu(x)
>  #define cpu_to_fdt64(x) cpu_to_be64(x)
> 
> +/* Xen-specific libfdt error code. */
> +#define FDT_ERR_XEN(err) (FDT_ERR_MAX + (err))

Looks like the only user is FDT_ERR_XEN(ENOMEM) which could as well be
FDT_ERR_NOSPACE?

FWIW I think adding a new error code would be a fair reason to diverge
in a controlled way from the pristine upstream source.

Ian.



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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.