From: Ruinland ChuanTzu Tsai <ruinland@andestech.com>

During my modification on my previous patch series for custom CSR support, I
believe this issue deserves its own discussion (or debate) because it's _not_
as simple as "just put those options in Kconfig".

The obstables I've encountered and the kluges I came up is listed as follow :

(1) Due to the design of top-level meson.build, all Kconfig options will land
into `*-config-devices.h` since minikconf will be only used after config_target
being processed. This will let to the fact that linux-users won't be able to
use custom CSR code properly becuase they only includes `*-config-devices.h`.
And that is reasonble due to the fact that changes on cpu.c and csr.c is a 
target-related matter and linux-user mode shouldn't include device related 
headers in most of cases.

So, modify meson.build to parse target/riscv/Kconfig during config_target phase
is without doubts necessary.

(2) Kconfig option `RISCV_CUSTOM_CSR` is introduced for RISC-V cpu models to 
toggle it at its will. Yet due to the fact that csr.o and cpu.o are linked
altogether for all CPU models, the suffer will be shared without option.
The only reasonable way to seperate build the fire lane which seperates vendor
flavored cpu and spec-conformed ones, is to build them seperately with options
toggled diffrently, just like RV32 and RV64 shares almost the same source base,
yet the sources are compiled with differnt flags/definitions.

To achieve that, miraculously, we can just put *.mak files into `target`
directoy, because that's how `configure` enumerates what targets are supported.

(3) The longest days are not over yet, if we take a good look at how the minikconf
is invoked during config_devices and in what way *.mak presented its options
inside `default-configs/devices`, we can see that *.mak files there is formated
in `CONFIG_*` style and the minikconf is reading directly during config_device
phase. That's totally different from *.mak files presented in
`default-configs/targets`. To make the parsing logic consistent, I
introduce a rv_custom directory inside which contains minikconf-parsable
mak files.

With this patches, ones can build a A25/AX25 linux-user platform by :
$ ./configure --target-list=riscv64-andes-linux-user,riscv32-andes-linux-user
$ make

P.S. The pacthes from :
https://lists.gnu.org/archive/html/qemu-devel/2021-08/msg00913.html
is needed. A clean-up and modified version will be sent out soon.

P.P.S.
I know these parts won't be easy to digest, and the further iterations will be
needed, so I didn't ask my colleagues to sign-off for now.

Cordially yours,
Ruinland ChuanTzu Tsai

Ruinland ChuanTzu Tsai (2):
  Adding Kconfig options for custom CSR support and Andes CPU model
  Adding necessary files for Andes platforms, cores to enable custom CSR
    support

 Kconfig                                       |  1 +
 .../devices/riscv32-andes-softmmu.mak         | 17 ++++++++++++
 .../devices/riscv64-andes-softmmu.mak         | 17 ++++++++++++
 .../targets/riscv32-andes-linux-user.mak      |  1 +
 .../targets/riscv32-andes-softmmu.mak         |  1 +
 .../targets/riscv64-andes-linux-user.mak      |  1 +
 .../targets/riscv64-andes-softmmu.mak         |  1 +
 .../targets/rv_custom/no_custom.mak           |  0
 .../rv_custom/riscv32-andes-linux-user.mak    |  1 +
 .../rv_custom/riscv32-andes-softmmu.mak       |  1 +
 .../targets/rv_custom/riscv32-linux-user.mak  |  1 +
 .../targets/rv_custom/riscv32-softmmu.mak     |  1 +
 .../rv_custom/riscv64-andes-linux-user.mak    |  1 +
 .../rv_custom/riscv64-andes-softmmu.mak       |  1 +
 .../targets/rv_custom/riscv64-linux-user.mak  |  1 +
 .../targets/rv_custom/riscv64-softmmu.mak     |  1 +
 meson.build                                   | 26 +++++++++++++++++++
 target/riscv/Kconfig                          |  6 +++++
 18 files changed, 79 insertions(+)
 create mode 100644 default-configs/devices/riscv32-andes-softmmu.mak
 create mode 100644 default-configs/devices/riscv64-andes-softmmu.mak
 create mode 120000 default-configs/targets/riscv32-andes-linux-user.mak
 create mode 120000 default-configs/targets/riscv32-andes-softmmu.mak
 create mode 120000 default-configs/targets/riscv64-andes-linux-user.mak
 create mode 120000 default-configs/targets/riscv64-andes-softmmu.mak
 create mode 100644 default-configs/targets/rv_custom/no_custom.mak
 create mode 100644 default-configs/targets/rv_custom/riscv32-andes-linux-user.mak
 create mode 100644 default-configs/targets/rv_custom/riscv32-andes-softmmu.mak
 create mode 120000 default-configs/targets/rv_custom/riscv32-linux-user.mak
 create mode 120000 default-configs/targets/rv_custom/riscv32-softmmu.mak
 create mode 100644 default-configs/targets/rv_custom/riscv64-andes-linux-user.mak
 create mode 100644 default-configs/targets/rv_custom/riscv64-andes-softmmu.mak
 create mode 120000 default-configs/targets/rv_custom/riscv64-linux-user.mak
 create mode 120000 default-configs/targets/rv_custom/riscv64-softmmu.mak
 create mode 100644 target/riscv/Kconfig

-- 
2.32.0

On Fri, Aug 27, 2021 at 1:16 AM Ruinland Chuan-Tzu Tsai
<ruinland@andestech.com> wrote:
>
> From: Ruinland ChuanTzu Tsai <ruinland@andestech.com>
>
> During my modification on my previous patch series for custom CSR support, I
> believe this issue deserves its own discussion (or debate) because it's _not_
> as simple as "just put those options in Kconfig".
>
> The obstables I've encountered and the kluges I came up is listed as follow :
>
> (1) Due to the design of top-level meson.build, all Kconfig options will land
> into `*-config-devices.h` since minikconf will be only used after config_target
> being processed. This will let to the fact that linux-users won't be able to
> use custom CSR code properly becuase they only includes `*-config-devices.h`.
> And that is reasonble due to the fact that changes on cpu.c and csr.c is a
> target-related matter and linux-user mode shouldn't include device related
> headers in most of cases.
>
> So, modify meson.build to parse target/riscv/Kconfig during config_target phase
> is without doubts necessary.
>
> (2) Kconfig option `RISCV_CUSTOM_CSR` is introduced for RISC-V cpu models to
> toggle it at its will. Yet due to the fact that csr.o and cpu.o are linked
> altogether for all CPU models, the suffer will be shared without option.
> The only reasonable way to seperate build the fire lane which seperates vendor
> flavored cpu and spec-conformed ones, is to build them seperately with options
> toggled diffrently, just like RV32 and RV64 shares almost the same source base,
> yet the sources are compiled with differnt flags/definitions.
>
> To achieve that, miraculously, we can just put *.mak files into `target`
> directoy, because that's how `configure` enumerates what targets are supported.
>
> (3) The longest days are not over yet, if we take a good look at how the minikconf
> is invoked during config_devices and in what way *.mak presented its options
> inside `default-configs/devices`, we can see that *.mak files there is formated
> in `CONFIG_*` style and the minikconf is reading directly during config_device
> phase. That's totally different from *.mak files presented in
> `default-configs/targets`. To make the parsing logic consistent, I
> introduce a rv_custom directory inside which contains minikconf-parsable
> mak files.
>
> With this patches, ones can build a A25/AX25 linux-user platform by :
> $ ./configure --target-list=riscv64-andes-linux-user,riscv32-andes-linux-user

Hey! Thanks for the patches

I'm not convinced that we want this though.

Right now we are trying to head towards a riscv64-softmmu binary being
able to run all RISC-V code. That include 32-bit cpus
(qemu-riscv64-softmmu -cpu r32...) and 64-bit CPUs. We shouldn't be
splitting out more targets.

It also goes against the general idea of RISC-V in that everyone has a
standard compliant implementation, they can then add extra
functionality.

In terms of Kconfig options. It doesn't seem like a bad idea to have
an option to fully disable custom CSRs. That way if someone really
wants performance and doesn't want custom CSRs they can disable the
switch. Otherwise we leave it on and all custom CSRs are available in
the build and then controlled by the CPU selection at runtime. If that
ends up being too difficult to implement though then we don't have to
have it.

Thanks again for working on this.

Alistair


> $ make
>
> P.S. The pacthes from :
> https://lists.gnu.org/archive/html/qemu-devel/2021-08/msg00913.html
> is needed. A clean-up and modified version will be sent out soon.
>
> P.P.S.
> I know these parts won't be easy to digest, and the further iterations will be
> needed, so I didn't ask my colleagues to sign-off for now.
>
> Cordially yours,
> Ruinland ChuanTzu Tsai
>
> Ruinland ChuanTzu Tsai (2):
>   Adding Kconfig options for custom CSR support and Andes CPU model
>   Adding necessary files for Andes platforms, cores to enable custom CSR
>     support
>
>  Kconfig                                       |  1 +
>  .../devices/riscv32-andes-softmmu.mak         | 17 ++++++++++++
>  .../devices/riscv64-andes-softmmu.mak         | 17 ++++++++++++
>  .../targets/riscv32-andes-linux-user.mak      |  1 +
>  .../targets/riscv32-andes-softmmu.mak         |  1 +
>  .../targets/riscv64-andes-linux-user.mak      |  1 +
>  .../targets/riscv64-andes-softmmu.mak         |  1 +
>  .../targets/rv_custom/no_custom.mak           |  0
>  .../rv_custom/riscv32-andes-linux-user.mak    |  1 +
>  .../rv_custom/riscv32-andes-softmmu.mak       |  1 +
>  .../targets/rv_custom/riscv32-linux-user.mak  |  1 +
>  .../targets/rv_custom/riscv32-softmmu.mak     |  1 +
>  .../rv_custom/riscv64-andes-linux-user.mak    |  1 +
>  .../rv_custom/riscv64-andes-softmmu.mak       |  1 +
>  .../targets/rv_custom/riscv64-linux-user.mak  |  1 +
>  .../targets/rv_custom/riscv64-softmmu.mak     |  1 +
>  meson.build                                   | 26 +++++++++++++++++++
>  target/riscv/Kconfig                          |  6 +++++
>  18 files changed, 79 insertions(+)
>  create mode 100644 default-configs/devices/riscv32-andes-softmmu.mak
>  create mode 100644 default-configs/devices/riscv64-andes-softmmu.mak
>  create mode 120000 default-configs/targets/riscv32-andes-linux-user.mak
>  create mode 120000 default-configs/targets/riscv32-andes-softmmu.mak
>  create mode 120000 default-configs/targets/riscv64-andes-linux-user.mak
>  create mode 120000 default-configs/targets/riscv64-andes-softmmu.mak
>  create mode 100644 default-configs/targets/rv_custom/no_custom.mak
>  create mode 100644 default-configs/targets/rv_custom/riscv32-andes-linux-user.mak
>  create mode 100644 default-configs/targets/rv_custom/riscv32-andes-softmmu.mak
>  create mode 120000 default-configs/targets/rv_custom/riscv32-linux-user.mak
>  create mode 120000 default-configs/targets/rv_custom/riscv32-softmmu.mak
>  create mode 100644 default-configs/targets/rv_custom/riscv64-andes-linux-user.mak
>  create mode 100644 default-configs/targets/rv_custom/riscv64-andes-softmmu.mak
>  create mode 120000 default-configs/targets/rv_custom/riscv64-linux-user.mak
>  create mode 120000 default-configs/targets/rv_custom/riscv64-softmmu.mak
>  create mode 100644 target/riscv/Kconfig
>
> --
> 2.32.0
>

Hi Alistair,

Thanks for the heads up about the upcoming unification of RISC-V 32/64 targets.
Yet I have several concerns and would like to have some brainstorming regarding
such topics - -

That is, could you elaborate more about the "runtime check/switch" which you
mentioned in the previous e-mail :
https://lists.nongnu.org/archive/html/qemu-devel/2021-08/msg02154.html
I'm not quite following the context.
If we don't have a way to toggle which (vendor) cores, which will be used,
during compile time, it means that we have to build all the vendor code and
link them all together; and we might have the chance to encounter collision on
csrno between different vendors.

Secondly, I'm not quite sure about how we're going to merge decode tree files
across RV32 and RV64. Vendor-designed custom instruction would have a different
encoding scheme on bitfields for RV32 and RV64. Currently, we (Andes) are using
different decodetree sources for gen32 and gen64 in `target/riscv/meson.build`.
I'm preparing the patch to demonstrate such hiccups.

As far as I know, there's no control flow logic for decodetree to parse
decodetree files differently. (e.g. ifdef XLEN == 32 then blah, blah).

To meet in the halfway, maybe after the grand unification on RV32/64, we can
still confine vendor custom logic (i.e. instrucions and CSRs) to be toggled by
whether a certain vendor cpu model is selected ?

By the way, I'm wondering how our friends from T-Head (Guo Ren ?) regard this
issue ? AFAIK, they forked QEMU from v3.2.0 and applied their vendor features
on top of it for quite a while.

Cordially yours,
Ruinland ChuanTzu Tsai

