From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.8 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id C5BB0C433B4 for ; Wed, 31 Mar 2021 16:10:50 +0000 (UTC) Received: from lists.ozlabs.org (lists.ozlabs.org [112.213.38.117]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 423496100A for ; Wed, 31 Mar 2021 16:10:50 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 423496100A Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Received: from boromir.ozlabs.org (localhost [IPv6:::1]) by lists.ozlabs.org (Postfix) with ESMTP id 4F9WWX6XSJz3btG for ; Thu, 1 Apr 2021 03:10:48 +1100 (AEDT) Authentication-Results: lists.ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=kernel.org header.i=@kernel.org header.a=rsa-sha256 header.s=k20201202 header.b=MPCLn42g; dkim-atps=neutral Authentication-Results: lists.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=kernel.org (client-ip=198.145.29.99; helo=mail.kernel.org; envelope-from=ardb@kernel.org; receiver=) Authentication-Results: lists.ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=kernel.org header.i=@kernel.org header.a=rsa-sha256 header.s=k20201202 header.b=MPCLn42g; dkim-atps=neutral Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4F9WW24gLMz2xfy for ; Thu, 1 Apr 2021 03:10:22 +1100 (AEDT) Received: by mail.kernel.org (Postfix) with ESMTPSA id 5634560BD3 for ; Wed, 31 Mar 2021 16:10:20 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1617207020; bh=/1ZfUq/hSkP8vyLmN2wN3gF/K+0R8P1I95At/e2yk3o=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=MPCLn42gcnWhwN/zKbA8+c1PbkFnwg8YwYyuVGUM+PRQBAaqJailiFH6ekOnlpoCz f/gDaJ8Qq+HQLeaUhLP/jmgd9+Ue0CK/lohnrM2laPXY7/BFGINxJLz9kDlyuXG7yD PsUQMmNfnEhkD9y74gOqg3x+FCJQ2b7NTHEPZot9bGc6QEmDWRxdrLXrpcgJyM+VH4 g1MFlnNSURc2Wti4P0kBL/M42y5bIj4CjxCGtcNcC5UlVKT0zrbISi6OplZuuHrQPa D9Sv+PSbuf1rNFDLO0ndktG+dsTlTWzCLE1OksBrdBB+UAVvYv0atkgWJVXgOp5RB9 zwZF7FS+jZj5A== Received: by mail-oi1-f181.google.com with SMTP id c16so20518322oib.3 for ; Wed, 31 Mar 2021 09:10:20 -0700 (PDT) X-Gm-Message-State: AOAM5329m6MKm02SGvqVJdDSSjlzxxcb8uL90idIOAKi7bRZdvX7Arjo P5XglCV8C2A91gpE7CQrX46LAgn32WngeYkvmK8= X-Google-Smtp-Source: ABdhPJzyQnLqutk8wJtGUcalgJX2yCNn0BLIq/ju4C/c+FzWFIriO7+Ss7TW+wiuTsHll9sO+01CXGbsemmBHeRSmAk= X-Received: by 2002:aca:1a01:: with SMTP id a1mr2775869oia.33.1617207019681; Wed, 31 Mar 2021 09:10:19 -0700 (PDT) MIME-Version: 1.0 References: <41021d66db2ab427c14255d2a24bb4517c8b58fd.1617126961.git.danielwa@cisco.com> In-Reply-To: From: Ard Biesheuvel Date: Wed, 31 Mar 2021 18:10:08 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH 6/8] drivers: firmware: efi: libstub: enable generic commandline To: Daniel Walker , Arvind Sankar Content-Type: text/plain; charset="UTF-8" X-BeenThere: linuxppc-dev@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: ob Herring , linux-efi , Daniel Gimpelevich , "open list:LINUX FOR POWERPC \(32-BIT AND 64-BIT\)" , X86 ML , "open list:MIPS" , Linux Kernel Mailing List , xe-linux-external@cisco.com, Andrew Morton , Will Deacon Errors-To: linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Sender: "Linuxppc-dev" (+ Arvind) On Tue, 30 Mar 2021 at 19:57, Daniel Walker wrote: > > This adds code to handle the generic command line changes. > The efi code appears that it doesn't benefit as much from this design > as it could. > > For example, if you had a prepend command line with "nokaslr" then > you might be helpful to re-enable it in the boot loader or dts, > but there appears to be no way to re-enable kaslr or some of the > other options. > > Cc: xe-linux-external@cisco.com > Signed-off-by: Daniel Walker > --- > .../firmware/efi/libstub/efi-stub-helper.c | 35 +++++++++++++++++++ > drivers/firmware/efi/libstub/efi-stub.c | 7 ++++ > drivers/firmware/efi/libstub/efistub.h | 1 + > drivers/firmware/efi/libstub/x86-stub.c | 13 +++++-- > 4 files changed, 54 insertions(+), 2 deletions(-) > > diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c > index aa8da0a49829..c155837cedc9 100644 > --- a/drivers/firmware/efi/libstub/efi-stub-helper.c > +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c > @@ -13,6 +13,7 @@ > #include > #include > #include /* For CONSOLE_LOGLEVEL_* */ > +#include > #include > #include > > @@ -172,6 +173,40 @@ int efi_printk(const char *fmt, ...) > return printed; > } > > +/** > + * efi_handle_cmdline() - handle adding in building parts of the command line > + * @cmdline: kernel command line > + * > + * Add in the generic parts of the commandline and start the parsing of the > + * command line. > + * > + * Return: status code > + */ > +efi_status_t efi_handle_cmdline(char const *cmdline) > +{ > + efi_status_t status; > + > + status = efi_parse_options(CMDLINE_PREPEND); > + if (status != EFI_SUCCESS) { > + efi_err("Failed to parse options\n"); > + return status; > + } Even though I am not a fan of the 'success handling' pattern, duplicating the exact same error handling three times is not great either. Could we reuse more of the code here? > + > + status = efi_parse_options(IS_ENABLED(CONFIG_CMDLINE_OVERRIDE) ? "" : cmdline); What is the point of calling efi_parse_options() with an empty string? > + if (status != EFI_SUCCESS) { > + efi_err("Failed to parse options\n"); > + return status; > + } > + > + status = efi_parse_options(CMDLINE_APPEND); > + if (status != EFI_SUCCESS) { > + efi_err("Failed to parse options\n"); > + return status; > + } > + > + return EFI_SUCCESS; > +} > + > /** > * efi_parse_options() - Parse EFI command line options > * @cmdline: kernel command line > diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c > index 26e69788f27a..760480248adf 100644 > --- a/drivers/firmware/efi/libstub/efi-stub.c > +++ b/drivers/firmware/efi/libstub/efi-stub.c > @@ -172,6 +172,12 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle, > goto fail; > } > > +#ifdef CONFIG_GENERIC_CMDLINE > + status = efi_handle_cmdline(cmdline_ptr); > + if (status != EFI_SUCCESS) { > + goto fail_free_cmdline; > + } > +#else > if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) || > IS_ENABLED(CONFIG_CMDLINE_FORCE) || Does this mean CONFIG_GENERIC_CMDLINE does not replace CMDLINE_EXTEND / CMDLINE_FORCE etc, but introduces yet another variant on top of those? That does not seem like an improvement to me. I think it is great that you are cleaning this up, but only if it means we can get rid of the old implementation. > cmdline_size == 0) { > @@ -189,6 +195,7 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle, > goto fail_free_cmdline; > } > } > +#endif > > efi_info("Booting Linux Kernel...\n"); > > diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h > index cde0a2ef507d..07c7f9fdfffc 100644 > --- a/drivers/firmware/efi/libstub/efistub.h > +++ b/drivers/firmware/efi/libstub/efistub.h > @@ -800,6 +800,7 @@ efi_status_t efi_relocate_kernel(unsigned long *image_addr, > unsigned long alignment, > unsigned long min_addr); > > +efi_status_t efi_handle_cmdline(char const *cmdline); > efi_status_t efi_parse_options(char const *cmdline); > > void efi_parse_option_graphics(char *option); > diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c > index f14c4ff5839f..30ad8fb7122d 100644 > --- a/drivers/firmware/efi/libstub/x86-stub.c > +++ b/drivers/firmware/efi/libstub/x86-stub.c > @@ -673,6 +673,8 @@ unsigned long efi_main(efi_handle_t handle, > unsigned long bzimage_addr = (unsigned long)startup_32; > unsigned long buffer_start, buffer_end; > struct setup_header *hdr = &boot_params->hdr; > + unsigned long cmdline_paddr = ((u64)hdr->cmd_line_ptr | > + ((u64)boot_params->ext_cmd_line_ptr << 32)); > efi_status_t status; > > efi_system_table = sys_table_arg; > @@ -735,6 +737,14 @@ unsigned long efi_main(efi_handle_t handle, > image_offset = 0; > } > > +#ifdef CONFIG_GENERIC_CMDLINE > + status = efi_handle_cmdline((char *)cmdline_paddr); > + if (status != EFI_SUCCESS) { > + efi_err("Failed to parse options\n"); > + goto fail; > + } > +#else /* CONFIG_GENERIC_CMDLINE */ > + > #ifdef CONFIG_CMDLINE_BOOL > status = efi_parse_options(CONFIG_CMDLINE); > if (status != EFI_SUCCESS) { > @@ -743,8 +753,6 @@ unsigned long efi_main(efi_handle_t handle, > } > #endif > if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) { > - unsigned long cmdline_paddr = ((u64)hdr->cmd_line_ptr | > - ((u64)boot_params->ext_cmd_line_ptr << 32)); > status = efi_parse_options((char *)cmdline_paddr); > if (status != EFI_SUCCESS) { > efi_err("Failed to parse options\n"); > @@ -752,6 +760,7 @@ unsigned long efi_main(efi_handle_t handle, > } > } > > +#endif > /* > * At this point, an initrd may already have been loaded by the > * bootloader and passed via bootparams. We permit an initrd loaded > -- > 2.25.1 >