[PATCH v2 0/2] spapr: interrupt presenter fixes

[PATCH v2 0/2] spapr: interrupt presenter fixes

[Cédric Le Goater]

Hello,

The interrupt presenters are created by a machine handler at the core
level and are reseted independently. This is not consistent and it
raises some issues when it comes to handle hot-plugged CPUs. These are
reseted from the realize handler of the core and the presenters are
not. This is less of an issue in XICS, although a zero MFFR could be a
concern, but in XIVE, the OS CAM line is not set and this breaks the
presenting algorithm. The current code has workarounds which need a
global cleanup.

Extend the sPAPR IRQ backend with a new cpu_intc_reset() handler which
will be called by the CPU reset handler and remove the XiveTCTX reset
handler which is redundant.

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.


These changes do not address the root problem: the reset of the core
at realize time which hides the fact that hot-plugged CPUs are not
reseted. Anyhow, they do fix the presenter reset and remove a
workaround of workaround in the case of XIVE.

Thanks,

C.

Changes inv v2:

 - removed property
 - simplified reset handlers

Cédric Le Goater (2):
  spapr: Introduce a interrupt presenter reset handler
  spapr/xive: Set the OS CAM line at reset

 include/hw/ppc/spapr_irq.h  |  2 ++
 include/hw/ppc/spapr_xive.h |  1 -
 include/hw/ppc/xics.h       |  1 +
 include/hw/ppc/xive.h       |  1 +
 hw/intc/spapr_xive.c        | 25 ++++++++++---------------
 hw/intc/xics.c              |  8 ++------
 hw/intc/xics_spapr.c        |  7 +++++++
 hw/intc/xive.c              | 12 +-----------
 hw/ppc/spapr_cpu_core.c     | 14 ++++++++++----
 hw/ppc/spapr_irq.c          | 14 ++++++++++++++
 10 files changed, 48 insertions(+), 37 deletions(-)

-- 
2.21.0

[PATCH v2 1/2] spapr: Introduce a interrupt presenter reset handler

[Cédric Le Goater]

The interrupt presenters are created by a machine handler at the core
level and are reseted independently. This is not consistent and it
raises some issues when it comes to handle hot-plugged CPUs. These are
reseted from the realize handler of the core and the presenters are
not. This is less of an issue in XICS, although a zero MFFR could be a
concern, but in XIVE, the OS CAM line is not set and this breaks the
presenting algorithm. The current code has workarounds which need a
global cleanup.

Extend the sPAPR IRQ backend with a new cpu_intc_reset() handler which
will be called by the CPU reset handler and remove the XiveTCTX reset
handler which is redundant.

spapr_realize_vcpu() is modified to call the CPU reset only after the
intc presenter has been created.

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

 The change of order of the CPU reset and the intc creation could be
 in its own patch for bisect. This is fragile.
 
 include/hw/ppc/spapr_irq.h |  2 ++
 include/hw/ppc/xics.h      |  1 +
 include/hw/ppc/xive.h      |  1 +
 hw/intc/spapr_xive.c       |  7 +++++++
 hw/intc/xics.c             |  8 ++------
 hw/intc/xics_spapr.c       |  7 +++++++
 hw/intc/xive.c             | 12 +-----------
 hw/ppc/spapr_cpu_core.c    | 14 ++++++++++----
 hw/ppc/spapr_irq.c         | 14 ++++++++++++++
 9 files changed, 45 insertions(+), 21 deletions(-)

diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
index 5e150a667902..09232999b07e 100644
--- a/include/hw/ppc/spapr_irq.h
+++ b/include/hw/ppc/spapr_irq.h
@@ -52,6 +52,7 @@ typedef struct SpaprInterruptControllerClass {
      */
     int (*cpu_intc_create)(SpaprInterruptController *intc,
                             PowerPCCPU *cpu, Error **errp);
+    void (*cpu_intc_reset)(SpaprInterruptController *intc, PowerPCCPU *cpu);
     int (*claim_irq)(SpaprInterruptController *intc, int irq, bool lsi,
                      Error **errp);
     void (*free_irq)(SpaprInterruptController *intc, int irq);
@@ -68,6 +69,7 @@ void spapr_irq_update_active_intc(SpaprMachineState *spapr);
 
 int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
                               PowerPCCPU *cpu, Error **errp);
+void spapr_irq_cpu_intc_reset(SpaprMachineState *spapr, PowerPCCPU *cpu);
 void spapr_irq_print_info(SpaprMachineState *spapr, Monitor *mon);
 void spapr_irq_dt(SpaprMachineState *spapr, uint32_t nr_servers,
                   void *fdt, uint32_t phandle);
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 1e6a9300eb2b..602173c12250 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -161,6 +161,7 @@ void icp_set_mfrr(ICPState *icp, uint8_t mfrr);
 uint32_t icp_accept(ICPState *ss);
 uint32_t icp_ipoll(ICPState *ss, uint32_t *mfrr);
 void icp_eoi(ICPState *icp, uint32_t xirr);
+void icp_reset(ICPState *icp);
 
 void ics_write_xive(ICSState *ics, int nr, int server,
                     uint8_t priority, uint8_t saved_priority);
diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
index fd3319bd3202..99381639f50c 100644
--- a/include/hw/ppc/xive.h
+++ b/include/hw/ppc/xive.h
@@ -415,6 +415,7 @@ uint64_t xive_tctx_tm_read(XiveTCTX *tctx, hwaddr offset, unsigned size);
 
 void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon);
 Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp);
+void xive_tctx_reset(XiveTCTX *tctx);
 
 static inline uint32_t xive_nvt_cam_line(uint8_t nvt_blk, uint32_t nvt_idx)
 {
diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
index ba32d2cc5b0f..258b1c5fb5ff 100644
--- a/hw/intc/spapr_xive.c
+++ b/hw/intc/spapr_xive.c
@@ -553,6 +553,12 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
     return 0;
 }
 
+static void spapr_xive_cpu_intc_reset(SpaprInterruptController *intc,
+                                     PowerPCCPU *cpu)
+{
+    xive_tctx_reset(spapr_cpu_state(cpu)->tctx);
+}
+
 static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
 {
     SpaprXive *xive = SPAPR_XIVE(intc);
@@ -697,6 +703,7 @@ static void spapr_xive_class_init(ObjectClass *klass, void *data)
     sicc->activate = spapr_xive_activate;
     sicc->deactivate = spapr_xive_deactivate;
     sicc->cpu_intc_create = spapr_xive_cpu_intc_create;
+    sicc->cpu_intc_reset = spapr_xive_cpu_intc_reset;
     sicc->claim_irq = spapr_xive_claim_irq;
     sicc->free_irq = spapr_xive_free_irq;
     sicc->set_irq = spapr_xive_set_irq;
diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index b5ac408f7b74..6da05763f9db 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -274,10 +274,8 @@ static const VMStateDescription vmstate_icp_server = {
     },
 };
 
-static void icp_reset_handler(void *dev)
+void icp_reset(ICPState *icp)
 {
-    ICPState *icp = ICP(dev);
-
     icp->xirr = 0;
     icp->pending_priority = 0xff;
     icp->mfrr = 0xff;
@@ -288,7 +286,7 @@ static void icp_reset_handler(void *dev)
     if (kvm_irqchip_in_kernel()) {
         Error *local_err = NULL;
 
-        icp_set_kvm_state(ICP(dev), &local_err);
+        icp_set_kvm_state(icp, &local_err);
         if (local_err) {
             error_report_err(local_err);
         }
@@ -351,7 +349,6 @@ static void icp_realize(DeviceState *dev, Error **errp)
         }
     }
 
-    qemu_register_reset(icp_reset_handler, dev);
     vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
 }
 
@@ -360,7 +357,6 @@ static void icp_unrealize(DeviceState *dev, Error **errp)
     ICPState *icp = ICP(dev);
 
     vmstate_unregister(NULL, &vmstate_icp_server, icp);
-    qemu_unregister_reset(icp_reset_handler, dev);
 }
 
 static void icp_class_init(ObjectClass *klass, void *data)
diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
index 4f64b9a9fc66..7418fb9f370c 100644
--- a/hw/intc/xics_spapr.c
+++ b/hw/intc/xics_spapr.c
@@ -346,6 +346,12 @@ static int xics_spapr_cpu_intc_create(SpaprInterruptController *intc,
     return 0;
 }
 
+static void xics_spapr_cpu_intc_reset(SpaprInterruptController *intc,
+                                     PowerPCCPU *cpu)
+{
+    icp_reset(spapr_cpu_state(cpu)->icp);
+}
+
 static int xics_spapr_claim_irq(SpaprInterruptController *intc, int irq,
                                 bool lsi, Error **errp)
 {
@@ -433,6 +439,7 @@ static void ics_spapr_class_init(ObjectClass *klass, void *data)
     sicc->activate = xics_spapr_activate;
     sicc->deactivate = xics_spapr_deactivate;
     sicc->cpu_intc_create = xics_spapr_cpu_intc_create;
+    sicc->cpu_intc_reset = xics_spapr_cpu_intc_reset;
     sicc->claim_irq = xics_spapr_claim_irq;
     sicc->free_irq = xics_spapr_free_irq;
     sicc->set_irq = xics_spapr_set_irq;
diff --git a/hw/intc/xive.c b/hw/intc/xive.c
index d420c6571e14..f066be5eb5e3 100644
--- a/hw/intc/xive.c
+++ b/hw/intc/xive.c
@@ -547,10 +547,8 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon)
     }
 }
 
-static void xive_tctx_reset(void *dev)
+void xive_tctx_reset(XiveTCTX *tctx)
 {
-    XiveTCTX *tctx = XIVE_TCTX(dev);
-
     memset(tctx->regs, 0, sizeof(tctx->regs));
 
     /* Set some defaults */
@@ -607,13 +605,6 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp)
             return;
         }
     }
-
-    qemu_register_reset(xive_tctx_reset, dev);
-}
-
-static void xive_tctx_unrealize(DeviceState *dev, Error **errp)
-{
-    qemu_unregister_reset(xive_tctx_reset, dev);
 }
 
 static int vmstate_xive_tctx_pre_save(void *opaque)
