[PATCH v1 0/5] plugins/next

[PATCH v1 0/5] plugins/next

[Alex Bennée]

Hi,

A few bits and pieces for plugin cleanup. The most major thing in this
series is tweaking the riscv decoder to only load the instruction
bytes it needs.

Alex Bennée (3):
  docs/devel: document query handle lifetimes
  target/riscv: progressively load the instruction during decode
  tests/plugins: make howvec clean-up after itself.

Chen Qun (1):
  tests/plugin: prevent uninitialized warning

Emilio G. Cota (1):
  plugins/core: add missing break in cb_to_tcg_flags

 docs/devel/tcg-plugins.rst | 13 +++++++++++--
 plugins/core.c             |  1 +
 target/riscv/translate.c   | 39 +++++++++++++++++++-------------------
 tests/plugin/bb.c          |  6 +++---
 tests/plugin/howvec.c      | 12 +++++++++++-
 tests/plugin/insn.c        |  3 +--
 6 files changed, 47 insertions(+), 27 deletions(-)

-- 
2.20.1

[PATCH v1 1/5] docs/devel: document query handle lifetimes

[Alex Bennée]

I forgot to document the lifetime of handles in the developer
documentation. Do so now.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 docs/devel/tcg-plugins.rst | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst
index 718eef00f22..a05990906cc 100644
--- a/docs/devel/tcg-plugins.rst
+++ b/docs/devel/tcg-plugins.rst
@@ -51,8 +51,17 @@ about how QEMU's translation works to the plugins. While there are
 conceptions such as translation time and translation blocks the
 details are opaque to plugins. The plugin is able to query select
 details of instructions and system configuration only through the
-exported *qemu_plugin* functions. The types used to describe
-instructions and events are opaque to the plugins themselves.
+exported *qemu_plugin* functions.
+
+Query Handle Lifetime
+---------------------
+
+Each callback provides an opaque anonymous information handle which
+can usually be further queried to find out information about a
+translation, instruction or operation. The handles themselves are only
+valid during the lifetime of the callback so it is important that any
+information that is needed is extracted during the callback and saved
+by the plugin.
 
 Usage
 =====
-- 
2.20.1

Re: [PATCH v1 1/5] docs/devel: document query handle lifetimes

[Robert Foley]

On Fri, 7 Feb 2020 at 10:01, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> I forgot to document the lifetime of handles in the developer
> documentation. Do so now.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Robert Foley <robert.foley@linaro.org>

Regards,
-Rob
> ---
>  docs/devel/tcg-plugins.rst | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst
> index 718eef00f22..a05990906cc 100644
> --- a/docs/devel/tcg-plugins.rst
> +++ b/docs/devel/tcg-plugins.rst
> @@ -51,8 +51,17 @@ about how QEMU's translation works to the plugins. While there are
>  conceptions such as translation time and translation blocks the
>  details are opaque to plugins. The plugin is able to query select
>  details of instructions and system configuration only through the
> -exported *qemu_plugin* functions. The types used to describe
> -instructions and events are opaque to the plugins themselves.
> +exported *qemu_plugin* functions.
> +
> +Query Handle Lifetime
> +---------------------
> +
> +Each callback provides an opaque anonymous information handle which
> +can usually be further queried to find out information about a
> +translation, instruction or operation. The handles themselves are only
> +valid during the lifetime of the callback so it is important that any
> +information that is needed is extracted during the callback and saved
> +by the plugin.
>
>  Usage
>  =====
> --
> 2.20.1
>

Re: [PATCH v1 1/5] docs/devel: document query handle lifetimes

[Richard Henderson]

On 2/7/20 3:01 PM, Alex Bennée wrote:
> I forgot to document the lifetime of handles in the developer
> documentation. Do so now.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  docs/devel/tcg-plugins.rst | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)

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


r~

[PATCH v1 2/5] plugins/core: add missing break in cb_to_tcg_flags

[Alex Bennée]

From: "Emilio G. Cota" <cota@braap.org>

Reported-by: Robert Henry <robhenry@microsoft.com>
Signed-off-by: Emilio G. Cota <cota@braap.org>
Message-Id: <20200105072940.32204-1-cota@braap.org>
---
 plugins/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/plugins/core.c b/plugins/core.c
index 9e1b9e7a915..ed863011baf 100644
--- a/plugins/core.c
+++ b/plugins/core.c
@@ -286,6 +286,7 @@ static inline uint32_t cb_to_tcg_flags(enum qemu_plugin_cb_flags flags)
     switch (flags) {
     case QEMU_PLUGIN_CB_RW_REGS:
         ret = 0;
+        break;
     case QEMU_PLUGIN_CB_R_REGS:
         ret = TCG_CALL_NO_WG;
         break;
-- 
2.20.1

Re: [PATCH v1 2/5] plugins/core: add missing break in cb_to_tcg_flags

[Richard Henderson]

On 2/7/20 7:01 AM, Alex Bennée wrote:
> From: "Emilio G. Cota" <cota@braap.org>
> 
> Reported-by: Robert Henry <robhenry@microsoft.com>
> Signed-off-by: Emilio G. Cota <cota@braap.org>
> Message-Id: <20200105072940.32204-1-cota@braap.org>
> ---
>  plugins/core.c | 1 +
>  1 file changed, 1 insertion(+)

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


r~

[PATCH v1 3/5] tests/plugin: prevent uninitialized warning

[Alex Bennée]

From: Chen Qun <kuhn.chenqun@huawei.com>

According to the glibc function requirements, we need initialise
 the variable. Otherwise there will be compilation warnings:

glib-autocleanups.h:28:3: warning: ‘out’ may be
used uninitialized in this function [-Wmaybe-uninitialized]
   g_free (*pp);
   ^~~~~~~~~~~~

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Message-Id: <20200206093238.203984-1-kuhn.chenqun@huawei.com>
[AJB: uses Thomas's single line allocation]
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/plugin/bb.c   | 6 +++---
 tests/plugin/insn.c | 3 +--
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/tests/plugin/bb.c b/tests/plugin/bb.c
index f30bea08dcc..df19fd359df 100644
--- a/tests/plugin/bb.c
+++ b/tests/plugin/bb.c
@@ -22,9 +22,9 @@ static bool do_inline;
 
 static void plugin_exit(qemu_plugin_id_t id, void *p)
 {
-    g_autofree gchar *out;
-    out = g_strdup_printf("bb's: %" PRIu64", insns: %" PRIu64 "\n",
-                          bb_count, insn_count);
+    g_autofree gchar *out = g_strdup_printf(
+        "bb's: %" PRIu64", insns: %" PRIu64 "\n",
+        bb_count, insn_count);
     qemu_plugin_outs(out);
 }
 
diff --git a/tests/plugin/insn.c b/tests/plugin/insn.c
index 0a8f5a0000e..a9a6e412373 100644
--- a/tests/plugin/insn.c
+++ b/tests/plugin/insn.c
@@ -44,8 +44,7 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
 
 static void plugin_exit(qemu_plugin_id_t id, void *p)
 {
-    g_autofree gchar *out;
-    out = g_strdup_printf("insns: %" PRIu64 "\n", insn_count);
+    g_autofree gchar *out = g_strdup_printf("insns: %" PRIu64 "\n", insn_count);
     qemu_plugin_outs(out);
 }
 
-- 
2.20.1

Re: [PATCH v1 3/5] tests/plugin: prevent uninitialized warning

[Richard Henderson]

On 2/7/20 7:01 AM, Alex Bennée wrote:
> From: Chen Qun <kuhn.chenqun@huawei.com>
> 
> According to the glibc function requirements, we need initialise

s/glibc/glib/

>  the variable. Otherwise there will be compilation warnings:
> 
> glib-autocleanups.h:28:3: warning: ‘out’ may be
> used uninitialized in this function [-Wmaybe-uninitialized]
>    g_free (*pp);
>    ^~~~~~~~~~~~
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Message-Id: <20200206093238.203984-1-kuhn.chenqun@huawei.com>
> [AJB: uses Thomas's single line allocation]
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  tests/plugin/bb.c   | 6 +++---
>  tests/plugin/insn.c | 3 +--
>  2 files changed, 4 insertions(+), 5 deletions(-)

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


r~

[PATCH v1 4/5] target/riscv: progressively load the instruction during decode

[Alex Bennée]

The plugin system would throw up a harmless warning when it detected
that a disassembly of an instruction didn't use all it's bytes. Fix
the riscv decoder to only load the instruction bytes it needs as it
needs them.

This drops opcode from the ctx in favour if passing the appropriately
sized opcode down a few levels of the decode.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/riscv/translate.c | 39 ++++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 14dc71156be..99f2bcf177c 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -44,7 +44,6 @@ typedef struct DisasContext {
     /* pc_succ_insn points to the instruction following base.pc_next */
     target_ulong pc_succ_insn;
     target_ulong priv_ver;
-    uint32_t opcode;
     uint32_t mstatus_fs;
     uint32_t misa;
     uint32_t mem_idx;
@@ -492,45 +491,45 @@ static void gen_set_rm(DisasContext *ctx, int rm)
     tcg_temp_free_i32(t0);
 }
 
-static void decode_RV32_64C0(DisasContext *ctx)
+static void decode_RV32_64C0(DisasContext *ctx, uint16_t opcode)
 {
-    uint8_t funct3 = extract32(ctx->opcode, 13, 3);
-    uint8_t rd_rs2 = GET_C_RS2S(ctx->opcode);
-    uint8_t rs1s = GET_C_RS1S(ctx->opcode);
+    uint8_t funct3 = extract32(opcode, 13, 3);
+    uint8_t rd_rs2 = GET_C_RS2S(opcode);
+    uint8_t rs1s = GET_C_RS1S(opcode);
 
     switch (funct3) {
     case 3:
 #if defined(TARGET_RISCV64)
         /* C.LD(RV64/128) -> ld rd', offset[7:3](rs1')*/
         gen_load_c(ctx, OPC_RISC_LD, rd_rs2, rs1s,
-                 GET_C_LD_IMM(ctx->opcode));
+                 GET_C_LD_IMM(opcode));
 #else
         /* C.FLW (RV32) -> flw rd', offset[6:2](rs1')*/
         gen_fp_load(ctx, OPC_RISC_FLW, rd_rs2, rs1s,
-                    GET_C_LW_IMM(ctx->opcode));
+                    GET_C_LW_IMM(opcode));
 #endif
         break;
     case 7:
 #if defined(TARGET_RISCV64)
         /* C.SD (RV64/128) -> sd rs2', offset[7:3](rs1')*/
         gen_store_c(ctx, OPC_RISC_SD, rs1s, rd_rs2,
-                  GET_C_LD_IMM(ctx->opcode));
+                  GET_C_LD_IMM(opcode));
 #else
         /* C.FSW (RV32) -> fsw rs2', offset[6:2](rs1')*/
         gen_fp_store(ctx, OPC_RISC_FSW, rs1s, rd_rs2,
-                     GET_C_LW_IMM(ctx->opcode));
+                     GET_C_LW_IMM(opcode));
 #endif
         break;
     }
 }
 
-static void decode_RV32_64C(DisasContext *ctx)
+static void decode_RV32_64C(DisasContext *ctx, uint16_t opcode)
 {
-    uint8_t op = extract32(ctx->opcode, 0, 2);
+    uint8_t op = extract32(opcode, 0, 2);
 
     switch (op) {
     case 0:
-        decode_RV32_64C0(ctx);
+        decode_RV32_64C0(ctx, opcode);
         break;
     }
 }
@@ -709,22 +708,24 @@ static bool gen_shift(DisasContext *ctx, arg_r *a,
 /* Include the auto-generated decoder for 16 bit insn */
 #include "decode_insn16.inc.c"
 
-static void decode_opc(DisasContext *ctx)
+static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
 {
     /* check for compressed insn */
-    if (extract32(ctx->opcode, 0, 2) != 3) {
+    if (extract32(opcode, 0, 2) != 3) {
         if (!has_ext(ctx, RVC)) {
             gen_exception_illegal(ctx);
         } else {
             ctx->pc_succ_insn = ctx->base.pc_next + 2;
-            if (!decode_insn16(ctx, ctx->opcode)) {
+            if (!decode_insn16(ctx, opcode)) {
                 /* fall back to old decoder */
-                decode_RV32_64C(ctx);
+                decode_RV32_64C(ctx, opcode);
             }
         }
     } else {
+        uint32_t opcode32 = opcode;
+        opcode32 = deposit32(opcode32, 16, 16, translator_lduw(env, ctx->base.pc_next + 2));
         ctx->pc_succ_insn = ctx->base.pc_next + 4;
-        if (!decode_insn32(ctx, ctx->opcode)) {
+        if (!decode_insn32(ctx, opcode32)) {
             gen_exception_illegal(ctx);
         }
     }
@@ -776,9 +777,9 @@ static void riscv_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
 {
     DisasContext *ctx = container_of(dcbase, DisasContext, base);
     CPURISCVState *env = cpu->env_ptr;
+    uint16_t opcode16 = translator_lduw(env, ctx->base.pc_next);
 
-    ctx->opcode = translator_ldl(env, ctx->base.pc_next);
-    decode_opc(ctx);
+    decode_opc(env, ctx, opcode16);
     ctx->base.pc_next = ctx->pc_succ_insn;
 
     if (ctx->base.is_jmp == DISAS_NEXT) {
-- 
2.20.1

[PATCH v1 4/5] target/riscv: progressively load the instruction during decode

[Alex Bennée]

The plugin system would throw up a harmless warning when it detected
that a disassembly of an instruction didn't use all it's bytes. Fix
the riscv decoder to only load the instruction bytes it needs as it
needs them.

This drops opcode from the ctx in favour if passing the appropriately
sized opcode down a few levels of the decode.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/riscv/translate.c | 39 ++++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 14dc71156be..99f2bcf177c 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -44,7 +44,6 @@ typedef struct DisasContext {
     /* pc_succ_insn points to the instruction following base.pc_next */
     target_ulong pc_succ_insn;
     target_ulong priv_ver;
-    uint32_t opcode;
     uint32_t mstatus_fs;
     uint32_t misa;
     uint32_t mem_idx;
@@ -492,45 +491,45 @@ static void gen_set_rm(DisasContext *ctx, int rm)
     tcg_temp_free_i32(t0);
 }
 
-static void decode_RV32_64C0(DisasContext *ctx)
+static void decode_RV32_64C0(DisasContext *ctx, uint16_t opcode)
 {
-    uint8_t funct3 = extract32(ctx->opcode, 13, 3);
-    uint8_t rd_rs2 = GET_C_RS2S(ctx->opcode);
-    uint8_t rs1s = GET_C_RS1S(ctx->opcode);
+    uint8_t funct3 = extract32(opcode, 13, 3);
+    uint8_t rd_rs2 = GET_C_RS2S(opcode);
+    uint8_t rs1s = GET_C_RS1S(opcode);
 
     switch (funct3) {
     case 3:
 #if defined(TARGET_RISCV64)
         /* C.LD(RV64/128) -> ld rd', offset[7:3](rs1')*/
         gen_load_c(ctx, OPC_RISC_LD, rd_rs2, rs1s,
-                 GET_C_LD_IMM(ctx->opcode));
+                 GET_C_LD_IMM(opcode));
 #else
         /* C.FLW (RV32) -> flw rd', offset[6:2](rs1')*/
         gen_fp_load(ctx, OPC_RISC_FLW, rd_rs2, rs1s,
-                    GET_C_LW_IMM(ctx->opcode));
+                    GET_C_LW_IMM(opcode));
 #endif
         break;
     case 7:
 #if defined(TARGET_RISCV64)
         /* C.SD (RV64/128) -> sd rs2', offset[7:3](rs1')*/
         gen_store_c(ctx, OPC_RISC_SD, rs1s, rd_rs2,
-                  GET_C_LD_IMM(ctx->opcode));
+                  GET_C_LD_IMM(opcode));
 #else
         /* C.FSW (RV32) -> fsw rs2', offset[6:2](rs1')*/
         gen_fp_store(ctx, OPC_RISC_FSW, rs1s, rd_rs2,
-                     GET_C_LW_IMM(ctx->opcode));
+                     GET_C_LW_IMM(opcode));
 #endif
         break;
     }
 }
 
-static void decode_RV32_64C(DisasContext *ctx)
+static void decode_RV32_64C(DisasContext *ctx, uint16_t opcode)
 {
-    uint8_t op = extract32(ctx->opcode, 0, 2);
+    uint8_t op = extract32(opcode, 0, 2);
 
     switch (op) {
     case 0:
-        decode_RV32_64C0(ctx);
+        decode_RV32_64C0(ctx, opcode);
         break;
     }
 }
@@ -709,22 +708,24 @@ static bool gen_shift(DisasContext *ctx, arg_r *a,
 /* Include the auto-generated decoder for 16 bit insn */
 #include "decode_insn16.inc.c"
 
-static void decode_opc(DisasContext *ctx)
+static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
 {
     /* check for compressed insn */
-    if (extract32(ctx->opcode, 0, 2) != 3) {
+    if (extract32(opcode, 0, 2) != 3) {
         if (!has_ext(ctx, RVC)) {
             gen_exception_illegal(ctx);
         } else {
             ctx->pc_succ_insn = ctx->base.pc_next + 2;
-            if (!decode_insn16(ctx, ctx->opcode)) {
+            if (!decode_insn16(ctx, opcode)) {
                 /* fall back to old decoder */
-                decode_RV32_64C(ctx);
+                decode_RV32_64C(ctx, opcode);
             }
         }
     } else {
+        uint32_t opcode32 = opcode;
+        opcode32 = deposit32(opcode32, 16, 16, translator_lduw(env, ctx->base.pc_next + 2));
         ctx->pc_succ_insn = ctx->base.pc_next + 4;
-        if (!decode_insn32(ctx, ctx->opcode)) {
+        if (!decode_insn32(ctx, opcode32)) {
             gen_exception_illegal(ctx);
         }
     }
@@ -776,9 +777,9 @@ static void riscv_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
 {
     DisasContext *ctx = container_of(dcbase, DisasContext, base);
     CPURISCVState *env = cpu->env_ptr;
+    uint16_t opcode16 = translator_lduw(env, ctx->base.pc_next);
 
-    ctx->opcode = translator_ldl(env, ctx->base.pc_next);
-    decode_opc(ctx);
+    decode_opc(env, ctx, opcode16);
     ctx->base.pc_next = ctx->pc_succ_insn;
 
     if (ctx->base.is_jmp == DISAS_NEXT) {
-- 
2.20.1

Re: [PATCH v1 4/5] target/riscv: progressively load the instruction during decode

[Robert Foley]

Hi,
On Fri, 7 Feb 2020 at 10:01, Alex Bennée <alex.bennee@linaro.org> wrote:
> -static void decode_RV32_64C0(DisasContext *ctx)
> +static void decode_RV32_64C0(DisasContext *ctx, uint16_t opcode)
>  {
> -    uint8_t funct3 = extract32(ctx->opcode, 13, 3);
> -    uint8_t rd_rs2 = GET_C_RS2S(ctx->opcode);
> -    uint8_t rs1s = GET_C_RS1S(ctx->opcode);
> +    uint8_t funct3 = extract32(opcode, 13, 3);

We noticed that a uint16_t opcode is passed into this function and
then passed on to extract32().
This is a minor point, but the extract32() will validate the start and
length values passed in according to 32 bits, not 16 bits.
static inline uint32_t extract32(uint32_t value, int start, int length)
{
    assert(start >= 0 && length > 0 && length <= 32 - start);
Since we have an extract32() and extract64(), maybe we could add an
extract16() and use it here?

Thanks & Regards,
-Rob
> +    uint8_t rd_rs2 = GET_C_RS2S(opcode);
> +    uint8_t rs1s = GET_C_RS1S(opcode);
>
>      switch (funct3) {
>      case 3:
>  #if defined(TARGET_RISCV64)
>          /* C.LD(RV64/128) -> ld rd', offset[7:3](rs1')*/
>          gen_load_c(ctx, OPC_RISC_LD, rd_rs2, rs1s,
> -                 GET_C_LD_IMM(ctx->opcode));
> +                 GET_C_LD_IMM(opcode));
>  #else
>          /* C.FLW (RV32) -> flw rd', offset[6:2](rs1')*/
>          gen_fp_load(ctx, OPC_RISC_FLW, rd_rs2, rs1s,
> -                    GET_C_LW_IMM(ctx->opcode));
> +                    GET_C_LW_IMM(opcode));
>  #endif
>          break;
>      case 7:
>  #if defined(TARGET_RISCV64)
>          /* C.SD (RV64/128) -> sd rs2', offset[7:3](rs1')*/
>          gen_store_c(ctx, OPC_RISC_SD, rs1s, rd_rs2,
> -                  GET_C_LD_IMM(ctx->opcode));
> +                  GET_C_LD_IMM(opcode));
>  #else
>          /* C.FSW (RV32) -> fsw rs2', offset[6:2](rs1')*/
>          gen_fp_store(ctx, OPC_RISC_FSW, rs1s, rd_rs2,
> -                     GET_C_LW_IMM(ctx->opcode));
> +                     GET_C_LW_IMM(opcode));
>  #endif
>          break;
>      }
>  }
>
> -static void decode_RV32_64C(DisasContext *ctx)
> +static void decode_RV32_64C(DisasContext *ctx, uint16_t opcode)
>  {
> -    uint8_t op = extract32(ctx->opcode, 0, 2);
> +    uint8_t op = extract32(opcode, 0, 2);
>
>      switch (op) {
>      case 0:
> -        decode_RV32_64C0(ctx);
> +        decode_RV32_64C0(ctx, opcode);
>          break;
>      }
>  }
> @@ -709,22 +708,24 @@ static bool gen_shift(DisasContext *ctx, arg_r *a,
>  /* Include the auto-generated decoder for 16 bit insn */
>  #include "decode_insn16.inc.c"
>
> -static void decode_opc(DisasContext *ctx)
> +static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
>  {
>      /* check for compressed insn */
> -    if (extract32(ctx->opcode, 0, 2) != 3) {
> +    if (extract32(opcode, 0, 2) != 3) {
>          if (!has_ext(ctx, RVC)) {
>              gen_exception_illegal(ctx);
>          } else {
>              ctx->pc_succ_insn = ctx->base.pc_next + 2;
> -            if (!decode_insn16(ctx, ctx->opcode)) {
> +            if (!decode_insn16(ctx, opcode)) {
>                  /* fall back to old decoder */
> -                decode_RV32_64C(ctx);
> +                decode_RV32_64C(ctx, opcode);
>              }
>          }
>      } else {
> +        uint32_t opcode32 = opcode;
> +        opcode32 = deposit32(opcode32, 16, 16, translator_lduw(env, ctx->base.pc_next + 2));
>          ctx->pc_succ_insn = ctx->base.pc_next + 4;
> -        if (!decode_insn32(ctx, ctx->opcode)) {
> +        if (!decode_insn32(ctx, opcode32)) {
>              gen_exception_illegal(ctx);
>          }
>      }
> @@ -776,9 +777,9 @@ static void riscv_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
>  {
>      DisasContext *ctx = container_of(dcbase, DisasContext, base);
>      CPURISCVState *env = cpu->env_ptr;
> +    uint16_t opcode16 = translator_lduw(env, ctx->base.pc_next);
>
> -    ctx->opcode = translator_ldl(env, ctx->base.pc_next);
> -    decode_opc(ctx);
> +    decode_opc(env, ctx, opcode16);
>      ctx->base.pc_next = ctx->pc_succ_insn;
>
>      if (ctx->base.is_jmp == DISAS_NEXT) {
> --
> 2.20.1
>

Re: [PATCH v1 4/5] target/riscv: progressively load the instruction during decode

[Robert Foley]

Hi,
On Fri, 7 Feb 2020 at 10:01, Alex Bennée <alex.bennee@linaro.org> wrote:
> -static void decode_RV32_64C0(DisasContext *ctx)
> +static void decode_RV32_64C0(DisasContext *ctx, uint16_t opcode)
>  {
> -    uint8_t funct3 = extract32(ctx->opcode, 13, 3);
> -    uint8_t rd_rs2 = GET_C_RS2S(ctx->opcode);
> -    uint8_t rs1s = GET_C_RS1S(ctx->opcode);
> +    uint8_t funct3 = extract32(opcode, 13, 3);

We noticed that a uint16_t opcode is passed into this function and
then passed on to extract32().
This is a minor point, but the extract32() will validate the start and
length values passed in according to 32 bits, not 16 bits.
static inline uint32_t extract32(uint32_t value, int start, int length)
{
    assert(start >= 0 && length > 0 && length <= 32 - start);
Since we have an extract32() and extract64(), maybe we could add an
extract16() and use it here?

Thanks & Regards,
-Rob
> +    uint8_t rd_rs2 = GET_C_RS2S(opcode);
> +    uint8_t rs1s = GET_C_RS1S(opcode);
>
>      switch (funct3) {
>      case 3:
>  #if defined(TARGET_RISCV64)
>          /* C.LD(RV64/128) -> ld rd', offset[7:3](rs1')*/
>          gen_load_c(ctx, OPC_RISC_LD, rd_rs2, rs1s,
> -                 GET_C_LD_IMM(ctx->opcode));
> +                 GET_C_LD_IMM(opcode));
>  #else
>          /* C.FLW (RV32) -> flw rd', offset[6:2](rs1')*/
>          gen_fp_load(ctx, OPC_RISC_FLW, rd_rs2, rs1s,
> -                    GET_C_LW_IMM(ctx->opcode));
> +                    GET_C_LW_IMM(opcode));
>  #endif
>          break;
>      case 7:
>  #if defined(TARGET_RISCV64)
>          /* C.SD (RV64/128) -> sd rs2', offset[7:3](rs1')*/
>          gen_store_c(ctx, OPC_RISC_SD, rs1s, rd_rs2,
> -                  GET_C_LD_IMM(ctx->opcode));
> +                  GET_C_LD_IMM(opcode));
>  #else
>          /* C.FSW (RV32) -> fsw rs2', offset[6:2](rs1')*/
>          gen_fp_store(ctx, OPC_RISC_FSW, rs1s, rd_rs2,
> -                     GET_C_LW_IMM(ctx->opcode));
> +                     GET_C_LW_IMM(opcode));
>  #endif
>          break;
>      }
>  }
>
> -static void decode_RV32_64C(DisasContext *ctx)
> +static void decode_RV32_64C(DisasContext *ctx, uint16_t opcode)
>  {
> -    uint8_t op = extract32(ctx->opcode, 0, 2);
> +    uint8_t op = extract32(opcode, 0, 2);
>
>      switch (op) {
>      case 0:
> -        decode_RV32_64C0(ctx);
> +        decode_RV32_64C0(ctx, opcode);
>          break;
>      }
>  }
> @@ -709,22 +708,24 @@ static bool gen_shift(DisasContext *ctx, arg_r *a,
>  /* Include the auto-generated decoder for 16 bit insn */
>  #include "decode_insn16.inc.c"
>
> -static void decode_opc(DisasContext *ctx)
> +static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
>  {
>      /* check for compressed insn */
> -    if (extract32(ctx->opcode, 0, 2) != 3) {
> +    if (extract32(opcode, 0, 2) != 3) {
>          if (!has_ext(ctx, RVC)) {
>              gen_exception_illegal(ctx);
>          } else {
>              ctx->pc_succ_insn = ctx->base.pc_next + 2;
> -            if (!decode_insn16(ctx, ctx->opcode)) {
> +            if (!decode_insn16(ctx, opcode)) {
>                  /* fall back to old decoder */
> -                decode_RV32_64C(ctx);
> +                decode_RV32_64C(ctx, opcode);
>              }
>          }
>      } else {
> +        uint32_t opcode32 = opcode;
> +        opcode32 = deposit32(opcode32, 16, 16, translator_lduw(env, ctx->base.pc_next + 2));
>          ctx->pc_succ_insn = ctx->base.pc_next + 4;
> -        if (!decode_insn32(ctx, ctx->opcode)) {
> +        if (!decode_insn32(ctx, opcode32)) {
>              gen_exception_illegal(ctx);
>          }
>      }
> @@ -776,9 +777,9 @@ static void riscv_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
>  {
>      DisasContext *ctx = container_of(dcbase, DisasContext, base);
>      CPURISCVState *env = cpu->env_ptr;
> +    uint16_t opcode16 = translator_lduw(env, ctx->base.pc_next);
>
> -    ctx->opcode = translator_ldl(env, ctx->base.pc_next);
> -    decode_opc(ctx);
> +    decode_opc(env, ctx, opcode16);
>      ctx->base.pc_next = ctx->pc_succ_insn;
>
>      if (ctx->base.is_jmp == DISAS_NEXT) {
> --
> 2.20.1
>

Re: [PATCH v1 4/5] target/riscv: progressively load the instruction during decode

[Alex Bennée]

Robert Foley <robert.foley@linaro.org> writes:

> Hi,
> On Fri, 7 Feb 2020 at 10:01, Alex Bennée <alex.bennee@linaro.org> wrote:
>> -static void decode_RV32_64C0(DisasContext *ctx)
>> +static void decode_RV32_64C0(DisasContext *ctx, uint16_t opcode)
>>  {
>> -    uint8_t funct3 = extract32(ctx->opcode, 13, 3);
>> -    uint8_t rd_rs2 = GET_C_RS2S(ctx->opcode);
>> -    uint8_t rs1s = GET_C_RS1S(ctx->opcode);
>> +    uint8_t funct3 = extract32(opcode, 13, 3);
>
> We noticed that a uint16_t opcode is passed into this function and
> then passed on to extract32().
> This is a minor point, but the extract32() will validate the start and
> length values passed in according to 32 bits, not 16 bits.
> static inline uint32_t extract32(uint32_t value, int start, int length)
> {
>     assert(start >= 0 && length > 0 && length <= 32 - start);
> Since we have an extract32() and extract64(), maybe we could add an
> extract16() and use it here?

Yeah - I did consider if it would be worth adding a extract16 helper.
There are a fair number of places in the code base where uint16_t is
silently promoted to a uint32_t (and a couple where it is downcast).

I guess 16 bit instruction words are common enough we should support
them in the utils.

-- 
Alex Bennée

Re: [PATCH v1 4/5] target/riscv: progressively load the instruction during decode

[Alex Bennée]

Robert Foley <robert.foley@linaro.org> writes:

> Hi,
> On Fri, 7 Feb 2020 at 10:01, Alex Bennée <alex.bennee@linaro.org> wrote:
>> -static void decode_RV32_64C0(DisasContext *ctx)
>> +static void decode_RV32_64C0(DisasContext *ctx, uint16_t opcode)
>>  {
>> -    uint8_t funct3 = extract32(ctx->opcode, 13, 3);
>> -    uint8_t rd_rs2 = GET_C_RS2S(ctx->opcode);
>> -    uint8_t rs1s = GET_C_RS1S(ctx->opcode);
>> +    uint8_t funct3 = extract32(opcode, 13, 3);
>
> We noticed that a uint16_t opcode is passed into this function and
> then passed on to extract32().
> This is a minor point, but the extract32() will validate the start and
> length values passed in according to 32 bits, not 16 bits.
> static inline uint32_t extract32(uint32_t value, int start, int length)
> {
>     assert(start >= 0 && length > 0 && length <= 32 - start);
> Since we have an extract32() and extract64(), maybe we could add an
> extract16() and use it here?

Yeah - I did consider if it would be worth adding a extract16 helper.
There are a fair number of places in the code base where uint16_t is
silently promoted to a uint32_t (and a couple where it is downcast).

I guess 16 bit instruction words are common enough we should support
them in the utils.

-- 
Alex Bennée

Re: [PATCH v1 4/5] target/riscv: progressively load the instruction during decode

[Richard Henderson]

On 2/7/20 7:01 AM, Alex Bennée wrote:
> The plugin system would throw up a harmless warning when it detected
> that a disassembly of an instruction didn't use all it's bytes. Fix
> the riscv decoder to only load the instruction bytes it needs as it
> needs them.
> 
> This drops opcode from the ctx in favour if passing the appropriately
> sized opcode down a few levels of the decode.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  target/riscv/translate.c | 39 ++++++++++++++++++++-------------------
>  1 file changed, 20 insertions(+), 19 deletions(-)

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


r~

Re: [PATCH v1 4/5] target/riscv: progressively load the instruction during decode

[Richard Henderson]

On 2/7/20 7:01 AM, Alex Bennée wrote:
> The plugin system would throw up a harmless warning when it detected
> that a disassembly of an instruction didn't use all it's bytes. Fix
> the riscv decoder to only load the instruction bytes it needs as it
> needs them.
> 
> This drops opcode from the ctx in favour if passing the appropriately
> sized opcode down a few levels of the decode.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  target/riscv/translate.c | 39 ++++++++++++++++++++-------------------
>  1 file changed, 20 insertions(+), 19 deletions(-)

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


r~

[PATCH v1 5/5] tests/plugins: make howvec clean-up after itself.

[Alex Bennée]

TCG plugins are responsible for their own memory usage and although
the plugin_exit is tied to the end of execution in this case it is
still poor practice. Ensure we delete the hash table and related data
when we are done to be a good plugin citizen.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/plugin/howvec.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/tests/plugin/howvec.c b/tests/plugin/howvec.c
index 4ca555e1239..7b77403559d 100644
--- a/tests/plugin/howvec.c
+++ b/tests/plugin/howvec.c
@@ -163,6 +163,13 @@ static gint cmp_exec_count(gconstpointer a, gconstpointer b)
     return ea->count > eb->count ? -1 : 1;
 }
 
+static void free_record(gpointer data)
+{
+    InsnExecCount *rec = (InsnExecCount *) data;
+    g_free(rec->insn);
+    g_free(rec);
+}
+
 static void plugin_exit(qemu_plugin_id_t id, void *p)
 {
     g_autoptr(GString) report = g_string_new("Instruction Classes:\n");
@@ -213,12 +220,15 @@ static void plugin_exit(qemu_plugin_id_t id, void *p)
         g_list_free(it);
     }
 
+    g_list_free(counts);
+    g_hash_table_destroy(insns);
+
     qemu_plugin_outs(report->str);
 }
 
 static void plugin_init(void)
 {
-    insns = g_hash_table_new(NULL, g_direct_equal);
+    insns = g_hash_table_new_full(NULL, g_direct_equal, NULL, &free_record);
 }
 
 static void vcpu_insn_exec_before(unsigned int cpu_index, void *udata)
-- 
2.20.1

Re: [PATCH v1 5/5] tests/plugins: make howvec clean-up after itself.

[Robert Foley]

On Fri, 7 Feb 2020 at 10:01, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> TCG plugins are responsible for their own memory usage and although
> the plugin_exit is tied to the end of execution in this case it is
> still poor practice. Ensure we delete the hash table and related data
> when we are done to be a good plugin citizen.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  tests/plugin/howvec.c | 12 +++++++++++-
>  static void plugin_exit(qemu_plugin_id_t id, void *p)
>  {
>      g_autoptr(GString) report = g_string_new("Instruction Classes:\n");
> @@ -213,12 +220,15 @@ static void plugin_exit(qemu_plugin_id_t id, void *p)
>          g_list_free(it);
>      }
>
> +    g_list_free(counts);
> +    g_hash_table_destroy(insns);
> +

Just one minor comment.  Seems like there might be an option to use
g_autoptr(GList) for counts.

Reviewed-by: Robert Foley <robert.foley@linaro.org>

>      qemu_plugin_outs(report->str);
>  }
>
>  static void plugin_init(void)
>  {
> -    insns = g_hash_table_new(NULL, g_direct_equal);
> +    insns = g_hash_table_new_full(NULL, g_direct_equal, NULL, &free_record);
>  }
>
>  static void vcpu_insn_exec_before(unsigned int cpu_index, void *udata)
> --
> 2.20.1
>

Re: [PATCH v1 5/5] tests/plugins: make howvec clean-up after itself.

[Richard Henderson]

On 2/7/20 7:01 AM, Alex Bennée wrote:
> TCG plugins are responsible for their own memory usage and although
> the plugin_exit is tied to the end of execution in this case it is
> still poor practice. Ensure we delete the hash table and related data
> when we are done to be a good plugin citizen.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  tests/plugin/howvec.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)

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


r~

Re: [PATCH v1 0/5] plugins/next

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200207150118.23007-1-alex.bennee@linaro.org/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [PATCH  v1 0/5] plugins/next
Message-id: 20200207150118.23007-1-alex.bennee@linaro.org
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20200207150118.23007-1-alex.bennee@linaro.org -> patchew/20200207150118.23007-1-alex.bennee@linaro.org
Switched to a new branch 'test'
41b2c3d tests/plugins: make howvec clean-up after itself.
107cac9 target/riscv: progressively load the instruction during decode
f4d9c2a tests/plugin: prevent uninitialized warning
f0c6141 plugins/core: add missing break in cb_to_tcg_flags
1b7fce4 docs/devel: document query handle lifetimes

=== OUTPUT BEGIN ===
1/5 Checking commit 1b7fce4dfde8 (docs/devel: document query handle lifetimes)
2/5 Checking commit f0c614169421 (plugins/core: add missing break in cb_to_tcg_flags)
3/5 Checking commit f4d9c2a8daaa (tests/plugin: prevent uninitialized warning)
4/5 Checking commit 107cac90cc74 (target/riscv: progressively load the instruction during decode)
ERROR: line over 90 characters
#110: FILE: target/riscv/translate.c:726:
+        opcode32 = deposit32(opcode32, 16, 16, translator_lduw(env, ctx->base.pc_next + 2));

total: 1 errors, 0 warnings, 103 lines checked

Patch 4/5 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

5/5 Checking commit 41b2c3dc8d77 (tests/plugins: make howvec clean-up after itself.)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200207150118.23007-1-alex.bennee@linaro.org/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v1 0/5] plugins/next

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200207150118.23007-1-alex.bennee@linaro.org/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

-...........................................................................................
+........................................................................E..................
+======================================================================
+ERROR: test_pause (__main__.TestSingleDriveUnalignedLength)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "041", line 106, in test_pause
---
 Ran 91 tests
 
-OK
+FAILED (errors=1)
  TEST    iotest-qcow2: 042
  TEST    iotest-qcow2: 043
  TEST    iotest-qcow2: 046
---
  TEST    iotest-qcow2: 283
Failures: 041
Failed 1 of 116 iotests
make: *** [check-tests/check-block.sh] Error 1
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 664, in <module>
    sys.exit(main())
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=44d027b6d11441338b7aff5d8515d6f2', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-82gd494d/src/docker-src.2020-02-07-16.16.22.27117:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=44d027b6d11441338b7aff5d8515d6f2
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-82gd494d/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    11m31.386s
user    0m8.696s


The full log is available at
http://patchew.org/logs/20200207150118.23007-1-alex.bennee@linaro.org/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com