Re: [PATCH v3 07/16] xen/arm: segregate and split GIC low level functionality

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

Hello Vijaya,

Thank you for the patch.

On 04/15/2014 12:17 PM, vijay.kilari@gmail.com wrote:
> +/* Access to the GIC Distributor registers through the fixmap */
> +#define GICD ((volatile uint32_t *) FIXMAP_ADDR(FIXMAP_GICD))
> +#define GICC ((volatile uint32_t *) FIXMAP_ADDR(FIXMAP_GICC1))
> +#define GICH ((volatile uint32_t *) FIXMAP_ADDR(FIXMAP_GICH))
> +
> +/* Global state */
> +static struct {
> +    paddr_t dbase;       /* Address of distributor registers */
> +    paddr_t cbase;       /* Address of CPU interface registers */
> +    paddr_t hbase;       /* Address of virtual interface registers */
> +    paddr_t vbase;       /* Address of virtual cpu interface registers */
> +    unsigned int lines;  /* Number of interrupts (SPIs + PPIs + SGIs) */
> +    struct dt_irq maintenance; /* IRQ maintenance */
> +    unsigned int cpus;
> +} gic;
> +
> +static struct gic_hw_operations gic_ops;
> +
> +static uint8_t nr_lrs;

As the number of LRs is store in gic_hw_operations, you don't need this one.

> +
> +/* The GIC mapping of CPU interfaces does not necessarily match the
> + * logical CPU numbering. Let's use mapping as returned by the GIC
> + * itself
> + */
> +static DEFINE_PER_CPU(u8, gic_cpu_id);
> +
> +/* Maximum cpu interface per GIC */
> +#define NR_GIC_CPU_IF 8
> +
> +static unsigned int gicv2_cpu_mask(const cpumask_t *cpumask)
> +{
> +    unsigned int cpu;
> +    unsigned int mask = 0;
> +    cpumask_t possible_mask;
> +
> +    cpumask_and(&possible_mask, cpumask, &cpu_possible_map);
> +    for_each_cpu(cpu, &possible_mask)

I know that coding style error was there before ... can you change it to

for_each_cpu( ... )

> +    {
> +        ASSERT(cpu < NR_GIC_CPU_IF);
> +        mask |= per_cpu(gic_cpu_id, cpu);
> +    }
> +
> +    return mask;
> +}

[..]

> +static void gicv2_dump_state(struct vcpu *v)
> +{
> +    int i;
> +    if ( v == current )
> +    {
> +        for ( i = 0; i < nr_lrs; i++ )
> +            printk("   HW_LR[%d]=%x\n", i, GICH[GICH_LR + i]);
> +    } else {

}
else
{

[..]

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

I have a serie which drop dt_struct and changes IRQW routing. It would
be nice if you can rebase your code on top of it now. I don't think it
will change heavily.

See git://xenbits.xen.org/people/julieng/xen-unstable.git branch
interrupt-mgmt-v3

[..]

>  static void gic_irq_enable(struct irq_desc *desc)
>  {
> -    int irq = desc->irq;
>      unsigned long flags;
>  
>      spin_lock_irqsave(&desc->lock, flags);
> @@ -147,20 +112,19 @@ static void gic_irq_enable(struct irq_desc *desc)
>      desc->status &= ~IRQ_DISABLED;
>      dsb();
>      /* Enable routing */
> -    GICD[GICD_ISENABLER + irq / 32] = (1u << (irq % 32));
> +    gic_hw_ops->gic_irq_ops->enable(desc);

This is not the right way to use gic_irq_ops. You should directly
assigned this structure to desc->handler.

I guess you don't do it because you have the desc->status and gic_lock
locking which is common.

IHMO, you have to let the GICv{2,3} drivers handling their own lock for
the hardware access.

>      spin_unlock(&gic_lock);
>      spin_unlock_irqrestore(&desc->lock, flags);
>  }
>  
>  static void gic_irq_disable(struct irq_desc *desc)
>  {
> -    int irq = desc->irq;
>      unsigned long flags;
>  
>      spin_lock_irqsave(&desc->lock, flags);
>      spin_lock(&gic_lock);
>      /* Disable routing */
> -    GICD[GICD_ICENABLER + irq / 32] = (1u << (irq % 32));
> +    gic_hw_ops->gic_irq_ops->disable(desc);
>      desc->status |= IRQ_DISABLED;
>      spin_unlock(&gic_lock);
>      spin_unlock_irqrestore(&desc->lock, flags);

Same here.

> @@ -186,16 +150,15 @@ static void gic_host_irq_end(struct irq_desc *desc)
>  {
>      int irq = desc->irq;
>      /* Lower the priority */
> -    GICC[GICC_EOIR] = irq;
> +    gic_hw_ops->gic_irq_ops->end(desc);
>      /* Deactivate */
> -    GICC[GICC_DIR] = irq;
> +    gic_hw_ops->deactivate_irq(irq);
>  }

>  static void gic_guest_irq_end(struct irq_desc *desc)
>  {
> -    int irq = desc->irq;
>      /* Lower the priority of the IRQ */
> -    GICC[GICC_EOIR] = irq;
> +    gic_hw_ops->gic_irq_ops->end(desc);
>      /* Deactivation happens in maintenance interrupt / via GICV */
>  }

Same here.

> @@ -215,6 +178,7 @@ static hw_irq_controller gic_host_irq_type = {
>      .end = gic_host_irq_end,
>      .set_affinity = gic_irq_set_affinity,
>  };
> +

Spurious change?

>  static hw_irq_controller gic_guest_irq_type = {
>      .typename = "gic",
>      .startup = gic_irq_startup,
> @@ -235,27 +199,7 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level,
>                                     const cpumask_t *cpu_mask,
>                                     unsigned int priority)
>  {
> -    volatile unsigned char *bytereg;
> -    uint32_t cfg, edgebit;
> -    unsigned int mask = gic_cpu_mask(cpu_mask);
> -
> -    /* Set edge / level */
> -    cfg = GICD[GICD_ICFGR + irq / 16];
> -    edgebit = 2u << (2 * (irq % 16));
> -    if ( level )
> -        cfg &= ~edgebit;
> -    else
> -        cfg |= edgebit;
> -    GICD[GICD_ICFGR + irq / 16] = cfg;
> -
> -    /* Set target CPU mask (RAZ/WI on uniprocessor) */
> -    bytereg = (unsigned char *) (GICD + GICD_ITARGETSR);
> -    bytereg[irq] = mask;
> -
> -    /* Set priority */
> -    bytereg = (unsigned char *) (GICD + GICD_IPRIORITYR);
> -    bytereg[irq] = priority;
> -
> +   return gic_hw_ops->set_irq_property(irq, level, cpu_mask, priority);

You don't need to return as gic_set_irq_properties is a void function.
Also why did you drop in plural in the callback name? It should be name
set_irq_properties.

[..]

> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index eba41ee..2387e38 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -43,12 +43,41 @@
>  #define SGI_TARGET_OTHERS  1
>  #define SGI_TARGET_SELF    2
>  
> +#define GICH_LR_PENDING    1
> +#define GICH_LR_ACTIVE     2
> +

Prefixing by GICH_ is confusing. I though we were using it for to set
the value in the hardware. Can you rename it?

> +#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)
> +

You dropped this lines in patch #5 and re-add here.
Please don't drop them on patch #5.

>  #ifndef __ASSEMBLY__
>  #include <xen/device_tree.h>
> +#include <xen/irq.h>
>  
>  #define DT_MATCH_GIC    DT_MATCH_COMPATIBLE("arm,cortex-a15-gic"), \
>                          DT_MATCH_COMPATIBLE("arm,cortex-a7-gic")
>  
> +/*
> + * Decode LR register content and populate below struct.

IHMO, the second part of the description (after the "and") is not useful.

[..]

> +struct gic_hw_operations {
> +    /* GIC version */
> +    enum gic_version hw_version;
> +    /* Number of GIC lines supported */
> +    unsigned int nr_lines;
> +    /* Number of LR registers */
> +    unsigned int nr_lrs;
> +    /* Maintenance irq is derived from dt node. Fetch from gic driver */
> +    struct dt_irq * (*get_maintenance_irq)(void);
> +    /* Save GIC registers */
> +    void (*save_state)(struct vcpu *);
> +    /* Restore GIC registers */
> +    void (*restore_state)(struct vcpu *);

Does restore state will modify the content of vcpu? If not, please add
const.

> +    /* Dump GIC LR register information */
> +    void (*dump_state)(struct vcpu *);

Same question here.

> +    /* Map MMIO region of GIC and get GIC address information */
> +    int (*gicv_setup)(struct domain *);
> +
> +    /* hw_irq_controller for enable/disable/eoi irq */

The hw_irq_controller should contains every callbacks. (see my previous
remark on it).

> +    hw_irq_controller *gic_irq_ops;

const hw_irq_controller *gic_irq_ops;

> +
> +    /* Deactivate/reduce priority of irq */
> +    void (*deactivate_irq)(int);
> +    /* Ack IRQ and return irq id */
> +    unsigned int (*ack_irq)(void);
> +    /* Set IRQ property */
> +    void (*set_irq_property)(unsigned int irq, bool_t level,
> +                            const cpumask_t *cpu_mask,
> +                            unsigned int priority);
> +    /* Send SGI */
> +    void (*send_sgi)(const cpumask_t *online_mask,
> +                         enum gic_sgi sgi, uint8_t irqmode);

Misaligned?

> +    /* Disable CPU physical and virtual interfaces */
> +    void (*disable_interface)(void);
> +    /* Update LR with state and priority */
> +    void (*update_lr)(int lr, struct pending_irq *, unsigned int state);

Update LR won't modify pending_irq, right? If so, please add const.

Also you are mixing named and unamed argument. Can you name every arguments?

> +    /* Update HCR status register */
> +    void (*update_hcr_status)(uint32_t flag, bool_t set);
> +    /* Clear LR register */
> +    void (*clear_lr)(int lr);
> +    /* Read LR register and populate gic_lr structure */
> +    void (*read_lr)(int lr, struct gic_lr *);
> +    /* Write LR register from gic_lr structure */
> +    void (*write_lr)(int lr, struct gic_lr *);

write_lr won't modify pending_irq, right? If so, please add const.

Regards,

-- 
Julien Grall