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

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

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.
Cheers,

--
Julien Grall

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