Re: [Xen-devel] [PATCH v5 06/25] xen/arm: introduce bootcmdlines

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 26 Oct 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 10/23/18 3:02 AM, Stefano Stabellini wrote:
> > @@ -314,12 +312,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++ )
> >       {
> > @@ -332,6 +330,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]);
> > +    printk("\n");
> 
> I am ok to see to accept this patch with this new prints. However,
> I would like to see a fix for it as soon as possible because this will be a
> massive pain to debug if a user has a issue. Indeed we can't figure out
> whether from the log the associatation cmdline <-> modules.

> 
> > @@ -2109,6 +2107,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 +2153,7 @@ int __init construct_dom0(struct domain *d)
> >     #endif
> >   +    kinfo.cmdline = kernel != NULL ? &kernel->cmdline[0] : NULL;
> 
> That's not the most obvious line to read. I would either put the condition
> between parentheses or use drop != NULL.

OK, I'll add the parentheses


> The rest of the code looks good to me. With that change and the other comment
> addressed in a separate patch:
> 

Turns out that to do that with the current stored information we need
access to the unflattened device tree, which we don't have yet in
early_print_info. So, I added a new start address field to struct
bootcmdline to store the start address in memory of the corresponding
bootmodule. That way, we can easily match a bootmodule with the
corresponding cmdline.

I did that in a new patch.


> Acked-by: Julien Grall <julien.grall@xxxxxxx>

Thanks!

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel