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

[Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>]

On 9/15/23 1:50 AM, Jan Beulich wrote:
> 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.
>

Okay, for now then I will leave it as-is.

> Jan

Thanks,
Shawn