[PATCH for-7.2 v4 00/21] QMP/HMP: add 'dumpdtb' and 'info fdt' commands

[PATCH for-7.2 v4 00/21] QMP/HMP: add 'dumpdtb' and 'info fdt' commands

[Daniel Henrique Barboza]

Hi,

In this new version we have changes based on review comments from v3.

I've also added a new patch (21) that adds an extra parameter called
'textformat' to the dumpdtb QMP command. Using this parameter will
write the FDT in text format, using DTS compatible formatting like
the 'info fdt' command is already using. This will allow users to
see the FDT contents in a text file without the need of decoding the
blob using 'dtc':

    (qemu) dumpdtb -t fdt.txt
    
    $ file fdt.txt
    fdt.txt: ASCII text, with very long lines (4746)
    
    $ grep -A 3 'persistent-memory' fdt.txt
        ibm,persistent-memory {
            device_type = "ibm,persistent-memory";
            #size-cells = <0x0>;
            #address-cells = <0x1>;
        };


Changes from v3:
- patch 10:
  - clarified in the commit message that, in the current code base, the
    spapr internal FDT won't always match what the guest is using 
- patch 14:
  - changed 'filename' to type F
  - use 'hmp_handle_error' instead of 'error_report_err'
- patch 15:
  - added a semi-colon at the end of each closing bracket
- patch 16:
  - added an extra space after each individual string in the string
    array
- patch 20:
  - fixed commit msg with the actual output of 'info fdt'
- patch 21 (NEW):
  - added a '-t' parameter to 'dumpdtb' to create a FDT file in plain
    text format, using the same formatting we already implemented with
    'info fdt'
- v3 link:
  https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg02419.html 

Daniel Henrique Barboza (21):
  hw/arm: do not free machine->fdt in arm_load_dtb()
  hw/microblaze: set machine->fdt in microblaze_load_dtb()
  hw/nios2: set machine->fdt in nios2_load_dtb()
  hw/ppc: set machine->fdt in ppce500_load_device_tree()
  hw/ppc: set machine->fdt in bamboo_load_device_tree()
  hw/ppc: set machine->fdt in sam460ex_load_device_tree()
  hw/ppc: set machine->fdt in xilinx_load_device_tree()
  hw/ppc: set machine->fdt in pegasos2_machine_reset()
  hw/ppc: set machine->fdt in pnv_reset()
  hw/ppc: set machine->fdt in spapr machine
  hw/riscv: set machine->fdt in sifive_u_machine_init()
  hw/riscv: set machine->fdt in spike_board_init()
  hw/xtensa: set machine->fdt in xtfpga_init()
  qmp/hmp, device_tree.c: introduce dumpdtb
  qmp/hmp, device_tree.c: introduce 'info fdt' command
  device_tree.c: support string array prop in fdt_format_node()
  device_tree.c: support remaining FDT prop types
  device_node.c: enable 'info fdt' to print subnodes
  device_tree.c: add fdt_format_property() helper
  hmp, device_tree.c: add 'info fdt <property>' support
  qmp/hmp, device_tree.c: add textformat dumpdtb option

 hmp-commands-info.hx         |  14 +++
 hmp-commands.hx              |  14 +++
 hw/arm/boot.c                |   6 +-
 hw/microblaze/boot.c         |  11 +-
 hw/microblaze/meson.build    |   2 +-
 hw/nios2/boot.c              |  11 +-
 hw/nios2/meson.build         |   2 +-
 hw/ppc/e500.c                |  13 +-
 hw/ppc/pegasos2.c            |   7 ++
 hw/ppc/pnv.c                 |   8 +-
 hw/ppc/ppc440_bamboo.c       |  11 +-
 hw/ppc/sam460ex.c            |   8 +-
 hw/ppc/spapr.c               |   6 +
 hw/ppc/spapr_hcall.c         |   8 ++
 hw/ppc/virtex_ml507.c        |  11 +-
 hw/riscv/sifive_u.c          |   6 +
 hw/riscv/spike.c             |   9 ++
 hw/xtensa/meson.build        |   2 +-
 hw/xtensa/xtfpga.c           |   9 +-
 include/monitor/hmp.h        |   2 +
 include/sysemu/device_tree.h |   8 ++
 monitor/hmp-cmds.c           |  29 +++++
 monitor/qmp-cmds.c           |  29 +++++
 qapi/machine.json            |  38 ++++++
 softmmu/device_tree.c        | 238 +++++++++++++++++++++++++++++++++++
 25 files changed, 490 insertions(+), 12 deletions(-)

-- 
2.37.2

[PATCH for-7.2 v4 01/21] hw/arm: 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 | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index ada2717f76..669a978157 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -684,7 +684,11 @@ 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 the ms->fdt pointer to enable support for
+     * 'dumpdtb' and 'info fdt' QMP/HMP commands.
+     */
+    ms->fdt = fdt;
 
     return size;
 
-- 
2.37.2

[PATCH for-7.2 v4 02/21] hw/microblaze: set machine->fdt in microblaze_load_dtb()

[Daniel Henrique Barboza]

This will enable support for 'dumpdtb' and 'info fdt' HMP commands for
all microblaze machines that uses microblaze_load_dtb().

Cc: Edgar E. Iglesias <edgar.iglesias@gmail.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/microblaze/boot.c      | 11 ++++++++++-
 hw/microblaze/meson.build |  2 +-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/hw/microblaze/boot.c b/hw/microblaze/boot.c
index 8b92a9801a..e9ebc04381 100644
--- a/hw/microblaze/boot.c
+++ b/hw/microblaze/boot.c
@@ -39,6 +39,8 @@
 
 #include "boot.h"
 
+#include <libfdt.h>
+
 static struct
 {
     void (*machine_cpu_reset)(MicroBlazeCPU *);
@@ -72,6 +74,7 @@ static int microblaze_load_dtb(hwaddr addr,
                                const char *kernel_cmdline,
                                const char *dtb_filename)
 {
+    MachineState *machine = MACHINE(qdev_get_machine());
     int fdt_size;
     void *fdt = NULL;
     int r;
@@ -100,7 +103,13 @@ static int microblaze_load_dtb(hwaddr addr,
     }
 
     cpu_physical_memory_write(addr, fdt, fdt_size);
-    g_free(fdt);
+
+    /*
+     * Update the machine->fdt pointer to enable support for
+     * 'dumpdtb' and 'info fdt' QMP/HMP commands.
+     */
+    machine->fdt = fdt;
+
     return fdt_size;
 }
 
diff --git a/hw/microblaze/meson.build b/hw/microblaze/meson.build
index bb9e4eb8f4..a38a397872 100644
--- a/hw/microblaze/meson.build
+++ b/hw/microblaze/meson.build
@@ -1,5 +1,5 @@
 microblaze_ss = ss.source_set()
-microblaze_ss.add(files('boot.c'))
+microblaze_ss.add(files('boot.c'), fdt)
 microblaze_ss.add(when: 'CONFIG_PETALOGIX_S3ADSP1800', if_true: files('petalogix_s3adsp1800_mmu.c'))
 microblaze_ss.add(when: 'CONFIG_PETALOGIX_ML605', if_true: files('petalogix_ml605_mmu.c'))
 microblaze_ss.add(when: 'CONFIG_XLNX_ZYNQMP_PMU', if_true: files('xlnx-zynqmp-pmu.c'))
-- 
2.37.2

[PATCH for-7.2 v4 03/21] hw/nios2: set machine->fdt in nios2_load_dtb()

[Daniel Henrique Barboza]

This will enable support for 'dumpdtb' and 'info fdt' HMP commands for
all nios2 machines that uses nios2_load_dtb().

Cc: Chris Wulff <crwulff@gmail.com>
Cc: Marek Vasut <marex@denx.de>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/nios2/boot.c      | 11 ++++++++++-
 hw/nios2/meson.build |  2 +-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/hw/nios2/boot.c b/hw/nios2/boot.c
index 21cbffff47..db3b21fea6 100644
--- a/hw/nios2/boot.c
+++ b/hw/nios2/boot.c
@@ -43,6 +43,8 @@
 
 #include "boot.h"
 
+#include <libfdt.h>
+
 #define NIOS2_MAGIC    0x534f494e
 
 static struct nios2_boot_info {
@@ -81,6 +83,7 @@ static uint64_t translate_kernel_address(void *opaque, uint64_t addr)
 static int nios2_load_dtb(struct nios2_boot_info bi, const uint32_t ramsize,
                           const char *kernel_cmdline, const char *dtb_filename)
 {
+    MachineState *machine = MACHINE(qdev_get_machine());
     int fdt_size;
     void *fdt = NULL;
     int r;
@@ -113,7 +116,13 @@ static int nios2_load_dtb(struct nios2_boot_info bi, const uint32_t ramsize,
     }
 
     cpu_physical_memory_write(bi.fdt, fdt, fdt_size);
-    g_free(fdt);
+
+    /*
+     * Update the machine->fdt pointer to enable support for
+     * 'dumpdtb' and 'info fdt' QMP/HMP commands.
+     */
+    machine->fdt = fdt;
+
     return fdt_size;
 }
 
diff --git a/hw/nios2/meson.build b/hw/nios2/meson.build
index 6c58e8082b..22277bd6c5 100644
--- a/hw/nios2/meson.build
+++ b/hw/nios2/meson.build
@@ -1,5 +1,5 @@
 nios2_ss = ss.source_set()
-nios2_ss.add(files('boot.c'))
+nios2_ss.add(files('boot.c'), fdt)
 nios2_ss.add(when: 'CONFIG_NIOS2_10M50', if_true: files('10m50_devboard.c'))
 nios2_ss.add(when: 'CONFIG_NIOS2_GENERIC_NOMMU', if_true: files('generic_nommu.c'))
 
-- 
2.37.2

[PATCH for-7.2 v4 04/21] hw/ppc: set machine->fdt in ppce500_load_device_tree()

[Daniel Henrique Barboza]

This will enable support for 'dumpdtb' and 'info fdt' HMP commands for
the e500 machine.

Cc: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/e500.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 32495d0123..6197a905b8 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -47,6 +47,8 @@
 #include "hw/i2c/i2c.h"
 #include "hw/irq.h"
 
+#include <libfdt.h>
+
 #define EPAPR_MAGIC                (0x45504150)
 #define DTC_LOAD_PAD               0x1800000
 #define DTC_PAD_MASK               0xFFFFF
@@ -600,7 +602,16 @@ done:
         cpu_physical_memory_write(addr, fdt, fdt_size);
     }
     ret = fdt_size;
-    g_free(fdt);
+
+    /*
+     * Update the machine->fdt pointer to enable support for
+     * 'dumpdtb' and 'info fdt' QMP/HMP commands.
+     *
+     * The FDT is re-created during reset, so free machine->fdt
+     * to avoid leaking the old FDT.
+     */
+    g_free(machine->fdt);
+    machine->fdt = fdt;
 
 out:
     g_free(pci_map);
-- 
2.37.2

[PATCH for-7.2 v4 05/21] hw/ppc: set machine->fdt in bamboo_load_device_tree()

[Daniel Henrique Barboza]

This will enable support for 'dumpdtb' and 'info fdt' HMP commands for
the bamboo machine.

Cc: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/ppc440_bamboo.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
index 873f930c77..9c89b23c08 100644
--- a/hw/ppc/ppc440_bamboo.c
+++ b/hw/ppc/ppc440_bamboo.c
@@ -34,6 +34,8 @@
 #include "hw/qdev-properties.h"
 #include "qapi/error.h"
 
+#include <libfdt.h>
+
 #define BINARY_DEVICE_TREE_FILE "bamboo.dtb"
 
 /* from u-boot */
@@ -62,6 +64,7 @@ static int bamboo_load_device_tree(hwaddr addr,
                                      hwaddr initrd_size,
                                      const char *kernel_cmdline)
 {
+    MachineState *machine = MACHINE(qdev_get_machine());
     int ret = -1;
     uint32_t mem_reg_property[] = { 0, 0, cpu_to_be32(ramsize) };
     char *filename;
@@ -116,7 +119,13 @@ static int bamboo_load_device_tree(hwaddr addr,
                           tb_freq);
 
     rom_add_blob_fixed(BINARY_DEVICE_TREE_FILE, fdt, fdt_size, addr);
-    g_free(fdt);
+
+    /*
+     * Update the machine->fdt pointer to enable support for
+     * 'dumpdtb' and 'info fdt' QMP/HMP commands.
+     */
+    machine->fdt = fdt;
+
     return 0;
 }
 
-- 
2.37.2

[PATCH for-7.2 v4 06/21] hw/ppc: set machine->fdt in sam460ex_load_device_tree()

[Daniel Henrique Barboza]

This will enable support for 'dumpdtb' and 'info fdt' HMP commands for
the sam460ex machine.

Cc: BALATON Zoltan <balaton@eik.bme.hu>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/sam460ex.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index 0357ee077f..413a425d37 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -138,6 +138,7 @@ static int sam460ex_load_device_tree(hwaddr addr,
                                      hwaddr initrd_size,
                                      const char *kernel_cmdline)
 {
+    MachineState *machine = MACHINE(qdev_get_machine());
     uint32_t mem_reg_property[] = { 0, 0, cpu_to_be32(ramsize) };
     char *filename;
     int fdt_size;
@@ -209,7 +210,12 @@ static int sam460ex_load_device_tree(hwaddr addr,
                               EBC_FREQ);
 
     rom_add_blob_fixed(BINARY_DEVICE_TREE_FILE, fdt, fdt_size, addr);
-    g_free(fdt);
+
+    /*
+     * Update the machine->fdt pointer to enable support for
+     * 'dumpdtb' and 'info fdt' QMP/HMP commands.
+     */
+    machine->fdt = fdt;
 
     return fdt_size;
 }
-- 
2.37.2

[PATCH for-7.2 v4 07/21] hw/ppc: set machine->fdt in xilinx_load_device_tree()

[Daniel Henrique Barboza]

This will enable support for 'dumpdtb' and 'info fdt' HMP commands for
the virtex_ml507 machine.

Cc: Edgar E. Iglesias <edgar.iglesias@gmail.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/virtex_ml507.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
index 53b126ff48..9f4c5d85a4 100644
--- a/hw/ppc/virtex_ml507.c
+++ b/hw/ppc/virtex_ml507.c
@@ -45,6 +45,8 @@
 #include "hw/qdev-properties.h"
 #include "ppc405.h"
 
+#include <libfdt.h>
+
 #define EPAPR_MAGIC    (0x45504150)
 #define FLASH_SIZE     (16 * MiB)
 
@@ -153,6 +155,7 @@ static int xilinx_load_device_tree(hwaddr addr,
                                       hwaddr initrd_size,
                                       const char *kernel_cmdline)
 {
+    MachineState *machine = MACHINE(qdev_get_machine());
     char *path;
     int fdt_size;
     void *fdt = NULL;
@@ -197,7 +200,13 @@ static int xilinx_load_device_tree(hwaddr addr,
     if (r < 0)
         fprintf(stderr, "couldn't set /chosen/bootargs\n");
     cpu_physical_memory_write(addr, fdt, fdt_size);
-    g_free(fdt);
+
+    /*
+     * Update the machine->fdt pointer to enable support for
+     * 'dumpdtb' and 'info fdt' QMP/HMP commands.
+     */
+    machine->fdt = fdt;
+
     return fdt_size;
 }
 
-- 
2.37.2

[PATCH for-7.2 v4 08/21] hw/ppc: set machine->fdt in pegasos2_machine_reset()

[Daniel Henrique Barboza]

We'll introduce QMP/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 | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
index 61f4263953..624036d88b 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -331,6 +331,13 @@ static void pegasos2_machine_reset(MachineState *machine)
 
     vof_build_dt(fdt, pm->vof);
     vof_client_open_store(fdt, pm->vof, "/chosen", "stdout", "/failsafe");
+
+    /*
+     * Set the common machine->fdt pointer to enable support
+     * for 'dumpdtb' and 'info fdt' QMP/HMP commands.
+     */
+    machine->fdt = fdt;
+
     pm->cpu->vhyp = PPC_VIRTUAL_HYPERVISOR(machine);
 }
 
-- 
2.37.2

[PATCH for-7.2 v4 09/21] hw/ppc: set machine->fdt in pnv_reset()

[Daniel Henrique Barboza]

This will enable support for 'dumpdtb' and 'info fdt' HMP commands for
all powernv machines.

Cc: Cédric Le Goater <clg@kaod.org>
Cc: Frederic Barrat <fbarrat@linux.ibm.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/pnv.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index d3f77c8367..296995a600 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -608,7 +608,13 @@ static void pnv_reset(MachineState *machine)
     qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
     cpu_physical_memory_write(PNV_FDT_ADDR, fdt, fdt_totalsize(fdt));
 
-    g_free(fdt);
+    /*
+     * Update the machine->fdt pointer to enable support for
+     * 'dumpdtb' and 'info fdt' commands. Free the existing
+     * machine->fdt to avoid leaking it during a reset.
+     */
+    g_free(machine->fdt);
+    machine->fdt = fdt;
 }
 
 static ISABus *pnv_chip_power8_isa_create(PnvChip *chip, Error **errp)
-- 
2.37.2

[PATCH for-7.2 v4 10/21] 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 two places where we manipulate the FDT:
spapr_machine_reset() and CAS. There are other places where the FDT is
manipulated in the pSeries machines, most notably the hotplug/unplug
path. For now we'll acknowledge that we won't have the most accurate
representation of the FDT, depending on the current machine state, when
using these QMP/HMP fdt commands. Making the internal FDT representation
always match the actual FDT representation that the guest is using is a
problem for another day.

spapr->fdt_blob is left untouched for now. To replace it with
machine->fdt, since we're migrating spapr->fdt_blob, we would need to
migrate machine->fdt as well. This is something that we would like to to
do keep our code simpler but it's also a work we'll leave for later.

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       | 6 ++++++
 hw/ppc/spapr_hcall.c | 8 ++++++++
 2 files changed, 14 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index bc9ba6e6dc..7031cf964a 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1713,6 +1713,12 @@ static void spapr_machine_reset(MachineState *machine)
     spapr->fdt_initial_size = spapr->fdt_size;
     spapr->fdt_blob = fdt;
 
+    /*
+     * Set the common machine->fdt pointer to enable support
+     * for 'dumpdtb' and 'info fdt' QMP/HMP commands.
+     */
+    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..a53bfd76f4 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1256,6 +1256,14 @@ target_ulong do_client_architecture_support(PowerPCCPU *cpu,
     spapr->fdt_initial_size = spapr->fdt_size;
     spapr->fdt_blob = fdt;
 
+    /*
+     * Set the machine->fdt pointer again since we just freed
+     * it above (by freeing spapr->fdt_blob). We set this
+     * pointer to enable support for 'dumpdtb' and 'info fdt'
+     * QMP/HMP commands.
+     */
+    MACHINE(spapr)->fdt = fdt;
+
     return H_SUCCESS;
 }
 
-- 
2.37.2

[PATCH for-7.2 v4 11/21] hw/riscv: set machine->fdt in sifive_u_machine_init()

[Daniel Henrique Barboza]

This will enable support for 'dumpdtb' and 'info fdt' HMP commands for
the sifive_u machine.

Cc: Alistair Francis <Alistair.Francis@wdc.com>
Cc: Bin Meng <bin.meng@windriver.com>
Cc: Palmer Dabbelt <palmer@dabbelt.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/riscv/sifive_u.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index e4c814a3ea..f14d8411df 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -634,6 +634,12 @@ static void sifive_u_machine_init(MachineState *machine)
         start_addr_hi32 = (uint64_t)start_addr >> 32;
     }
 
+    /*
+     * Update the machine->fdt pointer to enable support for
+     * 'dumpdtb' and 'info fdt' QMP/HMP commands.
+     */
+    machine->fdt = s->fdt;
+
     /* reset vector */
     uint32_t reset_vec[12] = {
         s->msel,                       /* MSEL pin state */
-- 
2.37.2

[PATCH for-7.2 v4 12/21] hw/riscv: set machine->fdt in spike_board_init()

[Daniel Henrique Barboza]

This will enable support for 'dumpdtb' and 'info fdt' HMP commands for
the spike machine.

Cc: Palmer Dabbelt <palmer@dabbelt.com>
Cc: Alistair Francis <alistair.francis@wdc.com>
Cc: Bin Meng <bin.meng@windriver.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/riscv/spike.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index e41b6aa9f0..17f517bfa6 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -40,6 +40,8 @@
 #include "sysemu/device_tree.h"
 #include "sysemu/sysemu.h"
 
+#include <libfdt.h>
+
 static const MemMapEntry spike_memmap[] = {
     [SPIKE_MROM] =     {     0x1000,     0xf000 },
     [SPIKE_HTIF] =     {  0x1000000,     0x1000 },
@@ -304,6 +306,13 @@ static void spike_board_init(MachineState *machine)
     /* Compute the fdt load address in dram */
     fdt_load_addr = riscv_load_fdt(memmap[SPIKE_DRAM].base,
                                    machine->ram_size, s->fdt);
+
+    /*
+     * Update the machine->fdt pointer to enable support for
+     * 'dumpdtb' and 'info fdt' QMP/HMP commands.
+     */
+    machine->fdt = s->fdt;
+
     /* load the reset vector */
     riscv_setup_rom_reset_vec(machine, &s->soc[0], memmap[SPIKE_DRAM].base,
                               memmap[SPIKE_MROM].base,
-- 
2.37.2

[PATCH for-7.2 v4 13/21] hw/xtensa: set machine->fdt in xtfpga_init()

[Daniel Henrique Barboza]

This will enable support for 'dumpdtb' and 'info fdt' HMP commands for
all xtensa machines that uses a FDT.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/xtensa/meson.build | 2 +-
 hw/xtensa/xtfpga.c    | 9 ++++++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/hw/xtensa/meson.build b/hw/xtensa/meson.build
index 1d5835df4b..ebba51cc74 100644
--- a/hw/xtensa/meson.build
+++ b/hw/xtensa/meson.build
@@ -6,6 +6,6 @@ xtensa_ss.add(files(
 ))
 xtensa_ss.add(when: 'CONFIG_XTENSA_SIM', if_true: files('sim.c'))
 xtensa_ss.add(when: 'CONFIG_XTENSA_VIRT', if_true: files('virt.c'))
-xtensa_ss.add(when: 'CONFIG_XTENSA_XTFPGA', if_true: files('xtfpga.c'))
+xtensa_ss.add(when: 'CONFIG_XTENSA_XTFPGA', if_true: [files('xtfpga.c'), fdt])
 
 hw_arch += {'xtensa': xtensa_ss}
diff --git a/hw/xtensa/xtfpga.c b/hw/xtensa/xtfpga.c
index 2a5556a35f..9e2f911caa 100644
--- a/hw/xtensa/xtfpga.c
+++ b/hw/xtensa/xtfpga.c
@@ -50,6 +50,8 @@
 #include "hw/xtensa/mx_pic.h"
 #include "migration/vmstate.h"
 
+#include <libfdt.h>
+
 typedef struct XtfpgaFlashDesc {
     hwaddr base;
     size_t size;
@@ -377,7 +379,12 @@ static void xtfpga_init(const XtfpgaBoardDesc *board, MachineState *machine)
             cur_tagptr = put_tag(cur_tagptr, BP_TAG_FDT,
                                  sizeof(dtb_addr), &dtb_addr);
             cur_lowmem = QEMU_ALIGN_UP(cur_lowmem + fdt_size, 4 * KiB);
-            g_free(fdt);
+
+            /*
+             * Update the machine->fdt pointer to enable support for
+             * 'dumpdtb' and 'info fdt' QMP/HMP commands.
+             */
+            machine->fdt = fdt;
         }
 #else
         if (dtb_filename) {
-- 
2.37.2

[PATCH for-7.2 v4 14/21] qmp/hmp, device_tree.c: introduce dumpdtb

[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 'dumpdtb' 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 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 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>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hmp-commands.hx              | 13 +++++++++++++
 include/monitor/hmp.h        |  1 +
 include/sysemu/device_tree.h |  1 +
 monitor/hmp-cmds.c           | 12 ++++++++++++
 monitor/qmp-cmds.c           | 13 +++++++++++++
 qapi/machine.json            | 17 +++++++++++++++++
 softmmu/device_tree.c        | 18 ++++++++++++++++++
 7 files changed, 75 insertions(+)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 182e639d14..2dd737078e 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1800,3 +1800,16 @@ ERST
                       "\n\t\t\t\t\t limit on a specified virtual cpu",
         .cmd        = hmp_cancel_vcpu_dirty_limit,
     },
+
+SRST
+``dumpdtb`` *filename*
+  Save the FDT in the 'filename' file to be decoded using dtc.
+  Requires 'libfdt' support.
+ERST
+    {
+        .name       = "dumpdtb",
+        .args_type  = "filename:F",
+        .params     = "filename",
+        .help       = "save the FDT in the 'filename' file to be decoded using dtc",
+        .cmd        = hmp_dumpdtb,
+    },
diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
index a618eb1e4e..d7f324da59 100644
--- a/include/monitor/hmp.h
+++ b/include/monitor/hmp.h
@@ -134,6 +134,7 @@ void hmp_calc_dirty_rate(Monitor *mon, const QDict *qdict);
 void hmp_set_vcpu_dirty_limit(Monitor *mon, const QDict *qdict);
 void hmp_cancel_vcpu_dirty_limit(Monitor *mon, const QDict *qdict);
 void hmp_info_vcpu_dirty_limit(Monitor *mon, const QDict *qdict);
+void hmp_dumpdtb(Monitor *mon, const QDict *qdict);
 void hmp_human_readable_text_helper(Monitor *mon,
                                     HumanReadableText *(*qmp_handler)(Error **));
 void hmp_info_stats(Monitor *mon, const QDict *qdict);
diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
index ef060a9759..bf7684e4ed 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 qemu_fdt_qmp_dumpdtb(const char *filename, Error **errp);
 
 /**
  * qemu_fdt_setprop_sized_cells_from_array:
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index c6cd6f91dd..1c7bfd3b9d 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -2472,3 +2472,15 @@ exit:
 exit_no_print:
     error_free(err);
 }
+
+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);
+
+    if (local_err) {
+        hmp_handle_error(mon, local_err);
+    }
+}
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 7314cd813d..8415aca08c 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -45,6 +45,7 @@
 #include "hw/intc/intc.h"
 #include "hw/rdma/rdma.h"
 #include "monitor/stats.h"
+#include "sysemu/device_tree.h"
 
 NameInfo *qmp_query_name(Error **errp)
 {
@@ -596,3 +597,15 @@ bool apply_str_list_filter(const char *string, strList *list)
     }
     return false;
 }
+
+#ifdef CONFIG_FDT
+void qmp_dumpdtb(const char *filename, Error **errp)
+{
+    return qemu_fdt_qmp_dumpdtb(filename, errp);
+}
+#else
+void qmp_dumpdtb(const char *filename, Error **errp)
+{
+    error_setg(errp, "dumpdtb requires libfdt");
+}
+#endif
diff --git a/qapi/machine.json b/qapi/machine.json
index 6afd1936b0..aeb013f3dd 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1664,3 +1664,20 @@
      '*size': 'size',
      '*max-size': 'size',
      '*slots': 'uint64' } }
+
+##
+# @dumpdtb:
+#
+# Save the FDT in dtb format. Requires 'libfdt' support.
+#
+# @filename: name of the FDT file to be created
+#
+# Since: 7.2
+#
+# Example:
+#   {"execute": "dumpdtb"}
+#    "arguments": { "filename": "/tmp/fdt.dtb" } }
+#
+##
+{ 'command': 'dumpdtb',
+  'data': { 'filename': 'str' } }
diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
index 6ca3fad285..cd487ddd4d 100644
--- a/softmmu/device_tree.c
+++ b/softmmu/device_tree.c
@@ -643,3 +643,21 @@ out:
     g_free(propcells);
     return ret;
 }
+
+void qemu_fdt_qmp_dumpdtb(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.37.2

[PATCH for-7.2 v4 15/21] qmp/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 node or property.

'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 QMP/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,
and then individual properties.

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).

'info fdt' is not something that we expect to be used aside from debugging,
so we're implementing it in QMP as 'x-query-fdt'.

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>
Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hmp-commands-info.hx         | 13 ++++++++++
 include/monitor/hmp.h        |  1 +
 include/sysemu/device_tree.h |  4 +++
 monitor/hmp-cmds.c           | 13 ++++++++++
 monitor/qmp-cmds.c           | 12 +++++++++
 qapi/machine.json            | 19 +++++++++++++++
 softmmu/device_tree.c        | 47 ++++++++++++++++++++++++++++++++++++
 7 files changed, 109 insertions(+)

diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index 188d9ece3b..743b48865d 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -921,3 +921,16 @@ SRST
   ``stats``
     Show runtime-collected statistics
 ERST
+
+    {
+        .name       = "fdt",
+        .args_type  = "nodepath:s",
+        .params     = "nodepath",
+        .help       = "show firmware device tree node given its path",
+        .cmd        = hmp_info_fdt,
+    },
+
+SRST
+  ``info fdt``
+    Show a firmware device tree node given its path. Requires libfdt.
+ERST
diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
index d7f324da59..c0883dd1e3 100644
--- a/include/monitor/hmp.h
+++ b/include/monitor/hmp.h
@@ -135,6 +135,7 @@ void hmp_set_vcpu_dirty_limit(Monitor *mon, const QDict *qdict);
 void hmp_cancel_vcpu_dirty_limit(Monitor *mon, const QDict *qdict);
 void hmp_info_vcpu_dirty_limit(Monitor *mon, const QDict *qdict);
 void hmp_dumpdtb(Monitor *mon, const QDict *qdict);
+void hmp_info_fdt(Monitor *mon, const QDict *qdict);
 void hmp_human_readable_text_helper(Monitor *mon,
                                     HumanReadableText *(*qmp_handler)(Error **));
 void hmp_info_stats(Monitor *mon, const QDict *qdict);
diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
index bf7684e4ed..057d13e397 100644
--- a/include/sysemu/device_tree.h
+++ b/include/sysemu/device_tree.h
@@ -14,6 +14,8 @@
 #ifndef DEVICE_TREE_H
 #define DEVICE_TREE_H
 
+#include "qapi/qapi-types-common.h"
+
 void *create_device_tree(int *sizep);
 void *load_device_tree(const char *filename_path, int *sizep);
 #ifdef CONFIG_LINUX
@@ -137,6 +139,8 @@ int qemu_fdt_add_path(void *fdt, const char *path);
 
 void qemu_fdt_dumpdtb(void *fdt, int size);
 void qemu_fdt_qmp_dumpdtb(const char *filename, Error **errp);
+HumanReadableText *qemu_fdt_qmp_query_fdt(const char *nodepath,
+                                          Error **errp);
 
 /**
  * qemu_fdt_setprop_sized_cells_from_array:
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 1c7bfd3b9d..93a4103afa 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -2484,3 +2484,16 @@ void hmp_dumpdtb(Monitor *mon, const QDict *qdict)
         hmp_handle_error(mon, local_err);
     }
 }
+
+void hmp_info_fdt(Monitor *mon, const QDict *qdict)
+{
+    const char *nodepath = qdict_get_str(qdict, "nodepath");
+    Error *err = NULL;
+    g_autoptr(HumanReadableText) info = qmp_x_query_fdt(nodepath, &err);
+
+    if (hmp_handle_error(mon, err)) {
+        return;
+    }
+
+    monitor_printf(mon, "%s", info->human_readable_text);
+}
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 8415aca08c..db2c6aa7da 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -603,9 +603,21 @@ void qmp_dumpdtb(const char *filename, Error **errp)
 {
     return qemu_fdt_qmp_dumpdtb(filename, errp);
 }
+
+HumanReadableText *qmp_x_query_fdt(const char *nodepath, Error **errp)
+{
+    return qemu_fdt_qmp_query_fdt(nodepath, errp);
+}
 #else
 void qmp_dumpdtb(const char *filename, Error **errp)
 {
     error_setg(errp, "dumpdtb requires libfdt");
 }
+
+HumanReadableText *qmp_x_query_fdt(const char *nodepath, Error **errp)
+{
+    error_setg(errp, "this command requires libfdt");
+
+    return NULL;
+}
 #endif
diff --git a/qapi/machine.json b/qapi/machine.json
index aeb013f3dd..96cff541ca 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1681,3 +1681,22 @@
 ##
 { 'command': 'dumpdtb',
   'data': { 'filename': 'str' } }
+
+##
+# @x-query-fdt:
+#
+# Query for FDT element (node or property). Requires 'libfdt'.
+#
+# @nodepath: the path of the FDT node to be retrieved
+#
+# Features:
+# @unstable: This command is meant for debugging.
+#
+# Returns: FDT node
+#
+# Since: 7.2
+##
+{ 'command': 'x-query-fdt',
+  'data': { 'nodepath': 'str' },
+  'returns': 'HumanReadableText',
+  'features': [ 'unstable' ]  }
diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
index cd487ddd4d..6b15f6ace2 100644
--- a/softmmu/device_tree.c
+++ b/softmmu/device_tree.c
@@ -18,6 +18,7 @@
 #endif
 
 #include "qapi/error.h"
+#include "qapi/type-helpers.h"
 #include "qemu/error-report.h"
 #include "qemu/option.h"
 #include "qemu/bswap.h"
@@ -661,3 +662,49 @@ void qemu_fdt_qmp_dumpdtb(const char *filename, Error **errp)
 
     error_setg(errp, "Error when saving machine FDT to file %s", filename);
 }
+
+static void fdt_format_node(GString *buf, 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;
+
+    g_string_append_printf(buf, "%*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));
+
+        g_string_append_printf(buf, "%*s%s;\n", padding, "", propname);
+    }
+
+    padding -= 4;
+    g_string_append_printf(buf, "%*s};\n", padding, "");
+}
+
+HumanReadableText *qemu_fdt_qmp_query_fdt(const char *nodepath, Error **errp)
+{
+    g_autoptr(GString) buf = g_string_new("");
+    int node;
+
+    if (!current_machine->fdt) {
+        error_setg(errp, "Unable to find the machine FDT");
+        return NULL;
+    }
+
+    node = fdt_path_offset(current_machine->fdt, nodepath);
+    if (node < 0) {
+        error_setg(errp, "node '%s' not found in FDT", nodepath);
+        return NULL;
+    }
+
+    fdt_format_node(buf, node, 0);
+
+    return human_readable_text_from_str(buf);
+}
-- 
2.37.2

[PATCH for-7.2 v4 16/21] device_tree.c: support string array prop in fdt_format_node()

[Daniel Henrique Barboza]

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

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

- check if the array finishes with a null character
- check if there's no empty string in the middle of the array (i.e.
consecutive \0\0 characters)
- check if all characters of each substring are printable

If all conditions are met, we'll consider it to be a string array data
type and print it accordingly. After this change, 'info fdt' is now able
to print string arrays. Here's an example of string arrays we're able to
print in the /rtas node of the ppc64 pSeries machine:

(qemu) info fdt /rtas
rtas {
    (...)
    qemu,hypertas-functions = "hcall-memop1";
    ibm,hypertas-functions = "hcall-pft", "hcall-term", "hcall-dabr", "hcall-interrupt", "hcall-tce", "hcall-vio", "hcall-splpar", "hcall-join", "hcall-bulk", "hcall-set-mode", "hcall-sprg0", "hcall-copy", "hcall-debug", "hcall-vphn", "hcall-multi-tce", "hcall-hpt-resize", "hcall-watchdog";
};

'qemu,hypertas-functions' is a property with a single string while
'ibm,hypertas-functions' is a string array.

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

diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
index 6b15f6ace2..3e38d9ddad 100644
--- a/softmmu/device_tree.c
+++ b/softmmu/device_tree.c
@@ -663,6 +663,63 @@ void qemu_fdt_qmp_dumpdtb(const char *filename, Error **errp)
     error_setg(errp, "Error when saving machine FDT to file %s", filename);
 }
 
+static bool fdt_prop_is_string_array(const void *data, int size)
+{
+    const char *str_arr, *str;
+    int i, str_len;
+
+    str_arr = str = data;
+
+    if (size <= 0 || str_arr[size - 1] != '\0') {
+        return false;
+    }
+
+    while (str < str_arr + size) {
+        str_len = strlen(str);
+
+        /*
+         * Do not consider empty strings (consecutives \0\0)
+         * as valid.
+         */
+        if (str_len == 0) {
+            return false;
+        }
+
+        for (i = 0; i < str_len; i++) {
+            if (!g_ascii_isprint(str[i])) {
+                return false;
+            }
+        }
+
+        str += str_len + 1;
+    }
+
+    return true;
+}
+
+static void fdt_prop_format_string_array(GString *buf,
+                                         const char *propname,
+                                         const char *data,
+                                         int prop_size, int padding)
+{
+    const char *str = data;
+
+    g_string_append_printf(buf, "%*s%s = ", padding, "", propname);
+
+    while (str < data + prop_size) {
+        /* appends up to the next '\0' */
+        g_string_append_printf(buf, "\"%s\"", str);
+
+        str += strlen(str) + 1;
+        if (str < data + prop_size) {
+            /* add a comma separator for the next string */
+            g_string_append_printf(buf, ", ");
+        }
+    }
+
+    g_string_append_printf(buf, ";\n");
+}
+
 static void fdt_format_node(GString *buf, int node, int depth)
 {
     const struct fdt_property *prop = NULL;
@@ -681,7 +738,12 @@ static void fdt_format_node(GString *buf, int node, int depth)
         prop = fdt_get_property_by_offset(fdt, property, &prop_size);
         propname = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
 
-        g_string_append_printf(buf, "%*s%s;\n", padding, "", propname);
+        if (fdt_prop_is_string_array(prop->data, prop_size)) {
+            fdt_prop_format_string_array(buf, propname, prop->data,
+                                         prop_size, padding);
+        } else {
+            g_string_append_printf(buf, "%*s%s;\n", padding, "", propname);
+        }
     }
 
     padding -= 4;
-- 
2.37.2

[PATCH for-7.2 v4 17/21] 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 = [00 00];
    rng-seed = <0x9cf5071b 0xf8804213 0xbe797764 0xad3d955 0xe0c9637 0x1f99c61e 0xe9243741 0xe800f17d>;
    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 | 57 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 56 insertions(+), 1 deletion(-)

diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
index 3e38d9ddad..a770c6c1cc 100644
--- a/softmmu/device_tree.c
+++ b/softmmu/device_tree.c
@@ -720,6 +720,52 @@ static void fdt_prop_format_string_array(GString *buf,
     g_string_append_printf(buf, ";\n");
 }
 
+static bool fdt_prop_is_uint32_array(int size)
+{
+    return size % 4 == 0;
+}
+
+static void fdt_prop_format_uint32_array(GString *buf,
+                                         const char *propname,
+                                         const void *data,
+                                         int prop_size, int padding)
+{
+    const fdt32_t *array = data;
+    int array_len = prop_size / 4;
+    int i;
+
+    g_string_append_printf(buf, "%*s%s = <", padding, "", propname);
+
+    for (i = 0; i < array_len; i++) {
+        g_string_append_printf(buf, "0x%" PRIx32, fdt32_to_cpu(array[i]));
+
+        if (i < array_len - 1) {
+            g_string_append_printf(buf, " ");
+        }
+    }
+
+    g_string_append_printf(buf, ">;\n");
+}
+
+static void fdt_prop_format_val(GString *buf, const char *propname,
+                                const void *data, int prop_size,
+                                int padding)
+{
+    const char *val = data;
+    int i;
+
+    g_string_append_printf(buf, "%*s%s = [", padding, "", propname);
+
+    for (i = 0; i < prop_size; i++) {
+        g_string_append_printf(buf, "%02x", val[i]);
+        if (i < prop_size - 1) {
+            g_string_append_printf(buf, " ");
+        }
+    }
+
+    g_string_append_printf(buf, "];\n");
+}
+
 static void fdt_format_node(GString *buf, int node, int depth)
 {
     const struct fdt_property *prop = NULL;
@@ -738,11 +784,20 @@ static void fdt_format_node(GString *buf, 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) {
+            g_string_append_printf(buf, "%*s%s;\n", padding, "", propname);
+            continue;
+        }
+
         if (fdt_prop_is_string_array(prop->data, prop_size)) {
             fdt_prop_format_string_array(buf, propname, prop->data,
                                          prop_size, padding);
+        } else if (fdt_prop_is_uint32_array(prop_size)) {
+            fdt_prop_format_uint32_array(buf, propname, prop->data, prop_size,
+                                         padding);
         } else {
-            g_string_append_printf(buf, "%*s%s;\n", padding, "", propname);
+            fdt_prop_format_val(buf, propname, prop->data,
+                                prop_size, padding);
         }
     }
 
-- 
2.37.2

[PATCH for-7.2 v4 18/21] 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 | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
index a770c6c1cc..5e4cb119f2 100644
--- a/softmmu/device_tree.c
+++ b/softmmu/device_tree.c
@@ -766,17 +766,26 @@ static void fdt_prop_format_val(GString *buf, const char *propname,
     g_string_append_printf(buf, "];\n");
 }
 
-static void fdt_format_node(GString *buf, int node, int depth)
+
+static void fdt_format_node(GString *buf, 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;
 
-    g_string_append_printf(buf, "%*s%s {\n", padding, "",
-                           fdt_get_name(fdt, node, NULL));
+    if (fullpath != NULL) {
+        nodename = fullpath;
+    } else {
+        nodename = fdt_get_name(fdt, node, NULL);
+    }
+
+    g_string_append_printf(buf, "%*s%s {\n", padding, "", nodename);
 
     padding += 4;
 
@@ -801,6 +810,10 @@ static void fdt_format_node(GString *buf, int node, int depth)
         }
     }
 
+    fdt_for_each_subnode(node, fdt, parent) {
+        fdt_format_node(buf, node, depth + 1, NULL);
+    }
+
     padding -= 4;
     g_string_append_printf(buf, "%*s};\n", padding, "");
 }
@@ -821,7 +834,7 @@ HumanReadableText *qemu_fdt_qmp_query_fdt(const char *nodepath, Error **errp)
         return NULL;
     }
 
-    fdt_format_node(buf, node, 0);
+    fdt_format_node(buf, node, 0, nodepath);
 
     return human_readable_text_from_str(buf);
 }
-- 
2.37.2

[PATCH for-7.2 v4 19/21] device_tree.c: add fdt_format_property() helper

[Daniel Henrique Barboza]

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

Create a helper to format properties based on the already existing code
from fdt_format_node().

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

diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
index 5e4cb119f2..70a011495d 100644
--- a/softmmu/device_tree.c
+++ b/softmmu/device_tree.c
@@ -766,6 +766,25 @@ static void fdt_prop_format_val(GString *buf, const char *propname,
     g_string_append_printf(buf, "];\n");
 }
 
+static void fdt_format_property(GString *buf, const char *propname,
+                                const void *data, int prop_size,
+                                int padding)
+{
+    if (prop_size == 0) {
+        g_string_append_printf(buf, "%*s%s;\n", padding, "", propname);
+        return;
+    }
+
+    if (fdt_prop_is_string_array(data, prop_size)) {
+        fdt_prop_format_string_array(buf, propname, data, prop_size,
+                                     padding);
+    } else if (fdt_prop_is_uint32_array(prop_size)) {
+        fdt_prop_format_uint32_array(buf, propname, data, prop_size,
+                                     padding);
+    } else {
+        fdt_prop_format_val(buf, propname, data, prop_size, padding);
+    }
+}
 
 static void fdt_format_node(GString *buf, int node, int depth,
                             const char *fullpath)
