diff mbox series

accel/tcg: nochain should disable goto_ptr

Message ID 20240531101744.1683192-1-niugen@loongson.cn (mailing list archive)
State New
Headers show
Series accel/tcg: nochain should disable goto_ptr | expand

Commit Message

niugen May 31, 2024, 10:17 a.m. UTC 
Signed-off-by: NiuGenen <niugen@loongson.cn>
---
 accel/tcg/cpu-exec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Richard Henderson May 31, 2024, 5:32 p.m. UTC | #1 
On 5/31/24 03:17, NiuGenen wrote:
> Signed-off-by: NiuGenen <niugen@loongson.cn>
> ---
>   accel/tcg/cpu-exec.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index 2972f75b96..084fa645c7 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -173,7 +173,7 @@ uint32_t curr_cflags(CPUState *cpu)
>       } else if (qatomic_read(&one_insn_per_tb)) {
>           cflags |= CF_NO_GOTO_TB | 1;
>       } else if (qemu_loglevel_mask(CPU_LOG_TB_NOCHAIN)) {
> -        cflags |= CF_NO_GOTO_TB;
> +        cflags |= CF_NO_GOTO_TB | CF_NO_GOTO_PTR;
>       }
>   
>       return cflags;

Why?

The original intent of nochain was so that -d exec would log all blocks, which requires 
excluding goto_tb.  There is exec logging in helper_lookup_goto_ptr, so there is no need 
to avoid goto_ptr.

You must provide a rationale, at minimum.


r~
niugen June 1, 2024, 5:57 a.m. UTC | #2 
on 2024/6/1 01:32, Richard Henderson wrote:
> On 5/31/24 03:17, NiuGenen wrote:
>> Signed-off-by: NiuGenen <niugen@loongson.cn>
>> ---
>>   accel/tcg/cpu-exec.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
>> index 2972f75b96..084fa645c7 100644
>> --- a/accel/tcg/cpu-exec.c
>> +++ b/accel/tcg/cpu-exec.c
>> @@ -173,7 +173,7 @@ uint32_t curr_cflags(CPUState *cpu)
>>       } else if (qatomic_read(&one_insn_per_tb)) {
>>           cflags |= CF_NO_GOTO_TB | 1;
>>       } else if (qemu_loglevel_mask(CPU_LOG_TB_NOCHAIN)) {
>> -        cflags |= CF_NO_GOTO_TB;
>> +        cflags |= CF_NO_GOTO_TB | CF_NO_GOTO_PTR;
>>       }
>>         return cflags;
>
> Why?
>
> The original intent of nochain was so that -d exec would log all 
> blocks, which requires excluding goto_tb.  There is exec logging in 
> helper_lookup_goto_ptr, so there is no need to avoid goto_ptr.
>
> You must provide a rationale, at minimum.
>
>
> r~


Sorry, my mistake. I thought nochain will disable all kinds of branches, 
including direct branch and indirect branch, but I found that indirect 
branch still call helper_lookup_tb_ptr to continue executing TB instead 
of epilogue-tblookup-prologue.

Maybe the exec logging can be removed from helper_lookup_tb_ptr and 
nochain can disable all the chaining of TB?

Thanks for your patience.
Richard Henderson June 1, 2024, 1:36 p.m. UTC | #3 
On 6/1/24 00:57, niugen wrote:
> on 2024/6/1 01:32, Richard Henderson wrote:
>> On 5/31/24 03:17, NiuGenen wrote:
>>> Signed-off-by: NiuGenen <niugen@loongson.cn>
>>> ---
>>>   accel/tcg/cpu-exec.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
>>> index 2972f75b96..084fa645c7 100644
>>> --- a/accel/tcg/cpu-exec.c
>>> +++ b/accel/tcg/cpu-exec.c
>>> @@ -173,7 +173,7 @@ uint32_t curr_cflags(CPUState *cpu)
>>>       } else if (qatomic_read(&one_insn_per_tb)) {
>>>           cflags |= CF_NO_GOTO_TB | 1;
>>>       } else if (qemu_loglevel_mask(CPU_LOG_TB_NOCHAIN)) {
>>> -        cflags |= CF_NO_GOTO_TB;
>>> +        cflags |= CF_NO_GOTO_TB | CF_NO_GOTO_PTR;
>>>       }
>>>         return cflags;
>>
>> Why?
>>
>> The original intent of nochain was so that -d exec would log all blocks, which requires 
>> excluding goto_tb.  There is exec logging in helper_lookup_goto_ptr, so there is no need 
>> to avoid goto_ptr.
>>
>> You must provide a rationale, at minimum.
>>
>>
>> r~
> 
> 
> Sorry, my mistake. I thought nochain will disable all kinds of branches, including direct 
> branch and indirect branch, but I found that indirect branch still call 
> helper_lookup_tb_ptr to continue executing TB instead of epilogue-tblookup-prologue.
> 
> Maybe the exec logging can be removed from helper_lookup_tb_ptr and nochain can disable 
> all the chaining of TB?

Why?  You still haven't provided a rationale.


r~
niugen June 3, 2024, 1:51 a.m. UTC | #4 
On 2024/6/1 21:36, Richard Henderson wrote:
> On 6/1/24 00:57, niugen wrote:
>> on 2024/6/1 01:32, Richard Henderson wrote:
>>> On 5/31/24 03:17, NiuGenen wrote:
>>>> Signed-off-by: NiuGenen <niugen@loongson.cn>
>>>> ---
>>>>   accel/tcg/cpu-exec.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
>>>> index 2972f75b96..084fa645c7 100644
>>>> --- a/accel/tcg/cpu-exec.c
>>>> +++ b/accel/tcg/cpu-exec.c
>>>> @@ -173,7 +173,7 @@ uint32_t curr_cflags(CPUState *cpu)
>>>>       } else if (qatomic_read(&one_insn_per_tb)) {
>>>>           cflags |= CF_NO_GOTO_TB | 1;
>>>>       } else if (qemu_loglevel_mask(CPU_LOG_TB_NOCHAIN)) {
>>>> -        cflags |= CF_NO_GOTO_TB;
>>>> +        cflags |= CF_NO_GOTO_TB | CF_NO_GOTO_PTR;
>>>>       }
>>>>         return cflags;
>>>
>>> Why?
>>>
>>> The original intent of nochain was so that -d exec would log all 
>>> blocks, which requires excluding goto_tb.  There is exec logging in 
>>> helper_lookup_goto_ptr, so there is no need to avoid goto_ptr.
>>>
>>> You must provide a rationale, at minimum.
>>>
>>>
>>> r~
>>
>>
>> Sorry, my mistake. I thought nochain will disable all kinds of 
>> branches, including direct branch and indirect branch, but I found 
>> that indirect branch still call helper_lookup_tb_ptr to continue 
>> executing TB instead of epilogue-tblookup-prologue.
>>
>> Maybe the exec logging can be removed from helper_lookup_tb_ptr and 
>> nochain can disable all the chaining of TB?
>
> Why?  You still haven't provided a rationale.
>
>
> r~


Currently, nochain is actually no-direct-chaining(i.e. no-goto-tb). And 
it works fine with -d exec because of the helper_lookup_tb_ptr generates 
the exec logging too. Other optimizations for indirect branch (eg. 
shadow-stack, inlined CPUJumpCache lookup, maybe developed in the 
future) that use goto_ptr will need to generate the exec logging too.

It makes the codes more clear if nochain is no-any-chaining. Any logging 
or other mechanism that needs to operate per TB's execution can be done 
in cpu_tb_exec only.
diff mbox series

Patch

diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 2972f75b96..084fa645c7 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -173,7 +173,7 @@  uint32_t curr_cflags(CPUState *cpu)
     } else if (qatomic_read(&one_insn_per_tb)) {
         cflags |= CF_NO_GOTO_TB | 1;
     } else if (qemu_loglevel_mask(CPU_LOG_TB_NOCHAIN)) {
-        cflags |= CF_NO_GOTO_TB;
+        cflags |= CF_NO_GOTO_TB | CF_NO_GOTO_PTR;
     }
 
     return cflags;