@@ -668,7 +659,6 @@ static void xive_tctx_class_init(ObjectClass *klass, void *data)
 
     dc->desc = "XIVE Interrupt Thread Context";
     dc->realize = xive_tctx_realize;
-    dc->unrealize = xive_tctx_unrealize;
     dc->vmsd = &vmstate_xive_tctx;
     /*
      * Reason: part of XIVE interrupt controller, needs to be wired up
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 3e4302c7d596..d04baa7cde4f 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -33,6 +33,7 @@ static void spapr_cpu_reset(void *opaque)
     PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
     SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
     target_ulong lpcr;
+    SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
 
     cpu_reset(cs);
 
@@ -77,9 +78,11 @@ static void spapr_cpu_reset(void *opaque)
     spapr_cpu->dtl_addr = 0;
     spapr_cpu->dtl_size = 0;
 
-    spapr_caps_cpu_apply(SPAPR_MACHINE(qdev_get_machine()), cpu);
+    spapr_caps_cpu_apply(spapr, cpu);
 
     kvm_check_mmu(cpu, &error_fatal);
+
+    spapr_irq_cpu_intc_reset(spapr, cpu);
 }
 
 void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3)
@@ -234,13 +237,16 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr,
     cpu_ppc_set_vhyp(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
     kvmppc_set_papr(cpu);
 
-    qemu_register_reset(spapr_cpu_reset, cpu);
-    spapr_cpu_reset(cpu);
-
     if (spapr_irq_cpu_intc_create(spapr, cpu, &local_err) < 0) {
         goto error_unregister;
     }
 
+    /*
+     * Hot-plugged CPUs are not reseted. Do it here.
+     */
+    qemu_register_reset(spapr_cpu_reset, cpu);
+    spapr_cpu_reset(cpu);
+
     if (!sc->pre_3_0_migration) {
         vmstate_register(NULL, cs->cpu_index, &vmstate_spapr_cpu_state,
                          cpu->machine_data);
diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
index 234d1073e518..b941608b69ba 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -220,6 +220,20 @@ int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
     return 0;
 }
 
