| Message ID | 20190927055028.11493-24-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
| Series | spapr: IRQ subsystem cleanup | expand |
On Fri, 27 Sep 2019 15:50:18 +1000 David Gibson <david@gibson.dropbear.id.au> 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> > 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 ea44378d8c..dfa875b7cd 100644 > --- a/hw/ppc/spapr_irq.c > +++ b/hw/ppc/spapr_irq.c > @@ -593,6 +593,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); > } > > @@ -600,6 +601,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); > } > @@ -626,6 +629,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 616086f9bb..3102d152b2 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; > > + void (*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); > + > void spapr_irq_print_info(SpaprMachineState *spapr, Monitor *mon); > void spapr_irq_dt(SpaprMachineState *spapr, uint32_t nr_servers, > void *fdt, uint32_t phandle);
On 27/09/2019 07:50, 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. I don't see the exact benefit of extra pointers and routines as we can calculate the exact same state from other pointers. > For now we just introduce the switch and hooks, later patches will > actually start using them. OK. > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > 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 ea44378d8c..dfa875b7cd 100644 > --- a/hw/ppc/spapr_irq.c > +++ b/hw/ppc/spapr_irq.c > @@ -593,6 +593,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); > } > > @@ -600,6 +601,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); > } > @@ -626,6 +629,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 616086f9bb..3102d152b2 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; > > + void (*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); > + > void spapr_irq_print_info(SpaprMachineState *spapr, Monitor *mon); > void spapr_irq_dt(SpaprMachineState *spapr, uint32_t nr_servers, > void *fdt, uint32_t phandle); >
diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c index ea44378d8c..dfa875b7cd 100644 --- a/hw/ppc/spapr_irq.c +++ b/hw/ppc/spapr_irq.c @@ -593,6 +593,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); } @@ -600,6 +601,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); } @@ -626,6 +629,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 616086f9bb..3102d152b2 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; + void (*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); + void spapr_irq_print_info(SpaprMachineState *spapr, Monitor *mon); void spapr_irq_dt(SpaprMachineState *spapr, uint32_t nr_servers, void *fdt, uint32_t phandle);
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> --- 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(-)