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

[David Vrabel <david.vrabel@citrix.com>]

On 30/03/12 16:59, Ian Campbell wrote:
> On Thu, 2012-03-22 at 19:17 +0000, David Vrabel wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> 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@citrix.com>
>> ---
>>  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.

No.  address_cells and size_cells can be 1 or 2.  This is enough for 64
addresses.

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

I've updated this as per your suggestion.

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

No.  FDT_ERR_NOSPACE means the buffer isn't large enough to add new
nodes etc.  This needs to be a different error code from a memory
allocation failure.

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

I don't know what you're asking for here.

David