On Tue, Feb 14, 2017 at 03:52:09PM +0100, Cédric Le Goater wrote: > 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 ?
I don't think I'll want to merge until the full picture is addressed. However, I'm fine with posting incomplete versions for earlier review - just label them RFC and note in the 0/N comment that there's more work to be done to complete the picture. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature