[PATCH 0/3] plugins: Few debugging cleanups

[PATCH 0/3] plugins: Few debugging cleanups

[Philippe Mathieu-Daudé]

- Assert cpu_index is assigned in INIT/EXIT hooks
- Free cpu->plugin_state
- Restrict qemu_plugin_vcpu_init__async() to plugins/

Philippe Mathieu-Daudé (3):
  plugins: Ensure vCPU index is assigned in init/exit hooks
  plugins: Free CPUPluginState before destroying vCPU state
  accel/tcg: Move qemu_plugin_vcpu_init__async() to plugins/

 include/qemu/plugin.h |  3 +++
 hw/core/cpu-common.c  | 14 ++++++--------
 plugins/core.c        | 10 +++++++++-
 3 files changed, 18 insertions(+), 9 deletions(-)

-- 
2.41.0

[PATCH 1/3] plugins: Ensure vCPU index is assigned in init/exit hooks

[Philippe Mathieu-Daudé]

Since vCPUs are hashed by their index, this index can't
be uninitialized (UNASSIGNED_CPU_INDEX).

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 plugins/core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/plugins/core.c b/plugins/core.c
index badede28cf..d339b3db4d 100644
--- a/plugins/core.c
+++ b/plugins/core.c
@@ -245,6 +245,7 @@ void qemu_plugin_vcpu_init_hook(CPUState *cpu)
 {
     bool success;
 
+    assert(cpu->cpu_index != UNASSIGNED_CPU_INDEX);
     qemu_rec_mutex_lock(&plugin.lock);
     plugin.num_vcpus = MAX(plugin.num_vcpus, cpu->cpu_index + 1);
     plugin_cpu_update__locked(&cpu->cpu_index, NULL, NULL);
@@ -263,6 +264,7 @@ void qemu_plugin_vcpu_exit_hook(CPUState *cpu)
 
     plugin_vcpu_cb__simple(cpu, QEMU_PLUGIN_EV_VCPU_EXIT);
 
+    assert(cpu->cpu_index != UNASSIGNED_CPU_INDEX);
     qemu_rec_mutex_lock(&plugin.lock);
     success = g_hash_table_remove(plugin.cpu_ht, &cpu->cpu_index);
     g_assert(success);
-- 
2.41.0

Re: [PATCH 1/3] plugins: Ensure vCPU index is assigned in init/exit hooks

[Pierrick Bouvier]

On 6/6/24 05:40, Philippe Mathieu-Daudé wrote:
> Since vCPUs are hashed by their index, this index can't
> be uninitialized (UNASSIGNED_CPU_INDEX).
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   plugins/core.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/plugins/core.c b/plugins/core.c
> index badede28cf..d339b3db4d 100644
> --- a/plugins/core.c
> +++ b/plugins/core.c
> @@ -245,6 +245,7 @@ void qemu_plugin_vcpu_init_hook(CPUState *cpu)
>   {
>       bool success;
>   
> +    assert(cpu->cpu_index != UNASSIGNED_CPU_INDEX);
>       qemu_rec_mutex_lock(&plugin.lock);
>       plugin.num_vcpus = MAX(plugin.num_vcpus, cpu->cpu_index + 1);
>       plugin_cpu_update__locked(&cpu->cpu_index, NULL, NULL);
> @@ -263,6 +264,7 @@ void qemu_plugin_vcpu_exit_hook(CPUState *cpu)
>   
>       plugin_vcpu_cb__simple(cpu, QEMU_PLUGIN_EV_VCPU_EXIT);
>   
> +    assert(cpu->cpu_index != UNASSIGNED_CPU_INDEX);
>       qemu_rec_mutex_lock(&plugin.lock);
>       success = g_hash_table_remove(plugin.cpu_ht, &cpu->cpu_index);
>       g_assert(success);

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

[PATCH 2/3] plugins: Free CPUPluginState before destroying vCPU state

[Philippe Mathieu-Daudé]

cpu::plugin_state is allocated in cpu_common_initfn() when
the vCPU state is created. Release it in cpu_common_finalize()
when we are done.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/qemu/plugin.h | 3 +++
 hw/core/cpu-common.c  | 5 +++++
 2 files changed, 8 insertions(+)

diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
index bc5aef979e..af5f9db469 100644
--- a/include/qemu/plugin.h
+++ b/include/qemu/plugin.h
@@ -149,6 +149,9 @@ struct CPUPluginState {
 
 /**
  * qemu_plugin_create_vcpu_state: allocate plugin state
+ *
+ * The returned data must be released with g_free()
+ * when no longer required.
  */
 CPUPluginState *qemu_plugin_create_vcpu_state(void);
 
diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
index bf1a7b8892..cd15402552 100644
--- a/hw/core/cpu-common.c
+++ b/hw/core/cpu-common.c
@@ -283,6 +283,11 @@ static void cpu_common_finalize(Object *obj)
 {
     CPUState *cpu = CPU(obj);
 
+#ifdef CONFIG_PLUGIN
+    if (tcg_enabled()) {
+        g_free(cpu->plugin_state);
+    }
+#endif
     g_array_free(cpu->gdb_regs, TRUE);
     qemu_lockcnt_destroy(&cpu->in_ioctl_lock);
     qemu_mutex_destroy(&cpu->work_mutex);
-- 
2.41.0

Re: [PATCH 2/3] plugins: Free CPUPluginState before destroying vCPU state

[Pierrick Bouvier]

On 6/6/24 05:40, Philippe Mathieu-Daudé wrote:
> cpu::plugin_state is allocated in cpu_common_initfn() when
> the vCPU state is created. Release it in cpu_common_finalize()
> when we are done.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/qemu/plugin.h | 3 +++
>   hw/core/cpu-common.c  | 5 +++++
>   2 files changed, 8 insertions(+)
> 
> diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
> index bc5aef979e..af5f9db469 100644
> --- a/include/qemu/plugin.h
> +++ b/include/qemu/plugin.h
> @@ -149,6 +149,9 @@ struct CPUPluginState {
>   
>   /**
>    * qemu_plugin_create_vcpu_state: allocate plugin state
> + *
> + * The returned data must be released with g_free()
> + * when no longer required.
>    */
>   CPUPluginState *qemu_plugin_create_vcpu_state(void);
>   
> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
> index bf1a7b8892..cd15402552 100644
> --- a/hw/core/cpu-common.c
> +++ b/hw/core/cpu-common.c
> @@ -283,6 +283,11 @@ static void cpu_common_finalize(Object *obj)
>   {
>       CPUState *cpu = CPU(obj);
>   
> +#ifdef CONFIG_PLUGIN
> +    if (tcg_enabled()) {
> +        g_free(cpu->plugin_state);
> +    }
> +#endif
>       g_array_free(cpu->gdb_regs, TRUE);
>       qemu_lockcnt_destroy(&cpu->in_ioctl_lock);
>       qemu_mutex_destroy(&cpu->work_mutex);

To ensure I get it right, order of cpu init/deinit is:
- init
- realize
- unrealize
- finalize
Is that correct?

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 2/3] plugins: Free CPUPluginState before destroying vCPU state

[Philippe Mathieu-Daudé]

On 6/6/24 23:14, Pierrick Bouvier wrote:
> On 6/6/24 05:40, Philippe Mathieu-Daudé wrote:
>> cpu::plugin_state is allocated in cpu_common_initfn() when
>> the vCPU state is created. Release it in cpu_common_finalize()
>> when we are done.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   include/qemu/plugin.h | 3 +++
>>   hw/core/cpu-common.c  | 5 +++++
>>   2 files changed, 8 insertions(+)
>>
>> diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
>> index bc5aef979e..af5f9db469 100644
>> --- a/include/qemu/plugin.h
>> +++ b/include/qemu/plugin.h
>> @@ -149,6 +149,9 @@ struct CPUPluginState {
>>   /**
>>    * qemu_plugin_create_vcpu_state: allocate plugin state
>> + *
>> + * The returned data must be released with g_free()
>> + * when no longer required.
>>    */
>>   CPUPluginState *qemu_plugin_create_vcpu_state(void);
>> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
>> index bf1a7b8892..cd15402552 100644
>> --- a/hw/core/cpu-common.c
>> +++ b/hw/core/cpu-common.c
>> @@ -283,6 +283,11 @@ static void cpu_common_finalize(Object *obj)
>>   {
>>       CPUState *cpu = CPU(obj);
>> +#ifdef CONFIG_PLUGIN
>> +    if (tcg_enabled()) {
>> +        g_free(cpu->plugin_state);
>> +    }
>> +#endif
>>       g_array_free(cpu->gdb_regs, TRUE);
>>       qemu_lockcnt_destroy(&cpu->in_ioctl_lock);
>>       qemu_mutex_destroy(&cpu->work_mutex);
> 
> To ensure I get it right, order of cpu init/deinit is:
> - init
> - realize
> - unrealize
> - finalize
> Is that correct?

Yes, this is valid for all QDev (CPU is based on it).

+ init: allocate state, expose configurable properties
. user configure properties
+ realize: consume properties to tune the object
+ reset: set default values
. object is used
+ unrealize: undo stuff from realize because the object
   might be realized again (unplug - plug)
+ finalize: release resources

See 
https://lore.kernel.org/qemu-devel/20240209123226.32576-1-philmd@linaro.org/

> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Thanks!

Re: [PATCH 2/3] plugins: Free CPUPluginState before destroying vCPU state

[Pierrick Bouvier]

On 6/6/24 21:53, Philippe Mathieu-Daudé wrote:
> On 6/6/24 23:14, Pierrick Bouvier wrote:
>> On 6/6/24 05:40, Philippe Mathieu-Daudé wrote:
>>> cpu::plugin_state is allocated in cpu_common_initfn() when
>>> the vCPU state is created. Release it in cpu_common_finalize()
>>> when we are done.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>    include/qemu/plugin.h | 3 +++
>>>    hw/core/cpu-common.c  | 5 +++++
>>>    2 files changed, 8 insertions(+)
>>>
>>> diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
>>> index bc5aef979e..af5f9db469 100644
>>> --- a/include/qemu/plugin.h
>>> +++ b/include/qemu/plugin.h
>>> @@ -149,6 +149,9 @@ struct CPUPluginState {
>>>    /**
>>>     * qemu_plugin_create_vcpu_state: allocate plugin state
>>> + *
>>> + * The returned data must be released with g_free()
>>> + * when no longer required.
>>>     */
>>>    CPUPluginState *qemu_plugin_create_vcpu_state(void);
>>> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
>>> index bf1a7b8892..cd15402552 100644
>>> --- a/hw/core/cpu-common.c
>>> +++ b/hw/core/cpu-common.c
>>> @@ -283,6 +283,11 @@ static void cpu_common_finalize(Object *obj)
>>>    {
>>>        CPUState *cpu = CPU(obj);
>>> +#ifdef CONFIG_PLUGIN
>>> +    if (tcg_enabled()) {
>>> +        g_free(cpu->plugin_state);
>>> +    }
>>> +#endif
>>>        g_array_free(cpu->gdb_regs, TRUE);
>>>        qemu_lockcnt_destroy(&cpu->in_ioctl_lock);
>>>        qemu_mutex_destroy(&cpu->work_mutex);
>>
>> To ensure I get it right, order of cpu init/deinit is:
>> - init
>> - realize
>> - unrealize
>> - finalize
>> Is that correct?
> 
> Yes, this is valid for all QDev (CPU is based on it).
> 
> + init: allocate state, expose configurable properties
> . user configure properties
> + realize: consume properties to tune the object
> + reset: set default values
> . object is used
> + unrealize: undo stuff from realize because the object
>     might be realized again (unplug - plug)
> + finalize: release resources
> 
> See
> https://lore.kernel.org/qemu-devel/20240209123226.32576-1-philmd@linaro.org/
> 
>> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> 
> Thanks!

Thanks, it definitely have its place in the official documentation, if 
you feel like adding it.

[PATCH 3/3] accel/tcg: Move qemu_plugin_vcpu_init__async() to plugins/

[Philippe Mathieu-Daudé]

Calling qemu_plugin_vcpu_init__async() on the vCPU thread
is a detail of plugins, not relevant to TCG vCPU management.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 hw/core/cpu-common.c | 9 +--------
 plugins/core.c       | 8 +++++++-
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
index cd15402552..79fcc0b286 100644
--- a/hw/core/cpu-common.c
+++ b/hw/core/cpu-common.c
@@ -192,13 +192,6 @@ static void cpu_common_parse_features(const char *typename, char *features,
     }
 }
 
-#ifdef CONFIG_PLUGIN
-static void qemu_plugin_vcpu_init__async(CPUState *cpu, run_on_cpu_data unused)
-{
-    qemu_plugin_vcpu_init_hook(cpu);
-}
-#endif
-
 static void cpu_common_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cpu = CPU(dev);
@@ -274,7 +267,7 @@ static void cpu_common_initfn(Object *obj)
 #ifdef CONFIG_PLUGIN
     if (tcg_enabled()) {
         cpu->plugin_state = qemu_plugin_create_vcpu_state();
-        async_run_on_cpu(cpu, qemu_plugin_vcpu_init__async, RUN_ON_CPU_NULL);
+        qemu_plugin_vcpu_init_hook(cpu);
     }
 #endif
 }
diff --git a/plugins/core.c b/plugins/core.c
index d339b3db4d..3dec3556c3 100644
--- a/plugins/core.c
+++ b/plugins/core.c
@@ -241,7 +241,7 @@ static void plugin_grow_scoreboards__locked(CPUState *cpu)
     end_exclusive();
 }
 
-void qemu_plugin_vcpu_init_hook(CPUState *cpu)
+static void qemu_plugin_vcpu_init__async(CPUState *cpu, run_on_cpu_data unused)
 {
     bool success;
 
@@ -258,6 +258,12 @@ void qemu_plugin_vcpu_init_hook(CPUState *cpu)
     plugin_vcpu_cb__simple(cpu, QEMU_PLUGIN_EV_VCPU_INIT);
 }
 
+void qemu_plugin_vcpu_init_hook(CPUState *cpu)
+{
+    /* Plugin initialization must wait until the cpu start executing code */
+    async_run_on_cpu(cpu, qemu_plugin_vcpu_init__async, RUN_ON_CPU_NULL);
+}
+
 void qemu_plugin_vcpu_exit_hook(CPUState *cpu)
 {
     bool success;
-- 
2.41.0

Re: [PATCH 0/3] plugins: Few debugging cleanups

[Philippe Mathieu-Daudé]

Ping? (series reviewed)

On 6/6/24 14:40, Philippe Mathieu-Daudé wrote:
> - Assert cpu_index is assigned in INIT/EXIT hooks
> - Free cpu->plugin_state
> - Restrict qemu_plugin_vcpu_init__async() to plugins/
> 
> Philippe Mathieu-Daudé (3):
>    plugins: Ensure vCPU index is assigned in init/exit hooks
>    plugins: Free CPUPluginState before destroying vCPU state
>    accel/tcg: Move qemu_plugin_vcpu_init__async() to plugins/

Re: [PATCH 0/3] plugins: Few debugging cleanups

[Alex Bennée]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> - Assert cpu_index is assigned in INIT/EXIT hooks
> - Free cpu->plugin_state
> - Restrict qemu_plugin_vcpu_init__async() to plugins/
>
> Philippe Mathieu-Daudé (3):
>   plugins: Ensure vCPU index is assigned in init/exit hooks
>   plugins: Free CPUPluginState before destroying vCPU state
>   accel/tcg: Move qemu_plugin_vcpu_init__async() to plugins/
>
>  include/qemu/plugin.h |  3 +++
>  hw/core/cpu-common.c  | 14 ++++++--------
>  plugins/core.c        | 10 +++++++++-
>  3 files changed, 18 insertions(+), 9 deletions(-)

Queued to plugins/next, thanks.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro