Re: [Qemu-devel] [PATCH] crypto: move 'opaque' parameter to (nearly) the end of parameter list

2017-04-25 Thread Markus Armbruster
"Daniel P. Berrange"  writes:

> Previous commit moved 'opaque' to be the 2nd parameter in the list:
>
>   commit 375092332eeaa6e47561ce47fd36144cdaf964d0
>   Author: Fam Zheng 
>   Date:   Fri Apr 21 20:27:02 2017 +0800
>
> crypto: Make errp the last parameter of functions
>
> Move opaque to 2nd instead of the 2nd to last, so that compilers help
> check with the conversion.
>
> this puts it back to the 2nd to last position.
>
> Signed-off-by: Daniel P. Berrange 

I could take this through my tree, but I suggest you stick it into your
next crypto pull request.



Re: [Qemu-devel] [PULL 0/21] Please pull xen-20170421-tag for 2.10

2017-04-25 Thread Markus Armbruster
Stefano Stabellini  writes:

> On Mon, 24 Apr 2017, Peter Maydell wrote:
>> On 24 April 2017 at 22:25, Stefano Stabellini  wrote:
>> > diff --git a/hw/9pfs/xen-9pfs.h b/hw/9pfs/xen-9pfs.h
>> > new file mode 100644
>> > index 000..18f0ec0
>> > --- /dev/null
>> > +++ b/hw/9pfs/xen-9pfs.h
>> > @@ -0,0 +1,14 @@
>> > +/*
>> > + * Xen 9p backend
>> > + *
>> > + * Copyright Aporeto 2017
>> > + *
>> > + * Authors:
>> > + *  Stefano Stabellini 
>> > + *
>> > + */
>> 
>> Trivial file, but I prefer it if we have a brief license
>> statement in every file, just to be clear (it might
>> accumulate more code later).
>
> Sure
>
>> > +
>> > +#include 
>> > +#include "hw/xen/io/ring.h"
>> > +
>> > +DEFINE_XEN_FLEX_RING_AND_INTF(xen_9pfs);
>> 
>> Is it worth a comment to dissuade people from thinking they can
>> inline the file back into xen-9p-backend.c ?
>
> Thanks for the quick review! Here you go:
>
>
> diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
> index 7e962aa..9c7f41a 100644
> --- a/hw/9pfs/xen-9p-backend.c
> +++ b/hw/9pfs/xen-9p-backend.c
> @@ -13,12 +13,9 @@
>  #include "hw/hw.h"
>  #include "hw/9pfs/9p.h"
>  #include "hw/xen/xen_backend.h"
> -#include "hw/xen/io/ring.h"
> +#include "hw/9pfs/xen-9pfs.h"
>  #include "qemu/config-file.h"
>  #include "fsdev/qemu-fsdev.h"
> -#include 
> -
> -DEFINE_XEN_FLEX_RING_AND_INTF(xen_9pfs);
>  
>  #define VERSIONS "1"
>  #define MAX_RINGS 8
> diff --git a/hw/9pfs/xen-9pfs.h b/hw/9pfs/xen-9pfs.h
> new file mode 100644
> index 000..6e33d77
> --- /dev/null
> +++ b/hw/9pfs/xen-9pfs.h
> @@ -0,0 +1,21 @@
> +/*
> + * Xen 9p backend
> + *
> + * Copyright Aporeto 2017
> + *
> + * Authors:
> + *  Stefano Stabellini 
> + *
> + * This work is licensed under the terms of the GNU GPL version 2.

Any particular reason for "version 2" instead of "version 2 or later"?


> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include 
> +#include "hw/xen/io/ring.h"
> +
> +/*
> + * Do not merge into xen-9p-backend.c: clang doesn't allow unused static
> + * inline functions in c files.
> + */
> +DEFINE_XEN_FLEX_RING_AND_INTF(xen_9pfs);



[Qemu-devel] [PATCH] net/dump: Issue a warning for the deprecated "-net dump"

2017-04-25 Thread Thomas Huth
Network dumping should be done with "-object filter-dump" nowadays.
Using "-net dump" via the VLAN mechanism is considered as deprecated
and might be removed in a future release. So warn the users now
to inform them to user the filter-dump method instead.

Signed-off-by: Thomas Huth 
---
 net/dump.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/dump.c b/net/dump.c
index 89a149b..442eb53 100644
--- a/net/dump.c
+++ b/net/dump.c
@@ -194,6 +194,9 @@ int net_init_dump(const Netdev *netdev, const char *name,
 
 assert(peer);
 
+error_report("'-net dump' is deprecated. "
+ "Please use '-object filter-dump' instead.");
+
 if (dump->has_file) {
 file = dump->file;
 } else {
-- 
1.8.3.1




[Qemu-devel] [PATCH v2 01/13] exec-all: add tb_from_jmp_cache

2017-04-25 Thread Emilio G. Cota
This paves the way for upcoming changes.

Signed-off-by: Emilio G. Cota 
---
 cpu-exec.c  | 19 +++
 include/exec/exec-all.h |  2 +-
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index 63a56d0..b4adf16 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -309,6 +309,25 @@ static bool tb_cmp(const void *p, const void *d)
 return false;
 }
 
+TranslationBlock *tb_from_jmp_cache(CPUArchState *env, target_ulong vaddr)
+{
+CPUState *cpu = ENV_GET_CPU(env);
+TranslationBlock *tb;
+target_ulong cs_base, pc;
+uint32_t flags;
+
+if (unlikely(atomic_read(&cpu->exit_request))) {
+return NULL;
+}
+cpu_get_tb_cpu_state(env, &pc, &cs_base, &flags);
+tb = atomic_rcu_read(&cpu->tb_jmp_cache[tb_jmp_cache_hash_func(vaddr)]);
+if (likely(tb && tb->pc == vaddr && tb->cs_base == cs_base &&
+   tb->flags == flags)) {
+return tb;
+}
+return NULL;
+}
+
 static TranslationBlock *tb_htable_lookup(CPUState *cpu,
   target_ulong pc,
   target_ulong cs_base,
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index bcde1e6..18b80bc 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -56,7 +56,6 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
   target_ulong pc, target_ulong cs_base,
   uint32_t flags,
   int cflags);
-
 void QEMU_NORETURN cpu_loop_exit(CPUState *cpu);
 void QEMU_NORETURN cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc);
 void QEMU_NORETURN cpu_loop_exit_atomic(CPUState *cpu, uintptr_t pc);
@@ -368,6 +367,7 @@ struct TranslationBlock {
 void tb_free(TranslationBlock *tb);
 void tb_flush(CPUState *cpu);
 void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr);
+TranslationBlock *tb_from_jmp_cache(CPUArchState *env, target_ulong vaddr);
 
 #if defined(USE_DIRECT_JUMP)
 
-- 
2.7.4




[Qemu-devel] [PATCH v2 02/13] exec-all: inline tb_from_jmp_cache

2017-04-25 Thread Emilio G. Cota
The inline improves performance, as shown in upcoming commits' logs.

This commit is kept separate to ease review, since the inclusion
of tb-hash.h might be controversial. The problem here, which was
introduced before this commit, is that tb_hash_func() depends on
page_addr_t: this defeats the original purpose of tb-hash.h,
which was to be self-contained and CPU-agnostic.

Signed-off-by: Emilio G. Cota 
---
 cpu-exec.c  | 19 ---
 include/exec/exec-all.h | 24 +++-
 2 files changed, 23 insertions(+), 20 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index b4adf16..63a56d0 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -309,25 +309,6 @@ static bool tb_cmp(const void *p, const void *d)
 return false;
 }
 
-TranslationBlock *tb_from_jmp_cache(CPUArchState *env, target_ulong vaddr)
-{
-CPUState *cpu = ENV_GET_CPU(env);
-TranslationBlock *tb;
-target_ulong cs_base, pc;
-uint32_t flags;
-
-if (unlikely(atomic_read(&cpu->exit_request))) {
-return NULL;
-}
-cpu_get_tb_cpu_state(env, &pc, &cs_base, &flags);
-tb = atomic_rcu_read(&cpu->tb_jmp_cache[tb_jmp_cache_hash_func(vaddr)]);
-if (likely(tb && tb->pc == vaddr && tb->cs_base == cs_base &&
-   tb->flags == flags)) {
-return tb;
-}
-return NULL;
-}
-
 static TranslationBlock *tb_htable_lookup(CPUState *cpu,
   target_ulong pc,
   target_ulong cs_base,
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 18b80bc..bd76987 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -367,7 +367,29 @@ struct TranslationBlock {
 void tb_free(TranslationBlock *tb);
 void tb_flush(CPUState *cpu);
 void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr);
-TranslationBlock *tb_from_jmp_cache(CPUArchState *env, target_ulong vaddr);
+
+/* tb_hash_func() in tb-hash.h needs tb_page_addr_t, defined above */
+#include "tb-hash.h"
+
+static inline
+TranslationBlock *tb_from_jmp_cache(CPUArchState *env, target_ulong vaddr)
+{
+CPUState *cpu = ENV_GET_CPU(env);
+TranslationBlock *tb;
+target_ulong cs_base, pc;
+uint32_t flags;
+
+if (unlikely(atomic_read(&cpu->exit_request))) {
+return NULL;
+}
+cpu_get_tb_cpu_state(env, &pc, &cs_base, &flags);
+tb = atomic_rcu_read(&cpu->tb_jmp_cache[tb_jmp_cache_hash_func(vaddr)]);
+if (likely(tb && tb->pc == vaddr && tb->cs_base == cs_base &&
+   tb->flags == flags)) {
+return tb;
+}
+return NULL;
+}
 
 #if defined(USE_DIRECT_JUMP)
 
-- 
2.7.4




[Qemu-devel] [PATCH v2 10/13] target/i386: introduce gen_jr() helper to jump to register

2017-04-25 Thread Emilio G. Cota
This helper will be used by subsequent commits.

Signed-off-by: Emilio G. Cota 
---
 target/i386/translate.c | 28 +++-
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/target/i386/translate.c b/target/i386/translate.c
index 1d1372f..445082b 100644
--- a/target/i386/translate.c
+++ b/target/i386/translate.c
@@ -141,6 +141,7 @@ typedef struct DisasContext {
 } DisasContext;
 
 static void gen_eob(DisasContext *s);
+static void gen_jr(DisasContext *s, TCGv dest);
 static void gen_jmp(DisasContext *s, target_ulong eip);
 static void gen_jmp_tb(DisasContext *s, target_ulong eip, int tb_num);
 static void gen_op(DisasContext *s1, int op, TCGMemOp ot, int d);
@@ -2509,7 +2510,8 @@ static void gen_bnd_jmp(DisasContext *s)
If INHIBIT, set HF_INHIBIT_IRQ_MASK if it isn't already set.
If RECHECK_TF, emit a rechecking helper for #DB, ignoring the state of
S->TF.  This is used by the syscall/sysret insns.  */
-static void gen_eob_worker(DisasContext *s, bool inhibit, bool recheck_tf)
+static void
+gen_eob_worker(DisasContext *s, bool inhibit, bool recheck_tf, TCGv jr)
 {
 gen_update_cc_op(s);
 
@@ -2530,6 +2532,16 @@ static void gen_eob_worker(DisasContext *s, bool 
inhibit, bool recheck_tf)
 tcg_gen_exit_tb(0);
 } else if (s->tf) {
 gen_helper_single_step(cpu_env);
+} else if (jr) {
+TCGv vaddr = tcg_temp_new();
+TCGv_ptr ptr = tcg_temp_new_ptr();
+
+tcg_gen_ld_tl(vaddr, cpu_env, offsetof(CPUX86State, segs[R_CS].base));
+tcg_gen_add_tl(vaddr, vaddr, jr);
+gen_helper_lookup_tb_ptr(ptr, cpu_env, vaddr);
+tcg_temp_free(vaddr);
+tcg_gen_goto_ptr(ptr);
+tcg_temp_free_ptr(ptr);
 } else {
 tcg_gen_exit_tb(0);
 }
@@ -2540,13 +2552,19 @@ static void gen_eob_worker(DisasContext *s, bool 
inhibit, bool recheck_tf)
If INHIBIT, set HF_INHIBIT_IRQ_MASK if it isn't already set.  */
 static void gen_eob_inhibit_irq(DisasContext *s, bool inhibit)
 {
-gen_eob_worker(s, inhibit, false);
+gen_eob_worker(s, inhibit, false, NULL);
 }
 
 /* End of block, resetting the inhibit irq flag.  */
 static void gen_eob(DisasContext *s)
 {
-gen_eob_worker(s, false, false);
+gen_eob_worker(s, false, false, NULL);
+}
+
+/* Jump to register */
+static void gen_jr(DisasContext *s, TCGv dest)
+{
+gen_eob_worker(s, false, false, dest);
 }
 
 /* generate a jump to eip. No segment change must happen before as a
@@ -7131,7 +7149,7 @@ static target_ulong disas_insn(CPUX86State *env, 
DisasContext *s,
 /* TF handling for the syscall insn is different. The TF bit is  
checked
after the syscall insn completes. This allows #DB to not be
generated after one has entered CPL0 if TF is set in FMASK.  */
-gen_eob_worker(s, false, true);
+gen_eob_worker(s, false, true, NULL);
 break;
 case 0x107: /* sysret */
 if (!s->pe) {
@@ -7146,7 +7164,7 @@ static target_ulong disas_insn(CPUX86State *env, 
DisasContext *s,
checked after the sysret insn completes. This allows #DB to be
generated "as if" the syscall insn in userspace has just
completed.  */
-gen_eob_worker(s, false, true);
+gen_eob_worker(s, false, true, NULL);
 }
 break;
 #endif
-- 
2.7.4




[Qemu-devel] [PATCH v2 03/13] tcg: enforce 64-byte alignment of TCGContext

2017-04-25 Thread Emilio G. Cota
This will allow us to prevent cache line false sharing in TCGContext.

Before:
$ objdump -t  build/x86_64-linux-user/qemu-x86_64 | grep tcg_ctx
003ea820 g O .bss   000152d8  tcg_ctx

After:
$ objdump -t  build/x86_64-linux-user/qemu-x86_64 | grep tcg_ctx
003ea880 g O .bss   00015300  tcg_ctx

Signed-off-by: Emilio G. Cota 
---
 tcg/tcg.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tcg/tcg.h b/tcg/tcg.h
index 6c216bb..5fdbfe3 100644
--- a/tcg/tcg.h
+++ b/tcg/tcg.h
@@ -727,7 +727,7 @@ struct TCGContext {
 
 uint16_t gen_insn_end_off[TCG_MAX_INSNS];
 target_ulong gen_insn_data[TCG_MAX_INSNS][TARGET_INSN_START_WORDS];
-};
+} QEMU_ALIGNED(64);
 
 extern TCGContext tcg_ctx;
 extern bool parallel_cpus;
-- 
2.7.4




[Qemu-devel] [PATCH v2 00/13] TCG optimizations for 2.10

2017-04-25 Thread Emilio G. Cota
v1 for context:
  https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg02021.html

This series is aimed at 2.10 or beyond. Its goal is to improve
TCG code execution performance by optimizing:

1- Cross-page direct jumps (softmmu only, obviously)
2- Indirect branches (both softmmu and user-mode)
3- tb_jmp_cache hashing in user-mode (last patch)

Optimizations 1 and 2 are optional. This series implements them
for the i386 TCG backend and the ARM and i386 front-ends; other
backends/frontends can easily opt-in later on.

Changes from v1:

- Followed Richard's design, i.e. have a single helper in tcg-runtime
  and have the TCG op (now called "goto_ptr") to directly jump
  to the host pointer. This pointer is always valid since it's
  either pointing to the (valid) target or to TCG's epilogue. This
  simplifies the whole thing; the only branch in the code path is
  now the one that checks whether the tb pointer from tb_jmp_cache
  is valid.

- Much better performance (e.g. 2.4x speedup for "train" xalancbmk) --
  I'm guessing the design with just one branch is the reason. Also,
  I was unconditionally assigning ret=0 when entering the epilogue;
  fixed now.

- Document goto_ptr in tcg/README, as suggested by Paolo.

- target/i386: also optimized ret/ret im.

- Ensure that TCGContext's read-mostly fields are accessed without
  cache line bouncing. Note that (1) every time we translate,
  TCGContext is heavily written to, and (2) the address of the
  epilogue, which is now accessed in a fast path, is part of
  TCGContext. So patches 3 and 4 make sure there is no false sharing
  of cache lines between these two access patterns.

- Evaluated Paolo's suggestion of using multiplicative hashing. See
  the last patch's commit log.

Things I didn't do:

- Apply the optimization to syscall instructions in target/i386.

- Look at the impact of TLB flushes. With these (new, improved) perf
  numbers there is less reason to worry about this, although they
  should explain the perf differences between softmmu and user-mode.
  Thanks Alex for pointing me out to your profiling code though!
  Learning to use trace points is next in my QEMU TODO list, so I'll
  take a look.

The series applies cleanly on v2.9.0. Measurements are in the
commit logs. You can inspect/fetch the changes at:
  https://github.com/cota/qemu/tree/tcg-opt-v2

Thanks,

Emilio




[Qemu-devel] [PATCH v2 07/13] tcg/i386: implement goto_ptr op

2017-04-25 Thread Emilio G. Cota
Signed-off-by: Emilio G. Cota 
---
 tcg/i386/tcg-target.h |  2 +-
 tcg/i386/tcg-target.inc.c | 13 +
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/tcg/i386/tcg-target.h b/tcg/i386/tcg-target.h
index 59d9835..73a15f7 100644
--- a/tcg/i386/tcg-target.h
+++ b/tcg/i386/tcg-target.h
@@ -107,7 +107,7 @@ extern bool have_popcnt;
 #define TCG_TARGET_HAS_muls2_i321
 #define TCG_TARGET_HAS_muluh_i320
 #define TCG_TARGET_HAS_mulsh_i320
-#define TCG_TARGET_HAS_goto_ptr 0
+#define TCG_TARGET_HAS_goto_ptr 1
 
 #if TCG_TARGET_REG_BITS == 64
 #define TCG_TARGET_HAS_extrl_i64_i320
diff --git a/tcg/i386/tcg-target.inc.c b/tcg/i386/tcg-target.inc.c
index 5918008..f6fb03e 100644
--- a/tcg/i386/tcg-target.inc.c
+++ b/tcg/i386/tcg-target.inc.c
@@ -1906,6 +1906,14 @@ static inline void tcg_out_op(TCGContext *s, TCGOpcode 
opc,
 }
 s->tb_jmp_reset_offset[a0] = tcg_current_code_size(s);
 break;
+case INDEX_op_goto_ptr:
+/* save target address into new register */
+tcg_out_mov(s, TCG_TYPE_PTR, TCG_REG_EDX, a0);
+/* set return value to 0 */
+tgen_arithr(s, ARITH_XOR, TCG_REG_EAX, TCG_REG_EAX);
+/* jmp to the target address (could be epilogue) */
+tcg_out_modrm(s, OPC_GRP5, EXT5_JMPN_Ev, TCG_REG_EDX);
+break;
 case INDEX_op_br:
 tcg_out_jxx(s, JCC_JMP, arg_label(a0), 0);
 break;
@@ -2277,6 +2285,7 @@ static inline void tcg_out_op(TCGContext *s, TCGOpcode 
opc,
 
 static const TCGTargetOpDef *tcg_target_op_def(TCGOpcode op)
 {
+static const TCGTargetOpDef ri = { .args_ct_str = { "ri" } };
 static const TCGTargetOpDef ri_r = { .args_ct_str = { "ri", "r" } };
 static const TCGTargetOpDef re_r = { .args_ct_str = { "re", "r" } };
 static const TCGTargetOpDef qi_r = { .args_ct_str = { "qi", "r" } };
@@ -2324,6 +2333,9 @@ static const TCGTargetOpDef *tcg_target_op_def(TCGOpcode 
op)
 case INDEX_op_st_i64:
 return &re_r;
 
+case INDEX_op_goto_ptr:
+return &ri;
+
 case INDEX_op_add_i32:
 case INDEX_op_add_i64:
 return &r_r_re;
@@ -2569,6 +2581,7 @@ static void tcg_target_qemu_prologue(TCGContext *s)
 
 /* TB epilogue */
 tb_ret_addr = s->code_ptr;
+s->code_gen_epilogue = s->code_ptr;
 
 tcg_out_addi(s, TCG_REG_CALL_STACK, stack_addend);
 
-- 
2.7.4




[Qemu-devel] [PATCH v2 09/13] target/arm: optimize indirect branches with TCG's goto_ptr

2017-04-25 Thread Emilio G. Cota
Speed up indirect branches by directly jumping to the target
if it is valid, i.e. if it is found in tb_jmp_cache.

Softmmu measurements: (see later commit for user-mode results)

Note: baseline (i.e. speedup == 1x) is QEMU v2.9.0.

- Impact on Boot time

| setup | ARM debian boot+shutdown time | stddev |
|---+---+|
| v2.9.0| 10.35 |   0.07 |
| +cross+inline | 10.32 |   0.03 |
| +jr+inline| 10.59 |   0.20 |

-NBench, arm-softmmu (debian jessie guest). Host: 
Intel i7-4790K @ 4.00GHz

 1.25x 
+-+-+-+
   | +++   |
 |
   |   cross+inline    |
 |
  1.2x 
+cross+jr+inline.#++#...+-+
   | +++#  #  #| #  
 |
   |  +++ +++  #  #| #  
+++  |
   |   | *  *  #  #++#  
 |   |
 1.15x 
+-+++#*..*..#..#..#.+-+
   |  *++*  #*  *  #  #  #  
   # |#  |
   |  *  *  #  +++   *  *  #  #  #  
   #++#  |
  1.1x 
+-+*..*..#...|*..*..#..#..#.#..#+-+
   |  *  *  #    *  *  #  #  #  
   #  #  |
   |+++   *  *  #  #++#  *  *  #  #  #  
   #  # #  # |
   |  *  *  #  #  #  *  *  #  #  #  
   #  # #  # |
 1.05x 
+-+..#++#..*..*..#..#..#..*..*..#...+++#..#.#..#+++..#..#...+-+
   |#  #  *  *  #  #  #  *  *  #  *  #  
   #  #+++  *  # |
   |  ++   +++ +++  #  #  *  *  #   +++#  #  *  *  #  * | *  #  
   #  #  #  *+++*  # |
1x 
+-++-+++###-+#++#+-*++*++#-+#-+#++*++*++#++*+-+*++#+-+++#++#-+*###++*++*++#++*+-+*++#+-++-+
   | *++#  *++*++#| #  #  *  *  #| #  #  *  *  #  *   *  #  
  #  *+++*++#  *  *  #  *   *  # |
   | *+++*  #  *  *  #  *  #  *  *  #  *  #  *  *  #  *   *  #  
*++*  #  *   *  #  *  *  #  *   *  # |
   | *   *  #  *  *  #  *+++*  #  *  *  #  * | *  #  *  *  #  *   *  #  
*  *  #  *   *  #  *  *  #  *   *  # |
 0.95x 
+-+...*...*..#..*..*..#..*...*..#..*..*..#..*+++*..#..*..*..#..*...*..#..*..*..#..*...*..#..*..*..#..*...*..#...+-+
   | *   *  #  *  *  #  *   *  #  *  *  #  *   *  #  *  *  #  *   *  #  
*  *  #  *   *  #  *  *  #  *   *  # |
   | *   *  #  *  *  #  *   *  #  *  *  #  *   *  #  *  *  #  *   *  #  
*  *  #  *   *  #  *  *  #  *   *  # |
  0.9x 
+-+---*###--###--*###--###--*###--###--*###--###--*###--###--*###---+-+
   ASSIGNMENT BITFIELD   FOURFP EMULATION   HUFFMAN   LU DECOMPOSITIONEURAL 
NNUMERIC SOSTRING SORT hmean
  png: http://imgur.com/528aS76

NB. 'cross' represents the previous commit.

Signed-off-by: Emilio G. Cota 
---
 target/arm/translate.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/target/arm/translate.c b/target/arm/translate.c
index 574cf70..d5296b1 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -65,6 +65,7 @@ static TCGv_i32 cpu_R[16];
 TCGv_i32 cpu_CF, cpu_NF, cpu_VF, cpu_ZF;
 TCGv_i64 cpu_exclusive_addr;
 TCGv_i64 cpu_exclusive_val;
+static bool gen_jr;
 
 /* FIXME:  These should be removed.  */
 static TCGv_i32 cpu_F0s, cpu_F1s;
@@ -221,6 +222,7 @@ static void store_reg(DisasContext *s, int reg, TCGv_i32 
var)
  */
 tcg_gen_andi_i32(var, var, s->thumb ? ~1 : ~3);
 s->is_jmp = DISAS_JUMP;
+gen_jr = true;
 }
 tcg_gen_mov_i32(cpu_R[reg], var);
 tcg_temp_free_i32(var);
@@ -893,6 +895,7 @@ static inline void gen_bx_im(DisasContext *s, uint32_t addr)
 tcg_temp_free_i32(tmp);
 }
 tcg_gen_movi_i32(cpu_R[15], addr & ~1);
+gen_jr = true;
 }
 
 /* Set PC and Thumb state from var.  var is marked as dead.  */
@@

[Qemu-devel] [PATCH v2 11/13] target/i386: optimize cross-page direct jumps in softmmu

2017-04-25 Thread Emilio G. Cota
Instead of unconditionally exiting to the exec loop, use the
gen_jr helper to jump to the target if it is valid.

As long as the hit rate in tb_jmp_cache remains high, this
change improves performance.

Perf impact: see the next commit's log.

Signed-off-by: Emilio G. Cota 
---
 target/i386/translate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/i386/translate.c b/target/i386/translate.c
index 445082b..9982a2d 100644
--- a/target/i386/translate.c
+++ b/target/i386/translate.c
@@ -2154,9 +2154,9 @@ static inline void gen_goto_tb(DisasContext *s, int 
tb_num, target_ulong eip)
 gen_jmp_im(eip);
 tcg_gen_exit_tb((uintptr_t)s->tb + tb_num);
 } else {
-/* jump to another page: currently not optimized */
+/* jump to another page */
 gen_jmp_im(eip);
-gen_eob(s);
+gen_jr(s, cpu_tmp0);
 }
 }
 
-- 
2.7.4




[Qemu-devel] [PATCH v2 13/13] tb-hash: improve tb_jmp_cache hash function in user mode

2017-04-25 Thread Emilio G. Cota
Optimizations to cross-page chaining and indirect branches make
performance more sensitive to the hit rate of tb_jmp_cache.
The constraint of reserving some bits for the page number
lowers the achievable quality of the hashing function.

However, user-mode does not have this requirement. Thus,
with this change we use for user-mode a hashing function that
is both faster and of better quality than the previous one.

Measurements:

Note: baseline (i.e. speedup == 1x) is QEMU v2.9.0.

-SPECint06 (test set), x86_64-linux-user. Host: 
Intel i7-6700K @ 4.00GHz

   2x 
+-+--+-+
  | 
+|
  |   jr+noinline   
 | | |
  | jr+inline  
++%%@ |
 1.8x +-+jr+hash+noinline
+..|%%@...+-+
  |jr+multhash+inline   
|%%@+|
  |jr+hash+inline 
+$$$%@ |
  | 
++##|$%@  +++|
 1.6x 
+-+|##|$%@+%%%...+-+
  |  @@+ 
**#+$%@$$+% |
  |   $$$%@+
+**#+$%@  ++$$+%@|
  |   $ $%@  
**# $%@   +$$%@@+++$$ %@|
 1.4x 
+-+.+%%%@..##+$%@..**#.$%@...+$$%.@***$$.%@..+-+
  | ++$$+%@  ##+$%@  
**# $%@$$% @* *#$+%@|
  |***#$ %@+**# $%@  
**# $%@ +###$% @* *#$ %@|
  |*+*#$ %@+%%@+**# $%@  
**# $%@ **+#$% @*+*#$ %@   +%%%@+   |
 1.2x 
+-+..*.*#$.%@***#$$%@+**#.$%@..**#.$%@.**.#$%.@*.*#$.%@***#$+%@+.+-+
  | +++* *#$ %@* *# $%@ **# $%@ +++  
**# $%@  +++%%@@** #$% @* *#$ %@*+*#$ %@|
  |   ++###$%+ * *#$ %@* *# $%@ **# $%@ **##$%@@ 
**# $%@+**#$$%+@** #$% @* *#$ %@* *#$ %@|
  |   +**+#$%@@ ++$$@@@* *#$ %@* *# $%@ **# $%@ ** #$% @+###++@@%%%+ 
**# $%@ **# $% @** #$% @* *#$ %@* *#$ %@|
   1x 
+-++-**+#$%-@**##$%+@*+*#$+%@*+*#+$%@+**#+$%@+**+#$%+@**+#$+@@***#$+%@+**#+$%@+**#+$%+@**+#$%+@*+*#$+%@*-*#$+%@-++-+
  |** #$% @** #$% @* *#$ %@* *# $%@ **# $%@ ** #$% @** #$%%@* *#$ %@ 
**# $%@ **# $% @** #$% @* *#$ %@* *#$ %@|
  |** #$% @** #$% @* *#$ %@* *# $%@ **# $%@ ** #$% @** #$+%@* *#$ %@ 
**# $%@ **# $% @** #$% @* *#$ %@* *#$ %@|
  |** #$% @** #$% @* *#$ %@* *# $%@ **# $%@ ** #$% @** #$ %@* *#$ %@ 
**# $%@ **# $% @** #$% @* *#$ %@* *#$ %@|
 0.8x 
+-+--**##$%@@**##$%@@***#$%%@***#$$%@-**#$$%@-**##$%@@**##$%%@***#$%%@-**#$$%@-**#$$%@@**##$%@@***#$%%@***#$%%@--+-+
 astar   bzip2  gcc   gobmk h264ref   hmmlibquantum  mcf 
omnetpperlbench   sjengxalancbmk   hmean
  png: http://imgur.com/1ZJGjzV

Here I also tried the hash function suggested by Paolo ("multhash"):
  return ((uint64_t) (pc * 2654435761) >> 32) & ();
As you can see it is just as good as the other new function ("hash"),
but I kept "hash" because with it all benchmarks have speedup > 1.

-  SPECint06 (train set), x86_64-linux-user. Host: 
Intel i7-6700K @ 4.00GHz

 2.6x 
+-+--+-+
  | 
 |
  | jr+inline   
 |
 2.4x 
+jr+inline+hash###...+-+
  | 
 # # |
  | 
 # # |
 2.2x 
+-+...

[Qemu-devel] [PATCH v2 05/13] tcg-runtime: add lookup_tb_ptr helper

2017-04-25 Thread Emilio G. Cota
Signed-off-by: Emilio G. Cota 
---
 tcg-runtime.c | 7 +++
 tcg/tcg-runtime.h | 2 ++
 tcg/tcg.h | 1 +
 3 files changed, 10 insertions(+)

diff --git a/tcg-runtime.c b/tcg-runtime.c
index 4c60c96..f291184 100644
--- a/tcg-runtime.c
+++ b/tcg-runtime.c
@@ -141,6 +141,13 @@ uint64_t HELPER(ctpop_i64)(uint64_t arg)
 return ctpop64(arg);
 }
 
