Re: [Xen-devel] [RFC 16/24] xen/arm: Build DOM0 FDT by browsing the device tree structure

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
> Remove the usage of the FDT in benefit of the device tree structure.

"in favour of" is what I think you mean.

> The latter is easier to use and can embedded meta-data for Xen (ie: is the
> device is used by Xen...).
> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
> ---
>  xen/arch/arm/domain_build.c |  270 
> ++++++++++++++++---------------------------
>  1 file changed, 101 insertions(+), 169 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 604ec1c..c8f24ed 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -63,10 +63,10 @@ struct vcpu *__init alloc_dom0_vcpu0(void)
>  }
>  
>  static int set_memory_reg_11(struct domain *d, struct kernel_info *kinfo,
> -                             const void *fdt, const u32 *cell, int len,
> -                             int address_cells, int size_cells, u32 
> *new_cell)
> +                             const struct dt_property *pp,
> +                             const struct dt_device_node *np, __be32 
> *new_cell)
> {
> -    int reg_size = (address_cells + size_cells) * sizeof(*cell);
> +    int reg_size = dt_cells_to_size(dt_n_addr_cells(np) + 
> dt_n_size_cells(np));
>      paddr_t start;
>      paddr_t size;
>      struct page_info *pg;
> @@ -90,7 +90,7 @@ static int set_memory_reg_11(struct domain *d, struct 
> kernel_info *kinfo,
>      if ( res )
>          panic("Unable to add pages in DOM0: %d\n", res);
>  
> -    device_tree_set_reg(&new_cell, address_cells, size_cells, start, size);
> +    dt_set_range(&new_cell, np, start, size);
>  
>      kinfo->mem.bank[0].start = start;
>      kinfo->mem.bank[0].size = size;
> @@ -100,25 +100,30 @@ static int set_memory_reg_11(struct domain *d, struct 
> kernel_info *kinfo,
>  }
>  
>  static int set_memory_reg(struct domain *d, struct kernel_info *kinfo,
> -                          const void *fdt, const u32 *cell, int len,
> -                          int address_cells, int size_cells, u32 *new_cell)
> +                          const struct dt_property *pp,
> +                          const struct dt_device_node *np, __be32 *new_cell)
>  {
> -    int reg_size = (address_cells + size_cells) * sizeof(*cell);
> +    int reg_size = dt_cells_to_size(dt_n_addr_cells(np) + 
> dt_n_size_cells(np));
>      int l = 0;
> +    unsigned int bank = 0;
>      u64 start;
>      u64 size;
> +    int ret;
>  
>      if ( platform_has_quirk(PLATFORM_QUIRK_DOM0_MAPPING_11) )
> -        return set_memory_reg_11(d, kinfo, fdt, cell, len, address_cells,
> -                                 size_cells, new_cell);
> +        return set_memory_reg_11(d, kinfo, pp, np, new_cell);
>  
> -    while ( kinfo->unassigned_mem > 0 && l + reg_size <= len
> +    while ( kinfo->unassigned_mem > 0 && l + reg_size <= pp->length
>              && kinfo->mem.nr_banks < NR_MEM_BANKS )
>      {
> -        device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
> +        ret = dt_device_get_address(np, bank, &start, &size);
> +        if ( ret )
> +            panic("Unable to retrieve the bank %u for %s\n",

Dropping "the" sounds more natural to me. Perhaps say "memory bank" too?
 
> -static void make_hypervisor_node(void *fdt, int addrcells, int sizecells)
> +static int make_hypervisor_node(void *fdt, const struct dt_device_node 
> *parent)
>  {
>      const char compat[] =
>          "xen,xen-"__stringify(XEN_VERSION)"."__stringify(XEN_SUBVERSION)"\0"
>          "xen,xen";
> -    u32 reg[4];
> -    u32 intr[3];
> -    u32 *cell;
> +    __be32 reg[4];
> +    __be32 intr[3];
> +    __be32 *cells;
> +    int res;
> +    int addrcells = dt_n_addr_cells(parent);
> +    int sizecells = dt_n_size_cells(parent);
> +
> +    DPRINT("Create hypervisor node\n");

Not sure there is any point in this print unless you also add DPRINT of
the things we put into it.
>  
>      /*
>       * Sanity-check address sizes, since addresses and sizes which do
> [...]

> izecells));
> +    cells = &reg[0];
> +    dt_set_cell(&cells, addrcells, 0xb0000000);
> +    dt_set_cell(&cells, sizecells, 0x20000);

Aside: this really ought to become dynamic, based on finding a hole in
the physical address map...

[...]
> +    res = fdt_property(fdt, "interrupts", intr, sizeof(intr[0]) * 3);
> +    if ( res )
> +        return res;

the * 3 come from the interrupt-controller nodes properties I think?
Should we assert somewhere that they match? Perhaps we would already die
if it weren't anyway?

> @@ -454,7 +374,8 @@ static int handle_node(struct domain *d, const struct 
> dt_device_node *np)
>      if ( dt_match_node(skip_matches, np ) )
>          return 0;
>  
> -    if ( dt_device_used_by(np) != DOMID_XEN )
> +    if ( dt_device_used_by(np) != DOMID_XEN &&
> +         !dt_device_type_is_equal(np, "memory") )

Can we get a comment about why memory is special here please?

>      {
>          res = map_device(d, np);
>  

Ian.


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