[qemu]: How to use qemu to run 64MB bmc image?

[www]

Hi Joel Stanley, Andrew Jeffery, Cédric Le Goater,


How to modify it so that QEMU can run 64MB BMC image?
In addition, how to learn the source code of QEMU? Are there any guidelines and 
reference documents?


thanks，
Byron

Re: [PATCH v2 7/7] target/riscv: check before allocating TCG temps

[Chih-Min Chao]

On Fri, Jul 24, 2020 at 8:32 AM Richard Henderson <
richard.hender...@linaro.org> wrote:

> From: LIU Zhiwei 
>
> Signed-off-by: LIU Zhiwei 
> Message-Id: <20200626205917.4545-5-zhiwei_...@c-sky.com>
> Signed-off-by: Richard Henderson 
> ---
>  target/riscv/insn_trans/trans_rvd.inc.c | 8 
>  target/riscv/insn_trans/trans_rvf.inc.c | 8 
>  2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/target/riscv/insn_trans/trans_rvd.inc.c
> b/target/riscv/insn_trans/trans_rvd.inc.c
> index ea1044f13b..4f832637fa 100644
> --- a/target/riscv/insn_trans/trans_rvd.inc.c
> +++ b/target/riscv/insn_trans/trans_rvd.inc.c
> @@ -20,10 +20,10 @@
>
>  static bool trans_fld(DisasContext *ctx, arg_fld *a)
>  {
> -TCGv t0 = tcg_temp_new();
> -gen_get_gpr(t0, a->rs1);
>  REQUIRE_FPU;
>  REQUIRE_EXT(ctx, RVD);
> +TCGv t0 = tcg_temp_new();
> +gen_get_gpr(t0, a->rs1);
>  tcg_gen_addi_tl(t0, t0, a->imm);
>
>  tcg_gen_qemu_ld_i64(cpu_fpr[a->rd], t0, ctx->mem_idx, MO_TEQ);
> @@ -35,10 +35,10 @@ static bool trans_fld(DisasContext *ctx, arg_fld *a)
>
>  static bool trans_fsd(DisasContext *ctx, arg_fsd *a)
>  {
> -TCGv t0 = tcg_temp_new();
> -gen_get_gpr(t0, a->rs1);
>  REQUIRE_FPU;
>  REQUIRE_EXT(ctx, RVD);
> +TCGv t0 = tcg_temp_new();
> +gen_get_gpr(t0, a->rs1);
>  tcg_gen_addi_tl(t0, t0, a->imm);
>
>  tcg_gen_qemu_st_i64(cpu_fpr[a->rs2], t0, ctx->mem_idx, MO_TEQ);
> diff --git a/target/riscv/insn_trans/trans_rvf.inc.c
> b/target/riscv/insn_trans/trans_rvf.inc.c
> index 0d04677a02..16df9c5ee2 100644
> --- a/target/riscv/insn_trans/trans_rvf.inc.c
> +++ b/target/riscv/insn_trans/trans_rvf.inc.c
> @@ -25,10 +25,10 @@
>
>  static bool trans_flw(DisasContext *ctx, arg_flw *a)
>  {
> -TCGv t0 = tcg_temp_new();
> -gen_get_gpr(t0, a->rs1);
>  REQUIRE_FPU;
>  REQUIRE_EXT(ctx, RVF);
> +TCGv t0 = tcg_temp_new();
> +gen_get_gpr(t0, a->rs1);
>  tcg_gen_addi_tl(t0, t0, a->imm);
>
>  tcg_gen_qemu_ld_i64(cpu_fpr[a->rd], t0, ctx->mem_idx, MO_TEUL);
> @@ -41,11 +41,11 @@ static bool trans_flw(DisasContext *ctx, arg_flw *a)
>
>  static bool trans_fsw(DisasContext *ctx, arg_fsw *a)
>  {
> +REQUIRE_FPU;
> +REQUIRE_EXT(ctx, RVF);
>  TCGv t0 = tcg_temp_new();
>  gen_get_gpr(t0, a->rs1);
>
> -REQUIRE_FPU;
> -REQUIRE_EXT(ctx, RVF);
>  tcg_gen_addi_tl(t0, t0, a->imm);
>
>  tcg_gen_qemu_st_i64(cpu_fpr[a->rs2], t0, ctx->mem_idx, MO_TEUL);
> --
> 2.25.1
>
>
>
Reviewed-by: Chih-Min Chao 

Chih-Min Chao

Re: [PATCH v2 6/7] target/riscv: Clean up fmv.w.x

[Chih-Min Chao]

On Fri, Jul 24, 2020 at 8:28 AM Richard Henderson <
richard.hender...@linaro.org> wrote:

> From: LIU Zhiwei 
>
> Use tcg_gen_extu_tl_i64 to avoid the ifdef.
>
> Signed-off-by: LIU Zhiwei 
> Message-Id: <20200626205917.4545-7-zhiwei_...@c-sky.com>
> Signed-off-by: Richard Henderson 
> ---
>  target/riscv/insn_trans/trans_rvf.inc.c | 6 +-
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/target/riscv/insn_trans/trans_rvf.inc.c
> b/target/riscv/insn_trans/trans_rvf.inc.c
> index f9a9e0643a..0d04677a02 100644
> --- a/target/riscv/insn_trans/trans_rvf.inc.c
> +++ b/target/riscv/insn_trans/trans_rvf.inc.c
> @@ -406,11 +406,7 @@ static bool trans_fmv_w_x(DisasContext *ctx,
> arg_fmv_w_x *a)
>  TCGv t0 = tcg_temp_new();
>  gen_get_gpr(t0, a->rs1);
>
> -#if defined(TARGET_RISCV64)
> -tcg_gen_mov_i64(cpu_fpr[a->rd], t0);
> -#else
> -tcg_gen_extu_i32_i64(cpu_fpr[a->rd], t0);
> -#endif
> +tcg_gen_extu_tl_i64(cpu_fpr[a->rd], t0);
>  gen_nanbox_s(cpu_fpr[a->rd], cpu_fpr[a->rd]);
>
>  mark_fs_dirty(ctx);
> --
> 2.25.1
>
>
>
Reviewed-by: Chih-Min Chao 

Chih-Min Chao

Re: [PATCH v2 5/7] target/riscv: Check nanboxed inputs in trans_rvf.inc.c

[Chih-Min Chao]

On Fri, Jul 24, 2020 at 8:28 AM Richard Henderson <
richard.hender...@linaro.org> wrote:

> If a 32-bit input is not properly nanboxed, then the input is replaced
> with the default qnan.  The only inline expansion is for the sign-changing
> set of instructions: FSGNJ.S, FSGNJX.S, FSGNJN.S.
>
> Signed-off-by: Richard Henderson 
> ---
>  target/riscv/insn_trans/trans_rvf.inc.c | 71 +++--
>  target/riscv/translate.c| 18 +++
>  2 files changed, 73 insertions(+), 16 deletions(-)
>
> diff --git a/target/riscv/insn_trans/trans_rvf.inc.c
> b/target/riscv/insn_trans/trans_rvf.inc.c
> index 264d3139f1..f9a9e0643a 100644
> --- a/target/riscv/insn_trans/trans_rvf.inc.c
> +++ b/target/riscv/insn_trans/trans_rvf.inc.c
> @@ -161,47 +161,86 @@ static bool trans_fsgnj_s(DisasContext *ctx,
> arg_fsgnj_s *a)
>  {
>  REQUIRE_FPU;
>  REQUIRE_EXT(ctx, RVF);
> +
>  if (a->rs1 == a->rs2) { /* FMOV */
> -tcg_gen_mov_i64(cpu_fpr[a->rd], cpu_fpr[a->rs1]);
> +gen_check_nanbox_s(cpu_fpr[a->rd], cpu_fpr[a->rs1]);
>  } else { /* FSGNJ */
> -tcg_gen_deposit_i64(cpu_fpr[a->rd], cpu_fpr[a->rs2],
> cpu_fpr[a->rs1],
> -0, 31);
> +TCGv_i64 rs1 = tcg_temp_new_i64();
> +TCGv_i64 rs2 = tcg_temp_new_i64();
> +
> +gen_check_nanbox_s(rs1, cpu_fpr[a->rs1]);
> +gen_check_nanbox_s(rs2, cpu_fpr[a->rs2]);
> +
> +/* This formulation retains the nanboxing of rs2. */
> +tcg_gen_deposit_i64(cpu_fpr[a->rd], rs2, rs1, 0, 31);
> +tcg_temp_free_i64(rs1);
> +tcg_temp_free_i64(rs2);
>  }
> -gen_nanbox_s(cpu_fpr[a->rd], cpu_fpr[a->rd]);
>  mark_fs_dirty(ctx);
>  return true;
>  }
>
>  static bool trans_fsgnjn_s(DisasContext *ctx, arg_fsgnjn_s *a)
>  {
> +TCGv_i64 rs1, rs2, mask;
> +
>  REQUIRE_FPU;
>  REQUIRE_EXT(ctx, RVF);
> +
> +rs1 = tcg_temp_new_i64();
> +gen_check_nanbox_s(rs1, cpu_fpr[a->rs1]);
> +
>  if (a->rs1 == a->rs2) { /* FNEG */
> -tcg_gen_xori_i64(cpu_fpr[a->rd], cpu_fpr[a->rs1], INT32_MIN);
> +tcg_gen_xori_i64(cpu_fpr[a->rd], rs1, MAKE_64BIT_MASK(31, 1));
>  } else {
> -TCGv_i64 t0 = tcg_temp_new_i64();
> -tcg_gen_not_i64(t0, cpu_fpr[a->rs2]);
> -tcg_gen_deposit_i64(cpu_fpr[a->rd], t0, cpu_fpr[a->rs1], 0, 31);
> -tcg_temp_free_i64(t0);
> +rs2 = tcg_temp_new_i64();
> +gen_check_nanbox_s(rs2, cpu_fpr[a->rs2]);
> +
> +/*
> + * Replace bit 31 in rs1 with inverse in rs2.
> + * This formulation retains the nanboxing of rs1.
> + */
> +mask = tcg_const_i64(~MAKE_64BIT_MASK(31, 1));
> +tcg_gen_andc_i64(rs2, mask, rs2);
> +tcg_gen_and_i64(rs1, mask, rs1);
> +tcg_gen_or_i64(cpu_fpr[a->rd], rs1, rs2);
> +
> +tcg_temp_free_i64(mask);
> +tcg_temp_free_i64(rs2);
>  }
> -gen_nanbox_s(cpu_fpr[a->rd], cpu_fpr[a->rd]);
> +tcg_temp_free_i64(rs1);
> +
>  mark_fs_dirty(ctx);
>  return true;
>  }
>
>  static bool trans_fsgnjx_s(DisasContext *ctx, arg_fsgnjx_s *a)
>  {
> +TCGv_i64 rs1, rs2;
> +
>  REQUIRE_FPU;
>  REQUIRE_EXT(ctx, RVF);
> +
> +rs1 = tcg_temp_new_i64();
> +gen_check_nanbox_s(rs1, cpu_fpr[a->rs1]);
> +
>  if (a->rs1 == a->rs2) { /* FABS */
> -tcg_gen_andi_i64(cpu_fpr[a->rd], cpu_fpr[a->rs1], ~INT32_MIN);
> +tcg_gen_andi_i64(cpu_fpr[a->rd], rs1, ~MAKE_64BIT_MASK(31, 1));
>  } else {
> -TCGv_i64 t0 = tcg_temp_new_i64();
> -tcg_gen_andi_i64(t0, cpu_fpr[a->rs2], INT32_MIN);
> -tcg_gen_xor_i64(cpu_fpr[a->rd], cpu_fpr[a->rs1], t0);
> -tcg_temp_free_i64(t0);
> +rs2 = tcg_temp_new_i64();
> +gen_check_nanbox_s(rs2, cpu_fpr[a->rs2]);
> +
> +/*
> + * Xor bit 31 in rs1 with that in rs2.
> + * This formulation retains the nanboxing of rs1.
> + */
> +tcg_gen_andi_i64(rs2, rs2, MAKE_64BIT_MASK(31, 1));
> +tcg_gen_xor_i64(cpu_fpr[a->rd], rs1, rs2);
> +
> +tcg_temp_free_i64(rs2);
>  }
> -gen_nanbox_s(cpu_fpr[a->rd], cpu_fpr[a->rd]);
> +tcg_temp_free_i64(rs1);
> +
>  mark_fs_dirty(ctx);
>  return true;
>  }
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index 12a746da97..bf35182776 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -101,6 +101,24 @@ static void gen_nanbox_s(TCGv_i64 out, TCGv_i64 in)
>  tcg_gen_ori_i64(out, in, MAKE_64BIT_MASK(32, 32));
>  }
>
> +/*
> + * A narrow n-bit operation, where n < FLEN, checks that input operands
> + * are correctly Nan-boxed, i.e., all upper FLEN - n bits are 1.
> + * If so, the least-significant bits of the input are used, otherwise the
> + * input value is treated as an n-bit canonical NaN (v2.2 section 9.2).
> + *
> + * Here, the result is always nan-boxed, even the canonical nan.
> + */
> +static void gen_check_nanbox_s(TCGv_i64 out, TCGv

Re: [PATCH v2 4/7] target/riscv: Check nanboxed inputs to fp helpers

[Chih-Min Chao]

On Fri, Jul 24, 2020 at 8:29 AM Richard Henderson <
richard.hender...@linaro.org> wrote:

> If a 32-bit input is not properly nanboxed, then the input is
> replaced with the default qnan.
>
> Signed-off-by: Richard Henderson 
> ---
>  target/riscv/internals.h  | 11 +++
>  target/riscv/fpu_helper.c | 64 ---
>  2 files changed, 57 insertions(+), 18 deletions(-)
>
> diff --git a/target/riscv/internals.h b/target/riscv/internals.h
> index 9f4ba7d617..f1a546dba6 100644
> --- a/target/riscv/internals.h
> +++ b/target/riscv/internals.h
> @@ -43,4 +43,15 @@ static inline uint64_t nanbox_s(float32 f)
>  return f | MAKE_64BIT_MASK(32, 32);
>  }
>
> +static inline float32 check_nanbox_s(uint64_t f)
> +{
> +uint64_t mask = MAKE_64BIT_MASK(32, 32);
> +
> +if (likely((f & mask) == mask)) {
> +return (uint32_t)f;
> +} else {
> +return 0x7fc0u; /* default qnan */
> +}
> +}
> +
>  #endif
> diff --git a/target/riscv/fpu_helper.c b/target/riscv/fpu_helper.c
> index 72541958a7..bb346a8249 100644
> --- a/target/riscv/fpu_helper.c
> +++ b/target/riscv/fpu_helper.c
> @@ -81,9 +81,12 @@ void helper_set_rounding_mode(CPURISCVState *env,
> uint32_t rm)
>  set_float_rounding_mode(softrm, &env->fp_status);
>  }
>
> -static uint64_t do_fmadd_s(CPURISCVState *env, uint64_t frs1, uint64_t
> frs2,
> -   uint64_t frs3, int flags)
> +static uint64_t do_fmadd_s(CPURISCVState *env, uint64_t rs1, uint64_t rs2,
> +   uint64_t rs3, int flags)
>  {
> +float32 frs1 = check_nanbox_s(rs1);
> +float32 frs2 = check_nanbox_s(rs2);
> +float32 frs3 = check_nanbox_s(rs3);
>  return nanbox_s(float32_muladd(frs1, frs2, frs3, flags,
> &env->fp_status));
>  }
>
> @@ -139,74 +142,97 @@ uint64_t helper_fnmadd_d(CPURISCVState *env,
> uint64_t frs1, uint64_t frs2,
>float_muladd_negate_product, &env->fp_status);
>  }
>
> -uint64_t helper_fadd_s(CPURISCVState *env, uint64_t frs1, uint64_t frs2)
> +uint64_t helper_fadd_s(CPURISCVState *env, uint64_t rs1, uint64_t rs2)
>  {
> +float32 frs1 = check_nanbox_s(rs1);
> +float32 frs2 = check_nanbox_s(rs2);
>  return nanbox_s(float32_add(frs1, frs2, &env->fp_status));
>  }
>
> -uint64_t helper_fsub_s(CPURISCVState *env, uint64_t frs1, uint64_t frs2)
> +uint64_t helper_fsub_s(CPURISCVState *env, uint64_t rs1, uint64_t rs2)
>  {
> +float32 frs1 = check_nanbox_s(rs1);
> +float32 frs2 = check_nanbox_s(rs2);
>  return nanbox_s(float32_sub(frs1, frs2, &env->fp_status));
>  }
>
> -uint64_t helper_fmul_s(CPURISCVState *env, uint64_t frs1, uint64_t frs2)
> +uint64_t helper_fmul_s(CPURISCVState *env, uint64_t rs1, uint64_t rs2)
>  {
> +float32 frs1 = check_nanbox_s(rs1);
> +float32 frs2 = check_nanbox_s(rs2);
>  return nanbox_s(float32_mul(frs1, frs2, &env->fp_status));
>  }
>
> -uint64_t helper_fdiv_s(CPURISCVState *env, uint64_t frs1, uint64_t frs2)
> +uint64_t helper_fdiv_s(CPURISCVState *env, uint64_t rs1, uint64_t rs2)
>  {
> +float32 frs1 = check_nanbox_s(rs1);
> +float32 frs2 = check_nanbox_s(rs2);
>  return nanbox_s(float32_div(frs1, frs2, &env->fp_status));
>  }
>
> -uint64_t helper_fmin_s(CPURISCVState *env, uint64_t frs1, uint64_t frs2)
> +uint64_t helper_fmin_s(CPURISCVState *env, uint64_t rs1, uint64_t rs2)
>  {
> +float32 frs1 = check_nanbox_s(rs1);
> +float32 frs2 = check_nanbox_s(rs2);
>  return nanbox_s(float32_minnum(frs1, frs2, &env->fp_status));
>  }
>
> -uint64_t helper_fmax_s(CPURISCVState *env, uint64_t frs1, uint64_t frs2)
> +uint64_t helper_fmax_s(CPURISCVState *env, uint64_t rs1, uint64_t rs2)
>  {
> +float32 frs1 = check_nanbox_s(rs1);
> +float32 frs2 = check_nanbox_s(rs2);
>  return nanbox_s(float32_maxnum(frs1, frs2, &env->fp_status));
>  }
>
> -uint64_t helper_fsqrt_s(CPURISCVState *env, uint64_t frs1)
> +uint64_t helper_fsqrt_s(CPURISCVState *env, uint64_t rs1)
>  {
> +float32 frs1 = check_nanbox_s(rs1);
>  return nanbox_s(float32_sqrt(frs1, &env->fp_status));
>  }
>
> -target_ulong helper_fle_s(CPURISCVState *env, uint64_t frs1, uint64_t
> frs2)
> +target_ulong helper_fle_s(CPURISCVState *env, uint64_t rs1, uint64_t rs2)
>  {
> +float32 frs1 = check_nanbox_s(rs1);
> +float32 frs2 = check_nanbox_s(rs2);
>  return float32_le(frs1, frs2, &env->fp_status);
>  }
>
> -target_ulong helper_flt_s(CPURISCVState *env, uint64_t frs1, uint64_t
> frs2)
> +target_ulong helper_flt_s(CPURISCVState *env, uint64_t rs1, uint64_t rs2)
>  {
> +float32 frs1 = check_nanbox_s(rs1);
> +float32 frs2 = check_nanbox_s(rs2);
>  return float32_lt(frs1, frs2, &env->fp_status);
>  }
>
> -target_ulong helper_feq_s(CPURISCVState *env, uint64_t frs1, uint64_t
> frs2)
> +target_ulong helper_feq_s(CPURISCVState *env, uint64_t rs1, uint64_t rs2)
>  {
> +float32 frs1 = check_nanbox_s(rs1);
> +float32 frs2 = check_nanbox_s(rs2);
>  return float32_eq_quiet

Re: [PATCH v2 3/7] target/riscv: Generate nanboxed results from trans_rvf.inc.c

[Chih-Min Chao]

On Fri, Jul 24, 2020 at 8:28 AM Richard Henderson <
richard.hender...@linaro.org> wrote:

> Make sure that all results from inline single-precision scalar
> operations are properly nan-boxed to 64-bits.
>
> Signed-off-by: Richard Henderson 
> ---
>  target/riscv/insn_trans/trans_rvf.inc.c | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/target/riscv/insn_trans/trans_rvf.inc.c
> b/target/riscv/insn_trans/trans_rvf.inc.c
> index c7057482e8..264d3139f1 100644
> --- a/target/riscv/insn_trans/trans_rvf.inc.c
> +++ b/target/riscv/insn_trans/trans_rvf.inc.c
> @@ -167,6 +167,7 @@ static bool trans_fsgnj_s(DisasContext *ctx,
> arg_fsgnj_s *a)
>  tcg_gen_deposit_i64(cpu_fpr[a->rd], cpu_fpr[a->rs2],
> cpu_fpr[a->rs1],
>  0, 31);
>  }
> +gen_nanbox_s(cpu_fpr[a->rd], cpu_fpr[a->rd]);
>  mark_fs_dirty(ctx);
>  return true;
>  }
> @@ -183,6 +184,7 @@ static bool trans_fsgnjn_s(DisasContext *ctx,
> arg_fsgnjn_s *a)
>  tcg_gen_deposit_i64(cpu_fpr[a->rd], t0, cpu_fpr[a->rs1], 0, 31);
>  tcg_temp_free_i64(t0);
>  }
> +gen_nanbox_s(cpu_fpr[a->rd], cpu_fpr[a->rd]);
>  mark_fs_dirty(ctx);
>  return true;
>  }
> @@ -199,6 +201,7 @@ static bool trans_fsgnjx_s(DisasContext *ctx,
> arg_fsgnjx_s *a)
>  tcg_gen_xor_i64(cpu_fpr[a->rd], cpu_fpr[a->rs1], t0);
>  tcg_temp_free_i64(t0);
>  }
> +gen_nanbox_s(cpu_fpr[a->rd], cpu_fpr[a->rd]);
>  mark_fs_dirty(ctx);
>  return true;
>  }
> @@ -369,6 +372,7 @@ static bool trans_fmv_w_x(DisasContext *ctx,
> arg_fmv_w_x *a)
>  #else
>  tcg_gen_extu_i32_i64(cpu_fpr[a->rd], t0);
>  #endif
> +gen_nanbox_s(cpu_fpr[a->rd], cpu_fpr[a->rd]);
>
>  mark_fs_dirty(ctx);
>  tcg_temp_free(t0);
> --
> 2.25.1
>
>
>
Reviewed-by: Chih-Min Chao 

Chih-Min Chao

Re: [PATCH v2 2/7] target/riscv: Generalize gen_nanbox_fpr to gen_nanbox_s

[Chih-Min Chao]

On Fri, Jul 24, 2020 at 8:28 AM Richard Henderson <
richard.hender...@linaro.org> wrote:

> Do not depend on the RVD extension, take input and output via
> TCGv_i64 instead of fpu regno.  Move the function to translate.c
> so that it can be used in multiple trans_*.inc.c files.
>
> Signed-off-by: Richard Henderson 
> ---
>  target/riscv/insn_trans/trans_rvf.inc.c | 16 +---
>  target/riscv/translate.c| 11 +++
>  2 files changed, 12 insertions(+), 15 deletions(-)
>
> diff --git a/target/riscv/insn_trans/trans_rvf.inc.c
> b/target/riscv/insn_trans/trans_rvf.inc.c
> index 3bfd8881e7..c7057482e8 100644
> --- a/target/riscv/insn_trans/trans_rvf.inc.c
> +++ b/target/riscv/insn_trans/trans_rvf.inc.c
> @@ -23,20 +23,6 @@
>  return false;   \
>  } while (0)
>
> -/*
> - * RISC-V requires NaN-boxing of narrower width floating
> - * point values.  This applies when a 32-bit value is
> - * assigned to a 64-bit FP register.  Thus this does not
> - * apply when the RVD extension is not present.
> - */
> -static void gen_nanbox_fpr(DisasContext *ctx, int regno)
> -{
> -if (has_ext(ctx, RVD)) {
> -tcg_gen_ori_i64(cpu_fpr[regno], cpu_fpr[regno],
> -MAKE_64BIT_MASK(32, 32));
> -}
> -}
> -
>  static bool trans_flw(DisasContext *ctx, arg_flw *a)
>  {
>  TCGv t0 = tcg_temp_new();
> @@ -46,7 +32,7 @@ static bool trans_flw(DisasContext *ctx, arg_flw *a)
>  tcg_gen_addi_tl(t0, t0, a->imm);
>
>  tcg_gen_qemu_ld_i64(cpu_fpr[a->rd], t0, ctx->mem_idx, MO_TEUL);
> -gen_nanbox_fpr(ctx, a->rd);
> +gen_nanbox_s(cpu_fpr[a->rd], cpu_fpr[a->rd]);
>
>  tcg_temp_free(t0);
>  mark_fs_dirty(ctx);
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index 9632e79cf3..12a746da97 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -90,6 +90,17 @@ static inline bool has_ext(DisasContext *ctx, uint32_t
> ext)
>  return ctx->misa & ext;
>  }
>
> +/*
> + * RISC-V requires NaN-boxing of narrower width floating point values.
> + * This applies when a 32-bit value is assigned to a 64-bit FP register.
> + * For consistency and simplicity, we nanbox results even when the RVD
> + * extension is not present.
> + */
> +static void gen_nanbox_s(TCGv_i64 out, TCGv_i64 in)
> +{
> +tcg_gen_ori_i64(out, in, MAKE_64BIT_MASK(32, 32));
> +}
> +
>  static void generate_exception(DisasContext *ctx, int excp)
>  {
>  tcg_gen_movi_tl(cpu_pc, ctx->base.pc_next);
> --
> 2.25.1
>
>
>
Reviewed-by: Chih-Min Chao 

Re: [for-5.2 v4 10/10] s390: Recognize host-trust-limitation option

[David Gibson]

On Mon, Jul 27, 2020 at 05:50:40PM +0200, Cornelia Huck wrote:
> On Fri, 24 Jul 2020 12:57:44 +1000
> David Gibson  wrote:
> 
> > At least some s390 cpu models support "Protected Virtualization" (PV),
> > a mechanism to protect guests from eavesdropping by a compromised
> > hypervisor.
> > 
> > This is similar in function to other mechanisms like AMD's SEV and
> > POWER's PEF, which are controlled bythe "host-trust-limitation"
> > machine option.  s390 is a slightly special case, because we already
> > supported PV, simply by using a CPU model with the required feature
> > (S390_FEAT_UNPACK).
> > 
> > To integrate this with the option used by other platforms, we
> > implement the following compromise:
> > 
> >  - When the host-trust-limitation option is set, s390 will recognize
> >it, verify that the CPU can support PV (failing if not) and set
> >virtio default options necessary for encrypted or protected guests,
> >as on other platforms.  i.e. if host-trust-limitation is set, we
> >will either create a guest capable of entering PV mode, or fail
> >outright
> > 
> >  - If host-trust-limitation is not set, guest's might still be able to
> >enter PV mode, if the CPU has the right model.  This may be a
> >little surprising, but shouldn't actually be harmful.
> 
> This could be workable, I guess. Would like a second opinion, though.
> 
> > 
> > To start a guest supporting Protected Virtualization using the new
> > option use the command line arguments:
> > -object s390-pv-guest,id=pv0 -machine host-trust-limitation=pv0
> > 
> > Signed-off-by: David Gibson 
> > ---
> >  hw/s390x/pv.c | 61 +++
> >  1 file changed, 61 insertions(+)
> > 
> > diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
> > index ab3a2482aa..4bf3b345b6 100644
> > --- a/hw/s390x/pv.c
> > +++ b/hw/s390x/pv.c
> > @@ -14,8 +14,11 @@
> >  #include 
> >  
> >  #include "cpu.h"
> > +#include "qapi/error.h"
> >  #include "qemu/error-report.h"
> >  #include "sysemu/kvm.h"
> > +#include "qom/object_interfaces.h"
> > +#include "exec/host-trust-limitation.h"
> >  #include "hw/s390x/ipl.h"
> >  #include "hw/s390x/pv.h"
> >  
> > @@ -111,3 +114,61 @@ void s390_pv_inject_reset_error(CPUState *cs)
> >  /* Report that we are unable to enter protected mode */
> >  env->regs[r1 + 1] = DIAG_308_RC_INVAL_FOR_PV;
> >  }
> > +
> > +#define TYPE_S390_PV_GUEST "s390-pv-guest"
> > +#define S390_PV_GUEST(obj)  \
> > +OBJECT_CHECK(S390PVGuestState, (obj), TYPE_S390_PV_GUEST)
> > +
> > +typedef struct S390PVGuestState S390PVGuestState;
> > +
> > +/**
> > + * S390PVGuestState:
> > + *
> > + * The S390PVGuestState object is basically a dummy used to tell the
> > + * host trust limitation system to use s390's PV mechanism.  guest.
> > + *
> > + * # $QEMU \
> > + * -object s390-pv-guest,id=pv0 \
> > + * -machine ...,host-trust-limitation=pv0
> > + */
> > +struct S390PVGuestState {
> > +Object parent_obj;
> > +};
> > +
> > +static int s390_pv_kvm_init(HostTrustLimitation *gmpo, Error **errp)
> > +{
> > +if (!s390_has_feat(S390_FEAT_UNPACK)) {
> > +error_setg(errp,
> > +   "CPU model does not support Protected Virtualization");
> > +return -1;
> > +}
> > +
> > +return 0;
> > +}
> 
> So here's where I'm confused: If I follow the code correctly, the
> ->kvm_init callback is invoked before kvm_arch_init() is called. The
> kvm_arch_init() implementation for s390x checks whether
> KVM_CAP_S390_PROTECTED is available, which is a pre-req for
> S390_FEAT_UNPACK. Am I missing something? Can someone with access to PV
> hardware check whether this works as intended?

Ah, yes, I need to rethink this.  kvm_arch_init() happens
substantially earlier than I realized.  Plus the setup of s390 cpu
models is confusing to me, it seems to set up the model after the cpu
instance is created, rather than having cpu models correspond to cpu
classes and thus existing before the cpus are actually instantiated.

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH v2 1/7] target/riscv: Generate nanboxed results from fp helpers

[Chih-Min Chao]

On Fri, Jul 24, 2020 at 2:06 PM LIU Zhiwei  wrote:

