| Message ID | 20190102055743.5052-3-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
| Series | spapr: introduce the 'dual' interrupt mode XICS/XIVE | expand |
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. 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, but that could also be handled by having different TIMA IO instances and mapping those individually to cpu_as. On the interrupt delivery side I think a tctx to cpu link will suffice. For sPAPR there might be complications with translating cpu numbers in hcalls to the right tctx. > > Signed-off-by: Cédric Le Goater <clg@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); > } > } >
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 <clg@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); >> } >> } >> >
On Thu, Jan 03, 2019 at 06:44:30PM +0100, Cédric Le Goater wrote: > 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. Hm, yes, I see your point. For now I'm applying patches 1-3, and hope to clean it up later with a revised version of my machine specific per-cpu patch when I get the chance.
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); } }
which will be used by the machine only when the XIVE interrupt mode is in use. Signed-off-by: Cédric Le Goater <clg@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(-)