On 1/3/19 4:57 AM, David Gibson wrote:
> On Wed, Jan 02, 2019 at 06:57:35AM +0100, Cédric Le Goater wrote:
>> which will be used by the machine only when the XIVE interrupt mode is
>> in use.
>
> I don't love the idea of putting a hook this specific into the
> PowerPCCPU structure, though it might be the easiest path in the short
> term.
>
> A couple of approaches: 1) revisit my changes to allow for a pointer
> to machine-defined per-cpu data.
ok but we still need two different presenters objects, one for each
mode.
> or 2) do we actually need a cpu to tctx pointer.
>
> Expanding on (2) - here you use the pointer to find the right TIMA
> state to access,
yes.
> but that could also be handled by having different TIMA IO instances
> and mapping those individually to cpu_as.
It might work for QEMU but I am not sure how to implement the KVM
backend with such a design. We only have a single ram ptr mapping
for the machine in the KVM case.
There are a couple of places where we need to loop on the XiveTCTX
(presenter, KVM) and we use the QEMU CPU list and the cpu->tctx for
that. One of the reasons we introduced the cpu->intc pointer some
time ago was to get rid of the ICP array.
I don't think we want to introduce a XiveTCTX array again.
> On the interrupt delivery side I think a tctx to cpu link will suffice.
yes. that we already have. It is mostly used by KVM in fact.
> For sPAPR there might be complications with translating cpu numbers in
> hcalls to the right tctx.
The XiveTCTX is not used by the hcalls. We are fine on that side.
The double pointer solution is what I prefer today, but if you are
uncomfortable with it, we can come back to the previous where I was
allocating a XiveTCTX child under the CPU and switching the presenter
objects at reset. The only issue remaining was the child unparenting
in the unrealize() method.
Thanks,
C.
>>
>> Signed-off-by: Cédric Le Goater <c...@kaod.org>
>> ---
>> target/ppc/cpu.h | 2 ++
>> hw/intc/xive.c | 6 +++---
>> hw/ppc/spapr_cpu_core.c | 7 ++++++-
>> hw/ppc/spapr_irq.c | 8 ++++----
>> 4 files changed, 15 insertions(+), 8 deletions(-)
>>
>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>> index d5f99f1fc7b9..c76036985623 100644
>> --- a/target/ppc/cpu.h
>> +++ b/target/ppc/cpu.h
>> @@ -1177,6 +1177,7 @@ do { \
>>
>> typedef struct PPCVirtualHypervisor PPCVirtualHypervisor;
>> typedef struct PPCVirtualHypervisorClass PPCVirtualHypervisorClass;
>> +typedef struct XiveTCTX XiveTCTX;
>>
>> /**
>> * PowerPCCPU:
>> @@ -1196,6 +1197,7 @@ struct PowerPCCPU {
>> uint32_t compat_pvr;
>> PPCVirtualHypervisor *vhyp;
>> Object *intc;
>> + XiveTCTX *tctx;
>> void *machine_data;
>> int32_t node_id; /* NUMA node this CPU belongs to */
>> PPCHash64Options *hash64_opts;
>> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
>> index ea33494338dc..410c53278a11 100644
>> --- a/hw/intc/xive.c
>> +++ b/hw/intc/xive.c
>> @@ -321,7 +321,7 @@ static void xive_tm_write(void *opaque, hwaddr offset,
>> uint64_t value, unsigned size)
>> {
>> PowerPCCPU *cpu = POWERPC_CPU(current_cpu);
>> - XiveTCTX *tctx = XIVE_TCTX(cpu->intc);
>> + XiveTCTX *tctx = cpu->tctx;
>> const XiveTmOp *xto;
>>
>> /*
>> @@ -360,7 +360,7 @@ static void xive_tm_write(void *opaque, hwaddr offset,
>> static uint64_t xive_tm_read(void *opaque, hwaddr offset, unsigned size)
>> {
>> PowerPCCPU *cpu = POWERPC_CPU(current_cpu);
>> - XiveTCTX *tctx = XIVE_TCTX(cpu->intc);
>> + XiveTCTX *tctx = cpu->tctx;
>> const XiveTmOp *xto;
>>
>> /*
>> @@ -1186,7 +1186,7 @@ static bool xive_presenter_match(XiveRouter *xrtr,
>> uint8_t format,
>>
>> CPU_FOREACH(cs) {
>> PowerPCCPU *cpu = POWERPC_CPU(cs);
>> - XiveTCTX *tctx = XIVE_TCTX(cpu->intc);
>> + XiveTCTX *tctx = cpu->tctx;
>> int ring;
>>
>> /*
>> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
>> index 2739b2a4b818..1473ef853336 100644
>> --- a/hw/ppc/spapr_cpu_core.c
>> +++ b/hw/ppc/spapr_cpu_core.c
>> @@ -194,7 +194,12 @@ static void spapr_unrealize_vcpu(PowerPCCPU *cpu,
>> sPAPRCPUCore *sc)
>> vmstate_unregister(NULL, &vmstate_spapr_cpu_state,
>> cpu->machine_data);
>> }
>> qemu_unregister_reset(spapr_cpu_reset, cpu);
>> - object_unparent(cpu->intc);
>> + if (cpu->intc) {
>> + object_unparent(cpu->intc);
>> + }
>> + if (cpu->tctx) {
>> + object_unparent(OBJECT(cpu->tctx));
>> + }
>> cpu_remove_sync(CPU(cpu));
>> object_unparent(OBJECT(cpu));
>> }
>> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
>> index 2d2e17b66533..8d028db44ff4 100644
>> --- a/hw/ppc/spapr_irq.c
>> +++ b/hw/ppc/spapr_irq.c
>> @@ -305,7 +305,7 @@ static void spapr_irq_print_info_xive(sPAPRMachineState
>> *spapr,
>> CPU_FOREACH(cs) {
>> PowerPCCPU *cpu = POWERPC_CPU(cs);
>>
>> - xive_tctx_pic_print_info(XIVE_TCTX(cpu->intc), mon);
>> + xive_tctx_pic_print_info(cpu->tctx, mon);
>> }
>>
>> spapr_xive_pic_print_info(spapr->xive, mon);
>> @@ -323,13 +323,13 @@ static void
>> spapr_irq_cpu_intc_create_xive(sPAPRMachineState *spapr,
>> return;
>> }
>>
>> - cpu->intc = obj;
>> + cpu->tctx = XIVE_TCTX(obj);
>>
>> /*
>> * (TCG) Early setting the OS CAM line for hotplugged CPUs as they
>> * don't beneficiate from the reset of the XIVE IRQ backend
>> */
>> - spapr_xive_set_tctx_os_cam(XIVE_TCTX(obj));
>> + spapr_xive_set_tctx_os_cam(cpu->tctx);
>> }
>>
>> static int spapr_irq_post_load_xive(sPAPRMachineState *spapr, int
>> version_id)
>> @@ -345,7 +345,7 @@ static void spapr_irq_reset_xive(sPAPRMachineState
>> *spapr, Error **errp)
>> PowerPCCPU *cpu = POWERPC_CPU(cs);
>>
>> /* (TCG) Set the OS CAM line of the thread interrupt context. */
>> - spapr_xive_set_tctx_os_cam(XIVE_TCTX(cpu->intc));
>> + spapr_xive_set_tctx_os_cam(cpu->tctx);
>> }
>> }
>>
>
