enough UEFI for standard distro boot

Needed to support efi file protocol. The fallback.efi loader wants to be able to read the contents of the /EFI directory to find an OS to boot.

For reference, the expected EFI semantics are described in (v2.7 of UEFI spec) in section 13.5 (page 609). Or for convenience, see:

http://wiki.phoenix.com/wiki/index.php/EFI_FILE_PROTOCOL#Read.28.29

The EFI level semantics are implemented in a later patch, so they are not too important to the understanding of this patch.

Signed-off-by Rob Clark <[email protected]>

---
 fs/fs.c      | 25 +++++++++++++++++++++++++
 include/fs.h | 21 +++++++++++++++++++++
 2 files changed, 46 insertions(+)

Quoted 12 lines · On Freitag, 4. August 2017 21:31:43 CEST Rob Clark wrote:

Needed to support efi file protocol. The fallback.efi loader wants to be able to read the contents of the /EFI directory to find an OS to boot.

For reference, the expected EFI semantics are described in (v2.7 of UEFI spec) in section 13.5 (page 609). Or for convenience, see:

http://wiki.phoenix.com/wiki/index.php/EFI_FILE_PROTOCOL#Read.28.29

The EFI level semantics are implemented in a later patch, so they are not too important to the understanding of this patch.

Quoted 5 lines

Signed-off-by Rob Clark <[email protected]>

---
 fs/fs.c      | 25 +++++++++++++++++++++++++
 include/fs.h | 21 +++++++++++++++++++++
 2 files changed, 46 insertions(+)

Still, the commit message is in no way helpful when trying to understand what your changes are actually doing.

You introduce an arbitrary new API in the filesystem level (you neither expose an existing API, nor are you implementing the API requested by EFI, nor anything roughly resembling it).

The API you expose adds an index-based directory lookup, while EFI wants an POSIX-like directory stream. After reading through both the EFI spec and U- Boots file system code, its clear you want to have some matching layer between the mostly stateless U-Boot filesystem layer and the stateful EFI API.

Please provide a thorough description why you create this new API in the fs layer, state that it is a hack to achieve what you want. If sometime later someone else wants to clean this up (both the FAT implementation, and the API), she/he should not have to go through all the code.

For now, pending conclusion on proposal about using c11 and u"string" instead of L"string" plus -fshort-wchar.

Background: UEFI uses utf16 strings universally. An UEFI implementation that does not use -fshort-wchar has to do a lot of cumbersome charset conversion back/forth. Mixing object files that use -fshort-wchar and others that do not is a bad idea (and gcc will warn about it). There are 3 reasonable solutions that I can think of:

1) Use -fshort-wchar across the board. We don't dynamically link against other shared libraries at runtime, so there isn't much downside to this approach. Heinrich brought up ext4, which uses utf32, but I guess this mostly matters for fs_ls(), and it does not seem so bad for it to do utf32->utf8 conversion.

2) Use -fshort-wchar only if CONFIG_EFI_LOADER=y.. UEFI only requires fat/vfat so we don't need ext4 and efi loader at the same time.

3) Go with Heinrich's suggestion of requiring c11. Possibly this requirement could be loosened to only require c11 for efi loader. This seems like the best approach, and at least no one has so far brought up any objection to his proposal.

Not-signed-off-by Rob Clark <[email protected]>

---
 config.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

config.mk +1 -1

index b77d58903c..5ad9e7198d 100644
@@ -74,7 +74,7 @@ endif
 RELFLAGS := $(PLATFORM_RELFLAGS)
 
 PLATFORM_CPPFLAGS += $(RELFLAGS)
-PLATFORM_CPPFLAGS += -pipe
+PLATFORM_CPPFLAGS += -pipe -fshort-wchar
 
 LDFLAGS += $(PLATFORM_LDFLAGS)
 LDFLAGS_FINAL += -Bstatic

Quoted 23 lines · Rob Clark

For now, pending conclusion on proposal about using c11 and u"string" instead of L"string" plus -fshort-wchar.

Background: UEFI uses utf16 strings universally. An UEFI implementation that does not use -fshort-wchar has to do a lot of cumbersome charset conversion back/forth. Mixing object files that use -fshort-wchar and others that do not is a bad idea (and gcc will warn about it). There are 3 reasonable solutions that I can think of:

1) Use -fshort-wchar across the board. We don't dynamically link against other shared libraries at runtime, so there isn't much downside to this approach. Heinrich brought up ext4, which uses utf32, but I guess this mostly matters for fs_ls(), and it does not seem so bad for it to do utf32->utf8 conversion.

2) Use -fshort-wchar only if CONFIG_EFI_LOADER=y.. UEFI only requires fat/vfat so we don't need ext4 and efi loader at the same time.

3) Go with Heinrich's suggestion of requiring c11. Possibly this requirement could be loosened to only require c11 for efi loader. This seems like the best approach, and at least no one has so far brought up any objection to his proposal.

Quoted 1 line

Not-signed-off-by Rob Clark <[email protected]>

Do I understand you right?
You do not want the current version of the patch series merged?

It might have been better to replace [PATCH] by [RFC] in the subject lines of the patch set in this case.

I am currently trying to find out if the switch to C11 is possible, cf.
https://travis-ci.org/xypron2/u-boot/builds/261098363.

Unfortunately Travis uses gcc 4.8.2 for some jobs which does not yet support C11.

Furthermore there seems to be some problem in disk/part_efi.c with PARTITION_SYSTEM_GUID.

Best regards

Heinrich

Quoted 18 lines

---
 config.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

config.mk +1 -1

index b77d58903c..5ad9e7198d 100644
@@ -74,7 +74,7 @@ endif
 RELFLAGS := $(PLATFORM_RELFLAGS)
 
 PLATFORM_CPPFLAGS += $(RELFLAGS)
-PLATFORM_CPPFLAGS += -pipe
+PLATFORM_CPPFLAGS += -pipe -fshort-wchar
 
 LDFLAGS += $(PLATFORM_LDFLAGS)
 LDFLAGS_FINAL += -Bstatic

We'll eventually want these in a few places in efi_loader, and also vsprintf.

Signed-off-by Rob Clark <[email protected]>

---
 common/Makefile              |  1 +
 common/charset.c             | 81 ++++++++++++++++++++++++++++++++++++++++++++
 include/charset.h            | 18 ++++++++++
 lib/efi_loader/efi_console.c | 17 ++--------
 4 files changed, 103 insertions(+), 14 deletions(-)
 create mode 100644 common/charset.c
 create mode 100644 include/charset.h

common/Makefile +1 -0

index 60681c845c..44c8e1ba52 100644
@@ -175,5 +175,6 @@ obj-$(CONFIG_CMD_DFU) += dfu.o
 obj-y += command.o
 obj-y += s_record.o
 obj-y += xyzModem.o
+obj-y += charset.o
 
 CFLAGS_env_embedded.o := -Wa,--no-warn -DENV_CRC=$(shell tools/envcrc 2>/dev/null)

common/charset.c new file +81 -0

new file mode 100644
index 0000000000..eaff2e542e
@@ -0,0 +1,81 @@
+/*
+ *  charset conversion utils
+ *
+ *  Copyright (c) 2017 Rob Clark
+ *
+ *  SPDX-License-Identifier:     GPL-2.0+
+ */
+
+#include <common.h>
+#include <charset.h>
+
+/*
+ * utf8/utf16 conversion mostly lifted from grub
+ */
+
+size_t utf16_strlen(uint16_t *in)
+{
+       size_t i;
+       for (i = 0; in[i]; i++);
+       return i;
+}
+
+size_t utf16_strnlen(const uint16_t *in, size_t count)
+{
+       size_t i;
+       for (i = 0; count-- && in[i]; i++);
+       return i;
+}
+
+/* Convert UTF-16 to UTF-8.  */
+uint8_t *utf16_to_utf8(uint8_t *dest, const uint16_t *src, size_t size)
+{
+       uint32_t code_high = 0;
+
+       while (size--) {
+               uint32_t code = *src++;
+
+               if (code_high) {
+                       if (code >= 0xDC00 && code <= 0xDFFF) {
+                               /* Surrogate pair.  */
+                               code = ((code_high - 0xD800) << 10) + (code - 0xDC00) + 0x10000;
+
+                               *dest++ = (code >> 18) | 0xF0;
+                               *dest++ = ((code >> 12) & 0x3F) | 0x80;
+                               *dest++ = ((code >> 6) & 0x3F) | 0x80;
+                               *dest++ = (code & 0x3F) | 0x80;
+                       } else {
+                               /* Error...  */
+                               *dest++ = '?';
+                               /* *src may be valid. Don't eat it.  */
+                               src--;
+                       }
+
+                       code_high = 0;
+               } else {
+                       if (code <= 0x007F) {
+                               *dest++ = code;
+                       } else if (code <= 0x07FF) {
+                               *dest++ = (code >> 6) | 0xC0;
+                               *dest++ = (code & 0x3F) | 0x80;
+                       } else if (code >= 0xD800 && code <= 0xDBFF) {
+                               code_high = code;
+                               continue;
+                       } else if (code >= 0xDC00 && code <= 0xDFFF) {
+                               /* Error... */
+                               *dest++ = '?';
+                       } else if (code < 0x10000) {
+                               *dest++ = (code >> 12) | 0xE0;
+                               *dest++ = ((code >> 6) & 0x3F) | 0x80;
+                               *dest++ = (code & 0x3F) | 0x80;
+                       } else {
+                               *dest++ = (code >> 18) | 0xF0;
+                               *dest++ = ((code >> 12) & 0x3F) | 0x80;
+                               *dest++ = ((code >> 6) & 0x3F) | 0x80;
+                               *dest++ = (code & 0x3F) | 0x80;
+                       }
+               }
+       }
+
+       return dest;
+}

include/charset.h new file +6 -0

new file mode 100644
index 0000000000..2ee1172182
@@ -0,0 +1,18 @@
+/*
+ *  charset conversion utils
+ *
+ *  Copyright (c) 2017 Rob Clark
+ *
+ *  SPDX-License-Identifier:     GPL-2.0+

include/charset.h new file +2 -0

+
+#endif /* __CHARSET_H_ */

lib/efi_loader/efi_console.c +3 -15

index 5ebce4b544..3fc82b8726 100644
@@ -7,6 +7,7 @@
  */
 
 #include <common.h>
+#include <charset.h>
 #include <efi_loader.h>
 
 static bool console_size_queried;

