From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH v1 6/8] xen/arm: vgic: Optimize the way to store the target vCPU in the rank Date: Tue, 29 Sep 2015 15:23:39 +0100 Message-ID: <1443536619.16718.104.camel@citrix.com> References: <1443192698-16163-1-git-send-email-julien.grall@citrix.com> <1443192698-16163-7-git-send-email-julien.grall@citrix.com> <1443532034.16718.63.camel@citrix.com> <560A93DB.4090701@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 1Zgvol-0004de-Lh for xen-devel@lists.xenproject.org; Tue, 29 Sep 2015 14:23:43 +0000 In-Reply-To: <560A93DB.4090701@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: Vijaya.Kumar@caviumnetworks.com, stefano.stabellini@citrix.com, manish.jaggi@caviumnetworks.com, vijay.kilari@gmail.com List-Id: xen-devel@lists.xenproject.org On Tue, 2015-09-29 at 14:36 +0100, Julien Grall wrote: > On 29/09/15 14:07, Ian Campbell wrote: > > On Fri, 2015-09-25 at 15:51 +0100, Julien Grall wrote: > > > Xen is currently directly storing the value of register > > > GICD_ITARGETSR > > > (for GICv2) and GICD_IROUTER (for GICv3) in the rank. This makes the > > > emulation of the registers access very simple but makes the code to > > > get > > > the target vCPU for a given IRQ more complex. > > > > > > While the target vCPU of an IRQ is retrieved everytime an IRQ is > > > injected to the guest, the access to the register occurs less often. > > > > > > So the data structure should be optimized for the most common case > > > rather than the inverse. > > > > > > This patch introduce the usage of an array to store the target vCPU > > > for > > > every interrupt in the rank. This will make the code to get the > > > target > > > very quick. The emulation code will now have to generate the > > > GICD_ITARGETSR > > > and GICD_IROUTER register for read access and split it to store in a > > > convenient way. > > > > > > Note that with these changes, any read to those registers will list > > > only > > > the target vCPU used by Xen. This is fine because the GIC spec > > > doesn't > > > require to return exactly the value written and it can be seen as if > > > we > > > decide to implement the register read-only. > > > > I think this is probably OK, but skirting round what the spec actually > > says > > a fair bit. > > Well, nothing in the spec clearly explain the behavior of a read access > on the register. An implementation could decide to make some bits RO or > even not store everything. > > FWIW, KVM is using the same trick. At least we'll both get screwed by a picky OS then ;-) > > Although I'm not 100% sure what that comment is trying to say. From the > > code I think you mean aligned to the appropriate 4 byte boundary? > > Yes. What about: "Note the offset will be aligned to the appropriate 4 > byte boundary" Yes, or just "the appropriate boundary". > I will rename to vgic_fetch_itargetsr(...). (and for the other similar functions I assume). > > I don't see anything which is handling the PPI and SGI special case > > (which > > is that they are r/o). > > > > Likewise on read they are supposed to always reflect the value of the > > CPU > > doing the read. > > > > Are both of those handled elsewhere in some way I'm missing? > > A write to those registers is ignored and handled by a separate case > (GICD_ITARGETSR ... GICD_ITARGETSR + 7). Ah, good! And a read just happens as normal due to the initialisation of the rank's fields I saw at some point. Good. > The only places where rank->vcpu[...] is set are vgic_store_* and at the > initialization time (always routed to vCPU 0). > Before the new value is stored, we check the validity of the vCPU the > value should always be valid here. Good.