Re: [Qemu-devel] [PATCH 1/2] target-arm: Split DISAS_YIELD from DISAS_WFE
On 27 June 2015 at 03:25, Peter Crosthwaite
peter.crosthwa...@xilinx.com wrote:
On Mon, Jun 15, 2015 at 11:49 AM, Peter Maydell
peter.mayd...@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.mayd...@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
Re: [Qemu-devel] [PATCH 1/2] target-arm: Split DISAS_YIELD from DISAS_WFE
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 1/2] target-arm: Split DISAS_YIELD from DISAS_WFE
On Mon, Jun 15, 2015 at 11:49 AM, Peter Maydell
peter.mayd...@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.mayd...@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
[Qemu-devel] [PATCH 1/2] target-arm: Split DISAS_YIELD from DISAS_WFE
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.mayd...@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
