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

2017-03-13 Thread Zhongze Liu 
>2017-03-13 19:52 GMT+08:00 Jan Beulich :
 @@ -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?
>
> Having thought about it again, I'm actually no longer of the opinion
> that hard coding a Dom0 command line (portion) in the hypervisor
> is sensible at all. With that eliminated, this discussion aspect is moot
> afaict.
>

That's it! I was just about to ask whether it's worth doing so. After
some thoughts,
I think it makes no sense to allow dom0 options to appear in CONFIG_CMDLINE,
either. What's more, if I don't need to handle it, things become much easier.

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

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

2017-03-13 Thread Jan Beulich 
>>> On 10.03.17 at 18:36,  wrote:
> 2017-03-10 23:03 GMT+08:00 Jan Beulich :
> On 09.03.17 at 04:13,  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).

I don't see why recursion is not suitable. All you need is a proper
guard to avoid recursing more than you really want.

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

Having thought about it again, I'm actually no longer of the opinion
that hard coding a Dom0 command line (portion) in the hypervisor
is sensible at all. With that eliminated, this discussion aspect is moot
afaict.

Jan


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

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

2017-03-10 Thread Zhongze Liu 
2017-03-10 23:03 GMT+08:00 Jan Beulich :
 On 09.03.17 at 04:13,  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@lists.xen.org
https://lists.xen.org/xen-devel

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

2017-03-10 Thread Jan Beulich 
>>> On 09.03.17 at 04:13,  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.

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

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

Jan


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

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

2017-03-08 Thread Zhongze Liu 
Added 2 new config entries in common/Kconfig:
CMDLINE and CMDLINE_OVERRIDE
Modified the interface common/kernel.c:cmdline_parse().

The 2 new entries allow an embedded command line to be compiled in the
hypervisor.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)

If CMDLINE is set, the cmdline_parse() routine will append the bootloader
command line to this string, forming the complete command line
before parsing. This order of concatenation implies that if any non-cumulative
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.

Since some architectures (e.g. x86) allow optional dom0 options to appear after
" -- " in the command line. cmdline_parse() was modified to handle these
dom0 options if the caller request it to do so, and thus architectures
supporting this feature don't need to handle them in their setup.c's anymore.

Signed-off-by: Zhongze Liu 
---
Changed since v3:
  * Remove the CMDLINE_BOOL option.
  * Make the option CMDLINE_OVERRIDE depend on EXPERT = "y".
  * Move the CMDLINE-related code from various /xen/$(ARCH)/setup.c's to
common/kernel.c:cmdline_parse().
  * Dealing with the optional dom0 options in cmdline_parse().
  * Remove some pointless initializers.
  * Remove unnecessary #ifdef-ary's
---
 xen/arch/arm/setup.c  |  6 ++---
 xen/arch/x86/setup.c  | 23 +---
 xen/common/Kconfig| 24 +
 xen/common/kernel.c   | 74 ++-
 xen/include/xen/lib.h |  2 +-
 5 files changed, 103 insertions(+), 26 deletions(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 92a2de6b70..a5ecdbb54a 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -705,7 +705,7 @@ 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, *complete_cmdline;
 struct bootmodule *xen_bootmodule;
 struct domain *dom0;
 struct xen_arch_domainconfig config;
@@ -730,8 +730,8 @@ void __init start_xen(unsigned long boot_phys_offset,
 fdt_size = boot_fdt_info(device_tree_flattened, fdt_paddr);
 
 cmdline = boot_fdt_cmdline(device_tree_flattened);
-printk("Command line: %s\n", cmdline);
-cmdline_parse(cmdline);
+complete_cmdline = cmdline_parse(cmdline, NULL, 0);
+printk("Command line: %s\n", complete_cmdline);
 
 /* Register Xen's load address as a boot module. */
 xen_bootmodule = add_boot_module(BOOTMOD_XEN,
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index dab67d5491..dc65d8f389 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -639,7 +639,9 @@ static char * __init cmdline_cook(char *p, const char 
*loader_name)
 void __init noreturn __start_xen(unsigned long mbi_p)
 {
 char *memmap_type = NULL;
-char *cmdline, *kextra, *loader;
+char *cmdline, *loader;
+const char *complete_cmdline;
+static char __initdata kextra[MAX_GUEST_CMDLINE];
 unsigned int initrdidx, domcr_flags = DOMCRF_s3_integrity;
 multiboot_info_t *mbi = __va(mbi_p);
 module_t *mod = (module_t *)__va(mbi->mods_addr);
@@ -679,18 +681,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 cmdline = cmdline_cook((mbi->flags & MBI_CMDLINE) ?
__va(mbi->cmdline) : NULL,
loader);
-if ( (kextra = strstr(cmdline, " -- ")) != 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++;
-}
-cmdline_parse(cmdline);
+complete_cmdline = cmdline_parse(cmdline, kextra, sizeof(kextra));
 
 /* Must be after command line argument parsing and before
  * allocing any xenheap structures wanted in lower memory. */
@@ -713,7 +704,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 
 printk("Bootloader: %s\n", loader);
 
-printk("Command line: %s\n", cmdline);
+printk("Command line: %s\n", complete_cmdline);
 
 printk("Video information:\n");
 
@@ -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) )
 {
 static char __initdata dom0_cmdline[MAX_GUEST_CMDLINE];
 
 cmdline =