On Thu, Sep 02, 2021 at 12:25:20PM +1000, Alistair Francis wrote:
> On Fri, Aug 27, 2021 at 1:16 AM Ruinland Chuan-Tzu Tsai
> <ruinland@andestech.com> wrote:
> >
> > From: Ruinland ChuanTzu Tsai <ruinland@andestech.com>
> >
> > During my modification on my previous patch series for custom CSR support, I
> > believe this issue deserves its own discussion (or debate) because it's _not_
> > as simple as "just put those options in Kconfig".
> >
> > The obstables I've encountered and the kluges I came up is listed as follow :
> >
> > (1) Due to the design of top-level meson.build, all Kconfig options will land
> > into `*-config-devices.h` since minikconf will be only used after config_target
> > being processed. This will let to the fact that linux-users won't be able to
> > use custom CSR code properly becuase they only includes `*-config-devices.h`.
> > And that is reasonble due to the fact that changes on cpu.c and csr.c is a
> > target-related matter and linux-user mode shouldn't include device related
> > headers in most of cases.
> >
> > So, modify meson.build to parse target/riscv/Kconfig during config_target phase
> > is without doubts necessary.
> >
> > (2) Kconfig option `RISCV_CUSTOM_CSR` is introduced for RISC-V cpu models to
> > toggle it at its will. Yet due to the fact that csr.o and cpu.o are linked
> > altogether for all CPU models, the suffer will be shared without option.
> > The only reasonable way to seperate build the fire lane which seperates vendor
> > flavored cpu and spec-conformed ones, is to build them seperately with options
> > toggled diffrently, just like RV32 and RV64 shares almost the same source base,
> > yet the sources are compiled with differnt flags/definitions.
> >
> > To achieve that, miraculously, we can just put *.mak files into `target`
> > directoy, because that's how `configure` enumerates what targets are supported.
> >
> > (3) The longest days are not over yet, if we take a good look at how the minikconf
> > is invoked during config_devices and in what way *.mak presented its options
> > inside `default-configs/devices`, we can see that *.mak files there is formated
> > in `CONFIG_*` style and the minikconf is reading directly during config_device
> > phase. That's totally different from *.mak files presented in
> > `default-configs/targets`. To make the parsing logic consistent, I
> > introduce a rv_custom directory inside which contains minikconf-parsable
> > mak files.
> >
> > With this patches, ones can build a A25/AX25 linux-user platform by :
> > $ ./configure --target-list=riscv64-andes-linux-user,riscv32-andes-linux-user
> 
> Hey! Thanks for the patches
> 
> I'm not convinced that we want this though.
> 
> Right now we are trying to head towards a riscv64-softmmu binary being
> able to run all RISC-V code. That include 32-bit cpus
> (qemu-riscv64-softmmu -cpu r32...) and 64-bit CPUs. We shouldn't be
> splitting out more targets.
> 
> It also goes against the general idea of RISC-V in that everyone has a
> standard compliant implementation, they can then add extra
> functionality.
> 
> In terms of Kconfig options. It doesn't seem like a bad idea to have
> an option to fully disable custom CSRs. That way if someone really
> wants performance and doesn't want custom CSRs they can disable the
> switch. Otherwise we leave it on and all custom CSRs are available in
> the build and then controlled by the CPU selection at runtime. If that
> ends up being too difficult to implement though then we don't have to
> have it.
> 
> Thanks again for working on this.
> 
> Alistair
> 
> 
> > $ make
> >
> > P.S. The pacthes from :
> > https://lists.gnu.org/archive/html/qemu-devel/2021-08/msg00913.html
> > is needed. A clean-up and modified version will be sent out soon.
> >
> > P.P.S.
> > I know these parts won't be easy to digest, and the further iterations will be
> > needed, so I didn't ask my colleagues to sign-off for now.
> >
> > Cordially yours,
> > Ruinland ChuanTzu Tsai
> >
> > Ruinland ChuanTzu Tsai (2):
> >   Adding Kconfig options for custom CSR support and Andes CPU model
> >   Adding necessary files for Andes platforms, cores to enable custom CSR
> >     support
> >
> >  Kconfig                                       |  1 +
> >  .../devices/riscv32-andes-softmmu.mak         | 17 ++++++++++++
> >  .../devices/riscv64-andes-softmmu.mak         | 17 ++++++++++++
> >  .../targets/riscv32-andes-linux-user.mak      |  1 +
> >  .../targets/riscv32-andes-softmmu.mak         |  1 +
> >  .../targets/riscv64-andes-linux-user.mak      |  1 +
> >  .../targets/riscv64-andes-softmmu.mak         |  1 +
> >  .../targets/rv_custom/no_custom.mak           |  0
> >  .../rv_custom/riscv32-andes-linux-user.mak    |  1 +
> >  .../rv_custom/riscv32-andes-softmmu.mak       |  1 +
> >  .../targets/rv_custom/riscv32-linux-user.mak  |  1 +
> >  .../targets/rv_custom/riscv32-softmmu.mak     |  1 +
> >  .../rv_custom/riscv64-andes-linux-user.mak    |  1 +
> >  .../rv_custom/riscv64-andes-softmmu.mak       |  1 +
> >  .../targets/rv_custom/riscv64-linux-user.mak  |  1 +
> >  .../targets/rv_custom/riscv64-softmmu.mak     |  1 +
> >  meson.build                                   | 26 +++++++++++++++++++
> >  target/riscv/Kconfig                          |  6 +++++
> >  18 files changed, 79 insertions(+)
> >  create mode 100644 default-configs/devices/riscv32-andes-softmmu.mak
> >  create mode 100644 default-configs/devices/riscv64-andes-softmmu.mak
> >  create mode 120000 default-configs/targets/riscv32-andes-linux-user.mak
> >  create mode 120000 default-configs/targets/riscv32-andes-softmmu.mak
> >  create mode 120000 default-configs/targets/riscv64-andes-linux-user.mak
> >  create mode 120000 default-configs/targets/riscv64-andes-softmmu.mak
> >  create mode 100644 default-configs/targets/rv_custom/no_custom.mak
> >  create mode 100644 default-configs/targets/rv_custom/riscv32-andes-linux-user.mak
> >  create mode 100644 default-configs/targets/rv_custom/riscv32-andes-softmmu.mak
> >  create mode 120000 default-configs/targets/rv_custom/riscv32-linux-user.mak
> >  create mode 120000 default-configs/targets/rv_custom/riscv32-softmmu.mak
> >  create mode 100644 default-configs/targets/rv_custom/riscv64-andes-linux-user.mak
> >  create mode 100644 default-configs/targets/rv_custom/riscv64-andes-softmmu.mak
> >  create mode 120000 default-configs/targets/rv_custom/riscv64-linux-user.mak
> >  create mode 120000 default-configs/targets/rv_custom/riscv64-softmmu.mak
> >  create mode 100644 target/riscv/Kconfig
> >
> > --
> > 2.32.0
> >

On Mon, Sep 6, 2021 at 4:49 PM Ruinland ChuanTzu Tsai
<ruinland@andestech.com> wrote:
>
>
> Hi Alistair,
>
> Thanks for the heads up about the upcoming unification of RISC-V 32/64 targets.
> Yet I have several concerns and would like to have some brainstorming regarding
> such topics - -

No worries, I'm happy to discuss.

>
> That is, could you elaborate more about the "runtime check/switch" which you
> mentioned in the previous e-mail :
> https://lists.nongnu.org/archive/html/qemu-devel/2021-08/msg02154.html
> I'm not quite following the context.

Yep, so something along the lines of this in `riscv_csrrw()`

if (cpu == "MyCustomCPU") {
    my_custom_csr[csrno].read();
}

So we check if using the CPU then apply extra CSR accesses.

> If we don't have a way to toggle which (vendor) cores, which will be used,
> during compile time, it means that we have to build all the vendor code and
> link them all together; and we might have the chance to encounter collision on
> csrno between different vendors.

I don't see how they will collide as we will only act on 1, based on
the CPU we are using.

>
> Secondly, I'm not quite sure about how we're going to merge decode tree files
> across RV32 and RV64. Vendor-designed custom instruction would have a different
> encoding scheme on bitfields for RV32 and RV64. Currently, we (Andes) are using
> different decodetree sources for gen32 and gen64 in `target/riscv/meson.build`.

Ok, so custom instructions are a whole different problem. I think we
should leave that for now and focus on CSRs.

A quick look though and I suspect we could do a similar CPU check in
decode_opc(). Dealing with the decodetree will be problematic though.

> I'm preparing the patch to demonstrate such hiccups.
>
> As far as I know, there's no control flow logic for decodetree to parse
> decodetree files differently. (e.g. ifdef XLEN == 32 then blah, blah).
>
> To meet in the halfway, maybe after the grand unification on RV32/64, we can
> still confine vendor custom logic (i.e. instrucions and CSRs) to be toggled by
> whether a certain vendor cpu model is selected ?

I honestly don't see a scenario where that happens. The maintenance
overhead and confusion of changing the CPUs at build time is too high.

I also don't think we should need that for CSR accesses. Custom
instructions are a whole different can of worms.

>
> By the way, I'm wondering how our friends from T-Head (Guo Ren ?) regard this
> issue ? AFAIK, they forked QEMU from v3.2.0 and applied their vendor features
> on top of it for quite a while.

I'm not sure.

Alistair

>
> Cordially yours,
> Ruinland ChuanTzu Tsai
>
> On Thu, Sep 02, 2021 at 12:25:20PM +1000, Alistair Francis wrote:
> > On Fri, Aug 27, 2021 at 1:16 AM Ruinland Chuan-Tzu Tsai
> > <ruinland@andestech.com> wrote:
> > >
> > > From: Ruinland ChuanTzu Tsai <ruinland@andestech.com>
> > >
> > > During my modification on my previous patch series for custom CSR support, I
> > > believe this issue deserves its own discussion (or debate) because it's _not_
> > > as simple as "just put those options in Kconfig".
> > >
> > > The obstables I've encountered and the kluges I came up is listed as follow :
> > >
> > > (1) Due to the design of top-level meson.build, all Kconfig options will land
> > > into `*-config-devices.h` since minikconf will be only used after config_target
> > > being processed. This will let to the fact that linux-users won't be able to
> > > use custom CSR code properly becuase they only includes `*-config-devices.h`.
> > > And that is reasonble due to the fact that changes on cpu.c and csr.c is a
> > > target-related matter and linux-user mode shouldn't include device related
> > > headers in most of cases.
> > >
> > > So, modify meson.build to parse target/riscv/Kconfig during config_target phase
> > > is without doubts necessary.
> > >
> > > (2) Kconfig option `RISCV_CUSTOM_CSR` is introduced for RISC-V cpu models to
> > > toggle it at its will. Yet due to the fact that csr.o and cpu.o are linked
> > > altogether for all CPU models, the suffer will be shared without option.
> > > The only reasonable way to seperate build the fire lane which seperates vendor
> > > flavored cpu and spec-conformed ones, is to build them seperately with options
> > > toggled diffrently, just like RV32 and RV64 shares almost the same source base,
> > > yet the sources are compiled with differnt flags/definitions.
> > >
> > > To achieve that, miraculously, we can just put *.mak files into `target`
> > > directoy, because that's how `configure` enumerates what targets are supported.
> > >
> > > (3) The longest days are not over yet, if we take a good look at how the minikconf
> > > is invoked during config_devices and in what way *.mak presented its options
> > > inside `default-configs/devices`, we can see that *.mak files there is formated
> > > in `CONFIG_*` style and the minikconf is reading directly during config_device
> > > phase. That's totally different from *.mak files presented in
> > > `default-configs/targets`. To make the parsing logic consistent, I
> > > introduce a rv_custom directory inside which contains minikconf-parsable
> > > mak files.
> > >
> > > With this patches, ones can build a A25/AX25 linux-user platform by :
> > > $ ./configure --target-list=riscv64-andes-linux-user,riscv32-andes-linux-user
> >
> > Hey! Thanks for the patches
> >
> > I'm not convinced that we want this though.
> >
> > Right now we are trying to head towards a riscv64-softmmu binary being
> > able to run all RISC-V code. That include 32-bit cpus
> > (qemu-riscv64-softmmu -cpu r32...) and 64-bit CPUs. We shouldn't be
> > splitting out more targets.
> >
> > It also goes against the general idea of RISC-V in that everyone has a
> > standard compliant implementation, they can then add extra
> > functionality.
> >
> > In terms of Kconfig options. It doesn't seem like a bad idea to have
> > an option to fully disable custom CSRs. That way if someone really
> > wants performance and doesn't want custom CSRs they can disable the
> > switch. Otherwise we leave it on and all custom CSRs are available in
> > the build and then controlled by the CPU selection at runtime. If that
> > ends up being too difficult to implement though then we don't have to
> > have it.
> >
> > Thanks again for working on this.
> >
> > Alistair
> >
> >
> > > $ make
> > >
> > > P.S. The pacthes from :
> > > https://lists.gnu.org/archive/html/qemu-devel/2021-08/msg00913.html
> > > is needed. A clean-up and modified version will be sent out soon.
> > >
> > > P.P.S.
> > > I know these parts won't be easy to digest, and the further iterations will be
> > > needed, so I didn't ask my colleagues to sign-off for now.
> > >
> > > Cordially yours,
> > > Ruinland ChuanTzu Tsai
> > >
> > > Ruinland ChuanTzu Tsai (2):
> > >   Adding Kconfig options for custom CSR support and Andes CPU model
> > >   Adding necessary files for Andes platforms, cores to enable custom CSR
> > >     support
> > >
> > >  Kconfig                                       |  1 +
> > >  .../devices/riscv32-andes-softmmu.mak         | 17 ++++++++++++
> > >  .../devices/riscv64-andes-softmmu.mak         | 17 ++++++++++++
> > >  .../targets/riscv32-andes-linux-user.mak      |  1 +
> > >  .../targets/riscv32-andes-softmmu.mak         |  1 +
> > >  .../targets/riscv64-andes-linux-user.mak      |  1 +
> > >  .../targets/riscv64-andes-softmmu.mak         |  1 +
> > >  .../targets/rv_custom/no_custom.mak           |  0
> > >  .../rv_custom/riscv32-andes-linux-user.mak    |  1 +
> > >  .../rv_custom/riscv32-andes-softmmu.mak       |  1 +
> > >  .../targets/rv_custom/riscv32-linux-user.mak  |  1 +
> > >  .../targets/rv_custom/riscv32-softmmu.mak     |  1 +
> > >  .../rv_custom/riscv64-andes-linux-user.mak    |  1 +
> > >  .../rv_custom/riscv64-andes-softmmu.mak       |  1 +
> > >  .../targets/rv_custom/riscv64-linux-user.mak  |  1 +
> > >  .../targets/rv_custom/riscv64-softmmu.mak     |  1 +
> > >  meson.build                                   | 26 +++++++++++++++++++
> > >  target/riscv/Kconfig                          |  6 +++++
> > >  18 files changed, 79 insertions(+)
> > >  create mode 100644 default-configs/devices/riscv32-andes-softmmu.mak
> > >  create mode 100644 default-configs/devices/riscv64-andes-softmmu.mak
> > >  create mode 120000 default-configs/targets/riscv32-andes-linux-user.mak
> > >  create mode 120000 default-configs/targets/riscv32-andes-softmmu.mak
> > >  create mode 120000 default-configs/targets/riscv64-andes-linux-user.mak
> > >  create mode 120000 default-configs/targets/riscv64-andes-softmmu.mak
> > >  create mode 100644 default-configs/targets/rv_custom/no_custom.mak
> > >  create mode 100644 default-configs/targets/rv_custom/riscv32-andes-linux-user.mak
> > >  create mode 100644 default-configs/targets/rv_custom/riscv32-andes-softmmu.mak
> > >  create mode 120000 default-configs/targets/rv_custom/riscv32-linux-user.mak
> > >  create mode 120000 default-configs/targets/rv_custom/riscv32-softmmu.mak
> > >  create mode 100644 default-configs/targets/rv_custom/riscv64-andes-linux-user.mak
> > >  create mode 100644 default-configs/targets/rv_custom/riscv64-andes-softmmu.mak
> > >  create mode 120000 default-configs/targets/rv_custom/riscv64-linux-user.mak
> > >  create mode 120000 default-configs/targets/rv_custom/riscv64-softmmu.mak
> > >  create mode 100644 target/riscv/Kconfig
> > >
> > > --
> > > 2.32.0
> > >

Hi Alistair,

So glad to hear from you.

On Mon, Sep 06, 2021 at 05:05:16PM +1000, Alistair Francis wrote:
> On Mon, Sep 6, 2021 at 4:49 PM Ruinland ChuanTzu Tsai
> <ruinland@andestech.com> wrote:
> >
> >
> > Hi Alistair,
> >
> > Thanks for the heads up about the upcoming unification of RISC-V 32/64 targets.
> > Yet I have several concerns and would like to have some brainstorming regarding
> > such topics - -
> 
> No worries, I'm happy to discuss.
> 
> >
> > That is, could you elaborate more about the "runtime check/switch" which you
> > mentioned in the previous e-mail :
> > https://lists.nongnu.org/archive/html/qemu-devel/2021-08/msg02154.html
> > I'm not quite following the context.
> 
> Yep, so something along the lines of this in `riscv_csrrw()`
> 
> if (cpu == "MyCustomCPU") {
>     my_custom_csr[csrno].read();
> }
> 
> So we check if using the CPU then apply extra CSR accesses.
> 
> > If we don't have a way to toggle which (vendor) cores, which will be used,
> > during compile time, it means that we have to build all the vendor code and
> > link them all together; and we might have the chance to encounter collision on
> > csrno between different vendors.
> 
> I don't see how they will collide as we will only act on 1, based on
> the CPU we are using.

AFAIK, we need to put CSR number into `target/riscv/cpu_bits.h`, and they are
exposed to the global and let others to use it. With my current design, which I
have sent out by RFC patch series v3, I introduced an abstraction layer,
`custom_cpu_bits.h`, which will toggle diffenet set of custom CSR number.

If we teardown the Kconfig, all symbols will be exposed and then it could have
a high chance to collide with each other.

> 
> >
> > Secondly, I'm not quite sure about how we're going to merge decode tree files
> > across RV32 and RV64. Vendor-designed custom instruction would have a different
> > encoding scheme on bitfields for RV32 and RV64. Currently, we (Andes) are using
> > different decodetree sources for gen32 and gen64 in `target/riscv/meson.build`.
> 
> Ok, so custom instructions are a whole different problem. I think we
> should leave that for now and focus on CSRs.
> 
> A quick look though and I suspect we could do a similar CPU check in
> decode_opc(). Dealing with the decodetree will be problematic though.
> 
> > I'm preparing the patch to demonstrate such hiccups.
> >
> > As far as I know, there's no control flow logic for decodetree to parse
> > decodetree files differently. (e.g. ifdef XLEN == 32 then blah, blah).
> >
> > To meet in the halfway, maybe after the grand unification on RV32/64, we can
> > still confine vendor custom logic (i.e. instrucions and CSRs) to be toggled by
> > whether a certain vendor cpu model is selected ?
> 
> I honestly don't see a scenario where that happens. The maintenance
> overhead and confusion of changing the CPUs at build time is too high.
> 
> I also don't think we should need that for CSR accesses. Custom
> instructions are a whole different can of worms.

IMHO, custom CSR and custom instructions are two sides of a same coin in some
way. Let me explain it with an example - - 

Andes has a custom instruction called `EXEC.IT`, which is a 16-bit long com-
pressed instruction. By executing such instrcution, an instruction table
reside in a particular address specified by a custom CSR called uitb will be
fetch, decode and execute. By doing so, the code size could be reduced.

The problem is that we have different address encoding on RV32 and RV64.

And just like you mentioned, in our in-house core, we apply the same logic on
decode_opc() to decode custom instructions first. If such decoding/trans
procedure fails, the original decoder will be invoked.

> 
> >
> > By the way, I'm wondering how our friends from T-Head (Guo Ren ?) regard this
> > issue ? AFAIK, they forked QEMU from v3.2.0 and applied their vendor features
> > on top of it for quite a while.
> 
> I'm not sure.

Sorry for the confusion, I was trying to ping Guo Ren :-D
I CC'ed him in the previous e-mail.

Cordially yours,
Ruinland ChuanTzu Tsai

> 
> Alistair
> 
> >
> > Cordially yours,
> > Ruinland ChuanTzu Tsai
> >
> > On Thu, Sep 02, 2021 at 12:25:20PM +1000, Alistair Francis wrote:
> > > On Fri, Aug 27, 2021 at 1:16 AM Ruinland Chuan-Tzu Tsai
> > > <ruinland@andestech.com> wrote:
> > > >
> > > > From: Ruinland ChuanTzu Tsai <ruinland@andestech.com>
> > > >
> > > > During my modification on my previous patch series for custom CSR support, I
> > > > believe this issue deserves its own discussion (or debate) because it's _not_
> > > > as simple as "just put those options in Kconfig".
> > > >
> > > > The obstables I've encountered and the kluges I came up is listed as follow :
> > > >
> > > > (1) Due to the design of top-level meson.build, all Kconfig options will land
> > > > into `*-config-devices.h` since minikconf will be only used after config_target
> > > > being processed. This will let to the fact that linux-users won't be able to
> > > > use custom CSR code properly becuase they only includes `*-config-devices.h`.
> > > > And that is reasonble due to the fact that changes on cpu.c and csr.c is a
> > > > target-related matter and linux-user mode shouldn't include device related
> > > > headers in most of cases.
> > > >
> > > > So, modify meson.build to parse target/riscv/Kconfig during config_target phase
> > > > is without doubts necessary.
> > > >
> > > > (2) Kconfig option `RISCV_CUSTOM_CSR` is introduced for RISC-V cpu models to
> > > > toggle it at its will. Yet due to the fact that csr.o and cpu.o are linked
> > > > altogether for all CPU models, the suffer will be shared without option.
> > > > The only reasonable way to seperate build the fire lane which seperates vendor
> > > > flavored cpu and spec-conformed ones, is to build them seperately with options
> > > > toggled diffrently, just like RV32 and RV64 shares almost the same source base,
> > > > yet the sources are compiled with differnt flags/definitions.
> > > >
> > > > To achieve that, miraculously, we can just put *.mak files into `target`
> > > > directoy, because that's how `configure` enumerates what targets are supported.
> > > >
> > > > (3) The longest days are not over yet, if we take a good look at how the minikconf
> > > > is invoked during config_devices and in what way *.mak presented its options
> > > > inside `default-configs/devices`, we can see that *.mak files there is formated
> > > > in `CONFIG_*` style and the minikconf is reading directly during config_device
> > > > phase. That's totally different from *.mak files presented in
> > > > `default-configs/targets`. To make the parsing logic consistent, I
> > > > introduce a rv_custom directory inside which contains minikconf-parsable
> > > > mak files.
> > > >
> > > > With this patches, ones can build a A25/AX25 linux-user platform by :
> > > > $ ./configure --target-list=riscv64-andes-linux-user,riscv32-andes-linux-user
> > >
> > > Hey! Thanks for the patches
> > >
> > > I'm not convinced that we want this though.
> > >
> > > Right now we are trying to head towards a riscv64-softmmu binary being
> > > able to run all RISC-V code. That include 32-bit cpus
> > > (qemu-riscv64-softmmu -cpu r32...) and 64-bit CPUs. We shouldn't be
> > > splitting out more targets.
> > >
> > > It also goes against the general idea of RISC-V in that everyone has a
> > > standard compliant implementation, they can then add extra
> > > functionality.
> > >
> > > In terms of Kconfig options. It doesn't seem like a bad idea to have
> > > an option to fully disable custom CSRs. That way if someone really
> > > wants performance and doesn't want custom CSRs they can disable the
> > > switch. Otherwise we leave it on and all custom CSRs are available in
> > > the build and then controlled by the CPU selection at runtime. If that
> > > ends up being too difficult to implement though then we don't have to
> > > have it.
> > >
> > > Thanks again for working on this.
> > >
> > > Alistair
> > >
> > >
> > > > $ make
> > > >
> > > > P.S. The pacthes from :
> > > > https://lists.gnu.org/archive/html/qemu-devel/2021-08/msg00913.html
> > > > is needed. A clean-up and modified version will be sent out soon.
> > > >
> > > > P.P.S.
> > > > I know these parts won't be easy to digest, and the further iterations will be
> > > > needed, so I didn't ask my colleagues to sign-off for now.
> > > >
> > > > Cordially yours,
> > > > Ruinland ChuanTzu Tsai
> > > >
> > > > Ruinland ChuanTzu Tsai (2):
> > > >   Adding Kconfig options for custom CSR support and Andes CPU model
> > > >   Adding necessary files for Andes platforms, cores to enable custom CSR
> > > >     support
> > > >
> > > >  Kconfig                                       |  1 +
> > > >  .../devices/riscv32-andes-softmmu.mak         | 17 ++++++++++++
> > > >  .../devices/riscv64-andes-softmmu.mak         | 17 ++++++++++++
> > > >  .../targets/riscv32-andes-linux-user.mak      |  1 +
> > > >  .../targets/riscv32-andes-softmmu.mak         |  1 +
> > > >  .../targets/riscv64-andes-linux-user.mak      |  1 +
> > > >  .../targets/riscv64-andes-softmmu.mak         |  1 +
> > > >  .../targets/rv_custom/no_custom.mak           |  0
> > > >  .../rv_custom/riscv32-andes-linux-user.mak    |  1 +
> > > >  .../rv_custom/riscv32-andes-softmmu.mak       |  1 +
> > > >  .../targets/rv_custom/riscv32-linux-user.mak  |  1 +
> > > >  .../targets/rv_custom/riscv32-softmmu.mak     |  1 +
> > > >  .../rv_custom/riscv64-andes-linux-user.mak    |  1 +
> > > >  .../rv_custom/riscv64-andes-softmmu.mak       |  1 +
> > > >  .../targets/rv_custom/riscv64-linux-user.mak  |  1 +
> > > >  .../targets/rv_custom/riscv64-softmmu.mak     |  1 +
> > > >  meson.build                                   | 26 +++++++++++++++++++
> > > >  target/riscv/Kconfig                          |  6 +++++
> > > >  18 files changed, 79 insertions(+)
> > > >  create mode 100644 default-configs/devices/riscv32-andes-softmmu.mak
> > > >  create mode 100644 default-configs/devices/riscv64-andes-softmmu.mak
> > > >  create mode 120000 default-configs/targets/riscv32-andes-linux-user.mak
> > > >  create mode 120000 default-configs/targets/riscv32-andes-softmmu.mak
> > > >  create mode 120000 default-configs/targets/riscv64-andes-linux-user.mak
> > > >  create mode 120000 default-configs/targets/riscv64-andes-softmmu.mak
> > > >  create mode 100644 default-configs/targets/rv_custom/no_custom.mak
> > > >  create mode 100644 default-configs/targets/rv_custom/riscv32-andes-linux-user.mak
> > > >  create mode 100644 default-configs/targets/rv_custom/riscv32-andes-softmmu.mak
> > > >  create mode 120000 default-configs/targets/rv_custom/riscv32-linux-user.mak
> > > >  create mode 120000 default-configs/targets/rv_custom/riscv32-softmmu.mak
> > > >  create mode 100644 default-configs/targets/rv_custom/riscv64-andes-linux-user.mak
> > > >  create mode 100644 default-configs/targets/rv_custom/riscv64-andes-softmmu.mak
> > > >  create mode 120000 default-configs/targets/rv_custom/riscv64-linux-user.mak
> > > >  create mode 120000 default-configs/targets/rv_custom/riscv64-softmmu.mak
> > > >  create mode 100644 target/riscv/Kconfig
> > > >
> > > > --
> > > > 2.32.0
> > > >

On Mon, Sep 6, 2021 at 5:37 PM Ruinland ChuanTzu Tsai
<ruinland@andestech.com> wrote:
>
> Hi Alistair,
>
> So glad to hear from you.
>
> On Mon, Sep 06, 2021 at 05:05:16PM +1000, Alistair Francis wrote:
> > On Mon, Sep 6, 2021 at 4:49 PM Ruinland ChuanTzu Tsai
> > <ruinland@andestech.com> wrote:
> > >
> > >
> > > Hi Alistair,
> > >
> > > Thanks for the heads up about the upcoming unification of RISC-V 32/64 targets.
> > > Yet I have several concerns and would like to have some brainstorming regarding
> > > such topics - -
> >
> > No worries, I'm happy to discuss.
> >
> > >
> > > That is, could you elaborate more about the "runtime check/switch" which you
> > > mentioned in the previous e-mail :
> > > https://lists.nongnu.org/archive/html/qemu-devel/2021-08/msg02154.html
> > > I'm not quite following the context.
> >
> > Yep, so something along the lines of this in `riscv_csrrw()`
> >
> > if (cpu == "MyCustomCPU") {
> >     my_custom_csr[csrno].read();
> > }
> >
> > So we check if using the CPU then apply extra CSR accesses.
> >
> > > If we don't have a way to toggle which (vendor) cores, which will be used,
> > > during compile time, it means that we have to build all the vendor code and
> > > link them all together; and we might have the chance to encounter collision on
> > > csrno between different vendors.
> >
> > I don't see how they will collide as we will only act on 1, based on
> > the CPU we are using.
>
> AFAIK, we need to put CSR number into `target/riscv/cpu_bits.h`, and they are
> exposed to the global and let others to use it. With my current design, which I
> have sent out by RFC patch series v3, I introduced an abstraction layer,
> `custom_cpu_bits.h`, which will toggle diffenet set of custom CSR number.
>
> If we teardown the Kconfig, all symbols will be exposed and then it could have
> a high chance to collide with each other.

I guess this depends on what you are trying to do.

We could have non public CSRs. So each CPU could have it's own custom
version of `riscv_csr_operations csr_ops[CSR_TABLE_SIZE]` which is in
it's own C file. We then just add a switch case to CSR accesses and if
using CPU "customcpu" then we check the `custom_cpu_csr_ops` table.
NOTE: That we can do something smarter than a switch, but you get the
point. We can implement a read/write function for every element in the
array, with the default just triggering an illegal instruction.

I guess that assumes that each CSR access is self contained. For
example if changing a custom CSR changes a core part of the
target/riscv code this won't really work.

On the other hand I'm not convinced we want vendor changes to affect
the core target/riscv code. Ideally all vendor code can be kept in
it's own file and it's fully self contained. That won't work for
everything, but it should work for enough use cases. We can even have
a custom vendor state that the vendor code can use (it can also change
the CPU state).

Does that make sense?

>
> >
> > >
> > > Secondly, I'm not quite sure about how we're going to merge decode tree files
> > > across RV32 and RV64. Vendor-designed custom instruction would have a different
> > > encoding scheme on bitfields for RV32 and RV64. Currently, we (Andes) are using
> > > different decodetree sources for gen32 and gen64 in `target/riscv/meson.build`.
> >
> > Ok, so custom instructions are a whole different problem. I think we
> > should leave that for now and focus on CSRs.
> >
> > A quick look though and I suspect we could do a similar CPU check in
> > decode_opc(). Dealing with the decodetree will be problematic though.
> >
> > > I'm preparing the patch to demonstrate such hiccups.
> > >
> > > As far as I know, there's no control flow logic for decodetree to parse
> > > decodetree files differently. (e.g. ifdef XLEN == 32 then blah, blah).
> > >
> > > To meet in the halfway, maybe after the grand unification on RV32/64, we can
> > > still confine vendor custom logic (i.e. instrucions and CSRs) to be toggled by
> > > whether a certain vendor cpu model is selected ?
> >wtih  the d
> > I honestly don't see a scenario where that happens. The maintenance
> > overhead and confusion of changing the CPUs at build time is too high.
> >
> > I also don't think we should need that for CSR accesses. Custom
> > instructions are a whole different can of worms.
>
> IMHO, custom CSR and custom instructions are two sides of a same coin in some
> way. Let me explain it with an example - -
>
> Andes has a custom instruction called `EXEC.IT`, which is a 16-bit long com-
> pressed instruction. By executing such instrcution, an instruction table
> reside in a particular address specified by a custom CSR called uitb will be
> fetch, decode and execute. By doing so, the code size could be reduced.

Hmmm... This is a much more complex use case than I was expecting. I
have been thinking more about custom CSRs to set a timer or control
the interrupt controller.

Something like what you described is going to be a lot more work.

In your case though I think we can still focus on the CSR aspect
first. Once that is sorted we can then look at the instruction part.

The main aim should be that all (or almost all) vendor code lives in
it's own file.

Alistair

>
> The problem is that we have different address encoding on RV32 and RV64.
>
> And just like you mentioned, in our in-house core, we apply the same logic on
> decode_opc() to decode custom instructions first. If such decoding/trans
> procedure fails, the original decoder will be invoked.
>
> >
> > >
> > > By the way, I'm wondering how our friends from T-Head (Guo Ren ?) regard this
> > > issue ? AFAIK, they forked QEMU from v3.2.0 and applied their vendor features
> > > on top of it for quite a while.
> >
> > I'm not sure.
>
> Sorry for the confusion, I was trying to ping Guo Ren :-D
> I CC'ed him in the previous e-mail.
>
> Cordially yours,
> Ruinland ChuanTzu Tsai
>
> >
> > Alistair
> >
> > >
> > > Cordially yours,
> > > Ruinland ChuanTzu Tsai
> > >
> > > On Thu, Sep 02, 2021 at 12:25:20PM +1000, Alistair Francis wrote:
> > > > On Fri, Aug 27, 2021 at 1:16 AM Ruinland Chuan-Tzu Tsai
> > > > <ruinland@andestech.com> wrote:
> > > > >
> > > > > From: Ruinland ChuanTzu Tsai <ruinland@andestech.com>
> > > > >
> > > > > During my modification on my previous patch series for custom CSR support, I
> > > > > believe this issue deserves its own discussion (or debate) because it's _not_
> > > > > as simple as "just put those options in Kconfig".
> > > > >
> > > > > The obstables I've encountered and the kluges I came up is listed as follow :
> > > > >
> > > > > (1) Due to the design of top-level meson.build, all Kconfig options will land
> > > > > into `*-config-devices.h` since minikconf will be only used after config_target
> > > > > being processed. This will let to the fact that linux-users won't be able to
> > > > > use custom CSR code properly becuase they only includes `*-config-devices.h`.
> > > > > And that is reasonble due to the fact that changes on cpu.c and csr.c is a
> > > > > target-related matter and linux-user mode shouldn't include device related
> > > > > headers in most of cases.
> > > > >
> > > > > So, modify meson.build to parse target/riscv/Kconfig during config_target phase
> > > > > is without doubts necessary.
> > > > >
> > > > > (2) Kconfig option `RISCV_CUSTOM_CSR` is introduced for RISC-V cpu models to
> > > > > toggle it at its will. Yet due to the fact that csr.o and cpu.o are linked
> > > > > altogether for all CPU models, the suffer will be shared without option.
> > > > > The only reasonable way to seperate build the fire lane which seperates vendor
> > > > > flavored cpu and spec-conformed ones, is to build them seperately with options
> > > > > toggled diffrently, just like RV32 and RV64 shares almost the same source base,
> > > > > yet the sources are compiled with differnt flags/definitions.
> > > > >
> > > > > To achieve that, miraculously, we can just put *.mak files into `target`
> > > > > directoy, because that's how `configure` enumerates what targets are supported.
> > > > >
> > > > > (3) The longest days are not over yet, if we take a good look at how the minikconf
> > > > > is invoked during config_devices and in what way *.mak presented its options
> > > > > inside `default-configs/devices`, we can see that *.mak files there is formated
> > > > > in `CONFIG_*` style and the minikconf is reading directly during config_device
> > > > > phase. That's totally different from *.mak files presented in
> > > > > `default-configs/targets`. To make the parsing logic consistent, I
> > > > > introduce a rv_custom directory inside which contains minikconf-parsable
> > > > > mak files.
> > > > >
> > > > > With this patches, ones can build a A25/AX25 linux-user platform by :
> > > > > $ ./configure --target-list=riscv64-andes-linux-user,riscv32-andes-linux-user
> > > >
> > > > Hey! Thanks for the patches
> > > >
> > > > I'm not convinced that we want this though.
> > > >
> > > > Right now we are trying to head towards a riscv64-softmmu binary being
> > > > able to run all RISC-V code. That include 32-bit cpus
> > > > (qemu-riscv64-softmmu -cpu r32...) and 64-bit CPUs. We shouldn't be
> > > > splitting out more targets.
> > > >
> > > > It also goes against the general idea of RISC-V in that everyone has a
> > > > standard compliant implementation, they can then add extra
> > > > functionality.
> > > >
> > > > In terms of Kconfig options. It doesn't seem like a bad idea to have
> > > > an option to fully disable custom CSRs. That way if someone really
> > > > wants performance and doesn't want custom CSRs they can disable the
> > > > switch. Otherwise we leave it on and all custom CSRs are available in
> > > > the build and then controlled by the CPU selection at runtime. If that
> > > > ends up being too difficult to implement though then we don't have to
> > > > have it.
> > > >
> > > > Thanks again for working on this.
> > > >
> > > > Alistair
> > > >
> > > >
> > > > > $ make
> > > > >
> > > > > P.S. The pacthes from :
> > > > > https://lists.gnu.org/archive/html/qemu-devel/2021-08/msg00913.html
> > > > > is needed. A clean-up and modified version will be sent out soon.
> > > > >
> > > > > P.P.S.
> > > > > I know these parts won't be easy to digest, and the further iterations will be
> > > > > needed, so I didn't ask my colleagues to sign-off for now.
> > > > >
> > > > > Cordially yours,
> > > > > Ruinland ChuanTzu Tsai
> > > > >
> > > > > Ruinland ChuanTzu Tsai (2):
> > > > >   Adding Kconfig options for custom CSR support and Andes CPU model
> > > > >   Adding necessary files for Andes platforms, cores to enable custom CSR
> > > > >     support
> > > > >
> > > > >  Kconfig                                       |  1 +
> > > > >  .../devices/riscv32-andes-softmmu.mak         | 17 ++++++++++++
> > > > >  .../devices/riscv64-andes-softmmu.mak         | 17 ++++++++++++
> > > > >  .../targets/riscv32-andes-linux-user.mak      |  1 +
> > > > >  .../targets/riscv32-andes-softmmu.mak         |  1 +
> > > > >  .../targets/riscv64-andes-linux-user.mak      |  1 +
> > > > >  .../targets/riscv64-andes-softmmu.mak         |  1 +
> > > > >  .../targets/rv_custom/no_custom.mak           |  0
> > > > >  .../rv_custom/riscv32-andes-linux-user.mak    |  1 +
> > > > >  .../rv_custom/riscv32-andes-softmmu.mak       |  1 +
> > > > >  .../targets/rv_custom/riscv32-linux-user.mak  |  1 +
> > > > >  .../targets/rv_custom/riscv32-softmmu.mak     |  1 +
> > > > >  .../rv_custom/riscv64-andes-linux-user.mak    |  1 +
> > > > >  .../rv_custom/riscv64-andes-softmmu.mak       |  1 +
> > > > >  .../targets/rv_custom/riscv64-linux-user.mak  |  1 +
> > > > >  .../targets/rv_custom/riscv64-softmmu.mak     |  1 +
> > > > >  meson.build                                   | 26 +++++++++++++++++++
> > > > >  target/riscv/Kconfig                          |  6 +++++
> > > > >  18 files changed, 79 insertions(+)
> > > > >  create mode 100644 default-configs/devices/riscv32-andes-softmmu.mak
> > > > >  create mode 100644 default-configs/devices/riscv64-andes-softmmu.mak
> > > > >  create mode 120000 default-configs/targets/riscv32-andes-linux-user.mak
> > > > >  create mode 120000 default-configs/targets/riscv32-andes-softmmu.mak
> > > > >  create mode 120000 default-configs/targets/riscv64-andes-linux-user.mak
> > > > >  create mode 120000 default-configs/targets/riscv64-andes-softmmu.mak
> > > > >  create mode 100644 default-configs/targets/rv_custom/no_custom.mak
> > > > >  create mode 100644 default-configs/targets/rv_custom/riscv32-andes-linux-user.mak
> > > > >  create mode 100644 default-configs/targets/rv_custom/riscv32-andes-softmmu.mak
> > > > >  create mode 120000 default-configs/targets/rv_custom/riscv32-linux-user.mak
> > > > >  create mode 120000 default-configs/targets/rv_custom/riscv32-softmmu.mak
> > > > >  create mode 100644 default-configs/targets/rv_custom/riscv64-andes-linux-user.mak
> > > > >  create mode 100644 default-configs/targets/rv_custom/riscv64-andes-softmmu.mak
> > > > >  create mode 120000 default-configs/targets/rv_custom/riscv64-linux-user.mak
> > > > >  create mode 120000 default-configs/targets/rv_custom/riscv64-softmmu.mak
> > > > >  create mode 100644 target/riscv/Kconfig
> > > > >
> > > > > --
> > > > > 2.32.0
> > > > >

Hi Alistair,

Thanks for the comment.

On Mon, Sep 06, 2021 at 05:55:25PM +1000, Alistair Francis wrote:
> On Mon, Sep 6, 2021 at 5:37 PM Ruinland ChuanTzu Tsai
> <ruinland@andestech.com> wrote:
> >
> > Hi Alistair,
> >
> > So glad to hear from you.
> >
> > On Mon, Sep 06, 2021 at 05:05:16PM +1000, Alistair Francis wrote:
> > > On Mon, Sep 6, 2021 at 4:49 PM Ruinland ChuanTzu Tsai
> > > <ruinland@andestech.com> wrote:
> > > >
> > > >
> > > > Hi Alistair,
> > > >
> > > > Thanks for the heads up about the upcoming unification of RISC-V 32/64 targets.
> > > > Yet I have several concerns and would like to have some brainstorming regarding
> > > > such topics - -
> > >
> > > No worries, I'm happy to discuss.
> > >
> > > >
> > > > That is, could you elaborate more about the "runtime check/switch" which you
> > > > mentioned in the previous e-mail :
> > > > https://lists.nongnu.org/archive/html/qemu-devel/2021-08/msg02154.html
> > > > I'm not quite following the context.
> > >
> > > Yep, so something along the lines of this in `riscv_csrrw()`
> > >
> > > if (cpu == "MyCustomCPU") {
> > >     my_custom_csr[csrno].read();
> > > }
> > >
> > > So we check if using the CPU then apply extra CSR accesses.
> > >
> > > > If we don't have a way to toggle which (vendor) cores, which will be used,
> > > > during compile time, it means that we have to build all the vendor code and
> > > > link them all together; and we might have the chance to encounter collision on
> > > > csrno between different vendors.
> > >
> > > I don't see how they will collide as we will only act on 1, based on
> > > the CPU we are using.
> >
> > AFAIK, we need to put CSR number into `target/riscv/cpu_bits.h`, and they are
> > exposed to the global and let others to use it. With my current design, which I
> > have sent out by RFC patch series v3, I introduced an abstraction layer,
> > `custom_cpu_bits.h`, which will toggle diffenet set of custom CSR number.
> >
> > If we teardown the Kconfig, all symbols will be exposed and then it could have
> > a high chance to collide with each other.
> 
> I guess this depends on what you are trying to do.
> 
> We could have non public CSRs. So each CPU could have it's own custom
> version of `riscv_csr_operations csr_ops[CSR_TABLE_SIZE]` which is in
> it's own C file. We then just add a switch case to CSR accesses and if
> using CPU "customcpu" then we check the `custom_cpu_csr_ops` table.
> NOTE: That we can do something smarter than a switch, but you get the
> point. We can implement a read/write function for every element in the
> array, with the default just triggering an illegal instruction.

One thing I would like to discuss here.

Firstly, I'm not quite sure how the picture of non-public CSR looks like.
Is it suggested that non-standard CSR number shall not be exposed ?

I know that we should focus on custom CSR part, yet I need to make sure
that if following logic is permitted to appear in `target/riscv/trans_insn` :

trans_vendor_A_insn_blah(...) {
    riscv_csrrw(env, CSR_VENDOR_A_CUSTOM, r, n, write_mask);
    }

As far as I know, csr number is not presented as a C++ enum, which we
can access via csr::custom::vendora::foobar, so it's a globally exposed
macro with possiblilty of collision.

IMHO, the key is that are we permitted to have a uniform interface to access
CSR, either standard or vendor designed ones, in other parts of QEMU.


> I guess that assumes that each CSR access is self contained. For
> example if changing a custom CSR changes a core part of the
> target/riscv code this won't really work.
> 
> On the other hand I'm not convinced we want vendor changes to affect
> the core target/riscv code. Ideally all vendor code can be kept in
> it's own file and it's fully self contained. That won't work for
> everything, but it should work for enough use cases. We can even have
> a custom vendor state that the vendor code can use (it can also change
> the CPU state).
> 
> Does that make sense?

In general, I agree with the point that vendor code should be self-contained.
Yet I have doubts that with the current design of CPU model, are we able to
unify the targets and in the meanwhile to keep things tight and neat ?

The execution flow will be bonded to have a shared instruction decoder/
translator and a shared handler for CSR (i.e. riscv_csrrw). It's not like we
get to choose what decoder we want to use or which CSR table we will be
using at xxx_cpu_init(). If we choose to use runtime check/diversion of all of
these parts, the overhead might be tremendous. 

Surely we should be focusing on CSR part for now, and just as you said, CSR
is not that perforamnce-centric.

Yet if we take a look at `configs/targets`, still we're having 6 MIPS32/64
linux-user targets, 4 ARM32/64 linux-user targets and 4 PPC32/64 linux-user
targets.

I guess it will be a very long journey to merge all the variants.

