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.hei...@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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
