| Message ID | 1411080607-32365-7-git-send-email-roy.franz@linaro.org |
|---|---|
| State | New |
| Headers | show |
>>> On 19.09.14 at 00:49, <roy.franz@linaro.org> wrote: > Add arch function for processing the Xen commandline and > updating internal structures. > > Signed-off-by: Roy Franz <roy.franz@linaro.org> With two small requests for adjustments below Acked-by: Jan Beulich <jbeulich@suse.com> > @@ -256,7 +257,8 @@ static void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode) > } > > static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv, > - CHAR16 *cmdline, UINTN cmdsize) > + CHAR16 *cmdline, UINTN cmdsize, > + CHAR16 **options) I think this would better be named "dom0_options" or "dom0_opts" or some such (also in the caller). > @@ -701,14 +701,14 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) > dir_handle = get_parent_handle(loaded_image, &file_name); > > argc = get_argv(0, NULL, loaded_image->LoadOptions, > - loaded_image->LoadOptionsSize); > + loaded_image->LoadOptionsSize, &options); I don't see why you don't pass NULL here, the more that you add a respective check to get_argv(). Jan
On Mon, Sep 22, 2014 at 5:17 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 19.09.14 at 00:49, <roy.franz@linaro.org> wrote: >> Add arch function for processing the Xen commandline and >> updating internal structures. >> >> Signed-off-by: Roy Franz <roy.franz@linaro.org> > > With two small requests for adjustments below > Acked-by: Jan Beulich <jbeulich@suse.com> > >> @@ -256,7 +257,8 @@ static void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode) >> } >> >> static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv, >> - CHAR16 *cmdline, UINTN cmdsize) >> + CHAR16 *cmdline, UINTN cmdsize, >> + CHAR16 **options) > > I think this would better be named "dom0_options" or "dom0_opts" or > some such (also in the caller). I think xen_options would be better, or else I'm really confused :) This string is combined with the Xen options from the config file into mbi.cmdline. Dom0 arguments come from the "kernel" line in the configuration tile. > >> @@ -701,14 +701,14 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) >> dir_handle = get_parent_handle(loaded_image, &file_name); >> >> argc = get_argv(0, NULL, loaded_image->LoadOptions, >> - loaded_image->LoadOptionsSize); >> + loaded_image->LoadOptionsSize, &options); > > I don't see why you don't pass NULL here, the more that you add > a respective check to get_argv(). I don't either - I will change this. > > Jan >
>>> On 23.09.14 at 03:40, <roy.franz@linaro.org> wrote: > On Mon, Sep 22, 2014 at 5:17 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 19.09.14 at 00:49, <roy.franz@linaro.org> wrote: >>> @@ -256,7 +257,8 @@ static void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode) >>> } >>> >>> static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv, >>> - CHAR16 *cmdline, UINTN cmdsize) >>> + CHAR16 *cmdline, UINTN cmdsize, >>> + CHAR16 **options) >> >> I think this would better be named "dom0_options" or "dom0_opts" or >> some such (also in the caller). > > I think xen_options would be better, or else I'm really confused :) > This string is combined with the Xen options from the > config file into mbi.cmdline. Dom0 arguments come from the "kernel" > line in the configuration tile. Indeed - I mixed things up: This is the first "--" separator, the Dom0 options come after the second. In which case staying with just "options" is quite fine. Jan
On Tue, Sep 23, 2014 at 5:25 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 23.09.14 at 03:40, <roy.franz@linaro.org> wrote: >> On Mon, Sep 22, 2014 at 5:17 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>> On 19.09.14 at 00:49, <roy.franz@linaro.org> wrote: >>>> @@ -256,7 +257,8 @@ static void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode) >>>> } >>>> >>>> static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv, >>>> - CHAR16 *cmdline, UINTN cmdsize) >>>> + CHAR16 *cmdline, UINTN cmdsize, >>>> + CHAR16 **options) >>> >>> I think this would better be named "dom0_options" or "dom0_opts" or >>> some such (also in the caller). >> >> I think xen_options would be better, or else I'm really confused :) >> This string is combined with the Xen options from the >> config file into mbi.cmdline. Dom0 arguments come from the "kernel" >> line in the configuration tile. > > Indeed - I mixed things up: This is the first "--" separator, the Dom0 > options come after the second. In which case staying with just > "options" is quite fine. > > Jan > Is this second "--" handled by XEN itself? I don't see any handling of that in the EFI boot code. The only thing that goes into the multiboot .string field for the dom0 kernel is from the config file. Roy
>>> On 24.09.14 at 02:09, <roy.franz@linaro.org> wrote: > On Tue, Sep 23, 2014 at 5:25 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 23.09.14 at 03:40, <roy.franz@linaro.org> wrote: >>> On Mon, Sep 22, 2014 at 5:17 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>> On 19.09.14 at 00:49, <roy.franz@linaro.org> wrote: >>>>> @@ -256,7 +257,8 @@ static void __init PrintErrMesg(const CHAR16 *mesg, > EFI_STATUS ErrCode) >>>>> } >>>>> >>>>> static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv, >>>>> - CHAR16 *cmdline, UINTN cmdsize) >>>>> + CHAR16 *cmdline, UINTN cmdsize, >>>>> + CHAR16 **options) >>>> >>>> I think this would better be named "dom0_options" or "dom0_opts" or >>>> some such (also in the caller). >>> >>> I think xen_options would be better, or else I'm really confused :) >>> This string is combined with the Xen options from the >>> config file into mbi.cmdline. Dom0 arguments come from the "kernel" >>> line in the configuration tile. >> >> Indeed - I mixed things up: This is the first "--" separator, the Dom0 >> options come after the second. In which case staying with just >> "options" is quite fine. > > Is this second "--" handled by XEN itself? I don't see any handling > of that in the EFI boot code. The only thing that goes > into the multiboot .string field for the dom0 kernel is from the config > file. Yes, that's being handled in __start_xen() (search for "kextra" if you want to see where exactly). Jan
On Wed, 2014-09-24 at 09:13 +0100, Jan Beulich wrote: > >>> On 24.09.14 at 02:09, <roy.franz@linaro.org> wrote: > > On Tue, Sep 23, 2014 at 5:25 AM, Jan Beulich <JBeulich@suse.com> wrote: > >>>>> On 23.09.14 at 03:40, <roy.franz@linaro.org> wrote: > >>> On Mon, Sep 22, 2014 at 5:17 AM, Jan Beulich <JBeulich@suse.com> wrote: > >>>>>>> On 19.09.14 at 00:49, <roy.franz@linaro.org> wrote: > >>>>> @@ -256,7 +257,8 @@ static void __init PrintErrMesg(const CHAR16 *mesg, > > EFI_STATUS ErrCode) > >>>>> } > >>>>> > >>>>> static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv, > >>>>> - CHAR16 *cmdline, UINTN cmdsize) > >>>>> + CHAR16 *cmdline, UINTN cmdsize, > >>>>> + CHAR16 **options) > >>>> > >>>> I think this would better be named "dom0_options" or "dom0_opts" or > >>>> some such (also in the caller). > >>> > >>> I think xen_options would be better, or else I'm really confused :) > >>> This string is combined with the Xen options from the > >>> config file into mbi.cmdline. Dom0 arguments come from the "kernel" > >>> line in the configuration tile. > >> > >> Indeed - I mixed things up: This is the first "--" separator, the Dom0 > >> options come after the second. In which case staying with just > >> "options" is quite fine. > > > > Is this second "--" handled by XEN itself? I don't see any handling > > of that in the EFI boot code. The only thing that goes > > into the multiboot .string field for the dom0 kernel is from the config > > file. > > Yes, that's being handled in __start_xen() (search for "kextra" if > you want to see where exactly). I'd never heard of this behaviour before, and it is (currently at least) x86 specific. I don't know if this is relevant to the discusion here though, probably not? Ian.
>>> On 24.09.14 at 11:33, <Ian.Campbell@citrix.com> wrote: > On Wed, 2014-09-24 at 09:13 +0100, Jan Beulich wrote: >> >>> On 24.09.14 at 02:09, <roy.franz@linaro.org> wrote: >> > Is this second "--" handled by XEN itself? I don't see any handling >> > of that in the EFI boot code. The only thing that goes >> > into the multiboot .string field for the dom0 kernel is from the config >> > file. >> >> Yes, that's being handled in __start_xen() (search for "kextra" if >> you want to see where exactly). > > I'd never heard of this behaviour before, and it is (currently at least) > x86 specific. Yeah, we needed this a long while ago for the case where the GrUB GUI shows only a single line for entering extra options, which then all get passed to the hypervisor making it impossible to add extra Dom0 kernel options this same way. > I don't know if this is relevant to the discusion here though, probably > not? Definitely not. I just didn't want to leave Roy's question unanswered. Jan
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index 58e3cbc..2f8dade 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -58,6 +58,7 @@ static char *get_value(const struct file *cfg, const char *section, const char *item); static void split_value(char *s); static CHAR16 *s2w(union string *str); +static char *w2s(const union string *str); static bool_t read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, struct file *file); @@ -256,7 +257,8 @@ static void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode) } static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv, - CHAR16 *cmdline, UINTN cmdsize) + CHAR16 *cmdline, UINTN cmdsize, + CHAR16 **options) { CHAR16 *ptr = (CHAR16 *)(argv + argc + 1), *prev = NULL; bool_t prev_sep = TRUE; @@ -282,10 +284,8 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv, ++argc; else if ( prev && wstrcmp(prev, L"--") == 0 ) { - union string rest = { .w = cmdline }; - - --argv; - place_string(&mbi.cmdline, w2s(&rest)); + if ( options ) + *options = cmdline; break; } else @@ -660,7 +660,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) EFI_LOADED_IMAGE *loaded_image; EFI_STATUS status; unsigned int i, argc; - CHAR16 **argv, *file_name, *cfg_file_name = NULL; + CHAR16 **argv, *file_name, *cfg_file_name = NULL, *options = NULL; UINTN cols, rows, depth, size, info_size, gop_mode = ~0; EFI_HANDLE *handles = NULL; EFI_SHIM_LOCK_PROTOCOL *shim_lock; @@ -701,14 +701,14 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) dir_handle = get_parent_handle(loaded_image, &file_name); argc = get_argv(0, NULL, loaded_image->LoadOptions, - loaded_image->LoadOptionsSize); + loaded_image->LoadOptionsSize, &options); if ( argc > 0 && efi_bs->AllocatePool(EfiLoaderData, (argc + 1) * sizeof(*argv) + loaded_image->LoadOptionsSize, (void **)&argv) == EFI_SUCCESS ) get_argv(argc, argv, loaded_image->LoadOptions, - loaded_image->LoadOptionsSize); + loaded_image->LoadOptionsSize, &options); else argc = 0; for ( i = 1; i < argc; ++i ) @@ -878,17 +878,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) } name.s = get_value(&cfg, section.s, "options"); - if ( name.s ) - place_string(&mbi.cmdline, name.s); - /* Insert image name last, as it gets prefixed to the other options. */ - if ( argc ) - { - name.w = *argv; - w2s(&name); - } - else - name.s = "xen"; - place_string(&mbi.cmdline, name.s); + efi_arch_handle_cmdline(argc ? *argv : NULL, options, name.s); cols = rows = depth = 0; if ( !base_video ) @@ -955,15 +945,6 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) } } - if ( mbi.cmdline ) - mbi.flags |= MBI_CMDLINE; - /* - * These must not be initialized statically, since the value must - * not get relocated when processing base relocations below. - */ - mbi.boot_loader_name = (long)"EFI"; - mbi.mods_addr = (long)mb_modules; - /* Collect EDD info. */ BUILD_BUG_ON(offsetof(struct edd_info, edd_device_params) != EDDEXTSIZE); BUILD_BUG_ON(sizeof(struct edd_device_params) != EDDPARMSIZE); diff --git a/xen/include/asm-x86/efi-boot.h b/xen/include/asm-x86/efi-boot.h index 93498bb..6e9376a 100644 --- a/xen/include/asm-x86/efi-boot.h +++ b/xen/include/asm-x86/efi-boot.h @@ -279,3 +279,37 @@ static void __init efi_arch_cfg_file_late(EFI_FILE_HANDLE dir_handle, char *sect efi_bs->FreePool(name.w); } } + +static void __init efi_arch_handle_cmdline(CHAR16 *image_name, + CHAR16 *cmdline_options, + char *cfgfile_options) +{ + union string name; + + if ( cmdline_options ) + { + name.w = cmdline_options; + w2s(&name); + place_string(&mbi.cmdline, name.s); + } + if ( cfgfile_options ) + place_string(&mbi.cmdline, cfgfile_options); + /* Insert image name last, as it gets prefixed to the other options. */ + if ( image_name ) + { + name.w = image_name; + w2s(&name); + } + else + name.s = "xen"; + place_string(&mbi.cmdline, name.s); + + if ( mbi.cmdline ) + mbi.flags |= MBI_CMDLINE; + /* + * These must not be initialized statically, since the value must + * not get relocated when processing base relocations later. + */ + mbi.boot_loader_name = (long)"EFI"; + mbi.mods_addr = (long)mb_modules; +}
Add arch function for processing the Xen commandline and updating internal structures. Signed-off-by: Roy Franz <roy.franz@linaro.org> --- xen/common/efi/boot.c | 37 +++++++++---------------------------- xen/include/asm-x86/efi-boot.h | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 28 deletions(-)