From mboxrd@z Thu Jan  1 00:00:00 1970
From: Julien Grall <julien.grall@linaro.org>
Subject: Re: [PATCH v5 06/21] xen/arm: segregate and split GIC
	low level functionality
Date: Thu, 12 Jun 2014 23:44:58 +0100
Message-ID: <539A2D6A.6020509@linaro.org>
References: <1402580192-13937-1-git-send-email-vijay.kilari@gmail.com>
	<1402580192-13937-7-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-7-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,

In my answer I prefixed every comments I made on V4 but you didn't 
address/answer by "From V4".

I spend lots of time to review carefully every version of series. Please 
do the same by reading carefully and addressing or answering to our 
comments. Thank you.

On 12/06/14 14:36, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>
> GIC driver contains both generic and hardware specific low
> level functionality in gic.c file.
>
> With this patch, low level functionality is moved to separate
> file gic-v2.c and generic code is kept in gic.c file
>
> Callbacks are registered by low level driver with generic driver
> and are called whereever required.

Again, whereever doesn't exist in english. What did you intend to mean? 
"When it's"?

> +static int gicv_v2_init(struct domain *d)
> +{
> +    int ret;
> +
> +    /*
> +     * Domain 0 gets the hardware address.
> +     * Guests get the virtual platform layout.
> +     */
> +    if ( d->domain_id == 0 )

 From v4:
Some of this code (here is one of the example) is modified while you are
sending new version of your patch series.

The original code (i.e in gic.c) was:

if ( is_hardware_domain(d) )
{
         d->arch.vgic.dbase = gicv2.dbase;
         d->arch.vgic.cbase = gicv2.cbase;
}

Now, you've moved the code and we end up to:

if ( d->domain_id == 0 )
{
         d->arch.vgic.dbase = gicv2.dbase;
         d->arch.vgic.cbase = gicv2.cbase;
}

[..]

> +void update_cpu_lr_mask(void)
> +{
> +    this_cpu(lr_mask) = 0ULL;
> +}

 From v4:
The name of this function doesn't match what it does. Hence, it seems 
you only call it during CPU initialization. Why can't you directly call 
in the common function gic_init_secondary_cpu?

[..]


>   static void gic_restore_pending_irqs(struct vcpu *v)
>   {
> -    int lr = 0, lrs = nr_lrs;
> +    int lr = 0, lrs;
>       struct pending_irq *p, *t, *p_r;
>       struct list_head *inflight_r;
>       unsigned long flags;
> +    unsigned int nr_lrs = gic_hw_ops->info->nr_lrs;
>
> +    lrs = nr_lrs;

 From V4:

Hmmm ... why did you create a temporary variable nr_lrs to set lrs just 
after???

-- 
Julien Grall