Re: [Qemu-devel] [PATCH 5/6] xics: split to xics and xics-common

[Andreas Färber]

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 <address@hidden>

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