Re: [PATCH v3 07/17] plugins: implement inline operation relative to cpu_index

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

On 2/7/24 07:42, Richard Henderson wrote:
> On 2/6/24 19:24, Pierrick Bouvier wrote:
>> Instead of working on a fixed memory location, allow to address it based
>> on cpu_index, an element size and a given offset.
>> Result address: ptr + offset + cpu_index * element_size.
>>
>> With this, we can target a member in a struct array from a base pointer.
>>
>> Current semantic is not modified, thus inline operation still targets
>> always the same memory location.
>>
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>>    plugins/plugin.h       |  2 +-
>>    accel/tcg/plugin-gen.c | 65 +++++++++++++++++++++++++++++++++++-------
>>    plugins/api.c          |  3 +-
>>    plugins/core.c         | 12 +++++---
>>    4 files changed, 65 insertions(+), 17 deletions(-)
>>
>> diff --git a/plugins/plugin.h b/plugins/plugin.h
>> index fd93a372803..77ed10689ca 100644
>> --- a/plugins/plugin.h
>> +++ b/plugins/plugin.h
>> @@ -100,7 +100,7 @@ void plugin_register_vcpu_mem_cb(GArray **arr,
>>                                     enum qemu_plugin_mem_rw rw,
>>                                     void *udata);
>>    
>> -void exec_inline_op(struct qemu_plugin_dyn_cb *cb);
>> +void exec_inline_op(struct qemu_plugin_dyn_cb *cb, int cpu_index);
>>    
>>    int plugin_num_vcpus(void);
>>    
>> diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
>> index b37ce7683e6..68dee4c68d3 100644
>> --- a/accel/tcg/plugin-gen.c
>> +++ b/accel/tcg/plugin-gen.c
>> @@ -132,16 +132,28 @@ static void gen_empty_udata_cb_no_rwg(void)
>>     */
>>    static void gen_empty_inline_cb(void)
>>    {
>> +    TCGv_i32 cpu_index = tcg_temp_ebb_new_i32();
>> +    TCGv_ptr cpu_index_as_ptr = tcg_temp_ebb_new_ptr();
>>        TCGv_i64 val = tcg_temp_ebb_new_i64();
>>        TCGv_ptr ptr = tcg_temp_ebb_new_ptr();
>>    
>> +    tcg_gen_ld_i32(cpu_index, tcg_env,
>> +                   -offsetof(ArchCPU, env) + offsetof(CPUState, cpu_index));
>> +    /* pass an immediate != 0 so that it doesn't get optimized away */
>> +    tcg_gen_muli_i32(cpu_index, cpu_index, 0xdeadbeef);
> 
> You don't need a random immediate here.
> You can just as easily use
> 
>       tcg_gen_mul_i32(cpu_index, cpu_index, cpu_index);
> 
> with a similar comment about the true size being inserted later.
> 

Followed the tcg_gen_addi_i64 that was using this pattern in the same 
file. I'll change this to what you recommend.

> Otherwise,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> 
> r~