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

[Olof Johansson]

On Mon, May 22, 2017 at 9:49 PM, Palmer Dabbelt <palmer@xxxxxxxxxxx> wrote:
> 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.

Sure, sounds good overall though.

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

The same is true for some other drivers. Actually, I wonder if it
might be just as easy to implement a sbi backend for hvc -- see
hvc_udbg.c for an example where, on power, you have a simple get/put
char hypervisor call in a very similar manner.

Either way (keeping discrete sbi driver or implementing hvc backend),
moving to drivers/tty is the right thing here -- we've worked hard on
ARM to get rid of random drivers under arch/ and it'd be nice to not
see new ones intoduced here.


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

Sounds good, you can still reference the ISA extensions if you want though.

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

Ah, I just missed the select.


-Olof