Re: [U-Boot] [PATCH 1/1] efi_loader: allow return value in EFI_CALL

[Rob Clark]

On Thu, Aug 17, 2017 at 12:57 PM, Heinrich Schuchardt
 wrote:
> On 08/17/2017 01:49 PM, Rob Clark wrote:
>> On Tue, Aug 15, 2017 at 3:42 PM, Heinrich Schuchardt  
>> wrote:
>>> Macro EFI_CALL was introduced to call an UEFI function.
>>> Unfortunately is did not support return values.
>>> Most UEFI functions have a return value.
>>>
>>> So let's rename EFI_CALL to EFI_CALL_VOID and introduce a
>>> new EFI_CALL macro that supports return values.
>>>
>>> Signed-off-by: Heinrich Schuchardt 
>>> ---
>>>  include/efi_loader.h  | 16 ++--
>>>  lib/efi_loader/efi_boottime.c |  3 ++-
>>>  2 files changed, 16 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>> index 037cc7c543..1cee10ea0c 100644
>>> --- a/include/efi_loader.h
>>> +++ b/include/efi_loader.h
>>> @@ -41,9 +41,21 @@ const char *__efi_nesting_dec(void);
>>> })
>>>
>>>  /*
>>> - * Callback into UEFI world from u-boot:
>>> + * Call non-void UEFI function from u-boot and retrieve return value:
>>>   */
>>> -#define EFI_CALL(exp) do { \
>>> +#define EFI_CALL(exp) ({ \
>>> +   debug("%sEFI: Call: %s\n", __efi_nesting_inc(), #exp); \
>>> +   assert(__efi_exit_check()); \
>>> +   typeof(exp) r = exp; \
>>> +   assert(__efi_entry_check()); \
>>> +   debug("%sEFI: Return From: %s\n", __efi_nesting_dec(), #exp); \
>>> +   r; \
>>> +})
>>> +
>>
>> So, I had considered something similar, for the purposes of being able
>> to print return value, but in the end decided that the EFI_EXIT()
>> print was usually sufficient (except in cases where you enable DEBUG
>> in some files but not others), and opted for the simpler MACRO(ret =
>> somecall()) approach.. also the longer macro name makes not going over
>> 80 chars harder in some cases ;-)
>>
>> I don't really feel strongly one way or another.. but I guess if we do
>> this we might as well add the return value (cast to u64 and print as
>> hex to cover both ptrs and efi_status_t?) to the "Return From" debug
>> print.
>
> We already print the return value in EFI_EXIT.
> Why should we print it twice?
>
> In c160d2f5ec9
>

the main reason would be enabling debug in a subset of the files (such
as the one calling the API but not the one implementing the API),
which is something I do pretty regularly.  And I guess in the future
we could have payloads installing protocols?

BR,
-R
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 1/1] efi_loader: allow return value in EFI_CALL

[Heinrich Schuchardt]

On 08/17/2017 01:49 PM, Rob Clark wrote:
> On Tue, Aug 15, 2017 at 3:42 PM, Heinrich Schuchardt  
> wrote:
>> Macro EFI_CALL was introduced to call an UEFI function.
>> Unfortunately is did not support return values.
>> Most UEFI functions have a return value.
>>
>> So let's rename EFI_CALL to EFI_CALL_VOID and introduce a
>> new EFI_CALL macro that supports return values.
>>
>> Signed-off-by: Heinrich Schuchardt 
>> ---
>>  include/efi_loader.h  | 16 ++--
>>  lib/efi_loader/efi_boottime.c |  3 ++-
>>  2 files changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>> index 037cc7c543..1cee10ea0c 100644
>> --- a/include/efi_loader.h
>> +++ b/include/efi_loader.h
>> @@ -41,9 +41,21 @@ const char *__efi_nesting_dec(void);
>> })
>>
>>  /*
>> - * Callback into UEFI world from u-boot:
>> + * Call non-void UEFI function from u-boot and retrieve return value:
>>   */
>> -#define EFI_CALL(exp) do { \
>> +#define EFI_CALL(exp) ({ \
>> +   debug("%sEFI: Call: %s\n", __efi_nesting_inc(), #exp); \
>> +   assert(__efi_exit_check()); \
>> +   typeof(exp) r = exp; \
>> +   assert(__efi_entry_check()); \
>> +   debug("%sEFI: Return From: %s\n", __efi_nesting_dec(), #exp); \
>> +   r; \
>> +})
>> +
> 
> So, I had considered something similar, for the purposes of being able
> to print return value, but in the end decided that the EFI_EXIT()
> print was usually sufficient (except in cases where you enable DEBUG
> in some files but not others), and opted for the simpler MACRO(ret =
> somecall()) approach.. also the longer macro name makes not going over
> 80 chars harder in some cases ;-)
> 
> I don't really feel strongly one way or another.. but I guess if we do
> this we might as well add the return value (cast to u64 and print as
> hex to cover both ptrs and efi_status_t?) to the "Return From" debug
> print.

We already print the return value in EFI_EXIT.
Why should we print it twice?

In c160d2f5ec9

Regards

Heinrich

> 
> BR,
> -R
> 
>> +/*
>> + * Call void UEFI function world from u-boot:
>> + */
>> +#define EFI_CALL_VOID(exp) do { \
>> debug("%sEFI: Call: %s\n", __efi_nesting_inc(), #exp); \
>> assert(__efi_exit_check()); \
>> exp; \
>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>> index 43f32385fa..6489a32505 100644
>> --- a/lib/efi_loader/efi_boottime.c
>> +++ b/lib/efi_loader/efi_boottime.c
>> @@ -158,7 +158,8 @@ void efi_signal_event(struct efi_event *event)
>> return;
>> event->signaled = 1;
>> if (event->type & EVT_NOTIFY_SIGNAL) {
>> -   EFI_CALL(event->notify_function(event, 
>> event->notify_context));
>> +   EFI_CALL_VOID(event->notify_function(event,
>> +event->notify_context));
>> }
>>  }
>>
>> --
>> 2.14.1
>>
> 

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 1/1] efi_loader: allow return value in EFI_CALL

[Rob Clark]

On Tue, Aug 15, 2017 at 3:42 PM, Heinrich Schuchardt  wrote:
> Macro EFI_CALL was introduced to call an UEFI function.
> Unfortunately is did not support return values.
> Most UEFI functions have a return value.
>
> So let's rename EFI_CALL to EFI_CALL_VOID and introduce a
> new EFI_CALL macro that supports return values.
>
> Signed-off-by: Heinrich Schuchardt 
> ---
>  include/efi_loader.h  | 16 ++--
>  lib/efi_loader/efi_boottime.c |  3 ++-
>  2 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 037cc7c543..1cee10ea0c 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -41,9 +41,21 @@ const char *__efi_nesting_dec(void);
> })
>
>  /*
> - * Callback into UEFI world from u-boot:
> + * Call non-void UEFI function from u-boot and retrieve return value:
>   */
> -#define EFI_CALL(exp) do { \
> +#define EFI_CALL(exp) ({ \
> +   debug("%sEFI: Call: %s\n", __efi_nesting_inc(), #exp); \
> +   assert(__efi_exit_check()); \
> +   typeof(exp) r = exp; \
> +   assert(__efi_entry_check()); \
> +   debug("%sEFI: Return From: %s\n", __efi_nesting_dec(), #exp); \
> +   r; \
> +})
> +

So, I had considered something similar, for the purposes of being able
to print return value, but in the end decided that the EFI_EXIT()
print was usually sufficient (except in cases where you enable DEBUG
in some files but not others), and opted for the simpler MACRO(ret =
somecall()) approach.. also the longer macro name makes not going over
80 chars harder in some cases ;-)

I don't really feel strongly one way or another.. but I guess if we do
this we might as well add the return value (cast to u64 and print as
hex to cover both ptrs and efi_status_t?) to the "Return From" debug
print.

BR,
-R

> +/*
> + * Call void UEFI function world from u-boot:
> + */
> +#define EFI_CALL_VOID(exp) do { \
> debug("%sEFI: Call: %s\n", __efi_nesting_inc(), #exp); \
> assert(__efi_exit_check()); \
> exp; \
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 43f32385fa..6489a32505 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -158,7 +158,8 @@ void efi_signal_event(struct efi_event *event)
> return;
> event->signaled = 1;
> if (event->type & EVT_NOTIFY_SIGNAL) {
> -   EFI_CALL(event->notify_function(event, 
> event->notify_context));
> +   EFI_CALL_VOID(event->notify_function(event,
> +event->notify_context));
> }
>  }
>
> --
> 2.14.1
>
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot