Re: [PATCH v3 30/34] spapr, xics, xive: Move SpaprIrq::reset hook logic into activate/deactivate

From: David Gibson <david@gibson.dropbear.id.au>
To: Greg Kurz <groug@kaod.org>
Cc: "Jason Wang" <jasowang@redhat.com>,
	"Riku Voipio" <riku.voipio@iki.fi>,
	qemu-devel@nongnu.org, "Laurent Vivier" <laurent@vivier.eu>,
	qemu-ppc@nongnu.org, clg@kaod.org,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Marc-André Lureau" <marcandre.lureau@redhat.com>,
	philmd@redhat.com
Subject: Re: [PATCH v3 30/34] spapr, xics, xive: Move SpaprIrq::reset hook logic into activate/deactivate
Date: Thu, 3 Oct 2019 10:22:22 +1000	[thread overview]
Message-ID: <20191003002222.GD11105@umbus.fritz.box> (raw)
In-Reply-To: <20191002112908.2bf1ea0b@bahia.lan>

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

On Wed, Oct 02, 2019 at 11:29:08AM +0200, Greg Kurz wrote:
> On Wed,  2 Oct 2019 12:52:04 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > It turns out that all the logic in the SpaprIrq::reset hooks (and some in
> > the SpaprIrq::post_load hooks) isn't really related to resetting the irq
> > backend (that's handled by the backends' own reset routines).  Rather its
> > about getting the backend ready to be the active interrupt controller or
> > stopping being the active interrupt controller - reset (and post_load) is
> > just the only time that changes at present.
> > 
> > To make this flow clearer, move the logic into the explicit backend
> > activate and deactivate hooks.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/intc/spapr_xive.c       | 35 ++++++++++++++++++++
> >  hw/intc/xics_spapr.c       | 16 +++++++++
> >  hw/ppc/spapr_irq.c         | 67 ++------------------------------------
> >  include/hw/ppc/spapr_irq.h |  4 ++-
> >  4 files changed, 57 insertions(+), 65 deletions(-)
> > 
> > diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> > index 37ffb74ca5..e8b946982c 100644
> > --- a/hw/intc/spapr_xive.c
> > +++ b/hw/intc/spapr_xive.c
> > @@ -640,6 +640,39 @@ static void spapr_xive_dt(SpaprInterruptController *intc, uint32_t nr_servers,
> >                       plat_res_int_priorities, sizeof(plat_res_int_priorities)));
> >  }
> >  
> > +static void spapr_xive_activate(SpaprInterruptController *intc, Error **errp)
> > +{
> > +    SpaprXive *xive = SPAPR_XIVE(intc);
> > +    CPUState *cs;
> > +
> > +    CPU_FOREACH(cs) {
> > +        PowerPCCPU *cpu = POWERPC_CPU(cs);
> > +
> > +        /* (TCG) Set the OS CAM line of the thread interrupt context. */
> > +        spapr_xive_set_tctx_os_cam(spapr_cpu_state(cpu)->tctx);
> > +    }
> > +
> 
> I think this ^^ can go...
> 
> > +    if (kvm_enabled()) {
> > +        if (spapr_irq_init_kvm(kvmppc_xive_connect, intc, errp) < 0) {
> > +            return;
> > +        }
> > +    }
> > +
> 
> ... here. If which case, spapr_irq_init_kvm() could be called from
> set_active_intc() instead of being called by each backend if I get
> it right. This would avoid the frontend->backend->frontend flow.

Hm.  I don't love the idea.  KVM setup seems like an internal
implementation detail of the backend, which I'd prefer not to handle
in the frontend.

I'm not really considering spapr_irq_init_kvm() as part of the
frontend proper, but rather a helper function for use in the backend.

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

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