+void spapr_irq_cpu_intc_reset(SpaprMachineState *spapr, PowerPCCPU *cpu)
+{
+    SpaprInterruptController *intcs[] = ALL_INTCS(spapr);
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(intcs); i++) {
+        SpaprInterruptController *intc = intcs[i];
+        if (intc) {
+            SpaprInterruptControllerClass *sicc = SPAPR_INTC_GET_CLASS(intc);
+            sicc->cpu_intc_reset(intc, cpu);
+        }
+    }
+}
+
 static void spapr_set_irq(void *opaque, int irq, int level)
 {
     SpaprMachineState *spapr = SPAPR_MACHINE(opaque);
-- 
2.21.0

Re: [PATCH v2 1/2] spapr: Introduce a interrupt presenter reset handler

[Greg Kurz]

On Fri, 18 Oct 2019 19:22:18 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> The interrupt presenters are created by a machine handler at the core
> level and are reseted independently. This is not consistent and it
> raises some issues when it comes to handle hot-plugged CPUs. These are
> reseted from the realize handler of the core and the presenters are
> not. This is less of an issue in XICS, although a zero MFFR could be a
> concern, but in XIVE, the OS CAM line is not set and this breaks the
> presenting algorithm. The current code has workarounds which need a
> global cleanup.
> 
> Extend the sPAPR IRQ backend with a new cpu_intc_reset() handler which
> will be called by the CPU reset handler and remove the XiveTCTX reset
> handler which is redundant.
> 
> spapr_realize_vcpu() is modified to call the CPU reset only after the
> intc presenter has been created.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> 
>  The change of order of the CPU reset and the intc creation could be
>  in its own patch for bisect. This is fragile.
>  

And there's a nit. See below.

Otherwise, LGTM.

>  include/hw/ppc/spapr_irq.h |  2 ++
>  include/hw/ppc/xics.h      |  1 +
>  include/hw/ppc/xive.h      |  1 +
>  hw/intc/spapr_xive.c       |  7 +++++++
>  hw/intc/xics.c             |  8 ++------
>  hw/intc/xics_spapr.c       |  7 +++++++
>  hw/intc/xive.c             | 12 +-----------
>  hw/ppc/spapr_cpu_core.c    | 14 ++++++++++----
>  hw/ppc/spapr_irq.c         | 14 ++++++++++++++
>  9 files changed, 45 insertions(+), 21 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> index 5e150a667902..09232999b07e 100644
> --- a/include/hw/ppc/spapr_irq.h
> +++ b/include/hw/ppc/spapr_irq.h
> @@ -52,6 +52,7 @@ typedef struct SpaprInterruptControllerClass {
>       */
>      int (*cpu_intc_create)(SpaprInterruptController *intc,
>                              PowerPCCPU *cpu, Error **errp);
> +    void (*cpu_intc_reset)(SpaprInterruptController *intc, PowerPCCPU *cpu);
>      int (*claim_irq)(SpaprInterruptController *intc, int irq, bool lsi,
>                       Error **errp);
>      void (*free_irq)(SpaprInterruptController *intc, int irq);
> @@ -68,6 +69,7 @@ void spapr_irq_update_active_intc(SpaprMachineState *spapr);
>  
>  int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
>                                PowerPCCPU *cpu, Error **errp);
> +void spapr_irq_cpu_intc_reset(SpaprMachineState *spapr, PowerPCCPU *cpu);
>  void spapr_irq_print_info(SpaprMachineState *spapr, Monitor *mon);
>  void spapr_irq_dt(SpaprMachineState *spapr, uint32_t nr_servers,
>                    void *fdt, uint32_t phandle);
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 1e6a9300eb2b..602173c12250 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -161,6 +161,7 @@ void icp_set_mfrr(ICPState *icp, uint8_t mfrr);
>  uint32_t icp_accept(ICPState *ss);
>  uint32_t icp_ipoll(ICPState *ss, uint32_t *mfrr);
>  void icp_eoi(ICPState *icp, uint32_t xirr);
> +void icp_reset(ICPState *icp);
>  
>  void ics_write_xive(ICSState *ics, int nr, int server,
>                      uint8_t priority, uint8_t saved_priority);
> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> index fd3319bd3202..99381639f50c 100644
> --- a/include/hw/ppc/xive.h
> +++ b/include/hw/ppc/xive.h
> @@ -415,6 +415,7 @@ uint64_t xive_tctx_tm_read(XiveTCTX *tctx, hwaddr offset, unsigned size);
>  
>  void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon);
>  Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp);
> +void xive_tctx_reset(XiveTCTX *tctx);
>  
>  static inline uint32_t xive_nvt_cam_line(uint8_t nvt_blk, uint32_t nvt_idx)
>  {
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index ba32d2cc5b0f..258b1c5fb5ff 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -553,6 +553,12 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
>      return 0;
>  }
>  
> +static void spapr_xive_cpu_intc_reset(SpaprInterruptController *intc,
> +                                     PowerPCCPU *cpu)
> +{
> +    xive_tctx_reset(spapr_cpu_state(cpu)->tctx);
> +}
> +
>  static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
>  {
>      SpaprXive *xive = SPAPR_XIVE(intc);
> @@ -697,6 +703,7 @@ static void spapr_xive_class_init(ObjectClass *klass, void *data)
>      sicc->activate = spapr_xive_activate;
>      sicc->deactivate = spapr_xive_deactivate;
>      sicc->cpu_intc_create = spapr_xive_cpu_intc_create;
> +    sicc->cpu_intc_reset = spapr_xive_cpu_intc_reset;
>      sicc->claim_irq = spapr_xive_claim_irq;
>      sicc->free_irq = spapr_xive_free_irq;
>      sicc->set_irq = spapr_xive_set_irq;
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index b5ac408f7b74..6da05763f9db 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -274,10 +274,8 @@ static const VMStateDescription vmstate_icp_server = {
>      },
>  };
>  
> -static void icp_reset_handler(void *dev)
> +void icp_reset(ICPState *icp)
>  {
> -    ICPState *icp = ICP(dev);
> -
>      icp->xirr = 0;
>      icp->pending_priority = 0xff;
>      icp->mfrr = 0xff;
> @@ -288,7 +286,7 @@ static void icp_reset_handler(void *dev)
>      if (kvm_irqchip_in_kernel()) {
>          Error *local_err = NULL;
>  
> -        icp_set_kvm_state(ICP(dev), &local_err);
> +        icp_set_kvm_state(icp, &local_err);
>          if (local_err) {
>              error_report_err(local_err);
>          }
> @@ -351,7 +349,6 @@ static void icp_realize(DeviceState *dev, Error **errp)
>          }
>      }
>  
> -    qemu_register_reset(icp_reset_handler, dev);
>      vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
>  }
>  
> @@ -360,7 +357,6 @@ static void icp_unrealize(DeviceState *dev, Error **errp)
>      ICPState *icp = ICP(dev);
>  
>      vmstate_unregister(NULL, &vmstate_icp_server, icp);
> -    qemu_unregister_reset(icp_reset_handler, dev);
>  }
>  
>  static void icp_class_init(ObjectClass *klass, void *data)
> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> index 4f64b9a9fc66..7418fb9f370c 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -346,6 +346,12 @@ static int xics_spapr_cpu_intc_create(SpaprInterruptController *intc,
>      return 0;
>  }
>  
> +static void xics_spapr_cpu_intc_reset(SpaprInterruptController *intc,
> +                                     PowerPCCPU *cpu)
> +{
> +    icp_reset(spapr_cpu_state(cpu)->icp);
> +}
> +
>  static int xics_spapr_claim_irq(SpaprInterruptController *intc, int irq,
>                                  bool lsi, Error **errp)
>  {
> @@ -433,6 +439,7 @@ static void ics_spapr_class_init(ObjectClass *klass, void *data)
>      sicc->activate = xics_spapr_activate;
>      sicc->deactivate = xics_spapr_deactivate;
>      sicc->cpu_intc_create = xics_spapr_cpu_intc_create;
> +    sicc->cpu_intc_reset = xics_spapr_cpu_intc_reset;
>      sicc->claim_irq = xics_spapr_claim_irq;
>      sicc->free_irq = xics_spapr_free_irq;
>      sicc->set_irq = xics_spapr_set_irq;
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index d420c6571e14..f066be5eb5e3 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -547,10 +547,8 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon)
>      }
>  }
>  
> -static void xive_tctx_reset(void *dev)
> +void xive_tctx_reset(XiveTCTX *tctx)
>  {
> -    XiveTCTX *tctx = XIVE_TCTX(dev);
> -
>      memset(tctx->regs, 0, sizeof(tctx->regs));
>  
>      /* Set some defaults */
> @@ -607,13 +605,6 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp)
>              return;
>          }
>      }
> -
> -    qemu_register_reset(xive_tctx_reset, dev);
> -}
> -
> -static void xive_tctx_unrealize(DeviceState *dev, Error **errp)
> -{
> -    qemu_unregister_reset(xive_tctx_reset, dev);
>  }
>  
>  static int vmstate_xive_tctx_pre_save(void *opaque)
> @@ -668,7 +659,6 @@ static void xive_tctx_class_init(ObjectClass *klass, void *data)
>  
>      dc->desc = "XIVE Interrupt Thread Context";
>      dc->realize = xive_tctx_realize;
> -    dc->unrealize = xive_tctx_unrealize;
>      dc->vmsd = &vmstate_xive_tctx;
>      /*
>       * Reason: part of XIVE interrupt controller, needs to be wired up
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 3e4302c7d596..d04baa7cde4f 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -33,6 +33,7 @@ static void spapr_cpu_reset(void *opaque)
>      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>      SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
>      target_ulong lpcr;
> +    SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>  
>      cpu_reset(cs);
>  
> @@ -77,9 +78,11 @@ static void spapr_cpu_reset(void *opaque)
>      spapr_cpu->dtl_addr = 0;
>      spapr_cpu->dtl_size = 0;
>  
> -    spapr_caps_cpu_apply(SPAPR_MACHINE(qdev_get_machine()), cpu);
> +    spapr_caps_cpu_apply(spapr, cpu);
>  
>      kvm_check_mmu(cpu, &error_fatal);
> +
> +    spapr_irq_cpu_intc_reset(spapr, cpu);
>  }
>  
>  void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3)
> @@ -234,13 +237,16 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr,
>      cpu_ppc_set_vhyp(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
>      kvmppc_set_papr(cpu);
>  
> -    qemu_register_reset(spapr_cpu_reset, cpu);
> -    spapr_cpu_reset(cpu);
> -
>      if (spapr_irq_cpu_intc_create(spapr, cpu, &local_err) < 0) {
>          goto error_unregister;

We no longer need to call qemu_unregister_reset() on rollback then.
Maybe rename the label as well.

>      }
>  
> +    /*
> +     * Hot-plugged CPUs are not reseted. Do it here.
> +     */
> +    qemu_register_reset(spapr_cpu_reset, cpu);
> +    spapr_cpu_reset(cpu);
> +
>      if (!sc->pre_3_0_migration) {
>          vmstate_register(NULL, cs->cpu_index, &vmstate_spapr_cpu_state,
>                           cpu->machine_data);
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 234d1073e518..b941608b69ba 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -220,6 +220,20 @@ int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
>      return 0;
>  }
>  
> +void spapr_irq_cpu_intc_reset(SpaprMachineState *spapr, PowerPCCPU *cpu)
> +{
> +    SpaprInterruptController *intcs[] = ALL_INTCS(spapr);
> +    int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(intcs); i++) {
> +        SpaprInterruptController *intc = intcs[i];
> +        if (intc) {
> +            SpaprInterruptControllerClass *sicc = SPAPR_INTC_GET_CLASS(intc);
> +            sicc->cpu_intc_reset(intc, cpu);
> +        }
> +    }
> +}
> +
>  static void spapr_set_irq(void *opaque, int irq, int level)
>  {
>      SpaprMachineState *spapr = SPAPR_MACHINE(opaque);

Re: [PATCH v2 1/2] spapr: Introduce a interrupt presenter reset handler

[Cédric Le Goater]

On 19/10/2019 08:02, Greg Kurz wrote:
> On Fri, 18 Oct 2019 19:22:18 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
> 
>> The interrupt presenters are created by a machine handler at the core
>> level and are reseted independently. This is not consistent and it
>> raises some issues when it comes to handle hot-plugged CPUs. These are
>> reseted from the realize handler of the core and the presenters are
>> not. This is less of an issue in XICS, although a zero MFFR could be a
>> concern, but in XIVE, the OS CAM line is not set and this breaks the
>> presenting algorithm. The current code has workarounds which need a
>> global cleanup.
>>
>> Extend the sPAPR IRQ backend with a new cpu_intc_reset() handler which
>> will be called by the CPU reset handler and remove the XiveTCTX reset
>> handler which is redundant.
>>
>> spapr_realize_vcpu() is modified to call the CPU reset only after the
>> intc presenter has been created.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>
>>  The change of order of the CPU reset and the intc creation could be
>>  in its own patch for bisect. This is fragile.
>>  
> 
> And there's a nit. See below.
> 
> Otherwise, LGTM.

ok. I will address that in a v3. 

The CPUs of the PowerNV machine are very similar, (no hotplug though) and 
I also need to add the same kind of intc reset.

Thanks,

C. 

> 
>>  include/hw/ppc/spapr_irq.h |  2 ++
>>  include/hw/ppc/xics.h      |  1 +
>>  include/hw/ppc/xive.h      |  1 +
>>  hw/intc/spapr_xive.c       |  7 +++++++
>>  hw/intc/xics.c             |  8 ++------
>>  hw/intc/xics_spapr.c       |  7 +++++++
>>  hw/intc/xive.c             | 12 +-----------
>>  hw/ppc/spapr_cpu_core.c    | 14 ++++++++++----
>>  hw/ppc/spapr_irq.c         | 14 ++++++++++++++
>>  9 files changed, 45 insertions(+), 21 deletions(-)
>>
>> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
>> index 5e150a667902..09232999b07e 100644
>> --- a/include/hw/ppc/spapr_irq.h
>> +++ b/include/hw/ppc/spapr_irq.h
>> @@ -52,6 +52,7 @@ typedef struct SpaprInterruptControllerClass {
>>       */
>>      int (*cpu_intc_create)(SpaprInterruptController *intc,
>>                              PowerPCCPU *cpu, Error **errp);
>> +    void (*cpu_intc_reset)(SpaprInterruptController *intc, PowerPCCPU *cpu);
>>      int (*claim_irq)(SpaprInterruptController *intc, int irq, bool lsi,
>>                       Error **errp);
>>      void (*free_irq)(SpaprInterruptController *intc, int irq);
>> @@ -68,6 +69,7 @@ void spapr_irq_update_active_intc(SpaprMachineState *spapr);
>>  
>>  int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
>>                                PowerPCCPU *cpu, Error **errp);
>> +void spapr_irq_cpu_intc_reset(SpaprMachineState *spapr, PowerPCCPU *cpu);
>>  void spapr_irq_print_info(SpaprMachineState *spapr, Monitor *mon);
>>  void spapr_irq_dt(SpaprMachineState *spapr, uint32_t nr_servers,
>>                    void *fdt, uint32_t phandle);
>> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
>> index 1e6a9300eb2b..602173c12250 100644
>> --- a/include/hw/ppc/xics.h
>> +++ b/include/hw/ppc/xics.h
>> @@ -161,6 +161,7 @@ void icp_set_mfrr(ICPState *icp, uint8_t mfrr);
>>  uint32_t icp_accept(ICPState *ss);
>>  uint32_t icp_ipoll(ICPState *ss, uint32_t *mfrr);
>>  void icp_eoi(ICPState *icp, uint32_t xirr);
>> +void icp_reset(ICPState *icp);
>>  
>>  void ics_write_xive(ICSState *ics, int nr, int server,
>>                      uint8_t priority, uint8_t saved_priority);
>> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
>> index fd3319bd3202..99381639f50c 100644
>> --- a/include/hw/ppc/xive.h
>> +++ b/include/hw/ppc/xive.h
>> @@ -415,6 +415,7 @@ uint64_t xive_tctx_tm_read(XiveTCTX *tctx, hwaddr offset, unsigned size);
>>  
>>  void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon);
>>  Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp);
>> +void xive_tctx_reset(XiveTCTX *tctx);
>>  
>>  static inline uint32_t xive_nvt_cam_line(uint8_t nvt_blk, uint32_t nvt_idx)
>>  {
>> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
>> index ba32d2cc5b0f..258b1c5fb5ff 100644
>> --- a/hw/intc/spapr_xive.c
>> +++ b/hw/intc/spapr_xive.c
>> @@ -553,6 +553,12 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
>>      return 0;
>>  }
>>  
>> +static void spapr_xive_cpu_intc_reset(SpaprInterruptController *intc,
>> +                                     PowerPCCPU *cpu)
>> +{
>> +    xive_tctx_reset(spapr_cpu_state(cpu)->tctx);
>> +}
>> +
>>  static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
>>  {
>>      SpaprXive *xive = SPAPR_XIVE(intc);
>> @@ -697,6 +703,7 @@ static void spapr_xive_class_init(ObjectClass *klass, void *data)
>>      sicc->activate = spapr_xive_activate;
>>      sicc->deactivate = spapr_xive_deactivate;
>>      sicc->cpu_intc_create = spapr_xive_cpu_intc_create;
>> +    sicc->cpu_intc_reset = spapr_xive_cpu_intc_reset;
>>      sicc->claim_irq = spapr_xive_claim_irq;
>>      sicc->free_irq = spapr_xive_free_irq;
>>      sicc->set_irq = spapr_xive_set_irq;
>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
>> index b5ac408f7b74..6da05763f9db 100644
>> --- a/hw/intc/xics.c
>> +++ b/hw/intc/xics.c
>> @@ -274,10 +274,8 @@ static const VMStateDescription vmstate_icp_server = {
>>      },
>>  };
>>  
>> -static void icp_reset_handler(void *dev)
>> +void icp_reset(ICPState *icp)
>>  {
>> -    ICPState *icp = ICP(dev);
>> -
>>      icp->xirr = 0;
>>      icp->pending_priority = 0xff;
>>      icp->mfrr = 0xff;
>> @@ -288,7 +286,7 @@ static void icp_reset_handler(void *dev)
>>      if (kvm_irqchip_in_kernel()) {
>>          Error *local_err = NULL;
>>  
>> -        icp_set_kvm_state(ICP(dev), &local_err);
>> +        icp_set_kvm_state(icp, &local_err);
>>          if (local_err) {
>>              error_report_err(local_err);
>>          }
>> @@ -351,7 +349,6 @@ static void icp_realize(DeviceState *dev, Error **errp)
>>          }
>>      }
>>  
>> -    qemu_register_reset(icp_reset_handler, dev);
>>      vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
>>  }
>>  
>> @@ -360,7 +357,6 @@ static void icp_unrealize(DeviceState *dev, Error **errp)
>>      ICPState *icp = ICP(dev);
>>  
>>      vmstate_unregister(NULL, &vmstate_icp_server, icp);
>> -    qemu_unregister_reset(icp_reset_handler, dev);
>>  }
>>  
>>  static void icp_class_init(ObjectClass *klass, void *data)
>> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
>> index 4f64b9a9fc66..7418fb9f370c 100644
>> --- a/hw/intc/xics_spapr.c
>> +++ b/hw/intc/xics_spapr.c
>> @@ -346,6 +346,12 @@ static int xics_spapr_cpu_intc_create(SpaprInterruptController *intc,
>>      return 0;
>>  }
>>  
>> +static void xics_spapr_cpu_intc_reset(SpaprInterruptController *intc,
>> +                                     PowerPCCPU *cpu)
>> +{
>> +    icp_reset(spapr_cpu_state(cpu)->icp);
>> +}
>> +
>>  static int xics_spapr_claim_irq(SpaprInterruptController *intc, int irq,
>>                                  bool lsi, Error **errp)
>>  {
>> @@ -433,6 +439,7 @@ static void ics_spapr_class_init(ObjectClass *klass, void *data)
>>      sicc->activate = xics_spapr_activate;
>>      sicc->deactivate = xics_spapr_deactivate;
>>      sicc->cpu_intc_create = xics_spapr_cpu_intc_create;
>> +    sicc->cpu_intc_reset = xics_spapr_cpu_intc_reset;
>>      sicc->claim_irq = xics_spapr_claim_irq;
>>      sicc->free_irq = xics_spapr_free_irq;
>>      sicc->set_irq = xics_spapr_set_irq;
>> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
>> index d420c6571e14..f066be5eb5e3 100644
>> --- a/hw/intc/xive.c
>> +++ b/hw/intc/xive.c
>> @@ -547,10 +547,8 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon)
>>      }
>>  }
>>  
>> -static void xive_tctx_reset(void *dev)
>> +void xive_tctx_reset(XiveTCTX *tctx)
>>  {
>> -    XiveTCTX *tctx = XIVE_TCTX(dev);
>> -
>>      memset(tctx->regs, 0, sizeof(tctx->regs));
>>  
>>      /* Set some defaults */
>> @@ -607,13 +605,6 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp)
>>              return;
>>          }
>>      }
>> -
>> -    qemu_register_reset(xive_tctx_reset, dev);
>> -}
>> -
>> -static void xive_tctx_unrealize(DeviceState *dev, Error **errp)
>> -{
>> -    qemu_unregister_reset(xive_tctx_reset, dev);
>>  }
>>  
>>  static int vmstate_xive_tctx_pre_save(void *opaque)
>> @@ -668,7 +659,6 @@ static void xive_tctx_class_init(ObjectClass *klass, void *data)
>>  
>>      dc->desc = "XIVE Interrupt Thread Context";
>>      dc->realize = xive_tctx_realize;
>> -    dc->unrealize = xive_tctx_unrealize;
>>      dc->vmsd = &vmstate_xive_tctx;
>>      /*
>>       * Reason: part of XIVE interrupt controller, needs to be wired up
>> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
>> index 3e4302c7d596..d04baa7cde4f 100644
>> --- a/hw/ppc/spapr_cpu_core.c
>> +++ b/hw/ppc/spapr_cpu_core.c
>> @@ -33,6 +33,7 @@ static void spapr_cpu_reset(void *opaque)
>>      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>>      SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
>>      target_ulong lpcr;
>> +    SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>>  
>>      cpu_reset(cs);
>>  
>> @@ -77,9 +78,11 @@ static void spapr_cpu_reset(void *opaque)
>>      spapr_cpu->dtl_addr = 0;
>>      spapr_cpu->dtl_size = 0;
>>  
>> -    spapr_caps_cpu_apply(SPAPR_MACHINE(qdev_get_machine()), cpu);
>> +    spapr_caps_cpu_apply(spapr, cpu);
>>  
>>      kvm_check_mmu(cpu, &error_fatal);
>> +
>> +    spapr_irq_cpu_intc_reset(spapr, cpu);
>>  }
>>  
>>  void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3)
>> @@ -234,13 +237,16 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr,
>>      cpu_ppc_set_vhyp(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
>>      kvmppc_set_papr(cpu);
>>  
>> -    qemu_register_reset(spapr_cpu_reset, cpu);
>> -    spapr_cpu_reset(cpu);
>> -
>>      if (spapr_irq_cpu_intc_create(spapr, cpu, &local_err) < 0) {
>>          goto error_unregister;
> 
> We no longer need to call qemu_unregister_reset() on rollback then.
> Maybe rename the label as well.
> 
>>      }
>>  
>> +    /*
>> +     * Hot-plugged CPUs are not reseted. Do it here.
>> +     */
>> +    qemu_register_reset(spapr_cpu_reset, cpu);
>> +    spapr_cpu_reset(cpu);
>> +
>>      if (!sc->pre_3_0_migration) {
>>          vmstate_register(NULL, cs->cpu_index, &vmstate_spapr_cpu_state,
>>                           cpu->machine_data);
>> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
>> index 234d1073e518..b941608b69ba 100644
>> --- a/hw/ppc/spapr_irq.c
>> +++ b/hw/ppc/spapr_irq.c
>> @@ -220,6 +220,20 @@ int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
>>      return 0;
>>  }
>>  
>> +void spapr_irq_cpu_intc_reset(SpaprMachineState *spapr, PowerPCCPU *cpu)
>> +{
>> +    SpaprInterruptController *intcs[] = ALL_INTCS(spapr);
>> +    int i;
>> +
>> +    for (i = 0; i < ARRAY_SIZE(intcs); i++) {
>> +        SpaprInterruptController *intc = intcs[i];
>> +        if (intc) {
>> +            SpaprInterruptControllerClass *sicc = SPAPR_INTC_GET_CLASS(intc);
>> +            sicc->cpu_intc_reset(intc, cpu);
>> +        }
>> +    }
>> +}
>> +
>>  static void spapr_set_irq(void *opaque, int irq, int level)
>>  {
>>      SpaprMachineState *spapr = SPAPR_MACHINE(opaque);
> 

[PATCH v2 2/2] spapr/xive: Set the OS CAM line at reset

[Cédric Le Goater]

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.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ppc/spapr_xive.h |  1 -
 hw/intc/spapr_xive.c        | 18 +++---------------
 2 files changed, 3 insertions(+), 16 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 258b1c5fb5ff..4f584e582b6c 100644
--- a/hw/intc/spapr_xive.c
+++ b/hw/intc/spapr_xive.c
@@ -210,7 +210,7 @@ void spapr_xive_mmio_set_enabled(SpaprXive *xive, bool enable)
  * hypervisor pushes its identifier in the OS CAM line. Emulate the
  * same behavior under QEMU.
  */
-void spapr_xive_set_tctx_os_cam(XiveTCTX *tctx)
+static void spapr_xive_set_tctx_os_cam(XiveTCTX *tctx)
 {
     uint8_t  nvt_blk;
     uint32_t nvt_idx;
@@ -544,12 +544,6 @@ 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;
 }
 
@@ -557,6 +551,8 @@ static void spapr_xive_cpu_intc_reset(SpaprInterruptController *intc,
                                      PowerPCCPU *cpu)
 {
     xive_tctx_reset(spapr_cpu_state(cpu)->tctx);
+
+    spapr_xive_set_tctx_os_cam(spapr_cpu_state(cpu)->tctx);
 }
 
 static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
@@ -649,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);
-- 
2.21.0

