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 <a...@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);
>
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
