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

[Zhongze Liu <blackskygg@xxxxxxxxx>]

Thanks for your time reviewing my code.

2017-03-07 17:36 GMT+08:00 Jan Beulich <JBeulich@xxxxxxxx>:
>>>> On 07.03.17 at 09:34, <blackskygg@xxxxxxxxx> wrote:
>
> As an initial remark: Am I right in guessing that you manually prefix
> [Xen-devel] to your message subject? Please don't do so - receiving
> patches without that prefix serves as an indication to the receiver
> that (s)he is on the Cc list.
>

yes, you're right. I thought this should be done manually, but It
seems that I'm wrong.
I won't do that anymore.

>
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -705,10 +705,16 @@ void __init start_xen(unsigned long boot_phys_offset,
>>      size_t fdt_size;
>>      int cpus, i;
>>      paddr_t xen_paddr;
>> -    const char *cmdline;
>> +    const char *cmdline, *boot_cmdline;
>>      struct bootmodule *xen_bootmodule;
>>      struct domain *dom0;
>>      struct xen_arch_domainconfig config;
>> +#ifdef CONFIG_CMDLINE_BOOL
>> +    static xen_commandline_t __initdata builtin_cmdline = CONFIG_CMDLINE;
>> +#ifndef CONFIG_CMDLINE_OVERRIDE
>> +    static xen_commandline_t __initdata complete_cmdline = "";
>
> Pointless initializer.
>

I'll remove this.

>
>> +#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.

However, I do agree with your suggestions on avoiding duplications of the same
pieces of code. I will address this problem later.

>
>> --- 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.

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.

>
>> --- 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.

>
>> +     ---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 bootloader 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
>
> Coming back to the #ifdef-ary remark earlier on - if instead of
> "depends on" you made the prompt conditional, CMDLINE would
> always be defined, i.e. you could use without #ifdef guards. Of
> course with that the question of the usefulness of
> CMDLINE_BOOL arises: Does this really serve a purpose?
>
>> +     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 was set twice, only the latter
>> +       (i.e. the one in the bootloader command line), will take effect.
>
> ... unless an option is cumulative (I don't think we have any such
> right now, but iirc Linux does, and so we should not exclude that we
> may gain such).
>
>> +       However, you can use the CONFIG_CMDLINE_OVERRIDE option to
>> +       change this behavior.
>> +
>> +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.

>
> Jan

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