>
>
> On 2020/7/24 11:55, Richard Henderson wrote:
> > On 7/23/20 7:35 PM, LIU Zhiwei wrote:
> >>
> >> On 2020/7/24 8:28, Richard Henderson wrote:
> >>> Make sure that all results from single-precision scalar helpers
> >>> are properly nan-boxed to 64-bits.
> >>>
> >>> Signed-off-by: Richard Henderson 
> >>> ---
> >>>target/riscv/internals.h  |  5 +
> >>>target/riscv/fpu_helper.c | 42
> +--
> >>>2 files changed, 28 insertions(+), 19 deletions(-)
> >>>
> >>> diff --git a/target/riscv/internals.h b/target/riscv/internals.h
> >>> index 37d33820ad..9f4ba7d617 100644
> >>> --- a/target/riscv/internals.h
> >>> +++ b/target/riscv/internals.h
> >>> @@ -38,4 +38,9 @@ target_ulong fclass_d(uint64_t frs1);
> >>>#define SEW32 2
> >>>#define SEW64 3
> >>>+static inline uint64_t nanbox_s(float32 f)
> >>> +{
> >>> +return f | MAKE_64BIT_MASK(32, 32);
> >>> +}
> >>> +
> >> If define it here,  we can also define a more general  function with
> flen.
> >>
> >> +static inline uint64_t nanbox_s(float32 f, uint32_t flen)
> >> +{
> >> +return f | MAKE_64BIT_MASK(flen, 64 - flen);
> >> +}
> >> +
> >>
> >> So we can reuse it in fp16 or bf16 scalar instruction and in vector
> instructions.
> > While we could do that, we will not encounter all possible lengths.  In
> the
> > cover letter, I mentioned defining a second function,
> >
> > static inline uint64_t nanbox_h(float16 f)
> > {
> > return f | MAKE_64BIT_MASK(16, 48);
> > }
> >
> > Having two separate functions will, I believe, be easier to use in
> practice.
> >
> Get  it. Thanks.
>
> Zhiwei
> >
> > r~
>
>
>
That is what has been implemented in spike.  It fills up the Nan-Box when
value is stored back internal structure and
unbox the value with difference floating type (half/single/double/quad).

By the way,  I prefer to keeping the suffix to tell different floating
type rather than pass arbitrary
since each floating type belong to each extension.

Reviewed-by: Chih-Min Chao 

Re: [PATCH v5 0/2] add new options to set smbios type 4 fields

[Michael S. Tsirkin]

On Thu, Aug 06, 2020 at 11:56:32AM +0800, Ying Fang wrote:
> From: fangying 
> 
> Hi, this patchset was previously posted by my teamate Heyi Guo several
> months ago, however we missed the merge window. It is reposted here to
> make it an end. Thanks.


Thanks, I will tag it for after the release.
Pls ping me after the release to make sure I don't drop it by mistake.

> Patch description:
>  
> Common VM users sometimes care about CPU speed, so we add two new
> options to allow VM vendors to present CPU speed to their users.
> Normally these information can be fetched from host smbios.
> 
> Strictly speaking, the "max speed" and "current speed" in type 4
> are not really for the max speed and current speed of processor, for
> "max speed" identifies a capability of the system, and "current speed"
> identifies the processor's speed at boot (see smbios spec), but some
> applications do not tell the differences.
> 
> Changelog:
> 
> v4 -> v5:
> - Rebase patch for lastest upstream
> 
> v3 -> v4:
> - Fix the default value when not specifying "-smbios type=4" option;
> it would be 0 instead of 2000 in previous versions
> - Use uint64_t type to check value overflow
> - Add test case to check smbios type 4 CPU speed
> - v4 https://patchwork.kernel.org/cover/11444635/
> 
> v2 -> v3:
> - Refine comments per Igor's suggestion.
> 
> v1 -> v2:
> - change "_" in option names to "-"
> - check if option value is too large to fit in SMBIOS type 4 speed
> fields.
> 
> Cc: "Michael S. Tsirkin" 
> Cc: Igor Mammedov 
> 
> Ying Fang (2):
>   hw/smbios: add options for type 4 max-speed and current-speed
>   tests/bios-tables-test: add smbios cpu speed test
> 
>  hw/smbios/smbios.c   | 36 ++
>  qemu-options.hx  |  2 +-
>  tests/bios-tables-test.c | 42 
>  3 files changed, 75 insertions(+), 5 deletions(-)
> 
> -- 
> 2.23.0

Re: [PATCH] ide: Get rid of IDEDrive struct

[Markus Armbruster]

Eduardo Habkost  writes:

> On Wed, Aug 05, 2020 at 09:41:25PM +0100, Peter Maydell wrote:
>> On Wed, 5 Aug 2020 at 20:49, Eduardo Habkost  wrote:
>> >
>> > The struct had a single field (IDEDevice dev), and is only used
>> > in the QOM type declarations and property lists.  We can simply
>> > use the IDEDevice struct directly instead.
>> >
>> > Signed-off-by: Eduardo Habkost 
>> > @@ -327,7 +323,6 @@ static void ide_hd_class_init(ObjectClass *klass, void 
>> > *data)
>> >  static const TypeInfo ide_hd_info = {
>> >  .name  = "ide-hd",
>> >  .parent= TYPE_IDE_DEVICE,
>> > -.instance_size = sizeof(IDEDrive),
>> >  .class_init= ide_hd_class_init,
>> >  };
>> 
>> This is one of those areas where this change works and reduces
>> amount of code, but on the other hand it means the QOM type
>> doesn't follow the common pattern for a leaf type of:
>>  * it has a struct
>>  * it has cast macros that cast to that struct
>>  * the typeinfo instance_size is the size of that struct
>> (it wasn't exactly following this pattern before, of course).
>
> Is this really a pattern that exists and we want to follow?
> I don't see why that pattern would be useful for simple leaf
> types.

I think the pattern exists, but we deviate from it in quite a few
places, probably just because it's so much boilerplate.

Related: Daniel's "[PATCH 0/4] qom: reduce boilerplate required for
declaring and defining objects".  Perhaps Daniel has an opinion on
taking shortcuts with leaf types.

> Also, in this case the code wasn't even following that pattern:
> it was using the same IDEDrive struct for all TYPE_IDE_DEVICE
> subtypes.

Rule of thumb: hw/ide/ is a bad example.  I don't mean to belittle the
efforts of quite a few people over the years.  It used to be worse.

>> We define in https://wiki.qemu.org/Documentation/QOMConventions
>> (in the 'When to create class types and macros' bit at the bottom)
>> what we expect for whether to provide class cast macros/a
>> class struct/class_size in the TypeInfo, essentially recommending
>> that types follow one of two patterns (simple leaf class with no
>> methods or class members, vs everything else) even if in a
>> particular case you could take a short-cut and not define
>> everything. We haven't really defined similar "this is the
>> standard pattern, provide it all even if you don't strictly
>> need it" rules for the instance struct/macros. Maybe we should?
>
> I think we should include the instance struct/macros in the
> recommendations there, but I would expect those recommendations
> to apply only to non-leaf types.

I'm fine with having a separate convention for leaf types if that helps,
but please let's have a convention.  I like my QOM boilerplate
uncreative.

>> Just a thought, not a nak; I know we have quite a number
>> of types that take this kind of "we don't really need to
>> provide all the standard QOM macros/structs/etc" approach
>> (some of which I wrote!).
>> 
>> thanks
>> -- PMM
>> 

Re: [PATCH] spapr: Clarify error and documentation for broken KVM XICS

[David Gibson]

On Wed, Aug 05, 2020 at 05:47:16PM +0200, Greg Kurz wrote:
> When starting an L2 KVM guest with `ic-mode=dual,kernel-irqchip=on`,
> QEMU fails with:
> 
> KVM is too old to support ic-mode=dual,kernel-irqchip=on
> 
> This error message was introduced to detect older KVM versions that
> didn't allow destruction and re-creation of the XICS KVM device that
> we do at reboot. But it is actually the same issue that we get with
> nested guests : when running under pseries, KVM currently provides
> a genuine XICS device (not the XICS-on-XIVE device that we get
> under powernv) which doesn't support destruction/re-creation.
> 
> This will eventually be fixed in KVM but in the meantime, update
> the error message and documentation to mention the nested case.
> While here, mention that in "No XIVE support in KVM" section that
> this can also happen with "guest OSes supporting XIVE" since
> we check this at init time before starting the guest.
> 
> Reported-by: Satheesh Rajendran 
> Buglink: https://bugs.launchpad.net/qemu/+bug/1890290
> Signed-off-by: Greg Kurz 

Applied to ppc-for-5.2.

> ---
>  docs/specs/ppc-spapr-xive.rst |5 -
>  hw/ppc/spapr_irq.c|   12 +---
>  2 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/docs/specs/ppc-spapr-xive.rst b/docs/specs/ppc-spapr-xive.rst
> index 7199db730b82..7144347560f1 100644
> --- a/docs/specs/ppc-spapr-xive.rst
> +++ b/docs/specs/ppc-spapr-xive.rst
> @@ -126,6 +126,9 @@ xicsXICS KVM   XICS emul. XICS KVM
>  
>  (1) QEMU warns with ``warning: kernel_irqchip requested but unavailable:
>  IRQ_XIVE capability must be present for KVM``
> +In some cases (old host kernels or KVM nested guests), one may hit a
> +QEMU/KVM incompatibility due to device destruction in reset. QEMU fails
> +with ``KVM is incompatible with ic-mode=dual,kernel-irqchip=on``
>  (2) QEMU fails with ``kernel_irqchip requested but unavailable:
>  IRQ_XIVE capability must be present for KVM``
>  
> @@ -148,7 +151,7 @@ xicsXICS KVM   XICS emul. XICS KVM
>  mode (XICS), either don't set the ic-mode machine property or try
>  ic-mode=xics or ic-mode=dual``
>  (4) QEMU/KVM incompatibility due to device destruction in reset. QEMU fails
> -with ``KVM is too old to support ic-mode=dual,kernel-irqchip=on``
> +with ``KVM is incompatible with ic-mode=dual,kernel-irqchip=on``
>  
>  
>  XIVE Device tree properties
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 2f8f7d62f875..72bb938375ef 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -139,6 +139,7 @@ SpaprIrq spapr_irq_dual = {
>  
>  static int spapr_irq_check(SpaprMachineState *spapr, Error **errp)
>  {
> +ERRP_GUARD();
>  MachineState *machine = MACHINE(spapr);
>  
>  /*
> @@ -179,14 +180,19 @@ static int spapr_irq_check(SpaprMachineState *spapr, 
> Error **errp)
>  
>  /*
>   * On a POWER9 host, some older KVM XICS devices cannot be destroyed and
> - * re-created. Detect that early to avoid QEMU to exit later when the
> - * guest reboots.
> + * re-created. Same happens with KVM nested guests. Detect that early to
> + * avoid QEMU to exit later when the guest reboots.
>   */
>  if (kvm_enabled() &&
>  spapr->irq == &spapr_irq_dual &&
>  kvm_kernel_irqchip_required() &&
>  xics_kvm_has_broken_disconnect(spapr)) {
> -error_setg(errp, "KVM is too old to support 
> ic-mode=dual,kernel-irqchip=on");
> +error_setg(errp,
> +"KVM is incompatible with ic-mode=dual,kernel-irqchip=on");
> +error_append_hint(errp,
> +"This can happen with an old KVM or in a KVM nested guest.\n");
> +error_append_hint(errp,
> +"Try without kernel-irqchip or with kernel-irqchip=off.\n");
>  return -1;
>  }
>  
> 
> 

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH v3 0/8] Generalize start-powered-off property from ARM

[David Gibson]

On Wed, Aug 05, 2020 at 08:04:11PM +0100, Peter Maydell wrote:
> On Wed, 5 Aug 2020 at 18:01, Thiago Jung Bauermann
>  wrote:
> > Any news on this? Is there something I should be doing? I saw -rc3 today
> > but not these patches.
> 
> Sorry, you've missed the bus for 5.1 at this point. I'd assumed
> that the relevant bits of the patchset would go into a PPC pullreq
> if it was important for 5.1.

Ah, bother.  Meanwhile I assumed that since it was cross-target it was
going in separately, so I didn't take it through my tree.

Oh well, I guess we apply ASAP in 5.2 and we'll do a backport
downstream.

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: Adding VHOST_USER_PROTOCOL_F_CONFIG_MEM_SLOTS to 5.1 release notes

[Michael S. Tsirkin]

A bit verbose. I shortened it to

A new feature, VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS, has been
added to the vhost-user protocol.
VMs with vhost-user device backends which support this feature will
not be subject to the current max RAM slots limit of 8 and will be able to
hot-add memory as many times as the target platform supports.

Peter, can you pls allow Raphael wiki access? I don't remember how it's done ...

On Wed, Jul 29, 2020 at 09:17:27PM -0600, Raphael Norwitz wrote:
> How about something like:
> "A new feature, VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS, has been
> added to the vhost-user protocol which, when negotiated, changes the
> way QEMU transmit memory regions to backend devices. Instead of
> sending all regions in a single VHOST_USER_SET_MEM_TABLE message, QEMU
> will send supporting backends individual VHOST_USER_ADD_MEM_REG and
> VHOST_USER_REM_MEM_REG messages to update the devices memory tables.
> VMs with vhost-user device backends which support this feature will
> not be subject to the max RAM slots limit of 8 and will be able to
> hot-add memory as many times as the target platform supports. Backends
> which do not support VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS are
> unaffected."
> 
> I don't have permission to edit the wiki. How can I get permission? Or
> can someone post it for me?
> 
> On Wed, Jul 29, 2020 at 8:19 AM Michael S. Tsirkin  wrote:
> >
> > On Tue, Jul 28, 2020 at 09:16:10PM -0600, Raphael Norwitz wrote:
> > > Hi mst,
> > >
> > > Looking at the current changelog
> > > https://wiki.qemu.org/ChangeLog/5.1#virtio, I don't see any mention of
> > > the VHOST_USER_PROTOCOL_F_CONFIG_MEM_SLOTS protocol feature. It is a
> > > user visible change so shouldn't we add a note?
> > >
> > > Thanks,
> > > Raphael
> >
> > I didn't look at updating the changelog yet.
> > Would be great if you could write up new vhost user things.
> >
> > --
> > MST
> >

Re: cleanups with long-term benefits

[Markus Armbruster]

Eduardo Habkost  writes:

> On Wed, Aug 05, 2020 at 06:23:23PM +0200, Kevin Wolf wrote:
>> We're basically weighing "git blame" against syntax highlighting
>> defaults. I don't think the latter has an obviously higher weight.
>
> I think "syntax highlight defaults" is far from being an accurate
> description of the motivations behind this discussion.

The motivation and the expected benefits are far from clear to me,
because all I have so far is a meandering e-mail thread.

For a change proposal as massive as "throw out working code and touch
basically every line in the QAPI schema", I'd like to see a concise memo
stating the goals, their benefits, the possible ways to get there, and
their cost.  I don't think that's too much to ask.

Re: [PATCH for-5.2 2/5] spapr/xive: Simplify kvmppc_xive_disconnect()

[David Gibson]

On Wed, Aug 05, 2020 at 07:35:29PM +0200, Greg Kurz wrote:
> Since this function begins with:
> 
> /* The KVM XIVE device is not in use */
> if (!xive || xive->fd == -1) {
> return;
> }
> 
> we obviously don't need to check xive->fd again.
> 
> Signed-off-by: Greg Kurz 

Applied to ppc-for-5.2.

> ---
>  hw/intc/spapr_xive_kvm.c |6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> index d55ea4670e0e..893a1ee77e70 100644
> --- a/hw/intc/spapr_xive_kvm.c
> +++ b/hw/intc/spapr_xive_kvm.c
> @@ -873,10 +873,8 @@ void kvmppc_xive_disconnect(SpaprInterruptController 
> *intc)
>   * and removed from the list of devices of the VM. The VCPU
>   * presenters are also detached from the device.
>   */
> -if (xive->fd != -1) {
> -close(xive->fd);
> -xive->fd = -1;
> -}
> +close(xive->fd);
> +xive->fd = -1;
>  
>  kvm_kernel_irqchip = false;
>  kvm_msi_via_irqfd_allowed = false;
> 
> 

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH for-5.2 1/5] spapr/xive: Fix xive->fd if kvm_create_device() fails

[David Gibson]

On Wed, Aug 05, 2020 at 07:35:22PM +0200, Greg Kurz wrote:
> If the creation of the KVM XIVE device fails for some reasons, the
> negative errno ends up in xive->fd, but the rest of the code assumes
> that xive->fd either contains an open fd, ie. positive value, or -1.
> 
> This doesn't cause any misbehavior except kvmppc_xive_disconnect()
> that will try to close(xive->fd) during rollback and likely be
> rewarded with an EBADF.
> 
> Only set xive->fd with a open fd.
> 
> Signed-off-by: Greg Kurz 

Applied to ppc-for-5.2.

> ---
>  hw/intc/spapr_xive_kvm.c |8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> index edb7ee0e74f1..d55ea4670e0e 100644
> --- a/hw/intc/spapr_xive_kvm.c
> +++ b/hw/intc/spapr_xive_kvm.c
> @@ -745,6 +745,7 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, 
> uint32_t nr_servers,
>  size_t esb_len = (1ull << xsrc->esb_shift) * xsrc->nr_irqs;
>  size_t tima_len = 4ull << TM_SHIFT;
>  CPUState *cs;
> +int fd;
>  
>  /*
>   * The KVM XIVE device already in use. This is the case when
> @@ -760,11 +761,12 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, 
> uint32_t nr_servers,
>  }
>  
>  /* First, create the KVM XIVE device */
> -xive->fd = kvm_create_device(kvm_state, KVM_DEV_TYPE_XIVE, false);
> -if (xive->fd < 0) {
> -error_setg_errno(errp, -xive->fd, "XIVE: error creating KVM device");
> +fd = kvm_create_device(kvm_state, KVM_DEV_TYPE_XIVE, false);
> +if (fd < 0) {
> +error_setg_errno(errp, -fd, "XIVE: error creating KVM device");
>  return -1;
>  }
> +xive->fd = fd;
>  
>  /* Tell KVM about the # of VCPUs we may have */
>  if (kvm_device_check_attr(xive->fd, KVM_DEV_XIVE_GRP_CTRL,
> 
> 

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH for-5.2 4/5] spapr/xive: Convert KVM device fd checks to assert()

[David Gibson]

On Wed, Aug 05, 2020 at 07:35:44PM +0200, Greg Kurz wrote:
> All callers guard these functions with kvmppc_xive_in_kernel() or one
> of its variants. Make it clear that these functions are only to be
> called when the KVM XIVE device is active.
> 
> Note that the check on xive is dropped in kvmppc_xive_disconnect(). It
> really cannot be NULL since it comes from set_active_intc() which only
> passes pointers to allocated objects.
> 
> Signed-off-by: Greg Kurz 

Reviewed-by: David Gibson 

> ---
>  hw/intc/spapr_xive_kvm.c |   35 +++
>  1 file changed, 7 insertions(+), 28 deletions(-)
> 
> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> index a9657e2b0cda..3364832de83a 100644
> --- a/hw/intc/spapr_xive_kvm.c
> +++ b/hw/intc/spapr_xive_kvm.c
> @@ -79,10 +79,7 @@ void kvmppc_xive_cpu_set_state(XiveTCTX *tctx, Error 
> **errp)
>  uint64_t state[2];
>  int ret;
>  
> -/* The KVM XIVE device is not in use yet */
> -if (xive->fd == -1) {
> -return;
> -}
> +assert(xive->fd != -1);
>  
>  /* word0 and word1 of the OS ring. */
>  state[0] = *((uint64_t *) &tctx->regs[TM_QW1_OS]);
> @@ -101,10 +98,7 @@ void kvmppc_xive_cpu_get_state(XiveTCTX *tctx, Error 
> **errp)
>  uint64_t state[2] = { 0 };
>  int ret;
>  
> -/* The KVM XIVE device is not in use */
> -if (xive->fd == -1) {
> -return;
> -}
> +assert(xive->fd != -1);
>  
>  ret = kvm_get_one_reg(tctx->cs, KVM_REG_PPC_VP_STATE, state);
>  if (ret != 0) {
> @@ -156,10 +150,7 @@ void kvmppc_xive_cpu_connect(XiveTCTX *tctx, Error 
> **errp)
>  unsigned long vcpu_id;
>  int ret;
>  
> -/* The KVM XIVE device is not in use */
> -if (xive->fd == -1) {
> -return;
> -}
> +assert(xive->fd != -1);
>  
>  /* Check if CPU was hot unplugged and replugged. */
>  if (kvm_cpu_is_enabled(tctx->cs)) {
> @@ -245,10 +236,7 @@ int kvmppc_xive_source_reset_one(XiveSource *xsrc, int 
> srcno, Error **errp)
>  SpaprXive *xive = SPAPR_XIVE(xsrc->xive);
>  uint64_t state = 0;
>  
> -/* The KVM XIVE device is not in use */
> -if (xive->fd == -1) {
> -return -ENODEV;
> -}
> +assert(xive->fd != -1);
>  
>  if (xive_source_irq_is_lsi(xsrc, srcno)) {
>  state |= KVM_XIVE_LEVEL_SENSITIVE;
> @@ -592,10 +580,7 @@ static void kvmppc_xive_change_state_handler(void 
> *opaque, int running,
>  
>  void kvmppc_xive_synchronize_state(SpaprXive *xive, Error **errp)
>  {
> -/* The KVM XIVE device is not in use */
> -if (xive->fd == -1) {
> -return;
> -}
> +assert(xive->fd != -1);
>  
>  /*
>   * When the VM is stopped, the sources are masked and the previous
> @@ -622,10 +607,7 @@ int kvmppc_xive_pre_save(SpaprXive *xive)
>  {
>  Error *local_err = NULL;
>  
> -/* The KVM XIVE device is not in use */
> -if (xive->fd == -1) {
> -return 0;
> -}
> +assert(xive->fd != -1);
>  
>  /* EAT: there is no extra state to query from KVM */
>  
> @@ -845,10 +827,7 @@ void kvmppc_xive_disconnect(SpaprInterruptController 
> *intc)
>  XiveSource *xsrc;
>  size_t esb_len;
>  
> -/* The KVM XIVE device is not in use */
> -if (!xive || xive->fd == -1) {
> -return;
> -}
> +assert(xive->fd != -1);
>  
>  /* Clear the KVM mapping */
>  xsrc = &xive->source;
> 
> 

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH for-5.2 5/5] spapr: Simplify error handling in spapr_phb_realize()

[David Gibson]

On Wed, Aug 05, 2020 at 07:35:51PM +0200, Greg Kurz wrote:
> The spapr_phb_realize() function has a local_err variable which
> is used to:
> 
> 1) check failures of spapr_irq_findone() and spapr_irq_claim()
> 
> 2) prepend extra information to the error message
> 
> Recent work from Markus Armbruster highlighted we get better
> code when testing the return value of a function, rather than
> setting up all the local_err boiler plate. For similar reasons,
> it is now preferred to use ERRP_GUARD() and error_prepend()
> rather than error_propagate_prepend().
> 
> Since spapr_irq_findone() and spapr_irq_claim() return negative
> values in case of failure, do both changes.
> 
> This is just cleanup, no functional impact.
> 
> Signed-off-by: Greg Kurz 
> Reviewed-by: Markus Armbruster 

Reviewed-by: David Gibson 

> ---
> Note that the int32_t=>int followup change suggested by Markus was squashed
> into this patch.
> ---
>  hw/ppc/spapr_pci.c |   16 +++-
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 363cdb3f7b8d..0a418f1e6711 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1796,6 +1796,7 @@ static void spapr_phb_destroy_msi(gpointer opaque)
>  
>  static void spapr_phb_realize(DeviceState *dev, Error **errp)
>  {
> +ERRP_GUARD();
>  /* We don't use SPAPR_MACHINE() in order to exit gracefully if the user
>   * tries to add a sPAPR PHB to a non-pseries machine.
>   */
> @@ -1813,7 +1814,6 @@ static void spapr_phb_realize(DeviceState *dev, Error 
> **errp)
>  uint64_t msi_window_size = 4096;
>  SpaprTceTable *tcet;
>  const unsigned windows_supported = spapr_phb_windows_supported(sphb);
> -Error *local_err = NULL;
>  
>  if (!spapr) {
>  error_setg(errp, TYPE_SPAPR_PCI_HOST_BRIDGE " needs a pseries 
> machine");
> @@ -1964,13 +1964,12 @@ static void spapr_phb_realize(DeviceState *dev, Error 
> **errp)
>  
>  /* Initialize the LSI table */
>  for (i = 0; i < PCI_NUM_PINS; i++) {
> -uint32_t irq = SPAPR_IRQ_PCI_LSI + sphb->index * PCI_NUM_PINS + i;
> +int irq = SPAPR_IRQ_PCI_LSI + sphb->index * PCI_NUM_PINS + i;
>  
>  if (smc->legacy_irq_allocation) {
> -irq = spapr_irq_findone(spapr, &local_err);
> -if (local_err) {
> -error_propagate_prepend(errp, local_err,
> -"can't allocate LSIs: ");
> +irq = spapr_irq_findone(spapr, errp);
> +if (irq < 0) {
> +error_prepend(errp, "can't allocate LSIs: ");
>  /*
>   * Older machines will never support PHB hotplug, ie, this 
> is an
>   * init only path and QEMU will terminate. No need to 
> rollback.
> @@ -1979,9 +1978,8 @@ static void spapr_phb_realize(DeviceState *dev, Error 
> **errp)
>  }
>  }
>  
> -spapr_irq_claim(spapr, irq, true, &local_err);
> -if (local_err) {
> -error_propagate_prepend(errp, local_err, "can't allocate LSIs: 
> ");
> +if (spapr_irq_claim(spapr, irq, true, errp) < 0) {
> +error_prepend(errp, "can't allocate LSIs: ");
>  goto unrealize;
>  }
>  
> 
> 

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH for-5.2 3/5] ppc/xive: Introduce dedicated kvm_irqchip_in_kernel() wrappers

[David Gibson]

On Wed, Aug 05, 2020 at 07:35:37PM +0200, Greg Kurz wrote:
> Calls to the KVM XIVE device are guarded by kvm_irqchip_in_kernel(). This
> ensures that QEMU won't try to use the device if KVM is disabled or if
> an in-kernel irqchip isn't required.
> 
> When using ic-mode=dual with the pseries machine, we have two possible
> interrupt controllers: XIVE and XICS. The kvm_irqchip_in_kernel() helper
> will return true as soon as any of the KVM device is created. It might
> lure QEMU to think that the other one is also around, while it is not.
> This is exactly what happens with ic-mode=dual at machine init when
> claiming IRQ numbers, which must be done on all possible IRQ backends,
> eg. RTAS event sources or the PHB0 LSI table : only the KVM XICS device
> is active but we end up calling kvmppc_xive_source_reset_one() anyway,
> which fails. This doesn't cause any trouble because of another bug :
> kvmppc_xive_source_reset_one() lacks an error_setg() and callers don't
> see the failure.
> 
> Most of the other kvmppc_xive_* functions have similar xive->fd
> checks to filter out the case when KVM XIVE isn't active. It
> might look safer to have idempotent functions but it doesn't
> really help to understand what's going on when debugging.
> 
> Since we already have all the kvm_irqchip_in_kernel() in place,
> also have the callers to check xive->fd as well before calling
> KVM XIVE specific code. Introduce helpers to access the underlying
> fd for various XIVE types and call them with a kvm_irqchip_in_kernel()
> guard : this allows the compiler to optimize the kvmppc_xive_* calls
> out if CONFIG_KVM isn't defined, thus avoiding the need for stubs.
> 
> Signed-off-by: Greg Kurz 
> ---
>  hw/intc/spapr_xive.c|   39 +--
>  hw/intc/spapr_xive_kvm.c|   15 +++
>  hw/intc/xive.c  |   30 +++---
>  include/hw/ppc/spapr_xive.h |1 +
>  include/hw/ppc/xive.h   |2 ++
>  5 files changed, 66 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index 89c8cd96670b..98e6489fb16d 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -148,12 +148,19 @@ static void spapr_xive_end_pic_print_info(SpaprXive 
> *xive, XiveEND *end,
>  xive_end_queue_pic_print_info(end, 6, mon);
>  }
>  
> +/*
> + * kvm_irqchip_in_kernel() will cause the compiler to turn this
> + * info a nop if CONFIG_KVM isn't defined.
> + */
> +#define kvmppc_xive_in_kernel(xive) \
> +(kvm_irqchip_in_kernel() && kvmppc_xive_kernel_irqchip(xive))

This all seems like a good idea, my only concern is that the semantic
difference between kvmppc_xive_in_kernel() and
kvmppc_xive_kernel_irqchip() is pretty subtle and non-obvious (and
likewise for the tctx and xsrc variations).

We're right in the XIVE implementation, so I suggest you eliminate the
kvmppc_xive_kernel_irqchip() etc. wrappers and just open code a check
on the fd in kvmppc_xive_in_kernel() etc.

> +
>  void spapr_xive_pic_print_info(SpaprXive *xive, Monitor *mon)
>  {
>  XiveSource *xsrc = &xive->source;
>  int i;
>  
> -if (kvm_irqchip_in_kernel()) {
> +if (kvmppc_xive_in_kernel(xive)) {
>  Error *local_err = NULL;
>  
>  kvmppc_xive_synchronize_state(xive, &local_err);
> @@ -507,8 +514,10 @@ static const VMStateDescription vmstate_spapr_xive_eas = 
> {
>  
>  static int vmstate_spapr_xive_pre_save(void *opaque)
>  {
> -if (kvm_irqchip_in_kernel()) {
> -return kvmppc_xive_pre_save(SPAPR_XIVE(opaque));
> +SpaprXive *xive = SPAPR_XIVE(opaque);
> +
> +if (kvmppc_xive_in_kernel(xive)) {
> +return kvmppc_xive_pre_save(xive);
>  }
>  
>  return 0;
> @@ -520,8 +529,10 @@ static int vmstate_spapr_xive_pre_save(void *opaque)
>   */
>  static int spapr_xive_post_load(SpaprInterruptController *intc, int 
> version_id)
>  {
> -if (kvm_irqchip_in_kernel()) {
> -return kvmppc_xive_post_load(SPAPR_XIVE(intc), version_id);
> +SpaprXive *xive = SPAPR_XIVE(intc);
> +
> +if (kvmppc_xive_in_kernel(xive)) {
> +return kvmppc_xive_post_load(xive, version_id);
>  }
>  
>  return 0;
> @@ -564,7 +575,7 @@ static int spapr_xive_claim_irq(SpaprInterruptController 
> *intc, int lisn,
>  xive_source_irq_set_lsi(xsrc, lisn);
>  }
>  
> -if (kvm_irqchip_in_kernel()) {
> +if (kvmppc_xive_in_kernel(xive)) {
>  return kvmppc_xive_source_reset_one(xsrc, lisn, errp);
>  }
>  
> @@ -641,7 +652,7 @@ static void spapr_xive_set_irq(SpaprInterruptController 
> *intc, int irq, int val)
>  {
>  SpaprXive *xive = SPAPR_XIVE(intc);
>  
> -if (kvm_irqchip_in_kernel()) {
> +if (kvmppc_xive_in_kernel(xive)) {
>  kvmppc_xive_source_set_irq(&xive->source, irq, val);
>  } else {
>  xive_source_set_irq(&xive->source, irq, val);
> @@ -749,7 +760,7 @@ static void 
> spapr_xive_deactivate(SpaprInterruptController *intc)
>  
>  s

Re: cleanups with long-term benefits (was Re: [PATCH] schemas: Add vim modeline)

[Markus Armbruster]

John Snow  writes:

> On 8/5/20 3:36 AM, Markus Armbruster wrote:
>> John Snow  writes:
>>
>>> On 8/4/20 4:03 AM, Markus Armbruster wrote:
 The pain of tweaking the parser is likely dwarved several times over by
 the pain of the flag day.
>>>
>>> You mention this often; I wonder if I misunderstand the critique,
>>> because the pain of a "flag day" for a new file format seems
>>> negligible to me.
>>>
>>> I don't think we edit these .json files very often. Generally, we add
>>> a new command when we need one. The edits are usually one or two lines
>>> plus docstrings.
>>>
>>> If anyone has patches in-flight, I genuinely doubt it will take more
>>> than a few minutes to rewrite for the new file format.
>>>
>>> No?
>>
>> You describe the the flag day's one-time pain.
>>
>> There's also the longer term pain of having to work around git-blame
>> unable to see beyond the flag day.
>>
>
> So it's not really the "flag day" we're worried about anymore, it's
> the ongoing ease-of-use for vcs history.

Feel free to call that pain however you want.  I'm going to call it
"lasting aftereffects of the flag day" :)

>> I'm not claiming the pain is prohibitive (if I thought it was, I
>> would've tried to strange this thread in its crib), I am claiming it'll
>> be much more painful (read: expensive) than a parser tweak.
>>
>
> I do use `git blame` quite a lot, but with a project as old as QEMU,
> most of my trips through history do involve jumping across a few
> refactor gaps as a normal part of that process.
>
> As Dan points out, I often have to back out and add refactorSHA^ to my
> invocation, and just keep hopping backwards until I find what I am
> truly after. It just feels like a fact of programmer life for me at
> this point.

The fact that we all need to cope with this class of issue doesn't mean
we should create more instances unthinkingly.

We should only when we believe the benefits are worth it, and can't find
a cheaper way to get them.

We've discussed "is it really that bad" at some length.  What I'm
missing so far is a clear writeup of the benefits beyond "editor works
out of the box" (which is quite a desirable benefit, but can also be had
without a flag day).

> I've not used Paolo's invocation before, but it looks like it might be
> useful. I'll try to remember to try it out.

[PATCH v5 1/2] hw/smbios: add options for type 4 max-speed and current-speed

[Ying Fang]

Common VM users sometimes care about CPU speed, so we add two new
options to allow VM vendors to present CPU speed to their users.
Normally these information can be fetched from host smbios.

Strictly speaking, the "max speed" and "current speed" in type 4
are not really for the max speed and current speed of processor, for
"max speed" identifies a capability of the system, and "current speed"
identifies the processor's speed at boot (see smbios spec), but some
applications do not tell the differences.

Reviewed-by: Igor Mammedov 
Signed-off-by: Ying Fang 
Signed-off-by: Heyi Guo 
---
 hw/smbios/smbios.c | 36 
 qemu-options.hx|  2 +-
 2 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index 11d476c4a2..53181a58eb 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -93,9 +93,21 @@ static struct {
 const char *manufacturer, *version, *serial, *asset, *sku;
 } type3;
 
+/*
+ * SVVP requires max_speed and current_speed to be set and not being
+ * 0 which counts as unknown (SMBIOS 3.1.0/Table 21). Set the
+ * default value to 2000MHz as we did before.
+ */
+#define DEFAULT_CPU_SPEED 2000
+
 static struct {
 const char *sock_pfx, *manufacturer, *version, *serial, *asset, *part;
-} type4;
+uint64_t max_speed;
+uint64_t current_speed;
+} type4 = {
+.max_speed = DEFAULT_CPU_SPEED,
+.current_speed = DEFAULT_CPU_SPEED
+};
 
 static struct {
 size_t nvalues;
@@ -273,6 +285,14 @@ static const QemuOptDesc qemu_smbios_type4_opts[] = {
 .name = "version",
 .type = QEMU_OPT_STRING,
 .help = "version number",
+},{
+.name = "max-speed",
+.type = QEMU_OPT_NUMBER,
+.help = "max speed in MHz",
+},{
+.name = "current-speed",
+.type = QEMU_OPT_NUMBER,
+.help = "speed at system boot in MHz",
 },{
 .name = "serial",
 .type = QEMU_OPT_STRING,
@@ -587,9 +607,8 @@ static void smbios_build_type_4_table(MachineState *ms, 
unsigned instance)
 SMBIOS_TABLE_SET_STR(4, processor_version_str, type4.version);
 t->voltage = 0;
 t->external_clock = cpu_to_le16(0); /* Unknown */
-/* SVVP requires max_speed and current_speed to not be unknown. */
-t->max_speed = cpu_to_le16(2000); /* 2000 MHz */
-t->current_speed = cpu_to_le16(2000); /* 2000 MHz */
+t->max_speed = cpu_to_le16(type4.max_speed);
+t->current_speed = cpu_to_le16(type4.current_speed);
 t->status = 0x41; /* Socket populated, CPU enabled */
 t->processor_upgrade = 0x01; /* Other */
 t->l1_cache_handle = cpu_to_le16(0x); /* N/A */
@@ -1130,6 +1149,15 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
 save_opt(&type4.serial, opts, "serial");
 save_opt(&type4.asset, opts, "asset");
 save_opt(&type4.part, opts, "part");
+type4.max_speed = qemu_opt_get_number(opts, "max-speed",
+  DEFAULT_CPU_SPEED);
+type4.current_speed = qemu_opt_get_number(opts, "current-speed",
+  DEFAULT_CPU_SPEED);
+if (type4.max_speed > UINT16_MAX ||
+type4.current_speed > UINT16_MAX) {
+error_setg(errp, "SMBIOS CPU speed is too large (> %d)",
+   UINT16_MAX);
+}
 return;
 case 11:
 qemu_opts_validate(opts, qemu_smbios_type11_opts, &err);
diff --git a/qemu-options.hx b/qemu-options.hx
index ea0638e92d..50b068423c 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2073,7 +2073,7 @@ DEF("smbios", HAS_ARG, QEMU_OPTION_smbios,
 "  [,sku=str]\n"
 "specify SMBIOS type 3 fields\n"
 "-smbios 
type=4[,sock_pfx=str][,manufacturer=str][,version=str][,serial=str]\n"
-"  [,asset=str][,part=str]\n"
+"  [,asset=str][,part=str][,max-speed=%d][,current-speed=%d]\n"
 "specify SMBIOS type 4 fields\n"
 "-smbios 
type=17[,loc_pfx=str][,bank=str][,manufacturer=str][,serial=str]\n"
 "   [,asset=str][,part=str][,speed=%d]\n"
-- 
2.23.0

[PATCH v5 2/2] tests/bios-tables-test: add smbios cpu speed test

[Ying Fang]

Add smbios type 4 CPU speed check for we added new options to set
smbios type 4 "max speed" and "current speed". The default value
should be 2000 when no option is specified, just as the old version
did.

We add the test case to one machine of each architecture, though it
doesn't really run on aarch64 platform for smbios test can't run on
uefi only platform yet.

Signed-off-by: Ying Fang 
Signed-off-by: Heyi Guo 
---
 tests/bios-tables-test.c | 42 
 1 file changed, 42 insertions(+)

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index a356ac3489..6bd165021b 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -37,6 +37,8 @@ typedef struct {
 GArray *tables;
 uint32_t smbios_ep_addr;
 struct smbios_21_entry_point smbios_ep_table;
+uint16_t smbios_cpu_max_speed;
+uint16_t smbios_cpu_curr_speed;
 uint8_t *required_struct_types;
 int required_struct_types_len;
 QTestState *qts;
@@ -516,6 +518,31 @@ static inline bool smbios_single_instance(uint8_t type)
 }
 }
 
+static bool smbios_cpu_test(test_data *data, uint32_t addr)
+{
+uint16_t expect_speed[2];
+uint16_t real;
+int offset[2];
+int i;
+
+/* Check CPU speed for backward compatibility */
+offset[0] = offsetof(struct smbios_type_4, max_speed);
+offset[1] = offsetof(struct smbios_type_4, current_speed);
+expect_speed[0] = data->smbios_cpu_max_speed ? : 2000;
+expect_speed[1] = data->smbios_cpu_curr_speed ? : 2000;
+
+for (i = 0; i < 2; i++) {
+real = qtest_readw(data->qts, addr + offset[i]);
+if (real != expect_speed[i]) {
+fprintf(stderr, "Unexpected SMBIOS CPU speed: real %u expect %u\n",
+real, expect_speed[i]);
+return false;
+}
+}
+
+return true;
+}
+
 static void test_smbios_structs(test_data *data)
 {
 DECLARE_BITMAP(struct_bitmap, SMBIOS_MAX_TYPE+1) = { 0 };
@@ -538,6 +565,10 @@ static void test_smbios_structs(test_data *data)
 }
 set_bit(type, struct_bitmap);
 
+if (type == 4) {
+g_assert(smbios_cpu_test(data, addr));
+}
+
 /* seek to end of unformatted string area of this struct ("\0\0") */
 prv = crt = 1;
 while (prv || crt) {
@@ -673,6 +704,11 @@ static void test_acpi_q35_tcg(void)
 data.required_struct_types_len = ARRAY_SIZE(base_required_struct_types);
 test_acpi_one(NULL, &data);
 free_test_data(&data);
+
+data.smbios_cpu_max_speed = 3000;
+data.smbios_cpu_curr_speed = 2600;
+test_acpi_one("-smbios type=4,max-speed=3000,current-speed=2600", &data);
+free_test_data(&data);
 }
 
 static void test_acpi_q35_tcg_bridge(void)
@@ -885,6 +921,12 @@ static void test_acpi_virt_tcg(void)
 
 test_acpi_one("-cpu cortex-a57", &data);
 free_test_data(&data);
+
+data.smbios_cpu_max_speed = 2900;
+data.smbios_cpu_curr_speed = 2700;
+test_acpi_one("-cpu cortex-a57 "
+  "-smbios type=4,max-speed=2900,current-speed=2700", &data);
+free_test_data(&data);
 }
 
 int main(int argc, char *argv[])
-- 
2.23.0

[PATCH v5 0/2] add new options to set smbios type 4 fields

[Ying Fang]

From: fangying 

Hi, this patchset was previously posted by my teamate Heyi Guo several
months ago, however we missed the merge window. It is reposted here to
make it an end. Thanks.

Patch description:
 
Common VM users sometimes care about CPU speed, so we add two new
options to allow VM vendors to present CPU speed to their users.
Normally these information can be fetched from host smbios.

Strictly speaking, the "max speed" and "current speed" in type 4
are not really for the max speed and current speed of processor, for
"max speed" identifies a capability of the system, and "current speed"
identifies the processor's speed at boot (see smbios spec), but some
applications do not tell the differences.

Changelog:

v4 -> v5:
- Rebase patch for lastest upstream

v3 -> v4:
- Fix the default value when not specifying "-smbios type=4" option;
it would be 0 instead of 2000 in previous versions
- Use uint64_t type to check value overflow
- Add test case to check smbios type 4 CPU speed
- v4 https://patchwork.kernel.org/cover/11444635/

v2 -> v3:
- Refine comments per Igor's suggestion.

v1 -> v2:
- change "_" in option names to "-"
- check if option value is too large to fit in SMBIOS type 4 speed
fields.

Cc: "Michael S. Tsirkin" 
Cc: Igor Mammedov 

Ying Fang (2):
  hw/smbios: add options for type 4 max-speed and current-speed
  tests/bios-tables-test: add smbios cpu speed test

 hw/smbios/smbios.c   | 36 ++
 qemu-options.hx  |  2 +-
 tests/bios-tables-test.c | 42 
 3 files changed, 75 insertions(+), 5 deletions(-)

-- 
2.23.0

Re: Adding VHOST_USER_PROTOCOL_F_CONFIG_MEM_SLOTS to 5.1 release notes

[Raphael Norwitz]

ping

On Wed, Jul 29, 2020 at 9:17 PM Raphael Norwitz
 wrote:
>
> How about something like:
> "A new feature, VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS, has been
> added to the vhost-user protocol which, when negotiated, changes the
> way QEMU transmit memory regions to backend devices. Instead of
> sending all regions in a single VHOST_USER_SET_MEM_TABLE message, QEMU
> will send supporting backends individual VHOST_USER_ADD_MEM_REG and
> VHOST_USER_REM_MEM_REG messages to update the devices memory tables.
> VMs with vhost-user device backends which support this feature will
> not be subject to the max RAM slots limit of 8 and will be able to
> hot-add memory as many times as the target platform supports. Backends
> which do not support VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS are
> unaffected."
>
> I don't have permission to edit the wiki. How can I get permission? Or
> can someone post it for me?
>
> On Wed, Jul 29, 2020 at 8:19 AM Michael S. Tsirkin  wrote:
> >
> > On Tue, Jul 28, 2020 at 09:16:10PM -0600, Raphael Norwitz wrote:
> > > Hi mst,
> > >
> > > Looking at the current changelog
> > > https://wiki.qemu.org/ChangeLog/5.1#virtio, I don't see any mention of
> > > the VHOST_USER_PROTOCOL_F_CONFIG_MEM_SLOTS protocol feature. It is a
> > > user visible change so shouldn't we add a note?
> > >
> > > Thanks,
> > > Raphael
> >
> > I didn't look at updating the changelog yet.
> > Would be great if you could write up new vhost user things.
> >
> > --
> > MST
> >

Re: [PATCH 0/2] Instruction set detection for clang.

[Shu-Chun Weng]

Ping: https://patchew.org/QEMU/cover.1595463707.git@google.com/

On Wed, Jul 22, 2020 at 5:27 PM Shu-Chun Weng  wrote:

> Currently when configuring QEMU with clang, AVX2, AVX512F, ATOMIC64, and
> ATOMIC128 are all disabled because the detection code is GCC-only. With
> these
> two patches, I am able to configure, build, and run tests with clang with
> all of
> the above enabled.
>
> Shu-Chun Weng (2):
>   configure: avx2 and avx512f detection for clang
>   configure: atomic64/128 detection for clang
>
>  configure   | 34 +++---
>  util/bufferiszero.c | 33 +++--
>  2 files changed, 46 insertions(+), 21 deletions(-)
>
> --
> 2.28.0.rc0.105.gf9edc3c819-goog
>
>


smime.p7s
Description: S/MIME Cryptographic Signature

Re: [PATCH v2] linux-user: Add most IFTUN ioctls

[Shu-Chun Weng]

Ping: https://patchew.org/QEMU/20200723231020.769893-1-...@google.com/

On Thu, Jul 23, 2020 at 4:10 PM Shu-Chun Weng  wrote:

> The three options handling `struct sock_fprog` (TUNATTACHFILTER,
> TUNDETACHFILTER, and TUNGETFILTER) are not implemented. Linux kernel
> keeps a user space pointer in them which we cannot correctly handle.
>
> Signed-off-by: Josh Kunz 
> Signed-off-by: Shu-Chun Weng 
> ---
> v2:
>   Title changed from "linux-user: Add several IFTUN ioctls"
>
>   Properly specify the argument types for various options, including a
> custom
>   implementation for TUNSETTXFILTER.
>
>   #ifdef guards for macros introduced up to 5 years ago.
>
>  linux-user/ioctls.h   | 45 +++
>  linux-user/syscall.c  | 36 +++
>  linux-user/syscall_defs.h | 32 
>  3 files changed, 113 insertions(+)
>
> diff --git a/linux-user/ioctls.h b/linux-user/ioctls.h
> index 0713ae1311..b9fb01f558 100644
> --- a/linux-user/ioctls.h
> +++ b/linux-user/ioctls.h
> @@ -593,3 +593,48 @@
>IOCTL(KCOV_DISABLE, 0, TYPE_NULL)
>IOCTL(KCOV_INIT_TRACE, IOC_R, TYPE_ULONG)
>  #endif
> +
> +  IOCTL(TUNSETDEBUG, IOC_W, TYPE_INT)
> +  IOCTL(TUNSETIFF,   IOC_RW, MK_PTR(MK_STRUCT(STRUCT_short_ifreq)))
> +  IOCTL(TUNSETPERSIST,   IOC_W, TYPE_INT)
> +  IOCTL(TUNSETOWNER, IOC_W, TYPE_INT)
> +  IOCTL(TUNSETLINK,  IOC_W, TYPE_INT)
> +  IOCTL(TUNSETGROUP, IOC_W, TYPE_INT)
> +  IOCTL(TUNGETFEATURES,  IOC_R, MK_PTR(TYPE_INT))
> +  IOCTL(TUNSETOFFLOAD,   IOC_W, TYPE_LONG)
> +  IOCTL_SPECIAL(TUNSETTXFILTER, IOC_W, do_ioctl_TUNSETTXFILTER,
> +/*
> + * We can't represent `struct tun_filter` in thunk so
> leaving
> + * this empty. do_ioctl_TUNSETTXFILTER will do the
> conversion.
> + */
> +TYPE_NULL)
> +  IOCTL(TUNGETIFF,   IOC_R, MK_PTR(MK_STRUCT(STRUCT_short_ifreq)))
> +  IOCTL(TUNGETSNDBUF,IOC_R, MK_PTR(TYPE_INT))
> +  IOCTL(TUNSETSNDBUF,IOC_W, MK_PTR(TYPE_INT))
> +  /*
> +   * TUNATTACHFILTER and TUNDETACHFILTER are not supported. Linux kernel
> keeps a
> +   * user pointer in TUNATTACHFILTER, which we are not able to correctly
> handle.
> +   */
> +  IOCTL(TUNGETVNETHDRSZ, IOC_R, MK_PTR(TYPE_INT))
> +  IOCTL(TUNSETVNETHDRSZ, IOC_W, MK_PTR(TYPE_INT))
> +  IOCTL(TUNSETQUEUE, IOC_W, MK_PTR(MK_STRUCT(STRUCT_short_ifreq)))
> +  IOCTL(TUNSETIFINDEX ,  IOC_W, MK_PTR(TYPE_INT))
> +  /* TUNGETFILTER is not supported: see TUNATTACHFILTER. */
> +  IOCTL(TUNSETVNETLE,IOC_W, MK_PTR(TYPE_INT))
> +  IOCTL(TUNGETVNETLE,IOC_R, MK_PTR(TYPE_INT))
> +#ifdef TUNSETVNETBE
> +  IOCTL(TUNSETVNETBE,IOC_W, MK_PTR(TYPE_INT))
> +  IOCTL(TUNGETVNETBE,IOC_R, MK_PTR(TYPE_INT))
> +#endif
> +#ifdef TUNSETSTEERINGEBPF
> +  IOCTL(TUNSETSTEERINGEBPF, IOC_W, MK_PTR(TYPE_INT))
> +#endif
> +#ifdef TUNSETFILTEREBPF
> +  IOCTL(TUNSETFILTEREBPF, IOC_W, MK_PTR(TYPE_INT))
> +#endif
> +#ifdef TUNSETCARRIER
> +  IOCTL(TUNSETCARRIER,   IOC_W, MK_PTR(TYPE_INT))
> +#endif
> +#ifdef TUNGETDEVNETNS
> +  IOCTL(TUNGETDEVNETNS,  IOC_R, TYPE_NULL)
> +#endif
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 1211e759c2..7f1efed189 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -56,6 +56,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #ifdef CONFIG_TIMERFD
> @@ -5422,6 +5423,41 @@ static abi_long do_ioctl_drm(const IOCTLEntry *ie,
> uint8_t *buf_temp,
>
>  #endif
>
> +static abi_long do_ioctl_TUNSETTXFILTER(const IOCTLEntry *ie, uint8_t
> *buf_temp,
> +int fd, int cmd, abi_long arg)
> +{
> +struct tun_filter *filter = (struct tun_filter *)buf_temp;
> +struct tun_filter *target_filter;
> +char *target_addr;
> +
> +assert(ie->access == IOC_W);
> +
> +target_filter = lock_user(VERIFY_READ, arg, sizeof(*filter), 1);
> +if (!target_filter) {
> +return -TARGET_EFAULT;
> +}
> +filter->flags = tswap16(target_filter->flags);
> +filter->count = tswap16(target_filter->count);
> +unlock_user(target_filter, arg, sizeof(*filter));
> +
> +if (filter->count) {
> +if (sizeof(*filter) + filter->count * ETH_ALEN > MAX_STRUCT_SIZE)
> {
> +return -TARGET_EFAULT;
> +}
> +
> +target_addr = lock_user(VERIFY_READ, arg + sizeof(*filter),
> +filter->count * ETH_ALEN, 1);
> +if (!target_addr) {
> +return -TARGET_EFAULT;
> +}
> +memcpy(filter->addr, target_addr, filter->count * ETH_ALEN);
> +unlock_user(target_addr, arg + sizeof(*filter),
> +filter->count * ETH_ALEN);
> +}
> +
> +return get_errno(safe_ioctl(fd, ie->host_cmd, filter));
> +}
> +
>  IOCTLEntry ioctl_entries[] = {
>  #define IOCTL(cmd, access, ...) \
>  { TARGET_ ## cmd, cmd, #cmd, access, 0, {  __VA_ARGS__ } },
> diff --git a/linux-u

Re: [PATCH 0/6] fcntl, sockopt, and ioctl options

[Shu-Chun Weng]

Ping! Patchew: https://patchew.org/QEMU/cover.1595461447.git@google.com/

On Wed, Jul 22, 2020 at 5:19 PM Shu-Chun Weng  wrote:

> Hi Laurent,
>
> This is a series of 6 patches in 4 groups, putting into a single thread for
> easier tracking.
>
> [PATCH 1/6] linux-user: Support F_ADD_SEALS and F_GET_SEALS fcntls
>   An incidental follow up on
>   https://lists.nongnu.org/archive/html/qemu-devel/2019-09/msg01925.html
>
> [PATCH 2/6] linux-user: add missing UDP and IPv6 get/setsockopt
>   Updated
> https://lists.nongnu.org/archive/html/qemu-devel/2019-09/msg01317.html
>   to consistently add them in get/setsockopt
>
> [PATCH 3/6] linux-user: Update SO_TIMESTAMP to SO_TIMESTAMP_OLD/NEW
> [PATCH 4/6] linux-user: setsockopt() SO_TIMESTAMPNS and SO_TIMESTAMPING
>   Updated
> https://lists.nongnu.org/archive/html/qemu-devel/2019-09/msg01319.html
>   to only use TARGET_SO_*_OLD/NEW
>
> [PATCH 5/6] thunk: supports flexible arrays
> [PATCH 6/6] linux-user: Add support for SIOCETHTOOL ioctl
>   Updated
> https://lists.nongnu.org/archive/html/qemu-devel/2019-08/msg05090.html
>
> Shu-Chun Weng (6):
>   linux-user: Support F_ADD_SEALS and F_GET_SEALS fcntls
>   linux-user: add missing UDP and IPv6 get/setsockopt options
>   linux-user: Update SO_TIMESTAMP to SO_TIMESTAMP_OLD/NEW
>   linux-user: setsockopt() SO_TIMESTAMPNS and SO_TIMESTAMPING
>   thunk: supports flexible arrays
>   linux-user: Add support for SIOCETHTOOL ioctl
>
>  include/exec/user/thunk.h  |  20 +
>  linux-user/Makefile.objs   |   3 +-
>  linux-user/alpha/sockbits.h|  21 +-
>  linux-user/ethtool.c   | 819 +
>  linux-user/ethtool.h   |  19 +
>  linux-user/ethtool_entries.h   | 107 
>  linux-user/fd-trans.h  |  41 +-
>  linux-user/generic/sockbits.h  |  17 +-
>  linux-user/hppa/sockbits.h |  20 +-
>  linux-user/ioctls.h|   2 +
>  linux-user/mips/sockbits.h |  16 +-
>  linux-user/qemu.h  |   1 +
>  linux-user/sparc/sockbits.h|  21 +-
>  linux-user/strace.c|  19 +-
>  linux-user/syscall.c   | 233 ++-
>  linux-user/syscall_defs.h  |  26 +-
>  linux-user/syscall_types.h | 277 +
>  tests/tcg/multiarch/ethtool.c  | 417 +
>  tests/tcg/multiarch/socket_timestamp.c | 542 
>  thunk.c| 151 -
>  20 files changed, 2706 insertions(+), 66 deletions(-)
>  create mode 100644 linux-user/ethtool.c
>  create mode 100644 linux-user/ethtool.h
>  create mode 100644 linux-user/ethtool_entries.h
>  create mode 100644 tests/tcg/multiarch/ethtool.c
>  create mode 100644 tests/tcg/multiarch/socket_timestamp.c
>
> --
> 2.28.0.rc0.105.gf9edc3c819-goog
>
>


smime.p7s
Description: S/MIME Cryptographic Signature

Re: vhost-user protocol feature negotiation

[Michael S. Tsirkin]

On Wed, Aug 05, 2020 at 03:13:06PM +, Alyssa Ross wrote:
> Quoting from the definition of VHOST_USER_SET_PROTOCOL_FEATURES in
> vhost-user.rst:
> 
> >   Only legal if feature bit ``VHOST_USER_F_PROTOCOL_FEATURES`` is present in
> >   ``VHOST_USER_GET_FEATURES``.
> > 
> > .. Note::
> >Slave that reported ``VHOST_USER_F_PROTOCOL_FEATURES`` must support
> >this message even before ``VHOST_USER_SET_FEATURES`` was called.
> 
> To me, this could mean either of two things:
> 
> (1) If VHOST_USER_F_PROTOCOL_FEATURES hasn't been set, upon receiving
> VHOST_USER_SET_PROTOCOL_FEATURES, a backend should enable the
> protocol features immediately.
> 
> (2) If VHOST_USER_F_PROTOCOL_FEATURES hasn't been set, upon receiving
> VHOST_USER_SET_PROTOCOL_FEATURES, a backend should store those
> feature bits, but not actually consider them to be enabled until
> after VHOST_USER_SET_FEATURES has been received (presumably
> containing VHOST_USER_F_PROTOCOL_FEATURES).
> 
> The reason I bring this up is that QEMU appears to interpret it as (1),
> while the vhost-user-net backend in Intel's cloud-hypervisor[1]
> interprets it as (2).  So I'm looking for a clarification.
> 
> [1]: https://github.com/cloud-hypervisor/cloud-hypervisor
> 
> Thanks in advance.


IMHO the intent was this: VHOST_USER_F_PROTOCOL_FEATURES bit in
VHOST_USER_GET_FEATURES means that qemu can send
VHOST_USER_GET_PROTOCOL_FEATURES and VHOST_USER_SET_PROTOCOL_FEATURES.

With most feature bits in VHOST_USER_GET_FEATURES, the
specific functionality needs to only be enabled after
VHOST_USER_SET_FEATURES.

However, this is for functionality dealing with guest activity.
VHOST_USER_SET_PROTOCOL_FEATURES has nothing to do with guest directly,
it's about negotiation between qemu and backend: it is only in
VHOST_USER_GET_FEATURES for the reason that this is the only message
(very) old backends reported.  Thus, the backend should not check
whether VHOST_USER_SET_FEATURES sets VHOST_USER_F_PROTOCOL_FEATURES,
instead it should simply always be ready to receive
VHOST_USER_GET_PROTOCOL_FEATURES and VHOST_USER_SET_PROTOCOL_FEATURES.

Backend that isn't always ready to handle
VHOST_USER_GET_PROTOCOL_FEATURES and VHOST_USER_SET_PROTOCOL_FEATURES
should not set VHOST_USER_F_PROTOCOL_FEATURES in
VHOST_USER_GET_FEATURES.

This appears to be closer to (1), but if qemu can't distinguish
then we don't care, right? For example, VHOST_USER_PROTOCOL_F_REPLY_ACK
enables acks on arbitrary messages. Does the backend in question
ignore the affected bit until SET_FEATURES? If yes won't this
make qemu hang?

How would you suggest clarifying the wording?

Thanks,

-- 
MST

Re: [PATCH] ide: Get rid of IDEDrive struct

[Eduardo Habkost]

On Wed, Aug 05, 2020 at 09:41:25PM +0100, Peter Maydell wrote:
> On Wed, 5 Aug 2020 at 20:49, Eduardo Habkost  wrote:
> >
> > The struct had a single field (IDEDevice dev), and is only used
> > in the QOM type declarations and property lists.  We can simply
> > use the IDEDevice struct directly instead.
> >
> > Signed-off-by: Eduardo Habkost 
> > @@ -327,7 +323,6 @@ static void ide_hd_class_init(ObjectClass *klass, void 
> > *data)
> >  static const TypeInfo ide_hd_info = {
> >  .name  = "ide-hd",
> >  .parent= TYPE_IDE_DEVICE,
> > -.instance_size = sizeof(IDEDrive),
> >  .class_init= ide_hd_class_init,
> >  };
> 
> This is one of those areas where this change works and reduces
> amount of code, but on the other hand it means the QOM type
> doesn't follow the common pattern for a leaf type of:
>  * it has a struct
>  * it has cast macros that cast to that struct
>  * the typeinfo instance_size is the size of that struct
> (it wasn't exactly following this pattern before, of course).

Is this really a pattern that exists and we want to follow?
I don't see why that pattern would be useful for simple leaf
types.

Also, in this case the code wasn't even following that pattern:
it was using the same IDEDrive struct for all TYPE_IDE_DEVICE
subtypes.

> 
> We define in https://wiki.qemu.org/Documentation/QOMConventions
> (in the 'When to create class types and macros' bit at the bottom)
> what we expect for whether to provide class cast macros/a
> class struct/class_size in the TypeInfo, essentially recommending
> that types follow one of two patterns (simple leaf class with no
> methods or class members, vs everything else) even if in a
> particular case you could take a short-cut and not define
> everything. We haven't really defined similar "this is the
> standard pattern, provide it all even if you don't strictly
> need it" rules for the instance struct/macros. Maybe we should?

I think we should include the instance struct/macros in the
recommendations there, but I would expect those recommendations
to apply only to non-leaf types.

> 
> Just a thought, not a nak; I know we have quite a number
> of types that take this kind of "we don't really need to
> provide all the standard QOM macros/structs/etc" approach
> (some of which I wrote!).
> 
> thanks
> -- PMM
> 

-- 
Eduardo

[PATCH] block/vhdx: Support vhdx image only with 512 bytes logical sector size

[Swapnil Ingle]

block/vhdx uses qemu block layer where sector size is always 512 byte.
This may have issues  with 4K logical sector sized vhdx image.

For e.g qemu-img convert on such images fails with following assert:

$qemu-img convert -f vhdx -O raw 4KTest1.vhdx test.raw
qemu-img: util/iov.c:388: qiov_slice: Assertion `offset + len <=
qiov->size' failed.
Aborted

This patch adds an check to return ENOTSUP for vhdx images which
has logical sector size other than 512 bytes.

Signed-off-by: Swapnil Ingle 
---
 block/vhdx.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/vhdx.c b/block/vhdx.c
index 791eb90..356ec4c 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -816,9 +816,9 @@ static int vhdx_parse_metadata(BlockDriverState *bs, 
BDRVVHDXState *s)
 goto exit;
 }
 
-/* only 2 supported sector sizes */
-if (s->logical_sector_size != 512 && s->logical_sector_size != 4096) {
-ret = -EINVAL;
+/* Currently we only support 512 */
+if (s->logical_sector_size != 512) {
+ret = -ENOTSUP;
 goto exit;
 }
 
-- 
1.8.3.1

Re: [PATCH] target/arm: Delete unused ARM_FEATURE_CRC

[Richard Henderson]

On 8/5/20 2:08 PM, Peter Maydell wrote:
> In commit 962fcbf2efe57231a9f5df we converted the uses of the
> ARM_FEATURE_CRC bit to use the aa32_crc32 isar_feature test
> instead. However we forgot to remove the now-unused definition
> of the feature name in the enum. Delete it now.
> 
> Signed-off-by: Peter Maydell 
> ---
>  target/arm/cpu.h | 1 -
>  1 file changed, 1 deletion(-)

Reviewed-by: Richard Henderson 

r~

[PATCH] target/arm: Delete unused ARM_FEATURE_CRC

[Peter Maydell]

In commit 962fcbf2efe57231a9f5df we converted the uses of the
ARM_FEATURE_CRC bit to use the aa32_crc32 isar_feature test
instead. However we forgot to remove the now-unused definition
of the feature name in the enum. Delete it now.

Signed-off-by: Peter Maydell 
---
 target/arm/cpu.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 9e8ed423ea1..9d2845c1797 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1950,7 +1950,6 @@ enum arm_features {
 ARM_FEATURE_V8,
 ARM_FEATURE_AARCH64, /* supports 64 bit mode */
 ARM_FEATURE_CBAR, /* has cp15 CBAR */
-ARM_FEATURE_CRC, /* ARMv8 CRC instructions */
 ARM_FEATURE_CBAR_RO, /* has cp15 CBAR and it is read-only */
 ARM_FEATURE_EL2, /* has EL2 Virtualization support */
 ARM_FEATURE_EL3, /* has EL3 Secure monitor support */
-- 
2.20.1

[ANNOUNCE] QEMU 5.1.0-rc3 is now available

[Michael Roth]

Hello,

On behalf of the QEMU Team, I'd like to announce the availability of the
fourth release candidate for the QEMU 5.1 release.  This release is meant
for testing purposes and should not be used in a production environment.

  http://download.qemu-project.org/qemu-5.1.0-rc3.tar.xz
  http://download.qemu-project.org/qemu-5.1.0-rc3.tar.xz.sig

A note from the maintainer:

  Unless any last minute release-critical bugs are discovered, this will
  be the last release candidate before we release 5.1. (If anything
  does turn up, as usual we'll roll another rc and do the full release
  a bit later than scheduled).

You can help improve the quality of the QEMU 5.1 release by testing this
release and reporting bugs on Launchpad:

  https://bugs.launchpad.net/qemu/

The release plan, as well a documented known issues for release
candidates, are available at:

  http://wiki.qemu.org/Planning/5.1

Please add entries to the ChangeLog for the 5.1 release below:

  http://wiki.qemu.org/ChangeLog/5.1

Thank you to everyone involved!

Changes since rc2:

e1d322c405: Update version for v5.1.0-rc3 release (Peter Maydell)
a65dabf71a: target/arm: Fix Rt/Rt2 in ESR_ELx for copro traps from AArch32 to 
64 (Peter Maydell)
348fcc4f7a: qcow2-cluster: Fix integer left shift error in 
qcow2_alloc_cluster_link_l2() (Tuguoyi)
d2a71d7474: Get rid of the libqemustub.a remainders (Thomas Huth)
35c7f5254b: target/riscv/vector_helper: Fix build on 32-bit big endian hosts 
(Thomas Huth)
5896c53954: gitlab-ci: Fix Avocado cache usage (Thomas Huth)
699616db64: gitlab-ci.yml: Add build-system-debian and build-system-centos jobs 
(Thomas Huth)
4d6862ffc7: tests/acceptance: Disable the rx sash and arm cubieboard replay 
test on Gitlab (Thomas Huth)
1caac1c0e4: tests/docker: Add python3-venv and netcat to the debian-amd64 
container (Thomas Huth)
facc68516a: virtio-mem: Correct format specifier mismatch for RISC-V (Bruce 
Rogers)
d250bb19ce: target/arm: Fix decode of LDRA[AB] instructions (Peter 
Collingbourne)
ffdfca6fac: docs/devel: Document decodetree no-overlap groups (Richard 
Henderson)
8e0ef06894: accel/xen: Fix xen_enabled() behavior on target-agnostic objects 
(Philippe Mathieu-Daudé)
035e69b063: hw/net/net_tx_pkt: fix assertion failure in 
net_tx_pkt_add_raw_fragment() (Mauro Matteo Cascella)
f81cddfe8a: colo-compare: Remove superfluous NULL-pointer checks for 
s->iothread (Lukas Straub)
13557fd392: hw/timer/imx_epit: Avoid assertion when CR.SWR is written (Peter 
Maydell)
ce4f70e81e: hw/arm/nrf51_soc: Set system_clock_scale (Peter Maydell)
88a90e3de6: target/arm: Avoid maybe-uninitialized warning with gcc 4.9 (Kaige 
Li)
8796fe40dd: target/arm: Fix AddPAC error indication (Richard Henderson)
fc6bb6e67e: msf2-soc, stellaris: Don't wire up SYSRESETREQ (Peter Maydell)
9e60d759d3: hw/intc/armv7m_nvic: Provide default "reset the system" behaviour 
for SYSRESETREQ (Peter Maydell)
faf7c6de34: include/hw/irq.h: New function qemu_irq_is_connected() (Peter 
Maydell)
e7e5a9595a: hw/arm/netduino2, netduinoplus2: Set system_clock_scale (Peter 
Maydell)
edadc99a2e: iotests/169: Test source cont with backing bmap (Max Reitz)
fe16c7ddf8: qcow2: Release read-only bitmaps when inactivated (Max Reitz)
f7160f3218: schemas: Add vim modeline (Andrea Bolognani)
fbeed19761: qapi: Delete unwanted indentation of top-level expressions (Markus 
Armbruster)
6ac3f1e799: qapi/machine.json: Fix missing newline in doc comment (Peter 
Maydell)
1f42e24699: seabios: update to master snapshot (Gerd Hoffmann)
000822441e: tracetool: carefully define SDT_USE_VARIADIC (Stefan Hajnoczi)
148d25e0f6: s390x/s390-virtio-ccw: fix off-by-one in loadparm getter (Halil 
Pasic)
1b7157be3a: trace/simple: Allow enabling simple traces from command line (Josh 
DuBois)

Re: [PATCH v3 7/8] hw/display/artist: Refactor artist_rop8() to avoid buffer over-run

[Helge Deller]

Hello Alexander,

* Alexander Bulekov :
> On 200804 2320, Helge Deller wrote:
> > * Alexander Bulekov :
> > > I applied this series and it fixes most of the problems I saw before.
> > > I still see a few crashes - I made issues for them on launchpad:
> > > https://bugs.launchpad.net/qemu/+bug/1890310
> > > https://bugs.launchpad.net/qemu/+bug/1890311
> > > https://bugs.launchpad.net/qemu/+bug/1890312
> Hi Helge, I can still reproduce this one  ^^^
> I'll fuzz it some more, but so far I am not finding anything else.

I've now updated the patch which does address all issues you found
so far. It's attached below.

If you like you can pull the full series from
https://github.com/hdeller/qemu-hppa/commits/target-hppa

I'd be happy if you could recheck if you find anything else.

In the next step I need to check if everything still works on HP-UX. If
that's Ok, I'd like to send out a new pull request.  If you could give
your Acked-by or Reviewed-by it would be great.

Thanks!
Helge

---
From 1657a7a95adc15552138c2b4d310a06128093892 Mon Sep 17 00:00:00 2001
From: Helge Deller 
Date: Tue, 4 Aug 2020 15:35:38 +0200
Subject: [PATCH] hw/display/artist: Prevent out of VRAM buffer accesses

Simplify various bounds checks by changing parameters like row and column
numbers to become unsigned instead of signed.
With that we can check if the calculated offset is bigger than the size of the
VRAM region and bail out if not.

Reported-by: LLVM libFuzzer
Reported-by: Alexander Bulekov 
Buglink: https://bugs.launchpad.net/qemu/+bug/1880326
Buglink: https://bugs.launchpad.net/qemu/+bug/1890310
Buglink: https://bugs.launchpad.net/qemu/+bug/1890311
Buglink: https://bugs.launchpad.net/qemu/+bug/1890312
Buglink: https://bugs.launchpad.net/qemu/+bug/1890370
Signed-off-by: Helge Deller 

diff --git a/hw/display/artist.c b/hw/display/artist.c
index 47de17b9e9..570811030f 100644
--- a/hw/display/artist.c
+++ b/hw/display/artist.c
@@ -35,9 +35,9 @@
 struct vram_buffer {
 MemoryRegion mr;
 uint8_t *data;
-int size;
-int width;
-int height;
+unsigned int size;
+unsigned int width;
+unsigned int height;
 };

 typedef struct ARTISTState {
@@ -203,14 +203,18 @@ static int16_t artist_get_y(uint32_t reg)
 }

 static void artist_invalidate_lines(struct vram_buffer *buf,
-int starty, int height)
+unsigned int starty, unsigned int height)
 {
-int start = starty * buf->width;
-int size = height * buf->width;
+unsigned int start, size;

-if (start + size <= buf->size) {
-memory_region_set_dirty(&buf->mr, start, size);
+if (starty >= buf->height) {
+return;
 }
+
+start = starty * buf->width;
+size = MIN(height * buf->width, buf->size - start);
+
+memory_region_set_dirty(&buf->mr, start, size);
 }

 static int vram_write_pix_per_transfer(ARTISTState *s)
@@ -274,15 +278,15 @@ static artist_rop_t artist_get_op(ARTISTState *s)
 }

 static void artist_rop8(ARTISTState *s, struct vram_buffer *buf,
-int offset, uint8_t val)
+unsigned int offset, uint8_t val)
 {
 const artist_rop_t op = artist_get_op(s);
 uint8_t plane_mask;
 uint8_t *dst;

-if (offset < 0 || offset >= buf->size) {
+if (offset >= buf->size) {
 qemu_log_mask(LOG_GUEST_ERROR,
-  "rop8 offset:%d bufsize:%u\n", offset, buf->size);
+  "rop8 offset:%u bufsize:%u\n", offset, buf->size);
 return;
 }
 dst = buf->data + offset;
@@ -294,8 +298,7 @@ static void artist_rop8(ARTISTState *s, struct vram_buffer 
*buf,
 break;

 case ARTIST_ROP_COPY:
-*dst &= ~plane_mask;
-*dst |= val & plane_mask;
+*dst = (*dst & ~plane_mask) | (val & plane_mask);
 break;

 case ARTIST_ROP_XOR:
@@ -349,7 +352,8 @@ static void vram_bit_write(ARTISTState *s, int posx, int 
posy, bool incr_x,
 {
 struct vram_buffer *buf;
 uint32_t vram_bitmask = s->vram_bitmask;
-int mask, i, pix_count, pix_length, offset, width;
+int mask, i, pix_count, pix_length;
+unsigned int offset, width;
 uint8_t *data8, *p;

 pix_count = vram_write_pix_per_transfer(s);
@@ -364,8 +368,7 @@ static void vram_bit_write(ARTISTState *s, int posx, int 
posy, bool incr_x,
 offset = posy * width + posx;
 }

-if (!buf->size) {
-qemu_log("write to non-existent buffer\n");
+if (!buf->size || offset >= buf->size) {
 return;
 }

@@ -394,7 +397,9 @@ static void vram_bit_write(ARTISTState *s, int posx, int 
posy, bool incr_x,

 case 3:
 if (s->cmap_bm_access) {
-*(uint32_t *)(p + offset) = data;
+if (offset + 3 < buf->size) {
+*(uint32_t *)(p + offset) = data;
+}
 break;
 }
 data8 = (uint8_t *)&data;
@@ -464,12 +469,14 @@ static void vram_bit_write(ARTISTState *s, int posx, int 
po

Re: [PATCH] ide: Get rid of IDEDrive struct

[Peter Maydell]

On Wed, 5 Aug 2020 at 20:49, Eduardo Habkost  wrote:
>
> The struct had a single field (IDEDevice dev), and is only used
> in the QOM type declarations and property lists.  We can simply
> use the IDEDevice struct directly instead.
>
> Signed-off-by: Eduardo Habkost 
> @@ -327,7 +323,6 @@ static void ide_hd_class_init(ObjectClass *klass, void 
> *data)
>  static const TypeInfo ide_hd_info = {
>  .name  = "ide-hd",
>  .parent= TYPE_IDE_DEVICE,
> -.instance_size = sizeof(IDEDrive),
>  .class_init= ide_hd_class_init,
>  };

This is one of those areas where this change works and reduces
amount of code, but on the other hand it means the QOM type
doesn't follow the common pattern for a leaf type of:
 * it has a struct
 * it has cast macros that cast to that struct
 * the typeinfo instance_size is the size of that struct
(it wasn't exactly following this pattern before, of course).

We define in https://wiki.qemu.org/Documentation/QOMConventions
(in the 'When to create class types and macros' bit at the bottom)
what we expect for whether to provide class cast macros/a
class struct/class_size in the TypeInfo, essentially recommending
that types follow one of two patterns (simple leaf class with no
methods or class members, vs everything else) even if in a
particular case you could take a short-cut and not define
everything. We haven't really defined similar "this is the
standard pattern, provide it all even if you don't strictly
need it" rules for the instance struct/macros. Maybe we should?

Just a thought, not a nak; I know we have quite a number
of types that take this kind of "we don't really need to
provide all the standard QOM macros/structs/etc" approach
(some of which I wrote!).

thanks
-- PMM

[Bug 1890370] Re: Segfault in artist vram_bit_write

[Helge Deller]

** Changed in: qemu
 Assignee: (unassigned) => Helge Deller (hdeller)

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1890370

Title:
  Segfault in artist vram_bit_write

Status in QEMU:
  Invalid

Bug description:
  Hello,
  Reproducer:

  cat << EOF | ./hppa-softmmu/qemu-system-hppa -m 64 -display none \
  -qtest stdio -accel qtest
  writeq 0xf810049f 0x
  writew 0xf8118001 0xff7c
  writew 0xf8118000 0x8300
  writeq 0xf81005fb 0x5c18006400189e
  EOF

  
  SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior 
/hw/display/artist.c:402:17 in
  AddressSanitizer:DEADLYSIGNAL
  =
  ==23157==ERROR: AddressSanitizer: SEGV on unknown address 0x7f17563f (pc 
0x560ce3ad742c bp 0x7ffe310c62e0 sp 0x7ffe310c5a60 T0)
  ==23157==The signal is caused by a WRITE memory access.
  #0 0x560ce3ad742c in vram_bit_write /hw/display/artist.c:402:43
  #1 0x560ce3acf2ab in artist_reg_write /hw/display/artist.c:892:9
  #2 0x560ce31c37a3 in memory_region_write_accessor /softmmu/memory.c:483:5
  #3 0x560ce31c2adc in access_with_adjusted_size /softmmu/memory.c:539:18
  #4 0x560ce31c0873 in memory_region_dispatch_write 
/softmmu/memory.c:1466:16
  #5 0x560ce286e056 in flatview_write_continue /exec.c:3176:23
  #6 0x560ce2856866 in flatview_write /exec.c:3216:14
  #7 0x560ce2856387 in address_space_write /exec.c:3308:18
  #8 0x560ce326a604 in qtest_process_command /softmmu/qtest.c:452:13
  #9 0x560ce3261c08 in qtest_process_inbuf /softmmu/qtest.c:710:9
  #10 0x560ce3260895 in qtest_read /softmmu/qtest.c:722:5
  #11 0x560ce571d343 in qemu_chr_be_write_impl /chardev/char.c:188:9
  #12 0x560ce571d4c7 in qemu_chr_be_write /chardev/char.c:200:9
  #13 0x560ce57317b3 in fd_chr_read /chardev/char-fd.c:68:9
  #14 0x560ce5885b74 in qio_channel_fd_source_dispatch 
/io/channel-watch.c:84:12
  #15 0x7f1665259897 in g_main_context_dispatch 
(/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4e897)
  #16 0x560ce5c7da7b in glib_pollfds_poll /util/main-loop.c:217:9
  #17 0x560ce5c7b1ab in os_host_main_loop_wait /util/main-loop.c:240:5
  #18 0x560ce5c7ab44 in main_loop_wait /util/main-loop.c:516:11
  #19 0x560ce3282d00 in qemu_main_loop /softmmu/vl.c:1676:9
  #20 0x560ce58bd961 in main /softmmu/main.c:49:5
  #21 0x7f1663ddfe0a in __libc_start_main 
/build/glibc-GwnBeO/glibc-2.30/csu/../csu/libc-start.c:308:16
  #22 0x560ce2761729 in _start 
(/home/alxndr/Development/qemu/general-fuzz/build/hppa-softmmu/qemu-system-hppa+0x22d4729)

  
  With -trace artist\*

  [I 1596601002.853158] OPENED
  [R +0.047035] writeq 0xf810049f 0x
  24590@1596601002.900238:artist_reg_write 1 0x10049f <- 0xff
  24590@1596601002.900258:artist_reg_write 4 0x1004a0 VRAM_IDX <- 0x
  24590@1596601002.900269:artist_reg_write 2 0x1004a4 <- 0x
  24590@1596601002.900280:artist_reg_write 1 0x1004a6 <- 0xff
  OK
  [S +0.047130] OK
  [R +0.047159] writew 0xf8118001 0xff7c
  24590@1596601002.900331:artist_reg_write 1 0x118001 CMAP_BM_ACCESS <- 0xff
  24590@1596601002.900344:artist_reg_write 1 0x118002 CMAP_BM_ACCESS <- 0x7c
  OK
  [S +0.047194] OK
  [R +0.047213] writew 0xf8118000 0x8300
  24590@1596601002.900383:artist_reg_write 2 0x118000 CMAP_BM_ACCESS <- 0x8300
  OK
  [S +0.047231] OK
  [R +0.047243] writeq 0xf81005fb 0x5c18006400189e
  24590@1596601002.900410:artist_reg_write 1 0x1005fb <- 0x0
  24590@1596601002.900418:artist_reg_write 4 0x1005fc <- 0x5c180064
  24590@1596601002.900424:artist_reg_write 2 0x100600 VRAM_WRITE_INCR_X <- 0x18
  /home/alxndr/Development/qemu/general-fuzz/hw/display/artist.c:402:17: 
runtime error: store to misaligned address 0x7fd01d3f for type 'uint32_t' 
(aka 'unsigned int'), which requires 4 byte alignment
  0x7fd01d3f: note: pointer points here
  
  SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior 
/home/alxndr/Development/qemu/general-fuzz/hw/display/artist.c:402:17 in
  AddressSanitizer:DEADLYSIGNAL

  -Alex

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1890370/+subscriptions

Re: [PATCH v3 0/8] Generalize start-powered-off property from ARM

[Thiago Jung Bauermann]

Peter Maydell  writes:

> On Wed, 5 Aug 2020 at 18:01, Thiago Jung Bauermann
>  wrote:
>> Any news on this? Is there something I should be doing? I saw -rc3 today
>> but not these patches.
>
> Sorry, you've missed the bus for 5.1 at this point. I'd assumed
> that the relevant bits of the patchset would go into a PPC pullreq
> if it was important for 5.1.
>
> As I understand it, this isn't a regression from 5.0, right?

Right, it isn't.

-- 
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [PATCH v1 02/21] target/alpha: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

On Wed, 5 Aug 2020 at 15:18, Richard Henderson
 wrote:
>
> On 8/5/20 11:12 AM, Robert Foley wrote:
> > @@ -299,8 +299,12 @@ void alpha_cpu_do_interrupt(CPUState *cs)
> >  {
> >  AlphaCPU *cpu = ALPHA_CPU(cs);
> >  CPUAlphaState *env = &cpu->env;
> > -int i = cs->exception_index;
> > -
> > +int i;
> > +bool bql = !qemu_mutex_iothread_locked();
> > +if (bql) {
> > +qemu_mutex_lock_iothread();
> > +}
>
> Why does this patch for alpha need to check qemu_mutex_iothread_locked and the
> next patch for arm does not?
>

In alpha (and arm) the do_interrupt function can be called separately or by
cpu_exec_interrupt.  In the case where do_interrupt gets called separately
it needs to take the BQL (bql == true).
In the case where cpu_exec_interrupt is holding the BQL, and calls do_interrupt,
do_interrupt needs to check qemu_mutex_iothread_locked, and in this case not get
the lock (bql == false).

The next patch for arm, checks qemu_mutex_iothread_locked in its do_interrupt
function, but not in its cpu_exec_interrupt function, the same pattern
as for alpha.

Thanks & Regards,
-Rob

>
> r~

[PATCH] ide: Get rid of IDEDrive struct

[Eduardo Habkost]

The struct had a single field (IDEDevice dev), and is only used
in the QOM type declarations and property lists.  We can simply
use the IDEDevice struct directly instead.

Signed-off-by: Eduardo Habkost 
---
 hw/ide/qdev.c | 25 +
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 27ff1f7f66..dd3867d8b3 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -157,10 +157,6 @@ int ide_get_bios_chs_trans(BusState *bus, int unit)
 
 /* - */
 
-typedef struct IDEDrive {
-IDEDevice dev;
-} IDEDrive;
-
 static void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp)
 {
 IDEBus *bus = DO_UPCAST(IDEBus, qbus, dev->qdev.parent_bus);
@@ -297,19 +293,19 @@ static void ide_drive_realize(IDEDevice *dev, Error 
**errp)
 }
 
 #define DEFINE_IDE_DEV_PROPERTIES() \
-DEFINE_BLOCK_PROPERTIES(IDEDrive, dev.conf),\
-DEFINE_BLOCK_ERROR_PROPERTIES(IDEDrive, dev.conf),  \
-DEFINE_PROP_STRING("ver",  IDEDrive, dev.version),  \
-DEFINE_PROP_UINT64("wwn",  IDEDrive, dev.wwn, 0),   \
-DEFINE_PROP_STRING("serial",  IDEDrive, dev.serial),\
-DEFINE_PROP_STRING("model", IDEDrive, dev.model)
+DEFINE_BLOCK_PROPERTIES(IDEDevice, conf),\
+DEFINE_BLOCK_ERROR_PROPERTIES(IDEDevice, conf),  \
+DEFINE_PROP_STRING("ver",  IDEDevice, version),  \
+DEFINE_PROP_UINT64("wwn",  IDEDevice, wwn, 0),   \
+DEFINE_PROP_STRING("serial",  IDEDevice, serial),\
+DEFINE_PROP_STRING("model", IDEDevice, model)
 
 static Property ide_hd_properties[] = {
 DEFINE_IDE_DEV_PROPERTIES(),
-DEFINE_BLOCK_CHS_PROPERTIES(IDEDrive, dev.conf),
+DEFINE_BLOCK_CHS_PROPERTIES(IDEDevice, conf),
 DEFINE_PROP_BIOS_CHS_TRANS("bios-chs-trans",
-IDEDrive, dev.chs_trans, BIOS_ATA_TRANSLATION_AUTO),
-DEFINE_PROP_UINT16("rotation_rate", IDEDrive, dev.rotation_rate, 0),
+IDEDevice, chs_trans, BIOS_ATA_TRANSLATION_AUTO),
+DEFINE_PROP_UINT16("rotation_rate", IDEDevice, rotation_rate, 0),
 DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -327,7 +323,6 @@ static void ide_hd_class_init(ObjectClass *klass, void 
*data)
 static const TypeInfo ide_hd_info = {
 .name  = "ide-hd",
 .parent= TYPE_IDE_DEVICE,
-.instance_size = sizeof(IDEDrive),
 .class_init= ide_hd_class_init,
 };
 
@@ -350,7 +345,6 @@ static void ide_cd_class_init(ObjectClass *klass, void 
*data)
 static const TypeInfo ide_cd_info = {
 .name  = "ide-cd",
 .parent= TYPE_IDE_DEVICE,
-.instance_size = sizeof(IDEDrive),
 .class_init= ide_cd_class_init,
 };
 
@@ -373,7 +367,6 @@ static void ide_drive_class_init(ObjectClass *klass, void 
*data)
 static const TypeInfo ide_drive_info = {
 .name  = "ide-drive",
 .parent= TYPE_IDE_DEVICE,
-.instance_size = sizeof(IDEDrive),
 .class_init= ide_drive_class_init,
 };
 
-- 
2.26.2

Re: [PATCH v1 01/21] accel/tcg: Change interrupt/exception handling to remove implied BQL

[Richard Henderson]

On 8/5/20 11:12 AM, Robert Foley wrote:
> This change removes the implied BQL from the cpu_handle_interrupt,
> and cpu_handle_exception paths. This BQL acquire is being pushed
> down into the per arch implementation.
> 
> Signed-off-by: Robert Foley 
> ---
>  accel/tcg/cpu-exec.c | 19 +++
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index 80d0e649b2..8e2bfd97a1 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -517,9 +517,7 @@ static inline bool cpu_handle_exception(CPUState *cpu, 
> int *ret)
>  #else
>  if (replay_exception()) {
>  CPUClass *cc = CPU_GET_CLASS(cpu);
> -qemu_mutex_lock_iothread();
>  cc->do_interrupt(cpu);
> -qemu_mutex_unlock_iothread();
>  cpu->exception_index = -1;
>  

This patch is not bisectable.  The removal of the lock here needs to happen at
the end, or something.


r~

Re: [PATCH v1 02/21] target/alpha: add BQL to do_interrupt and cpu_exec_interrupt

[Richard Henderson]

On 8/5/20 11:12 AM, Robert Foley wrote:
> @@ -299,8 +299,12 @@ void alpha_cpu_do_interrupt(CPUState *cs)
>  {
>  AlphaCPU *cpu = ALPHA_CPU(cs);
>  CPUAlphaState *env = &cpu->env;
> -int i = cs->exception_index;
> -
> +int i;
> +bool bql = !qemu_mutex_iothread_locked();
> +if (bql) {
> +qemu_mutex_lock_iothread();
> +}

Why does this patch for alpha need to check qemu_mutex_iothread_locked and the
next patch for arm does not?


r~

Re: v8.1M cpu emulation and target-arm feature-identification strategy

[Peter Maydell]

On Wed, 5 Aug 2020 at 18:00, Richard Henderson
 wrote:
> Older ones like XSCALE are obvious

Looking at the XScale manual we could actually implement
ARM_FEATURE_XSCALE as (cpu->midr & 0x == 0x6905)
[Vendor=intel, arch=ARMv5TE], and ARM_FEATURE_IWMMXT as
(cpu->midr & 0xe000 == 0x69054000) [Vendor=intel,
arch=ARMv5TE, Core Generation=2]... Doesn't really gain
us much, of course :-)

thanks
-- PMM

Re: [PATCH v3 0/8] Generalize start-powered-off property from ARM

[Peter Maydell]

On Wed, 5 Aug 2020 at 18:01, Thiago Jung Bauermann
 wrote:
> Any news on this? Is there something I should be doing? I saw -rc3 today
> but not these patches.

Sorry, you've missed the bus for 5.1 at this point. I'd assumed
that the relevant bits of the patchset would go into a PPC pullreq
if it was important for 5.1.

As I understand it, this isn't a regression from 5.0, right?

thanks
-- PMM

Re: v8.1M cpu emulation and target-arm feature-identification strategy

[Peter Maydell]

On Wed, 5 Aug 2020 at 18:00, Richard Henderson
 wrote:
> I've always assumed we'd never get rid of all of them.
>
> Older ones like XSCALE are obvious, but I don't think there's a clear 
> indicator
> for V{5,6,7,8} either.

MIDR.Architecture lets you distinguish v4/v4T/v5/v5T/v5TE/v5TEJ/v6,
and there are also some separate per-feature ID register fields for
things which we currently hang off those ARM_FEATURE_Vx flags.
In theory all the v7-and-later stuff should have its own ID register
field...

Regardless, it's hard to see a clear benefit from a hypothetical
concerted effort to convert all the ARM_FEATURE_* uses to ID
register checks, though we might choose to convert a few here
and there if we need to overhaul the code anyway.

thanks
-- PMM

[RFC PATCH] travis.yml: Drop the default softmmu builds

[Thomas Huth]

The total runtime of all Travis jobs is very long and we are testing
all softmmu targets in the gitlab-CI already - so we can speed up the
Travis testing a little bit by not testing the softmmu targets here
anymore.

Signed-off-by: Thomas Huth 
---
 Well, ok, we do not test all the softmmu targets on gitlab-CI with
 that same ancient version of Ubuntu ... but do we still care about
 testing all softmmut targets on Ubuntu Xenial at all? ... at least
 according to our support policy, we do not care about Xenial anymore.

 .travis.yml | 14 --
 1 file changed, 14 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index 6695c0620f..18290bc51d 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -123,20 +123,6 @@ jobs:
 - CONFIG="--disable-system --static"
 - CACHE_NAME="${TRAVIS_BRANCH}-linux-gcc-default"
 
-
-# we split the system builds as it takes a while to build them all
-- name: "GCC (main-softmmu)"
-  env:
-- CONFIG="--disable-user --target-list=${MAIN_SOFTMMU_TARGETS}"
-- CACHE_NAME="${TRAVIS_BRANCH}-linux-gcc-default"
-
-
-- name: "GCC (other-softmmu)"
-  env:
-   - CONFIG="--disable-user --target-list-exclude=${MAIN_SOFTMMU_TARGETS}"
-- CACHE_NAME="${TRAVIS_BRANCH}-linux-gcc-default"
-
-
 # Just build tools and run minimal unit and softfloat checks
 - name: "GCC check-softfloat (user)"
   env:
-- 
2.18.1

[PATCH 2/2] travis.yml: Drop the Python 3.5 and 3.6 builds

[Thomas Huth]

Python 3.5 is already the default in Ubuntu Xenial (which we use for
most jobs on Travis), and Python 3.6 is the default on Ubuntu Bionic
(which we use for the s390x jobs on Travis for example already), so
explicitely defining tests for Python 3.5 and 3.6 seems redundant.

Signed-off-by: Thomas Huth 
---
 .travis.yml | 17 -
 1 file changed, 17 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index 18290bc51d..b4c603f0ec 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -258,23 +258,6 @@ jobs:
 - TEST_CMD=""
 
 
-# Python builds
-- name: "GCC Python 3.5 (x86_64-softmmu)"
-  env:
-- CONFIG="--target-list=x86_64-softmmu"
-- CACHE_NAME="${TRAVIS_BRANCH}-linux-gcc-default"
-  language: python
-  python: 3.5
-
-
-- name: "GCC Python 3.6 (x86_64-softmmu)"
-  env:
-- CONFIG="--target-list=x86_64-softmmu"
-- CACHE_NAME="${TRAVIS_BRANCH}-linux-gcc-default"
-  language: python
-  python: 3.6
-
-
 # Using newer GCC with sanitizers
 - name: "GCC9 with sanitizers (softmmu)"
   addons:
-- 
2.18.1

[PATCH v1 21/21] target/xtensa: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley 
---
 target/xtensa/exc_helper.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/xtensa/exc_helper.c b/target/xtensa/exc_helper.c
index 01d1e56feb..fd33a56847 100644
--- a/target/xtensa/exc_helper.c
+++ b/target/xtensa/exc_helper.c
@@ -200,6 +200,7 @@ void xtensa_cpu_do_interrupt(CPUState *cs)
 XtensaCPU *cpu = XTENSA_CPU(cs);
 CPUXtensaState *env = &cpu->env;
 
+qemu_mutex_lock_iothread();
 if (cs->exception_index == EXC_IRQ) {
 qemu_log_mask(CPU_LOG_INT,
   "%s(EXC_IRQ) level = %d, cintlevel = %d, "
@@ -252,6 +253,7 @@ void xtensa_cpu_do_interrupt(CPUState *cs)
 break;
 }
 check_interrupts(env);
+qemu_mutex_unlock_iothread();
 }
 #else
 void xtensa_cpu_do_interrupt(CPUState *cs)
-- 
2.17.1

[PATCH v1 16/21] target/rx: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley 
---
 target/rx/helper.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/target/rx/helper.c b/target/rx/helper.c
index a6a337a311..a456b727ed 100644
--- a/target/rx/helper.c
+++ b/target/rx/helper.c
@@ -48,6 +48,10 @@ void rx_cpu_do_interrupt(CPUState *cs)
 CPURXState *env = &cpu->env;
 int do_irq = cs->interrupt_request & INT_FLAGS;
 uint32_t save_psw;
+bool bql = !qemu_mutex_iothread_locked();
+if (bql) {
+qemu_mutex_lock_iothread();
+}
 
 env->in_sleep = 0;
 
@@ -117,6 +121,9 @@ void rx_cpu_do_interrupt(CPUState *cs)
   (vec & 0xff), expname);
 }
 env->regs[0] = env->isp;
+if (bql) {
+qemu_mutex_unlock_iothread();
+}
 }
 
 bool rx_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
@@ -124,6 +131,7 @@ bool rx_cpu_exec_interrupt(CPUState *cs, int 
interrupt_request)
 RXCPU *cpu = RXCPU(cs);
 CPURXState *env = &cpu->env;
 int accept = 0;
+qemu_mutex_lock_iothread();
 /* hardware interrupt (Normal) */
 if ((interrupt_request & CPU_INTERRUPT_HARD) &&
 env->psw_i && (env->psw_ipl < env->req_ipl)) {
@@ -138,8 +146,10 @@ bool rx_cpu_exec_interrupt(CPUState *cs, int 
interrupt_request)
 }
 if (accept) {
 rx_cpu_do_interrupt(cs);
+qemu_mutex_unlock_iothread();
 return true;
 }
+qemu_mutex_unlock_iothread();
 return false;
 }
 
-- 
2.17.1

[PATCH v1 17/21] target/s390x: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley 
---
 target/s390x/excp_helper.c | 12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c
index dde7afc2f0..b215b4a4a7 100644
--- a/target/s390x/excp_helper.c
+++ b/target/s390x/excp_helper.c
@@ -470,7 +470,10 @@ void s390_cpu_do_interrupt(CPUState *cs)
 S390CPU *cpu = S390_CPU(cs);
 CPUS390XState *env = &cpu->env;
 bool stopped = false;
-
+bool bql = !qemu_mutex_iothread_locked();
+if (bql) {
+qemu_mutex_lock_iothread();
+}
 qemu_log_mask(CPU_LOG_INT, "%s: %d at psw=%" PRIx64 ":%" PRIx64 "\n",
   __func__, cs->exception_index, env->psw.mask, env->psw.addr);
 
@@ -541,10 +544,14 @@ try_deliver:
 /* unhalt if we had a WAIT PSW somehwere in our injection chain */
 s390_cpu_unhalt(cpu);
 }
+if (bql) {
+qemu_mutex_unlock_iothread();
+}
 }
 
 bool s390_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 {
+qemu_mutex_lock_iothread();
 if (interrupt_request & CPU_INTERRUPT_HARD) {
 S390CPU *cpu = S390_CPU(cs);
 CPUS390XState *env = &cpu->env;
@@ -552,10 +559,12 @@ bool s390_cpu_exec_interrupt(CPUState *cs, int 
interrupt_request)
 if (env->ex_value) {
 /* Execution of the target insn is indivisible from
the parent EXECUTE insn.  */
+qemu_mutex_unlock_iothread();
 return false;
 }
 if (s390_cpu_has_int(cpu)) {
 s390_cpu_do_interrupt(cs);
+qemu_mutex_unlock_iothread();
 return true;
 }
 if (env->psw.mask & PSW_MASK_WAIT) {
@@ -564,6 +573,7 @@ bool s390_cpu_exec_interrupt(CPUState *cs, int 
interrupt_request)
 cpu_interrupt(CPU(cpu), CPU_INTERRUPT_HALT);
 }
 }
+qemu_mutex_unlock_iothread();
 return false;
 }
 
-- 
2.17.1

[PATCH v1 08/21] target/lm32: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley 
---
 target/lm32/helper.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/target/lm32/helper.c b/target/lm32/helper.c
index 1130fc8884..4439b06ecc 100644
--- a/target/lm32/helper.c
+++ b/target/lm32/helper.c
@@ -152,6 +152,10 @@ void lm32_cpu_do_interrupt(CPUState *cs)
 {
 LM32CPU *cpu = LM32_CPU(cs);
 CPULM32State *env = &cpu->env;
+bool bql = !qemu_mutex_iothread_locked();
+if (bql) {
+qemu_mutex_lock_iothread();
+}
 
 qemu_log_mask(CPU_LOG_INT,
 "exception at pc=%x type=%x\n", env->pc, cs->exception_index);
@@ -196,18 +200,24 @@ void lm32_cpu_do_interrupt(CPUState *cs)
   cs->exception_index);
 break;
 }
+if (bql) {
+qemu_mutex_unlock_iothread();
+}
 }
 
 bool lm32_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 {
 LM32CPU *cpu = LM32_CPU(cs);
 CPULM32State *env = &cpu->env;
+qemu_mutex_lock_iothread();
 
 if ((interrupt_request & CPU_INTERRUPT_HARD) && (env->ie & IE_IE)) {
 cs->exception_index = EXCP_IRQ;
 lm32_cpu_do_interrupt(cs);
+qemu_mutex_unlock_iothread();
 return true;
 }
+qemu_mutex_unlock_iothread();
 return false;
 }
 
-- 
2.17.1

[PATCH v1 14/21] target/ppc: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley 
---
 target/ppc/excp_helper.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index bf9e1e27e9..4530230d65 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -870,7 +870,9 @@ void ppc_cpu_do_interrupt(CPUState *cs)
 PowerPCCPU *cpu = POWERPC_CPU(cs);
 CPUPPCState *env = &cpu->env;
 
+qemu_mutex_lock_iothread();
 powerpc_excp(cpu, env->excp_model, cs->exception_index);
+qemu_mutex_unlock_iothread();
 }
 
 static void ppc_hw_interrupt(CPUPPCState *env)
@@ -1056,14 +1058,17 @@ bool ppc_cpu_exec_interrupt(CPUState *cs, int 
interrupt_request)
 {
 PowerPCCPU *cpu = POWERPC_CPU(cs);
 CPUPPCState *env = &cpu->env;
+qemu_mutex_lock_iothread();
 
 if (interrupt_request & CPU_INTERRUPT_HARD) {
 ppc_hw_interrupt(env);
 if (env->pending_interrupts == 0) {
 cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
 }
+qemu_mutex_unlock_iothread();
 return true;
 }
+qemu_mutex_unlock_iothread();
 return false;
 }
 
-- 
2.17.1

[PATCH v1 20/21] target/unicore32: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley 
---
 target/unicore32/helper.c  | 3 +++
 target/unicore32/softmmu.c | 7 +++
 2 files changed, 10 insertions(+)

diff --git a/target/unicore32/helper.c b/target/unicore32/helper.c
index 54c26871fe..d79284d224 100644
--- a/target/unicore32/helper.c
+++ b/target/unicore32/helper.c
@@ -169,6 +169,7 @@ void helper_cp1_putc(target_ulong regval)
 
 bool uc32_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 {
+qemu_mutex_lock_iothread();
 if (interrupt_request & CPU_INTERRUPT_HARD) {
 UniCore32CPU *cpu = UNICORE32_CPU(cs);
 CPUUniCore32State *env = &cpu->env;
@@ -176,8 +177,10 @@ bool uc32_cpu_exec_interrupt(CPUState *cs, int 
interrupt_request)
 if (!(env->uncached_asr & ASR_I)) {
 cs->exception_index = UC32_EXCP_INTR;
 uc32_cpu_do_interrupt(cs);
+qemu_mutex_unlock_iothread();
 return true;
 }
 }
+qemu_mutex_unlock_iothread();
 return false;
 }
diff --git a/target/unicore32/softmmu.c b/target/unicore32/softmmu.c
index 9660bd2a27..ca9b92aad0 100644
--- a/target/unicore32/softmmu.c
+++ b/target/unicore32/softmmu.c
@@ -81,6 +81,10 @@ void uc32_cpu_do_interrupt(CPUState *cs)
 CPUUniCore32State *env = &cpu->env;
 uint32_t addr;
 int new_mode;
+bool bql = !qemu_mutex_iothread_locked();
+if (bql) {
+qemu_mutex_lock_iothread();
+}
 
 switch (cs->exception_index) {
 case UC32_EXCP_PRIV:
@@ -118,6 +122,9 @@ void uc32_cpu_do_interrupt(CPUState *cs)
 env->regs[30] = env->regs[31];
 env->regs[31] = addr;
 cpu_interrupt_request_or(cs, CPU_INTERRUPT_EXITTB);
+if (bql) {
+qemu_mutex_unlock_iothread();
+}
 }
 
 static int get_phys_addr_ucv2(CPUUniCore32State *env, uint32_t address,
-- 
2.17.1

[PATCH v1 18/21] target/sh4: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley 
---
 target/sh4/helper.c | 13 +++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/target/sh4/helper.c b/target/sh4/helper.c
index 1e32365c75..c4d5b9a374 100644
--- a/target/sh4/helper.c
+++ b/target/sh4/helper.c
@@ -62,8 +62,11 @@ void superh_cpu_do_interrupt(CPUState *cs)
 {
 SuperHCPU *cpu = SUPERH_CPU(cs);
 CPUSH4State *env = &cpu->env;
-int do_irq = cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD;
-int do_exp, irq_vector = cs->exception_index;
+int do_irq;
+int do_exp, irq_vector;
+qemu_mutex_lock_iothread();
+do_irq = cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD;
+irq_vector = cs->exception_index;
 
 /* prioritize exceptions over interrupts */
 
@@ -79,9 +82,11 @@ void superh_cpu_do_interrupt(CPUState *cs)
should be loaded with the kernel entry point.
qemu_system_reset_request takes care of that.  */
 qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+qemu_mutex_unlock_iothread();
 return;
 }
 if (do_irq && !env->in_sleep) {
+qemu_mutex_unlock_iothread();
 return; /* masked */
 }
 }
@@ -91,6 +96,7 @@ void superh_cpu_do_interrupt(CPUState *cs)
 irq_vector = sh_intc_get_pending_vector(env->intc_handle,
(env->sr >> 4) & 0xf);
 if (irq_vector == -1) {
+qemu_mutex_unlock_iothread();
 return; /* masked */
}
 }
@@ -180,14 +186,17 @@ void superh_cpu_do_interrupt(CPUState *cs)
 env->pc = env->vbr + 0x100;
 break;
 }
+qemu_mutex_unlock_iothread();
 return;
 }
 
 if (do_irq) {
 env->intevt = irq_vector;
 env->pc = env->vbr + 0x600;
+qemu_mutex_unlock_iothread();
 return;
 }
+qemu_mutex_unlock_iothread();
 }
 
 static void update_itlb_use(CPUSH4State * env, int itlbnb)
-- 
2.17.1

[PATCH v1 19/21] target/sparc: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley 
---
 target/sparc/cpu.c  |  3 +++
 target/sparc/int32_helper.c | 13 -
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
index 20c7c0c434..13b5a038e8 100644
--- a/target/sparc/cpu.c
+++ b/target/sparc/cpu.c
@@ -79,6 +79,7 @@ static void sparc_cpu_reset(DeviceState *dev)
 
 static bool sparc_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 {
+qemu_mutex_lock_iothread();
 if (interrupt_request & CPU_INTERRUPT_HARD) {
 SPARCCPU *cpu = SPARC_CPU(cs);
 CPUSPARCState *env = &cpu->env;
@@ -90,10 +91,12 @@ static bool sparc_cpu_exec_interrupt(CPUState *cs, int 
interrupt_request)
 if (type != TT_EXTINT || cpu_pil_allowed(env, pil)) {
 cs->exception_index = env->interrupt_index;
 sparc_cpu_do_interrupt(cs);
+qemu_mutex_unlock_iothread();
 return true;
 }
 }
 }
+qemu_mutex_unlock_iothread();
 return false;
 }
 
diff --git a/target/sparc/int32_helper.c b/target/sparc/int32_helper.c
index 9a71e1abd8..3940e945ed 100644
--- a/target/sparc/int32_helper.c
+++ b/target/sparc/int32_helper.c
@@ -69,7 +69,12 @@ void sparc_cpu_do_interrupt(CPUState *cs)
 {
 SPARCCPU *cpu = SPARC_CPU(cs);
 CPUSPARCState *env = &cpu->env;
-int cwp, intno = cs->exception_index;
+int cwp, intno;
+bool bql = !qemu_mutex_iothread_locked();
+if (bql) {
+qemu_mutex_lock_iothread();
+}
+intno = cs->exception_index;
 
 /* Compute PSR before exposing state.  */
 if (env->cc_op != CC_OP_FLAGS) {
@@ -115,6 +120,9 @@ void sparc_cpu_do_interrupt(CPUState *cs)
   "Error state",
   cs->exception_index, excp_name_str(cs->exception_index));
 }
+if (bql) {
+qemu_mutex_unlock_iothread();
+}
 return;
 }
 #endif
@@ -136,6 +144,9 @@ void sparc_cpu_do_interrupt(CPUState *cs)
 env->qemu_irq_ack(env, env->irq_manager, intno);
 }
 #endif
+if (bql) {
+qemu_mutex_unlock_iothread();
+}
 }
 
 #if !defined(CONFIG_USER_ONLY)
-- 
2.17.1

[PATCH v1 07/21] target/i386: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley 
---
 target/i386/seg_helper.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/target/i386/seg_helper.c b/target/i386/seg_helper.c
index 818f65f35f..114d4a0d24 100644
--- a/target/i386/seg_helper.c
+++ b/target/i386/seg_helper.c
@@ -1284,7 +1284,7 @@ void x86_cpu_do_interrupt(CPUState *cs)
 {
 X86CPU *cpu = X86_CPU(cs);
 CPUX86State *env = &cpu->env;
-
+qemu_mutex_lock_iothread();
 #if defined(CONFIG_USER_ONLY)
 /* if user mode only, we simulate a fake exception
which will be handled outside the cpu execution
@@ -1308,6 +1308,7 @@ void x86_cpu_do_interrupt(CPUState *cs)
 env->old_exception = -1;
 }
 #endif
+qemu_mutex_unlock_iothread();
 }
 
 void do_interrupt_x86_hardirq(CPUX86State *env, int intno, int is_hw)
@@ -1320,9 +1321,10 @@ bool x86_cpu_exec_interrupt(CPUState *cs, int 
interrupt_request)
 X86CPU *cpu = X86_CPU(cs);
 CPUX86State *env = &cpu->env;
 int intno;
-
+qemu_mutex_lock_iothread();
 interrupt_request = x86_cpu_pending_interrupt(cs, interrupt_request);
 if (!interrupt_request) {
+qemu_mutex_unlock_iothread();
 return false;
 }
 
@@ -1377,6 +1379,7 @@ bool x86_cpu_exec_interrupt(CPUState *cs, int 
interrupt_request)
 }
 
 /* Ensure that no TB jump will be modified as the program flow was 
changed.  */
+qemu_mutex_unlock_iothread();
 return true;
 }
 
-- 
2.17.1

[PATCH v1 13/21] target/openrisc: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley 
---
 target/openrisc/interrupt.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/target/openrisc/interrupt.c b/target/openrisc/interrupt.c
index 3eab771dcd..361f242954 100644
--- a/target/openrisc/interrupt.c
+++ b/target/openrisc/interrupt.c
@@ -28,6 +28,10 @@
 
 void openrisc_cpu_do_interrupt(CPUState *cs)
 {
+bool bql = !qemu_mutex_iothread_locked();
+if (bql) {
+qemu_mutex_lock_iothread();
+}
 #ifndef CONFIG_USER_ONLY
 OpenRISCCPU *cpu = OPENRISC_CPU(cs);
 CPUOpenRISCState *env = &cpu->env;
@@ -99,6 +103,9 @@ void openrisc_cpu_do_interrupt(CPUState *cs)
 #endif
 
 cs->exception_index = -1;
+if (bql) {
+qemu_mutex_unlock_iothread();
+}
 }
 
 bool openrisc_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
@@ -106,6 +113,7 @@ bool openrisc_cpu_exec_interrupt(CPUState *cs, int 
interrupt_request)
 OpenRISCCPU *cpu = OPENRISC_CPU(cs);
 CPUOpenRISCState *env = &cpu->env;
 int idx = -1;
+qemu_mutex_lock_iothread();
 
 if ((interrupt_request & CPU_INTERRUPT_HARD) && (env->sr & SR_IEE)) {
 idx = EXCP_INT;
@@ -116,7 +124,9 @@ bool openrisc_cpu_exec_interrupt(CPUState *cs, int 
interrupt_request)
 if (idx >= 0) {
 cs->exception_index = idx;
 openrisc_cpu_do_interrupt(cs);
+qemu_mutex_unlock_iothread();
 return true;
 }
+qemu_mutex_unlock_iothread();
 return false;
 }
-- 
2.17.1

[PATCH v1 05/21] target/cris: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley 
---
 target/cris/helper.c | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/target/cris/helper.c b/target/cris/helper.c
index 67946d9246..22aecde0f5 100644
--- a/target/cris/helper.c
+++ b/target/cris/helper.c
@@ -45,8 +45,10 @@ void cris_cpu_do_interrupt(CPUState *cs)
 CRISCPU *cpu = CRIS_CPU(cs);
 CPUCRISState *env = &cpu->env;
 
+qemu_mutex_lock_iothread();
 cs->exception_index = -1;
 env->pregs[PR_ERP] = env->pc;
+qemu_mutex_unlock_iothread();
 }
 
 void crisv10_cpu_do_interrupt(CPUState *cs)
@@ -128,6 +130,10 @@ void crisv10_cpu_do_interrupt(CPUState *cs)
 CRISCPU *cpu = CRIS_CPU(cs);
 CPUCRISState *env = &cpu->env;
 int ex_vec = -1;
+bool bql = !qemu_mutex_iothread_locked();
+if (bql) {
+qemu_mutex_lock_iothread();
+}
 
 D_LOG("exception index=%d interrupt_req=%d\n",
   cs->exception_index,
@@ -183,6 +189,9 @@ void crisv10_cpu_do_interrupt(CPUState *cs)
   env->pregs[PR_CCS],
   env->pregs[PR_PID],
   env->pregs[PR_ERP]);
+if (bql) {
+qemu_mutex_unlock_iothread();
+}
 }
 
 void cris_cpu_do_interrupt(CPUState *cs)
@@ -190,6 +199,10 @@ void cris_cpu_do_interrupt(CPUState *cs)
 CRISCPU *cpu = CRIS_CPU(cs);
 CPUCRISState *env = &cpu->env;
 int ex_vec = -1;
+bool bql = !qemu_mutex_iothread_locked();
+if (bql) {
+qemu_mutex_lock_iothread();
+}
 
 D_LOG("exception index=%d interrupt_req=%d\n",
   cs->exception_index,
@@ -265,6 +278,9 @@ void cris_cpu_do_interrupt(CPUState *cs)
   env->pregs[PR_CCS],
   env->pregs[PR_PID],
   env->pregs[PR_ERP]);
+if (bql) {
+qemu_mutex_unlock_iothread();
+}
 }
 
 hwaddr cris_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
@@ -294,6 +310,7 @@ bool cris_cpu_exec_interrupt(CPUState *cs, int 
interrupt_request)
 CRISCPU *cpu = CRIS_CPU(cs);
 CPUCRISState *env = &cpu->env;
 bool ret = false;
+qemu_mutex_lock_iothread();
 
 if (interrupt_request & CPU_INTERRUPT_HARD
 && (env->pregs[PR_CCS] & I_FLAG)
@@ -315,6 +332,7 @@ bool cris_cpu_exec_interrupt(CPUState *cs, int 
interrupt_request)
 ret = true;
 }
 }
+qemu_mutex_unlock_iothread();
 
 return ret;
 }
-- 
2.17.1

[PATCH v1 15/21] target/riscv: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley 
---
 target/riscv/cpu_helper.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 75d2ae3434..5050802e95 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -80,14 +80,17 @@ bool riscv_cpu_exec_interrupt(CPUState *cs, int 
interrupt_request)
 {
 #if !defined(CONFIG_USER_ONLY)
 if (interrupt_request & CPU_INTERRUPT_HARD) {
+qemu_mutex_lock_iothread();
 RISCVCPU *cpu = RISCV_CPU(cs);
 CPURISCVState *env = &cpu->env;
 int interruptno = riscv_cpu_local_irq_pending(env);
 if (interruptno >= 0) {
 cs->exception_index = RISCV_EXCP_INT_FLAG | interruptno;
 riscv_cpu_do_interrupt(cs);
+qemu_mutex_unlock_iothread();
 return true;
 }
+qemu_mutex_unlock_iothread();
 }
 #endif
 return false;
@@ -822,6 +825,10 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int 
size,
  */
 void riscv_cpu_do_interrupt(CPUState *cs)
 {
+bool bql = !qemu_mutex_iothread_locked();
+if (bql) {
+qemu_mutex_lock_iothread();
+}
 #if !defined(CONFIG_USER_ONLY)
 
 RISCVCPU *cpu = RISCV_CPU(cs);
@@ -982,4 +989,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
 
 #endif
 cs->exception_index = EXCP_NONE; /* mark handled to qemu */
+if (bql) {
+qemu_mutex_unlock_iothread();
+}
 }
-- 
2.17.1

[PATCH v1 09/21] target/m68k: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley 
---
 target/m68k/op_helper.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/target/m68k/op_helper.c b/target/m68k/op_helper.c
index 4a032a150e..0c476a 100644
--- a/target/m68k/op_helper.c
+++ b/target/m68k/op_helper.c
@@ -448,7 +448,9 @@ void m68k_cpu_do_interrupt(CPUState *cs)
 M68kCPU *cpu = M68K_CPU(cs);
 CPUM68KState *env = &cpu->env;
 
+qemu_mutex_lock_iothread();
 do_interrupt_all(env, 0);
+qemu_mutex_unlock_iothread();
 }
 
 static inline void do_interrupt_m68k_hardirq(CPUM68KState *env)
@@ -508,6 +510,7 @@ bool m68k_cpu_exec_interrupt(CPUState *cs, int 
interrupt_request)
 {
 M68kCPU *cpu = M68K_CPU(cs);
 CPUM68KState *env = &cpu->env;
+qemu_mutex_lock_iothread();
 
 if (interrupt_request & CPU_INTERRUPT_HARD
 && ((env->sr & SR_I) >> SR_I_SHIFT) < env->pending_level) {
@@ -519,8 +522,10 @@ bool m68k_cpu_exec_interrupt(CPUState *cs, int 
interrupt_request)
  */
 cs->exception_index = env->pending_vector;
 do_interrupt_m68k_hardirq(env);
+qemu_mutex_unlock_iothread();
 return true;
 }
+qemu_mutex_unlock_iothread();
 return false;
 }
 
-- 
2.17.1

[PATCH v1 11/21] target/mips: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley 
---
 target/mips/helper.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/target/mips/helper.c b/target/mips/helper.c
index afd78b1990..6595d18702 100644
--- a/target/mips/helper.c
+++ b/target/mips/helper.c
@@ -1085,6 +1085,10 @@ static inline void set_badinstr_registers(CPUMIPSState 
*env)
 
 void mips_cpu_do_interrupt(CPUState *cs)
 {
+bool bql = !qemu_mutex_iothread_locked();
+if (bql) {
+qemu_mutex_lock_iothread();
+}
 #if !defined(CONFIG_USER_ONLY)
 MIPSCPU *cpu = MIPS_CPU(cs);
 CPUMIPSState *env = &cpu->env;
@@ -1396,10 +1400,14 @@ void mips_cpu_do_interrupt(CPUState *cs)
 }
 #endif
 cs->exception_index = EXCP_NONE;
+if (bql) {
+qemu_mutex_unlock_iothread();
+}
 }
 
 bool mips_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 {
+qemu_mutex_lock_iothread();
 if (interrupt_request & CPU_INTERRUPT_HARD) {
 MIPSCPU *cpu = MIPS_CPU(cs);
 CPUMIPSState *env = &cpu->env;
@@ -1410,9 +1418,11 @@ bool mips_cpu_exec_interrupt(CPUState *cs, int 
interrupt_request)
 cs->exception_index = EXCP_EXT_INTERRUPT;
 env->error_code = 0;
 mips_cpu_do_interrupt(cs);
+qemu_mutex_unlock_iothread();
 return true;
 }
 }
+qemu_mutex_unlock_iothread();
 return false;
 }
 
-- 
2.17.1

[PATCH v1 12/21] target/nios2: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley 
---
 target/nios2/cpu.c| 3 +++
 target/nios2/helper.c | 8 +++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/target/nios2/cpu.c b/target/nios2/cpu.c
index fe5fd9adfd..fd05406eac 100644
--- a/target/nios2/cpu.c
+++ b/target/nios2/cpu.c
@@ -102,13 +102,16 @@ static bool nios2_cpu_exec_interrupt(CPUState *cs, int 
interrupt_request)
 {
 Nios2CPU *cpu = NIOS2_CPU(cs);
 CPUNios2State *env = &cpu->env;
+qemu_mutex_lock_iothread();
 
 if ((interrupt_request & CPU_INTERRUPT_HARD) &&
 (env->regs[CR_STATUS] & CR_STATUS_PIE)) {
 cs->exception_index = EXCP_IRQ;
 nios2_cpu_do_interrupt(cs);
+qemu_mutex_unlock_iothread();
 return true;
 }
+qemu_mutex_unlock_iothread();
 return false;
 }
 
diff --git a/target/nios2/helper.c b/target/nios2/helper.c
index 57c97bde3c..46d53551d4 100644
--- a/target/nios2/helper.c
+++ b/target/nios2/helper.c
@@ -52,7 +52,10 @@ void nios2_cpu_do_interrupt(CPUState *cs)
 {
 Nios2CPU *cpu = NIOS2_CPU(cs);
 CPUNios2State *env = &cpu->env;
-
+bool bql = !qemu_mutex_iothread_locked();
+if (bql) {
+qemu_mutex_lock_iothread();
+}
 switch (cs->exception_index) {
 case EXCP_IRQ:
 assert(env->regs[CR_STATUS] & CR_STATUS_PIE);
@@ -198,6 +201,9 @@ void nios2_cpu_do_interrupt(CPUState *cs)
   cs->exception_index);
 break;
 }
+if (bql) {
+qemu_mutex_unlock_iothread();
+}
 }
 
 hwaddr nios2_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
-- 
2.17.1

[PATCH v1 06/21] target/hppa: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley 
---
 target/hppa/int_helper.c | 25 +++--
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/target/hppa/int_helper.c b/target/hppa/int_helper.c
index 462747baf8..eda40bc5d9 100644
--- a/target/hppa/int_helper.c
+++ b/target/hppa/int_helper.c
@@ -94,12 +94,20 @@ void hppa_cpu_do_interrupt(CPUState *cs)
 {
 HPPACPU *cpu = HPPA_CPU(cs);
 CPUHPPAState *env = &cpu->env;
-int i = cs->exception_index;
-target_ureg iaoq_f = env->iaoq_f;
-target_ureg iaoq_b = env->iaoq_b;
-uint64_t iasq_f = env->iasq_f;
-uint64_t iasq_b = env->iasq_b;
-
+int i;
+target_ureg iaoq_f;
+target_ureg iaoq_b;
+uint64_t iasq_f;
+uint64_t iasq_b;
+bool bql = !qemu_mutex_iothread_locked();
+if (bql) {
+qemu_mutex_lock_iothread();
+}
+i = cs->exception_index;
+iaoq_f = env->iaoq_f;
+iaoq_b = env->iaoq_b;
+iasq_f = env->iasq_f;
+iasq_b = env->iasq_b;
 #ifndef CONFIG_USER_ONLY
 target_ureg old_psw;
 
@@ -244,6 +252,9 @@ void hppa_cpu_do_interrupt(CPUState *cs)
env->cr[CR_IOR]));
 }
 cs->exception_index = -1;
+if (bql) {
+qemu_mutex_unlock_iothread();
+}
 }
 
 bool hppa_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
@@ -251,6 +262,7 @@ bool hppa_cpu_exec_interrupt(CPUState *cs, int 
interrupt_request)
 #ifndef CONFIG_USER_ONLY
 HPPACPU *cpu = HPPA_CPU(cs);
 CPUHPPAState *env = &cpu->env;
+qemu_mutex_lock_iothread();
 
 /* If interrupts are requested and enabled, raise them.  */
 if ((env->psw & PSW_I) && (interrupt_request & CPU_INTERRUPT_HARD)) {
@@ -258,6 +270,7 @@ bool hppa_cpu_exec_interrupt(CPUState *cs, int 
interrupt_request)
 hppa_cpu_do_interrupt(cs);
 return true;
 }
+qemu_mutex_unlock_iothread();
 #endif
 return false;
 }
-- 
2.17.1

[PATCH v1 10/21] target/microblaze: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley 
---
 target/microblaze/helper.c | 20 
 1 file changed, 20 insertions(+)

diff --git a/target/microblaze/helper.c b/target/microblaze/helper.c
index ab2ceeb055..ae8ff2bea4 100644
--- a/target/microblaze/helper.c
+++ b/target/microblaze/helper.c
@@ -32,10 +32,17 @@ void mb_cpu_do_interrupt(CPUState *cs)
 {
 MicroBlazeCPU *cpu = MICROBLAZE_CPU(cs);
 CPUMBState *env = &cpu->env;
+bool bql = !qemu_mutex_iothread_locked();
+if (bql) {
+qemu_mutex_lock_iothread();
+}
 
 cs->exception_index = -1;
 env->res_addr = RES_ADDR_NONE;
 env->regs[14] = env->sregs[SR_PC];
+if (bql) {
+qemu_mutex_unlock_iothread();
+}
 }
 
 bool mb_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
@@ -113,6 +120,10 @@ void mb_cpu_do_interrupt(CPUState *cs)
 MicroBlazeCPU *cpu = MICROBLAZE_CPU(cs);
 CPUMBState *env = &cpu->env;
 uint32_t t;
+bool bql = !qemu_mutex_iothread_locked();
+if (bql) {
+qemu_mutex_lock_iothread();
+}
 
 /* IMM flag cannot propagate across a branch and into the dslot.  */
 assert(!((env->iflags & D_FLAG) && (env->iflags & IMM_FLAG)));
@@ -123,6 +134,9 @@ void mb_cpu_do_interrupt(CPUState *cs)
 case EXCP_HW_EXCP:
 if (!(env->pvr.regs[0] & PVR0_USE_EXC_MASK)) {
 qemu_log_mask(LOG_GUEST_ERROR, "Exception raised on system 
without exceptions!\n");
+if (bql) {
+qemu_mutex_unlock_iothread();
+}
 return;
 }
 
@@ -262,6 +276,9 @@ void mb_cpu_do_interrupt(CPUState *cs)
   cs->exception_index);
 break;
 }
+if (bql) {
+qemu_mutex_unlock_iothread();
+}
 }
 
 hwaddr mb_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
@@ -291,6 +308,7 @@ bool mb_cpu_exec_interrupt(CPUState *cs, int 
interrupt_request)
 {
 MicroBlazeCPU *cpu = MICROBLAZE_CPU(cs);
 CPUMBState *env = &cpu->env;
+qemu_mutex_lock_iothread();
 
 if ((interrupt_request & CPU_INTERRUPT_HARD)
 && (env->sregs[SR_MSR] & MSR_IE)
@@ -298,7 +316,9 @@ bool mb_cpu_exec_interrupt(CPUState *cs, int 
interrupt_request)
 && !(env->iflags & (D_FLAG | IMM_FLAG))) {
 cs->exception_index = EXCP_IRQ;
 mb_cpu_do_interrupt(cs);
+qemu_mutex_unlock_iothread();
 return true;
 }
+qemu_mutex_unlock_iothread();
 return false;
 }
-- 
2.17.1

[PATCH v1 03/21] target/arm: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley 
---
 target/arm/cpu.c| 13 ++---
 target/arm/helper.c | 17 -
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 401832ea95..b8544f0f0a 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -528,12 +528,17 @@ bool arm_cpu_exec_interrupt(CPUState *cs, int 
interrupt_request)
 {
 CPUClass *cc = CPU_GET_CLASS(cs);
 CPUARMState *env = cs->env_ptr;
-uint32_t cur_el = arm_current_el(env);
-bool secure = arm_is_secure(env);
-uint64_t hcr_el2 = arm_hcr_el2_eff(env);
+uint32_t cur_el;
+bool secure;
+uint64_t hcr_el2;
 uint32_t target_el;
 uint32_t excp_idx;
 
+qemu_mutex_lock_iothread();
+cur_el = arm_current_el(env);
+secure = arm_is_secure(env);
+hcr_el2 = arm_hcr_el2_eff(env);
+
 /* The prioritization of interrupts is IMPLEMENTATION DEFINED. */
 
 if (interrupt_request & CPU_INTERRUPT_FIQ) {
@@ -568,12 +573,14 @@ bool arm_cpu_exec_interrupt(CPUState *cs, int 
interrupt_request)
 goto found;
 }
 }
+qemu_mutex_unlock_iothread();
 return false;
 
  found:
 cs->exception_index = excp_idx;
 env->exception.target_el = target_el;
 cc->do_interrupt(cs);
+qemu_mutex_unlock_iothread();
 return true;
 }
 
diff --git a/target/arm/helper.c b/target/arm/helper.c
index c5ea2c25ea..3a22d40598 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -9759,7 +9759,13 @@ void arm_cpu_do_interrupt(CPUState *cs)
 {
 ARMCPU *cpu = ARM_CPU(cs);
 CPUARMState *env = &cpu->env;
-unsigned int new_el = env->exception.target_el;
+unsigned int new_el;
+
+bool bql = !qemu_mutex_iothread_locked();
+if (bql) {
+qemu_mutex_lock_iothread();
+}
+new_el = env->exception.target_el;
 
 assert(!arm_feature(env, ARM_FEATURE_M));
 
@@ -9776,6 +9782,9 @@ void arm_cpu_do_interrupt(CPUState *cs)
 if (arm_is_psci_call(cpu, cs->exception_index)) {
 arm_handle_psci_call(cpu);
 qemu_log_mask(CPU_LOG_INT, "...handled as PSCI call\n");
+if (bql) {
+qemu_mutex_unlock_iothread();
+}
 return;
 }
 
@@ -9787,6 +9796,9 @@ void arm_cpu_do_interrupt(CPUState *cs)
 #ifdef CONFIG_TCG
 if (cs->exception_index == EXCP_SEMIHOST) {
 handle_semihosting(cs);
+if (bql) {
+qemu_mutex_unlock_iothread();
+}
 return;
 }
 #endif
@@ -9808,6 +9820,9 @@ void arm_cpu_do_interrupt(CPUState *cs)
 if (!kvm_enabled()) {
 cpu_interrupt_request_or(cs, CPU_INTERRUPT_EXITTB);
 }
+if (bql) {
+qemu_mutex_unlock_iothread();
+}
 }
 #endif /* !CONFIG_USER_ONLY */
 
-- 
2.17.1

[PATCH v1 01/21] accel/tcg: Change interrupt/exception handling to remove implied BQL

[Robert Foley]

This change removes the implied BQL from the cpu_handle_interrupt,
and cpu_handle_exception paths. This BQL acquire is being pushed
down into the per arch implementation.

Signed-off-by: Robert Foley 
---
 accel/tcg/cpu-exec.c | 19 +++
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 80d0e649b2..8e2bfd97a1 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -517,9 +517,7 @@ static inline bool cpu_handle_exception(CPUState *cpu, int 
*ret)
 #else
 if (replay_exception()) {
 CPUClass *cc = CPU_GET_CLASS(cpu);
-qemu_mutex_lock_iothread();
 cc->do_interrupt(cpu);
-qemu_mutex_unlock_iothread();
 cpu->exception_index = -1;
 
 if (unlikely(cpu->singlestep_enabled)) {
@@ -558,7 +556,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
 if (unlikely(cpu_interrupt_request(cpu))) {
 int interrupt_request;
 
-qemu_mutex_lock_iothread();
+cpu_mutex_lock(cpu);
 interrupt_request = cpu_interrupt_request(cpu);
 if (unlikely(cpu->singlestep_enabled & SSTEP_NOIRQ)) {
 /* Mask out external interrupts for this step. */
@@ -567,7 +565,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
 if (interrupt_request & CPU_INTERRUPT_DEBUG) {
 cpu_reset_interrupt(cpu, CPU_INTERRUPT_DEBUG);
 cpu->exception_index = EXCP_DEBUG;
-qemu_mutex_unlock_iothread();
+cpu_mutex_unlock(cpu);
 return true;
 }
 if (replay_mode == REPLAY_MODE_PLAY && !replay_has_interrupt()) {
@@ -577,13 +575,15 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
 cpu_reset_interrupt(cpu, CPU_INTERRUPT_HALT);
 cpu_halted_set(cpu, 1);
 cpu->exception_index = EXCP_HLT;
-qemu_mutex_unlock_iothread();
+cpu_mutex_unlock(cpu);
 return true;
 }
 #if defined(TARGET_I386)
 else if (interrupt_request & CPU_INTERRUPT_INIT) {
 X86CPU *x86_cpu = X86_CPU(cpu);
 CPUArchState *env = &x86_cpu->env;
+cpu_mutex_unlock(cpu);
+qemu_mutex_lock_iothread();
 replay_interrupt();
 cpu_svm_check_intercept_param(env, SVM_EXIT_INIT, 0, 0);
 do_cpu_init(x86_cpu);
@@ -595,7 +595,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
 else if (interrupt_request & CPU_INTERRUPT_RESET) {
 replay_interrupt();
 cpu_reset(cpu);
-qemu_mutex_unlock_iothread();
+cpu_mutex_unlock(cpu);
 return true;
 }
 #endif
@@ -604,7 +604,9 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
True when it is, and we should restart on a new TB,
and via longjmp via cpu_loop_exit.  */
 else {
+cpu_mutex_unlock(cpu);
 if (cc->cpu_exec_interrupt(cpu, interrupt_request)) {
+cpu_mutex_lock(cpu);
 replay_interrupt();
 /*
  * After processing the interrupt, ensure an EXCP_DEBUG is
@@ -614,6 +616,8 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
 cpu->exception_index =
 (cpu->singlestep_enabled ? EXCP_DEBUG : -1);
 *last_tb = NULL;
+} else {
+cpu_mutex_lock(cpu);
 }
 /* The target hook may have updated the 'cpu->interrupt_request';
  * reload the 'interrupt_request' value */
@@ -627,7 +631,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
 }
 
 /* If we exit via cpu_loop_exit/longjmp it is reset in cpu_exec */
-qemu_mutex_unlock_iothread();
+cpu_mutex_unlock(cpu);
 }
 
 /* Finally, check if we need to exit to the main loop.  */
@@ -691,7 +695,6 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, 
TranslationBlock *tb,
 }
 #endif
 }
-
 /* main execution loop */
 
 int cpu_exec(CPUState *cpu)
-- 
2.17.1

[PATCH v1 00/21] accel/tcg: remove implied BQL from cpu_handle_interrupt/exception path

[Robert Foley]

The purpose of this change is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

The BQL is a bottleneck in scaling to more cores.
And this cpu_handle_interrupt/exception path is one of
the key BQL users as measured by the QEMU sync profiling (qsp).

We have chosen to break up the process of removing
BQL from this path into two pieces:

1) Changes to the core/common functions of cpu_handle_interrupt/exception
   to drop the holding of the BQL. The holding of the BQL is pushed down
   to the per-arch implementation code.
   This set of changes is handled in this patch.

   This approach of pushing the BQL down to the per arch functions was
   suggested by Paolo Bonzini.
   For reference, here are two key posts in the discussion, explaining
   the reasoning/benefits of this approach.
   https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
   https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

2) Removing the BQL from the per-arch functions.
   Since the arch now has the code that grabs the BQL, each arch can
   change its use of the BQL for interrupts independently.
   We leave it up to the arch to make the change at the time that makes sense.

It is worth mentioning that we are working on per-arch changes
in line with 2), and plan to submit these.
In other words, we plan to set the groundwork with this
patch series and then will take advantage of it in later series.

This patch series is based on the per-CPU locks patch:
https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg05314.html

Robert Foley (21):
  accel/tcg:  Change interrupt/exception handling to remove implied BQL
  target/alpha: add BQL to do_interrupt and cpu_exec_interrupt
  target/arm: add BQL to do_interrupt and cpu_exec_interrupt
  target/avr: add BQL to do_interrupt and cpu_exec_interrupt
  target/cris: add BQL to do_interrupt and cpu_exec_interrupt
  target/hppa: add BQL to do_interrupt and cpu_exec_interrupt
  target/i386: add BQL to do_interrupt and cpu_exec_interrupt
  target/lm32: add BQL to do_interrupt and cpu_exec_interrupt
  target/m68k: add BQL to do_interrupt and cpu_exec_interrupt
  target/microblaze: add BQL to do_interrupt and cpu_exec_interrupt
  target/mips: add BQL to do_interrupt and cpu_exec_interrupt
  target/nios2: add BQL to do_interrupt and cpu_exec_interrupt
  target/openrisc: add BQL to do_interrupt and cpu_exec_interrupt
  target/ppc: add BQL to do_interrupt and cpu_exec_interrupt
  target/riscv: add BQL to do_interrupt and cpu_exec_interrupt
  target/rx: add BQL to do_interrupt and cpu_exec_interrupt
  target/s390x: add BQL to do_interrupt and cpu_exec_interrupt
  target/sh4: add BQL to do_interrupt and cpu_exec_interrupt
  target/sparc: add BQL to do_interrupt and cpu_exec_interrupt
  target/unicore32: add BQL to do_interrupt and cpu_exec_interrupt
  target/xtensa: add BQL to do_interrupt and cpu_exec_interrupt

 accel/tcg/cpu-exec.c| 19 +++
 target/alpha/helper.c   | 15 +--
 target/arm/cpu.c| 13 ++---
 target/arm/helper.c | 17 -
 target/avr/helper.c | 12 +++-
 target/cris/helper.c| 18 ++
 target/hppa/int_helper.c| 25 +++--
 target/i386/seg_helper.c|  7 +--
 target/lm32/helper.c| 10 ++
 target/m68k/op_helper.c |  5 +
 target/microblaze/helper.c  | 20 
 target/mips/helper.c| 10 ++
 target/nios2/cpu.c  |  3 +++
 target/nios2/helper.c   |  8 +++-
 target/openrisc/interrupt.c | 10 ++
 target/ppc/excp_helper.c|  5 +
 target/riscv/cpu_helper.c   | 10 ++
 target/rx/helper.c  | 10 ++
 target/s390x/excp_helper.c  | 12 +++-
 target/sh4/helper.c | 13 +++--
 target/sparc/cpu.c  |  3 +++
 target/sparc/int32_helper.c | 13 -
 target/unicore32/helper.c   |  3 +++
 target/unicore32/softmmu.c  |  7 +++
 target/xtensa/exc_helper.c  |  2 ++
 25 files changed, 242 insertions(+), 28 deletions(-)

-- 
2.17.1

[PATCH v1 02/21] target/alpha: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley 
---
 target/alpha/helper.c | 15 +--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/target/alpha/helper.c b/target/alpha/helper.c
index 55d7274d94..18169ae1c5 100644
--- a/target/alpha/helper.c
+++ b/target/alpha/helper.c
@@ -299,8 +299,12 @@ void alpha_cpu_do_interrupt(CPUState *cs)
 {
 AlphaCPU *cpu = ALPHA_CPU(cs);
 CPUAlphaState *env = &cpu->env;
-int i = cs->exception_index;
-
+int i;
+bool bql = !qemu_mutex_iothread_locked();
+if (bql) {
+qemu_mutex_lock_iothread();
+}
+i = cs->exception_index;
 if (qemu_loglevel_mask(CPU_LOG_INT)) {
 static int count;
 const char *name = "";
@@ -405,6 +409,9 @@ void alpha_cpu_do_interrupt(CPUState *cs)
 /* Switch to PALmode.  */
 env->flags |= ENV_FLAG_PAL_MODE;
 #endif /* !USER_ONLY */
+if (bql) {
+qemu_mutex_unlock_iothread();
+}
 }
 
 bool alpha_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
@@ -412,9 +419,11 @@ bool alpha_cpu_exec_interrupt(CPUState *cs, int 
interrupt_request)
 AlphaCPU *cpu = ALPHA_CPU(cs);
 CPUAlphaState *env = &cpu->env;
 int idx = -1;
+qemu_mutex_lock_iothread();
 
 /* We never take interrupts while in PALmode.  */
 if (env->flags & ENV_FLAG_PAL_MODE) {
+qemu_mutex_unlock_iothread();
 return false;
 }
 
@@ -446,8 +455,10 @@ bool alpha_cpu_exec_interrupt(CPUState *cs, int 
interrupt_request)
 cs->exception_index = idx;
 env->error_code = 0;
 alpha_cpu_do_interrupt(cs);
+qemu_mutex_unlock_iothread();
 return true;
 }
+qemu_mutex_unlock_iothread();
 return false;
 }
 
-- 
2.17.1

[PATCH v1 04/21] target/avr: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley 
---
 target/avr/helper.c | 12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/target/avr/helper.c b/target/avr/helper.c
index d96d14372b..f0d625c195 100644
--- a/target/avr/helper.c
+++ b/target/avr/helper.c
@@ -30,6 +30,7 @@ bool avr_cpu_exec_interrupt(CPUState *cs, int 
interrupt_request)
 CPUClass *cc = CPU_GET_CLASS(cs);
 AVRCPU *cpu = AVR_CPU(cs);
 CPUAVRState *env = &cpu->env;
+qemu_mutex_lock_iothread();
 
 if (interrupt_request & CPU_INTERRUPT_RESET) {
 if (cpu_interrupts_enabled(env)) {
@@ -53,6 +54,7 @@ bool avr_cpu_exec_interrupt(CPUState *cs, int 
interrupt_request)
 ret = true;
 }
 }
+qemu_mutex_unlock_iothread();
 return ret;
 }
 
@@ -61,10 +63,15 @@ void avr_cpu_do_interrupt(CPUState *cs)
 AVRCPU *cpu = AVR_CPU(cs);
 CPUAVRState *env = &cpu->env;
 
-uint32_t ret = env->pc_w;
+uint32_t ret;
 int vector = 0;
 int size = avr_feature(env, AVR_FEATURE_JMP_CALL) ? 2 : 1;
 int base = 0;
+bool bql = !qemu_mutex_iothread_locked();
+if (bql) {
+qemu_mutex_lock_iothread();
+}
+ret = env->pc_w;
 
 if (cs->exception_index == EXCP_RESET) {
 vector = 0;
@@ -87,6 +94,9 @@ void avr_cpu_do_interrupt(CPUState *cs)
 env->sregI = 0; /* clear Global Interrupt Flag */
 
 cs->exception_index = -1;
+if (bql) {
+qemu_mutex_unlock_iothread();
+}
 }
 
 int avr_cpu_memory_rw_debug(CPUState *cs, vaddr addr, uint8_t *buf,
-- 
2.17.1

RE: [PATCH v2 4/4] net/colo: Match is-enabled probe to tracepoint

[Zhang, Chen]

> -Original Message-
> From: Stefan Hajnoczi 
> Sent: Wednesday, August 5, 2020 6:53 PM
> To: Zhang, Chen 
> Cc: Roman Bolshakov ; Li Zhijian
> ; Jason Wang ; qemu-
> de...@nongnu.org; Cameron Esfahani ; Philippe
> Mathieu-Daudé ; Daniel Berrange
> 
> Subject: Re: [PATCH v2 4/4] net/colo: Match is-enabled probe to tracepoint
> 
> On Wed, Jul 29, 2020 at 01:34:52PM +0100, Daniel P. Berrangé wrote:
> > On Wed, Jul 29, 2020 at 03:33:22PM +0300, Roman Bolshakov wrote:
> > > On Tue, Jul 21, 2020 at 03:06:57PM +0100, Daniel P. Berrangé wrote:
> > > > On Sat, Jul 18, 2020 at 05:58:56PM +, Zhang, Chen wrote:
> > > > >
> > > > >
> > > > > > -Original Message-
> > > > > > From: Roman Bolshakov 
> > > > > > Sent: Friday, July 17, 2020 5:35 PM
> > > > > > To: qemu-devel@nongnu.org
> > > > > > Cc: Daniel P. Berrangé ; Stefan Hajnoczi
> > > > > > ; Cameron Esfahani ;
> > > > > > Roman Bolshakov ; Philippe
> > > > > > Mathieu-Daudé ; Zhang, Chen
> > > > > > ; Li Zhijian ;
> > > > > > Jason Wang 
> > > > > > Subject: [PATCH v2 4/4] net/colo: Match is-enabled probe to
> > > > > > tracepoint
> > > > > >
> > > > > > Build of QEMU with dtrace fails on macOS:
> > > > > >
> > > > > >   LINKx86_64-softmmu/qemu-system-x86_64
> > > > > > error: probe colo_compare_miscompare doesn't exist
> > > > > > error: Could not register probes
> > > > > > ld: error creating dtrace DOF section for architecture x86_64
> > > > > >
> > > > > > The reason of the error is explained by Adam Leventhal [1]:
> > > > > >
> > > > > >   Note that is-enabled probes don't have the stability magic so I'm
> not
> > > > > >   sure how things would work if only is-enabled probes were used.
> > > > > >
> > > > > > net/colo code uses is-enabled probes to determine if other
> > > > > > probes should be used but colo_compare_miscompare itself is not
> used explicitly.
> > > > > > Linker doesn't include the symbol and build fails.
> > > > > >
> > > > > > The issue can be resolved if is-enabled probe matches the
> > > > > > actual trace point that is used inside the test. Packet dump
> > > > > > toggle is replaced with a compile- time conditional definition.
> > > > > >
> > > > > > 1. http://markmail.org/message/6grq2ygr5nwdwsnb
> > > > > >
> > > > > > Fixes: f4b618360e ("colo-compare: add TCP, UDP, ICMP packet
> > > > > > comparison")
> > > > > > Cc: Philippe Mathieu-Daudé 
> > > > > > Cc: Cameron Esfahani 
> > > > > > Signed-off-by: Roman Bolshakov 
> > > > > > ---
> > > > > >  net/colo-compare.c| 42 ++---
> -
> > > > > >  net/filter-rewriter.c | 10 --
> > > > > >  net/trace-events  |  2 --
> > > > > >  3 files changed, 30 insertions(+), 24 deletions(-)
> > > >
> > > >
> > > > > >
> (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)
> > > > > > )
> > > > > > {
> > > > > > +if
> > > > > > +
> (trace_event_get_state_backends(TRACE_COLO_COMPARE_IP_INFO))
> > > > > > {
> > > > > >  char pri_ip_src[20], pri_ip_dst[20], sec_ip_src[20],
> > > > > > sec_ip_dst[20];
> > > > > >
> > > > > >  strcpy(pri_ip_src, inet_ntoa(ppkt->ip->ip_src)); @@
> > > > > > -492,12 +494,12 @@ sec:
> > > > > >  g_queue_push_head(&conn->primary_list, ppkt);
> > > > > >  g_queue_push_head(&conn->secondary_list, spkt);
> > > > > >
> > > > > > -if
> > > > > >
> (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)
> > > > > > )
> > > > > > {
> > > > > > -qemu_hexdump((char *)ppkt->data, stderr,
> > > > > > -"colo-compare ppkt", ppkt->size);
> > > > > > -qemu_hexdump((char *)spkt->data, stderr,
> > > > > > -"colo-compare spkt", spkt->size);
> > > > > > -}
> > > > > > +#ifdef DEBUG_COLO_PACKETS
> > > > > > +qemu_hexdump((char *)ppkt->data, stderr,
> > > > > > + "colo-compare ppkt", ppkt->size);
> > > > > > +qemu_hexdump((char *)spkt->data, stderr,
> > > > > > + "colo-compare spkt", spkt->size); #endif
> > > > > >
> > > > > >  colo_compare_inconsistency_notify(s);
> > > > > >  }
> > > > > > @@ -533,12 +535,12 @@ static int
> > > > > > colo_packet_compare_udp(Packet *spkt, Packet *ppkt)
> > > > > >  ppkt->size - offset)) {
> > > > > >  trace_colo_compare_udp_miscompare("primary pkt size",
> ppkt->size);
> > > > > >  trace_colo_compare_udp_miscompare("Secondary pkt
> > > > > > size", spkt-
> > > > > > >size);
> > > > > > -if
> > > > > >
> (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)
> > > > > > )
> > > > > > {
> > > > > > -qemu_hexdump((char *)ppkt->data, stderr, "colo-compare
> pri pkt",
> > > > > > - ppkt->size);
> > > > > > -qemu_hexdump((char *)spkt->data, stderr, "colo-compare
> sec pkt",
> > > > > > - spkt->size);
> > > > > > -}
> > > > > > +#ifdef DEBUG_CO

Re: [PATCH for-5.2 0/5] spapr: Cleanups for XIVE and PHB

[no-reply]

Patchew URL: 
https://patchew.org/QEMU/159664891296.638781.18417631893299150932.st...@bahia.lan/



Hi,

This series failed the docker-mingw@fedora 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
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===




The full log is available at
http://patchew.org/logs/159664891296.638781.18417631893299150932.st...@bahia.lan/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

[PATCH for-5.2 5/5] spapr: Simplify error handling in spapr_phb_realize()

[Greg Kurz]

The spapr_phb_realize() function has a local_err variable which
is used to:

1) check failures of spapr_irq_findone() and spapr_irq_claim()

2) prepend extra information to the error message

Recent work from Markus Armbruster highlighted we get better
code when testing the return value of a function, rather than
setting up all the local_err boiler plate. For similar reasons,
it is now preferred to use ERRP_GUARD() and error_prepend()
rather than error_propagate_prepend().

Since spapr_irq_findone() and spapr_irq_claim() return negative
values in case of failure, do both changes.

This is just cleanup, no functional impact.

Signed-off-by: Greg Kurz 
Reviewed-by: Markus Armbruster 
---
Note that the int32_t=>int followup change suggested by Markus was squashed
into this patch.
---
 hw/ppc/spapr_pci.c |   16 +++-
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 363cdb3f7b8d..0a418f1e6711 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1796,6 +1796,7 @@ static void spapr_phb_destroy_msi(gpointer opaque)
 
 static void spapr_phb_realize(DeviceState *dev, Error **errp)
 {
+ERRP_GUARD();
 /* We don't use SPAPR_MACHINE() in order to exit gracefully if the user
  * tries to add a sPAPR PHB to a non-pseries machine.
  */
@@ -1813,7 +1814,6 @@ static void spapr_phb_realize(DeviceState *dev, Error 
**errp)
 uint64_t msi_window_size = 4096;
 SpaprTceTable *tcet;
 const unsigned windows_supported = spapr_phb_windows_supported(sphb);
-Error *local_err = NULL;
 
 if (!spapr) {
 error_setg(errp, TYPE_SPAPR_PCI_HOST_BRIDGE " needs a pseries 
machine");
@@ -1964,13 +1964,12 @@ static void spapr_phb_realize(DeviceState *dev, Error 
**errp)
 
 /* Initialize the LSI table */
 for (i = 0; i < PCI_NUM_PINS; i++) {
-uint32_t irq = SPAPR_IRQ_PCI_LSI + sphb->index * PCI_NUM_PINS + i;
+int irq = SPAPR_IRQ_PCI_LSI + sphb->index * PCI_NUM_PINS + i;
 
 if (smc->legacy_irq_allocation) {
-irq = spapr_irq_findone(spapr, &local_err);
-if (local_err) {
-error_propagate_prepend(errp, local_err,
-"can't allocate LSIs: ");
+irq = spapr_irq_findone(spapr, errp);
+if (irq < 0) {
+error_prepend(errp, "can't allocate LSIs: ");
 /*
  * Older machines will never support PHB hotplug, ie, this is 
an
  * init only path and QEMU will terminate. No need to rollback.
@@ -1979,9 +1978,8 @@ static void spapr_phb_realize(DeviceState *dev, Error 
**errp)
 }
 }
 
-spapr_irq_claim(spapr, irq, true, &local_err);
-if (local_err) {
-error_propagate_prepend(errp, local_err, "can't allocate LSIs: ");
+if (spapr_irq_claim(spapr, irq, true, errp) < 0) {
+error_prepend(errp, "can't allocate LSIs: ");
 goto unrealize;
 }
 

[PATCH for-5.2 4/5] spapr/xive: Convert KVM device fd checks to assert()

[Greg Kurz]

All callers guard these functions with kvmppc_xive_in_kernel() or one
of its variants. Make it clear that these functions are only to be
called when the KVM XIVE device is active.

Note that the check on xive is dropped in kvmppc_xive_disconnect(). It
really cannot be NULL since it comes from set_active_intc() which only
passes pointers to allocated objects.

Signed-off-by: Greg Kurz 
---
 hw/intc/spapr_xive_kvm.c |   35 +++
 1 file changed, 7 insertions(+), 28 deletions(-)

diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
index a9657e2b0cda..3364832de83a 100644
--- a/hw/intc/spapr_xive_kvm.c
+++ b/hw/intc/spapr_xive_kvm.c
@@ -79,10 +79,7 @@ void kvmppc_xive_cpu_set_state(XiveTCTX *tctx, Error **errp)
 uint64_t state[2];
 int ret;
 
-/* The KVM XIVE device is not in use yet */
-if (xive->fd == -1) {
-return;
-}
+assert(xive->fd != -1);
 
 /* word0 and word1 of the OS ring. */
 state[0] = *((uint64_t *) &tctx->regs[TM_QW1_OS]);
@@ -101,10 +98,7 @@ void kvmppc_xive_cpu_get_state(XiveTCTX *tctx, Error **errp)
 uint64_t state[2] = { 0 };
 int ret;
 
-/* The KVM XIVE device is not in use */
-if (xive->fd == -1) {
-return;
-}
+assert(xive->fd != -1);
 
 ret = kvm_get_one_reg(tctx->cs, KVM_REG_PPC_VP_STATE, state);
 if (ret != 0) {
@@ -156,10 +150,7 @@ void kvmppc_xive_cpu_connect(XiveTCTX *tctx, Error **errp)
 unsigned long vcpu_id;
 int ret;
 
-/* The KVM XIVE device is not in use */
-if (xive->fd == -1) {
-return;
-}
+assert(xive->fd != -1);
 
 /* Check if CPU was hot unplugged and replugged. */
 if (kvm_cpu_is_enabled(tctx->cs)) {
@@ -245,10 +236,7 @@ int kvmppc_xive_source_reset_one(XiveSource *xsrc, int 
srcno, Error **errp)
 SpaprXive *xive = SPAPR_XIVE(xsrc->xive);
 uint64_t state = 0;
 
-/* The KVM XIVE device is not in use */
-if (xive->fd == -1) {
-return -ENODEV;
-}
+assert(xive->fd != -1);
 
 if (xive_source_irq_is_lsi(xsrc, srcno)) {
 state |= KVM_XIVE_LEVEL_SENSITIVE;
@@ -592,10 +580,7 @@ static void kvmppc_xive_change_state_handler(void *opaque, 
int running,
 
 void kvmppc_xive_synchronize_state(SpaprXive *xive, Error **errp)
 {
-/* The KVM XIVE device is not in use */
-if (xive->fd == -1) {
-return;
-}
+assert(xive->fd != -1);
 
 /*
  * When the VM is stopped, the sources are masked and the previous
@@ -622,10 +607,7 @@ int kvmppc_xive_pre_save(SpaprXive *xive)
 {
 Error *local_err = NULL;
 
-/* The KVM XIVE device is not in use */
-if (xive->fd == -1) {
-return 0;
-}
+assert(xive->fd != -1);
 
 /* EAT: there is no extra state to query from KVM */
 
@@ -845,10 +827,7 @@ void kvmppc_xive_disconnect(SpaprInterruptController *intc)
 XiveSource *xsrc;
 size_t esb_len;
 
-/* The KVM XIVE device is not in use */
-if (!xive || xive->fd == -1) {
-return;
-}
+assert(xive->fd != -1);
 
 /* Clear the KVM mapping */
 xsrc = &xive->source;

[PATCH for-5.2 3/5] ppc/xive: Introduce dedicated kvm_irqchip_in_kernel() wrappers

[Greg Kurz]

Calls to the KVM XIVE device are guarded by kvm_irqchip_in_kernel(). This
ensures that QEMU won't try to use the device if KVM is disabled or if
an in-kernel irqchip isn't required.

When using ic-mode=dual with the pseries machine, we have two possible
interrupt controllers: XIVE and XICS. The kvm_irqchip_in_kernel() helper
will return true as soon as any of the KVM device is created. It might
lure QEMU to think that the other one is also around, while it is not.
This is exactly what happens with ic-mode=dual at machine init when
claiming IRQ numbers, which must be done on all possible IRQ backends,
eg. RTAS event sources or the PHB0 LSI table : only the KVM XICS device
is active but we end up calling kvmppc_xive_source_reset_one() anyway,
which fails. This doesn't cause any trouble because of another bug :
kvmppc_xive_source_reset_one() lacks an error_setg() and callers don't
see the failure.

Most of the other kvmppc_xive_* functions have similar xive->fd
checks to filter out the case when KVM XIVE isn't active. It
might look safer to have idempotent functions but it doesn't
really help to understand what's going on when debugging.

Since we already have all the kvm_irqchip_in_kernel() in place,
also have the callers to check xive->fd as well before calling
KVM XIVE specific code. Introduce helpers to access the underlying
fd for various XIVE types and call them with a kvm_irqchip_in_kernel()
guard : this allows the compiler to optimize the kvmppc_xive_* calls
out if CONFIG_KVM isn't defined, thus avoiding the need for stubs.

Signed-off-by: Greg Kurz 
---
 hw/intc/spapr_xive.c|   39 +--
 hw/intc/spapr_xive_kvm.c|   15 +++
 hw/intc/xive.c  |   30 +++---
 include/hw/ppc/spapr_xive.h |1 +
 include/hw/ppc/xive.h   |2 ++
 5 files changed, 66 insertions(+), 21 deletions(-)

diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
index 89c8cd96670b..98e6489fb16d 100644
--- a/hw/intc/spapr_xive.c
+++ b/hw/intc/spapr_xive.c
@@ -148,12 +148,19 @@ static void spapr_xive_end_pic_print_info(SpaprXive 
*xive, XiveEND *end,
 xive_end_queue_pic_print_info(end, 6, mon);
 }
 
+/*
+ * kvm_irqchip_in_kernel() will cause the compiler to turn this
+ * info a nop if CONFIG_KVM isn't defined.
+ */
+#define kvmppc_xive_in_kernel(xive) \
+(kvm_irqchip_in_kernel() && kvmppc_xive_kernel_irqchip(xive))
+
 void spapr_xive_pic_print_info(SpaprXive *xive, Monitor *mon)
 {
 XiveSource *xsrc = &xive->source;
 int i;
 
-if (kvm_irqchip_in_kernel()) {
+if (kvmppc_xive_in_kernel(xive)) {
 Error *local_err = NULL;
 
 kvmppc_xive_synchronize_state(xive, &local_err);
@@ -507,8 +514,10 @@ static const VMStateDescription vmstate_spapr_xive_eas = {
 
 static int vmstate_spapr_xive_pre_save(void *opaque)
 {
-if (kvm_irqchip_in_kernel()) {
-return kvmppc_xive_pre_save(SPAPR_XIVE(opaque));
+SpaprXive *xive = SPAPR_XIVE(opaque);
+
+if (kvmppc_xive_in_kernel(xive)) {
+return kvmppc_xive_pre_save(xive);
 }
 
 return 0;
@@ -520,8 +529,10 @@ static int vmstate_spapr_xive_pre_save(void *opaque)
  */
 static int spapr_xive_post_load(SpaprInterruptController *intc, int version_id)
 {
-if (kvm_irqchip_in_kernel()) {
-return kvmppc_xive_post_load(SPAPR_XIVE(intc), version_id);
+SpaprXive *xive = SPAPR_XIVE(intc);
+
+if (kvmppc_xive_in_kernel(xive)) {
+return kvmppc_xive_post_load(xive, version_id);
 }
 
 return 0;
@@ -564,7 +575,7 @@ static int spapr_xive_claim_irq(SpaprInterruptController 
*intc, int lisn,
 xive_source_irq_set_lsi(xsrc, lisn);
 }
 
-if (kvm_irqchip_in_kernel()) {
+if (kvmppc_xive_in_kernel(xive)) {
 return kvmppc_xive_source_reset_one(xsrc, lisn, errp);
 }
 
@@ -641,7 +652,7 @@ static void spapr_xive_set_irq(SpaprInterruptController 
*intc, int irq, int val)
 {
 SpaprXive *xive = SPAPR_XIVE(intc);
 
-if (kvm_irqchip_in_kernel()) {
+if (kvmppc_xive_in_kernel(xive)) {
 kvmppc_xive_source_set_irq(&xive->source, irq, val);
 } else {
 xive_source_set_irq(&xive->source, irq, val);
@@ -749,7 +760,7 @@ static void spapr_xive_deactivate(SpaprInterruptController 
*intc)
 
 spapr_xive_mmio_set_enabled(xive, false);
 
-if (kvm_irqchip_in_kernel()) {
+if (kvmppc_xive_in_kernel(xive)) {
 kvmppc_xive_disconnect(intc);
 }
 }
@@ -1058,7 +1069,7 @@ static target_ulong h_int_set_source_config(PowerPCCPU 
*cpu,
 new_eas.w = xive_set_field64(EAS_END_DATA, new_eas.w, eisn);
 }
 
-if (kvm_irqchip_in_kernel()) {
+if (kvmppc_xive_in_kernel(xive)) {
 Error *local_err = NULL;
 
 kvmppc_xive_set_source_config(xive, lisn, &new_eas, &local_err);
@@ -1379,7 +1390,7 @@ static target_ulong h_int_set_queue_config(PowerPCCPU 
*cpu,
  */
 
 out:
-if (kvm_irqchip_in_kernel()) {
+if (kvmppc_xive_in_kernel(xive)) {
 Err

[PATCH for-5.2 2/5] spapr/xive: Simplify kvmppc_xive_disconnect()

[Greg Kurz]

Since this function begins with:

/* The KVM XIVE device is not in use */
if (!xive || xive->fd == -1) {
return;
}

we obviously don't need to check xive->fd again.

Signed-off-by: Greg Kurz 
---
 hw/intc/spapr_xive_kvm.c |6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
index d55ea4670e0e..893a1ee77e70 100644
--- a/hw/intc/spapr_xive_kvm.c
+++ b/hw/intc/spapr_xive_kvm.c
@@ -873,10 +873,8 @@ void kvmppc_xive_disconnect(SpaprInterruptController *intc)
  * and removed from the list of devices of the VM. The VCPU
  * presenters are also detached from the device.
  */
-if (xive->fd != -1) {
-close(xive->fd);
-xive->fd = -1;
-}
+close(xive->fd);
+xive->fd = -1;
 
 kvm_kernel_irqchip = false;
 kvm_msi_via_irqfd_allowed = false;

[PATCH for-5.2 1/5] spapr/xive: Fix xive->fd if kvm_create_device() fails

[Greg Kurz]

If the creation of the KVM XIVE device fails for some reasons, the
negative errno ends up in xive->fd, but the rest of the code assumes
that xive->fd either contains an open fd, ie. positive value, or -1.

This doesn't cause any misbehavior except kvmppc_xive_disconnect()
that will try to close(xive->fd) during rollback and likely be
rewarded with an EBADF.

Only set xive->fd with a open fd.

Signed-off-by: Greg Kurz 
---
 hw/intc/spapr_xive_kvm.c |8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
index edb7ee0e74f1..d55ea4670e0e 100644
--- a/hw/intc/spapr_xive_kvm.c
+++ b/hw/intc/spapr_xive_kvm.c
@@ -745,6 +745,7 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, 
uint32_t nr_servers,
 size_t esb_len = (1ull << xsrc->esb_shift) * xsrc->nr_irqs;
 size_t tima_len = 4ull << TM_SHIFT;
 CPUState *cs;
+int fd;
 
 /*
  * The KVM XIVE device already in use. This is the case when
@@ -760,11 +761,12 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, 
uint32_t nr_servers,
 }
 
 /* First, create the KVM XIVE device */
-xive->fd = kvm_create_device(kvm_state, KVM_DEV_TYPE_XIVE, false);
-if (xive->fd < 0) {
-error_setg_errno(errp, -xive->fd, "XIVE: error creating KVM device");
+fd = kvm_create_device(kvm_state, KVM_DEV_TYPE_XIVE, false);
+if (fd < 0) {
+error_setg_errno(errp, -fd, "XIVE: error creating KVM device");
 return -1;
 }
+xive->fd = fd;
 
 /* Tell KVM about the # of VCPUs we may have */
 if (kvm_device_check_attr(xive->fd, KVM_DEV_XIVE_GRP_CTRL,

[PATCH for-5.2 0/5] spapr: Cleanups for XIVE and PHB

[Greg Kurz]

Recent cleanup patch "spapr: Simplify error handling in spapr_phb_realize"
had to be dropped from ppc-for-5.2 because it would cause QEMU to crash
at init time on some POWER9 setups (eg. Boston systems), as reported by
Daniel.

The crash was happening because the kvmppc_xive_source_reset_one() function
would get called at some point (eg. initializing the LSI table of PHB0) and
fail (because XIVE KVM isn't supported on Bostons) without calling
error_setg(), which the caller doesn't expect when the patch above is applied.

The issue isn't really about a missing call to error_setg() but why do
we end up trying to claim an IRQ number in a XIVE KVM device that doesn't
exist ? The root cause for this is that we guard calls to the XIVE KVM
code with kvm_irqchip_in_kernel(), which might return true when the XICS
KVM device is active, even though the XIVE one is not. This series
upgrade the guarding code to also check if the device is actually open.

A similar cleanup could be performed on XICS.

---

Greg Kurz (5):
  spapr/xive: Fix xive->fd if kvm_create_device() fails
  spapr/xive: Simplify kvmppc_xive_disconnect()
  ppc/xive: Introduce dedicated kvm_irqchip_in_kernel() wrappers
  spapr/xive: Convert KVM device fd checks to assert()
  spapr: Simplify error handling in spapr_phb_realize()


 hw/intc/spapr_xive.c|   39 +-
 hw/intc/spapr_xive_kvm.c|   64 +++
 hw/intc/xive.c  |   30 +++-
 hw/ppc/spapr_pci.c  |   16 +--
 include/hw/ppc/spapr_xive.h |1 +
 include/hw/ppc/xive.h   |2 +
 6 files changed, 87 insertions(+), 65 deletions(-)

--
Greg

Re: [PATCH v12 11/11] iotests: dump QCOW2 header in JSON in #303

[Vladimir Sementsov-Ogievskiy]

30.07.2020 17:15, Andrey Shinkevich wrote:

Extend the test case #303 by dumping QCOW2 image metadata in JSON
format.

Signed-off-by: Andrey Shinkevich


Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [PATCH v12 10/11] qcow2_format.py: support dumping metadata in JSON format

[Vladimir Sementsov-Ogievskiy]

30.07.2020 17:15, Andrey Shinkevich wrote:

Implementation of dumping QCOW2 image metadata.
The sample output:
{
 "Header_extensions": [
 {
 "name": "Feature table",
 "magic": 1745090647,
 "length": 192,
 "data_str": ""
 },
 {
 "name": "Bitmaps",
 "magic": 595929205,
 "length": 24,
 "data": {
 "nb_bitmaps": 2,
 "reserved32": 0,
 "bitmap_directory_size": 64,
 "bitmap_directory_offset": 1048576,
 "bitmap_directory": [
 {
 "name": "bitmap-1",
 "bitmap_table_offset": 589824,
 "bitmap_table_size": 1,
 "flags": 2,
 "type": 1,
 "granularity_bits": 15,
 "name_size": 8,
 "extra_data_size": 0,
 "bitmap_table": [
 {
 "type": "serialized",
 "offset": 655360
 },
 ...

Suggested-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Andrey Shinkevich 
---
  tests/qemu-iotests/qcow2_format.py | 17 +
  1 file changed, 17 insertions(+)

diff --git a/tests/qemu-iotests/qcow2_format.py 
b/tests/qemu-iotests/qcow2_format.py
index a4114cb..7487720 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -19,6 +19,15 @@
  
  import struct

  import string
+import json
+
+
+class ComplexEncoder(json.JSONEncoder):
+def default(self, obj):
+if hasattr(obj, 'to_dict'):
+return obj.to_dict()
+else:
+return json.JSONEncoder.default(self, obj)
  
  
  class Qcow2Field:

@@ -110,6 +119,10 @@ class Qcow2Struct(metaclass=Qcow2StructMeta):
   for i, field in enumerate(self.fields))
  
  def dump(self, is_json=False):

+if is_json:
+print(json.dumps(self.to_dict(), indent=4, cls=ComplexEncoder))
+return
+
  for f in self.fields:
  value = self.__dict__[f[2]]
  if isinstance(f[1], str):
@@ -440,6 +453,10 @@ class QcowHeader(Qcow2Struct):
  fd.write(buf)
  
  def dump_extensions(self, is_json=False):

+if is_json:
+print(json.dumps(self.extensions, indent=4, cls=ComplexEncoder))
+return
+
  for ex in self.extensions:
  print('Header extension:')
  ex.dump()



Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [RFC v3 0/8] QEMU cpus.c refactoring part2

[Claudio Fontana]

On 8/3/20 1:48 PM, Alex Bennée wrote:
> 
> Claudio Fontana  writes:
> 
>> Motivation and higher level steps:
>>
>> https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg04628.html
>>
>> The biggest open item for me is, does it makes sense to:
>>
>>
>> 1) make icount TCG-only (building the icount module only under
>> CONFIG_TCG), as this series suggests, and provide a separate virtual
>> counter for qtest,
> 
> Well icount certainly never has any use except with TCG - the fields are
> all wasted in the KVM case.
> 
>> or
>>
>>
>> 2) continue to keep icount functions and fields, including vmstate,
>> in all softmmu builds because of qtest current use of field
>> qemu_icount_bias to implement its virtual counter for
>> qtest_clock_warp?
> 
> Is this just a case of maintaining compatibility for saved VM images? We
> could certainly keep the fields in VM state and stub out (or warn?) if a
> icount related field turned up when reloading a VM into a KVM only build
> or a build with !tcg_enabled().
> 
> I would defer to the vmstate experts on the best way to do this? Is the
> field currently unconditional? Certainly the rr bits are only registered
> when RR is enabled.


Hi Alex and all,

do we have a compatibility issue to worry about?

Ie, I assumed looking at the "needed" functions in vmstate that
if a VM contains a subfield that is unneeded when loaded, it would just be 
ignored.

But maybe this was a too optimistic assumption?

Thank you,

Claudio


> 
>> If I understand correctly Paolo might be for 2) (?)
>> would also welcome additional input from the community in any direction
>> (Alex, Peter, Philippe?)
>>
>> 
>>
>> RFC v2 -> v3:
>>
>> * provided defaults for all methods.
>>   Only create_vcpu_thread is now a mandatory field. (Paolo)
>>
>> * separated new CpusAccel patch from its first user, new patch nr. 2:
>>   "cpus: prepare new CpusAccel cpu accelerator interface"
>>
>> * new CpusAccel methods: get_virtual_clock and get_elapsed_ticks.
>>   (Paolo)
>>
>>   In this series, get_virtual_clock has a separate implementation
>>   between TCG/icount and qtest,
>>   while get_elapsed_ticks only returns a virtual counter for icount.
>>
>>   Looking for more comments in this area.
>>
>> 
>>
>> RFC v1 -> v2:
>>
>> * split the cpus.c accelerator refactoring into 6 patches.
>>
>> * other minor changes to be able to proceed step by step.
>>
>> 
>>
>> * Rebased on commit 255ae6e2158c743717bed76c9a2365ee4bcd326e,
>> "replay: notify the main loop when there are no instructions"
>>
>> [SPLIT into part1 and part2]
>>
>> 
>>
>> v6 -> v7:
>>
>> * rebased changes on top of Pavel Dovgalyuk changes to dma-helpers.c
>>   "icount: make dma reads deterministic"
>>
>> 
>>
>> v5 -> v6:
>>
>> * rebased changes on top of Emilio G. Cota changes to cpus.c
>>   "cpu: convert queued work to a QSIMPLEQ"
>>
>> * keep a pointer in cpus.c instead of a copy of CpusAccel
>>   (Alex)
>>
>> 
>>
>>
>> v4 -> v5: rebase on latest master
>>
>> * rebased changes on top of roman series to remove one of the extra states 
>> for hvf.
>>   (Is the result now functional for HVF?)
>>
>> * rebased changes on top of icount changes and fixes to icount_configure and
>>   the new shift vmstate. (Markus)
>>
>> v3 -> v4:
>>
>> * overall: added copyright headers to all files that were missing them
>>   (used copyright and license of the module the stuff was extracted from).
>>   For the new interface files, added SUSE LLC.
>>
>> * 1/4 (move softmmu only files from root):
>>
>>   MAINTAINERS: moved softmmu/cpus.c to its final location (from patch 2)
>>
>> * 2/4 (cpu-throttle):
>>
>>   MAINTAINERS (to patch 1),
>>   copyright Fabrice Bellard and license from cpus.c
>>
>> * 3/4 (cpu-timers, icount):
>>
>>   - MAINTAINERS: add cpu-timers.c and icount.c to Paolo
>>
>>   - break very long lines (patchew)
>>
>>   - add copyright SUSE LLC, GPLv2 to cpu-timers.h
>>
>>   - add copyright Fabrice Bellard and license from cpus.c to timers-state.h
>> as it is lifted from cpus.c
>>
>>   - vl.c: in configure_accelerators bail out if icount_enabled()
>> and !tcg_enabled() as qtest does not enable icount anymore.
>>
>> * 4/4 (accel stuff to accel):
>>
>>   - add copyright SUSE LLC to files that mostly only consist of the
>> new interface. Add whatever copyright was in the accelerator code
>> if instead they mostly consist of accelerator code.
>>
>>   - change a comment to mention the result of the AccelClass experiment
>>
>>   - moved qtest accelerator into accel/qtest/ , make it like the others.
>>
>>   - rename xxx-cpus-interface to xxx-cpus (remove "interface" from names)
>>
>>   - rename accel_int to cpus_accel
>>
>>   - rename CpusAccel functions from cpu_synchronize_* to synchronize_*
>>
>>
>> 
>>
>> v2 -> v3:
>>
>> * turned into a 4 patch series, adding a first patch moving
>>   softmmu code currently in top_srcdir to softmmu/
>>
>> * cpu-throttle: moved to softmmu/
>>
>> * cpu-timers, icount:
>>
>>   - moved to softmmu/
>>

Re: [PATCH v3 0/8] Generalize start-powered-off property from ARM

[Thiago Jung Bauermann]

Thiago Jung Bauermann  writes:

> Philippe Mathieu-Daudé  writes:
>
>> Le jeu. 30 juil. 2020 03:00, David Gibson  a
>> écrit :
>>
>>> On Tue, Jul 28, 2020 at 09:56:36PM -0300, Thiago Jung Bauermann wrote:
>>> >
>>> > Thiago Jung Bauermann  writes:
>>> >
>>> > > The ARM code has a start-powered-off property in ARMCPU, which is a
>>> > > subclass of CPUState. This property causes arm_cpu_reset() to set
>>> > > CPUState::halted to 1, signalling that the CPU should start in a halted
>>> > > state. Other architectures also have code which aim to achieve the same
>>> > > effect, but without using a property.
>>> > >
>>> > > The ppc/spapr version has a bug where QEMU does a KVM_RUN on the vcpu
>>> > > before cs->halted is set to 1, causing the vcpu to run while it's
>>> still in
>>> > > an unitialized state (more details in patch 3).
>>> >
>>> > Since this series fixes a bug is it eligible for 5.1, at least the
>>> > patches that were already approved by the appropriate maintainers?
>>>
>>> Ok by me.
>>>
>>
>> Maybe just the arm generalization and ppc fix for 5.1, delaying all not
>> bugfix to 5.2?
>
> That would be great.

Any news on this? Is there something I should be doing? I saw -rc3 today
but not these patches.

-- 
Thiago Jung Bauermann
IBM Linux Technology Center

Re: v8.1M cpu emulation and target-arm feature-identification strategy

[Richard Henderson]

On 8/5/20 9:52 AM, Peter Maydell wrote:
> On Wed, 5 Aug 2020 at 17:45, Alex Bennée  wrote:
>> I wouldn't test other feature bits but what stopping us adding:
>>
>> struct ARMISARegisters {
>> uint32_t id_isar0;
>> ...
>> uint64_t id_aa64dfr1;
>> /*
>>  * The following are synthetic flags for features not exposed to
>>  * the directly exposed to the guest but needed by QEMU's
>>  * feature detection.
>>  */
>> bool v81m_lob;
>> } isar;
> 
> Nothing, except we already have a set of synthetic flags, that's
> what the ARM_FEATURE_* are...
> 
>> That said we still seem to have a number of ARM_FEATURE flags, are we
>> hoping they all go away eventually?
> 
> I think that they're a mixed bag. Some represent cleanups we
> haven't got round to doing yet (eg ARM_FEATURE_NEON, which would
> be a fair chunk of work, or ARM_FEATURE_PXN which would be pretty
> trivial to change to looking at ID_MMFR0.VMSA >=4). Some are
> features that pre-date the ID feature bit scheme and so might
> be awkward to convert (eg ARM_FEATURE_XSCALE). One or two
> we've already converted and just forgot to take out of the
> enum (eg ARM_FEATURE_CRC)...

I've always assumed we'd never get rid of all of them.

Older ones like XSCALE are obvious, but I don't think there's a clear indicator
for V{5,6,7,8} either.


r~

Re: [PATCH v12 09/11] qcow2_format.py: collect fields to dump in JSON format

[Vladimir Sementsov-Ogievskiy]

30.07.2020 17:15, Andrey Shinkevich wrote:

As __dict__ is being extended with class members we do not want to
print, add the to_dict() method to classes that returns a dictionary
with desired fields and their values. Extend it in subclass when
necessary to print the final dictionary in the JSON output which
follows.

Suggested-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Andrey Shinkevich 
---
  tests/qemu-iotests/qcow2_format.py | 34 ++
  1 file changed, 34 insertions(+)

diff --git a/tests/qemu-iotests/qcow2_format.py 
b/tests/qemu-iotests/qcow2_format.py
index 2000de3..a4114cb 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -119,6 +119,9 @@ class Qcow2Struct(metaclass=Qcow2StructMeta):
  
  print('{:<25} {}'.format(f[2], value_str))
  
+def to_dict(self):

+return dict((f[2], self.__dict__[f[2]]) for f in self.fields)
+
  
  class Qcow2BitmapExt(Qcow2Struct):
  
@@ -151,6 +154,11 @@ class Qcow2BitmapExt(Qcow2Struct):

  print()
  entry.dump()
  
+def to_dict(self):

+fields_dict = super().to_dict()
+fields_dict['bitmap_directory'] = self.bitmap_directory
+return fields_dict
+
  
  class Qcow2BitmapDirEntry(Qcow2Struct):
  
@@ -189,6 +197,14 @@ class Qcow2BitmapDirEntry(Qcow2Struct):

  super(Qcow2BitmapDirEntry, self).dump()
  self.bitmap_table.dump()
  
+def to_dict(self):

+fields_dict = super().to_dict()
+fields_dict['bitmap_table'] = self.bitmap_table.entries


the fact that we have to access internals of bitmap_table is not nice, but 
let's refactor it later.


+bmp_name = dict(name=self.name)
+# Put the name ahead of the dict


Aha. I don't think that ordering is necessary for json, but why not to order it 
a bit.


+bme_dict = {**bmp_name, **fields_dict}



strange to create bmp_name dict just to unpack it. You may as well do

bme_dict = {'name': self.name, **fields_dict}


+return bme_dict


bme_dict is extra variable: it's created just to return it, so, how about just

return {'name': self.name, **fields_dict}


or, maybe

return {
   'name': self.name,
   **super().to_dict(),
   'bitmap_table': self.bitmap_table.entries
   }


+
  
  class Qcow2BitmapTableEntry(Qcow2Struct):
  
@@ -214,6 +230,9 @@ class Qcow2BitmapTableEntry(Qcow2Struct):

  else:
  self.type = 'all-zeroes'
  
+def to_dict(self):

+return dict(type=self.type, offset=self.offset, reserved=self.reserved)
+


Python has a special syntax for creating dicts :), let's use:

return {'type': self.type, 'offset': self.offset, 'reserved': self.reserved}


  
  class Qcow2BitmapTable:
  
@@ -246,6 +265,9 @@ class QcowHeaderExtension(Qcow2Struct):

  0x44415441: 'Data file'
  }
  
+def to_dict(self):

+return self.mapping.get(self.value, "")


aha, so, actually, to_dict() returns not dict, but string. So it should better 
be named to_json() (and return any json-dumpable object, like string or dict)

and then, we can add to_json() method to Qcow2BitmapTable as well, to return 
list.



+
  fields = (
  ('u32', Magic, 'magic'),
  ('u32', '{}', 'length')
@@ -308,6 +330,18 @@ class QcowHeaderExtension(Qcow2Struct):
  else:
  self.obj.dump()
  
+def to_dict(self):

+fields_dict = super().to_dict()
+ext_name = dict(name=self.Magic(self.magic))


strange that we have to create Magic object here... Ok, let's refactor it later


+# Put the name ahead of the dict
+he_dict = {**ext_name, **fields_dict}
+if self.obj is not None:
+he_dict['data'] = self.obj
+else:
+he_dict['data_str'] = self.data_str
+
+return he_dict


again, let's avoid extra dict variables:

res = {'name': self.Magic(self.magic), **super().to_dict()}
if ...



+
  @classmethod
  def create(cls, magic, data):
  return QcowHeaderExtension(magic, len(data), data)




--
Best regards,
Vladimir

Re: v8.1M cpu emulation and target-arm feature-identification strategy

[Peter Maydell]

On Wed, 5 Aug 2020 at 17:45, Alex Bennée  wrote:
> I wouldn't test other feature bits but what stopping us adding:
>
> struct ARMISARegisters {
> uint32_t id_isar0;
> ...
> uint64_t id_aa64dfr1;
> /*
>  * The following are synthetic flags for features not exposed to
>  * the directly exposed to the guest but needed by QEMU's
>  * feature detection.
>  */
> bool v81m_lob;
> } isar;

Nothing, except we already have a set of synthetic flags, that's
what the ARM_FEATURE_* are...

> That said we still seem to have a number of ARM_FEATURE flags, are we
> hoping they all go away eventually?

I think that they're a mixed bag. Some represent cleanups we
haven't got round to doing yet (eg ARM_FEATURE_NEON, which would
be a fair chunk of work, or ARM_FEATURE_PXN which would be pretty
trivial to change to looking at ID_MMFR0.VMSA >=4). Some are
features that pre-date the ID feature bit scheme and so might
be awkward to convert (eg ARM_FEATURE_XSCALE). One or two
we've already converted and just forgot to take out of the
enum (eg ARM_FEATURE_CRC)...

thanks
-- PMM

[Bug 1879587] Re: Register number in ESR is incorrect for certain banked registers when switching from AA32 to AA64

[Peter Maydell]

** Changed in: qemu
   Status: In Progress => Fix Committed

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1879587

Title:
  Register number in ESR is incorrect for certain banked registers when
  switching from AA32 to AA64

Status in QEMU:
  Fix Committed

Bug description:
  I am running into a situation where I have:
  - A hypervisor running in EL2, AA64
  - A guest running in EL1, AA32

  We trap certain accesses to special registers such as DACR (via
  HCR.TVM). One instruction that is trapped is:

  ee03ef10  ->mcr 15, 0, lr, cr3, cr0, {0}

  The guest is running in SVC mode. So, LR should refer to LR_svc there.
  LR_svc is mapped to X18 in AA64. So, ESR should reflect that. However,
  the actual ESR value is: 0xfe00dc0

  If we decode the 'rt':
  >>> (0xfe00dc0 >> 5) & 0x1f
  14

  My understanding is that 14 is incorrect in the context of AA64. rt
  should be set to 18. The current mode being SVC, LR refers to LR_svc
  not LR_usr. In other words, the mapping between registers in AA64 and
  AA32 doesn't seem to be accounted for. I've tested this with Qemu
  5.0.0

  Let me know if that makes sense and if you would like more info. I am also 
happy to test patches.
  Thanks for all the great work on Qemu!

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1879587/+subscriptions

Re: [RFC v2 50/76] target/riscv: rvv-0.9: single-width saturating add and subtract instructions

[Richard Henderson]

On 8/3/20 7:40 PM, Frank Chang wrote:
> This isn't what spike does.
> 
> The manual could really stand to be more specific here...
> 
> Isn't Spike's vsaddu.vi  immediate value also 
> signed-extended? 
> /riscv/insns/vsaddu_vi.h:/
> /vd = vs2 + (insn.v_simm5() & (UINT64_MAX >> (64 - P.VU.vsew))); /

Whoops, quite right.  Masking to SEW, not (u)imm5.


r~

Re: [PATCH-for-5.0 1/2] hw/acpi/piix4: Add 'system-hotplug-support' property

[Igor Mammedov]

On Wed, 5 Aug 2020 08:01:24 +0200
Philippe Mathieu-Daudé  wrote:

> On 8/5/20 7:56 AM, Philippe Mathieu-Daudé wrote:
> > On 3/19/20 11:02 AM, Paolo Bonzini wrote:  
> >> On 19/03/20 10:42, Philippe Mathieu-Daudé wrote:  
> >>> On 3/19/20 10:36 AM, Paolo Bonzini wrote:  
>  On 18/03/20 23:15, Philippe Mathieu-Daudé wrote:  
> > The I/O ranges registered by the piix4_acpi_system_hot_add_init()
> > function are not documented in the PIIX4 datasheet.
> > This appears to be a PC-only feature added in commit 5e3cb5347e
> > ("initialize hot add system / acpi gpe") which was then moved
> > to the PIIX4 device model in commit 9d5e77a22f ("make
> > qemu_system_device_hot_add piix independent")
> > Add a property (default enabled, to not modify the current
> > behavior) to allow machines wanting to model a simple PIIX4
> > to disable this feature.  
> 
>  Yes, all hotplug stuff (PCI/memory/CPU) are custom additions by QEMU.
>   
> > +    DEFINE_PROP_BOOL("system-hotplug-support", PIIX4PMState,
> > + use_acpi_system_hotplug, true),  
> 
>  Why not cpu-hotplug-support?  
> >>>
> >>> Because I have no idea what this code is about, and it seems more than
> >>> cpu (pci, memory):  
> >>
> >> Right, I should have been more verbose.  You mentioned I/O port 0xaf00
> >> which is CPU hotplug.  Perhaps unless you can also crash with PCI
> >> hotplug (0xae00-0xae0f) it's worth removing only CPU hotplug from MIPS
> >> machines, and keep PCI hotplug.  
> > 
> > I am sorry I don't understand what PCI hotplug has to do with PIIX which
> > is a PCI-slave southbridge... If MIPS or other arch is interested in PCI
> > hotplug feature, that would be managed by the northbridge or another PCI
> > bridge.  
> 
> Ah, writing that comment made me realize the problem might be these
> 'virtualization' features have been implemented in the wrong place.
> Maybe the less disruptive path is to move them to the i440fx
> northbridge.
not sure if this option is on the table atm.
You will need a means to remap migration state to another device to keep 
migration working.

>That way we shouldn't need to maintain a piix_hw.c and
> piix_virt_twisted.c (child of piix_hw overloaded with hotplug features).
that's path I'd consider, i.e. split out virt parts out from piix hw
and make virt child that will have our hacks on top of native piix.

Still, You will need to keep typenames on virt part as it's now
for not to break migration stream (but I'm not sure, CCing David).

> 
> I haven't looked at the history yet, maybe the problem happened when
> i440fx/piix was split.
My guess would be due piix_pm having ACPI hw in spec (like sci/pm) so adding
other related ACPI hw was considered as the least disruptive back then.

> 
> >   
> >>
> >> Paolo
> >>  
> >>> static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
> >>>    PCIBus *bus, PIIX4PMState *s)
> >>> {
> >>>     memory_region_init_io(&s->io_gpe, OBJECT(s), &piix4_gpe_ops, s,
> >>>   "acpi-gpe0", GPE_LEN);
> >>>     memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
> >>>
> >>>     acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent,
> >>>     s->use_acpi_pci_hotplug);
> >>>
> >>>     s->cpu_hotplug_legacy = true;
> >>>     object_property_add_bool(OBJECT(s), "cpu-hotplug-legacy",
> >>>  piix4_get_cpu_hotplug_legacy,
> >>>  piix4_set_cpu_hotplug_legacy,
> >>>  NULL);
> >>>     legacy_acpi_cpu_hotplug_init(parent, OBJECT(s), &s->gpe_cpu,
> >>>  PIIX4_CPU_HOTPLUG_IO_BASE);
> >>>
> >>>     if (s->acpi_memory_hotplug.is_enabled) {
> >>>     acpi_memory_hotplug_init(parent, OBJECT(s),
> >>> &s->acpi_memory_hotplug,
> >>>  ACPI_MEMORY_HOTPLUG_BASE);
> >>>     }
> >>> }
> >>>  
> >>  
> >   
> 

Re: cleanups with long-term benefits

[Eduardo Habkost]

On Wed, Aug 05, 2020 at 06:23:23PM +0200, Kevin Wolf wrote:
> Am 05.08.2020 um 12:08 hat Daniel P. Berrangé geschrieben:
> > On Wed, Aug 05, 2020 at 11:11:55AM +0200, Cornelia Huck wrote:
> > > On Wed, 5 Aug 2020 10:05:40 +0100
> > > Daniel P. Berrangé  wrote:
> > > 
> > > > On Wed, Aug 05, 2020 at 10:49:35AM +0200, Paolo Bonzini wrote:
> > > > > On 05/08/20 10:39, Dr. David Alan Gilbert wrote:  
> > > > > >> Do you really use "git blame" that much?  "git log -S" does more 
> > > > > >> or less
> > > > > >> the same function (in a different way) and is not affected as much 
> > > > > >> by
> > > > > >> large code movement and transformation patches.  
> > > > > >
> > > > > > I use it a lot!   Following stuff back to find where a change came
> > > > > > from and then asking people.  
> > > > > 
> > > > > Indeed, but I use "git log -S" instead. :)  Another possibility is to
> > > > > just do "git log -p" and search for a relevant line of the code I'm
> > > > > "blaming".  
> > > > 
> > > > I used git blame alot too, but I don't think its a reason to not do the
> > > > cleanups. It is easy enough to just tell blame to use an earlier commit
> > > > if you see it displaying a refactor. I don't think such mild 
> > > > inconvenience
> > > > should stop us making otherwise desirable code changes
> > > 
> > > I don't think people argue that it should block changes; it it simply
> > > another thing to consider when weighing benefits vs. drawbacks.
> > 
> > Actually, I'm saying that including "git blame" in such a weighing is
> > the mechanism for blocking otherwise beneficial change to the code. Or
> > to put it another way, I believe the value assigned to "git blame" in
> > such a comparison is miniscule / effectively zero. The only time
> > "git blame" should win is if the change in question is purely the
> > bike shed colour and has no technical benefits at all.  If there's any
> > technical benefit to making the change it should always win.  We
> > shouldn't preserve technical debt in the code merely to avoid an impact
> > on "git blame".
> 
> We're basically weighing "git blame" against syntax highlighting
> defaults. I don't think the latter has an obviously higher weight.

I think "syntax highlight defaults" is far from being an accurate
description of the motivations behind this discussion.

-- 
Eduardo

Re: v8.1M cpu emulation and target-arm feature-identification strategy

[Alex Bennée]

Richard Henderson  writes:

> On 8/5/20 4:08 AM, Peter Maydell wrote:
>> Mostly recently we've been aiming for QEMU emulation code in
>> target/arm to use ID register fields to determine whether a
>> feature is present or not (the isar_feature_* functions) rather
>> than the old style of defining ARM_FEATURE_* flags. This seems to
>> be working out well for A-profile. However, for v8.1M there are
>> a small handful of minor behaviour differences which don't have an
>> associated ID register field, but which are instead in the spec
>> and pseudocode just called out as "if this is a v8.1M CPU".
>> (The major v8.1M new features do have ID register fields.)
>> 
>> I can think of two ways to handle this:
>>  (1) define an ARM_FEATURE_V81M flag
>>  (2) define an isar_feature_aa32_v81m() function which under the
>>  hood is actually testing for a specific feature which happens
>>  to be known to be always present in v8.1M, like low-overhead-branches
>>  (ie ID_ISAR0.CmpBranch >=3)
>
> I think (2) has the potential to be confusing in odd ways.  If there really is
> no official flag for this, I think we should use (1).

I wouldn't test other feature bits but what stopping us adding:

struct ARMISARegisters {
uint32_t id_isar0;
...
uint64_t id_aa64dfr1;
/*
 * The following are synthetic flags for features not exposed to
 * the directly exposed to the guest but needed by QEMU's
 * feature detection.
 */
bool v81m_lob;
} isar;
  

And having the normal:

static inline bool isar_feature_aa32_v81m_lob(const ARMISARegisters *id)
{
return id->v81m_lob;
}

That said we still seem to have a number of ARM_FEATURE flags, are we
hoping they all go away eventually?

>
>
> r~


-- 
Alex Bennée

Re: [PATCH v2 3/3] aio-posix: keep aio_notify_me disabled during polling

[Paolo Bonzini]

On 05/08/20 12:00, Stefan Hajnoczi wrote:
> +
> +/*
> + * aio_notify can avoid the expensive event_notifier_set if
> + * everything (file descriptors, bottom halves, timers) will
> + * be re-evaluated before the next blocking poll().  This is
> + * already true when aio_poll is called with blocking == false;
> + * if blocking == true, it is only true after poll() returns,
> + * so disable the optimization now.
> + */
> +if (use_notify_me) {
> +atomic_set(&ctx->notify_me, atomic_read(&ctx->notify_me) + 2);
> +/*
> + * Write ctx->notify_me before reading ctx->notified.  Pairs with
> + * smp_mb in aio_notify().
> + */
> +smp_mb();
> +
> +/* Don't block if aio_notify() was called */
> +if (atomic_read(&ctx->notified)) {
> +timeout = 0;
> +}

Aha, this is the trick: "timeout = 0" also applies if a timer was moved 
early.  In this case you uselessly keep notify_me set for a bit, but 
it's okay. Nice!

The code can be simplified a bit more, since the use_notify_me variable 
is just "timeout":

use_notify_me = (timeout != 0);
if (use_notify_me) {
 /*
  * aio_notify can avoid the expensive event_notifier_set if
  * everything (file descriptors, bottom halves, timers) will
  * be re-evaluated before the next blocking poll().  This is
  * already true when aio_poll is called with blocking == false;
  * if blocking == true, it is only true after poll() returns,
  * so disable the optimization now.
  */
 atomic_set(&ctx->notify_me, atomic_read(&ctx->notify_me) + 2);
 /*
  * Write ctx->notify_me before reading ctx->notified.  Pairs with
  * smp_mb in aio_notify().
  */
 smp_mb();
 
 /* Don't block if aio_notify() was called */
 if (atomic_read(&ctx->notified)) {
 timeout = 0;
 }
 }
 if (timeout || ctx->fdmon_ops->need_wait(ctx)) {
 ret = ctx->fdmon_ops->wait(ctx, &ready_list, timeout);
 }
 if (use_notify_me) {
 /* Finish the poll before clearing the flag.  */
 atomic_store_release(&ctx->notify_me,
  atomic_read(&ctx->notify_me) - 2);
 }

Paolo

Re: [PATCH v2 for-5.1?] target/arm: Fix Rt/Rt2 in ESR_ELx for copro traps from AArch32 to 64

[Peter Maydell]

On Wed, 5 Aug 2020 at 16:26, Richard Henderson
 wrote:
>
> On 8/4/20 12:39 PM, Peter Maydell wrote:
> > When a coprocessor instruction in an  AArch32 guest traps to AArch32
> > Hyp mode, the syndrome register (HSR) includes Rt and Rt2 fields
> > which are simply copies of the Rt and Rt2 fields from the trapped
> > instruction.  However, if the instruction is trapped from AArch32 to
> > an AArch64 higher exception level, the Rt and Rt2 fields in the
> > syndrome register (ESR_ELx) must be the AArch64 view of the register.
> > This makes a difference if the AArch32 guest was in a mode other than
> > User or System and it was using r13 or r14, or if it was in FIQ mode
> > and using r8-r14.
> >
> > We don't know at translate time which AArch32 CPU mode we are in, so
> > we leave the values we generate in our prototype syndrome register
> > value at translate time as the raw Rt/Rt2 from the instruction, and
> > instead correct them to the AArch64 view when we find we need to take
> > an exception from AArch32 to AArch64 with one of these syndrome
> > values.
> >
> > Fixes: https://bugs.launchpad.net/qemu/+bug/1879587
> > Reported-by: Julien Freche 
> > Signed-off-by: Peter Maydell 
> > ---
>
> Reviewed-by: Richard Henderson 

Thanks; applied to master for 5.1.

-- PMM

Re: [PATCH] spapr: Clarify error and documentation for broken KVM XICS

[Cédric Le Goater]

On 8/5/20 5:47 PM, Greg Kurz wrote:
> When starting an L2 KVM guest with `ic-mode=dual,kernel-irqchip=on`,
> QEMU fails with:
> 
> KVM is too old to support ic-mode=dual,kernel-irqchip=on
> 
> This error message was introduced to detect older KVM versions that
> didn't allow destruction and re-creation of the XICS KVM device that
> we do at reboot. But it is actually the same issue that we get with
> nested guests : when running under pseries, KVM currently provides
> a genuine XICS device (not the XICS-on-XIVE device that we get
> under powernv) which doesn't support destruction/re-creation.
> 
> This will eventually be fixed in KVM but in the meantime, update
> the error message and documentation to mention the nested case.
> While here, mention that in "No XIVE support in KVM" section that
> this can also happen with "guest OSes supporting XIVE" since
> we check this at init time before starting the guest.
> 
> Reported-by: Satheesh Rajendran 
> Buglink: https://bugs.launchpad.net/qemu/+bug/1890290
> Signed-off-by: Greg Kurz 

Reviewed-by: Cédric Le Goater 

Thanks,

C. 


> ---
>  docs/specs/ppc-spapr-xive.rst |5 -
>  hw/ppc/spapr_irq.c|   12 +---
>  2 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/docs/specs/ppc-spapr-xive.rst b/docs/specs/ppc-spapr-xive.rst
> index 7199db730b82..7144347560f1 100644
> --- a/docs/specs/ppc-spapr-xive.rst
> +++ b/docs/specs/ppc-spapr-xive.rst
> @@ -126,6 +126,9 @@ xicsXICS KVM   XICS emul. XICS KVM
>  
>  (1) QEMU warns with ``warning: kernel_irqchip requested but unavailable:
>  IRQ_XIVE capability must be present for KVM``
> +In some cases (old host kernels or KVM nested guests), one may hit a
> +QEMU/KVM incompatibility due to device destruction in reset. QEMU fails
> +with ``KVM is incompatible with ic-mode=dual,kernel-irqchip=on``
>  (2) QEMU fails with ``kernel_irqchip requested but unavailable:
>  IRQ_XIVE capability must be present for KVM``
>  
> @@ -148,7 +151,7 @@ xicsXICS KVM   XICS emul. XICS KVM
>  mode (XICS), either don't set the ic-mode machine property or try
>  ic-mode=xics or ic-mode=dual``
>  (4) QEMU/KVM incompatibility due to device destruction in reset. QEMU fails
> -with ``KVM is too old to support ic-mode=dual,kernel-irqchip=on``
> +with ``KVM is incompatible with ic-mode=dual,kernel-irqchip=on``
>  
>  
>  XIVE Device tree properties
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 2f8f7d62f875..72bb938375ef 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -139,6 +139,7 @@ SpaprIrq spapr_irq_dual = {
>  
>  static int spapr_irq_check(SpaprMachineState *spapr, Error **errp)
>  {
> +ERRP_GUARD();
>  MachineState *machine = MACHINE(spapr);
>  
>  /*
> @@ -179,14 +180,19 @@ static int spapr_irq_check(SpaprMachineState *spapr, 
> Error **errp)
>  
>  /*
>   * On a POWER9 host, some older KVM XICS devices cannot be destroyed and
> - * re-created. Detect that early to avoid QEMU to exit later when the
> - * guest reboots.
> + * re-created. Same happens with KVM nested guests. Detect that early to
> + * avoid QEMU to exit later when the guest reboots.
>   */
>  if (kvm_enabled() &&
>  spapr->irq == &spapr_irq_dual &&
>  kvm_kernel_irqchip_required() &&
>  xics_kvm_has_broken_disconnect(spapr)) {
> -error_setg(errp, "KVM is too old to support 
> ic-mode=dual,kernel-irqchip=on");
> +error_setg(errp,
> +"KVM is incompatible with ic-mode=dual,kernel-irqchip=on");
> +error_append_hint(errp,
> +"This can happen with an old KVM or in a KVM nested guest.\n");
> +error_append_hint(errp,
> +"Try without kernel-irqchip or with kernel-irqchip=off.\n");
>  return -1;
>  }
>  
> 
> 

Re: [PATCH v2 2/3] async: always set ctx->notified in aio_notify()

[Paolo Bonzini]

On 05/08/20 12:00, Stefan Hajnoczi wrote:
> aio_notify() does not set ctx->notified when called with
> ctx->aio_notify_me disabled. Therefore aio_notify_me needs to be enabled
> during polling.
> 
> This is suboptimal since expensive event_notifier_set(&ctx->notifier)
> and event_notifier_test_and_clear(&ctx->notifier) calls are required
> when ctx->aio_notify_me is enabled.
> 
> Change aio_notify() so that aio->notified is always set, regardless of
> ctx->aio_notify_me. This will make polling cheaper since
> ctx->aio_notify_me can remain disabled. Move the
> event_notifier_test_and_clear() to the fd handler function (which is now
> no longer an empty function so "dummy" has been dropped from its name).
> 
> The next patch takes advantage of this by optimizing polling in
> util/aio-posix.c.
> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  util/async.c | 32 +---
>  1 file changed, 21 insertions(+), 11 deletions(-)
> 
> diff --git a/util/async.c b/util/async.c
> index d9f987e133..3ec3e8d135 100644
> --- a/util/async.c
> +++ b/util/async.c
> @@ -419,25 +419,32 @@ LuringState *aio_get_linux_io_uring(AioContext *ctx)
>  
>  void aio_notify(AioContext *ctx)
>  {
> -/* Write e.g. bh->scheduled before reading ctx->notify_me.  Pairs
> +/*
> + * Write e.g. bh->flags before writing ctx->notified.  Pairs with smp_mb 
> in
> + * aio_notify_accept.
> + */
> +smp_wmb();
> +atomic_set(&ctx->notified, true);
> +
> +/*
> + * Write ctx->notified before reading ctx->notify_me.  Pairs
>   * with smp_mb in aio_ctx_prepare or aio_poll.
>   */
>  smp_mb();
>  if (atomic_read(&ctx->notify_me)) {
>  event_notifier_set(&ctx->notifier);
> -atomic_mb_set(&ctx->notified, true);
>  }
>  }
>  
>  void aio_notify_accept(AioContext *ctx)
>  {
> -if (atomic_xchg(&ctx->notified, false)
> -#ifdef WIN32
> -|| true
> -#endif
> -) {
> -event_notifier_test_and_clear(&ctx->notifier);
> -}
> +atomic_set(&ctx->notified, false);
> +
> +/*
> + * Write ctx->notified before reading e.g. bh->flags.  Pairs with smp_mb 
> in
> + * aio_notify.

Just a nit: it's an smp_wmb().  (It's okay for a wmb to pair with
anything stronger than a smp_rmb()).

> + */
> +smp_mb();
>  }
>  
>  static void aio_timerlist_notify(void *opaque, QEMUClockType type)
> @@ -445,8 +452,11 @@ static void aio_timerlist_notify(void *opaque, 
> QEMUClockType type)
>  aio_notify(opaque);
>  }
>  
> -static void aio_context_notifier_dummy_cb(EventNotifier *e)
> +static void aio_context_notifier_cb(EventNotifier *e)
>  {
> +AioContext *ctx = container_of(e, AioContext, notifier);
> +
> +event_notifier_test_and_clear(&ctx->notifier);
>  }
>  
>  /* Returns true if aio_notify() was called (e.g. a BH was scheduled) */
> @@ -508,7 +518,7 @@ AioContext *aio_context_new(Error **errp)
>  
>  aio_set_event_notifier(ctx, &ctx->notifier,
> false,
> -   aio_context_notifier_dummy_cb,
> +   aio_context_notifier_cb,
> aio_context_notifier_poll);
>  #ifdef CONFIG_LINUX_AIO
>  ctx->linux_aio = NULL;
> 

Reviewed-by: Paolo Bonzini 

Much better, you can almost trust the code to do the right thing. :)

Paolo

Re: cleanups with long-term benefits

[Kevin Wolf]

Am 05.08.2020 um 12:08 hat Daniel P. Berrangé geschrieben:
> On Wed, Aug 05, 2020 at 11:11:55AM +0200, Cornelia Huck wrote:
> > On Wed, 5 Aug 2020 10:05:40 +0100
> > Daniel P. Berrangé  wrote:
> > 
> > > On Wed, Aug 05, 2020 at 10:49:35AM +0200, Paolo Bonzini wrote:
> > > > On 05/08/20 10:39, Dr. David Alan Gilbert wrote:  
> > > > >> Do you really use "git blame" that much?  "git log -S" does more or 
> > > > >> less
> > > > >> the same function (in a different way) and is not affected as much by
> > > > >> large code movement and transformation patches.  
> > > > >
> > > > > I use it a lot!   Following stuff back to find where a change came
> > > > > from and then asking people.  
> > > > 
> > > > Indeed, but I use "git log -S" instead. :)  Another possibility is to
> > > > just do "git log -p" and search for a relevant line of the code I'm
> > > > "blaming".  
> > > 
> > > I used git blame alot too, but I don't think its a reason to not do the
> > > cleanups. It is easy enough to just tell blame to use an earlier commit
> > > if you see it displaying a refactor. I don't think such mild inconvenience
> > > should stop us making otherwise desirable code changes
> > 
> > I don't think people argue that it should block changes; it it simply
> > another thing to consider when weighing benefits vs. drawbacks.
> 
> Actually, I'm saying that including "git blame" in such a weighing is
> the mechanism for blocking otherwise beneficial change to the code. Or
> to put it another way, I believe the value assigned to "git blame" in
> such a comparison is miniscule / effectively zero. The only time
> "git blame" should win is if the change in question is purely the
> bike shed colour and has no technical benefits at all.  If there's any
> technical benefit to making the change it should always win.  We
> shouldn't preserve technical debt in the code merely to avoid an impact
> on "git blame".

We're basically weighing "git blame" against syntax highlighting
defaults. I don't think the latter has an obviously higher weight.

Kevin

Re: v8.1M cpu emulation and target-arm feature-identification strategy

[Richard Henderson]

On 8/5/20 4:08 AM, Peter Maydell wrote:
> Mostly recently we've been aiming for QEMU emulation code in
> target/arm to use ID register fields to determine whether a
> feature is present or not (the isar_feature_* functions) rather
> than the old style of defining ARM_FEATURE_* flags. This seems to
> be working out well for A-profile. However, for v8.1M there are
> a small handful of minor behaviour differences which don't have an
> associated ID register field, but which are instead in the spec
> and pseudocode just called out as "if this is a v8.1M CPU".
> (The major v8.1M new features do have ID register fields.)
> 
> I can think of two ways to handle this:
>  (1) define an ARM_FEATURE_V81M flag
>  (2) define an isar_feature_aa32_v81m() function which under the
>  hood is actually testing for a specific feature which happens
>  to be known to be always present in v8.1M, like low-overhead-branches
>  (ie ID_ISAR0.CmpBranch >=3)

I think (2) has the potential to be confusing in odd ways.  If there really is
no official flag for this, I think we should use (1).


r~

Re: [PATCH v12 08/11] qcow2.py: Introduce '-j' key to dump in JSON format

[Vladimir Sementsov-Ogievskiy]

30.07.2020 17:15, Andrey Shinkevich wrote:

Add the command key to the qcow2.py arguments list to dump QCOW2
metadata in JSON format. Here is the suggested way to do that. The
implementation of the dump in JSON format is in the patch that follows.

Signed-off-by: Andrey Shinkevich


Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [PATCH-for-5.1 v3 1/2] exec: Restrict icount to softmmu

[Richard Henderson]

On 8/5/20 3:01 AM, Philippe Mathieu-Daudé wrote:
> 'icount' feature is only meaningful when using softmmu.
> Move it out of the globally used exec.c, and define it as
> 'false' in user-mode emulation.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  include/sysemu/cpus.h | 4 
>  exec.c| 4 
>  softmmu/cpus.c| 7 +++
>  3 files changed, 11 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson 

r~

Re: cleanups with long-term benefits (was Re: [PATCH] schemas: Add vim modeline)

[John Snow]

On 8/5/20 3:36 AM, Markus Armbruster wrote:

John Snow  writes:


On 8/4/20 4:03 AM, Markus Armbruster wrote:

The pain of tweaking the parser is likely dwarved several times over by
the pain of the flag day.


You mention this often; I wonder if I misunderstand the critique,
because the pain of a "flag day" for a new file format seems
negligible to me.

I don't think we edit these .json files very often. Generally, we add
a new command when we need one. The edits are usually one or two lines
plus docstrings.

If anyone has patches in-flight, I genuinely doubt it will take more
than a few minutes to rewrite for the new file format.

No?


You describe the the flag day's one-time pain.

There's also the longer term pain of having to work around git-blame
unable to see beyond the flag day.



So it's not really the "flag day" we're worried about anymore, it's the 
ongoing ease-of-use for vcs history.



I'm not claiming the pain is prohibitive (if I thought it was, I
would've tried to strange this thread in its crib), I am claiming it'll
be much more painful (read: expensive) than a parser tweak.



I do use `git blame` quite a lot, but with a project as old as QEMU, 
most of my trips through history do involve jumping across a few 
refactor gaps as a normal part of that process.


As Dan points out, I often have to back out and add refactorSHA^ to my 
invocation, and just keep hopping backwards until I find what I am truly 
after. It just feels like a fact of programmer life for me at this point.


I've not used Paolo's invocation before, but it looks like it might be 
useful. I'll try to remember to try it out.

[Bug 1890290] Re: PowerPC L2(nested virt) kvm guest fails to boot with ic-mode=dual, kernel-irqchip=on - `KVM is too old to support ic-mode=dual, kernel-irqchip=on`

[Greg Kurz]

Posted a patch to the list.

http://patchwork.ozlabs.org/project/qemu-
devel/patch/159664243614.622889.18307368735989783528.st...@bahia.lan/

Satheesh,

Can you please review and test ?


** Changed in: qemu
   Status: New => In Progress

** Changed in: qemu
 Assignee: (unassigned) => Greg Kurz (gkurz)

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1890290

Title:
  PowerPC L2(nested virt) kvm guest fails to boot with ic-mode=dual
  ,kernel-irqchip=on - `KVM is too old to support ic-mode=dual,kernel-
  irqchip=on`

Status in QEMU:
  In Progress

Bug description:
  Env:
  HW: Power 9 DD2.3
  Host L0: 5.8.0-rc5-g8ba4ffcd8
  Qemu: 5.0.50 (v5.0.0-533-gdebe78ce14)
  Libvirt: 6.4.0
  L1: 5.8.0-rc5-ge9919e11e
  qemu_version': '5.0.50 (v5.1.0-rc2-dirty)
  libvirt_version': '6.4.0'
  L2: 5.8.0-rc7-g6ba1b005f

  
  1. boot a L2 KVM guest with `ic-mode=dual,kernel-irqchip=on`

  /usr/bin/virt-install --connect=qemu:///system --hvm --accelerate --name 
'vm1' --machine pseries --memory=8192 --vcpu=8,maxvcpus=8,sockets=1,cores=2,t
  hreads=4 --import --nographics --serial pty --memballoon model=virtio --disk 
path=/home/tests/data/avocado-vt/images/f31-ppc64le.qcow2,bus=virtio,size=10,format=qcow2
 --network
  =bridge=virbr0,model=virtio,mac=52:54:00:e6:fe:f6 --mac=52:54:00:e6:fe:f6 
--boot 
emulator=/usr/share/avocado-plugins-vt/bin/qemu,kernel=/tmp/linux/vmlinux,kernel_args="root=/de
  v/vda2 rw console=tty0 console=ttyS0,115200 init=/sbin/init initcall_debug 
selinux=0" --noautoconsole --qemu-commandline=" -M 
pseries,ic-mode=dual,kernel-irqchip=on"

  
  ERRORinternal error: process exited while connecting to monitor: 
2020-08-04T11:12:53.304482Z qemu: KVM is too old to support 
ic-mode=dual,kernel-irqchip=on


  
  Qemu Log:
  ```
  /usr/share/avocado-plugins-vt/bin/qemu \
  -name guest=vm1,debug-threads=on \
  -S \
  -object 
secret,id=masterKey0,format=raw,file=/var/lib/libvirt/qemu/domain-5-vm1/master-key.aes
 \
  -machine pseries-5.1,accel=kvm,usb=off,dump-guest-core=off \
  -cpu POWER9 \
  -m 8192 \
  -overcommit mem-lock=off \
  -smp 8,sockets=1,dies=1,cores=2,threads=4 \
  -uuid 20a3351b-2776-4e75-9059-c070fe3dd44b \
  -display none \
  -no-user-config \
  -nodefaults \
  -chardev socket,id=charmonitor,fd=34,server,nowait \
  -mon chardev=charmonitor,id=monitor,mode=control \
  -rtc base=utc \
  -no-shutdown \
  -boot strict=on \
  -kernel /tmp/linux/vmlinux \
  -append 'root=/dev/vda2 rw console=tty0 console=ttyS0,115200 init=/sbin/init 
initcall_debug selinux=0' \
  -device qemu-xhci,p2=15,p3=15,id=usb,bus=pci.0,addr=0x2 \
  -device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x3 \
  -blockdev 
'{"driver":"file","filename":"/home/tests/data/avocado-vt/images/f31-ppc64le.qcow2","node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}'
 \
  -blockdev 
'{"node-name":"libvirt-1-format","read-only":false,"driver":"qcow2","file":"libvirt-1-storage","backing":null}'
 \
  -device 
virtio-blk-pci,bus=pci.0,addr=0x4,drive=libvirt-1-format,id=virtio-disk0,bootindex=1
 \
  -netdev tap,fd=37,id=hostnet0,vhost=on,vhostfd=38 \
  -device 
virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:e6:fe:f6,bus=pci.0,addr=0x1 
\
  -chardev pty,id=charserial0 \
  -device spapr-vty,chardev=charserial0,id=serial0,reg=0x3000 \
  -chardev socket,id=charchannel0,fd=39,server,nowait \
  -device 
virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,id=channel0,name=org.qemu.guest_agent.0
 \
  -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5 \
  -M pseries,ic-mode=dual,kernel-irqchip=on \
  -msg timestamp=on
  2020-08-04 11:12:53.169+: Domain id=5 is tainted: custom-argv
  2020-08-04 11:12:53.179+: 11120: info : libvirt version: 6.4.0, package: 
1.fc31 (Unknown, 2020-06-02-05:09:40, ltc-wspoon4.aus.stglabs.ibm.com)
  2020-08-04 11:12:53.179+: 11120: info : hostname: atest-guest
  2020-08-04 11:12:53.179+: 11120: info : virObjectUnref:347 : 
OBJECT_UNREF: obj=0x7fff0c117c40
  char device redirected to /dev/pts/0 (label charserial0)
  2020-08-04T11:12:53.304482Z qemu: KVM is too old to support 
ic-mode=dual,kernel-irqchip=on
  2020-08-04 11:12:53.694+: shutting down, reason=failed
  ```

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1890290/+subscriptions

Re: [PATCH v12 07/11] qcow2_format.py: Dump bitmap table serialized entries

[Vladimir Sementsov-Ogievskiy]

30.07.2020 17:15, Andrey Shinkevich wrote:

Add bitmap table information to the QCOW2 metadata dump.

Bitmap name   bitmap-1
...
Bitmap table   typesize offset
0  serialized  6553610092544
1  all-zeroes  655360


For serialized, size and offset are size and offset of bitmap table.
But, all-zeroes bitmaps don't have any bitmap table, so size and offset
both are undefined (OK to print zero for them, but 65536 is unrelated).


2  all-zeroes  655360

Signed-off-by: Andrey Shinkevich 
---
  tests/qemu-iotests/303.out |  4 
  tests/qemu-iotests/qcow2_format.py | 47 ++
  2 files changed, 51 insertions(+)

diff --git a/tests/qemu-iotests/303.out b/tests/qemu-iotests/303.out
index dc3739b..d581fb4 100644
--- a/tests/qemu-iotests/303.out
+++ b/tests/qemu-iotests/303.out
@@ -70,6 +70,8 @@ type  1
  granularity_bits  15
  name_size 8
  extra_data_size   0
+Bitmap table   typesize offset
+0  serialized  6553610092544
  
  Bitmap name   bitmap-2

  bitmap_table_offset   0x9c
@@ -79,4 +81,6 @@ type  1
  granularity_bits  16
  name_size 8
  extra_data_size   0
+Bitmap table   typesize offset
+0  all-zeroes  655360
  
diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py

index ca0d350..1f033d4 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -175,6 +175,10 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
  entry_raw_size = self.bitmap_dir_entry_raw_size()
  padding = ((entry_raw_size + 7) & ~7) - entry_raw_size
  fd.seek(padding, 1)
+self.bitmap_table = Qcow2BitmapTable(fd=fd,
+ offset=self.bitmap_table_offset,
+ nb_entries=self.bitmap_table_size,
+ cluster_size=self.cluster_size)
  
  def bitmap_dir_entry_raw_size(self):

  return struct.calcsize(self.fmt) + self.name_size + \
@@ -183,6 +187,49 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
  def dump(self):
  print(f'{"Bitmap name":<25} {self.name}')
  super(Qcow2BitmapDirEntry, self).dump()
+self.bitmap_table.dump()
+
+
+class Qcow2BitmapTableEntry(Qcow2Struct):
+
+fields = (
+('u64',  '{}', 'entry'),
+)
+
+BME_TABLE_ENTRY_RESERVED_MASK = 0xff0001fe
+BME_TABLE_ENTRY_OFFSET_MASK = 0x00fffe00
+BME_TABLE_ENTRY_FLAG_ALL_ONES = 1
+
+def __init__(self, fd):
+super().__init__(fd=fd)
+self.reserved = self.entry & self.BME_TABLE_ENTRY_RESERVED_MASK
+self.offset = self.entry & self.BME_TABLE_ENTRY_OFFSET_MASK
+if self.offset:
+if self.entry & self.BME_TABLE_ENTRY_FLAG_ALL_ONES:
+self.type = 'invalid'
+else:
+self.type = 'serialized'
+elif self.entry & self.BME_TABLE_ENTRY_FLAG_ALL_ONES:
+self.type = 'all-ones'
+else:
+self.type = 'all-zeroes'
+
+
+class Qcow2BitmapTable:
+
+def __init__(self, fd, offset, nb_entries, cluster_size):
+self.cluster_size = cluster_size
+position = fd.tell()
+fd.seek(offset)
+self.entries = [Qcow2BitmapTableEntry(fd) for _ in range(nb_entries)]
+fd.seek(position)
+
+def dump(self):
+size = self.cluster_size
+bitmap_table = enumerate(self.entries)
+print(f'{"Bitmap table":<14} {"type":<15} {"size":<12} {"offset"}')
+for i, entry in bitmap_table:
+print(f'{i:<14} {entry.type:<15} {size:<12} {entry.offset}')
  


All this looks like 'cluster_size' is not really needed for Qcow2BitmapTable 
class (we can print only offsets).
Still, if you want to save it, can we print it only for entries with 
'serialized' type?

It's minor, I'm OK with it as is:
Reviewed-by: Vladimir Sementsov-Ogievskiy 


  
  QCOW2_EXT_MAGIC_BITMAPS = 0x23852875





--
Best regards,
Vladimir