Re: [Qemu-ppc] [PATCH 1/2] ppc/xics: remove set_nr_irqs() handler from XICSStateClass

[Cédric Le Goater]

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