From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH v1 5/8] xen/arm: vgic: Optimize the way to store GICD_IPRIORITYR in the rank Date: Tue, 29 Sep 2015 11:56:39 +0100 Message-ID: <1443524199.16718.43.camel@citrix.com> References: <1443192698-16163-1-git-send-email-julien.grall@citrix.com> <1443192698-16163-6-git-send-email-julien.grall@citrix.com> <1443437458.25250.262.camel@citrix.com> <56097475.7070600@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZgsaR-0004zk-7Q for xen-devel@lists.xenproject.org; Tue, 29 Sep 2015 10:56:43 +0000 In-Reply-To: <56097475.7070600@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 Mon, 2015-09-28 at 18:10 +0100, Julien Grall wrote: > Hi Ian, > > On 28/09/15 11:50, Ian Campbell wrote: > > > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c > > > index c013200..2787507 100644 > > > --- a/xen/arch/arm/vgic-v3.c > > > +++ b/xen/arch/arm/vgic-v3.c > > > @@ -430,18 +430,26 @@ static int > > > __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v, > > > return 0; > > > > > > case GICD_IPRIORITYR ... GICD_IPRIORITYRN: > > > + { > > > + uint32_t ipriorityr; > > > + > > > if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto > > > bad_width; > > > rank = vgic_rank_offset(v, 8, reg - GICD_IPRIORITYR, > > > DABT_WORD); > > > - if ( rank == NULL ) goto write_ignore; > > > + if ( rank == NULL) goto write_ignore; > > > + > > > vgic_lock_rank(v, rank, flags); > > > if ( dabt.size == DABT_WORD ) > > > - rank->ipriority[REG_RANK_INDEX(8, reg - GICD_IPRIORITYR, > > > - DABT_WORD)] = r; > > > + ipriorityr = r; > > > else > > > - vgic_byte_write(&rank->ipriority[REG_RANK_INDEX(8, > > > - reg - GICD_IPRIORITYR, DABT_WORD)], r, reg); > > > + { > > > + ipriorityr = vgic_generate_ipriorityr(rank, reg - > > > GICD_IPRIORITYR); > > > + vgic_byte_write(&ipriorityr, r, reg); > > > + } > > > + vgic_store_ipriorityr(rank, reg - GICD_IPRIORITYR, > > > ipriorityr); > > > > Given the underlying change to the datastructure I think you should > > instead > > supply a helper to set the priority of a given IRQ and use that for the > > DABT_BYTE case. > > > > For the DABT_WORD case your existing store helper would become 4 calls > > to > > the byte helper. > > > > That would be better than generating the full 32-bit value, masking in > > the > > updated byte and then deconstructing the word again. > > We discussed it IRL, so I will summarize here for everyone IRL. > > In a follow-up patch (#7), the code to emulate IPRIORITYR will call > vgic_reg32_write which is a generic helper to handle any access size. > The goal is to drop any access size handling in the code. After patch > #7, the code will look like: > > if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width; > rank = vgic_rank_offset(v, 8, reg - GICD_IPRIORITYR, DABT_WORD); > if ( rank == NULL) goto write_ignore; > > vgic_lock_rank(v, rank, flags); > ipriorityr = vgic_generate_ipriorityr(rank, reg - GICD_IPRIORITYR); > vgic_reg32_write(&ipriorityr, r, info); > vgic_store_ipriorityr(rank, reg - GICD_IPRIORITYR, ipriorityr); > vgic_unlock_rank(v, rank, flags); > > Introducing code path depending on access size means possibly more buggy > code. Note that we already hit a lot of wrong access size in the > emulation. So I would prefer to keep the code as generic as possible > even if it means a small impact on emulating byte access. Agreed, given this context that makes sense. [...] > > union { > > uint32_t ipriorityr[8]; > > uint8_t priority[32]; > > } > > > > Get us the best of both worlds, or are we stymied by endianess? > > The support of big-endian in Xen should be generic. I.e we deal with the > endianess before calling the handlers, so the handler will always have > the value in the hypervisor endianess. > > Note that I've got a support for big-endian but never had the chance to > upstream it. > > I think, as long as Xen stays in little endian, we would have no issue > with your solution. I wasn't concerned about Xen being big-endian, but rather about whether ipriorityr[0] is the same byte as priority[0]&0xff or if it was actually priority[0]>>24 in little endian mode. I wasn't feeling very well on Monday so I didn't try to think about it too hard. Ian.