Re: [PATCH v7 14/14] qmp/hmp, device_tree.c: introduce dumpdtb

[Philippe Mathieu-Daudé]

On 8/9/22 21:40, Daniel Henrique Barboza wrote:
> To save the FDT blob we have the '-machine dumpdtb=<file>' property.
> With this property set, the machine saves the FDT in <file> and exit.
> The created file can then be converted to plain text dts format using
> 'dtc'.
>
> There's nothing particularly sophisticated into saving the FDT that
> can't be done with the machine at any state, as long as the machine has
> a valid FDT to be saved.
>
> The 'dumpdtb' command receives a 'filename' paramenter and, if a valid

Typo "parameter".

> FDT is available, it'll save it in a file 'filename'. In short, this is
> a '-machine dumpdtb' that can be fired on demand via QMP/HMP.
>
> A valid FDT consists of a FDT that was created using libfdt being
> retrieved via 'current_machine->fdt' in device_tree.c.

This sentence is odd.

> This condition is
> met by most FDT users in QEMU.
>
> This command will always be executed in-band (i.e. holding BQL),
> avoiding potential race conditions with machines that might change the
> FDT during runtime (e.g. PowerPC 'pseries' machine).
>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Alistair Francis <alistair.francis@wdc.com>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>   hmp-commands.hx              | 15 +++++++++++++++
>   include/sysemu/device_tree.h |  1 +
>   monitor/misc.c               |  1 +
>   qapi/machine.json            | 18 ++++++++++++++++++
>   softmmu/device_tree.c        | 31 +++++++++++++++++++++++++++++++
>   5 files changed, 66 insertions(+)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 182e639d14..753669a2eb 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1800,3 +1800,18 @@ ERST
>                         "\n\t\t\t\t\t limit on a specified virtual cpu",
>           .cmd        = hmp_cancel_vcpu_dirty_limit,
>       },
> +
> +#if defined(CONFIG_FDT)
> +    {
> +        .name       = "dumpdtb",
> +        .args_type  = "filename:F",
> +        .params     = "filename",
> +        .help       = "save the FDT in the 'filename' file to be decoded using 
> dtc",
> +        .cmd        = hmp_dumpdtb,
> +    },
> +
> +SRST
> +``dumpdtb`` *filename*
> +  Save the FDT in the 'filename' file to be decoded using dtc.
> +ERST
> +#endif
> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
> index ef060a9759..e7c5441f56 100644
> --- a/include/sysemu/device_tree.h
> +++ b/include/sysemu/device_tree.h
> @@ -136,6 +136,7 @@ int qemu_fdt_add_path(void *fdt, const char *path);
>       } while (0)
>   
>   void qemu_fdt_dumpdtb(void *fdt, int size);
> +void hmp_dumpdtb(Monitor *mon, const QDict *qdict);
>   
>   /**
>    * qemu_fdt_setprop_sized_cells_from_array:
> diff --git a/monitor/misc.c b/monitor/misc.c
> index 3d2312ba8d..e7dd63030b 100644
> --- a/monitor/misc.c
> +++ b/monitor/misc.c
> @@ -49,6 +49,7 @@
>   #include "sysemu/blockdev.h"
>   #include "sysemu/sysemu.h"
>   #include "sysemu/tpm.h"
> +#include "sysemu/device_tree.h"
>   #include "qapi/qmp/qdict.h"
>   #include "qapi/qmp/qerror.h"
>   #include "qapi/qmp/qstring.h"
> diff --git a/qapi/machine.json b/qapi/machine.json
> index abb2f48808..9f0c8c8374 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1664,3 +1664,21 @@
>        '*size': 'size',
>        '*max-size': 'size',
>        '*slots': 'uint64' } }
> +
> +##
> +# @dumpdtb:
> +#
> +# Save the FDT in dtb format.
> +#
> +# @filename: name of the FDT file to be created

"name of the binary FDT ..."?

> +#
> +# Since: 7.2
> +#
> +# Example:
> +#   {"execute": "dumpdtb"}
> +#    "arguments": { "filename": "fdt.dtb" } }
> +#
> +##
> +{ 'command': 'dumpdtb',
> +  'data': { 'filename': 'str' },
> +  'if': 'CONFIG_FDT' }
> diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
> index 6ca3fad285..7031dcf89d 100644
> --- a/softmmu/device_tree.c
> +++ b/softmmu/device_tree.c
> @@ -26,6 +26,9 @@
>   #include "hw/loader.h"
>   #include "hw/boards.h"
>   #include "qemu/config-file.h"
> +#include "qapi/qapi-commands-machine.h"
> +#include "qapi/qmp/qdict.h"
> +#include "monitor/hmp.h"
>   
>   #include <libfdt.h>
>   
> @@ -643,3 +646,31 @@ out:
>       g_free(propcells);
>       return ret;
>   }
> +
> +void qmp_dumpdtb(const char *filename, Error **errp)
> +{
> +    g_autoptr(GError) err = NULL;
> +    int size;

fdt_totalsize() returns an uint32_t. Maybe use "unsigned" if you
don't want to use uint32_t?

> +
> +    if (!current_machine->fdt) {
> +        error_setg(errp, "This machine doesn't have a FDT");
> +        return;
> +    }
> +
> +    size = fdt_totalsize(current_machine->fdt);

       assert(size > 0); ?

> +
> +    if (!g_file_set_contents(filename, current_machine->fdt, size, &err)) {
> +        error_setg(errp, "Error saving FDT to file %s: %s",
> +                   filename, err->message);
> +    }

Eventually:

       info_report("Dumped %u bytes of FDT to %s\n", size, filename);

To have a feedback in HMP.

> +}
> +
> +void hmp_dumpdtb(Monitor *mon, const QDict *qdict)
> +{
> +    const char *filename = qdict_get_str(qdict, "filename");
> +    Error *local_err = NULL;
> +
> +    qmp_dumpdtb(filename, &local_err);
> +
> +    hmp_handle_error(mon, local_err);
> +}

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>