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

[Daniel Henrique Barboza <danielhb413@gmail.com>]

On 9/22/22 09:29, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> 
>> 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.
> 
> Seconded.

I removed it and changed the previous paragraph as follows:

The 'dumpdtb' command receives a 'filename' parameter and, if the FDT is available
via current_machine->fdt, save it in dtb format to 'filename'. In short, this is a
'-machine dumpdtb' that can be fired on demand via QMP/HMP.


> 
>>> 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",
> 
> Here, you document the format as "to be decoded using dtc".  In the QAPI
> schema below, you document it as "dtb format" and "FDT file".  Pick one
> and stick to it, please.
> 
> "the 'filename' file" feels a bit awkward.  Suggest something "dump the
> FDT in dtb format to 'filename'", similar to your phrasing in the QAPI
> schema.

Changed to:

         .help       = "dump the FDT in dtb format to 'filename'",


> 
> 
>>> +        .cmd        = hmp_dumpdtb,
>>> +    },
>>> +
>>> +SRST
>>> +``dumpdtb`` *filename*
>>> +  Save the FDT in the 'filename' file to be decoded using dtc.
> 
> *filename*, not 'filename'.

Changed the sentence to:


   Dump the FDT in dtb format to *filename*.


> 
>>> +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 ..."?

Changed to:


# @filename: name of the dtb file to be created



>>
>>> +#
>>> +# 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?
> 
> Best to avoid unnecessary conversions between signed and unsigned.
> 
> The value is passed to g_file_set_contents() below, which takes a
> gssize.  uint32_t should be narrower than gssize on anything capable of
> running QEMU.  So let's use that.


Changed size to 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); ?

Given that this is classified as a debug command I believe it's ok to g_assert()
if size == 0 here. Changed.


>>
>>> +
>>> +    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.
> 
> If feedback is desired, it needs to be done in hmp_dumpdtb().
> info_report() here would make the QMP command spam stderr.

Added the following in hmp_dumpdtb():

     if (hmp_handle_error(mon, local_err)) {
         return;
     }

     info_report("dtb dumped to %s",filename);

> 
>>> +}
>>> +
>>> +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>
> 
> With the commit message, the documentation, and the integer conversions
> tidied up:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thank you both for the review. I've added both R-bs after the changes mentioned.



Daniel

>