@@ -138,20 +139,8 @@ static efi_status_t EFIAPI efi_cout_reset(
 
 static void print_unicode_in_utf8(u16 c)
 {
-       char utf8[4] = { 0 };
-       char *b = utf8;
-
-       if (c < 0x80) {
-               *(b++) = c;
-       } else if (c < 0x800) {
-               *(b++) = 192 + c / 64;
-               *(b++) = 128 + c % 64;
-       } else {
-               *(b++) = 224 + c / 4096;
-               *(b++) = 128 + c / 64 % 64;
-               *(b++) = 128 + c % 64;
-       }
-
+       char utf8[MAX_UTF8_PER_UTF16] = { 0 };
+       utf16_to_utf8((u8 *)utf8, &c, 1);
        puts(utf8);
 }
 
--

2.13.0

We'll eventually want these in a few places in efi_loader, and also vsprintf.

Signed-off-by Rob Clark <[email protected]>

---
 common/Makefile              |  1 +
 common/charset.c             | 81 ++++++++++++++++++++++++++++++++++++++++++++
 include/charset.h            | 18 ++++++++++
 lib/efi_loader/efi_console.c | 17 ++--------
 4 files changed, 103 insertions(+), 14 deletions(-)
 create mode 100644 common/charset.c
 create mode 100644 include/charset.h

common/Makefile +1 -0

index 60681c845c..44c8e1ba52 100644
@@ -175,5 +175,6 @@ obj-$(CONFIG_CMD_DFU) += dfu.o
 obj-y += command.o
 obj-y += s_record.o
 obj-y += xyzModem.o
+obj-y += charset.o
 
 CFLAGS_env_embedded.o := -Wa,--no-warn -DENV_CRC=$(shell tools/envcrc 2>/dev/null)

common/charset.c new file +25 -0

new file mode 100644
index 0000000000..eaff2e542e
@@ -0,0 +1,81 @@
+/*
+ *  charset conversion utils
+ *
+ *  Copyright (c) 2017 Rob Clark
+ *
+ *  SPDX-License-Identifier:     GPL-2.0+
+ */
+
+#include <common.h>
+#include <charset.h>
+
+/*
+ * utf8/utf16 conversion mostly lifted from grub
+ */
+
+size_t utf16_strlen(uint16_t *in)
+{
+	size_t i;
+	for (i = 0; in[i]; i++);
+	return i;
+}
+
+size_t utf16_strnlen(const uint16_t *in, size_t count)
+{
+	size_t i;

common/charset.c new file +21 -0

+		uint32_t code = *src++;
+
+		if (code_high) {
+			if (code >= 0xDC00 && code <= 0xDFFF) {
+				/* Surrogate pair.  */
+				code = ((code_high - 0xD800) << 10) + (code - 0xDC00) + 0x10000;
+
+				*dest++ = (code >> 18) | 0xF0;
+				*dest++ = ((code >> 12) & 0x3F) | 0x80;
+				*dest++ = ((code >> 6) & 0x3F) | 0x80;
+				*dest++ = (code & 0x3F) | 0x80;
+			} else {
+				/* Error...  */
+				*dest++ = '?';
+				/* *src may be valid. Don't eat it.  */
+				src--;
+			}
+
+			code_high = 0;
+		} else {
+			if (code <= 0x007F) {

common/charset.c new file +15 -0

+			} else if (code < 0x10000) {
+				*dest++ = (code >> 12) | 0xE0;
+				*dest++ = ((code >> 6) & 0x3F) | 0x80;
+				*dest++ = (code & 0x3F) | 0x80;
+			} else {
+				*dest++ = (code >> 18) | 0xF0;
+				*dest++ = ((code >> 12) & 0x3F) | 0x80;
+				*dest++ = ((code >> 6) & 0x3F) | 0x80;
+				*dest++ = (code & 0x3F) | 0x80;
+			}
+		}
+	}
+
+	return dest;
+}

include/charset.h new file +18 -0

new file mode 100644
index 0000000000..2ee1172182
@@ -0,0 +1,18 @@
+/*
+ *  charset conversion utils
+ *
+ *  Copyright (c) 2017 Rob Clark
+ *
+ *  SPDX-License-Identifier:     GPL-2.0+
+ */
+
+#ifndef __CHARSET_H_
+#define __CHARSET_H_
+
+#define MAX_UTF8_PER_UTF16 4
+
+size_t utf16_strlen(uint16_t *in);
+size_t utf16_strnlen(const uint16_t *in, size_t count);
+uint8_t *utf16_to_utf8(uint8_t *dest, const uint16_t *src, size_t size);
+
+#endif /* __CHARSET_H_ */

lib/efi_loader/efi_console.c +3 -14

index 5ebce4b544..3fc82b8726 100644
@@ -7,6 +7,7 @@
  */
 
 #include <common.h>
+#include <charset.h>
 #include <efi_loader.h>
 
 static bool console_size_queried;

@@ -138,20 +139,8 @@ static efi_status_t EFIAPI efi_cout_reset(
 
 static void print_unicode_in_utf8(u16 c)
 {
-	char utf8[4] = { 0 };
-	char *b = utf8;
-
-	if (c < 0x80) {
-		*(b++) = c;
-	} else if (c < 0x800) {
-		*(b++) = 192 + c / 64;
-		*(b++) = 128 + c % 64;
-	} else {
-		*(b++) = 224 + c / 4096;
-		*(b++) = 128 + c / 64 % 64;
-		*(b++) = 128 + c % 64;
-	}
-
+	char utf8[MAX_UTF8_PER_UTF16] = { 0 };
+	utf16_to_utf8((u8 *)utf8, &c, 1);
 	puts(utf8);
 }
 

Quoted 59 lines · Rob Clark

We'll eventually want these in a few places in efi_loader, and also vsprintf.

Signed-off-by Rob Clark <[email protected]>

---
 common/Makefile              |  1 +
 common/charset.c             | 81 ++++++++++++++++++++++++++++++++++++++++++++
 include/charset.h            | 18 ++++++++++
 lib/efi_loader/efi_console.c | 17 ++--------
 4 files changed, 103 insertions(+), 14 deletions(-)
 create mode 100644 common/charset.c
 create mode 100644 include/charset.h

common/Makefile +1 -0

index 60681c845c..44c8e1ba52 100644
@@ -175,5 +175,6 @@ obj-$(CONFIG_CMD_DFU) += dfu.o
 obj-y += command.o
 obj-y += s_record.o
 obj-y += xyzModem.o
+obj-y += charset.o
 
 CFLAGS_env_embedded.o := -Wa,--no-warn -DENV_CRC=$(shell tools/envcrc 2>/dev/null)

common/charset.c new file +29 -0

new file mode 100644
index 0000000000..eaff2e542e
@@ -0,0 +1,81 @@
+/*
+ *  charset conversion utils
+ *
+ *  Copyright (c) 2017 Rob Clark
+ *
+ *  SPDX-License-Identifier:     GPL-2.0+
+ */
+
+#include <common.h>
+#include <charset.h>
+
+/*
+ * utf8/utf16 conversion mostly lifted from grub
+ */
+
+size_t utf16_strlen(uint16_t *in)
+{
+     size_t i;
+     for (i = 0; in[i]; i++);
+     return i;
+}
+
+size_t utf16_strnlen(const uint16_t *in, size_t count)
+{
+     size_t i;
+     for (i = 0; count-- && in[i]; i++);
+     return i;
+}
+

common/charset.c line 35

Quoted 6 lines

common/charset.c new file +6 -0

+/* Convert UTF-16 to UTF-8.  */
+uint8_t *utf16_to_utf8(uint8_t *dest, const uint16_t *src, size_t size)
+{
+     uint32_t code_high = 0;
+
+     while (size--) {

We should not read past the trailing null world. Check *src == 0 somewhere.

Quoted 21 lines

common/charset.c new file +21 -0

+             uint32_t code = *src++;
+
+             if (code_high) {
+                     if (code >= 0xDC00 && code <= 0xDFFF) {
+                             /* Surrogate pair.  */
+                             code = ((code_high - 0xD800) << 10) + (code - 0xDC00) + 0x10000;
+
+                             *dest++ = (code >> 18) | 0xF0;
+                             *dest++ = ((code >> 12) & 0x3F) | 0x80;
+                             *dest++ = ((code >> 6) & 0x3F) | 0x80;
+                             *dest++ = (code & 0x3F) | 0x80;
+                     } else {
+                             /* Error...  */
+                             *dest++ = '?';
+                             /* *src may be valid. Don't eat it.  */
+                             src--;
+                     }
+
+                     code_high = 0;
+             } else {
+                     if (code <= 0x007F) {

common/charset.c line 66

Quoted 10 lines

common/charset.c new file +10 -0

+                             *dest++ = code;
+                     } else if (code <= 0x07FF) {
+                             *dest++ = (code >> 6) | 0xC0;
+                             *dest++ = (code & 0x3F) | 0x80;
+                     } else if (code >= 0xD800 && code <= 0xDBFF) {
+                             code_high = code;
+                             continue;
+                     } else if (code >= 0xDC00 && code <= 0xDFFF) {
+                             /* Error... */
+                             *dest++ = '?';

The error handling is somewhat inconsistent:

No output if code 0xD800-0xDBFF is the last word.
Output '?' for 0xDC00-0xDFFF where not expected.
Output extraneous '?' for 0xD800-0xDBFF not followed by 0xDC00-0xDBFF.

Best regards

Heinrich

Quoted 75 lines

common/charset.c new file +15 -0

+                     } else if (code < 0x10000) {
+                             *dest++ = (code >> 12) | 0xE0;
+                             *dest++ = ((code >> 6) & 0x3F) | 0x80;
+                             *dest++ = (code & 0x3F) | 0x80;
+                     } else {
+                             *dest++ = (code >> 18) | 0xF0;
+                             *dest++ = ((code >> 12) & 0x3F) | 0x80;
+                             *dest++ = ((code >> 6) & 0x3F) | 0x80;
+                             *dest++ = (code & 0x3F) | 0x80;
+                     }
+             }
+     }
+
+     return dest;
+}

include/charset.h new file +18 -0

new file mode 100644
index 0000000000..2ee1172182
@@ -0,0 +1,18 @@
+/*
+ *  charset conversion utils
+ *
+ *  Copyright (c) 2017 Rob Clark
+ *
+ *  SPDX-License-Identifier:     GPL-2.0+
+ */
+
+#ifndef __CHARSET_H_
+#define __CHARSET_H_
+
+#define MAX_UTF8_PER_UTF16 4
+
+size_t utf16_strlen(uint16_t *in);
+size_t utf16_strnlen(const uint16_t *in, size_t count);
+uint8_t *utf16_to_utf8(uint8_t *dest, const uint16_t *src, size_t size);
+
+#endif /* __CHARSET_H_ */

lib/efi_loader/efi_console.c +3 -14

index 5ebce4b544..3fc82b8726 100644
@@ -7,6 +7,7 @@
  */
 
 #include <common.h>
+#include <charset.h>
 #include <efi_loader.h>
 
 static bool console_size_queried;

@@ -138,20 +139,8 @@ static efi_status_t EFIAPI efi_cout_reset(
 
 static void print_unicode_in_utf8(u16 c)
 {
-     char utf8[4] = { 0 };
-     char *b = utf8;
-
-     if (c < 0x80) {
-             *(b++) = c;
-     } else if (c < 0x800) {
-             *(b++) = 192 + c / 64;
-             *(b++) = 128 + c % 64;
-     } else {
-             *(b++) = 224 + c / 4096;
-             *(b++) = 128 + c / 64 % 64;
-             *(b++) = 128 + c % 64;
-     }
-
+     char utf8[MAX_UTF8_PER_UTF16] = { 0 };
+     utf16_to_utf8((u8 *)utf8, &c, 1);
      puts(utf8);
 }
 

This is convenient for efi_loader which deals a lot with utf16.

Signed-off-by Rob Clark <[email protected]>

---
 lib/vsprintf.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

lib/vsprintf.c +1 -0

index 874a2951f7..0c40f852ce 100644
@@ -17,6 +17,7 @@
 #include <linux/ctype.h>
 
 #include <common.h>
+#include <charset.h>
 
 #include <div64.h>
 #define noinline __attribute__((noinline))

lib/vsprintf.c +14 -0

+	int i;
+
+	*utf16_to_utf8(utf8, str, len) = '\0';
+
+	if (!(flags & LEFT))
+		while (len < field_width--)
+			ADDCH(buf, ' ');
+	for (i = 0; i < len; ++i)
+		ADDCH(buf, utf8[i]);
+	while (len < field_width--)
+		ADDCH(buf, ' ');
+	return buf;
+}
+
 #ifdef CONFIG_CMD_NET
 static const char hex_asc[] = "0123456789abcdef";
 #define hex_asc_lo(x)	hex_asc[((x) & 0x0f)]

lib/vsprintf.c +6 -0

+				str = string16(str, end, va_arg(args, u16 *),
+					       field_width, precision, flags);
+			} else {
+				str = string(str, end, va_arg(args, char *),
+					     field_width, precision, flags);
+			}
 			continue;
 
 		case 'p':

Quoted 3 lines · Rob Clark

This is convenient for efi_loader which deals a lot with utf16.

