Re: [PATCH 05/12] xen/arm64: bitops: Match the register size with the value size in flsl

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

Hi,

On 18/04/2019 19:40, Stefano Stabellini wrote:
> On Wed, 17 Apr 2019, Julien Grall wrote:
>> On 4/17/19 9:24 PM, Stefano Stabellini wrote:
>>> On Wed, 27 Mar 2019, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> On 27/03/2019 19:03, Andrew Cooper wrote:
>>>>> On 27/03/2019 18:45, Julien Grall wrote:
>>>>>> Clang is pickier than GCC for the register size in asm statement. It
>>>>>> expects
>>>>>> the register size to match the value size.
>>>>>>
>>>>>> The instruction clz is expecting the two operands to be the same size
>>>>>> (i.e 32-bit or 64-bit). As the flsl function is dealing with 64-bit
>>>>>> value, we need to make the destination variable 64-bit as well.
>>>>>>
>>>>>> While at it, add a newline before the return statement.
>>>>>>
>>>>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>>>> ---
>>>>>>     xen/include/asm-arm/arm64/bitops.h | 3 ++-
>>>>>>     1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/xen/include/asm-arm/arm64/bitops.h
>>>>>> b/xen/include/asm-arm/arm64/bitops.h
>>>>>> index 6bf1922680..05045f1109 100644
>>>>>> --- a/xen/include/asm-arm/arm64/bitops.h
>>>>>> +++ b/xen/include/asm-arm/arm64/bitops.h
>>>>>> @@ -34,12 +34,13 @@ static /*__*/always_inline unsigned long
>>>>>> __ffs(unsigned long word)
>>>>>>        static inline int flsl(unsigned long x)
>>>>>>     {
>>>>>> -        int ret;
>>>>>> +        uint64_t ret;
>>>>>>                if (__builtin_constant_p(x))
>>>>>>                    return generic_flsl(x);
>>>>>>                asm("clz\t%0, %1" : "=r" (ret) : "r" (x));
>>>>>
>>>>> As x is fixed by the ABI, ret should be unsigned long to match, surely?
>>>>
>>>> No need for it. The result of the instruction clz will always be smaller
>>>> than 64.
>>>>
>>>> I suspect they require a 64-bit register just for simplicity as they
>>>> encode the size for the 2 registers in only a single bit (i.e sf).
>>>>
>>>>>
>>>>> This will compile as it is arm64 specific, but it looks just as wrong
>>>>> (per the commit message) as using int in the first place.
>>>> See above. I can clarify it in the commit message.
>>>
>>> I agree with Andrew: I can see that the code is correct, but it really
>>> looks wrong at first sight. Why not use unsigned long? This is an arm64
>>> header anyway.
>>
>> While this is implemented in arm64, this is a function used in common code...
>> So the prototype would have to be changed everywhere.
>>
>> However, as I pointed out this is a design decision from Arm64 and I think it
>> would be wrong to return unsigned long.
>>
>> I offered the suggestion to update the commit message. I can also add a
>> comment in the code.
> 
> Reading your reply, I am pretty sure that we are talking about different
> things. I agree that we don't want to change the prototype everywhere,
> arm32, x86. It would be good to add a comment to the code or the commit
> message, I leave it to you.
> 
> My suggestion was basically "code style". I only meant the below. To me
> it looks nicer because at least the ret var is the same type of the
> argument, and it is more immediately obvious that the clz operation is
> correct. Not a big deal.

I find quite amusing that you suggest this when in the patch #7 you mention the 
MISRA rule 10.1 which forbid assigning a value to a narrower type (unsigned long 
-> int).

Regardless that, clz operation clearly ask for a 64-bit value. So uint64_t makes 
much more sense than "unsigned long". In general, I am a strong advocate to use 
uintX_t when the size is known. "unsigned long" should only be used in place you 
need compat between 32-bit and 64-bit code.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel