plugins: Few debugging cleanups

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

Posted by Philippe Mathieu-Daudé 1 year, 8 months ago

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

Posted by Pierrick Bouvier 1 year, 8 months ago

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

Posted by Philippe Mathieu-Daudé 1 year, 8 months ago

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

Posted by Pierrick Bouvier 1 year, 8 months ago

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.