Re: [PATCH 1/3] EFI: move efi-boot.h inclusion point

[Luca Fancellu <luca.fancellu@xxxxxxx>]

> On 3 Dec 2021, at 10:56, Jan Beulich <jbeulich@xxxxxxxx> 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@xxxxxxx>

> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> --- 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)
> {
> 
>