| Message ID | 1487252865-12064-17-git-send-email-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
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 <clg@kaod.org> 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. 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? > --- > 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);
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 <clg@kaod.org> > > 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. > 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. 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); >
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 <clg@kaod.org> > > > > 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); > > >
[ adding Peter for some insights ] On 02/27/2017 02:00 AM, David Gibson wrote: > 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 <clg@kaod.org> >>> >>> 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. Well, I am not sure either. I have reproduced this pattern as it is frequently used under ARM which has quite a few QOM'ified machines. Thanks, C. >>> 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); >>> >> >
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);
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 <clg@kaod.org> --- hw/intc/xics.c | 18 ------------------ hw/ppc/spapr.c | 1 + 2 files changed, 1 insertion(+), 18 deletions(-)