[Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Michael S. Tsirkin]

On Tue, Mar 16, 2010 at 07:51:16PM +0100, Juan Quintela wrote:
> Hi
> 
> This series introduces several virtio cleanups:
> - add comment to pci (mst)
> - tell virtio about DO_UPCAST

I think we should move away from struct layout assumptions that
DO_UPCAST enforces, and to use container_of where possible.
I'll post a series shortly that do this for virtio.

> - use QLIST instead of one open list
> - virtio-pci/msix: remove duplicated test
> 
> Please review and apply.
> 
> This is split for a series previously sent.  Will send the vmstate
> conversions as a different series on top of this one.
> 
> Later, Juan.
> 
> Juan Quintela (8):
>   virtio: Teach virtio-balloon about DO_UPCAST
>   virtio: Teach virtio-blk about DO_UPCAST
>   virtio: Teach virtio-net about DO_UPCAST
>   virtio: Use DO_UPCAST instead of a cast
>   virtio-pci: Remove duplicate test
>   QLIST: Introduce QLIST_COPY_HEAD
>   virtio-blk: change rq type to VirtIOBlockReq
>   virtio-blk: use QLIST for the list of requests
> 
> Michael S. Tsirkin (1):
>   qemu/pci: document msix_entries_nr field
> 
>  hw/msix.c   |8 ---
>  hw/pci.h|4 ++-
>  hw/virtio-balloon.c |   15 -
>  hw/virtio-blk.c |   54 --
>  hw/virtio-net.c |   29 +++
>  hw/virtio-pci.c |7 +++--
>  qemu-queue.h|4 +++
>  7 files changed, 54 insertions(+), 67 deletions(-)
> 
> 

[Qemu-devel] qemu-dm writing garbage into PCI BAR registers in HVM guest in XEN 3.3.1 on SLES11

[Dan Gora]

Hi All,

I've found a problem in qemu-dm from xen 3.3.1_18546_12-3.1 from
SLES11.  I've posted this question to xen-devel as well, but I thought
that I'd post it here since the problem appears to be in qemu-dm.

I began investigating this problem when we found that if we pass two
instances of our board through to a single HVM domU instance that if
we load then unload our driver, after many tens of seconds (30-60 or
so) something would write garbage to one of the board's PCI BAR
registers.  After a couple of days of debugging I've isolated the
problem down to the rtl8341 in QEMU, but I'm really not sure what is
causing it.  It appears that rtl8341 is writing an ethernet packet
with write() down a file descriptor which is actually attached to
sysfs and my board's PCI configuration space registers.

With ltrace and some kernel and hypervisor printfs I was able to find
that the problem was occuring that the writes were occurring via
write() on file descriptor 6 with a length of 590 bytes.  (see the
xen-devel thread for the details on that if you're really curious..)

I finally figured out how to trap the problem in gdb with the help of
the conditional break point.  Since I know that it's a write to fd 6
of length 590 bytes I just make gdb break on that:

(gdb) disass write
Dump of assembler code for function write:
0x7f2c3afe07b0 :   cmpl   $0x0,0x20db59(%rip)#
0x7f2c3b1ee310 <__pthread_multiple_threads>
0x7f2c3afe07b7 :   jne0x7f2c3afe07c9 
0x7f2c3afe07b9 <__write_nocancel+0>:mov$0x1,%eax
0x7f2c3afe07be <__write_nocancel+5>:syscall
0x7f2c3afe07c0 <__write_nocancel+7>:cmp$0xf001,%rax
0x7f2c3afe07c6 <__write_nocancel+13>:   jae0x7f2c3afe081b

0x7f2c3afe07c8 <__write_nocancel+15>:   retq
0x7f2c3afe07c9 :  sub$0x28,%rsp
0x7f2c3afe07cd :  mov%rdi,0x8(%rsp)
0x7f2c3afe07d2 :  mov%rsi,0x10(%rsp)
0x7f2c3afe07d7 :  mov%rdx,0x18(%rsp)
0x7f2c3afe07dc :  callq  0x7f2c3afe0380
<__pthread_enable_asynccancel>
0x7f2c3afe07e1 :  mov0x8(%rsp),%rdi
0x7f2c3afe07e6 :  mov0x10(%rsp),%rsi
0x7f2c3afe07eb :  mov0x18(%rsp),%rdx
0x7f2c3afe07f0 :  mov%rax,(%rsp)
0x7f2c3afe07f4 :  mov$0x1,%eax
0x7f2c3afe07f9 :  syscall
0x7f2c3afe07fb :  mov(%rsp),%rdi
0x7f2c3afe07ff :  mov%rax,0x8(%rsp)
0x7f2c3afe0804 :  callq  0x7f2c3afe0350
<__pthread_disable_asynccancel>
0x7f2c3afe0809 :  mov0x8(%rsp),%rax
0x7f2c3afe080e :  add$0x28,%rsp
0x7f2c3afe0812 :  cmp$0xf001,%rax
0x7f2c3afe0818 : jae0x7f2c3afe081b 
0x7f2c3afe081a : retq
0x7f2c3afe081b : mov0x20977e(%rip),%rcx#
0x7f2c3b1e9fa0
0x7f2c3afe0822 : xor%edx,%edx
0x7f2c3afe0824 : sub%rax,%rdx
0x7f2c3afe0827 : mov%edx,%fs:(%rcx)
0x7f2c3afe082a : or $0x,%rax
0x7f2c3afe082e : jmp0x7f2c3afe081a 
End of assembler dump.
(gdb) break *0x7f2c3afe07f4 if $rdi == 6 && $rdx == 590
Breakpoint 1 at 0x7f2c3afe07f4
(gdb) info break
Num Type   Disp Enb AddressWhat
1   breakpoint keep y   0x7f2c3afe07f4 
   stop only if $rdi == 6 && $rdx == 590

===
Here the breakpoint fires, but the bad writes do not occur when we perform
the syscall.
===

Breakpoint 1, 0x7f2c3afe07f4 in write () from /lib64/libpthread.so.0
(gdb) bt
#0  0x7f2c3afe07f4 in write () from /lib64/libpthread.so.0
#1  0x0040746d in tap_receive (opaque=0xa940f0,
   buf=0x7fff43a7f080 "��", size=)
   at /home/dg/rpmbuild/BUILD/xen-3.3.1-testing/tools/ioemu-dir/vl.c:4079
#2  0x00407081 in qemu_send_packet (vc1=0xaf99d0,
   buf=0x7fff43a7f080 "��", size=590)
   at /home/dg/rpmbuild/BUILD/xen-3.3.1-testing/tools/ioemu-dir/vl.c:3865
#3  0x004235a0 in rtl8139_transmit_one (s=0xaf9780,
   descriptor=)
   at 
/home/dg/rpmbuild/BUILD/xen-3.3.1-testing/tools/ioemu-dir/hw/rtl8139.c:1810
#4  0x00424ae7 in rtl8139_io_writel (opaque=0x6,
   addr=, val=524878)
   at 
/home/dg/rpmbuild/BUILD/xen-3.3.1-testing/tools/ioemu-dir/hw/rtl8139.c:2367
#5  0x004726a3 in cpu_physical_memory_rw (_addr=,
   buf=0x7f2c3ba7e018 ,
   _len=, is_write=1) at exec-dm.c:510
#6  0x004a4703 in cpu_ioreq_move (env=,
   req=0x7f2c3ba7e000) at helper2.c:319
#7  0x004a4fb1 in cpu_handle_ioreq (opaque=)
   at helper2.c:508
#8  0x0040aeb4 in main_loop_wait (timeout=10)
   at /home/dg/rpmbuild/BUILD/xen-3.3.1-testing/tools/ioemu-dir/vl.c:7175
#9  0x004a4dbd in main_loop () at helper2.c:571
#10 0x0040feb3 in main (argc=20, argv=0x7fff43a82968)
   at /home/dg/rpmbuild/BUILD/xen-3.3.1-testing/tools/ioemu-dir/vl.c:8950


Here is the data buffer that is being written by rtl8139_transmit():
Notice that it looks like an ethernet frame!  But it is being written
down the sysfs pci configuration space descriptor!

[Qemu-devel] virtio_blk_load() question

[OHMURA Kei]

Hi,

I have a question regarding virtio_blk_load().
(qemu-kvm.git d1fa468c1cc03ea362d8fe3ed9269bab4d197510)

VirtIOBlockReq structure is linked list of requests, but it doesn't seem to be
properly linked in virtio_blk_load().
...
req->next = s->rq;
s->rq = req->next;
...
In this case, we're losing req, and s->rq always point to be same entry.
If I'm understanding correctly, s->rq is NULL initially,
and this would be kept.

Although I'm not sure how these requests should be ordered, if the requests
should be added to the head of list to restore the saved status by
virtio_blk_save(), I think the following code is correct.  However, it seems to
reverse the order of the requests, and I'm wondering whether that is
appropriate.

Would somebody tell me how virtio_blk_load() is working?

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index b80402d..267b16f 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -457,7 +457,7 @@ static int virtio_blk_load(QEMUFile *f, void *opaque, int 
version_id)
VirtIOBlockReq *req = virtio_blk_alloc_request(s);
qemu_get_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem));
req->next = s->rq;
-s->rq = req->next;
+s->rq = req;
}

return 0;

[Qemu-devel] [PATCH 6/8] target-alpha: Use setcond for int comparisons.

[Richard Henderson]

Signed-off-by: Richard Henderson 
---
 target-alpha/translate.c |   43 ++-
 1 files changed, 22 insertions(+), 21 deletions(-)

diff --git a/target-alpha/translate.c b/target-alpha/translate.c
index c52cac3..3fa32b3 100644
--- a/target-alpha/translate.c
+++ b/target-alpha/translate.c
@@ -1288,33 +1288,34 @@ MVIOP2(pkwb)
 MVIOP2(unpkbl)
 MVIOP2(unpkbw)
 
-static inline void gen_cmp(TCGCond cond, int ra, int rb, int rc, int islit,
-   uint8_t lit)
+static void gen_cmp(TCGCond cond, int ra, int rb, int rc,
+int islit, uint8_t lit)
 {
-int l1, l2;
-TCGv tmp;
+TCGv va, vb;
 
-if (unlikely(rc == 31))
+if (unlikely(rc == 31)) {
 return;
+}
 
-l1 = gen_new_label();
-l2 = gen_new_label();
+if (ra == 31) {
+va = tcg_const_i64(0);
+} else {
+va = cpu_ir[ra];
+}
+if (islit) {
+vb = tcg_const_i64(lit);
+} else {
+vb = cpu_ir[rb];
+}
 
-if (ra != 31) {
-tmp = tcg_temp_new();
-tcg_gen_mov_i64(tmp, cpu_ir[ra]);
-} else
-tmp = tcg_const_i64(0);
-if (islit)
-tcg_gen_brcondi_i64(cond, tmp, lit, l1);
-else
-tcg_gen_brcond_i64(cond, tmp, cpu_ir[rb], l1);
+tcg_gen_setcond_i64(cond, cpu_ir[rc], va, vb);
 
-tcg_gen_movi_i64(cpu_ir[rc], 0);
-tcg_gen_br(l2);
-gen_set_label(l1);
-tcg_gen_movi_i64(cpu_ir[rc], 1);
-gen_set_label(l2);
+if (ra == 31) {
+tcg_temp_free(va);
+}
+if (islit) {
+tcg_temp_free(vb);
+}
 }
 
 static void gen_rx(int ra, int set)
-- 
1.6.6.1

[Qemu-devel] [PATCH 8/8] target-alpha: Emit goto_tb opcodes.

[Richard Henderson]

Use an ExitStatus enumeration instead of magic numbers as the return
value from translate_one.  Emit goto_tb opcodes when ending a TB via
a direct branch.

Signed-off-by: Richard Henderson 
---
 target-alpha/translate.c |  339 ++
 1 files changed, 193 insertions(+), 146 deletions(-)

