diff mbox

[U-Boot,1/1] efi_loader: provide meaningful status code

Message ID 20170625205620.1509-1-xypron.glpk@gmx.de
State Superseded
Delegated to: Alexander Graf
Headers show

Commit Message

Heinrich Schuchardt June 25, 2017, 8:56 p.m. UTC 
Currenty any EFI status other than EFI_SUCCESS is reported as
Application terminated, r = -22

With the patch the status code and its mnemonic is printed.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 cmd/bootefi.c                     | 17 ++++-----
 include/efi.h                     | 47 +++++++++++++++++-------
 include/efi_loader.h              |  3 ++
 lib/efi_loader/efi_image_loader.c | 76 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 122 insertions(+), 21 deletions(-)

Comments

Alexander Graf July 3, 2017, 12:21 p.m. UTC | #1 
On 06/25/2017 10:56 PM, Heinrich Schuchardt wrote:
> Currenty any EFI status other than EFI_SUCCESS is reported as
> Application terminated, r = -22
>
> With the patch the status code and its mnemonic is printed.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

While I think it's very useful to have the status code, I don't want to 
waste 1kb of .rodata on every binary out there - that will only drive 
more people to disable CONFIG_EFI_LOADER which is the opposite of what 
we want.

Can you please respin without the string return for now? Maybe we can 
add a CONFIG_EFI_LOADER_DEBUG option later on which gives us verbose 
printing on demand on a few more bits.


Alex
Simon Glass July 14, 2017, 1:47 p.m. UTC | #2 
Hi,

On 3 July 2017 at 06:21, Alexander Graf <agraf@suse.de> wrote:
> On 06/25/2017 10:56 PM, Heinrich Schuchardt wrote:
>>
>> Currenty any EFI status other than EFI_SUCCESS is reported as
>> Application terminated, r = -22
>>
>> With the patch the status code and its mnemonic is printed.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>
>
> While I think it's very useful to have the status code, I don't want to
> waste 1kb of .rodata on every binary out there - that will only drive more
> people to disable CONFIG_EFI_LOADER which is the opposite of what we want.
>
> Can you please respin without the string return for now? Maybe we can add a
> CONFIG_EFI_LOADER_DEBUG option later on which gives us verbose printing on
> demand on a few more bits.

I think the error messages are useful and would like to see this new
option. Bonus points if they can be in lower case :-)

Regards,
Simon
Alexander Graf July 14, 2017, 1:52 p.m. UTC | #3 
On 14.07.17 15:47, Simon Glass wrote:
> Hi,
> 
> On 3 July 2017 at 06:21, Alexander Graf <agraf@suse.de> wrote:
>> On 06/25/2017 10:56 PM, Heinrich Schuchardt wrote:
>>>
>>> Currenty any EFI status other than EFI_SUCCESS is reported as
>>> Application terminated, r = -22
>>>
>>> With the patch the status code and its mnemonic is printed.
>>>
>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>
>>
>> While I think it's very useful to have the status code, I don't want to
>> waste 1kb of .rodata on every binary out there - that will only drive more
>> people to disable CONFIG_EFI_LOADER which is the opposite of what we want.
>>
>> Can you please respin without the string return for now? Maybe we can add a
>> CONFIG_EFI_LOADER_DEBUG option later on which gives us verbose printing on
>> demand on a few more bits.
> 
> I think the error messages are useful and would like to see this new
> option. Bonus points if they can be in lower case :-)

Yeah, thinking about it again, we can even add the string conversion as 
is but not call it if DEBUG_EFI is not set ;).

And yes, I would really prefer to make all the debugging facilities here 
slightly more fine grained and smarter.


Alex
Alexander Graf July 14, 2017, 1:54 p.m. UTC | #4 
On 14.07.17 15:47, Simon Glass wrote:
> Hi,
> 
> On 3 July 2017 at 06:21, Alexander Graf <agraf@suse.de> wrote:
>> On 06/25/2017 10:56 PM, Heinrich Schuchardt wrote:
>>>
>>> Currenty any EFI status other than EFI_SUCCESS is reported as
>>> Application terminated, r = -22
>>>
>>> With the patch the status code and its mnemonic is printed.
>>>
>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>
>>
>> While I think it's very useful to have the status code, I don't want to
>> waste 1kb of .rodata on every binary out there - that will only drive more
>> people to disable CONFIG_EFI_LOADER which is the opposite of what we want.
>>
>> Can you please respin without the string return for now? Maybe we can add a
>> CONFIG_EFI_LOADER_DEBUG option later on which gives us verbose printing on
>> demand on a few more bits.
> 
> I think the error messages are useful and would like to see this new
> option. Bonus points if they can be in lower case :-)

Ah, one thing I forgot to mention:

If we want to revive the string conversion, it's probably a good idea to 
do it using macros. Something like

#define EFI_STATUS_CASE(x) case x: return stringify(x)
switch(status_code) {
     EFI_STATUS_CASE(EFI_SUCCESS);
     EFI_STATUS_CASE(EFI_amazing_error);
}
#undef EFI_STATUS_CASE

That way we don't repeat the code twice ;).


Alex
diff mbox

Patch

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index fcb223e999..51353b1217 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -247,8 +247,7 @@  static unsigned long do_bootefi_exec(void *efi, void *fdt)
 	debug("%s:%d Jumping to 0x%lx\n", __func__, __LINE__, (long)entry);
 
 	if (setjmp(&loaded_image_info.exit_jmp)) {
-		efi_status_t status = loaded_image_info.exit_status;
-		return status == EFI_SUCCESS ? 0 : -EINVAL;
+		return loaded_image_info.exit_status;
 	}
 	loaded_image_info.entry = entry;
 
@@ -277,7 +276,7 @@  static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
 	char *saddr, *sfdt;
 	unsigned long addr, fdt_addr = 0;
-	int r = 0;
+	unsigned long r;
 
 	if (argc < 2)
 		return CMD_RET_USAGE;
@@ -302,12 +301,14 @@  static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 
 	printf("## Starting EFI application at %08lx ...\n", addr);
 	r = do_bootefi_exec((void *)addr, (void*)fdt_addr);
-	printf("## Application terminated, r = %d\n", r);
+	printf("## Application terminated with status code\n"
+	       "   %lu, %s\n",
+	       r & ~(1UL << (EFI_BITS_PER_LONG - 1)),  efi_status_to_str(r));
 
-	if (r != 0)
-		r = 1;
-
-	return r;
+	if (r != EFI_SUCCESS)
+		return 1;
+	else
+		return 0;
 }
 
 #ifdef CONFIG_SYS_LONGHELP
diff --git a/include/efi.h b/include/efi.h
index 3d587807e8..242da3c3be 100644
--- a/include/efi.h
+++ b/include/efi.h
@@ -39,19 +39,40 @@  struct efi_device_path;
 #define EFI_BITS_PER_LONG	64
 #endif
 
-#define EFI_SUCCESS		0
-#define EFI_LOAD_ERROR		(1 | (1UL << (EFI_BITS_PER_LONG - 1)))
-#define EFI_INVALID_PARAMETER	(2 | (1UL << (EFI_BITS_PER_LONG - 1)))
-#define EFI_UNSUPPORTED		(3 | (1UL << (EFI_BITS_PER_LONG - 1)))
-#define EFI_BAD_BUFFER_SIZE	(4 | (1UL << (EFI_BITS_PER_LONG - 1)))
-#define EFI_BUFFER_TOO_SMALL	(5 | (1UL << (EFI_BITS_PER_LONG - 1)))
-#define EFI_NOT_READY		(6 | (1UL << (EFI_BITS_PER_LONG - 1)))
-#define EFI_DEVICE_ERROR	(7 | (1UL << (EFI_BITS_PER_LONG - 1)))
-#define EFI_WRITE_PROTECTED	(8 | (1UL << (EFI_BITS_PER_LONG - 1)))
-#define EFI_OUT_OF_RESOURCES	(9 | (1UL << (EFI_BITS_PER_LONG - 1)))
-#define EFI_NOT_FOUND		(14 | (1UL << (EFI_BITS_PER_LONG - 1)))
-#define EFI_ACCESS_DENIED	(15 | (1UL << (EFI_BITS_PER_LONG - 1)))
-#define EFI_SECURITY_VIOLATION	(26 | (1UL << (EFI_BITS_PER_LONG - 1)))
+#define EFI_SUCCESS			0
+#define EFI_LOAD_ERROR			(1 | (1UL << (EFI_BITS_PER_LONG - 1)))
+#define EFI_INVALID_PARAMETER		(2 | (1UL << (EFI_BITS_PER_LONG - 1)))
+#define EFI_UNSUPPORTED			(3 | (1UL << (EFI_BITS_PER_LONG - 1)))
+#define EFI_BAD_BUFFER_SIZE		(4 | (1UL << (EFI_BITS_PER_LONG - 1)))
+#define EFI_BUFFER_TOO_SMALL		(5 | (1UL << (EFI_BITS_PER_LONG - 1)))
+#define EFI_NOT_READY			(6 | (1UL << (EFI_BITS_PER_LONG - 1)))
+#define EFI_DEVICE_ERROR		(7 | (1UL << (EFI_BITS_PER_LONG - 1)))
+#define EFI_WRITE_PROTECTED		(8 | (1UL << (EFI_BITS_PER_LONG - 1)))
+#define EFI_OUT_OF_RESOURCES		(9 | (1UL << (EFI_BITS_PER_LONG - 1)))
+#define EFI_VOLUME_CORRUPTED		(10 | (1UL << (EFI_BITS_PER_LONG - 1)))
+#define EFI_VOLUME_FULL			(11 | (1UL << (EFI_BITS_PER_LONG - 1)))
+#define EFI_NO_MEDIA			(12 | (1UL << (EFI_BITS_PER_LONG - 1)))
+#define EFI_MEDIA_CHANGED		(13 | (1UL << (EFI_BITS_PER_LONG - 1)))
+#define EFI_NOT_FOUND			(14 | (1UL << (EFI_BITS_PER_LONG - 1)))
+#define EFI_ACCESS_DENIED		(15 | (1UL << (EFI_BITS_PER_LONG - 1)))
+#define EFI_NO_RESPONSE			(16 | (1UL << (EFI_BITS_PER_LONG - 1)))
+#define EFI_NO_MAPPING			(17 | (1UL << (EFI_BITS_PER_LONG - 1)))
+#define EFI_TIMEOUT			(18 | (1UL << (EFI_BITS_PER_LONG - 1)))
+#define EFI_NOT_STARTED			(19 | (1UL << (EFI_BITS_PER_LONG - 1)))
+#define EFI_ALREADY_STARTED		(20 | (1UL << (EFI_BITS_PER_LONG - 1)))
+#define EFI_ABORTED			(21 | (1UL << (EFI_BITS_PER_LONG - 1)))
+#define EFI_ICMP_ERROR			(22 | (1UL << (EFI_BITS_PER_LONG - 1)))
+#define EFI_TFTP_ERROR			(23 | (1UL << (EFI_BITS_PER_LONG - 1)))
+#define EFI_PROTOCOL_ERROR		(24 | (1UL << (EFI_BITS_PER_LONG - 1)))
+#define EFI_INCOMPATIBLE_VERSION	(25 | (1UL << (EFI_BITS_PER_LONG - 1)))
+#define EFI_SECURITY_VIOLATION		(26 | (1UL << (EFI_BITS_PER_LONG - 1)))
+#define EFI_CRC_ERROR			(27 | (1UL << (EFI_BITS_PER_LONG - 1)))
+#define EFI_END_OF_MEDIA		(28 | (1UL << (EFI_BITS_PER_LONG - 1)))
+#define EFI_END_OF_FILE			(31 | (1UL << (EFI_BITS_PER_LONG - 1)))
+#define EFI_INVALID_LANGUAGE		(32 | (1UL << (EFI_BITS_PER_LONG - 1)))
+#define EFI_COMPROMISED_DATA		(33 | (1UL << (EFI_BITS_PER_LONG - 1)))
+#define EFI_IP_ADDRESS_CONFLICT		(34 | (1UL << (EFI_BITS_PER_LONG - 1)))
+#define EFI_HTTP_ERROR			(35 | (1UL << (EFI_BITS_PER_LONG - 1)))
 
 typedef unsigned long efi_status_t;
 typedef u64 efi_physical_addr_t;
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 99619f53a9..47245049ab 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -145,6 +145,9 @@  extern void *efi_bounce_buffer;
 #define EFI_LOADER_BOUNCE_BUFFER_SIZE (64 * 1024 * 1024)
 #endif
 
