| Message ID | 20170309031321.19355-1-blackskygg@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 09.03.17 at 04:13, <blackskygg@gmail.com> 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
2017-03-10 23:03 GMT+08:00 Jan Beulich <JBeulich@suse.com>: >>>> On 09.03.17 at 04:13, <blackskygg@gmail.com> 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 >
>>> On 10.03.17 at 18:36, <blackskygg@gmail.com> wrote: > 2017-03-10 23:03 GMT+08:00 Jan Beulich <JBeulich@suse.com>: >>>>> On 09.03.17 at 04:13, <blackskygg@gmail.com> 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
>2017-03-13 19:52 GMT+08:00 Jan Beulich <JBeulich@suse.com>: >>>> @@ -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.
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 = cmdline_cook(cmdline, loader); safe_strcpy(dom0_cmdline, cmdline); - if ( kextra != NULL ) + if ( strlen(kextra) ) /* kextra always includes exactly one leading space. */ safe_strcat(dom0_cmdline, kextra); diff --git a/xen/common/Kconfig b/xen/common/Kconfig index f2ecbc43d6..d7288abdcc 100644 --- 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" + ---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 diff --git a/xen/common/kernel.c b/xen/common/kernel.c index 4b87c60845..77544be948 100644 --- a/xen/common/kernel.c +++ b/xen/common/kernel.c @@ -22,7 +22,7 @@ enum system_state system_state = SYS_STATE_early_boot; -xen_commandline_t saved_cmdline; +xen_commandline_t saved_cmdline = CONFIG_CMDLINE; static void __init assign_integer_param( const struct kernel_param *param, uint64_t val) @@ -46,17 +46,77 @@ static void __init assign_integer_param( } } -void __init cmdline_parse(const char *cmdline) +/* + * Extracts dom0 options from the @cmdline. + * 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. + * 3. Append dom0's options to kextra. + */ +static void __init gather_dom0_options( + char *cmdline, char *kextra, size_t kextra_len) +{ + char *p; + + if ( (p = strstr(cmdline, " -- ")) != NULL ) + { + *p = '\0'; + p += 3; + while ( p[1] == ' ' ) + p++; + + strlcat(kextra, p, kextra_len); + } +} + +/** + * cmdline_parse - parses the command line options. + * + * @cmdline: the command line to be parsed. + * @kextra: if not NULL, this will be filled with the optional dom0 options. + * (some architecture allows dom0 options to appear after " -- " in + * the cmdline); if set to NULL, the function won't seek for dom0 + * options. + * @kextra_len: length of @kextra, ignored if @kextra is NULL. + * + * If CONFIG_CMDLINE is set, @cmdline will be appended to CONFIG_CMDLINE + * to form the complete cmdline before parsing, and the optional dom0 options + * will also be concatenated in the same order before we put them in @kextra. + * But if CONFIG_CMDLINE_OVERRIDE is set to y, the whole bootloader command + * line will be ignored. + * The fucntion retruns the complete command line with the optional dom0 + * options stripped out. + */ +const char* __init cmdline_parse( + const char *cmdline, char *kextra, size_t kextra_len) { char opt[100], *optval, *optkey, *q; - const char *p = cmdline; + const char *p = saved_cmdline; const struct kernel_param *param; int bool_assert; - if ( cmdline == NULL ) - return; + if ( kextra != NULL ) + kextra[0] = '\0'; - safe_strcpy(saved_cmdline, cmdline); + if ( strlen(saved_cmdline) ) + { + if ( kextra != NULL ) + gather_dom0_options(saved_cmdline, kextra, kextra_len); + safe_strcat(saved_cmdline, " "); + } + +#ifndef CONFIG_CMDLINE_OVERRIDE + if ( cmdline != NULL ) + { + safe_strcat(saved_cmdline, cmdline); + if ( kextra != NULL ) + gather_dom0_options(saved_cmdline, kextra, kextra_len); + } +#endif + + if ( !strlen(saved_cmdline) ) + /* Nothing to parse. */ + return saved_cmdline; for ( ; ; ) { @@ -145,6 +205,8 @@ void __init cmdline_parse(const char *cmdline) } } } + + return saved_cmdline; } int __init parse_bool(const char *s) diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h index 995a85a7db..33e5fbfe20 100644 --- a/xen/include/xen/lib.h +++ b/xen/include/xen/lib.h @@ -70,7 +70,7 @@ struct domain; -void cmdline_parse(const char *cmdline); +const char *cmdline_parse(const char *cmdline, char *kextra, size_t kextra_len); int parse_bool(const char *s); /*#define DEBUG_TRACE_DUMP*/
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 <blackskygg@gmail.com> --- 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(-)