Re: [PATCH v4 3/5] xen/arm: vgic: Optimize the way to store the target vCPU in the rank

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

On Mon, 2015-10-12 at 15:22 +0100, Julien Grall wrote:
> [...]
>          /* Only migrate the vIRQ if the target vCPU has changed */
>          if ( new_target != old_target )
>          {
> +            unsigned int virq = rank->index * NR_INTERRUPT_PER_RANK + offset;

FWIW this was the value of offset before it was shifted + masked, I think.
Could you not just save it up top and remember it?

--- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -89,18 +89,72 @@ static struct vcpu *vgic_v3_irouter_to_vcpu(struct
> domain *d, uint64_t irouter)
>      return d->vcpu[vcpu_id];
>  }
>  
> -static struct vcpu *vgic_v3_get_target_vcpu(struct vcpu *v, unsigned int
> irq)
> -{
> -    struct vcpu *v_target;
> -    struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
> +#define NR_BYTE_PER_IROUTER 8U

BYTES

>  
> +/*
> + * Fetch a IROUTER register based on the offset from IROUTER0. Only one
> + * vCPU will be listed for a given vIRQ.
> + *
> + * Note the offset will be aligned to the appropritate  boundary.

appropriate.

> + */
> +static uint64_t vgic_fetch_irouter(struct vgic_irq_rank *rank,
> +                                   unsigned int offset)
> +{
>      ASSERT(spin_is_locked(&rank->lock));
>  
> -    v_target = vgic_v3_irouter_to_vcpu(v->domain, rank->v3.irouter[irq %
> 32]);
> +    /* There is exactly 1 vIRQ per IROUTER */
> +    offset /= NR_BYTE_PER_IROUTER;
>  
> -    ASSERT(v_target != NULL);
> +    /* Get the index in the rank */
> +    offset &= INTERRUPT_RANK_MASK;
>  
> -    return v_target;
> +    return vcpuid_to_vaffinity(rank->vcpu[offset]);
> +}
> +
> +/*
> + * Store a IROUTER register in a convenient way and migrate the vIRQ

There's been a few places now where you said "a I<foo>R", all of which
should be "an I<foo>R".

> + * if necessary. This function only deals with ITARGETSR32 and onwards.
> + *
> + * Note the offset will be aligned to the appriopriate boundary.

"appropriate" again.

The rest looks ok, mostly based on my recollection of previous versions.

Ian.