From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH v4 3/5] xen/arm: vgic: Optimize the way to store the target vCPU in the rank Date: Fri, 23 Oct 2015 11:14:29 +0100 Message-ID: <1445595269.2374.106.camel@citrix.com> References: <1444659760-24123-1-git-send-email-julien.grall@citrix.com> <1444659760-24123-4-git-send-email-julien.grall@citrix.com> <1445530664.2374.45.camel@citrix.com> <562919CF.1020709@citrix.com> <1445592868.2374.84.camel@citrix.com> <562A0572.1090701@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZpZMn-0006tR-Ab for xen-devel@lists.xenproject.org; Fri, 23 Oct 2015 10:14:33 +0000 In-Reply-To: <562A0572.1090701@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Julien Grall , xen-devel@lists.xenproject.org Cc: stefano.stabellini@eu.citrix.com List-Id: xen-devel@lists.xenproject.org On Fri, 2015-10-23 at 11:01 +0100, Julien Grall wrote: > On 23/10/15 10:34, Ian Campbell wrote: > > On Thu, 2015-10-22 at 18:15 +0100, Julien Grall wrote: > > > Hi Ian, > > > > > > On 22/10/15 17:17, Ian Campbell wrote: > > > > 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? > > > > > > In fact, the virq is already correctly set before the loop (see patch > > > #2): > > > > > > virq = rank->index * NR_INTERRUPT_PER_RANK + offset; > > > > > > The variable is incremented in the for loop. So I just forgot to drop > > > this line when I did the split. > > > > > > Not that it's not possible to use directly offset because for byte > > > access it will point to the byte modified and not the base address of > > > the register. > > > > > > Though, I could use a mask, but I find this solution clearer. > > > > But per the above what is actually going to happen is you drop this > > change? > > As said, the introduction of virq within this patch is a mistake. > Patch #2 already set virq before the loop: I thought that was what you said, but then your final line seemed to contradict that by implying that you wanted to keep virq here (the implicat ion of saying it is clearer to you). > offset &= INTERRUPT_RANK_MASK; > offset &= ~(NR_TARGET_PER_REG - 1); > > virq = rank->index * NR_INTERRUPT_PER_RANK + offset; > > for ( i = 0; i < NR_TARGET_PER_REG; i++, offset++, virq++ ) > >