Re: [PATCH] xen/arm: implement GICD_ICACTIVER read/write

[Julien Grall <julien.grall@citrix.com>]

Hi Stefano,

On 25/11/15 16:40, Stefano Stabellini wrote:
> Implement GICD_ICACTIVER and GICD_ISACTIVER reads by looking for the
> GIC_IRQ_GUEST_ACTIVE bit in the relevant struct pending_irq. However
> given that the pending to active transaction for irqs in LRs in done in
> hardware, the GIC_IRQ_GUEST_ACTIVE bit might be out of date. We'll have
> to live with that.

Is it acceptable? The guest may rely on it to save the active state of
an IRQ.

> Implement GICD_ICACTIVER writes by checking the state of the irq in our
> queues: if the irq is present in an LR, remove the hardware ACTIVE bit.
> If the irq is present in an LR of another vcpu, send an IPI. Set the
> GIC_IRQ_GUEST_DEACTIVATE bit to tell the receiving vcpu that the active
> bit needs to be deactivated.

I would explain that the write bits of GICD_IACTIVER is left unimplemented.

> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  xen/arch/arm/gic.c         |   40 +++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/vgic-v2.c     |   45 ++++++++++++++++++++++++++++++++++++++------
>  xen/arch/arm/vgic-v3.c     |   44 ++++++++++++++++++++++++++++++++++++++-----
>  xen/include/asm-arm/gic.h  |    1 +
>  xen/include/asm-arm/vgic.h |    4 ++++
>  5 files changed, 123 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 1e1e5ba..75c1f52 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -414,6 +414,15 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>      gic_hw_ops->read_lr(i, &lr_val);
>      irq = lr_val.virq;
>      p = irq_to_pending(v, irq);
> +
> +    if ( test_and_clear_bit(GIC_IRQ_GUEST_DEACTIVATE, &p->status) &&
> +         (lr_val.state & GICH_LR_ACTIVE) )
> +    {
> +        clear_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
> +        lr_val.state &= ~GICH_LR_ACTIVE;
> +        gic_hw_ops->write_lr(i, &lr_val);

You also have to handle the deactivation of an hardware IRQ route to a
guest.

> +    }
> +
>      if ( lr_val.state & GICH_LR_ACTIVE )
>      {
>          set_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
> @@ -489,6 +498,37 @@ void gic_clear_lrs(struct vcpu *v)
>      spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
>  }
>  
> +/* called with rank lock held */

Please add an ASSERT in the code to check this assumption.

> +void gic_deactivate_irq(struct vcpu *v, unsigned int irq)
> +{
> +    unsigned long flags;
> +    struct pending_irq *p;
> +    struct vcpu *v_target = v->domain->arch.vgic.handler->get_target_vcpu(v, irq);
> +
> +    spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
> +
> +    p = irq_to_pending(v_target, irq);
> +    /* the interrupt is not even in an LR */
> +    if ( list_empty(&p->inflight) || !list_empty(&p->lr_queue) )

I think this can be simplified by testing p->lr == GIC_INVALID_LR

> +    {
> +        spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
> +        return;
> +    }
> +
> +    /* it is in an LR, let's check */
> +    set_bit(GIC_IRQ_GUEST_DEACTIVATE, &p->status);
> +    if ( v_target == current )
> +    {
> +        gic_update_one_lr(v_target, p->lr);
> +        spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
> +    } else {
> +        spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
> +        vcpu_unblock(v_target);

Why do you need to unblock the vCPU?

> +        if (v_target->is_running )
> +            smp_send_event_check_mask(cpumask_of(v_target->processor));
> +    }
> +}
> +
>  static void gic_restore_pending_irqs(struct vcpu *v)
>  {
>      int lr = 0;
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index f7d784b..9042062 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -126,8 +126,31 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
>      /* Read the active status of an IRQ via GICD is not supported */
>      case GICD_ISACTIVER ... GICD_ISACTIVERN:
>      case GICD_ICACTIVER ... GICD_ICACTIVERN:
> -        goto read_as_zero;
> -
> +    {
> +        unsigned int i = 0, irq = 0;
> +        struct pending_irq *p;
> +        if ( dabt.size != DABT_WORD ) goto bad_width;
> +        rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICACTIVER, DABT_WORD);
> +        if ( rank == NULL) goto read_as_zero;
> +        vgic_lock_rank(v, rank, flags);
> +        *r = 0;
> +        irq = (gicd_reg - GICD_ICACTIVER) << 3;
> +        for (i = 0; i < 32; i++)
> +        {
> +            p = irq_to_pending(v, i + irq);
> +            /*
> +             * This information is likely out of date because we don't
> +             * actually know which interrupts have become ACTIVE from
> +             * PENDING in the LRs of other processors at it happens
> +             * transparently in hardware.  We would have to interrupt
> +             * all other running vcpus to get an accurate snapshot.
> +             * Let's not do that.
> +             */
> +            *r |= test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) ? (1 << i) : 0;
> +        }

