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

[Ard Biesheuvel <ardb@kernel.org>]

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