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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5166 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]