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

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

On 3/12/24 10:03, Pierrick Bouvier wrote:
> On 3/11/24 19:43, 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>
>> <snip>
>>>    
>>> +static TCGCond plugin_cond_to_tcgcond(enum qemu_plugin_cond cond)
>>> +{
>>> +    switch (cond) {
>>> +    case QEMU_PLUGIN_COND_EQ:
>>> +        return TCG_COND_EQ;
>>> +    case QEMU_PLUGIN_COND_NE:
>>> +        return TCG_COND_NE;
>>> +    case QEMU_PLUGIN_COND_LT:
>>> +        return TCG_COND_LTU;
>>> +    case QEMU_PLUGIN_COND_LE:
>>> +        return TCG_COND_LEU;
>>> +    case QEMU_PLUGIN_COND_GT:
>>> +        return TCG_COND_GTU;
>>> +    case QEMU_PLUGIN_COND_GE:
>>> +        return TCG_COND_GEU;
>>> +    default:
>>> +        /* ALWAYS and NEVER conditions should never reach */
>>> +        g_assert_not_reached();
>>> +    }
>>> +}
>>> +
>>> +static TCGOp *append_cond_udata_cb(const struct qemu_plugin_dyn_cb *cb,
>>> +                                   TCGOp *begin_op, TCGOp *op, int *cb_idx)
>>> +{
>>> +    char *ptr = cb->cond_cb.entry.score->data->data;
>>> +    size_t elem_size = g_array_get_element_size(
>>> +        cb->cond_cb.entry.score->data);
>>> +    size_t offset = cb->cond_cb.entry.offset;
>>> +    /* Condition should be negated, as calling the cb is the "else" path */
>>> +    TCGCond cond = tcg_invert_cond(plugin_cond_to_tcgcond(cb->cond_cb.cond));
>>> +
>>> +    op = copy_const_ptr(&begin_op, op, ptr);

This line was wrong, and cb->userp should be copied instead.
I'll fix this, add a test specifically checking udata for conditional 
callback and resend the series.

>>> +    op = copy_ld_i32(&begin_op, op);
>>> +    op = copy_mul_i32(&begin_op, op, elem_size);
>>> +    op = copy_ext_i32_ptr(&begin_op, op);
>>> +    op = copy_const_ptr(&begin_op, op, ptr + offset);
>>> +    op = copy_add_ptr(&begin_op, op);
>>> +    op = copy_ld_i64(&begin_op, op);
>>> +    op = copy_brcondi_i64(&begin_op, op, cond, cb->cond_cb.imm);
>>> +    op = copy_call(&begin_op, op, cb->f.vcpu_udata, cb_idx);
>>> +    op = copy_set_label(&begin_op, op);
>>> +    return op;
>>
>> I think we are missing something here to ensure that udata is set
>> correctly for the callback, see my RFC:
>>
>>     Subject: [RFC PATCH] contrib/plugins: control flow plugin (WIP!)
>>     Date: Mon, 11 Mar 2024 15:34:32 +0000
>>     Message-Id: <20240311153432.1395190-1-alex.bennee@linaro.org>
>>
>> which is seeing the same value every time in the callback.
>>
> 
> I'm trying to reproduce and will answer on this thread.