diff mbox series

[04/20] xics: Eliminate reset hook

Message ID 20190925064534.19155-5-david@gibson.dropbear.id.au (mailing list archive)
State New, archived
Headers show
Series spapr: IRQ subsystem cleanups | expand

Commit Message

David Gibson Sept. 25, 2019, 6:45 a.m. UTC 
Currently TYPE_XICS_BASE and TYPE_XICS_SIMPLE have their own reset methods,
using the standard technique for having the subtype call the supertype's
methods before doing its own thing.

But TYPE_XICS_SIMPLE is the only subtype of TYPE_XICS_BASE ever
instantiated, so there's no point having the split here.  Merge them
together into just an ics_reset() function.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/intc/xics.c        | 57 ++++++++++++++++++-------------------------
 include/hw/ppc/xics.h |  1 -
 2 files changed, 24 insertions(+), 34 deletions(-)

Comments

Cédric Le Goater Sept. 25, 2019, 7:33 a.m. UTC | #1 
On 25/09/2019 08:45, David Gibson wrote:
> Currently TYPE_XICS_BASE and TYPE_XICS_SIMPLE have their own reset methods,
> using the standard technique for having the subtype call the supertype's
> methods before doing its own thing.
> 
> But TYPE_XICS_SIMPLE is the only subtype of TYPE_XICS_BASE ever
> instantiated, so there's no point having the split here.  Merge them
> together into just an ics_reset() function.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

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

> ---
>  hw/intc/xics.c        | 57 ++++++++++++++++++-------------------------
>  include/hw/ppc/xics.h |  1 -
>  2 files changed, 24 insertions(+), 34 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 310dc72b46..82e6f09259 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -547,11 +547,28 @@ static void ics_eoi(ICSState *ics, uint32_t nr)
>      }
>  }
>  
> -static void ics_simple_reset(DeviceState *dev)
> +static void ics_reset_irq(ICSIRQState *irq)
>  {
> -    ICSStateClass *icsc = ICS_BASE_GET_CLASS(dev);
> +    irq->priority = 0xff;
> +    irq->saved_priority = 0xff;
> +}
>  
> -    icsc->parent_reset(dev);
> +static void ics_reset(DeviceState *dev)
> +{
> +    ICSState *ics = ICS_BASE(dev);
> +    int i;
> +    uint8_t flags[ics->nr_irqs];
> +
> +    for (i = 0; i < ics->nr_irqs; i++) {
> +        flags[i] = ics->irqs[i].flags;
> +    }
> +
> +    memset(ics->irqs, 0, sizeof(ICSIRQState) * ics->nr_irqs);
> +
> +    for (i = 0; i < ics->nr_irqs; i++) {
> +        ics_reset_irq(ics->irqs + i);
> +        ics->irqs[i].flags = flags[i];
> +    }
>  
>      if (kvm_irqchip_in_kernel()) {
>          Error *local_err = NULL;
> @@ -563,9 +580,9 @@ static void ics_simple_reset(DeviceState *dev)
>      }
>  }
>  
> -static void ics_simple_reset_handler(void *dev)
> +static void ics_reset_handler(void *dev)
>  {
> -    ics_simple_reset(dev);
> +    ics_reset(dev);
>  }
>  
>  static void ics_simple_realize(DeviceState *dev, Error **errp)
> @@ -580,7 +597,7 @@ static void ics_simple_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> -    qemu_register_reset(ics_simple_reset_handler, ics);
> +    qemu_register_reset(ics_reset_handler, ics);
>  }
>  
>  static void ics_simple_class_init(ObjectClass *klass, void *data)
> @@ -590,8 +607,6 @@ static void ics_simple_class_init(ObjectClass *klass, void *data)
>  
>      device_class_set_parent_realize(dc, ics_simple_realize,
>                                      &isc->parent_realize);
> -    device_class_set_parent_reset(dc, ics_simple_reset,
> -                                  &isc->parent_reset);
>  }
>  
>  static const TypeInfo ics_simple_info = {
> @@ -602,30 +617,6 @@ static const TypeInfo ics_simple_info = {
>      .class_size = sizeof(ICSStateClass),
>  };
>  
> -static void ics_reset_irq(ICSIRQState *irq)
> -{
> -    irq->priority = 0xff;
> -    irq->saved_priority = 0xff;
> -}
> -
> -static void ics_base_reset(DeviceState *dev)
> -{
> -    ICSState *ics = ICS_BASE(dev);
> -    int i;
> -    uint8_t flags[ics->nr_irqs];
> -
> -    for (i = 0; i < ics->nr_irqs; i++) {
> -        flags[i] = ics->irqs[i].flags;
> -    }
> -
> -    memset(ics->irqs, 0, sizeof(ICSIRQState) * ics->nr_irqs);
> -
> -    for (i = 0; i < ics->nr_irqs; i++) {
> -        ics_reset_irq(ics->irqs + i);
> -        ics->irqs[i].flags = flags[i];
> -    }
> -}
> -
>  static void ics_base_realize(DeviceState *dev, Error **errp)
>  {
>      ICSState *ics = ICS_BASE(dev);
> @@ -726,7 +717,7 @@ static void ics_base_class_init(ObjectClass *klass, void *data)
>  
>      dc->realize = ics_base_realize;
>      dc->props = ics_base_properties;
> -    dc->reset = ics_base_reset;
> +    dc->reset = ics_reset;
>      dc->vmsd = &vmstate_ics_base;
>  }
>  
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index e72fb67968..18fcd2b11c 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -105,7 +105,6 @@ struct ICSStateClass {
>      DeviceClass parent_class;
>  
>      DeviceRealize parent_realize;
> -    DeviceReset parent_reset;
>  };
>  
>  struct ICSState {
>
Greg Kurz Sept. 25, 2019, 7:59 a.m. UTC | #2 
On Wed, 25 Sep 2019 16:45:18 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> Currently TYPE_XICS_BASE and TYPE_XICS_SIMPLE have their own reset methods,
> using the standard technique for having the subtype call the supertype's
> methods before doing its own thing.
> 
> But TYPE_XICS_SIMPLE is the only subtype of TYPE_XICS_BASE ever
> instantiated, so there's no point having the split here.  Merge them
> together into just an ics_reset() function.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/intc/xics.c        | 57 ++++++++++++++++++-------------------------
>  include/hw/ppc/xics.h |  1 -
>  2 files changed, 24 insertions(+), 34 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 310dc72b46..82e6f09259 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -547,11 +547,28 @@ static void ics_eoi(ICSState *ics, uint32_t nr)
>      }
>  }
>  
> -static void ics_simple_reset(DeviceState *dev)
> +static void ics_reset_irq(ICSIRQState *irq)
>  {
> -    ICSStateClass *icsc = ICS_BASE_GET_CLASS(dev);
> +    irq->priority = 0xff;
> +    irq->saved_priority = 0xff;
> +}
>  
> -    icsc->parent_reset(dev);
> +static void ics_reset(DeviceState *dev)
> +{
> +    ICSState *ics = ICS_BASE(dev);
> +    int i;
> +    uint8_t flags[ics->nr_irqs];
> +
> +    for (i = 0; i < ics->nr_irqs; i++) {
> +        flags[i] = ics->irqs[i].flags;
> +    }
> +
> +    memset(ics->irqs, 0, sizeof(ICSIRQState) * ics->nr_irqs);
> +
> +    for (i = 0; i < ics->nr_irqs; i++) {
> +        ics_reset_irq(ics->irqs + i);
> +        ics->irqs[i].flags = flags[i];
> +    }
>  
>      if (kvm_irqchip_in_kernel()) {
>          Error *local_err = NULL;
> @@ -563,9 +580,9 @@ static void ics_simple_reset(DeviceState *dev)
>      }
>  }
>  
> -static void ics_simple_reset_handler(void *dev)
> +static void ics_reset_handler(void *dev)
>  {
> -    ics_simple_reset(dev);
> +    ics_reset(dev);
>  }
>  
>  static void ics_simple_realize(DeviceState *dev, Error **errp)
> @@ -580,7 +597,7 @@ static void ics_simple_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> -    qemu_register_reset(ics_simple_reset_handler, ics);
> +    qemu_register_reset(ics_reset_handler, ics);

As suggested by Philippe, this could be the opportunity to add
a comment that explain why we rely on qemu_register_reset()
rather than dc->reset.

