| Message ID | 20171123132955.1261-4-clg@kaod.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 23 Nov 2017 14:29:33 +0100 Cédric Le Goater <clg@kaod.org> wrote: > On sPAPR, the creation of the interrupt presenter depends on some of > the machine attributes. When the XIVE interrupt mode is available, > this will get more complex. So provide a machine-level helper to > isolate the process and hide the details to the sPAPR core realize > function. > Not sure it makes sense to introduce this helper that early in the series... what about folding it in patch 23 where it is really needed ? > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > hw/ppc/spapr.c | 14 ++++++++++++++ > hw/ppc/spapr_cpu_core.c | 2 +- > include/hw/ppc/spapr.h | 2 ++ > 3 files changed, 17 insertions(+), 1 deletion(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 174e7ff0678d..925cbd3c1bf4 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -3556,6 +3556,20 @@ static ICPState *spapr_icp_get(XICSFabric *xi, int vcpu_id) > return cpu ? ICP(cpu->intc) : NULL; > } > > +Object *spapr_icp_create(sPAPRMachineState *spapr, CPUState *cs, Error **errp) > +{ > + Error *local_err = NULL; > + Object *obj; > + > + obj = icp_create(cs, spapr->icp_type, XICS_FABRIC(spapr), &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + return NULL; > + } > + > + return obj; > +} > + > static void spapr_pic_print_info(InterruptStatsProvider *obj, > Monitor *mon) > { > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > index f7cc74512481..61a9850e688b 100644 > --- a/hw/ppc/spapr_cpu_core.c > +++ b/hw/ppc/spapr_cpu_core.c > @@ -122,7 +122,7 @@ static void spapr_cpu_core_realize_child(Object *child, > goto error; > } > > - cpu->intc = icp_create(cs, spapr->icp_type, XICS_FABRIC(spapr), &local_err); > + cpu->intc = spapr_icp_create(spapr, cs, &local_err); > if (local_err) { > goto error; > } > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index 9d21ca9bde3a..9da38de34277 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -707,4 +707,6 @@ void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg); > int spapr_vcpu_id(PowerPCCPU *cpu); > PowerPCCPU *spapr_find_cpu(int vcpu_id); > > +Object *spapr_icp_create(sPAPRMachineState *spapr, CPUState *cs, Error **errp); > + > #endif /* HW_SPAPR_H */
On 11/24/2017 10:09 AM, Greg Kurz wrote: > On Thu, 23 Nov 2017 14:29:33 +0100 > Cédric Le Goater <clg@kaod.org> wrote: > >> On sPAPR, the creation of the interrupt presenter depends on some of >> the machine attributes. When the XIVE interrupt mode is available, >> this will get more complex. So provide a machine-level helper to >> isolate the process and hide the details to the sPAPR core realize >> function. >> > > Not sure it makes sense to introduce this helper that early in the series... > what about folding it in patch 23 where it is really needed ? It does 'icp_type' and the 'xics_fabric' which are machine concepts around the sPAPR interrupt controller model. But yes, it could come before patch 23. May be not folded, though. C. >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> hw/ppc/spapr.c | 14 ++++++++++++++ >> hw/ppc/spapr_cpu_core.c | 2 +- >> include/hw/ppc/spapr.h | 2 ++ >> 3 files changed, 17 insertions(+), 1 deletion(-) >> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c >> index 174e7ff0678d..925cbd3c1bf4 100644 >> --- a/hw/ppc/spapr.c >> +++ b/hw/ppc/spapr.c >> @@ -3556,6 +3556,20 @@ static ICPState *spapr_icp_get(XICSFabric *xi, int vcpu_id) >> return cpu ? ICP(cpu->intc) : NULL; >> } >> >> +Object *spapr_icp_create(sPAPRMachineState *spapr, CPUState *cs, Error **errp) >> +{ >> + Error *local_err = NULL; >> + Object *obj; >> + >> + obj = icp_create(cs, spapr->icp_type, XICS_FABRIC(spapr), &local_err); >> + if (local_err) { >> + error_propagate(errp, local_err); >> + return NULL; >> + } >> + >> + return obj; >> +} >> + >> static void spapr_pic_print_info(InterruptStatsProvider *obj, >> Monitor *mon) >> { >> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c >> index f7cc74512481..61a9850e688b 100644 >> --- a/hw/ppc/spapr_cpu_core.c >> +++ b/hw/ppc/spapr_cpu_core.c >> @@ -122,7 +122,7 @@ static void spapr_cpu_core_realize_child(Object *child, >> goto error; >> } >> >> - cpu->intc = icp_create(cs, spapr->icp_type, XICS_FABRIC(spapr), &local_err); >> + cpu->intc = spapr_icp_create(spapr, cs, &local_err); >> if (local_err) { >> goto error; >> } >> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h >> index 9d21ca9bde3a..9da38de34277 100644 >> --- a/include/hw/ppc/spapr.h >> +++ b/include/hw/ppc/spapr.h >> @@ -707,4 +707,6 @@ void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg); >> int spapr_vcpu_id(PowerPCCPU *cpu); >> PowerPCCPU *spapr_find_cpu(int vcpu_id); >> >> +Object *spapr_icp_create(sPAPRMachineState *spapr, CPUState *cs, Error **errp); >> + >> #endif /* HW_SPAPR_H */ >
On Fri, 24 Nov 2017 12:26:21 +0000 Cédric Le Goater <clg@kaod.org> wrote: > On 11/24/2017 10:09 AM, Greg Kurz wrote: > > On Thu, 23 Nov 2017 14:29:33 +0100 > > Cédric Le Goater <clg@kaod.org> wrote: > > > >> On sPAPR, the creation of the interrupt presenter depends on some of > >> the machine attributes. When the XIVE interrupt mode is available, > >> this will get more complex. So provide a machine-level helper to > >> isolate the process and hide the details to the sPAPR core realize > >> function. > >> > > > > Not sure it makes sense to introduce this helper that early in the series... > > what about folding it in patch 23 where it is really needed ? > > It does 'icp_type' and the 'xics_fabric' which are machine concepts > around the sPAPR interrupt controller model. > Oh yes you're right, I guess I was looking at this from the perspective of my PHB hotplug series :) Hence, Reviewed-by: Greg Kurz <groug@kaod.org> > But yes, it could come before patch 23. May be not folded, though. > > C. > > > >> Signed-off-by: Cédric Le Goater <clg@kaod.org> > >> --- > >> hw/ppc/spapr.c | 14 ++++++++++++++ > >> hw/ppc/spapr_cpu_core.c | 2 +- > >> include/hw/ppc/spapr.h | 2 ++ > >> 3 files changed, 17 insertions(+), 1 deletion(-) > >> > >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > >> index 174e7ff0678d..925cbd3c1bf4 100644 > >> --- a/hw/ppc/spapr.c > >> +++ b/hw/ppc/spapr.c > >> @@ -3556,6 +3556,20 @@ static ICPState *spapr_icp_get(XICSFabric *xi, int vcpu_id) > >> return cpu ? ICP(cpu->intc) : NULL; > >> } > >> > >> +Object *spapr_icp_create(sPAPRMachineState *spapr, CPUState *cs, Error **errp) > >> +{ > >> + Error *local_err = NULL; > >> + Object *obj; > >> + > >> + obj = icp_create(cs, spapr->icp_type, XICS_FABRIC(spapr), &local_err); > >> + if (local_err) { > >> + error_propagate(errp, local_err); > >> + return NULL; > >> + } > >> + > >> + return obj; > >> +} > >> + > >> static void spapr_pic_print_info(InterruptStatsProvider *obj, > >> Monitor *mon) > >> { > >> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > >> index f7cc74512481..61a9850e688b 100644 > >> --- a/hw/ppc/spapr_cpu_core.c > >> +++ b/hw/ppc/spapr_cpu_core.c > >> @@ -122,7 +122,7 @@ static void spapr_cpu_core_realize_child(Object *child, > >> goto error; > >> } > >> > >> - cpu->intc = icp_create(cs, spapr->icp_type, XICS_FABRIC(spapr), &local_err); > >> + cpu->intc = spapr_icp_create(spapr, cs, &local_err); > >> if (local_err) { > >> goto error; > >> } > >> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > >> index 9d21ca9bde3a..9da38de34277 100644 > >> --- a/include/hw/ppc/spapr.h > >> +++ b/include/hw/ppc/spapr.h > >> @@ -707,4 +707,6 @@ void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg); > >> int spapr_vcpu_id(PowerPCCPU *cpu); > >> PowerPCCPU *spapr_find_cpu(int vcpu_id); > >> > >> +Object *spapr_icp_create(sPAPRMachineState *spapr, CPUState *cs, Error **errp); > >> + > >> #endif /* HW_SPAPR_H */ > > > >
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 174e7ff0678d..925cbd3c1bf4 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -3556,6 +3556,20 @@ static ICPState *spapr_icp_get(XICSFabric *xi, int vcpu_id) return cpu ? ICP(cpu->intc) : NULL; } +Object *spapr_icp_create(sPAPRMachineState *spapr, CPUState *cs, Error **errp) +{ + Error *local_err = NULL; + Object *obj; + + obj = icp_create(cs, spapr->icp_type, XICS_FABRIC(spapr), &local_err); + if (local_err) { + error_propagate(errp, local_err); + return NULL; + } + + return obj; +} + static void spapr_pic_print_info(InterruptStatsProvider *obj, Monitor *mon) { diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c index f7cc74512481..61a9850e688b 100644 --- a/hw/ppc/spapr_cpu_core.c +++ b/hw/ppc/spapr_cpu_core.c @@ -122,7 +122,7 @@ static void spapr_cpu_core_realize_child(Object *child, goto error; } - cpu->intc = icp_create(cs, spapr->icp_type, XICS_FABRIC(spapr), &local_err); + cpu->intc = spapr_icp_create(spapr, cs, &local_err); if (local_err) { goto error; } diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h index 9d21ca9bde3a..9da38de34277 100644 --- a/include/hw/ppc/spapr.h +++ b/include/hw/ppc/spapr.h @@ -707,4 +707,6 @@ void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg); int spapr_vcpu_id(PowerPCCPU *cpu); PowerPCCPU *spapr_find_cpu(int vcpu_id); +Object *spapr_icp_create(sPAPRMachineState *spapr, CPUState *cs, Error **errp); + #endif /* HW_SPAPR_H */
On sPAPR, the creation of the interrupt presenter depends on some of the machine attributes. When the XIVE interrupt mode is available, this will get more complex. So provide a machine-level helper to isolate the process and hide the details to the sPAPR core realize function. Signed-off-by: Cédric Le Goater <clg@kaod.org> --- hw/ppc/spapr.c | 14 ++++++++++++++ hw/ppc/spapr_cpu_core.c | 2 +- include/hw/ppc/spapr.h | 2 ++ 3 files changed, 17 insertions(+), 1 deletion(-)