Re: [Qemu-devel] [PATCH v7 4/7] exec: [tcg] Use different TBs according to the vCPU's dynamic tracing state

["Emilio G. Cota" <cota@braap.org>]

On Fri, Jan 13, 2017 at 21:48:35 +0100, Lluís Vilanova wrote:
>  9 files changed, 54 insertions(+), 11 deletions(-)
> 
> diff --git a/cpu-exec.c b/cpu-exec.c
> index 4188fed3c6..36709cba1f 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -261,6 +261,7 @@ struct tb_desc {
>      CPUArchState *env;
>      tb_page_addr_t phys_page1;
>      uint32_t flags;
> +    TRACE_QHT_VCPU_DSTATE_TYPE trace_vcpu_dstate;
(snip)
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 57cd978578..ae74f61ea2 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -200,6 +200,10 @@ static inline void tlb_flush_by_mmuidx(CPUState *cpu, ...)
>  #define USE_DIRECT_JUMP
>  #endif
>  
> +/**
> + * TranslationBlock:
> + * @trace_vcpu_dstate: Per-vCPU dynamic tracing state used to generate this TB.
> + */
>  struct TranslationBlock {
>      target_ulong pc;   /* simulated PC corresponding to this block (EIP + CS base) */
>      target_ulong cs_base; /* CS base for this block */
> @@ -215,6 +219,7 @@ struct TranslationBlock {
>  #define CF_IGNORE_ICOUNT 0x40000 /* Do not generate icount code */
>  
>      uint16_t invalid;
> +    TRACE_QHT_VCPU_DSTATE_TYPE trace_vcpu_dstate;
(snip)
> --- a/include/exec/tb-hash-xx.h
> +++ b/include/exec/tb-hash-xx.h
> @@ -35,6 +35,7 @@
>  #define EXEC_TB_HASH_XX_H
>  
>  #include "qemu/bitops.h"
> +#include "qemu-common.h"
>  
>  #define PRIME32_1   2654435761U
>  #define PRIME32_2   2246822519U
> @@ -49,7 +50,8 @@
>   * contiguous in memory.
>   */
>  static inline
> -uint32_t tb_hash_func5(uint64_t a0, uint64_t b0, uint32_t e)
> +uint32_t tb_hash_func6(uint64_t a0, uint64_t b0, uint32_t e,
> +                       TRACE_QHT_VCPU_DSTATE_TYPE f)

I find this typedef unnecessary. Why not use u32 everywhere?
If ever we need more bits, then we'll add additional u32's here
as well.

Also, including above qemu-common.h goes against the spirit of
keeping this file (as well as tb-hash.h) free of external
dependences. (originally tb-hash.h's contents were in exec-all.h)

If we're worried about forgetting to update the hash function,
we could have a compile-time check + a comment elsewhere
(e.g. translate-all.c).

>  {
>      uint32_t v1 = TB_HASH_XX_SEED + PRIME32_1 + PRIME32_2;
>      uint32_t v2 = TB_HASH_XX_SEED + PRIME32_2;
> @@ -83,6 +85,10 @@ uint32_t tb_hash_func5(uint64_t a0, uint64_t b0, uint32_t e)
Right here you should also do:

@@ -78,7 +78,7 @@ uint32_t tb_hash_func5(uint64_t a0, uint64_t b0, uint32_t e)
     v4 *= PRIME32_1;

     h32 = rol32(v1, 1) + rol32(v2, 7) + rol32(v3, 12) + rol32(v4, 18);
-    h32 += 20;
+    h32 += 24;

to take into account the newly added parameter.

>      h32 += e * PRIME32_3;
>      h32  = rol32(h32, 17) * PRIME32_4;
>  
> +    QEMU_BUILD_BUG_ON(sizeof(TRACE_QHT_VCPU_DSTATE_TYPE) != sizeof(uint32_t));
> +    h32 += f * PRIME32_3;
> +    h32  = rol32(h32, 17) * PRIME32_4;

If we get rid of the typedef, this compile-time check will go away too.

(snip)
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index 1430390eb6..aaaa73a6fe 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -151,4 +151,7 @@ void page_size_init(void);
>   * returned. */
>  bool dump_in_progress(void);
>  
> +/* Use a macro to allow safe changes to its size in the future */
> +#define TRACE_QHT_VCPU_DSTATE_TYPE uint32_t
> +
(snip)
> diff --git a/translate-all.c b/translate-all.c
> index 29ccb9e546..6e1b1d474c 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -54,6 +54,7 @@
>  #include "exec/tb-hash.h"
>  #include "translate-all.h"
>  #include "qemu/bitmap.h"
> +#include "qemu/error-report.h"
>  #include "qemu/timer.h"
>  #include "exec/log.h"
>  
> @@ -813,6 +814,12 @@ static void tb_htable_init(void)
>  {
>      unsigned int mode = QHT_MODE_AUTO_RESIZE;
>  
> +    /* Ensure TB hash function covers the bitmap size */
> +    if (DIV_ROUND_UP(trace_get_vcpu_event_count(), BITS_PER_BYTE) >
> +        sizeof(TRACE_QHT_VCPU_DSTATE_TYPE)) {
> +        error_report("too many 'vcpu' events for the TB hash function");
> +    }
> +

This is a better place to do the above check, I think.

Thanks,

		Emilio