@@ -793,21 +812,7 @@ static void fdt_format_node(GString *buf, 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) {
-            g_string_append_printf(buf, "%*s%s;\n", padding, "", propname);
-            continue;
-        }
-
-        if (fdt_prop_is_string_array(prop->data, prop_size)) {
-            fdt_prop_format_string_array(buf, propname, prop->data,
-                                         prop_size, padding);
-        } else if (fdt_prop_is_uint32_array(prop_size)) {
-            fdt_prop_format_uint32_array(buf, propname, prop->data, prop_size,
-                                         padding);
-        } else {
-            fdt_prop_format_val(buf, propname, prop->data,
-                                prop_size, padding);
-        }
+        fdt_format_property(buf, propname, prop->data, prop_size, padding);
     }
 
     fdt_for_each_subnode(node, fdt, parent) {
-- 
2.37.2

[PATCH for-7.2 v4 20/21] 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.

Add a new optional 'propname' parameter to x-query-fdt to specify the
property of a given node. If it's present, we'll proceed to find the
node as usual but, instead of printing the node, we'll attempt to find
the property and print it standalone.

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>;

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>;

Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hmp-commands-info.hx         |  9 +++++----
 include/sysemu/device_tree.h |  2 ++
 monitor/hmp-cmds.c           |  5 ++++-
 monitor/qmp-cmds.c           |  8 +++++---
 qapi/machine.json            |  4 +++-
 softmmu/device_tree.c        | 29 ++++++++++++++++++++++++-----
 6 files changed, 43 insertions(+), 14 deletions(-)

diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index 743b48865d..17d6ee4d30 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -924,13 +924,14 @@ ERST
 
     {
         .name       = "fdt",
-        .args_type  = "nodepath:s",
-        .params     = "nodepath",
-        .help       = "show firmware device tree node given its path",
+        .args_type  = "nodepath:s,propname:s?",
+        .params     = "nodepath [propname]",
+        .help       = "show firmware device tree node or property given its path",
         .cmd        = hmp_info_fdt,
     },
 
 SRST
   ``info fdt``
-    Show a firmware device tree node given its path. Requires libfdt.
+    Show a firmware device tree node or property given its path.
+    Requires libfdt.
 ERST
diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
index 057d13e397..551a02dee2 100644
--- a/include/sysemu/device_tree.h
+++ b/include/sysemu/device_tree.h
@@ -140,6 +140,8 @@ int qemu_fdt_add_path(void *fdt, const char *path);
 void qemu_fdt_dumpdtb(void *fdt, int size);
 void qemu_fdt_qmp_dumpdtb(const char *filename, Error **errp);
 HumanReadableText *qemu_fdt_qmp_query_fdt(const char *nodepath,
+                                          bool has_propname,
+                                          const char *propname,
                                           Error **errp);
 
 /**
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 93a4103afa..320204e982 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -2488,8 +2488,11 @@ void hmp_dumpdtb(Monitor *mon, const QDict *qdict)
 void hmp_info_fdt(Monitor *mon, const QDict *qdict)
 {
     const char *nodepath = qdict_get_str(qdict, "nodepath");
+    const char *propname = qdict_get_try_str(qdict, "propname");
     Error *err = NULL;
-    g_autoptr(HumanReadableText) info = qmp_x_query_fdt(nodepath, &err);
+    g_autoptr(HumanReadableText) info = NULL;
+
+    info = qmp_x_query_fdt(nodepath, propname != NULL, propname, &err);
 
     if (hmp_handle_error(mon, err)) {
         return;
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index db2c6aa7da..ca2a96cdf7 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -604,9 +604,10 @@ void qmp_dumpdtb(const char *filename, Error **errp)
     return qemu_fdt_qmp_dumpdtb(filename, errp);
 }
 
-HumanReadableText *qmp_x_query_fdt(const char *nodepath, Error **errp)
+HumanReadableText *qmp_x_query_fdt(const char *nodepath, bool has_propname,
+                                   const char *propname, Error **errp)
 {
-    return qemu_fdt_qmp_query_fdt(nodepath, errp);
+    return qemu_fdt_qmp_query_fdt(nodepath, has_propname, propname, errp);
 }
 #else
 void qmp_dumpdtb(const char *filename, Error **errp)
@@ -614,7 +615,8 @@ void qmp_dumpdtb(const char *filename, Error **errp)
     error_setg(errp, "dumpdtb requires libfdt");
 }
 
-HumanReadableText *qmp_x_query_fdt(const char *nodepath, Error **errp)
+HumanReadableText *qmp_x_query_fdt(const char *nodepath, bool has_propname,
+                                   const char *propname, Error **errp)
 {
     error_setg(errp, "this command requires libfdt");
 
diff --git a/qapi/machine.json b/qapi/machine.json
index 96cff541ca..c15ce60f46 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1688,6 +1688,7 @@
 # Query for FDT element (node or property). Requires 'libfdt'.
 #
 # @nodepath: the path of the FDT node to be retrieved
+# @propname: name of the property inside the node
 #
 # Features:
 # @unstable: This command is meant for debugging.
@@ -1697,6 +1698,7 @@
 # Since: 7.2
 ##
 { 'command': 'x-query-fdt',
-  'data': { 'nodepath': 'str' },
+  'data': { 'nodepath': 'str',
+            '*propname': 'str' },
   'returns': 'HumanReadableText',
   'features': [ 'unstable' ]  }
diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
index 70a011495d..ad2386295b 100644
--- a/softmmu/device_tree.c
+++ b/softmmu/device_tree.c
@@ -823,23 +823,42 @@ static void fdt_format_node(GString *buf, int node, int depth,
     g_string_append_printf(buf, "%*s};\n", padding, "");
 }
 
-HumanReadableText *qemu_fdt_qmp_query_fdt(const char *nodepath, Error **errp)
+HumanReadableText *qemu_fdt_qmp_query_fdt(const char *nodepath,
+                                          bool has_propname,
+                                          const char *propname,
+                                          Error **errp)
 {
     g_autoptr(GString) buf = g_string_new("");
-    int node;
+    const struct fdt_property *prop = NULL;
+    void *fdt = current_machine->fdt;
+    int node, prop_size;
 
-    if (!current_machine->fdt) {
+    if (!fdt) {
         error_setg(errp, "Unable to find the machine FDT");
         return NULL;
     }
 
-    node = fdt_path_offset(current_machine->fdt, nodepath);
+    node = fdt_path_offset(fdt, nodepath);
     if (node < 0) {
         error_setg(errp, "node '%s' not found in FDT", nodepath);
         return NULL;
     }
 
-    fdt_format_node(buf, node, 0, nodepath);
+    if (!has_propname) {
+        fdt_format_node(buf, node, 0, nodepath);
+    } else {
+        g_autofree char *proppath = g_strdup_printf("%s/%s", nodepath,
+                                                    propname);
+
+        prop = fdt_get_property(fdt, node, propname, &prop_size);
+        if (!prop) {
+            error_setg(errp, "property '%s' not found in node '%s' in FDT",
+                       propname, nodepath);
+            return NULL;
+        }
+
+        fdt_format_property(buf, proppath, prop->data, prop_size, 0);
+    }
 
     return human_readable_text_from_str(buf);
 }
-- 
2.37.2

[PATCH for-7.2 v4 21/21] qmp/hmp, device_tree.c: add textformat dumpdtb option

[Daniel Henrique Barboza]

The QMP/HMP 'dumpdtb' command is saving the FDT blob to be decoded using
'dtc' like the '-machine dumpdtb' always did. However, after adding
support for the 'info fdt' command, we're now able to format a full FDT
in text format - which is what 'info fdt /' already does.

Let's extend the 'dumpdtb' QMP/HMP command with the capability of saving
the FDT in plain text format. A new textformat '-t' option was added to
it. With this option, qemu_fdt_qmp_dumpdtb() will call
qemu_fdt_qmp_query_fdt() to retrieve the human string that represents
the output of 'info fdt /' and write it to the file.

This will allow users to dump the FDT in a text file and immediately
open it to see its contents, without the need of an extra step to decode
a dtb blob with 'dtc'. Here's an example:

(qemu) dumpdtb fdt.dtb

$ file fdt.dtb
fdt.dtb: Device Tree Blob version 17, size=15458, boot CPU=0, string block
size=2318, DT structure block size=13084

(qemu) dumpdtb -t fdt.txt

$ file fdt.txt
fdt.txt: ASCII text, with very long lines (4746)

$ grep -A 3 'persistent-memory' fdt.txt
    ibm,persistent-memory {
        device_type = "ibm,persistent-memory";
        #size-cells = <0x0>;
        #address-cells = <0x1>;
    };

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hmp-commands.hx              |  7 ++++---
 include/sysemu/device_tree.h |  3 ++-
 monitor/hmp-cmds.c           |  3 ++-
 monitor/qmp-cmds.c           |  8 +++++---
 qapi/machine.json            |  2 +-
 softmmu/device_tree.c        | 25 ++++++++++++++++++++++---
 6 files changed, 36 insertions(+), 12 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 2dd737078e..8a9595cc26 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1808,8 +1808,9 @@ SRST
 ERST
     {
         .name       = "dumpdtb",
-        .args_type  = "filename:F",
-        .params     = "filename",
-        .help       = "save the FDT in the 'filename' file to be decoded using dtc",
+        .args_type  = "textformat:-t,filename:F",
+        .params     = "[-t] filename",
+        .help       = "save the FDT in the 'filename' file to be decoded using dtc."
+                       "Use '-t' to save the file in text (dts) format.",
         .cmd        = hmp_dumpdtb,
     },
diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
index 551a02dee2..082ff69751 100644
--- a/include/sysemu/device_tree.h
+++ b/include/sysemu/device_tree.h
@@ -138,7 +138,8 @@ int qemu_fdt_add_path(void *fdt, const char *path);
     } while (0)
 
 void qemu_fdt_dumpdtb(void *fdt, int size);
-void qemu_fdt_qmp_dumpdtb(const char *filename, Error **errp);
+void qemu_fdt_qmp_dumpdtb(const char *filename, bool textformat,
+                          Error **errp);
 HumanReadableText *qemu_fdt_qmp_query_fdt(const char *nodepath,
                                           bool has_propname,
                                           const char *propname,
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 320204e982..ffcb9ffb67 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -2476,9 +2476,10 @@ exit_no_print:
 void hmp_dumpdtb(Monitor *mon, const QDict *qdict)
 {
     const char *filename = qdict_get_str(qdict, "filename");
+    bool textformat = qdict_get_try_bool(qdict, "textformat", false);
     Error *local_err = NULL;
 
-    qmp_dumpdtb(filename, &local_err);
+    qmp_dumpdtb(true, textformat, filename, &local_err);
 
     if (local_err) {
         hmp_handle_error(mon, local_err);
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index ca2a96cdf7..8d625e5e7d 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -599,9 +599,10 @@ bool apply_str_list_filter(const char *string, strList *list)
 }
 
 #ifdef CONFIG_FDT
-void qmp_dumpdtb(const char *filename, Error **errp)
+void qmp_dumpdtb(bool has_textformat, bool textformat,
+                 const char *filename, Error **errp)
 {
-    return qemu_fdt_qmp_dumpdtb(filename, errp);
+    return qemu_fdt_qmp_dumpdtb(filename, textformat, errp);
 }
 
 HumanReadableText *qmp_x_query_fdt(const char *nodepath, bool has_propname,
@@ -610,7 +611,8 @@ HumanReadableText *qmp_x_query_fdt(const char *nodepath, bool has_propname,
     return qemu_fdt_qmp_query_fdt(nodepath, has_propname, propname, errp);
 }
 #else
-void qmp_dumpdtb(const char *filename, Error **errp)
+void qmp_dumpdtb(bool has_textformat, bool textformat,
+                 const char *filename, Error **errp)
 {
     error_setg(errp, "dumpdtb requires libfdt");
 }
diff --git a/qapi/machine.json b/qapi/machine.json
index c15ce60f46..8573f96da8 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1680,7 +1680,7 @@
 #
 ##
 { 'command': 'dumpdtb',
-  'data': { 'filename': 'str' } }
+  'data': { '*textformat':'bool', 'filename': 'str' } }
 
 ##
 # @x-query-fdt:
diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
index ad2386295b..34af31552d 100644
--- a/softmmu/device_tree.c
+++ b/softmmu/device_tree.c
@@ -645,8 +645,10 @@ out:
     return ret;
 }
 
-void qemu_fdt_qmp_dumpdtb(const char *filename, Error **errp)
+void qemu_fdt_qmp_dumpdtb(const char *filename, bool textformat, Error **errp)
 {
+    g_autoptr(HumanReadableText) txt = NULL;
+    void *contents = NULL;
     int size;
 
     if (!current_machine->fdt) {
@@ -654,9 +656,26 @@ void qemu_fdt_qmp_dumpdtb(const char *filename, Error **errp)
         return;
     }
 
-    size = fdt_totalsize(current_machine->fdt);
+    if (textformat) {
+        /*
+         * 'info fdt /' returns all the FDT in text format, formatted
+         * with a style close to what 'dtc' uses to decode the blob
+         * to a .dts.
+         */
+        txt = qemu_fdt_qmp_query_fdt("/", false, NULL, errp);
+
+        if (!txt) {
+            return;
+        }
+
+        contents = txt->human_readable_text;
+        size = strlen(txt->human_readable_text);
+    } else {
+        contents = current_machine->fdt;
+        size = fdt_totalsize(current_machine->fdt);
+    }
 
-    if (g_file_set_contents(filename, current_machine->fdt, size, NULL)) {
+    if (g_file_set_contents(filename, contents, size, NULL)) {
         return;
     }
 
-- 
2.37.2

Re: [PATCH for-7.2 v4 06/21] hw/ppc: set machine->fdt in sam460ex_load_device_tree()

[BALATON Zoltan]

On Fri, 26 Aug 2022, Daniel Henrique Barboza wrote:
> This will enable support for 'dumpdtb' and 'info fdt' HMP commands for
> the sam460ex machine.

This only works when booting with -kernel not when the firmware is used 
which creates its own DT. (The same is true for pegasos2 but there VOF is 
the default as the firmware is not free like for sam460ex.) After reading 
the other comments I wonder if this info fdt command is really useful or 
would it be easier to boot some simple Linux guest and inspect the device 
tree from there. The dumpdtb command might be simple enough and a bit more 
useful for debugging before the guest boots but that alone could be enough 
as external tools can be used to decode the binary dump. The info fdt 
might be too complex and an overkill if it might not even work or give 
correct results. But I don't mind either way and not against adding it 
just noted the possible shortcoming here.

(In case you do another iteration I wouldn't mind if the comment could be 
shortened to one line instead of 4 but it's not critical. Something like:

/* Set machine->fdt for dumpdtb and info fdt QMP/HMP commands */

would be enough and use less space. The current one is unnecessarily 
verbose for a simple line.)

Regards,
BALATON Zoltan

> Cc: BALATON Zoltan <balaton@eik.bme.hu>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
> hw/ppc/sam460ex.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
> index 0357ee077f..413a425d37 100644
> --- a/hw/ppc/sam460ex.c
> +++ b/hw/ppc/sam460ex.c
> @@ -138,6 +138,7 @@ static int sam460ex_load_device_tree(hwaddr addr,
>                                      hwaddr initrd_size,
>                                      const char *kernel_cmdline)
> {
> +    MachineState *machine = MACHINE(qdev_get_machine());
>     uint32_t mem_reg_property[] = { 0, 0, cpu_to_be32(ramsize) };
>     char *filename;
>     int fdt_size;
> @@ -209,7 +210,12 @@ static int sam460ex_load_device_tree(hwaddr addr,
>                               EBC_FREQ);
>
>     rom_add_blob_fixed(BINARY_DEVICE_TREE_FILE, fdt, fdt_size, addr);
> -    g_free(fdt);
> +
> +    /*
> +     * Update the machine->fdt pointer to enable support for
> +     * 'dumpdtb' and 'info fdt' QMP/HMP commands.
> +     */
> +    machine->fdt = fdt;
>
>     return fdt_size;
> }
>

Re: [PATCH for-7.2 v4 06/21] hw/ppc: set machine->fdt in sam460ex_load_device_tree()

[Daniel Henrique Barboza]

On 8/26/22 15:56, BALATON Zoltan wrote:
> On Fri, 26 Aug 2022, Daniel Henrique Barboza wrote:
>> This will enable support for 'dumpdtb' and 'info fdt' HMP commands for
>> the sam460ex machine.
> 
> This only works when booting with -kernel not when the firmware is used which creates its own DT. (The same is true for pegasos2 but there VOF is the default as the firmware is not free like for sam460ex.) After reading the other comments I wonder if this info fdt command is really useful or would it be easier to boot some simple Linux guest and inspect the device tree from there. The dumpdtb command might be simple enough and a bit more useful for debugging before the guest boots but that alone could be enough as external tools can be used to decode the binary dump. The info fdt might be too complex and an overkill if it might not even work or give correct results. But I don't mind either way and not against adding it just noted the possible shortcoming here.

As an counter example of what you said, there's no guarantee that the FDT is
fully exposed in the guest sysfs. This is the case of the /chosen/rng-seed
property that was added recently in the pSeries machine - the property exists
in the FDT, the guest is aware of it, but the guest doesn't show it in under
/proc/device-tree. In fact, I think I mentioned in the cover letter of the first
version that this situation was the motivation of this work.

But you're right when you questioned how useful these QMP/HMP commands might be
for sam460ex. The utility of both commands relies on how the machine handles the
FDT internally and what you're trying to check.


> 
> (In case you do another iteration I wouldn't mind if the comment could be shortened to one line instead of 4 but it's not critical. Something like:
> 
> /* Set machine->fdt for dumpdtb and info fdt QMP/HMP commands */


Changed in my local tree.


Thanks,

Daniel

> 
> would be enough and use less space. The current one is unnecessarily verbose for a simple line.)
> 
> Regards,
> BALATON Zoltan
> 
>> Cc: BALATON Zoltan <balaton@eik.bme.hu>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>> hw/ppc/sam460ex.c | 8 +++++++-
>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
>> index 0357ee077f..413a425d37 100644
>> --- a/hw/ppc/sam460ex.c
>> +++ b/hw/ppc/sam460ex.c
>> @@ -138,6 +138,7 @@ static int sam460ex_load_device_tree(hwaddr addr,
>>                                      hwaddr initrd_size,
>>                                      const char *kernel_cmdline)
>> {
>> +    MachineState *machine = MACHINE(qdev_get_machine());
>>     uint32_t mem_reg_property[] = { 0, 0, cpu_to_be32(ramsize) };
>>     char *filename;
>>     int fdt_size;
>> @@ -209,7 +210,12 @@ static int sam460ex_load_device_tree(hwaddr addr,
>>                               EBC_FREQ);
>>
>>     rom_add_blob_fixed(BINARY_DEVICE_TREE_FILE, fdt, fdt_size, addr);
>> -    g_free(fdt);
>> +
>> +    /*
>> +     * Update the machine->fdt pointer to enable support for
>> +     * 'dumpdtb' and 'info fdt' QMP/HMP commands.
>> +     */
>> +    machine->fdt = fdt;
>>
>>     return fdt_size;
>> }
>>

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3448 bytes --]

On Fri, Aug 26, 2022 at 11:11:39AM -0300, Daniel Henrique Barboza wrote:
> 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 two places where we manipulate the FDT:
> spapr_machine_reset() and CAS. There are other places where the FDT is
> manipulated in the pSeries machines, most notably the hotplug/unplug
> path. For now we'll acknowledge that we won't have the most accurate
> representation of the FDT, depending on the current machine state, when
> using these QMP/HMP fdt commands. Making the internal FDT representation
> always match the actual FDT representation that the guest is using is a
> problem for another day.
> 
> spapr->fdt_blob is left untouched for now. To replace it with
> machine->fdt, since we're migrating spapr->fdt_blob, we would need to
> migrate machine->fdt as well. This is something that we would like to to
> do keep our code simpler but it's also a work we'll leave for later.

As discussed elswhere, this doesn't give a full picture of the
"runtime" device tree, which can get modified later.  For now, I think
that's ok - we can define the fdt property / dumpdtb etc. as
describing specifically the boot time DT before guest firmware or OS
does any further mangling of it.  That's effectively what it means for
all the other embedded cases, though in those cases the firmware
usually doesn't need to do further modifications, unlike a "full OF"
environment like spapr.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> 
> 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       | 6 ++++++
>  hw/ppc/spapr_hcall.c | 8 ++++++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index bc9ba6e6dc..7031cf964a 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1713,6 +1713,12 @@ static void spapr_machine_reset(MachineState *machine)
>      spapr->fdt_initial_size = spapr->fdt_size;
>      spapr->fdt_blob = fdt;
>  
> +    /*
> +     * Set the common machine->fdt pointer to enable support
> +     * for 'dumpdtb' and 'info fdt' QMP/HMP commands.
> +     */
> +    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..a53bfd76f4 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1256,6 +1256,14 @@ target_ulong do_client_architecture_support(PowerPCCPU *cpu,
>      spapr->fdt_initial_size = spapr->fdt_size;
>      spapr->fdt_blob = fdt;
>  
> +    /*
> +     * Set the machine->fdt pointer again since we just freed
> +     * it above (by freeing spapr->fdt_blob). We set this
> +     * pointer to enable support for 'dumpdtb' and 'info fdt'
> +     * QMP/HMP commands.
> +     */
> +    MACHINE(spapr)->fdt = fdt;
> +
>      return H_SUCCESS;
>  }
>  

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH for-7.2 v4 15/21] qmp/hmp, device_tree.c: introduce 'info fdt' command

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 9327 bytes --]

On Fri, Aug 26, 2022 at 11:11:44AM -0300, Daniel Henrique Barboza wrote:
> 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 node or property.
> 
> '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 QMP/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,
> and then individual properties.
> 
> 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).
> 
> 'info fdt' is not something that we expect to be used aside from debugging,
> so we're implementing it in QMP as 'x-query-fdt'.
> 
> 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;
> };

So, I'm reasonably convinced allowing dumping the whole dtb from
qmp/hmp is useful.  I'm less convined that info fdt is worth the
additional complexity it incurs.  Note that as well as being able to
decompile a whole dtb using dtc, you can also extract and list
specific properties from a dtb blob using the 'fdtget' tool which is
part of the dtc tree.

> 
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  hmp-commands-info.hx         | 13 ++++++++++
>  include/monitor/hmp.h        |  1 +
>  include/sysemu/device_tree.h |  4 +++
>  monitor/hmp-cmds.c           | 13 ++++++++++
>  monitor/qmp-cmds.c           | 12 +++++++++
>  qapi/machine.json            | 19 +++++++++++++++
>  softmmu/device_tree.c        | 47 ++++++++++++++++++++++++++++++++++++
>  7 files changed, 109 insertions(+)
> 
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index 188d9ece3b..743b48865d 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -921,3 +921,16 @@ SRST
>    ``stats``
>      Show runtime-collected statistics
>  ERST
> +
> +    {
> +        .name       = "fdt",
> +        .args_type  = "nodepath:s",
> +        .params     = "nodepath",
> +        .help       = "show firmware device tree node given its path",
> +        .cmd        = hmp_info_fdt,
> +    },
> +
> +SRST
> +  ``info fdt``
> +    Show a firmware device tree node given its path. Requires libfdt.
> +ERST
> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
> index d7f324da59..c0883dd1e3 100644
> --- a/include/monitor/hmp.h
> +++ b/include/monitor/hmp.h
> @@ -135,6 +135,7 @@ void hmp_set_vcpu_dirty_limit(Monitor *mon, const QDict *qdict);
>  void hmp_cancel_vcpu_dirty_limit(Monitor *mon, const QDict *qdict);
>  void hmp_info_vcpu_dirty_limit(Monitor *mon, const QDict *qdict);
>  void hmp_dumpdtb(Monitor *mon, const QDict *qdict);
> +void hmp_info_fdt(Monitor *mon, const QDict *qdict);
>  void hmp_human_readable_text_helper(Monitor *mon,
>                                      HumanReadableText *(*qmp_handler)(Error **));
>  void hmp_info_stats(Monitor *mon, const QDict *qdict);
> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
> index bf7684e4ed..057d13e397 100644
> --- a/include/sysemu/device_tree.h
> +++ b/include/sysemu/device_tree.h
> @@ -14,6 +14,8 @@
>  #ifndef DEVICE_TREE_H
>  #define DEVICE_TREE_H
>  
> +#include "qapi/qapi-types-common.h"
> +
>  void *create_device_tree(int *sizep);
>  void *load_device_tree(const char *filename_path, int *sizep);
>  #ifdef CONFIG_LINUX
> @@ -137,6 +139,8 @@ int qemu_fdt_add_path(void *fdt, const char *path);
>  
>  void qemu_fdt_dumpdtb(void *fdt, int size);
>  void qemu_fdt_qmp_dumpdtb(const char *filename, Error **errp);
> +HumanReadableText *qemu_fdt_qmp_query_fdt(const char *nodepath,
> +                                          Error **errp);
>  
>  /**
>   * qemu_fdt_setprop_sized_cells_from_array:
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 1c7bfd3b9d..93a4103afa 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -2484,3 +2484,16 @@ void hmp_dumpdtb(Monitor *mon, const QDict *qdict)
>          hmp_handle_error(mon, local_err);
>      }
>  }
> +
> +void hmp_info_fdt(Monitor *mon, const QDict *qdict)
> +{
> +    const char *nodepath = qdict_get_str(qdict, "nodepath");
> +    Error *err = NULL;
> +    g_autoptr(HumanReadableText) info = qmp_x_query_fdt(nodepath, &err);
> +
> +    if (hmp_handle_error(mon, err)) {
> +        return;
> +    }
> +
> +    monitor_printf(mon, "%s", info->human_readable_text);
> +}
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index 8415aca08c..db2c6aa7da 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -603,9 +603,21 @@ void qmp_dumpdtb(const char *filename, Error **errp)
>  {
>      return qemu_fdt_qmp_dumpdtb(filename, errp);
>  }
> +
> +HumanReadableText *qmp_x_query_fdt(const char *nodepath, Error **errp)
> +{
> +    return qemu_fdt_qmp_query_fdt(nodepath, errp);
> +}
>  #else
>  void qmp_dumpdtb(const char *filename, Error **errp)
>  {
>      error_setg(errp, "dumpdtb requires libfdt");
>  }
> +
> +HumanReadableText *qmp_x_query_fdt(const char *nodepath, Error **errp)
> +{
> +    error_setg(errp, "this command requires libfdt");
> +
> +    return NULL;
> +}
>  #endif
> diff --git a/qapi/machine.json b/qapi/machine.json
> index aeb013f3dd..96cff541ca 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1681,3 +1681,22 @@
>  ##
>  { 'command': 'dumpdtb',
>    'data': { 'filename': 'str' } }
> +
> +##
> +# @x-query-fdt:
> +#
> +# Query for FDT element (node or property). Requires 'libfdt'.
> +#
> +# @nodepath: the path of the FDT node to be retrieved
> +#
> +# Features:
> +# @unstable: This command is meant for debugging.
> +#
> +# Returns: FDT node
> +#
> +# Since: 7.2
> +##
> +{ 'command': 'x-query-fdt',
> +  'data': { 'nodepath': 'str' },
> +  'returns': 'HumanReadableText',
> +  'features': [ 'unstable' ]  }
> diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
> index cd487ddd4d..6b15f6ace2 100644
> --- a/softmmu/device_tree.c
> +++ b/softmmu/device_tree.c
> @@ -18,6 +18,7 @@
>  #endif
>  
>  #include "qapi/error.h"
> +#include "qapi/type-helpers.h"
>  #include "qemu/error-report.h"
>  #include "qemu/option.h"
>  #include "qemu/bswap.h"
> @@ -661,3 +662,49 @@ void qemu_fdt_qmp_dumpdtb(const char *filename, Error **errp)
>  
>      error_setg(errp, "Error when saving machine FDT to file %s", filename);
>  }
> +
> +static void fdt_format_node(GString *buf, 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;
> +
> +    g_string_append_printf(buf, "%*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));
> +
> +        g_string_append_printf(buf, "%*s%s;\n", padding, "", propname);
> +    }
> +
> +    padding -= 4;
> +    g_string_append_printf(buf, "%*s};\n", padding, "");
> +}
> +
> +HumanReadableText *qemu_fdt_qmp_query_fdt(const char *nodepath, Error **errp)
> +{
> +    g_autoptr(GString) buf = g_string_new("");
> +    int node;
> +
> +    if (!current_machine->fdt) {
> +        error_setg(errp, "Unable to find the machine FDT");
> +        return NULL;
> +    }
> +
> +    node = fdt_path_offset(current_machine->fdt, nodepath);
> +    if (node < 0) {
> +        error_setg(errp, "node '%s' not found in FDT", nodepath);
> +        return NULL;
> +    }
> +
> +    fdt_format_node(buf, node, 0);
> +
> +    return human_readable_text_from_str(buf);
> +}

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH for-7.2 v4 15/21] qmp/hmp, device_tree.c: introduce 'info fdt' command

[Daniel Henrique Barboza]

On 8/29/22 00:34, David Gibson wrote:
> On Fri, Aug 26, 2022 at 11:11:44AM -0300, Daniel Henrique Barboza wrote:
>> 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 node or property.
>>
>> '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 QMP/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,
>> and then individual properties.
>>
>> 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).
>>
>> 'info fdt' is not something that we expect to be used aside from debugging,
>> so we're implementing it in QMP as 'x-query-fdt'.
>>
>> 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;
>> };
> 
> So, I'm reasonably convinced allowing dumping the whole dtb from
> qmp/hmp is useful.  I'm less convined that info fdt is worth the
> additional complexity it incurs.  Note that as well as being able to
> decompile a whole dtb using dtc, you can also extract and list
> specific properties from a dtb blob using the 'fdtget' tool which is
> part of the dtc tree.

What's your opinion on patch 21/21, where 'dumpdtb' can write a formatted
FDT in a file with an extra option? That was possible because of the
format helpers introduced for 'info fdt'. The idea is that since we're
able to format a FDT in DTS format, we can also write the FDT in text
format without relying on DTC to decode it.

If we think that this 'dumpdtb' capability is worth having, I can respin
the patches without 'info fdt' but adding these helpers to enable this
'dumpdtb' support. If not, then we can just remove patches 15-21 and
be done with it.


Thanks,


Daniel

> 
>>
>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   hmp-commands-info.hx         | 13 ++++++++++
>>   include/monitor/hmp.h        |  1 +
>>   include/sysemu/device_tree.h |  4 +++
>>   monitor/hmp-cmds.c           | 13 ++++++++++
>>   monitor/qmp-cmds.c           | 12 +++++++++
>>   qapi/machine.json            | 19 +++++++++++++++
>>   softmmu/device_tree.c        | 47 ++++++++++++++++++++++++++++++++++++
>>   7 files changed, 109 insertions(+)
>>
>> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
>> index 188d9ece3b..743b48865d 100644
>> --- a/hmp-commands-info.hx
>> +++ b/hmp-commands-info.hx
>> @@ -921,3 +921,16 @@ SRST
>>     ``stats``
>>       Show runtime-collected statistics
>>   ERST
>> +
>> +    {
>> +        .name       = "fdt",
>> +        .args_type  = "nodepath:s",
>> +        .params     = "nodepath",
>> +        .help       = "show firmware device tree node given its path",
>> +        .cmd        = hmp_info_fdt,
>> +    },
>> +
>> +SRST
>> +  ``info fdt``
>> +    Show a firmware device tree node given its path. Requires libfdt.
>> +ERST
>> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
>> index d7f324da59..c0883dd1e3 100644
>> --- a/include/monitor/hmp.h
>> +++ b/include/monitor/hmp.h
>> @@ -135,6 +135,7 @@ void hmp_set_vcpu_dirty_limit(Monitor *mon, const QDict *qdict);
>>   void hmp_cancel_vcpu_dirty_limit(Monitor *mon, const QDict *qdict);
>>   void hmp_info_vcpu_dirty_limit(Monitor *mon, const QDict *qdict);
>>   void hmp_dumpdtb(Monitor *mon, const QDict *qdict);
>> +void hmp_info_fdt(Monitor *mon, const QDict *qdict);
>>   void hmp_human_readable_text_helper(Monitor *mon,
>>                                       HumanReadableText *(*qmp_handler)(Error **));
>>   void hmp_info_stats(Monitor *mon, const QDict *qdict);
>> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
>> index bf7684e4ed..057d13e397 100644
>> --- a/include/sysemu/device_tree.h
>> +++ b/include/sysemu/device_tree.h
>> @@ -14,6 +14,8 @@
>>   #ifndef DEVICE_TREE_H
>>   #define DEVICE_TREE_H
>>   
>> +#include "qapi/qapi-types-common.h"
>> +
>>   void *create_device_tree(int *sizep);
>>   void *load_device_tree(const char *filename_path, int *sizep);
>>   #ifdef CONFIG_LINUX
>> @@ -137,6 +139,8 @@ int qemu_fdt_add_path(void *fdt, const char *path);
>>   
>>   void qemu_fdt_dumpdtb(void *fdt, int size);
>>   void qemu_fdt_qmp_dumpdtb(const char *filename, Error **errp);
>> +HumanReadableText *qemu_fdt_qmp_query_fdt(const char *nodepath,
>> +                                          Error **errp);
>>   
>>   /**
>>    * qemu_fdt_setprop_sized_cells_from_array:
>> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
>> index 1c7bfd3b9d..93a4103afa 100644
>> --- a/monitor/hmp-cmds.c
>> +++ b/monitor/hmp-cmds.c
>> @@ -2484,3 +2484,16 @@ void hmp_dumpdtb(Monitor *mon, const QDict *qdict)
>>           hmp_handle_error(mon, local_err);
>>       }
>>   }
>> +
>> +void hmp_info_fdt(Monitor *mon, const QDict *qdict)
>> +{
>> +    const char *nodepath = qdict_get_str(qdict, "nodepath");
>> +    Error *err = NULL;
>> +    g_autoptr(HumanReadableText) info = qmp_x_query_fdt(nodepath, &err);
>> +
>> +    if (hmp_handle_error(mon, err)) {
>> +        return;
>> +    }
>> +
>> +    monitor_printf(mon, "%s", info->human_readable_text);
>> +}
>> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
>> index 8415aca08c..db2c6aa7da 100644
>> --- a/monitor/qmp-cmds.c
>> +++ b/monitor/qmp-cmds.c
>> @@ -603,9 +603,21 @@ void qmp_dumpdtb(const char *filename, Error **errp)
>>   {
>>       return qemu_fdt_qmp_dumpdtb(filename, errp);
>>   }
>> +
>> +HumanReadableText *qmp_x_query_fdt(const char *nodepath, Error **errp)
>> +{
>> +    return qemu_fdt_qmp_query_fdt(nodepath, errp);
>> +}
>>   #else
>>   void qmp_dumpdtb(const char *filename, Error **errp)
>>   {
>>       error_setg(errp, "dumpdtb requires libfdt");
>>   }
>> +
>> +HumanReadableText *qmp_x_query_fdt(const char *nodepath, Error **errp)
>> +{
>> +    error_setg(errp, "this command requires libfdt");
>> +
>> +    return NULL;
>> +}
>>   #endif
>> diff --git a/qapi/machine.json b/qapi/machine.json
>> index aeb013f3dd..96cff541ca 100644
>> --- a/qapi/machine.json
>> +++ b/qapi/machine.json
>> @@ -1681,3 +1681,22 @@
>>   ##
>>   { 'command': 'dumpdtb',
>>     'data': { 'filename': 'str' } }
>> +
>> +##
>> +# @x-query-fdt:
>> +#
>> +# Query for FDT element (node or property). Requires 'libfdt'.
>> +#
>> +# @nodepath: the path of the FDT node to be retrieved
>> +#
>> +# Features:
>> +# @unstable: This command is meant for debugging.
>> +#
>> +# Returns: FDT node
>> +#
>> +# Since: 7.2
>> +##
>> +{ 'command': 'x-query-fdt',
>> +  'data': { 'nodepath': 'str' },
>> +  'returns': 'HumanReadableText',
>> +  'features': [ 'unstable' ]  }
>> diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
>> index cd487ddd4d..6b15f6ace2 100644
>> --- a/softmmu/device_tree.c
>> +++ b/softmmu/device_tree.c
>> @@ -18,6 +18,7 @@
>>   #endif
>>   
>>   #include "qapi/error.h"
>> +#include "qapi/type-helpers.h"
>>   #include "qemu/error-report.h"
>>   #include "qemu/option.h"
>>   #include "qemu/bswap.h"
>> @@ -661,3 +662,49 @@ void qemu_fdt_qmp_dumpdtb(const char *filename, Error **errp)
>>   
>>       error_setg(errp, "Error when saving machine FDT to file %s", filename);
>>   }
>> +
>> +static void fdt_format_node(GString *buf, 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;
>> +
>> +    g_string_append_printf(buf, "%*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));
>> +
>> +        g_string_append_printf(buf, "%*s%s;\n", padding, "", propname);
>> +    }
>> +
>> +    padding -= 4;
>> +    g_string_append_printf(buf, "%*s};\n", padding, "");
>> +}
>> +
>> +HumanReadableText *qemu_fdt_qmp_query_fdt(const char *nodepath, Error **errp)
>> +{
>> +    g_autoptr(GString) buf = g_string_new("");
>> +    int node;
>> +
>> +    if (!current_machine->fdt) {
>> +        error_setg(errp, "Unable to find the machine FDT");
>> +        return NULL;
>> +    }
>> +
>> +    node = fdt_path_offset(current_machine->fdt, nodepath);
>> +    if (node < 0) {
>> +        error_setg(errp, "node '%s' not found in FDT", nodepath);
>> +        return NULL;
>> +    }
>> +
>> +    fdt_format_node(buf, node, 0);
>> +
>> +    return human_readable_text_from_str(buf);
>> +}
> 

Re: [PATCH for-7.2 v4 15/21] qmp/hmp, device_tree.c: introduce 'info fdt' command

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 11570 bytes --]

On Mon, Aug 29, 2022 at 07:00:55PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 8/29/22 00:34, David Gibson wrote:
> > On Fri, Aug 26, 2022 at 11:11:44AM -0300, Daniel Henrique Barboza wrote:
> > > 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 node or property.
> > > 
> > > '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 QMP/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,
> > > and then individual properties.
> > > 
> > > 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).
> > > 
> > > 'info fdt' is not something that we expect to be used aside from debugging,
> > > so we're implementing it in QMP as 'x-query-fdt'.
> > > 
> > > 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;
> > > };
> > 
> > So, I'm reasonably convinced allowing dumping the whole dtb from
> > qmp/hmp is useful.  I'm less convined that info fdt is worth the
> > additional complexity it incurs.  Note that as well as being able to
> > decompile a whole dtb using dtc, you can also extract and list
> > specific properties from a dtb blob using the 'fdtget' tool which is
> > part of the dtc tree.
> 
> What's your opinion on patch 21/21, where 'dumpdtb' can write a formatted
> FDT in a file with an extra option? That was possible because of the
> format helpers introduced for 'info fdt'. The idea is that since we're
> able to format a FDT in DTS format, we can also write the FDT in text
> format without relying on DTC to decode it.

Since it's mostly the same code, I think it's reasonable to throw in
if the info fdt stuff is there, but I don't think it's worth including
without that.  As a whole, I remain dubious that (info fdt + dumpdts)
is worth the complexity cost.

People with more practical experience debugging the embedded ARM
platforms might have a different opinion if they thing info fdt would
be really useful though.

> If we think that this 'dumpdtb' capability is worth having, I can respin
> the patches without 'info fdt' but adding these helpers to enable this
> 'dumpdtb' support. If not, then we can just remove patches 15-21 and
> be done with it.
> 
> 
> Thanks,
> 
> 
> Daniel
> 
> > 
> > > 
> > > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > > ---
> > >   hmp-commands-info.hx         | 13 ++++++++++
> > >   include/monitor/hmp.h        |  1 +
> > >   include/sysemu/device_tree.h |  4 +++
> > >   monitor/hmp-cmds.c           | 13 ++++++++++
> > >   monitor/qmp-cmds.c           | 12 +++++++++
> > >   qapi/machine.json            | 19 +++++++++++++++
> > >   softmmu/device_tree.c        | 47 ++++++++++++++++++++++++++++++++++++
> > >   7 files changed, 109 insertions(+)
> > > 
> > > diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> > > index 188d9ece3b..743b48865d 100644
> > > --- a/hmp-commands-info.hx
> > > +++ b/hmp-commands-info.hx
> > > @@ -921,3 +921,16 @@ SRST
> > >     ``stats``
> > >       Show runtime-collected statistics
> > >   ERST
> > > +
> > > +    {
> > > +        .name       = "fdt",
> > > +        .args_type  = "nodepath:s",
> > > +        .params     = "nodepath",
> > > +        .help       = "show firmware device tree node given its path",
> > > +        .cmd        = hmp_info_fdt,
> > > +    },
> > > +
> > > +SRST
> > > +  ``info fdt``
> > > +    Show a firmware device tree node given its path. Requires libfdt.
> > > +ERST
> > > diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
> > > index d7f324da59..c0883dd1e3 100644
> > > --- a/include/monitor/hmp.h
> > > +++ b/include/monitor/hmp.h
> > > @@ -135,6 +135,7 @@ void hmp_set_vcpu_dirty_limit(Monitor *mon, const QDict *qdict);
> > >   void hmp_cancel_vcpu_dirty_limit(Monitor *mon, const QDict *qdict);
> > >   void hmp_info_vcpu_dirty_limit(Monitor *mon, const QDict *qdict);
> > >   void hmp_dumpdtb(Monitor *mon, const QDict *qdict);
> > > +void hmp_info_fdt(Monitor *mon, const QDict *qdict);
> > >   void hmp_human_readable_text_helper(Monitor *mon,
> > >                                       HumanReadableText *(*qmp_handler)(Error **));
> > >   void hmp_info_stats(Monitor *mon, const QDict *qdict);
> > > diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
> > > index bf7684e4ed..057d13e397 100644
> > > --- a/include/sysemu/device_tree.h
> > > +++ b/include/sysemu/device_tree.h
> > > @@ -14,6 +14,8 @@
> > >   #ifndef DEVICE_TREE_H
> > >   #define DEVICE_TREE_H
> > > +#include "qapi/qapi-types-common.h"
> > > +
> > >   void *create_device_tree(int *sizep);
> > >   void *load_device_tree(const char *filename_path, int *sizep);
> > >   #ifdef CONFIG_LINUX
> > > @@ -137,6 +139,8 @@ int qemu_fdt_add_path(void *fdt, const char *path);
> > >   void qemu_fdt_dumpdtb(void *fdt, int size);
> > >   void qemu_fdt_qmp_dumpdtb(const char *filename, Error **errp);
> > > +HumanReadableText *qemu_fdt_qmp_query_fdt(const char *nodepath,
> > > +                                          Error **errp);
> > >   /**
> > >    * qemu_fdt_setprop_sized_cells_from_array:
> > > diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> > > index 1c7bfd3b9d..93a4103afa 100644
> > > --- a/monitor/hmp-cmds.c
> > > +++ b/monitor/hmp-cmds.c
> > > @@ -2484,3 +2484,16 @@ void hmp_dumpdtb(Monitor *mon, const QDict *qdict)
> > >           hmp_handle_error(mon, local_err);
> > >       }
> > >   }
> > > +
> > > +void hmp_info_fdt(Monitor *mon, const QDict *qdict)
> > > +{
> > > +    const char *nodepath = qdict_get_str(qdict, "nodepath");
> > > +    Error *err = NULL;
> > > +    g_autoptr(HumanReadableText) info = qmp_x_query_fdt(nodepath, &err);
> > > +
> > > +    if (hmp_handle_error(mon, err)) {
> > > +        return;
> > > +    }
> > > +
> > > +    monitor_printf(mon, "%s", info->human_readable_text);
> > > +}
> > > diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> > > index 8415aca08c..db2c6aa7da 100644
> > > --- a/monitor/qmp-cmds.c
> > > +++ b/monitor/qmp-cmds.c
> > > @@ -603,9 +603,21 @@ void qmp_dumpdtb(const char *filename, Error **errp)
> > >   {
> > >       return qemu_fdt_qmp_dumpdtb(filename, errp);
> > >   }
> > > +
> > > +HumanReadableText *qmp_x_query_fdt(const char *nodepath, Error **errp)
> > > +{
> > > +    return qemu_fdt_qmp_query_fdt(nodepath, errp);
> > > +}
> > >   #else
> > >   void qmp_dumpdtb(const char *filename, Error **errp)
> > >   {
> > >       error_setg(errp, "dumpdtb requires libfdt");
> > >   }
> > > +
> > > +HumanReadableText *qmp_x_query_fdt(const char *nodepath, Error **errp)
> > > +{
> > > +    error_setg(errp, "this command requires libfdt");
> > > +
> > > +    return NULL;
> > > +}
> > >   #endif
> > > diff --git a/qapi/machine.json b/qapi/machine.json
> > > index aeb013f3dd..96cff541ca 100644
> > > --- a/qapi/machine.json
> > > +++ b/qapi/machine.json
> > > @@ -1681,3 +1681,22 @@
> > >   ##
> > >   { 'command': 'dumpdtb',
> > >     'data': { 'filename': 'str' } }
> > > +
> > > +##
> > > +# @x-query-fdt:
> > > +#
> > > +# Query for FDT element (node or property). Requires 'libfdt'.
> > > +#
> > > +# @nodepath: the path of the FDT node to be retrieved
> > > +#
> > > +# Features:
> > > +# @unstable: This command is meant for debugging.
> > > +#
> > > +# Returns: FDT node
> > > +#
> > > +# Since: 7.2
> > > +##
> > > +{ 'command': 'x-query-fdt',
> > > +  'data': { 'nodepath': 'str' },
> > > +  'returns': 'HumanReadableText',
> > > +  'features': [ 'unstable' ]  }
> > > diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
> > > index cd487ddd4d..6b15f6ace2 100644
> > > --- a/softmmu/device_tree.c
> > > +++ b/softmmu/device_tree.c
> > > @@ -18,6 +18,7 @@
> > >   #endif
> > >   #include "qapi/error.h"
> > > +#include "qapi/type-helpers.h"
> > >   #include "qemu/error-report.h"
> > >   #include "qemu/option.h"
> > >   #include "qemu/bswap.h"
> > > @@ -661,3 +662,49 @@ void qemu_fdt_qmp_dumpdtb(const char *filename, Error **errp)
> > >       error_setg(errp, "Error when saving machine FDT to file %s", filename);
> > >   }
> > > +
> > > +static void fdt_format_node(GString *buf, 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;
> > > +
> > > +    g_string_append_printf(buf, "%*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));
> > > +
> > > +        g_string_append_printf(buf, "%*s%s;\n", padding, "", propname);
> > > +    }
> > > +
> > > +    padding -= 4;
> > > +    g_string_append_printf(buf, "%*s};\n", padding, "");
> > > +}
> > > +
> > > +HumanReadableText *qemu_fdt_qmp_query_fdt(const char *nodepath, Error **errp)
> > > +{
> > > +    g_autoptr(GString) buf = g_string_new("");
> > > +    int node;
> > > +
> > > +    if (!current_machine->fdt) {
> > > +        error_setg(errp, "Unable to find the machine FDT");
> > > +        return NULL;
> > > +    }
> > > +
> > > +    node = fdt_path_offset(current_machine->fdt, nodepath);
> > > +    if (node < 0) {
> > > +        error_setg(errp, "node '%s' not found in FDT", nodepath);
> > > +        return NULL;
> > > +    }
> > > +
> > > +    fdt_format_node(buf, node, 0);
> > > +
> > > +    return human_readable_text_from_str(buf);
> > > +}
> > 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH for-7.2 v4 15/21] qmp/hmp, device_tree.c: introduce 'info fdt' command

[Daniel Henrique Barboza]

On 8/29/22 22:50, David Gibson wrote:
> On Mon, Aug 29, 2022 at 07:00:55PM -0300, Daniel Henrique Barboza wrote:
>>
>>
>> On 8/29/22 00:34, David Gibson wrote:
>>> On Fri, Aug 26, 2022 at 11:11:44AM -0300, Daniel Henrique Barboza wrote:
>>>> 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 node or property.
>>>>
>>>> '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 QMP/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,
>>>> and then individual properties.
>>>>
>>>> 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).
>>>>
>>>> 'info fdt' is not something that we expect to be used aside from debugging,
>>>> so we're implementing it in QMP as 'x-query-fdt'.
>>>>
>>>> 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;
>>>> };
>>>
>>> So, I'm reasonably convinced allowing dumping the whole dtb from
>>> qmp/hmp is useful.  I'm less convined that info fdt is worth the
>>> additional complexity it incurs.  Note that as well as being able to
>>> decompile a whole dtb using dtc, you can also extract and list
>>> specific properties from a dtb blob using the 'fdtget' tool which is
>>> part of the dtc tree.
>>
>> What's your opinion on patch 21/21, where 'dumpdtb' can write a formatted
>> FDT in a file with an extra option? That was possible because of the
>> format helpers introduced for 'info fdt'. The idea is that since we're
>> able to format a FDT in DTS format, we can also write the FDT in text
>> format without relying on DTC to decode it.
> 
> Since it's mostly the same code, I think it's reasonable to throw in
> if the info fdt stuff is there, but I don't think it's worth including
> without that.  As a whole, I remain dubious that (info fdt + dumpdts)
> is worth the complexity cost.
> 
> People with more practical experience debugging the embedded ARM
> platforms might have a different opinion if they thing info fdt would
> be really useful though.

Fair enough. Let's wait to see if they have an option on that. Otherwise
I'll prune 'info fdt' from the next version and roll out just with
dumpdtb.


Thanks,

Daniel

> 
>> If we think that this 'dumpdtb' capability is worth having, I can respin
>> the patches without 'info fdt' but adding these helpers to enable this
>> 'dumpdtb' support. If not, then we can just remove patches 15-21 and
>> be done with it.
>>
>>
>> Thanks,
>>
>>
>> Daniel
>>
>>>
>>>>
>>>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>>> Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>>> ---
>>>>    hmp-commands-info.hx         | 13 ++++++++++
>>>>    include/monitor/hmp.h        |  1 +
>>>>    include/sysemu/device_tree.h |  4 +++
>>>>    monitor/hmp-cmds.c           | 13 ++++++++++
>>>>    monitor/qmp-cmds.c           | 12 +++++++++
>>>>    qapi/machine.json            | 19 +++++++++++++++
>>>>    softmmu/device_tree.c        | 47 ++++++++++++++++++++++++++++++++++++
>>>>    7 files changed, 109 insertions(+)
>>>>
>>>> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
>>>> index 188d9ece3b..743b48865d 100644
>>>> --- a/hmp-commands-info.hx
>>>> +++ b/hmp-commands-info.hx
>>>> @@ -921,3 +921,16 @@ SRST
>>>>      ``stats``
>>>>        Show runtime-collected statistics
>>>>    ERST
>>>> +
>>>> +    {
>>>> +        .name       = "fdt",
>>>> +        .args_type  = "nodepath:s",
>>>> +        .params     = "nodepath",
>>>> +        .help       = "show firmware device tree node given its path",
>>>> +        .cmd        = hmp_info_fdt,
>>>> +    },
>>>> +
>>>> +SRST
>>>> +  ``info fdt``
>>>> +    Show a firmware device tree node given its path. Requires libfdt.
>>>> +ERST
>>>> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
>>>> index d7f324da59..c0883dd1e3 100644
>>>> --- a/include/monitor/hmp.h
>>>> +++ b/include/monitor/hmp.h
>>>> @@ -135,6 +135,7 @@ void hmp_set_vcpu_dirty_limit(Monitor *mon, const QDict *qdict);
>>>>    void hmp_cancel_vcpu_dirty_limit(Monitor *mon, const QDict *qdict);
>>>>    void hmp_info_vcpu_dirty_limit(Monitor *mon, const QDict *qdict);
>>>>    void hmp_dumpdtb(Monitor *mon, const QDict *qdict);
>>>> +void hmp_info_fdt(Monitor *mon, const QDict *qdict);
>>>>    void hmp_human_readable_text_helper(Monitor *mon,
>>>>                                        HumanReadableText *(*qmp_handler)(Error **));
>>>>    void hmp_info_stats(Monitor *mon, const QDict *qdict);
>>>> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
>>>> index bf7684e4ed..057d13e397 100644
>>>> --- a/include/sysemu/device_tree.h
>>>> +++ b/include/sysemu/device_tree.h
>>>> @@ -14,6 +14,8 @@
>>>>    #ifndef DEVICE_TREE_H
>>>>    #define DEVICE_TREE_H
>>>> +#include "qapi/qapi-types-common.h"
>>>> +
>>>>    void *create_device_tree(int *sizep);
>>>>    void *load_device_tree(const char *filename_path, int *sizep);
>>>>    #ifdef CONFIG_LINUX
>>>> @@ -137,6 +139,8 @@ int qemu_fdt_add_path(void *fdt, const char *path);
>>>>    void qemu_fdt_dumpdtb(void *fdt, int size);
>>>>    void qemu_fdt_qmp_dumpdtb(const char *filename, Error **errp);
>>>> +HumanReadableText *qemu_fdt_qmp_query_fdt(const char *nodepath,
>>>> +                                          Error **errp);
>>>>    /**
>>>>     * qemu_fdt_setprop_sized_cells_from_array:
>>>> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
>>>> index 1c7bfd3b9d..93a4103afa 100644
>>>> --- a/monitor/hmp-cmds.c
>>>> +++ b/monitor/hmp-cmds.c
>>>> @@ -2484,3 +2484,16 @@ void hmp_dumpdtb(Monitor *mon, const QDict *qdict)
>>>>            hmp_handle_error(mon, local_err);
>>>>        }
>>>>    }
>>>> +
>>>> +void hmp_info_fdt(Monitor *mon, const QDict *qdict)
>>>> +{
>>>> +    const char *nodepath = qdict_get_str(qdict, "nodepath");
>>>> +    Error *err = NULL;
>>>> +    g_autoptr(HumanReadableText) info = qmp_x_query_fdt(nodepath, &err);
>>>> +
>>>> +    if (hmp_handle_error(mon, err)) {
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    monitor_printf(mon, "%s", info->human_readable_text);
>>>> +}
>>>> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
>>>> index 8415aca08c..db2c6aa7da 100644
>>>> --- a/monitor/qmp-cmds.c
>>>> +++ b/monitor/qmp-cmds.c
>>>> @@ -603,9 +603,21 @@ void qmp_dumpdtb(const char *filename, Error **errp)
>>>>    {
>>>>        return qemu_fdt_qmp_dumpdtb(filename, errp);
>>>>    }
>>>> +
>>>> +HumanReadableText *qmp_x_query_fdt(const char *nodepath, Error **errp)
>>>> +{
>>>> +    return qemu_fdt_qmp_query_fdt(nodepath, errp);
>>>> +}
>>>>    #else
>>>>    void qmp_dumpdtb(const char *filename, Error **errp)
>>>>    {
>>>>        error_setg(errp, "dumpdtb requires libfdt");
>>>>    }
>>>> +
>>>> +HumanReadableText *qmp_x_query_fdt(const char *nodepath, Error **errp)
>>>> +{
>>>> +    error_setg(errp, "this command requires libfdt");
>>>> +
>>>> +    return NULL;
>>>> +}
>>>>    #endif
>>>> diff --git a/qapi/machine.json b/qapi/machine.json
>>>> index aeb013f3dd..96cff541ca 100644
>>>> --- a/qapi/machine.json
>>>> +++ b/qapi/machine.json
>>>> @@ -1681,3 +1681,22 @@
>>>>    ##
>>>>    { 'command': 'dumpdtb',
>>>>      'data': { 'filename': 'str' } }
>>>> +
>>>> +##
>>>> +# @x-query-fdt:
>>>> +#
>>>> +# Query for FDT element (node or property). Requires 'libfdt'.
>>>> +#
>>>> +# @nodepath: the path of the FDT node to be retrieved
>>>> +#
>>>> +# Features:
>>>> +# @unstable: This command is meant for debugging.
>>>> +#
>>>> +# Returns: FDT node
>>>> +#
>>>> +# Since: 7.2
>>>> +##
>>>> +{ 'command': 'x-query-fdt',
>>>> +  'data': { 'nodepath': 'str' },
>>>> +  'returns': 'HumanReadableText',
>>>> +  'features': [ 'unstable' ]  }
>>>> diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
>>>> index cd487ddd4d..6b15f6ace2 100644
>>>> --- a/softmmu/device_tree.c
>>>> +++ b/softmmu/device_tree.c
>>>> @@ -18,6 +18,7 @@
>>>>    #endif
>>>>    #include "qapi/error.h"
>>>> +#include "qapi/type-helpers.h"
>>>>    #include "qemu/error-report.h"
>>>>    #include "qemu/option.h"
>>>>    #include "qemu/bswap.h"
>>>> @@ -661,3 +662,49 @@ void qemu_fdt_qmp_dumpdtb(const char *filename, Error **errp)
>>>>        error_setg(errp, "Error when saving machine FDT to file %s", filename);
>>>>    }
>>>> +
>>>> +static void fdt_format_node(GString *buf, 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;
>>>> +
>>>> +    g_string_append_printf(buf, "%*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));
>>>> +
>>>> +        g_string_append_printf(buf, "%*s%s;\n", padding, "", propname);
>>>> +    }
>>>> +
>>>> +    padding -= 4;
>>>> +    g_string_append_printf(buf, "%*s};\n", padding, "");
>>>> +}
>>>> +
>>>> +HumanReadableText *qemu_fdt_qmp_query_fdt(const char *nodepath, Error **errp)
>>>> +{
>>>> +    g_autoptr(GString) buf = g_string_new("");
>>>> +    int node;
>>>> +
>>>> +    if (!current_machine->fdt) {
>>>> +        error_setg(errp, "Unable to find the machine FDT");
>>>> +        return NULL;
>>>> +    }
>>>> +
>>>> +    node = fdt_path_offset(current_machine->fdt, nodepath);
>>>> +    if (node < 0) {
>>>> +        error_setg(errp, "node '%s' not found in FDT", nodepath);
>>>> +        return NULL;
>>>> +    }
>>>> +
>>>> +    fdt_format_node(buf, node, 0);
>>>> +
>>>> +    return human_readable_text_from_str(buf);
>>>> +}
>>>
>>
> 

Re: [PATCH for-7.2 v4 14/21] qmp/hmp, device_tree.c: introduce dumpdtb

[Markus Armbruster]

Daniel Henrique Barboza <danielhb413@gmail.com> writes:

> 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
> 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 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>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  hmp-commands.hx              | 13 +++++++++++++
>  include/monitor/hmp.h        |  1 +
>  include/sysemu/device_tree.h |  1 +
>  monitor/hmp-cmds.c           | 12 ++++++++++++
>  monitor/qmp-cmds.c           | 13 +++++++++++++
>  qapi/machine.json            | 17 +++++++++++++++++
>  softmmu/device_tree.c        | 18 ++++++++++++++++++
>  7 files changed, 75 insertions(+)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 182e639d14..2dd737078e 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1800,3 +1800,16 @@ ERST
>                        "\n\t\t\t\t\t limit on a specified virtual cpu",
>          .cmd        = hmp_cancel_vcpu_dirty_limit,
>      },
> +
> +SRST
> +``dumpdtb`` *filename*
> +  Save the FDT in the 'filename' file to be decoded using dtc.
> +  Requires 'libfdt' support.
> +ERST
> +    {
> +        .name       = "dumpdtb",
> +        .args_type  = "filename:F",
> +        .params     = "filename",
> +        .help       = "save the FDT in the 'filename' file to be decoded using dtc",
> +        .cmd        = hmp_dumpdtb,
> +    },
> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
> index a618eb1e4e..d7f324da59 100644
> --- a/include/monitor/hmp.h
> +++ b/include/monitor/hmp.h
> @@ -134,6 +134,7 @@ void hmp_calc_dirty_rate(Monitor *mon, const QDict *qdict);
>  void hmp_set_vcpu_dirty_limit(Monitor *mon, const QDict *qdict);
>  void hmp_cancel_vcpu_dirty_limit(Monitor *mon, const QDict *qdict);
>  void hmp_info_vcpu_dirty_limit(Monitor *mon, const QDict *qdict);
> +void hmp_dumpdtb(Monitor *mon, const QDict *qdict);
>  void hmp_human_readable_text_helper(Monitor *mon,
>                                      HumanReadableText *(*qmp_handler)(Error **));
>  void hmp_info_stats(Monitor *mon, const QDict *qdict);
> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
> index ef060a9759..bf7684e4ed 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 qemu_fdt_qmp_dumpdtb(const char *filename, Error **errp);
>  
>  /**
>   * qemu_fdt_setprop_sized_cells_from_array:
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index c6cd6f91dd..1c7bfd3b9d 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -2472,3 +2472,15 @@ exit:
>  exit_no_print:
>      error_free(err);
>  }
> +
> +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);
> +
> +    if (local_err) {
> +        hmp_handle_error(mon, local_err);
> +    }

Make this unconditional, please.

> +}
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index 7314cd813d..8415aca08c 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -45,6 +45,7 @@
>  #include "hw/intc/intc.h"
>  #include "hw/rdma/rdma.h"
>  #include "monitor/stats.h"
> +#include "sysemu/device_tree.h"
>  
>  NameInfo *qmp_query_name(Error **errp)
>  {
> @@ -596,3 +597,15 @@ bool apply_str_list_filter(const char *string, strList *list)
>      }
>      return false;
>  }
> +
> +#ifdef CONFIG_FDT
> +void qmp_dumpdtb(const char *filename, Error **errp)
> +{
> +    return qemu_fdt_qmp_dumpdtb(filename, errp);
> +}
> +#else
> +void qmp_dumpdtb(const char *filename, Error **errp)
> +{
> +    error_setg(errp, "dumpdtb requires libfdt");

Bad error message, in my opinion: not enough information to be
actionable.  But see below.

> +}
> +#endif

Have you considered adding this to softmmu/device_tree.c instead?

> diff --git a/qapi/machine.json b/qapi/machine.json
> index 6afd1936b0..aeb013f3dd 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1664,3 +1664,20 @@
>       '*size': 'size',
>       '*max-size': 'size',
>       '*slots': 'uint64' } }
> +
> +##
> +# @dumpdtb:
> +#
> +# Save the FDT in dtb format. Requires 'libfdt' support.
> +#
> +# @filename: name of the FDT file to be created
> +#
> +# Since: 7.2
> +#
> +# Example:
> +#   {"execute": "dumpdtb"}
> +#    "arguments": { "filename": "/tmp/fdt.dtb" } }
> +#
> +##
> +{ 'command': 'dumpdtb',
> +  'data': { 'filename': 'str' } }

Why isn't this 'if': 'CONFIG_FDT'?

If it was, you wouldn't need a qmp_dumpdtb() dummy that always fails.

Same for HMP.

> diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
> index 6ca3fad285..cd487ddd4d 100644
> --- a/softmmu/device_tree.c
> +++ b/softmmu/device_tree.c
> @@ -643,3 +643,21 @@ out:
>      g_free(propcells);
>      return ret;
>  }
> +
> +void qemu_fdt_qmp_dumpdtb(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);
> +}

Unhelpful error reporting.  Please try something like

       g_autoptr(GError) err = NULL;

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

Re: [PATCH for-7.2 v4 15/21] qmp/hmp, device_tree.c: introduce 'info fdt' command

[Markus Armbruster]

Daniel Henrique Barboza <danielhb413@gmail.com> writes:

> 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 node or property.
>
> '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 QMP/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,
> and then individual properties.
>
> 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).
>
> 'info fdt' is not something that we expect to be used aside from debugging,
> so we're implementing it in QMP as 'x-query-fdt'.
>
> 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>
> Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  hmp-commands-info.hx         | 13 ++++++++++
>  include/monitor/hmp.h        |  1 +
>  include/sysemu/device_tree.h |  4 +++
>  monitor/hmp-cmds.c           | 13 ++++++++++
>  monitor/qmp-cmds.c           | 12 +++++++++
>  qapi/machine.json            | 19 +++++++++++++++
>  softmmu/device_tree.c        | 47 ++++++++++++++++++++++++++++++++++++
>  7 files changed, 109 insertions(+)
>
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index 188d9ece3b..743b48865d 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -921,3 +921,16 @@ SRST
>    ``stats``
>      Show runtime-collected statistics
>  ERST
> +
> +    {
> +        .name       = "fdt",
> +        .args_type  = "nodepath:s",
> +        .params     = "nodepath",
> +        .help       = "show firmware device tree node given its path",
> +        .cmd        = hmp_info_fdt,
> +    },
> +
> +SRST
> +  ``info fdt``
> +    Show a firmware device tree node given its path. Requires libfdt.
> +ERST
> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
> index d7f324da59..c0883dd1e3 100644
> --- a/include/monitor/hmp.h
> +++ b/include/monitor/hmp.h
> @@ -135,6 +135,7 @@ void hmp_set_vcpu_dirty_limit(Monitor *mon, const QDict *qdict);
>  void hmp_cancel_vcpu_dirty_limit(Monitor *mon, const QDict *qdict);
>  void hmp_info_vcpu_dirty_limit(Monitor *mon, const QDict *qdict);
>  void hmp_dumpdtb(Monitor *mon, const QDict *qdict);
> +void hmp_info_fdt(Monitor *mon, const QDict *qdict);
>  void hmp_human_readable_text_helper(Monitor *mon,
>                                      HumanReadableText *(*qmp_handler)(Error **));
>  void hmp_info_stats(Monitor *mon, const QDict *qdict);
> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
> index bf7684e4ed..057d13e397 100644
> --- a/include/sysemu/device_tree.h
> +++ b/include/sysemu/device_tree.h
> @@ -14,6 +14,8 @@
>  #ifndef DEVICE_TREE_H
>  #define DEVICE_TREE_H
>  
> +#include "qapi/qapi-types-common.h"
> +
>  void *create_device_tree(int *sizep);
>  void *load_device_tree(const char *filename_path, int *sizep);
>  #ifdef CONFIG_LINUX
> @@ -137,6 +139,8 @@ int qemu_fdt_add_path(void *fdt, const char *path);
>  
>  void qemu_fdt_dumpdtb(void *fdt, int size);
>  void qemu_fdt_qmp_dumpdtb(const char *filename, Error **errp);
> +HumanReadableText *qemu_fdt_qmp_query_fdt(const char *nodepath,
> +                                          Error **errp);
>  
>  /**
>   * qemu_fdt_setprop_sized_cells_from_array:
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 1c7bfd3b9d..93a4103afa 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -2484,3 +2484,16 @@ void hmp_dumpdtb(Monitor *mon, const QDict *qdict)
>          hmp_handle_error(mon, local_err);
>      }
>  }
> +
> +void hmp_info_fdt(Monitor *mon, const QDict *qdict)
> +{
> +    const char *nodepath = qdict_get_str(qdict, "nodepath");
> +    Error *err = NULL;
> +    g_autoptr(HumanReadableText) info = qmp_x_query_fdt(nodepath, &err);
> +
> +    if (hmp_handle_error(mon, err)) {
> +        return;
> +    }
> +
> +    monitor_printf(mon, "%s", info->human_readable_text);
> +}
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index 8415aca08c..db2c6aa7da 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -603,9 +603,21 @@ void qmp_dumpdtb(const char *filename, Error **errp)
>  {
>      return qemu_fdt_qmp_dumpdtb(filename, errp);
>  }
> +
> +HumanReadableText *qmp_x_query_fdt(const char *nodepath, Error **errp)
> +{
> +    return qemu_fdt_qmp_query_fdt(nodepath, errp);
> +}
>  #else
>  void qmp_dumpdtb(const char *filename, Error **errp)
>  {
>      error_setg(errp, "dumpdtb requires libfdt");
>  }
> +
> +HumanReadableText *qmp_x_query_fdt(const char *nodepath, Error **errp)
> +{
> +    error_setg(errp, "this command requires libfdt");
> +
> +    return NULL;
> +}
>  #endif
> diff --git a/qapi/machine.json b/qapi/machine.json
> index aeb013f3dd..96cff541ca 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1681,3 +1681,22 @@
>  ##
>  { 'command': 'dumpdtb',
>    'data': { 'filename': 'str' } }
> +
> +##
> +# @x-query-fdt:
> +#
> +# Query for FDT element (node or property). Requires 'libfdt'.
> +#
> +# @nodepath: the path of the FDT node to be retrieved

In a QAPI schema, we separate words by dashes unless there's a
compelling reason not to.  Thus: @node-path.

> +#
> +# Features:
> +# @unstable: This command is meant for debugging.
> +#
> +# Returns: FDT node
> +#
> +# Since: 7.2
> +##
> +{ 'command': 'x-query-fdt',
> +  'data': { 'nodepath': 'str' },
> +  'returns': 'HumanReadableText',
> +  'features': [ 'unstable' ]  }

Same question as for the previous patch: why isn't this 'if':
'CONFIG_FDT'?

[...]

Re: [PATCH for-7.2 v4 15/21] qmp/hmp, device_tree.c: introduce 'info fdt' command

[Markus Armbruster]

David Gibson <david@gibson.dropbear.id.au> writes:

> On Mon, Aug 29, 2022 at 07:00:55PM -0300, Daniel Henrique Barboza wrote:
>> 
>> 
>> On 8/29/22 00:34, David Gibson wrote:
>> > On Fri, Aug 26, 2022 at 11:11:44AM -0300, Daniel Henrique Barboza wrote:
>> > > 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 node or property.
>> > > 
>> > > '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 QMP/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,
>> > > and then individual properties.
>> > > 
>> > > 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).
>> > > 
>> > > 'info fdt' is not something that we expect to be used aside from debugging,
>> > > so we're implementing it in QMP as 'x-query-fdt'.
>> > > 
>> > > 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;
>> > > };
>> > 
>> > So, I'm reasonably convinced allowing dumping the whole dtb from
>> > qmp/hmp is useful.  I'm less convined that info fdt is worth the
>> > additional complexity it incurs.  Note that as well as being able to
>> > decompile a whole dtb using dtc, you can also extract and list
>> > specific properties from a dtb blob using the 'fdtget' tool which is
>> > part of the dtc tree.
>> 
>> What's your opinion on patch 21/21, where 'dumpdtb' can write a formatted
>> FDT in a file with an extra option? That was possible because of the
>> format helpers introduced for 'info fdt'. The idea is that since we're
>> able to format a FDT in DTS format, we can also write the FDT in text
>> format without relying on DTC to decode it.
>
> Since it's mostly the same code, I think it's reasonable to throw in
> if the info fdt stuff is there, but I don't think it's worth including
> without that.  As a whole, I remain dubious that (info fdt + dumpdts)
> is worth the complexity cost.

How much code does it take, and who's going to maintain it?

> People with more practical experience debugging the embedded ARM
> platforms might have a different opinion if they thing info fdt would
> be really useful though.

They better speak up then :)

>> If we think that this 'dumpdtb' capability is worth having, I can respin
>> the patches without 'info fdt' but adding these helpers to enable this
>> 'dumpdtb' support. If not, then we can just remove patches 15-21 and
>> be done with it.
>> 
>> 
>> Thanks,
>> 
>> 
>> Daniel

Re: [PATCH for-7.2 v4 15/21] qmp/hmp, device_tree.c: introduce 'info fdt' command

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4510 bytes --]

On Tue, Aug 30, 2022 at 12:43:23PM +0200, Markus Armbruster wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> > On Mon, Aug 29, 2022 at 07:00:55PM -0300, Daniel Henrique Barboza wrote:
> >> 
> >> 
> >> On 8/29/22 00:34, David Gibson wrote:
> >> > On Fri, Aug 26, 2022 at 11:11:44AM -0300, Daniel Henrique Barboza wrote:
> >> > > 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 node or property.
> >> > > 
> >> > > '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 QMP/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,
> >> > > and then individual properties.
> >> > > 
> >> > > 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).
> >> > > 
> >> > > 'info fdt' is not something that we expect to be used aside from debugging,
> >> > > so we're implementing it in QMP as 'x-query-fdt'.
> >> > > 
> >> > > 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;
> >> > > };
> >> > 
> >> > So, I'm reasonably convinced allowing dumping the whole dtb from
> >> > qmp/hmp is useful.  I'm less convined that info fdt is worth the
> >> > additional complexity it incurs.  Note that as well as being able to
> >> > decompile a whole dtb using dtc, you can also extract and list
> >> > specific properties from a dtb blob using the 'fdtget' tool which is
> >> > part of the dtc tree.
> >> 
> >> What's your opinion on patch 21/21, where 'dumpdtb' can write a formatted
> >> FDT in a file with an extra option? That was possible because of the
> >> format helpers introduced for 'info fdt'. The idea is that since we're
> >> able to format a FDT in DTS format, we can also write the FDT in text
> >> format without relying on DTC to decode it.
> >
> > Since it's mostly the same code, I think it's reasonable to throw in
> > if the info fdt stuff is there, but I don't think it's worth including
> > without that.  As a whole, I remain dubious that (info fdt + dumpdts)
> > is worth the complexity cost.
> 
> How much code does it take, and who's going to maintain it?

It's not especially big, but it's not negligible.  Perhaps the part
that I'm most uncomfortable about is that it requires a bunch of messy
heuristics to guess how to format the output - DT properties are just
bytestrings, any internal interpretation is based on the specific
bindings for them.

dtc already has these and I don't love having a second, potentially
different copy of necessarily imperfect heuristics out in the wild.

> > People with more practical experience debugging the embedded ARM
> > platforms might have a different opinion if they thing info fdt would
> > be really useful though.
> 
> They better speak up then :)

Just so.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]