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

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

[Daniel Henrique Barboza]

Hi,

This second version implements the review comments/suggestion made in
the v1 review [1].

First 13 patches are adaptations (basically set machine->fdt) I made in
several machines to enable them to work with these commands. Patches
14-20 have the actual QMP/HMP/device_tree implementation. A plan for the
machine patches would be to get an ack from the machine owners and push
them with the QMP/HMP patches. Pick the machine patches in their own
trees and push it separately also works. Or, or course, NACK the machine
changes if I somewhat messed up. Either way, patches 14-20 are
independent from the individual machine changes.

Notable cases where I chose not to change the machine code:

- hw/rx/rx-gdbsim: I didn't see the point of enabling this command in
the GDB simulator. Can be done if required;

- hw/mips/boston: the FDT is handled as a const void* pointer inside
boston_mach_init(), as result of the return type of boston_fdt_filter()
return the 'const' modifier in the FDT. I left it alone because the
required change would be to either cast the const pointer as
non-constant in machine->fdt or change boston_fdt_filter() to not return
a const pointer. Can also be done but it requires a little more thought
(unless we're ok with a "void *" cast).

Other notable changes from v1:

- 'save-fdt' was renamed as 'dumpdtb'´;

- Both commands were changed to have a base QMP implementation that does
all the work, with the HMP support calling the QMP logic behind the
scenes.  'dumpdtb' is implemented as qmp_dumpdtb(), 'info fdt' is
implemented as x-query-fdt(). I didn't mark 'dumpdtb' as unstable
because we're doing the same thing as -machine dumpdtb does for awhile.
'info fdt' is more of a debug tool than something that we expect regular
users to consume;

- 'info fdt' is now receiving a second 'propname' argument to support
printing properties, as suggested by David Gilbert;

- the QMP/HMP implementations are now gated with "ifdef CONFIG_FDT" to
allow for successful build in systems that do not have libfdt support.

All other code changes made are consequences of the changes mentioned above.

Changes from v1:
- all patches that set machine->fdt:
  - updated comments to explain why we're setting machine->fdt
- patches 2-6, 8, 10-12: new
  - set machine->fdt for most of the machines that uses the FDT via
load_device_tree()
- patch 14 (former 4):
  - renamed 'fdt-save' command to 'dumpdtb'
  - added QMP counterpart 'qmp_dumpdtb'
- patch 15 (former 5):
  - added QMP counterpart 'x-query-fdt' of 'info fdt'
- patch 20 (former 10):
  - added the 'propname' parameter to support parameters instead of parsing
the fullpath of the parameter
  - helper fdt_find_property() was removed. The logic is short enough to
be open coded in qmp_x_query_fdt()
- v1 link: https://lists.gnu.org/archive/html/qemu-devel/2022-07/msg04119.html
  
[1] https://lists.gnu.org/archive/html/qemu-devel/2022-07/msg04119.html

Daniel Henrique Barboza (20):
  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 props 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

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

-- 
2.36.1

[PATCH for-7.2 v2 01/20] 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 | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index ada2717f76..9f5ceb62d2 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -684,7 +684,13 @@ 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' commands. Use fdt_pack() to shrink the blob
+     * size we're going to store.
+     */
+    fdt_pack(fdt);
+    ms->fdt = fdt;
 
     return size;
 
-- 
2.36.1

[PATCH for-7.2 v2 02/20] 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      | 13 ++++++++++++-
 hw/microblaze/meson.build |  2 +-
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/hw/microblaze/boot.c b/hw/microblaze/boot.c
index 8b92a9801a..001cdefd5c 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,15 @@ 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' commands. Use fdt_pack() to
+     * shrink the blob size we're going to store.
+     */
+    fdt_pack(fdt);
+    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.36.1

[PATCH for-7.2 v2 03/20] 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      | 13 ++++++++++++-
 hw/nios2/meson.build |  2 +-
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/hw/nios2/boot.c b/hw/nios2/boot.c
index 21cbffff47..6c40565704 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,15 @@ 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' commands. Use fdt_pack() to
+     * shrink the blob size we're going to store.
+     */
+    fdt_pack(fdt);
+    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.36.1

[PATCH for-7.2 v2 04/20] 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 | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 32495d0123..07c15fc181 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,18 @@ 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' commands. Use fdt_pack() to
+     * shrink the blob size we're going to store.
+     *
+     * The FDT is re-created during reset, so free machine->fdt
+     * to avoid leaking the old FDT.
+     */
+    g_free(machine->fdt);
+    fdt_pack(fdt);
+    machine->fdt = fdt;
 
 out:
     g_free(pci_map);
-- 
2.36.1

[PATCH for-7.2 v2 05/20] 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 | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
index 873f930c77..2ddbb1744d 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,15 @@ 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' commands. Use fdt_pack() to
+     * shrink the blob size we're going to store.
+     */
+    fdt_pack(fdt);
+    machine->fdt = fdt;
+
     return 0;
 }
 
-- 
2.36.1

[PATCH for-7.2 v2 06/20] 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 | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index 7e8da657c2..fa20a3ff49 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,14 @@ 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' commands. Use fdt_pack() to
+     * shrink the blob size we're going to store.
+     */
+    fdt_pack(fdt);
+    machine->fdt = fdt;
 
     return fdt_size;
 }
-- 
2.36.1

[PATCH for-7.2 v2 07/20] 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 | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
index 53b126ff48..ed1d37486d 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,15 @@ 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' commands. Use fdt_pack() to
+     * shrink the blob size we're going to store.
+     */
+    fdt_pack(fdt);
+    machine->fdt = fdt;
+
     return fdt_size;
 }
 
-- 
2.36.1

[PATCH for-7.2 v2 08/20] 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 | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
index 61f4263953..8a6c6e4b7d 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -331,6 +331,15 @@ 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' commands. Use fdt_pack()
+     * to shrink the blob size we're going to store.
+     */
+    fdt_pack(fdt);
+    machine->fdt = fdt;
+
     pm->cpu->vhyp = PPC_VIRTUAL_HYPERVISOR(machine);
 }
 
-- 
2.36.1

[PATCH for-7.2 v2 09/20] 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 | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index d3f77c8367..f5162f8b7b 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -608,7 +608,11 @@ 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.
+     */
+    machine->fdt = fdt;
 }
 
 static ISABus *pnv_chip_power8_isa_create(PnvChip *chip, Error **errp)
-- 
2.36.1

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

[Daniel Henrique Barboza]

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

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

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

Cc: Cédric Le Goater <clg@kaod.org>
Cc: qemu-ppc@nongnu.org
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr.c       | 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..94c90f0351 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' 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..0079bc6fdc 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'
+     * HMP commands.
+     */
+    MACHINE(spapr)->fdt = fdt;
+
     return H_SUCCESS;
 }
 
-- 
2.36.1

[PATCH for-7.2 v2 11/20] 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>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/riscv/sifive_u.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index e4c814a3ea..10f5289966 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -634,6 +634,14 @@ 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' commands. Use fdt_pack() to shrink
+     * the blob size we're going to store.
+     */
+    fdt_pack(s->fdt);
+    machine->fdt = s->fdt;
+
     /* reset vector */
     uint32_t reset_vec[12] = {
         s->msel,                       /* MSEL pin state */
-- 
2.36.1

[PATCH for-7.2 v2 12/20] 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>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/riscv/spike.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index e41b6aa9f0..2909f7b2a1 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,15 @@ 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' commands. Use fdt_pack() to
+     * shrink the blob size we're going to store.
+     */
+    fdt_pack(s->fdt);
+    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.36.1

[PATCH for-7.2 v2 13/20] 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    | 11 ++++++++++-
 2 files changed, 11 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..212f5f3dc7 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,14 @@ 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' commands. Use fdt_pack() to
+             * shrink the blob size we're going to store.
+             */
+            fdt_pack(fdt);
+            machine->fdt = fdt;
         }
 #else
         if (dtb_filename) {
-- 
2.36.1

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

Cc: Dr. David Alan Gilbert <dgilbert@redhat.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..d2554e9701 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:s",
+        .params     = "[filename] file to save the FDT",
+        .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..d23ec85f9d 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) {
+        error_report_err(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.36.1

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

'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>
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 d23ec85f9d..accde90380 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -2484,3 +2484,16 @@ void hmp_dumpdtb(Monitor *mon, const QDict *qdict)
         error_report_err(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..3fb07b537f 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.36.1

[PATCH for-7.2 v2 16/20] device_tree.c: support string props 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.

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

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

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

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

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

diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
index 3fb07b537f..8691c3ccc0 100644
--- a/softmmu/device_tree.c
+++ b/softmmu/device_tree.c
@@ -663,6 +663,24 @@ 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(const void *data, int size)
+{
+    const char *str = data;
+    int i;
+
+    if (size <= 0 || str[size - 1] != '\0') {
+        return false;
+    }
+
+    for (i = 0; i < size - 1; i++) {
+        if (!g_ascii_isprint(str[i])) {
+            return false;
+        }
+    }
+
+    return true;
+}
+
 static void fdt_format_node(GString *buf, int node, int depth)
 {
     const struct fdt_property *prop = NULL;
@@ -681,7 +699,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(prop->data, prop_size)) {
+            g_string_append_printf(buf, "%*s%s = '%s'\n",
+                                   padding, "", propname, prop->data);
+        } else {
+            g_string_append_printf(buf, "%*s%s;\n", padding, "", propname);
+        }
     }
 
     padding -= 4;
-- 
2.36.1

[PATCH for-7.2 v2 17/20] device_tree.c: support remaining FDT prop types

[Daniel Henrique Barboza]

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

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

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

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

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

diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
index 8691c3ccc0..9d95e4120b 100644
--- a/softmmu/device_tree.c
+++ b/softmmu/device_tree.c
@@ -681,6 +681,53 @@ static bool fdt_prop_is_string(const void *data, int size)
     return true;
 }
 
+static bool fdt_prop_is_uint32_array(int size)
+{
+    return size % 4 == 0;
+}
+
+static void fdt_prop_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, "%x", 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;
@@ -699,11 +746,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(prop->data, prop_size)) {
             g_string_append_printf(buf, "%*s%s = '%s'\n",
                                    padding, "", propname, prop->data);
+        } 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.36.1

