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

[Daniel Kiper]

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?

>    if (raw_fdt)
>        size = grub_fdt_get_totalsize (raw_fdt);
> @@ -167,10 +178,47 @@ out:
>    return grub_errno;
>  }
>
> +static grub_err_t
> +grub_cmd_fdtdump (grub_extcmd_context_t ctxt,
> +                 int argc __attribute__ ((unused)),
> +                 char **argv __attribute__ ((unused)))
> +{
> +  struct grub_arg_list *state = ctxt->state;
> +  const char *value = NULL;
> +
> +  if (fw_fdt == NULL)
> +      return grub_error (GRUB_ERR_IO,
> +                         N_("No device tree found"));
> +
> +  if (state[0].set)
> +    {
> +      value = grub_fdt_get_prop(fw_fdt, 0, state[0].arg, NULL);

Missing space before "(".

> +    }

Please drop redundant curly braces.

> +  if (value == NULL)
> +    return grub_error (GRUB_ERR_OUT_OF_RANGE,
> +                       N_("failed to retrieve the prop field"));
> +
> +  if (state[1].set)
> +    grub_env_set (state[1].arg, value);
> +  else
> +    grub_printf ("%s\n", value);
> +
> +  return GRUB_ERR_NONE;
> +}

Daniel