[Qemu-devel] [PATCH 0/2] Implement YIELD to yield in ARM and Thumb translators

[Qemu-devel] [PATCH 0/2] Implement YIELD to yield in ARM and Thumb translators

[Peter Maydell]

This patchset makes the ARM and Thumb encodings of the YIELD hint
instruction in the ARM cause the TCG CPU to yield control back
to the top level loop. This brings them into line with the A64
encoding which already did this.

Patch 1 splits out DISAS_YIELD from DISAS_WFE, because although
we currently implement them both the same way they're semantically
different, and in future we might well make WFE do something
else. (In particular when I was reviewing Greg's patches that
proposed enabling trap-to-EL2-on-WFE I didn't notice that we
would also have ended up trapping on YIELD !)


Peter Maydell (2):
  target-arm: Split DISAS_YIELD from DISAS_WFE
  target-arm: Implement YIELD insn to yield in ARM and Thumb translators

 target-arm/helper.h        |  1 +
 target-arm/op_helper.c     | 12 ++++++++++++
 target-arm/translate-a64.c |  6 ++++++
 target-arm/translate.c     |  7 +++++++
 target-arm/translate.h     |  1 +
 5 files changed, 27 insertions(+)

-- 
1.9.1

[Qemu-devel] [PATCH 1/2] target-arm: Split DISAS_YIELD from DISAS_WFE

[Peter Maydell]

Currently we use DISAS_WFE for both WFE and YIELD instructions.
This is functionally correct because at the moment both of them
are implemented as "yield this CPU back to the top level loop so
another CPU has a chance to run". However it's rather confusing
that YIELD ends up calling HELPER(wfe), and if we ever want to
implement real behaviour for WFE and SEV it's likely to trip us up.

Split out the yield codepath to use DISAS_YIELD and a new
HELPER(yield) function.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helper.h        |  1 +
 target-arm/op_helper.c     | 12 ++++++++++++
 target-arm/translate-a64.c |  6 ++++++
 target-arm/translate.h     |  1 +
 4 files changed, 20 insertions(+)

diff --git a/target-arm/helper.h b/target-arm/helper.h
index fc885de..827b33d 100644
--- a/target-arm/helper.h
+++ b/target-arm/helper.h
@@ -50,6 +50,7 @@ DEF_HELPER_2(exception_internal, void, env, i32)
 DEF_HELPER_4(exception_with_syndrome, void, env, i32, i32, i32)
 DEF_HELPER_1(wfi, void, env)
 DEF_HELPER_1(wfe, void, env)
+DEF_HELPER_1(yield, void, env)
 DEF_HELPER_1(pre_hvc, void, env)
 DEF_HELPER_2(pre_smc, void, env, i32)
 
diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index 7fa32c4..5f06ca0 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -334,6 +334,18 @@ void HELPER(wfe)(CPUARMState *env)
     cpu_loop_exit(cs);
 }
 
+void HELPER(yield)(CPUARMState *env)
+{
+    CPUState *cs = CPU(arm_env_get_cpu(env));
+
+    /* This is a non-trappable hint instruction, so semantically
+     * different from WFE even though we currently implement it
+     * identically. Yield control back to the top level loop.
+     */
+    cs->exception_index = EXCP_YIELD;
+    cpu_loop_exit(cs);
+}
+
 /* Raise an internal-to-QEMU exception. This is limited to only
  * those EXCP values which are special cases for QEMU to interrupt
  * execution and not to be used for exceptions which are passed to
diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index ffa6cb8..288121f 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -1199,6 +1199,8 @@ static void handle_hint(DisasContext *s, uint32_t insn,
         s->is_jmp = DISAS_WFI;
         return;
     case 1: /* YIELD */
+        s->is_jmp = DISAS_YIELD;
+        return;
     case 2: /* WFE */
         s->is_jmp = DISAS_WFE;
         return;
@@ -11107,6 +11109,10 @@ void gen_intermediate_code_internal_a64(ARMCPU *cpu,
             gen_a64_set_pc_im(dc->pc);
             gen_helper_wfe(cpu_env);
             break;
+        case DISAS_YIELD:
+            gen_a64_set_pc_im(dc->pc);
+            gen_helper_yield(cpu_env);
+            break;
         case DISAS_WFI:
             /* This is a special case because we don't want to just halt the CPU
              * if trying to debug across a WFI.
diff --git a/target-arm/translate.h b/target-arm/translate.h
index bcdcf11..9ab978f 100644
--- a/target-arm/translate.h
+++ b/target-arm/translate.h
@@ -103,6 +103,7 @@ static inline int default_exception_el(DisasContext *s)
 #define DISAS_WFE 7
 #define DISAS_HVC 8
 #define DISAS_SMC 9
+#define DISAS_YIELD 10
 
 #ifdef TARGET_AARCH64
 void a64_translate_init(void);
-- 
1.9.1

[Qemu-devel] [PATCH 2/2] target-arm: Implement YIELD insn to yield in ARM and Thumb translators

[Peter Maydell]

Implement the YIELD instruction in the ARM and Thumb translators to
actually yield control back to the top level loop rather than being
a simple no-op. (We already do this for A64.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/translate.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/target-arm/translate.c b/target-arm/translate.c
index ead08f4..05da26b 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -4080,6 +4080,10 @@ static void gen_rfe(DisasContext *s, TCGv_i32 pc, TCGv_i32 cpsr)
 static void gen_nop_hint(DisasContext *s, int val)
 {
     switch (val) {
+    case 1: /* yield */
+        gen_set_pc_im(s, s->pc);
+        s->is_jmp = DISAS_YIELD;
+        break;
     case 3: /* wfi */
         gen_set_pc_im(s, s->pc);
         s->is_jmp = DISAS_WFI;
@@ -11459,6 +11463,9 @@ static inline void gen_intermediate_code_internal(ARMCPU *cpu,
         case DISAS_WFE:
             gen_helper_wfe(cpu_env);
             break;
+        case DISAS_YIELD:
+            gen_helper_yield(cpu_env);
+            break;
         case DISAS_SWI:
             gen_exception(EXCP_SWI, syn_aa32_svc(dc->svc_imm, dc->thumb),
                           default_exception_el(dc));
-- 
1.9.1

Re: [Qemu-devel] [PATCH 0/2] Implement YIELD to yield in ARM and Thumb translators

[Peter Maydell]

Ping?

thanks
-- PMM

On 15 June 2015 at 19:49, Peter Maydell <peter.maydell@linaro.org> wrote:
> This patchset makes the ARM and Thumb encodings of the YIELD hint
> instruction in the ARM cause the TCG CPU to yield control back
> to the top level loop. This brings them into line with the A64
> encoding which already did this.
>
> Patch 1 splits out DISAS_YIELD from DISAS_WFE, because although
> we currently implement them both the same way they're semantically
> different, and in future we might well make WFE do something
> else. (In particular when I was reviewing Greg's patches that
> proposed enabling trap-to-EL2-on-WFE I didn't notice that we
> would also have ended up trapping on YIELD !)
>
>
> Peter Maydell (2):
>   target-arm: Split DISAS_YIELD from DISAS_WFE
>   target-arm: Implement YIELD insn to yield in ARM and Thumb translators
>
>  target-arm/helper.h        |  1 +
>  target-arm/op_helper.c     | 12 ++++++++++++
>  target-arm/translate-a64.c |  6 ++++++
>  target-arm/translate.c     |  7 +++++++
>  target-arm/translate.h     |  1 +
>  5 files changed, 27 insertions(+)

Re: [Qemu-devel] [PATCH 0/2] Implement YIELD to yield in ARM and Thumb translators

[Paolo Bonzini]

On 26/06/2015 16:04, Peter Maydell wrote:
> Ping?

Oh, YIELD was what I was thinking of in the thread about IPIs, not WFE
(which would require SEV on the other core)...

The patches look good, but I cannot really say much about this code.

Paolo

> thanks
> -- PMM
> 
> On 15 June 2015 at 19:49, Peter Maydell <peter.maydell@linaro.org> wrote:
>> This patchset makes the ARM and Thumb encodings of the YIELD hint
>> instruction in the ARM cause the TCG CPU to yield control back
>> to the top level loop. This brings them into line with the A64
>> encoding which already did this.
>>
>> Patch 1 splits out DISAS_YIELD from DISAS_WFE, because although
>> we currently implement them both the same way they're semantically
>> different, and in future we might well make WFE do something
>> else. (In particular when I was reviewing Greg's patches that
>> proposed enabling trap-to-EL2-on-WFE I didn't notice that we
>> would also have ended up trapping on YIELD !)
>>
>>
>> Peter Maydell (2):
>>   target-arm: Split DISAS_YIELD from DISAS_WFE
>>   target-arm: Implement YIELD insn to yield in ARM and Thumb translators
>>
>>  target-arm/helper.h        |  1 +
>>  target-arm/op_helper.c     | 12 ++++++++++++
>>  target-arm/translate-a64.c |  6 ++++++
>>  target-arm/translate.c     |  7 +++++++
>>  target-arm/translate.h     |  1 +
>>  5 files changed, 27 insertions(+)
> 
> 

Re: [Qemu-devel] [PATCH 0/2] Implement YIELD to yield in ARM and Thumb translators

[Peter Maydell]

On 26 June 2015 at 15:25, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 26/06/2015 16:04, Peter Maydell wrote:
>> Ping?
>
> Oh, YIELD was what I was thinking of in the thread about IPIs, not WFE
> (which would require SEV on the other core)...

Linux doesn't in practice put YIELD insns into its busy loops,
so this series is more of a completeness thing than an actual
attempt to solve the problem in the other thread.

I think the right fix for the other thread's problems is that
we should not allow any TCG CPU to run for an unbounded period
of time -- we should always switch to another CPU after some
period of time if the CPU hasn't ended up coming back out to
the top-level loop of its own accord. That's definitely post-2.4
material though.

-- PMM

Re: [Qemu-devel] [PATCH 0/2] Implement YIELD to yield in ARM and Thumb translators

[Alex Züpke]

Am 26.06.2015 um 16:04 schrieb Peter Maydell:
> Ping?

OK for me.

Best regards
Alex

> 
> thanks
> -- PMM
> 
> On 15 June 2015 at 19:49, Peter Maydell <peter.maydell@linaro.org> wrote:
>> This patchset makes the ARM and Thumb encodings of the YIELD hint
>> instruction in the ARM cause the TCG CPU to yield control back
>> to the top level loop. This brings them into line with the A64
>> encoding which already did this.
>>
>> Patch 1 splits out DISAS_YIELD from DISAS_WFE, because although
>> we currently implement them both the same way they're semantically
>> different, and in future we might well make WFE do something
>> else. (In particular when I was reviewing Greg's patches that
>> proposed enabling trap-to-EL2-on-WFE I didn't notice that we
>> would also have ended up trapping on YIELD !)
>>
>>
>> Peter Maydell (2):
>>   target-arm: Split DISAS_YIELD from DISAS_WFE
>>   target-arm: Implement YIELD insn to yield in ARM and Thumb translators
>>
>>  target-arm/helper.h        |  1 +
>>  target-arm/op_helper.c     | 12 ++++++++++++
>>  target-arm/translate-a64.c |  6 ++++++
>>  target-arm/translate.c     |  7 +++++++
>>  target-arm/translate.h     |  1 +
>>  5 files changed, 27 insertions(+)

Re: [Qemu-devel] [PATCH 1/2] target-arm: Split DISAS_YIELD from DISAS_WFE

[Andreas Färber]

Am 15.06.2015 um 20:49 schrieb Peter Maydell:
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index 7fa32c4..5f06ca0 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -334,6 +334,18 @@ void HELPER(wfe)(CPUARMState *env)
>      cpu_loop_exit(cs);
>  }
>  
> +void HELPER(yield)(CPUARMState *env)
> +{
> +    CPUState *cs = CPU(arm_env_get_cpu(env));

I'd appreciate if you could split this into two lines when applying.
No respin needed for that.

> +
> +    /* This is a non-trappable hint instruction, so semantically
> +     * different from WFE even though we currently implement it
> +     * identically. Yield control back to the top level loop.
> +     */
> +    cs->exception_index = EXCP_YIELD;
> +    cpu_loop_exit(cs);
> +}
> +
>  /* Raise an internal-to-QEMU exception. This is limited to only
>   * those EXCP values which are special cases for QEMU to interrupt
>   * execution and not to be used for exceptions which are passed to
[snip]

Looks fine otherwise.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH 0/2] Implement YIELD to yield in ARM and Thumb translators

[Peter Crosthwaite]

On Fri, Jun 26, 2015 at 7:28 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 26 June 2015 at 15:25, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>> On 26/06/2015 16:04, Peter Maydell wrote:
>>> Ping?
>>
>> Oh, YIELD was what I was thinking of in the thread about IPIs, not WFE
>> (which would require SEV on the other core)...
>
> Linux doesn't in practice put YIELD insns into its busy loops,
> so this series is more of a completeness thing than an actual
> attempt to solve the problem in the other thread.
>

So it does for AA64 now:

https://lkml.org/lkml/2015/3/2/672

Should we submit the same patch to Linux for AA32?

> I think the right fix for the other thread's problems is that
> we should not allow any TCG CPU to run for an unbounded period
> of time -- we should always switch to another CPU after some
> period of time if the CPU hasn't ended up coming back out to
> the top-level loop of its own accord. That's definitely post-2.4
> material though.

Agree. The guest should not be able to break the emulation.

Regards,
Peter

>
> -- PMM
>

Re: [Qemu-devel] [PATCH 1/2] target-arm: Split DISAS_YIELD from DISAS_WFE

[Peter Crosthwaite]

On Mon, Jun 15, 2015 at 11:49 AM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> Currently we use DISAS_WFE for both WFE and YIELD instructions.
> This is functionally correct because at the moment both of them
> are implemented as "yield this CPU back to the top level loop so
> another CPU has a chance to run". However it's rather confusing
> that YIELD ends up calling HELPER(wfe), and if we ever want to
> implement real behaviour for WFE and SEV it's likely to trip us up.
>
> Split out the yield codepath to use DISAS_YIELD and a new
> HELPER(yield) function.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/helper.h        |  1 +
>  target-arm/op_helper.c     | 12 ++++++++++++
>  target-arm/translate-a64.c |  6 ++++++
>  target-arm/translate.h     |  1 +
>  4 files changed, 20 insertions(+)
>
> diff --git a/target-arm/helper.h b/target-arm/helper.h
> index fc885de..827b33d 100644
> --- a/target-arm/helper.h
> +++ b/target-arm/helper.h
> @@ -50,6 +50,7 @@ DEF_HELPER_2(exception_internal, void, env, i32)
>  DEF_HELPER_4(exception_with_syndrome, void, env, i32, i32, i32)
>  DEF_HELPER_1(wfi, void, env)
>  DEF_HELPER_1(wfe, void, env)
> +DEF_HELPER_1(yield, void, env)
>  DEF_HELPER_1(pre_hvc, void, env)
>  DEF_HELPER_2(pre_smc, void, env, i32)
>
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index 7fa32c4..5f06ca0 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -334,6 +334,18 @@ void HELPER(wfe)(CPUARMState *env)
>      cpu_loop_exit(cs);
>  }
>
> +void HELPER(yield)(CPUARMState *env)
> +{
> +    CPUState *cs = CPU(arm_env_get_cpu(env));
> +
> +    /* This is a non-trappable hint instruction, so semantically
> +     * different from WFE even though we currently implement it
> +     * identically. Yield control back to the top level loop.
> +     */

Comment referencing out of scope functionality is a trap for
developers, anyone patching WFE and not thinking about yield needs to
be aware of comment staleness over here.

> +    cs->exception_index = EXCP_YIELD;
> +    cpu_loop_exit(cs);
> +}
> +

