The global variable general_user_opts is being assigned twice, losing
the reference to the previously allocated g_hash_table.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 target/riscv/cpu.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index e56470a374..afccfc2935 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1099,8 +1099,6 @@ static void riscv_cpu_init(Object *obj)
                             "riscv.cpu.rnmi", RNMI_MAX);
 #endif /* CONFIG_USER_ONLY */
 
-    general_user_opts = g_hash_table_new(g_str_hash, g_str_equal);
-
     /*
      * The timer and performance counters extensions were supported
      * in QEMU before they were added as discrete extensions in the
@@ -2751,6 +2749,7 @@ static void riscv_cpu_common_class_init(ObjectClass *c, const void *data)
     cc->tcg_ops = &riscv_tcg_ops;
 #endif /* CONFIG_TCG */
 
+    general_user_opts = g_hash_table_new(g_str_hash, g_str_equal);
     device_class_set_props(dc, riscv_cpu_properties);
 }
 
-- 
2.51.0

On Fri, 13 Mar 2026 at 18:30, Fabiano Rosas <farosas@suse.de> wrote:
>
> The global variable general_user_opts is being assigned twice, losing
> the reference to the previously allocated g_hash_table.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  target/riscv/cpu.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index e56470a374..afccfc2935 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1099,8 +1099,6 @@ static void riscv_cpu_init(Object *obj)
>                              "riscv.cpu.rnmi", RNMI_MAX);
>  #endif /* CONFIG_USER_ONLY */
>
> -    general_user_opts = g_hash_table_new(g_str_hash, g_str_equal);
> -
>      /*
>       * The timer and performance counters extensions were supported
>       * in QEMU before they were added as discrete extensions in the
> @@ -2751,6 +2749,7 @@ static void riscv_cpu_common_class_init(ObjectClass *c, const void *data)
>      cc->tcg_ops = &riscv_tcg_ops;
>  #endif /* CONFIG_TCG */
>
> +    general_user_opts = g_hash_table_new(g_str_hash, g_str_equal);
>      device_class_set_props(dc, riscv_cpu_properties);
>  }
>

This doesn't look to me like it's sufficient. These hash
tables are used to store the values of CPU properties.
Those are per-object, not per-class, so surely the hash
table needs to be in the object, not a global ?

There are also other hash tables in the TCG riscv code
with the same problem (using a global variable to store
property info that is per-CPU, not global).

thanks
-- PMM

On 3/16/2026 6:56 AM, Peter Maydell wrote:
> On Fri, 13 Mar 2026 at 18:30, Fabiano Rosas <farosas@suse.de> wrote:
>>
>> The global variable general_user_opts is being assigned twice, losing
>> the reference to the previously allocated g_hash_table.
>>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>   target/riscv/cpu.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index e56470a374..afccfc2935 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -1099,8 +1099,6 @@ static void riscv_cpu_init(Object *obj)
>>                               "riscv.cpu.rnmi", RNMI_MAX);
>>   #endif /* CONFIG_USER_ONLY */
>>
>> -    general_user_opts = g_hash_table_new(g_str_hash, g_str_equal);
>> -
>>       /*
>>        * The timer and performance counters extensions were supported
>>        * in QEMU before they were added as discrete extensions in the
>> @@ -2751,6 +2749,7 @@ static void riscv_cpu_common_class_init(ObjectClass *c, const void *data)
>>       cc->tcg_ops = &riscv_tcg_ops;
>>   #endif /* CONFIG_TCG */
>>
>> +    general_user_opts = g_hash_table_new(g_str_hash, g_str_equal);
>>       device_class_set_props(dc, riscv_cpu_properties);
>>   }
>>
> 
> This doesn't look to me like it's sufficient. These hash
> tables are used to store the values of CPU properties.
> Those are per-object, not per-class, so surely the hash
> table needs to be in the object, not a global ?


This hash is used as a hack to determine if a QEMU command line option was
user set or not (i.e. a global setting).  Back when this was introduced
(and I assume this is still the case today, need to double check) the command
line support in QEMU didn't provide this information.

Most QEMU targets don't care whether a given CPU property was user set or not,
but the RISC-V target has a lot of code that automagically disables stuff given
certain conditions (e.g. riscv_cpu_disable_priv_spec_isa_exts()).  The result is
that we were reaching situations, Gitlab bugs being opened and what have you,
where the user was doing things like this:

qemu-system..... -cpu X,extensionA=on (...)


But so it happens that extensionA isn't compatible with the existing extension set
in X.  The behavior back then was to disable extensionA during realize() and be
done with it, leaving users baffled and frustrated with their cmd line settings
being ignored.  Now we're able to error out in these cases.

All this said, most likely there's a better/cleaner way of storing this command
line information that has global effect.  Just point me in the right direction and
we can make it happen for the next release.


> 
> There are also other hash tables in the TCG riscv code
> with the same problem (using a global variable to store
> property info that is per-CPU, not global).


Not sure which hashes are you talking about but there's a high chance that they
have to do with tracking QEMU command line options that were set or not and we
want to know the difference.

In hindsight I should've sent a RFC detailing this situation we have in RISC-V
and maybe we could have discussed changes in the QEMU cmd line handling to provide
this info.  I'm willing to do that route if that means less hash table stuff to
deal with in RISC-V code.


Thanks,
Daniel


> 
> thanks
> -- PMM

On Mon, 16 Mar 2026 at 10:42, Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
> On 3/16/2026 6:56 AM, Peter Maydell wrote:
> > This doesn't look to me like it's sufficient. These hash
> > tables are used to store the values of CPU properties.
> > Those are per-object, not per-class, so surely the hash
> > table needs to be in the object, not a global ?
>
>
> This hash is used as a hack to determine if a QEMU command line option was
> user set or not (i.e. a global setting).  Back when this was introduced
> (and I assume this is still the case today, need to double check) the command
> line support in QEMU didn't provide this information.
>
> Most QEMU targets don't care whether a given CPU property was user set or not,
> but the RISC-V target has a lot of code that automagically disables stuff given
> certain conditions (e.g. riscv_cpu_disable_priv_spec_isa_exts()).  The result is
> that we were reaching situations, Gitlab bugs being opened and what have you,
> where the user was doing things like this:
>
> qemu-system..... -cpu X,extensionA=on (...)
>
>
> But so it happens that extensionA isn't compatible with the existing extension set
> in X.  The behavior back then was to disable extensionA during realize() and be
> done with it, leaving users baffled and frustrated with their cmd line settings
> being ignored.  Now we're able to error out in these cases.

OK, but why does this require the hash table to be global? This sounds
like it's a per-CPU thing: if I have two riscv CPUs and one is:
 cpu 0: has extension X
 cpu 1: has extension A, not extension X

that's fine -- A conflicts with X but we don't have both of them
in the same CPU. With a global hash table we have no way to distinguish
this from "one CPU with both A and X".

I would have thought we only want to error out if the user sets up
a single CPU with a conflicting set of extensions.

I'm also not sure why you need to care whether the property is set
by the user on the command line or by the board code with C code.
If a board or SoC model tries to create a CPU that has both extensions
X and A, shouldn't that also be an error that we want to bail out on?

> > There are also other hash tables in the TCG riscv code
> > with the same problem (using a global variable to store
> > property info that is per-CPU, not global).
>
>
> Not sure which hashes are you talking about but there's a high chance that they
> have to do with tracking QEMU command line options that were set or not and we
> want to know the difference.

riscv_tcg_cpu_instance_init() calls g_hash_table_new() for
misa_ext_user_opts and multi_ext_user_opts unconditionally.

thanks
-- PMM

On 3/16/2026 7:49 AM, Peter Maydell wrote:
> On Mon, 16 Mar 2026 at 10:42, Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>> On 3/16/2026 6:56 AM, Peter Maydell wrote:
>>> This doesn't look to me like it's sufficient. These hash
>>> tables are used to store the values of CPU properties.
>>> Those are per-object, not per-class, so surely the hash
>>> table needs to be in the object, not a global ?
>>
>>
>> This hash is used as a hack to determine if a QEMU command line option was
>> user set or not (i.e. a global setting).  Back when this was introduced
>> (and I assume this is still the case today, need to double check) the command
>> line support in QEMU didn't provide this information.
>>
>> Most QEMU targets don't care whether a given CPU property was user set or not,
>> but the RISC-V target has a lot of code that automagically disables stuff given
>> certain conditions (e.g. riscv_cpu_disable_priv_spec_isa_exts()).  The result is
>> that we were reaching situations, Gitlab bugs being opened and what have you,
>> where the user was doing things like this:
>>
>> qemu-system..... -cpu X,extensionA=on (...)
>>
>>
>> But so it happens that extensionA isn't compatible with the existing extension set
>> in X.  The behavior back then was to disable extensionA during realize() and be
>> done with it, leaving users baffled and frustrated with their cmd line settings
>> being ignored.  Now we're able to error out in these cases.
> 
> OK, but why does this require the hash table to be global? This sounds
> like it's a per-CPU thing: if I have two riscv CPUs and one is:
>   cpu 0: has extension X
>   cpu 1: has extension A, not extension X
> 
> that's fine -- A conflicts with X but we don't have both of them
> in the same CPU. With a global hash table we have no way to distinguish
> this from "one CPU with both A and X".
> 
> I would have thought we only want to error out if the user sets up
> a single CPU with a conflicting set of extensions.


