From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37982) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V6dxF-0002po-TD for qemu-devel@nongnu.org; Tue, 06 Aug 2013 05:53:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V6dx9-0004Ai-LJ for qemu-devel@nongnu.org; Tue, 06 Aug 2013 05:53:25 -0400 Message-ID: <5200C789.5030802@suse.de> Date: Tue, 06 Aug 2013 11:53:13 +0200 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1375777673-20274-1-git-send-email-aik@ozlabs.ru> <1375777673-20274-6-git-send-email-aik@ozlabs.ru> In-Reply-To: <1375777673-20274-6-git-send-email-aik@ozlabs.ru> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 5/6] xics: split to xics and xics-common List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexey Kardashevskiy Cc: Anthony Liguori , Alexander Graf , qemu-devel@nongnu.org, qemu-ppc@nongnu.org, Paul Mackerras , David Gibson Am 06.08.2013 10:27, schrieb Alexey Kardashevskiy: > The upcoming XICS-KVM support will use bits of emulated XICS code. > So this introduces new level of hierarchy - "xics-common" class. Both > emulated XICS and XICS-KVM will inherit from it and override class > callbacks when required. >=20 > The new "xics-common" class implements: > 1. replaces static "nr_irqs" and "nr_servers" properties with > the dynamic ones and adds callbacks to be executed when properties > are set. > 2. xics_cpu_setup() callback renamed to xics_common_cpu_setup() as > it is a common part for both XICS'es > 3. xics_reset() renamed to xics_common_reset() for the same reason. >=20 > The emulated XICS changes: > 1. instance_init() callback is replaced with "nr_irqs" property callbac= k > as it creates ICS which needs the nr_irqs property set. > 2. the part of xics_realize() which creates ICPs is moved to > the "nr_servers" property callback as realize() is too late to > create/initialize devices and instance_init() is too early to create > devices as the number of child devices comes via the "nr_servers" > property. > 3. added ics_initfn() which does a little part of what xics_realize() d= id. >=20 > Signed-off-by: Alexey Kardashevskiy This looks really good, except for error handling and introspection support - minor nits. > --- > hw/intc/xics.c | 190 +++++++++++++++++++++++++++++++++++-------= -------- > include/hw/ppc/xics.h | 11 ++- > 2 files changed, 143 insertions(+), 58 deletions(-) >=20 > diff --git a/hw/intc/xics.c b/hw/intc/xics.c > index 20840e3..95865aa 100644 > --- a/hw/intc/xics.c > +++ b/hw/intc/xics.c > @@ -30,6 +30,112 @@ > #include "hw/ppc/spapr.h" > #include "hw/ppc/xics.h" > #include "qemu/error-report.h" > +#include "qapi/visitor.h" > + > +/* > + * XICS Common class - parent for emulated XICS and KVM-XICS > + */ > + > +static void xics_common_cpu_setup(XICSState *icp, PowerPCCPU *cpu) > +{ > + CPUState *cs =3D CPU(cpu); > + CPUPPCState *env =3D &cpu->env; > + ICPState *ss =3D &icp->ss[cs->cpu_index]; > + > + assert(cs->cpu_index < icp->nr_servers); > + > + switch (PPC_INPUT(env)) { > + case PPC_FLAGS_INPUT_POWER7: > + ss->output =3D env->irq_inputs[POWER7_INPUT_INT]; > + break; > + > + case PPC_FLAGS_INPUT_970: > + ss->output =3D env->irq_inputs[PPC970_INPUT_INT]; > + break; > + > + default: > + error_report("XICS interrupt controller does not support this = CPU " > + "bus model"); Indentation is off. > + abort(); I previously wondered whether it may make sense to add Error **errp argument to avoid the abort, but this is only called from the machine init, right? > + } > +} > + > +void xics_prop_set_nr_irqs(Object *obj, struct Visitor *v, > + void *opaque, const char *name, struct Erro= r **errp) You should be able to drop both "struct"s. > +{ > + XICSState *icp =3D XICS_COMMON(obj); > + XICSStateClass *info =3D XICS_COMMON_GET_CLASS(icp); > + Error *error =3D NULL; > + int64_t value; > + > + visit_type_int(v, &value, name, &error); > + if (error) { > + error_propagate(errp, error); > + return; > + } > + > + assert(info->set_nr_irqs); > + assert(!icp->nr_irqs); For reasons of simplicity you're only implementing these as one-off setters. I think that's acceptable, but since someone can easily try to set this property via QMP qom-set you should do error_setg() rather than assert() then. Asserting the class methods is fine as they are not user-changeable. > + assert(!icp->ics); > + info->set_nr_irqs(icp, value); > +} > + > +void xics_prop_set_nr_servers(Object *obj, struct Visitor *v, > + void *opaque, const char *name, struct E= rror **errp) > +{ > + XICSState *icp =3D XICS_COMMON(obj); > + XICSStateClass *info =3D XICS_COMMON_GET_CLASS(icp); > + Error *error =3D NULL; > + int64_t value; > + > + visit_type_int(v, &value, name, &error); > + if (error) { > + error_propagate(errp, error); > + return; > + } > + > + assert(info->set_nr_servers); > + assert(!icp->nr_servers); Ditto. > + info->set_nr_servers(icp, value); > +} > + > +static void xics_common_initfn(Object *obj) > +{ > + object_property_add(obj, "nr_irqs", "int", > + NULL, xics_prop_set_nr_irqs, NULL, NULL, NULL)= ; > + object_property_add(obj, "nr_servers", "int", > + NULL, xics_prop_set_nr_servers, NULL, NULL, NU= LL); Since this property is visible in the QOM tree, it would be nice to implement trivial getters returns the value from the state fields. > +} > + > +static void xics_common_reset(DeviceState *d) > +{ > + XICSState *icp =3D XICS_COMMON(d); > + int i; > + > + for (i =3D 0; i < icp->nr_servers; i++) { > + device_reset(DEVICE(&icp->ss[i])); > + } > + > + device_reset(DEVICE(icp->ics)); > +} > + > +static void xics_common_class_init(ObjectClass *oc, void *data) > +{ > + DeviceClass *dc =3D DEVICE_CLASS(oc); > + XICSStateClass *xsc =3D XICS_COMMON_CLASS(oc); > + > + dc->reset =3D xics_common_reset; > + xsc->cpu_setup =3D xics_common_cpu_setup; > +} > + > +static const TypeInfo xics_common_info =3D { > + .name =3D TYPE_XICS_COMMON, > + .parent =3D TYPE_SYS_BUS_DEVICE, > + .instance_size =3D sizeof(XICSState), > + .class_size =3D sizeof(XICSStateClass), > + .instance_init =3D xics_common_initfn, > + .class_init =3D xics_common_class_init, > +}; > =20 > /* > * ICP: Presentation layer > @@ -443,6 +549,13 @@ static const VMStateDescription vmstate_ics =3D { > }, > }; > =20 > +static void ics_initfn(Object *obj) > +{ > + ICSState *ics =3D ICS(obj); > + > + ics->offset =3D XICS_IRQ_BASE; > +} > + > static int ics_realize(DeviceState *dev) > { > ICSState *ics =3D ICS(dev); > @@ -472,6 +585,7 @@ static const TypeInfo ics_info =3D { > .instance_size =3D sizeof(ICSState), > .class_init =3D ics_class_init, > .class_size =3D sizeof(ICSStateClass), > + .instance_init =3D ics_initfn, > }; > =20 > /* > @@ -651,47 +765,32 @@ static void rtas_int_on(PowerPCCPU *cpu, sPAPREnv= ironment *spapr, > /* > * XICS > */ > +void xics_set_nr_irqs(XICSState *icp, uint32_t nr_irqs) > +{ > + icp->ics =3D ICS(object_new(TYPE_ICS)); > + object_property_add_child(OBJECT(icp), "ics", OBJECT(icp->ics), NU= LL); Why create this single object in the setter? Can't you just create this in the common type's instance_init similar to before but using object_initialize() instead of object_new() and object_property_set_bool() in the realizefn? NULL above also shows that your callback should probably get an Error **errp argument to propagate any errors to the property and its callers. Common split-off, setters and hooks look good otherwise. Thanks, Andreas > + icp->ics->icp =3D icp; > + icp->nr_irqs =3D icp->ics->nr_irqs =3D nr_irqs; > +} > =20 > -static void xics_reset(DeviceState *d) > +void xics_set_nr_servers(XICSState *icp, uint32_t nr_servers) > { > - XICSState *icp =3D XICS(d); > int i; > =20 > + icp->nr_servers =3D nr_servers; > + > + icp->ss =3D g_malloc0(icp->nr_servers*sizeof(ICPState)); > for (i =3D 0; i < icp->nr_servers; i++) { > - device_reset(DEVICE(&icp->ss[i])); > - } > - > - device_reset(DEVICE(icp->ics)); > -} > - > -static void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu) > -{ > - CPUState *cs =3D CPU(cpu); > - CPUPPCState *env =3D &cpu->env; > - ICPState *ss =3D &icp->ss[cs->cpu_index]; > - > - assert(cs->cpu_index < icp->nr_servers); > - > - switch (PPC_INPUT(env)) { > - case PPC_FLAGS_INPUT_POWER7: > - ss->output =3D env->irq_inputs[POWER7_INPUT_INT]; > - break; > - > - case PPC_FLAGS_INPUT_970: > - ss->output =3D env->irq_inputs[PPC970_INPUT_INT]; > - break; > - > - default: > - error_report("XICS interrupt controller does not support this = CPU " > - "bus model"); > - abort(); > + char buffer[32]; > + object_initialize(&icp->ss[i], TYPE_ICP); > + snprintf(buffer, sizeof(buffer), "icp[%d]", i); > + object_property_add_child(OBJECT(icp), buffer, OBJECT(&icp->ss= [i]), NULL); > } > } > =20 > static void xics_realize(DeviceState *dev, Error **errp) > { > XICSState *icp =3D XICS(dev); > - ICSState *ics =3D icp->ics; > Error *error =3D NULL; > int i; > =20 > @@ -706,9 +805,6 @@ static void xics_realize(DeviceState *dev, Error **= errp) > spapr_register_hypercall(H_XIRR, h_xirr); > spapr_register_hypercall(H_EOI, h_eoi); > =20 > - ics->nr_irqs =3D icp->nr_irqs; > - ics->offset =3D XICS_IRQ_BASE; > - ics->icp =3D icp; > object_property_set_bool(OBJECT(icp->ics), true, "realized", &erro= r); > if (error) { > error_propagate(errp, error); > @@ -716,12 +812,7 @@ static void xics_realize(DeviceState *dev, Error *= *errp) > } > =20 > assert(icp->nr_servers); > - icp->ss =3D g_malloc0(icp->nr_servers*sizeof(ICPState)); > for (i =3D 0; i < icp->nr_servers; i++) { > - char buffer[32]; > - object_initialize(&icp->ss[i], TYPE_ICP); > - snprintf(buffer, sizeof(buffer), "icp[%d]", i); > - object_property_add_child(OBJECT(icp), buffer, OBJECT(&icp->ss= [i]), NULL); > object_property_set_bool(OBJECT(&icp->ss[i]), true, "realized"= , &error); > if (error) { > error_propagate(errp, error); > @@ -730,42 +821,27 @@ static void xics_realize(DeviceState *dev, Error = **errp) > } > } > =20 > -static void xics_initfn(Object *obj) > -{ > - XICSState *xics =3D XICS(obj); > - > - xics->ics =3D ICS(object_new(TYPE_ICS)); > - object_property_add_child(obj, "ics", OBJECT(xics->ics), NULL); > -} > - > -static Property xics_properties[] =3D { > - DEFINE_PROP_UINT32("nr_servers", XICSState, nr_servers, -1), > - DEFINE_PROP_UINT32("nr_irqs", XICSState, nr_irqs, -1), > - DEFINE_PROP_END_OF_LIST(), > -}; > - > static void xics_class_init(ObjectClass *oc, void *data) > { > DeviceClass *dc =3D DEVICE_CLASS(oc); > XICSStateClass *xsc =3D XICS_CLASS(oc); > =20 > dc->realize =3D xics_realize; > - dc->props =3D xics_properties; > - dc->reset =3D xics_reset; > - xsc->cpu_setup =3D xics_cpu_setup; > + xsc->set_nr_irqs =3D xics_set_nr_irqs; > + xsc->set_nr_servers =3D xics_set_nr_servers; > } > =20 > static const TypeInfo xics_info =3D { > .name =3D TYPE_XICS, > - .parent =3D TYPE_SYS_BUS_DEVICE, > + .parent =3D TYPE_XICS_COMMON, > .instance_size =3D sizeof(XICSState), > .class_size =3D sizeof(XICSStateClass), > .class_init =3D xics_class_init, > - .instance_init =3D xics_initfn, > }; > =20 > static void xics_register_types(void) > { > + type_register_static(&xics_common_info); > type_register_static(&xics_info); > type_register_static(&ics_info); > type_register_static(&icp_info); > diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h > index 90ecaf8..1066f69 100644 > --- a/include/hw/ppc/xics.h > +++ b/include/hw/ppc/xics.h > @@ -29,11 +29,18 @@ > =20 > #include "hw/sysbus.h" > =20 > +#define TYPE_XICS_COMMON "xics-common" > +#define XICS_COMMON(obj) OBJECT_CHECK(XICSState, (obj), TYPE_XICS_COMM= ON) > + > #define TYPE_XICS "xics" > #define XICS(obj) OBJECT_CHECK(XICSState, (obj), TYPE_XICS) > =20 > +#define XICS_COMMON_CLASS(klass) \ > + OBJECT_CLASS_CHECK(XICSStateClass, (klass), TYPE_XICS_COMMON) > #define XICS_CLASS(klass) \ > OBJECT_CLASS_CHECK(XICSStateClass, (klass), TYPE_XICS) > +#define XICS_COMMON_GET_CLASS(obj) \ > + OBJECT_GET_CLASS(XICSStateClass, (obj), TYPE_XICS_COMMON) > #define XICS_GET_CLASS(obj) \ > OBJECT_GET_CLASS(XICSStateClass, (obj), TYPE_XICS) > =20 > @@ -58,6 +65,8 @@ struct XICSStateClass { > DeviceClass parent_class; > =20 > void (*cpu_setup)(XICSState *icp, PowerPCCPU *cpu); > + void (*set_nr_irqs)(XICSState *icp, uint32_t nr_irqs); > + void (*set_nr_servers)(XICSState *icp, uint32_t nr_servers); > }; > =20 > struct XICSState { > @@ -138,7 +147,7 @@ void xics_set_irq_type(XICSState *icp, int irq, boo= l lsi); > =20 > static inline void xics_dispatch_cpu_setup(XICSState *icp, PowerPCCPU = *cpu) > { > - XICSStateClass *info =3D XICS_GET_CLASS(icp); > + XICSStateClass *info =3D XICS_COMMON_GET_CLASS(icp); > =20 > assert(info->cpu_setup); > info->cpu_setup(icp, cpu); >=20 --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg