Re: [PATCH v1 3/8] xen/arm: Support sign-extension for every read access

[Ian Campbell <ian.campbell@citrix.com>]

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.