Re: [PATCH 6/8] drivers: firmware: efi: libstub: enable generic commandline

On Wed, Mar 31, 2021 at 06:10:08PM +0200, Ard Biesheuvel wrote:

(+ Arvind)

On Tue, 30 Mar 2021 at 19:57, Daniel Walker [off-list ref] wrote:

quoted

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 <redacted>
---
 .../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 <linux/efi.h>
 #include <linux/kernel.h>
 #include <linux/printk.h> /* For CONSOLE_LOGLEVEL_* */
+#include <linux/cmdline.h>
 #include <asm/efi.h>
 #include <asm/setup.h>

@@ -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 ?

quoted

+
+       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)) ?

quoted

--- 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