[PATCH 0/6] spapr: Drop some users of qdev_get_machine()

[PATCH 0/6] spapr: Drop some users of qdev_get_machine()

[Greg Kurz]

Accessing the machine state using the global qdev_get_machine() function
isn't generally recommended. In a bunch of places in the sPAPR code, the
machine state can be obtained from the caller actually.

Greg Kurz (6):
  spapr: Add an "spapr" property to sPAPR CPU core
  spapr: Add an "spapr" property to sPAPR PHB
  spapr: Pass sPAPR machine state down to spapr_pci_switch_vga()
  spapr: Don't use qdev_get_machine() in spapr_msi_write()
  spapr: Pass sPAPR machine state to some RTAS events handling functions
  target/ppc: Add mce_req_event() handler to PPCVirtualHypervisorClass

 include/hw/pci-host/spapr.h     |  1 +
 include/hw/ppc/spapr.h          |  5 +++--
 include/hw/ppc/spapr_cpu_core.h |  2 ++
 target/ppc/cpu.h                |  2 ++
 hw/ppc/spapr.c                  |  9 +++++++++
 hw/ppc/spapr_cpu_core.c         | 17 +++++++----------
 hw/ppc/spapr_events.c           | 26 +++++++++++++-------------
 hw/ppc/spapr_hcall.c            |  7 ++++---
 hw/ppc/spapr_pci.c              | 22 +++++++++-------------
 hw/ppc/spapr_pci_nvlink2.c      |  2 +-
 target/ppc/kvm.c                |  9 +++++++--
 11 files changed, 58 insertions(+), 44 deletions(-)

-- 
2.26.2

[PATCH 1/6] spapr: Add an "spapr" property to sPAPR CPU core

[Greg Kurz]

The sPAPR CPU core device can only work with pseries machine types.
This is currently checked in the realize function with a dynamic
cast of qdev_get_machine(). Some other places also need to reach
out to the machine using qdev_get_machine().

Make this dependency explicit by introducing an "spapr" link
property which officialy points to the machine. This link is
set by pseries machine types only in the pre-plug handler. This
allows to drop some users of qdev_get_machine().

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 include/hw/ppc/spapr_cpu_core.h |  2 ++
 hw/ppc/spapr.c                  |  4 ++++
 hw/ppc/spapr_cpu_core.c         | 17 +++++++----------
 3 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
index dab3dfc76c0a..0969b29fd96c 100644
--- a/include/hw/ppc/spapr_cpu_core.h
+++ b/include/hw/ppc/spapr_cpu_core.h
@@ -10,6 +10,7 @@
 #define HW_SPAPR_CPU_CORE_H
 
 #include "hw/cpu/core.h"
+#include "hw/ppc/spapr.h"
 #include "hw/qdev-core.h"
 #include "target/ppc/cpu-qom.h"
 #include "target/ppc/cpu.h"
@@ -24,6 +25,7 @@ OBJECT_DECLARE_TYPE(SpaprCpuCore, SpaprCpuCoreClass,
 struct SpaprCpuCore {
     /*< private >*/
     CPUCore parent_obj;
+    SpaprMachineState *spapr;
 
     /*< public >*/
     PowerPCCPU **threads;
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index d1dcf3ab2c94..4cc51723c62e 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3816,6 +3816,10 @@ static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     int index;
     unsigned int smp_threads = machine->smp.threads;
 
+    /* Required by spapr_cpu_core_realize() */
+    object_property_set_link(OBJECT(dev), "spapr", OBJECT(hotplug_dev),
+                             &error_abort);
+
     if (dev->hotplugged && !mc->has_hotpluggable_cpus) {
         error_setg(errp, "CPU hotplug not supported for this machine");
         return;
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 2f7dc3c23ded..dec09367e4a0 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -25,14 +25,13 @@
 #include "sysemu/hw_accel.h"
 #include "qemu/error-report.h"
 
-static void spapr_reset_vcpu(PowerPCCPU *cpu)
+static void spapr_reset_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr)
 {
     CPUState *cs = CPU(cpu);
     CPUPPCState *env = &cpu->env;
     PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
     SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
     target_ulong lpcr;
-    SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
 
     cpu_reset(cs);
 
@@ -186,7 +185,7 @@ static void spapr_unrealize_vcpu(PowerPCCPU *cpu, SpaprCpuCore *sc)
     if (!sc->pre_3_0_migration) {
         vmstate_unregister(NULL, &vmstate_spapr_cpu_state, cpu->machine_data);
     }
-    spapr_irq_cpu_intc_destroy(SPAPR_MACHINE(qdev_get_machine()), cpu);
+    spapr_irq_cpu_intc_destroy(sc->spapr, cpu);
     qdev_unrealize(DEVICE(cpu));
 }
 
@@ -200,7 +199,7 @@ static void spapr_cpu_core_reset(DeviceState *dev)
     int i;
 
     for (i = 0; i < cc->nr_threads; i++) {
-        spapr_reset_vcpu(sc->threads[i]);
+        spapr_reset_vcpu(sc->threads[i], sc->spapr);
     }
 }
 
@@ -314,16 +313,12 @@ err:
 
 static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
 {
-    /* We don't use SPAPR_MACHINE() in order to exit gracefully if the user
-     * tries to add a sPAPR CPU core to a non-pseries machine.
-     */
-    SpaprMachineState *spapr =
-        (SpaprMachineState *) object_dynamic_cast(qdev_get_machine(),
-                                                  TYPE_SPAPR_MACHINE);
     SpaprCpuCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
+    SpaprMachineState *spapr = sc->spapr;
     CPUCore *cc = CPU_CORE(OBJECT(dev));
     int i;
 
+    /* Set in spapr_core_pre_plug() */
     if (!spapr) {
         error_setg(errp, TYPE_SPAPR_CPU_CORE " needs a pseries machine");
         return;
@@ -345,6 +340,8 @@ static Property spapr_cpu_core_properties[] = {
     DEFINE_PROP_INT32("node-id", SpaprCpuCore, node_id, CPU_UNSET_NUMA_NODE_ID),
     DEFINE_PROP_BOOL("pre-3.0-migration", SpaprCpuCore, pre_3_0_migration,
                      false),
+    DEFINE_PROP_LINK("spapr", SpaprCpuCore, spapr, TYPE_SPAPR_MACHINE,
+                     SpaprMachineState *),
     DEFINE_PROP_END_OF_LIST()
 };
 
-- 
2.26.2

[PATCH 2/6] spapr: Add an "spapr" property to sPAPR PHB

[Greg Kurz]

The sPAPR PHB device can only work with pseries machine types. This
is currently checked in the realize function with a dynamic cast of
qdev_get_machine(). Some other places also need to reach out to the
machine using qdev_get_machine().

Make this dependency explicit by introducing an "spapr" link
property which officialy points to the machine. This link is
set by pseries machine types only in the pre-plug handler. This
allows to drop some users of qdev_get_machine().

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 include/hw/pci-host/spapr.h |  1 +
 hw/ppc/spapr.c              |  4 ++++
 hw/ppc/spapr_pci.c          | 17 +++++++----------
 hw/ppc/spapr_pci_nvlink2.c  |  2 +-
 4 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
index 4f58f0223b56..622a4f1a07c6 100644
--- a/include/hw/pci-host/spapr.h
+++ b/include/hw/pci-host/spapr.h
@@ -76,6 +76,7 @@ struct SpaprPhbState {
     SpaprPciMsiMig *msi_devs;
 
     QLIST_ENTRY(SpaprPhbState) list;
+    SpaprMachineState *spapr;
 
     bool ddw_enabled;
     uint64_t page_size_mask;
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 4cc51723c62e..aca7d7af283a 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3892,6 +3892,10 @@ static bool spapr_phb_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     const unsigned windows_supported = spapr_phb_windows_supported(sphb);
     SpaprDrc *drc;
 
+    /* Required by spapr_phb_realize() */
+    object_property_set_link(OBJECT(dev), "spapr", OBJECT(hotplug_dev),
+                             &error_abort);
+
     if (dev->hotplugged && !smc->dr_phb_enabled) {
         error_setg(errp, "PHB hotplug not supported for this machine");
         return false;
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index e946bd5055cc..2d9b88b93122 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -722,7 +722,7 @@ static void pci_spapr_set_irq(void *opaque, int irq_num, int level)
      * corresponding qemu_irq.
      */
     SpaprPhbState *phb = opaque;
-    SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
+    SpaprMachineState *spapr = phb->spapr;
 
     trace_spapr_pci_lsi_set(phb->dtbusname, irq_num, phb->lsi_table[irq_num].irq);
     qemu_set_irq(spapr_qirq(spapr, phb->lsi_table[irq_num].irq), level);
@@ -1743,10 +1743,10 @@ static void spapr_phb_finalizefn(Object *obj)
 
 static void spapr_phb_unrealize(DeviceState *dev)
 {
-    SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
     SysBusDevice *s = SYS_BUS_DEVICE(dev);
     PCIHostState *phb = PCI_HOST_BRIDGE(s);
     SpaprPhbState *sphb = SPAPR_PCI_HOST_BRIDGE(phb);
+    SpaprMachineState *spapr = sphb->spapr;
     SpaprTceTable *tcet;
     int i;
     const unsigned windows_supported = spapr_phb_windows_supported(sphb);
@@ -1817,15 +1817,9 @@ static void spapr_phb_destroy_msi(gpointer opaque)
 static void spapr_phb_realize(DeviceState *dev, Error **errp)
 {
     ERRP_GUARD();
-    /* We don't use SPAPR_MACHINE() in order to exit gracefully if the user
-     * tries to add a sPAPR PHB to a non-pseries machine.
-     */
-    SpaprMachineState *spapr =
-        (SpaprMachineState *) object_dynamic_cast(qdev_get_machine(),
-                                                  TYPE_SPAPR_MACHINE);
-    SpaprMachineClass *smc = spapr ? SPAPR_MACHINE_GET_CLASS(spapr) : NULL;
     SysBusDevice *s = SYS_BUS_DEVICE(dev);
     SpaprPhbState *sphb = SPAPR_PCI_HOST_BRIDGE(s);
+    SpaprMachineState *spapr = sphb->spapr;
     PCIHostState *phb = PCI_HOST_BRIDGE(s);
     MachineState *ms = MACHINE(spapr);
     char *namebuf;
@@ -1835,6 +1829,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
     SpaprTceTable *tcet;
     const unsigned windows_supported = spapr_phb_windows_supported(sphb);
 
+    /* Set in spapr_phb_pre_plug() */
     if (!spapr) {
         error_setg(errp, TYPE_SPAPR_PCI_HOST_BRIDGE " needs a pseries machine");
         return;
@@ -1986,7 +1981,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
     for (i = 0; i < PCI_NUM_PINS; i++) {
         int irq = SPAPR_IRQ_PCI_LSI + sphb->index * PCI_NUM_PINS + i;
 
-        if (smc->legacy_irq_allocation) {
+        if (SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) {
             irq = spapr_irq_findone(spapr, errp);
             if (irq < 0) {
                 error_prepend(errp, "can't allocate LSIs: ");
@@ -2109,6 +2104,8 @@ static Property spapr_phb_properties[] = {
     DEFINE_PROP_UINT64("atsd", SpaprPhbState, nv2_atsd_win_addr, 0),
     DEFINE_PROP_BOOL("pre-5.1-associativity", SpaprPhbState,
                      pre_5_1_assoc, false),
+    DEFINE_PROP_LINK("spapr", SpaprPhbState, spapr, TYPE_SPAPR_MACHINE,
+                     SpaprMachineState *),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/ppc/spapr_pci_nvlink2.c b/hw/ppc/spapr_pci_nvlink2.c
index 8ef9b40a18dc..ce62318a6d73 100644
--- a/hw/ppc/spapr_pci_nvlink2.c
+++ b/hw/ppc/spapr_pci_nvlink2.c
@@ -368,7 +368,7 @@ void spapr_phb_nvgpu_ram_populate_dt(SpaprPhbState *sphb, void *fdt)
         _FDT((fdt_setprop_string(fdt, off, "device_type", "memory")));
         _FDT((fdt_setprop(fdt, off, "reg", mem_reg, sizeof(mem_reg))));
 
-        spapr_numa_write_associativity_dt(SPAPR_MACHINE(qdev_get_machine()),
+        spapr_numa_write_associativity_dt(sphb->spapr,
                                           fdt, off, nvslot->numa_id);
 
         _FDT((fdt_setprop_string(fdt, off, "compatible",
-- 
2.26.2

[PATCH 3/6] spapr: Pass sPAPR machine state down to spapr_pci_switch_vga()

[Greg Kurz]

This allows to drop a user of qdev_get_machine().

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 include/hw/ppc/spapr.h | 2 +-
 hw/ppc/spapr_hcall.c   | 7 ++++---
 hw/ppc/spapr_pci.c     | 3 +--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index b7ced9faebf5..e0f10f252c08 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -834,7 +834,7 @@ int spapr_dma_dt(void *fdt, int node_off, const char *propname,
                  uint32_t liobn, uint64_t window, uint32_t size);
 int spapr_tcet_dma_dt(void *fdt, int node_off, const char *propname,
                       SpaprTceTable *tcet);
-void spapr_pci_switch_vga(bool big_endian);
+void spapr_pci_switch_vga(SpaprMachineState *spapr, bool big_endian);
 void spapr_hotplug_req_add_by_index(SpaprDrc *drc);
 void spapr_hotplug_req_remove_by_index(SpaprDrc *drc);
 void spapr_hotplug_req_add_by_count(SpaprDrcType drc_type,
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 1d8e8e6a888f..c0ea0bd5794b 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1351,6 +1351,7 @@ static target_ulong h_logical_dcbf(PowerPCCPU *cpu, SpaprMachineState *spapr,
 }
 
 static target_ulong h_set_mode_resource_le(PowerPCCPU *cpu,
+                                           SpaprMachineState *spapr,
                                            target_ulong mflags,
                                            target_ulong value1,
                                            target_ulong value2)
@@ -1365,12 +1366,12 @@ static target_ulong h_set_mode_resource_le(PowerPCCPU *cpu,
     switch (mflags) {
     case H_SET_MODE_ENDIAN_BIG:
         spapr_set_all_lpcrs(0, LPCR_ILE);
-        spapr_pci_switch_vga(true);
+        spapr_pci_switch_vga(spapr, true);
         return H_SUCCESS;
 
     case H_SET_MODE_ENDIAN_LITTLE:
         spapr_set_all_lpcrs(LPCR_ILE, LPCR_ILE);
-        spapr_pci_switch_vga(false);
+        spapr_pci_switch_vga(spapr, false);
         return H_SUCCESS;
     }
 
@@ -1411,7 +1412,7 @@ static target_ulong h_set_mode(PowerPCCPU *cpu, SpaprMachineState *spapr,
 
     switch (resource) {
     case H_SET_MODE_RESOURCE_LE:
-        ret = h_set_mode_resource_le(cpu, args[0], args[2], args[3]);
+        ret = h_set_mode_resource_le(cpu, spapr, args[0], args[2], args[3]);
         break;
     case H_SET_MODE_RESOURCE_ADDR_TRANS_MODE:
         ret = h_set_mode_resource_addr_trans_mode(cpu, args[0],
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 2d9b88b93122..149bf4c21d22 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -2490,9 +2490,8 @@ static int spapr_switch_one_vga(DeviceState *dev, void *opaque)
     return 0;
 }
 
-void spapr_pci_switch_vga(bool big_endian)
+void spapr_pci_switch_vga(SpaprMachineState *spapr, bool big_endian)
 {
-    SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
     SpaprPhbState *sphb;
 
     /*
-- 
2.26.2

[PATCH 4/6] spapr: Don't use qdev_get_machine() in spapr_msi_write()

[Greg Kurz]

spapr_phb_realize() passes the sPAPR machine state as opaque data
for the I/O callbacks:

memory_region_init_io(&sphb->msiwindow, OBJECT(sphb), &spapr_msi_ops, spapr,
                                                                      ^^^^^
                      "msi", msi_window_size);

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 149bf4c21d22..890a0cc1d6af 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -747,7 +747,7 @@ static PCIINTxRoute spapr_route_intx_pin_to_irq(void *opaque, int pin)
 static void spapr_msi_write(void *opaque, hwaddr addr,
                             uint64_t data, unsigned size)
 {
-    SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
+    SpaprMachineState *spapr = opaque;
     uint32_t irq = data;
 
     trace_spapr_pci_msi_write(addr, data, irq);
-- 
2.26.2

[PATCH 5/6] spapr: Pass sPAPR machine state to some RTAS events handling functions

[Greg Kurz]

Some functions in hw/ppc/spapr_events.c get a pointer to the machine
state using qdev_get_machine(). Convert them to get it from their
caller when possible.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_events.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index 1add53547ec3..3f37b49fd8ad 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -480,9 +480,8 @@ static SpaprEventLogEntry *rtas_event_log_dequeue(SpaprMachineState *spapr,
     return entry;
 }
 
-static bool rtas_event_log_contains(uint32_t event_mask)
+static bool rtas_event_log_contains(SpaprMachineState *spapr, uint32_t event_mask)
 {
-    SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
     SpaprEventLogEntry *entry = NULL;
 
     QTAILQ_FOREACH(entry, &spapr->pending_events, next) {
@@ -509,10 +508,10 @@ static void spapr_init_v6hdr(struct rtas_event_log_v6 *v6hdr)
     v6hdr->company = cpu_to_be32(RTAS_LOG_V6_COMPANY_IBM);
 }
 
-static void spapr_init_maina(struct rtas_event_log_v6_maina *maina,
+static void spapr_init_maina(SpaprMachineState *spapr,
+                             struct rtas_event_log_v6_maina *maina,
                              int section_count)
 {
-    SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
     struct tm tm;
     int year;
 
@@ -560,7 +559,7 @@ static void spapr_powerdown_req(Notifier *n, void *opaque)
     entry->extended_length = sizeof(*new_epow);
 
     spapr_init_v6hdr(v6hdr);
-    spapr_init_maina(maina, 3 /* Main-A, Main-B and EPOW */);
+    spapr_init_maina(spapr, maina, 3 /* Main-A, Main-B and EPOW */);
 
     mainb->hdr.section_id = cpu_to_be16(RTAS_LOG_V6_SECTION_ID_MAINB);
     mainb->hdr.section_length = cpu_to_be16(sizeof(*mainb));
@@ -613,7 +612,7 @@ static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
     entry->extended_length = sizeof(*new_hp);
 
     spapr_init_v6hdr(v6hdr);
-    spapr_init_maina(maina, 3 /* Main-A, Main-B, HP */);
+    spapr_init_maina(spapr, maina, 3 /* Main-A, Main-B, HP */);
 
     mainb->hdr.section_id = cpu_to_be16(RTAS_LOG_V6_SECTION_ID_MAINB);
     mainb->hdr.section_length = cpu_to_be16(sizeof(*mainb));
@@ -808,9 +807,9 @@ static uint32_t spapr_mce_get_elog_type(PowerPCCPU *cpu, bool recovered,
     return summary;
 }
 
-static void spapr_mce_dispatch_elog(PowerPCCPU *cpu, bool recovered)
+static void spapr_mce_dispatch_elog(SpaprMachineState *spapr, PowerPCCPU *cpu,
+                                    bool recovered)
 {
-    SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
     CPUState *cs = CPU(cpu);
     CPUPPCState *env = &cpu->env;
     uint64_t rtas_addr;
@@ -927,7 +926,7 @@ void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered)
         warn_report("Received a fwnmi while migration was in progress");
     }
 
-    spapr_mce_dispatch_elog(cpu, recovered);
+    spapr_mce_dispatch_elog(spapr, cpu, recovered);
 }
 
 static void check_exception(PowerPCCPU *cpu, SpaprMachineState *spapr,
@@ -980,7 +979,7 @@ static void check_exception(PowerPCCPU *cpu, SpaprMachineState *spapr,
      * interrupts.
      */
     for (i = 0; i < EVENT_CLASS_MAX; i++) {
-        if (rtas_event_log_contains(EVENT_CLASS_MASK(i))) {
+        if (rtas_event_log_contains(spapr, EVENT_CLASS_MASK(i))) {
             const SpaprEventSource *source =
                 spapr_event_sources_get_source(spapr->event_sources, i);
 
@@ -1007,7 +1006,7 @@ static void event_scan(PowerPCCPU *cpu, SpaprMachineState *spapr,
     }
 
     for (i = 0; i < EVENT_CLASS_MAX; i++) {
-        if (rtas_event_log_contains(EVENT_CLASS_MASK(i))) {
+        if (rtas_event_log_contains(spapr, EVENT_CLASS_MASK(i))) {
             const SpaprEventSource *source =
                 spapr_event_sources_get_source(spapr->event_sources, i);
 
-- 
2.26.2

[PATCH 6/6] target/ppc: Add mce_req_event() handler to PPCVirtualHypervisorClass

[Greg Kurz]

kvm_handle_nmi() directly calls spapr_mce_req_event() which is machine
level code. Apart from being ugly, this forces spapr_mce_req_event()
to rely on qdev_get_machine() to get a pointer to the machine state.
This is a bit unfortunate since POWER CPUs have a backlink to the
virtual hypervisor, which happens to be the machine itself with
sPAPR.

Turn spapr_mce_req_event() into a PPC virtual hypervisor operation,
and adapt kvm_handle_nmi() to call it as such.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 include/hw/ppc/spapr.h | 3 ++-
 target/ppc/cpu.h       | 2 ++
 hw/ppc/spapr.c         | 1 +
 hw/ppc/spapr_events.c  | 5 +++--
 target/ppc/kvm.c       | 9 +++++++--
 5 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index e0f10f252c08..476c5b809794 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -852,7 +852,8 @@ void spapr_clear_pending_hotplug_events(SpaprMachineState *spapr);
 int spapr_max_server_number(SpaprMachineState *spapr);
 void spapr_store_hpte(PowerPCCPU *cpu, hwaddr ptex,
                       uint64_t pte0, uint64_t pte1);
-void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered);
+void spapr_mce_req_event(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu,
+                         bool recovered);
 bool spapr_machine_using_legacy_numa(SpaprMachineState *spapr);
 
 /* DRC callbacks. */
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 2609e4082ed8..5bac68aec826 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1219,6 +1219,8 @@ struct PPCVirtualHypervisorClass {
     target_ulong (*encode_hpt_for_kvm_pr)(PPCVirtualHypervisor *vhyp);
     void (*cpu_exec_enter)(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu);
     void (*cpu_exec_exit)(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu);
+    void (*mce_req_event)(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu,
+                          bool recovered);
 };
 
 #define TYPE_PPC_VIRTUAL_HYPERVISOR "ppc-virtual-hypervisor"
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index aca7d7af283a..09fc605f11ba 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -4441,6 +4441,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     vhc->encode_hpt_for_kvm_pr = spapr_encode_hpt_for_kvm_pr;
     vhc->cpu_exec_enter = spapr_cpu_exec_enter;
     vhc->cpu_exec_exit = spapr_cpu_exec_exit;
+    vhc->mce_req_event = spapr_mce_req_event;
     xic->ics_get = spapr_ics_get;
     xic->ics_resend = spapr_ics_resend;
     xic->icp_get = spapr_icp_get;
diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index 3f37b49fd8ad..8e988eb939da 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -868,9 +868,10 @@ static void spapr_mce_dispatch_elog(SpaprMachineState *spapr, PowerPCCPU *cpu,
     ppc_cpu_do_fwnmi_machine_check(cs, spapr->fwnmi_machine_check_addr);
 }
 
-void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered)
+void spapr_mce_req_event(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu,
+                         bool recovered)
 {
-    SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
+    SpaprMachineState *spapr = SPAPR_MACHINE(vhyp);
     CPUState *cs = CPU(cpu);
     int ret;
     Error *local_err = NULL;
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index daf690a67820..ba6edf178a39 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -2816,10 +2816,15 @@ int kvm_handle_nmi(PowerPCCPU *cpu, struct kvm_run *run)
 {
     uint16_t flags = run->flags & KVM_RUN_PPC_NMI_DISP_MASK;
 
-    cpu_synchronize_state(CPU(cpu));
+    if (cpu->vhyp) {
+        PPCVirtualHypervisorClass *vhc =
+            PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
 
-    spapr_mce_req_event(cpu, flags == KVM_RUN_PPC_NMI_DISP_FULLY_RECOV);
+        cpu_synchronize_state(CPU(cpu));
 
+        vhc->mce_req_event(cpu->vhyp, cpu,
+                           flags == KVM_RUN_PPC_NMI_DISP_FULLY_RECOV);
+    }
     return 0;
 }
 #endif
-- 
2.26.2

Re: [PATCH 1/6] spapr: Add an "spapr" property to sPAPR CPU core

[Philippe Mathieu-Daudé]

On 12/9/20 6:00 PM, Greg Kurz wrote:
> The sPAPR CPU core device can only work with pseries machine types.
> This is currently checked in the realize function with a dynamic
> cast of qdev_get_machine(). Some other places also need to reach
> out to the machine using qdev_get_machine().
> 
> Make this dependency explicit by introducing an "spapr" link
> property which officialy points to the machine. This link is
> set by pseries machine types only in the pre-plug handler. This
> allows to drop some users of qdev_get_machine().
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  include/hw/ppc/spapr_cpu_core.h |  2 ++
>  hw/ppc/spapr.c                  |  4 ++++
>  hw/ppc/spapr_cpu_core.c         | 17 +++++++----------
>  3 files changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> index dab3dfc76c0a..0969b29fd96c 100644
> --- a/include/hw/ppc/spapr_cpu_core.h
> +++ b/include/hw/ppc/spapr_cpu_core.h
> @@ -10,6 +10,7 @@
>  #define HW_SPAPR_CPU_CORE_H
>  
>  #include "hw/cpu/core.h"
> +#include "hw/ppc/spapr.h"
>  #include "hw/qdev-core.h"
>  #include "target/ppc/cpu-qom.h"
>  #include "target/ppc/cpu.h"
> @@ -24,6 +25,7 @@ OBJECT_DECLARE_TYPE(SpaprCpuCore, SpaprCpuCoreClass,
>  struct SpaprCpuCore {
>      /*< private >*/
>      CPUCore parent_obj;
> +    SpaprMachineState *spapr;
>  
>      /*< public >*/
>      PowerPCCPU **threads;
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index d1dcf3ab2c94..4cc51723c62e 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3816,6 +3816,10 @@ static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      int index;
>      unsigned int smp_threads = machine->smp.threads;
>  
> +    /* Required by spapr_cpu_core_realize() */
> +    object_property_set_link(OBJECT(dev), "spapr", OBJECT(hotplug_dev),
> +                             &error_abort);
> +
>      if (dev->hotplugged && !mc->has_hotpluggable_cpus) {
>          error_setg(errp, "CPU hotplug not supported for this machine");
>          return;
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 2f7dc3c23ded..dec09367e4a0 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -25,14 +25,13 @@
>  #include "sysemu/hw_accel.h"
>  #include "qemu/error-report.h"
>  
> -static void spapr_reset_vcpu(PowerPCCPU *cpu)
> +static void spapr_reset_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr)
>  {
>      CPUState *cs = CPU(cpu);
>      CPUPPCState *env = &cpu->env;
>      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>      SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
>      target_ulong lpcr;
> -    SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>  
>      cpu_reset(cs);
>  
> @@ -186,7 +185,7 @@ static void spapr_unrealize_vcpu(PowerPCCPU *cpu, SpaprCpuCore *sc)
>      if (!sc->pre_3_0_migration) {
>          vmstate_unregister(NULL, &vmstate_spapr_cpu_state, cpu->machine_data);
>      }
> -    spapr_irq_cpu_intc_destroy(SPAPR_MACHINE(qdev_get_machine()), cpu);
> +    spapr_irq_cpu_intc_destroy(sc->spapr, cpu);
>      qdev_unrealize(DEVICE(cpu));
>  }
>  
> @@ -200,7 +199,7 @@ static void spapr_cpu_core_reset(DeviceState *dev)
>      int i;
>  
>      for (i = 0; i < cc->nr_threads; i++) {
> -        spapr_reset_vcpu(sc->threads[i]);
> +        spapr_reset_vcpu(sc->threads[i], sc->spapr);

Why reset() needs access to the machine state, don't
you have it in realize()?

>      }
>  }
>  
> @@ -314,16 +313,12 @@ err:
>  
>  static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>  {
> -    /* We don't use SPAPR_MACHINE() in order to exit gracefully if the user
> -     * tries to add a sPAPR CPU core to a non-pseries machine.
> -     */
> -    SpaprMachineState *spapr =
> -        (SpaprMachineState *) object_dynamic_cast(qdev_get_machine(),
> -                                                  TYPE_SPAPR_MACHINE);
>      SpaprCpuCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> +    SpaprMachineState *spapr = sc->spapr;
>      CPUCore *cc = CPU_CORE(OBJECT(dev));
>      int i;
>  
> +    /* Set in spapr_core_pre_plug() */
>      if (!spapr) {
>          error_setg(errp, TYPE_SPAPR_CPU_CORE " needs a pseries machine");
>          return;
> @@ -345,6 +340,8 @@ static Property spapr_cpu_core_properties[] = {
>      DEFINE_PROP_INT32("node-id", SpaprCpuCore, node_id, CPU_UNSET_NUMA_NODE_ID),
>      DEFINE_PROP_BOOL("pre-3.0-migration", SpaprCpuCore, pre_3_0_migration,
>                       false),
> +    DEFINE_PROP_LINK("spapr", SpaprCpuCore, spapr, TYPE_SPAPR_MACHINE,
> +                     SpaprMachineState *),
>      DEFINE_PROP_END_OF_LIST()
>  };
>  
> 

Re: [PATCH 1/6] spapr: Add an "spapr" property to sPAPR CPU core

[Greg Kurz]

On Wed, 9 Dec 2020 18:34:31 +0100
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 12/9/20 6:00 PM, Greg Kurz wrote:
> > The sPAPR CPU core device can only work with pseries machine types.
> > This is currently checked in the realize function with a dynamic
> > cast of qdev_get_machine(). Some other places also need to reach
> > out to the machine using qdev_get_machine().
> > 
> > Make this dependency explicit by introducing an "spapr" link
> > property which officialy points to the machine. This link is
> > set by pseries machine types only in the pre-plug handler. This
> > allows to drop some users of qdev_get_machine().
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  include/hw/ppc/spapr_cpu_core.h |  2 ++
> >  hw/ppc/spapr.c                  |  4 ++++
> >  hw/ppc/spapr_cpu_core.c         | 17 +++++++----------
> >  3 files changed, 13 insertions(+), 10 deletions(-)
> > 
> > diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> > index dab3dfc76c0a..0969b29fd96c 100644
> > --- a/include/hw/ppc/spapr_cpu_core.h
> > +++ b/include/hw/ppc/spapr_cpu_core.h
> > @@ -10,6 +10,7 @@
> >  #define HW_SPAPR_CPU_CORE_H
> >  
> >  #include "hw/cpu/core.h"
> > +#include "hw/ppc/spapr.h"
> >  #include "hw/qdev-core.h"
> >  #include "target/ppc/cpu-qom.h"
> >  #include "target/ppc/cpu.h"
> > @@ -24,6 +25,7 @@ OBJECT_DECLARE_TYPE(SpaprCpuCore, SpaprCpuCoreClass,
> >  struct SpaprCpuCore {
> >      /*< private >*/
> >      CPUCore parent_obj;
> > +    SpaprMachineState *spapr;
> >  
> >      /*< public >*/
> >      PowerPCCPU **threads;
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index d1dcf3ab2c94..4cc51723c62e 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -3816,6 +3816,10 @@ static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >      int index;
> >      unsigned int smp_threads = machine->smp.threads;
> >  
> > +    /* Required by spapr_cpu_core_realize() */
> > +    object_property_set_link(OBJECT(dev), "spapr", OBJECT(hotplug_dev),
> > +                             &error_abort);
> > +
> >      if (dev->hotplugged && !mc->has_hotpluggable_cpus) {
> >          error_setg(errp, "CPU hotplug not supported for this machine");
> >          return;
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 2f7dc3c23ded..dec09367e4a0 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -25,14 +25,13 @@
> >  #include "sysemu/hw_accel.h"
> >  #include "qemu/error-report.h"
> >  
> > -static void spapr_reset_vcpu(PowerPCCPU *cpu)
> > +static void spapr_reset_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr)
> >  {
> >      CPUState *cs = CPU(cpu);
> >      CPUPPCState *env = &cpu->env;
> >      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> >      SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
> >      target_ulong lpcr;
> > -    SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> >  
> >      cpu_reset(cs);
> >  
> > @@ -186,7 +185,7 @@ static void spapr_unrealize_vcpu(PowerPCCPU *cpu, SpaprCpuCore *sc)
> >      if (!sc->pre_3_0_migration) {
> >          vmstate_unregister(NULL, &vmstate_spapr_cpu_state, cpu->machine_data);
> >      }
> > -    spapr_irq_cpu_intc_destroy(SPAPR_MACHINE(qdev_get_machine()), cpu);
> > +    spapr_irq_cpu_intc_destroy(sc->spapr, cpu);
> >      qdev_unrealize(DEVICE(cpu));
> >  }
> >  
> > @@ -200,7 +199,7 @@ static void spapr_cpu_core_reset(DeviceState *dev)
> >      int i;
> >  
> >      for (i = 0; i < cc->nr_threads; i++) {
> > -        spapr_reset_vcpu(sc->threads[i]);
> > +        spapr_reset_vcpu(sc->threads[i], sc->spapr);
> 
> Why reset() needs access to the machine state, don't
> you have it in realize()?
> 

This is for the vCPU threads of the sPAPR CPU core. They don't have the
link to the machine state.

> >      }
> >  }
> >  
> > @@ -314,16 +313,12 @@ err:
> >  
> >  static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> >  {
> > -    /* We don't use SPAPR_MACHINE() in order to exit gracefully if the user
> > -     * tries to add a sPAPR CPU core to a non-pseries machine.
> > -     */
> > -    SpaprMachineState *spapr =
> > -        (SpaprMachineState *) object_dynamic_cast(qdev_get_machine(),
> > -                                                  TYPE_SPAPR_MACHINE);
> >      SpaprCpuCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> > +    SpaprMachineState *spapr = sc->spapr;
> >      CPUCore *cc = CPU_CORE(OBJECT(dev));
> >      int i;
> >  
> > +    /* Set in spapr_core_pre_plug() */
> >      if (!spapr) {
> >          error_setg(errp, TYPE_SPAPR_CPU_CORE " needs a pseries machine");
> >          return;
> > @@ -345,6 +340,8 @@ static Property spapr_cpu_core_properties[] = {
> >      DEFINE_PROP_INT32("node-id", SpaprCpuCore, node_id, CPU_UNSET_NUMA_NODE_ID),
> >      DEFINE_PROP_BOOL("pre-3.0-migration", SpaprCpuCore, pre_3_0_migration,
> >                       false),
> > +    DEFINE_PROP_LINK("spapr", SpaprCpuCore, spapr, TYPE_SPAPR_MACHINE,
> > +                     SpaprMachineState *),
> >      DEFINE_PROP_END_OF_LIST()
> >  };
> >  
> > 
> 

Re: [PATCH 1/6] spapr: Add an "spapr" property to sPAPR CPU core

[Philippe Mathieu-Daudé]

On 12/9/20 6:42 PM, Greg Kurz wrote:
> On Wed, 9 Dec 2020 18:34:31 +0100
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> 
>> On 12/9/20 6:00 PM, Greg Kurz wrote:
>>> The sPAPR CPU core device can only work with pseries machine types.
>>> This is currently checked in the realize function with a dynamic
>>> cast of qdev_get_machine(). Some other places also need to reach
>>> out to the machine using qdev_get_machine().
>>>
>>> Make this dependency explicit by introducing an "spapr" link
>>> property which officialy points to the machine. This link is
>>> set by pseries machine types only in the pre-plug handler. This
>>> allows to drop some users of qdev_get_machine().
>>>
>>> Signed-off-by: Greg Kurz <groug@kaod.org>
>>> ---
>>>  include/hw/ppc/spapr_cpu_core.h |  2 ++
>>>  hw/ppc/spapr.c                  |  4 ++++
>>>  hw/ppc/spapr_cpu_core.c         | 17 +++++++----------
>>>  3 files changed, 13 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
>>> index dab3dfc76c0a..0969b29fd96c 100644
>>> --- a/include/hw/ppc/spapr_cpu_core.h
>>> +++ b/include/hw/ppc/spapr_cpu_core.h
>>> @@ -10,6 +10,7 @@
>>>  #define HW_SPAPR_CPU_CORE_H
>>>  
>>>  #include "hw/cpu/core.h"
>>> +#include "hw/ppc/spapr.h"
>>>  #include "hw/qdev-core.h"
>>>  #include "target/ppc/cpu-qom.h"
>>>  #include "target/ppc/cpu.h"
>>> @@ -24,6 +25,7 @@ OBJECT_DECLARE_TYPE(SpaprCpuCore, SpaprCpuCoreClass,
>>>  struct SpaprCpuCore {
>>>      /*< private >*/
>>>      CPUCore parent_obj;
>>> +    SpaprMachineState *spapr;
>>>  
>>>      /*< public >*/
>>>      PowerPCCPU **threads;
>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>> index d1dcf3ab2c94..4cc51723c62e 100644
>>> --- a/hw/ppc/spapr.c
>>> +++ b/hw/ppc/spapr.c
>>> @@ -3816,6 +3816,10 @@ static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>>>      int index;
>>>      unsigned int smp_threads = machine->smp.threads;
>>>  
>>> +    /* Required by spapr_cpu_core_realize() */
>>> +    object_property_set_link(OBJECT(dev), "spapr", OBJECT(hotplug_dev),
>>> +                             &error_abort);
>>> +
>>>      if (dev->hotplugged && !mc->has_hotpluggable_cpus) {
>>>          error_setg(errp, "CPU hotplug not supported for this machine");
>>>          return;
>>> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
>>> index 2f7dc3c23ded..dec09367e4a0 100644
>>> --- a/hw/ppc/spapr_cpu_core.c
>>> +++ b/hw/ppc/spapr_cpu_core.c
>>> @@ -25,14 +25,13 @@
>>>  #include "sysemu/hw_accel.h"
>>>  #include "qemu/error-report.h"
>>>  
>>> -static void spapr_reset_vcpu(PowerPCCPU *cpu)
>>> +static void spapr_reset_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr)
>>>  {
>>>      CPUState *cs = CPU(cpu);
>>>      CPUPPCState *env = &cpu->env;
>>>      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>>>      SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
>>>      target_ulong lpcr;
>>> -    SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>>>  
>>>      cpu_reset(cs);
>>>  
>>> @@ -186,7 +185,7 @@ static void spapr_unrealize_vcpu(PowerPCCPU *cpu, SpaprCpuCore *sc)
>>>      if (!sc->pre_3_0_migration) {
>>>          vmstate_unregister(NULL, &vmstate_spapr_cpu_state, cpu->machine_data);
>>>      }
>>> -    spapr_irq_cpu_intc_destroy(SPAPR_MACHINE(qdev_get_machine()), cpu);
>>> +    spapr_irq_cpu_intc_destroy(sc->spapr, cpu);
>>>      qdev_unrealize(DEVICE(cpu));
>>>  }
>>>  
>>> @@ -200,7 +199,7 @@ static void spapr_cpu_core_reset(DeviceState *dev)
>>>      int i;
>>>  
>>>      for (i = 0; i < cc->nr_threads; i++) {
>>> -        spapr_reset_vcpu(sc->threads[i]);
>>> +        spapr_reset_vcpu(sc->threads[i], sc->spapr);
>>
>> Why reset() needs access to the machine state, don't
>> you have it in realize()?
>>
> 
> This is for the vCPU threads of the sPAPR CPU core. They don't have the
> link to the machine state.

They are created by spapr_create_vcpu() + spapr_realize_vcpu() in
spapr_cpu_core_realize(), which has sc->spapr set... Am I missing
something?

> 
>>>      }
>>>  }
>>>  
>>> @@ -314,16 +313,12 @@ err:
>>>  
>>>  static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>>>  {
>>> -    /* We don't use SPAPR_MACHINE() in order to exit gracefully if the user
>>> -     * tries to add a sPAPR CPU core to a non-pseries machine.
>>> -     */
>>> -    SpaprMachineState *spapr =
>>> -        (SpaprMachineState *) object_dynamic_cast(qdev_get_machine(),
>>> -                                                  TYPE_SPAPR_MACHINE);
>>>      SpaprCpuCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
>>> +    SpaprMachineState *spapr = sc->spapr;
>>>      CPUCore *cc = CPU_CORE(OBJECT(dev));
>>>      int i;
>>>  
>>> +    /* Set in spapr_core_pre_plug() */
>>>      if (!spapr) {
>>>          error_setg(errp, TYPE_SPAPR_CPU_CORE " needs a pseries machine");
>>>          return;
>>> @@ -345,6 +340,8 @@ static Property spapr_cpu_core_properties[] = {
>>>      DEFINE_PROP_INT32("node-id", SpaprCpuCore, node_id, CPU_UNSET_NUMA_NODE_ID),
>>>      DEFINE_PROP_BOOL("pre-3.0-migration", SpaprCpuCore, pre_3_0_migration,
>>>                       false),
>>> +    DEFINE_PROP_LINK("spapr", SpaprCpuCore, spapr, TYPE_SPAPR_MACHINE,
>>> +                     SpaprMachineState *),
>>>      DEFINE_PROP_END_OF_LIST()
>>>  };
>>>  
>>>
>>
> 

Re: [PATCH 1/6] spapr: Add an "spapr" property to sPAPR CPU core

[Philippe Mathieu-Daudé]

On 12/9/20 6:53 PM, Philippe Mathieu-Daudé wrote:
> On 12/9/20 6:42 PM, Greg Kurz wrote:
>> On Wed, 9 Dec 2020 18:34:31 +0100
>> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>>> On 12/9/20 6:00 PM, Greg Kurz wrote:
>>>> The sPAPR CPU core device can only work with pseries machine types.
>>>> This is currently checked in the realize function with a dynamic
>>>> cast of qdev_get_machine(). Some other places also need to reach
>>>> out to the machine using qdev_get_machine().
>>>>
>>>> Make this dependency explicit by introducing an "spapr" link
>>>> property which officialy points to the machine. This link is
>>>> set by pseries machine types only in the pre-plug handler. This
>>>> allows to drop some users of qdev_get_machine().
>>>>
>>>> Signed-off-by: Greg Kurz <groug@kaod.org>
>>>> ---
>>>>  include/hw/ppc/spapr_cpu_core.h |  2 ++
>>>>  hw/ppc/spapr.c                  |  4 ++++
>>>>  hw/ppc/spapr_cpu_core.c         | 17 +++++++----------
>>>>  3 files changed, 13 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
>>>> index dab3dfc76c0a..0969b29fd96c 100644
>>>> --- a/include/hw/ppc/spapr_cpu_core.h
>>>> +++ b/include/hw/ppc/spapr_cpu_core.h
>>>> @@ -10,6 +10,7 @@
>>>>  #define HW_SPAPR_CPU_CORE_H
>>>>  
>>>>  #include "hw/cpu/core.h"
>>>> +#include "hw/ppc/spapr.h"
>>>>  #include "hw/qdev-core.h"
>>>>  #include "target/ppc/cpu-qom.h"
>>>>  #include "target/ppc/cpu.h"
>>>> @@ -24,6 +25,7 @@ OBJECT_DECLARE_TYPE(SpaprCpuCore, SpaprCpuCoreClass,
>>>>  struct SpaprCpuCore {
>>>>      /*< private >*/
>>>>      CPUCore parent_obj;
>>>> +    SpaprMachineState *spapr;
>>>>  
>>>>      /*< public >*/
>>>>      PowerPCCPU **threads;
>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>> index d1dcf3ab2c94..4cc51723c62e 100644
>>>> --- a/hw/ppc/spapr.c
>>>> +++ b/hw/ppc/spapr.c
>>>> @@ -3816,6 +3816,10 @@ static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>>>>      int index;
>>>>      unsigned int smp_threads = machine->smp.threads;
>>>>  
>>>> +    /* Required by spapr_cpu_core_realize() */
>>>> +    object_property_set_link(OBJECT(dev), "spapr", OBJECT(hotplug_dev),
>>>> +                             &error_abort);
>>>> +
>>>>      if (dev->hotplugged && !mc->has_hotpluggable_cpus) {
>>>>          error_setg(errp, "CPU hotplug not supported for this machine");
>>>>          return;
>>>> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
>>>> index 2f7dc3c23ded..dec09367e4a0 100644
>>>> --- a/hw/ppc/spapr_cpu_core.c
>>>> +++ b/hw/ppc/spapr_cpu_core.c
>>>> @@ -25,14 +25,13 @@
>>>>  #include "sysemu/hw_accel.h"
>>>>  #include "qemu/error-report.h"
>>>>  
>>>> -static void spapr_reset_vcpu(PowerPCCPU *cpu)
>>>> +static void spapr_reset_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr)
>>>>  {
>>>>      CPUState *cs = CPU(cpu);
>>>>      CPUPPCState *env = &cpu->env;
>>>>      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>>>>      SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
>>>>      target_ulong lpcr;
>>>> -    SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>>>>  
>>>>      cpu_reset(cs);
>>>>  
>>>> @@ -186,7 +185,7 @@ static void spapr_unrealize_vcpu(PowerPCCPU *cpu, SpaprCpuCore *sc)
>>>>      if (!sc->pre_3_0_migration) {
>>>>          vmstate_unregister(NULL, &vmstate_spapr_cpu_state, cpu->machine_data);
>>>>      }
>>>> -    spapr_irq_cpu_intc_destroy(SPAPR_MACHINE(qdev_get_machine()), cpu);
>>>> +    spapr_irq_cpu_intc_destroy(sc->spapr, cpu);
>>>>      qdev_unrealize(DEVICE(cpu));
>>>>  }
>>>>  
>>>> @@ -200,7 +199,7 @@ static void spapr_cpu_core_reset(DeviceState *dev)
>>>>      int i;
>>>>  
>>>>      for (i = 0; i < cc->nr_threads; i++) {
>>>> -        spapr_reset_vcpu(sc->threads[i]);
>>>> +        spapr_reset_vcpu(sc->threads[i], sc->spapr);
>>>
>>> Why reset() needs access to the machine state, don't
>>> you have it in realize()?
>>>
>>
>> This is for the vCPU threads of the sPAPR CPU core. They don't have the
>> link to the machine state.
> 
> They are created by spapr_create_vcpu() + spapr_realize_vcpu() in
> spapr_cpu_core_realize(), which has sc->spapr set... Am I missing
> something?

Anyhow, from a QOM design point of view, resetfn() is not the correct
place to set a property IMHO, so Cc'ing Eduardo.

Re: [PATCH 1/6] spapr: Add an "spapr" property to sPAPR CPU core

[Eduardo Habkost]

On Wed, Dec 09, 2020 at 07:11:40PM +0100, Philippe Mathieu-Daudé wrote:
[...]
> >>>> @@ -200,7 +199,7 @@ static void spapr_cpu_core_reset(DeviceState *dev)
> >>>>      int i;
> >>>>  
> >>>>      for (i = 0; i < cc->nr_threads; i++) {
> >>>> -        spapr_reset_vcpu(sc->threads[i]);
> >>>> +        spapr_reset_vcpu(sc->threads[i], sc->spapr);
> >>>
> >>> Why reset() needs access to the machine state, don't
> >>> you have it in realize()?
> >>>
> >>
> >> This is for the vCPU threads of the sPAPR CPU core. They don't have the
> >> link to the machine state.
> > 
> > They are created by spapr_create_vcpu() + spapr_realize_vcpu() in
> > spapr_cpu_core_realize(), which has sc->spapr set... Am I missing
> > something?
> 
> Anyhow, from a QOM design point of view, resetfn() is not the correct
> place to set a property IMHO, so Cc'ing Eduardo.

This patch is not setting the property on resetfn(), it is
setting it on CPU core pre_plug().

This is more complex than simply using qdev_get_machine() and I
don't see why it would be better, but I don't think it's wrong.

-- 
Eduardo

Re: [PATCH 1/6] spapr: Add an "spapr" property to sPAPR CPU core

[Greg Kurz]

On Wed, 9 Dec 2020 13:26:17 -0500
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Dec 09, 2020 at 07:11:40PM +0100, Philippe Mathieu-Daudé wrote:
> [...]
> > >>>> @@ -200,7 +199,7 @@ static void spapr_cpu_core_reset(DeviceState *dev)
> > >>>>      int i;
> > >>>>  
> > >>>>      for (i = 0; i < cc->nr_threads; i++) {
> > >>>> -        spapr_reset_vcpu(sc->threads[i]);
> > >>>> +        spapr_reset_vcpu(sc->threads[i], sc->spapr);
> > >>>
> > >>> Why reset() needs access to the machine state, don't
> > >>> you have it in realize()?
> > >>>
> > >>
> > >> This is for the vCPU threads of the sPAPR CPU core. They don't have the
> > >> link to the machine state.
> > > 
> > > They are created by spapr_create_vcpu() + spapr_realize_vcpu() in
> > > spapr_cpu_core_realize(), which has sc->spapr set... Am I missing
> > > something?
> > 
> > Anyhow, from a QOM design point of view, resetfn() is not the correct
> > place to set a property IMHO, so Cc'ing Eduardo.
> 
> This patch is not setting the property on resetfn(), it is
> setting it on CPU core pre_plug().
> 
> This is more complex than simply using qdev_get_machine() and I
> don't see why it would be better, but I don't think it's wrong.
> 

The reference to the machine state is basically needed to
setup/reset/teardown interrupt presenters in the IRQ chip
backend. It is a bit unfortunate to express this dependency
at realize(), reset() and unrealize(). Maybe having an
"irq_chip" property linked to the IRQ chip backend would
make more sense ?

Re: [PATCH 1/6] spapr: Add an "spapr" property to sPAPR CPU core

[Eduardo Habkost]

On Wed, Dec 09, 2020 at 09:24:36PM +0100, Greg Kurz wrote:
> On Wed, 9 Dec 2020 13:26:17 -0500
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Wed, Dec 09, 2020 at 07:11:40PM +0100, Philippe Mathieu-Daudé wrote:
> > [...]
> > > >>>> @@ -200,7 +199,7 @@ static void spapr_cpu_core_reset(DeviceState *dev)
> > > >>>>      int i;
> > > >>>>  
> > > >>>>      for (i = 0; i < cc->nr_threads; i++) {
> > > >>>> -        spapr_reset_vcpu(sc->threads[i]);
> > > >>>> +        spapr_reset_vcpu(sc->threads[i], sc->spapr);
> > > >>>
> > > >>> Why reset() needs access to the machine state, don't
> > > >>> you have it in realize()?
> > > >>>
> > > >>
> > > >> This is for the vCPU threads of the sPAPR CPU core. They don't have the
> > > >> link to the machine state.
> > > > 
> > > > They are created by spapr_create_vcpu() + spapr_realize_vcpu() in
> > > > spapr_cpu_core_realize(), which has sc->spapr set... Am I missing
> > > > something?
> > > 
> > > Anyhow, from a QOM design point of view, resetfn() is not the correct
> > > place to set a property IMHO, so Cc'ing Eduardo.
> > 
> > This patch is not setting the property on resetfn(), it is
> > setting it on CPU core pre_plug().
> > 
> > This is more complex than simply using qdev_get_machine() and I
> > don't see why it would be better, but I don't think it's wrong.
> > 
> 
> The reference to the machine state is basically needed to
> setup/reset/teardown interrupt presenters in the IRQ chip
> backend. It is a bit unfortunate to express this dependency
> at realize(), reset() and unrealize(). Maybe having an
> "irq_chip" property linked to the IRQ chip backend would
> make more sense ?
> 

Considering that the spapr_irq_*() functions get a
SpaprMachineState argument and deal with two interrupt
controllers, maybe you won't be able to save what you need in a
single irq_chip field?

I don't have a strong opinion here.  It feels weird to me to save
a reference to the global machine object that is always
available, but I don't think that's a problem if you believe the
resulting code looks better.

-- 
Eduardo

Re: [PATCH 3/6] spapr: Pass sPAPR machine state down to spapr_pci_switch_vga()

[David Gibson]

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

On Wed, Dec 09, 2020 at 06:00:49PM +0100, Greg Kurz wrote:
> This allows to drop a user of qdev_get_machine().
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-6.0.

> ---
>  include/hw/ppc/spapr.h | 2 +-
>  hw/ppc/spapr_hcall.c   | 7 ++++---
>  hw/ppc/spapr_pci.c     | 3 +--
>  3 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index b7ced9faebf5..e0f10f252c08 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -834,7 +834,7 @@ int spapr_dma_dt(void *fdt, int node_off, const char *propname,
>                   uint32_t liobn, uint64_t window, uint32_t size);
>  int spapr_tcet_dma_dt(void *fdt, int node_off, const char *propname,
>                        SpaprTceTable *tcet);
> -void spapr_pci_switch_vga(bool big_endian);
> +void spapr_pci_switch_vga(SpaprMachineState *spapr, bool big_endian);
>  void spapr_hotplug_req_add_by_index(SpaprDrc *drc);
>  void spapr_hotplug_req_remove_by_index(SpaprDrc *drc);
>  void spapr_hotplug_req_add_by_count(SpaprDrcType drc_type,
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 1d8e8e6a888f..c0ea0bd5794b 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1351,6 +1351,7 @@ static target_ulong h_logical_dcbf(PowerPCCPU *cpu, SpaprMachineState *spapr,
>  }
>  
>  static target_ulong h_set_mode_resource_le(PowerPCCPU *cpu,
> +                                           SpaprMachineState *spapr,
>                                             target_ulong mflags,
>                                             target_ulong value1,
>                                             target_ulong value2)
> @@ -1365,12 +1366,12 @@ static target_ulong h_set_mode_resource_le(PowerPCCPU *cpu,
>      switch (mflags) {
>      case H_SET_MODE_ENDIAN_BIG:
>          spapr_set_all_lpcrs(0, LPCR_ILE);
> -        spapr_pci_switch_vga(true);
> +        spapr_pci_switch_vga(spapr, true);
>          return H_SUCCESS;
>  
>      case H_SET_MODE_ENDIAN_LITTLE:
>          spapr_set_all_lpcrs(LPCR_ILE, LPCR_ILE);
> -        spapr_pci_switch_vga(false);
> +        spapr_pci_switch_vga(spapr, false);
>          return H_SUCCESS;
>      }
>  
> @@ -1411,7 +1412,7 @@ static target_ulong h_set_mode(PowerPCCPU *cpu, SpaprMachineState *spapr,
>  
>      switch (resource) {
>      case H_SET_MODE_RESOURCE_LE:
> -        ret = h_set_mode_resource_le(cpu, args[0], args[2], args[3]);
> +        ret = h_set_mode_resource_le(cpu, spapr, args[0], args[2], args[3]);
>          break;
>      case H_SET_MODE_RESOURCE_ADDR_TRANS_MODE:
>          ret = h_set_mode_resource_addr_trans_mode(cpu, args[0],
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 2d9b88b93122..149bf4c21d22 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -2490,9 +2490,8 @@ static int spapr_switch_one_vga(DeviceState *dev, void *opaque)
>      return 0;
>  }
>  
> -void spapr_pci_switch_vga(bool big_endian)
> +void spapr_pci_switch_vga(SpaprMachineState *spapr, bool big_endian)
>  {
> -    SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>      SpaprPhbState *sphb;
>  
>      /*

-- 
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 4/6] spapr: Don't use qdev_get_machine() in spapr_msi_write()

[David Gibson]

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

On Wed, Dec 09, 2020 at 06:00:50PM +0100, Greg Kurz wrote:
> spapr_phb_realize() passes the sPAPR machine state as opaque data
> for the I/O callbacks:
> 
> memory_region_init_io(&sphb->msiwindow, OBJECT(sphb), &spapr_msi_ops, spapr,
>                                                                       ^^^^^
>                       "msi", msi_window_size);
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-6.0.

> ---
>  hw/ppc/spapr_pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 149bf4c21d22..890a0cc1d6af 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -747,7 +747,7 @@ static PCIINTxRoute spapr_route_intx_pin_to_irq(void *opaque, int pin)
>  static void spapr_msi_write(void *opaque, hwaddr addr,
>                              uint64_t data, unsigned size)
>  {
> -    SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> +    SpaprMachineState *spapr = opaque;
>      uint32_t irq = data;
>  
>      trace_spapr_pci_msi_write(addr, data, irq);

-- 
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 5/6] spapr: Pass sPAPR machine state to some RTAS events handling functions

[David Gibson]

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

On Wed, Dec 09, 2020 at 06:00:51PM +0100, Greg Kurz wrote:
> Some functions in hw/ppc/spapr_events.c get a pointer to the machine
> state using qdev_get_machine(). Convert them to get it from their
> caller when possible.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-6.0.

> ---
>  hw/ppc/spapr_events.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index 1add53547ec3..3f37b49fd8ad 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -480,9 +480,8 @@ static SpaprEventLogEntry *rtas_event_log_dequeue(SpaprMachineState *spapr,
>      return entry;
>  }
>  
> -static bool rtas_event_log_contains(uint32_t event_mask)
> +static bool rtas_event_log_contains(SpaprMachineState *spapr, uint32_t event_mask)
>  {
> -    SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>      SpaprEventLogEntry *entry = NULL;
>  
>      QTAILQ_FOREACH(entry, &spapr->pending_events, next) {
> @@ -509,10 +508,10 @@ static void spapr_init_v6hdr(struct rtas_event_log_v6 *v6hdr)
>      v6hdr->company = cpu_to_be32(RTAS_LOG_V6_COMPANY_IBM);
>  }
>  
> -static void spapr_init_maina(struct rtas_event_log_v6_maina *maina,
> +static void spapr_init_maina(SpaprMachineState *spapr,
> +                             struct rtas_event_log_v6_maina *maina,
>                               int section_count)
>  {
> -    SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>      struct tm tm;
>      int year;
>  
> @@ -560,7 +559,7 @@ static void spapr_powerdown_req(Notifier *n, void *opaque)
>      entry->extended_length = sizeof(*new_epow);
>  
>      spapr_init_v6hdr(v6hdr);
> -    spapr_init_maina(maina, 3 /* Main-A, Main-B and EPOW */);
> +    spapr_init_maina(spapr, maina, 3 /* Main-A, Main-B and EPOW */);
>  
>      mainb->hdr.section_id = cpu_to_be16(RTAS_LOG_V6_SECTION_ID_MAINB);
>      mainb->hdr.section_length = cpu_to_be16(sizeof(*mainb));
> @@ -613,7 +612,7 @@ static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
>      entry->extended_length = sizeof(*new_hp);
>  
>      spapr_init_v6hdr(v6hdr);
> -    spapr_init_maina(maina, 3 /* Main-A, Main-B, HP */);
> +    spapr_init_maina(spapr, maina, 3 /* Main-A, Main-B, HP */);
>  
>      mainb->hdr.section_id = cpu_to_be16(RTAS_LOG_V6_SECTION_ID_MAINB);
>      mainb->hdr.section_length = cpu_to_be16(sizeof(*mainb));
> @@ -808,9 +807,9 @@ static uint32_t spapr_mce_get_elog_type(PowerPCCPU *cpu, bool recovered,
>      return summary;
>  }
>  
> -static void spapr_mce_dispatch_elog(PowerPCCPU *cpu, bool recovered)
> +static void spapr_mce_dispatch_elog(SpaprMachineState *spapr, PowerPCCPU *cpu,
> +                                    bool recovered)
>  {
> -    SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>      CPUState *cs = CPU(cpu);
>      CPUPPCState *env = &cpu->env;
>      uint64_t rtas_addr;
> @@ -927,7 +926,7 @@ void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered)
>          warn_report("Received a fwnmi while migration was in progress");
>      }
>  
> -    spapr_mce_dispatch_elog(cpu, recovered);
> +    spapr_mce_dispatch_elog(spapr, cpu, recovered);
>  }
>  
>  static void check_exception(PowerPCCPU *cpu, SpaprMachineState *spapr,
> @@ -980,7 +979,7 @@ static void check_exception(PowerPCCPU *cpu, SpaprMachineState *spapr,
>       * interrupts.
>       */
>      for (i = 0; i < EVENT_CLASS_MAX; i++) {
> -        if (rtas_event_log_contains(EVENT_CLASS_MASK(i))) {
> +        if (rtas_event_log_contains(spapr, EVENT_CLASS_MASK(i))) {
>              const SpaprEventSource *source =
>                  spapr_event_sources_get_source(spapr->event_sources, i);
>  
> @@ -1007,7 +1006,7 @@ static void event_scan(PowerPCCPU *cpu, SpaprMachineState *spapr,
>      }
>  
>      for (i = 0; i < EVENT_CLASS_MAX; i++) {
> -        if (rtas_event_log_contains(EVENT_CLASS_MASK(i))) {
> +        if (rtas_event_log_contains(spapr, EVENT_CLASS_MASK(i))) {
>              const SpaprEventSource *source =
>                  spapr_event_sources_get_source(spapr->event_sources, i);
>  

-- 
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 1/6] spapr: Add an "spapr" property to sPAPR CPU core

[David Gibson]

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

On Wed, Dec 09, 2020 at 01:26:17PM -0500, Eduardo Habkost wrote:
> On Wed, Dec 09, 2020 at 07:11:40PM +0100, Philippe Mathieu-Daudé wrote:
> [...]
> > >>>> @@ -200,7 +199,7 @@ static void spapr_cpu_core_reset(DeviceState *dev)
> > >>>>      int i;
> > >>>>  
> > >>>>      for (i = 0; i < cc->nr_threads; i++) {
> > >>>> -        spapr_reset_vcpu(sc->threads[i]);
> > >>>> +        spapr_reset_vcpu(sc->threads[i], sc->spapr);
> > >>>
> > >>> Why reset() needs access to the machine state, don't
> > >>> you have it in realize()?
> > >>>
> > >>
> > >> This is for the vCPU threads of the sPAPR CPU core. They don't have the
> > >> link to the machine state.
> > > 
> > > They are created by spapr_create_vcpu() + spapr_realize_vcpu() in
> > > spapr_cpu_core_realize(), which has sc->spapr set... Am I missing
> > > something?
> > 
> > Anyhow, from a QOM design point of view, resetfn() is not the correct
> > place to set a property IMHO, so Cc'ing Eduardo.
> 
> This patch is not setting the property on resetfn(), it is
> setting it on CPU core pre_plug().

Well, also machine reset, but the point is it's not the resetfn() of
the cpu.

Basically what this is doing is machine specific (rather than just cpu
specific) initialization of the cpu state - we need that because the
pseries machine is implementing an explicitly paravirtualized platform
which starts things off in a state a bit different from the "raw" cpu
behaviour.

So, although it's working on a CPU's state, this function actually
belongs to the machine, rather than the cpu.

> This is more complex than simply using qdev_get_machine() and I
> don't see why it would be better, but I don't think it's wrong.

But, yeah, this...

I've applied some of the later patches in this series, but I'm not
convinced on this one or 2/6.  It seems like they're just replacing
one ugly (access to qdev_machine_state() as a global) with a different
ugly (duplicating something which has to equal the global machine
pointer as properties in a bunch of other objects).

Both 1/6 and 2/6 are altering explicitly spapr specific devices, they
have interactions with the overall platform model which mean they have
to sit in that environment, so I think trying to add a property here
implies an abstraction that can't actually be used in practice.

-- 
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 6/6] target/ppc: Add mce_req_event() handler to PPCVirtualHypervisorClass

[David Gibson]

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

On Wed, Dec 09, 2020 at 06:00:52PM +0100, Greg Kurz wrote:
> kvm_handle_nmi() directly calls spapr_mce_req_event() which is machine
> level code. Apart from being ugly, this forces spapr_mce_req_event()
> to rely on qdev_get_machine() to get a pointer to the machine state.
> This is a bit unfortunate since POWER CPUs have a backlink to the
> virtual hypervisor, which happens to be the machine itself with
> sPAPR.
> 
> Turn spapr_mce_req_event() into a PPC virtual hypervisor operation,
> and adapt kvm_handle_nmi() to call it as such.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

I have somewhat mixed thoughts on this.  Putting it in vhyp makes a
certain sense.  But on the other hand, the MCE event from KVM is an
explicitly PAPR specific interface, so it can't really go to any other
implementation.

-- 
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 1/6] spapr: Add an "spapr" property to sPAPR CPU core

[Cédric Le Goater]

On 12/9/20 9:54 PM, Eduardo Habkost wrote:
> On Wed, Dec 09, 2020 at 09:24:36PM +0100, Greg Kurz wrote:
>> On Wed, 9 Dec 2020 13:26:17 -0500
>> Eduardo Habkost <ehabkost@redhat.com> wrote:
>>
>>> On Wed, Dec 09, 2020 at 07:11:40PM +0100, Philippe Mathieu-Daudé wrote:
>>> [...]
>>>>>>>> @@ -200,7 +199,7 @@ static void spapr_cpu_core_reset(DeviceState *dev)
>>>>>>>>      int i;
>>>>>>>>  
>>>>>>>>      for (i = 0; i < cc->nr_threads; i++) {
>>>>>>>> -        spapr_reset_vcpu(sc->threads[i]);
>>>>>>>> +        spapr_reset_vcpu(sc->threads[i], sc->spapr);
>>>>>>>
>>>>>>> Why reset() needs access to the machine state, don't
>>>>>>> you have it in realize()?
>>>>>>>
>>>>>>
>>>>>> This is for the vCPU threads of the sPAPR CPU core. They don't have the
>>>>>> link to the machine state.
>>>>>
>>>>> They are created by spapr_create_vcpu() + spapr_realize_vcpu() in
>>>>> spapr_cpu_core_realize(), which has sc->spapr set... Am I missing
>>>>> something?
>>>>
>>>> Anyhow, from a QOM design point of view, resetfn() is not the correct
>>>> place to set a property IMHO, so Cc'ing Eduardo.
>>>
>>> This patch is not setting the property on resetfn(), it is
>>> setting it on CPU core pre_plug().
>>>
>>> This is more complex than simply using qdev_get_machine() and I
>>> don't see why it would be better, but I don't think it's wrong.
>>>
>>
>> The reference to the machine state is basically needed to
>> setup/reset/teardown interrupt presenters in the IRQ chip
>> backend. It is a bit unfortunate to express this dependency
>> at realize(), reset() and unrealize(). Maybe having an
>> "irq_chip" property linked to the IRQ chip backend would
>> make more sense ?
>>
> 
> Considering that the spapr_irq_*() functions get a
> SpaprMachineState argument and deal with two interrupt
> controllers, maybe you won't be able to save what you need in a
> single irq_chip field?

The sPAPR machine needs to operate on all available interrupt 
controllers and the array is built under the spapr_irq* routines 
with : 

    SpaprInterruptController *intcs[] = ALL_INTCS(spapr);

We could add the array under the machine and use a link to that
instead but we need to keep the existing interrupt controllers 
anyway because of migration compatibility.
 
> I don't have a strong opinion here.  It feels weird to me to save
> a reference to the global machine object that is always
> available, but I don't think that's a problem if you believe the
> resulting code looks better.

I think it's a good cleanup to remove globals. QEMU might emulate 
multiple "machines" in a single binary one day.

C.

Re: [PATCH 1/6] spapr: Add an "spapr" property to sPAPR CPU core

[Greg Kurz]

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

On Thu, 10 Dec 2020 14:53:02 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Dec 09, 2020 at 01:26:17PM -0500, Eduardo Habkost wrote:
> > On Wed, Dec 09, 2020 at 07:11:40PM +0100, Philippe Mathieu-Daudé wrote:
> > [...]
> > > >>>> @@ -200,7 +199,7 @@ static void spapr_cpu_core_reset(DeviceState *dev)
> > > >>>>      int i;
> > > >>>>  
> > > >>>>      for (i = 0; i < cc->nr_threads; i++) {
> > > >>>> -        spapr_reset_vcpu(sc->threads[i]);
> > > >>>> +        spapr_reset_vcpu(sc->threads[i], sc->spapr);
> > > >>>
> > > >>> Why reset() needs access to the machine state, don't
> > > >>> you have it in realize()?
> > > >>>
> > > >>
> > > >> This is for the vCPU threads of the sPAPR CPU core. They don't have the
> > > >> link to the machine state.
> > > > 
> > > > They are created by spapr_create_vcpu() + spapr_realize_vcpu() in
> > > > spapr_cpu_core_realize(), which has sc->spapr set... Am I missing
> > > > something?
> > > 
> > > Anyhow, from a QOM design point of view, resetfn() is not the correct
> > > place to set a property IMHO, so Cc'ing Eduardo.
> > 
> > This patch is not setting the property on resetfn(), it is
> > setting it on CPU core pre_plug().
> 
> Well, also machine reset, but the point is it's not the resetfn() of
> the cpu.
> 
> Basically what this is doing is machine specific (rather than just cpu
> specific) initialization of the cpu state - we need that because the
> pseries machine is implementing an explicitly paravirtualized platform
> which starts things off in a state a bit different from the "raw" cpu
> behaviour.
> 
> So, although it's working on a CPU's state, this function actually
> belongs to the machine, rather than the cpu.
> 
> > This is more complex than simply using qdev_get_machine() and I
> > don't see why it would be better, but I don't think it's wrong.
> 
> But, yeah, this...
> 
> I've applied some of the later patches in this series, but I'm not
> convinced on this one or 2/6.  It seems like they're just replacing
> one ugly (access to qdev_machine_state() as a global) with a different
> ugly (duplicating something which has to equal the global machine
> pointer as properties in a bunch of other objects).
> 
> Both 1/6 and 2/6 are altering explicitly spapr specific devices, they
> have interactions with the overall platform model which mean they have
> to sit in that environment, so I think trying to add a property here
> implies an abstraction that can't actually be used in practice.
> 

Eduardo and you convinced me that 1/6 and 2/6 might not be an
improvement in the end, but rather making things more complex
than simply calling qdev_get_machine() when needed.

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

Re: [PATCH 6/6] target/ppc: Add mce_req_event() handler to PPCVirtualHypervisorClass

[Greg Kurz]

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

On Thu, 10 Dec 2020 14:54:08 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Dec 09, 2020 at 06:00:52PM +0100, Greg Kurz wrote:
> > kvm_handle_nmi() directly calls spapr_mce_req_event() which is machine
> > level code. Apart from being ugly, this forces spapr_mce_req_event()
> > to rely on qdev_get_machine() to get a pointer to the machine state.
> > This is a bit unfortunate since POWER CPUs have a backlink to the
> > virtual hypervisor, which happens to be the machine itself with
> > sPAPR.
> > 
> > Turn spapr_mce_req_event() into a PPC virtual hypervisor operation,
> > and adapt kvm_handle_nmi() to call it as such.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> 
> I have somewhat mixed thoughts on this.  Putting it in vhyp makes a
> certain sense.  But on the other hand, the MCE event from KVM is an
> explicitly PAPR specific interface, so it can't really go to any other
> implementation.
> 

True. Same thing goest for the hypercalls actually. So I guess it's
better to keep this dependency explicit, as long as we don't have
to support non-PAPR KVM guests. Please ignore this patch.

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