Re: [PATCH v11 03/10] xen/arm: inflight irqs during migration

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

Hi Stefano,

On 08/08/2014 06:13 PM, Stefano Stabellini wrote:
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index 63d4f65..39d8272 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -356,34 +356,60 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
>          goto write_ignore;
>  
>      case GICD_ITARGETSR + 8 ... GICD_ITARGETSRN:
> +    {
> +        /* unsigned long needed for find_next_bit */
> +        unsigned long target;
> +        int i;
>          if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
>          rank = vgic_rank_offset(v, 8, gicd_reg - GICD_ITARGETSR, DABT_WORD);
>          if ( rank == NULL) goto write_ignore;
>          /* 8-bit vcpu mask for this domain */
>          BUG_ON(v->domain->max_vcpus > 8);
> -        tr = (1 << v->domain->max_vcpus) - 1;
> +        target = (1 << v->domain->max_vcpus) - 1;
>          if ( dabt.size == 2 )
> -            tr = tr | (tr << 8) | (tr << 16) | (tr << 24);
> +            target = target | (target << 8) | (target << 16) | (target << 24);
>          else
> -            tr = (tr << (8 * (gicd_reg & 0x3)));
> -        tr &= *r;
> +            target = (target << (8 * (gicd_reg & 0x3)));
> +        target &= *r;

Renaming tr in target in this patch while most of the code has been
introduced in patch #1 is very odd. Actually it make the code harder to
read.

Anyway, I think it's fine a V11. I don't want to delay the merge just
for that.

> +void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
> +{
> +    unsigned long flags;
> +    struct pending_irq *p = irq_to_pending(old, irq);
> +
> +    /* nothing to do for virtual interrupts */
> +    if ( p->desc == NULL )
> +        return;
> +
> +    /* migration already in progress, no need to do anything */
> +    if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
> +        return;
> +
> +    spin_lock_irqsave(&old->arch.vgic.lock, flags);
> +
> +    if ( list_empty(&p->inflight) )
> +    {
> +        spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
> +        return;
> +    }

NIT: I would create a label unlock below and jump to it. It would avoid
at least one of the 3 call to spin_unlock.

> +    /* If the IRQ is still lr_pending, re-inject it to the new vcpu */
> +    if ( !list_empty(&p->lr_queue) )
> +    {
> +        list_del_init(&p->lr_queue);
> +        list_del_init(&p->inflight);
> +        spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
> +        vgic_vcpu_inject_irq(new, irq);

Shouldn't we also clear the p->status? At least for consistency?

Regards,

-- 
Julien Grall