Re: [PATCH v3 1/5] xen/ppc: Implement atomic.h

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.09.2023 20:25, Shawn Anastasio wrote:
> +static inline atomic_t atomic_compareandswap(atomic_t old, atomic_t new,
> +                                             atomic_t *v)
> +{
> +    atomic_t rc;
> +    rc.counter = __cmpxchg(&v->counter, old.counter, new.counter, 
> sizeof(int));

I can't seem to be able to spot where __cmpxchg() is defined. (I really
only wanted to check why it needs the 4th argument, which here rather
would want to be e.g. sizeof(v->counter). If it can't be omitted.)

> +    return rc;
> +}
> +
> +#define arch_cmpxchg(ptr, o, n)                                              
>   \
> +    ({                                                                       
>   \
> +        __typeof__(*(ptr)) o_ = (o);                                         
>   \
> +        __typeof__(*(ptr)) n_ = (n);                                         
>   \
> +        (__typeof__(*(ptr))) __cmpxchg((ptr), (unsigned long) o_,            
>   \
> +                                       (unsigned long) n_, sizeof(*(ptr)));  
>   \

Nit: Stray blanks again after cast specifiers.

> --- /dev/null
> +++ b/xen/arch/ppc/include/asm/memory.h
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) IBM Corp. 2005
> + *
> + * Authors: Jimi Xenidis <jimix@xxxxxxxxxxxxxx>
> + */
> +
> +#ifndef _ASM_MEMORY_H_
> +#define _ASM_MEMORY_H_
> +
> +#include <xen/config.h>

As mentioned before - no need for this explicit #include.

> +#ifdef CONFIG_SMP
> +#define PPC_ATOMIC_ENTRY_BARRIER "sync\n"
> +#define PPC_ATOMIC_EXIT_BARRIER  "sync\n"
> +#else
> +#define PPC_ATOMIC_ENTRY_BARRIER
> +#define PPC_ATOMIC_EXIT_BARRIER
> +#endif

Is this correct, considering that we have no CONFIG_SMP and assume SMP
in all cases?

I'm sorry for not paying attention earlier.

Jan