Re: [PATCH v2 05/15] xen/arm: segregate GIC low level functionality

[Ian Campbell <Ian.Campbell@citrix.com>]

On Fri, 2014-04-04 at 17:26 +0530, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> GIC low level functionality is segregated into
> separate functions and are called using registered
> callback wherever required.
> 
> This helps to separate generic and hardware functionality
> later
> 
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> ---
>  xen/arch/arm/gic.c                |  362 ++++++++++++++++++++++++++++---------
>  xen/include/asm-arm/gic.h         |   50 +++++
>  xen/include/asm-arm/gic_v2_defs.h |   16 +-
>  3 files changed, 328 insertions(+), 100 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 64699e4..9f03135 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -57,8 +57,21 @@ static irq_desc_t irq_desc[NR_IRQS];
>  static DEFINE_PER_CPU(irq_desc_t[NR_LOCAL_IRQS], local_irq_desc);
>  static DEFINE_PER_CPU(uint64_t, lr_mask);
>  
> +static struct gic_hw_operations *gic_hw_ops;
> +static struct gic_hw_operations gic_ops;

Why two? Is the second one actually gic_v2_ops?

> +void register_gic_ops(struct gic_hw_operations *ops)
> +{
> +    gic_hw_ops = ops;
> +}
> +
> +void update_cpu_lr_mask(void)
> +{
> +    this_cpu(lr_mask) = 0ULL;
> +}

This looks more like init_cpu_lr_mask.

> +
>  static uint8_t nr_lrs;

Shouldn't everywhere be using gic_hw_ops->nr_lrs() rendering this
unused?

> -#define lr_all_full() (this_cpu(lr_mask) == ((1 << nr_lrs) - 1))
> +#define lr_all_full() (this_cpu(lr_mask) == ((1 << gic_hw_ops->nr_lrs()) - 1))

I think nr_lrs can just be an integer field of gic_hw_ops, rather than
using a function to return it, or perhaps a global in gic. set by the
lower level driver somehow. Unless nr_lrs is somehow for gicv3?

> +static void restore_state(struct vcpu *v)

I suspect that all of these callbacks shoujld actually be
gic_v2_restore_state or some such?

> @@ -230,7 +319,7 @@ static hw_irq_controller gic_guest_irq_type = {
>   * - needs to be called with a valid cpu_mask, ie each cpu in the mask has
>   * already called gic_cpu_init
>   */
> -static void gic_set_irq_properties(unsigned int irq, bool_t level,
> +static void gic_set_irq_property(unsigned int irq, bool_t level,
>                                     const cpumask_t *cpu_mask,
>                                     unsigned int priority)

Why did this need to become singular?

> +static struct dt_irq * gic_maintenance_irq(void)
> +{
> +    return &gic.maintenance;
> +}

This isn't using gic_hw_ops, so what is it for?


Ian.