Re: [Xen-devel] [PATCH v7 1/5] xen/arm: Add support for GIC v3

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

On Tue, Jul 22, 2014 at 4:09 PM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> On Tue, 2014-07-22 at 15:18 +0530, Vijay Kilari wrote:
>> On Wed, Jul 16, 2014 at 5:11 PM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> 
>> wrote:
>> > On Fri, 2014-07-11 at 18:21 +0530, vijay.kilari@xxxxxxxxx wrote:
>> >> +static inline void gicv3_restore_lrs(int nr_lrs, const struct vcpu *v)
>> >> +static u16 gicv3_compute_target_list(int *base_cpu, const struct cpumask 
>> >> *mask,
>> >> +                                     uint64_t cluster_id)
>> >> +{
>> >> +    int cpu = *base_cpu;
>> >> +    uint64_t mpidr = cpu_logical_map(cpu);
>> >> +    u16 tlist = 0;
>> >> +
>> >> +    while ( cpu < nr_cpu_ids )
>> >> +    {
>> >> +        /*
>> >> +         * If we ever get a cluster of more than 16 CPUs, just
>> >> +         * scream and skip that CPU.
>> >> +         */
>> >> +        if ( (mpidr & 0xff) >= 16 )
>> >
>> > MPIPDR_EFF0_MASK if that's what this is, and at least once more in this
>> > function.
>> >
>> >> +        {
>> >> +            dprintk(XENLOG_WARNING, "GICv3:Cluster with more than 16's 
>> >> cpus\n");
>> >> +            goto out;
>> >
>> > Please validate this and complain in gicv3_init not every time we send
>> > an SGI.
>> >
>>
>>    We can move this check to gicv3_populate_rdist() which is called
>> for each cpu.
>> However we will miss check on mpidr value. But being MPIDR value which does
>> not change it should be ok to remove this check
>
> Why isn't mpidr available at that point?

  I mean check of mpidr value is not done here
(gicv3_compute_target_list()) to skip and
jump to out lable on wrong value of mpidr.

I have added following in gicv3_populate_rdist()

static int __init gicv3_populate_rdist(void)
{
    int i;
    uint32_t aff;
    uint32_t reg;
    uint64_t typer;
    uint64_t mpidr = cpu_logical_map(smp_processor_id());

+    /*
+    * If we ever get a cluster of more than 16 CPUs, just scream.
+     */
+    if ( (mpidr & 0xff) >= 16 )
+          dprintk(XENLOG_WARNING, "GICv3:Cluster with more than 16's cpus\n");



>
>> >> + * Additional registers defined in GIC v3.
>> >> + * Common GICD registers are defined in gic.h
>> >> + */
>> >> +
>> >> +#define GICD_STATUSR                 (0x010)
>> >> [...][
>> >> +#define GICV3_GICD_PIDR0             (0x92)
>> >
>> > What is the distinction between variables with GIC[DR]_ prefixes and
>> > those with GICV3_GIC[DR]_ ones?
>>
>> GICV3 is prefixed for indicating that there are values not the addresses.
>> In anycase I will remove GICV3 prefixes and postfix _VAL
>
> You mean the value used when emulating a read, I think?

   Yes, it is used in gicv3 driver for checking presence of re-distributor
and also in vgicv3 for emulating read.

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