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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

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?

~ Olkesii