| Message ID | 20190211024234.6975-1-93sam@debian.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | grub-install: check for arm-efi as a default target | expand |
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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
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. _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
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 > _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
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__)
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(-) -- 2.11.0 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel