Re: [PATCH] grub-install: check for arm-efi as a default target

[Leif Lindholm]

Hi Steve,

On Mon, Feb 11, 2019 at 02:42:34AM +0000, Steve McIntyre wrote:
> Much like on x86, we can work out if the system is running on top of
> EFI firmware. If so, return "arm-efi". If not, fall back to
> "arm-uboot" as previously.

Right, this clearly needs a fix.

> Heavily inspired by the existing code for x86.

Mmm. I would much prefer if we could break out the efi test in a
separate helper function. And clean it up while we're at it.

> Signed-off-by: Steve McIntyre <address@hidden>
> ---
>  grub-core/osdep/basic/platform.c |  6 ++++++
>  grub-core/osdep/linux/platform.c | 24 ++++++++++++++++++++++++
>  include/grub/util/install.h      |  3 +++
>  util/grub-install.c              |  2 +-
>  4 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/grub-core/osdep/basic/platform.c 
> b/grub-core/osdep/basic/platform.c
> index 4b5502aeb..a7dafd85a 100644
> --- a/grub-core/osdep/basic/platform.c
> +++ b/grub-core/osdep/basic/platform.c
> @@ -19,6 +19,12 @@
>  #include <grub/util/install.h>
>  
>  const char *
> +grub_install_get_default_arm_platform (void)
> +{
> +  return "arm-uboot";
> +}
> +
> +const char *
>  grub_install_get_default_x86_platform (void)
>  { 
>    return "i386-pc";
> diff --git a/grub-core/osdep/linux/platform.c 
> b/grub-core/osdep/linux/platform.c
> index 775b6c031..a3f9e9d28 100644
> --- a/grub-core/osdep/linux/platform.c
> +++ b/grub-core/osdep/linux/platform.c
> @@ -98,6 +98,30 @@ read_platform_size (void)
>  }
>  
>  const char *
> +grub_install_get_default_arm_platform (void)
> +{
> +  /*
> +     On Linux, we need the efivars kernel modules.
> +     If no EFI is available this module just does nothing
> +     besides a small hello and if we detect efi we'll load it
> +     anyway later. So it should be safe to
> +     try to load it here.
> +   */
> +  grub_util_exec_redirect_all ((const char * []){ "modprobe", "efivars", 
> NULL },
> +                            NULL, NULL, "/dev/null");

So, I guess this is a "safe" operation. But efivars isn't the
interface that should be used these days - efivarfs is.
The efivars library will always use efivarfs
(mounted on /sys/firmware/efi/efivars) in preference to efivars
(available through /sys/firmware/efi/vars).

Since it's safe, we may leave it in for someone running bleeding edge
grub on an ancient system, or just using a misconfigured kernel.
But the comment is misleading. I would suggest changing it to
something like:

  /*
      Linux uses efivarfs (mounted on /sys/firmware/efi/efivars)
      to access the EFI variable store.
      Some legacy systems may still use the deprecated efivars
      interface (accessed through /sys/firmware/efi/vars).
      Where both are present, libefivar will use the former in
      preference, so attempting to load efivars will not interfere
      with later operations.
  */

/
    Leif

> +  grub_util_info ("Looking for /sys/firmware/efi ..");
> +  if (is_not_empty_directory ("/sys/firmware/efi"))
> +    {
> +      grub_util_info ("...found");
> +      return "arm-efi";
> +    }
> +
> +  grub_util_info ("... not found");
> +  return "arm-uboot";
> +}
> +
> +const char *
>  grub_install_get_default_x86_platform (void)
>  { 
>    /*
> diff --git a/include/grub/util/install.h b/include/grub/util/install.h
> index af2bf65d7..80a51fcb1 100644
> --- a/include/grub/util/install.h
> +++ b/include/grub/util/install.h
> @@ -209,6 +209,9 @@ void
>  grub_install_create_envblk_file (const char *name);
>  
>  const char *
> +grub_install_get_default_arm_platform (void);
> +
> +const char *
>  grub_install_get_default_x86_platform (void);
>  
>  int
> diff --git a/util/grub-install.c b/util/grub-install.c
> index 4a0a66168..1d68cc5bb 100644
> --- a/util/grub-install.c
> +++ b/util/grub-install.c
> @@ -319,7 +319,7 @@ get_default_platform (void)
>  #elif defined (__ia64__)
>     return "ia64-efi";
>  #elif defined (__arm__)
> -   return "arm-uboot";
> +   return grub_install_get_default_arm_platform ();
>  #elif defined (__aarch64__)
>     return "arm64-efi";
>  #elif defined (__amd64__) || defined (__x86_64__) || defined (__i386__)
> -- 
> 2.11.0
> 
> 
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel