[PATCH for-7.2 00/10] add hmp 'save-fdt' and 'info fdt' commands

[PATCH for-7.2 00/10] add hmp 'save-fdt' and 'info fdt' commands

[Daniel Henrique Barboza]

Hi,

After dealing with a FDT element that isn't being shown in the userspace
and having to shutdown the guest, dump the FDT using 'machine -dumpdtb' and
then using 'dtc' to see what was inside the FDT, I thought it was a good
idea to add extra support for FDT handling in QEMU.

This series introduces 2 commands. 'fdt-save' behaves similar to what
'machine -dumpdtb' does, with the advantage of saving the FDT of a running
guest on demand. This command is implemented in patch 03.

The second command, 'info fdt <command>' is more sophisticated. This
command can print specific nodes and properties of the FDT. A few
examples:

- print the /cpus/cpu@0 from an ARM 'virt' machine:

(qemu) info fdt /cpus/cpu@0
/cpus/cpu@0 {
    phandle = <0x8001>
    reg = <0x0>
    compatible = 'arm,cortex-a57'
    device_type = 'cpu'
}
(qemu) 

- print the device_type property of the interrupt-controller node of a
pSeries machine:

(qemu) info fdt /interrupt-controller/device_type
/interrupt-controller/device_type = 'PowerPC-External-Interrupt-Presentation'
(qemu) 

Issuing a 'info fdt /' will dump all the FDT. 'info fdt' is implemented
in patches 4-10.

Both 'fdt-save' and 'info fdt' works across machines and archs based on
two premises: the FDT must be created using libfdt (which is the case of
all FDTs created with device_tree.c helpers and the _FDT macro) and the
FDT must be reachable via MachineState->fdt.

To meet the prerequisites for ARM machines, patch 1 makes a change in
arm_load_dtb(). Patches 2 and 3 makes a similar change for two PowerPC
machines that weren't using machine->fdt.

Tests were done using the ARM machvirt machine and ppc64 pSeries
machine, but any machine that meets the forementioned conditions will be
supported by these 2 new commands. 


Daniel Henrique Barboza (10):
  hw/arm/boot.c: do not free machine->fdt in arm_load_dtb()
  hw/ppc/pegasos2.c: set machine->fdt in machine_reset()
  hw/ppc: set machine->fdt in spapr machine
  hmp, device_tree.c: introduce fdt-save
  hmp, device_tree.c: introduce 'info fdt' command
  device_tree.c: support printing of strings props
  device_tree.c: support remaining FDT prop types
  device_node.c: enable 'info fdt' to print subnodes
  device_tree.c: add fdt_print_property() helper
  hmp, device_tree.c: add 'info fdt <property>' support

 hmp-commands-info.hx         |  13 +++
 hmp-commands.hx              |  13 +++
 hw/arm/boot.c                |   3 +-
 hw/ppc/pegasos2.c            |   3 +
 hw/ppc/spapr.c               |   3 +
 hw/ppc/spapr_hcall.c         |   3 +
 include/sysemu/device_tree.h |   3 +
 monitor/misc.c               |  25 ++++
 softmmu/device_tree.c        | 219 +++++++++++++++++++++++++++++++++++
 9 files changed, 284 insertions(+), 1 deletion(-)

-- 
2.36.1

[PATCH for-7.2 01/10] hw/arm/boot.c: do not free machine->fdt in arm_load_dtb()

[Daniel Henrique Barboza]

At this moment, arm_load_dtb() can free machine->fdt when
binfo->dtb_filename is NULL. If there's no 'dtb_filename', 'fdt' will be
retrieved by binfo->get_dtb(). If get_dtb() returns machine->fdt, as is
the case of machvirt_dtb() from hw/arm/virt.c, fdt now has a pointer to
machine->fdt. And, in that case, the existing g_free(fdt) at the end of
arm_load_dtb() will make machine->fdt point to an invalid memory region.

This is not an issue right now because there's no code that access
machine->fdt after arm_load_dtb(), but we're going to add a couple do
FDT HMP commands that will rely on machine->fdt being valid.

Instead of freeing 'fdt' at the end of arm_load_dtb(), assign it to
machine->fdt. This will allow the FDT of ARM machines that relies on
arm_load_dtb() to be accessed later on.

Since all ARM machines allocates the FDT only once, we don't need to
worry about leaking the existing FDT during a machine reset (which is
something that other machines have to look after, e.g. the ppc64 pSeries
machine).

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-arm@nongnu.org
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/arm/boot.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index ada2717f76..1d9c6047b1 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -684,7 +684,8 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
      */
     rom_add_blob_fixed_as("dtb", fdt, size, addr, as);
 
-    g_free(fdt);
+    /* Update ms->fdt pointer */
+    ms->fdt = fdt;
 
     return size;
 
-- 
2.36.1

[PATCH for-7.2 02/10] hw/ppc/pegasos2.c: set machine->fdt in machine_reset()

[Daniel Henrique Barboza]

We'll introduce HMP commands that requires machine->fdt to be set
properly.

Cc: BALATON Zoltan <balaton@eik.bme.hu>
Cc: qemu-ppc@nongnu.org
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/pegasos2.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
index 61f4263953..9827c3b4c2 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -329,6 +329,9 @@ static void pegasos2_machine_reset(MachineState *machine)
     g_free(pm->fdt_blob);
     pm->fdt_blob = fdt;
 
+    /* Set common MachineState->fdt */
+    machine->fdt = fdt;
+
     vof_build_dt(fdt, pm->vof);
     vof_client_open_store(fdt, pm->vof, "/chosen", "stdout", "/failsafe");
     pm->cpu->vhyp = PPC_VIRTUAL_HYPERVISOR(machine);
-- 
2.36.1

[PATCH for-7.2 03/10] hw/ppc: set machine->fdt in spapr machine

[Daniel Henrique Barboza]

The pSeries machine never bothered with the common machine->fdt
attribute. We do all the FDT related work using spapr->fdt_blob.

We're going to introduce HMP commands to read and save the FDT, which
will rely on setting machine->fdt properly to work across all machine
archs/types.

Let's set machine->fdt in the two places where we manipulate the FDT:
spapr_machine_reset() and CAS. spapr->fdt_blob is left untouched: what
we want is a way to access the FDT from HMP, not replace
spapr->fdt_blob.

Cc: Cédric Le Goater <clg@kaod.org>
Cc: qemu-ppc@nongnu.org
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr.c       | 3 +++
 hw/ppc/spapr_hcall.c | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index bc9ba6e6dc..7279583a4d 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1713,6 +1713,9 @@ static void spapr_machine_reset(MachineState *machine)
     spapr->fdt_initial_size = spapr->fdt_size;
     spapr->fdt_blob = fdt;
 
+    /* Set common MachineState->fdt */
+    machine->fdt = fdt;
+
     /* Set up the entry state */
     first_ppc_cpu->env.gpr[5] = 0;
 
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index a8d4a6bcf0..e6b960577d 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1256,6 +1256,9 @@ target_ulong do_client_architecture_support(PowerPCCPU *cpu,
     spapr->fdt_initial_size = spapr->fdt_size;
     spapr->fdt_blob = fdt;
 
+    /* Set common MachineState->fdt */
+    MACHINE(spapr)->fdt = fdt;
+
     return H_SUCCESS;
 }
 
-- 
2.36.1

[PATCH for-7.2 04/10] hmp, device_tree.c: introduce fdt-save

[Daniel Henrique Barboza]

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 'fdt-save' command receives a 'filename' paramenter and, if a valid
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 HMP.

A valid FDT consists of a FDT that was created using libfdt being
retrieved via 'current_machine->fdt' in device_tree.c. This condition is
met by most FDT users in QEMU.

Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hmp-commands.hx              | 13 +++++++++++++
 include/sysemu/device_tree.h |  2 ++
 monitor/misc.c               | 13 +++++++++++++
 softmmu/device_tree.c        | 18 ++++++++++++++++++
 4 files changed, 46 insertions(+)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index c9d465735a..3c134cf652 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1768,3 +1768,16 @@ ERST
                       "\n\t\t\t -b to specify dirty bitmap as method of calculation)",
         .cmd        = hmp_calc_dirty_rate,
     },
+
+    {
+        .name       = "fdt-save",
+        .args_type  = "filename:s",
+        .params     = "[filename] file to save the FDT",
+        .help       = "save the FDT in the 'filename' file to be decoded using dtc",
+        .cmd        = hmp_fdt_save,
+    },
+
+SRST
+``fdt-save`` *filename*
+  Save the FDT in the 'filename' file to be decoded using dtc
+ERST
diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
index ef060a9759..1397adb21c 100644
--- a/include/sysemu/device_tree.h
+++ b/include/sysemu/device_tree.h
@@ -123,6 +123,8 @@ int qemu_fdt_nop_node(void *fdt, const char *node_path);
 int qemu_fdt_add_subnode(void *fdt, const char *name);
 int qemu_fdt_add_path(void *fdt, const char *path);
 
+void fdt_save(const char *filename, Error **errp);
+
 #define qemu_fdt_setprop_cells(fdt, node_path, property, ...)                 \
     do {                                                                      \
         uint32_t qdt_tmp[] = { __VA_ARGS__ };                                 \
diff --git a/monitor/misc.c b/monitor/misc.c
index 3d2312ba8d..145285cec0 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -78,6 +78,7 @@
 #include "qapi/qmp-event.h"
 #include "sysemu/cpus.h"
 #include "qemu/cutils.h"
+#include "sysemu/device_tree.h"
 
 #if defined(TARGET_S390X)
 #include "hw/s390x/storage-keys.h"
@@ -936,6 +937,18 @@ static void hmp_boot_set(Monitor *mon, const QDict *qdict)
     }
 }
 
+static void hmp_fdt_save(Monitor *mon, const QDict *qdict)
+{
+    const char *path = qdict_get_str(qdict, "filename");
+    Error *local_err = NULL;
+
+    fdt_save(path, &local_err);
+
+    if (local_err) {
+        error_report_err(local_err);
+    }
+}
+
 static void hmp_info_mtree(Monitor *mon, const QDict *qdict)
 {
     bool flatview = qdict_get_try_bool(qdict, "flatview", false);
diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
index 6ca3fad285..eeab6a5ef0 100644
--- a/softmmu/device_tree.c
+++ b/softmmu/device_tree.c
@@ -643,3 +643,21 @@ out:
     g_free(propcells);
     return ret;
 }
+
+void fdt_save(const char *filename, Error **errp)
+{
+    int size;
+
+    if (!current_machine->fdt) {
+        error_setg(errp, "Unable to find the machine FDT");
+        return;
+    }
+
+    size = fdt_totalsize(current_machine->fdt);
+
+    if (g_file_set_contents(filename, current_machine->fdt, size, NULL)) {
+        return;
+    }
+
+    error_setg(errp, "Error when saving machine FDT to file %s", filename);
+}
-- 
2.36.1

[PATCH for-7.2 05/10] hmp, device_tree.c: introduce 'info fdt' command

[Daniel Henrique Barboza]

Reading the FDT requires that the user saves the fdt_blob and then use
'dtc' to read the contents. Saving the file and using 'dtc' is a strong
use case when we need to compare two FDTs, but it's a lot of steps if
you want to do quick check on a certain attribute.

'info fdt' retrieves FDT nodes (and properties, later on) and print it
to the user. This can be used to check the FDT on running machines
without having to save the blob and use 'dtc'.

The implementation is based on the premise that the machine thas a FDT
created using libfdt and pointed by 'machine->fdt'. As long as this
pre-requisite is met the machine should be able to support it.

For now we're going to add the required HMP boilerplate and the
capability of printing the name of the properties of a given node. Next
patches will extend 'info fdt' to be able to print nodes recursively.

This is an example of 'info fdt' fetching the '/chosen' node of the
pSeries machine:

(qemu) info fdt /chosen
chosen {
    ibm,architecture-vec-5;
    rng-seed;
    ibm,arch-vec-5-platform-support;
    linux,pci-probe-only;
    stdout-path;
    linux,stdout-path;
    qemu,graphic-depth;
    qemu,graphic-height;
    qemu,graphic-width;
}

And the same node for the aarch64 'virt' machine:

(qemu) info fdt /chosen
chosen {
    stdout-path;
    rng-seed;
    kaslr-seed;
}

Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hmp-commands-info.hx         | 13 +++++++++++
 include/sysemu/device_tree.h |  1 +
 monitor/misc.c               | 12 ++++++++++
 softmmu/device_tree.c        | 43 ++++++++++++++++++++++++++++++++++++
 4 files changed, 69 insertions(+)

diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index 3ffa24bd67..abf277be7d 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -908,3 +908,16 @@ SRST
   ``stats``
     Show runtime-collected statistics
 ERST
+
+    {
+        .name       = "fdt",
+        .args_type  = "fullpath:s",
+        .params     = "fullpath",
+        .help       = "show firmware device tree node given its full path",
+        .cmd        = hmp_info_fdt,
+    },
+
+SRST
+  ``info fdt``
+    Show firmware device tree (fdt).
+ERST
diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
index 1397adb21c..c0f98b1c88 100644
--- a/include/sysemu/device_tree.h
+++ b/include/sysemu/device_tree.h
@@ -124,6 +124,7 @@ int qemu_fdt_add_subnode(void *fdt, const char *name);
 int qemu_fdt_add_path(void *fdt, const char *path);
 
 void fdt_save(const char *filename, Error **errp);
+void fdt_info(const char *fullpath, Error **errp);
 
 #define qemu_fdt_setprop_cells(fdt, node_path, property, ...)                 \
     do {                                                                      \
diff --git a/monitor/misc.c b/monitor/misc.c
index 145285cec0..e709a7de91 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -973,6 +973,18 @@ static void hmp_info_capture(Monitor *mon, const QDict *qdict)
     }
 }
 
+static void hmp_info_fdt(Monitor *mon, const QDict *qdict)
+{
+    const char *fullpath = qdict_get_str(qdict, "fullpath");
+    Error *local_err = NULL;
+
+    fdt_info(fullpath, &local_err);
+
+    if (local_err) {
+        error_report_err(local_err);
+    }
+}
+
 static void hmp_stopcapture(Monitor *mon, const QDict *qdict)
 {
     int i;
diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
index eeab6a5ef0..899c239c5c 100644
--- a/softmmu/device_tree.c
+++ b/softmmu/device_tree.c
@@ -26,6 +26,7 @@
 #include "hw/loader.h"
 #include "hw/boards.h"
 #include "qemu/config-file.h"
+#include "qemu/qemu-print.h"
 
 #include <libfdt.h>
 
@@ -661,3 +662,45 @@ void fdt_save(const char *filename, Error **errp)
 
     error_setg(errp, "Error when saving machine FDT to file %s", filename);
 }
+
+static void fdt_print_node(int node, int depth)
+{
+    const struct fdt_property *prop = NULL;
+    const char *propname = NULL;
+    void *fdt = current_machine->fdt;
+    int padding = depth * 4;
+    int property = 0;
+    int prop_size;
+
+    qemu_printf("%*s%s {\n", padding, "", fdt_get_name(fdt, node, NULL));
+
+    padding += 4;
+
+    fdt_for_each_property_offset(property, fdt, node) {
+        prop = fdt_get_property_by_offset(fdt, property, &prop_size);
+        propname = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
+
+        qemu_printf("%*s%s;\n", padding, "", propname);
+    }
+
+    padding -= 4;
+    qemu_printf("%*s}\n", padding, "");
+}
+
+void fdt_info(const char *fullpath, Error **errp)
+{
+    int node;
+
+    if (!current_machine->fdt) {
+        error_setg(errp, "Unable to find the machine FDT");
+        return;
+    }
+
+    node = fdt_path_offset(current_machine->fdt, fullpath);
+    if (node < 0) {
+        error_setg(errp, "node '%s' not found in FDT", fullpath);
+        return;
+    }
+
+    fdt_print_node(node, 0);
+}
-- 
2.36.1

[PATCH for-7.2 06/10] device_tree.c: support printing of strings props

[Daniel Henrique Barboza]

To support printing string properties in 'info fdt' we need to determine
whether a void data might contain a string.

We do that by casting the void data to a string array and:

- check if the array finishes with a null character
- check if all characters are printable

If both conditions are met, we'll consider it to be a string data type
and print it accordingly. After this change, 'info fdt' is now able to
print string properties. Here's an example with the ARM 'virt' machine:

(qemu) info fdt /chosen
chosen {
    stdout-path = '/pl011@9000000'
    rng-seed;
    kaslr-seed;
}

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 softmmu/device_tree.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
index 899c239c5c..3c070acc0d 100644
--- a/softmmu/device_tree.c
+++ b/softmmu/device_tree.c
@@ -663,6 +663,24 @@ void fdt_save(const char *filename, Error **errp)
     error_setg(errp, "Error when saving machine FDT to file %s", filename);
 }
 
+static bool fdt_prop_is_string(const void *data, int size)
+{
+    const char *str = data;
+    int i;
+
+    if (size <= 0 || str[size - 1] != '\0') {
+        return false;
+    }
+
+    for (i = 0; i < size - 1; i++) {
+        if (!g_ascii_isprint(str[i])) {
+            return false;
+        }
+    }
+
+    return true;
+}
+
 static void fdt_print_node(int node, int depth)
 {
     const struct fdt_property *prop = NULL;
@@ -680,7 +698,11 @@ static void fdt_print_node(int node, int depth)
         prop = fdt_get_property_by_offset(fdt, property, &prop_size);
         propname = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
 
-        qemu_printf("%*s%s;\n", padding, "", propname);
+        if (fdt_prop_is_string(prop->data, prop_size)) {
+            qemu_printf("%*s%s = '%s'\n", padding, "", propname, prop->data);
+        } else {
+            qemu_printf("%*s%s;\n", padding, "", propname);
+        }
     }
 
     padding -= 4;
-- 
2.36.1

[PATCH for-7.2 07/10] device_tree.c: support remaining FDT prop types

[Daniel Henrique Barboza]

When printing a blob with 'dtc' using the '-O dts' option there are 3
distinct data types being printed: strings, arrays of uint32s and
regular byte arrays.

Previous patch added support to print strings. Let's add the remaining
formats. We want to resemble the format that 'dtc -O dts' uses, so every
uint32 array uses angle brackets (<>), and regular byte array uses square
brackets ([]). For properties that has no values we keep printing just
its name.

The /chosen FDT node from the pSeris machine gives an example of all
property types 'info fdt' is now able to display:

(qemu) info fdt /chosen
chosen {
    ibm,architecture-vec-5 = [0 0]
    rng-seed = <0x5967a270 0x62b0fb4f 0x8262b46a 0xabf48423 0xcce9615 0xf9daae64 0x66564790 0x357d1604>
    ibm,arch-vec-5-platform-support = <0x178018c0 0x19001a40>
    linux,pci-probe-only = <0x0>
    stdout-path = '/vdevice/vty@71000000'
    linux,stdout-path = '/vdevice/vty@71000000'
    qemu,graphic-depth = <0x20>
    qemu,graphic-height = <0x258>
    qemu,graphic-width = <0x320>
}

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 softmmu/device_tree.c | 53 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 51 insertions(+), 2 deletions(-)

diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
index 3c070acc0d..3a4d09483b 100644
--- a/softmmu/device_tree.c
+++ b/softmmu/device_tree.c
@@ -681,6 +681,46 @@ static bool fdt_prop_is_string(const void *data, int size)
     return true;
 }
 
+static bool fdt_prop_is_uint32_array(int size)
+{
+    return size % 4 == 0;
+}
+
+static void fdt_prop_print_uint32_array(const char *propname, const void *data,
+                                        int prop_size, int padding)
+{
+    const fdt32_t *array = data;
+    int array_len = prop_size / 4;
+    int i;
+
+    qemu_printf("%*s%s = <", padding, "", propname);
+    for (i = 0; i < array_len; i++) {
+        qemu_printf("0x%" PRIx32, fdt32_to_cpu(array[i]));
+
+        if (i < array_len - 1) {
+            qemu_printf(" ");
+        }
+    }
+    qemu_printf(">\n");
+}
+
+static void fdt_prop_print_val(const char *propname, const void *data,
+                               int prop_size, int padding)
+{
+    const char *val = data;
+    int i;
+
+    qemu_printf("%*s%s = [", padding, "", propname);
+    for (i = 0; i < prop_size; i++) {
+        qemu_printf("%x", val[i]);
+
+        if (i < prop_size - 1) {
+            qemu_printf(" ");
+        }
+    }
+    qemu_printf("]\n");
+}
+
 static void fdt_print_node(int node, int depth)
 {
     const struct fdt_property *prop = NULL;
@@ -698,10 +738,19 @@ static void fdt_print_node(int node, int depth)
         prop = fdt_get_property_by_offset(fdt, property, &prop_size);
         propname = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
 
+        if (prop_size == 0) {
+            qemu_printf("%*s%s;\n", padding, "", propname);
+            continue;
+        }
+
         if (fdt_prop_is_string(prop->data, prop_size)) {
-            qemu_printf("%*s%s = '%s'\n", padding, "", propname, prop->data);
+            qemu_printf("%*s%s = '%s'\n", padding, "",
+                        propname, (char *)prop->data);
+        } else if (fdt_prop_is_uint32_array(prop_size)) {
+            fdt_prop_print_uint32_array(propname, prop->data, prop_size,
+                                        padding);
         } else {
-            qemu_printf("%*s%s;\n", padding, "", propname);
+            fdt_prop_print_val(propname, prop->data, prop_size, padding);
         }
     }
 
-- 
2.36.1

[PATCH for-7.2 08/10] device_node.c: enable 'info fdt' to print subnodes

[Daniel Henrique Barboza]

Printing subnodes of a given node will allow us to show a whole subtree,
which the additional perk of 'info fdt /' being able to print the whole
FDT.

Since we're now printing more than one subnode, change 'fdt_info' to
print the full path of the first node. This small tweak helps
identifying which node or subnode are being displayed.

To demostrate this capability without printing the whole FDT, the
'/cpus/cpu-map' node from the ARM 'virt' machine has a lot of subnodes:

(qemu) info fdt /cpus/cpu-map
/cpus/cpu-map {
    socket0 {
        cluster0 {
            core0 {
                cpu = <0x8001>
            }
        }
    }
}

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 softmmu/device_tree.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
index 3a4d09483b..88b6a0c902 100644
--- a/softmmu/device_tree.c
+++ b/softmmu/device_tree.c
@@ -721,16 +721,24 @@ static void fdt_prop_print_val(const char *propname, const void *data,
     qemu_printf("]\n");
 }
 
-static void fdt_print_node(int node, int depth)
+static void fdt_print_node(int node, int depth, const char *fullpath)
 {
     const struct fdt_property *prop = NULL;
+    const char *nodename = NULL;
     const char *propname = NULL;
     void *fdt = current_machine->fdt;
     int padding = depth * 4;
     int property = 0;
+    int parent = node;
     int prop_size;
 
-    qemu_printf("%*s%s {\n", padding, "", fdt_get_name(fdt, node, NULL));
+    if (fullpath != NULL) {
+        nodename = fullpath;
+    } else {
+        nodename = fdt_get_name(fdt, node, NULL);
+    }
+
+    qemu_printf("%*s%s {\n", padding, "", nodename);
 
     padding += 4;
 
@@ -754,6 +762,10 @@ static void fdt_print_node(int node, int depth)
         }
     }
 
+    fdt_for_each_subnode(node, fdt, parent) {
+        fdt_print_node(node, depth + 1, NULL);
+    }
+
     padding -= 4;
     qemu_printf("%*s}\n", padding, "");
 }
@@ -773,5 +785,5 @@ void fdt_info(const char *fullpath, Error **errp)
         return;
     }
 
-    fdt_print_node(node, 0);
+    fdt_print_node(node, 0, fullpath);
 }
-- 
2.36.1

[PATCH for-7.2 09/10] device_tree.c: add fdt_print_property() helper

[Daniel Henrique Barboza]

We want to be able to also print properties with 'info fdt'.

Create a helper to print properties based on the already existing code
from fdt_print_node().

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 softmmu/device_tree.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
index 88b6a0c902..e41894fbef 100644
--- a/softmmu/device_tree.c
+++ b/softmmu/device_tree.c
@@ -721,6 +721,23 @@ static void fdt_prop_print_val(const char *propname, const void *data,
     qemu_printf("]\n");
 }
 
+static void fdt_print_property(const char *propname, const void *data,
+                               int prop_size, int padding)
+{
+    if (prop_size == 0) {
+        qemu_printf("%*s%s;\n", padding, "", propname);
+        return;
+    }
+
+    if (fdt_prop_is_string(data, prop_size)) {
+        qemu_printf("%*s%s = '%s'\n", padding, "", propname, (char *)data);
+    } else if (fdt_prop_is_uint32_array(prop_size)) {
+        fdt_prop_print_uint32_array(propname, data, prop_size, padding);
+    } else {
+        fdt_prop_print_val(propname, data, prop_size, padding);
+    }
+}
+
 static void fdt_print_node(int node, int depth, const char *fullpath)
 {
     const struct fdt_property *prop = NULL;
@@ -746,20 +763,7 @@ static void fdt_print_node(int node, int depth, const char *fullpath)
         prop = fdt_get_property_by_offset(fdt, property, &prop_size);
         propname = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
 
-        if (prop_size == 0) {
-            qemu_printf("%*s%s;\n", padding, "", propname);
-            continue;
-        }
-
-        if (fdt_prop_is_string(prop->data, prop_size)) {
-            qemu_printf("%*s%s = '%s'\n", padding, "",
-                        propname, (char *)prop->data);
-        } else if (fdt_prop_is_uint32_array(prop_size)) {
-            fdt_prop_print_uint32_array(propname, prop->data, prop_size,
-                                        padding);
-        } else {
-            fdt_prop_print_val(propname, prop->data, prop_size, padding);
-        }
+        fdt_print_property(propname, prop->data, prop_size, padding);
     }
 
     fdt_for_each_subnode(node, fdt, parent) {
-- 
2.36.1

[PATCH for-7.2 10/10] hmp, device_tree.c: add 'info fdt <property>' support

[Daniel Henrique Barboza]

'info fdt' is only able to print full nodes so far. It would be good to
be able to also print single properties, since ometimes we just want
to verify a single value from the FDT.

libfdt does not have support to find a property given its full path, but
it does have a way to return a fdt_property given a prop name and its
subnode.

This is how we're going to support it:

- given the same fullpath parameter, assume it's a node. If we have a
match with an existing node, print it. If not, assume it's a property;

- in fdt_find_property() we're going to split 'fullpath' into node and
property. Unfortunately we can't use g_path_get_basename() to helps us
because, although the device tree path format is similar to Linux, it'll
not work when trying to run QEMU under Windows where the path format is
different;

- after spliiting into node + property, try to find the node in the FDT.
If we have a match, use fdt_get_property() to retrieve fdt_property.
Return it if found;

- using the fdt_print_property() created previously, print the property.

After this change, if an user wants to print just the value of 'cpu' inside
/cpu/cpu-map(...) from an ARM FDT, we can do it:

(qemu) info fdt /cpus/cpu-map/socket0/cluster0/core0/cpu
/cpus/cpu-map/socket0/cluster0/core0/cpu = <0x8001>
(qemu)

Or the 'ibm,my-dma-window' from the v-scsi device inside the pSeries
FDT:

(qemu) info fdt /vdevice/v-scsi@71000003/ibm,my-dma-window
/vdevice/v-scsi@71000003/ibm,my-dma-window = <0x71000003 0x0 0x0 0x0 0x10000000>
(qemu)

Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hmp-commands-info.hx  |  2 +-
 softmmu/device_tree.c | 79 ++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 76 insertions(+), 5 deletions(-)

diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index abf277be7d..8891c2918a 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -913,7 +913,7 @@ ERST
         .name       = "fdt",
         .args_type  = "fullpath:s",
         .params     = "fullpath",
-        .help       = "show firmware device tree node given its full path",
+        .help       = "show firmware device tree node or property given its full path",
         .cmd        = hmp_info_fdt,
     },
 
diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
index e41894fbef..f6eb060acc 100644
--- a/softmmu/device_tree.c
+++ b/softmmu/device_tree.c
@@ -774,9 +774,74 @@ static void fdt_print_node(int node, int depth, const char *fullpath)
     qemu_printf("%*s}\n", padding, "");
 }
 
+static const struct fdt_property *fdt_find_property(const char *fullpath,
+                                                    int *prop_size,
+                                                    Error **errp)
+{
+    const struct fdt_property *prop = NULL;
+    void *fdt = current_machine->fdt;
+    g_autoptr(GString) nodename = NULL;
+    const char *propname = NULL;
+    int path_len = strlen(fullpath);
+    int node = 0; /* default to root node '/' */
+    int i, idx = -1;
+
+    /*
+     * We'll assume that we're dealing with a property. libfdt
+     * does not have an API to find a property given the full
+     * path, but it does have an API to find a property inside
+     * a node.
+     */
+    nodename = g_string_new("");
+
+    for (i = path_len - 1; i >= 0; i--) {
+        if (fullpath[i] == '/') {
+            idx = i;
+            break;
+        }
+    }
+
+    if (idx == -1) {
+        error_setg(errp, "FDT paths must contain at least one '/' character");
+        return NULL;
+    }
+
+    if (idx == path_len - 1) {
+        error_setg(errp, "FDT paths can't end with a '/' character");
+        return NULL;
+    }
+
+    propname = &fullpath[idx + 1];
+
+    if (idx != 0) {
+        g_string_append_len(nodename, fullpath, idx);
+
+        node = fdt_path_offset(fdt, nodename->str);
+        if (node < 0) {
+            error_setg(errp, "node '%s' of property '%s' not found in FDT",
+                       nodename->str, propname);
+            return NULL;
+        }
+    } else {
+        /* idx = 0 means that it's a property of the root node */
+        g_string_append(nodename, "/");
+    }
+
+    prop = fdt_get_property(fdt, node, propname, prop_size);
+    if (!prop) {
+        error_setg(errp, "property '%s' not found in node '%s' in FDT",
+                   propname, nodename->str);
+        return NULL;
+    }
+
+    return prop;
+}
+
 void fdt_info(const char *fullpath, Error **errp)
 {
-    int node;
+    const struct fdt_property *prop = NULL;
+    Error *local_err = NULL;
+    int node, prop_size;
 
     if (!current_machine->fdt) {
         error_setg(errp, "Unable to find the machine FDT");
@@ -784,10 +849,16 @@ void fdt_info(const char *fullpath, Error **errp)
     }
 
     node = fdt_path_offset(current_machine->fdt, fullpath);
-    if (node < 0) {
-        error_setg(errp, "node '%s' not found in FDT", fullpath);
+    if (node >= 0) {
+        fdt_print_node(node, 0, fullpath);
+        return;
+    }
+
+    prop = fdt_find_property(fullpath, &prop_size, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
         return;
     }
 
-    fdt_print_node(node, 0, fullpath);
+    fdt_print_property(fullpath, prop->data, prop_size, 0);
 }
-- 
2.36.1

Re: [PATCH for-7.2 01/10] hw/arm/boot.c: do not free machine->fdt in arm_load_dtb()

[BALATON Zoltan]

On Fri, 22 Jul 2022, Daniel Henrique Barboza wrote:
> At this moment, arm_load_dtb() can free machine->fdt when
> binfo->dtb_filename is NULL. If there's no 'dtb_filename', 'fdt' will be
> retrieved by binfo->get_dtb(). If get_dtb() returns machine->fdt, as is
> the case of machvirt_dtb() from hw/arm/virt.c, fdt now has a pointer to
> machine->fdt. And, in that case, the existing g_free(fdt) at the end of
> arm_load_dtb() will make machine->fdt point to an invalid memory region.
>
> This is not an issue right now because there's no code that access
> machine->fdt after arm_load_dtb(), but we're going to add a couple do
> FDT HMP commands that will rely on machine->fdt being valid.
>
> Instead of freeing 'fdt' at the end of arm_load_dtb(), assign it to
> machine->fdt. This will allow the FDT of ARM machines that relies on
> arm_load_dtb() to be accessed later on.
>
> Since all ARM machines allocates the FDT only once, we don't need to
> worry about leaking the existing FDT during a machine reset (which is
> something that other machines have to look after, e.g. the ppc64 pSeries
> machine).
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-arm@nongnu.org
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
> hw/arm/boot.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index ada2717f76..1d9c6047b1 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -684,7 +684,8 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>      */
>     rom_add_blob_fixed_as("dtb", fdt, size, addr, as);
>
> -    g_free(fdt);
> +    /* Update ms->fdt pointer */
> +    ms->fdt = fdt;

Not sure this comment is useful as it just states what the assignment does 
so provides no further info.

Regards,
BALATON Zoltan

>
>     return size;
>
>

Re: [PATCH for-7.2 02/10] hw/ppc/pegasos2.c: set machine->fdt in machine_reset()

[BALATON Zoltan]

On Fri, 22 Jul 2022, Daniel Henrique Barboza wrote:
> We'll introduce HMP commands that requires machine->fdt to be set
> properly.
>
> Cc: BALATON Zoltan <balaton@eik.bme.hu>
> Cc: qemu-ppc@nongnu.org
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
> hw/ppc/pegasos2.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
> index 61f4263953..9827c3b4c2 100644
> --- a/hw/ppc/pegasos2.c
> +++ b/hw/ppc/pegasos2.c
> @@ -329,6 +329,9 @@ static void pegasos2_machine_reset(MachineState *machine)
>     g_free(pm->fdt_blob);
>     pm->fdt_blob = fdt;
>
> +    /* Set common MachineState->fdt */
> +    machine->fdt = fdt;
> +

Again, comment just states what the next line does but does not explain 
why. Either add a comment that explains why it's set or drop the trivial 
comment. Otherwise,

Acked-by: BALATON Zoltan <balaton@eik.bme.hu>

>     vof_build_dt(fdt, pm->vof);
>     vof_client_open_store(fdt, pm->vof, "/chosen", "stdout", "/failsafe");
>     pm->cpu->vhyp = PPC_VIRTUAL_HYPERVISOR(machine);
>

Re: [PATCH for-7.2 04/10] hmp, device_tree.c: introduce fdt-save

[BALATON Zoltan]

On Fri, 22 Jul 2022, 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 'fdt-save' command receives a 'filename' paramenter and, if a valid
> 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 HMP.

If it does the same as -machine dumpdtb wouldn't it be more intuitive to 
call the HMP command the same, so either dumpdtb or machine-dumpdtb or 
similar? That way it's more obvious that these do the same.

Regards,
BALATON Zoltan

> A valid FDT consists of a FDT that was created using libfdt being
> retrieved via 'current_machine->fdt' in device_tree.c. This condition is
> met by most FDT users in QEMU.
>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
> hmp-commands.hx              | 13 +++++++++++++
> include/sysemu/device_tree.h |  2 ++
> monitor/misc.c               | 13 +++++++++++++
> softmmu/device_tree.c        | 18 ++++++++++++++++++
> 4 files changed, 46 insertions(+)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index c9d465735a..3c134cf652 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1768,3 +1768,16 @@ ERST
>                       "\n\t\t\t -b to specify dirty bitmap as method of calculation)",
>         .cmd        = hmp_calc_dirty_rate,
>     },
> +
> +    {
> +        .name       = "fdt-save",
> +        .args_type  = "filename:s",
> +        .params     = "[filename] file to save the FDT",
> +        .help       = "save the FDT in the 'filename' file to be decoded using dtc",
> +        .cmd        = hmp_fdt_save,
> +    },
> +
> +SRST
> +``fdt-save`` *filename*
> +  Save the FDT in the 'filename' file to be decoded using dtc
> +ERST
> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
> index ef060a9759..1397adb21c 100644
> --- a/include/sysemu/device_tree.h
> +++ b/include/sysemu/device_tree.h
> @@ -123,6 +123,8 @@ int qemu_fdt_nop_node(void *fdt, const char *node_path);
> int qemu_fdt_add_subnode(void *fdt, const char *name);
> int qemu_fdt_add_path(void *fdt, const char *path);
>
> +void fdt_save(const char *filename, Error **errp);
> +
> #define qemu_fdt_setprop_cells(fdt, node_path, property, ...)                 \
>     do {                                                                      \
>         uint32_t qdt_tmp[] = { __VA_ARGS__ };                                 \
> diff --git a/monitor/misc.c b/monitor/misc.c
> index 3d2312ba8d..145285cec0 100644
> --- a/monitor/misc.c
> +++ b/monitor/misc.c
> @@ -78,6 +78,7 @@
> #include "qapi/qmp-event.h"
> #include "sysemu/cpus.h"
> #include "qemu/cutils.h"
> +#include "sysemu/device_tree.h"
>
> #if defined(TARGET_S390X)
> #include "hw/s390x/storage-keys.h"
> @@ -936,6 +937,18 @@ static void hmp_boot_set(Monitor *mon, const QDict *qdict)
>     }
> }
>
> +static void hmp_fdt_save(Monitor *mon, const QDict *qdict)
> +{
> +    const char *path = qdict_get_str(qdict, "filename");
> +    Error *local_err = NULL;
> +
> +    fdt_save(path, &local_err);
> +
> +    if (local_err) {
> +        error_report_err(local_err);
> +    }
> +}
> +
> static void hmp_info_mtree(Monitor *mon, const QDict *qdict)
> {
>     bool flatview = qdict_get_try_bool(qdict, "flatview", false);
> diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
> index 6ca3fad285..eeab6a5ef0 100644
> --- a/softmmu/device_tree.c
> +++ b/softmmu/device_tree.c
> @@ -643,3 +643,21 @@ out:
>     g_free(propcells);
>     return ret;
> }
> +
> +void fdt_save(const char *filename, Error **errp)
> +{
> +    int size;
> +
> +    if (!current_machine->fdt) {
> +        error_setg(errp, "Unable to find the machine FDT");
> +        return;
> +    }
> +
> +    size = fdt_totalsize(current_machine->fdt);
> +
> +    if (g_file_set_contents(filename, current_machine->fdt, size, NULL)) {
> +        return;
> +    }
> +
> +    error_setg(errp, "Error when saving machine FDT to file %s", filename);
> +}
>

Re: [PATCH for-7.2 00/10] add hmp 'save-fdt' and 'info fdt' commands

[BALATON Zoltan]

On Fri, 22 Jul 2022, Daniel Henrique Barboza wrote:
> Hi,
>
> After dealing with a FDT element that isn't being shown in the userspace
> and having to shutdown the guest, dump the FDT using 'machine -dumpdtb' and
> then using 'dtc' to see what was inside the FDT, I thought it was a good
> idea to add extra support for FDT handling in QEMU.
>
> This series introduces 2 commands. 'fdt-save' behaves similar to what
> 'machine -dumpdtb' does, with the advantage of saving the FDT of a running
> guest on demand. This command is implemented in patch 03.
>
> The second command, 'info fdt <command>' is more sophisticated. This
> command can print specific nodes and properties of the FDT. A few
> examples:
>
> - print the /cpus/cpu@0 from an ARM 'virt' machine:
>
> (qemu) info fdt /cpus/cpu@0
> /cpus/cpu@0 {
>    phandle = <0x8001>
>    reg = <0x0>
>    compatible = 'arm,cortex-a57'
>    device_type = 'cpu'
> }
> (qemu)
>
> - print the device_type property of the interrupt-controller node of a
> pSeries machine:
>
> (qemu) info fdt /interrupt-controller/device_type
> /interrupt-controller/device_type = 'PowerPC-External-Interrupt-Presentation'
> (qemu)
>
> Issuing a 'info fdt /' will dump all the FDT. 'info fdt' is implemented
> in patches 4-10.
>
> Both 'fdt-save' and 'info fdt' works across machines and archs based on
> two premises: the FDT must be created using libfdt (which is the case of
> all FDTs created with device_tree.c helpers and the _FDT macro) and the
> FDT must be reachable via MachineState->fdt.
>
> To meet the prerequisites for ARM machines, patch 1 makes a change in
> arm_load_dtb(). Patches 2 and 3 makes a similar change for two PowerPC
> machines that weren't using machine->fdt.

There are some other machines that load a dtb with load_device_tree(). Do 
they need some patches too?

Regards,
BALATON Zoltan

> Tests were done using the ARM machvirt machine and ppc64 pSeries
> machine, but any machine that meets the forementioned conditions will be
> supported by these 2 new commands.
>
>
> Daniel Henrique Barboza (10):
>  hw/arm/boot.c: do not free machine->fdt in arm_load_dtb()
>  hw/ppc/pegasos2.c: set machine->fdt in machine_reset()
>  hw/ppc: set machine->fdt in spapr machine
>  hmp, device_tree.c: introduce fdt-save
>  hmp, device_tree.c: introduce 'info fdt' command
>  device_tree.c: support printing of strings props
>  device_tree.c: support remaining FDT prop types
>  device_node.c: enable 'info fdt' to print subnodes
>  device_tree.c: add fdt_print_property() helper
>  hmp, device_tree.c: add 'info fdt <property>' support
>
> hmp-commands-info.hx         |  13 +++
> hmp-commands.hx              |  13 +++
> hw/arm/boot.c                |   3 +-
> hw/ppc/pegasos2.c            |   3 +
> hw/ppc/spapr.c               |   3 +
> hw/ppc/spapr_hcall.c         |   3 +
> include/sysemu/device_tree.h |   3 +
> monitor/misc.c               |  25 ++++
> softmmu/device_tree.c        | 219 +++++++++++++++++++++++++++++++++++
> 9 files changed, 284 insertions(+), 1 deletion(-)
>
>

Re: [PATCH for-7.2 00/10] add hmp 'save-fdt' and 'info fdt' commands

[Daniel P. Berrangé]

On Fri, Jul 22, 2022 at 04:59:57PM -0300, Daniel Henrique Barboza wrote:
> Hi,
> 
> After dealing with a FDT element that isn't being shown in the userspace
> and having to shutdown the guest, dump the FDT using 'machine -dumpdtb' and
> then using 'dtc' to see what was inside the FDT, I thought it was a good
> idea to add extra support for FDT handling in QEMU.
> 
> This series introduces 2 commands. 'fdt-save' behaves similar to what
> 'machine -dumpdtb' does, with the advantage of saving the FDT of a running
> guest on demand. This command is implemented in patch 03.
> 
> The second command, 'info fdt <command>' is more sophisticated. This
> command can print specific nodes and properties of the FDT. A few
> examples:
> 
> - print the /cpus/cpu@0 from an ARM 'virt' machine:
> 
> (qemu) info fdt /cpus/cpu@0
> /cpus/cpu@0 {
>     phandle = <0x8001>
>     reg = <0x0>
>     compatible = 'arm,cortex-a57'
>     device_type = 'cpu'
> }
> (qemu) 
> 
> - print the device_type property of the interrupt-controller node of a
> pSeries machine:
> 
> (qemu) info fdt /interrupt-controller/device_type
> /interrupt-controller/device_type = 'PowerPC-External-Interrupt-Presentation'
> (qemu)

Please don't add new HMP-only commands. These should be provided
as QMP commands, where the HMP is a tiny shim to the QMP.

If you don't want to think about formally modelling the data
for 'info fdt' / 'query-fdt', then just put an 'x-' prefix on
the QMP command and return printed formatted data, as illustrated
in:

https://www.qemu.org/docs/master/devel/writing-monitor-commands.html#writing-a-debugging-aid-returning-unstructured-text

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH for-7.2 04/10] hmp, device_tree.c: introduce fdt-save

[Dr. David Alan Gilbert]

* Daniel Henrique Barboza (danielhb413@gmail.com) 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 'fdt-save' command receives a 'filename' paramenter and, if a valid
> 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 HMP.
> 
> A valid FDT consists of a FDT that was created using libfdt being
> retrieved via 'current_machine->fdt' in device_tree.c. This condition is
> met by most FDT users in QEMU.
> 
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

These all probably should be done as wrappers around qmp equivalents.

Dave

> ---
>  hmp-commands.hx              | 13 +++++++++++++
>  include/sysemu/device_tree.h |  2 ++
>  monitor/misc.c               | 13 +++++++++++++
>  softmmu/device_tree.c        | 18 ++++++++++++++++++
>  4 files changed, 46 insertions(+)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index c9d465735a..3c134cf652 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1768,3 +1768,16 @@ ERST
>                        "\n\t\t\t -b to specify dirty bitmap as method of calculation)",
>          .cmd        = hmp_calc_dirty_rate,
>      },
> +
> +    {
> +        .name       = "fdt-save",
> +        .args_type  = "filename:s",
> +        .params     = "[filename] file to save the FDT",
> +        .help       = "save the FDT in the 'filename' file to be decoded using dtc",
> +        .cmd        = hmp_fdt_save,
> +    },
> +
> +SRST
> +``fdt-save`` *filename*
> +  Save the FDT in the 'filename' file to be decoded using dtc
> +ERST
> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
> index ef060a9759..1397adb21c 100644
> --- a/include/sysemu/device_tree.h
> +++ b/include/sysemu/device_tree.h
> @@ -123,6 +123,8 @@ int qemu_fdt_nop_node(void *fdt, const char *node_path);
>  int qemu_fdt_add_subnode(void *fdt, const char *name);
>  int qemu_fdt_add_path(void *fdt, const char *path);
>  
> +void fdt_save(const char *filename, Error **errp);
> +
>  #define qemu_fdt_setprop_cells(fdt, node_path, property, ...)                 \
>      do {                                                                      \
>          uint32_t qdt_tmp[] = { __VA_ARGS__ };                                 \
> diff --git a/monitor/misc.c b/monitor/misc.c
> index 3d2312ba8d..145285cec0 100644
> --- a/monitor/misc.c
> +++ b/monitor/misc.c
> @@ -78,6 +78,7 @@
>  #include "qapi/qmp-event.h"
>  #include "sysemu/cpus.h"
>  #include "qemu/cutils.h"
> +#include "sysemu/device_tree.h"
>  
>  #if defined(TARGET_S390X)
>  #include "hw/s390x/storage-keys.h"
> @@ -936,6 +937,18 @@ static void hmp_boot_set(Monitor *mon, const QDict *qdict)
>      }
>  }
>  
> +static void hmp_fdt_save(Monitor *mon, const QDict *qdict)
> +{
> +    const char *path = qdict_get_str(qdict, "filename");
> +    Error *local_err = NULL;
> +
> +    fdt_save(path, &local_err);
> +
> +    if (local_err) {
> +        error_report_err(local_err);
> +    }
> +}
> +
>  static void hmp_info_mtree(Monitor *mon, const QDict *qdict)
>  {
>      bool flatview = qdict_get_try_bool(qdict, "flatview", false);
> diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
> index 6ca3fad285..eeab6a5ef0 100644
> --- a/softmmu/device_tree.c
> +++ b/softmmu/device_tree.c
> @@ -643,3 +643,21 @@ out:
>      g_free(propcells);
>      return ret;
>  }
> +
> +void fdt_save(const char *filename, Error **errp)
> +{
> +    int size;
> +
> +    if (!current_machine->fdt) {
> +        error_setg(errp, "Unable to find the machine FDT");
> +        return;
> +    }
> +
> +    size = fdt_totalsize(current_machine->fdt);
> +
> +    if (g_file_set_contents(filename, current_machine->fdt, size, NULL)) {
> +        return;
> +    }
> +
> +    error_setg(errp, "Error when saving machine FDT to file %s", filename);
> +}
> -- 
> 2.36.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH for-7.2 10/10] hmp, device_tree.c: add 'info fdt <property>' support

[Dr. David Alan Gilbert]

* Daniel Henrique Barboza (danielhb413@gmail.com) wrote:
> 'info fdt' is only able to print full nodes so far. It would be good to
> be able to also print single properties, since ometimes we just want
> to verify a single value from the FDT.
> 
> libfdt does not have support to find a property given its full path, but
> it does have a way to return a fdt_property given a prop name and its
> subnode.
> 
> This is how we're going to support it:
> 
> - given the same fullpath parameter, assume it's a node. If we have a
> match with an existing node, print it. If not, assume it's a property;
> 
> - in fdt_find_property() we're going to split 'fullpath' into node and
> property. Unfortunately we can't use g_path_get_basename() to helps us
> because, although the device tree path format is similar to Linux, it'll
> not work when trying to run QEMU under Windows where the path format is
> different;
> 
> - after spliiting into node + property, try to find the node in the FDT.
> If we have a match, use fdt_get_property() to retrieve fdt_property.
> Return it if found;
> 
> - using the fdt_print_property() created previously, print the property.

Would it be easier to make 'info fdt' have an optional 2nd parameter
(hmp can do optionals) which if present is the property name, then these
would become:

> After this change, if an user wants to print just the value of 'cpu' inside
> /cpu/cpu-map(...) from an ARM FDT, we can do it:
> 
> (qemu) info fdt /cpus/cpu-map/socket0/cluster0/core0/cpu

info fdt /cpus/cpu-map/socket0/cluster0/core0 cpu

> /cpus/cpu-map/socket0/cluster0/core0/cpu = <0x8001>
> (qemu)
> 
> Or the 'ibm,my-dma-window' from the v-scsi device inside the pSeries
> FDT:
> 
> (qemu) info fdt /vdevice/v-scsi@71000003/ibm,my-dma-window

info fdt /vdevice/v-scsi@71000003 ibm,my-dma-window

it seems more explicit, and seems easier to implement.

Dave

> /vdevice/v-scsi@71000003/ibm,my-dma-window = <0x71000003 0x0 0x0 0x0 0x10000000>
> (qemu)
> 
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  hmp-commands-info.hx  |  2 +-
>  softmmu/device_tree.c | 79 ++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 76 insertions(+), 5 deletions(-)
> 
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index abf277be7d..8891c2918a 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -913,7 +913,7 @@ ERST
>          .name       = "fdt",
>          .args_type  = "fullpath:s",
>          .params     = "fullpath",
> -        .help       = "show firmware device tree node given its full path",
> +        .help       = "show firmware device tree node or property given its full path",
>          .cmd        = hmp_info_fdt,
>      },
>  
> diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
> index e41894fbef..f6eb060acc 100644
> --- a/softmmu/device_tree.c
> +++ b/softmmu/device_tree.c
> @@ -774,9 +774,74 @@ static void fdt_print_node(int node, int depth, const char *fullpath)
>      qemu_printf("%*s}\n", padding, "");
>  }
>  
> +static const struct fdt_property *fdt_find_property(const char *fullpath,
> +                                                    int *prop_size,
> +                                                    Error **errp)
> +{
> +    const struct fdt_property *prop = NULL;
> +    void *fdt = current_machine->fdt;
> +    g_autoptr(GString) nodename = NULL;
> +    const char *propname = NULL;
> +    int path_len = strlen(fullpath);
> +    int node = 0; /* default to root node '/' */
> +    int i, idx = -1;
> +
> +    /*
> +     * We'll assume that we're dealing with a property. libfdt
> +     * does not have an API to find a property given the full
> +     * path, but it does have an API to find a property inside
> +     * a node.
> +     */
> +    nodename = g_string_new("");
> +
> +    for (i = path_len - 1; i >= 0; i--) {
> +        if (fullpath[i] == '/') {
> +            idx = i;
> +            break;
> +        }
> +    }
> +
> +    if (idx == -1) {
> +        error_setg(errp, "FDT paths must contain at least one '/' character");
> +        return NULL;
> +    }
> +
> +    if (idx == path_len - 1) {
> +        error_setg(errp, "FDT paths can't end with a '/' character");
> +        return NULL;
> +    }
> +
> +    propname = &fullpath[idx + 1];
> +
> +    if (idx != 0) {
> +        g_string_append_len(nodename, fullpath, idx);
> +
> +        node = fdt_path_offset(fdt, nodename->str);
> +        if (node < 0) {
> +            error_setg(errp, "node '%s' of property '%s' not found in FDT",
> +                       nodename->str, propname);
> +            return NULL;
> +        }
> +    } else {
> +        /* idx = 0 means that it's a property of the root node */
> +        g_string_append(nodename, "/");
> +    }
> +
> +    prop = fdt_get_property(fdt, node, propname, prop_size);
> +    if (!prop) {
> +        error_setg(errp, "property '%s' not found in node '%s' in FDT",
> +                   propname, nodename->str);
> +        return NULL;
> +    }
> +
> +    return prop;
> +}
> +
>  void fdt_info(const char *fullpath, Error **errp)
>  {
> -    int node;
> +    const struct fdt_property *prop = NULL;
> +    Error *local_err = NULL;
> +    int node, prop_size;
>  
>      if (!current_machine->fdt) {
>          error_setg(errp, "Unable to find the machine FDT");
> @@ -784,10 +849,16 @@ void fdt_info(const char *fullpath, Error **errp)
>      }
>  
>      node = fdt_path_offset(current_machine->fdt, fullpath);
> -    if (node < 0) {
> -        error_setg(errp, "node '%s' not found in FDT", fullpath);
> +    if (node >= 0) {
> +        fdt_print_node(node, 0, fullpath);
> +        return;
> +    }
> +
> +    prop = fdt_find_property(fullpath, &prop_size, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
>          return;
>      }
>  
> -    fdt_print_node(node, 0, fullpath);
> +    fdt_print_property(fullpath, prop->data, prop_size, 0);
>  }
> -- 
> 2.36.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH for-7.2 00/10] add hmp 'save-fdt' and 'info fdt' commands

[Daniel Henrique Barboza]

On 7/25/22 06:11, Daniel P. Berrangé wrote:
> On Fri, Jul 22, 2022 at 04:59:57PM -0300, Daniel Henrique Barboza wrote:
>> Hi,
>>
>> After dealing with a FDT element that isn't being shown in the userspace
>> and having to shutdown the guest, dump the FDT using 'machine -dumpdtb' and
>> then using 'dtc' to see what was inside the FDT, I thought it was a good
>> idea to add extra support for FDT handling in QEMU.
>>
>> This series introduces 2 commands. 'fdt-save' behaves similar to what
>> 'machine -dumpdtb' does, with the advantage of saving the FDT of a running
>> guest on demand. This command is implemented in patch 03.
>>
>> The second command, 'info fdt <command>' is more sophisticated. This
>> command can print specific nodes and properties of the FDT. A few
>> examples:
>>
>> - print the /cpus/cpu@0 from an ARM 'virt' machine:
>>
>> (qemu) info fdt /cpus/cpu@0
>> /cpus/cpu@0 {
>>      phandle = <0x8001>
>>      reg = <0x0>
>>      compatible = 'arm,cortex-a57'
>>      device_type = 'cpu'
>> }
>> (qemu)
>>
>> - print the device_type property of the interrupt-controller node of a
>> pSeries machine:
>>
>> (qemu) info fdt /interrupt-controller/device_type
>> /interrupt-controller/device_type = 'PowerPC-External-Interrupt-Presentation'
>> (qemu)
> 
> Please don't add new HMP-only commands. These should be provided
> as QMP commands, where the HMP is a tiny shim to the QMP.

I wasn't sure if this would be useful to be in QMP, but perhaps it's better to
let QMP consumers to decide whether use it or not.

I'll repost both as QMP commands with an HMP layer on top of them.


Daniel

> 
> If you don't want to think about formally modelling the data
> for 'info fdt' / 'query-fdt', then just put an 'x-' prefix on
> the QMP command and return printed formatted data, as illustrated
> in:
> 
> https://www.qemu.org/docs/master/devel/writing-monitor-commands.html#writing-a-debugging-aid-returning-unstructured-text
> 
> With regards,
> Daniel

Re: [PATCH for-7.2 04/10] hmp, device_tree.c: introduce fdt-save

[Daniel Henrique Barboza]

On 7/22/22 20:13, BALATON Zoltan wrote:
> On Fri, 22 Jul 2022, 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 'fdt-save' command receives a 'filename' paramenter and, if a valid
>> 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 HMP.
> 
> If it does the same as -machine dumpdtb wouldn't it be more intuitive to call the HMP command the same, so either dumpdtb or machine-dumpdtb or similar? That way it's more obvious that these do the same.


Good point. I'll rename 'fdt-save' to 'dumpdtb'.


Daniel

> 
> Regards,
> BALATON Zoltan
> 
>> A valid FDT consists of a FDT that was created using libfdt being
>> retrieved via 'current_machine->fdt' in device_tree.c. This condition is
>> met by most FDT users in QEMU.
>>
>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>> hmp-commands.hx              | 13 +++++++++++++
>> include/sysemu/device_tree.h |  2 ++
>> monitor/misc.c               | 13 +++++++++++++
>> softmmu/device_tree.c        | 18 ++++++++++++++++++
>> 4 files changed, 46 insertions(+)
>>
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index c9d465735a..3c134cf652 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -1768,3 +1768,16 @@ ERST
>>                       "\n\t\t\t -b to specify dirty bitmap as method of calculation)",
>>         .cmd        = hmp_calc_dirty_rate,
>>     },
>> +
>> +    {
>> +        .name       = "fdt-save",
>> +        .args_type  = "filename:s",
>> +        .params     = "[filename] file to save the FDT",
>> +        .help       = "save the FDT in the 'filename' file to be decoded using dtc",
>> +        .cmd        = hmp_fdt_save,
>> +    },
>> +
>> +SRST
>> +``fdt-save`` *filename*
>> +  Save the FDT in the 'filename' file to be decoded using dtc
>> +ERST
>> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
>> index ef060a9759..1397adb21c 100644
>> --- a/include/sysemu/device_tree.h
>> +++ b/include/sysemu/device_tree.h
>> @@ -123,6 +123,8 @@ int qemu_fdt_nop_node(void *fdt, const char *node_path);
>> int qemu_fdt_add_subnode(void *fdt, const char *name);
>> int qemu_fdt_add_path(void *fdt, const char *path);
>>
>> +void fdt_save(const char *filename, Error **errp);
>> +
>> #define qemu_fdt_setprop_cells(fdt, node_path, property, ...)                 \
>>     do {                                                                      \
>>         uint32_t qdt_tmp[] = { __VA_ARGS__ };                                 \
>> diff --git a/monitor/misc.c b/monitor/misc.c
>> index 3d2312ba8d..145285cec0 100644
>> --- a/monitor/misc.c
>> +++ b/monitor/misc.c
>> @@ -78,6 +78,7 @@
>> #include "qapi/qmp-event.h"
>> #include "sysemu/cpus.h"
>> #include "qemu/cutils.h"
>> +#include "sysemu/device_tree.h"
>>
>> #if defined(TARGET_S390X)
>> #include "hw/s390x/storage-keys.h"
>> @@ -936,6 +937,18 @@ static void hmp_boot_set(Monitor *mon, const QDict *qdict)
>>     }
>> }
>>
>> +static void hmp_fdt_save(Monitor *mon, const QDict *qdict)
>> +{
>> +    const char *path = qdict_get_str(qdict, "filename");
>> +    Error *local_err = NULL;
>> +
>> +    fdt_save(path, &local_err);
>> +
>> +    if (local_err) {
>> +        error_report_err(local_err);
>> +    }
>> +}
>> +
>> static void hmp_info_mtree(Monitor *mon, const QDict *qdict)
>> {
>>     bool flatview = qdict_get_try_bool(qdict, "flatview", false);
>> diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
>> index 6ca3fad285..eeab6a5ef0 100644
>> --- a/softmmu/device_tree.c
>> +++ b/softmmu/device_tree.c
>> @@ -643,3 +643,21 @@ out:
>>     g_free(propcells);
>>     return ret;
>> }
>> +
>> +void fdt_save(const char *filename, Error **errp)
>> +{
>> +    int size;
>> +
>> +    if (!current_machine->fdt) {
>> +        error_setg(errp, "Unable to find the machine FDT");
>> +        return;
>> +    }
>> +
>> +    size = fdt_totalsize(current_machine->fdt);
>> +
>> +    if (g_file_set_contents(filename, current_machine->fdt, size, NULL)) {
>> +        return;
>> +    }
>> +
>> +    error_setg(errp, "Error when saving machine FDT to file %s", filename);
>> +}
>>

Re: [PATCH for-7.2 00/10] add hmp 'save-fdt' and 'info fdt' commands

[Daniel P. Berrangé]

On Mon, Jul 25, 2022 at 10:16:11AM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 7/25/22 06:11, Daniel P. Berrangé wrote:
> > On Fri, Jul 22, 2022 at 04:59:57PM -0300, Daniel Henrique Barboza wrote:
> > > Hi,
> > > 
> > > After dealing with a FDT element that isn't being shown in the userspace
> > > and having to shutdown the guest, dump the FDT using 'machine -dumpdtb' and
> > > then using 'dtc' to see what was inside the FDT, I thought it was a good
> > > idea to add extra support for FDT handling in QEMU.
> > > 
> > > This series introduces 2 commands. 'fdt-save' behaves similar to what
> > > 'machine -dumpdtb' does, with the advantage of saving the FDT of a running
> > > guest on demand. This command is implemented in patch 03.
> > > 
> > > The second command, 'info fdt <command>' is more sophisticated. This
> > > command can print specific nodes and properties of the FDT. A few
> > > examples:
> > > 
> > > - print the /cpus/cpu@0 from an ARM 'virt' machine:
> > > 
> > > (qemu) info fdt /cpus/cpu@0
> > > /cpus/cpu@0 {
> > >      phandle = <0x8001>
> > >      reg = <0x0>
> > >      compatible = 'arm,cortex-a57'
> > >      device_type = 'cpu'
> > > }
> > > (qemu)
> > > 
> > > - print the device_type property of the interrupt-controller node of a
> > > pSeries machine:
> > > 
> > > (qemu) info fdt /interrupt-controller/device_type
> > > /interrupt-controller/device_type = 'PowerPC-External-Interrupt-Presentation'
> > > (qemu)
> > 
> > Please don't add new HMP-only commands. These should be provided
> > as QMP commands, where the HMP is a tiny shim to the QMP.
> 
> I wasn't sure if this would be useful to be in QMP, but perhaps it's better to
> let QMP consumers to decide whether use it or not.

That's not a relevant question to consider. The end goal is for HMP
to be 100% implemented in terms of QMP commands. So if anything is
required for HMP, then by definition it is also required for QMP.

The only question is whether the QMP command should be marked stable
or unstable. If there's any doubt, that pushes towards the 'unstable'
side, such that we don't have to promise API compat

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|