Re: [PATCH 25/26] tcg: exclude lookup_tb_ptr from helper instrumentation

[Alex Bennée]

Alex Bennée <alex.bennee@linaro.org> writes:

> Richard Henderson <richard.henderson@linaro.org> writes:
>
>> On 1/10/23 09:39, Alex Bennée wrote:
>>> From: Emilio Cota <cota@braap.org>
>>> It is internal to TCG and therefore we know it does not
>>> access guest memory.
>>> Related: #1381
>>> Signed-off-by: Emilio Cota <cota@braap.org>
>>> Message-Id: <20230108164731.61469-4-cota@braap.org>
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> ---
>>>   tcg/tcg.c | 6 ++++--
>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>> diff --git a/tcg/tcg.c b/tcg/tcg.c
>>> index da91779890..ee67eefc0c 100644
>>> --- a/tcg/tcg.c
>>> +++ b/tcg/tcg.c
>>> @@ -1652,8 +1652,10 @@ void tcg_gen_callN(void *func, TCGTemp *ret, int 
>>> nargs, TCGTemp **args)
>>>       op = tcg_op_alloc(INDEX_op_call, total_args);
>>>     #ifdef CONFIG_PLUGIN
>>> -    /* detect non-plugin helpers */
>>> -    if (tcg_ctx->plugin_insn && unlikely(strncmp(info->name, "plugin_", 
>>> 7))) {
>>> +    /* flag helpers that are not internal to TCG */
>>> +    if (tcg_ctx->plugin_insn &&
>>> +        strncmp(info->name, "plugin_", 7) &&
>>> +        strcmp(info->name, "lookup_tb_ptr")) {
>>>           tcg_ctx->plugin_insn->calls_helpers = true;
>>>       }
>>>   #endif
>>
>> I think this should be detected with
>>
>>   !(info->flags & TCG_CALL_NO_SIDE_EFFECTS)
>>
>> i.e., side-effects, which in this case is the possibility of a fault.
>
> That implies that:
>
> DEF_HELPER_FLAGS_2(plugin_vcpu_udata_cb, TCG_CALL_NO_RWG, void, i32, ptr)
> DEF_HELPER_FLAGS_4(plugin_vcpu_mem_cb, TCG_CALL_NO_RWG, void, i32, i32, i64, 
> ptr)
>
> should be the _SE variants as well right? They do have side-effects but
> not in guest state and they shouldn't cause a fault.

Hmm but we don't want them to go away. Maybe something like:

3 files changed, 7 insertions(+), 5 deletions(-)
accel/tcg/plugin-helpers.h | 4 ++--
include/tcg/tcg.h          | 2 ++
tcg/tcg.c                  | 6 +++---

modified   accel/tcg/plugin-helpers.h
@@ -1,4 +1,4 @@
 #ifdef CONFIG_PLUGIN
-DEF_HELPER_FLAGS_2(plugin_vcpu_udata_cb, TCG_CALL_NO_RWG, void, i32, ptr)
-DEF_HELPER_FLAGS_4(plugin_vcpu_mem_cb, TCG_CALL_NO_RWG, void, i32, i32, i64, 
ptr)
+DEF_HELPER_FLAGS_2(plugin_vcpu_udata_cb, TCG_CALL_NO_RWG | TCG_CALL_PLUGIN, 
void, i32, ptr)
+DEF_HELPER_FLAGS_4(plugin_vcpu_mem_cb, TCG_CALL_NO_RWG | TCG_CALL_PLUGIN, 
void, i32, i32, i64, ptr)
 #endif
modified   include/tcg/tcg.h
@@ -405,6 +405,8 @@ typedef TCGv_ptr TCGv_env;
 #define TCG_CALL_NO_SIDE_EFFECTS    0x0004
 /* Helper is G_NORETURN.  */
 #define TCG_CALL_NO_RETURN          0x0008
+/* Helper is part of Plugins.  */
+#define TCG_CALL_PLUGIN             0x0010
 
 /* convenience version of most used call flags */
 #define TCG_CALL_NO_RWG         TCG_CALL_NO_READ_GLOBALS
modified   tcg/tcg.c
@@ -1652,10 +1652,10 @@ void tcg_gen_callN(void *func, TCGTemp *ret, int nargs, 
TCGTemp **args)
     op = tcg_op_alloc(INDEX_op_call, total_args);
 
 #ifdef CONFIG_PLUGIN
-    /* flag helpers that are not internal to TCG */
+    /* Flag helpers that may affect guest state */
     if (tcg_ctx->plugin_insn &&
-        strncmp(info->name, "plugin_", 7) &&
-        strcmp(info->name, "lookup_tb_ptr")) {
+        !(info->flags & TCG_CALL_PLUGIN) &&
+        !(info->flags & TCG_CALL_NO_SIDE_EFFECTS)) {
         tcg_ctx->plugin_insn->calls_helpers = true;
     }
 #endif

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro