[PATCH 0/2] spapr: interrupt presenter fixes

[PATCH 0/2] spapr: interrupt presenter fixes

[Cédric Le Goater]

Hello,

First add a cpu_intc_reset() handler to reset interrupt presenters
which are not today and Introduce a 'os-cam' property which will be
used to set the OS CAM line at reset.

Thanks,

C.

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  |  4 ++++
 include/hw/ppc/spapr_xive.h |  1 -
 include/hw/ppc/xics.h       |  1 +
 include/hw/ppc/xive.h       |  5 ++++-
 hw/intc/spapr_xive.c        | 37 ++++++++++++-------------------------
 hw/intc/xics.c              |  5 +++++
 hw/intc/xics_spapr.c        |  8 ++++++++
 hw/intc/xive.c              | 33 +++++++++++++++++++++++++++++----
 hw/ppc/pnv.c                |  3 ++-
 hw/ppc/spapr_cpu_core.c     |  8 ++++++--
 hw/ppc/spapr_irq.c          | 21 +++++++++++++++++++++
 11 files changed, 92 insertions(+), 34 deletions(-)

-- 
2.21.0

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

[Cédric Le Goater]

The interrupt presenters are not reseted today. Extend the sPAPR IRQ
backend with a new cpu_intc_reset() handler which will be called by
the CPU reset handler.

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

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ppc/spapr_irq.h |  4 ++++
 include/hw/ppc/xics.h      |  1 +
 include/hw/ppc/xive.h      |  1 +
 hw/intc/spapr_xive.c       |  8 ++++++++
 hw/intc/xics.c             |  5 +++++
 hw/intc/xics_spapr.c       |  8 ++++++++
 hw/intc/xive.c             | 11 ++++++++---
 hw/ppc/spapr_cpu_core.c    |  8 ++++++--
 hw/ppc/spapr_irq.c         | 21 +++++++++++++++++++++
 9 files changed, 62 insertions(+), 5 deletions(-)

diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
index 5e150a667902..78327496c102 100644
--- a/include/hw/ppc/spapr_irq.h
+++ b/include/hw/ppc/spapr_irq.h
@@ -52,6 +52,8 @@ typedef struct SpaprInterruptControllerClass {
      */
     int (*cpu_intc_create)(SpaprInterruptController *intc,
                             PowerPCCPU *cpu, Error **errp);
+    int (*cpu_intc_reset)(SpaprInterruptController *intc, PowerPCCPU *cpu,
+                          Error **errp);
     int (*claim_irq)(SpaprInterruptController *intc, int irq, bool lsi,
                      Error **errp);
     void (*free_irq)(SpaprInterruptController *intc, int irq);
@@ -68,6 +70,8 @@ void spapr_irq_update_active_intc(SpaprMachineState *spapr);
 
 int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
                               PowerPCCPU *cpu, Error **errp);
+int spapr_irq_cpu_intc_reset(SpaprMachineState *spapr,
+                             PowerPCCPU *cpu, Error **errp);
 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..0c3acf1a4192 100644
--- a/hw/intc/spapr_xive.c
+++ b/hw/intc/spapr_xive.c
@@ -553,6 +553,13 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
     return 0;
 }
 
+static int spapr_xive_cpu_intc_reset(SpaprInterruptController *intc,
+                                     PowerPCCPU *cpu, Error **errp)
+{
+    xive_tctx_reset(spapr_cpu_state(cpu)->tctx);
+    return 0;
+}
+
 static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
 {
     SpaprXive *xive = SPAPR_XIVE(intc);
@@ -697,6 +704,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..652771d6a5a5 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -295,6 +295,11 @@ static void icp_reset_handler(void *dev)
     }
 }
 
+void icp_reset(ICPState *icp)
+{
+    icp_reset_handler(icp);
+}
+
 static void icp_realize(DeviceState *dev, Error **errp)
 {
     ICPState *icp = ICP(dev);
diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
index 4f64b9a9fc66..c0b2a576effe 100644
--- a/hw/intc/xics_spapr.c
+++ b/hw/intc/xics_spapr.c
@@ -346,6 +346,13 @@ static int xics_spapr_cpu_intc_create(SpaprInterruptController *intc,
     return 0;
 }
 
+static int xics_spapr_cpu_intc_reset(SpaprInterruptController *intc,
+                                     PowerPCCPU *cpu, Error **errp)
+{
+    icp_reset(spapr_cpu_state(cpu)->icp);
+    return 0;
+}
+
 static int xics_spapr_claim_irq(SpaprInterruptController *intc, int irq,
                                 bool lsi, Error **errp)
 {
@@ -433,6 +440,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..0ae3f9b1efe4 100644
--- a/hw/intc/xive.c
+++ b/hw/intc/xive.c
@@ -547,7 +547,7 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon)
     }
 }
 
-static void xive_tctx_reset(void *dev)
+static void xive_tctx_reset_handler(void *dev)
 {
     XiveTCTX *tctx = XIVE_TCTX(dev);
 
@@ -568,6 +568,11 @@ static void xive_tctx_reset(void *dev)
         ipb_to_pipr(tctx->regs[TM_QW3_HV_PHYS + TM_IPB]);
 }
 
+void xive_tctx_reset(XiveTCTX *tctx)
+{
+    xive_tctx_reset_handler(tctx);
+}
+
 static void xive_tctx_realize(DeviceState *dev, Error **errp)
 {
     XiveTCTX *tctx = XIVE_TCTX(dev);
@@ -608,12 +613,12 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp)
         }
     }
 
-    qemu_register_reset(xive_tctx_reset, dev);
+    qemu_register_reset(xive_tctx_reset_handler, dev);
 }
 
 static void xive_tctx_unrealize(DeviceState *dev, Error **errp)
 {
-    qemu_unregister_reset(xive_tctx_reset, dev);
+    qemu_unregister_reset(xive_tctx_reset_handler, dev);
 }
 
 static int vmstate_xive_tctx_pre_save(void *opaque)
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 3e4302c7d596..416aa75e5fba 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, &error_fatal);
 }
 
 void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3)
@@ -235,12 +238,13 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *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;
     }
 
+    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 bb91c61fa000..5d2b64029cd5 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -220,6 +220,27 @@ int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
     return 0;
 }
 
+int spapr_irq_cpu_intc_reset(SpaprMachineState *spapr,
+                             PowerPCCPU *cpu, Error **errp)
+{
+    SpaprInterruptController *intcs[] = ALL_INTCS(spapr);
+    int i;
+    int rc;
+
+    for (i = 0; i < ARRAY_SIZE(intcs); i++) {
+        SpaprInterruptController *intc = intcs[i];
+        if (intc) {
+            SpaprInterruptControllerClass *sicc = SPAPR_INTC_GET_CLASS(intc);
+            rc = sicc->cpu_intc_reset(intc, cpu, errp);
+            if (rc < 0) {
+                return rc;
+            }
+        }
+    }
+
+    return 0;
+}
+
 static void spapr_set_irq(void *opaque, int irq, int level)
 {
     SpaprMachineState *spapr = SPAPR_MACHINE(opaque);
-- 
2.21.0

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 10042 bytes --]

On Thu, Oct 17, 2019 at 04:42:40PM +0200, Cédric Le Goater wrote:
> The interrupt presenters are not reseted today.

I don't think that's accurate.  We register reset handlers for both
ICP and TCTX already.  We might not be resetting in quite the right
order, but this will need a clearer description of what's changing.

Also, with this patch as is, I think we'll reset twice (once from the
registered handler, once via the cpu).