> 
> >
> > >
> > > >
> > > > Secondly, I'm not quite sure about how we're going to merge decode tree files
> > > > across RV32 and RV64. Vendor-designed custom instruction would have a different
> > > > encoding scheme on bitfields for RV32 and RV64. Currently, we (Andes) are using
> > > > different decodetree sources for gen32 and gen64 in `target/riscv/meson.build`.
> > >
> > > Ok, so custom instructions are a whole different problem. I think we
> > > should leave that for now and focus on CSRs.
> > >
> > > A quick look though and I suspect we could do a similar CPU check in
> > > decode_opc(). Dealing with the decodetree will be problematic though.
> > >
> > > > I'm preparing the patch to demonstrate such hiccups.
> > > >
> > > > As far as I know, there's no control flow logic for decodetree to parse
> > > > decodetree files differently. (e.g. ifdef XLEN == 32 then blah, blah).
> > > >
> > > > To meet in the halfway, maybe after the grand unification on RV32/64, we can
> > > > still confine vendor custom logic (i.e. instrucions and CSRs) to be toggled by
> > > > whether a certain vendor cpu model is selected ?
> > >wtih  the d
> > > I honestly don't see a scenario where that happens. The maintenance
> > > overhead and confusion of changing the CPUs at build time is too high.
> > >
> > > I also don't think we should need that for CSR accesses. Custom
> > > instructions are a whole different can of worms.
> >
> > IMHO, custom CSR and custom instructions are two sides of a same coin in some
> > way. Let me explain it with an example - -
> >
> > Andes has a custom instruction called `EXEC.IT`, which is a 16-bit long com-
> > pressed instruction. By executing such instrcution, an instruction table
> > reside in a particular address specified by a custom CSR called uitb will be
> > fetch, decode and execute. By doing so, the code size could be reduced.
> 
> Hmmm... This is a much more complex use case than I was expecting. I
> have been thinking more about custom CSRs to set a timer or control
> the interrupt controller.
> 
> Something like what you described is going to be a lot more work.
> 
> In your case though I think we can still focus on the CSR aspect
> first. Once that is sorted we can then look at the instruction part.

Just like the mentioned question above , I'm wondering if we can assume
riscv_csrrw() to be a general interface for accessing all the CSRs ?

Cordially yours,
Ruinland ChuanTzu Tsai

> The main aim should be that all (or almost all) vendor code lives in
> it's own file.
> 
> Alistair
> 
> >
> > The problem is that we have different address encoding on RV32 and RV64.
> >
> > And just like you mentioned, in our in-house core, we apply the same logic on
> > decode_opc() to decode custom instructions first. If such decoding/trans
> > procedure fails, the original decoder will be invoked.
> >
> > >
> > > >
> > > > By the way, I'm wondering how our friends from T-Head (Guo Ren ?) regard this
> > > > issue ? AFAIK, they forked QEMU from v3.2.0 and applied their vendor features
> > > > on top of it for quite a while.
> > >
> > > I'm not sure.
> >
> > Sorry for the confusion, I was trying to ping Guo Ren :-D
> > I CC'ed him in the previous e-mail.
> >
> > Cordially yours,
> > Ruinland ChuanTzu Tsai
> >
> > >
> > > Alistair
> > >
> > > >
> > > > Cordially yours,
> > > > Ruinland ChuanTzu Tsai
> > > >
> > > > On Thu, Sep 02, 2021 at 12:25:20PM +1000, Alistair Francis wrote:
> > > > > On Fri, Aug 27, 2021 at 1:16 AM Ruinland Chuan-Tzu Tsai
> > > > > <ruinland@andestech.com> wrote:
> > > > > >
> > > > > > From: Ruinland ChuanTzu Tsai <ruinland@andestech.com>
> > > > > >
> > > > > > During my modification on my previous patch series for custom CSR support, I
> > > > > > believe this issue deserves its own discussion (or debate) because it's _not_
> > > > > > as simple as "just put those options in Kconfig".
> > > > > >
> > > > > > The obstables I've encountered and the kluges I came up is listed as follow :
> > > > > >
> > > > > > (1) Due to the design of top-level meson.build, all Kconfig options will land
> > > > > > into `*-config-devices.h` since minikconf will be only used after config_target
> > > > > > being processed. This will let to the fact that linux-users won't be able to
> > > > > > use custom CSR code properly becuase they only includes `*-config-devices.h`.
> > > > > > And that is reasonble due to the fact that changes on cpu.c and csr.c is a
> > > > > > target-related matter and linux-user mode shouldn't include device related
> > > > > > headers in most of cases.
> > > > > >
> > > > > > So, modify meson.build to parse target/riscv/Kconfig during config_target phase
> > > > > > is without doubts necessary.
> > > > > >
> > > > > > (2) Kconfig option `RISCV_CUSTOM_CSR` is introduced for RISC-V cpu models to
> > > > > > toggle it at its will. Yet due to the fact that csr.o and cpu.o are linked
> > > > > > altogether for all CPU models, the suffer will be shared without option.
> > > > > > The only reasonable way to seperate build the fire lane which seperates vendor
> > > > > > flavored cpu and spec-conformed ones, is to build them seperately with options
> > > > > > toggled diffrently, just like RV32 and RV64 shares almost the same source base,
> > > > > > yet the sources are compiled with differnt flags/definitions.
> > > > > >
> > > > > > To achieve that, miraculously, we can just put *.mak files into `target`
> > > > > > directoy, because that's how `configure` enumerates what targets are supported.
> > > > > >
> > > > > > (3) The longest days are not over yet, if we take a good look at how the minikconf
> > > > > > is invoked during config_devices and in what way *.mak presented its options
> > > > > > inside `default-configs/devices`, we can see that *.mak files there is formated
> > > > > > in `CONFIG_*` style and the minikconf is reading directly during config_device
> > > > > > phase. That's totally different from *.mak files presented in
> > > > > > `default-configs/targets`. To make the parsing logic consistent, I
> > > > > > introduce a rv_custom directory inside which contains minikconf-parsable
> > > > > > mak files.
> > > > > >
> > > > > > With this patches, ones can build a A25/AX25 linux-user platform by :
> > > > > > $ ./configure --target-list=riscv64-andes-linux-user,riscv32-andes-linux-user
> > > > >
> > > > > Hey! Thanks for the patches
> > > > >
> > > > > I'm not convinced that we want this though.
> > > > >
> > > > > Right now we are trying to head towards a riscv64-softmmu binary being
> > > > > able to run all RISC-V code. That include 32-bit cpus
> > > > > (qemu-riscv64-softmmu -cpu r32...) and 64-bit CPUs. We shouldn't be
> > > > > splitting out more targets.
> > > > >
> > > > > It also goes against the general idea of RISC-V in that everyone has a
> > > > > standard compliant implementation, they can then add extra
> > > > > functionality.
> > > > >
> > > > > In terms of Kconfig options. It doesn't seem like a bad idea to have
> > > > > an option to fully disable custom CSRs. That way if someone really
> > > > > wants performance and doesn't want custom CSRs they can disable the
> > > > > switch. Otherwise we leave it on and all custom CSRs are available in
> > > > > the build and then controlled by the CPU selection at runtime. If that
> > > > > ends up being too difficult to implement though then we don't have to
> > > > > have it.
> > > > >
> > > > > Thanks again for working on this.
> > > > >
> > > > > Alistair
> > > > >
> > > > >
> > > > > > $ make
> > > > > >
> > > > > > P.S. The pacthes from :
> > > > > > https://lists.gnu.org/archive/html/qemu-devel/2021-08/msg00913.html
> > > > > > is needed. A clean-up and modified version will be sent out soon.
> > > > > >
> > > > > > P.P.S.
> > > > > > I know these parts won't be easy to digest, and the further iterations will be
> > > > > > needed, so I didn't ask my colleagues to sign-off for now.
> > > > > >
> > > > > > Cordially yours,
> > > > > > Ruinland ChuanTzu Tsai
> > > > > >
> > > > > > Ruinland ChuanTzu Tsai (2):
> > > > > >   Adding Kconfig options for custom CSR support and Andes CPU model
> > > > > >   Adding necessary files for Andes platforms, cores to enable custom CSR
> > > > > >     support
> > > > > >
> > > > > >  Kconfig                                       |  1 +
> > > > > >  .../devices/riscv32-andes-softmmu.mak         | 17 ++++++++++++
> > > > > >  .../devices/riscv64-andes-softmmu.mak         | 17 ++++++++++++
> > > > > >  .../targets/riscv32-andes-linux-user.mak      |  1 +
> > > > > >  .../targets/riscv32-andes-softmmu.mak         |  1 +
> > > > > >  .../targets/riscv64-andes-linux-user.mak      |  1 +
> > > > > >  .../targets/riscv64-andes-softmmu.mak         |  1 +
> > > > > >  .../targets/rv_custom/no_custom.mak           |  0
> > > > > >  .../rv_custom/riscv32-andes-linux-user.mak    |  1 +
> > > > > >  .../rv_custom/riscv32-andes-softmmu.mak       |  1 +
> > > > > >  .../targets/rv_custom/riscv32-linux-user.mak  |  1 +
> > > > > >  .../targets/rv_custom/riscv32-softmmu.mak     |  1 +
> > > > > >  .../rv_custom/riscv64-andes-linux-user.mak    |  1 +
> > > > > >  .../rv_custom/riscv64-andes-softmmu.mak       |  1 +
> > > > > >  .../targets/rv_custom/riscv64-linux-user.mak  |  1 +
> > > > > >  .../targets/rv_custom/riscv64-softmmu.mak     |  1 +
> > > > > >  meson.build                                   | 26 +++++++++++++++++++
> > > > > >  target/riscv/Kconfig                          |  6 +++++
> > > > > >  18 files changed, 79 insertions(+)
> > > > > >  create mode 100644 default-configs/devices/riscv32-andes-softmmu.mak
> > > > > >  create mode 100644 default-configs/devices/riscv64-andes-softmmu.mak
> > > > > >  create mode 120000 default-configs/targets/riscv32-andes-linux-user.mak
> > > > > >  create mode 120000 default-configs/targets/riscv32-andes-softmmu.mak
> > > > > >  create mode 120000 default-configs/targets/riscv64-andes-linux-user.mak
> > > > > >  create mode 120000 default-configs/targets/riscv64-andes-softmmu.mak
> > > > > >  create mode 100644 default-configs/targets/rv_custom/no_custom.mak
> > > > > >  create mode 100644 default-configs/targets/rv_custom/riscv32-andes-linux-user.mak
> > > > > >  create mode 100644 default-configs/targets/rv_custom/riscv32-andes-softmmu.mak
> > > > > >  create mode 120000 default-configs/targets/rv_custom/riscv32-linux-user.mak
> > > > > >  create mode 120000 default-configs/targets/rv_custom/riscv32-softmmu.mak
> > > > > >  create mode 100644 default-configs/targets/rv_custom/riscv64-andes-linux-user.mak
> > > > > >  create mode 100644 default-configs/targets/rv_custom/riscv64-andes-softmmu.mak
> > > > > >  create mode 120000 default-configs/targets/rv_custom/riscv64-linux-user.mak
> > > > > >  create mode 120000 default-configs/targets/rv_custom/riscv64-softmmu.mak
> > > > > >  create mode 100644 target/riscv/Kconfig
> > > > > >
> > > > > > --
> > > > > > 2.32.0
> > > > > >

Hi Alistair,

One clarification: The unification of architectures is also going to allow
multi-arch CPUs (RV32/RV64) in a single machine instance? Or it's just
limited to only one in the runtime.

Rahul

On Tue, Sep 7, 2021 at 1:37 PM Ruinland ChuanTzu Tsai <
ruinland@andestech.com> wrote:

