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

[Palmer Dabbelt]

On Tue, 23 May 2017 05:55:15 PDT (-0700), Arnd Bergmann wrote:
> On Tue, May 23, 2017 at 2:41 AM, Palmer Dabbelt <palmer@xxxxxxxxxxx> 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.

Makes sense.  https://github.com/riscv/riscv-linux/commit/77647f9e4dccab68c69a212a63c9efe1db2b7b1c

>> 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.

OK, seems like a good thing.  https://github.com/riscv/riscv-linux/commit/67db001653614c6555424b3812d7edfba12a6d4c

>> +#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?

The RISC-V ISA manual doesn't actually mention caches anywhere, so there's no
restriction on L1 cache line size (we tried to keep microarchitecture out of
the ISA specification).  We provide the actual cache parameters as part of the
device tree, but it looks like this needs to be known staticly in some places
so we can't use that everywhere.

We could always make this a Kconfig parameter.

>> +
>> +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.

These must have been vestigial code, they appear safe to remove.

  https://github.com/riscv/riscv-linux/commit/d1c88783d5ff66464a25173f7a4af139f0ebf5e2

>> 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.

Makes sense.  These were all OK on existing implementations (as there's no
writable PMAs, so all MMIO regions are strictly ordered), but that's not
actually what the RISC-V ISA says.  I patterned this on arm64

  https://github.com/riscv/riscv-linux/commit/e200fa29a69451ef4d575076e4d2af6b7877b1fa

where I think the only odd thing is our definition of mmiowb

  +/* IO barriers.  These only fence on the IO bits because they're only required
  + * to order device access.  We're defining mmiowb because our AMO instructions
  + * (which are used to implement locks) don't specify ordering.  From Chapter 7
  + * of v2.2 of the user ISA:
  + * "The bits order accesses to one of the two address domains, memory or I/O,
  + * depending on which address domain the atomic instruction is accessing. No
  + * ordering constraint is implied to accesses to the other domain, and a FENCE
  + * instruction should be used to order across both domains."
  + */
  +
  +#define __iormb()               __asm__ __volatile__ ("fence i,io" : : : "memory");
  +#define __iowmb()               __asm__ __volatile__ ("fence io,o" : : : "memory");
  +
  +#define mmiowb()                __asm__ __volatile__ ("fence io,io" : : : "memory");

which I think is correct.

>> +#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

This is currently an architecture limitation.  In RISC-V these properties are
known as PMAs (Physical Memory Attributes).  While the supervisor spec mentions
PMAs, it doesn't provide a mechanism to read or write them so they are
essentially unspecified.  PMAs will be properly defined as part of the platform
specification, which isn't written yet.

>> 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.

Sorry, we had some hacks floating around for QEMU from before we actually had
any devices interfaces working (ie, before device tree and proper MMIO
support).  I'll go through and drop these before v2.

>> 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?

Yes.  https://github.com/riscv/riscv-linux/commit/8f35ce93bd1230ac0cb4aa92e5673c61e50dd862

>> +/*
>> + * 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.

OK, here's my attempt

  https://github.com/riscv/riscv-linux/commit/c618553e7aa65c85564a5d0a868ec7e6cf634afd

Since there's some actual meat, I left a commit message (these are more just
notes for me for my v2, I'll be squashing everything)

"
This is patterned more off the arm64 move than the x86 one, since we
still need to have at least addr_limit to emulate FS.

The patch itself changes sscratch from holding SP to holding TP, which
contains a pointer to task_struct.  thread_info must be at a 0 offset
from task_struct, but it looks like that's already enforced with a big
comment.  We now store both the user and kernel SP in task_struct, but
those are really acting more as extra scratch space than pemanent
storage.
"

>> +#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.

OK.  I've left in the "#ifdef CONFIG_MMU" blocks as the #ifdef/#endif doesn't
really add any code, but I can go ahead and drop the #ifdef if you think that's
better.

  https://github.com/riscv/riscv-linux/commit/e98ca23adfb9422bebc87cbfb58f70d4a63cf067

>> 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

https://github.com/riscv/riscv-linux/commit/52c5e300b498742390434891db34f9dbacd082e9

>> +#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?

I think that was just an oversight: that's not the right switch.  Either you or
someone else pointed out some problems with this.  There's going to be an
interposer in the VDSO, and then we'll always enable the system call.

I can change this to two system calls: sysriscv_cmpxchg32 and
sysriscv_cmpxchg64.