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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

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.



-- 
Alexey