Message ID | 20191009060818.29719-8-david@gibson.dropbear.id.au |
---|---|
State | New |
Headers | show |
Series | spapr: IRQ subsystem cleanup | expand |
On 09/10/2019 08:08, David Gibson wrote: > spapr now has the mechanism of constructing both XICS and XIVE instances of > the SpaprInterruptController interface. However, only one of the interrupt > controllers will actually be active at any given time, depending on feature > negotiation with the guest. This is handled in the current code via > spapr_irq_current() which checks the OV5 vector from feature negotiation to > determine the current backend. > > Determining the active controller at the point we need it like this > can be pretty confusing, because it makes it very non obvious at what > points the active controller can change. This can make it difficult > to reason about the code and where a change of active controller could > appear in sequence with other events. > > Make this mechanism more explicit by adding an 'active_intc' pointer > and an explicit spapr_irq_update_active_intc() function to update it > from the CAS state. We also add hooks on the intc backend which will > get called when it is activated or deactivated. > > For now we just introduce the switch and hooks, later patches will > actually start using them. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > Reviewed-by: Greg Kurz <groug@kaod.org> Reviewed-by: Cédric Le Goater <clg@kaod.org> > --- > hw/ppc/spapr_irq.c | 51 ++++++++++++++++++++++++++++++++++++++ > include/hw/ppc/spapr.h | 5 ++-- > include/hw/ppc/spapr_irq.h | 5 ++++ > 3 files changed, 59 insertions(+), 2 deletions(-) > > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c > index 83882cfad3..249a2688ac 100644 > --- a/hw/ppc/spapr_irq.c > +++ b/hw/ppc/spapr_irq.c > @@ -586,6 +586,7 @@ qemu_irq spapr_qirq(SpaprMachineState *spapr, int irq) > > int spapr_irq_post_load(SpaprMachineState *spapr, int version_id) > { > + spapr_irq_update_active_intc(spapr); > return spapr->irq->post_load(spapr, version_id); > } > > @@ -593,6 +594,8 @@ void spapr_irq_reset(SpaprMachineState *spapr, Error **errp) > { > assert(!spapr->irq_map || bitmap_empty(spapr->irq_map, spapr->irq_map_nr)); > > + spapr_irq_update_active_intc(spapr); > + > if (spapr->irq->reset) { > spapr->irq->reset(spapr, errp); > } > @@ -619,6 +622,54 @@ int spapr_irq_get_phandle(SpaprMachineState *spapr, void *fdt, Error **errp) > return phandle; > } > > +static void set_active_intc(SpaprMachineState *spapr, > + SpaprInterruptController *new_intc) > +{ > + SpaprInterruptControllerClass *sicc; > + > + assert(new_intc); > + > + if (new_intc == spapr->active_intc) { > + /* Nothing to do */ > + return; > + } > + > + if (spapr->active_intc) { > + sicc = SPAPR_INTC_GET_CLASS(spapr->active_intc); > + if (sicc->deactivate) { > + sicc->deactivate(spapr->active_intc); > + } > + } > + > + sicc = SPAPR_INTC_GET_CLASS(new_intc); > + if (sicc->activate) { > + sicc->activate(new_intc, &error_fatal); > + } > + > + spapr->active_intc = new_intc; > +} > + > +void spapr_irq_update_active_intc(SpaprMachineState *spapr) > +{ > + SpaprInterruptController *new_intc; > + > + if (!spapr->ics) { > + /* > + * XXX before we run CAS, ov5_cas is initialized empty, which > + * indicates XICS, even if we have ic-mode=xive. TODO: clean > + * up the CAS path so that we have a clearer way of handling > + * this. > + */ > + new_intc = SPAPR_INTC(spapr->xive); > + } else if (spapr_ovec_test(spapr->ov5_cas, OV5_XIVE_EXPLOIT)) { > + new_intc = SPAPR_INTC(spapr->xive); > + } else { > + new_intc = SPAPR_INTC(spapr->ics); > + } > + > + set_active_intc(spapr, new_intc); > +} > + > /* > * XICS legacy routines - to deprecate one day > */ > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index cbd1a4c9f3..763da757f0 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -143,7 +143,6 @@ struct SpaprMachineState { > struct SpaprVioBus *vio_bus; > QLIST_HEAD(, SpaprPhbState) phbs; > struct SpaprNvram *nvram; > - ICSState *ics; > SpaprRtcState rtc; > > SpaprResizeHpt resize_hpt; > @@ -195,9 +194,11 @@ struct SpaprMachineState { > > int32_t irq_map_nr; > unsigned long *irq_map; > - SpaprXive *xive; > SpaprIrq *irq; > qemu_irq *qirqs; > + SpaprInterruptController *active_intc; > + ICSState *ics; > + SpaprXive *xive; > > bool cmd_line_caps[SPAPR_CAP_NUM]; > SpaprCapabilities def, eff, mig; > diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h > index adfef0fcbe..593059eff5 100644 > --- a/include/hw/ppc/spapr_irq.h > +++ b/include/hw/ppc/spapr_irq.h > @@ -44,6 +44,9 @@ typedef struct SpaprInterruptController SpaprInterruptController; > typedef struct SpaprInterruptControllerClass { > InterfaceClass parent; > > + int (*activate)(SpaprInterruptController *intc, Error **errp); > + void (*deactivate)(SpaprInterruptController *intc); > + > /* > * These methods will typically be called on all intcs, active and > * inactive > @@ -55,6 +58,8 @@ typedef struct SpaprInterruptControllerClass { > void (*free_irq)(SpaprInterruptController *intc, int irq); > } SpaprInterruptControllerClass; > > +void spapr_irq_update_active_intc(SpaprMachineState *spapr); > + > int spapr_irq_cpu_intc_create(SpaprMachineState *spapr, > PowerPCCPU *cpu, Error **errp); > >
On 09/10/2019 08:08, David Gibson wrote: > spapr now has the mechanism of constructing both XICS and XIVE instances of > the SpaprInterruptController interface. However, only one of the interrupt > controllers will actually be active at any given time, depending on feature > negotiation with the guest. This is handled in the current code via > spapr_irq_current() which checks the OV5 vector from feature negotiation to > determine the current backend. > > Determining the active controller at the point we need it like this > can be pretty confusing, because it makes it very non obvious at what > points the active controller can change. This can make it difficult > to reason about the code and where a change of active controller could > appear in sequence with other events. > > Make this mechanism more explicit by adding an 'active_intc' pointer > and an explicit spapr_irq_update_active_intc() function to update it > from the CAS state. We also add hooks on the intc backend which will > get called when it is activated or deactivated. > > For now we just introduce the switch and hooks, later patches will > actually start using them. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > Reviewed-by: Greg Kurz <groug@kaod.org> Btw, we will need an extra xive2 pointer for the Power10 interrupt controller. C. > --- > hw/ppc/spapr_irq.c | 51 ++++++++++++++++++++++++++++++++++++++ > include/hw/ppc/spapr.h | 5 ++-- > include/hw/ppc/spapr_irq.h | 5 ++++ > 3 files changed, 59 insertions(+), 2 deletions(-) > > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c > index 83882cfad3..249a2688ac 100644 > --- a/hw/ppc/spapr_irq.c > +++ b/hw/ppc/spapr_irq.c > @@ -586,6 +586,7 @@ qemu_irq spapr_qirq(SpaprMachineState *spapr, int irq) > > int spapr_irq_post_load(SpaprMachineState *spapr, int version_id) > { > + spapr_irq_update_active_intc(spapr); > return spapr->irq->post_load(spapr, version_id); > } > > @@ -593,6 +594,8 @@ void spapr_irq_reset(SpaprMachineState *spapr, Error **errp) > { > assert(!spapr->irq_map || bitmap_empty(spapr->irq_map, spapr->irq_map_nr)); > > + spapr_irq_update_active_intc(spapr); > + > if (spapr->irq->reset) { > spapr->irq->reset(spapr, errp); > } > @@ -619,6 +622,54 @@ int spapr_irq_get_phandle(SpaprMachineState *spapr, void *fdt, Error **errp) > return phandle; > } > > +static void set_active_intc(SpaprMachineState *spapr, > + SpaprInterruptController *new_intc) > +{ > + SpaprInterruptControllerClass *sicc; > + > + assert(new_intc); > + > + if (new_intc == spapr->active_intc) { > + /* Nothing to do */ > + return; > + } > + > + if (spapr->active_intc) { > + sicc = SPAPR_INTC_GET_CLASS(spapr->active_intc); > + if (sicc->deactivate) { > + sicc->deactivate(spapr->active_intc); > + } > + } > + > + sicc = SPAPR_INTC_GET_CLASS(new_intc); > + if (sicc->activate) { > + sicc->activate(new_intc, &error_fatal); > + } > + > + spapr->active_intc = new_intc; > +} > + > +void spapr_irq_update_active_intc(SpaprMachineState *spapr) > +{ > + SpaprInterruptController *new_intc; > + > + if (!spapr->ics) { > + /* > + * XXX before we run CAS, ov5_cas is initialized empty, which > + * indicates XICS, even if we have ic-mode=xive. TODO: clean > + * up the CAS path so that we have a clearer way of handling > + * this. > + */ > + new_intc = SPAPR_INTC(spapr->xive); > + } else if (spapr_ovec_test(spapr->ov5_cas, OV5_XIVE_EXPLOIT)) { > + new_intc = SPAPR_INTC(spapr->xive); > + } else { > + new_intc = SPAPR_INTC(spapr->ics); > + } > + > + set_active_intc(spapr, new_intc); > +} > + > /* > * XICS legacy routines - to deprecate one day > */ > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index cbd1a4c9f3..763da757f0 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -143,7 +143,6 @@ struct SpaprMachineState { > struct SpaprVioBus *vio_bus; > QLIST_HEAD(, SpaprPhbState) phbs; > struct SpaprNvram *nvram; > - ICSState *ics; > SpaprRtcState rtc; > > SpaprResizeHpt resize_hpt; > @@ -195,9 +194,11 @@ struct SpaprMachineState { > > int32_t irq_map_nr; > unsigned long *irq_map; > - SpaprXive *xive; > SpaprIrq *irq; > qemu_irq *qirqs; > + SpaprInterruptController *active_intc; > + ICSState *ics; > + SpaprXive *xive; > > bool cmd_line_caps[SPAPR_CAP_NUM]; > SpaprCapabilities def, eff, mig; > diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h > index adfef0fcbe..593059eff5 100644 > --- a/include/hw/ppc/spapr_irq.h > +++ b/include/hw/ppc/spapr_irq.h > @@ -44,6 +44,9 @@ typedef struct SpaprInterruptController SpaprInterruptController; > typedef struct SpaprInterruptControllerClass { > InterfaceClass parent; > > + int (*activate)(SpaprInterruptController *intc, Error **errp); > + void (*deactivate)(SpaprInterruptController *intc); > + > /* > * These methods will typically be called on all intcs, active and > * inactive > @@ -55,6 +58,8 @@ typedef struct SpaprInterruptControllerClass { > void (*free_irq)(SpaprInterruptController *intc, int irq); > } SpaprInterruptControllerClass; > > +void spapr_irq_update_active_intc(SpaprMachineState *spapr); > + > int spapr_irq_cpu_intc_create(SpaprMachineState *spapr, > PowerPCCPU *cpu, Error **errp); > >
On Wed, Oct 09, 2019 at 11:19:26AM +0200, Cédric Le Goater wrote: > On 09/10/2019 08:08, David Gibson wrote: > > spapr now has the mechanism of constructing both XICS and XIVE instances of > > the SpaprInterruptController interface. However, only one of the interrupt > > controllers will actually be active at any given time, depending on feature > > negotiation with the guest. This is handled in the current code via > > spapr_irq_current() which checks the OV5 vector from feature negotiation to > > determine the current backend. > > > > Determining the active controller at the point we need it like this > > can be pretty confusing, because it makes it very non obvious at what > > points the active controller can change. This can make it difficult > > to reason about the code and where a change of active controller could > > appear in sequence with other events. > > > > Make this mechanism more explicit by adding an 'active_intc' pointer > > and an explicit spapr_irq_update_active_intc() function to update it > > from the CAS state. We also add hooks on the intc backend which will > > get called when it is activated or deactivated. > > > > For now we just introduce the switch and hooks, later patches will > > actually start using them. > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > Reviewed-by: Greg Kurz <groug@kaod.org> > Btw, we will need an extra xive2 pointer for the Power10 interrupt > controller. Yeah, I figured. I think it should be easier to add more irq backends with the new structure.
diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c index 83882cfad3..249a2688ac 100644 --- a/hw/ppc/spapr_irq.c +++ b/hw/ppc/spapr_irq.c @@ -586,6 +586,7 @@ qemu_irq spapr_qirq(SpaprMachineState *spapr, int irq) int spapr_irq_post_load(SpaprMachineState *spapr, int version_id) { + spapr_irq_update_active_intc(spapr); return spapr->irq->post_load(spapr, version_id); } @@ -593,6 +594,8 @@ void spapr_irq_reset(SpaprMachineState *spapr, Error **errp) { assert(!spapr->irq_map || bitmap_empty(spapr->irq_map, spapr->irq_map_nr)); + spapr_irq_update_active_intc(spapr); + if (spapr->irq->reset) { spapr->irq->reset(spapr, errp); } @@ -619,6 +622,54 @@ int spapr_irq_get_phandle(SpaprMachineState *spapr, void *fdt, Error **errp) return phandle; } +static void set_active_intc(SpaprMachineState *spapr, + SpaprInterruptController *new_intc) +{ + SpaprInterruptControllerClass *sicc; + + assert(new_intc); + + if (new_intc == spapr->active_intc) { + /* Nothing to do */ + return; + } + + if (spapr->active_intc) { + sicc = SPAPR_INTC_GET_CLASS(spapr->active_intc); + if (sicc->deactivate) { + sicc->deactivate(spapr->active_intc); + } + } + + sicc = SPAPR_INTC_GET_CLASS(new_intc); + if (sicc->activate) { + sicc->activate(new_intc, &error_fatal); + } + + spapr->active_intc = new_intc; +} + +void spapr_irq_update_active_intc(SpaprMachineState *spapr) +{ + SpaprInterruptController *new_intc; + + if (!spapr->ics) { + /* + * XXX before we run CAS, ov5_cas is initialized empty, which + * indicates XICS, even if we have ic-mode=xive. TODO: clean + * up the CAS path so that we have a clearer way of handling + * this. + */ + new_intc = SPAPR_INTC(spapr->xive); + } else if (spapr_ovec_test(spapr->ov5_cas, OV5_XIVE_EXPLOIT)) { + new_intc = SPAPR_INTC(spapr->xive); + } else { + new_intc = SPAPR_INTC(spapr->ics); + } + + set_active_intc(spapr, new_intc); +} + /* * XICS legacy routines - to deprecate one day */ diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h index cbd1a4c9f3..763da757f0 100644 --- a/include/hw/ppc/spapr.h +++ b/include/hw/ppc/spapr.h @@ -143,7 +143,6 @@ struct SpaprMachineState { struct SpaprVioBus *vio_bus; QLIST_HEAD(, SpaprPhbState) phbs; struct SpaprNvram *nvram; - ICSState *ics; SpaprRtcState rtc; SpaprResizeHpt resize_hpt; @@ -195,9 +194,11 @@ struct SpaprMachineState { int32_t irq_map_nr; unsigned long *irq_map; - SpaprXive *xive; SpaprIrq *irq; qemu_irq *qirqs; + SpaprInterruptController *active_intc; + ICSState *ics; + SpaprXive *xive; bool cmd_line_caps[SPAPR_CAP_NUM]; SpaprCapabilities def, eff, mig; diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h index adfef0fcbe..593059eff5 100644 --- a/include/hw/ppc/spapr_irq.h +++ b/include/hw/ppc/spapr_irq.h @@ -44,6 +44,9 @@ typedef struct SpaprInterruptController SpaprInterruptController; typedef struct SpaprInterruptControllerClass { InterfaceClass parent; + int (*activate)(SpaprInterruptController *intc, Error **errp); + void (*deactivate)(SpaprInterruptController *intc); + /* * These methods will typically be called on all intcs, active and * inactive @@ -55,6 +58,8 @@ typedef struct SpaprInterruptControllerClass { void (*free_irq)(SpaprInterruptController *intc, int irq); } SpaprInterruptControllerClass; +void spapr_irq_update_active_intc(SpaprMachineState *spapr); + int spapr_irq_cpu_intc_create(SpaprMachineState *spapr, PowerPCCPU *cpu, Error **errp);