Re: [RFC PATCH v4 0/4] Add basic support for custom CSR

[Alistair Francis <alistair23@gmail.com>]

On Fri, Aug 6, 2021 at 3:59 AM Ruinland Chuan-Tzu Tsai
<ruinland@andestech.com> wrote:
>
> From: Ruinland ChuanTzu Tsai <ruinland@andestech.com>
>
> Dear all,
>
> In this patch, the implementation of custom CSR handling logic is introduced.
>
> If --enable-riscv-custom is set during configuration, custom CSR logic will be
> turned on. During CPU model initialization, setup_custom_csr() is invoked to
> register vendor-provided custom CSR opsets into a hash table.
> When accessing a CSR, in riscv_csrrw(), is_custom_csr() will be called to check
> whether the encountering csrno is a custom CSR. If that's a custom one, a
> struct riscv_csr_operations will be returned and such CSR will be served
> accordingly.

Thanks for adding this. I don't think we need to expose it via the
config logic. A Kconfig option would be enough, it can be enabled by
default and users can choose to disable it if they really want.

>
> The performance slowdown could be easily tested with a simple program running
> on linux-user mode.
>
> /* test_csr.c */
> #include <stdio.h>
> #include <unistd.h>
> #include <sys/time.h>
>
> int main (int ac, char *av[]) {
>    struct  timeval start;
>    struct  timeval end;
>    gettimeofday(&start,NULL);
>    unsigned int loop_n = 999999 ;
>    unsigned char i;
>    unsigned char o;
>    do {
>        for(i=0; i<32; i++) {
>        #if defined(FCSR)
>        __asm__("csrw fcsr, %0;"::"r"(i));
>        __asm__("csrr %0, fcsr;":"=r"(o));
>        #elif defined(UITB)
>        __asm__("csrw 0x800, %0;"::"r"(i));
>        __asm__("csrr %0, 0x800;":"=r"(o));
>        #endif
>        }
>        --loop_n;
>    } while (loop_n > 0);
>    gettimeofday(&end,NULL);
>    unsigned long diff = 1000000 * (end.tv_sec-start.tv_sec)+end.tv_usec-start.tv_usec;
>    printf("%f\n", (double)(diff)/1000000);
>    return 0;
> }
>
> $ riscv64-linux-gnu-gcc -static -DUITB ./test_csr.c -o ./u
> $ riscv64-linux-gnu-gcc -static -DFCSR ./test_csr.c -o ./f
>
> For a custom CSR, uitb, being accessed on andes-ax25 :
> $ ./build/qemu-riscv64 -cpu andes-ax25 ./u
> 4.283091
>
> For a stock CSR, fcsr, being accessed on andes-ax25:
> $ ./build/qemu-riscv64 ./f
> 3.875519
>
> For a custom CSR being accessed on stock rv64:
> $ ./build/qemu-riscv64 -cpu rv64 ./u
> Illegal instruction (core dumped)
> # This is expected to fail.
>
> Currently, the statics on my hands shows that :
> When the custom CSR handling mechanism is activated, we will suffer a 17% slow-
> down on stock CSRs and the penalty of accessing to a custom CSR will be another
> 7% more.

Thanks for testing this.

The critical number here is what is the impact for a CPU that doesn't
have the custom CSRs. So for example what impact does this have on the
rv64 accessing a standard CSR? We don't want to affect performance of
other CPUs. CPUs with custom CSRs can take the performance hit as it
adds a useful feature for them.

Alistair

>
> Cordially yours,
> Ruinland ChuanTzu Tsai
>
> Changes from v3 :
> * Adding options in configure and meson files to turn on/off custom CSR logic.
> * Adding unlikely() to check if custom_csr_map is set.
> * Moving any32 and any out of !(CONFIG_USER_ONLY) for enabling linux-user mode.
> * Fix comment style, add missing license boilerplate.
>
>
> Ruinalnd ChuanTzu Tsai (4):
>   Adding basic custom/vendor CSR handling mechanism
>   Adding Andes AX25 and A25 CPU model
>   Enable custom CSR logic for Andes AX25 and A25
>   Add options to config/meson files for custom CSR
>
>  configure                      |   6 ++
>  meson.build                    |   2 +
>  meson_options.txt              |   2 +
>  target/riscv/andes_cpu_bits.h  | 124 +++++++++++++++++++++++++
>  target/riscv/cpu.c             |  51 +++++++++++
>  target/riscv/cpu.h             |  33 ++++++-
>  target/riscv/cpu_bits.h        |   4 +
>  target/riscv/csr.c             |  83 ++++++++++++++---
>  target/riscv/csr_andes.inc.c   | 160 +++++++++++++++++++++++++++++++++
>  target/riscv/custom_cpu_bits.h |  12 +++
>  10 files changed, 462 insertions(+), 15 deletions(-)
>  create mode 100644 target/riscv/andes_cpu_bits.h
>  create mode 100644 target/riscv/csr_andes.inc.c
>  create mode 100644 target/riscv/custom_cpu_bits.h
>
> --
> 2.32.0
>
>