> Extend the sPAPR IRQ
> backend with a new cpu_intc_reset() handler which will be called by
> the CPU reset handler.
> 
> spapr_realize_vcpu() is modified to call the CPU reset only after the
> the intc presenter has been created.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  include/hw/ppc/spapr_irq.h |  4 ++++
>  include/hw/ppc/xics.h      |  1 +
>  include/hw/ppc/xive.h      |  1 +
>  hw/intc/spapr_xive.c       |  8 ++++++++
>  hw/intc/xics.c             |  5 +++++
>  hw/intc/xics_spapr.c       |  8 ++++++++
>  hw/intc/xive.c             | 11 ++++++++---
>  hw/ppc/spapr_cpu_core.c    |  8 ++++++--
>  hw/ppc/spapr_irq.c         | 21 +++++++++++++++++++++
>  9 files changed, 62 insertions(+), 5 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> index 5e150a667902..78327496c102 100644
> --- a/include/hw/ppc/spapr_irq.h
> +++ b/include/hw/ppc/spapr_irq.h
> @@ -52,6 +52,8 @@ typedef struct SpaprInterruptControllerClass {
>       */
>      int (*cpu_intc_create)(SpaprInterruptController *intc,
>                              PowerPCCPU *cpu, Error **errp);
> +    int (*cpu_intc_reset)(SpaprInterruptController *intc, PowerPCCPU *cpu,
> +                          Error **errp);
>      int (*claim_irq)(SpaprInterruptController *intc, int irq, bool lsi,
>                       Error **errp);
>      void (*free_irq)(SpaprInterruptController *intc, int irq);
> @@ -68,6 +70,8 @@ void spapr_irq_update_active_intc(SpaprMachineState *spapr);
>  
>  int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
>                                PowerPCCPU *cpu, Error **errp);
> +int spapr_irq_cpu_intc_reset(SpaprMachineState *spapr,
> +                             PowerPCCPU *cpu, Error **errp);
>  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..0c3acf1a4192 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -553,6 +553,13 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
>      return 0;
>  }
>  
> +static int spapr_xive_cpu_intc_reset(SpaprInterruptController *intc,
> +                                     PowerPCCPU *cpu, Error **errp)
> +{
> +    xive_tctx_reset(spapr_cpu_state(cpu)->tctx);
> +    return 0;
> +}
> +
>  static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
>  {
>      SpaprXive *xive = SPAPR_XIVE(intc);
> @@ -697,6 +704,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..652771d6a5a5 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -295,6 +295,11 @@ static void icp_reset_handler(void *dev)
>      }
>  }
>  
> +void icp_reset(ICPState *icp)
> +{
> +    icp_reset_handler(icp);
> +}
> +
>  static void icp_realize(DeviceState *dev, Error **errp)
>  {
>      ICPState *icp = ICP(dev);
> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> index 4f64b9a9fc66..c0b2a576effe 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -346,6 +346,13 @@ static int xics_spapr_cpu_intc_create(SpaprInterruptController *intc,
>      return 0;
>  }
>  
> +static int xics_spapr_cpu_intc_reset(SpaprInterruptController *intc,
> +                                     PowerPCCPU *cpu, Error **errp)
> +{
> +    icp_reset(spapr_cpu_state(cpu)->icp);
> +    return 0;
> +}
> +
>  static int xics_spapr_claim_irq(SpaprInterruptController *intc, int irq,
>                                  bool lsi, Error **errp)
>  {
> @@ -433,6 +440,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..0ae3f9b1efe4 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -547,7 +547,7 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon)
>      }
>  }
>  
> -static void xive_tctx_reset(void *dev)
> +static void xive_tctx_reset_handler(void *dev)
>  {
>      XiveTCTX *tctx = XIVE_TCTX(dev);
>  
> @@ -568,6 +568,11 @@ static void xive_tctx_reset(void *dev)
>          ipb_to_pipr(tctx->regs[TM_QW3_HV_PHYS + TM_IPB]);
>  }
>  
> +void xive_tctx_reset(XiveTCTX *tctx)
> +{
> +    xive_tctx_reset_handler(tctx);
> +}
> +
>  static void xive_tctx_realize(DeviceState *dev, Error **errp)
>  {
>      XiveTCTX *tctx = XIVE_TCTX(dev);
> @@ -608,12 +613,12 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp)
>          }
>      }
>  
> -    qemu_register_reset(xive_tctx_reset, dev);
> +    qemu_register_reset(xive_tctx_reset_handler, dev);
>  }
>  
>  static void xive_tctx_unrealize(DeviceState *dev, Error **errp)
>  {
> -    qemu_unregister_reset(xive_tctx_reset, dev);
> +    qemu_unregister_reset(xive_tctx_reset_handler, dev);
>  }
>  
>  static int vmstate_xive_tctx_pre_save(void *opaque)
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 3e4302c7d596..416aa75e5fba 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, &error_fatal);
>  }
>  
>  void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3)
> @@ -235,12 +238,13 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *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;
>      }
>  
> +    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 bb91c61fa000..5d2b64029cd5 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -220,6 +220,27 @@ int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
>      return 0;
>  }
>  
> +int spapr_irq_cpu_intc_reset(SpaprMachineState *spapr,
> +                             PowerPCCPU *cpu, Error **errp)
> +{
> +    SpaprInterruptController *intcs[] = ALL_INTCS(spapr);
> +    int i;
> +    int rc;
> +
> +    for (i = 0; i < ARRAY_SIZE(intcs); i++) {
> +        SpaprInterruptController *intc = intcs[i];
> +        if (intc) {
> +            SpaprInterruptControllerClass *sicc = SPAPR_INTC_GET_CLASS(intc);
> +            rc = sicc->cpu_intc_reset(intc, cpu, errp);
> +            if (rc < 0) {
> +                return rc;
> +            }
> +        }
> +    }
> +
> +    return 0;
> +}
> +
>  static void spapr_set_irq(void *opaque, int irq, int level)
>  {
>      SpaprMachineState *spapr = SPAPR_MACHINE(opaque);

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 10661 bytes --]

On Fri, 18 Oct 2019 13:47:07 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Oct 17, 2019 at 04:42:40PM +0200, Cédric Le Goater wrote:
> > The interrupt presenters are not reseted today.
> 
> I don't think that's accurate.  We register reset handlers for both
> ICP and TCTX already.  We might not be resetting in quite the right
> order, but this will need a clearer description of what's changing.
> 
> Also, with this patch as is, I think we'll reset twice (once from the
> registered handler, once via the cpu).
> 

That makes three at first boot since we also reset the CPU during
realize :) But yes, if we go that way, we should probably change
the pnv machine to do the same and stop registering reset handlers.