diff --git a/target-alpha/translate.c b/target-alpha/translate.c
index b845094..d312939 100644
--- a/target-alpha/translate.c
+++ b/target-alpha/translate.c
@@ -43,12 +43,13 @@
 
 typedef struct DisasContext DisasContext;
 struct DisasContext {
+struct TranslationBlock *tb;
+CPUAlphaState *env;
 uint64_t pc;
 int mem_idx;
 #if !defined (CONFIG_USER_ONLY)
 int pal_mode;
 #endif
-CPUAlphaState *env;
 uint32_t amask;
 
 /* Current rounding mode for this TB.  */
@@ -57,6 +58,25 @@ struct DisasContext {
 int tb_ftz;
 };
 
+/* Return values from translate_one, indicating the state of the TB.
+   Note that zero indicates that we are not exiting the TB.  */
+
+typedef enum {
+NO_EXIT,
+
+/* We have emitted one or more goto_tb.  No fixup required.  */
+EXIT_GOTO_TB,
+
+/* We are not using a goto_tb (for whatever reason), but have updated
+   the PC (for whatever reason), so there's no need to do it again on
+   exiting the TB.  */
+EXIT_PC_UPDATED,
+
+/* We are exiting the TB, but have neither emitted a goto_tb, nor
+   updated the PC for the next instruction to be executed.  */
+EXIT_PC_STALE
+} ExitStatus;
+
 /* global register indexes */
 static TCGv_ptr cpu_env;
 static TCGv cpu_ir[31];
@@ -300,77 +320,126 @@ static inline void gen_store_mem(DisasContext *ctx,
 tcg_temp_free(addr);
 }
 
-static void gen_bcond_pcload(DisasContext *ctx, int32_t disp, int lab_true)
+static int use_goto_tb(DisasContext *ctx, uint64_t dest)
 {
-int lab_over = gen_new_label();
+/* Check for the dest on the same page as the start of the TB.  We
+   also want to suppress goto_tb in the case of single-steping and IO.  */
+return (((ctx->tb->pc ^ dest) & TARGET_PAGE_MASK) == 0
+&& !ctx->env->singlestep_enabled
+&& !(ctx->tb->cflags & CF_LAST_IO));
+}
 
-tcg_gen_movi_i64(cpu_pc, ctx->pc);
-tcg_gen_br(lab_over);
-gen_set_label(lab_true);
-tcg_gen_movi_i64(cpu_pc, ctx->pc + (int64_t)(disp << 2));
-gen_set_label(lab_over);
+static ExitStatus gen_bdirect(DisasContext *ctx, int ra, int32_t disp)
+{
+uint64_t dest = ctx->pc + (disp << 2);
+
+if (ra != 31) {
+tcg_gen_movi_i64(cpu_ir[ra], ctx->pc);
+}
+
+/* Notice branch-to-next; used to initialize RA with the PC.  */
+if (disp == 0) {
+return 0;
+} else if (use_goto_tb(ctx, dest)) {
+tcg_gen_goto_tb(0);
+tcg_gen_movi_i64(cpu_pc, dest);
+tcg_gen_exit_tb((long)ctx->tb);
+return EXIT_GOTO_TB;
+} else {
+tcg_gen_movi_i64(cpu_pc, dest);
+return EXIT_PC_UPDATED;
+}
 }
 
-static void gen_bcond(DisasContext *ctx, TCGCond cond, int ra,
-  int32_t disp, int mask)
+static ExitStatus gen_bcond_internal(DisasContext *ctx, TCGCond cond,
+ TCGv cmp, int32_t disp)
 {
+uint64_t dest = ctx->pc + (disp << 2);
 int lab_true = gen_new_label();
 
-if (likely(ra != 31)) {
+if (use_goto_tb(ctx, dest)) {
+tcg_gen_brcondi_i64(cond, cmp, 0, lab_true);
+
+tcg_gen_goto_tb(0);
+tcg_gen_movi_i64(cpu_pc, ctx->pc);
+tcg_gen_exit_tb((long)ctx->tb);
+
+gen_set_label(lab_true);
+tcg_gen_goto_tb(1);
+tcg_gen_movi_i64(cpu_pc, dest);
+tcg_gen_exit_tb((long)ctx->tb + 1);
+
+return EXIT_GOTO_TB;
+} else {
+int lab_over = gen_new_label();
+
+/* ??? Consider using either
+ movi pc, next
+ addi tmp, pc, disp
+ movcond pc, cond, 0, tmp, pc
+   or
+ setcond tmp, cond, 0
+ movi pc, next
+ neg tmp, tmp
+ andi tmp, tmp, disp
+ add pc, pc, tmp
+   The current diamond subgraph surely isn't efficient.  */
+
+tcg_gen_brcondi_i64(cond, cmp, 0, lab_true);
+tcg_gen_movi_i64(cpu_pc, ctx->pc);
+tcg_gen_br(lab_over);
+gen_set_label(lab_true);
+tcg_gen_movi_i64(cpu_pc, dest);
+gen_set_label(lab_over);
+
+return EXIT_PC_UPDATED;
+}
+}
+
+static ExitStatus gen_bcond(DisasContext *ctx, TCGCond cond, int ra,
+int32_t disp, int mask)
+{
+TCGv cmp_tmp;
+
+if (unlikely(ra == 31)) {
+cmp_tmp = tcg_const_i64(0);
+} else {
+cmp_tmp = tcg_temp_new();
 if (mask) {
-TCGv tmp = tcg_temp_new();
-tcg_gen_andi_i64(tmp, cpu_ir[ra], 1);
-tcg_gen_brcondi_i64(cond, tmp, 0, lab_true);
-tcg_temp_free(tmp);
+tcg_gen_andi_i64(cmp_tmp, cpu_ir[ra], 1);
 } else {
-

[Qemu-devel] [PATCH 4/8] target-alpha: Implement cvtql inline.

[Richard Henderson]

It's a simple mask and shift sequence.
Also, fix a typo in the actual masks used.

Signed-off-by: Richard Henderson 
---
 target-alpha/helper.h|4 
 target-alpha/op_helper.c |   20 
 target-alpha/translate.c |   45 +++--
 3 files changed, 39 insertions(+), 30 deletions(-)

diff --git a/target-alpha/helper.h b/target-alpha/helper.h
index c378195..10c78d0 100644
--- a/target-alpha/helper.h
+++ b/target-alpha/helper.h
@@ -89,10 +89,6 @@ DEF_HELPER_FLAGS_1(cvttq, TCG_CALL_CONST, i64, i64)
 DEF_HELPER_FLAGS_1(cvttq_c, TCG_CALL_CONST, i64, i64)
 DEF_HELPER_FLAGS_1(cvttq_svic, TCG_CALL_CONST, i64, i64)
 
-DEF_HELPER_FLAGS_1(cvtql, TCG_CALL_CONST | TCG_CALL_PURE, i64, i64)
-DEF_HELPER_1(cvtql_v, i64, i64)
-DEF_HELPER_1(cvtql_sv, i64, i64)
-
 DEF_HELPER_FLAGS_1(setroundmode, TCG_CALL_CONST, void, i32)
 DEF_HELPER_FLAGS_1(setflushzero, TCG_CALL_CONST, void, i32)
 DEF_HELPER_FLAGS_0(fp_exc_clear, TCG_CALL_CONST, void)
diff --git a/target-alpha/op_helper.c b/target-alpha/op_helper.c
index 84867b8..f9cd07a 100644
--- a/target-alpha/op_helper.c
+++ b/target-alpha/op_helper.c
@@ -1159,26 +1159,6 @@ uint64_t helper_cvtlq (uint64_t a)
 return (lo & 0x3FFF) | (hi & 0xc000);
 }
 
-uint64_t helper_cvtql (uint64_t a)
-{
-return ((a & 0xC000) << 32) | ((a & 0x7FFF) << 29);
-}
-
-uint64_t helper_cvtql_v (uint64_t a)
-{
-if ((int32_t)a != (int64_t)a)
-helper_excp(EXCP_ARITH, EXC_M_IOV);
-return helper_cvtql(a);
-}
-
-uint64_t helper_cvtql_sv (uint64_t a)
-{
-/* ??? I'm pretty sure there's nothing that /sv needs to do that /v
-   doesn't do.  The only thing I can think is that /sv is a valid
-   instruction merely for completeness in the ISA.  */
-return helper_cvtql_v(a);
-}
-
 /* PALcode support special instructions */
 #if !defined (CONFIG_USER_ONLY)
 void helper_hw_rei (void)
diff --git a/target-alpha/translate.c b/target-alpha/translate.c
index 188e76c..cfdf441 100644
--- a/target-alpha/translate.c
+++ b/target-alpha/translate.c
@@ -597,6 +597,41 @@ static inline void gen_fp_exc_raise(int rc, int fn11)
 gen_fp_exc_raise_ignore(rc, fn11, fn11 & QUAL_I ? 0 : float_flag_inexact);
 }
 
+static void gen_fcvtql(int rb, int rc)
+{
+if (unlikely(rc == 31)) {
+return;
+}
+if (unlikely(rb == 31)) {
+tcg_gen_movi_i64(cpu_fir[rc], 0);
+} else {
+TCGv tmp = tcg_temp_new();
+
+tcg_gen_andi_i64(tmp, cpu_fir[rb], 0xC000);
+tcg_gen_andi_i64(cpu_fir[rc], cpu_fir[rb], 0x3FFF);
+tcg_gen_shli_i64(tmp, tmp, 32);
+tcg_gen_shli_i64(cpu_fir[rc], cpu_fir[rc], 29);
+tcg_gen_or_i64(cpu_fir[rc], cpu_fir[rc], tmp);
+
+tcg_temp_free(tmp);
+}
+}
+
+static void gen_fcvtql_v(DisasContext *ctx, int rb, int rc)
+{
+if (rb != 31) {
+int lab = gen_new_label();
+TCGv tmp = tcg_temp_new();
+
+tcg_gen_ext_i32_i64(tmp, cpu_fir[rb]);
+tcg_gen_brcond_i64(TCG_COND_EQ, tmp, cpu_fir[rb], lab);
+gen_excp(ctx, EXCP_ARITH, EXC_M_IOV);
+
+gen_set_label(lab);
+}
+gen_fcvtql(rb, rc);
+}
+
 #define FARITH2(name)   \
 static inline void glue(gen_f, name)(int rb, int rc)\
 {   \
@@ -612,9 +647,6 @@ static inline void glue(gen_f, name)(int rb, int rc)\
 }   \
 }
 FARITH2(cvtlq)
-FARITH2(cvtql)
-FARITH2(cvtql_v)
-FARITH2(cvtql_sv)
 
 /* ??? VAX instruction qualifiers ignored.  */
 FARITH2(sqrtf)
@@ -2327,11 +2359,12 @@ static inline int translate_one(DisasContext *ctx, 
uint32_t insn)
 break;
 case 0x130:
 /* CVTQL/V */
-gen_fcvtql_v(rb, rc);
-break;
 case 0x530:
 /* CVTQL/SV */
-gen_fcvtql_sv(rb, rc);
+/* ??? I'm pretty sure there's nothing that /sv needs to do that
+   /v doesn't do.  The only thing I can think is that /sv is a
+   valid instruction merely for completeness in the ISA.  */
+gen_fcvtql_v(ctx, rb, rc);
 break;
 default:
 goto invalid_opc;
-- 
1.6.6.1

[Qemu-devel] [PATCH 2/8] target-alpha: Implement cpys{, n, e} inline.

[Richard Henderson]

Signed-off-by: Richard Henderson 
---
 target-alpha/helper.h|4 --
 target-alpha/op_helper.c |   18 --
 target-alpha/translate.c |   78 +++--
 3 files changed, 74 insertions(+), 26 deletions(-)

diff --git a/target-alpha/helper.h b/target-alpha/helper.h
index a508077..8e11304 100644
--- a/target-alpha/helper.h
+++ b/target-alpha/helper.h
@@ -77,10 +77,6 @@ DEF_HELPER_FLAGS_2(cmpgeq, TCG_CALL_CONST | TCG_CALL_PURE, 
i64, i64, i64)
 DEF_HELPER_FLAGS_2(cmpgle, TCG_CALL_CONST | TCG_CALL_PURE, i64, i64, i64)
 DEF_HELPER_FLAGS_2(cmpglt, TCG_CALL_CONST | TCG_CALL_PURE, i64, i64, i64)
 
-DEF_HELPER_FLAGS_2(cpys, TCG_CALL_CONST | TCG_CALL_PURE, i64, i64, i64)
-DEF_HELPER_FLAGS_2(cpysn, TCG_CALL_CONST | TCG_CALL_PURE, i64, i64, i64)
-DEF_HELPER_FLAGS_2(cpyse, TCG_CALL_CONST | TCG_CALL_PURE, i64, i64, i64)
-
 DEF_HELPER_FLAGS_1(cvtts, TCG_CALL_CONST, i64, i64)
 DEF_HELPER_FLAGS_1(cvtst, TCG_CALL_CONST, i64, i64)
 DEF_HELPER_FLAGS_1(cvtqs, TCG_CALL_CONST, i64, i64)
diff --git a/target-alpha/op_helper.c b/target-alpha/op_helper.c
index 4d2c2ee..2419dc4 100644
--- a/target-alpha/op_helper.c
+++ b/target-alpha/op_helper.c
@@ -921,24 +921,6 @@ uint64_t helper_sqrtt (uint64_t a)
 return float64_to_t(fr);
 }
 
-
-/* Sign copy */
-uint64_t helper_cpys(uint64_t a, uint64_t b)
-{
-return (a & 0x8000ULL) | (b & ~0x8000ULL);
-}
-
-uint64_t helper_cpysn(uint64_t a, uint64_t b)
-{
-return ((~a) & 0x8000ULL) | (b & ~0x8000ULL);
-}
-
-uint64_t helper_cpyse(uint64_t a, uint64_t b)
-{
-return (a & 0xFFF0ULL) | (b & ~0xFFF0ULL);
-}
-
-
 /* Comparisons */
 uint64_t helper_cmptun (uint64_t a, uint64_t b)
 {
diff --git a/target-alpha/translate.c b/target-alpha/translate.c
index 719b423..b677378 100644
--- a/target-alpha/translate.c
+++ b/target-alpha/translate.c
@@ -741,6 +741,80 @@ static inline void glue(gen_f, name)(DisasContext *ctx,
 \
 IEEE_INTCVT(cvtqs)
 IEEE_INTCVT(cvtqt)
 
+static void gen_cpys_internal(int ra, int rb, int rc, int inv_a, uint64_t mask)
+{
+TCGv va, vb, vmask;
+int za = 0, zb = 0;
+
+if (unlikely(rc == 31)) {
+return;
+}
+
+vmask = tcg_const_i64(mask);
+
+TCGV_UNUSED_I64(va);
+if (ra == 31) {
+if (inv_a) {
+va = vmask;
+} else {
+za = 1;
+}
+} else {
+va = tcg_temp_new_i64();
+tcg_gen_mov_i64(va, cpu_fir[ra]);
+if (inv_a) {
+tcg_gen_not_i64(va, va);
+}
+tcg_gen_and_i64(va, va, vmask);
+}
+
+TCGV_UNUSED_I64(vb);
+if (rb == 31) {
+zb = 1;
+} else {
+vb = tcg_temp_new_i64();
+tcg_gen_andc_i64(vb, cpu_fir[rb], vmask);
+}
+
+switch (za * 2 + zb) {
+case 0:
+tcg_gen_or_i64(cpu_fir[rc], va, vb);
+break;
+case 1:
+tcg_gen_mov_i64(cpu_fir[rc], va);
+break;
+case 2:
+tcg_gen_mov_i64(cpu_fir[rc], vb);
+break;
+case 3:
+tcg_gen_movi_i64(cpu_fir[rc], 0);
+break;
+}
+
+tcg_temp_free(vmask);
+if (ra != 31) {
+tcg_temp_free(va);
+}
+if (rb != 31) {
+tcg_temp_free(vb);
+}
+}
+
+static inline void gen_fcpys(int ra, int rb, int rc)
+{
+gen_cpys_internal(ra, rb, rc, 0, 0x8000ULL);
+}
+
+static inline void gen_fcpysn(int ra, int rb, int rc)
+{
+gen_cpys_internal(ra, rb, rc, 1, 0x8000ULL);
+}
+
+static inline void gen_fcpyse(int ra, int rb, int rc)
+{
+gen_cpys_internal(ra, rb, rc, 0, 0xFFF0ULL);
+}
+
 #define FARITH3(name)   \
 static inline void glue(gen_f, name)(int ra, int rb, int rc)\
 {   \
@@ -769,10 +843,6 @@ static inline void glue(gen_f, name)(int ra, int rb, int 
rc)\
 tcg_temp_free(vb);  \
 }   \
 }
-/* ??? Ought to expand these inline; simple masking operations.  */
-FARITH3(cpys)
-FARITH3(cpysn)
-FARITH3(cpyse)
 
 /* ??? VAX instruction qualifiers ignored.  */
 FARITH3(addf)
-- 
1.6.6.1

[Qemu-devel] [PATCH 3/8] target-alpha: Implement rs/rc properly.

[Richard Henderson]

This is a per-cpu flag; there's no need for a spinlock of any kind.

We were also failing to manipulate the flag with $31 as a target reg
and failing to clear the flag on execution of a return-from-interrupt
instruction.

Signed-off-by: Richard Henderson 
---
 linux-user/main.c|5 +
 target-alpha/helper.h|2 --
 target-alpha/op_helper.c |   28 ++--
 target-alpha/translate.c |   19 +++
 4 files changed, 22 insertions(+), 32 deletions(-)

diff --git a/linux-user/main.c b/linux-user/main.c
index 4614e3c..d4a29cb 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -2356,6 +2356,11 @@ void cpu_loop (CPUState *env)
 while (1) {
 trapnr = cpu_alpha_exec (env);
 
+   /* All of the traps imply a transition through PALcode, which
+  implies an REI instruction has been executed.  Which means
+  that the intr_flag should be cleared.  */
+   env->intr_flag = 0;
+
 switch (trapnr) {
 case EXCP_RESET:
 fprintf(stderr, "Reset requested. Exit\n");
diff --git a/target-alpha/helper.h b/target-alpha/helper.h
index 8e11304..c378195 100644
--- a/target-alpha/helper.h
+++ b/target-alpha/helper.h
@@ -2,8 +2,6 @@
 
 DEF_HELPER_2(excp, void, int, int)
 DEF_HELPER_FLAGS_0(load_pcc, TCG_CALL_CONST | TCG_CALL_PURE, i64)
-DEF_HELPER_FLAGS_0(rc, TCG_CALL_CONST, i64)
-DEF_HELPER_FLAGS_0(rs, TCG_CALL_CONST, i64)
 
 DEF_HELPER_2(addqv, i64, i64, i64)
 DEF_HELPER_2(addlv, i64, i64, i64)
diff --git a/target-alpha/op_helper.c b/target-alpha/op_helper.c
index 2419dc4..84867b8 100644
--- a/target-alpha/op_helper.c
+++ b/target-alpha/op_helper.c
@@ -47,32 +47,6 @@ void helper_store_fpcr (uint64_t val)
 cpu_alpha_store_fpcr (env, val);
 }
 
-static spinlock_t intr_cpu_lock = SPIN_LOCK_UNLOCKED;
-
-uint64_t helper_rs(void)
-{
-uint64_t tmp;
-
-spin_lock(&intr_cpu_lock);
-tmp = env->intr_flag;
-env->intr_flag = 1;
-spin_unlock(&intr_cpu_lock);
-
-return tmp;
-}
-
-uint64_t helper_rc(void)
-{
-uint64_t tmp;
-
-spin_lock(&intr_cpu_lock);
-tmp = env->intr_flag;
-env->intr_flag = 0;
-spin_unlock(&intr_cpu_lock);
-
-return tmp;
-}
-
 uint64_t helper_addqv (uint64_t op1, uint64_t op2)
 {
 uint64_t tmp = op1;
@@ -1211,6 +1185,7 @@ void helper_hw_rei (void)
 {
 env->pc = env->ipr[IPR_EXC_ADDR] & ~3;
 env->ipr[IPR_EXC_ADDR] = env->ipr[IPR_EXC_ADDR] & 1;
+env->intr_flag = 0;
 /* XXX: re-enable interrupts and memory mapping */
 }
 
@@ -1218,6 +1193,7 @@ void helper_hw_ret (uint64_t a)
 {
 env->pc = a & ~3;
 env->ipr[IPR_EXC_ADDR] = a & 1;
+env->intr_flag = 0;
 /* XXX: re-enable interrupts and memory mapping */
 }
 
diff --git a/target-alpha/translate.c b/target-alpha/translate.c
index b677378..188e76c 100644
--- a/target-alpha/translate.c
+++ b/target-alpha/translate.c
@@ -1266,6 +1266,19 @@ static inline void gen_cmp(TCGCond cond, int ra, int rb, 
int rc, int islit,
 gen_set_label(l2);
 }
 
+static void gen_rx(int ra, int set)
+{
+TCGv_i32 tmp;
+
+if (ra != 31) {
+tcg_gen_ld8u_i64(cpu_ir[ra], cpu_env, offsetof(CPUState, intr_flag));
+}
+
+tmp = tcg_const_i32(set);
+tcg_gen_st8_i32(tmp, cpu_env, offsetof(CPUState, intr_flag));
+tcg_temp_free_i32(tmp);
+}
+
 static inline int translate_one(DisasContext *ctx, uint32_t insn)
 {
 uint32_t palcode;
@@ -2359,16 +2372,14 @@ static inline int translate_one(DisasContext *ctx, 
uint32_t insn)
 break;
 case 0xE000:
 /* RC */
-if (ra != 31)
-gen_helper_rc(cpu_ir[ra]);
+gen_rx(ra, 0);
 break;
 case 0xE800:
 /* ECB */
 break;
 case 0xF000:
 /* RS */
-if (ra != 31)
-gen_helper_rs(cpu_ir[ra]);
+gen_rx(ra, 1);
 break;
 case 0xF800:
 /* WH64 */
-- 
1.6.6.1

[Qemu-devel] [PATCH 5/8] target-alpha: Implement cvtlq inline.

[Richard Henderson]

It's a simple shift and mask sequence.

Signed-off-by: Richard Henderson 
---
 target-alpha/helper.h|1 -
 target-alpha/op_helper.c |7 ---
 target-alpha/translate.c |   21 -
 3 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/target-alpha/helper.h b/target-alpha/helper.h
index 10c78d0..ccf6a2a 100644
--- a/target-alpha/helper.h
+++ b/target-alpha/helper.h
@@ -83,7 +83,6 @@ DEF_HELPER_FLAGS_1(cvtqf, TCG_CALL_CONST, i64, i64)
 DEF_HELPER_FLAGS_1(cvtgf, TCG_CALL_CONST, i64, i64)
 DEF_HELPER_FLAGS_1(cvtgq, TCG_CALL_CONST, i64, i64)
 DEF_HELPER_FLAGS_1(cvtqg, TCG_CALL_CONST, i64, i64)
-DEF_HELPER_FLAGS_1(cvtlq, TCG_CALL_CONST | TCG_CALL_PURE, i64, i64)
 
 DEF_HELPER_FLAGS_1(cvttq, TCG_CALL_CONST, i64, i64)
 DEF_HELPER_FLAGS_1(cvttq_c, TCG_CALL_CONST, i64, i64)
diff --git a/target-alpha/op_helper.c b/target-alpha/op_helper.c
index f9cd07a..a209130 100644
--- a/target-alpha/op_helper.c
+++ b/target-alpha/op_helper.c
@@ -1152,13 +1152,6 @@ uint64_t helper_cvtqg (uint64_t a)
 return float64_to_g(fr);
 }
 
-uint64_t helper_cvtlq (uint64_t a)
-{
-int32_t lo = a >> 29;
-int32_t hi = a >> 32;
-return (lo & 0x3FFF) | (hi & 0xc000);
-}
-
 /* PALcode support special instructions */
 #if !defined (CONFIG_USER_ONLY)
 void helper_hw_rei (void)
diff --git a/target-alpha/translate.c b/target-alpha/translate.c
index cfdf441..c52cac3 100644
--- a/target-alpha/translate.c
+++ b/target-alpha/translate.c
@@ -597,6 +597,26 @@ static inline void gen_fp_exc_raise(int rc, int fn11)
 gen_fp_exc_raise_ignore(rc, fn11, fn11 & QUAL_I ? 0 : float_flag_inexact);
 }
 
+static void gen_fcvtlq(int rb, int rc)
+{
+if (unlikely(rc == 31)) {
+return;
+}
+if (unlikely(rb == 31)) {
+tcg_gen_movi_i64(cpu_fir[rc], 0);
+} else {
+TCGv tmp = tcg_temp_new();
+
+tcg_gen_shri_i64(tmp, cpu_fir[rb], 32);
+tcg_gen_shri_i64(cpu_fir[rc], cpu_fir[rb], 29);
+tcg_gen_andi_i64(tmp, tmp, 0xc000);
+tcg_gen_andi_i64(cpu_fir[rc], cpu_fir[rc], 0x3FFF);
+tcg_gen_or_i64(cpu_fir[rc], cpu_fir[rc], tmp);
+
+tcg_temp_free(tmp);
+}
+}
+
 static void gen_fcvtql(int rb, int rc)
 {
 if (unlikely(rc == 31)) {
@@ -646,7 +666,6 @@ static inline void glue(gen_f, name)(int rb, int rc)\
 tcg_temp_free(tmp); \
 }   \
 }
-FARITH2(cvtlq)
 
 /* ??? VAX instruction qualifiers ignored.  */
 FARITH2(sqrtf)
-- 
1.6.6.1

[Qemu-devel] [PATCH 1/8] target-alpha: Add flags markups to helpers.h.

[Richard Henderson]

Almost all alpha helpers are at least TCG_CALL_CONST
and a fair few are also TCG_CALL_PURE.

Signed-off-by: Richard Henderson 
---
 target-alpha/helper.h |  184 
 1 files changed, 92 insertions(+), 92 deletions(-)

diff --git a/target-alpha/helper.h b/target-alpha/helper.h
index 79cf375..a508077 100644
--- a/target-alpha/helper.h
+++ b/target-alpha/helper.h
@@ -1,9 +1,9 @@
 #include "def-helper.h"
 
 DEF_HELPER_2(excp, void, int, int)
-DEF_HELPER_0(load_pcc, i64)
-DEF_HELPER_0(rc, i64)
-DEF_HELPER_0(rs, i64)
+DEF_HELPER_FLAGS_0(load_pcc, TCG_CALL_CONST | TCG_CALL_PURE, i64)
+DEF_HELPER_FLAGS_0(rc, TCG_CALL_CONST, i64)
+DEF_HELPER_FLAGS_0(rs, TCG_CALL_CONST, i64)
 
 DEF_HELPER_2(addqv, i64, i64, i64)
 DEF_HELPER_2(addlv, i64, i64, i64)
@@ -11,98 +11,98 @@ DEF_HELPER_2(subqv, i64, i64, i64)
 DEF_HELPER_2(sublv, i64, i64, i64)
 DEF_HELPER_2(mullv, i64, i64, i64)
 DEF_HELPER_2(mulqv, i64, i64, i64)
-DEF_HELPER_2(umulh, i64, i64, i64)
-
-DEF_HELPER_1(ctpop, i64, i64)
-DEF_HELPER_1(ctlz, i64, i64)
-DEF_HELPER_1(cttz, i64, i64)
-
-DEF_HELPER_2(zap, i64, i64, i64)
-DEF_HELPER_2(zapnot, i64, i64, i64)
-
-DEF_HELPER_2(cmpbge, i64, i64, i64)
-
-DEF_HELPER_2(minub8, i64, i64, i64)
-DEF_HELPER_2(minsb8, i64, i64, i64)
-DEF_HELPER_2(minuw4, i64, i64, i64)
-DEF_HELPER_2(minsw4, i64, i64, i64)
-DEF_HELPER_2(maxub8, i64, i64, i64)
-DEF_HELPER_2(maxsb8, i64, i64, i64)
-DEF_HELPER_2(maxuw4, i64, i64, i64)
-DEF_HELPER_2(maxsw4, i64, i64, i64)
-DEF_HELPER_2(perr, i64, i64, i64)
-DEF_HELPER_1(pklb, i64, i64)
-DEF_HELPER_1(pkwb, i64, i64)
-DEF_HELPER_1(unpkbl, i64, i64)
-DEF_HELPER_1(unpkbw, i64, i64)
-
-DEF_HELPER_0(load_fpcr, i64)
-DEF_HELPER_1(store_fpcr, void, i64)
-
-DEF_HELPER_1(f_to_memory, i32, i64)
-DEF_HELPER_1(memory_to_f, i64, i32)
-DEF_HELPER_2(addf, i64, i64, i64)
-DEF_HELPER_2(subf, i64, i64, i64)
-DEF_HELPER_2(mulf, i64, i64, i64)
-DEF_HELPER_2(divf, i64, i64, i64)
-DEF_HELPER_1(sqrtf, i64, i64)
-
-DEF_HELPER_1(g_to_memory, i64, i64)
-DEF_HELPER_1(memory_to_g, i64, i64)
-DEF_HELPER_2(addg, i64, i64, i64)
-DEF_HELPER_2(subg, i64, i64, i64)
-DEF_HELPER_2(mulg, i64, i64, i64)
-DEF_HELPER_2(divg, i64, i64, i64)
-DEF_HELPER_1(sqrtg, i64, i64)
-
-DEF_HELPER_1(s_to_memory, i32, i64)
-DEF_HELPER_1(memory_to_s, i64, i32)
-DEF_HELPER_2(adds, i64, i64, i64)
-DEF_HELPER_2(subs, i64, i64, i64)
-DEF_HELPER_2(muls, i64, i64, i64)
-DEF_HELPER_2(divs, i64, i64, i64)
-DEF_HELPER_1(sqrts, i64, i64)
-
-DEF_HELPER_2(addt, i64, i64, i64)
-DEF_HELPER_2(subt, i64, i64, i64)
-DEF_HELPER_2(mult, i64, i64, i64)
-DEF_HELPER_2(divt, i64, i64, i64)
-DEF_HELPER_1(sqrtt, i64, i64)
-
-DEF_HELPER_2(cmptun, i64, i64, i64)
-DEF_HELPER_2(cmpteq, i64, i64, i64)
-DEF_HELPER_2(cmptle, i64, i64, i64)
-DEF_HELPER_2(cmptlt, i64, i64, i64)
-DEF_HELPER_2(cmpgeq, i64, i64, i64)
-DEF_HELPER_2(cmpgle, i64, i64, i64)
-DEF_HELPER_2(cmpglt, i64, i64, i64)
-
-DEF_HELPER_2(cpys, i64, i64, i64)
-DEF_HELPER_2(cpysn, i64, i64, i64)
-DEF_HELPER_2(cpyse, i64, i64, i64)
-
-DEF_HELPER_1(cvtts, i64, i64)
-DEF_HELPER_1(cvtst, i64, i64)
-DEF_HELPER_1(cvtqs, i64, i64)
-DEF_HELPER_1(cvtqt, i64, i64)
-DEF_HELPER_1(cvtqf, i64, i64)
-DEF_HELPER_1(cvtgf, i64, i64)
-DEF_HELPER_1(cvtgq, i64, i64)
-DEF_HELPER_1(cvtqg, i64, i64)
-DEF_HELPER_1(cvtlq, i64, i64)
-
-DEF_HELPER_1(cvttq, i64, i64)
-DEF_HELPER_1(cvttq_c, i64, i64)
-DEF_HELPER_1(cvttq_svic, i64, i64)
-
-DEF_HELPER_1(cvtql, i64, i64)
+DEF_HELPER_FLAGS_2(umulh, TCG_CALL_CONST | TCG_CALL_PURE, i64, i64, i64)
+
+DEF_HELPER_FLAGS_1(ctpop, TCG_CALL_CONST | TCG_CALL_PURE, i64, i64)
+DEF_HELPER_FLAGS_1(ctlz, TCG_CALL_CONST | TCG_CALL_PURE, i64, i64)
+DEF_HELPER_FLAGS_1(cttz, TCG_CALL_CONST | TCG_CALL_PURE, i64, i64)
+
+DEF_HELPER_FLAGS_2(zap, TCG_CALL_CONST | TCG_CALL_PURE, i64, i64, i64)
+DEF_HELPER_FLAGS_2(zapnot, TCG_CALL_CONST | TCG_CALL_PURE, i64, i64, i64)
+
+DEF_HELPER_FLAGS_2(cmpbge, TCG_CALL_CONST | TCG_CALL_PURE, i64, i64, i64)
+
+DEF_HELPER_FLAGS_2(minub8, TCG_CALL_CONST | TCG_CALL_PURE, i64, i64, i64)
+DEF_HELPER_FLAGS_2(minsb8, TCG_CALL_CONST | TCG_CALL_PURE, i64, i64, i64)
+DEF_HELPER_FLAGS_2(minuw4, TCG_CALL_CONST | TCG_CALL_PURE, i64, i64, i64)
+DEF_HELPER_FLAGS_2(minsw4, TCG_CALL_CONST | TCG_CALL_PURE, i64, i64, i64)
+DEF_HELPER_FLAGS_2(maxub8, TCG_CALL_CONST | TCG_CALL_PURE, i64, i64, i64)
+DEF_HELPER_FLAGS_2(maxsb8, TCG_CALL_CONST | TCG_CALL_PURE, i64, i64, i64)
+DEF_HELPER_FLAGS_2(maxuw4, TCG_CALL_CONST | TCG_CALL_PURE, i64, i64, i64)
+DEF_HELPER_FLAGS_2(maxsw4, TCG_CALL_CONST | TCG_CALL_PURE, i64, i64, i64)
+DEF_HELPER_FLAGS_2(perr, TCG_CALL_CONST | TCG_CALL_PURE, i64, i64, i64)
+DEF_HELPER_FLAGS_1(pklb, TCG_CALL_CONST | TCG_CALL_PURE, i64, i64)
+DEF_HELPER_FLAGS_1(pkwb, TCG_CALL_CONST | TCG_CALL_PURE, i64, i64)
+DEF_HELPER_FLAGS_1(unpkbl, TCG_CALL_CONST | TCG_CALL_PURE, i64, i64)
+DEF_HELPER_FLAGS_1(unpkbw, TCG_CALL_CONST | TCG_CALL_PURE, i64, i64)
+
+DEF_HELPER_FLAGS_0(load_fpcr, TCG_CALL_CONST | TCG_CALL_PURE, i64)
+DEF_HELPER_FLAGS_1(store_fpcr, TCG_CALL_CONST, 

[Qemu-devel] [PATCH 0/8] target-alpha improvements, v2

[Richard Henderson]

Changes from v1:
  * Use setcond and goto_tb.


r~



Richard Henderson (8):
  target-alpha: Add flags markups to helpers.h.
  target-alpha: Implement cpys{,n,e} inline.
  target-alpha: Implement rs/rc properly.
  target-alpha: Implement cvtql inline.
  target-alpha: Implement cvtlq inline.
  target-alpha: Use setcond for int comparisons.
  target-alpha: Use non-inverted arguments to gen_{f}cmov.
  target-alpha: Emit goto_tb opcodes.

 linux-user/main.c|5 +
 target-alpha/helper.h|  179 +++
 target-alpha/op_helper.c |   73 +--
 target-alpha/translate.c |  576 ++
 4 files changed, 470 insertions(+), 363 deletions(-)

[Qemu-devel] [PATCH 7/8] target-alpha: Use non-inverted arguments to gen_{f}cmov.

[Richard Henderson]

The inverted conditions as argument to the function looks wrong
at a glance inside translate_one.  Since we have an easy function
to produce the inversion now, use it.

Signed-off-by: Richard Henderson 
---
 target-alpha/translate.c |   37 +++--
 1 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/target-alpha/translate.c b/target-alpha/translate.c
index 3fa32b3..b845094 100644
--- a/target-alpha/translate.c
+++ b/target-alpha/translate.c
@@ -394,9 +394,10 @@ static void gen_fbcond(DisasContext *ctx, TCGCond cond, 
int ra, int32_t disp)
 gen_bcond_pcload(ctx, disp, lab_true);
 }
 
-static inline void gen_cmov(TCGCond inv_cond, int ra, int rb, int rc,
-int islit, uint8_t lit, int mask)
+static void gen_cmov(TCGCond cond, int ra, int rb, int rc,
+int islit, uint8_t lit, int mask)
 {
+TCGCond inv_cond = tcg_invert_cond(cond);
 int l1;
 
 if (unlikely(rc == 31))
@@ -426,7 +427,7 @@ static inline void gen_cmov(TCGCond inv_cond, int ra, int 
rb, int rc,
 gen_set_label(l1);
 }
 
-static void gen_fcmov(TCGCond inv_cond, int ra, int rb, int rc)
+static void gen_fcmov(TCGCond cond, int ra, int rb, int rc)
 {
 TCGv va = cpu_fir[ra];
 int l1;
@@ -439,7 +440,7 @@ static void gen_fcmov(TCGCond inv_cond, int ra, int rb, int 
rc)
 }
 
 l1 = gen_new_label();
-gen_fbcond_internal(inv_cond, va, l1);
+gen_fbcond_internal(tcg_invert_cond(cond), va, l1);
 
 if (rb != 31)
 tcg_gen_mov_i64(cpu_fir[rc], cpu_fir[rb]);
@@ -1765,11 +1766,11 @@ static inline int translate_one(DisasContext *ctx, 
uint32_t insn)
 break;
 case 0x14:
 /* CMOVLBS */
-gen_cmov(TCG_COND_EQ, ra, rb, rc, islit, lit, 1);
+gen_cmov(TCG_COND_NE, ra, rb, rc, islit, lit, 1);
 break;
 case 0x16:
 /* CMOVLBC */
-gen_cmov(TCG_COND_NE, ra, rb, rc, islit, lit, 1);
+gen_cmov(TCG_COND_EQ, ra, rb, rc, islit, lit, 1);
 break;
 case 0x20:
 /* BIS */
@@ -1789,11 +1790,11 @@ static inline int translate_one(DisasContext *ctx, 
uint32_t insn)
 break;
 case 0x24:
 /* CMOVEQ */
-gen_cmov(TCG_COND_NE, ra, rb, rc, islit, lit, 0);
+gen_cmov(TCG_COND_EQ, ra, rb, rc, islit, lit, 0);
 break;
 case 0x26:
 /* CMOVNE */
-gen_cmov(TCG_COND_EQ, ra, rb, rc, islit, lit, 0);
+gen_cmov(TCG_COND_NE, ra, rb, rc, islit, lit, 0);
 break;
 case 0x28:
 /* ORNOT */
@@ -1829,11 +1830,11 @@ static inline int translate_one(DisasContext *ctx, 
uint32_t insn)
 break;
 case 0x44:
 /* CMOVLT */
-gen_cmov(TCG_COND_GE, ra, rb, rc, islit, lit, 0);
+gen_cmov(TCG_COND_LT, ra, rb, rc, islit, lit, 0);
 break;
 case 0x46:
 /* CMOVGE */
-gen_cmov(TCG_COND_LT, ra, rb, rc, islit, lit, 0);
+gen_cmov(TCG_COND_GE, ra, rb, rc, islit, lit, 0);
 break;
 case 0x48:
 /* EQV */
@@ -1873,11 +1874,11 @@ static inline int translate_one(DisasContext *ctx, 
uint32_t insn)
 break;
 case 0x64:
 /* CMOVLE */
-gen_cmov(TCG_COND_GT, ra, rb, rc, islit, lit, 0);
+gen_cmov(TCG_COND_LE, ra, rb, rc, islit, lit, 0);
 break;
 case 0x66:
 /* CMOVGT */
-gen_cmov(TCG_COND_LE, ra, rb, rc, islit, lit, 0);
+gen_cmov(TCG_COND_GT, ra, rb, rc, islit, lit, 0);
 break;
 case 0x6C:
 /* IMPLVER */
@@ -2351,27 +2352,27 @@ static inline int translate_one(DisasContext *ctx, 
uint32_t insn)
 break;
 case 0x02A:
 /* FCMOVEQ */
-gen_fcmov(TCG_COND_NE, ra, rb, rc);
+gen_fcmov(TCG_COND_EQ, ra, rb, rc);
 break;
 case 0x02B:
 /* FCMOVNE */
-gen_fcmov(TCG_COND_EQ, ra, rb, rc);
+gen_fcmov(TCG_COND_NE, ra, rb, rc);
 break;
 case 0x02C:
 /* FCMOVLT */
-gen_fcmov(TCG_COND_GE, ra, rb, rc);
+gen_fcmov(TCG_COND_LT, ra, rb, rc);
 break;
 case 0x02D:
 /* FCMOVGE */
-gen_fcmov(TCG_COND_LT, ra, rb, rc);
+gen_fcmov(TCG_COND_GE, ra, rb, rc);
 break;
 case 0x02E:
 /* FCMOVLE */
-gen_fcmov(TCG_COND_GT, ra, rb, rc);
+gen_fcmov(TCG_COND_LE, ra, rb, rc);
 break;
 case 0x02F:
 /* FCMOVGT */
-gen_fcmov(TCG_COND_LE, ra, rb, rc);
+gen_fcmov(TCG_COND_GT, ra, rb, rc);
 break;
 case 0x030:
 /* CVTQL */
-- 
1.6.6.1

[Qemu-devel] patches

[Edgar E. Iglesias]

Hi,

I usually mail out negative test reults but this time I've just
pulled and merged to my 4 different CRIS, PPC and MicroBlaze trees
from upstream and things Just Worked.

Good work to everybody involved!

Cheers,
Edgar

Re: [Qemu-devel] [PATCH 5/6] error: Link qemu-img, qemu-nbd, qemu-io with qemu-error.o

[Markus Armbruster]

Blue Swirl  writes:

> On 3/17/10, Markus Armbruster  wrote:
>> The location tracking interface is used by code shared with qemi-img,
>>  qemu-nbd and qemu-io, so it needs to be available there.  Commit
>>  827b0813 provides it in a rather hamfisted way: it adds a dummy
>>  implementation to qemu-tool.c.
>>
>>  It's cleaner to provide the real thing, and put a few more dummy
>>  monitor functions into qemu-tool.c.
>>
>>  Signed-off-by: Markus Armbruster 
>>  ---
>>   Makefile|6 +++---
>>   qemu-tool.c |   50 --
>>   2 files changed, 19 insertions(+), 37 deletions(-)
>>
>>  diff --git a/Makefile b/Makefile
>>  index 2066c12..aad361e 100644
>>  --- a/Makefile
>>  +++ b/Makefile
>>  @@ -129,11 +129,11 @@ bt-host.o: QEMU_CFLAGS += $(BLUEZ_CFLAGS)
>>   qemu-img.o: qemu-img-cmds.h
>>   qemu-img.o qemu-tool.o qemu-nbd.o qemu-io.o: $(GENERATED_HEADERS)
>>
>>  -qemu-img$(EXESUF): qemu-img.o qemu-tool.o $(block-obj-y) $(qobject-obj-y)
>>  +qemu-img$(EXESUF): qemu-img.o qemu-tool.o qemu-error.o $(block-obj-y) 
>> $(qobject-obj-y)
>>
>>  -qemu-nbd$(EXESUF): qemu-nbd.o qemu-tool.o $(block-obj-y) $(qobject-obj-y)
>>  +qemu-nbd$(EXESUF): qemu-nbd.o qemu-tool.o qemu-error.o $(block-obj-y) 
>> $(qobject-obj-y)
>>
>>  -qemu-io$(EXESUF): qemu-io.o qemu-tool.o cmd.o $(block-obj-y) 
>> $(qobject-obj-y)
>>  +qemu-io$(EXESUF): qemu-io.o cmd.o qemu-tool.o qemu-error.o $(block-obj-y) 
>> $(qobject-obj-y)
>>
>>   qemu-img-cmds.h: $(SRC_PATH)/qemu-img-cmds.hx
>> $(call quiet-command,sh $(SRC_PATH)/hxtool -h < $< > $@,"  GEN   $@")
>>  diff --git a/qemu-tool.c b/qemu-tool.c
>>  index 97ca949..8d6d0ef 100644
>>  --- a/qemu-tool.c
>>  +++ b/qemu-tool.c
>>  @@ -15,7 +15,6 @@
>>   #include "monitor.h"
>>   #include "qemu-timer.h"
>>   #include "qemu-log.h"
>>  -#include "qemu-error.h"
>>
>>   #include 
>>
>>  @@ -33,6 +32,22 @@ void qemu_service_io(void)
>>   {
>>   }
>>
>>  +Monitor *cur_mon;
>>  +
>>  +int monitor_cur_is_qmp(void)
>>  +{
>>  +return 0;
>>  +}
>>  +
>>  +void monitor_set_error(Monitor *mon, QError *qerror)
>>  +{
>>  +assert(0);
>
> Please use abort().

Why?

I'd like to understand the rules to avoid unnecessary respins in the
future.

[...]

Re: [Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement

[Blue Swirl]

On 3/17/10, Anthony Liguori  wrote:
> On 03/17/2010 03:56 PM, Paul Bolle wrote:
>
> > On Wed, 2010-03-17 at 15:41 -0500, Anthony Liguori wrote:
> >
> >
> > > On 03/17/2010 03:15 PM, Blue Swirl wrote:
> > >
> > >
> > > > This breaks build (gcc 4.3.2):
> > > >CCusb-linux.o
> > > > cc1: warnings being treated as errors
> > > > /src/qemu/usb-linux.c: In function 'usb_linux_update_endp_table':
> > > > /src/qemu/usb-linux.c:759: error: 'type' may be used uninitialized in
> > > > this function
> > > >
> > > >
> > > >
> > > That's unfortunate.  I'll revert.
> > >
> > >
> > I can't reproduce this with gcc-4.4.3-8.fc13.i686 (which I'm currently
> > running). The patch was tested and submitted when I was running
> > gcc-4.4.3-6.fc13.i686.
> >
> >
>
>  Yeah, it worked for me, but clearly older versions of gcc are less smart
> about calculating ranges.

Actually OpenBSD gcc (3.4.4) has no problems.

Re: [Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement

[Anthony Liguori]

On 03/17/2010 03:56 PM, Paul Bolle wrote:

On Wed, 2010-03-17 at 15:41 -0500, Anthony Liguori wrote:
   

On 03/17/2010 03:15 PM, Blue Swirl wrote:
 

This breaks build (gcc 4.3.2):
CCusb-linux.o
cc1: warnings being treated as errors
/src/qemu/usb-linux.c: In function 'usb_linux_update_endp_table':
/src/qemu/usb-linux.c:759: error: 'type' may be used uninitialized in
this function

   

That's unfortunate.  I'll revert.
 

I can't reproduce this with gcc-4.4.3-8.fc13.i686 (which I'm currently
running). The patch was tested and submitted when I was running
gcc-4.4.3-6.fc13.i686.
   


Yeah, it worked for me, but clearly older versions of gcc are less smart 
about calculating ranges.


Regards,

Anthony Liguori


Regards,


Paul Bolle

   

Re: [Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement

[Paul Bolle]

On Wed, 2010-03-17 at 15:41 -0500, Anthony Liguori wrote:
> On 03/17/2010 03:15 PM, Blue Swirl wrote:
> > This breaks build (gcc 4.3.2):
> >CCusb-linux.o
> > cc1: warnings being treated as errors
> > /src/qemu/usb-linux.c: In function 'usb_linux_update_endp_table':
> > /src/qemu/usb-linux.c:759: error: 'type' may be used uninitialized in
> > this function
> >
> 
> That's unfortunate.  I'll revert.

I can't reproduce this with gcc-4.4.3-8.fc13.i686 (which I'm currently
running). The patch was tested and submitted when I was running
gcc-4.4.3-6.fc13.i686.

Regards,


Paul Bolle

Re: [Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement

[Anthony Liguori]

On 03/17/2010 03:15 PM, Blue Swirl wrote:

On 3/17/10, Paul Brook  wrote:
   

If something should never happen (as in this case) then an abort/assert
 

  >  >  is completely appropriate. Once things get that screwed up there's no
  >  >  right answer, and the best thing we can do is terminate immediately to
  >  >  try and avoid further damage.
  >
  >  This case was:
  >
  >  switch (foo&  0x03) {
  >  case 0: case 1: case 2: case 3:
  >  default:
  >  }
  >
  >  The default is unreachable.  Having it there just introduces more code
  >  that serves no purpose.  Unless someone does something totally foolish
  >  and changes the mask in the switch statement, there's no way it will
  >  ever be reachable.


I mistakenly remembered this was using a symbolic mask rather than a literal
  0x03. In that case I'd argue it's much easier to make the dumb error you
  describe, and the assert can be a handy cluebat.

  I guess it's largely personal preference - I prefer to add the default case to
  make it clear that falling through is never gong to be the right answer.
 

This breaks build (gcc 4.3.2):
   CCusb-linux.o
cc1: warnings being treated as errors
/src/qemu/usb-linux.c: In function 'usb_linux_update_endp_table':
/src/qemu/usb-linux.c:759: error: 'type' may be used uninitialized in
this function
   


That's unfortunate.  I'll revert.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement

[Blue Swirl]

On 3/17/10, Paul Brook  wrote:
> > > If something should never happen (as in this case) then an abort/assert
>  > > is completely appropriate. Once things get that screwed up there's no
>  > > right answer, and the best thing we can do is terminate immediately to
>  > > try and avoid further damage.
>  >
>  > This case was:
>  >
>  > switch (foo & 0x03) {
>  > case 0: case 1: case 2: case 3:
>  > default:
>  > }
>  >
>  > The default is unreachable.  Having it there just introduces more code
>  > that serves no purpose.  Unless someone does something totally foolish
>  > and changes the mask in the switch statement, there's no way it will
>  > ever be reachable.
>
>
> I mistakenly remembered this was using a symbolic mask rather than a literal
>  0x03. In that case I'd argue it's much easier to make the dumb error you
>  describe, and the assert can be a handy cluebat.
>
>  I guess it's largely personal preference - I prefer to add the default case 
> to
>  make it clear that falling through is never gong to be the right answer.

This breaks build (gcc 4.3.2):
  CCusb-linux.o
cc1: warnings being treated as errors
/src/qemu/usb-linux.c: In function 'usb_linux_update_endp_table':
/src/qemu/usb-linux.c:759: error: 'type' may be used uninitialized in
this function

Re: [Qemu-devel] [PATCH 3/4] tcg-hppa: Finish the port.

[Stuart Brady]

On Wed, Mar 17, 2010 at 07:56:12AM -0700, Richard Henderson wrote:
> On 03/16/2010 06:58 PM, Stuart Brady wrote:
> > The tcg_reg_free() calls worry me slightly, but I assume they're safe...
> 
> Yeah, that one's rather gross.
> 
> Since Aurelien's generic div/rem patch I could simply remove all
> that millicode stuff, including that tcg_reg_free, and let the
> generic code handle this instead.  Which would get to the same
> millicode routine via one or two extra levels of indirection.
> 
> Thoughts?

Well, assuming that the millicode stuff does indeed work, I wouldn't worry
too much about it...  I only imagine that this is something that other
archs might need to do too, so it'd be worth doing as cleanly as possible.

Likewise, the qemu_ld/qemu_st refactoring should really be done for other
archs at some later stage, too, as it's quite a significant improvement in
readability of the code.

Cheers,
-- 
Stuart Brady

Re: [Qemu-devel] [PATCH 5/6] error: Link qemu-img, qemu-nbd, qemu-io with qemu-error.o

[Blue Swirl]

On 3/17/10, Markus Armbruster  wrote:
> The location tracking interface is used by code shared with qemi-img,
>  qemu-nbd and qemu-io, so it needs to be available there.  Commit
>  827b0813 provides it in a rather hamfisted way: it adds a dummy
>  implementation to qemu-tool.c.
>
>  It's cleaner to provide the real thing, and put a few more dummy
>  monitor functions into qemu-tool.c.
>
>  Signed-off-by: Markus Armbruster 
>  ---
>   Makefile|6 +++---
>   qemu-tool.c |   50 --
>   2 files changed, 19 insertions(+), 37 deletions(-)
>
>  diff --git a/Makefile b/Makefile
>  index 2066c12..aad361e 100644
>  --- a/Makefile
>  +++ b/Makefile
>  @@ -129,11 +129,11 @@ bt-host.o: QEMU_CFLAGS += $(BLUEZ_CFLAGS)
>   qemu-img.o: qemu-img-cmds.h
>   qemu-img.o qemu-tool.o qemu-nbd.o qemu-io.o: $(GENERATED_HEADERS)
>
>  -qemu-img$(EXESUF): qemu-img.o qemu-tool.o $(block-obj-y) $(qobject-obj-y)
>  +qemu-img$(EXESUF): qemu-img.o qemu-tool.o qemu-error.o $(block-obj-y) 
> $(qobject-obj-y)
>
>  -qemu-nbd$(EXESUF): qemu-nbd.o qemu-tool.o $(block-obj-y) $(qobject-obj-y)
>  +qemu-nbd$(EXESUF): qemu-nbd.o qemu-tool.o qemu-error.o $(block-obj-y) 
> $(qobject-obj-y)
>
>  -qemu-io$(EXESUF): qemu-io.o qemu-tool.o cmd.o $(block-obj-y) 
> $(qobject-obj-y)
>  +qemu-io$(EXESUF): qemu-io.o cmd.o qemu-tool.o qemu-error.o $(block-obj-y) 
> $(qobject-obj-y)
>
>   qemu-img-cmds.h: $(SRC_PATH)/qemu-img-cmds.hx
> $(call quiet-command,sh $(SRC_PATH)/hxtool -h < $< > $@,"  GEN   $@")
>  diff --git a/qemu-tool.c b/qemu-tool.c
>  index 97ca949..8d6d0ef 100644
>  --- a/qemu-tool.c
>  +++ b/qemu-tool.c
>  @@ -15,7 +15,6 @@
>   #include "monitor.h"
>   #include "qemu-timer.h"
>   #include "qemu-log.h"
>  -#include "qemu-error.h"
>
>   #include 
>
>  @@ -33,6 +32,22 @@ void qemu_service_io(void)
>   {
>   }
>
>  +Monitor *cur_mon;
>  +
>  +int monitor_cur_is_qmp(void)
>  +{
>  +return 0;
>  +}
>  +
>  +void monitor_set_error(Monitor *mon, QError *qerror)
>  +{
>  +assert(0);

Please use abort().

>  +}
>  +
>  +void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
>  +{
>  +}
>  +
>   void monitor_printf(Monitor *mon, const char *fmt, ...)
>   {
>   }
>  @@ -103,36 +118,3 @@ int64_t qemu_get_clock(QEMUClock *clock)
>  qemu_gettimeofday(&tv);
>  return (tv.tv_sec * 10LL + (tv.tv_usec * 1000)) / 100;
>   }
>  -
>  -Location *loc_push_restore(Location *loc)
>  -{
>  -return loc;
>  -}
>  -
>  -Location *loc_push_none(Location *loc)
>  -{
>  -return loc;
>  -}
>  -
>  -Location *loc_pop(Location *loc)
>  -{
>  -return loc;
>  -}
>  -
>  -Location *loc_save(Location *loc)
>  -{
>  -return loc;
>  -}
>  -
>  -void loc_restore(Location *loc)
>  -{
>  -}
>  -
>  -void error_report(const char *fmt, ...)
>  -{
>  -va_list args;
>  -
>  -va_start(args, fmt);
>  -vfprintf(stderr, fmt, args);
>  -va_end(args);
>  -}
>
> --
>  1.6.6.1
>
>
>
>

Re: [Qemu-devel] Re: [PATCH, RFC] Replace assert(0) with abort() or cpu_abort()

[Blue Swirl]

On 3/17/10, Paolo Bonzini  wrote:
> On 03/16/2010 06:55 PM, Jamie Lokier wrote:
>
> > A guest program is also allowed to trap SIGABRT with a signal handler,
> > and that does have some uses.  E.g. cleaning up temporary files and
> > shmem segments following a crash when calling 3rd party code.
> >
> > Whatever the guest does with SIGABRT, it should not result in_QEMU_
> > crashing - whether due to abort() returning, or QEMU's control flow
> > jumping to the guest's signal handler from an unexpected location.
> >
>
>  That's very hard to ensure however if QEMU was already in unstable state,
> as witnessed by its call to abort().  Things can only go downhill.
>
>  Maybe there should be a qemu_abort wrapper (and a QEMU_ASSERT companion)
> that does simply abort/assert under system emulation, but under user-mode
> emulation does
>
>   signal (SIGABRT, SIG_DFL);
>   abort ();

Fine, we cover this obscure corner case. But what if in addition to
this, the user process forked and the other process ptraced the QEMU
process, and when QEMU attempts to install the default signal handler,
the call is instead diverted to somewhere else with ptrace?

[Qemu-devel] Locking block devices for concurrent access?

[Michael Tokarev]

I remember quite long discussion about this issue
a while back.  But unfortunately, a) I can't find
it now, and b) as far as I remember, there was no
definitive solution presented at that time.  So I
thought it's Ok to ask again to get more conclusive
answer...

The original problem is that currently qemu provides
no attempt to prevent concurrent access to the same
"virtual disk" by multiple qemu instances, or it can
happily pass a filesystem mounted in host to a guest
it runs.

I understand pretty well that there are valid usage
cases for multiple qemu guests having the same block
device (file, whatever) open at the same time, even
in read-write mode (but it is still not quite safe
for formats with a structure, like qcow for example).
There are cluster filesystems out there, which works
on shared storage devices.

But the thing is that in almost all "usual" cases,
non-cluster filesystem will be used in guests, and
it'd be _very_ useful for qemu to actually at least
try to warn user that the given device is already
in use.  Because it is quite easy to trash the guest
filesystem by "mounting" the same "device" in two
different guests at the same time (or in host and in
guest simultaneously, for that matter).  I've run
across this already myself, not once, and there are
other people who've hit the same trap.

I understand also that there are qcow[2] base images
which needs to be opened in different locking mode,
since they're usually read-only; and even there, it'd
be a good idea to ensure that the base image is not
open in RW mode already, or that it WILL not be opened
RW while we're basing on it.  Or something like that
anyway.

The mentioned discussion which I can't find - there
was a proposal to add an argument like "share-mode"
or "lock" to -drive foo=bar,xyz=asdf parameter list,
with values from the set "none", "shared", "exclusive".
But what I can't remember is what the conclusion was...

Can we please have some summary of where it all sits
nowadays?

Thank you!

/mjt

[Qemu-devel] Re: guest kernel debugging through serial port

[Neo Jia]

Here is what I have asked before. The problem that I want to assign a
real serial port to the guest is that the debugging through network
becomes really slow.

Thanks,
Neo

On Thu, Mar 11, 2010 at 2:44 AM, Neo Jia  wrote:
> hi,
>
> I have followed the windows guest debugging procedure from
> http://www.linux-kvm.org/page/WindowsGuestDrivers/GuestDebugging. And
> it works when I start two guests and bind tcp port to guest serial
> port, but it is really slow.
>
> And if I use -serial /dev/ttyS1 for the guest debugging target, I
> can't talk to it from my dev machine that has connected to ttyS1 with
> target machine (host).
>
> Is this a known problem?
>
> Thanks,
> Neo
>
> --
> I would remember that if researchers were not ambitious
> probably today we haven't the technology we are using!
>



-- 
I would remember that if researchers were not ambitious
probably today we haven't the technology we are using!

Re: [Qemu-devel] [PATCH] Use snapshots from backing disks

[Rob Earhart]

On Wed, Mar 17, 2010 at 8:35 AM, Anthony Liguori wrote:

> On 03/08/2010 07:13 PM, Rob Earhart wrote:
>
>> Modify the snapshot load path to find and load snapshots contained in
>> backing
>> disks, useful when the current disk is a differencing disk.
>>
>> Add the source of a snapshot when listing snapshots.
>>
>> This should only break backwards compatibility for scenarios depending on
>> not
>> being able to load snapshots from backing disks (which doesn't seem like a
>> problem), and for code which parses the snapshot list output (if any).
>>
>> Signed-off-by: Rob Earhart
>>
>>
>
> Your patch is whitespace damaged.  Please use a different mailer (like
> git-send-email).
>
> Regards,
>
> Anthony Liguori
>
>
If git-send-email worked in my environment, I'd be happy to.  :-)  I'll work
on it.  I still need to address Kevin's comments from last week before I
worry about that, though.

)Rob


>
>  ---
>>  block.c|   10 +
>>  block.h|2 +
>>  block/qcow2-snapshot.c |  105
>> 
>>  qemu-img.c |   25 +++-
>>  savevm.c   |   92 +-
>>  5 files changed, 170 insertions(+), 64 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 31d1ba4..3588700 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1481,6 +1481,16 @@ void bdrv_get_backing_filename(BlockDriverState
>> *bs,
>>  }
>>  }
>>
>> +const char *bdrv_get_filename(BlockDriverState *bs)
>> +{
>> +return bs->filename;
>> +}
>> +
>> +BlockDriverState* bdrv_get_backing_bdrv(BlockDriverState *bs)
>> +{
>> +return bs->backing_hd;
>> +}
>> +
>>  int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
>>const uint8_t *buf, int nb_sectors)
>>  {
>> diff --git a/block.h b/block.h
>> index edf5704..1d04322 100644
>> --- a/block.h
>> +++ b/block.h
>> @@ -179,6 +179,8 @@ int bdrv_get_info(BlockDriverState *bs,
>> BlockDriverInfo *bdi);
>>  const char *bdrv_get_encrypted_filename(BlockDriverState *bs);
>>  void bdrv_get_backing_filename(BlockDriverState *bs,
>> char *filename, int filename_size);
>> +const char *bdrv_get_filename(BlockDriverState *bs);
>> +BlockDriverState* bdrv_get_backing_bdrv(BlockDriverState *bs);
>>  int bdrv_snapshot_create(BlockDriverState *bs,
>>   QEMUSnapshotInfo *sn_info);
>>  int bdrv_snapshot_goto(BlockDriverState *bs,
>> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
>> index 8ddaea2..ca06fcf 100644
>> --- a/block/qcow2-snapshot.c
>> +++ b/block/qcow2-snapshot.c
>> @@ -191,16 +191,23 @@ static int qcow_write_snapshots(BlockDriverState
>> *bs)
>>  static void find_new_snapshot_id(BlockDriverState *bs,
>>   char *id_str, int id_str_size)
>>  {
>> -BDRVQcowState *s = bs->opaque;
>> -QCowSnapshot *sn;
>> -int i, id, id_max = 0;
>> +int id_max = 0;
>> +
>> +assert(bs);
>> +
>> +do {
>> +BDRVQcowState *s = bs->opaque;
>> +QCowSnapshot *sn;
>> +int i, id;
>> +
>> +for(i = 0; i<  s->nb_snapshots; i++) {
>> +sn = s->snapshots + i;
>> +id = strtoul(sn->id_str, NULL, 10);
>> +if (id>  id_max)
>> +id_max = id;
>> +}
>> +} while ((bs = bdrv_get_backing_bdrv(bs)));
>>
>> -for(i = 0; i<  s->nb_snapshots; i++) {
>> -sn = s->snapshots + i;
>> -id = strtoul(sn->id_str, NULL, 10);
>> -if (id>  id_max)
>> -id_max = id;
>> -}
>>  snprintf(id_str, id_str_size, "%d", id_max + 1);
>>  }
>>
>> @@ -316,41 +323,88 @@ int qcow2_snapshot_goto(BlockDriverState *bs,
>> const char *snapshot_id)
>>  {
>>  BDRVQcowState *s = bs->opaque;
>>  QCowSnapshot *sn;
>> -int i, snapshot_index, l1_size2;
>> +int i, snapshot_index, l1_size2, ret;
>>
>>  snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_id);
>> -if (snapshot_index<  0)
>> -return -ENOENT;
>> +
>> +if (snapshot_index<  0) {
>> +/* if there is a backing file, use it. In this case, the
>> + * current image needs to be reset to be consistent
>> + */
>> +if (bs->backing_hd) {
>> +ret = bdrv_snapshot_goto(bs->backing_hd, snapshot_id);
>> +if (ret<  0) {
>> +return ret;
>> +}
>> +/* reset the sectors of the current image (Note: there can
>> + * still be snapshots, so the refcounts must be kept
>> + * consistent)
>> + */
>> +if (qcow2_update_snapshot_refcount(bs, s->l1_table_offset,
>> +   s->l1_size, -1)<  0) {
>> +return -EIO;
>> +}
>> +l1_size2 = s->l1_size * sizeof(uint64_t);
>> +memset(s->l1_table, 0, l1_size2);
>> +if (bdrv_pwrite(s->hd, s->l1_table_offset,
>> +  

[Qemu-devel] [PATCH 2/6] error: Trim includes in qerror.c

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
---
 qerror.c |2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/qerror.c b/qerror.c
index d0aba61..ff2fbd5 100644
--- a/qerror.c
+++ b/qerror.c
@@ -11,9 +11,7 @@
  */
 #include "qjson.h"
 #include "qerror.h"
-#include "qstring.h"
 #include "qemu-common.h"
-#include "qemu-error.h"
 
 static void qerror_destroy_obj(QObject *obj);
 
-- 
1.6.6.1

[Qemu-devel] [PATCH 5/6] error: Link qemu-img, qemu-nbd, qemu-io with qemu-error.o

[Markus Armbruster]

The location tracking interface is used by code shared with qemi-img,
qemu-nbd and qemu-io, so it needs to be available there.  Commit
827b0813 provides it in a rather hamfisted way: it adds a dummy
implementation to qemu-tool.c.

It's cleaner to provide the real thing, and put a few more dummy
monitor functions into qemu-tool.c.

Signed-off-by: Markus Armbruster 
---
 Makefile|6 +++---
 qemu-tool.c |   50 --
 2 files changed, 19 insertions(+), 37 deletions(-)

diff --git a/Makefile b/Makefile
index 2066c12..aad361e 100644
--- a/Makefile
+++ b/Makefile
@@ -129,11 +129,11 @@ bt-host.o: QEMU_CFLAGS += $(BLUEZ_CFLAGS)
 qemu-img.o: qemu-img-cmds.h
 qemu-img.o qemu-tool.o qemu-nbd.o qemu-io.o: $(GENERATED_HEADERS)
 
-qemu-img$(EXESUF): qemu-img.o qemu-tool.o $(block-obj-y) $(qobject-obj-y)
+qemu-img$(EXESUF): qemu-img.o qemu-tool.o qemu-error.o $(block-obj-y) 
$(qobject-obj-y)
 
-qemu-nbd$(EXESUF): qemu-nbd.o qemu-tool.o $(block-obj-y) $(qobject-obj-y)
+qemu-nbd$(EXESUF): qemu-nbd.o qemu-tool.o qemu-error.o $(block-obj-y) 
$(qobject-obj-y)
 
-qemu-io$(EXESUF): qemu-io.o qemu-tool.o cmd.o $(block-obj-y) $(qobject-obj-y)
+qemu-io$(EXESUF): qemu-io.o cmd.o qemu-tool.o qemu-error.o $(block-obj-y) 
$(qobject-obj-y)
 
 qemu-img-cmds.h: $(SRC_PATH)/qemu-img-cmds.hx
$(call quiet-command,sh $(SRC_PATH)/hxtool -h < $< > $@,"  GEN   $@")
diff --git a/qemu-tool.c b/qemu-tool.c
index 97ca949..8d6d0ef 100644
--- a/qemu-tool.c
+++ b/qemu-tool.c
@@ -15,7 +15,6 @@
 #include "monitor.h"
 #include "qemu-timer.h"
 #include "qemu-log.h"
-#include "qemu-error.h"
 
 #include 
 
@@ -33,6 +32,22 @@ void qemu_service_io(void)
 {
 }
 
+Monitor *cur_mon;
+
+int monitor_cur_is_qmp(void)
+{
+return 0;
+}
+
+void monitor_set_error(Monitor *mon, QError *qerror)
+{
+assert(0);
+}
+
+void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
+{
+}
+
 void monitor_printf(Monitor *mon, const char *fmt, ...)
 {
 }
@@ -103,36 +118,3 @@ int64_t qemu_get_clock(QEMUClock *clock)
 qemu_gettimeofday(&tv);
 return (tv.tv_sec * 10LL + (tv.tv_usec * 1000)) / 100;
 }
-
-Location *loc_push_restore(Location *loc)
-{
-return loc;
-}
-
-Location *loc_push_none(Location *loc)
-{
-return loc;
-}
-
-Location *loc_pop(Location *loc)
-{
-return loc;
-}
-
-Location *loc_save(Location *loc)
-{
-return loc;
-}
-
-void loc_restore(Location *loc)
-{
-}
-
-void error_report(const char *fmt, ...)
-{
-va_list args;
-
-va_start(args, fmt);
-vfprintf(stderr, fmt, args);
-va_end(args);
-}
-- 
1.6.6.1

[Qemu-devel] [PATCH 4/6] error: Make use of error_set_progname() optional

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
---
 qemu-error.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/qemu-error.c b/qemu-error.c
index 5d5fe37..9b9c0a1 100644
--- a/qemu-error.c
+++ b/qemu-error.c
@@ -167,7 +167,7 @@ void error_print_loc(void)
 int i;
 const char *const *argp;
 
-if (!cur_mon) {
+if (!cur_mon && progname) {
 fprintf(stderr, "%s:", progname);
 sep = " ";
 }
-- 
1.6.6.1

[Qemu-devel] [PATCH 6/6] error: Move qerror_report() from qemu-error.[ch] to qerror.[ch]

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
---
 qemu-error.c |   18 --
 qemu-error.h |6 --
 qerror.c |   20 
 qerror.h |5 +
 4 files changed, 25 insertions(+), 24 deletions(-)

diff --git a/qemu-error.c b/qemu-error.c
index 9b9c0a1..57d7555 100644
--- a/qemu-error.c
+++ b/qemu-error.c
@@ -207,21 +207,3 @@ void error_report(const char *fmt, ...)
 va_end(ap);
 error_printf("\n");
 }
-
-void qerror_report_internal(const char *file, int linenr, const char *func,
-const char *fmt, ...)
-{
-va_list va;
-QError *qerror;
-
-va_start(va, fmt);
-qerror = qerror_from_info(file, linenr, func, fmt, &va);
-va_end(va);
-
-if (monitor_cur_is_qmp()) {
-monitor_set_error(cur_mon, qerror);
-} else {
-qerror_print(qerror);
-QDECREF(qerror);
-}
-}
diff --git a/qemu-error.h b/qemu-error.h
index e63c6ab..a45609f 100644
--- a/qemu-error.h
+++ b/qemu-error.h
@@ -37,11 +37,5 @@ void error_printf_unless_qmp(const char *fmt, ...)
 void error_print_loc(void);
 void error_set_progname(const char *argv0);
 void error_report(const char *fmt, ...) __attribute__ ((format(printf, 1, 2)));
-void qerror_report_internal(const char *file, int linenr, const char *func,
-const char *fmt, ...)
-__attribute__ ((format(printf, 4, 5)));
-
-#define qerror_report(fmt, ...) \
-qerror_report_internal(__FILE__, __LINE__, __func__, fmt, ## __VA_ARGS__)
 
 #endif
diff --git a/qerror.c b/qerror.c
index ff2fbd5..eaa1deb 100644
--- a/qerror.c
+++ b/qerror.c
@@ -9,6 +9,8 @@
  * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
  * See the COPYING.LIB file in the top-level directory.
  */
+
+#include "monitor.h"
 #include "qjson.h"
 #include "qerror.h"
 #include "qemu-common.h"
@@ -377,6 +379,24 @@ void qerror_print(QError *qerror)
 QDECREF(qstring);
 }
 
+void qerror_report_internal(const char *file, int linenr, const char *func,
+const char *fmt, ...)
+{
+va_list va;
+QError *qerror;
+
+va_start(va, fmt);
+qerror = qerror_from_info(file, linenr, func, fmt, &va);
+va_end(va);
+
+if (monitor_cur_is_qmp()) {
+monitor_set_error(cur_mon, qerror);
+} else {
+qerror_print(qerror);
+QDECREF(qerror);
+}
+}
+
 /**
  * qobject_to_qerror(): Convert a QObject into a QError
  */
diff --git a/qerror.h b/qerror.h
index d96abe1..dd298d4 100644
--- a/qerror.h
+++ b/qerror.h
@@ -37,6 +37,11 @@ QError *qerror_from_info(const char *file, int linenr, const 
char *func,
  const char *fmt, va_list *va);
 QString *qerror_human(const QError *qerror);
 void qerror_print(QError *qerror);
+void qerror_report_internal(const char *file, int linenr, const char *func,
+const char *fmt, ...)
+__attribute__ ((format(printf, 4, 5)));
+#define qerror_report(fmt, ...) \
+qerror_report_internal(__FILE__, __LINE__, __func__, fmt, ## __VA_ARGS__)
 QError *qobject_to_qerror(const QObject *obj);
 
 /*
-- 
1.6.6.1

[Qemu-devel] [PATCH 1/6] error: Trim includes after "Move qemu_error & friends..."

[Markus Armbruster]

Missed in commit 2f792016.

Signed-off-by: Markus Armbruster 
---
 hw/qdev-properties.c |1 +
 monitor.c|2 --
 sysemu.h |2 --
 3 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 92d6793..157a111 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -1,6 +1,7 @@
 #include "sysemu.h"
 #include "net.h"
 #include "qdev.h"
+#include "qerror.h"
 
 void *qdev_get_prop_ptr(DeviceState *dev, Property *prop)
 {
diff --git a/monitor.c b/monitor.c
index 0448a70..822dc27 100644
--- a/monitor.c
+++ b/monitor.c
@@ -49,10 +49,8 @@
 #include "qint.h"
 #include "qfloat.h"
 #include "qlist.h"
-#include "qdict.h"
 #include "qbool.h"
 #include "qstring.h"
-#include "qerror.h"
 #include "qjson.h"
 #include "json-streamer.h"
 #include "json-parser.h"
diff --git a/sysemu.h b/sysemu.h
index 8a9c630..9d3d51d 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -6,8 +6,6 @@
 #include "qemu-option.h"
 #include "qemu-queue.h"
 #include "qemu-timer.h"
-#include "qdict.h"
-#include "qerror.h"
 
 #ifdef _WIN32
 #include 
-- 
1.6.6.1

[Qemu-devel] [PATCH 0/6] error: Clean up after recent changes

[Markus Armbruster]

Cleaner integration of location tracking with qemu-tool.c.  Move
qerror_report() where it belongs.

Markus Armbruster (6):
  error: Trim includes after "Move qemu_error & friends..."
  error: Trim includes in qerror.c
  error: Trim includes after "Infrastructure to track locations..."
  error: Make use of error_set_progname() optional
  error: Link qemu-img, qemu-nbd, qemu-io with qemu-error.o
  error: Move qerror_report() from qemu-error.[ch] to qerror.[ch]

 Makefile |6 +++---
 hw/qdev-properties.c |1 +
 monitor.c|2 --
 monitor.h|1 -
 qemu-error.c |   20 +---
 qemu-error.h |6 --
 qemu-tool.c  |   50 --
 qerror.c |   22 --
 qerror.h |5 +
 sysemu.h |2 --
 10 files changed, 46 insertions(+), 69 deletions(-)

[Qemu-devel] [PATCH 3/6] error: Trim includes after "Infrastructure to track locations..."

[Markus Armbruster]

Missed in commit 827b0813.

Signed-off-by: Markus Armbruster 
---
 monitor.h |1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/monitor.h b/monitor.h
index bd4ae34..5bdeed1 100644
--- a/monitor.h
+++ b/monitor.h
@@ -3,7 +3,6 @@
 
 #include "qemu-common.h"
 #include "qemu-char.h"
-#include "qemu-error.h"
 #include "qerror.h"
 #include "qdict.h"
 #include "block.h"
-- 
1.6.6.1

Re: [Qemu-devel] [PATCH] Support for multiple keyboard device

[Anthony Liguori]

On 03/11/2010 03:57 PM, Shahar Havivi wrote:

Currently qemu use the last keyboard device that added,
When removing keyboard (via device_del kbd) you get segfault next time
you try to write in the client.

i.e. start qemu
   x86_64-softmmu/qemu-system-x86_64 -usb -device usb-kbd,id=kbd
switch to monitor
   device_del kbd
switch back to client, segfault

This patch fix the segfault and add list of all the keyboard handle much
like the mouse device does.


Signed-off-by: Shahar Havivi
   


It's a good idea, but I'd like to commit my rework of input.c first 
(which I'll do in a couple hours).  A few comments:



---
  console.h|9 +-
  hw/usb-hid.c |9 --
  input.c  |   81 +-
  3 files changed, 88 insertions(+), 11 deletions(-)

diff --git a/console.h b/console.h
index 71e8ff2..e250008 100644
--- a/console.h
+++ b/console.h
@@ -38,7 +38,14 @@ typedef struct QEMUPutLEDEntry {
  QTAILQ_ENTRY(QEMUPutLEDEntry) next;
  } QEMUPutLEDEntry;

-void qemu_add_kbd_event_handler(QEMUPutKBDEvent *func, void *opaque);
+typedef struct QEMUPutKbdEntry {
+QEMUPutKBDEvent *qemu_put_kbd_event;
+void *qemu_put_kbd_event_opaque;
+struct QEMUPutKbdEntry *next;
+} QEMUPutKbdEntry;
+
+QEMUPutKbdEntry *qemu_add_kbd_event_handler(QEMUPutKBDEvent *func, void 
*opaque);
+void qemu_remove_kbd_event_handler(QEMUPutKbdEntry *entry);
  QEMUPutMouseEntry *qemu_add_mouse_event_handler(QEMUPutMouseEvent *func,
  void *opaque, int absolute,
  const char *name);
diff --git a/hw/usb-hid.c b/hw/usb-hid.c
index 2e4e647..1dd0cc9 100644
--- a/hw/usb-hid.c
+++ b/hw/usb-hid.c
@@ -55,6 +55,7 @@ typedef struct USBKeyboardState {
  uint8_t leds;
  uint8_t key[16];
  int keys;
+QEMUPutKbdEntry *eh_entry;
  } USBKeyboardState;

  typedef struct USBHIDState {
@@ -635,7 +636,7 @@ static void usb_keyboard_handle_reset(USBDevice *dev)
  {
  USBHIDState *s = (USBHIDState *)dev;

-qemu_add_kbd_event_handler(usb_keyboard_event, s);
+s->kbd.eh_entry = qemu_add_kbd_event_handler(usb_keyboard_event, s);
  s->protocol = 1;
  }

@@ -856,9 +857,11 @@ static void usb_hid_handle_destroy(USBDevice *dev)
  {
  USBHIDState *s = (USBHIDState *)dev;

-if (s->kind != USB_KEYBOARD)
+if (s->kind != USB_KEYBOARD) {
  qemu_remove_mouse_event_handler(s->ptr.eh_entry);
-/* TODO: else */
+} else {
+qemu_remove_kbd_event_handler(s->kbd.eh_entry);
+}
  }

  static int usb_hid_initfn(USBDevice *dev, int kind)
diff --git a/input.c b/input.c
index baaa4c6..90b6cfb 100644
--- a/input.c
+++ b/input.c
@@ -29,16 +29,82 @@
  #include "qjson.h"


-static QEMUPutKBDEvent *qemu_put_kbd_event;
-static void *qemu_put_kbd_event_opaque;
+static QEMUPutKbdEntry *qemu_put_kbd_event_head;
+static QEMUPutKbdEntry *qemu_put_kbd_event_current;
  static QEMUPutMouseEntry *qemu_put_mouse_event_head;
  static QEMUPutMouseEntry *qemu_put_mouse_event_current;
  static QTAILQ_HEAD(, QEMUPutLEDEntry) led_handlers = 
QTAILQ_HEAD_INITIALIZER(led_handlers);

-void qemu_add_kbd_event_handler(QEMUPutKBDEvent *func, void *opaque)
+QEMUPutKbdEntry *qemu_add_kbd_event_handler(QEMUPutKBDEvent *func, void 
*opaque)
  {
-qemu_put_kbd_event_opaque = opaque;
-qemu_put_kbd_event = func;
+QEMUPutKbdEntry *s, *cursor;
+
+cursor = qemu_put_kbd_event_head;
+while (cursor) {
+if (cursor->qemu_put_kbd_event == func&&
+cursor->qemu_put_kbd_event_opaque == opaque) {
+
+qemu_put_kbd_event_current = cursor;
+return cursor;
+}
+cursor = cursor->next;
+}
+
+s = qemu_mallocz(sizeof(QEMUPutKbdEntry));
+
+s->qemu_put_kbd_event_opaque = opaque;
+s->qemu_put_kbd_event = func;
+s->next = NULL;
+
+if (!qemu_put_kbd_event_head) {
+qemu_put_kbd_event_head = s;
+qemu_put_kbd_event_current = s;
+return s;
+}
+
+cursor = qemu_put_kbd_event_head;
+while (cursor->next) {
+cursor = cursor->next;
+}
+
+cursor->next = s;
+qemu_put_kbd_event_current = s;
+
+return s;
+}
+
+void qemu_remove_kbd_event_handler(QEMUPutKbdEntry *entry)
+{
+QEMUPutKbdEntry *prev = NULL, *cursor;
+
+if (!qemu_put_kbd_event_head || !entry) {
+return;
+}
+
+cursor = qemu_put_kbd_event_head;
+while (cursor&&  cursor != entry) {
+prev = cursor;
+cursor = cursor->next;
+}
+
+if (cursor == NULL) {
+return;
+} else if (prev == NULL) {
+qemu_put_kbd_event_head = cursor->next;
+if (qemu_put_kbd_event_current == entry) {
+qemu_put_kbd_event_current = cursor->next;
+}
+qemu_free(entry);
+return;
+}
+
+prev->next = entry->next;
+
+if (qemu_put_kbd_event_current == entry) {
+qemu_put_kbd_event_current = prev;
+}
+
+qemu_free(entry);

Re: [Qemu-devel] [PATCH 08/16] tap: insert tap_can_send() into tap_send()

[Anthony Liguori]

On 03/11/2010 10:55 AM, Juan Quintela wrote:

This way we can remove the poll test

Signed-off-by: Juan Quintela
   


Won't this cause unnecessary wake ups?

Right now, we use the can_read() poll function to determine whether to 
add an fd to a fdset.  With this patch, we always add the fd to the 
fdset and then we just fail the read.


To eliminate the poll function, we need to change the handlers from 
polling, to noticing when they no longer can be read, and remove the 
read callback entirely.


Regards,

Anthony Liguori


---
  net/tap.c |   27 +--
  1 files changed, 9 insertions(+), 18 deletions(-)

diff --git a/net/tap.c b/net/tap.c
index 7a7320c..3ab65a3 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -61,17 +61,15 @@ typedef struct TAPState {

  static int launch_script(const char *setup_script, const char *ifname, int 
fd);

-static int tap_can_send(void *opaque);
  static void tap_send(void *opaque);
  static void tap_writable(void *opaque);

  static void tap_update_fd_handler(TAPState *s)
  {
-qemu_set_fd_handler2(s->fd,
- s->read_poll  ? tap_can_send : NULL,
- s->read_poll  ? tap_send : NULL,
- s->write_poll ? tap_writable : NULL,
- s);
+qemu_set_fd_handler(s->fd,
+s->read_poll  ? tap_send : NULL,
+s->write_poll ? tap_writable : NULL,
+s);
  }

  static void tap_read_poll(TAPState *s, int enable)
@@ -165,13 +163,6 @@ static ssize_t tap_receive(VLANClientState *nc, const 
uint8_t *buf, size_t size)
  return tap_write_packet(s, iov, 1);
  }

-static int tap_can_send(void *opaque)
-{
-TAPState *s = opaque;
-
-return qemu_can_send_packet(&s->nc);
-}
-
  #ifndef __sun__
  ssize_t tap_read_packet(int tapfd, uint8_t *buf, int maxlen)
  {
@@ -188,12 +179,10 @@ static void tap_send_completed(VLANClientState *nc, 
ssize_t len)
  static void tap_send(void *opaque)
  {
  TAPState *s = opaque;
-int size;

-do {
+while (qemu_can_send_packet(&s->nc)>  0) {
  uint8_t *buf = s->buf;
-
-size = tap_read_packet(s->fd, s->buf, sizeof(s->buf));
+int size = tap_read_packet(s->fd, s->buf, sizeof(s->buf));
  if (size<= 0) {
  break;
  }
@@ -206,8 +195,10 @@ static void tap_send(void *opaque)
  size = qemu_send_packet_async(&s->nc, buf, size, tap_send_completed);
  if (size == 0) {
  tap_read_poll(s, 0);
+} else if (size<  0) {
+return;
  }
-} while (size>  0&&  qemu_can_send_packet(&s->nc));
+}
  }

  int tap_has_ufo(VLANClientState *nc)
   

Re: [Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement

[Paul Brook]

> > If something should never happen (as in this case) then an abort/assert
> > is completely appropriate. Once things get that screwed up there's no
> > right answer, and the best thing we can do is terminate immediately to
> > try and avoid further damage.
> 
> This case was:
> 
> switch (foo & 0x03) {
> case 0: case 1: case 2: case 3:
> default:
> }
> 
> The default is unreachable.  Having it there just introduces more code
> that serves no purpose.  Unless someone does something totally foolish
> and changes the mask in the switch statement, there's no way it will
> ever be reachable.

I mistakenly remembered this was using a symbolic mask rather than a literal 
0x03. In that case I'd argue it's much easier to make the dumb error you 
describe, and the assert can be a handy cluebat.

I guess it's largely personal preference - I prefer to add the default case to 
make it clear that falling through is never gong to be the right answer.

Paul

Re: [Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement

[Anthony Liguori]

On 03/17/2010 12:08 PM, Paul Brook wrote:

On 03/17/2010 11:14 AM, Paul Bolle wrote:
 

On Wed, 2010-03-17 at 10:59 -0500, Anthony Liguori wrote:
   

On 03/08/2010 06:58 AM, Paul Bolle wrote:
 

Signed-off-by: Paul Bolle
   

Applied.  Thanks.
 

Paul Brook was "tempted to replace it by an abort()" (about one and a
half week ago). Did you perhaps miss that message or weren't you tempted
to do this?
   

I missed it, but then again, I don't think the patch was wrong in the
first place.

I think we use too many aborts/exits in the device model that can
potentially be triggered by guest code.
 

If something should never happen (as in this case) then an abort/assert is
completely appropriate. Once things get that screwed up there's no right
answer, and the best thing we can do is terminate immediately to try and avoid
further damage.
   


This case was:

switch (foo & 0x03) {
case 0: case 1: case 2: case 3:
default:
}

The default is unreachable.  Having it there just introduces more code 
that serves no purpose.  Unless someone does something totally foolish 
and changes the mask in the switch statement, there's no way it will 
ever be reachable.



If an assert/abort can be triggered by a guest then you obviously have a bug.
   


Agreed.


Removing the assert is not the correct solution.  You should either fix
whatever caused the invalid state to occur, or replace it with an appropriate
retry, fallback or guest visible failure.
   


Also agreed.

Regards,

Anthony Liguori


Paul
   

Re: [Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement

[Paul Brook]

> On 03/17/2010 11:14 AM, Paul Bolle wrote:
> > On Wed, 2010-03-17 at 10:59 -0500, Anthony Liguori wrote:
> >> On 03/08/2010 06:58 AM, Paul Bolle wrote:
> >>> Signed-off-by: Paul Bolle
> >>
> >> Applied.  Thanks.
> >
> > Paul Brook was "tempted to replace it by an abort()" (about one and a
> > half week ago). Did you perhaps miss that message or weren't you tempted
> > to do this?
> 
> I missed it, but then again, I don't think the patch was wrong in the
> first place.
> 
> I think we use too many aborts/exits in the device model that can
> potentially be triggered by guest code.

If something should never happen (as in this case) then an abort/assert is 
completely appropriate. Once things get that screwed up there's no right 
answer, and the best thing we can do is terminate immediately to try and avoid 
further damage.

If an assert/abort can be triggered by a guest then you obviously have a bug.

Removing the assert is not the correct solution.  You should either fix 
whatever caused the invalid state to occur, or replace it with an appropriate 
retry, fallback or guest visible failure.

Paul

Re: [Qemu-devel] [PATCH 3/4] tcg-hppa: Finish the port.

[Richard Henderson]

On 03/17/2010 07:56 AM, Richard Henderson wrote:
> Since Aurelien's generic div/rem patch I could simply remove all
> that millicode stuff, including that tcg_reg_free, and let the
> generic code handle this instead.  Which would get to the same
> millicode routine via one or two extra levels of indirection.

The following patch implements this, and does indeed work.


r~



diff --git a/tcg/hppa/tcg-target.c b/tcg/hppa/tcg-target.c
index fe96779..c6ba593 100644
--- a/tcg/hppa/tcg-target.c
+++ b/tcg/hppa/tcg-target.c
@@ -193,11 +193,6 @@ static int target_parse_constraint(TCGArgConstraint *ct, 
const char **pct_str)
 ct->ct |= TCG_CT_REG;
 tcg_regset_set32(ct->u.regs, 0, 0x);
 break;
-case 'c':
-/* millicode division return register.  */
-ct->ct | TCG_CT_REG;
-tcg_regset_set32(ct->u.regs, 0, 1u << TCG_REG_RET1);
-break;
 case 'L': /* qemu_ld/st constraint */
 ct->ct |= TCG_CT_REG;
 tcg_regset_set32(ct->u.regs, 0, 0x);
@@ -680,65 +675,6 @@ static void tcg_out_xmpyu(TCGContext *s, int retl, int 
reth,
 }
 }
 
-/* We define the following table directly in assembly so that we
-   can avoid getting PLABEL addresses.  */
-
-extern tcg_target_long qemu_milli[4];
-asm(".section .rodata\n\
-   .import $$divI,millicode\n\
-   .import $$remI,millicode\n\
-   .import $$divU,millicode\n\
-   .import $$remU,millicode\n\
-   .type   qemu_milli,@object\n\
-   .size   qemu_milli,16\n\
-qemu_milli:\n\
-   .word   $$divI\n\
-   .word   $$remI\n\
-   .word   $$divU\n\
-   .word   $$remU\n\
-   .previous");
-
-#define MILLI_DIVI 0
-#define MILLI_REMI 1
-#define MILLI_DIVU 2
-#define MILLI_REMU 3
-
-/* ??? Move this up in tcg.c.  */
-static void tcg_reg_free(TCGContext *, int);
-
-/* Note that the return is constrained to be r29, and the inputs are
-   constrained to not be in the first two parameter registers.  Otherwise,
-   we clobber r1 and r31, which are already marked as reserved.  */
-static void tcg_out_milli(TCGContext *s, int arg1, int arg2, int which)
-{
-tcg_target_long routine, disp;
-
-/* The parameter registers are clobbered.  Store them back to their
-   stack slots before we lose the contents.  */
-tcg_reg_free(s, TCG_REG_R26);
-tcg_reg_free(s, TCG_REG_R25);
-tcg_out_mov(s, TCG_REG_R26, arg1);
-
-routine = qemu_milli[which];
-disp = (routine - ((tcg_target_long)s->code_ptr + 8)) >> 2;
-
-if (check_fit_tl(disp, 17)) {
-tcg_out32(s, INSN_BL | INSN_R2(TCG_REG_R31) | reassemble_17(disp));
-} else {
-uint32_t hi, lo;
-
-hi = routine >> 11;
-lo = routine & 0x7ff;
-
-tcg_out32(s, INSN_LDIL | INSN_R2(TCG_REG_R1) | reassemble_21(hi));
-tcg_out32(s, INSN_BLE_SR4 | INSN_R2(TCG_REG_R1)
- | reassemble_17(lo >> 2));
-}
-
-/* Second input copy in call delay slot.  */
-tcg_out_mov(s, TCG_REG_R25, arg2);
-}
-
 static void tcg_out_branch(TCGContext *s, int label_index, int nul)
 {
 TCGLabel *l = &s->labels[label_index];
@@ -1443,19 +1379,6 @@ static inline void tcg_out_op(TCGContext *s, int opc, 
const TCGArg *args,
 tcg_out_xmpyu(s, args[0], args[1], args[2], args[3]);
 break;
 
-case INDEX_op_div_i32:
-tcg_out_milli(s, args[1], args[2], MILLI_DIVI);
-break;
-case INDEX_op_divu_i32:
-tcg_out_milli(s, args[1], args[2], MILLI_DIVU);
-break;
-case INDEX_op_rem_i32:
-tcg_out_milli(s, args[1], args[2], MILLI_REMI);
-break;
-case INDEX_op_remu_i32:
-tcg_out_milli(s, args[1], args[2], MILLI_REMU);
-break;
-
 case INDEX_op_bswap16_i32:
 tcg_out_bswap16(s, args[0], args[1], 0);
 break;
@@ -1590,11 +1513,6 @@ static const TCGTargetOpDef hppa_op_defs[] = {
 { INDEX_op_mul_i32, { "r", "r", "r" } },
 { INDEX_op_mulu2_i32, { "r", "r", "r", "r" } },
 
-{ INDEX_op_div_i32, { "c", "L", "L" } },
-{ INDEX_op_divu_i32, { "c", "L", "L" } },
-{ INDEX_op_rem_i32, { "c", "L", "L" } },
-{ INDEX_op_remu_i32, { "c", "L", "L" } },
-
 { INDEX_op_shl_i32, { "r", "r", "ri" } },
 { INDEX_op_shr_i32, { "r", "r", "ri" } },
 { INDEX_op_sar_i32, { "r", "r", "ri" } },
diff --git a/tcg/hppa/tcg-target.h b/tcg/hppa/tcg-target.h
index cd44deb..007aa7a 100644
--- a/tcg/hppa/tcg-target.h
+++ b/tcg/hppa/tcg-target.h
@@ -82,7 +82,7 @@ enum {
 #define TCG_TARGET_STACK_GROWSUP
 
 /* optional instructions */
-#define TCG_TARGET_HAS_div_i32
+// #define TCG_TARGET_HAS_div_i32
 #define TCG_TARGET_HAS_rot_i32
 #define TCG_TARGET_HAS_ext8s_i32
 #define TCG_TARGET_HAS_ext16s_i32

Re: [Qemu-devel] [PATCH v2 1/2] read-only: minor cleanup

[Anthony Liguori]

On 03/14/2010 08:19 AM, Naphtali Sprei wrote:

Really use read-only flags for opening the file when asked for read-only

Signed-off-by: Naphtali Sprei
   


Applied all.  Thanks.

Regards,

Anthony Liguori

---
  qemu-nbd.c |1 +
  1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index eac0c21..a393583 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -258,6 +258,7 @@ int main(int argc, char **argv)
  break;
  case 'r':
  readonly = true;
+flags&= ~BDRV_O_RDWR;
  break;
  case 'P':
  partition = strtol(optarg,&end, 0);
   

Re: [Qemu-devel] [PATCH] scsi-disk: fix buffer overflow

[Anthony Liguori]

On 03/10/2010 10:47 AM, Gerd Hoffmann wrote:

In case s->version is shorter than 4 bytes we overflow the memcpy src
buffer.  Fix it by clearing the target buffer, then copy only the
amount of bytes we actually have.

Signed-off-by: Gerd Hoffmann
   


Applied.  Thanks.

Regards,

Anthony Liguori

---
  hw/scsi-disk.c |4 +++-
  1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index b2f61fe..5792545 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -459,7 +459,9 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, 
uint8_t *outbuf)
  memcpy(&outbuf[16], "QEMU HARDDISK   ", 16);
  }
  memcpy(&outbuf[8], "QEMU", 8);
-memcpy(&outbuf[32], s->version ? s->version : QEMU_VERSION, 4);
+memset(&outbuf[32], 0, 4);
+memcpy(&outbuf[32], s->version ? s->version : QEMU_VERSION,
+   MIN(4, strlen(s->version ? s->version : QEMU_VERSION)));
  /* Identify device as SCSI-3 rev 1.
 Some later commands are also implemented. */
  outbuf[2] = 5;
   

Re: [Qemu-devel] [PATCH] vnc: add no-lock-key-sync option

[Anthony Liguori]

On 03/10/2010 10:12 AM, Gerd Hoffmann wrote:

Add an option to disable the heuristics which try to keep
capslock and numlock state for guest and host in sync.

Signed-off-by: Gerd Hoffmann
   


Applied.  Thanks.

Regards,

Anthony Liguori

---
  vnc.c |   16 
  vnc.h |1 +
  2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/vnc.c b/vnc.c
index 38690e2..3cfe2ca 100644
--- a/vnc.c
+++ b/vnc.c
@@ -,7 +,8 @@ static void vnc_disconnect_finish(VncState *vs)
  }

  vnc_remove_timer(vs->vd);
-qemu_remove_led_event_handler(vs->led);
+if (vs->vd->lock_key_sync)
+qemu_remove_led_event_handler(vs->led);
  qemu_free(vs);
  }

@@ -1543,7 +1544,8 @@ static void do_key_event(VncState *vs, int down, int 
keycode, int sym)
  break;
  }

-if (keycode_is_keypad(vs->vd->kbd_layout, keycode)) {
+if (vs->vd->lock_key_sync&&
+keycode_is_keypad(vs->vd->kbd_layout, keycode)) {
  /* If the numlock state needs to change then simulate an additional
 keypress before sending this one.  This will happen if the user
 toggles numlock away from the VNC window.
@@ -1561,7 +1563,8 @@ static void do_key_event(VncState *vs, int down, int 
keycode, int sym)
  }
  }

-if ((sym>= 'A'&&  sym<= 'Z') || (sym>= 'a'&&  sym<= 'z')) {
+if (vs->vd->lock_key_sync&&
+((sym>= 'A'&&  sym<= 'Z') || (sym>= 'a'&&  sym<= 'z'))) {
  /* If the capslock state needs to change then simulate an additional
 keypress before sending this one.  This will happen if the user
 toggles capslock away from the VNC window.
@@ -2424,7 +2427,8 @@ static void vnc_connect(VncDisplay *vd, int csock)
  vnc_flush(vs);
  vnc_read_when(vs, protocol_version, 12);
  reset_keys(vs);
-vs->led = qemu_add_led_event_handler(kbd_leds, vs);
+if (vs->vd->lock_key_sync)
+vs->led = qemu_add_led_event_handler(kbd_leds, vs);

  vnc_init_timer(vd);

@@ -2545,6 +2549,7 @@ int vnc_display_open(DisplayState *ds, const char 
*display)
  int saslErr;
  #endif
  int acl = 0;
+int lock_key_sync = 1;

  if (!vnc_display)
  return -1;
@@ -2562,6 +2567,8 @@ int vnc_display_open(DisplayState *ds, const char 
*display)
  password = 1; /* Require password auth */
  } else if (strncmp(options, "reverse", 7) == 0) {
  reverse = 1;
+} else if (strncmp(options, "no-lock-key-sync", 9) == 0) {
+lock_key_sync = 0;
  #ifdef CONFIG_VNC_SASL
  } else if (strncmp(options, "sasl", 4) == 0) {
  sasl = 1; /* Require SASL auth */
@@ -2707,6 +2714,7 @@ int vnc_display_open(DisplayState *ds, const char 
*display)
  return -1;
  }
  #endif
+vs->lock_key_sync = lock_key_sync;

  if (reverse) {
  /* connect to viewer */
diff --git a/vnc.h b/vnc.h
index 0fc89bd..a4211ae 100644
--- a/vnc.h
+++ b/vnc.h
@@ -99,6 +99,7 @@ struct VncDisplay
  int lsock;
  DisplayState *ds;
  kbd_layout_t *kbd_layout;
+int lock_key_sync;

  struct VncSurface guest;   /* guest visible surface (aka ds->surface) */
  DisplaySurface *server;  /* vnc server surface */
   

Re: [Qemu-devel] [PATCH 01/18] avoid dubiously clever code in win32_start_timer

[Anthony Liguori]

On 03/10/2010 04:38 AM, Paolo Bonzini wrote:

The code is initializing an unsigned int to UINT_MAX using "-1", so that
the following always-true comparison seems to be always-false at a
first look.  Since alarm timer initializations are never nested, it is
simpler to unconditionally store the result of timeGetDevCaps into
data->period.

Signed-off-by: Paolo Bonzini
   


Applied all.  Thanks.

Nice cleanup.

Regards,

Anthony Liguori


---
  vl.c |6 ++
  1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/vl.c b/vl.c
index d8328c7..6b1e1a7 100644
--- a/vl.c
+++ b/vl.c
@@ -626,7 +626,7 @@ static struct qemu_alarm_timer *alarm_timer;
  struct qemu_alarm_win32 {
  MMRESULT timerId;
  unsigned int period;
-} alarm_win32_data = {0, -1};
+} alarm_win32_data = {0, 0};

  static int win32_start_timer(struct qemu_alarm_timer *t);
  static void win32_stop_timer(struct qemu_alarm_timer *t);
@@ -1360,9 +1360,7 @@ static int win32_start_timer(struct qemu_alarm_timer *t)
  memset(&tc, 0, sizeof(tc));
  timeGetDevCaps(&tc, sizeof(tc));

-if (data->period<  tc.wPeriodMin)
-data->period = tc.wPeriodMin;
-
+data->period = tc.wPeriodMin;
  timeBeginPeriod(data->period);

  flags = TIME_CALLBACK_FUNCTION;
   

Re: [Qemu-devel] [PATCH] hw/usb-msd: fix some usb requests

[Anthony Liguori]

On 03/10/2010 03:45 AM, Arnaud Patard (Rtp) wrote:

The usb-msd device emulation needs some small tweaks in the requests
emulations. For instance, the reset/maxlun requests are class/interface
specific so requests for them with the type class and recipient interface
bits sets have to be handled.

Signed-off-by: Arnaud Patard
---
   

Applied.  Thanks.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 1/2] migration: Clear fd also in error cases

[Anthony Liguori]

On 03/09/2010 05:10 PM, Juan Quintela wrote:

Not clearing the fd and closing the file makes qemu spin using 100%CPU
after incoming migration error.

See for instance bug:
https://bugzilla.redhat.com/show_bug.cgi?id=518032

Signed-off-by: Juan Quintela
   


Applied all.  Thanks.

Regards,

Anthony Liguori


---
  migration-exec.c |4 ++--
  migration-fd.c   |4 ++--
  migration-tcp.c  |5 ++---
  migration-unix.c |5 ++---
  4 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/migration-exec.c b/migration-exec.c
index 3edc026..6ff8449 100644
--- a/migration-exec.c
+++ b/migration-exec.c
@@ -120,12 +120,12 @@ static void exec_accept_incoming_migration(void *opaque)
  }
  qemu_announce_self();
  DPRINTF("successfully loaded vm state\n");
-/* we've successfully migrated, close the fd */
-qemu_set_fd_handler2(qemu_stdio_fd(f), NULL, NULL, NULL, NULL);
+
  if (autostart)
  vm_start();

  err:
+qemu_set_fd_handler2(qemu_stdio_fd(f), NULL, NULL, NULL, NULL);
  qemu_fclose(f);
  }

diff --git a/migration-fd.c b/migration-fd.c
index 0cc74ad..9cf52ce 100644
--- a/migration-fd.c
+++ b/migration-fd.c
@@ -113,12 +113,12 @@ static void fd_accept_incoming_migration(void *opaque)
  }
  qemu_announce_self();
  DPRINTF("successfully loaded vm state\n");
-/* we've successfully migrated, close the fd */
-qemu_set_fd_handler2(qemu_stdio_fd(f), NULL, NULL, NULL, NULL);
+
  if (autostart)
  vm_start();

  err:
+qemu_set_fd_handler2(qemu_stdio_fd(f), NULL, NULL, NULL, NULL);
  qemu_fclose(f);
  }

diff --git a/migration-tcp.c b/migration-tcp.c
index e7f307c..95ce722 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -170,15 +170,14 @@ static void tcp_accept_incoming_migration(void *opaque)
  qemu_announce_self();
  DPRINTF("successfully loaded vm state\n");

-/* we've successfully migrated, close the server socket */
-qemu_set_fd_handler2(s, NULL, NULL, NULL, NULL);
-close(s);
  if (autostart)
  vm_start();

  out_fopen:
  qemu_fclose(f);
  out:
+qemu_set_fd_handler2(s, NULL, NULL, NULL, NULL);
+close(s);
  close(c);
  }

diff --git a/migration-unix.c b/migration-unix.c
index b7aab38..ce59a2a 100644
--- a/migration-unix.c
+++ b/migration-unix.c
@@ -176,13 +176,12 @@ static void unix_accept_incoming_migration(void *opaque)
  qemu_announce_self();
  DPRINTF("successfully loaded vm state\n");

-/* we've successfully migrated, close the server socket */
-qemu_set_fd_handler2(s, NULL, NULL, NULL, NULL);
-close(s);

  out_fopen:
  qemu_fclose(f);
  out:
+qemu_set_fd_handler2(s, NULL, NULL, NULL, NULL);
+close(s);
  close(c);
  }

   

Re: [Qemu-devel] [PATCH] Fix SIGFPE for vnc display of width/height = 1

[Alexander Graf]

Anthony Liguori wrote:
> On 03/08/2010 08:34 AM, Chris Webb wrote:
>> During boot, the screen gets resized to height 1 and a mouse click at
>> this
>> point will cause a division by zero when calculating the absolute
>> pointer
>> position from the pixel (x, y). Return a click in the middle of the
>> screen
>> instead in this case.
>>
>> Signed-off-by: Chris Webb
>>
> Applied.  Thanks.

Also queued it to stable?


Alex

[Qemu-devel] [PATCH] monitor: convert do_device_del() to QObject, QError

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
---
 hw/qdev.c   |7 ---
 hw/qdev.h   |2 +-
 qemu-monitor.hx |3 ++-
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 17a46a7..35460eb 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -800,15 +800,16 @@ int do_device_add(Monitor *mon, const QDict *qdict, 
QObject **ret_data)
 return 0;
 }
 
