[PATCH 0/3]: EFI: some tidying

[PATCH 0/3]: EFI: some tidying

[Jan Beulich]

1: move efi-boot.h inclusion point
2: constify EFI_LOADED_IMAGE * function parameters
3: drop copy-in from QueryVariableInfo()'s OUT-only variable bouncing

Jan

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

[Jan Beulich]

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)
 {

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

[Andrew Cooper]

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

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

[Jan Beulich]

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

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

[Andrew Cooper]

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>

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

[Jan Beulich]

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

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

[Andrew Cooper]

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.

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

[Luca Fancellu]

> 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)
> {
> 
> 

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

[Jan Beulich]

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

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

[Luca Fancellu]

> 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

[PATCH 2/3] EFI: constify EFI_LOADED_IMAGE * function parameters

[Jan Beulich]

Instead of altering Arm's forward declarations, drop them. Like
elsewhere we should limit such to cases where the first use lives ahead
of the definition.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -44,20 +44,6 @@ void __flush_dcache_area(const void *vad
 
 static int get_module_file_index(const char *name, unsigned int name_len);
 static void PrintMessage(const CHAR16 *s);
-static int allocate_module_file(EFI_LOADED_IMAGE *loaded_image,
-                                EFI_FILE_HANDLE *dir_handle,
-                                const char *name,
-                                unsigned int name_len);
-static int handle_module_node(EFI_LOADED_IMAGE *loaded_image,
-                              EFI_FILE_HANDLE *dir_handle,
-                              int module_node_offset,
-                              int reg_addr_cells,
-                              int reg_size_cells,
-                              bool is_domu_module);
-static int handle_dom0less_domain_node(EFI_LOADED_IMAGE *loaded_image,
-                                       EFI_FILE_HANDLE *dir_handle,
-                                       int domain_node);
-static int efi_check_dt_boot(EFI_LOADED_IMAGE *loaded_image);
 
 #define DEVICE_TREE_GUID \
 {0xb1b621d5, 0xf19c, 0x41a5, {0x83, 0x0b, 0xd9, 0x15, 0x2c, 0x69, 0xaa, 0xe0}}
@@ -650,7 +636,7 @@ static void __init PrintMessage(const CH
  * This function allocates a binary and keeps track of its name, it returns the
  * index of the file in the modules array or a negative number on error.
  */
-static int __init allocate_module_file(EFI_LOADED_IMAGE *loaded_image,
+static int __init allocate_module_file(const EFI_LOADED_IMAGE *loaded_image,
                                        EFI_FILE_HANDLE *dir_handle,
                                        const char *name,
                                        unsigned int name_len)
@@ -713,7 +699,7 @@ static int __init allocate_module_file(E
  * for the reg property into the module DT node.
  * Returns 1 if module is multiboot,module, 0 if not, < 0 on error
  */
-static int __init handle_module_node(EFI_LOADED_IMAGE *loaded_image,
+static int __init handle_module_node(const EFI_LOADED_IMAGE *loaded_image,
                                      EFI_FILE_HANDLE *dir_handle,
                                      int module_node_offset,
                                      int reg_addr_cells,
@@ -814,7 +800,7 @@ static int __init handle_module_node(EFI
  * in the DT.
  * Returns number of multiboot,module found or negative number on error.
  */
-static int __init handle_dom0less_domain_node(EFI_LOADED_IMAGE *loaded_image,
+static int __init handle_dom0less_domain_node(const EFI_LOADED_IMAGE *loaded_image,
                                               EFI_FILE_HANDLE *dir_handle,
                                               int domain_node)
 {
@@ -862,7 +848,7 @@ static int __init handle_dom0less_domain
  * dom0 and domU guests to be loaded.
  * Returns the number of multiboot modules found or a negative number for error.
  */
-static int __init efi_check_dt_boot(EFI_LOADED_IMAGE *loaded_image)
+static int __init efi_check_dt_boot(const EFI_LOADED_IMAGE *loaded_image)
 {
     int chosen, node, addr_len, size_len;
     unsigned int i = 0, modules_found = 0;
@@ -942,7 +928,7 @@ static void __init efi_arch_halt(void)
     stop_cpu();
 }
 
-static void __init efi_arch_load_addr_check(EFI_LOADED_IMAGE *loaded_image)
+static void __init efi_arch_load_addr_check(const EFI_LOADED_IMAGE *loaded_image)
 {
     if ( (unsigned long)loaded_image->ImageBase & ((1 << 12) - 1) )
         blexit(L"Xen must be loaded at a 4 KByte boundary.");
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -709,7 +709,7 @@ static void __init efi_arch_halt(void)
         halt();
 }
 
-static void __init efi_arch_load_addr_check(EFI_LOADED_IMAGE *loaded_image)
+static void __init efi_arch_load_addr_check(const EFI_LOADED_IMAGE *loaded_image)
 {
     xen_phys_start = (UINTN)loaded_image->ImageBase;
     if ( (xen_phys_start + loaded_image->ImageSize - 1) >> 32 )
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -389,7 +389,7 @@ static unsigned int __init get_argv(unsi
     return argc;
 }
 
-static EFI_FILE_HANDLE __init get_parent_handle(EFI_LOADED_IMAGE *loaded_image,
+static EFI_FILE_HANDLE __init get_parent_handle(const EFI_LOADED_IMAGE *loaded_image,
                                                 CHAR16 **leaf)
 {
     static EFI_GUID __initdata fs_protocol = SIMPLE_FILE_SYSTEM_PROTOCOL;
@@ -561,7 +561,7 @@ static int __init __maybe_unused set_col
 }
 
 #ifndef CONFIG_HAS_DEVICE_TREE
-static int __init efi_check_dt_boot(EFI_LOADED_IMAGE *loaded_image)
+static int __init efi_check_dt_boot(const EFI_LOADED_IMAGE *loaded_image)
 {
     return 0;
 }

Re: [PATCH 2/3] EFI: constify EFI_LOADED_IMAGE * function parameters

[Luca Fancellu]

> On 3 Dec 2021, at 10:57, Jan Beulich <jbeulich@suse.com> wrote:
> 
> Instead of altering Arm's forward declarations, drop them. Like
> elsewhere we should limit such to cases where the first use lives ahead
> of the definition.
> 

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -44,20 +44,6 @@ void __flush_dcache_area(const void *vad
> 
> static int get_module_file_index(const char *name, unsigned int name_len);
> static void PrintMessage(const CHAR16 *s);
> -static int allocate_module_file(EFI_LOADED_IMAGE *loaded_image,
> -                                EFI_FILE_HANDLE *dir_handle,
> -                                const char *name,
> -                                unsigned int name_len);
> -static int handle_module_node(EFI_LOADED_IMAGE *loaded_image,
> -                              EFI_FILE_HANDLE *dir_handle,
> -                              int module_node_offset,
> -                              int reg_addr_cells,
> -                              int reg_size_cells,
> -                              bool is_domu_module);
> -static int handle_dom0less_domain_node(EFI_LOADED_IMAGE *loaded_image,
> -                                       EFI_FILE_HANDLE *dir_handle,
> -                                       int domain_node);
> -static int efi_check_dt_boot(EFI_LOADED_IMAGE *loaded_image);
> 
> #define DEVICE_TREE_GUID \
> {0xb1b621d5, 0xf19c, 0x41a5, {0x83, 0x0b, 0xd9, 0x15, 0x2c, 0x69, 0xaa, 0xe0}}
> @@ -650,7 +636,7 @@ static void __init PrintMessage(const CH
>  * This function allocates a binary and keeps track of its name, it returns the
>  * index of the file in the modules array or a negative number on error.
>  */
> -static int __init allocate_module_file(EFI_LOADED_IMAGE *loaded_image,
> +static int __init allocate_module_file(const EFI_LOADED_IMAGE *loaded_image,
>                                        EFI_FILE_HANDLE *dir_handle,
>                                        const char *name,
>                                        unsigned int name_len)
> @@ -713,7 +699,7 @@ static int __init allocate_module_file(E
>  * for the reg property into the module DT node.
>  * Returns 1 if module is multiboot,module, 0 if not, < 0 on error
>  */
> -static int __init handle_module_node(EFI_LOADED_IMAGE *loaded_image,
> +static int __init handle_module_node(const EFI_LOADED_IMAGE *loaded_image,
>                                      EFI_FILE_HANDLE *dir_handle,
>                                      int module_node_offset,
>                                      int reg_addr_cells,
> @@ -814,7 +800,7 @@ static int __init handle_module_node(EFI
>  * in the DT.
>  * Returns number of multiboot,module found or negative number on error.
>  */
> -static int __init handle_dom0less_domain_node(EFI_LOADED_IMAGE *loaded_image,
> +static int __init handle_dom0less_domain_node(const EFI_LOADED_IMAGE *loaded_image,
>                                               EFI_FILE_HANDLE *dir_handle,
>                                               int domain_node)
> {
> @@ -862,7 +848,7 @@ static int __init handle_dom0less_domain
>  * dom0 and domU guests to be loaded.
>  * Returns the number of multiboot modules found or a negative number for error.
>  */
> -static int __init efi_check_dt_boot(EFI_LOADED_IMAGE *loaded_image)
> +static int __init efi_check_dt_boot(const EFI_LOADED_IMAGE *loaded_image)
> {
>     int chosen, node, addr_len, size_len;
>     unsigned int i = 0, modules_found = 0;
> @@ -942,7 +928,7 @@ static void __init efi_arch_halt(void)
>     stop_cpu();
> }
> 
> -static void __init efi_arch_load_addr_check(EFI_LOADED_IMAGE *loaded_image)
> +static void __init efi_arch_load_addr_check(const EFI_LOADED_IMAGE *loaded_image)
> {
>     if ( (unsigned long)loaded_image->ImageBase & ((1 << 12) - 1) )
>         blexit(L"Xen must be loaded at a 4 KByte boundary.");
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -709,7 +709,7 @@ static void __init efi_arch_halt(void)
>         halt();
> }
> 
> -static void __init efi_arch_load_addr_check(EFI_LOADED_IMAGE *loaded_image)
> +static void __init efi_arch_load_addr_check(const EFI_LOADED_IMAGE *loaded_image)
> {
>     xen_phys_start = (UINTN)loaded_image->ImageBase;
>     if ( (xen_phys_start + loaded_image->ImageSize - 1) >> 32 )
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -389,7 +389,7 @@ static unsigned int __init get_argv(unsi
>     return argc;
> }
> 
> -static EFI_FILE_HANDLE __init get_parent_handle(EFI_LOADED_IMAGE *loaded_image,
> +static EFI_FILE_HANDLE __init get_parent_handle(const EFI_LOADED_IMAGE *loaded_image,
>                                                 CHAR16 **leaf)
> {
>     static EFI_GUID __initdata fs_protocol = SIMPLE_FILE_SYSTEM_PROTOCOL;
> @@ -561,7 +561,7 @@ static int __init __maybe_unused set_col
> }
> 
> #ifndef CONFIG_HAS_DEVICE_TREE
> -static int __init efi_check_dt_boot(EFI_LOADED_IMAGE *loaded_image)
> +static int __init efi_check_dt_boot(const EFI_LOADED_IMAGE *loaded_image)
> {
>     return 0;
> }
> 
> 

Ping: [PATCH 2/3] EFI: constify EFI_LOADED_IMAGE * function parameters

[Jan Beulich]

On 03.12.2021 11:57, Jan Beulich wrote:
> Instead of altering Arm's forward declarations, drop them. Like
> elsewhere we should limit such to cases where the first use lives ahead
> of the definition.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

May I please ask for an Arm side ack (or otherwise) here?

Jan

Re: Ping: [PATCH 2/3] EFI: constify EFI_LOADED_IMAGE * function parameters

[Julien Grall]

Hi Jan,

On 10/12/2021 09:44, Jan Beulich wrote:
> On 03.12.2021 11:57, Jan Beulich wrote:
>> Instead of altering Arm's forward declarations, drop them. Like
>> elsewhere we should limit such to cases where the first use lives ahead
>> of the definition.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> May I please ask for an Arm side ack (or otherwise) here?

Sorry I missed that patch. For the full patch:

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

[PATCH 3/3] EFI: drop copy-in from QueryVariableInfo()'s OUT-only variable bouncing

[Jan Beulich]

While be12fcca8b78 ("efi: fix alignment of function parameters in compat
mode") intentionally bounced them both ways to avoid any functional
change so close to the release of 4.16, the bouncing-in shouldn't really
be needed. In exchange the local variables need to gain initializers to
avoid copying back prior stack contents.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/efi/runtime.c
+++ b/xen/common/efi/runtime.c
@@ -608,7 +608,15 @@ int efi_runtime_call(struct xenpf_efi_ru
 
     case XEN_EFI_query_variable_info:
     {
-        uint64_t max_store_size, remain_store_size, max_size;
+        /*
+         * Put OUT variables on the stack to make them 8 byte aligned when
+         * called from the compat handler, as their placement in
+         * compat_pf_efi_runtime_call will make them 4 byte aligned instead
+         * and compilers may validly complain.  This is done regardless of
+         * whether called from the compat handler or not, as it's not worth
+         * the extra logic to differentiate.
+         */
+        uint64_t max_store_size = 0, remain_store_size = 0, max_size = 0;
 
         if ( op->misc & ~XEN_EFI_VARINFO_BOOT_SNAPSHOT )
             return -EINVAL;
@@ -642,21 +650,6 @@ int efi_runtime_call(struct xenpf_efi_ru
         if ( !efi_enabled(EFI_RS) || (efi_rs->Hdr.Revision >> 16) < 2 )
             return -EOPNOTSUPP;
 
-        /*
-         * Bounce the variables onto the stack to make them 8 byte aligned when
-         * called from the compat handler, as their placement in
-         * compat_pf_efi_runtime_call will make them 4 byte aligned instead and
-         * and compilers may validly complain.
-         *
-         * Note that while the function parameters are OUT only, copy the
-         * values here anyway just in case. This is done regardless of whether
-         * called from the compat handler or not, as it's not worth the extra
-         * logic to differentiate.
-         */
-        max_store_size = op->u.query_variable_info.max_store_size;
-        remain_store_size = op->u.query_variable_info.remain_store_size;
-        max_size = op->u.query_variable_info.max_size;
-
         state = efi_rs_enter();
         if ( !state.cr3 )
             return -EOPNOTSUPP;

Re: [PATCH 3/3] EFI: drop copy-in from QueryVariableInfo()'s OUT-only variable bouncing

[Luca Fancellu]

> On 3 Dec 2021, at 10:58, Jan Beulich <jbeulich@suse.com> wrote:
> 
> While be12fcca8b78 ("efi: fix alignment of function parameters in compat
> mode") intentionally bounced them both ways to avoid any functional
> change so close to the release of 4.16, the bouncing-in shouldn't really
> be needed. In exchange the local variables need to gain initializers to
> avoid copying back prior stack contents.
> 

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/common/efi/runtime.c
> +++ b/xen/common/efi/runtime.c
> @@ -608,7 +608,15 @@ int efi_runtime_call(struct xenpf_efi_ru
> 
>     case XEN_EFI_query_variable_info:
>     {
> -        uint64_t max_store_size, remain_store_size, max_size;
> +        /*
> +         * Put OUT variables on the stack to make them 8 byte aligned when
> +         * called from the compat handler, as their placement in
> +         * compat_pf_efi_runtime_call will make them 4 byte aligned instead
> +         * and compilers may validly complain.  This is done regardless of
> +         * whether called from the compat handler or not, as it's not worth
> +         * the extra logic to differentiate.
> +         */
> +        uint64_t max_store_size = 0, remain_store_size = 0, max_size = 0;
> 
>         if ( op->misc & ~XEN_EFI_VARINFO_BOOT_SNAPSHOT )
>             return -EINVAL;
> @@ -642,21 +650,6 @@ int efi_runtime_call(struct xenpf_efi_ru
>         if ( !efi_enabled(EFI_RS) || (efi_rs->Hdr.Revision >> 16) < 2 )
>             return -EOPNOTSUPP;
> 
> -        /*
> -         * Bounce the variables onto the stack to make them 8 byte aligned when
> -         * called from the compat handler, as their placement in
> -         * compat_pf_efi_runtime_call will make them 4 byte aligned instead and
> -         * and compilers may validly complain.
> -         *
> -         * Note that while the function parameters are OUT only, copy the
> -         * values here anyway just in case. This is done regardless of whether
> -         * called from the compat handler or not, as it's not worth the extra
> -         * logic to differentiate.
> -         */
> -        max_store_size = op->u.query_variable_info.max_store_size;
> -        remain_store_size = op->u.query_variable_info.remain_store_size;
> -        max_size = op->u.query_variable_info.max_size;
> -
>         state = efi_rs_enter();
>         if ( !state.cr3 )
>             return -EOPNOTSUPP;
> 
>