Re: [Qemu-devel] [RFC PATCH 11/26] ppc/xics: introduce a print_info() handler to the ICS and ICP objects

["Cédric Le Goater" <clg@kaod.org>]

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