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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.09.2023 20:15, Shawn Anastasio wrote:
> On 9/13/23 2:29 AM, Jan Beulich wrote:
>> On 12.09.2023 20:35, Shawn Anastasio wrote:
>>> --- a/xen/arch/ppc/include/asm/bitops.h
>>> +++ b/xen/arch/ppc/include/asm/bitops.h
>>> @@ -1,9 +1,335 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>> +/*
>>> + * Adapted from Linux's arch/powerpc/include/asm/bitops.h.
>>> + *
>>> + * Merged version by David Gibson <david@xxxxxxxxxxxxxxxxxxxxx>.
>>> + * Based on ppc64 versions by: Dave Engebretsen, Todd Inglett, Don
>>> + * Reed, Pat McCarthy, Peter Bergner, Anton Blanchard.  They
>>> + * originally took it from the ppc32 code.
>>> + */
>>>  #ifndef _ASM_PPC_BITOPS_H
>>>  #define _ASM_PPC_BITOPS_H
>>>
>>> +#include <asm/memory.h>
>>> +
>>> +#define __set_bit(n, p)         set_bit(n, p)
>>> +#define __clear_bit(n, p)       clear_bit(n, p)
>>> +
>>> +#define BITOP_BITS_PER_WORD     32
>>> +#define BITOP_MASK(nr)          (1U << ((nr) % BITOP_BITS_PER_WORD))
>>> +#define BITOP_WORD(nr)          ((nr) / BITOP_BITS_PER_WORD)
>>> +#define BITS_PER_BYTE           8
>>> +
>>>  /* PPC bit number conversion */
>>> -#define PPC_BITLSHIFT(be)  (BITS_PER_LONG - 1 - (be))
>>> -#define PPC_BIT(bit)               (1UL << PPC_BITLSHIFT(bit))
>>> -#define PPC_BITMASK(bs, be)        ((PPC_BIT(bs) - PPC_BIT(be)) | 
>>> PPC_BIT(bs))
>>> +#define PPC_BITLSHIFT(be)    (BITS_PER_LONG - 1 - (be))
>>> +#define PPC_BIT(bit)         (1UL << PPC_BITLSHIFT(bit))
>>> +#define PPC_BITMASK(bs, be)  ((PPC_BIT(bs) - PPC_BIT(be)) | PPC_BIT(bs))
>>> +
>>> +/* Macro for generating the ***_bits() functions */
>>> +#define DEFINE_BITOP(fn, op, prefix)                                       
>>>     \
>>> +static inline void fn(unsigned int mask,                                   
>>>    \
>>> +                      volatile unsigned int *p_)                           
>>>     \
>>> +{                                                                          
>>>     \
>>> +    unsigned int old;                                                      
>>>    \
>>> +    unsigned int *p = (unsigned int *)p_;                                  
>>>     \
>>
>> What use is this, when ...
>>
>>> +    asm volatile ( prefix                                                  
>>>     \
>>> +                   "1: lwarx %0,0,%3,0\n"                                  
>>>     \
>>> +                   #op "%I2 %0,%0,%2\n"                                    
>>>     \
>>> +                   "stwcx. %0,0,%3\n"                                      
>>>     \
>>> +                   "bne- 1b\n"                                             
>>>     \
>>> +                   : "=&r" (old), "+m" (*p)                                
>>>     \
>>
>> ... the "+m" operand isn't used and ...
>>
>>> +                   : "rK" (mask), "r" (p)                                  
>>>     \
>>> +                   : "cc", "memory" );                                     
>>>     \
>>
>> ... there's a memory clobber anyway?
>>
> 
> I see what you're saying, and I'm not sure why it was written this way
> in Linux. That said, this is the kind of thing that I'm hesitant to
> change without knowing the rationale of the original author. If you are
> confident that the this can be dropped given that there is already a
> memory clobber, I could drop it. Otherwise I'm inclined to leave it in a
> state that matches Linux.

This being an arch-independent property, I am confident. Yet still you're
the maintainer, so if you want to keep it like this initially, that'll be
okay. If I feel bothered enough, I could always send a patch afterwards.

Jan