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 <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?
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
