| Message ID | 1375777673-20274-6-git-send-email-aik@ozlabs.ru |
|---|---|
| State | New |
| Headers | show |
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. > > 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. > > The emulated XICS changes: > 1. instance_init() callback is replaced with "nr_irqs" property callback > 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() did. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> 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(-) > > 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 = CPU(cpu); > + CPUPPCState *env = &cpu->env; > + ICPState *ss = &icp->ss[cs->cpu_index]; > + > + assert(cs->cpu_index < icp->nr_servers); > + > + switch (PPC_INPUT(env)) { > + case PPC_FLAGS_INPUT_POWER7: > + ss->output = env->irq_inputs[POWER7_INPUT_INT]; > + break; > + > + case PPC_FLAGS_INPUT_970: > + ss->output = 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 Error **errp) You should be able to drop both "struct"s. > +{ > + XICSState *icp = XICS_COMMON(obj); > + XICSStateClass *info = XICS_COMMON_GET_CLASS(icp); > + Error *error = 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 Error **errp) > +{ > + XICSState *icp = XICS_COMMON(obj); > + XICSStateClass *info = XICS_COMMON_GET_CLASS(icp); > + Error *error = 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, NULL); 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 = XICS_COMMON(d); > + int i; > + > + for (i = 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 = DEVICE_CLASS(oc); > + XICSStateClass *xsc = XICS_COMMON_CLASS(oc); > + > + dc->reset = xics_common_reset; > + xsc->cpu_setup = xics_common_cpu_setup; > +} > + > +static const TypeInfo xics_common_info = { > + .name = TYPE_XICS_COMMON, > + .parent = TYPE_SYS_BUS_DEVICE, > + .instance_size = sizeof(XICSState), > + .class_size = sizeof(XICSStateClass), > + .instance_init = xics_common_initfn, > + .class_init = xics_common_class_init, > +}; > > /* > * ICP: Presentation layer > @@ -443,6 +549,13 @@ static const VMStateDescription vmstate_ics = { > }, > }; > > +static void ics_initfn(Object *obj) > +{ > + ICSState *ics = ICS(obj); > + > + ics->offset = XICS_IRQ_BASE; > +} > + > static int ics_realize(DeviceState *dev) > { > ICSState *ics = ICS(dev); > @@ -472,6 +585,7 @@ static const TypeInfo ics_info = { > .instance_size = sizeof(ICSState), > .class_init = ics_class_init, > .class_size = sizeof(ICSStateClass), > + .instance_init = ics_initfn, > }; > > /* > @@ -651,47 +765,32 @@ static void rtas_int_on(PowerPCCPU *cpu, sPAPREnvironment *spapr, > /* > * XICS > */ > +void xics_set_nr_irqs(XICSState *icp, uint32_t nr_irqs) > +{ > + icp->ics = ICS(object_new(TYPE_ICS)); > + object_property_add_child(OBJECT(icp), "ics", OBJECT(icp->ics), NULL); 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 = icp; > + icp->nr_irqs = icp->ics->nr_irqs = nr_irqs; > +} > > -static void xics_reset(DeviceState *d) > +void xics_set_nr_servers(XICSState *icp, uint32_t nr_servers) > { > - XICSState *icp = XICS(d); > int i; > > + icp->nr_servers = nr_servers; > + > + icp->ss = g_malloc0(icp->nr_servers*sizeof(ICPState)); > for (i = 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 = CPU(cpu); > - CPUPPCState *env = &cpu->env; > - ICPState *ss = &icp->ss[cs->cpu_index]; > - > - assert(cs->cpu_index < icp->nr_servers); > - > - switch (PPC_INPUT(env)) { > - case PPC_FLAGS_INPUT_POWER7: > - ss->output = env->irq_inputs[POWER7_INPUT_INT]; > - break; > - > - case PPC_FLAGS_INPUT_970: > - ss->output = 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); > } > } > > static void xics_realize(DeviceState *dev, Error **errp) > { > XICSState *icp = XICS(dev); > - ICSState *ics = icp->ics; > Error *error = NULL; > int i; > > @@ -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); > > - ics->nr_irqs = icp->nr_irqs; > - ics->offset = XICS_IRQ_BASE; > - ics->icp = icp; > object_property_set_bool(OBJECT(icp->ics), true, "realized", &error); > if (error) { > error_propagate(errp, error); > @@ -716,12 +812,7 @@ static void xics_realize(DeviceState *dev, Error **errp) > } > > assert(icp->nr_servers); > - icp->ss = g_malloc0(icp->nr_servers*sizeof(ICPState)); > for (i = 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) > } > } > > -static void xics_initfn(Object *obj) > -{ > - XICSState *xics = XICS(obj); > - > - xics->ics = ICS(object_new(TYPE_ICS)); > - object_property_add_child(obj, "ics", OBJECT(xics->ics), NULL); > -} > - > -static Property xics_properties[] = { > - 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 = DEVICE_CLASS(oc); > XICSStateClass *xsc = XICS_CLASS(oc); > > dc->realize = xics_realize; > - dc->props = xics_properties; > - dc->reset = xics_reset; > - xsc->cpu_setup = xics_cpu_setup; > + xsc->set_nr_irqs = xics_set_nr_irqs; > + xsc->set_nr_servers = xics_set_nr_servers; > } > > static const TypeInfo xics_info = { > .name = TYPE_XICS, > - .parent = TYPE_SYS_BUS_DEVICE, > + .parent = TYPE_XICS_COMMON, > .instance_size = sizeof(XICSState), > .class_size = sizeof(XICSStateClass), > .class_init = xics_class_init, > - .instance_init = xics_initfn, > }; > > 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 @@ > > #include "hw/sysbus.h" > > +#define TYPE_XICS_COMMON "xics-common" > +#define XICS_COMMON(obj) OBJECT_CHECK(XICSState, (obj), TYPE_XICS_COMMON) > + > #define TYPE_XICS "xics" > #define XICS(obj) OBJECT_CHECK(XICSState, (obj), TYPE_XICS) > > +#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) > > @@ -58,6 +65,8 @@ struct XICSStateClass { > DeviceClass parent_class; > > 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); > }; > > struct XICSState { > @@ -138,7 +147,7 @@ void xics_set_irq_type(XICSState *icp, int irq, bool lsi); > > static inline void xics_dispatch_cpu_setup(XICSState *icp, PowerPCCPU *cpu) > { > - XICSStateClass *info = XICS_GET_CLASS(icp); > + XICSStateClass *info = XICS_COMMON_GET_CLASS(icp); > > assert(info->cpu_setup); > info->cpu_setup(icp, cpu); >
On 08/06/2013 07:53 PM, Andreas Färber wrote: > 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. >> >> 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. >> >> The emulated XICS changes: >> 1. instance_init() callback is replaced with "nr_irqs" property callback >> 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() did. >> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > > 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(-) >> >> 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 = CPU(cpu); >> + CPUPPCState *env = &cpu->env; >> + ICPState *ss = &icp->ss[cs->cpu_index]; >> + >> + assert(cs->cpu_index < icp->nr_servers); >> + >> + switch (PPC_INPUT(env)) { >> + case PPC_FLAGS_INPUT_POWER7: >> + ss->output = env->irq_inputs[POWER7_INPUT_INT]; >> + break; >> + >> + case PPC_FLAGS_INPUT_970: >> + ss->output = 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? Right. If it fails, nothing for the caller to decide. >> + } >> +} >> + >> +void xics_prop_set_nr_irqs(Object *obj, struct Visitor *v, >> + void *opaque, const char *name, struct Error **errp) > > You should be able to drop both "struct"s. Yeah, fixed. This is just how the ObjectPropertyAccessor type is defined. >> +{ >> + XICSState *icp = XICS_COMMON(obj); >> + XICSStateClass *info = XICS_COMMON_GET_CLASS(icp); >> + Error *error = 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 Error **errp) >> +{ >> + XICSState *icp = XICS_COMMON(obj); >> + XICSStateClass *info = XICS_COMMON_GET_CLASS(icp); >> + Error *error = 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, NULL); > > Since this property is visible in the QOM tree, it would be nice to > implement trivial getters returns the value from the state fields. Added. How do I test it? "info qtree" prints only DEVICE_CLASS(class)->props which is null in this case. >> +} >> + >> +static void xics_common_reset(DeviceState *d) >> +{ >> + XICSState *icp = XICS_COMMON(d); >> + int i; >> + >> + for (i = 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 = DEVICE_CLASS(oc); >> + XICSStateClass *xsc = XICS_COMMON_CLASS(oc); >> + >> + dc->reset = xics_common_reset; >> + xsc->cpu_setup = xics_common_cpu_setup; >> +} >> + >> +static const TypeInfo xics_common_info = { >> + .name = TYPE_XICS_COMMON, >> + .parent = TYPE_SYS_BUS_DEVICE, >> + .instance_size = sizeof(XICSState), >> + .class_size = sizeof(XICSStateClass), >> + .instance_init = xics_common_initfn, >> + .class_init = xics_common_class_init, >> +}; >> >> /* >> * ICP: Presentation layer >> @@ -443,6 +549,13 @@ static const VMStateDescription vmstate_ics = { >> }, >> }; >> >> +static void ics_initfn(Object *obj) >> +{ >> + ICSState *ics = ICS(obj); >> + >> + ics->offset = XICS_IRQ_BASE; >> +} >> + >> static int ics_realize(DeviceState *dev) >> { >> ICSState *ics = ICS(dev); >> @@ -472,6 +585,7 @@ static const TypeInfo ics_info = { >> .instance_size = sizeof(ICSState), >> .class_init = ics_class_init, >> .class_size = sizeof(ICSStateClass), >> + .instance_init = ics_initfn, >> }; >> >> /* >> @@ -651,47 +765,32 @@ static void rtas_int_on(PowerPCCPU *cpu, sPAPREnvironment *spapr, >> /* >> * XICS >> */ >> +void xics_set_nr_irqs(XICSState *icp, uint32_t nr_irqs) >> +{ >> + icp->ics = ICS(object_new(TYPE_ICS)); >> + object_property_add_child(OBJECT(icp), "ics", OBJECT(icp->ics), NULL); > > 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? I cannot create in the common code as there are TYPE_ICS and TYPE_ICS_KVM (oops, KVM is at the end, will fix it) types. And I would really want not to create it in instance_init() as I want to have the object created and all its properties initialized in one place. > NULL above also shows that your callback should probably get an Error > **errp argument to propagate any errors to the property and its callers. Yep, will fix that.
Am 07.08.2013 08:06, schrieb Alexey Kardashevskiy: > On 08/06/2013 07:53 PM, Andreas Färber wrote: >> Am 06.08.2013 10:27, schrieb Alexey Kardashevskiy: >>> + } >>> +} >>> + >>> +void xics_prop_set_nr_irqs(Object *obj, struct Visitor *v, >>> + void *opaque, const char *name, struct Error **errp) >> >> You should be able to drop both "struct"s. > > Yeah, fixed. This is just how the ObjectPropertyAccessor type is defined. Yeah, reason is some circular header dependencies. ;) >>> +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, NULL); >> >> Since this property is visible in the QOM tree, it would be nice to >> implement trivial getters returns the value from the state fields. > > Added. How do I test it? ./QMP/qom-list to find the path, if not fixed in code yet, and ./QMP/qom-get path.nr_servers to retrieve the value, ./QMP/qom-set path.nr_servers to verify it doesn't kill the process. -qmp unix:./qmp-sock,server,nowait is what I use on the QEMU side. > "info qtree" prints only > DEVICE_CLASS(class)->props which is null in this case. True, info qtree is from qdev times and deprecated. I recently attached a "qom-tree" script doing the equivalent that you could try. I'll try to address -device xics,? showing them for 1.7. (Anthony indicated on a KVM call that the expected way to discover properties was to instantiate the type rather than looking at its class. Requires that types are instantiatable without asserting KVM accelerator, which gets initialized later.) >>> @@ -651,47 +765,32 @@ static void rtas_int_on(PowerPCCPU *cpu, sPAPREnvironment *spapr, >>> /* >>> * XICS >>> */ >>> +void xics_set_nr_irqs(XICSState *icp, uint32_t nr_irqs) >>> +{ >>> + icp->ics = ICS(object_new(TYPE_ICS)); >>> + object_property_add_child(OBJECT(icp), "ics", OBJECT(icp->ics), NULL); >> >> 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? > > I cannot create in the common code as there are TYPE_ICS and TYPE_ICS_KVM > (oops, KVM is at the end, will fix it) types. > > And I would really want not to create it in instance_init() as I want to > have the object created and all its properties initialized in one place. Doing it in instance_init facilitates improving the setter to allow multiple uses as a follow-up patch, since it must only be created once. If you don't, then the child will not be present in the composition tree before setting this seemingly unrelated property. That seems worse to me than accessing a field in two places - instance_init will be run before any property setter. BTW these dynamic setters do not check automatically whether the object has already been realized. If you want the setter to return an Error in that case, you will need to check for DeviceState *dev = DEVICE(icp); dev->realized yourself. Not sure if that's the semantics you desire, but I guess so? Andreas
On 08/07/2013 05:03 PM, Andreas Färber wrote: > Am 07.08.2013 08:06, schrieb Alexey Kardashevskiy: >> On 08/06/2013 07:53 PM, Andreas Färber wrote: >>> Am 06.08.2013 10:27, schrieb Alexey Kardashevskiy: >>>> + } >>>> +} >>>> + >>>> +void xics_prop_set_nr_irqs(Object *obj, struct Visitor *v, >>>> + void *opaque, const char *name, struct Error **errp) >>> >>> You should be able to drop both "struct"s. >> >> Yeah, fixed. This is just how the ObjectPropertyAccessor type is defined. > > Yeah, reason is some circular header dependencies. ;) > >>>> +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, NULL); >>> >>> Since this property is visible in the QOM tree, it would be nice to >>> implement trivial getters returns the value from the state fields. >> >> Added. How do I test it? > > ./QMP/qom-list to find the path, if not fixed in code yet, and Something is wrong. XICS does not have an id but it should not be a problem, right (btw how to set it from the code?)? [aik@dyn232 ~]$ ./qemu-impreza/QMP/qom-list -s ./qmp-sock / [aik@dyn232 ~]$ This is qtree: (qemu) info qtree bus: main-system-bus type System dev: spapr-pci-host-bridge, id "" index = 0 buid = 0x800000020000000 liobn = 0x80000000 mem_win_addr = 0x100a0000000 mem_win_size = 0x20000000 io_win_addr = 0x10080000000 io_win_size = 0x10000 msi_win_addr = 0x10090000000 irq 0 bus: pci type PCI dev: spapr-vio-bridge, id "" irq 0 bus: spapr-vio type spapr-vio-bus dev: spapr-vty, id "ser1" reg = 1895825409 chardev = char1 irq = 4102 dev: spapr-nvram, id "nvram@71000000" reg = 1895825408 drive = <null> irq = 4097 dev: xics-kvm, id "" irq 0 > ./QMP/qom-get path.nr_servers to retrieve the value, > ./QMP/qom-set path.nr_servers to verify it doesn't kill the process. > > -qmp unix:./qmp-sock,server,nowait is what I use on the QEMU side. >> "info qtree" prints only >> DEVICE_CLASS(class)->props which is null in this case. > > True, info qtree is from qdev times and deprecated. I recently attached > a "qom-tree" script doing the equivalent that you could try. > > I'll try to address -device xics,? showing them for 1.7. (Anthony > indicated on a KVM call that the expected way to discover properties was > to instantiate the type rather than looking at its class. Requires that > types are instantiatable without asserting KVM accelerator, which gets > initialized later.) > >>>> @@ -651,47 +765,32 @@ static void rtas_int_on(PowerPCCPU *cpu, sPAPREnvironment *spapr, >>>> /* >>>> * XICS >>>> */ >>>> +void xics_set_nr_irqs(XICSState *icp, uint32_t nr_irqs) >>>> +{ >>>> + icp->ics = ICS(object_new(TYPE_ICS)); >>>> + object_property_add_child(OBJECT(icp), "ics", OBJECT(icp->ics), NULL); >>> >>> 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? >> >> I cannot create in the common code as there are TYPE_ICS and TYPE_ICS_KVM >> (oops, KVM is at the end, will fix it) types. >> >> And I would really want not to create it in instance_init() as I want to >> have the object created and all its properties initialized in one place. > > Doing it in instance_init facilitates improving the setter to allow > multiple uses as a follow-up patch, since it must only be created once. > If you don't, then the child will not be present in the composition tree > before setting this seemingly unrelated property. It will still be happening to ICP's and cannot be avoided. Why to make it different for ICS and ICP? No IRQs number effectively means "no IRQ source". > That seems worse to me > than accessing a field in two places - instance_init will be run before > any property setter. > BTW these dynamic setters do not check automatically whether the object > has already been realized. If you want the setter to return an Error in > that case, you will need to check for DeviceState *dev = DEVICE(icp); > dev->realized yourself. Not sure if that's the semantics you desire, but > I guess so? realize() will fail if a property is not set. Setters will fail if a property is already set. By fail I mean that Error** thingy, not abort(). Do not really see why would we need something more here.
Am 07.08.2013 09:26, schrieb Alexey Kardashevskiy: > On 08/07/2013 05:03 PM, Andreas Färber wrote: >> Am 07.08.2013 08:06, schrieb Alexey Kardashevskiy: >>> [...] How do I test it? >> >> ./QMP/qom-list to find the path, if not fixed in code yet, and > > Something is wrong. XICS does not have an id but it should not be a > problem, right (btw how to set it from the code?)? > > [aik@dyn232 ~]$ ./qemu-impreza/QMP/qom-list -s ./qmp-sock > / That's expected for lack of argument. $ ./QMP/qom-list -s ./qmp-sock /machine unattached/ peripheral/ peripheral-anon/ type This confirms "path ... not fixed in code yet" (otherwise there would've been an "spapr/" entry or anything else telling. So you need to look through /machine/unassigned for the device[n] with qom-get /machine/unassigned/device[n].type matching your type, possibly device[0] as in my case. From there on you see your icp[0]/ and ics/ children without searching the unassigned list again. Or simply use my qom-tree script: http://lists.nongnu.org/archive/html/qemu-devel/2013-07/txte1WIbWzu5Z.txt Setting a canonical path is done via object_property_add_child() after instantiating and before realizing a device. For x86 the northbridge is used as name, i.e. "i440fx" for pc and "q35" for q35. Suggest to discuss with Anthony how to structure the composition tree for sPAPR. Andreas
On 08/08/2013 12:22 AM, Andreas Färber wrote: > Am 07.08.2013 09:26, schrieb Alexey Kardashevskiy: >> On 08/07/2013 05:03 PM, Andreas Färber wrote: >>> Am 07.08.2013 08:06, schrieb Alexey Kardashevskiy: >>>> [...] How do I test it? >>> >>> ./QMP/qom-list to find the path, if not fixed in code yet, and >> >> Something is wrong. XICS does not have an id but it should not be a >> problem, right (btw how to set it from the code?)? >> >> [aik@dyn232 ~]$ ./qemu-impreza/QMP/qom-list -s ./qmp-sock >> / > > That's expected for lack of argument. > > $ ./QMP/qom-list -s ./qmp-sock /machine > unattached/ > peripheral/ > peripheral-anon/ > type > > This confirms "path ... not fixed in code yet" (otherwise there would've > been an "spapr/" entry or anything else telling. > > So you need to look through /machine/unassigned for the device[n] with > qom-get /machine/unassigned/device[n].type matching your type, possibly > device[0] as in my case. From there on you see your icp[0]/ and ics/ > children without searching the unassigned list again. > > Or simply use my qom-tree script: > http://lists.nongnu.org/archive/html/qemu-devel/2013-07/txte1WIbWzu5Z.txt Yep. That works, thanks. > Setting a canonical path is done via object_property_add_child() after > instantiating and before realizing a device. > For x86 the northbridge is used as name, i.e. "i440fx" for pc and "q35" > for q35. Suggest to discuss with Anthony how to structure the > composition tree for sPAPR. I tried inserting this: object_property_add_child(qdev_get_machine(), busname, OBJECT(dev), NULL); in TYPE_SPAPR_PCI_HOST_BRIDGE's spapr_phb_init() (I thought this is what i440fx_init() does in the very beginning) but when I do that, spapr_phb already has a parent of a "container" type so assert happens. What is the aim of all of this? Is it not to have "unattached" in a path starting with /machine? btw I have added notes in the previous response against splitting ICS initialization into 2 functions and you skipped it. Does this mean you do not want to waste more of your time persuading me and this is the real stopped or you just gave up? :) Thanks.
Am 08.08.2013 05:10, schrieb Alexey Kardashevskiy: > On 08/08/2013 12:22 AM, Andreas Färber wrote: >> Setting a canonical path is done via object_property_add_child() after >> instantiating and before realizing a device. >> For x86 the northbridge is used as name, i.e. "i440fx" for pc and "q35" >> for q35. Suggest to discuss with Anthony how to structure the >> composition tree for sPAPR. > > I tried inserting this: > object_property_add_child(qdev_get_machine(), busname, OBJECT(dev), NULL); > > in TYPE_SPAPR_PCI_HOST_BRIDGE's spapr_phb_init() (I thought this is what > i440fx_init() does in the very beginning) but when I do that, spapr_phb > already has a parent of a "container" type so assert happens. See above: After instantiating (instance_init) and before realizing (realize/init). It needs to be done by the parent, not by the device itself - unless I'm completely misunderstanding what you're trying, in absence of code to look at. > What is the aim of all of this? Is it not to have "unattached" in a path > starting with /machine? Correct. To have a canonical path for management tools, qtest, etc. similar to /sys filesystem in Linux. > btw I have added notes in the previous response against splitting ICS > initialization into 2 functions and you skipped it. Does this mean you do > not want to waste more of your time persuading me and this is the real > stopped or you just gave up? :) Thanks. "Never give up, never surrender!" :P I believe I had already answered to that: I have no clue what ICS, ICP, XICS acronyms actually stand for, but I explained friendly and verbose why doing QOM things the way I asked you to makes sense. Ultimately I was told by Anthony when and how to do these things, and I do not have all day to argue with you, so if you don't like my feedback then please discuss with your IBM colleague directly. There is by definition a functional split between instance_init and realize, not my invention, and management tools should be able to tweak properties of objects, such as you setting nr_servers to 1 on machine init and QMP qom-set bumping it to 2. It is IMO not essential to implement that from the start because having a performant interrupt implementation is more important than our QOM'ish refactorings, therefore I said error_setg() should be okay, but my feedback is targeted at keeping our design future-proof, which means that instantiation of a single object that was in instance_init before should not be moved into a property setter that may be called multiple times because it would then need to be moved back anyway. Use of object_initialize() was requested by Anthony to have the QOM composition reflected in the allocation model, i.e. in this case g_try_malloc0(sizeof(XICSState)) including the ICS(?) thingy. So since it was created once independent of properties before your patch, I assume we should keep that behavior, while only changing the storage location. Still a whole week left for cleaning up 1.7 patches! :) Andreas
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 = CPU(cpu); + CPUPPCState *env = &cpu->env; + ICPState *ss = &icp->ss[cs->cpu_index]; + + assert(cs->cpu_index < icp->nr_servers); + + switch (PPC_INPUT(env)) { + case PPC_FLAGS_INPUT_POWER7: + ss->output = env->irq_inputs[POWER7_INPUT_INT]; + break; + + case PPC_FLAGS_INPUT_970: + ss->output = env->irq_inputs[PPC970_INPUT_INT]; + break; + + default: + error_report("XICS interrupt controller does not support this CPU " + "bus model"); + abort(); + } +} + +void xics_prop_set_nr_irqs(Object *obj, struct Visitor *v, + void *opaque, const char *name, struct Error **errp) +{ + XICSState *icp = XICS_COMMON(obj); + XICSStateClass *info = XICS_COMMON_GET_CLASS(icp); + Error *error = 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); + 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 Error **errp) +{ + XICSState *icp = XICS_COMMON(obj); + XICSStateClass *info = XICS_COMMON_GET_CLASS(icp); + Error *error = 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); + 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, NULL); +} + +static void xics_common_reset(DeviceState *d) +{ + XICSState *icp = XICS_COMMON(d); + int i; + + for (i = 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 = DEVICE_CLASS(oc); + XICSStateClass *xsc = XICS_COMMON_CLASS(oc); + + dc->reset = xics_common_reset; + xsc->cpu_setup = xics_common_cpu_setup; +} + +static const TypeInfo xics_common_info = { + .name = TYPE_XICS_COMMON, + .parent = TYPE_SYS_BUS_DEVICE, + .instance_size = sizeof(XICSState), + .class_size = sizeof(XICSStateClass), + .instance_init = xics_common_initfn, + .class_init = xics_common_class_init, +}; /* * ICP: Presentation layer @@ -443,6 +549,13 @@ static const VMStateDescription vmstate_ics = { }, }; +static void ics_initfn(Object *obj) +{ + ICSState *ics = ICS(obj); + + ics->offset = XICS_IRQ_BASE; +} + static int ics_realize(DeviceState *dev) { ICSState *ics = ICS(dev); @@ -472,6 +585,7 @@ static const TypeInfo ics_info = { .instance_size = sizeof(ICSState), .class_init = ics_class_init, .class_size = sizeof(ICSStateClass), + .instance_init = ics_initfn, }; /* @@ -651,47 +765,32 @@ static void rtas_int_on(PowerPCCPU *cpu, sPAPREnvironment *spapr, /* * XICS */ +void xics_set_nr_irqs(XICSState *icp, uint32_t nr_irqs) +{ + icp->ics = ICS(object_new(TYPE_ICS)); + object_property_add_child(OBJECT(icp), "ics", OBJECT(icp->ics), NULL); + icp->ics->icp = icp; + icp->nr_irqs = icp->ics->nr_irqs = nr_irqs; +} -static void xics_reset(DeviceState *d) +void xics_set_nr_servers(XICSState *icp, uint32_t nr_servers) { - XICSState *icp = XICS(d); int i; + icp->nr_servers = nr_servers; + + icp->ss = g_malloc0(icp->nr_servers*sizeof(ICPState)); for (i = 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 = CPU(cpu); - CPUPPCState *env = &cpu->env; - ICPState *ss = &icp->ss[cs->cpu_index]; - - assert(cs->cpu_index < icp->nr_servers); - - switch (PPC_INPUT(env)) { - case PPC_FLAGS_INPUT_POWER7: - ss->output = env->irq_inputs[POWER7_INPUT_INT]; - break; - - case PPC_FLAGS_INPUT_970: - ss->output = 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); } } static void xics_realize(DeviceState *dev, Error **errp) { XICSState *icp = XICS(dev); - ICSState *ics = icp->ics; Error *error = NULL; int i; @@ -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); - ics->nr_irqs = icp->nr_irqs; - ics->offset = XICS_IRQ_BASE; - ics->icp = icp; object_property_set_bool(OBJECT(icp->ics), true, "realized", &error); if (error) { error_propagate(errp, error); @@ -716,12 +812,7 @@ static void xics_realize(DeviceState *dev, Error **errp) } assert(icp->nr_servers); - icp->ss = g_malloc0(icp->nr_servers*sizeof(ICPState)); for (i = 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) } } -static void xics_initfn(Object *obj) -{ - XICSState *xics = XICS(obj); - - xics->ics = ICS(object_new(TYPE_ICS)); - object_property_add_child(obj, "ics", OBJECT(xics->ics), NULL); -} - -static Property xics_properties[] = { - 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 = DEVICE_CLASS(oc); XICSStateClass *xsc = XICS_CLASS(oc); dc->realize = xics_realize; - dc->props = xics_properties; - dc->reset = xics_reset; - xsc->cpu_setup = xics_cpu_setup; + xsc->set_nr_irqs = xics_set_nr_irqs; + xsc->set_nr_servers = xics_set_nr_servers; } static const TypeInfo xics_info = { .name = TYPE_XICS, - .parent = TYPE_SYS_BUS_DEVICE, + .parent = TYPE_XICS_COMMON, .instance_size = sizeof(XICSState), .class_size = sizeof(XICSStateClass), .class_init = xics_class_init, - .instance_init = xics_initfn, }; 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 @@ #include "hw/sysbus.h" +#define TYPE_XICS_COMMON "xics-common" +#define XICS_COMMON(obj) OBJECT_CHECK(XICSState, (obj), TYPE_XICS_COMMON) + #define TYPE_XICS "xics" #define XICS(obj) OBJECT_CHECK(XICSState, (obj), TYPE_XICS) +#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) @@ -58,6 +65,8 @@ struct XICSStateClass { DeviceClass parent_class; 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); }; struct XICSState { @@ -138,7 +147,7 @@ void xics_set_irq_type(XICSState *icp, int irq, bool lsi); static inline void xics_dispatch_cpu_setup(XICSState *icp, PowerPCCPU *cpu) { - XICSStateClass *info = XICS_GET_CLASS(icp); + XICSStateClass *info = XICS_COMMON_GET_CLASS(icp); assert(info->cpu_setup); info->cpu_setup(icp, cpu);
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. 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. The emulated XICS changes: 1. instance_init() callback is replaced with "nr_irqs" property callback 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() did. Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- hw/intc/xics.c | 190 +++++++++++++++++++++++++++++++++++--------------- include/hw/ppc/xics.h | 11 ++- 2 files changed, 143 insertions(+), 58 deletions(-)