On 02/14/2017 08:04 AM, Cédric Le Goater wrote:
> On 02/14/2017 06:02 AM, David Gibson wrote:
>> On Mon, Feb 13, 2017 at 03:09:16PM +0100, Cédric Le Goater wrote:
>>> Today, the ICS (Interrupt Controller Source) object is created and
>>> realized by the init and realize routines of the XICS object, but some
>>> of the parameters are only known at the machine level.
>>>
>>> These parameters are passed from the sPAPR machine to the ICS object
>>> in a rather convoluted way using property handlers and a class handler
>>> of the XICS object. The number of irqs required to allocate the IRQ
>>> state objects in the ICS realize routine is one of them.
>>>
>>> Let's simplify the process by creating the ICS object along with the
>>> XICS object at the machine level and link the ICS into the XICS list
>>> of ICSs at this level also. In the sPAPR machine, there is only a
>>> single ICS but that will change with the PowerNV machine.
>>>
>>> Also, QOMify the creation of the objects and get rid of the
>>> superfluous code.
>>>
>>> Signed-off-by: Cédric Le Goater <c...@kaod.org>
>>
>> I like the basic idea here: while the ics and icp objects are pretty
>> straightforward, the "xics" object has always been a bit of a hack,
>> with logic that really belongs in the machine.
>>
>> But.. I don't think the approach here really works. Specifically..
>>
>> [snip]
>>> -static XICSState *try_create_xics(const char *type, int nr_servers,
>>> - int nr_irqs, Error **errp)
>>> -{
>>> - Error *err = NULL;
>>> - DeviceState *dev;
>>> +static XICSState *try_create_xics(const char *type, const char *type_ics,
>>> + int nr_servers, int nr_irqs, Error
>>> **errp)
>>> +{
>>> + Error *err = NULL, *local_err = NULL;
>>> + XICSState *xics;
>>> + ICSState *ics = NULL;
>>> +
>>> + xics = XICS_COMMON(object_new(type));
>>> + qdev_set_parent_bus(DEVICE(xics), sysbus_get_default());
>>> + object_property_set_int(OBJECT(xics), nr_servers, "nr_servers", &err);
>>> + object_property_set_bool(OBJECT(xics), true, "realized", &local_err);
>>> + error_propagate(&err, local_err);
>>> + if (err) {
>>> + goto error;
>>> + }
>>>
>>> - dev = qdev_create(NULL, type);
>>> - qdev_prop_set_uint32(dev, "nr_servers", nr_servers);
>>> - qdev_prop_set_uint32(dev, "nr_irqs", nr_irqs);
>>> - object_property_set_bool(OBJECT(dev), true, "realized", &err);
>>> + ics = ICS_SIMPLE(object_new(type_ics));
>>> + object_property_add_child(OBJECT(xics), "ics", OBJECT(ics), NULL);
>>> + object_property_set_int(OBJECT(ics), nr_irqs, "nr-irqs", &err);
>>> + object_property_set_bool(OBJECT(ics), true, "realized", &local_err);
>>> + error_propagate(&err, local_err);
>>> if (err) {
>>> - error_propagate(errp, err);
>>> - object_unparent(OBJECT(dev));
>>> - return NULL;
>>> + goto error;
>>> + }
>>> +
>>> + ics->xics = xics;
>>> + QLIST_INSERT_HEAD(&xics->ics, ics, list);
>>
>> Poking into the ics and xics objects directly from the machine here
>> violates abstraction even worse than the existing xics device does.
>> In fact, avoiding that is basically why the xics device exists in the
>> first place.
>
> Well, currently, xics_set_nr_servers() also does a :
>
> icp->xics = xics;
>
> So, I think we can live with it until we move the ICS and ICP objects
> out of XICS ?
>
> if this is the only worrisome problem in the patch, I can start the
> series with a QOM Interface handler implemented at the machine level,
> something like :
>
> void (*ics_insert)(ICSState *ics);
>
> doing the insert above to hide the temporary hideousness. Is that ok ?
After some thought, I don't think this one is important. At the
machine level, it seems OK to me to insert an ICS in the XICS list.
I agree that XICS should disappear, but it is still there for the
moment.
> And, as you propose below, I think we can add the 'xics' link property
> right now to get rid of :
>
> ic[ps]->xics = xics;
I have a v2 ready adding the 'xics' link property but keeping the
QLIST_INSERT_HEAD() call in try_create_xics(). Do you think it is
worth sending as an initial cleanup :
5 files changed, 96 insertions(+), 225 deletions(-)
or do you want the full picture to be addressed ?
Thanks,
C.