[PATCH for-7.2 v2 18/20] 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 9d95e4120b..99d95c1cb9 100644
--- a/softmmu/device_tree.c
+++ b/softmmu/device_tree.c
@@ -728,17 +728,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;
 
@@ -763,6 +772,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, "");
 }
@@ -783,7 +796,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.36.1

[PATCH for-7.2 v2 19/20] 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 99d95c1cb9..902a7f680b 100644
--- a/softmmu/device_tree.c
+++ b/softmmu/device_tree.c
@@ -728,6 +728,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(data, prop_size)) {
+        g_string_append_printf(buf, "%*s%s = '%s'\n", padding, "",
+                               propname, (char *)data);
+    } 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)
@@ -755,21 +774,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(prop->data, prop_size)) {
-            g_string_append_printf(buf, "%*s%s = '%s'\n",
-                                   padding, "", propname, prop->data);
-        } 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.36.1

[PATCH for-7.2 v2 20/20] 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>
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 accde90380..df8493adc5 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 902a7f680b..be7b7e297e 100644
--- a/softmmu/device_tree.c
+++ b/softmmu/device_tree.c
@@ -785,23 +785,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.36.1

Re: [PATCH for-7.2 v2 09/20] hw/ppc: set machine->fdt in pnv_reset()

[Frederic Barrat]

On 05/08/2022 11:39, Daniel Henrique Barboza wrote:
> 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 | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index d3f77c8367..f5162f8b7b 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -608,7 +608,11 @@ 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.
> +     */
> +    machine->fdt = fdt;


Can pnv_reset() be called several times in the same instance of the qemu 
process, in which case we leak memory?

   Fred


>   }
>   
>   static ISABus *pnv_chip_power8_isa_create(PnvChip *chip, Error **errp)

Re: [PATCH for-7.2 v2 09/20] hw/ppc: set machine->fdt in pnv_reset()

[Daniel Henrique Barboza]

On 8/5/22 08:03, Frederic Barrat wrote:
> 
> 
> On 05/08/2022 11:39, Daniel Henrique Barboza wrote:
>> 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 | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index d3f77c8367..f5162f8b7b 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -608,7 +608,11 @@ 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.
>> +     */
>> +    machine->fdt = fdt;
> 
> 
> Can pnv_reset() be called several times in the same instance of the qemu process, in which case we leak memory?

hmmm I think it's possible if we issue a 'system_reset' via the monitor.

I'll put a g_free(machine->fdt) before the assignment.


Daniel


> 
>    Fred
> 
> 
>>   }
>>   static ISABus *pnv_chip_power8_isa_create(PnvChip *chip, Error **errp)

Re: [PATCH for-7.2 v2 11/20] hw/riscv: set machine->fdt in sifive_u_machine_init()

[Alistair Francis]

On Fri, Aug 5, 2022 at 7:49 PM Daniel Henrique Barboza
<danielhb413@gmail.com> wrote:
>
> 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>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/riscv/sifive_u.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index e4c814a3ea..10f5289966 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -634,6 +634,14 @@ 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' commands. Use fdt_pack() to shrink
> +     * the blob size we're going to store.
> +     */
> +    fdt_pack(s->fdt);
> +    machine->fdt = s->fdt;
> +
>      /* reset vector */
>      uint32_t reset_vec[12] = {
>          s->msel,                       /* MSEL pin state */
> --
> 2.36.1
>
>

Re: [PATCH for-7.2 v2 12/20] hw/riscv: set machine->fdt in spike_board_init()

[Alistair Francis]

On Fri, Aug 5, 2022 at 8:08 PM Daniel Henrique Barboza
<danielhb413@gmail.com> wrote:
>
> 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>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/riscv/spike.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index e41b6aa9f0..2909f7b2a1 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,15 @@ 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' commands. Use fdt_pack() to
> +     * shrink the blob size we're going to store.
> +     */
> +    fdt_pack(s->fdt);
> +    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.36.1
>
>

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

[Alistair Francis]

On Fri, Aug 5, 2022 at 7:59 PM Daniel Henrique Barboza
<danielhb413@gmail.com> wrote:
>
> To save the FDT blob we have the '-machine dumpdtb=<file>' property.
> With this property set, the machine saves the FDT in <file> and exit.
> The created file can then be converted to plain text dts format using
> 'dtc'.
>
> There's nothing particularly sophisticated into saving the FDT that
> can't be done with the machine at any state, as long as the machine has
> a valid FDT to be saved.
>
> The '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.
>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  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..d2554e9701 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:s",
> +        .params     = "[filename] file to save the FDT",
> +        .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..d23ec85f9d 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) {
> +        error_report_err(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.36.1
>
>

Re: [PATCH for-7.2 v2 01/20] hw/arm: do not free machine->fdt in arm_load_dtb()

[David Gibson]

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

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

fdt_pack() could change (reduce) the effective size of the dtb blob,
so returning a 'size' value from above rather than the new value of
fdt_totalsize(fdt) doesn't see right.

I believe some of the other patches in the series have similar concerns.

-- 
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 v2 09/20] hw/ppc: set machine->fdt in pnv_reset()

[David Gibson]

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

On Fri, Aug 05, 2022 at 09:31:11AM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 8/5/22 08:03, Frederic Barrat wrote:
> > 
> > 
> > On 05/08/2022 11:39, Daniel Henrique Barboza wrote:
> > > 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 | 6 +++++-
> > >   1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> > > index d3f77c8367..f5162f8b7b 100644
> > > --- a/hw/ppc/pnv.c
> > > +++ b/hw/ppc/pnv.c
> > > @@ -608,7 +608,11 @@ 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.
> > > +     */
> > > +    machine->fdt = fdt;
> > 
> > 
> > Can pnv_reset() be called several times in the same instance of the qemu process, in which case we leak memory?
> 
> hmmm I think it's possible if we issue a 'system_reset' via the
> monitor.

Right.  I'm not certain about pnv, but on most platforms there's a way
to trigger system_reset from the guest side as well.

> I'll put a g_free(machine->fdt) before the assignment.
> 
> 
> Daniel
> 
> 
> > 
> >    Fred
> > 
> > 
> > >   }
> > >   static ISABus *pnv_chip_power8_isa_create(PnvChip *chip, Error **errp)
> 

-- 
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 v2 10/20] hw/ppc: set machine->fdt in spapr machine

[David Gibson]

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

On Fri, Aug 05, 2022 at 06:39:38AM -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 the two places where we manipulate the FDT:
> spapr_machine_reset() and CAS. spapr->fdt_blob is left untouched: what
> we want is a way to access the FDT from HMP, not replace
> spapr->fdt_blob.

Given there is now an fdt field in the generic MACHINE structure, we
should be able to remove the one in spapr->fdt_blob, yes?

> 
> 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..94c90f0351 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' 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..0079bc6fdc 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'
> +     * 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 v2 14/20] qmp/hmp, device_tree.c: introduce dumpdtb

[David Gibson]

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

On Fri, Aug 05, 2022 at 06:39:42AM -0300, Daniel Henrique Barboza wrote:
> To save the FDT blob we have the '-machine dumpdtb=<file>' property.
> With this property set, the machine saves the FDT in <file> and exit.
> The created file can then be converted to plain text dts format using
> 'dtc'.
> 
> There's nothing particularly sophisticated into saving the FDT that
> can't be done with the machine at any state, as long as the machine has
> a valid FDT to be saved.
> 
> The 'dumpdtb' command receives a 'filename' paramenter and, if a valid
> 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.

On spapr at least, the fdt can change at runtime (due to hotplugs).
So we need to think about concurrency between fdt updates and dumping
it with this command.  I'm assuming it's protected by the BQL, but I'm
wondering if we should outright state that somewhere for clarity.

> 
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.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..d2554e9701 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:s",
> +        .params     = "[filename] file to save the FDT",
> +        .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..d23ec85f9d 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) {
> +        error_report_err(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);
> +}

-- 
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 v2 15/20] qmp/hmp, device_tree.c: introduce 'info fdt' command

[David Gibson]

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

On Fri, Aug 05, 2022 at 06:39:43AM -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.
> 
> '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... it's listing the names of the properties, but not the contents?
That seems kind of odd.

> 
> Cc: 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 d23ec85f9d..accde90380 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -2484,3 +2484,16 @@ void hmp_dumpdtb(Monitor *mon, const QDict *qdict)
>          error_report_err(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' ]  }


A QMP command that returns human readable text, rather than something
JSON structured seems... odd.

Admittedly, *how* you'd JSON structure the results gets a bit tricky.
Listing nodes or property names would be easy enough, but getting the
property contents is hairy, since JSON strings are supposed to be
Unicode, but DT properties can be arbitrary bytestrings.

> diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
> index cd487ddd4d..3fb07b537f 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, "");

So, this lists it in dts format... kind of.  Because you don't include
the property values, it makes it look like all properties are binary
(either absent or present-but-empty).  I think that's misleading.  If
you're only going to list properties, I think you'd be better off
using different formatting (and maybe a more specific command name as
well).

> +}
> +
> +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 v2 16/20] device_tree.c: support string props in fdt_format_node()

[David Gibson]

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

On Fri, Aug 05, 2022 at 06:39:44AM -0300, Daniel Henrique Barboza wrote:
> To support printing string properties in 'info fdt' we need to determine
> whether a void data might contain a string.

Oh... sorry, obviously I hadn't read these later patches when I
complained about the command not printing property values.

> 
> We do that by casting the void data to a string array and:
> 
> - check if the array finishes with a null character
> - check if all characters are printable

This won't handle the case of the "string list" several strings tacked
together, separated by their terminating \0 characters.

> 
> If both conditions are met, we'll consider it to be a string data type
> and print it accordingly. After this change, 'info fdt' is now able to
> print string properties. Here's an example with the ARM 'virt' machine:
> 
> (qemu) info fdt /chosen
> chosen {
>     stdout-path = '/pl011@9000000'
>     rng-seed;
>     kaslr-seed;
> }
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  softmmu/device_tree.c | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
> index 3fb07b537f..8691c3ccc0 100644
> --- a/softmmu/device_tree.c
> +++ b/softmmu/device_tree.c
> @@ -663,6 +663,24 @@ 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(const void *data, int size)
> +{
> +    const char *str = data;
> +    int i;
> +
> +    if (size <= 0 || str[size - 1] != '\0') {
> +        return false;
> +    }
> +
> +    for (i = 0; i < size - 1; i++) {
> +        if (!g_ascii_isprint(str[i])) {
> +            return false;
> +        }
> +    }
> +
> +    return true;
> +}
> +
>  static void fdt_format_node(GString *buf, int node, int depth)
>  {
>      const struct fdt_property *prop = NULL;
> @@ -681,7 +699,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(prop->data, prop_size)) {
> +            g_string_append_printf(buf, "%*s%s = '%s'\n",
> +                                   padding, "", propname, prop->data);

If you're going for dts like output, I'd suggest going all the way.
That means \" instead of \' and a ';' at the end of the line.

> +        } else {
> +            g_string_append_printf(buf, "%*s%s;\n", padding, "", propname);
> +        }
>      }
>  
>      padding -= 4;

-- 
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 v2 17/20] device_tree.c: support remaining FDT prop types

[David Gibson]

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

On Fri, Aug 05, 2022 at 06:39:45AM -0300, Daniel Henrique Barboza wrote:
> When printing a blob with 'dtc' using the '-O dts' option there are 3
> distinct data types being printed: strings, arrays of uint32s and
> regular byte arrays.
> 
> Previous patch added support to print strings. Let's add the remaining
> formats. We want to resemble the format that 'dtc -O dts' uses, so every
> uint32 array uses angle brackets (<>), and regular byte array uses square
> brackets ([]). For properties that has no values we keep printing just
> its name.
> 
> The /chosen FDT node from the pSeris machine gives an example of all
> property types 'info fdt' is now able to display:
> 
> (qemu) info fdt /chosen
> chosen {
>     ibm,architecture-vec-5 = [0 0]
>     rng-seed = <0x5967a270 0x62b0fb4f 0x8262b46a 0xabf48423 0xcce9615 0xf9daae64 0x66564790 0x357d1604>
>     ibm,arch-vec-5-platform-support = <0x178018c0 0x19001a40>
>     linux,pci-probe-only = <0x0>
>     stdout-path = '/vdevice/vty@71000000'
>     linux,stdout-path = '/vdevice/vty@71000000'
>     qemu,graphic-depth = <0x20>
>     qemu,graphic-height = <0x258>
>     qemu,graphic-width = <0x320>
> }
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  softmmu/device_tree.c | 58 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 57 insertions(+), 1 deletion(-)
> 
> diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
> index 8691c3ccc0..9d95e4120b 100644
> --- a/softmmu/device_tree.c
> +++ b/softmmu/device_tree.c
> @@ -681,6 +681,53 @@ static bool fdt_prop_is_string(const void *data, int size)
>      return true;
>  }
>  
> +static bool fdt_prop_is_uint32_array(int size)
> +{
> +    return size % 4 == 0;
> +}
> +
> +static void fdt_prop_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");

Add a ';' to match dts.

> +}
> +
> +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, "%x", val[i]);

For dts like output you need %02x.  In [] notation, the spaces are
actually optional and ignored, so [0 0] is equivalent to [00], which is
equivalent to "".

> +
> +        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;
> @@ -699,11 +746,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(prop->data, prop_size)) {
>              g_string_append_printf(buf, "%*s%s = '%s'\n",
>                                     padding, "", propname, prop->data);
> +        } 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);
>          }
>      }
>  

-- 
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 v2 09/20] hw/ppc: set machine->fdt in pnv_reset()

[Cédric Le Goater]

On 8/5/22 11:39, Daniel Henrique Barboza wrote:
> This will enable support for 'dumpdtb' and 'info fdt' HMP commands for
> all powernv machines.

I might have missed some emails but dumpdtb is already suppported :
commit 8d4092614161 ("ppc/pnv: activate the "dumpdtb" option on the
powernv machine")

Thanks,

C.


> 
> 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 | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index d3f77c8367..f5162f8b7b 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -608,7 +608,11 @@ 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.
> +     */
> +    machine->fdt = fdt;
>   }
>   
>   static ISABus *pnv_chip_power8_isa_create(PnvChip *chip, Error **errp)

Re: [PATCH for-7.2 v2 09/20] hw/ppc: set machine->fdt in pnv_reset()

[Cédric Le Goater]

On 8/8/22 08:47, Cédric Le Goater wrote:
> On 8/5/22 11:39, Daniel Henrique Barboza wrote:
>> This will enable support for 'dumpdtb' and 'info fdt' HMP commands for
>> all powernv machines.
> 
> I might have missed some emails but dumpdtb is already suppported :
> commit 8d4092614161 ("ppc/pnv: activate the "dumpdtb" option on the
> powernv machine")

ok. found the patchset "QMP/HMP: add 'dumpdtb' and 'info fdt' commands"

'info fdt' would have been of great help when we were developing the
PowerNV machine. Initially, I was even using pmemsave to extract the
FDT blob ... So this is a great idea ! (which needs a g_free() )

Do we have something similar to dump ACPI tables, btw ?

Thanks,
  
C.


>>
>> 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 | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index d3f77c8367..f5162f8b7b 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -608,7 +608,11 @@ 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.
>> +     */
>> +    machine->fdt = fdt;
>>   }
>>   static ISABus *pnv_chip_power8_isa_create(PnvChip *chip, Error **errp)
> 

Re: [PATCH for-7.2 v2 01/20] hw/arm: do not free machine->fdt in arm_load_dtb()

[Daniel Henrique Barboza]

On 8/8/22 00:23, David Gibson wrote:
> On Fri, Aug 05, 2022 at 06:39:29AM -0300, Daniel Henrique Barboza wrote:
>> At this moment, arm_load_dtb() can free machine->fdt when
>> binfo->dtb_filename is NULL. If there's no 'dtb_filename', 'fdt' will be
>> retrieved by binfo->get_dtb(). If get_dtb() returns machine->fdt, as is
>> the case of machvirt_dtb() from hw/arm/virt.c, fdt now has a pointer to
>> machine->fdt. And, in that case, the existing g_free(fdt) at the end of
>> arm_load_dtb() will make machine->fdt point to an invalid memory region.
>>
>> This is not an issue right now because there's no code that access
>> machine->fdt after arm_load_dtb(), but we're going to add a couple do
>> FDT HMP commands that will rely on machine->fdt being valid.
>>
>> Instead of freeing 'fdt' at the end of arm_load_dtb(), assign it to
>> machine->fdt. This will allow the FDT of ARM machines that relies on
>> arm_load_dtb() to be accessed later on.
>>
>> Since all ARM machines allocates the FDT only once, we don't need to
>> worry about leaking the existing FDT during a machine reset (which is
>> something that other machines have to look after, e.g. the ppc64 pSeries
>> machine).
>>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: qemu-arm@nongnu.org
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   hw/arm/boot.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>> index ada2717f76..9f5ceb62d2 100644
>> --- a/hw/arm/boot.c
>> +++ b/hw/arm/boot.c
>> @@ -684,7 +684,13 @@ 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' commands. Use fdt_pack() to shrink the blob
>> +     * size we're going to store.
>> +     */
>> +    fdt_pack(fdt);
>> +    ms->fdt = fdt;
>>   
>>       return size;
> 
> fdt_pack() could change (reduce) the effective size of the dtb blob,
> so returning a 'size' value from above rather than the new value of
> fdt_totalsize(fdt) doesn't see right.
> 
> I believe some of the other patches in the series have similar concerns.

Ok! I'll revisit those patches and be sure to return the updated value
of the fdt.


Thanks,


Daniel

> 

Re: [PATCH for-7.2 v2 09/20] hw/ppc: set machine->fdt in pnv_reset()

[Daniel Henrique Barboza]

On 8/8/22 04:13, Cédric Le Goater wrote:
> On 8/8/22 08:47, Cédric Le Goater wrote:
>> On 8/5/22 11:39, Daniel Henrique Barboza wrote:
>>> This will enable support for 'dumpdtb' and 'info fdt' HMP commands for
>>> all powernv machines.
>>
>> I might have missed some emails but dumpdtb is already suppported :
>> commit 8d4092614161 ("ppc/pnv: activate the "dumpdtb" option on the
>> powernv machine")
> 
> ok. found the patchset "QMP/HMP: add 'dumpdtb' and 'info fdt' commands"
> 
> 'info fdt' would have been of great help when we were developing the
> PowerNV machine. Initially, I was even using pmemsave to extract the
> FDT blob ... So this is a great idea ! (which needs a g_free() )
> 
> Do we have something similar to dump ACPI tables, btw ?

In QEMU? No idea. I didn't find users of libfdt in x86 files so I didn't
bothered checking.

I am aware of something you can do in userland to dump the ACPI tables. I
did it once for research when I was working in the NUMA FORM2 extension
for pseries. This is the procedure do dump the ACPI SLIT table:


danielhb@ubuntu-vm:~$ sudo acpidump > acpidata.dat
[sudo] password for danielhb:
danielhb@ubuntu-vm:~$
danielhb@ubuntu-vm:~$ sudo acpixtract -sSLIT acpidata.dat

Intel ACPI Component Architecture
ACPI Binary Table Extraction Utility version 20200925
Copyright (c) 2000 - 2020 Intel Corporation

   SLIT -      60 bytes written (0x0000003C) - slit.dat
danielhb@ubuntu-vm:~$
danielhb@ubuntu-vm:~$ iasl -d slit.dat

Intel ACPI Component Architecture
ASL+ Optimizing Compiler/Disassembler version 20200925
Copyright (c) 2000 - 2020 Intel Corporation

File appears to be binary: found 24 non-ASCII characters, disassembling
Binary file appears to be a valid ACPI table, disassembling
Input file slit.dat, Length 0x3C (60) bytes
ACPI: SLIT 0x0000000000000000 00003C (v01 BOCHS  BXPCSLIT 00000001 BXPC 00000001)
Acpi Data Table [SLIT] decoded
Formatted output:  slit.dsl - 1489 bytes
danielhb@ubuntu-vm:~$
danielhb@ubuntu-vm:~$ cat slit.dsl
/*
  * Intel ACPI Component Architecture
  * AML/ASL+ Disassembler version 20200925 (64-bit version)
  * Copyright (c) 2000 - 2020 Intel Corporation
  *
  * Disassembly of slit.dat, Wed Jun  2 19:00:54 2021
  *
  * ACPI Data Table [SLIT]
  *
  * Format: [HexOffset DecimalOffset ByteLength]  FieldName : FieldValue
  */

[000h 0000   4]                    Signature : "SLIT"    [System Locality Information Table]
[004h 0004   4]                 Table Length : 0000003C
[008h 0008   1]                     Revision : 01
[009h 0009   1]                     Checksum : A0
[00Ah 0010   6]                       Oem ID : "BOCHS "
[010h 0016   8]                 Oem Table ID : "BXPCSLIT"
[018h 0024   4]                 Oem Revision : 00000001
[01Ch 0028   4]              Asl Compiler ID : "BXPC"
[020h 0032   4]        Asl Compiler Revision : 00000001

[024h 0036   8]                   Localities : 0000000000000004
[02Ch 0044   4]                 Locality   0 : 0A 16 16 16
[030h 0048   4]                 Locality   1 : 2C 0A 2C 2C
[034h 0052   4]                 Locality   2 : 42 42 0A 42
[038h 0056   4]                 Locality   3 : 58 58 58 0A

Raw Table Data: Length 60 (0x3C)

     0000: 53 4C 49 54 3C 00 00 00 01 A0 42 4F 43 48 53 20  // SLIT<.....BOCHS
     0010: 42 58 50 43 53 4C 49 54 01 00 00 00 42 58 50 43  // BXPCSLIT....BXPC
     0020: 01 00 00 00 04 00 00 00 00 00 00 00 0A 16 16 16  // ................
     0030: 2C 0A 2C 2C 42 42 0A 42 58 58 58 0A              // ,.,,BB.BXXX.
danielhb@ubuntu-vm:~$


So basically a combination of acpidump and acpixtract commands in the guest.


Daniel


> 
> Thanks,
> 
> C.
> 
> 
>>>
>>> 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 | 6 +++++-
>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>>> index d3f77c8367..f5162f8b7b 100644
>>> --- a/hw/ppc/pnv.c
>>> +++ b/hw/ppc/pnv.c
>>> @@ -608,7 +608,11 @@ 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.
>>> +     */
>>> +    machine->fdt = fdt;
>>>   }
>>>   static ISABus *pnv_chip_power8_isa_create(PnvChip *chip, Error **errp)
>>
> 

Re: [PATCH for-7.2 v2 16/20] device_tree.c: support string props in fdt_format_node()

[Daniel Henrique Barboza]

On 8/8/22 01:36, David Gibson wrote:
> On Fri, Aug 05, 2022 at 06:39:44AM -0300, Daniel Henrique Barboza wrote:
>> To support printing string properties in 'info fdt' we need to determine
>> whether a void data might contain a string.
> 
> Oh... sorry, obviously I hadn't read these later patches when I
> complained about the command not printing property values.
> 
>>
>> We do that by casting the void data to a string array and:
>>
>> - check if the array finishes with a null character
>> - check if all characters are printable
> 
> This won't handle the case of the "string list" several strings tacked
> together, separated by their terminating \0 characters.

Hmmmm how is this printed? Should we concatenate them? Replace the \0
with a whitespace? Or ignore the zero and concatenate them?

E.g. this is a\0string\0list

Should we print it as:

this is astringlist

or

this is a string list ?


Thanks,


Daniel


> 
>>
>> If both conditions are met, we'll consider it to be a string data type
>> and print it accordingly. After this change, 'info fdt' is now able to
>> print string properties. Here's an example with the ARM 'virt' machine:
>>
>> (qemu) info fdt /chosen
>> chosen {
>>      stdout-path = '/pl011@9000000'
>>      rng-seed;
>>      kaslr-seed;
>> }
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   softmmu/device_tree.c | 25 ++++++++++++++++++++++++-
>>   1 file changed, 24 insertions(+), 1 deletion(-)
>>
>> diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
>> index 3fb07b537f..8691c3ccc0 100644
>> --- a/softmmu/device_tree.c
>> +++ b/softmmu/device_tree.c
>> @@ -663,6 +663,24 @@ 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(const void *data, int size)
>> +{
>> +    const char *str = data;
>> +    int i;
>> +
>> +    if (size <= 0 || str[size - 1] != '\0') {
>> +        return false;
>> +    }
>> +
>> +    for (i = 0; i < size - 1; i++) {
>> +        if (!g_ascii_isprint(str[i])) {
>> +            return false;
>> +        }
>> +    }
>> +
>> +    return true;
>> +}
>> +
>>   static void fdt_format_node(GString *buf, int node, int depth)
>>   {
>>       const struct fdt_property *prop = NULL;
>> @@ -681,7 +699,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(prop->data, prop_size)) {
>> +            g_string_append_printf(buf, "%*s%s = '%s'\n",
>> +                                   padding, "", propname, prop->data);
> 
> If you're going for dts like output, I'd suggest going all the way.
> That means \" instead of \' and a ';' at the end of the line.
> 
>> +        } else {
>> +            g_string_append_printf(buf, "%*s%s;\n", padding, "", propname);
>> +        }
>>       }
>>   
>>       padding -= 4;
> 

Re: [PATCH for-7.2 v2 16/20] device_tree.c: support string props in fdt_format_node()

[David Gibson]

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

On Wed, Aug 10, 2022 at 04:40:18PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 8/8/22 01:36, David Gibson wrote:
> > On Fri, Aug 05, 2022 at 06:39:44AM -0300, Daniel Henrique Barboza wrote:
> > > To support printing string properties in 'info fdt' we need to determine
> > > whether a void data might contain a string.
> > 
> > Oh... sorry, obviously I hadn't read these later patches when I
> > complained about the command not printing property values.
> > 
> > > 
> > > We do that by casting the void data to a string array and:
> > > 
> > > - check if the array finishes with a null character
> > > - check if all characters are printable
> > 
> > This won't handle the case of the "string list" several strings tacked
> > together, separated by their terminating \0 characters.
> 
> Hmmmm how is this printed? Should we concatenate them? Replace the \0
> with a whitespace? Or ignore the zero and concatenate them?
> 
> E.g. this is a\0string\0list
> 
> Should we print it as:
> 
> this is astringlist
> 
> or
> 
> this is a string list ?

Well, if you're going for dts like output, which you seem to be, you
have two options:

1) Escape the medial nulls

	"this\0is\0a\0string\0list"

2) Multiple strings:

	"this", "is", "a", "string", "list"

Both forms are allowed in dts and will result in an identical
bytestring in the property.

> > > If both conditions are met, we'll consider it to be a string data type
> > > and print it accordingly. After this change, 'info fdt' is now able to
> > > print string properties. Here's an example with the ARM 'virt' machine:
> > > 
> > > (qemu) info fdt /chosen
> > > chosen {
> > >      stdout-path = '/pl011@9000000'
> > >      rng-seed;
> > >      kaslr-seed;
> > > }
> > > 
> > > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > > ---
> > >   softmmu/device_tree.c | 25 ++++++++++++++++++++++++-
> > >   1 file changed, 24 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
> > > index 3fb07b537f..8691c3ccc0 100644
> > > --- a/softmmu/device_tree.c
> > > +++ b/softmmu/device_tree.c
> > > @@ -663,6 +663,24 @@ 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(const void *data, int size)
> > > +{
> > > +    const char *str = data;
> > > +    int i;
> > > +
> > > +    if (size <= 0 || str[size - 1] != '\0') {
> > > +        return false;
> > > +    }
> > > +
> > > +    for (i = 0; i < size - 1; i++) {
> > > +        if (!g_ascii_isprint(str[i])) {
> > > +            return false;
> > > +        }
> > > +    }
> > > +
> > > +    return true;
> > > +}
> > > +
> > >   static void fdt_format_node(GString *buf, int node, int depth)
> > >   {
> > >       const struct fdt_property *prop = NULL;
> > > @@ -681,7 +699,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(prop->data, prop_size)) {
> > > +            g_string_append_printf(buf, "%*s%s = '%s'\n",
> > > +                                   padding, "", propname, prop->data);
> > 
> > If you're going for dts like output, I'd suggest going all the way.
> > That means \" instead of \' and a ';' at the end of the line.
> > 
> > > +        } else {
> > > +            g_string_append_printf(buf, "%*s%s;\n", padding, "", propname);
> > > +        }
> > >       }
> > >       padding -= 4;
> > 
> 

-- 
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 v2 01/20] hw/arm: do not free machine->fdt in arm_load_dtb()

[Daniel Henrique Barboza]

David,

On 8/8/22 00:23, David Gibson wrote:
> On Fri, Aug 05, 2022 at 06:39:29AM -0300, Daniel Henrique Barboza wrote:
>> At this moment, arm_load_dtb() can free machine->fdt when
>> binfo->dtb_filename is NULL. If there's no 'dtb_filename', 'fdt' will be
>> retrieved by binfo->get_dtb(). If get_dtb() returns machine->fdt, as is
>> the case of machvirt_dtb() from hw/arm/virt.c, fdt now has a pointer to
>> machine->fdt. And, in that case, the existing g_free(fdt) at the end of
>> arm_load_dtb() will make machine->fdt point to an invalid memory region.
>>
>> This is not an issue right now because there's no code that access
>> machine->fdt after arm_load_dtb(), but we're going to add a couple do
>> FDT HMP commands that will rely on machine->fdt being valid.
>>
>> Instead of freeing 'fdt' at the end of arm_load_dtb(), assign it to
>> machine->fdt. This will allow the FDT of ARM machines that relies on
>> arm_load_dtb() to be accessed later on.
>>
>> Since all ARM machines allocates the FDT only once, we don't need to
>> worry about leaking the existing FDT during a machine reset (which is
>> something that other machines have to look after, e.g. the ppc64 pSeries
>> machine).
>>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: qemu-arm@nongnu.org
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   hw/arm/boot.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>> index ada2717f76..9f5ceb62d2 100644
>> --- a/hw/arm/boot.c
>> +++ b/hw/arm/boot.c
>> @@ -684,7 +684,13 @@ 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' commands. Use fdt_pack() to shrink the blob
>> +     * size we're going to store.
>> +     */
>> +    fdt_pack(fdt);
>> +    ms->fdt = fdt;
>>   
>>       return size;
> 
> fdt_pack() could change (reduce) the effective size of the dtb blob,
> so returning a 'size' value from above rather than the new value of
> fdt_totalsize(fdt) doesn't see right.

After some thought I think executing fdt_pack() like I'm doing here is not
a good idea. The first problem is that I'm not returning the updated size,
as you've said.

But I can't just amend a 'return fdt_totalsize(fdt);' either. I'm packing the
FDT **after** the machine store it in the guest physical memory. If I return
the packed size, but the machine isn't packing the FDT before a
cpu_physical_memory_write(), I'll be under-reporting the FDT size written.

Machines such as e500 (patch 4) uses this returned value to put more stuff in
the guest memory. In that case, returning a smaller size that what was actually
written can cause the machine to overwrite the FDT by accident. In fact, only
a handful of machines (ppc/pseries, ppc/pvn, riscv, oepenrisc) is doing a
fdt_pack() before a cpu_physical_memory_write(). So this change would be
potentially harmful to a lot of people.

One alternative would be to do a fdt_pack() before the machine writes the
FDT in the guest memory, but that is too intrusive to do because I can't say
if each of these machines will be OK with that. I have a hunch that it would
be OK, but a hunch isn't going to cut it.

I'll just drop the fdt_pack() for each of these patches. If the machine code
is already packing it, fine. If not, that's fine too. Each maintainer can
assess whether packing the FDT is worth it or not.


Thanks,


Daniel


> 
> I believe some of the other patches in the series have similar concerns.
> 

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

[Daniel Henrique Barboza]

On 8/8/22 00:26, David Gibson wrote:
> On Fri, Aug 05, 2022 at 06:39:38AM -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 the two places where we manipulate the FDT:
>> spapr_machine_reset() and CAS. spapr->fdt_blob is left untouched: what
>> we want is a way to access the FDT from HMP, not replace
>> spapr->fdt_blob.
> 
> Given there is now an fdt field in the generic MACHINE structure, we
> should be able to remove the one in spapr->fdt_blob, yes?

I thought about it but I backed down when I realized that spapr->fdt_blob is
being migrated.

At first glance it would be a matter of migrating ms->fdt and then removing
spapr->fdt_blob, but then I got confused about how a migration to an older
version would occur (e.g. QEMU 7.2 with ms->fdt to a QEMU 7.0 with
spapr->fdt_blob).

Migration to a newer QEMU would require us to move the spapr->version_id to 4
and then handle the old version accordingly in spapr_post_load().

Probably something to think about after this work is accepted.


Thanks,


Daniel



> 
>>
>> 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..94c90f0351 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' 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..0079bc6fdc 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'
>> +     * HMP commands.
>> +     */
>> +    MACHINE(spapr)->fdt = fdt;
>> +
>>       return H_SUCCESS;
>>   }
>>   
> 

Re: [PATCH for-7.2 v2 01/20] hw/arm: do not free machine->fdt in arm_load_dtb()

[David Gibson]

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

On Fri, Aug 12, 2022 at 07:03:26PM -0300, Daniel Henrique Barboza wrote:
> David,
> 
> On 8/8/22 00:23, David Gibson wrote:
> > On Fri, Aug 05, 2022 at 06:39:29AM -0300, Daniel Henrique Barboza wrote:
> > > At this moment, arm_load_dtb() can free machine->fdt when
> > > binfo->dtb_filename is NULL. If there's no 'dtb_filename', 'fdt' will be
> > > retrieved by binfo->get_dtb(). If get_dtb() returns machine->fdt, as is
> > > the case of machvirt_dtb() from hw/arm/virt.c, fdt now has a pointer to
> > > machine->fdt. And, in that case, the existing g_free(fdt) at the end of
> > > arm_load_dtb() will make machine->fdt point to an invalid memory region.
> > > 
> > > This is not an issue right now because there's no code that access
> > > machine->fdt after arm_load_dtb(), but we're going to add a couple do
> > > FDT HMP commands that will rely on machine->fdt being valid.
> > > 
> > > Instead of freeing 'fdt' at the end of arm_load_dtb(), assign it to
> > > machine->fdt. This will allow the FDT of ARM machines that relies on
> > > arm_load_dtb() to be accessed later on.
> > > 
> > > Since all ARM machines allocates the FDT only once, we don't need to
> > > worry about leaking the existing FDT during a machine reset (which is
> > > something that other machines have to look after, e.g. the ppc64 pSeries
> > > machine).
> > > 
> > > Cc: Peter Maydell <peter.maydell@linaro.org>
> > > Cc: qemu-arm@nongnu.org
> > > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > > ---
> > >   hw/arm/boot.c | 8 +++++++-
> > >   1 file changed, 7 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> > > index ada2717f76..9f5ceb62d2 100644
> > > --- a/hw/arm/boot.c
> > > +++ b/hw/arm/boot.c
> > > @@ -684,7 +684,13 @@ 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' commands. Use fdt_pack() to shrink the blob
> > > +     * size we're going to store.
> > > +     */
> > > +    fdt_pack(fdt);
> > > +    ms->fdt = fdt;
> > >       return size;
> > 
> > fdt_pack() could change (reduce) the effective size of the dtb blob,
> > so returning a 'size' value from above rather than the new value of
> > fdt_totalsize(fdt) doesn't see right.
> 
> After some thought I think executing fdt_pack() like I'm doing here is not
> a good idea. The first problem is that I'm not returning the updated size,
> as you've said.
> 
> But I can't just amend a 'return fdt_totalsize(fdt);' either. I'm packing the
> FDT **after** the machine store it in the guest physical memory. If I return
> the packed size, but the machine isn't packing the FDT before a
> cpu_physical_memory_write(), I'll be under-reporting the FDT size written.

Ah... good point.

> Machines such as e500 (patch 4) uses this returned value to put more stuff in
> the guest memory. In that case, returning a smaller size that what was actually
> written can cause the machine to overwrite the FDT by accident. In fact, only
> a handful of machines (ppc/pseries, ppc/pvn, riscv, oepenrisc) is doing a
> fdt_pack() before a cpu_physical_memory_write(). So this change would be
> potentially harmful to a lot of people.
> 
> One alternative would be to do a fdt_pack() before the machine writes the
> FDT in the guest memory, but that is too intrusive to do because I can't say
> if each of these machines will be OK with that. I have a hunch that it would
> be OK, but a hunch isn't going to cut it.

Hmm.. I'd be fairly confident that would be ok.  It certainly should
be ok for the fdt content itself, fdt_pack() doesn't change that
semantically.  If the machine is using that size to put stuff after, I
can't really see how that could break, either.  Unless the machine
were using the fdtsize in one place and a fixed size somewhere else to
address the same data, which would be a bug.

> I'll just drop the fdt_pack() for each of these patches. If the machine code
> is already packing it, fine. If not, that's fine too. Each maintainer can
> assess whether packing the FDT is worth it or not.

That's probably reasonable for the time being.

-- 
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 v2 10/20] hw/ppc: set machine->fdt in spapr machine

[David Gibson]

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

On Fri, Aug 12, 2022 at 07:23:09PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 8/8/22 00:26, David Gibson wrote:
> > On Fri, Aug 05, 2022 at 06:39:38AM -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 the two places where we manipulate the FDT:
> > > spapr_machine_reset() and CAS. spapr->fdt_blob is left untouched: what
> > > we want is a way to access the FDT from HMP, not replace
> > > spapr->fdt_blob.
> > 
> > Given there is now an fdt field in the generic MACHINE structure, we
> > should be able to remove the one in spapr->fdt_blob, yes?
> 
> I thought about it but I backed down when I realized that spapr->fdt_blob is
> being migrated.
> 
> At first glance it would be a matter of migrating ms->fdt and then removing
> spapr->fdt_blob, but then I got confused about how a migration to an older
> version would occur (e.g. QEMU 7.2 with ms->fdt to a QEMU 7.0 with
> spapr->fdt_blob).
> 
> Migration to a newer QEMU would require us to move the spapr->version_id to 4
> and then handle the old version accordingly in spapr_post_load().
> 
> Probably something to think about after this work is accepted.

Fair enough.  I'm confident the migration semantics can be worked out,
but it will require some care.

-- 
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 v2 14/20] qmp/hmp, device_tree.c: introduce dumpdtb

[Daniel Henrique Barboza]

On 8/8/22 00:30, David Gibson wrote:
> On Fri, Aug 05, 2022 at 06:39:42AM -0300, Daniel Henrique Barboza wrote:
>> To save the FDT blob we have the '-machine dumpdtb=<file>' property.
>> With this property set, the machine saves the FDT in <file> and exit.
>> The created file can then be converted to plain text dts format using
>> 'dtc'.
>>
>> There's nothing particularly sophisticated into saving the FDT that
>> can't be done with the machine at any state, as long as the machine has
>> a valid FDT to be saved.
>>
>> The 'dumpdtb' command receives a 'filename' paramenter and, if a valid
>> 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.
> 
> On spapr at least, the fdt can change at runtime (due to hotplugs).
> So we need to think about concurrency between fdt updates and dumping
> it with this command.  I'm assuming it's protected by the BQL, but I'm
> wondering if we should outright state that somewhere for clarity.

Your assumption is right. It is protected by BQL since it's always executed
in-band. To not hold BQL we would need to declare an extra flag "allow-oob"
to execute the command out-of-band.

I'll mention in the commit msg that we're holding BQL. I'll do the same for
the 'info fdt' commit msg.


Thanks,


Daniel


> 
>>
>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.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..d2554e9701 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:s",
>> +        .params     = "[filename] file to save the FDT",
>> +        .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..d23ec85f9d 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) {
>> +        error_report_err(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);
>> +}
> 

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

[Dr. David Alan Gilbert]

* Daniel Henrique Barboza (danielhb413@gmail.com) wrote:
> 
> 
> On 8/8/22 00:30, David Gibson wrote:
> > On Fri, Aug 05, 2022 at 06:39:42AM -0300, Daniel Henrique Barboza wrote:
> > > To save the FDT blob we have the '-machine dumpdtb=<file>' property.
> > > With this property set, the machine saves the FDT in <file> and exit.
> > > The created file can then be converted to plain text dts format using
> > > 'dtc'.
> > > 
> > > There's nothing particularly sophisticated into saving the FDT that
> > > can't be done with the machine at any state, as long as the machine has
> > > a valid FDT to be saved.
> > > 
> > > The 'dumpdtb' command receives a 'filename' paramenter and, if a valid
> > > 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.
> > 
> > On spapr at least, the fdt can change at runtime (due to hotplugs).
> > So we need to think about concurrency between fdt updates and dumping
> > it with this command.  I'm assuming it's protected by the BQL, but I'm
> > wondering if we should outright state that somewhere for clarity.
> 
> Your assumption is right. It is protected by BQL since it's always executed
> in-band. To not hold BQL we would need to declare an extra flag "allow-oob"
> to execute the command out-of-band.
> 
> I'll mention in the commit msg that we're holding BQL. I'll do the same for
> the 'info fdt' commit msg.

Is the update of the FDT also protected that way?
(I guess hotplug commands are also protected by the bql - but is the
hotplug completion?)

Dave
> 
> Thanks,
> 
> 
> Daniel
> 
> 
> > 
> > > 
> > > Cc: Dr. David Alan Gilbert <dgilbert@redhat.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..d2554e9701 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:s",
> > > +        .params     = "[filename] file to save the FDT",
> > > +        .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..d23ec85f9d 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) {
> > > +        error_report_err(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);
> > > +}
> > 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Dr. David Alan Gilbert]

* Daniel Henrique Barboza (danielhb413@gmail.com) wrote:
> 'info fdt' is only able to print full nodes so far. It would be good to
> be able to also print single properties, since ometimes we just want
> to verify a single value from the FDT.
> 
> libfdt does not have support to find a property given its full path, but
> it does have a way to return a fdt_property given a prop name and its
> subnode.
> 
> 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>
> 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,
>      },

Yeh that seems easier to me; from HMP:


Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

>  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 accde90380..df8493adc5 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 902a7f680b..be7b7e297e 100644
> --- a/softmmu/device_tree.c
> +++ b/softmmu/device_tree.c
> @@ -785,23 +785,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.36.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Daniel Henrique Barboza]

On 8/15/22 15:31, Dr. David Alan Gilbert wrote:
> * Daniel Henrique Barboza (danielhb413@gmail.com) wrote:
>>
>>
>> On 8/8/22 00:30, David Gibson wrote:
>>> On Fri, Aug 05, 2022 at 06:39:42AM -0300, Daniel Henrique Barboza wrote:
>>>> To save the FDT blob we have the '-machine dumpdtb=<file>' property.
>>>> With this property set, the machine saves the FDT in <file> and exit.
>>>> The created file can then be converted to plain text dts format using
>>>> 'dtc'.
>>>>
>>>> There's nothing particularly sophisticated into saving the FDT that
>>>> can't be done with the machine at any state, as long as the machine has
>>>> a valid FDT to be saved.
>>>>
>>>> The 'dumpdtb' command receives a 'filename' paramenter and, if a valid
>>>> 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.
>>>
>>> On spapr at least, the fdt can change at runtime (due to hotplugs).
>>> So we need to think about concurrency between fdt updates and dumping
>>> it with this command.  I'm assuming it's protected by the BQL, but I'm
>>> wondering if we should outright state that somewhere for clarity.
>>
>> Your assumption is right. It is protected by BQL since it's always executed
>> in-band. To not hold BQL we would need to declare an extra flag "allow-oob"
>> to execute the command out-of-band.
>>
>> I'll mention in the commit msg that we're holding BQL. I'll do the same for
>> the 'info fdt' commit msg.
> 
> Is the update of the FDT also protected that way?

Yes. At least for the pseries machine, the only machine we have ATM that updates
the FDT after boot, without hotplug/unplug, and uses libfdt.

The update of the pseries FDT occurs in the h_client_architecture_support hcall,
which is always called holding BQL (either via kvm_arch_handle_exit() or
emulate_spapr_hypercall).

> (I guess hotplug commands are also protected by the bql - but is the
> hotplug completion?)

Hotplug completion is protected by BQL in all machines, as far as I know.


Thanks,


Daniel

> 
> Dave
>>
>> Thanks,
>>
>>
>> Daniel
>>
>>
>>>
>>>>
>>>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.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..d2554e9701 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:s",
>>>> +        .params     = "[filename] file to save the FDT",
>>>> +        .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..d23ec85f9d 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) {
>>>> +        error_report_err(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);
>>>> +}
>>>
>>

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

[Daniel Henrique Barboza]

On 8/8/22 01:21, David Gibson wrote:
> On Fri, Aug 05, 2022 at 06:39:43AM -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.
>>
>> '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... it's listing the names of the properties, but not the contents?
> That seems kind of odd.
> 
>>
>> Cc: 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 d23ec85f9d..accde90380 100644
>> --- a/monitor/hmp-cmds.c
>> +++ b/monitor/hmp-cmds.c
>> @@ -2484,3 +2484,16 @@ void hmp_dumpdtb(Monitor *mon, const QDict *qdict)
>>           error_report_err(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' ]  }
> 
> 
> A QMP command that returns human readable text, rather than something
> JSON structured seems... odd.
> 
> Admittedly, *how* you'd JSON structure the results gets a bit tricky.
> Listing nodes or property names would be easy enough, but getting the
> property contents is hairy, since JSON strings are supposed to be
> Unicode, but DT properties can be arbitrary bytestrings.
> 
>> diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
>> index cd487ddd4d..3fb07b537f 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, "");
> 
> So, this lists it in dts format... kind of.  Because you don't include
> the property values, it makes it look like all properties are binary
> (either absent or present-but-empty).  I think that's misleading.  If
> you're only going to list properties, I think you'd be better off
> using different formatting (and maybe a more specific command name as
> well).

As you've already noticed in the next patch, I decided to split between QMP/HMP
introduction and the dts formatting to avoid clogging everything in a single patch.
In the end the whole fdt tree can be printed with all the properties types.

As for using HumanReadableText, I tried to imagine a structured output for
'info fdt'. It would be something like:

- struct Property: an union of the possible types (none, string, uint32 array,
byte array)

- struct Node: an array of properties and subnodes

And then 'info fdt <node>' would return a struct node and  'info fdt <node> <prop>'
would return a struct Property

Adding this stable ABI sounded like too much extra work for a command that would be
used mostly for debug/development purposes. Every other command that outputs
internal guest state (info roms, info rdma, info irq, info numa ...) are happy with
returning HumanReadableFormat and being considered debug only. I decided to do the
same thing.


Thanks,

Daniel



> 
>> +}
>> +
>> +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 v2 15/20] qmp/hmp, device_tree.c: introduce 'info fdt' command

[David Gibson]

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

On Mon, Aug 15, 2022 at 07:48:14PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 8/8/22 01:21, David Gibson wrote:
> > On Fri, Aug 05, 2022 at 06:39:43AM -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.
> > > 
> > > '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... it's listing the names of the properties, but not the contents?
> > That seems kind of odd.
> > 
> > > 
> > > Cc: 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 d23ec85f9d..accde90380 100644
> > > --- a/monitor/hmp-cmds.c
> > > +++ b/monitor/hmp-cmds.c
> > > @@ -2484,3 +2484,16 @@ void hmp_dumpdtb(Monitor *mon, const QDict *qdict)
> > >           error_report_err(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' ]  }
> > 
> > 
> > A QMP command that returns human readable text, rather than something
> > JSON structured seems... odd.
> > 
> > Admittedly, *how* you'd JSON structure the results gets a bit tricky.
> > Listing nodes or property names would be easy enough, but getting the
> > property contents is hairy, since JSON strings are supposed to be
> > Unicode, but DT properties can be arbitrary bytestrings.
> > 
> > > diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
> > > index cd487ddd4d..3fb07b537f 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, "");
> > 
> > So, this lists it in dts format... kind of.  Because you don't include
> > the property values, it makes it look like all properties are binary
> > (either absent or present-but-empty).  I think that's misleading.  If
> > you're only going to list properties, I think you'd be better off
> > using different formatting (and maybe a more specific command name as
> > well).
> 
> As you've already noticed in the next patch, I decided to split between QMP/HMP
> introduction and the dts formatting to avoid clogging everything in a single patch.
> In the end the whole fdt tree can be printed with all the properties types.
> 
> As for using HumanReadableText, I tried to imagine a structured output for
> 'info fdt'. It would be something like:
> 
> - struct Property: an union of the possible types (none, string, uint32 array,
> byte array)

I don't think it's wise to try to interpret the property any further
than a raw bytestring in a machine oriented interface.  You can't
really know the type of a property without knowing all the DT bindings
that apply to it, what we do for formatting in this patch series, and
in dtc is a heuristic only.  That's useful for debuging output aimed
at humans, but not a good idea for a structured interface.

Note that even for the human interface it might be a better compromise
to just always emit properties essentially as a hexdump.  I wouldn't
suggest approximating dts format in this case though, to make the
distinction clearer.

> - struct Node: an array of properties and subnodes
> 
> And then 'info fdt <node>' would return a struct node and  'info fdt <node> <prop>'
> would return a struct Property

Well, for a "native" QMP interface, I'd suggest splitting up the
operations more.  Say "fdt-ls" (takes a path, returns array of subnode
names), "fdt-properties" (takes a path, returns array of property names), and getprop
(takes a path and a property name, returns bytestring).  I guess a
"dump" operation of some sort to get the whole thing without a million
commands might be a good idea too.

> Adding this stable ABI sounded like too much extra work for a command that would be
> used mostly for debug/development purposes. Every other command that outputs
> internal guest state (info roms, info rdma, info irq, info numa ...) are happy with
> returning HumanReadableFormat and being considered debug only. I decided to do the
> same thing.

Ok, I hadn't realized there was precedent for this approach.  If
that's the case I'm less bothered by this.

-- 
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 v2 10/20] hw/ppc: set machine->fdt in spapr machine

[Alexey Kardashevskiy]

On 05/08/2022 19:39, 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.


Out of curiosity - why new HMP command, is not QOM'ing this ms::fdt 
property enough?

Another thing is that on every HMP dump I'd probably rebuild the entire 
FDT for the reasons David explained. Thanks,


> 
> Let's set machine->fdt in the two places where we manipulate the FDT:
> spapr_machine_reset() and CAS. spapr->fdt_blob is left untouched: what
> we want is a way to access the FDT from HMP, not replace
> spapr->fdt_blob.
> 
> Cc: Cédric Le Goater <clg@kaod.org>
> Cc: qemu-ppc@nongnu.org
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>   hw/ppc/spapr.c       | 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..94c90f0351 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' 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..0079bc6fdc 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'
> +     * HMP commands.
> +     */
> +    MACHINE(spapr)->fdt = fdt;
> +
>       return H_SUCCESS;
>   }
>   

-- 
Alexey

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

[David Gibson]

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

On Fri, Aug 19, 2022 at 12:11:40PM +1000, Alexey Kardashevskiy wrote:
> 
> 
> On 05/08/2022 19:39, 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.
> 
> Out of curiosity - why new HMP command, is not QOM'ing this ms::fdt property
> enough?

Huh.. I didn't think of that.  For dumpdtb you could be right, that
you might be able to use existing qom commands to extract the
property.  Would need to check that the size is is handled properly,
fdt's are a bit weird in having their size "in band".

"info fdt" etc. obviously have additional funtionality in formatting
the contents more helpfully.


> Another thing is that on every HMP dump I'd probably rebuild the entire FDT
> for the reasons David explained. Thanks,

This would require per-machine hooks, however.

-- 
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 v2 10/20] hw/ppc: set machine->fdt in spapr machine

[Daniel Henrique Barboza]

On 8/18/22 23:11, Alexey Kardashevskiy wrote:
> 
> 
> On 05/08/2022 19:39, 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.
> 
> 
> Out of curiosity - why new HMP command, is not QOM'ing this ms::fdt property enough?

I tried to do the minimal changes needed for the commands to work. ms::fdt is
one of the few MachineState fields that hasn't been QOMified by
machine_class_init() yet. All pre-existing code that uses ms::fdt are using the
pointer directly. To make a QOMified use of it would require extra patches
in machine.c to QOMify the property first.

There's also the issue with how each machine is creating the FDT. Most are using
helpers from device_tree.c, some are creating it from scratch, others required
a .dtb file, most of them are not doing a fdt_pack() and so on. To really QOMify
the use of ms::fdt we would need some machine hooks that standardize all that.
I believe it's worth the trouble, but it would be too much to do right now.


Thanks,



Daniel

> 
> Another thing is that on every HMP dump I'd probably rebuild the entire FDT for the reasons David explained. Thanks,
> 
> 
>>
>> Let's set machine->fdt in the two places where we manipulate the FDT:
>> spapr_machine_reset() and CAS. spapr->fdt_blob is left untouched: what
>> we want is a way to access the FDT from HMP, not replace
>> spapr->fdt_blob.
>>
>> Cc: Cédric Le Goater <clg@kaod.org>
>> Cc: qemu-ppc@nongnu.org
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   hw/ppc/spapr.c       | 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..94c90f0351 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' 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..0079bc6fdc 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'
>> +     * HMP commands.
>> +     */
>> +    MACHINE(spapr)->fdt = fdt;
>> +
>>       return H_SUCCESS;
>>   }
> 

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

[David Gibson]

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

On Fri, Aug 19, 2022 at 06:42:34AM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 8/18/22 23:11, Alexey Kardashevskiy wrote:
> > 
> > 
> > On 05/08/2022 19:39, 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.
> > 
> > 
> > Out of curiosity - why new HMP command, is not QOM'ing this ms::fdt property enough?
> 
> I tried to do the minimal changes needed for the commands to work. ms::fdt is
> one of the few MachineState fields that hasn't been QOMified by
> machine_class_init() yet. All pre-existing code that uses ms::fdt are using the
> pointer directly. To make a QOMified use of it would require extra patches
> in machine.c to QOMify the property first.
> 
> There's also the issue with how each machine is creating the FDT. Most are using
> helpers from device_tree.c, some are creating it from scratch, others required
> a .dtb file, most of them are not doing a fdt_pack() and so on. To really QOMify
> the use of ms::fdt we would need some machine hooks that standardize all that.
> I believe it's worth the trouble, but it would be too much to do
> right now.

Hmm.. I think this depends on what you mean by "QOM"ify exactly.  If
you're meaning make the full DT representation QOM objects, that you
can look into in detail, then, yes, that's pretty complicated.

I suspect what Alexey was suggesting though, was merely to make
ms::fdt accessible as a single bytestring property on the machine QOM
object.  Effectively it's just "dumpdtb" but as a property get.

I'm not 100% certain if QOM can safely represent arbitrary bytestrings
as QOM properties, which would need checking.

-- 
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 v2 10/20] hw/ppc: set machine->fdt in spapr machine

[Alexey Kardashevskiy]

On 22/08/2022 13:05, David Gibson wrote:
> On Fri, Aug 19, 2022 at 06:42:34AM -0300, Daniel Henrique Barboza wrote:
>>
>>
>> On 8/18/22 23:11, Alexey Kardashevskiy wrote:
>>>
>>>
>>> On 05/08/2022 19:39, 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.
>>>
>>>
>>> Out of curiosity - why new HMP command, is not QOM'ing this ms::fdt property enough?
>>
>> I tried to do the minimal changes needed for the commands to work. ms::fdt is
>> one of the few MachineState fields that hasn't been QOMified by
>> machine_class_init() yet. All pre-existing code that uses ms::fdt are using the
>> pointer directly. To make a QOMified use of it would require extra patches
>> in machine.c to QOMify the property first.
>>
>> There's also the issue with how each machine is creating the FDT. Most are using
>> helpers from device_tree.c, some are creating it from scratch, others required
>> a .dtb file, most of them are not doing a fdt_pack() and so on. To really QOMify
>> the use of ms::fdt we would need some machine hooks that standardize all that.
>> I believe it's worth the trouble, but it would be too much to do
>> right now.
> 
> Hmm.. I think this depends on what you mean by "QOM"ify exactly.  If
> you're meaning make the full DT representation QOM objects, that you
> can look into in detail, then, yes, that's pretty complicated.
> 
> I suspect what Alexey was suggesting though, was merely to make
> ms::fdt accessible as a single bytestring property on the machine QOM
> object.  Effectively it's just "dumpdtb" but as a property get.


Yes, I meant the bytestream, as DTC can easily decompile it onto a DTS.


> I'm not 100% certain if QOM can safely represent arbitrary bytestrings
> as QOM properties, which would need checking.

I am not sure either but rather than adding another command to HMP, I'd 
explore this option first.



-- 
Alexey

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

[Daniel Henrique Barboza]

On 8/22/22 00:29, Alexey Kardashevskiy wrote:
> 
> 
> On 22/08/2022 13:05, David Gibson wrote:
>> On Fri, Aug 19, 2022 at 06:42:34AM -0300, Daniel Henrique Barboza wrote:
>>>
>>>
>>> On 8/18/22 23:11, Alexey Kardashevskiy wrote:
>>>>
>>>>
>>>> On 05/08/2022 19:39, 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.
>>>>
>>>>
>>>> Out of curiosity - why new HMP command, is not QOM'ing this ms::fdt property enough?
>>>
>>> I tried to do the minimal changes needed for the commands to work. ms::fdt is
>>> one of the few MachineState fields that hasn't been QOMified by
>>> machine_class_init() yet. All pre-existing code that uses ms::fdt are using the
>>> pointer directly. To make a QOMified use of it would require extra patches
>>> in machine.c to QOMify the property first.
>>>
>>> There's also the issue with how each machine is creating the FDT. Most are using
>>> helpers from device_tree.c, some are creating it from scratch, others required
>>> a .dtb file, most of them are not doing a fdt_pack() and so on. To really QOMify
>>> the use of ms::fdt we would need some machine hooks that standardize all that.
>>> I believe it's worth the trouble, but it would be too much to do
>>> right now.
>>
>> Hmm.. I think this depends on what you mean by "QOM"ify exactly.  If
>> you're meaning make the full DT representation QOM objects, that you
>> can look into in detail, then, yes, that's pretty complicated.
>>
>> I suspect what Alexey was suggesting though, was merely to make
>> ms::fdt accessible as a single bytestring property on the machine QOM
>> object.  Effectively it's just "dumpdtb" but as a property get.
> 
> 
> Yes, I meant the bytestream, as DTC can easily decompile it onto a DTS.
> 
> 
>> I'm not 100% certain if QOM can safely represent arbitrary bytestrings
>> as QOM properties, which would need checking.
> 
> I am not sure either but rather than adding another command to HMP, I'd explore this option first.


I'm not sure what you mean by that. The HMP version of 'dumpdtb' is more flexible
that the current "-machine dumpdtb", an extra machine option that would cause
the guest to exit after writing the dtb. And 'info fdt' is a new command that
makes it easier to inspect specific nodes/props.

I don't see how making ms::fdt being retrievable by object_property_get() internally
(remember that ms::fdt it's not fully QOMified, so there's no introspection of its
value from the QEMU monitor) would make any of these new HMP commands obsolete.


Thanks,


Daniel

> 
> 
> 

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

[Alexey Kardashevskiy]

On 22/08/2022 20:30, Daniel Henrique Barboza wrote:
> 
> 
> On 8/22/22 00:29, Alexey Kardashevskiy wrote:
>>
>>
>> On 22/08/2022 13:05, David Gibson wrote:
>>> On Fri, Aug 19, 2022 at 06:42:34AM -0300, Daniel Henrique Barboza wrote:
>>>>
>>>>
>>>> On 8/18/22 23:11, Alexey Kardashevskiy wrote:
>>>>>
>>>>>
>>>>> On 05/08/2022 19:39, 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.
>>>>>
>>>>>
>>>>> Out of curiosity - why new HMP command, is not QOM'ing this ms::fdt 
>>>>> property enough?
>>>>
>>>> I tried to do the minimal changes needed for the commands to work. 
>>>> ms::fdt is
>>>> one of the few MachineState fields that hasn't been QOMified by
>>>> machine_class_init() yet. All pre-existing code that uses ms::fdt 
>>>> are using the
>>>> pointer directly. To make a QOMified use of it would require extra 
>>>> patches
>>>> in machine.c to QOMify the property first.
>>>>
>>>> There's also the issue with how each machine is creating the FDT. 
>>>> Most are using
>>>> helpers from device_tree.c, some are creating it from scratch, 
>>>> others required
>>>> a .dtb file, most of them are not doing a fdt_pack() and so on. To 
>>>> really QOMify
>>>> the use of ms::fdt we would need some machine hooks that standardize 
>>>> all that.
>>>> I believe it's worth the trouble, but it would be too much to do
>>>> right now.
>>>
>>> Hmm.. I think this depends on what you mean by "QOM"ify exactly.  If
>>> you're meaning make the full DT representation QOM objects, that you
>>> can look into in detail, then, yes, that's pretty complicated.
>>>
>>> I suspect what Alexey was suggesting though, was merely to make
>>> ms::fdt accessible as a single bytestring property on the machine QOM
>>> object.  Effectively it's just "dumpdtb" but as a property get.
>>
>>
>> Yes, I meant the bytestream, as DTC can easily decompile it onto a DTS.
>>
>>
>>> I'm not 100% certain if QOM can safely represent arbitrary bytestrings
>>> as QOM properties, which would need checking.
>>
>> I am not sure either but rather than adding another command to HMP, 
>> I'd explore this option first.
> 
> 
> I'm not sure what you mean by that. The HMP version of 'dumpdtb' is more 
> flexible
> that the current "-machine dumpdtb", an extra machine option that would 
> cause
> the guest to exit after writing the dtb

True. Especially with CAS :)

> And 'info fdt' is a new command 
> that
> makes it easier to inspect specific nodes/props.

btw what is this new command going to do? decompile the tree or save dtb?

> I don't see how making ms::fdt being retrievable by 
> object_property_get() internally
> (remember that ms::fdt it's not fully QOMified, so there's no 
> introspection of its
> value from the QEMU monitor) would make any of these new HMP commands 
> obsolete.

Well, there are QMP and HMP and my feeling was that HMP is slowly 
getting deprecated or something and QMP is the superior one. So I 
thought since this FDT is a property and there is no associated action 
with it, making it a property would do.

For ages I've been using a python3 script to talk to QMP as HMP is 
really quite limited, the only thing in HMP which is not in QMP is 
dumping memory ("x", "xp"), in this case I wrap HMP into QMP and keep 
using QMP :)


> 
> 
> Thanks,
> 
> 
> Daniel
> 
>>
>>
>>

-- 
Alexey

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

[Daniel Henrique Barboza]

On 8/23/22 05:58, Alexey Kardashevskiy wrote:
> 
> 
> On 22/08/2022 20:30, Daniel Henrique Barboza wrote:
>>
>>
>> On 8/22/22 00:29, Alexey Kardashevskiy wrote:
>>>
>>>
>>> On 22/08/2022 13:05, David Gibson wrote:
>>>> On Fri, Aug 19, 2022 at 06:42:34AM -0300, Daniel Henrique Barboza wrote:
>>>>>
>>>>>
>>>>> On 8/18/22 23:11, Alexey Kardashevskiy wrote:
>>>>>>
>>>>>>
>>>>>> On 05/08/2022 19:39, 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.
>>>>>>
>>>>>>
>>>>>> Out of curiosity - why new HMP command, is not QOM'ing this ms::fdt property enough?
>>>>>
>>>>> I tried to do the minimal changes needed for the commands to work. ms::fdt is
>>>>> one of the few MachineState fields that hasn't been QOMified by
>>>>> machine_class_init() yet. All pre-existing code that uses ms::fdt are using the
>>>>> pointer directly. To make a QOMified use of it would require extra patches
>>>>> in machine.c to QOMify the property first.
>>>>>
>>>>> There's also the issue with how each machine is creating the FDT. Most are using
>>>>> helpers from device_tree.c, some are creating it from scratch, others required
>>>>> a .dtb file, most of them are not doing a fdt_pack() and so on. To really QOMify
>>>>> the use of ms::fdt we would need some machine hooks that standardize all that.
>>>>> I believe it's worth the trouble, but it would be too much to do
>>>>> right now.
>>>>
>>>> Hmm.. I think this depends on what you mean by "QOM"ify exactly.  If
>>>> you're meaning make the full DT representation QOM objects, that you
>>>> can look into in detail, then, yes, that's pretty complicated.
>>>>
>>>> I suspect what Alexey was suggesting though, was merely to make
>>>> ms::fdt accessible as a single bytestring property on the machine QOM
>>>> object.  Effectively it's just "dumpdtb" but as a property get.
>>>
>>>
>>> Yes, I meant the bytestream, as DTC can easily decompile it onto a DTS.
>>>
>>>
>>>> I'm not 100% certain if QOM can safely represent arbitrary bytestrings
>>>> as QOM properties, which would need checking.
>>>
>>> I am not sure either but rather than adding another command to HMP, I'd explore this option first.
>>
>>
>> I'm not sure what you mean by that. The HMP version of 'dumpdtb' is more flexible
>> that the current "-machine dumpdtb", an extra machine option that would cause
>> the guest to exit after writing the dtb
> 
> True. Especially with CAS :)
> 
>> And 'info fdt' is a new command that
>> makes it easier to inspect specific nodes/props.
> 
> btw what is this new command going to do? decompile the tree or save dtb?

At this moment, 'info fdt <node> [prop]' is using the current ms->fdt bytestream
plus libfdt to output nodes and properties.

> 
>> I don't see how making ms::fdt being retrievable by object_property_get() internally
>> (remember that ms::fdt it's not fully QOMified, so there's no introspection of its
>> value from the QEMU monitor) would make any of these new HMP commands obsolete.
> 
> Well, there are QMP and HMP and my feeling was that HMP is slowly getting deprecated or something and QMP is the superior one. So I thought since this FDT is a property and there is no associated action with it, making it a property would do.

I don't have an option about HMP being deprecated and QMP being the preferable
choice. Perhaps someone else reading this thread can tell more about it.

> 
> For ages I've been using a python3 script to talk to QMP as HMP is really quite limited, the only thing in HMP which is not in QMP is dumping memory ("x", "xp"), in this case I wrap HMP into QMP and keep using QMP :)


Apparently we have a QMP enthusiast over here ;)



Daniel

> 
> 
>>
>>
>> Thanks,
>>
>>
>> Daniel
>>
>>>
>>>
>>>
> 

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

[David Gibson]

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

On Mon, Aug 22, 2022 at 07:30:36AM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 8/22/22 00:29, Alexey Kardashevskiy wrote:
> > 
> > 
> > On 22/08/2022 13:05, David Gibson wrote:
> > > On Fri, Aug 19, 2022 at 06:42:34AM -0300, Daniel Henrique Barboza wrote:
> > > > 
> > > > 
> > > > On 8/18/22 23:11, Alexey Kardashevskiy wrote:
> > > > > 
> > > > > 
> > > > > On 05/08/2022 19:39, 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.
> > > > > 
> > > > > 
> > > > > Out of curiosity - why new HMP command, is not QOM'ing this ms::fdt property enough?
> > > > 
> > > > I tried to do the minimal changes needed for the commands to work. ms::fdt is
> > > > one of the few MachineState fields that hasn't been QOMified by
> > > > machine_class_init() yet. All pre-existing code that uses ms::fdt are using the
> > > > pointer directly. To make a QOMified use of it would require extra patches
> > > > in machine.c to QOMify the property first.
> > > > 
> > > > There's also the issue with how each machine is creating the FDT. Most are using
> > > > helpers from device_tree.c, some are creating it from scratch, others required
> > > > a .dtb file, most of them are not doing a fdt_pack() and so on. To really QOMify
> > > > the use of ms::fdt we would need some machine hooks that standardize all that.
> > > > I believe it's worth the trouble, but it would be too much to do
> > > > right now.
> > > 
> > > Hmm.. I think this depends on what you mean by "QOM"ify exactly.  If
> > > you're meaning make the full DT representation QOM objects, that you
> > > can look into in detail, then, yes, that's pretty complicated.
> > > 
> > > I suspect what Alexey was suggesting though, was merely to make
> > > ms::fdt accessible as a single bytestring property on the machine QOM
> > > object.  Effectively it's just "dumpdtb" but as a property get.
> > 
> > 
> > Yes, I meant the bytestream, as DTC can easily decompile it onto a DTS.
> > 
> > 
> > > I'm not 100% certain if QOM can safely represent arbitrary bytestrings
> > > as QOM properties, which would need checking.
> > 
> > I am not sure either but rather than adding another command to HMP, I'd explore this option first.
> 
> 
> I'm not sure what you mean by that. The HMP version of 'dumpdtb' is more flexible
> that the current "-machine dumpdtb", an extra machine option that would cause
> the guest to exit after writing the dtb. And 'info fdt' is a new command that
> makes it easier to inspect specific nodes/props.
> 
> I don't see how making ms::fdt being retrievable by object_property_get() internally
> (remember that ms::fdt it's not fully QOMified, so there's no introspection of its
> value from the QEMU monitor) would make any of these new HMP commands obsolete.

I believe what we were thinking is if the dtb (as a single bytestring) can be
retrieved with a qom-get on a suitable property on the machine, that
might make things marginally simpler than adding a new command.  I'm
not certain if the JSON format of the QMP responses can safely encode
an arbitrary bytestring, though (as opoosed to a Unicode string).

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