| Message ID | 1376891726-26122-5-git-send-email-aik@ozlabs.ru |
|---|---|
| State | New |
| Headers | show |
On 19.08.2013, at 07:55, Alexey Kardashevskiy wrote: > Before XICS-KVM comes, it makes sense to clean up the emulated XICS a bit. > > This does: > 1. add assert in ics_realize() > 2. change variable names from "k" to more informative ones > 3. add "const" to every TypeInfo > 4. replace fprintf(stderr, ..."\n") with error_report > 5. replace old style qdev_init_nofail() with new style > object_property_set_bool(). > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > Changes: > v3: > * ics_realize() fixed to be actual realize callback rather than initfn > * asserts replaced with Error** > --- > hw/intc/xics.c | 42 ++++++++++++++++++++++++++++++------------ > 1 file changed, 30 insertions(+), 12 deletions(-) > > diff --git a/hw/intc/xics.c b/hw/intc/xics.c > index c80fa80..4d08c58 100644 > --- a/hw/intc/xics.c > +++ b/hw/intc/xics.c > @@ -29,6 +29,7 @@ > #include "trace.h" > #include "hw/ppc/spapr.h" > #include "hw/ppc/xics.h" > +#include "qemu/error-report.h" > > /* > * ICP: Presentation layer > @@ -211,7 +212,7 @@ static void icp_class_init(ObjectClass *klass, void *data) > dc->vmsd = &vmstate_icp_server; > } > > -static TypeInfo icp_info = { > +static const TypeInfo icp_info = { > .name = TYPE_ICP, > .parent = TYPE_DEVICE, > .instance_size = sizeof(ICPState), > @@ -442,15 +443,17 @@ static const VMStateDescription vmstate_ics = { > }, > }; > > -static int ics_realize(DeviceState *dev) > +static void ics_realize(DeviceState *dev, Error **errp) > { > ICSState *ics = ICS(dev); > > + if (!ics->nr_irqs) { > + error_setg(errp, "Number of interrupts needs to be greater 0"); > + return; > + } > ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState)); > ics->islsi = g_malloc0(ics->nr_irqs * sizeof(bool)); > ics->qirqs = qemu_allocate_irqs(ics_set_irq, ics, ics->nr_irqs); > - > - return 0; > } > > static void ics_class_init(ObjectClass *klass, void *data) > @@ -458,13 +461,14 @@ static void ics_class_init(ObjectClass *klass, void *data) > DeviceClass *dc = DEVICE_CLASS(klass); > ICSStateClass *isc = ICS_CLASS(klass); > > - dc->init = ics_realize; > + dc->realize = ics_realize; > dc->vmsd = &vmstate_ics; > dc->reset = ics_reset; > isc->post_load = ics_post_load; > + isc->post_load = ics_post_load; Same line twice? > } > > -static TypeInfo ics_info = { > +static const TypeInfo ics_info = { > .name = TYPE_ICS, > .parent = TYPE_DEVICE, > .instance_size = sizeof(ICSState), > @@ -680,8 +684,8 @@ static void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu) > break; > > default: > - fprintf(stderr, "XICS interrupt controller does not support this CPU " > - "bus model\n"); > + error_report("XICS interrupt controller does not support this CPU " > + "bus model"); > abort(); > } > } > @@ -690,8 +694,14 @@ static void xics_realize(DeviceState *dev, Error **errp) > { > XICSState *icp = XICS(dev); > ICSState *ics = icp->ics; > + Error *error = NULL; > int i; > > + if (!icp->nr_servers) { > + error_setg(errp, "Number of servers needs to be greater 0"); > + return; > + } > + > /* Registration of global state belongs into realize */ > spapr_rtas_register("ibm,set-xive", rtas_set_xive); > spapr_rtas_register("ibm,get-xive", rtas_get_xive); > @@ -706,7 +716,11 @@ static void xics_realize(DeviceState *dev, Error **errp) > ics->nr_irqs = icp->nr_irqs; > ics->offset = XICS_IRQ_BASE; > ics->icp = icp; > - qdev_init_nofail(DEVICE(ics)); > + object_property_set_bool(OBJECT(icp->ics), true, "realized", &error); Are you sure this is correct? So you are trying to force set the ics to realize? Is this the right way to do this? Andreas? Alex
Am 26.08.2013 15:36, schrieb Alexander Graf: > > On 19.08.2013, at 07:55, Alexey Kardashevskiy wrote: > >> Before XICS-KVM comes, it makes sense to clean up the emulated XICS a bit. >> >> This does: >> 1. add assert in ics_realize() >> 2. change variable names from "k" to more informative ones >> 3. add "const" to every TypeInfo >> 4. replace fprintf(stderr, ..."\n") with error_report >> 5. replace old style qdev_init_nofail() with new style >> object_property_set_bool(). >> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >> --- >> Changes: >> v3: >> * ics_realize() fixed to be actual realize callback rather than initfn >> * asserts replaced with Error** >> --- >> hw/intc/xics.c | 42 ++++++++++++++++++++++++++++++------------ >> 1 file changed, 30 insertions(+), 12 deletions(-) >> >> diff --git a/hw/intc/xics.c b/hw/intc/xics.c >> index c80fa80..4d08c58 100644 >> --- a/hw/intc/xics.c >> +++ b/hw/intc/xics.c >> @@ -29,6 +29,7 @@ >> #include "trace.h" >> #include "hw/ppc/spapr.h" >> #include "hw/ppc/xics.h" >> +#include "qemu/error-report.h" >> >> /* >> * ICP: Presentation layer >> @@ -211,7 +212,7 @@ static void icp_class_init(ObjectClass *klass, void *data) >> dc->vmsd = &vmstate_icp_server; >> } >> >> -static TypeInfo icp_info = { >> +static const TypeInfo icp_info = { >> .name = TYPE_ICP, >> .parent = TYPE_DEVICE, >> .instance_size = sizeof(ICPState), >> @@ -442,15 +443,17 @@ static const VMStateDescription vmstate_ics = { >> }, >> }; >> >> -static int ics_realize(DeviceState *dev) >> +static void ics_realize(DeviceState *dev, Error **errp) >> { >> ICSState *ics = ICS(dev); >> >> + if (!ics->nr_irqs) { >> + error_setg(errp, "Number of interrupts needs to be greater 0"); >> + return; >> + } >> ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState)); >> ics->islsi = g_malloc0(ics->nr_irqs * sizeof(bool)); >> ics->qirqs = qemu_allocate_irqs(ics_set_irq, ics, ics->nr_irqs); >> - >> - return 0; >> } >> >> static void ics_class_init(ObjectClass *klass, void *data) >> @@ -458,13 +461,14 @@ static void ics_class_init(ObjectClass *klass, void *data) >> DeviceClass *dc = DEVICE_CLASS(klass); >> ICSStateClass *isc = ICS_CLASS(klass); >> >> - dc->init = ics_realize; >> + dc->realize = ics_realize; >> dc->vmsd = &vmstate_ics; >> dc->reset = ics_reset; >> isc->post_load = ics_post_load; >> + isc->post_load = ics_post_load; > > Same line twice? > >> } >> >> -static TypeInfo ics_info = { >> +static const TypeInfo ics_info = { >> .name = TYPE_ICS, >> .parent = TYPE_DEVICE, >> .instance_size = sizeof(ICSState), >> @@ -680,8 +684,8 @@ static void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu) >> break; >> >> default: >> - fprintf(stderr, "XICS interrupt controller does not support this CPU " >> - "bus model\n"); >> + error_report("XICS interrupt controller does not support this CPU " >> + "bus model"); >> abort(); >> } >> } >> @@ -690,8 +694,14 @@ static void xics_realize(DeviceState *dev, Error **errp) >> { >> XICSState *icp = XICS(dev); >> ICSState *ics = icp->ics; >> + Error *error = NULL; >> int i; >> >> + if (!icp->nr_servers) { >> + error_setg(errp, "Number of servers needs to be greater 0"); >> + return; >> + } >> + >> /* Registration of global state belongs into realize */ >> spapr_rtas_register("ibm,set-xive", rtas_set_xive); >> spapr_rtas_register("ibm,get-xive", rtas_get_xive); >> @@ -706,7 +716,11 @@ static void xics_realize(DeviceState *dev, Error **errp) >> ics->nr_irqs = icp->nr_irqs; >> ics->offset = XICS_IRQ_BASE; >> ics->icp = icp; >> - qdev_init_nofail(DEVICE(ics)); >> + object_property_set_bool(OBJECT(icp->ics), true, "realized", &error); > > Are you sure this is correct? So you are trying to force set the ics to realize? Is this the right way to do this? Andreas? Yes, it's correct, I requested it. This assures that errors are forwarded up the chain to whomever sets realized = true. That's a general pattern that some devices have been ignoring. Obviously for KVM Another request apart from checking the duplicate line I have, is to please split up this big patch. Everything except const should be in its own patch with short description for better review and independent ack'ing, so that we can move forward here. An explicit list of changes within one patch should be a yellow warning sign. ;) (I would prefer the patches to go through ppc-next with my Rb, but if you prefer nonfunctional QOM changes to go through qom-next I'm sure we could make that work as well.) Andreas
On 26.08.2013, at 16:20, Andreas Färber wrote: > Am 26.08.2013 15:36, schrieb Alexander Graf: >> >> On 19.08.2013, at 07:55, Alexey Kardashevskiy wrote: >> >>> Before XICS-KVM comes, it makes sense to clean up the emulated XICS a bit. >>> >>> This does: >>> 1. add assert in ics_realize() >>> 2. change variable names from "k" to more informative ones >>> 3. add "const" to every TypeInfo >>> 4. replace fprintf(stderr, ..."\n") with error_report >>> 5. replace old style qdev_init_nofail() with new style >>> object_property_set_bool(). >>> >>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >>> --- >>> Changes: >>> v3: >>> * ics_realize() fixed to be actual realize callback rather than initfn >>> * asserts replaced with Error** >>> --- >>> hw/intc/xics.c | 42 ++++++++++++++++++++++++++++++------------ >>> 1 file changed, 30 insertions(+), 12 deletions(-) >>> >>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c >>> index c80fa80..4d08c58 100644 >>> --- a/hw/intc/xics.c >>> +++ b/hw/intc/xics.c >>> @@ -29,6 +29,7 @@ >>> #include "trace.h" >>> #include "hw/ppc/spapr.h" >>> #include "hw/ppc/xics.h" >>> +#include "qemu/error-report.h" >>> >>> /* >>> * ICP: Presentation layer >>> @@ -211,7 +212,7 @@ static void icp_class_init(ObjectClass *klass, void *data) >>> dc->vmsd = &vmstate_icp_server; >>> } >>> >>> -static TypeInfo icp_info = { >>> +static const TypeInfo icp_info = { >>> .name = TYPE_ICP, >>> .parent = TYPE_DEVICE, >>> .instance_size = sizeof(ICPState), >>> @@ -442,15 +443,17 @@ static const VMStateDescription vmstate_ics = { >>> }, >>> }; >>> >>> -static int ics_realize(DeviceState *dev) >>> +static void ics_realize(DeviceState *dev, Error **errp) >>> { >>> ICSState *ics = ICS(dev); >>> >>> + if (!ics->nr_irqs) { >>> + error_setg(errp, "Number of interrupts needs to be greater 0"); >>> + return; >>> + } >>> ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState)); >>> ics->islsi = g_malloc0(ics->nr_irqs * sizeof(bool)); >>> ics->qirqs = qemu_allocate_irqs(ics_set_irq, ics, ics->nr_irqs); >>> - >>> - return 0; >>> } >>> >>> static void ics_class_init(ObjectClass *klass, void *data) >>> @@ -458,13 +461,14 @@ static void ics_class_init(ObjectClass *klass, void *data) >>> DeviceClass *dc = DEVICE_CLASS(klass); >>> ICSStateClass *isc = ICS_CLASS(klass); >>> >>> - dc->init = ics_realize; >>> + dc->realize = ics_realize; >>> dc->vmsd = &vmstate_ics; >>> dc->reset = ics_reset; >>> isc->post_load = ics_post_load; >>> + isc->post_load = ics_post_load; >> >> Same line twice? >> >>> } >>> >>> -static TypeInfo ics_info = { >>> +static const TypeInfo ics_info = { >>> .name = TYPE_ICS, >>> .parent = TYPE_DEVICE, >>> .instance_size = sizeof(ICSState), >>> @@ -680,8 +684,8 @@ static void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu) >>> break; >>> >>> default: >>> - fprintf(stderr, "XICS interrupt controller does not support this CPU " >>> - "bus model\n"); >>> + error_report("XICS interrupt controller does not support this CPU " >>> + "bus model"); >>> abort(); >>> } >>> } >>> @@ -690,8 +694,14 @@ static void xics_realize(DeviceState *dev, Error **errp) >>> { >>> XICSState *icp = XICS(dev); >>> ICSState *ics = icp->ics; >>> + Error *error = NULL; >>> int i; >>> >>> + if (!icp->nr_servers) { >>> + error_setg(errp, "Number of servers needs to be greater 0"); >>> + return; >>> + } >>> + >>> /* Registration of global state belongs into realize */ >>> spapr_rtas_register("ibm,set-xive", rtas_set_xive); >>> spapr_rtas_register("ibm,get-xive", rtas_get_xive); >>> @@ -706,7 +716,11 @@ static void xics_realize(DeviceState *dev, Error **errp) >>> ics->nr_irqs = icp->nr_irqs; >>> ics->offset = XICS_IRQ_BASE; >>> ics->icp = icp; >>> - qdev_init_nofail(DEVICE(ics)); >>> + object_property_set_bool(OBJECT(icp->ics), true, "realized", &error); >> >> Are you sure this is correct? So you are trying to force set the ics to realize? Is this the right way to do this? Andreas? > > Yes, it's correct, I requested it. This assures that errors are > forwarded up the chain to whomever sets realized = true. That's a > general pattern that some devices have been ignoring. Obviously for KVM > > Another request apart from checking the duplicate line I have, is to > please split up this big patch. Everything except const should be in its > own patch with short description for better review and independent > ack'ing, so that we can move forward here. An explicit list of changes > within one patch should be a yellow warning sign. ;) > (I would prefer the patches to go through ppc-next with my Rb, but if > you prefer nonfunctional QOM changes to go through qom-next I'm sure we > could make that work as well.) I'm more than happy to take them through ppc-next :). Alex
diff --git a/hw/intc/xics.c b/hw/intc/xics.c index c80fa80..4d08c58 100644 --- a/hw/intc/xics.c +++ b/hw/intc/xics.c @@ -29,6 +29,7 @@ #include "trace.h" #include "hw/ppc/spapr.h" #include "hw/ppc/xics.h" +#include "qemu/error-report.h" /* * ICP: Presentation layer @@ -211,7 +212,7 @@ static void icp_class_init(ObjectClass *klass, void *data) dc->vmsd = &vmstate_icp_server; } -static TypeInfo icp_info = { +static const TypeInfo icp_info = { .name = TYPE_ICP, .parent = TYPE_DEVICE, .instance_size = sizeof(ICPState), @@ -442,15 +443,17 @@ static const VMStateDescription vmstate_ics = { }, }; -static int ics_realize(DeviceState *dev) +static void ics_realize(DeviceState *dev, Error **errp) { ICSState *ics = ICS(dev); + if (!ics->nr_irqs) { + error_setg(errp, "Number of interrupts needs to be greater 0"); + return; + } ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState)); ics->islsi = g_malloc0(ics->nr_irqs * sizeof(bool)); ics->qirqs = qemu_allocate_irqs(ics_set_irq, ics, ics->nr_irqs); - - return 0; } static void ics_class_init(ObjectClass *klass, void *data) @@ -458,13 +461,14 @@ static void ics_class_init(ObjectClass *klass, void *data) DeviceClass *dc = DEVICE_CLASS(klass); ICSStateClass *isc = ICS_CLASS(klass); - dc->init = ics_realize; + dc->realize = ics_realize; dc->vmsd = &vmstate_ics; dc->reset = ics_reset; isc->post_load = ics_post_load; + isc->post_load = ics_post_load; } -static TypeInfo ics_info = { +static const TypeInfo ics_info = { .name = TYPE_ICS, .parent = TYPE_DEVICE, .instance_size = sizeof(ICSState), @@ -680,8 +684,8 @@ static void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu) break; default: - fprintf(stderr, "XICS interrupt controller does not support this CPU " - "bus model\n"); + error_report("XICS interrupt controller does not support this CPU " + "bus model"); abort(); } } @@ -690,8 +694,14 @@ static void xics_realize(DeviceState *dev, Error **errp) { XICSState *icp = XICS(dev); ICSState *ics = icp->ics; + Error *error = NULL; int i; + if (!icp->nr_servers) { + error_setg(errp, "Number of servers needs to be greater 0"); + return; + } + /* Registration of global state belongs into realize */ spapr_rtas_register("ibm,set-xive", rtas_set_xive); spapr_rtas_register("ibm,get-xive", rtas_get_xive); @@ -706,7 +716,11 @@ static void xics_realize(DeviceState *dev, Error **errp) ics->nr_irqs = icp->nr_irqs; ics->offset = XICS_IRQ_BASE; ics->icp = icp; - qdev_init_nofail(DEVICE(ics)); + object_property_set_bool(OBJECT(icp->ics), true, "realized", &error); + if (error) { + error_propagate(errp, error); + return; + } icp->ss = g_malloc0(icp->nr_servers*sizeof(ICPState)); for (i = 0; i < icp->nr_servers; i++) { @@ -714,7 +728,11 @@ static void xics_realize(DeviceState *dev, Error **errp) 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); - qdev_init_nofail(DEVICE(&icp->ss[i])); + object_property_set_bool(OBJECT(&icp->ss[i]), true, "realized", &error); + if (error) { + error_propagate(errp, error); + return; + } } } @@ -735,12 +753,12 @@ static Property xics_properties[] = { static void xics_class_init(ObjectClass *oc, void *data) { DeviceClass *dc = DEVICE_CLASS(oc); - XICSStateClass *k = XICS_CLASS(oc); + XICSStateClass *xsc = XICS_CLASS(oc); dc->realize = xics_realize; dc->props = xics_properties; dc->reset = xics_reset; - k->cpu_setup = xics_cpu_setup; + xsc->cpu_setup = xics_cpu_setup; } static const TypeInfo xics_info = {
Before XICS-KVM comes, it makes sense to clean up the emulated XICS a bit. This does: 1. add assert in ics_realize() 2. change variable names from "k" to more informative ones 3. add "const" to every TypeInfo 4. replace fprintf(stderr, ..."\n") with error_report 5. replace old style qdev_init_nofail() with new style object_property_set_bool(). Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- Changes: v3: * ics_realize() fixed to be actual realize callback rather than initfn * asserts replaced with Error** --- hw/intc/xics.c | 42 ++++++++++++++++++++++++++++++------------ 1 file changed, 30 insertions(+), 12 deletions(-)