[PATCH v2 0/7] hw/ppc: Snapshot support for several ppc devices

[PATCH v2 0/7] hw/ppc: Snapshot support for several ppc devices

[Caleb Schlossin]

Addressing comments from V1 review:

Updates in V2:
- Added new patch set for PnvPsi support as it fits with the rest
- Added vmstate support for Power8 and Power9 for LPC
- Fixed pnv_core.c commit message

Tested:
passed make check

Thanks,
Caleb

Michael Kowal (2):
  hw/ppc: Add VMSTATE information for LPC model
  hw/ppc: Add VMSTATE information to PnvPsi

Caleb Schlossin (2):
  hw/ppc: Add pnv_spi vmstate support
  hw/ppc: Add pnv_i2c vmstate support

Angelo Jaramillo (3):
  hw/ppc: pnv_adu.c added vmstate support
  hw/ppc: pnv_core.c add vmstate support
  hw/ppc: pnv_chiptod.c add vmstate support
  

 hw/ppc/pnv_adu.c             | 12 ++++++++++
 hw/ppc/pnv_chiptod.c         | 38 +++++++++++++++++++++++++++++
 hw/ppc/pnv_core.c            | 22 +++++++++++++++++
 hw/ppc/pnv_i2c.c             | 11 +++++++++
 hw/ppc/pnv_lpc.c             | 41 ++++++++++++++++++++++++++++++++
 hw/ppc/pnv_psi.c             | 46 ++++++++++++++++++++++++++++++++++--
 hw/ssi/pnv_spi.c             | 27 +++++++++++++++++++++
 include/hw/ppc/pnv_chiptod.h |  2 ++
 8 files changed, 197 insertions(+), 2 deletions(-)

-- 
2.47.3

[PATCH v2 1/7] hw/ppc: Add VMSTATE information for LPC model

[Caleb Schlossin]

The PNV LPC model needs snapshot/migration support.  Added a VMSTATE
descriptor to save model data and an associated post_load() method.
Snapshot support added for Power8, Power9, and Power10.

Signed-off-by: Michael Kowal <kowal@linux.ibm.com>
Signed-off-by: Caleb Schlossin <calebs@linux.ibm.com>
---
 hw/ppc/pnv_lpc.c | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
index f6beba0917..e52a062181 100644
--- a/hw/ppc/pnv_lpc.c
+++ b/hw/ppc/pnv_lpc.c
@@ -30,6 +30,7 @@
 #include "hw/ppc/pnv_lpc.h"
 #include "hw/ppc/pnv_xscom.h"
 #include "hw/ppc/fdt.h"
+#include "migration/vmstate.h"
 
 #include <libfdt.h>
 
@@ -696,6 +697,43 @@ static const MemoryRegionOps opb_master_ops = {
     },
 };
 
