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);
}
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 {
--
2.21.0
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 {
>
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 {
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 {
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
© 2016 - 2026 Red Hat, Inc.