The realize method for the TYPE_ICS class uses qemu_register_reset()
to register a reset handler, as a workaround for the fact that
currently objects which directly inherit from TYPE_DEVICE don't get
automatically reset.  However, the reset function directly calls
ics_reset(), which is the function that implements the legacy reset
method.  This means that only the parent class's data gets reset, and
a subclass which also needs to handle reset, like TYPE_PHB3_MSI, has
to register its own reset function.

Make the TYPE_ICS reset function call device_cold_reset() instead:
this will handle reset for both the parent class and the subclass,
and will work whether the classes are using legacy reset or 3-phase
reset. This allows us to remove the reset function that the subclass
currently has to set up.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/xics.c             | 2 +-
 hw/pci-host/pnv_phb3_msi.c | 7 -------
 2 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index dcd021af668..dd130467ccc 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -593,7 +593,7 @@ static void ics_reset(DeviceState *dev)
 
 static void ics_reset_handler(void *dev)
 {
-    ics_reset(dev);
+    device_cold_reset(dev);
 }
 
 static void ics_realize(DeviceState *dev, Error **errp)
diff --git a/hw/pci-host/pnv_phb3_msi.c b/hw/pci-host/pnv_phb3_msi.c
index 2f4112907b8..ae908fd9e41 100644
--- a/hw/pci-host/pnv_phb3_msi.c
+++ b/hw/pci-host/pnv_phb3_msi.c
@@ -239,11 +239,6 @@ static void phb3_msi_reset(DeviceState *dev)
     msi->rba_sum = 0;
 }
 
-static void phb3_msi_reset_handler(void *dev)
-{
-    phb3_msi_reset(dev);
-}
-
 void pnv_phb3_msi_update_config(Phb3MsiState *msi, uint32_t base,
                                 uint32_t count)
 {
@@ -272,8 +267,6 @@ static void phb3_msi_realize(DeviceState *dev, Error **errp)
     }
 
     msi->qirqs = qemu_allocate_irqs(phb3_msi_set_irq, msi, ics->nr_irqs);
-
-    qemu_register_reset(phb3_msi_reset_handler, dev);
 }
 
 static void phb3_msi_instance_init(Object *obj)
-- 
2.25.1

On 11/25/22 12:52, Peter Maydell wrote:
> The realize method for the TYPE_ICS class uses qemu_register_reset()
> to register a reset handler, as a workaround for the fact that
> currently objects which directly inherit from TYPE_DEVICE don't get
> automatically reset.  However, the reset function directly calls
> ics_reset(), which is the function that implements the legacy reset
> method.  This means that only the parent class's data gets reset, and
> a subclass which also needs to handle reset, like TYPE_PHB3_MSI, has
> to register its own reset function.
> 
> Make the TYPE_ICS reset function call device_cold_reset() instead:
> this will handle reset for both the parent class and the subclass,
> and will work whether the classes are using legacy reset or 3-phase
> reset. This allows us to remove the reset function that the subclass
> currently has to set up.

Nice !

> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> ---
>   hw/intc/xics.c             | 2 +-
>   hw/pci-host/pnv_phb3_msi.c | 7 -------
>   2 files changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index dcd021af668..dd130467ccc 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -593,7 +593,7 @@ static void ics_reset(DeviceState *dev)
>   
>   static void ics_reset_handler(void *dev)
>   {
> -    ics_reset(dev);
> +    device_cold_reset(dev);
>   }
>   
>   static void ics_realize(DeviceState *dev, Error **errp)
> diff --git a/hw/pci-host/pnv_phb3_msi.c b/hw/pci-host/pnv_phb3_msi.c
> index 2f4112907b8..ae908fd9e41 100644
> --- a/hw/pci-host/pnv_phb3_msi.c
> +++ b/hw/pci-host/pnv_phb3_msi.c
> @@ -239,11 +239,6 @@ static void phb3_msi_reset(DeviceState *dev)
>       msi->rba_sum = 0;
>   }
>   
> -static void phb3_msi_reset_handler(void *dev)
> -{
> -    phb3_msi_reset(dev);
> -}
> -
>   void pnv_phb3_msi_update_config(Phb3MsiState *msi, uint32_t base,
>                                   uint32_t count)
>   {
> @@ -272,8 +267,6 @@ static void phb3_msi_realize(DeviceState *dev, Error **errp)
>       }
>   
>       msi->qirqs = qemu_allocate_irqs(phb3_msi_set_irq, msi, ics->nr_irqs);
> -
> -    qemu_register_reset(phb3_msi_reset_handler, dev);
>   }
>   
>   static void phb3_msi_instance_init(Object *obj)

