From mboxrd@z Thu Jan  1 00:00:00 1970
From: Julien Grall <julien.grall@linaro.org>
Subject: Re: [PATCH v5 17/21] xen/arm: Add support for GIC v3
Date: Sun, 15 Jun 2014 19:10:49 +0100
Message-ID: <539DE1A9.9030000@linaro.org>
References: <1402580192-13937-1-git-send-email-vijay.kilari@gmail.com>
	<1402580192-13937-18-git-send-email-vijay.kilari@gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1402580192-13937-18-git-send-email-vijay.kilari@gmail.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: vijay.kilari@gmail.com, Ian.Campbell@citrix.com, stefano.stabellini@eu.citrix.com, stefano.stabellini@citrix.com, tim@xen.org, xen-devel@lists.xen.org
Cc: Prasun.Kapoor@caviumnetworks.com, vijaya.kumar@caviumnetworks.com
List-Id: xen-devel@lists.xenproject.org

Hi Vijay,

On 12/06/14 14:36, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>
> 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