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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 18 Apr 2019, Julien Grall wrote:
> 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@xxxxxxx>
> > > > > > > ---
> > > > > > >     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.

all right, OK by me

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