This is the second step to abstract the IRQ 'server' number of the
XICS layer. Now that the prereq cleanups have been done in the
previous patch, we can move down the 'cpu_dt_id' to 'cpu_index'
mapping in the sPAPR machine handler.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/intc/xics_spapr.c    | 5 ++---
 hw/ppc/spapr.c          | 3 ++-
 hw/ppc/spapr_cpu_core.c | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
index 58f100d379cb..f05308b897f2 100644
--- a/hw/intc/xics_spapr.c
+++ b/hw/intc/xics_spapr.c
@@ -52,9 +52,8 @@ static target_ulong h_cppr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
 static target_ulong h_ipi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                           target_ulong opcode, target_ulong *args)
 {
-    target_ulong server = xics_get_cpu_index_by_dt_id(args[0]);
     target_ulong mfrr = args[1];
-    ICPState *icp = xics_icp_get(XICS_FABRIC(spapr), server);
+    ICPState *icp = xics_icp_get(XICS_FABRIC(spapr), args[0]);
 
     if (!icp) {
         return H_PARAMETER;
@@ -122,7 +121,7 @@ static void rtas_set_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr,
     }
 
     nr = rtas_ld(args, 0);
-    server = xics_get_cpu_index_by_dt_id(rtas_ld(args, 1));
+    server = rtas_ld(args, 1);
     priority = rtas_ld(args, 2);
 
     if (!ics_valid_irq(ics, nr) || !xics_icp_get(XICS_FABRIC(spapr), server)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 8aecea3dd10c..b9f7f8607869 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3024,9 +3024,10 @@ static void spapr_ics_resend(XICSFabric *dev)
     ics_resend(spapr->ics);
 }
 
-static ICPState *spapr_icp_get(XICSFabric *xi, int server)
+static ICPState *spapr_icp_get(XICSFabric *xi, int cpu_dt_id)
 {
     sPAPRMachineState *spapr = SPAPR_MACHINE(xi);
+    int server = xics_get_cpu_index_by_dt_id(cpu_dt_id);
 
     return (server < spapr->nr_servers) ? &spapr->icps[server] : NULL;
 }
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 7db61bd72476..4e1a99591d19 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -64,7 +64,7 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
 {
     CPUPPCState *env = &cpu->env;
     XICSFabric *xi = XICS_FABRIC(spapr);
-    ICPState *icp = xics_icp_get(xi, CPU(cpu)->cpu_index);
+    ICPState *icp = xics_icp_get(xi, cpu->cpu_dt_id);
 
     /* Set time-base frequency to 512 MHz */
     cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
-- 
2.7.4

On Wed, Mar 29, 2017 at 03:53:24PM +0200, Cédric Le Goater wrote:
> This is the second step to abstract the IRQ 'server' number of the
> XICS layer. Now that the prereq cleanups have been done in the
> previous patch, we can move down the 'cpu_dt_id' to 'cpu_index'
> mapping in the sPAPR machine handler.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/intc/xics_spapr.c    | 5 ++---
>  hw/ppc/spapr.c          | 3 ++-
>  hw/ppc/spapr_cpu_core.c | 2 +-
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> index 58f100d379cb..f05308b897f2 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -52,9 +52,8 @@ static target_ulong h_cppr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>  static target_ulong h_ipi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                            target_ulong opcode, target_ulong *args)
>  {
> -    target_ulong server = xics_get_cpu_index_by_dt_id(args[0]);
>      target_ulong mfrr = args[1];
> -    ICPState *icp = xics_icp_get(XICS_FABRIC(spapr), server);
> +    ICPState *icp = xics_icp_get(XICS_FABRIC(spapr), args[0]);
>  
>      if (!icp) {
>          return H_PARAMETER;
> @@ -122,7 +121,7 @@ static void rtas_set_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>      }
>  
>      nr = rtas_ld(args, 0);
> -    server = xics_get_cpu_index_by_dt_id(rtas_ld(args, 1));
> +    server = rtas_ld(args, 1);
>      priority = rtas_ld(args, 2);
>  
>      if (!ics_valid_irq(ics, nr) || !xics_icp_get(XICS_FABRIC(spapr), server)
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 8aecea3dd10c..b9f7f8607869 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3024,9 +3024,10 @@ static void spapr_ics_resend(XICSFabric *dev)
>      ics_resend(spapr->ics);
>  }
>  
> -static ICPState *spapr_icp_get(XICSFabric *xi, int server)
> +static ICPState *spapr_icp_get(XICSFabric *xi, int cpu_dt_id)
>  {
>      sPAPRMachineState *spapr = SPAPR_MACHINE(xi);
> +    int server = xics_get_cpu_index_by_dt_id(cpu_dt_id);

The idea is good, but this is a bad name (as it was in the original
version, too).  The whole point here is that the XICS server number
(as it appears in the ICS registers) is the input to this function,
and we no longer assume that is equal to cpu_index.

Seems we could just get the cpu object by dt_id here, then go to ICP(cpu->intc).

>      return (server < spapr->nr_servers) ? &spapr->icps[server] : NULL;
>  }
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 7db61bd72476..4e1a99591d19 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -64,7 +64,7 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
>  {
>      CPUPPCState *env = &cpu->env;
>      XICSFabric *xi = XICS_FABRIC(spapr);
> -    ICPState *icp = xics_icp_get(xi, CPU(cpu)->cpu_index);
> +    ICPState *icp = xics_icp_get(xi, cpu->cpu_dt_id);
>  
>      /* Set time-base frequency to 512 MHz */
>      cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);

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

On 03/30/2017 08:46 AM, David Gibson wrote:
> On Wed, Mar 29, 2017 at 03:53:24PM +0200, Cédric Le Goater wrote:
>> This is the second step to abstract the IRQ 'server' number of the
>> XICS layer. Now that the prereq cleanups have been done in the
>> previous patch, we can move down the 'cpu_dt_id' to 'cpu_index'
>> mapping in the sPAPR machine handler.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>> ---
>>  hw/intc/xics_spapr.c    | 5 ++---
>>  hw/ppc/spapr.c          | 3 ++-
>>  hw/ppc/spapr_cpu_core.c | 2 +-
>>  3 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
>> index 58f100d379cb..f05308b897f2 100644
>> --- a/hw/intc/xics_spapr.c
>> +++ b/hw/intc/xics_spapr.c
>> @@ -52,9 +52,8 @@ static target_ulong h_cppr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>>  static target_ulong h_ipi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>>                            target_ulong opcode, target_ulong *args)
>>  {
>> -    target_ulong server = xics_get_cpu_index_by_dt_id(args[0]);
>>      target_ulong mfrr = args[1];
>> -    ICPState *icp = xics_icp_get(XICS_FABRIC(spapr), server);
>> +    ICPState *icp = xics_icp_get(XICS_FABRIC(spapr), args[0]);
>>  
>>      if (!icp) {
>>          return H_PARAMETER;
>> @@ -122,7 +121,7 @@ static void rtas_set_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>>      }
>>  
>>      nr = rtas_ld(args, 0);
>> -    server = xics_get_cpu_index_by_dt_id(rtas_ld(args, 1));
>> +    server = rtas_ld(args, 1);
>>      priority = rtas_ld(args, 2);
>>  
>>      if (!ics_valid_irq(ics, nr) || !xics_icp_get(XICS_FABRIC(spapr), server)
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 8aecea3dd10c..b9f7f8607869 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -3024,9 +3024,10 @@ static void spapr_ics_resend(XICSFabric *dev)
>>      ics_resend(spapr->ics);
>>  }
>>  
>> -static ICPState *spapr_icp_get(XICSFabric *xi, int server)
>> +static ICPState *spapr_icp_get(XICSFabric *xi, int cpu_dt_id)
>>  {
>>      sPAPRMachineState *spapr = SPAPR_MACHINE(xi);
>> +    int server = xics_get_cpu_index_by_dt_id(cpu_dt_id);
> 
> The idea is good, but this is a bad name (as it was in the original
> version, too).  The whole point here is that the XICS server number
> (as it appears in the ICS registers) is the input to this function,
> and we no longer assume that is equal to cpu_index.
> 
> Seems we could just get the cpu object by dt_id here, then go to ICP(cpu->intc).

yes that would be nice and this is exactly what pnv does now, but 
this is only possible because the PnvICPState objects are allocated 
from under PnvCore. This is not the case for sPAPR yet.

Currently, when the sPAPR core are realized, we call spapr_cpu_init() 
which first gets its ICP with :

	xics_icp_get(xi, cpu->cpu_dt_id);

so we cannot use ICP(cpu->intc) in this XICSFabric handler, it is not
assigned yet. It only will be at the end of spapr_cpu_init() when 

	xics_cpu_setup(xi, cpu, icp);

is called. 


As suggested in an email this morning, I think we could allocate 
the ICP from under the sPAPR core if we knew which ICP type to use 
(KVM or not).  

For that, we could use a new XICSFabric handler :

	const char *icp_type(XICSFabric *xi)

or a machine attribute to get the type. The ICP type would be chosen 
in xics_system_init() a bit like it is done today but we would not 
create the ICP object. 

or we could use a machine helper (probably better) :  

	ICPState *spapr_icp_create(MachineState *machine);   

which would only do the ICP part of xics_system_init(). The ICS
object can be created later on, it is not a problem. 

We have kind of a chicken and egg problem with the Core and the 
ICP objects today that it would be nice to untangle. 

Suggestions ?


C. 


> 
>>      return (server < spapr->nr_servers) ? &spapr->icps[server] : NULL;
>>  }
>> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
>> index 7db61bd72476..4e1a99591d19 100644
>> --- a/hw/ppc/spapr_cpu_core.c
>> +++ b/hw/ppc/spapr_cpu_core.c
>> @@ -64,7 +64,7 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
>>  {
>>      CPUPPCState *env = &cpu->env;
>>      XICSFabric *xi = XICS_FABRIC(spapr);
>> -    ICPState *icp = xics_icp_get(xi, CPU(cpu)->cpu_index);
>> +    ICPState *icp = xics_icp_get(xi, cpu->cpu_dt_id);
>>  
>>      /* Set time-base frequency to 512 MHz */
>>      cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
> 

On 03/30/2017 03:04 PM, Cédric Le Goater wrote:
> On 03/30/2017 08:46 AM, David Gibson wrote:
>> On Wed, Mar 29, 2017 at 03:53:24PM +0200, Cédric Le Goater wrote:
>>> This is the second step to abstract the IRQ 'server' number of the
>>> XICS layer. Now that the prereq cleanups have been done in the
>>> previous patch, we can move down the 'cpu_dt_id' to 'cpu_index'
>>> mapping in the sPAPR machine handler.
>>>
>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>>> ---
>>>  hw/intc/xics_spapr.c    | 5 ++---
>>>  hw/ppc/spapr.c          | 3 ++-
>>>  hw/ppc/spapr_cpu_core.c | 2 +-
>>>  3 files changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
>>> index 58f100d379cb..f05308b897f2 100644
>>> --- a/hw/intc/xics_spapr.c
>>> +++ b/hw/intc/xics_spapr.c
>>> @@ -52,9 +52,8 @@ static target_ulong h_cppr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>>>  static target_ulong h_ipi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>>>                            target_ulong opcode, target_ulong *args)
>>>  {
>>> -    target_ulong server = xics_get_cpu_index_by_dt_id(args[0]);
>>>      target_ulong mfrr = args[1];
>>> -    ICPState *icp = xics_icp_get(XICS_FABRIC(spapr), server);
>>> +    ICPState *icp = xics_icp_get(XICS_FABRIC(spapr), args[0]);
>>>  
>>>      if (!icp) {
>>>          return H_PARAMETER;
>>> @@ -122,7 +121,7 @@ static void rtas_set_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>>>      }
>>>  
>>>      nr = rtas_ld(args, 0);
>>> -    server = xics_get_cpu_index_by_dt_id(rtas_ld(args, 1));
>>> +    server = rtas_ld(args, 1);
>>>      priority = rtas_ld(args, 2);
>>>  
>>>      if (!ics_valid_irq(ics, nr) || !xics_icp_get(XICS_FABRIC(spapr), server)
>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>> index 8aecea3dd10c..b9f7f8607869 100644
>>> --- a/hw/ppc/spapr.c
>>> +++ b/hw/ppc/spapr.c
>>> @@ -3024,9 +3024,10 @@ static void spapr_ics_resend(XICSFabric *dev)
>>>      ics_resend(spapr->ics);
>>>  }
>>>  
>>> -static ICPState *spapr_icp_get(XICSFabric *xi, int server)
>>> +static ICPState *spapr_icp_get(XICSFabric *xi, int cpu_dt_id)
>>>  {
>>>      sPAPRMachineState *spapr = SPAPR_MACHINE(xi);
>>> +    int server = xics_get_cpu_index_by_dt_id(cpu_dt_id);
>>
>> The idea is good, but this is a bad name (as it was in the original
>> version, too).  The whole point here is that the XICS server number
>> (as it appears in the ICS registers) is the input to this function,
>> and we no longer assume that is equal to cpu_index.
>>
>> Seems we could just get the cpu object by dt_id here, then go to ICP(cpu->intc).
> 
> yes that would be nice and this is exactly what pnv does now, but 
> this is only possible because the PnvICPState objects are allocated 
> from under PnvCore. This is not the case for sPAPR yet.
> 
> Currently, when the sPAPR core are realized, we call spapr_cpu_init() 
> which first gets its ICP with :
> 
> 	xics_icp_get(xi, cpu->cpu_dt_id);
> 
> so we cannot use ICP(cpu->intc) in this XICSFabric handler, it is not
> assigned yet. It only will be at the end of spapr_cpu_init() when 
> 
> 	xics_cpu_setup(xi, cpu, icp);
> 
> is called. 
> 
> 
> As suggested in an email this morning, I think we could allocate 
> the ICP from under the sPAPR core if we knew which ICP type to use 
> (KVM or not).  
> 
> For that, we could use a new XICSFabric handler :
> 
> 	const char *icp_type(XICSFabric *xi)
> 
> or a machine attribute to get the type. The ICP type would be chosen 
> in xics_system_init() a bit like it is done today but we would not 
> create the ICP object. 
> 
> or we could use a machine helper (probably better) :  
> 
> 	ICPState *spapr_icp_create(MachineState *machine);   
> 
> which would only do the ICP part of xics_system_init(). The ICS
> object can be created later on, it is not a problem. 
> 
> We have kind of a chicken and egg problem with the Core and the 
> ICP objects today that it would be nice to untangle. 
> 
> Suggestions ?

I have cooked a patch for this idea. It applies correctly in the 
v4 patchset between 'PATCH v4 2/9' and 'PATCH v4 3/9'. I will 
send as a follow up of 'PATCH v4 2/9'. 

Thanks,

C.  


> 
> C. 
> 
> 
>>
>>>      return (server < spapr->nr_servers) ? &spapr->icps[server] : NULL;
>>>  }
>>> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
>>> index 7db61bd72476..4e1a99591d19 100644
>>> --- a/hw/ppc/spapr_cpu_core.c
>>> +++ b/hw/ppc/spapr_cpu_core.c
>>> @@ -64,7 +64,7 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
>>>  {
>>>      CPUPPCState *env = &cpu->env;
>>>      XICSFabric *xi = XICS_FABRIC(spapr);
>>> -    ICPState *icp = xics_icp_get(xi, CPU(cpu)->cpu_index);
>>> +    ICPState *icp = xics_icp_get(xi, cpu->cpu_dt_id);
>>>  
>>>      /* Set time-base frequency to 512 MHz */
>>>      cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
>>
> 

On Thu, Mar 30, 2017 at 03:04:47PM +0200, Cédric Le Goater wrote:
> On 03/30/2017 08:46 AM, David Gibson wrote:
> > On Wed, Mar 29, 2017 at 03:53:24PM +0200, Cédric Le Goater wrote:
> >> This is the second step to abstract the IRQ 'server' number of the
> >> XICS layer. Now that the prereq cleanups have been done in the
> >> previous patch, we can move down the 'cpu_dt_id' to 'cpu_index'
> >> mapping in the sPAPR machine handler.
> >>
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> >> ---
> >>  hw/intc/xics_spapr.c    | 5 ++---
> >>  hw/ppc/spapr.c          | 3 ++-
> >>  hw/ppc/spapr_cpu_core.c | 2 +-
> >>  3 files changed, 5 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> >> index 58f100d379cb..f05308b897f2 100644
> >> --- a/hw/intc/xics_spapr.c
> >> +++ b/hw/intc/xics_spapr.c
> >> @@ -52,9 +52,8 @@ static target_ulong h_cppr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >>  static target_ulong h_ipi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >>                            target_ulong opcode, target_ulong *args)
> >>  {
> >> -    target_ulong server = xics_get_cpu_index_by_dt_id(args[0]);
> >>      target_ulong mfrr = args[1];
> >> -    ICPState *icp = xics_icp_get(XICS_FABRIC(spapr), server);
> >> +    ICPState *icp = xics_icp_get(XICS_FABRIC(spapr), args[0]);
> >>  
> >>      if (!icp) {
> >>          return H_PARAMETER;
> >> @@ -122,7 +121,7 @@ static void rtas_set_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >>      }
> >>  
> >>      nr = rtas_ld(args, 0);
> >> -    server = xics_get_cpu_index_by_dt_id(rtas_ld(args, 1));
> >> +    server = rtas_ld(args, 1);
> >>      priority = rtas_ld(args, 2);
> >>  
> >>      if (!ics_valid_irq(ics, nr) || !xics_icp_get(XICS_FABRIC(spapr), server)
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index 8aecea3dd10c..b9f7f8607869 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -3024,9 +3024,10 @@ static void spapr_ics_resend(XICSFabric *dev)
> >>      ics_resend(spapr->ics);
> >>  }
> >>  
> >> -static ICPState *spapr_icp_get(XICSFabric *xi, int server)
> >> +static ICPState *spapr_icp_get(XICSFabric *xi, int cpu_dt_id)
> >>  {
> >>      sPAPRMachineState *spapr = SPAPR_MACHINE(xi);
> >> +    int server = xics_get_cpu_index_by_dt_id(cpu_dt_id);
> > 
> > The idea is good, but this is a bad name (as it was in the original
> > version, too).  The whole point here is that the XICS server number
> > (as it appears in the ICS registers) is the input to this function,
> > and we no longer assume that is equal to cpu_index.
> > 
> > Seems we could just get the cpu object by dt_id here, then go to ICP(cpu->intc).
> 
> yes that would be nice and this is exactly what pnv does now, but 
> this is only possible because the PnvICPState objects are allocated 
> from under PnvCore. This is not the case for sPAPR yet.
> 
> Currently, when the sPAPR core are realized, we call spapr_cpu_init() 
> which first gets its ICP with :
> 
> 	xics_icp_get(xi, cpu->cpu_dt_id);
> 
> so we cannot use ICP(cpu->intc) in this XICSFabric handler, it is not
> assigned yet. It only will be at the end of spapr_cpu_init() when 
> 
> 	xics_cpu_setup(xi, cpu, icp);
> 
> is called.

Uh.. but spapr_cpu_init() has the spapr pointer and can obviously get
the cpu_index.  So it could look up the right ICP in the array easily
enough instead of using xics_icp_get().

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

Today, all the ICPs are created before the CPUs, stored in an array
under the sPAPR machine and linked to the CPU when the core threads
are realized. This modeling brings some complexity when a lookup in
the array is required and it can be simplified by allocating the ICPs
when the CPUs are.

This is the purpose of this proposal which introduces a new 'icp_type'
field under the machine and creates the ICP objects of the right type
(KVM or not) before the PowerPCCPU object are.

This change allows more cleanups : the removal of the icps array under
the sPAPR machine and the removal xics_get_cpu_index_by_dt_id() helper.

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

 Tested with KVM and TCG. migration is fine.

 hw/intc/xics.c          | 11 -----------
 hw/ppc/spapr.c          | 47 ++++++++++++++---------------------------------
 hw/ppc/spapr_cpu_core.c | 18 ++++++++++++++----
 include/hw/ppc/spapr.h  |  2 +-
 include/hw/ppc/xics.h   |  2 --
 5 files changed, 29 insertions(+), 51 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index 56fe70cd10e9..d4428b41b03a 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -38,17 +38,6 @@
 #include "monitor/monitor.h"
 #include "hw/intc/intc.h"
 
-int xics_get_cpu_index_by_dt_id(int cpu_dt_id)
-{
-    PowerPCCPU *cpu = ppc_get_vcpu_by_dt_id(cpu_dt_id);
-
-    if (cpu) {
-        return cpu->parent_obj.cpu_index;
-    }
-
-    return -1;
-}
-
 void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu)
 {
     CPUState *cs = CPU(cpu);
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index b9f7f8607869..18d8fe7458c1 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -103,7 +103,6 @@ static int try_create_xics(sPAPRMachineState *spapr, const char *type_ics,
     XICSFabric *xi = XICS_FABRIC(spapr);
     Error *err = NULL, *local_err = NULL;
     ICSState *ics = NULL;
-    int i;
 
     ics = ICS_SIMPLE(object_new(type_ics));
     object_property_add_child(OBJECT(spapr), "ics", OBJECT(ics), NULL);
@@ -112,34 +111,14 @@ static int try_create_xics(sPAPRMachineState *spapr, const char *type_ics,
     object_property_set_bool(OBJECT(ics), true, "realized", &local_err);
     error_propagate(&err, local_err);
     if (err) {
-        goto error;
+        error_propagate(errp, err);
+        return -1;
     }
 
-    spapr->icps = g_malloc0(nr_servers * sizeof(ICPState));
     spapr->nr_servers = nr_servers;
-
-    for (i = 0; i < nr_servers; i++) {
-        ICPState *icp = &spapr->icps[i];
-
-        object_initialize(icp, sizeof(*icp), type_icp);
-        object_property_add_child(OBJECT(spapr), "icp[*]", OBJECT(icp), NULL);
-        object_property_add_const_link(OBJECT(icp), "xics", OBJECT(xi), NULL);
-        object_property_set_bool(OBJECT(icp), true, "realized", &err);
-        if (err) {
-            goto error;
-        }
-        object_unref(OBJECT(icp));
-    }
-
     spapr->ics = ics;
+    spapr->icp_type = type_icp;
     return 0;
-
-error:
-    error_propagate(errp, err);
-    if (ics) {
-        object_unparent(OBJECT(ics));
-    }
-    return -1;
 }
 
 static int xics_system_init(MachineState *machine,
@@ -1366,9 +1345,10 @@ static int spapr_post_load(void *opaque, int version_id)
     int err = 0;
 
     if (!object_dynamic_cast(OBJECT(spapr->ics), TYPE_ICS_KVM)) {
-        int i;
-        for (i = 0; i < spapr->nr_servers; i++) {
-            icp_resend(&spapr->icps[i]);
+        CPUState *cs;
+        CPU_FOREACH(cs) {
+            PowerPCCPU *cpu = POWERPC_CPU(cs);
+            icp_resend(ICP(cpu->intc));
         }
     }
 
@@ -3026,20 +3006,21 @@ static void spapr_ics_resend(XICSFabric *dev)
 
 static ICPState *spapr_icp_get(XICSFabric *xi, int cpu_dt_id)
 {
-    sPAPRMachineState *spapr = SPAPR_MACHINE(xi);
-    int server = xics_get_cpu_index_by_dt_id(cpu_dt_id);
+    PowerPCCPU *cpu = ppc_get_vcpu_by_dt_id(cpu_dt_id);
 
-    return (server < spapr->nr_servers) ? &spapr->icps[server] : NULL;
+    return cpu ? ICP(cpu->intc) : NULL;
 }
 
 static void spapr_pic_print_info(InterruptStatsProvider *obj,
                                  Monitor *mon)
 {
     sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
-    int i;
+    CPUState *cs;
+
+    CPU_FOREACH(cs) {
+        PowerPCCPU *cpu = POWERPC_CPU(cs);
 
-    for (i = 0; i < spapr->nr_servers; i++) {
-        icp_pic_print_info(&spapr->icps[i], mon);
+        icp_pic_print_info(ICP(cpu->intc), mon);
     }
 
     ics_pic_print_info(spapr->ics, mon);
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 4e1a99591d19..c80eb7c34f9f 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -63,8 +63,6 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
                            Error **errp)
 {
     CPUPPCState *env = &cpu->env;
-    XICSFabric *xi = XICS_FABRIC(spapr);
-    ICPState *icp = xics_icp_get(xi, cpu->cpu_dt_id);
 
     /* Set time-base frequency to 512 MHz */
     cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
@@ -82,8 +80,6 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
         }
     }
 
-    xics_cpu_setup(xi, cpu, icp);
-
     qemu_register_reset(spapr_cpu_reset, cpu);
     spapr_cpu_reset(cpu);
 }
@@ -143,18 +139,32 @@ static void spapr_cpu_core_realize_child(Object *child, Error **errp)
     sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
     CPUState *cs = CPU(child);
     PowerPCCPU *cpu = POWERPC_CPU(cs);
+    Object *obj;
+
+    obj = object_new(spapr->icp_type);
+    object_property_add_child(OBJECT(spapr), "icp[*]", obj, NULL);
+    object_property_add_const_link(obj, "xics", OBJECT(spapr), NULL);
+    object_property_set_bool(obj, true, "realized", &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
 
     object_property_set_bool(child, true, "realized", &local_err);
     if (local_err) {
+        object_unparent(obj);
         error_propagate(errp, local_err);
         return;
     }
 
     spapr_cpu_init(spapr, cpu, &local_err);
     if (local_err) {
+        object_unparent(obj);
         error_propagate(errp, local_err);
         return;
     }
+
+    xics_cpu_setup(XICS_FABRIC(spapr), cpu, ICP(obj));
 }
 
 static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 808aac870359..db3d4acb18a6 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -96,7 +96,7 @@ struct sPAPRMachineState {
     MemoryHotplugState hotplug_memory;
 
     uint32_t nr_servers;
-    ICPState *icps;
+    const char *icp_type;
 };
 
 #define H_SUCCESS         0
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 5e0244447fcd..88e0f021b168 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -172,8 +172,6 @@ void xics_cpu_setup(XICSFabric *xi, PowerPCCPU *cpu, ICPState *icp);
 void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu);
 
 /* Internal XICS interfaces */
-int xics_get_cpu_index_by_dt_id(int cpu_dt_id);
-
 void icp_set_cppr(ICPState *icp, uint8_t cppr);
 void icp_set_mfrr(ICPState *icp, uint8_t mfrr);
 uint32_t icp_accept(ICPState *ss);
-- 
2.7.4

On 03/30/2017 05:08 PM, Cédric Le Goater wrote:
> Today, all the ICPs are created before the CPUs, stored in an array
> under the sPAPR machine and linked to the CPU when the core threads
> are realized. This modeling brings some complexity when a lookup in
> the array is required and it can be simplified by allocating the ICPs
> when the CPUs are.
> 
> This is the purpose of this proposal which introduces a new 'icp_type'
> field under the machine and creates the ICP objects of the right type
> (KVM or not) before the PowerPCCPU object are.
> 
> This change allows more cleanups : the removal of the icps array under
> the sPAPR machine and the removal xics_get_cpu_index_by_dt_id() helper.
I forgot :                         ^
                                 of the

and some more below.  
 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> 
>  Tested with KVM and TCG. migration is fine.
> 
>  hw/intc/xics.c          | 11 -----------
>  hw/ppc/spapr.c          | 47 ++++++++++++++---------------------------------
>  hw/ppc/spapr_cpu_core.c | 18 ++++++++++++++----
>  include/hw/ppc/spapr.h  |  2 +-
>  include/hw/ppc/xics.h   |  2 --
>  5 files changed, 29 insertions(+), 51 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 56fe70cd10e9..d4428b41b03a 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -38,17 +38,6 @@
>  #include "monitor/monitor.h"
>  #include "hw/intc/intc.h"
>  
> -int xics_get_cpu_index_by_dt_id(int cpu_dt_id)
> -{
> -    PowerPCCPU *cpu = ppc_get_vcpu_by_dt_id(cpu_dt_id);
> -
> -    if (cpu) {
> -        return cpu->parent_obj.cpu_index;
> -    }
> -
> -    return -1;
> -}
> -
>  void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu)
>  {
>      CPUState *cs = CPU(cpu);
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index b9f7f8607869..18d8fe7458c1 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -103,7 +103,6 @@ static int try_create_xics(sPAPRMachineState *spapr, const char *type_ics,
>      XICSFabric *xi = XICS_FABRIC(spapr);
>      Error *err = NULL, *local_err = NULL;
>      ICSState *ics = NULL;
> -    int i;
>  
>      ics = ICS_SIMPLE(object_new(type_ics));
>      object_property_add_child(OBJECT(spapr), "ics", OBJECT(ics), NULL);
> @@ -112,34 +111,14 @@ static int try_create_xics(sPAPRMachineState *spapr, const char *type_ics,
>      object_property_set_bool(OBJECT(ics), true, "realized", &local_err);
>      error_propagate(&err, local_err);
>      if (err) {
> -        goto error;
> +        error_propagate(errp, err);
> +        return -1;
>      }
>  
> -    spapr->icps = g_malloc0(nr_servers * sizeof(ICPState));
>      spapr->nr_servers = nr_servers;
> -
> -    for (i = 0; i < nr_servers; i++) {
> -        ICPState *icp = &spapr->icps[i];
> -
> -        object_initialize(icp, sizeof(*icp), type_icp);
> -        object_property_add_child(OBJECT(spapr), "icp[*]", OBJECT(icp), NULL);
> -        object_property_add_const_link(OBJECT(icp), "xics", OBJECT(xi), NULL);
> -        object_property_set_bool(OBJECT(icp), true, "realized", &err);
> -        if (err) {
> -            goto error;
> -        }
> -        object_unref(OBJECT(icp));
> -    }
> -
>      spapr->ics = ics;
> +    spapr->icp_type = type_icp;
>      return 0;
> -
> -error:
> -    error_propagate(errp, err);
> -    if (ics) {
> -        object_unparent(OBJECT(ics));
> -    }
> -    return -1;
>  }
>  
>  static int xics_system_init(MachineState *machine,
> @@ -1366,9 +1345,10 @@ static int spapr_post_load(void *opaque, int version_id)
>      int err = 0;
>  
>      if (!object_dynamic_cast(OBJECT(spapr->ics), TYPE_ICS_KVM)) {
> -        int i;
> -        for (i = 0; i < spapr->nr_servers; i++) {
> -            icp_resend(&spapr->icps[i]);
> +        CPUState *cs;
> +        CPU_FOREACH(cs) {
> +            PowerPCCPU *cpu = POWERPC_CPU(cs);
> +            icp_resend(ICP(cpu->intc));
>          }
>      }
>  
> @@ -3026,20 +3006,21 @@ static void spapr_ics_resend(XICSFabric *dev)
>  
>  static ICPState *spapr_icp_get(XICSFabric *xi, int cpu_dt_id)
>  {
> -    sPAPRMachineState *spapr = SPAPR_MACHINE(xi);
> -    int server = xics_get_cpu_index_by_dt_id(cpu_dt_id);
> +    PowerPCCPU *cpu = ppc_get_vcpu_by_dt_id(cpu_dt_id);
>  
> -    return (server < spapr->nr_servers) ? &spapr->icps[server] : NULL;
> +    return cpu ? ICP(cpu->intc) : NULL;
>  }
>  
>  static void spapr_pic_print_info(InterruptStatsProvider *obj,
>                                   Monitor *mon)
>  {
>      sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> -    int i;
> +    CPUState *cs;
> +
> +    CPU_FOREACH(cs) {
> +        PowerPCCPU *cpu = POWERPC_CPU(cs);
>  
> -    for (i = 0; i < spapr->nr_servers; i++) {
> -        icp_pic_print_info(&spapr->icps[i], mon);
> +        icp_pic_print_info(ICP(cpu->intc), mon);
>      }
>  
>      ics_pic_print_info(spapr->ics, mon);
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 4e1a99591d19..c80eb7c34f9f 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -63,8 +63,6 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
>                             Error **errp)
>  {
>      CPUPPCState *env = &cpu->env;
> -    XICSFabric *xi = XICS_FABRIC(spapr);
> -    ICPState *icp = xics_icp_get(xi, cpu->cpu_dt_id);
>  
>      /* Set time-base frequency to 512 MHz */
>      cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
> @@ -82,8 +80,6 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
>          }
>      }
>  
> -    xics_cpu_setup(xi, cpu, icp);
> -
>      qemu_register_reset(spapr_cpu_reset, cpu);
>      spapr_cpu_reset(cpu);
>  }
> @@ -143,18 +139,32 @@ static void spapr_cpu_core_realize_child(Object *child, Error **errp)
>      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>      CPUState *cs = CPU(child);
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    Object *obj;
> +
> +    obj = object_new(spapr->icp_type);
> +    object_property_add_child(OBJECT(spapr), "icp[*]", obj, NULL);
> +    object_property_add_const_link(obj, "xics", OBJECT(spapr), NULL);

That's wrong. It should be :

    object_property_add_child(OBJECT(cpu), "icp", obj, NULL);
    object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);

but you have the idea.

Thanks,

C. 

> +    object_property_set_bool(obj, true, "realized", &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      object_property_set_bool(child, true, "realized", &local_err);
>      if (local_err) {
> +        object_unparent(obj);
>          error_propagate(errp, local_err);
>          return;
>      }
>  
>      spapr_cpu_init(spapr, cpu, &local_err);
>      if (local_err) {
> +        object_unparent(obj);
>          error_propagate(errp, local_err);
>          return;
>      }
> +
> +    xics_cpu_setup(XICS_FABRIC(spapr), cpu, ICP(obj));
>  }
>  
>  static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 808aac870359..db3d4acb18a6 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -96,7 +96,7 @@ struct sPAPRMachineState {
>      MemoryHotplugState hotplug_memory;
>  
>      uint32_t nr_servers;
> -    ICPState *icps;
> +    const char *icp_type;
>  };
>  
>  #define H_SUCCESS         0
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 5e0244447fcd..88e0f021b168 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -172,8 +172,6 @@ void xics_cpu_setup(XICSFabric *xi, PowerPCCPU *cpu, ICPState *icp);
>  void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu);
>  
>  /* Internal XICS interfaces */
> -int xics_get_cpu_index_by_dt_id(int cpu_dt_id);
> -
>  void icp_set_cppr(ICPState *icp, uint8_t cppr);
>  void icp_set_mfrr(ICPState *icp, uint8_t mfrr);
>  uint32_t icp_accept(ICPState *ss);
> 

On Thu, Mar 30, 2017 at 05:08:04PM +0200, Cédric Le Goater wrote:
> Today, all the ICPs are created before the CPUs, stored in an array
> under the sPAPR machine and linked to the CPU when the core threads
> are realized. This modeling brings some complexity when a lookup in
> the array is required and it can be simplified by allocating the ICPs
> when the CPUs are.
> 
> This is the purpose of this proposal which introduces a new 'icp_type'
> field under the machine and creates the ICP objects of the right type
> (KVM or not) before the PowerPCCPU object are.
> 
> This change allows more cleanups : the removal of the icps array under
> the sPAPR machine and the removal xics_get_cpu_index_by_dt_id() helper.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> 
>  Tested with KVM and TCG. migration is fine.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

But please resend with the whole series, I've lost track of which
versions are which.

> 
>  hw/intc/xics.c          | 11 -----------
>  hw/ppc/spapr.c          | 47 ++++++++++++++---------------------------------
>  hw/ppc/spapr_cpu_core.c | 18 ++++++++++++++----
>  include/hw/ppc/spapr.h  |  2 +-
>  include/hw/ppc/xics.h   |  2 --
>  5 files changed, 29 insertions(+), 51 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 56fe70cd10e9..d4428b41b03a 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -38,17 +38,6 @@
>  #include "monitor/monitor.h"
>  #include "hw/intc/intc.h"
>  
> -int xics_get_cpu_index_by_dt_id(int cpu_dt_id)
> -{
> -    PowerPCCPU *cpu = ppc_get_vcpu_by_dt_id(cpu_dt_id);
> -
> -    if (cpu) {
> -        return cpu->parent_obj.cpu_index;
> -    }
> -
> -    return -1;
> -}
> -
>  void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu)
>  {
>      CPUState *cs = CPU(cpu);
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index b9f7f8607869..18d8fe7458c1 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -103,7 +103,6 @@ static int try_create_xics(sPAPRMachineState *spapr, const char *type_ics,
>      XICSFabric *xi = XICS_FABRIC(spapr);
>      Error *err = NULL, *local_err = NULL;
>      ICSState *ics = NULL;
> -    int i;
>  
>      ics = ICS_SIMPLE(object_new(type_ics));
>      object_property_add_child(OBJECT(spapr), "ics", OBJECT(ics), NULL);
> @@ -112,34 +111,14 @@ static int try_create_xics(sPAPRMachineState *spapr, const char *type_ics,
>      object_property_set_bool(OBJECT(ics), true, "realized", &local_err);
>      error_propagate(&err, local_err);
>      if (err) {
> -        goto error;
> +        error_propagate(errp, err);
> +        return -1;
>      }
>  
> -    spapr->icps = g_malloc0(nr_servers * sizeof(ICPState));
>      spapr->nr_servers = nr_servers;
> -
> -    for (i = 0; i < nr_servers; i++) {
> -        ICPState *icp = &spapr->icps[i];
> -
> -        object_initialize(icp, sizeof(*icp), type_icp);
> -        object_property_add_child(OBJECT(spapr), "icp[*]", OBJECT(icp), NULL);
> -        object_property_add_const_link(OBJECT(icp), "xics", OBJECT(xi), NULL);
> -        object_property_set_bool(OBJECT(icp), true, "realized", &err);
> -        if (err) {
> -            goto error;
> -        }
> -        object_unref(OBJECT(icp));
> -    }
> -
>      spapr->ics = ics;
> +    spapr->icp_type = type_icp;
>      return 0;
> -
> -error:
> -    error_propagate(errp, err);
> -    if (ics) {
> -        object_unparent(OBJECT(ics));
> -    }
> -    return -1;
>  }
>  
>  static int xics_system_init(MachineState *machine,
> @@ -1366,9 +1345,10 @@ static int spapr_post_load(void *opaque, int version_id)
>      int err = 0;
>  
>      if (!object_dynamic_cast(OBJECT(spapr->ics), TYPE_ICS_KVM)) {
> -        int i;
> -        for (i = 0; i < spapr->nr_servers; i++) {
> -            icp_resend(&spapr->icps[i]);
> +        CPUState *cs;
> +        CPU_FOREACH(cs) {
> +            PowerPCCPU *cpu = POWERPC_CPU(cs);
> +            icp_resend(ICP(cpu->intc));
>          }
>      }
>  
> @@ -3026,20 +3006,21 @@ static void spapr_ics_resend(XICSFabric *dev)
>  
>  static ICPState *spapr_icp_get(XICSFabric *xi, int cpu_dt_id)
>  {
> -    sPAPRMachineState *spapr = SPAPR_MACHINE(xi);
> -    int server = xics_get_cpu_index_by_dt_id(cpu_dt_id);
> +    PowerPCCPU *cpu = ppc_get_vcpu_by_dt_id(cpu_dt_id);
>  
> -    return (server < spapr->nr_servers) ? &spapr->icps[server] : NULL;
> +    return cpu ? ICP(cpu->intc) : NULL;
>  }
>  
>  static void spapr_pic_print_info(InterruptStatsProvider *obj,
>                                   Monitor *mon)
>  {
>      sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> -    int i;
> +    CPUState *cs;
> +
> +    CPU_FOREACH(cs) {
> +        PowerPCCPU *cpu = POWERPC_CPU(cs);
>  
> -    for (i = 0; i < spapr->nr_servers; i++) {
> -        icp_pic_print_info(&spapr->icps[i], mon);
> +        icp_pic_print_info(ICP(cpu->intc), mon);
>      }
>  
>      ics_pic_print_info(spapr->ics, mon);
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 4e1a99591d19..c80eb7c34f9f 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -63,8 +63,6 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
>                             Error **errp)
>  {
>      CPUPPCState *env = &cpu->env;
> -    XICSFabric *xi = XICS_FABRIC(spapr);
> -    ICPState *icp = xics_icp_get(xi, cpu->cpu_dt_id);
>  
>      /* Set time-base frequency to 512 MHz */
>      cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
> @@ -82,8 +80,6 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
>          }
>      }
>  
> -    xics_cpu_setup(xi, cpu, icp);
> -
>      qemu_register_reset(spapr_cpu_reset, cpu);
>      spapr_cpu_reset(cpu);
>  }
> @@ -143,18 +139,32 @@ static void spapr_cpu_core_realize_child(Object *child, Error **errp)
>      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>      CPUState *cs = CPU(child);
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    Object *obj;
> +
> +    obj = object_new(spapr->icp_type);
> +    object_property_add_child(OBJECT(spapr), "icp[*]", obj, NULL);
> +    object_property_add_const_link(obj, "xics", OBJECT(spapr), NULL);
> +    object_property_set_bool(obj, true, "realized", &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      object_property_set_bool(child, true, "realized", &local_err);
>      if (local_err) {
> +        object_unparent(obj);
>          error_propagate(errp, local_err);
>          return;
>      }
>  
>      spapr_cpu_init(spapr, cpu, &local_err);
>      if (local_err) {
> +        object_unparent(obj);
>          error_propagate(errp, local_err);
>          return;
>      }
> +
> +    xics_cpu_setup(XICS_FABRIC(spapr), cpu, ICP(obj));
>  }
>  
>  static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 808aac870359..db3d4acb18a6 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -96,7 +96,7 @@ struct sPAPRMachineState {
>      MemoryHotplugState hotplug_memory;
>  
>      uint32_t nr_servers;
> -    ICPState *icps;
> +    const char *icp_type;
>  };
>  
>  #define H_SUCCESS         0
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 5e0244447fcd..88e0f021b168 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -172,8 +172,6 @@ void xics_cpu_setup(XICSFabric *xi, PowerPCCPU *cpu, ICPState *icp);
>  void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu);
>  
>  /* Internal XICS interfaces */
> -int xics_get_cpu_index_by_dt_id(int cpu_dt_id);
> -
>  void icp_set_cppr(ICPState *icp, uint8_t cppr);
>  void icp_set_mfrr(ICPState *icp, uint8_t mfrr);
>  uint32_t icp_accept(ICPState *ss);

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

On 04/02/2017 10:25 AM, David Gibson wrote:
> On Thu, Mar 30, 2017 at 05:08:04PM +0200, Cédric Le Goater wrote:
>> Today, all the ICPs are created before the CPUs, stored in an array
>> under the sPAPR machine and linked to the CPU when the core threads
>> are realized. This modeling brings some complexity when a lookup in
>> the array is required and it can be simplified by allocating the ICPs
>> when the CPUs are.
>>
>> This is the purpose of this proposal which introduces a new 'icp_type'
>> field under the machine and creates the ICP objects of the right type
>> (KVM or not) before the PowerPCCPU object are.
>>
>> This change allows more cleanups : the removal of the icps array under
>> the sPAPR machine and the removal xics_get_cpu_index_by_dt_id() helper.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>
>>  Tested with KVM and TCG. migration is fine.
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> 
> But please resend with the whole series, I've lost track of which
> versions are which.

sure. I was sending material to clarify what I was saying. I will 
resend tomorrow.

Thanks,

C. 

>>
>>  hw/intc/xics.c          | 11 -----------
>>  hw/ppc/spapr.c          | 47 ++++++++++++++---------------------------------
>>  hw/ppc/spapr_cpu_core.c | 18 ++++++++++++++----
>>  include/hw/ppc/spapr.h  |  2 +-
>>  include/hw/ppc/xics.h   |  2 --
>>  5 files changed, 29 insertions(+), 51 deletions(-)
>>
>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
>> index 56fe70cd10e9..d4428b41b03a 100644
>> --- a/hw/intc/xics.c
>> +++ b/hw/intc/xics.c
>> @@ -38,17 +38,6 @@
>>  #include "monitor/monitor.h"
>>  #include "hw/intc/intc.h"
>>  
>> -int xics_get_cpu_index_by_dt_id(int cpu_dt_id)
>> -{
>> -    PowerPCCPU *cpu = ppc_get_vcpu_by_dt_id(cpu_dt_id);
>> -
>> -    if (cpu) {
>> -        return cpu->parent_obj.cpu_index;
>> -    }
>> -
>> -    return -1;
>> -}
>> -
>>  void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu)
>>  {
>>      CPUState *cs = CPU(cpu);
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index b9f7f8607869..18d8fe7458c1 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -103,7 +103,6 @@ static int try_create_xics(sPAPRMachineState *spapr, const char *type_ics,
>>      XICSFabric *xi = XICS_FABRIC(spapr);
>>      Error *err = NULL, *local_err = NULL;
>>      ICSState *ics = NULL;
>> -    int i;
>>  
>>      ics = ICS_SIMPLE(object_new(type_ics));
>>      object_property_add_child(OBJECT(spapr), "ics", OBJECT(ics), NULL);
>> @@ -112,34 +111,14 @@ static int try_create_xics(sPAPRMachineState *spapr, const char *type_ics,
>>      object_property_set_bool(OBJECT(ics), true, "realized", &local_err);
>>      error_propagate(&err, local_err);
>>      if (err) {
>> -        goto error;
>> +        error_propagate(errp, err);
>> +        return -1;
>>      }
>>  
>> -    spapr->icps = g_malloc0(nr_servers * sizeof(ICPState));
>>      spapr->nr_servers = nr_servers;
>> -
>> -    for (i = 0; i < nr_servers; i++) {
>> -        ICPState *icp = &spapr->icps[i];
>> -
>> -        object_initialize(icp, sizeof(*icp), type_icp);
>> -        object_property_add_child(OBJECT(spapr), "icp[*]", OBJECT(icp), NULL);
>> -        object_property_add_const_link(OBJECT(icp), "xics", OBJECT(xi), NULL);
>> -        object_property_set_bool(OBJECT(icp), true, "realized", &err);
>> -        if (err) {
>> -            goto error;
>> -        }
>> -        object_unref(OBJECT(icp));
>> -    }
>> -
>>      spapr->ics = ics;
>> +    spapr->icp_type = type_icp;
>>      return 0;
>> -
>> -error:
>> -    error_propagate(errp, err);
>> -    if (ics) {
>> -        object_unparent(OBJECT(ics));
>> -    }
>> -    return -1;
>>  }
>>  
>>  static int xics_system_init(MachineState *machine,
>> @@ -1366,9 +1345,10 @@ static int spapr_post_load(void *opaque, int version_id)
>>      int err = 0;
>>  
>>      if (!object_dynamic_cast(OBJECT(spapr->ics), TYPE_ICS_KVM)) {
>> -        int i;
>> -        for (i = 0; i < spapr->nr_servers; i++) {
>> -            icp_resend(&spapr->icps[i]);
>> +        CPUState *cs;
>> +        CPU_FOREACH(cs) {
>> +            PowerPCCPU *cpu = POWERPC_CPU(cs);
>> +            icp_resend(ICP(cpu->intc));
>>          }
>>      }
>>  
>> @@ -3026,20 +3006,21 @@ static void spapr_ics_resend(XICSFabric *dev)
>>  
>>  static ICPState *spapr_icp_get(XICSFabric *xi, int cpu_dt_id)
>>  {
>> -    sPAPRMachineState *spapr = SPAPR_MACHINE(xi);
>> -    int server = xics_get_cpu_index_by_dt_id(cpu_dt_id);
>> +    PowerPCCPU *cpu = ppc_get_vcpu_by_dt_id(cpu_dt_id);
>>  
>> -    return (server < spapr->nr_servers) ? &spapr->icps[server] : NULL;
>> +    return cpu ? ICP(cpu->intc) : NULL;
>>  }
>>  
>>  static void spapr_pic_print_info(InterruptStatsProvider *obj,
>>                                   Monitor *mon)
>>  {
>>      sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
>> -    int i;
>> +    CPUState *cs;
>> +
>> +    CPU_FOREACH(cs) {
>> +        PowerPCCPU *cpu = POWERPC_CPU(cs);
>>  
>> -    for (i = 0; i < spapr->nr_servers; i++) {
>> -        icp_pic_print_info(&spapr->icps[i], mon);
>> +        icp_pic_print_info(ICP(cpu->intc), mon);
>>      }
>>  
>>      ics_pic_print_info(spapr->ics, mon);
>> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
>> index 4e1a99591d19..c80eb7c34f9f 100644
>> --- a/hw/ppc/spapr_cpu_core.c
>> +++ b/hw/ppc/spapr_cpu_core.c
>> @@ -63,8 +63,6 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
>>                             Error **errp)
>>  {
>>      CPUPPCState *env = &cpu->env;
>> -    XICSFabric *xi = XICS_FABRIC(spapr);
>> -    ICPState *icp = xics_icp_get(xi, cpu->cpu_dt_id);
>>  
>>      /* Set time-base frequency to 512 MHz */
>>      cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
>> @@ -82,8 +80,6 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
>>          }
>>      }
>>  
>> -    xics_cpu_setup(xi, cpu, icp);
>> -
>>      qemu_register_reset(spapr_cpu_reset, cpu);
>>      spapr_cpu_reset(cpu);
>>  }
>> @@ -143,18 +139,32 @@ static void spapr_cpu_core_realize_child(Object *child, Error **errp)
>>      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>>      CPUState *cs = CPU(child);
>>      PowerPCCPU *cpu = POWERPC_CPU(cs);
>> +    Object *obj;
>> +
>> +    obj = object_new(spapr->icp_type);
>> +    object_property_add_child(OBJECT(spapr), "icp[*]", obj, NULL);
>> +    object_property_add_const_link(obj, "xics", OBJECT(spapr), NULL);
>> +    object_property_set_bool(obj, true, "realized", &local_err);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>>  
>>      object_property_set_bool(child, true, "realized", &local_err);
>>      if (local_err) {
>> +        object_unparent(obj);
>>          error_propagate(errp, local_err);
>>          return;
>>      }
>>  
>>      spapr_cpu_init(spapr, cpu, &local_err);
>>      if (local_err) {
>> +        object_unparent(obj);
>>          error_propagate(errp, local_err);
>>          return;
>>      }
>> +
>> +    xics_cpu_setup(XICS_FABRIC(spapr), cpu, ICP(obj));
>>  }
>>  
>>  static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index 808aac870359..db3d4acb18a6 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -96,7 +96,7 @@ struct sPAPRMachineState {
>>      MemoryHotplugState hotplug_memory;
>>  
>>      uint32_t nr_servers;
>> -    ICPState *icps;
>> +    const char *icp_type;
>>  };
>>  
>>  #define H_SUCCESS         0
>> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
>> index 5e0244447fcd..88e0f021b168 100644
>> --- a/include/hw/ppc/xics.h
>> +++ b/include/hw/ppc/xics.h
>> @@ -172,8 +172,6 @@ void xics_cpu_setup(XICSFabric *xi, PowerPCCPU *cpu, ICPState *icp);
>>  void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu);
>>  
>>  /* Internal XICS interfaces */
>> -int xics_get_cpu_index_by_dt_id(int cpu_dt_id);
>> -
>>  void icp_set_cppr(ICPState *icp, uint8_t cppr);
>>  void icp_set_mfrr(ICPState *icp, uint8_t mfrr);
>>  uint32_t icp_accept(ICPState *ss);
>