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
