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 <gr...@kaod.org>
I don't like the "ICP_PROP_CPU" define. A part from that :
Reviewed-by: Cédric Le Goater <c...@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);
>
