| Message ID | 1411609352-24549-12-git-send-email-roy.franz@linaro.org |
|---|---|
| State | New |
| Headers | show |
>>> On 25.09.14 at 03:42, <roy.franz@linaro.org> wrote: > + /* > + * Make a copy, as conversion is destructive, and caller still wants > + * wide string available after this call returns. > + */ > + if ( efi_bs->AllocatePool(EfiLoaderData, (wstrlen(name) + 1) * sizeof(*name), > + &ptr) != EFI_SUCCESS ) > + blexit(L"ERROR Unable to allocate string buffer"); Kind of disappointing: You said you'd drop these ERROR prefixes, but this is the second one I come across. I'm ditching them in preparation for committing. > + > + local_name.w = ptr; > + wstrcpy(local_name.w, name); > + w2s(&local_name); > + > + place_string(&mb_modules[mbi.mods_count].string, options); > + place_string(&mb_modules[mbi.mods_count].string, ""); Hmm, this one's still here, and I don't recall having seen an explanation for it. I guess I need to cut off committing at this patch then... > @@ -861,16 +882,16 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) > name.s = get_value(&cfg, section.s, "ramdisk"); > if ( name.s ) > { > - split_value(name.s); > - read_file(dir_handle, s2w(&name), &ramdisk); > + option_str = split_string(name.s); > + read_file(dir_handle, s2w(&name), &ramdisk, option_str); As said before, this should be NULL as not having and never going to have a consumer. Whether you keep the XSM one below is - as also said before - up to you. Jan > efi_bs->FreePool(name.w); > } > > name.s = get_value(&cfg, section.s, "xsm"); > if ( name.s ) > { > - split_value(name.s); > - read_file(dir_handle, s2w(&name), &xsm); > + option_str = split_string(name.s); > + read_file(dir_handle, s2w(&name), &xsm, option_str); > efi_bs->FreePool(name.w); > } >
On Thu, Sep 25, 2014 at 3:34 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 25.09.14 at 03:42, <roy.franz@linaro.org> wrote: >> + /* >> + * Make a copy, as conversion is destructive, and caller still wants >> + * wide string available after this call returns. >> + */ >> + if ( efi_bs->AllocatePool(EfiLoaderData, (wstrlen(name) + 1) * sizeof(*name), >> + &ptr) != EFI_SUCCESS ) >> + blexit(L"ERROR Unable to allocate string buffer"); > > Kind of disappointing: You said you'd drop these ERROR prefixes, > but this is the second one I come across. I'm ditching them in > preparation for committing. > >> + >> + local_name.w = ptr; >> + wstrcpy(local_name.w, name); >> + w2s(&local_name); >> + >> + place_string(&mb_modules[mbi.mods_count].string, options); >> + place_string(&mb_modules[mbi.mods_count].string, ""); > > Hmm, this one's still here, and I don't recall having seen an > explanation for it. I guess I need to cut off committing at this > patch then... > I apologize - I missed adding the above to items to my checklist, and was likely too focused on getting an updated set out yesterday so these items got missed. >> @@ -861,16 +882,16 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) >> name.s = get_value(&cfg, section.s, "ramdisk"); >> if ( name.s ) >> { >> - split_value(name.s); >> - read_file(dir_handle, s2w(&name), &ramdisk); >> + option_str = split_string(name.s); >> + read_file(dir_handle, s2w(&name), &ramdisk, option_str); > > As said before, this should be NULL as not having and never going to > have a consumer. Whether you keep the XSM one below is - as also > said before - up to you. > > Jan I'll drop the options for ramdisk and xsm. I addresses the options on the ucode module, which is what you had previously commented on. I will rebase on staging ( or master if they get pushed there today) to get the patches you have already committed. Roy > >> efi_bs->FreePool(name.w); >> } >> >> name.s = get_value(&cfg, section.s, "xsm"); >> if ( name.s ) >> { >> - split_value(name.s); >> - read_file(dir_handle, s2w(&name), &xsm); >> + option_str = split_string(name.s); >> + read_file(dir_handle, s2w(&name), &xsm, option_str); >> efi_bs->FreePool(name.w); >> } >> > >
On Thu, Sep 25, 2014 at 9:44 AM, Roy Franz <roy.franz@linaro.org> wrote: > On Thu, Sep 25, 2014 at 3:34 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 25.09.14 at 03:42, <roy.franz@linaro.org> wrote: >>> + /* >>> + * Make a copy, as conversion is destructive, and caller still wants >>> + * wide string available after this call returns. >>> + */ >>> + if ( efi_bs->AllocatePool(EfiLoaderData, (wstrlen(name) + 1) * sizeof(*name), >>> + &ptr) != EFI_SUCCESS ) >>> + blexit(L"ERROR Unable to allocate string buffer"); >> >> Kind of disappointing: You said you'd drop these ERROR prefixes, >> but this is the second one I come across. I'm ditching them in >> preparation for committing. >> >>> + >>> + local_name.w = ptr; >>> + wstrcpy(local_name.w, name); >>> + w2s(&local_name); >>> + >>> + place_string(&mb_modules[mbi.mods_count].string, options); >>> + place_string(&mb_modules[mbi.mods_count].string, ""); >> >> Hmm, this one's still here, and I don't recall having seen an >> explanation for it. I guess I need to cut off committing at this >> patch then... >> The original code put the entire line from the config file into the mb_modules[i].string field, which consists of the filename and module options, space separated. This was done at the same time the filename was extracted to pass to read_file(). My refactored version of read_file() takes a filename and option string as separate arguments, and then after the file is loaded and address known, the module info is updated. The efi_arch_handle_module() gets the filename and options separately, and the above place_string() commands reconstruct the the single string of filename followed by options that is placed in the mb_modules[i].string field. I will add a comment to this effect, and also only add the options if present, since for several module types options will be NULL. > > I apologize - I missed adding the above to items to my checklist, and > was likely too focused > on getting an updated set out yesterday so these items got missed. > >>> @@ -861,16 +882,16 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) >>> name.s = get_value(&cfg, section.s, "ramdisk"); >>> if ( name.s ) >>> { >>> - split_value(name.s); >>> - read_file(dir_handle, s2w(&name), &ramdisk); >>> + option_str = split_string(name.s); >>> + read_file(dir_handle, s2w(&name), &ramdisk, option_str); >> >> As said before, this should be NULL as not having and never going to >> have a consumer. Whether you keep the XSM one below is - as also >> said before - up to you. >> >> Jan > > I'll drop the options for ramdisk and xsm. I addresses the options on > the ucode module, > which is what you had previously commented on. > > I will rebase on staging ( or master if they get pushed there today) > to get the patches you have already committed. > > Roy > >> >>> efi_bs->FreePool(name.w); >>> } >>> >>> name.s = get_value(&cfg, section.s, "xsm"); >>> if ( name.s ) >>> { >>> - split_value(name.s); >>> - read_file(dir_handle, s2w(&name), &xsm); >>> + option_str = split_string(name.s); >>> + read_file(dir_handle, s2w(&name), &xsm, option_str); >>> efi_bs->FreePool(name.w); >>> } >>> >> >>
On Thu, Sep 25, 2014 at 11:52 AM, Roy Franz <roy.franz@linaro.org> wrote: > On Thu, Sep 25, 2014 at 9:44 AM, Roy Franz <roy.franz@linaro.org> wrote: >> On Thu, Sep 25, 2014 at 3:34 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>> On 25.09.14 at 03:42, <roy.franz@linaro.org> wrote: >>>> + /* >>>> + * Make a copy, as conversion is destructive, and caller still wants >>>> + * wide string available after this call returns. >>>> + */ >>>> + if ( efi_bs->AllocatePool(EfiLoaderData, (wstrlen(name) + 1) * sizeof(*name), >>>> + &ptr) != EFI_SUCCESS ) >>>> + blexit(L"ERROR Unable to allocate string buffer"); >>> >>> Kind of disappointing: You said you'd drop these ERROR prefixes, >>> but this is the second one I come across. I'm ditching them in >>> preparation for committing. >>> >>>> + >>>> + local_name.w = ptr; >>>> + wstrcpy(local_name.w, name); >>>> + w2s(&local_name); >>>> + >>>> + place_string(&mb_modules[mbi.mods_count].string, options); >>>> + place_string(&mb_modules[mbi.mods_count].string, ""); >>> >>> Hmm, this one's still here, and I don't recall having seen an >>> explanation for it. I guess I need to cut off committing at this >>> patch then... >>> > > The original code put the entire line from the config file into the > mb_modules[i].string field, > which consists of the filename and module options, space separated. > This was done at the > same time the filename was extracted to pass to read_file(). > > My refactored version of read_file() takes a filename and option > string as separate arguments, > and then after the file is loaded and address known, the module info is updated. > The efi_arch_handle_module() gets the filename and options separately, > and the above place_string() > commands reconstruct the the single string of filename followed by > options that is placed > in the mb_modules[i].string field. > > I will add a comment to this effect, and also only add the options if > present, since for > several module types options will be NULL. > Reviewing this in more detail, the line: place_string(&mb_modules[mbi.mods_count].string, ""); is not needed and has been removed. I have verified the same kernel commandline in dom0 (/proc/cmdline) before and after the change. > >> >> I apologize - I missed adding the above to items to my checklist, and >> was likely too focused >> on getting an updated set out yesterday so these items got missed. >> >>>> @@ -861,16 +882,16 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) >>>> name.s = get_value(&cfg, section.s, "ramdisk"); >>>> if ( name.s ) >>>> { >>>> - split_value(name.s); >>>> - read_file(dir_handle, s2w(&name), &ramdisk); >>>> + option_str = split_string(name.s); >>>> + read_file(dir_handle, s2w(&name), &ramdisk, option_str); >>> >>> As said before, this should be NULL as not having and never going to >>> have a consumer. Whether you keep the XSM one below is - as also >>> said before - up to you. >>> >>> Jan >> >> I'll drop the options for ramdisk and xsm. I addresses the options on >> the ucode module, >> which is what you had previously commented on. >> >> I will rebase on staging ( or master if they get pushed there today) >> to get the patches you have already committed. >> >> Roy >> >>> >>>> efi_bs->FreePool(name.w); >>>> } >>>> >>>> name.s = get_value(&cfg, section.s, "xsm"); >>>> if ( name.s ) >>>> { >>>> - split_value(name.s); >>>> - read_file(dir_handle, s2w(&name), &xsm); >>>> + option_str = split_string(name.s); >>>> + read_file(dir_handle, s2w(&name), &xsm, option_str); >>>> efi_bs->FreePool(name.w); >>>> } >>>> >>> >>>
>>> On 26.09.14 at 02:25, <roy.franz@linaro.org> wrote: > On Thu, Sep 25, 2014 at 11:52 AM, Roy Franz <roy.franz@linaro.org> wrote: >> On Thu, Sep 25, 2014 at 9:44 AM, Roy Franz <roy.franz@linaro.org> wrote: >>> On Thu, Sep 25, 2014 at 3:34 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>> On 25.09.14 at 03:42, <roy.franz@linaro.org> wrote: >>>>> + place_string(&mb_modules[mbi.mods_count].string, options); >>>>> + place_string(&mb_modules[mbi.mods_count].string, ""); >>>> >>>> Hmm, this one's still here, and I don't recall having seen an >>>> explanation for it. I guess I need to cut off committing at this >>>> patch then... >>>> >> >> The original code put the entire line from the config file into the >> mb_modules[i].string field, >> which consists of the filename and module options, space separated. >> This was done at the >> same time the filename was extracted to pass to read_file(). >> >> My refactored version of read_file() takes a filename and option >> string as separate arguments, >> and then after the file is loaded and address known, the module info is > updated. >> The efi_arch_handle_module() gets the filename and options separately, >> and the above place_string() >> commands reconstruct the the single string of filename followed by >> options that is placed >> in the mb_modules[i].string field. >> >> I will add a comment to this effect, and also only add the options if >> present, since for >> several module types options will be NULL. >> > > Reviewing this in more detail, the line: > > place_string(&mb_modules[mbi.mods_count].string, ""); > > is not needed and has been removed. I have verified the same kernel > commandline > in dom0 (/proc/cmdline) before and after the change. Indeed, since if ( s && *s ) in place_string() made this a no-op. Jan
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index 2f032e0..ed5e15b 100644 --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -263,8 +263,8 @@ static void __init efi_arch_cfg_file_late(EFI_FILE_HANDLE dir_handle, char *sect if ( name.s ) { microcode_set_module(mbi.mods_count); - split_value(name.s); - read_file(dir_handle, s2w(&name), &ucode); + split_string(name.s); + read_file(dir_handle, s2w(&name), &ucode, NULL); efi_bs->FreePool(name.w); } } @@ -564,3 +564,30 @@ static void __init efi_arch_memory_setup(void) l3_bootmap[l3_table_offset(xen_phys_start + (8 << L2_PAGETABLE_SHIFT) - 1)] = l3e_from_paddr((UINTN)l2_bootmap, __PAGE_HYPERVISOR); } + +static void __init efi_arch_handle_module(struct file *file, const CHAR16 *name, + char *options) +{ + union string local_name; + void *ptr; + + /* + * Make a copy, as conversion is destructive, and caller still wants + * wide string available after this call returns. + */ + if ( efi_bs->AllocatePool(EfiLoaderData, (wstrlen(name) + 1) * sizeof(*name), + &ptr) != EFI_SUCCESS ) + blexit(L"ERROR Unable to allocate string buffer"); + + local_name.w = ptr; + wstrcpy(local_name.w, name); + w2s(&local_name); + + place_string(&mb_modules[mbi.mods_count].string, options); + place_string(&mb_modules[mbi.mods_count].string, ""); + place_string(&mb_modules[mbi.mods_count].string, local_name.s); + mb_modules[mbi.mods_count].mod_start = file->addr >> PAGE_SHIFT; + mb_modules[mbi.mods_count].mod_end = file->size; + ++mbi.mods_count; + efi_bs->FreePool(ptr); +} diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index 5661aff..f19db81 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -59,11 +59,12 @@ static void noreturn blexit(const CHAR16 *str); static void PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode); static char *get_value(const struct file *cfg, const char *section, const char *item); -static void split_value(char *s); +static char *split_string(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); +static bool_t read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, + struct file *file, char *options); +static size_t wstrlen(const CHAR16 * s); static int set_color(u32 mask, int bpp, u8 *pos, u8 *sz); static EFI_BOOT_SERVICES *__initdata efi_bs; @@ -120,6 +121,15 @@ static void __init DisplayUint(UINT64 Val, INTN Width) PrintStr(PrintString); } +static size_t __init wstrlen(const CHAR16 *s) +{ + const CHAR16 *sc; + + for ( sc = s; *sc != L'\0'; ++sc ) + /* nothing */; + return sc - s; +} + static CHAR16 *__init wstrcpy(CHAR16 *d, const CHAR16 *s) { CHAR16 *r = d; @@ -409,9 +419,25 @@ static CHAR16 *__init point_tail(CHAR16 *fn) break; } } +/* + * Truncate string at first space, and return pointer + * to remainder of string, if any/ NULL returned if + * no remainder after space. + */ +static char * __init split_string(char *s) +{ + while ( *s && !isspace(*s) ) + ++s; + if ( *s ) + { + *s = 0; + return s + 1; + } + return NULL; +} static bool_t __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, - struct file *file) + struct file *file, char *options) { EFI_FILE_HANDLE FileHandle = NULL; UINT64 size; @@ -452,6 +478,7 @@ static bool_t __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, } else { + file->size = size; if ( file != &cfg ) { PrintStr(name); @@ -460,12 +487,9 @@ static bool_t __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, PrintStr(L"-"); DisplayUint(file->addr + size, 2 * sizeof(file->addr)); PrintStr(newline); - mb_modules[mbi.mods_count].mod_start = file->addr >> PAGE_SHIFT; - mb_modules[mbi.mods_count].mod_end = size; - ++mbi.mods_count; + efi_arch_handle_module(file, name, options); } - file->size = size; ret = FileHandle->Read(FileHandle, &file->size, file->ptr); if ( !EFI_ERROR(ret) && file->size != size ) ret = EFI_ABORTED; @@ -536,7 +560,13 @@ static char *__init get_value(const struct file *cfg, const char *section, break; default: if ( match && strncmp(ptr, item, ilen) == 0 && ptr[ilen] == '=' ) - return ptr + ilen + 1; + { + ptr += ilen + 1; + /* strip off any leading spaces */ + while ( *ptr && isspace(*ptr) ) + ptr++; + return ptr; + } break; } ptr += strlen(ptr); @@ -544,16 +574,6 @@ static char *__init get_value(const struct file *cfg, const char *section, return NULL; } -static void __init split_value(char *s) -{ - while ( *s && isspace(*s) ) - ++s; - place_string(&mb_modules[mbi.mods_count].string, s); - while ( *s && !isspace(*s) ) - ++s; - *s = 0; -} - static void __init setup_efi_pci(void) { EFI_STATUS status; @@ -674,6 +694,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) EFI_FILE_HANDLE dir_handle; union string section = { NULL }, name; bool_t base_video = 0; + char *option_str; efi_ih = ImageHandle; efi_bs = SystemTable->BootServices; @@ -805,7 +826,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) while ( (tail = point_tail(file_name)) != NULL ) { wstrcpy(tail, L".cfg"); - if ( read_file(dir_handle, file_name, &cfg) ) + if ( read_file(dir_handle, file_name, &cfg, NULL) ) break; *tail = 0; } @@ -815,7 +836,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) PrintStr(file_name); PrintStr(L"'\r\n"); } - else if ( !read_file(dir_handle, cfg_file_name, &cfg) ) + else if ( !read_file(dir_handle, cfg_file_name, &cfg, NULL) ) blexit(L"Configuration file not found."); pre_parse(&cfg); @@ -834,7 +855,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) break; efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size)); cfg.addr = 0; - if ( !read_file(dir_handle, s2w(&name), &cfg) ) + if ( !read_file(dir_handle, s2w(&name), &cfg, NULL) ) { PrintStr(L"Chained configuration file '"); PrintStr(name.w); @@ -849,8 +870,8 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) efi_arch_cfg_file_early(dir_handle, section.s); - split_value(name.s); - read_file(dir_handle, s2w(&name), &kernel); + option_str = split_string(name.s); + read_file(dir_handle, s2w(&name), &kernel, option_str); efi_bs->FreePool(name.w); if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL, @@ -861,16 +882,16 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) name.s = get_value(&cfg, section.s, "ramdisk"); if ( name.s ) { - split_value(name.s); - read_file(dir_handle, s2w(&name), &ramdisk); + option_str = split_string(name.s); + read_file(dir_handle, s2w(&name), &ramdisk, option_str); efi_bs->FreePool(name.w); } name.s = get_value(&cfg, section.s, "xsm"); if ( name.s ) { - split_value(name.s); - read_file(dir_handle, s2w(&name), &xsm); + option_str = split_string(name.s); + read_file(dir_handle, s2w(&name), &xsm, option_str); efi_bs->FreePool(name.w); }
Each architecture tracks modules differently internally, so add efi_arch_handle_module() routine to enable the common code to invoke the proper handling of modules as the are loaded. ucode module handling is changed to not process remainder of string after filename as options, since ucode module does not take options. Signed-off-by: Roy Franz <roy.franz@linaro.org> --- xen/arch/x86/efi/efi-boot.h | 31 ++++++++++++++++-- xen/common/efi/boot.c | 77 ++++++++++++++++++++++++++++----------------- 2 files changed, 78 insertions(+), 30 deletions(-)