Re: [PATCH for-6.0 7/8] spapr: Drop "nr_servers" argument of the sPAPR IC activate() operation

From: Greg Kurz <groug@kaod.org>
To: David Gibson <david@gibson.dropbear.id.au>
Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org,
	"Cédric Le Goater" <clg@kaod.org>
Subject: Re: [PATCH for-6.0 7/8] spapr: Drop "nr_servers" argument of the sPAPR IC activate() operation
Date: Mon, 23 Nov 2020 10:47:41 +0100	[thread overview]
Message-ID: <20201123104741.7866bae9@bahia.lan> (raw)
In-Reply-To: <20201123043832.GG521467@yekko.fritz.box>

[-- Attachment #1: Type: text/plain, Size: 3806 bytes --]

On Mon, 23 Nov 2020 15:38:32 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Nov 20, 2020 at 06:46:45PM +0100, Greg Kurz wrote:
> > This argument isn't used by the backends anymore.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  include/hw/ppc/spapr_irq.h | 3 +--
> >  hw/intc/spapr_xive.c       | 3 +--
> >  hw/intc/xics_spapr.c       | 3 +--
> >  hw/ppc/spapr_irq.c         | 3 +--
> >  4 files changed, 4 insertions(+), 8 deletions(-)
> > 
> > diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> > index c22a72c9e270..3e1c619d4c06 100644
> > --- a/include/hw/ppc/spapr_irq.h
> > +++ b/include/hw/ppc/spapr_irq.h
> > @@ -43,8 +43,7 @@ DECLARE_CLASS_CHECKERS(SpaprInterruptControllerClass, SPAPR_INTC,
> >  struct SpaprInterruptControllerClass {
> >      InterfaceClass parent;
> >  
> > -    int (*activate)(SpaprInterruptController *intc, uint32_t nr_servers,
> > -                    Error **errp);
> 
> Hm.  Thinking back on this, is the problem just that it's not clear
> here if the 'nr_servers' parameter here indicates the number of CPU
> targets, or the maximum index of CPU targets?
> 

AIUI 'nr_servers' has always been referring to the number of consecutive
server ids that we put in the "ibm,interrupt-server-ranges" property for
XICS.

> If so, I wonder if it might be better to pass both numbers as
> parameters here, rather than shuffling the properties of the devices
> themselves.
> 

Maybe. I'll give a try.

> > +    int (*activate)(SpaprInterruptController *intc, Error **errp);
> >      void (*deactivate)(SpaprInterruptController *intc);
> >  
> >      /*
> > diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> > index d13a2955ce9b..8331026fdb12 100644
> > --- a/hw/intc/spapr_xive.c
> > +++ b/hw/intc/spapr_xive.c
> > @@ -754,8 +754,7 @@ static void spapr_xive_dt(SpaprInterruptController *intc, uint32_t nr_servers,
> >                       plat_res_int_priorities, sizeof(plat_res_int_priorities)));
> >  }
> >  
> > -static int spapr_xive_activate(SpaprInterruptController *intc,
> > -                               uint32_t nr_servers, Error **errp)
> > +static int spapr_xive_activate(SpaprInterruptController *intc, Error **errp)
> >  {
> >      SpaprXive *xive = SPAPR_XIVE(intc);
> >  
> > diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> > index ce147e8980ed..8810bd93c856 100644
> > --- a/hw/intc/xics_spapr.c
> > +++ b/hw/intc/xics_spapr.c
> > @@ -426,8 +426,7 @@ static int xics_spapr_post_load(SpaprInterruptController *intc, int version_id)
> >      return 0;
> >  }
> >  
> > -static int xics_spapr_activate(SpaprInterruptController *intc,
> > -                               uint32_t nr_servers, Error **errp)
> > +static int xics_spapr_activate(SpaprInterruptController *intc, Error **errp)
> >  {
> >      if (kvm_enabled()) {
> >          return spapr_irq_init_kvm(xics_kvm_connect, intc,
> > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> > index be6f4041e433..f2897fbc942a 100644
> > --- a/hw/ppc/spapr_irq.c
> > +++ b/hw/ppc/spapr_irq.c
> > @@ -480,7 +480,6 @@ static void set_active_intc(SpaprMachineState *spapr,
> >                              SpaprInterruptController *new_intc)
> >  {
> >      SpaprInterruptControllerClass *sicc;
> > -    uint32_t nr_servers = spapr_max_server_number(spapr);
> >  
> >      assert(new_intc);
> >  
> > @@ -498,7 +497,7 @@ static void set_active_intc(SpaprMachineState *spapr,
> >  
> >      sicc = SPAPR_INTC_GET_CLASS(new_intc);
> >      if (sicc->activate) {
> > -        sicc->activate(new_intc, nr_servers, &error_fatal);
> > +        sicc->activate(new_intc, &error_fatal);
> >      }
> >  
> >      spapr->active_intc = new_intc;
> 

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]