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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.03.17 at 12:21, <blackskygg@xxxxxxxxx> wrote:
> 2017-03-07 17:36 GMT+08:00 Jan Beulich <JBeulich@xxxxxxxx>:
>>>>> On 07.03.17 at 09:34, <blackskygg@xxxxxxxxx> wrote:
>>> +#endif /* CONFIG_CMDLINE_OVERRIDE */
>>> +#endif /* CONFIG_CMDLINE_BOOL */
>>
>> I think this #ifdef-ary can be reduced, along the lines of Andrew's
>> earlier suggestion. But my main complaint remains that this
>> continues to be duplicated between ARM and x86.
>>
> 
> I think wrapping CMDLINE and CMDLINE_OVERRIDE in
> a #ifdef CONFIG_CMDLINE_BOOL block makes the struture more clear,
> and makes the code easier to read, because CMDLINE and CMDLINE_OVERRIDE
> should be grouped together. BTW, this is exactly what linux did:
> 
> See https://github.com/torvalds/linux/blob/master/arch/x86/Kconfig#L2193 ,
> https://github.com/torvalds/linux/blob/master/arch/x86/kernel/setup.c#L237,
> https://github.com/torvalds/linux/blob/master/arch/x86/kernel/setup.c#L963,
> https://github.com/torvalds/linux/blob/master/arch/mips/kernel/setup.c#L70 
> and 
> https://github.com/torvalds/linux/blob/master/arch/mips/kernel/setup.c#L807.

Well, we don't have to slavishly follow what Linux does. I'd be
interested to hear particularly Doug's and Andrew's opinions.

>>> --- a/xen/arch/x86/setup.c
>>> +++ b/xen/arch/x86/setup.c
>>> @@ -636,10 +636,41 @@ static char * __init cmdline_cook(char *p, const char 
>>> *loader_name)
>>>      return p;
>>>  }
>>>
>>> +/*
>>> + * Extracts dom0 options from the commandline.
>>> + *
>>> + * Options after ' -- ' separator belong to dom0.
>>> + *  1. Orphan dom0's options from Xen's command line.
>>> + *  2. Skip all but final leading space from dom0's options.
>>> + */
>>> +
>>> +static inline char* __init extract_dom0_options(char *cmdline)
>>> +{
>>> +    char *kextra;
>>> +
>>> +    if ( (kextra = strstr(cmdline, " -- ")) != NULL )
>>> +    {
>>> +        *kextra = '\0';
>>> +        kextra += 3;
>>> +        while ( kextra[1] == ' ' ) kextra++;
>>
>> The body of the while() wants to go on its own line.
>>
>> And then - why is this Dom0 option handling done only on x86?
>>
> 
> As you might have noticed, there isn't any code dealing with the dom0 options
> in arch/arm/setup.c, and the arm version of construct_dom0() doesn't take any
> command line options as its parameter,
> so I have the reason to believe that this feature is not available
> under the arm architecture.

Looks like an omission to me - Julien, Stefano?

> As for the duplicated code. What do you say if I add a wrapper to
> common/kernerl.c:cmdline_parse(). The wrapper automatically deals
> with the CMDLINE_BOOL option, if toggled, and call the original 
> cmdline_parser
> on the concatenated line. The wrapper could be something like:
>     void cmdline_parse(char *cmdline, char *kextra);
> where kextra will be filed with the concatenated dom0_options, if presented.
> And for those who don't expect dom0_options, I mean, for arm, kextra could be
> set to NULL, telling cmdline_parse() not to find any " -- " in the cmdline.

Sounds reasonable.

>>> --- a/xen/common/Kconfig
>>> +++ b/xen/common/Kconfig
>>> @@ -237,4 +237,44 @@ 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
>>
>> I don't think this line serves any purpose (also another time below).
> 
> But I do think this make the struture of the config set more clear.

Well, not sure. Let's see what others think (as said above).

>>> +config CMDLINE_OVERRIDE
>>> +     bool "Built-in command line overrides bootloader arguments"
>>> +     default n
>>> +     depends on CMDLINE_BOOL
>>> +     ---help---
>>> +       Set this option to 'Y' to have the hypervisor ignore the bootloader
>>> +       command line, and use ONLY the built-in command line.
>>> +
>>> +       This is used to work around broken bootloaders. This should
>>> +       be set to 'N' under normal conditions.
>>
>> I think this calls for an EXPERT dependency.
> 
> I think the last line of the help message can serve this purpose. But
> If you think adding an EXPERT option in the Kconfig file is necessary,
> I'll just do that.

I didn't ask for adding an EXPERT option (we already have one), but
for adding a dependency to it. I for one would find it quite surprising
if none of the options I pass the Xen would be honored.

Jan

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