On Fri, 25 Nov 2022 13:24:00 +0100
Cédric Le Goater <clg@kaod.org> wrote:

> On 11/25/22 12:52, Peter Maydell wrote:
> > The realize method for the TYPE_ICS class uses qemu_register_reset()
> > to register a reset handler, as a workaround for the fact that
> > currently objects which directly inherit from TYPE_DEVICE don't get
> > automatically reset.  However, the reset function directly calls
> > ics_reset(), which is the function that implements the legacy reset
> > method.  This means that only the parent class's data gets reset, and
> > a subclass which also needs to handle reset, like TYPE_PHB3_MSI, has
> > to register its own reset function.
> > 
> > Make the TYPE_ICS reset function call device_cold_reset() instead:
> > this will handle reset for both the parent class and the subclass,
> > and will work whether the classes are using legacy reset or 3-phase
> > reset. This allows us to remove the reset function that the subclass
> > currently has to set up.
> 
> Nice !
> 

Seconded.

Reviewed-by: Greg Kurz <groug@kaod.org>

> > 
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> Thanks,
> 
> C.
> 
> > ---
> >   hw/intc/xics.c             | 2 +-
> >   hw/pci-host/pnv_phb3_msi.c | 7 -------
> >   2 files changed, 1 insertion(+), 8 deletions(-)
> > 
> > diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> > index dcd021af668..dd130467ccc 100644
> > --- a/hw/intc/xics.c
> > +++ b/hw/intc/xics.c
> > @@ -593,7 +593,7 @@ static void ics_reset(DeviceState *dev)
> >   
> >   static void ics_reset_handler(void *dev)
> >   {
> > -    ics_reset(dev);
> > +    device_cold_reset(dev);
> >   }
> >   
> >   static void ics_realize(DeviceState *dev, Error **errp)
> > diff --git a/hw/pci-host/pnv_phb3_msi.c b/hw/pci-host/pnv_phb3_msi.c
> > index 2f4112907b8..ae908fd9e41 100644
> > --- a/hw/pci-host/pnv_phb3_msi.c
> > +++ b/hw/pci-host/pnv_phb3_msi.c
> > @@ -239,11 +239,6 @@ static void phb3_msi_reset(DeviceState *dev)
> >       msi->rba_sum = 0;
> >   }
> >   
> > -static void phb3_msi_reset_handler(void *dev)
> > -{
> > -    phb3_msi_reset(dev);
> > -}
> > -
> >   void pnv_phb3_msi_update_config(Phb3MsiState *msi, uint32_t base,
> >                                   uint32_t count)
> >   {
> > @@ -272,8 +267,6 @@ static void phb3_msi_realize(DeviceState *dev, Error **errp)
> >       }
> >   
> >       msi->qirqs = qemu_allocate_irqs(phb3_msi_set_irq, msi, ics->nr_irqs);
> > -
> > -    qemu_register_reset(phb3_msi_reset_handler, dev);
> >   }
> >   
> >   static void phb3_msi_instance_init(Object *obj)
> 

On 25/11/22 14:45, Greg Kurz wrote:
> On Fri, 25 Nov 2022 13:24:00 +0100
> Cédric Le Goater <clg@kaod.org> wrote:
> 
>> On 11/25/22 12:52, Peter Maydell wrote:
>>> The realize method for the TYPE_ICS class uses qemu_register_reset()
>>> to register a reset handler, as a workaround for the fact that
>>> currently objects which directly inherit from TYPE_DEVICE don't get
>>> automatically reset.  However, the reset function directly calls
>>> ics_reset(), which is the function that implements the legacy reset
>>> method.  This means that only the parent class's data gets reset, and
>>> a subclass which also needs to handle reset, like TYPE_PHB3_MSI, has
>>> to register its own reset function.
>>>
>>> Make the TYPE_ICS reset function call device_cold_reset() instead:
>>> this will handle reset for both the parent class and the subclass,
>>> and will work whether the classes are using legacy reset or 3-phase
>>> reset. This allows us to remove the reset function that the subclass
>>> currently has to set up.
>>
>> Nice !
>>
> 
> Seconded.
> 

Thirded :)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> Reviewed-by: Greg Kurz <groug@kaod.org>
> 
>>>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>
>> Reviewed-by: Cédric Le Goater <clg@kaod.org>