Re: [PATCH v4 2/4] target/riscv: configure and turn on vector extension from command line
On Tue, Feb 18, 2020 at 4:46 PM LIU Zhiwei wrote:
>
> Hi, Alistair
>
> On 2020/2/19 6:34, Alistair Francis wrote:
> > On Mon, Feb 10, 2020 at 12:12 AM LIU Zhiwei wrote:
> >> Vector extension is default on only for "any" cpu. It can be turned
> >> on by command line "-cpu rv64,v=true,vlen=128,elen=64,vext_spec=v0.7.1".
> >>
> >> vlen is the vector register length, default value is 128 bit.
> >> elen is the max operator size in bits, default value is 64 bit.
> >> vext_spec is the vector specification version, default value is v0.7.1.
> >> Thest properties and cpu can be specified with other values.
> >>
> >> Signed-off-by: LIU Zhiwei
> > This looks fine to me. Shouldn't this be the last patch though?
> Yes， it should be the last patch.
> > As in
> > once the vector extension has been added to QEMU you can turn it on
> > from the command line. Right now this turns it on but it isn't
> > implemented.
> Maybe I should just add fields in RISCVCPU structure. And never open the
> vector extension on or add configure properties until the implementation
> is ready.
Yes, I think that is a good idea.
>
> It's still a little awkward as the reviewers will not be able to test
> the patch until the
> last patch.
I understand, but I don't think anyone is going to want to test the
extension half way through it being added to QEMU. This way we can
start to merge patches even without full support as users can't turn
it on.
Alistair
>
> > Alistair
> >
> >> ---
> >> target/riscv/cpu.c | 48 --
> >> target/riscv/cpu.h | 8
> >> 2 files changed, 54 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> >> index 8c86ebc109..95fdb6261e 100644
> >> --- a/target/riscv/cpu.c
> >> +++ b/target/riscv/cpu.c
> >> @@ -98,6 +98,11 @@ static void set_priv_version(CPURISCVState *env, int
> >> priv_ver)
> >> env->priv_ver = priv_ver;
> >> }
> >>
> >> +static void set_vext_version(CPURISCVState *env, int vext_ver)
> >> +{
> >> +env->vext_ver = vext_ver;
> >> +}
> >> +
> >> static void set_feature(CPURISCVState *env, int feature)
> >> {
> >> env->features |= (1ULL << feature);
> >> @@ -113,7 +118,7 @@ static void set_resetvec(CPURISCVState *env, int
> >> resetvec)
> >> static void riscv_any_cpu_init(Object *obj)
> >> {
> >> CPURISCVState *env = &RISCV_CPU(obj)->env;
> >> -set_misa(env, RVXLEN | RVI | RVM | RVA | RVF | RVD | RVC | RVU);
> >> +set_misa(env, RVXLEN | RVI | RVM | RVA | RVF | RVD | RVC | RVU | RVV);
> >> set_priv_version(env, PRIV_VERSION_1_11_0);
> >> set_resetvec(env, DEFAULT_RSTVEC);
> >> }
> >> @@ -320,6 +325,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error
> >> **errp)
> >> CPURISCVState *env = &cpu->env;
> >> RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(dev);
> >> int priv_version = PRIV_VERSION_1_11_0;
> >> +int vext_version = VEXT_VERSION_0_07_1;
> >> target_ulong target_misa = 0;
> >> Error *local_err = NULL;
> >>
> >> @@ -343,8 +349,18 @@ static void riscv_cpu_realize(DeviceState *dev, Error
> >> **errp)
> >> return;
> >> }
> >> }
> >> -
> >> +if (cpu->cfg.vext_spec) {
> >> +if (!g_strcmp0(cpu->cfg.vext_spec, "v0.7.1")) {
> >> +vext_version = VEXT_VERSION_0_07_1;
> >> +} else {
> >> +error_setg(errp,
> >> + "Unsupported vector spec version '%s'",
> >> + cpu->cfg.vext_spec);
> >> +return;
> >> +}
> >> +}
> >> set_priv_version(env, priv_version);
> >> +set_vext_version(env, vext_version);
> >> set_resetvec(env, DEFAULT_RSTVEC);
> >>
> >> if (cpu->cfg.mmu) {
> >> @@ -409,6 +425,30 @@ static void riscv_cpu_realize(DeviceState *dev, Error
> >> **errp)
> >> if (cpu->cfg.ext_u) {
> >> target_misa |= RVU;
> >> }
> >> +if (cpu->cfg.ext_v) {
> >> +target_misa |= RVV;
> >> +if (!is_power_of_2(cpu->cfg.vlen)) {
> >> +error_setg(errp,
> >> + "Vector extension VLEN must be power of 2");
> >> +return;
> >> +}
> >> +if (cpu->cfg.vlen > RV_VLEN_MAX || cpu->cfg.vlen < 128) {
> >> +error_setg(errp,
> >> + "Vector extension implementation only supports
> >> VLEN "
> >> + "in the range [128, %d]", RV_VLEN_MAX);
> >> +return;
> >> +}
> >> +if (!is_power_of_2(cpu->cfg.elen)) {
> >> +error_setg(errp,
> >> + "Vector extension ELEN must be power of 2");
> >> +return;
> >> +}
> >> +if (cpu->cfg.elen > 64) {
> >> +error_setg(errp,
> >> + "Vector extension ELEN must <= 64");
> >> +return;
> >> +}
> >> +
Re: [PATCH v4 2/4] target/riscv: configure and turn on vector extension from command line
Hi, Alistair
On 2020/2/19 6:34, Alistair Francis wrote:
On Mon, Feb 10, 2020 at 12:12 AM LIU Zhiwei wrote:
Vector extension is default on only for "any" cpu. It can be turned
on by command line "-cpu rv64,v=true,vlen=128,elen=64,vext_spec=v0.7.1".
vlen is the vector register length, default value is 128 bit.
elen is the max operator size in bits, default value is 64 bit.
vext_spec is the vector specification version, default value is v0.7.1.
Thest properties and cpu can be specified with other values.
Signed-off-by: LIU Zhiwei
This looks fine to me. Shouldn't this be the last patch though?
Yes， it should be the last patch.
As in
once the vector extension has been added to QEMU you can turn it on
from the command line. Right now this turns it on but it isn't
implemented.
Maybe I should just add fields in RISCVCPU structure. And never open the
vector extension on or add configure properties until the implementation
is ready.
It's still a little awkward as the reviewers will not be able to test
the patch until the
last patch.
Alistair
---
target/riscv/cpu.c | 48 --
target/riscv/cpu.h | 8
2 files changed, 54 insertions(+), 2 deletions(-)
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 8c86ebc109..95fdb6261e 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -98,6 +98,11 @@ static void set_priv_version(CPURISCVState *env, int
priv_ver)
env->priv_ver = priv_ver;
}
+static void set_vext_version(CPURISCVState *env, int vext_ver)
+{
+env->vext_ver = vext_ver;
+}
+
static void set_feature(CPURISCVState *env, int feature)
{
env->features |= (1ULL << feature);
@@ -113,7 +118,7 @@ static void set_resetvec(CPURISCVState *env, int resetvec)
static void riscv_any_cpu_init(Object *obj)
{
CPURISCVState *env = &RISCV_CPU(obj)->env;
-set_misa(env, RVXLEN | RVI | RVM | RVA | RVF | RVD | RVC | RVU);
+set_misa(env, RVXLEN | RVI | RVM | RVA | RVF | RVD | RVC | RVU | RVV);
set_priv_version(env, PRIV_VERSION_1_11_0);
set_resetvec(env, DEFAULT_RSTVEC);
}
@@ -320,6 +325,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error
**errp)
CPURISCVState *env = &cpu->env;
RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(dev);
int priv_version = PRIV_VERSION_1_11_0;
+int vext_version = VEXT_VERSION_0_07_1;
target_ulong target_misa = 0;
Error *local_err = NULL;
@@ -343,8 +349,18 @@ static void riscv_cpu_realize(DeviceState *dev, Error
**errp)
return;
}
}
-
+if (cpu->cfg.vext_spec) {
+if (!g_strcmp0(cpu->cfg.vext_spec, "v0.7.1")) {
+vext_version = VEXT_VERSION_0_07_1;
+} else {
+error_setg(errp,
+ "Unsupported vector spec version '%s'",
+ cpu->cfg.vext_spec);
+return;
+}
+}
set_priv_version(env, priv_version);
+set_vext_version(env, vext_version);
set_resetvec(env, DEFAULT_RSTVEC);
if (cpu->cfg.mmu) {
@@ -409,6 +425,30 @@ static void riscv_cpu_realize(DeviceState *dev, Error
**errp)
if (cpu->cfg.ext_u) {
target_misa |= RVU;
}
+if (cpu->cfg.ext_v) {
+target_misa |= RVV;
+if (!is_power_of_2(cpu->cfg.vlen)) {
+error_setg(errp,
+ "Vector extension VLEN must be power of 2");
+return;
+}
+if (cpu->cfg.vlen > RV_VLEN_MAX || cpu->cfg.vlen < 128) {
+error_setg(errp,
+ "Vector extension implementation only supports VLEN "
+ "in the range [128, %d]", RV_VLEN_MAX);
+return;
+}
+if (!is_power_of_2(cpu->cfg.elen)) {
+error_setg(errp,
+ "Vector extension ELEN must be power of 2");
+return;
+}
+if (cpu->cfg.elen > 64) {
+error_setg(errp,
+ "Vector extension ELEN must <= 64");
+return;
+}
+}
set_misa(env, RVXLEN | target_misa);
}
@@ -444,10 +484,14 @@ static Property riscv_cpu_properties[] = {
DEFINE_PROP_BOOL("c", RISCVCPU, cfg.ext_c, true),
DEFINE_PROP_BOOL("s", RISCVCPU, cfg.ext_s, true),
DEFINE_PROP_BOOL("u", RISCVCPU, cfg.ext_u, true),
+DEFINE_PROP_BOOL("v", RISCVCPU, cfg.ext_v, false),
DEFINE_PROP_BOOL("Counters", RISCVCPU, cfg.ext_counters, true),
DEFINE_PROP_BOOL("Zifencei", RISCVCPU, cfg.ext_ifencei, true),
DEFINE_PROP_BOOL("Zicsr", RISCVCPU, cfg.ext_icsr, true),
DEFINE_PROP_STRING("priv_spec", RISCVCPU, cfg.priv_spec),
+DEFINE_PROP_STRING("vext_spec", RISCVCPU, cfg.vext_spec),
+DEFINE_PROP_UINT16("vlen", RISCVCPU, cfg.vlen, 128),
+DEFINE_PROP_UINT16("elen", RISCVCPU, cfg.elen, 64),
DEFINE_PROP_BOOL("mmu", RISCVCPU, cfg.mmu, true),
Re: [PATCH v4 2/4] target/riscv: configure and turn on vector extension from command line
On Mon, Feb 10, 2020 at 12:12 AM LIU Zhiwei wrote:
>
> Vector extension is default on only for "any" cpu. It can be turned
> on by command line "-cpu rv64,v=true,vlen=128,elen=64,vext_spec=v0.7.1".
>
> vlen is the vector register length, default value is 128 bit.
> elen is the max operator size in bits, default value is 64 bit.
> vext_spec is the vector specification version, default value is v0.7.1.
> Thest properties and cpu can be specified with other values.
>
> Signed-off-by: LIU Zhiwei
This looks fine to me. Shouldn't this be the last patch though? As in
once the vector extension has been added to QEMU you can turn it on
from the command line. Right now this turns it on but it isn't
implemented.
Alistair
> ---
> target/riscv/cpu.c | 48 --
> target/riscv/cpu.h | 8
> 2 files changed, 54 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 8c86ebc109..95fdb6261e 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -98,6 +98,11 @@ static void set_priv_version(CPURISCVState *env, int
> priv_ver)
> env->priv_ver = priv_ver;
> }
>
> +static void set_vext_version(CPURISCVState *env, int vext_ver)
> +{
> +env->vext_ver = vext_ver;
> +}
> +
> static void set_feature(CPURISCVState *env, int feature)
> {
> env->features |= (1ULL << feature);
> @@ -113,7 +118,7 @@ static void set_resetvec(CPURISCVState *env, int resetvec)
> static void riscv_any_cpu_init(Object *obj)
> {
> CPURISCVState *env = &RISCV_CPU(obj)->env;
> -set_misa(env, RVXLEN | RVI | RVM | RVA | RVF | RVD | RVC | RVU);
> +set_misa(env, RVXLEN | RVI | RVM | RVA | RVF | RVD | RVC | RVU | RVV);
> set_priv_version(env, PRIV_VERSION_1_11_0);
> set_resetvec(env, DEFAULT_RSTVEC);
> }
> @@ -320,6 +325,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error
> **errp)
> CPURISCVState *env = &cpu->env;
> RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(dev);
> int priv_version = PRIV_VERSION_1_11_0;
> +int vext_version = VEXT_VERSION_0_07_1;
> target_ulong target_misa = 0;
> Error *local_err = NULL;
>
> @@ -343,8 +349,18 @@ static void riscv_cpu_realize(DeviceState *dev, Error
> **errp)
> return;
> }
> }
> -
> +if (cpu->cfg.vext_spec) {
> +if (!g_strcmp0(cpu->cfg.vext_spec, "v0.7.1")) {
> +vext_version = VEXT_VERSION_0_07_1;
> +} else {
> +error_setg(errp,
> + "Unsupported vector spec version '%s'",
> + cpu->cfg.vext_spec);
> +return;
> +}
> +}
> set_priv_version(env, priv_version);
> +set_vext_version(env, vext_version);
> set_resetvec(env, DEFAULT_RSTVEC);
>
> if (cpu->cfg.mmu) {
> @@ -409,6 +425,30 @@ static void riscv_cpu_realize(DeviceState *dev, Error
> **errp)
> if (cpu->cfg.ext_u) {
> target_misa |= RVU;
> }
> +if (cpu->cfg.ext_v) {
> +target_misa |= RVV;
> +if (!is_power_of_2(cpu->cfg.vlen)) {
> +error_setg(errp,
> + "Vector extension VLEN must be power of 2");
> +return;
> +}
> +if (cpu->cfg.vlen > RV_VLEN_MAX || cpu->cfg.vlen < 128) {
> +error_setg(errp,
> + "Vector extension implementation only supports VLEN "
> + "in the range [128, %d]", RV_VLEN_MAX);
> +return;
> +}
> +if (!is_power_of_2(cpu->cfg.elen)) {
> +error_setg(errp,
> + "Vector extension ELEN must be power of 2");
> +return;
> +}
> +if (cpu->cfg.elen > 64) {
> +error_setg(errp,
> + "Vector extension ELEN must <= 64");
> +return;
> +}
> +}
>
> set_misa(env, RVXLEN | target_misa);
> }
> @@ -444,10 +484,14 @@ static Property riscv_cpu_properties[] = {
> DEFINE_PROP_BOOL("c", RISCVCPU, cfg.ext_c, true),
> DEFINE_PROP_BOOL("s", RISCVCPU, cfg.ext_s, true),
> DEFINE_PROP_BOOL("u", RISCVCPU, cfg.ext_u, true),
> +DEFINE_PROP_BOOL("v", RISCVCPU, cfg.ext_v, false),
> DEFINE_PROP_BOOL("Counters", RISCVCPU, cfg.ext_counters, true),
> DEFINE_PROP_BOOL("Zifencei", RISCVCPU, cfg.ext_ifencei, true),
> DEFINE_PROP_BOOL("Zicsr", RISCVCPU, cfg.ext_icsr, true),
> DEFINE_PROP_STRING("priv_spec", RISCVCPU, cfg.priv_spec),
> +DEFINE_PROP_STRING("vext_spec", RISCVCPU, cfg.vext_spec),
> +DEFINE_PROP_UINT16("vlen", RISCVCPU, cfg.vlen, 128),
> +DEFINE_PROP_UINT16("elen", RISCVCPU, cfg.elen, 64),
> DEFINE_PROP_BOOL("mmu", RISCVCPU, cfg.mmu, true),
> DEFINE_PROP_BOOL("pmp", RISCVCPU, cfg.pmp, true),
> DEFINE_PROP_END_OF_LIST(),
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 07e63016a7..bf2b4b55af 1
Re: [PATCH v4 2/4] target/riscv: configure and turn on vector extension from command line
On 2020/2/11 23:56, Richard Henderson wrote:
On 2/10/20 8:12 AM, LIU Zhiwei wrote:
+if (cpu->cfg.vlen > RV_VLEN_MAX || cpu->cfg.vlen < 128) {
+error_setg(errp,
+ "Vector extension implementation only supports VLEN "
+ "in the range [128, %d]", RV_VLEN_MAX);
+return;
+}
+if (!is_power_of_2(cpu->cfg.elen)) {
+error_setg(errp,
+ "Vector extension ELEN must be power of 2");
+return;
+}
+if (cpu->cfg.elen > 64) {
+error_setg(errp,
+ "Vector extension ELEN must <= 64");
+return;
+}
ELEN should use the same "only supports ELEN in the range" language as VLEN.
OK. I will printf "only supports ELEN in the range[8, 64]".
Otherwise,
Reviewed-by: Richard Henderson
r~
Re: [PATCH v4 2/4] target/riscv: configure and turn on vector extension from command line
On 2/10/20 8:12 AM, LIU Zhiwei wrote:
> +if (cpu->cfg.vlen > RV_VLEN_MAX || cpu->cfg.vlen < 128) {
> +error_setg(errp,
> + "Vector extension implementation only supports VLEN "
> + "in the range [128, %d]", RV_VLEN_MAX);
> +return;
> +}
> +if (!is_power_of_2(cpu->cfg.elen)) {
> +error_setg(errp,
> + "Vector extension ELEN must be power of 2");
> +return;
> +}
> +if (cpu->cfg.elen > 64) {
> +error_setg(errp,
> + "Vector extension ELEN must <= 64");
> +return;
> +}
ELEN should use the same "only supports ELEN in the range" language as VLEN.
Otherwise,
Reviewed-by: Richard Henderson
r~
