Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 16/22] ppc/xics: register the reset handler of ICP objects

[David Gibson]

On Fri, Feb 24, 2017 at 12:27:35PM +0100, Cédric Le Goater wrote:
> On 02/23/2017 03:42 AM, David Gibson wrote:
> > On Thu, Feb 16, 2017 at 02:47:39PM +0100, Cédric Le Goater wrote:
> >> The reset of the ICP objects is currently handled by XICS but this can
> >> be done for each individual ICP.
> >>
> >> Signed-off-by: Cédric Le Goater <address@hidden>
> > 
> > Hrm.  I think whether device_reset() gets called automatically depends
> > on how the device is wired into the composition tree, and I'm not sure
> > the icps are in the right place for it to work.
> 
> reset gets called only if it under sysbus or if you have registered 
> a reset_handler. previously, XICS was a sysbus object so 
> xics_common_reset() was getting called automatically.

Right.  Hmm.  So I think artificially placing the ics under the sysbus
is not the right way to get the reset called - I think explicitly
registering a reset handler is better.

The only thing that concerns me about that is that the MMIO ICS
variants we'll want for powernv really _do_ have a bus presence,
either on sysbus or some descendent, so that will get its reset called
automatically.  I'm not sure what the best way to ensure the reset
gets called exactly once in all cases.

> > This doesn't replace the code in xics_common_reset() so if it does
> > work it means we must have previously been resetting the ICPs twice.
> > Is that right?
> 
> no. but there has been some confusion with the recent changes
> on XICS.
> 
> What replace the code in xics_common_reset() is :
> 
>       qdev_set_parent_bus(DEVICE(icp), sysbus_get_default());
> 
> That's how the reset handlers get called from QOM objects.

Right, I saw that later on.

> 
> C.
> 
> >> ---
> >>  hw/intc/xics.c | 18 ------------------
> >>  hw/ppc/spapr.c |  1 +
> >>  2 files changed, 1 insertion(+), 18 deletions(-)
> >>
> >> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> >> index dd41340d41a5..3ad7e8cf8ec4 100644
> >> --- a/hw/intc/xics.c
> >> +++ b/hw/intc/xics.c
> >> @@ -137,29 +137,11 @@ static void 
> >> ics_simple_pic_print_info(InterruptStatsProvider *obj,
> >>  /*
> >>   * XICS Common class - parent for emulated XICS and KVM-XICS
> >>   */
> >> -static void xics_common_reset(DeviceState *d)
> >> -{
> >> -    XICSState *xics = XICS_COMMON(d);
> >> -    int i;
> >> -
> >> -    for (i = 0; i < xics->nr_servers; i++) {
> >> -        device_reset(DEVICE(&xics->ss[i]));
> >> -    }
> >> -}
> >> -
> >> -static void xics_common_class_init(ObjectClass *oc, void *data)
> >> -{
> >> -    DeviceClass *dc = DEVICE_CLASS(oc);
> >> -
> >> -    dc->reset = xics_common_reset;
> >> -}
> >> -
> >>  static const TypeInfo xics_common_info = {
> >>      .name          = TYPE_XICS_COMMON,
> >>      .parent        = TYPE_DEVICE,
> >>      .instance_size = sizeof(XICSState),
> >>      .class_size    = sizeof(XICSStateClass),
> >> -    .class_init    = xics_common_class_init,
> >>  };
> >>  
> >>  /*
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index 9c1772f93155..445d9a6ddad4 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -130,6 +130,7 @@ static XICSState *try_create_xics(sPAPRMachineState 
> >> *spapr,
> >>          ICPState *icp = &xics->ss[i];
> >>  
> >>          object_initialize(icp, sizeof(*icp), type_icp);
> >> +        qdev_set_parent_bus(DEVICE(icp), sysbus_get_default());
> >>          object_property_add_child(OBJECT(xics), "icp[*]", OBJECT(icp), 
> >> NULL);
> >>          object_property_add_const_link(OBJECT(icp), "xics", OBJECT(xics), 
> >> NULL);
> >>          object_property_set_bool(OBJECT(icp), true, "realized", &err);
> > 
> 

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