| Message ID | 20220708073943.54781-2-kito.cheng@sifive.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] target/riscv: Lower bound of VLEN is 32, and check VLEN >= ELEN | expand |
在 2022/7/8 下午3:39, Kito Cheng 写道: > Default ELEN is setting to 64 for now, which is incorrect setting for > Zve32*, and spec has mention minimum VLEN and supported EEW in chapter > "Zve*: Vector Extensions for Embedded Processors" is 32 for Zve32. > > ELEN actaully could be derived from which extensions are enabled, > so this patch set elen to 0 as auto detect, and keep the capability to > let user could configure that. > > Signed-off-by: Kito Cheng <kito.cheng@sifive.com> > --- > target/riscv/cpu.c | 16 +++++++++++++--- > 1 file changed, 13 insertions(+), 3 deletions(-) > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index 487d0faa63..c1b96da7da 100644 > --- a/target/riscv/cpu.c > +++ b/target/riscv/cpu.c > @@ -751,13 +751,22 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp) > "Vector extension ELEN must be power of 2"); > return; > } > - if (cpu->cfg.elen > 64 || cpu->cfg.vlen < 8) { > + if (cpu->cfg.elen == 0) { > + if (cpu->cfg.ext_zve32f) { > + cpu->cfg.elen = 32; > + } > + if (cpu->cfg.ext_zve64f || cpu->cfg.ext_v) { > + cpu->cfg.elen = 64; > + } This code is in the "if(cpu->cfg.ext_v){...}", so "cpu->cfg.ext_zve64f || cpu->cfg.ext_v" will always be true. It can use "else ... " directly. > + } > + if (cpu->cfg.elen != 0 && (cpu->cfg.elen > 64 || > + cpu->cfg.elen < 8)) { > error_setg(errp, > "Vector extension implementation only supports ELEN " > "in the range [8, 64]"); > return; > } > - if (cpu->cfg.vlen < cpu->cfg.elen) { > + if (cpu->cfg.elen != 0 && cpu->cfg.vlen < cpu->cfg.elen) { when cfg.elen is set to zero, it will be changed to the auto detect value(32/64) before this two check. So this two modifications seem unnecessary. Regards, Weiwei Li > error_setg(errp, > "Vector extension VLEN must be greater than or equal " > "to ELEN"); > @@ -901,7 +910,8 @@ static Property riscv_cpu_extensions[] = { > 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), > + /* elen = 0 means set from v or zve* extension */ > + DEFINE_PROP_UINT16("elen", RISCVCPU, cfg.elen, 0), > > DEFINE_PROP_BOOL("svinval", RISCVCPU, cfg.ext_svinval, false), > DEFINE_PROP_BOOL("svnapot", RISCVCPU, cfg.ext_svnapot, false),
在 2022/7/9 下午4:11, Weiwei Li 写道: > 在 2022/7/8 下午3:39, Kito Cheng 写道: >> Default ELEN is setting to 64 for now, which is incorrect setting for >> Zve32*, and spec has mention minimum VLEN and supported EEW in chapter >> "Zve*: Vector Extensions for Embedded Processors" is 32 for Zve32. >> >> ELEN actaully could be derived from which extensions are enabled, >> so this patch set elen to 0 as auto detect, and keep the capability to >> let user could configure that. >> >> Signed-off-by: Kito Cheng <kito.cheng@sifive.com> >> --- >> target/riscv/cpu.c | 16 +++++++++++++--- >> 1 file changed, 13 insertions(+), 3 deletions(-) >> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c >> index 487d0faa63..c1b96da7da 100644 >> --- a/target/riscv/cpu.c >> +++ b/target/riscv/cpu.c >> @@ -751,13 +751,22 @@ static void riscv_cpu_realize(DeviceState *dev, >> Error **errp) >> "Vector extension ELEN must be power of 2"); >> return; >> } >> - if (cpu->cfg.elen > 64 || cpu->cfg.vlen < 8) { >> + if (cpu->cfg.elen == 0) { >> + if (cpu->cfg.ext_zve32f) { >> + cpu->cfg.elen = 32; >> + } >> + if (cpu->cfg.ext_zve64f || cpu->cfg.ext_v) { >> + cpu->cfg.elen = 64; >> + } > > This code is in the "if(cpu->cfg.ext_v){...}", so > "cpu->cfg.ext_zve64f || cpu->cfg.ext_v" will always be true. > > It can use "else ... " directly. Sorry, misunderstood the logic. It seems that we should set cpu->cfg.elen=64 directly here. Regards, Weiwei Li >> + } >> + if (cpu->cfg.elen != 0 && (cpu->cfg.elen > 64 || >> + cpu->cfg.elen < 8)) { >> error_setg(errp, >> "Vector extension implementation only >> supports ELEN " >> "in the range [8, 64]"); >> return; >> } >> - if (cpu->cfg.vlen < cpu->cfg.elen) { >> + if (cpu->cfg.elen != 0 && cpu->cfg.vlen < cpu->cfg.elen) { > > when cfg.elen is set to zero, it will be changed to the auto detect > value(32/64) before this two check. > > So this two modifications seem unnecessary. > > Regards, > > Weiwei Li > >> error_setg(errp, >> "Vector extension VLEN must be greater than >> or equal " >> "to ELEN"); >> @@ -901,7 +910,8 @@ static Property riscv_cpu_extensions[] = { >> 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), >> + /* elen = 0 means set from v or zve* extension */ >> + DEFINE_PROP_UINT16("elen", RISCVCPU, cfg.elen, 0), >> DEFINE_PROP_BOOL("svinval", RISCVCPU, cfg.ext_svinval, false), >> DEFINE_PROP_BOOL("svnapot", RISCVCPU, cfg.ext_svnapot, false), >
On Fri, Jul 8, 2022 at 3:39 PM Kito Cheng <kito.cheng@sifive.com> wrote: > Default ELEN is setting to 64 for now, which is incorrect setting for > Zve32*, and spec has mention minimum VLEN and supported EEW in chapter > "Zve*: Vector Extensions for Embedded Processors" is 32 for Zve32. > > ELEN actaully could be derived from which extensions are enabled, > so this patch set elen to 0 as auto detect, and keep the capability to > let user could configure that. > > Signed-off-by: Kito Cheng <kito.cheng@sifive.com> > --- > target/riscv/cpu.c | 16 +++++++++++++--- > 1 file changed, 13 insertions(+), 3 deletions(-) > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index 487d0faa63..c1b96da7da 100644 > --- a/target/riscv/cpu.c > +++ b/target/riscv/cpu.c > @@ -751,13 +751,22 @@ static void riscv_cpu_realize(DeviceState *dev, > Error **errp) > "Vector extension ELEN must be power of 2"); > return; > } > - if (cpu->cfg.elen > 64 || cpu->cfg.vlen < 8) { > + if (cpu->cfg.elen == 0) { > + if (cpu->cfg.ext_zve32f) { > + cpu->cfg.elen = 32; > + } > + if (cpu->cfg.ext_zve64f || cpu->cfg.ext_v) { > + cpu->cfg.elen = 64; > + } > + } > + if (cpu->cfg.elen != 0 && (cpu->cfg.elen > 64 || > + cpu->cfg.elen < 8)) { > error_setg(errp, > "Vector extension implementation only supports > ELEN " > "in the range [8, 64]"); > return; > } > Should we also check whether cpu->cfg.elen set by user must >= 32 if Zve32f is enabled? Same for Zve64f. Regards, Frank Chang > - if (cpu->cfg.vlen < cpu->cfg.elen) { > + if (cpu->cfg.elen != 0 && cpu->cfg.vlen < cpu->cfg.elen) { > error_setg(errp, > "Vector extension VLEN must be greater than or > equal " > "to ELEN"); > @@ -901,7 +910,8 @@ static Property riscv_cpu_extensions[] = { > 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), > + /* elen = 0 means set from v or zve* extension */ > + DEFINE_PROP_UINT16("elen", RISCVCPU, cfg.elen, 0), > > DEFINE_PROP_BOOL("svinval", RISCVCPU, cfg.ext_svinval, false), > DEFINE_PROP_BOOL("svnapot", RISCVCPU, cfg.ext_svnapot, false), > -- > 2.34.0 > >
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index 487d0faa63..c1b96da7da 100644 --- a/target/riscv/cpu.c +++ b/target/riscv/cpu.c @@ -751,13 +751,22 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp) "Vector extension ELEN must be power of 2"); return; } - if (cpu->cfg.elen > 64 || cpu->cfg.vlen < 8) { + if (cpu->cfg.elen == 0) { + if (cpu->cfg.ext_zve32f) { + cpu->cfg.elen = 32; + } + if (cpu->cfg.ext_zve64f || cpu->cfg.ext_v) { + cpu->cfg.elen = 64; + } + } + if (cpu->cfg.elen != 0 && (cpu->cfg.elen > 64 || + cpu->cfg.elen < 8)) { error_setg(errp, "Vector extension implementation only supports ELEN " "in the range [8, 64]"); return; } - if (cpu->cfg.vlen < cpu->cfg.elen) { + if (cpu->cfg.elen != 0 && cpu->cfg.vlen < cpu->cfg.elen) { error_setg(errp, "Vector extension VLEN must be greater than or equal " "to ELEN"); @@ -901,7 +910,8 @@ static Property riscv_cpu_extensions[] = { 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), + /* elen = 0 means set from v or zve* extension */ + DEFINE_PROP_UINT16("elen", RISCVCPU, cfg.elen, 0), DEFINE_PROP_BOOL("svinval", RISCVCPU, cfg.ext_svinval, false), DEFINE_PROP_BOOL("svnapot", RISCVCPU, cfg.ext_svnapot, false),
Default ELEN is setting to 64 for now, which is incorrect setting for Zve32*, and spec has mention minimum VLEN and supported EEW in chapter "Zve*: Vector Extensions for Embedded Processors" is 32 for Zve32. ELEN actaully could be derived from which extensions are enabled, so this patch set elen to 0 as auto detect, and keep the capability to let user could configure that. Signed-off-by: Kito Cheng <kito.cheng@sifive.com> --- target/riscv/cpu.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)