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

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

[Zhongze Liu]

Added 3 new config entries in common/Kconfig:
    CMDLINE_BOOL, CMDLINE and CMDLINE_OVERRIDE

These 3 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 append the bootloader command line to the built-in one,
forming the complete command line. This order of concatenation
implies that if any options are set in both the built-in and
bootloader command line, only the ones in the latter will take effect.
And if CMDLINE_OVERRIDE is set to y, the whole bootloader command line
will be ignored, which will be useful to work around broken bootloaders.

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/browse/XEN-41

Signed-off-by: Zhongze Liu <blackskygg@gmail.com>
---
Changed since v2:
  * Clarify the parsing order (and thus the overiding behavior)
    of the two commandlines when parsing.
  * Shortened the XEN-41 issue url in the commit message.
  * Wrap the related work into a #ifdef CONFIG_CMDLINE_BOOL block
    in various setup.c's.
---
 xen/arch/arm/setup.c | 30 +++++++++++++++++--
 xen/arch/x86/setup.c | 84 ++++++++++++++++++++++++++++++++++++++++++++--------
 xen/common/Kconfig   | 40 +++++++++++++++++++++++++
 3 files changed, 139 insertions(+), 15 deletions(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 92a2de6b70..e6b8f4da34 100644
--- 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 = "";
+#endif /* CONFIG_CMDLINE_OVERRIDE */
+#endif /* CONFIG_CMDLINE_BOOL */
 
     setup_cache();
 
@@ -729,7 +735,27 @@ 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);
 
-    cmdline = boot_fdt_cmdline(device_tree_flattened);
+    boot_cmdline = boot_fdt_cmdline(device_tree_flattened);
+    cmdline = boot_cmdline;
+
+#ifdef CONFIG_CMDLINE_BOOL
+    /* Process the built-in command line options. */
+
+    printk("Compiled-in command line: %s\n", builtin_cmdline);
+
+#ifdef CONFIG_CMDLINE_OVERRIDE
+    /* Ignore the bootloader cmdline. */
+    cmdline = builtin_cmdline;
+#else
+    safe_strcat(complete_cmdline, builtin_cmdline);
+    safe_strcat(complete_cmdline, " ");
+    safe_strcat(complete_cmdline, boot_cmdline);
+
+    cmdline = complete_cmdline;
+#endif /* CONFIG_CMDLINE_OVERRIDE */
+
+#endif /* CONFIG_CMDLINE_BOOL */
+
     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..3a6341fd3c 100644
