Re: [PATCH-4.5 2/4] xen/arm: support HW interrupts in gic_set_lr

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

Hi Stefano,

On 07/02/14 18:56, Stefano Stabellini wrote:
> If the irq to be injected is an hardware irq (p->desc != NULL), set
> GICH_LR_HW.

If you set the GICH_LR_HW, I think you should remove the EOI of physical 
interrupt in maintenance IRQ in this patch. Otherwise we will EOI twice 
and from the documentation the behavior is unpredicatable.

> Also add a struct vcpu* parameter to gic_set_lr.
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>   xen/arch/arm/gic.c |   28 ++++++++++++++++------------
>   1 file changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index acf7195..215b679 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -618,20 +618,24 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
>       return rc;
>   }
>
> -static inline void gic_set_lr(int lr, unsigned int virtual_irq,
> +static inline void gic_set_lr(struct vcpu *v, int lr, unsigned int irq,
>           unsigned int state, unsigned int priority)
>   {
> -    int maintenance_int = GICH_LR_MAINTENANCE_IRQ;
> -    struct pending_irq *p = irq_to_pending(current, virtual_irq);
> +    struct pending_irq *p = irq_to_pending(v, irq);
>
>       BUG_ON(lr >= nr_lrs);
>       BUG_ON(lr < 0);
>       BUG_ON(state & ~(GICH_LR_STATE_MASK<<GICH_LR_STATE_SHIFT));
>
> -    GICH[GICH_LR + lr] = state |
> -        maintenance_int |
> -        ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
> -        ((virtual_irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
> +    if ( p->desc != NULL )
> +        GICH[GICH_LR + lr] = GICH_LR_HW | state | GICH_LR_MAINTENANCE_IRQ |
> +            ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
> +            ((irq & GICH_LR_PHYSICAL_MASK) << GICH_LR_PHYSICAL_SHIFT) |

We should not assume that the physical IRQ == virtual IRQ. You should 
use p->desc->irq

> +            ((irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
> +    else
> +        GICH[GICH_LR + lr] = state | GICH_LR_MAINTENANCE_IRQ |
> +            ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
> +            ((irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);

The final result of virtual IRQ is a subset of the physical IRQ. Can you 
factor the code?

-- 
Julien Grall