Re: [Qemu-devel] [PATCH v1 06/11] ppc/xics: Make the ICSState a list

[Nikunj A Dadhania]

David Gibson <address@hidden> writes:

> [ Unknown signature status ]
> On Thu, Jun 23, 2016 at 11:17:25PM +0530, Nikunj A Dadhania wrote:
>> From: Benjamin Herrenschmidt <address@hidden>
>> 
>> Instead of an array of fixed sized blocks, use a list, as we will need
>> to have sources with variable number of interrupts. SPAPR only uses
>> a single entry. Native will create more. If performance becomes an
>> issue we can add some hashed lookup but for now this will do fine.
>> 
>> Signed-off-by: Benjamin Herrenschmidt <address@hidden>
>> Signed-off-by: Nikunj A Dadhania <address@hidden>
>> ---
>>  hw/intc/xics.c        | 78 +++++++++++++++++++++++++++---------------------
>>  hw/intc/xics_kvm.c    | 29 +++++++++++++-----
>>  hw/intc/xics_spapr.c  | 82 
>> +++++++++++++++++++++++++++++----------------------
>>  hw/ppc/spapr_events.c |  2 +-
>>  hw/ppc/spapr_pci.c    |  5 ++--
>>  hw/ppc/spapr_vio.c    |  2 +-
>>  include/hw/ppc/xics.h | 13 ++++----
>>  7 files changed, 124 insertions(+), 87 deletions(-)
>> 
>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
>> index 38e51fc..ef2a1e4 100644
>> --- a/hw/intc/xics.c
>> +++ b/hw/intc/xics.c
>> @@ -96,13 +96,16 @@ void xics_cpu_setup(XICSState *xics, PowerPCCPU *cpu)
>>  static void xics_common_reset(DeviceState *d)
>>  {
>>      XICSState *xics = XICS_COMMON(d);
>> +    ICSState *ics;
>>      int i;
>>  
>>      for (i = 0; i < xics->nr_servers; i++) {
>>          device_reset(DEVICE(&xics->ss[i]));
>>      }
>>  
>> -    device_reset(DEVICE(xics->ics));
>> +    QLIST_FOREACH(ics, &xics->ics, list) {
>> +        device_reset(DEVICE(ics));
>> +    }
>>  }
>>  
>>  static void xics_prop_get_nr_irqs(Object *obj, Visitor *v, const char *name,
>> @@ -134,7 +137,6 @@ static void xics_prop_set_nr_irqs(Object *obj, Visitor 
>> *v, const char *name,
>>      }
>>  
>>      assert(info->set_nr_irqs);
>> -    assert(xics->ics);
>>      info->set_nr_irqs(xics, value, errp);
>>  }
>>  
>> @@ -212,33 +214,35 @@ static void ics_reject(ICSState *ics, int nr);
>>  static void ics_resend(ICSState *ics);
>>  static void ics_eoi(ICSState *ics, int nr);
>>  
>> -static void icp_check_ipi(XICSState *xics, int server)
>> +static void icp_check_ipi(ICPState *ss, int server)
>
> Since you're now passing ICPState, you don't need the server
> parameter, since its only purpose is to locate the right ICPState in
> the XICSState.

Right, i had retained this as there was a trace function using it. Maybe
we can then move the trace to the caller.

>> @@ -361,13 +375,16 @@ int xics_spapr_alloc(XICSState *xics, int src, int 
>> irq_hint, bool lsi,
>>   * Allocate block of consecutive IRQs, and return the number of the first 
>> IRQ in
>>   * the block. If align==true, aligns the first IRQ number to num.
>>   */
>> -int xics_spapr_alloc_block(XICSState *xics, int src, int num, bool lsi,
>> -                           bool align, Error **errp)
>> +int xics_spapr_alloc_block(XICSState *xics, int num, bool lsi, bool align,
>> +                           Error **errp)
>>  {
>> +    ICSState *ics = QLIST_FIRST(&xics->ics);
>>      int i, first = -1;
>> -    ICSState *ics = &xics->ics[src];
>
> Ouch.. AFAICT this would have SEGVed if it was ever called with src != 0.
>
>>  
>> -    assert(src == 0);
>> +    if (!ics) {
>> +        return -1;
>> +    }
>> +
>>      /*
>>       * MSIMesage::data is used for storing VIRQ so
>>       * it has to be aligned to num to support multiple
>> @@ -394,7 +411,7 @@ int xics_spapr_alloc_block(XICSState *xics, int src, int 
>> num, bool lsi,
>>      }
>>      first += ics->offset;
>>  
>> -    trace_xics_alloc_block(src, first, num, lsi, align);
>> +    trace_xics_alloc_block(0, first, num, lsi, align);
>
> You should remove the src from the trave definition since it has to be
> 0.

Sure.

>>  
>>      return first;
>>  }
>> @@ -405,7 +422,7 @@ static void ics_free(ICSState *ics, int srcno, int num)
>>  
>>      for (i = srcno; i < srcno + num; ++i) {
>>          if (ICS_IRQ_FREE(ics, i)) {
>> -            trace_xics_ics_free_warn(ics - ics->xics->ics, i + ics->offset);
>> +            trace_xics_ics_free_warn(0, i + ics->offset);
>
> Likewise here you should remove the always 0 parameter from the trace
> definition.

Sure.

Regards
Nikunj