David Gibson <da...@gibson.dropbear.id.au> writes:
> [ Unknown signature status ]
> On Thu, Jun 23, 2016 at 11:17:25PM +0530, Nikunj A Dadhania wrote:
>> From: Benjamin Herrenschmidt <b...@kernel.crashing.org>
>>
>> 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 <b...@kernel.crashing.org>
>> Signed-off-by: Nikunj A Dadhania <nik...@linux.vnet.ibm.com>
>> ---
>> 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
