diff mbox series

[02/10] ppc/xive: introduce a XiveTCTX pointer under PowerPCCPU

Message ID 20190102055743.5052-3-clg@kaod.org (mailing list archive)
State New, archived
Headers show
Series spapr: introduce the 'dual' interrupt mode XICS/XIVE | expand

Commit Message

Cédric Le Goater Jan. 2, 2019, 5:57 a.m. UTC
which will be used by the machine only when the XIVE interrupt mode is
in use.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 target/ppc/cpu.h        | 2 ++
 hw/intc/xive.c          | 6 +++---
 hw/ppc/spapr_cpu_core.c | 7 ++++++-
 hw/ppc/spapr_irq.c      | 8 ++++----
 4 files changed, 15 insertions(+), 8 deletions(-)

Comments

David Gibson Jan. 3, 2019, 3:57 a.m. UTC | #1
On Wed, Jan 02, 2019 at 06:57:35AM +0100, Cédric Le Goater wrote:
> which will be used by the machine only when the XIVE interrupt mode is
> in use.

I don't love the idea of putting a hook this specific into the
PowerPCCPU structure, though it might be the easiest path in the short
term.

A couple of approaches: 1) revisit my changes to allow for a pointer
to machine-defined per-cpu data.  or 2) do we actually need a cpu to
tctx pointer.

Expanding on (2) - here you use the pointer to find the right TIMA
state to access, but that could also be handled by having different
TIMA IO instances and mapping those individually to cpu_as.  On the
interrupt delivery side I think a tctx to cpu link will suffice.  For
sPAPR there might be complications with translating cpu numbers in
hcalls to the right tctx.

> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  target/ppc/cpu.h        | 2 ++
>  hw/intc/xive.c          | 6 +++---
>  hw/ppc/spapr_cpu_core.c | 7 ++++++-
>  hw/ppc/spapr_irq.c      | 8 ++++----
>  4 files changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index d5f99f1fc7b9..c76036985623 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1177,6 +1177,7 @@ do {                                            \
>  
>  typedef struct PPCVirtualHypervisor PPCVirtualHypervisor;
>  typedef struct PPCVirtualHypervisorClass PPCVirtualHypervisorClass;
> +typedef struct XiveTCTX XiveTCTX;
>  
>  /**
>   * PowerPCCPU:
> @@ -1196,6 +1197,7 @@ struct PowerPCCPU {
>      uint32_t compat_pvr;
>      PPCVirtualHypervisor *vhyp;
>      Object *intc;
> +    XiveTCTX *tctx;
>      void *machine_data;
>      int32_t node_id; /* NUMA node this CPU belongs to */
>      PPCHash64Options *hash64_opts;
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index ea33494338dc..410c53278a11 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -321,7 +321,7 @@ static void xive_tm_write(void *opaque, hwaddr offset,
>                            uint64_t value, unsigned size)
>  {
>      PowerPCCPU *cpu = POWERPC_CPU(current_cpu);
> -    XiveTCTX *tctx = XIVE_TCTX(cpu->intc);
> +    XiveTCTX *tctx = cpu->tctx;
>      const XiveTmOp *xto;
>  
>      /*
> @@ -360,7 +360,7 @@ static void xive_tm_write(void *opaque, hwaddr offset,
>  static uint64_t xive_tm_read(void *opaque, hwaddr offset, unsigned size)
>  {
>      PowerPCCPU *cpu = POWERPC_CPU(current_cpu);
> -    XiveTCTX *tctx = XIVE_TCTX(cpu->intc);
> +    XiveTCTX *tctx = cpu->tctx;
>      const XiveTmOp *xto;
>  
>      /*
> @@ -1186,7 +1186,7 @@ static bool xive_presenter_match(XiveRouter *xrtr, uint8_t format,
>  
>      CPU_FOREACH(cs) {
>          PowerPCCPU *cpu = POWERPC_CPU(cs);
> -        XiveTCTX *tctx = XIVE_TCTX(cpu->intc);
> +        XiveTCTX *tctx = cpu->tctx;
>          int ring;
>  
>          /*
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 2739b2a4b818..1473ef853336 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -194,7 +194,12 @@ static void spapr_unrealize_vcpu(PowerPCCPU *cpu, sPAPRCPUCore *sc)
>          vmstate_unregister(NULL, &vmstate_spapr_cpu_state, cpu->machine_data);
>      }
>      qemu_unregister_reset(spapr_cpu_reset, cpu);
> -    object_unparent(cpu->intc);
> +    if (cpu->intc) {
> +        object_unparent(cpu->intc);
> +    }
> +    if (cpu->tctx) {
> +        object_unparent(OBJECT(cpu->tctx));
> +    }
>      cpu_remove_sync(CPU(cpu));
>      object_unparent(OBJECT(cpu));
>  }
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 2d2e17b66533..8d028db44ff4 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -305,7 +305,7 @@ static void spapr_irq_print_info_xive(sPAPRMachineState *spapr,
>      CPU_FOREACH(cs) {
>          PowerPCCPU *cpu = POWERPC_CPU(cs);
>  
> -        xive_tctx_pic_print_info(XIVE_TCTX(cpu->intc), mon);
> +        xive_tctx_pic_print_info(cpu->tctx, mon);
>      }
>  
>      spapr_xive_pic_print_info(spapr->xive, mon);
> @@ -323,13 +323,13 @@ static void spapr_irq_cpu_intc_create_xive(sPAPRMachineState *spapr,
>          return;
>      }
>  
> -    cpu->intc = obj;
> +    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(XIVE_TCTX(obj));
> +    spapr_xive_set_tctx_os_cam(cpu->tctx);
>  }
>  
>  static int spapr_irq_post_load_xive(sPAPRMachineState *spapr, int version_id)
> @@ -345,7 +345,7 @@ static void spapr_irq_reset_xive(sPAPRMachineState *spapr, Error **errp)
>          PowerPCCPU *cpu = POWERPC_CPU(cs);
>  
>          /* (TCG) Set the OS CAM line of the thread interrupt context. */
> -        spapr_xive_set_tctx_os_cam(XIVE_TCTX(cpu->intc));
> +        spapr_xive_set_tctx_os_cam(cpu->tctx);
>      }
>  }
>
Cédric Le Goater Jan. 3, 2019, 5:44 p.m. UTC | #2
On 1/3/19 4:57 AM, David Gibson wrote:
> On Wed, Jan 02, 2019 at 06:57:35AM +0100, Cédric Le Goater wrote:
>> which will be used by the machine only when the XIVE interrupt mode is
>> in use.
> 
> I don't love the idea of putting a hook this specific into the
> PowerPCCPU structure, though it might be the easiest path in the short
> term.
> 
> A couple of approaches: 1) revisit my changes to allow for a pointer
> to machine-defined per-cpu data.  

ok but we still need two different presenters objects, one for each
mode.

> or 2) do we actually need a cpu to tctx pointer.
> 
> Expanding on (2) - here you use the pointer to find the right TIMA
> state to access,

yes. 

> but that could also be handled by having different TIMA IO instances 
> and mapping those individually to cpu_as.  

It might work for QEMU but I am not sure how to implement the KVM 
backend with such a design. We only have a single ram ptr mapping 
for the machine in the KVM case.

There are a couple of places where we need to loop on the XiveTCTX 
(presenter, KVM) and we use the QEMU CPU list and the cpu->tctx for 
that. One of the reasons we introduced the cpu->intc pointer some 
time ago was to get rid of the ICP array. 

I don't think we want to introduce a XiveTCTX array again.

> On the interrupt delivery side I think a tctx to cpu link will suffice.  

yes. that we already have. It is mostly used by KVM in fact. 

> For sPAPR there might be complications with translating cpu numbers in
> hcalls to the right tctx.

The XiveTCTX is not used by the hcalls. We are fine on that side.


The double pointer solution is what I prefer today, but if you are 
uncomfortable with it, we can come back to the previous where I was 
allocating a XiveTCTX child under the CPU and switching the presenter 
objects at reset. The only issue remaining was the child unparenting 
in the unrealize() method.    

