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

[Cédric Le Goater]

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 <address@hidden>
> ---
> 
>  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);
>