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>
--- 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 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
© 2016 - 2026 Red Hat, Inc.