[PATCH 00/24] efi/libstub: Add printf implementation

[PATCH 00/24] efi/libstub: Add printf implementation

[Arvind Sankar]

This series (on top of efi/next) adds a basic printf implementation for
the EFI stub to use.

Patches 1-2 are minor cleanup.

Patch 3 adds an on-stack buffer for efi_puts to do the char to UTF-16
conversion, to avoid calling into the firmware for each character.

Patches 4-16 copy the simple printf implementation from
arch/x86/boot/printf.c and spruce it up. The main enhancements are
support for 64-bit integers and conversion to vsnprintf. There are small
fixes to correct the behavior for edge cases (detailed in the individual
patches).

Patch 17 changes efi_info/efi_err to actually use efi_printk, and
introduces a loglevel into the EFI stub instead of just quiet/non-quiet.
The loglevels are reused from the main kernel. An efi_debug macro is
also added, but is currently unused. As an aside, we chose efi_info over
efi_pr_info, but I'm wondering whether we should make it efi_pr_info
after all: there is already an efi_info symbol (structure type and field
name) so it might lead to some confusion.

Patch 18-19 add an option to list the available modes from the GOP,
which should make using the new video=efifb parameters easier.

Patches 20-24 incorporate UTF-16 support into the new printf function
and use it in a couple of places.

Arvind Sankar (24):
  efi/libstub: Include dependencies of efistub.h
  efi/libstub: Rename efi_[char16_]printk to efi_[char16_]puts
  efi/libstub: Buffer output of efi_puts
  efi/libstub: Add a basic printf implementation
  efi/libstub: Optimize for size instead of speed
  efi/printf: Drop %n format and L qualifier
  efi/printf: Add 64-bit and 8-bit integer support
  efi/printf: Factor out flags parsing and handle '%' earlier
  efi/printf: Fix minor bug in precision handling
  efi/printf: Merge 'p' with the integer formats
  efi/printf: Factor out width/precision parsing
  efi/printf: Factor out integer argument retrieval
  efi/printf: Handle null string input
  efi/printf: Refactor code to consolidate padding and output
  efi/printf: Abort on invalid format
  efi/printf: Turn vsprintf into vsnprintf
  efi/libstub: Implement printk-style logging
  efi/libstub: Add definitions for console input and events
  efi/gop: Add an option to list out the available GOP modes
  efi/printf: Add support for wchar_t (UTF-16)
  efi/libstub: Add UTF-8 decoding to efi_puts
  efi/libstub: Use %ls for filename
  efi/libstub: Get the exact UTF-8 length
  efi/libstub: Use snprintf with %ls to convert the command line

 Documentation/fb/efifb.rst                    |   5 +
 arch/x86/include/asm/efi.h                    |  10 +
 arch/x86/xen/efi.c                            |   2 +-
 drivers/firmware/efi/libstub/Makefile         |   6 +-
 .../firmware/efi/libstub/efi-stub-helper.c    | 275 ++++++---
 drivers/firmware/efi/libstub/efistub.h        | 107 +++-
 drivers/firmware/efi/libstub/file.c           |   4 +-
 drivers/firmware/efi/libstub/gop.c            |  97 ++-
 drivers/firmware/efi/libstub/vsprintf.c       | 563 ++++++++++++++++++
 include/linux/efi.h                           |   4 +-
 10 files changed, 968 insertions(+), 105 deletions(-)
 create mode 100644 drivers/firmware/efi/libstub/vsprintf.c

-- 
2.26.2

[PATCH 01/24] efi/libstub: Include dependencies of efistub.h

[Arvind Sankar]

Add #include directives for include files that efistub.h depends on,
instead of relying on them having been included by the C source files
prior to efistub.h.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 drivers/firmware/efi/libstub/efistub.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index 15d0b6f3f6c6..998924916b03 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -3,6 +3,11 @@
 #ifndef _DRIVERS_FIRMWARE_EFI_EFISTUB_H
 #define _DRIVERS_FIRMWARE_EFI_EFISTUB_H
 
+#include <linux/efi.h>
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include <asm/efi.h>
+
 /* error code which can't be mistaken for valid address */
 #define EFI_ERROR	(~0UL)
 
-- 
2.26.2

[PATCH 02/24] efi/libstub: Rename efi_[char16_]printk to efi_[char16_]puts

[Arvind Sankar]

These functions do not support formatting, unlike printk. Rename them to
puts to make that clear.

Move the implementations of these two functions next to each other.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 drivers/firmware/efi/libstub/efi-stub-helper.c | 18 +++++++++---------
 drivers/firmware/efi/libstub/efistub.h         | 10 +++++-----
 drivers/firmware/efi/libstub/file.c            |  4 ++--
 3 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index 2927f3d30344..c6d7ef35e9f7 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -26,15 +26,21 @@ bool __pure __efi_soft_reserve_enabled(void)
 	return !efi_nosoftreserve;
 }
 
-void efi_printk(const char *str)
+void efi_char16_puts(efi_char16_t *str)
+{
+	efi_call_proto(efi_table_attr(efi_system_table, con_out),
+		       output_string, str);
+}
+
+void efi_puts(const char *str)
 {
 	while (*str) {
 		efi_char16_t ch[] = { *str++, L'\0' };
 
 		if (ch[0] == L'\n')
-			efi_char16_printk(L"\r\n");
+			efi_char16_puts(L"\r\n");
 		else
-			efi_char16_printk(ch);
+			efi_char16_puts(ch);
 	}
 }
 
@@ -279,12 +285,6 @@ void *get_efi_config_table(efi_guid_t guid)
 	return NULL;
 }
 
-void efi_char16_printk(efi_char16_t *str)
-{
-	efi_call_proto(efi_table_attr(efi_system_table, con_out),
-		       output_string, str);
-}
-
 /*
  * The LINUX_EFI_INITRD_MEDIA_GUID vendor media device path below provides a way
  * for the firmware or bootloader to expose the initrd data directly to the stub
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index 998924916b03..4f57611a65f2 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -49,10 +49,10 @@ extern const efi_system_table_t *efi_system_table;
 #endif
 
 #define efi_info(msg)		do {			\
-	if (!efi_quiet) efi_printk("EFI stub: "msg);	\
+	if (!efi_quiet) efi_puts("EFI stub: "msg);	\
 } while (0)
 
-#define efi_err(msg) efi_printk("EFI stub: ERROR: "msg)
+#define efi_err(msg) efi_puts("EFI stub: ERROR: "msg)
 
 /* Helper macros for the usual case of using simple C variables: */
 #ifndef fdt_setprop_inplace_var
@@ -605,8 +605,6 @@ efi_status_t efi_exit_boot_services(void *handle,
 				    void *priv,
 				    efi_exit_boot_map_processing priv_func);
 
-void efi_char16_printk(efi_char16_t *);
-
 efi_status_t allocate_new_fdt_and_exit_boot(void *handle,
 					    unsigned long *new_fdt_addr,
 					    unsigned long max_addr,
@@ -630,7 +628,9 @@ efi_status_t check_platform_features(void);
 
 void *get_efi_config_table(efi_guid_t guid);
 
-void efi_printk(const char *str);
+/* NOTE: These functions do not print a trailing newline after the string */
+void efi_char16_puts(efi_char16_t *);
+void efi_puts(const char *str);
 
 void efi_free(unsigned long size, unsigned long addr);
 
diff --git a/drivers/firmware/efi/libstub/file.c b/drivers/firmware/efi/libstub/file.c
index cc177152d0df..933536c5236a 100644
--- a/drivers/firmware/efi/libstub/file.c
+++ b/drivers/firmware/efi/libstub/file.c
@@ -47,8 +47,8 @@ static efi_status_t efi_open_file(efi_file_protocol_t *volume,
 	status = volume->open(volume, &fh, fi->filename, EFI_FILE_MODE_READ, 0);
 	if (status != EFI_SUCCESS) {
 		efi_err("Failed to open file: ");
-		efi_char16_printk(fi->filename);
-		efi_printk("\n");
+		efi_char16_puts(fi->filename);
+		efi_puts("\n");
 		return status;
 	}
 
-- 
2.26.2

[PATCH 03/24] efi/libstub: Buffer output of efi_puts

[Arvind Sankar]

Use a buffer to convert the string to UTF-16. This will reduce the
number of firmware calls required to print the string from one per
character to one per string in most cases.

Cast the input char to unsigned char before converting to efi_char16_t
to avoid sign-extension in case there are any non-ASCII characters in
the input.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 .../firmware/efi/libstub/efi-stub-helper.c    | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index c6d7ef35e9f7..3cf506ab9ead 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -8,6 +8,7 @@
  */
 
 #include <linux/efi.h>
+#include <linux/kernel.h>
 #include <asm/efi.h>
 
 #include "efistub.h"
@@ -34,13 +35,19 @@ void efi_char16_puts(efi_char16_t *str)
 
 void efi_puts(const char *str)
 {
-	while (*str) {
-		efi_char16_t ch[] = { *str++, L'\0' };
+	efi_char16_t buf[128];
+	size_t pos = 0, lim = ARRAY_SIZE(buf);
 
-		if (ch[0] == L'\n')
-			efi_char16_puts(L"\r\n");
-		else
-			efi_char16_puts(ch);
+	while (*str) {
+		if (*str == '\n')
+			buf[pos++] = L'\r';
+		/* Cast to unsigned char to avoid sign-extension */
+		buf[pos++] = (unsigned char)(*str++);
+		if (*str == '\0' || pos >= lim - 2) {
+			buf[pos] = L'\0';
+			efi_char16_puts(buf);
+			pos = 0;
+		}
 	}
 }
 
-- 
2.26.2

[PATCH 04/24] efi/libstub: Add a basic printf implementation

[Arvind Sankar]

Copy vsprintf from arch/x86/boot/printf.c to get a simple printf
implementation.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 drivers/firmware/efi/libstub/Makefile         |   2 +-
 .../firmware/efi/libstub/efi-stub-helper.c    |  17 +
 drivers/firmware/efi/libstub/efistub.h        |   3 +
 drivers/firmware/efi/libstub/vsprintf.c       | 298 ++++++++++++++++++
 4 files changed, 319 insertions(+), 1 deletion(-)
 create mode 100644 drivers/firmware/efi/libstub/vsprintf.c

diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
index e5a49dc8e9bc..fb34c9d14a3c 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -44,7 +44,7 @@ KCOV_INSTRUMENT			:= n
 lib-y				:= efi-stub-helper.o gop.o secureboot.o tpm.o \
 				   file.o mem.o random.o randomalloc.o pci.o \
 				   skip_spaces.o lib-cmdline.o lib-ctype.o \
-				   alignedmem.o relocate.o
+				   alignedmem.o relocate.o vsprintf.o
 
 # include the stub's generic dependencies from lib/ when building for ARM/arm64
 efi-deps-y := fdt_rw.c fdt_ro.c fdt_wip.c fdt.c fdt_empty_tree.c fdt_sw.c
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index 3cf506ab9ead..56b3b84fd3bd 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -7,6 +7,8 @@
  * Copyright 2011 Intel Corporation; author Matt Fleming
  */
 
+#include <stdarg.h>
+
 #include <linux/efi.h>
 #include <linux/kernel.h>
 #include <asm/efi.h>
@@ -51,6 +53,21 @@ void efi_puts(const char *str)
 	}
 }
 
+int efi_printk(const char *fmt, ...)
+{
+	char printf_buf[256];
+	va_list args;
+	int printed;
+
+	va_start(args, fmt);
+	printed = vsprintf(printf_buf, fmt, args);
+	va_end(args);
+
+	efi_puts(printf_buf);
+
+	return printed;
+}
+
 /*
  * Parse the ASCII string 'cmdline' for EFI options, denoted by the efi=
  * option, e.g. efi=nochunk.
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index 4f57611a65f2..caa7dcc71c69 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -3,6 +3,7 @@
 #ifndef _DRIVERS_FIRMWARE_EFI_EFISTUB_H
 #define _DRIVERS_FIRMWARE_EFI_EFISTUB_H
 
+#include <linux/compiler.h>
 #include <linux/efi.h>
 #include <linux/kernel.h>
 #include <linux/types.h>
@@ -632,6 +633,8 @@ void *get_efi_config_table(efi_guid_t guid);
 void efi_char16_puts(efi_char16_t *);
 void efi_puts(const char *str);
 
+__printf(1, 2) int efi_printk(char const *fmt, ...);
+
 void efi_free(unsigned long size, unsigned long addr);
 
 char *efi_convert_cmdline(efi_loaded_image_t *image, int *cmd_line_len,
diff --git a/drivers/firmware/efi/libstub/vsprintf.c b/drivers/firmware/efi/libstub/vsprintf.c
new file mode 100644
index 000000000000..88483a49bac4
--- /dev/null
+++ b/drivers/firmware/efi/libstub/vsprintf.c
@@ -0,0 +1,298 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* -*- linux-c -*- ------------------------------------------------------- *
+ *
+ *   Copyright (C) 1991, 1992 Linus Torvalds
+ *   Copyright 2007 rPath, Inc. - All Rights Reserved
+ *
+ * ----------------------------------------------------------------------- */
+
+/*
+ * Oh, it's a waste of space, but oh-so-yummy for debugging.  This
+ * version of printf() does not include 64-bit support.  "Live with
+ * it."
+ *
+ */
+
+#include <stdarg.h>
+
+#include <linux/compiler.h>
+#include <linux/ctype.h>
+#include <linux/string.h>
+
+static int skip_atoi(const char **s)
+{
+	int i = 0;
+
+	while (isdigit(**s))
+		i = i * 10 + *((*s)++) - '0';
+	return i;
+}
+
+#define ZEROPAD	1		/* pad with zero */
+#define SIGN	2		/* unsigned/signed long */
+#define PLUS	4		/* show plus */
+#define SPACE	8		/* space if plus */
+#define LEFT	16		/* left justified */
+#define SMALL	32		/* Must be 32 == 0x20 */
+#define SPECIAL	64		/* 0x */
+
+#define __do_div(n, base) ({ \
+int __res; \
+__res = ((unsigned long) n) % (unsigned) base; \
+n = ((unsigned long) n) / (unsigned) base; \
+__res; })
+
+static char *number(char *str, long num, int base, int size, int precision,
+		    int type)
+{
+	/* we are called with base 8, 10 or 16, only, thus don't need "G..."  */
+	static const char digits[16] = "0123456789ABCDEF"; /* "GHIJKLMNOPQRSTUVWXYZ"; */
+
+	char tmp[66];
+	char c, sign, locase;
+	int i;
+
+	/* locase = 0 or 0x20. ORing digits or letters with 'locase'
+	 * produces same digits or (maybe lowercased) letters */
+	locase = (type & SMALL);
+	if (type & LEFT)
+		type &= ~ZEROPAD;
+	if (base < 2 || base > 16)
+		return NULL;
+	c = (type & ZEROPAD) ? '0' : ' ';
+	sign = 0;
+	if (type & SIGN) {
+		if (num < 0) {
+			sign = '-';
+			num = -num;
+			size--;
+		} else if (type & PLUS) {
+			sign = '+';
+			size--;
+		} else if (type & SPACE) {
+			sign = ' ';
+			size--;
+		}
+	}
+	if (type & SPECIAL) {
+		if (base == 16)
+			size -= 2;
+		else if (base == 8)
+			size--;
+	}
+	i = 0;
+	if (num == 0)
+		tmp[i++] = '0';
+	else
+		while (num != 0)
+			tmp[i++] = (digits[__do_div(num, base)] | locase);
+	if (i > precision)
+		precision = i;
+	size -= precision;
+	if (!(type & (ZEROPAD + LEFT)))
+		while (size-- > 0)
+			*str++ = ' ';
+	if (sign)
+		*str++ = sign;
+	if (type & SPECIAL) {
+		if (base == 8)
+			*str++ = '0';
+		else if (base == 16) {
+			*str++ = '0';
+			*str++ = ('X' | locase);
+		}
+	}
+	if (!(type & LEFT))
+		while (size-- > 0)
+			*str++ = c;
+	while (i < precision--)
+		*str++ = '0';
+	while (i-- > 0)
+		*str++ = tmp[i];
+	while (size-- > 0)
+		*str++ = ' ';
+	return str;
+}
+
+int vsprintf(char *buf, const char *fmt, va_list args)
+{
+	int len;
+	unsigned long num;
+	int i, base;
+	char *str;
+	const char *s;
+
+	int flags;		/* flags to number() */
+
+	int field_width;	/* width of output field */
+	int precision;		/* min. # of digits for integers; max
+				   number of chars for from string */
+	int qualifier;		/* 'h', 'l', or 'L' for integer fields */
+
+	for (str = buf; *fmt; ++fmt) {
+		if (*fmt != '%') {
+			*str++ = *fmt;
+			continue;
+		}
+
+		/* process flags */
+		flags = 0;
+	      repeat:
+		++fmt;		/* this also skips first '%' */
+		switch (*fmt) {
+		case '-':
+			flags |= LEFT;
+			goto repeat;
+		case '+':
+			flags |= PLUS;
+			goto repeat;
+		case ' ':
+			flags |= SPACE;
+			goto repeat;
+		case '#':
+			flags |= SPECIAL;
+			goto repeat;
+		case '0':
+			flags |= ZEROPAD;
+			goto repeat;
+		}
+
+		/* get field width */
+		field_width = -1;
+		if (isdigit(*fmt))
+			field_width = skip_atoi(&fmt);
+		else if (*fmt == '*') {
+			++fmt;
+			/* it's the next argument */
+			field_width = va_arg(args, int);
+			if (field_width < 0) {
+				field_width = -field_width;
+				flags |= LEFT;
+			}
+		}
+
+		/* get the precision */
+		precision = -1;
+		if (*fmt == '.') {
+			++fmt;
+			if (isdigit(*fmt))
+				precision = skip_atoi(&fmt);
+			else if (*fmt == '*') {
+				++fmt;
+				/* it's the next argument */
+				precision = va_arg(args, int);
+			}
+			if (precision < 0)
+				precision = 0;
+		}
+
+		/* get the conversion qualifier */
+		qualifier = -1;
+		if (*fmt == 'h' || *fmt == 'l' || *fmt == 'L') {
+			qualifier = *fmt;
+			++fmt;
+		}
+
+		/* default base */
+		base = 10;
+
+		switch (*fmt) {
+		case 'c':
+			if (!(flags & LEFT))
+				while (--field_width > 0)
+					*str++ = ' ';
+			*str++ = (unsigned char)va_arg(args, int);
+			while (--field_width > 0)
+				*str++ = ' ';
+			continue;
+
+		case 's':
+			s = va_arg(args, char *);
+			len = strnlen(s, precision);
+
+			if (!(flags & LEFT))
+				while (len < field_width--)
+					*str++ = ' ';
+			for (i = 0; i < len; ++i)
+				*str++ = *s++;
+			while (len < field_width--)
+				*str++ = ' ';
+			continue;
+
+		case 'p':
+			if (field_width == -1) {
+				field_width = 2 * sizeof(void *);
+				flags |= ZEROPAD;
+			}
+			str = number(str,
+				     (unsigned long)va_arg(args, void *), 16,
+				     field_width, precision, flags);
+			continue;
+
+		case 'n':
+			if (qualifier == 'l') {
+				long *ip = va_arg(args, long *);
+				*ip = (str - buf);
+			} else {
+				int *ip = va_arg(args, int *);
+				*ip = (str - buf);
+			}
+			continue;
+
+		case '%':
+			*str++ = '%';
+			continue;
+
+			/* integer number formats - set up the flags and "break" */
+		case 'o':
+			base = 8;
+			break;
+
+		case 'x':
+			flags |= SMALL;
+			fallthrough;
+		case 'X':
+			base = 16;
+			break;
+
+		case 'd':
+		case 'i':
+			flags |= SIGN;
+			fallthrough;
+		case 'u':
+			break;
+
+		default:
+			*str++ = '%';
+			if (*fmt)
+				*str++ = *fmt;
+			else
+				--fmt;
+			continue;
+		}
+		if (qualifier == 'l')
+			num = va_arg(args, unsigned long);
+		else if (qualifier == 'h') {
+			num = (unsigned short)va_arg(args, int);
+			if (flags & SIGN)
+				num = (short)num;
+		} else if (flags & SIGN)
+			num = va_arg(args, int);
+		else
+			num = va_arg(args, unsigned int);
+		str = number(str, num, base, field_width, precision, flags);
+	}
+	*str = '\0';
+	return str - buf;
+}
+
+int sprintf(char *buf, const char *fmt, ...)
+{
+	va_list args;
+	int i;
+
+	va_start(args, fmt);
+	i = vsprintf(buf, fmt, args);
+	va_end(args);
+	return i;
+}
-- 
2.26.2

[PATCH 05/24] efi/libstub: Optimize for size instead of speed

[Arvind Sankar]

Reclaim the bloat from the addition of printf by optimizing the stub for
size. With gcc 9, the text size of the stub is:

ARCH    before  +printf    -Os
arm      35197    37889  34638
arm64    34883    38159  34479
i386     18571    21657  17025
x86_64   25677    29328  22144

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 drivers/firmware/efi/libstub/Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
index fb34c9d14a3c..034d71663b1e 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -7,7 +7,7 @@
 #
 cflags-$(CONFIG_X86_32)		:= -march=i386
 cflags-$(CONFIG_X86_64)		:= -mcmodel=small
-cflags-$(CONFIG_X86)		+= -m$(BITS) -D__KERNEL__ -O2 \
+cflags-$(CONFIG_X86)		+= -m$(BITS) -D__KERNEL__ \
 				   -fPIC -fno-strict-aliasing -mno-red-zone \
 				   -mno-mmx -mno-sse -fshort-wchar \
 				   -Wno-pointer-sign \
@@ -25,7 +25,7 @@ cflags-$(CONFIG_ARM)		:= $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
 
 cflags-$(CONFIG_EFI_GENERIC_STUB) += -I$(srctree)/scripts/dtc/libfdt
 
-KBUILD_CFLAGS			:= $(cflags-y) -DDISABLE_BRANCH_PROFILING \
+KBUILD_CFLAGS			:= $(cflags-y) -Os -DDISABLE_BRANCH_PROFILING \
 				   -include $(srctree)/drivers/firmware/efi/libstub/hidden.h \
 				   -D__NO_FORTIFY \
 				   $(call cc-option,-ffreestanding) \
-- 
2.26.2

[PATCH 06/24] efi/printf: Drop %n format and L qualifier

[Arvind Sankar]

%n is unused and deprecated.

The L qualifer is parsed but not actually implemented.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 drivers/firmware/efi/libstub/vsprintf.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/firmware/efi/libstub/vsprintf.c b/drivers/firmware/efi/libstub/vsprintf.c
index 88483a49bac4..2815196de40b 100644
--- a/drivers/firmware/efi/libstub/vsprintf.c
+++ b/drivers/firmware/efi/libstub/vsprintf.c
@@ -127,7 +127,7 @@ int vsprintf(char *buf, const char *fmt, va_list args)
 	int field_width;	/* width of output field */
 	int precision;		/* min. # of digits for integers; max
 				   number of chars for from string */
-	int qualifier;		/* 'h', 'l', or 'L' for integer fields */
+	int qualifier;		/* 'h' or 'l' for integer fields */
 
 	for (str = buf; *fmt; ++fmt) {
 		if (*fmt != '%') {
@@ -188,7 +188,7 @@ int vsprintf(char *buf, const char *fmt, va_list args)
 
 		/* get the conversion qualifier */
 		qualifier = -1;
-		if (*fmt == 'h' || *fmt == 'l' || *fmt == 'L') {
+		if (*fmt == 'h' || *fmt == 'l') {
 			qualifier = *fmt;
 			++fmt;
 		}
@@ -229,16 +229,6 @@ int vsprintf(char *buf, const char *fmt, va_list args)
 				     field_width, precision, flags);
 			continue;
 
-		case 'n':
-			if (qualifier == 'l') {
-				long *ip = va_arg(args, long *);
-				*ip = (str - buf);
-			} else {
-				int *ip = va_arg(args, int *);
-				*ip = (str - buf);
-			}
-			continue;
-
 		case '%':
 			*str++ = '%';
 			continue;
-- 
2.26.2

[PATCH 07/24] efi/printf: Add 64-bit and 8-bit integer support

[Arvind Sankar]

Support 'll' qualifier for long long by copying the decimal printing
code from lib/vsprintf.c. For simplicity, the 32-bit code is used on
64-bit architectures as well.

Support 'hh' qualifier for signed/unsigned char type integers.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 drivers/firmware/efi/libstub/vsprintf.c | 172 ++++++++++++++++++++----
 1 file changed, 144 insertions(+), 28 deletions(-)

diff --git a/drivers/firmware/efi/libstub/vsprintf.c b/drivers/firmware/efi/libstub/vsprintf.c
index 2815196de40b..20fc2530f805 100644
--- a/drivers/firmware/efi/libstub/vsprintf.c
+++ b/drivers/firmware/efi/libstub/vsprintf.c
@@ -7,10 +7,7 @@
  * ----------------------------------------------------------------------- */
 
 /*
- * Oh, it's a waste of space, but oh-so-yummy for debugging.  This
- * version of printf() does not include 64-bit support.  "Live with
- * it."
- *
+ * Oh, it's a waste of space, but oh-so-yummy for debugging.
  */
 
 #include <stdarg.h>
@@ -28,6 +25,86 @@ static int skip_atoi(const char **s)
 	return i;
 }
 
+/*
+ * put_dec_full4 handles numbers in the range 0 <= r < 10000.
+ * The multiplier 0xccd is round(2^15/10), and the approximation
+ * r/10 == (r * 0xccd) >> 15 is exact for all r < 16389.
+ */
+static
+void put_dec_full4(char *buf, unsigned int r)
+{
+	int i;
+
+	for (i = 0; i < 3; i++) {
+		unsigned int q = (r * 0xccd) >> 15;
+		*buf++ = '0' + (r - q * 10);
+		r = q;
+	}
+	*buf++ = '0' + r;
+}
+
+/* put_dec is copied from lib/vsprintf.c with small modifications */
+
+/*
+ * Call put_dec_full4 on x % 10000, return x / 10000.
+ * The approximation x/10000 == (x * 0x346DC5D7) >> 43
+ * holds for all x < 1,128,869,999.  The largest value this
+ * helper will ever be asked to convert is 1,125,520,955.
+ * (second call in the put_dec code, assuming n is all-ones).
+ */
+static
+unsigned int put_dec_helper4(char *buf, unsigned int x)
+{
+	unsigned int q = (x * 0x346DC5D7ULL) >> 43;
+
+	put_dec_full4(buf, x - q * 10000);
+	return q;
+}
+
+/* Based on code by Douglas W. Jones found at
+ * <http://www.cs.uiowa.edu/~jones/bcd/decimal.html#sixtyfour>
+ * (with permission from the author).
+ * Performs no 64-bit division and hence should be fast on 32-bit machines.
+ */
+static
+int put_dec(char *buf, unsigned long long n)
+{
+	unsigned int d3, d2, d1, q, h;
+	char *p = buf;
+
+	d1  = ((unsigned int)n >> 16); /* implicit "& 0xffff" */
+	h   = (n >> 32);
+	d2  = (h      ) & 0xffff;
+	d3  = (h >> 16); /* implicit "& 0xffff" */
+
+	/* n = 2^48 d3 + 2^32 d2 + 2^16 d1 + d0
+	     = 281_4749_7671_0656 d3 + 42_9496_7296 d2 + 6_5536 d1 + d0 */
+	q = 656 * d3 + 7296 * d2 + 5536 * d1 + ((unsigned int)n & 0xffff);
+	q = put_dec_helper4(p, q);
+	p += 4;
+
+	q += 7671 * d3 + 9496 * d2 + 6 * d1;
+	q = put_dec_helper4(p, q);
+	p += 4;
+
+	q += 4749 * d3 + 42 * d2;
+	q = put_dec_helper4(p, q);
+	p += 4;
+
+	q += 281 * d3;
+	q = put_dec_helper4(p, q);
+	p += 4;
+
+	put_dec_full4(p, q);
+	p += 4;
+
+	/* strip off the extra 0's we printed */
+	while (p > buf && p[-1] == '0')
+		--p;
+
+	return p - buf;
+}
+
 #define ZEROPAD	1		/* pad with zero */
 #define SIGN	2		/* unsigned/signed long */
 #define PLUS	4		/* show plus */
@@ -36,13 +113,7 @@ static int skip_atoi(const char **s)
 #define SMALL	32		/* Must be 32 == 0x20 */
 #define SPECIAL	64		/* 0x */
 
-#define __do_div(n, base) ({ \
-int __res; \
-__res = ((unsigned long) n) % (unsigned) base; \
-n = ((unsigned long) n) / (unsigned) base; \
-__res; })
-
-static char *number(char *str, long num, int base, int size, int precision,
+static char *number(char *str, long long num, int base, int size, int precision,
 		    int type)
 {
 	/* we are called with base 8, 10 or 16, only, thus don't need "G..."  */
@@ -57,8 +128,6 @@ static char *number(char *str, long num, int base, int size, int precision,
 	locase = (type & SMALL);
 	if (type & LEFT)
 		type &= ~ZEROPAD;
-	if (base < 2 || base > 16)
-		return NULL;
 	c = (type & ZEROPAD) ? '0' : ' ';
 	sign = 0;
 	if (type & SIGN) {
@@ -83,9 +152,27 @@ static char *number(char *str, long num, int base, int size, int precision,
 	i = 0;
 	if (num == 0)
 		tmp[i++] = '0';
-	else
-		while (num != 0)
-			tmp[i++] = (digits[__do_div(num, base)] | locase);
+	else switch (base) {
+		case 10:
+			i += put_dec(&tmp[i], num);
+			break;
+		case 8:
+			while (num != 0) {
+				tmp[i++] = '0' + (num & 07);
+				num = (unsigned long long)num >> 3;
+			}
+			break;
+		case 16:
+			while (num != 0) {
+				tmp[i++] = digits[num & 0xf] | locase;
+				num = (unsigned long long)num >> 4;
+			}
+			break;
+
+		default:
+			unreachable();
+	}
+
 	if (i > precision)
 		precision = i;
 	size -= precision;
@@ -117,7 +204,7 @@ static char *number(char *str, long num, int base, int size, int precision,
 int vsprintf(char *buf, const char *fmt, va_list args)
 {
 	int len;
-	unsigned long num;
+	unsigned long long num;
 	int i, base;
 	char *str;
 	const char *s;
@@ -127,7 +214,7 @@ int vsprintf(char *buf, const char *fmt, va_list args)
 	int field_width;	/* width of output field */
 	int precision;		/* min. # of digits for integers; max
 				   number of chars for from string */
-	int qualifier;		/* 'h' or 'l' for integer fields */
+	int qualifier;		/* 'h', 'hh', 'l' or 'll' for integer fields */
 
 	for (str = buf; *fmt; ++fmt) {
 		if (*fmt != '%') {
@@ -191,6 +278,10 @@ int vsprintf(char *buf, const char *fmt, va_list args)
 		if (*fmt == 'h' || *fmt == 'l') {
 			qualifier = *fmt;
 			++fmt;
+			if (qualifier == *fmt) {
+				qualifier -= 'a'-'A';
+				++fmt;
+			}
 		}
 
 		/* default base */
@@ -260,16 +351,41 @@ int vsprintf(char *buf, const char *fmt, va_list args)
 				--fmt;
 			continue;
 		}
-		if (qualifier == 'l')
-			num = va_arg(args, unsigned long);
-		else if (qualifier == 'h') {
-			num = (unsigned short)va_arg(args, int);
-			if (flags & SIGN)
-				num = (short)num;
-		} else if (flags & SIGN)
-			num = va_arg(args, int);
-		else
-			num = va_arg(args, unsigned int);
+		if (flags & SIGN) {
+			switch (qualifier) {
+			case 'L':
+				num = va_arg(args, long long);
+				break;
+			case 'l':
+				num = va_arg(args, long);
+				break;
+			case 'h':
+				num = (short)va_arg(args, int);
+				break;
+			case 'H':
+				num = (signed char)va_arg(args, int);
+				break;
+			default:
+				num = va_arg(args, int);
+			}
+		} else {
+			switch (qualifier) {
+			case 'L':
+				num = va_arg(args, unsigned long long);
+				break;
+			case 'l':
+				num = va_arg(args, unsigned long);
+				break;
+			case 'h':
+				num = (unsigned short)va_arg(args, int);
+				break;
+			case 'H':
+				num = (unsigned char)va_arg(args, int);
+				break;
+			default:
+				num = va_arg(args, unsigned int);
+			}
+		}
 		str = number(str, num, base, field_width, precision, flags);
 	}
 	*str = '\0';
-- 
2.26.2

[PATCH 08/24] efi/printf: Factor out flags parsing and handle '%' earlier

[Arvind Sankar]

Move flags parsing code out into a helper function.

The '%%' case can be handled up front: it is not allowed to have flags,
width etc.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 drivers/firmware/efi/libstub/vsprintf.c | 56 ++++++++++++++-----------
 1 file changed, 31 insertions(+), 25 deletions(-)

diff --git a/drivers/firmware/efi/libstub/vsprintf.c b/drivers/firmware/efi/libstub/vsprintf.c
index 20fc2530f805..b3ed4af77af5 100644
--- a/drivers/firmware/efi/libstub/vsprintf.c
+++ b/drivers/firmware/efi/libstub/vsprintf.c
@@ -201,6 +201,35 @@ static char *number(char *str, long long num, int base, int size, int precision,
 	return str;
 }
 
+static
+int get_flags(const char **fmt)
+{
+	int flags = 0;
+
+	do {
+		switch (**fmt) {
+		case '-':
+			flags |= LEFT;
+			break;
+		case '+':
+			flags |= PLUS;
+			break;
+		case ' ':
+			flags |= SPACE;
+			break;
+		case '#':
+			flags |= SPECIAL;
+			break;
+		case '0':
+			flags |= ZEROPAD;
+			break;
+		default:
+			return flags;
+		}
+		++(*fmt);
+	} while (1);
+}
+
 int vsprintf(char *buf, const char *fmt, va_list args)
 {
 	int len;
@@ -217,32 +246,13 @@ int vsprintf(char *buf, const char *fmt, va_list args)
 	int qualifier;		/* 'h', 'hh', 'l' or 'll' for integer fields */
 
 	for (str = buf; *fmt; ++fmt) {
-		if (*fmt != '%') {
+		if (*fmt != '%' || *++fmt == '%') {
 			*str++ = *fmt;
 			continue;
 		}
 
 		/* process flags */
-		flags = 0;
-	      repeat:
-		++fmt;		/* this also skips first '%' */
-		switch (*fmt) {
-		case '-':
-			flags |= LEFT;
-			goto repeat;
-		case '+':
-			flags |= PLUS;
-			goto repeat;
-		case ' ':
-			flags |= SPACE;
-			goto repeat;
-		case '#':
-			flags |= SPECIAL;
-			goto repeat;
-		case '0':
-			flags |= ZEROPAD;
-			goto repeat;
-		}
+		flags = get_flags(&fmt);
 
 		/* get field width */
 		field_width = -1;
@@ -320,10 +330,6 @@ int vsprintf(char *buf, const char *fmt, va_list args)
 				     field_width, precision, flags);
 			continue;
 
-		case '%':
-			*str++ = '%';
-			continue;
-
 			/* integer number formats - set up the flags and "break" */
 		case 'o':
 			base = 8;
-- 
2.26.2

[PATCH 09/24] efi/printf: Fix minor bug in precision handling

[Arvind Sankar]

A negative precision should be ignored completely, and the presence of a
valid precision should turn off the 0 flag.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 drivers/firmware/efi/libstub/vsprintf.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/efi/libstub/vsprintf.c b/drivers/firmware/efi/libstub/vsprintf.c
index b3ed4af77af5..d7938e44f067 100644
--- a/drivers/firmware/efi/libstub/vsprintf.c
+++ b/drivers/firmware/efi/libstub/vsprintf.c
@@ -278,9 +278,10 @@ int vsprintf(char *buf, const char *fmt, va_list args)
 				++fmt;
 				/* it's the next argument */
 				precision = va_arg(args, int);
-			}
-			if (precision < 0)
+			} else
 				precision = 0;
+			if (precision >= 0)
+				flags &= ~ZEROPAD;
 		}
 
 		/* get the conversion qualifier */
-- 
2.26.2

[PATCH 10/24] efi/printf: Merge 'p' with the integer formats

[Arvind Sankar]

Treat 'p' as a hexadecimal integer with precision equal to the number of
digits in void *.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 drivers/firmware/efi/libstub/vsprintf.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/firmware/efi/libstub/vsprintf.c b/drivers/firmware/efi/libstub/vsprintf.c
index d7938e44f067..88c503077b92 100644
--- a/drivers/firmware/efi/libstub/vsprintf.c
+++ b/drivers/firmware/efi/libstub/vsprintf.c
@@ -295,9 +295,6 @@ int vsprintf(char *buf, const char *fmt, va_list args)
 			}
 		}
 
-		/* default base */
-		base = 10;
-
 		switch (*fmt) {
 		case 'c':
 			if (!(flags & LEFT))
@@ -321,21 +318,15 @@ int vsprintf(char *buf, const char *fmt, va_list args)
 				*str++ = ' ';
 			continue;
 
-		case 'p':
-			if (field_width == -1) {
-				field_width = 2 * sizeof(void *);
-				flags |= ZEROPAD;
-			}
-			str = number(str,
-				     (unsigned long)va_arg(args, void *), 16,
-				     field_width, precision, flags);
-			continue;
-
 			/* integer number formats - set up the flags and "break" */
 		case 'o':
 			base = 8;
 			break;
 
+		case 'p':
+			if (precision < 0)
+				precision = 2 * sizeof(void *);
+			fallthrough;
 		case 'x':
 			flags |= SMALL;
 			fallthrough;
@@ -348,6 +339,7 @@ int vsprintf(char *buf, const char *fmt, va_list args)
 			flags |= SIGN;
 			fallthrough;
 		case 'u':
+			base = 10;
 			break;
 
 		default:
@@ -358,7 +350,9 @@ int vsprintf(char *buf, const char *fmt, va_list args)
 				--fmt;
 			continue;
 		}
-		if (flags & SIGN) {
+		if (*fmt == 'p')
+			num = (unsigned long)va_arg(args, void *);
+		else if (flags & SIGN) {
 			switch (qualifier) {
 			case 'L':
 				num = va_arg(args, long long);
-- 
2.26.2

[PATCH 11/24] efi/printf: Factor out width/precision parsing

[Arvind Sankar]

Factor out the width/precision parsing into a helper function.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 drivers/firmware/efi/libstub/vsprintf.c | 60 ++++++++++++++++---------
 1 file changed, 40 insertions(+), 20 deletions(-)

diff --git a/drivers/firmware/efi/libstub/vsprintf.c b/drivers/firmware/efi/libstub/vsprintf.c
index 88c503077b92..1b71651fe6bc 100644
--- a/drivers/firmware/efi/libstub/vsprintf.c
+++ b/drivers/firmware/efi/libstub/vsprintf.c
@@ -230,7 +230,20 @@ int get_flags(const char **fmt)
 	} while (1);
 }
 
-int vsprintf(char *buf, const char *fmt, va_list args)
+static
+int get_int(const char **fmt, va_list *ap)
+{
+	if (isdigit(**fmt))
+		return skip_atoi(fmt);
+	if (**fmt == '*') {
+		++(*fmt);
+		/* it's the next argument */
+		return va_arg(*ap, int);
+	}
+	return 0;
+}
+
+int vsprintf(char *buf, const char *fmt, va_list ap)
 {
 	int len;
 	unsigned long long num;
@@ -245,6 +258,24 @@ int vsprintf(char *buf, const char *fmt, va_list args)
 				   number of chars for from string */
 	int qualifier;		/* 'h', 'hh', 'l' or 'll' for integer fields */
 
+	va_list args;
+
+	/*
+	 * We want to pass our input va_list to helper functions by reference,
+	 * but there's an annoying edge case. If va_list was originally passed
+	 * to us by value, we could just pass &ap down to the helpers. This is
+	 * the case on, for example, X86_32.
+	 * However, on X86_64 (and possibly others), va_list is actually a
+	 * size-1 array containing a structure. Our function parameter ap has
+	 * decayed from T[1] to T*, and &ap has type T** rather than T(*)[1],
+	 * which is what will be expected by a function taking a va_list *
+	 * parameter.
+	 * One standard way to solve this mess is by creating a copy in a local
+	 * variable of type va_list and then passing a pointer to that local
+	 * copy instead, which is what we do here.
+	 */
+	va_copy(args, ap);
+
 	for (str = buf; *fmt; ++fmt) {
 		if (*fmt != '%' || *++fmt == '%') {
 			*str++ = *fmt;
@@ -255,31 +286,17 @@ int vsprintf(char *buf, const char *fmt, va_list args)
 		flags = get_flags(&fmt);
 
 		/* get field width */
-		field_width = -1;
-		if (isdigit(*fmt))
-			field_width = skip_atoi(&fmt);
-		else if (*fmt == '*') {
-			++fmt;
-			/* it's the next argument */
-			field_width = va_arg(args, int);
-			if (field_width < 0) {
-				field_width = -field_width;
-				flags |= LEFT;
-			}
+		field_width = get_int(&fmt, &args);
+		if (field_width < 0) {
+			field_width = -field_width;
+			flags |= LEFT;
 		}
 
 		/* get the precision */
 		precision = -1;
 		if (*fmt == '.') {
 			++fmt;
-			if (isdigit(*fmt))
-				precision = skip_atoi(&fmt);
-			else if (*fmt == '*') {
-				++fmt;
-				/* it's the next argument */
-				precision = va_arg(args, int);
-			} else
-				precision = 0;
+			precision = get_int(&fmt, &args);
 			if (precision >= 0)
 				flags &= ~ZEROPAD;
 		}
@@ -390,6 +407,9 @@ int vsprintf(char *buf, const char *fmt, va_list args)
 		str = number(str, num, base, field_width, precision, flags);
 	}
 	*str = '\0';
+
+	va_end(args);
+
 	return str - buf;
 }
 
-- 
2.26.2

[PATCH 12/24] efi/printf: Factor out integer argument retrieval

[Arvind Sankar]

Factor out the code to get the correct type of numeric argument into a
helper function.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 drivers/firmware/efi/libstub/vsprintf.c | 69 ++++++++++++-------------
 1 file changed, 34 insertions(+), 35 deletions(-)

diff --git a/drivers/firmware/efi/libstub/vsprintf.c b/drivers/firmware/efi/libstub/vsprintf.c
index 1b71651fe6bc..dee9bd068cb6 100644
--- a/drivers/firmware/efi/libstub/vsprintf.c
+++ b/drivers/firmware/efi/libstub/vsprintf.c
@@ -243,6 +243,38 @@ int get_int(const char **fmt, va_list *ap)
 	return 0;
 }
 
+static
+unsigned long long get_number(int sign, int qualifier, va_list *ap)
+{
+	if (sign) {
+		switch (qualifier) {
+		case 'L':
+			return va_arg(*ap, long long);
+		case 'l':
+			return va_arg(*ap, long);
+		case 'h':
+			return (short)va_arg(*ap, int);
+		case 'H':
+			return (signed char)va_arg(*ap, int);
+		default:
+			return va_arg(*ap, int);
+		};
+	} else {
+		switch (qualifier) {
+		case 'L':
+			return va_arg(*ap, unsigned long long);
+		case 'l':
+			return va_arg(*ap, unsigned long);
+		case 'h':
+			return (unsigned short)va_arg(*ap, int);
+		case 'H':
+			return (unsigned char)va_arg(*ap, int);
+		default:
+			return va_arg(*ap, unsigned int);
+		}
+	}
+}
+
 int vsprintf(char *buf, const char *fmt, va_list ap)
 {
 	int len;
@@ -369,41 +401,8 @@ int vsprintf(char *buf, const char *fmt, va_list ap)
 		}
 		if (*fmt == 'p')
 			num = (unsigned long)va_arg(args, void *);
-		else if (flags & SIGN) {
-			switch (qualifier) {
-			case 'L':
-				num = va_arg(args, long long);
-				break;
-			case 'l':
-				num = va_arg(args, long);
-				break;
-			case 'h':
-				num = (short)va_arg(args, int);
-				break;
-			case 'H':
-				num = (signed char)va_arg(args, int);
-				break;
-			default:
-				num = va_arg(args, int);
-			}
-		} else {
-			switch (qualifier) {
-			case 'L':
-				num = va_arg(args, unsigned long long);
-				break;
-			case 'l':
-				num = va_arg(args, unsigned long);
-				break;
-			case 'h':
-				num = (unsigned short)va_arg(args, int);
-				break;
-			case 'H':
-				num = (unsigned char)va_arg(args, int);
-				break;
-			default:
-				num = va_arg(args, unsigned int);
-			}
-		}
+		else
+			num = get_number(flags & SIGN, qualifier, &args);
 		str = number(str, num, base, field_width, precision, flags);
 	}
 	*str = '\0';
-- 
2.26.2

[PATCH 13/24] efi/printf: Handle null string input

[Arvind Sankar]

Print "(null)" for 's' if the input is a NULL pointer.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 drivers/firmware/efi/libstub/vsprintf.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/firmware/efi/libstub/vsprintf.c b/drivers/firmware/efi/libstub/vsprintf.c
index dee9bd068cb6..048276d9c376 100644
--- a/drivers/firmware/efi/libstub/vsprintf.c
+++ b/drivers/firmware/efi/libstub/vsprintf.c
@@ -14,6 +14,7 @@
 
 #include <linux/compiler.h>
 #include <linux/ctype.h>
+#include <linux/limits.h>
 #include <linux/string.h>
 
 static int skip_atoi(const char **s)
@@ -355,7 +356,11 @@ int vsprintf(char *buf, const char *fmt, va_list ap)
 			continue;
 
 		case 's':
+			if (precision < 0)
+				precision = INT_MAX;
 			s = va_arg(args, char *);
+			if (!s)
+				s = precision < 6 ? "" : "(null)";
 			len = strnlen(s, precision);
 
 			if (!(flags & LEFT))
-- 
2.26.2

[PATCH 14/24] efi/printf: Refactor code to consolidate padding and output

[Arvind Sankar]

Consolidate the actual output of the formatted text into one place.

Fix a couple of edge cases:
1. If 0 is printed with a precision of 0, the printf specification says
   that nothing should be output, with one exception (2b).
2. The specification for octal alternate format (%#o) adds the leading
   zero not as a prefix as the 0x for hexadecimal is, but by increasing
   the precision if necessary to add the zero. This means that
   a. %#.2o turns 8 into "010", but 1 into "01" rather than "001".
   b. %#.o prints 0 as "0" rather than "", unlike the situation for
      decimal, hexadecimal and regular octal format, which all output an
      empty string.

Reduce the space allocated for printing a number to the maximum actually
required (22 bytes for a 64-bit number in octal), instead of the 66
bytes previously allocated.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 drivers/firmware/efi/libstub/vsprintf.c | 273 +++++++++++++-----------
 1 file changed, 148 insertions(+), 125 deletions(-)

diff --git a/drivers/firmware/efi/libstub/vsprintf.c b/drivers/firmware/efi/libstub/vsprintf.c
index 048276d9c376..3352ba394797 100644
--- a/drivers/firmware/efi/libstub/vsprintf.c
+++ b/drivers/firmware/efi/libstub/vsprintf.c
@@ -14,10 +14,12 @@
 
 #include <linux/compiler.h>
 #include <linux/ctype.h>
+#include <linux/kernel.h>
 #include <linux/limits.h>
 #include <linux/string.h>
 
-static int skip_atoi(const char **s)
+static
+int skip_atoi(const char **s)
 {
 	int i = 0;
 
@@ -32,16 +34,16 @@ static int skip_atoi(const char **s)
  * r/10 == (r * 0xccd) >> 15 is exact for all r < 16389.
  */
 static
-void put_dec_full4(char *buf, unsigned int r)
+void put_dec_full4(char *end, unsigned int r)
 {
 	int i;
 
 	for (i = 0; i < 3; i++) {
 		unsigned int q = (r * 0xccd) >> 15;
-		*buf++ = '0' + (r - q * 10);
+		*--end = '0' + (r - q * 10);
 		r = q;
 	}
-	*buf++ = '0' + r;
+	*--end = '0' + r;
 }
 
 /* put_dec is copied from lib/vsprintf.c with small modifications */
@@ -54,11 +56,11 @@ void put_dec_full4(char *buf, unsigned int r)
  * (second call in the put_dec code, assuming n is all-ones).
  */
 static
-unsigned int put_dec_helper4(char *buf, unsigned int x)
+unsigned int put_dec_helper4(char *end, unsigned int x)
 {
 	unsigned int q = (x * 0x346DC5D7ULL) >> 43;
 
-	put_dec_full4(buf, x - q * 10000);
+	put_dec_full4(end, x - q * 10000);
 	return q;
 }
 
@@ -68,10 +70,10 @@ unsigned int put_dec_helper4(char *buf, unsigned int x)
  * Performs no 64-bit division and hence should be fast on 32-bit machines.
  */
 static
-int put_dec(char *buf, unsigned long long n)
+char *put_dec(char *end, unsigned long long n)
 {
 	unsigned int d3, d2, d1, q, h;
-	char *p = buf;
+	char *p = end;
 
 	d1  = ((unsigned int)n >> 16); /* implicit "& 0xffff" */
 	h   = (n >> 32);
@@ -82,28 +84,59 @@ int put_dec(char *buf, unsigned long long n)
 	     = 281_4749_7671_0656 d3 + 42_9496_7296 d2 + 6_5536 d1 + d0 */
 	q = 656 * d3 + 7296 * d2 + 5536 * d1 + ((unsigned int)n & 0xffff);
 	q = put_dec_helper4(p, q);
-	p += 4;
+	p -= 4;
 
 	q += 7671 * d3 + 9496 * d2 + 6 * d1;
 	q = put_dec_helper4(p, q);
-	p += 4;
+	p -= 4;
 
 	q += 4749 * d3 + 42 * d2;
 	q = put_dec_helper4(p, q);
-	p += 4;
+	p -= 4;
 
 	q += 281 * d3;
 	q = put_dec_helper4(p, q);
-	p += 4;
+	p -= 4;
 
 	put_dec_full4(p, q);
-	p += 4;
+	p -= 4;
 
 	/* strip off the extra 0's we printed */
-	while (p > buf && p[-1] == '0')
-		--p;
+	while (p < end && *p == '0')
+		++p;
 
-	return p - buf;
+	return p;
+}
+
+static
+char *number(char *end, unsigned long long num, int base, char locase)
+{
+	/*
+	 * locase = 0 or 0x20. ORing digits or letters with 'locase'
+	 * produces same digits or (maybe lowercased) letters
+	 */
+
+	/* we are called with base 8, 10 or 16, only, thus don't need "G..."  */
+	static const char digits[16] = "0123456789ABCDEF"; /* "GHIJKLMNOPQRSTUVWXYZ"; */
+
+	switch (base) {
+	case 10:
+		if (num != 0)
+			end = put_dec(end, num);
+		break;
+	case 8:
+		for (; num != 0; num >>= 3)
+			*--end = '0' + (num & 07);
+		break;
+	case 16:
+		for (; num != 0; num >>= 4)
+			*--end = digits[num & 0xf] | locase;
+		break;
+	default:
+		unreachable();
+	};
+
+	return end;
 }
 
 #define ZEROPAD	1		/* pad with zero */
@@ -114,94 +147,6 @@ int put_dec(char *buf, unsigned long long n)
 #define SMALL	32		/* Must be 32 == 0x20 */
 #define SPECIAL	64		/* 0x */
 
-static char *number(char *str, long long num, int base, int size, int precision,
-		    int type)
-{
-	/* we are called with base 8, 10 or 16, only, thus don't need "G..."  */
-	static const char digits[16] = "0123456789ABCDEF"; /* "GHIJKLMNOPQRSTUVWXYZ"; */
-
-	char tmp[66];
-	char c, sign, locase;
-	int i;
-
-	/* locase = 0 or 0x20. ORing digits or letters with 'locase'
-	 * produces same digits or (maybe lowercased) letters */
-	locase = (type & SMALL);
-	if (type & LEFT)
-		type &= ~ZEROPAD;
-	c = (type & ZEROPAD) ? '0' : ' ';
-	sign = 0;
-	if (type & SIGN) {
-		if (num < 0) {
-			sign = '-';
-			num = -num;
-			size--;
-		} else if (type & PLUS) {
-			sign = '+';
-			size--;
-		} else if (type & SPACE) {
-			sign = ' ';
-			size--;
-		}
-	}
-	if (type & SPECIAL) {
-		if (base == 16)
-			size -= 2;
-		else if (base == 8)
-			size--;
-	}
-	i = 0;
-	if (num == 0)
-		tmp[i++] = '0';
-	else switch (base) {
-		case 10:
-			i += put_dec(&tmp[i], num);
-			break;
-		case 8:
-			while (num != 0) {
-				tmp[i++] = '0' + (num & 07);
-				num = (unsigned long long)num >> 3;
-			}
-			break;
-		case 16:
-			while (num != 0) {
-				tmp[i++] = digits[num & 0xf] | locase;
-				num = (unsigned long long)num >> 4;
-			}
-			break;
-
-		default:
-			unreachable();
-	}
-
-	if (i > precision)
-		precision = i;
-	size -= precision;
-	if (!(type & (ZEROPAD + LEFT)))
-		while (size-- > 0)
-			*str++ = ' ';
-	if (sign)
-		*str++ = sign;
-	if (type & SPECIAL) {
-		if (base == 8)
-			*str++ = '0';
-		else if (base == 16) {
-			*str++ = '0';
-			*str++ = ('X' | locase);
-		}
-	}
-	if (!(type & LEFT))
-		while (size-- > 0)
-			*str++ = c;
-	while (i < precision--)
-		*str++ = '0';
-	while (i-- > 0)
-		*str++ = tmp[i];
-	while (size-- > 0)
-		*str++ = ' ';
-	return str;
-}
-
 static
 int get_flags(const char **fmt)
 {
@@ -276,13 +221,33 @@ unsigned long long get_number(int sign, int qualifier, va_list *ap)
 	}
 }
 
+static
+char get_sign(long long *num, int flags)
+{
+	if (!(flags & SIGN))
+		return 0;
+	if (*num < 0) {
+		*num = -(*num);
+		return '-';
+	}
+	if (flags & PLUS)
+		return '+';
+	if (flags & SPACE)
+		return ' ';
+	return 0;
+}
+
 int vsprintf(char *buf, const char *fmt, va_list ap)
 {
-	int len;
-	unsigned long long num;
-	int i, base;
+	/* The maximum space required is to print a 64-bit number in octal */
+	char tmp[(sizeof(unsigned long long) * 8 + 2) / 3];
+	char *tmp_end = &tmp[ARRAY_SIZE(tmp)];
+	long long num;
+	int base;
 	char *str;
 	const char *s;
+	int len;
+	char sign;
 
 	int flags;		/* flags to number() */
 
@@ -325,6 +290,9 @@ int vsprintf(char *buf, const char *fmt, va_list ap)
 			flags |= LEFT;
 		}
 
+		if (flags & LEFT)
+			flags &= ~ZEROPAD;
+
 		/* get the precision */
 		precision = -1;
 		if (*fmt == '.') {
@@ -345,32 +313,25 @@ int vsprintf(char *buf, const char *fmt, va_list ap)
 			}
 		}
 
+		sign = 0;
+
 		switch (*fmt) {
 		case 'c':
-			if (!(flags & LEFT))
-				while (--field_width > 0)
-					*str++ = ' ';
-			*str++ = (unsigned char)va_arg(args, int);
-			while (--field_width > 0)
-				*str++ = ' ';
-			continue;
+			flags &= LEFT;
+			tmp[0] = (unsigned char)va_arg(args, int);
+			s = tmp;
+			precision = len = 1;
+			goto output;
 
 		case 's':
+			flags &= LEFT;
 			if (precision < 0)
 				precision = INT_MAX;
 			s = va_arg(args, char *);
 			if (!s)
 				s = precision < 6 ? "" : "(null)";
-			len = strnlen(s, precision);
-
-			if (!(flags & LEFT))
-				while (len < field_width--)
-					*str++ = ' ';
-			for (i = 0; i < len; ++i)
-				*str++ = *s++;
-			while (len < field_width--)
-				*str++ = ' ';
-			continue;
+			precision = len = strnlen(s, precision);
+			goto output;
 
 			/* integer number formats - set up the flags and "break" */
 		case 'o':
@@ -393,6 +354,7 @@ int vsprintf(char *buf, const char *fmt, va_list ap)
 			flags |= SIGN;
 			fallthrough;
 		case 'u':
+			flags &= ~SPECIAL;
 			base = 10;
 			break;
 
@@ -408,7 +370,68 @@ int vsprintf(char *buf, const char *fmt, va_list ap)
 			num = (unsigned long)va_arg(args, void *);
 		else
 			num = get_number(flags & SIGN, qualifier, &args);
-		str = number(str, num, base, field_width, precision, flags);
+
+		sign = get_sign(&num, flags);
+		if (sign)
+			--field_width;
+
+		s = number(tmp_end, num, base, flags & SMALL);
+		len = tmp_end - s;
+		/* default precision is 1 */
+		if (precision < 0)
+			precision = 1;
+		/* precision is minimum number of digits to print */
+		if (precision < len)
+			precision = len;
+		if (flags & SPECIAL) {
+			/*
+			 * For octal, a leading 0 is printed only if necessary,
+			 * i.e. if it's not already there because of the
+			 * precision.
+			 */
+			if (base == 8 && precision == len)
+				++precision;
+			/*
+			 * For hexadecimal, the leading 0x is skipped if the
+			 * output is empty, i.e. both the number and the
+			 * precision are 0.
+			 */
+			if (base == 16 && precision > 0)
+				field_width -= 2;
+			else
+				flags &= ~SPECIAL;
+		}
+		/*
+		 * For zero padding, increase the precision to fill the field
+		 * width.
+		 */
+		if ((flags & ZEROPAD) && field_width > precision)
+			precision = field_width;
+
+output:
+		/* Calculate the padding necessary */
+		field_width -= precision;
+		/* Leading padding with ' ' */
+		if (!(flags & LEFT))
+			while (field_width-- > 0)
+				*str++ = ' ';
+		/* sign */
+		if (sign)
+			*str++ = sign;
+		/* 0x/0X for hexadecimal */
+		if (flags & SPECIAL) {
+			*str++ = '0';
+			*str++ = 'X' | (flags & SMALL);
+		}
+		/* Zero padding and excess precision */
+		while (precision-- > len)
+			*str++ = '0';
+		/* Actual output */
+		while (len-- > 0)
+			*str++ = *s++;
+		/* Trailing padding with ' ' */
+		while (field_width-- > 0)
+			*str++ = ' ';
 	}
 	*str = '\0';
 
-- 
2.26.2

[PATCH 15/24] efi/printf: Abort on invalid format

[Arvind Sankar]

If we get an invalid conversion specifier, bail out instead of trying to
fix it up. The format string likely has a typo or assumed we support
something that we don't, in either case the remaining arguments won't
match up with the remaining format string.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 drivers/firmware/efi/libstub/vsprintf.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/efi/libstub/vsprintf.c b/drivers/firmware/efi/libstub/vsprintf.c
index 3352ba394797..7dcbc04498e7 100644
--- a/drivers/firmware/efi/libstub/vsprintf.c
+++ b/drivers/firmware/efi/libstub/vsprintf.c
@@ -359,12 +359,13 @@ int vsprintf(char *buf, const char *fmt, va_list ap)
 			break;
 
 		default:
-			*str++ = '%';
-			if (*fmt)
-				*str++ = *fmt;
-			else
-				--fmt;
-			continue;
+			/*
+			 * Bail out if the conversion specifier is invalid.
+			 * There's probably a typo in the format string and the
+			 * remaining specifiers are unlikely to match up with
+			 * the arguments.
+			 */
+			goto fail;
 		}
 		if (*fmt == 'p')
 			num = (unsigned long)va_arg(args, void *);
@@ -433,6 +434,7 @@ int vsprintf(char *buf, const char *fmt, va_list ap)
 		while (field_width-- > 0)
 			*str++ = ' ';
 	}
+fail:
 	*str = '\0';
 
 	va_end(args);
-- 
2.26.2

[PATCH 16/24] efi/printf: Turn vsprintf into vsnprintf

[Arvind Sankar]

Implement vsnprintf instead of vsprintf to avoid the possibility of a
buffer overflow.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 .../firmware/efi/libstub/efi-stub-helper.c    |  6 ++-
 drivers/firmware/efi/libstub/vsprintf.c       | 42 +++++++++++--------
 2 files changed, 30 insertions(+), 18 deletions(-)

diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index 56b3b84fd3bd..5ecafc57619a 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -60,10 +60,14 @@ int efi_printk(const char *fmt, ...)
 	int printed;
 
 	va_start(args, fmt);
-	printed = vsprintf(printf_buf, fmt, args);
+	printed = vsnprintf(printf_buf, sizeof(printf_buf), fmt, args);
 	va_end(args);
 
 	efi_puts(printf_buf);
+	if (printed >= sizeof(printf_buf)) {
+		efi_puts("[Message truncated]\n");
+		return -1;
+	}
 
 	return printed;
 }
diff --git a/drivers/firmware/efi/libstub/vsprintf.c b/drivers/firmware/efi/libstub/vsprintf.c
index 7dcbc04498e7..36f2f4cf7434 100644
--- a/drivers/firmware/efi/libstub/vsprintf.c
+++ b/drivers/firmware/efi/libstub/vsprintf.c
@@ -17,6 +17,7 @@
 #include <linux/kernel.h>
 #include <linux/limits.h>
 #include <linux/string.h>
+#include <linux/types.h>
 
 static
 int skip_atoi(const char **s)
@@ -237,16 +238,22 @@ char get_sign(long long *num, int flags)
 	return 0;
 }
 
-int vsprintf(char *buf, const char *fmt, va_list ap)
+#define PUTC(c) \
+do {				\
+	if (pos < size)		\
+		buf[pos] = (c);	\
+	++pos;			\
+} while (0);
+
+int vsnprintf(char *buf, size_t size, const char *fmt, va_list ap)
 {
 	/* The maximum space required is to print a 64-bit number in octal */
 	char tmp[(sizeof(unsigned long long) * 8 + 2) / 3];
 	char *tmp_end = &tmp[ARRAY_SIZE(tmp)];
 	long long num;
 	int base;
-	char *str;
 	const char *s;
-	int len;
+	size_t len, pos;
 	char sign;
 
 	int flags;		/* flags to number() */
@@ -274,9 +281,9 @@ int vsprintf(char *buf, const char *fmt, va_list ap)
 	 */
 	va_copy(args, ap);
 
-	for (str = buf; *fmt; ++fmt) {
+	for (pos = 0; *fmt; ++fmt) {
 		if (*fmt != '%' || *++fmt == '%') {
-			*str++ = *fmt;
+			PUTC(*fmt);
 			continue;
 		}
 
@@ -415,40 +422,41 @@ int vsprintf(char *buf, const char *fmt, va_list ap)
 		/* Leading padding with ' ' */
 		if (!(flags & LEFT))
 			while (field_width-- > 0)
-				*str++ = ' ';
+				PUTC(' ');
 		/* sign */
 		if (sign)
-			*str++ = sign;
+			PUTC(sign);
 		/* 0x/0X for hexadecimal */
 		if (flags & SPECIAL) {
-			*str++ = '0';
-			*str++ = 'X' | (flags & SMALL);
+			PUTC('0');
+			PUTC( 'X' | (flags & SMALL));
 		}
 		/* Zero padding and excess precision */
 		while (precision-- > len)
-			*str++ = '0';
+			PUTC('0');
 		/* Actual output */
 		while (len-- > 0)
-			*str++ = *s++;
+			PUTC(*s++);
 		/* Trailing padding with ' ' */
 		while (field_width-- > 0)
-			*str++ = ' ';
+			PUTC(' ');
 	}
 fail:
-	*str = '\0';
-
 	va_end(args);
 
-	return str - buf;
+	if (size)
+		buf[min(pos, size-1)] = '\0';
+
+	return pos;
 }
 
-int sprintf(char *buf, const char *fmt, ...)
+int snprintf(char *buf, size_t size, const char *fmt, ...)
 {
 	va_list args;
 	int i;
 
 	va_start(args, fmt);
-	i = vsprintf(buf, fmt, args);
+	i = vsnprintf(buf, size, fmt, args);
 	va_end(args);
 	return i;
 }
-- 
2.26.2

[PATCH 17/24] efi/libstub: Implement printk-style logging

[Arvind Sankar]

Use the efi_printk function in efi_info/efi_err, and add efi_debug. This
allows formatted output at different log levels.

Add the notion of a loglevel instead of just quiet/not-quiet, and
parse the debug kernel parameter in addition to quiet.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 .../firmware/efi/libstub/efi-stub-helper.c    | 29 +++++++++++++++++--
 drivers/firmware/efi/libstub/efistub.h        | 14 +++++----
 2 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index 5ecafc57619a..c0278a8063b7 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -11,6 +11,7 @@
 
 #include <linux/efi.h>
 #include <linux/kernel.h>
+#include <linux/printk.h> /* For CONSOLE_LOGLEVEL_* */
 #include <asm/efi.h>
 
 #include "efistub.h"
@@ -18,7 +19,7 @@
 bool efi_nochunk;
 bool efi_nokaslr;
 bool efi_noinitrd;
-bool efi_quiet;
+int efi_loglevel = CONSOLE_LOGLEVEL_DEFAULT;
 bool efi_novamap;
 
 static bool efi_nosoftreserve;
@@ -58,6 +59,28 @@ int efi_printk(const char *fmt, ...)
 	char printf_buf[256];
 	va_list args;
 	int printed;
+	int loglevel = printk_get_level(fmt);
+
+	switch (loglevel) {
+	case '0' ... '9':
+		loglevel -= '0';
+		break;
+	default:
+		/*
+		 * Use loglevel -1 for cases where we just want to print to
+		 * the screen.
+		 */
+		loglevel = -1;
+		break;
+	}
+
+	if (loglevel >= efi_loglevel)
+		return 0;
+
+	if (loglevel >= 0)
+		efi_puts("EFI stub: ");
+
+	fmt = printk_skip_level(fmt);
 
 	va_start(args, fmt);
 	printed = vsnprintf(printf_buf, sizeof(printf_buf), fmt, args);
@@ -100,7 +123,9 @@ efi_status_t efi_parse_options(char const *cmdline)
 		if (!strcmp(param, "nokaslr")) {
 			efi_nokaslr = true;
 		} else if (!strcmp(param, "quiet")) {
-			efi_quiet = true;
+			efi_loglevel = CONSOLE_LOGLEVEL_QUIET;
+		} else if (!strcmp(param, "debug")) {
+			efi_loglevel = CONSOLE_LOGLEVEL_DEBUG;
 		} else if (!strcmp(param, "noinitrd")) {
 			efi_noinitrd = true;
 		} else if (!strcmp(param, "efi") && val) {
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index caa7dcc71c69..3a323a009836 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -6,6 +6,7 @@
 #include <linux/compiler.h>
 #include <linux/efi.h>
 #include <linux/kernel.h>
+#include <linux/kern_levels.h>
 #include <linux/types.h>
 #include <asm/efi.h>
 
@@ -34,7 +35,7 @@
 extern bool efi_nochunk;
 extern bool efi_nokaslr;
 extern bool efi_noinitrd;
-extern bool efi_quiet;
+extern int efi_loglevel;
 extern bool efi_novamap;
 
 extern const efi_system_table_t *efi_system_table;
@@ -49,11 +50,12 @@ extern const efi_system_table_t *efi_system_table;
 
 #endif
 
-#define efi_info(msg)		do {			\
-	if (!efi_quiet) efi_puts("EFI stub: "msg);	\
-} while (0)
-
-#define efi_err(msg) efi_puts("EFI stub: ERROR: "msg)
+#define efi_info(fmt, ...) \
+	efi_printk(KERN_INFO fmt, ##__VA_ARGS__)
+#define efi_err(fmt, ...) \
+	efi_printk(KERN_ERR "ERROR: " fmt, ##__VA_ARGS__)
+#define efi_debug(fmt, ...) \
+	efi_printk(KERN_DEBUG "DEBUG: " fmt, ##__VA_ARGS__)
 
 /* Helper macros for the usual case of using simple C variables: */
 #ifndef fdt_setprop_inplace_var
-- 
2.26.2

[PATCH 18/24] efi/libstub: Add definitions for console input and events

[Arvind Sankar]

Add the required typedefs etc for using con_in's simple text input
protocol, and for using the boottime event services.

Also add the prototype for the "stall" boot service.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 arch/x86/include/asm/efi.h             | 10 ++++
 arch/x86/xen/efi.c                     |  2 +-
 drivers/firmware/efi/libstub/efistub.h | 77 ++++++++++++++++++++++++--
 include/linux/efi.h                    |  3 +-
 4 files changed, 85 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 6b9ab0d8b2a7..89dcc7aa7e2c 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -9,6 +9,7 @@
 #include <asm/nospec-branch.h>
 #include <asm/mmu_context.h>
 #include <linux/build_bug.h>
+#include <linux/kernel.h>
 
 extern unsigned long efi_fw_vendor, efi_config_table;
 
@@ -293,6 +294,15 @@ static inline u32 efi64_convert_status(efi_status_t status)
 #define __efi64_argmap_allocate_pool(type, size, buffer)		\
 	((type), (size), efi64_zero_upper(buffer))
 
+#define __efi64_argmap_create_event(type, tpl, f, c, event)		\
+	((type), (tpl), (f), (c), efi64_zero_upper(event))
+
+#define __efi64_argmap_set_timer(event, type, time)			\
+	((event), (type), lower_32_bits(time), upper_32_bits(time))
+
+#define __efi64_argmap_wait_for_event(num, event, index)		\
+	((num), (event), efi64_zero_upper(index))
+
 #define __efi64_argmap_handle_protocol(handle, protocol, interface)	\
 	((handle), (protocol), efi64_zero_upper(interface))
 
diff --git a/arch/x86/xen/efi.c b/arch/x86/xen/efi.c
index 1abe455d926a..205a9bc981b0 100644
--- a/arch/x86/xen/efi.c
+++ b/arch/x86/xen/efi.c
@@ -29,7 +29,7 @@ static efi_system_table_t efi_systab_xen __initdata = {
 	.fw_vendor	= EFI_INVALID_TABLE_ADDR, /* Initialized later. */
 	.fw_revision	= 0,			  /* Initialized later. */
 	.con_in_handle	= EFI_INVALID_TABLE_ADDR, /* Not used under Xen. */
-	.con_in		= EFI_INVALID_TABLE_ADDR, /* Not used under Xen. */
+	.con_in		= NULL,			  /* Not used under Xen. */
 	.con_out_handle	= EFI_INVALID_TABLE_ADDR, /* Not used under Xen. */
 	.con_out	= NULL, 		  /* Not used under Xen. */
 	.stderr_handle	= EFI_INVALID_TABLE_ADDR, /* Not used under Xen. */
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index 3a323a009836..c7c03099367f 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -111,6 +111,16 @@ void efi_set_u64_split(u64 data, u32 *lo, u32 *hi)
 #define EFI_LOCATE_BY_REGISTER_NOTIFY		1
 #define EFI_LOCATE_BY_PROTOCOL			2
 
+/*
+ * boottime->stall takes the time period in microseconds
+ */
+#define EFI_USEC_PER_SEC		1000000
+
+/*
+ * boottime->set_timer takes the time in 100ns units
+ */
+#define EFI_100NSEC_PER_USEC	((u64)10)
+
 struct efi_boot_memmap {
 	efi_memory_desc_t	**map;
 	unsigned long		*map_size;
@@ -122,6 +132,39 @@ struct efi_boot_memmap {
 
 typedef struct efi_generic_dev_path efi_device_path_protocol_t;
 
+typedef void *efi_event_t;
+/* Note that notifications won't work in mixed mode */
+typedef void (__efiapi *efi_event_notify_t)(efi_event_t, void *);
+
+#define EFI_EVT_TIMER		0x80000000U
+#define EFI_EVT_RUNTIME		0x40000000U
+#define EFI_EVT_NOTIFY_WAIT	0x00000100U
+#define EFI_EVT_NOTIFY_SIGNAL	0x00000200U
+
+/*
+ * boottime->wait_for_event takes an array of events as input.
+ * Provide a helper to set it up correctly for mixed mode.
+ */
+static inline
+void efi_set_event_at(efi_event_t *events, size_t idx, efi_event_t event)
+{
+	if (efi_is_native())
+		events[idx] = event;
+	else
+		((u32 *)events)[idx] = (u32)(unsigned long)event;
+}
+
+#define EFI_TPL_APPLICATION	4
+#define EFI_TPL_CALLBACK	8
+#define EFI_TPL_NOTIFY		16
+#define EFI_TPL_HIGH_LEVEL	31
+
+typedef enum {
+	EfiTimerCancel,
+	EfiTimerPeriodic,
+	EfiTimerRelative
+} EFI_TIMER_DELAY;
+
 /*
  * EFI Boot Services table
  */
@@ -140,11 +183,16 @@ union efi_boot_services {
 		efi_status_t (__efiapi *allocate_pool)(int, unsigned long,
 						       void **);
 		efi_status_t (__efiapi *free_pool)(void *);
-		void *create_event;
-		void *set_timer;
-		void *wait_for_event;
+		efi_status_t (__efiapi *create_event)(u32, unsigned long,
+						      efi_event_notify_t, void *,
+						      efi_event_t *);
+		efi_status_t (__efiapi *set_timer)(efi_event_t,
+						  EFI_TIMER_DELAY, u64);
+		efi_status_t (__efiapi *wait_for_event)(unsigned long,
+							efi_event_t *,
+							unsigned long *);
 		void *signal_event;
-		void *close_event;
+		efi_status_t (__efiapi *close_event)(efi_event_t);
 		void *check_event;
 		void *install_protocol_interface;
 		void *reinstall_protocol_interface;
@@ -171,7 +219,7 @@ union efi_boot_services {
 		efi_status_t (__efiapi *exit_boot_services)(efi_handle_t,
 							    unsigned long);
 		void *get_next_monotonic_count;
-		void *stall;
+		efi_status_t (__efiapi *stall)(unsigned long);
 		void *set_watchdog_timer;
 		void *connect_controller;
 		efi_status_t (__efiapi *disconnect_controller)(efi_handle_t,
@@ -256,6 +304,25 @@ union efi_uga_draw_protocol {
 	} mixed_mode;
 };
 
+typedef struct {
+	u16 scan_code;
+	efi_char16_t unicode_char;
+} efi_input_key_t;
+
+union efi_simple_text_input_protocol {
+	struct {
+		void *reset;
+		efi_status_t (__efiapi *read_keystroke)(efi_simple_text_input_protocol_t *,
+							efi_input_key_t *);
+		efi_event_t wait_for_key;
+	};
+	struct {
+		u32 reset;
+		u32 read_keystroke;
+		u32 wait_for_key;
+	} mixed_mode;
+};
+
 union efi_simple_text_output_protocol {
 	struct {
 		void *reset;
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 9b7c7ec319ac..974648db0c68 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -426,6 +426,7 @@ typedef struct {
 	u32 tables;
 } efi_system_table_32_t;
 
+typedef union efi_simple_text_input_protocol efi_simple_text_input_protocol_t;
 typedef union efi_simple_text_output_protocol efi_simple_text_output_protocol_t;
 
 typedef union {
@@ -434,7 +435,7 @@ typedef union {
 		unsigned long fw_vendor;	/* physical addr of CHAR16 vendor string */
 		u32 fw_revision;
 		unsigned long con_in_handle;
-		unsigned long con_in;
+		efi_simple_text_input_protocol_t *con_in;
 		unsigned long con_out_handle;
 		efi_simple_text_output_protocol_t *con_out;
 		unsigned long stderr_handle;
-- 
2.26.2

[PATCH 19/24] efi/gop: Add an option to list out the available GOP modes

[Arvind Sankar]

Add video=efifb:list option to list the modes that are available.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 Documentation/fb/efifb.rst                    |  5 +
 .../firmware/efi/libstub/efi-stub-helper.c    | 35 +++++++
 drivers/firmware/efi/libstub/efistub.h        |  2 +
 drivers/firmware/efi/libstub/gop.c            | 97 ++++++++++++++++++-
 include/linux/efi.h                           |  1 +
 5 files changed, 139 insertions(+), 1 deletion(-)

diff --git a/Documentation/fb/efifb.rst b/Documentation/fb/efifb.rst
index 519550517fd4..6badff64756f 100644
--- a/Documentation/fb/efifb.rst
+++ b/Documentation/fb/efifb.rst
@@ -63,4 +63,9 @@ auto
         with the highest resolution, it will choose one with the highest color
         depth.
 
+list
+        The EFI stub will list out all the display modes that are available. A
+        specific mode can then be chosen using one of the above options for the
+        next boot.
+
 Edgar Hucek <gimli@dark-green.com>
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index c0278a8063b7..a36f3af6e130 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -463,3 +463,38 @@ efi_status_t efi_load_initrd(efi_loaded_image_t *image,
 
 	return status;
 }
+
+efi_status_t efi_wait_for_key(unsigned long usec, efi_input_key_t *key)
+{
+	efi_event_t events[2], timer;
+	unsigned long index;
+	efi_simple_text_input_protocol_t *con_in;
+	efi_status_t status;
+
+	con_in = efi_table_attr(efi_system_table, con_in);
+	if (!con_in)
+		return EFI_UNSUPPORTED;
+	efi_set_event_at(events, 0, efi_table_attr(con_in, wait_for_key));
+
+	status = efi_bs_call(create_event, EFI_EVT_TIMER, 0, NULL, NULL, &timer);
+	if (status != EFI_SUCCESS)
+		return status;
+
+	status = efi_bs_call(set_timer, timer, EfiTimerRelative,
+			     EFI_100NSEC_PER_USEC * usec);
+	if (status != EFI_SUCCESS)
+		return status;
+	efi_set_event_at(events, 1, timer);
+
+	status = efi_bs_call(wait_for_event, 2, events, &index);
+	if (status == EFI_SUCCESS) {
+		if (index == 0)
+			status = efi_call_proto(con_in, read_keystroke, key);
+		else
+			status = EFI_TIMEOUT;
+	}
+
+	efi_bs_call(close_event, timer);
+
+	return status;
+}
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index c7c03099367f..ad7e0406d0ba 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -323,6 +323,8 @@ union efi_simple_text_input_protocol {
 	} mixed_mode;
 };
 
+efi_status_t efi_wait_for_key(unsigned long usec, efi_input_key_t *key);
+
 union efi_simple_text_output_protocol {
 	struct {
 		void *reset;
diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c
index 34c0cba2c8bf..ea5da307d542 100644
--- a/drivers/firmware/efi/libstub/gop.c
+++ b/drivers/firmware/efi/libstub/gop.c
@@ -19,7 +19,8 @@ enum efi_cmdline_option {
 	EFI_CMDLINE_NONE,
 	EFI_CMDLINE_MODE_NUM,
 	EFI_CMDLINE_RES,
-	EFI_CMDLINE_AUTO
+	EFI_CMDLINE_AUTO,
+	EFI_CMDLINE_LIST
 };
 
 static struct {
@@ -100,6 +101,19 @@ static bool parse_auto(char *option, char **next)
 	return true;
 }
 
+static bool parse_list(char *option, char **next)
+{
+	if (!strstarts(option, "list"))
+		return false;
+	option += strlen("list");
+	if (*option && *option++ != ',')
+		return false;
+	cmdline.option = EFI_CMDLINE_LIST;
+
+	*next = option;
+	return true;
+}
+
 void efi_parse_option_graphics(char *option)
 {
 	while (*option) {
@@ -109,6 +123,8 @@ void efi_parse_option_graphics(char *option)
 			continue;
 		if (parse_auto(option, &option))
 			continue;
+		if (parse_list(option, &option))
+			continue;
 
 		while (*option && *option++ != ',')
 			;
@@ -290,6 +306,82 @@ static u32 choose_mode_auto(efi_graphics_output_protocol_t *gop)
 	return best_mode;
 }
 
+static u32 choose_mode_list(efi_graphics_output_protocol_t *gop)
+{
+	efi_status_t status;
+
+	efi_graphics_output_protocol_mode_t *mode;
+	efi_graphics_output_mode_info_t *info;
+	unsigned long info_size;
+
+	u32 max_mode, cur_mode;
+	int pf;
+	efi_pixel_bitmask_t pi;
+	u32 m, w, h;
+	u8 d;
+	const char *dstr;
+	bool valid;
+	efi_input_key_t key;
+
+	mode = efi_table_attr(gop, mode);
+
+	cur_mode = efi_table_attr(mode, mode);
+	max_mode = efi_table_attr(mode, max_mode);
+
+	efi_printk("Available graphics modes are 0-%u\n", max_mode-1);
+	efi_puts("  * = current mode\n"
+		 "  - = unusable mode\n");
+	for (m = 0; m < max_mode; m++) {
+		status = efi_call_proto(gop, query_mode, m,
+					&info_size, &info);
+		if (status != EFI_SUCCESS)
+			continue;
+
+		pf = info->pixel_format;
+		pi = info->pixel_information;
+		w  = info->horizontal_resolution;
+		h  = info->vertical_resolution;
+
+		efi_bs_call(free_pool, info);
+
+		valid = !(pf == PIXEL_BLT_ONLY || pf >= PIXEL_FORMAT_MAX);
+		d = 0;
+		switch (pf) {
+		case PIXEL_RGB_RESERVED_8BIT_PER_COLOR:
+			dstr = "rgb";
+			break;
+		case PIXEL_BGR_RESERVED_8BIT_PER_COLOR:
+			dstr = "bgr";
+			break;
+		case PIXEL_BIT_MASK:
+			dstr = "";
+			d = pixel_bpp(pf, pi);
+			break;
+		case PIXEL_BLT_ONLY:
+			dstr = "blt";
+			break;
+		default:
+			dstr = "xxx";
+			break;
+		}
+
+		efi_printk("Mode %3u %c%c: Resolution %ux%u-%s%.0hhu\n",
+			   m,
+			   m == cur_mode ? '*' : ' ',
+			   !valid ? '-' : ' ',
+			   w, h, dstr, d);
+	}
+
+	efi_puts("\nPress any key to continue (or wait 10 seconds)\n");
+	status = efi_wait_for_key(10 * EFI_USEC_PER_SEC, &key);
+	if (status != EFI_SUCCESS && status != EFI_TIMEOUT) {
+		efi_err("Unable to read key, continuing in 10 seconds\n");
+		efi_bs_call(stall, 10 * EFI_USEC_PER_SEC);
+	}
+
+	return cur_mode;
+}
+
 static void set_mode(efi_graphics_output_protocol_t *gop)
 {
 	efi_graphics_output_protocol_mode_t *mode;
@@ -305,6 +397,9 @@ static void set_mode(efi_graphics_output_protocol_t *gop)
 	case EFI_CMDLINE_AUTO:
 		new_mode = choose_mode_auto(gop);
 		break;
+	case EFI_CMDLINE_LIST:
+		new_mode = choose_mode_list(gop);
+		break;
 	default:
 		return;
 	}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 974648db0c68..609201bd4682 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -39,6 +39,7 @@
 #define EFI_WRITE_PROTECTED	( 8 | (1UL << (BITS_PER_LONG-1)))
 #define EFI_OUT_OF_RESOURCES	( 9 | (1UL << (BITS_PER_LONG-1)))
 #define EFI_NOT_FOUND		(14 | (1UL << (BITS_PER_LONG-1)))
+#define EFI_TIMEOUT		(18 | (1UL << (BITS_PER_LONG-1)))
 #define EFI_ABORTED		(21 | (1UL << (BITS_PER_LONG-1)))
 #define EFI_SECURITY_VIOLATION	(26 | (1UL << (BITS_PER_LONG-1)))
 
-- 
2.26.2

[PATCH 20/24] efi/printf: Add support for wchar_t (UTF-16)

[Arvind Sankar]

Support %lc and %ls to output UTF-16 strings (converted to UTF-8).

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 drivers/firmware/efi/libstub/vsprintf.c | 111 ++++++++++++++++++++++--
 1 file changed, 106 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/efi/libstub/vsprintf.c b/drivers/firmware/efi/libstub/vsprintf.c
index 36f2f4cf7434..136ec18e2f46 100644
--- a/drivers/firmware/efi/libstub/vsprintf.c
+++ b/drivers/firmware/efi/libstub/vsprintf.c
@@ -147,6 +147,7 @@ char *number(char *end, unsigned long long num, int base, char locase)
 #define LEFT	16		/* left justified */
 #define SMALL	32		/* Must be 32 == 0x20 */
 #define SPECIAL	64		/* 0x */
+#define WIDE	128		/* UTF-16 string */
 
 static
 int get_flags(const char **fmt)
@@ -238,6 +239,58 @@ char get_sign(long long *num, int flags)
 	return 0;
 }
 
+static
+size_t utf16s_utf8nlen(const u16 *s16, size_t maxlen)
+{
+	size_t len, clen;
+
+	for (len = 0; len < maxlen && *s16; len += clen) {
+		u16 c0 = *s16++;
+
+		/* First, get the length for a BMP character */
+		clen = 1 + (c0 >= 0x80) + (c0 >= 0x800);
+		if (len + clen > maxlen)
+			break;
+		/*
+		 * If this is a high surrogate, and we're already at maxlen, we
+		 * can't include the character if it's a valid surrogate pair.
+		 * Avoid accessing one extra word just to check if it's valid
+		 * or not.
+		 */
+		if ((c0 & 0xfc00) == 0xd800) {
+			if (len + clen == maxlen)
+				break;
+			if ((*s16 & 0xfc00) == 0xdc00) {
+				++s16;
+				++clen;
+			}
+		}
+	}
+
+	return len;
+}
+
+static
+u32 utf16_to_utf32(const u16 **s16)
+{
+	u16 c0, c1;
+
+	c0 = *(*s16)++;
+	/* not a surrogate */
+	if ((c0 & 0xf800) != 0xd800)
+		return c0;
+	/* invalid: low surrogate instead of high */
+	if (c0 & 0x0400)
+		return 0xfffd;
+	c1 = **s16;
+	/* invalid: missing low surrogate */
+	if ((c1 & 0xfc00) != 0xdc00)
+		return 0xfffd;
+	/* valid surrogate pair */
+	++(*s16);
+	return (0x10000 - (0xd800 << 10) - 0xdc00) + (c0 << 10) + c1;
+}
+
 #define PUTC(c) \
 do {				\
 	if (pos < size)		\
@@ -325,18 +378,31 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list ap)
 		switch (*fmt) {
 		case 'c':
 			flags &= LEFT;
-			tmp[0] = (unsigned char)va_arg(args, int);
 			s = tmp;
-			precision = len = 1;
+			if (qualifier == 'l') {
+				((u16 *)tmp)[0] = (u16)va_arg(args, unsigned int);
+				((u16 *)tmp)[1] = L'\0';
+				precision = INT_MAX;
+				goto wstring;
+			} else {
+				tmp[0] = (unsigned char)va_arg(args, int);
+				precision = len = 1;
+			}
 			goto output;
 
 		case 's':
 			flags &= LEFT;
 			if (precision < 0)
 				precision = INT_MAX;
-			s = va_arg(args, char *);
+			s = va_arg(args, void *);
 			if (!s)
 				s = precision < 6 ? "" : "(null)";
+			else if (qualifier == 'l') {
+		wstring:
+				flags |= WIDE;
+				precision = len = utf16s_utf8nlen((const u16 *)s, precision);
+				goto output;
+			}
 			precision = len = strnlen(s, precision);
 			goto output;
 
@@ -435,8 +501,43 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list ap)
 		while (precision-- > len)
 			PUTC('0');
 		/* Actual output */
-		while (len-- > 0)
-			PUTC(*s++);
+		if (flags & WIDE) {
+			const u16 *ws = (const u16 *)s;
+
+			while (len-- > 0) {
+				u32 c32 = utf16_to_utf32(&ws);
+				u8 *s8;
+				size_t clen;
+
+				if (c32 < 0x80) {
+					PUTC(c32);
+					continue;
+				}
+
+				/* Number of trailing octets */
+				clen = 1 + (c32 >= 0x800) + (c32 >= 0x10000);
+
+				len -= clen;
+				s8 = (u8 *)&buf[pos];
+
+				/* Avoid writing partial character */
+				PUTC('\0');
+				pos += clen;
+				if (pos >= size)
+					continue;
+
+				/* Set high bits of leading octet */
+				*s8 = (0xf00 >> 1) >> clen;
+				/* Write trailing octets in reverse order */
+				for (s8 += clen; clen; --clen, c32 >>= 6)
+					*s8-- = 0x80 | (c32 & 0x3f);
+				/* Set low bits of leading octet */
+				*s8 |= c32;
+			}
+		} else {
+			while (len-- > 0)
+				PUTC(*s++);
+		}
 		/* Trailing padding with ' ' */
 		while (field_width-- > 0)
 			PUTC(' ');
-- 
2.26.2

[PATCH 21/24] efi/libstub: Add UTF-8 decoding to efi_puts

[Arvind Sankar]

In order to be able to use the UTF-16 support added to vsprintf in the
previous commit, enhance efi_puts to decode UTF-8 into UTF-16. Invalid
UTF-8 encodings are passed through unchanged.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 .../firmware/efi/libstub/efi-stub-helper.c    | 67 +++++++++++++++++--
 1 file changed, 62 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index a36f3af6e130..48242bc982a3 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -36,17 +36,74 @@ void efi_char16_puts(efi_char16_t *str)
 		       output_string, str);
 }
 
+static
+u32 utf8_to_utf32(const u8 **s8)
+{
+	u32 c32;
+	u8 c0, cx;
+	size_t clen, i;
+
+	c0 = cx = *(*s8)++;
+	/*
+	 * The position of the most-significant 0 bit gives us the length of
+	 * a multi-octet encoding.
+	 */
+	for (clen = 0; cx & 0x80; ++clen)
+		cx <<= 1;
+	/*
+	 * If the 0 bit is in position 8, this is a valid single-octet
+	 * encoding. If the 0 bit is in position 7 or positions 1-3, the
+	 * encoding is invalid.
+	 * In either case, we just return the first octet.
+	 */
+	if (clen < 2 || clen > 4)
+		return c0;
+	/* Get the bits from the first octet. */
+	c32 = cx >> clen--;
+	for (i = 0; i < clen; ++i) {
+		/* Trailing octets must have 10 in most significant bits. */
+		cx = (*s8)[i] ^ 0x80;
+		if (cx & 0xc0)
+			return c0;
+		c32 = (c32 << 6) | cx;
+	}
+	/*
+	 * Check for validity:
+	 * - The character must be in the Unicode range.
+	 * - It must not be a surrogate.
+	 * - It must be encoded using the correct number of octets.
+	 */
+	if (c32 > 0x10ffff ||
+	    (c32 & 0xf800) == 0xd800 ||
+	    clen != (c32 >= 0x80) + (c32 >= 0x800) + (c32 >= 0x10000))
+		return c0;
+	*s8 += clen;
+	return c32;
+}
+
 void efi_puts(const char *str)
 {
 	efi_char16_t buf[128];
 	size_t pos = 0, lim = ARRAY_SIZE(buf);
+	const u8 *s8 = (const u8 *)str;
+	u32 c32;
 
-	while (*str) {
-		if (*str == '\n')
+	while (*s8) {
+		if (*s8 == '\n')
 			buf[pos++] = L'\r';
-		/* Cast to unsigned char to avoid sign-extension */
-		buf[pos++] = (unsigned char)(*str++);
-		if (*str == '\0' || pos >= lim - 2) {
+		c32 = utf8_to_utf32(&s8);
+		if (c32 < 0x10000)
+			/* Characters in plane 0 use a single word. */
+			buf[pos++] = c32;
+		else {
+			/*
+			 * Characters in other planes encode into a surrogate
+			 * pair.
+			 */
+			buf[pos++] = (0xd800 - (0x10000 >> 10)) + (c32 >> 10);
+			buf[pos++] = 0xdc00 + (c32 & 0x3ff);
+		}
+		if (*s8 == '\0' || pos >= lim - 2) {
 			buf[pos] = L'\0';
 			efi_char16_puts(buf);
 			pos = 0;
-- 
2.26.2

[PATCH 22/24] efi/libstub: Use %ls for filename

[Arvind Sankar]

efi_printk can now handle the UTF-16 filename, so print it using efi_err
instead of a separate efi_char16_puts call.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 drivers/firmware/efi/libstub/file.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/firmware/efi/libstub/file.c b/drivers/firmware/efi/libstub/file.c
index 933536c5236a..2005e33b33d5 100644
--- a/drivers/firmware/efi/libstub/file.c
+++ b/drivers/firmware/efi/libstub/file.c
@@ -46,9 +46,7 @@ static efi_status_t efi_open_file(efi_file_protocol_t *volume,
 
 	status = volume->open(volume, &fh, fi->filename, EFI_FILE_MODE_READ, 0);
 	if (status != EFI_SUCCESS) {
-		efi_err("Failed to open file: ");
-		efi_char16_puts(fi->filename);
-		efi_puts("\n");
+		efi_err("Failed to open file: %ls\n", fi->filename);
 		return status;
 	}
 
-- 
2.26.2

[PATCH 23/24] efi/libstub: Get the exact UTF-8 length

[Arvind Sankar]

efi_convert_cmdline currently overestimates the length of the equivalent
UTF-8 encoding.

snprintf can now be used to do the conversion to UTF-8, however, it does
not have a way to specify the size of the UTF-16 string, only the size
of the resulting UTF-8 string. So in order to use it, we need to
precalculate the exact UTF-8 size.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 .../firmware/efi/libstub/efi-stub-helper.c    | 44 ++++++++++++++-----
 1 file changed, 32 insertions(+), 12 deletions(-)

diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index 48242bc982a3..01476d8f8edf 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -205,15 +205,6 @@ efi_status_t efi_parse_options(char const *cmdline)
 	return EFI_SUCCESS;
 }
 
-/*
- * Get the number of UTF-8 bytes corresponding to an UTF-16 character.
- * This overestimates for surrogates, but that is okay.
- */
-static int efi_utf8_bytes(u16 c)
-{
-	return 1 + (c >= 0x80) + (c >= 0x800);
-}
-
 /*
  * Convert an UTF-16 string, not necessarily null terminated, to UTF-8.
  */
@@ -274,10 +265,39 @@ char *efi_convert_cmdline(efi_loaded_image_t *image,
 
 	if (options) {
 		s2 = options;
-		while (*s2 && *s2 != '\n'
-		       && options_chars < load_options_chars) {
-			options_bytes += efi_utf8_bytes(*s2++);
+		while (options_chars < load_options_chars) {
+			u16 c = *s2++;
+
+			if (c == L'\0' || c == L'\n')
+				break;
+			/*
+			 * Get the number of UTF-8 bytes corresponding to a
+			 * UTF-16 character.
+			 * The first part handles everything in the BMP.
+			 */
+			options_bytes += 1 + (c >= 0x80) + (c >= 0x800);
 			options_chars++;
+			/*
+			 * Add one more byte for valid surrogate pairs. Invalid
+			 * surrogates will be replaced with 0xfffd and take up
+			 * only 3 bytes.
+			 */
+			if ((c & 0xfc00) == 0xd800) {
+				/*
+				 * If the very last word is a high surrogate,
+				 * we must ignore it since we can't access the
+				 * low surrogate.
+				 */
+				if (options_chars == load_options_chars) {
+					options_bytes -= 3;
+					options_chars--;
+					break;
+				} else if ((*s2 & 0xfc00) == 0xdc00) {
+					options_bytes++;
+					options_chars++;
+					s2++;
+				}
+			}
 		}
 	}
 
-- 
2.26.2

[PATCH 24/24] efi/libstub: Use snprintf with %ls to convert the command line

[Arvind Sankar]

Now we can use snprintf to do the UTF-16 to UTF-8 translation for the
command line.

Drop the special "zero" trick to handle an empty command line. This was
unnecessary even before this since with options_chars == 0,
efi_utf16_to_utf8 would not have accessed options at all. snprintf won't
access it either with a precision of 0.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 .../firmware/efi/libstub/efi-stub-helper.c    | 68 ++-----------------
 1 file changed, 7 insertions(+), 61 deletions(-)

diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index 01476d8f8edf..e3324f7b38b8 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -205,46 +205,6 @@ efi_status_t efi_parse_options(char const *cmdline)
 	return EFI_SUCCESS;
 }
 
-/*
- * Convert an UTF-16 string, not necessarily null terminated, to UTF-8.
- */
-static u8 *efi_utf16_to_utf8(u8 *dst, const u16 *src, int n)
-{
-	unsigned int c;
-
-	while (n--) {
-		c = *src++;
-		if (n && c >= 0xd800 && c <= 0xdbff &&
-		    *src >= 0xdc00 && *src <= 0xdfff) {
-			c = 0x10000 + ((c & 0x3ff) << 10) + (*src & 0x3ff);
-			src++;
-			n--;
-		}
-		if (c >= 0xd800 && c <= 0xdfff)
-			c = 0xfffd; /* Unmatched surrogate */
-		if (c < 0x80) {
-			*dst++ = c;
-			continue;
-		}
-		if (c < 0x800) {
-			*dst++ = 0xc0 + (c >> 6);
-			goto t1;
-		}
-		if (c < 0x10000) {
-			*dst++ = 0xe0 + (c >> 12);
-			goto t2;
-		}
-		*dst++ = 0xf0 + (c >> 18);
-		*dst++ = 0x80 + ((c >> 12) & 0x3f);
-	t2:
-		*dst++ = 0x80 + ((c >> 6) & 0x3f);
-	t1:
-		*dst++ = 0x80 + (c & 0x3f);
-	}
-
-	return dst;
-}
-
 /*
  * Convert the unicode UEFI command line to ASCII to pass to kernel.
  * Size of memory allocated return in *cmd_line_len.
@@ -254,18 +214,15 @@ char *efi_convert_cmdline(efi_loaded_image_t *image,
 			  int *cmd_line_len, unsigned long max_addr)
 {
 	const u16 *s2;
-	u8 *s1 = NULL;
 	unsigned long cmdline_addr = 0;
-	int load_options_chars = efi_table_attr(image, load_options_size) / 2;
+	int options_chars = efi_table_attr(image, load_options_size) / 2;
 	const u16 *options = efi_table_attr(image, load_options);
 	int options_bytes = 0;  /* UTF-8 bytes */
-	int options_chars = 0;  /* UTF-16 chars */
 	efi_status_t status;
-	u16 zero = 0;
 
 	if (options) {
 		s2 = options;
-		while (options_chars < load_options_chars) {
+		while (options_chars--) {
 			u16 c = *s2++;
 
 			if (c == L'\0' || c == L'\n')
@@ -276,7 +233,6 @@ char *efi_convert_cmdline(efi_loaded_image_t *image,
 			 * The first part handles everything in the BMP.
 			 */
 			options_bytes += 1 + (c >= 0x80) + (c >= 0x800);
-			options_chars++;
 			/*
 			 * Add one more byte for valid surrogate pairs. Invalid
 			 * surrogates will be replaced with 0xfffd and take up
@@ -288,35 +244,25 @@ char *efi_convert_cmdline(efi_loaded_image_t *image,
 				 * we must ignore it since we can't access the
 				 * low surrogate.
 				 */
-				if (options_chars == load_options_chars) {
+				if (!options_chars)
 					options_bytes -= 3;
-					options_chars--;
-					break;
-				} else if ((*s2 & 0xfc00) == 0xdc00) {
+				else if ((*s2 & 0xfc00) == 0xdc00) {
 					options_bytes++;
-					options_chars++;
+					options_chars--;
 					s2++;
 				}
 			}
 		}
 	}
 
-	if (!options_chars) {
-		/* No command line options, so return empty string*/
-		options = &zero;
-	}
-
 	options_bytes++;	/* NUL termination */
 
 	status = efi_allocate_pages(options_bytes, &cmdline_addr, max_addr);
 	if (status != EFI_SUCCESS)
 		return NULL;
 
-	s1 = (u8 *)cmdline_addr;
-	s2 = (const u16 *)options;
-
-	s1 = efi_utf16_to_utf8(s1, s2, options_chars);
-	*s1 = '\0';
+	snprintf((char *)cmdline_addr, options_bytes,
+		 "%.*ls", options_bytes-1, options);
 
 	*cmd_line_len = options_bytes;
 	return (char *)cmdline_addr;
-- 
2.26.2

Re: [PATCH 00/24] efi/libstub: Add printf implementation

[Ard Biesheuvel]

On Mon, 18 May 2020 at 21:07, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> This series (on top of efi/next) adds a basic printf implementation for
> the EFI stub to use.
>
> Patches 1-2 are minor cleanup.
>
> Patch 3 adds an on-stack buffer for efi_puts to do the char to UTF-16
> conversion, to avoid calling into the firmware for each character.
>
> Patches 4-16 copy the simple printf implementation from
> arch/x86/boot/printf.c and spruce it up. The main enhancements are
> support for 64-bit integers and conversion to vsnprintf. There are small
> fixes to correct the behavior for edge cases (detailed in the individual
> patches).
>
> Patch 17 changes efi_info/efi_err to actually use efi_printk, and
> introduces a loglevel into the EFI stub instead of just quiet/non-quiet.
> The loglevels are reused from the main kernel. An efi_debug macro is
> also added, but is currently unused. As an aside, we chose efi_info over
> efi_pr_info, but I'm wondering whether we should make it efi_pr_info
> after all: there is already an efi_info symbol (structure type and field
> name) so it might lead to some confusion.
>
> Patch 18-19 add an option to list the available modes from the GOP,
> which should make using the new video=efifb parameters easier.
>
> Patches 20-24 incorporate UTF-16 support into the new printf function
> and use it in a couple of places.
>
> Arvind Sankar (24):
>   efi/libstub: Include dependencies of efistub.h
>   efi/libstub: Rename efi_[char16_]printk to efi_[char16_]puts
>   efi/libstub: Buffer output of efi_puts
>   efi/libstub: Add a basic printf implementation
>   efi/libstub: Optimize for size instead of speed
>   efi/printf: Drop %n format and L qualifier
>   efi/printf: Add 64-bit and 8-bit integer support
>   efi/printf: Factor out flags parsing and handle '%' earlier
>   efi/printf: Fix minor bug in precision handling
>   efi/printf: Merge 'p' with the integer formats
>   efi/printf: Factor out width/precision parsing
>   efi/printf: Factor out integer argument retrieval
>   efi/printf: Handle null string input
>   efi/printf: Refactor code to consolidate padding and output
>   efi/printf: Abort on invalid format
>   efi/printf: Turn vsprintf into vsnprintf
>   efi/libstub: Implement printk-style logging
>   efi/libstub: Add definitions for console input and events
>   efi/gop: Add an option to list out the available GOP modes
>   efi/printf: Add support for wchar_t (UTF-16)
>   efi/libstub: Add UTF-8 decoding to efi_puts
>   efi/libstub: Use %ls for filename
>   efi/libstub: Get the exact UTF-8 length
>   efi/libstub: Use snprintf with %ls to convert the command line
>

Thanks Arvind, this is looking really good!

Did you use any test code for the printf() parsing? Given that the
kernel command line is not covered by secure boot signing (or the
initrd, come to think of it), I'd hate to open up a security hole
here.



>  Documentation/fb/efifb.rst                    |   5 +
>  arch/x86/include/asm/efi.h                    |  10 +
>  arch/x86/xen/efi.c                            |   2 +-
>  drivers/firmware/efi/libstub/Makefile         |   6 +-
>  .../firmware/efi/libstub/efi-stub-helper.c    | 275 ++++++---
>  drivers/firmware/efi/libstub/efistub.h        | 107 +++-
>  drivers/firmware/efi/libstub/file.c           |   4 +-
>  drivers/firmware/efi/libstub/gop.c            |  97 ++-
>  drivers/firmware/efi/libstub/vsprintf.c       | 563 ++++++++++++++++++
>  include/linux/efi.h                           |   4 +-
>  10 files changed, 968 insertions(+), 105 deletions(-)
>  create mode 100644 drivers/firmware/efi/libstub/vsprintf.c
>
> --
> 2.26.2
>

Re: [PATCH 17/24] efi/libstub: Implement printk-style logging

[Ard Biesheuvel]

On Mon, 18 May 2020 at 21:07, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> Use the efi_printk function in efi_info/efi_err, and add efi_debug. This
> allows formatted output at different log levels.
>
> Add the notion of a loglevel instead of just quiet/not-quiet, and
> parse the debug kernel parameter in addition to quiet.
>
> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> ---
>  .../firmware/efi/libstub/efi-stub-helper.c    | 29 +++++++++++++++++--
>  drivers/firmware/efi/libstub/efistub.h        | 14 +++++----
>  2 files changed, 35 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> index 5ecafc57619a..c0278a8063b7 100644
> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> @@ -11,6 +11,7 @@
>
>  #include <linux/efi.h>
>  #include <linux/kernel.h>
> +#include <linux/printk.h> /* For CONSOLE_LOGLEVEL_* */
>  #include <asm/efi.h>
>
>  #include "efistub.h"
> @@ -18,7 +19,7 @@
>  bool efi_nochunk;
>  bool efi_nokaslr;
>  bool efi_noinitrd;
> -bool efi_quiet;
> +int efi_loglevel = CONSOLE_LOGLEVEL_DEFAULT;
>  bool efi_novamap;
>
>  static bool efi_nosoftreserve;
> @@ -58,6 +59,28 @@ int efi_printk(const char *fmt, ...)
>         char printf_buf[256];
>         va_list args;
>         int printed;
> +       int loglevel = printk_get_level(fmt);
> +
> +       switch (loglevel) {
> +       case '0' ... '9':
> +               loglevel -= '0';
> +               break;
> +       default:
> +               /*
> +                * Use loglevel -1 for cases where we just want to print to
> +                * the screen.
> +                */
> +               loglevel = -1;
> +               break;
> +       }
> +
> +       if (loglevel >= efi_loglevel)
> +               return 0;
> +
> +       if (loglevel >= 0)
> +               efi_puts("EFI stub: ");
> +
> +       fmt = printk_skip_level(fmt);
>
>         va_start(args, fmt);
>         printed = vsnprintf(printf_buf, sizeof(printf_buf), fmt, args);
> @@ -100,7 +123,9 @@ efi_status_t efi_parse_options(char const *cmdline)
>                 if (!strcmp(param, "nokaslr")) {
>                         efi_nokaslr = true;
>                 } else if (!strcmp(param, "quiet")) {
> -                       efi_quiet = true;
> +                       efi_loglevel = CONSOLE_LOGLEVEL_QUIET;
> +               } else if (!strcmp(param, "debug")) {
> +                       efi_loglevel = CONSOLE_LOGLEVEL_DEBUG;

Should we wire this to 'efi=debug' instead?

>                 } else if (!strcmp(param, "noinitrd")) {
>                         efi_noinitrd = true;
>                 } else if (!strcmp(param, "efi") && val) {
> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> index caa7dcc71c69..3a323a009836 100644
> --- a/drivers/firmware/efi/libstub/efistub.h
> +++ b/drivers/firmware/efi/libstub/efistub.h
> @@ -6,6 +6,7 @@
>  #include <linux/compiler.h>
>  #include <linux/efi.h>
>  #include <linux/kernel.h>
> +#include <linux/kern_levels.h>
>  #include <linux/types.h>
>  #include <asm/efi.h>
>
> @@ -34,7 +35,7 @@
>  extern bool efi_nochunk;
>  extern bool efi_nokaslr;
>  extern bool efi_noinitrd;
> -extern bool efi_quiet;
> +extern int efi_loglevel;
>  extern bool efi_novamap;
>
>  extern const efi_system_table_t *efi_system_table;
> @@ -49,11 +50,12 @@ extern const efi_system_table_t *efi_system_table;
>
>  #endif
>
> -#define efi_info(msg)          do {                    \
> -       if (!efi_quiet) efi_puts("EFI stub: "msg);      \
> -} while (0)
> -
> -#define efi_err(msg) efi_puts("EFI stub: ERROR: "msg)
> +#define efi_info(fmt, ...) \
> +       efi_printk(KERN_INFO fmt, ##__VA_ARGS__)
> +#define efi_err(fmt, ...) \
> +       efi_printk(KERN_ERR "ERROR: " fmt, ##__VA_ARGS__)
> +#define efi_debug(fmt, ...) \
> +       efi_printk(KERN_DEBUG "DEBUG: " fmt, ##__VA_ARGS__)
>
>  /* Helper macros for the usual case of using simple C variables: */
>  #ifndef fdt_setprop_inplace_var
> --
> 2.26.2
>

Re: [PATCH 00/24] efi/libstub: Add printf implementation

[Arvind Sankar]

On Tue, May 19, 2020 at 09:53:47AM +0200, Ard Biesheuvel wrote:
> 
> Thanks Arvind, this is looking really good!
> 
> Did you use any test code for the printf() parsing? Given that the
> kernel command line is not covered by secure boot signing (or the
> initrd, come to think of it), I'd hate to open up a security hole
> here.
> 
I only did basic functional testing, I haven't tried to actually break
it.

I think the code will be robust enough to avoid overflowing the buffer
passed to vsnprintf, even if the output ends up being garbage due to
bugs.

That said, one thing in efi_convert_cmdline is that we use int to hold
both options_chars and options_bytes. The size of load options is
limited to uint32, so int should be ok for options_chars but
options_bytes could theoretically overflow?

In any case, there's no point parsing beyond COMMAND_LINE_SIZE anyway,
so we should limit options_bytes to COMMAND_LINE_SIZE-1 + terminating
NUL, and if it's longer we can either truncate it (blindly or at
whitespace?) or ignore the options altogether. I can add that in v2.

One more question -- since the first version of the stub, we truncate
the command line at the first newline character. Do you know if there's
something that actually needs that?

efibootmgr can actually even set up the load options as a series of
NUL-terminated strings if you miss putting them all inside quotes :)

Re: [PATCH 17/24] efi/libstub: Implement printk-style logging

[Arvind Sankar]

On Tue, May 19, 2020 at 10:22:40AM +0200, Ard Biesheuvel wrote:
> On Mon, 18 May 2020 at 21:07, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > @@ -100,7 +123,9 @@ efi_status_t efi_parse_options(char const *cmdline)
> >                 if (!strcmp(param, "nokaslr")) {
> >                         efi_nokaslr = true;
> >                 } else if (!strcmp(param, "quiet")) {
> > -                       efi_quiet = true;
> > +                       efi_loglevel = CONSOLE_LOGLEVEL_QUIET;
> > +               } else if (!strcmp(param, "debug")) {
> > +                       efi_loglevel = CONSOLE_LOGLEVEL_DEBUG;
> 
> Should we wire this to 'efi=debug' instead?
> 

Sure.

Re: [PATCH 00/24] efi/libstub: Add printf implementation

[Ard Biesheuvel]

On Tue, 19 May 2020 at 17:06, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Tue, May 19, 2020 at 09:53:47AM +0200, Ard Biesheuvel wrote:
> >
> > Thanks Arvind, this is looking really good!
> >
> > Did you use any test code for the printf() parsing? Given that the
> > kernel command line is not covered by secure boot signing (or the
> > initrd, come to think of it), I'd hate to open up a security hole
> > here.
> >
> I only did basic functional testing, I haven't tried to actually break
> it.
>
> I think the code will be robust enough to avoid overflowing the buffer
> passed to vsnprintf, even if the output ends up being garbage due to
> bugs.
>
> That said, one thing in efi_convert_cmdline is that we use int to hold
> both options_chars and options_bytes. The size of load options is
> limited to uint32, so int should be ok for options_chars but
> options_bytes could theoretically overflow?
>
> In any case, there's no point parsing beyond COMMAND_LINE_SIZE anyway,
> so we should limit options_bytes to COMMAND_LINE_SIZE-1 + terminating
> NUL, and if it's longer we can either truncate it (blindly or at
> whitespace?) or ignore the options altogether. I can add that in v2.
>

Anything that will make it more robust is good to have.

> One more question -- since the first version of the stub, we truncate
> the command line at the first newline character. Do you know if there's
> something that actually needs that?
>

Not that I am aware of.

> efibootmgr can actually even set up the load options as a series of
> NUL-terminated strings if you miss putting them all inside quotes :)

Someone else may have thought of that already, so we can't simply
start treating anything past the first newline or \0 as part of the
command line.

Re: [PATCH 17/24] efi/libstub: Implement printk-style logging

[Arvind Sankar]

On Tue, May 19, 2020 at 11:07:55AM -0400, Arvind Sankar wrote:
> On Tue, May 19, 2020 at 10:22:40AM +0200, Ard Biesheuvel wrote:
> > On Mon, 18 May 2020 at 21:07, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > > @@ -100,7 +123,9 @@ efi_status_t efi_parse_options(char const *cmdline)
> > >                 if (!strcmp(param, "nokaslr")) {
> > >                         efi_nokaslr = true;
> > >                 } else if (!strcmp(param, "quiet")) {
> > > -                       efi_quiet = true;
> > > +                       efi_loglevel = CONSOLE_LOGLEVEL_QUIET;
> > > +               } else if (!strcmp(param, "debug")) {
> > > +                       efi_loglevel = CONSOLE_LOGLEVEL_DEBUG;
> > 
> > Should we wire this to 'efi=debug' instead?
> > 
> 
> Sure.

Do you prefer it wired up to both or just efi=debug?

THanks.

Re: [PATCH 17/24] efi/libstub: Implement printk-style logging

[Ard Biesheuvel]

On Wed, 20 May 2020 at 18:38, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Tue, May 19, 2020 at 11:07:55AM -0400, Arvind Sankar wrote:
> > On Tue, May 19, 2020 at 10:22:40AM +0200, Ard Biesheuvel wrote:
> > > On Mon, 18 May 2020 at 21:07, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > > > @@ -100,7 +123,9 @@ efi_status_t efi_parse_options(char const *cmdline)
> > > >                 if (!strcmp(param, "nokaslr")) {
> > > >                         efi_nokaslr = true;
> > > >                 } else if (!strcmp(param, "quiet")) {
> > > > -                       efi_quiet = true;
> > > > +                       efi_loglevel = CONSOLE_LOGLEVEL_QUIET;
> > > > +               } else if (!strcmp(param, "debug")) {
> > > > +                       efi_loglevel = CONSOLE_LOGLEVEL_DEBUG;
> > >
> > > Should we wire this to 'efi=debug' instead?
> > >
> >
> > Sure.
>
> Do you prefer it wired up to both or just efi=debug?
>

Let's stick with efi=debug, that is what all other EFI code uses.

Re: [PATCH 17/24] efi/libstub: Implement printk-style logging

[Arvind Sankar]

On Wed, May 20, 2020 at 06:38:53PM +0200, Ard Biesheuvel wrote:
> On Wed, 20 May 2020 at 18:38, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > On Tue, May 19, 2020 at 11:07:55AM -0400, Arvind Sankar wrote:
> > > On Tue, May 19, 2020 at 10:22:40AM +0200, Ard Biesheuvel wrote:
> > > > On Mon, 18 May 2020 at 21:07, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > > > > @@ -100,7 +123,9 @@ efi_status_t efi_parse_options(char const *cmdline)
> > > > >                 if (!strcmp(param, "nokaslr")) {
> > > > >                         efi_nokaslr = true;
> > > > >                 } else if (!strcmp(param, "quiet")) {
> > > > > -                       efi_quiet = true;
> > > > > +                       efi_loglevel = CONSOLE_LOGLEVEL_QUIET;
> > > > > +               } else if (!strcmp(param, "debug")) {
> > > > > +                       efi_loglevel = CONSOLE_LOGLEVEL_DEBUG;
> > > >
> > > > Should we wire this to 'efi=debug' instead?
> > > >
> > >
> > > Sure.
> >
> > Do you prefer it wired up to both or just efi=debug?
> >
> 
> Let's stick with efi=debug, that is what all other EFI code uses.

Ok -- you can replace the patch with the attached version. Only change
is in efi_parse_options.

Author: Arvind Sankar <nivedita@alum.mit.edu>
Date:   Thu May 14 19:33:39 2020 -0400

    efi/libstub: Implement printk-style logging
    
    Use the efi_printk function in efi_info/efi_err, and add efi_debug. This
    allows formatted output at different log levels.
    
    Add the notion of a loglevel instead of just quiet/not-quiet, and
    parse the efi=debug kernel parameter in addition to quiet.
    
    Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>

diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index 5ecafc57619a..1f5a00b4f201 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -11,6 +11,7 @@
 
 #include <linux/efi.h>
 #include <linux/kernel.h>
+#include <linux/printk.h> /* For CONSOLE_LOGLEVEL_* */
 #include <asm/efi.h>
 
 #include "efistub.h"
@@ -18,7 +19,7 @@
 bool efi_nochunk;
 bool efi_nokaslr;
 bool efi_noinitrd;
-bool efi_quiet;
+int efi_loglevel = CONSOLE_LOGLEVEL_DEFAULT;
 bool efi_novamap;
 
 static bool efi_nosoftreserve;
@@ -58,6 +59,28 @@ int efi_printk(const char *fmt, ...)
 	char printf_buf[256];
 	va_list args;
 	int printed;
+	int loglevel = printk_get_level(fmt);
+
+	switch (loglevel) {
+	case '0' ... '9':
+		loglevel -= '0';
+		break;
+	default:
+		/*
+		 * Use loglevel -1 for cases where we just want to print to
+		 * the screen.
+		 */
+		loglevel = -1;
+		break;
+	}
+
+	if (loglevel >= efi_loglevel)
+		return 0;
+
+	if (loglevel >= 0)
+		efi_puts("EFI stub: ");
+
+	fmt = printk_skip_level(fmt);
 
 	va_start(args, fmt);
 	printed = vsnprintf(printf_buf, sizeof(printf_buf), fmt, args);
@@ -100,7 +123,7 @@ efi_status_t efi_parse_options(char const *cmdline)
 		if (!strcmp(param, "nokaslr")) {
 			efi_nokaslr = true;
 		} else if (!strcmp(param, "quiet")) {
-			efi_quiet = true;
+			efi_loglevel = CONSOLE_LOGLEVEL_QUIET;
 		} else if (!strcmp(param, "noinitrd")) {
 			efi_noinitrd = true;
 		} else if (!strcmp(param, "efi") && val) {
@@ -114,6 +137,8 @@ efi_status_t efi_parse_options(char const *cmdline)
 				efi_disable_pci_dma = true;
 			if (parse_option_str(val, "no_disable_early_pci_dma"))
 				efi_disable_pci_dma = false;
+			if (parse_option_str(val, "debug"))
+				efi_loglevel = CONSOLE_LOGLEVEL_DEBUG;
 		} else if (!strcmp(param, "video") &&
 			   val && strstarts(val, "efifb:")) {
 			efi_parse_option_graphics(val + strlen("efifb:"));
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index caa7dcc71c69..3a323a009836 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -6,6 +6,7 @@
 #include <linux/compiler.h>
 #include <linux/efi.h>
 #include <linux/kernel.h>
+#include <linux/kern_levels.h>
 #include <linux/types.h>
 #include <asm/efi.h>
 
@@ -34,7 +35,7 @@
 extern bool efi_nochunk;
 extern bool efi_nokaslr;
 extern bool efi_noinitrd;
-extern bool efi_quiet;
+extern int efi_loglevel;
 extern bool efi_novamap;
 
 extern const efi_system_table_t *efi_system_table;
@@ -49,11 +50,12 @@ extern const efi_system_table_t *efi_system_table;
 
 #endif
 
-#define efi_info(msg)		do {			\
-	if (!efi_quiet) efi_puts("EFI stub: "msg);	\
-} while (0)
-
-#define efi_err(msg) efi_puts("EFI stub: ERROR: "msg)
+#define efi_info(fmt, ...) \
+	efi_printk(KERN_INFO fmt, ##__VA_ARGS__)
+#define efi_err(fmt, ...) \
+	efi_printk(KERN_ERR "ERROR: " fmt, ##__VA_ARGS__)
+#define efi_debug(fmt, ...) \
+	efi_printk(KERN_DEBUG "DEBUG: " fmt, ##__VA_ARGS__)
 
 /* Helper macros for the usual case of using simple C variables: */
 #ifndef fdt_setprop_inplace_var

Re: [PATCH 17/24] efi/libstub: Implement printk-style logging

[Ard Biesheuvel]

On Wed, 20 May 2020 at 19:02, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Wed, May 20, 2020 at 06:38:53PM +0200, Ard Biesheuvel wrote:
> > On Wed, 20 May 2020 at 18:38, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > >
> > > On Tue, May 19, 2020 at 11:07:55AM -0400, Arvind Sankar wrote:
> > > > On Tue, May 19, 2020 at 10:22:40AM +0200, Ard Biesheuvel wrote:
> > > > > On Mon, 18 May 2020 at 21:07, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > > > > > @@ -100,7 +123,9 @@ efi_status_t efi_parse_options(char const *cmdline)
> > > > > >                 if (!strcmp(param, "nokaslr")) {
> > > > > >                         efi_nokaslr = true;
> > > > > >                 } else if (!strcmp(param, "quiet")) {
> > > > > > -                       efi_quiet = true;
> > > > > > +                       efi_loglevel = CONSOLE_LOGLEVEL_QUIET;
> > > > > > +               } else if (!strcmp(param, "debug")) {
> > > > > > +                       efi_loglevel = CONSOLE_LOGLEVEL_DEBUG;
> > > > >
> > > > > Should we wire this to 'efi=debug' instead?
> > > > >
> > > >
> > > > Sure.
> > >
> > > Do you prefer it wired up to both or just efi=debug?
> > >
> >
> > Let's stick with efi=debug, that is what all other EFI code uses.
>
> Ok -- you can replace the patch with the attached version. Only change
> is in efi_parse_options.
>

Done - thanks!


> Author: Arvind Sankar <nivedita@alum.mit.edu>
> Date:   Thu May 14 19:33:39 2020 -0400
>
>     efi/libstub: Implement printk-style logging
>
>     Use the efi_printk function in efi_info/efi_err, and add efi_debug. This
>     allows formatted output at different log levels.
>
>     Add the notion of a loglevel instead of just quiet/not-quiet, and
>     parse the efi=debug kernel parameter in addition to quiet.
>
>     Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
>
> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> index 5ecafc57619a..1f5a00b4f201 100644
> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> @@ -11,6 +11,7 @@
>
>  #include <linux/efi.h>
>  #include <linux/kernel.h>
> +#include <linux/printk.h> /* For CONSOLE_LOGLEVEL_* */
>  #include <asm/efi.h>
>
>  #include "efistub.h"
> @@ -18,7 +19,7 @@
>  bool efi_nochunk;
>  bool efi_nokaslr;
>  bool efi_noinitrd;
> -bool efi_quiet;
> +int efi_loglevel = CONSOLE_LOGLEVEL_DEFAULT;
>  bool efi_novamap;
>
>  static bool efi_nosoftreserve;
> @@ -58,6 +59,28 @@ int efi_printk(const char *fmt, ...)
>         char printf_buf[256];
>         va_list args;
>         int printed;
> +       int loglevel = printk_get_level(fmt);
> +
> +       switch (loglevel) {
> +       case '0' ... '9':
> +               loglevel -= '0';
> +               break;
> +       default:
> +               /*
> +                * Use loglevel -1 for cases where we just want to print to
> +                * the screen.
> +                */
> +               loglevel = -1;
> +               break;
> +       }
> +
> +       if (loglevel >= efi_loglevel)
> +               return 0;
> +
> +       if (loglevel >= 0)
> +               efi_puts("EFI stub: ");
> +
> +       fmt = printk_skip_level(fmt);
>
>         va_start(args, fmt);
>         printed = vsnprintf(printf_buf, sizeof(printf_buf), fmt, args);
> @@ -100,7 +123,7 @@ efi_status_t efi_parse_options(char const *cmdline)
>                 if (!strcmp(param, "nokaslr")) {
>                         efi_nokaslr = true;
>                 } else if (!strcmp(param, "quiet")) {
> -                       efi_quiet = true;
> +                       efi_loglevel = CONSOLE_LOGLEVEL_QUIET;
>                 } else if (!strcmp(param, "noinitrd")) {
>                         efi_noinitrd = true;
>                 } else if (!strcmp(param, "efi") && val) {
> @@ -114,6 +137,8 @@ efi_status_t efi_parse_options(char const *cmdline)
>                                 efi_disable_pci_dma = true;
>                         if (parse_option_str(val, "no_disable_early_pci_dma"))
>                                 efi_disable_pci_dma = false;
> +                       if (parse_option_str(val, "debug"))
> +                               efi_loglevel = CONSOLE_LOGLEVEL_DEBUG;
>                 } else if (!strcmp(param, "video") &&
>                            val && strstarts(val, "efifb:")) {
>                         efi_parse_option_graphics(val + strlen("efifb:"));
> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> index caa7dcc71c69..3a323a009836 100644
> --- a/drivers/firmware/efi/libstub/efistub.h
> +++ b/drivers/firmware/efi/libstub/efistub.h
> @@ -6,6 +6,7 @@
>  #include <linux/compiler.h>
>  #include <linux/efi.h>
>  #include <linux/kernel.h>
> +#include <linux/kern_levels.h>
>  #include <linux/types.h>
>  #include <asm/efi.h>
>
> @@ -34,7 +35,7 @@
>  extern bool efi_nochunk;
>  extern bool efi_nokaslr;
>  extern bool efi_noinitrd;
> -extern bool efi_quiet;
> +extern int efi_loglevel;
>  extern bool efi_novamap;
>
>  extern const efi_system_table_t *efi_system_table;
> @@ -49,11 +50,12 @@ extern const efi_system_table_t *efi_system_table;
>
>  #endif
>
> -#define efi_info(msg)          do {                    \
> -       if (!efi_quiet) efi_puts("EFI stub: "msg);      \
> -} while (0)
> -
> -#define efi_err(msg) efi_puts("EFI stub: ERROR: "msg)
> +#define efi_info(fmt, ...) \
> +       efi_printk(KERN_INFO fmt, ##__VA_ARGS__)
> +#define efi_err(fmt, ...) \
> +       efi_printk(KERN_ERR "ERROR: " fmt, ##__VA_ARGS__)
> +#define efi_debug(fmt, ...) \
> +       efi_printk(KERN_DEBUG "DEBUG: " fmt, ##__VA_ARGS__)
>
>  /* Helper macros for the usual case of using simple C variables: */
>  #ifndef fdt_setprop_inplace_var

[PATCH] efi/libstub: Don't parse overlong command lines

[Arvind Sankar]

Check if the command line passed in is larger than COMMAND_LINE_SIZE,
and truncate it to the last full argument if so.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 .../firmware/efi/libstub/efi-stub-helper.c    | 28 +++++++++++++++----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index 2a6aded4f2a9..89f075275300 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -9,10 +9,12 @@
 
 #include <stdarg.h>
 
+#include <linux/ctype.h>
 #include <linux/efi.h>
 #include <linux/kernel.h>
 #include <linux/printk.h> /* For CONSOLE_LOGLEVEL_* */
 #include <asm/efi.h>
+#include <asm/setup.h>
 
 #include "efistub.h"
 
@@ -216,22 +218,33 @@ char *efi_convert_cmdline(efi_loaded_image_t *image, int *cmd_line_len)
 	unsigned long cmdline_addr = 0;
 	int options_chars = efi_table_attr(image, load_options_size) / 2;
 	const u16 *options = efi_table_attr(image, load_options);
-	int options_bytes = 0;  /* UTF-8 bytes */
+	int options_bytes = 0, safe_options_bytes = 0;  /* UTF-8 bytes */
+	bool in_quote = false;
 	efi_status_t status;
 
 	if (options) {
 		s2 = options;
-		while (options_chars--) {
+		while (options_bytes < COMMAND_LINE_SIZE && options_chars--) {
 			u16 c = *s2++;
 
-			if (c == L'\0' || c == L'\n')
-				break;
+			if (c < 0x80) {
+				if (c == L'\0' || c == L'\n')
+					break;
+				if (c == L'"')
+					in_quote = !in_quote;
+				else if (!in_quote && isspace((char)c))
+					safe_options_bytes = options_bytes;
+
+				options_bytes++;
+				continue;
+			}
+
 			/*
 			 * Get the number of UTF-8 bytes corresponding to a
 			 * UTF-16 character.
 			 * The first part handles everything in the BMP.
 			 */
-			options_bytes += 1 + (c >= 0x80) + (c >= 0x800);
+			options_bytes += 2 + (c >= 0x800);
 			/*
 			 * Add one more byte for valid surrogate pairs. Invalid
 			 * surrogates will be replaced with 0xfffd and take up
@@ -252,6 +265,11 @@ char *efi_convert_cmdline(efi_loaded_image_t *image, int *cmd_line_len)
 				}
 			}
 		}
+		if (options_bytes >= COMMAND_LINE_SIZE) {
+			options_bytes = safe_options_bytes;
+			efi_err("Command line is too long: truncated to %d bytes\n",
+				options_bytes);
+		}
 	}
 
 	options_bytes++;	/* NUL termination */
-- 
2.26.2

Re: [PATCH] efi/libstub: Don't parse overlong command lines

[Ard Biesheuvel]

On Thu, 21 May 2020 at 02:29, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> Check if the command line passed in is larger than COMMAND_LINE_SIZE,
> and truncate it to the last full argument if so.
>
> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>

Queued up in efi/next, thanks.

> ---
>  .../firmware/efi/libstub/efi-stub-helper.c    | 28 +++++++++++++++----
>  1 file changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> index 2a6aded4f2a9..89f075275300 100644
> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> @@ -9,10 +9,12 @@
>
>  #include <stdarg.h>
>
> +#include <linux/ctype.h>
>  #include <linux/efi.h>
>  #include <linux/kernel.h>
>  #include <linux/printk.h> /* For CONSOLE_LOGLEVEL_* */
>  #include <asm/efi.h>
> +#include <asm/setup.h>
>
>  #include "efistub.h"
>
> @@ -216,22 +218,33 @@ char *efi_convert_cmdline(efi_loaded_image_t *image, int *cmd_line_len)
>         unsigned long cmdline_addr = 0;
>         int options_chars = efi_table_attr(image, load_options_size) / 2;
>         const u16 *options = efi_table_attr(image, load_options);
> -       int options_bytes = 0;  /* UTF-8 bytes */
> +       int options_bytes = 0, safe_options_bytes = 0;  /* UTF-8 bytes */
> +       bool in_quote = false;
>         efi_status_t status;
>
>         if (options) {
>                 s2 = options;
> -               while (options_chars--) {
> +               while (options_bytes < COMMAND_LINE_SIZE && options_chars--) {
>                         u16 c = *s2++;
>
> -                       if (c == L'\0' || c == L'\n')
> -                               break;
> +                       if (c < 0x80) {
> +                               if (c == L'\0' || c == L'\n')
> +                                       break;
> +                               if (c == L'"')
> +                                       in_quote = !in_quote;
> +                               else if (!in_quote && isspace((char)c))
> +                                       safe_options_bytes = options_bytes;
> +
> +                               options_bytes++;
> +                               continue;
> +                       }
> +
>                         /*
>                          * Get the number of UTF-8 bytes corresponding to a
>                          * UTF-16 character.
>                          * The first part handles everything in the BMP.
>                          */
> -                       options_bytes += 1 + (c >= 0x80) + (c >= 0x800);
> +                       options_bytes += 2 + (c >= 0x800);
>                         /*
>                          * Add one more byte for valid surrogate pairs. Invalid
>                          * surrogates will be replaced with 0xfffd and take up
> @@ -252,6 +265,11 @@ char *efi_convert_cmdline(efi_loaded_image_t *image, int *cmd_line_len)
>                                 }
>                         }
>                 }
> +               if (options_bytes >= COMMAND_LINE_SIZE) {
> +                       options_bytes = safe_options_bytes;
> +                       efi_err("Command line is too long: truncated to %d bytes\n",
> +                               options_bytes);
> +               }
>         }
>
>         options_bytes++;        /* NUL termination */
> --
> 2.26.2
>

Re: [PATCH 05/24] efi/libstub: Optimize for size instead of speed

[Andrey Ignatov]

[-- Attachment #1: Type: text/plain, Size: 5222 bytes --]

Arvind Sankar <nivedita@alum.mit.edu> [Wed, 1969-12-31 23:00 -0800]:
> Reclaim the bloat from the addition of printf by optimizing the stub for
> size. With gcc 9, the text size of the stub is:
> 
> ARCH    before  +printf    -Os
> arm      35197    37889  34638
> arm64    34883    38159  34479
> i386     18571    21657  17025
> x86_64   25677    29328  22144
> 
> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> ---
>  drivers/firmware/efi/libstub/Makefile | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> index fb34c9d14a3c..034d71663b1e 100644
> --- a/drivers/firmware/efi/libstub/Makefile
> +++ b/drivers/firmware/efi/libstub/Makefile
> @@ -7,7 +7,7 @@
>  #
>  cflags-$(CONFIG_X86_32)		:= -march=i386
>  cflags-$(CONFIG_X86_64)		:= -mcmodel=small
> -cflags-$(CONFIG_X86)		+= -m$(BITS) -D__KERNEL__ -O2 \
> +cflags-$(CONFIG_X86)		+= -m$(BITS) -D__KERNEL__ \
>  				   -fPIC -fno-strict-aliasing -mno-red-zone \
>  				   -mno-mmx -mno-sse -fshort-wchar \
>  				   -Wno-pointer-sign \
> @@ -25,7 +25,7 @@ cflags-$(CONFIG_ARM)		:= $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
>  
>  cflags-$(CONFIG_EFI_GENERIC_STUB) += -I$(srctree)/scripts/dtc/libfdt
>  
> -KBUILD_CFLAGS			:= $(cflags-y) -DDISABLE_BRANCH_PROFILING \
> +KBUILD_CFLAGS			:= $(cflags-y) -Os -DDISABLE_BRANCH_PROFILING \
>  				   -include $(srctree)/drivers/firmware/efi/libstub/hidden.h \
>  				   -D__NO_FORTIFY \
>  				   $(call cc-option,-ffreestanding) \

Hi Arvind,

This patch breaks build for me:

$>make -j32 -s bzImage
drivers/firmware/efi/libstub/alignedmem.c: In function \x2018efi_allocate_pages_aligned\x2019:
drivers/firmware/efi/libstub/alignedmem.c:38:9: sorry, unimplemented: ms_abi attribute requires -maccumulate-outgoing-args or subtarget optimization implying it
  status = efi_bs_call(allocate_pages, EFI_ALLOCATE_MAX_ADDRESS,
         ^
In file included from drivers/firmware/efi/libstub/alignedmem.c:6:0:
drivers/firmware/efi/libstub/efistub.h:49:64: sorry, unimplemented: ms_abi attribute requires -maccumulate-outgoing-args or subtarget optimization implying it
 #define efi_bs_call(func, ...) efi_system_table->boottime->func(__VA_ARGS__)
                                                                ^
drivers/firmware/efi/libstub/alignedmem.c:50:4: note: in expansion of macro \x2018efi_bs_call\x2019
    efi_bs_call(free_pages, alloc_addr, slack - l + 1);
    ^
drivers/firmware/efi/libstub/alignedmem.c:50: confused by earlier errors, bailing out
In file included from drivers/firmware/efi/libstub/efi-stub-helper.c:19:0:
drivers/firmware/efi/libstub/efi-stub-helper.c: In function \x2018efi_char16_puts\x2019:
drivers/firmware/efi/libstub/efistub.h:52:51: sorry, unimplemented: ms_abi attribute requires -maccumulate-outgoing-args or subtarget optimization implying it
 #define efi_call_proto(inst, func, ...) inst->func(inst, ##__VA_ARGS__)
                                                   ^
drivers/firmware/efi/libstub/efi-stub-helper.c:37:2: note: in expansion of macro \x2018efi_call_proto\x2019
  efi_call_proto(efi_table_attr(efi_system_table, con_out),
  ^
drivers/firmware/efi/libstub/efi-stub-helper.c:37: confused by earlier errors, bailing out
drivers/firmware/efi/libstub/file.c: In function \x2018handle_cmdline_files\x2019:
drivers/firmware/efi/libstub/file.c:73:9: sorry, unimplemented: ms_abi attribute requires -maccumulate-outgoing-args or subtarget optimization implying it
  status = efi_bs_call(handle_protocol, image->device_handle, &fs_proto,
         ^
drivers/firmware/efi/libstub/file.c:73: confused by earlier errors, bailing out
Preprocessed source stored into /tmp/ccc4T4FI.out file, please attach this to your bugreport.
make[5]: *** [drivers/firmware/efi/libstub/alignedmem.o] Error 1
make[5]: *** Waiting for unfinished jobs....
drivers/firmware/efi/libstub/gop.c: In function \x2018efi_setup_gop\x2019:
drivers/firmware/efi/libstub/gop.c:565:9: sorry, unimplemented: ms_abi attribute requires -maccumulate-outgoing-args or subtarget optimization implying it
  status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size,
         ^
drivers/firmware/efi/libstub/gop.c:565: confused by earlier errors, bailing out
Preprocessed source stored into /tmp/ccgQG7p4.out file, please attach this to your bugreport.
make[5]: *** [drivers/firmware/efi/libstub/efi-stub-helper.o] Error 1
Preprocessed source stored into /tmp/ccnqGnqG.out file, please attach this to your bugreport.
make[5]: *** [drivers/firmware/efi/libstub/file.o] Error 1
Preprocessed source stored into /tmp/ccle7n2N.out file, please attach this to your bugreport.
make[5]: *** [drivers/firmware/efi/libstub/gop.o] Error 1
make[4]: *** [drivers/firmware/efi/libstub] Error 2
make[3]: *** [drivers/firmware/efi] Error 2
make[2]: *** [drivers/firmware] Error 2
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [drivers] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [sub-make] Error 2

Either reverting the patch or disabling CONFIG_EFI_STUB fixes it.

Initially I found it on bpf/bpf-next HEAD but same happens on
torvalds/linux.

I attach the config I used.

-- 
Andrey Ignatov

[-- Attachment #2: .config.gz --]
[-- Type: application/x-gunzip, Size: 33357 bytes --]

Re: [PATCH 05/24] efi/libstub: Optimize for size instead of speed

[Ard Biesheuvel]

Hello Andrey,

On Fri, 5 Jun 2020 at 02:31, Andrey Ignatov <rdna@fb.com> wrote:
>
> Arvind Sankar <nivedita@alum.mit.edu> [Wed, 1969-12-31 23:00 -0800]:
> > Reclaim the bloat from the addition of printf by optimizing the stub for
> > size. With gcc 9, the text size of the stub is:
> >
> > ARCH    before  +printf    -Os
> > arm      35197    37889  34638
> > arm64    34883    38159  34479
> > i386     18571    21657  17025
> > x86_64   25677    29328  22144
> >
> > Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> > ---
> >  drivers/firmware/efi/libstub/Makefile | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> > index fb34c9d14a3c..034d71663b1e 100644
> > --- a/drivers/firmware/efi/libstub/Makefile
> > +++ b/drivers/firmware/efi/libstub/Makefile
> > @@ -7,7 +7,7 @@
> >  #
> >  cflags-$(CONFIG_X86_32)              := -march=i386
> >  cflags-$(CONFIG_X86_64)              := -mcmodel=small
> > -cflags-$(CONFIG_X86)         += -m$(BITS) -D__KERNEL__ -O2 \
> > +cflags-$(CONFIG_X86)         += -m$(BITS) -D__KERNEL__ \
> >                                  -fPIC -fno-strict-aliasing -mno-red-zone \
> >                                  -mno-mmx -mno-sse -fshort-wchar \
> >                                  -Wno-pointer-sign \
> > @@ -25,7 +25,7 @@ cflags-$(CONFIG_ARM)                := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> >
> >  cflags-$(CONFIG_EFI_GENERIC_STUB) += -I$(srctree)/scripts/dtc/libfdt
> >
> > -KBUILD_CFLAGS                        := $(cflags-y) -DDISABLE_BRANCH_PROFILING \
> > +KBUILD_CFLAGS                        := $(cflags-y) -Os -DDISABLE_BRANCH_PROFILING \
> >                                  -include $(srctree)/drivers/firmware/efi/libstub/hidden.h \
> >                                  -D__NO_FORTIFY \
> >                                  $(call cc-option,-ffreestanding) \
>
> Hi Arvind,
>
> This patch breaks build for me:
>
> $>make -j32 -s bzImage
> drivers/firmware/efi/libstub/alignedmem.c: In function \x2018efi_allocate_pages_aligned\x2019:
> drivers/firmware/efi/libstub/alignedmem.c:38:9: sorry, unimplemented: ms_abi attribute requires -maccumulate-outgoing-args or subtarget optimization implying it
>   status = efi_bs_call(allocate_pages, EFI_ALLOCATE_MAX_ADDRESS,
>          ^

Which version of GCC are you using?


> In file included from drivers/firmware/efi/libstub/alignedmem.c:6:0:
> drivers/firmware/efi/libstub/efistub.h:49:64: sorry, unimplemented: ms_abi attribute requires -maccumulate-outgoing-args or subtarget optimization implying it
>  #define efi_bs_call(func, ...) efi_system_table->boottime->func(__VA_ARGS__)
>                                                                 ^
> drivers/firmware/efi/libstub/alignedmem.c:50:4: note: in expansion of macro \x2018efi_bs_call\x2019
>     efi_bs_call(free_pages, alloc_addr, slack - l + 1);
>     ^
> drivers/firmware/efi/libstub/alignedmem.c:50: confused by earlier errors, bailing out
> In file included from drivers/firmware/efi/libstub/efi-stub-helper.c:19:0:
> drivers/firmware/efi/libstub/efi-stub-helper.c: In function \x2018efi_char16_puts\x2019:
> drivers/firmware/efi/libstub/efistub.h:52:51: sorry, unimplemented: ms_abi attribute requires -maccumulate-outgoing-args or subtarget optimization implying it
>  #define efi_call_proto(inst, func, ...) inst->func(inst, ##__VA_ARGS__)
>                                                    ^
> drivers/firmware/efi/libstub/efi-stub-helper.c:37:2: note: in expansion of macro \x2018efi_call_proto\x2019
>   efi_call_proto(efi_table_attr(efi_system_table, con_out),
>   ^
> drivers/firmware/efi/libstub/efi-stub-helper.c:37: confused by earlier errors, bailing out
> drivers/firmware/efi/libstub/file.c: In function \x2018handle_cmdline_files\x2019:
> drivers/firmware/efi/libstub/file.c:73:9: sorry, unimplemented: ms_abi attribute requires -maccumulate-outgoing-args or subtarget optimization implying it
>   status = efi_bs_call(handle_protocol, image->device_handle, &fs_proto,
>          ^
> drivers/firmware/efi/libstub/file.c:73: confused by earlier errors, bailing out
> Preprocessed source stored into /tmp/ccc4T4FI.out file, please attach this to your bugreport.
> make[5]: *** [drivers/firmware/efi/libstub/alignedmem.o] Error 1
> make[5]: *** Waiting for unfinished jobs....
> drivers/firmware/efi/libstub/gop.c: In function \x2018efi_setup_gop\x2019:
> drivers/firmware/efi/libstub/gop.c:565:9: sorry, unimplemented: ms_abi attribute requires -maccumulate-outgoing-args or subtarget optimization implying it
>   status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size,
>          ^
> drivers/firmware/efi/libstub/gop.c:565: confused by earlier errors, bailing out
> Preprocessed source stored into /tmp/ccgQG7p4.out file, please attach this to your bugreport.
> make[5]: *** [drivers/firmware/efi/libstub/efi-stub-helper.o] Error 1
> Preprocessed source stored into /tmp/ccnqGnqG.out file, please attach this to your bugreport.
> make[5]: *** [drivers/firmware/efi/libstub/file.o] Error 1
> Preprocessed source stored into /tmp/ccle7n2N.out file, please attach this to your bugreport.
> make[5]: *** [drivers/firmware/efi/libstub/gop.o] Error 1
> make[4]: *** [drivers/firmware/efi/libstub] Error 2
> make[3]: *** [drivers/firmware/efi] Error 2
> make[2]: *** [drivers/firmware] Error 2
> make[2]: *** Waiting for unfinished jobs....
> make[1]: *** [drivers] Error 2
> make[1]: *** Waiting for unfinished jobs....
> make: *** [sub-make] Error 2
>
> Either reverting the patch or disabling CONFIG_EFI_STUB fixes it.
>
> Initially I found it on bpf/bpf-next HEAD but same happens on
> torvalds/linux.
>
> I attach the config I used.
>
> --
> Andrey Ignatov

Re: [PATCH 05/24] efi/libstub: Optimize for size instead of speed

[Arvind Sankar]

On Fri, Jun 05, 2020 at 08:33:22AM +0200, Ard Biesheuvel wrote:
> Hello Andrey,
> 
> On Fri, 5 Jun 2020 at 02:31, Andrey Ignatov <rdna@fb.com> wrote:
> >
> > Arvind Sankar <nivedita@alum.mit.edu> [Wed, 1969-12-31 23:00 -0800]:
> > > Reclaim the bloat from the addition of printf by optimizing the stub for
> > > size. With gcc 9, the text size of the stub is:
> > >
> > > ARCH    before  +printf    -Os
> > > arm      35197    37889  34638
> > > arm64    34883    38159  34479
> > > i386     18571    21657  17025
> > > x86_64   25677    29328  22144
> > >
> > > Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> > > ---
> > >  drivers/firmware/efi/libstub/Makefile | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> > > index fb34c9d14a3c..034d71663b1e 100644
> > > --- a/drivers/firmware/efi/libstub/Makefile
> > > +++ b/drivers/firmware/efi/libstub/Makefile
> > > @@ -7,7 +7,7 @@
> > >  #
> > >  cflags-$(CONFIG_X86_32)              := -march=i386
> > >  cflags-$(CONFIG_X86_64)              := -mcmodel=small
> > > -cflags-$(CONFIG_X86)         += -m$(BITS) -D__KERNEL__ -O2 \
> > > +cflags-$(CONFIG_X86)         += -m$(BITS) -D__KERNEL__ \
> > >                                  -fPIC -fno-strict-aliasing -mno-red-zone \
> > >                                  -mno-mmx -mno-sse -fshort-wchar \
> > >                                  -Wno-pointer-sign \
> > > @@ -25,7 +25,7 @@ cflags-$(CONFIG_ARM)                := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> > >
> > >  cflags-$(CONFIG_EFI_GENERIC_STUB) += -I$(srctree)/scripts/dtc/libfdt
> > >
> > > -KBUILD_CFLAGS                        := $(cflags-y) -DDISABLE_BRANCH_PROFILING \
> > > +KBUILD_CFLAGS                        := $(cflags-y) -Os -DDISABLE_BRANCH_PROFILING \
> > >                                  -include $(srctree)/drivers/firmware/efi/libstub/hidden.h \
> > >                                  -D__NO_FORTIFY \
> > >                                  $(call cc-option,-ffreestanding) \
> >
> > Hi Arvind,
> >
> > This patch breaks build for me:
> >
> > $>make -j32 -s bzImage
> > drivers/firmware/efi/libstub/alignedmem.c: In function \x2018efi_allocate_pages_aligned\x2019:
> > drivers/firmware/efi/libstub/alignedmem.c:38:9: sorry, unimplemented: ms_abi attribute requires -maccumulate-outgoing-args or subtarget optimization implying it
> >   status = efi_bs_call(allocate_pages, EFI_ALLOCATE_MAX_ADDRESS,
> >          ^
> 
> Which version of GCC are you using?

gcc-4.8.5 from the config. I got a copy and can reproduce it. Just
adding -maccumulate-outgoing-args appears to fix it, checking some more.

> 
> 
> > In file included from drivers/firmware/efi/libstub/alignedmem.c:6:0:
> > drivers/firmware/efi/libstub/efistub.h:49:64: sorry, unimplemented: ms_abi attribute requires -maccumulate-outgoing-args or subtarget optimization implying it
> >  #define efi_bs_call(func, ...) efi_system_table->boottime->func(__VA_ARGS__)
> >                                                                 ^
> > drivers/firmware/efi/libstub/alignedmem.c:50:4: note: in expansion of macro \x2018efi_bs_call\x2019
> >     efi_bs_call(free_pages, alloc_addr, slack - l + 1);
> >     ^
> > drivers/firmware/efi/libstub/alignedmem.c:50: confused by earlier errors, bailing out
> > In file included from drivers/firmware/efi/libstub/efi-stub-helper.c:19:0:
> > drivers/firmware/efi/libstub/efi-stub-helper.c: In function \x2018efi_char16_puts\x2019:
> > drivers/firmware/efi/libstub/efistub.h:52:51: sorry, unimplemented: ms_abi attribute requires -maccumulate-outgoing-args or subtarget optimization implying it
> >  #define efi_call_proto(inst, func, ...) inst->func(inst, ##__VA_ARGS__)
> >                                                    ^
> > drivers/firmware/efi/libstub/efi-stub-helper.c:37:2: note: in expansion of macro \x2018efi_call_proto\x2019
> >   efi_call_proto(efi_table_attr(efi_system_table, con_out),
> >   ^
> > drivers/firmware/efi/libstub/efi-stub-helper.c:37: confused by earlier errors, bailing out
> > drivers/firmware/efi/libstub/file.c: In function \x2018handle_cmdline_files\x2019:
> > drivers/firmware/efi/libstub/file.c:73:9: sorry, unimplemented: ms_abi attribute requires -maccumulate-outgoing-args or subtarget optimization implying it
> >   status = efi_bs_call(handle_protocol, image->device_handle, &fs_proto,
> >          ^
> > drivers/firmware/efi/libstub/file.c:73: confused by earlier errors, bailing out
> > Preprocessed source stored into /tmp/ccc4T4FI.out file, please attach this to your bugreport.
> > make[5]: *** [drivers/firmware/efi/libstub/alignedmem.o] Error 1
> > make[5]: *** Waiting for unfinished jobs....
> > drivers/firmware/efi/libstub/gop.c: In function \x2018efi_setup_gop\x2019:
> > drivers/firmware/efi/libstub/gop.c:565:9: sorry, unimplemented: ms_abi attribute requires -maccumulate-outgoing-args or subtarget optimization implying it
> >   status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size,
> >          ^
> > drivers/firmware/efi/libstub/gop.c:565: confused by earlier errors, bailing out
> > Preprocessed source stored into /tmp/ccgQG7p4.out file, please attach this to your bugreport.
> > make[5]: *** [drivers/firmware/efi/libstub/efi-stub-helper.o] Error 1
> > Preprocessed source stored into /tmp/ccnqGnqG.out file, please attach this to your bugreport.
> > make[5]: *** [drivers/firmware/efi/libstub/file.o] Error 1
> > Preprocessed source stored into /tmp/ccle7n2N.out file, please attach this to your bugreport.
> > make[5]: *** [drivers/firmware/efi/libstub/gop.o] Error 1
> > make[4]: *** [drivers/firmware/efi/libstub] Error 2
> > make[3]: *** [drivers/firmware/efi] Error 2
> > make[2]: *** [drivers/firmware] Error 2
> > make[2]: *** Waiting for unfinished jobs....
> > make[1]: *** [drivers] Error 2
> > make[1]: *** Waiting for unfinished jobs....
> > make: *** [sub-make] Error 2
> >
> > Either reverting the patch or disabling CONFIG_EFI_STUB fixes it.
> >
> > Initially I found it on bpf/bpf-next HEAD but same happens on
> > torvalds/linux.
> >
> > I attach the config I used.
> >
> > --
> > Andrey Ignatov

Re: [PATCH 05/24] efi/libstub: Optimize for size instead of speed

[Arvind Sankar]

On Fri, Jun 05, 2020 at 09:14:19AM -0400, Arvind Sankar wrote:
> On Fri, Jun 05, 2020 at 08:33:22AM +0200, Ard Biesheuvel wrote:
> > Hello Andrey,
> > 
> > On Fri, 5 Jun 2020 at 02:31, Andrey Ignatov <rdna@fb.com> wrote:
> > >
> > > Arvind Sankar <nivedita@alum.mit.edu> [Wed, 1969-12-31 23:00 -0800]:
> > > > Reclaim the bloat from the addition of printf by optimizing the stub for
> > > > size. With gcc 9, the text size of the stub is:
> > > >
> > > > ARCH    before  +printf    -Os
> > > > arm      35197    37889  34638
> > > > arm64    34883    38159  34479
> > > > i386     18571    21657  17025
> > > > x86_64   25677    29328  22144
> > > >
> > > > Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> > > > ---
> > > >  drivers/firmware/efi/libstub/Makefile | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> > > > index fb34c9d14a3c..034d71663b1e 100644
> > > > --- a/drivers/firmware/efi/libstub/Makefile
> > > > +++ b/drivers/firmware/efi/libstub/Makefile
> > > > @@ -7,7 +7,7 @@
> > > >  #
> > > >  cflags-$(CONFIG_X86_32)              := -march=i386
> > > >  cflags-$(CONFIG_X86_64)              := -mcmodel=small
> > > > -cflags-$(CONFIG_X86)         += -m$(BITS) -D__KERNEL__ -O2 \
> > > > +cflags-$(CONFIG_X86)         += -m$(BITS) -D__KERNEL__ \
> > > >                                  -fPIC -fno-strict-aliasing -mno-red-zone \
> > > >                                  -mno-mmx -mno-sse -fshort-wchar \
> > > >                                  -Wno-pointer-sign \
> > > > @@ -25,7 +25,7 @@ cflags-$(CONFIG_ARM)                := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> > > >
> > > >  cflags-$(CONFIG_EFI_GENERIC_STUB) += -I$(srctree)/scripts/dtc/libfdt
> > > >
> > > > -KBUILD_CFLAGS                        := $(cflags-y) -DDISABLE_BRANCH_PROFILING \
> > > > +KBUILD_CFLAGS                        := $(cflags-y) -Os -DDISABLE_BRANCH_PROFILING \
> > > >                                  -include $(srctree)/drivers/firmware/efi/libstub/hidden.h \
> > > >                                  -D__NO_FORTIFY \
> > > >                                  $(call cc-option,-ffreestanding) \
> > >
> > > Hi Arvind,
> > >
> > > This patch breaks build for me:
> > >
> > > $>make -j32 -s bzImage
> > > drivers/firmware/efi/libstub/alignedmem.c: In function \x2018efi_allocate_pages_aligned\x2019:
> > > drivers/firmware/efi/libstub/alignedmem.c:38:9: sorry, unimplemented: ms_abi attribute requires -maccumulate-outgoing-args or subtarget optimization implying it
> > >   status = efi_bs_call(allocate_pages, EFI_ALLOCATE_MAX_ADDRESS,
> > >          ^
> > 
> > Which version of GCC are you using?
> 
> gcc-4.8.5 from the config. I got a copy and can reproduce it. Just
> adding -maccumulate-outgoing-args appears to fix it, checking some more.
> 

On a simple test:
	extern void __attribute__ (( ms_abi )) ms_abi();
	void sysv_abi(void) { ms_abi(); }
it only breaks with -Os -fno-asynchronous-unwind-tables, weirdly enough.

Re: [PATCH 05/24] efi/libstub: Optimize for size instead of speed

[Ard Biesheuvel]

On Fri, 5 Jun 2020 at 15:32, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Fri, Jun 05, 2020 at 09:14:19AM -0400, Arvind Sankar wrote:
> > On Fri, Jun 05, 2020 at 08:33:22AM +0200, Ard Biesheuvel wrote:
> > > Hello Andrey,
> > >
> > > On Fri, 5 Jun 2020 at 02:31, Andrey Ignatov <rdna@fb.com> wrote:
> > > >
> > > > Arvind Sankar <nivedita@alum.mit.edu> [Wed, 1969-12-31 23:00 -0800]:
> > > > > Reclaim the bloat from the addition of printf by optimizing the stub for
> > > > > size. With gcc 9, the text size of the stub is:
> > > > >
> > > > > ARCH    before  +printf    -Os
> > > > > arm      35197    37889  34638
> > > > > arm64    34883    38159  34479
> > > > > i386     18571    21657  17025
> > > > > x86_64   25677    29328  22144
> > > > >
> > > > > Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> > > > > ---
> > > > >  drivers/firmware/efi/libstub/Makefile | 4 ++--
> > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> > > > > index fb34c9d14a3c..034d71663b1e 100644
> > > > > --- a/drivers/firmware/efi/libstub/Makefile
> > > > > +++ b/drivers/firmware/efi/libstub/Makefile
> > > > > @@ -7,7 +7,7 @@
> > > > >  #
> > > > >  cflags-$(CONFIG_X86_32)              := -march=i386
> > > > >  cflags-$(CONFIG_X86_64)              := -mcmodel=small
> > > > > -cflags-$(CONFIG_X86)         += -m$(BITS) -D__KERNEL__ -O2 \
> > > > > +cflags-$(CONFIG_X86)         += -m$(BITS) -D__KERNEL__ \
> > > > >                                  -fPIC -fno-strict-aliasing -mno-red-zone \
> > > > >                                  -mno-mmx -mno-sse -fshort-wchar \
> > > > >                                  -Wno-pointer-sign \
> > > > > @@ -25,7 +25,7 @@ cflags-$(CONFIG_ARM)                := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> > > > >
> > > > >  cflags-$(CONFIG_EFI_GENERIC_STUB) += -I$(srctree)/scripts/dtc/libfdt
> > > > >
> > > > > -KBUILD_CFLAGS                        := $(cflags-y) -DDISABLE_BRANCH_PROFILING \
> > > > > +KBUILD_CFLAGS                        := $(cflags-y) -Os -DDISABLE_BRANCH_PROFILING \
> > > > >                                  -include $(srctree)/drivers/firmware/efi/libstub/hidden.h \
> > > > >                                  -D__NO_FORTIFY \
> > > > >                                  $(call cc-option,-ffreestanding) \
> > > >
> > > > Hi Arvind,
> > > >
> > > > This patch breaks build for me:
> > > >
> > > > $>make -j32 -s bzImage
> > > > drivers/firmware/efi/libstub/alignedmem.c: In function \x2018efi_allocate_pages_aligned\x2019:
> > > > drivers/firmware/efi/libstub/alignedmem.c:38:9: sorry, unimplemented: ms_abi attribute requires -maccumulate-outgoing-args or subtarget optimization implying it
> > > >   status = efi_bs_call(allocate_pages, EFI_ALLOCATE_MAX_ADDRESS,
> > > >          ^
> > >
> > > Which version of GCC are you using?
> >
> > gcc-4.8.5 from the config. I got a copy and can reproduce it. Just
> > adding -maccumulate-outgoing-args appears to fix it, checking some more.
> >
>
> On a simple test:
>         extern void __attribute__ (( ms_abi )) ms_abi();
>         void sysv_abi(void) { ms_abi(); }
> it only breaks with -Os -fno-asynchronous-unwind-tables, weirdly enough.

I guess the logic that decides whether -maccumulate-outgoing-args is
enabled is somewhat opaque.

Could we perhaps back out the -Os change for 4.8 and earlier?

[PATCH] efi/x86: Fix build with gcc 4

[Arvind Sankar]

Commit
  bbf8e8b0fe04 ("efi/libstub: Optimize for size instead of speed")

changed the optimization level for the EFI stub to -Os from -O2.

Andrey Ignatov reports that this breaks the build with gcc 4.8.5.

Testing on godbolt.org, the combination of -Os,
-fno-asynchronous-unwind-tables, and ms_abi functions doesn't work,
failing with the error:
  sorry, unimplemented: ms_abi attribute requires
  -maccumulate-outgoing-args or subtarget optimization implying it

This does appear to work with gcc 4.9 onwards.

Add -maccumulate-outgoing-args explicitly to unbreak the build with
pre-4.9 versions of gcc.

Reported-by: Andrey Ignatov <rdna@fb.com>
Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 drivers/firmware/efi/libstub/Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
index cce4a7436052..d67418de768c 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -6,7 +6,8 @@
 # enabled, even if doing so doesn't break the build.
 #
 cflags-$(CONFIG_X86_32)		:= -march=i386
-cflags-$(CONFIG_X86_64)		:= -mcmodel=small
+cflags-$(CONFIG_X86_64)		:= -mcmodel=small \
+				   $(call cc-option,-maccumulate-outgoing-args)
 cflags-$(CONFIG_X86)		+= -m$(BITS) -D__KERNEL__ \
 				   -fPIC -fno-strict-aliasing -mno-red-zone \
 				   -mno-mmx -mno-sse -fshort-wchar \
-- 
2.26.2

Re: [PATCH 05/24] efi/libstub: Optimize for size instead of speed

[Arvind Sankar]

On Fri, Jun 05, 2020 at 04:53:59PM +0200, Ard Biesheuvel wrote:
> I guess the logic that decides whether -maccumulate-outgoing-args is
> enabled is somewhat opaque.
> 
> Could we perhaps back out the -Os change for 4.8 and earlier?

I just sent a patch to add the accumulate-outgoing-args option
explicitly. That fixes 4.8.5 and doesn't seem to affect at least
gcc-9.3.0, which presumably already enables it automatically.

Thanks.

Re: [PATCH 05/24] efi/libstub: Optimize for size instead of speed

[Ard Biesheuvel]

On Fri, 5 Jun 2020 at 17:10, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Fri, Jun 05, 2020 at 04:53:59PM +0200, Ard Biesheuvel wrote:
> > I guess the logic that decides whether -maccumulate-outgoing-args is
> > enabled is somewhat opaque.
> >
> > Could we perhaps back out the -Os change for 4.8 and earlier?
>
> I just sent a patch to add the accumulate-outgoing-args option
> explicitly. That fixes 4.8.5 and doesn't seem to affect at least
> gcc-9.3.0, which presumably already enables it automatically.
>

Fair enough.

Re: [PATCH] efi/x86: Fix build with gcc 4

[Andrey Ignatov]

Arvind Sankar <nivedita@alum.mit.edu> [Fri, 2020-06-05 08:06 -0700]:
> Commit
>   bbf8e8b0fe04 ("efi/libstub: Optimize for size instead of speed")
> 
> changed the optimization level for the EFI stub to -Os from -O2.
> 
> Andrey Ignatov reports that this breaks the build with gcc 4.8.5.
> 
> Testing on godbolt.org, the combination of -Os,
> -fno-asynchronous-unwind-tables, and ms_abi functions doesn't work,
> failing with the error:
>   sorry, unimplemented: ms_abi attribute requires
>   -maccumulate-outgoing-args or subtarget optimization implying it
> 
> This does appear to work with gcc 4.9 onwards.
> 
> Add -maccumulate-outgoing-args explicitly to unbreak the build with
> pre-4.9 versions of gcc.
> 
> Reported-by: Andrey Ignatov <rdna@fb.com>
> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>

Thanks. I confirmed it fixes the problem on my setup with gcc 4.8.5 and
also works as before with clang 9.0.20190721.

> ---
>  drivers/firmware/efi/libstub/Makefile | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> index cce4a7436052..d67418de768c 100644
> --- a/drivers/firmware/efi/libstub/Makefile
> +++ b/drivers/firmware/efi/libstub/Makefile
> @@ -6,7 +6,8 @@
>  # enabled, even if doing so doesn't break the build.
>  #
>  cflags-$(CONFIG_X86_32)		:= -march=i386
> -cflags-$(CONFIG_X86_64)		:= -mcmodel=small
> +cflags-$(CONFIG_X86_64)		:= -mcmodel=small \
> +				   $(call cc-option,-maccumulate-outgoing-args)
>  cflags-$(CONFIG_X86)		+= -m$(BITS) -D__KERNEL__ \
>  				   -fPIC -fno-strict-aliasing -mno-red-zone \
>  				   -mno-mmx -mno-sse -fshort-wchar \
> -- 
> 2.26.2
> 

-- 
Andrey Ignatov

Re: [PATCH] efi/x86: Fix build with gcc 4

[Ard Biesheuvel]

On Fri, 5 Jun 2020 at 18:09, Andrey Ignatov <rdna@fb.com> wrote:
>
> Arvind Sankar <nivedita@alum.mit.edu> [Fri, 2020-06-05 08:06 -0700]:
> > Commit
> >   bbf8e8b0fe04 ("efi/libstub: Optimize for size instead of speed")
> >
> > changed the optimization level for the EFI stub to -Os from -O2.
> >
> > Andrey Ignatov reports that this breaks the build with gcc 4.8.5.
> >
> > Testing on godbolt.org, the combination of -Os,
> > -fno-asynchronous-unwind-tables, and ms_abi functions doesn't work,
> > failing with the error:
> >   sorry, unimplemented: ms_abi attribute requires
> >   -maccumulate-outgoing-args or subtarget optimization implying it
> >
> > This does appear to work with gcc 4.9 onwards.
> >
> > Add -maccumulate-outgoing-args explicitly to unbreak the build with
> > pre-4.9 versions of gcc.
> >
> > Reported-by: Andrey Ignatov <rdna@fb.com>
> > Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
>
> Thanks. I confirmed it fixes the problem on my setup with gcc 4.8.5 and
> also works as before with clang 9.0.20190721.
>

Queued in efi/urgent, thanks.

> > ---
> >  drivers/firmware/efi/libstub/Makefile | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> > index cce4a7436052..d67418de768c 100644
> > --- a/drivers/firmware/efi/libstub/Makefile
> > +++ b/drivers/firmware/efi/libstub/Makefile
> > @@ -6,7 +6,8 @@
> >  # enabled, even if doing so doesn't break the build.
> >  #
> >  cflags-$(CONFIG_X86_32)              := -march=i386
> > -cflags-$(CONFIG_X86_64)              := -mcmodel=small
> > +cflags-$(CONFIG_X86_64)              := -mcmodel=small \
> > +                                $(call cc-option,-maccumulate-outgoing-args)
> >  cflags-$(CONFIG_X86)         += -m$(BITS) -D__KERNEL__ \
> >                                  -fPIC -fno-strict-aliasing -mno-red-zone \
> >                                  -mno-mmx -mno-sse -fshort-wchar \
> > --
> > 2.26.2
> >
>
> --
> Andrey Ignatov