Re: [PATCH 1/7] xen/arm: fix rank calculation in vgic_vcpu_inject_irq

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

On Fri, 2012-10-26 at 19:42 +0100, Stefano Stabellini wrote:
> I think that the problem is the usage of vgic_irq_rank with registers
> that have 1 bit per interrupt.

That's very plausible indeed.

> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 3c3983f..92731b6 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -42,13 +42,7 @@
>   */
>  static inline int REG_RANK_NR(int b, uint32_t n)
>  {
> -    switch ( b )
> -    {
> -    case 8: return n >> 3;
> -    case 4: return n >> 2;
> -    case 2: return n >> 1;
> -    default: BUG();
> -    }
> +    return n / b;

All the infrastructure will fall apart if b isn't a power of two, that's
why I used the switch. Can you just add the appropriate case 1 instead?
Probably the bug should be a call to an undefined function to make this
a compile time rather than runtime failure too.

> @@ -577,9 +571,9 @@ struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq)
>  
>  void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq, int virtual)

The changes to this function are all unnecessary, since it was already
consistent within itself. If you want to change it please add a helper
which takes an irq and hides the shifts.

I'd be inclined to rename the existing vgic_irq_rank to vgic_reg_rank
and then call the new helper vgic_irq_rank since that would better
reflect the purpose of both.

>  {
> -    int idx = irq >> 2, byte = irq & 0x3;
> +    int byte = irq & 0x3;

byte isn't actually needed either.

>      uint8_t priority;
> -    struct vgic_irq_rank *rank = vgic_irq_rank(v, 8, idx);
> +    struct vgic_irq_rank *rank = vgic_irq_rank(v, 1, irq / 32);
>      struct pending_irq *iter, *n = irq_to_pending(v, irq);
>      unsigned long flags;
>  
> @@ -587,7 +581,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq, int virtual)
>      if (!list_empty(&n->inflight))
>          return;
>  
> -    priority = byte_read(rank->ipriority[REG_RANK_INDEX(8, idx)], 0, byte);
> +    priority = byte_read(rank->ipriority[REG_RANK_INDEX(8, irq / 4)], 0, byte);
>  
>      n->irq = irq;
>      n->priority = priority;