Re: [PATCH 08/10] xen: arm: support bootmodule type detection by ordering

[Stefano Stabellini <stefano.stabellini@eu.citrix.com>]

On Mon, 16 Jun 2014, Ian Campbell wrote:
> Assign modules types based on the order in which they are defined in the FDT.
> This is supported only for the dom0 kernel and ramdisk when given as the first
> and second modules respectively, similar to how
> http://wiki.xen.org/wiki?title=Xen_ARM_with_Virtualization_Extensions/Multiboot&oldid=11824
> defined the default types from the bootloader side.
> 
> This is compatible with how Xen interprets the modules with x86 multiboot and I
> think simplifies things for bootloaders which now need not contain similar
> guessing code if they only care about the most basic case.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/bootfdt.c      |   10 +++++++---
>  xen/arch/arm/setup.c        |   14 ++++++++++++++
>  xen/include/asm-arm/setup.h |   11 ++++++++++-
>  3 files changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index e983aa7..a80055c 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -163,6 +163,7 @@ static void __init process_multiboot_node(const void *fdt, int node,
>                                            const char *name,
>                                            u32 address_cells, u32 size_cells)
>  {
> +    static bootmodulekind kind_guess = BOOTMOD_LAST_PRESERVE + 1;

kind_guess = BOOTMOD_KERNEL would be clearer


>      const struct fdt_property *prop;
>      const __be32 *cell;
>      bootmodulekind kind;
> @@ -178,8 +179,10 @@ static void __init process_multiboot_node(const void *fdt, int node,
>          kind = BOOTMOD_RAMDISK;
>      else if ( fdt_node_check_compatible(fdt, node, "xen,xsm-policy") == 0 )
>          kind = BOOTMOD_XSM;
> +    else if ( kind_guess < BOOTMOD_UNKNOWN )
> +        kind = kind_guess++;

This would allow for BOOTMOD_XSM to be specified this way. Do we want
that? If so, we should write to the commit message and to the wiki.
Otherwise I would prefer:

else if ( kind_guess == BOOTMOD_KERNEL || kind_guess == BOOTMOD_RAMDISK )
{
    kind = kind_guess;
    kind_guess++;
}


>      else
> -        panic("%s not a known xen multiboot type\n", name);
> +        kind = BOOTMOD_UNKNOWN;
>  
>      prop = fdt_get_property(fdt, node, "reg", &len);
>      if ( !prop )
> @@ -278,11 +281,12 @@ static void __init early_print_info(void)
>                       mi->bank[i].start,
>                       mi->bank[i].start + mi->bank[i].size - 1);
>      printk("\n");
> -    for ( i = 1 ; i < mods->nr_mods; i++ )
> -        printk("MODULE[%d]: %"PRIpaddr" - %"PRIpaddr" %s\n",
> +    for ( i = 0 ; i < mods->nr_mods; i++ )
> +        printk("MODULE[%d]: %"PRIpaddr" - %"PRIpaddr" %-8s %s\n",
>                       i,
>                       mods->module[i].start,
>                       mods->module[i].start + mods->module[i].size,
> +                     boot_module_kind_as_string(mods->module[i].kind),
>                       mods->module[i].cmdline);
>      nr_rsvd = fdt_num_mem_rsv(device_tree_flattened);
>      for ( i = 0; i < nr_rsvd; i++ )
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 4c9dd3d..d358d04 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -221,6 +221,20 @@ struct bootmodule * __init boot_module_find_by_kind(bootmodulekind kind)
>      return NULL;
>  }
>  
> +const char * __init boot_module_kind_as_string(bootmodulekind kind)
> +{
> +    switch ( kind )
> +    {
> +    case BOOTMOD_XEN:     return "Xen";
> +    case BOOTMOD_FDT:     return "FDT";
> +    case BOOTMOD_KERNEL:  return "Kernel";
> +    case BOOTMOD_RAMDISK: return "Ramdisk";
> +    case BOOTMOD_XSM:     return "XSM";
> +    case BOOTMOD_UNKNOWN: return "Unknown";
> +    default: BUG();
> +    }
> +}
> +
>  void __init discard_initial_modules(void)
>  {
>      struct bootmodules *mi = &bootinfo.modules;
> diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
> index 57c98cb..f1a27fb 100644
> --- a/xen/include/asm-arm/setup.h
> +++ b/xen/include/asm-arm/setup.h
> @@ -10,12 +10,20 @@
>  typedef enum {
>      BOOTMOD_XEN,
>      BOOTMOD_FDT,
> +
>      /* Everything up to here is not freed after start of day */
>      BOOTMOD_LAST_PRESERVE = BOOTMOD_FDT,
> +
> +    /*
> +     * Default module types. For modules which are not given an
> +     * explict type these are automatically used, in this order.
> +     */
>      BOOTMOD_KERNEL,
>      BOOTMOD_RAMDISK,
> +    BOOTMOD_UNKNOWN,
> +
> +    /* The remaining module types are never automatically assigned. */
>      BOOTMOD_XSM,
> -    BOOTMOD_UNKNOWN
>  }  bootmodulekind;
>  
>  
> @@ -66,6 +74,7 @@ const char __init *boot_fdt_cmdline(const void *fdt);
>  void add_boot_module(bootmodulekind kind, paddr_t start, paddr_t size,
>                       const char *cmdline);
>  struct bootmodule *boot_module_find_by_kind(bootmodulekind kind);
> +const char * __init boot_module_kind_as_string(bootmodulekind kind);
>  
>  #endif
>  /*
> -- 
> 1.7.10.4
>