Thanks, 

C.

 
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  target/ppc/cpu.h        | 2 ++
>>  hw/intc/xive.c          | 6 +++---
>>  hw/ppc/spapr_cpu_core.c | 7 ++++++-
>>  hw/ppc/spapr_irq.c      | 8 ++++----
>>  4 files changed, 15 insertions(+), 8 deletions(-)
>>
>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>> index d5f99f1fc7b9..c76036985623 100644
>> --- a/target/ppc/cpu.h
>> +++ b/target/ppc/cpu.h
>> @@ -1177,6 +1177,7 @@ do {                                            \
>>  
>>  typedef struct PPCVirtualHypervisor PPCVirtualHypervisor;
>>  typedef struct PPCVirtualHypervisorClass PPCVirtualHypervisorClass;
>> +typedef struct XiveTCTX XiveTCTX;
>>  
>>  /**
>>   * PowerPCCPU:
>> @@ -1196,6 +1197,7 @@ struct PowerPCCPU {
>>      uint32_t compat_pvr;
>>      PPCVirtualHypervisor *vhyp;
>>      Object *intc;
>> +    XiveTCTX *tctx;
>>      void *machine_data;
>>      int32_t node_id; /* NUMA node this CPU belongs to */
>>      PPCHash64Options *hash64_opts;
>> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
>> index ea33494338dc..410c53278a11 100644
>> --- a/hw/intc/xive.c
>> +++ b/hw/intc/xive.c
>> @@ -321,7 +321,7 @@ static void xive_tm_write(void *opaque, hwaddr offset,
>>                            uint64_t value, unsigned size)
>>  {
>>      PowerPCCPU *cpu = POWERPC_CPU(current_cpu);
>> -    XiveTCTX *tctx = XIVE_TCTX(cpu->intc);
>> +    XiveTCTX *tctx = cpu->tctx;
>>      const XiveTmOp *xto;
>>  
>>      /*
>> @@ -360,7 +360,7 @@ static void xive_tm_write(void *opaque, hwaddr offset,
>>  static uint64_t xive_tm_read(void *opaque, hwaddr offset, unsigned size)
>>  {
>>      PowerPCCPU *cpu = POWERPC_CPU(current_cpu);
>> -    XiveTCTX *tctx = XIVE_TCTX(cpu->intc);
>> +    XiveTCTX *tctx = cpu->tctx;
>>      const XiveTmOp *xto;
>>  
>>      /*
>> @@ -1186,7 +1186,7 @@ static bool xive_presenter_match(XiveRouter *xrtr, uint8_t format,
>>  
>>      CPU_FOREACH(cs) {
>>          PowerPCCPU *cpu = POWERPC_CPU(cs);
>> -        XiveTCTX *tctx = XIVE_TCTX(cpu->intc);
>> +        XiveTCTX *tctx = cpu->tctx;
>>          int ring;
>>  
>>          /*
>> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
>> index 2739b2a4b818..1473ef853336 100644
>> --- a/hw/ppc/spapr_cpu_core.c
>> +++ b/hw/ppc/spapr_cpu_core.c
>> @@ -194,7 +194,12 @@ static void spapr_unrealize_vcpu(PowerPCCPU *cpu, sPAPRCPUCore *sc)
>>          vmstate_unregister(NULL, &vmstate_spapr_cpu_state, cpu->machine_data);
>>      }
>>      qemu_unregister_reset(spapr_cpu_reset, cpu);
>> -    object_unparent(cpu->intc);
>> +    if (cpu->intc) {
>> +        object_unparent(cpu->intc);
>> +    }
>> +    if (cpu->tctx) {
>> +        object_unparent(OBJECT(cpu->tctx));
>> +    }
>>      cpu_remove_sync(CPU(cpu));
>>      object_unparent(OBJECT(cpu));
>>  }
>> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
>> index 2d2e17b66533..8d028db44ff4 100644
>> --- a/hw/ppc/spapr_irq.c
>> +++ b/hw/ppc/spapr_irq.c
>> @@ -305,7 +305,7 @@ static void spapr_irq_print_info_xive(sPAPRMachineState *spapr,
>>      CPU_FOREACH(cs) {
>>          PowerPCCPU *cpu = POWERPC_CPU(cs);
>>  
>> -        xive_tctx_pic_print_info(XIVE_TCTX(cpu->intc), mon);
>> +        xive_tctx_pic_print_info(cpu->tctx, mon);
>>      }
>>  
>>      spapr_xive_pic_print_info(spapr->xive, mon);
>> @@ -323,13 +323,13 @@ static void spapr_irq_cpu_intc_create_xive(sPAPRMachineState *spapr,
>>          return;
>>      }
>>  
>> -    cpu->intc = obj;
>> +    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(XIVE_TCTX(obj));
>> +    spapr_xive_set_tctx_os_cam(cpu->tctx);
>>  }
>>  
>>  static int spapr_irq_post_load_xive(sPAPRMachineState *spapr, int version_id)
>> @@ -345,7 +345,7 @@ static void spapr_irq_reset_xive(sPAPRMachineState *spapr, Error **errp)
>>          PowerPCCPU *cpu = POWERPC_CPU(cs);
>>  
>>          /* (TCG) Set the OS CAM line of the thread interrupt context. */
>> -        spapr_xive_set_tctx_os_cam(XIVE_TCTX(cpu->intc));
>> +        spapr_xive_set_tctx_os_cam(cpu->tctx);
>>      }
>>  }
>>  
>
David Gibson Jan. 4, 2019, 5:25 a.m. UTC | #3
On Thu, Jan 03, 2019 at 06:44:30PM +0100, Cédric Le Goater wrote:
> On 1/3/19 4:57 AM, David Gibson wrote:
> > On Wed, Jan 02, 2019 at 06:57:35AM +0100, Cédric Le Goater wrote:
> >> which will be used by the machine only when the XIVE interrupt mode is
> >> in use.
> > 
> > I don't love the idea of putting a hook this specific into the
> > PowerPCCPU structure, though it might be the easiest path in the short
> > term.
> > 
> > A couple of approaches: 1) revisit my changes to allow for a pointer
> > to machine-defined per-cpu data.  
> 
> ok but we still need two different presenters objects, one for each
> mode.
> 
> > or 2) do we actually need a cpu to tctx pointer.
> > 
> > Expanding on (2) - here you use the pointer to find the right TIMA
> > state to access,
> 
> yes. 
> 
> > but that could also be handled by having different TIMA IO instances 
> > and mapping those individually to cpu_as.  
> 
> It might work for QEMU but I am not sure how to implement the KVM 
> backend with such a design. We only have a single ram ptr mapping 
> for the machine in the KVM case.
> 
> There are a couple of places where we need to loop on the XiveTCTX 
> (presenter, KVM) and we use the QEMU CPU list and the cpu->tctx for 
> that. One of the reasons we introduced the cpu->intc pointer some 
> time ago was to get rid of the ICP array. 
> 
> I don't think we want to introduce a XiveTCTX array again.
> 
> > On the interrupt delivery side I think a tctx to cpu link will suffice.  
> 
> yes. that we already have. It is mostly used by KVM in fact. 
> 
> > For sPAPR there might be complications with translating cpu numbers in
> > hcalls to the right tctx.
> 
> The XiveTCTX is not used by the hcalls. We are fine on that side.
> 
> 
> The double pointer solution is what I prefer today, but if you are 
> uncomfortable with it, we can come back to the previous where I was 
> allocating a XiveTCTX child under the CPU and switching the presenter 
> objects at reset. The only issue remaining was the child unparenting 
> in the unrealize() method.

