* Re: [PATCH] target/riscv/tcg: remove RVG warning
2023-10-03 12:25 [PATCH] target/riscv/tcg: remove RVG warning Daniel Henrique Barboza
@ 2023-10-04 8:13 ` Andrew Jones
2023-10-09 1:16 ` Alistair Francis
2023-10-09 1:19 ` Alistair Francis
2 siblings, 0 replies; 4+ messages in thread
From: Andrew Jones @ 2023-10-04 8:13 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: qemu-devel, qemu-riscv, alistair.francis, bmeng, liweiwei,
zhiwei_liu, palmer, Paul A . Clarke
On Tue, Oct 03, 2023 at 09:25:39AM -0300, Daniel Henrique Barboza wrote:
> Vendor CPUs that set RVG are displaying user warnings about other
> extensions that RVG must enable, one warning per CPU. E.g.:
>
> $ ./build/qemu-system-riscv64 -smp 8 -M virt -cpu veyron-v1 -nographic
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
>
> This happens because we decided a while ago that, for simplicity, vendor
> CPUs could set RVG instead of setting each G extension individually in
> their cpu_init(). Our warning isn't taking that into account, and we're
> bugging users with a warning that we're causing ourselves.
>
> In a closer look we conclude that this warning is not warranted in any
> other circumstance since we're just following the ISA [1], which states
> in chapter 24:
>
> "One goal of the RISC-V project is that it be used as a stable software
> development target. For this purpose, we define a combination of a base
> ISA (RV32I or RV64I) plus selected standard extensions (IMAFD, Zicsr,
> Zifencei) as a 'general-purpose' ISA, and we use the abbreviation G for
> the IMAFDZicsr Zifencei combination of instruction-set extensions."
>
> With this in mind, enabling IMAFD_Zicsr_Zifencei if the user explicitly
> enables 'G' is an expected behavior and the warning is unneeded. Any
> user caught by surprise should refer to the ISA.
>
> Remove the warning when handling RVG.
>
> [1] https://github.com/riscv/riscv-isa-manual/releases/download/Ratified-IMAFDQC/riscv-spec-20191213.pdf
>
> Reported-by: Paul A. Clarke <pclarke@ventanamicro.com>
> Suggested-by: Andrew Jones <ajones@ventanamicro.com>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
> target/riscv/tcg/tcg-cpu.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 08b806dc07..f50ce57602 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -293,7 +293,6 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
> return;
> }
>
> - warn_report("Setting G will also set IMAFD_Zicsr_Zifencei");
> cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_icsr), true);
> cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_ifencei), true);
>
> --
> 2.41.0
>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] target/riscv/tcg: remove RVG warning
2023-10-03 12:25 [PATCH] target/riscv/tcg: remove RVG warning Daniel Henrique Barboza
2023-10-04 8:13 ` Andrew Jones
@ 2023-10-09 1:16 ` Alistair Francis
2023-10-09 1:19 ` Alistair Francis
2 siblings, 0 replies; 4+ messages in thread
From: Alistair Francis @ 2023-10-09 1:16 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: qemu-devel, qemu-riscv, alistair.francis, bmeng, liweiwei,
zhiwei_liu, palmer, Paul A . Clarke, Andrew Jones
On Tue, Oct 3, 2023 at 10:26 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Vendor CPUs that set RVG are displaying user warnings about other
> extensions that RVG must enable, one warning per CPU. E.g.:
>
> $ ./build/qemu-system-riscv64 -smp 8 -M virt -cpu veyron-v1 -nographic
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
>
> This happens because we decided a while ago that, for simplicity, vendor
> CPUs could set RVG instead of setting each G extension individually in
> their cpu_init(). Our warning isn't taking that into account, and we're
> bugging users with a warning that we're causing ourselves.
>
> In a closer look we conclude that this warning is not warranted in any
> other circumstance since we're just following the ISA [1], which states
> in chapter 24:
>
> "One goal of the RISC-V project is that it be used as a stable software
> development target. For this purpose, we define a combination of a base
> ISA (RV32I or RV64I) plus selected standard extensions (IMAFD, Zicsr,
> Zifencei) as a 'general-purpose' ISA, and we use the abbreviation G for
> the IMAFDZicsr Zifencei combination of instruction-set extensions."
>
> With this in mind, enabling IMAFD_Zicsr_Zifencei if the user explicitly
> enables 'G' is an expected behavior and the warning is unneeded. Any
> user caught by surprise should refer to the ISA.
>
> Remove the warning when handling RVG.
>
> [1] https://github.com/riscv/riscv-isa-manual/releases/download/Ratified-IMAFDQC/riscv-spec-20191213.pdf
>
> Reported-by: Paul A. Clarke <pclarke@ventanamicro.com>
> Suggested-by: Andrew Jones <ajones@ventanamicro.com>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> target/riscv/tcg/tcg-cpu.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 08b806dc07..f50ce57602 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -293,7 +293,6 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
> return;
> }
>
> - warn_report("Setting G will also set IMAFD_Zicsr_Zifencei");
> cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_icsr), true);
> cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_ifencei), true);
>
> --
> 2.41.0
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] target/riscv/tcg: remove RVG warning
2023-10-03 12:25 [PATCH] target/riscv/tcg: remove RVG warning Daniel Henrique Barboza
2023-10-04 8:13 ` Andrew Jones
2023-10-09 1:16 ` Alistair Francis
@ 2023-10-09 1:19 ` Alistair Francis
2 siblings, 0 replies; 4+ messages in thread
From: Alistair Francis @ 2023-10-09 1:19 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: qemu-devel, qemu-riscv, alistair.francis, bmeng, liweiwei,
zhiwei_liu, palmer, Paul A . Clarke, Andrew Jones
On Tue, Oct 3, 2023 at 10:26 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Vendor CPUs that set RVG are displaying user warnings about other
> extensions that RVG must enable, one warning per CPU. E.g.:
>
> $ ./build/qemu-system-riscv64 -smp 8 -M virt -cpu veyron-v1 -nographic
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
>
> This happens because we decided a while ago that, for simplicity, vendor
> CPUs could set RVG instead of setting each G extension individually in
> their cpu_init(). Our warning isn't taking that into account, and we're
> bugging users with a warning that we're causing ourselves.
>
> In a closer look we conclude that this warning is not warranted in any
> other circumstance since we're just following the ISA [1], which states
> in chapter 24:
>
> "One goal of the RISC-V project is that it be used as a stable software
> development target. For this purpose, we define a combination of a base
> ISA (RV32I or RV64I) plus selected standard extensions (IMAFD, Zicsr,
> Zifencei) as a 'general-purpose' ISA, and we use the abbreviation G for
> the IMAFDZicsr Zifencei combination of instruction-set extensions."
>
> With this in mind, enabling IMAFD_Zicsr_Zifencei if the user explicitly
> enables 'G' is an expected behavior and the warning is unneeded. Any
> user caught by surprise should refer to the ISA.
>
> Remove the warning when handling RVG.
>
> [1] https://github.com/riscv/riscv-isa-manual/releases/download/Ratified-IMAFDQC/riscv-spec-20191213.pdf
>
> Reported-by: Paul A. Clarke <pclarke@ventanamicro.com>
> Suggested-by: Andrew Jones <ajones@ventanamicro.com>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Thanks!
Applied to riscv-to-apply.next
Alistair
> ---
> target/riscv/tcg/tcg-cpu.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 08b806dc07..f50ce57602 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -293,7 +293,6 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
> return;
> }
>
> - warn_report("Setting G will also set IMAFD_Zicsr_Zifencei");
> cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_icsr), true);
> cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_ifencei), true);
>
> --
> 2.41.0
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread