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

[Cédric Le Goater]

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

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