Re: [RFC PATCH] gdb: Add more support for debugging on EFI platforms

[Glenn Washburn]

On 3/16/23 21:05, Robbie Harwood wrote:
> Glenn Washburn <development@efficientek.com> writes:
>
> > On 3/9/23 23:00, Robbie Harwood wrote:
> > > Glenn Washburn <development@efficientek.com> writes:
> > >
> > > > If the configure option --enable-efi-debug is given, then enable the
> > > > printing early in EFI startup of the command needed to load symbols for
> > > > the GRUB EFI kernel. This is needed because EFI firmware determines where
> > > > to load the GRUB EFI at runtime, and so the relevant addresses are not
> > > > known ahead of time. This is not printed when secure boot is enabled.
> > > >
> > > > The command is a custom command defined in the gdb_grub GDB script. So
> > > > GDB should be started with the script as an argument to the -x option or
> > > > sourced into an active GDB session before running the outputted command.
> > > >
> > > > Also a command named "gdbinfo" is enabled which allows the user to print
> > > > the gdb command string on-demand, which can be valuable as the printing
> > > > early in EFI startup is quickly replaced by other text. So if using a
> > > > physical screen it may appear too briefly to be registered.
> > > >
> > > > Co-developed-by: Peter Jones <pjones@redhat.com>
> > > > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > > > ---
> > > > This is patch 9 from the v6 "GDB script fixes and improvements" series, with
> > > > one modification. Now the gdbinfo command will print the gdb load command
> > > > even when the configure option is not enabled (though still not when lockdown
> > > > is enabled).
> > > >
> > > > Robbie had 2 concerns with the last patch.
> > > >
> > > >    1. Does this need to be configurable?
> > > >      * I responded that this was requested by Daniel because of concerns about
> > > >        it breaking silent boot and it seemed reasonable to me, but that I don't
> > > >        have a strong opinion. I've left it configurable until Dnaiel weighs in.
> > >
> > > Yeah, I think these concerns are valid.  The version in the rhboot
> > > tree gates printing on an env var.  Right now, it seems to me that:
> > >
> > > - we want it to be default-off because silent boot
> >
> > I understand you to be talking about a default-off at runtime, not
> > built time. Right now there is a configure option which defaults to
> > off, is this acceptable?
>
> Indeed, I'm talking about runtime configurability.  Build-time
> configurability means it's either always on (bad) or we have to rebuild
> in order to debug (annoying, interacts poorly with scureboot).
>
> > > - we want to have the ability to reenable without rebuilding because
> > >     secureboot, convenience, etc.
> >
> > This would be great, but how do you propose that this would work? This
> > patch will print very early in EFI init. We can't use GRUB variables. We
> > probably could use EFI variables, but this needs to be well defined (and
> > not by me, since I don't have this requirement). I'm not sure if the
> > GRUB env block is available at this point, but that might be an option.
> >
> > Can you point me to RH's patch you've referred to? Does it meet this
> > requirement, and if so how?
>
> I thought you were basing your work off it, given the "Co-developed-by"
> and your review on v1:
> https://lists.gnu.org/archive/html/grub-devel/2021-10/msg00076.html

Yes, I am off that patch. I was thinking things had changed or there 
were other patches to get the requirement met. Thanks for providing the 
info anyway. Looks like nothing has changed and the missing piece is the 
envblk from an EFI variable patch.

> (v2 was
> https://lists.gnu.org/archive/html/grub-devel/2021-11/msg00006.html and
> the versions we're shipping are
> https://github.com/rhboot/grub2/commit/93803e83135e074ed5a3e7f67af22538896dbefe
> and
> https://github.com/rhboot/grub2/commit/c314270222b4adffa16840e3ca764d793e93a0e8
> )
>
> As you say, the grub env block isn't available (module loading happens
> before filesystems).  We have an additional patch that allows the use of
> an EFI variable for setting grubenv:
> https://github.com/rhboot/grub2/commit/22a11bfa59ff8f239cfe8698274281bc919673dc

Looks like the above commit relies on this commit:

https://github.com/rhboot/grub2/commit/636612cec23868d9165d79f2c3e2a9ea6c278c38

> which lets one do e.g.
>
>      grub2-editenv ./grubenv set debug=gdb,modules,dl
>      { printf '\x07\x00\x00\x00' ; cat ./grubenv ; } > 
> GRUB_ENV-91376aff-cba6-42be-949d-06fde81128e8
>      cp GRUB_ENV-91376aff-cba6-42be-949d-06fde81128e8 /sys/firmware/efi/efivars/

A bit off topic, but perhaps this could be wrapped in an option to 
grub2-editenv and doing something like:

  grub2-editenv -E set debug=gdb,modules,dl

> and then have configuration for the next boot.  Javier informally
> proposed this to the list a few years ago for ZFS-related things:
> https://lists.gnu.org/archive/html/grub-devel/2020-02/msg00045.html but
> I don't think it was ever followed up on, possibly because ZFS is not
> one of our use cases :)
>
> I'm happy to formally propose it, or feel free to include/adapt it if
> it's helpful to you, etc..

The envblk EFI variable seems to me to be a nice feature to have, so I'm 
supportive in general. One concern I have is that this efi-envblk 
feature is a non-trivial amount of code for this particular patch to be 
dependent on. So if the efi-envblk is proposed and doesn't get included 
for this release cycle, neither will this patch. I would say yes to 
submitting the efi-envblk patches, regardless of what we do with this 
patch. And I'd like to hear Daniel's thoughts on this.

Glenn