Re: [PATCH v2 2/2] spapr/xive: Set the OS CAM line at reset

[Greg Kurz]

On Fri, 18 Oct 2019 19:22:19 +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.
> 

Yeah, CPU_FOREACH() can bite hard... it's easy as 1-2-3 to crash QEMU with the
ones in xics_spapr_print_info() and spapr_xive_print_info(). I'll post fixes
soon.

> 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        | 18 +++---------------
>  2 files changed, 3 insertions(+), 16 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 258b1c5fb5ff..4f584e582b6c 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -210,7 +210,7 @@ void spapr_xive_mmio_set_enabled(SpaprXive *xive, bool enable)
>   * hypervisor pushes its identifier in the OS CAM line. Emulate the
>   * same behavior under QEMU.
>   */
> -void spapr_xive_set_tctx_os_cam(XiveTCTX *tctx)
> +static void spapr_xive_set_tctx_os_cam(XiveTCTX *tctx)
>  {
>      uint8_t  nvt_blk;
>      uint32_t nvt_idx;
> @@ -544,12 +544,6 @@ 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;
>  }
>  
> @@ -557,6 +551,8 @@ static void spapr_xive_cpu_intc_reset(SpaprInterruptController *intc,
>                                       PowerPCCPU *cpu)
>  {
>      xive_tctx_reset(spapr_cpu_state(cpu)->tctx);
> +
> +    spapr_xive_set_tctx_os_cam(spapr_cpu_state(cpu)->tctx);
>  }
>  
>  static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
> @@ -649,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);