[PATCH] 10_linux: Add devicetree command, if a dtb is present.

[PATCH] 10_linux: Add devicetree command, if a dtb is present.

[Dimitri John Ledkov]

Specifically support dtb paths as created by flash-kernel package on
Debian/Ubuntu systems, and a generic one. Possibly this needs to be
extended to support other devicetree blob paths as installed on other
distributions (similar to how multiple initrd paths are
supported).

This is particularly useful during new platforms bring up, which may
not yet be fully supported by Linux kernel. Currently I have tested
this patch, on top of Debian grub2 packaging/distro-patches to
successfully boot Asus Novago TP3700QL. Similarly other laptops would
find this useful, like Lenovo Mixx 630, Lenovo Yoga C630, and HP Envy
X2.

Signed-off-by: Dimitri John Ledkov <xnox@ubuntu.com>
---
 util/grub.d/10_linux.in | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/util/grub.d/10_linux.in b/util/grub.d/10_linux.in
index 4532266be..bb6c8912f 100644
--- a/util/grub.d/10_linux.in
+++ b/util/grub.d/10_linux.in
@@ -153,6 +153,13 @@ EOF
     sed "s/^/$submenu_indentation/" << EOF
 	echo	'$(echo "$message" | grub_quote)'
 	initrd	$(echo $initrd_path)
+EOF
+  fi
+  if test -n "${dtb}" ; then
+    message="$(gettext_printf "Loading device tree blob...")"
+    sed "s/^/$submenu_indentation/" << EOF
+	echo	'$(echo "$message" | grub_quote)'
+	devicetree  ${rel_dirname}/${dtb}
 EOF
   fi
   sed "s/^/$submenu_indentation/" << EOF
@@ -236,6 +243,14 @@ while [ "x$list" != "x" ] ; do
     gettext_printf "Found initrd image: %s\n" "$(echo $initrd_display)" >&2
   fi
 
+  dtb=
+  for i in "dtb-${version}" "dtb-${alt_version}" "dtb"; do
+    if test -e "${dirname}/${i}" ; then
+      dtb="$i"
+      break
+    fi
+  done
+
   config=
   for i in "${dirname}/config-${version}" "${dirname}/config-${alt_version}" "/etc/kernels/kernel-config-${version}" ; do
     if test -e "${i}" ; then
-- 
2.20.1

Re: [PATCH] 10_linux: Add devicetree command, if a dtb is present.

[Leif Lindholm]

On Tue, May 28, 2019 at 11:27:08AM +0200, Daniel Kiper wrote:
> On Thu, May 23, 2019 at 10:31:13PM +0100, Dimitri John Ledkov wrote:
> > Specifically support dtb paths as created by flash-kernel package on
> > Debian/Ubuntu systems, and a generic one. Possibly this needs to be
> > extended to support other devicetree blob paths as installed on other
> > distributions (similar to how multiple initrd paths are
> > supported).
> >
> > This is particularly useful during new platforms bring up, which may
> > not yet be fully supported by Linux kernel. Currently I have tested
> > this patch, on top of Debian grub2 packaging/distro-patches to
> > successfully boot Asus Novago TP3700QL. Similarly other laptops would
> > find this useful, like Lenovo Mixx 630, Lenovo Yoga C630, and HP Envy
> > X2.
> >
> > Signed-off-by: Dimitri John Ledkov <xnox@ubuntu.com>
> 
> LGTM but I would like to hear Leif and/or Alex (CC-ed) opinion here.

This is not my preferred solution to this problem (Dimitri, I'll ping
you offline regarding this).

Also it is fundamentally incompatible with UEFI Secure Boot (see
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=927888).

We should probably look into disabling the devicetree command by
default for the UEFI port. I'll revisit that point post release.

Regards,

Leif

> And this is not a release material.
> 
> Daniel
> 
> > ---
> >  util/grub.d/10_linux.in | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/util/grub.d/10_linux.in b/util/grub.d/10_linux.in
> > index 4532266be..bb6c8912f 100644
> > --- a/util/grub.d/10_linux.in
> > +++ b/util/grub.d/10_linux.in
> > @@ -153,6 +153,13 @@ EOF
> >      sed "s/^/$submenu_indentation/" << EOF
> >  	echo	'$(echo "$message" | grub_quote)'
> >  	initrd	$(echo $initrd_path)
> > +EOF
> > +  fi
> > +  if test -n "${dtb}" ; then
> > +    message="$(gettext_printf "Loading device tree blob...")"
> > +    sed "s/^/$submenu_indentation/" << EOF
> > +	echo	'$(echo "$message" | grub_quote)'
> > +	devicetree  ${rel_dirname}/${dtb}
> >  EOF
> >    fi
> >    sed "s/^/$submenu_indentation/" << EOF
> > @@ -236,6 +243,14 @@ while [ "x$list" != "x" ] ; do
> >      gettext_printf "Found initrd image: %s\n" "$(echo $initrd_display)" >&2
> >    fi
> >
> > +  dtb=
> > +  for i in "dtb-${version}" "dtb-${alt_version}" "dtb"; do
> > +    if test -e "${dirname}/${i}" ; then
> > +      dtb="$i"
> > +      break
> > +    fi
> > +  done
> > +
> >    config=
> >    for i in "${dirname}/config-${version}" "${dirname}/config-${alt_version}" "/etc/kernels/kernel-config-${version}" ; do
> >      if test -e "${i}" ; then
> > --
> > 2.20.1

Re: [PATCH] 10_linux: Add devicetree command, if a dtb is present.

[Dimitri John Ledkov]

On Tue, 28 May 2019 at 12:01, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Tue, May 28, 2019 at 11:27:08AM +0200, Daniel Kiper wrote:
> > On Thu, May 23, 2019 at 10:31:13PM +0100, Dimitri John Ledkov wrote:
> > > Specifically support dtb paths as created by flash-kernel package on
> > > Debian/Ubuntu systems, and a generic one. Possibly this needs to be
> > > extended to support other devicetree blob paths as installed on other
> > > distributions (similar to how multiple initrd paths are
> > > supported).
> > >
> > > This is particularly useful during new platforms bring up, which may
> > > not yet be fully supported by Linux kernel. Currently I have tested
> > > this patch, on top of Debian grub2 packaging/distro-patches to
> > > successfully boot Asus Novago TP3700QL. Similarly other laptops would
> > > find this useful, like Lenovo Mixx 630, Lenovo Yoga C630, and HP Envy
> > > X2.
> > >
> > > Signed-off-by: Dimitri John Ledkov <xnox@ubuntu.com>
> >
> > LGTM but I would like to hear Leif and/or Alex (CC-ed) opinion here.
>
> This is not my preferred solution to this problem (Dimitri, I'll ping
> you offline regarding this).
>
> Also it is fundamentally incompatible with UEFI Secure Boot (see
> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=927888).
>
> We should probably look into disabling the devicetree command by
> default for the UEFI port. I'll revisit that point post release.
>

Yes, in an ideal world all devices would boot correctly with
SecureBoot enforced, using plain ACPI and no dtbs would be needed and
no devicetree command would be needed in grub at all. And when bugs in
ACPI are discovered vendors would issue firmware updates promptly that
would get applied by users in a timely manner using fwupd mechanism /
uefi capsules / windows updater.

In practice, I have had to patch dtbs on Intel NUCs, arm64 servers and
laptops. I suspect we will experience broken ACPI on other devices and
architectures in the future too (RISC-V comes to mind). This is an
unsecureboot crutch, that works in concert with other crutches already
available in the distros (i.e. at package upgrade time copying things
to /boot & generating matching sets of kernel/initrd/dtb). And makes
crutches look like a useful matching set, and works today. And yes,
this is not the preferred way of doing things. But I hope it will be
useful, however temporary, to whomever is working on new devices
brings-ups / iterating kernels&dtbs. In those cases automatic
integration of devicetree command in grub.cfg generator would be a
nice touch.

The alternative proposal would probably still end up reading the dtbs
from these very same paths, if we expect to ship dtbs in distro's and
make them upgradable using package managers. Unless they are
statically embeded / signed in the alternative utility to poke them
into UEFI environment.

Originally I submitted this as a distro patch to Debian, with
maintainer requesting it to be forwarded upstream - as this is a
universally general feature, rather than something distro specific:
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=929427


Regards,

Dimitri.


> Regards,
>
> Leif
>
> > And this is not a release material.
> >
> > Daniel
> >
> > > ---
> > >  util/grub.d/10_linux.in | 15 +++++++++++++++
> > >  1 file changed, 15 insertions(+)
> > >
> > > diff --git a/util/grub.d/10_linux.in b/util/grub.d/10_linux.in
> > > index 4532266be..bb6c8912f 100644
> > > --- a/util/grub.d/10_linux.in
> > > +++ b/util/grub.d/10_linux.in
> > > @@ -153,6 +153,13 @@ EOF
> > >      sed "s/^/$submenu_indentation/" << EOF
> > >     echo    '$(echo "$message" | grub_quote)'
> > >     initrd  $(echo $initrd_path)
> > > +EOF
> > > +  fi
> > > +  if test -n "${dtb}" ; then
> > > +    message="$(gettext_printf "Loading device tree blob...")"
> > > +    sed "s/^/$submenu_indentation/" << EOF
> > > +   echo    '$(echo "$message" | grub_quote)'
> > > +   devicetree  ${rel_dirname}/${dtb}
> > >  EOF
> > >    fi
> > >    sed "s/^/$submenu_indentation/" << EOF
> > > @@ -236,6 +243,14 @@ while [ "x$list" != "x" ] ; do
> > >      gettext_printf "Found initrd image: %s\n" "$(echo $initrd_display)" >&2
> > >    fi
> > >
> > > +  dtb=
> > > +  for i in "dtb-${version}" "dtb-${alt_version}" "dtb"; do
> > > +    if test -e "${dirname}/${i}" ; then
> > > +      dtb="$i"
> > > +      break
> > > +    fi
> > > +  done
> > > +
> > >    config=
> > >    for i in "${dirname}/config-${version}" "${dirname}/config-${alt_version}" "/etc/kernels/kernel-config-${version}" ; do
> > >      if test -e "${i}" ; then
> > > --
> > > 2.20.1



-- 
Regards,

Dimitri.