Re: [PATCH 4/7] RISC-V: arch/riscv/include

[Arnd Bergmann <arnd@arndb.de>]

On Tue, May 23, 2017 at 2:41 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> +/**
> + * atomic_read - read atomic variable
> + * @v: pointer of type atomic_t
> + *
> + * Atomically reads the value of @v.
> + */
> +static inline int atomic_read(const atomic_t *v)
> +{
> +       return *((volatile int *)(&(v->counter)));
> +}
> +/**
> + * atomic_set - set atomic variable
> + * @v: pointer of type atomic_t
> + * @i: required value
> + *
> + * Atomically sets the value of @v to @i.
> + */
> +static inline void atomic_set(atomic_t *v, int i)
> +{
> +       v->counter = i;
> +}

These commonly use READ_ONCE() and WRITE_ONCE,
I'd recommend doing the same here to be on the safe side.

> +/**
> + * atomic64_read - read atomic64 variable
> + * @v: pointer of type atomic64_t
> + *
> + * Atomically reads the value of @v.
> + */
> +static inline s64 atomic64_read(const atomic64_t *v)
> +{
> +       return *((volatile long *)(&(v->counter)));
> +}
> +
> +/**
> + * atomic64_set - set atomic64 variable
> + * @v: pointer to type atomic64_t
> + * @i: required value
> + *
> + * Atomically sets the value of @v to @i.
> + */
> +static inline void atomic64_set(atomic64_t *v, s64 i)
> +{
> +       v->counter = i;
> +}

same here

> diff --git a/arch/riscv/include/asm/bug.h b/arch/riscv/include/asm/bug.h
> new file mode 100644
> index 000000000000..10d894ac3137
> --- /dev/null
> +++ b/arch/riscv/include/asm/bug.h
> @@ -0,0 +1,81 @@
> +/*
>
> +#ifndef _ASM_RISCV_BUG_H
> +#define _ASM_RISCV_BUG_H

> +#ifdef CONFIG_GENERIC_BUG
> +#define __BUG_INSN     _AC(0x00100073, UL) /* sbreak */

Please have a look at the modifications I did for !CONFIG_BUG
on x86, arm and arm64. It's generally better to define BUG to a
trap even when CONFIG_BUG is disabled, otherwise you run
into undefined behavior in some code, and gcc will print annoying
warnings about that.

> +#ifndef _ASM_RISCV_CACHE_H
> +#define _ASM_RISCV_CACHE_H
> +
> +#define L1_CACHE_SHIFT         6
> +
> +#define L1_CACHE_BYTES         (1 << L1_CACHE_SHIFT)

Is this the only valid cache line size on riscv, or just the largest
one that is allowed?

> +
> +static inline dma_addr_t phys_to_dma(struct device *dev, phys_addr_t paddr)
> +{
> +       return (dma_addr_t)paddr;
> +}
> +
> +static inline phys_addr_t dma_to_phys(struct device *dev, dma_addr_t dev_addr)
> +{
> +       return (phys_addr_t)dev_addr;
> +}

What do you need these for? If possible, try to remove them.

> +static inline void dma_cache_sync(struct device *dev, void *vaddr, size_t size, enum dma_data_direction dir)
> +{
> +       /*
> +        * RISC-V is cache-coherent, so this is mostly a no-op.
> +        * However, we do need to ensure that dma_cache_sync()
> +        * enforces order, hence the mb().
> +        */
> +       mb();
> +}

Do you even support any drivers that use
dma_alloc_noncoherent()/dma_cache_sync()?

I would guess you can just leave this out.

> diff --git a/arch/riscv/include/asm/io.h b/arch/riscv/include/asm/io.h
> new file mode 100644
> index 000000000000..d942555a7a08
> --- /dev/null
> +++ b/arch/riscv/include/asm/io.h
> @@ -0,0 +1,36 @@

> +#ifndef _ASM_RISCV_IO_H
> +#define _ASM_RISCV_IO_H
> +
> +#include <asm-generic/io.h>

I would recommend providing your own {read,write}{b,w,l,q}{,_relaxed}
helpers using inline assembly, to prevent the compiler for breaking
up accesses into byte accesses.

Also, most architectures require to some synchronization after a
non-relaxed readl() to prevent prefetching of DMA buffers, and
before a writel() to flush write buffers when a DMA gets triggered.

> +#ifdef __KERNEL__
> +
> +#ifdef CONFIG_MMU
> +
> +extern void __iomem *ioremap(phys_addr_t offset, unsigned long size);
> +
> +#define ioremap_nocache(addr, size) ioremap((addr), (size))
> +#define ioremap_wc(addr, size) ioremap((addr), (size))
> +#define ioremap_wt(addr, size) ioremap((addr), (size))

Is this a hard architecture limitation? Normally you really want
write-combined access on frame buffer memory and a few other
cases for performance reasons, and ioremap_wc() gets used
for by memremap() for addressing RAM in some cases, and you
normally don't want to have PTEs for the same memory using
cached and uncached page flags

> diff --git a/arch/riscv/include/asm/serial.h b/arch/riscv/include/asm/serial.h
> new file mode 100644
> index 000000000000..d783dbe80a4b
> --- /dev/null
> +++ b/arch/riscv/include/asm/serial.h
> @@ -0,0 +1,43 @@
> +/*
> + * Copyright (C) 2014 Regents of the University of California
> + *
> + *   This program is free software; you can redistribute it and/or
> + *   modify it under the terms of the GNU General Public License
> + *   as published by the Free Software Foundation, version 2.
> + *
> + *   This program is distributed in the hope that it will be useful, but
> + *   WITHOUT ANY WARRANTY; without even the implied warranty of
> + *   MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
> + *   NON INFRINGEMENT.  See the GNU General Public License for
> + *   more details.
> + */
> +
> +#ifndef _ASM_RISCV_SERIAL_H
> +#define _ASM_RISCV_SERIAL_H
> +
> +/*
> + * FIXME: interim serial support for riscv-qemu
> + *
> + * Currently requires that the emulator itself create a hole at addresses
> + * 0x3f8 - 0x3ff without looking through page tables.

This sounds like something we want to fix in qemu and not have in the
mainline kernel. In particular, something seems really wrong if your
inb()/outb() get remapped to physical CPU address 0+offset.

> diff --git a/arch/riscv/include/asm/setup.h b/arch/riscv/include/asm/setup.h
> new file mode 100644
> index 000000000000..e457854e9988
> --- /dev/null
> +++ b/arch/riscv/include/asm/setup.h
> @@ -0,0 +1,20 @@
> +/*
> + * Copyright (C) 2012 Regents of the University of California
> + *
> + *   This program is free software; you can redistribute it and/or
> + *   modify it under the terms of the GNU General Public License
> + *   as published by the Free Software Foundation, version 2.
> + *
> + *   This program is distributed in the hope that it will be useful, but
> + *   WITHOUT ANY WARRANTY; without even the implied warranty of
> + *   MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
> + *   NON INFRINGEMENT.  See the GNU General Public License for
> + *   more details.
> + */
> +
> +#ifndef _ASM_RISCV_SETUP_H
> +#define _ASM_RISCV_SETUP_H
> +
> +#include <asm-generic/setup.h>
> +
> +#endif /* _ASM_RISCV_SETUP_H */

Can you remove this file and add it to asm/Kbuild as generic-y instead?

> +/*
> + * low level task data that entry.S needs immediate access to
> + * - this struct should fit entirely inside of one cache line
> + * - this struct resides at the bottom of the supervisor stack
> + * - if the members of this struct changes, the assembly constants
> + *   in asm-offsets.c must be updated accordingly
> + */
> +struct thread_info {
> +       struct task_struct      *task;          /* main task structure */
> +       unsigned long           flags;          /* low level flags */
> +       __u32                   cpu;            /* current CPU */
> +       int                     preempt_count;  /* 0 => preemptable, <0 => BUG */
> +       mm_segment_t            addr_limit;
> +};

Please see 15f4eae70d36 ("x86: Move thread_info into task_struct")
and try to do the same.

> +#else /* !CONFIG_MMU */
> +
> +static inline void flush_tlb_all(void)
> +{
> +       BUG();
> +}
> +
> +static inline void flush_tlb_mm(struct mm_struct *mm)
> +{
> +       BUG();
> +}

The NOMMU support is rather incomplete and CONFIG_MMU is
hard-enabled, so I'd just drop any !CONFIG_MMU #ifdefs.

> diff --git a/arch/riscv/include/uapi/asm/Kbuild b/arch/riscv/include/uapi/asm/Kbuild
> new file mode 100644
> index 000000000000..276b6dae745c
> --- /dev/null
> +++ b/arch/riscv/include/uapi/asm/Kbuild
> @@ -0,0 +1,10 @@
> +# UAPI Header export list
> +include include/uapi/asm-generic/Kbuild.asm
> +
> +header-y += auxvec.h
> +header-y += bitsperlong.h
> +header-y += byteorder.h
> +header-y += ptrace.h
> +header-y += sigcontext.h
> +header-y += siginfo.h
> +header-y += unistd.h

Please see
fcc8487d477a ("uapi: export all headers under uapi directories")

and adapt the file accordingly

> +#include <asm-generic/unistd.h>
> +
> +#define __NR_sysriscv  __NR_arch_specific_syscall
> +#ifndef __riscv_atomic
> +__SYSCALL(__NR_sysriscv, sys_sysriscv)
> +#endif

Please make this a straight cmpxchg syscall and remove the multiplexer.
Why does the definition depend on __riscv_atomic rather than the
Kconfig symbol?

       Arnd