+/* Convert EFI status code to string */
+const char *efi_status_to_str(efi_status_t r);
+
 /* Convert strings from normal C strings to uEFI strings */
 static inline void ascii2unicode(u16 *unicode, const char *ascii)
 {
diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
index bc8e04d6d9..c1d09c8806 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -18,6 +18,82 @@  DECLARE_GLOBAL_DATA_PTR;
 const efi_guid_t efi_guid_device_path = DEVICE_PATH_GUID;
 const efi_guid_t efi_guid_loaded_image = LOADED_IMAGE_GUID;
 
+const char *efi_status_to_str(efi_status_t r)
+{
+	switch (r) {
+	case EFI_SUCCESS:
+		return "EFI_SUCCESS";
+	case EFI_LOAD_ERROR:
+		return "EFI_LOAD_ERROR";
+	case EFI_INVALID_PARAMETER:
+		return "EFI_INVALID_PARAMETER";
+	case EFI_UNSUPPORTED:
+		return "EFI_UNSUPPORTED";
+	case EFI_BAD_BUFFER_SIZE:
+		return "EFI_BAD_BUFFER_SIZE";
+	case EFI_BUFFER_TOO_SMALL:
+		return "EFI_BUFFER_TOO_SMALL";
+	case EFI_NOT_READY:
+		return "EFI_NOT_READY";
+	case EFI_DEVICE_ERROR:
+		return "EFI_DEVICE_ERROR";
+	case EFI_WRITE_PROTECTED:
+		return "EFI_WRITE_PROTECTED";
+	case EFI_OUT_OF_RESOURCES:
+		return "EFI_OUT_OF_RESOURCES";
+	case EFI_VOLUME_CORRUPTED:
+		return "EFI_VOLUME_CORRUPTED";
+	case EFI_VOLUME_FULL:
+		return "EFI_VOLUME_FULL";
+	case EFI_NO_MEDIA:
+		return "EFI_NO_MEDIA";
+	case EFI_MEDIA_CHANGED:
+		return "EFI_MEDIA_CHANGED";
+	case EFI_NOT_FOUND:
+		return "EFI_NOT_FOUND";
+	case EFI_ACCESS_DENIED:
+		return "EFI_ACCESS_DENIED";
+	case EFI_NO_RESPONSE:
+		return "EFI_NO_RESPONSE";
+	case EFI_NO_MAPPING:
+		return "EFI_NO_MAPPING";
+	case EFI_TIMEOUT:
+		return "EFI_TIMEOUT";
+	case EFI_NOT_STARTED:
+		return "EFI_NOT_STARTED";
+	case EFI_ALREADY_STARTED:
+		return "EFI_ALREADY_STARTED";
+	case EFI_ABORTED:
+		return "EFI_ABORTED";
+	case EFI_ICMP_ERROR:
+		return "EFI_ICMP_ERROR";
+	case EFI_TFTP_ERROR:
+		return "EFI_TFTP_ERROR";
+	case EFI_PROTOCOL_ERROR:
+		return "EFI_PROTOCOL_ERROR";
+	case EFI_INCOMPATIBLE_VERSION:
+		return "EFI_INCOMPATIBLE_VERSION";
+	case EFI_SECURITY_VIOLATION:
+		return "EFI_SECURITY_VIOLATION";
+	case EFI_CRC_ERROR:
+		return "EFI_CRC_ERROR";
+	case EFI_END_OF_MEDIA:
+		return "EFI_END_OF_MEDIA";
+	case EFI_END_OF_FILE:
+		return "EFI_END_OF_FILE";
+	case EFI_INVALID_LANGUAGE:
+		return "EFI_INVALID_LANGUAGE";
+	case EFI_COMPROMISED_DATA:
+		return "EFI_COMPROMISED_DATA";
+	case EFI_IP_ADDRESS_CONFLICT:
+		return "EFI_IP_ADDRESS_CONFLICT";
+	case EFI_HTTP_ERROR:
+		return "EFI_HTTP_ERROR";
+	default:
+		return "Unknown EFI status code";
+	}
+}
+
 efi_status_t EFIAPI efi_return_handle(void *handle, efi_guid_t *protocol,
 			void **protocol_interface, void *agent_handle,
 			void *controller_handle, uint32_t attributes)