[Qemu-devel] [RFC PATCH] ppc/xics: introduce helpers to find an ICP from some (CPU) index

[Qemu-devel] [RFC PATCH] ppc/xics: introduce helpers to find an ICP from some (CPU) index

[Cédric Le Goater]

Today, the CPU index is used to index the icp array under xics. This
works correctly when the indexes are sync but that is an assumption
that could break future implementations. For instance, the PowerNV
platform CPUs use real HW ids and they are not contiguous.

Let's introduce some helpers to hide the underlying structure of the
ICP array. The criteria to look for an ICPstate is still the CPU index
but it is decorrelated from the array index.

This is an RFC to see what people think. It is used on the powernv
branch but it won't apply as it is on upstream. It should certainly be
optimised when a large number of CPUs are configured.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/intc/xics.c        |   59 ++++++++++++++++++++++++++++++++++++++++----------
 hw/intc/xics_kvm.c    |    7 +----
 hw/intc/xics_spapr.c  |   14 ++++++-----
 include/hw/ppc/xics.h |    1 
 4 files changed, 59 insertions(+), 22 deletions(-)

Index: qemu-dgibson-for-2.8.git/hw/intc/xics.c
===================================================================
--- qemu-dgibson-for-2.8.git.orig/hw/intc/xics.c
+++ qemu-dgibson-for-2.8.git/hw/intc/xics.c
@@ -50,12 +50,41 @@ int xics_get_cpu_index_by_dt_id(int cpu_
     return -1;
 }
 
