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 <address@hidden> 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 <address@hidden>
> >
> > 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.