Re: [Qemu-devel] [PATCH v4 2+/9] spapr: allocate the ICPState object from under sPAPRCPUCore

[Cédric Le Goater]

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 <address@hidden>
>> ---
>>
>>  Tested with KVM and TCG. migration is fine.
> 
> Reviewed-by: David Gibson <address@hidden>
> 
> 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);
>