Message ID | 20170307083405.5822-1-blackskygg@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
>>> 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
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
>>> 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
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,
>>> 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
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
>>> 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
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,
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.
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,
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.
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.
>>> 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
>>> 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
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,
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
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(-)