| Message ID | 20190925064534.19155-21-david@gibson.dropbear.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spapr: IRQ subsystem cleanups | expand |
On 25/09/2019 08:45, David Gibson wrote: > This method is used to set up the interrupt backends for the current > configuration. However, this means some confusing redirection between > the "dual" mode init and the init hooks for xics only and xive only modes. > > Since we now have simple flags indicating whether XICS and/or XIVE are > supported, it's easier to just open code each initialization directly in > spapr_irq_init(). This will also make some future cleanups simpler. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Reviewed-by: Cédric Le Goater <clg@kaod.org> one comment below, > --- > hw/ppc/spapr_irq.c | 138 ++++++++++++++++-------------------- > include/hw/ppc/spapr_irq.h | 1 - > include/hw/ppc/xics_spapr.h | 1 + > 3 files changed, 63 insertions(+), 77 deletions(-) > > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c > index 073f375ba2..62647dd5a3 100644 > --- a/hw/ppc/spapr_irq.c > +++ b/hw/ppc/spapr_irq.c > @@ -91,27 +91,6 @@ static void spapr_irq_init_kvm(SpaprMachineState *spapr, > /* > * XICS IRQ backend. > */ > - > -static void spapr_irq_init_xics(SpaprMachineState *spapr, Error **errp) > -{ > - Object *obj; > - Error *local_err = NULL; > - > - obj = object_new(TYPE_ICS_SPAPR); > - object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort); > - object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr), > - &error_fatal); > - object_property_set_int(obj, spapr->irq->nr_xirqs, > - "nr-irqs", &error_fatal); > - object_property_set_bool(obj, true, "realized", &local_err); > - if (local_err) { > - error_propagate(errp, local_err); > - return; > - } > - > - spapr->ics = ICS_SPAPR(obj); > -} > - > static void spapr_irq_claim_xics(SpaprMachineState *spapr, int irq, bool lsi, > Error **errp) > { > @@ -212,7 +191,6 @@ SpaprIrq spapr_irq_xics = { > .xics = true, > .xive = false, > > - .init = spapr_irq_init_xics, > .claim = spapr_irq_claim_xics, > .free = spapr_irq_free_xics, > .print_info = spapr_irq_print_info_xics, > @@ -227,37 +205,6 @@ SpaprIrq spapr_irq_xics = { > /* > * XIVE IRQ backend. > */ > -static void spapr_irq_init_xive(SpaprMachineState *spapr, Error **errp) > -{ > - uint32_t nr_servers = spapr_max_server_number(spapr); > - DeviceState *dev; > - int i; > - > - dev = qdev_create(NULL, TYPE_SPAPR_XIVE); > - qdev_prop_set_uint32(dev, "nr-irqs", > - spapr->irq->nr_xirqs + SPAPR_XIRQ_BASE); > - /* > - * 8 XIVE END structures per CPU. One for each available priority > - */ > - qdev_prop_set_uint32(dev, "nr-ends", nr_servers << 3); > - qdev_init_nofail(dev); > - > - spapr->xive = SPAPR_XIVE(dev); > - > - /* Enable the CPU IPIs */ > - for (i = 0; i < nr_servers; ++i) { > - Error *local_err = NULL; > - > - spapr_xive_irq_claim(spapr->xive, SPAPR_IRQ_IPI + i, false, &local_err); > - if (local_err) { > - error_propagate(errp, local_err); > - return; > - } > - } > - > - spapr_xive_hcall_init(spapr); > -} > - > static void spapr_irq_claim_xive(SpaprMachineState *spapr, int irq, bool lsi, > Error **errp) > { > @@ -361,7 +308,6 @@ SpaprIrq spapr_irq_xive = { > .xics = false, > .xive = true, > > - .init = spapr_irq_init_xive, > .claim = spapr_irq_claim_xive, > .free = spapr_irq_free_xive, > .print_info = spapr_irq_print_info_xive, > @@ -392,23 +338,6 @@ static SpaprIrq *spapr_irq_current(SpaprMachineState *spapr) > &spapr_irq_xive : &spapr_irq_xics; > } > > -static void spapr_irq_init_dual(SpaprMachineState *spapr, Error **errp) > -{ > - Error *local_err = NULL; > - > - spapr_irq_xics.init(spapr, &local_err); > - if (local_err) { > - error_propagate(errp, local_err); > - return; > - } > - > - spapr_irq_xive.init(spapr, &local_err); > - if (local_err) { > - error_propagate(errp, local_err); > - return; > - } > -} > - > static void spapr_irq_claim_dual(SpaprMachineState *spapr, int irq, bool lsi, > Error **errp) > { > @@ -520,7 +449,6 @@ SpaprIrq spapr_irq_dual = { > .xics = true, > .xive = true, > > - .init = spapr_irq_init_dual, > .claim = spapr_irq_claim_dual, > .free = spapr_irq_free_dual, > .print_info = spapr_irq_print_info_dual, > @@ -608,8 +536,7 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp) > > spapr_irq_check(spapr, &local_err); > if (local_err) { > - error_propagate(errp, local_err); > - return; > + goto out; > } > > /* Initialize the MSI IRQ allocator. */ > @@ -617,10 +544,70 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp) > spapr_irq_msi_init(spapr, spapr->irq->nr_msis); > } > > - spapr->irq->init(spapr, errp); > + if (spapr->irq->xics) { > + Object *obj; > + > + obj = object_new(TYPE_ICS_SPAPR); > + object_property_add_child(OBJECT(spapr), "ics", obj, &local_err); > + if (local_err) { > + goto out; > + } > + > + object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr), > + &local_err); > + if (local_err) { > + goto out; > + } > + > + object_property_set_int(obj, spapr->irq->nr_xirqs, "nr-irqs", > + &local_err); > + if (local_err) { > + goto out; > + } > + > + object_property_set_bool(obj, true, "realized", &local_err); > + if (local_err) { > + goto out; > + } > + > + spapr->ics = ICS_SPAPR(obj); > + } > + > + if (spapr->irq->xive) { > + uint32_t nr_servers = spapr_max_server_number(spapr); > + DeviceState *dev; > + int i; > + > + dev = qdev_create(NULL, TYPE_SPAPR_XIVE); > + qdev_prop_set_uint32(dev, "nr-irqs", > + spapr->irq->nr_xirqs + SPAPR_XIRQ_BASE); > + /* > + * 8 XIVE END structures per CPU. One for each available > + * priority > + */ > + qdev_prop_set_uint32(dev, "nr-ends", nr_servers << 3); > + qdev_init_nofail(dev); > + > + spapr->xive = SPAPR_XIVE(dev); > + > + /* Enable the CPU IPIs */ > + for (i = 0; i < nr_servers; ++i) { > + Error *local_err = NULL; > + > + spapr_xive_irq_claim(spapr->xive, SPAPR_IRQ_IPI + i, false, &local_err); > + if (local_err) { > + goto out; > + } > + } We could move the IPI claim part in the realize routine of SPAPR_XIVE. > + spapr_xive_hcall_init(spapr); This also. It can be done later one. C. > + } > > spapr->qirqs = qemu_allocate_irqs(spapr->irq->set_irq, spapr, > spapr->irq->nr_xirqs + SPAPR_XIRQ_BASE); > + > +out: > + error_propagate(errp, local_err); > } > > void spapr_irq_claim(SpaprMachineState *spapr, int irq, bool lsi, Error **errp) > @@ -757,7 +744,6 @@ SpaprIrq spapr_irq_xics_legacy = { > .xics = true, > .xive = false, > > - .init = spapr_irq_init_xics, > .claim = spapr_irq_claim_xics, > .free = spapr_irq_free_xics, > .print_info = spapr_irq_print_info_xics, > diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h > index 6816cb0500..fa862c665b 100644 > --- a/include/hw/ppc/spapr_irq.h > +++ b/include/hw/ppc/spapr_irq.h > @@ -42,7 +42,6 @@ typedef struct SpaprIrq { > bool xics; > bool xive; > > - void (*init)(SpaprMachineState *spapr, Error **errp); > void (*claim)(SpaprMachineState *spapr, int irq, bool lsi, Error **errp); > void (*free)(SpaprMachineState *spapr, int irq); > void (*print_info)(SpaprMachineState *spapr, Monitor *mon); > diff --git a/include/hw/ppc/xics_spapr.h b/include/hw/ppc/xics_spapr.h > index 691a6d00f7..267984a97b 100644 > --- a/include/hw/ppc/xics_spapr.h > +++ b/include/hw/ppc/xics_spapr.h > @@ -34,6 +34,7 @@ > #define TYPE_ICS_SPAPR "ics-spapr" > #define ICS_SPAPR(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_SPAPR) > > +void ics_spapr_create(SpaprMachineState *spapr, int nr_xirqs, Error **errp); > void spapr_dt_xics(SpaprMachineState *spapr, uint32_t nr_servers, void *fdt, > uint32_t phandle); > int xics_kvm_connect(SpaprMachineState *spapr, Error **errp); >
On Wed, Sep 25, 2019 at 09:31:54AM +0200, Cédric Le Goater wrote: > On 25/09/2019 08:45, David Gibson wrote: > > This method is used to set up the interrupt backends for the current > > configuration. However, this means some confusing redirection between > > the "dual" mode init and the init hooks for xics only and xive only modes. > > > > Since we now have simple flags indicating whether XICS and/or XIVE are > > supported, it's easier to just open code each initialization directly in > > spapr_irq_init(). This will also make some future cleanups simpler. > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > Reviewed-by: Cédric Le Goater <clg@kaod.org> > > one comment below, > > > --- > > hw/ppc/spapr_irq.c | 138 ++++++++++++++++-------------------- > > include/hw/ppc/spapr_irq.h | 1 - > > include/hw/ppc/xics_spapr.h | 1 + > > 3 files changed, 63 insertions(+), 77 deletions(-) > > > > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c > > index 073f375ba2..62647dd5a3 100644 > > --- a/hw/ppc/spapr_irq.c > > +++ b/hw/ppc/spapr_irq.c > > @@ -91,27 +91,6 @@ static void spapr_irq_init_kvm(SpaprMachineState *spapr, > > /* > > * XICS IRQ backend. > > */ > > - > > -static void spapr_irq_init_xics(SpaprMachineState *spapr, Error **errp) > > -{ > > - Object *obj; > > - Error *local_err = NULL; > > - > > - obj = object_new(TYPE_ICS_SPAPR); > > - object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort); > > - object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr), > > - &error_fatal); > > - object_property_set_int(obj, spapr->irq->nr_xirqs, > > - "nr-irqs", &error_fatal); > > - object_property_set_bool(obj, true, "realized", &local_err); > > - if (local_err) { > > - error_propagate(errp, local_err); > > - return; > > - } > > - > > - spapr->ics = ICS_SPAPR(obj); > > -} > > - > > static void spapr_irq_claim_xics(SpaprMachineState *spapr, int irq, bool lsi, > > Error **errp) > > { > > @@ -212,7 +191,6 @@ SpaprIrq spapr_irq_xics = { > > .xics = true, > > .xive = false, > > > > - .init = spapr_irq_init_xics, > > .claim = spapr_irq_claim_xics, > > .free = spapr_irq_free_xics, > > .print_info = spapr_irq_print_info_xics, > > @@ -227,37 +205,6 @@ SpaprIrq spapr_irq_xics = { > > /* > > * XIVE IRQ backend. > > */ > > -static void spapr_irq_init_xive(SpaprMachineState *spapr, Error **errp) > > -{ > > - uint32_t nr_servers = spapr_max_server_number(spapr); > > - DeviceState *dev; > > - int i; > > - > > - dev = qdev_create(NULL, TYPE_SPAPR_XIVE); > > - qdev_prop_set_uint32(dev, "nr-irqs", > > - spapr->irq->nr_xirqs + SPAPR_XIRQ_BASE); > > - /* > > - * 8 XIVE END structures per CPU. One for each available priority > > - */ > > - qdev_prop_set_uint32(dev, "nr-ends", nr_servers << 3); > > - qdev_init_nofail(dev); > > - > > - spapr->xive = SPAPR_XIVE(dev); > > - > > - /* Enable the CPU IPIs */ > > - for (i = 0; i < nr_servers; ++i) { > > - Error *local_err = NULL; > > - > > - spapr_xive_irq_claim(spapr->xive, SPAPR_IRQ_IPI + i, false, &local_err); > > - if (local_err) { > > - error_propagate(errp, local_err); > > - return; > > - } > > - } > > - > > - spapr_xive_hcall_init(spapr); > > -} > > - > > static void spapr_irq_claim_xive(SpaprMachineState *spapr, int irq, bool lsi, > > Error **errp) > > { > > @@ -361,7 +308,6 @@ SpaprIrq spapr_irq_xive = { > > .xics = false, > > .xive = true, > > > > - .init = spapr_irq_init_xive, > > .claim = spapr_irq_claim_xive, > > .free = spapr_irq_free_xive, > > .print_info = spapr_irq_print_info_xive, > > @@ -392,23 +338,6 @@ static SpaprIrq *spapr_irq_current(SpaprMachineState *spapr) > > &spapr_irq_xive : &spapr_irq_xics; > > } > > > > -static void spapr_irq_init_dual(SpaprMachineState *spapr, Error **errp) > > -{ > > - Error *local_err = NULL; > > - > > - spapr_irq_xics.init(spapr, &local_err); > > - if (local_err) { > > - error_propagate(errp, local_err); > > - return; > > - } > > - > > - spapr_irq_xive.init(spapr, &local_err); > > - if (local_err) { > > - error_propagate(errp, local_err); > > - return; > > - } > > -} > > - > > static void spapr_irq_claim_dual(SpaprMachineState *spapr, int irq, bool lsi, > > Error **errp) > > { > > @@ -520,7 +449,6 @@ SpaprIrq spapr_irq_dual = { > > .xics = true, > > .xive = true, > > > > - .init = spapr_irq_init_dual, > > .claim = spapr_irq_claim_dual, > > .free = spapr_irq_free_dual, > > .print_info = spapr_irq_print_info_dual, > > @@ -608,8 +536,7 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp) > > > > spapr_irq_check(spapr, &local_err); > > if (local_err) { > > - error_propagate(errp, local_err); > > - return; > > + goto out; > > } > > > > /* Initialize the MSI IRQ allocator. */ > > @@ -617,10 +544,70 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp) > > spapr_irq_msi_init(spapr, spapr->irq->nr_msis); > > } > > > > - spapr->irq->init(spapr, errp); > > + if (spapr->irq->xics) { > > + Object *obj; > > + > > + obj = object_new(TYPE_ICS_SPAPR); > > + object_property_add_child(OBJECT(spapr), "ics", obj, &local_err); > > + if (local_err) { > > + goto out; > > + } > > + > > + object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr), > > + &local_err); > > + if (local_err) { > > + goto out; > > + } > > + > > + object_property_set_int(obj, spapr->irq->nr_xirqs, "nr-irqs", > > + &local_err); > > + if (local_err) { > > + goto out; > > + } > > + > > + object_property_set_bool(obj, true, "realized", &local_err); > > + if (local_err) { > > + goto out; > > + } > > + > > + spapr->ics = ICS_SPAPR(obj); > > + } > > + > > + if (spapr->irq->xive) { > > + uint32_t nr_servers = spapr_max_server_number(spapr); > > + DeviceState *dev; > > + int i; > > + > > + dev = qdev_create(NULL, TYPE_SPAPR_XIVE); > > + qdev_prop_set_uint32(dev, "nr-irqs", > > + spapr->irq->nr_xirqs + SPAPR_XIRQ_BASE); > > + /* > > + * 8 XIVE END structures per CPU. One for each available > > + * priority > > + */ > > + qdev_prop_set_uint32(dev, "nr-ends", nr_servers << 3); > > + qdev_init_nofail(dev); > > + > > + spapr->xive = SPAPR_XIVE(dev); > > + > > + /* Enable the CPU IPIs */ > > + for (i = 0; i < nr_servers; ++i) { > > + Error *local_err = NULL; > > + > > + spapr_xive_irq_claim(spapr->xive, SPAPR_IRQ_IPI + i, false, &local_err); > > + if (local_err) { > > + goto out; > > + } > > + } > > We could move the IPI claim part in the realize routine of SPAPR_XIVE. Yeah, I know. I thought about this, but there's a slight complication in that the XIVE part doesn't know nr_servers directly. There's several possible ways to handle that, but I wasn't 100% happy with any that I came up with yet. > > > + spapr_xive_hcall_init(spapr); > > This also. Right. > It can be done later one. That's my intention. > > C. > > > + } > > > > spapr->qirqs = qemu_allocate_irqs(spapr->irq->set_irq, spapr, > > spapr->irq->nr_xirqs + SPAPR_XIRQ_BASE); > > + > > +out: > > + error_propagate(errp, local_err); > > } > > > > void spapr_irq_claim(SpaprMachineState *spapr, int irq, bool lsi, Error **errp) > > @@ -757,7 +744,6 @@ SpaprIrq spapr_irq_xics_legacy = { > > .xics = true, > > .xive = false, > > > > - .init = spapr_irq_init_xics, > > .claim = spapr_irq_claim_xics, > > .free = spapr_irq_free_xics, > > .print_info = spapr_irq_print_info_xics, > > diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h > > index 6816cb0500..fa862c665b 100644 > > --- a/include/hw/ppc/spapr_irq.h > > +++ b/include/hw/ppc/spapr_irq.h > > @@ -42,7 +42,6 @@ typedef struct SpaprIrq { > > bool xics; > > bool xive; > > > > - void (*init)(SpaprMachineState *spapr, Error **errp); > > void (*claim)(SpaprMachineState *spapr, int irq, bool lsi, Error **errp); > > void (*free)(SpaprMachineState *spapr, int irq); > > void (*print_info)(SpaprMachineState *spapr, Monitor *mon); > > diff --git a/include/hw/ppc/xics_spapr.h b/include/hw/ppc/xics_spapr.h > > index 691a6d00f7..267984a97b 100644 > > --- a/include/hw/ppc/xics_spapr.h > > +++ b/include/hw/ppc/xics_spapr.h > > @@ -34,6 +34,7 @@ > > #define TYPE_ICS_SPAPR "ics-spapr" > > #define ICS_SPAPR(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_SPAPR) > > > > +void ics_spapr_create(SpaprMachineState *spapr, int nr_xirqs, Error **errp); > > void spapr_dt_xics(SpaprMachineState *spapr, uint32_t nr_servers, void *fdt, > > uint32_t phandle); > > int xics_kvm_connect(SpaprMachineState *spapr, Error **errp); > > >
>>> + if (spapr->irq->xive) { >>> + uint32_t nr_servers = spapr_max_server_number(spapr); >>> + DeviceState *dev; >>> + int i; >>> + >>> + dev = qdev_create(NULL, TYPE_SPAPR_XIVE); >>> + qdev_prop_set_uint32(dev, "nr-irqs", >>> + spapr->irq->nr_xirqs + SPAPR_XIRQ_BASE); >>> + /* >>> + * 8 XIVE END structures per CPU. One for each available >>> + * priority >>> + */ >>> + qdev_prop_set_uint32(dev, "nr-ends", nr_servers << 3); >>> + qdev_init_nofail(dev); >>> + >>> + spapr->xive = SPAPR_XIVE(dev); >>> + >>> + /* Enable the CPU IPIs */ >>> + for (i = 0; i < nr_servers; ++i) { >>> + Error *local_err = NULL; >>> + >>> + spapr_xive_irq_claim(spapr->xive, SPAPR_IRQ_IPI + i, false, &local_err); >>> + if (local_err) { >>> + goto out; >>> + } >>> + } >> >> We could move the IPI claim part in the realize routine of SPAPR_XIVE. > > Yeah, I know. I thought about this, but there's a slight complication > in that the XIVE part doesn't know nr_servers directly. There's > several possible ways to handle that, but I wasn't 100% happy with any > that I came up with yet. The "nr-ends" property was inappropriate, "nr-servers" would have been better and we would have hidden the calculation of ENDs 'nr_servers << 3' in the realize routine of SpaprXive. I don't think we can change that without breaking migration though :/ C. >> >>> + spapr_xive_hcall_init(spapr); >> >> This also. > > Right. > >> It can be done later one. > > That's my intention. > >> >> C. >> >>> + } >>> >>> spapr->qirqs = qemu_allocate_irqs(spapr->irq->set_irq, spapr, >>> spapr->irq->nr_xirqs + SPAPR_XIRQ_BASE); >>> + >>> +out: >>> + error_propagate(errp, local_err); >>> } >>> >>> void spapr_irq_claim(SpaprMachineState *spapr, int irq, bool lsi, Error **errp) >>> @@ -757,7 +744,6 @@ SpaprIrq spapr_irq_xics_legacy = { >>> .xics = true, >>> .xive = false, >>> >>> - .init = spapr_irq_init_xics, >>> .claim = spapr_irq_claim_xics, >>> .free = spapr_irq_free_xics, >>> .print_info = spapr_irq_print_info_xics, >>> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h >>> index 6816cb0500..fa862c665b 100644 >>> --- a/include/hw/ppc/spapr_irq.h >>> +++ b/include/hw/ppc/spapr_irq.h >>> @@ -42,7 +42,6 @@ typedef struct SpaprIrq { >>> bool xics; >>> bool xive; >>> >>> - void (*init)(SpaprMachineState *spapr, Error **errp); >>> void (*claim)(SpaprMachineState *spapr, int irq, bool lsi, Error **errp); >>> void (*free)(SpaprMachineState *spapr, int irq); >>> void (*print_info)(SpaprMachineState *spapr, Monitor *mon); >>> diff --git a/include/hw/ppc/xics_spapr.h b/include/hw/ppc/xics_spapr.h >>> index 691a6d00f7..267984a97b 100644 >>> --- a/include/hw/ppc/xics_spapr.h >>> +++ b/include/hw/ppc/xics_spapr.h >>> @@ -34,6 +34,7 @@ >>> #define TYPE_ICS_SPAPR "ics-spapr" >>> #define ICS_SPAPR(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_SPAPR) >>> >>> +void ics_spapr_create(SpaprMachineState *spapr, int nr_xirqs, Error **errp); >>> void spapr_dt_xics(SpaprMachineState *spapr, uint32_t nr_servers, void *fdt, >>> uint32_t phandle); >>> int xics_kvm_connect(SpaprMachineState *spapr, Error **errp); >>> >> >
On Thu, Sep 26, 2019 at 09:05:56AM +0200, Cédric Le Goater wrote: > >>> + if (spapr->irq->xive) { > >>> + uint32_t nr_servers = spapr_max_server_number(spapr); > >>> + DeviceState *dev; > >>> + int i; > >>> + > >>> + dev = qdev_create(NULL, TYPE_SPAPR_XIVE); > >>> + qdev_prop_set_uint32(dev, "nr-irqs", > >>> + spapr->irq->nr_xirqs + SPAPR_XIRQ_BASE); > >>> + /* > >>> + * 8 XIVE END structures per CPU. One for each available > >>> + * priority > >>> + */ > >>> + qdev_prop_set_uint32(dev, "nr-ends", nr_servers << 3); > >>> + qdev_init_nofail(dev); > >>> + > >>> + spapr->xive = SPAPR_XIVE(dev); > >>> + > >>> + /* Enable the CPU IPIs */ > >>> + for (i = 0; i < nr_servers; ++i) { > >>> + Error *local_err = NULL; > >>> + > >>> + spapr_xive_irq_claim(spapr->xive, SPAPR_IRQ_IPI + i, false, &local_err); > >>> + if (local_err) { > >>> + goto out; > >>> + } > >>> + } > >> > >> We could move the IPI claim part in the realize routine of SPAPR_XIVE. > > > > Yeah, I know. I thought about this, but there's a slight complication > > in that the XIVE part doesn't know nr_servers directly. There's > > several possible ways to handle that, but I wasn't 100% happy with any > > that I came up with yet. > > The "nr-ends" property was inappropriate, "nr-servers" would have been > better and we would have hidden the calculation of ENDs 'nr_servers << 3' > in the realize routine of SpaprXive. Ah, interesting. > I don't think we can change that without breaking migration though > :/ Hm, there might be a way around it, I'll see what I can do, but probably as a rather later patch. > > C. > > >> > >>> + spapr_xive_hcall_init(spapr); > >> > >> This also. > > > > Right. > > > >> It can be done later one. > > > > That's my intention. > > > >> > >> C. > >> > >>> + } > >>> > >>> spapr->qirqs = qemu_allocate_irqs(spapr->irq->set_irq, spapr, > >>> spapr->irq->nr_xirqs + SPAPR_XIRQ_BASE); > >>> + > >>> +out: > >>> + error_propagate(errp, local_err); > >>> } > >>> > >>> void spapr_irq_claim(SpaprMachineState *spapr, int irq, bool lsi, Error **errp) > >>> @@ -757,7 +744,6 @@ SpaprIrq spapr_irq_xics_legacy = { > >>> .xics = true, > >>> .xive = false, > >>> > >>> - .init = spapr_irq_init_xics, > >>> .claim = spapr_irq_claim_xics, > >>> .free = spapr_irq_free_xics, > >>> .print_info = spapr_irq_print_info_xics, > >>> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h > >>> index 6816cb0500..fa862c665b 100644 > >>> --- a/include/hw/ppc/spapr_irq.h > >>> +++ b/include/hw/ppc/spapr_irq.h > >>> @@ -42,7 +42,6 @@ typedef struct SpaprIrq { > >>> bool xics; > >>> bool xive; > >>> > >>> - void (*init)(SpaprMachineState *spapr, Error **errp); > >>> void (*claim)(SpaprMachineState *spapr, int irq, bool lsi, Error **errp); > >>> void (*free)(SpaprMachineState *spapr, int irq); > >>> void (*print_info)(SpaprMachineState *spapr, Monitor *mon); > >>> diff --git a/include/hw/ppc/xics_spapr.h b/include/hw/ppc/xics_spapr.h > >>> index 691a6d00f7..267984a97b 100644 > >>> --- a/include/hw/ppc/xics_spapr.h > >>> +++ b/include/hw/ppc/xics_spapr.h > >>> @@ -34,6 +34,7 @@ > >>> #define TYPE_ICS_SPAPR "ics-spapr" > >>> #define ICS_SPAPR(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_SPAPR) > >>> > >>> +void ics_spapr_create(SpaprMachineState *spapr, int nr_xirqs, Error **errp); > >>> void spapr_dt_xics(SpaprMachineState *spapr, uint32_t nr_servers, void *fdt, > >>> uint32_t phandle); > >>> int xics_kvm_connect(SpaprMachineState *spapr, Error **errp); > >>> > >> > > >
On Thu, 26 Sep 2019 09:05:56 +0200 Cédric Le Goater <clg@kaod.org> wrote: > >>> + if (spapr->irq->xive) { > >>> + uint32_t nr_servers = spapr_max_server_number(spapr); > >>> + DeviceState *dev; > >>> + int i; > >>> + > >>> + dev = qdev_create(NULL, TYPE_SPAPR_XIVE); > >>> + qdev_prop_set_uint32(dev, "nr-irqs", > >>> + spapr->irq->nr_xirqs + SPAPR_XIRQ_BASE); > >>> + /* > >>> + * 8 XIVE END structures per CPU. One for each available > >>> + * priority > >>> + */ > >>> + qdev_prop_set_uint32(dev, "nr-ends", nr_servers << 3); > >>> + qdev_init_nofail(dev); > >>> + > >>> + spapr->xive = SPAPR_XIVE(dev); > >>> + > >>> + /* Enable the CPU IPIs */ > >>> + for (i = 0; i < nr_servers; ++i) { > >>> + Error *local_err = NULL; > >>> + > >>> + spapr_xive_irq_claim(spapr->xive, SPAPR_IRQ_IPI + i, false, &local_err); > >>> + if (local_err) { > >>> + goto out; > >>> + } > >>> + } > >> > >> We could move the IPI claim part in the realize routine of SPAPR_XIVE. > > > > Yeah, I know. I thought about this, but there's a slight complication > > in that the XIVE part doesn't know nr_servers directly. There's > > several possible ways to handle that, but I wasn't 100% happy with any > > that I came up with yet. > > The "nr-ends" property was inappropriate, "nr-servers" would have been > better and we would have hidden the calculation of ENDs 'nr_servers << 3' > in the realize routine of SpaprXive. > Yeah it would make sense to have nr_servers within the sPAPR XIVE object, so that we don't have to pass it when building the FDT node. Same stands for XICS actually. And as part of my current work to reduce HW VP consumption, I also need nr_servers to pass it to the KVM device. > I don't think we can change that without breaking migration though :/ > Hmm... why ? The QOM property is just an interface, it doesn't change the state. In the end we migrate the same number of XiveEND objects. > C. > > >> > >>> + spapr_xive_hcall_init(spapr); > >> > >> This also. > > > > Right. > > > >> It can be done later one. > > > > That's my intention. > > > >> > >> C. > >> > >>> + } > >>> > >>> spapr->qirqs = qemu_allocate_irqs(spapr->irq->set_irq, spapr, > >>> spapr->irq->nr_xirqs + SPAPR_XIRQ_BASE); > >>> + > >>> +out: > >>> + error_propagate(errp, local_err); > >>> } > >>> > >>> void spapr_irq_claim(SpaprMachineState *spapr, int irq, bool lsi, Error **errp) > >>> @@ -757,7 +744,6 @@ SpaprIrq spapr_irq_xics_legacy = { > >>> .xics = true, > >>> .xive = false, > >>> > >>> - .init = spapr_irq_init_xics, > >>> .claim = spapr_irq_claim_xics, > >>> .free = spapr_irq_free_xics, > >>> .print_info = spapr_irq_print_info_xics, > >>> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h > >>> index 6816cb0500..fa862c665b 100644 > >>> --- a/include/hw/ppc/spapr_irq.h > >>> +++ b/include/hw/ppc/spapr_irq.h > >>> @@ -42,7 +42,6 @@ typedef struct SpaprIrq { > >>> bool xics; > >>> bool xive; > >>> > >>> - void (*init)(SpaprMachineState *spapr, Error **errp); > >>> void (*claim)(SpaprMachineState *spapr, int irq, bool lsi, Error **errp); > >>> void (*free)(SpaprMachineState *spapr, int irq); > >>> void (*print_info)(SpaprMachineState *spapr, Monitor *mon); > >>> diff --git a/include/hw/ppc/xics_spapr.h b/include/hw/ppc/xics_spapr.h > >>> index 691a6d00f7..267984a97b 100644 > >>> --- a/include/hw/ppc/xics_spapr.h > >>> +++ b/include/hw/ppc/xics_spapr.h > >>> @@ -34,6 +34,7 @@ > >>> #define TYPE_ICS_SPAPR "ics-spapr" > >>> #define ICS_SPAPR(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_SPAPR) > >>> > >>> +void ics_spapr_create(SpaprMachineState *spapr, int nr_xirqs, Error **errp); > >>> void spapr_dt_xics(SpaprMachineState *spapr, uint32_t nr_servers, void *fdt, > >>> uint32_t phandle); > >>> int xics_kvm_connect(SpaprMachineState *spapr, Error **errp); > >>> > >> > > >
On Wed, 25 Sep 2019 16:45:34 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > This method is used to set up the interrupt backends for the current > configuration. However, this means some confusing redirection between > the "dual" mode init and the init hooks for xics only and xive only modes. > > Since we now have simple flags indicating whether XICS and/or XIVE are > supported, it's easier to just open code each initialization directly in > spapr_irq_init(). This will also make some future cleanups simpler. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- Reviewed-by: Greg Kurz <groug@kaod.org> > hw/ppc/spapr_irq.c | 138 ++++++++++++++++-------------------- > include/hw/ppc/spapr_irq.h | 1 - > include/hw/ppc/xics_spapr.h | 1 + > 3 files changed, 63 insertions(+), 77 deletions(-) > > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c > index 073f375ba2..62647dd5a3 100644 > --- a/hw/ppc/spapr_irq.c > +++ b/hw/ppc/spapr_irq.c > @@ -91,27 +91,6 @@ static void spapr_irq_init_kvm(SpaprMachineState *spapr, > /* > * XICS IRQ backend. > */ > - > -static void spapr_irq_init_xics(SpaprMachineState *spapr, Error **errp) > -{ > - Object *obj; > - Error *local_err = NULL; > - > - obj = object_new(TYPE_ICS_SPAPR); > - object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort); > - object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr), > - &error_fatal); > - object_property_set_int(obj, spapr->irq->nr_xirqs, > - "nr-irqs", &error_fatal); > - object_property_set_bool(obj, true, "realized", &local_err); > - if (local_err) { > - error_propagate(errp, local_err); > - return; > - } > - > - spapr->ics = ICS_SPAPR(obj); > -} > - > static void spapr_irq_claim_xics(SpaprMachineState *spapr, int irq, bool lsi, > Error **errp) > { > @@ -212,7 +191,6 @@ SpaprIrq spapr_irq_xics = { > .xics = true, > .xive = false, > > - .init = spapr_irq_init_xics, > .claim = spapr_irq_claim_xics, > .free = spapr_irq_free_xics, > .print_info = spapr_irq_print_info_xics, > @@ -227,37 +205,6 @@ SpaprIrq spapr_irq_xics = { > /* > * XIVE IRQ backend. > */ > -static void spapr_irq_init_xive(SpaprMachineState *spapr, Error **errp) > -{ > - uint32_t nr_servers = spapr_max_server_number(spapr); > - DeviceState *dev; > - int i; > - > - dev = qdev_create(NULL, TYPE_SPAPR_XIVE); > - qdev_prop_set_uint32(dev, "nr-irqs", > - spapr->irq->nr_xirqs + SPAPR_XIRQ_BASE); > - /* > - * 8 XIVE END structures per CPU. One for each available priority > - */ > - qdev_prop_set_uint32(dev, "nr-ends", nr_servers << 3); > - qdev_init_nofail(dev); > - > - spapr->xive = SPAPR_XIVE(dev); > - > - /* Enable the CPU IPIs */ > - for (i = 0; i < nr_servers; ++i) { > - Error *local_err = NULL; > - > - spapr_xive_irq_claim(spapr->xive, SPAPR_IRQ_IPI + i, false, &local_err); > - if (local_err) { > - error_propagate(errp, local_err); > - return; > - } > - } > - > - spapr_xive_hcall_init(spapr); > -} > - > static void spapr_irq_claim_xive(SpaprMachineState *spapr, int irq, bool lsi, > Error **errp) > { > @@ -361,7 +308,6 @@ SpaprIrq spapr_irq_xive = { > .xics = false, > .xive = true, > > - .init = spapr_irq_init_xive, > .claim = spapr_irq_claim_xive, > .free = spapr_irq_free_xive, > .print_info = spapr_irq_print_info_xive, > @@ -392,23 +338,6 @@ static SpaprIrq *spapr_irq_current(SpaprMachineState *spapr) > &spapr_irq_xive : &spapr_irq_xics; > } > > -static void spapr_irq_init_dual(SpaprMachineState *spapr, Error **errp) > -{ > - Error *local_err = NULL; > - > - spapr_irq_xics.init(spapr, &local_err); > - if (local_err) { > - error_propagate(errp, local_err); > - return; > - } > - > - spapr_irq_xive.init(spapr, &local_err); > - if (local_err) { > - error_propagate(errp, local_err); > - return; > - } > -} > - > static void spapr_irq_claim_dual(SpaprMachineState *spapr, int irq, bool lsi, > Error **errp) > { > @@ -520,7 +449,6 @@ SpaprIrq spapr_irq_dual = { > .xics = true, > .xive = true, > > - .init = spapr_irq_init_dual, > .claim = spapr_irq_claim_dual, > .free = spapr_irq_free_dual, > .print_info = spapr_irq_print_info_dual, > @@ -608,8 +536,7 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp) > > spapr_irq_check(spapr, &local_err); > if (local_err) { > - error_propagate(errp, local_err); > - return; > + goto out; > } > > /* Initialize the MSI IRQ allocator. */ > @@ -617,10 +544,70 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp) > spapr_irq_msi_init(spapr, spapr->irq->nr_msis); > } > > - spapr->irq->init(spapr, errp); > + if (spapr->irq->xics) { > + Object *obj; > + > + obj = object_new(TYPE_ICS_SPAPR); > + object_property_add_child(OBJECT(spapr), "ics", obj, &local_err); > + if (local_err) { > + goto out; > + } > + > + object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr), > + &local_err); > + if (local_err) { > + goto out; > + } > + > + object_property_set_int(obj, spapr->irq->nr_xirqs, "nr-irqs", > + &local_err); > + if (local_err) { > + goto out; > + } > + > + object_property_set_bool(obj, true, "realized", &local_err); > + if (local_err) { > + goto out; > + } > + > + spapr->ics = ICS_SPAPR(obj); > + } > + > + if (spapr->irq->xive) { > + uint32_t nr_servers = spapr_max_server_number(spapr); > + DeviceState *dev; > + int i; > + > + dev = qdev_create(NULL, TYPE_SPAPR_XIVE); > + qdev_prop_set_uint32(dev, "nr-irqs", > + spapr->irq->nr_xirqs + SPAPR_XIRQ_BASE); > + /* > + * 8 XIVE END structures per CPU. One for each available > + * priority > + */ > + qdev_prop_set_uint32(dev, "nr-ends", nr_servers << 3); > + qdev_init_nofail(dev); > + > + spapr->xive = SPAPR_XIVE(dev); > + > + /* Enable the CPU IPIs */ > + for (i = 0; i < nr_servers; ++i) { > + Error *local_err = NULL; > + > + spapr_xive_irq_claim(spapr->xive, SPAPR_IRQ_IPI + i, false, &local_err); > + if (local_err) { > + goto out; > + } > + } > + > + spapr_xive_hcall_init(spapr); > + } > > spapr->qirqs = qemu_allocate_irqs(spapr->irq->set_irq, spapr, > spapr->irq->nr_xirqs + SPAPR_XIRQ_BASE); > + > +out: > + error_propagate(errp, local_err); > } > > void spapr_irq_claim(SpaprMachineState *spapr, int irq, bool lsi, Error **errp) > @@ -757,7 +744,6 @@ SpaprIrq spapr_irq_xics_legacy = { > .xics = true, > .xive = false, > > - .init = spapr_irq_init_xics, > .claim = spapr_irq_claim_xics, > .free = spapr_irq_free_xics, > .print_info = spapr_irq_print_info_xics, > diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h > index 6816cb0500..fa862c665b 100644 > --- a/include/hw/ppc/spapr_irq.h > +++ b/include/hw/ppc/spapr_irq.h > @@ -42,7 +42,6 @@ typedef struct SpaprIrq { > bool xics; > bool xive; > > - void (*init)(SpaprMachineState *spapr, Error **errp); > void (*claim)(SpaprMachineState *spapr, int irq, bool lsi, Error **errp); > void (*free)(SpaprMachineState *spapr, int irq); > void (*print_info)(SpaprMachineState *spapr, Monitor *mon); > diff --git a/include/hw/ppc/xics_spapr.h b/include/hw/ppc/xics_spapr.h > index 691a6d00f7..267984a97b 100644 > --- a/include/hw/ppc/xics_spapr.h > +++ b/include/hw/ppc/xics_spapr.h > @@ -34,6 +34,7 @@ > #define TYPE_ICS_SPAPR "ics-spapr" > #define ICS_SPAPR(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_SPAPR) > > +void ics_spapr_create(SpaprMachineState *spapr, int nr_xirqs, Error **errp); > void spapr_dt_xics(SpaprMachineState *spapr, uint32_t nr_servers, void *fdt, > uint32_t phandle); > int xics_kvm_connect(SpaprMachineState *spapr, Error **errp);
On Thu, Sep 26, 2019 at 05:35:39PM +0200, Greg Kurz wrote: > On Thu, 26 Sep 2019 09:05:56 +0200 > Cédric Le Goater <clg@kaod.org> wrote: > > > >>> + if (spapr->irq->xive) { > > >>> + uint32_t nr_servers = spapr_max_server_number(spapr); > > >>> + DeviceState *dev; > > >>> + int i; > > >>> + > > >>> + dev = qdev_create(NULL, TYPE_SPAPR_XIVE); > > >>> + qdev_prop_set_uint32(dev, "nr-irqs", > > >>> + spapr->irq->nr_xirqs + SPAPR_XIRQ_BASE); > > >>> + /* > > >>> + * 8 XIVE END structures per CPU. One for each available > > >>> + * priority > > >>> + */ > > >>> + qdev_prop_set_uint32(dev, "nr-ends", nr_servers << 3); > > >>> + qdev_init_nofail(dev); > > >>> + > > >>> + spapr->xive = SPAPR_XIVE(dev); > > >>> + > > >>> + /* Enable the CPU IPIs */ > > >>> + for (i = 0; i < nr_servers; ++i) { > > >>> + Error *local_err = NULL; > > >>> + > > >>> + spapr_xive_irq_claim(spapr->xive, SPAPR_IRQ_IPI + i, false, &local_err); > > >>> + if (local_err) { > > >>> + goto out; > > >>> + } > > >>> + } > > >> > > >> We could move the IPI claim part in the realize routine of SPAPR_XIVE. > > > > > > Yeah, I know. I thought about this, but there's a slight complication > > > in that the XIVE part doesn't know nr_servers directly. There's > > > several possible ways to handle that, but I wasn't 100% happy with any > > > that I came up with yet. > > > > The "nr-ends" property was inappropriate, "nr-servers" would have been > > better and we would have hidden the calculation of ENDs 'nr_servers << 3' > > in the realize routine of SpaprXive. > > > > Yeah it would make sense to have nr_servers within the sPAPR XIVE object, > so that we don't have to pass it when building the FDT node. Same stands > for XICS actually. > > And as part of my current work to reduce HW VP consumption, I also need > nr_servers to pass it to the KVM device. > > > I don't think we can change that without breaking migration though :/ > > > > Hmm... why ? The QOM property is just an interface, it doesn't change the > state. In the end we migrate the same number of XiveEND objects. Yeah, I think it can probably be done. I don't really have the energy left to sort it out for the time being, maybe one day.
On Fri, 27 Sep 2019 15:51:04 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Thu, Sep 26, 2019 at 05:35:39PM +0200, Greg Kurz wrote: > > On Thu, 26 Sep 2019 09:05:56 +0200 > > Cédric Le Goater <clg@kaod.org> wrote: > > > > > >>> + if (spapr->irq->xive) { > > > >>> + uint32_t nr_servers = spapr_max_server_number(spapr); > > > >>> + DeviceState *dev; > > > >>> + int i; > > > >>> + > > > >>> + dev = qdev_create(NULL, TYPE_SPAPR_XIVE); > > > >>> + qdev_prop_set_uint32(dev, "nr-irqs", > > > >>> + spapr->irq->nr_xirqs + SPAPR_XIRQ_BASE); > > > >>> + /* > > > >>> + * 8 XIVE END structures per CPU. One for each available > > > >>> + * priority > > > >>> + */ > > > >>> + qdev_prop_set_uint32(dev, "nr-ends", nr_servers << 3); > > > >>> + qdev_init_nofail(dev); > > > >>> + > > > >>> + spapr->xive = SPAPR_XIVE(dev); > > > >>> + > > > >>> + /* Enable the CPU IPIs */ > > > >>> + for (i = 0; i < nr_servers; ++i) { > > > >>> + Error *local_err = NULL; > > > >>> + > > > >>> + spapr_xive_irq_claim(spapr->xive, SPAPR_IRQ_IPI + i, false, &local_err); > > > >>> + if (local_err) { > > > >>> + goto out; > > > >>> + } > > > >>> + } > > > >> > > > >> We could move the IPI claim part in the realize routine of SPAPR_XIVE. > > > > > > > > Yeah, I know. I thought about this, but there's a slight complication > > > > in that the XIVE part doesn't know nr_servers directly. There's > > > > several possible ways to handle that, but I wasn't 100% happy with any > > > > that I came up with yet. > > > > > > The "nr-ends" property was inappropriate, "nr-servers" would have been > > > better and we would have hidden the calculation of ENDs 'nr_servers << 3' > > > in the realize routine of SpaprXive. > > > > > > > Yeah it would make sense to have nr_servers within the sPAPR XIVE object, > > so that we don't have to pass it when building the FDT node. Same stands > > for XICS actually. > > > > And as part of my current work to reduce HW VP consumption, I also need > > nr_servers to pass it to the KVM device. > > > > > I don't think we can change that without breaking migration though :/ > > > > > > > Hmm... why ? The QOM property is just an interface, it doesn't change the > > state. In the end we migrate the same number of XiveEND objects. > > Yeah, I think it can probably be done. I don't really have the energy > left to sort it out for the time being, maybe one day. > As mentioned above I have another need for "nr-servers", I'll have a look.
On Thu, 26 Sep 2019 17:39:37 +0200 Greg Kurz <groug@kaod.org> wrote: > On Wed, 25 Sep 2019 16:45:34 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > This method is used to set up the interrupt backends for the current > > configuration. However, this means some confusing redirection between > > the "dual" mode init and the init hooks for xics only and xive only modes. > > > > Since we now have simple flags indicating whether XICS and/or XIVE are > > supported, it's easier to just open code each initialization directly in > > spapr_irq_init(). This will also make some future cleanups simpler. > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > --- > > Reviewed-by: Greg Kurz <groug@kaod.org> > Just one nit... > > hw/ppc/spapr_irq.c | 138 ++++++++++++++++-------------------- > > include/hw/ppc/spapr_irq.h | 1 - > > include/hw/ppc/xics_spapr.h | 1 + [...] > > diff --git a/include/hw/ppc/xics_spapr.h b/include/hw/ppc/xics_spapr.h > > index 691a6d00f7..267984a97b 100644 > > --- a/include/hw/ppc/xics_spapr.h > > +++ b/include/hw/ppc/xics_spapr.h > > @@ -34,6 +34,7 @@ > > #define TYPE_ICS_SPAPR "ics-spapr" > > #define ICS_SPAPR(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_SPAPR) > > > > +void ics_spapr_create(SpaprMachineState *spapr, int nr_xirqs, Error **errp); ... this looks like a leftover. > > void spapr_dt_xics(SpaprMachineState *spapr, uint32_t nr_servers, void *fdt, > > uint32_t phandle); > > int xics_kvm_connect(SpaprMachineState *spapr, Error **errp); >
On Fri, Sep 27, 2019 at 04:12:40PM +0200, Greg Kurz wrote: > On Thu, 26 Sep 2019 17:39:37 +0200 > Greg Kurz <groug@kaod.org> wrote: > > > On Wed, 25 Sep 2019 16:45:34 +1000 > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > This method is used to set up the interrupt backends for the current > > > configuration. However, this means some confusing redirection between > > > the "dual" mode init and the init hooks for xics only and xive only modes. > > > > > > Since we now have simple flags indicating whether XICS and/or XIVE are > > > supported, it's easier to just open code each initialization directly in > > > spapr_irq_init(). This will also make some future cleanups simpler. > > > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > > --- > > > > Reviewed-by: Greg Kurz <groug@kaod.org> > > > > Just one nit... > > > > hw/ppc/spapr_irq.c | 138 ++++++++++++++++-------------------- > > > include/hw/ppc/spapr_irq.h | 1 - > > > include/hw/ppc/xics_spapr.h | 1 + > [...] > > > diff --git a/include/hw/ppc/xics_spapr.h b/include/hw/ppc/xics_spapr.h > > > index 691a6d00f7..267984a97b 100644 > > > --- a/include/hw/ppc/xics_spapr.h > > > +++ b/include/hw/ppc/xics_spapr.h > > > @@ -34,6 +34,7 @@ > > > #define TYPE_ICS_SPAPR "ics-spapr" > > > #define ICS_SPAPR(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_SPAPR) > > > > > > +void ics_spapr_create(SpaprMachineState *spapr, int nr_xirqs, Error **errp); > > ... this looks like a leftover. Oops, yes, fixed.
diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c index 073f375ba2..62647dd5a3 100644 --- a/hw/ppc/spapr_irq.c +++ b/hw/ppc/spapr_irq.c @@ -91,27 +91,6 @@ static void spapr_irq_init_kvm(SpaprMachineState *spapr, /* * XICS IRQ backend. */ - -static void spapr_irq_init_xics(SpaprMachineState *spapr, Error **errp) -{ - Object *obj; - Error *local_err = NULL; - - obj = object_new(TYPE_ICS_SPAPR); - object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort); - object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr), - &error_fatal); - object_property_set_int(obj, spapr->irq->nr_xirqs, - "nr-irqs", &error_fatal); - object_property_set_bool(obj, true, "realized", &local_err); - if (local_err) { - error_propagate(errp, local_err); - return; - } - - spapr->ics = ICS_SPAPR(obj); -} - static void spapr_irq_claim_xics(SpaprMachineState *spapr, int irq, bool lsi, Error **errp) { @@ -212,7 +191,6 @@ SpaprIrq spapr_irq_xics = { .xics = true, .xive = false, - .init = spapr_irq_init_xics, .claim = spapr_irq_claim_xics, .free = spapr_irq_free_xics, .print_info = spapr_irq_print_info_xics, @@ -227,37 +205,6 @@ SpaprIrq spapr_irq_xics = { /* * XIVE IRQ backend. */ -static void spapr_irq_init_xive(SpaprMachineState *spapr, Error **errp) -{ - uint32_t nr_servers = spapr_max_server_number(spapr); - DeviceState *dev; - int i; - - dev = qdev_create(NULL, TYPE_SPAPR_XIVE); - qdev_prop_set_uint32(dev, "nr-irqs", - spapr->irq->nr_xirqs + SPAPR_XIRQ_BASE); - /* - * 8 XIVE END structures per CPU. One for each available priority - */ - qdev_prop_set_uint32(dev, "nr-ends", nr_servers << 3); - qdev_init_nofail(dev); - - spapr->xive = SPAPR_XIVE(dev); - - /* Enable the CPU IPIs */ - for (i = 0; i < nr_servers; ++i) { - Error *local_err = NULL; - - spapr_xive_irq_claim(spapr->xive, SPAPR_IRQ_IPI + i, false, &local_err); - if (local_err) { - error_propagate(errp, local_err); - return; - } - } - - spapr_xive_hcall_init(spapr); -} - static void spapr_irq_claim_xive(SpaprMachineState *spapr, int irq, bool lsi, Error **errp) { @@ -361,7 +308,6 @@ SpaprIrq spapr_irq_xive = { .xics = false, .xive = true, - .init = spapr_irq_init_xive, .claim = spapr_irq_claim_xive, .free = spapr_irq_free_xive, .print_info = spapr_irq_print_info_xive, @@ -392,23 +338,6 @@ static SpaprIrq *spapr_irq_current(SpaprMachineState *spapr) &spapr_irq_xive : &spapr_irq_xics; } -static void spapr_irq_init_dual(SpaprMachineState *spapr, Error **errp) -{ - Error *local_err = NULL; - - spapr_irq_xics.init(spapr, &local_err); - if (local_err) { - error_propagate(errp, local_err); - return; - } - - spapr_irq_xive.init(spapr, &local_err); - if (local_err) { - error_propagate(errp, local_err); - return; - } -} - static void spapr_irq_claim_dual(SpaprMachineState *spapr, int irq, bool lsi, Error **errp) { @@ -520,7 +449,6 @@ SpaprIrq spapr_irq_dual = { .xics = true, .xive = true, - .init = spapr_irq_init_dual, .claim = spapr_irq_claim_dual, .free = spapr_irq_free_dual, .print_info = spapr_irq_print_info_dual, @@ -608,8 +536,7 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp) spapr_irq_check(spapr, &local_err); if (local_err) { - error_propagate(errp, local_err); - return; + goto out; } /* Initialize the MSI IRQ allocator. */ @@ -617,10 +544,70 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp) spapr_irq_msi_init(spapr, spapr->irq->nr_msis); } - spapr->irq->init(spapr, errp); + if (spapr->irq->xics) { + Object *obj; + + obj = object_new(TYPE_ICS_SPAPR); + object_property_add_child(OBJECT(spapr), "ics", obj, &local_err); + if (local_err) { + goto out; + } + + object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr), + &local_err); + if (local_err) { + goto out; + } + + object_property_set_int(obj, spapr->irq->nr_xirqs, "nr-irqs", + &local_err); + if (local_err) { + goto out; + } + + object_property_set_bool(obj, true, "realized", &local_err); + if (local_err) { + goto out; + } + + spapr->ics = ICS_SPAPR(obj); + } + + if (spapr->irq->xive) { + uint32_t nr_servers = spapr_max_server_number(spapr); + DeviceState *dev; + int i; + + dev = qdev_create(NULL, TYPE_SPAPR_XIVE); + qdev_prop_set_uint32(dev, "nr-irqs", + spapr->irq->nr_xirqs + SPAPR_XIRQ_BASE); + /* + * 8 XIVE END structures per CPU. One for each available + * priority + */ + qdev_prop_set_uint32(dev, "nr-ends", nr_servers << 3); + qdev_init_nofail(dev); + + spapr->xive = SPAPR_XIVE(dev); + + /* Enable the CPU IPIs */ + for (i = 0; i < nr_servers; ++i) { + Error *local_err = NULL; + + spapr_xive_irq_claim(spapr->xive, SPAPR_IRQ_IPI + i, false, &local_err); + if (local_err) { + goto out; + } + } + + spapr_xive_hcall_init(spapr); + } spapr->qirqs = qemu_allocate_irqs(spapr->irq->set_irq, spapr, spapr->irq->nr_xirqs + SPAPR_XIRQ_BASE); + +out: + error_propagate(errp, local_err); } void spapr_irq_claim(SpaprMachineState *spapr, int irq, bool lsi, Error **errp) @@ -757,7 +744,6 @@ SpaprIrq spapr_irq_xics_legacy = { .xics = true, .xive = false, - .init = spapr_irq_init_xics, .claim = spapr_irq_claim_xics, .free = spapr_irq_free_xics, .print_info = spapr_irq_print_info_xics, diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h index 6816cb0500..fa862c665b 100644 --- a/include/hw/ppc/spapr_irq.h +++ b/include/hw/ppc/spapr_irq.h @@ -42,7 +42,6 @@ typedef struct SpaprIrq { bool xics; bool xive; - void (*init)(SpaprMachineState *spapr, Error **errp); void (*claim)(SpaprMachineState *spapr, int irq, bool lsi, Error **errp); void (*free)(SpaprMachineState *spapr, int irq); void (*print_info)(SpaprMachineState *spapr, Monitor *mon); diff --git a/include/hw/ppc/xics_spapr.h b/include/hw/ppc/xics_spapr.h index 691a6d00f7..267984a97b 100644 --- a/include/hw/ppc/xics_spapr.h +++ b/include/hw/ppc/xics_spapr.h @@ -34,6 +34,7 @@ #define TYPE_ICS_SPAPR "ics-spapr" #define ICS_SPAPR(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_SPAPR) +void ics_spapr_create(SpaprMachineState *spapr, int nr_xirqs, Error **errp); void spapr_dt_xics(SpaprMachineState *spapr, uint32_t nr_servers, void *fdt, uint32_t phandle); int xics_kvm_connect(SpaprMachineState *spapr, Error **errp);
This method is used to set up the interrupt backends for the current configuration. However, this means some confusing redirection between the "dual" mode init and the init hooks for xics only and xive only modes. Since we now have simple flags indicating whether XICS and/or XIVE are supported, it's easier to just open code each initialization directly in spapr_irq_init(). This will also make some future cleanups simpler. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- hw/ppc/spapr_irq.c | 138 ++++++++++++++++-------------------- include/hw/ppc/spapr_irq.h | 1 - include/hw/ppc/xics_spapr.h | 1 + 3 files changed, 63 insertions(+), 77 deletions(-)