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

[Julien Grall <julien.grall@linaro.org>]

Hi Vijaya,

Thank you for the patch.

On 04/04/2014 12:56 PM, 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

Honestly, your patch on V1 was far better to read. As you are nearly
modify every functions, you should directly move it to a new file (e.g
merge with patch #9).

> 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;
> +
> +void register_gic_ops(struct gic_hw_operations *ops)

the callback should not change during runtime. Should the prototype
should be:

void register_gic_ops(const struct gic_hw_operations *ops);

> +{
> +    gic_hw_ops = ops;
> +}
> +
> +void update_cpu_lr_mask(void)
> +{
> +    this_cpu(lr_mask) = 0ULL;
> +}
> +
>  static uint8_t nr_lrs;
> -#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))
>  
>  /* The GIC mapping of CPU interfaces does not necessarily match the
>   * logical CPU numbering. Let's use mapping as returned by the GIC
> @@ -89,48 +102,124 @@ static unsigned int gic_cpu_mask(const cpumask_t *cpumask)
>  
>  unsigned int gic_number_lines(void)
>  {
> +    return gic_hw_ops->nr_lines();
> +}
> +
> +static unsigned int gic_nr_lines(void)
> +{
>      return gic.lines;
>  }

If you had directly move the code to another files you would avoid this
kind of non sense name...

>  
> -irq_desc_t *__irq_to_desc(int irq)
> +static unsigned int gic_nr_lrs(void)
>  {
> -    if (irq < NR_LOCAL_IRQS) return &this_cpu(local_irq_desc)[irq];
> -    return &irq_desc[irq-NR_LOCAL_IRQS];

Why did you had to modify __irq_to_desc?

> +    return nr_lrs;
>  }
>  
> -void gic_save_state(struct vcpu *v)
> +static int gic_state_init(struct vcpu *v)
> +{
> +     v->arch.gic_state = xzalloc(struct gic_state_data);
> +     if(!v->arch.gic_state)

if ( !v->arch.gic_state )

> +        return -ENOMEM;
> +     return 0;
> +}
> +
> +static void save_state(struct vcpu *v)
>  {
>      int i;
> -    ASSERT(!local_irq_is_enabled());
>  
>      /* No need for spinlocks here because interrupts are disabled around
>       * this call and it only accesses struct vcpu fields that cannot be
>       * accessed simultaneously by another pCPU.
>       */
> -    for ( i = 0; i < nr_lrs; i++ )
> +    for ( i = 0; i < nr_lrs; i++)

Spurious change

[..]

> @@ -129,6 +149,36 @@ int gic_irq_xlate(const u32 *intspec, unsigned int intsize,
>                    unsigned int *out_hwirq, unsigned int *out_type);
>  void gic_clear_lrs(struct vcpu *v);
>  
> +struct gic_hw_operations {

I made some remarks on v1, that you completely forgot to address on this
structure.

Can you document every callbacks (see asm-arm/platform.h for an
example)? Why do you need them...

> +    struct dt_irq * (*get_maintenance_irq)(void);

With my interrupts patch series dt_irq will be removed soon.

> +    unsigned int (*nr_lines)(void);
> +    unsigned int (*nr_lrs)(void);

I don't think we need a callbacks for both of them. 2 unsigned int are fine.

[..]

> +    void (*enable_irq)(int);
> +    void (*disable_irq)(int);
> +    void (*eoi_irq)(int);
> +    void (*deactivate_irq)(int);
> +    unsigned int (*ack_irq)(void);

I would prefer to introduce a new hw_irq_controller here rather than
adding another abstraction.

> +    void (*set_irq_property)(unsigned int irq, bool_t level,
> +                            const cpumask_t *cpu_mask,
> +                            unsigned int priority);

I would rename the function into set_irq_properties.

[..]

> diff --git a/xen/include/asm-arm/gic_v2_defs.h b/xen/include/asm-arm/gic_v2_defs.h
> index f9ff885..e1bb09c 100644
> --- a/xen/include/asm-arm/gic_v2_defs.h
> +++ b/xen/include/asm-arm/gic_v2_defs.h
> @@ -93,15 +93,6 @@
>  #define GICC_IA_CPU_MASK  0x1c00
>  #define GICC_IA_CPU_SHIFT 10
>  
> -#define GICH_HCR_EN       (1 << 0)
> -#define GICH_HCR_UIE      (1 << 1)
> -#define GICH_HCR_LRENPIE  (1 << 2)
> -#define GICH_HCR_NPIE     (1 << 3)
> -#define GICH_HCR_VGRP0EIE (1 << 4)
> -#define GICH_HCR_VGRP0DIE (1 << 5)
> -#define GICH_HCR_VGRP1EIE (1 << 6)
> -#define GICH_HCR_VGRP1DIE (1 << 7)
> -
>  #define GICH_MISR_EOI     (1 << 0)
>  #define GICH_MISR_U       (1 << 1)
>  #define GICH_MISR_LRENP   (1 << 2)
> @@ -118,9 +109,12 @@
>  #define GICH_LR_STATE_MASK      0x3
>  #define GICH_LR_STATE_SHIFT     28
>  #define GICH_LR_PRIORITY_SHIFT  23
> +#define GICH_LR_PRIORITY_MASK   0x1f
> +#define GICH_LR_HW_SHIFT        31
> +#define GICH_LR_HW_MASK         0x1
> +#define GICH_LR_GRP_SHIFT       30
> +#define GICH_LR_GRP_MASK        0x1
>  #define GICH_LR_MAINTENANCE_IRQ (1<<19)
> -#define GICH_LR_PENDING         (1<<28)
> -#define GICH_LR_ACTIVE          (1<<29)
>  #define GICH_LR_GRP1            (1<<30)
>  #define GICH_LR_HW              (1<<31)
>  #define GICH_LR_CPUID_SHIFT     9

Why did you move all theses define on gic_v2_defs.h, and move back to
gic.h again?

Regards,


-- 
Julien Grall