On Wed, Jul 26, 2017 at 4:12 PM, Alexander Graf <ag...@suse.de> wrote:
>
>
> On 26.07.17 18:41, Rob Clark wrote:
>>
>> On Wed, Jul 26, 2017 at 11:25 AM, Alexander Graf <ag...@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 <robdcl...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
