Re: [RFC 0/2] RISC-V: A proposal to add vendor-specific code

From: Nick Kossifidis
Date: Mon Nov 05 2018 - 14:39:40 EST


Hello Vincent,

ÎÏÎÏ 2018-10-31 12:35, Vincent Chen ÎÎÏÎÏÎ:
RISC-V permits each vendor to develop respective extension ISA based
on RISC-V standard ISA. This means that these vendor-specific features
may be compatible to their compiler and CPU. Therefore, each vendor may
be considered a sub-architecture of RISC-V. Currently, vendors do not
have the appropriate examples to add these specific features to the
kernel. In this RFC set, we propose an infrastructure that vendor can
easily hook their specific features into kernel. The first commit is
the main body of this infrastructure. In the second commit, we provide
a solution that allows dma_map_ops() to work without cache coherent
agent support. Cache coherent agent is unsupported for low-end CPUs in
the AndeStar RISC-V series. In order for Linux to run on these CPUs, we
need this solution to overcome the limitation of cache coherent agent
support. Hence, it also can be used as an example for the first commit.

I am glad to discuss any ideas, so if you have any idea, please give
me some feedback.

So if I got this right, in your case you've added some CSRs (ucctlbeginaddr
and ucctlcommand) for marking regions as non-cacheable, and you provide your
own get_arch_dma_ops by overriding the default header files with your
vendor-specific ones.

Some things that are IMHO wrong with the proposed approach:

a) By directly modifying your custom CSRs, it means that we will need
compiler support in order to compile a kernel with your code in it. This
will break CI systems and will introduce various issues on testing and
reviewing your code. In general if we need custom toolchains to compile
the kernel, that may be unavailable (vendors will not always open source
their compiler support), we won't be able to maintain a decent level of
code quality in the tree. How can the maintainer push your code on the
repository if he/she can't even perform a basic compilation test ?

In contrast with ARM and others that have a standard set of possible
extensions (e.g. NEON, crc32 etc), and provide compiler support for those,
a similar approach is not valid for RISC-V. We could demand that vendors
add compiler support e.g. on gcc before submitting a patch with custom
assembly but I don't think this approach is feasible (one vendor's CSRs
or custom instructions may overlap with another's). I believe we should
just use SBI calls instead and let the firmware (and/or custom userspace
libraries) handle the custom CSRs/assembly instructions.

This is a concern also for lib/ and crypto/ where vendors might want to use
their own extensions for providing optimized assembly for their cores. It's
not a big deal to use SBI and handle vendor-specific stuff on firmware and/or
userspace, it's actually more flexible for the vendors since they can have
their own process for maintaining their firmware and releasing it in their
own terms/license etc. If we see that the SBI has too much overhead or is not
enough we can design it in a better way or extend it. It will still be
standard and easier to maintain than a fragmented ecosystem of mostly
unusable code, inside the mainline kernel.

In case we need to save/store registers related to a custom extension, I
guess we can also have an SBI call for saving/restoring custom registers
to/from S-managed memory and we should be ok. It should also be possible
to do any extra state handling in firmware if needed. I believe we can and
should avoid custom assembly on the kernel at all costs !


b) By using CONFIG_VENDOR_FOLDER_NAME you add a compile time dependency that
allows this kernel image to be built for a specific vendor. This is problematic
in different ways. At first it's not possible to have a kernel image that's
generic and can be used for all RISC-V systems. That's what distros want
and that's how they 've been working so far.

Also in case a vendor has many different boards with different implementation
details how will this approach help ? It would make more sense to have something
like arch/riscv/<platform>. Also have in mind that some extensions might be
available as IP to vendors so multiple vendors might use the same extension
made/licensed from another vendor. I think arch/riscv/<extension> is more
appropriate. Since we can use mvendorid/marchid/mimpid to identify that
at runtime maybe we can have some wrapper code that initializes a struct of
function pointers early on setup_arch(), allowing vendors to populate it
according to the available extensions in their hw, based on the above ids.

We can also just use device-tree for that and mark the used extensions there.
We can even pass extension-specific parameters this way (e.g. to save CSRs)
in case of extensions that can be parametrized on hw design phase (I'm
thinking of IPs sold from companies with licenses for unlocking the X feature
or for supporting up to Y channels/slots/instances/whatever).


In any case it's an interesting subject that we definitely need to think about,
thanks for bringing this up !

Regards,
Nick