target/riscv/tcg/tcg-cpu.c | 2 ++ 1 file changed, 2 insertions(+)
riscv_cpu_validate_v() left its variable, min_vlen, uninitialized if
no vector extension is available, causing a compiler warning. Avoid
the warning by calling g_assert_not_reached() in the case.
Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
target/riscv/tcg/tcg-cpu.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index 1150bd14697c..acbfac5d9e2c 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -426,6 +426,8 @@ static void riscv_cpu_validate_v(CPURISCVState *env, RISCVCPUConfig *cfg,
min_vlen = 64;
} else if (cfg->ext_zve32x) {
min_vlen = 32;
+ } else {
+ g_assert_not_reached();
}
if (vlen > RV_VLEN_MAX || vlen < min_vlen) {
---
base-commit: c85ba2d7a4056595166689890285105579db446a
change-id: 20251019-vlen-30a57c03bd93
Best regards,
--
Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
On 10/19/25 5:19 AM, Akihiko Odaki wrote:
> riscv_cpu_validate_v() left its variable, min_vlen, uninitialized if
> no vector extension is available, causing a compiler warning. Avoid
> the warning by calling g_assert_not_reached() in the case.
For the compiler point of view the variable will be left uninitialized.
In reality we'll always set it to at least '32' in validate_v(). This
is how the function is being called:
if (cpu->cfg.ext_zve32x) {
riscv_cpu_validate_v(env, &cpu->cfg, &local_err);
if (local_err != NULL) {
error_propagate(errp, local_err);
return;
}
}
This means that inside the function we guarantee that min_vlen will be
at least set to 32 because cfg->ext_zve32x will always be true:
if (riscv_has_ext(env, RVV)) {
min_vlen = 128;
} else if (cfg->ext_zve64x) {
min_vlen = 64;
} else if (cfg->ext_zve32x) {
min_vlen = 32;
}
To make the compiler happy and the code a bit clearer I suggest initializing
min_vlen = 32 and folding the "if (cpu->cfg.ext_zve32x)" check inside
validate_v() for an early exit. Something like this:
@@ -417,15 +417,19 @@ static void riscv_cpu_validate_misa_priv(CPURISCVState *env, Error **errp)
static void riscv_cpu_validate_v(CPURISCVState *env, RISCVCPUConfig *cfg,
Error **errp)
{
- uint32_t min_vlen;
- uint32_t vlen = cfg->vlenb << 3;
+ uint32_t min_vlen, vlen;
+
+ if (cfg->ext_zve32x) {
+ return;
+ }
+
+ min_vlen = 32;
+ vlen = cfg->vlenb << 3;
if (riscv_has_ext(env, RVV)) {
min_vlen = 128;
} else if (cfg->ext_zve64x) {
min_vlen = 64;
- } else if (cfg->ext_zve32x) {
- min_vlen = 32;
}
if (vlen > RV_VLEN_MAX || vlen < min_vlen) {
@@ -676,12 +680,10 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
return;
}
- if (cpu->cfg.ext_zve32x) {
- riscv_cpu_validate_v(env, &cpu->cfg, &local_err);
- if (local_err != NULL) {
- error_propagate(errp, local_err);
- return;
- }
+ riscv_cpu_validate_v(env, &cpu->cfg, &local_err);
+ if (local_err != NULL) {
+ error_propagate(errp, local_err);
+ return;
}
Note: I wonder why we're allowing settings of VLEN and so on when we do
not have RVV set. Seems like a bug ...
Thanks,
Daniel
>
> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> ---
> target/riscv/tcg/tcg-cpu.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 1150bd14697c..acbfac5d9e2c 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -426,6 +426,8 @@ static void riscv_cpu_validate_v(CPURISCVState *env, RISCVCPUConfig *cfg,
> min_vlen = 64;
> } else if (cfg->ext_zve32x) {
> min_vlen = 32;
> + } else {
> + g_assert_not_reached();
> }
>
> if (vlen > RV_VLEN_MAX || vlen < min_vlen) {
>
> ---
> base-commit: c85ba2d7a4056595166689890285105579db446a
> change-id: 20251019-vlen-30a57c03bd93
>
> Best regards,
> --
> Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
>
On 2025/10/19 19:01, Daniel Henrique Barboza wrote:
>
>
> On 10/19/25 5:19 AM, Akihiko Odaki wrote:
>> riscv_cpu_validate_v() left its variable, min_vlen, uninitialized if
>> no vector extension is available, causing a compiler warning. Avoid
>> the warning by calling g_assert_not_reached() in the case.
>
> For the compiler point of view the variable will be left uninitialized.
> In reality we'll always set it to at least '32' in validate_v(). This
> is how the function is being called:
>
> if (cpu->cfg.ext_zve32x) {
> riscv_cpu_validate_v(env, &cpu->cfg, &local_err);
> if (local_err != NULL) {
> error_propagate(errp, local_err);
> return;
> }
> }
>
> This means that inside the function we guarantee that min_vlen will be
> at least set to 32 because cfg->ext_zve32x will always be true:
>
> if (riscv_has_ext(env, RVV)) {
> min_vlen = 128;
> } else if (cfg->ext_zve64x) {
> min_vlen = 64;
> } else if (cfg->ext_zve32x) {
> min_vlen = 32;
> }
>
>
> To make the compiler happy and the code a bit clearer I suggest
> initializing
> min_vlen = 32 and folding the "if (cpu->cfg.ext_zve32x)" check inside
> validate_v() for an early exit. Something like this:
>
>
> @@ -417,15 +417,19 @@ static void
> riscv_cpu_validate_misa_priv(CPURISCVState *env, Error **errp)
> static void riscv_cpu_validate_v(CPURISCVState *env, RISCVCPUConfig *cfg,
> Error **errp)
> {
> - uint32_t min_vlen;
> - uint32_t vlen = cfg->vlenb << 3;
> + uint32_t min_vlen, vlen;
> +
> + if (cfg->ext_zve32x) {
> + return;
> + }
> +
> + min_vlen = 32;
> + vlen = cfg->vlenb << 3;
>
> if (riscv_has_ext(env, RVV)) {
> min_vlen = 128;
> } else if (cfg->ext_zve64x) {
> min_vlen = 64;
> - } else if (cfg->ext_zve32x) {
> - min_vlen = 32;
> }
What about:
if (riscv_has_ext(env, RVV)) {
min_vlen = 128;
} else if (cfg->ext_zve64x) {
min_vlen = 64;
} else if (cfg->ext_zve32x) {
min_vlen = 32;
} else {
return;
}
Always initializing min_vlen with 32 looks a bit misleading to me.
min_vlen is inherently dependent on the RVV and zve* flags; initializing
the value after checking the flags show that more clearly.
And I find separating the cfg->ext_zve64x and cfg->ext_zve32x checks a
bit awkward as they are semantically not that different. In terms of
semantics, I see the code like as follows:
if (RVV) {
initialize the extension parameters for RVV
} else if (zve64x) {
initialize the extension parameters for zve64x
} else if (zve32x) {
initialize the extension parameters for zve32x
} else {
no vector extension is present so skip validation
}
>
> if (vlen > RV_VLEN_MAX || vlen < min_vlen) {
> @@ -676,12 +680,10 @@ void riscv_cpu_validate_set_extensions(RISCVCPU
> *cpu, Error **errp)
> return;
> }
>
> - if (cpu->cfg.ext_zve32x) {
> - riscv_cpu_validate_v(env, &cpu->cfg, &local_err);
> - if (local_err != NULL) {
> - error_propagate(errp, local_err);
> - return;
> - }
> + riscv_cpu_validate_v(env, &cpu->cfg, &local_err);
> + if (local_err != NULL) {
> + error_propagate(errp, local_err);
> + return;
Removing this duplicate cpu->cfg.ext_zve32x looks good.
> }
>
>
> Note: I wonder why we're allowing settings of VLEN and so on when we do
> not have RVV set. Seems like a bug ...
I think this is because the ordering of property setting is not
deterministic. It is possible to set the vlen property before setting
the v, zve64x or zve32x properties.
Regards,
Akihiko Odaki
On 10/19/25 9:22 PM, Akihiko Odaki wrote:
> On 2025/10/19 19:01, Daniel Henrique Barboza wrote:
>>
>>
>> On 10/19/25 5:19 AM, Akihiko Odaki wrote:
>>> riscv_cpu_validate_v() left its variable, min_vlen, uninitialized if
>>> no vector extension is available, causing a compiler warning. Avoid
>>> the warning by calling g_assert_not_reached() in the case.
>>
>> For the compiler point of view the variable will be left uninitialized.
>> In reality we'll always set it to at least '32' in validate_v(). This
>> is how the function is being called:
>>
>> if (cpu->cfg.ext_zve32x) {
>> riscv_cpu_validate_v(env, &cpu->cfg, &local_err);
>> if (local_err != NULL) {
>> error_propagate(errp, local_err);
>> return;
>> }
>> }
>>
>> This means that inside the function we guarantee that min_vlen will be
>> at least set to 32 because cfg->ext_zve32x will always be true:
>>
>> if (riscv_has_ext(env, RVV)) {
>> min_vlen = 128;
>> } else if (cfg->ext_zve64x) {
>> min_vlen = 64;
>> } else if (cfg->ext_zve32x) {
>> min_vlen = 32;
>> }
>>
>>
>> To make the compiler happy and the code a bit clearer I suggest initializing
>> min_vlen = 32 and folding the "if (cpu->cfg.ext_zve32x)" check inside
>> validate_v() for an early exit. Something like this:
>>
>>
>> @@ -417,15 +417,19 @@ static void riscv_cpu_validate_misa_priv(CPURISCVState *env, Error **errp)
>> static void riscv_cpu_validate_v(CPURISCVState *env, RISCVCPUConfig *cfg,
>> Error **errp)
>> {
>> - uint32_t min_vlen;
>> - uint32_t vlen = cfg->vlenb << 3;
>> + uint32_t min_vlen, vlen;
>> +
>> + if (cfg->ext_zve32x) {
>> + return;
>> + }
>> +
>> + min_vlen = 32;
>> + vlen = cfg->vlenb << 3;
>>
>> if (riscv_has_ext(env, RVV)) {
>> min_vlen = 128;
>> } else if (cfg->ext_zve64x) {
>> min_vlen = 64;
>> - } else if (cfg->ext_zve32x) {
>> - min_vlen = 32;
>> }
>
> What about:
>
> if (riscv_has_ext(env, RVV)) {
> min_vlen = 128;
> } else if (cfg->ext_zve64x) {
> min_vlen = 64;
> } else if (cfg->ext_zve32x) {
> min_vlen = 32;
> } else {
> return;
> }
>
> Always initializing min_vlen with 32 looks a bit misleading to me. min_vlen is inherently dependent on the RVV and zve* flags; initializing the value after checking the flags show that more clearly.
LGTM
>
> And I find separating the cfg->ext_zve64x and cfg->ext_zve32x checks a bit awkward as they are semantically not that different. In terms of semantics, I see the code like as follows:
>
> if (RVV) {
> initialize the extension parameters for RVV
> } else if (zve64x) {
> initialize the extension parameters for zve64x
> } else if (zve32x) {
> initialize the extension parameters for zve32x
> } else {
> no vector extension is present so skip validation
> }
>
>>
>> if (vlen > RV_VLEN_MAX || vlen < min_vlen) {
>> @@ -676,12 +680,10 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>> return;
>> }
>>
>> - if (cpu->cfg.ext_zve32x) {
>> - riscv_cpu_validate_v(env, &cpu->cfg, &local_err);
>> - if (local_err != NULL) {
>> - error_propagate(errp, local_err);
>> - return;
>> - }
>> + riscv_cpu_validate_v(env, &cpu->cfg, &local_err);
>> + if (local_err != NULL) {
>> + error_propagate(errp, local_err);
>> + return;
>
> Removing this duplicate cpu->cfg.ext_zve32x looks good.
>
>> }
>>
>>
>> Note: I wonder why we're allowing settings of VLEN and so on when we do
>> not have RVV set. Seems like a bug ...
>
> I think this is because the ordering of property setting is not deterministic. It is possible to set the vlen property before setting the v, zve64x or zve32x properties.
Hmmm good point. The logic predates the current structure we have ATM.
Thanks,
Daniel
>
> Regards,
> Akihiko Odaki
© 2016 - 2025 Red Hat, Inc.