| Message ID | 1402919103-29642-8-git-send-email-ian.campbell@citrix.com |
|---|---|
| State | New |
| Headers | show |
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 > 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? If so, we should write to the commit message and to the wiki. Otherwise I would prefer: else if ( kind_guess == BOOTMOD_KERNEL || kind_guess == BOOTMOD_RAMDISK ) { kind = kind_guess; kind_guess++; } > else > - panic("%s not a known xen multiboot type\n", name); > + kind = BOOTMOD_UNKNOWN; > > prop = fdt_get_property(fdt, node, "reg", &len); > if ( !prop ) > @@ -278,11 +281,12 @@ static void __init early_print_info(void) > mi->bank[i].start, > mi->bank[i].start + mi->bank[i].size - 1); > printk("\n"); > - for ( i = 1 ; i < mods->nr_mods; i++ ) > - printk("MODULE[%d]: %"PRIpaddr" - %"PRIpaddr" %s\n", > + for ( i = 0 ; i < mods->nr_mods; i++ ) > + printk("MODULE[%d]: %"PRIpaddr" - %"PRIpaddr" %-8s %s\n", > i, > mods->module[i].start, > mods->module[i].start + mods->module[i].size, > + boot_module_kind_as_string(mods->module[i].kind), > mods->module[i].cmdline); > nr_rsvd = fdt_num_mem_rsv(device_tree_flattened); > for ( i = 0; i < nr_rsvd; i++ ) > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index 4c9dd3d..d358d04 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -221,6 +221,20 @@ struct bootmodule * __init boot_module_find_by_kind(bootmodulekind kind) > return NULL; > } > > +const char * __init boot_module_kind_as_string(bootmodulekind kind) > +{ > + switch ( kind ) > + { > + case BOOTMOD_XEN: return "Xen"; > + case BOOTMOD_FDT: return "FDT"; > + case BOOTMOD_KERNEL: return "Kernel"; > + case BOOTMOD_RAMDISK: return "Ramdisk"; > + case BOOTMOD_XSM: return "XSM"; > + case BOOTMOD_UNKNOWN: return "Unknown"; > + default: BUG(); > + } > +} > + > void __init discard_initial_modules(void) > { > struct bootmodules *mi = &bootinfo.modules; > diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h > index 57c98cb..f1a27fb 100644 > --- a/xen/include/asm-arm/setup.h > +++ b/xen/include/asm-arm/setup.h > @@ -10,12 +10,20 @@ > typedef enum { > BOOTMOD_XEN, > BOOTMOD_FDT, > + > /* Everything up to here is not freed after start of day */ > BOOTMOD_LAST_PRESERVE = BOOTMOD_FDT, > + > + /* > + * Default module types. For modules which are not given an > + * explict type these are automatically used, in this order. > + */ > BOOTMOD_KERNEL, > BOOTMOD_RAMDISK, > + BOOTMOD_UNKNOWN, > + > + /* The remaining module types are never automatically assigned. */ > BOOTMOD_XSM, > - BOOTMOD_UNKNOWN > } bootmodulekind; > > > @@ -66,6 +74,7 @@ const char __init *boot_fdt_cmdline(const void *fdt); > void add_boot_module(bootmodulekind kind, paddr_t start, paddr_t size, > const char *cmdline); > struct bootmodule *boot_module_find_by_kind(bootmodulekind kind); > +const char * __init boot_module_kind_as_string(bootmodulekind kind); > > #endif > /* > -- > 1.7.10.4 >
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
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@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) 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 >
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; 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++; else - panic("%s not a known xen multiboot type\n", name); + kind = BOOTMOD_UNKNOWN; prop = fdt_get_property(fdt, node, "reg", &len); if ( !prop ) @@ -278,11 +281,12 @@ static void __init early_print_info(void) mi->bank[i].start, mi->bank[i].start + mi->bank[i].size - 1); printk("\n"); - for ( i = 1 ; i < mods->nr_mods; i++ ) - printk("MODULE[%d]: %"PRIpaddr" - %"PRIpaddr" %s\n", + for ( i = 0 ; i < mods->nr_mods; i++ ) + printk("MODULE[%d]: %"PRIpaddr" - %"PRIpaddr" %-8s %s\n", i, mods->module[i].start, mods->module[i].start + mods->module[i].size, + boot_module_kind_as_string(mods->module[i].kind), mods->module[i].cmdline); nr_rsvd = fdt_num_mem_rsv(device_tree_flattened); for ( i = 0; i < nr_rsvd; i++ ) diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index 4c9dd3d..d358d04 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -221,6 +221,20 @@ struct bootmodule * __init boot_module_find_by_kind(bootmodulekind kind) return NULL; } +const char * __init boot_module_kind_as_string(bootmodulekind kind) +{ + switch ( kind ) + { + case BOOTMOD_XEN: return "Xen"; + case BOOTMOD_FDT: return "FDT"; + case BOOTMOD_KERNEL: return "Kernel"; + case BOOTMOD_RAMDISK: return "Ramdisk"; + case BOOTMOD_XSM: return "XSM"; + case BOOTMOD_UNKNOWN: return "Unknown"; + default: BUG(); + } +} + void __init discard_initial_modules(void) { struct bootmodules *mi = &bootinfo.modules; diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h index 57c98cb..f1a27fb 100644 --- a/xen/include/asm-arm/setup.h +++ b/xen/include/asm-arm/setup.h @@ -10,12 +10,20 @@ typedef enum { BOOTMOD_XEN, BOOTMOD_FDT, + /* Everything up to here is not freed after start of day */ BOOTMOD_LAST_PRESERVE = BOOTMOD_FDT, + + /* + * Default module types. For modules which are not given an + * explict type these are automatically used, in this order. + */ BOOTMOD_KERNEL, BOOTMOD_RAMDISK, + BOOTMOD_UNKNOWN, + + /* The remaining module types are never automatically assigned. */ BOOTMOD_XSM, - BOOTMOD_UNKNOWN } bootmodulekind; @@ -66,6 +74,7 @@ const char __init *boot_fdt_cmdline(const void *fdt); void add_boot_module(bootmodulekind kind, paddr_t start, paddr_t size, const char *cmdline); struct bootmodule *boot_module_find_by_kind(bootmodulekind kind); +const char * __init boot_module_kind_as_string(bootmodulekind kind); #endif /*
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(-)