Re: [PATCH v2 05/15] xen/arm: segregate GIC low level functionality

[Vijay Kilari <vijay.kilari@gmail.com>]

On Wed, Apr 9, 2014 at 2:20 PM, Julien Grall <julien.grall@linaro.org> wrote:
>
> Hello Vijaya,
>
>
> On 04/04/14 12:56, vijay.kilari@gmail.com wrote:
>>
>> +static void gic_send_sgi(const cpumask_t *cpumask, enum gic_sgi sgi)
>>   {
>>       unsigned int mask = 0;
>>       cpumask_t online_mask;
>> @@ -498,30 +606,26 @@ void send_SGI_mask(const cpumask_t *cpumask, enum
>> gic_sgi sgi)
>>           | sgi;
>>   }
>>
>> -void send_SGI_one(unsigned int cpu, enum gic_sgi sgi)
>> +void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi)
>>   {
>> -    ASSERT(cpu < NR_GIC_CPU_IF);  /* Targets bitmap only supports 8 CPUs
>> */
>> -    send_SGI_mask(cpumask_of(cpu), sgi);
>> +   gic_hw_ops->send_sgi(cpumask, sgi);
>>   }
>>
>> -void send_SGI_self(enum gic_sgi sgi)
>> +void send_SGI_one(unsigned int cpu, enum gic_sgi sgi)
>>   {
>> -    ASSERT(sgi < 16); /* There are only 16 SGIs */
>> -
>> -    dsb(sy);
>> -
>> -    GICD[GICD_SGIR] = GICD_SGI_TARGET_SELF
>> -        | sgi;
>> +    ASSERT(cpu < NR_GIC_CPU_IF);  /* Targets bitmap only supports 8 CPUs
>> */
>> +    send_SGI_mask(cpumask_of(cpu), sgi);
>>   }
>>
>>   void send_SGI_allbutself(enum gic_sgi sgi)
>>   {
>> -   ASSERT(sgi < 16); /* There are only 16 SGIs */
>> +    cpumask_t all_others_mask;
>> +    ASSERT(sgi < 16); /* There are only 16 SGIs */
>>
>> -   dsb(sy);
>> +    cpumask_andnot(&all_others_mask, &cpu_possible_map,
>> cpumask_of(smp_processor_id()));
>>
>> -   GICD[GICD_SGIR] = GICD_SGI_TARGET_OTHERS
>> -       | sgi;
>> +    dsb(sy);
>> +    send_SGI_mask(&all_others_mask, sgi);
>>   }
>
>
> As I said in V1, this change breaks SMP boot with GICv2...
>
> GICD_SGI_TARGERT_OTHERS will send an SGI to every CPUs even if the CPU is
> not yet online (i.e. not registered by Xen). It's used during secondary boot
> (cpu_up_send_sgi).


cpumask_andnot(&all_others_mask,
&cpu_possible_map,cpumask_of(smp_processor_id()));

In my understanding, with the above statement, I am using
cpu_possible_map (all possible cpu's) which should
contains all the cpu possible cpu masks. so this is fine.

The issue could be in gic_send_sgi() call which is always "and" with
cpu_online_map

static void gic_send_sgi(const cpumask_t *cpumask, enum gic_sgi sgi)
{
    unsigned int mask = 0;
    cpumask_t online_mask;

    ASSERT(sgi < 16); /* There are only 16 SGIs */

    cpumask_and(&online_mask, cpumask, &cpu_online_map);
    mask = gic_cpu_mask(&online_mask);

    dsb(sy);

    GICD[GICD_SGIR] = GICD_SGI_TARGET_LIST
        | (mask<<GICD_SGI_TARGET_SHIFT)
        | sgi;
}

I think gic_send_sgi should be passed with absolute mask value
and get rid of and'ing with cpu_online_map

> Your solution won't work because send_SGI_mask only deal with online CPU.
>
> All the changes of send_sgi is more than splitting functions... this should
> be moved on another patch and you should justify the modifications.
>

I will check if I could make this fix in a separate patch.

> [..]
>
>
>>   int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq,
>> @@ -921,10 +1046,10 @@ out:
>>       return retval;
>>   }
>>
>> -static void do_sgi(struct cpu_user_regs *regs, int othercpu, enum gic_sgi
>> sgi)
>> +static void do_sgi(struct cpu_user_regs *regs, enum gic_sgi sgi)
>
>
> Why did you drop the othercpu here?
>
othercpu is not used at all. and also othercpu is computed with
IAR fields #defines which is not required in this generic code.

> Again, please justify every change on the prototype of every functions. If
> it's not trivial, split in small patches...
>
> --
> Julien Grall