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

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

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

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

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

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

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

Jan

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