| Message ID | 20190927055028.11493-25-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
| Series | spapr: IRQ subsystem cleanup | expand |
On Fri, 27 Sep 2019 15:50:19 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > This method depends only on the active irq controller. Now that we've > formalized the notion of active controller we can dispatch directly through > that, rather than dispatching via SpaprIrq with the dual version having > to do a second conditional dispatch. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/intc/spapr_xive.c | 12 +++++++++++ > hw/intc/xics_spapr.c | 9 +++++++++ > hw/ppc/spapr_irq.c | 41 ++++++++++---------------------------- > include/hw/ppc/spapr_irq.h | 4 +++- > 4 files changed, 34 insertions(+), 32 deletions(-) > > diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c > index ff1a175b44..52d5e71793 100644 > --- a/hw/intc/spapr_xive.c > +++ b/hw/intc/spapr_xive.c > @@ -553,6 +553,17 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc, > return 0; > } > > +static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val) > +{ > + SpaprXive *xive = SPAPR_XIVE(intc); > + > + if (kvm_irqchip_in_kernel()) { > + kvmppc_xive_source_set_irq(&xive->source, irq, val); > + } else { > + xive_source_set_irq(&xive->source, irq, val); > + } > +} > + > static void spapr_xive_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > @@ -574,6 +585,7 @@ static void spapr_xive_class_init(ObjectClass *klass, void *data) > sicc->cpu_intc_create = spapr_xive_cpu_intc_create; > sicc->claim_irq = spapr_xive_claim_irq; > sicc->free_irq = spapr_xive_free_irq; > + sicc->set_irq = spapr_xive_set_irq; > } > > static const TypeInfo spapr_xive_info = { > diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c > index 224fe1efcd..02372697f6 100644 > --- a/hw/intc/xics_spapr.c > +++ b/hw/intc/xics_spapr.c > @@ -373,6 +373,14 @@ static void xics_spapr_free_irq(SpaprInterruptController *intc, int irq) > memset(&ics->irqs[srcno], 0, sizeof(ICSIRQState)); > } > > +static void xics_spapr_set_irq(SpaprInterruptController *intc, int irq, int val) > +{ > + ICSState *ics = ICS_SPAPR(intc); > + uint32_t srcno = irq - ics->offset; > + > + ics_set_irq(ics, srcno, val); And we have: void ics_set_irq(void *opaque, int srcno, int val) { ICSState *ics = (ICSState *)opaque; if (kvm_irqchip_in_kernel()) { ics_kvm_set_irq(ics, srcno, val); return; } if (ics->irqs[srcno].flags & XICS_FLAGS_IRQ_LSI) { ics_set_irq_lsi(ics, srcno, val); } else { ics_set_irq_msi(ics, srcno, val); } } The kvm_irqchip_in_kernel() block would fit better in xics_spapr_set_irq(), like it is already the case for XIVE. Maybe do it now while here ? Anyway, Reviewed-by: Greg Kurz <groug@kaod.org> > +} > + > static void ics_spapr_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > @@ -384,6 +392,7 @@ static void ics_spapr_class_init(ObjectClass *klass, void *data) > sicc->cpu_intc_create = xics_spapr_cpu_intc_create; > sicc->claim_irq = xics_spapr_claim_irq; > sicc->free_irq = xics_spapr_free_irq; > + sicc->set_irq = xics_spapr_set_irq; > } > > static const TypeInfo ics_spapr_info = { > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c > index dfa875b7cd..4922062908 100644 > --- a/hw/ppc/spapr_irq.c > +++ b/hw/ppc/spapr_irq.c > @@ -123,14 +123,6 @@ static int spapr_irq_post_load_xics(SpaprMachineState *spapr, int version_id) > return 0; > } > > -static void spapr_irq_set_irq_xics(void *opaque, int irq, int val) > -{ > - SpaprMachineState *spapr = opaque; > - uint32_t srcno = irq - spapr->ics->offset; > - > - ics_set_irq(spapr->ics, srcno, val); > -} > - > static void spapr_irq_reset_xics(SpaprMachineState *spapr, Error **errp) > { > Error *local_err = NULL; > @@ -159,7 +151,6 @@ SpaprIrq spapr_irq_xics = { > .dt_populate = spapr_dt_xics, > .post_load = spapr_irq_post_load_xics, > .reset = spapr_irq_reset_xics, > - .set_irq = spapr_irq_set_irq_xics, > .init_kvm = spapr_irq_init_kvm_xics, > }; > > @@ -208,17 +199,6 @@ static void spapr_irq_reset_xive(SpaprMachineState *spapr, Error **errp) > spapr_xive_mmio_set_enabled(spapr->xive, true); > } > > -static void spapr_irq_set_irq_xive(void *opaque, int irq, int val) > -{ > - SpaprMachineState *spapr = opaque; > - > - if (kvm_irqchip_in_kernel()) { > - kvmppc_xive_source_set_irq(&spapr->xive->source, irq, val); > - } else { > - xive_source_set_irq(&spapr->xive->source, irq, val); > - } > -} > - > static void spapr_irq_init_kvm_xive(SpaprMachineState *spapr, Error **errp) > { > if (kvm_enabled()) { > @@ -236,7 +216,6 @@ SpaprIrq spapr_irq_xive = { > .dt_populate = spapr_dt_xive, > .post_load = spapr_irq_post_load_xive, > .reset = spapr_irq_reset_xive, > - .set_irq = spapr_irq_set_irq_xive, > .init_kvm = spapr_irq_init_kvm_xive, > }; > > @@ -316,13 +295,6 @@ static void spapr_irq_reset_dual(SpaprMachineState *spapr, Error **errp) > spapr_irq_current(spapr)->reset(spapr, errp); > } > > -static void spapr_irq_set_irq_dual(void *opaque, int irq, int val) > -{ > - SpaprMachineState *spapr = opaque; > - > - spapr_irq_current(spapr)->set_irq(spapr, irq, val); > -} > - > /* > * Define values in sync with the XIVE and XICS backend > */ > @@ -336,7 +308,6 @@ SpaprIrq spapr_irq_dual = { > .dt_populate = spapr_irq_dt_populate_dual, > .post_load = spapr_irq_post_load_dual, > .reset = spapr_irq_reset_dual, > - .set_irq = spapr_irq_set_irq_dual, > .init_kvm = NULL, /* should not be used */ > }; > > @@ -422,6 +393,15 @@ int spapr_irq_cpu_intc_create(SpaprMachineState *spapr, > return 0; > } > > +static void spapr_set_irq(void *opaque, int irq, int level) > +{ > + SpaprMachineState *spapr = SPAPR_MACHINE(opaque); > + SpaprInterruptControllerClass *sicc > + = SPAPR_INTC_GET_CLASS(spapr->active_intc); > + > + sicc->set_irq(spapr->active_intc, irq, level); > +} > + > void spapr_irq_init(SpaprMachineState *spapr, Error **errp) > { > MachineState *machine = MACHINE(spapr); > @@ -510,7 +490,7 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp) > spapr_xive_hcall_init(spapr); > } > > - spapr->qirqs = qemu_allocate_irqs(spapr->irq->set_irq, spapr, > + spapr->qirqs = qemu_allocate_irqs(spapr_set_irq, spapr, > spapr->irq->nr_xirqs + SPAPR_XIRQ_BASE); > > out: > @@ -744,7 +724,6 @@ SpaprIrq spapr_irq_xics_legacy = { > .dt_populate = spapr_dt_xics, > .post_load = spapr_irq_post_load_xics, > .reset = spapr_irq_reset_xics, > - .set_irq = spapr_irq_set_irq_xics, > .init_kvm = spapr_irq_init_kvm_xics, > }; > > diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h > index 3102d152b2..8286a9aa63 100644 > --- a/include/hw/ppc/spapr_irq.h > +++ b/include/hw/ppc/spapr_irq.h > @@ -56,6 +56,9 @@ typedef struct SpaprInterruptControllerClass { > int (*claim_irq)(SpaprInterruptController *intc, int irq, bool lsi, > Error **errp); > void (*free_irq)(SpaprInterruptController *intc, int irq); > + > + /* These methods should only be called on the active intc */ > + void (*set_irq)(SpaprInterruptController *intc, int irq, int val); > } SpaprInterruptControllerClass; > > void spapr_irq_update_active_intc(SpaprMachineState *spapr); > @@ -83,7 +86,6 @@ typedef struct SpaprIrq { > void *fdt, uint32_t phandle); > int (*post_load)(SpaprMachineState *spapr, int version_id); > void (*reset)(SpaprMachineState *spapr, Error **errp); > - void (*set_irq)(void *opaque, int srcno, int val); > void (*init_kvm)(SpaprMachineState *spapr, Error **errp); > } SpaprIrq; >
On Fri, Sep 27, 2019 at 04:27:12PM +0200, Greg Kurz wrote: > On Fri, 27 Sep 2019 15:50:19 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > This method depends only on the active irq controller. Now that we've > > formalized the notion of active controller we can dispatch directly through > > that, rather than dispatching via SpaprIrq with the dual version having > > to do a second conditional dispatch. > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > --- > > hw/intc/spapr_xive.c | 12 +++++++++++ > > hw/intc/xics_spapr.c | 9 +++++++++ > > hw/ppc/spapr_irq.c | 41 ++++++++++---------------------------- > > include/hw/ppc/spapr_irq.h | 4 +++- > > 4 files changed, 34 insertions(+), 32 deletions(-) > > > > diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c > > index ff1a175b44..52d5e71793 100644 > > --- a/hw/intc/spapr_xive.c > > +++ b/hw/intc/spapr_xive.c > > @@ -553,6 +553,17 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc, > > return 0; > > } > > > > +static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val) > > +{ > > + SpaprXive *xive = SPAPR_XIVE(intc); > > + > > + if (kvm_irqchip_in_kernel()) { > > + kvmppc_xive_source_set_irq(&xive->source, irq, val); > > + } else { > > + xive_source_set_irq(&xive->source, irq, val); > > + } > > +} > > + > > static void spapr_xive_class_init(ObjectClass *klass, void *data) > > { > > DeviceClass *dc = DEVICE_CLASS(klass); > > @@ -574,6 +585,7 @@ static void spapr_xive_class_init(ObjectClass *klass, void *data) > > sicc->cpu_intc_create = spapr_xive_cpu_intc_create; > > sicc->claim_irq = spapr_xive_claim_irq; > > sicc->free_irq = spapr_xive_free_irq; > > + sicc->set_irq = spapr_xive_set_irq; > > } > > > > static const TypeInfo spapr_xive_info = { > > diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c > > index 224fe1efcd..02372697f6 100644 > > --- a/hw/intc/xics_spapr.c > > +++ b/hw/intc/xics_spapr.c > > @@ -373,6 +373,14 @@ static void xics_spapr_free_irq(SpaprInterruptController *intc, int irq) > > memset(&ics->irqs[srcno], 0, sizeof(ICSIRQState)); > > } > > > > +static void xics_spapr_set_irq(SpaprInterruptController *intc, int irq, int val) > > +{ > > + ICSState *ics = ICS_SPAPR(intc); > > + uint32_t srcno = irq - ics->offset; > > + > > + ics_set_irq(ics, srcno, val); > > And we have: > > void ics_set_irq(void *opaque, int srcno, int val) > { > ICSState *ics = (ICSState *)opaque; > > if (kvm_irqchip_in_kernel()) { > ics_kvm_set_irq(ics, srcno, val); > return; > } > > if (ics->irqs[srcno].flags & XICS_FLAGS_IRQ_LSI) { > ics_set_irq_lsi(ics, srcno, val); > } else { > ics_set_irq_msi(ics, srcno, val); > } > } > > The kvm_irqchip_in_kernel() block would fit better in xics_spapr_set_irq(), > like it is already the case for XIVE. Hmm.. I don't really see why you say that. > Maybe do it now while here ? > > Anyway, > > Reviewed-by: Greg Kurz <groug@kaod.org> > > > +} > > + > > static void ics_spapr_class_init(ObjectClass *klass, void *data) > > { > > DeviceClass *dc = DEVICE_CLASS(klass); > > @@ -384,6 +392,7 @@ static void ics_spapr_class_init(ObjectClass *klass, void *data) > > sicc->cpu_intc_create = xics_spapr_cpu_intc_create; > > sicc->claim_irq = xics_spapr_claim_irq; > > sicc->free_irq = xics_spapr_free_irq; > > + sicc->set_irq = xics_spapr_set_irq; > > } > > > > static const TypeInfo ics_spapr_info = { > > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c > > index dfa875b7cd..4922062908 100644 > > --- a/hw/ppc/spapr_irq.c > > +++ b/hw/ppc/spapr_irq.c > > @@ -123,14 +123,6 @@ static int spapr_irq_post_load_xics(SpaprMachineState *spapr, int version_id) > > return 0; > > } > > > > -static void spapr_irq_set_irq_xics(void *opaque, int irq, int val) > > -{ > > - SpaprMachineState *spapr = opaque; > > - uint32_t srcno = irq - spapr->ics->offset; > > - > > - ics_set_irq(spapr->ics, srcno, val); > > -} > > - > > static void spapr_irq_reset_xics(SpaprMachineState *spapr, Error **errp) > > { > > Error *local_err = NULL; > > @@ -159,7 +151,6 @@ SpaprIrq spapr_irq_xics = { > > .dt_populate = spapr_dt_xics, > > .post_load = spapr_irq_post_load_xics, > > .reset = spapr_irq_reset_xics, > > - .set_irq = spapr_irq_set_irq_xics, > > .init_kvm = spapr_irq_init_kvm_xics, > > }; > > > > @@ -208,17 +199,6 @@ static void spapr_irq_reset_xive(SpaprMachineState *spapr, Error **errp) > > spapr_xive_mmio_set_enabled(spapr->xive, true); > > } > > > > -static void spapr_irq_set_irq_xive(void *opaque, int irq, int val) > > -{ > > - SpaprMachineState *spapr = opaque; > > - > > - if (kvm_irqchip_in_kernel()) { > > - kvmppc_xive_source_set_irq(&spapr->xive->source, irq, val); > > - } else { > > - xive_source_set_irq(&spapr->xive->source, irq, val); > > - } > > -} > > - > > static void spapr_irq_init_kvm_xive(SpaprMachineState *spapr, Error **errp) > > { > > if (kvm_enabled()) { > > @@ -236,7 +216,6 @@ SpaprIrq spapr_irq_xive = { > > .dt_populate = spapr_dt_xive, > > .post_load = spapr_irq_post_load_xive, > > .reset = spapr_irq_reset_xive, > > - .set_irq = spapr_irq_set_irq_xive, > > .init_kvm = spapr_irq_init_kvm_xive, > > }; > > > > @@ -316,13 +295,6 @@ static void spapr_irq_reset_dual(SpaprMachineState *spapr, Error **errp) > > spapr_irq_current(spapr)->reset(spapr, errp); > > } > > > > -static void spapr_irq_set_irq_dual(void *opaque, int irq, int val) > > -{ > > - SpaprMachineState *spapr = opaque; > > - > > - spapr_irq_current(spapr)->set_irq(spapr, irq, val); > > -} > > - > > /* > > * Define values in sync with the XIVE and XICS backend > > */ > > @@ -336,7 +308,6 @@ SpaprIrq spapr_irq_dual = { > > .dt_populate = spapr_irq_dt_populate_dual, > > .post_load = spapr_irq_post_load_dual, > > .reset = spapr_irq_reset_dual, > > - .set_irq = spapr_irq_set_irq_dual, > > .init_kvm = NULL, /* should not be used */ > > }; > > > > @@ -422,6 +393,15 @@ int spapr_irq_cpu_intc_create(SpaprMachineState *spapr, > > return 0; > > } > > > > +static void spapr_set_irq(void *opaque, int irq, int level) > > +{ > > + SpaprMachineState *spapr = SPAPR_MACHINE(opaque); > > + SpaprInterruptControllerClass *sicc > > + = SPAPR_INTC_GET_CLASS(spapr->active_intc); > > + > > + sicc->set_irq(spapr->active_intc, irq, level); > > +} > > + > > void spapr_irq_init(SpaprMachineState *spapr, Error **errp) > > { > > MachineState *machine = MACHINE(spapr); > > @@ -510,7 +490,7 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp) > > spapr_xive_hcall_init(spapr); > > } > > > > - spapr->qirqs = qemu_allocate_irqs(spapr->irq->set_irq, spapr, > > + spapr->qirqs = qemu_allocate_irqs(spapr_set_irq, spapr, > > spapr->irq->nr_xirqs + SPAPR_XIRQ_BASE); > > > > out: > > @@ -744,7 +724,6 @@ SpaprIrq spapr_irq_xics_legacy = { > > .dt_populate = spapr_dt_xics, > > .post_load = spapr_irq_post_load_xics, > > .reset = spapr_irq_reset_xics, > > - .set_irq = spapr_irq_set_irq_xics, > > .init_kvm = spapr_irq_init_kvm_xics, > > }; > > > > diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h > > index 3102d152b2..8286a9aa63 100644 > > --- a/include/hw/ppc/spapr_irq.h > > +++ b/include/hw/ppc/spapr_irq.h > > @@ -56,6 +56,9 @@ typedef struct SpaprInterruptControllerClass { > > int (*claim_irq)(SpaprInterruptController *intc, int irq, bool lsi, > > Error **errp); > > void (*free_irq)(SpaprInterruptController *intc, int irq); > > + > > + /* These methods should only be called on the active intc */ > > + void (*set_irq)(SpaprInterruptController *intc, int irq, int val); > > } SpaprInterruptControllerClass; > > > > void spapr_irq_update_active_intc(SpaprMachineState *spapr); > > @@ -83,7 +86,6 @@ typedef struct SpaprIrq { > > void *fdt, uint32_t phandle); > > int (*post_load)(SpaprMachineState *spapr, int version_id); > > void (*reset)(SpaprMachineState *spapr, Error **errp); > > - void (*set_irq)(void *opaque, int srcno, int val); > > void (*init_kvm)(SpaprMachineState *spapr, Error **errp); > > } SpaprIrq; > > >
On 27/09/2019 07:50, David Gibson wrote: > This method depends only on the active irq controller. Now that we've > formalized the notion of active controller we can dispatch directly through > that, rather than dispatching via SpaprIrq with the dual version having > to do a second conditional dispatch. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/intc/spapr_xive.c | 12 +++++++++++ > hw/intc/xics_spapr.c | 9 +++++++++ > hw/ppc/spapr_irq.c | 41 ++++++++++---------------------------- > include/hw/ppc/spapr_irq.h | 4 +++- > 4 files changed, 34 insertions(+), 32 deletions(-) > > diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c > index ff1a175b44..52d5e71793 100644 > --- a/hw/intc/spapr_xive.c > +++ b/hw/intc/spapr_xive.c > @@ -553,6 +553,17 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc, > return 0; > } > > +static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val) > +{ > + SpaprXive *xive = SPAPR_XIVE(intc); > + > + if (kvm_irqchip_in_kernel()) { > + kvmppc_xive_source_set_irq(&xive->source, irq, val); > + } else { > + xive_source_set_irq(&xive->source, irq, val); > + } > +} > static void spapr_xive_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > @@ -574,6 +585,7 @@ static void spapr_xive_class_init(ObjectClass *klass, void *data) > sicc->cpu_intc_create = spapr_xive_cpu_intc_create; > sicc->claim_irq = spapr_xive_claim_irq; > sicc->free_irq = spapr_xive_free_irq; > + sicc->set_irq = spapr_xive_set_irq; > } > > static const TypeInfo spapr_xive_info = { > diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c > index 224fe1efcd..02372697f6 100644 > --- a/hw/intc/xics_spapr.c > +++ b/hw/intc/xics_spapr.c > @@ -373,6 +373,14 @@ static void xics_spapr_free_irq(SpaprInterruptController *intc, int irq) > memset(&ics->irqs[srcno], 0, sizeof(ICSIRQState)); > } > > +static void xics_spapr_set_irq(SpaprInterruptController *intc, int irq, int val) > +{ > + ICSState *ics = ICS_SPAPR(intc); > + uint32_t srcno = irq - ics->offset; > + > + ics_set_irq(ics, srcno, val); > +} > + > static void ics_spapr_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > @@ -384,6 +392,7 @@ static void ics_spapr_class_init(ObjectClass *klass, void *data) > sicc->cpu_intc_create = xics_spapr_cpu_intc_create; > sicc->claim_irq = xics_spapr_claim_irq; > sicc->free_irq = xics_spapr_free_irq; > + sicc->set_irq = xics_spapr_set_irq; > } > > static const TypeInfo ics_spapr_info = { > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c > index dfa875b7cd..4922062908 100644 > --- a/hw/ppc/spapr_irq.c > +++ b/hw/ppc/spapr_irq.c > @@ -123,14 +123,6 @@ static int spapr_irq_post_load_xics(SpaprMachineState *spapr, int version_id) > return 0; > } > > -static void spapr_irq_set_irq_xics(void *opaque, int irq, int val) > -{ > - SpaprMachineState *spapr = opaque; > - uint32_t srcno = irq - spapr->ics->offset; > - > - ics_set_irq(spapr->ics, srcno, val); > -} > - > static void spapr_irq_reset_xics(SpaprMachineState *spapr, Error **errp) > { > Error *local_err = NULL; > @@ -159,7 +151,6 @@ SpaprIrq spapr_irq_xics = { > .dt_populate = spapr_dt_xics, > .post_load = spapr_irq_post_load_xics, > .reset = spapr_irq_reset_xics, > - .set_irq = spapr_irq_set_irq_xics, > .init_kvm = spapr_irq_init_kvm_xics, > }; > > @@ -208,17 +199,6 @@ static void spapr_irq_reset_xive(SpaprMachineState *spapr, Error **errp) > spapr_xive_mmio_set_enabled(spapr->xive, true); > } > > -static void spapr_irq_set_irq_xive(void *opaque, int irq, int val) > -{ > - SpaprMachineState *spapr = opaque; > - > - if (kvm_irqchip_in_kernel()) { > - kvmppc_xive_source_set_irq(&spapr->xive->source, irq, val); > - } else { > - xive_source_set_irq(&spapr->xive->source, irq, val); > - } > -} > - > static void spapr_irq_init_kvm_xive(SpaprMachineState *spapr, Error **errp) > { > if (kvm_enabled()) { > @@ -236,7 +216,6 @@ SpaprIrq spapr_irq_xive = { > .dt_populate = spapr_dt_xive, > .post_load = spapr_irq_post_load_xive, > .reset = spapr_irq_reset_xive, > - .set_irq = spapr_irq_set_irq_xive, > .init_kvm = spapr_irq_init_kvm_xive, > }; > > @@ -316,13 +295,6 @@ static void spapr_irq_reset_dual(SpaprMachineState *spapr, Error **errp) > spapr_irq_current(spapr)->reset(spapr, errp); > } > > -static void spapr_irq_set_irq_dual(void *opaque, int irq, int val) > -{ > - SpaprMachineState *spapr = opaque; > - > - spapr_irq_current(spapr)->set_irq(spapr, irq, val); > -} > - > /* > * Define values in sync with the XIVE and XICS backend > */ > @@ -336,7 +308,6 @@ SpaprIrq spapr_irq_dual = { > .dt_populate = spapr_irq_dt_populate_dual, > .post_load = spapr_irq_post_load_dual, > .reset = spapr_irq_reset_dual, > - .set_irq = spapr_irq_set_irq_dual, > .init_kvm = NULL, /* should not be used */ > }; Above changes are all fine. I am still dubious for the 'active_intc'. C. > @@ -422,6 +393,15 @@ int spapr_irq_cpu_intc_create(SpaprMachineState *spapr, > return 0; > } > > +static void spapr_set_irq(void *opaque, int irq, int level) > +{ > + SpaprMachineState *spapr = SPAPR_MACHINE(opaque); > + SpaprInterruptControllerClass *sicc > + = SPAPR_INTC_GET_CLASS(spapr->active_intc); > + > + sicc->set_irq(spapr->active_intc, irq, level); > +} > + > void spapr_irq_init(SpaprMachineState *spapr, Error **errp) > { > MachineState *machine = MACHINE(spapr); > @@ -510,7 +490,7 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp) > spapr_xive_hcall_init(spapr); > } > > - spapr->qirqs = qemu_allocate_irqs(spapr->irq->set_irq, spapr, > + spapr->qirqs = qemu_allocate_irqs(spapr_set_irq, spapr, > spapr->irq->nr_xirqs + SPAPR_XIRQ_BASE); > > out: > @@ -744,7 +724,6 @@ SpaprIrq spapr_irq_xics_legacy = { > .dt_populate = spapr_dt_xics, > .post_load = spapr_irq_post_load_xics, > .reset = spapr_irq_reset_xics, > - .set_irq = spapr_irq_set_irq_xics, > .init_kvm = spapr_irq_init_kvm_xics, > }; > > diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h > index 3102d152b2..8286a9aa63 100644 > --- a/include/hw/ppc/spapr_irq.h > +++ b/include/hw/ppc/spapr_irq.h > @@ -56,6 +56,9 @@ typedef struct SpaprInterruptControllerClass { > int (*claim_irq)(SpaprInterruptController *intc, int irq, bool lsi, > Error **errp); > void (*free_irq)(SpaprInterruptController *intc, int irq); > + > + /* These methods should only be called on the active intc */ > + void (*set_irq)(SpaprInterruptController *intc, int irq, int val); > } SpaprInterruptControllerClass; > > void spapr_irq_update_active_intc(SpaprMachineState *spapr); > @@ -83,7 +86,6 @@ typedef struct SpaprIrq { > void *fdt, uint32_t phandle); > int (*post_load)(SpaprMachineState *spapr, int version_id); > void (*reset)(SpaprMachineState *spapr, Error **errp); > - void (*set_irq)(void *opaque, int srcno, int val); > void (*init_kvm)(SpaprMachineState *spapr, Error **errp); > } SpaprIrq; > >
On Mon, 30 Sep 2019 12:41:39 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Fri, Sep 27, 2019 at 04:27:12PM +0200, Greg Kurz wrote: > > On Fri, 27 Sep 2019 15:50:19 +1000 > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > This method depends only on the active irq controller. Now that we've > > > formalized the notion of active controller we can dispatch directly through > > > that, rather than dispatching via SpaprIrq with the dual version having > > > to do a second conditional dispatch. > > > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > > --- > > > hw/intc/spapr_xive.c | 12 +++++++++++ > > > hw/intc/xics_spapr.c | 9 +++++++++ > > > hw/ppc/spapr_irq.c | 41 ++++++++++---------------------------- > > > include/hw/ppc/spapr_irq.h | 4 +++- > > > 4 files changed, 34 insertions(+), 32 deletions(-) > > > > > > diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c > > > index ff1a175b44..52d5e71793 100644 > > > --- a/hw/intc/spapr_xive.c > > > +++ b/hw/intc/spapr_xive.c > > > @@ -553,6 +553,17 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc, > > > return 0; > > > } > > > > > > +static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val) > > > +{ > > > + SpaprXive *xive = SPAPR_XIVE(intc); > > > + > > > + if (kvm_irqchip_in_kernel()) { > > > + kvmppc_xive_source_set_irq(&xive->source, irq, val); > > > + } else { > > > + xive_source_set_irq(&xive->source, irq, val); > > > + } > > > +} > > > + > > > static void spapr_xive_class_init(ObjectClass *klass, void *data) > > > { > > > DeviceClass *dc = DEVICE_CLASS(klass); > > > @@ -574,6 +585,7 @@ static void spapr_xive_class_init(ObjectClass *klass, void *data) > > > sicc->cpu_intc_create = spapr_xive_cpu_intc_create; > > > sicc->claim_irq = spapr_xive_claim_irq; > > > sicc->free_irq = spapr_xive_free_irq; > > > + sicc->set_irq = spapr_xive_set_irq; > > > } > > > > > > static const TypeInfo spapr_xive_info = { > > > diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c > > > index 224fe1efcd..02372697f6 100644 > > > --- a/hw/intc/xics_spapr.c > > > +++ b/hw/intc/xics_spapr.c > > > @@ -373,6 +373,14 @@ static void xics_spapr_free_irq(SpaprInterruptController *intc, int irq) > > > memset(&ics->irqs[srcno], 0, sizeof(ICSIRQState)); > > > } > > > > > > +static void xics_spapr_set_irq(SpaprInterruptController *intc, int irq, int val) > > > +{ > > > + ICSState *ics = ICS_SPAPR(intc); > > > + uint32_t srcno = irq - ics->offset; > > > + > > > + ics_set_irq(ics, srcno, val); > > > > And we have: > > > > void ics_set_irq(void *opaque, int srcno, int val) > > { > > ICSState *ics = (ICSState *)opaque; > > > > if (kvm_irqchip_in_kernel()) { > > ics_kvm_set_irq(ics, srcno, val); > > return; > > } > > > > if (ics->irqs[srcno].flags & XICS_FLAGS_IRQ_LSI) { > > ics_set_irq_lsi(ics, srcno, val); > > } else { > > ics_set_irq_msi(ics, srcno, val); > > } > > } > > > > The kvm_irqchip_in_kernel() block would fit better in xics_spapr_set_irq(), > > like it is already the case for XIVE. > > Hmm.. I don't really see why you say that. > I mean this: static void xics_spapr_set_irq(SpaprInterruptController *intc, int irq, int val) { IcsSpapr *icss = ICS_SPAPR(intc); ICSState *ics = &icss->parent; uint32_t srcno = irq - ics->offset; if (kvm_irqchip_in_kernel()) { ics_kvm_set_irq(ics, srcno, val); } else { ics_set_irq(ics, srcno, val); } } It is very similar to spapr_xive_set_irq() and looks nicer to me. > > Maybe do it now while here ? > > > > Anyway, > > > > Reviewed-by: Greg Kurz <groug@kaod.org> > > > > > +} > > > + > > > static void ics_spapr_class_init(ObjectClass *klass, void *data) > > > { > > > DeviceClass *dc = DEVICE_CLASS(klass); > > > @@ -384,6 +392,7 @@ static void ics_spapr_class_init(ObjectClass *klass, void *data) > > > sicc->cpu_intc_create = xics_spapr_cpu_intc_create; > > > sicc->claim_irq = xics_spapr_claim_irq; > > > sicc->free_irq = xics_spapr_free_irq; > > > + sicc->set_irq = xics_spapr_set_irq; > > > } > > > > > > static const TypeInfo ics_spapr_info = { > > > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c > > > index dfa875b7cd..4922062908 100644 > > > --- a/hw/ppc/spapr_irq.c > > > +++ b/hw/ppc/spapr_irq.c > > > @@ -123,14 +123,6 @@ static int spapr_irq_post_load_xics(SpaprMachineState *spapr, int version_id) > > > return 0; > > > } > > > > > > -static void spapr_irq_set_irq_xics(void *opaque, int irq, int val) > > > -{ > > > - SpaprMachineState *spapr = opaque; > > > - uint32_t srcno = irq - spapr->ics->offset; > > > - > > > - ics_set_irq(spapr->ics, srcno, val); > > > -} > > > - > > > static void spapr_irq_reset_xics(SpaprMachineState *spapr, Error **errp) > > > { > > > Error *local_err = NULL; > > > @@ -159,7 +151,6 @@ SpaprIrq spapr_irq_xics = { > > > .dt_populate = spapr_dt_xics, > > > .post_load = spapr_irq_post_load_xics, > > > .reset = spapr_irq_reset_xics, > > > - .set_irq = spapr_irq_set_irq_xics, > > > .init_kvm = spapr_irq_init_kvm_xics, > > > }; > > > > > > @@ -208,17 +199,6 @@ static void spapr_irq_reset_xive(SpaprMachineState *spapr, Error **errp) > > > spapr_xive_mmio_set_enabled(spapr->xive, true); > > > } > > > > > > -static void spapr_irq_set_irq_xive(void *opaque, int irq, int val) > > > -{ > > > - SpaprMachineState *spapr = opaque; > > > - > > > - if (kvm_irqchip_in_kernel()) { > > > - kvmppc_xive_source_set_irq(&spapr->xive->source, irq, val); > > > - } else { > > > - xive_source_set_irq(&spapr->xive->source, irq, val); > > > - } > > > -} > > > - > > > static void spapr_irq_init_kvm_xive(SpaprMachineState *spapr, Error **errp) > > > { > > > if (kvm_enabled()) { > > > @@ -236,7 +216,6 @@ SpaprIrq spapr_irq_xive = { > > > .dt_populate = spapr_dt_xive, > > > .post_load = spapr_irq_post_load_xive, > > > .reset = spapr_irq_reset_xive, > > > - .set_irq = spapr_irq_set_irq_xive, > > > .init_kvm = spapr_irq_init_kvm_xive, > > > }; > > > > > > @@ -316,13 +295,6 @@ static void spapr_irq_reset_dual(SpaprMachineState *spapr, Error **errp) > > > spapr_irq_current(spapr)->reset(spapr, errp); > > > } > > > > > > -static void spapr_irq_set_irq_dual(void *opaque, int irq, int val) > > > -{ > > > - SpaprMachineState *spapr = opaque; > > > - > > > - spapr_irq_current(spapr)->set_irq(spapr, irq, val); > > > -} > > > - > > > /* > > > * Define values in sync with the XIVE and XICS backend > > > */ > > > @@ -336,7 +308,6 @@ SpaprIrq spapr_irq_dual = { > > > .dt_populate = spapr_irq_dt_populate_dual, > > > .post_load = spapr_irq_post_load_dual, > > > .reset = spapr_irq_reset_dual, > > > - .set_irq = spapr_irq_set_irq_dual, > > > .init_kvm = NULL, /* should not be used */ > > > }; > > > > > > @@ -422,6 +393,15 @@ int spapr_irq_cpu_intc_create(SpaprMachineState *spapr, > > > return 0; > > > } > > > > > > +static void spapr_set_irq(void *opaque, int irq, int level) > > > +{ > > > + SpaprMachineState *spapr = SPAPR_MACHINE(opaque); > > > + SpaprInterruptControllerClass *sicc > > > + = SPAPR_INTC_GET_CLASS(spapr->active_intc); > > > + > > > + sicc->set_irq(spapr->active_intc, irq, level); > > > +} > > > + > > > void spapr_irq_init(SpaprMachineState *spapr, Error **errp) > > > { > > > MachineState *machine = MACHINE(spapr); > > > @@ -510,7 +490,7 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp) > > > spapr_xive_hcall_init(spapr); > > > } > > > > > > - spapr->qirqs = qemu_allocate_irqs(spapr->irq->set_irq, spapr, > > > + spapr->qirqs = qemu_allocate_irqs(spapr_set_irq, spapr, > > > spapr->irq->nr_xirqs + SPAPR_XIRQ_BASE); > > > > > > out: > > > @@ -744,7 +724,6 @@ SpaprIrq spapr_irq_xics_legacy = { > > > .dt_populate = spapr_dt_xics, > > > .post_load = spapr_irq_post_load_xics, > > > .reset = spapr_irq_reset_xics, > > > - .set_irq = spapr_irq_set_irq_xics, > > > .init_kvm = spapr_irq_init_kvm_xics, > > > }; > > > > > > diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h > > > index 3102d152b2..8286a9aa63 100644 > > > --- a/include/hw/ppc/spapr_irq.h > > > +++ b/include/hw/ppc/spapr_irq.h > > > @@ -56,6 +56,9 @@ typedef struct SpaprInterruptControllerClass { > > > int (*claim_irq)(SpaprInterruptController *intc, int irq, bool lsi, > > > Error **errp); > > > void (*free_irq)(SpaprInterruptController *intc, int irq); > > > + > > > + /* These methods should only be called on the active intc */ > > > + void (*set_irq)(SpaprInterruptController *intc, int irq, int val); > > > } SpaprInterruptControllerClass; > > > > > > void spapr_irq_update_active_intc(SpaprMachineState *spapr); > > > @@ -83,7 +86,6 @@ typedef struct SpaprIrq { > > > void *fdt, uint32_t phandle); > > > int (*post_load)(SpaprMachineState *spapr, int version_id); > > > void (*reset)(SpaprMachineState *spapr, Error **errp); > > > - void (*set_irq)(void *opaque, int srcno, int val); > > > void (*init_kvm)(SpaprMachineState *spapr, Error **errp); > > > } SpaprIrq; > > > > > >
On Mon, Sep 30, 2019 at 09:22:16AM +0200, Greg Kurz wrote: > On Mon, 30 Sep 2019 12:41:39 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Fri, Sep 27, 2019 at 04:27:12PM +0200, Greg Kurz wrote: > > > On Fri, 27 Sep 2019 15:50:19 +1000 > > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > > > This method depends only on the active irq controller. Now that we've > > > > formalized the notion of active controller we can dispatch directly through > > > > that, rather than dispatching via SpaprIrq with the dual version having > > > > to do a second conditional dispatch. > > > > > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > > > --- > > > > hw/intc/spapr_xive.c | 12 +++++++++++ > > > > hw/intc/xics_spapr.c | 9 +++++++++ > > > > hw/ppc/spapr_irq.c | 41 ++++++++++---------------------------- > > > > include/hw/ppc/spapr_irq.h | 4 +++- > > > > 4 files changed, 34 insertions(+), 32 deletions(-) > > > > > > > > diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c > > > > index ff1a175b44..52d5e71793 100644 > > > > --- a/hw/intc/spapr_xive.c > > > > +++ b/hw/intc/spapr_xive.c > > > > @@ -553,6 +553,17 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc, > > > > return 0; > > > > } > > > > > > > > +static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val) > > > > +{ > > > > + SpaprXive *xive = SPAPR_XIVE(intc); > > > > + > > > > + if (kvm_irqchip_in_kernel()) { > > > > + kvmppc_xive_source_set_irq(&xive->source, irq, val); > > > > + } else { > > > > + xive_source_set_irq(&xive->source, irq, val); > > > > + } > > > > +} > > > > + > > > > static void spapr_xive_class_init(ObjectClass *klass, void *data) > > > > { > > > > DeviceClass *dc = DEVICE_CLASS(klass); > > > > @@ -574,6 +585,7 @@ static void spapr_xive_class_init(ObjectClass *klass, void *data) > > > > sicc->cpu_intc_create = spapr_xive_cpu_intc_create; > > > > sicc->claim_irq = spapr_xive_claim_irq; > > > > sicc->free_irq = spapr_xive_free_irq; > > > > + sicc->set_irq = spapr_xive_set_irq; > > > > } > > > > > > > > static const TypeInfo spapr_xive_info = { > > > > diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c > > > > index 224fe1efcd..02372697f6 100644 > > > > --- a/hw/intc/xics_spapr.c > > > > +++ b/hw/intc/xics_spapr.c > > > > @@ -373,6 +373,14 @@ static void xics_spapr_free_irq(SpaprInterruptController *intc, int irq) > > > > memset(&ics->irqs[srcno], 0, sizeof(ICSIRQState)); > > > > } > > > > > > > > +static void xics_spapr_set_irq(SpaprInterruptController *intc, int irq, int val) > > > > +{ > > > > + ICSState *ics = ICS_SPAPR(intc); > > > > + uint32_t srcno = irq - ics->offset; > > > > + > > > > + ics_set_irq(ics, srcno, val); > > > > > > And we have: > > > > > > void ics_set_irq(void *opaque, int srcno, int val) > > > { > > > ICSState *ics = (ICSState *)opaque; > > > > > > if (kvm_irqchip_in_kernel()) { > > > ics_kvm_set_irq(ics, srcno, val); > > > return; > > > } > > > > > > if (ics->irqs[srcno].flags & XICS_FLAGS_IRQ_LSI) { > > > ics_set_irq_lsi(ics, srcno, val); > > > } else { > > > ics_set_irq_msi(ics, srcno, val); > > > } > > > } > > > > > > The kvm_irqchip_in_kernel() block would fit better in xics_spapr_set_irq(), > > > like it is already the case for XIVE. > > > > Hmm.. I don't really see why you say that. > > > > I mean this: > > static void xics_spapr_set_irq(SpaprInterruptController *intc, int irq, int val) > { > IcsSpapr *icss = ICS_SPAPR(intc); > ICSState *ics = &icss->parent; > uint32_t srcno = irq - ics->offset; > > if (kvm_irqchip_in_kernel()) { > ics_kvm_set_irq(ics, srcno, val); > } else { > ics_set_irq(ics, srcno, val); > } > } > > It is very similar to spapr_xive_set_irq() and looks nicer to me. Eh.. I don't really agree. Seems to me KVM is essentialy an implementation detail, so belongs in the ICS proper rather than the papr specific wrapper. It's not really obvious where it belongs, because the KVM implementation only works for the PAPR version of the thing, but the way we call it doesn't explicitly depend on any PAPR specific information.
diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c index ff1a175b44..52d5e71793 100644 --- a/hw/intc/spapr_xive.c +++ b/hw/intc/spapr_xive.c @@ -553,6 +553,17 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc, return 0; } +static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val) +{ + SpaprXive *xive = SPAPR_XIVE(intc); + + if (kvm_irqchip_in_kernel()) { + kvmppc_xive_source_set_irq(&xive->source, irq, val); + } else { + xive_source_set_irq(&xive->source, irq, val); + } +} + static void spapr_xive_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); @@ -574,6 +585,7 @@ static void spapr_xive_class_init(ObjectClass *klass, void *data) sicc->cpu_intc_create = spapr_xive_cpu_intc_create; sicc->claim_irq = spapr_xive_claim_irq; sicc->free_irq = spapr_xive_free_irq; + sicc->set_irq = spapr_xive_set_irq; } static const TypeInfo spapr_xive_info = { diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c index 224fe1efcd..02372697f6 100644 --- a/hw/intc/xics_spapr.c +++ b/hw/intc/xics_spapr.c @@ -373,6 +373,14 @@ static void xics_spapr_free_irq(SpaprInterruptController *intc, int irq) memset(&ics->irqs[srcno], 0, sizeof(ICSIRQState)); } +static void xics_spapr_set_irq(SpaprInterruptController *intc, int irq, int val) +{ + ICSState *ics = ICS_SPAPR(intc); + uint32_t srcno = irq - ics->offset; + + ics_set_irq(ics, srcno, val); +} + static void ics_spapr_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); @@ -384,6 +392,7 @@ static void ics_spapr_class_init(ObjectClass *klass, void *data) sicc->cpu_intc_create = xics_spapr_cpu_intc_create; sicc->claim_irq = xics_spapr_claim_irq; sicc->free_irq = xics_spapr_free_irq; + sicc->set_irq = xics_spapr_set_irq; } static const TypeInfo ics_spapr_info = { diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c index dfa875b7cd..4922062908 100644 --- a/hw/ppc/spapr_irq.c +++ b/hw/ppc/spapr_irq.c @@ -123,14 +123,6 @@ static int spapr_irq_post_load_xics(SpaprMachineState *spapr, int version_id) return 0; } -static void spapr_irq_set_irq_xics(void *opaque, int irq, int val) -{ - SpaprMachineState *spapr = opaque; - uint32_t srcno = irq - spapr->ics->offset; - - ics_set_irq(spapr->ics, srcno, val); -} - static void spapr_irq_reset_xics(SpaprMachineState *spapr, Error **errp) { Error *local_err = NULL; @@ -159,7 +151,6 @@ SpaprIrq spapr_irq_xics = { .dt_populate = spapr_dt_xics, .post_load = spapr_irq_post_load_xics, .reset = spapr_irq_reset_xics, - .set_irq = spapr_irq_set_irq_xics, .init_kvm = spapr_irq_init_kvm_xics, }; @@ -208,17 +199,6 @@ static void spapr_irq_reset_xive(SpaprMachineState *spapr, Error **errp) spapr_xive_mmio_set_enabled(spapr->xive, true); } -static void spapr_irq_set_irq_xive(void *opaque, int irq, int val) -{ - SpaprMachineState *spapr = opaque; - - if (kvm_irqchip_in_kernel()) { - kvmppc_xive_source_set_irq(&spapr->xive->source, irq, val); - } else { - xive_source_set_irq(&spapr->xive->source, irq, val); - } -} - static void spapr_irq_init_kvm_xive(SpaprMachineState *spapr, Error **errp) { if (kvm_enabled()) { @@ -236,7 +216,6 @@ SpaprIrq spapr_irq_xive = { .dt_populate = spapr_dt_xive, .post_load = spapr_irq_post_load_xive, .reset = spapr_irq_reset_xive, - .set_irq = spapr_irq_set_irq_xive, .init_kvm = spapr_irq_init_kvm_xive, }; @@ -316,13 +295,6 @@ static void spapr_irq_reset_dual(SpaprMachineState *spapr, Error **errp) spapr_irq_current(spapr)->reset(spapr, errp); } -static void spapr_irq_set_irq_dual(void *opaque, int irq, int val) -{ - SpaprMachineState *spapr = opaque; - - spapr_irq_current(spapr)->set_irq(spapr, irq, val); -} - /* * Define values in sync with the XIVE and XICS backend */ @@ -336,7 +308,6 @@ SpaprIrq spapr_irq_dual = { .dt_populate = spapr_irq_dt_populate_dual, .post_load = spapr_irq_post_load_dual, .reset = spapr_irq_reset_dual, - .set_irq = spapr_irq_set_irq_dual, .init_kvm = NULL, /* should not be used */ }; @@ -422,6 +393,15 @@ int spapr_irq_cpu_intc_create(SpaprMachineState *spapr, return 0; } +static void spapr_set_irq(void *opaque, int irq, int level) +{ + SpaprMachineState *spapr = SPAPR_MACHINE(opaque); + SpaprInterruptControllerClass *sicc + = SPAPR_INTC_GET_CLASS(spapr->active_intc); + + sicc->set_irq(spapr->active_intc, irq, level); +} + void spapr_irq_init(SpaprMachineState *spapr, Error **errp) { MachineState *machine = MACHINE(spapr); @@ -510,7 +490,7 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp) spapr_xive_hcall_init(spapr); } - spapr->qirqs = qemu_allocate_irqs(spapr->irq->set_irq, spapr, + spapr->qirqs = qemu_allocate_irqs(spapr_set_irq, spapr, spapr->irq->nr_xirqs + SPAPR_XIRQ_BASE); out: @@ -744,7 +724,6 @@ SpaprIrq spapr_irq_xics_legacy = { .dt_populate = spapr_dt_xics, .post_load = spapr_irq_post_load_xics, .reset = spapr_irq_reset_xics, - .set_irq = spapr_irq_set_irq_xics, .init_kvm = spapr_irq_init_kvm_xics, }; diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h index 3102d152b2..8286a9aa63 100644 --- a/include/hw/ppc/spapr_irq.h +++ b/include/hw/ppc/spapr_irq.h @@ -56,6 +56,9 @@ typedef struct SpaprInterruptControllerClass { int (*claim_irq)(SpaprInterruptController *intc, int irq, bool lsi, Error **errp); void (*free_irq)(SpaprInterruptController *intc, int irq); + + /* These methods should only be called on the active intc */ + void (*set_irq)(SpaprInterruptController *intc, int irq, int val); } SpaprInterruptControllerClass; void spapr_irq_update_active_intc(SpaprMachineState *spapr); @@ -83,7 +86,6 @@ typedef struct SpaprIrq { void *fdt, uint32_t phandle); int (*post_load)(SpaprMachineState *spapr, int version_id); void (*reset)(SpaprMachineState *spapr, Error **errp); - void (*set_irq)(void *opaque, int srcno, int val); void (*init_kvm)(SpaprMachineState *spapr, Error **errp); } SpaprIrq;
This method depends only on the active irq controller. Now that we've formalized the notion of active controller we can dispatch directly through that, rather than dispatching via SpaprIrq with the dual version having to do a second conditional dispatch. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- hw/intc/spapr_xive.c | 12 +++++++++++ hw/intc/xics_spapr.c | 9 +++++++++ hw/ppc/spapr_irq.c | 41 ++++++++++---------------------------- include/hw/ppc/spapr_irq.h | 4 +++- 4 files changed, 34 insertions(+), 32 deletions(-)