>  }
>  
>  static void ics_simple_class_init(ObjectClass *klass, void *data)
> @@ -590,8 +607,6 @@ static void ics_simple_class_init(ObjectClass *klass, void *data)
>  
>      device_class_set_parent_realize(dc, ics_simple_realize,
>                                      &isc->parent_realize);
> -    device_class_set_parent_reset(dc, ics_simple_reset,
> -                                  &isc->parent_reset);
>  }
>  
>  static const TypeInfo ics_simple_info = {
> @@ -602,30 +617,6 @@ static const TypeInfo ics_simple_info = {
>      .class_size = sizeof(ICSStateClass),
>  };
>  
> -static void ics_reset_irq(ICSIRQState *irq)
> -{
> -    irq->priority = 0xff;
> -    irq->saved_priority = 0xff;
> -}
> -
> -static void ics_base_reset(DeviceState *dev)
> -{
> -    ICSState *ics = ICS_BASE(dev);
> -    int i;
> -    uint8_t flags[ics->nr_irqs];
> -
> -    for (i = 0; i < ics->nr_irqs; i++) {
> -        flags[i] = ics->irqs[i].flags;
> -    }
> -
> -    memset(ics->irqs, 0, sizeof(ICSIRQState) * ics->nr_irqs);
> -
> -    for (i = 0; i < ics->nr_irqs; i++) {
> -        ics_reset_irq(ics->irqs + i);
> -        ics->irqs[i].flags = flags[i];
> -    }
> -}
> -
>  static void ics_base_realize(DeviceState *dev, Error **errp)
>  {
>      ICSState *ics = ICS_BASE(dev);
> @@ -726,7 +717,7 @@ static void ics_base_class_init(ObjectClass *klass, void *data)
>  
>      dc->realize = ics_base_realize;
>      dc->props = ics_base_properties;
> -    dc->reset = ics_base_reset;
> +    dc->reset = ics_reset;

I hadn't spotted it previously but since you're removing the call to
device_class_set_parent_reset(), we don't need dc->reset anymore.

This basically reverts:

commit eeefd43b3cf342d1696128462a16e092995ff1b5
Author: Cédric Le Goater <clg@kaod.org>
Date:   Mon Jun 25 11:17:16 2018 +0200

    ppx/xics: introduce a parent_reset in ICSStateClass
    
    Just like for the realize handlers, this makes possible to move the
    common ICSState code of the reset handlers in the ics-base class.
    
    Signed-off-by: Cédric Le Goater <clg@kaod.org>
    Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

With dc->reset removed,

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

>      dc->vmsd = &vmstate_ics_base;
>  }
>  
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index e72fb67968..18fcd2b11c 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -105,7 +105,6 @@ struct ICSStateClass {
>      DeviceClass parent_class;
>  
>      DeviceRealize parent_realize;
> -    DeviceReset parent_reset;
>  };
>  
>  struct ICSState {
David Gibson Sept. 26, 2019, 2:54 a.m. UTC | #3 
On Wed, Sep 25, 2019 at 09:59:52AM +0200, Greg Kurz wrote:
> On Wed, 25 Sep 2019 16:45:18 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Currently TYPE_XICS_BASE and TYPE_XICS_SIMPLE have their own reset methods,
> > using the standard technique for having the subtype call the supertype's
> > methods before doing its own thing.
> > 
> > But TYPE_XICS_SIMPLE is the only subtype of TYPE_XICS_BASE ever
> > instantiated, so there's no point having the split here.  Merge them
> > together into just an ics_reset() function.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/intc/xics.c        | 57 ++++++++++++++++++-------------------------
> >  include/hw/ppc/xics.h |  1 -
> >  2 files changed, 24 insertions(+), 34 deletions(-)
> > 
> > diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> > index 310dc72b46..82e6f09259 100644
> > --- a/hw/intc/xics.c
> > +++ b/hw/intc/xics.c
> > @@ -547,11 +547,28 @@ static void ics_eoi(ICSState *ics, uint32_t nr)
> >      }
> >  }
> >  
> > -static void ics_simple_reset(DeviceState *dev)
> > +static void ics_reset_irq(ICSIRQState *irq)
> >  {
> > -    ICSStateClass *icsc = ICS_BASE_GET_CLASS(dev);
> > +    irq->priority = 0xff;
> > +    irq->saved_priority = 0xff;
> > +}
> >  
> > -    icsc->parent_reset(dev);
> > +static void ics_reset(DeviceState *dev)
> > +{
> > +    ICSState *ics = ICS_BASE(dev);
> > +    int i;
> > +    uint8_t flags[ics->nr_irqs];
> > +
> > +    for (i = 0; i < ics->nr_irqs; i++) {
> > +        flags[i] = ics->irqs[i].flags;
> > +    }
> > +
> > +    memset(ics->irqs, 0, sizeof(ICSIRQState) * ics->nr_irqs);
> > +
> > +    for (i = 0; i < ics->nr_irqs; i++) {
> > +        ics_reset_irq(ics->irqs + i);
> > +        ics->irqs[i].flags = flags[i];
> > +    }
> >  
> >      if (kvm_irqchip_in_kernel()) {
> >          Error *local_err = NULL;
> > @@ -563,9 +580,9 @@ static void ics_simple_reset(DeviceState *dev)
> >      }
> >  }
> >  
> > -static void ics_simple_reset_handler(void *dev)
> > +static void ics_reset_handler(void *dev)
> >  {
> > -    ics_simple_reset(dev);
> > +    ics_reset(dev);
> >  }
> >  
> >  static void ics_simple_realize(DeviceState *dev, Error **errp)
> > @@ -580,7 +597,7 @@ static void ics_simple_realize(DeviceState *dev, Error **errp)
> >          return;
> >      }
> >  
> > -    qemu_register_reset(ics_simple_reset_handler, ics);
> > +    qemu_register_reset(ics_reset_handler, ics);
> 
> As suggested by Philippe, this could be the opportunity to add
> a comment that explain why we rely on qemu_register_reset()
> rather than dc->reset.