> Hi Alistair,
>
> Thanks for the comment.
>
> On Mon, Sep 06, 2021 at 05:55:25PM +1000, Alistair Francis wrote:
> > On Mon, Sep 6, 2021 at 5:37 PM Ruinland ChuanTzu Tsai
> > <ruinland@andestech.com> wrote:
> > >
> > > Hi Alistair,
> > >
> > > So glad to hear from you.
> > >
> > > On Mon, Sep 06, 2021 at 05:05:16PM +1000, Alistair Francis wrote:
> > > > On Mon, Sep 6, 2021 at 4:49 PM Ruinland ChuanTzu Tsai
> > > > <ruinland@andestech.com> wrote:
> > > > >
> > > > >
> > > > > Hi Alistair,
> > > > >
> > > > > Thanks for the heads up about the upcoming unification of RISC-V
> 32/64 targets.
> > > > > Yet I have several concerns and would like to have some
> brainstorming regarding
> > > > > such topics - -
> > > >
> > > > No worries, I'm happy to discuss.
> > > >
> > > > >
> > > > > That is, could you elaborate more about the "runtime check/switch"
> which you
> > > > > mentioned in the previous e-mail :
> > > > >
> https://lists.nongnu.org/archive/html/qemu-devel/2021-08/msg02154.html
> > > > > I'm not quite following the context.
> > > >
> > > > Yep, so something along the lines of this in `riscv_csrrw()`
> > > >
> > > > if (cpu == "MyCustomCPU") {
> > > >     my_custom_csr[csrno].read();
> > > > }
> > > >
> > > > So we check if using the CPU then apply extra CSR accesses.
> > > >
> > > > > If we don't have a way to toggle which (vendor) cores, which will
> be used,
> > > > > during compile time, it means that we have to build all the vendor
> code and
> > > > > link them all together; and we might have the chance to encounter
> collision on
> > > > > csrno between different vendors.
> > > >
> > > > I don't see how they will collide as we will only act on 1, based on
> > > > the CPU we are using.
> > >
> > > AFAIK, we need to put CSR number into `target/riscv/cpu_bits.h`, and
> they are
> > > exposed to the global and let others to use it. With my current
> design, which I
> > > have sent out by RFC patch series v3, I introduced an abstraction
> layer,
> > > `custom_cpu_bits.h`, which will toggle diffenet set of custom CSR
> number.
> > >
> > > If we teardown the Kconfig, all symbols will be exposed and then it
> could have
> > > a high chance to collide with each other.
> >
> > I guess this depends on what you are trying to do.
> >
> > We could have non public CSRs. So each CPU could have it's own custom
> > version of `riscv_csr_operations csr_ops[CSR_TABLE_SIZE]` which is in
> > it's own C file. We then just add a switch case to CSR accesses and if
> > using CPU "customcpu" then we check the `custom_cpu_csr_ops` table.
> > NOTE: That we can do something smarter than a switch, but you get the
> > point. We can implement a read/write function for every element in the
> > array, with the default just triggering an illegal instruction.
>
> One thing I would like to discuss here.
>
> Firstly, I'm not quite sure how the picture of non-public CSR looks like.
> Is it suggested that non-standard CSR number shall not be exposed ?
>
> I know that we should focus on custom CSR part, yet I need to make sure
> that if following logic is permitted to appear in
> `target/riscv/trans_insn` :
>
> trans_vendor_A_insn_blah(...) {
>     riscv_csrrw(env, CSR_VENDOR_A_CUSTOM, r, n, write_mask);
>     }
>
> As far as I know, csr number is not presented as a C++ enum, which we
> can access via csr::custom::vendora::foobar, so it's a globally exposed
> macro with possiblilty of collision.
>
> IMHO, the key is that are we permitted to have a uniform interface to
> access
> CSR, either standard or vendor designed ones, in other parts of QEMU.
>
>
> > I guess that assumes that each CSR access is self contained. For
> > example if changing a custom CSR changes a core part of the
> > target/riscv code this won't really work.
> >
> > On the other hand I'm not convinced we want vendor changes to affect
> > the core target/riscv code. Ideally all vendor code can be kept in
> > it's own file and it's fully self contained. That won't work for
> > everything, but it should work for enough use cases. We can even have
> > a custom vendor state that the vendor code can use (it can also change
> > the CPU state).
> >
> > Does that make sense?
>
> In general, I agree with the point that vendor code should be
> self-contained.
> Yet I have doubts that with the current design of CPU model, are we able to
> unify the targets and in the meanwhile to keep things tight and neat ?
>
> The execution flow will be bonded to have a shared instruction decoder/
> translator and a shared handler for CSR (i.e. riscv_csrrw). It's not like
> we
> get to choose what decoder we want to use or which CSR table we will be
> using at xxx_cpu_init(). If we choose to use runtime check/diversion of
> all of
> these parts, the overhead might be tremendous.
>
> Surely we should be focusing on CSR part for now, and just as you said, CSR
> is not that perforamnce-centric.
>
> Yet if we take a look at `configs/targets`, still we're having 6 MIPS32/64
> linux-user targets, 4 ARM32/64 linux-user targets and 4 PPC32/64 linux-user
> targets.
>
> I guess it will be a very long journey to merge all the variants.
>
> >
> > >
> > > >
> > > > >
> > > > > Secondly, I'm not quite sure about how we're going to merge decode
> tree files
> > > > > across RV32 and RV64. Vendor-designed custom instruction would
> have a different
> > > > > encoding scheme on bitfields for RV32 and RV64. Currently, we
> (Andes) are using
> > > > > different decodetree sources for gen32 and gen64 in
> `target/riscv/meson.build`.
> > > >
> > > > Ok, so custom instructions are a whole different problem. I think we
> > > > should leave that for now and focus on CSRs.
> > > >
> > > > A quick look though and I suspect we could do a similar CPU check in
> > > > decode_opc(). Dealing with the decodetree will be problematic though.
> > > >
> > > > > I'm preparing the patch to demonstrate such hiccups.
> > > > >
> > > > > As far as I know, there's no control flow logic for decodetree to
> parse
> > > > > decodetree files differently. (e.g. ifdef XLEN == 32 then blah,
> blah).
> > > > >
> > > > > To meet in the halfway, maybe after the grand unification on
> RV32/64, we can
> > > > > still confine vendor custom logic (i.e. instrucions and CSRs) to
> be toggled by
> > > > > whether a certain vendor cpu model is selected ?
> > > >wtih  the d
> > > > I honestly don't see a scenario where that happens. The maintenance
> > > > overhead and confusion of changing the CPUs at build time is too
> high.
> > > >
> > > > I also don't think we should need that for CSR accesses. Custom
> > > > instructions are a whole different can of worms.
> > >
> > > IMHO, custom CSR and custom instructions are two sides of a same coin
> in some
> > > way. Let me explain it with an example - -
> > >
> > > Andes has a custom instruction called `EXEC.IT`, which is a 16-bit
> long com-
> > > pressed instruction. By executing such instrcution, an instruction
> table
> > > reside in a particular address specified by a custom CSR called uitb
> will be
> > > fetch, decode and execute. By doing so, the code size could be reduced.
> >
> > Hmmm... This is a much more complex use case than I was expecting. I
> > have been thinking more about custom CSRs to set a timer or control
> > the interrupt controller.
> >
> > Something like what you described is going to be a lot more work.
> >
> > In your case though I think we can still focus on the CSR aspect
> > first. Once that is sorted we can then look at the instruction part.
>
> Just like the mentioned question above , I'm wondering if we can assume
> riscv_csrrw() to be a general interface for accessing all the CSRs ?
>
> Cordially yours,
> Ruinland ChuanTzu Tsai
>
> > The main aim should be that all (or almost all) vendor code lives in
> > it's own file.
> >
> > Alistair
> >
> > >
> > > The problem is that we have different address encoding on RV32 and
> RV64.
> > >
> > > And just like you mentioned, in our in-house core, we apply the same
> logic on
> > > decode_opc() to decode custom instructions first. If such
> decoding/trans
> > > procedure fails, the original decoder will be invoked.
> > >
> > > >
> > > > >
> > > > > By the way, I'm wondering how our friends from T-Head (Guo Ren ?)
> regard this
> > > > > issue ? AFAIK, they forked QEMU from v3.2.0 and applied their
> vendor features
> > > > > on top of it for quite a while.
> > > >
> > > > I'm not sure.
> > >
> > > Sorry for the confusion, I was trying to ping Guo Ren :-D
> > > I CC'ed him in the previous e-mail.
> > >
> > > Cordially yours,
> > > Ruinland ChuanTzu Tsai
> > >
> > > >
> > > > Alistair
> > > >
> > > > >
> > > > > Cordially yours,
> > > > > Ruinland ChuanTzu Tsai
> > > > >
> > > > > On Thu, Sep 02, 2021 at 12:25:20PM +1000, Alistair Francis wrote:
> > > > > > On Fri, Aug 27, 2021 at 1:16 AM Ruinland Chuan-Tzu Tsai
> > > > > > <ruinland@andestech.com> wrote:
> > > > > > >
> > > > > > > From: Ruinland ChuanTzu Tsai <ruinland@andestech.com>
> > > > > > >
> > > > > > > During my modification on my previous patch series for custom
> CSR support, I
> > > > > > > believe this issue deserves its own discussion (or debate)
> because it's _not_
> > > > > > > as simple as "just put those options in Kconfig".
> > > > > > >
> > > > > > > The obstables I've encountered and the kluges I came up is
> listed as follow :
> > > > > > >
> > > > > > > (1) Due to the design of top-level meson.build, all Kconfig
> options will land
> > > > > > > into `*-config-devices.h` since minikconf will be only used
> after config_target
> > > > > > > being processed. This will let to the fact that linux-users
> won't be able to
> > > > > > > use custom CSR code properly becuase they only includes
> `*-config-devices.h`.
> > > > > > > And that is reasonble due to the fact that changes on cpu.c
> and csr.c is a
> > > > > > > target-related matter and linux-user mode shouldn't include
> device related
> > > > > > > headers in most of cases.
> > > > > > >
> > > > > > > So, modify meson.build to parse target/riscv/Kconfig during
> config_target phase
> > > > > > > is without doubts necessary.
> > > > > > >
> > > > > > > (2) Kconfig option `RISCV_CUSTOM_CSR` is introduced for RISC-V
> cpu models to
> > > > > > > toggle it at its will. Yet due to the fact that csr.o and
> cpu.o are linked
> > > > > > > altogether for all CPU models, the suffer will be shared
> without option.
> > > > > > > The only reasonable way to seperate build the fire lane which
> seperates vendor
> > > > > > > flavored cpu and spec-conformed ones, is to build them
> seperately with options
> > > > > > > toggled diffrently, just like RV32 and RV64 shares almost the
> same source base,
> > > > > > > yet the sources are compiled with differnt flags/definitions.
> > > > > > >
> > > > > > > To achieve that, miraculously, we can just put *.mak files
> into `target`
> > > > > > > directoy, because that's how `configure` enumerates what
> targets are supported.
> > > > > > >
> > > > > > > (3) The longest days are not over yet, if we take a good look
> at how the minikconf
> > > > > > > is invoked during config_devices and in what way *.mak
> presented its options
> > > > > > > inside `default-configs/devices`, we can see that *.mak files
> there is formated
> > > > > > > in `CONFIG_*` style and the minikconf is reading directly
> during config_device
> > > > > > > phase. That's totally different from *.mak files presented in
> > > > > > > `default-configs/targets`. To make the parsing logic
> consistent, I
> > > > > > > introduce a rv_custom directory inside which contains
> minikconf-parsable
> > > > > > > mak files.
> > > > > > >
> > > > > > > With this patches, ones can build a A25/AX25 linux-user
> platform by :
> > > > > > > $ ./configure
> --target-list=riscv64-andes-linux-user,riscv32-andes-linux-user
> > > > > >
> > > > > > Hey! Thanks for the patches
> > > > > >
> > > > > > I'm not convinced that we want this though.
> > > > > >
> > > > > > Right now we are trying to head towards a riscv64-softmmu binary
> being
> > > > > > able to run all RISC-V code. That include 32-bit cpus
> > > > > > (qemu-riscv64-softmmu -cpu r32...) and 64-bit CPUs. We shouldn't
> be
> > > > > > splitting out more targets.
> > > > > >
> > > > > > It also goes against the general idea of RISC-V in that everyone
> has a
> > > > > > standard compliant implementation, they can then add extra
> > > > > > functionality.
> > > > > >
> > > > > > In terms of Kconfig options. It doesn't seem like a bad idea to
> have
> > > > > > an option to fully disable custom CSRs. That way if someone
> really
> > > > > > wants performance and doesn't want custom CSRs they can disable
> the
> > > > > > switch. Otherwise we leave it on and all custom CSRs are
> available in
> > > > > > the build and then controlled by the CPU selection at runtime.
> If that
> > > > > > ends up being too difficult to implement though then we don't
> have to
> > > > > > have it.
> > > > > >
> > > > > > Thanks again for working on this.
> > > > > >
> > > > > > Alistair
> > > > > >
> > > > > >
> > > > > > > $ make
> > > > > > >
> > > > > > > P.S. The pacthes from :
> > > > > > >
> https://lists.gnu.org/archive/html/qemu-devel/2021-08/msg00913.html
> > > > > > > is needed. A clean-up and modified version will be sent out
> soon.
> > > > > > >
> > > > > > > P.P.S.
> > > > > > > I know these parts won't be easy to digest, and the further
> iterations will be
> > > > > > > needed, so I didn't ask my colleagues to sign-off for now.
> > > > > > >
> > > > > > > Cordially yours,
> > > > > > > Ruinland ChuanTzu Tsai
> > > > > > >
> > > > > > > Ruinland ChuanTzu Tsai (2):
> > > > > > >   Adding Kconfig options for custom CSR support and Andes CPU
> model
> > > > > > >   Adding necessary files for Andes platforms, cores to enable
> custom CSR
> > > > > > >     support
> > > > > > >
> > > > > > >  Kconfig                                       |  1 +
> > > > > > >  .../devices/riscv32-andes-softmmu.mak         | 17
> ++++++++++++
> > > > > > >  .../devices/riscv64-andes-softmmu.mak         | 17
> ++++++++++++
> > > > > > >  .../targets/riscv32-andes-linux-user.mak      |  1 +
> > > > > > >  .../targets/riscv32-andes-softmmu.mak         |  1 +
> > > > > > >  .../targets/riscv64-andes-linux-user.mak      |  1 +
> > > > > > >  .../targets/riscv64-andes-softmmu.mak         |  1 +
> > > > > > >  .../targets/rv_custom/no_custom.mak           |  0
> > > > > > >  .../rv_custom/riscv32-andes-linux-user.mak    |  1 +
> > > > > > >  .../rv_custom/riscv32-andes-softmmu.mak       |  1 +
> > > > > > >  .../targets/rv_custom/riscv32-linux-user.mak  |  1 +
> > > > > > >  .../targets/rv_custom/riscv32-softmmu.mak     |  1 +
> > > > > > >  .../rv_custom/riscv64-andes-linux-user.mak    |  1 +
> > > > > > >  .../rv_custom/riscv64-andes-softmmu.mak       |  1 +
> > > > > > >  .../targets/rv_custom/riscv64-linux-user.mak  |  1 +
> > > > > > >  .../targets/rv_custom/riscv64-softmmu.mak     |  1 +
> > > > > > >  meson.build                                   | 26
> +++++++++++++++++++
> > > > > > >  target/riscv/Kconfig                          |  6 +++++
> > > > > > >  18 files changed, 79 insertions(+)
> > > > > > >  create mode 100644
> default-configs/devices/riscv32-andes-softmmu.mak
> > > > > > >  create mode 100644
> default-configs/devices/riscv64-andes-softmmu.mak
> > > > > > >  create mode 120000
> default-configs/targets/riscv32-andes-linux-user.mak
> > > > > > >  create mode 120000
> default-configs/targets/riscv32-andes-softmmu.mak
> > > > > > >  create mode 120000
> default-configs/targets/riscv64-andes-linux-user.mak
> > > > > > >  create mode 120000
> default-configs/targets/riscv64-andes-softmmu.mak
> > > > > > >  create mode 100644
> default-configs/targets/rv_custom/no_custom.mak
> > > > > > >  create mode 100644
> default-configs/targets/rv_custom/riscv32-andes-linux-user.mak
> > > > > > >  create mode 100644
> default-configs/targets/rv_custom/riscv32-andes-softmmu.mak
> > > > > > >  create mode 120000
> default-configs/targets/rv_custom/riscv32-linux-user.mak
> > > > > > >  create mode 120000
> default-configs/targets/rv_custom/riscv32-softmmu.mak
> > > > > > >  create mode 100644
> default-configs/targets/rv_custom/riscv64-andes-linux-user.mak
> > > > > > >  create mode 100644
> default-configs/targets/rv_custom/riscv64-andes-softmmu.mak
> > > > > > >  create mode 120000
> default-configs/targets/rv_custom/riscv64-linux-user.mak
> > > > > > >  create mode 120000
> default-configs/targets/rv_custom/riscv64-softmmu.mak
> > > > > > >  create mode 100644 target/riscv/Kconfig
> > > > > > >
> > > > > > > --
> > > > > > > 2.32.0
> > > > > > >
>
>

On Tue, Sep 7, 2021 at 8:15 PM Rahul Pathak <rpathakmailbox@gmail.com> wrote:
>
> Hi Alistair,
>
> One clarification: The unification of architectures is also going to allow multi-arch CPUs (RV32/RV64) in a single machine instance? Or it's just limited to only one in the runtime.

