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

[Jerry ZJ <jerry.zhangjian@sifive.com>]

[-- Attachment #1: Type: text/plain, Size: 4050 bytes --]

Use use mode QEMU can reproduce this
Reproduce steps

Compile: riscv64-unknown-elf-clang -march=rv32imac_zicsr_zifencei_zba_zbb ./test.c -o hello_rv32imac

Run: qemu-riscv32 -cpu rv32,g=false,f=false,v=false,d=false,e=false,h=false,c=true,m=true,i=true,a=true,Zicsr=true,Zifencei=true,zmmul=true,zba=true,zbb=true ./hello_rv32imac

Then you will get the following error message:
qemu-riscv32: Zfa extension requires F extension

Since Zfa has just been ratified, I suppose disabling it by default makes sense and will not break anything that exists.

Best Regards,
Jerry ZJ
SiFive Inc. Taiwan
On Nov 7, 2023 at 00:38 +0800, Daniel Henrique Barboza <dbarboza@ventanamicro.com>, wrote:
>
>
> 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),

[-- Attachment #2: Type: text/html, Size: 5051 bytes --]