I think the real problem here is the inaccuracy of WFE and not yield,
so that is where such an explanatory comment should go. You can also
make it more self documenting by maying wfe call yield:

HELPER(wfe)(CPUARMState *env)
{
    /* This is a hint instruction semantically different from YIELD
even though we currently
     * implement it identically. Yield control back to the top level loop ...
     */
    HELPER(yield)(env);
}

Regards,
Peter

>  /* Raise an internal-to-QEMU exception. This is limited to only
>   * those EXCP values which are special cases for QEMU to interrupt
>   * execution and not to be used for exceptions which are passed to
> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> index ffa6cb8..288121f 100644
> --- a/target-arm/translate-a64.c
> +++ b/target-arm/translate-a64.c
> @@ -1199,6 +1199,8 @@ static void handle_hint(DisasContext *s, uint32_t insn,
>          s->is_jmp = DISAS_WFI;
>          return;
>      case 1: /* YIELD */
> +        s->is_jmp = DISAS_YIELD;
> +        return;
>      case 2: /* WFE */
>          s->is_jmp = DISAS_WFE;
>          return;
> @@ -11107,6 +11109,10 @@ void gen_intermediate_code_internal_a64(ARMCPU *cpu,
>              gen_a64_set_pc_im(dc->pc);
>              gen_helper_wfe(cpu_env);
>              break;
> +        case DISAS_YIELD:
> +            gen_a64_set_pc_im(dc->pc);
> +            gen_helper_yield(cpu_env);
> +            break;
>          case DISAS_WFI:
>              /* This is a special case because we don't want to just halt the CPU
>               * if trying to debug across a WFI.
> diff --git a/target-arm/translate.h b/target-arm/translate.h
> index bcdcf11..9ab978f 100644
> --- a/target-arm/translate.h
> +++ b/target-arm/translate.h
> @@ -103,6 +103,7 @@ static inline int default_exception_el(DisasContext *s)
>  #define DISAS_WFE 7
>  #define DISAS_HVC 8
>  #define DISAS_SMC 9
> +#define DISAS_YIELD 10
>
>  #ifdef TARGET_AARCH64
>  void a64_translate_init(void);
> --
> 1.9.1
>
>

Re: [Qemu-devel] [PATCH 1/2] target-arm: Split DISAS_YIELD from DISAS_WFE

[Peter Maydell]

On 27 June 2015 at 03:25, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Mon, Jun 15, 2015 at 11:49 AM, Peter Maydell
> <peter.maydell@linaro.org> wrote:
>> Currently we use DISAS_WFE for both WFE and YIELD instructions.
>> This is functionally correct because at the moment both of them
>> are implemented as "yield this CPU back to the top level loop so
>> another CPU has a chance to run". However it's rather confusing
>> that YIELD ends up calling HELPER(wfe), and if we ever want to
>> implement real behaviour for WFE and SEV it's likely to trip us up.
>>
>> Split out the yield codepath to use DISAS_YIELD and a new
>> HELPER(yield) function.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  target-arm/helper.h        |  1 +
>>  target-arm/op_helper.c     | 12 ++++++++++++
>>  target-arm/translate-a64.c |  6 ++++++
>>  target-arm/translate.h     |  1 +
>>  4 files changed, 20 insertions(+)
>>
>> diff --git a/target-arm/helper.h b/target-arm/helper.h
>> index fc885de..827b33d 100644
>> --- a/target-arm/helper.h
>> +++ b/target-arm/helper.h
>> @@ -50,6 +50,7 @@ DEF_HELPER_2(exception_internal, void, env, i32)
>>  DEF_HELPER_4(exception_with_syndrome, void, env, i32, i32, i32)
>>  DEF_HELPER_1(wfi, void, env)
>>  DEF_HELPER_1(wfe, void, env)
>> +DEF_HELPER_1(yield, void, env)
>>  DEF_HELPER_1(pre_hvc, void, env)
>>  DEF_HELPER_2(pre_smc, void, env, i32)
>>
>> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
>> index 7fa32c4..5f06ca0 100644
>> --- a/target-arm/op_helper.c
>> +++ b/target-arm/op_helper.c
>> @@ -334,6 +334,18 @@ void HELPER(wfe)(CPUARMState *env)
>>      cpu_loop_exit(cs);
>>  }
>>
>> +void HELPER(yield)(CPUARMState *env)
>> +{
>> +    CPUState *cs = CPU(arm_env_get_cpu(env));
>> +
>> +    /* This is a non-trappable hint instruction, so semantically
>> +     * different from WFE even though we currently implement it
>> +     * identically. Yield control back to the top level loop.
>> +     */
>
> Comment referencing out of scope functionality is a trap for
> developers, anyone patching WFE and not thinking about yield needs to
> be aware of comment staleness over here.
>
>> +    cs->exception_index = EXCP_YIELD;
>> +    cpu_loop_exit(cs);
>> +}
>> +
>
> I think the real problem here is the inaccuracy of WFE and not yield,
> so that is where such an explanatory comment should go. You can also
> make it more self documenting by maying wfe call yield:
>
> HELPER(wfe)(CPUARMState *env)
> {
>     /* This is a hint instruction semantically different from YIELD
> even though we currently
>      * implement it identically. Yield control back to the top level loop ...
>      */
>     HELPER(yield)(env);
> }

Yeah, I agree. I'll respin.

-- PMM