Re: [PATCH 2/7] RISC-V: arch/riscv Makefile and Kconfigs

[Palmer Dabbelt]

On Mon, 22 May 2017 18:27:21 PDT (-0700), olof@xxxxxxxxx wrote:
> Hi,
>
>
> On Mon, May 22, 2017 at 5:41 PM, Palmer Dabbelt <palmer@xxxxxxxxxxx> wrote:
>> ---
>>  arch/riscv/.gitignore                |  35 ++++
>>  arch/riscv/Kconfig                   | 300 +++++++++++++++++++++++++++++++++++
>>  arch/riscv/Makefile                  |  64 ++++++++
>>  arch/riscv/configs/riscv32_spike     |  47 ++++++
>>  arch/riscv/configs/riscv64_freedom-u |  52 ++++++
>>  arch/riscv/configs/riscv64_qemu      |  64 ++++++++
>>  arch/riscv/configs/riscv64_spike     |  45 ++++++
>>  7 files changed, 607 insertions(+)
>>  create mode 100644 arch/riscv/.gitignore
>>  create mode 100644 arch/riscv/Kconfig
>>  create mode 100644 arch/riscv/Makefile
>>  create mode 100644 arch/riscv/configs/riscv32_spike
>>  create mode 100644 arch/riscv/configs/riscv64_freedom-u
>>  create mode 100644 arch/riscv/configs/riscv64_qemu
>>  create mode 100644 arch/riscv/configs/riscv64_spike
>
> Nearly all other platforms have _defconfig in the config names. It
> might get a bit excessive to prepend riscv{32,64} to all of them
> though. Most other platforms have shortened it to, for example,
> spike_defconfig, spike64_defconfig, qemu_defconfig,
> freedom-u_defconfig.
>
> Not going to argue too much about the color of the shed here, but
> using the _defconfig naming is recommended.

Works for me <https://github.com/riscv/riscv-linux/commit/b1165397ba6cb54f23910537c4bf4c3488ef9aad>

I'll squash all the CR comments into a v2.

>
>>
>> diff --git a/arch/riscv/.gitignore b/arch/riscv/.gitignore
>> new file mode 100644
>> index 000000000000..376d06eb5d52
>> --- /dev/null
>> +++ b/arch/riscv/.gitignore
>> @@ -0,0 +1,35 @@
>> +# Now un-ignore all files.
>> +!*
>> +
>> +# But then re-ignore the files listed in the Linux .gitignore
>> +# Normal rules
>> +#
>> +.*
>> +*.o
>> +*.o.*
>> +*.a
>> +*.s
>> +*.ko
>> +*.so
>> +*.so.dbg
>> +*.mod.c
>> +*.i
>> +*.lst
>> +*.symtypes
>> +*.order
>> +modules.builtin
>> +*.elf
>> +*.bin
>> +*.gz
>> +*.bz2
>> +*.lzma
>> +*.xz
>> +*.lzo
>> +*.patch
>> +*.gcno
>
> I don't think you need to do any of this, just inherit the global one
> (by not having one here)?

Sorry, that's a holdover from how we used to manage our out-of-tree port and
can just be deleted.

  https://github.com/riscv/riscv-linux/commit/68032fb592297331a2f2caf246968da7b70373fe

>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>> new file mode 100644
>> index 000000000000..510ead1d3343
>> --- /dev/null
>> +++ b/arch/riscv/Kconfig
>> +config CPU_RV_GENERIC
>> +       bool "Generic RISC-V"
>> +       select CPU_SUPPORTS_32BIT_KERNEL
>> +       select CPU_SUPPORTS_64BIT_KERNEL
>
> Is this even needed at this point? If the only CPU you can pick
> supports this, you might as well not make it an option (CPU_RV_GENERIC
> that is), and just make CPU_SUPPORTS_{32,64}BIT_KERNEL 'def_bool y'
> for now.

I think this is actually broken in the opposite direction: we only support
32-bit kernels on RV32 and 64-bit kernels on RV64, not both at the same time.
I don't really think it makes sense to build a kernel that supports both 32-bit
and 64-bit on RISC-V (as they're different base ISAs), so I think it'd be
better to just have a "base ISA" configuration.

  https://github.com/riscv/riscv-linux/commit/695d428d65bf6fe1382d34393e9d07f40b74e1b2

We'll think on this a bit more and get something saner.

>> +config SBI_CONSOLE
>> +       tristate "SBI console support"
>> +       select TTY
>> +       default y
>
> Usually you end up having a DRIVER_FOO and DRIVER_FOO_CONSOLE option
> to enable registering it as console.
>
> Also, unless there's strong reason to keep it under arch/, it should
> probably go under drivers/tty/.

In this case "SBI" is the "Supervisor Binary Interface".  This is a set of
routines that are provided by the platform for OS use that do things like
writing to the boot console or TLB shootdowns.  The SBI is part of the RISC-V
ISA, so there isn't a config option for turning it off.  SBI_CONSOLE
enables/disables the SBI's console support, so I think this option is sane.

It's in arch/riscv because the SBI is part of the RISC-V ISA -- essentially
there's special SBI instructions that mean "write some register to the console"
(there's some implementation tricks behind this, so it's really just a
specification).  That said, I'm fine moving this to drivers.

>> +config RVC
>> +       bool "Use compressed instructions (RV32C or RV64C)"
>> +       default n
>
> What does "use" here mean? Use during build, or allow userspace to use them?
>
>> +
>> +config RV_ATOMIC
>> +       bool "Use atomic memory instructions (RV32A or RV64A)"
>> +       default y
>
> Same for this.

These mean "tell the compiler that it can emit these instructions when building
Linux".  Userspace applications can still use these instructions either way.
How does "Emit compressed instructions when building Linux" sound?

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

>
>> +config RV_SYSRISCV_ATOMIC
>> +       bool "Include support for atomic operation syscalls"
>> +       default n
>> +       help
>> +         If atomic memory instructions are present, i.e.,
>> +         CONFIG_RV_ATOMIC, this includes support for the syscall that
>> +         provides atomic accesses.  This is only useful to run
>> +         binaries that require atomic access but were compiled with
>> +         -mno-atomic.
>> +
>> +         If CONFIG_RV_ATOMIC is unset, this option is mandatory.
>
> If it's mandatory then Kconfig language should make it so.

I'm not sure what you mean by this.  We have

  config RISCV
  	...
  	select RV_SYSRISCV_ATOMIC if !RV_ATOMIC

Should this constraint just live within "config RV_SYSRISCV_ATOMIC"?  It seems
cleaner to have the constraints next to the config definitions.

Thanks!