Re: [Xen-devel] [PATCH v4 06/16] xen/arm: segregate and split GIC low level functionality

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Vijay,

On 26/05/14 11:26, vijay.kilari@xxxxxxxxx wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
>
> 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.
whereever? Did you intend to mean "when it's" ?

[..]

> +static void gicv2_enable_irq(struct irq_desc *irqd)
> +{
> +    int irq = irqd->irq;
> +    /* Enable routing */
> +    writel_relaxed((1u << (irq % 32)), GICD + GICD_ISENABLER + (irq / 32) * 4);
> +}
> +
> +static void gicv2_disable_irq(struct irq_desc *irqd)
> +{
> +    int irq = irqd->irq;
> +    /* Disable routing */
> +    writel_relaxed(1u << (irq % 32), GICD + GICD_ICENABLER + (irq / 32) * 4);
> +}
Why do you create gicv2_{enable,disable}_irq ? They are only used in 
gicv2_irq_{enable,disable}. Hence you've create odd names which may 
confuse the developer...
Please inline these 2 functions as the shouldn't be used directly.

[..]

> +static void gicv2_update_lr(int lr, const struct pending_irq *p,
> +                            unsigned int state)
> +{
> +    uint32_t lr_reg;
> +
> +    BUG_ON(lr >= gicv2_info.nr_lrs);
> +    BUG_ON(lr < 0);
> +    BUG_ON(state & ~(GICH_V2_LR_STATE_MASK << GICH_V2_LR_STATE_SHIFT));
Hmmm... why do you validate a value given by the common code with an 
architectural value?
[..]

> -static unsigned int gic_cpu_mask(const cpumask_t *cpumask)
> +static const struct gic_hw_operations *gic_hw_ops;
> +
> +void register_gic_ops(const struct gic_hw_operations *ops)
>   {
> -    unsigned int cpu;
> -    unsigned int mask = 0;
> -    cpumask_t possible_mask;
> +    gic_hw_ops = ops;
> +}
>
> -    cpumask_and(&possible_mask, cpumask, &cpu_possible_map);
> -    for_each_cpu(cpu, &possible_mask)
> -    {
> -        ASSERT(cpu < NR_GIC_CPU_IF);
> -        mask |= per_cpu(gic_cpu_id, cpu);
> -    }
> +void update_cpu_lr_mask(void)
> +{
> +    this_cpu(lr_mask) = 0ULL;
> +}
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?
> -    return mask;
> +int gic_hw_version(void)
> +{
> +   return gic_hw_ops->info->hw_version;
hw_version is an enum gic_version. Why don't you return a enum gic_version?

>   }
>
>   unsigned int gic_number_lines(void)
>   {
> -    return gic.lines;
> +    return gic_hw_ops->info->nr_lines;
>   }
>
>   void gic_save_state(struct vcpu *v)
>   {
> -    int i;
>       ASSERT(!local_irq_is_enabled());
>
> +    if ( is_idle_vcpu(v) )
> +        return;
I though this patch was only code movement... This change doesn't seem 
to be one.
[..]

> @@ -684,28 +317,31 @@ void gic_raise_guest_irq(struct vcpu *v, unsigned int 
> virtual_irq,
>   static void gic_update_one_lr(struct vcpu *v, int i)
>   {
[..]

> -    } else if ( lr & GICH_LR_PENDING ) {
> +    } else if ( lr_val.state & GICH_LR_PENDING ) {
Please respect the coding style...

[..]

>   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;
Hmmm ... why did you create a temporary variable nr_lrs to set lrs just 
after???
>       spin_lock_irqsave(&v->arch.vgic.lock, flags);
>
>       if ( list_empty(&v->arch.vgic.lr_pending) )
> @@ -815,13 +454,15 @@ void gic_clear_pending_irqs(struct vcpu *v)
>
>   int gic_events_need_delivery(void)
>   {
> -    int mask_priority, lrs = nr_lrs;
> +    int mask_priority, lrs;
>       int max_priority = 0xff, active_priority = 0xff;
>       struct vcpu *v = current;
>       struct pending_irq *p;
>       unsigned long flags;
> +    unsigned int nr_lrs = gic_hw_ops->info->nr_lrs;
> +    lrs = nr_lrs;
Same question here.

>   static void do_sgi(struct cpu_user_regs *regs, enum gic_sgi sgi)
>   {
>       /* Lower the priority */
> -    writel_relaxed(sgi, GICC + GICC_EOIR);
> +    struct irq_desc *desc = irq_to_desc(sgi);
Missing newline here.

Also the comment "/* Lower the priority */" should be just before eoi_irq...

> +    gic_hw_ops->eoi_irq(desc);
>
>       switch (sgi)
>       {
> @@ -890,19 +531,17 @@ static void do_sgi(struct cpu_user_regs *regs, enum 
> gic_sgi sgi)
>       }
>
>       /* Deactivate */
> -    writel_relaxed(sgi, GICC + GICC_DIR);
> +    gic_hw_ops->deactivate_irq(sgi);
I will try to send a patch to use desc->handler for SGI. It will avoid 
use to export {eoi,deactivate}_irq callback.
I'm not sure I will have time to do it before this serie will be upstreamed.

>   }
>
>   /* Accept an interrupt from the GIC and dispatch its handler */
>   void gic_interrupt(struct cpu_user_regs *regs, int is_fiq)
>   {
> -    uint32_t intack;
>       unsigned int irq;
>
> -
>       do  {
> -        intack = readl_relaxed(GICC + GICC_IAR);
> -        irq = intack & GICC_IA_IRQ;
> +        /* Reading IRQ will ACK it */
> +        irq = gic_hw_ops->read_irq();
[..]

> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 7fa3b95..f119bcd 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
[..]


> +#define GICH_LR_PENDING         1
> +#define GICH_LR_ACTIVE          2
Ok that what I though.... you didn't test this code on GICv2 otherwise 
you will hit a BUG_ON in gicv2_update_lr. You are checking the value 
against the architectural value which will always fail.
Please either test this code (with the fastmodel if you don't have real 
hardware) or ask someone to test for you.
[..]

> @@ -213,9 +215,9 @@ enum gic_sgi {
>
>   /* SGI irq mode types */
>   enum gic_sgi_mode {
> -    SGI_TARGET_LIST,
> -    SGI_TARGET_OTHERS,
> -    SGI_TARGET_SELF,
> +    SGI_TARGET_LIST   = 0,
> +    SGI_TARGET_OTHERS = 1,
> +    SGI_TARGET_SELF   = 2,
Why did you introduce the enum in patch #3 and change it here? Hence, I 
don't think it's useful to assign a value for each item.
[..]

> +enum gic_version {
> +    GIC_V2 = 2,
Do you really need to assign a value here?

> +};
> +
> +struct gic_info {
> +    /* GIC version */
> +    enum gic_version hw_version;
> +    /* Number of GIC lines supported */
> +    unsigned int nr_lines;
> +    /* Number of LR registers */
> +    unsigned int nr_lrs;
nr_lrs as been changes to uin8_t with Stefano's series.

> +    /* Maintenance irq number */
> +    unsigned int maintenance_irq;
> +};
> +
> +struct gic_hw_operations {
> +    /* Hold GIC HW information */
> +    struct gic_info *info;
const gic_info *info ?

> +    /* hw_irq_controller to enable/disable/eoi host irq */
> +    hw_irq_controller *gic_host_irq_type;
> +
> +    /* hw_irq_controller to enable/disable/eoi guest irq */
> +    hw_irq_controller *gic_guest_irq_type;
It's a shame that we can't constify hw_irq_controller. I will look at it 
later.
> +    /* End of Interrupt */
> +    void (*eoi_irq)(struct irq_desc *irqd);
> +    /* Deactivate/reduce priority of irq */
> +    void (*deactivate_irq)(int);
> +    /* Read IRQ id and Ack */
> +    unsigned int (*read_irq)(void);
> +    /* Set IRQ property */
> +    void (*set_irq_properties)(struct irq_desc *desc,
> +                               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);
see patch #3 for my remark on the type of irqmode.

Regards,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel