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

[Zhongze Liu <blackskygg@xxxxxxxxx>]

2017-03-10 23:03 GMT+08:00 Jan Beulich <JBeulich@xxxxxxxx>:
>>>> On 09.03.17 at 04:13, <blackskygg@xxxxxxxxx> wrote:
>> If CMDLINE is set, the cmdline_parse() routine will append the bootloader
>> command line to this string, forming the complete command line
>> before parsing.
>
> I disagree to making it behave like this: There's no need to
> concatenate both, simply parse them one after the other. The
> built-in one is well known (and hence also has no need to be in
> saved_cmdline). That'll avoid reducing the space available for
> user specified options.
>

I did so because I do think the embedded one should be in
saved_cmdline, too, but your suggestion
sounds quite reasonable.
Then I really have to introduce a wrapper to the original
cmdline_parse(). (Recursion seems not suitable for
this, since a new variable indicating the depth is inevitable, making
the parameter list even longer).
My solution will be: the parser calls gather_dom0_options() and the
original cmdline_parse first on
CONFIG_CMDLINE and then on the bootloader cmdline. The interface would
still be the same as
I did in this patch and leaving the original cmdline_parse() unchanged.
BTW, in the Xen tradition, will we rename the original cmdline_parse()
to _cmdline_parse() or
do_cmdline_parse() or something else?

>
>> @@ -1566,14 +1557,14 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>
>>      /* Grab the DOM0 command line. */
>>      cmdline = (char *)(mod[0].string ? __va(mod[0].string) : NULL);
>> -    if ( (cmdline != NULL) || (kextra != NULL) )
>> +    if ( (cmdline != NULL) || strlen(kextra) )
>
> Is there any reason why kextra can't come out as NULL if unset,
> avoiding the need to touch the code here? That would also avoid
> making kextra a static variable.
>

In the original code, kextra is a pointer to a suffix of the original
cmdline. It's
orphaned from cmdline by turning the first blank in " -- " into a '\0'.
But now since the dom0 options can appear in both CONFIG_CMDLINE and
the bootloader
cmdline, there must be some place (an array) to hold the concatenated
dom0 option string.
So if we want to avoid modifying this piece of code, I can only come
up with two solutions:
1.  Define a new array in this function and let kextra point to it.
Set kextra to NULL if the array is empty.
     But I think this is too awkward.
 2. Define a new array,  say, opt_dom0_options[],  in kernel.c, and
return the pointer to this array back to
     the caller when cmdline_parse() is invoked, or return NULL if the
array is empty.
What do you say?

>
>> --- a/xen/common/Kconfig
>> +++ b/xen/common/Kconfig
>> @@ -237,4 +237,28 @@ config FAST_SYMBOL_LOOKUP
>>         The only user of this is Live patching.
>>
>>         If unsure, say Y.
>> +
>> +config CMDLINE
>> +     string "Built-in hypervisor command string"
>> +     default ""
>> +     ---help---
>> +       Enter arguments here that should be compiled into the hypervisor
>> +       image and used at boot time.  If the bootloader provides a
>> +       command line at boot time, it is appended to this string to
>> +       form the full hypervisor command line, when the system boots.
>> +       So if the same command line option is not cumulative,
>> +       and was set both in this string and in the bootloader command line,
>> +       only the latter (i.e. the one in the bootloader command line),see
>> +       will take effect.
>> +
>> +config CMDLINE_OVERRIDE
>> +     bool "Built-in command line overrides bootloader arguments"
>> +     default n
>> +     depends on EXPERT = "y"
>
> Personally I think the other option should also be dependent on
> EXPERT. And the one here should probably depend on CMDLINE != ""
> - if someone really wants to force an empty one, (s)he could make
> it be just a single blank.
>

Well, sounds reasonable. But I still want to hear what others say.

>
> Jan
>

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