efi_loader: Add an EFI_CALL() macro
PATCH 1/3 efi_loader: Add an EFI_CALL() macro
Rather than open-coding EFI_EXIT() + callback + EFI_ENTRY(), introduce an EFI_CALL() macro. This makes callbacks into UEFI world (of which there will be more in the future) more concise and easier to locate in the code.
--- include/efi_loader.h | 17 +++++++++++++++++ lib/efi_loader/efi_boottime.c | 4 +--- 2 files changed, 18 insertions(+), 3 deletions(-)
include/efi_loader.h +17 -0
@@ -15,16 +15,33 @@#include <linux/list.h> +/* +* Enter the u-boot world from UEFI: +*/ #define EFI_ENTRY (format, ...) do { \ efi_restore_gd(); \ debug("EFI: Entry %s(" format ")\n", __func__, ##__VA_ARGS__); \ } while(0) +/* +* Exit the u-boot world back to UEFI: +*/ #define EFI_EXIT (ret) ({ \ debug("EFI: Exit: %s: %u\n", __func__, (u32)((ret) & ~EFI_ERROR_MASK)); \ efi_exit_func(ret); \ }) +/* +* Callback into UEFI world from u-boot: +*/ +#define EFI_CALL (exp) do { \ + debug("EFI: Call: %s\n", #exp); \ + efi_exit_func(EFI_SUCCESS); \ + exp; \ + efi_restore_gd(); \ + debug("EFI: Return From: %s\n", #exp); \ + } while(0) +extern struct efi_runtime_services efi_runtime_services;extern struct efi_system_table systab;
lib/efi_loader/efi_boottime.c +1 -3
@@ -120,9 +120,7 @@ void efi_signal_event(struct efi_event *event)return ; event->signaled =1 ;if (event->type &EVT_NOTIFY_SIGNAL ) { -EFI_EXIT (EFI_SUCCESS ); - event->notify_function (event, event->notify_context); -EFI_ENTRY ("returning from notification function" ); +EFI_CALL (event->notify_function (event, event->notify_context)); } }
Signature · 1 line
2.13.0
PATCH 2/3 efi_loader: add checking for incorrect use of EFI_ENTRY/EXIT
Missing an EFI_ENTRY() or doubling up EFI_EXIT() leads to non-obvious crashes. Let's add some error checking.
--- include/efi_loader.h | 5 +++++ lib/efi_loader/efi_boottime.c | 13 +++++++++++++ 2 files changed, 18 insertions(+)
include/efi_loader.h +5 -0
@@ -15,11 +15,13 @@#include <linux/list.h> +int __efi_check_nesting (int delta);/* * Enter the u-boot world from UEFI: */ #define EFI_ENTRY (format, ...) do { \ efi_restore_gd(); \ + assert(__efi_check_nesting(+1)); \ debug("EFI: Entry %s(" format ")\n", __func__, ##__VA_ARGS__); \ } while(0)
@@ -28,6 +30,7 @@*/ #define EFI_EXIT (ret) ({ \ debug("EFI: Exit: %s: %u\n", __func__, (u32)((ret) & ~EFI_ERROR_MASK)); \ + assert(__efi_check_nesting(-1)); \ efi_exit_func(ret); \ })
@@ -36,10 +39,12 @@*/ #define EFI_CALL (exp) do { \ debug("EFI: Call: %s\n", #exp); \ + assert(__efi_check_nesting(-1)); \ efi_exit_func(EFI_SUCCESS); \ exp; \ efi_restore_gd(); \ debug("EFI: Return From: %s\n", #exp); \ + assert(__efi_check_nesting(+1)); \ } while(0)extern struct efi_runtime_services efi_runtime_services;
lib/efi_loader/efi_boottime.c +13 -0
@@ -57,6 +57,17 @@ void efi_save_gd(void)#endif } +/* Check for incorrect use of EFI_ENTRY() / EFI_EXIT() */ +int __efi_check_nesting (int delta) +{ +static int entry_count; +/* post-increment, pre-decrement: */ +if (delta >0 ) +return entry_count++ ==0 ; +else +return --entry_count ==0 ; +} +/* Called on every callback entry */ void efi_restore_gd (void ) {
@@ -716,7 +727,9 @@ static efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,return EFI_EXIT (info->exit_status); } +__efi_check_nesting (-1 );entry (image_handle, &systab); +__efi_check_nesting (+1 );/* Should usually never get here */ return EFI_EXIT (EFI_SUCCESS );
Signature · 1 line
2.13.0
PATCH 2/3 efi_loader: add checking for incorrect use of EFI_ENTRY/EXIT
Quoted 53 lines · Rob Clark
Missing an
EFI_ENTRY()or doubling upEFI_EXIT()leads to non-obvious crashes. Let's add some error checking.Signed-off-by Rob Clark <[email protected]>--- include/efi_loader.h | 5 +++++ lib/efi_loader/efi_boottime.c | 13 +++++++++++++ 2 files changed, 18 insertions(+)include/efi_loader.h +5
-0@@ -15,11 +15,13 @@#include <linux/list.h> +int __efi_check_nesting (int delta);/* * Enter the u-boot world from UEFI: */ #define EFI_ENTRY (format, ...) do { \ efi_restore_gd(); \ + assert(__efi_check_nesting(+1)); \ debug("EFI: Entry %s(" format ")\n", __func__, ##__VA_ARGS__); \ } while(0)@@ -28,6 +30,7 @@*/ #define EFI_EXIT (ret) ({ \ debug("EFI: Exit: %s: %u\n", __func__, (u32)((ret) & ~EFI_ERROR_MASK)); \ + assert(__efi_check_nesting(-1)); \ efi_exit_func(ret); \ })@@ -36,10 +39,12 @@*/ #define EFI_CALL (exp) do { \ debug("EFI: Call: %s\n", #exp); \ + assert(__efi_check_nesting(-1)); \ efi_exit_func(EFI_SUCCESS); \ exp; \ efi_restore_gd(); \ debug("EFI: Return From: %s\n", #exp); \ + assert(__efi_check_nesting(+1)); \ } while(0)extern struct efi_runtime_services efi_runtime_services;lib/efi_loader/efi_boottime.c +0
-0
lib/efi_loader/efi_boottime.c void efi_save_gd(void) · line 63
Quoted 7 lines
lib/efi_loader/efi_boottime.c +4
-0@@ -57,6 +57,17 @@ void efi_save_gd(void)#endif } +/* Check for incorrect use of EFI_ENTRY() / EFI_EXIT() */ +int __efi_check_nesting (int delta) +{ +static int entry_count;
I'd prefer if we marked globals as such (read: somewhere at the top of a .c file).
Also I think this function would be better off as a static inline in a header, as it should get compressed quite well.
lib/efi_loader/efi_boottime.c void efi_save_gd(void) · line 68
Quoted 5 lines
lib/efi_loader/efi_boottime.c +5
-0+/* post-increment, pre-decrement: */ +if (delta >0 ) +return entry_count++ ==0 ; +else +return --entry_count ==0 ;
I have a hard time to wrap my head around the logic. At least, couldn't you split this into 2 functions? :)
Alex
Quoted 16 lines
lib/efi_loader/efi_boottime.c +4
-0+} +/* Called on every callback entry */ void efi_restore_gd (void ) {@@ -716,7 +727,9 @@ static efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,return EFI_EXIT (info->exit_status); } +__efi_check_nesting (-1 );entry (image_handle, &systab); +__efi_check_nesting (+1 );/* Should usually never get here */ return EFI_EXIT (EFI_SUCCESS );
PATCH 2/3 efi_loader: add checking for incorrect use of EFI_ENTRY/EXIT
Quoted 55 lines · Alexander Graf
Quoted 52 lines · Rob Clark
Missing an
EFI_ENTRY()or doubling upEFI_EXIT()leads to non-obvious crashes. Let's add some error checking.Signed-off-by Rob Clark <[email protected]>--- include/efi_loader.h | 5 +++++ lib/efi_loader/efi_boottime.c | 13 +++++++++++++ 2 files changed, 18 insertions(+)include/efi_loader.h +1
-0@@ -15,11 +15,13 @@#include <linux/list.h> +int __efi_check_nesting(int delta); /* * Enter the u-boot world from UEFI: */ #define EFI_ENTRY (format, ...) do { \ efi_restore_gd(); \ + assert(__efi_check_nesting(+1)); \ debug("EFI: Entry %s(" format ")\n", __func__, ##__VA_ARGS__); \ } while(0) @@ -28,6 +30,7 @@ */#define EFI_EXIT (ret) ({ \ debug("EFI: Exit: %s: %u\n", __func__, (u32)((ret) &~EFI_ERROR_MASK)); \
include/efi_loader.h +3
-0+assert(__efi_check_nesting(-1)); \ efi_exit_func(ret); \ }) @@ -36,10 +39,12 @@ */ #define EFI_CALL (exp) do { \ debug("EFI: Call: %s\n", #exp); \ + assert(__efi_check_nesting(-1)); \ efi_exit_func(EFI_SUCCESS); \ exp; \ efi_restore_gd(); \ debug("EFI: Return From: %s\n", #exp); \ + assert(__efi_check_nesting(+1)); \ } while(0)extern struct efi_runtime_services efi_runtime_services;lib/efi_loader/efi_boottime.c +0
-0@@ -57,6 +57,17 @@ void efi_save_gd(void)#endif }
Quoted 8 lines
lib/efi_loader/efi_boottime.c void efi_save_gd(void) · line 63
Quoted 4 lines
lib/efi_loader/efi_boottime.c +3
-0+/* Check for incorrect use of EFI_ENTRY() / EFI_EXIT() */ +int __efi_check_nesting(int delta) +{ +static int entry_count;I'd prefer if we marked globals as such (read: somewhere at the top of a .c file).
hmm, well that does increase the scope unnecessarily, which is why I made it static inside the fxn. But if you can prefer I guess I can put it just above __efi_check_nesting().
Quoted 2 lines
Also I think this function would be better off as a static inline in a header, as it should get compressed quite well.
That would mean making entry_count also non-static (ie. broader than function or file level scope), otherwise you end up with different copies of it in each .o file. (Which would mostly work, but it would fall apart in confusing ways in some edge cases..)
Quoted 9 lines
lib/efi_loader/efi_boottime.c void efi_save_gd(void) · line 68
Quoted 5 lines
lib/efi_loader/efi_boottime.c +5
-0+/* post-increment, pre-decrement: */ +if (delta >0 ) +return entry_count++ ==0 ; +else +return --entry_count ==0 ;I have a hard time to wrap my head around the logic. At least, couldn't you split this into 2 functions? :)
Would that make the logic more clear, or just move it far enough apart that you don't notice it is confusing ;-)
BR,
-R
Quoted 19 lines
Alex
Quoted 15 lines
lib/efi_loader/efi_boottime.c +3
-0+} +/* Called on every callback entry */ void efi_restore_gd (void ) {@@ -716,7 +727,9 @@ static efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle, return EFI_EXIT(info->exit_status); } +__efi_check_nesting (-1 );entry (image_handle, &systab); +__efi_check_nesting (+1 );/* Should usually never get here */ return EFI_EXIT (EFI_SUCCESS );
PATCH 2/3 efi_loader: add checking for incorrect use of EFI_ENTRY/EXIT
Quoted 65 lines · Rob Clark
Quoted 55 lines · Alexander Graf
Quoted 52 lines · Rob Clark
Missing an
EFI_ENTRY()or doubling upEFI_EXIT()leads to non-obvious crashes. Let's add some error checking.Signed-off-by Rob Clark <[email protected]>--- include/efi_loader.h | 5 +++++ lib/efi_loader/efi_boottime.c | 13 +++++++++++++ 2 files changed, 18 insertions(+)include/efi_loader.h +1
-0@@ -15,11 +15,13 @@#include <linux/list.h> +int __efi_check_nesting(int delta); /* * Enter the u-boot world from UEFI: */ #define EFI_ENTRY (format, ...) do { \ efi_restore_gd(); \ + assert(__efi_check_nesting(+1)); \ debug("EFI: Entry %s(" format ")\n", __func__, ##__VA_ARGS__); \ } while(0) @@ -28,6 +30,7 @@ */#define EFI_EXIT (ret) ({ \ debug("EFI: Exit: %s: %u\n", __func__, (u32)((ret) &~EFI_ERROR_MASK)); \
include/efi_loader.h +3
-0+assert(__efi_check_nesting(-1)); \ efi_exit_func(ret); \ }) @@ -36,10 +39,12 @@ */ #define EFI_CALL (exp) do { \ debug("EFI: Call: %s\n", #exp); \ + assert(__efi_check_nesting(-1)); \ efi_exit_func(EFI_SUCCESS); \ exp; \ efi_restore_gd(); \ debug("EFI: Return From: %s\n", #exp); \ + assert(__efi_check_nesting(+1)); \ } while(0)extern struct efi_runtime_services efi_runtime_services;lib/efi_loader/efi_boottime.c +0
-0@@ -57,6 +57,17 @@ void efi_save_gd(void)#endif }Quoted 8 lines
lib/efi_loader/efi_boottime.c void efi_save_gd(void) · line 63
Quoted 4 lines
lib/efi_loader/efi_boottime.c +3
-0+/* Check for incorrect use of EFI_ENTRY() / EFI_EXIT() */ +int __efi_check_nesting(int delta) +{ +static int entry_count;I'd prefer if we marked globals as such (read: somewhere at the top of a .c file).
Quoted 3 lines
hmm, well that does increase the scope unnecessarily, which is why I made it static inside the fxn. But if you can prefer I guess I can put it just above
__efi_check_nesting().
It's a matter of taste, but in general I find it very useful to know at a glimpse where .bss, .rodata and .data come from.
Quoted 5 lines
Quoted 3 lines
Also I think this function would be better off as a static inline in a header, as it should get compressed quite well.
That would mean making
entry_countalso non-static (ie. broader than
Ah, no, entry_count would still be in efi_boottime.c, but the function using it would be in a .h file. That way we don't need to do the register save/restore dance we need to do for full remote function calls.
Quoted 11 lines
function or file level scope), otherwise you end up with different copies of it in each .o file. (Which would mostly work, but it would fall apart in confusing ways in some edge cases..)
Quoted 7 lines
Quoted 5 lines
lib/efi_loader/efi_boottime.c +5
-0+/* post-increment, pre-decrement: */ +if (delta >0 ) +return entry_count++ ==0 ; +else +return --entry_count ==0 ;
Quoted 5 lines
Quoted 2 lines
I have a hard time to wrap my head around the logic. At least, couldn't you split this into 2 functions? :)
Would that make the logic more clear, or just move it far enough apart that you don't notice it is confusing ;-)
I can't really tell yet, but I'd say it's worth a try :).
Alex
PATCH 2/3 efi_loader: add checking for incorrect use of EFI_ENTRY/EXIT
Quoted 89 lines · Alexander Graf
Quoted 73 lines · Rob Clark
Quoted 61 lines · Alexander Graf
Quoted 57 lines · Rob Clark
Missing an
EFI_ENTRY()or doubling upEFI_EXIT()leads to non-obvious crashes. Let's add some error checking.Signed-off-by Rob Clark <[email protected]>--- include/efi_loader.h | 5 +++++ lib/efi_loader/efi_boottime.c | 13 +++++++++++++ 2 files changed, 18 insertions(+)include/efi_loader.h +1
-0@@ -15,11 +15,13 @@#include <linux/list.h> +int __efi_check_nesting(int delta); /* * Enter the u-boot world from UEFI: */ #define EFI_ENTRY (format, ...) do { \ efi_restore_gd(); \ + assert(__efi_check_nesting(+1)); \ debug("EFI: Entry %s(" format ")\n", __func__, ##__VA_ARGS__); \ } while(0) @@ -28,6 +30,7 @@ */ #define EFI_EXIT(ret) ({ \ debug("EFI: Exit: %s: %u\n", __func__, (u32)((ret) &~EFI_ERROR_MASK)); \
include/efi_loader.h +3
-0+assert(__efi_check_nesting(-1)); \ efi_exit_func(ret); \ }) @@ -36,10 +39,12 @@ */ #define EFI_CALL (exp) do { \ debug("EFI: Call: %s\n", #exp); \ + assert(__efi_check_nesting(-1)); \ efi_exit_func(EFI_SUCCESS); \ exp; \ efi_restore_gd(); \ debug("EFI: Return From: %s\n", #exp); \ + assert(__efi_check_nesting(+1)); \ } while(0)extern struct efi_runtime_services efi_runtime_services;lib/efi_loader/efi_boottime.c +1
-0@@ -57,6 +57,17 @@ void efi_save_gd(void)#endif } +/* Check for incorrect use of EFI_ENTRY() / EFI_EXIT() */ +int __efi_check_nesting(int delta)Quoted 8 lines
lib/efi_loader/efi_boottime.c void efi_save_gd(void) · line 63
Quoted 2 lines
lib/efi_loader/efi_boottime.c +2
-0+{ +static int entry_count;I'd prefer if we marked globals as such (read: somewhere at the top of a .c
file).Quoted 3 lines
hmm, well that does increase the scope unnecessarily, which is why I made it static inside the fxn. But if you can prefer I guess I can put it just above
__efi_check_nesting().It's a matter of taste, but in general I find it very useful to know at a glimpse where .bss, .rodata and .data come from.
Quoted 5 lines
Quoted 3 lines
Also I think this function would be better off as a static inline in a header, as it should get compressed quite well.
Quoted 6 lines
Quoted 1 line
That would mean making
entry_countalso non-static (ie. broader thanAh, no,
entry_countwould still be inefi_boottime.c, but the function using it would be in a .h file. That way we don't need to do the register save/restore dance we need to do for full remote function calls.
Ok.. I'm thinking about just moving these calls into efi_restore_gd()/efi_exit_func() but leaving the asserts in the macro (so the assert reflects the proper file/line). Minor downside is the assert would come before the debug() print, but since it has file/line I guess that isn't really important.
Fortunately the one place we need to special-case call the check_nesting() (ie. efi_start_image()) is in the same object file.
I'll give that a go this evening and see how it turns out.
BR,
-R
PATCH 2/3 efi_loader: add checking for incorrect use of EFI_ENTRY/EXIT
In reply to Rob ClarkQuoted 72 lines · Rob Clark
Missing an
EFI_ENTRY()or doubling upEFI_EXIT()leads to non-obvious crashes. Let's add some error checking.Signed-off-by Rob Clark <[email protected]>--- include/efi_loader.h | 5 +++++ lib/efi_loader/efi_boottime.c | 13 +++++++++++++ 2 files changed, 18 insertions(+)include/efi_loader.h +5
-0@@ -15,11 +15,13 @@#include <linux/list.h> +int __efi_check_nesting (int delta);/* * Enter the u-boot world from UEFI: */ #define EFI_ENTRY (format, ...) do { \ efi_restore_gd(); \ + assert(__efi_check_nesting(+1)); \ debug("EFI: Entry %s(" format ")\n", __func__, ##__VA_ARGS__); \ } while(0)@@ -28,6 +30,7 @@*/ #define EFI_EXIT (ret) ({ \ debug("EFI: Exit: %s: %u\n", __func__, (u32)((ret) & ~EFI_ERROR_MASK)); \ + assert(__efi_check_nesting(-1)); \ efi_exit_func(ret); \ })@@ -36,10 +39,12 @@*/ #define EFI_CALL (exp) do { \ debug("EFI: Call: %s\n", #exp); \ + assert(__efi_check_nesting(-1)); \ efi_exit_func(EFI_SUCCESS); \ exp; \ efi_restore_gd(); \ debug("EFI: Return From: %s\n", #exp); \ + assert(__efi_check_nesting(+1)); \ } while(0)extern struct efi_runtime_services efi_runtime_services;lib/efi_loader/efi_boottime.c +11
-0@@ -57,6 +57,17 @@ void efi_save_gd(void)#endif } +/* Check for incorrect use of EFI_ENTRY() / EFI_EXIT() */ +int __efi_check_nesting (int delta) +{ +static int entry_count; +/* post-increment, pre-decrement: */ +if (delta >0 ) +return entry_count++ ==0 ; +else +return --entry_count ==0 ; +} +/* Called on every callback entry */ void efi_restore_gd (void ) {
lib/efi_loader/efi_boottime.c static efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle, · line 735
Quoted 9 lines
lib/efi_loader/efi_boottime.c +2
-0@@ -716,7 +727,9 @@ static efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,return EFI_EXIT (info->exit_status); } +__efi_check_nesting (-1 );entry (image_handle, &systab); +__efi_check_nesting (+1 );/* Should usually never get here */ return EFI_EXIT (EFI_SUCCESS );
PATCH 3/3 efi_loader: indent entry/exit prints to show nesting level
In reply to Rob ClarkThis should make it easier to see when a callback back to UEFI world calls back in to the u-boot world, and generally match up EFI_ENTRY() and EFI_EXIT() calls.
--- include/efi_loader.h | 12 ++++++++---- lib/efi_loader/efi_boottime.c | 28 ++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 4 deletions(-)
include/efi_loader.h +8 -4
@@ -16,20 +16,24 @@#include <linux/list.h> int __efi_check_nesting (int delta); +const char *__efi_nesting_level (int delta); +/* * Enter the u-boot world from UEFI: */ #define EFI_ENTRY (format, ...) do { \ efi_restore_gd(); \ + debug("%sEFI: Entry %s(" format ")\n", __efi_nesting_level(1), \ + __func__, ##__VA_ARGS__); \ assert(__efi_check_nesting(+1)); \ - debug("EFI: Entry %s(" format ")\n", __func__, ##__VA_ARGS__); \ } while(0)/* * Exit the u-boot world back to UEFI: */ #define EFI_EXIT (ret) ({ \ - debug("EFI: Exit: %s: %u\n", __func__, (u32)((ret) & ~EFI_ERROR_MASK)); \ + debug("%sEFI: Exit: %s: %u\n", __efi_nesting_level(-1), \ + __func__, (u32)((ret) & ~EFI_ERROR_MASK)); \ assert(__efi_check_nesting(-1)); \ efi_exit_func(ret); \ })
@@ -38,12 +42,12 @@ int __efi_check_nesting(int delta);* Callback into UEFI world from u-boot: */ #define EFI_CALL (exp) do { \ - debug("EFI: Call: %s\n", #exp); \ + debug("%sEFI: Call: %s\n", __efi_nesting_level(1), #exp); \ assert(__efi_check_nesting(-1)); \ efi_exit_func(EFI_SUCCESS); \ exp; \ efi_restore_gd(); \ - debug("EFI: Return From: %s\n", #exp); \ + debug("%sEFI: Return From: %s\n", __efi_nesting_level(-1), #exp); \ assert(__efi_check_nesting(+1)); \ } while(0)
lib/efi_loader/efi_boottime.c +28 -0
@@ -92,6 +92,34 @@ efi_status_t efi_exit_func(efi_status_t ret)return ret; } +/* +* Two spaces per indent level, maxing out at 10.. which ought to be +* enough for anyone ;-) +*/ +static const char *indent_string (int level) +{ +static const char *indent =" " ; +const int max =strlen (indent); + level =min (max, level *2 ); +return &indent[max - level]; +} + +const char *__efi_nesting_level (int delta) +{ +static int nesting_level; +const char *s; +/* post-increment, pre-decrement */ +if (delta >0 ) { + s =indent_string (nesting_level); + nesting_level += delta; + }else { + nesting_level += delta; + s =indent_string (nesting_level); +assert (nesting_level >=0 ); + } +return s; +} +/* Low 32 bit */ #define EFI_LOW32 (a) (a & 0xFFFFFFFFULL)/* High 32 bit */
Signature · 1 line
2.13.0
PATCH 3/3 efi_loader: indent entry/exit prints to show nesting level
Quoted 79 lines · Rob Clark
This should make it easier to see when a callback back to UEFI world calls back in to the u-boot world, and generally match up
EFI_ENTRY()andEFI_EXIT()calls.Signed-off-by Rob Clark <[email protected]>--- include/efi_loader.h | 12 ++++++++---- lib/efi_loader/efi_boottime.c | 28 ++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 4 deletions(-)include/efi_loader.h +8
-4@@ -16,20 +16,24 @@#include <linux/list.h> int __efi_check_nesting (int delta); +const char *__efi_nesting_level (int delta); +/* * Enter the u-boot world from UEFI: */ #define EFI_ENTRY (format, ...) do { \ efi_restore_gd(); \ + debug("%sEFI: Entry %s(" format ")\n", __efi_nesting_level(1), \ + __func__, ##__VA_ARGS__); \ assert(__efi_check_nesting(+1)); \ - debug("EFI: Entry %s(" format ")\n", __func__, ##__VA_ARGS__); \ } while(0)/* * Exit the u-boot world back to UEFI: */ #define EFI_EXIT (ret) ({ \ - debug("EFI: Exit: %s: %u\n", __func__, (u32)((ret) & ~EFI_ERROR_MASK)); \ + debug("%sEFI: Exit: %s: %u\n", __efi_nesting_level(-1), \ + __func__, (u32)((ret) & ~EFI_ERROR_MASK)); \ assert(__efi_check_nesting(-1)); \ efi_exit_func(ret); \ })@@ -38,12 +42,12 @@ int __efi_check_nesting(int delta);* Callback into UEFI world from u-boot: */ #define EFI_CALL (exp) do { \ - debug("EFI: Call: %s\n", #exp); \ + debug("%sEFI: Call: %s\n", __efi_nesting_level(1), #exp); \ assert(__efi_check_nesting(-1)); \ efi_exit_func(EFI_SUCCESS); \ exp; \ efi_restore_gd(); \ - debug("EFI: Return From: %s\n", #exp); \ + debug("%sEFI: Return From: %s\n", __efi_nesting_level(-1), #exp); \ assert(__efi_check_nesting(+1)); \ } while(0)lib/efi_loader/efi_boottime.c +15
-0@@ -92,6 +92,34 @@ efi_status_t efi_exit_func(efi_status_t ret)return ret; } +/* +* Two spaces per indent level, maxing out at 10.. which ought to be +* enough for anyone ;-) +*/ +static const char *indent_string (int level) +{ +static const char *indent =" " ; +const int max =strlen (indent); + level =min (max, level *2 ); +return &indent[max - level]; +} + +const char *__efi_nesting_level (int delta) +{ +static int nesting_level;
lib/efi_loader/efi_boottime.c efi_status_t efi_exit_func(efi_status_t ret) · line 119
Quoted 10 lines
lib/efi_loader/efi_boottime.c +10
-0+const char *s; +/* post-increment, pre-decrement */ +if (delta >0 ) { + s =indent_string (nesting_level); + nesting_level += delta; + }else { + nesting_level += delta; + s =indent_string (nesting_level); +assert (nesting_level >=0 ); + }
Can you do the same here? Move globals to the top of the file and split the function?
In this particular case I think leaving them as functions is reasonable, as we should only call them when #define DEBUG is set, so we're not size constrained.
Alex
PATCH 3/3 efi_loader: indent entry/exit prints to show nesting level
In reply to Rob ClarkQuoted 86 lines · Rob Clark
This should make it easier to see when a callback back to UEFI world calls back in to the u-boot world, and generally match up
EFI_ENTRY()andEFI_EXIT()calls.Signed-off-by Rob Clark <[email protected]>--- include/efi_loader.h | 12 ++++++++---- lib/efi_loader/efi_boottime.c | 28 ++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 4 deletions(-)include/efi_loader.h +8
-4@@ -16,20 +16,24 @@#include <linux/list.h> int __efi_check_nesting (int delta); +const char *__efi_nesting_level (int delta); +/* * Enter the u-boot world from UEFI: */ #define EFI_ENTRY (format, ...) do { \ efi_restore_gd(); \ + debug("%sEFI: Entry %s(" format ")\n", __efi_nesting_level(1), \ + __func__, ##__VA_ARGS__); \ assert(__efi_check_nesting(+1)); \ - debug("EFI: Entry %s(" format ")\n", __func__, ##__VA_ARGS__); \ } while(0)/* * Exit the u-boot world back to UEFI: */ #define EFI_EXIT (ret) ({ \ - debug("EFI: Exit: %s: %u\n", __func__, (u32)((ret) & ~EFI_ERROR_MASK)); \ + debug("%sEFI: Exit: %s: %u\n", __efi_nesting_level(-1), \ + __func__, (u32)((ret) & ~EFI_ERROR_MASK)); \ assert(__efi_check_nesting(-1)); \ efi_exit_func(ret); \ })@@ -38,12 +42,12 @@ int __efi_check_nesting(int delta);* Callback into UEFI world from u-boot: */ #define EFI_CALL (exp) do { \ - debug("EFI: Call: %s\n", #exp); \ + debug("%sEFI: Call: %s\n", __efi_nesting_level(1), #exp); \ assert(__efi_check_nesting(-1)); \ efi_exit_func(EFI_SUCCESS); \ exp; \ efi_restore_gd(); \ - debug("EFI: Return From: %s\n", #exp); \ + debug("%sEFI: Return From: %s\n", __efi_nesting_level(-1), #exp); \ assert(__efi_check_nesting(+1)); \ } while(0)lib/efi_loader/efi_boottime.c +21
-0@@ -92,6 +92,34 @@ efi_status_t efi_exit_func(efi_status_t ret)return ret; } +/* +* Two spaces per indent level, maxing out at 10.. which ought to be +* enough for anyone ;-) +*/ +static const char *indent_string (int level) +{ +static const char *indent =" " ; +const int max =strlen (indent); + level =min (max, level *2 ); +return &indent[max - level]; +} + +const char *__efi_nesting_level (int delta) +{ +static int nesting_level; +const char *s; +/* post-increment, pre-decrement */ +if (delta >0 ) { + s =indent_string (nesting_level); + nesting_level += delta; + }else {
lib/efi_loader/efi_boottime.c efi_status_t efi_exit_func(efi_status_t ret) · line 125
Quoted 10 lines
lib/efi_loader/efi_boottime.c +7
-0+ nesting_level += delta; + s =indent_string (nesting_level); +assert (nesting_level >=0 ); + } +return s; +} +/* Low 32 bit */ #define EFI_LOW32 (a) (a & 0xFFFFFFFFULL)/* High 32 bit */
PATCH 1/3 efi_loader: Add an EFI_CALL() macro
In reply to Rob ClarkQuoted 55 lines · Rob Clark
Rather than open-coding
EFI_EXIT()+ callback +EFI_ENTRY(), introduce anEFI_CALL()macro. This makes callbacks into UEFI world (of which there will be more in the future) more concise and easier to locate in the code.Signed-off-by Rob Clark <[email protected]>--- include/efi_loader.h | 17 +++++++++++++++++ lib/efi_loader/efi_boottime.c | 4 +--- 2 files changed, 18 insertions(+), 3 deletions(-)include/efi_loader.h +17
-0@@ -15,16 +15,33 @@#include <linux/list.h> +/* +* Enter the u-boot world from UEFI: +*/ #define EFI_ENTRY (format, ...) do { \ efi_restore_gd(); \ debug("EFI: Entry %s(" format ")\n", __func__, ##__VA_ARGS__); \ } while(0) +/* +* Exit the u-boot world back to UEFI: +*/ #define EFI_EXIT (ret) ({ \ debug("EFI: Exit: %s: %u\n", __func__, (u32)((ret) & ~EFI_ERROR_MASK)); \ efi_exit_func(ret); \ }) +/* +* Callback into UEFI world from u-boot: +*/ +#define EFI_CALL (exp) do { \ + debug("EFI: Call: %s\n", #exp); \ + efi_exit_func(EFI_SUCCESS); \ + exp; \ + efi_restore_gd(); \ + debug("EFI: Return From: %s\n", #exp); \ + } while(0) +extern struct efi_runtime_services efi_runtime_services;extern struct efi_system_table systab;lib/efi_loader/efi_boottime.c +0
-0
lib/efi_loader/efi_boottime.c void efi_signal_event(struct efi_event *event) · line 126
Quoted 10 lines
lib/efi_loader/efi_boottime.c +1
-3@@ -120,9 +120,7 @@ void efi_signal_event(struct efi_event *event)return ; event->signaled =1 ;if (event->type &EVT_NOTIFY_SIGNAL ) { -EFI_EXIT (EFI_SUCCESS ); - event->notify_function (event, event->notify_context); -EFI_ENTRY ("returning from notification function" ); +EFI_CALL (event->notify_function (event, event->notify_context)); } }