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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Wed, 18 Jun 2014, Ian Campbell wrote:
> 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@xxxxxxxxxx>
> > > ---
> > >  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)

I didn't spot that you moved BOOTMOD_UNKNOWN before BOOTMOD_XSM in this
patch. I would rather avoid it for the principle of least astonishment.
I would expect the "unknown" type to be the last one in the enum.


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

The point is trying to avoid relying on the integer ordering in the
enum, otherwise we loose the benefits of using an enum instead of the
old macros.


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

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