Re: [PATCH v3 09/34] xen/riscv: introduce system.h

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Thu, 2024-01-11 at 17:00 +0100, Jan Beulich wrote:
> On 22.12.2023 16:12, Oleksii Kurochko wrote:
> > --- /dev/null
> > +++ b/xen/arch/riscv/include/asm/system.h
> > @@ -0,0 +1,90 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +
> > +#ifndef _ASM_RISCV_BARRIER_H
> > +#define _ASM_RISCV_BARRIER_H
> 
> s/BARRIER/SYSTEM/ ?
Yes, it should be SYSTEM. Thanks for noticing that.

> 
> With that taken care of (which I'd be happy to do while committing)
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
Thanks a lot. I'll be happy with that.

> 
> > +#include <xen/stdbool.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 bool local_irq_is_enabled(void)
> > +{
> > +    unsigned long flags = local_save_flags();
> > +
> > +    return (flags & SSTATUS_SIE) != 0;
> 
> Just as a remark - when the resulting type is bool, we generally
> prefer to omit the "!= 0".
Thanks. I'll take into account that.

~ Oleksii