-void do_device_del(Monitor *mon, const QDict *qdict)
+int do_device_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
 const char *id = qdict_get_str(qdict, "id");
 DeviceState *dev;
 
 dev = qdev_find_recursive(main_system_bus, id);
 if (NULL == dev) {
-error_report("Device '%s' not found", id);
-return;
+qerror_report(QERR_DEVICE_NOT_FOUND, id);
+return -1;
 }
 qdev_unplug(dev);
+return 0;
 }
diff --git a/hw/qdev.h b/hw/qdev.h
index 9475705..40373c8 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -176,7 +176,7 @@ void qbus_free(BusState *bus);
 void do_info_qtree(Monitor *mon);
 void do_info_qdm(Monitor *mon);
 int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data);
-void do_device_del(Monitor *mon, const QDict *qdict);
+int do_device_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
 
 /*** qdev-properties.c ***/
 
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 5308f36..d290b4b 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -589,7 +589,8 @@ ETEXI
 .args_type  = "id:s",
 .params = "device",
 .help   = "remove device",
-.mhandler.cmd = do_device_del,
+.user_print = monitor_user_noop,
+.mhandler.cmd_new = do_device_del,
 },
 
 STEXI
-- 
1.6.6.1

Re: [Qemu-devel] [PATCH] better describe -net options

[Anthony Liguori]

On 03/17/2010 11:13 AM, Jamie Lokier wrote:

Anthony Liguori wrote:
   

On 03/05/2010 03:07 PM, Brian Thomason wrote:

  The manpage reflects that multiple -net user calls may be made,
  but if this is done, it can cause the program to hang. Instead,
  multiple -net calls can be combined into one, and this patch adds
  that information to qemu-options.hx. Also, -net user may only
  be used in conjunction with -net nic.  This is already implcitly
  stated, but this patch makes that statement more explicit.

Actually, the problem is that the user created a loop.  It's actually
valid to have something like:
-net user -net dump,file=foo.pcap -net nic
But having -net user -net user creates a loop.
 

When I used -net user -net user, it didn't hang: QEMU crashed with a
stack overflow.

It crashed after a doing a successful full OS install, because
everything was fine until the first network packet.

That's not nice, even if it is user error.
   


Well we should dedicate this case in the code and throw an error.

Regards,

Anthony Liguori


-- Jamie
   

Re: [Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement

[Anthony Liguori]

On 03/17/2010 11:14 AM, Paul Bolle wrote:

On Wed, 2010-03-17 at 10:59 -0500, Anthony Liguori wrote:
   

On 03/08/2010 06:58 AM, Paul Bolle wrote:
 

Signed-off-by: Paul Bolle

   

Applied.  Thanks.
 

Paul Brook was "tempted to replace it by an abort()" (about one and a
half week ago). Did you perhaps miss that message or weren't you tempted
to do this?
   


I missed it, but then again, I don't think the patch was wrong in the 
first place.


I think we use too many aborts/exits in the device model that can 
potentially be triggered by guest code.


Regards,

Anthony Liguori


Regards,


Paul Bolle

   

Re: [Qemu-devel] Re: [PATCH 2/6] qemu-kvm: Modify and introduce wrapper functions to access phys_ram_dirty.

[Avi Kivity]

On 03/17/2010 06:06 PM, Paul Brook wrote:

On 03/16/2010 10:10 PM, Blue Swirl wrote:
 

   Yes, and is what tlb_protect_code() does and it's called from
tb_alloc_page() which is what's code when a TB is created.
 

Just a tangential note: a long time ago, I tried to disable self
modifying code detection for Sparc. On most RISC architectures, SMC
needs explicit flushing so in theory we need not track code memory
writes. However, during exceptions the translator needs to access the
original unmodified code that was used to generate the TB. But maybe
there are other ways to avoid SMC tracking, on x86 it's still needed
   

On x86 you're supposed to execute a serializing instruction (one of
INVD, INVEPT, INVLPG, INVVPID, LGDT, LIDT, LLDT, LTR, MOV (to control
register, with the exception of MOV CR8), MOV (to debug register),
WBINVD, WRMSR, CPUID, IRET, and RSM) before running modified code.
 

Last time I checked, a jump instruction was sufficient to ensure coherency
withing a core.  Serializing instructions are only required for coherency
between cores on SMP systems.
   


Yeah, the docs say either a jump or a serializing instruction is needed.


QEMU effectively has a very large physically tagged icache[1] with very
expensive cache loads.  AFAIK The only practical way to maintain that cache on
x86 targets is to do write snooping via dirty bits. On targets that mandate
explicit icache invalidation we might be able to get away with this, however I
doubt it actually gains you anything - a correctly written guest is going to
invalidate at least as much as we get from dirty tracking, and we still need
to provide correct behaviour when executing with cache disabled.
   


Agreed.

   

but I suppose SMC is pretty rare.
   

Every time you demand load a code page from disk, you're running self
modifying code (though it usually doesn't exist in the tlb, so there's
no previous version that can cause trouble).
 

I think you're confusing TLB flushes with TB flushes.
   


No - my thinking was page fault, load page, invlpg, continue.  But the 
invlpg is unneeded, and "continue" has to include a jump anyway.


--
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] better describe -net options

[Jamie Lokier]

Anthony Liguori wrote:
> 
>On 03/05/2010 03:07 PM, Brian Thomason wrote:
> 
>  The manpage reflects that multiple -net user calls may be made,
>  but if this is done, it can cause the program to hang. Instead,
>  multiple -net calls can be combined into one, and this patch adds
>  that information to qemu-options.hx. Also, -net user may only
>  be used in conjunction with -net nic.  This is already implcitly
>  stated, but this patch makes that statement more explicit.
> 
>Actually, the problem is that the user created a loop.  It's actually
>valid to have something like:
>-net user -net dump,file=foo.pcap -net nic
>But having -net user -net user creates a loop.

When I used -net user -net user, it didn't hang: QEMU crashed with a
stack overflow.

It crashed after a doing a successful full OS install, because
everything was fine until the first network packet.

That's not nice, even if it is user error.

-- Jamie

Re: [Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement

[Paul Bolle]

On Wed, 2010-03-17 at 10:59 -0500, Anthony Liguori wrote:
> On 03/08/2010 06:58 AM, Paul Bolle wrote:
> > Signed-off-by: Paul Bolle
> >
> Applied.  Thanks.

Paul Brook was "tempted to replace it by an abort()" (about one and a
half week ago). Did you perhaps miss that message or weren't you tempted
to do this?

Regards,


Paul Bolle

Re: [Qemu-devel] [PATCH QEMU] Transparent Hugepage Support #3

[Andrea Arcangeli]

On Wed, Mar 17, 2010 at 04:07:09PM +, Paul Brook wrote:
> > On Wed, Mar 17, 2010 at 03:52:15PM +, Paul Brook wrote:
> > > > > > Size not multiple I think is legitimate, the below-4G chunk isn't
> > > > > > required to end 2M aligned, all it matters is that the above-4G
> > > > > > then starts aligned. In short one thing to add in the future as
> > > > > > parameter to qemu_ram_alloc is the physical address that the host
> > > > > > virtual address corresponds to.
> > > > >
> > > > > In general you don't know this at allocation time.
> > > >
> > > > Caller knows it, it's not like the caller is outside of qemu, it's not
> > > > some library. We know this is enough with the caller that there is now.
> > >
> > > No we don't.  As discussed previously, there are machines where the
> > > physical location of RAM is configurable at runtime.  In fact it's common
> > > for the ram to be completely absent at reset.
> > 
> > This is why PREFERRED_RAM_ALIGN is only defined for __x86_64__. I'm
> > not talking about other archs that may never support transparent
> > hugepages in the kernel because of other architectural constrains that
> > may prevent to map hugepages mixed with regular pages in the same vma.
> 
> __x86__64 only tells you about the host. I'm talking about the guest machine.

When it's qemu and not kvm (so when the guest might not be x86 arch) the
guest physical address becomes as irrelevant as the size and only the
host virtual address has to start 2M aligned on x86_64 host.

I think this already takes care of all practical issues, and there's
no need of further work until pc.c will start allocating chunks of ram
starting at guest physical addresses not 2M aligned. Maybe if we add
memory hotplug or something.

Re: [Qemu-devel] [PATCH 1/2] QError: New QERR_DEVICE_NOT_ENCRYPTED

[Anthony Liguori]

On 03/05/2010 04:25 PM, Shahar Havivi wrote:

Signed-off-by: Shahar Havivi
   

Applied.  Thanks.

Regards,

Anthony Liguori

---
  qerror.c |4 
  qerror.h |3 +++
  2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/qerror.c b/qerror.c
index 2f657f4..4e63a54 100644
--- a/qerror.c
+++ b/qerror.c
@@ -49,6 +49,10 @@ static const QErrorStringTable qerror_table[] = {
  .desc  = "The %(device) is encrypted",
  },
  {
+.error_fmt = QERR_DEVICE_NOT_ENCRYPTED,
+.desc  = "Device '%(device)' is not encrypted",
+},
+{
  .error_fmt = QERR_DEVICE_LOCKED,
  .desc  = "Device %(device) is locked",
  },
diff --git a/qerror.h b/qerror.h
index ee59615..b93fff6 100644
--- a/qerror.h
+++ b/qerror.h
@@ -46,6 +46,9 @@ QError *qobject_to_qerror(const QObject *obj);
  #define QERR_DEVICE_ENCRYPTED \
  "{ 'class': 'DeviceEncrypted', 'data': { 'device': %s } }"

+#define QERR_DEVICE_NOT_ENCRYPTED \
+"{ 'class': 'DeviceNotEncrypted', 'data': { 'device': %s } }"
+
  #define QERR_DEVICE_LOCKED  \
  "{ 'class': 'DeviceLocked', 'data': { 'device': %s } }"

   

Re: [Qemu-devel] [PATCH QEMU] Transparent Hugepage Support #3

[Paul Brook]

> On Wed, Mar 17, 2010 at 03:52:15PM +, Paul Brook wrote:
> > > > > Size not multiple I think is legitimate, the below-4G chunk isn't
> > > > > required to end 2M aligned, all it matters is that the above-4G
> > > > > then starts aligned. In short one thing to add in the future as
> > > > > parameter to qemu_ram_alloc is the physical address that the host
> > > > > virtual address corresponds to.
> > > >
> > > > In general you don't know this at allocation time.
> > >
> > > Caller knows it, it's not like the caller is outside of qemu, it's not
> > > some library. We know this is enough with the caller that there is now.
> >
> > No we don't.  As discussed previously, there are machines where the
> > physical location of RAM is configurable at runtime.  In fact it's common
> > for the ram to be completely absent at reset.
> 
> This is why PREFERRED_RAM_ALIGN is only defined for __x86_64__. I'm
> not talking about other archs that may never support transparent
> hugepages in the kernel because of other architectural constrains that
> may prevent to map hugepages mixed with regular pages in the same vma.

__x86__64 only tells you about the host. I'm talking about the guest machine.

Paul

Re: [Qemu-devel] Re: [PATCH 2/6] qemu-kvm: Modify and introduce wrapper functions to access phys_ram_dirty.

[Paul Brook]

> On 03/16/2010 10:10 PM, Blue Swirl wrote:
> >>   Yes, and is what tlb_protect_code() does and it's called from
> >> tb_alloc_page() which is what's code when a TB is created.
> >
> > Just a tangential note: a long time ago, I tried to disable self
> > modifying code detection for Sparc. On most RISC architectures, SMC
> > needs explicit flushing so in theory we need not track code memory
> > writes. However, during exceptions the translator needs to access the
> > original unmodified code that was used to generate the TB. But maybe
> > there are other ways to avoid SMC tracking, on x86 it's still needed
> 
> On x86 you're supposed to execute a serializing instruction (one of
> INVD, INVEPT, INVLPG, INVVPID, LGDT, LIDT, LLDT, LTR, MOV (to control
> register, with the exception of MOV CR8), MOV (to debug register),
> WBINVD, WRMSR, CPUID, IRET, and RSM) before running modified code.

Last time I checked, a jump instruction was sufficient to ensure coherency 
withing a core.  Serializing instructions are only required for coherency 
between cores on SMP systems.

QEMU effectively has a very large physically tagged icache[1] with very 
expensive cache loads.  AFAIK The only practical way to maintain that cache on 
x86 targets is to do write snooping via dirty bits. On targets that mandate 
explicit icache invalidation we might be able to get away with this, however I 
doubt it actually gains you anything - a correctly written guest is going to 
invalidate at least as much as we get from dirty tracking, and we still need 
to provide correct behaviour when executing with cache disabled.

> > but I suppose SMC is pretty rare.
> 
> Every time you demand load a code page from disk, you're running self
> modifying code (though it usually doesn't exist in the tlb, so there's
> no previous version that can cause trouble).

I think you're confusing TLB flushes with TB flushes.

Paul

[1] Even modern x86 only have relatively small icache. The large L2/L3 caches 
aren't relevant as they are unified I/D caches.

Re: [Qemu-devel] [PATCH] balloon: Do not save VM state wrt asynchronous virtio operations

[Anthony Liguori]

On 03/09/2010 12:54 PM, Adam Litke wrote:

When working with the VM state (for loadvm/savevm and migration), it is not
valid to load and store pointers since the validity of those pointers cannot be
assured in the new qemu address space.  Therefore, virtio_balloon_save() and
virtio_balloon_load() must not handle the stats-related fields in struct
VirtIOBalloon.

If a memory stats request is in-flight at the time of a migration or savevm,
the request will not complete and should be resubmitted once migration or
loadvm completes.  Note that this extremely small race window can only be
triggered using QMP so it is not possible to hang the user monitor.

Signed-off-by: Adam Litke
   

Applied.  Thanks.

Regards,

Anthony Liguori

diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index 086d9d1..6d12024 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -261,10 +261,6 @@ static void virtio_balloon_save(QEMUFile *f, void *opaque)

  qemu_put_be32(f, s->num_pages);
  qemu_put_be32(f, s->actual);
-qemu_put_buffer(f, (uint8_t *)&s->stats_vq_elem, sizeof(VirtQueueElement));
-qemu_put_buffer(f, (uint8_t *)&s->stats_vq_offset, sizeof(size_t));
-qemu_put_buffer(f, (uint8_t *)&s->stats_callback, 
sizeof(MonitorCompletion));
-qemu_put_buffer(f, (uint8_t *)&s->stats_opaque_callback_data, 
sizeof(void));
  }

  static int virtio_balloon_load(QEMUFile *f, void *opaque, int version_id)
@@ -278,11 +274,6 @@ static int virtio_balloon_load(QEMUFile *f, void *opaque, 
int version_id)

  s->num_pages = qemu_get_be32(f);
  s->actual = qemu_get_be32(f);
-qemu_get_buffer(f, (uint8_t *)&s->stats_vq_elem, sizeof(VirtQueueElement));
-qemu_get_buffer(f, (uint8_t *)&s->stats_vq_offset, sizeof(size_t));
-qemu_get_buffer(f, (uint8_t *)&s->stats_callback, 
sizeof(MonitorCompletion));
-qemu_get_buffer(f, (uint8_t *)&s->stats_opaque_callback_data, 
sizeof(void));
-
  return 0;
  }



   

Re: [Qemu-devel] [PATCH] Fix SIGFPE for vnc display of width/height = 1

[Anthony Liguori]

On 03/08/2010 08:34 AM, Chris Webb wrote:

During boot, the screen gets resized to height 1 and a mouse click at this
point will cause a division by zero when calculating the absolute pointer
position from the pixel (x, y). Return a click in the middle of the screen
instead in this case.

Signed-off-by: Chris Webb
   

Applied.  Thanks.

Regards,

Anthony Liguori

---
  vnc.c |6 --
  1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/vnc.c b/vnc.c
index 01353a9..676a707 100644
--- a/vnc.c
+++ b/vnc.c
@@ -1457,8 +1457,10 @@ static void pointer_event(VncState *vs, int button_mask, 
int x, int y)
  dz = 1;

  if (vs->absolute) {
-kbd_mouse_event(x * 0x7FFF / (ds_get_width(vs->ds) - 1),
-y * 0x7FFF / (ds_get_height(vs->ds) - 1),
+kbd_mouse_event(ds_get_width(vs->ds)>  1 ?
+  x * 0x7FFF / (ds_get_width(vs->ds) - 1) : 0x4000,
+ds_get_height(vs->ds)>  1 ?
+  y * 0x7FFF / (ds_get_height(vs->ds) - 1) : 0x4000,
  dz, buttons);
  } else if (vnc_has_feature(vs, VNC_FEATURE_POINTER_TYPE_CHANGE)) {
  x -= 0x7FFF;
   

Re: [Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement

[Anthony Liguori]

On 03/08/2010 06:58 AM, Paul Bolle wrote:

Signed-off-by: Paul Bolle
   

Applied.  Thanks.

Regards,

Anthony Liguori

---
  usb-linux.c |3 ---
  1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/usb-linux.c b/usb-linux.c
index a9c15c6..23155dd 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -846,9 +846,6 @@ static int usb_linux_update_endp_table(USBHostDevice *s)
  case 0x03:
  type = USBDEVFS_URB_TYPE_INTERRUPT;
  break;
-default:
-DPRINTF("usb_host: malformed endpoint type\n");
-type = USBDEVFS_URB_TYPE_BULK;
  }
  s->endp_table[(devep&  0xf) - 1].type = type;
  s->endp_table[(devep&  0xf) - 1].halted = 0;
   

Re: [Qemu-devel] [PATCH] sdl: improve error message on fatal error

[Anthony Liguori]

On 03/08/2010 06:07 AM, Bjørn Mork wrote:

The SDL_SetVideoMode() error condition is easily triggered by a user by
simply configure a guest with a host unsupported display resolution
and attempting to enable fullscreen.  Since the error is fatal, adding
a bit of debugging help can't harm.

Sample output with this change:

  (qemu) Could not open SDL display (1280x1024x32): No video mode large enough 
for 1280x1024

The width x height might seem redundant as SDL also provides it in
SDL_GetError(), but I believe there are situations where it is
useful.  I.e. if there is some other SDL error.  Anyway, redundant
information in fatal error messages has never harmed a single gerbil.

Signed-off-by: Bjørn Mork
   

Applied.  Thanks.

Regards,

Anthony Liguori

---
  sdl.c |3 ++-
  1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/sdl.c b/sdl.c
index f26035c..34061c0 100644
--- a/sdl.c
+++ b/sdl.c
@@ -112,7 +112,8 @@ static void do_sdl_resize(int new_width, int new_height, 
int bpp)
  height = new_height;
  real_screen = SDL_SetVideoMode(width, height, bpp, flags);
  if (!real_screen) {
-fprintf(stderr, "Could not open SDL display\n");
+   fprintf(stderr, "Could not open SDL display (%dx%dx%d): %s\n", width,
+   height, bpp, SDL_GetError());
  exit(1);
  }
  }
   

Re: [Qemu-devel] [PATCH] block: add logical_block_size property

[Anthony Liguori]

On 03/04/2010 07:20 AM, Christoph Hellwig wrote:

Add a logical block size attribute as various guest side tools only
increase the filesystem sector size based on it, not the advisory
physical block size.

For scsi we already have support for a different logical block size
in place for CDROMs that we can built upon.  Only my recent block
device characteristics VPD page needs some fixups.  Note that we
leave the logial block size for CDROMs hardcoded as the 2k value
is expected for it in general.

For virtio-blk we already have a feature flag claiming to support
a variable logical block size that was added for the s390 kuli
hypervisor.  Interestingly it does not actually change the units
in which the protocol works, which is still fixed at 512 bytes,
but only communicates a different minimum I/O granularity.  So
all we need to do in virtio is to add a trap for unaligned I/O
and round down the device size to the next multiple of the logical
block size.

IDE does not support any other logical block size than 512 bytes.

Signed-off-by: Christoph Hellwig
   


Applied.  Thanks.

Regards,

Anthony Liguori


Index: qemu/block_int.h
===
--- qemu.orig/block_int.h   2010-03-03 19:16:13.408253228 +0100
+++ qemu/block_int.h2010-03-03 19:16:43.030003751 +0100
@@ -209,6 +209,7 @@ struct DriveInfo;
  typedef struct BlockConf {
  struct DriveInfo *dinfo;
  uint16_t physical_block_size;
+uint16_t logical_block_size;
  uint16_t min_io_size;
  uint32_t opt_io_size;
  } BlockConf;
@@ -226,6 +227,8 @@ static inline unsigned int get_physical_

  #define DEFINE_BLOCK_PROPERTIES(_state, _conf)  \
  DEFINE_PROP_DRIVE("drive", _state, _conf.dinfo),\
+DEFINE_PROP_UINT16("logical_block_size", _state,\
+   _conf.logical_block_size, 512),  \
  DEFINE_PROP_UINT16("physical_block_size", _state,   \
 _conf.physical_block_size, 512), \
  DEFINE_PROP_UINT16("min_io_size", _state, _conf.min_io_size, 512),  \
Index: qemu/hw/scsi-disk.c
===
--- qemu.orig/hw/scsi-disk.c2010-03-03 19:16:13.419254346 +0100
+++ qemu/hw/scsi-disk.c 2010-03-03 19:16:43.031004240 +0100
@@ -397,8 +397,10 @@ static int scsi_disk_emulate_inquiry(SCS
  }
  case 0xb0: /* block device characteristics */
  {
-unsigned int min_io_size = s->qdev.conf.min_io_size>>  9;
-unsigned int opt_io_size = s->qdev.conf.opt_io_size>>  9;
+unsigned int min_io_size =
+s->qdev.conf.min_io_size / s->qdev.blocksize;
+unsigned int opt_io_size =
+s->qdev.conf.opt_io_size / s->qdev.blocksize;

  /* required VPD size with unmap support */
  outbuf[3] = buflen = 0x3c;
@@ -1028,11 +1030,12 @@ static int scsi_disk_initfn(SCSIDevice *
  s->bs = s->qdev.conf.dinfo->bdrv;

  if (bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM) {
-s->cluster_size = 4;
+s->qdev.blocksize = 2048;
  } else {
-s->cluster_size = 1;
+s->qdev.blocksize = s->qdev.conf.logical_block_size;
  }
-s->qdev.blocksize = 512 * s->cluster_size;
+s->cluster_size = s->qdev.blocksize / 512;
+
  s->qdev.type = TYPE_DISK;
  bdrv_get_geometry(s->bs,&nb_sectors);
  nb_sectors /= s->cluster_size;
Index: qemu/hw/virtio-blk.c
===
--- qemu.orig/hw/virtio-blk.c   2010-03-03 19:16:13.426273971 +0100
+++ qemu/hw/virtio-blk.c2010-03-03 19:35:16.636028605 +0100
@@ -27,6 +27,7 @@ typedef struct VirtIOBlock
  void *rq;
  QEMUBH *bh;
  BlockConf *conf;
+unsigned short sector_mask;
  } VirtIOBlock;

  static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
@@ -250,6 +251,11 @@ static void virtio_blk_handle_flush(Virt
  static void virtio_blk_handle_write(BlockRequest *blkreq, int *num_writes,
  VirtIOBlockReq *req, BlockDriverState **old_bs)
  {
+if (req->out->sector&  req->dev->sector_mask) {
+virtio_blk_rw_complete(req, -EIO);
+return;
+}
+
  if (req->dev->bs != *old_bs || *num_writes == 32) {
  if (*old_bs != NULL) {
  do_multiwrite(*old_bs, blkreq, *num_writes);
@@ -272,6 +278,11 @@ static void virtio_blk_handle_read(VirtI
  {
  BlockDriverAIOCB *acb;

+if (req->out->sector&  req->dev->sector_mask) {
+virtio_blk_rw_complete(req, -EIO);
+return;
+}
+
  acb = bdrv_aio_readv(req->dev->bs, req->out->sector,&req->qiov,
   req->qiov.size / 512, virtio_blk_rw_complete, req);
  if (!acb) {
@@ -404,12 +415,13 @@ static void virtio_blk_update_config(Vir
  stl_raw(&blkcfg.seg_max, 128 - 2);
  stw_raw(&blkcfg.cylinders, cylinders);
  b

Re: [Qemu-devel] [PATCH QEMU] Transparent Hugepage Support #3

[Andrea Arcangeli]

On Wed, Mar 17, 2010 at 03:52:15PM +, Paul Brook wrote:
> > > > Size not multiple I think is legitimate, the below-4G chunk isn't
> > > > required to end 2M aligned, all it matters is that the above-4G then
> > > > starts aligned. In short one thing to add in the future as parameter
> > > > to qemu_ram_alloc is the physical address that the host virtual
> > > > address corresponds to.
> > >
> > > In general you don't know this at allocation time.
> > 
> > Caller knows it, it's not like the caller is outside of qemu, it's not
> > some library. We know this is enough with the caller that there is now.
> 
> No we don't.  As discussed previously, there are machines where the physical 
> location of RAM is configurable at runtime.  In fact it's common for the ram 
> to be completely absent at reset.

This is why PREFERRED_RAM_ALIGN is only defined for __x86_64__. I'm
not talking about other archs that may never support transparent
hugepages in the kernel because of other architectural constrains that
may prevent to map hugepages mixed with regular pages in the same vma.

Re: [Qemu-devel] [PATCH QEMU] Transparent Hugepage Support #3

[Paul Brook]

> > > Size not multiple I think is legitimate, the below-4G chunk isn't
> > > required to end 2M aligned, all it matters is that the above-4G then
> > > starts aligned. In short one thing to add in the future as parameter
> > > to qemu_ram_alloc is the physical address that the host virtual
> > > address corresponds to.
> >
> > In general you don't know this at allocation time.
> 
> Caller knows it, it's not like the caller is outside of qemu, it's not
> some library. We know this is enough with the caller that there is now.

No we don't.  As discussed previously, there are machines where the physical 
location of RAM is configurable at runtime.  In fact it's common for the ram 
to be completely absent at reset.

Paul

Re: [Qemu-devel] [PATCH] Use snapshots from backing disks

[Anthony Liguori]

On 03/08/2010 07:13 PM, Rob Earhart wrote:

Modify the snapshot load path to find and load snapshots contained in backing
disks, useful when the current disk is a differencing disk.

Add the source of a snapshot when listing snapshots.

This should only break backwards compatibility for scenarios depending on not
being able to load snapshots from backing disks (which doesn't seem like a
problem), and for code which parses the snapshot list output (if any).

Signed-off-by: Rob Earhart
   


Your patch is whitespace damaged.  Please use a different mailer (like 
git-send-email).


Regards,

Anthony Liguori


---
  block.c|   10 +
  block.h|2 +
  block/qcow2-snapshot.c |  105 
  qemu-img.c |   25 +++-
  savevm.c   |   92 +-
  5 files changed, 170 insertions(+), 64 deletions(-)

diff --git a/block.c b/block.c
index 31d1ba4..3588700 100644
--- a/block.c
+++ b/block.c
@@ -1481,6 +1481,16 @@ void bdrv_get_backing_filename(BlockDriverState *bs,
  }
  }

+const char *bdrv_get_filename(BlockDriverState *bs)
+{
+return bs->filename;
+}
+
+BlockDriverState* bdrv_get_backing_bdrv(BlockDriverState *bs)
+{
+return bs->backing_hd;
+}
+
  int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
const uint8_t *buf, int nb_sectors)
  {
diff --git a/block.h b/block.h
index edf5704..1d04322 100644
--- a/block.h
+++ b/block.h
@@ -179,6 +179,8 @@ int bdrv_get_info(BlockDriverState *bs,
BlockDriverInfo *bdi);
  const char *bdrv_get_encrypted_filename(BlockDriverState *bs);
  void bdrv_get_backing_filename(BlockDriverState *bs,
 char *filename, int filename_size);
+const char *bdrv_get_filename(BlockDriverState *bs);
+BlockDriverState* bdrv_get_backing_bdrv(BlockDriverState *bs);
  int bdrv_snapshot_create(BlockDriverState *bs,
   QEMUSnapshotInfo *sn_info);
  int bdrv_snapshot_goto(BlockDriverState *bs,
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 8ddaea2..ca06fcf 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -191,16 +191,23 @@ static int qcow_write_snapshots(BlockDriverState *bs)
  static void find_new_snapshot_id(BlockDriverState *bs,
   char *id_str, int id_str_size)
  {
-BDRVQcowState *s = bs->opaque;
-QCowSnapshot *sn;
-int i, id, id_max = 0;
+int id_max = 0;
+
+assert(bs);
+
+do {
+BDRVQcowState *s = bs->opaque;
+QCowSnapshot *sn;
+int i, id;
+
+for(i = 0; i<  s->nb_snapshots; i++) {
+sn = s->snapshots + i;
+id = strtoul(sn->id_str, NULL, 10);
+if (id>  id_max)
+id_max = id;
+}
+} while ((bs = bdrv_get_backing_bdrv(bs)));

-for(i = 0; i<  s->nb_snapshots; i++) {
-sn = s->snapshots + i;
-id = strtoul(sn->id_str, NULL, 10);
-if (id>  id_max)
-id_max = id;
-}
  snprintf(id_str, id_str_size, "%d", id_max + 1);
  }

@@ -316,41 +323,88 @@ int qcow2_snapshot_goto(BlockDriverState *bs,
const char *snapshot_id)
  {
  BDRVQcowState *s = bs->opaque;
  QCowSnapshot *sn;
-int i, snapshot_index, l1_size2;
+int i, snapshot_index, l1_size2, ret;

  snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_id);
-if (snapshot_index<  0)
-return -ENOENT;
+
+if (snapshot_index<  0) {
+/* if there is a backing file, use it. In this case, the
+ * current image needs to be reset to be consistent
+ */
+if (bs->backing_hd) {
+ret = bdrv_snapshot_goto(bs->backing_hd, snapshot_id);
+if (ret<  0) {
+return ret;
+}
+/* reset the sectors of the current image (Note: there can
+ * still be snapshots, so the refcounts must be kept
+ * consistent)
+ */
+if (qcow2_update_snapshot_refcount(bs, s->l1_table_offset,
+   s->l1_size, -1)<  0) {
+return -EIO;
+}
+l1_size2 = s->l1_size * sizeof(uint64_t);
+memset(s->l1_table, 0, l1_size2);
+if (bdrv_pwrite(s->hd, s->l1_table_offset,
+s->l1_table, l1_size2) != l1_size2) {
+return -EIO;
+}
+return 0;
+} else {
+return -ENOENT;
+}
+}
+
+assert(0<= snapshot_index);
+
  sn =&s->snapshots[snapshot_index];

-if (qcow2_update_snapshot_refcount(bs, s->l1_table_offset,
s->l1_size, -1)<  0)
-goto fail;
+/* if the file is read-only, all can be done by updating the
+ * in-memory L1 table. Otherwise, we'll need to do some writes.
+ */

-if (qcow2_grow_l1_table(bs, sn->l1_size)<  0)
-goto fail;
+if (!

Re: [Qemu-devel] [PATCH QEMU] Transparent Hugepage Support #3

[Andrea Arcangeli]

On Wed, Mar 17, 2010 at 03:21:26PM +, Paul Brook wrote:
> > On Wed, Mar 17, 2010 at 03:05:57PM +, Paul Brook wrote:
> > > > +   if (size >= PREFERRED_RAM_ALIGN)
> > > > +   new_block->host = qemu_memalign(PREFERRED_RAM_ALIGN,
> > > > size);
> > >
> > > Is this deliberately bigger-than rather than multiple-of?
> > > Having the size not be a multiple of alignment seems somewhat strange,
> > > it's always going to be wrong at one end...
> > 
> > Size not multiple I think is legitimate, the below-4G chunk isn't
> > required to end 2M aligned, all it matters is that the above-4G then
> > starts aligned. In short one thing to add in the future as parameter
> > to qemu_ram_alloc is the physical address that the host virtual
> > address corresponds to.
> 
> In general you don't know this at allocation time.

Caller knows it, it's not like the caller is outside of qemu, it's not
some library. We know this is enough with the caller that there is now.

Again there is absolutely no relation between the "size" and
this. Size can be anything and it's absolutely irrelevant.

All it matter is the _start_. Both the guest physical address _start_
and the host virtual address _start_. And they don't have to be
aligned to 2M, simply their alignment or misalignment have to match
and this is the simplest way to have them match.

> > The guest physical address that the host
> > retval corresponds to, has to be aligned with PREFERRED_RAM_ALIGN for
> > NPT/EPT to work. I don't think it's a big concern right now.
>  
> If you allocating chinks that are multiples of the relevant page size, then I 
> don't think you can expect anything particularly sensible to happen.

If you want me to do a bigger more complex patch that passes down to
qemu_ram_alloc the actual guest physical address that the virtual
address returned by qram_mem_alloc will correspond to, I will do
it. That likely would be something like qemu_ram_alloc_align.

And if somebody volunteers to avoid me to do it, you're welcome.

I don't care how this happens but it must happen.

Re: [Qemu-devel] [PATCH] better describe -net options

[Anthony Liguori]

On 03/05/2010 03:07 PM, Brian Thomason wrote:

The manpage reflects that multiple -net user calls may be made,
but if this is done, it can cause the program to hang. Instead,
multiple -net calls can be combined into one, and this patch adds
that information to qemu-options.hx. Also, -net user may only
be used in conjunction with -net nic.  This is already implcitly
stated, but this patch makes that statement more explicit.


Actually, the problem is that the user created a loop.  It's actually 
valid to have something like:


-net user -net dump,file=foo.pcap -net nic

But having -net user -net user creates a loop.

Regards,

Anthony Liguori



https://bugs.launchpad.net/ubuntu/+source/qemu-kvm/+bug/474969
https://bugs.launchpad.net/ubuntu/+source/qemu-kvm/+bug/453617

Signed-off-by: Brian Thomason >

---
 qemu-options.hx |   15 +--
 1 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index fd50add..724f434 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -952,7 +952,8 @@ for a list of available devices for your target.

 @item -net user[,@var{option}][,@var{option}][,...]
 Use the user mode network stack which requires no administrator
-privilege to run. Valid options are:
+privilege to run. Please note that this can be used only in 
conjunction with

+...@option{-net nic}. Valid options are:

 @table @option
 @item vl...@var{n}
@@ -1028,7 +1029,8 @@ the guest IP address @var{guestaddr} on guest 
port @var{guestport}. If
 @var{guestaddr} is not specified, its value is x.x.x.15 (default 
first address
 given by the built-in DHCP server). By specifying @var{hostaddr}, the 
rule can
 be bound to a specific host interface. If no connection type is set, 
TCP is

-used. This option can be given multiple times.
+used. This option can not be given multiple times, but multiple rules may
+be combined.

 For example, to redirect host X11 connection from screen 1 to guest
 screen 0, use the following:
@@ -1052,6 +1054,15 @@ telnet localhost 
 Then when you use on the host @code{telnet localhost }, you
 connect to the guest telnet server.

+To combine two or more hostfwd rules, simply use a comma as a 
delimiter. For
+example, to combine the two rules mentioned in the examples above, 
use the

+following:
+
+...@example
+#on the host
+qemu -net user,hostfwd=tcp:127.0.0.1:6001-:6000,hostfwd=tcp:-::23 
[...]

+...@end example
+
 @item guestfwd=[tcp]:@var{server}:@var{por...@var{dev}
 Forward guest TCP connections to the IP address @var{server} on port 
@var{port}
 to the character device @var{dev}. This option can be given multiple 
times.

--
1.6.3.3

Re: [Qemu-devel] [PULL v2] Convert device_add to QObject / QError

[Anthony Liguori]

On 03/16/2010 01:31 PM, Markus Armbruster wrote:

Anthony ran into conflicts and asked me to rebase and send out a pull
request.

Complete list of conflicts:

* qdev: Improve diagnostics for bad property values
   commit 6bf38816df80a3b50529119c5458b151b3e2c728

   Adds two new errors to qdev_prop_parse(), which need conversion to
   QError.  Resolution straighforward, just needs new
   QERR_PROPERTY_VALUE_IN_USE, QERR_PROPERTY_VALUE_NOT_FOUND.

* scsi: Make device scsi-disk reject /dev/sg*
   commit 32bb404a6a4d726dfd691f75704f08257ce65ffe

   Adds a qemu_error() use, which needs to be changed to error_report().

* slirp: check system() success
   commit 24ac07dec7f23c58dc48aa7754f872781b386d46

   Context changed.  Resolution trivial.


The following changes since commit 0aef4261ac0ec9089ade0e3a92f986cb4ba7317e:
   Aurelien Jarno (1):
 target-ppc: fix evsrwu and evsrws (second try)

are available in the git repository at:

   git://repo.or.cz/qemu/armbru.git qerror
   


Applied all.  Thanks.

Regards,

Anthony Liguori


Markus Armbruster (52):
   usb: Remove disabled monitor_printf() in usb_read_file()
   savevm: Fix -loadvm to report errors to stderr, not the monitor
   pc: Fix error reporting for -boot once
   pc: Factor common code out of pc_boot_set() and cmos_init()
   tools: Remove unused cur_mon from qemu-tool.c
   monitor: Separate "default monitor" and "current monitor" cleanly
   block: Simplify usb_msd_initfn() test for "can read bdrv key"
   monitor: Factor monitor_set_error() out of qemu_error_internal()
   error: Move qemu_error()&  friends from monitor.c to own file
   error: Simplify error sink setup
   error: Move qemu_error&  friends into their own header
   error: New error_printf() and error_vprintf()
   error: Don't abuse qemu_error() for non-error in qdev_device_help()
   error: Don't abuse qemu_error() for non-error in qbus_find()
   error: Don't abuse qemu_error() for non-error in scsi_hot_add()
   error: Replace qemu_error() by error_report()
   error: Rename qemu_error_new() to qerror_report()
   error: Infrastructure to track locations for error reporting
   error: Include the program name in error messages to stderr
   error: Track locations in configuration files
   QemuOpts: Fix qemu_config_parse() to catch file read errors
   error: Track locations on command line
   qdev: Fix -device and device_add to handle unsuitable bus gracefully
   qdev: Factor qdev_create_from_info() out of qdev_create()
   qdev: Hide "no_user" devices from users
   qdev: Hide "ptr" properties from users
   monitor: New monitor_cur_is_qmp()
   error: Let converted handlers print in human monitor
   error: Polish human-readable error descriptions
   error: New QERR_PROPERTY_NOT_FOUND
   error: New QERR_PROPERTY_VALUE_BAD
   error: New QERR_PROPERTY_VALUE_IN_USE
   error: New QERR_PROPERTY_VALUE_NOT_FOUND
   qdev: convert setting device properties to QError
   qdev: Relax parsing of bus option
   error: New QERR_BUS_NOT_FOUND
   error: New QERR_DEVICE_MULTIPLE_BUSSES
   error: New QERR_DEVICE_NO_BUS
   qdev: Convert qbus_find() to QError
   error: New error_printf_unless_qmp()
   error: New QERR_BAD_BUS_FOR_DEVICE
   error: New QERR_BUS_NO_HOTPLUG
   error: New QERR_DEVICE_INIT_FAILED
   error: New QERR_NO_BUS_FOR_DEVICE
   Revert "qdev: Use QError for 'device not found' error"
   error: Convert do_device_add() to QError
   qemu-option: Functions to convert to/from QDict
   qemu-option: Move the implied first name into QemuOptsList
   qemu-option: Rename find_list() to qemu_find_opts()&  external linkage
   monitor: New argument type 'O'
   monitor: Use argument type 'O' for device_add
   monitor: convert do_device_add() to QObject

  Makefile.target|1 +
  audio/audio.c  |4 +-
  hw/pc.c|   35 ++
  hw/pci-hotplug.c   |   14 +-
  hw/pci.c   |   14 +-
  hw/qdev-properties.c   |   36 ++---
  hw/qdev.c  |  236 --
  hw/qdev.h  |2 +-
  hw/scsi-bus.c  |4 +-
  hw/scsi-disk.c |7 +-
  hw/scsi-generic.c  |9 +-
  hw/usb-bus.c   |4 +-
  hw/usb-msd.c   |4 +-
  hw/usb-net.c   |2 +-
  hw/usb-serial.c|9 +-
  hw/virtio-net.c|5 +-
  hw/virtio-pci.c|4 +-
  hw/virtio-serial-bus.c |2 +-
  monitor.c  |  337 +---
  monitor.h  |7 +
  net.c  |   32 +++---
  net/dump.c |5 +-
  net/slirp.c|   28 ++--
  net/socket.c   |   12 +-
  net/tap-bsd.c  |7 +-
  net/tap-linux.c|9 +-
  net/tap-solaris.c  |4 +-
  net/tap-win32.c|2 +-
  net/t

Re: [Qemu-devel] [PULL] e100/pci fixes

[Anthony Liguori]

On 03/16/2010 09:56 AM, Michael S. Tsirkin wrote:

The following changes since commit cb66ffcf9e298dc1bfc11682172ff9472bcd4495:
   Kevin Wolf (1):
 qemu-img rebase: Document -f option

are available in the git repository at:

   git://git.kernel.org/pub/scm/linux/kernel/git/mst/qemu.git pci
   


Pulled, thanks.

Regards,

Anthony Liguori


Michael S. Tsirkin (2):
   eepro100: address pci todo's, use pci_set_xx
   pcnet: make subsystem vendor id match hardware

  hw/eepro100.c |   94 +++-
  hw/pcnet.c|3 ++
  2 files changed, 35 insertions(+), 62 deletions(-)



   

Re: [Qemu-devel] [PATCH QEMU] Transparent Hugepage Support #3

[Paul Brook]

> On Wed, Mar 17, 2010 at 03:05:57PM +, Paul Brook wrote:
> > > +   if (size >= PREFERRED_RAM_ALIGN)
> > > +   new_block->host = qemu_memalign(PREFERRED_RAM_ALIGN,
> > > size);
> >
> > Is this deliberately bigger-than rather than multiple-of?
> > Having the size not be a multiple of alignment seems somewhat strange,
> > it's always going to be wrong at one end...
> 
> Size not multiple I think is legitimate, the below-4G chunk isn't
> required to end 2M aligned, all it matters is that the above-4G then
> starts aligned. In short one thing to add in the future as parameter
> to qemu_ram_alloc is the physical address that the host virtual
> address corresponds to.

In general you don't know this at allocation time.

> The guest physical address that the host
> retval corresponds to, has to be aligned with PREFERRED_RAM_ALIGN for
> NPT/EPT to work. I don't think it's a big concern right now.
 
If you allocating chinks that are multiples of the relevant page size, then I 
don't think you can expect anything particularly sensible to happen.

Paul

Re: [Qemu-devel] [PATCH QEMU] Transparent Hugepage Support #3

[Andrea Arcangeli]

On Wed, Mar 17, 2010 at 03:05:57PM +, Paul Brook wrote:
> > +   if (size >= PREFERRED_RAM_ALIGN)
> > +   new_block->host = qemu_memalign(PREFERRED_RAM_ALIGN, size);
> > 
> 
> Is this deliberately bigger-than rather than multiple-of?
> Having the size not be a multiple of alignment seems somewhat strange, it's 
> always going to be wrong at one end...

Size not multiple I think is legitimate, the below-4G chunk isn't
required to end 2M aligned, all it matters is that the above-4G then
starts aligned. In short one thing to add in the future as parameter
to qemu_ram_alloc is the physical address that the host virtual
address corresponds to. The guest physical address that the host
retval corresponds to, has to be aligned with PREFERRED_RAM_ALIGN for
NPT/EPT to work. I don't think it's a big concern right now.

Re: [Qemu-devel] [PATCH QEMU] Transparent Hugepage Support #3

[Paul Brook]

> +   if (size >= PREFERRED_RAM_ALIGN)
> +   new_block->host = qemu_memalign(PREFERRED_RAM_ALIGN, size);
> 

Is this deliberately bigger-than rather than multiple-of?
Having the size not be a multiple of alignment seems somewhat strange, it's 
always going to be wrong at one end...

Paul

[Qemu-devel] [PATCH QEMU] Transparent Hugepage Support #3

[Andrea Arcangeli]

From: Andrea Arcangeli 

This will allow proper alignment so NPT/EPT can take advantage of
linux host backing the guest memory with hugepages. It also ensures
that when KVM isn't used the first 2M of guest physical memory are
backed by a large TLB. To complete it, it will also notify the kernel
that this memory is important to be backed by hugepages with madvise
(needed for both KVM and QEMU) so that hugepages can also be used in
embedded systems without any memory waste and in the future it will
allow khugepaged to prioritize the collapse of hugepages into the
madvise regions.

Ideally the max hugepage size provided by the transparent hugepage
support in the kernel should be exported by some sysfs file, but
there is no reason to expect x86_64 host to have hugepages larger than
2M or to expect those to be supported by the kernel transparent
hugepage support in the short and medium term, so we can defer the
invention of a fixed kernel API until this happens, by that time we'll
surely have a better clue of what's the best way to provide that
information to userland and it'll be a few liner change to adapt qemu
to use it so there's no hurry to do it right now. Plus the below will
keep to remain optimal and there is no risk of memory waste as virtual
memory is practically zero cost on 64bit archs.

NOTE: if the callers of qemu_ram_alloc changes significantly we may
later be required to pass a second parameter to qemu_ram_alloc that
will tell it what is the first guest physical address that corresponds
to the "sized" memory block being allocated. I'd defer this change for
later too as it may never be needed.

I verified this is more than enough to get the max benefit from the
kernel side feature.

cat /sys/kernel/debug/kvm/largepages 
301

Signed-off-by: Andrea Arcangeli 
---
diff --git a/exec.c b/exec.c
index 14767b7..ab33f6b 100644
--- a/exec.c
+++ b/exec.c
@@ -2745,6 +2745,18 @@ static void *file_ram_alloc(ram_addr_t memory, const 
char *path)
 }
 #endif
 
+#if defined(__linux__) && defined(__x86_64__)
+/*
+ * Align on the max transparent hugepage size so that
+ * "(gfn ^ pfn) & (HPAGE_SIZE-1) == 0" to allow KVM to
+ * take advantage of hugepages with NPT/EPT or to
+ * ensure the first 2M of the guest physical ram will
+ * be mapped by the same hugetlb for QEMU (it is worth
+ * it even without NPT/EPT).
+ */
+#define PREFERRED_RAM_ALIGN (2*1024*1024)
+#endif
+
 ram_addr_t qemu_ram_alloc(ram_addr_t size)
 {
 RAMBlock *new_block;
@@ -2768,11 +2780,19 @@ ram_addr_t qemu_ram_alloc(ram_addr_t size)
 PROT_EXEC|PROT_READ|PROT_WRITE,
 MAP_SHARED | MAP_ANONYMOUS, -1, 0);
 #else
-new_block->host = qemu_vmalloc(size);
+#ifdef PREFERRED_RAM_ALIGN
+   if (size >= PREFERRED_RAM_ALIGN)
+   new_block->host = qemu_memalign(PREFERRED_RAM_ALIGN, size);
+   else
+#endif 
+   new_block->host = qemu_vmalloc(size);
 #endif
 #ifdef MADV_MERGEABLE
 madvise(new_block->host, size, MADV_MERGEABLE);
 #endif
+#ifdef MADV_HUGEPAGE
+madvise(new_block->host, size, MADV_HUGEPAGE);
+#endif
 }
 new_block->offset = last_ram_offset;
 new_block->length = size;

Re: [Qemu-devel] [PATCH 3/4] tcg-hppa: Finish the port.

[Richard Henderson]

On 03/16/2010 06:58 PM, Stuart Brady wrote:
> The tcg_reg_free() calls worry me slightly, but I assume they're safe...

Yeah, that one's rather gross.

Since Aurelien's generic div/rem patch I could simply remove all
that millicode stuff, including that tcg_reg_free, and let the
generic code handle this instead.  Which would get to the same
millicode routine via one or two extra levels of indirection.

Thoughts?


r~

Re: [Qemu-devel] [PATCH 4/4] tcg-hppa: Compute is_write in cpu_signal_handler.

[Richard Henderson]

On 03/16/2010 07:23 PM, Stuart Brady wrote:
> On Wed, Mar 17, 2010 at 02:10:43AM +, Stuart Brady wrote:
>> Argh.  It just seems mind bogglingly silly that is_write doesn't seem to
>> be included in the siginfo on archs where doing so would make sense...
> 
>> It's at least something I'd have hoped libc could take care of in the case
>> where the hardware doesn't indicate whether the fault was due to a read
>> or a write...
> 
> Oh, actually, I guess you'd just test the equivalent of
> '(info->si_isr >> 33) & 1' or 'DSISR_sig(uc) & 0x0080)' on such
> an arch, but it still seems a shame that this isn't done for us on
> archs without that ability...

Unfortunately there's no reserved space in the hppa sigcontext, which
is what would need to be expanded in order to store the additional info
from the kernel.  So in addition to a kernel patch, userland would need
to change its abi.  All kinda nasty, that.


r~

Re: [Qemu-devel] [PATCH 0/6] Multi-level page tables and userland mapping fixes, v3

[Richard Henderson]

On 03/17/2010 04:48 AM, Riku Voipio wrote:
>>> page_check_range:
>>>
>>> -if (start + len < start)
>>> -/* we've wrapped around */
>>> ...
>>> +if (start + len - 1 < start) {
>>> +/* We've wrapped around.  */
>>>
>>> This now blows up with len = 0;
> 
>> Confirmed. A quick test with  if (len > 0) around and ldconfig.real runs.
> 
> Richard, would you be ok with that change? Or should we rather have explicit 
> 
> if (len == 0) return 0;

My personal preference is 

  if (len != 0 && start + len - 1 < start)

but don't let that stop anyone checking in any correct variant.


r~

[Qemu-devel] [PATCH 001/399] target-arm: Fix handling of AL condition in IT instruction

[Johan Bengtsson]

Do not try to insert a conditional jump over next instruction when the
condition code is AL as this will trigger an internal error.

Signed-off-by: Johan Bengtsson 
---
 target-arm/translate.c |8 +---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/target-arm/translate.c b/target-arm/translate.c
index 786c329..554583d 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -8328,9 +8328,11 @@ static void disas_thumb_insn(CPUState *env, DisasContext 
*s)
 
 if (s->condexec_mask) {
 cond = s->condexec_cond;
-s->condlabel = gen_new_label();
-gen_test_cc(cond ^ 1, s->condlabel);
-s->condjmp = 1;
+if (cond != 0x0e) { /* Skip conditional when condition is AL. */
+  s->condlabel = gen_new_label();
+  gen_test_cc(cond ^ 1, s->condlabel);
+  s->condjmp = 1;
+}
 }
 
 insn = lduw_code(s->pc);
-- 
1.6.3.3

Re: [Qemu-devel] [PATCH] pcnet: make subsystem vendor id match hardware

[Michael S. Tsirkin]

On Tue, Mar 16, 2010 at 10:29:43PM +0100, Stefan Weil wrote:
> Michael S. Tsirkin schrieb:
> > Real pcnet device (AT2450) apparently has subsystem
> > device and vendor id set to 0, this is out of spec
> > (which requires that vendor id is obtained from PCI SIG)
> > but windows xp driver seems to need this in order
> > to associate.
> >
> > qemu sets pci subsystem id to qumranet/qemu
> > since d350d97d196a632b6c7493acf07a061017fc6f7d,
> > debian does not yet have this patch.
> >
> > https://bugzilla.redhat.com/show_bug.cgi?id=521247
> >
> > Signed-off-by: Michael S. Tsirkin 
> > Cc: Gerd Hoffmann 
> > Cc: Anthony Liguori 
> > ---
> >  hw/pcnet.c |3 +++
> >  1 files changed, 3 insertions(+), 0 deletions(-)
> >
> > diff --git a/hw/pcnet.c b/hw/pcnet.c
> > index 44b5b31..12260be 100644
> > --- a/hw/pcnet.c
> > +++ b/hw/pcnet.c
> > @@ -1997,6 +1997,9 @@ static int pci_pcnet_init(PCIDevice *pci_dev)
> >  pci_set_long(pci_conf + PCI_BASE_ADDRESS_0 + 4,
> >   PCI_BASE_ADDRESS_SPACE_MEMORY);
> >  
> > +pci_set_word(pci_conf + PCI_SUBSYSTEM_VENDOR_ID, 0x0);
> > +pci_set_word(pci_conf + PCI_SUBSYSTEM_ID, 0x0);
> > +
> >  /* TODO: value must be 0 at RST# */
> >  pci_conf[PCI_INTERRUPT_PIN] = 1; // interrupt pin 0
> >  pci_conf[PCI_MIN_GNT] = 0x06;
> >   
> 
> Don't you think that a comment in the code
> (with the same explanation which you wrote above)
> would be helpful in this special case?
> Of course one can always call git blame, but
> a comment is easier to read.
> 
> Regards
> Stefan

Yea, ok. Send a patch.

One thing that I'm still converned about is what pci.c should do with
subsystem id, since setting it to redhat/qumranet by default is not
really the best thing to do: it clearly is unlikely to match any real
hardware, and the heuristic it uses to detect when to set subsystem id
is also kind of off: the spec ties it with class not with header type.

Maybe I'll just add a TODO for now :)

-- 
MST

[Qemu-devel] Re: virtio-serial NULL deference

[Amit Shah]

On (Tue) Mar 09 2010 [20:59:58], Amit Shah wrote:
> On (Tue) Mar 09 2010 [14:15:45], Juan Quintela wrote:
> > 
> > Hi Amit
> 
> Hey Juan,
> 
> > Checking migration, I just found this problem:
> > 
> > I don't know what to put there.  a return -EINVAL or continue?
> > Looking more at the code, I am not sure what checks:
> > 
> > a- that bus->max_nr_ports is the same in both sides (or at least bigger
> >on migration destination)
> 
> Yes, we should check for this.

I've added this check in my local tree.

> > b- We sent the value of config.nr_ports, but ... we assign it back on
> >destination, instead of checking that they are the same.
> 
> This is done to accomodate for hot-plug/unplug. nr_ports will go up /
> down after those operations. (Current implementation only increases this
> value, on hotplug operations. On hot-unplug, this value is not
> decremented.)

For now I've added a check that tests nr_ports == s->config.nr_ports. If
that's not true, then return with -EINVAL.

These two were small changes, no problem.

> > c- port->id is taken from nr_ports again, and nothing checks that ports
> >appear in the same order in source and destination.
> 
> Actually, this has me thinking about how would this work:
> - start vm with 3 ports
> - hotplug 2 more ports
> - migrate
> 
> Would the destination have 5 ports, or would it have 3? I thought qdev
> would take care of this scenario (hotplug). I don't think I've tested
> this, so I'll do this soon, but in case anyone knows the answer, please
> let me know.

I spoke with Dan and he confirmed libvirt starts qemu instances on the
destination computer with the appropriate devices, taking into
consideration the hotplug/unplug status.

However, the only problem here is virtio-serial ports are allocated an
'id', ie., a port number, and this is auto-assigned when a new port is
found by adding 1 to the previous id.

To make this whole thing independent of the order in which ports are
added / removed, we'll have to accept the port id on the command line.

This also means that we'll have to let the kernel know of the id because
the control messages that the kernel and qemu exchange contain the port
id.

So basically some design changes will have to be incorporated both, in
the kernel as well as in qemu to accomodate this. If this sounds right,
I'll get to this right away. If there's an easier solution, please let
me know.

Amit

Re: [Qemu-devel] [PATCH 0/6] Multi-level page tables and userland mapping fixes, v3

[Riku Voipio]

On Mon, Mar 15, 2010 at 04:08:46PM +0100, Jan-Simon Möller wrote:
> Am Montag, 15. März 2010 15:48:03 schrieb Riku Voipio:
> > On Mon, Mar 15, 2010 at 01:46:10PM +0100, Jan-Simon Möller wrote:
> > > r...@frodo:/# qemu-arm -strace /sbin/ldconfig.real
> > > 16359 uname(0x403fef78) = 0
> > > 16359 brk(NULL) = 0x000a9000
> > > 16359 brk(0x000a9d08) = 0x000a9d08
> > > 16359 open("/dev/urandom",O_RDONLY) = 3
> > > 16359 read(3,0x403ff27d,3) = 3
> > > 16359 close(3) = 0
> > > [...]
> > > 16359 stat64("/usr/lib/libgettextlib.so",0x403fdf28) = 0
> > > 16359 stat64("/usr/lib/libgettextpo.so.0",0x403fdec0) = 0
> > > 16359 stat64("/usr/lib/libgettextpo.so.0.4.0",0x403fdf28) = 0
> > > 16359 stat64("/usr/lib/libpython2.6.so.1.0",0x403fdec0) = 0
> > > 16359 stat64("/usr/lib/libpython2.6.so.1.0",0x403fdf28) = 0
> > > 16359 open("/etc/ld.so.cache~",O_WRONLY|O_CREAT|O_NOFOLLOW|O_TRUNC,0600)
> > > = 3 16359 write(3,0xb03d0,1288) = 1288
> > > 16359 write(3,0x403ff0a0,0) = -1 errno=14 (Bad address)
 
> > A zero sized write. According to manpage ok.
 
> > In qemu we do a lock_user to to get the string to write. Richards change
> >  changes the access checks the get called by lock_user:

> > page_check_range:
> > 
> > -if (start + len < start)
> > -/* we've wrapped around */
> > ...
> > +if (start + len - 1 < start) {
> > +/* We've wrapped around.  */
> > 
> > This now blows up with len = 0;

> Confirmed. A quick test with  if (len > 0) around and ldconfig.real runs.

Richard, would you be ok with that change? Or should we rather have explicit 

if (len == 0) return 0;

?

[Qemu-devel] [PATCHv6 06/11] virtio: move typedef to qemu-common

[Michael S. Tsirkin]

make it possible to use type without header include,
simplifying header dependencies.

Signed-off-by: Michael S. Tsirkin 
---
 hw/virtio.h   |1 -
 qemu-common.h |1 +
 2 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/virtio.h b/hw/virtio.h
index d3eb714..f885f1b 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -69,7 +69,6 @@ static inline target_phys_addr_t 
vring_align(target_phys_addr_t addr,
 }
 
 typedef struct VirtQueue VirtQueue;
-typedef struct VirtIODevice VirtIODevice;
 
 #define VIRTQUEUE_MAX_SIZE 1024
 
diff --git a/qemu-common.h b/qemu-common.h
index f12a8f5..90ca3b8 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -228,6 +228,7 @@ typedef struct I2SCodec I2SCodec;
 typedef struct DeviceState DeviceState;
 typedef struct SSIBus SSIBus;
 typedef struct EventNotifier EventNotifier;
+typedef struct VirtIODevice VirtIODevice;
 
 typedef uint64_t pcibus_t;
 
-- 
1.7.0.18.g0d53a5

[Qemu-devel] [PATCHv6 08/11] vhost: vhost net support

[Michael S. Tsirkin]

This adds vhost net device support in qemu. Will be tied to tap device
and virtio by following patches.  Raw backend is currently missing,
will be worked on/submitted separately.

Signed-off-by: Michael S. Tsirkin 
---
 Makefile.target |2 +
 configure   |   37 +++
 hw/vhost.c  |  711 +++
 hw/vhost.h  |   48 
 hw/vhost_net.c  |  195 +++
 hw/vhost_net.h  |   19 ++
 6 files changed, 1012 insertions(+), 0 deletions(-)
 create mode 100644 hw/vhost.c
 create mode 100644 hw/vhost.h
 create mode 100644 hw/vhost_net.c
 create mode 100644 hw/vhost_net.h

diff --git a/Makefile.target b/Makefile.target
index 004a703..ea5207c 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -176,6 +176,8 @@ obj-y = vl.o async.o monitor.o pci.o pci_host.o pcie_host.o 
machine.o gdbstub.o
 # need to fix this properly
 obj-y += virtio-blk.o virtio-balloon.o virtio-net.o virtio-pci.o 
virtio-serial-bus.o
 obj-y += event_notifier.o
+obj-y += vhost_net.o
+obj-$(CONFIG_VHOST_NET) += vhost.o
 obj-y += rwhandler.o
 obj-$(CONFIG_KVM) += kvm.o kvm-all.o
 obj-$(CONFIG_ISA_MMIO) += isa_mmio.o
diff --git a/configure b/configure
index d728799..93015e3 100755
--- a/configure
+++ b/configure
@@ -263,6 +263,7 @@ vnc_tls=""
 vnc_sasl=""
 xen=""
 linux_aio=""
+vhost_net=""
 
 gprof="no"
 debug_tcg="no"
@@ -651,6 +652,10 @@ for opt do
   ;;
   --enable-docs) docs="yes"
   ;;
+  --disable-vhost-net) vhost_net="no"
+  ;;
+  --enable-vhost-net) vhost_net="yes"
+  ;;
   *) echo "ERROR: unknown option $opt"; show_help="yes"
   ;;
   esac
@@ -806,6 +811,8 @@ echo "  --disable-blobs  disable installing 
provided firmware blobs"
 echo "  --kerneldir=PATH look for kernel includes in PATH"
 echo "  --enable-docsenable documentation build"
 echo "  --disable-docs   disable documentation build"
+echo "  --disable-vhost-net  disable vhost-net acceleration support"
+echo "  --enable-vhost-net   enable vhost-net acceleration support"
 echo ""
 echo "NOTE: The object files are built at the place where configure is 
launched"
 exit 1
@@ -1498,6 +1505,32 @@ EOF
 fi
 
 ##
+# test for vhost net
+
+if test "$vhost_net" != "no"; then
+if test "$kvm" != "no"; then
+cat > $TMPC <
+int main(void) { return 0; }
+EOF
+if compile_prog "$kvm_cflags" "" ; then
+vhost_net=yes
+else
+if "$vhost_net" == "yes" ; then
+feature_not_found "vhost-net"
+fi
+vhost_net=no
+fi
+else
+if "$vhost_net" == "yes" ; then
+echo -e "NOTE: vhost-net feature requires KVM (--enable-kvm)."
+feature_not_found "vhost-net"
+fi
+vhost_net=no
+fi
+fi
+
+##
 # pthread probe
 PTHREADLIBS_LIST="-lpthread -lpthreadGC2"
 
@@ -1968,6 +2001,7 @@ echo "fdt support   $fdt"
 echo "preadv support$preadv"
 echo "fdatasync $fdatasync"
 echo "uuid support  $uuid"
+echo "vhost-net support $vhost_net"
 
 if test $sdl_too_old = "yes"; then
 echo "-> Your SDL version is too old - please upgrade to have SDL support"
@@ -2492,6 +2526,9 @@ case "$target_arch2" in
   if test "$kvm_para" = "yes"; then
 echo "CONFIG_KVM_PARA=y" >> $config_target_mak
   fi
+  if test $vhost_net = "yes" ; then
+echo "CONFIG_VHOST_NET=y" >> $config_target_mak
+  fi
 fi
 esac
 if test "$target_bigendian" = "yes" ; then
diff --git a/hw/vhost.c b/hw/vhost.c
new file mode 100644
index 000..1e5bc69
--- /dev/null
+++ b/hw/vhost.c
@@ -0,0 +1,711 @@
+/*
+ * vhost support
+ *
+ * Copyright Red Hat, Inc. 2010
+ *
+ * Authors:
+ *  Michael S. Tsirkin 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ */
+
+#include 
+#include 
+#include 
+#include "vhost.h"
+#include "hw/hw.h"
+/* For range_get_last */
+#include "pci.h"
+
+static void vhost_dev_sync_region(struct vhost_dev *dev,
+  uint64_t mfirst, uint64_t mlast,
+  uint64_t rfirst, uint64_t rlast)
+{
+uint64_t start = MAX(mfirst, rfirst);
+uint64_t end = MIN(mlast, rlast);
+vhost_log_chunk_t *from = dev->log + start / VHOST_LOG_CHUNK;
+vhost_log_chunk_t *to = dev->log + end / VHOST_LOG_CHUNK + 1;
+uint64_t addr = (start / VHOST_LOG_CHUNK) * VHOST_LOG_CHUNK;
+
+assert(end / VHOST_LOG_CHUNK < dev->log_size);
+assert(start / VHOST_LOG_CHUNK < dev->log_size);
+if (end < start) {
+return;
+}
+for (;from < to; ++from) {
+vhost_log_chunk_t log;
+int bit;
+/* We first check with non-atomic: much cheaper,
+ * and we expect non-dirty to be the common case. */
+if (!*from) {
+continue;
+}
+/* 

[Qemu-devel] [PATCHv6 07/11] virtio-pci: fill in notifier support

[Michael S. Tsirkin]

Support host/guest notifiers in virtio-pci.
The last one only with kvm, that's okay
because vhost relies on kvm anyway.

Note on kvm usage: kvm ioeventfd API
is implemented on non-kvm systems as well,
this is the reason we don't need if (kvm_enabled())
around it.

Signed-off-by: Michael S. Tsirkin 
---
 hw/virtio-pci.c |   63 +++
 1 files changed, 63 insertions(+), 0 deletions(-)

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index ee67a8a..25abc5f 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -24,6 +24,7 @@
 #include "net.h"
 #include "block_int.h"
 #include "loader.h"
+#include "kvm.h"
 
 /* from Linux's linux/virtio_pci.h */
 
@@ -392,6 +393,66 @@ static unsigned virtio_pci_get_features(void *opaque)
 return proxy->host_features;
 }
 
+static void virtio_pci_guest_notifier_read(void *opaque)
+{
+VirtQueue *vq = opaque;
+EventNotifier *n = virtio_queue_get_guest_notifier(vq);
+if (event_notifier_test_and_clear(n)) {
+virtio_irq(vq);
+}
+}
+
+static int virtio_pci_set_guest_notifier(void *opaque, int n, bool assign)
+{
+VirtIOPCIProxy *proxy = opaque;
+VirtQueue *vq = virtio_get_queue(proxy->vdev, n);
+EventNotifier *notifier = virtio_queue_get_guest_notifier(vq);
+
+if (assign) {
+int r = event_notifier_init(notifier, 0);
+if (r < 0) {
+return r;
+}
+qemu_set_fd_handler(event_notifier_get_fd(notifier),
+virtio_pci_guest_notifier_read, NULL, vq);
+} else {
+qemu_set_fd_handler(event_notifier_get_fd(notifier),
+NULL, NULL, NULL);
+event_notifier_cleanup(notifier);
+}
+
+return 0;
+}
+
+static int virtio_pci_set_host_notifier(void *opaque, int n, bool assign)
+{
+VirtIOPCIProxy *proxy = opaque;
+VirtQueue *vq = virtio_get_queue(proxy->vdev, n);
+EventNotifier *notifier = virtio_queue_get_host_notifier(vq);
+int r;
+if (assign) {
+r = event_notifier_init(notifier, 1);
+if (r < 0) {
+return r;
+}
+r = kvm_set_ioeventfd_pio_word(event_notifier_get_fd(notifier),
+   proxy->addr + VIRTIO_PCI_QUEUE_NOTIFY,
+   n, assign);
+if (r < 0) {
+event_notifier_cleanup(notifier);
+}
+} else {
+r = kvm_set_ioeventfd_pio_word(event_notifier_get_fd(notifier),
+   proxy->addr + VIRTIO_PCI_QUEUE_NOTIFY,
+   n, assign);
+if (r < 0) {
+return r;
+}
+event_notifier_cleanup(notifier);
+}
+return r;
+}
+
 static const VirtIOBindings virtio_pci_bindings = {
 .notify = virtio_pci_notify,
 .save_config = virtio_pci_save_config,
@@ -399,6 +460,8 @@ static const VirtIOBindings virtio_pci_bindings = {
 .save_queue = virtio_pci_save_queue,
 .load_queue = virtio_pci_load_queue,
 .get_features = virtio_pci_get_features,
+.set_host_notifier = virtio_pci_set_host_notifier,
+.set_guest_notifier = virtio_pci_set_guest_notifier,
 };
 
 static void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev,
-- 
1.7.0.18.g0d53a5

[Qemu-devel] [PATCHv6 05/11] virtio: add set_status callback

[Michael S. Tsirkin]

vhost net backend needs to be notified when
frontend status changes. Add a callback,
similar to set_features.

Signed-off-by: Michael S. Tsirkin 
---
 hw/s390-virtio-bus.c |2 +-
 hw/syborg_virtio.c   |2 +-
 hw/virtio-pci.c  |5 +++--
 hw/virtio.h  |9 +
 4 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c
index 9fc01e9..3efbaab 100644
--- a/hw/s390-virtio-bus.c
+++ b/hw/s390-virtio-bus.c
@@ -242,7 +242,7 @@ void s390_virtio_device_update_status(VirtIOS390Device *dev)
 VirtIODevice *vdev = dev->vdev;
 uint32_t features;
 
-vdev->status = ldub_phys(dev->dev_offs + VIRTIO_DEV_OFFS_STATUS);
+virtio_set_status(vdev, ldub_phys(dev->dev_offs + VIRTIO_DEV_OFFS_STATUS));
 
 /* Update guest supported feature bitmap */
 
diff --git a/hw/syborg_virtio.c b/hw/syborg_virtio.c
index 65239a0..abf0370 100644
--- a/hw/syborg_virtio.c
+++ b/hw/syborg_virtio.c
@@ -149,7 +149,7 @@ static void syborg_virtio_writel(void *opaque, 
target_phys_addr_t offset,
 virtio_queue_notify(vdev, value);
 break;
 case SYBORG_VIRTIO_STATUS:
-vdev->status = value & 0xFF;
+virtio_set_status(vdev, value & 0xFF);
 if (vdev->status == 0)
 virtio_reset(vdev);
 break;
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 799f664..ee67a8a 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -206,7 +206,7 @@ static void virtio_ioport_write(void *opaque, uint32_t 
addr, uint32_t val)
 virtio_queue_notify(vdev, val);
 break;
 case VIRTIO_PCI_STATUS:
-vdev->status = val & 0xFF;
+virtio_set_status(vdev, val & 0xFF);
 if (vdev->status == 0) {
 virtio_reset(proxy->vdev);
 msix_unuse_all_vectors(&proxy->pci_dev);
@@ -377,7 +377,8 @@ static void virtio_write_config(PCIDevice *pci_dev, 
uint32_t address,
 
 if (PCI_COMMAND == address) {
 if (!(val & PCI_COMMAND_MASTER)) {
-proxy->vdev->status &= ~VIRTIO_CONFIG_S_DRIVER_OK;
+virtio_set_status(proxy->vdev,
+  proxy->vdev->status & 
~VIRTIO_CONFIG_S_DRIVER_OK);
 }
 }
 
diff --git a/hw/virtio.h b/hw/virtio.h
index d0155e2..d3eb714 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -115,12 +115,21 @@ struct VirtIODevice
 void (*get_config)(VirtIODevice *vdev, uint8_t *config);
 void (*set_config)(VirtIODevice *vdev, const uint8_t *config);
 void (*reset)(VirtIODevice *vdev);
+void (*set_status)(VirtIODevice *vdev, uint8_t val);
 VirtQueue *vq;
 const VirtIOBindings *binding;
 void *binding_opaque;
 uint16_t device_id;
 };
 
+static inline void virtio_set_status(VirtIODevice *vdev, uint8_t val)
+{
+if (vdev->set_status) {
+vdev->set_status(vdev, val);
+}
+vdev->status = val;
+}
+
 VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
 void (*handle_output)(VirtIODevice *,
   VirtQueue *));
-- 
1.7.0.18.g0d53a5

[Qemu-devel] [PATCHv6 03/11] notifier: event notifier implementation

[Michael S. Tsirkin]

event notifiers are slightly generalized eventfd descriptors. Current
implementation depends on eventfd because vhost is the only user, and
vhost depends on eventfd anyway, but a stub is provided for non-eventfd
case.

We'll be able to further generalize this when another user comes along
and we see how to best do this.

Signed-off-by: Michael S. Tsirkin 
---
 Makefile.target |1 +
 hw/event_notifier.c |   62 +++
 hw/event_notifier.h |   16 +
 qemu-common.h   |1 +
 4 files changed, 80 insertions(+), 0 deletions(-)
 create mode 100644 hw/event_notifier.c
 create mode 100644 hw/event_notifier.h

diff --git a/Makefile.target b/Makefile.target
index ab3c438..004a703 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -175,6 +175,7 @@ obj-y = vl.o async.o monitor.o pci.o pci_host.o pcie_host.o 
machine.o gdbstub.o
 # virtio has to be here due to weird dependency between PCI and virtio-net.
 # need to fix this properly
 obj-y += virtio-blk.o virtio-balloon.o virtio-net.o virtio-pci.o 
virtio-serial-bus.o
+obj-y += event_notifier.o
 obj-y += rwhandler.o
 obj-$(CONFIG_KVM) += kvm.o kvm-all.o
 obj-$(CONFIG_ISA_MMIO) += isa_mmio.o
diff --git a/hw/event_notifier.c b/hw/event_notifier.c
new file mode 100644
index 000..13f3656
--- /dev/null
+++ b/hw/event_notifier.c
@@ -0,0 +1,62 @@
+/*
+ * event notifier support
+ *
+ * Copyright Red Hat, Inc. 2010
+ *
+ * Authors:
+ *  Michael S. Tsirkin 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ */
+
+#include "hw.h"
+#include "event_notifier.h"
+#ifdef CONFIG_EVENTFD
+#include 
+#endif
+
+int event_notifier_init(EventNotifier *e, int active)
+{
+#ifdef CONFIG_EVENTFD
+int fd = eventfd(!!active, EFD_NONBLOCK | EFD_CLOEXEC);
+if (fd < 0)
+return -errno;
+e->fd = fd;
+return 0;
+#else
+return -ENOSYS;
+#endif
+}
+
+void event_notifier_cleanup(EventNotifier *e)
+{
+close(e->fd);
+}
+
+int event_notifier_get_fd(EventNotifier *e)
+{
+return e->fd;
+}
+
+int event_notifier_test_and_clear(EventNotifier *e)
+{
+uint64_t value;
+int r = read(e->fd, &value, sizeof(value));
+return r == sizeof(value);
+}
+
+int event_notifier_test(EventNotifier *e)
+{
+uint64_t value;
+int r = read(e->fd, &value, sizeof(value));
+if (r == sizeof(value)) {
+/* restore previous value. */
+int s = write(e->fd, &value, sizeof(value));
+/* never blocks because we use EFD_SEMAPHORE.
+ * If we didn't we'd get EAGAIN on overflow
+ * and we'd have to write code to ignore it. */
+assert(s == sizeof(value));
+}
+return r == sizeof(value);
+}
diff --git a/hw/event_notifier.h b/hw/event_notifier.h
new file mode 100644
index 000..24117ea
--- /dev/null
+++ b/hw/event_notifier.h
@@ -0,0 +1,16 @@
+#ifndef QEMU_EVENT_NOTIFIER_H
+#define QEMU_EVENT_NOTIFIER_H
+
+#include "qemu-common.h"
+
+struct EventNotifier {
+   int fd;
+};
+
+int event_notifier_init(EventNotifier *, int active);
+void event_notifier_cleanup(EventNotifier *);
+int event_notifier_get_fd(EventNotifier *);
+int event_notifier_test_and_clear(EventNotifier *);
+int event_notifier_test(EventNotifier *);
+
+#endif
diff --git a/qemu-common.h b/qemu-common.h
index 805be1a..f12a8f5 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -227,6 +227,7 @@ typedef struct uWireSlave uWireSlave;
 typedef struct I2SCodec I2SCodec;
 typedef struct DeviceState DeviceState;
 typedef struct SSIBus SSIBus;
+typedef struct EventNotifier EventNotifier;
 
 typedef uint64_t pcibus_t;
 
-- 
1.7.0.18.g0d53a5

[Qemu-devel] [PATCHv6 04/11] virtio: notifier support + APIs for queue fields

[Michael S. Tsirkin]

vhost needs physical addresses for ring and other queue fields,
so add APIs for these. In particular, add binding API to set
host/guest notifiers.  Will be used by vhost.

Signed-off-by: Michael S. Tsirkin 
---
 hw/virtio.c |   80 ++-
 hw/virtio.h |   18 -
 2 files changed, 96 insertions(+), 2 deletions(-)

diff --git a/hw/virtio.c b/hw/virtio.c
index 7c020a3..f54129f 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -73,6 +73,9 @@ struct VirtQueue
 int inuse;
 uint16_t vector;
 void (*handle_output)(VirtIODevice *vdev, VirtQueue *vq);
+VirtIODevice *vdev;
+EventNotifier guest_notifier;
+EventNotifier host_notifier;
 };
 
 /* virt queue functions */
@@ -592,6 +595,12 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int 
queue_size,
 return &vdev->vq[i];
 }
 
+void virtio_irq(VirtQueue *vq)
+{
+vq->vdev->isr |= 0x01;
+virtio_notify_vector(vq->vdev, vq->vector);
+}
+
 void virtio_notify(VirtIODevice *vdev, VirtQueue *vq)
 {
 /* Always notify when queue is empty (when feature acknowledge) */
@@ -714,8 +723,10 @@ VirtIODevice *virtio_common_init(const char *name, 
uint16_t device_id,
 vdev->queue_sel = 0;
 vdev->config_vector = VIRTIO_NO_VECTOR;
 vdev->vq = qemu_mallocz(sizeof(VirtQueue) * VIRTIO_PCI_QUEUE_MAX);
-for(i = 0; i < VIRTIO_PCI_QUEUE_MAX; i++)
+for(i = 0; i < VIRTIO_PCI_QUEUE_MAX; i++) {
 vdev->vq[i].vector = VIRTIO_NO_VECTOR;
+vdev->vq[i].vdev = vdev;
+}
 
 vdev->name = name;
 vdev->config_len = config_size;
@@ -733,3 +744,70 @@ void virtio_bind_device(VirtIODevice *vdev, const 
VirtIOBindings *binding,
 vdev->binding = binding;
 vdev->binding_opaque = opaque;
 }
+
+target_phys_addr_t virtio_queue_get_desc_addr(VirtIODevice *vdev, int n)
+{
+return vdev->vq[n].vring.desc;
+}
+
+target_phys_addr_t virtio_queue_get_avail_addr(VirtIODevice *vdev, int n)
+{
+return vdev->vq[n].vring.avail;
+}
+
+target_phys_addr_t virtio_queue_get_used_addr(VirtIODevice *vdev, int n)
+{
+return vdev->vq[n].vring.used;
+}
+
+target_phys_addr_t virtio_queue_get_ring_addr(VirtIODevice *vdev, int n)
+{
+return vdev->vq[n].vring.desc;
+}
+
+target_phys_addr_t virtio_queue_get_desc_size(VirtIODevice *vdev, int n)
+{
+return sizeof(VRingDesc) * vdev->vq[n].vring.num;
+}
+
+target_phys_addr_t virtio_queue_get_avail_size(VirtIODevice *vdev, int n)
+{
+return offsetof(VRingAvail, ring) +
+sizeof(u_int64_t) * vdev->vq[n].vring.num;
+}
+
+target_phys_addr_t virtio_queue_get_used_size(VirtIODevice *vdev, int n)
+{
+return offsetof(VRingUsed, ring) +
+sizeof(VRingUsedElem) * vdev->vq[n].vring.num;
+}
+
+target_phys_addr_t virtio_queue_get_ring_size(VirtIODevice *vdev, int n)
+{
+return vdev->vq[n].vring.used - vdev->vq[n].vring.desc +
+   virtio_queue_get_used_size(vdev, n);
+}
+
+uint16_t virtio_queue_get_last_avail_idx(VirtIODevice *vdev, int n)
+{
+return vdev->vq[n].last_avail_idx;
+}
+
+void virtio_queue_set_last_avail_idx(VirtIODevice *vdev, int n, uint16_t idx)
+{
+vdev->vq[n].last_avail_idx = idx;
+}
+
+VirtQueue *virtio_get_queue(VirtIODevice *vdev, int n)
+{
+return vdev->vq + n;
+}
+
+EventNotifier *virtio_queue_get_guest_notifier(VirtQueue *vq)
+{
+return &vq->guest_notifier;
+}
+EventNotifier *virtio_queue_get_host_notifier(VirtQueue *vq)
+{
+return &vq->host_notifier;
+}
diff --git a/hw/virtio.h b/hw/virtio.h
index 3baa2a3..d0155e2 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -19,6 +19,7 @@
 #include "qdev.h"
 #include "sysemu.h"
 #include "block_int.h"
+#include "event_notifier.h"
 
 /* from Linux's linux/virtio_config.h */
 
@@ -89,6 +90,8 @@ typedef struct {
 int (*load_config)(void * opaque, QEMUFile *f);
 int (*load_queue)(void * opaque, int n, QEMUFile *f);
 unsigned (*get_features)(void * opaque);
+int (*set_guest_notifier)(void * opaque, int n, bool assigned);
+int (*set_host_notifier)(void * opaque, int n, bool assigned);
 } VirtIOBindings;
 
 #define VIRTIO_PCI_QUEUE_MAX 64
@@ -181,5 +184,18 @@ void virtio_net_exit(VirtIODevice *vdev);
DEFINE_PROP_BIT("indirect_desc", _state, _field, \
VIRTIO_RING_F_INDIRECT_DESC, true)
 
-
+target_phys_addr_t virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);
+target_phys_addr_t virtio_queue_get_avail_addr(VirtIODevice *vdev, int n);
+target_phys_addr_t virtio_queue_get_used_addr(VirtIODevice *vdev, int n);
+target_phys_addr_t virtio_queue_get_ring_addr(VirtIODevice *vdev, int n);
+target_phys_addr_t virtio_queue_get_desc_size(VirtIODevice *vdev, int n);
+target_phys_addr_t virtio_queue_get_avail_size(VirtIODevice *vdev, int n);
+target_phys_addr_t virtio_queue_get_used_size(VirtIODevice *vdev, int n);
+target_phys_addr_t virtio_queue_get_ring_size(VirtIODevice *vdev, int n);
+uint16_t virtio_queue_get_last_avail_idx(VirtIODevice *vdev, int n);
+void virtio_queue_set_last_avail_idx(Vir

[Qemu-devel] [PATCHv6 10/11] tap: add API to retrieve vhost net header

[Michael S. Tsirkin]

will be used by virtio-net for vhost net support

Signed-off-by: Michael S. Tsirkin 
---
 net/tap.c |7 +++
 net/tap.h |3 +++
 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/net/tap.c b/net/tap.c
index 19c4fa2..35c05d7 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -487,3 +487,10 @@ int net_init_tap(QemuOpts *opts, Monitor *mon, const char 
*name, VLANState *vlan
 
 return 0;
 }
+
+VHostNetState *tap_get_vhost_net(VLANClientState *nc)
+{
+TAPState *s = DO_UPCAST(TAPState, nc, nc);
+assert(nc->info->type == NET_CLIENT_TYPE_TAP);
+return s->vhost_net;
+}
diff --git a/net/tap.h b/net/tap.h
index a244b28..b8cec83 100644
--- a/net/tap.h
+++ b/net/tap.h
@@ -50,4 +50,7 @@ void tap_fd_set_offload(int fd, int csum, int tso4, int tso6, 
int ecn, int ufo);
 
 int tap_get_fd(VLANClientState *vc);
 
+struct vhost_net;
+struct vhost_net *tap_get_vhost_net(VLANClientState *vc);
+
 #endif /* QEMU_NET_TAP_H */
-- 
1.7.0.18.g0d53a5

[Qemu-devel] [PATCHv6 11/11] virtio-net: vhost net support

[Michael S. Tsirkin]

This connects virtio-net to vhost net backend.
The code is structured in a way analogous to what we have with vnet
header capability in tap.

We start/stop backend on driver start/stop as
well as on save and vm start (for migration).

Signed-off-by: Michael S. Tsirkin 
---
 hw/virtio-net.c |   71 +-
 1 files changed, 69 insertions(+), 2 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 5c0093e..9ddd58c 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -17,6 +17,7 @@
 #include "net/tap.h"
 #include "qemu-timer.h"
 #include "virtio-net.h"
+#include "vhost_net.h"
 
 #define VIRTIO_NET_VM_VERSION11
 
@@ -47,6 +48,8 @@ typedef struct VirtIONet
 uint8_t nomulti;
 uint8_t nouni;
 uint8_t nobcast;
+uint8_t vhost_started;
+VMChangeStateEntry *vmstate;
 struct {
 int in_use;
 int first_multi;
@@ -114,6 +117,10 @@ static void virtio_net_reset(VirtIODevice *vdev)
 n->nomulti = 0;
 n->nouni = 0;
 n->nobcast = 0;
+if (n->vhost_started) {
+vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), vdev);
+n->vhost_started = 0;
+}
 
 /* Flush any MAC and VLAN filter table state */
 n->mac_table.in_use = 0;
@@ -172,7 +179,14 @@ static uint32_t virtio_net_get_features(VirtIODevice 
*vdev, uint32_t features)
 features &= ~(0x1 << VIRTIO_NET_F_HOST_UFO);
 }
 
-return features;
+if (!n->nic->nc.peer ||
+n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) {
+return features;
+}
+if (!tap_get_vhost_net(n->nic->nc.peer)) {
+return features;
+}
+return vhost_net_get_features(tap_get_vhost_net(n->nic->nc.peer), 
features);
 }
 
 static uint32_t virtio_net_bad_features(VirtIODevice *vdev)
@@ -698,6 +712,12 @@ static void virtio_net_save(QEMUFile *f, void *opaque)
 {
 VirtIONet *n = opaque;
 
+if (n->vhost_started) {
+/* TODO: should we really stop the backend?
+ * If we don't, it might keep writing to memory. */
+vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), &n->vdev);
+n->vhost_started = 0;
+}
 virtio_save(&n->vdev, f);
 
 qemu_put_buffer(f, n->mac, ETH_ALEN);
@@ -810,7 +830,6 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int 
version_id)
 qemu_mod_timer(n->tx_timer,
qemu_get_clock(vm_clock) + TX_TIMER_INTERVAL);
 }
-
 return 0;
 }
 
@@ -830,6 +849,47 @@ static NetClientInfo net_virtio_info = {
 .link_status_changed = virtio_net_set_link_status,
 };
 
+static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
+{
+VirtIONet *n = to_virtio_net(vdev);
+if (!n->nic->nc.peer) {
+return;
+}
+if (n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) {
+return;
+}
+
+if (!tap_get_vhost_net(n->nic->nc.peer)) {
+return;
+}
+if (!!n->vhost_started == !!(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
+return;
+}
+if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
+int r = vhost_net_start(tap_get_vhost_net(n->nic->nc.peer), vdev);
+if (r < 0) {
+fprintf(stderr, "unable to start vhost net: %d: "
+"falling back on userspace virtio\n", -r);
+} else {
+n->vhost_started = 1;
+}
+} else {
+vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), vdev);
+n->vhost_started = 0;
+}
+}
+
+static void virtio_net_vmstate_change(void *opaque, int running, int reason)
+{
+VirtIONet *n = opaque;
+if (!running) {
+return;
+}
+/* This is called when vm is started, it will start vhost backend if
+ * appropriate e.g. after migration. */
+virtio_net_set_status(&n->vdev, n->vdev.status);
+}
+
 VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf)
 {
 VirtIONet *n;
@@ -845,6 +905,7 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf 
*conf)
 n->vdev.set_features = virtio_net_set_features;
 n->vdev.bad_features = virtio_net_bad_features;
 n->vdev.reset = virtio_net_reset;
+n->vdev.set_status = virtio_net_set_status;
 n->rx_vq = virtio_add_queue(&n->vdev, 256, virtio_net_handle_rx);
 n->tx_vq = virtio_add_queue(&n->vdev, 256, virtio_net_handle_tx);
 n->ctrl_vq = virtio_add_queue(&n->vdev, 64, virtio_net_handle_ctrl);
@@ -867,6 +928,7 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf 
*conf)
 
 register_savevm("virtio-net", virtio_net_id++, VIRTIO_NET_VM_VERSION,
 virtio_net_save, virtio_net_load, n);
