diff mbox series

[v3,15/34] xen/riscv: introduce atomic.h

Message ID 54f5f13a4ee3de3c3cf4ba2b4d0347bb77bb7d08.1703255175.git.oleksii.kurochko@gmail.com (mailing list archive)
State Superseded
Headers show
Series Enable build of full Xen for RISC-V | expand

Commit Message

Oleksii Kurochko Dec. 22, 2023, 3:12 p.m. UTC
From: Bobby Eshleman <bobbyeshleman@gmail.com>

Additionally, this patch introduces macros in fence.h,
which are utilized in atomic.h.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V3:
  - update the commit message
  - add SPDX for fence.h
  - code style fixes
  - Remove /* TODO: ... */ for add_sized macros. It looks correct to me.
  - re-order the patch
  - merge to this patch fence.h
---
Changes in V2:
 - Change an author of commit. I got this header from Bobby's old repo.
---
 xen/arch/riscv/include/asm/atomic.h | 384 ++++++++++++++++++++++++++++
 xen/arch/riscv/include/asm/fence.h  |  13 +
 2 files changed, 397 insertions(+)
 create mode 100644 xen/arch/riscv/include/asm/atomic.h
 create mode 100644 xen/arch/riscv/include/asm/fence.h

Comments

Jan Beulich Jan. 22, 2024, 4:56 p.m. UTC | #1
On 22.12.2023 16:12, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/atomic.h
> @@ -0,0 +1,384 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Taken and modified from Linux.
> + * 
> + * Copyright (C) 2007 Red Hat, Inc. All Rights Reserved.
> + * Copyright (C) 2012 Regents of the University of California
> + * Copyright (C) 2017 SiFive
> + * Copyright (C) 2021 Vates SAS
> + */
> +
> +#ifndef _ASM_RISCV_ATOMIC_H
> +#define _ASM_RISCV_ATOMIC_H
> +
> +#include <xen/atomic.h>
> +#include <asm/cmpxchg.h>
> +#include <asm/fence.h>
> +#include <asm/io.h>
> +#include <asm/system.h>
> +
> +void __bad_atomic_size(void);
> +
> +static always_inline void read_atomic_size(const volatile void *p,
> +                                           void *res,
> +                                           unsigned int size)
> +{
> +    switch ( size )
> +    {
> +    case 1: *(uint8_t *)res = readb((const uint8_t *)p); break;
> +    case 2: *(uint16_t *)res = readw((const uint16_t *)p); break;
> +    case 4: *(uint32_t *)res = readl((const uint32_t *)p); break;
> +    case 8: *(uint32_t *)res  = readq((const uint64_t *)p); break;

Just like const, you should also avoid casting away volatile.

> +    default: __bad_atomic_size(); break;
> +    }
> +}
> +
> +#define read_atomic(p) ({                                               \
> +    union { typeof(*p) val; char c[0]; } x_;                            \
> +    read_atomic_size(p, x_.c, sizeof(*p));                              \
> +    x_.val;                                                             \
> +})
> +
> +

Nit: No double blank lines please.

> +#define write_atomic(p, x) ({                                           \
> +    typeof(*p) x__ = (x);                                               \
> +    switch ( sizeof(*p) )												\
> +    {                                             						\

These lines look excessively long, possibly as a result of leaving hard tabs
in place.

Overall some of the style comments on the earlier patch seem to apply here
as well.

> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/fence.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +#ifndef _ASM_RISCV_FENCE_H
> +#define _ASM_RISCV_FENCE_H
> +
> +#ifdef CONFIG_SMP
> +#define RISCV_ACQUIRE_BARRIER		"\tfence r , rw\n"
> +#define RISCV_RELEASE_BARRIER		"\tfence rw,  w\n"
> +#else
> +#define RISCV_ACQUIRE_BARRIER
> +#define RISCV_RELEASE_BARRIER
> +#endif

Do you really care about the !SMP case? On x86 at least we stopped special-
casing that configuration many years ago (the few cases where for typically
build reasons it matters, using CONFIG_NR_CPUS is sufficient). If you care
about it, there needs to be somewhere you actually #define CONFIG_SMP.

Jan
Oleksii Kurochko Jan. 23, 2024, 10:21 a.m. UTC | #2
On Mon, 2024-01-22 at 17:56 +0100, Jan Beulich wrote:
> On 22.12.2023 16:12, Oleksii Kurochko wrote:
> > --- /dev/null
> > +++ b/xen/arch/riscv/include/asm/atomic.h
> > @@ -0,0 +1,384 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Taken and modified from Linux.
> > + * 
> > + * Copyright (C) 2007 Red Hat, Inc. All Rights Reserved.
> > + * Copyright (C) 2012 Regents of the University of California
> > + * Copyright (C) 2017 SiFive
> > + * Copyright (C) 2021 Vates SAS
> > + */
> > +
> > +#ifndef _ASM_RISCV_ATOMIC_H
> > +#define _ASM_RISCV_ATOMIC_H
> > +
> > +#include <xen/atomic.h>
> > +#include <asm/cmpxchg.h>
> > +#include <asm/fence.h>
> > +#include <asm/io.h>
> > +#include <asm/system.h>
> > +
> > +void __bad_atomic_size(void);
> > +
> > +static always_inline void read_atomic_size(const volatile void *p,
> > +                                           void *res,
> > +                                           unsigned int size)
> > +{
> > +    switch ( size )
> > +    {
> > +    case 1: *(uint8_t *)res = readb((const uint8_t *)p); break;
> > +    case 2: *(uint16_t *)res = readw((const uint16_t *)p); break;
> > +    case 4: *(uint32_t *)res = readl((const uint32_t *)p); break;
> > +    case 8: *(uint32_t *)res  = readq((const uint64_t *)p); break;
> 
> Just like const, you should also avoid casting away volatile.
Thanks. I will drop casting.

> 
> > +    default: __bad_atomic_size(); break;
> > +    }
> > +}
> > +
> > +#define read_atomic(p)
> > ({                                               \
> > +    union { typeof(*p) val; char c[0]; }
> > x_;                            \
> > +    read_atomic_size(p, x_.c,
> > sizeof(*p));                              \
> > +   
> > x_.val;                                                            
> > \
> > +})
> > +
> > +
> 
> Nit: No double blank lines please.
Sure. I'll drtop one blank line.
> 
> > +#define write_atomic(p, x)
> > ({                                           \
> > +    typeof(*p) x__ =
> > (x);                                               \
> > +    switch ( sizeof(*p)
> > )												\
> > +   
> > {                                             						\
> 
> These lines look excessively long, possibly as a result of leaving
> hard tabs
> in place.
> 
> Overall some of the style comments on the earlier patch seem to apply
> here
> as well.
> 
> > --- /dev/null
> > +++ b/xen/arch/riscv/include/asm/fence.h
> > @@ -0,0 +1,13 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +#ifndef _ASM_RISCV_FENCE_H
> > +#define _ASM_RISCV_FENCE_H
> > +
> > +#ifdef CONFIG_SMP
> > +#define RISCV_ACQUIRE_BARRIER		"\tfence r , rw\n"
> > +#define RISCV_RELEASE_BARRIER		"\tfence rw,  w\n"
> > +#else
> > +#define RISCV_ACQUIRE_BARRIER
> > +#define RISCV_RELEASE_BARRIER
> > +#endif
> 
> Do you really care about the !SMP case? On x86 at least we stopped
> special-
> casing that configuration many years ago (the few cases where for
> typically
> build reasons it matters, using CONFIG_NR_CPUS is sufficient). If you
> care
> about it, there needs to be somewhere you actually #define
> CONFIG_SMP.
Can't we use instead of CONFIG_SMP - CONFIG_NR_CPUS? I missed that
CONFIG_SMP is present in Linux, but not in Xen.

~ Oleksii
Jan Beulich Jan. 23, 2024, 10:30 a.m. UTC | #3
On 23.01.2024 11:21, Oleksii wrote:
> On Mon, 2024-01-22 at 17:56 +0100, Jan Beulich wrote:
>> On 22.12.2023 16:12, Oleksii Kurochko wrote:
>>> --- /dev/null
>>> +++ b/xen/arch/riscv/include/asm/fence.h
>>> @@ -0,0 +1,13 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>> +#ifndef _ASM_RISCV_FENCE_H
>>> +#define _ASM_RISCV_FENCE_H
>>> +
>>> +#ifdef CONFIG_SMP
>>> +#define RISCV_ACQUIRE_BARRIER		"\tfence r , rw\n"
>>> +#define RISCV_RELEASE_BARRIER		"\tfence rw,  w\n"
>>> +#else
>>> +#define RISCV_ACQUIRE_BARRIER
>>> +#define RISCV_RELEASE_BARRIER
>>> +#endif
>>
>> Do you really care about the !SMP case? On x86 at least we stopped
>> special-
>> casing that configuration many years ago (the few cases where for
>> typically
>> build reasons it matters, using CONFIG_NR_CPUS is sufficient). If you
>> care
>> about it, there needs to be somewhere you actually #define
>> CONFIG_SMP.
> Can't we use instead of CONFIG_SMP - CONFIG_NR_CPUS?

You can. Question is whether there's a point in doing so. Do you
expect people to actually want to run Xen on single-CPU systems?
They're generally not overly well suited for virtualization ...

Jan
Oleksii Kurochko Jan. 23, 2024, 12:24 p.m. UTC | #4
On Tue, 2024-01-23 at 11:30 +0100, Jan Beulich wrote:
> On 23.01.2024 11:21, Oleksii wrote:
> > On Mon, 2024-01-22 at 17:56 +0100, Jan Beulich wrote:
> > > On 22.12.2023 16:12, Oleksii Kurochko wrote:
> > > > --- /dev/null
> > > > +++ b/xen/arch/riscv/include/asm/fence.h
> > > > @@ -0,0 +1,13 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > > +#ifndef _ASM_RISCV_FENCE_H
> > > > +#define _ASM_RISCV_FENCE_H
> > > > +
> > > > +#ifdef CONFIG_SMP
> > > > +#define RISCV_ACQUIRE_BARRIER		"\tfence r , rw\n"
> > > > +#define RISCV_RELEASE_BARRIER		"\tfence rw,  w\n"
> > > > +#else
> > > > +#define RISCV_ACQUIRE_BARRIER
> > > > +#define RISCV_RELEASE_BARRIER
> > > > +#endif
> > > 
> > > Do you really care about the !SMP case? On x86 at least we
> > > stopped
> > > special-
> > > casing that configuration many years ago (the few cases where for
> > > typically
> > > build reasons it matters, using CONFIG_NR_CPUS is sufficient). If
> > > you
> > > care
> > > about it, there needs to be somewhere you actually #define
> > > CONFIG_SMP.
> > Can't we use instead of CONFIG_SMP - CONFIG_NR_CPUS?
> 
> You can. Question is whether there's a point in doing so. Do you
> expect people to actually want to run Xen on single-CPU systems?
> They're generally not overly well suited for virtualization ...
Just to clarify.

Do you mean physically single based CPU?
Then I don't expect to run Xen on such systems and it is not nesessary
to define *_BARRIER in this case. Should we have to add build error
notification that we don't support single-CPU systems in this header?

If you are speaking about we have ,let it be, 4 CPUs and only 1 CPU is
currently supported by Xen then it still makes sense.

~ Oleksii
Jan Beulich Jan. 23, 2024, 1:30 p.m. UTC | #5
On 23.01.2024 13:24, Oleksii wrote:
> On Tue, 2024-01-23 at 11:30 +0100, Jan Beulich wrote:
>> On 23.01.2024 11:21, Oleksii wrote:
>>> On Mon, 2024-01-22 at 17:56 +0100, Jan Beulich wrote:
>>>> On 22.12.2023 16:12, Oleksii Kurochko wrote:
>>>>> --- /dev/null
>>>>> +++ b/xen/arch/riscv/include/asm/fence.h
>>>>> @@ -0,0 +1,13 @@
>>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>>> +#ifndef _ASM_RISCV_FENCE_H
>>>>> +#define _ASM_RISCV_FENCE_H
>>>>> +
>>>>> +#ifdef CONFIG_SMP
>>>>> +#define RISCV_ACQUIRE_BARRIER		"\tfence r , rw\n"
>>>>> +#define RISCV_RELEASE_BARRIER		"\tfence rw,  w\n"
>>>>> +#else
>>>>> +#define RISCV_ACQUIRE_BARRIER
>>>>> +#define RISCV_RELEASE_BARRIER
>>>>> +#endif
>>>>
>>>> Do you really care about the !SMP case? On x86 at least we
>>>> stopped
>>>> special-
>>>> casing that configuration many years ago (the few cases where for
>>>> typically
>>>> build reasons it matters, using CONFIG_NR_CPUS is sufficient). If
>>>> you
>>>> care
>>>> about it, there needs to be somewhere you actually #define
>>>> CONFIG_SMP.
>>> Can't we use instead of CONFIG_SMP - CONFIG_NR_CPUS?
>>
>> You can. Question is whether there's a point in doing so. Do you
>> expect people to actually want to run Xen on single-CPU systems?
>> They're generally not overly well suited for virtualization ...
> Just to clarify.
> 
> Do you mean physically single based CPU?
> Then I don't expect to run Xen on such systems and it is not nesessary
> to define *_BARRIER in this case. Should we have to add build error
> notification that we don't support single-CPU systems in this header?
> 
> If you are speaking about we have ,let it be, 4 CPUs and only 1 CPU is
> currently supported by Xen then it still makes sense.

No, that's still not what I mean. The question is: Is it useful for you
to _special case_ the NR_CPUS=1 case? Or is it instead simpler to handle
NR_CPUS=1 the same as NR_CPUS>1 (accepting less than ideal performance,
on the basis that in reality nobody's expected to use such in production
anyway)?

Jan
Oleksii Kurochko Jan. 24, 2024, 9:23 a.m. UTC | #6
On Tue, 2024-01-23 at 14:30 +0100, Jan Beulich wrote:
> On 23.01.2024 13:24, Oleksii wrote:
> > On Tue, 2024-01-23 at 11:30 +0100, Jan Beulich wrote:
> > > On 23.01.2024 11:21, Oleksii wrote:
> > > > On Mon, 2024-01-22 at 17:56 +0100, Jan Beulich wrote:
> > > > > On 22.12.2023 16:12, Oleksii Kurochko wrote:
> > > > > > --- /dev/null
> > > > > > +++ b/xen/arch/riscv/include/asm/fence.h
> > > > > > @@ -0,0 +1,13 @@
> > > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > > > > +#ifndef _ASM_RISCV_FENCE_H
> > > > > > +#define _ASM_RISCV_FENCE_H
> > > > > > +
> > > > > > +#ifdef CONFIG_SMP
> > > > > > +#define RISCV_ACQUIRE_BARRIER		"\tfence r , rw\n"
> > > > > > +#define RISCV_RELEASE_BARRIER		"\tfence rw,  w\n"
> > > > > > +#else
> > > > > > +#define RISCV_ACQUIRE_BARRIER
> > > > > > +#define RISCV_RELEASE_BARRIER
> > > > > > +#endif
> > > > > 
> > > > > Do you really care about the !SMP case? On x86 at least we
> > > > > stopped
> > > > > special-
> > > > > casing that configuration many years ago (the few cases where
> > > > > for
> > > > > typically
> > > > > build reasons it matters, using CONFIG_NR_CPUS is
> > > > > sufficient). If
> > > > > you
> > > > > care
> > > > > about it, there needs to be somewhere you actually #define
> > > > > CONFIG_SMP.
> > > > Can't we use instead of CONFIG_SMP - CONFIG_NR_CPUS?
> > > 
> > > You can. Question is whether there's a point in doing so. Do you
> > > expect people to actually want to run Xen on single-CPU systems?
> > > They're generally not overly well suited for virtualization ...
> > Just to clarify.
> > 
> > Do you mean physically single based CPU?
> > Then I don't expect to run Xen on such systems and it is not
> > nesessary
> > to define *_BARRIER in this case. Should we have to add build error
> > notification that we don't support single-CPU systems in this
> > header?
> > 
> > If you are speaking about we have ,let it be, 4 CPUs and only 1 CPU
> > is
> > currently supported by Xen then it still makes sense.
> 
> No, that's still not what I mean. The question is: Is it useful for
> you
> to _special case_ the NR_CPUS=1 case? Or is it instead simpler to
> handle
> NR_CPUS=1 the same as NR_CPUS>1 (accepting less than ideal
> performance,
> on the basis that in reality nobody's expected to use such in
> production
> anyway)?
NR_CPUS=1 sometimes is useful for debugging. At least, at the start I
used that several times, but ITBO I don't remember when I used that
case after SMP support was added and context_switch() was fixed.

Probably, I misunderstand the real idea of NR_CPUS. Does NR_CPUS
represent a number of logical CPUs which can be different from physical
amount of CPU?
If yes, then what I wrote above it was about physical CPU and then in
context of logical CPUs I don't need a special case when NR_CPUS=1.

~ Oleksii
Jan Beulich Jan. 24, 2024, 11:19 a.m. UTC | #7
On 24.01.2024 10:23, Oleksii wrote:
> On Tue, 2024-01-23 at 14:30 +0100, Jan Beulich wrote:
>> On 23.01.2024 13:24, Oleksii wrote:
>>> On Tue, 2024-01-23 at 11:30 +0100, Jan Beulich wrote:
>>>> On 23.01.2024 11:21, Oleksii wrote:
>>>>> On Mon, 2024-01-22 at 17:56 +0100, Jan Beulich wrote:
>>>>>> On 22.12.2023 16:12, Oleksii Kurochko wrote:
>>>>>>> --- /dev/null
>>>>>>> +++ b/xen/arch/riscv/include/asm/fence.h
>>>>>>> @@ -0,0 +1,13 @@
>>>>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>>>>> +#ifndef _ASM_RISCV_FENCE_H
>>>>>>> +#define _ASM_RISCV_FENCE_H
>>>>>>> +
>>>>>>> +#ifdef CONFIG_SMP
>>>>>>> +#define RISCV_ACQUIRE_BARRIER		"\tfence r , rw\n"
>>>>>>> +#define RISCV_RELEASE_BARRIER		"\tfence rw,  w\n"
>>>>>>> +#else
>>>>>>> +#define RISCV_ACQUIRE_BARRIER
>>>>>>> +#define RISCV_RELEASE_BARRIER
>>>>>>> +#endif
>>>>>>
>>>>>> Do you really care about the !SMP case? On x86 at least we
>>>>>> stopped
>>>>>> special-
>>>>>> casing that configuration many years ago (the few cases where
>>>>>> for
>>>>>> typically
>>>>>> build reasons it matters, using CONFIG_NR_CPUS is
>>>>>> sufficient). If
>>>>>> you
>>>>>> care
>>>>>> about it, there needs to be somewhere you actually #define
>>>>>> CONFIG_SMP.
>>>>> Can't we use instead of CONFIG_SMP - CONFIG_NR_CPUS?
>>>>
>>>> You can. Question is whether there's a point in doing so. Do you
>>>> expect people to actually want to run Xen on single-CPU systems?
>>>> They're generally not overly well suited for virtualization ...
>>> Just to clarify.
>>>
>>> Do you mean physically single based CPU?
>>> Then I don't expect to run Xen on such systems and it is not
>>> nesessary
>>> to define *_BARRIER in this case. Should we have to add build error
>>> notification that we don't support single-CPU systems in this
>>> header?
>>>
>>> If you are speaking about we have ,let it be, 4 CPUs and only 1 CPU
>>> is
>>> currently supported by Xen then it still makes sense.
>>
>> No, that's still not what I mean. The question is: Is it useful for
>> you
>> to _special case_ the NR_CPUS=1 case? Or is it instead simpler to
>> handle
>> NR_CPUS=1 the same as NR_CPUS>1 (accepting less than ideal
>> performance,
>> on the basis that in reality nobody's expected to use such in
>> production
>> anyway)?
> NR_CPUS=1 sometimes is useful for debugging. At least, at the start I
> used that several times, but ITBO I don't remember when I used that
> case after SMP support was added and context_switch() was fixed.

And "sometimes is useful for debugging" warrants introducing special
cases? I've not suggested disallowing that configuration. I'm merely
asking whether it isn't easier to have the barriers there at all
times. Just like on x86 we now leave the LOCK prefixes in place at
all times.

> Probably, I misunderstand the real idea of NR_CPUS. Does NR_CPUS
> represent a number of logical CPUs which can be different from physical
> amount of CPU?

No.

Jan
Oleksii Kurochko Jan. 24, 2024, 2:56 p.m. UTC | #8
On Wed, 2024-01-24 at 12:19 +0100, Jan Beulich wrote:
> On 24.01.2024 10:23, Oleksii wrote:
> > On Tue, 2024-01-23 at 14:30 +0100, Jan Beulich wrote:
> > > On 23.01.2024 13:24, Oleksii wrote:
> > > > On Tue, 2024-01-23 at 11:30 +0100, Jan Beulich wrote:
> > > > > On 23.01.2024 11:21, Oleksii wrote:
> > > > > > On Mon, 2024-01-22 at 17:56 +0100, Jan Beulich wrote:
> > > > > > > On 22.12.2023 16:12, Oleksii Kurochko wrote:
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/xen/arch/riscv/include/asm/fence.h
> > > > > > > > @@ -0,0 +1,13 @@
> > > > > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > > > > > > +#ifndef _ASM_RISCV_FENCE_H
> > > > > > > > +#define _ASM_RISCV_FENCE_H
> > > > > > > > +
> > > > > > > > +#ifdef CONFIG_SMP
> > > > > > > > +#define RISCV_ACQUIRE_BARRIER		"\tfence r ,
> > > > > > > > rw\n"
> > > > > > > > +#define RISCV_RELEASE_BARRIER		"\tfence rw, 
> > > > > > > > w\n"
> > > > > > > > +#else
> > > > > > > > +#define RISCV_ACQUIRE_BARRIER
> > > > > > > > +#define RISCV_RELEASE_BARRIER
> > > > > > > > +#endif
> > > > > > > 
> > > > > > > Do you really care about the !SMP case? On x86 at least
> > > > > > > we
> > > > > > > stopped
> > > > > > > special-
> > > > > > > casing that configuration many years ago (the few cases
> > > > > > > where
> > > > > > > for
> > > > > > > typically
> > > > > > > build reasons it matters, using CONFIG_NR_CPUS is
> > > > > > > sufficient). If
> > > > > > > you
> > > > > > > care
> > > > > > > about it, there needs to be somewhere you actually
> > > > > > > #define
> > > > > > > CONFIG_SMP.
> > > > > > Can't we use instead of CONFIG_SMP - CONFIG_NR_CPUS?
> > > > > 
> > > > > You can. Question is whether there's a point in doing so. Do
> > > > > you
> > > > > expect people to actually want to run Xen on single-CPU
> > > > > systems?
> > > > > They're generally not overly well suited for virtualization
> > > > > ...
> > > > Just to clarify.
> > > > 
> > > > Do you mean physically single based CPU?
> > > > Then I don't expect to run Xen on such systems and it is not
> > > > nesessary
> > > > to define *_BARRIER in this case. Should we have to add build
> > > > error
> > > > notification that we don't support single-CPU systems in this
> > > > header?
> > > > 
> > > > If you are speaking about we have ,let it be, 4 CPUs and only 1
> > > > CPU
> > > > is
> > > > currently supported by Xen then it still makes sense.
> > > 
> > > No, that's still not what I mean. The question is: Is it useful
> > > for
> > > you
> > > to _special case_ the NR_CPUS=1 case? Or is it instead simpler to
> > > handle
> > > NR_CPUS=1 the same as NR_CPUS>1 (accepting less than ideal
> > > performance,
> > > on the basis that in reality nobody's expected to use such in
> > > production
> > > anyway)?
> > NR_CPUS=1 sometimes is useful for debugging. At least, at the start
> > I
> > used that several times, but ITBO I don't remember when I used that
> > case after SMP support was added and context_switch() was fixed.
> 
> And "sometimes is useful for debugging" warrants introducing special
> cases? I've not suggested disallowing that configuration. I'm merely
> asking whether it isn't easier to have the barriers there at all
> times. Just like on x86 we now leave the LOCK prefixes in place at
> all times.
I misunderstood your initial suggestion. In this case we can always
have the barriers. I'll drop then #ifdef CONFIG_SMP.

Thanks for clarification.

~ Oleksii
> 
> > Probably, I misunderstand the real idea of NR_CPUS. Does NR_CPUS
> > represent a number of logical CPUs which can be different from
> > physical
> > amount of CPU?
> 
> No.
> 
> Jan
diff mbox series

Patch

diff --git a/xen/arch/riscv/include/asm/atomic.h b/xen/arch/riscv/include/asm/atomic.h
new file mode 100644
index 0000000000..725326a9d1
--- /dev/null
+++ b/xen/arch/riscv/include/asm/atomic.h
@@ -0,0 +1,384 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Taken and modified from Linux.
+ * 
+ * Copyright (C) 2007 Red Hat, Inc. All Rights Reserved.
+ * Copyright (C) 2012 Regents of the University of California
+ * Copyright (C) 2017 SiFive
+ * Copyright (C) 2021 Vates SAS
+ */
+
+#ifndef _ASM_RISCV_ATOMIC_H
+#define _ASM_RISCV_ATOMIC_H
+
+#include <xen/atomic.h>
+#include <asm/cmpxchg.h>
+#include <asm/fence.h>
+#include <asm/io.h>
+#include <asm/system.h>
+
+void __bad_atomic_size(void);
+
+static always_inline void read_atomic_size(const volatile void *p,
+                                           void *res,
+                                           unsigned int size)
+{
+    switch ( size )
+    {
+    case 1: *(uint8_t *)res = readb((const uint8_t *)p); break;
+    case 2: *(uint16_t *)res = readw((const uint16_t *)p); break;
+    case 4: *(uint32_t *)res = readl((const uint32_t *)p); break;
+    case 8: *(uint32_t *)res  = readq((const uint64_t *)p); break;
+    default: __bad_atomic_size(); break;
+    }
+}
+
+#define read_atomic(p) ({                                               \
+    union { typeof(*p) val; char c[0]; } x_;                            \
+    read_atomic_size(p, x_.c, sizeof(*p));                              \
+    x_.val;                                                             \
+})
+
+
+#define write_atomic(p, x) ({                                           \
+    typeof(*p) x__ = (x);                                               \
+    switch ( sizeof(*p) )												\
+    {                                             						\
+    case 1: writeb((uint8_t)x__,  (uint8_t *)  p); break;              	\
+    case 2: writew((uint16_t)x__, (uint16_t *) p); break;              	\
+    case 4: writel((uint32_t)x__, (uint32_t *) p); break;              	\
+    case 8: writeq((uint64_t)x__, (uint64_t *) p); break;              	\
+    default: __bad_atomic_size(); break;                                \
+    }                                                                   \
+    x__;                                                                \
+})
+
+#define add_sized(p, x) ({                                              \
+    typeof(*(p)) x__ = (x);                                             \
+    switch ( sizeof(*(p)) )                                             \
+    {                                                                   \
+    case 1: writeb(read_atomic(p) + x__, (uint8_t *)(p)); break;        \
+    case 2: writew(read_atomic(p) + x__, (uint16_t *)(p)); break;       \
+    case 4: writel(read_atomic(p) + x__, (uint32_t *)(p)); break;       \
+    default: __bad_atomic_size(); break;                                \
+    }                                                                   \
+})
+
+/*
+ *  __unqual_scalar_typeof(x) - Declare an unqualified scalar type, leaving
+ *               non-scalar types unchanged.
+ *
+ * Prefer C11 _Generic for better compile-times and simpler code. Note: 'char'
+ * is not type-compatible with 'signed char', and we define a separate case.
+ */
+#define __scalar_type_to_expr_cases(type)               \
+    unsigned type:  (unsigned type)0,                   \
+    signed type:    (signed type)0
+
+#define __unqual_scalar_typeof(x) typeof(               \
+    _Generic((x),                                       \
+        char:  (char)0,                                 \
+        __scalar_type_to_expr_cases(char),              \
+        __scalar_type_to_expr_cases(short),             \
+        __scalar_type_to_expr_cases(int),               \
+        __scalar_type_to_expr_cases(long),              \
+        __scalar_type_to_expr_cases(long long),         \
+        default: (x)))
+
+#define READ_ONCE(x)  (*(const volatile __unqual_scalar_typeof(x) *)&(x))
+#define WRITE_ONCE(x, val)                                      \
+    do {                                                        \
+        *(volatile typeof(x) *)&(x) = (val);                    \
+    } while (0)
+
+#define __atomic_acquire_fence() \
+    __asm__ __volatile__( RISCV_ACQUIRE_BARRIER "" ::: "memory" )
+
+#define __atomic_release_fence() \
+    __asm__ __volatile__( RISCV_RELEASE_BARRIER "" ::: "memory" );
+
+static inline int atomic_read(const atomic_t *v)
+{
+    return READ_ONCE(v->counter);
+}
+
+static inline int _atomic_read(atomic_t v)
+{
+    return v.counter;
+}
+
+static inline void atomic_set(atomic_t *v, int i)
+{
+    WRITE_ONCE(v->counter, i);
+}
+
+static inline void _atomic_set(atomic_t *v, int i)
+{
+    v->counter = i;
+}
+
+static inline int atomic_sub_and_test(int i, atomic_t *v)
+{
+    return atomic_sub_return(i, v) == 0;
+}
+
+static inline void atomic_inc(atomic_t *v)
+{
+    atomic_add(1, v);
+}
+
+static inline int atomic_inc_return(atomic_t *v)
+{
+    return atomic_add_return(1, v);
+}
+
+static inline void atomic_dec(atomic_t *v)
+{
+    atomic_sub(1, v);
+}
+
+static inline int atomic_dec_return(atomic_t *v)
+{
+    return atomic_sub_return(1, v);
+}
+
+static inline int atomic_dec_and_test(atomic_t *v)
+{
+    return atomic_sub_return(1, v) == 0;
+}
+
+static inline int atomic_add_negative(int i, atomic_t *v)
+{
+    return atomic_add_return(i, v) < 0;
+}
+
+static inline int atomic_inc_and_test(atomic_t *v)
+{
+    return atomic_add_return(1, v) == 0;
+}
+
+/*
+ * First, the atomic ops that have no ordering constraints and therefor don't
+ * have the AQ or RL bits set.  These don't return anything, so there's only
+ * one version to worry about.
+ */
+#define ATOMIC_OP(op, asm_op, I, asm_type, c_type, prefix)  \
+static inline                                               \
+void atomic##prefix##_##op(c_type i, atomic##prefix##_t *v) \
+{                                                           \
+    __asm__ __volatile__ (                                  \
+        "   amo" #asm_op "." #asm_type " zero, %1, %0"      \
+        : "+A" (v->counter)                                 \
+        : "r" (I)                                           \
+        : "memory" );                                       \
+}                                                           \
+
+#define ATOMIC_OPS(op, asm_op, I)                           \
+        ATOMIC_OP (op, asm_op, I, w, int,   )
+
+ATOMIC_OPS(add, add,  i)
+ATOMIC_OPS(sub, add, -i)
+ATOMIC_OPS(and, and,  i)
+ATOMIC_OPS( or,  or,  i)
+ATOMIC_OPS(xor, xor,  i)
+
+#undef ATOMIC_OP
+#undef ATOMIC_OPS
+
+/*
+ * Atomic ops that have ordered, relaxed, acquire, and release variants.
+ * There's two flavors of these: the arithmatic ops have both fetch and return
+ * versions, while the logical ops only have fetch versions.
+ */
+#define ATOMIC_FETCH_OP(op, asm_op, I, asm_type, c_type, prefix)    \
+static inline                                                       \
+c_type atomic##prefix##_fetch_##op##_relaxed(c_type i,              \
+                         atomic##prefix##_t *v)                     \
+{                                                                   \
+    register c_type ret;                                            \
+    __asm__ __volatile__ (                                          \
+        "   amo" #asm_op "." #asm_type " %1, %2, %0"                \
+        : "+A" (v->counter), "=r" (ret)                             \
+        : "r" (I)                                                   \
+        : "memory" );                                               \
+    return ret;                                                     \
+}                                                                   \
+static inline                                                       \
+c_type atomic##prefix##_fetch_##op(c_type i, atomic##prefix##_t *v) \
+{                                                                   \
+    register c_type ret;                                            \
+    __asm__ __volatile__ (                                          \
+        "   amo" #asm_op "." #asm_type ".aqrl  %1, %2, %0"          \
+        : "+A" (v->counter), "=r" (ret)                             \
+        : "r" (I)                                                   \
+        : "memory" );                                               \
+    return ret;                                                     \
+}
+
+#define ATOMIC_OP_RETURN(op, asm_op, c_op, I, asm_type, c_type, prefix) \
+static inline                                                           \
+c_type atomic##prefix##_##op##_return_relaxed(c_type i,                 \
+                          atomic##prefix##_t *v)                        \
+{                                                                       \
+        return atomic##prefix##_fetch_##op##_relaxed(i, v) c_op I;      \
+}                                                                       \
+static inline                                                           \
+c_type atomic##prefix##_##op##_return(c_type i, atomic##prefix##_t *v)  \
+{                                                                       \
+        return atomic##prefix##_fetch_##op(i, v) c_op I;                \
+}
+
+#define ATOMIC_OPS(op, asm_op, c_op, I)                                 \
+        ATOMIC_FETCH_OP( op, asm_op,       I, w, int,   )               \
+        ATOMIC_OP_RETURN(op, asm_op, c_op, I, w, int,   )
+
+ATOMIC_OPS(add, add, +,  i)
+ATOMIC_OPS(sub, add, +, -i)
+
+#define atomic_add_return_relaxed   atomic_add_return_relaxed
+#define atomic_sub_return_relaxed   atomic_sub_return_relaxed
+#define atomic_add_return   atomic_add_return
+#define atomic_sub_return   atomic_sub_return
+
+#define atomic_fetch_add_relaxed    atomic_fetch_add_relaxed
+#define atomic_fetch_sub_relaxed    atomic_fetch_sub_relaxed
+#define atomic_fetch_add    atomic_fetch_add
+#define atomic_fetch_sub    atomic_fetch_sub
+
+#undef ATOMIC_OPS
+
+#define ATOMIC_OPS(op, asm_op, I) \
+        ATOMIC_FETCH_OP(op, asm_op, I, w, int,   )
+
+ATOMIC_OPS(and, and, i)
+ATOMIC_OPS( or,  or, i)
+ATOMIC_OPS(xor, xor, i)
+
+#define atomic_fetch_and_relaxed    atomic_fetch_and_relaxed
+#define atomic_fetch_or_relaxed	    atomic_fetch_or_relaxed
+#define atomic_fetch_xor_relaxed    atomic_fetch_xor_relaxed
+#define atomic_fetch_and    atomic_fetch_and
+#define atomic_fetch_or     atomic_fetch_or
+#define atomic_fetch_xor    atomic_fetch_xor
+
+#undef ATOMIC_OPS
+
+#undef ATOMIC_FETCH_OP
+#undef ATOMIC_OP_RETURN
+
+/* This is required to provide a full barrier on success. */
+static inline int atomic_add_unless(atomic_t *v, int a, int u)
+{
+       int prev, rc;
+
+    __asm__ __volatile__ (
+        "0: lr.w     %[p],  %[c]\n"
+        "   beq      %[p],  %[u], 1f\n"
+        "   add      %[rc], %[p], %[a]\n"
+        "   sc.w.rl  %[rc], %[rc], %[c]\n"
+        "   bnez     %[rc], 0b\n"
+        "   fence    rw, rw\n"
+        "1:\n"
+        : [p]"=&r" (prev), [rc]"=&r" (rc), [c]"+A" (v->counter)
+        : [a]"r" (a), [u]"r" (u)
+        : "memory");
+    return prev;
+}
+#define atomic_fetch_add_unless atomic_fetch_add_unless
+
+/*
+ * atomic_{cmp,}xchg is required to have exactly the same ordering semantics as
+ * {cmp,}xchg and the operations that return, so they need a full barrier.
+ */
+#define ATOMIC_OP(c_t, prefix, size)                            \
+static inline                                                   \
+c_t atomic##prefix##_xchg_relaxed(atomic##prefix##_t *v, c_t n) \
+{                                                               \
+    return __xchg_relaxed(&(v->counter), n, size);              \
+}                                                               \
+static inline                                                   \
+c_t atomic##prefix##_xchg_acquire(atomic##prefix##_t *v, c_t n) \
+{                                                               \
+    return __xchg_acquire(&(v->counter), n, size);              \
+}                                                               \
+static inline                                                   \
+c_t atomic##prefix##_xchg_release(atomic##prefix##_t *v, c_t n) \
+{                                                               \
+    return __xchg_release(&(v->counter), n, size);              \
+}                                                               \
+static inline                                                   \
+c_t atomic##prefix##_xchg(atomic##prefix##_t *v, c_t n)         \
+{                                                               \
+    return __xchg(&(v->counter), n, size);                      \
+}                                                               \
+static inline                                                   \
+c_t atomic##prefix##_cmpxchg_relaxed(atomic##prefix##_t *v,     \
+                     c_t o, c_t n)                              \
+{                                                               \
+    return __cmpxchg_relaxed(&(v->counter), o, n, size);        \
+}                                                               \
+static inline                                                   \
+c_t atomic##prefix##_cmpxchg_acquire(atomic##prefix##_t *v,     \
+                     c_t o, c_t n)                              \
+{                                                               \
+    return __cmpxchg_acquire(&(v->counter), o, n, size);        \
+}                                                               \
+static inline                                                   \
+c_t atomic##prefix##_cmpxchg_release(atomic##prefix##_t *v,     \
+                     c_t o, c_t n)                              \
+{	                                                            \
+    return __cmpxchg_release(&(v->counter), o, n, size);        \
+}                                                               \
+static inline                                                   \
+c_t atomic##prefix##_cmpxchg(atomic##prefix##_t *v, c_t o, c_t n) \
+{                                                               \
+    return __cmpxchg(&(v->counter), o, n, size);                \
+}
+
+#define ATOMIC_OPS() \
+    ATOMIC_OP(int,   , 4)
+
+ATOMIC_OPS()
+
+#define atomic_xchg_relaxed atomic_xchg_relaxed
+#define atomic_xchg_acquire atomic_xchg_acquire
+#define atomic_xchg_release atomic_xchg_release
+#define atomic_xchg atomic_xchg
+#define atomic_cmpxchg_relaxed atomic_cmpxchg_relaxed
+#define atomic_cmpxchg_acquire atomic_cmpxchg_acquire
+#define atomic_cmpxchg_release atomic_cmpxchg_release
+#define atomic_cmpxchg atomic_cmpxchg
+
+#undef ATOMIC_OPS
+#undef ATOMIC_OP
+
+static inline int atomic_sub_if_positive(atomic_t *v, int offset)
+{
+       int prev, rc;
+
+    __asm__ __volatile__ (
+        "0: lr.w     %[p],  %[c]\n"
+        "   sub      %[rc], %[p], %[o]\n"
+        "   bltz     %[rc], 1f\n"
+        "   sc.w.rl  %[rc], %[rc], %[c]\n"
+        "   bnez     %[rc], 0b\n"
+        "   fence    rw, rw\n"
+        "1:\n"
+        : [p]"=&r" (prev), [rc]"=&r" (rc), [c]"+A" (v->counter)
+        : [o]"r" (offset)
+        : "memory" );
+    return prev - offset;
+}
+
+#define atomic_dec_if_positive(v)	atomic_sub_if_positive(v, 1)
+
+#endif /* _ASM_RISCV_ATOMIC_H */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/riscv/include/asm/fence.h b/xen/arch/riscv/include/asm/fence.h
new file mode 100644
index 0000000000..b3f6b1c20a
--- /dev/null
+++ b/xen/arch/riscv/include/asm/fence.h
@@ -0,0 +1,13 @@ 
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#ifndef _ASM_RISCV_FENCE_H
+#define _ASM_RISCV_FENCE_H
+
+#ifdef CONFIG_SMP
+#define RISCV_ACQUIRE_BARRIER		"\tfence r , rw\n"
+#define RISCV_RELEASE_BARRIER		"\tfence rw,  w\n"
+#else
+#define RISCV_ACQUIRE_BARRIER
+#define RISCV_RELEASE_BARRIER
+#endif
+
+#endif	/* _ASM_RISCV_FENCE_H */