[U-Boot] [PATCH 1/4] efi_loader: only evaluate EFI_EXIT()'s ret once

[U-Boot] [PATCH 1/4] efi_loader: only evaluate EFI_EXIT()'s ret once

[Rob Clark]

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;
-- 
2.13.0

[U-Boot] [PATCH 2/4] efi_loader: Add an EFI_CALL() macro

[Rob Clark]

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));
 	}
 }
 
-- 
2.13.0

[U-Boot] [U-Boot,2/4] efi_loader: Add an EFI_CALL() macro

[Alexander Graf]

> 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

[U-Boot] [PATCH 3/4] efi_loader: add checking for incorrect use of EFI_ENTRY/EXIT

[Rob Clark]

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);
-- 
2.13.0

[U-Boot] [U-Boot, 3/4] efi_loader: add checking for incorrect use of EFI_ENTRY/EXIT

[Alexander Graf]

> 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

[U-Boot] [PATCH 4/4] efi_loader: indent entry/exit prints to show nesting level

[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()
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);
-- 
2.13.0

[U-Boot] [PATCH 4/4] efi_loader: indent entry/exit prints to show nesting level

[Alexander Graf]

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)); \
>   	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?


Alex

[U-Boot] [PATCH 4/4] efi_loader: indent entry/exit prints to show nesting level

[Rob Clark]

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

BR,
-R

[U-Boot] [PATCH 4/4] efi_loader: indent entry/exit prints to show nesting level

[Alexander Graf]

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


Alex

[U-Boot] [PATCH 4/4] efi_loader: indent entry/exit prints to show nesting level

[Rob Clark]

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

BR,
-R

[U-Boot] [PATCH 4/4] efi_loader: indent entry/exit prints to show nesting level

[Alexander Graf]

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

[U-Boot] [PATCH 4/4] efi_loader: indent entry/exit prints to show nesting level

[Rob Clark]

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

[U-Boot] [PATCH 4/4] efi_loader: indent entry/exit prints to show nesting level

[Alexander Graf]

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

[U-Boot] [PATCH 4/4] efi_loader: indent entry/exit prints to show nesting level

[Rob Clark]

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

[U-Boot] [U-Boot, 4/4] efi_loader: indent entry/exit prints to show nesting level

[Alexander Graf]

> 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

[U-Boot] [U-Boot, 1/4] efi_loader: only evaluate EFI_EXIT()'s ret once

[Alexander Graf]

> 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