--- 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++;
+    }
+
+    return kextra;
+}
+
 void __init noreturn __start_xen(unsigned long mbi_p)
 {
     char *memmap_type = NULL;
-    char *cmdline, *kextra, *loader;
+    char *boot_cmdline, *boot_kextra, *loader;
+    char *cmdline, *kextra = NULL;
+#ifdef CONFIG_CMDLINE_BOOL
+    static xen_commandline_t __initdata builtin_cmdline = CONFIG_CMDLINE;
+    char *builtin_kextra;
+#ifndef CONFIG_CMDLINE_OVERRIDE
+    static xen_commandline_t __initdata complete_cmdline = "";
+    static char __initdata complete_kextra[MAX_GUEST_CMDLINE] = "";
+#endif /* CONFIG_CMDLINE_OVERRIDE */
+#endif /* CONFIG_CMDLINE_BOOL */
     unsigned int initrdidx, domcr_flags = DOMCRF_s3_integrity;
     multiboot_info_t *mbi = __va(mbi_p);
     module_t *mod = (module_t *)__va(mbi->mods_addr);
@@ -676,20 +707,47 @@ void __init noreturn __start_xen(unsigned long mbi_p)
         ? (char *)__va(mbi->boot_loader_name) : "unknown";
 
     /* Parse the command-line options. */
-    cmdline = cmdline_cook((mbi->flags & MBI_CMDLINE) ?
-                           __va(mbi->cmdline) : NULL,
-                           loader);
-    if ( (kextra = strstr(cmdline, " -- ")) != NULL )
+
+    boot_cmdline = cmdline_cook((mbi->flags & MBI_CMDLINE) ?
+                                __va(mbi->cmdline) : NULL,
+                                loader);
+    boot_kextra = extract_dom0_options(boot_cmdline);
+    cmdline = boot_cmdline;
+    kextra = boot_kextra;
+
+#ifdef CONFIG_CMDLINE_BOOL
+    /* Process the built-in command line options. */
+    builtin_kextra = extract_dom0_options(builtin_cmdline);
+    printk("Compiled-in command line: %s\n", builtin_cmdline);
+
+#ifdef CONFIG_CMDLINE_OVERRIDE
+    /* Ignore the bootloader cmdline. */
+    cmdline = builtin_cmdline;
+    kextra = builtin_kextra;
+#else
+    /* Append the bootloader cmdline to the builtin one
+     * to form the complete cmdline. And do the same to dom0 options. */
+    safe_strcat(complete_cmdline, builtin_cmdline);
+    safe_strcat(complete_cmdline, " ");
+    safe_strcat(complete_cmdline, boot_cmdline);
+
+    if ( builtin_kextra != NULL )
     {
-        /*
-         * 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.
-         */
-        *kextra = '\0';
-        kextra += 3;
-        while ( kextra[1] == ' ' ) kextra++;
+        safe_strcat(complete_kextra, builtin_kextra);
+        safe_strcat(complete_kextra, " ");
+    }
+    if ( boot_kextra != NULL )
+    {
+        safe_strcat(complete_kextra, boot_kextra);
     }
+
+    cmdline = complete_cmdline;
+    /* kextra should be NULL if it's empty */
+    kextra = complete_kextra[0] ? complete_kextra : NULL;
+#endif /* CONFIG_CMDLINE_OVERRIDE */
+
+#endif /* CONFIG_CMDLINE_BOOL */
+
     cmdline_parse(cmdline);
 
     /* Must be after command line argument parsing and before
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index f2ecbc43d6..544b8e7804 100644
--- 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
+	---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
+	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.
+
+	  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.
 endmenu
-- 
2.12.0


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

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

[Jan Beulich]

>>> On 07.03.17 at 09:34, <blackskygg@gmail.com> 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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Zhongze Liu]

Thanks for your time reviewing my code.

2017-03-07 17:36 GMT+08:00 Jan Beulich <JBeulich@suse.com>:
>>>> On 07.03.17 at 09:34, <blackskygg@gmail.com> 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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Jan Beulich]

>>> On 07.03.17 at 12:21, <blackskygg@gmail.com> wrote:
> 2017-03-07 17:36 GMT+08:00 Jan Beulich <JBeulich@suse.com>:
>>>>> On 07.03.17 at 09:34, <blackskygg@gmail.com> 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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Julien Grall]

Hi Jan,

On 03/07/2017 12:52 PM, Jan Beulich wrote:
>>>> On 07.03.17 at 12:21, <blackskygg@gmail.com> wrote:
>> 2017-03-07 17:36 GMT+08:00 Jan Beulich <JBeulich@suse.com>:
>>>>>> On 07.03.17 at 09:34, <blackskygg@gmail.com> wrote:
>>>> +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?

DOM0 and Xen command line are passed separately through either Device 
Tree or for UEFI xen configuration file (see -cfg=...).

So I am not sure to understand what would be the benefits to handle DOM0 
parameters after -- on Xen command line.

Cheers,

-- 
Julien Grall

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

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

[Jan Beulich]

>>> On 07.03.17 at 14:48, <julien.grall@arm.com> wrote:
> On 03/07/2017 12:52 PM, Jan Beulich wrote:
>>>>> On 07.03.17 at 12:21, <blackskygg@gmail.com> wrote:
>>> 2017-03-07 17:36 GMT+08:00 Jan Beulich <JBeulich@suse.com>:
>>>>>>> On 07.03.17 at 09:34, <blackskygg@gmail.com> wrote:
>>>>> +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?
> 
> DOM0 and Xen command line are passed separately through either Device 
> Tree or for UEFI xen configuration file (see -cfg=...).
> 
> So I am not sure to understand what would be the benefits to handle DOM0 
> parameters after -- on Xen command line.

So you have no case of a boot loader which allows you to type in
extra options on just a single line? On x86 the feature had originally
been added because old grub didn't have a separate line for Dom0
options in its graphical menu. Nowadays the functionality is handy
namely when starting xen.efi from the EFI shell (where again you
obviously only have a single command line), but quite likely this may
also be of use with grub's chain loading model (which I simply don't
use very often, so I'm not finally sure on that one).

Jan


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

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

[Julien Grall]

Hi Jan,

On 03/07/2017 02:18 PM, Jan Beulich wrote:
>>>> On 07.03.17 at 14:48, <julien.grall@arm.com> wrote:
>> On 03/07/2017 12:52 PM, Jan Beulich wrote:
>>>>>> On 07.03.17 at 12:21, <blackskygg@gmail.com> wrote:
>>>> 2017-03-07 17:36 GMT+08:00 Jan Beulich <JBeulich@suse.com>:
>>>>>>>> On 07.03.17 at 09:34, <blackskygg@gmail.com> wrote:
>>>>>> +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?
>>
>> DOM0 and Xen command line are passed separately through either Device
>> Tree or for UEFI xen configuration file (see -cfg=...).
>>
>> So I am not sure to understand what would be the benefits to handle DOM0
>> parameters after -- on Xen command line.
>
> So you have no case of a boot loader which allows you to type in
> extra options on just a single line? On x86 the feature had originally
> been added because old grub didn't have a separate line for Dom0
> options in its graphical menu. Nowadays the functionality is handy
> namely when starting xen.efi from the EFI shell (where again you
> obviously only have a single command line), but quite likely this may
> also be of use with grub's chain loading model (which I simply don't
> use very often, so I'm not finally sure on that one).

My knowledge is quite limited on boot process for x86. How do you pass 
the kernel/initrd/xsm blob on UEFI? Can you do it from the command line 
or are you using the -cfg=... and specify it in a file?

On ARM we have two way to boot Xen:
	- Using UEFI bootloader with either Device-Tree or ACPI
	- Using non-UEFI bootloader with Device-Tree only

In the case of UEFI bootloader, we are using the xen configuration file 
to describe the modules (e.g kernel, initramfs, XSM) and the both xen 
and DOM0 command line.

For non-UEFI bootloader, we have designed the boot protocol based on the 
device-tree and will allows you to specify both xen and DOM0 and all the 
modules (see [1]). The bootloader needs to be able to modify the 
device-tree (such via a shell like on U-boot) or the user needs to 
modify the device-tree before hand.

To answer your question, a bootloader supporting only a single line 
would still need to be modified to provide the various modules. At that 
stage, adding DOM0 command line is very easy.

Now, if you tell me that all the modules can be described on the UEFI 
command line, then I think handling '--' would be nice. If not, I don't 
think it is worth it.

Cheers,

[1] http://xenbits.xen.org/docs/unstable/misc/arm/device-tree/booting.txt
-- 
Julien Grall

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

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

[Jan Beulich]

>>> On 07.03.17 at 15:41, <julien.grall@arm.com> wrote:
> Hi Jan,
> 
> On 03/07/2017 02:18 PM, Jan Beulich wrote:
>>>>> On 07.03.17 at 14:48, <julien.grall@arm.com> wrote:
>>> On 03/07/2017 12:52 PM, Jan Beulich wrote:
>>>>>>> On 07.03.17 at 12:21, <blackskygg@gmail.com> wrote:
>>>>> 2017-03-07 17:36 GMT+08:00 Jan Beulich <JBeulich@suse.com>:
>>>>>>>>> On 07.03.17 at 09:34, <blackskygg@gmail.com> wrote:
>>>>>>> +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?
>>>
>>> DOM0 and Xen command line are passed separately through either Device
>>> Tree or for UEFI xen configuration file (see -cfg=...).
>>>
>>> So I am not sure to understand what would be the benefits to handle DOM0
>>> parameters after -- on Xen command line.
>>
>> So you have no case of a boot loader which allows you to type in
>> extra options on just a single line? On x86 the feature had originally
>> been added because old grub didn't have a separate line for Dom0
>> options in its graphical menu. Nowadays the functionality is handy
>> namely when starting xen.efi from the EFI shell (where again you
>> obviously only have a single command line), but quite likely this may
>> also be of use with grub's chain loading model (which I simply don't
>> use very often, so I'm not finally sure on that one).
> 
> My knowledge is quite limited on boot process for x86. How do you pass 
> the kernel/initrd/xsm blob on UEFI? Can you do it from the command line 
> or are you using the -cfg=... and specify it in a file?

Only the latter.

> On ARM we have two way to boot Xen:
> 	- Using UEFI bootloader with either Device-Tree or ACPI
> 	- Using non-UEFI bootloader with Device-Tree only
> 
> In the case of UEFI bootloader, we are using the xen configuration file 
> to describe the modules (e.g kernel, initramfs, XSM) and the both xen 
> and DOM0 command line.
> 
> For non-UEFI bootloader, we have designed the boot protocol based on the 
> device-tree and will allows you to specify both xen and DOM0 and all the 
> modules (see [1]). The bootloader needs to be able to modify the 
> device-tree (such via a shell like on U-boot) or the user needs to 
> modify the device-tree before hand.

All fine, but this doesn't tell me what interactive change options a
user has _after_ having set up the config file (or alike), while the
system is booting.

> To answer your question, a bootloader supporting only a single line 
> would still need to be modified to provide the various modules. At that 
> stage, adding DOM0 command line is very easy.

But aiui that's again needed to be done _before_ the system is
rebooted.

> Now, if you tell me that all the modules can be described on the UEFI 
> command line, then I think handling '--' would be nice. If not, I don't 
> think it is worth it.

As per above I'm getting the impression that we're talking of
different scenarios - I don't think I've yet understood what options
of adding to or editing of any of the command lines you have on
ARM _during_ the boot process of a system.

Jan


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

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

[Julien Grall]

Hi Jan,

On 03/07/2017 02:52 PM, Jan Beulich wrote:
>>>> On 07.03.17 at 15:41, <julien.grall@arm.com> wrote:
>> Hi Jan,
>>
>> On 03/07/2017 02:18 PM, Jan Beulich wrote:
>>>>>> On 07.03.17 at 14:48, <julien.grall@arm.com> wrote:
>>>> On 03/07/2017 12:52 PM, Jan Beulich wrote:
>>>>>>>> On 07.03.17 at 12:21, <blackskygg@gmail.com> wrote:
>>>>>> 2017-03-07 17:36 GMT+08:00 Jan Beulich <JBeulich@suse.com>:
>>>>>>>>>> On 07.03.17 at 09:34, <blackskygg@gmail.com> wrote:
>>>>>>>> +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?
>>>>
>>>> DOM0 and Xen command line are passed separately through either Device
>>>> Tree or for UEFI xen configuration file (see -cfg=...).
>>>>
>>>> So I am not sure to understand what would be the benefits to handle DOM0
>>>> parameters after -- on Xen command line.
>>>
>>> So you have no case of a boot loader which allows you to type in
>>> extra options on just a single line? On x86 the feature had originally
>>> been added because old grub didn't have a separate line for Dom0
>>> options in its graphical menu. Nowadays the functionality is handy
>>> namely when starting xen.efi from the EFI shell (where again you
>>> obviously only have a single command line), but quite likely this may
>>> also be of use with grub's chain loading model (which I simply don't
>>> use very often, so I'm not finally sure on that one).
>>
>> My knowledge is quite limited on boot process for x86. How do you pass
>> the kernel/initrd/xsm blob on UEFI? Can you do it from the command line
>> or are you using the -cfg=... and specify it in a file?
>
> Only the latter.
>
>> On ARM we have two way to boot Xen:
>> 	- Using UEFI bootloader with either Device-Tree or ACPI
>> 	- Using non-UEFI bootloader with Device-Tree only
>>
>> In the case of UEFI bootloader, we are using the xen configuration file
>> to describe the modules (e.g kernel, initramfs, XSM) and the both xen
>> and DOM0 command line.
>>
>> For non-UEFI bootloader, we have designed the boot protocol based on the
>> device-tree and will allows you to specify both xen and DOM0 and all the
>> modules (see [1]). The bootloader needs to be able to modify the
>> device-tree (such via a shell like on U-boot) or the user needs to
>> modify the device-tree before hand.
>
> All fine, but this doesn't tell me what interactive change options a
> user has _after_ having set up the config file (or alike), while the
> system is booting.

Here some concrete examples. The major bootloaders on ARM today are:
	* UEFI
	* U-boot
	* GRUB

I will leave UEFI aside as people will usually chainload to GRUB and 
then boot whatever they want.

In both GRUB and U-boot a user will be able to modify the command line 
from the bootloader shell.

Cheers,

-- 
Julien Grall

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

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

[Stefano Stabellini]

On Tue, 7 Mar 2017, Julien Grall wrote:
> Hi Jan,
> 
> On 03/07/2017 02:52 PM, Jan Beulich wrote:
> > > > > On 07.03.17 at 15:41, <julien.grall@arm.com> wrote:
> > > Hi Jan,
> > > 
> > > On 03/07/2017 02:18 PM, Jan Beulich wrote:
> > > > > > > On 07.03.17 at 14:48, <julien.grall@arm.com> wrote:
> > > > > On 03/07/2017 12:52 PM, Jan Beulich wrote:
> > > > > > > > > On 07.03.17 at 12:21, <blackskygg@gmail.com> wrote:
> > > > > > > 2017-03-07 17:36 GMT+08:00 Jan Beulich <JBeulich@suse.com>:
> > > > > > > > > > > On 07.03.17 at 09:34, <blackskygg@gmail.com> wrote:
> > > > > > > > > +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?
> > > > > 
> > > > > DOM0 and Xen command line are passed separately through either Device
> > > > > Tree or for UEFI xen configuration file (see -cfg=...).
> > > > > 
> > > > > So I am not sure to understand what would be the benefits to handle
> > > > > DOM0
> > > > > parameters after -- on Xen command line.
> > > > 
> > > > So you have no case of a boot loader which allows you to type in
> > > > extra options on just a single line? On x86 the feature had originally
> > > > been added because old grub didn't have a separate line for Dom0
> > > > options in its graphical menu. Nowadays the functionality is handy
> > > > namely when starting xen.efi from the EFI shell (where again you
> > > > obviously only have a single command line), but quite likely this may
> > > > also be of use with grub's chain loading model (which I simply don't
> > > > use very often, so I'm not finally sure on that one).
> > > 
> > > My knowledge is quite limited on boot process for x86. How do you pass
> > > the kernel/initrd/xsm blob on UEFI? Can you do it from the command line
> > > or are you using the -cfg=... and specify it in a file?
> > 
> > Only the latter.
> > 
> > > On ARM we have two way to boot Xen:
> > > 	- Using UEFI bootloader with either Device-Tree or ACPI
> > > 	- Using non-UEFI bootloader with Device-Tree only
> > > 
> > > In the case of UEFI bootloader, we are using the xen configuration file
> > > to describe the modules (e.g kernel, initramfs, XSM) and the both xen
> > > and DOM0 command line.
> > > 
> > > For non-UEFI bootloader, we have designed the boot protocol based on the
> > > device-tree and will allows you to specify both xen and DOM0 and all the
> > > modules (see [1]). The bootloader needs to be able to modify the
> > > device-tree (such via a shell like on U-boot) or the user needs to
> > > modify the device-tree before hand.
> > 
> > All fine, but this doesn't tell me what interactive change options a
> > user has _after_ having set up the config file (or alike), while the
> > system is booting.
> 
> Here some concrete examples. The major bootloaders on ARM today are:
> 	* UEFI
> 	* U-boot
> 	* GRUB
> 
> I will leave UEFI aside as people will usually chainload to GRUB and then boot
> whatever they want.
> 
> In both GRUB and U-boot a user will be able to modify the command line from
> the bootloader shell.

Given that upstream GRUB doesn't yet support booting Xen on ARM (without
any additional patches), I think that the ability to completely change
the command line from the EFI shell would be useful. Besides, although
it is not mandatory, I think it would be best not to unnecessarily
diverge from x86 in terms of EFI booting.

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

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

[Julien Grall]

Hi Stefano,

On 03/07/2017 07:54 PM, Stefano Stabellini wrote:
> On Tue, 7 Mar 2017, Julien Grall wrote:
> Given that upstream GRUB doesn't yet support booting Xen on ARM (without
> any additional patches), I think that the ability to completely change
> the command line from the EFI shell would be useful. Besides, although
> it is not mandatory, I think it would be best not to unnecessarily
> diverge from x86 in terms of EFI booting.

I don't consider x86 solution as granted for ARM, and I would have 
thought it was the same on your side given the change you requested for 
dom0_mem recently.

I still don't see a reason to override the command line option as 
usually the issue will not be because of the command line but the kernel 
itself. At least this is my experience on ARM so far.

Anyway, I am not going to argue on this. If you think it should be done, 
then it should be in a separate patch.

Cheers,

-- 
Julien Grall

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

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

[Stefano Stabellini]

On Tue, 7 Mar 2017, Julien Grall wrote:
> Hi Stefano,
> 
> On 03/07/2017 07:54 PM, Stefano Stabellini wrote:
> > On Tue, 7 Mar 2017, Julien Grall wrote:
> > Given that upstream GRUB doesn't yet support booting Xen on ARM (without
> > any additional patches), I think that the ability to completely change
> > the command line from the EFI shell would be useful. Besides, although
> > it is not mandatory, I think it would be best not to unnecessarily
> > diverge from x86 in terms of EFI booting.
> 
> I don't consider x86 solution as granted for ARM, and I would have thought it
> was the same on your side given the change you requested for dom0_mem
> recently.

I agree. I am only saying that all things being equal, we might as well
keep compatibility. If nothing else, it will be easier to write docs.
The dom0_mem case is an example where things are not equal between x86
and arm, but the parameter still works similarly across the two archs.


> I still don't see a reason to override the command line option as usually the
> issue will not be because of the command line but the kernel itself. At least
> this is my experience on ARM so far.

I think it can be useful, even just for tests, especially given that
grub is still unable to boot Xen.


> Anyway, I am not going to argue on this. If you think it should be done, then
> it should be in a separate patch.

That would be best.

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

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

[Zhongze Liu]

So, taking all of the conversations above into consideration, the
following changes
should be done to this patch:

1. According to Andrew and Jan's suggestions, I'll remove the
CMDLINE_BOOL option,
    and deal with CMDLINE without the #ifdef-ary's.

2. Make the option CMDLINE_OVERRIDE depends on EXPERT.

3. Move the duplicated code in the various setup.c's into
common/kernel.c; Introduce a wrapper
    to common/kernel.c:cmdline_parse(), and let that wrapper
automatically deal with CONFIG_CMDLINE,
    so the start_xen()'s won't be bothered to do the concatenation by
themselves.

4. As for the different behavior between arm and x86 on handling the
dom0 options after " -- " in the
    command line, I will left this difference as untouched, coz
whether to add this feature to arm or to remove
    this feature from x86 is beyond the scope of this patch.

But there's one thing that I'm not quite sure. Since currently there
isn't any cumulative options in
Xen, I just can't deal with them - Jan?

2017-03-08 5:37 GMT+08:00 Stefano Stabellini <sstabellini@kernel.org>:
> On Tue, 7 Mar 2017, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 03/07/2017 07:54 PM, Stefano Stabellini wrote:
>> > On Tue, 7 Mar 2017, Julien Grall wrote:
>> > Given that upstream GRUB doesn't yet support booting Xen on ARM (without
>> > any additional patches), I think that the ability to completely change
>> > the command line from the EFI shell would be useful. Besides, although
>> > it is not mandatory, I think it would be best not to unnecessarily
>> > diverge from x86 in terms of EFI booting.
>>
>> I don't consider x86 solution as granted for ARM, and I would have thought it
>> was the same on your side given the change you requested for dom0_mem
>> recently.
>
> I agree. I am only saying that all things being equal, we might as well
> keep compatibility. If nothing else, it will be easier to write docs.
> The dom0_mem case is an example where things are not equal between x86
> and arm, but the parameter still works similarly across the two archs.
>
>
>> I still don't see a reason to override the command line option as usually the
>> issue will not be because of the command line but the kernel itself. At least
>> this is my experience on ARM so far.
>
> I think it can be useful, even just for tests, especially given that
> grub is still unable to boot Xen.
>
>
>> Anyway, I am not going to argue on this. If you think it should be done, then
>> it should be in a separate patch.
>
> That would be best.

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

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

[Jan Beulich]

>>> On 07.03.17 at 18:48, <julien.grall@arm.com> wrote:
> Here some concrete examples. The major bootloaders on ARM today are:
> 	* UEFI
> 	* U-boot
> 	* GRUB
> 
> I will leave UEFI aside as people will usually chainload to GRUB and 
> then boot whatever they want.
> 
> In both GRUB and U-boot a user will be able to modify the command line 
> from the bootloader shell.

Thanks. So I think the takeaway is that ARM doesn't strictly need
the Dom0 part of the handling, but then again if this was common
code it also shouldn't hurt.

Jan


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

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

[Jan Beulich]

>>> On 08.03.17 at 08:16, <blackskygg@gmail.com> wrote:
> 4. As for the different behavior between arm and x86 on handling the
> dom0 options after " -- " in the
>     command line, I will left this difference as untouched, coz
> whether to add this feature to arm or to remove
>     this feature from x86 is beyond the scope of this patch.

Since you want to move the logic to common code, the result
would likely end up better if there was no arch-dependent
behavior.

> But there's one thing that I'm not quite sure. Since currently there
> isn't any cumulative options in
> Xen, I just can't deal with them - Jan?

Well, dealing with them simply means writing the config option
help text accordingly (i.e. avoid explicitly ruling out that case).

Jan


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

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

[Julien Grall]

Hi Jan,

On 08/03/17 08:03, Jan Beulich wrote:
>>>> On 07.03.17 at 18:48, <julien.grall@arm.com> wrote:
>> Here some concrete examples. The major bootloaders on ARM today are:
>> 	* UEFI
>> 	* U-boot
>> 	* GRUB
>>
>> I will leave UEFI aside as people will usually chainload to GRUB and
>> then boot whatever they want.
>>
>> In both GRUB and U-boot a user will be able to modify the command line
>> from the bootloader shell.
>
> Thanks. So I think the takeaway is that ARM doesn't strictly need
> the Dom0 part of the handling, but then again if this was common
> code it also shouldn't hurt.

Correct. As long as the he expected behavior (e.g how they options are 
combined) is fully documented.

Note that I was not able to find any documentation about this in 
xen-commandline.markdown.

Cheers,

-- 
Julien Grall

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