+
+static ICPState *xics_get_icp(XICSState *xics, CPUState *cs)
+{
+    int i;
+
+    for (i = 0 ; i < xics->nr_servers; i++) {
+        ICPState *ss = &xics->ss[i];
+        if (ss->cs == NULL) {
+            ss->cs = cs;
+            return ss;
+        }
+    }
+
+    return NULL;
+}
+
+ICPState *xics_find_icp(XICSState *xics, int cpu_index)
+{
+    int i;
+
+    for (i = 0 ; i < xics->nr_servers; i++) {
+        ICPState *ss = &xics->ss[i];
+        if (ss->cs && ss->cs->cpu_index == cpu_index)
+            return ss;
+    }
+
+    return NULL;
+}
+
 void xics_cpu_destroy(XICSState *xics, PowerPCCPU *cpu)
 {
     CPUState *cs = CPU(cpu);
-    ICPState *ss = &xics->ss[cs->cpu_index];
+    ICPState *ss = xics_find_icp(xics, cs->cpu_index);
 
-    assert(cs->cpu_index < xics->nr_servers);
+    assert(ss);
     assert(cs == ss->cs);
 
     ss->output = NULL;
@@ -66,12 +95,10 @@ void xics_cpu_setup(XICSState *xics, Pow
 {
     CPUState *cs = CPU(cpu);
     CPUPPCState *env = &cpu->env;
-    ICPState *ss = &xics->ss[cs->cpu_index];
+    ICPState *ss = xics_get_icp(xics, cs);
     XICSStateClass *info = XICS_COMMON_GET_CLASS(xics);
 
-    assert(cs->cpu_index < xics->nr_servers);
-
-    ss->cs = cs;
+    assert(ss);
 
     if (info->cpu_setup) {
         info->cpu_setup(xics, cpu);
@@ -276,9 +303,11 @@ static void icp_check_ipi(ICPState *ss)
 
 static void icp_resend(XICSState *xics, int server)
 {
-    ICPState *ss = xics->ss + server;
+    ICPState *ss = xics_find_icp(xics, server);
     ICSState *ics;
 
+    assert(ss);
+
     if (ss->mfrr < CPPR(ss)) {
         icp_check_ipi(ss);
     }
@@ -289,10 +318,12 @@ static void icp_resend(XICSState *xics,
 
 void icp_set_cppr(XICSState *xics, int server, uint8_t cppr)
 {
-    ICPState *ss = xics->ss + server;
+    ICPState *ss = xics_find_icp(xics, server);
     uint8_t old_cppr;
     uint32_t old_xisr;
 
+    assert(ss);
+
     old_cppr = CPPR(ss);
     ss->xirr = (ss->xirr & ~CPPR_MASK) | (cppr << 24);
 
@@ -316,7 +347,9 @@ void icp_set_cppr(XICSState *xics, int s
 
 void icp_set_mfrr(XICSState *xics, int server, uint8_t mfrr)
 {
-    ICPState *ss = xics->ss + server;
+    ICPState *ss = xics_find_icp(xics, server);
+
+    assert(ss);
 
     ss->mfrr = mfrr;
     if (mfrr < CPPR(ss)) {
@@ -348,10 +381,12 @@ uint32_t icp_ipoll(ICPState *ss, uint32_
 
 void icp_eoi(XICSState *xics, int server, uint32_t xirr)
 {
-    ICPState *ss = xics->ss + server;
+    ICPState *ss = xics_find_icp(xics, server);
     ICSState *ics;
     uint32_t irq;
 
+    assert(ss);
+
     /* Send EOI -> ICS */
     ss->xirr = (ss->xirr & ~CPPR_MASK) | (xirr & CPPR_MASK);
     trace_xics_icp_eoi(server, xirr, ss->xirr);
@@ -369,7 +404,9 @@ void icp_eoi(XICSState *xics, int server
 static void icp_irq(ICSState *ics, int server, int nr, uint8_t priority)
 {
     XICSState *xics = ics->xics;
-    ICPState *ss = xics->ss + server;
+    ICPState *ss = xics_find_icp(xics, server);
+
+    assert(ss);
 
     trace_xics_icp_irq(server, nr, priority);
 
Index: qemu-dgibson-for-2.8.git/include/hw/ppc/xics.h
===================================================================
--- qemu-dgibson-for-2.8.git.orig/include/hw/ppc/xics.h
+++ qemu-dgibson-for-2.8.git/include/hw/ppc/xics.h
@@ -207,5 +207,6 @@ ICSState *xics_find_source(XICSState *ic
 void xics_add_ics(XICSState *xics);
 
 void xics_hmp_info_pic(Monitor *mon, const QDict *qdict);
+ICPState *xics_find_icp(XICSState *xics, int cpu_index);
 
 #endif /* XICS_H */
Index: qemu-dgibson-for-2.8.git/hw/intc/xics_spapr.c
===================================================================
--- qemu-dgibson-for-2.8.git.orig/hw/intc/xics_spapr.c
+++ qemu-dgibson-for-2.8.git/hw/intc/xics_spapr.c
@@ -67,9 +67,11 @@ static target_ulong h_xirr(PowerPCCPU *c
                            target_ulong opcode, target_ulong *args)
 {
     CPUState *cs = CPU(cpu);
-    uint32_t xirr = icp_accept(spapr->xics->ss + cs->cpu_index);
+    ICPState *ss = xics_find_icp(spapr->xics, cs->cpu_index);
 
-    args[0] = xirr;
+    assert(ss);
+
+    args[0] = icp_accept(ss);
     return H_SUCCESS;
 }
 
@@ -77,10 +79,9 @@ static target_ulong h_xirr_x(PowerPCCPU
                              target_ulong opcode, target_ulong *args)
 {
     CPUState *cs = CPU(cpu);
-    ICPState *ss = &spapr->xics->ss[cs->cpu_index];
-    uint32_t xirr = icp_accept(ss);
+    ICPState *ss = xics_find_icp(spapr->xics, cs->cpu_index);
 
-    args[0] = xirr;
+    args[0] = icp_accept(ss);
     args[1] = cpu_get_host_ticks();
     return H_SUCCESS;
 }
@@ -99,8 +100,9 @@ static target_ulong h_ipoll(PowerPCCPU *
                             target_ulong opcode, target_ulong *args)
 {
     CPUState *cs = CPU(cpu);
+    ICPState *ss = xics_find_icp(spapr->xics, cs->cpu_index);
     uint32_t mfrr;
-    uint32_t xirr = icp_ipoll(spapr->xics->ss + cs->cpu_index, &mfrr);
+    uint32_t xirr = icp_ipoll(ss, &mfrr);
 
     args[0] = xirr;
     args[1] = mfrr;
Index: qemu-dgibson-for-2.8.git/hw/intc/xics_kvm.c
===================================================================
--- qemu-dgibson-for-2.8.git.orig/hw/intc/xics_kvm.c
+++ qemu-dgibson-for-2.8.git/hw/intc/xics_kvm.c
@@ -326,14 +326,11 @@ static const TypeInfo ics_kvm_info = {
  */
 static void xics_kvm_cpu_setup(XICSState *xics, PowerPCCPU *cpu)
 {
-    CPUState *cs;
-    ICPState *ss;
+    CPUState *cs = CPU(cpu);
+    ICPState *ss = xics_find_icp(xics, cs->cpu_index);
     KVMXICSState *xicskvm = XICS_SPAPR_KVM(xics);
     int ret;
 
-    cs = CPU(cpu);
-    ss = &xics->ss[cs->cpu_index];
-
     assert(cs->cpu_index < xics->nr_servers);
     if (xicskvm->kernel_xics_fd == -1) {
         abort();

Re: [Qemu-devel] [RFC PATCH] ppc/xics: introduce helpers to find an ICP from some (CPU) index

[David Gibson]

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

On Mon, Sep 19, 2016 at 10:54:10AM +0200, Cédric Le Goater wrote:
> Today, the CPU index is used to index the icp array under xics. This
> works correctly when the indexes are sync but that is an assumption
> that could break future implementations. For instance, the PowerNV
> platform CPUs use real HW ids and they are not contiguous.
> 
> Let's introduce some helpers to hide the underlying structure of the
> ICP array. The criteria to look for an ICPstate is still the CPU index
> but it is decorrelated from the array index.
> 
> This is an RFC to see what people think. It is used on the powernv
> branch but it won't apply as it is on upstream. It should certainly be
> optimised when a large number of CPUs are configured.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Hm, so.

First, I think the helpers to get the right icp state are a good idea,
regardless of anything else.  I'm happy to go ahead with a patch which
introduces just that, because it will make future revisions easier.

But, the rest of the changes seem to be predicated on allowing
non-contiguous cpu_index values.  Maybe we want to do that eventually,
but we're a pretty long way off at present, doing so involves work in
libvirt as well as qemu itself, and it's not clear we actually want
it.

I think for the time being you'd be better off keeping the simple
array of icp states indexed by contiguous (barring cpu hotplug)
cpu_index values, and dissociating the physical IDs (PIR == interrupt
server number == DT id, IIUC) from the cpu_index.  Obviously you'll
need helpers to convert between cpu_index and physical ID at the
machine level.

> ---
>  hw/intc/xics.c        |   59 ++++++++++++++++++++++++++++++++++++++++----------
>  hw/intc/xics_kvm.c    |    7 +----
>  hw/intc/xics_spapr.c  |   14 ++++++-----
>  include/hw/ppc/xics.h |    1 
>  4 files changed, 59 insertions(+), 22 deletions(-)
> 
> Index: qemu-dgibson-for-2.8.git/hw/intc/xics.c
> ===================================================================
> --- qemu-dgibson-for-2.8.git.orig/hw/intc/xics.c
> +++ qemu-dgibson-for-2.8.git/hw/intc/xics.c
> @@ -50,12 +50,41 @@ int xics_get_cpu_index_by_dt_id(int cpu_
>      return -1;
>  }
>  
> +
> +static ICPState *xics_get_icp(XICSState *xics, CPUState *cs)
> +{
> +    int i;
> +
> +    for (i = 0 ; i < xics->nr_servers; i++) {
> +        ICPState *ss = &xics->ss[i];
> +        if (ss->cs == NULL) {
> +            ss->cs = cs;
> +            return ss;
> +        }
> +    }
> +
> +    return NULL;
> +}
> +
> +ICPState *xics_find_icp(XICSState *xics, int cpu_index)
> +{
> +    int i;
> +
> +    for (i = 0 ; i < xics->nr_servers; i++) {
> +        ICPState *ss = &xics->ss[i];
> +        if (ss->cs && ss->cs->cpu_index == cpu_index)
> +            return ss;
> +    }
> +
> +    return NULL;
> +}
> +
>  void xics_cpu_destroy(XICSState *xics, PowerPCCPU *cpu)
>  {
>      CPUState *cs = CPU(cpu);
> -    ICPState *ss = &xics->ss[cs->cpu_index];
> +    ICPState *ss = xics_find_icp(xics, cs->cpu_index);
>  
> -    assert(cs->cpu_index < xics->nr_servers);
> +    assert(ss);
>      assert(cs == ss->cs);
>  
>      ss->output = NULL;
> @@ -66,12 +95,10 @@ void xics_cpu_setup(XICSState *xics, Pow
>  {
>      CPUState *cs = CPU(cpu);
>      CPUPPCState *env = &cpu->env;
> -    ICPState *ss = &xics->ss[cs->cpu_index];
> +    ICPState *ss = xics_get_icp(xics, cs);
>      XICSStateClass *info = XICS_COMMON_GET_CLASS(xics);
>  
> -    assert(cs->cpu_index < xics->nr_servers);
> -
> -    ss->cs = cs;
> +    assert(ss);
>  
>      if (info->cpu_setup) {
>          info->cpu_setup(xics, cpu);
> @@ -276,9 +303,11 @@ static void icp_check_ipi(ICPState *ss)
>  
>  static void icp_resend(XICSState *xics, int server)
>  {
> -    ICPState *ss = xics->ss + server;
> +    ICPState *ss = xics_find_icp(xics, server);
>      ICSState *ics;
>  
> +    assert(ss);
> +
>      if (ss->mfrr < CPPR(ss)) {
>          icp_check_ipi(ss);
>      }
> @@ -289,10 +318,12 @@ static void icp_resend(XICSState *xics,
>  
>  void icp_set_cppr(XICSState *xics, int server, uint8_t cppr)
>  {
> -    ICPState *ss = xics->ss + server;
> +    ICPState *ss = xics_find_icp(xics, server);
>      uint8_t old_cppr;
>      uint32_t old_xisr;
>  
> +    assert(ss);
> +
>      old_cppr = CPPR(ss);
>      ss->xirr = (ss->xirr & ~CPPR_MASK) | (cppr << 24);
>  
> @@ -316,7 +347,9 @@ void icp_set_cppr(XICSState *xics, int s
>  
>  void icp_set_mfrr(XICSState *xics, int server, uint8_t mfrr)
>  {
> -    ICPState *ss = xics->ss + server;
> +    ICPState *ss = xics_find_icp(xics, server);
> +
> +    assert(ss);
>  
>      ss->mfrr = mfrr;
>      if (mfrr < CPPR(ss)) {
> @@ -348,10 +381,12 @@ uint32_t icp_ipoll(ICPState *ss, uint32_
>  
>  void icp_eoi(XICSState *xics, int server, uint32_t xirr)
>  {
> -    ICPState *ss = xics->ss + server;
> +    ICPState *ss = xics_find_icp(xics, server);
>      ICSState *ics;
>      uint32_t irq;
>  
> +    assert(ss);
> +
>      /* Send EOI -> ICS */
>      ss->xirr = (ss->xirr & ~CPPR_MASK) | (xirr & CPPR_MASK);
>      trace_xics_icp_eoi(server, xirr, ss->xirr);
> @@ -369,7 +404,9 @@ void icp_eoi(XICSState *xics, int server
>  static void icp_irq(ICSState *ics, int server, int nr, uint8_t priority)
>  {
>      XICSState *xics = ics->xics;
> -    ICPState *ss = xics->ss + server;
> +    ICPState *ss = xics_find_icp(xics, server);
> +
> +    assert(ss);
>  
>      trace_xics_icp_irq(server, nr, priority);
>  
> Index: qemu-dgibson-for-2.8.git/include/hw/ppc/xics.h
> ===================================================================
> --- qemu-dgibson-for-2.8.git.orig/include/hw/ppc/xics.h
> +++ qemu-dgibson-for-2.8.git/include/hw/ppc/xics.h
> @@ -207,5 +207,6 @@ ICSState *xics_find_source(XICSState *ic
>  void xics_add_ics(XICSState *xics);
>  
>  void xics_hmp_info_pic(Monitor *mon, const QDict *qdict);
> +ICPState *xics_find_icp(XICSState *xics, int cpu_index);
>  
>  #endif /* XICS_H */
> Index: qemu-dgibson-for-2.8.git/hw/intc/xics_spapr.c
> ===================================================================
> --- qemu-dgibson-for-2.8.git.orig/hw/intc/xics_spapr.c
> +++ qemu-dgibson-for-2.8.git/hw/intc/xics_spapr.c
> @@ -67,9 +67,11 @@ static target_ulong h_xirr(PowerPCCPU *c
>                             target_ulong opcode, target_ulong *args)
>  {
>      CPUState *cs = CPU(cpu);
> -    uint32_t xirr = icp_accept(spapr->xics->ss + cs->cpu_index);
> +    ICPState *ss = xics_find_icp(spapr->xics, cs->cpu_index);
>  
> -    args[0] = xirr;
> +    assert(ss);
> +
> +    args[0] = icp_accept(ss);
>      return H_SUCCESS;
>  }
>  
> @@ -77,10 +79,9 @@ static target_ulong h_xirr_x(PowerPCCPU
>                               target_ulong opcode, target_ulong *args)
>  {
>      CPUState *cs = CPU(cpu);
> -    ICPState *ss = &spapr->xics->ss[cs->cpu_index];
> -    uint32_t xirr = icp_accept(ss);
> +    ICPState *ss = xics_find_icp(spapr->xics, cs->cpu_index);
>  
> -    args[0] = xirr;
> +    args[0] = icp_accept(ss);
>      args[1] = cpu_get_host_ticks();
>      return H_SUCCESS;
>  }
> @@ -99,8 +100,9 @@ static target_ulong h_ipoll(PowerPCCPU *
>                              target_ulong opcode, target_ulong *args)
>  {
>      CPUState *cs = CPU(cpu);
> +    ICPState *ss = xics_find_icp(spapr->xics, cs->cpu_index);
>      uint32_t mfrr;
> -    uint32_t xirr = icp_ipoll(spapr->xics->ss + cs->cpu_index, &mfrr);
> +    uint32_t xirr = icp_ipoll(ss, &mfrr);
>  
>      args[0] = xirr;
>      args[1] = mfrr;
> Index: qemu-dgibson-for-2.8.git/hw/intc/xics_kvm.c
> ===================================================================
> --- qemu-dgibson-for-2.8.git.orig/hw/intc/xics_kvm.c
> +++ qemu-dgibson-for-2.8.git/hw/intc/xics_kvm.c
> @@ -326,14 +326,11 @@ static const TypeInfo ics_kvm_info = {
>   */
>  static void xics_kvm_cpu_setup(XICSState *xics, PowerPCCPU *cpu)
>  {
> -    CPUState *cs;
> -    ICPState *ss;
> +    CPUState *cs = CPU(cpu);
> +    ICPState *ss = xics_find_icp(xics, cs->cpu_index);
>      KVMXICSState *xicskvm = XICS_SPAPR_KVM(xics);
>      int ret;
>  
> -    cs = CPU(cpu);
> -    ss = &xics->ss[cs->cpu_index];
> -
>      assert(cs->cpu_index < xics->nr_servers);
>      if (xicskvm->kernel_xics_fd == -1) {
>          abort();
> 

-- 
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: 819 bytes --]

Re: [Qemu-devel] [RFC PATCH] ppc/xics: introduce helpers to find an ICP from some (CPU) index

[Cédric Le Goater]

On 09/20/2016 03:25 PM, David Gibson wrote:
> On Mon, Sep 19, 2016 at 10:54:10AM +0200, Cédric Le Goater wrote:
>> Today, the CPU index is used to index the icp array under xics. This
>> works correctly when the indexes are sync but that is an assumption
>> that could break future implementations. For instance, the PowerNV
>> platform CPUs use real HW ids and they are not contiguous.
>>
>> Let's introduce some helpers to hide the underlying structure of the
>> ICP array. The criteria to look for an ICPstate is still the CPU index
>> but it is decorrelated from the array index.
>>
>> This is an RFC to see what people think. It is used on the powernv
>> branch but it won't apply as it is on upstream. It should certainly be
>> optimised when a large number of CPUs are configured.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> 
> Hm, so.
> 
> First, I think the helpers to get the right icp state are a good idea,
> regardless of anything else.  I'm happy to go ahead with a patch which
> introduces just that, because it will make future revisions easier.

yes. that would be a first step, to hide the underlying implementation. 

> But, the rest of the changes seem to be predicated on allowing
> non-contiguous cpu_index values.  Maybe we want to do that eventually,
> but we're a pretty long way off at present, doing so involves work in
> libvirt as well as qemu itself, and it's not clear we actually want
> it.

OK. The pir being used in most places in the code, this is not a big
problem, and even this part in the core object :

+    env->spr[SPR_PIR] = cs->cpu_index;

could be easily worked out. I was just lazy. 

So the only area where we are using cpu_index is in xics for the 
ICPState indexing.

> I think for the time being you'd be better off keeping the simple
> array of icp states indexed by contiguous (barring cpu hotplug)
> cpu_index values, and dissociating the physical IDs (PIR == interrupt
> server number == DT id, IIUC) from the cpu_index.  Obviously you'll
> need helpers to convert between cpu_index and physical ID at the
> machine level.

If we keep the helper idea but transform them into XICSStateClass 
methods, we can define customs ops in xics_native and handle the 
conversion real-id <-> icp id there. I think a simple array under 
xics_native would do.

I will try that in v4.

Thanks,

C.