Re: [PATCH 4/5] plugins: conditional callbacks

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

On 3/12/24 19:04, Alex Bennée wrote:
> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
> 
>> On 3/11/24 14:08, Alex Bennée wrote:
>>> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
>>>
>>>> Extend plugins API to support callback called with a given criteria
>>>> (evaluated inline).
>>>>
>>>> Added functions:
>>>> - qemu_plugin_register_vcpu_tb_exec_cond_cb
>>>> - qemu_plugin_register_vcpu_insn_exec_cond_cb
>>>>
>>>> They expect as parameter a condition, a qemu_plugin_u64_t (op1) and an
>>>> immediate (op2). Callback is called if op1 |cond| op2 is true.
>>>>
>>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>>> ---
>>>>    include/qemu/plugin.h        |   7 ++
>>>>    include/qemu/qemu-plugin.h   |  76 +++++++++++++++
>>>>    plugins/plugin.h             |   8 ++
>>>>    accel/tcg/plugin-gen.c       | 174 ++++++++++++++++++++++++++++++++++-
>>>>    plugins/api.c                |  51 ++++++++++
>>>>    plugins/core.c               |  19 ++++
>>>>    plugins/qemu-plugins.symbols |   2 +
>>>>    7 files changed, 334 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
>>>> index d92d64744e6..056102b2361 100644
>>>> --- a/include/qemu/plugin.h
>>>> +++ b/include/qemu/plugin.h
>>>> @@ -74,6 +74,8 @@ enum plugin_dyn_cb_type {
>>>>    enum plugin_dyn_cb_subtype {
>>>>        PLUGIN_CB_REGULAR,
>>>>        PLUGIN_CB_REGULAR_R,
>>>> +    PLUGIN_CB_COND,
>>>> +    PLUGIN_CB_COND_R,
>>>>        PLUGIN_CB_INLINE_ADD_U64,
>>>>        PLUGIN_CB_INLINE_STORE_U64,
>>>>        PLUGIN_N_CB_SUBTYPES,
>>>> @@ -97,6 +99,11 @@ struct qemu_plugin_dyn_cb {
>>>>                enum qemu_plugin_op op;
>>>>                uint64_t imm;
>>>>            } inline_insn;
>>>> +        struct {
>>>> +            qemu_plugin_u64 entry;
>>>> +            enum qemu_plugin_cond cond;
>>>> +            uint64_t imm;
>>>> +        } cond_cb;
>>>>        };
>>>>    };
>>>>    diff --git a/include/qemu/qemu-plugin.h
>>>> b/include/qemu/qemu-plugin.h
>>>> index c5cac897a0b..337de25ece7 100644
>>>> --- a/include/qemu/qemu-plugin.h
>>>> +++ b/include/qemu/qemu-plugin.h
>>>> @@ -262,6 +262,29 @@ enum qemu_plugin_mem_rw {
>>>>        QEMU_PLUGIN_MEM_RW,
>>>>    };
>>>>    +/**
>>>> + * enum qemu_plugin_cond - condition to enable callback
>>>> + *
>>>> + * @QEMU_PLUGIN_COND_NEVER: false
>>>> + * @QEMU_PLUGIN_COND_ALWAYS: true
>>>> + * @QEMU_PLUGIN_COND_EQ: is equal?
>>>> + * @QEMU_PLUGIN_COND_NE: is not equal?
>>>> + * @QEMU_PLUGIN_COND_LT: is less than?
>>>> + * @QEMU_PLUGIN_COND_LE: is less than or equal?
>>>> + * @QEMU_PLUGIN_COND_GT: is greater than?
>>>> + * @QEMU_PLUGIN_COND_GE: is greater than or equal?
>>>> + */
>>>> +enum qemu_plugin_cond {
>>>> +    QEMU_PLUGIN_COND_NEVER,
>>>> +    QEMU_PLUGIN_COND_ALWAYS,
>>>> +    QEMU_PLUGIN_COND_EQ,
>>>> +    QEMU_PLUGIN_COND_NE,
>>>> +    QEMU_PLUGIN_COND_LT,
>>>> +    QEMU_PLUGIN_COND_LE,
>>>> +    QEMU_PLUGIN_COND_GT,
>>>> +    QEMU_PLUGIN_COND_GE,
>>>> +};
>>>> +
>>>>    /**
>>>>     * typedef qemu_plugin_vcpu_tb_trans_cb_t - translation callback
>>>>     * @id: unique plugin id
>>>> @@ -301,6 +324,32 @@ void qemu_plugin_register_vcpu_tb_exec_cb(struct qemu_plugin_tb *tb,
>>>>                                              enum qemu_plugin_cb_flags flags,
>>>>                                              void *userdata);
>>>>    +/**
>>>> + * qemu_plugin_register_vcpu_tb_exec_cond_cb() - register conditional callback
>>>> + * @tb: the opaque qemu_plugin_tb handle for the translation
>>>> + * @cb: callback function
>>>> + * @cond: condition to enable callback
>>>> + * @entry: first operand for condition
>>>> + * @imm: second operand for condition
>>>> + * @flags: does the plugin read or write the CPU's registers?
>>>> + * @userdata: any plugin data to pass to the @cb?
>>>> + *
>>>> + * The @cb function is called when a translated unit executes if
>>>> + * entry @cond imm is true.
>>>> + * If condition is QEMU_PLUGIN_COND_ALWAYS, condition is never interpreted and
>>>> + * this function is equivalent to qemu_plugin_register_vcpu_tb_exec_cb.
>>>> + * If condition QEMU_PLUGIN_COND_NEVER, condition is never interpreted and
>>>> + * callback is never installed.
>>>> + */
>>>> +QEMU_PLUGIN_API
>>>> +void qemu_plugin_register_vcpu_tb_exec_cond_cb(struct qemu_plugin_tb *tb,
>>>> +                                               qemu_plugin_vcpu_udata_cb_t cb,
>>>> +                                               enum qemu_plugin_cb_flags flags,
>>>> +                                               enum qemu_plugin_cond cond,
>>>> +                                               qemu_plugin_u64 entry,
>>> Is this a fixed entry or part of a scoreboard?
>>>
>>
>> entry is an entry of scoreboard (automatically associated to each vcpu
>> using vcpu_index) and can be modified by any other inline op, or
>> callback. @imm (next parameter) is fixed yes.
>>
>> callback will be called only if entry <cond> imm true.
> 
> I wonder if having an alternate form for comparing two scoreboard
> entries would be useful?
> 

We can always add a new API for that in the future if a specific need is 
identified. In our current use cases, this need was not revealed.