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

[Cornelia Huck]

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.