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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Fri, 2024-04-05 at 13:53 +0200, Oleksii wrote:
> 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
Or as an options just update <xen/types.h> with after inclusion of
<asm/types.h>:
   #ifndef BITOP_TYPE
      #define BITOP_BITS_PER_WORD 32
      /* typedef uint32_t bitop_uint_t; */
      #define bitop_uint_t uint32_t
   #endif
   
And then just include <xen/types.h> to <<xen/bitops.h>.

~ Oleksii
> 
> ~ Oleksii