The first step (which doesn't work yet) is to allow running a 32-bit
CPU on the qemu-system-riscv64 binary.

Next step is mixed XLEN machines. Like the FU540 with 4 64-bit cores
and a 32-bit control core.

Eventually aiming for configurable XLEN changes, like what is possible
with the Hypervisor extension.

We don't have any of them yet, but it's worth keeping in mind.

Alistair

>
> Rahul
>
> On Tue, Sep 7, 2021 at 1:37 PM Ruinland ChuanTzu Tsai <ruinland@andestech.com> wrote:
>>
>> Hi Alistair,
>>
>> Thanks for the comment.
>>
>> On Mon, Sep 06, 2021 at 05:55:25PM +1000, Alistair Francis wrote:
>> > On Mon, Sep 6, 2021 at 5:37 PM Ruinland ChuanTzu Tsai
>> > <ruinland@andestech.com> wrote:
>> > >
>> > > Hi Alistair,
>> > >
>> > > So glad to hear from you.
>> > >
>> > > On Mon, Sep 06, 2021 at 05:05:16PM +1000, Alistair Francis wrote:
>> > > > On Mon, Sep 6, 2021 at 4:49 PM Ruinland ChuanTzu Tsai
>> > > > <ruinland@andestech.com> wrote:
>> > > > >
>> > > > >
>> > > > > Hi Alistair,
>> > > > >
>> > > > > Thanks for the heads up about the upcoming unification of RISC-V 32/64 targets.
>> > > > > Yet I have several concerns and would like to have some brainstorming regarding
>> > > > > such topics - -
>> > > >
>> > > > No worries, I'm happy to discuss.
>> > > >
>> > > > >
>> > > > > That is, could you elaborate more about the "runtime check/switch" which you
>> > > > > mentioned in the previous e-mail :
>> > > > > https://lists.nongnu.org/archive/html/qemu-devel/2021-08/msg02154.html
>> > > > > I'm not quite following the context.
>> > > >
>> > > > Yep, so something along the lines of this in `riscv_csrrw()`
>> > > >
>> > > > if (cpu == "MyCustomCPU") {
>> > > >     my_custom_csr[csrno].read();
>> > > > }
>> > > >
>> > > > So we check if using the CPU then apply extra CSR accesses.
>> > > >
>> > > > > If we don't have a way to toggle which (vendor) cores, which will be used,
>> > > > > during compile time, it means that we have to build all the vendor code and
>> > > > > link them all together; and we might have the chance to encounter collision on
>> > > > > csrno between different vendors.
>> > > >
>> > > > I don't see how they will collide as we will only act on 1, based on
>> > > > the CPU we are using.
>> > >
>> > > AFAIK, we need to put CSR number into `target/riscv/cpu_bits.h`, and they are
>> > > exposed to the global and let others to use it. With my current design, which I
>> > > have sent out by RFC patch series v3, I introduced an abstraction layer,
>> > > `custom_cpu_bits.h`, which will toggle diffenet set of custom CSR number.
>> > >
>> > > If we teardown the Kconfig, all symbols will be exposed and then it could have
>> > > a high chance to collide with each other.
>> >
>> > I guess this depends on what you are trying to do.
>> >
>> > We could have non public CSRs. So each CPU could have it's own custom
>> > version of `riscv_csr_operations csr_ops[CSR_TABLE_SIZE]` which is in
>> > it's own C file. We then just add a switch case to CSR accesses and if
>> > using CPU "customcpu" then we check the `custom_cpu_csr_ops` table.
>> > NOTE: That we can do something smarter than a switch, but you get the
>> > point. We can implement a read/write function for every element in the
>> > array, with the default just triggering an illegal instruction.
>>
>> One thing I would like to discuss here.
>>
>> Firstly, I'm not quite sure how the picture of non-public CSR looks like.
>> Is it suggested that non-standard CSR number shall not be exposed ?
>>
>> I know that we should focus on custom CSR part, yet I need to make sure
>> that if following logic is permitted to appear in `target/riscv/trans_insn` :
>>
>> trans_vendor_A_insn_blah(...) {
>>     riscv_csrrw(env, CSR_VENDOR_A_CUSTOM, r, n, write_mask);
>>     }
>>
>> As far as I know, csr number is not presented as a C++ enum, which we
>> can access via csr::custom::vendora::foobar, so it's a globally exposed
>> macro with possiblilty of collision.
>>
>> IMHO, the key is that are we permitted to have a uniform interface to access
>> CSR, either standard or vendor designed ones, in other parts of QEMU.
>>
>>
>> > I guess that assumes that each CSR access is self contained. For
>> > example if changing a custom CSR changes a core part of the
>> > target/riscv code this won't really work.
>> >
>> > On the other hand I'm not convinced we want vendor changes to affect
>> > the core target/riscv code. Ideally all vendor code can be kept in
>> > it's own file and it's fully self contained. That won't work for
>> > everything, but it should work for enough use cases. We can even have
>> > a custom vendor state that the vendor code can use (it can also change
>> > the CPU state).
>> >
>> > Does that make sense?
>>
>> In general, I agree with the point that vendor code should be self-contained.
>> Yet I have doubts that with the current design of CPU model, are we able to
>> unify the targets and in the meanwhile to keep things tight and neat ?
>>
>> The execution flow will be bonded to have a shared instruction decoder/
>> translator and a shared handler for CSR (i.e. riscv_csrrw). It's not like we
>> get to choose what decoder we want to use or which CSR table we will be
>> using at xxx_cpu_init(). If we choose to use runtime check/diversion of all of
>> these parts, the overhead might be tremendous.
>>
>> Surely we should be focusing on CSR part for now, and just as you said, CSR
>> is not that perforamnce-centric.
>>
>> Yet if we take a look at `configs/targets`, still we're having 6 MIPS32/64
>> linux-user targets, 4 ARM32/64 linux-user targets and 4 PPC32/64 linux-user
>> targets.
>>
>> I guess it will be a very long journey to merge all the variants.
>>
>> >
>> > >
>> > > >
>> > > > >
>> > > > > Secondly, I'm not quite sure about how we're going to merge decode tree files
>> > > > > across RV32 and RV64. Vendor-designed custom instruction would have a different
>> > > > > encoding scheme on bitfields for RV32 and RV64. Currently, we (Andes) are using
>> > > > > different decodetree sources for gen32 and gen64 in `target/riscv/meson.build`.
>> > > >
>> > > > Ok, so custom instructions are a whole different problem. I think we
>> > > > should leave that for now and focus on CSRs.
>> > > >
>> > > > A quick look though and I suspect we could do a similar CPU check in
>> > > > decode_opc(). Dealing with the decodetree will be problematic though.
>> > > >
>> > > > > I'm preparing the patch to demonstrate such hiccups.
>> > > > >
>> > > > > As far as I know, there's no control flow logic for decodetree to parse
>> > > > > decodetree files differently. (e.g. ifdef XLEN == 32 then blah, blah).
>> > > > >
>> > > > > To meet in the halfway, maybe after the grand unification on RV32/64, we can
>> > > > > still confine vendor custom logic (i.e. instrucions and CSRs) to be toggled by
>> > > > > whether a certain vendor cpu model is selected ?
>> > > >wtih  the d
>> > > > I honestly don't see a scenario where that happens. The maintenance
>> > > > overhead and confusion of changing the CPUs at build time is too high.
>> > > >
>> > > > I also don't think we should need that for CSR accesses. Custom
>> > > > instructions are a whole different can of worms.
>> > >
>> > > IMHO, custom CSR and custom instructions are two sides of a same coin in some
>> > > way. Let me explain it with an example - -
>> > >
>> > > Andes has a custom instruction called `EXEC.IT`, which is a 16-bit long com-
>> > > pressed instruction. By executing such instrcution, an instruction table
>> > > reside in a particular address specified by a custom CSR called uitb will be
>> > > fetch, decode and execute. By doing so, the code size could be reduced.
>> >
>> > Hmmm... This is a much more complex use case than I was expecting. I
>> > have been thinking more about custom CSRs to set a timer or control
>> > the interrupt controller.
>> >
>> > Something like what you described is going to be a lot more work.
>> >
>> > In your case though I think we can still focus on the CSR aspect
>> > first. Once that is sorted we can then look at the instruction part.
>>
>> Just like the mentioned question above , I'm wondering if we can assume
>> riscv_csrrw() to be a general interface for accessing all the CSRs ?
>>
>> Cordially yours,
>> Ruinland ChuanTzu Tsai
>>
>> > The main aim should be that all (or almost all) vendor code lives in
>> > it's own file.
>> >
>> > Alistair
>> >
>> > >
>> > > The problem is that we have different address encoding on RV32 and RV64.
>> > >
>> > > And just like you mentioned, in our in-house core, we apply the same logic on
>> > > decode_opc() to decode custom instructions first. If such decoding/trans
>> > > procedure fails, the original decoder will be invoked.
>> > >
>> > > >
>> > > > >
>> > > > > By the way, I'm wondering how our friends from T-Head (Guo Ren ?) regard this
>> > > > > issue ? AFAIK, they forked QEMU from v3.2.0 and applied their vendor features
>> > > > > on top of it for quite a while.
>> > > >
>> > > > I'm not sure.
>> > >
>> > > Sorry for the confusion, I was trying to ping Guo Ren :-D
>> > > I CC'ed him in the previous e-mail.
>> > >
>> > > Cordially yours,
>> > > Ruinland ChuanTzu Tsai
>> > >
>> > > >
>> > > > Alistair
>> > > >
>> > > > >
>> > > > > Cordially yours,
>> > > > > Ruinland ChuanTzu Tsai
>> > > > >
>> > > > > On Thu, Sep 02, 2021 at 12:25:20PM +1000, Alistair Francis wrote:
>> > > > > > On Fri, Aug 27, 2021 at 1:16 AM Ruinland Chuan-Tzu Tsai
>> > > > > > <ruinland@andestech.com> wrote:
>> > > > > > >
>> > > > > > > From: Ruinland ChuanTzu Tsai <ruinland@andestech.com>
>> > > > > > >
>> > > > > > > During my modification on my previous patch series for custom CSR support, I
>> > > > > > > believe this issue deserves its own discussion (or debate) because it's _not_
>> > > > > > > as simple as "just put those options in Kconfig".
>> > > > > > >
>> > > > > > > The obstables I've encountered and the kluges I came up is listed as follow :
>> > > > > > >
>> > > > > > > (1) Due to the design of top-level meson.build, all Kconfig options will land
>> > > > > > > into `*-config-devices.h` since minikconf will be only used after config_target
>> > > > > > > being processed. This will let to the fact that linux-users won't be able to
>> > > > > > > use custom CSR code properly becuase they only includes `*-config-devices.h`.
>> > > > > > > And that is reasonble due to the fact that changes on cpu.c and csr.c is a
>> > > > > > > target-related matter and linux-user mode shouldn't include device related
>> > > > > > > headers in most of cases.
>> > > > > > >
>> > > > > > > So, modify meson.build to parse target/riscv/Kconfig during config_target phase
>> > > > > > > is without doubts necessary.
>> > > > > > >
>> > > > > > > (2) Kconfig option `RISCV_CUSTOM_CSR` is introduced for RISC-V cpu models to
>> > > > > > > toggle it at its will. Yet due to the fact that csr.o and cpu.o are linked
>> > > > > > > altogether for all CPU models, the suffer will be shared without option.
>> > > > > > > The only reasonable way to seperate build the fire lane which seperates vendor
>> > > > > > > flavored cpu and spec-conformed ones, is to build them seperately with options
>> > > > > > > toggled diffrently, just like RV32 and RV64 shares almost the same source base,
>> > > > > > > yet the sources are compiled with differnt flags/definitions.
>> > > > > > >
>> > > > > > > To achieve that, miraculously, we can just put *.mak files into `target`
>> > > > > > > directoy, because that's how `configure` enumerates what targets are supported.
>> > > > > > >
>> > > > > > > (3) The longest days are not over yet, if we take a good look at how the minikconf
>> > > > > > > is invoked during config_devices and in what way *.mak presented its options
>> > > > > > > inside `default-configs/devices`, we can see that *.mak files there is formated
>> > > > > > > in `CONFIG_*` style and the minikconf is reading directly during config_device
>> > > > > > > phase. That's totally different from *.mak files presented in
>> > > > > > > `default-configs/targets`. To make the parsing logic consistent, I
>> > > > > > > introduce a rv_custom directory inside which contains minikconf-parsable
>> > > > > > > mak files.
>> > > > > > >
>> > > > > > > With this patches, ones can build a A25/AX25 linux-user platform by :
>> > > > > > > $ ./configure --target-list=riscv64-andes-linux-user,riscv32-andes-linux-user
>> > > > > >
>> > > > > > Hey! Thanks for the patches
>> > > > > >
>> > > > > > I'm not convinced that we want this though.
>> > > > > >
>> > > > > > Right now we are trying to head towards a riscv64-softmmu binary being
>> > > > > > able to run all RISC-V code. That include 32-bit cpus
>> > > > > > (qemu-riscv64-softmmu -cpu r32...) and 64-bit CPUs. We shouldn't be
>> > > > > > splitting out more targets.
>> > > > > >
>> > > > > > It also goes against the general idea of RISC-V in that everyone has a
>> > > > > > standard compliant implementation, they can then add extra
>> > > > > > functionality.
>> > > > > >
>> > > > > > In terms of Kconfig options. It doesn't seem like a bad idea to have
>> > > > > > an option to fully disable custom CSRs. That way if someone really
>> > > > > > wants performance and doesn't want custom CSRs they can disable the
>> > > > > > switch. Otherwise we leave it on and all custom CSRs are available in
>> > > > > > the build and then controlled by the CPU selection at runtime. If that
>> > > > > > ends up being too difficult to implement though then we don't have to
>> > > > > > have it.
>> > > > > >
>> > > > > > Thanks again for working on this.
>> > > > > >
>> > > > > > Alistair
>> > > > > >
>> > > > > >
>> > > > > > > $ make
>> > > > > > >
>> > > > > > > P.S. The pacthes from :
>> > > > > > > https://lists.gnu.org/archive/html/qemu-devel/2021-08/msg00913.html
>> > > > > > > is needed. A clean-up and modified version will be sent out soon.
>> > > > > > >
>> > > > > > > P.P.S.
>> > > > > > > I know these parts won't be easy to digest, and the further iterations will be
>> > > > > > > needed, so I didn't ask my colleagues to sign-off for now.
>> > > > > > >
>> > > > > > > Cordially yours,
>> > > > > > > Ruinland ChuanTzu Tsai
>> > > > > > >
>> > > > > > > Ruinland ChuanTzu Tsai (2):
>> > > > > > >   Adding Kconfig options for custom CSR support and Andes CPU model
>> > > > > > >   Adding necessary files for Andes platforms, cores to enable custom CSR
>> > > > > > >     support
>> > > > > > >
>> > > > > > >  Kconfig                                       |  1 +
>> > > > > > >  .../devices/riscv32-andes-softmmu.mak         | 17 ++++++++++++
>> > > > > > >  .../devices/riscv64-andes-softmmu.mak         | 17 ++++++++++++
>> > > > > > >  .../targets/riscv32-andes-linux-user.mak      |  1 +
>> > > > > > >  .../targets/riscv32-andes-softmmu.mak         |  1 +
>> > > > > > >  .../targets/riscv64-andes-linux-user.mak      |  1 +
>> > > > > > >  .../targets/riscv64-andes-softmmu.mak         |  1 +
>> > > > > > >  .../targets/rv_custom/no_custom.mak           |  0
>> > > > > > >  .../rv_custom/riscv32-andes-linux-user.mak    |  1 +
>> > > > > > >  .../rv_custom/riscv32-andes-softmmu.mak       |  1 +
>> > > > > > >  .../targets/rv_custom/riscv32-linux-user.mak  |  1 +
>> > > > > > >  .../targets/rv_custom/riscv32-softmmu.mak     |  1 +
>> > > > > > >  .../rv_custom/riscv64-andes-linux-user.mak    |  1 +
>> > > > > > >  .../rv_custom/riscv64-andes-softmmu.mak       |  1 +
>> > > > > > >  .../targets/rv_custom/riscv64-linux-user.mak  |  1 +
>> > > > > > >  .../targets/rv_custom/riscv64-softmmu.mak     |  1 +
>> > > > > > >  meson.build                                   | 26 +++++++++++++++++++
>> > > > > > >  target/riscv/Kconfig                          |  6 +++++
>> > > > > > >  18 files changed, 79 insertions(+)
>> > > > > > >  create mode 100644 default-configs/devices/riscv32-andes-softmmu.mak
>> > > > > > >  create mode 100644 default-configs/devices/riscv64-andes-softmmu.mak
>> > > > > > >  create mode 120000 default-configs/targets/riscv32-andes-linux-user.mak
>> > > > > > >  create mode 120000 default-configs/targets/riscv32-andes-softmmu.mak
>> > > > > > >  create mode 120000 default-configs/targets/riscv64-andes-linux-user.mak
>> > > > > > >  create mode 120000 default-configs/targets/riscv64-andes-softmmu.mak
>> > > > > > >  create mode 100644 default-configs/targets/rv_custom/no_custom.mak
>> > > > > > >  create mode 100644 default-configs/targets/rv_custom/riscv32-andes-linux-user.mak
>> > > > > > >  create mode 100644 default-configs/targets/rv_custom/riscv32-andes-softmmu.mak
>> > > > > > >  create mode 120000 default-configs/targets/rv_custom/riscv32-linux-user.mak
>> > > > > > >  create mode 120000 default-configs/targets/rv_custom/riscv32-softmmu.mak
>> > > > > > >  create mode 100644 default-configs/targets/rv_custom/riscv64-andes-linux-user.mak
>> > > > > > >  create mode 100644 default-configs/targets/rv_custom/riscv64-andes-softmmu.mak
>> > > > > > >  create mode 120000 default-configs/targets/rv_custom/riscv64-linux-user.mak
>> > > > > > >  create mode 120000 default-configs/targets/rv_custom/riscv64-softmmu.mak
>> > > > > > >  create mode 100644 target/riscv/Kconfig
>> > > > > > >
>> > > > > > > --
>> > > > > > > 2.32.0
>> > > > > > >
>>

On Tue, Sep 7, 2021 at 6:05 PM Ruinland ChuanTzu Tsai
<ruinland@andestech.com> wrote:
>
> Hi Alistair,
>
> Thanks for the comment.
>
> On Mon, Sep 06, 2021 at 05:55:25PM +1000, Alistair Francis wrote:
> > On Mon, Sep 6, 2021 at 5:37 PM Ruinland ChuanTzu Tsai
> > <ruinland@andestech.com> wrote:
> > >
> > > Hi Alistair,
> > >
> > > So glad to hear from you.
> > >
> > > On Mon, Sep 06, 2021 at 05:05:16PM +1000, Alistair Francis wrote:
> > > > On Mon, Sep 6, 2021 at 4:49 PM Ruinland ChuanTzu Tsai
> > > > <ruinland@andestech.com> wrote:
> > > > >
> > > > >
> > > > > Hi Alistair,
> > > > >
> > > > > Thanks for the heads up about the upcoming unification of RISC-V 32/64 targets.
> > > > > Yet I have several concerns and would like to have some brainstorming regarding
> > > > > such topics - -
> > > >
> > > > No worries, I'm happy to discuss.
> > > >
> > > > >
> > > > > That is, could you elaborate more about the "runtime check/switch" which you
> > > > > mentioned in the previous e-mail :
> > > > > https://lists.nongnu.org/archive/html/qemu-devel/2021-08/msg02154.html
> > > > > I'm not quite following the context.
> > > >
> > > > Yep, so something along the lines of this in `riscv_csrrw()`
> > > >
> > > > if (cpu == "MyCustomCPU") {
> > > >     my_custom_csr[csrno].read();
> > > > }
> > > >
> > > > So we check if using the CPU then apply extra CSR accesses.
> > > >
> > > > > If we don't have a way to toggle which (vendor) cores, which will be used,
> > > > > during compile time, it means that we have to build all the vendor code and
> > > > > link them all together; and we might have the chance to encounter collision on
> > > > > csrno between different vendors.
> > > >
> > > > I don't see how they will collide as we will only act on 1, based on
> > > > the CPU we are using.
> > >
> > > AFAIK, we need to put CSR number into `target/riscv/cpu_bits.h`, and they are
> > > exposed to the global and let others to use it. With my current design, which I
> > > have sent out by RFC patch series v3, I introduced an abstraction layer,
> > > `custom_cpu_bits.h`, which will toggle diffenet set of custom CSR number.
> > >
> > > If we teardown the Kconfig, all symbols will be exposed and then it could have
> > > a high chance to collide with each other.
> >
> > I guess this depends on what you are trying to do.
> >
> > We could have non public CSRs. So each CPU could have it's own custom
> > version of `riscv_csr_operations csr_ops[CSR_TABLE_SIZE]` which is in
> > it's own C file. We then just add a switch case to CSR accesses and if
> > using CPU "customcpu" then we check the `custom_cpu_csr_ops` table.
> > NOTE: That we can do something smarter than a switch, but you get the
> > point. We can implement a read/write function for every element in the
> > array, with the default just triggering an illegal instruction.
>
> One thing I would like to discuss here.
>
> Firstly, I'm not quite sure how the picture of non-public CSR looks like.
> Is it suggested that non-standard CSR number shall not be exposed ?
>
> I know that we should focus on custom CSR part, yet I need to make sure
> that if following logic is permitted to appear in `target/riscv/trans_insn` :
>
> trans_vendor_A_insn_blah(...) {
>     riscv_csrrw(env, CSR_VENDOR_A_CUSTOM, r, n, write_mask);
>     }
>
> As far as I know, csr number is not presented as a C++ enum, which we
> can access via csr::custom::vendora::foobar, so it's a globally exposed
> macro with possiblilty of collision.
>
> IMHO, the key is that are we permitted to have a uniform interface to access
> CSR, either standard or vendor designed ones, in other parts of QEMU.

I think we could still do that.

As an example the WDC Swerv EL2 core
(https://github.com/chipsalliance/Cores-SweRV-EL2/blob/master/docs/RISC-V_SweRV_EL2_PRM.pdf)
has a custom power management CSR called mpmc. The code below is
completely untested (it won't compile), but wouldn't something like
this do what you want?

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index bf1c899c00..bf0c0b733c 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -509,4 +509,6 @@ void riscv_set_csr_ops(int csrno,
riscv_csr_operations *ops);

 void riscv_cpu_register_gdb_regs_for_features(CPUState *cs);

+void wdc_swerv_el2_cpu_init(Object *obj);
+
 #endif /* RISCV_CPU_H */

We expose a public init function for the vendor CPUS

diff --git a/target/riscv/vendor/swerv_el2.h b/target/riscv/vendor/swerv_el2.h
new file mode 100644
index 0000000000..ab392ed757
--- /dev/null
+++ b/target/riscv/vendor/swerv_el2.h
@@ -0,0 +1 @@
+#define CSR_MPMC        0x7C6

The CSRs are in their own header file.


diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 096e3003aa..fd6efee239 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -789,6 +789,7 @@ static const TypeInfo riscv_cpu_type_infos[] = {
     DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_E31,       rv32_sifive_e_cpu_init),
     DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_E34,       rv32_imafcu_nommu_cpu_init),
     DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_U34,       rv32_sifive_u_cpu_init),
+    DEFINE_CPU("WDC-SweRV-El2",                 wdc_swerv_el2_cpu_init),
 #elif defined(TARGET_RISCV64)
     DEFINE_CPU(TYPE_RISCV_CPU_BASE64,           rv64_base_cpu_init),
     DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_E51,       rv64_sifive_e_cpu_init),

