* [U-Boot] [PATCH 1/3] efi_loader: Add an EFI_CALL() macro
@ 2017-07-26 13:55 Rob Clark
2017-07-26 13:55 ` [U-Boot] [PATCH 2/3] efi_loader: add checking for incorrect use of EFI_ENTRY/EXIT Rob Clark
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Rob Clark @ 2017-07-26 13:55 UTC (permalink / raw)
To: u-boot
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 f384cbbe77..09bab7dbc6 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -15,16 +15,33 @@
#include <linux/list.h>
+/*
+ * Enter the u-boot world from UEFI:
+ */
#define EFI_ENTRY(format, ...) do { \
efi_restore_gd(); \
debug("EFI: Entry %s(" format ")\n", __func__, ##__VA_ARGS__); \
} while(0)
+/*
+ * Exit the u-boot world back to UEFI:
+ */
#define EFI_EXIT(ret) ({ \
debug("EFI: Exit: %s: %u\n", __func__, (u32)((ret) & ~EFI_ERROR_MASK)); \
efi_exit_func(ret); \
})
+/*
+ * Callback into UEFI world from u-boot:
+ */
+#define EFI_CALL(exp) do { \
+ debug("EFI: Call: %s\n", #exp); \
+ efi_exit_func(EFI_SUCCESS); \
+ exp; \
+ efi_restore_gd(); \
+ debug("EFI: Return From: %s\n", #exp); \
+ } while(0)
+
extern struct efi_runtime_services efi_runtime_services;
extern struct efi_system_table systab;
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 9a1a93fade..76cafffc1d 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH 2/3] efi_loader: add checking for incorrect use of EFI_ENTRY/EXIT
2017-07-26 13:55 [U-Boot] [PATCH 1/3] efi_loader: Add an EFI_CALL() macro Rob Clark
@ 2017-07-26 13:55 ` Rob Clark
2017-07-26 15:25 ` Alexander Graf
2017-07-26 20:16 ` Heinrich Schuchardt
2017-07-26 13:55 ` [U-Boot] [PATCH 3/3] efi_loader: indent entry/exit prints to show nesting level Rob Clark
2017-07-26 20:15 ` [U-Boot] [PATCH 1/3] efi_loader: Add an EFI_CALL() macro Heinrich Schuchardt
2 siblings, 2 replies; 11+ messages in thread
From: Rob Clark @ 2017-07-26 13:55 UTC (permalink / raw)
To: u-boot
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 | 5 +++++
lib/efi_loader/efi_boottime.c | 13 +++++++++++++
2 files changed, 18 insertions(+)
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 09bab7dbc6..4b49fac84b 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -15,11 +15,13 @@
#include <linux/list.h>
+int __efi_check_nesting(int delta);
/*
* Enter the u-boot world from UEFI:
*/
#define EFI_ENTRY(format, ...) do { \
efi_restore_gd(); \
+ assert(__efi_check_nesting(+1)); \
debug("EFI: Entry %s(" format ")\n", __func__, ##__VA_ARGS__); \
} while(0)
@@ -28,6 +30,7 @@
*/
#define EFI_EXIT(ret) ({ \
debug("EFI: Exit: %s: %u\n", __func__, (u32)((ret) & ~EFI_ERROR_MASK)); \
+ assert(__efi_check_nesting(-1)); \
efi_exit_func(ret); \
})
@@ -36,10 +39,12 @@
*/
#define EFI_CALL(exp) do { \
debug("EFI: Call: %s\n", #exp); \
+ assert(__efi_check_nesting(-1)); \
efi_exit_func(EFI_SUCCESS); \
exp; \
efi_restore_gd(); \
debug("EFI: Return From: %s\n", #exp); \
+ assert(__efi_check_nesting(+1)); \
} while(0)
extern struct efi_runtime_services efi_runtime_services;
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 76cafffc1d..b21df7bd5d 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -57,6 +57,17 @@ void efi_save_gd(void)
#endif
}
+/* Check for incorrect use of EFI_ENTRY() / EFI_EXIT() */
+int __efi_check_nesting(int delta)
+{
+ static int entry_count;
+ /* post-increment, pre-decrement: */
+ if (delta > 0)
+ return entry_count++ == 0;
+ else
+ return --entry_count == 0;
+}
+
/* Called on every callback entry */
void efi_restore_gd(void)
{
@@ -716,7 +727,9 @@ static efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
return EFI_EXIT(info->exit_status);
}
+ __efi_check_nesting(-1);
entry(image_handle, &systab);
+ __efi_check_nesting(+1);
/* Should usually never get here */
return EFI_EXIT(EFI_SUCCESS);
--
2.13.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH 3/3] efi_loader: indent entry/exit prints to show nesting level
2017-07-26 13:55 [U-Boot] [PATCH 1/3] efi_loader: Add an EFI_CALL() macro Rob Clark
2017-07-26 13:55 ` [U-Boot] [PATCH 2/3] efi_loader: add checking for incorrect use of EFI_ENTRY/EXIT Rob Clark
@ 2017-07-26 13:55 ` Rob Clark
2017-07-26 15:36 ` Alexander Graf
2017-07-26 20:16 ` Heinrich Schuchardt
2017-07-26 20:15 ` [U-Boot] [PATCH 1/3] efi_loader: Add an EFI_CALL() macro Heinrich Schuchardt
2 siblings, 2 replies; 11+ messages in thread
From: Rob Clark @ 2017-07-26 13:55 UTC (permalink / raw)
To: u-boot
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 | 28 ++++++++++++++++++++++++++++
2 files changed, 36 insertions(+), 4 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 4b49fac84b..88091d4914 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -16,20 +16,24 @@
#include <linux/list.h>
int __efi_check_nesting(int delta);
+const char *__efi_nesting_level(int delta);
+
/*
* Enter the u-boot world from UEFI:
*/
#define EFI_ENTRY(format, ...) do { \
efi_restore_gd(); \
+ debug("%sEFI: Entry %s(" format ")\n", __efi_nesting_level(1), \
+ __func__, ##__VA_ARGS__); \
assert(__efi_check_nesting(+1)); \
- debug("EFI: Entry %s(" format ")\n", __func__, ##__VA_ARGS__); \
} while(0)
/*
* Exit the u-boot world back to UEFI:
*/
#define EFI_EXIT(ret) ({ \
- debug("EFI: Exit: %s: %u\n", __func__, (u32)((ret) & ~EFI_ERROR_MASK)); \
+ debug("%sEFI: Exit: %s: %u\n", __efi_nesting_level(-1), \
+ __func__, (u32)((ret) & ~EFI_ERROR_MASK)); \
assert(__efi_check_nesting(-1)); \
efi_exit_func(ret); \
})
@@ -38,12 +42,12 @@ int __efi_check_nesting(int delta);
* Callback into UEFI world from u-boot:
*/
#define EFI_CALL(exp) do { \
- debug("EFI: Call: %s\n", #exp); \
+ debug("%sEFI: Call: %s\n", __efi_nesting_level(1), #exp); \
assert(__efi_check_nesting(-1)); \
efi_exit_func(EFI_SUCCESS); \
exp; \
efi_restore_gd(); \
- debug("EFI: Return From: %s\n", #exp); \
+ debug("%sEFI: Return From: %s\n", __efi_nesting_level(-1), #exp); \
assert(__efi_check_nesting(+1)); \
} while(0)
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index b21df7bd5d..f6c4c162cf 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -92,6 +92,34 @@ efi_status_t efi_exit_func(efi_status_t ret)
return ret;
}
+/*
+ * Two spaces per indent level, maxing out at 10.. which ought to be
+ * enough for anyone ;-)
+ */
+static const char *indent_string(int level)
+{
+ static const char *indent = " ";
+ const int max = strlen(indent);
+ level = min(max, level * 2);
+ return &indent[max - level];
+}
+
+const char *__efi_nesting_level(int delta)
+{
+ static int nesting_level;
+ const char *s;
+ /* post-increment, pre-decrement */
+ if (delta > 0) {
+ s = indent_string(nesting_level);
+ nesting_level += delta;
+ } else {
+ nesting_level += delta;
+ s = indent_string(nesting_level);
+ assert(nesting_level >= 0);
+ }
+ return s;
+}
+
/* Low 32 bit */
#define EFI_LOW32(a) (a & 0xFFFFFFFFULL)
/* High 32 bit */
--
2.13.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH 2/3] efi_loader: add checking for incorrect use of EFI_ENTRY/EXIT
2017-07-26 13:55 ` [U-Boot] [PATCH 2/3] efi_loader: add checking for incorrect use of EFI_ENTRY/EXIT Rob Clark
@ 2017-07-26 15:25 ` Alexander Graf
2017-07-26 16:41 ` Rob Clark
2017-07-26 20:16 ` Heinrich Schuchardt
1 sibling, 1 reply; 11+ messages in thread
From: Alexander Graf @ 2017-07-26 15:25 UTC (permalink / raw)
To: u-boot
On 26.07.17 15:55, Rob Clark wrote:
> 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 | 5 +++++
> lib/efi_loader/efi_boottime.c | 13 +++++++++++++
> 2 files changed, 18 insertions(+)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 09bab7dbc6..4b49fac84b 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -15,11 +15,13 @@
>
> #include <linux/list.h>
>
> +int __efi_check_nesting(int delta);
> /*
> * Enter the u-boot world from UEFI:
> */
> #define EFI_ENTRY(format, ...) do { \
> efi_restore_gd(); \
> + assert(__efi_check_nesting(+1)); \
> debug("EFI: Entry %s(" format ")\n", __func__, ##__VA_ARGS__); \
> } while(0)
>
> @@ -28,6 +30,7 @@
> */
> #define EFI_EXIT(ret) ({ \
> debug("EFI: Exit: %s: %u\n", __func__, (u32)((ret) & ~EFI_ERROR_MASK)); \
> + assert(__efi_check_nesting(-1)); \
> efi_exit_func(ret); \
> })
>
> @@ -36,10 +39,12 @@
> */
> #define EFI_CALL(exp) do { \
> debug("EFI: Call: %s\n", #exp); \
> + assert(__efi_check_nesting(-1)); \
> efi_exit_func(EFI_SUCCESS); \
> exp; \
> efi_restore_gd(); \
> debug("EFI: Return From: %s\n", #exp); \
> + assert(__efi_check_nesting(+1)); \
> } while(0)
>
> extern struct efi_runtime_services efi_runtime_services;
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 76cafffc1d..b21df7bd5d 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -57,6 +57,17 @@ void efi_save_gd(void)
> #endif
> }
>
> +/* Check for incorrect use of EFI_ENTRY() / EFI_EXIT() */
> +int __efi_check_nesting(int delta)
> +{
> + static int entry_count;
I'd prefer if we marked globals as such (read: somewhere at the top of a
.c file).
Also I think this function would be better off as a static inline in a
header, as it should get compressed quite well.
> + /* post-increment, pre-decrement: */
> + if (delta > 0)
> + return entry_count++ == 0;
> + else
> + return --entry_count == 0;
I have a hard time to wrap my head around the logic. At least, couldn't
you split this into 2 functions? :)
Alex
> +}
> +
> /* Called on every callback entry */
> void efi_restore_gd(void)
> {
> @@ -716,7 +727,9 @@ static efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
> return EFI_EXIT(info->exit_status);
> }
>
> + __efi_check_nesting(-1);
> entry(image_handle, &systab);
> + __efi_check_nesting(+1);
>
> /* Should usually never get here */
> return EFI_EXIT(EFI_SUCCESS);
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH 3/3] efi_loader: indent entry/exit prints to show nesting level
2017-07-26 13:55 ` [U-Boot] [PATCH 3/3] efi_loader: indent entry/exit prints to show nesting level Rob Clark
@ 2017-07-26 15:36 ` Alexander Graf
2017-07-26 20:16 ` Heinrich Schuchardt
1 sibling, 0 replies; 11+ messages in thread
From: Alexander Graf @ 2017-07-26 15:36 UTC (permalink / raw)
To: u-boot
On 26.07.17 15:55, 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>
> ---
> include/efi_loader.h | 12 ++++++++----
> lib/efi_loader/efi_boottime.c | 28 ++++++++++++++++++++++++++++
> 2 files changed, 36 insertions(+), 4 deletions(-)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 4b49fac84b..88091d4914 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -16,20 +16,24 @@
> #include <linux/list.h>
>
> int __efi_check_nesting(int delta);
> +const char *__efi_nesting_level(int delta);
> +
> /*
> * Enter the u-boot world from UEFI:
> */
> #define EFI_ENTRY(format, ...) do { \
> efi_restore_gd(); \
> + debug("%sEFI: Entry %s(" format ")\n", __efi_nesting_level(1), \
> + __func__, ##__VA_ARGS__); \
> assert(__efi_check_nesting(+1)); \
> - debug("EFI: Entry %s(" format ")\n", __func__, ##__VA_ARGS__); \
> } while(0)
>
> /*
> * Exit the u-boot world back to UEFI:
> */
> #define EFI_EXIT(ret) ({ \
> - debug("EFI: Exit: %s: %u\n", __func__, (u32)((ret) & ~EFI_ERROR_MASK)); \
> + debug("%sEFI: Exit: %s: %u\n", __efi_nesting_level(-1), \
> + __func__, (u32)((ret) & ~EFI_ERROR_MASK)); \
> assert(__efi_check_nesting(-1)); \
> efi_exit_func(ret); \
> })
> @@ -38,12 +42,12 @@ int __efi_check_nesting(int delta);
> * Callback into UEFI world from u-boot:
> */
> #define EFI_CALL(exp) do { \
> - debug("EFI: Call: %s\n", #exp); \
> + debug("%sEFI: Call: %s\n", __efi_nesting_level(1), #exp); \
> assert(__efi_check_nesting(-1)); \
> efi_exit_func(EFI_SUCCESS); \
> exp; \
> efi_restore_gd(); \
> - debug("EFI: Return From: %s\n", #exp); \
> + debug("%sEFI: Return From: %s\n", __efi_nesting_level(-1), #exp); \
> assert(__efi_check_nesting(+1)); \
> } while(0)
>
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index b21df7bd5d..f6c4c162cf 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -92,6 +92,34 @@ efi_status_t efi_exit_func(efi_status_t ret)
> return ret;
> }
>
> +/*
> + * Two spaces per indent level, maxing out at 10.. which ought to be
> + * enough for anyone ;-)
> + */
> +static const char *indent_string(int level)
> +{
> + static const char *indent = " ";
> + const int max = strlen(indent);
> + level = min(max, level * 2);
> + return &indent[max - level];
> +}
> +
> +const char *__efi_nesting_level(int delta)
> +{
> + static int nesting_level;
> + const char *s;
> + /* post-increment, pre-decrement */
> + if (delta > 0) {
> + s = indent_string(nesting_level);
> + nesting_level += delta;
> + } else {
> + nesting_level += delta;
> + s = indent_string(nesting_level);
> + assert(nesting_level >= 0);
> + }
Can you do the same here? Move globals to the top of the file and split
the function?
In this particular case I think leaving them as functions is reasonable,
as we should only call them when #define DEBUG is set, so we're not size
constrained.
Alex
^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH 2/3] efi_loader: add checking for incorrect use of EFI_ENTRY/EXIT
2017-07-26 15:25 ` Alexander Graf
@ 2017-07-26 16:41 ` Rob Clark
2017-07-26 20:12 ` Alexander Graf
0 siblings, 1 reply; 11+ messages in thread
From: Rob Clark @ 2017-07-26 16:41 UTC (permalink / raw)
To: u-boot
On Wed, Jul 26, 2017 at 11:25 AM, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 26.07.17 15:55, Rob Clark wrote:
>>
>> 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 | 5 +++++
>> lib/efi_loader/efi_boottime.c | 13 +++++++++++++
>> 2 files changed, 18 insertions(+)
>>
>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>> index 09bab7dbc6..4b49fac84b 100644
>> --- a/include/efi_loader.h
>> +++ b/include/efi_loader.h
>> @@ -15,11 +15,13 @@
>> #include <linux/list.h>
>> +int __efi_check_nesting(int delta);
>> /*
>> * Enter the u-boot world from UEFI:
>> */
>> #define EFI_ENTRY(format, ...) do { \
>> efi_restore_gd(); \
>> + assert(__efi_check_nesting(+1)); \
>> debug("EFI: Entry %s(" format ")\n", __func__, ##__VA_ARGS__); \
>> } while(0)
>> @@ -28,6 +30,7 @@
>> */
>> #define EFI_EXIT(ret) ({ \
>> debug("EFI: Exit: %s: %u\n", __func__, (u32)((ret) &
>> ~EFI_ERROR_MASK)); \
>> + assert(__efi_check_nesting(-1)); \
>> efi_exit_func(ret); \
>> })
>> @@ -36,10 +39,12 @@
>> */
>> #define EFI_CALL(exp) do { \
>> debug("EFI: Call: %s\n", #exp); \
>> + assert(__efi_check_nesting(-1)); \
>> efi_exit_func(EFI_SUCCESS); \
>> exp; \
>> efi_restore_gd(); \
>> debug("EFI: Return From: %s\n", #exp); \
>> + assert(__efi_check_nesting(+1)); \
>> } while(0)
>> extern struct efi_runtime_services efi_runtime_services;
>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>> index 76cafffc1d..b21df7bd5d 100644
>> --- a/lib/efi_loader/efi_boottime.c
>> +++ b/lib/efi_loader/efi_boottime.c
>> @@ -57,6 +57,17 @@ void efi_save_gd(void)
>> #endif
>> }
>> +/* Check for incorrect use of EFI_ENTRY() / EFI_EXIT() */
>> +int __efi_check_nesting(int delta)
>> +{
>> + static int entry_count;
>
>
> I'd prefer if we marked globals as such (read: somewhere at the top of a .c
> file).
hmm, well that does increase the scope unnecessarily, which is why I
made it static inside the fxn. But if you can prefer I guess I can
put it just above __efi_check_nesting().
> Also I think this function would be better off as a static inline in a
> header, as it should get compressed quite well.
That would mean making entry_count also non-static (ie. broader than
function or file level scope), otherwise you end up with different
copies of it in each .o file. (Which would mostly work, but it would
fall apart in confusing ways in some edge cases..)
>> + /* post-increment, pre-decrement: */
>> + if (delta > 0)
>> + return entry_count++ == 0;
>> + else
>> + return --entry_count == 0;
>
>
> I have a hard time to wrap my head around the logic. At least, couldn't you
> split this into 2 functions? :)
Would that make the logic more clear, or just move it far enough apart
that you don't notice it is confusing ;-)
BR,
-R
> Alex
>
>
>> +}
>> +
>> /* Called on every callback entry */
>> void efi_restore_gd(void)
>> {
>> @@ -716,7 +727,9 @@ static efi_status_t EFIAPI
>> efi_start_image(efi_handle_t image_handle,
>> return EFI_EXIT(info->exit_status);
>> }
>> + __efi_check_nesting(-1);
>> entry(image_handle, &systab);
>> + __efi_check_nesting(+1);
>> /* Should usually never get here */
>> return EFI_EXIT(EFI_SUCCESS);
>>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH 2/3] efi_loader: add checking for incorrect use of EFI_ENTRY/EXIT
2017-07-26 16:41 ` Rob Clark
@ 2017-07-26 20:12 ` Alexander Graf
2017-07-26 20:55 ` Rob Clark
0 siblings, 1 reply; 11+ messages in thread
From: Alexander Graf @ 2017-07-26 20:12 UTC (permalink / raw)
To: u-boot
On 26.07.17 18:41, Rob Clark wrote:
> On Wed, Jul 26, 2017 at 11:25 AM, Alexander Graf <agraf@suse.de> wrote:
>>
>>
>> On 26.07.17 15:55, Rob Clark wrote:
>>>
>>> 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 | 5 +++++
>>> lib/efi_loader/efi_boottime.c | 13 +++++++++++++
>>> 2 files changed, 18 insertions(+)
>>>
>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>> index 09bab7dbc6..4b49fac84b 100644
>>> --- a/include/efi_loader.h
>>> +++ b/include/efi_loader.h
>>> @@ -15,11 +15,13 @@
>>> #include <linux/list.h>
>>> +int __efi_check_nesting(int delta);
>>> /*
>>> * Enter the u-boot world from UEFI:
>>> */
>>> #define EFI_ENTRY(format, ...) do { \
>>> efi_restore_gd(); \
>>> + assert(__efi_check_nesting(+1)); \
>>> debug("EFI: Entry %s(" format ")\n", __func__, ##__VA_ARGS__); \
>>> } while(0)
>>> @@ -28,6 +30,7 @@
>>> */
>>> #define EFI_EXIT(ret) ({ \
>>> debug("EFI: Exit: %s: %u\n", __func__, (u32)((ret) &
>>> ~EFI_ERROR_MASK)); \
>>> + assert(__efi_check_nesting(-1)); \
>>> efi_exit_func(ret); \
>>> })
>>> @@ -36,10 +39,12 @@
>>> */
>>> #define EFI_CALL(exp) do { \
>>> debug("EFI: Call: %s\n", #exp); \
>>> + assert(__efi_check_nesting(-1)); \
>>> efi_exit_func(EFI_SUCCESS); \
>>> exp; \
>>> efi_restore_gd(); \
>>> debug("EFI: Return From: %s\n", #exp); \
>>> + assert(__efi_check_nesting(+1)); \
>>> } while(0)
>>> extern struct efi_runtime_services efi_runtime_services;
>>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>>> index 76cafffc1d..b21df7bd5d 100644
>>> --- a/lib/efi_loader/efi_boottime.c
>>> +++ b/lib/efi_loader/efi_boottime.c
>>> @@ -57,6 +57,17 @@ void efi_save_gd(void)
>>> #endif
>>> }
>>> +/* Check for incorrect use of EFI_ENTRY() / EFI_EXIT() */
>>> +int __efi_check_nesting(int delta)
>>> +{
>>> + static int entry_count;
>>
>>
>> I'd prefer if we marked globals as such (read: somewhere at the top of a .c
>> file).
>
> hmm, well that does increase the scope unnecessarily, which is why I
> made it static inside the fxn. But if you can prefer I guess I can
> put it just above __efi_check_nesting().
It's a matter of taste, but in general I find it very useful to know at
a glimpse where .bss, .rodata and .data come from.
>
>> Also I think this function would be better off as a static inline in a
>> header, as it should get compressed quite well.
>
> That would mean making entry_count also non-static (ie. broader than
Ah, no, entry_count would still be in efi_boottime.c, but the function
using it would be in a .h file. That way we don't need to do the
register save/restore dance we need to do for full remote function calls.
> function or file level scope), otherwise you end up with different
> copies of it in each .o file. (Which would mostly work, but it would
> fall apart in confusing ways in some edge cases..)
>
>>> + /* post-increment, pre-decrement: */
>>> + if (delta > 0)
>>> + return entry_count++ == 0;
>>> + else
>>> + return --entry_count == 0;
>>
>>
>> I have a hard time to wrap my head around the logic. At least, couldn't you
>> split this into 2 functions? :)
>
> Would that make the logic more clear, or just move it far enough apart
> that you don't notice it is confusing ;-)
I can't really tell yet, but I'd say it's worth a try :).
Alex
^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH 1/3] efi_loader: Add an EFI_CALL() macro
2017-07-26 13:55 [U-Boot] [PATCH 1/3] efi_loader: Add an EFI_CALL() macro Rob Clark
2017-07-26 13:55 ` [U-Boot] [PATCH 2/3] efi_loader: add checking for incorrect use of EFI_ENTRY/EXIT Rob Clark
2017-07-26 13:55 ` [U-Boot] [PATCH 3/3] efi_loader: indent entry/exit prints to show nesting level Rob Clark
@ 2017-07-26 20:15 ` Heinrich Schuchardt
2 siblings, 0 replies; 11+ messages in thread
From: Heinrich Schuchardt @ 2017-07-26 20:15 UTC (permalink / raw)
To: u-boot
On 07/26/2017 03:55 PM, Rob Clark wrote:
> 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 f384cbbe77..09bab7dbc6 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -15,16 +15,33 @@
>
> #include <linux/list.h>
>
> +/*
> + * Enter the u-boot world from UEFI:
> + */
> #define EFI_ENTRY(format, ...) do { \
> efi_restore_gd(); \
> debug("EFI: Entry %s(" format ")\n", __func__, ##__VA_ARGS__); \
> } while(0)
>
> +/*
> + * Exit the u-boot world back to UEFI:
> + */
> #define EFI_EXIT(ret) ({ \
> debug("EFI: Exit: %s: %u\n", __func__, (u32)((ret) & ~EFI_ERROR_MASK)); \
> efi_exit_func(ret); \
> })
>
> +/*
> + * Callback into UEFI world from u-boot:
> + */
> +#define EFI_CALL(exp) do { \
> + debug("EFI: Call: %s\n", #exp); \
> + efi_exit_func(EFI_SUCCESS); \
> + exp; \
> + efi_restore_gd(); \
> + debug("EFI: Return From: %s\n", #exp); \
> + } while(0)
> +
> extern struct efi_runtime_services efi_runtime_services;
> extern struct efi_system_table systab;
>
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 9a1a93fade..76cafffc1d 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));
> }
> }
>
>
Tested-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH 2/3] efi_loader: add checking for incorrect use of EFI_ENTRY/EXIT
2017-07-26 13:55 ` [U-Boot] [PATCH 2/3] efi_loader: add checking for incorrect use of EFI_ENTRY/EXIT Rob Clark
2017-07-26 15:25 ` Alexander Graf
@ 2017-07-26 20:16 ` Heinrich Schuchardt
1 sibling, 0 replies; 11+ messages in thread
From: Heinrich Schuchardt @ 2017-07-26 20:16 UTC (permalink / raw)
To: u-boot
On 07/26/2017 03:55 PM, Rob Clark wrote:
> 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 | 5 +++++
> lib/efi_loader/efi_boottime.c | 13 +++++++++++++
> 2 files changed, 18 insertions(+)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 09bab7dbc6..4b49fac84b 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -15,11 +15,13 @@
>
> #include <linux/list.h>
>
> +int __efi_check_nesting(int delta);
> /*
> * Enter the u-boot world from UEFI:
> */
> #define EFI_ENTRY(format, ...) do { \
> efi_restore_gd(); \
> + assert(__efi_check_nesting(+1)); \
> debug("EFI: Entry %s(" format ")\n", __func__, ##__VA_ARGS__); \
> } while(0)
>
> @@ -28,6 +30,7 @@
> */
> #define EFI_EXIT(ret) ({ \
> debug("EFI: Exit: %s: %u\n", __func__, (u32)((ret) & ~EFI_ERROR_MASK)); \
> + assert(__efi_check_nesting(-1)); \
> efi_exit_func(ret); \
> })
>
> @@ -36,10 +39,12 @@
> */
> #define EFI_CALL(exp) do { \
> debug("EFI: Call: %s\n", #exp); \
> + assert(__efi_check_nesting(-1)); \
> efi_exit_func(EFI_SUCCESS); \
> exp; \
> efi_restore_gd(); \
> debug("EFI: Return From: %s\n", #exp); \
> + assert(__efi_check_nesting(+1)); \
> } while(0)
>
> extern struct efi_runtime_services efi_runtime_services;
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 76cafffc1d..b21df7bd5d 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -57,6 +57,17 @@ void efi_save_gd(void)
> #endif
> }
>
> +/* Check for incorrect use of EFI_ENTRY() / EFI_EXIT() */
> +int __efi_check_nesting(int delta)
> +{
> + static int entry_count;
> + /* post-increment, pre-decrement: */
> + if (delta > 0)
> + return entry_count++ == 0;
> + else
> + return --entry_count == 0;
> +}
> +
> /* Called on every callback entry */
> void efi_restore_gd(void)
> {
> @@ -716,7 +727,9 @@ static efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
> return EFI_EXIT(info->exit_status);
> }
>
> + __efi_check_nesting(-1);
> entry(image_handle, &systab);
> + __efi_check_nesting(+1);
>
> /* Should usually never get here */
> return EFI_EXIT(EFI_SUCCESS);
>
Tested-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH 3/3] efi_loader: indent entry/exit prints to show nesting level
2017-07-26 13:55 ` [U-Boot] [PATCH 3/3] efi_loader: indent entry/exit prints to show nesting level Rob Clark
2017-07-26 15:36 ` Alexander Graf
@ 2017-07-26 20:16 ` Heinrich Schuchardt
1 sibling, 0 replies; 11+ messages in thread
From: Heinrich Schuchardt @ 2017-07-26 20:16 UTC (permalink / raw)
To: u-boot
On 07/26/2017 03:55 PM, 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>
> ---
> include/efi_loader.h | 12 ++++++++----
> lib/efi_loader/efi_boottime.c | 28 ++++++++++++++++++++++++++++
> 2 files changed, 36 insertions(+), 4 deletions(-)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 4b49fac84b..88091d4914 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -16,20 +16,24 @@
> #include <linux/list.h>
>
> int __efi_check_nesting(int delta);
> +const char *__efi_nesting_level(int delta);
> +
> /*
> * Enter the u-boot world from UEFI:
> */
> #define EFI_ENTRY(format, ...) do { \
> efi_restore_gd(); \
> + debug("%sEFI: Entry %s(" format ")\n", __efi_nesting_level(1), \
> + __func__, ##__VA_ARGS__); \
> assert(__efi_check_nesting(+1)); \
> - debug("EFI: Entry %s(" format ")\n", __func__, ##__VA_ARGS__); \
> } while(0)
>
> /*
> * Exit the u-boot world back to UEFI:
> */
> #define EFI_EXIT(ret) ({ \
> - debug("EFI: Exit: %s: %u\n", __func__, (u32)((ret) & ~EFI_ERROR_MASK)); \
> + debug("%sEFI: Exit: %s: %u\n", __efi_nesting_level(-1), \
> + __func__, (u32)((ret) & ~EFI_ERROR_MASK)); \
> assert(__efi_check_nesting(-1)); \
> efi_exit_func(ret); \
> })
> @@ -38,12 +42,12 @@ int __efi_check_nesting(int delta);
> * Callback into UEFI world from u-boot:
> */
> #define EFI_CALL(exp) do { \
> - debug("EFI: Call: %s\n", #exp); \
> + debug("%sEFI: Call: %s\n", __efi_nesting_level(1), #exp); \
> assert(__efi_check_nesting(-1)); \
> efi_exit_func(EFI_SUCCESS); \
> exp; \
> efi_restore_gd(); \
> - debug("EFI: Return From: %s\n", #exp); \
> + debug("%sEFI: Return From: %s\n", __efi_nesting_level(-1), #exp); \
> assert(__efi_check_nesting(+1)); \
> } while(0)
>
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index b21df7bd5d..f6c4c162cf 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -92,6 +92,34 @@ efi_status_t efi_exit_func(efi_status_t ret)
> return ret;
> }
>
> +/*
> + * Two spaces per indent level, maxing out at 10.. which ought to be
> + * enough for anyone ;-)
> + */
> +static const char *indent_string(int level)
> +{
> + static const char *indent = " ";
> + const int max = strlen(indent);
> + level = min(max, level * 2);
> + return &indent[max - level];
> +}
> +
> +const char *__efi_nesting_level(int delta)
> +{
> + static int nesting_level;
> + const char *s;
> + /* post-increment, pre-decrement */
> + if (delta > 0) {
> + s = indent_string(nesting_level);
> + nesting_level += delta;
> + } else {
> + nesting_level += delta;
> + s = indent_string(nesting_level);
> + assert(nesting_level >= 0);
> + }
> + return s;
> +}
> +
> /* Low 32 bit */
> #define EFI_LOW32(a) (a & 0xFFFFFFFFULL)
> /* High 32 bit */
>
Tested-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH 2/3] efi_loader: add checking for incorrect use of EFI_ENTRY/EXIT
2017-07-26 20:12 ` Alexander Graf
@ 2017-07-26 20:55 ` Rob Clark
0 siblings, 0 replies; 11+ messages in thread
From: Rob Clark @ 2017-07-26 20:55 UTC (permalink / raw)
To: u-boot
On Wed, Jul 26, 2017 at 4:12 PM, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 26.07.17 18:41, Rob Clark wrote:
>>
>> On Wed, Jul 26, 2017 at 11:25 AM, Alexander Graf <agraf@suse.de> wrote:
>>>
>>>
>>>
>>> On 26.07.17 15:55, Rob Clark wrote:
>>>>
>>>>
>>>> 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 | 5 +++++
>>>> lib/efi_loader/efi_boottime.c | 13 +++++++++++++
>>>> 2 files changed, 18 insertions(+)
>>>>
>>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>>> index 09bab7dbc6..4b49fac84b 100644
>>>> --- a/include/efi_loader.h
>>>> +++ b/include/efi_loader.h
>>>> @@ -15,11 +15,13 @@
>>>> #include <linux/list.h>
>>>> +int __efi_check_nesting(int delta);
>>>> /*
>>>> * Enter the u-boot world from UEFI:
>>>> */
>>>> #define EFI_ENTRY(format, ...) do { \
>>>> efi_restore_gd(); \
>>>> + assert(__efi_check_nesting(+1)); \
>>>> debug("EFI: Entry %s(" format ")\n", __func__, ##__VA_ARGS__);
>>>> \
>>>> } while(0)
>>>> @@ -28,6 +30,7 @@
>>>> */
>>>> #define EFI_EXIT(ret) ({ \
>>>> debug("EFI: Exit: %s: %u\n", __func__, (u32)((ret) &
>>>> ~EFI_ERROR_MASK)); \
>>>> + assert(__efi_check_nesting(-1)); \
>>>> efi_exit_func(ret); \
>>>> })
>>>> @@ -36,10 +39,12 @@
>>>> */
>>>> #define EFI_CALL(exp) do { \
>>>> debug("EFI: Call: %s\n", #exp); \
>>>> + assert(__efi_check_nesting(-1)); \
>>>> efi_exit_func(EFI_SUCCESS); \
>>>> exp; \
>>>> efi_restore_gd(); \
>>>> debug("EFI: Return From: %s\n", #exp); \
>>>> + assert(__efi_check_nesting(+1)); \
>>>> } while(0)
>>>> extern struct efi_runtime_services efi_runtime_services;
>>>> diff --git a/lib/efi_loader/efi_boottime.c
>>>> b/lib/efi_loader/efi_boottime.c
>>>> index 76cafffc1d..b21df7bd5d 100644
>>>> --- a/lib/efi_loader/efi_boottime.c
>>>> +++ b/lib/efi_loader/efi_boottime.c
>>>> @@ -57,6 +57,17 @@ void efi_save_gd(void)
>>>> #endif
>>>> }
>>>> +/* Check for incorrect use of EFI_ENTRY() / EFI_EXIT() */
>>>> +int __efi_check_nesting(int delta)
>>>> +{
>>>> + static int entry_count;
>>>
>>>
>>>
>>> I'd prefer if we marked globals as such (read: somewhere at the top of a
>>> .c
>>> file).
>>
>>
>> hmm, well that does increase the scope unnecessarily, which is why I
>> made it static inside the fxn. But if you can prefer I guess I can
>> put it just above __efi_check_nesting().
>
>
> It's a matter of taste, but in general I find it very useful to know at a
> glimpse where .bss, .rodata and .data come from.
>
>>
>>> Also I think this function would be better off as a static inline in a
>>> header, as it should get compressed quite well.
>>
>>
>> That would mean making entry_count also non-static (ie. broader than
>
>
> Ah, no, entry_count would still be in efi_boottime.c, but the function using
> it would be in a .h file. That way we don't need to do the register
> save/restore dance we need to do for full remote function calls.
Ok.. I'm thinking about just moving these calls into
efi_restore_gd()/efi_exit_func() but leaving the asserts in the macro
(so the assert reflects the proper file/line). Minor downside is the
assert would come before the debug() print, but since it has file/line
I guess that isn't really important.
Fortunately the one place we need to special-case call the
check_nesting() (ie. efi_start_image()) is in the same object file.
I'll give that a go this evening and see how it turns out.
BR,
-R
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-07-26 20:55 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-26 13:55 [U-Boot] [PATCH 1/3] efi_loader: Add an EFI_CALL() macro Rob Clark
2017-07-26 13:55 ` [U-Boot] [PATCH 2/3] efi_loader: add checking for incorrect use of EFI_ENTRY/EXIT Rob Clark
2017-07-26 15:25 ` Alexander Graf
2017-07-26 16:41 ` Rob Clark
2017-07-26 20:12 ` Alexander Graf
2017-07-26 20:55 ` Rob Clark
2017-07-26 20:16 ` Heinrich Schuchardt
2017-07-26 13:55 ` [U-Boot] [PATCH 3/3] efi_loader: indent entry/exit prints to show nesting level Rob Clark
2017-07-26 15:36 ` Alexander Graf
2017-07-26 20:16 ` Heinrich Schuchardt
2017-07-26 20:15 ` [U-Boot] [PATCH 1/3] efi_loader: Add an EFI_CALL() macro Heinrich Schuchardt
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.