Re: [PATCH v5 0/9] TCG plugins new inline operations

[Gustavo Romero <gustavo.romero@linaro.org>]

Hi Pierrick,

On 5/2/24 5:09 PM, Gustavo Romero wrote:
> On 5/2/24 4:45 PM, Pierrick Bouvier wrote:
>> Hi Gustavo,
>>
>> On 5/2/24 12:16, Gustavo Romero wrote:
>>> Hi Pierrick,
>>>
>>> On 5/2/24 3:08 PM, Pierrick Bouvier wrote:
>>>> This series implement two new operations for plugins:
>>>> - Store inline allows to write a specific value to a scoreboard.
>>>> - Conditional callback executes a callback only when a given condition is true.
>>>>     The condition is evaluated inline.
>>>>
>>>> It's possible to mix various inline operations (add, store) with conditional
>>>> callbacks, allowing efficient "trap" based counters.
>>>>
>>>> It builds on top of new scoreboard API, introduced in the previous series.
>>>>
>>>> NOTE: Two patches still need review
>>>>
>>>> v2
>>>> -- 
>>>>
>>>> - fixed issue with udata not being passed to conditional callback
>>>> - added specific test for this in tests/plugin/inline.c (udata was NULL before).
>>>>
>>>> v3
>>>> -- 
>>>>
>>>> - rebased on top of "plugins: Rewrite plugin code generation":
>>>>     20240316015720.3661236-1-richard.henderson@linaro.org
>>>> - single pass code generation
>>>> - small cleanups for code generation
>>>>
>>>> v4
>>>> -- 
>>>>
>>>> - remove op field from qemu_plugin_inline_cb
>>>> - use tcg_constant_i64 to load immediate value to store
>>>>
>>>> v5
>>>> -- 
>>>>
>>>> - rebase on top of master now that Richard's series was merged
>>>>
>>>> Pierrick Bouvier (9):
>>>>     plugins: prepare introduction of new inline ops
>>>>     plugins: extract generate ptr for qemu_plugin_u64
>>>>     plugins: add new inline op STORE_U64
>>>>     tests/plugin/inline: add test for STORE_U64 inline op
>>>>     plugins: conditional callbacks
>>>>     tests/plugin/inline: add test for conditional callback
>>>>     plugins: distinct types for callbacks
>>>>     plugins: extract cpu_index generate
>>>>     plugins: remove op from qemu_plugin_inline_cb
>>>>
>>>>    include/qemu/plugin.h        |  42 +++++++----
>>>>    include/qemu/qemu-plugin.h   |  80 ++++++++++++++++++++-
>>>>    plugins/plugin.h             |  12 +++-
>>>>    accel/tcg/plugin-gen.c       | 136 +++++++++++++++++++++++++++--------
>>>>    plugins/api.c                |  39 ++++++++++
>>>>    plugins/core.c               | 109 ++++++++++++++++++++--------
>>>>    tests/plugin/inline.c        | 130 +++++++++++++++++++++++++++++++--
>>>>    plugins/qemu-plugins.symbols |   2 +
>>>>    8 files changed, 466 insertions(+), 84 deletions(-)
>>>
>>> The description in the commit message of patches 1/9, 2/9, 6/9, 7/9, and 8/9 is missing.
>>>
>>> Is this intentional?
>>>
>>
>> Do you mean there is no multiline commit message for those changes?
>> Indeed, for some of those patches, the change is a single line commit message.
> 
> I just see a commit title and the Signed-off-by. For example, in 8/9
> I see the following on git log:
> 
> commit f518898aa09b42e317b887237bb75a432b477c6d
> Author: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> Date:   Thu May 2 11:08:46 2024 -0700
> 
>      plugins: extract cpu_index generate
>      Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>      Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> 
> It has only the title: "plugins: extract cpu_index generate"
> and R-b and S-b, so no description about the changes.

The description (the body of the commit message) is mentioned here:

https://www.qemu.org/docs/master/devel/submitting-a-patch.html#write-a-meaningful-commit-message

After quickly looking at the committed logs, I do notice some commits missing it,
but I really believe it's important to have it for the reasons outlined in these
guidelines.


Cheers,
Gustavo