+n->vmstate = qemu_add_vm_change_state_handler(virtio_net_vmstate_change, 
n);
 
 return &n->vdev;
 }
@@ -874,6 +936,11 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf 
*conf)
 void virtio_net_exit(VirtIODevice *vdev)
 {
 VirtIONet *n = DO_UPCAST(VirtIONet, vdev, vdev);
+qemu_del_vm_change_state_handler(n->vmstate);
+
+if (n->vhost_started) {
+  

[Qemu-devel] [PATCHv6 09/11] tap: add vhost/vhostfd options

[Michael S. Tsirkin]

This adds vhost binary option to tap, to enable vhost net accelerator.
Default is off for now, we'll be able to make default on long term
when we know it's stable.

vhostfd option can be used by management, to pass in the fd. Assigning
vhostfd implies vhost=on.

Signed-off-by: Michael S. Tsirkin 
---
 net.c   |8 
 net/tap.c   |   29 +
 qemu-options.hx |4 +++-
 3 files changed, 40 insertions(+), 1 deletions(-)

diff --git a/net.c b/net.c
index e47f727..48d9fb0 100644
--- a/net.c
+++ b/net.c
@@ -976,6 +976,14 @@ static const struct {
 .name = "vnet_hdr",
 .type = QEMU_OPT_BOOL,
 .help = "enable the IFF_VNET_HDR flag on the tap interface"
+}, {
+.name = "vhost",
+.type = QEMU_OPT_BOOL,
+.help = "enable vhost-net network accelerator",
+}, {
+.name = "vhostfd",
+.type = QEMU_OPT_STRING,
+.help = "file descriptor of an already opened vhost net 
device",
 },
 #endif /* _WIN32 */
 { /* end of list */ }
diff --git a/net/tap.c b/net/tap.c
index fc59fd4..19c4fa2 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -41,6 +41,8 @@
 
 #include "net/tap-linux.h"
 
+#include "hw/vhost_net.h"
+
 /* Maximum GSO packet size (64k) plus plenty of room for
  * the ethernet and virtio_net headers
  */
@@ -57,6 +59,7 @@ typedef struct TAPState {
 unsigned int has_vnet_hdr : 1;
 unsigned int using_vnet_hdr : 1;
 unsigned int has_ufo: 1;
+VHostNetState *vhost_net;
 } TAPState;
 
 static int launch_script(const char *setup_script, const char *ifname, int fd);
@@ -252,6 +255,10 @@ static void tap_cleanup(VLANClientState *nc)
 {
 TAPState *s = DO_UPCAST(TAPState, nc, nc);
 
+if (s->vhost_net) {
+vhost_net_cleanup(s->vhost_net);
+}
+
 qemu_purge_queued_packets(nc);
 
 if (s->down_script[0])
@@ -307,6 +314,7 @@ static TAPState *net_tap_fd_init(VLANState *vlan,
 s->has_ufo = tap_probe_has_ufo(s->fd);
 tap_set_offload(&s->nc, 0, 0, 0, 0, 0);
 tap_read_poll(s, 1);
+s->vhost_net = NULL;
 return s;
 }
 
@@ -456,5 +464,26 @@ int net_init_tap(QemuOpts *opts, Monitor *mon, const char 
*name, VLANState *vlan
 }
 }
 
+if (qemu_opt_get_bool(opts, "vhost", !!qemu_opt_get(opts, "vhostfd"))) {
+int vhostfd, r;
+if (qemu_opt_get(opts, "vhostfd")) {
+r = net_handle_fd_param(mon, qemu_opt_get(opts, "vhostfd"));
+if (r == -1) {
+return -1;
+}
+vhostfd = r;
+} else {
+vhostfd = -1;
+}
+s->vhost_net = vhost_net_init(&s->nc, vhostfd);
+if (!s->vhost_net) {
+qemu_error("vhost-net requested but could not be initialized\n");
+return -1;
+}
+} else if (qemu_opt_get(opts, "vhostfd")) {
+qemu_error("vhostfd= is not valid without vhost\n");
+return -1;
+}
+
 return 0;
 }
diff --git a/qemu-options.hx b/qemu-options.hx
index fd50add..4d9f4da 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -895,7 +895,7 @@ DEF("net", HAS_ARG, QEMU_OPTION_net,
 "-net tap[,vlan=n][,name=str],ifname=name\n"
 "connect the host TAP network interface to VLAN 'n'\n"
 #else
-"-net 
tap[,vlan=n][,name=str][,fd=h][,ifname=name][,script=file][,downscript=dfile][,sndbuf=nbytes][,vnet_hdr=on|off]\n"
+"-net 
tap[,vlan=n][,name=str][,fd=h][,ifname=name][,script=file][,downscript=dfile][,sndbuf=nbytes][,vnet_hdr=on|off][,vhost=on|off][,vhostfd=h]\n"
 "connect the host TAP network interface to VLAN 'n' and 
use the\n"
 "network scripts 'file' (default=" DEFAULT_NETWORK_SCRIPT 
")\n"
 "and 'dfile' (default=" DEFAULT_NETWORK_DOWN_SCRIPT ")\n"
@@ -905,6 +905,8 @@ DEF("net", HAS_ARG, QEMU_OPTION_net,
 "default of 'sndbuf=1048576' can be disabled using 
'sndbuf=0')\n"
 "use vnet_hdr=off to avoid enabling the IFF_VNET_HDR tap 
flag\n"
 "use vnet_hdr=on to make the lack of IFF_VNET_HDR support 
an error condition\n"
+"use vhost=on to enable experimental in kernel 
accelerator\n"
+"use 'vhostfd=h' to connect to an already opened vhost net 
device\n"
 #endif
 "-net 
socket[,vlan=n][,name=str][,fd=h][,listen=[host]:port][,connect=host:port]\n"
 "connect the vlan 'n' to another VLAN using a socket 
connection\n"
-- 
1.7.0.18.g0d53a5

Re: [Qemu-devel] Re: >2 serial ports?

[Paul Brook]

> Oh, well, yes, I remember.  qemu is more strict on ISA irq sharing now.
>   A bit too strict.
> 
> /me goes dig out a old patch which never made it upstream for some
> reason I forgot.  Attached.

This is wrong. Two devices should never be manipulating the same qemu_irq 
object.  If you want multiple devices connected to the same IRQ then you need 
an explicit multiplexer. e.g. arm_timer.c:sp804_set_irq.

Paul

[Qemu-devel] [PATCHv6 02/11] kvm: add API to set ioeventfd

[Michael S. Tsirkin]

Comment on kvm usage: rather than require users to do if (kvm_enabled())
and/or ifdefs, this patch adds an API that, internally, is defined to
stub function on non-kvm build, and checks kvm_enabled for non-kvm
run.

While rest of qemu code still uses if (kvm_enabled()), I think this
approach is cleaner, and we should convert rest of code to it
long term.

Signed-off-by: Michael S. Tsirkin 
---
 kvm-all.c |   22 ++
 kvm.h |   16 
 2 files changed, 38 insertions(+), 0 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index 534ead0..f427f73 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1153,3 +1153,25 @@ int kvm_set_signal_mask(CPUState *env, const sigset_t 
*sigset)
 
 return r;
 }
+
+#ifdef KVM_IOEVENTFD
+int kvm_set_ioeventfd_pio_word(int fd, uint16_t addr, uint16_t val, bool 
assign)
+{
+struct kvm_ioeventfd kick = {
+.datamatch = val,
+.addr = addr,
+.len = 2,
+.flags = KVM_IOEVENTFD_FLAG_DATAMATCH | KVM_IOEVENTFD_FLAG_PIO,
+.fd = fd,
+};
+int r;
+if (!kvm_enabled())
+return -ENOSYS;
+if (!assign)
+kick.flags |= KVM_IOEVENTFD_FLAG_DEASSIGN;
+r = kvm_vm_ioctl(kvm_state, KVM_IOEVENTFD, &kick);
+if (r < 0)
+return r;
+return 0;
+}
+#endif
diff --git a/kvm.h b/kvm.h
index fd8d0c1..2dfcb15 100644
--- a/kvm.h
+++ b/kvm.h
@@ -14,10 +14,16 @@
 #ifndef QEMU_KVM_H
 #define QEMU_KVM_H
 
+#include 
+#include 
 #include "config.h"
 #include "qemu-queue.h"
 
 #ifdef CONFIG_KVM
+#include 
+#endif
+
+#ifdef CONFIG_KVM
 extern int kvm_allowed;
 
 #define kvm_enabled() (kvm_allowed)
@@ -161,4 +167,14 @@ static inline void cpu_synchronize_post_init(CPUState *env)
 }
 }
 
