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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2012-04-02 at 10:21 +0100, David Vrabel wrote:
> 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@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.
> 
> No.  address_cells and size_cells can be 1 or 2.

Where is that checked or enforced? What happens if someone feeds us a
DTB with 3? Or are you saying that this isn't possible (perhaps due to
some constraint in DTB itself)?

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

OK.

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

I was suggesting that it would be fine to just add a new error code,
IMHO it's a reasonable reason to diverge from upstream. Could perhaps
even send them the patch?

Ian.


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