Re: [PATCH v5 2/5] xen/ppc: Implement bitops.h
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 .
+ * 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
+
+#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"
\
+ : "=" (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
Re: [PATCH v5 2/5] xen/ppc: Implement bitops.h
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 .
>>> + * 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
>>> +
>>> +#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"
>>> \
>>> + : "=" (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
Re: [PATCH v5 2/5] xen/ppc: Implement bitops.h
On 9/13/23 2:29 AM, Jan Beulich wrote:
> On 12.09.2023 20:35, Shawn Anastasio wrote:
>> Implement bitops.h, based on Linux's implementation as of commit
>> 5321d1b1afb9a17302c6cec79f0cbf823eb0d3fc. Though it is based off of
>> Linux's implementation, this code diverges significantly in a number of
>> ways:
>> - Bitmap entries changed to 32-bit words to match X86 and Arm on Xen
>> - PPC32-specific code paths dropped
>> - Formatting completely re-done to more closely line up with Xen.
>> Including 4 space indentation.
>> - Use GCC's __builtin_popcount* for hweight* implementation
>>
>> Signed-off-by: Shawn Anastasio
>> Acked-by: Jan Beulich
>> ---
>> v5:
>> - Switch lingering ulong bitop parameters/return values to uint.
>>
>> v4:
>> - Mention __builtin_popcount impelmentation of hweight* in message
>> - Change type of BITOP_MASK expression from unsigned long to unsigned
>> int
>> - Fix remaining underscore-prefixed macro params/vars
>> - Fix line wrapping in test_and_clear_bit{,s}
>> - Change type of test_and_clear_bits' pointer parameter from void *
>> to unsigned int *. This was already done for other functions but
>> missed here.
>> - De-macroize test_and_set_bits.
>> - Fix formatting of ffs{,l} macro's unary operators
>> - Drop extra blank in ffz() macro definition
>>
>> v3:
>> - Fix style of inline asm blocks.
>> - Fix underscore-prefixed macro parameters/variables
>> - Use __builtin_popcount for hweight* implementation
>> - Update C functions to use proper Xen coding style
>>
>> v2:
>> - Clarify changes from Linux implementation in commit message
>> - Use PPC_ATOMIC_{ENTRY,EXIT}_BARRIER macros from instead
>> of hardcoded "sync" instructions in inline assembly blocks.
>> - Fix macro line-continuing backslash spacing
>> - Fix hard-tab usage in find_*_bit C functions.
>>
>> xen/arch/ppc/include/asm/bitops.h | 332 +-
>> 1 file changed, 329 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/ppc/include/asm/bitops.h
>> b/xen/arch/ppc/include/asm/bitops.h
>> index 548e97b414..0f75ff3f9d 100644
>> --- 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 .
>> + * 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
>> +
>> +#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"
>>\
>> + : "=" (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
Re: [PATCH v5 2/5] xen/ppc: Implement bitops.h
On 12.09.2023 20:35, Shawn Anastasio wrote:
> Implement bitops.h, based on Linux's implementation as of commit
> 5321d1b1afb9a17302c6cec79f0cbf823eb0d3fc. Though it is based off of
> Linux's implementation, this code diverges significantly in a number of
> ways:
> - Bitmap entries changed to 32-bit words to match X86 and Arm on Xen
> - PPC32-specific code paths dropped
> - Formatting completely re-done to more closely line up with Xen.
> Including 4 space indentation.
> - Use GCC's __builtin_popcount* for hweight* implementation
>
> Signed-off-by: Shawn Anastasio
> Acked-by: Jan Beulich
> ---
> v5:
> - Switch lingering ulong bitop parameters/return values to uint.
>
> v4:
> - Mention __builtin_popcount impelmentation of hweight* in message
> - Change type of BITOP_MASK expression from unsigned long to unsigned
> int
> - Fix remaining underscore-prefixed macro params/vars
> - Fix line wrapping in test_and_clear_bit{,s}
> - Change type of test_and_clear_bits' pointer parameter from void *
> to unsigned int *. This was already done for other functions but
> missed here.
> - De-macroize test_and_set_bits.
> - Fix formatting of ffs{,l} macro's unary operators
> - Drop extra blank in ffz() macro definition
>
> v3:
> - Fix style of inline asm blocks.
> - Fix underscore-prefixed macro parameters/variables
> - Use __builtin_popcount for hweight* implementation
> - Update C functions to use proper Xen coding style
>
> v2:
> - Clarify changes from Linux implementation in commit message
> - Use PPC_ATOMIC_{ENTRY,EXIT}_BARRIER macros from instead
> of hardcoded "sync" instructions in inline assembly blocks.
> - Fix macro line-continuing backslash spacing
> - Fix hard-tab usage in find_*_bit C functions.
>
> xen/arch/ppc/include/asm/bitops.h | 332 +-
> 1 file changed, 329 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/ppc/include/asm/bitops.h
> b/xen/arch/ppc/include/asm/bitops.h
> index 548e97b414..0f75ff3f9d 100644
> --- 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 .
> + * 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
> +
> +#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"
> \
> + : "=" (old), "+m" (*p)
> \
... the "+m" operand isn't used and ...
> + : "rK" (mask), "r" (p)
> \
> + : "cc", "memory" );
> \
... there's a memory clobber anyway?
Also (nit) note that the long -> int change has caused some of the
backslashes to no longer align.
> +}
> +
> +DEFINE_BITOP(set_bits, or, "")
> +DEFINE_BITOP(change_bits, xor, "")
Are there further plans to add uses of DEFINE_BITOP() with the last argument
not an empty string? If not,
[PATCH v5 2/5] xen/ppc: Implement bitops.h
Implement bitops.h, based on Linux's implementation as of commit
5321d1b1afb9a17302c6cec79f0cbf823eb0d3fc. Though it is based off of
Linux's implementation, this code diverges significantly in a number of
ways:
- Bitmap entries changed to 32-bit words to match X86 and Arm on Xen
- PPC32-specific code paths dropped
- Formatting completely re-done to more closely line up with Xen.
Including 4 space indentation.
- Use GCC's __builtin_popcount* for hweight* implementation
Signed-off-by: Shawn Anastasio
Acked-by: Jan Beulich
---
v5:
- Switch lingering ulong bitop parameters/return values to uint.
v4:
- Mention __builtin_popcount impelmentation of hweight* in message
- Change type of BITOP_MASK expression from unsigned long to unsigned
int
- Fix remaining underscore-prefixed macro params/vars
- Fix line wrapping in test_and_clear_bit{,s}
- Change type of test_and_clear_bits' pointer parameter from void *
to unsigned int *. This was already done for other functions but
missed here.
- De-macroize test_and_set_bits.
- Fix formatting of ffs{,l} macro's unary operators
- Drop extra blank in ffz() macro definition
v3:
- Fix style of inline asm blocks.
- Fix underscore-prefixed macro parameters/variables
- Use __builtin_popcount for hweight* implementation
- Update C functions to use proper Xen coding style
v2:
- Clarify changes from Linux implementation in commit message
- Use PPC_ATOMIC_{ENTRY,EXIT}_BARRIER macros from instead
of hardcoded "sync" instructions in inline assembly blocks.
- Fix macro line-continuing backslash spacing
- Fix hard-tab usage in find_*_bit C functions.
xen/arch/ppc/include/asm/bitops.h | 332 +-
1 file changed, 329 insertions(+), 3 deletions(-)
diff --git a/xen/arch/ppc/include/asm/bitops.h
b/xen/arch/ppc/include/asm/bitops.h
index 548e97b414..0f75ff3f9d 100644
--- 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 .
+ * 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
+
+#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_;
\
+asm volatile ( prefix
\
+ "1: lwarx %0,0,%3,0\n"
\
+ #op "%I2 %0,%0,%2\n"
\
+ "stwcx. %0,0,%3\n"
\
+ "bne- 1b\n"
\
+ : "=" (old), "+m" (*p)
\
+ : "rK" (mask), "r" (p)
\
+ : "cc", "memory" );
\
+}
+
+DEFINE_BITOP(set_bits, or, "")
+DEFINE_BITOP(change_bits, xor, "")
+
+#define DEFINE_CLROP(fn, prefix)
\
+static inline void fn(unsigned int mask, volatile unsigned int *p_) \
+{
\
+unsigned int old; \
+unsigned int *p = (unsigned int *)p_;
\
+asm volatile ( prefix
\
+ "1: lwarx %0,0,%3,0\n"
\
+
