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

[Ian Campbell <Ian.Campbell@citrix.com>]

On Wed, 2014-06-18 at 15:40 +0100, Stefano Stabellini wrote:
> 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

Yeah.

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

No, explicitly not. And we don't I think because BOOTMOD_XSM >=
BOOTMOD_UNKNOWN (for exactly this reason, see comment in the enum defn)

>  If so, we should write to the commit message and to the wiki.
> Otherwise I would prefer:

Really?

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

Ian