Re: [PATCH 1/2] loader/efi/fdt: Add fdtdump command to access device tree

[Daniel Kiper]

On Fri, Jun 14, 2024 at 06:26:00PM +0200, Tobias Heider wrote:
> On Fri, Jun 14, 2024 at 06:03:23PM +0200, Daniel Kiper wrote:
> > On Wed, Jun 12, 2024 at 01:12:28PM +0200, Tobias Heider wrote:
> > > The fdtdump command allows dumping arbitrary device tree properties
> > > and saving them to a variable similar to the smbios command.
> > >
> > > This is useful in scripts where further actions such as selecting a
> > > kernel or loading another device tree depend on the compatible or
> > > model values of the device tree provided by the firmware.
> > >
> > > For now only the root level properties of the dtb are exposed.
> > >
> > > Signed-off-by: Tobias Heider <tobias.heider@canonical.com>
> > > ---
> > >  grub-core/loader/efi/fdt.c | 51 +++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 50 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/grub-core/loader/efi/fdt.c b/grub-core/loader/efi/fdt.c
> > > index 439964b9c..8fa0b3b09 100644
> > > --- a/grub-core/loader/efi/fdt.c
> > > +++ b/grub-core/loader/efi/fdt.c
> > > @@ -1,6 +1,7 @@
> > >  /*
> > >   *  GRUB  --  GRand Unified Bootloader
> > >   *  Copyright (C) 2013-2015  Free Software Foundation, Inc.
> > > + *  Copyright (C) 2024       Canonical, Ltd.
> > >   *
> > >   *  GRUB is free software: you can redistribute it and/or modify
> > >   *  it under the terms of the GNU General Public License as published by
> > > @@ -18,15 +19,18 @@
> > >
> > >  #include <grub/fdt.h>
> > >  #include <grub/mm.h>
> > > +#include <grub/env.h>
> > >  #include <grub/err.h>
> > >  #include <grub/dl.h>
> > >  #include <grub/command.h>
> > > +#include <grub/extcmd.h>
> > >  #include <grub/file.h>
> > >  #include <grub/efi/efi.h>
> > >  #include <grub/efi/fdtload.h>
> > >  #include <grub/efi/memory.h>
> > >  #include <grub/cpu/efi/memory.h>
> > >
> > > +static void *fw_fdt;
> > >  static void *loaded_fdt;
> > >  static void *fdt;
> > >
> > > @@ -36,6 +40,13 @@ static void *fdt;
> > >                               sizeof (FDT_ADDR_CELLS_STRING) + \
> > >                               sizeof (FDT_SIZE_CELLS_STRING))
> > >
> > > +static const struct grub_arg_option options_fdtdump[] = {
> > > +  {"prop",       'p', 0, N_("Get property."), N_("prop"), 
> > > ARG_TYPE_STRING},
> > > +  {"set",       '\0', 0, N_("Store the value in the given variable 
> > > name."),
> > > +                         N_("variable"), ARG_TYPE_STRING},
> > > +  {0, 0, 0, 0, 0, 0}
> > > +};
> > > +
> > >  void *
> > >  grub_fdt_load (grub_size_t additional_size)
> > >  {
> > > @@ -51,7 +62,7 @@ grub_fdt_load (grub_size_t additional_size)
> > >    if (loaded_fdt)
> > >      raw_fdt = loaded_fdt;
> > >    else
> > > -    raw_fdt = grub_efi_get_firmware_fdt();
> > > +    raw_fdt = fw_fdt;
> >
> > This change seems suspicious for me. Could you explain why it is needed?
>
> Since I added grub_efi_get_firmware_fdt() to module init function and the
> device tree passed by the firmware is a fairly static property it made
> sense to me to use the address we have instead of rereading it in 
> grub_fdt_load().
>
> If you are more comfortable if I don't touch that code path I can drop that
> change and simply read it twice.

I am OK with this grub_efi_get_firmware_fdt() shuffling but it has to be
explained in the commit message.

Daniel