[v3] Multi-phase reset mechanism

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

Posted by Damien Hedde 6 years, 3 months ago

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 <damien.hedde@greensocs.com>
---
 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.
+     */
+    qemu_register_reset(resettable_reset_cold_fn, dev);
 error:
     error_propagate(errp, err);
 }
-- 
2.22.0

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

Posted by Peter Maydell 6 years, 3 months ago

On Mon, 29 Jul 2019 at 15:59, Damien Hedde <damien.hedde@greensocs.com> 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 <damien.hedde@greensocs.com>
> ---
>  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.
 */

> +    qemu_register_reset(resettable_reset_cold_fn, dev);
>  error:
>      error_propagate(errp, err);
>  }
> --
> 2.22.0
>

thanks
-- PMM

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

Posted by Cornelia Huck 6 years, 3 months ago

On Wed, 7 Aug 2019 16:24:30 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On Mon, 29 Jul 2019 at 15:59, Damien Hedde <damien.hedde@greensocs.com> 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 <damien.hedde@greensocs.com>
> > ---
> >  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