Re: [PATCH-4.5 3/4] xen/arm: do not request maintenance_interrupts

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

Hi Stefano,

On 07/02/14 18:56, Stefano Stabellini wrote:
> Do not set GICH_LR_MAINTENANCE_IRQ for every interrupt with set in the
> GICH_LR registers.
>
> Introduce a new function, gic_clear_lrs, that goes over the GICH_LR
> registers, clear the invalid ones and free the corresponding interrupts
> from the inflight queue if appropriate. Add the interrupt to lr_pending
> if the GIC_IRQ_GUEST_PENDING is still set.
>
> Call gic_clear_lrs from gic_restore_state and on return to guest
> (gic_inject).
>
> Remove the now unused code in maintenance_interrupts and gic_irq_eoi.
>
> In vgic_vcpu_inject_irq, if the target is a vcpu running on another cpu,
> send and SGI to it to interrupt it and force it to clear the old LRs.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>   xen/arch/arm/gic.c  |  126 ++++++++++++++++++++++-----------------------------
>   xen/arch/arm/vgic.c |    3 +-
>   2 files changed, 56 insertions(+), 73 deletions(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 215b679..87bd5d3 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> +static void gic_clear_lrs(struct vcpu *v)
> +{
> +    struct pending_irq *p;
> +    int i = 0, irq;
> +    uint32_t lr;
> +    bool_t inflight;
> +
> +    ASSERT(!local_irq_is_enabled());
> +
> +    while ((i = find_next_bit((const long unsigned int *) &this_cpu(lr_mask),
> +                              nr_lrs, i)) < nr_lrs) {
> +        lr = GICH[GICH_LR + i];
> +        if ( !(lr & (GICH_LR_PENDING|GICH_LR_ACTIVE)) )
> +        {
> +            if ( lr & GICH_LR_HW )
> +                irq = (lr >> GICH_LR_PHYSICAL_SHIFT) & GICH_LR_PHYSICAL_MASK;
> +            else
> +                irq = (lr >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK;
> +

The if sentence can be simply by:

irq = (lr >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK;


> +            inflight = 0;
> +            GICH[GICH_LR + i] = 0;
> +            clear_bit(i, &this_cpu(lr_mask));
> +
> +            spin_lock(&gic.lock);
> +            p = irq_to_pending(v, irq);
> +            if ( p->desc != NULL )
> +                p->desc->status &= ~IRQ_INPROGRESS;
> +            clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
> +            if ( test_bit(GIC_IRQ_GUEST_PENDING, &p->status) &&
> +                    test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
> +            {

I would add a WARN_ON(p->desc != NULL) here. AFAIK, this code path 
shouldn't be used for physical IRQ.

-- 
Julien Grall