diff mbox series

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

Message ID 20230110173922.265055-26-alex.bennee@linaro.org
State New
Headers show
Series current maintainer trees (testing/semihosting/plugins) | expand

Commit Message

Alex Bennée Jan. 10, 2023, 5:39 p.m. UTC 
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(-)

Comments

Richard Henderson Jan. 11, 2023, 7:15 p.m. UTC | #1 
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.


r~
Alex Bennée Jan. 12, 2023, 9:52 a.m. UTC | #2 
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.

>
>
> r~
Alex Bennée Jan. 12, 2023, 11:59 a.m. UTC | #3 
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
diff mbox series

Patch

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