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

[Julien Grall <julien.grall@citrix.com>]

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