Re: [PATCH v7 04/19] xen: introduce generic non-atomic test_*bit()

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Fri, 2024-04-05 at 10:05 +0200, Jan Beulich wrote:
> On 05.04.2024 09:56, Oleksii wrote:
> > On Fri, 2024-04-05 at 08:11 +0200, Jan Beulich wrote:
> > > On 04.04.2024 18:24, Oleksii wrote:
> > > > On Thu, 2024-04-04 at 18:12 +0200, Jan Beulich wrote:
> > > > > On 04.04.2024 17:45, Oleksii wrote:
> > > > > > On Thu, 2024-04-04 at 15:22 +0200, Jan Beulich wrote:
> > > > > > > On 03.04.2024 12:19, Oleksii Kurochko wrote:
> > > > > > > > --- a/xen/include/xen/bitops.h
> > > > > > > > +++ b/xen/include/xen/bitops.h
> > > > > > > > @@ -65,10 +65,164 @@ static inline int
> > > > > > > > generic_flsl(unsigned
> > > > > > > > long
> > > > > > > > x)
> > > > > > > >   * scope
> > > > > > > >   */
> > > > > > > >  
> > > > > > > > +#define BITOP_BITS_PER_WORD 32
> > > > > > > > +/* typedef uint32_t bitop_uint_t; */
> > > > > > > > +#define bitop_uint_t uint32_t
> > > > > > > 
> > > > > > > So no arch overrides permitted anymore at all?
> > > > > > Not really, I agree that it is ugly, but I expected that
> > > > > > arch
> > > > > > will
> > > > > > use
> > > > > > undef to override.
> > > > > 
> > > > > Which would be fine in principle, just that Misra wants us to
> > > > > avoid
> > > > > #undef-s
> > > > > (iirc).
> > > > Could you please give me a recommendation how to do that
> > > > better?
> > > > 
> > > > The reason why I put this defintions before inclusion of
> > > > asm/bitops.h
> > > > as RISC-V specific code uses these definitions inside it, so
> > > > they
> > > > should be defined before asm/bitops.h; other option is to
> > > > define
> > > > these
> > > > definitions inside asm/bitops.h for each architecture.
> > > 
> > > Earlier on you had it that other way already (in a different
> > > header,
> > > but the principle is the same): Move the generic definitions
> > > immediately
> > > past inclusion of asm/bitops.h and frame them with #ifndef.
> > It can be done in this way:
> > xen/bitops.h:
> >    ...
> >    #include <asm/bitops.h>
> >    
> >    #ifndef BITOP_TYPE
> >    #define BITOP_BITS_PER_WORD 32
> >    /* typedef uint32_t bitop_uint_t; */
> >    #define bitop_uint_t uint32_t
> >    #endif
> >    ...
> >    
> > But then RISC-V will fail as it is using bitop_uint_t inside
> > asm/bitops.h.
> > So, at least, for RISC-V it will be needed to add asm/bitops.h:
> >    #define BITOP_BITS_PER_WORD 32
> >    /* typedef uint32_t bitop_uint_t; */
> >    #define bitop_uint_t uint32_t
> >    
> > 
> > It seems to me that this breaks the idea of having these macro
> > definitions generic, as RISC-V will redefine BITOP_BITS_PER_WORD
> > and
> > bitop_uint_t with the same values as the generic ones.
> 
> I don't follow. Right now patch 7 has
> 
> #undef BITOP_BITS_PER_WORD
> #undef bitop_uint_t
> 
> #define BITOP_BITS_PER_WORD BITS_PER_LONG
> #define bitop_uint_t unsigned long
> 
> You'd drop the #undef-s and keep the #define-s. You want to override
> them
> both, after all.
> 
> A problem would arise for _another_ arch wanting to use these
> (default)
> types in its asm/bitops.h. Which then could still be solved by having
> a
> types-only header.
This problem arise now for Arm and PPC which use BITOP_BITS_PER_WORD
inside it. Then it is needed to define BITOP_BITS_PER_WORD=32 in
asm/bitops.h for Arm and PPC. If it is okay, then I will happy to
follow this approach.

>  Recall the discussion on the last summit of us meaning
> to switch to such a model anyway (perhaps it being xen/types/bitops.h
> and
> asm/types/bitops.h then), in a broader fashion? IOW for now you could
> use
> the simple approach as long as no other arch needs the types in its
> asm/bitops.h. Later we would introduce the types-only headers, thus
> catering for possible future uses.
Do we really need asm/types/bitops.h? Can't we just do the following in
asm/bitops.h:
  #ifndef BITOP_TYPE
  #include <xen/types/bitops.h>
  #endif

~ Oleksii