Hm, yes, I see your point.  For now I'm applying patches 1-3, and hope
to clean it up later with a revised version of my machine specific
per-cpu patch when I get the chance.
diff mbox series

Patch

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index d5f99f1fc7b9..c76036985623 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1177,6 +1177,7 @@  do {                                            \
 
 typedef struct PPCVirtualHypervisor PPCVirtualHypervisor;
 typedef struct PPCVirtualHypervisorClass PPCVirtualHypervisorClass;
+typedef struct XiveTCTX XiveTCTX;
 
 /**
  * PowerPCCPU:
@@ -1196,6 +1197,7 @@  struct PowerPCCPU {
     uint32_t compat_pvr;
     PPCVirtualHypervisor *vhyp;
     Object *intc;
+    XiveTCTX *tctx;
     void *machine_data;
     int32_t node_id; /* NUMA node this CPU belongs to */
     PPCHash64Options *hash64_opts;
diff --git a/hw/intc/xive.c b/hw/intc/xive.c
index ea33494338dc..410c53278a11 100644
--- a/hw/intc/xive.c
+++ b/hw/intc/xive.c
@@ -321,7 +321,7 @@  static void xive_tm_write(void *opaque, hwaddr offset,
                           uint64_t value, unsigned size)
 {
     PowerPCCPU *cpu = POWERPC_CPU(current_cpu);
-    XiveTCTX *tctx = XIVE_TCTX(cpu->intc);
+    XiveTCTX *tctx = cpu->tctx;
     const XiveTmOp *xto;
 
     /*
@@ -360,7 +360,7 @@  static void xive_tm_write(void *opaque, hwaddr offset,
 static uint64_t xive_tm_read(void *opaque, hwaddr offset, unsigned size)
 {
     PowerPCCPU *cpu = POWERPC_CPU(current_cpu);
-    XiveTCTX *tctx = XIVE_TCTX(cpu->intc);
+    XiveTCTX *tctx = cpu->tctx;
     const XiveTmOp *xto;
 
     /*
@@ -1186,7 +1186,7 @@  static bool xive_presenter_match(XiveRouter *xrtr, uint8_t format,
 
     CPU_FOREACH(cs) {
         PowerPCCPU *cpu = POWERPC_CPU(cs);
-        XiveTCTX *tctx = XIVE_TCTX(cpu->intc);
+        XiveTCTX *tctx = cpu->tctx;
         int ring;
 
         /*
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 2739b2a4b818..1473ef853336 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -194,7 +194,12 @@  static void spapr_unrealize_vcpu(PowerPCCPU *cpu, sPAPRCPUCore *sc)
         vmstate_unregister(NULL, &vmstate_spapr_cpu_state, cpu->machine_data);
     }
     qemu_unregister_reset(spapr_cpu_reset, cpu);
-    object_unparent(cpu->intc);
+    if (cpu->intc) {
+        object_unparent(cpu->intc);
+    }
+    if (cpu->tctx) {
+        object_unparent(OBJECT(cpu->tctx));
+    }
     cpu_remove_sync(CPU(cpu));
     object_unparent(OBJECT(cpu));
 }
diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
index 2d2e17b66533..8d028db44ff4 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -305,7 +305,7 @@  static void spapr_irq_print_info_xive(sPAPRMachineState *spapr,
     CPU_FOREACH(cs) {
         PowerPCCPU *cpu = POWERPC_CPU(cs);
 
-        xive_tctx_pic_print_info(XIVE_TCTX(cpu->intc), mon);
+        xive_tctx_pic_print_info(cpu->tctx, mon);
     }
 
     spapr_xive_pic_print_info(spapr->xive, mon);
@@ -323,13 +323,13 @@  static void spapr_irq_cpu_intc_create_xive(sPAPRMachineState *spapr,
         return;
     }
 
-    cpu->intc = obj;
+    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(XIVE_TCTX(obj));
+    spapr_xive_set_tctx_os_cam(cpu->tctx);
 }
 
 static int spapr_irq_post_load_xive(sPAPRMachineState *spapr, int version_id)
@@ -345,7 +345,7 @@  static void spapr_irq_reset_xive(sPAPRMachineState *spapr, Error **errp)
         PowerPCCPU *cpu = POWERPC_CPU(cs);
 
         /* (TCG) Set the OS CAM line of the thread interrupt context. */
-        spapr_xive_set_tctx_os_cam(XIVE_TCTX(cpu->intc));
+        spapr_xive_set_tctx_os_cam(cpu->tctx);
     }
 }