1. Patchew
  2. QEMU
  3. View series

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

Jerry Zhang Jian posted 1 patch 1 year ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20231106111440.59995-1-jerry.zhangjian@sifive.com
Maintainers: Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <alistair.francis@wdc.com>, Bin Meng <bin.meng@windriver.com>, Weiwei Li <liweiwei@iscas.ac.cn>, Daniel Henrique Barboza <dbarboza@ventanamicro.com>, Liu Zhiwei <zhiwei_liu@linux.alibaba.com>
target/riscv/cpu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH] target/riscv: don't enable Zfa by default
Posted by Jerry Zhang Jian 1 year ago 
- Zfa requires F, we should not assume all CPUs have F extension
  support.

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),
-- 
2.42.0
Re: [PATCH] target/riscv: don't enable Zfa by default
Posted by Daniel Henrique Barboza 1 year ago 

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),
Re: [PATCH] target/riscv: don't enable Zfa by default
Posted by Jerry ZJ 1 year ago 
We do have some cases that failed. SiFive e-series cores (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

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

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),
Re: [PATCH] target/riscv: don't enable Zfa by default
Posted by Daniel Henrique Barboza 1 year ago 

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),
Re: [PATCH] target/riscv: don't enable Zfa by default
Posted by Jerry ZJ 1 year ago 
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),
Re: [PATCH] target/riscv: don't enable Zfa by default
Posted by Daniel Henrique Barboza 1 year ago 

On 11/6/23 23:53, Jerry ZJ wrote:
> 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

I'll definitely send patches to add a 'rv32i' CPU after seeing the amount of
X=false you're having to use here. You'll be doing something like:

qemu-riscv32 -cpu rv32i,c=true,m=true,a=true,Zicsr=true,Zifencei=true,zmmul=true,zba=true,zbb=true ./hello_rv32imac

I.e. you'll only enable what you need. You won't have to disable defaults you
don't want. I can put you in the CC if you want to give it a shot.

> 
> 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.

Please elaborate a bit more in the commit msg (feel free to add the command line you
sent) about how inconvenient it is to deal with having to disable 'zfa' when you want
'F' disabled. I'll ack it.


Thanks,

Daniel

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.