From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Campbell <ian.campbell@citrix.com>
Subject: Re: [PATCH v4 3/5] xen/arm: vgic: Optimize the way to
 store the target vCPU in the rank
Date: Thu, 22 Oct 2015 17:17:44 +0100
Message-ID: <1445530664.2374.45.camel@citrix.com>
References: <1444659760-24123-1-git-send-email-julien.grall@citrix.com>
	<1444659760-24123-4-git-send-email-julien.grall@citrix.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta3.messagelabs.com ([195.245.230.39])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <prvs=730db3765=Ian.Campbell@citrix.com>)
	id 1ZpIYn-0001xG-9s
	for xen-devel@lists.xenproject.org; Thu, 22 Oct 2015 16:17:49 +0000
In-Reply-To: <1444659760-24123-4-git-send-email-julien.grall@citrix.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Julien Grall <julien.grall@citrix.com>, xen-devel@lists.xenproject.org
Cc: stefano.stabellini@eu.citrix.com
List-Id: xen-devel@lists.xenproject.org

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.