| Message ID | 157184234176.3053790.8577967462603127139.stgit@bahia.lan |
|---|---|
| State | New |
| Headers | show |
| Series | ppc: Reparent the interrupt presenter | expand |
On Wed, Oct 23, 2019 at 04:52:21PM +0200, Greg Kurz wrote: > Now that presenter objects are parented to the interrupt controller, stop > relying on CPU_FOREACH() which can race with CPU hotplug and crash QEMU. > > Signed-off-by: Greg Kurz <groug@kaod.org> So.. we might be able to go further than this. Having the TYPE_INTERRUPT_STATS_PROVIDER interrupt on the machine is actually an spapr and pnv oddity. In most cases that interface is on the various components of the interrupt controller directly. hmp_info_irq() scans the whole QOM tree looking for everything with the interface to produce the info pic output. It would be nice if we can do the same for xics and xive. The tricky bit is that we do have the possibility of both, in which case the individual components would need to know if they're currently "active" and minimize their output if so. > --- > hw/intc/spapr_xive.c | 8 +------- > hw/intc/xics.c | 12 ++++++++++++ > hw/intc/xics_spapr.c | 8 +------- > hw/intc/xive.c | 12 ++++++++++++ > include/hw/ppc/xics.h | 1 + > include/hw/ppc/xive.h | 2 ++ > 6 files changed, 29 insertions(+), 14 deletions(-) > > diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c > index d74ee71e76b4..05763a58cf5d 100644 > --- a/hw/intc/spapr_xive.c > +++ b/hw/intc/spapr_xive.c > @@ -579,14 +579,8 @@ static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val) > static void spapr_xive_print_info(SpaprInterruptController *intc, Monitor *mon) > { > SpaprXive *xive = SPAPR_XIVE(intc); > - CPUState *cs; > - > - CPU_FOREACH(cs) { > - PowerPCCPU *cpu = POWERPC_CPU(cs); > - > - xive_tctx_pic_print_info(spapr_cpu_state(cpu)->tctx, mon); > - } > > + xive_presenter_print_info(XIVE_ROUTER(intc), mon); > spapr_xive_pic_print_info(xive, mon); > } > > diff --git a/hw/intc/xics.c b/hw/intc/xics.c > index d5e4db668a4b..6e820c4851f3 100644 > --- a/hw/intc/xics.c > +++ b/hw/intc/xics.c > @@ -88,6 +88,18 @@ void ics_pic_print_info(ICSState *ics, Monitor *mon) > } > } > > +static int do_ics_pic_print_icp_infos(Object *child, void *opaque) > +{ > + icp_pic_print_info(ICP(child), opaque); > + return 0; > +} > + > +void ics_pic_print_icp_infos(ICSState *ics, const char *type, Monitor *mon) > +{ > + object_child_foreach_type(OBJECT(ics), type, do_ics_pic_print_icp_infos, > + mon); > +} > + > /* > * ICP: Presentation layer > */ > diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c > index 080ed73aad64..7624d693c8da 100644 > --- a/hw/intc/xics_spapr.c > +++ b/hw/intc/xics_spapr.c > @@ -400,14 +400,8 @@ static void xics_spapr_set_irq(SpaprInterruptController *intc, int irq, int val) > static void xics_spapr_print_info(SpaprInterruptController *intc, Monitor *mon) > { > ICSState *ics = ICS_SPAPR(intc); > - CPUState *cs; > - > - CPU_FOREACH(cs) { > - PowerPCCPU *cpu = POWERPC_CPU(cs); > - > - icp_pic_print_info(spapr_cpu_state(cpu)->icp, mon); > - } > > + ics_pic_print_icp_infos(ics, TYPE_ICP, mon); > ics_pic_print_info(ics, mon); > } > > diff --git a/hw/intc/xive.c b/hw/intc/xive.c > index 8d2da4a11163..40ce43152456 100644 > --- a/hw/intc/xive.c > +++ b/hw/intc/xive.c > @@ -547,6 +547,18 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon) > } > } > > +static int do_xive_presenter_print_info(Object *child, void *opaque) > +{ > + xive_tctx_pic_print_info(XIVE_TCTX(child), opaque); > + return 0; > +} > + > +void xive_presenter_print_info(XiveRouter *xrtr, Monitor *mon) > +{ > + object_child_foreach_type(OBJECT(xrtr), TYPE_XIVE_TCTX, > + do_xive_presenter_print_info, mon); > +} > + > void xive_tctx_reset(XiveTCTX *tctx) > { > memset(tctx->regs, 0, sizeof(tctx->regs)); > diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h > index f4827e748fd8..4de1f421c997 100644 > --- a/include/hw/ppc/xics.h > +++ b/include/hw/ppc/xics.h > @@ -175,6 +175,7 @@ static inline bool ics_irq_free(ICSState *ics, uint32_t srcno) > void ics_set_irq_type(ICSState *ics, int srcno, bool lsi); > void icp_pic_print_info(ICPState *icp, Monitor *mon); > void ics_pic_print_info(ICSState *ics, Monitor *mon); > +void ics_pic_print_icp_infos(ICSState *ics, const char *type, Monitor *mon); > > void ics_resend(ICSState *ics); > void icp_resend(ICPState *ss); > diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h > index 8fd439ec9bba..14690428a0aa 100644 > --- a/include/hw/ppc/xive.h > +++ b/include/hw/ppc/xive.h > @@ -367,6 +367,8 @@ int xive_router_write_nvt(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t nvt_idx, > XiveTCTX *xive_router_get_tctx(XiveRouter *xrtr, CPUState *cs); > void xive_router_notify(XiveNotifier *xn, uint32_t lisn); > > +void xive_presenter_print_info(XiveRouter *xrtr, Monitor *mon); > + > /* > * XIVE END ESBs > */ >
On Thu, 24 Oct 2019 14:02:31 +1100 David Gibson <david@gibson.dropbear.id.au> wrote: > On Wed, Oct 23, 2019 at 04:52:21PM +0200, Greg Kurz wrote: > > Now that presenter objects are parented to the interrupt controller, stop > > relying on CPU_FOREACH() which can race with CPU hotplug and crash QEMU. > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > So.. we might be able to go further than this. Having the > TYPE_INTERRUPT_STATS_PROVIDER interrupt on the machine is actually an > spapr and pnv oddity. In most cases that interface is on the various > components of the interrupt controller directly. hmp_info_irq() scans > the whole QOM tree looking for everything with the interface to > produce the info pic output. > > It would be nice if we can do the same for xics and xive. The tricky > bit is that we do have the possibility of both, in which case the > individual components would need to know if they're currently "active" > and minimize their output if so. > Yes but this looks like 4.3 material. If we want to fix this for 4.2, I'm now thinking it might be safer to keep CPU_FOREACH() and check the state. > > --- > > hw/intc/spapr_xive.c | 8 +------- > > hw/intc/xics.c | 12 ++++++++++++ > > hw/intc/xics_spapr.c | 8 +------- > > hw/intc/xive.c | 12 ++++++++++++ > > include/hw/ppc/xics.h | 1 + > > include/hw/ppc/xive.h | 2 ++ > > 6 files changed, 29 insertions(+), 14 deletions(-) > > > > diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c > > index d74ee71e76b4..05763a58cf5d 100644 > > --- a/hw/intc/spapr_xive.c > > +++ b/hw/intc/spapr_xive.c > > @@ -579,14 +579,8 @@ static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val) > > static void spapr_xive_print_info(SpaprInterruptController *intc, Monitor *mon) > > { > > SpaprXive *xive = SPAPR_XIVE(intc); > > - CPUState *cs; > > - > > - CPU_FOREACH(cs) { > > - PowerPCCPU *cpu = POWERPC_CPU(cs); > > - > > - xive_tctx_pic_print_info(spapr_cpu_state(cpu)->tctx, mon); > > - } > > > > + xive_presenter_print_info(XIVE_ROUTER(intc), mon); > > spapr_xive_pic_print_info(xive, mon); > > } > > > > diff --git a/hw/intc/xics.c b/hw/intc/xics.c > > index d5e4db668a4b..6e820c4851f3 100644 > > --- a/hw/intc/xics.c > > +++ b/hw/intc/xics.c > > @@ -88,6 +88,18 @@ void ics_pic_print_info(ICSState *ics, Monitor *mon) > > } > > } > > > > +static int do_ics_pic_print_icp_infos(Object *child, void *opaque) > > +{ > > + icp_pic_print_info(ICP(child), opaque); > > + return 0; > > +} > > + > > +void ics_pic_print_icp_infos(ICSState *ics, const char *type, Monitor *mon) > > +{ > > + object_child_foreach_type(OBJECT(ics), type, do_ics_pic_print_icp_infos, > > + mon); > > +} > > + > > /* > > * ICP: Presentation layer > > */ > > diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c > > index 080ed73aad64..7624d693c8da 100644 > > --- a/hw/intc/xics_spapr.c > > +++ b/hw/intc/xics_spapr.c > > @@ -400,14 +400,8 @@ static void xics_spapr_set_irq(SpaprInterruptController *intc, int irq, int val) > > static void xics_spapr_print_info(SpaprInterruptController *intc, Monitor *mon) > > { > > ICSState *ics = ICS_SPAPR(intc); > > - CPUState *cs; > > - > > - CPU_FOREACH(cs) { > > - PowerPCCPU *cpu = POWERPC_CPU(cs); > > - > > - icp_pic_print_info(spapr_cpu_state(cpu)->icp, mon); > > - } > > > > + ics_pic_print_icp_infos(ics, TYPE_ICP, mon); > > ics_pic_print_info(ics, mon); > > } > > > > diff --git a/hw/intc/xive.c b/hw/intc/xive.c > > index 8d2da4a11163..40ce43152456 100644 > > --- a/hw/intc/xive.c > > +++ b/hw/intc/xive.c > > @@ -547,6 +547,18 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon) > > } > > } > > > > +static int do_xive_presenter_print_info(Object *child, void *opaque) > > +{ > > + xive_tctx_pic_print_info(XIVE_TCTX(child), opaque); > > + return 0; > > +} > > + > > +void xive_presenter_print_info(XiveRouter *xrtr, Monitor *mon) > > +{ > > + object_child_foreach_type(OBJECT(xrtr), TYPE_XIVE_TCTX, > > + do_xive_presenter_print_info, mon); > > +} > > + > > void xive_tctx_reset(XiveTCTX *tctx) > > { > > memset(tctx->regs, 0, sizeof(tctx->regs)); > > diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h > > index f4827e748fd8..4de1f421c997 100644 > > --- a/include/hw/ppc/xics.h > > +++ b/include/hw/ppc/xics.h > > @@ -175,6 +175,7 @@ static inline bool ics_irq_free(ICSState *ics, uint32_t srcno) > > void ics_set_irq_type(ICSState *ics, int srcno, bool lsi); > > void icp_pic_print_info(ICPState *icp, Monitor *mon); > > void ics_pic_print_info(ICSState *ics, Monitor *mon); > > +void ics_pic_print_icp_infos(ICSState *ics, const char *type, Monitor *mon); > > > > void ics_resend(ICSState *ics); > > void icp_resend(ICPState *ss); > > diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h > > index 8fd439ec9bba..14690428a0aa 100644 > > --- a/include/hw/ppc/xive.h > > +++ b/include/hw/ppc/xive.h > > @@ -367,6 +367,8 @@ int xive_router_write_nvt(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t nvt_idx, > > XiveTCTX *xive_router_get_tctx(XiveRouter *xrtr, CPUState *cs); > > void xive_router_notify(XiveNotifier *xn, uint32_t lisn); > > > > +void xive_presenter_print_info(XiveRouter *xrtr, Monitor *mon); > > + > > /* > > * XIVE END ESBs > > */ > > >
On Thu, Oct 24, 2019 at 11:28:45AM +0200, Greg Kurz wrote: > On Thu, 24 Oct 2019 14:02:31 +1100 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Wed, Oct 23, 2019 at 04:52:21PM +0200, Greg Kurz wrote: > > > Now that presenter objects are parented to the interrupt controller, stop > > > relying on CPU_FOREACH() which can race with CPU hotplug and crash QEMU. > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > > So.. we might be able to go further than this. Having the > > TYPE_INTERRUPT_STATS_PROVIDER interrupt on the machine is actually an > > spapr and pnv oddity. In most cases that interface is on the various > > components of the interrupt controller directly. hmp_info_irq() scans > > the whole QOM tree looking for everything with the interface to > > produce the info pic output. > > > > It would be nice if we can do the same for xics and xive. The tricky > > bit is that we do have the possibility of both, in which case the > > individual components would need to know if they're currently "active" > > and minimize their output if so. > > > > Yes but this looks like 4.3 material. If we want to fix this for 4.2, > I'm now thinking it might be safer to keep CPU_FOREACH() and check the > state. Yeah, for 4.2 that sounds like a good idea. > > > > --- > > > hw/intc/spapr_xive.c | 8 +------- > > > hw/intc/xics.c | 12 ++++++++++++ > > > hw/intc/xics_spapr.c | 8 +------- > > > hw/intc/xive.c | 12 ++++++++++++ > > > include/hw/ppc/xics.h | 1 + > > > include/hw/ppc/xive.h | 2 ++ > > > 6 files changed, 29 insertions(+), 14 deletions(-) > > > > > > diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c > > > index d74ee71e76b4..05763a58cf5d 100644 > > > --- a/hw/intc/spapr_xive.c > > > +++ b/hw/intc/spapr_xive.c > > > @@ -579,14 +579,8 @@ static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val) > > > static void spapr_xive_print_info(SpaprInterruptController *intc, Monitor *mon) > > > { > > > SpaprXive *xive = SPAPR_XIVE(intc); > > > - CPUState *cs; > > > - > > > - CPU_FOREACH(cs) { > > > - PowerPCCPU *cpu = POWERPC_CPU(cs); > > > - > > > - xive_tctx_pic_print_info(spapr_cpu_state(cpu)->tctx, mon); > > > - } > > > > > > + xive_presenter_print_info(XIVE_ROUTER(intc), mon); > > > spapr_xive_pic_print_info(xive, mon); > > > } > > > > > > diff --git a/hw/intc/xics.c b/hw/intc/xics.c > > > index d5e4db668a4b..6e820c4851f3 100644 > > > --- a/hw/intc/xics.c > > > +++ b/hw/intc/xics.c > > > @@ -88,6 +88,18 @@ void ics_pic_print_info(ICSState *ics, Monitor *mon) > > > } > > > } > > > > > > +static int do_ics_pic_print_icp_infos(Object *child, void *opaque) > > > +{ > > > + icp_pic_print_info(ICP(child), opaque); > > > + return 0; > > > +} > > > + > > > +void ics_pic_print_icp_infos(ICSState *ics, const char *type, Monitor *mon) > > > +{ > > > + object_child_foreach_type(OBJECT(ics), type, do_ics_pic_print_icp_infos, > > > + mon); > > > +} > > > + > > > /* > > > * ICP: Presentation layer > > > */ > > > diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c > > > index 080ed73aad64..7624d693c8da 100644 > > > --- a/hw/intc/xics_spapr.c > > > +++ b/hw/intc/xics_spapr.c > > > @@ -400,14 +400,8 @@ static void xics_spapr_set_irq(SpaprInterruptController *intc, int irq, int val) > > > static void xics_spapr_print_info(SpaprInterruptController *intc, Monitor *mon) > > > { > > > ICSState *ics = ICS_SPAPR(intc); > > > - CPUState *cs; > > > - > > > - CPU_FOREACH(cs) { > > > - PowerPCCPU *cpu = POWERPC_CPU(cs); > > > - > > > - icp_pic_print_info(spapr_cpu_state(cpu)->icp, mon); > > > - } > > > > > > + ics_pic_print_icp_infos(ics, TYPE_ICP, mon); > > > ics_pic_print_info(ics, mon); > > > } > > > > > > diff --git a/hw/intc/xive.c b/hw/intc/xive.c > > > index 8d2da4a11163..40ce43152456 100644 > > > --- a/hw/intc/xive.c > > > +++ b/hw/intc/xive.c > > > @@ -547,6 +547,18 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon) > > > } > > > } > > > > > > +static int do_xive_presenter_print_info(Object *child, void *opaque) > > > +{ > > > + xive_tctx_pic_print_info(XIVE_TCTX(child), opaque); > > > + return 0; > > > +} > > > + > > > +void xive_presenter_print_info(XiveRouter *xrtr, Monitor *mon) > > > +{ > > > + object_child_foreach_type(OBJECT(xrtr), TYPE_XIVE_TCTX, > > > + do_xive_presenter_print_info, mon); > > > +} > > > + > > > void xive_tctx_reset(XiveTCTX *tctx) > > > { > > > memset(tctx->regs, 0, sizeof(tctx->regs)); > > > diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h > > > index f4827e748fd8..4de1f421c997 100644 > > > --- a/include/hw/ppc/xics.h > > > +++ b/include/hw/ppc/xics.h > > > @@ -175,6 +175,7 @@ static inline bool ics_irq_free(ICSState *ics, uint32_t srcno) > > > void ics_set_irq_type(ICSState *ics, int srcno, bool lsi); > > > void icp_pic_print_info(ICPState *icp, Monitor *mon); > > > void ics_pic_print_info(ICSState *ics, Monitor *mon); > > > +void ics_pic_print_icp_infos(ICSState *ics, const char *type, Monitor *mon); > > > > > > void ics_resend(ICSState *ics); > > > void icp_resend(ICPState *ss); > > > diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h > > > index 8fd439ec9bba..14690428a0aa 100644 > > > --- a/include/hw/ppc/xive.h > > > +++ b/include/hw/ppc/xive.h > > > @@ -367,6 +367,8 @@ int xive_router_write_nvt(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t nvt_idx, > > > XiveTCTX *xive_router_get_tctx(XiveRouter *xrtr, CPUState *cs); > > > void xive_router_notify(XiveNotifier *xn, uint32_t lisn); > > > > > > +void xive_presenter_print_info(XiveRouter *xrtr, Monitor *mon); > > > + > > > /* > > > * XIVE END ESBs > > > */ > > > > > >
diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c index d74ee71e76b4..05763a58cf5d 100644 --- a/hw/intc/spapr_xive.c +++ b/hw/intc/spapr_xive.c @@ -579,14 +579,8 @@ static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val) static void spapr_xive_print_info(SpaprInterruptController *intc, Monitor *mon) { SpaprXive *xive = SPAPR_XIVE(intc); - CPUState *cs; - - CPU_FOREACH(cs) { - PowerPCCPU *cpu = POWERPC_CPU(cs); - - xive_tctx_pic_print_info(spapr_cpu_state(cpu)->tctx, mon); - } + xive_presenter_print_info(XIVE_ROUTER(intc), mon); spapr_xive_pic_print_info(xive, mon); } diff --git a/hw/intc/xics.c b/hw/intc/xics.c index d5e4db668a4b..6e820c4851f3 100644 --- a/hw/intc/xics.c +++ b/hw/intc/xics.c @@ -88,6 +88,18 @@ void ics_pic_print_info(ICSState *ics, Monitor *mon) } } +static int do_ics_pic_print_icp_infos(Object *child, void *opaque) +{ + icp_pic_print_info(ICP(child), opaque); + return 0; +} + +void ics_pic_print_icp_infos(ICSState *ics, const char *type, Monitor *mon) +{ + object_child_foreach_type(OBJECT(ics), type, do_ics_pic_print_icp_infos, + mon); +} + /* * ICP: Presentation layer */ diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c index 080ed73aad64..7624d693c8da 100644 --- a/hw/intc/xics_spapr.c +++ b/hw/intc/xics_spapr.c @@ -400,14 +400,8 @@ static void xics_spapr_set_irq(SpaprInterruptController *intc, int irq, int val) static void xics_spapr_print_info(SpaprInterruptController *intc, Monitor *mon) { ICSState *ics = ICS_SPAPR(intc); - CPUState *cs; - - CPU_FOREACH(cs) { - PowerPCCPU *cpu = POWERPC_CPU(cs); - - icp_pic_print_info(spapr_cpu_state(cpu)->icp, mon); - } + ics_pic_print_icp_infos(ics, TYPE_ICP, mon); ics_pic_print_info(ics, mon); } diff --git a/hw/intc/xive.c b/hw/intc/xive.c index 8d2da4a11163..40ce43152456 100644 --- a/hw/intc/xive.c +++ b/hw/intc/xive.c @@ -547,6 +547,18 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon) } } +static int do_xive_presenter_print_info(Object *child, void *opaque) +{ + xive_tctx_pic_print_info(XIVE_TCTX(child), opaque); + return 0; +} + +void xive_presenter_print_info(XiveRouter *xrtr, Monitor *mon) +{ + object_child_foreach_type(OBJECT(xrtr), TYPE_XIVE_TCTX, + do_xive_presenter_print_info, mon); +} + void xive_tctx_reset(XiveTCTX *tctx) { memset(tctx->regs, 0, sizeof(tctx->regs)); diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h index f4827e748fd8..4de1f421c997 100644 --- a/include/hw/ppc/xics.h +++ b/include/hw/ppc/xics.h @@ -175,6 +175,7 @@ static inline bool ics_irq_free(ICSState *ics, uint32_t srcno) void ics_set_irq_type(ICSState *ics, int srcno, bool lsi); void icp_pic_print_info(ICPState *icp, Monitor *mon); void ics_pic_print_info(ICSState *ics, Monitor *mon); +void ics_pic_print_icp_infos(ICSState *ics, const char *type, Monitor *mon); void ics_resend(ICSState *ics); void icp_resend(ICPState *ss); diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h index 8fd439ec9bba..14690428a0aa 100644 --- a/include/hw/ppc/xive.h +++ b/include/hw/ppc/xive.h @@ -367,6 +367,8 @@ int xive_router_write_nvt(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t nvt_idx, XiveTCTX *xive_router_get_tctx(XiveRouter *xrtr, CPUState *cs); void xive_router_notify(XiveNotifier *xn, uint32_t lisn); +void xive_presenter_print_info(XiveRouter *xrtr, Monitor *mon); + /* * XIVE END ESBs */
Now that presenter objects are parented to the interrupt controller, stop relying on CPU_FOREACH() which can race with CPU hotplug and crash QEMU. Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/intc/spapr_xive.c | 8 +------- hw/intc/xics.c | 12 ++++++++++++ hw/intc/xics_spapr.c | 8 +------- hw/intc/xive.c | 12 ++++++++++++ include/hw/ppc/xics.h | 1 + include/hw/ppc/xive.h | 2 ++ 6 files changed, 29 insertions(+), 14 deletions(-)