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

[Julien Grall <julien.grall@xxxxxxx>]

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.
The rest of the code looks good to me. With that change and the other 
comment addressed in a separate patch:
Acked-by: Julien Grall <julien.grall@xxxxxxx>

Cheers,

--
Julien Grall

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