[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [PATCH 1/2] ppc/xics: remove set_nr_irqs() handler from X
From: |
David Gibson |
Subject: |
Re: [Qemu-ppc] [PATCH 1/2] ppc/xics: remove set_nr_irqs() handler from XICSStateClass |
Date: |
Wed, 15 Feb 2017 12:59:57 +1100 |
User-agent: |
Mutt/1.7.1 (2016-10-04) |
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 <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 ?
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
[Qemu-ppc] [PATCH 2/2] ppc/xics: remove set_nr_servers() handler from XICSStateClass, Cédric Le Goater, 2017/02/13