[Qemu-devel] [PATCH] nios2: define tcg_env

[Qemu-devel] [PATCH] nios2: define tcg_env

[Paolo Bonzini]

This should be done by all target and, since commit 53f6672bcf
("gen-icount: use tcg_ctx.tcg_env instead of cpu_env", 2017-06-30),
is causing the NIOS2 target to hang.

This is because the test for "should I exit to the main loop"
was being done with the correct offset to the icount decrementer,
but using TCG temporary 0 (the frame pointer) rather than the
env pointer.

Cc: qemu-stable@nongnu.org
Cc: Marek Vasut <marex@denx.de>
Reported-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/nios2/translate.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/nios2/translate.c b/target/nios2/translate.c
index 6b0961837d..54fbe898df 100644
--- a/target/nios2/translate.c
+++ b/target/nios2/translate.c
@@ -948,6 +948,7 @@ void nios2_tcg_init(void)
     int i;
 
     cpu_env = tcg_global_reg_new_ptr(TCG_AREG0, "env");
+    tcg_ctx.tcg_env = cpu_env;
 
     for (i = 0; i < NUM_CORE_REGS; i++) {
         cpu_R[i] = tcg_global_mem_new(cpu_env,
-- 
2.14.2

Re: [Qemu-devel] [PATCH] nios2: define tcg_env

[Richard Henderson]

On 10/10/2017 08:18 AM, Paolo Bonzini wrote:
> This should be done by all target and, since commit 53f6672bcf
> ("gen-icount: use tcg_ctx.tcg_env instead of cpu_env", 2017-06-30),
> is causing the NIOS2 target to hang.
> 
> This is because the test for "should I exit to the main loop"
> was being done with the correct offset to the icount decrementer,
> but using TCG temporary 0 (the frame pointer) rather than the
> env pointer.
> 
> Cc: qemu-stable@nongnu.org
> Cc: Marek Vasut <marex@denx.de>
> Reported-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  target/nios2/translate.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH] nios2: define tcg_env

[Peter Maydell]

On 10 October 2017 at 16:18, Paolo Bonzini <pbonzini@redhat.com> wrote:
> This should be done by all target and, since commit 53f6672bcf
> ("gen-icount: use tcg_ctx.tcg_env instead of cpu_env", 2017-06-30),
> is causing the NIOS2 target to hang.
>
> This is because the test for "should I exit to the main loop"
> was being done with the correct offset to the icount decrementer,
> but using TCG temporary 0 (the frame pointer) rather than the
> env pointer.
>
> Cc: qemu-stable@nongnu.org
> Cc: Marek Vasut <marex@denx.de>
> Reported-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  target/nios2/translate.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/target/nios2/translate.c b/target/nios2/translate.c
> index 6b0961837d..54fbe898df 100644
> --- a/target/nios2/translate.c
> +++ b/target/nios2/translate.c
> @@ -948,6 +948,7 @@ void nios2_tcg_init(void)
>      int i;
>
>      cpu_env = tcg_global_reg_new_ptr(TCG_AREG0, "env");
> +    tcg_ctx.tcg_env = cpu_env;
>
>      for (i = 0; i < NUM_CORE_REGS; i++) {
>          cpu_R[i] = tcg_global_mem_new(cpu_env,
> --

Is there somewhere we could put an assert() so we can catch this
error for future targets rather than them just silently generating
bad code?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] nios2: define tcg_env

[Richard Henderson]

On 10/10/2017 09:49 AM, Peter Maydell wrote:
> On 10 October 2017 at 16:18, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> This should be done by all target and, since commit 53f6672bcf
>> ("gen-icount: use tcg_ctx.tcg_env instead of cpu_env", 2017-06-30),
>> is causing the NIOS2 target to hang.
>>
>> This is because the test for "should I exit to the main loop"
>> was being done with the correct offset to the icount decrementer,
>> but using TCG temporary 0 (the frame pointer) rather than the
>> env pointer.
>>
>> Cc: qemu-stable@nongnu.org
>> Cc: Marek Vasut <marex@denx.de>
>> Reported-by: Thomas Huth <thuth@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  target/nios2/translate.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/target/nios2/translate.c b/target/nios2/translate.c
>> index 6b0961837d..54fbe898df 100644
>> --- a/target/nios2/translate.c
>> +++ b/target/nios2/translate.c
>> @@ -948,6 +948,7 @@ void nios2_tcg_init(void)
>>      int i;
>>
>>      cpu_env = tcg_global_reg_new_ptr(TCG_AREG0, "env");
>> +    tcg_ctx.tcg_env = cpu_env;
>>
>>      for (i = 0; i < NUM_CORE_REGS; i++) {
>>          cpu_R[i] = tcg_global_mem_new(cpu_env,
>> --
> 
> Is there somewhere we could put an assert() so we can catch this
> error for future targets rather than them just silently generating
> bad code?

Yes, I can arrange for this.

That said, probably the best change we could make is to eliminate cpu_env
entirely, and create tcg_ctx.tcg_env generically (possibly renaming this to a
standalone global variable called cpu_env, minimizing the changes within
target/*).  It is, after all, the same for every target, and there is no longer
any other TCG_AREG* value that can be used.


r~