Re: [Qemu-devel] [PATCH v2 04/22] ppc/xics: add an InterruptStatsProvider interface to ICS and ICP objects

[David Gibson]

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 <address@hidden>

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 = {

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature