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

From: Arvind Sankar <nivedita@alum.mit.edu>
To: Ard Biesheuvel <ardb@kernel.org>
Cc: Arvind Sankar <nivedita@alum.mit.edu>,
	linux-efi <linux-efi@vger.kernel.org>
Subject: Re: [PATCH 17/24] efi/libstub: Implement printk-style logging
Date: Wed, 20 May 2020 13:02:23 -0400	[thread overview]
Message-ID: <20200520170223.GA3333632@rani.riverdale.lan> (raw)
In-Reply-To: <CAMj1kXG2h=x9sfu0ELyUyveu_1hU5qiuRkQXf8Y4dnRPErb3Xw@mail.gmail.com>

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