+void *HELPER(lookup_tb_ptr)(CPUArchState *env, target_ulong addr)
+{
+TranslationBlock *tb = tb_from_jmp_cache(env, addr);
+
+return tb ? tb->tc_ptr : tcg_ctx.code_gen_epilogue;
+}
+
 void HELPER(exit_atomic)(CPUArchState *env)
 {
 cpu_loop_exit_atomic(ENV_GET_CPU(env), GETPC());
diff --git a/tcg/tcg-runtime.h b/tcg/tcg-runtime.h
index 114ea6f..c41d38a 100644
--- a/tcg/tcg-runtime.h
+++ b/tcg/tcg-runtime.h
@@ -24,6 +24,8 @@ DEF_HELPER_FLAGS_1(clrsb_i64, TCG_CALL_NO_RWG_SE, i64, i64)
 DEF_HELPER_FLAGS_1(ctpop_i32, TCG_CALL_NO_RWG_SE, i32, i32)
 DEF_HELPER_FLAGS_1(ctpop_i64, TCG_CALL_NO_RWG_SE, i64, i64)
 
+DEF_HELPER_FLAGS_2(lookup_tb_ptr, TCG_CALL_NO_WG_SE, ptr, env, tl)
+
 DEF_HELPER_FLAGS_1(exit_atomic, TCG_CALL_NO_WG, noreturn, env)
 
 #ifdef CONFIG_SOFTMMU
diff --git a/tcg/tcg.h b/tcg/tcg.h
index b26f0ef..625e2aa 100644
--- a/tcg/tcg.h
+++ b/tcg/tcg.h
@@ -647,6 +647,7 @@ struct TCGContext {
 GHashTable *helpers;
 
 void *code_gen_prologue;
+void *code_gen_epilogue;
 void *code_gen_buffer;
 size_t code_gen_buffer_size;
 
-- 
2.7.4




Re: [Qemu-devel] [PATCH 3/6] migration: add UFFD_FEATURE_THREAD_ID feature support

2017-04-25 Thread Alexey
+ Andrea Arcangeli

On Mon, Apr 24, 2017 at 06:10:02PM +0100, Dr. David Alan Gilbert wrote:
> * Alexey (a.pereva...@samsung.com) wrote:
> > On Mon, Apr 24, 2017 at 04:12:29PM +0800, Peter Xu wrote:
> > > On Fri, Apr 21, 2017 at 06:22:12PM +0300, Alexey wrote:
> > > > On Fri, Apr 21, 2017 at 11:24:54AM +0100, Dr. David Alan Gilbert wrote:
> > > > > * Alexey Perevalov (a.pereva...@samsung.com) wrote:
> > > > > > Userfaultfd mechanism is able to provide process thread id,
> > > > > > in case when client request it with UFDD_API ioctl.
> > > > > > 
> > > > > > Signed-off-by: Alexey Perevalov 
> > > > > 
> > > > > There seem to be two parts to this:
> > > > >   a) Adding the mis parameter to ufd_version_check
> > > > >   b) Asking for the feature
> > > > > 
> > > > > Please split it into two patches.
> > > > > 
> > > > > Also
> > > > > 
> > > > > > ---
> > > > > >  include/migration/postcopy-ram.h |  2 +-
> > > > > >  migration/migration.c|  2 +-
> > > > > >  migration/postcopy-ram.c | 12 ++--
> > > > > >  migration/savevm.c   |  2 +-
> > > > > >  4 files changed, 9 insertions(+), 9 deletions(-)
> > > > > > 
> > > > > > diff --git a/include/migration/postcopy-ram.h 
> > > > > > b/include/migration/postcopy-ram.h
> > > > > > index 8e036b9..809f6db 100644
> > > > > > --- a/include/migration/postcopy-ram.h
> > > > > > +++ b/include/migration/postcopy-ram.h
> > > > > > @@ -14,7 +14,7 @@
> > > > > >  #define QEMU_POSTCOPY_RAM_H
> > > > > >  
> > > > > >  /* Return true if the host supports everything we need to do 
> > > > > > postcopy-ram */
> > > > > > -bool postcopy_ram_supported_by_host(void);
> > > > > > +bool postcopy_ram_supported_by_host(MigrationIncomingState *mis);
> > > > > >  
> > > > > >  /*
> > > > > >   * Make all of RAM sensitive to accesses to areas that haven't yet 
> > > > > > been written
> > > > > > diff --git a/migration/migration.c b/migration/migration.c
> > > > > > index ad4036f..79f6425 100644
> > > > > > --- a/migration/migration.c
> > > > > > +++ b/migration/migration.c
> > > > > > @@ -802,7 +802,7 @@ void 
> > > > > > qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
> > > > > >   * special support.
> > > > > >   */
> > > > > >  if (!old_postcopy_cap && 
> > > > > > runstate_check(RUN_STATE_INMIGRATE) &&
> > > > > > -!postcopy_ram_supported_by_host()) {
> > > > > > +!postcopy_ram_supported_by_host(NULL)) {
> > > > > >  /* postcopy_ram_supported_by_host will have emitted a 
> > > > > > more
> > > > > >   * detailed message
> > > > > >   */
> > > > > > diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> > > > > > index dc80dbb..70f0480 100644
> > > > > > --- a/migration/postcopy-ram.c
> > > > > > +++ b/migration/postcopy-ram.c
> > > > > > @@ -60,13 +60,13 @@ struct PostcopyDiscardState {
> > > > > >  #include 
> > > > > >  #include 
> > > > > >  
> > > > > > -static bool ufd_version_check(int ufd)
> > > > > > +static bool ufd_version_check(int ufd, MigrationIncomingState *mis)
> > > > > >  {
> > > > > >  struct uffdio_api api_struct;
> > > > > >  uint64_t ioctl_mask;
> > > > > >  
> > > > > >  api_struct.api = UFFD_API;
> > > > > > -api_struct.features = 0;
> > > > > > +api_struct.features = UFFD_FEATURE_THREAD_ID;
> > > > > >  if (ioctl(ufd, UFFDIO_API, &api_struct)) {
> > > > > >  error_report("postcopy_ram_supported_by_host: UFFDIO_API 
> > > > > > failed: %s",
> > > > > >   strerror(errno));
> > > > > 
> > > > > You're not actually using the 'mis' here - what I'd expected was
> > > > > something that was going to check if the UFFDIO_API return said that 
> > > > > it really
> > > > > had the feature, and if so store a flag in the MIS somewhere.
> > > > > 
> > > > > Also, I'm not sure it's right to set 'api_struct.features' on the 
> > > > > input - what
> > > > > happens if this is run on an old kernel - we don't want postcopy to 
> > > > > fail on
> > > > > an old kernel without your feature.
> > > > > I'm not 100% sure of the interface, but I think the way it works is 
> > > > > you set
> > > > > features = 0 before the call, and then check the api_struct.features 
> > > > > in the
> > > > > return - in the same way that I check for 
> > > > > UFFD_FEATURE_MISSING_HUGETLBFS.
> > > > > 
> > > > We need to ask kernel about that feature,
> > > > right,
> > > > kernel returns back available features
> > > > uffdio_api.features = UFFD_API_FEATURES
> > > > but it also stores requested features
> > > 
> > > I feel like this does not against Dave's comment, maybe we just need
> > > to send the UFFDIO_API twice? Like:
> > yes, ioctl with UFFDIO_API will fail on old kernel if we will request
> > e.g. UFFD_FEATURE_THREAD_ID or other new feature.
> > 
> > So in general way need a per feature request, for better error handling.
> 
> No, we don't need to - I think the way the kernel works

[Qemu-devel] [PATCH v2 08/13] target/arm: optimize cross-page block chaining in softmmu

2017-04-25 Thread Emilio G. Cota
Instead of unconditionally exiting to the exec loop, use the
lookup_tb_ptr helper to jump to the target if it is valid.
As long as the hit rate in tb_jmp_cache remains high, this
will improve performance.

Perf impact: see the next commit's log.

Signed-off-by: Emilio G. Cota 
---
 target/arm/translate.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/target/arm/translate.c b/target/arm/translate.c
index e32e38c..574cf70 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -4085,8 +4085,12 @@ static inline void gen_goto_tb(DisasContext *s, int n, 
target_ulong dest)
 gen_set_pc_im(s, dest);
 tcg_gen_exit_tb((uintptr_t)s->tb + n);
 } else {
+TCGv_ptr ptr = tcg_temp_new_ptr();
+
 gen_set_pc_im(s, dest);
-tcg_gen_exit_tb(0);
+gen_helper_lookup_tb_ptr(ptr, cpu_env, cpu_R[15]);
+tcg_gen_goto_ptr(ptr);
+tcg_temp_free_ptr(ptr);
 }
 }
 
-- 
2.7.4




Re: [Qemu-devel] [QEMU-2.8] Source QEMU crashes with: "bdrv_co_pwritev: Assertion `!(bs->open_flags & BDRV_O_INACTIVE)' failed"

2017-04-25 Thread Hailiang Zhang

On 2017/4/24 15:59, Kashyap Chamarthy wrote:

On Sat, Apr 22, 2017 at 05:23:49PM +0800, Hailiang Zhang wrote:

Hi,

Hi Hailiang,


I think the bellow patch can fix your problme.
[PATCH 2/4] qmp-cont: invalidate on RUN_STATE_PRELAUNCH
https://patchwork.kernel.org/patch/9591885/

Hmm, the above patch ("qmp-cont: invalidate on RUN_STATE_PRELAUNCH") is
not merged in Git, as it's stalled on design discussion between Kevin
Wolf and Vladimir.

And the below patch, from you, seems to be not submitted upstream (2.8
stable tree, perhaps).  Do you intend to do so?


Er, since this patch does the same thing with the above patch, I'm not sure if 
i should
send this patch ...


Actually, we encounter the same problem in our test, we fix it with the follow 
patch:

  From 0e4d6d706afd9909b5fd71536b45c58af60892f8 Mon Sep 17 00:00:00 2001
  From: zhanghailiang
  Date: Tue, 21 Mar 2017 09:44:36 +0800
  Subject: [PATCH] migration: Re-activate blocks whenever migration been
   cancelled

  In commit 1d2acc3162d9c7772510c973f446353fbdd1f9a8, we try to fix the bug
  'bdrv_co_do_pwritev: Assertion `!(bs->open_flags & 0x0800)' failed'
  which occured in migration cancelling process.

  But it seems that we didn't cover all the cases, we caught such a case 
which
  slipped from the old fixup in our test: if libvirtd cancelled the 
migration
  process for a shutting down VM, it will send 'system_reset' command first,
  and then 'cont' command behind, after VM resumes to run, it will trigger 
the above
  error reports, because we didn't regain the control of blocks for VM.

  Signed-off-by: zhanghailiang
  Signed-off-by: Hongyang Yang
  ---
   block.c   | 12 +++-
   include/block/block.h |  1 +
   include/migration/migration.h |  3 ---
   migration/migration.c |  7 +--
   qmp.c |  4 +---
   5 files changed, 14 insertions(+), 13 deletions(-)

[...]







[Qemu-devel] [PATCH v2 06/13] tcg: add goto_ptr opcode

2017-04-25 Thread Emilio G. Cota
Signed-off-by: Emilio G. Cota 
---
 tcg/README   | 11 +++
 tcg/aarch64/tcg-target.h |  1 +
 tcg/arm/tcg-target.h |  1 +
 tcg/i386/tcg-target.h|  1 +
 tcg/ia64/tcg-target.h|  1 +
 tcg/mips/tcg-target.h|  1 +
 tcg/ppc/tcg-target.h |  1 +
 tcg/s390/tcg-target.h|  1 +
 tcg/sparc/tcg-target.h   |  1 +
 tcg/tcg-op.c |  9 +
 tcg/tcg-op.h |  9 +
 tcg/tcg-opc.h|  1 +
 tcg/tcg.c|  1 +
 tcg/tci/tcg-target.h |  1 +
 14 files changed, 40 insertions(+)

diff --git a/tcg/README b/tcg/README
index a9858c2..9cfd422 100644
--- a/tcg/README
+++ b/tcg/README
@@ -477,6 +477,17 @@ current TB was linked to this TB. Otherwise execute the 
next
 instructions. Only indices 0 and 1 are valid and tcg_gen_goto_tb may be issued
 at most once with each slot index per TB.
 
+* goto_ptr ptr
+
+Jump to a host address given by host pointer 'ptr'.  Typically ptr is obtained
+from the lookup_tb_ptr TCG helper. The return value of this helper depends on
+whether the TB is currently valid: if it is, the corresponding host address
+is returned; if it is not valid, the helper returns the address of the TCG
+epilogue, which restores state to go back to the exec loop.
+
+Implementing goto_ptr is optional for TCG backends. When not implemented,
+calling it is equivalent to calling exit_tb(0).
+
 * qemu_ld_i32/i64 t0, t1, flags, memidx
 * qemu_st_i32/i64 t0, t1, flags, memidx
 
diff --git a/tcg/aarch64/tcg-target.h b/tcg/aarch64/tcg-target.h
index 1a5ea23..b82eac4 100644
--- a/tcg/aarch64/tcg-target.h
+++ b/tcg/aarch64/tcg-target.h
@@ -77,6 +77,7 @@ typedef enum {
 #define TCG_TARGET_HAS_mulsh_i320
 #define TCG_TARGET_HAS_extrl_i64_i320
 #define TCG_TARGET_HAS_extrh_i64_i320
+#define TCG_TARGET_HAS_goto_ptr 0
 
 #define TCG_TARGET_HAS_div_i64  1
 #define TCG_TARGET_HAS_rem_i64  1
diff --git a/tcg/arm/tcg-target.h b/tcg/arm/tcg-target.h
index 09a19c6..2f3ecfd 100644
--- a/tcg/arm/tcg-target.h
+++ b/tcg/arm/tcg-target.h
@@ -123,6 +123,7 @@ extern bool use_idiv_instructions;
 #define TCG_TARGET_HAS_mulsh_i320
 #define TCG_TARGET_HAS_div_i32  use_idiv_instructions
 #define TCG_TARGET_HAS_rem_i32  0
+#define TCG_TARGET_HAS_goto_ptr 0
 
 enum {
 TCG_AREG0 = TCG_REG_R6,
diff --git a/tcg/i386/tcg-target.h b/tcg/i386/tcg-target.h
index 4275787..59d9835 100644
--- a/tcg/i386/tcg-target.h
+++ b/tcg/i386/tcg-target.h
@@ -107,6 +107,7 @@ extern bool have_popcnt;
 #define TCG_TARGET_HAS_muls2_i321
 #define TCG_TARGET_HAS_muluh_i320
 #define TCG_TARGET_HAS_mulsh_i320
+#define TCG_TARGET_HAS_goto_ptr 0
 
 #if TCG_TARGET_REG_BITS == 64
 #define TCG_TARGET_HAS_extrl_i64_i320
diff --git a/tcg/ia64/tcg-target.h b/tcg/ia64/tcg-target.h
index 42aea03..901bb75 100644
--- a/tcg/ia64/tcg-target.h
+++ b/tcg/ia64/tcg-target.h
@@ -173,6 +173,7 @@ typedef enum {
 #define TCG_TARGET_HAS_mulsh_i640
 #define TCG_TARGET_HAS_extrl_i64_i320
 #define TCG_TARGET_HAS_extrh_i64_i320
+#define TCG_TARGET_HAS_goto_ptr 0
 
 #define TCG_TARGET_deposit_i32_valid(ofs, len) ((len) <= 16)
 #define TCG_TARGET_deposit_i64_valid(ofs, len) ((len) <= 16)
diff --git a/tcg/mips/tcg-target.h b/tcg/mips/tcg-target.h
index f46d64a..e3240cf 100644
--- a/tcg/mips/tcg-target.h
+++ b/tcg/mips/tcg-target.h
@@ -130,6 +130,7 @@ extern bool use_mips32r2_instructions;
 #define TCG_TARGET_HAS_muluh_i321
 #define TCG_TARGET_HAS_mulsh_i321
 #define TCG_TARGET_HAS_bswap32_i32  1
+#define TCG_TARGET_HAS_goto_ptr 0
 
 #if TCG_TARGET_REG_BITS == 64
 #define TCG_TARGET_HAS_add2_i32 0
diff --git a/tcg/ppc/tcg-target.h b/tcg/ppc/tcg-target.h
index abd8b3d..a9aa974 100644
--- a/tcg/ppc/tcg-target.h
+++ b/tcg/ppc/tcg-target.h
@@ -82,6 +82,7 @@ extern bool have_isa_3_00;
 #define TCG_TARGET_HAS_muls2_i320
 #define TCG_TARGET_HAS_muluh_i321
 #define TCG_TARGET_HAS_mulsh_i321
+#define TCG_TARGET_HAS_goto_ptr 0
 
 #if TCG_TARGET_REG_BITS == 64
 #define TCG_TARGET_HAS_add2_i32 0
diff --git a/tcg/s390/tcg-target.h b/tcg/s390/tcg-target.h
index cbdd2a6..6b7bcfb 100644
--- a/tcg/s390/tcg-target.h
+++ b/tcg/s390/tcg-target.h
@@ -92,6 +92,7 @@ extern uint64_t s390_facilities;
 #define TCG_TARGET_HAS_mulsh_i32  0
 #define TCG_TARGET_HAS_extrl_i64_i32  0
 #define TCG_TARGET_HAS_extrh_i64_i32  0
+#define TCG_TARGET_HAS_goto_ptr   0
 
 #define TCG_TARGET_HAS_div2_i64   1
 #define TCG_TARGET_HAS_rot_i641
diff --git a/tcg/sparc/tcg-target.h b/tcg/sparc/tcg-target.h
index b8b74f9..9348ddd 100644
--- a/tcg/sparc/tcg-target.h
+++ b/tcg/sparc/tcg-target.h
@@ -123,6 +123,7 @@ extern bool use_vis3_instructions;
 #define TCG_TARGET_HAS_muls2_i321
 #define TCG_TARGET_HAS_muluh_i320
 #define TCG_TARGET_HAS_mulsh_i320
+#define TCG_TARGET_HAS_goto_ptr 0
 
 #define TCG_TARGET_HAS_ex

[Qemu-devel] [PATCH v2 12/13] target/i386: optimize indirect branches

2017-04-25 Thread Emilio G. Cota
The appended minimizes exits to the exec loop for indirect branches.
By using the gen_jr helper, we can remain in TCG mode as long as
the indirect branch target is found in tb_jmp_cache.

This should improve performance for workloads that have a high
hit rate in tb_jmp_cache.

Softmmu Measurements: (see user-mode measurements in later commit)

Note: baseline (i.e. speedup == 1x) is QEMU v2.9.0.

-   SPECint06 (test set), x86_64-softmmu (Ubuntu 16.04 guest). 
Host: Intel i7-4790K @ 4.00GHz

 2.2x 
+-+--+-+
  | 
 +++ |
  |   cross+inline  
  |  |
   2x 
+cross+jr+inline+++.|+-+
  | 
   |  |  |
  | 
   |  |  |
  | 
   |  |  |
 1.8x 
+-+..|..|+-+
  | 
   | |
  | 
   |# |# |
 1.6x 
+-+.|#...+-+
  | 
 * |* |# |
  | 
 * |* |# |
  | 
 * |* |# |
 1.4x 
+-+...+++..*.|*.|#...+-+
  |  +++
 |   * |*++# +++ |
  |+++| 
 * |*  #  +++ |  |
 1.2x 
+-+..###.+++|.+++.#++#.*++*..#...|..|+-+
  | +++# # +++  | |  |
++#  # *  *  # +++  ## |
  | ++   #  +++  *** |   
  # *  *  #  ++  *| *|#  ++#|
  |++#  ++  *  * #    #  ++#| #  ++  *|*###  ##  *  
*  # *  *  #  *** |#  *++*+#  *++*  #|
   1x 
+-++-*++*++#++***+-#++*++*+#++*+-*++#+++#++***++#+-*+*++#-+*++*+#++*++*-+#+*++*-+#++*+*++#++*-+*+#++*++*++#-++-+
  |*  *  #  * *  #  *  * #  *  *  # *++*  #  * *  #  *+* |#  *  * #  *  
*  # *  *  #  * *  #  *  * #  *  *  #|
  |*  *  #  * *  #  *  * #  *  *  # *  *  #  * *  #  * *++#  *  * #  *  
*  # *  *  #  * *  #  *  * #  *  *  #|
 0.8x 
+-+--###--***###--##--###-###--***###--***###--##--###-###--***###--##--###--+-+
 astar   bzip2  gcc   gobmk h264ref   hmmlibquantum  mcf 
omnetpperlbench   sjengxalancbmk   hmean
  png: http://imgur.com/aSXm0qh

NB. 'cross' represents the previous commit.

Signed-off-by: Emilio G. Cota 
---
 target/i386/translate.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/target/i386/translate.c b/target/i386/translate.c
index 9982a2d..0b4e1e1 100644
--- a/target/i386/translate.c
+++ b/target/i386/translate.c
@@ -4991,7 +4991,7 @@ static target_ulong disas_insn(CPUX86State *env, 
DisasContext *s,
 gen_push_v(s, cpu_T1);
 gen_op_jmp_v(cpu_T0);
 gen_bnd_jmp(s);
-gen_eob(s);
+gen_jr(s, cpu_T0);
 break;
 case 3: /* lcall Ev */
 gen_op_ld_v(s, ot, cpu_T1, cpu_A0);
@@ -5009,7 +5009,8 @@ static target_ulong disas_insn(CPUX86State *env, 
DisasContext *s,
   tcg_const_i32(dflag - 1),
   tcg_const_i32(s->pc - s->cs_base));
 }
-gen_eob(s);
+tcg_gen_ld_tl(cpu_tmp4, cpu_env, offsetof(CPUX86State, eip));
+gen_jr(s, cpu_tmp4);
 break;
 case 4: /* jmp Ev */
 if (dflag == MO_16) {
@@ -5017,7 +5018,7 @@ static target_ulong disas_insn(CPUX86State *env, 
DisasContext *s,
 

[Qemu-devel] [PATCH v2 04/13] tcg: keep TCGContext's read-mostly fields in a separate cache line

2017-04-25 Thread Emilio G. Cota
Upcoming changes will require reading from TCGContext from a
parallel fast path. Prepare for this by keeping the struct's
read-mostly fields in a separate cache line, thereby preventing
false cache line sharing.

Signed-off-by: Emilio G. Cota 
---
 tcg/tcg.h | 23 ++-
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/tcg/tcg.h b/tcg/tcg.h
index 5fdbfe3..b26f0ef 100644
--- a/tcg/tcg.h
+++ b/tcg/tcg.h
@@ -642,6 +642,20 @@ QEMU_BUILD_BUG_ON(OPPARAM_BUF_SIZE > (1 << 14));
 QEMU_BUILD_BUG_ON(sizeof(TCGOp) > 8);
 
 struct TCGContext {
+/* Read-mostly fields go here to prevent false sharing */
+struct {
+GHashTable *helpers;
+
+void *code_gen_prologue;
+void *code_gen_buffer;
+size_t code_gen_buffer_size;
+
+/* Threshold to flush the translated code buffer.  */
+void *code_gen_highwater;
+
+int code_gen_max_blocks;
+} QEMU_ALIGNED(64);
+
 uint8_t *pool_cur, *pool_end;
 TCGPool *pool_first, *pool_current, *pool_first_large;
 int nb_labels;
@@ -663,8 +677,6 @@ struct TCGContext {
 
 tcg_insn_unit *code_ptr;
 
-GHashTable *helpers;
-
 #ifdef CONFIG_PROFILER
 /* profiling info */
 int64_t tb_count1;
@@ -697,15 +709,8 @@ struct TCGContext {
here, because there's too much arithmetic throughout that relies
on addition and subtraction working on bytes.  Rely on the GCC
extension that allows arithmetic on void*.  */
-int code_gen_max_blocks;
-void *code_gen_prologue;
-void *code_gen_buffer;
-size_t code_gen_buffer_size;
 void *code_gen_ptr;
 
-/* Threshold to flush the translated code buffer.  */
-void *code_gen_highwater;
-
 TBContext tb_ctx;
 
 /* Track which vCPU triggers events */
-- 
2.7.4




[Qemu-devel] [PATCH] Issue a deprecation warning if the user specifies the "-hdachs" option.

2017-04-25 Thread Thomas Huth
If the user needs to specify the disk geometry, the corresponding
parameters of the "-drive" option should be used instead. "-hdachs"
is considered as deprecated and might be removed soon.

Signed-off-by: Thomas Huth 
---
 vl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/vl.c b/vl.c
index 0b4ed52..0980213 100644
--- a/vl.c
+++ b/vl.c
@@ -3230,6 +3230,8 @@ int main(int argc, char **argv, char **envp)
 }
 }
 }
+error_report("'-hdachs' is deprecated. Please use '-drive"
+ " ...,cyls=c,heads=h,secs=s,trans=t' instead.");
 break;
 case QEMU_OPTION_numa:
 opts = qemu_opts_parse_noisily(qemu_find_opts("numa"),
-- 
1.8.3.1




Re: [Qemu-devel] [PATCH 8/9] Add end of test message

2017-04-25 Thread Peter Maydell
On 25 April 2017 at 04:24, G 3  wrote:
> Print the message "End of test" on the risu host end.
>
> Signed-off-by: John Arbuckle 
> ---
>  risu.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/risu.c b/risu.c
> index ed5b605..e7cbd57 100644
> --- a/risu.c
> +++ b/risu.c
> @@ -63,6 +63,7 @@ void apprentice_sigill(int sig, siginfo_t *si, void *uc)
>   return;
>case 1:
>   /* end of test */
> + printf("End of test\n");
>   exit(0);
>default:
>   /* mismatch */

This code is a signal handler, you can't use printf() here.

thanks
-- PMM



Re: [Qemu-devel] [PATCH 2/9] Add risu_ppc.c file

2017-04-25 Thread Peter Maydell
On 25 April 2017 at 04:18, G 3  wrote:
> Add the risu_ppc.c file. It defines several functions used by risu.
>
> Signed-off-by: John Arbuckle 
> ---
>  risu_ppc.c | 41 +
>  1 file changed, 41 insertions(+)
>  create mode 100644 risu_ppc.c
>
> diff --git a/risu_ppc.c b/risu_ppc.c
> new file mode 100644
> index 000..70a1cf7
> --- /dev/null
> +++ b/risu_ppc.c
> @@ -0,0 +1,41 @@
> +/*
> + * File: risu_ppc.c
> + * Date: 3-27-2017
> + * Description: Implement advance_pc(), set_ucontext_paramreg(),
> + *  get_reginfo_paramreg(), get_risuop()
> + */

All new files should have a brief copyright and license statement
in their header comment.

thanks
-- PMM



Re: [Qemu-devel] [PATCH 1/9] Add ppc.risu file

2017-04-25 Thread Peter Maydell
On 25 April 2017 at 04:17, G 3  wrote:
> Add the ppc.risu file. It defines the format for various PowerPC
> instructions.
>
> Signed-off-by: John Arbuckle 
> ---
>  ppc.risu | 527
> +++
>  1 file changed, 527 insertions(+)
>  create mode 100644 ppc.risu
>
> diff --git a/ppc.risu b/ppc.risu
> new file mode 100644
> index 000..b6d6aee
> --- /dev/null
> +++ b/ppc.risu
> @@ -0,0 +1,527 @@
> +###
> +# File: ppc.risu
> +# Date: 3-27-2017
> +# Description: Specifies PowerPC instruction test patterns.
> +###

Missing copyright and license statement.

> +
> +.mode ppc
> +
> +# Note: register r1 cannot be used because it is the stack pointer.
> +# The branching, VEA, and OEA instructions cannot be used here currently.
> +
> +ADD PPC 01 rD:5 rA:5 rB:5 OE:1 11010 Rc:1 \
> +!constraints { $rD != 1 && $rA != 1 && $rB != 1; }
> +
> +ADDC PPC 01 rD:5 rA:5 rB:5 OE:1 01010 Rc:1 \
> +!constraints { $rD != 1 && $rA != 1 && $rB != 1; }
> +
> +ADDE PPC 01 rD:5 rA:5 rB:5 OE:1 010001010 Rc:1 \
> +!constraints { $rD != 1 && $rA != 1 && $rB != 1; }


There's an awful lot of duplication with ppc64 here. Is it
worth trying to share?


thanks
-- PMM



Re: [Qemu-devel] [PATCH 3/9] Add risu_reginfo_ppc.c file

2017-04-25 Thread Peter Maydell
On 25 April 2017 at 04:19, G 3  wrote:
> Add the risu_reginfo_ppc.c file. It handles operations involving the reginfo
> structure.

> +/* check each floating point register */
> +for (i = 0; i < NUMBER_OF_FPR; i++) {
> +if (r1->fpr[i] != r2->fpr[i]) {
> +if (!(isnan(r1->fpr[i]) && isnan(r2->fpr[i]))) {
> +if ( fabs(r1->fpr[i] - r2->fpr[i]) < 0.01) {
> +debug_print("float point register %d mismatch
> detected\n", i);
> +return 0;
> +}

This is definitely wrong. Risu is supposed to check for exact
binary correctness, so you simply want to compare the binary
values of the FP regs, not check whether they're vaguely
close to the right answer.

> +/*
> + * Shows the classification of a floating point value.
> + * Input: floating point value
> + * Output: string description
> + */
> +const char *show_classification(double x) {
> +switch(fpclassify(x)) {
> +case FP_INFINITE:  return "Inf";
> +case FP_NAN:   return "NaN";
> +case FP_NORMAL:return "normal";
> +case FP_SUBNORMAL: return "subnormal";
> +case FP_ZERO:  return "zero";
> +default:   return "unknown";
> +}
> +}

None of the other backends do this. If we want to do it
(and I'm not convinced it's worth the effort) we should
do it consistently everywhere.

thanks
-- PMM



Re: [Qemu-devel] [PATCH 4/6] migration: calculate downtime on dst side

2017-04-25 Thread Peter Xu
On Fri, Apr 14, 2017 at 04:17:18PM +0300, Alexey Perevalov wrote:

[...]

> +/*
> + * This function calculates downtime per cpu and trace it
> + *
> + *  Also it calculates total downtime as an interval's overlap,
> + *  for many vCPU.
> + *
> + *  The approach is following:
> + *  Initially intervals are represented in tree where key is
> + *  pagefault address, and values:
> + *   begin - page fault time
> + *   end   - page load time
> + *   cpus  - bit mask shows affected cpus
> + *
> + *  To calculate overlap on all cpus, intervals converted into
> + *  array of points in time (downtime_points), the size of
> + *  array is 2 * number of nodes in tree of intervals (2 array
> + *  elements per one in element of interval).
> + *  Each element is marked as end (E) or as start (S) of interval.
> + *  The overlap downtime will be calculated for SE, only in case
> + *  there is sequence S(0..N)E(M) for every vCPU.
> + *
> + * As example we have 3 CPU
> + *
> + *  S1E1   S1   E1
> + * -***xxx***> 
> CPU1
> + *
> + * S2E2
> + * xxx---> 
> CPU2
> + *
> + * S3E3
> + * xxx---> 
> CPU3
> + *
> + * We have sequence S1,S2,E1,S3,S1,E2,E3,E1
> + * S2,E1 - doesn't match condition due to sequence S1,S2,E1 doesn't include 
> CPU3
> + * S3,S1,E2 - sequenece includes all CPUs, in this case overlap will be S1,E2
> + * Legend of picture is following: * - means downtime per vCPU
> + * x - means overlapped downtime
> + */

Not sure whether I get the point in this patch... iiuc we defined the
downtime here as the period when all vcpus are halted, right?

If so, I have a few questions:

- will this algorithm consume lots of memory? since I see we have one
  trace object per fault page address

- do we need to protect the tree to make sure there's no insertion
  when doing the calculation?

- if the only thing we want here is the "total downtime", whether
  below would work? (assuming N is vcpu numbers)

  a. define array cpu_fault_addr[N], to store current faulted address
 for each vcpu. When vcpu X is running, cpu_fault_addr[X] should
 be 0.

  b. when page fault happens on vcpu A, setup cpu_fault_addr[A] with
 corresponding fault address.

  c. when page copy finished, loop over cpu_fault_addr[] to see
 whether that matches any, clear corresponding element if matched.

  Then, we can just measure the period when cpu_fault_addr[] is all
  set (by tracing at both b. and c.). Can this work?

Thanks,

-- 
Peter Xu



[Qemu-devel] [Bug 1289898] Re: qemu-system-ppc64 easily cause file corruption

2017-04-25 Thread Thomas Huth
** Changed in: qemu
   Status: New => Incomplete

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

Title:
  qemu-system-ppc64 easily cause file corruption

Status in QEMU:
  Incomplete

Bug description:
  the qemu-system-ppc64 is used to run RHEL5.9 for IBM Power on RHEL 5.3.
  Previously I was using QEMU 1.5.x for several months with no problem. But 
after the RHEL 5.3 host damaged, and rebuilt, now I tried both QEMU 1.6.2 and 
QEMU 1.7.0, found both can easily cause file corruptions. Symptoms:

  * using scp to transfer a tar.bz file from the RHEL 5.3 host to the RHEL 5.9 
PPC VM, found the size is correct, but the content is corrupted from the middle 
of the 90+ MB file.  re-transfer again, got a correct file. But after untar, 
found some extracted files corrupted.
  The extracted file corruption happened several times, and also the filesystem 
in the VM had corrupted several times, had to restore the boot image to recover.

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



[Qemu-devel] [Bug 1288620] Re: memory leak with config file

2017-04-25 Thread Thomas Huth
Can you still reproduce this issue with the latest version of QEMU
(currently version 2.9)?

** Changed in: qemu
   Status: New => Incomplete

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

Title:
  memory leak with config file

Status in QEMU:
  Incomplete

Bug description:
  I have a Windows 7 SP1 Professional 64-bit installation on a QCOW2
  image with compat=1.1, which I launch via

  qemu-system-x86_64 -drive file=windows_base_HDD.img,index=0,media=disk
  -enable-kvm -m 512M -vga std -net nic,vlan=0 -net user,vlan=0

  As soon as I start using the network in any application — for example,
  visiting www.google.com in Internet Explorer — QEMU starts gobbling
  memory until the (host) kernel kills it because of an OOM condition.
  If I run the QEMU with the same options, but with model=e1000 option
  set for the NIC (i.e. -net -nic,vlan=0,model=e1000), I can use the
  network from the guest OS without any noticeable effect on QEMU's
  memory consumption.

  I do not have this problem when running QEMU with the exact same options (as 
above, without model=e1000) but with a Debian wheezy installation (on a QCOW 
image of the same format).  My host system in Ubuntu 13.10 x86_64, kernel image 
3.11.0-17-generic, but with the QEMU packages from trusty (the codename for the 
next release):
  Output of `dpkg -l \*qemu\* | grep '^ii'`:
  ii  ipxe-qemu 1.0.0+git-20130710.936134e-0ubuntu1 
   all  Virtual package to support use of kvm-ipxe with qemu
  ii  qemu-keymaps  1.7.0+dfsg-3ubuntu2 
   all  QEMU keyboard maps
  ii  qemu-system-common1.7.0+dfsg-3ubuntu2 
   amd64QEMU full system emulation binaries (common files)
  ii  qemu-system-x86   1.7.0+dfsg-3ubuntu2 
   amd64QEMU full system emulation binaries (x86)
  ii  qemu-utils1.7.0+dfsg-3ubuntu2 
   amd64QEMU utilities

  (If necessary, I can try to reproduce this with QEMU built from the
  upstream source or the latest source from version control.)

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



Re: [Qemu-devel] [PATCH 2/9] Add risu_ppc.c file

2017-04-25 Thread Peter Maydell
On 25 April 2017 at 04:18, G 3  wrote:
> Add the risu_ppc.c file. It defines several functions used by risu.
>
> Signed-off-by: John Arbuckle 
> ---
>  risu_ppc.c | 41 +
>  1 file changed, 41 insertions(+)
>  create mode 100644 risu_ppc.c
>
> diff --git a/risu_ppc.c b/risu_ppc.c
> new file mode 100644
> index 000..70a1cf7
> --- /dev/null
> +++ b/risu_ppc.c
> @@ -0,0 +1,41 @@
> +/*
> + * File: risu_ppc.c
> + * Date: 3-27-2017
> + * Description: Implement advance_pc(), set_ucontext_paramreg(),
> + *  get_reginfo_paramreg(), get_risuop()
> + */
> +
> +#include "risu.h"
> +
> +/* Advances the program counter register to the next instruction */
> +void advance_pc(void *vuc)
> +{
> +ucontext_t *uc = (ucontext_t*)vuc;
> +uc->uc_mcontext->ss.srr0 += 4;
> +}
> +
> +/* Sets register r0 to the address of a memory block. */
> +void set_ucontext_paramreg(void *vuc, uint64_t value)
> +{
> +ucontext_t *uc = vuc;
> +uc->uc_mcontext->ss.r0 = value;
> +}

This doesn't look like it matches the Linux PPC uc_mcontext
struct, which (like the ppc64 one) has a uc_mcontext.regs
field, not uc_mcontext->ss.

Risu is a Linux program, so all CPU backends must build
on Linux. It could perhaps be ported to other host OSes,
but we'd need to put in more infrastructure for dealing
with host OS differences, not just have "PPC works only
on OSX and the others work only on Linux".

thanks
-- PMM



[Qemu-devel] [PATCH RFC] configure: fix clang failure for libatomic

2017-04-25 Thread Nikunj A Dadhania
Travis builds failure was reported for powernv boot-serial test with
qemu built with clang.

Debugging revealed that CONFIG_ATOMIC64 wasnt getting set for the clang
build because of that atomic operations weren't being used and was
resulting in MTTCG failure in the powernv boot-serial test.

libatomic is required to successfully test atomic64 and atomic128 for
clang. Introduced newer checks for the same. And on failure default to
single threaded tcg support in PPC64.

Signed-off-by: Nikunj A Dadhania 
---

Reference:
https://lists.gnu.org/archive/html/qemu-ppc/2017-04/msg00277.html

---
 configure | 16 
 1 file changed, 16 insertions(+)

diff --git a/configure b/configure
index d31a3e8..1e5f7af 100755
--- a/configure
+++ b/configure
@@ -4598,6 +4598,9 @@ int main(void)
 EOF
   if compile_prog "" "" ; then
 atomic128=yes
+  elif compile_prog "" "-latomic" ; then
+ atomic128=yes
+ lib_atomic="-latomic"
   fi
 fi
 
@@ -4628,6 +4631,9 @@ int main(void)
 EOF
 if compile_prog "" "" ; then
   atomic64=yes
+elif compile_prog "" "-latomic" ; then
+  atomic64=yes
+  lib_atomic="-latomic"
 fi
 
 
@@ -6065,6 +6071,16 @@ if [ "$TARGET_BASE_ARCH" = "" ]; then
   TARGET_BASE_ARCH=$TARGET_ARCH
 fi
 
+if test $atomic64 == "yes" || test $atomic128 == "yes" ; then
+libs_softmmu="$lib_atomic $libs_softmmu"
+elif test $mttcg == "yes" && test $TARGET_BASE_ARCH == "ppc"; then
+echo
+echo "Note: Atomic library (-latomic) not available, falling"
+echo "  back to single threaded mode by default"
+echo
+mttcg=no
+fi
+
 symlink "$source_path/Makefile.target" "$target_dir/Makefile"
 
 upper() {
-- 
2.9.3




Re: [Qemu-devel] [PATCH 1/3] colo-compare: serialize compare thread's initialization with main thread

2017-04-25 Thread Jason Wang



On 2017年04月24日 14:03, Hailiang Zhang wrote:

On 2017/4/24 12:10, Jason Wang wrote:


On 2017年04月20日 15:46, zhanghailiang wrote:

We call qemu_chr_fe_set_handlers() in colo-compare thread, it is used
to detach watched fd from default main context, so it has chance to
handle the same watched fd with main thread concurrently, which will
trigger an error report:
"qemu-char.c:918: io_watch_poll_finalize: Assertion `iwp->src == 
((void *)0)' failed."

Anyway to prevent fd from being handled by main thread before creating
colo thread? Using semaphore seems not elegant.


So how about calling qemu_mutex_lock_iothread() before 
qemu_chr_fe_set_handlers() ?


Looks better, but I needs more information e.g how main thread can touch it?

Thanks



Re: [Qemu-devel] [PATCH RFC] configure: fix clang failure for libatomic

2017-04-25 Thread Peter Maydell
On 25 April 2017 at 09:35, Nikunj A Dadhania  wrote:
> Travis builds failure was reported for powernv boot-serial test with
> qemu built with clang.
>
> Debugging revealed that CONFIG_ATOMIC64 wasnt getting set for the clang
> build because of that atomic operations weren't being used and was
> resulting in MTTCG failure in the powernv boot-serial test.
>
> libatomic is required to successfully test atomic64 and atomic128 for
> clang. Introduced newer checks for the same. And on failure default to
> single threaded tcg support in PPC64.
>
> Signed-off-by: Nikunj A Dadhania 

Do we really need libatomic? I thought the intention here was
that atomic operations were only done on types of width of the
pointer or less, which should all be doable by the compiler inline,
and thus don't need the external library. In the past "doesn't
work without libatomic" usually meant "accidentally tried to
do an atomic operation on a type that is too wide".

thanks
-- PMM



Re: [Qemu-devel] [PATCH RFC] configure: fix clang failure for libatomic

2017-04-25 Thread Nikunj A Dadhania
Peter Maydell  writes:

> On 25 April 2017 at 09:35, Nikunj A Dadhania  
> wrote:
>> Travis builds failure was reported for powernv boot-serial test with
>> qemu built with clang.
>>
>> Debugging revealed that CONFIG_ATOMIC64 wasnt getting set for the clang
>> build because of that atomic operations weren't being used and was
>> resulting in MTTCG failure in the powernv boot-serial test.
>>
>> libatomic is required to successfully test atomic64 and atomic128 for
>> clang. Introduced newer checks for the same. And on failure default to
>> single threaded tcg support in PPC64.
>>
>> Signed-off-by: Nikunj A Dadhania 
>
> Do we really need libatomic?

I was trying out the program in the configure script with clang and I do
get errors without libatomic:

$ clang /tmp/atomic.c 
/tmp/atomic.c:6:7: warning: implicit declaration of function 
'__atomic_load_8' is invalid in C99 [-Wimplicit-function-declaration]
  y = __atomic_load_8(&x, 0);
  ^
/tmp/atomic.c:7:3: warning: implicit declaration of function 
'__atomic_store_8' is invalid in C99 [-Wimplicit-function-declaration]
  __atomic_store_8(&x, y, 0);
  ^
/tmp/atomic.c:8:3: warning: implicit declaration of function 
'__atomic_compare_exchange_8' is invalid in C99 
[-Wimplicit-function-declaration]
  __atomic_compare_exchange_8(&x, &y, x, 0, 0, 0);
  ^
/tmp/atomic.c:9:3: warning: implicit declaration of function 
'__atomic_exchange_8' is invalid in C99 [-Wimplicit-function-declaration]
  __atomic_exchange_8(&x, y, 0);
  ^
/tmp/atomic.c:10:3: warning: implicit declaration of function 
'__atomic_fetch_add_8' is invalid in C99 [-Wimplicit-function-declaration]
  __atomic_fetch_add_8(&x, y, 0);
  ^
5 warnings generated.
/tmp/atomic-1660e0.o: In function `main':
/tmp/atomic.c:(.text+0x28): undefined reference to `__atomic_load_8'
/tmp/atomic.c:(.text+0x40): undefined reference to `__atomic_store_8'
/tmp/atomic.c:(.text+0x69): undefined reference to 
`__atomic_compare_exchange_8'
/tmp/atomic.c:(.text+0x7d): undefined reference to `__atomic_exchange_8'
/tmp/atomic.c:(.text+0x91): undefined reference to `__atomic_fetch_add_8'
clang-3.9: error: linker command failed with exit code 1 (use -v to see 
invocation)

With -latomic, the linker succeeds in getting the binary.

> I thought the intention here was that atomic operations were only done
> on types of width of the pointer or less, which should all be doable
> by the compiler inline, and thus don't need the external library.

You are right, even without -latomic in libs_softmmu, tests are passing.
But then __atomic_load_8() isn't used in qemu, if it was using them,
build would fail.

> In the past "doesn't work without libatomic" usually meant
>"accidentally tried to do an atomic operation on a type that is too
>wide".

Regards
Nikunj




Re: [Qemu-devel] [PATCH 02/19] migration: They are called vmstate_foo, move them to vmstate.c

2017-04-25 Thread Juan Quintela
"Dr. David Alan Gilbert"  wrote:
> * Juan Quintela (quint...@redhat.com) wrote:
>> Signed-off-by: Juan Quintela 
>> ---
>>  migration/savevm.c  | 17 -
>>  migration/vmstate.c | 16 
>>  2 files changed, 16 insertions(+), 17 deletions(-)
>
> OK, but it's a little odd, savevm.c still has the 
> vmstate_register_with_alias_id
> and vmstate-unregister,  so this is just the simple ram cases.

Dropped the patch.

vmstate.c ends only having the interpreter on it.



Re: [Qemu-devel] [PATCH 07/19] migration: Export socket.c functions in its own file

2017-04-25 Thread Juan Quintela
"Dr. David Alan Gilbert"  wrote:
> * Juan Quintela (quint...@redhat.com) wrote:
>> Signed-off-by: Juan Quintela 
>> ---
>>  include/migration/migration.h |  8 
>>  include/migration/socket.h| 25 +
>>  migration/migration.c |  1 +
>>  migration/socket.c|  1 +
>>  4 files changed, 27 insertions(+), 8 deletions(-)
>>  create mode 100644 include/migration/socket.h
>> 
>> diff --git a/include/migration/migration.h b/include/migration/migration.h
>> index 077b75b..0c6dae5 100644
>> --- a/include/migration/migration.h
>> +++ b/include/migration/migration.h
>> @@ -194,14 +194,6 @@ void migration_tls_channel_connect(MigrationState *s,
>>  
>>  uint64_t migrate_max_downtime(void);
>>  
>> -void tcp_start_incoming_migration(const char *host_port, Error **errp);
>> -
>> -void tcp_start_outgoing_migration(MigrationState *s, const char
>> *host_port, Error **errp);
>> -
>> -void unix_start_incoming_migration(const char *path, Error **errp);
>> -
>> -void unix_start_outgoing_migration(MigrationState *s, const char
>> *path, Error **errp);
>> -
>>  void rdma_start_outgoing_migration(void *opaque, const char
>> *host_port, Error **errp);
>>  
>>  void rdma_start_incoming_migration(const char *host_port, Error **errp);
>
> Have you considered a header with just the transports in it?
>
> There doesn't seem to be that much point with one with just the
> rdma's in, and one iwth just the tcp's in etc.

In the future, the transports should be using module_type() and no need
of this stuff at all.  It should make easier to compile transports out
(famous last words).

Later, Juan.



Re: [Qemu-devel] [PATCH RFC] configure: fix clang failure for libatomic

2017-04-25 Thread Peter Maydell
On 25 April 2017 at 09:58, Nikunj A Dadhania  wrote:
> I was trying out the program in the configure script with clang and I do
> get errors without libatomic:
>
> $ clang /tmp/atomic.c
> /tmp/atomic.c:6:7: warning: implicit declaration of function 
> '__atomic_load_8' is invalid in C99 [-Wimplicit-function-declaration]
>   y = __atomic_load_8(&x, 0);
>   ^
> /tmp/atomic.c:7:3: warning: implicit declaration of function 
> '__atomic_store_8' is invalid in C99 [-Wimplicit-function-declaration]
>   __atomic_store_8(&x, y, 0);
>   ^
> /tmp/atomic.c:8:3: warning: implicit declaration of function 
> '__atomic_compare_exchange_8' is invalid in C99 
> [-Wimplicit-function-declaration]
>   __atomic_compare_exchange_8(&x, &y, x, 0, 0, 0);
>   ^
> /tmp/atomic.c:9:3: warning: implicit declaration of function 
> '__atomic_exchange_8' is invalid in C99 [-Wimplicit-function-declaration]
>   __atomic_exchange_8(&x, y, 0);
>   ^
> /tmp/atomic.c:10:3: warning: implicit declaration of function 
> '__atomic_fetch_add_8' is invalid in C99 [-Wimplicit-function-declaration]
>   __atomic_fetch_add_8(&x, y, 0);
>   ^
> 5 warnings generated.
> /tmp/atomic-1660e0.o: In function `main':
> /tmp/atomic.c:(.text+0x28): undefined reference to `__atomic_load_8'
> /tmp/atomic.c:(.text+0x40): undefined reference to `__atomic_store_8'
> /tmp/atomic.c:(.text+0x69): undefined reference to 
> `__atomic_compare_exchange_8'
> /tmp/atomic.c:(.text+0x7d): undefined reference to `__atomic_exchange_8'
> /tmp/atomic.c:(.text+0x91): undefined reference to `__atomic_fetch_add_8'
> clang-3.9: error: linker command failed with exit code 1 (use -v to see 
> invocation)
>
> With -latomic, the linker succeeds in getting the binary.

What host is this on ?

thanks
-- PMM



Re: [Qemu-devel] [PATCH] crypto: move 'opaque' parameter to (nearly) the end of parameter list

2017-04-25 Thread Daniel P. Berrange
On Tue, Apr 25, 2017 at 09:30:13AM +0200, Markus Armbruster wrote:
> "Daniel P. Berrange"  writes:
> 
> > Previous commit moved 'opaque' to be the 2nd parameter in the list:
> >
> >   commit 375092332eeaa6e47561ce47fd36144cdaf964d0
> >   Author: Fam Zheng 
> >   Date:   Fri Apr 21 20:27:02 2017 +0800
> >
> > crypto: Make errp the last parameter of functions
> >
> > Move opaque to 2nd instead of the 2nd to last, so that compilers help
> > check with the conversion.
> >
> > this puts it back to the 2nd to last position.
> >
> > Signed-off-by: Daniel P. Berrange 
> 
> I could take this through my tree, but I suggest you stick it into your
> next crypto pull request.

Yes, I'll submit it myself as I have a bunch of other stuff pending.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|



Re: [Qemu-devel] [PATCH v2 00/25] qmp: add async command type

2017-04-25 Thread Daniel P. Berrange
On Mon, Apr 24, 2017 at 09:10:22PM +0200, Markus Armbruster wrote:
> With 2.9 out of the way, how can we make progress on this one?
> 
> I can see two ways to get asynchronous QMP commands accepted:
> 
> 1. We break QMP compatibility in QEMU 3.0 and convert all long-running
>tasks from "synchronous command + event" to "asynchronous command".
> 
>This is design option 1 quoted below.  *If* we decide to leave
>compatibility behind for 3.0, *and* we decide we like the
>asynchronous sufficiently better to put in the work, we can do it.
> 
>I guess there's nothing to do here until we decide on breaking
>compatibility in 3.0.

>From the libvirt POV we'll generally be against QEMU breaking back
compatibility, if there's a viable option which can avoid the back
compat breakage.

Is it possible to do option 1, but have it be an opt-in. ie when
libvirt does the initial QMP greeting, it could issue a command
to active async mode. For simplicity that could be an all-or-nothing
switch - ie makes all commands be async. That way we avoid breaking
any existing libvirt, but give new libvirt the chance to opt-in to
the new way.

Regardless new libvirt will end up having to support both modes of
QMP for 5-10 years...

> 2. We don't break QMP compatibility, but we add asynchronous commands
>anyway, because we decide that's how we want to do "jobs".
> 
>This is design option 3 quoted below.  As I said, I dislike its lack
>of orthogonality.  But if asynchronous commands help us get jobs
>done, I can bury my dislike.
> 
>I feel this is something you should investigate with John Snow.
>Going through a middleman (me) makes no sense.  So I'm going to dump
>my thoughts, then get out of the way.
> 
>You need to take care not to get bogged down in the jobs project's
>complexity.  This is really only how to package up jobs for QMP.
> 
>With synchronous commands, the job-creating command creates a job,
>jobs state changes trigger events, and job termination is just
>another state change.  Job control commands interact with the job.
>  
>Events obviously need to carry a job ID.  We can either require the
>user to pass it as argument to the job-creating command (hopefully
>unique), or have the job-creating command pick one (a unique one) and
>return it.
> 
>With asynchronous commands, we could make the asynchronous command
>the job.  The only difference is that job termination triggers the
>command response.  When termination is of no interest to anyone but
>the job's creator, the termination event can be omitted then.
> 
>Instead of a job ID, we could use the (user-specified and hopefully
>unique) command ID that ties the command response to the command.
>Perhaps throw in a monitor ID.
> 
>To be honest, I'm not sure asynchronous commands buy us much here.
>But my view is from 10,000 feet, and John might have different ideas.
> 
> Rejecting asynchronous QMP commands is of course design option 2 quoted
> below.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|



Re: [Qemu-devel] [PATCH RFC] configure: fix clang failure for libatomic

2017-04-25 Thread Nikunj A Dadhania
Peter Maydell  writes:

> On 25 April 2017 at 09:58, Nikunj A Dadhania  
> wrote:
>> I was trying out the program in the configure script with clang and I do
>> get errors without libatomic:
>>
>> $ clang /tmp/atomic.c
>> /tmp/atomic.c:6:7: warning: implicit declaration of function 
>> '__atomic_load_8' is invalid in C99 [-Wimplicit-function-declaration]
>>   y = __atomic_load_8(&x, 0);
>>   ^
>> /tmp/atomic.c:7:3: warning: implicit declaration of function 
>> '__atomic_store_8' is invalid in C99 [-Wimplicit-function-declaration]
>>   __atomic_store_8(&x, y, 0);
>>   ^
>> /tmp/atomic.c:8:3: warning: implicit declaration of function 
>> '__atomic_compare_exchange_8' is invalid in C99 
>> [-Wimplicit-function-declaration]
>>   __atomic_compare_exchange_8(&x, &y, x, 0, 0, 0);
>>   ^
>> /tmp/atomic.c:9:3: warning: implicit declaration of function 
>> '__atomic_exchange_8' is invalid in C99 [-Wimplicit-function-declaration]
>>   __atomic_exchange_8(&x, y, 0);
>>   ^
>> /tmp/atomic.c:10:3: warning: implicit declaration of function 
>> '__atomic_fetch_add_8' is invalid in C99 [-Wimplicit-function-declaration]
>>   __atomic_fetch_add_8(&x, y, 0);
>>   ^
>> 5 warnings generated.
>> /tmp/atomic-1660e0.o: In function `main':
>> /tmp/atomic.c:(.text+0x28): undefined reference to `__atomic_load_8'
>> /tmp/atomic.c:(.text+0x40): undefined reference to `__atomic_store_8'
>> /tmp/atomic.c:(.text+0x69): undefined reference to 
>> `__atomic_compare_exchange_8'
>> /tmp/atomic.c:(.text+0x7d): undefined reference to `__atomic_exchange_8'
>> /tmp/atomic.c:(.text+0x91): undefined reference to `__atomic_fetch_add_8'
>> clang-3.9: error: linker command failed with exit code 1 (use -v to see 
>> invocation)
>>
>> With -latomic, the linker succeeds in getting the binary.
>
> What host is this on ?

$ uname -a
Linux abhimanyu 4.9.13-200.fc25.x86_64 #1 SMP Mon Feb 27 16:48:42 UTC 2017 
x86_64 x86_64 x86_64 GNU/Linux
$

Regards
Nikunj




[Qemu-devel] pull requests and ARM patches during May

2017-04-25 Thread Peter Maydell
Hi all; I'm taking a break during May and won't be reading work
emails during that time. Alex Bennée has volunteered to look after
ARM patches, and Stefan Hajnoczi has agreed to handle applying
pull requests. People working on ARM related patches can also
help each other out by reviewing each others' patches.

See you all in June :-)

thanks
-- PMM



Re: [Qemu-devel] [PATCH v3 3/3] tests: Add a tester for HMP commands

2017-04-25 Thread Dr. David Alan Gilbert
* Thomas Huth (th...@redhat.com) wrote:
> HMP commands do not get any automatic testing yet, so on certain
> QEMU machines, some HMP commands were causing crashes in the past.
> Thus we should test HMP commands in our test suite, too, to avoid
> that such problems creep in again in the future.
> 
> Signed-off-by: Thomas Huth 

Thanks,

Reviewed-by: Dr. David Alan Gilbert 

and queued

Dave

> ---
>  v3:
>  - Fixed the stupid "!strcmp() == 0" problem
>  - Removed "isapc" from the blacklist since the "info lapic" problem
>has already been fixed (see commit c7f15bc93661a36fe)
>  - Fixed the g_strdup_printf("hmp/%s", mname) memory leak
> 
>  tests/Makefile.include |   2 +
>  tests/test-hmp.c   | 161 
> +
>  2 files changed, 163 insertions(+)
>  create mode 100644 tests/test-hmp.c
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 579ec07..31931c0 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -331,6 +331,7 @@ check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
>  check-qtest-s390x-y = tests/boot-serial-test$(EXESUF)
>  
>  check-qtest-generic-y += tests/qom-test$(EXESUF)
> +check-qtest-generic-y += tests/test-hmp$(EXESUF)
>  
>  qapi-schema += alternate-any.json
>  qapi-schema += alternate-array.json
> @@ -720,6 +721,7 @@ tests/tpci200-test$(EXESUF): tests/tpci200-test.o
>  tests/display-vga-test$(EXESUF): tests/display-vga-test.o
>  tests/ipoctal232-test$(EXESUF): tests/ipoctal232-test.o
>  tests/qom-test$(EXESUF): tests/qom-test.o
> +tests/test-hmp$(EXESUF): tests/test-hmp.o
>  tests/drive_del-test$(EXESUF): tests/drive_del-test.o $(libqos-pc-obj-y)
>  tests/qdev-monitor-test$(EXESUF): tests/qdev-monitor-test.o 
> $(libqos-pc-obj-y)
>  tests/nvme-test$(EXESUF): tests/nvme-test.o
> diff --git a/tests/test-hmp.c b/tests/test-hmp.c
> new file mode 100644
> index 000..99e35ec
> --- /dev/null
> +++ b/tests/test-hmp.c
> @@ -0,0 +1,161 @@
> +/*
> + * Test HMP commands.
> + *
> + * Copyright (c) 2017 Red Hat Inc.
> + *
> + * Author:
> + *Thomas Huth 
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2
> + * or later. See the COPYING file in the top-level directory.
> + *
> + * This test calls some HMP commands for all machines that the current
> + * QEMU binary provides, to check whether they terminate successfully
> + * (i.e. do not crash QEMU).
> + */
> +
> +#include "qemu/osdep.h"
> +#include "libqtest.h"
> +
> +static int verbose;
> +
> +static const char *hmp_cmds[] = {
> +"boot_set ndc",
> +"chardev-add null,id=testchardev1",
> +"chardev-remove testchardev1",
> +"commit all",
> +"cpu-add 1",
> +"cpu 0",
> +"device_add ?",
> +"device_add usb-mouse,id=mouse1",
> +"mouse_button 7",
> +"mouse_move 10 10",
> +"mouse_button 0",
> +"device_del mouse1",
> +"dump-guest-memory /dev/null 0 4096",
> +"gdbserver",
> +"host_net_add user id=net0",
> +"hostfwd_add tcp::43210-:43210",
> +"hostfwd_remove tcp::43210-:43210",
> +"host_net_remove 0 net0",
> +"i /w 0",
> +"log all",
> +"log none",
> +"memsave 0 4096 \"/dev/null\"",
> +"migrate_set_cache_size 1",
> +"migrate_set_downtime 1",
> +"migrate_set_speed 1",
> +"netdev_add user,id=net1",
> +"set_link net1 off",
> +"set_link net1 on",
> +"netdev_del net1",
> +"nmi",
> +"o /w 0 0x1234",
> +"object_add memory-backend-ram,id=mem1,size=256M",
> +"object_del mem1",
> +"pmemsave 0 4096 \"/dev/null\"",
> +"p $pc + 8",
> +"qom-list /",
> +"qom-set /machine initrd test",
> +"screendump /dev/null",
> +"sendkey x",
> +"singlestep on",
> +"wavcapture /dev/null",
> +"stopcapture 0",
> +"sum 0 512",
> +"x /8i 0x100",
> +"xp /16x 0",
> +NULL
> +};
> +
> +/* Run through the list of pre-defined commands */
> +static void test_commands(void)
> +{
> +char *response;
> +int i;
> +
> +for (i = 0; hmp_cmds[i] != NULL; i++) {
> +if (verbose) {
> +fprintf(stderr, "\t%s\n", hmp_cmds[i]);
> +}
> +response = hmp(hmp_cmds[i]);
> +g_free(response);
> +}
> +
> +}
> +
> +/* Run through all info commands and call them blindly (without arguments) */
> +static void test_info_commands(void)
> +{
> +char *resp, *info, *info_buf, *endp;
> +
> +info_buf = info = hmp("help info");
> +
> +while (*info) {
> +/* Extract the info command, ignore parameters and description */
> +g_assert(strncmp(info, "info ", 5) == 0);
> +endp = strchr(&info[5], ' ');
> +g_assert(endp != NULL);
> +*endp = '\0';
> +/* Now run the info command */
> +if (verbose) {
> +fprintf(stderr, "\t%s\n", info);
> +}
> +resp = hmp(info);
> +g_free(resp);
> +/* And move forward to the next line */
> +info = strchr(endp + 1, '\n');
> +if (!info) {
> +  

Re: [Qemu-devel] [PATCH RFC] configure: fix clang failure for libatomic

2017-04-25 Thread Peter Maydell
On 25 April 2017 at 10:16, Nikunj A Dadhania  wrote:
> Peter Maydell  writes:
>
>> On 25 April 2017 at 09:58, Nikunj A Dadhania  
>> wrote:
>>> /tmp/atomic-1660e0.o: In function `main':
>>> /tmp/atomic.c:(.text+0x28): undefined reference to `__atomic_load_8'
>>> /tmp/atomic.c:(.text+0x40): undefined reference to `__atomic_store_8'
>>> /tmp/atomic.c:(.text+0x69): undefined reference to 
>>> `__atomic_compare_exchange_8'
>>> /tmp/atomic.c:(.text+0x7d): undefined reference to `__atomic_exchange_8'
>>> /tmp/atomic.c:(.text+0x91): undefined reference to 
>>> `__atomic_fetch_add_8'
>>> clang-3.9: error: linker command failed with exit code 1 (use -v to see 
>>> invocation)
>>>
>>> With -latomic, the linker succeeds in getting the binary.
>>
>> What host is this on ?
>
> $ uname -a
> Linux abhimanyu 4.9.13-200.fc25.x86_64 #1 SMP Mon Feb 27 16:48:42 UTC 2017 
> x86_64 x86_64 x86_64 GNU/Linux

Hmm, I can repro that, but there's a problem even if we do link against
libatomic. If you disassemble the resulting binaries, gcc produces
what we want:
 f0 48 01 45 e8  lock add %rax,-0x18(%rbp)
and other inline atomic instructions.
clang on the other hand produces
 e8 33 fe ff ff  callq  400610 <__atomic_fetch_add_8@plt>
which is an expensive call to an out of line subroutine.

We need to figure out how to get clang to just emit the inline
operations.

thanks
-- PMM



Re: [Qemu-devel] [PATCH 16/19] migration: Export ram.c functions in its own file

2017-04-25 Thread Juan Quintela
"Dr. David Alan Gilbert"  wrote:
> * Juan Quintela (quint...@redhat.com) wrote:
>> Signed-off-by: Juan Quintela 
>> ---
>>  include/migration/migration.h | 36 -
>>  include/migration/ram.h   | 54 
>> +++
>>  migration/migration.c |  1 +
>>  migration/postcopy-ram.c  |  1 +
>>  migration/ram.c   |  1 +
>>  migration/rdma.c  |  1 +
>>  migration/savevm.c|  1 +
>>  7 files changed, 59 insertions(+), 36 deletions(-)
>>  create mode 100644 include/migration/ram.h
>> 
>> diff --git a/include/migration/migration.h b/include/migration/migration.h
>> index 1451067..3e5d106 100644
>> --- a/include/migration/migration.h
>> +++ b/include/migration/migration.h
>> @@ -192,36 +192,6 @@ bool migration_is_blocked(Error **errp);
>>  bool migration_in_postcopy(void);
>>  MigrationState *migrate_get_current(void);
>>  
>> -void migrate_compress_threads_create(void);
>> -void migrate_compress_threads_join(void);
>> -void migrate_decompress_threads_create(void);
>> -void migrate_decompress_threads_join(void);
>> -uint64_t ram_bytes_remaining(void);
>> -uint64_t ram_bytes_transferred(void);
>> -uint64_t ram_bytes_total(void);
>> -uint64_t ram_dirty_sync_count(void);
>> -uint64_t ram_dirty_pages_rate(void);
>> -uint64_t ram_postcopy_requests(void);
>> -void free_xbzrle_decoded_buf(void);
>> -
>> -void acct_update_position(QEMUFile *f, size_t size, bool zero);
>> -
>> -uint64_t dup_mig_pages_transferred(void);
>> -uint64_t norm_mig_pages_transferred(void);
>> -uint64_t xbzrle_mig_bytes_transferred(void);
>> -uint64_t xbzrle_mig_pages_transferred(void);
>> -uint64_t xbzrle_mig_pages_overflow(void);
>> -uint64_t xbzrle_mig_pages_cache_miss(void);
>> -double xbzrle_mig_cache_miss_rate(void);
>> -
>> -void ram_handle_compressed(void *host, uint8_t ch, uint64_t size);
>> -void ram_debug_dump_bitmap(unsigned long *todump, bool expected);
>> -/* For outgoing discard bitmap */
>> -int ram_postcopy_send_discard_bitmap(MigrationState *ms);
>> -/* For incoming postcopy discard */
>> -int ram_discard_range(const char *block_name, uint64_t start, size_t 
>> length);
>> -int ram_postcopy_incoming_init(MigrationIncomingState *mis);
>> -void ram_postcopy_migrated_memory_release(MigrationState *ms);
>>  
>>  bool migrate_release_ram(void);
>>  bool migrate_postcopy_ram(void);
>> @@ -233,8 +203,6 @@ int migrate_use_xbzrle(void);
>>  int64_t migrate_xbzrle_cache_size(void);
>>  bool migrate_colo_enabled(void);
>>  
>> -int64_t xbzrle_cache_resize(int64_t new_size);
>> -
>>  bool migrate_use_block_enabled(void);
>>  bool migrate_use_block_shared(void);
>>  
>> @@ -273,10 +241,6 @@ size_t ram_control_save_page(QEMUFile *f, ram_addr_t 
>> block_offset,
>>   ram_addr_t offset, size_t size,
>>   uint64_t *bytes_sent);
>>  
>> -void migration_page_queue_free(void);
>> -int ram_save_queue_pages(const char *rbname, ram_addr_t start, ram_addr_t 
>> len);
>> -uint64_t ram_pagesize_summary(void);
>> -
>>  PostcopyState postcopy_state_get(void);
>>  /* Set the state and return the old state */
>>  PostcopyState postcopy_state_set(PostcopyState new_state);
>> diff --git a/include/migration/ram.h b/include/migration/ram.h
>> new file mode 100644
>> index 000..c3653b3
>> --- /dev/null
>> +++ b/include/migration/ram.h
>> @@ -0,0 +1,54 @@
>> +/*
>> + * QEMU migration ram
>> + *
>> + * Copyright IBM, Corp. 2008
>> + *
>> + * Authors:
>> + *  Anthony Liguori   
>
> Shouldn't that be updated a bit?
>
> Other than that,

With what?

I can put myself there, but it is just copying a lot of function
prototypes, i.e. not as if I had done lot of work there right now.

But I am open to suggestions.

>
> Reviewed-by: Dr. David Alan Gilbert 
>
>> + * This work is licensed under the terms of the GNU GPL, version 2.  See
>> + * the COPYING file in the top-level directory.
>> + *
>> + */
>> +
>> +#ifndef QEMU_MIGRATION_RAM_H
>> +#define QEMU_MIGRATION_RAM_H
>> +
>> +#include "qemu-common.h"
>> +#include "exec/cpu-common.h"
>> +
>> +int64_t xbzrle_cache_resize(int64_t new_size);
>> +uint64_t dup_mig_pages_transferred(void);
>> +uint64_t norm_mig_pages_transferred(void);
>> +uint64_t xbzrle_mig_bytes_transferred(void);
>> +uint64_t xbzrle_mig_pages_transferred(void);
>> +uint64_t xbzrle_mig_pages_cache_miss(void);
>> +double xbzrle_mig_cache_miss_rate(void);
>> +uint64_t xbzrle_mig_pages_overflow(void);

Will try to take a look at that, but on the other hand, I am trying to
get the qmp code out of migration.c in its own file.  One can't have
everything.

Thanks, Juan.



Re: [Qemu-devel] [PATCH RFC] configure: fix clang failure for libatomic

2017-04-25 Thread Richard Henderson

On 04/25/2017 10:35 AM, Nikunj A Dadhania wrote:

if compile_prog "" "" ; then
  atomic128=yes
+  elif compile_prog "" "-latomic" ; then
+ atomic128=yes
+ lib_atomic="-latomic"
fi


This is a problem, because I think you'll find that gcc now advertises 
CONFIG_ATOMIC128 for *all* hosts.


This is because by definition, libatomic supplies fallback routines for types 
of arbitrary size.  However, the fallback routines use locking, not actual 
atomic operations.  So the configure test is no longer testing what we intended.



r~



[Qemu-devel] [PATCH] pc: add 2.10 machine type

2017-04-25 Thread Peter Xu
CC: "Michael S. Tsirkin" 
CC: Paolo Bonzini 
CC: Richard Henderson 
CC: Eduardo Habkost 
Signed-off-by: Peter Xu 
---
 hw/i386/pc_piix.c| 15 ---
 hw/i386/pc_q35.c | 13 +++--
 include/hw/i386/pc.h |  3 +++
 3 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 9f102aa..8fb6553 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -437,21 +437,30 @@ static void pc_i440fx_machine_options(MachineClass *m)
 m->default_display = "std";
 }
 
-static void pc_i440fx_2_9_machine_options(MachineClass *m)
+static void pc_i440fx_2_10_machine_options(MachineClass *m)
 {
 pc_i440fx_machine_options(m);
 m->alias = "pc";
 m->is_default = 1;
 }
 
+DEFINE_I440FX_MACHINE(v2_10, "pc-i440fx-2.10", NULL,
+  pc_i440fx_2_10_machine_options);
+
+static void pc_i440fx_2_9_machine_options(MachineClass *m)
+{
+pc_i440fx_2_10_machine_options(m);
+m->is_default = 0;
+m->alias = NULL;
+SET_MACHINE_COMPAT(m, PC_COMPAT_2_9);
+}
+
 DEFINE_I440FX_MACHINE(v2_9, "pc-i440fx-2.9", NULL,
   pc_i440fx_2_9_machine_options);
 
 static void pc_i440fx_2_8_machine_options(MachineClass *m)
 {
 pc_i440fx_2_9_machine_options(m);
-m->is_default = 0;
-m->alias = NULL;
 SET_MACHINE_COMPAT(m, PC_COMPAT_2_8);
 }
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index dd792a8..f07ebec 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -301,19 +301,28 @@ static void pc_q35_machine_options(MachineClass *m)
 m->max_cpus = 288;
 }
 
-static void pc_q35_2_9_machine_options(MachineClass *m)
+static void pc_q35_2_10_machine_options(MachineClass *m)
 {
 pc_q35_machine_options(m);
 m->alias = "q35";
 }
 
+DEFINE_Q35_MACHINE(v2_10, "pc-q35-2.10", NULL,
+   pc_q35_2_10_machine_options);
+
+static void pc_q35_2_9_machine_options(MachineClass *m)
+{
+pc_q35_2_10_machine_options(m);
+m->alias = NULL;
+SET_MACHINE_COMPAT(m, PC_COMPAT_2_9);
+}
+
 DEFINE_Q35_MACHINE(v2_9, "pc-q35-2.9", NULL,
pc_q35_2_9_machine_options);
 
 static void pc_q35_2_8_machine_options(MachineClass *m)
 {
 pc_q35_2_9_machine_options(m);
-m->alias = NULL;
 SET_MACHINE_COMPAT(m, PC_COMPAT_2_8);
 }
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index f278b3a..7546d01 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -373,6 +373,9 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
 int e820_get_num_entries(void);
 bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
 
+#define PC_COMPAT_2_9 \
+HW_COMPAT_2_9 \
+
 #define PC_COMPAT_2_8 \
 HW_COMPAT_2_8 \
 {\
-- 
2.7.4




Re: [Qemu-devel] DMG chunk size independence

2017-04-25 Thread Peter Wu
On Mon, Apr 24, 2017 at 05:19:48PM -0400, John Snow wrote:
> 
> 
> On 04/23/2017 05:03 AM, Ashijeet Acharya wrote:
> > Hi,
> > 
> > Great news!
> > I have almost completed this task and the results are looking
> > promising. I have not yet attended to the DMG files having bz2
> > compressed chunks but that should be easy and pretty similar to my
> > approach for zlib compressed files. So, no worries there.
> > 
> > For testing I am first converting the images to raw format and then
> > comparing the resulting image with the one converted using v2.9.0 DMG
> > driver and after battling for 2 days with my code, it finally prints
> > "Images are identical." According to John, that should be pretty
> > conclusive and I completely agree.
> > 
> 
> Yes, comparing a sample.dmg against a raw file generated from the 2.9.0
> qemu-img tool should be reasonably good evidence that you have not
> altered the behavior of the tool.
> 
> > Now, the real thing I wanted to ask was, if someone is aware of a DMG
> > file which has a chunk size above 64 MiB so that I can test those too.
> > If yes, please share the download link with me.
> > Currently I am testing the ones posted by Peter Wu while submitting
> > his DMG work in 2014.
> > Here -> 
> > https://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg03606.html
> > 
> 
> Are any of those over 64MB? I assume you're implying that they aren't.
> 
> Maybe Peter knows?...

I don't know DMG with bzip2-compressed chunks over 64M. Looking through
more recent files, there is this log for "Install macOS Sierra
10.12(16A323)-B.dmg" which contains only zlib-compressed or raw data
where the uncompressed size (in the MISH block) is always at most 1MiB:
https://github.com/Lekensteyn/dmg2img/issues/1#issuecomment-273662984

In an Xcode_7.2.dmg file, the situation was similar, only zlib or raw
and also with a max uncompressed size of 1MiB (actually, an exact size
of 1MiB in both cases based on "sectorCount").

Perhaps bzip2-compressed chunks are not so common for larger disk images
since zlib is faster.
-- 
Kind regards,
Peter Wu
https://lekensteyn.nl



Re: [Qemu-devel] [PATCH] target-s390x: Mask the SIGP order_code to 8bit.

2017-04-25 Thread Richard Henderson

On 04/24/2017 10:25 AM, Alexander Graf wrote:



On 24.04.17 00:32, Aurelien Jarno wrote:

From: Philipp Kern 

According to "CPU Signaling and Response", "Signal-Processor Orders",
the order field is bit position 56-63. Without this, the Linux
guest kernel is sometimes unable to stop emulation and enters
an infinite loop of "XXX unknown sigp: 0x0005".

Signed-off-by: Philipp Kern 
Signed-off-by: Aurelien Jarno 
---
 target/s390x/misc_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

This patch has been sent by Philipp Kern a lot of time ago, and it seems
has been lost. I am resending it, as it is still useful.

diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
index 3bf09ea222..4946b56ab3 100644
--- a/target/s390x/misc_helper.c
+++ b/target/s390x/misc_helper.c
@@ -534,7 +534,7 @@ uint32_t HELPER(sigp)(CPUS390XState *env, uint64_t 
order_code, uint32_t r1,

 /* Remember: Use "R1 or R1 + 1, whichever is the odd-numbered register"
as parameter (input). Status (output) is always R1. */

-switch (order_code) {
+switch (order_code & 0xff) {


This definitely needs a comment above the mask. Ideally I'd love to just change 
the function prototype to pass order_code as uint8_t, but I don't think that's 
possible with the TCG glue.


Correct.  We'll need to leave the mask here.


r~



Re: [Qemu-devel] [Qemu-stable] [PATCH v2 1/1] qemu-img: wait for convert coroutines to complete

2017-04-25 Thread Peter Lieven

Am 24.04.2017 um 22:13 schrieb Anton Nefedov:


On 24/04/2017 21:16, Peter Lieven wrote:




Am 24.04.2017 um 18:27 schrieb Anton Nefedov :


On 04/21/2017 03:37 PM, Peter Lieven wrote:

Am 21.04.2017 um 14:19 schrieb Anton Nefedov:

On 04/21/2017 01:44 PM, Peter Lieven wrote:

Am 21.04.2017 um 12:04 schrieb Anton Nefedov:
On error path (like i/o error in one of the coroutines), it's required to
 - wait for coroutines completion before cleaning the common structures
 - reenter dependent coroutines so they ever finish

Introduced in 2d9187bc65.

Signed-off-by: Anton Nefedov 
---
[..]




And what if we error out in the read path? Wouldn't be something like this 
easier?


diff --git a/qemu-img.c b/qemu-img.c
index 22f559a..4ff1085 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1903,6 +1903,16 @@ static int convert_do_copy(ImgConvertState *s)
main_loop_wait(false);
}

+/* on error path we need to enter all coroutines that are still
+ * running before cleaning up common structures */
+if (s->ret) {
+for (i = 0; i < s->num_coroutines; i++) {
+ if (s->co[i]) {
+ qemu_coroutine_enter(s->co[i]);
+ }
+}
+}
+
if (s->compressed && !s->ret) {
/* signal EOF to align */
ret = blk_pwrite_compressed(s->target, 0, NULL, 0);


Peter



seemed a bit too daring to me to re-enter every coroutine potentially including 
the ones that yielded waiting for I/O completion.
If that's ok - that is for sure easier :)


I think we should enter every coroutine that is still running and have it 
terminate correctly. It was my mistake that I have not
done this in the original patch. Can you check if the above fixes your test 
cases that triggered the bug?



hi, sorry I'm late with the answer

this segfaults in bdrv_close(). Looks like it tries to finish some i/o which 
coroutine we have already entered and terminated?

(gdb) run
Starting program: /vz/anefedov/qemu-build/us/./qemu-img convert -O qcow2 
./harddisk.hdd.c ./harddisk.hdd
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
[New Thread 0x7fffeac2d700 (LWP 436020)]
[New Thread 0x7fffe4ed6700 (LWP 436021)]
qemu-img: error while reading sector 20480: Input/output error
qemu-img: error while writing sector 19712: Operation now in progress

Program received signal SIGSEGV, Segmentation fault.
aio_co_wake (co=0x0) at /mnt/code/us-qemu/util/async.c:454
454 ctx = atomic_read(&co->ctx);
(gdb) bt
#0  aio_co_wake (co=0x0) at /mnt/code/us-qemu/util/async.c:454
/* [Anton]: thread_pool_co_cb () here */
#1  0x55634629 in thread_pool_completion_bh (opaque=0x55cfe020) at 
/mnt/code/us-qemu/util/thread-pool.c:189
#2  0x55633b31 in aio_bh_call (bh=0x55cfe0f0) at 
/mnt/code/us-qemu/util/async.c:90
#3  aio_bh_poll (ctx=ctx@entry=0x55cee6d0) at 
/mnt/code/us-qemu/util/async.c:118
#4  0x55636f14 in aio_poll (ctx=ctx@entry=0x55cee6d0, 
blocking=) at /mnt/code/us-qemu/util/aio-posix.c:682
#5  0x555c52d4 in bdrv_drain_recurse (bs=bs@entry=0x55d22560) at 
/mnt/code/us-qemu/block/io.c:164
#6  0x555c5aed in bdrv_drained_begin (bs=bs@entry=0x55d22560) at 
/mnt/code/us-qemu/block/io.c:248
#7  0x55581443 in bdrv_close (bs=0x55d22560) at 
/mnt/code/us-qemu/block.c:2909
#8  bdrv_delete (bs=0x55d22560) at /mnt/code/us-qemu/block.c:3100
#9  bdrv_unref (bs=0x55d22560) at /mnt/code/us-qemu/block.c:4087
#10 0x555baf44 in blk_remove_bs (blk=blk@entry=0x55d22380) at 
/mnt/code/us-qemu/block/block-backend.c:552
#11 0x555bb173 in blk_delete (blk=0x55d22380) at 
/mnt/code/us-qemu/block/block-backend.c:238
#12 blk_unref (blk=blk@entry=0x55d22380) at 
/mnt/code/us-qemu/block/block-backend.c:282
#13 0x5557a22c in img_convert (argc=, argv=) at /mnt/code/us-qemu/qemu-img.c:2359
#14 0x55574189 in main (argc=5, argv=0x7fffe4a0) at 
/mnt/code/us-qemu/qemu-img.c:4464



Peter



/Anton



it seems that this is a bit tricky, can you share how your test case works?

thanks,
peter



how I tested today was basically

diff --git a/qemu-img.c b/qemu-img.c
index 4425aaa..3d2d506 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1788,6 +1788,10 @@ static void coroutine_fn convert_co_do_copy(void *opaque)

 if (status == BLK_DATA) {
 ret = convert_co_read(s, sector_num, n, buf);
+const uint64_t fsector = 10*1024*1024/512;
+if (sector_num <= fsector && fsector < sector_num+n) {
+ret = -EIO;
+}
 if (ret < 0) {
 error_report("error while reading sector %" PRId64
  ": %s", sector_num, strerror(-ret));



I ended up with sth similar to your approach. Can you check this?


Thanks,

Peter


diff --git a/qemu-img.c b/qemu-img.c
index b94fc11..ed9ce9e 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1761,13 +1761,13 @@ static void corouti

Re: [Qemu-devel] [PATCH 1/3] colo-compare: serialize compare thread's initialization with main thread

2017-04-25 Thread Hailiang Zhang

On 2017/4/25 16:41, Jason Wang wrote:


On 2017年04月24日 14:03, Hailiang Zhang wrote:

On 2017/4/24 12:10, Jason Wang wrote:

On 2017年04月20日 15:46, zhanghailiang wrote:

We call qemu_chr_fe_set_handlers() in colo-compare thread, it is used
to detach watched fd from default main context, so it has chance to
handle the same watched fd with main thread concurrently, which will
trigger an error report:
"qemu-char.c:918: io_watch_poll_finalize: Assertion `iwp->src ==
((void *)0)' failed."

Anyway to prevent fd from being handled by main thread before creating
colo thread? Using semaphore seems not elegant.

So how about calling qemu_mutex_lock_iothread() before
qemu_chr_fe_set_handlers() ?

Looks better, but I needs more information e.g how main thread can touch it?


Hmm, this happened quite occasionally, and we didn't catch the first place 
(backtrace)
of removing fd from been watched, but  from the codes logic, we found there 
should
be such possible cases:
tcp_chr_write (Or tcp_chr_read/tcp_chr_sync_read/chr_disconnect)
 ->tcp_chr_disconnect (Or char_socket_finalize)
->tcp_chr_free_connection
  -> remove_fd_in_watch(chr);

Anyway, it needs the protection from been freed twice.

Thanks,
Hailiang

Thanks

.







Re: [Qemu-devel] [Qemu-devel RFC v2 4/4] msf2: Add Emcraft's Smartfusion2 SOM kit.

2017-04-25 Thread sundeep subbaraya
Hi Alistair,

On Mon, Apr 24, 2017 at 11:23 PM, Alistair Francis  wrote:
>>>
>>> Instead of calling all of these in the init function you should split
>>> it up over the machines init and realize function.
>>>
>>> Look at the stm32f205_soc or xlnx-zynqmp files for examples of how to do 
>>> this.
>>>
>>> It also moves away from calling qdev_create() and qdev_init_nofail()
>>> and instead manually creates the objects.
>>>
>> I am still learning all these. Please correct me if am wrong.
>> I need to create a SoC file and a board file like stm32f205 and
>> xlnx-zynqmp now right?
>
> Hey Sundeep,
>
> I don't think you have to do it like that. I think for some
> SoCs/boards it makes sense. For example the ZynqMP SoCs are included
> on multiple different boards (EP108 and ZCU102) so it makes sense to
> have a SoC and a board separately defined. On the other hand if you
> had a SoC that is always on the same board it doesn't make as much
> sense.
>
> It is probably is a good idea to split it between a board and an SoC
> unless you have a good reason not to though.

There are multiples boards with the SoC. I will split into seperate files.

Thanks,
Sundeep
>
> Thanks,
>
> Alistair
>
>>
>>> Otherwise this patch looks pretty good.
>>
>> Thank you :)
>> Sundeep
>>
>>>
>>> Thanks,
>>>
>>> Alistair
>>>
 +}
 +
 +static void msf2_machine_init(MachineClass *mc)
 +{
 +mc->desc = "SmartFusion2 SOM kit from Emcraft";
 +mc->init = msf2_init;
 +}
 +
 +DEFINE_MACHINE("smartfusion2-som", msf2_machine_init)
 --
 2.5.0





Re: [Qemu-devel] [Qemu-devel RFC v2 1/4] msf2: Add Smartfusion2 System timer

2017-04-25 Thread sundeep subbaraya
Hi Alistair and Peter,

On Mon, Apr 24, 2017 at 11:28 PM, Peter Maydell
 wrote:
> On 24 April 2017 at 18:44, Alistair Francis  wrote:
>> Basically the simple explanation is that init is called when the
>> object is created and realize is called when the object is realized.
>>
>> Generally for devices it will go something like this:
>>  1. init
>>  2. Set properties
>>  3. Connect things
>>  4. realize
>>  5. Map to memory
>>
>>> Don't we need to use realize function for new models?
>>
>> AFAIK we still put things like: sysbus_init_irq(),
>> memory_region_init_io() and sysbus_init_mmio() in the init function.
>>
>> I don't think we are at a stage yet to not use init functions.
>
> Two-phase init is here to stay -- some things must be done in
> init, some must be done in realize, and some can be done in
> either. Some simple devices may find they can do everything
> in only one function.
>
> Must be done in init:
>  * creating properties (for the cases where that is done "by hand"
>by calling object_property_add_*())
>  * calling init on child objects which you want to pass through
>alias properties for
>
> Must be done in realize:
>  * anything that can fail such that we need to report the
>error and abandon creation of the device
>  * anything which depends on the values of QOM properties that
>the caller might have set
>
> We should probably sit down and write up some guidelines for
> how we recommend dealing with the various things that could
> be called in either function -- this is basically a code
> style and consistency question.

Thanks for the brief. It makes sense for me now.
I will make changes and send the patches.

Thanks,
Sundeep

>
> thanks
> -- PMM



Re: [Qemu-devel] [PATCH RFC] target/openrisc: Support non-busy idle state using PMR SPR

2017-04-25 Thread Richard Henderson

On 04/23/2017 11:54 PM, Stafford Horne wrote:

The OpenRISC architecture has the Power Management Register (PMR)
special purpose register to manage cpu power states.  The interesting
modes are:

  * Doze Mode (DME) - Stop cpu except timer & pic - wake on interrupt
  * Sleep Mode (SME) - Stop cpu and all units - wake on interrupt
  * Suspend Model (SUME) - Stop cpu and all units - wake on reset

The linux kernel will set DME when idle.


And SUME would be, essentially, poweroff?  Perhaps at least for the purposes of 
QEMU; on real hardware one could press a button to assert reset and reboot.



Also, I don't know if its due to this patch of an issue with the timer
interrupts.  After applying this patch the timer interrupts do not trigger
until a keypress is make.  i.e. something like this...

   $ sleep 5
   

...

+cpu_restore_state(cs, GETPC() + 4);


This isn't correct.  You want

cpu_restore_state(cs, GETPC());
cs->env.pc += 4;

So what's happening is that you're re-executing the MTSPR and going back to 
sleep again.  Which probably explains the hang.



r~



Re: [Qemu-devel] [PATCH 4/6] migration: calculate downtime on dst side

2017-04-25 Thread Alexey Perevalov

On 04/25/2017 11:24 AM, Peter Xu wrote:

On Fri, Apr 14, 2017 at 04:17:18PM +0300, Alexey Perevalov wrote:

[...]


+/*
+ * This function calculates downtime per cpu and trace it
+ *
+ *  Also it calculates total downtime as an interval's overlap,
+ *  for many vCPU.
+ *
+ *  The approach is following:
+ *  Initially intervals are represented in tree where key is
+ *  pagefault address, and values:
+ *   begin - page fault time
+ *   end   - page load time
+ *   cpus  - bit mask shows affected cpus
+ *
+ *  To calculate overlap on all cpus, intervals converted into
+ *  array of points in time (downtime_points), the size of
+ *  array is 2 * number of nodes in tree of intervals (2 array
+ *  elements per one in element of interval).
+ *  Each element is marked as end (E) or as start (S) of interval.
+ *  The overlap downtime will be calculated for SE, only in case
+ *  there is sequence S(0..N)E(M) for every vCPU.
+ *
+ * As example we have 3 CPU
+ *
+ *  S1E1   S1   E1
+ * -***xxx***> CPU1
+ *
+ * S2E2
+ * xxx---> CPU2
+ *
+ * S3E3
+ * xxx---> CPU3
+ *
+ * We have sequence S1,S2,E1,S3,S1,E2,E3,E1
+ * S2,E1 - doesn't match condition due to sequence S1,S2,E1 doesn't include 
CPU3
+ * S3,S1,E2 - sequenece includes all CPUs, in this case overlap will be S1,E2
+ * Legend of picture is following: * - means downtime per vCPU
+ * x - means overlapped downtime
+ */

Not sure whether I get the point in this patch... iiuc we defined the
downtime here as the period when all vcpus are halted, right?

If so, I have a few questions:

- will this algorithm consume lots of memory? since I see we have one
   trace object per fault page address

I don't think, it consumes too much, one DowntimeDuration
takes (if I'm using bitmap_try_new, in this patch set I used pointer to 
uint64_t array to keep bitmap array,

but I'm going to use include/qemu/bitmap.h, it works with pointers to long)

(2* int64 + (ROUND_UP((smp_cpus + BITS_PER_BYTE * sizeof(long) - 1 / 
(BITS_PER_BYTE * sizeof(long * siezof(long)

so it's about 16 + at least 4 bytes, per page fault,
Lets assume we migration 256 vCPU and 256 Gb of ram and that ram is 
based on 4Kb pages - it's really bad case

16 + ((256 + 8 * 4 - 1) / ( 8 * 4 )) * 4 = 52 bytes
(256 * 1024 * 1024 * 1024)/(4 * 1024) = 67108864 page faults, but not 
all of these pages will be pagefaulted, due to

page pre-fetching
67108864 * 52 = 3489660928 bytes (3.5 Gb for that operation),
but I have a doubt, who will use 4Kb pages for 256 Gb, probably
2Mb or 1G huge page will be chosen on x86, on ARM or other architecture 
it could be another values.




- do we need to protect the tree to make sure there's no insertion
   when doing the calculation?

I asked the same question when sent RFC patches,
the answer here is no, we should not, due to right now,
it's only one socket and one listen thread (maybe in future,
it will be required, maybe after multi fd patch set),
and calculation is doing synchronously right after migration complete.



- if the only thing we want here is the "total downtime", whether
   below would work? (assuming N is vcpu numbers)

   a. define array cpu_fault_addr[N], to store current faulted address
  for each vcpu. When vcpu X is running, cpu_fault_addr[X] should
  be 0.

   b. when page fault happens on vcpu A, setup cpu_fault_addr[A] with
  corresponding fault address.

at this time need to is fault happens for all another vCPU,
and if it happens mark current time as total vCPU downtime start.


   c. when page copy finished, loop over cpu_fault_addr[] to see
  whether that matches any, clear corresponding element if matched.

so when page copy finished and mark for total vCPU is set,
yes that interval is a part of total downtime.


   Then, we can just measure the period when cpu_fault_addr[] is all
   set (by tracing at both b. and c.). Can this work?

Yes, it works, but it's better to keep time - cpu_fault_time,
address is not important here, it doesn't matter the reason of pagefault.
2 vCPU could fault due to access to one page, ok, it's not a problem, 
just store

time when it was faulted.
Looks like it's better algorithm, with lesser complexity,
thank you a lot.




Thanks,




--
Best regards,
Alexey Perevalov



[Qemu-devel] [PATCH v4] Split migration bitmaps by ramblock

2017-04-25 Thread Juan Quintela
Hi

Make postcopy_chunk_hostpages work at ramblock level, so we don't do
the double look over ramblocks.  Tested that postcopy still works as
expected.

Later, Juan.


[v3]
I messed up previous submission and sent the wrong patch.

- This verios is a rebase on top of the ramstate series
- Fixed the problem with postcopy

Important bit is this one:

-pds->start_list[pds->cur_entry] = (start - pds->offset) * tp_size;
+pds->start_list[pds->cur_entry] = start  * tp_size;

This chunk is only used in postcopy, that is why it didn't failed
precopy tests.  Once ther, just remove the start parameter and offset
member because it is not used anywhere else.

Please comment.

Later, Juan.


[v2]

For v2 this is a rebase on top of all the changes that happened in the
prvious two series.

Please, review.

[RFC]
This series split the migration and unsent bitmaps by ramblock.  This
makes it easier to synchronize in small bits.  This is on top of the
RAMState and not-hotplug series.

Why?

reason 1:

People have complained that by the time that we detect that a page is
sent, it has already been marked dirty "again" inside kvm, so we are
going to send it again.  On top of this patch, my idea is, for words
of the bitmap that have any bit set, just synchonize the bitmap before
sending the pages.  I have not looking into performance numbers yet,
jsut asking for comments about how it is done.

reason 2:

In case where the host page is a multiple of the the TARGET_PAGE_SIZE,
we do a lot of work when we are synchronizing the bitmaps to pass it
to target page size.  The idea is to change the bitmaps on that
RAMBlocks to mean host page size and not TARGET_PAGE_SIZE.

Note that there are two reason for this, ARM and PPC do things like
guests with 4kb pages on hosts with 16/64kb hosts, and then we have
HugePages.  Note all the workarounds that postcopy has to do because
to work in HugePages size.

Please, comment?

Later, Juan.

Juan Quintela (1):
  ram: Split dirty bitmap by RAMBlock

 include/exec/ram_addr.h  |  13 +-
 include/migration/postcopy-ram.h |   3 -
 migration/postcopy-ram.c |   5 +-
 migration/ram.c  | 257 +++
 4 files changed, 109 insertions(+), 169 deletions(-)

-- 
2.9.3




[Qemu-devel] [PATCH] ram: Split dirty bitmap by RAMBlock

2017-04-25 Thread Juan Quintela
Both the ram bitmap and the unsent bitmap are split by RAMBlock.

Signed-off-by: Juan Quintela 
---
 include/exec/ram_addr.h  |  13 +-
 include/migration/postcopy-ram.h |   3 -
 migration/postcopy-ram.c |   5 +-
 migration/ram.c  | 257 +++
 4 files changed, 109 insertions(+), 169 deletions(-)

diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index 6436a41..c56b35b 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -39,6 +39,14 @@ struct RAMBlock {
 QLIST_HEAD(, RAMBlockNotifier) ramblock_notifiers;
 int fd;
 size_t page_size;
+/* dirty bitmap used during migration */
+unsigned long *bmap;
+/* bitmap of pages that haven't been sent even once
+ * only maintained and used in postcopy at the moment
+ * where it's used to send the dirtymap at the start
+ * of the postcopy phase
+ */
+unsigned long *unsentmap;
 };
 
 static inline bool offset_in_ramblock(RAMBlock *b, ram_addr_t offset)
@@ -360,16 +368,15 @@ static inline void 
cpu_physical_memory_clear_dirty_range(ram_addr_t start,
 
 
 static inline
-uint64_t cpu_physical_memory_sync_dirty_bitmap(unsigned long *dest,
-   RAMBlock *rb,
+uint64_t cpu_physical_memory_sync_dirty_bitmap(RAMBlock *rb,
ram_addr_t start,
ram_addr_t length,
uint64_t *real_dirty_pages)
 {
 ram_addr_t addr;
-start = rb->offset + start;
 unsigned long page = BIT_WORD(start >> TARGET_PAGE_BITS);
 uint64_t num_dirty = 0;
+unsigned long *dest = rb->bmap;
 
 /* start address is aligned at the start of a word? */
 if (((page * BITS_PER_LONG) << TARGET_PAGE_BITS) == start) {
diff --git a/include/migration/postcopy-ram.h b/include/migration/postcopy-ram.h
index 8e036b9..4c25f03 100644
--- a/include/migration/postcopy-ram.h
+++ b/include/migration/postcopy-ram.h
@@ -43,12 +43,9 @@ int postcopy_ram_prepare_discard(MigrationIncomingState 
*mis);
 
 /*
  * Called at the start of each RAMBlock by the bitmap code.
- * 'offset' is the bitmap offset of the named RAMBlock in the migration
- * bitmap.
  * Returns a new PDS
  */
 PostcopyDiscardState *postcopy_discard_send_init(MigrationState *ms,
- unsigned long offset,
  const char *name);
 
 /*
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index 85fd8d7..e3f4a37 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -33,7 +33,6 @@
 
 struct PostcopyDiscardState {
 const char *ramblock_name;
-uint64_t offset; /* Bitmap entry for the 1st bit of this RAMBlock */
 uint16_t cur_entry;
 /*
  * Start and length of a discard range (bytes)
@@ -717,14 +716,12 @@ void *postcopy_get_tmp_page(MigrationIncomingState *mis)
  * returns: a new PDS.
  */
 PostcopyDiscardState *postcopy_discard_send_init(MigrationState *ms,
- unsigned long offset,
  const char *name)
 {
 PostcopyDiscardState *res = g_malloc0(sizeof(PostcopyDiscardState));
 
 if (res) {
 res->ramblock_name = name;
-res->offset = offset;
 }
 
 return res;
@@ -745,7 +742,7 @@ void postcopy_discard_send_range(MigrationState *ms, 
PostcopyDiscardState *pds,
 {
 size_t tp_size = qemu_target_page_size();
 /* Convert to byte offsets within the RAM block */
-pds->start_list[pds->cur_entry] = (start - pds->offset) * tp_size;
+pds->start_list[pds->cur_entry] = start  * tp_size;
 pds->length_list[pds->cur_entry] = length * tp_size;
 trace_postcopy_discard_send_range(pds->ramblock_name, start, length);
 pds->cur_entry++;
diff --git a/migration/ram.c b/migration/ram.c
index f48664e..d99f6e2 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -138,19 +138,6 @@ out:
 return ret;
 }
 
-struct RAMBitmap {
-struct rcu_head rcu;
-/* Main migration bitmap */
-unsigned long *bmap;
-/* bitmap of pages that haven't been sent even once
- * only maintained and used in postcopy at the moment
- * where it's used to send the dirtymap at the start
- * of the postcopy phase
- */
-unsigned long *unsentmap;
-};
-typedef struct RAMBitmap RAMBitmap;
-
 /*
  * An outstanding page request, on the source, having been received
  * and queued
@@ -220,8 +207,6 @@ struct RAMState {
 uint64_t postcopy_requests;
 /* protects modification of the bitmap */
 QemuMutex bitmap_mutex;
-/* Ram Bitmap protected by RCU */
-RAMBitmap *ram_bitmap;
 /* The RAMBlock used in the last src_page_requests */
 RAMBlock *last_req_rb;
 /* Queue of outstanding page requests from the destination */
@@ -614,22 +599,17 @@ static inline
 unsigned

Re: [Qemu-devel] [PULL v2 00/12] Block patches

2017-04-25 Thread Peter Maydell
On 24 April 2017 at 20:19, Jeff Cody  wrote:
> The following changes since commit 4c55b1d0bad8a703f0499fe62e3761a0cd288da3:
>
>   Merge remote-tracking branch 'remotes/armbru/tags/pull-error-2017-04-24' 
> into staging (2017-04-24 14:49:48 +0100)
>
> are available in the git repository at:
>
>   git://github.com/codyprime/qemu-kvm-jtc.git tags/block-pull-request
>
> for you to fetch changes up to ecfa185400ade2abc9915efa924cbad1e15a21a4:
>
>   qemu-iotests: _cleanup_qemu must be called on exit (2017-04-24 15:09:33 
> -0400)
>
> 
> Pull v2, with 32-bit errors fixed.  I don't have OS X to test compile on,
> but I think it is safe to assume the cause of the compile error was the same.
> 
>
> Ashish Mittal (2):
>   block/vxhs.c: Add support for a new block device type called "vxhs"
>   block/vxhs.c: Add qemu-iotests for new block device type "vxhs"
>
> Jeff Cody (10):
>   qemu-iotests: exclude vxhs from image creation via protocol
>   block: add bdrv_set_read_only() helper function
>   block: do not set BDS read_only if copy_on_read enabled
>   block: honor BDRV_O_ALLOW_RDWR when clearing bs->read_only
>   block: code movement
>   block: introduce bdrv_can_set_read_only()
>   block: use bdrv_can_set_read_only() during reopen
>   block/rbd - update variable names to more apt names
>   block/rbd: Add support for reopen()
>   qemu-iotests: _cleanup_qemu must be called on exit
>

Applied, thanks.

-- PMM



[Qemu-devel] [PATCH 0/2] Misc migration fixes

2017-04-25 Thread Juan Quintela
Hi

This are independent of all my series, so send then here.

- check_migratable needs to now about objects, device class, etc, that
  migration code don't care.  So move it to qdev.c.

- to_dst_file is set to NULL before this call, just remove it.  Long
  term idea is that nothing outside migration.c should now about
  members of MigrationState.

Please, review.

Juan Quintela (2):
  migration: Move check_migratable() into qdev.c
  migration: to_dst_file at that point is NULL

 hw/core/qdev.c| 15 ++-
 include/migration/migration.h |  3 ---
 include/migration/vmstate.h   |  2 ++
 migration/migration.c | 15 ---
 migration/savevm.c| 10 ++
 migration/socket.c|  1 -
 migration/tls.c   |  1 -
 stubs/vmstate.c   |  5 ++---
 8 files changed, 28 insertions(+), 24 deletions(-)

-- 
2.9.3




[Qemu-devel] [PATCH 2/2] migration: to_dst_file at that point is NULL

2017-04-25 Thread Juan Quintela
We have just arrived as:

migration.c: qemu_migrate()
  
  s = migrate_init() <- puts it to NULL
  
  {tcp,unix}_start_outgoing_migration ->
 socket_outgoing_migration
migration_channel_connect()
   sets to_dst_file

if tls is enabled, we do another round through
migrate_channel_tls_connect(), but we only set it up if there is no
error.  So we don't need the assignation.  I am removing it to remove
in the follwing patches the knowledge about MigrationState in that two
files.

Signed-off-by: Juan Quintela 
---
 migration/socket.c | 1 -
 migration/tls.c| 1 -
 2 files changed, 2 deletions(-)

diff --git a/migration/socket.c b/migration/socket.c
index 13966f1..dc88812 100644
--- a/migration/socket.c
+++ b/migration/socket.c
@@ -79,7 +79,6 @@ static void socket_outgoing_migration(QIOTask *task,
 
 if (qio_task_propagate_error(task, &err)) {
 trace_migration_socket_outgoing_error(error_get_pretty(err));
-data->s->to_dst_file = NULL;
 migrate_fd_error(data->s, err);
 error_free(err);
 } else {
diff --git a/migration/tls.c b/migration/tls.c
index 45bec44..a33ecb7 100644
--- a/migration/tls.c
+++ b/migration/tls.c
@@ -116,7 +116,6 @@ static void migration_tls_outgoing_handshake(QIOTask *task,
 
 if (qio_task_propagate_error(task, &err)) {
 trace_migration_tls_outgoing_handshake_error(error_get_pretty(err));
-s->to_dst_file = NULL;
 migrate_fd_error(s, err);
 error_free(err);
 } else {
-- 
2.9.3




[Qemu-devel] [PATCH 1/2] migration: Move check_migratable() into qdev.c

2017-04-25 Thread Juan Quintela
The function is only used once, and nothing else in migration knows
about objects.  Create the function vmstate_device_is_migratable() in
savem.c that really do the bit that is related with migration.

Signed-off-by: Juan Quintela 
---
 hw/core/qdev.c| 15 ++-
 include/migration/migration.h |  3 ---
 include/migration/vmstate.h   |  2 ++
 migration/migration.c | 15 ---
 migration/savevm.c| 10 ++
 stubs/vmstate.c   |  5 ++---
 6 files changed, 28 insertions(+), 22 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 02b632f..17ff638 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -37,7 +37,7 @@
 #include "hw/boards.h"
 #include "hw/sysbus.h"
 #include "qapi-event.h"
-#include "migration/migration.h"
+#include "migration/vmstate.h"
 
 bool qdev_hotplug = false;
 static bool qdev_hot_added = false;
@@ -861,6 +861,19 @@ static bool device_get_realized(Object *obj, Error **errp)
 return dev->realized;
 }
 
+static int check_migratable(Object *obj, Error **err)
+{
+DeviceClass *dc = DEVICE_GET_CLASS(obj);
+if (!vmstate_device_is_migratable(dc->vmsd)) {
+error_setg(err, "Device %s is not migratable, but "
+   "--only-migratable was specified",
+   object_get_typename(obj));
+return -1;
+}
+
+return 0;
+}
+
 static void device_set_realized(Object *obj, bool value, Error **errp)
 {
 DeviceState *dev = DEVICE(obj);
diff --git a/include/migration/migration.h b/include/migration/migration.h
index ba1a16c..dfeca38 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -22,7 +22,6 @@
 #include "qapi-types.h"
 #include "exec/cpu-common.h"
 #include "qemu/coroutine_int.h"
-#include "qom/object.h"
 
 #define QEMU_VM_FILE_MAGIC   0x5145564d
 #define QEMU_VM_FILE_VERSION_COMPAT  0x0002
@@ -292,8 +291,6 @@ int migrate_add_blocker(Error *reason, Error **errp);
  */
 void migrate_del_blocker(Error *reason);
 
-int check_migratable(Object *obj, Error **err);
-
 bool migrate_release_ram(void);
 bool migrate_postcopy_ram(void);
 bool migrate_zero_blocks(void);
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index dad3984..9452dec 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -1049,4 +1049,6 @@ int64_t self_announce_delay(int round)
 
 void dump_vmstate_json_to_file(FILE *out_fp);
 
+bool vmstate_device_is_migratable(const VMStateDescription *vmsd);
+
 #endif
diff --git a/migration/migration.c b/migration/migration.c
index 353f272..5447cab 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1158,21 +1158,6 @@ void migrate_del_blocker(Error *reason)
 migration_blockers = g_slist_remove(migration_blockers, reason);
 }
 
-int check_migratable(Object *obj, Error **err)
-{
-DeviceClass *dc = DEVICE_GET_CLASS(obj);
-if (only_migratable && dc->vmsd) {
-if (dc->vmsd->unmigratable) {
-error_setg(err, "Device %s is not migratable, but "
-   "--only-migratable was specified",
-   object_get_typename(obj));
-return -1;
-}
-}
-
-return 0;
-}
-
 void qmp_migrate_incoming(const char *uri, Error **errp)
 {
 Error *local_err = NULL;
diff --git a/migration/savevm.c b/migration/savevm.c
index 03ae1bd..7421a67 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2480,3 +2480,13 @@ void vmstate_register_ram_global(MemoryRegion *mr)
 {
 vmstate_register_ram(mr, NULL);
 }
+
+bool vmstate_device_is_migratable(const VMStateDescription *vmsd)
+{
+if (only_migratable && vmsd) {
+if (vmsd->unmigratable) {
+return false;
+}
+}
+return true;
+}
diff --git a/stubs/vmstate.c b/stubs/vmstate.c
index 6d52f29..5af824b 100644
--- a/stubs/vmstate.c
+++ b/stubs/vmstate.c
@@ -1,7 +1,6 @@
 #include "qemu/osdep.h"
 #include "qemu-common.h"
 #include "migration/vmstate.h"
-#include "migration/migration.h"
 
 const VMStateDescription vmstate_dummy = {};
 
@@ -21,7 +20,7 @@ void vmstate_unregister(DeviceState *dev,
 {
 }
 
-int check_migratable(Object *obj, Error **err)
+bool vmstate_device_is_migratable(const VMStateDescription *vmsd)
 {
-return 0;
+return true;
 }
-- 
2.9.3




Re: [Qemu-devel] [PATCH v2 00/25] qmp: add async command type

2017-04-25 Thread Kevin Wolf
Am 24.04.2017 um 21:10 hat Markus Armbruster geschrieben:
> With 2.9 out of the way, how can we make progress on this one?
> 
> I can see two ways to get asynchronous QMP commands accepted:
> 
> 1. We break QMP compatibility in QEMU 3.0 and convert all long-running
>tasks from "synchronous command + event" to "asynchronous command".
> 
>This is design option 1 quoted below.  *If* we decide to leave
>compatibility behind for 3.0, *and* we decide we like the
>asynchronous sufficiently better to put in the work, we can do it.
> 
>I guess there's nothing to do here until we decide on breaking
>compatibility in 3.0.
> 
> 2. We don't break QMP compatibility, but we add asynchronous commands
>anyway, because we decide that's how we want to do "jobs".
> 
>This is design option 3 quoted below.  As I said, I dislike its lack
>of orthogonality.  But if asynchronous commands help us get jobs
>done, I can bury my dislike.

I don't think async commands are attractive at all for doing jobs. I
feel they bring up more questions that they answer, for example, what
happens if libvirt crashes and then reconnects? Which monitor connection
does get the reply for an async command sent on the now disconnected
one?

We already have a model for doing long-running jobs, and as far as I'm
aware, it's working and we're not fighting limitations of the design. So
what are we even trying to solve here? In the context of jobs, async
commands feel like a solution in need of a problem to me.

Things may look a bit different in typically quick, but potentially
long-running commands. That is, anything that we currently execute
synchronously while holding the BQL, but that involves I/O and could
therefore take a while (impacting the performance of the VM) or even
block indefinitely.

The first problem (we're holding the lock too long) can be addressed by
making things async just inside qemu and we don't need to expose the
change on the QMP level. The second one (blocking indefinitely) requires
being async on the QMP level if we want the monitor to be responsive
even if we're using an image on an NFS server that went down.

On the other hand, using the traditional job infrastructure is way over
the top if all you want to do is 'query-block', so we need something
different for making it async. And if a client disconnects, the
'query-block' result can just be thrown away, it's much simpler than
actual jobs.

So where I can see advantages for a new async command type is not for
converting real long-running commands like block jobs, but only for the
typically, but not necessarily quick operations. At the same time it is
where you're rightfully afraid that the less common case might not
receive much testing in management tools.

In the end, I'm unsure whether async commands are a good idea, I can see
good arguments for both stances. But I'm almost certain that they are
the wrong tool for jobs.

Kevin



[Qemu-devel] [PATCH 3/6] monitor: Move hmp_savevm from savevm.c to hmp.c

2017-04-25 Thread Juan Quintela
It is a monitor command, and has nothing migration specific in it.

Signed-off-by: Juan Quintela 
---
 hmp.c   | 5 +
 hmp.h   | 1 +
 include/sysemu/sysemu.h | 1 -
 migration/savevm.c  | 5 -
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/hmp.c b/hmp.c
index f6b8738..a82a952 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1281,6 +1281,11 @@ void hmp_loadvm(Monitor *mon, const QDict *qdict)
 }
 }
 
+void hmp_savevm(Monitor *mon, const QDict *qdict)
+{
+save_vmstate(qdict_get_try_str(qdict, "name"));
+}
+
 void hmp_migrate_cancel(Monitor *mon, const QDict *qdict)
 {
 qmp_migrate_cancel(NULL);
diff --git a/hmp.h b/hmp.h
index 385332c..b302c8d 100644
--- a/hmp.h
+++ b/hmp.h
@@ -64,6 +64,7 @@ void hmp_snapshot_delete_blkdev_internal(Monitor *mon, const 
QDict *qdict);
 void hmp_drive_mirror(Monitor *mon, const QDict *qdict);
 void hmp_drive_backup(Monitor *mon, const QDict *qdict);
 void hmp_loadvm(Monitor *mon, const QDict *qdict);
+void hmp_savevm(Monitor *mon, const QDict *qdict);
 void hmp_migrate_cancel(Monitor *mon, const QDict *qdict);
 void hmp_migrate_incoming(Monitor *mon, const QDict *qdict);
 void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict);
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index b6daf9d..914c36c 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -75,7 +75,6 @@ void qemu_remove_exit_notifier(Notifier *notify);
 void qemu_add_machine_init_done_notifier(Notifier *notify);
 void qemu_remove_machine_init_done_notifier(Notifier *notify);
 
-void hmp_savevm(Monitor *mon, const QDict *qdict);
 int save_vmstate(const char *name);
 int load_vmstate(const char *name);
 void hmp_delvm(Monitor *mon, const QDict *qdict);
diff --git a/migration/savevm.c b/migration/savevm.c
index ff934aa..bbff4d8 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2171,11 +2171,6 @@ int save_vmstate(const char *name)
 return ret;
 }
 
-void hmp_savevm(Monitor *mon, const QDict *qdict)
-{
-save_vmstate(qdict_get_try_str(qdict, "name"));
-}
-
 void qmp_xen_save_devices_state(const char *filename, Error **errp)
 {
 QEMUFile *f;
-- 
2.9.3




[Qemu-devel] [PATCH 0/6] Move snapshots commands to hmp

2017-04-25 Thread Juan Quintela
Hi

This series:
- Move snapshots commands to hmp.c, as they don't have code for migration
- Make them work with errors in a modern way instead of writting to the monitor
- make paolo happy and use hmp_handle_error

Later, Juan.

Juan Quintela (6):
  monitor: Remove monitor parameter from save_vmstate
  monitor: Move hmp_loadvm from monitor.c to hmp.c
  monitor: Move hmp_savevm from savevm.c to hmp.c
  monitor: Move hmp_delvm from savevm.c to hmp.c
  monitor: Move hmp_info_snapshots from savevm.c to hmp.c
  migration: Pass Error ** argument to {save,load}_vmstate

 hmp.c| 179 +++
 hmp.h|   4 +
 include/sysemu/sysemu.h  |   7 +-
 migration/savevm.c   | 216 ++-
 monitor.c|  13 ---
 replay/replay-snapshot.c |   6 +-
 vl.c |   4 +-
 7 files changed, 217 insertions(+), 212 deletions(-)

-- 
2.9.3




[Qemu-devel] [PATCH 1/6] monitor: Remove monitor parameter from save_vmstate

2017-04-25 Thread Juan Quintela
load_vmstate() already use error_report, so be consistent.  There is
an identical error message in load_vmstate() that ends in a
period. Remove it.

Signed-off-by: Juan Quintela 
---
 include/sysemu/sysemu.h  |  2 +-
 migration/savevm.c   | 20 ++--
 replay/replay-snapshot.c |  2 +-
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 16175f7..b6daf9d 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -76,7 +76,7 @@ void qemu_add_machine_init_done_notifier(Notifier *notify);
 void qemu_remove_machine_init_done_notifier(Notifier *notify);
 
 void hmp_savevm(Monitor *mon, const QDict *qdict);
-int save_vmstate(Monitor *mon, const char *name);
+int save_vmstate(const char *name);
 int load_vmstate(const char *name);
 void hmp_delvm(Monitor *mon, const QDict *qdict);
 void hmp_info_snapshots(Monitor *mon, const QDict *qdict);
diff --git a/migration/savevm.c b/migration/savevm.c
index 7421a67..ff934aa 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2070,7 +2070,7 @@ int qemu_loadvm_state(QEMUFile *f)
 return ret;
 }
 
-int save_vmstate(Monitor *mon, const char *name)
+int save_vmstate(const char *name)
 {
 BlockDriverState *bs, *bs1;
 QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
@@ -2084,8 +2084,8 @@ int save_vmstate(Monitor *mon, const char *name)
 AioContext *aio_context;
 
 if (!bdrv_all_can_snapshot(&bs)) {
-monitor_printf(mon, "Device '%s' is writable but does not "
-   "support snapshots.\n", bdrv_get_device_name(bs));
+error_report("Device '%s' is writable but does not support snapshots",
+ bdrv_get_device_name(bs));
 return ret;
 }
 
@@ -2102,7 +2102,7 @@ int save_vmstate(Monitor *mon, const char *name)
 
 bs = bdrv_all_find_vmstate_bs();
 if (bs == NULL) {
-monitor_printf(mon, "No block device can accept snapshots\n");
+error_report("No block device can accept snapshots");
 return ret;
 }
 aio_context = bdrv_get_aio_context(bs);
@@ -2111,7 +2111,7 @@ int save_vmstate(Monitor *mon, const char *name)
 
 ret = global_state_store();
 if (ret) {
-monitor_printf(mon, "Error saving global state\n");
+error_report("Error saving global state");
 return ret;
 }
 vm_stop(RUN_STATE_SAVE_VM);
@@ -2143,7 +2143,7 @@ int save_vmstate(Monitor *mon, const char *name)
 /* save the VM state */
 f = qemu_fopen_bdrv(bs, 1);
 if (!f) {
-monitor_printf(mon, "Could not open VM state file\n");
+error_report("Could not open VM state file");
 goto the_end;
 }
 ret = qemu_savevm_state(f, &local_err);
@@ -2156,8 +2156,8 @@ int save_vmstate(Monitor *mon, const char *name)
 
 ret = bdrv_all_create_snapshot(sn, bs, vm_state_size, &bs);
 if (ret < 0) {
-monitor_printf(mon, "Error while creating snapshot on '%s'\n",
-   bdrv_get_device_name(bs));
+error_report("Error while creating snapshot on '%s'",
+ bdrv_get_device_name(bs));
 goto the_end;
 }
 
@@ -2173,7 +2173,7 @@ int save_vmstate(Monitor *mon, const char *name)
 
 void hmp_savevm(Monitor *mon, const QDict *qdict)
 {
-save_vmstate(mon, qdict_get_try_str(qdict, "name"));
+save_vmstate(qdict_get_try_str(qdict, "name"));
 }
 
 void qmp_xen_save_devices_state(const char *filename, Error **errp)
@@ -2245,7 +2245,7 @@ int load_vmstate(const char *name)
 MigrationIncomingState *mis = migration_incoming_get_current();
 
 if (!bdrv_all_can_snapshot(&bs)) {
-error_report("Device '%s' is writable but does not support snapshots.",
+error_report("Device '%s' is writable but does not support snapshots",
  bdrv_get_device_name(bs));
 return -ENOTSUP;
 }
diff --git a/replay/replay-snapshot.c b/replay/replay-snapshot.c
index 65e2d37..8cced46 100644
--- a/replay/replay-snapshot.c
+++ b/replay/replay-snapshot.c
@@ -64,7 +64,7 @@ void replay_vmstate_init(void)
 {
 if (replay_snapshot) {
 if (replay_mode == REPLAY_MODE_RECORD) {
-if (save_vmstate(cur_mon, replay_snapshot) != 0) {
+if (save_vmstate(replay_snapshot) != 0) {
 error_report("Could not create snapshot for icount record");
 exit(1);
 }
-- 
2.9.3




[Qemu-devel] [PATCH 2/6] monitor: Move hmp_loadvm from monitor.c to hmp.c

2017-04-25 Thread Juan Quintela
We are going to move the rest of hmp snapshots functions there instead
of monitor.c.

Signed-off-by: Juan Quintela 
---
 hmp.c | 13 +
 hmp.h |  1 +
 monitor.c | 13 -
 3 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/hmp.c b/hmp.c
index ab407d6..f6b8738 100644
--- a/hmp.c
+++ b/hmp.c
@@ -19,6 +19,7 @@
 #include "net/eth.h"
 #include "sysemu/char.h"
 #include "sysemu/block-backend.h"
+#include "sysemu/sysemu.h"
 #include "qemu/config-file.h"
 #include "qemu/option.h"
 #include "qemu/timer.h"
@@ -1268,6 +1269,18 @@ void hmp_snapshot_delete_blkdev_internal(Monitor *mon, 
const QDict *qdict)
 hmp_handle_error(mon, &err);
 }
 
+void hmp_loadvm(Monitor *mon, const QDict *qdict)
+{
+int saved_vm_running  = runstate_is_running();
+const char *name = qdict_get_str(qdict, "name");
+
+vm_stop(RUN_STATE_RESTORE_VM);
+
+if (load_vmstate(name) == 0 && saved_vm_running) {
+vm_start();
+}
+}
+
 void hmp_migrate_cancel(Monitor *mon, const QDict *qdict)
 {
 qmp_migrate_cancel(NULL);
diff --git a/hmp.h b/hmp.h
index 799fd37..385332c 100644
--- a/hmp.h
+++ b/hmp.h
@@ -63,6 +63,7 @@ void hmp_snapshot_blkdev_internal(Monitor *mon, const QDict 
*qdict);
 void hmp_snapshot_delete_blkdev_internal(Monitor *mon, const QDict *qdict);
 void hmp_drive_mirror(Monitor *mon, const QDict *qdict);
 void hmp_drive_backup(Monitor *mon, const QDict *qdict);
+void hmp_loadvm(Monitor *mon, const QDict *qdict);
 void hmp_migrate_cancel(Monitor *mon, const QDict *qdict);
 void hmp_migrate_incoming(Monitor *mon, const QDict *qdict);
 void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict);
diff --git a/monitor.c b/monitor.c
index be282ec..d02900d 100644
--- a/monitor.c
+++ b/monitor.c
@@ -37,7 +37,6 @@
 #include "net/slirp.h"
 #include "sysemu/char.h"
 #include "ui/qemu-spice.h"
-#include "sysemu/sysemu.h"
 #include "sysemu/numa.h"
 #include "monitor/monitor.h"
 #include "qemu/config-file.h"
@@ -1843,18 +1842,6 @@ void qmp_closefd(const char *fdname, Error **errp)
 error_setg(errp, QERR_FD_NOT_FOUND, fdname);
 }
 
-static void hmp_loadvm(Monitor *mon, const QDict *qdict)
-{
-int saved_vm_running  = runstate_is_running();
-const char *name = qdict_get_str(qdict, "name");
-
-vm_stop(RUN_STATE_RESTORE_VM);
-
-if (load_vmstate(name) == 0 && saved_vm_running) {
-vm_start();
-}
-}
-
 int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
 {
 mon_fd_t *monfd;
-- 
2.9.3




[Qemu-devel] [PATCH 4/6] monitor: Move hmp_delvm from savevm.c to hmp.c

2017-04-25 Thread Juan Quintela
It really uses block/* stuff, not migration one.

Signed-off-by: Juan Quintela 
---
 hmp.c   | 13 +
 hmp.h   |  1 +
 include/sysemu/sysemu.h |  1 -
 migration/savevm.c  | 13 -
 4 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/hmp.c b/hmp.c
index a82a952..bb739ce 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1286,6 +1286,19 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
 save_vmstate(qdict_get_try_str(qdict, "name"));
 }
 
+void hmp_delvm(Monitor *mon, const QDict *qdict)
+{
+BlockDriverState *bs;
+Error *err;
+const char *name = qdict_get_str(qdict, "name");
+
+if (bdrv_all_delete_snapshot(name, &bs, &err) < 0) {
+error_reportf_err(err,
+  "Error while deleting snapshot on device '%s': ",
+  bdrv_get_device_name(bs));
+}
+}
+
 void hmp_migrate_cancel(Monitor *mon, const QDict *qdict)
 {
 qmp_migrate_cancel(NULL);
diff --git a/hmp.h b/hmp.h
index b302c8d..6a402b1 100644
--- a/hmp.h
+++ b/hmp.h
@@ -65,6 +65,7 @@ void hmp_drive_mirror(Monitor *mon, const QDict *qdict);
 void hmp_drive_backup(Monitor *mon, const QDict *qdict);
 void hmp_loadvm(Monitor *mon, const QDict *qdict);
 void hmp_savevm(Monitor *mon, const QDict *qdict);
+void hmp_delvm(Monitor *mon, const QDict *qdict);
 void hmp_migrate_cancel(Monitor *mon, const QDict *qdict);
 void hmp_migrate_incoming(Monitor *mon, const QDict *qdict);
 void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict);
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 914c36c..e4f355ceb 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -77,7 +77,6 @@ void qemu_remove_machine_init_done_notifier(Notifier *notify);
 
 int save_vmstate(const char *name);
 int load_vmstate(const char *name);
-void hmp_delvm(Monitor *mon, const QDict *qdict);
 void hmp_info_snapshots(Monitor *mon, const QDict *qdict);
 
 void qemu_announce_self(void);
diff --git a/migration/savevm.c b/migration/savevm.c
index bbff4d8..acd304b 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2304,19 +2304,6 @@ int load_vmstate(const char *name)
 return 0;
 }
 
-void hmp_delvm(Monitor *mon, const QDict *qdict)
-{
-BlockDriverState *bs;
-Error *err;
-const char *name = qdict_get_str(qdict, "name");
-
-if (bdrv_all_delete_snapshot(name, &bs, &err) < 0) {
-error_reportf_err(err,
-  "Error while deleting snapshot on device '%s': ",
-  bdrv_get_device_name(bs));
-}
-}
-
 void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
 {
 BlockDriverState *bs, *bs1;
-- 
2.9.3




[Qemu-devel] [PATCH 5/6] monitor: Move hmp_info_snapshots from savevm.c to hmp.c

2017-04-25 Thread Juan Quintela
It only uses block/* functions, nothing from migration.

Signed-off-by: Juan Quintela 
---
 hmp.c   | 143 ++
 hmp.h   |   1 +
 include/sysemu/sysemu.h |   1 -
 migration/savevm.c  | 147 
 4 files changed, 144 insertions(+), 148 deletions(-)

diff --git a/hmp.c b/hmp.c
index bb739ce..bd7b1ca 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1299,6 +1299,149 @@ void hmp_delvm(Monitor *mon, const QDict *qdict)
 }
 }
 
+void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
+{
+BlockDriverState *bs, *bs1;
+BdrvNextIterator it1;
+QEMUSnapshotInfo *sn_tab, *sn;
+bool no_snapshot = true;
+int nb_sns, i;
+int total;
+int *global_snapshots;
+AioContext *aio_context;
+
+typedef struct SnapshotEntry {
+QEMUSnapshotInfo sn;
+QTAILQ_ENTRY(SnapshotEntry) next;
+} SnapshotEntry;
+
+typedef struct ImageEntry {
+const char *imagename;
+QTAILQ_ENTRY(ImageEntry) next;
+QTAILQ_HEAD(, SnapshotEntry) snapshots;
+} ImageEntry;
+
+QTAILQ_HEAD(, ImageEntry) image_list =
+QTAILQ_HEAD_INITIALIZER(image_list);
+
+ImageEntry *image_entry, *next_ie;
+SnapshotEntry *snapshot_entry;
+
+bs = bdrv_all_find_vmstate_bs();
+if (!bs) {
+monitor_printf(mon, "No available block device supports snapshots\n");
+return;
+}
+aio_context = bdrv_get_aio_context(bs);
+
+aio_context_acquire(aio_context);
+nb_sns = bdrv_snapshot_list(bs, &sn_tab);
+aio_context_release(aio_context);
+
+if (nb_sns < 0) {
+monitor_printf(mon, "bdrv_snapshot_list: error %d\n", nb_sns);
+return;
+}
+
+for (bs1 = bdrv_first(&it1); bs1; bs1 = bdrv_next(&it1)) {
+int bs1_nb_sns = 0;
+ImageEntry *ie;
+SnapshotEntry *se;
+AioContext *ctx = bdrv_get_aio_context(bs1);
+
+aio_context_acquire(ctx);
+if (bdrv_can_snapshot(bs1)) {
+sn = NULL;
+bs1_nb_sns = bdrv_snapshot_list(bs1, &sn);
+if (bs1_nb_sns > 0) {
+no_snapshot = false;
+ie = g_new0(ImageEntry, 1);
+ie->imagename = bdrv_get_device_name(bs1);
+QTAILQ_INIT(&ie->snapshots);
+QTAILQ_INSERT_TAIL(&image_list, ie, next);
+for (i = 0; i < bs1_nb_sns; i++) {
+se = g_new0(SnapshotEntry, 1);
+se->sn = sn[i];
+QTAILQ_INSERT_TAIL(&ie->snapshots, se, next);
+}
+}
+g_free(sn);
+}
+aio_context_release(ctx);
+}
+
+if (no_snapshot) {
+monitor_printf(mon, "There is no snapshot available.\n");
+return;
+}
+
+global_snapshots = g_new0(int, nb_sns);
+total = 0;
+for (i = 0; i < nb_sns; i++) {
+SnapshotEntry *next_sn;
+if (bdrv_all_find_snapshot(sn_tab[i].name, &bs1) == 0) {
+global_snapshots[total] = i;
+total++;
+QTAILQ_FOREACH(image_entry, &image_list, next) {
+QTAILQ_FOREACH_SAFE(snapshot_entry, &image_entry->snapshots,
+next, next_sn) {
+if (!strcmp(sn_tab[i].name, snapshot_entry->sn.name)) {
+QTAILQ_REMOVE(&image_entry->snapshots, snapshot_entry,
+  next);
+g_free(snapshot_entry);
+}
+}
+}
+}
+}
+
+monitor_printf(mon, "List of snapshots present on all disks:\n");
+
+if (total > 0) {
+bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, NULL);
+monitor_printf(mon, "\n");
+for (i = 0; i < total; i++) {
+sn = &sn_tab[global_snapshots[i]];
+/* The ID is not guaranteed to be the same on all images, so
+ * overwrite it.
+ */
+pstrcpy(sn->id_str, sizeof(sn->id_str), "--");
+bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, sn);
+monitor_printf(mon, "\n");
+}
+} else {
+monitor_printf(mon, "None\n");
+}
+
+QTAILQ_FOREACH(image_entry, &image_list, next) {
+if (QTAILQ_EMPTY(&image_entry->snapshots)) {
+continue;
+}
+monitor_printf(mon,
+   "\nList of partial (non-loadable) snapshots on '%s':\n",
+   image_entry->imagename);
+bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, NULL);
+monitor_printf(mon, "\n");
+QTAILQ_FOREACH(snapshot_entry, &image_entry->snapshots, next) {
+bdrv_snapshot_dump((fprintf_function)monitor_printf, mon,
+   &snapshot_entry->sn);
+monitor_printf(mon, "\n");
+}
+}
+
+QTAILQ_FOREACH_SAFE(image_entry, &image_list, nex

[Qemu-devel] [PATCH 6/6] migration: Pass Error ** argument to {save, load}_vmstate

2017-04-25 Thread Juan Quintela
This way we use the "normal" way of printing errors for hmp commands.

--
Paolo suggestion

Signed-off-by: Juan Quintela 
---
 hmp.c|  9 +++--
 include/sysemu/sysemu.h  |  4 ++--
 migration/savevm.c   | 51 
 replay/replay-snapshot.c |  6 --
 vl.c |  4 +++-
 5 files changed, 41 insertions(+), 33 deletions(-)

diff --git a/hmp.c b/hmp.c
index bd7b1ca..d81f71e 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1273,17 +1273,22 @@ void hmp_loadvm(Monitor *mon, const QDict *qdict)
 {
 int saved_vm_running  = runstate_is_running();
 const char *name = qdict_get_str(qdict, "name");
+Error *err = NULL;
 
 vm_stop(RUN_STATE_RESTORE_VM);
 
-if (load_vmstate(name) == 0 && saved_vm_running) {
+if (load_vmstate(name, &err) == 0 && saved_vm_running) {
 vm_start();
 }
+hmp_handle_error(mon, &err);
 }
 
 void hmp_savevm(Monitor *mon, const QDict *qdict)
 {
-save_vmstate(qdict_get_try_str(qdict, "name"));
+Error *err = NULL;
+
+save_vmstate(qdict_get_try_str(qdict, "name"), &err);
+hmp_handle_error(mon, &err);
 }
 
 void hmp_delvm(Monitor *mon, const QDict *qdict)
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 15656b7..058d5eb 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -75,8 +75,8 @@ void qemu_remove_exit_notifier(Notifier *notify);
 void qemu_add_machine_init_done_notifier(Notifier *notify);
 void qemu_remove_machine_init_done_notifier(Notifier *notify);
 
-int save_vmstate(const char *name);
-int load_vmstate(const char *name);
+int save_vmstate(const char *name, Error **errp);
+int load_vmstate(const char *name, Error **errp);
 
 void qemu_announce_self(void);
 
diff --git a/migration/savevm.c b/migration/savevm.c
index 8dd4306..0c01988 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2066,7 +2066,7 @@ int qemu_loadvm_state(QEMUFile *f)
 return ret;
 }
 
-int save_vmstate(const char *name)
+int save_vmstate(const char *name, Error **errp)
 {
 BlockDriverState *bs, *bs1;
 QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
@@ -2076,29 +2076,27 @@ int save_vmstate(const char *name)
 uint64_t vm_state_size;
 qemu_timeval tv;
 struct tm tm;
-Error *local_err = NULL;
 AioContext *aio_context;
 
 if (!bdrv_all_can_snapshot(&bs)) {
-error_report("Device '%s' is writable but does not support snapshots",
- bdrv_get_device_name(bs));
+error_setg(errp, "Device '%s' is writable but does not support "
+   "snapshots", bdrv_get_device_name(bs));
 return ret;
 }
 
 /* Delete old snapshots of the same name */
 if (name) {
-ret = bdrv_all_delete_snapshot(name, &bs1, &local_err);
+ret = bdrv_all_delete_snapshot(name, &bs1, errp);
 if (ret < 0) {
-error_reportf_err(local_err,
-  "Error while deleting snapshot on device '%s': ",
-  bdrv_get_device_name(bs1));
+error_prepend(errp, "Error while deleting snapshot on device "
+  "'%s': ", bdrv_get_device_name(bs1));
 return ret;
 }
 }
 
 bs = bdrv_all_find_vmstate_bs();
 if (bs == NULL) {
-error_report("No block device can accept snapshots");
+error_setg(errp, "No block device can accept snapshots");
 return ret;
 }
 aio_context = bdrv_get_aio_context(bs);
@@ -2107,7 +2105,7 @@ int save_vmstate(const char *name)
 
 ret = global_state_store();
 if (ret) {
-error_report("Error saving global state");
+error_setg(errp, "Error saving global state");
 return ret;
 }
 vm_stop(RUN_STATE_SAVE_VM);
@@ -2139,21 +2137,20 @@ int save_vmstate(const char *name)
 /* save the VM state */
 f = qemu_fopen_bdrv(bs, 1);
 if (!f) {
-error_report("Could not open VM state file");
+error_setg(errp, "Could not open VM state file");
 goto the_end;
 }
-ret = qemu_savevm_state(f, &local_err);
+ret = qemu_savevm_state(f, errp);
 vm_state_size = qemu_ftell(f);
 qemu_fclose(f);
 if (ret < 0) {
-error_report_err(local_err);
 goto the_end;
 }
 
 ret = bdrv_all_create_snapshot(sn, bs, vm_state_size, &bs);
 if (ret < 0) {
-error_report("Error while creating snapshot on '%s'",
- bdrv_get_device_name(bs));
+error_setg(errp, "Error while creating snapshot on '%s'",
+   bdrv_get_device_name(bs));
 goto the_end;
 }
 
@@ -2226,7 +2223,7 @@ void qmp_xen_load_devices_state(const char *filename, 
Error **errp)
 migration_incoming_state_destroy();
 }
 
-int load_vmstate(const char *name)
+int load_vmstate(const char *name, Error **errp)
 {
 BlockDriverState *bs, *bs_vm_state;
 QEMUSnapshotInfo sn;
@@ -2236,20 +2233,22 @@ int load_vmstat

Re: [Qemu-devel] [PATCH 4/6] migration: calculate downtime on dst side

2017-04-25 Thread Peter Xu
On Tue, Apr 25, 2017 at 01:10:30PM +0300, Alexey Perevalov wrote:
> On 04/25/2017 11:24 AM, Peter Xu wrote:
> >On Fri, Apr 14, 2017 at 04:17:18PM +0300, Alexey Perevalov wrote:
> >
> >[...]
> >
> >>+/*
> >>+ * This function calculates downtime per cpu and trace it
> >>+ *
> >>+ *  Also it calculates total downtime as an interval's overlap,
> >>+ *  for many vCPU.
> >>+ *
> >>+ *  The approach is following:
> >>+ *  Initially intervals are represented in tree where key is
> >>+ *  pagefault address, and values:
> >>+ *   begin - page fault time
> >>+ *   end   - page load time
> >>+ *   cpus  - bit mask shows affected cpus
> >>+ *
> >>+ *  To calculate overlap on all cpus, intervals converted into
> >>+ *  array of points in time (downtime_points), the size of
> >>+ *  array is 2 * number of nodes in tree of intervals (2 array
> >>+ *  elements per one in element of interval).
> >>+ *  Each element is marked as end (E) or as start (S) of interval.
> >>+ *  The overlap downtime will be calculated for SE, only in case
> >>+ *  there is sequence S(0..N)E(M) for every vCPU.
> >>+ *
> >>+ * As example we have 3 CPU
> >>+ *
> >>+ *  S1E1   S1   E1
> >>+ * -***xxx***> 
> >>CPU1
> >>+ *
> >>+ * S2E2
> >>+ * xxx---> 
> >>CPU2
> >>+ *
> >>+ * S3E3
> >>+ * xxx---> 
> >>CPU3
> >>+ *
> >>+ * We have sequence S1,S2,E1,S3,S1,E2,E3,E1
> >>+ * S2,E1 - doesn't match condition due to sequence S1,S2,E1 doesn't 
> >>include CPU3
> >>+ * S3,S1,E2 - sequenece includes all CPUs, in this case overlap will be 
> >>S1,E2
> >>+ * Legend of picture is following: * - means downtime per vCPU
> >>+ * x - means overlapped downtime
> >>+ */
> >Not sure whether I get the point in this patch... iiuc we defined the
> >downtime here as the period when all vcpus are halted, right?
> >
> >If so, I have a few questions:
> >
> >- will this algorithm consume lots of memory? since I see we have one
> >   trace object per fault page address
> I don't think, it consumes too much, one DowntimeDuration
> takes (if I'm using bitmap_try_new, in this patch set I used pointer to
> uint64_t array to keep bitmap array,
> but I'm going to use include/qemu/bitmap.h, it works with pointers to long)
> 
> (2* int64 + (ROUND_UP((smp_cpus + BITS_PER_BYTE * sizeof(long) - 1 /
> (BITS_PER_BYTE * sizeof(long * siezof(long)
> so it's about 16 + at least 4 bytes, per page fault,
> Lets assume we migration 256 vCPU and 256 Gb of ram and that ram is based on
> 4Kb pages - it's really bad case
> 16 + ((256 + 8 * 4 - 1) / ( 8 * 4 )) * 4 = 52 bytes
> (256 * 1024 * 1024 * 1024)/(4 * 1024) = 67108864 page faults, but not all of
> these pages will be pagefaulted, due to
> page pre-fetching
> 67108864 * 52 = 3489660928 bytes (3.5 Gb for that operation),
> but I have a doubt, who will use 4Kb pages for 256 Gb, probably
> 2Mb or 1G huge page will be chosen on x86, on ARM or other architecture it
> could be another values.

Hmm, it looks still big though...

> 
> >
> >- do we need to protect the tree to make sure there's no insertion
> >   when doing the calculation?
> I asked the same question when sent RFC patches,
> the answer here is no, we should not, due to right now,
> it's only one socket and one listen thread (maybe in future,
> it will be required, maybe after multi fd patch set),
> and calculation is doing synchronously right after migration complete.

Okay.

> 
> >
> >- if the only thing we want here is the "total downtime", whether
> >   below would work? (assuming N is vcpu numbers)
> >
> >   a. define array cpu_fault_addr[N], to store current faulted address
> >  for each vcpu. When vcpu X is running, cpu_fault_addr[X] should
> >  be 0.
> >
> >   b. when page fault happens on vcpu A, setup cpu_fault_addr[A] with
> >  corresponding fault address.
> at this time need to is fault happens for all another vCPU,
> and if it happens mark current time as total vCPU downtime start.
> 
> >   c. when page copy finished, loop over cpu_fault_addr[] to see
> >  whether that matches any, clear corresponding element if matched.
> so when page copy finished and mark for total vCPU is set,
> yes that interval is a part of total downtime.
> >
> >   Then, we can just measure the period when cpu_fault_addr[] is all
> >   set (by tracing at both b. and c.). Can this work?
> Yes, it works, but it's better to keep time - cpu_fault_time,
> address is not important here, it doesn't matter the reason of pagefault.

We still need the addresses? So that when we do COPY, we can check the
new page address against these stored ones, to know which vcpus to
clear the bit.

> 2 vCPU could fault due to access to one page, ok, it's not a problem, just
> store
> time when it w

[Qemu-devel] [PATCH 3/3] migration: Remove old MigrationParams

2017-04-25 Thread Juan Quintela
Not used anymore after moving block migration to use capabilities.

Signed-off-by: Juan Quintela 
---
 include/migration/migration.h | 10 ++
 include/migration/vmstate.h   |  1 -
 include/qemu/typedefs.h   |  1 -
 include/sysemu/sysemu.h   |  3 +--
 migration/colo.c  |  2 +-
 migration/migration.c |  8 +++-
 migration/savevm.c| 16 +++-
 7 files changed, 10 insertions(+), 31 deletions(-)

diff --git a/include/migration/migration.h b/include/migration/migration.h
index 2917baa..3495162 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -41,10 +41,6 @@
 /* for vl.c */
 extern int only_migratable;
 
-struct MigrationParams {
-bool unused; /* C don't allow empty structs */
-};
-
 /* Messages sent on the return path from destination to source */
 enum mig_rp_message_type {
 MIG_RP_MSG_INVALID = 0,  /* Must be 0 */
@@ -134,12 +130,10 @@ struct MigrationState
 QEMUBH *cleanup_bh;
 QEMUFile *to_dst_file;
 
-/* New style params from 'migrate-set-parameters' */
+/* params from 'migrate-set-parameters' */
 MigrationParameters parameters;
 
 int state;
-/* Old style params from 'migrate' command */
-MigrationParams params;
 
 /* State related to return path */
 struct {
@@ -229,7 +223,7 @@ void migrate_fd_connect(MigrationState *s);
 
 void add_migration_state_change_notifier(Notifier *notify);
 void remove_migration_state_change_notifier(Notifier *notify);
-MigrationState *migrate_init(const MigrationParams *params);
+MigrationState *migrate_init(void);
 bool migration_is_blocked(Error **errp);
 bool migration_in_setup(MigrationState *);
 bool migration_is_idle(void);
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 9452dec..4396d7e 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -37,7 +37,6 @@ typedef int LoadStateHandler(QEMUFile *f, void *opaque, int 
version_id);
 
 typedef struct SaveVMHandlers {
 /* This runs inside the iothread lock.  */
-void (*set_params)(const MigrationParams *params, void * opaque);
 SaveStateHandler *save_state;
 
 void (*cleanup)(void *opaque);
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index f08d327..a388243 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -49,7 +49,6 @@ typedef struct MemoryRegion MemoryRegion;
 typedef struct MemoryRegionCache MemoryRegionCache;
 typedef struct MemoryRegionSection MemoryRegionSection;
 typedef struct MigrationIncomingState MigrationIncomingState;
-typedef struct MigrationParams MigrationParams;
 typedef struct MigrationState MigrationState;
 typedef struct Monitor Monitor;
 typedef struct MonitorDef MonitorDef;
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 058d5eb..3340202 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -102,8 +102,7 @@ enum qemu_vm_cmd {
 #define MAX_VM_CMD_PACKAGED_SIZE (1ul << 24)
 
 bool qemu_savevm_state_blocked(Error **errp);
-void qemu_savevm_state_begin(QEMUFile *f,
- const MigrationParams *params);
+void qemu_savevm_state_begin(QEMUFile *f);
 void qemu_savevm_state_header(QEMUFile *f);
 int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy);
 void qemu_savevm_state_cleanup(void);
diff --git a/migration/colo.c b/migration/colo.c
index 5c6c2f0..75e8807 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -333,7 +333,7 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
 }
 
 qemu_savevm_state_header(fb);
-qemu_savevm_state_begin(fb, &s->params);
+qemu_savevm_state_begin(fb);
 qemu_mutex_lock_iothread();
 qemu_savevm_state_complete_precopy(fb, false);
 qemu_mutex_unlock_iothread();
diff --git a/migration/migration.c b/migration/migration.c
index 9b96f1a..f094079 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1102,7 +1102,7 @@ bool migration_is_idle(void)
 return false;
 }
 
-MigrationState *migrate_init(const MigrationParams *params)
+MigrationState *migrate_init(void)
 {
 MigrationState *s = migrate_get_current();
 
@@ -1116,7 +1116,6 @@ MigrationState *migrate_init(const MigrationParams 
*params)
 s->cleanup_bh = 0;
 s->to_dst_file = NULL;
 s->state = MIGRATION_STATUS_NONE;
-s->params = *params;
 s->rp_state.from_dst_file = NULL;
 s->rp_state.error = false;
 s->mbps = 0.0;
@@ -1215,7 +1214,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
 {
 Error *local_err = NULL;
 MigrationState *s = migrate_get_current();
-MigrationParams params;
 const char *p;
 
 if (migration_is_setup_or_active(s->state) ||
@@ -1233,7 +1231,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
 return;
 }
 
-s = migrate_init(¶ms);
+s = migrate_init();
 
 if (has_blk && blk) {
 migrate_set_block_enabled(s);
@@ -1966,7 +1964,7 @@ static void *migration_threa

[Qemu-devel] [PATCH 0/3] Remove old MigrationParams

2017-04-25 Thread Juan Quintela
Hi

Upon a time there were MigrationParms (only used for block migration)
and then MigrationParams used for everything else.  This series:

- create migration capabilities for block parameters
- make the migrate command line parameters to use capabilities
- remove MigrationParams completely

Please, review.


Juan Quintela (3):
  migration: Create block capabilities for shared and enable
  migration: Remove use of old MigrationParams
  migration: Remove old MigrationParams

 include/migration/migration.h | 14 +---
 include/migration/vmstate.h   |  1 -
 include/qemu/typedefs.h   |  1 -
 include/sysemu/sysemu.h   |  3 +--
 migration/block.c | 17 ++
 migration/colo.c  |  5 +
 migration/migration.c | 52 ---
 migration/savevm.c| 18 +++
 qapi-schema.json  |  7 +-
 9 files changed, 62 insertions(+), 56 deletions(-)

-- 
2.9.3




[Qemu-devel] [PATCH 1/3] migration: Create block capabilities for shared and enable

2017-04-25 Thread Juan Quintela
Those two capabilities were added through the command line.  Notice that
we just created them.  This is just the boilerplate.

Signed-off-by: Juan Quintela 
Reviewed-by: Eric Blake 
---
 include/migration/migration.h |  3 +++
 migration/migration.c | 36 
 qapi-schema.json  |  7 ++-
 3 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/include/migration/migration.h b/include/migration/migration.h
index dfeca38..618ab0e 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -307,6 +307,9 @@ bool migrate_colo_enabled(void);
 
 int64_t xbzrle_cache_resize(int64_t new_size);
 
+bool migrate_use_block_enabled(void);
+bool migrate_use_block_shared(void);
+
 bool migrate_use_compression(void);
 int migrate_compress_level(void);
 int migrate_compress_threads(void);
diff --git a/migration/migration.c b/migration/migration.c
index 5447cab..775b24c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1195,6 +1195,16 @@ bool migration_is_blocked(Error **errp)
 return false;
 }
 
+static void migrate_set_block_shared(MigrationState *s)
+{
+s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = true;
+}
+
+static void migrate_set_block_enabled(MigrationState *s)
+{
+s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_ENABLED] = true;
+}
+
 void qmp_migrate(const char *uri, bool has_blk, bool blk,
  bool has_inc, bool inc, bool has_detach, bool detach,
  Error **errp)
@@ -1224,6 +1234,14 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
 
 s = migrate_init(¶ms);
 
+if (has_blk && blk) {
+migrate_set_block_enabled(s);
+}
+
+if (has_inc && inc) {
+migrate_set_block_shared(s);
+}
+
 if (strstart(uri, "tcp:", &p)) {
 tcp_start_outgoing_migration(s, p, &local_err);
 #ifdef CONFIG_RDMA
@@ -1419,6 +1437,24 @@ int64_t migrate_xbzrle_cache_size(void)
 return s->xbzrle_cache_size;
 }
 
+bool migrate_use_block_enabled(void)
+{
+MigrationState *s;
+
+s = migrate_get_current();
+
+return s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_ENABLED];
+}
+
+bool migrate_use_block_shared(void)
+{
+MigrationState *s;
+
+s = migrate_get_current();
+
+return s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED];
+}
+
 /* migration thread support */
 /*
  * Something bad happened to the RP stream, mark an error
diff --git a/qapi-schema.json b/qapi-schema.json
index 01b087f..e963bb3 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -894,11 +894,16 @@
 # @release-ram: if enabled, qemu will free the migrated ram pages on the source
 #during postcopy-ram migration. (since 2.9)
 #
+# @block-enabled: enable block migration (Since 2.10)
+#
+# @block-shared: enable block shared migration (Since 2.10)
+#
 # Since: 1.2
 ##
 { 'enum': 'MigrationCapability',
   'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
-   'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram'] }
+   'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram',
+   'block-enabled', 'block-shared' ] }
 
 ##
 # @MigrationCapabilityStatus:
-- 
2.9.3




[Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

2017-04-25 Thread Juan Quintela
We have change in the previous patch to use migration capabilities for
it.  Notice that we continue using the old command line flags from
migrate command from the time being.  Remove the set_params method as
now it is empty.

Signed-off-by: Juan Quintela 
---
 include/migration/migration.h |  3 +--
 migration/block.c | 17 ++---
 migration/colo.c  |  3 ---
 migration/migration.c |  8 +---
 migration/savevm.c|  2 --
 5 files changed, 8 insertions(+), 25 deletions(-)

diff --git a/include/migration/migration.h b/include/migration/migration.h
index 618ab0e..2917baa 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -42,8 +42,7 @@
 extern int only_migratable;
 
 struct MigrationParams {
-bool blk;
-bool shared;
+bool unused; /* C don't allow empty structs */
 };
 
 /* Messages sent on the return path from destination to source */
diff --git a/migration/block.c b/migration/block.c
index 060087f..fcfa823 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -94,9 +94,6 @@ typedef struct BlkMigBlock {
 } BlkMigBlock;
 
 typedef struct BlkMigState {
-/* Written during setup phase.  Can be read without a lock.  */
-int blk_enable;
-int shared_base;
 QSIMPLEQ_HEAD(bmds_list, BlkMigDevState) bmds_list;
 int64_t total_sector_sum;
 bool zero_blocks;
@@ -425,7 +422,7 @@ static int init_blk_migration(QEMUFile *f)
 bmds->bulk_completed = 0;
 bmds->total_sectors = sectors;
 bmds->completed_sectors = 0;
-bmds->shared_base = block_mig_state.shared_base;
+bmds->shared_base = migrate_use_block_shared();
 
 assert(i < num_bs);
 bmds_bs[i].bmds = bmds;
@@ -994,22 +991,12 @@ static int block_load(QEMUFile *f, void *opaque, int 
version_id)
 return 0;
 }
 
-static void block_set_params(const MigrationParams *params, void *opaque)
-{
-block_mig_state.blk_enable = params->blk;
-block_mig_state.shared_base = params->shared;
-
-/* shared base means that blk_enable = 1 */
-block_mig_state.blk_enable |= params->shared;
-}
-
 static bool block_is_active(void *opaque)
 {
-return block_mig_state.blk_enable == 1;
+return migrate_use_block_enabled();
 }
 
 static SaveVMHandlers savevm_block_handlers = {
-.set_params = block_set_params,
 .save_live_setup = block_save_setup,
 .save_live_iterate = block_save_iterate,
 .save_live_complete_precopy = block_save_complete,
diff --git a/migration/colo.c b/migration/colo.c
index c19eb3f..5c6c2f0 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -332,9 +332,6 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
 goto out;
 }
 
-/* Disable block migration */
-s->params.blk = 0;
-s->params.shared = 0;
 qemu_savevm_state_header(fb);
 qemu_savevm_state_begin(fb, &s->params);
 qemu_mutex_lock_iothread();
diff --git a/migration/migration.c b/migration/migration.c
index 775b24c..9b96f1a 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -787,6 +787,10 @@ void 
qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
 s->enabled_capabilities[cap->value->capability] = cap->value->state;
 }
 
+if (s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED]) {
+s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_ENABLED] = true;
+}
+
 if (migrate_postcopy_ram()) {
 if (migrate_use_compression()) {
 /* The decompression threads asynchronously write into RAM
@@ -1214,9 +1218,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
 MigrationParams params;
 const char *p;
 
-params.blk = has_blk && blk;
-params.shared = has_inc && inc;
-
 if (migration_is_setup_or_active(s->state) ||
 s->state == MIGRATION_STATUS_CANCELLING ||
 s->state == MIGRATION_STATUS_COLO) {
@@ -1239,6 +1240,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
 }
 
 if (has_inc && inc) {
+migrate_set_block_enabled(s);
 migrate_set_block_shared(s);
 }
 
diff --git a/migration/savevm.c b/migration/savevm.c
index 0c01988..102b11d 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1233,8 +1233,6 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
 {
 int ret;
 MigrationParams params = {
-.blk = 0,
-.shared = 0
 };
 MigrationState *ms = migrate_init(¶ms);
 MigrationStatus status;
-- 
2.9.3




Re: [Qemu-devel] [Qemu-devel RFC v2 1/4] msf2: Add Smartfusion2 System timer

2017-04-25 Thread sundeep subbaraya
Hi Alistair,

On Mon, Apr 24, 2017 at 11:14 PM, Alistair Francis  wrote:
 +
 +isr = !!(st->regs[R_RIS] & TIMER_RIS_ACK);
 +ier = !!(st->regs[R_CTRL] & TIMER_CTRL_INTR);
 +
 +qemu_set_irq(st->irq, (ier && isr));
 +}
 +
 +static uint64_t
 +timer_read(void *opaque, hwaddr addr, unsigned int size)
 +{
 +struct timerblock *t = opaque;
 +struct msf2_timer *st;
 +uint32_t r = 0;
 +unsigned int timer;
 +int isr;
 +int ier;
 +
 +addr >>= 2;
 +timer = timer_from_addr(addr);
 +st = &t->timers[timer];
 +
 +if (timer) {
 +addr -= 6;
 +}
>>>
>>> Isn't this timer logic just checking if (addr >> 2) == R_MAX and if it
>>> is set (addr >> 2) back to zero? This seems an overly complex way to
>>> check that.
>> I did not get you clearly. Do you want me to write like this:
>> unsigned int timer = 0;
>>
>> addr >>= 2;
>> if (addr >= R_MAX) {
>> timer = 1;
>> addr =  addr - R_MAX;
>> }
>
> Yeah, I think this is clearer then what you had earlier.
>
> Although why do you have to subtract R_MAX, shouldn't it just be an
> error if accessing values larger then R_MAX?

Sorry I forgot about replying to this in earlier mail.
There are two independent timer blocks accessing same base address.
Based on offset passed in read/write functions we figure out
which block has to be handled.
0x0 to 0x14 -> timer1
0x18 to 0x2C -> timer2
Here R_MAX is 0x18 hence addr >= R_MAX is valid and refers to timer2.
Although I missed the bounds checking 0 < addr < 0x2C. I will add that
check in read and
write functions.

Thanks,
Sundeep
>
>>
>>>
 +
 +switch (addr) {
 +case R_VAL:
 +r = ptimer_get_count(st->ptimer);
 +D(qemu_log("msf2_timer t=%d read counter=%x\n", timer, r));
 +break;
 +
 +case R_MIS:
 +isr = !!(st->regs[R_RIS] & TIMER_RIS_ACK);
 +ier = !!(st->regs[R_CTRL] & TIMER_CTRL_INTR);
 +r = ier && isr;
 +break;
 +
 +default:
 +if (addr < ARRAY_SIZE(st->regs)) {
 +r = st->regs[addr];
 +}
 +break;
 +}
 +D(fprintf(stderr, "%s timer=%d %x=%x\n", __func__, timer, addr * 4, 
 r));
 +return r;
 +}
 +
 +static void timer_update(struct msf2_timer *st)
 +{
 +uint64_t count;
 +
 +D(fprintf(stderr, "%s timer=%d\n", __func__, st->nr));
 +
 +if (!(st->regs[R_CTRL] & TIMER_CTRL_ENBL)) {
 +ptimer_stop(st->ptimer);
 +return;
 +}
 +
 +count = st->regs[R_LOADVAL];
 +ptimer_set_limit(st->ptimer, count, 1);
 +ptimer_run(st->ptimer, 1);
 +}
>>>
>>> The update function should be above the read/write functions.
>>>
>> Ok I will change
>>
 +
 +static void
 +timer_write(void *opaque, hwaddr addr,
 +uint64_t val64, unsigned int size)
 +{
 +struct timerblock *t = opaque;
 +struct msf2_timer *st;
 +unsigned int timer;
 +uint32_t value = val64;
 +
 +addr >>= 2;
 +timer = timer_from_addr(addr);
 +st = &t->timers[timer];
 +D(fprintf(stderr, "%s addr=%x val=%x (timer=%d)\n",
 + __func__, addr * 4, value, timer));
 +
 +if (timer) {
 +addr -= 6;
 +}
>>>
>>> Same comment from the read function.
>>>
 +
 +switch (addr) {
 +case R_CTRL:
 +st->regs[R_CTRL] = value;
 +timer_update(st);
 +break;
 +
 +case R_RIS:
 +if (value & TIMER_RIS_ACK) {
 +st->regs[R_RIS] &= ~TIMER_RIS_ACK;
 +}
 +break;
 +
 +case R_LOADVAL:
 +st->regs[R_LOADVAL] = value;
 +if (st->regs[R_CTRL] & TIMER_CTRL_ENBL) {
 +timer_update(st);
 +}
 +break;
 +
 +case R_BGLOADVAL:
 +st->regs[R_BGLOADVAL] = value;
 +st->regs[R_LOADVAL] = value;
 +break;
 +
 +case R_VAL:
 +case R_MIS:
 +break;
 +
 +default:
 +if (addr < ARRAY_SIZE(st->regs)) {
 +st->regs[addr] = value;
 +}
 +break;
 +}
 +timer_update_irq(st);
 +}
 +
 +static const MemoryRegionOps timer_ops = {
 +.read = timer_read,
 +.write = timer_write,
 +.endianness = DEVICE_NATIVE_ENDIAN,
 +.valid = {
 +.min_access_size = 4,
 +.max_access_size = 4
 +}
 +};
 +
 +static void timer_hit(void *opaque)
 +{
 +struct msf2_timer *st = opaque;
 +D(fprintf(stderr, "%s %d\n", __func__, st->nr));
 +st->regs[R_RIS] |= TIMER_RIS_ACK;
 +
 +if (!(st->regs[R_CTRL] & TIMER_CTRL_ONESHOT)) {
 +

[Qemu-devel] [PULL 3/4] libqtest: Add a generic function to run a callback function for every machine

2017-04-25 Thread Dr. David Alan Gilbert (git)
From: Thomas Huth 

Some tests need to run single tests for every available machine of the
current QEMU binary. To avoid code duplication, let's extract this
code that deals with 'query-machines' into a separate function.

Signed-off-by: Thomas Huth 
Message-Id: <1490860207-8302-3-git-send-email-th...@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Dr. David Alan Gilbert 
---
 tests/libqtest.c| 30 +
 tests/libqtest.h|  8 +
 tests/pc-cpu-test.c | 95 -
 tests/qom-test.c| 36 
 4 files changed, 80 insertions(+), 89 deletions(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index 0b0bf1d460..512c150266 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -946,3 +946,33 @@ bool qtest_big_endian(QTestState *s)
 {
 return s->big_endian;
 }
+
+void qtest_cb_for_every_machine(void (*cb)(const char *machine))
+{
+QDict *response, *minfo;
+QList *list;
+const QListEntry *p;
+QObject *qobj;
+QString *qstr;
+const char *mname;
+
+qtest_start("-machine none");
+response = qmp("{ 'execute': 'query-machines' }");
+g_assert(response);
+list = qdict_get_qlist(response, "return");
+g_assert(list);
+
+for (p = qlist_first(list); p; p = qlist_next(p)) {
+minfo = qobject_to_qdict(qlist_entry_obj(p));
+g_assert(minfo);
+qobj = qdict_get(minfo, "name");
+g_assert(qobj);
+qstr = qobject_to_qstring(qobj);
+g_assert(qstr);
+mname = qstring_get_str(qstr);
+cb(mname);
+}
+
+qtest_end();
+QDECREF(response);
+}
diff --git a/tests/libqtest.h b/tests/libqtest.h
index ee237448da..38bc1e9953 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -919,4 +919,12 @@ void qmp_fd_send(int fd, const char *fmt, ...);
 QDict *qmp_fdv(int fd, const char *fmt, va_list ap);
 QDict *qmp_fd(int fd, const char *fmt, ...);
 
+/**
+ * qtest_cb_for_every_machine:
+ * @cb: Pointer to the callback function
+ *
+ *  Call a callback function for every name of all available machines.
+ */
+void qtest_cb_for_every_machine(void (*cb)(const char *machine));
+
 #endif
diff --git a/tests/pc-cpu-test.c b/tests/pc-cpu-test.c
index c3a2633d3c..c4211a4e85 100644
--- a/tests/pc-cpu-test.c
+++ b/tests/pc-cpu-test.c
@@ -79,69 +79,46 @@ static void test_data_free(gpointer data)
 g_free(pc);
 }
 
-static void add_pc_test_cases(void)
+static void add_pc_test_case(const char *mname)
 {
-QDict *response, *minfo;
-QList *list;
-const QListEntry *p;
-QObject *qobj;
-QString *qstr;
-const char *mname;
 char *path;
 PCTestData *data;
 
-qtest_start("-machine none");
-response = qmp("{ 'execute': 'query-machines' }");
-g_assert(response);
-list = qdict_get_qlist(response, "return");
-g_assert(list);
-
-for (p = qlist_first(list); p; p = qlist_next(p)) {
-minfo = qobject_to_qdict(qlist_entry_obj(p));
-g_assert(minfo);
-qobj = qdict_get(minfo, "name");
-g_assert(qobj);
-qstr = qobject_to_qstring(qobj);
-g_assert(qstr);
-mname = qstring_get_str(qstr);
-if (!g_str_has_prefix(mname, "pc-")) {
-continue;
-}
-data = g_malloc(sizeof(PCTestData));
-data->machine = g_strdup(mname);
-data->cpu_model = "Haswell"; /* 1.3+ theoretically */
-data->sockets = 1;
-data->cores = 3;
-data->threads = 2;
-data->maxcpus = data->sockets * data->cores * data->threads * 2;
-if (g_str_has_suffix(mname, "-1.4") ||
-(strcmp(mname, "pc-1.3") == 0) ||
-(strcmp(mname, "pc-1.2") == 0) ||
-(strcmp(mname, "pc-1.1") == 0) ||
-(strcmp(mname, "pc-1.0") == 0) ||
-(strcmp(mname, "pc-0.15") == 0) ||
-(strcmp(mname, "pc-0.14") == 0) ||
-(strcmp(mname, "pc-0.13") == 0) ||
-(strcmp(mname, "pc-0.12") == 0) ||
-(strcmp(mname, "pc-0.11") == 0) ||
-(strcmp(mname, "pc-0.10") == 0)) {
-path = g_strdup_printf("cpu/%s/init/%ux%ux%u&maxcpus=%u",
-   mname, data->sockets, data->cores,
-   data->threads, data->maxcpus);
-qtest_add_data_func_full(path, data, test_pc_without_cpu_add,
- test_data_free);
-g_free(path);
-} else {
-path = g_strdup_printf("cpu/%s/add/%ux%ux%u&maxcpus=%u",
-   mname, data->sockets, data->cores,
-   data->threads, data->maxcpus);
-qtest_add_data_func_full(path, data, test_pc_with_cpu_add,
- test_data_free);
-g_free(path);
-}
+if (!g_str_has_prefix(mname, "pc-")) {
+return;
+}
+data = g_malloc(sizeof(PCTestData));
+data->machine = g_str

[Qemu-devel] [PULL 0/4] hmp queue

2017-04-25 Thread Dr. David Alan Gilbert (git)
From: "Dr. David Alan Gilbert" 

The following changes since commit f4b5b021c847669b1c78050aea26fe9abceef6dd:

  Merge remote-tracking branch 'remotes/cody/tags/block-pull-request' into 
staging (2017-04-25 09:21:54 +0100)

are available in the git repository at:

  git://github.com/dagrh/qemu.git tags/pull-hmp-20170425

for you to fetch changes up to 1eb8e78dd1cd4e0b4170fd42f6d8882c867f334b:

  tests: Add a tester for HMP commands (2017-04-25 11:26:52 +0100)


HMP pull with fixed test/strcmp case


Paolo Bonzini (1):
  hmp: gpa2hva and gpa2hpa hostaddr command

Thomas Huth (3):
  libqtest: Ignore QMP events when parsing the response for HMP commands
  libqtest: Add a generic function to run a callback function for every 
machine
  tests: Add a tester for HMP commands

 hmp-commands.hx|  32 ++
 monitor.c  | 101 +++
 tests/Makefile.include |   2 +
 tests/libqtest.c   |  36 +++
 tests/libqtest.h   |  12 +++-
 tests/pc-cpu-test.c|  95 +++--
 tests/qom-test.c   |  36 ++-
 tests/test-hmp.c   | 161 +
 8 files changed, 385 insertions(+), 90 deletions(-)
 create mode 100644 tests/test-hmp.c



[Qemu-devel] [PULL 4/4] tests: Add a tester for HMP commands

2017-04-25 Thread Dr. David Alan Gilbert (git)
From: Thomas Huth 

HMP commands do not get any automatic testing yet, so on certain
QEMU machines, some HMP commands were causing crashes in the past.
Thus we should test HMP commands in our test suite, too, to avoid
that such problems creep in again in the future.

Signed-off-by: Thomas Huth 
Message-Id: <1493097407-20482-1-git-send-email-th...@redhat.com>
Reviewed-by: Dr. David Alan Gilbert 
Signed-off-by: Dr. David Alan Gilbert 
---
 tests/Makefile.include |   2 +
 tests/test-hmp.c   | 161 +
 2 files changed, 163 insertions(+)
 create mode 100644 tests/test-hmp.c

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 579ec07cce..31931c0d77 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -331,6 +331,7 @@ check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
 check-qtest-s390x-y = tests/boot-serial-test$(EXESUF)
 
 check-qtest-generic-y += tests/qom-test$(EXESUF)
+check-qtest-generic-y += tests/test-hmp$(EXESUF)
 
 qapi-schema += alternate-any.json
 qapi-schema += alternate-array.json
@@ -720,6 +721,7 @@ tests/tpci200-test$(EXESUF): tests/tpci200-test.o
 tests/display-vga-test$(EXESUF): tests/display-vga-test.o
 tests/ipoctal232-test$(EXESUF): tests/ipoctal232-test.o
 tests/qom-test$(EXESUF): tests/qom-test.o
+tests/test-hmp$(EXESUF): tests/test-hmp.o
 tests/drive_del-test$(EXESUF): tests/drive_del-test.o $(libqos-pc-obj-y)
 tests/qdev-monitor-test$(EXESUF): tests/qdev-monitor-test.o $(libqos-pc-obj-y)
 tests/nvme-test$(EXESUF): tests/nvme-test.o
diff --git a/tests/test-hmp.c b/tests/test-hmp.c
new file mode 100644
index 00..99e35ec15a
--- /dev/null
+++ b/tests/test-hmp.c
@@ -0,0 +1,161 @@
+/*
+ * Test HMP commands.
+ *
+ * Copyright (c) 2017 Red Hat Inc.
+ *
+ * Author:
+ *Thomas Huth 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2
+ * or later. See the COPYING file in the top-level directory.
+ *
+ * This test calls some HMP commands for all machines that the current
+ * QEMU binary provides, to check whether they terminate successfully
+ * (i.e. do not crash QEMU).
+ */
+
+#include "qemu/osdep.h"
+#include "libqtest.h"
+
+static int verbose;
+
+static const char *hmp_cmds[] = {
+"boot_set ndc",
+"chardev-add null,id=testchardev1",
+"chardev-remove testchardev1",
+"commit all",
+"cpu-add 1",
+"cpu 0",
+"device_add ?",
+"device_add usb-mouse,id=mouse1",
+"mouse_button 7",
+"mouse_move 10 10",
+"mouse_button 0",
+"device_del mouse1",
+"dump-guest-memory /dev/null 0 4096",
+"gdbserver",
+"host_net_add user id=net0",
+"hostfwd_add tcp::43210-:43210",
+"hostfwd_remove tcp::43210-:43210",
+"host_net_remove 0 net0",
+"i /w 0",
+"log all",
+"log none",
+"memsave 0 4096 \"/dev/null\"",
+"migrate_set_cache_size 1",
+"migrate_set_downtime 1",
+"migrate_set_speed 1",
+"netdev_add user,id=net1",
+"set_link net1 off",
+"set_link net1 on",
+"netdev_del net1",
+"nmi",
+"o /w 0 0x1234",
+"object_add memory-backend-ram,id=mem1,size=256M",
+"object_del mem1",
+"pmemsave 0 4096 \"/dev/null\"",
+"p $pc + 8",
+"qom-list /",
+"qom-set /machine initrd test",
+"screendump /dev/null",
+"sendkey x",
+"singlestep on",
+"wavcapture /dev/null",
+"stopcapture 0",
+"sum 0 512",
+"x /8i 0x100",
+"xp /16x 0",
+NULL
+};
+
+/* Run through the list of pre-defined commands */
+static void test_commands(void)
+{
+char *response;
+int i;
+
+for (i = 0; hmp_cmds[i] != NULL; i++) {
+if (verbose) {
+fprintf(stderr, "\t%s\n", hmp_cmds[i]);
+}
+response = hmp(hmp_cmds[i]);
+g_free(response);
+}
+
+}
+
+/* Run through all info commands and call them blindly (without arguments) */
+static void test_info_commands(void)
+{
+char *resp, *info, *info_buf, *endp;
+
+info_buf = info = hmp("help info");
+
+while (*info) {
+/* Extract the info command, ignore parameters and description */
+g_assert(strncmp(info, "info ", 5) == 0);
+endp = strchr(&info[5], ' ');
+g_assert(endp != NULL);
+*endp = '\0';
+/* Now run the info command */
+if (verbose) {
+fprintf(stderr, "\t%s\n", info);
+}
+resp = hmp(info);
+g_free(resp);
+/* And move forward to the next line */
+info = strchr(endp + 1, '\n');
+if (!info) {
+break;
+}
+info += 1;
+}
+
+g_free(info_buf);
+}
+
+static void test_machine(gconstpointer data)
+{
+const char *machine = data;
+char *args;
+
+args = g_strdup_printf("-S -M %s", machine);
+qtest_start(args);
+
+test_info_commands();
+test_commands();
+
+qtest_end();
+g_free(args);
+g_free((void *)data);
+}
+
+static void add_machine_test_case(const char *mname)
+{
+char *path;
+
+/* Ignore blackli

[Qemu-devel] [PULL 2/4] libqtest: Ignore QMP events when parsing the response for HMP commands

2017-04-25 Thread Dr. David Alan Gilbert (git)
From: Thomas Huth 

When running certain HMP commands (like "device_del") via QMP, we
can sometimes get a QMP event in the response first, so that the
"g_assert(ret)" statement in qtest_hmp() triggers and the test
fails. Fix this by ignoring such QMP events while looking for the
real return value from QMP.

Signed-off-by: Thomas Huth 
Message-Id: <1490860207-8302-2-git-send-email-th...@redhat.com>
Reviewed-by: Markus Armbruster 
Signed-off-by: Dr. David Alan Gilbert 
  Added note to qtest_hmp/qtest_hmpv's header description to say
  it discards events
---
 tests/libqtest.c | 6 ++
 tests/libqtest.h | 4 +++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index 99b1195355..0b0bf1d460 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -588,6 +588,12 @@ char *qtest_hmpv(QTestState *s, const char *fmt, va_list 
ap)
  " 'arguments': {'command-line': %s}}",
  cmd);
 ret = g_strdup(qdict_get_try_str(resp, "return"));
+while (ret == NULL && qdict_get_try_str(resp, "event")) {
+/* Ignore asynchronous QMP events */
+QDECREF(resp);
+resp = qtest_qmp_receive(s);
+ret = g_strdup(qdict_get_try_str(resp, "return"));
+}
 g_assert(ret);
 QDECREF(resp);
 g_free(cmd);
diff --git a/tests/libqtest.h b/tests/libqtest.h
index 2c9962d94f..ee237448da 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -132,11 +132,12 @@ void qtest_qmp_eventwait(QTestState *s, const char 
*event);
 QDict *qtest_qmp_eventwait_ref(QTestState *s, const char *event);
 
 /**
- * qtest_hmpv:
+ * qtest_hmp:
  * @s: #QTestState instance to operate on.
  * @fmt...: HMP command to send to QEMU
  *
  * Send HMP command to QEMU via QMP's human-monitor-command.
+ * QMP events are discarded.
  *
  * Returns: the command's output.  The caller should g_free() it.
  */
@@ -149,6 +150,7 @@ char *qtest_hmp(QTestState *s, const char *fmt, ...);
  * @ap: HMP command arguments
  *
  * Send HMP command to QEMU via QMP's human-monitor-command.
+ * QMP events are discarded.
  *
  * Returns: the command's output.  The caller should g_free() it.
  */
-- 
2.12.2




[Qemu-devel] [PULL 1/4] hmp: gpa2hva and gpa2hpa hostaddr command

2017-04-25 Thread Dr. David Alan Gilbert (git)
From: Paolo Bonzini 

These commands are useful when testing machine-check passthrough.
gpa2hva is useful to inject a MADV_HWPOISON madvise from gdb, while
gpa2hpa is useful to inject an error with the mce-inject kernel
module.

Signed-off-by: Paolo Bonzini 
Message-Id: <1490021158-4469-1-git-send-email-pbonz...@redhat.com>
Signed-off-by: Paolo Bonzini 
Message-Id: <20170420133058.12911-1-pbonz...@redhat.com>
Reviewed-by: Dr. David Alan Gilbert 
Signed-off-by: Dr. David Alan Gilbert 
---
 hmp-commands.hx |  32 ++
 monitor.c   | 101 
 2 files changed, 133 insertions(+)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 88192817b2..0aca984261 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -526,6 +526,38 @@ Dump 80 16 bit values at the start of the video memory.
 ETEXI
 
 {
+.name   = "gpa2hva",
+.args_type  = "addr:l",
+.params = "addr",
+.help   = "print the host virtual address corresponding to a guest 
physical address",
+.cmd= hmp_gpa2hva,
+},
+
+STEXI
+@item gpa2hva @var{addr}
+@findex gpa2hva
+Print the host virtual address at which the guest's physical address @var{addr}
+is mapped.
+ETEXI
+
+#ifdef CONFIG_LINUX
+{
+.name   = "gpa2hpa",
+.args_type  = "addr:l",
+.params = "addr",
+.help   = "print the host physical address corresponding to a 
guest physical address",
+.cmd= hmp_gpa2hpa,
+},
+#endif
+
+STEXI
+@item gpa2hpa @var{addr}
+@findex gpa2hpa
+Print the host physical address at which the guest's physical address 
@var{addr}
+is mapped.
+ETEXI
+
+{
 .name   = "p|print",
 .args_type  = "fmt:/,val:l",
 .params = "/fmt expr",
diff --git a/monitor.c b/monitor.c
index be282ecb80..a27dc8003f 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1421,6 +1421,107 @@ static void hmp_physical_memory_dump(Monitor *mon, 
const QDict *qdict)
 memory_dump(mon, count, format, size, addr, 1);
 }
 
+static void *gpa2hva(MemoryRegion **p_mr, hwaddr addr, Error **errp)
+{
+MemoryRegionSection mrs = memory_region_find(get_system_memory(),
+ addr, 1);
+
+if (!mrs.mr) {
+error_setg(errp, "No memory is mapped at address 0x%" HWADDR_PRIx, 
addr);
+return NULL;
+}
+
+if (!memory_region_is_ram(mrs.mr) && !memory_region_is_romd(mrs.mr)) {
+error_setg(errp, "Memory at address 0x%" HWADDR_PRIx "is not RAM", 
addr);
+memory_region_unref(mrs.mr);
+return NULL;
+}
+
+*p_mr = mrs.mr;
+return qemu_map_ram_ptr(mrs.mr->ram_block, mrs.offset_within_region);
+}
+
+static void hmp_gpa2hva(Monitor *mon, const QDict *qdict)
+{
+hwaddr addr = qdict_get_int(qdict, "addr");
+Error *local_err = NULL;
+MemoryRegion *mr = NULL;
+void *ptr;
+
+ptr = gpa2hva(&mr, addr, &local_err);
+if (local_err) {
+error_report_err(local_err);
+return;
+}
+
+monitor_printf(mon, "Host virtual address for 0x%" HWADDR_PRIx
+   " (%s) is %p\n",
+   addr, mr->name, ptr);
+
+memory_region_unref(mr);
+}
+
+#ifdef CONFIG_LINUX
+static uint64_t vtop(void *ptr, Error **errp)
+{
+uint64_t pinfo;
+uint64_t ret = -1;
+uintptr_t addr = (uintptr_t) ptr;
+uintptr_t pagesize = getpagesize();
+off_t offset = addr / pagesize * sizeof(pinfo);
+int fd;
+
+fd = open("/proc/self/pagemap", O_RDONLY);
+if (fd == -1) {
+error_setg_errno(errp, errno, "Cannot open /proc/self/pagemap");
+return -1;
+}
+
+/* Force copy-on-write if necessary.  */
+atomic_add((uint8_t *)ptr, 0);
+
+if (pread(fd, &pinfo, sizeof(pinfo), offset) != sizeof(pinfo)) {
+error_setg_errno(errp, errno, "Cannot read pagemap");
+goto out;
+}
+if ((pinfo & (1ull << 63)) == 0) {
+error_setg(errp, "Page not present");
+goto out;
+}
+ret = ((pinfo & 0x007full) * pagesize) | (addr & (pagesize - 
1));
+
+out:
+close(fd);
+return ret;
+}
+
+static void hmp_gpa2hpa(Monitor *mon, const QDict *qdict)
+{
+hwaddr addr = qdict_get_int(qdict, "addr");
+Error *local_err = NULL;
+MemoryRegion *mr = NULL;
+void *ptr;
+uint64_t physaddr;
+
+ptr = gpa2hva(&mr, addr, &local_err);
+if (local_err) {
+error_report_err(local_err);
+return;
+}
+
+physaddr = vtop(ptr, &local_err);
+if (local_err) {
+error_report_err(local_err);
+} else {
+monitor_printf(mon, "Host physical address for 0x%" HWADDR_PRIx
+   " (%s) is 0x%" PRIx64 "\n",
+   addr, mr->name, (uint64_t) physaddr);
+}
+
+memory_region_unref(mr);
+}
+#endif
+
 static void do_print(Monitor *mon, const QDict *qdict)
 {
 int format = qdict_get_int(qdict, "format");
-- 
2.12.2




[Qemu-devel] [PATCH] tcg: Initialize return value after exit_atomic

2017-04-25 Thread Richard Henderson
Users of tcg_gen_atomic_cmpxchg and do_atomic_op rightfully utilize
the output.  Even though this code is dead, it gets translated, and
without the initialization we encounter a tcg_error.

Reported-by: Nikunj A Dadhania 
Signed-off-by: Richard Henderson 
---
 tcg/tcg-op.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
index 95a39b7..6b1f415 100644
--- a/tcg/tcg-op.c
+++ b/tcg/tcg-op.c
@@ -2861,6 +2861,9 @@ void tcg_gen_atomic_cmpxchg_i64(TCGv_i64 retv, TCGv addr, 
TCGv_i64 cmpv,
 #endif
 #else
 gen_helper_exit_atomic(tcg_ctx.tcg_env);
+/* Produce a result, so that we have a well-formed opcode stream
+   with respect to uses of the result in the (dead) code following.  */
+tcg_gen_movi_i64(retv, 0);
 #endif /* CONFIG_ATOMIC64 */
 } else {
 TCGv_i32 c32 = tcg_temp_new_i32();
@@ -2966,6 +2969,9 @@ static void do_atomic_op_i64(TCGv_i64 ret, TCGv addr, 
TCGv_i64 val,
 #endif
 #else
 gen_helper_exit_atomic(tcg_ctx.tcg_env);
+/* Produce a result, so that we have a well-formed opcode stream
+   with respect to uses of the result in the (dead) code following.  */
+tcg_gen_movi_i64(ret, 0);
 #endif /* CONFIG_ATOMIC64 */
 } else {
 TCGv_i32 v32 = tcg_temp_new_i32();
-- 
2.9.3




[Qemu-devel] [PATCH 03/11] hw/s390x/sclp: update LOADPARM in SCP Info

2017-04-25 Thread Cornelia Huck
From: Farhan Ali 

LOADPARM has two copies:
1. in SCP Information Block
2. in IPL Information Parameter Block

So, update SCLP intrinsics now. We always store LOADPARM in SCP
information block even if we don't have a valid IPL Information
Parameter Block.

Initial patch from Eugene (jno) Dvurechenski.

Signed-off-by: Eugene (jno) Dvurechenski 
Signed-off-by: Farhan Ali 
Reviewed-by: Christian Borntraeger 
Signed-off-by: Cornelia Huck 
---
 hw/s390x/sclp.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index e741da1141..b4f6dd58dd 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -23,6 +23,7 @@
 #include "hw/s390x/sclp.h"
 #include "hw/s390x/event-facility.h"
 #include "hw/s390x/s390-pci-bus.h"
+#include "hw/s390x/ipl.h"
 
 static inline SCLPDevice *get_sclp_device(void)
 {
@@ -57,6 +58,7 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
 int cpu_count = 0;
 int rnsize, rnmax;
 int slots = MIN(machine->ram_slots, s390_get_memslot_count(kvm_state));
+IplParameterBlock *ipib = s390_ipl_get_iplb();
 
 CPU_FOREACH(cpu) {
 cpu_count++;
@@ -129,6 +131,13 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
 read_info->rnmax2 = cpu_to_be64(rnmax);
 }
 
+if (ipib && ipib->flags & DIAG308_FLAGS_LP_VALID) {
+memcpy(&read_info->loadparm, &ipib->loadparm,
+   sizeof(read_info->loadparm));
+} else {
+s390_ipl_set_loadparm(read_info->loadparm);
+}
+
 sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_READ_COMPLETION);
 }
 
-- 
2.11.0




[Qemu-devel] [PATCH 00/11] s390x: support for LOADPARM

2017-04-25 Thread Cornelia Huck
This patchset implements the LOADPARM machine property. This is
exposed via SCLP and diagnose 308 to the guest. It will be used
by the bios to select a boot entry; guest operating systems can
use it as well.

Cornelia Huck (1):
  pc-bios/s390-ccw.img: update image

Eugene (jno) Dvurechenski (2):
  pc-bios/s390-ccw: Make ebcdic/ascii conversion public
  pc-bios/s390-ccw: add boot entry selection to El Torito routine

Farhan Ali (8):
  hw/s390x: provide loadparm property for the machine
  hw/s390x/ipl: enable LOADPARM in IPIB for a boot device
  hw/s390x/sclp: update LOADPARM in SCP Info
  util/qemu-config: Add loadparm to qemu machine_opts
  pc-bios/s390-ccw: get LOADPARM stored in SCP Read Info
  pc-bios/s390-ccw: provide a function to interpret LOADPARM value
  pc-bios/s390-ccw: provide entry selection on LOADPARM for SCSI disk
  pc-bios/s390-ccw: add boot entry selection for ECKD DASD

 hw/s390x/ipl.c|  31 ++--
 hw/s390x/ipl.h|   3 +++
 hw/s390x/s390-virtio-ccw.c|  37 +
 hw/s390x/sclp.c   |   9 +++
 include/hw/s390x/s390-virtio-ccw.h|   1 +
 pc-bios/s390-ccw.img  | Bin 26456 -> 26472 bytes
 pc-bios/s390-ccw/Makefile |   2 +-
 pc-bios/s390-ccw/bootmap.c|  34 +-
 pc-bios/s390-ccw/bootmap.h|  24 +--
 pc-bios/s390-ccw/main.c   |  38 +-
 pc-bios/s390-ccw/s390-ccw.h   |  17 -
 pc-bios/s390-ccw/{sclp-ascii.c => sclp.c} |  12 ++
 pc-bios/s390-ccw/sclp.h   |   2 ++
 util/qemu-config.c|   6 +
 14 files changed, 177 insertions(+), 39 deletions(-)
 rename pc-bios/s390-ccw/{sclp-ascii.c => sclp.c} (87%)

-- 
2.11.0




[Qemu-devel] [PATCH 01/11] hw/s390x: provide loadparm property for the machine

2017-04-25 Thread Cornelia Huck
From: Farhan Ali 

In order to specify the LOADPARM value one may now add ",loadparm=xxx"
parameter to the "-machine s390-ccw-virtio" option.

The property setter will normalize and check the value provided much
like the way the HMC does.

The value is stored, but not used at the moment.

Initial patch from Eugene (jno) Dvurechenski.

Signed-off-by: Eugene (jno) Dvurechenski 
Signed-off-by: Farhan Ali 
Reviewed-by: Christian Borntraeger 
Signed-off-by: Cornelia Huck 
---
 hw/s390x/s390-virtio-ccw.c | 37 +
 include/hw/s390x/s390-virtio-ccw.h |  1 +
 2 files changed, 38 insertions(+)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 04bd0ebe40..fdd4384ff0 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -274,6 +274,36 @@ bool cpu_model_allowed(void)
 return true;
 }
 
+static char *machine_get_loadparm(Object *obj, Error **errp)
+{
+S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
+
+return g_memdup(ms->loadparm, sizeof(ms->loadparm));
+}
+
+static void machine_set_loadparm(Object *obj, const char *val, Error **errp)
+{
+S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
+int i;
+
+for (i = 0; i < sizeof(ms->loadparm) && val[i]; i++) {
+uint8_t c = toupper(val[i]); /* mimic HMC */
+
+if (('A' <= c && c <= 'Z') || ('0' <= c && c <= '9') || (c == '.') ||
+(c == ' ')) {
+ms->loadparm[i] = c;
+} else {
+error_setg(errp, "LOADPARM: invalid character '%c' (ASCII 0x%02x)",
+   c, c);
+return;
+}
+}
+
+for (; i < sizeof(ms->loadparm); i++) {
+ms->loadparm[i] = ' '; /* pad right with spaces */
+}
+}
+
 static inline void s390_machine_initfn(Object *obj)
 {
 object_property_add_bool(obj, "aes-key-wrap",
@@ -291,6 +321,13 @@ static inline void s390_machine_initfn(Object *obj)
 "enable/disable DEA key wrapping using the CPACF wrapping key",
 NULL);
 object_property_set_bool(obj, true, "dea-key-wrap", NULL);
+object_property_add_str(obj, "loadparm",
+machine_get_loadparm, machine_set_loadparm, NULL);
+object_property_set_description(obj, "loadparm",
+"Up to 8 chars in set of [A-Za-z0-9. ] (lower case chars converted"
+" to upper case) to pass to machine loader, boot manager,"
+" and guest kernel",
+NULL);
 }
 
 static const TypeInfo ccw_machine_info = {
diff --git a/include/hw/s390x/s390-virtio-ccw.h 
b/include/hw/s390x/s390-virtio-ccw.h
index 6ecae00386..7b8a3e4d74 100644
--- a/include/hw/s390x/s390-virtio-ccw.h
+++ b/include/hw/s390x/s390-virtio-ccw.h
@@ -28,6 +28,7 @@ typedef struct S390CcwMachineState {
 /*< public >*/
 bool aes_key_wrap;
 bool dea_key_wrap;
+uint8_t loadparm[8];
 } S390CcwMachineState;
 
 typedef struct S390CcwMachineClass {
-- 
2.11.0




[Qemu-devel] [PATCH 06/11] pc-bios/s390-ccw: get LOADPARM stored in SCP Read Info

2017-04-25 Thread Cornelia Huck
From: Farhan Ali 

Obtain the loadparm value stored in SCP Read Info by performing
a SCLP Read Info request.

Rename sclp-ascii.c to sclp.c to reflect the changed scope of
the file.

Signed-off-by: Farhan Ali 
Reviewed-by: Christian Borntraeger 
Signed-off-by: Cornelia Huck 
---
 pc-bios/s390-ccw/Makefile |  2 +-
 pc-bios/s390-ccw/s390-ccw.h   |  3 ++-
 pc-bios/s390-ccw/{sclp-ascii.c => sclp.c} | 12 
 pc-bios/s390-ccw/sclp.h   |  2 ++
 4 files changed, 17 insertions(+), 2 deletions(-)
 rename pc-bios/s390-ccw/{sclp-ascii.c => sclp.c} (87%)

diff --git a/pc-bios/s390-ccw/Makefile b/pc-bios/s390-ccw/Makefile
index 0339c24789..79a46b6735 100644
--- a/pc-bios/s390-ccw/Makefile
+++ b/pc-bios/s390-ccw/Makefile
@@ -9,7 +9,7 @@ $(call set-vpath, $(SRC_PATH)/pc-bios/s390-ccw)
 
 .PHONY : all clean build-all
 
-OBJECTS = start.o main.o bootmap.o sclp-ascii.o virtio.o virtio-scsi.o
+OBJECTS = start.o main.o bootmap.o sclp.o virtio.o virtio-scsi.o
 QEMU_CFLAGS := $(filter -W%, $(QEMU_CFLAGS))
 QEMU_CFLAGS += -ffreestanding -fno-delete-null-pointer-checks -msoft-float
 QEMU_CFLAGS += -march=z900 -fPIE -fno-strict-aliasing
diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index 4e0aab27d3..903d2ce816 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -63,9 +63,10 @@ void panic(const char *string);
 void write_subsystem_identification(void);
 extern char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE)));
 
-/* sclp-ascii.c */
+/* sclp.c */
 void sclp_print(const char *string);
 void sclp_setup(void);
+void sclp_get_loadparm_ascii(char *loadparm);
 
 /* virtio.c */
 unsigned long virtio_load_direct(ulong rec_list1, ulong rec_list2,
diff --git a/pc-bios/s390-ccw/sclp-ascii.c b/pc-bios/s390-ccw/sclp.c
similarity index 87%
rename from pc-bios/s390-ccw/sclp-ascii.c
rename to pc-bios/s390-ccw/sclp.c
index dc1c3e4f4d..a1639baed7 100644
--- a/pc-bios/s390-ccw/sclp-ascii.c
+++ b/pc-bios/s390-ccw/sclp.c
@@ -80,3 +80,15 @@ void sclp_print(const char *str)
 
 sclp_service_call(SCLP_CMD_WRITE_EVENT_DATA, sccb);
 }
+
+void sclp_get_loadparm_ascii(char *loadparm)
+{
+
+ReadInfo *sccb = (void *)_sccb;
+
+memset((char *)_sccb, 0, sizeof(ReadInfo));
+sccb->h.length = sizeof(ReadInfo);
+if (!sclp_service_call(SCLP_CMDW_READ_SCP_INFO, sccb)) {
+ebcdic_to_ascii((char *) sccb->loadparm, loadparm, 8);
+}
+}
diff --git a/pc-bios/s390-ccw/sclp.h b/pc-bios/s390-ccw/sclp.h
index 3cbfb78930..0dd987ff5d 100644
--- a/pc-bios/s390-ccw/sclp.h
+++ b/pc-bios/s390-ccw/sclp.h
@@ -55,6 +55,8 @@ typedef struct ReadInfo {
 SCCBHeader h;
 uint16_t rnmax;
 uint8_t rnsize;
+uint8_t reserved[13];
+uint8_t loadparm[8];
 } __attribute__((packed)) ReadInfo;
 
 typedef struct SCCB {
-- 
2.11.0




[Qemu-devel] [PATCH 05/11] pc-bios/s390-ccw: Make ebcdic/ascii conversion public

2017-04-25 Thread Cornelia Huck
From: "Eugene (jno) Dvurechenski" 

Make the ebcdic_to_ascii function public to the rest of the
"bios" code, as the volume label is no more the single thing
to be converted.

Signed-off-by: Eugene (jno) Dvurechenski 
Signed-off-by: Farhan Ali 
Reviewed-by: Christian Borntraeger 
Signed-off-by: Cornelia Huck 
---
 pc-bios/s390-ccw/bootmap.h  | 22 --
 pc-bios/s390-ccw/main.c | 11 +++
 pc-bios/s390-ccw/s390-ccw.h | 13 +
 3 files changed, 24 insertions(+), 22 deletions(-)

diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
index bea168714b..9073de2238 100644
--- a/pc-bios/s390-ccw/bootmap.h
+++ b/pc-bios/s390-ccw/bootmap.h
@@ -264,28 +264,6 @@ typedef enum {
 
 /* utility code below */
 
-static const unsigned char ebc2asc[256] =
-  /* 0123456789abcdef0123456789abcdef */
-"" /* 1F */
-"" /* 3F */
-" ...<(+|&.!$*);." /* 5F first.chr.here.is.real.space 
*/
-"-/.,%_>?.`:#@'=\""/* 7F */
-".abcdefghi...jklmnopqr.." /* 9F */
-"..stuvwxyz.." /* BF */
-".ABCDEFGHI...JKLMNOPQR.." /* DF */
-"..STUVWXYZ..0123456789..";/* FF */
-
-static inline void ebcdic_to_ascii(const char *src,
-   char *dst,
-   unsigned int size)
-{
-unsigned int i;
-for (i = 0; i < size; i++) {
-unsigned c = src[i];
-dst[i] = ebc2asc[c];
-}
-}
-
 static inline void print_volser(const void *volser)
 {
 char ascii[8];
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 0946766d86..393d732353 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -15,6 +15,17 @@ char stack[PAGE_SIZE * 8] 
__attribute__((__aligned__(PAGE_SIZE)));
 static SubChannelId blk_schid = { .one = 1 };
 IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE)));
 
+const unsigned char ebc2asc[256] =
+  /* 0123456789abcdef0123456789abcdef */
+"" /* 1F */
+"" /* 3F */
+" ...<(+|&.!$*);." /* 5F first.chr.here.is.real.space 
*/
+"-/.,%_>?.`:#@'=\""/* 7F */
+".abcdefghi...jklmnopqr.." /* 9F */
+"..stuvwxyz.." /* BF */
+".ABCDEFGHI...JKLMNOPQR.." /* DF */
+"..STUVWXYZ..0123456789..";/* FF */
+
 /*
  * Priniciples of Operations (SA22-7832-09) chapter 17 requires that
  * a subsystem-identification is at 184-187 and bytes 188-191 are zero
diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index ded67bcbc6..4e0aab27d3 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -189,4 +189,17 @@ static inline void IPL_check(bool term, const char 
*message)
 }
 }
 
+extern const unsigned char ebc2asc[256];
+static inline void ebcdic_to_ascii(const char *src,
+   char *dst,
+   unsigned int size)
+{
+unsigned int i;
+
+for (i = 0; i < size; i++) {
+unsigned c = src[i];
+dst[i] = ebc2asc[c];
+}
+}
+
 #endif /* S390_CCW_H */
-- 
2.11.0




[Qemu-devel] [PATCH 07/11] pc-bios/s390-ccw: provide a function to interpret LOADPARM value

2017-04-25 Thread Cornelia Huck
From: Farhan Ali 

The LOADPARM value is fetched from SCP Read Info, but it's applied
only at the phase of bootmap interpretation. So let's read the LOARPARM
value and store it. Also provide a parsing function to detect numbers in
the LOADPARM which can be used during bootmap interpretation.

Remove a stray whitespace.

Initial patch from Eugene (jno) Dvurechenski.

Signed-off-by: Eugene (jno) Dvurechenski 
Signed-off-by: Farhan Ali 
Signed-off-by: Cornelia Huck 
---
 pc-bios/s390-ccw/main.c | 27 ++-
 pc-bios/s390-ccw/s390-ccw.h |  1 +
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 393d732353..1cacc1b46f 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -14,6 +14,7 @@
 char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE)));
 static SubChannelId blk_schid = { .one = 1 };
 IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE)));
+static char loadparm[8] = { 0, 0, 0, 0, 0, 0, 0, 0 };
 
 const unsigned char ebc2asc[256] =
   /* 0123456789abcdef0123456789abcdef */
@@ -40,7 +41,6 @@ void write_subsystem_identification(void)
 *zeroes = 0;
 }
 
-
 void panic(const char *string)
 {
 sclp_print(string);
@@ -48,6 +48,26 @@ void panic(const char *string)
 while (1) { }
 }
 
+unsigned int get_loadparm_index(void)
+{
+const char *lp = loadparm;
+int i;
+unsigned int idx = 0;
+
+for (i = 0; i < 8; i++) {
+char c = lp[i];
+
+if (c < '0' || c > '9') {
+break;
+}
+
+idx *= 10;
+idx += c - '0';
+}
+
+return idx;
+}
+
 static bool find_dev(Schib *schib, int dev_no)
 {
 int i, r;
@@ -84,6 +104,7 @@ static void virtio_setup(void)
 int ssid;
 bool found = false;
 uint16_t dev_no;
+char ldp[] = "LOADPARM=[]\n";
 VDev *vdev = virtio_get_device();
 
 /*
@@ -93,6 +114,10 @@ static void virtio_setup(void)
  */
 enable_mss_facility();
 
+sclp_get_loadparm_ascii(loadparm);
+memcpy(ldp + 10, loadparm, 8);
+sclp_print(ldp);
+
 if (store_iplb(&iplb)) {
 switch (iplb.pbt) {
 case S390_IPL_TYPE_CCW:
diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index 903d2ce816..07d8cbcb20 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -62,6 +62,7 @@ void consume_sclp_int(void);
 void panic(const char *string);
 void write_subsystem_identification(void);
 extern char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE)));
+unsigned int get_loadparm_index(void);
 
 /* sclp.c */
 void sclp_print(const char *string);
-- 
2.11.0




[Qemu-devel] [PATCH 04/11] util/qemu-config: Add loadparm to qemu machine_opts

2017-04-25 Thread Cornelia Huck
From: Farhan Ali 

Add S390CcwMachineState machine parameter "loadparm" to qemu machine_opts so
libvirt can query for it.

Signed-off-by: Farhan Ali 
Signed-off-by: Cornelia Huck 
---
 util/qemu-config.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/util/qemu-config.c b/util/qemu-config.c
index 5527100a01..405dd1a1d7 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -227,6 +227,12 @@ static QemuOptsList machine_opts = {
 .name = "dea-key-wrap",
 .type = QEMU_OPT_BOOL,
 .help = "enable/disable DEA key wrapping using the CPACF wrapping 
key",
+},{
+.name = "loadparm",
+.type = QEMU_OPT_STRING,
+.help = "Up to 8 chars in set of [A-Za-z0-9. ](lower case chars"
+" converted to upper case) to pass to machine"
+" loader, boot manager, and guest kernel",
 },
 { /* End of list */ }
 }
-- 
2.11.0




[Qemu-devel] [PATCH 02/11] hw/s390x/ipl: enable LOADPARM in IPIB for a boot device

2017-04-25 Thread Cornelia Huck
From: Farhan Ali 

Insert the LOADPARM value to the IPL Information Parameter Block.

An IPL Information Parameter Block is created when "bootindex" is
specified for a device. If a user specifies "loadparm=", then we
store the loadparm value in the created IPIB for that boot device.

Initial patch from Eugene (jno) Dvurechenski.

Signed-off-by: Eugene (jno) Dvurechenski 
Signed-off-by: Farhan Ali 
Reviewed-by: Christian Borntraeger 
Signed-off-by: Cornelia Huck 
---
 hw/s390x/ipl.c | 31 +--
 hw/s390x/ipl.h |  3 +++
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 7978c7d52a..0711ee927a 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -17,8 +17,10 @@
 #include "cpu.h"
 #include "elf.h"
 #include "hw/loader.h"
+#include "hw/boards.h"
 #include "hw/s390x/virtio-ccw.h"
 #include "hw/s390x/css.h"
+#include "hw/s390x/ebcdic.h"
 #include "ipl.h"
 #include "qemu/error-report.h"
 
@@ -243,7 +245,6 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
 ipl->iplb.pbt = S390_IPL_TYPE_CCW;
 ipl->iplb.ccw.devno = cpu_to_be16(ccw_dev->sch->devno);
 ipl->iplb.ccw.ssid = ccw_dev->sch->ssid & 3;
-return true;
 } else if (sd) {
 SCSIBus *bus = scsi_bus_from_device(sd);
 VirtIOSCSI *vdev = container_of(bus, VirtIOSCSI, bus);
@@ -259,13 +260,39 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
 ipl->iplb.scsi.channel = cpu_to_be16(sd->channel);
 ipl->iplb.scsi.devno = cpu_to_be16(ccw_dev->sch->devno);
 ipl->iplb.scsi.ssid = ccw_dev->sch->ssid & 3;
-return true;
+} else {
+return false; /* unknown device */
+}
+
+if (!s390_ipl_set_loadparm(ipl->iplb.loadparm)) {
+ipl->iplb.flags |= DIAG308_FLAGS_LP_VALID;
 }
+return true;
 }
 
 return false;
 }
 
+int s390_ipl_set_loadparm(uint8_t *loadparm)
+{
+MachineState *machine = MACHINE(qdev_get_machine());
+char *lp = object_property_get_str(OBJECT(machine), "loadparm", NULL);
+
+if (lp) {
+int i;
+
+/* lp is an uppercase string without leading/embedded spaces */
+for (i = 0; i < 8 && lp[i]; i++) {
+loadparm[i] = ascii2ebcdic[(uint8_t) lp[i]];
+}
+
+g_free(lp);
+return 0;
+}
+
+return -1;
+}
+
 static int load_netboot_image(Error **errp)
 {
 S390IPLState *ipl = get_ipl_device();
diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
index 46930e4c64..8a705e0428 100644
--- a/hw/s390x/ipl.h
+++ b/hw/s390x/ipl.h
@@ -57,6 +57,8 @@ struct IplBlockQemuScsi {
 } QEMU_PACKED;
 typedef struct IplBlockQemuScsi IplBlockQemuScsi;
 
+#define DIAG308_FLAGS_LP_VALID 0x80
+
 union IplParameterBlock {
 struct {
 uint32_t len;
@@ -82,6 +84,7 @@ union IplParameterBlock {
 } QEMU_PACKED;
 typedef union IplParameterBlock IplParameterBlock;
 
+int s390_ipl_set_loadparm(uint8_t *loadparm);
 void s390_ipl_update_diag308(IplParameterBlock *iplb);
 void s390_ipl_prepare_cpu(S390CPU *cpu);
 IplParameterBlock *s390_ipl_get_iplb(void);
-- 
2.11.0




[Qemu-devel] [PATCH 08/11] pc-bios/s390-ccw: provide entry selection on LOADPARM for SCSI disk

2017-04-25 Thread Cornelia Huck
From: Farhan Ali 

Fix SCSI bootmap interpreter to make use of any specified entry of the
Program Table using the leftmost numeric value from the LOADPARM, if specified.

Initial patch from Eugene (jno) Dvurechenski.

Signed-off-by: Eugene (jno) Dvurechenski 
Signed-off-by: Farhan Ali 
Reviewed-by: Christian Borntraeger 
Signed-off-by: Cornelia Huck 
---
 pc-bios/s390-ccw/bootmap.c | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index b21c877b53..e39e67e07b 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -444,7 +444,8 @@ static void ipl_scsi(void)
 uint8_t *ns, *ns_end;
 int program_table_entries = 0;
 const int pte_len = sizeof(ScsiBlockPtr);
-ScsiBlockPtr *prog_table_entry;
+ScsiBlockPtr *prog_table_entry = NULL;
+unsigned int loadparm = get_loadparm_index();
 
 /* Grab the MBR */
 memset(sec, FREE_SPACE_FILLER, sizeof(sec));
@@ -467,6 +468,7 @@ static void ipl_scsi(void)
 
 IPL_assert(magic_match(sec, ZIPL_MAGIC), "No zIPL magic in PT");
 
+debug_print_int("loadparm index", loadparm);
 ns_end = sec + virtio_get_block_size();
 for (ns = (sec + pte_len); (ns + pte_len) < ns_end; ns += pte_len) {
 prog_table_entry = (ScsiBlockPtr *)ns;
@@ -475,16 +477,15 @@ static void ipl_scsi(void)
 }
 
 program_table_entries++;
+if (program_table_entries == loadparm + 1) {
+break; /* selected entry found */
+}
 }
 
 debug_print_int("program table entries", program_table_entries);
 
 IPL_assert(program_table_entries != 0, "Empty Program Table");
 
-/* Run the default entry */
-
-prog_table_entry = (ScsiBlockPtr *)(sec + pte_len);
-
 zipl_run(prog_table_entry); /* no return */
 }
 
-- 
2.11.0




[Qemu-devel] [PATCH 11/11] pc-bios/s390-ccw.img: update image

2017-04-25 Thread Cornelia Huck
Contains the following commits:

- pc-bios/s390-ccw: Make ebcdic/ascii conversion public
- pc-bios/s390-ccw: get LOADPARM stored in SCP Read Info
- pc-bios/s390-ccw: provide a function to interpret LOADPARM value
- pc-bios/s390-ccw: provide entry selection on LOADPARM for SCSI disk
- pc-bios/s390-ccw: add boot entry selection for ECKD DASD
- pc-bios/s390-ccw: add boot entry selection to El Torito routine

Signed-off-by: Cornelia Huck 
---
 pc-bios/s390-ccw.img | Bin 26456 -> 26472 bytes
 1 file changed, 0 insertions(+), 0 deletions(-)

diff --git a/pc-bios/s390-ccw.img b/pc-bios/s390-ccw.img
index 
2a4adfa654844040dbc84e90a9c2e0af56fdb9fa..0b01d49495c607b67d3f1b2359395534631deb88
 100644
GIT binary patch
delta 10797
zcmZ`<3wTu3wch8x_
zD;l!JNA=cJp~sI}8Vp`RQAdj{qQDr%qEe+Du&Kd1k%+wH+<&bzNx*yW`M$|sXYYSK
z_S$Q&{m40SROvjbG-oH-8io0I&0=ZiHqY;37nPfDi^{j18f`aU)c0ko@{7EA2h67L
zm&$`?z4D+8D35!8a93w>t>@gzC&pcTsYF(a%##1!1iV>q@kXlECJ?zved_HOA`Mk2e{fCx39gsNEt%=d
zm)>5qN~^NHLLCRE`NA}CUdYZ76YGFO#McqVw
zqOi}X@QUdbTCAR782WhQQlMFgaW|E-*cSm8A%PmJUMQkBs}{SSBNV+!Ow>0B5oiT;
zd}(9&K#ikS=o5rxr^~qL9RGkvB)XD)sDlxOvM{P3Kl@1*cpur4{TS0fAv*UTL@N~`
zN3&cPmE+&jSnhLVmurqpuWwJ2SKD)cjnCos`SxrHX^yp`nN$}4y-EuTQu%-;5=o^i
zDdZs|NhNxBAms7XiESiV{5~m9Z!+46c5Ky0vGL<;v{v?#^AV~~UQHN59`cL$pR$R!
zGxNoAkE0HzL3Dr=?+kg#2vS|;4GL2IfERsr?z};;;{8rBmW*KTI=#pE6HCjc_k^!a
zN$7}MSVP*LW>EmdUkH(PmO0)S#V;2b{t2`4_$C}DI@imfKL)}=dCIHzT>ZOzIeMFt
zBInpv3`o9(JNhL-20kkTwqkW`K%Td0{)CLc8N)Ct^d3w}enP?UDl>#Kw^HCt{uxGB
zP%xl>LNx0&MlikV)rz2SS3cl_%L+t1Ty4=agpxi;ABNTfgi6hVnDY;UX+^%Eu=^ZvFU*cVfN)bbCtB=;
z9$QL?tdx)gFV?Qly6wVh>+bqv*k_uq{T9LX3}!$r@h08S8}EtVm5WxjD^E-i2_bK*
z5Zk(v7O(q?Y2g5j
z_n_up`9(~XXVE-1LgV;LCgB-8fN{qV#UD{y++kKZj#OUVA?sq@NsGqnPY5M+D%=WL
zKFG?KkH_Aw9FbRIYqgA(%r%Q;z0dRvqO*6igegLhV-GTpjfj1XaYGsRIB>OQz;-QI7xtLLp1lV{K0zNU9OTD^>G~|8kct6QXe7mu
zKr3nolGn43A|!r|-@S&3T2L@~x5qR60n^b$XWdCgT{*=_UKrB-DbyVEEkiq)?q=v>hQ7nb
zEoS2yjJ?}c2hxGjw!PYstv49YZ{Ov}LHXS47Ke&*M`^QT5OFV_!F)c1{Bxop
z@YSP!G7U}E=lXszE#yJ1)xUjLcnQ;9{q3JIfU$H-m*}R?C`(ry+CMZ4g
z`iy3*NygtpTC+uDlYbUluk+6!o?sm9X}{qvHnO}r(0&nfFTG@*;r>?fC=V6x8A>CK
zvY=6hl6D^7A;$UY+F-nXP(^l9
z^M?2KhV2jgVj%v6{~FsckuVfv*|u3sk7UyxXVYvot*y_{@;vQPa^JnM99~s
z3hP22tZY)2Vj-J^J|V?)&A?VLAw9|GtQR%K%_5`7hrip@wlr`(rtcOu;HU5cvI}X6
z#}wSLT@K<436kNfXcWHOTKqhMrCPh%skJ7fUidM|?D|2Yje6>Sk9uuyCpzsU(|=$O
zxXA;Te^HZBE3WHJgYguU&F*rtGB4*ujQ@|KX>Npa;UE`Di67kWW%tu8_vftRK)_E5
zdC|}KoRw6Ed=O*pN?{9?W|2hxY3p@bO(NXeV5}sy>jU#+;rwiU3vjr6h=+U2UGEdSY^*h-lxGtdLwr$Zi!shZMy0Y}9OYx<0g
zdOUbJA%}r?jGpEdExTa0e=-I*)wqI6PU_Wrh&gW4HD2VU-8P0r
z_ps>UEc#Iv9fee&?6-?y*r-@BO(;!bM*<`3$y|Ljjby|~&gC}F2BaQ!w|6bHgYiun=)5V)8H&k=)?PbHH=K1Qc5ag{~>2P
z2dfu9mFt~@{9S3AFvmDSySA`JYuLI1EWrY>PyDG%k)1PhmHaF25__Cv0<)(*>TJX;yb5efKrfp?2A7+}j5$>;<^!=dWp*7T(b^D!j8LHvGF55!THUn3Bc3H_|C~{zEJ+m213A
zCx*1(JoMQu3}+Iys$Q%xV^Dq#3$DlwsrebaLm6I2Yjwy6)mCiic!M*4k!(xN9+etJ
zf~@b6Z<%_R#HX0_pK%h3V%6g)9_JUyE2(b({6Pe?pYNY1PGkWH*-5Of6HGr%v^$pi
z`RzUWYd8`$iH(#cQtUO@IWBYr>wCTA+`%08Gxu?(dDD^F#oUXwE5U|7>_dSpQdQ63
zcNokQAQg;IRVLfqnSN(IH&im{Fw;DGsV^~Ta3~nO&ph?o_N8_7$*yMPc;>ESI=;HPbyrr`|@2&L0H_9|W8TP~iA5;kt?GQ;Y)*+{?h-
z+cKtrR9roY{BE8BA;`4``MoC4iXN0O+gtK%TBg5}$Jutde<+g=aq}*=^Tu7!iNdfVC|^s@^jEw=EVhhQ^>h?t9cUjdTtoRKmce&1m{*FS
zKkuJyrNMA3wO6c@n=R6l6`qz|j?MOsPf)2o3?Be!eYoZkBtQ
z@24zm&Naf)s7B$OpTdKze8YXft_;GCv9KbVI=)-_M2@P#zMESL7iz2|5xW2jaa?&u
z9?r@dD*N7%6mjQr>_^ct2?Afwy0HJtwyo
zA=W}RTE2>0dOgY32)6GYpeDPY;k-BROM*qc*5A2Aga|sfmOA7_)Pi2gbO$q+)-;;k
zNbI1)$h;#98@V&)1m6vHDgtXFN;{0mBYx}a`Y1YOC`s(30W1@@0ci0fli+G&Gx=pm
zfSa=-r@V*5r_gIo^(nlB^;jA82S&wy4+UYC)96Hl_=F^$=f&Rg33bZ|B97kTT3)oA
zJWA*OMh5sT3)n#o#%jKjGZnePu!Q@(oIs)z`On^Lh44+>=c?Cp+MMX+!#Hx%?Ue_9
zm`%v2jD9~lI+^z{%lR?W2bmsCrkwlDLikblN3x9r&V{1A4EPVGcQcJh19%Io&82#4
zx?V1n(ihr9`fNl5pZl<)IgZc%G%{}x#(5}y63e5bbZoQ0!rzYL`&rbr
z@TG>+Pf8q*u)@1p;drKbDN1Hno}(=avI5Nb4t}9DA(Brs>K3NYF#SWO-((t>jYb{4
zJNQ0I@O@PBzqpO}Vw7AVFDo>b9mwuR-z;N56;RVRjY#%in%6&c%jImq{c!Jv+-wE_t{83o7n;tSWm)bBwQiAIpPQ%F&6AJj7Ptn_GH)q+8Ax8;a2d?i{QdWd
z$=@KqY3QkWrwH5Y;_+70B0NHm?5VLM(Yy`~$EKk51x+NqkSE%39LdyQHm;J<-LF=+
z1qDtQ>D2c@7y2_MU9M8y+JA>iQ8dpe^NH!Dt@72>m`qG^#1JeA(nIspfBxpAX)x=4t!ZO7*pt9?4LKE>jKSZ>BZ04ZLzmSg)EzWbK68_(_Fl=A-n=tTgPDs$2+av!y
z^uEMxW`e>*=d0G-Kgf;4!nT2KGAvhqGQ2=OHhikBlU}v&mGzV{zsd`a+y=fV|ugavGNm~*i%6GoS|eD
z+XHiQb`Zyh{ba~a|_I&#?@m!&eWrO)2jO@Q-
zj_0-rt?!tN>kkSZL~{0ISvzK8&Tm-{dnMk(^nIi=7OMlg`a`*I%t+e>T0z#nC(n*4
zRM#GmuA-Z5%LA=KnKnnRDcYb;{YaiJ@{j$Jq{e6Jm#>ET;7#|c0lt~wBe!inYfbHN
zgimM!;jc_>l#hyknqOC
zcvQzdE$0+xDNo8Z#YIUmTM-e@lGV{XSl?WcyNfgZ-+Za385csiZCI4{tGJJDFnY*@
zOx`-g%!#bf{_gVLbv)`T>hw470lf=@<-&h50+<2AN}mV|-1pkK^-WW1^R-f;I)>MU
zPC3KT77KHp?vS&_jaIW_<-Oy!#~nn?NU}R>X?yw&E+xIB({v8bc}mIi$p1leZp0SJ
z+2b`ecCfr-{1Dq2a&pO0`Rnm>29#{YWdPiX#H*B4$e!^Ydr2K;tukqk95JCtEy<9z
z6P{JJNbAH|apy`s;Z91?*446R;{4%r$ntadn)e}0H;}txu(L+M9ERjJ%yCU96Hm$y
zCl;y`kF`viYLDUA{M;dDPAXH=@01Tt8mvz0mU}0y%sEAjU%yF0>^v*KKEVDTLiJ}J
zm6Im#PM`Q;sI?n&KGcdsegSmgE`}NIk!s1vmrPcklRqob
z>~=QoYaxGEqATrk?vx?QF?sitTWgff#MjMP$c9Zb=-WuD^)S(=Ct_XRQdJvt>UFpT
zZ@Bs^yg$k9LB{e@Z=1^W46f&;zI%!sF*PIZZukv%r
zRLyT)K?1^`k$~uXnC44JYZ=q;mus!+tNDB8>IbiI7PHRf`oAz9vl+Z^u<{X1zk*G;
zb&n6|1zd|g4iIgO=EG=s7iOR`

[Qemu-devel] [PATCH 09/11] pc-bios/s390-ccw: add boot entry selection for ECKD DASD

2017-04-25 Thread Cornelia Huck
From: Farhan Ali 

1. change a bit definition of ScsiMbr to allow an array of pointers
2. add loadparm fetch to boot script processing
3. apply loadparm index to boot entry selection, if any

Initial patch from Eugene (jno) Dvurechenski.

Signed-off-by: Eugene (jno) Dvurechenski 
Signed-off-by: Farhan Ali 
Reviewed-by: Christian Borntraeger 
Signed-off-by: Cornelia Huck 
---
 pc-bios/s390-ccw/bootmap.c | 16 +++-
 pc-bios/s390-ccw/bootmap.h |  2 +-
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index e39e67e07b..e974350b6f 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -183,15 +183,21 @@ static block_number_t load_eckd_segments(block_number_t 
blk, uint64_t *address)
 static void run_eckd_boot_script(block_number_t mbr_block_nr)
 {
 int i;
+unsigned int loadparm = get_loadparm_index();
 block_number_t block_nr;
 uint64_t address;
-ScsiMbr *scsi_mbr = (void *)sec;
+ScsiMbr *bte = (void *)sec; /* Eckd bootmap table entry */
 BootMapScript *bms = (void *)sec;
 
+debug_print_int("loadparm", loadparm);
+IPL_assert(loadparm < 31, "loadparm value greater than"
+   " maximum number of boot entries allowed");
+
 memset(sec, FREE_SPACE_FILLER, sizeof(sec));
 read_block(mbr_block_nr, sec, "Cannot read MBR");
 
-block_nr = eckd_block_num((void *)&(scsi_mbr->blockptr));
+block_nr = eckd_block_num((void *)&(bte->blockptr[loadparm]));
+IPL_assert(block_nr != -1, "No Boot Map");
 
 memset(sec, FREE_SPACE_FILLER, sizeof(sec));
 read_block(block_nr, sec, "Cannot read Boot Map Script");
@@ -459,11 +465,11 @@ static void ipl_scsi(void)
 debug_print_int("MBR Version", mbr->version_id);
 IPL_check(mbr->version_id == 1,
   "Unknown MBR layout version, assuming version 1");
-debug_print_int("program table", mbr->blockptr.blockno);
-IPL_assert(mbr->blockptr.blockno, "No Program Table");
+debug_print_int("program table", mbr->blockptr[0].blockno);
+IPL_assert(mbr->blockptr[0].blockno, "No Program Table");
 
 /* Parse the program table */
-read_block(mbr->blockptr.blockno, sec,
+read_block(mbr->blockptr[0].blockno, sec,
"Error reading Program Table");
 
 IPL_assert(magic_match(sec, ZIPL_MAGIC), "No zIPL magic in PT");
diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
index 9073de2238..7f367820f3 100644
--- a/pc-bios/s390-ccw/bootmap.h
+++ b/pc-bios/s390-ccw/bootmap.h
@@ -70,7 +70,7 @@ typedef struct ScsiMbr {
 uint8_t magic[4];
 uint32_t version_id;
 uint8_t reserved[8];
-ScsiBlockPtr blockptr;
+ScsiBlockPtr blockptr[];
 } __attribute__ ((packed)) ScsiMbr;
 
 #define ZIPL_MAGIC  "zIPL"
-- 
2.11.0




[Qemu-devel] [PATCH 10/11] pc-bios/s390-ccw: add boot entry selection to El Torito routine

2017-04-25 Thread Cornelia Huck
From: "Eugene (jno) Dvurechenski" 

If there is no LOADPARM given or '0' specified, then IPL the first
matched entry. Otherwise IPL the matching entry of that number.

Signed-off-by: Eugene (jno) Dvurechenski 
Signed-off-by: Farhan Ali 
Reviewed-by: Christian Borntraeger 
Signed-off-by: Cornelia Huck 
---
 pc-bios/s390-ccw/bootmap.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index e974350b6f..523fa78c5f 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -655,6 +655,7 @@ static IsoBcSection *find_iso_bc_entry(void)
 IsoBcEntry *e = (IsoBcEntry *)sec;
 uint32_t offset = find_iso_bc();
 int i;
+unsigned int loadparm = get_loadparm_index();
 
 if (!offset) {
 return NULL;
@@ -675,7 +676,11 @@ static IsoBcSection *find_iso_bc_entry(void)
 for (i = 1; i < ISO_BC_ENTRY_PER_SECTOR; i++) {
 if (e[i].id == ISO_BC_BOOTABLE_SECTION) {
 if (is_iso_bc_entry_compatible(&e[i].body.sect)) {
-return &e[i].body.sect;
+if (loadparm <= 1) {
+/* found, default, or unspecified */
+return &e[i].body.sect;
+}
+loadparm--;
 }
 }
 }
-- 
2.11.0




Re: [Qemu-devel] [PATCH 4/6] migration: calculate downtime on dst side

2017-04-25 Thread Alexey Perevalov

On 04/25/2017 01:25 PM, Peter Xu wrote:

On Tue, Apr 25, 2017 at 01:10:30PM +0300, Alexey Perevalov wrote:

On 04/25/2017 11:24 AM, Peter Xu wrote:

On Fri, Apr 14, 2017 at 04:17:18PM +0300, Alexey Perevalov wrote:

[...]


+/*
+ * This function calculates downtime per cpu and trace it
+ *
+ *  Also it calculates total downtime as an interval's overlap,
+ *  for many vCPU.
+ *
+ *  The approach is following:
+ *  Initially intervals are represented in tree where key is
+ *  pagefault address, and values:
+ *   begin - page fault time
+ *   end   - page load time
+ *   cpus  - bit mask shows affected cpus
+ *
+ *  To calculate overlap on all cpus, intervals converted into
+ *  array of points in time (downtime_points), the size of
+ *  array is 2 * number of nodes in tree of intervals (2 array
+ *  elements per one in element of interval).
+ *  Each element is marked as end (E) or as start (S) of interval.
+ *  The overlap downtime will be calculated for SE, only in case
+ *  there is sequence S(0..N)E(M) for every vCPU.
+ *
+ * As example we have 3 CPU
+ *
+ *  S1E1   S1   E1
+ * -***xxx***> CPU1
+ *
+ * S2E2
+ * xxx---> CPU2
+ *
+ * S3E3
+ * xxx---> CPU3
+ *
+ * We have sequence S1,S2,E1,S3,S1,E2,E3,E1
+ * S2,E1 - doesn't match condition due to sequence S1,S2,E1 doesn't include 
CPU3
+ * S3,S1,E2 - sequenece includes all CPUs, in this case overlap will be S1,E2
+ * Legend of picture is following: * - means downtime per vCPU
+ * x - means overlapped downtime
+ */

Not sure whether I get the point in this patch... iiuc we defined the
downtime here as the period when all vcpus are halted, right?

If so, I have a few questions:

- will this algorithm consume lots of memory? since I see we have one
   trace object per fault page address

I don't think, it consumes too much, one DowntimeDuration
takes (if I'm using bitmap_try_new, in this patch set I used pointer to
uint64_t array to keep bitmap array,
but I'm going to use include/qemu/bitmap.h, it works with pointers to long)

(2* int64 + (ROUND_UP((smp_cpus + BITS_PER_BYTE * sizeof(long) - 1 /
(BITS_PER_BYTE * sizeof(long * siezof(long)
so it's about 16 + at least 4 bytes, per page fault,
Lets assume we migration 256 vCPU and 256 Gb of ram and that ram is based on
4Kb pages - it's really bad case
16 + ((256 + 8 * 4 - 1) / ( 8 * 4 )) * 4 = 52 bytes
(256 * 1024 * 1024 * 1024)/(4 * 1024) = 67108864 page faults, but not all of
these pages will be pagefaulted, due to
page pre-fetching
67108864 * 52 = 3489660928 bytes (3.5 Gb for that operation),
but I have a doubt, who will use 4Kb pages for 256 Gb, probably
2Mb or 1G huge page will be chosen on x86, on ARM or other architecture it
could be another values.

Hmm, it looks still big though...


- do we need to protect the tree to make sure there's no insertion
   when doing the calculation?

I asked the same question when sent RFC patches,
the answer here is no, we should not, due to right now,
it's only one socket and one listen thread (maybe in future,
it will be required, maybe after multi fd patch set),
and calculation is doing synchronously right after migration complete.

Okay.


- if the only thing we want here is the "total downtime", whether
   below would work? (assuming N is vcpu numbers)

   a. define array cpu_fault_addr[N], to store current faulted address
  for each vcpu. When vcpu X is running, cpu_fault_addr[X] should
  be 0.

   b. when page fault happens on vcpu A, setup cpu_fault_addr[A] with
  corresponding fault address.

at this time need to is fault happens for all another vCPU,
and if it happens mark current time as total vCPU downtime start.


   c. when page copy finished, loop over cpu_fault_addr[] to see
  whether that matches any, clear corresponding element if matched.

so when page copy finished and mark for total vCPU is set,
yes that interval is a part of total downtime.

   Then, we can just measure the period when cpu_fault_addr[] is all
   set (by tracing at both b. and c.). Can this work?

Yes, it works, but it's better to keep time - cpu_fault_time,
address is not important here, it doesn't matter the reason of pagefault.

We still need the addresses? So that when we do COPY, we can check the
new page address against these stored ones, to know which vcpus to
clear the bit.

Frankly say, we need ) because there is not another way to determine
vCPU at COPY time.




2 vCPU could fault due to access to one page, ok, it's not a problem, just
store
time when it was faulted.
Looks like it's better algorithm, with lesser complexity,
thank you a lot.

My pleasure. Thanks,




--
Best regards,
Alexey Perevalov



Re: [Qemu-devel] DMG chunk size independence

2017-04-25 Thread Ashijeet Acharya
Hi,

cc'ing Peter Wu in...

Currently I have completed the task for zlib, uncompressed and zeroed
chunks in a DMG file using the approach we discussed earlier.
Unfortunately, this approach is not appropriate for bz2 chunks since
we cannot restart our decompression from the access point we cached
since bz2 decompression checks for a special magic key 'BZh' before it
starts decompressing. Since our cached point can be pointing to any
random location inside the compressed stream and not necessarily the
start of a "block", dmg_uncompress_bz2_do() fails with an error value
BZ_DATA_ERROR_MAGIC (-5) and thus our approach fails.
This blog post here explains this limitation too ->
https://blastedbio.blogspot.in/2011/11/random-access-to-bzip2.html

Now, there is an interesting thing I found out about bz2 compressed
streams i.e. the size of a compressed block varies from 0 to a max of
900 KiB. This is guaranteed and can be verified because each block has
a 4 byte header attached to it at the beginning in which the first
three bytes are the magic key "BZh" followed by a number from 1-9.
These help us find the max size that block will have as the size
increments by 100KiB for each value (eg. BZh3 has a max of 300KiB).

Now the wikipedia page here
(https://en.wikipedia.org/wiki/Bzip2#File_format) states that a 900KiB
block can expand to a max of 46MiB in its uncompressed form. Thus we
need not worry about QEMU allocating wild sized memory at once as we
have a limit of 64MiB as of now and stick to the approach of
decompressing the whole block every time we enter it. This solves our
problem of caching an access point and ultimately failing with this
error value BZ_DATA_ERROR_MAGIC (-5).

I am hesitant in this approach because I am not sure yet that "blocks"
and "chunks" mean the same thing and are just two different
terminologies (i.e. chunks == blocks) OR chunks are made up of blocks
(i.e chunks = x * blocks).

I approached Peter Wu (who worked on DMG a few years ago) about this
and he's not sure either.

(Peter, you may skip this part as I already explained you this earlier :-) )
I did a little naive test of my own, where I downloaded one of the bz2
DMG images and tried reading it with a HEX editor.

First, I manually calculated the size between the appearance of two
sequential magic keys ('BZh') offsets which marked the length of a
block starting at the offset of first magic key. Next I compared it to
the size of the corresponding chunk whose size (s->lenghts[chunk]) we
get by reading the mish blocks and all that stuff while opening the
image in QEMU, and interestingly both the sizes appeared to be equal.
I repeated it for quite a few chunks and this test stayed valid for
all.

Peter thinks we cannot rely on this test thus I wouldn't mind more
views on it...

Ashijeet



Re: [Qemu-devel] [PATCH v2 02/13] exec-all: inline tb_from_jmp_cache

2017-04-25 Thread Richard Henderson

On 04/25/2017 09:53 AM, Emilio G. Cota wrote:

The inline improves performance, as shown in upcoming commits' logs.

This commit is kept separate to ease review, since the inclusion
of tb-hash.h might be controversial. The problem here, which was
introduced before this commit, is that tb_hash_func() depends on
page_addr_t: this defeats the original purpose of tb-hash.h,
which was to be self-contained and CPU-agnostic.

Signed-off-by: Emilio G. Cota
---
  cpu-exec.c  | 19 ---
  include/exec/exec-all.h | 24 +++-
  2 files changed, 23 insertions(+), 20 deletions(-)


Is there a reason we should just inline this code directly into 
HELPER(lookup_tb_ptr)?  I think that would save a bit of churn, and I can't 
think of any other reason we'd want to use this function.



r~



Re: [Qemu-devel] [PATCH v2 03/13] tcg: enforce 64-byte alignment of TCGContext

2017-04-25 Thread Richard Henderson

On 04/25/2017 09:53 AM, Emilio G. Cota wrote:

This will allow us to prevent cache line false sharing in TCGContext.

Before:
$ objdump -t  build/x86_64-linux-user/qemu-x86_64 | grep tcg_ctx
003ea820 g O .bss   000152d8  tcg_ctx

After:
$ objdump -t  build/x86_64-linux-user/qemu-x86_64 | grep tcg_ctx
003ea880 g O .bss   00015300  tcg_ctx

Signed-off-by: Emilio G. Cota 
---
  tcg/tcg.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tcg/tcg.h b/tcg/tcg.h
index 6c216bb..5fdbfe3 100644
--- a/tcg/tcg.h
+++ b/tcg/tcg.h
@@ -727,7 +727,7 @@ struct TCGContext {
  
  uint16_t gen_insn_end_off[TCG_MAX_INSNS];

  target_ulong gen_insn_data[TCG_MAX_INSNS][TARGET_INSN_START_WORDS];
-};
+} QEMU_ALIGNED(64);


Let's drop the alignment and structure re-arrangement for now and focus on the 
task of goto_ptr.



r~



Re: [Qemu-devel] [PATCH v2 05/13] tcg-runtime: add lookup_tb_ptr helper

2017-04-25 Thread Richard Henderson

On 04/25/2017 09:53 AM, Emilio G. Cota wrote:

Signed-off-by: Emilio G. Cota 
---
  tcg-runtime.c | 7 +++
  tcg/tcg-runtime.h | 2 ++
  tcg/tcg.h | 1 +
  3 files changed, 10 insertions(+)


Modulo what I mentioned earlier about maybe directly inlining tb_from_jmp_cache,

Reviewed-by: Richard Henderson 


r~



Re: [Qemu-devel] [PATCH RESEND v2 04/18] COLO: integrate colo compare with colo frame

2017-04-25 Thread Hailiang Zhang

On 2017/4/25 2:18, Juan Quintela wrote:

zhanghailiang  wrote:

For COLO FT, both the PVM and SVM run at the same time,
only sync the state while it needs.

So here, let SVM runs while not doing checkpoint, change
DEFAULT_MIGRATE_X_CHECKPOINT_DELAY to 200*100.

Besides, we forgot to release colo_checkpoint_semd and
colo_delay_timer, fix them here.

Cc: Jason Wang 
Signed-off-by: zhanghailiang 
Reviewed-by: Dr. David Alan Gilbert 




diff --git a/migration/migration.c b/migration/migration.c
index 353f272..2ade2aa 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -70,7 +70,7 @@
  /* The delay time (in ms) between two COLO checkpoints
   * Note: Please change this default value to 1 when we support hybrid 
mode.
   */
-#define DEFAULT_MIGRATE_X_CHECKPOINT_DELAY 200
+#define DEFAULT_MIGRATE_X_CHECKPOINT_DELAY (200 * 100)
  
  static NotifierList migration_state_notifiers =

  NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);

1000 or 200 * 100

Please, fix value or comment?


OK, will fix in next version, thanks.


Later, Juan.

.







Re: [Qemu-devel] [PATCH v2 06/13] tcg: add goto_ptr opcode

2017-04-25 Thread Richard Henderson

On 04/25/2017 09:53 AM, Emilio G. Cota wrote:

@@ -1138,6 +1138,7 @@ void tcg_dump_ops(TCGContext *s)
  }
  switch (c) {
  case INDEX_op_set_label:
+case INDEX_op_goto_tb:
  case INDEX_op_br:
  case INDEX_op_brcond_i32:
  case INDEX_op_brcond_i64:


This is wrong, and causes crashes when dumping.  Nor should goto_ptr be here, 
so I don't know what you were after.



r~



Re: [Qemu-devel] [PATCH RESEND v2 07/18] COLO: Load dirty pages into SVM's RAM cache firstly

2017-04-25 Thread Hailiang Zhang

On 2017/4/25 2:27, Juan Quintela wrote:

zhanghailiang  wrote:

We should not load PVM's state directly into SVM, because there maybe some
errors happen when SVM is receving data, which will break SVM.

We need to ensure receving all data before load the state into SVM. We use
an extra memory to cache these data (PVM's ram). The ram cache in secondary side
is initially the same as SVM/PVM's memory. And in the process of checkpoint,
we cache the dirty pages of PVM into this ram cache firstly, so this ram cache
always the same as PVM's memory at every checkpoint, then we flush this cached 
ram
to SVM after we receive all PVM's state.

Cc: Dr. David Alan Gilbert 
Signed-off-by: zhanghailiang 
Signed-off-by: Li Zhijian 
---
v2:
- Move colo_init_ram_cache() and colo_release_ram_cache() out of
   incoming thread since both of them need the global lock, if we keep
   colo_release_ram_cache() in incoming thread, there are potential
   dead-lock.
- Remove bool ram_cache_enable flag, use migration_incoming_in_state() instead.
- Remove the Reviewd-by tag because of the above changes.



+out_locked:
+QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
+if (block->colo_cache) {
+qemu_anon_ram_free(block->colo_cache, block->used_length);
+block->colo_cache = NULL;
+}
+}
+
+rcu_read_unlock();
+return -errno;
+}
+
+/* It is need to hold the global lock to call this helper */
+void colo_release_ram_cache(void)
+{
+RAMBlock *block;
+
+rcu_read_lock();
+QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
+if (block->colo_cache) {
+qemu_anon_ram_free(block->colo_cache, block->used_length);
+block->colo_cache = NULL;
+}
+}
+rcu_read_unlock();
+}

Create a function from the creation/removal?  We have exactly two copies
of the same code.  Right now the code inside the function is very small,
but it could be bigger, no?


Yes, we add more codes  in next patch (patch 08).  :)


Later, Juan.


.







Re: [Qemu-devel] [PATCH v2 08/13] target/arm: optimize cross-page block chaining in softmmu

2017-04-25 Thread Richard Henderson

On 04/25/2017 09:53 AM, Emilio G. Cota wrote:

Instead of unconditionally exiting to the exec loop, use the
lookup_tb_ptr helper to jump to the target if it is valid.
As long as the hit rate in tb_jmp_cache remains high, this
will improve performance.

Perf impact: see the next commit's log.

Signed-off-by: Emilio G. Cota 
---
  target/arm/translate.c | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/target/arm/translate.c b/target/arm/translate.c
index e32e38c..574cf70 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -4085,8 +4085,12 @@ static inline void gen_goto_tb(DisasContext *s, int n, 
target_ulong dest)
  gen_set_pc_im(s, dest);
  tcg_gen_exit_tb((uintptr_t)s->tb + n);
  } else {
+TCGv_ptr ptr = tcg_temp_new_ptr();
+
  gen_set_pc_im(s, dest);
-tcg_gen_exit_tb(0);
+gen_helper_lookup_tb_ptr(ptr, cpu_env, cpu_R[15]);
+tcg_gen_goto_ptr(ptr);
+tcg_temp_free_ptr(ptr);
  }


This does not compile for aarch64.  You need to tcg_gen_extu_i32_tl first.


r~



Re: [Qemu-devel] [PATCH v2 09/13] target/arm: optimize indirect branches with TCG's goto_ptr

2017-04-25 Thread Richard Henderson

On 04/25/2017 09:53 AM, Emilio G. Cota wrote:

+
+gen_jr = false;
+gen_helper_lookup_tb_ptr(ptr, cpu_env, cpu_R[15]);
+tcg_gen_goto_ptr(ptr);
+tcg_temp_free_ptr(ptr);
+break;


Likewise doesn't compile for aarch64.


r~



  1   2   3   4   5   >