I don't thinmk it's really in scope for this patch, since it was there
just as bare before.  I'm considering it for another patch, but I'm
still thinking about exactly what I want to do with the reset.

> 
> >  }
> >  
> >  static void ics_simple_class_init(ObjectClass *klass, void *data)
> > @@ -590,8 +607,6 @@ static void ics_simple_class_init(ObjectClass *klass, void *data)
> >  
> >      device_class_set_parent_realize(dc, ics_simple_realize,
> >                                      &isc->parent_realize);
> > -    device_class_set_parent_reset(dc, ics_simple_reset,
> > -                                  &isc->parent_reset);
> >  }
> >  
> >  static const TypeInfo ics_simple_info = {
> > @@ -602,30 +617,6 @@ static const TypeInfo ics_simple_info = {
> >      .class_size = sizeof(ICSStateClass),
> >  };
> >  
> > -static void ics_reset_irq(ICSIRQState *irq)
> > -{
> > -    irq->priority = 0xff;
> > -    irq->saved_priority = 0xff;
> > -}
> > -
> > -static void ics_base_reset(DeviceState *dev)
> > -{
> > -    ICSState *ics = ICS_BASE(dev);
> > -    int i;
> > -    uint8_t flags[ics->nr_irqs];
> > -
> > -    for (i = 0; i < ics->nr_irqs; i++) {
> > -        flags[i] = ics->irqs[i].flags;
> > -    }
> > -
> > -    memset(ics->irqs, 0, sizeof(ICSIRQState) * ics->nr_irqs);
> > -
> > -    for (i = 0; i < ics->nr_irqs; i++) {
> > -        ics_reset_irq(ics->irqs + i);
> > -        ics->irqs[i].flags = flags[i];
> > -    }
> > -}
> > -
> >  static void ics_base_realize(DeviceState *dev, Error **errp)
> >  {
> >      ICSState *ics = ICS_BASE(dev);
> > @@ -726,7 +717,7 @@ static void ics_base_class_init(ObjectClass *klass, void *data)
> >  
> >      dc->realize = ics_base_realize;
> >      dc->props = ics_base_properties;
> > -    dc->reset = ics_base_reset;
> > +    dc->reset = ics_reset;
> 
> I hadn't spotted it previously but since you're removing the call to
> device_class_set_parent_reset(), we don't need dc->reset anymore.

Hrm, I'd prefer to leave it in there, even though it's not strictly
necessary - this way calling device_reset() on the ICS will do what
you'd expect

> 
> This basically reverts:
> 
> commit eeefd43b3cf342d1696128462a16e092995ff1b5
> Author: Cédric Le Goater <clg@kaod.org>
> Date:   Mon Jun 25 11:17:16 2018 +0200
> 
>     ppx/xics: introduce a parent_reset in ICSStateClass
>     
>     Just like for the realize handlers, this makes possible to move the
>     common ICSState code of the reset handlers in the ics-base class.
>     
>     Signed-off-by: Cédric Le Goater <clg@kaod.org>
>     Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> 
> With dc->reset removed,
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> >      dc->vmsd = &vmstate_ics_base;
> >  }
> >  
> > diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> > index e72fb67968..18fcd2b11c 100644
> > --- a/include/hw/ppc/xics.h
> > +++ b/include/hw/ppc/xics.h
> > @@ -105,7 +105,6 @@ struct ICSStateClass {
> >      DeviceClass parent_class;
> >  
> >      DeviceRealize parent_realize;
> > -    DeviceReset parent_reset;
> >  };
> >  
> >  struct ICSState {
>
diff mbox series

Patch

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index 310dc72b46..82e6f09259 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -547,11 +547,28 @@  static void ics_eoi(ICSState *ics, uint32_t nr)
     }
 }
 
