Re: [PATCH] target/riscv: don't enable Zfa by default

[Daniel Henrique Barboza]

On 11/6/23 12:21, Jerry ZJ wrote:
> We do have some cases that failed. SiFive e-series cores 
> (https://static.dev.sifive.com/SiFive-E21-Manual-v1p0.pdf 
> <https://static.dev.sifive.com/SiFive-E21-Manual-v1p0.pdf>) do not have F 
> extension (For example: rv32imc_zicsr_zifencei_zba_zbb). When we use the 
> corresponding extension options to configure QEMU, i.e., rv32, i=true, m=true, 
> a=true, c=true, Zicsr=true, Zifencei=true, zba=true, zbb=true, the QEMU will have the 
> following error.
> Zfa extension requires F extension

Can you send your whole command line? I'm unable to reproduce it here. This
will boot:

./build/qemu-system-riscv32 -M virt -cpu 
rv32,i=true,m=true,a=true,c=true,zicsr=true,zifencei=true,zba=true,zbb=true 
--nographic


In a side note, we have a new CPU type (still pending, not yet queue) called
"rv64i", which comes only with 'RVI' enabled and nothing else - no defaults,
nothing.

I believe this use case you testing here would benefit from a "rv32i" CPU that
does the same but for 32 bits. Then you can specify the whole CPU and not worry
about hidden defaults. Does that makes sense?

> IMHO, we should not enable Zfa extension by default, especially when Zfa 
> requires F to be enabled implicitly.

If the rv32 use case you mentioned is really breaking because of zfa and
Fm, I'm fine with disabling zfa because it's now a bug. We just need a
reproducer.


Thanks,

Daniel

> Best Regards,
> Jerry ZJ
> *SiFive Inc. Taiwan*
> On Nov 6, 2023 at 22:55 +0800, Daniel Henrique Barboza 
> <dbarboza@ventanamicro.com>, wrote:
> > On 11/6/23 08:14, Jerry Zhang Jian wrote:
> > > - Zfa requires F, we should not assume all CPUs have F extension
> > > support.
> >
> > We do not have a case where this happen, do we? The default CPUs have F
> > enabled (see misa_ext_cfgs[] in target/riscv/tcg/tcg-cpu.c), so zfa being
> > enable isn't a problem for them. Vendor CPUs might not have F enabled, but
> > they don't use the default values for extensions, so they're not affected.
> > Having zfa enabled by default does not hurt the default CPU setups we have.
> >
> > I am not a fan of these defaults for rv64 and so on, but once we set them to
> > 'true' people can complain if we set them to 'false' because it might break
> > existing configs in the wild. We need a strong case (i.e. a bug) to do so.
> >
> >
> > Thanks,
> >
> > Daniel
> >
> >
> > > Signed-off-by: Jerry Zhang Jian <jerry.zhangjian@sifive.com>
> > > ---
> > > target/riscv/cpu.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > index ac4a6c7eec..c9f11509c8 100644
> > > --- a/target/riscv/cpu.c
> > > +++ b/target/riscv/cpu.c
> > > @@ -1247,7 +1247,7 @@ const RISCVCPUMultiExtConfig riscv_cpu_extensions[] = {
> > > MULTI_EXT_CFG_BOOL("zihintntl", ext_zihintntl, true),
> > > MULTI_EXT_CFG_BOOL("zihintpause", ext_zihintpause, true),
> > > MULTI_EXT_CFG_BOOL("zawrs", ext_zawrs, true),
> > > - MULTI_EXT_CFG_BOOL("zfa", ext_zfa, true),
> > > + MULTI_EXT_CFG_BOOL("zfa", ext_zfa, false),
> > > MULTI_EXT_CFG_BOOL("zfh", ext_zfh, false),
> > > MULTI_EXT_CFG_BOOL("zfhmin", ext_zfhmin, false),
> > > MULTI_EXT_CFG_BOOL("zve32f", ext_zve32f, false),