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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2014-06-16 at 13:48 +0100, Julien Grall wrote:
> 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@xxxxxxxxxx>
> > ---
> >  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.

Ah, I was relying on the compiler to tell me about all uses of MOD_* but
I didn't have XSM enabled so it didn't build this file. Will fix.

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

OK.

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

Agreed, this was an unintentional change.

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

OK.



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