Re: [PATCH 04/20] xics: Eliminate reset hook

From: "Cédric Le Goater" <clg@kaod.org>
To: David Gibson <david@gibson.dropbear.id.au>,
	qemu-ppc@nongnu.org, qemu-devel@nongnu.org
Cc: "Jason Wang" <jasowang@redhat.com>,
	"Riku Voipio" <riku.voipio@iki.fi>,
	groug@kaod.org, "Laurent Vivier" <laurent@vivier.eu>,
	"Marc-André Lureau" <marcandre.lureau@redhat.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	philmd@redhat.com
Subject: Re: [PATCH 04/20] xics: Eliminate reset hook
Date: Wed, 25 Sep 2019 09:33:04 +0200	[thread overview]
Message-ID: <d4e86f40-90c5-c172-2743-14115dfd7830@kaod.org> (raw)
In-Reply-To: <20190925064534.19155-5-david@gibson.dropbear.id.au>

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 {
>