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

[Julien Grall <julien.grall@linaro.org>]

On 08/22/2013 02:49 PM, Ian Campbell wrote:
> 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@linaro.org>
>> ---
>>  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?

I will fix it.

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

I will add more print. I think it can be usefull for debugging the
device tree.

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

Is this address used somewhere in Xen? I didn't find any place.

> 
> [...]
>> +    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?

The GIC node always has #interrupt-size equals to 3. I don't think an
assert is necessary.

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

I will add it.

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


-- 
Julien Grall