| Message ID | 1487252865-12064-5-git-send-email-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
On Thu, Feb 16, 2017 at 02:47:27PM +0100, Cédric Le Goater wrote: > This is, again, to reduce the use of the list of ICS objects. Let's > make each individual ICS and ICP object an InterruptStatsProvider and > remove this same interface from XICSState. > > Signed-off-by: Cédric Le Goater <clg@kaod.org> I'm a little hesitant about this, because it means that getting the interrupt stats information is now spread out over the qom tree, whereas previously there was a single location to get a good summary of the systems overall interrupt status. The previous behaviour seems like it would be more convenient for debugging. That said, I see the structural advantages of this split. Hmm.. still thinking.. > --- > hw/intc/xics.c | 76 +++++++++++++++++++++++++++++++--------------------------- > 1 file changed, 41 insertions(+), 35 deletions(-) > > diff --git a/hw/intc/xics.c b/hw/intc/xics.c > index 9f22814815c9..b1294417a0ae 100644 > --- a/hw/intc/xics.c > +++ b/hw/intc/xics.c > @@ -92,44 +92,44 @@ void xics_cpu_setup(XICSState *xics, PowerPCCPU *cpu) > } > } > > -static void xics_common_pic_print_info(InterruptStatsProvider *obj, > - Monitor *mon) > +static void icp_pic_print_info(InterruptStatsProvider *obj, > + Monitor *mon) > { > - XICSState *xics = XICS_COMMON(obj); > - ICSState *ics; > + ICPState *icp = ICP(obj); > + int cpu_index = icp->cs ? icp->cs->cpu_index : -1; > + > + if (!icp->output) { > + return; > + } > + monitor_printf(mon, "CPU %d XIRR=%08x (%p) PP=%02x MFRR=%02x\n", > + cpu_index, icp->xirr, icp->xirr_owner, > + icp->pending_priority, icp->mfrr); > +} > + > +static void ics_simple_pic_print_info(InterruptStatsProvider *obj, > + Monitor *mon) > +{ > + ICSState *ics = ICS_SIMPLE(obj); > uint32_t i; > > - for (i = 0; i < xics->nr_servers; i++) { > - ICPState *icp = &xics->ss[i]; > + monitor_printf(mon, "ICS %4x..%4x %p\n", > + ics->offset, ics->offset + ics->nr_irqs - 1, ics); > > - if (!icp->output) { > - continue; > - } > - monitor_printf(mon, "CPU %d XIRR=%08x (%p) PP=%02x MFRR=%02x\n", > - i, icp->xirr, icp->xirr_owner, > - icp->pending_priority, icp->mfrr); > + if (!ics->irqs) { > + return; > } > > - QLIST_FOREACH(ics, &xics->ics, list) { > - monitor_printf(mon, "ICS %4x..%4x %p\n", > - ics->offset, ics->offset + ics->nr_irqs - 1, ics); > + for (i = 0; i < ics->nr_irqs; i++) { > + ICSIRQState *irq = ics->irqs + i; > > - if (!ics->irqs) { > + if (!(irq->flags & XICS_FLAGS_IRQ_MASK)) { > continue; > } > - > - for (i = 0; i < ics->nr_irqs; i++) { > - ICSIRQState *irq = ics->irqs + i; > - > - if (!(irq->flags & XICS_FLAGS_IRQ_MASK)) { > - continue; > - } > - monitor_printf(mon, " %4x %s %02x %02x\n", > - ics->offset + i, > - (irq->flags & XICS_FLAGS_IRQ_LSI) ? > - "LSI" : "MSI", > - irq->priority, irq->status); > - } > + monitor_printf(mon, " %4x %s %02x %02x\n", > + ics->offset + i, > + (irq->flags & XICS_FLAGS_IRQ_LSI) ? > + "LSI" : "MSI", > + irq->priority, irq->status); > } > } > > @@ -161,10 +161,8 @@ static void xics_common_initfn(Object *obj) > static void xics_common_class_init(ObjectClass *oc, void *data) > { > DeviceClass *dc = DEVICE_CLASS(oc); > - InterruptStatsProviderClass *ic = INTERRUPT_STATS_PROVIDER_CLASS(oc); > > dc->reset = xics_common_reset; > - ic->print_info = xics_common_pic_print_info; > } > > static const TypeInfo xics_common_info = { > @@ -174,10 +172,6 @@ static const TypeInfo xics_common_info = { > .class_size = sizeof(XICSStateClass), > .instance_init = xics_common_initfn, > .class_init = xics_common_class_init, > - .interfaces = (InterfaceInfo[]) { > - { TYPE_INTERRUPT_STATS_PROVIDER }, > - { } > - }, > }; > > /* > @@ -414,10 +408,12 @@ static void icp_realize(DeviceState *dev, Error **errp) > static void icp_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > + InterruptStatsProviderClass *ic = INTERRUPT_STATS_PROVIDER_CLASS(klass); > > dc->reset = icp_reset; > dc->vmsd = &vmstate_icp_server; > dc->realize = icp_realize; > + ic->print_info = icp_pic_print_info; > } > > static const TypeInfo icp_info = { > @@ -426,6 +422,10 @@ static const TypeInfo icp_info = { > .instance_size = sizeof(ICPState), > .class_init = icp_class_init, > .class_size = sizeof(ICPStateClass), > + .interfaces = (InterfaceInfo[]) { > + { TYPE_INTERRUPT_STATS_PROVIDER }, > + { } > + }, > }; > > /* > @@ -692,6 +692,7 @@ static void ics_simple_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > ICSStateClass *isc = ICS_BASE_CLASS(klass); > + InterruptStatsProviderClass *ic = INTERRUPT_STATS_PROVIDER_CLASS(klass); > > dc->realize = ics_simple_realize; > dc->props = ics_simple_properties; > @@ -701,6 +702,7 @@ static void ics_simple_class_init(ObjectClass *klass, void *data) > isc->reject = ics_simple_reject; > isc->resend = ics_simple_resend; > isc->eoi = ics_simple_eoi; > + ic->print_info = ics_simple_pic_print_info; > } > > static const TypeInfo ics_simple_info = { > @@ -710,6 +712,10 @@ static const TypeInfo ics_simple_info = { > .class_init = ics_simple_class_init, > .class_size = sizeof(ICSStateClass), > .instance_init = ics_simple_initfn, > + .interfaces = (InterfaceInfo[]) { > + { TYPE_INTERRUPT_STATS_PROVIDER }, > + { } > + }, > }; > > static const TypeInfo ics_base_info = {
On 02/23/2017 03:15 AM, David Gibson wrote: > On Thu, Feb 16, 2017 at 02:47:27PM +0100, Cédric Le Goater wrote: >> This is, again, to reduce the use of the list of ICS objects. Let's >> make each individual ICS and ICP object an InterruptStatsProvider and >> remove this same interface from XICSState. >> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> > > I'm a little hesitant about this, because it means that getting the > interrupt stats information is now spread out over the qom tree, > whereas previously there was a single location to get a good summary > of the systems overall interrupt status. The previous behaviour seems > like it would be more convenient for debugging. > > That said, I see the structural advantages of this split. Hmm.. still > thinking.. This is true. Another argument in favour of what you are saying is the order in which these are printed. See below. What we could do after the cleanup is to make the machine an InterruptStatsProvider to clarify things. Thanks, C. (qemu) info pic CPU 18 XIRR=00000000 ((nil)) PP=ff MFRR=ff CPU 0 XIRR=00000000 ((nil)) PP=ff MFRR=ff CPU 21 XIRR=00000000 ((nil)) PP=ff MFRR=ff CPU 2 XIRR=00000000 ((nil)) PP=ff MFRR=ff CPU 23 XIRR=00000000 ((nil)) PP=ff MFRR=ff CPU 4 XIRR=00000000 ((nil)) PP=ff MFRR=ff CPU 11 XIRR=00000000 ((nil)) PP=ff MFRR=ff CPU 25 XIRR=00000000 ((nil)) PP=ff MFRR=ff CPU 6 XIRR=00000000 ((nil)) PP=ff MFRR=ff CPU 13 XIRR=00000000 ((nil)) PP=ff MFRR=ff CPU 27 XIRR=00000000 ((nil)) PP=ff MFRR=ff CPU 8 XIRR=00000000 ((nil)) PP=ff MFRR=ff CPU 15 XIRR=00000000 ((nil)) PP=ff MFRR=ff CPU 29 XIRR=00000000 ((nil)) PP=ff MFRR=ff CPU 30 XIRR=00000000 ((nil)) PP=ff MFRR=ff CPU 17 XIRR=00000000 ((nil)) PP=ff MFRR=ff CPU 20 XIRR=00000000 ((nil)) PP=ff MFRR=ff CPU 19 XIRR=00000000 ((nil)) PP=ff MFRR=ff CPU 1 XIRR=00000000 ((nil)) PP=ff MFRR=ff ICS 1000..13ff 0x10018f1ea40 1000 MSI ff 00 1001 MSI ff 00 1002 MSI ff 00 1003 MSI ff 00 1004 LSI ff 00 1005 LSI ff 00 1006 LSI ff 00 1007 LSI ff 00 1008 MSI ff 00 1009 MSI ff 00 100a MSI ff 00 100b MSI ff 00 100c MSI ff 00 CPU 22 XIRR=00000000 ((nil)) PP=ff MFRR=ff CPU 3 XIRR=00000000 ((nil)) PP=ff MFRR=ff CPU 10 XIRR=00000000 ((nil)) PP=ff MFRR=ff CPU 24 XIRR=00000000 ((nil)) PP=ff MFRR=ff CPU 5 XIRR=00000000 ((nil)) PP=ff MFRR=ff CPU 12 XIRR=00000000 ((nil)) PP=ff MFRR=ff CPU 26 XIRR=00000000 ((nil)) PP=ff MFRR=ff CPU 7 XIRR=00000000 ((nil)) PP=ff MFRR=ff CPU 14 XIRR=00000000 ((nil)) PP=ff MFRR=ff CPU 28 XIRR=00000000 ((nil)) PP=ff MFRR=ff CPU 9 XIRR=00000000 ((nil)) PP=ff MFRR=ff CPU 16 XIRR=00000000 ((nil)) PP=ff MFRR=ff CPU 31 XIRR=00000000 ((nil)) PP=ff MFRR=ff
On Fri, Feb 24, 2017 at 11:52:01AM +0100, Cédric Le Goater wrote: > On 02/23/2017 03:15 AM, David Gibson wrote: > > On Thu, Feb 16, 2017 at 02:47:27PM +0100, Cédric Le Goater wrote: > >> This is, again, to reduce the use of the list of ICS objects. Let's > >> make each individual ICS and ICP object an InterruptStatsProvider and > >> remove this same interface from XICSState. > >> > >> Signed-off-by: Cédric Le Goater <clg@kaod.org> > > > > I'm a little hesitant about this, because it means that getting the > > interrupt stats information is now spread out over the qom tree, > > whereas previously there was a single location to get a good summary > > of the systems overall interrupt status. The previous behaviour seems > > like it would be more convenient for debugging. > > > > That said, I see the structural advantages of this split. Hmm.. still > > thinking.. > > This is true. Another argument in favour of what you are saying > is the order in which these are printed. See below. > > What we could do after the cleanup is to make the machine an > InterruptStatsProvider to clarify things. Right. So "info pic" does at least iterate through all the providers, but the semi-random order is pretty icky. I think putting the stats provider on the machine would be a better idea - I guess it should be easy enough if the xics code provides a helper. > > Thanks, > > C. > > > (qemu) info pic > CPU 18 XIRR=00000000 ((nil)) PP=ff MFRR=ff > CPU 0 XIRR=00000000 ((nil)) PP=ff MFRR=ff > CPU 21 XIRR=00000000 ((nil)) PP=ff MFRR=ff > CPU 2 XIRR=00000000 ((nil)) PP=ff MFRR=ff > CPU 23 XIRR=00000000 ((nil)) PP=ff MFRR=ff > CPU 4 XIRR=00000000 ((nil)) PP=ff MFRR=ff > CPU 11 XIRR=00000000 ((nil)) PP=ff MFRR=ff > CPU 25 XIRR=00000000 ((nil)) PP=ff MFRR=ff > CPU 6 XIRR=00000000 ((nil)) PP=ff MFRR=ff > CPU 13 XIRR=00000000 ((nil)) PP=ff MFRR=ff > CPU 27 XIRR=00000000 ((nil)) PP=ff MFRR=ff > CPU 8 XIRR=00000000 ((nil)) PP=ff MFRR=ff > CPU 15 XIRR=00000000 ((nil)) PP=ff MFRR=ff > CPU 29 XIRR=00000000 ((nil)) PP=ff MFRR=ff > CPU 30 XIRR=00000000 ((nil)) PP=ff MFRR=ff > CPU 17 XIRR=00000000 ((nil)) PP=ff MFRR=ff > CPU 20 XIRR=00000000 ((nil)) PP=ff MFRR=ff > CPU 19 XIRR=00000000 ((nil)) PP=ff MFRR=ff > CPU 1 XIRR=00000000 ((nil)) PP=ff MFRR=ff > ICS 1000..13ff 0x10018f1ea40 > 1000 MSI ff 00 > 1001 MSI ff 00 > 1002 MSI ff 00 > 1003 MSI ff 00 > 1004 LSI ff 00 > 1005 LSI ff 00 > 1006 LSI ff 00 > 1007 LSI ff 00 > 1008 MSI ff 00 > 1009 MSI ff 00 > 100a MSI ff 00 > 100b MSI ff 00 > 100c MSI ff 00 > CPU 22 XIRR=00000000 ((nil)) PP=ff MFRR=ff > CPU 3 XIRR=00000000 ((nil)) PP=ff MFRR=ff > CPU 10 XIRR=00000000 ((nil)) PP=ff MFRR=ff > CPU 24 XIRR=00000000 ((nil)) PP=ff MFRR=ff > CPU 5 XIRR=00000000 ((nil)) PP=ff MFRR=ff > CPU 12 XIRR=00000000 ((nil)) PP=ff MFRR=ff > CPU 26 XIRR=00000000 ((nil)) PP=ff MFRR=ff > CPU 7 XIRR=00000000 ((nil)) PP=ff MFRR=ff > CPU 14 XIRR=00000000 ((nil)) PP=ff MFRR=ff > CPU 28 XIRR=00000000 ((nil)) PP=ff MFRR=ff > CPU 9 XIRR=00000000 ((nil)) PP=ff MFRR=ff > CPU 16 XIRR=00000000 ((nil)) PP=ff MFRR=ff > CPU 31 XIRR=00000000 ((nil)) PP=ff MFRR=ff > >
On 02/27/2017 12:43 AM, David Gibson wrote: > On Fri, Feb 24, 2017 at 11:52:01AM +0100, Cédric Le Goater wrote: >> On 02/23/2017 03:15 AM, David Gibson wrote: >>> On Thu, Feb 16, 2017 at 02:47:27PM +0100, Cédric Le Goater wrote: >>>> This is, again, to reduce the use of the list of ICS objects. Let's >>>> make each individual ICS and ICP object an InterruptStatsProvider and >>>> remove this same interface from XICSState. >>>> >>>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>> >>> I'm a little hesitant about this, because it means that getting the >>> interrupt stats information is now spread out over the qom tree, >>> whereas previously there was a single location to get a good summary >>> of the systems overall interrupt status. The previous behaviour seems >>> like it would be more convenient for debugging. >>> >>> That said, I see the structural advantages of this split. Hmm.. still >>> thinking.. >> >> This is true. Another argument in favour of what you are saying >> is the order in which these are printed. See below. >> >> What we could do after the cleanup is to make the machine an >> InterruptStatsProvider to clarify things. > > Right. So "info pic" does at least iterate through all the providers, > but the semi-random order is pretty icky. I think putting the stats > provider on the machine would be a better idea - I guess it should be > easy enough if the xics code provides a helper. OK. This is on my TODO list for the next version of the patchset but as a followup patch. I don't want to change too much the initial cleanups as he took me a while to find a working path to redo XICS. Thanks, C.
diff --git a/hw/intc/xics.c b/hw/intc/xics.c index 9f22814815c9..b1294417a0ae 100644 --- a/hw/intc/xics.c +++ b/hw/intc/xics.c @@ -92,44 +92,44 @@ void xics_cpu_setup(XICSState *xics, PowerPCCPU *cpu) } } -static void xics_common_pic_print_info(InterruptStatsProvider *obj, - Monitor *mon) +static void icp_pic_print_info(InterruptStatsProvider *obj, + Monitor *mon) { - XICSState *xics = XICS_COMMON(obj); - ICSState *ics; + ICPState *icp = ICP(obj); + int cpu_index = icp->cs ? icp->cs->cpu_index : -1; + + if (!icp->output) { + return; + } + monitor_printf(mon, "CPU %d XIRR=%08x (%p) PP=%02x MFRR=%02x\n", + cpu_index, icp->xirr, icp->xirr_owner, + icp->pending_priority, icp->mfrr); +} + +static void ics_simple_pic_print_info(InterruptStatsProvider *obj, + Monitor *mon) +{ + ICSState *ics = ICS_SIMPLE(obj); uint32_t i; - for (i = 0; i < xics->nr_servers; i++) { - ICPState *icp = &xics->ss[i]; + monitor_printf(mon, "ICS %4x..%4x %p\n", + ics->offset, ics->offset + ics->nr_irqs - 1, ics); - if (!icp->output) { - continue; - } - monitor_printf(mon, "CPU %d XIRR=%08x (%p) PP=%02x MFRR=%02x\n", - i, icp->xirr, icp->xirr_owner, - icp->pending_priority, icp->mfrr); + if (!ics->irqs) { + return; } - QLIST_FOREACH(ics, &xics->ics, list) { - monitor_printf(mon, "ICS %4x..%4x %p\n", - ics->offset, ics->offset + ics->nr_irqs - 1, ics); + for (i = 0; i < ics->nr_irqs; i++) { + ICSIRQState *irq = ics->irqs + i; - if (!ics->irqs) { + if (!(irq->flags & XICS_FLAGS_IRQ_MASK)) { continue; } - - for (i = 0; i < ics->nr_irqs; i++) { - ICSIRQState *irq = ics->irqs + i; - - if (!(irq->flags & XICS_FLAGS_IRQ_MASK)) { - continue; - } - monitor_printf(mon, " %4x %s %02x %02x\n", - ics->offset + i, - (irq->flags & XICS_FLAGS_IRQ_LSI) ? - "LSI" : "MSI", - irq->priority, irq->status); - } + monitor_printf(mon, " %4x %s %02x %02x\n", + ics->offset + i, + (irq->flags & XICS_FLAGS_IRQ_LSI) ? + "LSI" : "MSI", + irq->priority, irq->status); } } @@ -161,10 +161,8 @@ static void xics_common_initfn(Object *obj) static void xics_common_class_init(ObjectClass *oc, void *data) { DeviceClass *dc = DEVICE_CLASS(oc); - InterruptStatsProviderClass *ic = INTERRUPT_STATS_PROVIDER_CLASS(oc); dc->reset = xics_common_reset; - ic->print_info = xics_common_pic_print_info; } static const TypeInfo xics_common_info = { @@ -174,10 +172,6 @@ static const TypeInfo xics_common_info = { .class_size = sizeof(XICSStateClass), .instance_init = xics_common_initfn, .class_init = xics_common_class_init, - .interfaces = (InterfaceInfo[]) { - { TYPE_INTERRUPT_STATS_PROVIDER }, - { } - }, }; /* @@ -414,10 +408,12 @@ static void icp_realize(DeviceState *dev, Error **errp) static void icp_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); + InterruptStatsProviderClass *ic = INTERRUPT_STATS_PROVIDER_CLASS(klass); dc->reset = icp_reset; dc->vmsd = &vmstate_icp_server; dc->realize = icp_realize; + ic->print_info = icp_pic_print_info; } static const TypeInfo icp_info = { @@ -426,6 +422,10 @@ static const TypeInfo icp_info = { .instance_size = sizeof(ICPState), .class_init = icp_class_init, .class_size = sizeof(ICPStateClass), + .interfaces = (InterfaceInfo[]) { + { TYPE_INTERRUPT_STATS_PROVIDER }, + { } + }, }; /* @@ -692,6 +692,7 @@ static void ics_simple_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); ICSStateClass *isc = ICS_BASE_CLASS(klass); + InterruptStatsProviderClass *ic = INTERRUPT_STATS_PROVIDER_CLASS(klass); dc->realize = ics_simple_realize; dc->props = ics_simple_properties; @@ -701,6 +702,7 @@ static void ics_simple_class_init(ObjectClass *klass, void *data) isc->reject = ics_simple_reject; isc->resend = ics_simple_resend; isc->eoi = ics_simple_eoi; + ic->print_info = ics_simple_pic_print_info; } static const TypeInfo ics_simple_info = { @@ -710,6 +712,10 @@ static const TypeInfo ics_simple_info = { .class_init = ics_simple_class_init, .class_size = sizeof(ICSStateClass), .instance_init = ics_simple_initfn, + .interfaces = (InterfaceInfo[]) { + { TYPE_INTERRUPT_STATS_PROVIDER }, + { } + }, }; static const TypeInfo ics_base_info = {
This is, again, to reduce the use of the list of ICS objects. Let's make each individual ICS and ICP object an InterruptStatsProvider and remove this same interface from XICSState. Signed-off-by: Cédric Le Goater <clg@kaod.org> --- hw/intc/xics.c | 76 +++++++++++++++++++++++++++++++--------------------------- 1 file changed, 41 insertions(+), 35 deletions(-)