Re: [PATCH v4 2/4] target/riscv: configure and turn on vector extension from command line

[Alistair Francis <alistair23@gmail.com>]

On Tue, Feb 18, 2020 at 4:46 PM LIU Zhiwei <zhiwei_liu@c-sky.com> wrote:
>
> Hi, Alistair
>
> On 2020/2/19 6:34, Alistair Francis wrote:
> > On Mon, Feb 10, 2020 at 12:12 AM LIU Zhiwei <zhiwei_liu@c-sky.com> 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 <zhiwei_liu@c-sky.com>
> > 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;
> >> +            }
> >> +        }
> >>
> >>           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 100644
> >> --- a/target/riscv/cpu.h
> >> +++ b/target/riscv/cpu.h
> >> @@ -64,6 +64,7 @@
> >>   #define RVA RV('A')
> >>   #define RVF RV('F')
> >>   #define RVD RV('D')
> >> +#define RVV RV('V')
> >>   #define RVC RV('C')
> >>   #define RVS RV('S')
> >>   #define RVU RV('U')
> >> @@ -82,6 +83,8 @@ enum {
> >>   #define PRIV_VERSION_1_10_0 0x00011000
> >>   #define PRIV_VERSION_1_11_0 0x00011100
> >>
> >> +#define VEXT_VERSION_0_07_1 0x00000701
> >> +
> >>   #define TRANSLATE_PMP_FAIL 2
> >>   #define TRANSLATE_FAIL 1
> >>   #define TRANSLATE_SUCCESS 0
> >> @@ -118,6 +121,7 @@ struct CPURISCVState {
> >>       target_ulong badaddr;
> >>
> >>       target_ulong priv_ver;
> >> +    target_ulong vext_ver;
> >>       target_ulong misa;
> >>       target_ulong misa_mask;
> >>
> >> @@ -226,12 +230,16 @@ typedef struct RISCVCPU {
> >>           bool ext_c;
> >>           bool ext_s;
> >>           bool ext_u;
> >> +        bool ext_v;
> >>           bool ext_counters;
> >>           bool ext_ifencei;
> >>           bool ext_icsr;
> >>
> >>           char *priv_spec;
> >>           char *user_spec;
> >> +        char *vext_spec;
> >> +        uint16_t vlen;
> >> +        uint16_t elen;
> >>           bool mmu;
> >>           bool pmp;
> >>       } cfg;
> >> --
> >> 2.23.0
> >>
>