+#if defined(KVM_IOEVENTFD) && defined(CONFIG_KVM)
+int kvm_set_ioeventfd_pio_word(int fd, uint16_t adr, uint16_t val, bool 
assign);
+#else
+static inline
+int kvm_set_ioeventfd_pio_word(int fd, uint16_t adr, uint16_t val, bool assign)
+{
+return -ENOSYS;
+}
+#endif
+
 #endif
-- 
1.7.0.18.g0d53a5

[Qemu-devel] [PATCHv6 01/11] tap: add interface to get device fd

[Michael S. Tsirkin]

Will be used by vhost to attach/detach to backend.

Signed-off-by: Michael S. Tsirkin 
---
 net/tap.c |7 +++
 net/tap.h |2 ++
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/net/tap.c b/net/tap.c
index 7a7320c..fc59fd4 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -269,6 +269,13 @@ static void tap_poll(VLANClientState *nc, bool enable)
 tap_write_poll(s, enable);
 }
 
+int tap_get_fd(VLANClientState *nc)
+{
+TAPState *s = DO_UPCAST(TAPState, nc, nc);
+assert(nc->info->type == NET_CLIENT_TYPE_TAP);
+return s->fd;
+}
+
 /* fd support */
 
 static NetClientInfo net_tap_info = {
diff --git a/net/tap.h b/net/tap.h
index 538a562..a244b28 100644
--- a/net/tap.h
+++ b/net/tap.h
@@ -48,4 +48,6 @@ int tap_probe_vnet_hdr(int fd);
 int tap_probe_has_ufo(int fd);
 void tap_fd_set_offload(int fd, int csum, int tso4, int tso6, int ecn, int 
ufo);
 
+int tap_get_fd(VLANClientState *vc);
+
 #endif /* QEMU_NET_TAP_H */
-- 
1.7.0.18.g0d53a5

[Qemu-devel] [PATCHv6 00/11] vhost-net: upstream integration

[Michael S. Tsirkin]

Here's a patchset with vhost support for upstream qemu,
rebased to latest bits, and with all comments I'm aware of
addressed.

Please consider for merging.

Changes from v5:
  address minor comments by Amit and Juan

Changes from v4:
  address amit's style comments: mostly renaming for clarity

Changes from v3:
  vhost: vhost net support: use typedef instead of struct name
  virtio: add set_status callback: fix up non-PCI bindings

Changes from v2:
  Addressed style comments
  Detect mapping changes and abort
  Unmap ring on cleanup

Changes from v1:
  Addressed style comments
  Migration fixes.
  Gracefully fail with non-tap backends.

Michael S. Tsirkin (11):
  tap: add interface to get device fd
  kvm: add API to set ioeventfd
  notifier: event notifier implementation
  virtio: notifier support + APIs for queue fields
  virtio: add set_status callback
  virtio: move typedef to qemu-common
  virtio-pci: fill in notifier support
  vhost: vhost net support
  tap: add vhost/vhostfd options
  tap: add API to retrieve vhost net header
  virtio-net: vhost net support

 Makefile.target  |3 +
 configure|   37 +++
 hw/event_notifier.c  |   62 +
 hw/event_notifier.h  |   16 ++
 hw/s390-virtio-bus.c |2 +-
 hw/syborg_virtio.c   |2 +-
 hw/vhost.c   |  711 ++
 hw/vhost.h   |   48 
 hw/vhost_net.c   |  195 ++
 hw/vhost_net.h   |   19 ++
 hw/virtio-net.c  |   71 +-
 hw/virtio-pci.c  |   68 +-
 hw/virtio.c  |   80 ++-
 hw/virtio.h  |   28 ++-
 kvm-all.c|   22 ++
 kvm.h|   16 ++
 net.c|8 +
 net/tap.c|   43 +++
 net/tap.h|5 +
 qemu-common.h|2 +
 qemu-options.hx  |4 +-
 21 files changed, 1432 insertions(+), 10 deletions(-)
 create mode 100644 hw/event_notifier.c
 create mode 100644 hw/event_notifier.h
 create mode 100644 hw/vhost.c
 create mode 100644 hw/vhost.h
 create mode 100644 hw/vhost_net.c
 create mode 100644 hw/vhost_net.h

[Qemu-devel] Re: [PATCHv5 04/11] virtio: notifier support + APIs for queue fields

[Michael S. Tsirkin]

On Wed, Mar 17, 2010 at 09:44:05AM +0530, Amit Shah wrote:
> On (Tue) Mar 16 2010 [19:10:58], Michael S. Tsirkin wrote:
> > 
> > diff --git a/hw/virtio.c b/hw/virtio.c
> > index 7c020a3..f54129f 100644
> > --- a/hw/virtio.c
> > +++ b/hw/virtio.c
> > @@ -73,6 +73,9 @@ struct VirtQueue
> >  int inuse;
> >  uint16_t vector;
> >  void (*handle_output)(VirtIODevice *vdev, VirtQueue *vq);
> > +VirtIODevice *vdev;
> > +EventNotifier guest_notifier;
> > +EventNotifier host_notifier;
> 
> Another thing that terribly confused me was this: in the vq struct you
> have guest_notifier and host_notifier of type EventNotifier and in the
> virtio binding structs you have guest_notifier and host_notifier which
> are function callbacks.
> Also, these vq->{guest_|host_}notifier assignments weren't easily found
> using grep because they're assigned by assigning a pointer value and
> initialising that pointer. Thanks to Juan for finding that for me :-)
> 
>   Amit

I've renamed them to set_xxx_notifier to avoid confusion.

> -- 
> http://log.amitshah.net/

[Qemu-devel] Re: [PATCHv5 11/11] virtio-net: vhost net support

[Michael S. Tsirkin]

On Tue, Mar 16, 2010 at 07:57:51PM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin"  wrote:
> > This connects virtio-net to vhost net backend.
> > The code is structured in a way analogous to what we have with vnet
> > header capability in tap.
> >
> > We start/stop backend on driver start/stop as
> > well as on save and vm start (for migration).
> >
> 
> > +uint8_t vhost_started;
> 
> 
> I didn't noticed the 1st time.  vhost_started looks like a bool :p
> 
> Later, Juan.

Yes. I have no idea why, but the surrounding code has a lot of flags
all of which are uint8_t, so I followed suit.

-- 
MST

[Qemu-devel] Re: >2 serial ports?

[Michael Tokarev]

Gerd Hoffmann wrote:
> On 03/17/10 09:38, Michael Tokarev wrote:
>> Since 0.12, it appears that kvm does not allow more than
>> 2 serial ports for a guest:
>>
>> $ kvm \
>>   -serial unix:s1,server,nowait \
>>   -serial unix:s2,server,nowait \
>>   -serial unix:s3,server,nowait
>> isa irq 4 already assigned
>>
>> Is there a work-around for this?
> 
> Oh, well, yes, I remember.  qemu is more strict on ISA irq sharing now.
>  A bit too strict.
> 
> /me goes dig out a old patch which never made it upstream for some
> reason I forgot.  Attached.

I tried the patch, and it now appears to work.  I did not try
to run various stress tests so far, but basic tests are fine.

Thank you Gerd!  And I think it's time to push it finally :)

/mjt

[Qemu-devel] Re: >2 serial ports?

[Gerd Hoffmann]

On 03/17/10 09:38, Michael Tokarev wrote:

Since 0.12, it appears that kvm does not allow more than
2 serial ports for a guest:

$ kvm \
  -serial unix:s1,server,nowait \
  -serial unix:s2,server,nowait \
  -serial unix:s3,server,nowait
isa irq 4 already assigned

Is there a work-around for this?


Oh, well, yes, I remember.  qemu is more strict on ISA irq sharing now. 
 A bit too strict.


/me goes dig out a old patch which never made it upstream for some 
reason I forgot.  Attached.


HTH,
  Gerd
>From 7d5d53e8a23544ac6413487a8ecdd43537ade9f3 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann 
Date: Fri, 11 Sep 2009 13:43:46 +0200
Subject: [PATCH] isa: refine irq reservations

There are a few cases where IRQ sharing on the ISA bus is used and
possible.  In general only devices of the same kind can do that.
A few use cases:

  * serial lines 1+3 share irq 4
  * serial lines 2+4 share irq 3
  * parallel ports share irq 7
  * ppc/prep: ide ports share irq 13

This patch refines the irq reservation mechanism for the isa bus to
handle those cases.  It keeps track of the driver which owns the IRQ in
question and allows irq sharing for devices handled by the same driver.

Signed-off-by: Gerd Hoffmann 
---
 hw/isa-bus.c |   16 +---
 1 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/hw/isa-bus.c b/hw/isa-bus.c
index 4d489d2..bd2f69c 100644
--- a/hw/isa-bus.c
+++ b/hw/isa-bus.c
@@ -26,6 +26,7 @@ struct ISABus {
 BusState qbus;
 qemu_irq *irqs;
 uint32_t assigned;
+DeviceInfo *irq_owner[16];
 };
 static ISABus *isabus;
 
@@ -71,7 +72,9 @@ qemu_irq isa_reserve_irq(int isairq)
 exit(1);
 }
 if (isabus->assigned & (1 << isairq)) {
-fprintf(stderr, "isa irq %d already assigned\n", isairq);
+DeviceInfo *owner = isabus->irq_owner[isairq];
+fprintf(stderr, "isa irq %d already assigned (%s)\n",
+isairq, owner ? owner->name : "unknown");
 exit(1);
 }
 isabus->assigned |= (1 << isairq);
@@ -82,10 +85,17 @@ void isa_init_irq(ISADevice *dev, qemu_irq *p, int isairq)
 {
 assert(dev->nirqs < ARRAY_SIZE(dev->isairq));
 if (isabus->assigned & (1 << isairq)) {
-fprintf(stderr, "isa irq %d already assigned\n", isairq);
-exit(1);
+DeviceInfo *owner = isabus->irq_owner[isairq];
+if (owner == dev->qdev.info) {
+/* irq sharing is ok in case the same driver handles both */;
+} else {
+fprintf(stderr, "isa irq %d already assigned (%s)\n",
+isairq, owner ? owner->name : "unknown");
+exit(1);
+}
 }
 isabus->assigned |= (1 << isairq);
+isabus->irq_owner[isairq] = dev->qdev.info;
 dev->isairq[dev->nirqs] = isairq;
 *p = isabus->irqs[isairq];
 dev->nirqs++;
-- 
1.6.6.1

Re: [Qemu-devel] Re: [PATCH, RFC] Replace assert(0) with abort() or cpu_abort()

[Paolo Bonzini]

On 03/16/2010 06:55 PM, Jamie Lokier wrote:

A guest program is also allowed to trap SIGABRT with a signal handler,
and that does have some uses.  E.g. cleaning up temporary files and
shmem segments following a crash when calling 3rd party code.

Whatever the guest does with SIGABRT, it should not result in_QEMU_
crashing - whether due to abort() returning, or QEMU's control flow
jumping to the guest's signal handler from an unexpected location.


That's very hard to ensure however if QEMU was already in unstable 
state, as witnessed by its call to abort().  Things can only go downhill.


Maybe there should be a qemu_abort wrapper (and a QEMU_ASSERT companion) 
that does simply abort/assert under system emulation, but under 
user-mode emulation does


  signal (SIGABRT, SIG_DFL);
  abort ();

Paolo