Re: [PATCH v3 2/5] xen/ppc: Implement bitops.h

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.09.2023 01:12, Shawn Anastasio wrote:
> On 9/5/23 10:19 AM, Jan Beulich wrote:
>> On 01.09.2023 20:25, Shawn Anastasio wrote:
>>> +#define DEFINE_TESTOP(fn, op, eh)                                          
>>>     \
>>> +static inline unsigned long fn(unsigned long mask, volatile unsigned int 
>>> *_p)  \
>>> +{                                                                          
>>>     \
>>> +    unsigned long old, t;                                                  
>>>     \
>>> +    unsigned int *p = (unsigned int *)_p;                                  
>>>     \
>>> +    asm volatile ( PPC_ATOMIC_ENTRY_BARRIER                                
>>>     \
>>> +                   "1: lwarx %0,0,%3,%4\n"                                 
>>>     \
>>> +                   #op "%I2 %1,%0,%2\n"                                    
>>>     \
>>> +                   "stwcx. %1,0,%3\n"                                      
>>>     \
>>> +                   "bne- 1b\n"                                             
>>>     \
>>> +                   PPC_ATOMIC_EXIT_BARRIER                                 
>>>     \
>>> +                   : "=&r" (old), "=&r" (t)                                
>>>     \
>>> +                   : "rK" (mask), "r" (p), "n" (eh)                        
>>>     \
>>> +                   : "cc", "memory" );                                     
>>>     \
>>> +    return (old & mask);                                                   
>>>     \
>>> +}
>>> +
>>> +DEFINE_TESTOP(test_and_set_bits, or, 0)
>>
>> Why can't test_and_clear_bits() not use this macro-ization? And if it
>> can't and hence there's only this single use, wouldn't it make sense
>> to avoid going through a macro here, too?
>>
> 
> I've just tried this, but unfortunately the "rK" constraint on the mask
> operand only works when instructions have both a reg/reg/reg as well as
> a reg/reg/imm16 form. This is the case for `or` but not `andc`. I guess
> it would be better to keep the two separate implementations rather than
> try to accomodate both cases in the macro somehow (e.g, by making the
> constraint's type a macro parameter as well).
> 
> I can also change the macro template into a standard function for just
> test_and_set_bits, given that it's the only user as you pointed out.
> 
> As for your previous observation about the superfluous `p` variable, it
> would seem the same applies to the macro here. Other than casting away
> the volatile qualifier on `p_` it doesn't seem to be doing much, so I'm
> inclined to remove it.

Right. Comments like this are generally intended to apply to the entire
patch or even entire series.

Jan