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

[Alex Bennée]

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);
> +    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.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro