Re: [PATCH 2/2] target/riscv: Auto set elen from vector extension by default

[Weiwei Li]

在 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),