|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 09/25] xen/arm: introduce bootcmdlines
On Wed, 1 Aug 2018, Julien Grall wrote:
> Hi Stefano,
>
> On 01/08/18 00:27, Stefano Stabellini wrote:
> > Introduce a new array to store the cmdline of each boot module. It is
> > separate from struct bootmodules. Remove the cmdline field from struct
> > boot_module. This way, kernels and initrds with the same address in
> > memory can share struct bootmodule (important because we want them to be
> > free'd only once), but they can still have their separate bootcmdline
> > entries.
> >
> > Add a dt_name field to struct bootcmdline to make it easier to find the
> > correct entry. Store the name of the "xen,domain" compatible node (for
> > example "Dom1"). This is a better choice compared to the name of the
> > "multiboot,kernel" compatible node, because their names are not unique.
> > For instance there can be more than one "module@0x4c000000" in the
> > system, but there can only be one "/chosen/Dom1".
>
> As I mentioned in the previous version, the code is currently looking for
> multiboot,module everywhere in the DT rather than only in /chosen. So your
> name could not be uniq.
>
> However, this is not compliant with the protocol. Therefore you need to fix
> the code first to ensure the name will be uniq.
I'll fix this and everything else you pointed out
> >
> > Add a pointer to struct kernel_info to point to the cmdline for a given
> > kernel.
> >
> > Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> >
> > ---
> >
> > Changes in v3:
> > - introduce bootcmdlines
> > - do not modify boot_fdt_cmdline
> > - add comments
>
> I see no comments in the code. Did I miss anything?
>
> >
> > Changes in v2:
> > - new patch
> > ---
> > xen/arch/arm/bootfdt.c | 66
> > +++++++++++++++++++++++++++++++--------------
> > xen/arch/arm/domain_build.c | 8 +++---
> > xen/arch/arm/kernel.h | 1 +
> > xen/arch/arm/setup.c | 23 +++++++++++-----
> > xen/include/asm-arm/setup.h | 16 +++++++++--
> > 5 files changed, 82 insertions(+), 32 deletions(-)
> >
> > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > index 8eba42c..6f44022 100644
> > --- a/xen/arch/arm/bootfdt.c
> > +++ b/xen/arch/arm/bootfdt.c
> > @@ -163,6 +163,38 @@ static void __init process_memory_node(const void *fdt,
> > int node,
> > }
> > }
> > +static void __init add_boot_cmdline(const void *fdt, int node,
> > + const char *name, bootmodule_kind kind)
> > +{
> > + struct bootcmdlines *mods = &bootinfo.cmdlines;
> > + struct bootcmdline *mod;
>
> This feels sligthtly strange to use "mod" here. We are not dealing with boot
> modules but boot command line.
I'l rename it
> > + const struct fdt_property *prop;
> > + int len;
> > + const char *cmdline;
> > +
> > + if ( mods->nr_mods == MAX_MODULES )
> > + {
> > + printk("Ignoring %s boot module (too many)\n", name);
>
> Same here. This needs to be updated.
I'll reword it
> > + return;
> > + }
> > +
> > + mod = &mods->cmdline[mods->nr_mods++];
> > + mod->kind = kind;
> > +
> > + if ( strlen(name) > DT_MAX_NAME )
> > + panic("module %s name too long\n", name);
>
> This would really never happen. It feels an ASSERT(strlen(name) >
> DT_MAX_NAME)) would be more suitable.
OK, easy to change
> > + safe_strcpy(mod->dt_name, name);
> > +
> > + prop = fdt_get_property(fdt, node, "bootargs", &len);
> > + if ( prop )
> > + {
> > + if ( len > BOOTMOD_MAX_CMDLINE )
> > + panic("module %s command line too long\n", name);
> > + cmdline = prop->data;
> > + safe_strcpy(mod->cmdline, cmdline);
> > + }
> > +}
> > +
> > static void __init process_multiboot_node(const void *fdt, int node,
> > const char *name,
> > u32 address_cells, u32
> > size_cells)
> > @@ -172,8 +204,12 @@ static void __init process_multiboot_node(const void
> > *fdt, int node,
> > const __be32 *cell;
> > bootmodule_kind kind;
> > paddr_t start, size;
> > - const char *cmdline;
> > int len;
> > + int parent_node;
> > +
> > + parent_node = fdt_parent_offset(fdt, node);
> > + if ( parent_node < 0 )
> > + panic("node %s missing a parent\n", name);
>
> It feels an ASSERT(parent_node < 0) would be more suitable as this should
> never really happen.
OK
> > prop = fdt_get_property(fdt, node, "reg", &len);
> > if ( !prop )
> > @@ -220,17 +256,8 @@ static void __init process_multiboot_node(const void
> > *fdt, int node,
> > kind = BOOTMOD_XSM;
> > }
> > - prop = fdt_get_property(fdt, node, "bootargs", &len);
> > - if ( prop )
> > - {
> > - if ( len > BOOTMOD_MAX_CMDLINE )
> > - panic("module %s command line too long\n", name);
> > - cmdline = prop->data;
> > - }
> > - else
> > - cmdline = NULL;
> > -
>
> I am not entirely sure to understand why this code has been moved. With your
> new code, if you have a module without commandline then you will still add a
> cmdline with nothing. This looks quite pointless.
>
> Instead, you could keep this code here and only call add_bootcmdline when it
> is not NULL (or ignore it directly in the function).
I'll do that
> > - add_boot_module(kind, start, size, cmdline);
> > + add_boot_module(kind, start, size);
> > + add_boot_cmdline(fdt, node, fdt_get_name(fdt, parent_node, &len),
> > kind);
> > }
> > static void __init process_chosen_node(const void *fdt, int node,
> > @@ -276,7 +303,7 @@ static void __init process_chosen_node(const void *fdt,
> > int node,
> > printk("Initrd %"PRIpaddr"-%"PRIpaddr"\n", start, end);
> > - add_boot_module(BOOTMOD_RAMDISK, start, end-start, NULL);
> > + add_boot_module(BOOTMOD_RAMDISK, start, end-start);
> > }
> > static int __init early_scan_node(const void *fdt,
> > @@ -307,12 +334,11 @@ static void __init early_print_info(void)
> > mi->bank[i].start + mi->bank[i].size - 1);
> > printk("\n");
> > for ( i = 0 ; i < mods->nr_mods; i++ )
> > - printk("MODULE[%d]: %"PRIpaddr" - %"PRIpaddr" %-12s %s\n",
> > + printk("MODULE[%d]: %"PRIpaddr" - %"PRIpaddr" %-12s\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);
> > + boot_module_kind_as_string(mods->module[i].kind));
>
> With that change the command line is not printed anymore and also not
> associated to a module. This was quite useful for debugging bootloader issue.
I'll add more debug info, but in a separate loop for simplicity
> > nr_rsvd = fdt_num_mem_rsv(device_tree_flattened);
> > for ( i = 0; i < nr_rsvd; i++ )
> > {
>
> [...]
>
> > +struct bootcmdline * __init boot_cmdline_find_by_kind(bootmodule_kind kind)
> > +{
> > + struct bootcmdlines *mods = &bootinfo.cmdlines;
> > + struct bootcmdline *mod;
>
> Again, the name "mod" is misleading here.
I'll rename
> > + int i;
>
> Newline here.
I'll add
> > + for (i = 0 ; i < mods->nr_mods ; i++ )
>
> for ( ... )
I'll fix
> > + {
> > + mod = &mods->cmdline[i];
> > + if ( mod->kind == kind )
> > + return mod;
> > + }
> > + return NULL;
> > +}
> > +
> > const char * __init boot_module_kind_as_string(bootmodule_kind kind)
> > {
> > switch ( kind )
> > @@ -723,7 +732,7 @@ void __init start_xen(unsigned long boot_phys_offset,
> > /* Register Xen's load address as a boot module. */
> > xen_bootmodule = add_boot_module(BOOTMOD_XEN,
> > (paddr_t)(uintptr_t)(_start +
> > boot_phys_offset),
> > - (paddr_t)(uintptr_t)(_end - _start + 1),
> > NULL);
> > + (paddr_t)(uintptr_t)(_end - _start + 1));
> > BUG_ON(!xen_bootmodule);
> > xen_paddr = get_xen_paddr();
> > diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
> > index f1e4a3f..cb7da51 100644
> > --- a/xen/include/asm-arm/setup.h
> > +++ b/xen/include/asm-arm/setup.h
> > @@ -35,6 +35,12 @@ struct bootmodule {
> > bootmodule_kind kind;
> > paddr_t start;
> > paddr_t size;
> > +};
> > +
> > +#define DT_MAX_NAME 32
>
> It might be useful to explain where 32 comes from.
The limit is somewhat arbitrary. I'll explain in a comment.
> > +struct bootcmdline {
> > + bootmodule_kind kind;
> > + char dt_name[DT_MAX_NAME];
> > char cmdline[BOOTMOD_MAX_CMDLINE];
> > };
> > @@ -43,9 +49,15 @@ struct bootmodules {
> > struct bootmodule module[MAX_MODULES];
> > };
> > +struct bootcmdlines {
> > + int nr_mods;
>
> unsigned int here please.
OK
> > + struct bootcmdline cmdline[MAX_MODULES];
> > +};
> > +
> > struct bootinfo {
> > struct meminfo mem;
> > struct bootmodules modules;
> > + struct bootcmdlines cmdlines;
> > #ifdef CONFIG_ACPI
> > struct meminfo acpi;
> > #endif
> > @@ -78,9 +90,9 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t
> > paddr);
> > const char __init *boot_fdt_cmdline(const void *fdt);
> > struct bootmodule *add_boot_module(bootmodule_kind kind,
> > - paddr_t start, paddr_t size,
> > - const char *cmdline);
> > + paddr_t start, paddr_t size);
> > struct bootmodule *boot_module_find_by_kind(bootmodule_kind kind);
> > +struct bootcmdline *boot_cmdline_find_by_kind(bootmodule_kind kind);
> > const char * __init boot_module_kind_as_string(bootmodule_kind kind);
> > #endif
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |