Re: [PATCH 5/6] spapr: Don't use CPU_FOREACH() in 'info pic'

[David Gibson]

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 <address@hidden>

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
>   */
> 

-- 
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

signature.asc
Description: PGP signature