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

[Pierrick Bouvier]

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.