Re: [PATCH v4 13/30] xen/riscv: introduce io.h

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Tue, 2024-02-13 at 12:05 +0100, Jan Beulich wrote:
> On 05.02.2024 16:32, Oleksii Kurochko wrote:
> > The header taken form Linux 6.4.0-rc1 and is based on
> > arch/riscv/include/asm/mmio.h.
> > 
> > Addionally, to the header was added definions of ioremap_*().
> > 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > ---
> > Changes in V4:
> >  - delete inner parentheses in macros.
> >  - s/u<N>/uint<N>.
> > ---
> > Changes in V3:
> >  - re-sync with linux kernel
> >  - update the commit message
> > ---
> > Changes in V2:
> >  - Nothing changed. Only rebase.
> > ---
> >  xen/arch/riscv/include/asm/io.h | 142
> > ++++++++++++++++++++++++++++++++
> >  1 file changed, 142 insertions(+)
> >  create mode 100644 xen/arch/riscv/include/asm/io.h
> > 
> > diff --git a/xen/arch/riscv/include/asm/io.h
> > b/xen/arch/riscv/include/asm/io.h
> > new file mode 100644
> > index 0000000000..1e61a40522
> > --- /dev/null
> > +++ b/xen/arch/riscv/include/asm/io.h
> > @@ -0,0 +1,142 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * {read,write}{b,w,l,q} based on arch/arm64/include/asm/io.h
> > + *   which was based on arch/arm/include/io.h
> > + *
> > + * Copyright (C) 1996-2000 Russell King
> > + * Copyright (C) 2012 ARM Ltd.
> > + * Copyright (C) 2014 Regents of the University of California
> > + */
> > +
> > +
> > +#ifndef _ASM_RISCV_IO_H
> > +#define _ASM_RISCV_IO_H
> > +
> > +#include <asm/byteorder.h>
> > +
> > +/*
> > + * The RISC-V ISA doesn't yet specify how to query or modify PMAs,
> > so we can't
> > + * change the properties of memory regions.  This should be fixed
> > by the
> > + * upcoming platform spec.
> > + */
> > +#define ioremap_nocache(addr, size) ioremap(addr, size)
> > +#define ioremap_wc(addr, size) ioremap(addr, size)
> > +#define ioremap_wt(addr, size) ioremap(addr, size)
> > +
> > +/* Generic IO read/write.  These perform native-endian accesses.
> > */
> > +#define __raw_writeb __raw_writeb
> 
> What use are this and the similar other #define-s?
I don't know specific reason for that, this file fully based on Linux's
arch/riscv/include/asm/mmio.h, so we can drop such defines.

> 
> > +static inline void __raw_writeb(uint8_t val, volatile void __iomem
> > *addr)
> > +{
> > +   asm volatile("sb %0, 0(%1)" : : "r" (val), "r" (addr));
> 
> Nit (throughout): Missing blanks. Or wait - is this file intended to
> be Linux style? If so, it's just one blank that's missing.
I started to update the code style, so I am OK to add missing blanks.
Thanks.
> 
> > +/*
> > + * Unordered I/O memory access primitives.  These are even more
> > relaxed than
> > + * the relaxed versions, as they don't even order accesses between
> > successive
> > + * operations to the I/O regions.
> > + */
> > +#define readb_cpu(c)               ({ uint8_t  __r = __raw_readb(c);
> > __r; })
> > +#define readw_cpu(c)               ({ uint16_t __r =
> > le16_to_cpu((__force __le16)__raw_readw(c)); __r; })
> > +#define readl_cpu(c)               ({ uint32_t __r =
> > le32_to_cpu((__force __le32)__raw_readl(c)); __r; })
> 
> Didn't we settle on the little-endian stuff to be dropped from here?
> No matter what CPU endianness, what endianness a particular device
> (and hence its MMIO region(s)) is using is entirely independent.
> Hence
> conversion, where necessary, needs to occur at a layer up.
Yes, just missed to remove that.

> 
> Also, what good do the __r variables do here? If they weren't here,
> we also wouldn't need to discuss their naming.
I don't see to much sense in __r, so it could be dropped.

> 
> > +#define writeb_cpu(v,c)            ((void)__raw_writeb(v,c))
> > +#define
> > writew_cpu(v,c)             ((void)__raw_writew((__force 
> > uint16_t)cpu_to_le16(v),c))
> > +#define
> > writel_cpu(v,c)             ((void)__raw_writel((__force 
> > uint32_t)cpu_to_le32(v),c))
> 
> Nit: Blanks after commas please (also again further down).
Thanks, I'll update that.

> 
> > +#ifdef CONFIG_64BIT
> > +#define readq_cpu(c)               ({ u64 __r = le64_to_cpu((__force
> > __le64)__raw_readq(c)); __r; })
> > +#define
> > writeq_cpu(v,c)             ((void)__raw_writeq((__force 
> > u64)cpu_to_le64(v),c))
> 
> uint64_t (twice)
> 
> > +#endif
> > +
> > +/*
> > + * I/O memory access primitives. Reads are ordered relative to any
> > + * following Normal memory access. Writes are ordered relative to
> > any prior
> > + * Normal memory access.  The memory barriers here are necessary
> > as RISC-V
> > + * doesn't define any ordering between the memory space and the
> > I/O space.
> > + */
> > +#define __io_br()  do {} while (0)
> 
> Nit: This and ...
> 
> > +#define __io_ar(v) __asm__ __volatile__ ("fence i,r" : : :
> > "memory");
> > +#define __io_bw()  __asm__ __volatile__ ("fence w,o" : : :
> > "memory");
> > +#define __io_aw()  do { } while (0)
> 
> ... this want to be spelled exactly the same.
Oh, overlooked that. Thanks.

> 
> Also, why does __io_ar() have a parameter (which it then doesn't
> use)?
In case of Xen and RISC-V, it can be droped. in case of Linux,
__io_ar() is also defined, at least, for Arm where this parameter is
used, so I assume that intention was to have the same API for
__io_ar().

> 
> Finally at least within a single file please be consistent about
> asm()
> vs __asm__() use.
> 
> > +#define readb(c)   ({ uint8_t  __v; __io_br(); __v =
> > readb_cpu(c); __io_ar(__v); __v; })
> > +#define readw(c)   ({ uint16_t __v; __io_br(); __v =
> > readw_cpu(c); __io_ar(__v); __v; })
> > +#define readl(c)   ({ uint32_t __v; __io_br(); __v =
> > readl_cpu(c); __io_ar(__v); __v; })
> 
> Here the local variables are surely needed. Still they would
> preferably
> not have any underscores as prefixes.
Thanks. This header was left untouched as it was mostly just copy of
Linux's mmio.h, but I'll update it according to Xen code style. Thanks.

> 
> > +#define writeb(v,c)        ({ __io_bw(); writeb_cpu(v,c); __io_aw();
> > })
> > +#define writew(v,c)        ({ __io_bw(); writew_cpu(v,c); __io_aw();
> > })
> > +#define writel(v,c)        ({ __io_bw(); writel_cpu(v,c); __io_aw();
> > })
> > +
> > +#ifdef CONFIG_64BIT
> > +#define readq(c)   ({ u64 __v; __io_br(); __v = readq_cpu(c);
> > __io_ar(__v); __v; })
> 
> uint64_t again
Thanks. I'll update that too.

> 
> > +#define writeq(v,c)        ({ __io_bw(); writeq_cpu((v),(c));
> > __io_aw(); })
> 
> Inner parentheses still left?
Overlooked. Thanks, I'll update that in the next patch version.

~ Oleksii