Re: [PATCH v8 02/17] xen: introduce generic non-atomic test_*bit()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.04.2024 12:04, Oleksii Kurochko wrote:
> --- a/xen/arch/ppc/include/asm/page.h
> +++ b/xen/arch/ppc/include/asm/page.h
> @@ -4,7 +4,7 @@
>  
>  #include <xen/types.h>
>  
> -#include <asm/bitops.h>
> +#include <xen/bitops.h>
>  #include <asm/byteorder.h>

This wants to move up into the xen/*.h group then, like you have done ...

> --- a/xen/arch/ppc/mm-radix.c
> +++ b/xen/arch/ppc/mm-radix.c
> @@ -1,11 +1,11 @@
>  /* SPDX-License-Identifier: GPL-2.0-or-later */
> +#include <xen/bitops.h>
>  #include <xen/init.h>
>  #include <xen/kernel.h>
>  #include <xen/mm.h>
>  #include <xen/types.h>
>  #include <xen/lib.h>
>  
> -#include <asm/bitops.h>
>  #include <asm/byteorder.h>
>  #include <asm/early_printk.h>
>  #include <asm/page.h>

.. e.g. here.

> --- a/xen/include/xen/bitops.h
> +++ b/xen/include/xen/bitops.h
> @@ -65,10 +65,137 @@ static inline int generic_flsl(unsigned long x)
>   * scope
>   */
>  
> +#define BITOP_MASK(nr)  ((bitop_uint_t)1 << ((nr) % BITOP_BITS_PER_WORD))
> +
> +#define BITOP_WORD(nr)  ((nr) / BITOP_BITS_PER_WORD)
> +
>  /* --------------------- Please tidy above here --------------------- */
>  
>  #include <asm/bitops.h>
>  
> +#ifndef arch_check_bitop_size
> +#define arch_check_bitop_size(addr)

Can this really do nothing? Passing the address of an object smaller than
bitop_uint_t will read past the object in the generic__*_bit() functions.

> +#endif
> +
> +/**
> + * generic__test_and_set_bit - Set a bit and return its old value
> + * @nr: Bit to set
> + * @addr: Address to count from
> + *
> + * This operation is non-atomic and can be reordered.
> + * If two examples of this operation race, one can appear to succeed
> + * but actually fail.  You must protect multiple accesses with a lock.
> + */
> +static always_inline bool
> +generic__test_and_set_bit(unsigned long nr, volatile void *addr)
> +{
> +    bitop_uint_t mask = BITOP_MASK(nr);
> +    volatile bitop_uint_t *p = ((volatile bitop_uint_t *)addr) + 
> BITOP_WORD(nr);

The revision log suggests excess parentheses were dropped from such cast
expressions.

> --- a/xen/include/xen/types.h
> +++ b/xen/include/xen/types.h
> @@ -64,6 +64,11 @@ typedef __u64 __be64;
>  
>  typedef unsigned int __attribute__((__mode__(__pointer__))) uintptr_t;
>  
> +#ifndef BITOP_TYPE
> +    #define BITOP_BITS_PER_WORD 32
> +    typedef uint32_t bitop_uint_t;

Personally I find this indentation odd / misleading. For pre-processor
directives the # preferrably remains first on a line (as was iirc
demanded by earlier C standards), followed by one or more blanks if so
desired. File-scope declarations imo should never be indented.

Jan