Re: [Patch 05/14] target/riscv: Fix relationship between V, Zve*, F and D

[Daniel Henrique Barboza]

On 2/14/23 05:38, Weiwei Li wrote:
> Add dependence chain:
> *  V => Zve64d => Zve64f => Zve32f => F
> *  V => Zve64d => D
>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> ---
>   target/riscv/cpu.c | 21 ++++++++++++++++++---
>   1 file changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 9a89bea2a3..4797ef9c42 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -743,12 +743,27 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU 
> *cpu, Error **errp)
>           return;
>       }
>   
> -    if (cpu->cfg.ext_v && !cpu->cfg.ext_d) {
> -        error_setg(errp, "V extension requires D extension");
> +    /* The V vector extension depends on the Zve64d extension */
> +    if (cpu->cfg.ext_v) {
> +        cpu->cfg.ext_zve64d = true;
> +    }
> +
> +    /* The Zve64d extension depends on the Zve64f extension */
> +    if (cpu->cfg.ext_zve64d) {
> +        cpu->cfg.ext_zve64f = true;
> +    }
> +
> +    /* The Zve64f extension depends on the Zve32f extension */
> +    if (cpu->cfg.ext_zve64f) {
> +        cpu->cfg.ext_zve32f = true;
> +    }
> +
> +    if (cpu->cfg.ext_zve64d && !cpu->cfg.ext_d) {
> +        error_setg(errp, "Zve64d extensions require D extension");
>           return;

I'll be honest and confess that I wrote a short essay about the problems I have
with this code. I gave up because in the end it's all stuff that we've been 
doing
for a long time in riscv_cpu_validate_set_extensions(). I'll see if I can work 
in
a redesign of that function and in how we're setting extensions automatically
without checking user input and so on.

For now I'll say that this error message seems weird because Zve64d was set to 
true
without user input. So this ends up happening:

$ ./qemu-system-riscv64 -M virt -cpu rv64,v=true,d=false
qemu-system-riscv64: Zve64d extensions require D extension

It's weird because the user didn't enabled Zve64d but the error message is 
complaining
about it. Given that the root cause is that ext_v was set, and then we've set 
other
extensions under the hood, a saner error message in this case would be "V 
extension
requires D extension".


Thanks,


Daniel



>       }
>   
> -    if ((cpu->cfg.ext_zve32f || cpu->cfg.ext_zve64f) && !cpu->cfg.ext_f) {
> +    if (cpu->cfg.ext_zve32f && !cpu->cfg.ext_f) {
>           error_setg(errp, "Zve32f/Zve64f extensions require F extension");
>           return;
>       }