> > Extend the sPAPR IRQ
> > backend with a new cpu_intc_reset() handler which will be called by
> > the CPU reset handler.
> > 
> > spapr_realize_vcpu() is modified to call the CPU reset only after the
> > the intc presenter has been created.
> > 
> > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > ---
> >  include/hw/ppc/spapr_irq.h |  4 ++++
> >  include/hw/ppc/xics.h      |  1 +
> >  include/hw/ppc/xive.h      |  1 +
> >  hw/intc/spapr_xive.c       |  8 ++++++++
> >  hw/intc/xics.c             |  5 +++++
> >  hw/intc/xics_spapr.c       |  8 ++++++++
> >  hw/intc/xive.c             | 11 ++++++++---
> >  hw/ppc/spapr_cpu_core.c    |  8 ++++++--
> >  hw/ppc/spapr_irq.c         | 21 +++++++++++++++++++++
> >  9 files changed, 62 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> > index 5e150a667902..78327496c102 100644
> > --- a/include/hw/ppc/spapr_irq.h
> > +++ b/include/hw/ppc/spapr_irq.h
> > @@ -52,6 +52,8 @@ typedef struct SpaprInterruptControllerClass {
> >       */
> >      int (*cpu_intc_create)(SpaprInterruptController *intc,
> >                              PowerPCCPU *cpu, Error **errp);
> > +    int (*cpu_intc_reset)(SpaprInterruptController *intc, PowerPCCPU *cpu,
> > +                          Error **errp);
> >      int (*claim_irq)(SpaprInterruptController *intc, int irq, bool lsi,
> >                       Error **errp);
> >      void (*free_irq)(SpaprInterruptController *intc, int irq);
> > @@ -68,6 +70,8 @@ void spapr_irq_update_active_intc(SpaprMachineState *spapr);
> >  
> >  int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
> >                                PowerPCCPU *cpu, Error **errp);
> > +int spapr_irq_cpu_intc_reset(SpaprMachineState *spapr,
> > +                             PowerPCCPU *cpu, Error **errp);
> >  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..0c3acf1a4192 100644
> > --- a/hw/intc/spapr_xive.c
> > +++ b/hw/intc/spapr_xive.c
> > @@ -553,6 +553,13 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
> >      return 0;
> >  }
> >  
> > +static int spapr_xive_cpu_intc_reset(SpaprInterruptController *intc,
> > +                                     PowerPCCPU *cpu, Error **errp)
> > +{
> > +    xive_tctx_reset(spapr_cpu_state(cpu)->tctx);
> > +    return 0;
> > +}
> > +
> >  static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
> >  {
> >      SpaprXive *xive = SPAPR_XIVE(intc);
> > @@ -697,6 +704,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..652771d6a5a5 100644
> > --- a/hw/intc/xics.c
> > +++ b/hw/intc/xics.c
> > @@ -295,6 +295,11 @@ static void icp_reset_handler(void *dev)
> >      }
> >  }
> >  
> > +void icp_reset(ICPState *icp)
> > +{
> > +    icp_reset_handler(icp);
> > +}
> > +
> >  static void icp_realize(DeviceState *dev, Error **errp)
> >  {
> >      ICPState *icp = ICP(dev);
> > diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> > index 4f64b9a9fc66..c0b2a576effe 100644
> > --- a/hw/intc/xics_spapr.c
> > +++ b/hw/intc/xics_spapr.c
> > @@ -346,6 +346,13 @@ static int xics_spapr_cpu_intc_create(SpaprInterruptController *intc,
> >      return 0;
> >  }
> >  
> > +static int xics_spapr_cpu_intc_reset(SpaprInterruptController *intc,
> > +                                     PowerPCCPU *cpu, Error **errp)
> > +{
> > +    icp_reset(spapr_cpu_state(cpu)->icp);
> > +    return 0;
> > +}
> > +
> >  static int xics_spapr_claim_irq(SpaprInterruptController *intc, int irq,
> >                                  bool lsi, Error **errp)
> >  {
> > @@ -433,6 +440,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..0ae3f9b1efe4 100644
> > --- a/hw/intc/xive.c
> > +++ b/hw/intc/xive.c
> > @@ -547,7 +547,7 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon)
> >      }
> >  }
> >  
> > -static void xive_tctx_reset(void *dev)
> > +static void xive_tctx_reset_handler(void *dev)
> >  {
> >      XiveTCTX *tctx = XIVE_TCTX(dev);
> >  
> > @@ -568,6 +568,11 @@ static void xive_tctx_reset(void *dev)
> >          ipb_to_pipr(tctx->regs[TM_QW3_HV_PHYS + TM_IPB]);
> >  }
> >  
> > +void xive_tctx_reset(XiveTCTX *tctx)
> > +{
> > +    xive_tctx_reset_handler(tctx);
> > +}
> > +
> >  static void xive_tctx_realize(DeviceState *dev, Error **errp)
> >  {
> >      XiveTCTX *tctx = XIVE_TCTX(dev);
> > @@ -608,12 +613,12 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp)
> >          }
> >      }
> >  
> > -    qemu_register_reset(xive_tctx_reset, dev);
> > +    qemu_register_reset(xive_tctx_reset_handler, dev);
> >  }
> >  
> >  static void xive_tctx_unrealize(DeviceState *dev, Error **errp)
> >  {
> > -    qemu_unregister_reset(xive_tctx_reset, dev);
> > +    qemu_unregister_reset(xive_tctx_reset_handler, dev);
> >  }
> >  
> >  static int vmstate_xive_tctx_pre_save(void *opaque)
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 3e4302c7d596..416aa75e5fba 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, &error_fatal);
> >  }
> >  
> >  void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3)
> > @@ -235,12 +238,13 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *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;
> >      }
> >  
> > +    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 bb91c61fa000..5d2b64029cd5 100644
> > --- a/hw/ppc/spapr_irq.c
> > +++ b/hw/ppc/spapr_irq.c
> > @@ -220,6 +220,27 @@ int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
> >      return 0;
> >  }
> >  
> > +int spapr_irq_cpu_intc_reset(SpaprMachineState *spapr,
> > +                             PowerPCCPU *cpu, Error **errp)
> > +{
> > +    SpaprInterruptController *intcs[] = ALL_INTCS(spapr);
> > +    int i;
> > +    int rc;
> > +
> > +    for (i = 0; i < ARRAY_SIZE(intcs); i++) {
> > +        SpaprInterruptController *intc = intcs[i];
> > +        if (intc) {
> > +            SpaprInterruptControllerClass *sicc = SPAPR_INTC_GET_CLASS(intc);
> > +            rc = sicc->cpu_intc_reset(intc, cpu, errp);
> > +            if (rc < 0) {
> > +                return rc;
> > +            }
> > +        }
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >  static void spapr_set_irq(void *opaque, int irq, int level)
> >  {
> >      SpaprMachineState *spapr = SPAPR_MACHINE(opaque);
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Cédric Le Goater]

On 18/10/2019 04:47, David Gibson wrote:
> On Thu, Oct 17, 2019 at 04:42:40PM +0200, Cédric Le Goater wrote:
>> The interrupt presenters are not reseted today.
> 
> I don't think that's accurate.  We register reset handlers for both
> ICP and TCTX already.  We might not be resetting in quite the right
> order, but this will need a clearer description of what's changing.
> 
> Also, with this patch as is, I think we'll reset twice (once from the
> registered handler, once via the cpu).

Unless the CPU is hotplugged, in which case it is only called once
in the realize handler ...

C. 

> 
>> Extend the sPAPR IRQ
>> backend with a new cpu_intc_reset() handler which will be called by
>> the CPU reset handler.
>>
>> spapr_realize_vcpu() is modified to call the CPU reset only after the
>> the intc presenter has been created.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  include/hw/ppc/spapr_irq.h |  4 ++++
>>  include/hw/ppc/xics.h      |  1 +
>>  include/hw/ppc/xive.h      |  1 +
>>  hw/intc/spapr_xive.c       |  8 ++++++++
>>  hw/intc/xics.c             |  5 +++++
>>  hw/intc/xics_spapr.c       |  8 ++++++++
>>  hw/intc/xive.c             | 11 ++++++++---
>>  hw/ppc/spapr_cpu_core.c    |  8 ++++++--
>>  hw/ppc/spapr_irq.c         | 21 +++++++++++++++++++++
>>  9 files changed, 62 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
>> index 5e150a667902..78327496c102 100644
>> --- a/include/hw/ppc/spapr_irq.h
>> +++ b/include/hw/ppc/spapr_irq.h
>> @@ -52,6 +52,8 @@ typedef struct SpaprInterruptControllerClass {
>>       */
>>      int (*cpu_intc_create)(SpaprInterruptController *intc,
>>                              PowerPCCPU *cpu, Error **errp);
>> +    int (*cpu_intc_reset)(SpaprInterruptController *intc, PowerPCCPU *cpu,
>> +                          Error **errp);
>>      int (*claim_irq)(SpaprInterruptController *intc, int irq, bool lsi,
>>                       Error **errp);
>>      void (*free_irq)(SpaprInterruptController *intc, int irq);
>> @@ -68,6 +70,8 @@ void spapr_irq_update_active_intc(SpaprMachineState *spapr);
>>  
>>  int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
>>                                PowerPCCPU *cpu, Error **errp);
>> +int spapr_irq_cpu_intc_reset(SpaprMachineState *spapr,
>> +                             PowerPCCPU *cpu, Error **errp);
>>  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..0c3acf1a4192 100644
>> --- a/hw/intc/spapr_xive.c
>> +++ b/hw/intc/spapr_xive.c
>> @@ -553,6 +553,13 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
>>      return 0;
>>  }
>>  
>> +static int spapr_xive_cpu_intc_reset(SpaprInterruptController *intc,
>> +                                     PowerPCCPU *cpu, Error **errp)
>> +{
>> +    xive_tctx_reset(spapr_cpu_state(cpu)->tctx);
>> +    return 0;
>> +}
>> +
>>  static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
>>  {
>>      SpaprXive *xive = SPAPR_XIVE(intc);
>> @@ -697,6 +704,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..652771d6a5a5 100644
>> --- a/hw/intc/xics.c
>> +++ b/hw/intc/xics.c
>> @@ -295,6 +295,11 @@ static void icp_reset_handler(void *dev)
>>      }
>>  }
>>  
>> +void icp_reset(ICPState *icp)
>> +{
>> +    icp_reset_handler(icp);
>> +}
>> +
>>  static void icp_realize(DeviceState *dev, Error **errp)
>>  {
>>      ICPState *icp = ICP(dev);
>> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
>> index 4f64b9a9fc66..c0b2a576effe 100644
>> --- a/hw/intc/xics_spapr.c
>> +++ b/hw/intc/xics_spapr.c
>> @@ -346,6 +346,13 @@ static int xics_spapr_cpu_intc_create(SpaprInterruptController *intc,
>>      return 0;
>>  }
>>  
>> +static int xics_spapr_cpu_intc_reset(SpaprInterruptController *intc,
>> +                                     PowerPCCPU *cpu, Error **errp)
>> +{
>> +    icp_reset(spapr_cpu_state(cpu)->icp);
>> +    return 0;
>> +}
>> +
>>  static int xics_spapr_claim_irq(SpaprInterruptController *intc, int irq,
>>                                  bool lsi, Error **errp)
>>  {
>> @@ -433,6 +440,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..0ae3f9b1efe4 100644
>> --- a/hw/intc/xive.c
>> +++ b/hw/intc/xive.c
>> @@ -547,7 +547,7 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon)
>>      }
>>  }
>>  
>> -static void xive_tctx_reset(void *dev)
>> +static void xive_tctx_reset_handler(void *dev)
>>  {
>>      XiveTCTX *tctx = XIVE_TCTX(dev);
>>  
>> @@ -568,6 +568,11 @@ static void xive_tctx_reset(void *dev)
>>          ipb_to_pipr(tctx->regs[TM_QW3_HV_PHYS + TM_IPB]);
>>  }
>>  
>> +void xive_tctx_reset(XiveTCTX *tctx)
>> +{
>> +    xive_tctx_reset_handler(tctx);
>> +}
>> +
>>  static void xive_tctx_realize(DeviceState *dev, Error **errp)
>>  {
>>      XiveTCTX *tctx = XIVE_TCTX(dev);
>> @@ -608,12 +613,12 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp)
>>          }
>>      }
>>  
>> -    qemu_register_reset(xive_tctx_reset, dev);
>> +    qemu_register_reset(xive_tctx_reset_handler, dev);
>>  }
>>  
>>  static void xive_tctx_unrealize(DeviceState *dev, Error **errp)
>>  {
>> -    qemu_unregister_reset(xive_tctx_reset, dev);
>> +    qemu_unregister_reset(xive_tctx_reset_handler, dev);
>>  }
>>  
>>  static int vmstate_xive_tctx_pre_save(void *opaque)
>> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
>> index 3e4302c7d596..416aa75e5fba 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, &error_fatal);
>>  }
>>  
>>  void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3)
>> @@ -235,12 +238,13 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *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;
>>      }
>>  
>> +    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 bb91c61fa000..5d2b64029cd5 100644
>> --- a/hw/ppc/spapr_irq.c
>> +++ b/hw/ppc/spapr_irq.c
>> @@ -220,6 +220,27 @@ int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
>>      return 0;
>>  }
>>  
>> +int spapr_irq_cpu_intc_reset(SpaprMachineState *spapr,
>> +                             PowerPCCPU *cpu, Error **errp)
>> +{
>> +    SpaprInterruptController *intcs[] = ALL_INTCS(spapr);
>> +    int i;
>> +    int rc;
>> +
>> +    for (i = 0; i < ARRAY_SIZE(intcs); i++) {
>> +        SpaprInterruptController *intc = intcs[i];
>> +        if (intc) {
>> +            SpaprInterruptControllerClass *sicc = SPAPR_INTC_GET_CLASS(intc);
>> +            rc = sicc->cpu_intc_reset(intc, cpu, errp);
>> +            if (rc < 0) {
>> +                return rc;
>> +            }
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>  static void spapr_set_irq(void *opaque, int irq, int level)
>>  {
>>      SpaprMachineState *spapr = SPAPR_MACHINE(opaque);
> 

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

[Cédric Le Goater]

On 17/10/2019 16:42, Cédric Le Goater wrote:
> The interrupt presenters are not reseted today.

I should have added : 

... when CPU are hot-plugged.

> Extend the sPAPR IRQ
> backend with a new cpu_intc_reset() handler which will be called by
> the CPU reset handler.
> 
> spapr_realize_vcpu() is modified to call the CPU reset only after the
> the intc presenter has been created.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  include/hw/ppc/spapr_irq.h |  4 ++++
>  include/hw/ppc/xics.h      |  1 +
>  include/hw/ppc/xive.h      |  1 +
>  hw/intc/spapr_xive.c       |  8 ++++++++
>  hw/intc/xics.c             |  5 +++++
>  hw/intc/xics_spapr.c       |  8 ++++++++
>  hw/intc/xive.c             | 11 ++++++++---
>  hw/ppc/spapr_cpu_core.c    |  8 ++++++--
>  hw/ppc/spapr_irq.c         | 21 +++++++++++++++++++++
>  9 files changed, 62 insertions(+), 5 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> index 5e150a667902..78327496c102 100644
> --- a/include/hw/ppc/spapr_irq.h
> +++ b/include/hw/ppc/spapr_irq.h
> @@ -52,6 +52,8 @@ typedef struct SpaprInterruptControllerClass {
>       */
>      int (*cpu_intc_create)(SpaprInterruptController *intc,
>                              PowerPCCPU *cpu, Error **errp);
> +    int (*cpu_intc_reset)(SpaprInterruptController *intc, PowerPCCPU *cpu,
> +                          Error **errp);
>      int (*claim_irq)(SpaprInterruptController *intc, int irq, bool lsi,
>                       Error **errp);
>      void (*free_irq)(SpaprInterruptController *intc, int irq);
> @@ -68,6 +70,8 @@ void spapr_irq_update_active_intc(SpaprMachineState *spapr);
>  
>  int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
>                                PowerPCCPU *cpu, Error **errp);
> +int spapr_irq_cpu_intc_reset(SpaprMachineState *spapr,
> +                             PowerPCCPU *cpu, Error **errp);
>  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..0c3acf1a4192 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -553,6 +553,13 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
>      return 0;
>  }
>  
> +static int spapr_xive_cpu_intc_reset(SpaprInterruptController *intc,
> +                                     PowerPCCPU *cpu, Error **errp)
> +{
> +    xive_tctx_reset(spapr_cpu_state(cpu)->tctx);
> +    return 0;
> +}
> +
>  static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
>  {
>      SpaprXive *xive = SPAPR_XIVE(intc);
> @@ -697,6 +704,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..652771d6a5a5 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -295,6 +295,11 @@ static void icp_reset_handler(void *dev)
>      }
>  }
>  
> +void icp_reset(ICPState *icp)
> +{
> +    icp_reset_handler(icp);
> +}
> +
>  static void icp_realize(DeviceState *dev, Error **errp)
>  {
>      ICPState *icp = ICP(dev);
> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> index 4f64b9a9fc66..c0b2a576effe 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -346,6 +346,13 @@ static int xics_spapr_cpu_intc_create(SpaprInterruptController *intc,
>      return 0;
>  }
>  
> +static int xics_spapr_cpu_intc_reset(SpaprInterruptController *intc,
> +                                     PowerPCCPU *cpu, Error **errp)
> +{
> +    icp_reset(spapr_cpu_state(cpu)->icp);
> +    return 0;
> +}
> +
>  static int xics_spapr_claim_irq(SpaprInterruptController *intc, int irq,
>                                  bool lsi, Error **errp)
>  {
> @@ -433,6 +440,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..0ae3f9b1efe4 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -547,7 +547,7 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon)
>      }
>  }
>  
> -static void xive_tctx_reset(void *dev)
> +static void xive_tctx_reset_handler(void *dev)
>  {
>      XiveTCTX *tctx = XIVE_TCTX(dev);
>  
> @@ -568,6 +568,11 @@ static void xive_tctx_reset(void *dev)
>          ipb_to_pipr(tctx->regs[TM_QW3_HV_PHYS + TM_IPB]);
>  }
>  
> +void xive_tctx_reset(XiveTCTX *tctx)
> +{
> +    xive_tctx_reset_handler(tctx);
> +}
> +
>  static void xive_tctx_realize(DeviceState *dev, Error **errp)
>  {
>      XiveTCTX *tctx = XIVE_TCTX(dev);
> @@ -608,12 +613,12 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp)
>          }
>      }
>  
> -    qemu_register_reset(xive_tctx_reset, dev);
> +    qemu_register_reset(xive_tctx_reset_handler, dev);
>  }
>  
>  static void xive_tctx_unrealize(DeviceState *dev, Error **errp)
>  {
> -    qemu_unregister_reset(xive_tctx_reset, dev);
> +    qemu_unregister_reset(xive_tctx_reset_handler, dev);
>  }
>  
>  static int vmstate_xive_tctx_pre_save(void *opaque)
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 3e4302c7d596..416aa75e5fba 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, &error_fatal);
>  }
>  
>  void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3)
> @@ -235,12 +238,13 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *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;
>      }
>  
> +    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 bb91c61fa000..5d2b64029cd5 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -220,6 +220,27 @@ int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
>      return 0;
>  }
>  
> +int spapr_irq_cpu_intc_reset(SpaprMachineState *spapr,
> +                             PowerPCCPU *cpu, Error **errp)
> +{
> +    SpaprInterruptController *intcs[] = ALL_INTCS(spapr);
> +    int i;
> +    int rc;
> +
> +    for (i = 0; i < ARRAY_SIZE(intcs); i++) {
> +        SpaprInterruptController *intc = intcs[i];
> +        if (intc) {
> +            SpaprInterruptControllerClass *sicc = SPAPR_INTC_GET_CLASS(intc);
> +            rc = sicc->cpu_intc_reset(intc, cpu, errp);
> +            if (rc < 0) {
> +                return rc;
> +            }
> +        }
> +    }
> +
> +    return 0;
> +}
> +
>  static void spapr_set_irq(void *opaque, int irq, int level)
>  {
>      SpaprMachineState *spapr = SPAPR_MACHINE(opaque);
> 

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

[Greg Kurz]

On Thu, 17 Oct 2019 16:42:40 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> The interrupt presenters are not reseted today. Extend the sPAPR IRQ
> backend with a new cpu_intc_reset() handler which will be called by
> the CPU reset handler.
> 
> spapr_realize_vcpu() is modified to call the CPU reset only after the
> the intc presenter has been created.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  include/hw/ppc/spapr_irq.h |  4 ++++
>  include/hw/ppc/xics.h      |  1 +
>  include/hw/ppc/xive.h      |  1 +
>  hw/intc/spapr_xive.c       |  8 ++++++++
>  hw/intc/xics.c             |  5 +++++
>  hw/intc/xics_spapr.c       |  8 ++++++++
>  hw/intc/xive.c             | 11 ++++++++---
>  hw/ppc/spapr_cpu_core.c    |  8 ++++++--
>  hw/ppc/spapr_irq.c         | 21 +++++++++++++++++++++
>  9 files changed, 62 insertions(+), 5 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> index 5e150a667902..78327496c102 100644
> --- a/include/hw/ppc/spapr_irq.h
> +++ b/include/hw/ppc/spapr_irq.h
> @@ -52,6 +52,8 @@ typedef struct SpaprInterruptControllerClass {
>       */
>      int (*cpu_intc_create)(SpaprInterruptController *intc,
>                              PowerPCCPU *cpu, Error **errp);
> +    int (*cpu_intc_reset)(SpaprInterruptController *intc, PowerPCCPU *cpu,
> +                          Error **errp);

Looking at the rest of the patch, it seems that we don't need error
reporting. I suggest you make this void and drop the errp parameter.

>      int (*claim_irq)(SpaprInterruptController *intc, int irq, bool lsi,
>                       Error **errp);
>      void (*free_irq)(SpaprInterruptController *intc, int irq);
> @@ -68,6 +70,8 @@ void spapr_irq_update_active_intc(SpaprMachineState *spapr);
>  
>  int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
>                                PowerPCCPU *cpu, Error **errp);
> +int spapr_irq_cpu_intc_reset(SpaprMachineState *spapr,
> +                             PowerPCCPU *cpu, Error **errp);
>  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..0c3acf1a4192 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -553,6 +553,13 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
>      return 0;
>  }
>  
> +static int spapr_xive_cpu_intc_reset(SpaprInterruptController *intc,
> +                                     PowerPCCPU *cpu, Error **errp)
> +{
> +    xive_tctx_reset(spapr_cpu_state(cpu)->tctx);
> +    return 0;
> +}
> +
>  static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
>  {
>      SpaprXive *xive = SPAPR_XIVE(intc);
> @@ -697,6 +704,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..652771d6a5a5 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -295,6 +295,11 @@ static void icp_reset_handler(void *dev)
>      }
>  }
>  
> +void icp_reset(ICPState *icp)
> +{
> +    icp_reset_handler(icp);
> +}
> +
>  static void icp_realize(DeviceState *dev, Error **errp)
>  {
>      ICPState *icp = ICP(dev);
> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> index 4f64b9a9fc66..c0b2a576effe 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -346,6 +346,13 @@ static int xics_spapr_cpu_intc_create(SpaprInterruptController *intc,
>      return 0;
>  }
>  
> +static int xics_spapr_cpu_intc_reset(SpaprInterruptController *intc,
> +                                     PowerPCCPU *cpu, Error **errp)
> +{
> +    icp_reset(spapr_cpu_state(cpu)->icp);
> +    return 0;
> +}
> +
>  static int xics_spapr_claim_irq(SpaprInterruptController *intc, int irq,
>                                  bool lsi, Error **errp)
>  {
> @@ -433,6 +440,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..0ae3f9b1efe4 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -547,7 +547,7 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon)
>      }
>  }
>  
> -static void xive_tctx_reset(void *dev)
> +static void xive_tctx_reset_handler(void *dev)
>  {
>      XiveTCTX *tctx = XIVE_TCTX(dev);
>  
> @@ -568,6 +568,11 @@ static void xive_tctx_reset(void *dev)
>          ipb_to_pipr(tctx->regs[TM_QW3_HV_PHYS + TM_IPB]);
>  }
>  
> +void xive_tctx_reset(XiveTCTX *tctx)
> +{
> +    xive_tctx_reset_handler(tctx);
> +}
> +
>  static void xive_tctx_realize(DeviceState *dev, Error **errp)
>  {
>      XiveTCTX *tctx = XIVE_TCTX(dev);
> @@ -608,12 +613,12 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp)
>          }
>      }
>  
> -    qemu_register_reset(xive_tctx_reset, dev);
> +    qemu_register_reset(xive_tctx_reset_handler, dev);
>  }
>  
>  static void xive_tctx_unrealize(DeviceState *dev, Error **errp)
>  {
> -    qemu_unregister_reset(xive_tctx_reset, dev);
> +    qemu_unregister_reset(xive_tctx_reset_handler, dev);
>  }
>  
>  static int vmstate_xive_tctx_pre_save(void *opaque)
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 3e4302c7d596..416aa75e5fba 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, &error_fatal);

So now, interrupt presenters are reset twice: from here and
from qemu_devices_reset(). Not a big deal, but if you add
something equivalent in the pnv machine, then the presenters
no longer need to call qemu_register_reset() at all.

>  }
>  
>  void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3)
> @@ -235,12 +238,13 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *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;
>      }
>  
> +    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 bb91c61fa000..5d2b64029cd5 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -220,6 +220,27 @@ int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
>      return 0;
>  }
>  
> +int spapr_irq_cpu_intc_reset(SpaprMachineState *spapr,
> +                             PowerPCCPU *cpu, Error **errp)
> +{
> +    SpaprInterruptController *intcs[] = ALL_INTCS(spapr);
> +    int i;
> +    int rc;
> +
> +    for (i = 0; i < ARRAY_SIZE(intcs); i++) {
> +        SpaprInterruptController *intc = intcs[i];
> +        if (intc) {
> +            SpaprInterruptControllerClass *sicc = SPAPR_INTC_GET_CLASS(intc);
> +            rc = sicc->cpu_intc_reset(intc, cpu, errp);
> +            if (rc < 0) {
> +                return rc;
> +            }
> +        }
> +    }
> +
> +    return 0;
> +}
> +
>  static void spapr_set_irq(void *opaque, int irq, int level)
>  {
>      SpaprMachineState *spapr = SPAPR_MACHINE(opaque);

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

[Cédric Le Goater]

On 18/10/2019 09:46, Greg Kurz wrote:
> On Thu, 17 Oct 2019 16:42:40 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
> 
>> The interrupt presenters are not reseted today. Extend the sPAPR IRQ
>> backend with a new cpu_intc_reset() handler which will be called by
>> the CPU reset handler.
>>
>> spapr_realize_vcpu() is modified to call the CPU reset only after the
>> the intc presenter has been created.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  include/hw/ppc/spapr_irq.h |  4 ++++
>>  include/hw/ppc/xics.h      |  1 +
>>  include/hw/ppc/xive.h      |  1 +
>>  hw/intc/spapr_xive.c       |  8 ++++++++
>>  hw/intc/xics.c             |  5 +++++
>>  hw/intc/xics_spapr.c       |  8 ++++++++
>>  hw/intc/xive.c             | 11 ++++++++---
>>  hw/ppc/spapr_cpu_core.c    |  8 ++++++--
>>  hw/ppc/spapr_irq.c         | 21 +++++++++++++++++++++
>>  9 files changed, 62 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
>> index 5e150a667902..78327496c102 100644
>> --- a/include/hw/ppc/spapr_irq.h
>> +++ b/include/hw/ppc/spapr_irq.h
>> @@ -52,6 +52,8 @@ typedef struct SpaprInterruptControllerClass {
>>       */
>>      int (*cpu_intc_create)(SpaprInterruptController *intc,
>>                              PowerPCCPU *cpu, Error **errp);
>> +    int (*cpu_intc_reset)(SpaprInterruptController *intc, PowerPCCPU *cpu,
>> +                          Error **errp);
> 
> Looking at the rest of the patch, it seems that we don't need error
> reporting. I suggest you make this void and drop the errp parameter.

yes. we can drop the error on that path.

> 
>>      int (*claim_irq)(SpaprInterruptController *intc, int irq, bool lsi,
>>                       Error **errp);
>>      void (*free_irq)(SpaprInterruptController *intc, int irq);
>> @@ -68,6 +70,8 @@ void spapr_irq_update_active_intc(SpaprMachineState *spapr);
>>  
>>  int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
>>                                PowerPCCPU *cpu, Error **errp);
>> +int spapr_irq_cpu_intc_reset(SpaprMachineState *spapr,
>> +                             PowerPCCPU *cpu, Error **errp);
>>  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..0c3acf1a4192 100644
>> --- a/hw/intc/spapr_xive.c
>> +++ b/hw/intc/spapr_xive.c
>> @@ -553,6 +553,13 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
>>      return 0;
>>  }
>>  
>> +static int spapr_xive_cpu_intc_reset(SpaprInterruptController *intc,
>> +                                     PowerPCCPU *cpu, Error **errp)
>> +{
>> +    xive_tctx_reset(spapr_cpu_state(cpu)->tctx);
>> +    return 0;
>> +}
>> +
>>  static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
>>  {
>>      SpaprXive *xive = SPAPR_XIVE(intc);
>> @@ -697,6 +704,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..652771d6a5a5 100644
>> --- a/hw/intc/xics.c
>> +++ b/hw/intc/xics.c
>> @@ -295,6 +295,11 @@ static void icp_reset_handler(void *dev)
>>      }
>>  }
>>  
>> +void icp_reset(ICPState *icp)
>> +{
>> +    icp_reset_handler(icp);
>> +}
>> +
>>  static void icp_realize(DeviceState *dev, Error **errp)
>>  {
>>      ICPState *icp = ICP(dev);
>> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
>> index 4f64b9a9fc66..c0b2a576effe 100644
>> --- a/hw/intc/xics_spapr.c
>> +++ b/hw/intc/xics_spapr.c
>> @@ -346,6 +346,13 @@ static int xics_spapr_cpu_intc_create(SpaprInterruptController *intc,
>>      return 0;
>>  }
>>  
>> +static int xics_spapr_cpu_intc_reset(SpaprInterruptController *intc,
>> +                                     PowerPCCPU *cpu, Error **errp)
>> +{
>> +    icp_reset(spapr_cpu_state(cpu)->icp);
>> +    return 0;
>> +}
>> +
>>  static int xics_spapr_claim_irq(SpaprInterruptController *intc, int irq,
>>                                  bool lsi, Error **errp)
>>  {
>> @@ -433,6 +440,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..0ae3f9b1efe4 100644
>> --- a/hw/intc/xive.c
>> +++ b/hw/intc/xive.c
>> @@ -547,7 +547,7 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon)
>>      }
>>  }
>>  
>> -static void xive_tctx_reset(void *dev)
>> +static void xive_tctx_reset_handler(void *dev)
>>  {
>>      XiveTCTX *tctx = XIVE_TCTX(dev);
>>  
>> @@ -568,6 +568,11 @@ static void xive_tctx_reset(void *dev)
>>          ipb_to_pipr(tctx->regs[TM_QW3_HV_PHYS + TM_IPB]);
>>  }
>>  
>> +void xive_tctx_reset(XiveTCTX *tctx)
>> +{
>> +    xive_tctx_reset_handler(tctx);
>> +}
>> +
>>  static void xive_tctx_realize(DeviceState *dev, Error **errp)
>>  {
>>      XiveTCTX *tctx = XIVE_TCTX(dev);
>> @@ -608,12 +613,12 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp)
>>          }
>>      }
>>  
>> -    qemu_register_reset(xive_tctx_reset, dev);
>> +    qemu_register_reset(xive_tctx_reset_handler, dev);
>>  }
>>  
>>  static void xive_tctx_unrealize(DeviceState *dev, Error **errp)
>>  {
>> -    qemu_unregister_reset(xive_tctx_reset, dev);
>> +    qemu_unregister_reset(xive_tctx_reset_handler, dev);
>>  }
>>  
>>  static int vmstate_xive_tctx_pre_save(void *opaque)
>> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
>> index 3e4302c7d596..416aa75e5fba 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, &error_fatal);
> 
> So now, interrupt presenters are reset twice: from here and
> from qemu_devices_reset(). Not a big deal, but if you add
> something equivalent in the pnv machine, then the presenters
> no longer need to call qemu_register_reset() at all.

indeed. We could remove the reset handler in the presenter model.


>>  }
>>  
>>  void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3)
>> @@ -235,12 +238,13 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *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;
>>      }
>>  
>> +    spapr_cpu_reset(cpu);
>> +


We need to fix that reset one day. I will add a comment to say it is for
hotplug.

C.


>>      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 bb91c61fa000..5d2b64029cd5 100644
>> --- a/hw/ppc/spapr_irq.c
>> +++ b/hw/ppc/spapr_irq.c
>> @@ -220,6 +220,27 @@ int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
>>      return 0;
>>  }
>>  
>> +int spapr_irq_cpu_intc_reset(SpaprMachineState *spapr,
>> +                             PowerPCCPU *cpu, Error **errp)
>> +{
>> +    SpaprInterruptController *intcs[] = ALL_INTCS(spapr);
>> +    int i;
>> +    int rc;
>> +
>> +    for (i = 0; i < ARRAY_SIZE(intcs); i++) {
>> +        SpaprInterruptController *intc = intcs[i];
>> +        if (intc) {
>> +            SpaprInterruptControllerClass *sicc = SPAPR_INTC_GET_CLASS(intc);
>> +            rc = sicc->cpu_intc_reset(intc, cpu, errp);
>> +            if (rc < 0) {
>> +                return rc;
>> +            }
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>  static void spapr_set_irq(void *opaque, int irq, int level)
>>  {
>>      SpaprMachineState *spapr = SPAPR_MACHINE(opaque);
> 

[PATCH 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 in
TCG or kernel_irqchip=off, QEMU needs to emulate the same behavior.

Introduce a 'os-cam' property which will be used to set the OS CAM
line at reset and remove the spapr_xive_set_tctx_os_cam() calls which
are done when the XIVE interrupt controller are activated.

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 -
 include/hw/ppc/xive.h       |  4 +++-
 hw/intc/spapr_xive.c        | 31 +++++--------------------------
 hw/intc/xive.c              | 22 +++++++++++++++++++++-
 hw/ppc/pnv.c                |  3 ++-
 5 files changed, 31 insertions(+), 30 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/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
index 99381639f50c..e273069c25a9 100644
--- a/include/hw/ppc/xive.h
+++ b/include/hw/ppc/xive.h
@@ -319,6 +319,7 @@ typedef struct XiveTCTX {
     qemu_irq    os_output;
 
     uint8_t     regs[XIVE_TM_RING_COUNT * XIVE_TM_RING_SIZE];
+    uint32_t    os_cam;
 } XiveTCTX;
 
 /*
@@ -414,7 +415,8 @@ void xive_tctx_tm_write(XiveTCTX *tctx, hwaddr offset, uint64_t value,
 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);
+Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, uint32_t os_cam,
+                         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 0c3acf1a4192..71f138512a1c 100644
--- a/hw/intc/spapr_xive.c
+++ b/hw/intc/spapr_xive.c
@@ -205,21 +205,13 @@ 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)
+static uint32_t spapr_xive_get_os_cam(PowerPCCPU *cpu)
 {
     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);
+    spapr_xive_cpu_to_nvt(cpu, &nvt_blk, &nvt_idx);
+    return xive_nvt_cam_line(nvt_blk, nvt_idx);
 }
 
 static void spapr_xive_end_reset(XiveEND *end)
@@ -537,19 +529,14 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
     SpaprXive *xive = SPAPR_XIVE(intc);
     Object *obj;
     SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
+    uint32_t os_cam = spapr_xive_get_os_cam(cpu);
 
-    obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(xive), errp);
+    obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(xive), os_cam, errp);
     if (!obj) {
         return -1;
     }
 
     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;
 }
 
@@ -650,14 +637,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);
diff --git a/hw/intc/xive.c b/hw/intc/xive.c
index 0ae3f9b1efe4..be4f2c974178 100644
--- a/hw/intc/xive.c
+++ b/hw/intc/xive.c
@@ -566,6 +566,18 @@ static void xive_tctx_reset_handler(void *dev)
         ipb_to_pipr(tctx->regs[TM_QW1_OS + TM_IPB]);
     tctx->regs[TM_QW3_HV_PHYS + TM_PIPR] =
         ipb_to_pipr(tctx->regs[TM_QW3_HV_PHYS + TM_IPB]);
+
+    /*
+     * (TCG) Set the OS CAM line of the thread interrupt context.
+     *
+     * 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.
+     */
+    if (tctx->os_cam) {
+        uint32_t qw1w2 = cpu_to_be32(TM_QW1W2_VO | tctx->os_cam);
+        memcpy(&tctx->regs[TM_QW1_OS + TM_WORD2], &qw1w2, 4);
+    }
 }
 
 void xive_tctx_reset(XiveTCTX *tctx)
@@ -667,11 +679,17 @@ static const VMStateDescription vmstate_xive_tctx = {
     },
 };
 
+static Property  xive_tctx_properties[] = {
+    DEFINE_PROP_UINT32("os-cam", XiveTCTX, os_cam, 0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void xive_tctx_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->desc = "XIVE Interrupt Thread Context";
+    dc->props = xive_tctx_properties;
     dc->realize = xive_tctx_realize;
     dc->unrealize = xive_tctx_unrealize;
     dc->vmsd = &vmstate_xive_tctx;
@@ -689,7 +707,8 @@ static const TypeInfo xive_tctx_info = {
     .class_init    = xive_tctx_class_init,
 };
 
-Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp)
+Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, uint32_t os_cam,
+                         Error **errp)
 {
     Error *local_err = NULL;
     Object *obj;
@@ -698,6 +717,7 @@ Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp)
     object_property_add_child(cpu, TYPE_XIVE_TCTX, obj, &error_abort);
     object_unref(obj);
     object_property_add_const_link(obj, "cpu", cpu, &error_abort);
+    object_property_set_int(obj, os_cam, "os-cam", &local_err);
     object_property_set_bool(obj, true, "realized", &local_err);
     if (local_err) {
         goto error;
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 7cf64b6d2533..99c06842573e 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -806,7 +806,8 @@ static void pnv_chip_power9_intc_create(PnvChip *chip, PowerPCCPU *cpu,
      * controller object is initialized afterwards. Hopefully, it's
      * only used at runtime.
      */
-    obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(&chip9->xive), &local_err);
+    obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(&chip9->xive), 0,
+                           &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
-- 
2.21.0

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 8459 bytes --]

On Thu, Oct 17, 2019 at 04:42:41PM +0200, Cédric Le Goater 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 in
> TCG or kernel_irqchip=off, QEMU needs to emulate the same behavior.
> 
> Introduce a 'os-cam' property which will be used to set the OS CAM
> line at reset and remove the spapr_xive_set_tctx_os_cam() calls which
> are done when the XIVE interrupt controller are activated.

I'm not immediately seeing the advantage of doing this via a property,
rather than poking it from the PAPR code which already knows the right
values.

Also, let me check my understanding:
  IIUC, on powernv the OS (running in HV mode) can alter the OS CAM
  lines for itself and/or its guests, but for pseries they're fixed in
  place.  Is that right?

> 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 -
>  include/hw/ppc/xive.h       |  4 +++-
>  hw/intc/spapr_xive.c        | 31 +++++--------------------------
>  hw/intc/xive.c              | 22 +++++++++++++++++++++-
>  hw/ppc/pnv.c                |  3 ++-
>  5 files changed, 31 insertions(+), 30 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/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> index 99381639f50c..e273069c25a9 100644
> --- a/include/hw/ppc/xive.h
> +++ b/include/hw/ppc/xive.h
> @@ -319,6 +319,7 @@ typedef struct XiveTCTX {
>      qemu_irq    os_output;
>  
>      uint8_t     regs[XIVE_TM_RING_COUNT * XIVE_TM_RING_SIZE];
> +    uint32_t    os_cam;
>  } XiveTCTX;
>  
>  /*
> @@ -414,7 +415,8 @@ void xive_tctx_tm_write(XiveTCTX *tctx, hwaddr offset, uint64_t value,
>  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);
> +Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, uint32_t os_cam,
> +                         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 0c3acf1a4192..71f138512a1c 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -205,21 +205,13 @@ 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)
> +static uint32_t spapr_xive_get_os_cam(PowerPCCPU *cpu)
>  {
>      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);
> +    spapr_xive_cpu_to_nvt(cpu, &nvt_blk, &nvt_idx);
> +    return xive_nvt_cam_line(nvt_blk, nvt_idx);
>  }
>  
>  static void spapr_xive_end_reset(XiveEND *end)
> @@ -537,19 +529,14 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
>      SpaprXive *xive = SPAPR_XIVE(intc);
>      Object *obj;
>      SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
> +    uint32_t os_cam = spapr_xive_get_os_cam(cpu);
>  
> -    obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(xive), errp);
> +    obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(xive), os_cam, errp);
>      if (!obj) {
>          return -1;
>      }
>  
>      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;
>  }
>  
> @@ -650,14 +637,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);
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index 0ae3f9b1efe4..be4f2c974178 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -566,6 +566,18 @@ static void xive_tctx_reset_handler(void *dev)
>          ipb_to_pipr(tctx->regs[TM_QW1_OS + TM_IPB]);
>      tctx->regs[TM_QW3_HV_PHYS + TM_PIPR] =
>          ipb_to_pipr(tctx->regs[TM_QW3_HV_PHYS + TM_IPB]);
> +
> +    /*
> +     * (TCG) Set the OS CAM line of the thread interrupt context.
> +     *
> +     * 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.
> +     */
> +    if (tctx->os_cam) {
> +        uint32_t qw1w2 = cpu_to_be32(TM_QW1W2_VO | tctx->os_cam);
> +        memcpy(&tctx->regs[TM_QW1_OS + TM_WORD2], &qw1w2, 4);
> +    }
>  }
>  
>  void xive_tctx_reset(XiveTCTX *tctx)
> @@ -667,11 +679,17 @@ static const VMStateDescription vmstate_xive_tctx = {
>      },
>  };
>  
> +static Property  xive_tctx_properties[] = {
> +    DEFINE_PROP_UINT32("os-cam", XiveTCTX, os_cam, 0),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  static void xive_tctx_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->desc = "XIVE Interrupt Thread Context";
> +    dc->props = xive_tctx_properties;
>      dc->realize = xive_tctx_realize;
>      dc->unrealize = xive_tctx_unrealize;
>      dc->vmsd = &vmstate_xive_tctx;
> @@ -689,7 +707,8 @@ static const TypeInfo xive_tctx_info = {
>      .class_init    = xive_tctx_class_init,
>  };
>  
> -Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp)
> +Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, uint32_t os_cam,
> +                         Error **errp)
>  {
>      Error *local_err = NULL;
>      Object *obj;
> @@ -698,6 +717,7 @@ Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp)
>      object_property_add_child(cpu, TYPE_XIVE_TCTX, obj, &error_abort);
>      object_unref(obj);
>      object_property_add_const_link(obj, "cpu", cpu, &error_abort);
> +    object_property_set_int(obj, os_cam, "os-cam", &local_err);
>      object_property_set_bool(obj, true, "realized", &local_err);
>      if (local_err) {
>          goto error;
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 7cf64b6d2533..99c06842573e 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -806,7 +806,8 @@ static void pnv_chip_power9_intc_create(PnvChip *chip, PowerPCCPU *cpu,
>       * controller object is initialized afterwards. Hopefully, it's
>       * only used at runtime.
>       */
> -    obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(&chip9->xive), &local_err);
> +    obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(&chip9->xive), 0,
> +                           &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Cédric Le Goater]

