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

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

On 25/09/15 17:44, Ian Campbell wrote:
>> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
>> index 32b2194..e1b03a2 100644
>> --- a/xen/arch/arm/io.c
>> +++ b/xen/arch/arm/io.c
>> @@ -23,6 +23,32 @@
>>  #include <asm/current.h>
>>  #include <asm/mmio.h>
>>  
>> +static int handle_read(mmio_read_t read_cb, struct vcpu *v,
>> +                       mmio_info_t *info, register_t *r)
>> +{
>> +    uint8_t size = (1 << info->dabt.size) * 8;
>> +
>> +    if ( !read_cb(v, info, r) )
>> +        return 0;
>> +
>> +    /*
>> +     * Extend the bit sign if required.
> 
> I think you meant s/bit sign/sign bit/ but more correct would be "Sign
> extend if required".

Will fix it.

> 
>> +     * 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.
> 
> Did you confirm that all existing handlers meet this requirement? Perhaps
> an ASSERT would be handy?
> 
>> +     */
>> +    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"

> 
>> +         */
>> +        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?

Regards,

-- 
Julien Grall