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

[Cédric Le Goater]

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 ? 

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've thought about this a bit more, and I think I know how to solve
> this better now.
>
> I think that "xics" shouldn't be a concrete object at all, instead it
> should be a QOM interface, implemented by the machine type.  Both ICS
> and ICP would take a link property to find their xics. 

yes.

> The xics
> interface would provide methods to return an ics object given an irq
> number and to return an icp object given a server number. This gives
> full control of the irq and server number spaces back to the machine
> type, which is really where it belongs.

Yes, I agree, we started talking about it a while ago :

    http://lists.nongnu.org/archive/html/qemu-ppc/2016-11/msg00056.html

and this is what this first series is trying to do. It prepares ground 
by emptying the XICS object. 

The next step is to move out :

    ICPState *ss;
    QLIST_HEAD(, ICSState) ics;

from XICSState to sPAPRMachineState I think. This is when the QOM 
interfaces will come in play but we need to untangle the code slowly, 
to understand what is being done (honestly, it is not that obvious).  

As for xics->nr_servers, the only place where I still have doubts is 
ics_simple_post_load(). It should be easier after more cleanups.

This is like mahjong, you need to start from the right piece :)

Thanks,
C.