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

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

[Steve McIntyre]

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.

Heavily inspired by the existing code for x86.

Signed-off-by: Steve McIntyre <93sam@debian.org>
---
 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");
+
+  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

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

[Steve McIntyre]

Ping?

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.
>
>Heavily inspired by the existing code for x86.
>
>Signed-off-by: Steve McIntyre <93sam@debian.org>
>---
> 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");
>+
>+  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
>
>
-- 
Steve McIntyre, Cambridge, UK.                                steve@einval.com
"I used to be the first kid on the block wanting a cranial implant,
 now I want to be the first with a cranial firewall. " -- Charlie Stross

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 <93sam@debian.org>
> ---
>  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
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

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

[dann frazier]

On Thu, Feb 21, 2019 at 12:42 PM Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> 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.

fyi, I made an attempt at this a while back:
http://lists.gnu.org/archive/html/grub-devel/2018-08/msg00082.html
http://lists.gnu.org/archive/html/grub-devel/2018-08/msg00081.html
http://lists.gnu.org/archive/html/grub-devel/2018-08/msg00083.html

  -dann

> > Signed-off-by: Steve McIntyre <93sam@debian.org>
> > ---
> >  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
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

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

[Steve McIntyre]

Hey Leif,

On Thu, Feb 21, 2019 at 11:41:10AM +0000, Leif Lindholm wrote:
>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.

Yup!

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

ACK. I was looking for a quick change, but I'll be less lazy. :-)

...

>>  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.
>  */

ACK. Using your text. :-)

Patch v2 on its way in a mo.

-- 
Steve McIntyre, Cambridge, UK.                                steve@einval.com
We don't need no education.
We don't need no thought control.

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

[Steve McIntyre]

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.

Split out the code to (maybe) load the efivar module and check for
/sys/firmware/efi into a common helper routine is_efi_system()

Signed-off-by: Steve McIntyre <93sam@debian.org>
---
 grub-core/osdep/basic/platform.c |  6 ++++++
 grub-core/osdep/linux/platform.c | 46 +++++++++++++++++++++++++++++++---------
 include/grub/util/install.h      |  3 +++
 util/grub-install.c              |  2 +-
 4 files changed, 46 insertions(+), 11 deletions(-)

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..5cb7bf923 100644
--- a/grub-core/osdep/linux/platform.c
+++ b/grub-core/osdep/linux/platform.c
@@ -97,16 +97,19 @@ read_platform_size (void)
   return ret;
 }
 
-const char *
-grub_install_get_default_x86_platform (void)
-{ 
+/* Are we running on an EFI-based system? */
+static int
+is_efi_system (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.
-   */
+     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.
+  */
   grub_util_exec_redirect_all ((const char * []){ "modprobe", "efivars", NULL },
 			       NULL, NULL, "/dev/null");
 
@@ -114,13 +117,36 @@ grub_install_get_default_x86_platform (void)
   if (is_not_empty_directory ("/sys/firmware/efi"))
     {
       grub_util_info ("...found");
+      return 1;
+    }
+  else
+    {
+      grub_util_info ("... not found");
+      return 0;
+    }
+}
+
+const char *
+grub_install_get_default_arm_platform (void)
+{
+  if (is_efi_system())
+    return "arm-efi";
+  else
+    return "arm-uboot";
+}
+
+const char *
+grub_install_get_default_x86_platform (void)
+{ 
+  if (is_efi_system())
+    {
       if (read_platform_size() == 64)
 	return "x86_64-efi";
       else
 	return "i386-efi";
     }
 
-  grub_util_info ("... not found. Looking for /proc/device-tree ..");
+  grub_util_info ("Looking for /proc/device-tree ..");
   if (is_not_empty_directory ("/proc/device-tree"))
     {
       grub_util_info ("...found");
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

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

[Steve McIntyre]

On Thu, Feb 21, 2019 at 03:23:55PM +0100, dann frazier wrote:
>On Thu, Feb 21, 2019 at 12:42 PM Leif Lindholm <leif.lindholm@linaro.org> wrote:
>>
>> 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.
>
>fyi, I made an attempt at this a while back:
>http://lists.gnu.org/archive/html/grub-devel/2018-08/msg00082.html
>http://lists.gnu.org/archive/html/grub-devel/2018-08/msg00081.html
>http://lists.gnu.org/archive/html/grub-devel/2018-08/msg00083.html

Arg, sorry - never saw those or I'd have commented already. :-/

Looking now:

 * I'm not sure I'd bother with the is_efi() change in the
   Windows-specific code but meh. :-)

 * You're only allowing arm-efi as a default when is_virt() is
   true. While I'm also *initially* caring about armhf VMs myself, I
   think that changes in recent U-Boot mean that arm-efi is a sensible
   option on bare metal too?

-- 
Steve McIntyre, Cambridge, UK.                                steve@einval.com
You lock the door
And throw away the key
There's someone in my head but it's not me 

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

[Leif Lindholm]

On Thu, Feb 21, 2019 at 02:53:48PM +0000, Steve McIntyre wrote:
> >> 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.
> >
> >fyi, I made an attempt at this a while back:
> >http://lists.gnu.org/archive/html/grub-devel/2018-08/msg00082.html
> >http://lists.gnu.org/archive/html/grub-devel/2018-08/msg00081.html
> >http://lists.gnu.org/archive/html/grub-devel/2018-08/msg00083.html
> 
> Arg, sorry - never saw those or I'd have commented already. :-/
> 
> Looking now:
> 
>  * I'm not sure I'd bother with the is_efi() change in the
>    Windows-specific code but meh. :-)
> 
>  * You're only allowing arm-efi as a default when is_virt() is
>    true. While I'm also *initially* caring about armhf VMs myself, I
>    think that changes in recent U-Boot mean that arm-efi is a sensible
>    option on bare metal too?

My take is that at some point _after_ the next release of GRUB, we
should consider switching the default for arm. And at some later point
we should probably nuke the U-Boot port completely.

The U-Boot API is not properly maintained, and the UEFI API in U-Boot
is.

/
    Leif

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

[Leif Lindholm]

On Thu, Feb 21, 2019 at 02:46:11PM +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.
> 
> Split out the code to (maybe) load the efivar module and check for
> /sys/firmware/efi into a common helper routine is_efi_system()
> 
> Signed-off-by: Steve McIntyre <93sam@debian.org>

LGTM:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  grub-core/osdep/basic/platform.c |  6 ++++++
>  grub-core/osdep/linux/platform.c | 46 +++++++++++++++++++++++++++++++---------
>  include/grub/util/install.h      |  3 +++
>  util/grub-install.c              |  2 +-
>  4 files changed, 46 insertions(+), 11 deletions(-)
> 
> 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..5cb7bf923 100644
> --- a/grub-core/osdep/linux/platform.c
> +++ b/grub-core/osdep/linux/platform.c
> @@ -97,16 +97,19 @@ read_platform_size (void)
>    return ret;
>  }
>  
> -const char *
> -grub_install_get_default_x86_platform (void)
> -{ 
> +/* Are we running on an EFI-based system? */
> +static int
> +is_efi_system (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.
> -   */
> +     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.
> +  */
>    grub_util_exec_redirect_all ((const char * []){ "modprobe", "efivars", NULL },
>  			       NULL, NULL, "/dev/null");
>  
> @@ -114,13 +117,36 @@ grub_install_get_default_x86_platform (void)
>    if (is_not_empty_directory ("/sys/firmware/efi"))
>      {
>        grub_util_info ("...found");
> +      return 1;
> +    }
> +  else
> +    {
> +      grub_util_info ("... not found");
> +      return 0;
> +    }
> +}
> +
> +const char *
> +grub_install_get_default_arm_platform (void)
> +{
> +  if (is_efi_system())
> +    return "arm-efi";
> +  else
> +    return "arm-uboot";
> +}
> +
> +const char *
> +grub_install_get_default_x86_platform (void)
> +{ 
> +  if (is_efi_system())
> +    {
>        if (read_platform_size() == 64)
>  	return "x86_64-efi";
>        else
>  	return "i386-efi";
>      }
>  
> -  grub_util_info ("... not found. Looking for /proc/device-tree ..");
> +  grub_util_info ("Looking for /proc/device-tree ..");
>    if (is_not_empty_directory ("/proc/device-tree"))
>      {
>        grub_util_info ("...found");
> 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
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

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

[dann frazier]

On Thu, Feb 21, 2019 at 3:54 PM Steve McIntyre <93sam@debian.org> wrote:
>
> On Thu, Feb 21, 2019 at 03:23:55PM +0100, dann frazier wrote:
> >On Thu, Feb 21, 2019 at 12:42 PM Leif Lindholm <leif.lindholm@linaro.org> wrote:
> >>
> >> 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.
> >
> >fyi, I made an attempt at this a while back:
> >http://lists.gnu.org/archive/html/grub-devel/2018-08/msg00082.html
> >http://lists.gnu.org/archive/html/grub-devel/2018-08/msg00081.html
> >http://lists.gnu.org/archive/html/grub-devel/2018-08/msg00083.html
>
> Arg, sorry - never saw those or I'd have commented already. :-/

No worries. Happy to let you run with it, just wanted to share in case
there was anything worth stealing.

> Looking now:
>
>  * I'm not sure I'd bother with the is_efi() change in the
>    Windows-specific code but meh. :-)

Right, only done for consistency.

>  * You're only allowing arm-efi as a default when is_virt() is
>    true. While I'm also *initially* caring about armhf VMs myself, I
>    think that changes in recent U-Boot mean that arm-efi is a sensible
>    option on bare metal too?

Yeah, at the time I was aware of the efi work on uboot, but it wasn't
clear to me what the right build of GRUB would be in that case, so I
left that for later. And later it has become! :)

  -dann

> --
> Steve McIntyre, Cambridge, UK.                                steve@einval.com
> You lock the door
> And throw away the key
> There's someone in my head but it's not me
>

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

[Daniel Kiper]

On Thu, Feb 21, 2019 at 03:04:26PM +0000, Leif Lindholm wrote:
> On Thu, Feb 21, 2019 at 02:46:11PM +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.
> >
> > Split out the code to (maybe) load the efivar module and check for
> > /sys/firmware/efi into a common helper routine is_efi_system()
> >
> > Signed-off-by: Steve McIntyre <93sam@debian.org>
>
> LGTM:
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

If there are no objections I will push this patch next week.

Daniel

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

[Leif Lindholm]

On Thu, Feb 21, 2019 at 04:31:32PM +0100, Daniel Kiper wrote:
> On Thu, Feb 21, 2019 at 03:04:26PM +0000, Leif Lindholm wrote:
> > On Thu, Feb 21, 2019 at 02:46:11PM +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.
> > >
> > > Split out the code to (maybe) load the efivar module and check for
> > > /sys/firmware/efi into a common helper routine is_efi_system()
> > >
> > > Signed-off-by: Steve McIntyre <93sam@debian.org>
> >
> > LGTM:
> > Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> 
> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
> 
> If there are no objections I will push this patch next week.

Eep.

As pointed out by Colin on IRC - it appears you have pushed Steves
original submission rather than the revised one which I gave my R-b
for.

Could you address?

Regards,

Leif

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

[Daniel Kiper]

On Tue, Feb 26, 2019 at 01:27:21PM +0000, Leif Lindholm wrote:
> On Thu, Feb 21, 2019 at 04:31:32PM +0100, Daniel Kiper wrote:
> > On Thu, Feb 21, 2019 at 03:04:26PM +0000, Leif Lindholm wrote:
> > > On Thu, Feb 21, 2019 at 02:46:11PM +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.
> > > >
> > > > Split out the code to (maybe) load the efivar module and check for
> > > > /sys/firmware/efi into a common helper routine is_efi_system()
> > > >
> > > > Signed-off-by: Steve McIntyre <93sam@debian.org>
> > >
> > > LGTM:
> > > Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> >
> > Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
> >
> > If there are no objections I will push this patch next week.
>
> Eep.
>
> As pointed out by Colin on IRC - it appears you have pushed Steves
> original submission rather than the revised one which I gave my R-b
> for.
>
> Could you address?

Argh... Addressed. Sorry for the confusion. I took first email from
the thread by mistake. Folks, next time please send new patches as
not a reply to. Just create new threads.

Daniel

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

[Steve McIntyre]

On Tue, Feb 26, 2019 at 03:55:47PM +0100, Daniel Kiper wrote:
>On Tue, Feb 26, 2019 at 01:27:21PM +0000, Leif Lindholm wrote:
>>
>> As pointed out by Colin on IRC - it appears you have pushed Steves
>> original submission rather than the revised one which I gave my R-b
>> for.
>>
>> Could you address?
>
>Argh... Addressed. Sorry for the confusion. I took first email from
>the thread by mistake. Folks, next time please send new patches as
>not a reply to. Just create new threads.

ACK, will do!

-- 
Steve McIntyre, Cambridge, UK.                                steve@einval.com
"You can't barbecue lettuce!" -- Ellie Crane