Signed-off-by Rob Clark <[email protected]>

Please, put this patch together with
[PATCH] vsprintf.c: add GUID printing
https://patchwork.ozlabs.org/patch/798362/
and
[PATCH v0 06/20] common: add some utf16 handling helpers
https://patchwork.ozlabs.org/patch/797968/
into a separate patch series.

These three patches can be reviewed independently of the efi_loader patches and probably will not be integrated via the efi-next tree.

Quoted 20 lines

---
 lib/vsprintf.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

lib/vsprintf.c +1 -0

index 874a2951f7..0c40f852ce 100644
@@ -17,6 +17,7 @@
 #include <linux/ctype.h>
 
 #include <common.h>
+#include <charset.h>
 
 #include <div64.h>
 #define noinline __attribute__((noinline))

@@ -270,6 +271,26 @@ static char *string(char *buf, char *end, char *s, int field_width,
      return buf;
 }
 

lib/vsprintf.c static char *string(char *buf, char *end, char *s, int field_width, · line 277

Quoted 4 lines

lib/vsprintf.c +4 -0

+static char *string16(char *buf, char *end, u16 *s, int field_width,
+             int precision, int flags)
+{
+     u16 *str = s ? s : L"<NULL>";

Please, do not use the L-notation here as it requires -fshort-wchar. As we currently cannot switch the complete project to C11 you cannot use the u-notation either.

lib/vsprintf.c static char *string(char *buf, char *end, char *s, int field_width, · line 279

Quoted 3 lines

lib/vsprintf.c +2 -0

+     int len = utf16_strnlen(str, precision);
+     u8 utf8[len * MAX_UTF8_PER_UTF16];

Didn't you forget 1 byte for \0 here?

This is what strlnlen does:

The strnlen() function returns the number of characters in the string pointed to by s, **excluding** the terminating null byte ('\0'), but at most maxlen.

I would expect the exclusion of the terminating null word by an utf16_strnlen function.

Quoted 20 lines

lib/vsprintf.c +14 -0

+     int i;
+
+     *utf16_to_utf8(utf8, str, len) = '\0';
+
+     if (!(flags & LEFT))
+             while (len < field_width--)
+                     ADDCH(buf, ' ');
+     for (i = 0; i < len; ++i)
+             ADDCH(buf, utf8[i]);
+     while (len < field_width--)
+             ADDCH(buf, ' ');
+     return buf;
+}
+
 #ifdef CONFIG_CMD_NET
 static const char hex_asc[] = "0123456789abcdef";
 #define hex_asc_lo(x)        hex_asc[((x) & 0x0f)]

@@ -528,8 +549,13 @@ repeat:
                      continue;
 

lib/vsprintf.c repeat: · line 552

Quoted 4 lines

lib/vsprintf.c +1 -2

              case 's':
-                     str = string(str, end, va_arg(args, char *),
-                                  field_width, precision, flags);
+                     if (qualifier == 'l') {

%ls refers to wchar with implementation dependent width in the C standard. There is no qualifier for 16-bit wchar. Couldn't we use %us here in reference to the u-notation ( u'MyString' ). This would leave the path open for a standard compliant '%ls'.

Best regards

Heinrich

Quoted 10 lines

lib/vsprintf.c +6 -0

+                             str = string16(str, end, va_arg(args, u16 *),
+                                            field_width, precision, flags);
+                     } else {
+                             str = string(str, end, va_arg(args, char *),
+                                          field_width, precision, flags);
+                     }
                      continue;
 
              case 'p':

Quoted 13 lines · Heinrich Schuchardt

Quoted 3 lines · Rob Clark

This is convenient for efi_loader which deals a lot with utf16.

Signed-off-by Rob Clark <[email protected]>

Please, put this patch together with
[PATCH] vsprintf.c: add GUID printing
https://patchwork.ozlabs.org/patch/798362/
and
[PATCH v0 06/20] common: add some utf16 handling helpers
https://patchwork.ozlabs.org/patch/797968/
into a separate patch series.

Quoted 2 lines

These three patches can be reviewed independently of the efi_loader patches and probably will not be integrated via the efi-next tree.

I'll resend these as a separate patchset, and just not in next revision of efi_loader patchset that it is a dependency

Quoted 20 lines

Quoted 20 lines

---
 lib/vsprintf.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

lib/vsprintf.c +1 -0

index 874a2951f7..0c40f852ce 100644
@@ -17,6 +17,7 @@
 #include <linux/ctype.h>
 
 #include <common.h>
+#include <charset.h>
 
 #include <div64.h>
 #define noinline __attribute__((noinline))

@@ -270,6 +271,26 @@ static char *string(char *buf, char *end, char *s, int field_width,
      return buf;
 }
 

Quoted 7 lines

lib/vsprintf.c static char *string(char *buf, char *end, char *s, int field_width, · line 277

Quoted 4 lines

lib/vsprintf.c +4 -0

+static char *string16(char *buf, char *end, u16 *s, int field_width,
+             int precision, int flags)
+{
+     u16 *str = s ? s : L"<NULL>";

Please, do not use the L-notation here as it requires -fshort-wchar. As we currently cannot switch the complete project to C11 you cannot use the u-notation either.

current plan was to either switch whole project to -fshort-wchar or c11 and rework these patches (as well as a few patches in the efi_loader patchset). (In the c11 case, I'm not sure what we'll use as the fmt string, since afaict that isn't specified. We could use %S although that seems to be a deprecated way to do %ls, or something different like %A, I guess)..

how far are we from c11? If there is stuff I can do to help let me know. If feasible, I'd rather do that first rather than have a bunch of stuff in vsprintf and elsewhere that needs to be cleaned up later after the switch.

Quoted 7 lines

lib/vsprintf.c static char *string(char *buf, char *end, char *s, int field_width, · line 279

Quoted 4 lines

lib/vsprintf.c +2 -0

+     int len = utf16_strnlen(str, precision);
+     u8 utf8[len * MAX_UTF8_PER_UTF16];

Didn't you forget 1 byte for \0 here?

Quoted 8 lines

This is what strlnlen does:

The strnlen() function returns the number of characters in the string pointed to by s, **excluding** the terminating null byte ('\0'), but at most maxlen.

I would expect the exclusion of the terminating null word by an utf16_strnlen function.

you are right, but fixing the wrong problem.. the code is definitely wrong since length of a utf16 string != length of a utf8 string, and we don't need to append a null terminator.. so my logic below using 'len' is wrong. I'll fix that in the next version.

Quoted 20 lines

Quoted 20 lines

lib/vsprintf.c +14 -0

+     int i;
+
+     *utf16_to_utf8(utf8, str, len) = '\0';
+
+     if (!(flags & LEFT))
+             while (len < field_width--)
+                     ADDCH(buf, ' ');
+     for (i = 0; i < len; ++i)
+             ADDCH(buf, utf8[i]);
+     while (len < field_width--)
+             ADDCH(buf, ' ');
+     return buf;
+}
+
 #ifdef CONFIG_CMD_NET
 static const char hex_asc[] = "0123456789abcdef";
 #define hex_asc_lo(x)        hex_asc[((x) & 0x0f)]

@@ -528,8 +549,13 @@ repeat:
                      continue;
 

Quoted 9 lines

lib/vsprintf.c repeat: · line 552

Quoted 4 lines

lib/vsprintf.c +1 -2

              case 's':
-                     str = string(str, end, va_arg(args, char *),
-                                  field_width, precision, flags);
+                     if (qualifier == 'l') {

%ls refers to wchar with implementation dependent width in the C standard. There is no qualifier for 16-bit wchar. Couldn't we use %us here in reference to the u-notation ( u'MyString' ). This would leave the path open for a standard compliant '%ls'.

hmm, yeah, that is a clever idea, and I like it better than %A or %S.. so if we go the c11 route I'll do that. The c11 committee should have thought of that ;-)

BR,
-R

Quoted 14 lines

Best regards

Heinrich

Quoted 10 lines

lib/vsprintf.c +6 -0

+                             str = string16(str, end, va_arg(args, u16 *),
+                                            field_width, precision, flags);
+                     } else {
+                             str = string(str, end, va_arg(args, char *),
+                                          field_width, precision, flags);
+                     }
                      continue;
 
              case 'p':

Am 09.08.2017 um 00:08 schrieb Heinrich Schuchardt <[email protected]>:

Quoted 56 lines · Rob Clark

Quoted 14 lines · Heinrich Schuchardt

Quoted 4 lines · Rob Clark

This is convenient for efi_loader which deals a lot with utf16.

Signed-off-by Rob Clark <[email protected]>

Please, put this patch together with
[PATCH] vsprintf.c: add GUID printing
https://patchwork.ozlabs.org/patch/798362/
and
[PATCH v0 06/20] common: add some utf16 handling helpers
https://patchwork.ozlabs.org/patch/797968/
into a separate patch series.

Quoted 2 lines

These three patches can be reviewed independently of the efi_loader patches and probably will not be integrated via the efi-next tree.

I'll resend these as a separate patchset, and just not in next revision of efi_loader patchset that it is a dependency

Quoted 20 lines

Quoted 20 lines

---
lib/vsprintf.c | 30 ++++++++++++++++++++++++++++--

1 file changed, 28 insertions(+), 2 deletions(-)

lib/vsprintf.c +1 -0

index 874a2951f7..0c40f852ce 100644
@@ -17,6 +17,7 @@
 #include <linux/ctype.h>
 
 #include <common.h>
+#include <charset.h>
 
 #include <div64.h>
 #define noinline __attribute__((noinline))

@@ -270,6 +271,26 @@ static char *string(char *buf, char *end, char *s, int field_width,
     return buf;
 }
 

Quoted 7 lines

lib/vsprintf.c static char *string(char *buf, char *end, char *s, int field_width, · line 277

Quoted 4 lines

lib/vsprintf.c +4 -0

+static char *string16(char *buf, char *end, u16 *s, int field_width,
+             int precision, int flags)
+{
+     u16 *str = s ? s : L"<NULL>";

Please, do not use the L-notation here as it requires -fshort-wchar. As we currently cannot switch the complete project to C11 you cannot use the u-notation either.

current plan was to either switch whole project to -fshort-wchar or c11 and rework these patches (as well as a few patches in the efi_loader patchset). (In the c11 case, I'm not sure what we'll use as the fmt string, since afaict that isn't specified. We could use %S although that seems to be a deprecated way to do %ls, or something different like %A, I guess)..

Quoted 4 lines

how far are we from c11? If there is stuff I can do to help let me know. If feasible, I'd rather do that first rather than have a bunch of stuff in vsprintf and elsewhere that needs to be cleaned up later after the switch.

buildman downloads very old compilers (gcc < 4.8) from kernel.org which do not support C11.
Travis CI uses Ubuntu 14.04 with gcc 4.8.4 which incorrectly throws an error for disk/part.c in C11 mode.

To get things right we would have to
* build our own cross tool chains based on a current gcc version
* use our own tool chain in Travis for x86-64 or use a docker container with a current gcc version.

In the long run heading for C11 would be the right thing to do. Until then use an initializer { '<', 'N', 'U', 'L', 'L', '>' }.

It looks ugly but does not consume more bytes once compiled.

Regards

Heinrich

Quoted 73 lines

Quoted 7 lines

lib/vsprintf.c static char *string(char *buf, char *end, char *s, int field_width, · line 279

Quoted 4 lines

lib/vsprintf.c +2 -0

+     int len = utf16_strnlen(str, precision);
+     u8 utf8[len * MAX_UTF8_PER_UTF16];

Didn't you forget 1 byte for \0 here?

Quoted 8 lines

This is what strlnlen does:

The strnlen() function returns the number of characters in the string pointed to by s, **excluding** the terminating null byte ('\0'), but at most maxlen.

I would expect the exclusion of the terminating null word by an utf16_strnlen function.

you are right, but fixing the wrong problem.. the code is definitely wrong since length of a utf16 string != length of a utf8 string, and we don't need to append a null terminator.. so my logic below using 'len' is wrong. I'll fix that in the next version.

Quoted 20 lines

Quoted 20 lines

lib/vsprintf.c +14 -0

+     int i;
+
+     *utf16_to_utf8(utf8, str, len) = '\0';
+
+     if (!(flags & LEFT))
+             while (len < field_width--)
+                     ADDCH(buf, ' ');
+     for (i = 0; i < len; ++i)
+             ADDCH(buf, utf8[i]);
+     while (len < field_width--)
+             ADDCH(buf, ' ');
+     return buf;
+}
+
 #ifdef CONFIG_CMD_NET
 static const char hex_asc[] = "0123456789abcdef";
 #define hex_asc_lo(x)        hex_asc[((x) & 0x0f)]

@@ -528,8 +549,13 @@ repeat:
                     continue;
 

Quoted 9 lines

lib/vsprintf.c repeat: · line 552

Quoted 4 lines

lib/vsprintf.c +1 -2

             case 's':
-                     str = string(str, end, va_arg(args, char *),
-                                  field_width, precision, flags);
+                     if (qualifier == 'l') {

%ls refers to wchar with implementation dependent width in the C standard. There is no qualifier for 16-bit wchar. Couldn't we use %us here in reference to the u-notation ( u'MyString' ). This would leave the path open for a standard compliant '%ls'.

hmm, yeah, that is a clever idea, and I like it better than %A or %S.. so if we go the c11 route I'll do that. The c11 committee should have thought of that ;-)

BR,
-R

Quoted 14 lines

Best regards

Heinrich

Quoted 10 lines

lib/vsprintf.c +6 -0

+                             str = string16(str, end, va_arg(args, u16 *),
+                                            field_width, precision, flags);
+                     } else {
+                             str = string(str, end, va_arg(args, char *),
+                                          field_width, precision, flags);
+                     }
                     continue;
 
             case 'p':

Quoted 55 lines · Rob Clark

Quoted 13 lines · Heinrich Schuchardt

Quoted 3 lines · Rob Clark

This is convenient for efi_loader which deals a lot with utf16.

Signed-off-by Rob Clark <[email protected]>

Please, put this patch together with
[PATCH] vsprintf.c: add GUID printing
https://patchwork.ozlabs.org/patch/798362/
and
[PATCH v0 06/20] common: add some utf16 handling helpers
https://patchwork.ozlabs.org/patch/797968/
into a separate patch series.

Quoted 2 lines

These three patches can be reviewed independently of the efi_loader patches and probably will not be integrated via the efi-next tree.

I'll resend these as a separate patchset, and just not in next revision of efi_loader patchset that it is a dependency

Quoted 20 lines

Quoted 20 lines

---
 lib/vsprintf.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

lib/vsprintf.c +1 -0

index 874a2951f7..0c40f852ce 100644
@@ -17,6 +17,7 @@
 #include <linux/ctype.h>
 
 #include <common.h>
+#include <charset.h>
 
 #include <div64.h>
 #define noinline __attribute__((noinline))

@@ -270,6 +271,26 @@ static char *string(char *buf, char *end, char *s, int field_width,
      return buf;
 }
 

Quoted 7 lines

lib/vsprintf.c static char *string(char *buf, char *end, char *s, int field_width, · line 277

Quoted 4 lines

lib/vsprintf.c +4 -0

+static char *string16(char *buf, char *end, u16 *s, int field_width,
+             int precision, int flags)
+{
+     u16 *str = s ? s : L"<NULL>";

Please, do not use the L-notation here as it requires -fshort-wchar. As we currently cannot switch the complete project to C11 you cannot use the u-notation either.

current plan was to either switch whole project to -fshort-wchar or c11 and rework these patches (as well as a few patches in the efi_loader patchset). (In the c11 case, I'm not sure what we'll use as the fmt string, since afaict that isn't specified. We could use %S although that seems to be a deprecated way to do %ls, or something different like %A, I guess)..

Quoted 4 lines

how far are we from c11? If there is stuff I can do to help let me know. If feasible, I'd rather do that first rather than have a bunch of stuff in vsprintf and elsewhere that needs to be cleaned up later after the switch.

buildman downloads very old compilers (gcc < 4.8) from kernel.org which do not support C11.
Travis CI uses Ubuntu 14.04 with gcc 4.8.4 which incorrectly throws an error for disk/part.c in C11 mode.

To get things right we would have to
* build our own cross tool chains based on a current gcc version
* use our own tool chain in Travis for x86-64 or use a docker container with a current gcc version.

In the long run heading for C11 would be the right thing to do. Until then use an initializer { '<', 'N', 'U', 'L', 'L', '>' }. It looks ugly but does not consume more bytes once compiled.

Quoted 61 lines · Heinrich Schuchardt

Quoted 55 lines · Rob Clark

Quoted 13 lines · Heinrich Schuchardt

Quoted 3 lines · Rob Clark

This is convenient for efi_loader which deals a lot with utf16.

Signed-off-by Rob Clark <[email protected]>

Please, put this patch together with
[PATCH] vsprintf.c: add GUID printing
https://patchwork.ozlabs.org/patch/798362/
and
[PATCH v0 06/20] common: add some utf16 handling helpers
https://patchwork.ozlabs.org/patch/797968/
into a separate patch series.

Quoted 2 lines

These three patches can be reviewed independently of the efi_loader patches and probably will not be integrated via the efi-next tree.

I'll resend these as a separate patchset, and just not in next revision of efi_loader patchset that it is a dependency

Quoted 20 lines

Quoted 20 lines

---
  lib/vsprintf.c | 30 ++++++++++++++++++++++++++++--
  1 file changed, 28 insertions(+), 2 deletions(-)

lib/vsprintf.c +1 -0

index 874a2951f7..0c40f852ce 100644
@@ -17,6 +17,7 @@
  #include <linux/ctype.h>
 
  #include <common.h>
+#include <charset.h>
 
  #include <div64.h>
  #define noinline __attribute__((noinline))

@@ -270,6 +271,26 @@ static char *string(char *buf, char *end, char *s, int field_width,
       return buf;
  }
 

Quoted 7 lines

lib/vsprintf.c static char *string(char *buf, char *end, char *s, int field_width, · line 277

Quoted 4 lines

lib/vsprintf.c +4 -0

+static char *string16(char *buf, char *end, u16 *s, int field_width,
+             int precision, int flags)
+{
+     u16 *str = s ? s : L"<NULL>";

Please, do not use the L-notation here as it requires -fshort-wchar. As we currently cannot switch the complete project to C11 you cannot use the u-notation either.

current plan was to either switch whole project to -fshort-wchar or c11 and rework these patches (as well as a few patches in the efi_loader patchset). (In the c11 case, I'm not sure what we'll use as the fmt string, since afaict that isn't specified. We could use %S although that seems to be a deprecated way to do %ls, or something different like %A, I guess)..

Quoted 4 lines

how far are we from c11? If there is stuff I can do to help let me know. If feasible, I'd rather do that first rather than have a bunch of stuff in vsprintf and elsewhere that needs to be cleaned up later after the switch.

Quoted 4 lines

buildman downloads very old compilers (gcc < 4.8) from kernel.org which do not support C11.
Travis CI uses Ubuntu 14.04 with gcc 4.8.4 which incorrectly throws an error for disk/part.c in C11 mode.

ugg, 4.8 is pretty old.. Not sure how much older than 4.8 buildman
uses. It seems like *some* c11 was supported w/ >=4.6 so if we approach the conversion piecemeal (for example skipping code that triggers gcc bugs on old compilers) we might be able to keep 4.8.4 working until travis provides something newer.

(btw, even going back say 8 fedora releases or more, I've used distro packaged arm and aarch64 toolchains exclusively.. are there that many distro's where we really can't assume availability of an cross-toolchain? If there isn't something newer from kernel.org can we just drop relying on ancient prebuilt toolchains? I'm anyways not hugely a fan of downloading binary executables from even kernel.org, instead of using something from a distro build system which I at least know is very locked down.)

Quoted 9 lines

To get things right we would have to
* build our own cross tool chains based on a current gcc version
* use our own tool chain in Travis for x86-64 or use a docker container with a current gcc version.

In the long run heading for C11 would be the right thing to do. Until then use an initializer { '<', 'N', 'U', 'L', 'L', '>' }. It looks ugly but does not consume more bytes once compiled.

Sure, that I'm less worried about, as much as adding stuff that is very soon going to be legacy. Even in vfprintf.c it isn't such a big deal, as efi_loader where it would be more cumbersome.

Maybe we can write out u"<NULL>" longhand in vsprintf.c as you suggest, but restrict efi_loader to gcc >= 4.9? That seems like it shouldn't be a problem for any arm/arm64 device and it shouldn't be a problem for any device that is likely to have an efi payload to load in the first place..

Quoted 111 lines · Rob Clark

Quoted 73 lines · Heinrich Schuchardt

Quoted 65 lines · Rob Clark

Quoted 16 lines · Heinrich Schuchardt

Quoted 4 lines · Rob Clark

This is convenient for efi_loader which deals a lot with utf16.

Signed-off-by Rob Clark <[email protected]>

Please, put this patch together with
[PATCH] vsprintf.c: add GUID printing
https://patchwork.ozlabs.org/patch/798362/
and
[PATCH v0 06/20] common: add some utf16 handling helpers
https://patchwork.ozlabs.org/patch/797968/
into a separate patch series.

Quoted 2 lines

These three patches can be reviewed independently of the efi_loader patches and probably will not be integrated via the efi-next tree.

I'll resend these as a separate patchset, and just not in next revision of efi_loader patchset that it is a dependency

Quoted 21 lines

Quoted 21 lines

---
  lib/vsprintf.c | 30 ++++++++++++++++++++++++++++--
  1 file changed, 28 insertions(+), 2 deletions(-)

lib/vsprintf.c +1 -0

index 874a2951f7..0c40f852ce 100644
@@ -17,6 +17,7 @@
  #include <linux/ctype.h>
 
  #include <common.h>
+#include <charset.h>
 
  #include <div64.h>
  #define noinline __attribute__((noinline))

@@ -270,6 +271,26 @@ static char *string(char *buf, char *end, char
 *s, int field_width,
       return buf;
  }
 

Quoted 9 lines

lib/vsprintf.c static char *string(char *buf, char *end, char *s, int field_width, · line 277

Quoted 4 lines

lib/vsprintf.c +4 -0

+static char *string16(char *buf, char *end, u16 *s, int field_width,
+             int precision, int flags)
+{
+     u16 *str = s ? s : L"<NULL>";

Please, do not use the L-notation here as it requires -fshort-wchar. As we currently cannot switch the complete project to C11 you cannot use
the u-notation either.

current plan was to either switch whole project to -fshort-wchar or c11 and rework these patches (as well as a few patches in the efi_loader patchset). (In the c11 case, I'm not sure what we'll use as the fmt string, since afaict that isn't specified. We could use %S although that seems to be a deprecated way to do %ls, or something different like %A, I guess)..

Quoted 4 lines

how far are we from c11? If there is stuff I can do to help let me know. If feasible, I'd rather do that first rather than have a bunch of stuff in vsprintf and elsewhere that needs to be cleaned up later after the switch.

Quoted 4 lines

buildman downloads very old compilers (gcc < 4.8) from kernel.org which do not support C11.
Travis CI uses Ubuntu 14.04 with gcc 4.8.4 which incorrectly throws an error for disk/part.c in C11 mode.

ugg, 4.8 is pretty old.. Not sure how much older than 4.8 buildman
uses. It seems like *some* c11 was supported w/ >=4.6 so if we approach the conversion piecemeal (for example skipping code that triggers gcc bugs on old compilers) we might be able to keep 4.8.4 working until travis provides something newer.

(btw, even going back say 8 fedora releases or more, I've used distro packaged arm and aarch64 toolchains exclusively.. are there that many distro's where we really can't assume availability of an cross-toolchain? If there isn't something newer from kernel.org can we just drop relying on ancient prebuilt toolchains? I'm anyways not hugely a fan of downloading binary executables from even kernel.org, instead of using something from a distro build system which I at least know is very locked down.)

Quoted 9 lines

To get things right we would have to
* build our own cross tool chains based on a current gcc version
* use our own tool chain in Travis for x86-64 or use a docker container with a current gcc version.

In the long run heading for C11 would be the right thing to do. Until then use an initializer { '<', 'N', 'U', 'L', 'L', '>' }. It looks ugly but does not consume more bytes once compiled.

Sure, that I'm less worried about, as much as adding stuff that is very soon going to be legacy. Even in vfprintf.c it isn't such a big deal, as efi_loader where it would be more cumbersome.

Quoted 5 lines

Maybe we can write out u"<NULL>" longhand in vsprintf.c as you suggest, but restrict efi_loader to gcc >= 4.9? That seems like it shouldn't be a problem for any arm/arm64 device and it shouldn't be a problem for any device that is likely to have an efi payload to load in the first place..

I don't understand? We enable EFI_LOADER on all arm/arm64 systems for a good reason, so they all get checked by travis. If we break travis, that won't do anyone any good.

I do remember however that Tom wanted to set certain compiler versions as minimum required versions. Tom, do you remember which one that was?

Alex

Quoted 121 lines · Alexander Graf

Quoted 111 lines · Rob Clark

Quoted 73 lines · Heinrich Schuchardt

Quoted 65 lines · Rob Clark

Quoted 16 lines · Heinrich Schuchardt

Quoted 4 lines · Rob Clark

This is convenient for efi_loader which deals a lot with utf16.

Signed-off-by Rob Clark <[email protected]>

Please, put this patch together with
[PATCH] vsprintf.c: add GUID printing
https://patchwork.ozlabs.org/patch/798362/
and
[PATCH v0 06/20] common: add some utf16 handling helpers
https://patchwork.ozlabs.org/patch/797968/
into a separate patch series.

Quoted 2 lines

These three patches can be reviewed independently of the efi_loader patches and probably will not be integrated via the efi-next tree.

I'll resend these as a separate patchset, and just not in next revision of efi_loader patchset that it is a dependency

Quoted 21 lines

Quoted 21 lines

---
  lib/vsprintf.c | 30 ++++++++++++++++++++++++++++--
  1 file changed, 28 insertions(+), 2 deletions(-)

lib/vsprintf.c +1 -0

index 874a2951f7..0c40f852ce 100644
@@ -17,6 +17,7 @@
  #include <linux/ctype.h>
 
  #include <common.h>
+#include <charset.h>
 
  #include <div64.h>
  #define noinline __attribute__((noinline))

@@ -270,6 +271,26 @@ static char *string(char *buf, char *end, char
 *s, int field_width,
       return buf;
  }
 

Quoted 9 lines

lib/vsprintf.c static char *string(char *buf, char *end, char · line 278

Quoted 4 lines

lib/vsprintf.c +4 -0

+static char *string16(char *buf, char *end, u16 *s, int field_width,
+             int precision, int flags)
+{
+     u16 *str = s ? s : L"<NULL>";

Please, do not use the L-notation here as it requires -fshort-wchar. As we currently cannot switch the complete project to C11 you cannot use
the u-notation either.

current plan was to either switch whole project to -fshort-wchar or c11 and rework these patches (as well as a few patches in the efi_loader patchset). (In the c11 case, I'm not sure what we'll use as the fmt string, since afaict that isn't specified. We could use %S although that seems to be a deprecated way to do %ls, or something different like %A, I guess)..

Quoted 4 lines

how far are we from c11? If there is stuff I can do to help let me know. If feasible, I'd rather do that first rather than have a bunch of stuff in vsprintf and elsewhere that needs to be cleaned up later after the switch.

Quoted 4 lines

buildman downloads very old compilers (gcc < 4.8) from kernel.org which do not support C11.
Travis CI uses Ubuntu 14.04 with gcc 4.8.4 which incorrectly throws an error for disk/part.c in C11 mode.

ugg, 4.8 is pretty old.. Not sure how much older than 4.8 buildman
uses. It seems like *some* c11 was supported w/ >=4.6 so if we approach the conversion piecemeal (for example skipping code that triggers gcc bugs on old compilers) we might be able to keep 4.8.4 working until travis provides something newer.

(btw, even going back say 8 fedora releases or more, I've used distro packaged arm and aarch64 toolchains exclusively.. are there that many distro's where we really can't assume availability of an cross-toolchain? If there isn't something newer from kernel.org can we just drop relying on ancient prebuilt toolchains? I'm anyways not hugely a fan of downloading binary executables from even kernel.org, instead of using something from a distro build system which I at least know is very locked down.)

Quoted 9 lines

To get things right we would have to
* build our own cross tool chains based on a current gcc version
* use our own tool chain in Travis for x86-64 or use a docker container with a current gcc version.

In the long run heading for C11 would be the right thing to do. Until then use an initializer { '<', 'N', 'U', 'L', 'L', '>' }. It looks ugly but does not consume more bytes once compiled.

Sure, that I'm less worried about, as much as adding stuff that is very soon going to be legacy. Even in vfprintf.c it isn't such a big deal, as efi_loader where it would be more cumbersome.

Quoted 5 lines

Maybe we can write out u"<NULL>" longhand in vsprintf.c as you suggest, but restrict efi_loader to gcc >= 4.9? That seems like it shouldn't be a problem for any arm/arm64 device and it shouldn't be a problem for any device that is likely to have an efi payload to load in the first place..

Quoted 3 lines

I don't understand? We enable EFI_LOADER on all arm/arm64 systems for a good reason, so they all get checked by travis. If we break travis, that won't do anyone any good.

I was more thinking if there was some oddball non-arm arch that u-boot supported which didn't haven good mainline gcc support and required something ancient ;-)

For arm/arm64, it seems like we could somehow come up w/ a solution using a new enough toolchain, given that arm support in gcc has been good for a long time.. not like the old days where we had to use some codesourcery build (or figure out how to compile the cs src code drop ourselves). A toolchain >= 4.9 for arm/arm64 shouldn't be hard to come by.

Quoted 121 lines · Alexander Graf

Quoted 116 lines · Rob Clark

Quoted 73 lines · Heinrich Schuchardt

Quoted 65 lines · Rob Clark

Quoted 16 lines · Heinrich Schuchardt

Quoted 4 lines · Rob Clark

This is convenient for efi_loader which deals a lot with utf16.

Signed-off-by Rob Clark <[email protected]>

Please, put this patch together with
[PATCH] vsprintf.c: add GUID printing
https://patchwork.ozlabs.org/patch/798362/
and
[PATCH v0 06/20] common: add some utf16 handling helpers
https://patchwork.ozlabs.org/patch/797968/
into a separate patch series.

Quoted 2 lines

These three patches can be reviewed independently of the efi_loader patches and probably will not be integrated via the efi-next tree.

I'll resend these as a separate patchset, and just not in next revision of efi_loader patchset that it is a dependency

Quoted 21 lines

Quoted 21 lines

---
  lib/vsprintf.c | 30 ++++++++++++++++++++++++++++--
  1 file changed, 28 insertions(+), 2 deletions(-)

lib/vsprintf.c +1 -0

index 874a2951f7..0c40f852ce 100644
@@ -17,6 +17,7 @@
  #include <linux/ctype.h>
 
  #include <common.h>
+#include <charset.h>
 
  #include <div64.h>
  #define noinline __attribute__((noinline))

@@ -270,6 +271,26 @@ static char *string(char *buf, char *end, char
 *s, int field_width,
       return buf;
  }
 

Quoted 9 lines

lib/vsprintf.c static char *string(char *buf, char *end, char · line 278

Quoted 4 lines

lib/vsprintf.c +4 -0

+static char *string16(char *buf, char *end, u16 *s, int field_width,
+             int precision, int flags)
+{
+     u16 *str = s ? s : L"<NULL>";

Please, do not use the L-notation here as it requires -fshort-wchar. As we currently cannot switch the complete project to C11 you cannot use
the u-notation either.

current plan was to either switch whole project to -fshort-wchar or c11 and rework these patches (as well as a few patches in the efi_loader patchset). (In the c11 case, I'm not sure what we'll use as the fmt string, since afaict that isn't specified. We could use %S although that seems to be a deprecated way to do %ls, or something different like %A, I guess)..

Quoted 4 lines

how far are we from c11? If there is stuff I can do to help let me know. If feasible, I'd rather do that first rather than have a bunch of stuff in vsprintf and elsewhere that needs to be cleaned up later after the switch.

Quoted 4 lines

buildman downloads very old compilers (gcc < 4.8) from kernel.org which do not support C11.
Travis CI uses Ubuntu 14.04 with gcc 4.8.4 which incorrectly throws an error for disk/part.c in C11 mode.

ugg, 4.8 is pretty old.. Not sure how much older than 4.8 buildman
uses. It seems like *some* c11 was supported w/ >=4.6 so if we approach the conversion piecemeal (for example skipping code that triggers gcc bugs on old compilers) we might be able to keep 4.8.4 working until travis provides something newer.

(btw, even going back say 8 fedora releases or more, I've used distro packaged arm and aarch64 toolchains exclusively.. are there that many distro's where we really can't assume availability of an cross-toolchain? If there isn't something newer from kernel.org can we just drop relying on ancient prebuilt toolchains? I'm anyways not hugely a fan of downloading binary executables from even kernel.org, instead of using something from a distro build system which I at least know is very locked down.)

Quoted 9 lines

To get things right we would have to
* build our own cross tool chains based on a current gcc version
* use our own tool chain in Travis for x86-64 or use a docker container with a current gcc version.

In the long run heading for C11 would be the right thing to do. Until then use an initializer { '<', 'N', 'U', 'L', 'L', '>' }. It looks ugly but does not consume more bytes once compiled.

Sure, that I'm less worried about, as much as adding stuff that is very soon going to be legacy. Even in vfprintf.c it isn't such a big deal, as efi_loader where it would be more cumbersome.

Maybe we can write out u"<NULL>" longhand in vsprintf.c as you suggest, but restrict efi_loader to gcc >= 4.9? That seems like it shouldn't be a problem for any arm/arm64 device and it shouldn't be a problem for any device that is likely to have an efi payload to load in the first place..

Quoted 3 lines

I don't understand? We enable EFI_LOADER on all arm/arm64 systems for a good reason, so they all get checked by travis. If we break travis, that won't do anyone any good.

I was more thinking if there was some oddball non-arm arch that u-boot supported which didn't haven good mainline gcc support and required something ancient ;-)

Quoted 120 lines · Alexander Graf

Quoted 115 lines · Rob Clark

Quoted 71 lines · Heinrich Schuchardt

Quoted 63 lines · Rob Clark

Quoted 15 lines · Heinrich Schuchardt

Quoted 4 lines · Rob Clark

This is convenient for efi_loader which deals a lot with utf16.

Signed-off-by Rob Clark <[email protected]>

Please, put this patch together with [PATCH] vsprintf.c: add GUID printing https://patchwork.ozlabs.org/patch/798362/
and
[PATCH v0 06/20] common: add some utf16 handling helpers
https://patchwork.ozlabs.org/patch/797968/
into a separate patch series.

Quoted 2 lines

These three patches can be reviewed independently of the efi_loader patches and probably will not be integrated via the efi-next tree.

I'll resend these as a separate patchset, and just not in next revision of efi_loader patchset that it is a dependency

Quoted 21 lines

Quoted 21 lines

---
  lib/vsprintf.c | 30 ++++++++++++++++++++++++++++--
  1 file changed, 28 insertions(+), 2 deletions(-)

lib/vsprintf.c index renamed from lib/vsprintf.c +0 -0

874a2951f7..0c40f852ce 100644

lib/vsprintf.c +1 -0

@@ -17,6 +17,7 @@
  #include <linux/ctype.h>
 
  #include <common.h>
+#include <charset.h>
 
  #include <div64.h>
  #define noinline __attribute__((noinline)) @@ -270,6 +271,26

@@ static char *string(char *buf, char *end, char *s, int field_width,

       return buf;

}

Quoted 7 lines

lib/vsprintf.c

Quoted 3 lines

lib/vsprintf.c +3 -0

+static char *string16(char *buf, char *end, u16 *s, int field_width,
+             int precision, int flags) {
+     u16 *str = s ? s : L"<NULL>";

Please, do not use the L-notation here as it requires -fshort-wchar. As we currently cannot switch the complete project to C11 you cannot use the u-notation either.

current plan was to either switch whole project to -fshort-wchar or c11 and rework these patches (as well as a few patches in the efi_loader patchset). (In the c11 case, I'm not sure what we'll use as the fmt string, since afaict that isn't specified. We could use %S although that seems to be a deprecated way to do %ls, or something different like %A, I guess)..

Quoted 4 lines

how far are we from c11? If there is stuff I can do to help let me know. If feasible, I'd rather do that first rather than have a bunch of stuff in vsprintf and elsewhere that needs to be cleaned up later after the switch.

Quoted 4 lines

buildman downloads very old compilers (gcc < 4.8) from kernel.org which do not support C11.
Travis CI uses Ubuntu 14.04 with gcc 4.8.4 which incorrectly throws an error for disk/part.c in C11 mode.

ugg, 4.8 is pretty old.. Not sure how much older than 4.8 buildman
uses. It seems like *some* c11 was supported w/ >=4.6 so if we approach the conversion piecemeal (for example skipping code that triggers gcc bugs on old compilers) we might be able to keep 4.8.4 working until travis provides something newer.

(btw, even going back say 8 fedora releases or more, I've used distro packaged arm and aarch64 toolchains exclusively.. are there that many distro's where we really can't assume availability of an cross-toolchain? If there isn't something newer from kernel.org can we just drop relying on ancient prebuilt toolchains? I'm anyways not hugely a fan of downloading binary executables from even kernel.org, instead of using something from a distro build system which I at least know is very locked down.)

Quoted 9 lines

To get things right we would have to
* build our own cross tool chains based on a current gcc version
* use our own tool chain in Travis for x86-64 or use a docker container with a current gcc version.

In the long run heading for C11 would be the right thing to do. Until then use an initializer { '<', 'N', 'U', 'L', 'L', '>' }. It looks ugly but does not consume more bytes once compiled.

Sure, that I'm less worried about, as much as adding stuff that is very soon going to be legacy. Even in vfprintf.c it isn't such a big deal, as efi_loader where it would be more cumbersome.

Maybe we can write out u"<NULL>" longhand in vsprintf.c as you suggest, but restrict efi_loader to gcc >= 4.9? That seems like it shouldn't be a problem for any arm/arm64 device and it shouldn't be a problem for any device that is likely to have an efi payload to load in the first place..

Quoted 3 lines

I don't understand? We enable EFI_LOADER on all arm/arm64 systems for a good reason, so they all get checked by travis. If we break travis, that won't do anyone any good.

I was more thinking if there was some oddball non-arm arch that u-boot supported which didn't haven good mainline gcc support and required something ancient ;-)

Quoted 4 lines

buildman downloads very old compilers (gcc < 4.8) from kernel.org which do not support C11.
Travis CI uses Ubuntu 14.04 with gcc 4.8.4 which incorrectly throws an error for disk/part.c in C11 mode.

ugg, 4.8 is pretty old.. Not sure how much older than 4.8 buildman
uses. It seems like *some* c11 was supported w/ >=4.6 so if we approach the conversion piecemeal (for example skipping code that triggers gcc bugs on old compilers) we might be able to keep 4.8.4 working until travis provides something newer.

(btw, even going back say 8 fedora releases or more, I've used distro packaged arm and aarch64 toolchains exclusively.. are there that many distro's where we really can't assume availability of an cross-toolchain? If there isn't something newer from kernel.org can we just drop relying on ancient prebuilt toolchains? I'm anyways not hugely a fan of downloading binary executables from even kernel.org, instead of using something from a distro build system which I at least know is very locked down.)

Quoted 9 lines

To get things right we would have to
* build our own cross tool chains based on a current gcc version
* use our own tool chain in Travis for x86-64 or use a docker container with a current gcc version.

In the long run heading for C11 would be the right thing to do. Until then use an initializer { '<', 'N', 'U', 'L', 'L', '>' }. It looks ugly but does not consume more bytes once compiled.

Sure, that I'm less worried about, as much as adding stuff that is very soon going to be legacy. Even in vfprintf.c it isn't such a big deal, as efi_loader where it would be more cumbersome.

Maybe we can write out u"<NULL>" longhand in vsprintf.c as you suggest, but restrict efi_loader to gcc >= 4.9? That seems like it shouldn't be a problem for any arm/arm64 device and it shouldn't be a problem for any device that is likely to have an efi payload to load in the first place..

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

Quoted 3 lines · Rob Clark

This is convenient for efi_loader which deals a lot with utf16.

Signed-off-by Rob Clark <[email protected]>

Please, put this patch together with
[PATCH] vsprintf.c: add GUID printing
https://patchwork.ozlabs.org/patch/798362/
and
[PATCH v0 06/20] common: add some utf16 handling helpers
https://patchwork.ozlabs.org/patch/797968/
into a separate patch series.

These three patches can be reviewed independently of the efi_loader patches and probably will not be integrated via the efi-next tree.

Quoted 20 lines

---
 lib/vsprintf.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

lib/vsprintf.c +1 -0

index 874a2951f7..0c40f852ce 100644
@@ -17,6 +17,7 @@
 #include <linux/ctype.h>
 
 #include <common.h>
+#include <charset.h>
 
 #include <div64.h>
 #define noinline __attribute__((noinline))

@@ -270,6 +271,26 @@ static char *string(char *buf, char *end, char *s, int field_width,
      return buf;
 }
 

lib/vsprintf.c static char *string(char *buf, char *end, char *s, int field_width, · line 277

Quoted 4 lines

lib/vsprintf.c +4 -0

+static char *string16(char *buf, char *end, u16 *s, int field_width,
+             int precision, int flags)
+{
+     u16 *str = s ? s : L"<NULL>";

Please, do not use the L-notation here as it requires -fshort-wchar. As we currently cannot switch the complete project to C11 you cannot use the u-notation either.

Quoted 4 lines · Rob Clark

(quoted diff) +0 -0

@@ -528,8 +549,13 @@ repeat:
                      continue;
 

lib/vsprintf.c repeat: · line 552

Quoted 4 lines

lib/vsprintf.c +1 -2

              case 's':
-                     str = string(str, end, va_arg(args, char *),
-                                  field_width, precision, flags);
+                     if (qualifier == 'l') {

%ls refers to wchar with implementation dependent width in the C standard. There is no qualifier for 16-bit wchar. Couldn't we use %us here in reference to the u-notation ( u'MyString' ). This would leave the path open for a standard compliant '%ls'.

Am 09.08.2017 um 14:38 schrieb Rob Clark <[email protected]>:

Quoted 6 lines · Heinrich Schuchardt

Quoted 5 lines · Rob Clark

(quoted diff) +0 -0

@@ -528,8 +549,13 @@ repeat:
                     continue;
 

Quoted 10 lines

lib/vsprintf.c repeat: · line 552

Quoted 4 lines

lib/vsprintf.c +1 -2

             case 's':
-                     str = string(str, end, va_arg(args, char *),
-                                  field_width, precision, flags);
+                     if (qualifier == 'l') {

%ls refers to wchar with implementation dependent width in the C standard. There is no qualifier for 16-bit wchar. Couldn't we use %us here in reference to the u-notation ( u'MyString' ). This would leave the path open for a standard compliant '%ls'.

So two drawbacks I'm running into when converting to c11 u"string" style, compared to the -fshort-wchar:

1) with -fshort-wchar plus %ls, gcc knows how to typecheck the printf/sprintf/etc args
2) introducing a non-standard conversion character (since there doesn't seem to be a standard one) means we need to drop -Wformat

So far, afaict, the only argument against -fshort-wchar seems to be that someday ext4 might support utf32 filenames? (And really -fshort-wchar doesn't preclude that. So I'm not sure this is a valid argument.)

So independent of c11 (which might be a good idea for other reasons), I'm back to thinking we should use -fshort-wchar. Possibly as a kconfig option that EFI_LOADER selects.. or possibly just unconditionally.

Thoughts?

BR,
-R

Quoted 34 lines

Am 09.08.2017 um 14:38 schrieb Rob Clark <[email protected]>:

Quoted 6 lines · Heinrich Schuchardt

Quoted 5 lines · Rob Clark

(quoted diff) +0 -0

@@ -528,8 +549,13 @@ repeat:
                     continue;
 

Quoted 10 lines

lib/vsprintf.c repeat: · line 552

Quoted 4 lines

lib/vsprintf.c +1 -2

             case 's':
-                     str = string(str, end, va_arg(args, char *),
-                                  field_width, precision, flags);
+                     if (qualifier == 'l') {

%ls refers to wchar with implementation dependent width in the C standard. There is no qualifier for 16-bit wchar. Couldn't we use %us here in reference to the u-notation ( u'MyString' ). This would leave the path open for a standard compliant '%ls'.

So two drawbacks I'm running into when converting to c11 u"string" style, compared to the -fshort-wchar:

1) with -fshort-wchar plus %ls, gcc knows how to typecheck the printf/sprintf/etc args
2) introducing a non-standard conversion character (since there doesn't seem to be a standard one) means we need to drop -Wformat

So far, afaict, the only argument against -fshort-wchar seems to be that someday ext4 might support utf32 filenames? (And really -fshort-wchar doesn't preclude that. So I'm not sure this is a valid argument.)

Quoted 6 lines

So independent of c11 (which might be a good idea for other reasons), I'm back to thinking we should use -fshort-wchar. Possibly as a kconfig option that EFI_LOADER selects.. or possibly just unconditionally.

Thoughts?

If we select it, I'd rather have it be unconditional, to not oprn potential for undetected breakage.

There are a couple places where we'll need GUID -> string. So add a helper.

Signed-off-by Rob Clark <[email protected]>

---
 include/efi_loader.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

include/efi_loader.h +8 -0

index 1028bfb75d..e6c46f713e 100644
@@ -239,6 +239,21 @@ static inline int guidcmp(const efi_guid_t *g1, const efi_guid_t *g2)
 	return memcmp(g1, g2, sizeof(efi_guid_t));
 }
 
+static inline int guidstr(char *s, const efi_guid_t *g)
+{
+	/* unpacked-guid, otherwise we have to have to consider endianess */
+	struct {
+		uint32_t data1;
+		uint16_t data2;
+		uint16_t data3;
+		uint8_t  data4[8];

Quoted 25 lines · Rob Clark

There are a couple places where we'll need GUID -> string. So add a helper.

Signed-off-by Rob Clark <[email protected]>

---
 include/efi_loader.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

include/efi_loader.h +8 -0

index 1028bfb75d..e6c46f713e 100644
@@ -239,6 +239,21 @@ static inline int guidcmp(const efi_guid_t *g1, const efi_guid_t *g2)
      return memcmp(g1, g2, sizeof(efi_guid_t));
 }
 
+static inline int guidstr(char *s, const efi_guid_t *g)
+{
+     /* unpacked-guid, otherwise we have to have to consider endianess */
+     struct {
+             uint32_t data1;
+             uint16_t data2;
+             uint16_t data3;
+             uint8_t  data4[8];

include/efi_loader.h static inline int guidcmp(const efi_guid_t *g1, const efi_guid_t *g2) · line 259

Quoted 10 lines

include/efi_loader.h +7 -0

+     } *ug = (void *)g;
+     return sprintf(s, "%08x-%04x-%04x-%02x%02x-%02x%02x%02x%02x%02x%02x",
+                    ug->data1, ug->data2, ug->data3, ug->data4[0],
+                    ug->data4[1], ug->data4[2], ug->data4[3], ug->data4[4],
+                    ug->data4[5], ug->data4[6], ug->data4[7]);
+}
+
 /*
  * Use these to indicate that your code / data should go into the EFI runtime
  * section and thus still be available when the OS is running

You may want to have a look at these:

include/uuid.h:40
int uuid_guid_get_bin(const char *guid_str, unsigned char *guid_bin);

int uuid_guid_get_str(unsigned char *guid_bin, char *guid_str);

With a conversion to uchar* you should be able to use uuid_guid_get_str for your purposes. So this patch could be eliminated from the series.

Quoted 40 lines · Heinrich Schuchardt

Quoted 25 lines · Rob Clark

There are a couple places where we'll need GUID -> string. So add a helper.

Signed-off-by Rob Clark <[email protected]>

---
 include/efi_loader.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

include/efi_loader.h +8 -0

index 1028bfb75d..e6c46f713e 100644
@@ -239,6 +239,21 @@ static inline int guidcmp(const efi_guid_t *g1, const efi_guid_t *g2)
      return memcmp(g1, g2, sizeof(efi_guid_t));
 }
 
+static inline int guidstr(char *s, const efi_guid_t *g)
+{
+     /* unpacked-guid, otherwise we have to have to consider endianess */
+     struct {
+             uint32_t data1;
+             uint16_t data2;
+             uint16_t data3;
+             uint8_t  data4[8];

include/efi_loader.h static inline int guidcmp(const efi_guid_t *g1, const efi_guid_t *g2) · line 259

Quoted 10 lines

include/efi_loader.h +7 -0

+     } *ug = (void *)g;
+     return sprintf(s, "%08x-%04x-%04x-%02x%02x-%02x%02x%02x%02x%02x%02x",
+                    ug->data1, ug->data2, ug->data3, ug->data4[0],
+                    ug->data4[1], ug->data4[2], ug->data4[3], ug->data4[4],
+                    ug->data4[5], ug->data4[6], ug->data4[7]);
+}
+
 /*
  * Use these to indicate that your code / data should go into the EFI runtime
  * section and thus still be available when the OS is running

You may want to have a look at these:

Quoted 8 lines

include/uuid.h:40
int uuid_guid_get_bin(const char *guid_str, unsigned char *guid_bin);

int uuid_guid_get_str(unsigned char *guid_bin, char *guid_str);

With a conversion to uchar* you should be able to use uuid_guid_get_str for your purposes. So this patch could be eliminated from the series.

hmm, wow, those seem a bit over-engineered. But I think we should add this to vsprintf.c and drop both this patch plus your patch that adds a macro to print GUIDs. (And vsprintf.c should re-use uuid_guid_get_str().. and I could re-use uuid_guid_get_bin() when I get around to implementing efi_get_next_variable(), so thanks for pointing that out.)

BR,
-R

From: Rob Clark <[email protected]>
Date: Fri, 4 Aug 2017 15:31:55 -0400

Also, create disk objects for the disk itself, in addition to the partitions. (UEFI terminology is a bit confusing, a "disk" object is really a partition.) This helps grub properly identify the boot device since it is trying to match up partition "disk" object with it's parent device.

Now instead of seeing devices like:

/File(sdhci at 07864000.blk)/EndEntire
/File(usb_mass_storage.lun0)/EndEntire

You see:

/ACPI(133741d0,0)/UnknownMessaging(1d)/EndEntire /ACPI(133741d0,0)/UnknownMessaging(1d)/HD(0,800,64000,dd904a8c00000000,1,1)/EndEntire /ACPI(133741d0,0)/UnknownMessaging(1d)/HD(1,64800,200000,dd904a8c00000000,1,1)/EndEntire /ACPI(133741d0,0)/UnknownMessaging(1d)/HD(2,264800,19a000,dd904a8c00000000,1,1)/EndEntire /ACPI(133741d0,0)/USB(0,0)/USB(0,0)/USB(0,0)/EndEntire /ACPI(133741d0,0)/USB(0,0)/USB(0,0)/USB(0,0)/HD(0,800,60000,38ca680200000000,1,1)/EndEntire /ACPI(133741d0,0)/USB(0,0)/USB(0,0)/USB(0,0)/HD(1,61000,155000,38ca680200000000,1,1)/EndEntire /ACPI(133741d0,0)/USB(0,0)/USB(0,0)/USB(0,0)/HD(2,20fa800,1bbf8800,38ca680200000000,1,1)/EndEntire /ACPI(133741d0,0)/USB(0,0)/USB(0,0)/USB(0,0)/HD(3,1b6800,1f44000,38ca680200000000,1,1)/EndEntire

This is on a board with single USB disk and single sd-card. The UnknownMessaging(1d) node in the device-path is the MMC device, but grub_efi_print_device_path() hasn't been updated yet for some of the newer device-path sub-types.

Quoted 2 lines

From: Rob Clark <[email protected]>
Date: Fri, 4 Aug 2017 15:31:55 -0400

Hi Rob,

OpenBSD has been an early adopter of efi_loader and pretty much completely relies on it for booting OpenBSD/armv7 and OpenBSD/arm64. We use our own bootloader which is fairly lightweight. Obviously we'd like to keep it working if this patchset gets adopted. We don't make use of EFI variables and don't really plan to make use of them on our ARM platforms. But obviously we have to deal with device paths...

Quoted 23 lines

Also, create disk objects for the disk itself, in addition to the partitions. (UEFI terminology is a bit confusing, a "disk" object is really a partition.) This helps grub properly identify the boot device since it is trying to match up partition "disk" object with it's parent device.

Now instead of seeing devices like:

/File(sdhci at 07864000.blk)/EndEntire
/File(usb_mass_storage.lun0)/EndEntire

You see:

/ACPI(133741d0,0)/UnknownMessaging(1d)/EndEntire /ACPI(133741d0,0)/UnknownMessaging(1d)/HD(0,800,64000,dd904a8c00000000,1,1)/EndEntire /ACPI(133741d0,0)/UnknownMessaging(1d)/HD(1,64800,200000,dd904a8c00000000,1,1)/EndEntire /ACPI(133741d0,0)/UnknownMessaging(1d)/HD(2,264800,19a000,dd904a8c00000000,1,1)/EndEntire /ACPI(133741d0,0)/USB(0,0)/USB(0,0)/USB(0,0)/EndEntire /ACPI(133741d0,0)/USB(0,0)/USB(0,0)/USB(0,0)/HD(0,800,60000,38ca680200000000,1,1)/EndEntire /ACPI(133741d0,0)/USB(0,0)/USB(0,0)/USB(0,0)/HD(1,61000,155000,38ca680200000000,1,1)/EndEntire /ACPI(133741d0,0)/USB(0,0)/USB(0,0)/USB(0,0)/HD(2,20fa800,1bbf8800,38ca680200000000,1,1)/EndEntire /ACPI(133741d0,0)/USB(0,0)/USB(0,0)/USB(0,0)/HD(3,1b6800,1f44000,38ca680200000000,1,1)/EndEntire

Quoted 4 lines

This is on a board with single USB disk and single sd-card. The UnknownMessaging(1d) node in the device-path is the MMC device, but grub_efi_print_device_path() hasn't been updated yet for some of the newer device-path sub-types.

..and what you're sketching out here should work with recent enough versions of our bootloader.

However, to me having an ACPI Device Path component in there doesn't make much sense on a board without ACPI. It certainly doesn't help mapping a boot path back to an actual hardware device. Wouldn't it be more logical to a Hardware Device Path there instead? In particular a Memory Mapped Device Path would make a lot of sense as the start address would make it fairly easy to do the mapping.

Authentication-Results: xs4all.nl; spf=pass smtp.mailfrom=gmail.com;
dkim=pass header.d=gmail.com; dmarc=pass header.from=gmail.com

DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
        d=gmail.com; s=20161025;
        h=mime-version:in-reply-to:references:from:date:message-id:subject:to
         :cc;
        bh=qkk49+XF6+Jn7RUjFU/ZFP7/ho5kdUEUNpSrCpqy9yw=;
        b=knLDTd1vrl7R3BnRReKrxUD1UxYSIahqh5VTND3PEt1cLHtskGRiDc280ADRTm6ffV
         izImBjr6hq94Qu/Jnbd6kn7+jSDuhDva9FU1/ZndFaNkTUhsatlgtvrK5RzJ38FpwvLa
         0X7Y8NuVm99K+ijWdKI34YruaZfz47t863L40UYJhjdaj3/TLdIWrC/NzEBiQ/fXemHU
         8mN1HM9JBD7STB64mMlDSttSTC2vJOPyX81CCbDnXLI/puqcyXewaJ+kW8Km+VDra2xs
         PIWVzyA0PoV7nIihMbkrv95K7GQgpSOUUL7nlEmAreQoEbCCb8Iw2inSe6LqhN6IbJ4u
         Js9A==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
        d=1e100.net; s=20161025;
        h=x-gm-message-state:mime-version:in-reply-to:references:from:date
         :message-id:subject:to:cc;
        bh=qkk49+XF6+Jn7RUjFU/ZFP7/ho5kdUEUNpSrCpqy9yw=;
        b=kiBq9lLF93IqOIhjRAbghuqi5rcOJ7mf7OvAvgo8T4qg72k9iEBX84R7yIY7KVyDpX
         DxV+JJDPePbOtIN3qxpmIZF6vqT/HJ0w+z8RnsUiUGJxGCxTA9R97e+tcB1Ovh7zRIJe
         JPSP/6/TqRmFgrlIsZdoPZAsypVgTx/AHfdMA/z3l4EOOdSOcd3AOd+sDdGnhCAm2G0+
         QyAXxkxleKeB3t2AvXKiXpJknXzSb9FxM+0ug0gAPlTTT7d2JFEOZ43gzgLCTTsCVDdL
         lWiJepjIeynmIuGKwcZJpFdV6C8nx2RMztWLpbhPMrbbR+eLkQcaJnJJrgv859lzbUrM
         9XhQ==
X-Gm-Message-State: AHYfb5hmb8q9IgSaZxMt3OsQOUjxd0l+fyjrLiox4JEB/7NaaHlf1K9h
	HiWJc8WMqU156stR2hj9qS4oZRxkCuHp0xg=

X-Received: by 10.25.59.29 with SMTP id i29mr1058615lfa.224.1501880271850;
Fri, 04 Aug 2017 13:57:51 -0700 (PDT)
From: Rob Clark <[email protected]>
Date: Fri, 4 Aug 2017 16:57:50 -0400
Cc: U-Boot Mailing List <[email protected]>,
Heinrich Schuchardt <[email protected]>,
Peter Jones <[email protected]>
X-CNFS-Analysis: v=2.2 cv=eoad9chX c=1 sm=0 tr=0

	a=bdpqe3ZLSLbgpdprsY8WZA==:117 a=IkcTkHD0fZMA:10 a=x7bEGLp0ZPQA:10
	a=D2htVsi0J-IA:10 a=KeKAF7QvOSUA:10 a=pGLkceISAAAA:8 a=NEAV23lmAAAA:8
	a=YyEjxAJ5dRpCWmjka9UA:9 a=QEXdDO2ut3YA:10 a=6kGIvZw6iX1k4Y-7sg4_:22

X-Virus-Scanned: by XS4ALL Virus Scanner
X-XS4ALL-Spam-Score: -0.1 () DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU,
FREEMAIL_FROM, SPF_PASS
X-XS4ALL-Spam: NO
Envelope-To: mark.kettenis at xs4all.nl

Quoted 43 lines · Mark Kettenis

Quoted 2 lines

From: Rob Clark <[email protected]>
Date: Fri, 4 Aug 2017 15:31:55 -0400

Hi Rob,

OpenBSD has been an early adopter of efi_loader and pretty much completely relies on it for booting OpenBSD/armv7 and OpenBSD/arm64. We use our own bootloader which is fairly lightweight. Obviously we'd like to keep it working if this patchset gets adopted. We don't make use of EFI variables and don't really plan to make use of them on our ARM platforms. But obviously we have to deal with device paths...

Quoted 23 lines

Also, create disk objects for the disk itself, in addition to the partitions. (UEFI terminology is a bit confusing, a "disk" object is really a partition.) This helps grub properly identify the boot device since it is trying to match up partition "disk" object with it's parent device.

Now instead of seeing devices like:

/File(sdhci at 07864000.blk)/EndEntire
/File(usb_mass_storage.lun0)/EndEntire

You see:

/ACPI(133741d0,0)/UnknownMessaging(1d)/EndEntire /ACPI(133741d0,0)/UnknownMessaging(1d)/HD(0,800,64000,dd904a8c00000000,1,1)/EndEntire /ACPI(133741d0,0)/UnknownMessaging(1d)/HD(1,64800,200000,dd904a8c00000000,1,1)/EndEntire /ACPI(133741d0,0)/UnknownMessaging(1d)/HD(2,264800,19a000,dd904a8c00000000,1,1)/EndEntire /ACPI(133741d0,0)/USB(0,0)/USB(0,0)/USB(0,0)/EndEntire /ACPI(133741d0,0)/USB(0,0)/USB(0,0)/USB(0,0)/HD(0,800,60000,38ca680200000000,1,1)/EndEntire /ACPI(133741d0,0)/USB(0,0)/USB(0,0)/USB(0,0)/HD(1,61000,155000,38ca680200000000,1,1)/EndEntire /ACPI(133741d0,0)/USB(0,0)/USB(0,0)/USB(0,0)/HD(2,20fa800,1bbf8800,38ca680200000000,1,1)/EndEntire /ACPI(133741d0,0)/USB(0,0)/USB(0,0)/USB(0,0)/HD(3,1b6800,1f44000,38ca680200000000,1,1)/EndEntire

Quoted 4 lines

This is on a board with single USB disk and single sd-card. The UnknownMessaging(1d) node in the device-path is the MMC device, but grub_efi_print_device_path() hasn't been updated yet for some of the newer device-path sub-types.

..and what you're sketching out here should work with recent enough versions of our bootloader.

Quoted 7 lines

However, to me having an ACPI Device Path component in there doesn't make much sense on a board without ACPI. It certainly doesn't help mapping a boot path back to an actual hardware device. Wouldn't it be more logical to a Hardware Device Path there instead? In particular a Memory Mapped Device Path would make a lot of sense as the start address would make it fairly easy to do the mapping.

It was pretty much an arbitrary choice, and it wouldn't be hard to change. From my reading of the grub code, all it really cares about is that there is *some* parent of the "partition" HD device. I'm not really sure what maps best in a UEFI world to the "soc" node in a device tree world. I'm certainly open to changing this.

It would be cool if you have a chance to give this a try w/ OpenBSD and let me know if you run into issues. I want this to be something that works across-distro so I'll try to help as much as I can. (Not sure if there is an OpenBSD port for db410c, but I guess there is always qemu..). Fwiw, if git pull/cherry-pick is easier than grabbing patches from list, then [1].

Quoted 1 line

OpenBSD/armv7 BOOTARM 0.8

Quoted 59 lines · Mark Kettenis

Quoted 43 lines

Quoted 2 lines

From: Rob Clark <[email protected]>
Date: Fri, 4 Aug 2017 15:31:55 -0400

Hi Rob,

OpenBSD has been an early adopter of efi_loader and pretty much completely relies on it for booting OpenBSD/armv7 and OpenBSD/arm64. We use our own bootloader which is fairly lightweight. Obviously we'd like to keep it working if this patchset gets adopted. We don't make use of EFI variables and don't really plan to make use of them on our ARM platforms. But obviously we have to deal with device paths...

Quoted 23 lines

Also, create disk objects for the disk itself, in addition to the partitions. (UEFI terminology is a bit confusing, a "disk" object is really a partition.) This helps grub properly identify the boot device since it is trying to match up partition "disk" object with it's parent device.

Now instead of seeing devices like:

/File(sdhci at 07864000.blk)/EndEntire
/File(usb_mass_storage.lun0)/EndEntire

You see:

/ACPI(133741d0,0)/UnknownMessaging(1d)/EndEntire /ACPI(133741d0,0)/UnknownMessaging(1d)/HD(0,800,64000,dd904a8c00000000,1,1)/EndEntire /ACPI(133741d0,0)/UnknownMessaging(1d)/HD(1,64800,200000,dd904a8c00000000,1,1)/EndEntire /ACPI(133741d0,0)/UnknownMessaging(1d)/HD(2,264800,19a000,dd904a8c00000000,1,1)/EndEntire /ACPI(133741d0,0)/USB(0,0)/USB(0,0)/USB(0,0)/EndEntire /ACPI(133741d0,0)/USB(0,0)/USB(0,0)/USB(0,0)/HD(0,800,60000,38ca680200000000,1,1)/EndEntire /ACPI(133741d0,0)/USB(0,0)/USB(0,0)/USB(0,0)/HD(1,61000,155000,38ca680200000000,1,1)/EndEntire /ACPI(133741d0,0)/USB(0,0)/USB(0,0)/USB(0,0)/HD(2,20fa800,1bbf8800,38ca680200000000,1,1)/EndEntire /ACPI(133741d0,0)/USB(0,0)/USB(0,0)/USB(0,0)/HD(3,1b6800,1f44000,38ca680200000000,1,1)/EndEntire

Quoted 4 lines

This is on a board with single USB disk and single sd-card. The UnknownMessaging(1d) node in the device-path is the MMC device, but grub_efi_print_device_path() hasn't been updated yet for some of the newer device-path sub-types.

..and what you're sketching out here should work with recent enough versions of our bootloader.

Quoted 7 lines

However, to me having an ACPI Device Path component in there doesn't make much sense on a board without ACPI. It certainly doesn't help mapping a boot path back to an actual hardware device. Wouldn't it be more logical to a Hardware Device Path there instead? In particular a Memory Mapped Device Path would make a lot of sense as the start address would make it fairly easy to do the mapping.

It was pretty much an arbitrary choice, and it wouldn't be hard to change. From my reading of the grub code, all it really cares about is that there is *some* parent of the "partition" HD device. I'm not really sure what maps best in a UEFI world to the "soc" node in a device tree world. I'm certainly open to changing this.

Quoted 6 lines

It would be cool if you have a chance to give this a try w/ OpenBSD and let me know if you run into issues. I want this to be something that works across-distro so I'll try to help as much as I can. (Not sure if there is an OpenBSD port for db410c, but I guess there is always qemu..). Fwiw, if git pull/cherry-pick is easier than grabbing patches from list, then [1].

OpenBSD doesn't run on the db410c. However, our EFI bootloader should just run. You can download it from:

http://ftp.openbsd.org/pub/OpenBSD/snapshots/arm64/BOOTAA64.EFI

for the 64-bit (AArch64) and

http://ftp.openbsd.org/pub/OpenBSD/snapshots/armv7/BOOTARM.EFI

for the 32-bit version (AArch32).

Unfortunately something in this patch series breaks things for me on a Banana Pi:

U-Boot SPL 2017.09-rc1-00020-g2ad6933c64-dirty (Aug 05 2017 - 15:26:15)
DRAM: 1024 MiB

CPU: 912000000Hz, AXI/AHB/APB: 3/2/2

Trying to boot from MMC1

U-Boot 2017.09-rc1-00020-g2ad6933c64-dirty (Aug 05 2017 - 15:26:15 +0200) Allwinner Technology

CPU:   Allwinner A20 (SUN7I)
Model: LeMaker Banana Pi
I2C:   ready
DRAM:  1 GiB
MMC:   SUNXI SD/MMC: 0

*** Warning - bad CRC, using default environment

Setting up a 720x576i composite-pal console (overscan 32x20)

In:    serial
Out:   vga
Err:   vga
SCSI:  Target spinup took 0 ms.
AHCI 0001.0100 32 slots 1 ports 3 Gbps 0x1 impl SATA mode
flags: ncq stag pm led clo only pmp pio slum part ccc apst
Net:   eth0: ethernet at 01c50000
starting USB...
USB0:   USB EHCI 1.00
USB1:   USB OHCI 1.0
USB2:   USB EHCI 1.00
USB3:   USB OHCI 1.0

scanning bus 0 for devices... 1 USB Device(s) found scanning bus 2 for devices... 1 USB Device(s) found
scanning usb for storage devices... 0 Storage Device(s) found
Hit any key to stop autoboot: 0
switch to partitions #0, OK
mmc0 is current device
Scanning mmc 0:1...
Found EFI removable media binary efi/boot/bootarm.efi Scanning disks on scsi...
Scanning disks on usb...
Scanning disks on mmc...
MMC Device 1 not found
MMC Device 2 not found
MMC Device 3 not found
Found 6 disks
data abort

pc : [<7ef8d878>]          lr : [<7ef8d874>]

reloc pc : [<4a039878>] lr : [<4a039874>]
sp : 7af29660 ip : 00000000 fp : 7af29774
r10: 7efec230 r9 : 7af33ee0 r8 : 00000000
r7 : 00000009 r6 : 7ef9e8b8 r5 : 7af296a0 r4 : 7efa4495
r3 : 7af296a0 r2 : 0000008c r1 : 7af29658 r0 : 00000004
Flags: nzCV IRQs off FIQs off Mode SVC_32
Resetting CPU ...

resetting ...

Normally it would print something like:

Quoted 1 line

Quoted 1 line

OpenBSD/armv7 BOOTARM 0.8

boot>

at that stage.

Date: Sat, 5 Aug 2017 16:01:51 +0200 (CEST)
From: Mark Kettenis <[email protected]>

Unfortunately something in this patch series breaks things for me on a Banana Pi:

Quoted 5 lines

Date: Sat, 5 Aug 2017 16:01:51 +0200 (CEST)
From: Mark Kettenis <[email protected]>

Unfortunately something in this patch series breaks things for me on a Banana Pi:

And according to git bisect:

4e3e748a50fc3f43e20c7ff407184596d7c9a589 is the first bad commit commit 4e3e748a50fc3f43e20c7ff407184596d7c9a589

Author: Peter Jones <[email protected]>
Date:   Wed Jun 21 16:39:02 2017 -0400

efi: add some more device path structures

Signed-off-by Peter Jones <[email protected]>

Signed-off-by Rob Clark <[email protected]>

From: Rob Clark <[email protected]>
Date: Sat, 5 Aug 2017 10:35:08 -0400

Quoted 10 lines · Mark Kettenis

Quoted 5 lines

Date: Sat, 5 Aug 2017 16:01:51 +0200 (CEST)
From: Mark Kettenis <[email protected]>

Unfortunately something in this patch series breaks things for me on a Banana Pi:

And according to git bisect:

4e3e748a50fc3f43e20c7ff407184596d7c9a589 is the first bad commit commit 4e3e748a50fc3f43e20c7ff407184596d7c9a589

Quoted 7 lines

Author: Peter Jones <[email protected]>
Date:   Wed Jun 21 16:39:02 2017 -0400

efi: add some more device path structures

Signed-off-by Peter Jones <[email protected]>

Signed-off-by Rob Clark <[email protected]>

hmm, odd.. it is only adding some #define's and structs that are not used until a later commit..