Re: [PATCH v5 06/21] xen/arm: segregate and split GIC low level functionality

[Julien Grall <julien.grall@linaro.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