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

[Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>]

On 9/5/23 9:58 AM, Jan Beulich wrote:
> 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.)
>

As you later saw, it's defined in system.h later in patch 3. This was an
error I made when splitting up the changes into this patchset. If you're
fine with it I'll just add a mention in the commit message that the
definitions in this commit are not yet fully usable.

Also I will change the size parameter to sizeof(v->counter) per your
suggestion.

>> +    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.
>

Will fix.

>> --- /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.
>

Will drop.

>> +#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.
>

Good observation, and no problem. I will remove the ifdef and the !SMP
case.

> Jan

Thanks,
Shawn