diff mbox series

[v2,13/39] xen/riscv: introduce asm/system.h

Message ID 3007fb3d841dbaa078bb5b7af699c883356badca.1700761381.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 Nov. 24, 2023, 10:30 a.m. UTC 
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V2:
 - Nothing changed. Only rebase.
---
 xen/arch/riscv/include/asm/system.h | 79 +++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)
 create mode 100644 xen/arch/riscv/include/asm/system.h

Comments

Jan Beulich Dec. 7, 2023, 3:07 p.m. UTC | #1 
On 24.11.2023 11:30, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/system.h
> @@ -0,0 +1,79 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef _ASM_RISCV_BARRIER_H
> +#define _ASM_RISCV_BARRIER_H
> +
> +#include <asm/csr.h>
> +
> +#ifndef __ASSEMBLY__
> +
> +#define RISCV_FENCE(p, s) \
> +    __asm__ __volatile__ ("fence " #p "," #s : : : "memory")

Nit (style): Missing blanks immediately inside the parentheses.

> +/* These barriers need to enforce ordering on both devices or memory. */
> +#define mb()                    RISCV_FENCE(iorw,iorw)
> +#define rmb()                   RISCV_FENCE(ir,ir)
> +#define wmb()                   RISCV_FENCE(ow,ow)

Nit (style): Missing blanks after the commas (also again below).

> +/* These barriers do not need to enforce ordering on devices, just memory. */
> +#define smp_mb()                RISCV_FENCE(rw,rw)
> +#define smp_rmb()               RISCV_FENCE(r,r)
> +#define smp_wmb()               RISCV_FENCE(w,w)
> +#define smp_mb__before_atomic() smp_mb()
> +#define smp_mb__after_atomic()  smp_mb()
> +
> +/*
> +#define __smp_store_release(p, v)       \

Is there a need for the double underscores here? We try to not
introduce new instances of undue leading underscores, but there might
be e.g. a strong desire to stay in sync with, say, Linux.

> +do {                                    \
> +	compiletime_assert_atomic_type(*p); \
> +	RISCV_FENCE(rw,w);                  \
> +	WRITE_ONCE(*p, v);                  \

Nit: Can the trailing backslashes be aligned, please?

> +} while (0)
> +
> +#define __smp_load_acquire(p)           \
> +({                                      \
> +    typeof(*p) ___p1 = READ_ONCE(*p);   \

Hmm, yet more leading underscores, and here surely not needed.

> +    compiletime_assert_atomic_type(*p); \
> +    RISCV_FENCE(r,rw);                  \
> +    ___p1;                              \
> +})
> +*/
> +
> +static inline unsigned long local_save_flags(void)
> +{
> +    return csr_read(sstatus);
> +}
> +
> +static inline void local_irq_enable(void)
> +{
> +    csr_set(sstatus, SSTATUS_SIE);
> +}
> +
> +static inline void local_irq_disable(void)
> +{
> +    csr_clear(sstatus, SSTATUS_SIE);
> +}
> +
> +#define local_irq_save(x)                           \
> +({                                                  \
> +    x = csr_read_clear(CSR_SSTATUS, SSTATUS_SIE);   \
> +    local_irq_disable();                            \
> +})
> +
> +static inline void local_irq_restore(unsigned long flags)
> +{
> +	csr_set(CSR_SSTATUS, flags & SSTATUS_SIE);
> +}
> +
> +static inline int local_irq_is_enabled(void)
> +{
> +    unsigned long flags = local_save_flags();
> +
> +    return flags & SSTATUS_SIE;

SSTATUS_SIE doesn't even happen to be 1, so I think you're better off
adding != 0, unless you would do as I think I had suggested before and
have the function return bool right away.

Jan
Oleksii Kurochko Dec. 8, 2023, 9:43 a.m. UTC | #2 
On Thu, 2023-12-07 at 16:07 +0100, Jan Beulich wrote:
> On 24.11.2023 11:30, Oleksii Kurochko wrote:
> > --- /dev/null
> > +++ b/xen/arch/riscv/include/asm/system.h
> > @@ -0,0 +1,79 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +#ifndef _ASM_RISCV_BARRIER_H
> > +#define _ASM_RISCV_BARRIER_H
> > +
> > +#include <asm/csr.h>
> > +
> > +#ifndef __ASSEMBLY__
> > +
> > +#define RISCV_FENCE(p, s) \
> > +    __asm__ __volatile__ ("fence " #p "," #s : : : "memory")
> 
> Nit (style): Missing blanks immediately inside the parentheses.
Thanks for the comment. I'll update code style.

> 
> > +/* These barriers need to enforce ordering on both devices or
> > memory. */
> > +#define mb()                    RISCV_FENCE(iorw,iorw)
> > +#define rmb()                   RISCV_FENCE(ir,ir)
> > +#define wmb()                   RISCV_FENCE(ow,ow)
> 
> Nit (style): Missing blanks after the commas (also again below).
Thanks for the comment. I'll update code style.

> 
> > +/* These barriers do not need to enforce ordering on devices, just
> > memory. */
> > +#define smp_mb()                RISCV_FENCE(rw,rw)
> > +#define smp_rmb()               RISCV_FENCE(r,r)
> > +#define smp_wmb()               RISCV_FENCE(w,w)
> > +#define smp_mb__before_atomic() smp_mb()
> > +#define smp_mb__after_atomic()  smp_mb()
> > +
> > +/*
> > +#define __smp_store_release(p, v)       \
> 
> Is there a need for the double underscores here? We try to not
> introduce new instances of undue leading underscores, but there might
> be e.g. a strong desire to stay in sync with, say, Linux.
I don't have such a strong desire to be in sync with Linux so let's
stick to Xen code style. I'll update this place in next patch version.

> 
> > +do {                                    \
> > +	compiletime_assert_atomic_type(*p); \
> > +	RISCV_FENCE(rw,w);                  \
> > +	WRITE_ONCE(*p, v);                  \
> 
> Nit: Can the trailing backslashes be aligned, please?
Sure. I'll aligned them. Thanks.
> 
> > +} while (0)
> > +
> > +#define __smp_load_acquire(p)           \
> > +({                                      \
> > +    typeof(*p) ___p1 = READ_ONCE(*p);   \
> 
> Hmm, yet more leading underscores, and here surely not needed.
I'll update the code according to your recommendation. Thanks.

> 
> > +    compiletime_assert_atomic_type(*p); \
> > +    RISCV_FENCE(r,rw);                  \
> > +    ___p1;                              \
> > +})
> > +*/
> > +
> > +static inline unsigned long local_save_flags(void)
> > +{
> > +    return csr_read(sstatus);
> > +}
> > +
> > +static inline void local_irq_enable(void)
> > +{
> > +    csr_set(sstatus, SSTATUS_SIE);
> > +}
> > +
> > +static inline void local_irq_disable(void)
> > +{
> > +    csr_clear(sstatus, SSTATUS_SIE);
> > +}
> > +
> > +#define local_irq_save(x)                           \
> > +({                                                  \
> > +    x = csr_read_clear(CSR_SSTATUS, SSTATUS_SIE);   \
> > +    local_irq_disable();                            \
> > +})
> > +
> > +static inline void local_irq_restore(unsigned long flags)
> > +{
> > +	csr_set(CSR_SSTATUS, flags & SSTATUS_SIE);
> > +}
> > +
> > +static inline int local_irq_is_enabled(void)
> > +{
> > +    unsigned long flags = local_save_flags();
> > +
> > +    return flags & SSTATUS_SIE;
> 
> SSTATUS_SIE doesn't even happen to be 1, so I think you're better off
> adding != 0, unless you would do as I think I had suggested before
> and
> have the function return bool right away.
It makes sense. I'll apply your recommendations in the next patch
version.

~ Oleksii
diff mbox series

Patch

diff --git a/xen/arch/riscv/include/asm/system.h b/xen/arch/riscv/include/asm/system.h
new file mode 100644
index 0000000000..45bfed4e2b
--- /dev/null
+++ b/xen/arch/riscv/include/asm/system.h
@@ -0,0 +1,79 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _ASM_RISCV_BARRIER_H
+#define _ASM_RISCV_BARRIER_H
+
+#include <asm/csr.h>
+
+#ifndef __ASSEMBLY__
+
+#define RISCV_FENCE(p, s) \
+    __asm__ __volatile__ ("fence " #p "," #s : : : "memory")
+
+/* These barriers need to enforce ordering on both devices or memory. */
+#define mb()                    RISCV_FENCE(iorw,iorw)
+#define rmb()                   RISCV_FENCE(ir,ir)
+#define wmb()                   RISCV_FENCE(ow,ow)
+
+/* These barriers do not need to enforce ordering on devices, just memory. */
+#define smp_mb()                RISCV_FENCE(rw,rw)
+#define smp_rmb()               RISCV_FENCE(r,r)
+#define smp_wmb()               RISCV_FENCE(w,w)
+#define smp_mb__before_atomic() smp_mb()
+#define smp_mb__after_atomic()  smp_mb()
+
+/*
+#define __smp_store_release(p, v)       \
+do {                                    \
+	compiletime_assert_atomic_type(*p); \
+	RISCV_FENCE(rw,w);                  \
+	WRITE_ONCE(*p, v);                  \
+} while (0)
+
+#define __smp_load_acquire(p)           \
+({                                      \
+    typeof(*p) ___p1 = READ_ONCE(*p);   \
+    compiletime_assert_atomic_type(*p); \
+    RISCV_FENCE(r,rw);                  \
+    ___p1;                              \
+})
+*/
+
+static inline unsigned long local_save_flags(void)
+{
+    return csr_read(sstatus);
+}
+
+static inline void local_irq_enable(void)
+{
+    csr_set(sstatus, SSTATUS_SIE);
+}
+
+static inline void local_irq_disable(void)
+{
+    csr_clear(sstatus, SSTATUS_SIE);
+}
+
+#define local_irq_save(x)                           \
+({                                                  \
+    x = csr_read_clear(CSR_SSTATUS, SSTATUS_SIE);   \
+    local_irq_disable();                            \
+})
+
+static inline void local_irq_restore(unsigned long flags)
+{
+	csr_set(CSR_SSTATUS, flags & SSTATUS_SIE);
+}
+
+static inline int local_irq_is_enabled(void)
+{
+    unsigned long flags = local_save_flags();
+
+    return flags & SSTATUS_SIE;
+}
+
+#define arch_fetch_and_add(x, v) __sync_fetch_and_add(x, v)
+
+#endif /* __ASSEMBLY__ */
+
+#endif /* _ASM_RISCV_BARRIER_H */