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

[David Hildenbrand]

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?

-- 

Thanks,

David / dhildenb