From mboxrd@z Thu Jan  1 00:00:00 1970
From: Julien Grall <julien.grall@linaro.org>
Subject: Re: [PATCH 07/10] xen: arm: store per-boot module type
 instead of relying on index
Date: Mon, 16 Jun 2014 13:48:04 +0100
Message-ID: <539EE784.40803@linaro.org>
References: <1402919079.14907.22.camel@kazak.uk.xensource.com>
	<1402919103-29642-7-git-send-email-ian.campbell@citrix.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1402919103-29642-7-git-send-email-ian.campbell@citrix.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Ian Campbell <ian.campbell@citrix.com>, xen-devel@lists.xen.org
Cc: Roy Franz <roy.franz@linaro.org>, Naresh Bhat <naresh.bhat@linaro.org>, tim@xen.org, Fu Wei <fu.wei@linaro.org>, stefano.stabellini@eu.citrix.com
List-Id: xen-devel@lists.xenproject.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