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

[Julien Grall <julien.grall@xxxxxxxxxx>]

On 29/09/15 12:03, Ian Campbell wrote:
> On Mon, 2015-09-28 at 19:22 +0100, Julien Grall wrote:
>> On 25/09/15 17:44, Ian Campbell wrote:
>>> On Fri, 2015-09-25 at 15:51 +0100, Julien Grall wrote:
>>> I think you meant s/bit sign/sign bit/ but more correct would be "Sign
>>> extend if required".
>>>
>>>> +     * Note that we expect the read handler to have zeroed the bit
>>>> +     * unused in the register.
>>>
>>> "... to have zeroed the unused bits in the register".
>>>
>>> But I think "unused" is a bit misleading, you mean the ones outside the
>>> requested access size, those bits are still "used" IYSWIM. I can't
>>> think of
>>> a terse term for "outside the requested access size I'm afraid.
>>
>> I will switch to "Note that we expect the read handler to have zeroed
>> the bits outside the requested access size."
>>
>>>
>>> Did you confirm that all existing handlers meet this requirement?
>>
>> Yes, we always do *r in the existing handlers.
>>
>>> Perhaps an ASSERT would be handy?
>>
>> What about:
>>
>> ASSERT((*r & ((~0UL) >> (BITS_PER_LONG - size))) == 0)
> 
> Is that not backwards, e.g for size = 8, then 
> 
> ~0UL >> (32-8) == 0xffffffff >> 24 == 0xff, so you end up checking that the
> lowest byte is zero, but that's the one you expected to change.

I just mess up the computation. It should be

(*r & ((~OUL) << size)) but the behavior is undefined by the spec when
the size = 32 (on arm32) and size = 64 (on arm64) (see 6.5.7 [1])

> Or is it, couldn't we be updating a byte in the middle of the word?

*r is a register, the byte/half-word/word... are always living in the
lowest bit of the register.

> Probably figuring out the correct assertion is more hassle than it is
> worth..

I would rather skip it.

[1] http://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel