Discussion:
[RFC][PATCH 2/2] arm: grub-install: Default to arm-efi target in EFI-based QEMU virt models
(too old to reply)
dann frazier
2018-08-28 19:33:48 UTC
Permalink
From: dann frazier <***@canonical.com>

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.

Signed-off-by: dann frazier <***@canonical.com>
---
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
dann frazier
2018-08-28 19:33:47 UTC
Permalink
From: dann frazier <***@canonical.com>

No functional change. This will let us re-use the code for ARM.

Signed-off-by: dann frazier <***@canonical.com>
---
grub-core/osdep/linux/platform.c | 20 ++++++++++++++++----
grub-core/osdep/windows/platform.c | 10 ++++++++--
2 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/grub-core/osdep/linux/platform.c b/grub-core/osdep/linux/platform.c
index 775b6c031..d278c299b 100644
--- a/grub-core/osdep/linux/platform.c
+++ b/grub-core/osdep/linux/platform.c
@@ -97,9 +97,9 @@ read_platform_size (void)
return ret;
}

-const char *
-grub_install_get_default_x86_platform (void)
-{
+static int
+is_efi (void)
+{
/*
On Linux, we need the efivars kernel modules.
If no EFI is available this module just does nothing
@@ -114,13 +114,25 @@ grub_install_get_default_x86_platform (void)
if (is_not_empty_directory ("/sys/firmware/efi"))
{
grub_util_info ("...found");
+ return 1;
+ }
+
+ grub_util_info ("... not found.");
+ return 0;
+}
+
+const char *
+grub_install_get_default_x86_platform (void)
+{
+ if (is_efi())
+ {
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/grub-core/osdep/windows/platform.c b/grub-core/osdep/windows/platform.c
index 912269191..58b322887 100644
--- a/grub-core/osdep/windows/platform.c
+++ b/grub-core/osdep/windows/platform.c
@@ -109,13 +109,19 @@ get_platform (void)
return;
}

+static int
+is_efi (void)
+{
+ get_platform ();
+ return platform == PLAT_EFI;
+}
+
const char *
grub_install_get_default_x86_platform (void)
{
SYSTEM_INFO si;

- get_platform ();
- if (platform != PLAT_EFI)
+ if (!is_efi())
return "i386-pc";

/* EFI */
--
2.18.0
Daniel Kiper
2018-09-20 13:47:33 UTC
Permalink
Now that Leif has fixed up the arm-uefi target, I'm looking at adding that
support into Debian/Ubuntu for virtual machines. One issue is that
'grub-install' errors out by default when only the arm-efi flavor is
$ sudo grub-install
grub-install: error: /usr/lib/grub/arm-uboot/modinfo.sh doesn't exist. Please specify --target or --directory
This series avoids that by whitelisting EFI-based linux/virt platforms.
In general LGTM except one nit pick. I will describe it in separate email.

Anyway, I am not ARM expert, so, I will push the patches if Leif is OK with them.
Or at least dose not object.

Leif?

Daniel
Daniel Kiper
2018-09-20 13:50:13 UTC
Permalink
Post by dann frazier
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.
---
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";
Do we really need qemu_virt variable? It is used only in one place.

Daniel
Leif Lindholm
2018-09-20 18:52:49 UTC
Permalink
Post by dann frazier
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
Post by dann frazier
---
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
Continue reading on narkive:
Loading...