|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 05/23] xen/arm: introduce bootcmdlines
On Tue, 9 Oct 2018, Julien Grall wrote:
> On 05/10/2018 19:47, 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".
> >
> > 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 v4:
> > - check that the multiboot node is under /chosen
> > - use cmd and cmds as variable names for struct bootcmdline and struct
> > bootcmdline*
> > - fix printk
> > - use ASSERT instea of panic
> > - do not add empty cmdline entries
> > - add more debug printks to early_print_info
> > - code style fixes
> > - add comment on DT_MAX_NAME
> > - increase DT_MAX_NAME to 41
> > - make nr_mods unsigned int
> >
> > Changes in v3:
> > - introduce bootcmdlines
> > - do not modify boot_fdt_cmdline
> > - add comments
> >
> > Changes in v2:
> > - new patch
> > ---
> > xen/arch/arm/bootfdt.c | 82
> > +++++++++++++++++++++++++++++++++------------
> > xen/arch/arm/domain_build.c | 8 +++--
> > xen/arch/arm/kernel.h | 1 +
> > xen/arch/arm/setup.c | 24 +++++++++----
> > xen/include/asm-arm/setup.h | 17 ++++++++--
> > 5 files changed, 99 insertions(+), 33 deletions(-)
> >
> > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > index 8eba42c..273032b 100644
> > --- a/xen/arch/arm/bootfdt.c
> > +++ b/xen/arch/arm/bootfdt.c
> > @@ -163,6 +163,37 @@ 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 *cmds = &bootinfo.cmdlines;
> > + struct bootcmdline *cmd;
> > + const struct fdt_property *prop;
> > + int len;
> > + const char *cmdline;
> > +
> > + if ( cmds->nr_mods == MAX_MODULES )
> > + {
> > + printk("Ignoring %s cmdline (too many)\n", name);
> > + return;
> > + }
> > +
> > + prop = fdt_get_property(fdt, node, "bootargs", &len);
> > + if ( !prop )
> > + return;
> > +
> > + cmd = &cmds->cmdline[cmds->nr_mods++];
> > + cmd->kind = kind;
> > +
> > + ASSERT(strlen(name) <= DT_MAX_NAME);
> > + safe_strcpy(cmd->dt_name, name);
> > +
> > + if ( len > BOOTMOD_MAX_CMDLINE )
> > + panic("module %s command line too long\n", name);
> > + cmdline = prop->data;
> > + safe_strcpy(cmd->cmdline, cmdline);
> > +}
> > +
> > static void __init process_multiboot_node(const void *fdt, int node,
> > const char *name,
> > u32 address_cells, u32
> > size_cells)
> > @@ -172,8 +203,20 @@ 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 len = sizeof("/chosen");
> > + char path[8]; /* sizeof "/chosen" */
> > + int parent_node;
> > +
> > + parent_node = fdt_parent_offset(fdt, node);
> > + ASSERT(parent_node >= 0);
> > +
> > + /* Check that the node is under "chosen" */
> > + fdt_get_path(fdt, node, path, len);
>
> It would be good if you test the code with wrong node. For instance
> fdt_get_path may not fill path (i.e if the buffer is too short). So path will
> contain garbage.
I'll do
> > + if ( strncmp(path, "/chosen", len - 1) )
> > + {
> > + printk("DEBUG %s %s\n",name,path);
>
> This looks like a left-over from your debug.
I'll remove
> > + return;
> > + }
> As I said in the previous patch, this needs to be fixed first. By that I meant
> this should be a separate patch so it could be backported.
I'll separate it out
> Also, with this patch you change correctly the behavior to deny node not in
> chosen. But you don't explain in the commit message that it affects the
> current multiboot node.
I'll mention it.
> However, I still don't think this code is correct. You would allow the
> following path /chosen/foo/bar/multiboot. The parent would be "bar" and there
> are no guarantee it would be uniq (it is not under /chosen). I think this
> could be fixed by taking into account the depth of the node.
> AFAICT, the multiboot node should always be at maximum depth 3.
I'll add a check.
> > prop = fdt_get_property(fdt, node, "reg", &len);
> > if ( !prop )
> > @@ -220,17 +263,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;
> > -
> > - 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 +310,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,
> > @@ -299,6 +333,7 @@ static void __init early_print_info(void)
> > {
> > struct meminfo *mi = &bootinfo.mem;
> > struct bootmodules *mods = &bootinfo.modules;
> > + struct bootcmdlines *cmds = &bootinfo.cmdlines;
> > int i, nr_rsvd;
> > for ( i = 0; i < mi->nr_banks; i++ )
> > @@ -307,12 +342,12 @@ 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));
> > +
> > nr_rsvd = fdt_num_mem_rsv(device_tree_flattened);
> > for ( i = 0; i < nr_rsvd; i++ )
> > {
> > @@ -325,6 +360,11 @@ static void __init early_print_info(void)
> > i, s, e);
> > }
> > printk("\n");
> > + for ( i = 0 ; i < cmds->nr_mods; i++ )
> > + printk("CMDLINE[%d]:%s %s\n", i,
> > + cmds->cmdline[i].dt_name,
> > + &cmds->cmdline[i].cmdline[0]);
>
> Thank you for adding the command line. However, there are still no way to
> associate the command line with a module. For each command line, you should be
> able to know the associate module.
It is possible once the whole series is committed, by using
boot_cmdline_find_by_name and the domU field introduced later in this
series. I could move boot_cmdline_find_by_name earlier, but since there
are no callers, I would have to #if 0 it. My choice would be to leave
things as they are.
> > + printk("\n");
> > }
> > /**
> > @@ -341,7 +381,7 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t
> > paddr)
> > if ( ret < 0 )
> > panic("No valid device tree\n");
> > - add_boot_module(BOOTMOD_FDT, paddr, fdt_totalsize(fdt), NULL);
> > + add_boot_module(BOOTMOD_FDT, paddr, fdt_totalsize(fdt));
> > device_tree_for_each_node((void *)fdt, early_scan_node, NULL);
> > early_print_info();
> > @@ -361,11 +401,11 @@ const char *boot_fdt_cmdline(const void *fdt)
> > prop = fdt_get_property(fdt, node, "xen,xen-bootargs", NULL);
> > if ( prop == NULL )
> > {
> > - struct bootmodule *dom0_mod =
> > - boot_module_find_by_kind(BOOTMOD_KERNEL);
> > + struct bootcmdline *dom0_cmdline =
> > + boot_cmdline_find_by_kind(BOOTMOD_KERNEL);
> > if (fdt_get_property(fdt, node, "xen,dom0-bootargs", NULL) ||
> > - ( dom0_mod && dom0_mod->cmdline[0] ) )
> > + ( dom0_cmdline && dom0_cmdline->cmdline[0] ) )
> > prop = fdt_get_property(fdt, node, "bootargs", NULL);
> > }
> > if ( prop == NULL )
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index f552154..64f8d6b 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -375,7 +375,7 @@ static int __init write_properties(struct domain *d,
> > struct kernel_info *kinfo,
> > int res = 0;
> > int had_dom0_bootargs = 0;
> > - const struct bootmodule *kernel = kinfo->kernel_bootmodule;
> > + const struct bootcmdline *kernel =
> > boot_cmdline_find_by_kind(BOOTMOD_KERNEL);
>
> Why do you need to lookup the command line here? You have just introduced a
> field in kinfo to store command line.
good point, I'll use the new field here
> > if ( kernel && kernel->cmdline[0] )
> > bootargs = &kernel->cmdline[0];
> > @@ -952,9 +952,9 @@ static int __init make_chosen_node(const struct
> > kernel_info *kinfo)
> > if ( res )
> > return res;
> > - if ( mod && mod->cmdline[0] )
> > + if ( kinfo->cmdline && kinfo->cmdline[0] )
> > {
> > - bootargs = &mod->cmdline[0];
> > + bootargs = &kinfo->cmdline[0];
> > res = fdt_property(fdt, "bootargs", bootargs, strlen(bootargs) +
> > 1);
> > if ( res )
> > return res;
> > @@ -2109,6 +2109,7 @@ static void __init find_gnttab_region(struct domain
> > *d,
> > int __init construct_dom0(struct domain *d)
> > {
> > + const struct bootcmdline *kernel =
> > boot_cmdline_find_by_kind(BOOTMOD_KERNEL);
> > struct kernel_info kinfo = {};
> > struct vcpu *saved_current;
> > int rc, i, cpu;
> > @@ -2154,6 +2155,7 @@ int __init construct_dom0(struct domain *d)
> > #endif
> > + kinfo.cmdline = kernel != NULL ? &kernel->cmdline[0] : NULL;
> > allocate_memory(d, &kinfo);
> > find_gnttab_region(d, &kinfo);
> > diff --git a/xen/arch/arm/kernel.h b/xen/arch/arm/kernel.h
> > index 47eacb5..39b7828 100644
> > --- a/xen/arch/arm/kernel.h
> > +++ b/xen/arch/arm/kernel.h
> > @@ -29,6 +29,7 @@ struct kernel_info {
> > /* boot blob load addresses */
> > const struct bootmodule *kernel_bootmodule, *initrd_bootmodule;
> > + const char* cmdline;
> > paddr_t dtb_paddr;
> > paddr_t initrd_paddr;
> > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > index ea2495a..bc7dd97 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -201,8 +201,7 @@ void __init dt_unreserved_regions(paddr_t s, paddr_t e,
> > }
> > struct bootmodule __init *add_boot_module(bootmodule_kind kind,
> > - paddr_t start, paddr_t size,
> > - const char *cmdline)
> > + paddr_t start, paddr_t size)
> > {
> > struct bootmodules *mods = &bootinfo.modules;
> > struct bootmodule *mod;
> > @@ -218,10 +217,6 @@ struct bootmodule __init
> > *add_boot_module(bootmodule_kind kind,
> > mod->kind = kind;
> > mod->start = start;
> > mod->size = size;
> > - if ( cmdline )
> > - safe_strcpy(mod->cmdline, cmdline);
> > - else
> > - mod->cmdline[0] = 0;
> > return mod;
> > }
> > @@ -240,6 +235,21 @@ struct bootmodule * __init
> > boot_module_find_by_kind(bootmodule_kind kind)
> > return NULL;
> > }
> > +struct bootcmdline * __init boot_cmdline_find_by_kind(bootmodule_kind
> > kind)
>
> Functions dealing with a same structure should stick together. I actually
> quite dislike that add_bood_cmdline is part of bootfdt.c. There are no need
> for it to be there except the fact you moved the FDT lookup in the function.
> As I suggested in the previous patch, the lookup could have been left outside
> of the function.
I'll move add_bood_cmdline to setup.c.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |