| Message ID | 8b369fc8-8f9e-c350-95de-790d47fd9aae@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | : EFI: some tidying | expand |
On 03/12/2021 10:56, Jan Beulich wrote: > When it was introduced, it was imo placed way too high up, making it > necessary to forward-declare way too many static functions. Move it down > together with > - the efi_check_dt_boot() stub, which afaict was deliberately placed > immediately ahead of the #include, > - blexit(), because of its use of the efi_arch_blexit() hook. > Move up get_value() and set_color() to before the inclusion so their > forward declarations can also be zapped. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Why does blexit() need moving? It isn't static, and has a real prototype in efi.h ~Andrew
On 03.12.2021 12:21, Andrew Cooper wrote: > On 03/12/2021 10:56, Jan Beulich wrote: >> When it was introduced, it was imo placed way too high up, making it >> necessary to forward-declare way too many static functions. Move it down >> together with >> - the efi_check_dt_boot() stub, which afaict was deliberately placed >> immediately ahead of the #include, >> - blexit(), because of its use of the efi_arch_blexit() hook. >> Move up get_value() and set_color() to before the inclusion so their >> forward declarations can also be zapped. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Why does blexit() need moving? It isn't static, and has a real > prototype in efi.h Oops - clearly an oversight of mine. Jan
On 03/12/2021 11:25, Jan Beulich wrote: > On 03.12.2021 12:21, Andrew Cooper wrote: >> On 03/12/2021 10:56, Jan Beulich wrote: >>> When it was introduced, it was imo placed way too high up, making it >>> necessary to forward-declare way too many static functions. Move it down >>> together with >>> - the efi_check_dt_boot() stub, which afaict was deliberately placed >>> immediately ahead of the #include, >>> - blexit(), because of its use of the efi_arch_blexit() hook. >>> Move up get_value() and set_color() to before the inclusion so their >>> forward declarations can also be zapped. >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> Why does blexit() need moving? It isn't static, and has a real >> prototype in efi.h > Oops - clearly an oversight of mine. With that left as was, everything else looks fine, so the whole series Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
On 03.12.2021 12:21, Andrew Cooper wrote: > On 03/12/2021 10:56, Jan Beulich wrote: >> When it was introduced, it was imo placed way too high up, making it >> necessary to forward-declare way too many static functions. Move it down >> together with >> - the efi_check_dt_boot() stub, which afaict was deliberately placed >> immediately ahead of the #include, >> - blexit(), because of its use of the efi_arch_blexit() hook. >> Move up get_value() and set_color() to before the inclusion so their >> forward declarations can also be zapped. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Why does blexit() need moving? It isn't static, and has a real > prototype in efi.h Correct, but the movement is for the functions it uses from efi-boot.h: efi_arch_halt() and efi_arch_blexit() at least (which actually the commit message also says, for one of the two at least). Jan
On 03/12/2021 12:50, Jan Beulich wrote: > On 03.12.2021 12:21, Andrew Cooper wrote: >> On 03/12/2021 10:56, Jan Beulich wrote: >>> When it was introduced, it was imo placed way too high up, making it >>> necessary to forward-declare way too many static functions. Move it down >>> together with >>> - the efi_check_dt_boot() stub, which afaict was deliberately placed >>> immediately ahead of the #include, >>> - blexit(), because of its use of the efi_arch_blexit() hook. >>> Move up get_value() and set_color() to before the inclusion so their >>> forward declarations can also be zapped. >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> Why does blexit() need moving? It isn't static, and has a real >> prototype in efi.h > Correct, but the movement is for the functions it uses from efi-boot.h: > efi_arch_halt() and efi_arch_blexit() at least (which actually the > commit message also says, for one of the two at least). Ah ok.
> On 3 Dec 2021, at 10:56, Jan Beulich <jbeulich@suse.com> wrote: > > When it was introduced, it was imo placed way too high up, making it > necessary to forward-declare way too many static functions. Move it down > together with > - the efi_check_dt_boot() stub, which afaict was deliberately placed > immediately ahead of the #include, > - blexit(), because of its use of the efi_arch_blexit() hook. > Move up get_value() and set_color() to before the inclusion so their > forward declarations can also be zapped. > With the “const” attribute now some function in this serie are above the char line limit, however everything looks fine. Reviewed-by: Luca Fancellu <luca.fancellu@arm.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/common/efi/boot.c > +++ b/xen/common/efi/boot.c > @@ -111,25 +111,10 @@ struct file { > }; > }; > > -static CHAR16 *FormatDec(UINT64 Val, CHAR16 *Buffer); > -static CHAR16 *FormatHex(UINT64 Val, UINTN Width, CHAR16 *Buffer); > -static void DisplayUint(UINT64 Val, INTN Width); > -static CHAR16 *wstrcpy(CHAR16 *d, const CHAR16 *s); > -static void PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode); > -static char *get_value(const struct file *cfg, const char *section, > - const char *item); > -static char *split_string(char *s); > -static CHAR16 *s2w(union string *str); > -static char *w2s(const union string *str); > -static EFI_FILE_HANDLE get_parent_handle(EFI_LOADED_IMAGE *loaded_image, > - CHAR16 **leaf); > static bool read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, > struct file *file, const char *options); > static bool read_section(const EFI_LOADED_IMAGE *image, const CHAR16 *name, > struct file *file, const char *options); > -static size_t wstrlen(const CHAR16 * s); > -static int set_color(u32 mask, int bpp, u8 *pos, u8 *sz); > -static bool match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2); > > static void efi_init(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable); > static void efi_console_set_mode(void); > @@ -168,19 +153,6 @@ static void __init PrintErr(const CHAR16 > StdErr->OutputString(StdErr, (CHAR16 *)s ); > } > > -#ifndef CONFIG_HAS_DEVICE_TREE > -static int __init efi_check_dt_boot(EFI_LOADED_IMAGE *loaded_image) > -{ > - return 0; > -} > -#endif > - > -/* > - * Include architecture specific implementation here, which references the > - * static globals defined above. > - */ > -#include "efi-boot.h" > - > static CHAR16 *__init FormatDec(UINT64 Val, CHAR16 *Buffer) > { > if ( Val >= 10 ) > @@ -291,30 +263,6 @@ static bool __init match_guid(const EFI_ > !memcmp(guid1->Data4, guid2->Data4, sizeof(guid1->Data4)); > } > > -void __init noreturn blexit(const CHAR16 *str) > -{ > - if ( str ) > - PrintStr(str); > - PrintStr(newline); > - > - if ( !efi_bs ) > - efi_arch_halt(); > - > - if ( cfg.need_to_free ) > - efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size)); > - if ( kernel.need_to_free ) > - efi_bs->FreePages(kernel.addr, PFN_UP(kernel.size)); > - if ( ramdisk.need_to_free ) > - efi_bs->FreePages(ramdisk.addr, PFN_UP(ramdisk.size)); > - if ( xsm.need_to_free ) > - efi_bs->FreePages(xsm.addr, PFN_UP(xsm.size)); > - > - efi_arch_blexit(); > - > - efi_bs->Exit(efi_ih, EFI_SUCCESS, 0, NULL); > - unreachable(); /* not reached */ > -} > - > /* generic routine for printing error messages */ > static void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode) > { > @@ -542,6 +490,7 @@ static CHAR16 *__init point_tail(CHAR16 > break; > } > } > + > /* > * Truncate string at first space, and return pointer > * to remainder of string, if any/ NULL returned if > @@ -559,6 +508,95 @@ static char * __init split_string(char * > return NULL; > } > > +static char *__init get_value(const struct file *cfg, const char *section, > + const char *item) > +{ > + char *ptr = cfg->str, *end = ptr + cfg->size; > + size_t slen = section ? strlen(section) : 0, ilen = strlen(item); > + bool match = !slen; > + > + for ( ; ptr < end; ++ptr ) > + { > + switch ( *ptr ) > + { > + case 0: > + continue; > + case '[': > + if ( !slen ) > + break; > + if ( match ) > + return NULL; > + match = strncmp(++ptr, section, slen) == 0 && ptr[slen] == ']'; > + break; > + default: > + if ( match && strncmp(ptr, item, ilen) == 0 && ptr[ilen] == '=' ) > + { > + ptr += ilen + 1; > + /* strip off any leading spaces */ > + while ( *ptr && isspace(*ptr) ) > + ptr++; > + return ptr; > + } > + break; > + } > + ptr += strlen(ptr); > + } > + return NULL; > +} > + > +static int __init __maybe_unused set_color(uint32_t mask, int bpp, > + uint8_t *pos, uint8_t *sz) > +{ > + if ( bpp < 0 ) > + return bpp; > + if ( !mask ) > + return -EINVAL; > + for ( *pos = 0; !(mask & 1); ++*pos ) > + mask >>= 1; > + for ( *sz = 0; mask & 1; ++*sz) > + mask >>= 1; > + if ( mask ) > + return -EINVAL; > + return max(*pos + *sz, bpp); > +} > + > +#ifndef CONFIG_HAS_DEVICE_TREE > +static int __init efi_check_dt_boot(EFI_LOADED_IMAGE *loaded_image) > +{ > + return 0; > +} > +#endif > + > +/* > + * Include architecture specific implementation here, which references the > + * static globals defined above. > + */ > +#include "efi-boot.h" > + > +void __init noreturn blexit(const CHAR16 *str) > +{ > + if ( str ) > + PrintStr(str); > + PrintStr(newline); > + > + if ( !efi_bs ) > + efi_arch_halt(); > + > + if ( cfg.need_to_free ) > + efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size)); > + if ( kernel.need_to_free ) > + efi_bs->FreePages(kernel.addr, PFN_UP(kernel.size)); > + if ( ramdisk.need_to_free ) > + efi_bs->FreePages(ramdisk.addr, PFN_UP(ramdisk.size)); > + if ( xsm.need_to_free ) > + efi_bs->FreePages(xsm.addr, PFN_UP(xsm.size)); > + > + efi_arch_blexit(); > + > + efi_bs->Exit(efi_ih, EFI_SUCCESS, 0, NULL); > + unreachable(); /* not reached */ > +} > + > static void __init handle_file_info(const CHAR16 *name, > const struct file *file, const char *options) > { > @@ -685,42 +723,6 @@ static void __init pre_parse(const struc > " last line will be ignored.\r\n"); > } > > -static char *__init get_value(const struct file *cfg, const char *section, > - const char *item) > -{ > - char *ptr = cfg->str, *end = ptr + cfg->size; > - size_t slen = section ? strlen(section) : 0, ilen = strlen(item); > - bool match = !slen; > - > - for ( ; ptr < end; ++ptr ) > - { > - switch ( *ptr ) > - { > - case 0: > - continue; > - case '[': > - if ( !slen ) > - break; > - if ( match ) > - return NULL; > - match = strncmp(++ptr, section, slen) == 0 && ptr[slen] == ']'; > - break; > - default: > - if ( match && strncmp(ptr, item, ilen) == 0 && ptr[ilen] == '=' ) > - { > - ptr += ilen + 1; > - /* strip off any leading spaces */ > - while ( *ptr && isspace(*ptr) ) > - ptr++; > - return ptr; > - } > - break; > - } > - ptr += strlen(ptr); > - } > - return NULL; > -} > - > static void __init efi_init(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) > { > efi_ih = ImageHandle; > @@ -1114,21 +1116,6 @@ static void __init efi_exit_boot(EFI_HAN > efi_fw_vendor = (void *)efi_fw_vendor + DIRECTMAP_VIRT_START; > } > > -static int __init __maybe_unused set_color(u32 mask, int bpp, u8 *pos, u8 *sz) > -{ > - if ( bpp < 0 ) > - return bpp; > - if ( !mask ) > - return -EINVAL; > - for ( *pos = 0; !(mask & 1); ++*pos ) > - mask >>= 1; > - for ( *sz = 0; mask & 1; ++*sz) > - mask >>= 1; > - if ( mask ) > - return -EINVAL; > - return max(*pos + *sz, bpp); > -} > - > void EFIAPI __init noreturn > efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) > { > >
On 03.12.2021 17:10, Luca Fancellu wrote: >> On 3 Dec 2021, at 10:56, Jan Beulich <jbeulich@suse.com> wrote: >> >> When it was introduced, it was imo placed way too high up, making it >> necessary to forward-declare way too many static functions. Move it down >> together with >> - the efi_check_dt_boot() stub, which afaict was deliberately placed >> immediately ahead of the #include, >> - blexit(), because of its use of the efi_arch_blexit() hook. >> Move up get_value() and set_color() to before the inclusion so their >> forward declarations can also be zapped. >> > > With the “const” attribute now some function in this serie are above the char line > limit, however everything looks fine. I wonder which part of this patch you're referring to. I don't recall any addition of const here - I think I'm strictly only moving code around some code and delete some declarations. I've further checked the code being moved, and I couldn't spot any line going beyond 80 chars. > Reviewed-by: Luca Fancellu <luca.fancellu@arm.com> Thanks. Jan
> On 6 Dec 2021, at 07:27, Jan Beulich <jbeulich@suse.com> wrote: > > On 03.12.2021 17:10, Luca Fancellu wrote: >>> On 3 Dec 2021, at 10:56, Jan Beulich <jbeulich@suse.com> wrote: >>> >>> When it was introduced, it was imo placed way too high up, making it >>> necessary to forward-declare way too many static functions. Move it down >>> together with >>> - the efi_check_dt_boot() stub, which afaict was deliberately placed >>> immediately ahead of the #include, >>> - blexit(), because of its use of the efi_arch_blexit() hook. >>> Move up get_value() and set_color() to before the inclusion so their >>> forward declarations can also be zapped. >>> >> >> With the “const” attribute now some function in this serie are above the char line >> limit, however everything looks fine. > > I wonder which part of this patch you're referring to. I don't recall any > addition of const here - I think I'm strictly only moving code around some > code and delete some declarations. I've further checked the code being > moved, and I couldn't spot any line going beyond 80 chars. Yes sorry, it was a comment for the second patch, where you constify a function parameter > >> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com> > > Thanks. > > Jan
--- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -111,25 +111,10 @@ struct file { }; }; -static CHAR16 *FormatDec(UINT64 Val, CHAR16 *Buffer); -static CHAR16 *FormatHex(UINT64 Val, UINTN Width, CHAR16 *Buffer); -static void DisplayUint(UINT64 Val, INTN Width); -static CHAR16 *wstrcpy(CHAR16 *d, const CHAR16 *s); -static void PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode); -static char *get_value(const struct file *cfg, const char *section, - const char *item); -static char *split_string(char *s); -static CHAR16 *s2w(union string *str); -static char *w2s(const union string *str); -static EFI_FILE_HANDLE get_parent_handle(EFI_LOADED_IMAGE *loaded_image, - CHAR16 **leaf); static bool read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, struct file *file, const char *options); static bool read_section(const EFI_LOADED_IMAGE *image, const CHAR16 *name, struct file *file, const char *options); -static size_t wstrlen(const CHAR16 * s); -static int set_color(u32 mask, int bpp, u8 *pos, u8 *sz); -static bool match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2); static void efi_init(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable); static void efi_console_set_mode(void); @@ -168,19 +153,6 @@ static void __init PrintErr(const CHAR16 StdErr->OutputString(StdErr, (CHAR16 *)s ); } -#ifndef CONFIG_HAS_DEVICE_TREE -static int __init efi_check_dt_boot(EFI_LOADED_IMAGE *loaded_image) -{ - return 0; -} -#endif - -/* - * Include architecture specific implementation here, which references the - * static globals defined above. - */ -#include "efi-boot.h" - static CHAR16 *__init FormatDec(UINT64 Val, CHAR16 *Buffer) { if ( Val >= 10 ) @@ -291,30 +263,6 @@ static bool __init match_guid(const EFI_ !memcmp(guid1->Data4, guid2->Data4, sizeof(guid1->Data4)); } -void __init noreturn blexit(const CHAR16 *str) -{ - if ( str ) - PrintStr(str); - PrintStr(newline); - - if ( !efi_bs ) - efi_arch_halt(); - - if ( cfg.need_to_free ) - efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size)); - if ( kernel.need_to_free ) - efi_bs->FreePages(kernel.addr, PFN_UP(kernel.size)); - if ( ramdisk.need_to_free ) - efi_bs->FreePages(ramdisk.addr, PFN_UP(ramdisk.size)); - if ( xsm.need_to_free ) - efi_bs->FreePages(xsm.addr, PFN_UP(xsm.size)); - - efi_arch_blexit(); - - efi_bs->Exit(efi_ih, EFI_SUCCESS, 0, NULL); - unreachable(); /* not reached */ -} - /* generic routine for printing error messages */ static void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode) { @@ -542,6 +490,7 @@ static CHAR16 *__init point_tail(CHAR16 break; } } + /* * Truncate string at first space, and return pointer * to remainder of string, if any/ NULL returned if @@ -559,6 +508,95 @@ static char * __init split_string(char * return NULL; } +static char *__init get_value(const struct file *cfg, const char *section, + const char *item) +{ + char *ptr = cfg->str, *end = ptr + cfg->size; + size_t slen = section ? strlen(section) : 0, ilen = strlen(item); + bool match = !slen; + + for ( ; ptr < end; ++ptr ) + { + switch ( *ptr ) + { + case 0: + continue; + case '[': + if ( !slen ) + break; + if ( match ) + return NULL; + match = strncmp(++ptr, section, slen) == 0 && ptr[slen] == ']'; + break; + default: + if ( match && strncmp(ptr, item, ilen) == 0 && ptr[ilen] == '=' ) + { + ptr += ilen + 1; + /* strip off any leading spaces */ + while ( *ptr && isspace(*ptr) ) + ptr++; + return ptr; + } + break; + } + ptr += strlen(ptr); + } + return NULL; +} + +static int __init __maybe_unused set_color(uint32_t mask, int bpp, + uint8_t *pos, uint8_t *sz) +{ + if ( bpp < 0 ) + return bpp; + if ( !mask ) + return -EINVAL; + for ( *pos = 0; !(mask & 1); ++*pos ) + mask >>= 1; + for ( *sz = 0; mask & 1; ++*sz) + mask >>= 1; + if ( mask ) + return -EINVAL; + return max(*pos + *sz, bpp); +} + +#ifndef CONFIG_HAS_DEVICE_TREE +static int __init efi_check_dt_boot(EFI_LOADED_IMAGE *loaded_image) +{ + return 0; +} +#endif + +/* + * Include architecture specific implementation here, which references the + * static globals defined above. + */ +#include "efi-boot.h" + +void __init noreturn blexit(const CHAR16 *str) +{ + if ( str ) + PrintStr(str); + PrintStr(newline); + + if ( !efi_bs ) + efi_arch_halt(); + + if ( cfg.need_to_free ) + efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size)); + if ( kernel.need_to_free ) + efi_bs->FreePages(kernel.addr, PFN_UP(kernel.size)); + if ( ramdisk.need_to_free ) + efi_bs->FreePages(ramdisk.addr, PFN_UP(ramdisk.size)); + if ( xsm.need_to_free ) + efi_bs->FreePages(xsm.addr, PFN_UP(xsm.size)); + + efi_arch_blexit(); + + efi_bs->Exit(efi_ih, EFI_SUCCESS, 0, NULL); + unreachable(); /* not reached */ +} + static void __init handle_file_info(const CHAR16 *name, const struct file *file, const char *options) { @@ -685,42 +723,6 @@ static void __init pre_parse(const struc " last line will be ignored.\r\n"); } -static char *__init get_value(const struct file *cfg, const char *section, - const char *item) -{ - char *ptr = cfg->str, *end = ptr + cfg->size; - size_t slen = section ? strlen(section) : 0, ilen = strlen(item); - bool match = !slen; - - for ( ; ptr < end; ++ptr ) - { - switch ( *ptr ) - { - case 0: - continue; - case '[': - if ( !slen ) - break; - if ( match ) - return NULL; - match = strncmp(++ptr, section, slen) == 0 && ptr[slen] == ']'; - break; - default: - if ( match && strncmp(ptr, item, ilen) == 0 && ptr[ilen] == '=' ) - { - ptr += ilen + 1; - /* strip off any leading spaces */ - while ( *ptr && isspace(*ptr) ) - ptr++; - return ptr; - } - break; - } - ptr += strlen(ptr); - } - return NULL; -} - static void __init efi_init(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) { efi_ih = ImageHandle; @@ -1114,21 +1116,6 @@ static void __init efi_exit_boot(EFI_HAN efi_fw_vendor = (void *)efi_fw_vendor + DIRECTMAP_VIRT_START; } -static int __init __maybe_unused set_color(u32 mask, int bpp, u8 *pos, u8 *sz) -{ - if ( bpp < 0 ) - return bpp; - if ( !mask ) - return -EINVAL; - for ( *pos = 0; !(mask & 1); ++*pos ) - mask >>= 1; - for ( *sz = 0; mask & 1; ++*sz) - mask >>= 1; - if ( mask ) - return -EINVAL; - return max(*pos + *sz, bpp); -} - void EFIAPI __init noreturn efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) {
When it was introduced, it was imo placed way too high up, making it necessary to forward-declare way too many static functions. Move it down together with - the efi_check_dt_boot() stub, which afaict was deliberately placed immediately ahead of the #include, - blexit(), because of its use of the efi_arch_blexit() hook. Move up get_value() and set_color() to before the inclusion so their forward declarations can also be zapped. Signed-off-by: Jan Beulich <jbeulich@suse.com>