Re: [PATCH 3/6] ppc: Reparent presenter objects to the interrupt controller object

[David Gibson]

On Thu, Oct 24, 2019 at 11:04:53AM +0200, Greg Kurz wrote:
> On Thu, 24 Oct 2019 13:58:41 +1100
> David Gibson <address@hidden> wrote:
> 
> > On Wed, Oct 23, 2019 at 04:52:10PM +0200, Greg Kurz wrote:
> > > Each VCPU is associated to a presenter object within the interrupt
> > > controller, ie. TCTX for XIVE and ICP for XICS, but our current
> > > models put these objects below the VCPU, and we rely on CPU_FOREACH()
> > > to do anything that involves presenters.
> > > 
> > > This recently bit us with the CAM line matching logic in XIVE because
> > > CPU_FOREACH() can race with CPU hotplug and we ended up considering a
> > > VCPU that wasn't associated to a TCTX object yet. Other users of
> > > CPU_FOREACH() are 'info pic' for both XICS and XIVE. It is again very
> > > easy to crash QEMU with concurrent VCPU hotplug/unplug and 'info pic'.
> > > 
> > > Reparent the presenter objects to the corresponding interrupt controller
> > > object, ie. XIVE router or ICS, to make it clear they are not some extra
> > > data hanging from the CPU but internal XIVE or XICS entities. The CPU
> > > object now needs to explicitely take a reference on the presenter to
> > > ensure its pointer remains valid until unrealize time.
> > > 
> > > This will allow to get rid of CPU_FOREACH() and ease further improvements
> > > to the XIVE model.
> > > 
> > > This change doesn't impact section ids and is thus transparent to
> > > migration.
> > > 
> > > Signed-off-by: Greg Kurz <address@hidden>
> > 
> > 
> > Urgh.  I see why we want something like this, but reparenting the
> > presenters to the source modules (particularly for XICS) makes me
> > uncomfortable.
> > 
> > AFAICT the association here is *purely* about managing lifetimes, not
> > because those ICPs inherently have something to do with those ICSes.
> > Same with XIVE, although since they'll be on the same chip there's a
> > bit more logic to it.
> > 
> 
> I did it this way for XIVE because they are indeed on the same chip,
> but I agree it is questionable for XICS.
> 
> > For spapr we kinda-sorta treat the (single) ICS or XiveRouter object
> > as the "master" of the interrupt controller.  But that's not really
> 
> Agreed for XICS. On the other side, the XIVE controller chip really has
> a routing sub-engine (IVRE) and a presenter sub-engine (IVPE), and the
> TCTXs do reside in an SRAM within the IVPE. The XiveRouter type might
> be better named XiveController, and each instance to expose a XiveRouter
> and a XivePresenter interface. We have one per chip for PNV and only a
> single one for sPAPR.

Yes, that sounds like a reasonable approach for XIVE.

> 
> > accurate to the hardware, so I don't really want to push that way of
> > looking at it any further than it already is.
> > 
> > If we could make the presenters children of the machine (spapr) or
> > chip (pnv) that might make more sense?
> > 
> 
> It probably makes sense for XICS, not sure this makes things clearer
> for XIVE.
> 
> > I'm also not totally convinced that having the presenter as a child of
> > the cpu is inherently bad.  Yes we have bugs now, but maybe the right
> > fix *is* actually to do the NULL check on every CPU_FOREACH().
> > 
> > For comparison, the lapic on x86 machines is a child of the cpu, and I
> > believe they do have NULL checks on cpu->apic_state in various places
> > they use CPU_FOREACH().
> > 
> 
> I didn't want to go that way because I was finding CPU_FOREACH() to
> be fragile since all users are broken,

Hm, ok.  There aren't that many existing users though, right?

> but I can revisit that. Maybe
> worth consolidating the logic in a PRESENTER_FOREACH() macro in order
> to avoid future breakage with CPU_FOREACH() ?

That sounds worth considering at least, yes.

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

signature.asc
Description: PGP signature