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.6 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,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 44386C433ED for ; Wed, 31 Mar 2021 18:22:25 +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 AB9F861003 for ; Wed, 31 Mar 2021 18:22:24 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org AB9F861003 Authentication-Results: mail.kernel.org; dmarc=fail (p=quarantine dis=none) header.from=cisco.com 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 4F9ZRM0LBxz3btF for ; Thu, 1 Apr 2021 05:22:23 +1100 (AEDT) Authentication-Results: lists.ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=cisco.com header.i=@cisco.com header.a=rsa-sha256 header.s=iport header.b=Mh3qzQkN; dkim-atps=neutral Authentication-Results: lists.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=cisco.com (client-ip=173.37.86.76; helo=rcdn-iport-5.cisco.com; envelope-from=danielwa@cisco.com; receiver=) Authentication-Results: lists.ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=cisco.com header.i=@cisco.com header.a=rsa-sha256 header.s=iport header.b=Mh3qzQkN; dkim-atps=neutral Received: from rcdn-iport-5.cisco.com (rcdn-iport-5.cisco.com [173.37.86.76]) (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 4F9ZQq5rK8z2yx8 for ; Thu, 1 Apr 2021 05:21:52 +1100 (AEDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=cisco.com; i=@cisco.com; l=3897; q=dns/txt; s=iport; t=1617214915; x=1618424515; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=t8lk/7k5wkWXyrsD9aMvvKA4Te42fmRyrTbrrcgdlHo=; b=Mh3qzQkN8dB8hJlswW8NH363HeUouRgZUl7niJ7bFxHZiP06D+HI9DjW jJg2kJ2KXgMRGT6dUHYLzanpVXq4oOcTyzmajSIP9S0qhDgwlVGlDsHln hUU0W6E4XG/NxrmJR3hDXzGTALoybmXWm8ntenRa+Ce3XDPBNO+Ikt9aI Q=; IronPort-HdrOrdr: =?us-ascii?q?A9a23=3AWKfFL6jA4QpEqfo1THc148MX4HBQX4F13D?= =?us-ascii?q?Abvn1ZSRFFG/GwvcrGppsm/DXzjyscX2xltNCbIa+bQW7d85kd2/h1AZ6JWg?= =?us-ascii?q?76tGy0aLxz9IeK+UyDJwTS/vNQvJ0LT4FQE9v1ZGIWse/b502CH88k0J279s?= =?us-ascii?q?mT9IPj5lNMaS0vVK169Qd+DW+gYy5LbS1LH4AwGpbZxucvnVudUE8aZMi6GX?= =?us-ascii?q?UJNtKrz7b2vanrbhIcCxks5BPmt1OVwYTnGBuV1Ap2aV1y6IolmFKoryXJoo?= =?us-ascii?q?2+rvf+8RPHzmnV9ZgTosf508BOHtbksLlzFhzcziC1eY9mR7qO+Bcyre3H0i?= =?us-ascii?q?dSrPD85zE9Is9093TdOluQnCKo8Qzh3DEygkWSr2OlvQ=3D=3D?= X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: =?us-ascii?q?A0AkAABzvGRg/5FdJa1aGwEBAQEBAQE?= =?us-ascii?q?BBQEBARIBAQEDAwEBAUCBPAYBAQELAYIqgUwBOTGMZYkuA5AIFopFgXwLAQE?= =?us-ascii?q?BDQEBNAQBAYRQAoF7AiU0CQ4CAwEBDAEBBQEBAQIBBgRxhW6GRQEFOj8QCxg?= =?us-ascii?q?uPBsGE4V5qnd1gTSJCIFEFA6BFwGNSSYcgUlChC4+ijYEgkAGAXsUgmWQfwa?= =?us-ascii?q?CdopQgSCZb4EUgxGBI5s5MRCkSLgbAgQGBQIWgVQ6gVkzGggbFYMkUBkNjis?= =?us-ascii?q?WjkchAy84AgYKAQEDCY8JAQE?= X-IronPort-AV: E=Sophos;i="5.81,293,1610409600"; d="scan'208";a="609843726" Received: from rcdn-core-9.cisco.com ([173.37.93.145]) by rcdn-iport-5.cisco.com with ESMTP/TLS/DHE-RSA-SEED-SHA; 31 Mar 2021 18:21:48 +0000 Received: from zorba ([10.24.8.227]) by rcdn-core-9.cisco.com (8.15.2/8.15.2) with ESMTPS id 12VILjhE015710 (version=TLSv1.2 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Wed, 31 Mar 2021 18:21:47 GMT Date: Wed, 31 Mar 2021 11:21:45 -0700 From: Daniel Walker To: Ard Biesheuvel Subject: Re: [PATCH 6/8] drivers: firmware: efi: libstub: enable generic commandline Message-ID: <20210331182145.GJ2469518@zorba> References: <41021d66db2ab427c14255d2a24bb4517c8b58fd.1617126961.git.danielwa@cisco.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Auto-Response-Suppress: DR, OOF, AutoReply X-Outbound-SMTP-Client: 10.24.8.227, [10.24.8.227] X-Outbound-Node: rcdn-core-9.cisco.com 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 , Arvind Sankar , 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" On Wed, Mar 31, 2021 at 06:10:08PM +0200, Ard Biesheuvel wrote: > (+ 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? How about efi_status_t status = 0; status |= efi_parse_options(CMDLINE_PREPEND); then error checking once ? > > + > > + status = efi_parse_options(IS_ENABLED(CONFIG_CMDLINE_OVERRIDE) ? "" : cmdline); > > What is the point of calling efi_parse_options() with an empty string? I could change it to if ((IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) ? > > --- 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. It does replace extend and force. I was under the impression this code was shared between arm64 and arm32. If that's not the case I can delete the extend and force section. I haven't submitted a conversion for arm32 yet. Daniel