Rather than duplicating the code twice (vgic-v2 and vgic-v3), could you
create a stub function to fetch the value. See vgic_fetch_itargetsr for
instance.

> +        vgic_unlock_rank(v, rank, flags);
> +        return 1;
> +    }
>      case GICD_ITARGETSR ... GICD_ITARGETSRN:
>          if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
>          rank = vgic_rank_offset(v, 8, gicd_reg - GICD_ITARGETSR, DABT_WORD);
> @@ -332,11 +355,21 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
>          return 0;
>  
>      case GICD_ICACTIVER ... GICD_ICACTIVERN:
> +    {
> +        unsigned int i = 0, irq;
>          if ( dabt.size != DABT_WORD ) goto bad_width;
> -        printk(XENLOG_G_ERR
> -               "%pv: vGICD: unhandled word write %#"PRIregister" to ICACTIVER%d\n",
> -               v, r, gicd_reg - GICD_ICACTIVER);
> -        return 0;
> +        rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICACTIVER, DABT_WORD);
> +        if ( rank == NULL) goto write_ignore;
> +        vgic_lock_rank(v, rank, flags);
> +        irq = (gicd_reg - GICD_ICACTIVER) << 3;
> +        while ( (i = find_next_bit(&r, 32, i)) < 32 )
> +        {
> +            gic_deactivate_irq(v, i + irq);
> +            i++;
> +        }

Ditto.

> +        vgic_unlock_rank(v, rank, flags);
> +        return 1;
> +    }
>  
>      case GICD_ITARGETSR ... GICD_ITARGETSR + 7:
>          /* SGI/PPI target is read only */
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index b5249ff..c779f75 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -325,7 +325,31 @@ static int __vgic_v3_distr_common_mmio_read(const char *name, struct vcpu *v,
>      /* Read the active status of an IRQ via GICD/GICR is not supported */
>      case GICD_ISACTIVER ... GICD_ISACTIVERN:
>      case GICD_ICACTIVER ... GICD_ICACTIVERN:
> -        goto read_as_zero;
> +    {
> +        unsigned int i = 0, irq = 0;
> +        struct pending_irq *p;
> +        if ( dabt.size != DABT_WORD ) goto bad_width;
> +        rank = vgic_rank_offset(v, 1, reg - GICD_ICACTIVER, DABT_WORD);
> +        if ( rank == NULL) goto read_as_zero;
> +        vgic_lock_rank(v, rank, flags);
> +        *r = 0;
> +        irq = (reg - GICD_ICACTIVER) << 3;
> +        for (i = 0; i < 32; i++)
> +        {
> +            p = irq_to_pending(v, i + irq);
> +            /*
> +             * This information is likely out of date because we don't
> +             * actually know which interrupts have become ACTIVE from
> +             * PENDING in the LRs of other processors at it happens
> +             * transparently in hardware.  We would have to interrupt
> +             * all other running vcpus to get an accurate snapshot.
> +             * Let's not do that.
> +             */
> +            *r |= test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) ? (1 << i) : 0;
> +        }
> +        vgic_unlock_rank(v, rank, flags);
> +        return 1;
> +    }
>  
>      case GICD_IPRIORITYR ... GICD_IPRIORITYRN:
>          if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
> @@ -421,11 +445,21 @@ static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
>          return 0;
>  
>      case GICD_ICACTIVER ... GICD_ICACTIVERN:
> +    {
> +        unsigned int i = 0, irq;
>          if ( dabt.size != DABT_WORD ) goto bad_width;
> -        printk(XENLOG_G_ERR
> -               "%pv: %s: unhandled word write %#"PRIregister" to ICACTIVER%d\n",
> -               v, name, r, reg - GICD_ICACTIVER);
> -        return 0;
> +        rank = vgic_rank_offset(v, 1, reg - GICD_ICACTIVER, DABT_WORD);
> +        if ( rank == NULL) goto write_ignore;
> +        vgic_lock_rank(v, rank, flags);
> +        irq = (reg - GICD_ICACTIVER) << 3;
> +        while ( (i = find_next_bit(&r, 32, i)) < 32 )
> +        {
> +            gic_deactivate_irq(v, i + irq);
> +            i++;
> +        }
> +        vgic_unlock_rank(v, rank, flags);
> +        return 1;
> +    }
>  
>      case GICD_IPRIORITYR ... GICD_IPRIORITYRN:
>          if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;

Regards,

-- 
Julien Grall