Hello Alistair,
Le 31/08/2021 à 05:13, Alistair Francis a écrit :
> On Tue, Aug 31, 2021 at 5:26 AM Frédéric Pétrot
> <frederic.pet...@univ-grenoble-alpes.fr> wrote:
>>
>> Starting 128-bit extension support implies a few modifications in the
>> existing sources because checking for 32-bit is done by checking that
>> it is not 64-bit and vice-versa.
>> We now consider the 3 possible xlen values so as to allow correct
>> compilation for both existing targets while setting the compilation
>> framework so that it can also handle the riscv128-softmmu target.
>> This includes gdb configuration files, that are just the bare copy of the
>> 64-bit ones as gdb does not honor, yet, 128-bit CPUs.
>> To consider the 3 xlen values, we had to add a misah field, representing the
>> upper 64 bits of the misa register.
>>
>> Signed-off-by: Frédéric Pétrot <frederic.pet...@univ-grenoble-alpes.fr>
>> Co-authored-by: Fabien Portas <fabien.por...@grenoble-inp.org>
>> ---
>> configs/devices/riscv128-softmmu/default.mak | 16 ++++++
>> configs/targets/riscv128-softmmu.mak | 5 ++
>> gdb-xml/riscv-128bit-cpu.xml | 48 ++++++++++++++++++
>> gdb-xml/riscv-128bit-virtual.xml | 12 +++++
>> include/hw/riscv/sifive_cpu.h | 4 ++
>> target/riscv/Kconfig | 3 ++
>> target/riscv/arch_dump.c | 3 +-
>> target/riscv/cpu-param.h | 3 +-
>> target/riscv/cpu.c | 51 +++++++++++++++++---
>> target/riscv/cpu.h | 19 ++++++++
>> target/riscv/gdbstub.c | 3 ++
>> target/riscv/insn_trans/trans_rvd.c.inc | 10 ++--
>> target/riscv/insn_trans/trans_rvf.c.inc | 2 +-
>> target/riscv/translate.c | 45 ++++++++++++++++-
>> 14 files changed, 209 insertions(+), 15 deletions(-)
>> create mode 100644 configs/devices/riscv128-softmmu/default.mak
>> create mode 100644 configs/targets/riscv128-softmmu.mak
>> create mode 100644 gdb-xml/riscv-128bit-cpu.xml
>> create mode 100644 gdb-xml/riscv-128bit-virtual.xml
>
> Hey!
>
> Thanks for the patches!
>
> Overall this patch looks good.
Thanks for cheering!
> It would greatly help reviewing and the speed in which this can be
> merged if you can split it up more. A lot of these changes probably
> can be separate patches (for example a patch to add misah). I know it
> can sometimes seem a little silly, but it greatly helps with reviewing
> when patches are small and self contained.
Ok, got it.
>>
>> diff --git a/configs/devices/riscv128-softmmu/default.mak
>> b/configs/devices/riscv128-softmmu/default.mak
>> new file mode 100644
>> index 0000000000..31439dbcfe
>> --- /dev/null
>> +++ b/configs/devices/riscv128-softmmu/default.mak
>> @@ -0,0 +1,16 @@
>> +# Default configuration for riscv128-softmmu
>> +
>> +# Uncomment the following lines to disable these optional devices:
>> +#
>> +#CONFIG_PCI_DEVICES=n
>> +CONFIG_SEMIHOSTING=y
>> +CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y
>> +
>> +# Boards:
>> +#
>> +CONFIG_SPIKE=n
>> +CONFIG_SIFIVE_E=n
>> +CONFIG_SIFIVE_U=n
>> +CONFIG_RISCV_VIRT=y
>> +CONFIG_MICROCHIP_PFSOC=n
>> +CONFIG_SHAKTI_C=n
>> diff --git a/configs/targets/riscv128-softmmu.mak
>> b/configs/targets/riscv128-softmmu.mak
>> new file mode 100644
>> index 0000000000..e300c43c8e
>> --- /dev/null
>> +++ b/configs/targets/riscv128-softmmu.mak
>> @@ -0,0 +1,5 @@
>> +TARGET_ARCH=riscv128
>> +TARGET_BASE_ARCH=riscv
>> +TARGET_SUPPORTS_MTTCG=y
>> +TARGET_XML_FILES= gdb-xml/riscv-128bit-cpu.xml gdb-xml/riscv-32bit-fpu.xml
>> gdb-xml/riscv-64bit-fpu.xml gdb-xml/riscv-128bit-virtual.xml
>> +TARGET_NEED_FDT=y
>> diff --git a/gdb-xml/riscv-128bit-cpu.xml b/gdb-xml/riscv-128bit-cpu.xml
>> new file mode 100644
>> index 0000000000..c98168148f
>> --- /dev/null
>> +++ b/gdb-xml/riscv-128bit-cpu.xml
>> @@ -0,0 +1,48 @@
>> +<?xml version="1.0"?>
>> +<!-- Copyright (C) 2018-2019 Free Software Foundation, Inc.
>> +
>> + Copying and distribution of this file, with or without modification,
>> + are permitted in any medium without royalty provided the copyright
>> + notice and this notice are preserved. -->
>> +
>> +<!-- Register numbers are hard-coded in order to maintain backward
>> + compatibility with older versions of tools that didn't use xml
>> + register descriptions. -->
>> +
>> +<!DOCTYPE feature SYSTEM "gdb-target.dtd">
>> +<!-- FIXME : All GPRs are marked as 64-bits since gdb doesn't like 128-bit
>> registers for now. -->
>> +<feature name="org.gnu.gdb.riscv.cpu">
>> + <reg name="zero" bitsize="64" type="int" regnum="0"/>
>> + <reg name="ra" bitsize="64" type="code_ptr"/>
>> + <reg name="sp" bitsize="64" type="data_ptr"/>
>> + <reg name="gp" bitsize="64" type="data_ptr"/>
>> + <reg name="tp" bitsize="64" type="data_ptr"/>
>> + <reg name="t0" bitsize="64" type="int"/>
>> + <reg name="t1" bitsize="64" type="int"/>
>> + <reg name="t2" bitsize="64" type="int"/>
>> + <reg name="fp" bitsize="64" type="data_ptr"/>
>> + <reg name="s1" bitsize="64" type="int"/>
>> + <reg name="a0" bitsize="64" type="int"/>
>> + <reg name="a1" bitsize="64" type="int"/>
>> + <reg name="a2" bitsize="64" type="int"/>
>> + <reg name="a3" bitsize="64" type="int"/>
>> + <reg name="a4" bitsize="64" type="int"/>
>> + <reg name="a5" bitsize="64" type="int"/>
>> + <reg name="a6" bitsize="64" type="int"/>
>> + <reg name="a7" bitsize="64" type="int"/>
>> + <reg name="s2" bitsize="64" type="int"/>
>> + <reg name="s3" bitsize="64" type="int"/>
>> + <reg name="s4" bitsize="64" type="int"/>
>> + <reg name="s5" bitsize="64" type="int"/>
>> + <reg name="s6" bitsize="64" type="int"/>
>> + <reg name="s7" bitsize="64" type="int"/>
>> + <reg name="s8" bitsize="64" type="int"/>
>> + <reg name="s9" bitsize="64" type="int"/>
>> + <reg name="s10" bitsize="64" type="int"/>
>> + <reg name="s11" bitsize="64" type="int"/>
>> + <reg name="t3" bitsize="64" type="int"/>
>> + <reg name="t4" bitsize="64" type="int"/>
>> + <reg name="t5" bitsize="64" type="int"/>
>> + <reg name="t6" bitsize="64" type="int"/>
>> + <reg name="pc" bitsize="64" type="code_ptr"/>
>> +</feature>
>> diff --git a/gdb-xml/riscv-128bit-virtual.xml
>> b/gdb-xml/riscv-128bit-virtual.xml
>> new file mode 100644
>> index 0000000000..db9a0ff677
>> --- /dev/null
>> +++ b/gdb-xml/riscv-128bit-virtual.xml
>> @@ -0,0 +1,12 @@
>> +<?xml version="1.0"?>
>> +<!-- Copyright (C) 2018-2019 Free Software Foundation, Inc.
>> +
>> + Copying and distribution of this file, with or without modification,
>> + are permitted in any medium without royalty provided the copyright
>> + notice and this notice are preserved. -->
>> +
>> +<!DOCTYPE feature SYSTEM "gdb-target.dtd">
>> +<!-- FIXME : priv marked as 64-bits since gdb doesn't like 128-bit
>> registers for now. -->
>> +<feature name="org.gnu.gdb.riscv.virtual">
>> + <reg name="priv" bitsize="64"/>
>> +</feature>
>> diff --git a/include/hw/riscv/sifive_cpu.h b/include/hw/riscv/sifive_cpu.h
>> index 136799633a..2fd441664f 100644
>> --- a/include/hw/riscv/sifive_cpu.hthat
>> +++ b/include/hw/riscv/sifive_cpu.h
>> @@ -26,6 +26,10 @@
>> #elif defined(TARGET_RISCV64)
>> #define SIFIVE_E_CPU TYPE_RISCV_CPU_SIFIVE_E51
>> #define SIFIVE_U_CPU TYPE_RISCV_CPU_SIFIVE_U54
>> +#elif defined(TARGET_RISCV128)
>> +/* 128-bit uses 64-bit CPU for now, since no cpu implements RV128 */
>> +#define SIFIVE_E_CPU TYPE_RISCV_CPU_SIFIVE_E51
>> +#define SIFIVE_U_CPU TYPE_RISCV_CPU_SIFIVE_U54
>> #endif
>>
>> #endif /* HW_SIFIVE_CPU_H */
>> diff --git a/target/riscv/Kconfig b/target/riscv/Kconfig
>> index b9e5932f13..f9ea52a59a 100644
>> --- a/target/riscv/Kconfig
>> +++ b/target/riscv/Kconfig
>> @@ -3,3 +3,6 @@ config RISCV32
>>
>> config RISCV64
>> bool
>> +
>> +config RISCV128
>> + bool
>> diff --git a/target/riscv/arch_dump.c b/target/riscv/arch_dump.c
>> index 709f621d82..f756ed2988 100644
>> --- a/target/riscv/arch_dump.c
>> +++ b/target/riscv/arch_dump.c
>> @@ -176,7 +176,8 @@ int cpu_get_dump_info(ArchDumpInfo *info,
>>
>> info->d_machine = EM_RISCV;
>>
>> -#if defined(TARGET_RISCV64)
>> +#if defined(TARGET_RISCV64) || defined(TARGET_RISCV128)
>> + /* FIXME : No 128-bit ELF class exists (for now), use 64-bit one. */
>> info->d_class = ELFCLASS64;
>> #else
>> info->d_class = ELFCLASS32;
>> diff --git a/target/riscv/cpu-param.h b/target/riscv/cpu-param.h
>> index 80eb615f93..e6d0651f60 100644
>> --- a/target/riscv/cpu-param.h
>> +++ b/target/riscv/cpu-param.h
>> @@ -8,7 +8,8 @@
>> #ifndef RISCV_CPU_PARAM_H
>> #define RISCV_CPU_PARAM_H 1
>>
>> -#if defined(TARGET_RISCV64)
>> +/* 64-bit target, since QEMU isn't built to have TARGET_LONG_BITS over 64 */
>> +#if defined(TARGET_RISCV64) || defined(TARGET_RISCV128)
>> # define TARGET_LONG_BITS 64
>> # define TARGET_PHYS_ADDR_SPACE_BITS 56 /* 44-bit PPN */
>> # define TARGET_VIRT_ADDR_SPACE_BITS 48 /* sv48 */
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index 991a6bb760..1f15026e9c 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -110,18 +110,38 @@ const char *riscv_cpu_get_trap_name(target_ulong
>> cause, bool async)
>>
>> bool riscv_cpu_is_32bit(CPURISCVState *env)
>> {
>> - if (env->misa & RV64) {
>> - return false;
>> - }
>> + return (env->misa & MXLEN_MASK) == RV32;
>> +}
>>
>> - return true;
>> +bool riscv_cpu_is_64bit(CPURISCVState *env)
>> +{
>> + return (env->misa & MXLEN_MASK) == RV64;
>> }
>>
>> +#if defined(TARGET_RISCV128)
>
> Don't add any TARGET_* defines.
>
> We are trying to move to a point where the 64-bit RISC-V softmmu can
> run 32-bit CPUs. Ideally we want the same with 128-bit. You don't have
> to get that working, but don't add any compile time conditionals.
>
> That applies to all code, not just this patch. Unless there is already
> a conditional TARGET_* compile please don't add one.
Dully noted,
Frédéric
>
> Alistair
>
--
+---------------------------------------------------------------------------+
| Frédéric Pétrot, Pr. Grenoble INP-Ensimag/TIMA, Ensimag deputy director |
| Mob/Pho: +33 6 74 57 99 65/+33 4 76 57 48 70 Ad augusta per angusta |
| http://tima.univ-grenoble-alpes.fr frederic.pet...@univ-grenoble-alpes.fr |
+---------------------------------------------------------------------------+
