Re: [PATCH v2 14/39] xen/riscv: introduce bitops.h

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.11.2023 11:30, Oleksii Kurochko wrote:
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>

So this looks to have been taken from Linux, which could do with saying
(including which version or most recent commit). It may e.g. justify you
using tab indentation here, albeit ...

> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/bitops.h
> @@ -0,0 +1,288 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (C) 2012 Regents of the University of California */
> +
> +#ifndef _ASM_RISCV_BITOPS_H
> +#define _ASM_RISCV_BITOPS_H
> +
> +#include <asm/system.h>
> +
> +#define BITOP_BITS_PER_WORD     32
> +#define BITOP_MASK(nr)               (1UL << ((nr) % BITOP_BITS_PER_WORD))
> +#define BITOP_WORD(nr)               ((nr) / BITOP_BITS_PER_WORD)
> +#define BITS_PER_BYTE                8
> +
> +#define __set_bit(n,p)          set_bit(n,p)
> +#define __clear_bit(n,p)        clear_bit(n,p)

... then please consistently. Other style related remarks made on the
system.h patch apply here as well (unless again there's a goal of
keeping the diff to the Linux original small; yet then I guess the
delta to the Linux file is already pretty large).

> +/* Based on linux/include/asm-generic/bitops/find.h */
> +
> +#ifndef find_next_bit
> +/**
> + * find_next_bit - find the next set bit in a memory region
> + * @addr: The address to base the search on
> + * @offset: The bitnumber to start searching at
> + * @size: The bitmap size in bits
> + */
> +extern unsigned long find_next_bit(const unsigned long *addr, unsigned long
> +             size, unsigned long offset);
> +#endif
> +
> +#ifndef find_next_zero_bit
> +/**
> + * find_next_zero_bit - find the next cleared bit in a memory region
> + * @addr: The address to base the search on
> + * @offset: The bitnumber to start searching at
> + * @size: The bitmap size in bits
> + */
> +extern unsigned long find_next_zero_bit(const unsigned long *addr, unsigned
> +             long size, unsigned long offset);
> +#endif
> +
> +/**
> + * find_first_bit - find the first set bit in a memory region
> + * @addr: The address to start the search at
> + * @size: The maximum size to search
> + *
> + * Returns the bit number of the first set bit.
> + */
> +extern unsigned long find_first_bit(const unsigned long *addr,
> +                                 unsigned long size);
> +
> +/**
> + * find_first_zero_bit - find the first cleared bit in a memory region
> + * @addr: The address to start the search at
> + * @size: The maximum size to search
> + *
> + * Returns the bit number of the first cleared bit.
> + */
> +extern unsigned long find_first_zero_bit(const unsigned long *addr,
> +                                      unsigned long size);

Looking over the titles of the rest of the series, I can't spot where
these are going to be implemented. The again maybe you indeed can get
away without those, at least initially.

> +#define ffs(x) ({ unsigned int __t = (x); fls(__t & -__t); })

This wants to use ISOLATE_LSB() now.

Jan