| Message ID | 1411080607-32365-11-git-send-email-roy.franz@linaro.org |
|---|---|
| State | New |
| Headers | show |
>>> On 19.09.14 at 00:50, <roy.franz@linaro.org> wrote: > --- a/xen/common/efi/boot.c > +++ b/xen/common/efi/boot.c > @@ -56,12 +56,13 @@ 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 size_t wstrlen(const CHAR16 * s); > static int set_color(u32 mask, int bpp, u8 *pos, u8 *sz); > +static bool_t read_file(EFI_FILE_HANDLE dir_handle, struct file *file, > + CHAR16 *filename, char *options); Please don't needlessly move this declaration down. > @@ -115,6 +116,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; > +} Coding style (many issues). > @@ -404,18 +414,33 @@ static CHAR16 *__init point_tail(CHAR16 *fn) > break; > } > } > +/* > + * Truncate string at first space, and return pointer > + * to remainder of string. ... (if any). > + */ > +static char * __init split_string(char *s) > +{ > + while ( *s && !isspace(*s) ) > + ++s; > + if ( *s ) > + { > + *s = 0; > + return(s + 1); No parentheses here please. > + } > + return NULL; > +} > > -static bool_t __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, > - struct file *file) > +static bool_t __init read_file(EFI_FILE_HANDLE dir_handle, struct file *file, > + CHAR16 *filename, char *options) Is the renaming from "name" to "filename" really necessary/useful? And the flipping of the order of pre-existing parameters? > @@ -659,18 +678,19 @@ 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, *options = NULL; > + CHAR16 **argv, *options = NULL; > UINTN cols, rows, depth, size, info_size, gop_mode = ~0; > EFI_HANDLE *handles = NULL; > EFI_SHIM_LOCK_PROTOCOL *shim_lock; > EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL; > EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *mode_info; > EFI_FILE_HANDLE dir_handle; > - union string section = { NULL }, name; > + union string section = { NULL }, name, file_name, cfg_file_name = {NULL}; Your addition should be consistent with existing code (blanks around NULL). But - why are you changing the types of the two variables in the first place? Afaics all references to them now are using the .w member access, suggesting that this is just a remnant of your earlier version changes. > @@ -274,8 +275,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); > + options = split_string(name.s); > + read_file(dir_handle, &ucode, s2w(&name), options); Perhaps rather NULL instead of options? > @@ -575,3 +576,30 @@ static void __init efi_arch_memory(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\r\n"); Iirc I said this before, but just in case: No explicit newline on strings passed to blexit() please. Jan
On Mon, Sep 22, 2014 at 5:44 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 19.09.14 at 00:50, <roy.franz@linaro.org> wrote: >> --- a/xen/common/efi/boot.c >> +++ b/xen/common/efi/boot.c >> @@ -56,12 +56,13 @@ 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 size_t wstrlen(const CHAR16 * s); >> static int set_color(u32 mask, int bpp, u8 *pos, u8 *sz); >> +static bool_t read_file(EFI_FILE_HANDLE dir_handle, struct file *file, >> + CHAR16 *filename, char *options); > > Please don't needlessly move this declaration down. > I'll fix this. >> @@ -115,6 +116,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; >> +} > > Coding style (many issues). static size_t __init wstrlen(const CHAR16 *s) { const CHAR16 *sc; for ( sc = s; *sc != L'\0'; ++sc ) ; return sc - s; } Is the above OK? Not sure what else to change here... > >> @@ -404,18 +414,33 @@ static CHAR16 *__init point_tail(CHAR16 *fn) >> break; >> } >> } >> +/* >> + * Truncate string at first space, and return pointer >> + * to remainder of string. > > ... (if any). > Sure. >> + */ >> +static char * __init split_string(char *s) >> +{ >> + while ( *s && !isspace(*s) ) >> + ++s; >> + if ( *s ) >> + { >> + *s = 0; >> + return(s + 1); > > No parentheses here please. sure > >> + } >> + return NULL; >> +} >> >> -static bool_t __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, >> - struct file *file) >> +static bool_t __init read_file(EFI_FILE_HANDLE dir_handle, struct file *file, >> + CHAR16 *filename, char *options) > > Is the renaming from "name" to "filename" really necessary/useful? > And the flipping of the order of pre-existing parameters? I'll fix this. > >> @@ -659,18 +678,19 @@ 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, *options = NULL; >> + CHAR16 **argv, *options = NULL; >> UINTN cols, rows, depth, size, info_size, gop_mode = ~0; >> EFI_HANDLE *handles = NULL; >> EFI_SHIM_LOCK_PROTOCOL *shim_lock; >> EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL; >> EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *mode_info; >> EFI_FILE_HANDLE dir_handle; >> - union string section = { NULL }, name; >> + union string section = { NULL }, name, file_name, cfg_file_name = {NULL}; > > Your addition should be consistent with existing code (blanks around > NULL). But - why are you changing the types of the two variables in > the first place? Afaics all references to them now are using the .w > member access, suggesting that this is just a remnant of your earlier > version changes. > Yes, this is leftover type changes from a previous version. Both these should be able to to be reverted to simple CHAR16 * >> @@ -274,8 +275,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); >> + options = split_string(name.s); >> + read_file(dir_handle, &ucode, s2w(&name), options); > > Perhaps rather NULL instead of options? Yup. > >> @@ -575,3 +576,30 @@ static void __init efi_arch_memory(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\r\n"); > > Iirc I said this before, but just in case: No explicit newline on strings > passed to blexit() please. Yes. > > Jan
>>> On 23.09.14 at 03:57, <roy.franz@linaro.org> wrote: > On Mon, Sep 22, 2014 at 5:44 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 19.09.14 at 00:50, <roy.franz@linaro.org> wrote: >>> @@ -115,6 +116,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; >>> +} >> >> Coding style (many issues). > > static size_t __init wstrlen(const CHAR16 *s) > { > const CHAR16 *sc; > > for ( sc = s; *sc != L'\0'; ++sc ) > ; > return sc - s; > } > > Is the above OK? Not sure what else to change here... Almost - the lone ; is still indented one level (4 spaces) too deep. Jan
On Tue, 2014-09-23 at 13:28 +0100, Jan Beulich wrote: > >>> On 23.09.14 at 03:57, <roy.franz@linaro.org> wrote: > > On Mon, Sep 22, 2014 at 5:44 AM, Jan Beulich <JBeulich@suse.com> wrote: > >>>>> On 19.09.14 at 00:50, <roy.franz@linaro.org> wrote: > >>> @@ -115,6 +116,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; > >>> +} > >> > >> Coding style (many issues). > > > > static size_t __init wstrlen(const CHAR16 *s) > > { > > const CHAR16 *sc; > > > > for ( sc = s; *sc != L'\0'; ++sc ) > > ; > > return sc - s; > > } > > > > Is the above OK? Not sure what else to change here... > > Almost - the lone ; is still indented one level (4 spaces) too deep. Would you be inclined to keep the /* nothing */ too as a signal to the reader that it is deliberate? Ian
>>> On 23.09.14 at 14:41, <Ian.Campbell@citrix.com> wrote: > On Tue, 2014-09-23 at 13:28 +0100, Jan Beulich wrote: >> >>> On 23.09.14 at 03:57, <roy.franz@linaro.org> wrote: >> > On Mon, Sep 22, 2014 at 5:44 AM, Jan Beulich <JBeulich@suse.com> wrote: >> >>>>> On 19.09.14 at 00:50, <roy.franz@linaro.org> wrote: >> >>> @@ -115,6 +116,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; >> >>> +} >> >> >> >> Coding style (many issues). >> > >> > static size_t __init wstrlen(const CHAR16 *s) >> > { >> > const CHAR16 *sc; >> > >> > for ( sc = s; *sc != L'\0'; ++sc ) >> > ; >> > return sc - s; >> > } >> > >> > Is the above OK? Not sure what else to change here... >> >> Almost - the lone ; is still indented one level (4 spaces) too deep. > > Would you be inclined to keep the /* nothing */ too as a signal to the > reader that it is deliberate? Yes - I didn't pay attention to it having got dropped with the re-formatting. Jan
On Mon, Sep 22, 2014 at 6:57 PM, Roy Franz <roy.franz@linaro.org> wrote: > On Mon, Sep 22, 2014 at 5:44 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 19.09.14 at 00:50, <roy.franz@linaro.org> wrote: >>> --- a/xen/common/efi/boot.c >>> +++ b/xen/common/efi/boot.c >>> @@ -56,12 +56,13 @@ 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 size_t wstrlen(const CHAR16 * s); >>> static int set_color(u32 mask, int bpp, u8 *pos, u8 *sz); >>> +static bool_t read_file(EFI_FILE_HANDLE dir_handle, struct file *file, >>> + CHAR16 *filename, char *options); >> >> Please don't needlessly move this declaration down. >> > I'll fix this. > >>> @@ -115,6 +116,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; >>> +} >> >> Coding style (many issues). > > static size_t __init wstrlen(const CHAR16 *s) > { > const CHAR16 *sc; > > for ( sc = s; *sc != L'\0'; ++sc ) > ; > return sc - s; > } > > Is the above OK? Not sure what else to change here... > >> >>> @@ -404,18 +414,33 @@ static CHAR16 *__init point_tail(CHAR16 *fn) >>> break; >>> } >>> } >>> +/* >>> + * Truncate string at first space, and return pointer >>> + * to remainder of string. >> >> ... (if any). >> > > Sure. > > >>> + */ >>> +static char * __init split_string(char *s) >>> +{ >>> + while ( *s && !isspace(*s) ) >>> + ++s; >>> + if ( *s ) >>> + { >>> + *s = 0; >>> + return(s + 1); >> >> No parentheses here please. > > sure > > >> >>> + } >>> + return NULL; >>> +} >>> >>> -static bool_t __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, >>> - struct file *file) >>> +static bool_t __init read_file(EFI_FILE_HANDLE dir_handle, struct file *file, >>> + CHAR16 *filename, char *options) >> >> Is the renaming from "name" to "filename" really necessary/useful? >> And the flipping of the order of pre-existing parameters? > > I'll fix this. > >> >>> @@ -659,18 +678,19 @@ 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, *options = NULL; >>> + CHAR16 **argv, *options = NULL; >>> UINTN cols, rows, depth, size, info_size, gop_mode = ~0; >>> EFI_HANDLE *handles = NULL; >>> EFI_SHIM_LOCK_PROTOCOL *shim_lock; >>> EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL; >>> EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *mode_info; >>> EFI_FILE_HANDLE dir_handle; >>> - union string section = { NULL }, name; >>> + union string section = { NULL }, name, file_name, cfg_file_name = {NULL}; >> >> Your addition should be consistent with existing code (blanks around >> NULL). But - why are you changing the types of the two variables in >> the first place? Afaics all references to them now are using the .w >> member access, suggesting that this is just a remnant of your earlier >> version changes. >> > > Yes, this is leftover type changes from a previous version. Both > these should be able to to be > reverted to simple CHAR16 * >>> @@ -274,8 +275,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); >>> + options = split_string(name.s); >>> + read_file(dir_handle, &ucode, s2w(&name), options); >> >> Perhaps rather NULL instead of options? > > Yup. > OK, I looked at this a little more, and the original code would take anything after the ucode filename and put it into the mbi.string field for the ucode module. All the modules where treated the same in this regard - they could all have options. If ucode never has options, I can remove this, but this would be a change in behavior. >> >>> @@ -575,3 +576,30 @@ static void __init efi_arch_memory(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\r\n"); >> >> Iirc I said this before, but just in case: No explicit newline on strings >> passed to blexit() please. > Yes. >> >> Jan
On Tue, Sep 23, 2014 at 6:23 PM, Roy Franz <roy.franz@linaro.org> wrote: > On Mon, Sep 22, 2014 at 6:57 PM, Roy Franz <roy.franz@linaro.org> wrote: >> On Mon, Sep 22, 2014 at 5:44 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>> On 19.09.14 at 00:50, <roy.franz@linaro.org> wrote: >>>> --- a/xen/common/efi/boot.c >>>> +++ b/xen/common/efi/boot.c >>>> @@ -56,12 +56,13 @@ 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 size_t wstrlen(const CHAR16 * s); >>>> static int set_color(u32 mask, int bpp, u8 *pos, u8 *sz); >>>> +static bool_t read_file(EFI_FILE_HANDLE dir_handle, struct file *file, >>>> + CHAR16 *filename, char *options); >>> >>> Please don't needlessly move this declaration down. >>> >> I'll fix this. >> >>>> @@ -115,6 +116,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; >>>> +} >>> >>> Coding style (many issues). >> >> static size_t __init wstrlen(const CHAR16 *s) >> { >> const CHAR16 *sc; >> >> for ( sc = s; *sc != L'\0'; ++sc ) >> ; >> return sc - s; >> } >> >> Is the above OK? Not sure what else to change here... >> >>> >>>> @@ -404,18 +414,33 @@ static CHAR16 *__init point_tail(CHAR16 *fn) >>>> break; >>>> } >>>> } >>>> +/* >>>> + * Truncate string at first space, and return pointer >>>> + * to remainder of string. >>> >>> ... (if any). >>> >> >> Sure. >> >> >>>> + */ >>>> +static char * __init split_string(char *s) >>>> +{ >>>> + while ( *s && !isspace(*s) ) >>>> + ++s; >>>> + if ( *s ) >>>> + { >>>> + *s = 0; >>>> + return(s + 1); >>> >>> No parentheses here please. >> >> sure >> >> >>> >>>> + } >>>> + return NULL; >>>> +} >>>> >>>> -static bool_t __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, >>>> - struct file *file) >>>> +static bool_t __init read_file(EFI_FILE_HANDLE dir_handle, struct file *file, >>>> + CHAR16 *filename, char *options) >>> >>> Is the renaming from "name" to "filename" really necessary/useful? >>> And the flipping of the order of pre-existing parameters? >> >> I'll fix this. >> >>> >>>> @@ -659,18 +678,19 @@ 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, *options = NULL; >>>> + CHAR16 **argv, *options = NULL; >>>> UINTN cols, rows, depth, size, info_size, gop_mode = ~0; >>>> EFI_HANDLE *handles = NULL; >>>> EFI_SHIM_LOCK_PROTOCOL *shim_lock; >>>> EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL; >>>> EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *mode_info; >>>> EFI_FILE_HANDLE dir_handle; >>>> - union string section = { NULL }, name; >>>> + union string section = { NULL }, name, file_name, cfg_file_name = {NULL}; >>> >>> Your addition should be consistent with existing code (blanks around >>> NULL). But - why are you changing the types of the two variables in >>> the first place? Afaics all references to them now are using the .w >>> member access, suggesting that this is just a remnant of your earlier >>> version changes. >>> >> >> Yes, this is leftover type changes from a previous version. Both >> these should be able to to be >> reverted to simple CHAR16 * >>>> @@ -274,8 +275,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); >>>> + options = split_string(name.s); >>>> + read_file(dir_handle, &ucode, s2w(&name), options); >>> >>> Perhaps rather NULL instead of options? >> >> Yup. >> > OK, I looked at this a little more, and the original code would take > anything after the ucode filename > and put it into the mbi.string field for the ucode module. All the > modules where treated the same in this > regard - they could all have options. > > If ucode never has options, I can remove this, but this would be a > change in behavior. The EFI config file documentation specifies just the filename, so I have simplified this in the next version. > > >>> >>>> @@ -575,3 +576,30 @@ static void __init efi_arch_memory(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\r\n"); >>> >>> Iirc I said this before, but just in case: No explicit newline on strings >>> passed to blexit() please. >> Yes. >>> >>> Jan
>>> On 24.09.14 at 03:23, <roy.franz@linaro.org> wrote: > On Mon, Sep 22, 2014 at 6:57 PM, Roy Franz <roy.franz@linaro.org> wrote: >> On Mon, Sep 22, 2014 at 5:44 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>> On 19.09.14 at 00:50, <roy.franz@linaro.org> wrote: >>>> @@ -274,8 +275,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); >>>> + options = split_string(name.s); >>>> + read_file(dir_handle, &ucode, s2w(&name), options); >>> >>> Perhaps rather NULL instead of options? >> >> Yup. >> > OK, I looked at this a little more, and the original code would take > anything after the ucode filename > and put it into the mbi.string field for the ucode module. All the > modules where treated the same in this > regard - they could all have options. > > If ucode never has options, I can remove this, but this would be a > change in behavior. This was done that way just to avoid extra conditionals. The resulting mbi.string doesn't get consumed anywhere (just like for initrd and xsm, albeit in the latter case one could at least envision a potential future use). Jan
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index 38177bf..4acab40 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -56,12 +56,13 @@ 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 size_t wstrlen(const CHAR16 * s); static int set_color(u32 mask, int bpp, u8 *pos, u8 *sz); +static bool_t read_file(EFI_FILE_HANDLE dir_handle, struct file *file, + CHAR16 *filename, char *options); static EFI_BOOT_SERVICES *__initdata efi_bs; static EFI_HANDLE __initdata efi_ih; @@ -115,6 +116,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; @@ -404,18 +414,33 @@ static CHAR16 *__init point_tail(CHAR16 *fn) break; } } +/* + * Truncate string at first space, and return pointer + * to remainder of string. + */ +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) +static bool_t __init read_file(EFI_FILE_HANDLE dir_handle, struct file *file, + CHAR16 *filename, char *options) { EFI_FILE_HANDLE FileHandle = NULL; UINT64 size; EFI_STATUS ret; CHAR16 *what = NULL; - if ( !name ) + if ( !filename ) PrintErrMesg(L"No filename", EFI_OUT_OF_RESOURCES); - ret = dir_handle->Open(dir_handle, &FileHandle, name, + ret = dir_handle->Open(dir_handle, &FileHandle, filename, EFI_FILE_MODE_READ, 0); if ( file == &cfg && ret == EFI_NOT_FOUND ) return 0; @@ -447,20 +472,18 @@ static bool_t __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, } else { + file->size = size; if ( file != &cfg ) { - PrintStr(name); + PrintStr(filename); PrintStr(L": "); DisplayUint(file->addr, 2 * sizeof(file->addr)); 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, filename, options); } - file->size = size; ret = FileHandle->Read(FileHandle, &file->size, file->ptr); if ( !EFI_ERROR(ret) && file->size != size ) ret = EFI_ABORTED; @@ -475,7 +498,7 @@ static bool_t __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, { PrintErr(what); PrintErr(L" failed for "); - PrintErrMesg(name, ret); + PrintErrMesg(filename, ret); } return 1; @@ -531,7 +554,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); @@ -539,16 +568,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; @@ -659,18 +678,19 @@ 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, *options = NULL; + CHAR16 **argv, *options = NULL; UINTN cols, rows, depth, size, info_size, gop_mode = ~0; EFI_HANDLE *handles = NULL; EFI_SHIM_LOCK_PROTOCOL *shim_lock; EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL; EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *mode_info; EFI_FILE_HANDLE dir_handle; - union string section = { NULL }, name; + union string section = { NULL }, name, file_name, cfg_file_name = {NULL}; bool_t base_video = 0; UINT32 mmap_desc_ver = 0; UINTN mmap_size, mmap_desc_size, mmap_key = 0; void *mmap; + char *option_str; efi_ih = ImageHandle; efi_bs = SystemTable->BootServices; @@ -697,7 +717,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) trampoline_xen_phys_start = xen_phys_start; /* Get the file system interface. */ - dir_handle = get_parent_handle(loaded_image, &file_name); + dir_handle = get_parent_handle(loaded_image, &file_name.w); argc = get_argv(0, NULL, loaded_image->LoadOptions, loaded_image->LoadOptionsSize, &options); @@ -721,9 +741,9 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) if ( wstrcmp(ptr + 1, L"basevideo") == 0 ) base_video = 1; else if ( wstrncmp(ptr + 1, L"cfg=", 4) == 0 ) - cfg_file_name = ptr + 5; + cfg_file_name.w = ptr + 5; else if ( i + 1 < argc && wstrcmp(ptr + 1, L"cfg") == 0 ) - cfg_file_name = argv[++i]; + cfg_file_name.w = argv[++i]; else if ( wstrcmp(ptr + 1, L"help") == 0 || (ptr[1] == L'?' && !ptr[2]) ) { @@ -795,24 +815,24 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) gop = NULL; /* Read and parse the config file. */ - if ( !cfg_file_name ) + if ( !cfg_file_name.w ) { CHAR16 *tail; - while ( (tail = point_tail(file_name)) != NULL ) + while ( (tail = point_tail(file_name.w)) != NULL ) { wstrcpy(tail, L".cfg"); - if ( read_file(dir_handle, file_name, &cfg) ) + if ( read_file(dir_handle, &cfg, file_name.w, NULL) ) break; *tail = 0; } if ( !tail ) blexit(L"No configuration file found."); PrintStr(L"Using configuration file '"); - PrintStr(file_name); + PrintStr(file_name.w); PrintStr(L"'\r\n"); } - else if ( !read_file(dir_handle, cfg_file_name, &cfg) ) + else if ( !read_file(dir_handle, &cfg, cfg_file_name.w, NULL) ) blexit(L"Configuration file not found."); pre_parse(&cfg); @@ -831,7 +851,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, &cfg, s2w(&name), NULL) ) { PrintStr(L"Chained configuration file '"); PrintStr(name.w); @@ -846,8 +866,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, &kernel, s2w(&name), option_str); efi_bs->FreePool(name.w); if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL, @@ -858,16 +878,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, &ramdisk, s2w(&name), 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, &xsm, s2w(&name), option_str); efi_bs->FreePool(name.w); } diff --git a/xen/include/asm-x86/efi-boot.h b/xen/include/asm-x86/efi-boot.h index 2902970..3902b6c 100644 --- a/xen/include/asm-x86/efi-boot.h +++ b/xen/include/asm-x86/efi-boot.h @@ -267,6 +267,7 @@ static void __init efi_arch_cfg_file_early(EFI_FILE_HANDLE dir_handle, char *sec static void __init efi_arch_cfg_file_late(EFI_FILE_HANDLE dir_handle, char *section) { union string name; + char *options; name.s = get_value(&cfg, section, "ucode"); if ( !name.s ) @@ -274,8 +275,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); + options = split_string(name.s); + read_file(dir_handle, &ucode, s2w(&name), options); efi_bs->FreePool(name.w); } } @@ -575,3 +576,30 @@ static void __init efi_arch_memory(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\r\n"); + + 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); +}
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. Signed-off-by: Roy Franz <roy.franz@linaro.org> --- xen/common/efi/boot.c | 102 ++++++++++++++++++++++++----------------- xen/include/asm-x86/efi-boot.h | 32 ++++++++++++- 2 files changed, 91 insertions(+), 43 deletions(-)