Re: [PATCH 07/10] xen: arm: store per-boot module type instead of relying on index

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

Hi Ian,

On 06/16/2014 12:45 PM, Ian Campbell wrote:
> This is more natural and better matches how multiboot is actually supposed to
> work.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/bootfdt.c      |   49 +++++++++++++++----------------------------
>  xen/arch/arm/domain_build.c |   20 +++++++++++-------
>  xen/arch/arm/kernel.c       |   15 ++++++-------
>  xen/arch/arm/setup.c        |   47 ++++++++++++++++++++++++++++++++++++-----
>  xen/include/asm-arm/setup.h |   29 +++++++++++++++++--------
>  5 files changed, 100 insertions(+), 60 deletions(-)

It looks like you forgot to modify xsm/xsm_policy.c.

> +void add_boot_module(bootmodulekind kind, paddr_t start, paddr_t size,
> +                     const char *cmdline)
> +{
> +    struct bootmodules *mods = &bootinfo.modules;
> +    struct bootmodule *mod;
> +
> +    if ( mods->nr_mods == MAX_MODULES )
> +    {
> +        printk("Ignoring boot module at %"PRIpaddr"-%"PRIpaddr" (too many)\n",
> +               start, start + size);
> +        return;
> +    }

As we don't have any way to know if the user add multiple the kernel
and/or the initramfs, I would add a print message here to say what kind
of boot module we are currently adding. It will help the guy to find the
problem quickly.

[..]

>  void __init discard_initial_modules(void)
>  {
>      struct bootmodules *mi = &bootinfo.modules;
>      int i;
>  
> -    for ( i = MOD_DISCARD_FIRST; i <= mi->nr_mods; i++ )
> +    for ( i = 0; i <= mi->nr_mods; i++ )
>      {
>          paddr_t s = mi->module[i].start;
>          paddr_t e = s + PAGE_ALIGN(mi->module[i].size);
>  
> -        dt_unreserved_regions(s, e, init_domheap_pages, 0);
> +        if ( mi->module[i].kind > BOOTMOD_LAST_PRESERVE )
> +            dt_unreserved_regions(s, e, init_domheap_pages, 0);
>      }

[..]


> +typedef enum {
> +    BOOTMOD_XEN,
> +    BOOTMOD_FDT,
> +    /* Everything up to here is not freed after start of day */
> +    BOOTMOD_LAST_PRESERVE = BOOTMOD_FDT,

Currently we also discard the FDT, this is because the FDT is copied
another place in the memory. With this patch the FDT module stays in memory.

I think BOOTMOD_LAST_PRESERVE should be equal to BOOTMOD_XEN.

> +    BOOTMOD_KERNEL,
> +    BOOTMOD_RAMDISK,
> +    BOOTMOD_XSM,
> +    BOOTMOD_UNKNOWN
> +}  bootmodulekind;

I would rename to bootmodule_kind. It's easier to read and you know that
the enum is used for a bootmodule.

Regards,

-- 
Julien Grall