Re: [Xen-devel] [PATCH v5 17/21] xen/arm: Add support for GIC v3

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Vijay,

On 12/06/14 14:36, vijay.kilari@xxxxxxxxx wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
>
> Add support for GIC v3 specification System register access(SRE)
> is enabled to access cpu and virtual interface regiseters based
s/regiseters/registers/

[..]

> +    for ( i = 0; i < gicv3.rdist_count; i++ )
> +    {
> +        void __iomem *ptr = gicv3.rdist_regions[i].map_base;
> +
> +        reg = readl_relaxed(ptr + GICR_PIDR2) & GICR_PIDR2_ARCH_MASK;
> +        if ( reg  != (GICV3_GICR_PIDR2 & GICR_PIDR2_ARCH_MASK) )
GICV3_GICR_PIDR2 is used for the emulation. You can't use this value to
test hardware stuff. Please use 0x30 (GICv3) which is clearer.

[..]

> +static void __cpuinit gicv3_hyp_init(void)
> +{
> +    uint32_t vtr;
> +
> +    vtr = READ_SYSREG32(ICH_VTR_EL2);
> +    gicv3_info.nr_lrs  = (vtr & GICH_VTR_NRLRGS) + 1;
> +    gicv3.nr_priorities = ((vtr >> GICH_VTR_PRIBITS_SHIFT) &
> +                          GICH_VTR_PRIBITS_MASK) + 1;
> +
> +    ASSERT((gicv3.nr_priorities > 4 && gicv3.nr_priorities < 8));
For hardware "feature" checking, it's *better* to print a correct error
message than ASSERT (which is not enabled on non-debug build).

[..]

> +static void gicv3_send_sgi(const cpumask_t *cpumask, enum gic_sgi sgi,
> +                           enum gic_sgi_mode mode)
> +{
> +    int cpu = 0;
> +    uint64_t val;
> +
> +    dsb(sy);
Why a dsb here? I don't think it's useful there.

> +    for_each_cpu(cpu, cpumask)
> +    {
> +        uint64_t cluster_id = cpu_logical_map(cpu) & ~0xffUL;
~0xff???? Can't we use a define here?

Or at least a comment explain why this value.

[..]

> +static void gicv3_irq_enable(struct irq_desc *desc)
> +{
> +    unsigned long flags;
> +
We require to call this function with the desc->lock locked. Please add 
an ASSERT here:
ASSERT(spin_is_locked(&desc->lock));

[..]

> +static void gicv3_irq_disable(struct irq_desc *desc)
> +{
> +    unsigned long flags;
ASSERT(spin_is_locked(&desc->lock));


[..]

> diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
> index e81b80e..7aef378 100644
> --- a/xen/include/xen/lib.h
> +++ b/xen/include/xen/lib.h
> @@ -67,6 +67,8 @@ do {                                                          
>   \
>
>   #define reserve_bootmem(_p,_l) ((void)0)
>
> +#define SZ_64K  0x00010000
> +
As said on V4, you have to cc the relevant maintainers for every file 
you've modified... (I didn't cced them this time)
--
Julien Grall

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