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