[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v1 3/8] xen/arm: Support sign-extension for every read access
On 29/09/15 12:01, Ian Campbell wrote: > 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 will update the commit message and mention the spec. > > 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. We have quite a few place in Xen using the idiom foo >> bar and foo << bar when bar is a parameter to a function or local variable. So I don't think we need to worry about this... Regards, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |