Re: [PATCH v3 4/4] spapr/xive: Set the OS CAM line at reset

[Greg Kurz <groug@kaod.org>]

On Tue, 22 Oct 2019 09:22:46 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> When a Virtual Processor is scheduled to run on a HW thread, the
> hypervisor pushes its identifier in the OS CAM line. When running with
> kernel_irqchip=off, QEMU needs to emulate the same behavior.
> 
> Set the OS CAM line when the interrupt presenter of the sPAPR core is
> reseted. This will also cover the case of hot-plugged CPUs.
> 
> This change also has the benefit to remove the use of CPU_FOREACH()
> which can be unsafe.
> 

CPU_FOREACH() is indeed an unsafe way to loop on presenters. There
are some other users in XICS and XIVE that do cause QEMU to crash.
I shall send fixes as soon as this series reaches ppc-for-4.2.

> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  include/hw/ppc/spapr_xive.h |  1 -
>  hw/intc/spapr_xive.c        | 48 +++++++++++++------------------------
>  2 files changed, 17 insertions(+), 32 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> index d84bd5c229f0..742b7e834f2a 100644
> --- a/include/hw/ppc/spapr_xive.h
> +++ b/include/hw/ppc/spapr_xive.h
> @@ -57,7 +57,6 @@ typedef struct SpaprXive {
>  void spapr_xive_pic_print_info(SpaprXive *xive, Monitor *mon);
>  
>  void spapr_xive_hcall_init(SpaprMachineState *spapr);
> -void spapr_xive_set_tctx_os_cam(XiveTCTX *tctx);
>  void spapr_xive_mmio_set_enabled(SpaprXive *xive, bool enable);
>  void spapr_xive_map_mmio(SpaprXive *xive);
>  
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index 20a8d8285f64..d8e1291905c3 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -205,23 +205,6 @@ void spapr_xive_mmio_set_enabled(SpaprXive *xive, bool enable)
>      memory_region_set_enabled(&xive->end_source.esb_mmio, false);
>  }
>  
> -/*
> - * When a Virtual Processor is scheduled to run on a HW thread, the
> - * hypervisor pushes its identifier in the OS CAM line. Emulate the
> - * same behavior under QEMU.
> - */
> -void spapr_xive_set_tctx_os_cam(XiveTCTX *tctx)
> -{
> -    uint8_t  nvt_blk;
> -    uint32_t nvt_idx;
> -    uint32_t nvt_cam;
> -
> -    spapr_xive_cpu_to_nvt(POWERPC_CPU(tctx->cs), &nvt_blk, &nvt_idx);
> -
> -    nvt_cam = cpu_to_be32(TM_QW1W2_VO | xive_nvt_cam_line(nvt_blk, nvt_idx));
> -    memcpy(&tctx->regs[TM_QW1_OS + TM_WORD2], &nvt_cam, 4);
> -}
> -
>  static void spapr_xive_end_reset(XiveEND *end)
>  {
>      memset(end, 0, sizeof(*end));
> @@ -544,21 +527,32 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
>      }
>  
>      spapr_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(spapr_cpu->tctx);
>      return 0;
>  }
>  
> +static void xive_tctx_set_os_cam(XiveTCTX *tctx, uint32_t os_cam)
> +{
> +    uint32_t qw1w2 = cpu_to_be32(TM_QW1W2_VO | os_cam);
> +    memcpy(&tctx->regs[TM_QW1_OS + TM_WORD2], &qw1w2, 4);
> +}
> +
>  static void spapr_xive_cpu_intc_reset(SpaprInterruptController *intc,
>                                       PowerPCCPU *cpu)
>  {
>      XiveTCTX *tctx = spapr_cpu_state(cpu)->tctx;
> +    uint8_t  nvt_blk;
> +    uint32_t nvt_idx;
>  
>      xive_tctx_reset(tctx);
> +
> +    /*
> +     * When a Virtual Processor is scheduled to run on a HW thread,
> +     * the hypervisor pushes its identifier in the OS CAM line.
> +     * Emulate the same behavior under QEMU.
> +     */
> +    spapr_xive_cpu_to_nvt(cpu, &nvt_blk, &nvt_idx);
> +
> +    xive_tctx_set_os_cam(tctx, xive_nvt_cam_line(nvt_blk, nvt_idx));
>  }
>  
>  static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
> @@ -651,14 +645,6 @@ static void spapr_xive_dt(SpaprInterruptController *intc, uint32_t nr_servers,
>  static int spapr_xive_activate(SpaprInterruptController *intc, Error **errp)
>  {
>      SpaprXive *xive = SPAPR_XIVE(intc);
> -    CPUState *cs;
> -
> -    CPU_FOREACH(cs) {
> -        PowerPCCPU *cpu = POWERPC_CPU(cs);
> -
> -        /* (TCG) Set the OS CAM line of the thread interrupt context. */
> -        spapr_xive_set_tctx_os_cam(spapr_cpu_state(cpu)->tctx);
> -    }
>  
>      if (kvm_enabled()) {
>          int rc = spapr_irq_init_kvm(kvmppc_xive_connect, intc, errp);