* [Qemu-devel] [PATCHv3 0/7] Better handling of machine specific per-cpu information
@ 2018-06-14 4:41 David Gibson
2018-06-14 4:41 ` [Qemu-devel] [PATCHv3 1/7] spapr: Clean up cpu realize/unrealize paths David Gibson
` (6 more replies)
0 siblings, 7 replies; 15+ messages in thread
From: David Gibson @ 2018-06-14 4:41 UTC (permalink / raw)
To: groug; +Cc: clg, qemu-devel, qemu-ppc, David Gibson
It's moderately common for a machine type to need to keep track of
information that is specific to the platform it implements, but
per-cpu.
While it could keep such information inside the MachineState, this
makes lookup from the CPUState awkward. So, this series adds a
standard way to stash machine-specific per-cpu information using a
void pointer in the PowerPCCPU object. The machine is responsible for
alloc()ing, free()ing and (if applicable) migrating this state.
The meat of the series is the last two patches. The first 5 clean up
a number of minor uglies I encountered while implementing.
Chganges since v2:
* Fix compile (and other) errors in KVM code.
Changes since v1:
* Simplify the error handling cleanup in 2/7 to use &error_abort
instead of more propagations
David Gibson (7):
spapr: Clean up cpu realize/unrealize paths
pnv: Fix some error handling cpu realize()
pnv_core: Allocate cpu thread objects individually
pnv: Clean up cpu realize path
pnv: Add cpu unrealize path
target/ppc: Replace intc pointer with a general machine_data pointer
target/ppc, spapr: Move VPA information to machine_data
hw/intc/xics.c | 5 +-
hw/intc/xics_spapr.c | 16 +++--
hw/ppc/pnv.c | 8 +--
hw/ppc/pnv_core.c | 100 ++++++++++++++++++--------------
hw/ppc/spapr.c | 8 +--
hw/ppc/spapr_cpu_core.c | 85 +++++++++++++--------------
hw/ppc/spapr_hcall.c | 77 +++++++++++++-----------
include/hw/ppc/pnv_core.h | 11 +++-
include/hw/ppc/spapr_cpu_core.h | 13 +++++
include/hw/ppc/xics.h | 4 +-
target/ppc/cpu.h | 8 +--
target/ppc/kvm.c | 36 ++++++------
target/ppc/translate_init.inc.c | 8 ---
13 files changed, 203 insertions(+), 176 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCHv3 1/7] spapr: Clean up cpu realize/unrealize paths
2018-06-14 4:41 [Qemu-devel] [PATCHv3 0/7] Better handling of machine specific per-cpu information David Gibson
@ 2018-06-14 4:41 ` David Gibson
2018-06-14 4:41 ` [Qemu-devel] [PATCHv3 2/7] pnv: Fix some error handling cpu realize() David Gibson
` (5 subsequent siblings)
6 siblings, 0 replies; 15+ messages in thread
From: David Gibson @ 2018-06-14 4:41 UTC (permalink / raw)
To: groug; +Cc: clg, qemu-devel, qemu-ppc, David Gibson
spapr_cpu_init() and spapr_cpu_destroy() are only called from the spapr
cpu core realize/unrealize paths, and really can only be called from there.
Those are all short functions, so fold the pairs together for simplicity.
While we're there rename some functions and change some parameter types
for brevity and clarity.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Greg Kurz <groug@kaod.org>
---
hw/ppc/spapr_cpu_core.c | 69 +++++++++++++++--------------------------
1 file changed, 25 insertions(+), 44 deletions(-)
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index f3e9b879b2..7fdb3b6667 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -83,26 +83,6 @@ void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r
ppc_store_lpcr(cpu, env->spr[SPR_LPCR] | pcc->lpcr_pm);
}
-static void spapr_cpu_destroy(PowerPCCPU *cpu)
-{
- qemu_unregister_reset(spapr_cpu_reset, cpu);
-}
-
-static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
- Error **errp)
-{
- CPUPPCState *env = &cpu->env;
-
- /* Set time-base frequency to 512 MHz */
- cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
-
- cpu_ppc_set_vhyp(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
- kvmppc_set_papr(cpu);
-
- qemu_register_reset(spapr_cpu_reset, cpu);
- spapr_cpu_reset(cpu);
-}
-
/*
* Return the sPAPR CPU core type for @model which essentially is the CPU
* model specified with -cpu cmdline option.
@@ -122,44 +102,47 @@ const char *spapr_get_cpu_core_type(const char *cpu_type)
return object_class_get_name(oc);
}
-static void spapr_cpu_core_unrealizefn(DeviceState *dev, Error **errp)
+static void spapr_unrealize_vcpu(PowerPCCPU *cpu)
+{
+ qemu_unregister_reset(spapr_cpu_reset, cpu);
+ object_unparent(cpu->intc);
+ cpu_remove_sync(CPU(cpu));
+ object_unparent(OBJECT(cpu));
+}
+
+static void spapr_cpu_core_unrealize(DeviceState *dev, Error **errp)
{
sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
CPUCore *cc = CPU_CORE(dev);
int i;
for (i = 0; i < cc->nr_threads; i++) {
- Object *obj = OBJECT(sc->threads[i]);
- DeviceState *dev = DEVICE(obj);
- CPUState *cs = CPU(dev);
- PowerPCCPU *cpu = POWERPC_CPU(cs);
-
- spapr_cpu_destroy(cpu);
- object_unparent(cpu->intc);
- cpu_remove_sync(cs);
- object_unparent(obj);
+ spapr_unrealize_vcpu(sc->threads[i]);
}
g_free(sc->threads);
}
-static void spapr_cpu_core_realize_child(Object *child,
- sPAPRMachineState *spapr, Error **errp)
+static void spapr_realize_vcpu(PowerPCCPU *cpu, sPAPRMachineState *spapr,
+ Error **errp)
{
+ CPUPPCState *env = &cpu->env;
Error *local_err = NULL;
- CPUState *cs = CPU(child);
- PowerPCCPU *cpu = POWERPC_CPU(cs);
- object_property_set_bool(child, true, "realized", &local_err);
+ object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
if (local_err) {
goto error;
}
- spapr_cpu_init(spapr, cpu, &local_err);
- if (local_err) {
- goto error;
- }
+ /* Set time-base frequency to 512 MHz */
+ cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
+
+ cpu_ppc_set_vhyp(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
+ kvmppc_set_papr(cpu);
- cpu->intc = icp_create(child, spapr->icp_type, XICS_FABRIC(spapr),
+ qemu_register_reset(spapr_cpu_reset, cpu);
+ spapr_cpu_reset(cpu);
+
+ cpu->intc = icp_create(OBJECT(cpu), spapr->icp_type, XICS_FABRIC(spapr),
&local_err);
if (local_err) {
goto error;
@@ -220,9 +203,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
}
for (j = 0; j < cc->nr_threads; j++) {
- obj = OBJECT(sc->threads[j]);
-
- spapr_cpu_core_realize_child(obj, spapr, &local_err);
+ spapr_realize_vcpu(sc->threads[j], spapr, &local_err);
if (local_err) {
goto err;
}
@@ -249,7 +230,7 @@ static void spapr_cpu_core_class_init(ObjectClass *oc, void *data)
sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_CLASS(oc);
dc->realize = spapr_cpu_core_realize;
- dc->unrealize = spapr_cpu_core_unrealizefn;
+ dc->unrealize = spapr_cpu_core_unrealize;
dc->props = spapr_cpu_core_properties;
scc->cpu_type = data;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCHv3 2/7] pnv: Fix some error handling cpu realize()
2018-06-14 4:41 [Qemu-devel] [PATCHv3 0/7] Better handling of machine specific per-cpu information David Gibson
2018-06-14 4:41 ` [Qemu-devel] [PATCHv3 1/7] spapr: Clean up cpu realize/unrealize paths David Gibson
@ 2018-06-14 4:41 ` David Gibson
2018-06-14 5:26 ` Cédric Le Goater
2018-06-14 6:35 ` Greg Kurz
2018-06-14 4:41 ` [Qemu-devel] [PATCHv3 3/7] pnv_core: Allocate cpu thread objects individually David Gibson
` (4 subsequent siblings)
6 siblings, 2 replies; 15+ messages in thread
From: David Gibson @ 2018-06-14 4:41 UTC (permalink / raw)
To: groug; +Cc: clg, qemu-devel, qemu-ppc, David Gibson
In pnv_core_realize() we call two functions with an Error * parameter in
succession, which will go badly if they both cause errors. In fact, a
failure in either of them indicates a qemu internal error, so we can just
use &error_abort in both cases.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
hw/ppc/pnv_core.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
index 13ad7d9e04..01f47c8037 100644
--- a/hw/ppc/pnv_core.c
+++ b/hw/ppc/pnv_core.c
@@ -172,12 +172,9 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
object_initialize(obj, size, typename);
snprintf(name, sizeof(name), "thread[%d]", i);
- object_property_add_child(OBJECT(pc), name, obj, &local_err);
+ object_property_add_child(OBJECT(pc), name, obj, &error_abort);
object_property_add_alias(obj, "core-pir", OBJECT(pc),
- "pir", &local_err);
- if (local_err) {
- goto err;
- }
+ "pir", &error_abort);
object_unref(obj);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCHv3 3/7] pnv_core: Allocate cpu thread objects individually
2018-06-14 4:41 [Qemu-devel] [PATCHv3 0/7] Better handling of machine specific per-cpu information David Gibson
2018-06-14 4:41 ` [Qemu-devel] [PATCHv3 1/7] spapr: Clean up cpu realize/unrealize paths David Gibson
2018-06-14 4:41 ` [Qemu-devel] [PATCHv3 2/7] pnv: Fix some error handling cpu realize() David Gibson
@ 2018-06-14 4:41 ` David Gibson
2018-06-14 4:41 ` [Qemu-devel] [PATCHv3 4/7] pnv: Clean up cpu realize path David Gibson
` (3 subsequent siblings)
6 siblings, 0 replies; 15+ messages in thread
From: David Gibson @ 2018-06-14 4:41 UTC (permalink / raw)
To: groug; +Cc: clg, qemu-devel, qemu-ppc, David Gibson
Currently, we allocate space for all the cpu objects within a single core
in one big block. This was copied from an older version of the spapr code
and requires some ugly pointer manipulation to extract the individual
objects.
This design was due to a misunderstanding of qemu lifetime conventions and
has already been changed in spapr (in 94ad93bd "spapr_cpu_core: instantiate
CPUs separately".
Make an equivalent change in pnv_core to get rid of the nasty pointer
arithmetic.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Greg Kurz <groug@kaod.org>
---
hw/ppc/pnv.c | 4 ++--
hw/ppc/pnv_core.c | 11 +++++------
include/hw/ppc/pnv_core.h | 2 +-
3 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 0314881316..0b9508d94d 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -121,9 +121,9 @@ static int get_cpus_node(void *fdt)
*/
static void pnv_dt_core(PnvChip *chip, PnvCore *pc, void *fdt)
{
- CPUState *cs = CPU(DEVICE(pc->threads));
+ PowerPCCPU *cpu = pc->threads[0];
+ CPUState *cs = CPU(cpu);
DeviceClass *dc = DEVICE_GET_CLASS(cs);
- PowerPCCPU *cpu = POWERPC_CPU(cs);
int smt_threads = CPU_CORE(pc)->nr_threads;
CPUPPCState *env = &cpu->env;
PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cs);
diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
index 01f47c8037..1e40f01e98 100644
--- a/hw/ppc/pnv_core.c
+++ b/hw/ppc/pnv_core.c
@@ -151,7 +151,6 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
PnvCore *pc = PNV_CORE(OBJECT(dev));
CPUCore *cc = CPU_CORE(OBJECT(dev));
const char *typename = pnv_core_cpu_typename(pc);
- size_t size = object_type_get_instance_size(typename);
Error *local_err = NULL;
void *obj;
int i, j;
@@ -165,11 +164,11 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
return;
}
- pc->threads = g_malloc0(size * cc->nr_threads);
+ pc->threads = g_new(PowerPCCPU *, cc->nr_threads);
for (i = 0; i < cc->nr_threads; i++) {
- obj = pc->threads + i * size;
+ obj = object_new(typename);
- object_initialize(obj, size, typename);
+ pc->threads[i] = POWERPC_CPU(obj);
snprintf(name, sizeof(name), "thread[%d]", i);
object_property_add_child(OBJECT(pc), name, obj, &error_abort);
@@ -179,7 +178,7 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
}
for (j = 0; j < cc->nr_threads; j++) {
- obj = pc->threads + j * size;
+ obj = OBJECT(pc->threads[j]);
pnv_core_realize_child(obj, XICS_FABRIC(xi), &local_err);
if (local_err) {
@@ -194,7 +193,7 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
err:
while (--i >= 0) {
- obj = pc->threads + i * size;
+ obj = OBJECT(pc->threads[i]);
object_unparent(obj);
}
g_free(pc->threads);
diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
index e337af7a3a..447ae761f7 100644
--- a/include/hw/ppc/pnv_core.h
+++ b/include/hw/ppc/pnv_core.h
@@ -34,7 +34,7 @@ typedef struct PnvCore {
CPUCore parent_obj;
/*< public >*/
- void *threads;
+ PowerPCCPU **threads;
uint32_t pir;
MemoryRegion xscom_regs;
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCHv3 4/7] pnv: Clean up cpu realize path
2018-06-14 4:41 [Qemu-devel] [PATCHv3 0/7] Better handling of machine specific per-cpu information David Gibson
` (2 preceding siblings ...)
2018-06-14 4:41 ` [Qemu-devel] [PATCHv3 3/7] pnv_core: Allocate cpu thread objects individually David Gibson
@ 2018-06-14 4:41 ` David Gibson
2018-06-14 4:41 ` [Qemu-devel] [PATCHv3 5/7] pnv: Add cpu unrealize path David Gibson
` (2 subsequent siblings)
6 siblings, 0 replies; 15+ messages in thread
From: David Gibson @ 2018-06-14 4:41 UTC (permalink / raw)
To: groug; +Cc: clg, qemu-devel, qemu-ppc, David Gibson
pnv_cpu_init() is only called from the the pnv cpu core realize path, and
really only can be called from there. So fold it into its caller, which
we also rename for brevity.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Greg Kurz <groug@kaod.org>
---
hw/ppc/pnv_core.c | 56 ++++++++++++++++++-----------------------------
1 file changed, 21 insertions(+), 35 deletions(-)
diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
index 1e40f01e98..f4c41d89d6 100644
--- a/hw/ppc/pnv_core.c
+++ b/hw/ppc/pnv_core.c
@@ -54,28 +54,6 @@ static void pnv_cpu_reset(void *opaque)
env->msr |= MSR_HVB; /* Hypervisor mode */
}
-static void pnv_cpu_init(PowerPCCPU *cpu, Error **errp)
-{
- CPUPPCState *env = &cpu->env;
- int core_pir;
- int thread_index = 0; /* TODO: TCG supports only one thread */
- ppc_spr_t *pir = &env->spr_cb[SPR_PIR];
-
- core_pir = object_property_get_uint(OBJECT(cpu), "core-pir", &error_abort);
-
- /*
- * The PIR of a thread is the core PIR + the thread index. We will
- * need to find a way to get the thread index when TCG supports
- * more than 1. We could use the object name ?
- */
- pir->default_value = core_pir + thread_index;
-
- /* Set time-base frequency to 512 MHz */
- cpu_ppc_tb_init(env, PNV_TIMEBASE_FREQ);
-
- qemu_register_reset(pnv_cpu_reset, cpu);
-}
-
/*
* These values are read by the PowerNV HW monitors under Linux
*/
@@ -121,29 +99,39 @@ static const MemoryRegionOps pnv_core_xscom_ops = {
.endianness = DEVICE_BIG_ENDIAN,
};
-static void pnv_core_realize_child(Object *child, XICSFabric *xi, Error **errp)
+static void pnv_realize_vcpu(PowerPCCPU *cpu, XICSFabric *xi, Error **errp)
{
+ CPUPPCState *env = &cpu->env;
+ int core_pir;
+ int thread_index = 0; /* TODO: TCG supports only one thread */
+ ppc_spr_t *pir = &env->spr_cb[SPR_PIR];
Error *local_err = NULL;
- CPUState *cs = CPU(child);
- PowerPCCPU *cpu = POWERPC_CPU(cs);
- object_property_set_bool(child, true, "realized", &local_err);
+ object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
if (local_err) {
error_propagate(errp, local_err);
return;
}
- cpu->intc = icp_create(child, TYPE_PNV_ICP, xi, &local_err);
+ cpu->intc = icp_create(OBJECT(cpu), TYPE_PNV_ICP, xi, &local_err);
if (local_err) {
error_propagate(errp, local_err);
return;
}
- pnv_cpu_init(cpu, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
- return;
- }
+ core_pir = object_property_get_uint(OBJECT(cpu), "core-pir", &error_abort);
+
+ /*
+ * The PIR of a thread is the core PIR + the thread index. We will
+ * need to find a way to get the thread index when TCG supports
+ * more than 1. We could use the object name ?
+ */
+ pir->default_value = core_pir + thread_index;
+
+ /* Set time-base frequency to 512 MHz */
+ cpu_ppc_tb_init(env, PNV_TIMEBASE_FREQ);
+
+ qemu_register_reset(pnv_cpu_reset, cpu);
}
static void pnv_core_realize(DeviceState *dev, Error **errp)
@@ -178,9 +166,7 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
}
for (j = 0; j < cc->nr_threads; j++) {
- obj = OBJECT(pc->threads[j]);
-
- pnv_core_realize_child(obj, XICS_FABRIC(xi), &local_err);
+ pnv_realize_vcpu(pc->threads[j], XICS_FABRIC(xi), &local_err);
if (local_err) {
goto err;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCHv3 5/7] pnv: Add cpu unrealize path
2018-06-14 4:41 [Qemu-devel] [PATCHv3 0/7] Better handling of machine specific per-cpu information David Gibson
` (3 preceding siblings ...)
2018-06-14 4:41 ` [Qemu-devel] [PATCHv3 4/7] pnv: Clean up cpu realize path David Gibson
@ 2018-06-14 4:41 ` David Gibson
2018-06-14 4:41 ` [Qemu-devel] [PATCHv3 6/7] target/ppc: Replace intc pointer with a general machine_data pointer David Gibson
2018-06-14 4:41 ` [Qemu-devel] [PATCHv3 7/7] target/ppc, spapr: Move VPA information to machine_data David Gibson
6 siblings, 0 replies; 15+ messages in thread
From: David Gibson @ 2018-06-14 4:41 UTC (permalink / raw)
To: groug; +Cc: clg, qemu-devel, qemu-ppc, David Gibson
Currently we don't have any unrealize path for pnv cpu cores. We get away
with this because we don't yet support cpu hotplug for pnv.
However, we're going to want it eventually, and in the meantime, it makes
it non-obvious why there are a bunch of allocations on the realize() path
that don't have matching frees.
So, implement the missing unrealize path.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Greg Kurz <groug@kaod.org>
---
hw/ppc/pnv_core.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
index f4c41d89d6..f7cf33f547 100644
--- a/hw/ppc/pnv_core.c
+++ b/hw/ppc/pnv_core.c
@@ -186,6 +186,26 @@ err:
error_propagate(errp, local_err);
}
+static void pnv_unrealize_vcpu(PowerPCCPU *cpu)
+{
+ qemu_unregister_reset(pnv_cpu_reset, cpu);
+ object_unparent(cpu->intc);
+ cpu_remove_sync(CPU(cpu));
+ object_unparent(OBJECT(cpu));
+}
+
+static void pnv_core_unrealize(DeviceState *dev, Error **errp)
+{
+ PnvCore *pc = PNV_CORE(dev);
+ CPUCore *cc = CPU_CORE(dev);
+ int i;
+
+ for (i = 0; i < cc->nr_threads; i++) {
+ pnv_unrealize_vcpu(pc->threads[i]);
+ }
+ g_free(pc->threads);
+}
+
static Property pnv_core_properties[] = {
DEFINE_PROP_UINT32("pir", PnvCore, pir, 0),
DEFINE_PROP_END_OF_LIST(),
@@ -196,6 +216,7 @@ static void pnv_core_class_init(ObjectClass *oc, void *data)
DeviceClass *dc = DEVICE_CLASS(oc);
dc->realize = pnv_core_realize;
+ dc->unrealize = pnv_core_unrealize;
dc->props = pnv_core_properties;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCHv3 6/7] target/ppc: Replace intc pointer with a general machine_data pointer
2018-06-14 4:41 [Qemu-devel] [PATCHv3 0/7] Better handling of machine specific per-cpu information David Gibson
` (4 preceding siblings ...)
2018-06-14 4:41 ` [Qemu-devel] [PATCHv3 5/7] pnv: Add cpu unrealize path David Gibson
@ 2018-06-14 4:41 ` David Gibson
2018-06-14 5:30 ` Cédric Le Goater
` (2 more replies)
2018-06-14 4:41 ` [Qemu-devel] [PATCHv3 7/7] target/ppc, spapr: Move VPA information to machine_data David Gibson
6 siblings, 3 replies; 15+ messages in thread
From: David Gibson @ 2018-06-14 4:41 UTC (permalink / raw)
To: groug; +Cc: clg, qemu-devel, qemu-ppc, David Gibson
PowerPCCPU contains an (Object *)intc used to point to the cpu's interrupt
controller. Or more precisely to the "presentation" component of the
interrupt controller relevant to this cpu.
Really, this field is machine specific. The machines which use it can
point it to different types of object depending on their needs, and most
machines don't use it at all (since they have older style PICs which don't
have per-cpu presentation components).
There's also other information that's per-cpu, but platform/machine
specific. So replace the intc pointer with a (void *)machine_data which
can be managed as the machine type likes to conveniently store per cpu
information.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Greg Kurz <groug@kaod.org>
---
hw/intc/xics.c | 5 +++--
hw/intc/xics_spapr.c | 16 +++++++++++-----
hw/ppc/pnv.c | 4 ++--
hw/ppc/pnv_core.c | 11 +++++++++--
hw/ppc/spapr.c | 8 ++++----
hw/ppc/spapr_cpu_core.c | 13 ++++++++++---
include/hw/ppc/pnv_core.h | 9 +++++++++
include/hw/ppc/spapr_cpu_core.h | 10 ++++++++++
include/hw/ppc/xics.h | 4 ++--
target/ppc/cpu.h | 2 +-
10 files changed, 61 insertions(+), 21 deletions(-)
diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index e73e623e3b..689ad44e5f 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -383,7 +383,8 @@ static const TypeInfo icp_info = {
.class_size = sizeof(ICPStateClass),
};
-Object *icp_create(Object *cpu, const char *type, XICSFabric *xi, Error **errp)
+ICPState *icp_create(Object *cpu, const char *type, XICSFabric *xi,
+ Error **errp)
{
Error *local_err = NULL;
Object *obj;
@@ -401,7 +402,7 @@ Object *icp_create(Object *cpu, const char *type, XICSFabric *xi, Error **errp)
obj = NULL;
}
- return obj;
+ return ICP(obj);
}
/*
diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
index 2e27b92b87..01c76717cf 100644
--- a/hw/intc/xics_spapr.c
+++ b/hw/intc/xics_spapr.c
@@ -31,6 +31,7 @@
#include "trace.h"
#include "qemu/timer.h"
#include "hw/ppc/spapr.h"
+#include "hw/ppc/spapr_cpu_core.h"
#include "hw/ppc/xics.h"
#include "hw/ppc/fdt.h"
#include "qapi/visitor.h"
@@ -43,8 +44,9 @@ static target_ulong h_cppr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
target_ulong opcode, target_ulong *args)
{
target_ulong cppr = args[0];
+ sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
- icp_set_cppr(ICP(cpu->intc), cppr);
+ icp_set_cppr(spapr_cpu->icp, cppr);
return H_SUCCESS;
}
@@ -65,7 +67,8 @@ static target_ulong h_ipi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
static target_ulong h_xirr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
target_ulong opcode, target_ulong *args)
{
- uint32_t xirr = icp_accept(ICP(cpu->intc));
+ sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
+ uint32_t xirr = icp_accept(spapr_cpu->icp);
args[0] = xirr;
return H_SUCCESS;
@@ -74,7 +77,8 @@ static target_ulong h_xirr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
static target_ulong h_xirr_x(PowerPCCPU *cpu, sPAPRMachineState *spapr,
target_ulong opcode, target_ulong *args)
{
- uint32_t xirr = icp_accept(ICP(cpu->intc));
+ sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
+ uint32_t xirr = icp_accept(spapr_cpu->icp);
args[0] = xirr;
args[1] = cpu_get_host_ticks();
@@ -84,9 +88,10 @@ static target_ulong h_xirr_x(PowerPCCPU *cpu, sPAPRMachineState *spapr,
static target_ulong h_eoi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
target_ulong opcode, target_ulong *args)
{
+ sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
target_ulong xirr = args[0];
- icp_eoi(ICP(cpu->intc), xirr);
+ icp_eoi(spapr_cpu->icp, xirr);
return H_SUCCESS;
}
@@ -94,7 +99,8 @@ static target_ulong h_ipoll(PowerPCCPU *cpu, sPAPRMachineState *spapr,
target_ulong opcode, target_ulong *args)
{
uint32_t mfrr;
- uint32_t xirr = icp_ipoll(ICP(cpu->intc), &mfrr);
+ sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
+ uint32_t xirr = icp_ipoll(spapr_cpu->icp, &mfrr);
args[0] = xirr;
args[1] = mfrr;
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 0b9508d94d..3a36c6ac6a 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1013,7 +1013,7 @@ static ICPState *pnv_icp_get(XICSFabric *xi, int pir)
{
PowerPCCPU *cpu = ppc_get_vcpu_by_pir(pir);
- return cpu ? ICP(cpu->intc) : NULL;
+ return cpu ? pnv_cpu_state(cpu)->icp : NULL;
}
static void pnv_pic_print_info(InterruptStatsProvider *obj,
@@ -1026,7 +1026,7 @@ static void pnv_pic_print_info(InterruptStatsProvider *obj,
CPU_FOREACH(cs) {
PowerPCCPU *cpu = POWERPC_CPU(cs);
- icp_pic_print_info(ICP(cpu->intc), mon);
+ icp_pic_print_info(pnv_cpu_state(cpu)->icp, mon);
}
for (i = 0; i < pnv->num_chips; i++) {
diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
index f7cf33f547..746bc5c2c5 100644
--- a/hw/ppc/pnv_core.c
+++ b/hw/ppc/pnv_core.c
@@ -105,6 +105,7 @@ static void pnv_realize_vcpu(PowerPCCPU *cpu, XICSFabric *xi, Error **errp)
int core_pir;
int thread_index = 0; /* TODO: TCG supports only one thread */
ppc_spr_t *pir = &env->spr_cb[SPR_PIR];
+ PnvCPUState *pnv_cpu;
Error *local_err = NULL;
object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
@@ -113,7 +114,9 @@ static void pnv_realize_vcpu(PowerPCCPU *cpu, XICSFabric *xi, Error **errp)
return;
}
- cpu->intc = icp_create(OBJECT(cpu), TYPE_PNV_ICP, xi, &local_err);
+ cpu->machine_data = pnv_cpu = g_new0(PnvCPUState, 1);
+
+ pnv_cpu->icp = icp_create(OBJECT(cpu), TYPE_PNV_ICP, xi, &local_err);
if (local_err) {
error_propagate(errp, local_err);
return;
@@ -188,8 +191,12 @@ err:
static void pnv_unrealize_vcpu(PowerPCCPU *cpu)
{
+ PnvCPUState *pnv_cpu = pnv_cpu_state(cpu);
+
qemu_unregister_reset(pnv_cpu_reset, cpu);
- object_unparent(cpu->intc);
+ object_unparent(OBJECT(pnv_cpu->icp));
+ cpu->machine_data = NULL;
+ g_free(pnv_cpu);
cpu_remove_sync(CPU(cpu));
object_unparent(OBJECT(cpu));
}
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index f59999daac..cbab6b6b7e 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1745,8 +1745,8 @@ static int spapr_post_load(void *opaque, int version_id)
if (!object_dynamic_cast(OBJECT(spapr->ics), TYPE_ICS_KVM)) {
CPUState *cs;
CPU_FOREACH(cs) {
- PowerPCCPU *cpu = POWERPC_CPU(cs);
- icp_resend(ICP(cpu->intc));
+ sPAPRCPUState *spapr_cpu = spapr_cpu_state(POWERPC_CPU(cs));
+ icp_resend(spapr_cpu->icp);
}
}
@@ -3783,7 +3783,7 @@ static ICPState *spapr_icp_get(XICSFabric *xi, int vcpu_id)
{
PowerPCCPU *cpu = spapr_find_cpu(vcpu_id);
- return cpu ? ICP(cpu->intc) : NULL;
+ return cpu ? spapr_cpu_state(cpu)->icp : NULL;
}
#define ICS_IRQ_FREE(ics, srcno) \
@@ -3925,7 +3925,7 @@ static void spapr_pic_print_info(InterruptStatsProvider *obj,
CPU_FOREACH(cs) {
PowerPCCPU *cpu = POWERPC_CPU(cs);
- icp_pic_print_info(ICP(cpu->intc), mon);
+ icp_pic_print_info(spapr_cpu_state(cpu)->icp, mon);
}
ics_pic_print_info(spapr->ics, mon);
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 7fdb3b6667..544bda93e2 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -104,8 +104,12 @@ const char *spapr_get_cpu_core_type(const char *cpu_type)
static void spapr_unrealize_vcpu(PowerPCCPU *cpu)
{
+ sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
+
qemu_unregister_reset(spapr_cpu_reset, cpu);
- object_unparent(cpu->intc);
+ object_unparent(OBJECT(spapr_cpu->icp));
+ cpu->machine_data = NULL;
+ g_free(spapr_cpu);
cpu_remove_sync(CPU(cpu));
object_unparent(OBJECT(cpu));
}
@@ -127,12 +131,15 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, sPAPRMachineState *spapr,
{
CPUPPCState *env = &cpu->env;
Error *local_err = NULL;
+ sPAPRCPUState *spapr_cpu;
object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
if (local_err) {
goto error;
}
+ spapr_cpu = cpu->machine_data = g_new0(sPAPRCPUState, 1);
+
/* Set time-base frequency to 512 MHz */
cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
@@ -142,8 +149,8 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, sPAPRMachineState *spapr,
qemu_register_reset(spapr_cpu_reset, cpu);
spapr_cpu_reset(cpu);
- cpu->intc = icp_create(OBJECT(cpu), spapr->icp_type, XICS_FABRIC(spapr),
- &local_err);
+ spapr_cpu->icp = icp_create(OBJECT(cpu), spapr->icp_type,
+ XICS_FABRIC(spapr), &local_err);
if (local_err) {
goto error;
}
diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
index 447ae761f7..f81dff28aa 100644
--- a/include/hw/ppc/pnv_core.h
+++ b/include/hw/ppc/pnv_core.h
@@ -47,4 +47,13 @@ typedef struct PnvCoreClass {
#define PNV_CORE_TYPE_SUFFIX "-" TYPE_PNV_CORE
#define PNV_CORE_TYPE_NAME(cpu_model) cpu_model PNV_CORE_TYPE_SUFFIX
+typedef struct PnvCPUState {
+ ICPState *icp;
+} PnvCPUState;
+
+static inline PnvCPUState *pnv_cpu_state(PowerPCCPU *cpu)
+{
+ return cpu->machine_data;
+}
+
#endif /* _PPC_PNV_CORE_H */
diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
index 47dcfda12b..e3d2aa45a4 100644
--- a/include/hw/ppc/spapr_cpu_core.h
+++ b/include/hw/ppc/spapr_cpu_core.h
@@ -41,4 +41,14 @@ typedef struct sPAPRCPUCoreClass {
const char *spapr_get_cpu_core_type(const char *cpu_type);
void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3);
+typedef struct ICPState ICPState;
+typedef struct sPAPRCPUState {
+ ICPState *icp;
+} sPAPRCPUState;
+
+static inline sPAPRCPUState *spapr_cpu_state(PowerPCCPU *cpu)
+{
+ return (sPAPRCPUState *)cpu->machine_data;
+}
+
#endif
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 6cebff47a7..48930d91e5 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -207,7 +207,7 @@ typedef struct sPAPRMachineState sPAPRMachineState;
int xics_kvm_init(sPAPRMachineState *spapr, Error **errp);
void xics_spapr_init(sPAPRMachineState *spapr);
-Object *icp_create(Object *cpu, const char *type, XICSFabric *xi,
- Error **errp);
+ICPState *icp_create(Object *cpu, const char *type, XICSFabric *xi,
+ Error **errp);
#endif /* XICS_H */
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index a91f1a8777..abf0bf0224 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1204,7 +1204,7 @@ struct PowerPCCPU {
int vcpu_id;
uint32_t compat_pvr;
PPCVirtualHypervisor *vhyp;
- Object *intc;
+ void *machine_data;
int32_t node_id; /* NUMA node this CPU belongs to */
PPCHash64Options *hash64_opts;
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCHv3 7/7] target/ppc, spapr: Move VPA information to machine_data
2018-06-14 4:41 [Qemu-devel] [PATCHv3 0/7] Better handling of machine specific per-cpu information David Gibson
` (5 preceding siblings ...)
2018-06-14 4:41 ` [Qemu-devel] [PATCHv3 6/7] target/ppc: Replace intc pointer with a general machine_data pointer David Gibson
@ 2018-06-14 4:41 ` David Gibson
6 siblings, 0 replies; 15+ messages in thread
From: David Gibson @ 2018-06-14 4:41 UTC (permalink / raw)
To: groug; +Cc: clg, qemu-devel, qemu-ppc, David Gibson
CPUPPCState currently contains a number of fields containing the state of
the VPA. The VPA is a PAPR specific concept covering several guest/host
shared memory areas used to communicate some information with the
hypervisor.
As a PAPR concept this is really machine specific information, although it
is per-cpu, so it doesn't really belong in the core CPU state structure.
So, move it to the PAPR specific 'machine_data' structure.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Greg Kurz <groug@kaod.org>
---
hw/ppc/spapr_cpu_core.c | 7 +++
hw/ppc/spapr_hcall.c | 77 ++++++++++++++++++---------------
include/hw/ppc/spapr_cpu_core.h | 3 ++
target/ppc/cpu.h | 6 ---
target/ppc/kvm.c | 36 +++++++--------
target/ppc/translate_init.inc.c | 8 ----
6 files changed, 70 insertions(+), 67 deletions(-)
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 544bda93e2..f642c95967 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -28,6 +28,7 @@ static void spapr_cpu_reset(void *opaque)
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;
cpu_reset(cs);
@@ -69,6 +70,12 @@ static void spapr_cpu_reset(void *opaque)
/* Set a full AMOR so guest can use the AMR as it sees fit */
env->spr[SPR_AMOR] = 0xffffffffffffffffull;
+
+ spapr_cpu->vpa_addr = 0;
+ spapr_cpu->slb_shadow_addr = 0;
+ spapr_cpu->slb_shadow_size = 0;
+ spapr_cpu->dtl_addr = 0;
+ spapr_cpu->dtl_size = 0;
}
void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3)
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 8b9a4b577f..ae913d070f 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -8,6 +8,7 @@
#include "exec/exec-all.h"
#include "helper_regs.h"
#include "hw/ppc/spapr.h"
+#include "hw/ppc/spapr_cpu_core.h"
#include "mmu-hash64.h"
#include "cpu-models.h"
#include "trace.h"
@@ -908,9 +909,11 @@ unmap_out:
#define VPA_SHARED_PROC_OFFSET 0x9
#define VPA_SHARED_PROC_VAL 0x2
-static target_ulong register_vpa(CPUPPCState *env, target_ulong vpa)
+static target_ulong register_vpa(PowerPCCPU *cpu, target_ulong vpa)
{
- CPUState *cs = CPU(ppc_env_get_cpu(env));
+ CPUState *cs = CPU(cpu);
+ CPUPPCState *env = &cpu->env;
+ sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
uint16_t size;
uint8_t tmp;
@@ -935,32 +938,34 @@ static target_ulong register_vpa(CPUPPCState *env, target_ulong vpa)
return H_PARAMETER;
}
- env->vpa_addr = vpa;
+ spapr_cpu->vpa_addr = vpa;
- tmp = ldub_phys(cs->as, env->vpa_addr + VPA_SHARED_PROC_OFFSET);
+ tmp = ldub_phys(cs->as, spapr_cpu->vpa_addr + VPA_SHARED_PROC_OFFSET);
tmp |= VPA_SHARED_PROC_VAL;
- stb_phys(cs->as, env->vpa_addr + VPA_SHARED_PROC_OFFSET, tmp);
+ stb_phys(cs->as, spapr_cpu->vpa_addr + VPA_SHARED_PROC_OFFSET, tmp);
return H_SUCCESS;
}
-static target_ulong deregister_vpa(CPUPPCState *env, target_ulong vpa)
+static target_ulong deregister_vpa(PowerPCCPU *cpu, target_ulong vpa)
{
- if (env->slb_shadow_addr) {
+ sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
+
+ if (spapr_cpu->slb_shadow_addr) {
return H_RESOURCE;
}
- if (env->dtl_addr) {
+ if (spapr_cpu->dtl_addr) {
return H_RESOURCE;
}
- env->vpa_addr = 0;
+ spapr_cpu->vpa_addr = 0;
return H_SUCCESS;
}
-static target_ulong register_slb_shadow(CPUPPCState *env, target_ulong addr)
+static target_ulong register_slb_shadow(PowerPCCPU *cpu, target_ulong addr)
{
- CPUState *cs = CPU(ppc_env_get_cpu(env));
+ sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
uint32_t size;
if (addr == 0) {
@@ -968,7 +973,7 @@ static target_ulong register_slb_shadow(CPUPPCState *env, target_ulong addr)
return H_HARDWARE;
}
- size = ldl_be_phys(cs->as, addr + 0x4);
+ size = ldl_be_phys(CPU(cpu)->as, addr + 0x4);
if (size < 0x8) {
return H_PARAMETER;
}
@@ -977,26 +982,28 @@ static target_ulong register_slb_shadow(CPUPPCState *env, target_ulong addr)
return H_PARAMETER;
}
- if (!env->vpa_addr) {
+ if (!spapr_cpu->vpa_addr) {
return H_RESOURCE;
}
- env->slb_shadow_addr = addr;
- env->slb_shadow_size = size;
+ spapr_cpu->slb_shadow_addr = addr;
+ spapr_cpu->slb_shadow_size = size;
return H_SUCCESS;
}
-static target_ulong deregister_slb_shadow(CPUPPCState *env, target_ulong addr)
+static target_ulong deregister_slb_shadow(PowerPCCPU *cpu, target_ulong addr)
{
- env->slb_shadow_addr = 0;
- env->slb_shadow_size = 0;
+ sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
+
+ spapr_cpu->slb_shadow_addr = 0;
+ spapr_cpu->slb_shadow_size = 0;
return H_SUCCESS;
}
-static target_ulong register_dtl(CPUPPCState *env, target_ulong addr)
+static target_ulong register_dtl(PowerPCCPU *cpu, target_ulong addr)
{
- CPUState *cs = CPU(ppc_env_get_cpu(env));
+ sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
uint32_t size;
if (addr == 0) {
@@ -1004,26 +1011,28 @@ static target_ulong register_dtl(CPUPPCState *env, target_ulong addr)
return H_HARDWARE;
}
- size = ldl_be_phys(cs->as, addr + 0x4);
+ size = ldl_be_phys(CPU(cpu)->as, addr + 0x4);
if (size < 48) {
return H_PARAMETER;
}
- if (!env->vpa_addr) {
+ if (!spapr_cpu->vpa_addr) {
return H_RESOURCE;
}
- env->dtl_addr = addr;
- env->dtl_size = size;
+ spapr_cpu->dtl_addr = addr;
+ spapr_cpu->dtl_size = size;
return H_SUCCESS;
}
-static target_ulong deregister_dtl(CPUPPCState *env, target_ulong addr)
+static target_ulong deregister_dtl(PowerPCCPU *cpu, target_ulong addr)
{
- env->dtl_addr = 0;
- env->dtl_size = 0;
+ sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
+
+ spapr_cpu->dtl_addr = 0;
+ spapr_cpu->dtl_size = 0;
return H_SUCCESS;
}
@@ -1035,38 +1044,36 @@ static target_ulong h_register_vpa(PowerPCCPU *cpu, sPAPRMachineState *spapr,
target_ulong procno = args[1];
target_ulong vpa = args[2];
target_ulong ret = H_PARAMETER;
- CPUPPCState *tenv;
PowerPCCPU *tcpu;
tcpu = spapr_find_cpu(procno);
if (!tcpu) {
return H_PARAMETER;
}
- tenv = &tcpu->env;
switch (flags) {
case FLAGS_REGISTER_VPA:
- ret = register_vpa(tenv, vpa);
+ ret = register_vpa(tcpu, vpa);
break;
case FLAGS_DEREGISTER_VPA:
- ret = deregister_vpa(tenv, vpa);
+ ret = deregister_vpa(tcpu, vpa);
break;
case FLAGS_REGISTER_SLBSHADOW:
- ret = register_slb_shadow(tenv, vpa);
+ ret = register_slb_shadow(tcpu, vpa);
break;
case FLAGS_DEREGISTER_SLBSHADOW:
- ret = deregister_slb_shadow(tenv, vpa);
+ ret = deregister_slb_shadow(tcpu, vpa);
break;
case FLAGS_REGISTER_DTL:
- ret = register_dtl(tenv, vpa);
+ ret = register_dtl(tcpu, vpa);
break;
case FLAGS_DEREGISTER_DTL:
- ret = deregister_dtl(tenv, vpa);
+ ret = deregister_dtl(tcpu, vpa);
break;
}
diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
index e3d2aa45a4..40129cc452 100644
--- a/include/hw/ppc/spapr_cpu_core.h
+++ b/include/hw/ppc/spapr_cpu_core.h
@@ -44,6 +44,9 @@ void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r
typedef struct ICPState ICPState;
typedef struct sPAPRCPUState {
ICPState *icp;
+ uint64_t vpa_addr;
+ uint64_t slb_shadow_addr, slb_shadow_size;
+ uint64_t dtl_addr, dtl_size;
} sPAPRCPUState;
static inline sPAPRCPUState *spapr_cpu_state(PowerPCCPU *cpu)
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index abf0bf0224..6c2f4d29f2 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1091,12 +1091,6 @@ struct CPUPPCState {
target_ulong rmls;
#endif
-#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
- uint64_t vpa_addr;
- uint64_t slb_shadow_addr, slb_shadow_size;
- uint64_t dtl_addr, dtl_size;
-#endif /* TARGET_PPC64 */
-
int error_code;
uint32_t pending_interrupts;
#if !defined(CONFIG_USER_ONLY)
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 7fe9d0126b..dd0de527f1 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -829,22 +829,22 @@ static int kvm_get_fp(CPUState *cs)
static int kvm_get_vpa(CPUState *cs)
{
PowerPCCPU *cpu = POWERPC_CPU(cs);
- CPUPPCState *env = &cpu->env;
+ sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
struct kvm_one_reg reg;
int ret;
reg.id = KVM_REG_PPC_VPA_ADDR;
- reg.addr = (uintptr_t)&env->vpa_addr;
+ reg.addr = (uintptr_t)&spapr_cpu->vpa_addr;
ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, ®);
if (ret < 0) {
DPRINTF("Unable to get VPA address from KVM: %s\n", strerror(errno));
return ret;
}
- assert((uintptr_t)&env->slb_shadow_size
- == ((uintptr_t)&env->slb_shadow_addr + 8));
+ assert((uintptr_t)&spapr_cpu->slb_shadow_size
+ == ((uintptr_t)&spapr_cpu->slb_shadow_addr + 8));
reg.id = KVM_REG_PPC_VPA_SLB;
- reg.addr = (uintptr_t)&env->slb_shadow_addr;
+ reg.addr = (uintptr_t)&spapr_cpu->slb_shadow_addr;
ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, ®);
if (ret < 0) {
DPRINTF("Unable to get SLB shadow state from KVM: %s\n",
@@ -852,9 +852,9 @@ static int kvm_get_vpa(CPUState *cs)
return ret;
}
- assert((uintptr_t)&env->dtl_size == ((uintptr_t)&env->dtl_addr + 8));
+ assert((uintptr_t)&spapr_cpu->dtl_size == ((uintptr_t)&spapr_cpu->dtl_addr + 8));
reg.id = KVM_REG_PPC_VPA_DTL;
- reg.addr = (uintptr_t)&env->dtl_addr;
+ reg.addr = (uintptr_t)&spapr_cpu->dtl_addr;
ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, ®);
if (ret < 0) {
DPRINTF("Unable to get dispatch trace log state from KVM: %s\n",
@@ -868,7 +868,7 @@ static int kvm_get_vpa(CPUState *cs)
static int kvm_put_vpa(CPUState *cs)
{
PowerPCCPU *cpu = POWERPC_CPU(cs);
- CPUPPCState *env = &cpu->env;
+ sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
struct kvm_one_reg reg;
int ret;
@@ -876,11 +876,11 @@ static int kvm_put_vpa(CPUState *cs)
* registered. That means when restoring state, if a VPA *is*
* registered, we need to set that up first. If not, we need to
* deregister the others before deregistering the master VPA */
- assert(env->vpa_addr || !(env->slb_shadow_addr || env->dtl_addr));
+ assert(spapr_cpu->vpa_addr || !(spapr_cpu->slb_shadow_addr || spapr_cpu->dtl_addr));
- if (env->vpa_addr) {
+ if (spapr_cpu->vpa_addr) {
reg.id = KVM_REG_PPC_VPA_ADDR;
- reg.addr = (uintptr_t)&env->vpa_addr;
+ reg.addr = (uintptr_t)&spapr_cpu->vpa_addr;
ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, ®);
if (ret < 0) {
DPRINTF("Unable to set VPA address to KVM: %s\n", strerror(errno));
@@ -888,19 +888,19 @@ static int kvm_put_vpa(CPUState *cs)
}
}
- assert((uintptr_t)&env->slb_shadow_size
- == ((uintptr_t)&env->slb_shadow_addr + 8));
+ assert((uintptr_t)&spapr_cpu->slb_shadow_size
+ == ((uintptr_t)&spapr_cpu->slb_shadow_addr + 8));
reg.id = KVM_REG_PPC_VPA_SLB;
- reg.addr = (uintptr_t)&env->slb_shadow_addr;
+ reg.addr = (uintptr_t)&spapr_cpu->slb_shadow_addr;
ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, ®);
if (ret < 0) {
DPRINTF("Unable to set SLB shadow state to KVM: %s\n", strerror(errno));
return ret;
}
- assert((uintptr_t)&env->dtl_size == ((uintptr_t)&env->dtl_addr + 8));
+ assert((uintptr_t)&spapr_cpu->dtl_size == ((uintptr_t)&spapr_cpu->dtl_addr + 8));
reg.id = KVM_REG_PPC_VPA_DTL;
- reg.addr = (uintptr_t)&env->dtl_addr;
+ reg.addr = (uintptr_t)&spapr_cpu->dtl_addr;
ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, ®);
if (ret < 0) {
DPRINTF("Unable to set dispatch trace log state to KVM: %s\n",
@@ -908,9 +908,9 @@ static int kvm_put_vpa(CPUState *cs)
return ret;
}
- if (!env->vpa_addr) {
+ if (!spapr_cpu->vpa_addr) {
reg.id = KVM_REG_PPC_VPA_ADDR;
- reg.addr = (uintptr_t)&env->vpa_addr;
+ reg.addr = (uintptr_t)&spapr_cpu->vpa_addr;
ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, ®);
if (ret < 0) {
DPRINTF("Unable to set VPA address to KVM: %s\n", strerror(errno));
diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
index bb9296f5a3..76d6f3fd5e 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -10316,14 +10316,6 @@ static void ppc_cpu_reset(CPUState *s)
s->exception_index = POWERPC_EXCP_NONE;
env->error_code = 0;
-#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
- env->vpa_addr = 0;
- env->slb_shadow_addr = 0;
- env->slb_shadow_size = 0;
- env->dtl_addr = 0;
- env->dtl_size = 0;
-#endif /* TARGET_PPC64 */
-
for (i = 0; i < ARRAY_SIZE(env->spr_cb); i++) {
ppc_spr_t *spr = &env->spr_cb[i];
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCHv3 2/7] pnv: Fix some error handling cpu realize()
2018-06-14 4:41 ` [Qemu-devel] [PATCHv3 2/7] pnv: Fix some error handling cpu realize() David Gibson
@ 2018-06-14 5:26 ` Cédric Le Goater
2018-06-14 6:35 ` Greg Kurz
1 sibling, 0 replies; 15+ messages in thread
From: Cédric Le Goater @ 2018-06-14 5:26 UTC (permalink / raw)
To: David Gibson, groug; +Cc: qemu-devel, qemu-ppc
On 06/14/2018 06:41 AM, David Gibson wrote:
> In pnv_core_realize() we call two functions with an Error * parameter in
> succession, which will go badly if they both cause errors. In fact, a
> failure in either of them indicates a qemu internal error, so we can just
> use &error_abort in both cases.
Yes. I think that most of the object_property_add_child() could use
&error_abort, object_property_add_const_link() also.
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Thanks,
C.
> ---
> hw/ppc/pnv_core.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index 13ad7d9e04..01f47c8037 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -172,12 +172,9 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
> object_initialize(obj, size, typename);
>
> snprintf(name, sizeof(name), "thread[%d]", i);
> - object_property_add_child(OBJECT(pc), name, obj, &local_err);
> + object_property_add_child(OBJECT(pc), name, obj, &error_abort);
> object_property_add_alias(obj, "core-pir", OBJECT(pc),
> - "pir", &local_err);
> - if (local_err) {
> - goto err;
> - }
> + "pir", &error_abort);
> object_unref(obj);
> }
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCHv3 6/7] target/ppc: Replace intc pointer with a general machine_data pointer
2018-06-14 4:41 ` [Qemu-devel] [PATCHv3 6/7] target/ppc: Replace intc pointer with a general machine_data pointer David Gibson
@ 2018-06-14 5:30 ` Cédric Le Goater
2018-06-14 13:34 ` Greg Kurz
2018-06-14 15:20 ` Cédric Le Goater
2 siblings, 0 replies; 15+ messages in thread
From: Cédric Le Goater @ 2018-06-14 5:30 UTC (permalink / raw)
To: David Gibson, groug; +Cc: qemu-devel, qemu-ppc
On 06/14/2018 06:41 AM, David Gibson wrote:
> PowerPCCPU contains an (Object *)intc used to point to the cpu's interrupt
> controller. Or more precisely to the "presentation" component of the
> interrupt controller relevant to this cpu.
>
> Really, this field is machine specific. The machines which use it can
> point it to different types of object depending on their needs, and most
> machines don't use it at all (since they have older style PICs which don't
> have per-cpu presentation components).
>
> There's also other information that's per-cpu, but platform/machine
> specific. So replace the intc pointer with a (void *)machine_data which
> can be managed as the machine type likes to conveniently store per cpu
> information.
I will add the XiveTCXT (Thread interrupt state context) on both machines.
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> Reviewed-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Thanks,
C.
> ---
> hw/intc/xics.c | 5 +++--
> hw/intc/xics_spapr.c | 16 +++++++++++-----
> hw/ppc/pnv.c | 4 ++--
> hw/ppc/pnv_core.c | 11 +++++++++--
> hw/ppc/spapr.c | 8 ++++----
> hw/ppc/spapr_cpu_core.c | 13 ++++++++++---
> include/hw/ppc/pnv_core.h | 9 +++++++++
> include/hw/ppc/spapr_cpu_core.h | 10 ++++++++++
> include/hw/ppc/xics.h | 4 ++--
> target/ppc/cpu.h | 2 +-
> 10 files changed, 61 insertions(+), 21 deletions(-)
>
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index e73e623e3b..689ad44e5f 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -383,7 +383,8 @@ static const TypeInfo icp_info = {
> .class_size = sizeof(ICPStateClass),
> };
>
> -Object *icp_create(Object *cpu, const char *type, XICSFabric *xi, Error **errp)
> +ICPState *icp_create(Object *cpu, const char *type, XICSFabric *xi,
> + Error **errp)
> {
> Error *local_err = NULL;
> Object *obj;
> @@ -401,7 +402,7 @@ Object *icp_create(Object *cpu, const char *type, XICSFabric *xi, Error **errp)
> obj = NULL;
> }
>
> - return obj;
> + return ICP(obj);
> }
>
> /*
> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> index 2e27b92b87..01c76717cf 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -31,6 +31,7 @@
> #include "trace.h"
> #include "qemu/timer.h"
> #include "hw/ppc/spapr.h"
> +#include "hw/ppc/spapr_cpu_core.h"
> #include "hw/ppc/xics.h"
> #include "hw/ppc/fdt.h"
> #include "qapi/visitor.h"
> @@ -43,8 +44,9 @@ static target_ulong h_cppr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> target_ulong opcode, target_ulong *args)
> {
> target_ulong cppr = args[0];
> + sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
>
> - icp_set_cppr(ICP(cpu->intc), cppr);
> + icp_set_cppr(spapr_cpu->icp, cppr);
> return H_SUCCESS;
> }
>
> @@ -65,7 +67,8 @@ static target_ulong h_ipi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> static target_ulong h_xirr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> target_ulong opcode, target_ulong *args)
> {
> - uint32_t xirr = icp_accept(ICP(cpu->intc));
> + sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> + uint32_t xirr = icp_accept(spapr_cpu->icp);
>
> args[0] = xirr;
> return H_SUCCESS;
> @@ -74,7 +77,8 @@ static target_ulong h_xirr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> static target_ulong h_xirr_x(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> target_ulong opcode, target_ulong *args)
> {
> - uint32_t xirr = icp_accept(ICP(cpu->intc));
> + sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> + uint32_t xirr = icp_accept(spapr_cpu->icp);
>
> args[0] = xirr;
> args[1] = cpu_get_host_ticks();
> @@ -84,9 +88,10 @@ static target_ulong h_xirr_x(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> static target_ulong h_eoi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> target_ulong opcode, target_ulong *args)
> {
> + sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> target_ulong xirr = args[0];
>
> - icp_eoi(ICP(cpu->intc), xirr);
> + icp_eoi(spapr_cpu->icp, xirr);
> return H_SUCCESS;
> }
>
> @@ -94,7 +99,8 @@ static target_ulong h_ipoll(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> target_ulong opcode, target_ulong *args)
> {
> uint32_t mfrr;
> - uint32_t xirr = icp_ipoll(ICP(cpu->intc), &mfrr);
> + sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> + uint32_t xirr = icp_ipoll(spapr_cpu->icp, &mfrr);
>
> args[0] = xirr;
> args[1] = mfrr;
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 0b9508d94d..3a36c6ac6a 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1013,7 +1013,7 @@ static ICPState *pnv_icp_get(XICSFabric *xi, int pir)
> {
> PowerPCCPU *cpu = ppc_get_vcpu_by_pir(pir);
>
> - return cpu ? ICP(cpu->intc) : NULL;
> + return cpu ? pnv_cpu_state(cpu)->icp : NULL;
> }
>
> static void pnv_pic_print_info(InterruptStatsProvider *obj,
> @@ -1026,7 +1026,7 @@ static void pnv_pic_print_info(InterruptStatsProvider *obj,
> CPU_FOREACH(cs) {
> PowerPCCPU *cpu = POWERPC_CPU(cs);
>
> - icp_pic_print_info(ICP(cpu->intc), mon);
> + icp_pic_print_info(pnv_cpu_state(cpu)->icp, mon);
> }
>
> for (i = 0; i < pnv->num_chips; i++) {
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index f7cf33f547..746bc5c2c5 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -105,6 +105,7 @@ static void pnv_realize_vcpu(PowerPCCPU *cpu, XICSFabric *xi, Error **errp)
> int core_pir;
> int thread_index = 0; /* TODO: TCG supports only one thread */
> ppc_spr_t *pir = &env->spr_cb[SPR_PIR];
> + PnvCPUState *pnv_cpu;
> Error *local_err = NULL;
>
> object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
> @@ -113,7 +114,9 @@ static void pnv_realize_vcpu(PowerPCCPU *cpu, XICSFabric *xi, Error **errp)
> return;
> }
>
> - cpu->intc = icp_create(OBJECT(cpu), TYPE_PNV_ICP, xi, &local_err);
> + cpu->machine_data = pnv_cpu = g_new0(PnvCPUState, 1);
> +
> + pnv_cpu->icp = icp_create(OBJECT(cpu), TYPE_PNV_ICP, xi, &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> return;
> @@ -188,8 +191,12 @@ err:
>
> static void pnv_unrealize_vcpu(PowerPCCPU *cpu)
> {
> + PnvCPUState *pnv_cpu = pnv_cpu_state(cpu);
> +
> qemu_unregister_reset(pnv_cpu_reset, cpu);
> - object_unparent(cpu->intc);
> + object_unparent(OBJECT(pnv_cpu->icp));
> + cpu->machine_data = NULL;
> + g_free(pnv_cpu);
> cpu_remove_sync(CPU(cpu));
> object_unparent(OBJECT(cpu));
> }
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index f59999daac..cbab6b6b7e 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1745,8 +1745,8 @@ static int spapr_post_load(void *opaque, int version_id)
> if (!object_dynamic_cast(OBJECT(spapr->ics), TYPE_ICS_KVM)) {
> CPUState *cs;
> CPU_FOREACH(cs) {
> - PowerPCCPU *cpu = POWERPC_CPU(cs);
> - icp_resend(ICP(cpu->intc));
> + sPAPRCPUState *spapr_cpu = spapr_cpu_state(POWERPC_CPU(cs));
> + icp_resend(spapr_cpu->icp);
> }
> }
>
> @@ -3783,7 +3783,7 @@ static ICPState *spapr_icp_get(XICSFabric *xi, int vcpu_id)
> {
> PowerPCCPU *cpu = spapr_find_cpu(vcpu_id);
>
> - return cpu ? ICP(cpu->intc) : NULL;
> + return cpu ? spapr_cpu_state(cpu)->icp : NULL;
> }
>
> #define ICS_IRQ_FREE(ics, srcno) \
> @@ -3925,7 +3925,7 @@ static void spapr_pic_print_info(InterruptStatsProvider *obj,
> CPU_FOREACH(cs) {
> PowerPCCPU *cpu = POWERPC_CPU(cs);
>
> - icp_pic_print_info(ICP(cpu->intc), mon);
> + icp_pic_print_info(spapr_cpu_state(cpu)->icp, mon);
> }
>
> ics_pic_print_info(spapr->ics, mon);
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 7fdb3b6667..544bda93e2 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -104,8 +104,12 @@ const char *spapr_get_cpu_core_type(const char *cpu_type)
>
> static void spapr_unrealize_vcpu(PowerPCCPU *cpu)
> {
> + sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> +
> qemu_unregister_reset(spapr_cpu_reset, cpu);
> - object_unparent(cpu->intc);
> + object_unparent(OBJECT(spapr_cpu->icp));
> + cpu->machine_data = NULL;
> + g_free(spapr_cpu);
> cpu_remove_sync(CPU(cpu));
> object_unparent(OBJECT(cpu));
> }
> @@ -127,12 +131,15 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> {
> CPUPPCState *env = &cpu->env;
> Error *local_err = NULL;
> + sPAPRCPUState *spapr_cpu;
>
> object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
> if (local_err) {
> goto error;
> }
>
> + spapr_cpu = cpu->machine_data = g_new0(sPAPRCPUState, 1);
> +
> /* Set time-base frequency to 512 MHz */
> cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
>
> @@ -142,8 +149,8 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> qemu_register_reset(spapr_cpu_reset, cpu);
> spapr_cpu_reset(cpu);
>
> - cpu->intc = icp_create(OBJECT(cpu), spapr->icp_type, XICS_FABRIC(spapr),
> - &local_err);
> + spapr_cpu->icp = icp_create(OBJECT(cpu), spapr->icp_type,
> + XICS_FABRIC(spapr), &local_err);
> if (local_err) {
> goto error;
> }
> diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
> index 447ae761f7..f81dff28aa 100644
> --- a/include/hw/ppc/pnv_core.h
> +++ b/include/hw/ppc/pnv_core.h
> @@ -47,4 +47,13 @@ typedef struct PnvCoreClass {
> #define PNV_CORE_TYPE_SUFFIX "-" TYPE_PNV_CORE
> #define PNV_CORE_TYPE_NAME(cpu_model) cpu_model PNV_CORE_TYPE_SUFFIX
>
> +typedef struct PnvCPUState {
> + ICPState *icp;
> +} PnvCPUState;
> +
> +static inline PnvCPUState *pnv_cpu_state(PowerPCCPU *cpu)
> +{
> + return cpu->machine_data;
> +}
> +
> #endif /* _PPC_PNV_CORE_H */
> diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> index 47dcfda12b..e3d2aa45a4 100644
> --- a/include/hw/ppc/spapr_cpu_core.h
> +++ b/include/hw/ppc/spapr_cpu_core.h
> @@ -41,4 +41,14 @@ typedef struct sPAPRCPUCoreClass {
> const char *spapr_get_cpu_core_type(const char *cpu_type);
> void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3);
>
> +typedef struct ICPState ICPState;
> +typedef struct sPAPRCPUState {
> + ICPState *icp;
> +} sPAPRCPUState;
> +
> +static inline sPAPRCPUState *spapr_cpu_state(PowerPCCPU *cpu)
> +{
> + return (sPAPRCPUState *)cpu->machine_data;
> +}
> +
> #endif
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 6cebff47a7..48930d91e5 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -207,7 +207,7 @@ typedef struct sPAPRMachineState sPAPRMachineState;
> int xics_kvm_init(sPAPRMachineState *spapr, Error **errp);
> void xics_spapr_init(sPAPRMachineState *spapr);
>
> -Object *icp_create(Object *cpu, const char *type, XICSFabric *xi,
> - Error **errp);
> +ICPState *icp_create(Object *cpu, const char *type, XICSFabric *xi,
> + Error **errp);
>
> #endif /* XICS_H */
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index a91f1a8777..abf0bf0224 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1204,7 +1204,7 @@ struct PowerPCCPU {
> int vcpu_id;
> uint32_t compat_pvr;
> PPCVirtualHypervisor *vhyp;
> - Object *intc;
> + void *machine_data;
> int32_t node_id; /* NUMA node this CPU belongs to */
> PPCHash64Options *hash64_opts;
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCHv3 2/7] pnv: Fix some error handling cpu realize()
2018-06-14 4:41 ` [Qemu-devel] [PATCHv3 2/7] pnv: Fix some error handling cpu realize() David Gibson
2018-06-14 5:26 ` Cédric Le Goater
@ 2018-06-14 6:35 ` Greg Kurz
1 sibling, 0 replies; 15+ messages in thread
From: Greg Kurz @ 2018-06-14 6:35 UTC (permalink / raw)
To: David Gibson; +Cc: clg, qemu-devel, qemu-ppc
On Thu, 14 Jun 2018 14:41:24 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:
> In pnv_core_realize() we call two functions with an Error * parameter in
> succession, which will go badly if they both cause errors. In fact, a
> failure in either of them indicates a qemu internal error, so we can just
> use &error_abort in both cases.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
Reviewed-by: Greg Kurz <groug@kaod.org>
> hw/ppc/pnv_core.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index 13ad7d9e04..01f47c8037 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -172,12 +172,9 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
> object_initialize(obj, size, typename);
>
> snprintf(name, sizeof(name), "thread[%d]", i);
> - object_property_add_child(OBJECT(pc), name, obj, &local_err);
> + object_property_add_child(OBJECT(pc), name, obj, &error_abort);
> object_property_add_alias(obj, "core-pir", OBJECT(pc),
> - "pir", &local_err);
> - if (local_err) {
> - goto err;
> - }
> + "pir", &error_abort);
> object_unref(obj);
> }
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCHv3 6/7] target/ppc: Replace intc pointer with a general machine_data pointer
2018-06-14 4:41 ` [Qemu-devel] [PATCHv3 6/7] target/ppc: Replace intc pointer with a general machine_data pointer David Gibson
2018-06-14 5:30 ` Cédric Le Goater
@ 2018-06-14 13:34 ` Greg Kurz
2018-06-15 0:19 ` David Gibson
2018-06-14 15:20 ` Cédric Le Goater
2 siblings, 1 reply; 15+ messages in thread
From: Greg Kurz @ 2018-06-14 13:34 UTC (permalink / raw)
To: David Gibson; +Cc: clg, qemu-devel, qemu-ppc
On Thu, 14 Jun 2018 14:41:28 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:
> PowerPCCPU contains an (Object *)intc used to point to the cpu's interrupt
> controller. Or more precisely to the "presentation" component of the
> interrupt controller relevant to this cpu.
>
> Really, this field is machine specific. The machines which use it can
> point it to different types of object depending on their needs, and most
> machines don't use it at all (since they have older style PICs which don't
> have per-cpu presentation components).
>
> There's also other information that's per-cpu, but platform/machine
> specific. So replace the intc pointer with a (void *)machine_data which
> can be managed as the machine type likes to conveniently store per cpu
> information.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> ---
Ouch... I am having some concerns with this patch now.
First, I gave a try to CPU hotplug with the full series applied. It
causes QEMU to crash:
Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffee3feaa0 (LWP 23290)]
kvm_arch_put_registers (cs=0x11497fd0, level=<optimized out>) at /home/greg/Work/qemu/qemu-spapr/target/ppc/kvm.c:1068
1068 if (kvm_put_vpa(cs) < 0) {
(gdb) p ((PowerPCCPU *)cs)->machine_data
$1 = (void *) 0x0
(gdb) thread apply all bt
Thread 6 (Thread 0x7fffee3feaa0 (LWP 23290)):
#0 kvm_arch_put_registers (cs=0x11497fd0, level=<optimized out>) at /home/greg/Work/qemu/qemu-spapr/target/ppc/kvm.c:1068
#1 0x00000000100b3a18 in do_kvm_cpu_synchronize_post_init (cpu=<optimized out>, arg=...) at /home/greg/Work/qemu/qemu-spapr/accel/kvm/kvm-all.c:1817
#2 0x00000000102c2d18 in process_queued_cpu_work (cpu=<optimized out>) at /home/greg/Work/qemu/qemu-spapr/cpus-common.c:342
#3 0x0000000010081e88 in qemu_wait_io_event_common (cpu=0x11497fd0) at /home/greg/Work/qemu/qemu-spapr/cpus.c:1158
#4 0x0000000010081f38 in qemu_wait_io_event (cpu=0x11497fd0) at /home/greg/Work/qemu/qemu-spapr/cpus.c:1185
#5 0x0000000010084248 in qemu_kvm_cpu_thread_fn (arg=0x11497fd0) at /home/greg/Work/qemu/qemu-spapr/cpus.c:1220
#6 0x0000000010608cb0 in qemu_thread_start (args=0x10eb0510) at /home/greg/Work/qemu/qemu-spapr/util/qemu-thread-posix.c:507
#7 0x00007ffff2758af4 in start_thread () from /lib64/libpthread.so.0
#8 0x00007ffff2688814 in clone () from /lib64/libc.so.6
[...]
Thread 1 (Thread 0x7ffff0ee2650 (LWP 23234)):
#0 0x00007ffff275e72c in pthread_cond_wait@@GLIBC_2.17 () from /lib64/libpthread.so.0
#1 0x00000000106095d0 in qemu_cond_wait_impl (cond=0x10cf7028 <qemu_work_cond>, mutex=0x10cd4d60 <qemu_global_mutex>, file=0x107b2ea8 "/home/greg/Work/qemu/qemu-spapr/cpus-common.c", line=<optimized out>) at /home/greg/Work/qemu/qemu-spapr/util/qemu-thread-posix.c:164
#2 0x00000000102c23d8 in do_run_on_cpu (cpu=<optimized out>, func=<optimized out>, data=..., mutex=0x10cd4d60 <qemu_global_mutex>) at /home/greg/Work/qemu/qemu-spapr/cpus-common.c:152
#3 0x0000000010083cb0 in run_on_cpu (cpu=<optimized out>, func=<optimized out>, data=...) at /home/greg/Work/qemu/qemu-spapr/cpus.c:1126
#4 0x00000000100b4bc4 in kvm_cpu_synchronize_post_init (cpu=<optimized out>) at /home/greg/Work/qemu/qemu-spapr/accel/kvm/kvm-all.c:1823
#5 0x000000001047dbe8 in cpu_synchronize_post_init (cpu=0x11497fd0) at /home/greg/Work/qemu/qemu-spapr/include/sysemu/hw_accel.h:48
#6 cpu_common_realizefn (dev=0x11497fd0, errp=<optimized out>) at /home/greg/Work/qemu/qemu-spapr/qom/cpu.c:347
#7 0x00000000101aded4 in ppc_cpu_realize (dev=0x11497fd0, errp=0x7fffffffce50) at /home/greg/Work/qemu/qemu-spapr/target/ppc/translate_init.inc.c:9695
#8 0x0000000010326410 in device_set_realized (obj=0x11497fd0, value=<optimized out>, errp=0x7fffffffd080) at /home/greg/Work/qemu/qemu-spapr/hw/core/qdev.c:826
#9 0x00000000104c6ae0 in property_set_bool (obj=0x11497fd0, v=<optimized out>, name=<optimized out>, opaque=0x1182a120, errp=0x7fffffffd080) at /home/greg/Work/qemu/qemu-spapr/qom/object.c:1930
#10 0x00000000104c9838 in object_property_set (obj=0x11497fd0, v=0x119a0e20, name=0x1074fd90 "realized", errp=0x7fffffffd080) at /home/greg/Work/qemu/qemu-spapr/qom/object.c:1122
#11 0x00000000104ccd6c in object_property_set_qobject (obj=0x11497fd0, value=<optimized out>, name=<optimized out>, errp=<optimized out>) at /home/greg/Work/qemu/qemu-spapr/qom/qom-qobject.c:27
#12 0x00000000104c9b00 in object_property_set_bool (obj=0x11497fd0, value=<optimized out>, name=<optimized out>, errp=<optimized out>) at /home/greg/Work/qemu/qemu-spapr/qom/object.c:1188
#13 0x0000000010168500 in spapr_realize_vcpu (errp=0x7fffffffd078, spapr=<optimized out>, cpu=0x11497fd0) at /home/greg/Work/qemu/qemu-spapr/hw/ppc/spapr_cpu_core.c:143
This happens when the core CPU realization code decides to copy the full
set of registers to KVM, but the machine_data pointer is still NULL.
So I think the fix belongs to this patch, see below.
> hw/intc/xics.c | 5 +++--
> hw/intc/xics_spapr.c | 16 +++++++++++-----
> hw/ppc/pnv.c | 4 ++--
> hw/ppc/pnv_core.c | 11 +++++++++--
> hw/ppc/spapr.c | 8 ++++----
> hw/ppc/spapr_cpu_core.c | 13 ++++++++++---
> include/hw/ppc/pnv_core.h | 9 +++++++++
> include/hw/ppc/spapr_cpu_core.h | 10 ++++++++++
> include/hw/ppc/xics.h | 4 ++--
> target/ppc/cpu.h | 2 +-
> 10 files changed, 61 insertions(+), 21 deletions(-)
>
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index e73e623e3b..689ad44e5f 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -383,7 +383,8 @@ static const TypeInfo icp_info = {
> .class_size = sizeof(ICPStateClass),
> };
>
> -Object *icp_create(Object *cpu, const char *type, XICSFabric *xi, Error **errp)
> +ICPState *icp_create(Object *cpu, const char *type, XICSFabric *xi,
> + Error **errp)
> {
> Error *local_err = NULL;
> Object *obj;
> @@ -401,7 +402,7 @@ Object *icp_create(Object *cpu, const char *type, XICSFabric *xi, Error **errp)
> obj = NULL;
> }
>
> - return obj;
> + return ICP(obj);
> }
>
> /*
> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> index 2e27b92b87..01c76717cf 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -31,6 +31,7 @@
> #include "trace.h"
> #include "qemu/timer.h"
> #include "hw/ppc/spapr.h"
> +#include "hw/ppc/spapr_cpu_core.h"
> #include "hw/ppc/xics.h"
> #include "hw/ppc/fdt.h"
> #include "qapi/visitor.h"
> @@ -43,8 +44,9 @@ static target_ulong h_cppr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> target_ulong opcode, target_ulong *args)
> {
> target_ulong cppr = args[0];
> + sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
>
> - icp_set_cppr(ICP(cpu->intc), cppr);
> + icp_set_cppr(spapr_cpu->icp, cppr);
> return H_SUCCESS;
> }
>
> @@ -65,7 +67,8 @@ static target_ulong h_ipi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> static target_ulong h_xirr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> target_ulong opcode, target_ulong *args)
> {
> - uint32_t xirr = icp_accept(ICP(cpu->intc));
> + sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> + uint32_t xirr = icp_accept(spapr_cpu->icp);
>
> args[0] = xirr;
> return H_SUCCESS;
> @@ -74,7 +77,8 @@ static target_ulong h_xirr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> static target_ulong h_xirr_x(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> target_ulong opcode, target_ulong *args)
> {
> - uint32_t xirr = icp_accept(ICP(cpu->intc));
> + sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> + uint32_t xirr = icp_accept(spapr_cpu->icp);
>
> args[0] = xirr;
> args[1] = cpu_get_host_ticks();
> @@ -84,9 +88,10 @@ static target_ulong h_xirr_x(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> static target_ulong h_eoi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> target_ulong opcode, target_ulong *args)
> {
> + sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> target_ulong xirr = args[0];
>
> - icp_eoi(ICP(cpu->intc), xirr);
> + icp_eoi(spapr_cpu->icp, xirr);
> return H_SUCCESS;
> }
>
> @@ -94,7 +99,8 @@ static target_ulong h_ipoll(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> target_ulong opcode, target_ulong *args)
> {
> uint32_t mfrr;
> - uint32_t xirr = icp_ipoll(ICP(cpu->intc), &mfrr);
> + sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> + uint32_t xirr = icp_ipoll(spapr_cpu->icp, &mfrr);
>
> args[0] = xirr;
> args[1] = mfrr;
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 0b9508d94d..3a36c6ac6a 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1013,7 +1013,7 @@ static ICPState *pnv_icp_get(XICSFabric *xi, int pir)
> {
> PowerPCCPU *cpu = ppc_get_vcpu_by_pir(pir);
>
> - return cpu ? ICP(cpu->intc) : NULL;
> + return cpu ? pnv_cpu_state(cpu)->icp : NULL;
> }
>
> static void pnv_pic_print_info(InterruptStatsProvider *obj,
> @@ -1026,7 +1026,7 @@ static void pnv_pic_print_info(InterruptStatsProvider *obj,
> CPU_FOREACH(cs) {
> PowerPCCPU *cpu = POWERPC_CPU(cs);
>
> - icp_pic_print_info(ICP(cpu->intc), mon);
> + icp_pic_print_info(pnv_cpu_state(cpu)->icp, mon);
> }
>
> for (i = 0; i < pnv->num_chips; i++) {
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index f7cf33f547..746bc5c2c5 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -105,6 +105,7 @@ static void pnv_realize_vcpu(PowerPCCPU *cpu, XICSFabric *xi, Error **errp)
> int core_pir;
> int thread_index = 0; /* TODO: TCG supports only one thread */
> ppc_spr_t *pir = &env->spr_cb[SPR_PIR];
> + PnvCPUState *pnv_cpu;
> Error *local_err = NULL;
>
> object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
> @@ -113,7 +114,9 @@ static void pnv_realize_vcpu(PowerPCCPU *cpu, XICSFabric *xi, Error **errp)
> return;
> }
>
> - cpu->intc = icp_create(OBJECT(cpu), TYPE_PNV_ICP, xi, &local_err);
> + cpu->machine_data = pnv_cpu = g_new0(PnvCPUState, 1);
> +
> + pnv_cpu->icp = icp_create(OBJECT(cpu), TYPE_PNV_ICP, xi, &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> return;
> @@ -188,8 +191,12 @@ err:
>
> static void pnv_unrealize_vcpu(PowerPCCPU *cpu)
> {
> + PnvCPUState *pnv_cpu = pnv_cpu_state(cpu);
> +
> qemu_unregister_reset(pnv_cpu_reset, cpu);
> - object_unparent(cpu->intc);
> + object_unparent(OBJECT(pnv_cpu->icp));
> + cpu->machine_data = NULL;
> + g_free(pnv_cpu);
> cpu_remove_sync(CPU(cpu));
> object_unparent(OBJECT(cpu));
> }
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index f59999daac..cbab6b6b7e 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1745,8 +1745,8 @@ static int spapr_post_load(void *opaque, int version_id)
> if (!object_dynamic_cast(OBJECT(spapr->ics), TYPE_ICS_KVM)) {
> CPUState *cs;
> CPU_FOREACH(cs) {
> - PowerPCCPU *cpu = POWERPC_CPU(cs);
> - icp_resend(ICP(cpu->intc));
> + sPAPRCPUState *spapr_cpu = spapr_cpu_state(POWERPC_CPU(cs));
> + icp_resend(spapr_cpu->icp);
> }
> }
>
> @@ -3783,7 +3783,7 @@ static ICPState *spapr_icp_get(XICSFabric *xi, int vcpu_id)
> {
> PowerPCCPU *cpu = spapr_find_cpu(vcpu_id);
>
> - return cpu ? ICP(cpu->intc) : NULL;
> + return cpu ? spapr_cpu_state(cpu)->icp : NULL;
> }
>
> #define ICS_IRQ_FREE(ics, srcno) \
> @@ -3925,7 +3925,7 @@ static void spapr_pic_print_info(InterruptStatsProvider *obj,
> CPU_FOREACH(cs) {
> PowerPCCPU *cpu = POWERPC_CPU(cs);
>
> - icp_pic_print_info(ICP(cpu->intc), mon);
> + icp_pic_print_info(spapr_cpu_state(cpu)->icp, mon);
> }
>
> ics_pic_print_info(spapr->ics, mon);
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 7fdb3b6667..544bda93e2 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -104,8 +104,12 @@ const char *spapr_get_cpu_core_type(const char *cpu_type)
>
> static void spapr_unrealize_vcpu(PowerPCCPU *cpu)
> {
> + sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> +
> qemu_unregister_reset(spapr_cpu_reset, cpu);
> - object_unparent(cpu->intc);
> + object_unparent(OBJECT(spapr_cpu->icp));
> + cpu->machine_data = NULL;
> + g_free(spapr_cpu);
> cpu_remove_sync(CPU(cpu));
> object_unparent(OBJECT(cpu));
> }
> @@ -127,12 +131,15 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> {
> CPUPPCState *env = &cpu->env;
> Error *local_err = NULL;
> + sPAPRCPUState *spapr_cpu;
>
> object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
> if (local_err) {
> goto error;
> }
>
> + spapr_cpu = cpu->machine_data = g_new0(sPAPRCPUState, 1);
> +
machine_data is a cpu attribute and all users of spapr_cpu_state() assume it
is always valid. It should be set before any code tries to dereference it,
which I believe cannot happen before the call to object_property_set_bool().
So I guess setting it at the beginning of the function should be fine (at
least, it fixes the crash).
And now, looking at the function again, I realize it doesn't do any rollback
in case of error... and spapr_cpu_core_realize() doesn't care to rollback
already realized CPUs either, but it will happily free all them :)
Simulating a failure in icp_create() during CPU hotplug instantly crashes
QEMU:
(qemu) device_add host-spapr-cpu-core,core-id=1,id=gku
GKU: failing icp_create (cpu 0x11497fd0)
Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffee3feaa0 (LWP 24725)]
0x00000000104c8374 in object_dynamic_cast_assert (obj=0x11497fd0, typename=0x107904a0 "powerpc64-cpu", file=0x1079a1b0 "/home/greg/Work/qemu/qemu-spapr/target/ppc/kvm.c", line=658, func=0x10799c80 <__func__.31720> "kvm_put_one_spr") at /home/greg/Work/qemu/qemu-spapr/qom/object.c:623
623 trace_object_dynamic_cast_assert(obj ? obj->class->type->name : "(null)",
(gdb) p obj->class->type
$1 = (Type) 0x0
(gdb) p * obj
$2 = {class = 0x10ea9c10, free = 0x11244620, properties = 0x0, ref = 0, parent = 0x0}
obj is a dangling pointer to the CPU that was just destroyed in spapr_cpu_core_realize().
This is a long standing issue actually, not related to this patch. But your cleanup
helped to uncover it. I'll send patches soon.
> /* Set time-base frequency to 512 MHz */
> cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
>
> @@ -142,8 +149,8 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> qemu_register_reset(spapr_cpu_reset, cpu);
> spapr_cpu_reset(cpu);
>
> - cpu->intc = icp_create(OBJECT(cpu), spapr->icp_type, XICS_FABRIC(spapr),
> - &local_err);
> + spapr_cpu->icp = icp_create(OBJECT(cpu), spapr->icp_type,
> + XICS_FABRIC(spapr), &local_err);
> if (local_err) {
> goto error;
> }
> diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
> index 447ae761f7..f81dff28aa 100644
> --- a/include/hw/ppc/pnv_core.h
> +++ b/include/hw/ppc/pnv_core.h
> @@ -47,4 +47,13 @@ typedef struct PnvCoreClass {
> #define PNV_CORE_TYPE_SUFFIX "-" TYPE_PNV_CORE
> #define PNV_CORE_TYPE_NAME(cpu_model) cpu_model PNV_CORE_TYPE_SUFFIX
>
> +typedef struct PnvCPUState {
> + ICPState *icp;
> +} PnvCPUState;
> +
> +static inline PnvCPUState *pnv_cpu_state(PowerPCCPU *cpu)
> +{
> + return cpu->machine_data;
> +}
> +
> #endif /* _PPC_PNV_CORE_H */
> diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> index 47dcfda12b..e3d2aa45a4 100644
> --- a/include/hw/ppc/spapr_cpu_core.h
> +++ b/include/hw/ppc/spapr_cpu_core.h
> @@ -41,4 +41,14 @@ typedef struct sPAPRCPUCoreClass {
> const char *spapr_get_cpu_core_type(const char *cpu_type);
> void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3);
>
> +typedef struct ICPState ICPState;
> +typedef struct sPAPRCPUState {
> + ICPState *icp;
> +} sPAPRCPUState;
> +
> +static inline sPAPRCPUState *spapr_cpu_state(PowerPCCPU *cpu)
> +{
> + return (sPAPRCPUState *)cpu->machine_data;
> +}
> +
> #endif
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 6cebff47a7..48930d91e5 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -207,7 +207,7 @@ typedef struct sPAPRMachineState sPAPRMachineState;
> int xics_kvm_init(sPAPRMachineState *spapr, Error **errp);
> void xics_spapr_init(sPAPRMachineState *spapr);
>
> -Object *icp_create(Object *cpu, const char *type, XICSFabric *xi,
> - Error **errp);
> +ICPState *icp_create(Object *cpu, const char *type, XICSFabric *xi,
> + Error **errp);
>
> #endif /* XICS_H */
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index a91f1a8777..abf0bf0224 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1204,7 +1204,7 @@ struct PowerPCCPU {
> int vcpu_id;
> uint32_t compat_pvr;
> PPCVirtualHypervisor *vhyp;
> - Object *intc;
> + void *machine_data;
> int32_t node_id; /* NUMA node this CPU belongs to */
> PPCHash64Options *hash64_opts;
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCHv3 6/7] target/ppc: Replace intc pointer with a general machine_data pointer
2018-06-14 4:41 ` [Qemu-devel] [PATCHv3 6/7] target/ppc: Replace intc pointer with a general machine_data pointer David Gibson
2018-06-14 5:30 ` Cédric Le Goater
2018-06-14 13:34 ` Greg Kurz
@ 2018-06-14 15:20 ` Cédric Le Goater
2018-06-15 1:34 ` David Gibson
2 siblings, 1 reply; 15+ messages in thread
From: Cédric Le Goater @ 2018-06-14 15:20 UTC (permalink / raw)
To: David Gibson, groug; +Cc: qemu-devel, qemu-ppc
On 06/14/2018 06:41 AM, David Gibson wrote:
> PowerPCCPU contains an (Object *)intc used to point to the cpu's interrupt
> controller. Or more precisely to the "presentation" component of the
> interrupt controller relevant to this cpu.
>
> Really, this field is machine specific. The machines which use it can
> point it to different types of object depending on their needs, and most
> machines don't use it at all (since they have older style PICs which don't
> have per-cpu presentation components).
>
> There's also other information that's per-cpu, but platform/machine
> specific. So replace the intc pointer with a (void *)machine_data which
> can be managed as the machine type likes to conveniently store per cpu
> information.
I took a closer look. I tried to port the XIVE models on top this
new machine data field and it does not work out at all.
The problem is that the XIVE models are common to PowerNV and sPAPR,
they are machine agnostic, and when the cpu->intc pointer is used in
the TIMA ops, we have no idea in which machine the CPU are running.
We only expect the CPU to be a POWER9. We need a CPU related pointer
and not a machine data pointer, but we could imagine having a machine
data pointer under the CPU data pointer but the first need is a CPU
data.
Sorry, not have said it ealier, but I will just add back the intc
pointer under the CPU, for the moment.
C.
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> ---
> hw/intc/xics.c | 5 +++--
> hw/intc/xics_spapr.c | 16 +++++++++++-----
> hw/ppc/pnv.c | 4 ++--
> hw/ppc/pnv_core.c | 11 +++++++++--
> hw/ppc/spapr.c | 8 ++++----
> hw/ppc/spapr_cpu_core.c | 13 ++++++++++---
> include/hw/ppc/pnv_core.h | 9 +++++++++
> include/hw/ppc/spapr_cpu_core.h | 10 ++++++++++
> include/hw/ppc/xics.h | 4 ++--
> target/ppc/cpu.h | 2 +-
> 10 files changed, 61 insertions(+), 21 deletions(-)
>
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index e73e623e3b..689ad44e5f 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -383,7 +383,8 @@ static const TypeInfo icp_info = {
> .class_size = sizeof(ICPStateClass),
> };
>
> -Object *icp_create(Object *cpu, const char *type, XICSFabric *xi, Error **errp)
> +ICPState *icp_create(Object *cpu, const char *type, XICSFabric *xi,
> + Error **errp)
> {
> Error *local_err = NULL;
> Object *obj;
> @@ -401,7 +402,7 @@ Object *icp_create(Object *cpu, const char *type, XICSFabric *xi, Error **errp)
> obj = NULL;
> }
>
> - return obj;
> + return ICP(obj);
> }
>
> /*
> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> index 2e27b92b87..01c76717cf 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -31,6 +31,7 @@
> #include "trace.h"
> #include "qemu/timer.h"
> #include "hw/ppc/spapr.h"
> +#include "hw/ppc/spapr_cpu_core.h"
> #include "hw/ppc/xics.h"
> #include "hw/ppc/fdt.h"
> #include "qapi/visitor.h"
> @@ -43,8 +44,9 @@ static target_ulong h_cppr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> target_ulong opcode, target_ulong *args)
> {
> target_ulong cppr = args[0];
> + sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
>
> - icp_set_cppr(ICP(cpu->intc), cppr);
> + icp_set_cppr(spapr_cpu->icp, cppr);
> return H_SUCCESS;
> }
>
> @@ -65,7 +67,8 @@ static target_ulong h_ipi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> static target_ulong h_xirr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> target_ulong opcode, target_ulong *args)
> {
> - uint32_t xirr = icp_accept(ICP(cpu->intc));
> + sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> + uint32_t xirr = icp_accept(spapr_cpu->icp);
>
> args[0] = xirr;
> return H_SUCCESS;
> @@ -74,7 +77,8 @@ static target_ulong h_xirr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> static target_ulong h_xirr_x(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> target_ulong opcode, target_ulong *args)
> {
> - uint32_t xirr = icp_accept(ICP(cpu->intc));
> + sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> + uint32_t xirr = icp_accept(spapr_cpu->icp);
>
> args[0] = xirr;
> args[1] = cpu_get_host_ticks();
> @@ -84,9 +88,10 @@ static target_ulong h_xirr_x(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> static target_ulong h_eoi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> target_ulong opcode, target_ulong *args)
> {
> + sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> target_ulong xirr = args[0];
>
> - icp_eoi(ICP(cpu->intc), xirr);
> + icp_eoi(spapr_cpu->icp, xirr);
> return H_SUCCESS;
> }
>
> @@ -94,7 +99,8 @@ static target_ulong h_ipoll(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> target_ulong opcode, target_ulong *args)
> {
> uint32_t mfrr;
> - uint32_t xirr = icp_ipoll(ICP(cpu->intc), &mfrr);
> + sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> + uint32_t xirr = icp_ipoll(spapr_cpu->icp, &mfrr);
>
> args[0] = xirr;
> args[1] = mfrr;
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 0b9508d94d..3a36c6ac6a 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1013,7 +1013,7 @@ static ICPState *pnv_icp_get(XICSFabric *xi, int pir)
> {
> PowerPCCPU *cpu = ppc_get_vcpu_by_pir(pir);
>
> - return cpu ? ICP(cpu->intc) : NULL;
> + return cpu ? pnv_cpu_state(cpu)->icp : NULL;
> }
>
> static void pnv_pic_print_info(InterruptStatsProvider *obj,
> @@ -1026,7 +1026,7 @@ static void pnv_pic_print_info(InterruptStatsProvider *obj,
> CPU_FOREACH(cs) {
> PowerPCCPU *cpu = POWERPC_CPU(cs);
>
> - icp_pic_print_info(ICP(cpu->intc), mon);
> + icp_pic_print_info(pnv_cpu_state(cpu)->icp, mon);
> }
>
> for (i = 0; i < pnv->num_chips; i++) {
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index f7cf33f547..746bc5c2c5 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -105,6 +105,7 @@ static void pnv_realize_vcpu(PowerPCCPU *cpu, XICSFabric *xi, Error **errp)
> int core_pir;
> int thread_index = 0; /* TODO: TCG supports only one thread */
> ppc_spr_t *pir = &env->spr_cb[SPR_PIR];
> + PnvCPUState *pnv_cpu;
> Error *local_err = NULL;
>
> object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
> @@ -113,7 +114,9 @@ static void pnv_realize_vcpu(PowerPCCPU *cpu, XICSFabric *xi, Error **errp)
> return;
> }
>
> - cpu->intc = icp_create(OBJECT(cpu), TYPE_PNV_ICP, xi, &local_err);
> + cpu->machine_data = pnv_cpu = g_new0(PnvCPUState, 1);
> +
> + pnv_cpu->icp = icp_create(OBJECT(cpu), TYPE_PNV_ICP, xi, &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> return;
> @@ -188,8 +191,12 @@ err:
>
> static void pnv_unrealize_vcpu(PowerPCCPU *cpu)
> {
> + PnvCPUState *pnv_cpu = pnv_cpu_state(cpu);
> +
> qemu_unregister_reset(pnv_cpu_reset, cpu);
> - object_unparent(cpu->intc);
> + object_unparent(OBJECT(pnv_cpu->icp));
> + cpu->machine_data = NULL;
> + g_free(pnv_cpu);
> cpu_remove_sync(CPU(cpu));
> object_unparent(OBJECT(cpu));
> }
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index f59999daac..cbab6b6b7e 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1745,8 +1745,8 @@ static int spapr_post_load(void *opaque, int version_id)
> if (!object_dynamic_cast(OBJECT(spapr->ics), TYPE_ICS_KVM)) {
> CPUState *cs;
> CPU_FOREACH(cs) {
> - PowerPCCPU *cpu = POWERPC_CPU(cs);
> - icp_resend(ICP(cpu->intc));
> + sPAPRCPUState *spapr_cpu = spapr_cpu_state(POWERPC_CPU(cs));
> + icp_resend(spapr_cpu->icp);
> }
> }
>
> @@ -3783,7 +3783,7 @@ static ICPState *spapr_icp_get(XICSFabric *xi, int vcpu_id)
> {
> PowerPCCPU *cpu = spapr_find_cpu(vcpu_id);
>
> - return cpu ? ICP(cpu->intc) : NULL;
> + return cpu ? spapr_cpu_state(cpu)->icp : NULL;
> }
>
> #define ICS_IRQ_FREE(ics, srcno) \
> @@ -3925,7 +3925,7 @@ static void spapr_pic_print_info(InterruptStatsProvider *obj,
> CPU_FOREACH(cs) {
> PowerPCCPU *cpu = POWERPC_CPU(cs);
>
> - icp_pic_print_info(ICP(cpu->intc), mon);
> + icp_pic_print_info(spapr_cpu_state(cpu)->icp, mon);
> }
>
> ics_pic_print_info(spapr->ics, mon);
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 7fdb3b6667..544bda93e2 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -104,8 +104,12 @@ const char *spapr_get_cpu_core_type(const char *cpu_type)
>
> static void spapr_unrealize_vcpu(PowerPCCPU *cpu)
> {
> + sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> +
> qemu_unregister_reset(spapr_cpu_reset, cpu);
> - object_unparent(cpu->intc);
> + object_unparent(OBJECT(spapr_cpu->icp));
> + cpu->machine_data = NULL;
> + g_free(spapr_cpu);
> cpu_remove_sync(CPU(cpu));
> object_unparent(OBJECT(cpu));
> }
> @@ -127,12 +131,15 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> {
> CPUPPCState *env = &cpu->env;
> Error *local_err = NULL;
> + sPAPRCPUState *spapr_cpu;
>
> object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
> if (local_err) {
> goto error;
> }
>
> + spapr_cpu = cpu->machine_data = g_new0(sPAPRCPUState, 1);
> +
> /* Set time-base frequency to 512 MHz */
> cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
>
> @@ -142,8 +149,8 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> qemu_register_reset(spapr_cpu_reset, cpu);
> spapr_cpu_reset(cpu);
>
> - cpu->intc = icp_create(OBJECT(cpu), spapr->icp_type, XICS_FABRIC(spapr),
> - &local_err);
> + spapr_cpu->icp = icp_create(OBJECT(cpu), spapr->icp_type,
> + XICS_FABRIC(spapr), &local_err);
> if (local_err) {
> goto error;
> }
> diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
> index 447ae761f7..f81dff28aa 100644
> --- a/include/hw/ppc/pnv_core.h
> +++ b/include/hw/ppc/pnv_core.h
> @@ -47,4 +47,13 @@ typedef struct PnvCoreClass {
> #define PNV_CORE_TYPE_SUFFIX "-" TYPE_PNV_CORE
> #define PNV_CORE_TYPE_NAME(cpu_model) cpu_model PNV_CORE_TYPE_SUFFIX
>
> +typedef struct PnvCPUState {
> + ICPState *icp;
> +} PnvCPUState;
> +
> +static inline PnvCPUState *pnv_cpu_state(PowerPCCPU *cpu)
> +{
> + return cpu->machine_data;
> +}
> +
> #endif /* _PPC_PNV_CORE_H */
> diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> index 47dcfda12b..e3d2aa45a4 100644
> --- a/include/hw/ppc/spapr_cpu_core.h
> +++ b/include/hw/ppc/spapr_cpu_core.h
> @@ -41,4 +41,14 @@ typedef struct sPAPRCPUCoreClass {
> const char *spapr_get_cpu_core_type(const char *cpu_type);
> void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3);
>
> +typedef struct ICPState ICPState;
> +typedef struct sPAPRCPUState {
> + ICPState *icp;
> +} sPAPRCPUState;
> +
> +static inline sPAPRCPUState *spapr_cpu_state(PowerPCCPU *cpu)
> +{
> + return (sPAPRCPUState *)cpu->machine_data;
> +}
> +
> #endif
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 6cebff47a7..48930d91e5 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -207,7 +207,7 @@ typedef struct sPAPRMachineState sPAPRMachineState;
> int xics_kvm_init(sPAPRMachineState *spapr, Error **errp);
> void xics_spapr_init(sPAPRMachineState *spapr);
>
> -Object *icp_create(Object *cpu, const char *type, XICSFabric *xi,
> - Error **errp);
> +ICPState *icp_create(Object *cpu, const char *type, XICSFabric *xi,
> + Error **errp);
>
> #endif /* XICS_H */
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index a91f1a8777..abf0bf0224 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1204,7 +1204,7 @@ struct PowerPCCPU {
> int vcpu_id;
> uint32_t compat_pvr;
> PPCVirtualHypervisor *vhyp;
> - Object *intc;
> + void *machine_data;
> int32_t node_id; /* NUMA node this CPU belongs to */
> PPCHash64Options *hash64_opts;
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCHv3 6/7] target/ppc: Replace intc pointer with a general machine_data pointer
2018-06-14 13:34 ` Greg Kurz
@ 2018-06-15 0:19 ` David Gibson
0 siblings, 0 replies; 15+ messages in thread
From: David Gibson @ 2018-06-15 0:19 UTC (permalink / raw)
To: Greg Kurz; +Cc: clg, qemu-devel, qemu-ppc
[-- Attachment #1: Type: text/plain, Size: 14686 bytes --]
On Thu, Jun 14, 2018 at 03:34:43PM +0200, Greg Kurz wrote:
> On Thu, 14 Jun 2018 14:41:28 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
> > PowerPCCPU contains an (Object *)intc used to point to the cpu's interrupt
> > controller. Or more precisely to the "presentation" component of the
> > interrupt controller relevant to this cpu.
> >
> > Really, this field is machine specific. The machines which use it can
> > point it to different types of object depending on their needs, and most
> > machines don't use it at all (since they have older style PICs which don't
> > have per-cpu presentation components).
> >
> > There's also other information that's per-cpu, but platform/machine
> > specific. So replace the intc pointer with a (void *)machine_data which
> > can be managed as the machine type likes to conveniently store per cpu
> > information.
> >
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > Reviewed-by: Greg Kurz <groug@kaod.org>
> > ---
>
> Ouch... I am having some concerns with this patch now.
>
> First, I gave a try to CPU hotplug with the full series applied. It
> causes QEMU to crash:
>
> Program received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 0x7fffee3feaa0 (LWP 23290)]
> kvm_arch_put_registers (cs=0x11497fd0, level=<optimized out>) at /home/greg/Work/qemu/qemu-spapr/target/ppc/kvm.c:1068
> 1068 if (kvm_put_vpa(cs) < 0) {
> (gdb) p ((PowerPCCPU *)cs)->machine_data
> $1 = (void *) 0x0
> (gdb) thread apply all bt
>
> Thread 6 (Thread 0x7fffee3feaa0 (LWP 23290)):
> #0 kvm_arch_put_registers (cs=0x11497fd0, level=<optimized out>) at /home/greg/Work/qemu/qemu-spapr/target/ppc/kvm.c:1068
> #1 0x00000000100b3a18 in do_kvm_cpu_synchronize_post_init (cpu=<optimized out>, arg=...) at /home/greg/Work/qemu/qemu-spapr/accel/kvm/kvm-all.c:1817
> #2 0x00000000102c2d18 in process_queued_cpu_work (cpu=<optimized out>) at /home/greg/Work/qemu/qemu-spapr/cpus-common.c:342
> #3 0x0000000010081e88 in qemu_wait_io_event_common (cpu=0x11497fd0) at /home/greg/Work/qemu/qemu-spapr/cpus.c:1158
> #4 0x0000000010081f38 in qemu_wait_io_event (cpu=0x11497fd0) at /home/greg/Work/qemu/qemu-spapr/cpus.c:1185
> #5 0x0000000010084248 in qemu_kvm_cpu_thread_fn (arg=0x11497fd0) at /home/greg/Work/qemu/qemu-spapr/cpus.c:1220
> #6 0x0000000010608cb0 in qemu_thread_start (args=0x10eb0510) at /home/greg/Work/qemu/qemu-spapr/util/qemu-thread-posix.c:507
> #7 0x00007ffff2758af4 in start_thread () from /lib64/libpthread.so.0
> #8 0x00007ffff2688814 in clone () from /lib64/libc.so.6
>
> [...]
>
> Thread 1 (Thread 0x7ffff0ee2650 (LWP 23234)):
> #0 0x00007ffff275e72c in pthread_cond_wait@@GLIBC_2.17 () from /lib64/libpthread.so.0
> #1 0x00000000106095d0 in qemu_cond_wait_impl (cond=0x10cf7028 <qemu_work_cond>, mutex=0x10cd4d60 <qemu_global_mutex>, file=0x107b2ea8 "/home/greg/Work/qemu/qemu-spapr/cpus-common.c", line=<optimized out>) at /home/greg/Work/qemu/qemu-spapr/util/qemu-thread-posix.c:164
> #2 0x00000000102c23d8 in do_run_on_cpu (cpu=<optimized out>, func=<optimized out>, data=..., mutex=0x10cd4d60 <qemu_global_mutex>) at /home/greg/Work/qemu/qemu-spapr/cpus-common.c:152
> #3 0x0000000010083cb0 in run_on_cpu (cpu=<optimized out>, func=<optimized out>, data=...) at /home/greg/Work/qemu/qemu-spapr/cpus.c:1126
> #4 0x00000000100b4bc4 in kvm_cpu_synchronize_post_init (cpu=<optimized out>) at /home/greg/Work/qemu/qemu-spapr/accel/kvm/kvm-all.c:1823
> #5 0x000000001047dbe8 in cpu_synchronize_post_init (cpu=0x11497fd0) at /home/greg/Work/qemu/qemu-spapr/include/sysemu/hw_accel.h:48
> #6 cpu_common_realizefn (dev=0x11497fd0, errp=<optimized out>) at /home/greg/Work/qemu/qemu-spapr/qom/cpu.c:347
> #7 0x00000000101aded4 in ppc_cpu_realize (dev=0x11497fd0, errp=0x7fffffffce50) at /home/greg/Work/qemu/qemu-spapr/target/ppc/translate_init.inc.c:9695
> #8 0x0000000010326410 in device_set_realized (obj=0x11497fd0, value=<optimized out>, errp=0x7fffffffd080) at /home/greg/Work/qemu/qemu-spapr/hw/core/qdev.c:826
> #9 0x00000000104c6ae0 in property_set_bool (obj=0x11497fd0, v=<optimized out>, name=<optimized out>, opaque=0x1182a120, errp=0x7fffffffd080) at /home/greg/Work/qemu/qemu-spapr/qom/object.c:1930
> #10 0x00000000104c9838 in object_property_set (obj=0x11497fd0, v=0x119a0e20, name=0x1074fd90 "realized", errp=0x7fffffffd080) at /home/greg/Work/qemu/qemu-spapr/qom/object.c:1122
> #11 0x00000000104ccd6c in object_property_set_qobject (obj=0x11497fd0, value=<optimized out>, name=<optimized out>, errp=<optimized out>) at /home/greg/Work/qemu/qemu-spapr/qom/qom-qobject.c:27
> #12 0x00000000104c9b00 in object_property_set_bool (obj=0x11497fd0, value=<optimized out>, name=<optimized out>, errp=<optimized out>) at /home/greg/Work/qemu/qemu-spapr/qom/object.c:1188
> #13 0x0000000010168500 in spapr_realize_vcpu (errp=0x7fffffffd078, spapr=<optimized out>, cpu=0x11497fd0) at /home/greg/Work/qemu/qemu-spapr/hw/ppc/spapr_cpu_core.c:143
>
> This happens when the core CPU realization code decides to copy the full
> set of registers to KVM, but the machine_data pointer is still NULL.
>
> So I think the fix belongs to this patch, see below.
>
> > hw/intc/xics.c | 5 +++--
> > hw/intc/xics_spapr.c | 16 +++++++++++-----
> > hw/ppc/pnv.c | 4 ++--
> > hw/ppc/pnv_core.c | 11 +++++++++--
> > hw/ppc/spapr.c | 8 ++++----
> > hw/ppc/spapr_cpu_core.c | 13 ++++++++++---
> > include/hw/ppc/pnv_core.h | 9 +++++++++
> > include/hw/ppc/spapr_cpu_core.h | 10 ++++++++++
> > include/hw/ppc/xics.h | 4 ++--
> > target/ppc/cpu.h | 2 +-
> > 10 files changed, 61 insertions(+), 21 deletions(-)
> >
> > diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> > index e73e623e3b..689ad44e5f 100644
> > --- a/hw/intc/xics.c
> > +++ b/hw/intc/xics.c
> > @@ -383,7 +383,8 @@ static const TypeInfo icp_info = {
> > .class_size = sizeof(ICPStateClass),
> > };
> >
> > -Object *icp_create(Object *cpu, const char *type, XICSFabric *xi, Error **errp)
> > +ICPState *icp_create(Object *cpu, const char *type, XICSFabric *xi,
> > + Error **errp)
> > {
> > Error *local_err = NULL;
> > Object *obj;
> > @@ -401,7 +402,7 @@ Object *icp_create(Object *cpu, const char *type, XICSFabric *xi, Error **errp)
> > obj = NULL;
> > }
> >
> > - return obj;
> > + return ICP(obj);
> > }
> >
> > /*
> > diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> > index 2e27b92b87..01c76717cf 100644
> > --- a/hw/intc/xics_spapr.c
> > +++ b/hw/intc/xics_spapr.c
> > @@ -31,6 +31,7 @@
> > #include "trace.h"
> > #include "qemu/timer.h"
> > #include "hw/ppc/spapr.h"
> > +#include "hw/ppc/spapr_cpu_core.h"
> > #include "hw/ppc/xics.h"
> > #include "hw/ppc/fdt.h"
> > #include "qapi/visitor.h"
> > @@ -43,8 +44,9 @@ static target_ulong h_cppr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> > target_ulong opcode, target_ulong *args)
> > {
> > target_ulong cppr = args[0];
> > + sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> >
> > - icp_set_cppr(ICP(cpu->intc), cppr);
> > + icp_set_cppr(spapr_cpu->icp, cppr);
> > return H_SUCCESS;
> > }
> >
> > @@ -65,7 +67,8 @@ static target_ulong h_ipi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> > static target_ulong h_xirr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> > target_ulong opcode, target_ulong *args)
> > {
> > - uint32_t xirr = icp_accept(ICP(cpu->intc));
> > + sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> > + uint32_t xirr = icp_accept(spapr_cpu->icp);
> >
> > args[0] = xirr;
> > return H_SUCCESS;
> > @@ -74,7 +77,8 @@ static target_ulong h_xirr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> > static target_ulong h_xirr_x(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> > target_ulong opcode, target_ulong *args)
> > {
> > - uint32_t xirr = icp_accept(ICP(cpu->intc));
> > + sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> > + uint32_t xirr = icp_accept(spapr_cpu->icp);
> >
> > args[0] = xirr;
> > args[1] = cpu_get_host_ticks();
> > @@ -84,9 +88,10 @@ static target_ulong h_xirr_x(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> > static target_ulong h_eoi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> > target_ulong opcode, target_ulong *args)
> > {
> > + sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> > target_ulong xirr = args[0];
> >
> > - icp_eoi(ICP(cpu->intc), xirr);
> > + icp_eoi(spapr_cpu->icp, xirr);
> > return H_SUCCESS;
> > }
> >
> > @@ -94,7 +99,8 @@ static target_ulong h_ipoll(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> > target_ulong opcode, target_ulong *args)
> > {
> > uint32_t mfrr;
> > - uint32_t xirr = icp_ipoll(ICP(cpu->intc), &mfrr);
> > + sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> > + uint32_t xirr = icp_ipoll(spapr_cpu->icp, &mfrr);
> >
> > args[0] = xirr;
> > args[1] = mfrr;
> > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> > index 0b9508d94d..3a36c6ac6a 100644
> > --- a/hw/ppc/pnv.c
> > +++ b/hw/ppc/pnv.c
> > @@ -1013,7 +1013,7 @@ static ICPState *pnv_icp_get(XICSFabric *xi, int pir)
> > {
> > PowerPCCPU *cpu = ppc_get_vcpu_by_pir(pir);
> >
> > - return cpu ? ICP(cpu->intc) : NULL;
> > + return cpu ? pnv_cpu_state(cpu)->icp : NULL;
> > }
> >
> > static void pnv_pic_print_info(InterruptStatsProvider *obj,
> > @@ -1026,7 +1026,7 @@ static void pnv_pic_print_info(InterruptStatsProvider *obj,
> > CPU_FOREACH(cs) {
> > PowerPCCPU *cpu = POWERPC_CPU(cs);
> >
> > - icp_pic_print_info(ICP(cpu->intc), mon);
> > + icp_pic_print_info(pnv_cpu_state(cpu)->icp, mon);
> > }
> >
> > for (i = 0; i < pnv->num_chips; i++) {
> > diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> > index f7cf33f547..746bc5c2c5 100644
> > --- a/hw/ppc/pnv_core.c
> > +++ b/hw/ppc/pnv_core.c
> > @@ -105,6 +105,7 @@ static void pnv_realize_vcpu(PowerPCCPU *cpu, XICSFabric *xi, Error **errp)
> > int core_pir;
> > int thread_index = 0; /* TODO: TCG supports only one thread */
> > ppc_spr_t *pir = &env->spr_cb[SPR_PIR];
> > + PnvCPUState *pnv_cpu;
> > Error *local_err = NULL;
> >
> > object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
> > @@ -113,7 +114,9 @@ static void pnv_realize_vcpu(PowerPCCPU *cpu, XICSFabric *xi, Error **errp)
> > return;
> > }
> >
> > - cpu->intc = icp_create(OBJECT(cpu), TYPE_PNV_ICP, xi, &local_err);
> > + cpu->machine_data = pnv_cpu = g_new0(PnvCPUState, 1);
> > +
> > + pnv_cpu->icp = icp_create(OBJECT(cpu), TYPE_PNV_ICP, xi, &local_err);
> > if (local_err) {
> > error_propagate(errp, local_err);
> > return;
> > @@ -188,8 +191,12 @@ err:
> >
> > static void pnv_unrealize_vcpu(PowerPCCPU *cpu)
> > {
> > + PnvCPUState *pnv_cpu = pnv_cpu_state(cpu);
> > +
> > qemu_unregister_reset(pnv_cpu_reset, cpu);
> > - object_unparent(cpu->intc);
> > + object_unparent(OBJECT(pnv_cpu->icp));
> > + cpu->machine_data = NULL;
> > + g_free(pnv_cpu);
> > cpu_remove_sync(CPU(cpu));
> > object_unparent(OBJECT(cpu));
> > }
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index f59999daac..cbab6b6b7e 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1745,8 +1745,8 @@ static int spapr_post_load(void *opaque, int version_id)
> > if (!object_dynamic_cast(OBJECT(spapr->ics), TYPE_ICS_KVM)) {
> > CPUState *cs;
> > CPU_FOREACH(cs) {
> > - PowerPCCPU *cpu = POWERPC_CPU(cs);
> > - icp_resend(ICP(cpu->intc));
> > + sPAPRCPUState *spapr_cpu = spapr_cpu_state(POWERPC_CPU(cs));
> > + icp_resend(spapr_cpu->icp);
> > }
> > }
> >
> > @@ -3783,7 +3783,7 @@ static ICPState *spapr_icp_get(XICSFabric *xi, int vcpu_id)
> > {
> > PowerPCCPU *cpu = spapr_find_cpu(vcpu_id);
> >
> > - return cpu ? ICP(cpu->intc) : NULL;
> > + return cpu ? spapr_cpu_state(cpu)->icp : NULL;
> > }
> >
> > #define ICS_IRQ_FREE(ics, srcno) \
> > @@ -3925,7 +3925,7 @@ static void spapr_pic_print_info(InterruptStatsProvider *obj,
> > CPU_FOREACH(cs) {
> > PowerPCCPU *cpu = POWERPC_CPU(cs);
> >
> > - icp_pic_print_info(ICP(cpu->intc), mon);
> > + icp_pic_print_info(spapr_cpu_state(cpu)->icp, mon);
> > }
> >
> > ics_pic_print_info(spapr->ics, mon);
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 7fdb3b6667..544bda93e2 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -104,8 +104,12 @@ const char *spapr_get_cpu_core_type(const char *cpu_type)
> >
> > static void spapr_unrealize_vcpu(PowerPCCPU *cpu)
> > {
> > + sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> > +
> > qemu_unregister_reset(spapr_cpu_reset, cpu);
> > - object_unparent(cpu->intc);
> > + object_unparent(OBJECT(spapr_cpu->icp));
> > + cpu->machine_data = NULL;
> > + g_free(spapr_cpu);
> > cpu_remove_sync(CPU(cpu));
> > object_unparent(OBJECT(cpu));
> > }
> > @@ -127,12 +131,15 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> > {
> > CPUPPCState *env = &cpu->env;
> > Error *local_err = NULL;
> > + sPAPRCPUState *spapr_cpu;
> >
> > object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
> > if (local_err) {
> > goto error;
> > }
> >
> > + spapr_cpu = cpu->machine_data = g_new0(sPAPRCPUState, 1);
> > +
>
> machine_data is a cpu attribute and all users of spapr_cpu_state() assume it
> is always valid. It should be set before any code tries to dereference it,
> which I believe cannot happen before the call to object_property_set_bool().
> So I guess setting it at the beginning of the function should be fine (at
> least, it fixes the crash).
Ah, good point. I've moved it up before setting realized, for both
spapr and pnv.
--
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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCHv3 6/7] target/ppc: Replace intc pointer with a general machine_data pointer
2018-06-14 15:20 ` Cédric Le Goater
@ 2018-06-15 1:34 ` David Gibson
0 siblings, 0 replies; 15+ messages in thread
From: David Gibson @ 2018-06-15 1:34 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: groug, qemu-devel, qemu-ppc
[-- Attachment #1: Type: text/plain, Size: 1942 bytes --]
On Thu, Jun 14, 2018 at 05:20:56PM +0200, Cédric Le Goater wrote:
> On 06/14/2018 06:41 AM, David Gibson wrote:
> > PowerPCCPU contains an (Object *)intc used to point to the cpu's interrupt
> > controller. Or more precisely to the "presentation" component of the
> > interrupt controller relevant to this cpu.
> >
> > Really, this field is machine specific. The machines which use it can
> > point it to different types of object depending on their needs, and most
> > machines don't use it at all (since they have older style PICs which don't
> > have per-cpu presentation components).
> >
> > There's also other information that's per-cpu, but platform/machine
> > specific. So replace the intc pointer with a (void *)machine_data which
> > can be managed as the machine type likes to conveniently store per cpu
> > information.
>
> I took a closer look. I tried to port the XIVE models on top this
> new machine data field and it does not work out at all.
>
> The problem is that the XIVE models are common to PowerNV and sPAPR,
> they are machine agnostic, and when the cpu->intc pointer is used in
> the TIMA ops, we have no idea in which machine the CPU are running.
> We only expect the CPU to be a POWER9. We need a CPU related pointer
> and not a machine data pointer, but we could imagine having a machine
> data pointer under the CPU data pointer but the first need is a CPU
> data.
>
> Sorry, not have said it ealier, but I will just add back the intc
> pointer under the CPU, for the moment.
Bother. Long term, I'm not sure what to do about this. Fow now, I'll
back out the patch which moved intc to the machine_data (and rework
the vpa -> machine_data patch to apply without it).
--
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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2018-06-15 2:11 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-14 4:41 [Qemu-devel] [PATCHv3 0/7] Better handling of machine specific per-cpu information David Gibson
2018-06-14 4:41 ` [Qemu-devel] [PATCHv3 1/7] spapr: Clean up cpu realize/unrealize paths David Gibson
2018-06-14 4:41 ` [Qemu-devel] [PATCHv3 2/7] pnv: Fix some error handling cpu realize() David Gibson
2018-06-14 5:26 ` Cédric Le Goater
2018-06-14 6:35 ` Greg Kurz
2018-06-14 4:41 ` [Qemu-devel] [PATCHv3 3/7] pnv_core: Allocate cpu thread objects individually David Gibson
2018-06-14 4:41 ` [Qemu-devel] [PATCHv3 4/7] pnv: Clean up cpu realize path David Gibson
2018-06-14 4:41 ` [Qemu-devel] [PATCHv3 5/7] pnv: Add cpu unrealize path David Gibson
2018-06-14 4:41 ` [Qemu-devel] [PATCHv3 6/7] target/ppc: Replace intc pointer with a general machine_data pointer David Gibson
2018-06-14 5:30 ` Cédric Le Goater
2018-06-14 13:34 ` Greg Kurz
2018-06-15 0:19 ` David Gibson
2018-06-14 15:20 ` Cédric Le Goater
2018-06-15 1:34 ` David Gibson
2018-06-14 4:41 ` [Qemu-devel] [PATCHv3 7/7] target/ppc, spapr: Move VPA information to machine_data David Gibson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.