On 18/10/2019 05:55, David Gibson wrote:
> On Thu, Oct 17, 2019 at 04:42:41PM +0200, Cédric Le Goater 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 in
>> TCG or kernel_irqchip=off, QEMU needs to emulate the same behavior.
>>
>> Introduce a 'os-cam' property which will be used to set the OS CAM
>> line at reset and remove the spapr_xive_set_tctx_os_cam() calls which
>> are done when the XIVE interrupt controller are activated.
> 
> I'm not immediately seeing the advantage of doing this via a property,
> rather than poking it from the PAPR code which already knows the right
> values.

we can simplify by passing the OS CAM line value as a parameter of the 
xive_tctx_reset routine, as suggested by Greg.

> 
> Also, let me check my understanding:
>   IIUC, on powernv the OS (running in HV mode) can alter the OS CAM
>   lines for itself 

OPAL only sets the VT bit in the HW cam line.

Linux PowerNV sets the POOL CAM line.

> and/or its guests, 

KVM sets the OS CAM line when a vCPU is scheduled to run.

> but for pseries they're fixed in place.  Is that right?

QEMU emulates KVM and sets the OS CAM line to a value similar to what KVM
would use. We can consider this value a reset constant.

C.

 
>> 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 -
>>  include/hw/ppc/xive.h       |  4 +++-
>>  hw/intc/spapr_xive.c        | 31 +++++--------------------------
>>  hw/intc/xive.c              | 22 +++++++++++++++++++++-
>>  hw/ppc/pnv.c                |  3 ++-
>>  5 files changed, 31 insertions(+), 30 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/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
>> index 99381639f50c..e273069c25a9 100644
>> --- a/include/hw/ppc/xive.h
>> +++ b/include/hw/ppc/xive.h
>> @@ -319,6 +319,7 @@ typedef struct XiveTCTX {
>>      qemu_irq    os_output;
>>  
>>      uint8_t     regs[XIVE_TM_RING_COUNT * XIVE_TM_RING_SIZE];
>> +    uint32_t    os_cam;
>>  } XiveTCTX;
>>  
>>  /*
>> @@ -414,7 +415,8 @@ void xive_tctx_tm_write(XiveTCTX *tctx, hwaddr offset, uint64_t value,
>>  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);
>> +Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, uint32_t os_cam,
>> +                         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 0c3acf1a4192..71f138512a1c 100644
>> --- a/hw/intc/spapr_xive.c
>> +++ b/hw/intc/spapr_xive.c
>> @@ -205,21 +205,13 @@ 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)
>> +static uint32_t spapr_xive_get_os_cam(PowerPCCPU *cpu)
>>  {
>>      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);
>> +    spapr_xive_cpu_to_nvt(cpu, &nvt_blk, &nvt_idx);
>> +    return xive_nvt_cam_line(nvt_blk, nvt_idx);
>>  }
>>  
>>  static void spapr_xive_end_reset(XiveEND *end)
>> @@ -537,19 +529,14 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
>>      SpaprXive *xive = SPAPR_XIVE(intc);
>>      Object *obj;
>>      SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
>> +    uint32_t os_cam = spapr_xive_get_os_cam(cpu);
>>  
>> -    obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(xive), errp);
>> +    obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(xive), os_cam, errp);
>>      if (!obj) {
>>          return -1;
>>      }
>>  
>>      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;
>>  }
>>  
>> @@ -650,14 +637,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);
>> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
>> index 0ae3f9b1efe4..be4f2c974178 100644
>> --- a/hw/intc/xive.c
>> +++ b/hw/intc/xive.c
>> @@ -566,6 +566,18 @@ static void xive_tctx_reset_handler(void *dev)
>>          ipb_to_pipr(tctx->regs[TM_QW1_OS + TM_IPB]);
>>      tctx->regs[TM_QW3_HV_PHYS + TM_PIPR] =
>>          ipb_to_pipr(tctx->regs[TM_QW3_HV_PHYS + TM_IPB]);
>> +
>> +    /*
>> +     * (TCG) Set the OS CAM line of the thread interrupt context.
>> +     *
>> +     * 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.
>> +     */
>> +    if (tctx->os_cam) {
>> +        uint32_t qw1w2 = cpu_to_be32(TM_QW1W2_VO | tctx->os_cam);
>> +        memcpy(&tctx->regs[TM_QW1_OS + TM_WORD2], &qw1w2, 4);
>> +    }
>>  }
>>  
>>  void xive_tctx_reset(XiveTCTX *tctx)
>> @@ -667,11 +679,17 @@ static const VMStateDescription vmstate_xive_tctx = {
>>      },
>>  };
>>  
>> +static Property  xive_tctx_properties[] = {
>> +    DEFINE_PROP_UINT32("os-cam", XiveTCTX, os_cam, 0),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>>  static void xive_tctx_class_init(ObjectClass *klass, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>>  
>>      dc->desc = "XIVE Interrupt Thread Context";
>> +    dc->props = xive_tctx_properties;
>>      dc->realize = xive_tctx_realize;
>>      dc->unrealize = xive_tctx_unrealize;
>>      dc->vmsd = &vmstate_xive_tctx;
>> @@ -689,7 +707,8 @@ static const TypeInfo xive_tctx_info = {
>>      .class_init    = xive_tctx_class_init,
>>  };
>>  
>> -Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp)
>> +Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, uint32_t os_cam,
>> +                         Error **errp)
>>  {
>>      Error *local_err = NULL;
>>      Object *obj;
>> @@ -698,6 +717,7 @@ Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp)
>>      object_property_add_child(cpu, TYPE_XIVE_TCTX, obj, &error_abort);
>>      object_unref(obj);
>>      object_property_add_const_link(obj, "cpu", cpu, &error_abort);
>> +    object_property_set_int(obj, os_cam, "os-cam", &local_err);
>>      object_property_set_bool(obj, true, "realized", &local_err);
>>      if (local_err) {
>>          goto error;
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 7cf64b6d2533..99c06842573e 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -806,7 +806,8 @@ static void pnv_chip_power9_intc_create(PnvChip *chip, PowerPCCPU *cpu,
>>       * controller object is initialized afterwards. Hopefully, it's
>>       * only used at runtime.
>>       */
>> -    obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(&chip9->xive), &local_err);
>> +    obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(&chip9->xive), 0,
>> +                           &local_err);
>>      if (local_err) {
>>          error_propagate(errp, local_err);
>>          return;
> 

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

[Cédric Le Goater]

On 18/10/2019 11:42, Cédric Le Goater wrote:
> On 18/10/2019 05:55, David Gibson wrote:
>> On Thu, Oct 17, 2019 at 04:42:41PM +0200, Cédric Le Goater 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 in
>>> TCG or kernel_irqchip=off, QEMU needs to emulate the same behavior.
>>>
>>> Introduce a 'os-cam' property which will be used to set the OS CAM
>>> line at reset and remove the spapr_xive_set_tctx_os_cam() calls which
>>> are done when the XIVE interrupt controller are activated.
>>
>> I'm not immediately seeing the advantage of doing this via a property,
>> rather than poking it from the PAPR code which already knows the right
>> values.
> 
> we can simplify by passing the OS CAM line value as a parameter of the 
> xive_tctx_reset routine, as suggested by Greg.

and if we remove the reset handlers from XiveTCTX and rely only on the 
CPU reset handler to reset the presenter. 

C.

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

[Greg Kurz]

On Thu, 17 Oct 2019 16:42:41 +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 in
> TCG or kernel_irqchip=off, QEMU needs to emulate the same behavior.
> 

This is only related to kernel_irqchip=off, which is always the case
when running in TCG actually. Maybe rephrase to "When not running with
an in-kernel irqchip, QEMU needs..." ?

> Introduce a 'os-cam' property which will be used to set the OS CAM
> line at reset and remove the spapr_xive_set_tctx_os_cam() calls which
> are done when the XIVE interrupt controller are activated.
> 

Since OS CAM is constant, I guess it is ok to make it a property.
Alternatively, you could pass it as an extra parameter to
xive_tctx_reset().

> This change also has the benefit to remove the use of CPU_FOREACH()
> which can be unsafe.
> 

Nice !

> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  include/hw/ppc/spapr_xive.h |  1 -
>  include/hw/ppc/xive.h       |  4 +++-
>  hw/intc/spapr_xive.c        | 31 +++++--------------------------
>  hw/intc/xive.c              | 22 +++++++++++++++++++++-
>  hw/ppc/pnv.c                |  3 ++-
>  5 files changed, 31 insertions(+), 30 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/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> index 99381639f50c..e273069c25a9 100644
> --- a/include/hw/ppc/xive.h
> +++ b/include/hw/ppc/xive.h
> @@ -319,6 +319,7 @@ typedef struct XiveTCTX {
>      qemu_irq    os_output;
>  
>      uint8_t     regs[XIVE_TM_RING_COUNT * XIVE_TM_RING_SIZE];
> +    uint32_t    os_cam;
>  } XiveTCTX;
>  
>  /*
> @@ -414,7 +415,8 @@ void xive_tctx_tm_write(XiveTCTX *tctx, hwaddr offset, uint64_t value,
>  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);
> +Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, uint32_t os_cam,
> +                         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 0c3acf1a4192..71f138512a1c 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -205,21 +205,13 @@ 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)
> +static uint32_t spapr_xive_get_os_cam(PowerPCCPU *cpu)
>  {
>      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);
> +    spapr_xive_cpu_to_nvt(cpu, &nvt_blk, &nvt_idx);
> +    return xive_nvt_cam_line(nvt_blk, nvt_idx);
>  }
>  
>  static void spapr_xive_end_reset(XiveEND *end)
> @@ -537,19 +529,14 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
>      SpaprXive *xive = SPAPR_XIVE(intc);
>      Object *obj;
>      SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
> +    uint32_t os_cam = spapr_xive_get_os_cam(cpu);
>  
> -    obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(xive), errp);
> +    obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(xive), os_cam, errp);
>      if (!obj) {
>          return -1;
>      }
>  
>      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;
>  }
>  
> @@ -650,14 +637,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);
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index 0ae3f9b1efe4..be4f2c974178 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -566,6 +566,18 @@ static void xive_tctx_reset_handler(void *dev)
>          ipb_to_pipr(tctx->regs[TM_QW1_OS + TM_IPB]);
>      tctx->regs[TM_QW3_HV_PHYS + TM_PIPR] =
>          ipb_to_pipr(tctx->regs[TM_QW3_HV_PHYS + TM_IPB]);
> +
> +    /*
> +     * (TCG) Set the OS CAM line of the thread interrupt context.

As per my remark above, this shouldn't mention TCG but rather
kernel-irqchip=off.

> +     *
> +     * 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.
> +     */
> +    if (tctx->os_cam) {
> +        uint32_t qw1w2 = cpu_to_be32(TM_QW1W2_VO | tctx->os_cam);
> +        memcpy(&tctx->regs[TM_QW1_OS + TM_WORD2], &qw1w2, 4);
> +    }
>  }
>  
>  void xive_tctx_reset(XiveTCTX *tctx)
> @@ -667,11 +679,17 @@ static const VMStateDescription vmstate_xive_tctx = {
>      },
>  };
>  
> +static Property  xive_tctx_properties[] = {
> +    DEFINE_PROP_UINT32("os-cam", XiveTCTX, os_cam, 0),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  static void xive_tctx_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->desc = "XIVE Interrupt Thread Context";
> +    dc->props = xive_tctx_properties;
>      dc->realize = xive_tctx_realize;
>      dc->unrealize = xive_tctx_unrealize;
>      dc->vmsd = &vmstate_xive_tctx;
> @@ -689,7 +707,8 @@ static const TypeInfo xive_tctx_info = {
>      .class_init    = xive_tctx_class_init,
>  };
>  
> -Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp)
> +Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, uint32_t os_cam,
> +                         Error **errp)
>  {
>      Error *local_err = NULL;
>      Object *obj;
> @@ -698,6 +717,7 @@ Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp)
>      object_property_add_child(cpu, TYPE_XIVE_TCTX, obj, &error_abort);
>      object_unref(obj);
>      object_property_add_const_link(obj, "cpu", cpu, &error_abort);
> +    object_property_set_int(obj, os_cam, "os-cam", &local_err);
>      object_property_set_bool(obj, true, "realized", &local_err);
>      if (local_err) {
>          goto error;
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 7cf64b6d2533..99c06842573e 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -806,7 +806,8 @@ static void pnv_chip_power9_intc_create(PnvChip *chip, PowerPCCPU *cpu,
>       * controller object is initialized afterwards. Hopefully, it's
>       * only used at runtime.
>       */
> -    obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(&chip9->xive), &local_err);
> +    obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(&chip9->xive), 0,
> +                           &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;

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

[Cédric Le Goater]

On 18/10/2019 10:07, Greg Kurz wrote:
> On Thu, 17 Oct 2019 16:42:41 +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 in
>> TCG or kernel_irqchip=off, QEMU needs to emulate the same behavior.
>>
> 
> This is only related to kernel_irqchip=off, which is always the case
> when running in TCG actually. Maybe rephrase to "When not running with
> an in-kernel irqchip, QEMU needs..." ?

yes. 


>> Introduce a 'os-cam' property which will be used to set the OS CAM
>> line at reset and remove the spapr_xive_set_tctx_os_cam() calls which
>> are done when the XIVE interrupt controller are activated.
>>
> 
> Since OS CAM is constant, I guess it is ok to make it a property.
> Alternatively, you could pass it as an extra parameter to
> xive_tctx_reset().


indeed. We have all we need to do that. I will wait for some feedback.

>> This change also has the benefit to remove the use of CPU_FOREACH()
>> which can be unsafe.
>>
> 
> Nice !
> 
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  include/hw/ppc/spapr_xive.h |  1 -
>>  include/hw/ppc/xive.h       |  4 +++-
>>  hw/intc/spapr_xive.c        | 31 +++++--------------------------
>>  hw/intc/xive.c              | 22 +++++++++++++++++++++-
>>  hw/ppc/pnv.c                |  3 ++-
>>  5 files changed, 31 insertions(+), 30 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/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
>> index 99381639f50c..e273069c25a9 100644
>> --- a/include/hw/ppc/xive.h
>> +++ b/include/hw/ppc/xive.h
>> @@ -319,6 +319,7 @@ typedef struct XiveTCTX {
>>      qemu_irq    os_output;
>>  
>>      uint8_t     regs[XIVE_TM_RING_COUNT * XIVE_TM_RING_SIZE];
>> +    uint32_t    os_cam;
>>  } XiveTCTX;
>>  
>>  /*
>> @@ -414,7 +415,8 @@ void xive_tctx_tm_write(XiveTCTX *tctx, hwaddr offset, uint64_t value,
>>  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);
>> +Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, uint32_t os_cam,
>> +                         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 0c3acf1a4192..71f138512a1c 100644
>> --- a/hw/intc/spapr_xive.c
>> +++ b/hw/intc/spapr_xive.c
>> @@ -205,21 +205,13 @@ 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)
>> +static uint32_t spapr_xive_get_os_cam(PowerPCCPU *cpu)
>>  {
>>      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);
>> +    spapr_xive_cpu_to_nvt(cpu, &nvt_blk, &nvt_idx);
>> +    return xive_nvt_cam_line(nvt_blk, nvt_idx);
>>  }
>>  
>>  static void spapr_xive_end_reset(XiveEND *end)
>> @@ -537,19 +529,14 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
>>      SpaprXive *xive = SPAPR_XIVE(intc);
>>      Object *obj;
>>      SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
>> +    uint32_t os_cam = spapr_xive_get_os_cam(cpu);
>>  
>> -    obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(xive), errp);
>> +    obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(xive), os_cam, errp);
>>      if (!obj) {
>>          return -1;
>>      }
>>  
>>      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;
>>  }
>>  
>> @@ -650,14 +637,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);
>> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
>> index 0ae3f9b1efe4..be4f2c974178 100644
>> --- a/hw/intc/xive.c
>> +++ b/hw/intc/xive.c
>> @@ -566,6 +566,18 @@ static void xive_tctx_reset_handler(void *dev)
>>          ipb_to_pipr(tctx->regs[TM_QW1_OS + TM_IPB]);
>>      tctx->regs[TM_QW3_HV_PHYS + TM_PIPR] =
>>          ipb_to_pipr(tctx->regs[TM_QW3_HV_PHYS + TM_IPB]);
>> +
>> +    /*
>> +     * (TCG) Set the OS CAM line of the thread interrupt context.
> 
> As per my remark above, this shouldn't mention TCG but rather
> kernel-irqchip=off.

ok.

Thanks,

C.

> 
>> +     *
>> +     * 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.
>> +     */
>> +    if (tctx->os_cam) {
>> +        uint32_t qw1w2 = cpu_to_be32(TM_QW1W2_VO | tctx->os_cam);
>> +        memcpy(&tctx->regs[TM_QW1_OS + TM_WORD2], &qw1w2, 4);
>> +    }
>>  }
>>  
>>  void xive_tctx_reset(XiveTCTX *tctx)
>> @@ -667,11 +679,17 @@ static const VMStateDescription vmstate_xive_tctx = {
>>      },
>>  };
>>  
>> +static Property  xive_tctx_properties[] = {
>> +    DEFINE_PROP_UINT32("os-cam", XiveTCTX, os_cam, 0),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>>  static void xive_tctx_class_init(ObjectClass *klass, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>>  
>>      dc->desc = "XIVE Interrupt Thread Context";
>> +    dc->props = xive_tctx_properties;
>>      dc->realize = xive_tctx_realize;
>>      dc->unrealize = xive_tctx_unrealize;
>>      dc->vmsd = &vmstate_xive_tctx;
>> @@ -689,7 +707,8 @@ static const TypeInfo xive_tctx_info = {
>>      .class_init    = xive_tctx_class_init,
>>  };
>>  
>> -Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp)
>> +Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, uint32_t os_cam,
>> +                         Error **errp)
>>  {
>>      Error *local_err = NULL;
>>      Object *obj;
>> @@ -698,6 +717,7 @@ Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp)
>>      object_property_add_child(cpu, TYPE_XIVE_TCTX, obj, &error_abort);
>>      object_unref(obj);
>>      object_property_add_const_link(obj, "cpu", cpu, &error_abort);
>> +    object_property_set_int(obj, os_cam, "os-cam", &local_err);
>>      object_property_set_bool(obj, true, "realized", &local_err);
>>      if (local_err) {
>>          goto error;
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 7cf64b6d2533..99c06842573e 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -806,7 +806,8 @@ static void pnv_chip_power9_intc_create(PnvChip *chip, PowerPCCPU *cpu,
>>       * controller object is initialized afterwards. Hopefully, it's
>>       * only used at runtime.
>>       */
>> -    obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(&chip9->xive), &local_err);
>> +    obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(&chip9->xive), 0,
>> +                           &local_err);
>>      if (local_err) {
>>          error_propagate(errp, local_err);
>>          return;
>