[ 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 <c...@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);
>>>
>>
>
