Re: [Qemu-devel] [PATCH v4 3/6] xics: setup cpu at realize time

["Cédric Le Goater" <clg@kaod.org>]

On 06/08/2017 03:42 PM, Greg Kurz wrote:
> Until recently, spapr used to allocate ICPState objects for the lifetime
> of the machine. They would only be associated to vCPUs in xics_cpu_setup()
> when plugging a CPU core.
> 
> Now that ICPState objects have the same lifecycle as vCPUs, it is
> possible to associate them during realization.
> 
> This patch hence open-codes xics_cpu_setup() in icp_realize(). The vCPU
> is passed as a property. Note that vCPU now needs to be realized first
> for the IRQs to be allocated. It also needs to resetted before ICPState
> realization in order to synchronize with KVM.
> 
> Since ICPState objects are freed when unrealized, xics_cpu_destroy() isn't
> needed anymore and can be safely dropped.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

I don't like the "ICP_PROP_CPU" define. A part from that :

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

C.


> ---
> v4: - renamed property from "cs" to "cpu" and make it a macro (ICP_PROP_CPU)
>     - fixed copy/paste mistake in error message in icp_realize()
> ---
>  hw/intc/xics.c          |   76 ++++++++++++++++++++---------------------------
>  hw/ppc/pnv_core.c       |   18 +++++------
>  hw/ppc/spapr_cpu_core.c |   23 ++++++--------
>  include/hw/ppc/xics.h   |    3 +-
>  4 files changed, 51 insertions(+), 69 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index f74a96e932d7..fdbfddffeea5 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -38,50 +38,6 @@
>  #include "monitor/monitor.h"
>  #include "hw/intc/intc.h"
>  
> -void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu)
> -{
> -    CPUState *cs = CPU(cpu);
> -    ICPState *icp = ICP(cpu->intc);
> -
> -    assert(icp);
> -    assert(cs == icp->cs);
> -
> -    icp->output = NULL;
> -    icp->cs = NULL;
> -}
> -
> -void xics_cpu_setup(XICSFabric *xi, PowerPCCPU *cpu, ICPState *icp)
> -{
> -    CPUState *cs = CPU(cpu);
> -    CPUPPCState *env = &cpu->env;
> -    ICPStateClass *icpc;
> -
> -    assert(icp);
> -
> -    cpu->intc = OBJECT(icp);
> -    icp->cs = cs;
> -
> -    icpc = ICP_GET_CLASS(icp);
> -    if (icpc->cpu_setup) {
> -        icpc->cpu_setup(icp, cpu);
> -    }
> -
> -    switch (PPC_INPUT(env)) {
> -    case PPC_FLAGS_INPUT_POWER7:
> -        icp->output = env->irq_inputs[POWER7_INPUT_INT];
> -        break;
> -
> -    case PPC_FLAGS_INPUT_970:
> -        icp->output = env->irq_inputs[PPC970_INPUT_INT];
> -        break;
> -
> -    default:
> -        error_report("XICS interrupt controller does not support this CPU "
> -                     "bus model");
> -        abort();
> -    }
> -}
> -
>  void icp_pic_print_info(ICPState *icp, Monitor *mon)
>  {
>      int cpu_index = icp->cs ? icp->cs->cpu_index : -1;
> @@ -343,6 +299,8 @@ static void icp_realize(DeviceState *dev, Error **errp)
>  {
>      ICPState *icp = ICP(dev);
>      ICPStateClass *icpc = ICP_GET_CLASS(dev);
> +    PowerPCCPU *cpu;
> +    CPUPPCState *env;
>      Object *obj;
>      Error *err = NULL;
>  
> @@ -355,6 +313,36 @@ static void icp_realize(DeviceState *dev, Error **errp)
>  
>      icp->xics = XICS_FABRIC(obj);
>  
> +    obj = object_property_get_link(OBJECT(dev), ICP_PROP_CPU, &err);
> +    if (!obj) {
> +        error_setg(errp, "%s: required link '" ICP_PROP_CPU "' not found: %s",
> +                   __func__, error_get_pretty(err));
> +        return;
> +    }
> +
> +    cpu = POWERPC_CPU(obj);
> +    cpu->intc = OBJECT(icp);
> +    icp->cs = CPU(obj);
> +
> +    if (icpc->cpu_setup) {
> +        icpc->cpu_setup(icp, cpu);
> +    }
> +
> +    env = &cpu->env;
> +    switch (PPC_INPUT(env)) {
> +    case PPC_FLAGS_INPUT_POWER7:
> +        icp->output = env->irq_inputs[POWER7_INPUT_INT];
> +        break;
> +
> +    case PPC_FLAGS_INPUT_970:
> +        icp->output = env->irq_inputs[PPC970_INPUT_INT];
> +        break;
> +
> +    default:
> +        error_setg(errp, "XICS interrupt controller does not support this CPU bus model");
> +        return;
> +    }
> +
>      if (icpc->realize) {
>          icpc->realize(icp, errp);
>      }
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index 0b6e72950ca3..c7b00b610c1e 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -118,20 +118,20 @@ static void pnv_core_realize_child(Object *child, XICSFabric *xi, Error **errp)
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
>      Object *obj;
>  
> -    obj = object_new(TYPE_PNV_ICP);
> -    object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort);
> -    object_unref(obj);
> -    object_property_add_const_link(obj, ICP_PROP_XICS, OBJECT(xi),
> -                                   &error_abort);
> -    object_property_set_bool(obj, true, "realized", &local_err);
> +    object_property_set_bool(child, true, "realized", &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
>      }
>  
> -    object_property_set_bool(child, true, "realized", &local_err);
> +    obj = object_new(TYPE_PNV_ICP);
> +    object_property_add_child(child, "icp", obj, NULL);
> +    object_unref(obj);
> +    object_property_add_const_link(obj, ICP_PROP_XICS, OBJECT(xi),
> +                                   &error_abort);
> +    object_property_add_const_link(obj, ICP_PROP_CPU, child, &error_abort);
> +    object_property_set_bool(obj, true, "realized", &local_err);
>      if (local_err) {
> -        object_unparent(obj);
>          error_propagate(errp, local_err);
>          return;
>      }
> @@ -142,8 +142,6 @@ static void pnv_core_realize_child(Object *child, XICSFabric *xi, Error **errp)
>          error_propagate(errp, local_err);
>          return;
>      }
> -
> -    xics_cpu_setup(xi, cpu, ICP(obj));
>  }
>  
>  static void pnv_core_realize(DeviceState *dev, Error **errp)
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index e81879c7cad7..9fb896b407db 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -53,9 +53,6 @@ static void spapr_cpu_reset(void *opaque)
>  
>  static void spapr_cpu_destroy(PowerPCCPU *cpu)
>  {
> -    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> -
> -    xics_cpu_destroy(XICS_FABRIC(spapr), cpu);
>      qemu_unregister_reset(spapr_cpu_reset, cpu);
>  }
>  
> @@ -140,29 +137,29 @@ 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;
> +    Object *obj = NULL;
>  
> -    obj = object_new(spapr->icp_type);
> -    object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort);
> -    object_unref(obj);
> -    object_property_add_const_link(obj, ICP_PROP_XICS, OBJECT(spapr),
> -                                   &error_abort);
> -    object_property_set_bool(obj, true, "realized", &local_err);
> +    object_property_set_bool(child, true, "realized", &local_err);
>      if (local_err) {
>          goto error;
>      }
>  
> -    object_property_set_bool(child, true, "realized", &local_err);
> +    spapr_cpu_init(spapr, cpu, &local_err);
>      if (local_err) {
>          goto error;
>      }
>  
> -    spapr_cpu_init(spapr, cpu, &local_err);
> +    obj = object_new(spapr->icp_type);
> +    object_property_add_child(child, "icp", obj, &error_abort);
> +    object_unref(obj);
> +    object_property_add_const_link(obj, ICP_PROP_XICS, OBJECT(spapr),
> +                                   &error_abort);
> +    object_property_add_const_link(obj, ICP_PROP_CPU, child, &error_abort);
> +    object_property_set_bool(obj, true, "realized", &local_err);
>      if (local_err) {
>          goto error;
>      }
>  
> -    xics_cpu_setup(XICS_FABRIC(spapr), cpu, ICP(obj));
>      return;
>  
>  error:
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 797df82fefc0..37b8fb1e8817 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -87,6 +87,7 @@ struct ICPState {
>  };
>  
>  #define ICP_PROP_XICS "xics"
> +#define ICP_PROP_CPU "cpu"
>  
>  struct PnvICPState {
>      ICPState parent_obj;
> @@ -187,8 +188,6 @@ void spapr_dt_xics(int nr_servers, void *fdt, uint32_t phandle);
>  
>  qemu_irq xics_get_qirq(XICSFabric *xi, int irq);
>  ICPState *xics_icp_get(XICSFabric *xi, int server);
> -void xics_cpu_setup(XICSFabric *xi, PowerPCCPU *cpu, ICPState *icp);
> -void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu);
>  
>  /* Internal XICS interfaces */
>  void icp_set_cppr(ICPState *icp, uint8_t cppr);
>