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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Thu, 2023-12-07 at 16:37 +0100, Jan Beulich wrote:
> 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 ...
Thanks. I'll update the commit message.

> 
> > --- /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.
It's introduced in:
        [PATCH v2 21/39] xen/riscv: introduce bit operations
I think we have to merge this patch with patch 21.
> 
> > +#define ffs(x) ({ unsigned int __t = (x); fls(__t & -__t); })
> 
> This wants to use ISOLATE_LSB() now.
> 
~ Oleksii