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

[Andreas Färber]

Am 07.08.2013 08:06, schrieb Alexey Kardashevskiy:
> On 08/06/2013 07:53 PM, Andreas Färber wrote:
>> Am 06.08.2013 10:27, schrieb Alexey Kardashevskiy:
>>> +    }
>>> +}
>>> +
>>> +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.

Yeah, reason is some circular header dependencies. ;)

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

./QMP/qom-list to find the path, if not fixed in code yet, and
./QMP/qom-get path.nr_servers to retrieve the value,
./QMP/qom-set path.nr_servers to verify it doesn't kill the process.

-qmp unix:./qmp-sock,server,nowait is what I use on the QEMU side.

> "info qtree" prints only
> DEVICE_CLASS(class)->props which is null in this case.

True, info qtree is from qdev times and deprecated. I recently attached
a "qom-tree" script doing the equivalent that you could try.

I'll try to address -device xics,? showing them for 1.7. (Anthony
indicated on a KVM call that the expected way to discover properties was
to instantiate the type rather than looking at its class. Requires that
types are instantiatable without asserting KVM accelerator, which gets
initialized later.)

>>> @@ -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.

Doing it in instance_init facilitates improving the setter to allow
multiple uses as a follow-up patch, since it must only be created once.
If you don't, then the child will not be present in the composition tree
before setting this seemingly unrelated property. That seems worse to me
than accessing a field in two places - instance_init will be run before
any property setter.

BTW these dynamic setters do not check automatically whether the object
has already been realized. If you want the setter to return an Error in
that case, you will need to check for DeviceState *dev = DEVICE(icp);
dev->realized yourself. Not sure if that's the semantics you desire, but
I guess so?

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg