Patchwork tcg: Document tcg_qemu_tb_exec() and provide constants for low bit uses

login
register
mail settings
Submitter Peter Maydell
Date Feb. 20, 2013, 12:20 p.m.
Message ID <1361362828-19589-1-git-send-email-peter.maydell@linaro.org>
Download mbox | patch
Permalink /patch/222074/
State New
Headers show

Comments

Peter Maydell - Feb. 20, 2013, 12:20 p.m.
Document tcg_qemu_tb_exec(). In particular, its return value is a
combination of a pointer to the next translation block and some
extra information in the low two bits. Provide some #defines for
the values passed in these bits to improve code clarity.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
I have a patch cooking which uses the final remaining bottom-two-bits
combo to indicate "exited TB due to pending interrupt" so I thought it
would be nice to document what was going on here and get rid of some
of the magic numbers in the code.

 cpu-exec.c                |    9 +++++----
 include/exec/gen-icount.h |    2 +-
 tcg/tcg.h                 |   36 +++++++++++++++++++++++++++++++++++-
 3 files changed, 41 insertions(+), 6 deletions(-)
Richard Henderson - Feb. 20, 2013, 4:50 p.m.
On 02/20/2013 04:20 AM, Peter Maydell wrote:
> Document tcg_qemu_tb_exec(). In particular, its return value is a
> combination of a pointer to the next translation block and some
> extra information in the low two bits. Provide some #defines for
> the values passed in these bits to improve code clarity.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~
Andreas Färber - Feb. 20, 2013, 6:49 p.m.
Am 20.02.2013 13:20, schrieb Peter Maydell:
> Document tcg_qemu_tb_exec(). In particular, its return value is a
> combination of a pointer to the next translation block and some
> extra information in the low two bits. Provide some #defines for
> the values passed in these bits to improve code clarity.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I have a patch cooking which uses the final remaining bottom-two-bits
> combo to indicate "exited TB due to pending interrupt" so I thought it
> would be nice to document what was going on here and get rid of some
> of the magic numbers in the code.
> 
>  cpu-exec.c                |    9 +++++----
>  include/exec/gen-icount.h |    2 +-
>  tcg/tcg.h                 |   36 +++++++++++++++++++++++++++++++++++-
>  3 files changed, 41 insertions(+), 6 deletions(-)
> 
> diff --git a/cpu-exec.c b/cpu-exec.c
> index 9fcfe9e0..ea63e7d 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -72,7 +72,7 @@ static void cpu_exec_nocache(CPUArchState *env, int max_cycles,
>      next_tb = tcg_qemu_tb_exec(env, tb->tc_ptr);
>      cpu->current_tb = NULL;
>  
> -    if ((next_tb & 3) == 2) {
> +    if ((next_tb & TB_EXIT_MASK) == TB_EXIT_ICOUNT_EXPIRED) {
>          /* Restore PC.  This may happen if async event occurs before
>             the TB starts executing.  */
>          cpu_pc_from_tb(env, tb);
> @@ -584,7 +584,8 @@ int cpu_exec(CPUArchState *env)
>                     spans two pages, we cannot safely do a direct
>                     jump. */
>                  if (next_tb != 0 && tb->page_addr[1] == -1) {
> -                    tb_add_jump((TranslationBlock *)(next_tb & ~3), next_tb & 3, tb);
> +                    tb_add_jump((TranslationBlock *)(next_tb & ~TB_EXIT_MASK),
> +                                next_tb & TB_EXIT_MASK, tb);
>                  }
>                  spin_unlock(&tcg_ctx.tb_ctx.tb_lock);
>  
> @@ -598,10 +599,10 @@ int cpu_exec(CPUArchState *env)
>                      tc_ptr = tb->tc_ptr;
>                      /* execute the generated code */
>                      next_tb = tcg_qemu_tb_exec(env, tc_ptr);
> -                    if ((next_tb & 3) == 2) {
> +                    if ((next_tb & TB_EXIT_MASK) == TB_EXIT_ICOUNT_EXPIRED) {
>                          /* Instruction counter expired.  */
>                          int insns_left;
> -                        tb = (TranslationBlock *)(next_tb & ~3);
> +                        tb = (TranslationBlock *)(next_tb & ~TB_EXIT_MASK);
>                          /* Restore PC.  */
>                          cpu_pc_from_tb(env, tb);
>                          insns_left = env->icount_decr.u32;
> diff --git a/include/exec/gen-icount.h b/include/exec/gen-icount.h
> index 8043b3b..c858a73 100644
> --- a/include/exec/gen-icount.h
> +++ b/include/exec/gen-icount.h
> @@ -32,7 +32,7 @@ static void gen_icount_end(TranslationBlock *tb, int num_insns)
>      if (use_icount) {
>          *icount_arg = num_insns;
>          gen_set_label(icount_label);
> -        tcg_gen_exit_tb((tcg_target_long)tb + 2);
> +        tcg_gen_exit_tb((tcg_target_long)tb + TB_EXIT_ICOUNT_EXPIRED);
>      }
>  }
>  
> diff --git a/tcg/tcg.h b/tcg/tcg.h
> index 51c8176..7cf4c15 100644
> --- a/tcg/tcg.h
> +++ b/tcg/tcg.h
> @@ -669,7 +669,41 @@ TCGv_i64 tcg_const_i64(int64_t val);
>  TCGv_i32 tcg_const_local_i32(int32_t val);
>  TCGv_i64 tcg_const_local_i64(int64_t val);
>  
> -/* TCG targets may use a different definition of tcg_qemu_tb_exec. */
> +/**
> + * tcg_qemu_tb_exec:
> + * @env: CPUArchState * for the CPU

"@env: #CPUArchState for the CPU."?

> + * @tb_ptr: address of generated code for the TB to execute
> + *
> + * Start executing code from a given translation block.
> + * Where translation blocks have been linked, execution
> + * may proceed from the given TB into successive ones.
> + * Control eventually returns only when some action is needed
> + * from the top-level loop: either control must pass to a TB
> + * which has not yet been directly linked, or an asynchronous
> + * event such as an interrupt needs handling.
> + *
> + * The return value is a pointer to the next TB to execute
> + * (if known; otherwise zero). This pointer is assumed to be
> + * 4-aligned, and the bottom two bits are used to return further
> + * information:
> + *  0, 1: the link between this TB and the next is via the specified
> + *        TB index (0 or 1). That is, we left the TB via (the equivalent
> + *        of) "goto_tb <index>". The main loop uses this to determine
> + *        how to link the TB just executed to the next.
> + *  2:    we are using instruction counting code generation, and we
> + *        stopped executing this TB because the instruction counter
> + *        hit zero. In this case the next-TB pointer returned is the
> + *        TB we were partway through.
> + *
> + * Note that TCG targets may use a different definition of tcg_qemu_tb_exec
> + * to this default (which just calls the prologue code emitted by
> + * tcg_target_qemu_prologue()).
> + */

Are you sure it works to separate this comment from the macro by the
#defines below?

> +#define TB_EXIT_MASK 3

> +#define TB_EXIT_IDX0 0
> +#define TB_EXIT_IDX1 1
> +#define TB_EXIT_ICOUNT_EXPIRED 2

This smells like an enum (which would then get its own documentation
comment some day). IDX0 and IDX1 don't seem to be used, do you have a
follow-up?

Andreas

> +
>  #if !defined(tcg_qemu_tb_exec)
>  # define tcg_qemu_tb_exec(env, tb_ptr) \
>      ((tcg_target_ulong (*)(void *, void *))tcg_ctx.code_gen_prologue)(env, \
Peter Maydell - Feb. 20, 2013, 7:26 p.m.
On 20 February 2013 18:49, Andreas Färber <afaerber@suse.de> wrote:
> Am 20.02.2013 13:20, schrieb Peter Maydell:
> Are you sure it works to separate this comment from the macro by the
> #defines below?

My main use case for doc comments is "read them in situ in the
source code" so that's all I test :-)

>> +#define TB_EXIT_MASK 3
>
>> +#define TB_EXIT_IDX0 0
>> +#define TB_EXIT_IDX1 1
>> +#define TB_EXIT_ICOUNT_EXPIRED 2
>
> This smells like an enum (which would then get its own documentation
> comment some day). IDX0 and IDX1 don't seem to be used, do you have a
> follow-up?

No, they're mostly provided for completeness. (The indexes go in
via tcg_gen_goto_tb() calls mostly, and the TCG spec says the
argument is 0 or 1; they're not mixed in with the other oddities
like expiring icount at that point.)

-- PMM

Patch

diff --git a/cpu-exec.c b/cpu-exec.c
index 9fcfe9e0..ea63e7d 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -72,7 +72,7 @@  static void cpu_exec_nocache(CPUArchState *env, int max_cycles,
     next_tb = tcg_qemu_tb_exec(env, tb->tc_ptr);
     cpu->current_tb = NULL;
 
-    if ((next_tb & 3) == 2) {
+    if ((next_tb & TB_EXIT_MASK) == TB_EXIT_ICOUNT_EXPIRED) {
         /* Restore PC.  This may happen if async event occurs before
            the TB starts executing.  */
         cpu_pc_from_tb(env, tb);
@@ -584,7 +584,8 @@  int cpu_exec(CPUArchState *env)
                    spans two pages, we cannot safely do a direct
                    jump. */
                 if (next_tb != 0 && tb->page_addr[1] == -1) {
-                    tb_add_jump((TranslationBlock *)(next_tb & ~3), next_tb & 3, tb);
+                    tb_add_jump((TranslationBlock *)(next_tb & ~TB_EXIT_MASK),
+                                next_tb & TB_EXIT_MASK, tb);
                 }
                 spin_unlock(&tcg_ctx.tb_ctx.tb_lock);
 
@@ -598,10 +599,10 @@  int cpu_exec(CPUArchState *env)
                     tc_ptr = tb->tc_ptr;
                     /* execute the generated code */
                     next_tb = tcg_qemu_tb_exec(env, tc_ptr);
-                    if ((next_tb & 3) == 2) {
+                    if ((next_tb & TB_EXIT_MASK) == TB_EXIT_ICOUNT_EXPIRED) {
                         /* Instruction counter expired.  */
                         int insns_left;
-                        tb = (TranslationBlock *)(next_tb & ~3);
+                        tb = (TranslationBlock *)(next_tb & ~TB_EXIT_MASK);
                         /* Restore PC.  */
                         cpu_pc_from_tb(env, tb);
                         insns_left = env->icount_decr.u32;
diff --git a/include/exec/gen-icount.h b/include/exec/gen-icount.h
index 8043b3b..c858a73 100644
--- a/include/exec/gen-icount.h
+++ b/include/exec/gen-icount.h
@@ -32,7 +32,7 @@  static void gen_icount_end(TranslationBlock *tb, int num_insns)
     if (use_icount) {
         *icount_arg = num_insns;
         gen_set_label(icount_label);
-        tcg_gen_exit_tb((tcg_target_long)tb + 2);
+        tcg_gen_exit_tb((tcg_target_long)tb + TB_EXIT_ICOUNT_EXPIRED);
     }
 }
 
diff --git a/tcg/tcg.h b/tcg/tcg.h
index 51c8176..7cf4c15 100644
--- a/tcg/tcg.h
+++ b/tcg/tcg.h
@@ -669,7 +669,41 @@  TCGv_i64 tcg_const_i64(int64_t val);
 TCGv_i32 tcg_const_local_i32(int32_t val);
 TCGv_i64 tcg_const_local_i64(int64_t val);
 
-/* TCG targets may use a different definition of tcg_qemu_tb_exec. */
+/**
+ * tcg_qemu_tb_exec:
+ * @env: CPUArchState * for the CPU
+ * @tb_ptr: address of generated code for the TB to execute
+ *
+ * Start executing code from a given translation block.
+ * Where translation blocks have been linked, execution
+ * may proceed from the given TB into successive ones.
+ * Control eventually returns only when some action is needed
+ * from the top-level loop: either control must pass to a TB
+ * which has not yet been directly linked, or an asynchronous
+ * event such as an interrupt needs handling.
+ *
+ * The return value is a pointer to the next TB to execute
+ * (if known; otherwise zero). This pointer is assumed to be
+ * 4-aligned, and the bottom two bits are used to return further
+ * information:
+ *  0, 1: the link between this TB and the next is via the specified
+ *        TB index (0 or 1). That is, we left the TB via (the equivalent
+ *        of) "goto_tb <index>". The main loop uses this to determine
+ *        how to link the TB just executed to the next.
+ *  2:    we are using instruction counting code generation, and we
+ *        stopped executing this TB because the instruction counter
+ *        hit zero. In this case the next-TB pointer returned is the
+ *        TB we were partway through.
+ *
+ * Note that TCG targets may use a different definition of tcg_qemu_tb_exec
+ * to this default (which just calls the prologue code emitted by
+ * tcg_target_qemu_prologue()).
+ */
+#define TB_EXIT_MASK 3
+#define TB_EXIT_IDX0 0
+#define TB_EXIT_IDX1 1
+#define TB_EXIT_ICOUNT_EXPIRED 2
+
 #if !defined(tcg_qemu_tb_exec)
 # define tcg_qemu_tb_exec(env, tb_ptr) \
     ((tcg_target_ulong (*)(void *, void *))tcg_ctx.code_gen_prologue)(env, \