[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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.