Re: [qemu-s390x] hw/s390x/ipl: Dubious use of qdev_reset_all_fn

[Philippe Mathieu-Daudé]

On 5/28/19 10:33 AM, Cornelia Huck wrote:
> On Tue, 28 May 2019 10:29:09 +0200
> David Hildenbrand <address@hidden> wrote:
> 
>> On 24.05.19 21:45, Christian Borntraeger wrote:
>>>
>>>
>>> On 24.05.19 21:00, David Hildenbrand wrote:  
>>>> On 24.05.19 20:36, David Hildenbrand wrote:  
>>>>> On 24.05.19 20:28, Christian Borntraeger wrote:  
>>>>>>
>>>>>>
>>>>>> On 24.05.19 20:04, David Hildenbrand wrote:  
>>>>>>> On 24.05.19 19:54, Philippe Mathieu-Daudé wrote:  
>>>>>>>> Hi Christian,
>>>>>>>>
>>>>>>>> I'm having hard time to understand why the S390_IPL object calls
>>>>>>>> qemu_register_reset(qdev_reset_all_fn) in its realize() method, while
>>>>>>>> being QOM'ified (it has a reset method).
>>>>>>>>
>>>>>>>> It doesn't seem to have a qdev children added explicitly to it.
>>>>>>>> I see it is used as a singleton, what else am I missing?
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> Phil.
>>>>>>>>  
>>>>>>>
>>>>>>> Looks like I added it back then (~4 years ago) when converting it into a
>>>>>>> TYPE_DEVICE.
>>>>>>>
>>>>>>> I could imagine that - back then - this was needed because only
>>>>>>> TYPE_SYS_BUS_DEVICE would recursively get reset.  
>>>>>>
>>>>>> Yes, back then singleton devices were not recursively resetted. Has that 
>>>>>> changed?  
>>>>>
>>>>> Hacking that call out, I don't see it getting called anymore. So it is
>>>>> still required. The question is if it can be reworked.
>>>>>  
>>>>
>>>> Yes, as it is not a sysbus device, it won't get reset.
>>>> The owner (machine) has to take care of this. The following works:
>>>>
>>>>
>>>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>>>> index b93750c14e..91a31c2cd0 100644
>>>> --- a/hw/s390x/ipl.c
>>>> +++ b/hw/s390x/ipl.c
>>>> @@ -232,7 +232,6 @@ static void s390_ipl_realize(DeviceState *dev, Error 
>>>> **errp)
>>>>       */
>>>>      ipl->compat_start_addr = ipl->start_addr;
>>>>      ipl->compat_bios_start_addr = ipl->bios_start_addr;
>>>> -    qemu_register_reset(qdev_reset_all_fn, dev);
>>>>  error:
>>>>      error_propagate(errp, err);
>>>>  }
>>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>>> index bbc6e8fa0b..658ab529a1 100644
>>>> --- a/hw/s390x/s390-virtio-ccw.c
>>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>>> @@ -338,6 +338,11 @@ static inline void s390_do_cpu_ipl(CPUState *cs, 
>>>> run_on_cpu_data arg)
>>>>      s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>>>>  }
>>>>  
>>>> +static void s390_ipl_reset(void)
>>>> +{
>>>> +    qdev_reset_all(DEVICE(object_resolve_path_type("", TYPE_S390_IPL, 
>>>> NULL)));
>>>> +}
>>>> +
>>>>  static void s390_machine_reset(void)
>>>>  {
>>>>      enum s390_reset reset_type;
>>>> @@ -353,6 +358,7 @@ static void s390_machine_reset(void)
>>>>      case S390_RESET_EXTERNAL:
>>>>      case S390_RESET_REIPL:
>>>>          qemu_devices_reset();
>>>> +        s390_ipl_reset();
>>>>          s390_crypto_reset();
>>>>  
>>>>          /* configure and start the ipl CPU only */
>>>>  
>>>
>>> While this patch is certainly ok, I find it disturbing that qdev devices 
>>> are being resetted,
>>> but qom devices not.
>>>   
>>
>> Shall I send that as a proper patch, or do we want to stick to the
>> existing approach until we have improved the general reset approach?
> 
> I don't think the current code is really broken, so personally I'd
> prefer to just leave it alone until we figured out how the reset should
> work in general.

Agreed, I'd rather wait we better understand QOM/reset limitations, then
fix this properly, and finally kill the qdev_reset_all_fn() function.

Thanks all for having a look at this btw :)

Regards,

Phil.