In light of Phil's efforts w.r.t single binary QEMU, hybrid targets and etc, I
agree that this hash table should be an object instance so each CPU can have
their own extension set.  Back then the scenario you described wasn't something
we were concerning with, thus create multiple instance hashes with the same content
(since all CPUs would always be the same) seemed too much.


> 
> I'm also not sure why you need to care whether the property is set
> by the user on the command line or by the board code with C code.
> If a board or SoC model tries to create a CPU that has both extensions
> X and A, shouldn't that also be an error that we want to bail out on?

We have too much code that enable/disable extensions and chained dependencies
under the hood.

Let's say you have "-cpu rv64,extA=off,extX=on", i.e. disable A and enable X.
But X has a dependency of Y, which has a dependency of Z, and Z requires A
enabled.

So now we're on realize(), see extA=off, we disable A.  Then we go to X, figure
out that we need to enable Y because of it, but then we need to enable Z, oh and we
need to enable A as well.

In the end we ended up enabling A because it's a 3 hop dependency of Z, and now the
user is confused because he demanded A disabled.  And why wouldn't we enable A?
The default state of extensions in TCG is off, thus there was nothing to tell us if
that particular extA=off was something that the user demanded or not.

And this is how the hash table to track user opts business started. Keep in mind that
we have 148 extensions (and counting!) to deal with.  Users aren't able to track these
dependencies and get frustrated when QEMU doesn't do what it is told, and they were
going to Gitlab letting us all know about it.

Maybe the right course of action would be to claim that the user command line was bogus,
QEMU did nothing wrong and call it a day.  And maybe, now that we have profile CPUs and
people care mostly about them, we could remove all this stuff from the code and let
users fall in their faces with bogus cmd lines.  I have plans to simplify some things
(mostly the distinction between CPUs types) and might touch on this area too.  But
for now this is where we are.


Thanks,
Daniel





> 
>>> There are also other hash tables in the TCG riscv code
>>> with the same problem (using a global variable to store
>>> property info that is per-CPU, not global).
>>
>>
>> Not sure which hashes are you talking about but there's a high chance that they
>> have to do with tracking QEMU command line options that were set or not and we
>> want to know the difference.
> 
> riscv_tcg_cpu_instance_init() calls g_hash_table_new() for
> misa_ext_user_opts and multi_ext_user_opts unconditionally.
> 
> thanks
> -- PMM

On 3/13/2026 3:29 PM, Fabiano Rosas wrote:
> The global variable general_user_opts is being assigned twice, losing
> the reference to the previously allocated g_hash_table.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---

Reviewed-by: Daniel Henrique Barboza <daniel.barboza@oss.qualcomm.com>

>   target/riscv/cpu.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index e56470a374..afccfc2935 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1099,8 +1099,6 @@ static void riscv_cpu_init(Object *obj)
>                               "riscv.cpu.rnmi", RNMI_MAX);
>   #endif /* CONFIG_USER_ONLY */
>   
> -    general_user_opts = g_hash_table_new(g_str_hash, g_str_equal);
> -
>       /*
>        * The timer and performance counters extensions were supported
>        * in QEMU before they were added as discrete extensions in the
> @@ -2751,6 +2749,7 @@ static void riscv_cpu_common_class_init(ObjectClass *c, const void *data)
>       cc->tcg_ops = &riscv_tcg_ops;
>   #endif /* CONFIG_TCG */
>   
> +    general_user_opts = g_hash_table_new(g_str_hash, g_str_equal);
>       device_class_set_props(dc, riscv_cpu_properties);
>   }
>