Re: [PATCH v3 10/34] xen/riscv: introduce bitops.h

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Thu, 2024-01-18 at 12:01 +0100, Jan Beulich wrote:
> On 18.01.2024 10:43, Oleksii wrote:
> > On Wed, 2024-01-17 at 14:42 +0100, Jan Beulich wrote:
> > > On 17.01.2024 12:37, Oleksii wrote:
> > > > > > > 
> > > > > > > > > Also you want to make sure asm-generic/bitops/bitops-
> > > > > > > > > bits.h
> > > > > > > > > is
> > > > > > > > > really in use here, or else an arch overriding / not
> > > > > > > > > using
> > > > > > > > > that
> > > > > > > > > header may end up screwed.
> > > > > > > > I am not really understand what do you mean. Could you
> > > > > > > > please
> > > > > > > > explain a
> > > > > > > > little bit more.
> > > > > > > 
> > > > > > > Whichever type you use here, it needs to be in sync with
> > > > > > > BITOP_BITS_PER_WORD. Hence you want to include the
> > > > > > > _local_
> > > > > > > bitops-
> > > > > > > bits.h
> > > > > > > here, such that in case of an inconsistent override by an
> > > > > > > arch
> > > > > > > the
> > > > > > > compiler would complain about the two differring #define-
> > > > > > > s.
> > > > > > > (IOW
> > > > > > > an
> > > > > > > arch overriding BITOP_BITS_PER_WORD cannot re-use this
> > > > > > > header
> > > > > > > as-
> > > > > > > is.)
> > > > > > > 
> > > > > > > The same may, btw, be true for others of the new headers
> > > > > > > you
> > > > > > > add
> > > > > > > -
> > > > > > > the
> > > > > > > same #include would therefore be needed there as well.
> > > > > > Now it clear to me.
> > > > > > 
> > > > > > 
> > > > > > It seems like BITOP_BITS_PER_WORD, BITOP_MASK, BITOP_WORD,
> > > > > > and
> > > > > > BITS_PER_BYTE are defined in {arm, ppc,
> > > > > > riscv}/include/asm/bitops.h.
> > > > > > I expected that any architecture planning to use asm-
> > > > > > generic/bitops/bitops-bits.h would include it at the
> > > > > > beginning
> > > > > > of
> > > > > > <arch>/include/asm/bitops.h, similar to what is done for
> > > > > > RISC-
> > > > > > V:
> > > > > >    #ifndef _ASM_RISCV_BITOPS_H
> > > > > >    #define _ASM_RISCV_BITOPS_H
> > > > > >    
> > > > > >    #include <asm/system.h>
> > > > > >    
> > > > > >    #include <asm-generic/bitops/bitops-bits.h>
> > > > > >    ...
> > > > > > 
> > > > > > But in this case, to allow architecture overrides macros,
> > > > > > it is
> > > > > > necessary to update asm-generic/bitops/bitops-bits.h:
> > > > > >     #ifndef BITOP_BITS_PER_WORD
> > > > > >     #define BITOP_BITS_PER_WORD     32
> > > > > >     #endif
> > > > > >    ...
> > > > > > Therefore,  if an architecture needs to override something,
> > > > > > it
> > > > > > will
> > > > > > add
> > > > > > #define ... before #include <asm-generic/bitops/bitops-
> > > > > > bits.h>.
> > > > > > 
> > > > > > Does it make sense?
> > > > > 
> > > > > Sure. But then the arch also needs to provide a corresponding
> > > > > typedef
> > > > > (and bitops-bits.h the fallback one), for use wherever you
> > > > > use
> > > > > any of
> > > > > those #define-s.
> > > > Which one typedef is needed to provide?
> > > > <asm-generic/bitops/bitops-bits.h> contains only macros.
> > > 
> > > A new one, to replace where right now you use "unsigned int" and
> > > I
> > > initially said you need to use "uint32_t" instead. With what you
> > > said
> > > earlier, uint32_t won't work there (anymore).
> > Wouldn't it be enough just to "#include <xen/types.h>" in headers
> > where
> > "uint32_t" is used?
> 
> No, my point wasn't to make uint32_t available. We need a _separate_
> typedef which matches the #define-s. Otherwise, if an arch defines
> BITOP_BITS_PER_WORD to, say, 64, this generic code would do the wrong
> thing.
Oh, yeah this is true.

We have to introduce in bitops-bits.h:
   typedef uint_32t bitops_type; 

And then use it in function such as test_bit:
   static inline int test_bit(int nr, const volatile void *addr)
   {
       const volatile bitops_type *p = addr;
       return 1 & (p[BITOP_WORD(nr)] >> (nr & (BITOP_BITS_PER_WORD -
   1)));
   }

Thanks for clarification.

~ Oleksii