+static int vmstate_pnv_lpc_post_load(void *opaque, int version_id)
+{
+    PnvLpcController *lpc = PNV_LPC(opaque);
+
+    memory_region_set_alias_offset(&lpc->opb_isa_fw,
+                                   lpc->lpc_hc_fw_seg_idsel * LPC_FW_OPB_SIZE);
+    pnv_lpc_eval_serirq_routes(lpc);
+
+    pnv_lpc_eval_irqs(lpc);
+    return 0;
+}
+
+static const VMStateDescription vmstate_pnv_lpc = {
+    .name = TYPE_PNV_LPC,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .post_load = vmstate_pnv_lpc_post_load,
+    .fields = (const VMStateField[]) {
+        VMSTATE_UINT64(eccb_stat_reg,       PnvLpcController),
+        VMSTATE_UINT32(eccb_data_reg,       PnvLpcController),
+        VMSTATE_UINT32(opb_irq_route0,      PnvLpcController),
+        VMSTATE_UINT32(opb_irq_route1,      PnvLpcController),
+        VMSTATE_UINT32(opb_irq_stat,        PnvLpcController),
+        VMSTATE_UINT32(opb_irq_mask,        PnvLpcController),
+        VMSTATE_UINT32(opb_irq_pol,         PnvLpcController),
+        VMSTATE_UINT32(opb_irq_input,       PnvLpcController),
+        VMSTATE_UINT32(lpc_hc_irq_inputs,   PnvLpcController),
+        VMSTATE_UINT32(lpc_hc_fw_seg_idsel, PnvLpcController),
+        VMSTATE_UINT32(lpc_hc_irqser_ctrl,  PnvLpcController),
+        VMSTATE_UINT32(lpc_hc_irqmask,      PnvLpcController),
+        VMSTATE_UINT32(lpc_hc_irqstat,      PnvLpcController),
+        VMSTATE_UINT32(lpc_hc_error_addr,   PnvLpcController),
+        VMSTATE_UINT32(lpc_hc_fw_rd_acc_size,     PnvLpcController),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void pnv_lpc_power8_realize(DeviceState *dev, Error **errp)
 {
     PnvLpcController *lpc = PNV_LPC(dev);
@@ -721,6 +759,7 @@ static void pnv_lpc_power8_class_init(ObjectClass *klass, const void *data)
     PnvLpcClass *plc = PNV_LPC_CLASS(klass);
 
     dc->desc = "PowerNV LPC Controller POWER8";
+    dc->vmsd = &vmstate_pnv_lpc;
 
     xdc->dt_xscom = pnv_lpc_dt_xscom;
 
@@ -766,6 +805,7 @@ static void pnv_lpc_power9_class_init(ObjectClass *klass, const void *data)
     PnvLpcClass *plc = PNV_LPC_CLASS(klass);
 
     dc->desc = "PowerNV LPC Controller POWER9";
+    dc->vmsd = &vmstate_pnv_lpc;
 
     device_class_set_parent_realize(dc, pnv_lpc_power9_realize,
                                     &plc->parent_realize);
@@ -782,6 +822,7 @@ static void pnv_lpc_power10_class_init(ObjectClass *klass, const void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->desc = "PowerNV LPC Controller POWER10";
+    dc->vmsd = &vmstate_pnv_lpc;
 }
 
 static const TypeInfo pnv_lpc_power10_info = {
-- 
2.47.3

[PATCH v2 2/7] hw/ppc: Add pnv_spi vmstate support

[Caleb Schlossin]

- Add support for needed PnvSpi structure variables

Signed-off-by: Caleb Schlossin <calebs@linux.ibm.com>
---
 hw/ssi/pnv_spi.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
index f40e8836b9..389a2cca6b 100644
--- a/hw/ssi/pnv_spi.c
+++ b/hw/ssi/pnv_spi.c
@@ -13,6 +13,7 @@
 #include "hw/ssi/pnv_spi.h"
 #include "hw/ssi/pnv_spi_regs.h"
 #include "hw/ssi/ssi.h"
+#include "migration/vmstate.h"
 #include <libfdt.h>
 #include "hw/irq.h"
 #include "trace.h"
@@ -1199,6 +1200,31 @@ static int pnv_spi_dt_xscom(PnvXScomInterface *dev, void *fdt,
     return 0;
 }
 
+static const VMStateDescription pnv_spi_vmstate = {
+    .name = TYPE_PNV_SPI,
+    .version_id = 1,
+    .fields = (const VMStateField[]) {
+        VMSTATE_UINT8(fail_count, PnvSpi),
+        VMSTATE_UINT8(transfer_len, PnvSpi),
+        VMSTATE_UINT8(responder_select, PnvSpi),
+        VMSTATE_BOOL(shift_n1_done, PnvSpi),
+        VMSTATE_UINT8(loop_counter_1, PnvSpi),
+        VMSTATE_UINT8(loop_counter_2, PnvSpi),
+        VMSTATE_UINT8(N1_bits, PnvSpi),
+        VMSTATE_UINT8(N2_bits, PnvSpi),
+        VMSTATE_UINT8(N1_bytes, PnvSpi),
+        VMSTATE_UINT8(N2_bytes, PnvSpi),
+        VMSTATE_UINT8(N1_tx, PnvSpi),
+        VMSTATE_UINT8(N2_tx, PnvSpi),
+        VMSTATE_UINT8(N1_rx, PnvSpi),
+        VMSTATE_UINT8(N2_rx, PnvSpi),
+        VMSTATE_UINT64_ARRAY(regs, PnvSpi, PNV_SPI_REGS),
+        VMSTATE_UINT8_ARRAY(seq_op, PnvSpi, PNV_SPI_REG_SIZE),
+        VMSTATE_UINT64(status, PnvSpi),
+        VMSTATE_END_OF_LIST(),
+    },
+};
+
 static void pnv_spi_class_init(ObjectClass *klass, const void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -1209,6 +1235,7 @@ static void pnv_spi_class_init(ObjectClass *klass, const void *data)
     dc->desc = "PowerNV SPI";
     dc->realize = pnv_spi_realize;
     device_class_set_legacy_reset(dc, do_reset);
+    dc->vmsd = &pnv_spi_vmstate;
     device_class_set_props(dc, pnv_spi_properties);
 }
 
-- 
2.47.3

[PATCH v2 3/7] hw/ppc: Add pnv_i2c vmstate support

[Caleb Schlossin]

- Add vmstate support for i2c registers

Signed-off-by: Caleb Schlossin <calebs@linux.ibm.com>
---
 hw/ppc/pnv_i2c.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/ppc/pnv_i2c.c b/hw/ppc/pnv_i2c.c
index 60de479491..1018078228 100644
--- a/hw/ppc/pnv_i2c.c
+++ b/hw/ppc/pnv_i2c.c
@@ -19,6 +19,7 @@
 #include "hw/ppc/pnv_i2c.h"
 #include "hw/ppc/pnv_xscom.h"
 #include "hw/ppc/fdt.h"
+#include "migration/vmstate.h"
 
 #include <libfdt.h>
 
@@ -549,6 +550,15 @@ static const Property pnv_i2c_properties[] = {
     DEFINE_PROP_UINT32("num-busses", PnvI2C, num_busses, 1),
 };
 
+static const VMStateDescription pnv_i2c_vmstate = {
+    .name = TYPE_PNV_I2C,
+    .version_id = 1,
+    .fields = (const VMStateField[]) {
+        VMSTATE_UINT64_ARRAY(regs, PnvI2C, PNV_I2C_REGS),
+        VMSTATE_END_OF_LIST(),
+    },
+};
+
 static void pnv_i2c_class_init(ObjectClass *klass, const void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -561,6 +571,7 @@ static void pnv_i2c_class_init(ObjectClass *klass, const void *data)
 
     dc->desc = "PowerNV I2C";
     dc->realize = pnv_i2c_realize;
+    dc->vmsd = &pnv_i2c_vmstate;
     device_class_set_props(dc, pnv_i2c_properties);
 }
 
-- 
2.47.3

[PATCH v2 4/7] hw/ppc: pnv_adu.c added vmstate support

[Caleb Schlossin]

- Added vmstate support for ADU model

Signed-off-by: Angelo Jaramillo <angelo.jaramillo@linux.ibm.com>
Signed-off-by: Caleb Schlossin <calebs@linux.ibm.com>
---
 hw/ppc/pnv_adu.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/ppc/pnv_adu.c b/hw/ppc/pnv_adu.c
index 005fbda475..bd2a9e233a 100644
--- a/hw/ppc/pnv_adu.c
+++ b/hw/ppc/pnv_adu.c
@@ -23,6 +23,7 @@
 #include "hw/ppc/pnv_chip.h"
 #include "hw/ppc/pnv_lpc.h"
 #include "hw/ppc/pnv_xscom.h"
+#include "migration/vmstate.h"
 #include "trace.h"
 
 #define ADU_LPC_BASE_REG     0x40
@@ -189,6 +190,16 @@ static const Property pnv_adu_properties[] = {
     DEFINE_PROP_LINK("lpc", PnvADU, lpc, TYPE_PNV_LPC, PnvLpcController *),
 };
 
+static const VMStateDescription pnv_adu_vmstate = {
+    .name = TYPE_PNV_ADU,
+    .version_id = 1,
+    .fields = (const VMStateField[]) {
+        VMSTATE_UINT64(lpc_cmd_reg, PnvADU),
+        VMSTATE_UINT64(lpc_data_reg, PnvADU),
+        VMSTATE_END_OF_LIST(),
+    },
+};
+
 static void pnv_adu_class_init(ObjectClass *klass, const void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -197,6 +208,7 @@ static void pnv_adu_class_init(ObjectClass *klass, const void *data)
     dc->desc = "PowerNV ADU";
     device_class_set_props(dc, pnv_adu_properties);
     dc->user_creatable = false;
+    dc->vmsd = &pnv_adu_vmstate;
 }
 
 static const TypeInfo pnv_adu_type_info = {
-- 
2.47.3

[PATCH v2 5/7] hw/ppc: pnv_core.c add vmstate support

[Caleb Schlossin]

- Add vmstate support PnvCore and PnvQuad capturing scratch
registers and special wakeup registers

Signed-off-by: Angelo Jaramillo <angelo.jaramillo@linux.ibm.com>
Signed-off-by: Caleb Schlossin <calebs@linux.ibm.com>
---
 hw/ppc/pnv_core.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
index fb2dfc7ba2..03b64f0013 100644
--- a/hw/ppc/pnv_core.c
+++ b/hw/ppc/pnv_core.c
@@ -31,6 +31,7 @@
 #include "hw/ppc/xics.h"
 #include "hw/qdev-properties.h"
 #include "helper_regs.h"
+#include "migration/vmstate.h"
 
 static const char *pnv_core_cpu_typename(PnvCore *pc)
 {
@@ -478,6 +479,15 @@ static void pnv_core_power11_class_init(ObjectClass *oc, const void *data)
     pnv_core_power10_class_init(oc, data);
 }
 
+static const VMStateDescription pnv_core_vmstate = {
+    .name = TYPE_PNV_CORE,
+    .version_id = 1,
+    .fields = (const VMStateField[]) {
+        VMSTATE_UINT64_ARRAY(scratch, PnvCore, 8),
+        VMSTATE_END_OF_LIST(),
+    },
+};
+
 static void pnv_core_class_init(ObjectClass *oc, const void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
@@ -486,6 +496,7 @@ static void pnv_core_class_init(ObjectClass *oc, const void *data)
     dc->unrealize = pnv_core_unrealize;
     device_class_set_props(dc, pnv_core_properties);
     dc->user_creatable = false;
+    dc->vmsd = &pnv_core_vmstate;
 }
 
 #define DEFINE_PNV_CORE_TYPE(family, cpu_model) \
@@ -737,12 +748,23 @@ static void pnv_quad_power11_class_init(ObjectClass *oc, const void *data)
     pnv_quad_power10_class_init(oc, data);
 }
 
+static const VMStateDescription pnv_quad_vmstate = {
+    .name = TYPE_PNV_QUAD,
+    .version_id = 1,
+    .fields = (const VMStateField[]) {
+        VMSTATE_BOOL(special_wakeup_done, PnvQuad),
+        VMSTATE_BOOL_ARRAY(special_wakeup, PnvQuad, 4),
+        VMSTATE_END_OF_LIST(),
+    },
+};
+
 static void pnv_quad_class_init(ObjectClass *oc, const void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
 
     device_class_set_props(dc, pnv_quad_properties);
     dc->user_creatable = false;
+    dc->vmsd = &pnv_quad_vmstate;
 }
 
 static const TypeInfo pnv_quad_infos[] = {
-- 
2.47.3

[PATCH v2 6/7] hw/ppc: pnv_chiptod.c add vmstate support

[Caleb Schlossin]

- Added pre_save and post_load methods to handle slave_pc_target and tod_state

Signed-off-by: Angelo Jaramillo <angelo.jaramillo@linux.ibm.com>
Signed-off-by: Caleb Schlossin <calebs@linux.ibm.com>
---
 hw/ppc/pnv_chiptod.c         | 38 ++++++++++++++++++++++++++++++++++++
 include/hw/ppc/pnv_chiptod.h |  2 ++
 2 files changed, 40 insertions(+)

diff --git a/hw/ppc/pnv_chiptod.c b/hw/ppc/pnv_chiptod.c
index f887a18cde..9dc5942ca0 100644
--- a/hw/ppc/pnv_chiptod.c
+++ b/hw/ppc/pnv_chiptod.c
@@ -37,6 +37,7 @@
 #include "hw/ppc/pnv_core.h"
 #include "hw/ppc/pnv_xscom.h"
 #include "hw/ppc/pnv_chiptod.h"
+#include "migration/vmstate.h"
 #include "trace.h"
 
 #include <libfdt.h>
@@ -341,6 +342,8 @@ static void pnv_chiptod_xscom_write(void *opaque, hwaddr addr,
                           " TOD_TX_TTYPE_CTRL_REG val 0x%" PRIx64
                           " invalid slave address\n", val);
         }
+        /* Write slave_pc_target to a uint64_t variable for vmstate support. */
+        chiptod->tx_ttype_ctrl = val;
         break;
     case TOD_ERROR_REG:
         chiptod->tod_error &= ~val;
@@ -613,6 +616,40 @@ static void pnv_chiptod_unrealize(DeviceState *dev)
     qemu_unregister_reset(pnv_chiptod_reset, chiptod);
 }
 
+static int vmstate_pnv_chiptod_pre_save(void *opaque)
+{
+    PnvChipTOD *chiptod = PNV_CHIPTOD(opaque);
+    chiptod->tod_state_val = (uint8_t)chiptod->tod_state;
+    return 0;
+}
+
+static int vmstate_pnv_chiptod_post_load(void *opaque)
+{
+    PnvChipTOD *chiptod = PNV_CHIPTOD(opaque);
+    if (chiptod->tx_ttype_ctrl != 0) {
+        pnv_chiptod_xscom_write(chiptod, TOD_TX_TTYPE_CTRL_REG << 3,
+                                chiptod->tx_ttype_ctrl, 8);
+    }
+    chiptod->tod_state = (enum tod_state)chiptod->tod_state_val;
+    return 0;
+}
+
+static const VMStateDescription pnv_chiptod_vmstate = {
+    .name = TYPE_PNV_CHIPTOD,
+    .version_id = 1,
+    .pre_save = vmstate_pnv_chiptod_pre_save,
+    .pre_load = vmstate_pnv_chiptod_post_load,
+    .fields = (const VMStateField[]) {
+        VMSTATE_BOOL(primary, PnvChipTOD),
+        VMSTATE_BOOL(secondary, PnvChipTOD),
+        VMSTATE_UINT64(tod_error, PnvChipTOD),
+        VMSTATE_UINT64(pss_mss_ctrl_reg, PnvChipTOD),
+        VMSTATE_UINT64(tx_ttype_ctrl, PnvChipTOD),
+        VMSTATE_UINT8(tod_state_val, PnvChipTOD),
+        VMSTATE_END_OF_LIST(),
+    },
+};
+
 static void pnv_chiptod_class_init(ObjectClass *klass, const void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -621,6 +658,7 @@ static void pnv_chiptod_class_init(ObjectClass *klass, const void *data)
     dc->unrealize = pnv_chiptod_unrealize;
     dc->desc = "PowerNV ChipTOD Controller";
     dc->user_creatable = false;
+    dc->vmsd = &pnv_chiptod_vmstate;
 }
 
 static const TypeInfo pnv_chiptod_type_info = {
diff --git a/include/hw/ppc/pnv_chiptod.h b/include/hw/ppc/pnv_chiptod.h
index 466b06560a..3e5e3b02b2 100644
--- a/include/hw/ppc/pnv_chiptod.h
+++ b/include/hw/ppc/pnv_chiptod.h
@@ -41,6 +41,8 @@ struct PnvChipTOD {
     uint64_t tod_error;
     uint64_t pss_mss_ctrl_reg;
     PnvCore *slave_pc_target;
+    uint64_t tx_ttype_ctrl;
+    uint8_t tod_state_val;
 };
 
 struct PnvChipTODClass {
-- 
2.47.3

[PATCH v2 7/7] hw/ppc: Add VMSTATE information to PnvPsi

[Caleb Schlossin]

PnvPsi needs to be able to save/load snapshots.  Add VMSTATE information
to the device class and a post_load() method to restore dynamic data items and
memory region mappings.

Signed-off-by: Michael Kowal <kowal@linux.ibm.com>
Signed-off-by: Caleb Schlossin <calebs@linux.ibm.com>
---
 hw/ppc/pnv_psi.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 44 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
index 5d947d8b52..88d5f1d45d 100644
--- a/hw/ppc/pnv_psi.c
+++ b/hw/ppc/pnv_psi.c
@@ -25,6 +25,7 @@
 #include "qemu/module.h"
 #include "system/reset.h"
 #include "qapi/error.h"
+#include "migration/vmstate.h"
 
 
 #include "hw/ppc/fdt.h"
@@ -35,6 +36,8 @@
 
 #include <libfdt.h>
 
+#undef PSI_DEBUG
+
 #define PSIHB_XSCOM_FIR_RW      0x00
 #define PSIHB_XSCOM_FIR_AND     0x01
 #define PSIHB_XSCOM_FIR_OR      0x02
@@ -130,12 +133,11 @@ static void pnv_psi_set_bar(PnvPsi *psi, uint64_t bar)
 {
     PnvPsiClass *ppc = PNV_PSI_GET_CLASS(psi);
     MemoryRegion *sysmem = get_system_memory();
-    uint64_t old = psi->regs[PSIHB_XSCOM_BAR];
 
     psi->regs[PSIHB_XSCOM_BAR] = bar & (ppc->bar_mask | PSIHB_BAR_EN);
 
     /* Update MR, always remove it first */
-    if (old & PSIHB_BAR_EN) {
+    if (memory_region_is_mapped(&psi->regs_mr)) {
         memory_region_del_subregion(sysmem, &psi->regs_mr);
     }
 
@@ -975,6 +977,40 @@ static void pnv_psi_register_types(void)
 
 type_init(pnv_psi_register_types);
 
+#ifdef PSI_DEBUG
+static void psi_regs_pic_print_info(uint64_t *regs, uint32_t nr_regs,
+                                    GString *buf) {
+    uint i, prev_idx = -1;
+    uint64_t  reg1, prev_reg1 = -1;
+    uint64_t  reg2, prev_reg2 = -1;
+    uint64_t  reg3, prev_reg3 = -1;
+    uint64_t  reg4, prev_reg4 = -1;
+    for (i = 0; i < nr_regs; i = i + 4) {
+        /* Don't print if values do not change, but print last*/
+        reg1 = regs[i];
+        reg2 = regs[i + 1];
+        reg3 = regs[i + 2];
+        reg4 = regs[i + 3];
+        if (reg1 == prev_reg1 && reg2 == prev_reg2 &&
+            reg3 == prev_reg3 && reg4 == prev_reg4 &&
+            i < (nr_regs - 4)) {
+            if (i == (prev_idx + 4)) {
+                g_string_append_printf(buf, "        . . .\n");
+            }
+            continue;
+        }
+
+        g_string_append_printf(buf, "  [%03X] 0x%016lX %016lX %016lX %016lX\n",
+            i, reg1, reg2, reg3, reg4);
+        prev_idx = i;
+        prev_reg1 = reg1;
+        prev_reg2 = reg2;
+        prev_reg3 = reg3;
+        prev_reg4 = reg4;
+    }
+}
+#endif
+
 void pnv_psi_pic_print_info(Pnv9Psi *psi9, GString *buf)
 {
     PnvPsi *psi = PNV_PSI(psi9);
@@ -985,4 +1021,10 @@ void pnv_psi_pic_print_info(Pnv9Psi *psi9, GString *buf)
     g_string_append_printf(buf, "PSIHB Source %08x .. %08x\n",
                            offset, offset + psi9->source.nr_irqs - 1);
     xive_source_pic_print_info(&psi9->source, offset, buf);
+#ifdef PSI_DEBUG
+    /* Print PSI registers */
+    g_string_append_printf(buf, "\nPSI Regs[0x0..%X]\n",
+                           PSIHB_XSCOM_MAX);
+    psi_regs_pic_print_info(psi->regs, PSIHB_XSCOM_MAX, buf);
+#endif
 }
-- 
2.47.3

Re: [PATCH v2 7/7] hw/ppc: Add VMSTATE information to PnvPsi

[Aditya Gupta]

Hello Caleb,

On 25/12/15 11:18AM, Caleb Schlossin wrote:
> <...snip...>
>
> --- a/hw/ppc/pnv_psi.c
> +++ b/hw/ppc/pnv_psi.c
> @@ -25,6 +25,7 @@
>  #include "qemu/module.h"
>  #include "system/reset.h"
>  #include "qapi/error.h"
> +#include "migration/vmstate.h"
>  
>  
>  #include "hw/ppc/fdt.h"
> @@ -35,6 +36,8 @@
>  
>  #include <libfdt.h>
>  
> +#undef PSI_DEBUG
> +

Is this intended or got left over from debugging ?

>  #define PSIHB_XSCOM_FIR_RW      0x00
>  #define PSIHB_XSCOM_FIR_AND     0x01
>  #define PSIHB_XSCOM_FIR_OR      0x02
> @@ -130,12 +133,11 @@ static void pnv_psi_set_bar(PnvPsi *psi, uint64_t bar)
>  {
>      PnvPsiClass *ppc = PNV_PSI_GET_CLASS(psi);
>      MemoryRegion *sysmem = get_system_memory();
> -    uint64_t old = psi->regs[PSIHB_XSCOM_BAR];
>  
>      psi->regs[PSIHB_XSCOM_BAR] = bar & (ppc->bar_mask | PSIHB_BAR_EN);
>  
>      /* Update MR, always remove it first */
> -    if (old & PSIHB_BAR_EN) {
> +    if (memory_region_is_mapped(&psi->regs_mr)) {
>          memory_region_del_subregion(sysmem, &psi->regs_mr);
>      }
>  
> @@ -975,6 +977,40 @@ static void pnv_psi_register_types(void)
>  
>  type_init(pnv_psi_register_types);
>  
> +#ifdef PSI_DEBUG
> +static void psi_regs_pic_print_info(uint64_t *regs, uint32_t nr_regs,
> +                                    GString *buf) {
> +    uint i, prev_idx = -1;
> +    uint64_t  reg1, prev_reg1 = -1;
> +    uint64_t  reg2, prev_reg2 = -1;
> +    uint64_t  reg3, prev_reg3 = -1;
> +    uint64_t  reg4, prev_reg4 = -1;

Very minor nitpick, 2 spaces in the declaration between type and name.
checkpatch doesn't point it out, so it's okay with me.

Looks good to me overall. Please just see if the #undef was intentional,
if so:

Reviewed-by: Aditya Gupta <adityag@linux.ibm.com>

Thanks,
- Aditya G

Re: [PATCH v2 7/7] hw/ppc: Add VMSTATE information to PnvPsi

[Caleb Schlossin]

On 12/16/25 6:58 AM, Aditya Gupta wrote:
> Hello Caleb,
> 
> On 25/12/15 11:18AM, Caleb Schlossin wrote:
>> <...snip...>
>>
>> --- a/hw/ppc/pnv_psi.c
>> +++ b/hw/ppc/pnv_psi.c
>> @@ -25,6 +25,7 @@
>>  #include "qemu/module.h"
>>  #include "system/reset.h"
>>  #include "qapi/error.h"
>> +#include "migration/vmstate.h"
>>  
>>  
>>  #include "hw/ppc/fdt.h"
>> @@ -35,6 +36,8 @@
>>  
>>  #include <libfdt.h>
>>  
>> +#undef PSI_DEBUG
>> +
> 
> Is this intended or got left over from debugging ?

This was indented to aid future debug, if needed.

> 
>>  #define PSIHB_XSCOM_FIR_RW      0x00
>>  #define PSIHB_XSCOM_FIR_AND     0x01
>>  #define PSIHB_XSCOM_FIR_OR      0x02
>> @@ -130,12 +133,11 @@ static void pnv_psi_set_bar(PnvPsi *psi, uint64_t bar)
>>  {
>>      PnvPsiClass *ppc = PNV_PSI_GET_CLASS(psi);
>>      MemoryRegion *sysmem = get_system_memory();
>> -    uint64_t old = psi->regs[PSIHB_XSCOM_BAR];
>>  
>>      psi->regs[PSIHB_XSCOM_BAR] = bar & (ppc->bar_mask | PSIHB_BAR_EN);
>>  
>>      /* Update MR, always remove it first */
>> -    if (old & PSIHB_BAR_EN) {
>> +    if (memory_region_is_mapped(&psi->regs_mr)) {
>>          memory_region_del_subregion(sysmem, &psi->regs_mr);
>>      }
>>  
>> @@ -975,6 +977,40 @@ static void pnv_psi_register_types(void)
>>  
>>  type_init(pnv_psi_register_types);
>>  
>> +#ifdef PSI_DEBUG
>> +static void psi_regs_pic_print_info(uint64_t *regs, uint32_t nr_regs,
>> +                                    GString *buf) {
>> +    uint i, prev_idx = -1;
>> +    uint64_t  reg1, prev_reg1 = -1;
>> +    uint64_t  reg2, prev_reg2 = -1;
>> +    uint64_t  reg3, prev_reg3 = -1;
>> +    uint64_t  reg4, prev_reg4 = -1;
> 
> Very minor nitpick, 2 spaces in the declaration between type and name.
> checkpatch doesn't point it out, so it's okay with me.
> 
> Looks good to me overall. Please just see if the #undef was intentional,
> if so:
> 
> Reviewed-by: Aditya Gupta <adityag@linux.ibm.com>
> 
> Thanks,
> - Aditya G
> 

Re: [PATCH v2 7/7] hw/ppc: Add VMSTATE information to PnvPsi

[Aditya Gupta]

On 25/12/16 07:35AM, Caleb Schlossin wrote:
> 
> 
> On 12/16/25 6:58 AM, Aditya Gupta wrote:
> > Hello Caleb,
> > 
> > On 25/12/15 11:18AM, Caleb Schlossin wrote:
> >> <...snip...>
> >>
> >> --- a/hw/ppc/pnv_psi.c
> >> +++ b/hw/ppc/pnv_psi.c
> >> @@ -25,6 +25,7 @@
> >>  #include "qemu/module.h"
> >>  #include "system/reset.h"
> >>  #include "qapi/error.h"
> >> +#include "migration/vmstate.h"
> >>  
> >>  
> >>  #include "hw/ppc/fdt.h"
> >> @@ -35,6 +36,8 @@
> >>  
> >>  #include <libfdt.h>
> >>  
> >> +#undef PSI_DEBUG
> >> +
> > 
> > Is this intended or got left over from debugging ?
> 
> This was indented to aid future debug, if needed.
> 

If so, i believe this should be removed.

With this we just unconditionally noped out the #ifdef PSI_DEBUG
portions in this file, even if the user explicitly compiles with PSI_DEBUG

What do you say ?

- Aditya G

> > 
> >>  #define PSIHB_XSCOM_FIR_RW      0x00
> >>  #define PSIHB_XSCOM_FIR_AND     0x01
> >>  #define PSIHB_XSCOM_FIR_OR      0x02
> >> @@ -130,12 +133,11 @@ static void pnv_psi_set_bar(PnvPsi *psi, uint64_t bar)
> >>  {
> >>      PnvPsiClass *ppc = PNV_PSI_GET_CLASS(psi);
> >>      MemoryRegion *sysmem = get_system_memory();
> >> -    uint64_t old = psi->regs[PSIHB_XSCOM_BAR];
> >>  
> >>      psi->regs[PSIHB_XSCOM_BAR] = bar & (ppc->bar_mask | PSIHB_BAR_EN);
> >>  
> >>      /* Update MR, always remove it first */
> >> -    if (old & PSIHB_BAR_EN) {
> >> +    if (memory_region_is_mapped(&psi->regs_mr)) {
> >>          memory_region_del_subregion(sysmem, &psi->regs_mr);
> >>      }
> >>  
> >> @@ -975,6 +977,40 @@ static void pnv_psi_register_types(void)
> >>  
> >>  type_init(pnv_psi_register_types);
> >>  
> >> +#ifdef PSI_DEBUG
> >> +static void psi_regs_pic_print_info(uint64_t *regs, uint32_t nr_regs,
> >> +                                    GString *buf) {
> >> +    uint i, prev_idx = -1;
> >> +    uint64_t  reg1, prev_reg1 = -1;
> >> +    uint64_t  reg2, prev_reg2 = -1;
> >> +    uint64_t  reg3, prev_reg3 = -1;
> >> +    uint64_t  reg4, prev_reg4 = -1;
> > 
> > Very minor nitpick, 2 spaces in the declaration between type and name.
> > checkpatch doesn't point it out, so it's okay with me.
> > 
> > Looks good to me overall. Please just see if the #undef was intentional,
> > if so:
> > 
> > Reviewed-by: Aditya Gupta <adityag@linux.ibm.com>
> > 
> > Thanks,
> > - Aditya G
> > 
> 

Re: [PATCH v2 7/7] hw/ppc: Add VMSTATE information to PnvPsi

[Caleb Schlossin]

On 12/16/25 8:36 AM, Aditya Gupta wrote:
> On 25/12/16 07:35AM, Caleb Schlossin wrote:
>>
>>
>> On 12/16/25 6:58 AM, Aditya Gupta wrote:
>>> Hello Caleb,
>>>
>>> On 25/12/15 11:18AM, Caleb Schlossin wrote:
>>>> <...snip...>
>>>>
>>>> --- a/hw/ppc/pnv_psi.c
>>>> +++ b/hw/ppc/pnv_psi.c
>>>> @@ -25,6 +25,7 @@
>>>>  #include "qemu/module.h"
>>>>  #include "system/reset.h"
>>>>  #include "qapi/error.h"
>>>> +#include "migration/vmstate.h"
>>>>  
>>>>  
>>>>  #include "hw/ppc/fdt.h"
>>>> @@ -35,6 +36,8 @@
>>>>  
>>>>  #include <libfdt.h>
>>>>  
>>>> +#undef PSI_DEBUG
>>>> +
>>>
>>> Is this intended or got left over from debugging ?
>>
>> This was indented to aid future debug, if needed.
>>
> 
> If so, i believe this should be removed.
> 
> With this we just unconditionally noped out the #ifdef PSI_DEBUG
> portions in this file, even if the user explicitly compiles with PSI_DEBUG
> 
> What do you say ?
> 
> - Aditya G

Sounds good, I'll remove it and send up a new set of patches.

> 
>>>
>>>>  #define PSIHB_XSCOM_FIR_RW      0x00
>>>>  #define PSIHB_XSCOM_FIR_AND     0x01
>>>>  #define PSIHB_XSCOM_FIR_OR      0x02
>>>> @@ -130,12 +133,11 @@ static void pnv_psi_set_bar(PnvPsi *psi, uint64_t bar)
>>>>  {
>>>>      PnvPsiClass *ppc = PNV_PSI_GET_CLASS(psi);
>>>>      MemoryRegion *sysmem = get_system_memory();
>>>> -    uint64_t old = psi->regs[PSIHB_XSCOM_BAR];
>>>>  
>>>>      psi->regs[PSIHB_XSCOM_BAR] = bar & (ppc->bar_mask | PSIHB_BAR_EN);
>>>>  
>>>>      /* Update MR, always remove it first */
>>>> -    if (old & PSIHB_BAR_EN) {
>>>> +    if (memory_region_is_mapped(&psi->regs_mr)) {
>>>>          memory_region_del_subregion(sysmem, &psi->regs_mr);
>>>>      }
>>>>  
>>>> @@ -975,6 +977,40 @@ static void pnv_psi_register_types(void)
>>>>  
>>>>  type_init(pnv_psi_register_types);
>>>>  
>>>> +#ifdef PSI_DEBUG
>>>> +static void psi_regs_pic_print_info(uint64_t *regs, uint32_t nr_regs,
>>>> +                                    GString *buf) {
>>>> +    uint i, prev_idx = -1;
>>>> +    uint64_t  reg1, prev_reg1 = -1;
>>>> +    uint64_t  reg2, prev_reg2 = -1;
>>>> +    uint64_t  reg3, prev_reg3 = -1;
>>>> +    uint64_t  reg4, prev_reg4 = -1;
>>>
>>> Very minor nitpick, 2 spaces in the declaration between type and name.
>>> checkpatch doesn't point it out, so it's okay with me.
>>>
>>> Looks good to me overall. Please just see if the #undef was intentional,
>>> if so:
>>>
>>> Reviewed-by: Aditya Gupta <adityag@linux.ibm.com>
>>>
>>> Thanks,
>>> - Aditya G
>>>
>>

Re: [PATCH v2 0/7] hw/ppc: Snapshot support for several ppc devices

[Aditya Gupta]

On 25/12/15 11:18AM, Caleb Schlossin wrote:
> Addressing comments from V1 review:
> 
> Updates in V2:
> - Added new patch set for PnvPsi support as it fits with the rest
> - Added vmstate support for Power8 and Power9 for LPC
> - Fixed pnv_core.c commit message
> 
> Tested:
> passed make check

Looks good to me.

Had one comment in PSI about #undef. With that sorted:

For the series:
Reviewed-by: Aditya Gupta <adityag@linux.ibm.com>

Thanks,
- Aditya G