We expose the CPU

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index ba9818f6a5..d78234842d 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1440,6 +1440,11 @@ RISCVException riscv_csrrw(CPURISCVState *env, int csrno,
         return RISCV_EXCP_ILLEGAL_INST;
     }
 #endif
+    /* TODO: Don't hard code name check */
+    if (cpu->custom_csrs) {
+        cpu->custom_csrs[csrno].op(env, csrno, ret_value, new_value,
write_mask);
+    }
+
     if (write_mask && read_only) {
         return RISCV_EXCP_ILLEGAL_INST;
     }

We have a change in the CSR handler to call custom CSRs if they exist

diff --git a/target/riscv/vendor/swerv_el2.c b/target/riscv/vendor/swerv_el2.c
new file mode 100644
index 0000000000..5d1cb3678e
--- /dev/null
+++ b/target/riscv/vendor/swerv_el2.c
@@ -0,0 +1,30 @@
+static RISCVException read_mpmc(CPURISCVState *env, int csrno,
+                                target_ulong *val)
+{
+    *val = env->mpmc;
+
+    return RISCV_EXCP_NONE;
+}
+
+static RISCVException write_mpmc(CPURISCVState *env, int csrno,
+                                 target_ulong val)
+{
+    env->mpmc = val;
+
+    return RISCV_EXCP_NONE;
+}
+
+/* Control and Status Register function table */
+riscv_csr_operations swerv_el2_ops[CSR_TABLE_SIZE] = {
+    [CSR_MPMC]     = { "swerv-el2-mpmc",   any,     read_mpmc,  write_mpmc },
+}
+
+void wdc_swerv_el2_cpu_init(Object *obj)
+{
+    CPURISCVState *env = &RISCV_CPU(obj)->env;
+    RISCVCPU *cpu = RISCV_CPU(env);
+
+    cpu->custom_csrs = swerv_el2_ops;
+    /* TODO: Set MISA */
+    set_priv_version(env, PRIV_VERSION_1_10_0);
+}

All of the CPU logic is hidden away in it's own file.

diff --git a/target/riscv/insn_trans/trans_wdc_swerv_el2.c.inc
b/target/riscv/insn_trans/trans_wdc_swerv_el2.c.inc
new file mode 100644
index 0000000000..fba11bee13
--- /dev/null
+++ b/target/riscv/insn_trans/trans_wdc_swerv_el2.c.inc
@@ -0,0 +1,3 @@
+trans_wdc_special_ins(...) {
+    riscv_csrrw(env, CSR_MPMC, r, n, write_mask);
+}

We can also add instructions in their own file. The higher level
decode tree will select these if required as Richard mentioned.

We do have to be careful with names, but I think we can just prefix
functions with the CPU name and we should be ok.

>
>
> > I guess that assumes that each CSR access is self contained. For
> > example if changing a custom CSR changes a core part of the
> > target/riscv code this won't really work.
> >
> > On the other hand I'm not convinced we want vendor changes to affect
> > the core target/riscv code. Ideally all vendor code can be kept in
> > it's own file and it's fully self contained. That won't work for
> > everything, but it should work for enough use cases. We can even have
> > a custom vendor state that the vendor code can use (it can also change
> > the CPU state).
> >
> > Does that make sense?
>
> In general, I agree with the point that vendor code should be self-contained.
> Yet I have doubts that with the current design of CPU model, are we able to
> unify the targets and in the meanwhile to keep things tight and neat ?
>
> The execution flow will be bonded to have a shared instruction decoder/
> translator and a shared handler for CSR (i.e. riscv_csrrw). It's not like we
> get to choose what decoder we want to use or which CSR table we will be
> using at xxx_cpu_init(). If we choose to use runtime check/diversion of all of
> these parts, the overhead might be tremendous.

I don't see why it should be that bad.

We can then check to see if any custom CSR table or instruction
decoder exists. If there isn't we just wasted a few instructions to
perform the check, but if there is we can then run the custom logic.

>
> Surely we should be focusing on CSR part for now, and just as you said, CSR
> is not that perforamnce-centric.
>
> Yet if we take a look at `configs/targets`, still we're having 6 MIPS32/64
> linux-user targets, 4 ARM32/64 linux-user targets and 4 PPC32/64 linux-user
> targets.

RISC-V also has 4, but we shouldn't be adding any new ones.

>
> I guess it will be a very long journey to merge all the variants.
>
> >
> > >
> > > >
> > > > >
> > > > > Secondly, I'm not quite sure about how we're going to merge decode tree files
> > > > > across RV32 and RV64. Vendor-designed custom instruction would have a different
> > > > > encoding scheme on bitfields for RV32 and RV64. Currently, we (Andes) are using
> > > > > different decodetree sources for gen32 and gen64 in `target/riscv/meson.build`.
> > > >
> > > > Ok, so custom instructions are a whole different problem. I think we
> > > > should leave that for now and focus on CSRs.
> > > >
> > > > A quick look though and I suspect we could do a similar CPU check in
> > > > decode_opc(). Dealing with the decodetree will be problematic though.
> > > >
> > > > > I'm preparing the patch to demonstrate such hiccups.
> > > > >
> > > > > As far as I know, there's no control flow logic for decodetree to parse
> > > > > decodetree files differently. (e.g. ifdef XLEN == 32 then blah, blah).
> > > > >
> > > > > To meet in the halfway, maybe after the grand unification on RV32/64, we can
> > > > > still confine vendor custom logic (i.e. instrucions and CSRs) to be toggled by
> > > > > whether a certain vendor cpu model is selected ?
> > > >wtih  the d
> > > > I honestly don't see a scenario where that happens. The maintenance
> > > > overhead and confusion of changing the CPUs at build time is too high.
> > > >
> > > > I also don't think we should need that for CSR accesses. Custom
> > > > instructions are a whole different can of worms.
> > >
> > > IMHO, custom CSR and custom instructions are two sides of a same coin in some
> > > way. Let me explain it with an example - -
> > >
> > > Andes has a custom instruction called `EXEC.IT`, which is a 16-bit long com-
> > > pressed instruction. By executing such instrcution, an instruction table
> > > reside in a particular address specified by a custom CSR called uitb will be
> > > fetch, decode and execute. By doing so, the code size could be reduced.
> >
> > Hmmm... This is a much more complex use case than I was expecting. I
> > have been thinking more about custom CSRs to set a timer or control
> > the interrupt controller.
> >
> > Something like what you described is going to be a lot more work.
> >
> > In your case though I think we can still focus on the CSR aspect
> > first. Once that is sorted we can then look at the instruction part.
>
> Just like the mentioned question above , I'm wondering if we can assume
> riscv_csrrw() to be a general interface for accessing all the CSRs ?

It will be for guest accesses.

Things become tricky when you have knock on effects though, where a
custom CSR can affect the standard running of the machine. In that
case maybe we can override the generic instruction or set some state
in the CPU to have the desired outcome.

Alistair

On 9/6/21 9:05 AM, Alistair Francis wrote:
> I honestly don't see a scenario where that happens. The maintenance
> overhead and confusion of changing the CPUs at build time is too high.

Yes indeed.  One qemu image should support all cpu variations at once.

> I also don't think we should need that for CSR accesses. Custom
> instructions are a whole different can of worms.

Custom instruction sets are manageable.

In general, we use separate decodetree instances, and select on them at a high level of 
translation.  We have examples of this in both target/arm/ and target/mips/ dealing with 
different ISAs.

Prod me when you get to that point.


r~