From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH v1 3/8] xen/arm: Support sign-extension for every read access Date: Tue, 29 Sep 2015 12:01:31 +0100 Message-ID: <1443524491.16718.47.camel@citrix.com> References: <1443192698-16163-1-git-send-email-julien.grall@citrix.com> <1443192698-16163-4-git-send-email-julien.grall@citrix.com> <1443199485.25250.212.camel@citrix.com> <56096E0D.8060901@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 1Zgsf9-00060M-Ox for xen-devel@lists.xenproject.org; Tue, 29 Sep 2015 11:01:35 +0000 In-Reply-To: <56096E0D.8060901@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 17:42 +0100, Julien Grall wrote: > > > + */ > > > + if ( info->dabt.sign && (*r & (1UL << (size - 1)) )) > > > + { > > > + /* > > > + * We are relying on register_t as the same size as > > > + * an unsigned long or order to keep the 32bit some smaller > > > > "order"? I'm not sure what you meant here so I can't suggest an > > alternative. > > hmmm... the end of the comment is badly written :/. I wanted to say > "We are relying on register_t using the same size as and unsigned long > in order to keep the 32-bit assembly code smaller" "as an unsigned long", then it works. > > > > > > + */ > > > + BUILD_BUG_ON(sizeof(register_t) != sizeof(unsigned long)); > > > + *r |= (~0UL) << size; > > > > I think here and in the initial if you need to be careful of the case > > where > > size == 32 (on arm32) or == 64 (on arm64), since a shift by >= the size > > of > > the variable is, I think, undefined behaviour. > > > > It's also a waste of time sign extending in that case. > > From the spec, dabt.sign is only set when smaller size than the register > size. For instance for ARMv7 spec (ARM DDI 0406C.b page B3-1433): > > "SSE, Syndrome sign extend. For a byte or halfword load operation, > indicates whether > the data item must be sign extended. [...] For all other operations this > bit is 0." > > So we don't have to worry about waste of time and undefined behavior. > Note that mention it in the commit message. Maybe it wasn't clear enough? It's a fairly oblique mention in the commit message, so I think it indeed wasn't explicit enough. I'm unsure if we need to worry about the fact that the compiler does't know about that bit of the spec, so it might assume that size could be >=32 or 64 and do something unhelpful. Probably not. Ian.