| Message ID | 1499274819-15607-12-git-send-email-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
On Wed, Jul 05, 2017 at 07:13:24PM +0200, Cédric Le Goater wrote: > This handler will be used to customize the ouput of the XIVE interrupt > source and presenter objects. I'm not really happy with this without having a clear idea of where this is heading - are you trying to share ICP and or ICS object classes between XICS and XIVE, or will they eventually be separated again? > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > hw/intc/xics.c | 36 ++++++++++++++++++++++++------------ > include/hw/ppc/xics.h | 2 ++ > 2 files changed, 26 insertions(+), 12 deletions(-) > > diff --git a/hw/intc/xics.c b/hw/intc/xics.c > index faa5c631f655..7837c2022b4a 100644 > --- a/hw/intc/xics.c > +++ b/hw/intc/xics.c > @@ -40,18 +40,26 @@ > > void icp_pic_print_info(ICPState *icp, Monitor *mon) > { > + ICPStateClass *k = ICP_GET_CLASS(icp); > 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); > + > + monitor_printf(mon, "CPU %d ", cpu_index); > + if (k->print_info) { > + k->print_info(icp, mon); > + } else { > + monitor_printf(mon, "XIRR=%08x (%p) PP=%02x MFRR=%02x\n", > + icp->xirr, icp->xirr_owner, > + icp->pending_priority, icp->mfrr); > + } > } > > void ics_pic_print_info(ICSState *ics, Monitor *mon) > { > + ICSStateClass *k = ICS_BASE_GET_CLASS(ics); > uint32_t i; > > monitor_printf(mon, "ICS %4x..%4x %p\n", > @@ -61,17 +69,21 @@ void ics_pic_print_info(ICSState *ics, Monitor *mon) > return; > } > > - for (i = 0; i < ics->nr_irqs; i++) { > - ICSIRQState *irq = ics->irqs + i; > + if (k->print_info) { > + k->print_info(ics, mon); > + } else { > + for (i = 0; i < ics->nr_irqs; i++) { > + ICSIRQState *irq = ics->irqs + i; > > - if (!(irq->flags & XICS_FLAGS_IRQ_MASK)) { > - continue; > + 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); > } > } > > diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h > index 28d248abad61..902f3bfd0e33 100644 > --- a/include/hw/ppc/xics.h > +++ b/include/hw/ppc/xics.h > @@ -69,6 +69,7 @@ struct ICPStateClass { > void (*pre_save)(ICPState *icp); > int (*post_load)(ICPState *icp, int version_id); > void (*reset)(ICPState *icp); > + void (*print_info)(ICPState *icp, Monitor *mon); > }; > > struct ICPState { > @@ -119,6 +120,7 @@ struct ICSStateClass { > void (*reject)(ICSState *s, uint32_t irq); > void (*resend)(ICSState *s); > void (*eoi)(ICSState *s, uint32_t irq); > + void (*print_info)(ICSState *s, Monitor *mon); > }; > > struct ICSState {
On 07/24/2017 07:13 AM, David Gibson wrote: > On Wed, Jul 05, 2017 at 07:13:24PM +0200, Cédric Le Goater wrote: >> This handler will be used to customize the ouput of the XIVE interrupt >> source and presenter objects. > > I'm not really happy with this without having a clear idea of where > this is heading - are you trying to share ICP and or ICS object > classes between XICS and XIVE, or will they eventually be separated > again? Because of the XICSFabric interface of the sPAPR machine, we need to use ICPState and ICSState objects. sPAPR is strongly tied to ICPState and it is complex to introduce a new ICPState class for the sPAPR machine. We did introduce a new class in the past but that was for a new machine : PowerNV. So I think we should just add a couple of attributes to ICPState to support XIVE. That is not what the patchset does but I have made progress since with hotplug and migration and came to that conclusion. The consequence is that the print_info() handler is now obsolete for ICPs and we will need to find another way to customize the output. For the interrupt source, the constraints are less strong, adding a new ICSState class seems like a good option and so does the print_info() handler. Thanks, C. >> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> hw/intc/xics.c | 36 ++++++++++++++++++++++++------------ >> include/hw/ppc/xics.h | 2 ++ >> 2 files changed, 26 insertions(+), 12 deletions(-) >> >> diff --git a/hw/intc/xics.c b/hw/intc/xics.c >> index faa5c631f655..7837c2022b4a 100644 >> --- a/hw/intc/xics.c >> +++ b/hw/intc/xics.c >> @@ -40,18 +40,26 @@ >> >> void icp_pic_print_info(ICPState *icp, Monitor *mon) >> { >> + ICPStateClass *k = ICP_GET_CLASS(icp); >> 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); >> + >> + monitor_printf(mon, "CPU %d ", cpu_index); >> + if (k->print_info) { >> + k->print_info(icp, mon); >> + } else { >> + monitor_printf(mon, "XIRR=%08x (%p) PP=%02x MFRR=%02x\n", >> + icp->xirr, icp->xirr_owner, >> + icp->pending_priority, icp->mfrr); >> + } >> } >> >> void ics_pic_print_info(ICSState *ics, Monitor *mon) >> { >> + ICSStateClass *k = ICS_BASE_GET_CLASS(ics); >> uint32_t i; >> >> monitor_printf(mon, "ICS %4x..%4x %p\n", >> @@ -61,17 +69,21 @@ void ics_pic_print_info(ICSState *ics, Monitor *mon) >> return; >> } >> >> - for (i = 0; i < ics->nr_irqs; i++) { >> - ICSIRQState *irq = ics->irqs + i; >> + if (k->print_info) { >> + k->print_info(ics, mon); >> + } else { >> + for (i = 0; i < ics->nr_irqs; i++) { >> + ICSIRQState *irq = ics->irqs + i; >> >> - if (!(irq->flags & XICS_FLAGS_IRQ_MASK)) { >> - continue; >> + 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); >> } >> } >> >> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h >> index 28d248abad61..902f3bfd0e33 100644 >> --- a/include/hw/ppc/xics.h >> +++ b/include/hw/ppc/xics.h >> @@ -69,6 +69,7 @@ struct ICPStateClass { >> void (*pre_save)(ICPState *icp); >> int (*post_load)(ICPState *icp, int version_id); >> void (*reset)(ICPState *icp); >> + void (*print_info)(ICPState *icp, Monitor *mon); >> }; >> >> struct ICPState { >> @@ -119,6 +120,7 @@ struct ICSStateClass { >> void (*reject)(ICSState *s, uint32_t irq); >> void (*resend)(ICSState *s); >> void (*eoi)(ICSState *s, uint32_t irq); >> + void (*print_info)(ICSState *s, Monitor *mon); >> }; >> >> struct ICSState { >
On Mon, Jul 24, 2017 at 03:58:50PM +0200, Cédric Le Goater wrote: > On 07/24/2017 07:13 AM, David Gibson wrote: > > On Wed, Jul 05, 2017 at 07:13:24PM +0200, Cédric Le Goater wrote: > >> This handler will be used to customize the ouput of the XIVE interrupt > >> source and presenter objects. > > > > I'm not really happy with this without having a clear idea of where > > this is heading - are you trying to share ICP and or ICS object > > classes between XICS and XIVE, or will they eventually be separated > > again? > > Because of the XICSFabric interface of the sPAPR machine, we need > to use ICPState and ICSState objects. Yeah, I don't think we do. See further comments elsewhere > sPAPR is strongly tied to ICPState and it is complex to introduce > a new ICPState class for the sPAPR machine. We did introduce a new > class in the past but that was for a new machine : PowerNV. > So I think we should just add a couple of attributes to ICPState > to support XIVE. That is not what the patchset does but I have > made progress since with hotplug and migration and came to that > conclusion. The consequence is that the print_info() handler is > now obsolete for ICPs and we will need to find another way to > customize the output. > > For the interrupt source, the constraints are less strong, adding > a new ICSState class seems like a good option and so does the > print_info() handler. > > Thanks, > > C. > > > >> > >> Signed-off-by: Cédric Le Goater <clg@kaod.org> > >> --- > >> hw/intc/xics.c | 36 ++++++++++++++++++++++++------------ > >> include/hw/ppc/xics.h | 2 ++ > >> 2 files changed, 26 insertions(+), 12 deletions(-) > >> > >> diff --git a/hw/intc/xics.c b/hw/intc/xics.c > >> index faa5c631f655..7837c2022b4a 100644 > >> --- a/hw/intc/xics.c > >> +++ b/hw/intc/xics.c > >> @@ -40,18 +40,26 @@ > >> > >> void icp_pic_print_info(ICPState *icp, Monitor *mon) > >> { > >> + ICPStateClass *k = ICP_GET_CLASS(icp); > >> 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); > >> + > >> + monitor_printf(mon, "CPU %d ", cpu_index); > >> + if (k->print_info) { > >> + k->print_info(icp, mon); > >> + } else { > >> + monitor_printf(mon, "XIRR=%08x (%p) PP=%02x MFRR=%02x\n", > >> + icp->xirr, icp->xirr_owner, > >> + icp->pending_priority, icp->mfrr); > >> + } > >> } > >> > >> void ics_pic_print_info(ICSState *ics, Monitor *mon) > >> { > >> + ICSStateClass *k = ICS_BASE_GET_CLASS(ics); > >> uint32_t i; > >> > >> monitor_printf(mon, "ICS %4x..%4x %p\n", > >> @@ -61,17 +69,21 @@ void ics_pic_print_info(ICSState *ics, Monitor *mon) > >> return; > >> } > >> > >> - for (i = 0; i < ics->nr_irqs; i++) { > >> - ICSIRQState *irq = ics->irqs + i; > >> + if (k->print_info) { > >> + k->print_info(ics, mon); > >> + } else { > >> + for (i = 0; i < ics->nr_irqs; i++) { > >> + ICSIRQState *irq = ics->irqs + i; > >> > >> - if (!(irq->flags & XICS_FLAGS_IRQ_MASK)) { > >> - continue; > >> + 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); > >> } > >> } > >> > >> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h > >> index 28d248abad61..902f3bfd0e33 100644 > >> --- a/include/hw/ppc/xics.h > >> +++ b/include/hw/ppc/xics.h > >> @@ -69,6 +69,7 @@ struct ICPStateClass { > >> void (*pre_save)(ICPState *icp); > >> int (*post_load)(ICPState *icp, int version_id); > >> void (*reset)(ICPState *icp); > >> + void (*print_info)(ICPState *icp, Monitor *mon); > >> }; > >> > >> struct ICPState { > >> @@ -119,6 +120,7 @@ struct ICSStateClass { > >> void (*reject)(ICSState *s, uint32_t irq); > >> void (*resend)(ICSState *s); > >> void (*eoi)(ICSState *s, uint32_t irq); > >> + void (*print_info)(ICSState *s, Monitor *mon); > >> }; > >> > >> struct ICSState { > > >
diff --git a/hw/intc/xics.c b/hw/intc/xics.c index faa5c631f655..7837c2022b4a 100644 --- a/hw/intc/xics.c +++ b/hw/intc/xics.c @@ -40,18 +40,26 @@ void icp_pic_print_info(ICPState *icp, Monitor *mon) { + ICPStateClass *k = ICP_GET_CLASS(icp); 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); + + monitor_printf(mon, "CPU %d ", cpu_index); + if (k->print_info) { + k->print_info(icp, mon); + } else { + monitor_printf(mon, "XIRR=%08x (%p) PP=%02x MFRR=%02x\n", + icp->xirr, icp->xirr_owner, + icp->pending_priority, icp->mfrr); + } } void ics_pic_print_info(ICSState *ics, Monitor *mon) { + ICSStateClass *k = ICS_BASE_GET_CLASS(ics); uint32_t i; monitor_printf(mon, "ICS %4x..%4x %p\n", @@ -61,17 +69,21 @@ void ics_pic_print_info(ICSState *ics, Monitor *mon) return; } - for (i = 0; i < ics->nr_irqs; i++) { - ICSIRQState *irq = ics->irqs + i; + if (k->print_info) { + k->print_info(ics, mon); + } else { + for (i = 0; i < ics->nr_irqs; i++) { + ICSIRQState *irq = ics->irqs + i; - if (!(irq->flags & XICS_FLAGS_IRQ_MASK)) { - continue; + 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); } } diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h index 28d248abad61..902f3bfd0e33 100644 --- a/include/hw/ppc/xics.h +++ b/include/hw/ppc/xics.h @@ -69,6 +69,7 @@ struct ICPStateClass { void (*pre_save)(ICPState *icp); int (*post_load)(ICPState *icp, int version_id); void (*reset)(ICPState *icp); + void (*print_info)(ICPState *icp, Monitor *mon); }; struct ICPState { @@ -119,6 +120,7 @@ struct ICSStateClass { void (*reject)(ICSState *s, uint32_t irq); void (*resend)(ICSState *s); void (*eoi)(ICSState *s, uint32_t irq); + void (*print_info)(ICSState *s, Monitor *mon); }; struct ICSState {
This handler will be used to customize the ouput of the XIVE interrupt source and presenter objects. Signed-off-by: Cédric Le Goater <clg@kaod.org> --- hw/intc/xics.c | 36 ++++++++++++++++++++++++------------ include/hw/ppc/xics.h | 2 ++ 2 files changed, 26 insertions(+), 12 deletions(-)