-static void ics_simple_reset(DeviceState *dev)
+static void ics_reset_irq(ICSIRQState *irq)
 {
-    ICSStateClass *icsc = ICS_BASE_GET_CLASS(dev);
+    irq->priority = 0xff;
+    irq->saved_priority = 0xff;
+}
 
-    icsc->parent_reset(dev);
+static void ics_reset(DeviceState *dev)
+{
+    ICSState *ics = ICS_BASE(dev);
+    int i;
+    uint8_t flags[ics->nr_irqs];
+
+    for (i = 0; i < ics->nr_irqs; i++) {
+        flags[i] = ics->irqs[i].flags;
+    }
+
+    memset(ics->irqs, 0, sizeof(ICSIRQState) * ics->nr_irqs);
+
+    for (i = 0; i < ics->nr_irqs; i++) {
+        ics_reset_irq(ics->irqs + i);
+        ics->irqs[i].flags = flags[i];
+    }
 
     if (kvm_irqchip_in_kernel()) {
         Error *local_err = NULL;
@@ -563,9 +580,9 @@  static void ics_simple_reset(DeviceState *dev)
     }
 }
 
-static void ics_simple_reset_handler(void *dev)
+static void ics_reset_handler(void *dev)
 {
-    ics_simple_reset(dev);
+    ics_reset(dev);
 }
 
 static void ics_simple_realize(DeviceState *dev, Error **errp)
@@ -580,7 +597,7 @@  static void ics_simple_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    qemu_register_reset(ics_simple_reset_handler, ics);
+    qemu_register_reset(ics_reset_handler, ics);
 }
 
 static void ics_simple_class_init(ObjectClass *klass, void *data)
@@ -590,8 +607,6 @@  static void ics_simple_class_init(ObjectClass *klass, void *data)
 
     device_class_set_parent_realize(dc, ics_simple_realize,
                                     &isc->parent_realize);
-    device_class_set_parent_reset(dc, ics_simple_reset,
-                                  &isc->parent_reset);
 }
 
 static const TypeInfo ics_simple_info = {
@@ -602,30 +617,6 @@  static const TypeInfo ics_simple_info = {
     .class_size = sizeof(ICSStateClass),
 };
 
-static void ics_reset_irq(ICSIRQState *irq)
-{
-    irq->priority = 0xff;
-    irq->saved_priority = 0xff;
-}
-
-static void ics_base_reset(DeviceState *dev)
-{
-    ICSState *ics = ICS_BASE(dev);
-    int i;
-    uint8_t flags[ics->nr_irqs];
-
-    for (i = 0; i < ics->nr_irqs; i++) {
-        flags[i] = ics->irqs[i].flags;
-    }
-
-    memset(ics->irqs, 0, sizeof(ICSIRQState) * ics->nr_irqs);
-
-    for (i = 0; i < ics->nr_irqs; i++) {
-        ics_reset_irq(ics->irqs + i);
-        ics->irqs[i].flags = flags[i];
-    }
-}
-
 static void ics_base_realize(DeviceState *dev, Error **errp)
 {
     ICSState *ics = ICS_BASE(dev);
@@ -726,7 +717,7 @@  static void ics_base_class_init(ObjectClass *klass, void *data)
 
     dc->realize = ics_base_realize;
     dc->props = ics_base_properties;
-    dc->reset = ics_base_reset;
+    dc->reset = ics_reset;
     dc->vmsd = &vmstate_ics_base;
 }
 
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index e72fb67968..18fcd2b11c 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -105,7 +105,6 @@  struct ICSStateClass {
     DeviceClass parent_class;
 
     DeviceRealize parent_realize;
-    DeviceReset parent_reset;
 };
 
 struct ICSState {