[U-Boot] [PATCH 1/4] efi_loader: only evaluate EFI_EXIT()'s ret once
There are a couple spots doing things like:
return EFI_EXIT(some_fxn(...));
which I handn't noticed before. With addition of printing return value in the EFI_EXIT() macro, now the fxn call was getting evaluated twice. Which we didn't really want.
Signed-off-by: Rob Clark robdclark@gmail.com --- include/efi_loader.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index f384cbbe77..9700a88d69 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -21,8 +21,9 @@ } while(0)
#define EFI_EXIT(ret) ({ \ - debug("EFI: Exit: %s: %u\n", __func__, (u32)((ret) & ~EFI_ERROR_MASK)); \ - efi_exit_func(ret); \ + efi_status_t _r = ret; \ + debug("EFI: Exit: %s: %u\n", __func__, (u32)(_r & ~EFI_ERROR_MASK)); \ + efi_exit_func(_r); \ })
extern struct efi_runtime_services efi_runtime_services;
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.
Signed-off-by: Rob Clark robdclark@gmail.com --- include/efi_loader.h | 17 +++++++++++++++++ lib/efi_loader/efi_boottime.c | 4 +--- 2 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index 9700a88d69..eb16c14b69 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -15,17 +15,34 @@
#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) ({ \ efi_status_t _r = ret; \ debug("EFI: Exit: %s: %u\n", __func__, (u32)(_r & ~EFI_ERROR_MASK)); \ efi_exit_func(_r); \ })
+/* + * 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;
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 17c531a480..849d229821 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -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)); } }
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.
Signed-off-by: Rob Clark robdclark@gmail.com
Thanks, applied to efi-next
Alex
Missing an EFI_ENTRY() or doubling up EFI_EXIT() leads to non-obvious crashes. Let's add some error checking.
Signed-off-by: Rob Clark robdclark@gmail.com --- include/efi_loader.h | 17 +++++++++------- lib/efi_loader/efi_boottime.c | 45 +++++++++++++++++++++++++++++-------------- 2 files changed, 41 insertions(+), 21 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index eb16c14b69..4262d0ac6b 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -15,11 +15,14 @@
#include <linux/list.h>
+int __efi_entry_check(void); +int __efi_exit_check(void); + /* * Enter the u-boot world from UEFI: */ #define EFI_ENTRY(format, ...) do { \ - efi_restore_gd(); \ + assert(__efi_entry_check()); \ debug("EFI: Entry %s(" format ")\n", __func__, ##__VA_ARGS__); \ } while(0)
@@ -29,7 +32,8 @@ #define EFI_EXIT(ret) ({ \ efi_status_t _r = ret; \ debug("EFI: Exit: %s: %u\n", __func__, (u32)(_r & ~EFI_ERROR_MASK)); \ - efi_exit_func(_r); \ + assert(__efi_exit_check()); \ + _r; \ })
/* @@ -37,9 +41,9 @@ */ #define EFI_CALL(exp) do { \ debug("EFI: Call: %s\n", #exp); \ - efi_exit_func(EFI_SUCCESS); \ + assert(__efi_exit_check()); \ exp; \ - efi_restore_gd(); \ + assert(__efi_entry_check()); \ debug("EFI: Return From: %s\n", #exp); \ } while(0)
@@ -139,10 +143,9 @@ void efi_timer_check(void); void *efi_load_pe(void *efi, struct efi_loaded_image *loaded_image_info); /* Called once to store the pristine gd pointer */ void efi_save_gd(void); -/* Called from EFI_ENTRY on callback entry to put gd into the gd register */ +/* Special case handler for error/abort that just tries to dtrt to get + * back to u-boot world */ void efi_restore_gd(void); -/* Called from EFI_EXIT on callback exit to restore the gd register */ -efi_status_t efi_exit_func(efi_status_t ret); /* Call this to relocate the runtime section to an address space */ void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map); /* Call this to set the current device name */ diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 849d229821..66137d4ff9 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -49,6 +49,30 @@ static struct efi_configuration_table __efi_runtime_data efi_conf_table[2]; static volatile void *efi_gd, *app_gd; #endif
+static int entry_count; + +/* Called on every callback entry */ +int __efi_entry_check(void) +{ + int ret = entry_count++ == 0; +#ifdef CONFIG_ARM + assert(efi_gd); + assert(gd != efi_gd); + gd = efi_gd; +#endif + return ret; +} + +/* Called on every callback exit */ +int __efi_exit_check(void) +{ + int ret = --entry_count == 0; +#ifdef CONFIG_ARM + gd = app_gd; +#endif + return ret; +} + /* Called from do_bootefi_exec() */ void efi_save_gd(void) { @@ -57,30 +81,21 @@ void efi_save_gd(void) #endif }
-/* Called on every callback entry */ +/* + * Special case handler for error/abort that just forces things back + * to u-boot world so we can dump out an abort msg, without any care + * about returning back to UEFI world. + */ void efi_restore_gd(void) { #ifdef CONFIG_ARM /* Only restore if we're already in EFI context */ if (!efi_gd) return; - - if (gd != efi_gd) - app_gd = gd; gd = efi_gd; #endif }
-/* Called on every callback exit */ -efi_status_t efi_exit_func(efi_status_t ret) -{ -#ifdef CONFIG_ARM - gd = app_gd; -#endif - - return ret; -} - /* Low 32 bit */ #define EFI_LOW32(a) (a & 0xFFFFFFFFULL) /* High 32 bit */ @@ -733,7 +748,9 @@ static efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle, return EFI_EXIT(info->exit_status); }
+ __efi_exit_check(); entry(image_handle, &systab); + __efi_entry_check();
/* Should usually never get here */ return EFI_EXIT(EFI_SUCCESS);
Missing an EFI_ENTRY() or doubling up EFI_EXIT() leads to non-obvious crashes. Let's add some error checking.
Signed-off-by: Rob Clark robdclark@gmail.com
Thanks, applied to efi-next
Alex
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() and EFI_EXIT() calls.
Signed-off-by: Rob Clark robdclark@gmail.com --- include/efi_loader.h | 12 ++++++++---- lib/efi_loader/efi_boottime.c | 25 +++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 4 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index 4262d0ac6b..037cc7c543 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -17,13 +17,16 @@
int __efi_entry_check(void); int __efi_exit_check(void); +const char *__efi_nesting_inc(void); +const char *__efi_nesting_dec(void);
/* * Enter the u-boot world from UEFI: */ #define EFI_ENTRY(format, ...) do { \ assert(__efi_entry_check()); \ - debug("EFI: Entry %s(" format ")\n", __func__, ##__VA_ARGS__); \ + debug("%sEFI: Entry %s(" format ")\n", __efi_nesting_inc(), \ + __func__, ##__VA_ARGS__); \ } while(0)
/* @@ -31,7 +34,8 @@ int __efi_exit_check(void); */ #define EFI_EXIT(ret) ({ \ efi_status_t _r = ret; \ - debug("EFI: Exit: %s: %u\n", __func__, (u32)(_r & ~EFI_ERROR_MASK)); \ + debug("%sEFI: Exit: %s: %u\n", __efi_nesting_dec(), \ + __func__, (u32)(_r & ~EFI_ERROR_MASK)); \ assert(__efi_exit_check()); \ _r; \ }) @@ -40,11 +44,11 @@ int __efi_exit_check(void); * 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_inc(), #exp); \ assert(__efi_exit_check()); \ exp; \ assert(__efi_entry_check()); \ - debug("EFI: Return From: %s\n", #exp); \ + debug("%sEFI: Return From: %s\n", __efi_nesting_dec(), #exp); \ } while(0)
extern struct efi_runtime_services efi_runtime_services; diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 66137d4ff9..de338f009c 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -50,6 +50,7 @@ static volatile void *efi_gd, *app_gd; #endif
static int entry_count; +static int nesting_level;
/* Called on every callback entry */ int __efi_entry_check(void) @@ -96,6 +97,28 @@ void efi_restore_gd(void) #endif }
+/* + * 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_inc(void) +{ + return indent_string(nesting_level++); +} + +const char *__efi_nesting_dec(void) +{ + return indent_string(--nesting_level); +} + /* Low 32 bit */ #define EFI_LOW32(a) (a & 0xFFFFFFFFULL) /* High 32 bit */ @@ -748,9 +771,11 @@ static efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle, return EFI_EXIT(info->exit_status); }
+ __efi_nesting_dec(); __efi_exit_check(); entry(image_handle, &systab); __efi_entry_check(); + __efi_nesting_inc();
/* Should usually never get here */ return EFI_EXIT(EFI_SUCCESS);
On 27.07.17 14:04, Rob Clark wrote:
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() and EFI_EXIT() calls.
Signed-off-by: Rob Clark robdclark@gmail.com
Doesn't the previous patch ensure that we're always only going 1 level deep?
include/efi_loader.h | 12 ++++++++---- lib/efi_loader/efi_boottime.c | 25 +++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 4 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index 4262d0ac6b..037cc7c543 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -17,13 +17,16 @@
int __efi_entry_check(void); int __efi_exit_check(void); +const char *__efi_nesting_inc(void); +const char *__efi_nesting_dec(void);
/*
- Enter the u-boot world from UEFI:
*/ #define EFI_ENTRY(format, ...) do { \ assert(__efi_entry_check()); \
- debug("EFI: Entry %s(" format ")\n", __func__, ##__VA_ARGS__); \
debug("%sEFI: Entry %s(" format ")\n", __efi_nesting_inc(), \
__func__, ##__VA_ARGS__); \} while(0)
/*
@@ -31,7 +34,8 @@ int __efi_exit_check(void); */ #define EFI_EXIT(ret) ({ \ efi_status_t _r = ret; \
- debug("EFI: Exit: %s: %u\n", __func__, (u32)(_r & ~EFI_ERROR_MASK)); \
- debug("%sEFI: Exit: %s: %u\n", __efi_nesting_dec(), \
__func__, (u32)(_r & ~EFI_ERROR_MASK)); \@@ -40,11 +44,11 @@ int __efi_exit_check(void);
- 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_inc(), #exp); \ assert(__efi_exit_check()); \ exp; \ assert(__efi_entry_check()); \
- debug("EFI: Return From: %s\n", #exp); \
debug("%sEFI: Return From: %s\n", __efi_nesting_dec(), #exp); \ } while(0)
extern struct efi_runtime_services efi_runtime_services;
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 66137d4ff9..de338f009c 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -50,6 +50,7 @@ static volatile void *efi_gd, *app_gd; #endif
static int entry_count; +static int nesting_level;
/* Called on every callback entry */ int __efi_entry_check(void) @@ -96,6 +97,28 @@ void efi_restore_gd(void) #endif }
+/*
- 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 = " ";
There's no need for this to be static, no?
Alex
On Fri, Jul 28, 2017 at 3:24 AM, Alexander Graf agraf@suse.de wrote:
On 27.07.17 14:04, Rob Clark wrote:
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() and EFI_EXIT() calls.
Signed-off-by: Rob Clark robdclark@gmail.com
Doesn't the previous patch ensure that we're always only going 1 level deep?
two separate counters for nesting and entry level. We can be more deeply nested when EFI_CALL() is used :-)
include/efi_loader.h | 12 ++++++++---- lib/efi_loader/efi_boottime.c | 25 +++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 4 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index 4262d0ac6b..037cc7c543 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -17,13 +17,16 @@ int __efi_entry_check(void); int __efi_exit_check(void); +const char *__efi_nesting_inc(void); +const char *__efi_nesting_dec(void); /*
- Enter the u-boot world from UEFI:
*/ #define EFI_ENTRY(format, ...) do { \ assert(__efi_entry_check()); \
debug("EFI: Entry %s(" format ")\n", __func__, ##__VA_ARGS__); \
debug("%sEFI: Entry %s(" format ")\n", __efi_nesting_inc(), \__func__, ##__VA_ARGS__); \ } while(0)@@ -31,7 +34,8 @@ int __efi_exit_check(void); */ #define EFI_EXIT(ret) ({ \ efi_status_t _r = ret; \
debug("EFI: Exit: %s: %u\n", __func__, (u32)(_r &~EFI_ERROR_MASK)); \
debug("%sEFI: Exit: %s: %u\n", __efi_nesting_dec(), \__func__, (u32)(_r & ~EFI_ERROR_MASK)); \ assert(__efi_exit_check()); \ _r; \ })@@ -40,11 +44,11 @@ int __efi_exit_check(void);
- 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_inc(), #exp); \ assert(__efi_exit_check()); \ exp; \ assert(__efi_entry_check()); \
debug("EFI: Return From: %s\n", #exp); \
debug("%sEFI: Return From: %s\n", __efi_nesting_dec(), #exp); \ } while(0)diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 66137d4ff9..de338f009c 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -50,6 +50,7 @@ static volatile void *efi_gd, *app_gd; #endif static int entry_count; +static int nesting_level; /* Called on every callback entry */ int __efi_entry_check(void) @@ -96,6 +97,28 @@ void efi_restore_gd(void) #endif } +/*
- 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 = " ";There's no need for this to be static, no?
I suppose it doesn't *need* to be.. but it also doesn't need to have scope outside the file, and usually static is a good hint to the compiler to inline it. (If non-static the compiler needs to emit a non-inlined version of it since it doesn't know it won't be called outside of this object file.
BR, -R
On 28.07.17 11:19, Rob Clark wrote:
On Fri, Jul 28, 2017 at 3:24 AM, Alexander Graf agraf@suse.de wrote:
On 27.07.17 14:04, Rob Clark wrote:
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() and EFI_EXIT() calls.
Signed-off-by: Rob Clark robdclark@gmail.com
Doesn't the previous patch ensure that we're always only going 1 level deep?
two separate counters for nesting and entry level. We can be more deeply nested when EFI_CALL() is used :-)
Ah, so this basically gives you the EFI_CALL nesting level? Wouldn't it make sense to also increase the nesting level on every application invocation?
include/efi_loader.h | 12 ++++++++---- lib/efi_loader/efi_boottime.c | 25 +++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 4 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index 4262d0ac6b..037cc7c543 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -17,13 +17,16 @@ int __efi_entry_check(void); int __efi_exit_check(void); +const char *__efi_nesting_inc(void); +const char *__efi_nesting_dec(void); /* * Enter the u-boot world from UEFI: */ #define EFI_ENTRY(format, ...) do { \ assert(__efi_entry_check()); \
debug("EFI: Entry %s(" format ")\n", __func__, ##__VA_ARGS__); \
debug("%sEFI: Entry %s(" format ")\n", __efi_nesting_inc(), \__func__, ##__VA_ARGS__); \ } while(0)@@ -31,7 +34,8 @@ int __efi_exit_check(void); */ #define EFI_EXIT(ret) ({ \ efi_status_t _r = ret; \
debug("EFI: Exit: %s: %u\n", __func__, (u32)(_r &~EFI_ERROR_MASK)); \
debug("%sEFI: Exit: %s: %u\n", __efi_nesting_dec(), \__func__, (u32)(_r & ~EFI_ERROR_MASK)); \ assert(__efi_exit_check()); \ _r; \ })@@ -40,11 +44,11 @@ int __efi_exit_check(void); * 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_inc(), #exp); \ assert(__efi_exit_check()); \ exp; \ assert(__efi_entry_check()); \
debug("EFI: Return From: %s\n", #exp); \
debug("%sEFI: Return From: %s\n", __efi_nesting_dec(), #exp); \ } while(0)diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 66137d4ff9..de338f009c 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -50,6 +50,7 @@ static volatile void *efi_gd, *app_gd; #endif static int entry_count; +static int nesting_level; /* Called on every callback entry */ int __efi_entry_check(void) @@ -96,6 +97,28 @@ void efi_restore_gd(void) #endif } +/*
- 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 = " ";There's no need for this to be static, no?
I suppose it doesn't *need* to be.. but it also doesn't need to have scope outside the file, and usually static is a good hint to the compiler to inline it. (If non-static the compiler needs to emit a non-inlined version of it since it doesn't know it won't be called outside of this object file.
I don't mean the function, I mean the indent. If you do
static const char *indent = <const value>;
it should be practically the same as
const char *indent = <const value>;
no?
Alex
On Fri, Jul 28, 2017 at 5:25 AM, Alexander Graf agraf@suse.de wrote:
On 28.07.17 11:19, Rob Clark wrote:
On Fri, Jul 28, 2017 at 3:24 AM, Alexander Graf agraf@suse.de wrote:
On 27.07.17 14:04, Rob Clark wrote:
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() and EFI_EXIT() calls.
Signed-off-by: Rob Clark robdclark@gmail.com
Doesn't the previous patch ensure that we're always only going 1 level deep?
two separate counters for nesting and entry level. We can be more deeply nested when EFI_CALL() is used :-)
Ah, so this basically gives you the EFI_CALL nesting level? Wouldn't it make sense to also increase the nesting level on every application invocation?
I specifically avoided that since (at least at what I was looking at) each successive application invocation never returns.
Maybe instead we should just do something like: debug("========================================\n") to show the application invocation boundaries more easily?
include/efi_loader.h | 12 ++++++++---- lib/efi_loader/efi_boottime.c | 25 +++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 4 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index 4262d0ac6b..037cc7c543 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -17,13 +17,16 @@ int __efi_entry_check(void); int __efi_exit_check(void); +const char *__efi_nesting_inc(void); +const char *__efi_nesting_dec(void); /* * Enter the u-boot world from UEFI: */ #define EFI_ENTRY(format, ...) do { \ assert(__efi_entry_check()); \
debug("EFI: Entry %s(" format ")\n", __func__, ##__VA_ARGS__); \
debug("%sEFI: Entry %s(" format ")\n", __efi_nesting_inc(), \__func__, ##__VA_ARGS__); \ } while(0)@@ -31,7 +34,8 @@ int __efi_exit_check(void); */ #define EFI_EXIT(ret) ({ \ efi_status_t _r = ret; \
debug("EFI: Exit: %s: %u\n", __func__, (u32)(_r &~EFI_ERROR_MASK)); \
debug("%sEFI: Exit: %s: %u\n", __efi_nesting_dec(), \__func__, (u32)(_r & ~EFI_ERROR_MASK)); \ assert(__efi_exit_check()); \ _r; \ })@@ -40,11 +44,11 @@ int __efi_exit_check(void); * 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_inc(), #exp); \ assert(__efi_exit_check()); \ exp; \ assert(__efi_entry_check()); \
debug("EFI: Return From: %s\n", #exp); \
debug("%sEFI: Return From: %s\n", __efi_nesting_dec(), #exp); \ } while(0)diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 66137d4ff9..de338f009c 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -50,6 +50,7 @@ static volatile void *efi_gd, *app_gd; #endif static int entry_count; +static int nesting_level; /* Called on every callback entry */ int __efi_entry_check(void) @@ -96,6 +97,28 @@ void efi_restore_gd(void) #endif } +/*
- 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 = " ";There's no need for this to be static, no?
I suppose it doesn't *need* to be.. but it also doesn't need to have scope outside the file, and usually static is a good hint to the compiler to inline it. (If non-static the compiler needs to emit a non-inlined version of it since it doesn't know it won't be called outside of this object file.
I don't mean the function, I mean the indent. If you do
static const char *indent = <const value>;
it should be practically the same as
const char *indent = <const value>;
no?
hmm, I didn't want the compiler to instantiate the array on the stack. But I suppose I need to check the generated asm to see how clever it is.
BR, -R
On 28.07.17 11:34, Rob Clark wrote:
On Fri, Jul 28, 2017 at 5:25 AM, Alexander Graf agraf@suse.de wrote:
On 28.07.17 11:19, Rob Clark wrote:
On Fri, Jul 28, 2017 at 3:24 AM, Alexander Graf agraf@suse.de wrote:
On 27.07.17 14:04, Rob Clark wrote:
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() and EFI_EXIT() calls.
Signed-off-by: Rob Clark robdclark@gmail.com
Doesn't the previous patch ensure that we're always only going 1 level deep?
two separate counters for nesting and entry level. We can be more deeply nested when EFI_CALL() is used :-)
Ah, so this basically gives you the EFI_CALL nesting level? Wouldn't it make sense to also increase the nesting level on every application invocation?
I specifically avoided that since (at least at what I was looking at) each successive application invocation never returns.
Maybe instead we should just do something like: debug("========================================\n") to show the application invocation boundaries more easily?
Sounds like a good idea to me :). Ideally with a bit more information such as the file path.
include/efi_loader.h | 12 ++++++++---- lib/efi_loader/efi_boottime.c | 25 +++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 4 deletions(-)diff --git a/include/efi_loader.h b/include/efi_loader.h index 4262d0ac6b..037cc7c543 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -17,13 +17,16 @@ int __efi_entry_check(void); int __efi_exit_check(void); +const char *__efi_nesting_inc(void); +const char *__efi_nesting_dec(void); /* * Enter the u-boot world from UEFI: */ #define EFI_ENTRY(format, ...) do { \ assert(__efi_entry_check()); \
debug("EFI: Entry %s(" format ")\n", __func__, ##__VA_ARGS__); \
debug("%sEFI: Entry %s(" format ")\n", __efi_nesting_inc(), \__func__, ##__VA_ARGS__); \ } while(0) /*@@ -31,7 +34,8 @@ int __efi_exit_check(void); */ #define EFI_EXIT(ret) ({ \ efi_status_t _r = ret; \
debug("EFI: Exit: %s: %u\n", __func__, (u32)(_r &~EFI_ERROR_MASK)); \
debug("%sEFI: Exit: %s: %u\n", __efi_nesting_dec(), \__func__, (u32)(_r & ~EFI_ERROR_MASK)); \ assert(__efi_exit_check()); \ _r; \ })@@ -40,11 +44,11 @@ int __efi_exit_check(void); * 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_inc(), #exp); \ assert(__efi_exit_check()); \ exp; \ assert(__efi_entry_check()); \
debug("EFI: Return From: %s\n", #exp); \
debug("%sEFI: Return From: %s\n", __efi_nesting_dec(), #exp); \ } while(0) extern struct efi_runtime_services efi_runtime_services;diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 66137d4ff9..de338f009c 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -50,6 +50,7 @@ static volatile void *efi_gd, *app_gd; #endif static int entry_count; +static int nesting_level; /* Called on every callback entry */ int __efi_entry_check(void) @@ -96,6 +97,28 @@ void efi_restore_gd(void) #endif } +/*
- 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 = " ";There's no need for this to be static, no?
I suppose it doesn't *need* to be.. but it also doesn't need to have scope outside the file, and usually static is a good hint to the compiler to inline it. (If non-static the compiler needs to emit a non-inlined version of it since it doesn't know it won't be called outside of this object file.
I don't mean the function, I mean the indent. If you do
static const char *indent = <const value>;
it should be practically the same as
const char *indent = <const value>;
no?
hmm, I didn't want the compiler to instantiate the array on the stack. But I suppose I need to check the generated asm to see how clever it is.
It really shouldn't do that. As long as you're just juggling pointers to a region in .rodata it should know exactly what's going on.
Alex
On Fri, Jul 28, 2017 at 5:36 AM, Alexander Graf agraf@suse.de wrote:
On 28.07.17 11:34, Rob Clark wrote:
On Fri, Jul 28, 2017 at 5:25 AM, Alexander Graf agraf@suse.de wrote:
On 28.07.17 11:19, Rob Clark wrote:
On Fri, Jul 28, 2017 at 3:24 AM, Alexander Graf agraf@suse.de wrote:
On 27.07.17 14:04, Rob Clark wrote:
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() and EFI_EXIT() calls.
Signed-off-by: Rob Clark robdclark@gmail.com
Doesn't the previous patch ensure that we're always only going 1 level deep?
two separate counters for nesting and entry level. We can be more deeply nested when EFI_CALL() is used :-)
Ah, so this basically gives you the EFI_CALL nesting level? Wouldn't it make sense to also increase the nesting level on every application invocation?
I specifically avoided that since (at least at what I was looking at) each successive application invocation never returns.
Maybe instead we should just do something like: debug("========================================\n") to show the application invocation boundaries more easily?
Sounds like a good idea to me :). Ideally with a bit more information such as the file path.
well, until my RFC patchset, there is no guarantee to be a file path ;-)
And both the device and file path will be efi_device_path. I have some thoughts to spiff out device_path_to_text a bit more to make it more complete and also suitable for internal debugging use.. but that is going to be post-MW. Once that happens we can add some more information, but for now the boring "===========" is all we can do.
include/efi_loader.h | 12 ++++++++---- lib/efi_loader/efi_boottime.c | 25 +++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 4 deletions(-)diff --git a/include/efi_loader.h b/include/efi_loader.h index 4262d0ac6b..037cc7c543 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -17,13 +17,16 @@ int __efi_entry_check(void); int __efi_exit_check(void); +const char *__efi_nesting_inc(void); +const char *__efi_nesting_dec(void); /* * Enter the u-boot world from UEFI: */ #define EFI_ENTRY(format, ...) do { \ assert(__efi_entry_check()); \
debug("EFI: Entry %s(" format ")\n", __func__, ##__VA_ARGS__);\
debug("%sEFI: Entry %s(" format ")\n", __efi_nesting_inc(), \__func__, ##__VA_ARGS__); \ } while(0) /*@@ -31,7 +34,8 @@ int __efi_exit_check(void); */ #define EFI_EXIT(ret) ({ \ efi_status_t _r = ret; \
debug("EFI: Exit: %s: %u\n", __func__, (u32)(_r &~EFI_ERROR_MASK)); \
debug("%sEFI: Exit: %s: %u\n", __efi_nesting_dec(), \__func__, (u32)(_r & ~EFI_ERROR_MASK)); \ assert(__efi_exit_check()); \ _r; \ })@@ -40,11 +44,11 @@ int __efi_exit_check(void); * 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_inc(), #exp); \ assert(__efi_exit_check()); \ exp; \ assert(__efi_entry_check()); \
debug("EFI: Return From: %s\n", #exp); \
debug("%sEFI: Return From: %s\n", __efi_nesting_dec(), #exp);\ } while(0) extern struct efi_runtime_services efi_runtime_services; diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 66137d4ff9..de338f009c 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -50,6 +50,7 @@ static volatile void *efi_gd, *app_gd; #endif static int entry_count; +static int nesting_level; /* Called on every callback entry */ int __efi_entry_check(void) @@ -96,6 +97,28 @@ void efi_restore_gd(void) #endif } +/*
- 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 = " ";There's no need for this to be static, no?
I suppose it doesn't *need* to be.. but it also doesn't need to have scope outside the file, and usually static is a good hint to the compiler to inline it. (If non-static the compiler needs to emit a non-inlined version of it since it doesn't know it won't be called outside of this object file.
I don't mean the function, I mean the indent. If you do
static const char *indent = <const value>;
it should be practically the same as
const char *indent = <const value>;
no?
hmm, I didn't want the compiler to instantiate the array on the stack. But I suppose I need to check the generated asm to see how clever it is.
It really shouldn't do that. As long as you're just juggling pointers to a region in .rodata it should know exactly what's going on.
I'll double check when I get to the office.. making it static doesn't hurt and forces the compiler to do what we want (.rodata). Without the 'static' it might end up doing the same thing.
BR, -R
On 28.07.17 12:11, Rob Clark wrote:
On Fri, Jul 28, 2017 at 5:36 AM, Alexander Graf agraf@suse.de wrote:
On 28.07.17 11:34, Rob Clark wrote:
On Fri, Jul 28, 2017 at 5:25 AM, Alexander Graf agraf@suse.de wrote:
On 28.07.17 11:19, Rob Clark wrote:
On Fri, Jul 28, 2017 at 3:24 AM, Alexander Graf agraf@suse.de wrote:
On 27.07.17 14:04, Rob Clark wrote: > > > > 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() > and EFI_EXIT() calls. > > Signed-off-by: Rob Clark robdclark@gmail.com
Doesn't the previous patch ensure that we're always only going 1 level deep?
two separate counters for nesting and entry level. We can be more deeply nested when EFI_CALL() is used :-)
Ah, so this basically gives you the EFI_CALL nesting level? Wouldn't it make sense to also increase the nesting level on every application invocation?
I specifically avoided that since (at least at what I was looking at) each successive application invocation never returns.
Maybe instead we should just do something like: debug("========================================\n") to show the application invocation boundaries more easily?
Sounds like a good idea to me :). Ideally with a bit more information such as the file path.
well, until my RFC patchset, there is no guarantee to be a file path ;-)
And both the device and file path will be efi_device_path. I have some thoughts to spiff out device_path_to_text a bit more to make it more complete and also suitable for internal debugging use.. but that is going to be post-MW. Once that happens we can add some more information, but for now the boring "===========" is all we can do.
No worries, we can introduce a fancy ========= when the time is ripe :).
> --- > include/efi_loader.h | 12 ++++++++---- > lib/efi_loader/efi_boottime.c | 25 +++++++++++++++++++++++++ > 2 files changed, 33 insertions(+), 4 deletions(-) > > diff --git a/include/efi_loader.h b/include/efi_loader.h > index 4262d0ac6b..037cc7c543 100644 > --- a/include/efi_loader.h > +++ b/include/efi_loader.h > @@ -17,13 +17,16 @@ > int __efi_entry_check(void); > int __efi_exit_check(void); > +const char *__efi_nesting_inc(void); > +const char *__efi_nesting_dec(void); > /* > * Enter the u-boot world from UEFI: > */ > #define EFI_ENTRY(format, ...) do { \ > assert(__efi_entry_check()); \ > - debug("EFI: Entry %s(" format ")\n", __func__, ##__VA_ARGS__); > \ > + debug("%sEFI: Entry %s(" format ")\n", __efi_nesting_inc(), \ > + __func__, ##__VA_ARGS__); \ > } while(0) > /* > @@ -31,7 +34,8 @@ int __efi_exit_check(void); > */ > #define EFI_EXIT(ret) ({ \ > efi_status_t _r = ret; \ > - debug("EFI: Exit: %s: %u\n", __func__, (u32)(_r & > ~EFI_ERROR_MASK)); \ > + debug("%sEFI: Exit: %s: %u\n", __efi_nesting_dec(), \ > + __func__, (u32)(_r & ~EFI_ERROR_MASK)); \ > assert(__efi_exit_check()); \ > _r; \ > }) > @@ -40,11 +44,11 @@ int __efi_exit_check(void); > * 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_inc(), #exp); \ > assert(__efi_exit_check()); \ > exp; \ > assert(__efi_entry_check()); \ > - debug("EFI: Return From: %s\n", #exp); \ > + debug("%sEFI: Return From: %s\n", __efi_nesting_dec(), #exp); > \ > } while(0) > extern struct efi_runtime_services efi_runtime_services; > diff --git a/lib/efi_loader/efi_boottime.c > b/lib/efi_loader/efi_boottime.c > index 66137d4ff9..de338f009c 100644 > --- a/lib/efi_loader/efi_boottime.c > +++ b/lib/efi_loader/efi_boottime.c > @@ -50,6 +50,7 @@ static volatile void *efi_gd, *app_gd; > #endif > static int entry_count; > +static int nesting_level; > /* Called on every callback entry */ > int __efi_entry_check(void) > @@ -96,6 +97,28 @@ void efi_restore_gd(void) > #endif > } > +/* > + * 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 = " ";
There's no need for this to be static, no?
I suppose it doesn't *need* to be.. but it also doesn't need to have scope outside the file, and usually static is a good hint to the compiler to inline it. (If non-static the compiler needs to emit a non-inlined version of it since it doesn't know it won't be called outside of this object file.
I don't mean the function, I mean the indent. If you do
static const char *indent = <const value>;it should be practically the same as
const char *indent = <const value>;no?
hmm, I didn't want the compiler to instantiate the array on the stack. But I suppose I need to check the generated asm to see how clever it is.
It really shouldn't do that. As long as you're just juggling pointers to a region in .rodata it should know exactly what's going on.
I'll double check when I get to the office.. making it static doesn't hurt and forces the compiler to do what we want (.rodata). Without the 'static' it might end up doing the same thing.
Well, every time i see a "static" declared variable inside a function scope my alarm bells ring :). So I'd prefer we had as few as possible.
In fact, I *think* what your line does is it puts the pointer into a global in .data which really isn't what we want.
Alex
On Fri, Jul 28, 2017 at 6:22 AM, Alexander Graf agraf@suse.de wrote:
On 28.07.17 12:11, Rob Clark wrote:
On Fri, Jul 28, 2017 at 5:36 AM, Alexander Graf agraf@suse.de wrote:
On 28.07.17 11:34, Rob Clark wrote:
On Fri, Jul 28, 2017 at 5:25 AM, Alexander Graf agraf@suse.de wrote:
On 28.07.17 11:19, Rob Clark wrote:
On Fri, Jul 28, 2017 at 3:24 AM, Alexander Graf agraf@suse.de wrote: > > > > > > On 27.07.17 14:04, Rob Clark wrote: >> >> >> >> >> 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() >> and EFI_EXIT() calls. >> >> Signed-off-by: Rob Clark robdclark@gmail.com > > > > > > Doesn't the previous patch ensure that we're always only going 1 > level > deep?
two separate counters for nesting and entry level. We can be more deeply nested when EFI_CALL() is used :-)
Ah, so this basically gives you the EFI_CALL nesting level? Wouldn't it make sense to also increase the nesting level on every application invocation?
I specifically avoided that since (at least at what I was looking at) each successive application invocation never returns.
Maybe instead we should just do something like: debug("========================================\n") to show the application invocation boundaries more easily?
Sounds like a good idea to me :). Ideally with a bit more information such as the file path.
well, until my RFC patchset, there is no guarantee to be a file path ;-)
And both the device and file path will be efi_device_path. I have some thoughts to spiff out device_path_to_text a bit more to make it more complete and also suitable for internal debugging use.. but that is going to be post-MW. Once that happens we can add some more information, but for now the boring "===========" is all we can do.
No worries, we can introduce a fancy ========= when the time is ripe :).
> >> --- >> include/efi_loader.h | 12 ++++++++---- >> lib/efi_loader/efi_boottime.c | 25 +++++++++++++++++++++++++ >> 2 files changed, 33 insertions(+), 4 deletions(-) >> >> diff --git a/include/efi_loader.h b/include/efi_loader.h >> index 4262d0ac6b..037cc7c543 100644 >> --- a/include/efi_loader.h >> +++ b/include/efi_loader.h >> @@ -17,13 +17,16 @@ >> int __efi_entry_check(void); >> int __efi_exit_check(void); >> +const char *__efi_nesting_inc(void); >> +const char *__efi_nesting_dec(void); >> /* >> * Enter the u-boot world from UEFI: >> */ >> #define EFI_ENTRY(format, ...) do { \ >> assert(__efi_entry_check()); \ >> - debug("EFI: Entry %s(" format ")\n", __func__, >> ##__VA_ARGS__); >> \ >> + debug("%sEFI: Entry %s(" format ")\n", __efi_nesting_inc(), >> \ >> + __func__, ##__VA_ARGS__); \ >> } while(0) >> /* >> @@ -31,7 +34,8 @@ int __efi_exit_check(void); >> */ >> #define EFI_EXIT(ret) ({ \ >> efi_status_t _r = ret; \ >> - debug("EFI: Exit: %s: %u\n", __func__, (u32)(_r & >> ~EFI_ERROR_MASK)); \ >> + debug("%sEFI: Exit: %s: %u\n", __efi_nesting_dec(), \ >> + __func__, (u32)(_r & ~EFI_ERROR_MASK)); \ >> assert(__efi_exit_check()); \ >> _r; \ >> }) >> @@ -40,11 +44,11 @@ int __efi_exit_check(void); >> * 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_inc(), #exp); \ >> assert(__efi_exit_check()); \ >> exp; \ >> assert(__efi_entry_check()); \ >> - debug("EFI: Return From: %s\n", #exp); \ >> + debug("%sEFI: Return From: %s\n", __efi_nesting_dec(), >> #exp); >> \ >> } while(0) >> extern struct efi_runtime_services efi_runtime_services; >> diff --git a/lib/efi_loader/efi_boottime.c >> b/lib/efi_loader/efi_boottime.c >> index 66137d4ff9..de338f009c 100644 >> --- a/lib/efi_loader/efi_boottime.c >> +++ b/lib/efi_loader/efi_boottime.c >> @@ -50,6 +50,7 @@ static volatile void *efi_gd, *app_gd; >> #endif >> static int entry_count; >> +static int nesting_level; >> /* Called on every callback entry */ >> int __efi_entry_check(void) >> @@ -96,6 +97,28 @@ void efi_restore_gd(void) >> #endif >> } >> +/* >> + * 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 = " "; > > > > > > There's no need for this to be static, no?
I suppose it doesn't *need* to be.. but it also doesn't need to have scope outside the file, and usually static is a good hint to the compiler to inline it. (If non-static the compiler needs to emit a non-inlined version of it since it doesn't know it won't be called outside of this object file.
I don't mean the function, I mean the indent. If you do
static const char *indent = <const value>;it should be practically the same as
const char *indent = <const value>;no?
hmm, I didn't want the compiler to instantiate the array on the stack. But I suppose I need to check the generated asm to see how clever it is.
It really shouldn't do that. As long as you're just juggling pointers to a region in .rodata it should know exactly what's going on.
I'll double check when I get to the office.. making it static doesn't hurt and forces the compiler to do what we want (.rodata). Without the 'static' it might end up doing the same thing.
Well, every time i see a "static" declared variable inside a function scope my alarm bells ring :). So I'd prefer we had as few as possible.
In fact, I *think* what your line does is it puts the pointer into a global in .data which really isn't what we want.
it seems to do the identical thing in both cases (based on diffing objdump output)
BR, -R
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() and EFI_EXIT() calls.
Signed-off-by: Rob Clark robdclark@gmail.com
Thanks, applied to efi-next
Alex
There are a couple spots doing things like:
return EFI_EXIT(some_fxn(...));
which I handn't noticed before. With addition of printing return value in the EFI_EXIT() macro, now the fxn call was getting evaluated twice. Which we didn't really want.
Signed-off-by: Rob Clark robdclark@gmail.com
Thanks, applied to efi-next
Alex
participants (2)
-
Alexander Graf -
Rob Clark