Re: [PATCH v5 12/23] xen/riscv: introduce io.h

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.02.2024 18:38, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/io.h
> @@ -0,0 +1,157 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + *  The header taken form Linux 6.4.0-rc1 and is based on
> + *  arch/riscv/include/asm/mmio.h with the following changes:
> + *   - drop forcing of endianess for read*(), write*() functions as
> + *     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.
> + *     Another one reason to drop endianess conversion is:
> + *     
> https://patchwork.kernel.org/project/linux-riscv/patch/20190411115623.5749-3-hch@xxxxxx/
> + *     One of the answers of the author of the commit:
> + *       And we don't know if Linux will be around if that ever changes.
> + *       The point is:
> + *        a) the current RISC-V spec is LE only
> + *        b) the current linux port is LE only except for this little bit
> + *       There is no point in leaving just this bitrotting code around.  It
> + *       just confuses developers, (very very slightly) slows down compiles
> +  *      and will bitrot.  It also won't be any significant help to a future

Nit: Stray extra leading blank.

> + *       developer down the road doing a hypothetical BE RISC-V Linux port.
> + *   - drop unused argument of __io_ar() macros.
> + *   - drop "#define _raw_{read,write}{b,w,l,d,q} 
> _raw_{read,write}{b,w,l,d,q}"
> + *     as they are unnessary.

Nit: unnecessary (also ...

> + *   - Adopt the Xen code style for this header, considering that 
> significant changes
> + *     are not anticipated in the future.
> + *     In the event of any issues, adapting them to Xen style should be 
> easily
> + *     manageable.
> + *   - drop unnessary __r variables in macros read*_cpu()

... again here)

> + * Copyright (C) 1996-2000 Russell King
> + * Copyright (C) 2012 ARM Ltd.
> + * Copyright (C) 2014 Regents of the University of California
> + * Copyright (C) 2024 Vates
> + */
> +
> +#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. */
> +static inline void __raw_writeb(uint8_t val, volatile void __iomem *addr)
> +{
> +    asm volatile ( "sb %0, 0(%1)" : : "r" (val), "r" (addr) );
> +}

I realize this is like Linux has it, but how is the compiler to know that
*addr is being access here? If the omission of respective constraints here
and below is intentional, I think a comment (covering all instances) is
needed. Note that while supposedly cloned from Arm code, Arm variants do
have such constraints in Linux.

I'm sorry for not having paid (enough) attention earlier.

> +static inline void __raw_writew(uint16_t val, volatile void __iomem *addr)
> +{
> +    asm volatile ( "sh %0, 0(%1)" : : "r" (val), "r" (addr) );
> +}
> +
> +static inline void __raw_writel(uint32_t val, volatile void __iomem *addr)
> +{
> +    asm volatile ( "sw %0, 0(%1)" : : "r" (val), "r" (addr) );
> +}
> +
> +#ifdef CONFIG_64BIT
> +static inline void __raw_writeq(u64 val, volatile void __iomem *addr)

uint64_t please

> +{
> +    asm volatile ( "sd %0, 0(%1)" : : "r" (val), "r" (addr) );
> +}
> +#endif
> +
> +static inline uint8_t __raw_readb(const volatile void __iomem *addr)
> +{
> +    uint8_t val;
> +
> +    asm volatile ( "lb %0, 0(%1)" : "=r" (val) : "r" (addr) );
> +    return val;
> +}
> +
> +static inline uint16_t __raw_readw(const volatile void __iomem *addr)
> +{
> +    uint16_t val;
> +
> +    asm volatile ( "lh %0, 0(%1)" : "=r" (val) : "r" (addr) );
> +    return val;
> +}
> +
> +static inline uint32_t __raw_readl(const volatile void __iomem *addr)
> +{
> +    uint32_t val;
> +
> +    asm volatile ( "lw %0, 0(%1)" : "=r" (val) : "r" (addr) );
> +    return val;
> +}
> +
> +#ifdef CONFIG_64BIT
> +static inline u64 __raw_readq(const volatile void __iomem *addr)

uint64_t please

> +{
> +    u64 val;

and again

> +    asm volatile ( "ld %0, 0(%1)" : "=r" (val) : "r" (addr) );
> +    return val;
> +}
> +#endif
> +
> +/*
> + * 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)        __raw_readb(c)
> +#define readw_cpu(c)        __raw_readw(c)
> +#define readl_cpu(c)        __raw_readl(c)
> +
> +#define writeb_cpu(v, c)    __raw_writeb(v, c)
> +#define writew_cpu(v, c)    __raw_writew(v, c)
> +#define writel_cpu(v, c)    __raw_writel(v, c)
> +
> +#ifdef CONFIG_64BIT
> +#define readq_cpu(c)        __raw_readq(c)
> +#define writeq_cpu(v, c)    __raw_writeq(v, c)
> +#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)
> +#define __io_ar()   asm volatile ( "fence i,r" : : : "memory" );
> +#define __io_bw()   asm volatile ( "fence w,o" : : : "memory" );
> +#define __io_aw()   do { } while (0)
> +
> +#define readb(c)    ({ uint8_t  v; __io_br(); v = readb_cpu(c); __io_ar(); 
> v; })
> +#define readw(c)    ({ uint16_t v; __io_br(); v = readw_cpu(c); __io_ar(); 
> v; })
> +#define readl(c)    ({ uint32_t v; __io_br(); v = readl_cpu(c); __io_ar(); 
> v; })
> +
> +#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)        ({ uint64_t v; __io_br(); v = readq_cpu(c); 
> __io_ar(); v; })
> +#define writeq(v, c)    ({ __io_bw(); writeq_cpu(v, c); __io_aw(); })
> +#endif

Overall looks much tidier now, thanks.

Jan