Re: [Qemu-ppc] [PATCH v9 2/7] target/ppc: replace __builtin_ffssl() by the equivalent ctz routines

[Cédric Le Goater]

On 12/18/18 3:23 AM, David Gibson wrote:
> On Mon, Dec 17, 2018 at 11:34:40PM +0100, Cédric Le Goater wrote:
>> And remove the intermediate MASK_TO_LSH macro which does not add any value.
>>
>> This fixes a compile breakage on windows.
>>
>> Signed-off-by: Cédric Le Goater <address@hidden>
> 
> It's an improvement over what's there, but it still leaves macros
> whose primary use would be for guest registers, but are typed
> according to host values, which doesn't make much sense.
> 
> I think instead we should redefine your BE64 / BE32 variants in terms
> of the existing extract*() and deposit*() primitives, and get rid of
> the GETFIELD / SETFIELD macros.

I will get rid of the GETFIELD/SETFIELD macros and rewrite the BE64/BE32 
variants but I won't use the extract*() and deposit*(). I prefer to keep
the same pattern, which is similar to shpc_get/set_status(). I will make 
the code clearer with static inlines. 

I don't really like the names also. what about xive_(get/set)_field(32/64) ?  

C.
 
>> ---
>>  target/ppc/cpu.h | 15 +++++++++------
>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>> index 527181c0f09f..f4ef4f214564 100644
>> --- a/target/ppc/cpu.h
>> +++ b/target/ppc/cpu.h
>> @@ -78,18 +78,21 @@
>>                                   PPC_BIT32(bs))
>>  #define PPC_BITMASK8(bs, be)    ((PPC_BIT8(bs) - PPC_BIT8(be)) | 
>> PPC_BIT8(bs))
>>  
>> +/*
>> + * OPAL PPC bitmask field manipulation, used in XIVE, PHB3 and PHB4
>> + */
>>  #if HOST_LONG_BITS == 32
>> -# define MASK_TO_LSH(m)          (__builtin_ffsll(m) - 1)
>> +#  define GETFIELD(m, v)        (((v) & (m)) >> ctz32(m))
>> +#  define SETFIELD(m, v, val)                                   \
>> +    (((v) & ~(m)) | ((((typeof(v))(val)) << ctz32(m)) & (m)))
>>  #elif HOST_LONG_BITS == 64
>> -# define MASK_TO_LSH(m)          (__builtin_ffsl(m) - 1)
>> +#  define GETFIELD(m, v)        (((v) & (m)) >> ctz64(m))
>> +#  define SETFIELD(m, v, val)                                   \
>> +    (((v) & ~(m)) | ((((typeof(v))(val)) << ctz64(m)) & (m)))
>>  #else
>>  # error Unknown sizeof long
>>  #endif
>>  
>> -#define GETFIELD(m, v)          (((v) & (m)) >> MASK_TO_LSH(m))
>> -#define SETFIELD(m, v, val)                             \
>> -        (((v) & ~(m)) | ((((typeof(v))(val)) << MASK_TO_LSH(m)) & (m)))
>> -
>>  
>> /*****************************************************************************/
>>  /* Exception vectors definitions                                            
>>  */
>>  enum {
>