Re: [PATCH v8 02/17] xen: introduce generic non-atomic test_*bit()

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Mon, 2024-05-06 at 08:33 +0200, Jan Beulich wrote:
> On 03.05.2024 19:15, Oleksii wrote:
> > On Thu, 2024-04-25 at 17:35 +0200, Jan Beulich wrote:
> > > >   #include <asm/bitops.h>
> > > >   
> > > > +#ifndef arch_check_bitop_size
> > > > +#define arch_check_bitop_size(addr)
> > > 
> > > Can this really do nothing? Passing the address of an object
> > > smaller
> > > than
> > > bitop_uint_t will read past the object in the generic__*_bit()
> > > functions.
> > It seems RISC-V isn' happy with the following generic definition:
> >    extern void __bitop_bad_size(void);
> >    
> >    /* --------------------- Please tidy above here ----------------
> > ----
> >    - */
> >    
> >    #include <asm/bitops.h>
> >    
> >    #ifndef arch_check_bitop_size
> >    
> >    #define bitop_bad_size(addr) sizeof(*(addr)) <
> > sizeof(bitop_uint_t)
> >    
> >    #define arch_check_bitop_size(addr) \
> >        if ( bitop_bad_size(addr) ) __bitop_bad_size();
> >    
> >    #endif /* arch_check_bitop_size */
> 
> I'm afraid you've re-discovered something that was also found during
> the
> original Arm porting effort. As nice and logical as it would (seem
> to) be
> to have bitop_uint_t match machine word size, there are places ...
> 
> > The following errors occurs. bitop_uint_t for RISC-V is defined as
> > unsigned long for now:
> 
> ... where we assume such operations can be done on 32-bit quantities.
Based on RISC-V spec machine word is 32-bit, so then I can just drop
re-definition of bitop_uint_t in riscv/asm/types.h and use the
definition of bitop_uint_t in xen/types.h.
Also it will be needed to update __AMO() macros in <riscv>/asm/bitops.h
in the following way:
   #if BITOP_BITS_PER_WORD == 64
   #define __AMO(op)   "amo" #op ".d"
   #elif BITOP_BITS_PER_WORD == 32
   #define __AMO(op)   "amo" #op ".w"
   #else
   #error "Unexpected BITS_PER_LONG"
   #endif
Note: BITS_PER_LONG was changed to BITOP_BITS_PER_WORD !

Only one question remains for me. Given that there are some operations whichcan 
be performed on 32-bit quantities, it seems to me that bitop_uint_t
can only be uint32_t.
Am I correct? If yes, do we need to have ability to redefine
bitop_uint_t and BITOP_BITS_PER_WORD in xen/types.h:
   #ifndef BITOP_TYPE
   #define BITOP_BITS_PER_WORD 32
   
   typedef uint32_t bitop_uint_t;
   #endif

~ Oleksii

> 
> Jan
> 
> >     ./common/symbols-dummy.o -o ./.xen-syms.0
> > riscv64-linux-gnu-ld: prelink.o: in function `atomic_sub':
> > /build/xen/./arch/riscv/include/asm/atomic.h:152: undefined
> > reference
> > to `__bitop_bad_size'
> > riscv64-linux-gnu-ld: prelink.o: in function
> > `evtchn_check_pollers':
> > /build/xen/common/event_channel.c:1531: undefined reference to
> > `__bitop_bad_size'
> > riscv64-linux-gnu-ld: /build/xen/common/event_channel.c:1521:
> > undefined
> > reference to `__bitop_bad_size'
> > riscv64-linux-gnu-ld: prelink.o: in function `evtchn_init':
> > /build/xen/common/event_channel.c:1541: undefined reference to
> > `__bitop_bad_size'
> > riscv64-linux-gnu-ld: prelink.o: in function `_read_lock':
> > /build/xen/./include/xen/rwlock.h:94: undefined reference to
> > `__bitop_bad_size'
> > riscv64-linux-gnu-ld:
> > prelink.o:/build/xen/./arch/riscv/include/asm/atomic.h:195: more
> > undefined references to `__bitop_bad_size' follow
> > riscv64-linux-gnu-ld: ./.xen-syms.0: hidden symbol
> > `__bitop_bad_size'
> > isn't defined
> > riscv64-linux-gnu-ld: final link failed: bad value
> > make[2]: *** [arch/riscv/Makefile:15: xen-syms] Error 1
> > 
> > ~ Oleksii
>