Re: [qemu-s390x] [PATCH v3 11/33] hw/s390x/ipl.c: remove qbus_reset_all registration

[Cornelia Huck]

On Wed, 7 Aug 2019 16:24:30 +0100
Peter Maydell <address@hidden> wrote:

> On Mon, 29 Jul 2019 at 15:59, Damien Hedde <address@hidden> wrote:
> >
> > Replace deprecated qbus_reset_all by resettable_reset_cold_fn for
> > the ipl registration in the main reset handlers.
> >
> > This does not impact the behavior.
> >
> > Signed-off-by: Damien Hedde <address@hidden>
> > ---
> >  hw/s390x/ipl.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> > index 60bd081d3e..402770a2c9 100644
> > --- a/hw/s390x/ipl.c
> > +++ b/hw/s390x/ipl.c
> > @@ -234,7 +234,11 @@ 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);
> > +    /*
> > +     * TODO: when we add some kind of main reset container / domain
> > +     * switch to it to really benefit from multi-phase.
> > +     */  
> 
> I think this comment misses the mark a bit. Here's my suggestion:
> 
> /*
>  * Because this Device is not on any bus in the qbus tree (it is
>  * not a sysbus device and it's not on some other bus like a PCI
>  * bus) it will not be automatically reset by the 'reset the
>  * sysbus' hook registered by vl.c like most devices. So we must
>  * manually register a reset hook for it.
>  * TODO: there should be a better way to do this.
>  */

Agreed, that explains much better why we're doing this.

> 
> > +    qemu_register_reset(resettable_reset_cold_fn, dev);

This is fine for the conversion done within this series; but resetting
the ipl device is one case where the cold vs. warm distinction falls a
bit short (there's a s390_reset enum which covers more cases). Not sure
if we want some custom reset types?

> >  error:
> >      error_propagate(errp, err);
> >  }
> > --
> > 2.22.0
> >  
> 
> thanks
> -- PMM