Re: [Xen-devel] [PATCH] xen: Allow a default compiled-in command line using Kconfig

[Zhongze Liu <blackskygg@xxxxxxxxx>]

2017-03-06 18:50 GMT+08:00 Andrew Cooper <andrew.cooper3@xxxxxxxxxx>:
> On 06/03/17 02:38, Sky Liu wrote:
>> Added two new config entries in common/Kconfig: CMDLINE_BOOL and CMDLINE.
>
> Hello.  Thankyou for looking into this task.
>

Hi, Cooper. Thanks for your suggestions.

>
> As a start, why do you introduce CMDLINE_BOOL?  you don't actually use it.
>

My original motivation was to make CMDLINE and CMDLINE_OVERRIDE depends on
CMDLINE_BOOL, which is off by default. But I forget to add
CMDLINE_OVERRIDE in this patch.

>
>> These two entries enable an embedded command line
>> to be compiled in the hypervisor.
>> If CMDLINE_BOOL is set to Y, both arm and x86 startup routines
>> will call cmdline_parse() first on CMDLINE,
>> then on the line passed by the bootloader.
>> This allows downstreams to set their defaults
>> without modifying the source code all over the place.
>> Also probably useful for the embedded space.
>>
>> See Also: 
>> https://xenproject.atlassian.net/projects/XEN/issues/XEN-41?filter=allopenissues
>
> If you are going to include this URL, please drop the ?filter=allopenissues.
>

I'll drop that in the next version.

>
>>
>> Signed-off-by: Zhongze Liu <blackskygg@xxxxxxxxx>
>> ---
>>  xen/arch/arm/setup.c |  5 +++++
>>  xen/arch/x86/setup.c |  5 +++++
>>  xen/common/Kconfig   | 27 +++++++++++++++++++++++++++
>>  3 files changed, 37 insertions(+)
>>
>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>> index 92a2de6b70..55860a1299 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -729,6 +729,11 @@ void __init start_xen(unsigned long boot_phys_offset,
>>          + (fdt_paddr & ((1 << SECOND_SHIFT) - 1));
>>      fdt_size = boot_fdt_info(device_tree_flattened, fdt_paddr);
>>
>> +#ifdef CONFIG_CMDLINE
>> +    printk("Compiled-in command line: %s\n", CONFIG_CMDLINE);
>> +    cmdline_parse(cmdline);
>> +#endif
>
> You can be rather more cunning, and avoid the #ifdefary here.
>
> Given something like this:
>
> diff --git a/xen/common/kernel.c b/xen/common/kernel.c
> index 4b87c60..2da9cfe 100644
> --- a/xen/common/kernel.c
> +++ b/xen/common/kernel.c
> @@ -24,6 +24,8 @@ enum system_state system_state = SYS_STATE_early_boot;
>
>  xen_commandline_t saved_cmdline;
>
> +const char opt_def_cmdline[] = CONFIG_CMDLINE;
> +
>  static void __init assign_integer_param(
>      const struct kernel_param *param, uint64_t val)
>  {
> diff --git a/xen/include/xen/kernel.h b/xen/include/xen/kernel.h
> index 548b64d..99a428d 100644
> --- a/xen/include/xen/kernel.h
> +++ b/xen/include/xen/kernel.h
> @@ -100,5 +100,7 @@ extern enum system_state {
>
>  bool_t is_active_kernel_text(unsigned long addr);
>
> +extern const char opt_def_cmdline[];
> +
>  #endif /* _LINUX_KERNEL_H */
>
>
> Your code in the various setup.c's can be
>
> if ( strlen(opt_def_cmdline) )
> {
>     printk("Compiled-in command line: %s\n", opt_def_cmdline);
>     cmdline_parse(opt_def_cmdline);
> }
>
> which is rather neater and not liable to code-rot.
>
>> +
>>      cmdline = boot_fdt_cmdline(device_tree_flattened);
>>      printk("Command line: %s\n", cmdline);
>>      cmdline_parse(cmdline);
>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>> index dab67d5491..2b961f95cf 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -690,6 +690,11 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>          kextra += 3;
>>          while ( kextra[1] == ' ' ) kextra++;
>>      }
>> +
>> +#ifdef CONFIG_CMDLINE
>> +    printk("Compiled-in command line: %s\n", CONFIG_CMDLINE);
>> +    cmdline_parse(CONFIG_CMDLINE);
>> +#endif
>>      cmdline_parse(cmdline);
>>
>>      /* Must be after command line argument parsing and before
>> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
>> index f2ecbc43d6..1afcd8142c 100644
>> --- a/xen/common/Kconfig
>> +++ b/xen/common/Kconfig
>> @@ -237,4 +237,31 @@ config FAST_SYMBOL_LOOKUP
>>    The only user of this is Live patching.
>>
>>    If unsure, say Y.
>> +
>> +config CMDLINE_BOOL
>> + bool "Built-in hypervisor command line"
>> + default n
>> + ---help---
>> +  Allow for specifying boot arguments to the hypervisor at
>> +  build time.  On some systems (e.g. embedded ones), it is
>> +  necessary or convenient to provide some or all of the
>> +  hypervisor boot arguments with the hypervisor itself (that is,
>> +  to not rely on the boot loader to provide them.)
>> +
>> +  To compile command line arguments into the hypervisor,
>> +  set this option to 'Y', then fill in the
>> +  boot arguments in CONFIG_CMDLINE.
>> +
>> +config CMDLINE
>> + string "Built-in hypervisor command string"
>> + depends on CMDLINE_BOOL
>> + default ""
>> + ---help---
>> +  Enter arguments here that should be compiled into the hypervisor
>> +  image and used at boot time.  If the boot loader provides a
>> +  command line at boot time, it is appended to this string to
>> +  form the full hypervisor command line, when the system boots.
>
> As Jan identified, please make the indentation here match the rest of
> the file.
>
> You should state that if present, this command line is parsed first, so
> parameters from the bootloader will take precedence.
>
>> +
>> +  However, you can use the CONFIG_CMDLINE_OVERRIDE option to
>> +  change this behavior.
>
> This line isn't applicable to Xen.
>

Actually, I saw your reply after I send v2 of this patch, sorry for that.
I'll take your advice and revise my patch ASAP.
This is my first patch to the Xen project, and I'm still not very
familiar with the code base.
Since some details have been changed in my newly sent patch,
so please do review my patch v2 and give me more of your precious suggestions.

>
> ~Andrew
>
>>  endmenu
>

Thanks again.

Best Regards.

Zhongze Liu

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel