Re: [RFC PATCH v1 08/10] xen/arm: Add support for GIC v3

[Julien Grall <julien.grall@linaro.org>]

On 03/24/2014 11:26 AM, Ian Campbell wrote:
> On Sun, 2014-03-23 at 14:49 +0000, Julien Grall wrote:
>> Hello Vijay,
>>
>> For next time, can you try to quote only what you need? It's hard to 
>> find your answer in a long mail.
>>
>> On 22/03/14 10:21, Vijay Kilari wrote:
>>> On Thu, Mar 20, 2014 at 10:10 PM, Julien Grall <julien.grall@linaro.org> wrote:
>>>> Please check all the file against CODING_STYLE. I won't shout anymore on
>>>> every coding style error in this mail.
>>> OK. Is there any script to check coding style of Xen?
>>
>> Unfortunately no. It might be interesting to have one for Xen as the 
>> coding style differs from Linux. People are often confusing between them :)
>>
>>>>> +static void save_state(struct vcpu *v)
>>>>> +{
>>>>> +    int i;
>>>>> +    struct gic_state_data *d;
>>>>> +    d = (struct gic_state_data *)v->arch.gic_state;
>>>>> +
>>>>> +    /* No need for spinlocks here because interrupts are disabled around
>>>>> +     * this call and it only accesses struct vcpu fields that cannot be
>>>>> +     * accessed simultaneously by another pCPU.
>>>>> +     */
>>>>> +    for ( i=0; i<nr_lrs; i++)
>>>>> +        d->gic_lr[i] = gich_read_lr(i);
>>>>
>>>> You are introducing a helper to read/write lr. How the compiler handle
>>>> it? Will it optimize?
>>>>
>>>> For me it seems very slow...
>>>    because LR registers are system registers, we have to
>>> use READ/WRITE_SYSREG
>>
>> Is it possible to read all of them (without looking at nr_lrs)?
> 
> What's the concern here? That the compiler doesn't inline and then do
> dead code elimination of gich_read_lr to effectively unroll the loop
> into a series of sysreg reads+memory stores?
> 
>> I see that KVM is using this solution, e.g not looping.
> 
> You mean they've just manually unrolled the loop? That's likely to be as
> much to do with having to write it in asm any way as anything else.
> 
> I'm not saying it isn't worth measuring+optimising this code, but the
> measuring bit should happen first not just guessing what might optimise
> badly (a simple form of "measure" might be "look at the disassembly").

The compiler will likely not optimize this code because nr_lrs is set at
boot time.

-- 
Julien Grall