Re: [Xen-devel] [PATCH v3 13/16] xen/arm: Add support for GIC v3

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

On Wed, Apr 23, 2014 at 10:54 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> On 04/23/2014 06:01 PM, Ian Campbell wrote:
>>> +    } while ( count-- );
>>> +
>>> +    if ( !count )
>>> +        dprintk(XENLOG_WARNING, "RWP timeout\n");
>>
>> Shouldn't we panic here?
>
> I don't think panic is the solution here. This might be use on secondary
> CPU.
>
> IHMO, we should return an error code here an handle here upper in the
> call stack.
>
>>> +static void gicv3_restore_state(struct vcpu *v)
>>> +{
>>> +    int i;
>>> +
>>> +    for ( i = 0; i < nr_lrs; i++)
>>> +        gich_write_lr(i, v->arch.gic.v3.lr[i]);
>>
>> I wonder if the compiler could do a better job of this using the same
>> switch and fallthrough method you used for aprn regs?
>
> Marc has written a very nice solution for this stuff. It as written an
> assembly function where all registers is save decreasing the number.
>
> When the function, it will calculate the number of regs we don't need of
> LRs save. This will avoid lots of jumps.
>
> See code below (see http://permalink.gmane.org
> /gmane.linux.ports.arm.kernel/310855 for the whole patch):
>
> +       sub     w23, w23, w22   // How many regs we have to skip
> +
> +       adr     x24, 1f
> +       add     x24, x24, x23, lsl #2
> +       br      x24
> +
> +1:
> +       mrs     x20, ICH_LR15_EL2
> +       mrs     x19, ICH_LR14_EL2
> +       mrs     x18, ICH_LR13_EL2
> +       mrs     x17, ICH_LR12_EL2
> +       mrs     x16, ICH_LR11_EL2
> +       mrs     x15, ICH_LR10_EL2
> +       mrs     x14, ICH_LR9_EL2
> +       mrs     x13, ICH_LR8_EL2
> +       mrs     x12, ICH_LR7_EL2
> +       mrs     x11, ICH_LR6_EL2
> +       mrs     x10, ICH_LR5_EL2
> +       mrs     x9, ICH_LR4_EL2
> +       mrs     x8, ICH_LR3_EL2
> +       mrs     x7, ICH_LR2_EL2
> +       mrs     x6, ICH_LR1_EL2
> +       mrs     x5, ICH_LR0_EL2
> +
>
 Instead of having assembly function for store & restore of LR's
How about having one function that stores & restores all 15 LR's
without any checks using READ_SYSREG/WRITE_SYSREG macro's?

For reading specific LR we can use existing switch case function?

> [..]
>
>>> +                return 0;
>>
>> So basically we only know the number of redistributors from device tree
>> and not the mapping from RDIST to CPU, so we have to try every one?
>>
>> Do you have a reference to the GIC v3 Device Tree bindings? I don't see
>> them in linux/Documentation/devicetree/bindings/.
>
> GICv3 is not upstream for now. Marc as sent the V3 recently:
>
> http://comments.gmane.org/gmane.linux.ports.arm.kernel/316902
>
> --
> Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel