Re: [RFC][PATCH 2/2] arm: grub-install: Default to arm-efi target in EFI-based QEMU virt models

[Leif Lindholm]

On Tue, Aug 28, 2018 at 01:33:48PM -0600, dann frazier wrote:
> From: dann frazier <address@hidden>
> 
> We currently default to the arm-uboot target in grub-install,
> but arm-efi should be used for some systems with UEFI firmware, such as
> Tianocore/EDK2-based QEMU models. We could change the default to arm-efi
> anytime we successfully probe for an EFI runtime. However, that would
> apparently do the wrong thing on U-Boot implementations that implement the
> UEFI spec, such as those compliant with the upcoming Embedded Base Boot
> Requirements specification, as they are not required to support
> SetVariable() in runtime services.
> 
> For now, whitelist QEMU linux/virt models with runtime EFI support,
> but otherwise continue to default to the arm-uboot target.

I don't think this is correct. (And I'm probably the one who gave you
the wrong impression - sorry!)

A U-Boot that implements (even part of) the UEFI specification SHOULD[1]
use uboot-efi. It may well be that some things will not work as
expected on these platforms, but EBBR will define how to detect the
missing functionality and operating systems will need to work around
it according to defined behaviour.

[1] rfc2119 definition.

I expect simply flipping the default to arm-efi _will_ make sense at
some point after EBBR-compliant U-Boot becomes more widespread.

/
    Leif

> Signed-off-by: dann frazier <address@hidden>
> ---
>  grub-core/osdep/basic/platform.c   |  6 +++++
>  grub-core/osdep/linux/platform.c   | 37 ++++++++++++++++++++++++++++++
>  grub-core/osdep/windows/platform.c |  6 +++++
>  include/grub/util/install.h        |  3 +++
>  util/grub-install.c                |  2 +-
>  5 files changed, 53 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
> @@ -18,6 +18,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)
>  { 
> diff --git a/grub-core/osdep/linux/platform.c 
> b/grub-core/osdep/linux/platform.c
> index d278c299b..835aa4856 100644
> --- a/grub-core/osdep/linux/platform.c
> +++ b/grub-core/osdep/linux/platform.c
> @@ -121,6 +121,43 @@ is_efi (void)
>    return 0;
>  }
>  
> +static int
> +is_virt (void)
> +{
> +  FILE *fp;
> +  char *buf = NULL;
> +  int ret = 0;
> +  size_t len = 0;
> +  const char qemu_virt[] = "linux,dummy-virt";
> +
> +  fp = grub_util_fopen ("/sys/firmware/devicetree/base/compatible", "r");
> +  if (fp == NULL)
> +    return 0;
> +
> +  if (getline (&buf, &len, fp) == -1)
> +    {
> +      fclose (fp);
> +      return 0;
> +    }
> +
> +  if (strncmp (buf, qemu_virt, len) == 0)
> +    ret = 1;
> +
> +  free (buf);
> +  fclose (fp);
> +
> +  return ret;
> +}
> +
> +const char *
> +grub_install_get_default_arm_platform (void)
> +{
> +  if (is_efi() && is_virt())
> +      return "arm-efi";
> +
> +  return "arm-uboot";
> +}
> +
>  const char *
>  grub_install_get_default_x86_platform (void)
>  {
> diff --git a/grub-core/osdep/windows/platform.c 
> b/grub-core/osdep/windows/platform.c
> index 58b322887..2dabd1758 100644
> --- a/grub-core/osdep/windows/platform.c
> +++ b/grub-core/osdep/windows/platform.c
> @@ -116,6 +116,12 @@ is_efi (void)
>    return platform == PLAT_EFI;
>  }
>  
> +const char *
> +grub_install_get_default_arm_platform (void)
> +{
> +  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 0dba8b67f..212ada422 100644
> --- a/include/grub/util/install.h
> +++ b/include/grub/util/install.h
> @@ -207,6 +207,9 @@ grub_util_get_target_dirname (const struct 
> grub_install_image_target_desc *t);
>  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);
>  
> diff --git a/util/grub-install.c b/util/grub-install.c
> index 78d0138cb..81bbec778 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.18.0
>