Re: [PATCH v2 29/33] spapr, xics, xive: Move SpaprIrq::reset hook logic into activate/deactivate

[David Gibson]

On Mon, Sep 30, 2019 at 09:29:14PM +0200, Cédric Le Goater wrote:
> On 30/09/2019 10:25, David Gibson wrote:
> > On Mon, Sep 30, 2019 at 08:11:56AM +0200, Cédric Le Goater wrote:
> >> On 27/09/2019 07:50, David Gibson 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.
> >>
> >> This is a 'critical' part which impacts all the migration cases: 
> >>
> >> ic-mode=xics,xive,dual + kernel_irqchip=on/off + TCG
> > 
> > Yes... and?
> 
> and it's fragile.

How so?  Explicitly having logic for when an intc becomes active or
inactive, and having a single callsite which does that and updates the
active controller seems less fragile to me.  At least compared to
having the update to the active controller (implicit in changing the
CAS vector) and the logic to get the controllers ready for being
active/inactive in totally different places and relying on the fact
they both only happen at reset for them to travel together.

> 
> >>> To make this flow clearer, move the logic into the explicit backend
> >>> activate and deactivate hooks.
> >>
> >> I don't see where the hooks are called ?
> > 
> > spapr_irq_reset()
> >   -> spapr_irq_update_active_intc()
> >     -> set_active_intc()
> >       -> activate/deactivate hooks
> > 
> > Similarly via spapr_irq_post_load().
> > 
> > I'm hoping to add one at CAS time to avoid the CAS reboot, too.
> 
> OK. I think the first 22 patches are ready, just some minor comments
> if I am correct. Could you merge them ?

I might repost first, because one of the changes Greg suggested to
error handling caused a larger than expected number of ripple on
fixes.

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