[PATCH 0/2] grub.texi: Document fwsetup and fix typo

[PATCH 0/2] grub.texi: Document fwsetup and fix typo

[Robbie Harwood]

As requested in review of the recent fwsetup support series, document fwsetup
(and its option), and fix a nearby typo I noticed that caused halt's options
to not render correctly.

Be well,
--Robbie

Robbie Harwood (2):
  grub.text: Document fwsetup command
  grub.texi: Fix mismatched brackets in halt command

 docs/grub.texi | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

-- 
2.35.1

[PATCH 1/1] grub.texi: Fix mismatched brackets in halt command

[Robbie Harwood]

Signed-off-by: Robbie Harwood <rharwood@redhat.com>
---
 docs/grub.texi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/grub.texi b/docs/grub.texi
index 949b5a3601..ac1f401a52 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -4800,7 +4800,7 @@ type are omitted, then the partition will be inactive.
 @node halt
 @subsection halt
 
-@deffn Command halt [@option{--no-apm}[
+@deffn Command halt [@option{--no-apm}]
 The command halts the computer. On the i386-pc target, the @option{--no-apm}
 option, or short @option{-n}, is specified, no APM BIOS call is performed.
 Otherwise, the computer is shut down using APM on that target.
-- 
2.35.1

[PATCH 1/2] grub.text: Document fwsetup command

[Robbie Harwood]

Signed-off-by: Robbie Harwood <rharwood@redhat.com>
---
 docs/grub.texi | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/docs/grub.texi b/docs/grub.texi
index b848d0928b..949b5a3601 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -4242,6 +4242,7 @@ you forget a command, you can run the command @command{help}
 * eval::                        Evaluate agruments as GRUB commands
 * export::                      Export an environment variable
 * false::                       Do nothing, unsuccessfully
+* fwsetup::                     Reboot into the firmware setup menu
 * gettext::                     Translate a string
 * gptsync::                     Fill an MBR based on GPT entries
 * halt::                        Shut down your computer
@@ -4754,6 +4755,16 @@ such as @code{if} and @code{while} (@pxref{Shell-like scripting}).
 @end deffn
 
 
+@node fwsetup
+@subsection fwsetup
+
+@deffn Command fwsetup [@option{--is-supported}]
+Reboot into the firmware setup menu.  If @option{--is-supported} option is
+specified, instead check whether the firmware supports a setup menu and
+exit successfully if so.
+@end deffn
+
+
 @node gettext
 @subsection gettext
 
@@ -5939,7 +5950,6 @@ GRUB shell may provide more information on parameters and usage.
 @item @command{fix_video} - Fix video problem.
 @item @command{fpswa} - Display FPSWA version.
 @item @command{functional_test} - Run all loaded functional tests.
-@item @command{fwsetup} - Reboot into firmware setup menu.
 @item @command{gdbstub_break} - Break into GDB
 @item @command{gdbstub} -  Start GDB stub on given port
 @item @command{gdbstub_stop} - Stop GDB stub
-- 
2.35.1

[PATCH 2/2] grub.texi: Fix mismatched brackets in halt command

[Robbie Harwood]

Signed-off-by: Robbie Harwood <rharwood@redhat.com>
---
 docs/grub.texi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/grub.texi b/docs/grub.texi
index 949b5a3601..ac1f401a52 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -4800,7 +4800,7 @@ type are omitted, then the partition will be inactive.
 @node halt
 @subsection halt
 
-@deffn Command halt [@option{--no-apm}[
+@deffn Command halt [@option{--no-apm}]
 The command halts the computer. On the i386-pc target, the @option{--no-apm}
 option, or short @option{-n}, is specified, no APM BIOS call is performed.
 Otherwise, the computer is shut down using APM on that target.
-- 
2.35.1

[PATCH v4 0/5] Improve logic to check for fwsetup support

[Robbie Harwood]

In this version: fix wrong arg parsing in patch to address Glenn's review.

Be well,
--Robbie

Javier Martinez Canillas (2):
  templates: Check for EFI at runtime instead of config generation time
  efi: Print an error if boot to firmware setup is not supported

Robbie Harwood (3):
  commands/efi/efifwsetup: Add missing grub_free()s
  Make all grub_efi_guid_t variables static
  Don't display a uefi-firmware entry if it's not supported

 grub-core/commands/efi/efifwsetup.c  | 30 +++++++++++++++++++---------
 grub-core/efiemu/i386/pc/cfgtables.c |  6 +++---
 grub-core/kern/efi/fdt.c             |  2 +-
 grub-core/loader/efi/fdt.c           |  2 +-
 grub-core/term/efi/console.c         |  2 +-
 util/grub.d/30_uefi-firmware.in      | 21 ++++++++-----------
 6 files changed, 35 insertions(+), 28 deletions(-)

-- 
2.35.1

[PATCH v4 1/5] commands/efi/efifwsetup: Add missing grub_free()s

[Robbie Harwood]

Each call of grub_efi_get_variable() needs a grub_free().

Signed-off-by: Robbie Harwood <rharwood@redhat.com>
---
 grub-core/commands/efi/efifwsetup.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/grub-core/commands/efi/efifwsetup.c b/grub-core/commands/efi/efifwsetup.c
index eaca032838..b5b1f106c6 100644
--- a/grub-core/commands/efi/efifwsetup.c
+++ b/grub-core/commands/efi/efifwsetup.c
@@ -44,6 +44,8 @@ grub_cmd_fwsetup (grub_command_t cmd __attribute__ ((unused)),
   if (old_os_indications != NULL && oi_size == sizeof (os_indications))
     os_indications |= *old_os_indications;
 
+  grub_free (old_os_indications);
+
   status = grub_efi_set_variable ("OsIndications", &global, &os_indications,
 				  sizeof (os_indications));
   if (status != GRUB_ERR_NONE)
@@ -62,17 +64,20 @@ efifwsetup_is_supported (void)
   grub_efi_uint64_t *os_indications_supported = NULL;
   grub_size_t oi_size = 0;
   grub_efi_guid_t global = GRUB_EFI_GLOBAL_VARIABLE_GUID;
+  grub_boolean_t ret = 0;
 
   grub_efi_get_variable ("OsIndicationsSupported", &global, &oi_size,
 			 (void **) &os_indications_supported);
 
   if (!os_indications_supported)
-    return 0;
+    goto done;
 
   if (*os_indications_supported & GRUB_EFI_OS_INDICATIONS_BOOT_TO_FW_UI)
-    return 1;
+    ret = 1;
 
-  return 0;
+ done:
+  grub_free (os_indications_supported);
+  return ret;
 }
 
 GRUB_MOD_INIT (efifwsetup)
-- 
2.35.1

[PATCH v4 2/5] Make all grub_efi_guid_t variables static

[Robbie Harwood]

This is believed to result in smaller code.

Signed-off-by: Robbie Harwood <rharwood@redhat.com>
---
 grub-core/commands/efi/efifwsetup.c  | 4 ++--
 grub-core/efiemu/i386/pc/cfgtables.c | 6 +++---
 grub-core/kern/efi/fdt.c             | 2 +-
 grub-core/loader/efi/fdt.c           | 2 +-
 grub-core/term/efi/console.c         | 2 +-
 5 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/grub-core/commands/efi/efifwsetup.c b/grub-core/commands/efi/efifwsetup.c
index b5b1f106c6..51bb2ace3b 100644
--- a/grub-core/commands/efi/efifwsetup.c
+++ b/grub-core/commands/efi/efifwsetup.c
@@ -36,7 +36,7 @@ grub_cmd_fwsetup (grub_command_t cmd __attribute__ ((unused)),
   grub_efi_uint64_t os_indications = GRUB_EFI_OS_INDICATIONS_BOOT_TO_FW_UI;
   grub_err_t status;
   grub_size_t oi_size;
-  grub_efi_guid_t global = GRUB_EFI_GLOBAL_VARIABLE_GUID;
+  static grub_efi_guid_t global = GRUB_EFI_GLOBAL_VARIABLE_GUID;
 
   grub_efi_get_variable ("OsIndications", &global, &oi_size,
 			 (void **) &old_os_indications);
@@ -63,7 +63,7 @@ efifwsetup_is_supported (void)
 {
   grub_efi_uint64_t *os_indications_supported = NULL;
   grub_size_t oi_size = 0;
-  grub_efi_guid_t global = GRUB_EFI_GLOBAL_VARIABLE_GUID;
+  static grub_efi_guid_t global = GRUB_EFI_GLOBAL_VARIABLE_GUID;
   grub_boolean_t ret = 0;
 
   grub_efi_get_variable ("OsIndicationsSupported", &global, &oi_size,
diff --git a/grub-core/efiemu/i386/pc/cfgtables.c b/grub-core/efiemu/i386/pc/cfgtables.c
index e5fffb7d4a..1098f0b79f 100644
--- a/grub-core/efiemu/i386/pc/cfgtables.c
+++ b/grub-core/efiemu/i386/pc/cfgtables.c
@@ -29,9 +29,9 @@ grub_machine_efiemu_init_tables (void)
 {
   void *table;
   grub_err_t err;
-  grub_efi_guid_t smbios = GRUB_EFI_SMBIOS_TABLE_GUID;
-  grub_efi_guid_t acpi20 = GRUB_EFI_ACPI_20_TABLE_GUID;
-  grub_efi_guid_t acpi = GRUB_EFI_ACPI_TABLE_GUID;
+  static grub_efi_guid_t smbios = GRUB_EFI_SMBIOS_TABLE_GUID;
+  static grub_efi_guid_t acpi20 = GRUB_EFI_ACPI_20_TABLE_GUID;
+  static grub_efi_guid_t acpi = GRUB_EFI_ACPI_TABLE_GUID;
 
   err = grub_efiemu_unregister_configuration_table (smbios);
   if (err)
diff --git a/grub-core/kern/efi/fdt.c b/grub-core/kern/efi/fdt.c
index 30100c61c1..24f955289f 100644
--- a/grub-core/kern/efi/fdt.c
+++ b/grub-core/kern/efi/fdt.c
@@ -24,7 +24,7 @@ void *
 grub_efi_get_firmware_fdt (void)
 {
   grub_efi_configuration_table_t *tables;
-  grub_efi_guid_t fdt_guid = GRUB_EFI_DEVICE_TREE_GUID;
+  static grub_efi_guid_t fdt_guid = GRUB_EFI_DEVICE_TREE_GUID;
   void *firmware_fdt = NULL;
   unsigned int i;
 
diff --git a/grub-core/loader/efi/fdt.c b/grub-core/loader/efi/fdt.c
index c86f283d75..80d7088747 100644
--- a/grub-core/loader/efi/fdt.c
+++ b/grub-core/loader/efi/fdt.c
@@ -86,7 +86,7 @@ grub_err_t
 grub_fdt_install (void)
 {
   grub_efi_boot_services_t *b;
-  grub_efi_guid_t fdt_guid = GRUB_EFI_DEVICE_TREE_GUID;
+  static grub_efi_guid_t fdt_guid = GRUB_EFI_DEVICE_TREE_GUID;
   grub_efi_status_t status;
 
   b = grub_efi_system_table->boot_services;
diff --git a/grub-core/term/efi/console.c b/grub-core/term/efi/console.c
index a3622e4fe5..532948a8e1 100644
--- a/grub-core/term/efi/console.c
+++ b/grub-core/term/efi/console.c
@@ -353,7 +353,7 @@ grub_console_getkeystatus (struct grub_term_input *term)
 static grub_err_t
 grub_efi_console_input_init (struct grub_term_input *term)
 {
-  grub_efi_guid_t text_input_ex_guid =
+  static grub_efi_guid_t text_input_ex_guid =
     GRUB_EFI_SIMPLE_TEXT_INPUT_EX_PROTOCOL_GUID;
 
   if (grub_efi_is_finished)
-- 
2.35.1

[PATCH v4 3/5] templates: Check for EFI at runtime instead of config generation time

[Robbie Harwood]

From: Javier Martinez Canillas <javierm@redhat.com>

The 30_uefi-firmware template checks if an OsIndicationsSupported UEFI var
exists and EFI_OS_INDICATIONS_BOOT_TO_FW_UI bit is set, to decide whether
a "fwsetup" menu entry would be added or not to the GRUB menu.

But this has the problem that it will only work if the configuration file
was created on an UEFI machine that supports booting to a firmware UI.

This for example doesn't support creating GRUB config files when executing
on systems that support both UEFI and legacy BIOS booting. Since creating
the config file from legacy BIOS wouldn't allow to access the firmware UI.

To prevent this, make the template to unconditionally create the grub.cfg
snippet but check at runtime if was booted through UEFI to decide if this
entry should be added. That way it won't be added when booting with BIOS.

There's no need to check if EFI_OS_INDICATIONS_BOOT_TO_FW_UI bit is set,
since that's already done by the "fwsetup" command when is executed.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Signed-off-by: Robbie Harwood <rharwood@redhat.com>
---
 util/grub.d/30_uefi-firmware.in | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/util/grub.d/30_uefi-firmware.in b/util/grub.d/30_uefi-firmware.in
index d344d3883d..b6041b55e2 100644
--- a/util/grub.d/30_uefi-firmware.in
+++ b/util/grub.d/30_uefi-firmware.in
@@ -26,19 +26,14 @@ export TEXTDOMAINDIR="@localedir@"
 
 . "$pkgdatadir/grub-mkconfig_lib"
 
-EFI_VARS_DIR=/sys/firmware/efi/efivars
-EFI_GLOBAL_VARIABLE=8be4df61-93ca-11d2-aa0d-00e098032b8c
-OS_INDICATIONS="$EFI_VARS_DIR/OsIndicationsSupported-$EFI_GLOBAL_VARIABLE"
+LABEL="UEFI Firmware Settings"
 
-if [ -e "$OS_INDICATIONS" ] && \
-   [ "$(( $(printf 0x%x \'"$(cat $OS_INDICATIONS | cut -b5)"\') & 1 ))" = 1 ]; then
-  LABEL="UEFI Firmware Settings"
+gettext_printf "Adding boot menu entry for UEFI Firmware Settings ...\n" >&2
 
-  gettext_printf "Adding boot menu entry for UEFI Firmware Settings ...\n" >&2
-
-  cat << EOF
-menuentry '$LABEL' \$menuentry_id_option 'uefi-firmware' {
-	fwsetup
-}
-EOF
+cat << EOF
+if [ "\$grub_platform" = "efi" ]; then
+	menuentry '$LABEL' \$menuentry_id_option 'uefi-firmware' {
+		fwsetup
+	}
 fi
+EOF
-- 
2.35.1

[PATCH v4 4/5] efi: Print an error if boot to firmware setup is not supported

[Robbie Harwood]

From: Javier Martinez Canillas <javierm@redhat.com>

The "fwsetup" command is only registered if the firmware supports booting
to the firmware setup UI. But it could be possible that the GRUB config
already contains a "fwsetup" entry, because it was generated in a machine
that has support for this feature.

To prevent users getting an error like:

    error: ../../grub-core/script/function.c:109:can't find command `fwsetup'.

if it is not supported by the firmware, let's just always register the
command but print a more accurate message if the firmware doesn't
support this option.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Signed-off-by: Robbie Harwood <rharwood@redhat.com>
---
 grub-core/commands/efi/efifwsetup.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/grub-core/commands/efi/efifwsetup.c b/grub-core/commands/efi/efifwsetup.c
index 51bb2ace3b..cb4b6ff18c 100644
--- a/grub-core/commands/efi/efifwsetup.c
+++ b/grub-core/commands/efi/efifwsetup.c
@@ -27,6 +27,8 @@
 
 GRUB_MOD_LICENSE ("GPLv3+");
 
+static grub_efi_boolean_t efifwsetup_is_supported (void);
+
 static grub_err_t
 grub_cmd_fwsetup (grub_command_t cmd __attribute__ ((unused)),
 		  int argc __attribute__ ((unused)),
@@ -38,6 +40,10 @@ grub_cmd_fwsetup (grub_command_t cmd __attribute__ ((unused)),
   grub_size_t oi_size;
   static grub_efi_guid_t global = GRUB_EFI_GLOBAL_VARIABLE_GUID;
 
+  if (!efifwsetup_is_supported ())
+	  return grub_error (GRUB_ERR_INVALID_COMMAND,
+			     N_("reboot to firmware setup is not supported by the current firmware"));
+
   grub_efi_get_variable ("OsIndications", &global, &oi_size,
 			 (void **) &old_os_indications);
 
@@ -82,10 +88,8 @@ efifwsetup_is_supported (void)
 
 GRUB_MOD_INIT (efifwsetup)
 {
-  if (efifwsetup_is_supported ())
-    cmd = grub_register_command ("fwsetup", grub_cmd_fwsetup, NULL,
-				 N_("Reboot into firmware setup menu."));
-
+  cmd = grub_register_command ("fwsetup", grub_cmd_fwsetup, NULL,
+                               N_("Reboot into firmware setup menu."));
 }
 
 GRUB_MOD_FINI (efifwsetup)
-- 
2.35.1

[PATCH v4 5/5] Don't display a uefi-firmware entry if it's not supported

[Robbie Harwood]

Add a new --is-supported option to commands/efi/efifwsetup and
conditionalize display on it.

Signed-off-by: Robbie Harwood <rharwood@redhat.com>
---
 grub-core/commands/efi/efifwsetup.c | 3 +++
 util/grub.d/30_uefi-firmware.in     | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/grub-core/commands/efi/efifwsetup.c b/grub-core/commands/efi/efifwsetup.c
index cb4b6ff18c..53d23deea7 100644
--- a/grub-core/commands/efi/efifwsetup.c
+++ b/grub-core/commands/efi/efifwsetup.c
@@ -40,6 +40,9 @@ grub_cmd_fwsetup (grub_command_t cmd __attribute__ ((unused)),
   grub_size_t oi_size;
   static grub_efi_guid_t global = GRUB_EFI_GLOBAL_VARIABLE_GUID;
 
+  if (argc >= 1 && grub_strcmp(args[0], "--is-supported") == 0)
+    return !efifwsetup_is_supported ();
+
   if (!efifwsetup_is_supported ())
 	  return grub_error (GRUB_ERR_INVALID_COMMAND,
 			     N_("reboot to firmware setup is not supported by the current firmware"));
diff --git a/util/grub.d/30_uefi-firmware.in b/util/grub.d/30_uefi-firmware.in
index b6041b55e2..78aef67d78 100644
--- a/util/grub.d/30_uefi-firmware.in
+++ b/util/grub.d/30_uefi-firmware.in
@@ -31,7 +31,7 @@ LABEL="UEFI Firmware Settings"
 gettext_printf "Adding boot menu entry for UEFI Firmware Settings ...\n" >&2
 
 cat << EOF
-if [ "\$grub_platform" = "efi" ]; then
+if [ "\$grub_platform" = "efi" ] && fwsetup --is-supported; then
 	menuentry '$LABEL' \$menuentry_id_option 'uefi-firmware' {
 		fwsetup
 	}
-- 
2.35.1

Re: [PATCH v4 0/5] Improve logic to check for fwsetup support

[Daniel Kiper]

On Fri, Aug 19, 2022 at 04:54:39PM -0400, Robbie Harwood wrote:
> In this version: fix wrong arg parsing in patch to address Glenn's review.
>
> Be well,
> --Robbie
>
> Javier Martinez Canillas (2):
>   templates: Check for EFI at runtime instead of config generation time
>   efi: Print an error if boot to firmware setup is not supported
>
> Robbie Harwood (3):
>   commands/efi/efifwsetup: Add missing grub_free()s
>   Make all grub_efi_guid_t variables static
>   Don't display a uefi-firmware entry if it's not supported
>
>  grub-core/commands/efi/efifwsetup.c  | 30 +++++++++++++++++++---------
>  grub-core/efiemu/i386/pc/cfgtables.c |  6 +++---
>  grub-core/kern/efi/fdt.c             |  2 +-
>  grub-core/loader/efi/fdt.c           |  2 +-
>  grub-core/term/efi/console.c         |  2 +-
>  util/grub.d/30_uefi-firmware.in      | 21 ++++++++-----------
>  6 files changed, 35 insertions(+), 28 deletions(-)

It seems to me you sent this once again by mistake...

Daniel