Re: [PATCH v8 06/17] xen/riscv: introduce cmpxchg.h

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.04.2024 12:04, Oleksii Kurochko wrote:
> +/*
> + * This function doesn't exist, so you'll get a linker error
> + * if something tries to do an invalid xchg().
> + */
> +extern void __bad_xchg(volatile void *ptr, int size);
> +
> +static always_inline unsigned long __xchg(volatile void *ptr, unsigned long 
> new, int size)
> +{
> +    unsigned long ret;
> +
> +    switch ( size )
> +    {
> +    case 1:
> +        ret = emulate_xchg_1_2((volatile uint8_t *)ptr, new, ".aq", ".aqrl");
> +        break;
> +    case 2:
> +        ret = emulate_xchg_1_2((volatile uint16_t *)ptr, new, ".aq", 
> ".aqrl");
> +        break;
> +    case 4:
> +        _amoswap_generic((volatile uint32_t *)ptr, new, ret, ".w.aqrl");
> +        break;
> +#ifndef CONFIG_RISCV_32
> +    case 8:
> +        _amoswap_generic((volatile uint64_t *)ptr, new, ret, ".d.aqrl");
> +        break;
> +#endif
> +    default:
> +        __bad_xchg(ptr, size), ret = 0;
> +    }

I see no real reason to use a comma expression here, the more that "break"
needs adding anyway. I wonder why here you don't use ...

> +/* This function doesn't exist, so you'll get a linker error
> +   if something tries to do an invalid cmpxchg().  */
> +extern unsigned long __bad_cmpxchg(volatile void *ptr, int size);
> +
> +/*
> + * Atomic compare and exchange.  Compare OLD with MEM, if identical,
> + * store NEW in MEM.  Return the initial value in MEM.  Success is
> + * indicated by comparing RETURN with OLD.
> + */
> +static always_inline unsigned long __cmpxchg(volatile void *ptr,
> +                                             unsigned long old,
> +                                             unsigned long new,
> +                                             int size)
> +{
> +    unsigned long ret;
> +
> +    switch ( size )
> +    {
> +    case 1:
> +        ret = emulate_cmpxchg_1_2((volatile uint8_t *)ptr, old, new,
> +                                  ".aq", ".aqrl");
> +        break;
> +    case 2:
> +        ret = emulate_cmpxchg_1_2((volatile uint16_t *)ptr, old, new,
> +                                   ".aq", ".aqrl");
> +        break;
> +    case 4:
> +        ret = _generic_cmpxchg((volatile uint32_t *)ptr, old, new,
> +                          ".w.aq", ".w.aqrl");
> +        break;
> +#ifndef CONFIG_32BIT
> +    case 8:
> +        ret = _generic_cmpxchg((volatile uint64_t *)ptr, old, new,
> +                           ".d.aq", ".d.aqrl");
> +        break;
> +#endif
> +    default:
> +        return __bad_cmpxchg(ptr, size);

... the approach used here.

Also (style nit) the comment on __bad_cmpxchg() is malformed, unlike that
ahead of __bad_xchg().

Jan