[Qemu-devel] [PATCH v2 2/4] target-arm: Add trace events for the generic timers

[Peter Maydell]

Add some useful trace events for the ARM generic timers (notably
the various register writes and the resulting IRQ line state).

Signed-off-by: Peter Maydell 
Reviewed-by: Edgar E. Iglesias 
---
 Makefile.objs   |  1 +
 target-arm/helper.c | 20 
 target-arm/trace-events | 10 ++
 3 files changed, 27 insertions(+), 4 deletions(-)
 create mode 100644 target-arm/trace-events

diff --git a/Makefile.objs b/Makefile.objs
index 02fb8e7..69fdd48 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -155,6 +155,7 @@ trace-events-y += hw/alpha/trace-events
 trace-events-y += ui/trace-events
 trace-events-y += audio/trace-events
 trace-events-y += net/trace-events
+trace-events-y += target-arm/trace-events
 trace-events-y += target-i386/trace-events
 trace-events-y += target-sparc/trace-events
 trace-events-y += target-s390x/trace-events
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 23792ab..5fcdc2b 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -1,4 +1,5 @@
 #include "qemu/osdep.h"
+#include "trace.h"
 #include "cpu.h"
 #include "internals.h"
 #include "exec/gdbstub.h"
@@ -1560,10 +1561,13 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx)
 /* Note that this must be unsigned 64 bit arithmetic: */
 int istatus = count - offset >= gt->cval;
 uint64_t nexttick;
+int irqstate;
 
 gt->ctl = deposit32(gt->ctl, 2, 1, istatus);
-qemu_set_irq(cpu->gt_timer_outputs[timeridx],
- (istatus && !(gt->ctl & 2)));
+
+irqstate = (istatus && !(gt->ctl & 2));
+qemu_set_irq(cpu->gt_timer_outputs[timeridx], irqstate);
+
 if (istatus) {
 /* Next transition is when count rolls back over to zero */
 nexttick = UINT64_MAX;
@@ -1580,11 +1584,13 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx)
 nexttick = INT64_MAX / GTIMER_SCALE;
 }
 timer_mod(cpu->gt_timer[timeridx], nexttick);
+trace_arm_gt_recalc(timeridx, irqstate, nexttick);
 } else {
 /* Timer disabled: ISTATUS and timer output always clear */
 gt->ctl &= ~4;
 qemu_set_irq(cpu->gt_timer_outputs[timeridx], 0);
 timer_del(cpu->gt_timer[timeridx]);
+trace_arm_gt_recalc_disabled(timeridx);
 }
 }
 
@@ -1610,6 +1616,7 @@ static void gt_cval_write(CPUARMState *env, const 
ARMCPRegInfo *ri,
   int timeridx,
   uint64_t value)
 {
+trace_arm_gt_cval_write(timeridx, value);
 env->cp15.c14_timer[timeridx].cval = value;
 gt_recalc_timer(arm_env_get_cpu(env), timeridx);
 }
@@ -1629,6 +1636,7 @@ static void gt_tval_write(CPUARMState *env, const 
ARMCPRegInfo *ri,
 {
 uint64_t offset = timeridx == GTIMER_VIRT ? env->cp15.cntvoff_el2 : 0;
 
+trace_arm_gt_tval_write(timeridx, value);
 env->cp15.c14_timer[timeridx].cval = gt_get_countervalue(env) - offset +
  sextract64(value, 0, 32);
 gt_recalc_timer(arm_env_get_cpu(env), timeridx);
@@ -1641,6 +1649,7 @@ static void gt_ctl_write(CPUARMState *env, const 
ARMCPRegInfo *ri,
 ARMCPU *cpu = arm_env_get_cpu(env);
 uint32_t oldval = env->cp15.c14_timer[timeridx].ctl;
 
+trace_arm_gt_ctl_write(timeridx, value);
 env->cp15.c14_timer[timeridx].ctl = deposit64(oldval, 0, 2, value);
 if ((oldval ^ value) & 1) {
 /* Enable toggled */
@@ -1649,8 +1658,10 @@ static void gt_ctl_write(CPUARMState *env, const 
ARMCPRegInfo *ri,
 /* IMASK toggled: don't need to recalculate,
  * just set the interrupt line based on ISTATUS
  */
-qemu_set_irq(cpu->gt_timer_outputs[timeridx],
- (oldval & 4) && !(value & 2));
+int irqstate = (oldval & 4) && !(value & 2);
+
+trace_arm_gt_imask_toggle(timeridx, irqstate);
+qemu_set_irq(cpu->gt_timer_outputs[timeridx], irqstate);
 }
 }
 
@@ -1715,6 +1726,7 @@ static void gt_cntvoff_write(CPUARMState *env, const 
ARMCPRegInfo *ri,
 {
 ARMCPU *cpu = arm_env_get_cpu(env);
 
+trace_arm_gt_cntvoff_write(value);
 raw_write(env, ri, value);
 gt_recalc_timer(cpu, GTIMER_VIRT);
 }
diff --git a/target-arm/trace-events b/target-arm/trace-events
new file mode 100644
index 000..9f726bd
--- /dev/null
+++ b/target-arm/trace-events
@@ -0,0 +1,10 @@
+# See docs/tracing.txt for syntax documentation.
+
+# target-arm/helper.c
+arm_gt_recalc(int timer, int irqstate, uint64_t nexttick) "gt recalc: timer %d 
irqstate %d next tick %" PRIx64
+arm_gt_recalc_disabled(int timer) "gt recalc: timer %d irqstate 0 timer 
disabled"
+arm_gt_cval_write(int timer, uint64_t value) "gt_cval_write: timer %d value %" 
PRIx64
+arm_gt_tval_write(int timer, uint64_t value) "gt_tval_write: timer %d value %" 
PRIx64
+arm_gt_ctl_write(int timer, uint64_t value) "gt_ctl_write: timer %d value %" 
PRIx64

[Qemu-devel] [PATCH v2 0/4] preliminaries for GICv3 virt support

[Peter Maydell]

This set of straightforward patches is a preliminary
for adding virtualization support to the GICv3 emulation:
 * add a (nop implementation of) MDCCINT_EL1, since KVM
   will read/write it on worldswitch
 * fix some bugs in the GICv3 trace events
 * add trace events for the generic timers
   (which I have been using for debugging)
 * add trace events for the serial port (again, debug use)

Changes v1->v2:
 * added the missing target-arm/trace-events file to
   patch 2
 * new patch 4 with pl011 trace events

Peter Maydell (4):
  target-arm: Implement dummy MDCCINT_EL1
  target-arm: Add trace events for the generic timers
  hw/intc/arm_gicv3: Fix ICC register tracepoints
  hw/char/pl011: Add trace events

 Makefile.objs |  1 +
 hw/char/pl011.c   | 71 +--
 hw/char/trace-events  |  9 ++
 hw/intc/arm_gicv3_cpuif.c | 23 +--
 hw/intc/trace-events  | 14 +-
 target-arm/helper.c   | 28 ---
 target-arm/trace-events   | 10 +++
 7 files changed, 116 insertions(+), 40 deletions(-)
 create mode 100644 target-arm/trace-events

-- 
2.7.4

Re: [Qemu-devel] [PATCH v14 12/21] option: allow qemu_opts_to_qdict to merge repeated options

[Markus Armbruster]

"Daniel P. Berrange"  writes:

> If given an option string such as
>
>   size=1024,nodes=10,nodes=4-5,nodes=1-2,policy=bind
>
> the qemu_opts_to_qdict() method will currently overwrite
> the values for repeated option keys, so only the last
> value is in the returned dict:
>
> size=QString("1024")
> nodes=QString("1-2")
> policy=QString("bind")
>
> With this change the caller can optionally ask for all
> the repeated values to be stored in a QList. In the
> above example that would result in 'nodes' being a
> QList, so the returned dict would contain
>
> size=QString("1024")
> nodes=QList([QString("10"),
>  QString("4-5"),
>  QString("1-2")])
> policy=QString("bind")
>
> Note that the conversion has no way of knowing whether
> any given key is expected to be a list upfront - it can
> only figure that out when seeing the first duplicated
> key. Thus the caller has to be prepared to deal with the
> fact that if a key 'foo' is a list, then the returned
> qdict may contain either a QString or a QList for the
> key 'foo'.
>
> In a third mode, it is possible to ask for repeated
> options to be reported as an error, rather than silently
> dropping all but the last one.

To serve as a replacement for the options visitor, this needs to be able
to behave exactly the same together with a suitably hacked up QObject
input visitor.  Before I dive into the actual patch, let me summarize
QemuOpts and options visitor behavior.

Warning, this is going to get ugly.

QemuOpts faithfully represents a key=value,... string as a list of
QemuOpt.  Each QemuOpt represents one key=value.  They are in the same
order.  If key occurs multiple times in the string, it occurs just the
same in the list.

*Except* key "id" is special: it's stored outside the list, and all but
the first one are silently ignored.

Most users only ever get the last value of a key.  Any non-last
key=value are silently ignored.

We actually exploit this behavior to do defaults, by *prepending* them
to the list.  See the use of qemu_opts_set_defaults() in main().

A few users get all values of keys (other than key "id"):

* -device, in qdev_device_add() with callback set_property().

  We first get "driver" and "bus" normally (silently ignoring non-last
  values, as usual).  All other keys are device properties.  To set
  them, we get all (key, value), ignore keys "driver" and "bus", and set
  the rest.  If a key occurs multiple times, it gets set multiple times.
  This effectively ignores all but the last one, silently.

* -semihosting-config, in main() with callback add_semihosting_arg().

  We first get a bunch of keys normally.  Key "arg" is special: it may
  be repeated to build a list.  To implement that, we get all (key,
  value), ignore keys other than "arg", and accumulate the values.

* -machine & friends, in main() with callback machine_set_property()

  Similar to -device, only for machines, with "type" instead of "driver"
  and "bus".

* -spice, in qemu_spice_init() with callback add_channel()

  Keys "tls-channel" and "plaintext-channel" may be used repeated to
  specify multiple channels.  To implement that, we get all (key,
  value), ignore keys other than "tls-channel" and "plaintext-channel",
  and set up a channel for each of the others.

* -writeconfig, in config_write_opts() with callback config_write_opt()

  We write out all keys in order.

* The options visitor, in opts_start_struct()

  We convert the list of (key, value) to a hash table of (key, list of
  values).  Most of the time, the list of values has exactly one
  element.

  When the visitor's user asks for a scalar, we return the last element
  of the list of values, in lookup_scalar().

  When the user asks for list elements, we return the elements of the
  list of values in order, in opts_next_list(), or if there are none,
  the empty list in opts_start_list().

Unlike the options visitor, this patch (judging from your description)
makes a list only when keys are repeated.  The QObject visitor will have
to cope with finding both scalars and lists.  When it finds a scalar,
but needs a list, it'll have to wrap it in a list (PATCH 09, I think).
When it finds a list, but needs a scalar, it'll have to fish it out of
the list (where is that?).

> All existing callers are all converted to explicitly
> request the historical behaviour of only reporting the
> last key. Later patches will make use of the new modes.
>
> Signed-off-by: Daniel P. Berrange 

Out of steam for today.

Re: [Qemu-devel] [Qemu-ppc] [QEMU PATCH v5 3/6] migration: extend VMStateInfo

[Jianjun Duan]

On 10/12/2016 05:07 AM, Paolo Bonzini wrote:
> 
> 
> On 12/10/2016 13:59, Halil Pasic wrote:
>> IMHO this would:
>> * allow us to keep the good old MVStateInfo objects unmodified and
>>   the semantic of VMStateInfo unchanged
>> * make clear that VMStateLinked does not care about the calculated size
>>   and provide a new anchor for documentation
>> * if overloading the semantic of VMStateField.start is not desired we
>>   could put it into  VMStateLinked, if needed we could also put more
>>   stuff in there
>> * have clearer separation between special handling for (linked/certain)
>>   data structures and the usual scenario with the DAG.
> 
> No, I disagree.  We shouldn't be worried about making intrusive changes
> to all invocations or declarations, if that leads to a simpler API.
> 
If VMStateInfo was meant for complete customization, then it was missing
some parts. I think the API is indeed simpler. Just like
definition for VMStateField. Not all of its fields are used all time.

> I agree that overloading .start can be a bit ugly but you can choose to
> overload .num_offset instead, which is already better.
>
I did considered num_offset. But it is associated with an actual field.
On the other hand, start means the start position of the q in the
structure. So it is not that far stretched.

>> I would also suggest unit tests in test/test-vmstate.c. Maybe with
>> that the vmstate migration of QTAILQ could be factored out as a separate
>> patch series.
> 
> Yes, definitely.
> 
This sounds a good idea. Will do it.

> Paolo
> 
Thanks,
Jianjun

Re: [Qemu-devel] [PATCH] target-lm32: fix style issue

[Peter Maydell]

On 12 October 2016 at 18:11, Michael Walle  wrote:
> Am 2016-10-12 18:35, schrieb Peter Maydell:
>> but I noticed while doing the review that our LOG_DIS
>> is wrong for the compare-immediates:
>>
>> LOG_DIS("cmpei r%d, r%d, %d\n", dc->r0, dc->r1,
>> sign_extend(dc->imm16, 16));
>>
>> but the processor reference manual says cmpei's mnemonic
>> should have dc->r1 first and dc->r0 second.

> can I drop the DISAS_LM32 macro and just always enable the
> qemu_log_mask(CPU_LOG_TB_IN_ASM)? Looking at other CPUs this is sometimes a
> (debug) compile switch (eg ppc) and sometimes its always enabled (tilegx).

tilegx unconditionally does this logging because there's
no tilegx disassembler in disas/, and so printing log
statements in the decoder is a dubious but low effort way
to achieve the desired effect (that -d in_asm prints the
guest disassembly).

ppc on the other hand has a proper disassembler in disas/,
and so the LOG_DISAS() stuff is just debug-printf stuff
for the purposes of tracking down bugs in the decoder,
and like most of the debug-printf macros in devices it
is not compiled in by default (you turn it on by hand if
you have a bug it might help with), and exactly what is
logged is very much ad-hoc.

But lm32 has a disassembler in disas, so it doesn't need
to emit this stuff for -d in_asm to work, and indeed shouldn't
emit it by default, otherwise all the disassembly would be
printed twice. You can see this bug if you do:

./build/x86-all/lm32-softmmu/qemu-system-lm32  -d in_asm -D
/tmp/lm32.log -M milkymist -serial stdio -kernel
~/test-images/milkymist/flickernoise

as the resulting log looks like:

4000:   9800xor r0, r0, r0
4004:   d000wcsr r0, 0

0x4000:  98 00 00 00xor  r0, r0, r0
0x4004:  d0 00 00 00wcsr ie, r0

isize=8 osize=11
4008:   d020wcsr r0, 1

0x4008:  d0 20 00 00wcsr im, r0

isize=4 osize=9
400c:   78014000mvhi r1, 16384
4010:   3821ori r1, r1, 0
4014:   d0e1wcsr r1, 7
4018:   e03abi 232

0x400c:  78 01 40 00orhi r1, r0, 0x4000
0x4010:  38 21 00 00ori  r1, r1, 0x0
0x4014:  d0 e1 00 00wcsr eba, r1
0x4018:  e0 00 00 3abi   4100

mixing the disassembly from the disassembler
with the debug output from the translate.c code in
a somewhat confusing way.

thanks
-- PMM

[Qemu-devel] [PATCH] target-lm32: fix LOG_DIS operand order

[Michael Walle]

The order of most opcodes with immediates was wrong (according to the
reference manual) in the (debug) logging. Additionally, one operand for the
andhi instruction was completly wrong. Fix these.

Signed-off-by: Michael Walle 
---
 target-lm32/translate.c | 34 +-
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/target-lm32/translate.c b/target-lm32/translate.c
index 534c17c..dc64cc6 100644
--- a/target-lm32/translate.c
+++ b/target-lm32/translate.c
@@ -211,7 +211,7 @@ static void dec_and(DisasContext *dc)
 
 static void dec_andhi(DisasContext *dc)
 {
-LOG_DIS("andhi r%d, r%d, %d\n", dc->r2, dc->r0, dc->imm16);
+LOG_DIS("andhi r%d, r%d, %d\n", dc->r1, dc->r0, dc->imm16);
 
 tcg_gen_andi_tl(cpu_R[dc->r1], cpu_R[dc->r0], (dc->imm16 << 16));
 }
@@ -274,7 +274,7 @@ static inline void gen_cond_branch(DisasContext *dc, int 
cond)
 
 static void dec_be(DisasContext *dc)
 {
-LOG_DIS("be r%d, r%d, %d\n", dc->r0, dc->r1,
+LOG_DIS("be r%d, r%d, %d\n", dc->r1, dc->r0,
 sign_extend(dc->imm16, 16) * 4);
 
 gen_cond_branch(dc, TCG_COND_EQ);
@@ -282,7 +282,7 @@ static void dec_be(DisasContext *dc)
 
 static void dec_bg(DisasContext *dc)
 {
-LOG_DIS("bg r%d, r%d, %d\n", dc->r0, dc->r1,
+LOG_DIS("bg r%d, r%d, %d\n", dc->r1, dc->r0,
 sign_extend(dc->imm16, 16 * 4));
 
 gen_cond_branch(dc, TCG_COND_GT);
@@ -290,7 +290,7 @@ static void dec_bg(DisasContext *dc)
 
 static void dec_bge(DisasContext *dc)
 {
-LOG_DIS("bge r%d, r%d, %d\n", dc->r0, dc->r1,
+LOG_DIS("bge r%d, r%d, %d\n", dc->r1, dc->r0,
 sign_extend(dc->imm16, 16) * 4);
 
 gen_cond_branch(dc, TCG_COND_GE);
@@ -298,7 +298,7 @@ static void dec_bge(DisasContext *dc)
 
 static void dec_bgeu(DisasContext *dc)
 {
-LOG_DIS("bgeu r%d, r%d, %d\n", dc->r0, dc->r1,
+LOG_DIS("bgeu r%d, r%d, %d\n", dc->r1, dc->r0,
 sign_extend(dc->imm16, 16) * 4);
 
 gen_cond_branch(dc, TCG_COND_GEU);
@@ -306,7 +306,7 @@ static void dec_bgeu(DisasContext *dc)
 
 static void dec_bgu(DisasContext *dc)
 {
-LOG_DIS("bgu r%d, r%d, %d\n", dc->r0, dc->r1,
+LOG_DIS("bgu r%d, r%d, %d\n", dc->r1, dc->r0,
 sign_extend(dc->imm16, 16) * 4);
 
 gen_cond_branch(dc, TCG_COND_GTU);
@@ -314,7 +314,7 @@ static void dec_bgu(DisasContext *dc)
 
 static void dec_bne(DisasContext *dc)
 {
-LOG_DIS("bne r%d, r%d, %d\n", dc->r0, dc->r1,
+LOG_DIS("bne r%d, r%d, %d\n", dc->r1, dc->r0,
 sign_extend(dc->imm16, 16) * 4);
 
 gen_cond_branch(dc, TCG_COND_NE);
@@ -367,7 +367,7 @@ static inline void gen_compare(DisasContext *dc, int cond)
 static void dec_cmpe(DisasContext *dc)
 {
 if (dc->format == OP_FMT_RI) {
-LOG_DIS("cmpei r%d, r%d, %d\n", dc->r0, dc->r1,
+LOG_DIS("cmpei r%d, r%d, %d\n", dc->r1, dc->r0,
 sign_extend(dc->imm16, 16));
 } else {
 LOG_DIS("cmpe r%d, r%d, r%d\n", dc->r2, dc->r0, dc->r1);
@@ -379,7 +379,7 @@ static void dec_cmpe(DisasContext *dc)
 static void dec_cmpg(DisasContext *dc)
 {
 if (dc->format == OP_FMT_RI) {
-LOG_DIS("cmpgi r%d, r%d, %d\n", dc->r0, dc->r1,
+LOG_DIS("cmpgi r%d, r%d, %d\n", dc->r1, dc->r0,
 sign_extend(dc->imm16, 16));
 } else {
 LOG_DIS("cmpg r%d, r%d, r%d\n", dc->r2, dc->r0, dc->r1);
@@ -391,7 +391,7 @@ static void dec_cmpg(DisasContext *dc)
 static void dec_cmpge(DisasContext *dc)
 {
 if (dc->format == OP_FMT_RI) {
-LOG_DIS("cmpgei r%d, r%d, %d\n", dc->r0, dc->r1,
+LOG_DIS("cmpgei r%d, r%d, %d\n", dc->r1, dc->r0,
 sign_extend(dc->imm16, 16));
 } else {
 LOG_DIS("cmpge r%d, r%d, r%d\n", dc->r2, dc->r0, dc->r1);
@@ -403,7 +403,7 @@ static void dec_cmpge(DisasContext *dc)
 static void dec_cmpgeu(DisasContext *dc)
 {
 if (dc->format == OP_FMT_RI) {
-LOG_DIS("cmpgeui r%d, r%d, %d\n", dc->r0, dc->r1,
+LOG_DIS("cmpgeui r%d, r%d, %d\n", dc->r1, dc->r0,
 zero_extend(dc->imm16, 16));
 } else {
 LOG_DIS("cmpgeu r%d, r%d, r%d\n", dc->r2, dc->r0, dc->r1);
@@ -415,7 +415,7 @@ static void dec_cmpgeu(DisasContext *dc)
 static void dec_cmpgu(DisasContext *dc)
 {
 if (dc->format == OP_FMT_RI) {
-LOG_DIS("cmpgui r%d, r%d, %d\n", dc->r0, dc->r1,
+LOG_DIS("cmpgui r%d, r%d, %d\n", dc->r1, dc->r0,
 zero_extend(dc->imm16, 16));
 } else {
 LOG_DIS("cmpgu r%d, r%d, r%d\n", dc->r2, dc->r0, dc->r1);
@@ -427,7 +427,7 @@ static void dec_cmpgu(DisasContext *dc)
 static void dec_cmpne(DisasContext *dc)
 {
 if (dc->format == OP_FMT_RI) {
-LOG_DIS("cmpnei r%d, r%d, %d\n", dc->r0, dc->r1,
+LOG_DIS("cmpnei r%d, r%d, %d\n", dc->r1, dc->r0,
 sign_extend(dc->imm16, 16));
 } else {
 LOG_DIS("cmpne r%d, r%d, r%d\n", dc->r2, dc->r0, dc->r1);
@@ -539,7 +539,7 @@ static void dec_modu(DisasContext *dc)
 static void 

Re: [Qemu-devel] [PATCH 1/2] compiler: add ignore_value() macro

[Felipe Franciosi]

> On 21 Sep 2016, at 19:15, Eric Blake  wrote:
> 
> On 09/21/2016 10:27 AM, Felipe Franciosi wrote:
>> On GCC versions 3.4 and newer, simply using (void) in front of a
>> function that has been declared with WUR will no longer suppress a
>> compilation warning. This commit brings the ignore_value() macro from
>> GNULIB's ignore_value.h, licensed under the terms of LGPLv2+.
>> 
>> See the link below for the original author's comment:
>> https://lists.nongnu.org/archive/html/qemu-devel/2016-09/msg05148.html
>> 
>> Signed-off-by: Felipe Franciosi 
>> ---
>> include/qemu/compiler.h | 8 
>> 1 file changed, 8 insertions(+)
> 
> Reviewed-by: Eric Blake 
> 
> -- 
> Eric Blake   eblake redhat com+1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

Hi Eric,

Now that the header and licensing of compiler.h got amended, can we look at 
this again?

I think the right solution (at least for the moment) is to stick with my series 
to fix the build.
1/2) We get the ignore_value() macro in.
2/2) We ignore the return value of that fwrite().

As I clarified on my last e-mail in this thread (21/sept), there are many other 
calls around the replay code which could potentially fail and are all 
unchecked. We could at least fix the build now and then talk about a separate 
series to address those, if neccesary.

Thanks,
Felipe

Re: [Qemu-devel] [PATCH] target-lm32: fix style issue

[Peter Maydell]

On 12 October 2016 at 17:42, Michael Walle  wrote:
> Am 2016-10-12 18:35, schrieb Peter Maydell:
>>
>> but I noticed while doing the review that our LOG_DIS
>> is wrong for the compare-immediates:
>>
>> LOG_DIS("cmpei r%d, r%d, %d\n", dc->r0, dc->r1,
>> sign_extend(dc->imm16, 16));
>>
>> but the processor reference manual says cmpei's mnemonic
>> should have dc->r1 first and dc->r0 second.
>>
>> (Similarly for the logging for the other immediate compares.)
>
>
> Argh, you're eyes are too good ;) I'll have a look.

If you're looking at lm32 bugs in general, you might also
be interested in the one coverity report for lm32, which
is that in hw/display/milkymist-tmu2.c this code from tmu2_start()

fb_len = 2*s->regs[R_TEXHRES]*s->regs[R_TEXVRES];

is reported as a potential overflow, because the s->regs[]
fields are 32 bits and so the multiplies are done as
32*32 (truncating) but fb_len is 64 bit. Changing the
2 to 2ULL is probably the simplest fix...

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target-lm32: fix style issue

[Michael Walle]

Am 2016-10-12 18:35, schrieb Peter Maydell:

On 12 October 2016 at 17:23, Michael Walle  wrote:
Both branches of the ternary operator have the same expressions. Drop 
the

operator.

This fixes: https://bugs.launchpad.net/qemu/+bug/1414293

Signed-off-by: Michael Walle 
---
 target-lm32/translate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-lm32/translate.c b/target-lm32/translate.c
index 2d8caeb..534c17c 100644
--- a/target-lm32/translate.c
+++ b/target-lm32/translate.c
@@ -343,7 +343,7 @@ static void dec_calli(DisasContext *dc)
 static inline void gen_compare(DisasContext *dc, int cond)
 {
 int rX = (dc->format == OP_FMT_RR) ? dc->r2 : dc->r1;
-int rY = (dc->format == OP_FMT_RR) ? dc->r0 : dc->r0;
+int rY = dc->r0;
 int rZ = (dc->format == OP_FMT_RR) ? dc->r1 : -1;
 int i;


This checks against the processor reference manual, so:

Reviewed-by: Peter Maydell 

but I noticed while doing the review that our LOG_DIS
is wrong for the compare-immediates:

LOG_DIS("cmpei r%d, r%d, %d\n", dc->r0, dc->r1,
sign_extend(dc->imm16, 16));

but the processor reference manual says cmpei's mnemonic
should have dc->r1 first and dc->r0 second.


Hi Peter,

can I drop the DISAS_LM32 macro and just always enable the 
qemu_log_mask(CPU_LOG_TB_IN_ASM)? Looking at other CPUs this is 
sometimes a (debug) compile switch (eg ppc) and sometimes its always 
enabled (tilegx).


-michael

Re: [Qemu-devel] [PATCH 0/4] Allow blockdev-add for SSH

[Ashijeet Acharya]

On Wed, Oct 12, 2016 at 10:10 PM, Kevin Wolf  wrote:
> Am 12.10.2016 um 18:20 hat Ashijeet Acharya geschrieben:
>> On Wed, Oct 12, 2016 at 9:31 PM, Kevin Wolf  wrote:
>> > Am 11.10.2016 um 09:37 hat Ashijeet Acharya geschrieben:
>> >> This series adds blockdev-add support for SSH block driver.
>> >>
>> >> Patch 1 prepares the code for the addition of a new option prefix,
>> >> which is "server.". This is accomplished by adding a
>> >> ssh_has_filename_options_conflict() function which helps to iterate
>> >> over the various options and check for conflict.
>> >>
>> >> Patch 2 first adds InetSocketAddress compatibility to SSH block driver
>> >> and then makes it accept a InetSocketAddress under the "server" option.
>> >> The old options "host" and "port" are supported as legacy options and
>> >> then translated to the respective InetSocketAddress representation.
>> >>
>> >> Patch 3 drops the usage of "host" and "port" outside of
>> >> ssh_has_filename_options_conflict() and
>> >> ssh_process_legacy_socket_options() functions in order to make them
>> >> legacy options completely.
>> >>
>> >> Patch 4 helps to allow blockdev-add support for the SSH block driver
>> >> by making the SSH option available.

>> First thing I made sure if I wasn't breaking anything. Basically I
>> checked if blockdev-add worked with it and then if I am able to remove
>> it with x-blockdev-del.
>
> Did you try out all of the options that we support, including those in
> InetSocketAddress?

No, only 'host' and 'port' from InetSocketAddress which is why I think
the tests were successful in the first place. Using other options seem
to break it. Like you suggested, I think using
qobject_input_visitor_new_autocast() should fix this.

>
>> Also, there were no major changes which could
>> make the patches to break. I don't see tests available for SSH either
>> so there was nothing much I can do.
>> Do you have anything in mind?
>
> Actually, qemu-iotests has ssh support. It's probably not run very
> often, so I'm not sure whether it completely passes on master, but worth
> a try anyway. Check out ./check --help in tests/qemu-iotests, you'll
> probably want something like './check -T -ssh'.
>

Oh, I wasn't aware of that. I will look around now. Thanks!

> The commit message that added ssh support to the tests says:
>
> Note in order to run these tests on ssh, you must be running a local
> ssh daemon, and that daemon must accept loopback connections, and
> ssh-agent has to be set up to allow logins on the local daemon.  In
> other words, the following command should just work without demanding
> any passphrase:
>
>  ssh localhost
>
> Hope this is helpful.

Yes very helpful!!!

Thanks again!

Ashijeet
>
> Kevin

Re: [Qemu-devel] [PATCH 2/4] block/ssh: Add InetSocketAddress and accept it

[Ashijeet Acharya]

On Wed, Oct 12, 2016 at 9:21 PM, Kevin Wolf  wrote:
> Am 11.10.2016 um 09:37 hat Ashijeet Acharya geschrieben:
>> Add InetSocketAddress compatibility to SSH driver.
>>
>> Add a new option "server" to the SSH block driver which then accepts
>> a InetSocketAddress.
>>
>> "host" and "port" are supported as legacy options and are mapped to
>> their InetSocketAddress representation.
>>
>> Signed-off-by: Ashijeet Acharya 
>> ---
>>  block/ssh.c | 87 
>> ++---
>>  1 file changed, 78 insertions(+), 9 deletions(-)
>>
>> diff --git a/block/ssh.c b/block/ssh.c
>> index 75cb7bc..702871a 100644
>> --- a/block/ssh.c
>> +++ b/block/ssh.c
>> @@ -32,8 +32,11 @@
>>  #include "qemu/error-report.h"
>>  #include "qemu/sockets.h"
>>  #include "qemu/uri.h"
>> +#include "qapi-visit.h"
>>  #include "qapi/qmp/qint.h"
>>  #include "qapi/qmp/qstring.h"
>> +#include "qapi/qmp-input-visitor.h"
>> +#include "qapi/qmp-output-visitor.h"
>>
>>  /* DEBUG_SSH=1 enables the DPRINTF (debugging printf) statements in
>>   * this block driver code.
>> @@ -74,6 +77,8 @@ typedef struct BDRVSSHState {
>>   */
>>  LIBSSH2_SFTP_ATTRIBUTES attrs;
>>
>> +InetSocketAddress *inet;
>> +
>>  /* Used to warn if 'flush' is not supported. */
>>  char *hostport;
>>  bool unsafe_flush_warning;
>> @@ -263,7 +268,9 @@ static bool ssh_has_filename_options_conflict(QDict 
>> *options, Error **errp)
>>  !strcmp(qe->key, "port") ||
>>  !strcmp(qe->key, "path") ||
>>  !strcmp(qe->key, "user") ||
>> -!strcmp(qe->key, "host_key_check"))
>> +!strcmp(qe->key, "host_key_check") ||
>> +!strcmp(qe->key, "server") ||
>> +!strncmp(qe->key, "server.", 7))
>
> There is strstart() from cutils.c which looks a bit nicer (you don't
> have to specify the string length then).

Okay, I will use that.

>
>>  {
>>  error_setg(errp, "Option '%s' cannot be used with a file name",
>> qe->key);
>> @@ -555,13 +562,71 @@ static QemuOptsList ssh_runtime_opts = {
>>  },
>>  };
>>
>> +static bool ssh_process_legacy_socket_options(QDict *output_opts,
>> +  QemuOpts *legacy_opts,
>> +  Error **errp)
>> +{
>> +const char *host = qemu_opt_get(legacy_opts, "host");
>> +const char *port = qemu_opt_get(legacy_opts, "port");
>> +
>> +if (!host && port) {
>> +error_setg(errp, "port may not be used without host");
>> +return false;
>> +}
>
> This check is rather pointless if !host causes an error anyway.

Hmm... I will remove it.

>
>> +if (!host) {
>> +error_setg(errp, "No hostname was specified");
>> +return false;
>> +} else {
>> +qdict_put(output_opts, "server.host", qstring_from_str(host));
>> +qdict_put(output_opts, "server.port",
>> +  qstring_from_str(port ?: stringify(22)));
>> +}
>> +
>> +return true;
>> +}
>> +
>> +static InetSocketAddress *ssh_config(BDRVSSHState *s, QDict *options,
>> + Error **errp)
>> +{
>> +InetSocketAddress *inet = NULL;
>> +QDict *addr = NULL;
>> +QObject *crumpled_addr = NULL;
>> +Visitor *iv = NULL;
>> +Error *local_error = NULL;
>> +
>> +qdict_extract_subqdict(options, , "server.");
>> +if (!qdict_size(addr)) {
>> +error_setg(errp, "SSH server address missing");
>> +goto out;
>> +}
>> +
>> +crumpled_addr = qdict_crumple(addr, true, errp);
>> +if (!crumpled_addr) {
>> +goto out;
>> +}
>> +
>> +iv = qmp_input_visitor_new(crumpled_addr, true);
>
> I believe you need qobject_input_visitor_new_autocast() here.
>
> Do integer properties like port work for you without it?

In InetSocketAddress 'port' is of the type 'char*' but now I think
using qobject_input_visitor_new_autocast() will be right since there
are other fields as well.

>
>> +visit_type_InetSocketAddress(iv, NULL, , _error);
>> +if (local_error) {
>> +error_propagate(errp, local_error);
>> +goto out;
>> +}
>> +
>> +out:
>> +QDECREF(addr);
>> +qobject_decref(crumpled_addr);
>> +visit_free(iv);
>> +return inet;
>> +}
>> +
>>  static int connect_to_ssh(BDRVSSHState *s, QDict *options,
>>int ssh_flags, int creat_mode, Error **errp)
>>  {
>>  int r, ret;
>>  QemuOpts *opts = NULL;
>>  Error *local_err = NULL;
>> -const char *host, *user, *path, *host_key_check;
>> +const char *user, *path, *host_key_check;
>>  int port;
>>
>>  opts = qemu_opts_create(_runtime_opts, NULL, 0, _abort);
>> @@ -572,15 +637,11 @@ static int connect_to_ssh(BDRVSSHState *s, QDict 
>> *options,
>>  goto err;
>>  }
>>
>> -host = qemu_opt_get(opts, "host");
>> -if (!host) {
>> +if 

Re: [Qemu-devel] [PATCH v1 2/2] gdbstub: Fix vCont behaviour

[Paolo Bonzini]

On 12/10/2016 15:55, Claudio Imbrenda wrote:
 >>> +for (cx = 0; ccpus && ccpus[cx]; cx++) {
 >>> +cpu_single_step(cpu, 0);
>> > 
>> > This looks suspicious
> why? we set all cpus to single step, since that is the default, and then
> we clear the single-step property from all CPUs that should be restarted
> in normal mode, then we restart all CPUs. Those in single-step will
> indeed only perform one single step, the others will run freely (at
> least until the first single-step CPU stops again).
> 

Yeah, it makes sense.

Paolo

Re: [Qemu-devel] [PATCH] target-lm32: fix style issue

[Michael Walle]

Am 2016-10-12 18:35, schrieb Peter Maydell:

but I noticed while doing the review that our LOG_DIS
is wrong for the compare-immediates:

LOG_DIS("cmpei r%d, r%d, %d\n", dc->r0, dc->r1,
sign_extend(dc->imm16, 16));

but the processor reference manual says cmpei's mnemonic
should have dc->r1 first and dc->r0 second.

(Similarly for the logging for the other immediate compares.)


Argh, you're eyes are too good ;) I'll have a look.

-michael

Re: [Qemu-devel] [PATCH 0/4] Allow blockdev-add for SSH

[Kevin Wolf]

Am 12.10.2016 um 18:20 hat Ashijeet Acharya geschrieben:
> On Wed, Oct 12, 2016 at 9:31 PM, Kevin Wolf  wrote:
> > Am 11.10.2016 um 09:37 hat Ashijeet Acharya geschrieben:
> >> This series adds blockdev-add support for SSH block driver.
> >>
> >> Patch 1 prepares the code for the addition of a new option prefix,
> >> which is "server.". This is accomplished by adding a
> >> ssh_has_filename_options_conflict() function which helps to iterate
> >> over the various options and check for conflict.
> >>
> >> Patch 2 first adds InetSocketAddress compatibility to SSH block driver
> >> and then makes it accept a InetSocketAddress under the "server" option.
> >> The old options "host" and "port" are supported as legacy options and
> >> then translated to the respective InetSocketAddress representation.
> >>
> >> Patch 3 drops the usage of "host" and "port" outside of
> >> ssh_has_filename_options_conflict() and
> >> ssh_process_legacy_socket_options() functions in order to make them
> >> legacy options completely.
> >>
> >> Patch 4 helps to allow blockdev-add support for the SSH block driver
> >> by making the SSH option available.
> >
> > Commented on patch 2, the rest looks good to me at first sight.
> 
> Yes, I am going through that currently and will ask you if I have any queries.
> 
> >
> > Just curious, what kind of testing did you give the series?
> 
> First thing I made sure if I wasn't breaking anything. Basically I
> checked if blockdev-add worked with it and then if I am able to remove
> it with x-blockdev-del.

Did you try out all of the options that we support, including those in
InetSocketAddress?

> Also, there were no major changes which could
> make the patches to break. I don't see tests available for SSH either
> so there was nothing much I can do.
> Do you have anything in mind?

Actually, qemu-iotests has ssh support. It's probably not run very
often, so I'm not sure whether it completely passes on master, but worth
a try anyway. Check out ./check --help in tests/qemu-iotests, you'll
probably want something like './check -T -ssh'.

The commit message that added ssh support to the tests says:

Note in order to run these tests on ssh, you must be running a local
ssh daemon, and that daemon must accept loopback connections, and
ssh-agent has to be set up to allow logins on the local daemon.  In
other words, the following command should just work without demanding
any passphrase:

 ssh localhost

Hope this is helpful.

Kevin

Re: [Qemu-devel] [PATCH] target-lm32: fix style issue

[Peter Maydell]

On 12 October 2016 at 17:23, Michael Walle  wrote:
> Both branches of the ternary operator have the same expressions. Drop the
> operator.
>
> This fixes: https://bugs.launchpad.net/qemu/+bug/1414293
>
> Signed-off-by: Michael Walle 
> ---
>  target-lm32/translate.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target-lm32/translate.c b/target-lm32/translate.c
> index 2d8caeb..534c17c 100644
> --- a/target-lm32/translate.c
> +++ b/target-lm32/translate.c
> @@ -343,7 +343,7 @@ static void dec_calli(DisasContext *dc)
>  static inline void gen_compare(DisasContext *dc, int cond)
>  {
>  int rX = (dc->format == OP_FMT_RR) ? dc->r2 : dc->r1;
> -int rY = (dc->format == OP_FMT_RR) ? dc->r0 : dc->r0;
> +int rY = dc->r0;
>  int rZ = (dc->format == OP_FMT_RR) ? dc->r1 : -1;
>  int i;

This checks against the processor reference manual, so:

Reviewed-by: Peter Maydell 

but I noticed while doing the review that our LOG_DIS
is wrong for the compare-immediates:

LOG_DIS("cmpei r%d, r%d, %d\n", dc->r0, dc->r1,
sign_extend(dc->imm16, 16));

but the processor reference manual says cmpei's mnemonic
should have dc->r1 first and dc->r0 second.

(Similarly for the logging for the other immediate compares.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target-lm32: fix style issue

[Thomas Huth]

On 12.10.2016 18:23, Michael Walle wrote:
> Both branches of the ternary operator have the same expressions. Drop the
> operator.
> 
> This fixes: https://bugs.launchpad.net/qemu/+bug/1414293
> 
> Signed-off-by: Michael Walle 
> ---
>  target-lm32/translate.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target-lm32/translate.c b/target-lm32/translate.c
> index 2d8caeb..534c17c 100644
> --- a/target-lm32/translate.c
> +++ b/target-lm32/translate.c
> @@ -343,7 +343,7 @@ static void dec_calli(DisasContext *dc)
>  static inline void gen_compare(DisasContext *dc, int cond)
>  {
>  int rX = (dc->format == OP_FMT_RR) ? dc->r2 : dc->r1;
> -int rY = (dc->format == OP_FMT_RR) ? dc->r0 : dc->r0;
> +int rY = dc->r0;
>  int rZ = (dc->format == OP_FMT_RR) ? dc->r1 : -1;
>  int i;

Reviewed-by: Thomas Huth 





signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [Bug 1541643] Re: IA32_FEATURE_CONTROL MSR unset for nested virtualization

[man]

** Also affects: archlinux
   Importance: Undecided
   Status: New

** No longer affects: archlinux

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

Title:
  IA32_FEATURE_CONTROL MSR unset for nested virtualization

Status in QEMU:
  New

Bug description:
  I enabled nested virtualization for the kvm_intel module, and passed
  -enable-kvm and -cpu host to qemu. However, the qemu BIOS did not set
  IA32_FEATURE_CONTROL MSR (index 0x3a) to a non-zero value allow VMXON.
  According to the Intel manual Section 23.7 ENABLING AND ENTERING VMX
  OPERATION: "To enable VMX support in a platform, BIOS must set bit 1,
  bit 2, or both (see below), as well as the lock bit."

  I noticed an old mailing list thread on this
  (https://lists.nongnu.org/archive/html/qemu-
  devel/2015-01/msg01372.html), but I wanted to point out that the Intel
  manual (and all the physical hardware I've tested) specifically
  contradicts this response.

  Tested on kernel 4.3.3 and qemu 2.4.1.

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

[Qemu-devel] [PATCH] target-lm32: fix style issue

[Michael Walle]

Both branches of the ternary operator have the same expressions. Drop the
operator.

This fixes: https://bugs.launchpad.net/qemu/+bug/1414293

Signed-off-by: Michael Walle 
---
 target-lm32/translate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-lm32/translate.c b/target-lm32/translate.c
index 2d8caeb..534c17c 100644
--- a/target-lm32/translate.c
+++ b/target-lm32/translate.c
@@ -343,7 +343,7 @@ static void dec_calli(DisasContext *dc)
 static inline void gen_compare(DisasContext *dc, int cond)
 {
 int rX = (dc->format == OP_FMT_RR) ? dc->r2 : dc->r1;
-int rY = (dc->format == OP_FMT_RR) ? dc->r0 : dc->r0;
+int rY = dc->r0;
 int rZ = (dc->format == OP_FMT_RR) ? dc->r1 : -1;
 int i;
 
-- 
2.1.4

Re: [Qemu-devel] [PATCH 0/4] Allow blockdev-add for SSH

[Ashijeet Acharya]

On Wed, Oct 12, 2016 at 9:31 PM, Kevin Wolf  wrote:
> Am 11.10.2016 um 09:37 hat Ashijeet Acharya geschrieben:
>> This series adds blockdev-add support for SSH block driver.
>>
>> Patch 1 prepares the code for the addition of a new option prefix,
>> which is "server.". This is accomplished by adding a
>> ssh_has_filename_options_conflict() function which helps to iterate
>> over the various options and check for conflict.
>>
>> Patch 2 first adds InetSocketAddress compatibility to SSH block driver
>> and then makes it accept a InetSocketAddress under the "server" option.
>> The old options "host" and "port" are supported as legacy options and
>> then translated to the respective InetSocketAddress representation.
>>
>> Patch 3 drops the usage of "host" and "port" outside of
>> ssh_has_filename_options_conflict() and
>> ssh_process_legacy_socket_options() functions in order to make them
>> legacy options completely.
>>
>> Patch 4 helps to allow blockdev-add support for the SSH block driver
>> by making the SSH option available.
>
> Commented on patch 2, the rest looks good to me at first sight.

Yes, I am going through that currently and will ask you if I have any queries.

>
> Just curious, what kind of testing did you give the series?

First thing I made sure if I wasn't breaking anything. Basically I
checked if blockdev-add worked with it and then if I am able to remove
it with x-blockdev-del. Also, there were no major changes which could
make the patches to break. I don't see tests available for SSH either
so there was nothing much I can do.
Do you have anything in mind?

Ashijeet

>
> Kevin

Re: [Qemu-devel] [PATCH v6 15/35] tcg: Add CONFIG_ATOMIC64

[Alex Bennée]

Richard Henderson  writes:

> Allow qemu to build on 32-bit hosts without 64-bit atomic ops.
>
> Even if we only allow 32-bit hosts to multi-thread emulate 32-bit
> guests, we still need some way to handle the 32-bit guest using a
> 64-bit atomic operation.  Do so by dropping back to single-step.
>
> Signed-off-by: Richard Henderson 

Reviewed-by: Alex Bennée 

> ---
>  configure | 33 +
>  cputlb.c  |  4 
>  tcg-runtime.c |  7 +++
>  tcg/tcg-op.c  | 22 ++
>  tcg/tcg-runtime.h | 46 --
>  tcg/tcg.h | 15 ---
>  6 files changed, 114 insertions(+), 13 deletions(-)
>
> diff --git a/configure b/configure
> index 5b38357..0616043 100755
> --- a/configure
> +++ b/configure
> @@ -4501,6 +4501,35 @@ EOF
>fi
>  fi
>
> +#
> +# See if 64-bit atomic operations are supported.
> +# Note that without __atomic builtins, we can only
> +# assume atomic loads/stores max at pointer size.
> +
> +cat > $TMPC << EOF
> +#include 
> +int main(void)
> +{
> +  uint64_t x = 0, y = 0;
> +#ifdef __ATOMIC_RELAXED
> +  y = __atomic_load_8(, 0);
> +  __atomic_store_8(, y, 0);
> +  __atomic_compare_exchange_8(, , x, 0, 0, 0);
> +  __atomic_exchange_8(, y, 0);
> +  __atomic_fetch_add_8(, y, 0);
> +#else
> +  typedef char is_host64[sizeof(void *) >= sizeof(uint64_t) ? 1 : -1];
> +  __sync_lock_test_and_set(, y);
> +  __sync_val_compare_and_swap(, y, 0);
> +  __sync_fetch_and_add(, y);
> +#endif
> +  return 0;
> +}
> +EOF
> +if compile_prog "" "" ; then
> +  atomic64=yes
> +fi
> +
>  
>  # check if getauxval is available.
>
> @@ -5458,6 +5487,10 @@ if test "$atomic128" = "yes" ; then
>echo "CONFIG_ATOMIC128=y" >> $config_host_mak
>  fi
>
> +if test "$atomic64" = "yes" ; then
> +  echo "CONFIG_ATOMIC64=y" >> $config_host_mak
> +fi
> +
>  if test "$getauxval" = "yes" ; then
>echo "CONFIG_GETAUXVAL=y" >> $config_host_mak
>  fi
> diff --git a/cputlb.c b/cputlb.c
> index 845b2a7..cc4da4d 100644
> --- a/cputlb.c
> +++ b/cputlb.c
> @@ -687,8 +687,10 @@ static void *atomic_mmu_lookup(CPUArchState *env, 
> target_ulong addr,
>  #define DATA_SIZE 4
>  #include "atomic_template.h"
>
> +#ifdef CONFIG_ATOMIC64
>  #define DATA_SIZE 8
>  #include "atomic_template.h"
> +#endif
>
>  #ifdef CONFIG_ATOMIC128
>  #define DATA_SIZE 16
> @@ -713,8 +715,10 @@ static void *atomic_mmu_lookup(CPUArchState *env, 
> target_ulong addr,
>  #define DATA_SIZE 4
>  #include "atomic_template.h"
>
> +#ifdef CONFIG_ATOMIC64
>  #define DATA_SIZE 8
>  #include "atomic_template.h"
> +#endif
>
>  /* Code access functions.  */
>
> diff --git a/tcg-runtime.c b/tcg-runtime.c
> index e952153..9327b6f 100644
> --- a/tcg-runtime.c
> +++ b/tcg-runtime.c
> @@ -101,6 +101,11 @@ int64_t HELPER(mulsh_i64)(int64_t arg1, int64_t arg2)
>  return h;
>  }
>
> +void HELPER(exit_atomic)(CPUArchState *env)
> +{
> +cpu_loop_exit_atomic(ENV_GET_CPU(env), GETPC());
> +}
> +
>  #ifndef CONFIG_SOFTMMU
>  /* The softmmu versions of these helpers are in cputlb.c.  */
>
> @@ -130,8 +135,10 @@ static void *atomic_mmu_lookup(CPUArchState *env, 
> target_ulong addr,
>  #define DATA_SIZE 4
>  #include "atomic_template.h"
>
> +#ifdef CONFIG_ATOMIC64
>  #define DATA_SIZE 8
>  #include "atomic_template.h"
> +#endif
>
>  /* The following is only callable from other helpers, and matches up
> with the softmmu version.  */
> diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
> index 65e3663..cdd61d6 100644
> --- a/tcg/tcg-op.c
> +++ b/tcg/tcg-op.c
> @@ -2040,14 +2040,20 @@ typedef void (*gen_atomic_op_i32)(TCGv_i32, TCGv_env, 
> TCGv, TCGv_i32);
>  typedef void (*gen_atomic_op_i64)(TCGv_i64, TCGv_env, TCGv, TCGv_i64);
>  #endif
>
> +#ifdef CONFIG_ATOMIC64
> +# define WITH_ATOMIC64(X) X,
> +#else
> +# define WITH_ATOMIC64(X)
> +#endif
> +
>  static void * const table_cmpxchg[16] = {
>  [MO_8] = gen_helper_atomic_cmpxchgb,
>  [MO_16 | MO_LE] = gen_helper_atomic_cmpxchgw_le,
>  [MO_16 | MO_BE] = gen_helper_atomic_cmpxchgw_be,
>  [MO_32 | MO_LE] = gen_helper_atomic_cmpxchgl_le,
>  [MO_32 | MO_BE] = gen_helper_atomic_cmpxchgl_be,
> -[MO_64 | MO_LE] = gen_helper_atomic_cmpxchgq_le,
> -[MO_64 | MO_BE] = gen_helper_atomic_cmpxchgq_be,
> +WITH_ATOMIC64([MO_64 | MO_LE] = gen_helper_atomic_cmpxchgq_le)
> +WITH_ATOMIC64([MO_64 | MO_BE] = gen_helper_atomic_cmpxchgq_be)
>  };
>
>  void tcg_gen_atomic_cmpxchg_i32(TCGv_i32 retv, TCGv addr, TCGv_i32 cmpv,
> @@ -2117,6 +2123,7 @@ void tcg_gen_atomic_cmpxchg_i64(TCGv_i64 retv, TCGv 
> addr, TCGv_i64 cmpv,
>  }
>  tcg_temp_free_i64(t1);
>  } else if ((memop & MO_SIZE) == MO_64) {
> +#ifdef CONFIG_ATOMIC64
>  gen_atomic_cx_i64 gen;
>
>  gen = table_cmpxchg[memop & (MO_SIZE | MO_BSWAP)];
> @@ -2131,6 +2138,9 @@ void 

Re: [Qemu-devel] [PATCH v6 13/35] tcg: Add atomic helpers

[Alex Bennée]

Richard Henderson  writes:

> Add all of cmpxchg, op_fetch, fetch_op, and xchg.
> Handle both endian-ness, and sizes up to 8.
> Handle expanding non-atomically, when emulating in serial.
>
> Signed-off-by: Richard Henderson 

Reviewed-by: Alex Bennée 

> ---
>  Makefile.objs |   2 +-
>  Makefile.target   |   1 +
>  atomic_template.h | 173 ++
>  cputlb.c  | 112 -
>  include/qemu/atomic.h |  43 ---
>  tcg-runtime.c |  49 ++--
>  tcg/tcg-op.c  | 328 
> ++
>  tcg/tcg-op.h  |  44 +++
>  tcg/tcg-runtime.h |  75 
>  tcg/tcg.h |  53 
>  10 files changed, 848 insertions(+), 32 deletions(-)
>  create mode 100644 atomic_template.h
>
> diff --git a/Makefile.objs b/Makefile.objs
> index 02fb8e7..99d1f6d 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -89,7 +89,7 @@ endif
>
>  ###
>  # Target-independent parts used in system and user emulation
> -common-obj-y += tcg-runtime.o cpus-common.o
> +common-obj-y += cpus-common.o
>  common-obj-y += hw/
>  common-obj-y += qom/
>  common-obj-y += disas/
> diff --git a/Makefile.target b/Makefile.target
> index 9968871..91b6fbd 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -94,6 +94,7 @@ obj-$(CONFIG_TCG_INTERPRETER) += disas/tci.o
>  obj-y += fpu/softfloat.o
>  obj-y += target-$(TARGET_BASE_ARCH)/
>  obj-y += disas.o
> +obj-y += tcg-runtime.o
>  obj-$(call notempty,$(TARGET_XML_FILES)) += gdbstub-xml.o
>  obj-$(call lnot,$(CONFIG_KVM)) += kvm-stub.o
>
> diff --git a/atomic_template.h b/atomic_template.h
> new file mode 100644
> index 000..d2c8a08
> --- /dev/null
> +++ b/atomic_template.h
> @@ -0,0 +1,173 @@
> +/*
> + * Atomic helper templates
> + * Included from tcg-runtime.c and cputlb.c.
> + *
> + * Copyright (c) 2016 Red Hat, Inc
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see 
> .
> + */
> +
> +#if DATA_SIZE == 8
> +# define SUFFIX q
> +# define DATA_TYPE  uint64_t
> +# define BSWAP  bswap64
> +#elif DATA_SIZE == 4
> +# define SUFFIX l
> +# define DATA_TYPE  uint32_t
> +# define BSWAP  bswap32
> +#elif DATA_SIZE == 2
> +# define SUFFIX w
> +# define DATA_TYPE  uint16_t
> +# define BSWAP  bswap16
> +#elif DATA_SIZE == 1
> +# define SUFFIX b
> +# define DATA_TYPE  uint8_t
> +# define BSWAP
> +#else
> +# error unsupported data size
> +#endif
> +
> +#if DATA_SIZE >= 4
> +# define ABI_TYPE  DATA_TYPE
> +#else
> +# define ABI_TYPE  uint32_t
> +#endif
> +
> +#if DATA_SIZE == 1
> +# define END
> +#elif defined(HOST_WORDS_BIGENDIAN)
> +# define END  _be
> +#else
> +# define END  _le
> +#endif
> +
> +ABI_TYPE ATOMIC_NAME(cmpxchg)(CPUArchState *env, target_ulong addr,
> +  ABI_TYPE cmpv, ABI_TYPE newv EXTRA_ARGS)
> +{
> +DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP;
> +return atomic_cmpxchg__nocheck(haddr, cmpv, newv);
> +}
> +
> +ABI_TYPE ATOMIC_NAME(xchg)(CPUArchState *env, target_ulong addr,
> +   ABI_TYPE val EXTRA_ARGS)
> +{
> +DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP;
> +return atomic_xchg__nocheck(haddr, val);
> +}
> +
> +#define GEN_ATOMIC_HELPER(X)\
> +ABI_TYPE ATOMIC_NAME(X)(CPUArchState *env, target_ulong addr,   \
> + ABI_TYPE val EXTRA_ARGS)   \
> +{   \
> +DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP;   \
> +return atomic_##X(haddr, val);  \
> +}   \
> +
> +GEN_ATOMIC_HELPER(fetch_add)
> +GEN_ATOMIC_HELPER(fetch_and)
> +GEN_ATOMIC_HELPER(fetch_or)
> +GEN_ATOMIC_HELPER(fetch_xor)
> +GEN_ATOMIC_HELPER(add_fetch)
> +GEN_ATOMIC_HELPER(and_fetch)
> +GEN_ATOMIC_HELPER(or_fetch)
> +GEN_ATOMIC_HELPER(xor_fetch)
> +
> +#undef GEN_ATOMIC_HELPER
> +#undef END
> +
> +#if DATA_SIZE > 1
> +
> +#ifdef HOST_WORDS_BIGENDIAN
> +# define END  _le
> +#else
> +# define END  _be
> +#endif
> +
> +ABI_TYPE ATOMIC_NAME(cmpxchg)(CPUArchState *env, target_ulong addr,
> +   

Re: [Qemu-devel] [PATCH 1/2] dma-helpers: explicitly pass alignment into dma-helpers

[John Snow]

On 10/12/2016 06:22 AM, Kevin Wolf wrote:

Am 11.10.2016 um 17:47 hat John Snow geschrieben:

On 10/10/2016 03:23 PM, Mark Cave-Ayland wrote:

On 10/10/16 17:34, Eric Blake wrote:


On 10/09/2016 11:43 AM, Mark Cave-Ayland wrote:

The hard-coded default alignment is BDRV_SECTOR_SIZE, however this is not
necessarily the case for all platforms. Use this as the default alignment for
all current callers.

Signed-off-by: Mark Cave-Ayland 
---



@@ -160,8 +161,8 @@ static void dma_blk_cb(void *opaque, int ret)
return;
}

-if (dbs->iov.size & ~BDRV_SECTOR_MASK) {
-qemu_iovec_discard_back(>iov, dbs->iov.size & ~BDRV_SECTOR_MASK);
+if (dbs->iov.size & (dbs->align - 1)) {
+qemu_iovec_discard_back(>iov, dbs->iov.size & (dbs->align - 1));


Would it be any smarter to use osdep.h's QEMU_IS_ALIGNED(dbs->iov.size,
dbs->align) and QEMU_ALIGN_DOWN(dbs->iov.size, dbs->align)?
Semantically it is the same, but the macros make it obvious what the
bit-twiddling is doing.

Unless you think that needs a tweak,
Reviewed-by: Eric Blake 


I can't say I feel too strongly about it since there are plenty of other
examples of this style in the codebase, so I'm happy to go with whatever
John/Paolo are most happy with.


ATB,

Mark.



I can't pretend I am consistent, but when in doubt use the macro.
Not worth a respin IMO, but I think this falls out of my
jurisdiction :)

Acked-by: John Snow 


dma-helpers.c is officially unmaintained, and as the other patch is
clearly IDE, I think the series should go through your tree.

Kevin



Oh! I was under the impression that Paolo had the dma-helpers. My 
mistake. I will test and stage this.


--js

Re: [Qemu-devel] [PATCH] hw/tpm/tpm_passthrough: Simplify if-statements a little bit

[Peter Maydell]

On 12 October 2016 at 16:33, Thomas Huth  wrote:
> The condition  '!A || (A && B)' is equivalent to '!A || B'
>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1464611
> Signed-off-by: Thomas Huth 

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 11/11] iotests: add transactional failure race test

[John Snow]

On 10/12/2016 07:26 AM, Vladimir Sementsov-Ogievskiy wrote:

it is almost a duplication of test_transaction_failure, I think it would
be better to make separate do_test_transaction_failure with parameter
and two wrappers



Yes, sorry -- I missed that for this iteration, but I'll act on it for 
the next iteration.



On 01.10.2016 01:00, John Snow wrote:

Add a regression test for the case found by Vladimir.

Reported-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: John Snow 
---
  tests/qemu-iotests/124 | 91
++
  tests/qemu-iotests/124.out |  4 +-
  2 files changed, 93 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
index 2f0bc24..b8f7dad 100644
--- a/tests/qemu-iotests/124
+++ b/tests/qemu-iotests/124
@@ -512,6 +512,97 @@ class
TestIncrementalBackup(TestIncrementalBackupBase):
  self.check_backups()
+def test_transaction_failure_race(self):
+'''Test: Verify that transactions with jobs that have no data to
+transfer do not cause race conditions in the cancellation of
the entire
+transaction job group.
+'''
+
+# Create a second drive, with pattern:
+drive1 = self.add_node('drive1')
+self.img_create(drive1['file'], drive1['fmt'])
+io_write_patterns(drive1['file'], (('0x14', 0, 512),
+   ('0x5d', '1M', '32k'),
+   ('0xcd', '32M', '124k')))
+
+# Create a blkdebug interface to this img as 'drive1'
+result = self.vm.qmp('blockdev-add', options={
+'node-name': drive1['id'],
+'driver': drive1['fmt'],
+'file': {
+'driver': 'blkdebug',
+'image': {
+'driver': 'file',
+'filename': drive1['file']
+},
+'set-state': [{
+'event': 'flush_to_disk',
+'state': 1,
+'new_state': 2
+}],
+'inject-error': [{
+'event': 'read_aio',
+'errno': 5,
+'state': 2,
+'immediately': False,
+'once': True
+}],
+}
+})
+self.assert_qmp(result, 'return', {})
+
+# Create bitmaps and full backups for both drives
+drive0 = self.drives[0]
+dr0bm0 = self.add_bitmap('bitmap0', drive0)
+dr1bm0 = self.add_bitmap('bitmap0', drive1)
+self.create_anchor_backup(drive0)
+self.create_anchor_backup(drive1)
+self.assert_no_active_block_jobs()
+self.assertFalse(self.vm.get_qmp_events(wait=False))
+
+# Emulate some writes
+self.hmp_io_writes(drive1['id'], (('0xba', 0, 512),
+  ('0xef', '16M', '256k'),
+  ('0x46', '32736k', '64k')))
+
+# Create incremental backup targets
+target0 = self.prepare_backup(dr0bm0)
+target1 = self.prepare_backup(dr1bm0)
+
+# Ask for a new incremental backup per-each drive, expecting
drive1's
+# backup to fail and attempt to cancel the empty drive0 job.
+transaction = [
+transaction_drive_backup(drive0['id'], target0,
sync='incremental',
+ format=drive0['fmt'],
mode='existing',
+ bitmap=dr0bm0.name),
+transaction_drive_backup(drive1['id'], target1,
sync='incremental',
+ format=drive1['fmt'],
mode='existing',
+ bitmap=dr1bm0.name)
+]
+result = self.vm.qmp('transaction', actions=transaction,
+ properties={'completion-mode': 'grouped'} )
+self.assert_qmp(result, 'return', {})
+
+# Observe that drive0's backup is cancelled and drive1
completes with
+# an error.
+self.wait_qmp_backup_cancelled(drive0['id'])
+self.assertFalse(self.wait_qmp_backup(drive1['id']))
+error = self.vm.event_wait('BLOCK_JOB_ERROR')
+self.assert_qmp(error, 'data', {'device': drive1['id'],
+'action': 'report',
+'operation': 'read'})
+self.assertFalse(self.vm.get_qmp_events(wait=False))
+self.assert_no_active_block_jobs()
+
+# Delete drive0's successful target and eliminate our record
of the
+# unsuccessful drive1 target.
+dr0bm0.del_target()
+dr1bm0.del_target()
+
+self.vm.shutdown()
+
+
+
  def test_sync_dirty_bitmap_missing(self):
  self.assert_no_active_block_jobs()
  self.files.append(self.err_img)
diff --git a/tests/qemu-iotests/124.out b/tests/qemu-iotests/124.out

Re: [Qemu-devel] [Qemu-block] [PATCH v14 11/21] qapi: add integer range support for QObjectInputVisitor

[Kevin Wolf]

Am 12.10.2016 um 17:50 hat Markus Armbruster geschrieben:
> "Daniel P. Berrange"  writes:
> 
> > The traditional CLI arg syntax allows two ways to specify
> > integer lists, either one value per key, or a range of
> > values per key. eg the following are identical:
> >
> >   -arg foo=5,foo=6,foo=7
> >   -arg foo=5-7
> >
> > This extends the QObjectInputVisitor so that it is able
> > to parse ranges and turn them into distinct list entries.
> >
> > This means that
> >
> >   -arg foo=5-7
> >
> > is treated as equivalent to
> >
> >   -arg foo.0=5,foo.1=6,foo.2=7
> >
> > Edge case tests are copied from test-opts-visitor to
> > ensure identical behaviour when parsing.
> >
> > Signed-off-by: Daniel P. Berrange 

> > @@ -329,21 +335,87 @@ static void qobject_input_type_int64_autocast(Visitor 
> > *v, const char *name,
> >int64_t *obj, Error **errp)
> >  {
> >  QObjectInputVisitor *qiv = to_qiv(v);
> > -QString *qstr = qobject_to_qstring(qobject_input_get_object(qiv, name,
> > -true));
> > +QString *qstr;
> >  int64_t ret;
> > +const char *end = NULL;
> > +StackObject *tos;
> > +bool inlist = false;
> > +
> > +/* Preferentially generate values from a range, before
> > + * trying to consume another QList element */
> > +tos = QSLIST_FIRST(>stack);
> > +if (tos) {
> > +if ((int64_t)tos->range_val < (int64_t)tos->range_limit) {
> > +*obj = tos->range_val + 1;
> > +tos->range_val++;
> 
> Roundabout way to write
> 
>*obj = tos->range_val++;

*obj = ++tos->range_val, actually.

Kevin

Re: [Qemu-devel] [PATCH 0/4] Allow blockdev-add for SSH

[Kevin Wolf]

Am 11.10.2016 um 09:37 hat Ashijeet Acharya geschrieben:
> This series adds blockdev-add support for SSH block driver.
> 
> Patch 1 prepares the code for the addition of a new option prefix,
> which is "server.". This is accomplished by adding a
> ssh_has_filename_options_conflict() function which helps to iterate
> over the various options and check for conflict.
> 
> Patch 2 first adds InetSocketAddress compatibility to SSH block driver
> and then makes it accept a InetSocketAddress under the "server" option.
> The old options "host" and "port" are supported as legacy options and
> then translated to the respective InetSocketAddress representation.
> 
> Patch 3 drops the usage of "host" and "port" outside of
> ssh_has_filename_options_conflict() and
> ssh_process_legacy_socket_options() functions in order to make them
> legacy options completely.
> 
> Patch 4 helps to allow blockdev-add support for the SSH block driver
> by making the SSH option available.

Commented on patch 2, the rest looks good to me at first sight.

Just curious, what kind of testing did you give the series?

Kevin

Re: [Qemu-devel] [PATCH v8 4/6] docs: Add Documentation for Mediated devices

[Alex Williamson]

On Wed, 12 Oct 2016 20:43:48 +0530
Kirti Wankhede  wrote:

> On 10/12/2016 7:22 AM, Tian, Kevin wrote:
> >> From: Kirti Wankhede [mailto:kwankh...@nvidia.com]
> >> Sent: Wednesday, October 12, 2016 4:45 AM  
>  +* mdev_supported_types:
>  +List of current supported mediated device types and its details are 
>  added
>  +in this directory in following format:
>  +
>  +|- 
>  +|--- Vendor-specific-attributes [optional]
>  +|--- mdev_supported_types
>  +| |--- 
>  +| |   |--- create
>  +| |   |--- name
>  +| |   |--- available_instances
>  +| |   |--- description /class
>  +| |   |--- [devices]
>  +| |--- 
>  +| |   |--- create
>  +| |   |--- name
>  +| |   |--- available_instances
>  +| |   |--- description /class
>  +| |   |--- [devices]
>  +| |--- 
>  +|  |--- create
>  +|  |--- name
>  +|  |--- available_instances
>  +|  |--- description /class
>  +|  |--- [devices]
>  +
>  +[TBD : description or class is yet to be decided. This will change.]  
> >>>
> >>> I thought that in previous discussions we had agreed to drop
> >>> the  concept and use the name as the unique identifier.
> >>> When reporting these types in libvirt we won't want to report
> >>> the type id values - we'll want the name strings to be unique.
> >>>  
> >>
> >> The 'name' might not be unique but type_id will be. For example that Neo
> >> pointed out in earlier discussion, virtual devices can come from two
> >> different physical devices, end user would be presented with what they
> >> had selected but there will be internal implementation differences. In
> >> that case 'type_id' will be unique.
> >>  
> > 
> > Hi, Kirti, my understanding is that Neo agreed to use an unique type
> > string (if you still called it ), and then no need of additional
> > 'name' field which can be put inside 'description' field. See below quote:
> >   
> 
> We had internal discussions about this within NVIDIA and found that
> 'name' might not be unique where as 'type_id' would be unique. I'm
> refering to Neo's mail after that, where Neo do pointed that out.
> 
> https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg07714.html

Everyone not privy to those internal discussions, including me, seems to
think we dropped type_id and that if a vendor does not have a stable
name, they can compose some sort of stable type description based on the
name+id, or even vendor+id, ex. NVIDIA-11.  So please share why we
haven't managed to kill off type_id yet.  No matter what internal
representation each vendor driver has of "type_id" it seems possible
for it to come up with stable string to define a given configuration.
Thanks,

Alex

Re: [Qemu-devel] [PATCH 2/4] block/ssh: Add InetSocketAddress and accept it

[Kevin Wolf]

Am 11.10.2016 um 09:37 hat Ashijeet Acharya geschrieben:
> Add InetSocketAddress compatibility to SSH driver.
> 
> Add a new option "server" to the SSH block driver which then accepts
> a InetSocketAddress.
> 
> "host" and "port" are supported as legacy options and are mapped to
> their InetSocketAddress representation.
> 
> Signed-off-by: Ashijeet Acharya 
> ---
>  block/ssh.c | 87 
> ++---
>  1 file changed, 78 insertions(+), 9 deletions(-)
> 
> diff --git a/block/ssh.c b/block/ssh.c
> index 75cb7bc..702871a 100644
> --- a/block/ssh.c
> +++ b/block/ssh.c
> @@ -32,8 +32,11 @@
>  #include "qemu/error-report.h"
>  #include "qemu/sockets.h"
>  #include "qemu/uri.h"
> +#include "qapi-visit.h"
>  #include "qapi/qmp/qint.h"
>  #include "qapi/qmp/qstring.h"
> +#include "qapi/qmp-input-visitor.h"
> +#include "qapi/qmp-output-visitor.h"
>  
>  /* DEBUG_SSH=1 enables the DPRINTF (debugging printf) statements in
>   * this block driver code.
> @@ -74,6 +77,8 @@ typedef struct BDRVSSHState {
>   */
>  LIBSSH2_SFTP_ATTRIBUTES attrs;
>  
> +InetSocketAddress *inet;
> +
>  /* Used to warn if 'flush' is not supported. */
>  char *hostport;
>  bool unsafe_flush_warning;
> @@ -263,7 +268,9 @@ static bool ssh_has_filename_options_conflict(QDict 
> *options, Error **errp)
>  !strcmp(qe->key, "port") ||
>  !strcmp(qe->key, "path") ||
>  !strcmp(qe->key, "user") ||
> -!strcmp(qe->key, "host_key_check"))
> +!strcmp(qe->key, "host_key_check") ||
> +!strcmp(qe->key, "server") ||
> +!strncmp(qe->key, "server.", 7))

There is strstart() from cutils.c which looks a bit nicer (you don't
have to specify the string length then).

>  {
>  error_setg(errp, "Option '%s' cannot be used with a file name",
> qe->key);
> @@ -555,13 +562,71 @@ static QemuOptsList ssh_runtime_opts = {
>  },
>  };
>  
> +static bool ssh_process_legacy_socket_options(QDict *output_opts,
> +  QemuOpts *legacy_opts,
> +  Error **errp)
> +{
> +const char *host = qemu_opt_get(legacy_opts, "host");
> +const char *port = qemu_opt_get(legacy_opts, "port");
> +
> +if (!host && port) {
> +error_setg(errp, "port may not be used without host");
> +return false;
> +}

This check is rather pointless if !host causes an error anyway.

> +if (!host) {
> +error_setg(errp, "No hostname was specified");
> +return false;
> +} else {
> +qdict_put(output_opts, "server.host", qstring_from_str(host));
> +qdict_put(output_opts, "server.port",
> +  qstring_from_str(port ?: stringify(22)));
> +}
> +
> +return true;
> +}
> +
> +static InetSocketAddress *ssh_config(BDRVSSHState *s, QDict *options,
> + Error **errp)
> +{
> +InetSocketAddress *inet = NULL;
> +QDict *addr = NULL;
> +QObject *crumpled_addr = NULL;
> +Visitor *iv = NULL;
> +Error *local_error = NULL;
> +
> +qdict_extract_subqdict(options, , "server.");
> +if (!qdict_size(addr)) {
> +error_setg(errp, "SSH server address missing");
> +goto out;
> +}
> +
> +crumpled_addr = qdict_crumple(addr, true, errp);
> +if (!crumpled_addr) {
> +goto out;
> +}
> +
> +iv = qmp_input_visitor_new(crumpled_addr, true);

I believe you need qobject_input_visitor_new_autocast() here.

Do integer properties like port work for you without it?

> +visit_type_InetSocketAddress(iv, NULL, , _error);
> +if (local_error) {
> +error_propagate(errp, local_error);
> +goto out;
> +}
> +
> +out:
> +QDECREF(addr);
> +qobject_decref(crumpled_addr);
> +visit_free(iv);
> +return inet;
> +}
> +
>  static int connect_to_ssh(BDRVSSHState *s, QDict *options,
>int ssh_flags, int creat_mode, Error **errp)
>  {
>  int r, ret;
>  QemuOpts *opts = NULL;
>  Error *local_err = NULL;
> -const char *host, *user, *path, *host_key_check;
> +const char *user, *path, *host_key_check;
>  int port;
>  
>  opts = qemu_opts_create(_runtime_opts, NULL, 0, _abort);
> @@ -572,15 +637,11 @@ static int connect_to_ssh(BDRVSSHState *s, QDict 
> *options,
>  goto err;
>  }
>  
> -host = qemu_opt_get(opts, "host");
> -if (!host) {
> +if (!ssh_process_legacy_socket_options(options, opts, errp)) {
>  ret = -EINVAL;
> -error_setg(errp, "No hostname was specified");
>  goto err;
>  }
>  
> -port = qemu_opt_get_number(opts, "port", 22);
> -
>  path = qemu_opt_get(opts, "path");
>  if (!path) {
>  ret = -EINVAL;
> @@ -603,9 +664,16 @@ static int connect_to_ssh(BDRVSSHState *s, QDict 
> *options,
>  

Re: [Qemu-devel] [PATCH v14 11/21] qapi: add integer range support for QObjectInputVisitor

[Markus Armbruster]

"Daniel P. Berrange"  writes:

> The traditional CLI arg syntax allows two ways to specify
> integer lists, either one value per key, or a range of
> values per key. eg the following are identical:
>
>   -arg foo=5,foo=6,foo=7
>   -arg foo=5-7
>
> This extends the QObjectInputVisitor so that it is able
> to parse ranges and turn them into distinct list entries.
>
> This means that
>
>   -arg foo=5-7
>
> is treated as equivalent to
>
>   -arg foo.0=5,foo.1=6,foo.2=7
>
> Edge case tests are copied from test-opts-visitor to
> ensure identical behaviour when parsing.
>
> Signed-off-by: Daniel P. Berrange 
> ---
>  include/qapi/qobject-input-visitor.h |  23 -
>  qapi/qobject-input-visitor.c | 158 ++--
>  tests/test-qobject-input-visitor.c   | 195 
> +--
>  3 files changed, 360 insertions(+), 16 deletions(-)
>
> diff --git a/include/qapi/qobject-input-visitor.h 
> b/include/qapi/qobject-input-visitor.h
> index 94051f3..242b767 100644
> --- a/include/qapi/qobject-input-visitor.h
> +++ b/include/qapi/qobject-input-visitor.h
> @@ -19,6 +19,12 @@
>  
>  typedef struct QObjectInputVisitor QObjectInputVisitor;
>  
> +/* Inclusive upper bound on the size of any flattened range. This is a safety
> + * (= anti-annoyance) measure; wrong ranges should not cause long startup
> + * delays nor exhaust virtual memory.
> + */
> +#define QIV_RANGE_MAX 65536
> +
>  /**
>   * Create a new input visitor that converts @obj to a QAPI object.
>   *
> @@ -71,6 +77,19 @@ Visitor *qobject_input_visitor_new(QObject *obj, bool 
> strict);
>   * The value given determines how many levels of structs are allowed to
>   * be flattened in this way.
>   *
> + * If @permit_int_ranges is true, then when visiting a list of integers,
> + * the integer value strings may encode ranges eg a single element
> + * containing "5-7" is treated as if there were three elements "5", "6",
> + * "7". This should only be used if compatibility is required with the
> + * OptsVisitor which would allow integer ranges. e.g. set this to true
> + * if you have compatibility requirements for
> + *
> + *   -arg val=5-8
> + *
> + * to be treated as equivalent to the preferred syntax:
> + *
> + *   -arg val.0=5,val.1=6,val.2=7,val.3=8
> + *
>   * The visitor always operates in strict mode, requiring all dict keys
>   * to be consumed during visitation. An error will be reported if this
>   * does not happen.
> @@ -80,7 +99,8 @@ Visitor *qobject_input_visitor_new(QObject *obj, bool 
> strict);
>   */
>  Visitor *qobject_input_visitor_new_autocast(QObject *obj,
>  bool autocreate_list,
> -size_t autocreate_struct_levels);
> +size_t autocreate_struct_levels,
> +bool permit_int_ranges);
>  
>  
>  /**
> @@ -98,6 +118,7 @@ Visitor *qobject_input_visitor_new_autocast(QObject *obj,
>  Visitor *qobject_input_visitor_new_opts(const QemuOpts *opts,
>  bool autocreate_list,
>  size_t autocreate_struct_levels,
> +bool permit_int_ranges,
>  Error **errp);
>  
>  #endif
> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> index 1be4865..6b3d0f2 100644
> --- a/qapi/qobject-input-visitor.c
> +++ b/qapi/qobject-input-visitor.c
> @@ -31,6 +31,8 @@ typedef struct StackObject
>  
>  GHashTable *h;   /* If obj is dict: unvisited keys */
>  const QListEntry *entry; /* If obj is list: unvisited tail */
> +uint64_t range_val;
> +uint64_t range_limit;

It's either ugly unions or ugly type casts.  The options visitor picked
ugly unions, you're picking ugly type casts.  Matter of taste, as long
as the type casts are all kosher.

>  
>  QSLIST_ENTRY(StackObject) node;
>  } StackObject;
> @@ -60,6 +62,10 @@ struct QObjectInputVisitor
>   * consider auto-creating a struct containing
>   * remaining unvisited items */
>  size_t autocreate_struct_levels;
> +
> +/* Whether int lists can have single values representing
> + * ranges of values */
> +bool permit_int_ranges;
>  };
>  
>  static QObjectInputVisitor *to_qiv(Visitor *v)
> @@ -282,7 +288,7 @@ static GenericList *qobject_input_next_list(Visitor *v, 
> GenericList *tail,
>  QObjectInputVisitor *qiv = to_qiv(v);
>  StackObject *so = QSLIST_FIRST(>stack);
>  
> -if (!so->entry) {
> +if ((so->range_val == so->range_limit) && !so->entry) {
>  return NULL;
>  }
>  tail->next = g_malloc0(size);
> @@ -329,21 +335,87 @@ static void qobject_input_type_int64_autocast(Visitor 
> *v, const char *name,
>int64_t *obj, Error **errp)
>  {
>  QObjectInputVisitor *qiv = 

Re: [Qemu-devel] [kvm-unit-tests PATCHv6 2/3] arm: pmu: Check cycle count increases

[Wei Huang]

On 10/11/2016 01:40 PM, Christopher Covington wrote:
> Ensure that reads of the PMCCNTR_EL0 are monotonically increasing,
> even for the smallest delta of two subsequent reads.
> 
> Signed-off-by: Christopher Covington 
> Reviewed-by: Andrew Jones 
> ---
>  arm/pmu.c | 60 
>  1 file changed, 60 insertions(+)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index 42d0ee1..4334de4 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -14,6 +14,8 @@
>   */
>  #include "libcflat.h"
>  
> +#define NR_SAMPLES 10
> +
>  #if defined(__arm__)
>  static inline uint32_t get_pmcr(void)
>  {
> @@ -22,6 +24,25 @@ static inline uint32_t get_pmcr(void)
>   asm volatile("mrc p15, 0, %0, c9, c12, 0" : "=r" (ret));
>   return ret;
>  }
> +
> +static inline void set_pmcr(uint32_t pmcr)
> +{
> + asm volatile("mcr p15, 0, %0, c9, c12, 0" : : "r" (pmcr));
> +}
> +
> +/*
> + * While PMCCNTR can be accessed as a 64 bit coprocessor register, returning 
> 64
> + * bits doesn't seem worth the trouble when differential usage of the result 
> is
> + * expected (with differences that can easily fit in 32 bits). So just return
> + * the lower 32 bits of the cycle count in AArch32.
> + */
> +static inline unsigned long get_pmccntr(void)
> +{
> + unsigned long cycles;
> +
> + asm volatile("mrc p15, 0, %0, c9, c13, 0" : "=r" (cycles));
> + return cycles;
> +}
>  #elif defined(__aarch64__)
>  static inline uint32_t get_pmcr(void)
>  {
> @@ -30,6 +51,19 @@ static inline uint32_t get_pmcr(void)
>   asm volatile("mrs %0, pmcr_el0" : "=r" (ret));
>   return ret;
>  }
> +
> +static inline void set_pmcr(uint32_t pmcr)
> +{
> + asm volatile("msr pmcr_el0, %0" : : "r" (pmcr));
> +}
> +
> +static inline unsigned long get_pmccntr(void)
> +{
> + unsigned long cycles;
> +
> + asm volatile("mrs %0, pmccntr_el0" : "=r" (cycles));
> + return cycles;
> +}
>  #endif
>  
>  struct pmu_data {
> @@ -72,11 +106,37 @@ static bool check_pmcr(void)
>   return pmu.implementer != 0;
>  }
>  
> +/*
> + * Ensure that the cycle counter progresses between back-to-back reads.
> + */
> +static bool check_cycles_increase(void)
> +{
> + struct pmu_data pmu = {0};

Compilation error on my machine:

arm/pmu.c: In function ‘check_cycles_increase’:
arm/pmu.c:148:9: error: missing braces around initializer
[-Werror=missing-braces]
  struct pmu_data pmu = {0};

Same for Patch 3.


> +
> + pmu.enable = 1;
> + set_pmcr(pmu.pmcr_el0);
> +
> + for (int i = 0; i < NR_SAMPLES; i++) {
> + unsigned long a, b;
> +
> + a = get_pmccntr();
> + b = get_pmccntr();
> +
> + if (a >= b) {
> + printf("Read %ld then %ld.\n", a, b);
> + return false;
> + }
> + }
> +
> + return true;
> +}
> +
>  int main(void)
>  {
>   report_prefix_push("pmu");
>  
>   report("Control register", check_pmcr());
> + report("Monotonically increasing cycle count", check_cycles_increase());
>  
>   return report_summary();
>  }
> 

[Qemu-devel] error reporting in functions

[Vladimir Sementsov-Ogievskiy]

HI all!

My questions is: what are general recommendations in Qemu for return 
code, if we have Error **errp?
What should I prefer: errp, duplicated by int return code, or void 
functions with errp?


void + errp seems good, just to not duplicate things. But it has a 
disadvantage of necessity of "local_err" and "error_propagate" in caller 
function, if its behaviour depends on callee function success...


--
Best regards,
Vladimir

[Qemu-devel] [PATCH] hw/tpm/tpm_passthrough: Simplify if-statements a little bit

[Thomas Huth]

The condition  '!A || (A && B)' is equivalent to '!A || B'

Buglink: https://bugs.launchpad.net/qemu/+bug/1464611
Signed-off-by: Thomas Huth 
---
 hw/tpm/tpm_passthrough.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/tpm/tpm_passthrough.c b/hw/tpm/tpm_passthrough.c
index e88c0d2..9234eb3 100644
--- a/hw/tpm/tpm_passthrough.c
+++ b/hw/tpm/tpm_passthrough.c
@@ -165,8 +165,7 @@ static int tpm_passthrough_unix_tx_bufs(TPMPassthruState 
*tpm_pt,
 
 ret = tpm_passthrough_unix_write(tpm_pt->tpm_fd, in, in_len);
 if (ret != in_len) {
-if (!tpm_pt->tpm_op_canceled ||
-(tpm_pt->tpm_op_canceled && errno != ECANCELED)) {
+if (!tpm_pt->tpm_op_canceled || errno != ECANCELED) {
 error_report("tpm_passthrough: error while transmitting data "
  "to TPM: %s (%i)",
  strerror(errno), errno);
@@ -178,8 +177,7 @@ static int tpm_passthrough_unix_tx_bufs(TPMPassthruState 
*tpm_pt,
 
 ret = tpm_passthrough_unix_read(tpm_pt->tpm_fd, out, out_len);
 if (ret < 0) {
-if (!tpm_pt->tpm_op_canceled ||
-(tpm_pt->tpm_op_canceled && errno != ECANCELED)) {
+if (!tpm_pt->tpm_op_canceled || errno != ECANCELED) {
 error_report("tpm_passthrough: error while reading data from "
  "TPM: %s (%i)",
  strerror(errno), errno);
-- 
1.8.3.1

Re: [Qemu-devel] [kvm-unit-tests PATCHv6 1/3] arm: Add PMU test

[Wei Huang]

On 10/11/2016 01:40 PM, Christopher Covington wrote:
> Beginning with a simple sanity check of the control register, add
> a unit test for the ARM Performance Monitors Unit (PMU). As of
> October 2016, whether KVM mode has a PMU at all is a tricky
> question of which QEMU / mach-virt version is used. So only enable
> the TCG mode tests for now.

Given that we have KVM-based vPMU enabled in QEMU, it is very desirable
for these unit testing cases to work on KVM. Right now both
check_cycles_increase() and check_cpi() fail under KVM. I will help
debug it.

> 
> Signed-off-by: Christopher Covington 
> Reviewed-by: Andrew Jones 
> ---
>  arm/Makefile.common |  3 +-
>  arm/pmu.c   | 82 
> +
>  arm/unittests.cfg   | 14 +
>  3 files changed, 98 insertions(+), 1 deletion(-)
>  create mode 100644 arm/pmu.c
> 
> diff --git a/arm/Makefile.common b/arm/Makefile.common
> index ccb554d..f98f422 100644
> --- a/arm/Makefile.common
> +++ b/arm/Makefile.common
> @@ -11,7 +11,8 @@ endif
>  
>  tests-common = \
>   $(TEST_DIR)/selftest.flat \
> - $(TEST_DIR)/spinlock-test.flat
> + $(TEST_DIR)/spinlock-test.flat \
> + $(TEST_DIR)/pmu.flat
>  
>  all: test_cases
>  
> diff --git a/arm/pmu.c b/arm/pmu.c
> new file mode 100644
> index 000..42d0ee1
> --- /dev/null
> +++ b/arm/pmu.c
> @@ -0,0 +1,82 @@
> +/*
> + * Test the ARM Performance Monitors Unit (PMU).
> + *
> + * Copyright 2015 The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU Lesser General Public License version 2.1 and
> + * only version 2.1 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but 
> WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public 
> License
> + * for more details.
> + */
> +#include "libcflat.h"
> +
> +#if defined(__arm__)
> +static inline uint32_t get_pmcr(void)
> +{
> + uint32_t ret;
> +
> + asm volatile("mrc p15, 0, %0, c9, c12, 0" : "=r" (ret));
> + return ret;
> +}
> +#elif defined(__aarch64__)
> +static inline uint32_t get_pmcr(void)
> +{
> + uint32_t ret;
> +
> + asm volatile("mrs %0, pmcr_el0" : "=r" (ret));
> + return ret;
> +}
> +#endif
> +
> +struct pmu_data {
> + union {
> + uint32_t pmcr_el0;
> + struct {
> + uint32_t enable:1;
> + uint32_t event_counter_reset:1;
> + uint32_t cycle_counter_reset:1;
> + uint32_t cycle_counter_clock_divider:1;
> + uint32_t event_counter_export:1;
> + uint32_t cycle_counter_disable_when_prohibited:1;
> + uint32_t cycle_counter_long:1;
> + uint32_t reserved:4;
> + uint32_t counters:5;
> + uint32_t identification_code:8;
> + uint32_t implementer:8;
> + };
> + };
> +};
> +
> +/*
> + * As a simple sanity check on the PMCR_EL0, ensure the implementer field 
> isn't
> + * null. Also print out a couple other interesting fields for diagnostic
> + * purposes. For example, as of fall 2015, QEMU TCG mode doesn't implement
> + * event counters and therefore reports zero event counters, but hopefully
> + * support for at least the instructions event will be added in the future 
> and
> + * the reported number of event counters will become nonzero.
> + */
> +static bool check_pmcr(void)
> +{
> + struct pmu_data pmu;
> +
> + pmu.pmcr_el0 = get_pmcr();
> +
> + printf("PMU implementer: %c\n", pmu.implementer);
> + printf("Identification code: 0x%x\n", pmu.identification_code);
> + printf("Event counters:  %d\n", pmu.counters);
> +
> + return pmu.implementer != 0;
> +}
> +
> +int main(void)
> +{
> + report_prefix_push("pmu");
> +
> + report("Control register", check_pmcr());
> +
> + return report_summary();
> +}
> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> index ffd12e5..edaed4a 100644
> --- a/arm/unittests.cfg
> +++ b/arm/unittests.cfg
> @@ -51,3 +51,17 @@ file = selftest.flat
>  smp = $MAX_SMP
>  extra_params = -append 'smp'
>  groups = selftest
> +
> +# Test PMU support with -icount IPC=1
> +[pmu-icount-1]
> +file = pmu.flat
> +extra_params = -icount 0 -append '1'
> +groups = pmu
> +accel = tcg
> +
> +# Test PMU support with -icount IPC=256
> +[pmu-icount-256]
> +file = pmu.flat
> +extra_params = -icount 8 -append '256'
> +groups = pmu
> +accel = tcg
> 

Re: [Qemu-devel] Async savevm using userfaultfd(2)

[Denis V. Lunev]

On 10/12/2016 05:04 PM, Stefan Hajnoczi wrote:
> John and I recently discussed asynchronous savevm and I wanted to post
> the ideas so they aren't forgotten.  (We're not actively working on this
> feature.)
>
> Asynchronous savevm has the same effect as the 'savevm' monitor command:
> it saves RAM, device state, and a snapshot of all disks at the point in
> time the command was issued.
>
> The current 'savevm' monitor command is synchronous so the guest and
> QEMU monitor are blocked while the operation runs (it can take a
> while!).  Asynchronous savevm has the advantage of allowing the guest
> and QEMU monitor to continue while the operation is running.
>
> This sounds similar to live migration to file but remember that live
> migration's consistency point is when the guest is paused at the end of
> the iteration phase.  The user has no control over *when* live migration
> captures the guest state.  Therefore it's not a useful command for
> taking snapshots of guest state at a specific point in time - we need
> asynchronous savevm for that.
>
> Async savevm must copy-on-write guest RAM so the guest can continue
> writing to memory while the snapshot is being saved.  Rik van Riel
> suggested using userfaultfd(2) to do this on Linux.
>
> Unlike post-copy live migration, we want to track memory writes (instead
> of missing page faults).  The userfaultfd(2) flag
> UFFDIO_REGISTER_MODE_WP provides these semantics.  Unfortunately I think
> UFFDIO_REGISTER_MODE_WP is not yet implemented?
>
> Once UFFDIO_REGISTER_MODE_WP is available QEMU can catch writes to guest
> RAM and copy the original pages to a buffer.  If memory is dirtied too
> quickly then it's necessary to throttle the guest or fail the savevm
> operation.
>
> Perhaps this approach can be prototyped with mprotect and a SIGSEGV
> handler if anyone wants to get async savevm going.  I don't know if
> there are any disadvantages to mprotecting guest RAM that the kvm kernel
> module is using.  Hopefully in-kernel devices and vhost will continue to
> work.
>
> Stefan
good idea!

Re: [Qemu-devel] MTTCG memory ordering

[Pranith Kumar]

Hi Stefan,

Stefan Hajnoczi writes:

> Hi Pranith,
> I was curious about the status of your MTTCG GSoC work:
>
> I saw your fence series which implements the noop memory barrier/fence
> instructions on various architectures, but I wasn't sure if that also
> covers the case where a strong target is emulated on a weak host.
>

No, this work is still pending. The current implementation only supports weak
on strong (the simplest case) by emitting barriers explicitly.

> Did you make TCG automatically emit barriers so stronger targets (x86)
> run correctly on weaker targets (ARM)?

We did consider doing this by emitting barriers implicitly for each memory
instruction of a strong target but decided that it would be too costly. There
is, AFAIK, no trivial solution to avoiding this overhead as of now.

I will start working on this next step soon, once I finish the tcg test setup.

Thanks,
-- 
Pranith

Re: [Qemu-devel] [PATCH V2] docs: add PCIe devices placement guidelines

[Laszlo Ersek]

Marcel,

On 10/11/16 15:45, Marcel Apfelbaum wrote:
> Proposes best practices on how to use PCI Express/PCI device
> in PCI Express based machines and explain the reasoning behind them.
> 
> Signed-off-by: Marcel Apfelbaum 
> ---
> 
> Hi,
> 
> RFC->v2:
>  - Addressed a lot of comments from the reviewers (many thanks to all, 
> especially to Laszlo)
> 
> Since the RFC mail-thread was relatively long and already
> has passed a lot of time from the RFC, I post this version
> even if is very possible that I left some of the comments out,
> my apologies if so.
> 
> I will go over the comments again, in the meantime please
> feel free to comment on this version, even if on something
> you've already pointed out.
> 
> It may take a day or two until I'll be able to respond, but I
> will do my best to address all comments.
> 
> Thanks,
> Marcel
> 
> 
>  docs/pcie.txt | 273 
> ++
>  1 file changed, 273 insertions(+)
>  create mode 100644 docs/pcie.txt

Your patch doesn't seem to have reached qemu-devel. I got one copy from
you directly, and no copy reflected by the qemu-devel list server. I
also checked the mailing list archive:
- searched for the subject with google -- only the RFC version was found,
- checked mail-archive.com by message-id
- checked the primary archive for October
(http://lists.nongnu.org/archive/html/qemu-devel/2016-10/threads.html)
-- I found only messages in the RFC thread.

So, before I start reading this version and commenting on it, can you
please repost version 2, and verify that the list reflects it?

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH] hw/block/nvme: Simplify if-statements a little bit

[Peter Maydell]

On 12 October 2016 at 16:18, Thomas Huth  wrote:
> The condition  '!A || (A && B)' is equivalent to '!A || B'.
>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1464611
> Signed-off-by: Thomas Huth 
> ---
>  hw/block/nvme.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index cef3bb4..53d9d2e 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -373,7 +373,7 @@ static uint16_t nvme_create_sq(NvmeCtrl *n, NvmeCmd *cmd)
>  if (!cqid || nvme_check_cqid(n, cqid)) {
>  return NVME_INVALID_CQID | NVME_DNR;
>  }
> -if (!sqid || (sqid && !nvme_check_sqid(n, sqid))) {
> +if (!sqid || !nvme_check_sqid(n, sqid)) {
>  return NVME_INVALID_QID | NVME_DNR;
>  }
>  if (!qsize || qsize > NVME_CAP_MQES(n->bar.cap)) {
> @@ -447,7 +447,7 @@ static uint16_t nvme_create_cq(NvmeCtrl *n, NvmeCmd *cmd)
>  uint16_t qflags = le16_to_cpu(c->cq_flags);
>  uint64_t prp1 = le64_to_cpu(c->prp1);
>
> -if (!cqid || (cqid && !nvme_check_cqid(n, cqid))) {
> +if (!cqid || !nvme_check_cqid(n, cqid)) {
>  return NVME_INVALID_CQID | NVME_DNR;
>  }
>  if (!qsize || qsize > NVME_CAP_MQES(n->bar.cap)) {

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [Qemu-devel] [PATCH v14 07/21] qapi: permit scalar type conversions in QObjectInputVisitor

[Markus Armbruster]

Markus Armbruster  writes:

> Markus Armbruster  writes:
>
>> Markus Armbruster  writes:
>>
>>> "Daniel P. Berrange"  writes:
>>>
 Currently the QObjectInputVisitor assumes that all scalar
 values are directly represented as the final types declared
 by the thing being visited. ie it assumes an 'int' is using
>>>
>>> i.e.
>>>
 QInt, and a 'bool' is using QBool, etc.  This is good when
 QObjectInputVisitor is fed a QObject that came from a JSON
 document on the QMP monitor, as it will strictly validate
 correctness.

 To allow QObjectInputVisitor to be reused for visiting
 a QObject originating from QemuOpts, an alternative mode
 is needed where all the scalars types are represented as
 QString and converted on the fly to the final desired
 type.

 Reviewed-by: Kevin Wolf 
 Reviewed-by: Marc-André Lureau 
 Signed-off-by: Daniel P. Berrange 
>> [...]
 diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
 index 5ff3db3..cf41df6 100644
 --- a/qapi/qobject-input-visitor.c
 +++ b/qapi/qobject-input-visitor.c
 @@ -20,6 +20,7 @@
  #include "qemu-common.h"
  #include "qapi/qmp/types.h"
  #include "qapi/qmp/qerror.h"
 +#include "qemu/cutils.h"
  
  #define QIV_STACK_SIZE 1024
  
 @@ -263,6 +264,28 @@ static void qobject_input_type_int64(Visitor *v, 
 const char *name, int64_t *obj,
  *obj = qint_get_int(qint);
  }
  
 +
 +static void qobject_input_type_int64_autocast(Visitor *v, const char 
 *name,
 +  int64_t *obj, Error **errp)
 +{
 +QObjectInputVisitor *qiv = to_qiv(v);
 +QString *qstr = qobject_to_qstring(qobject_input_get_object(qiv, name,
 +true));
>>>
>>> Needs a rebase for commit 1382d4a.  Same elsewhere.
>>>
 +int64_t ret;
 +
 +if (!qstr || !qstr->string) {
>>>
>>> I don't think !qstr->string can happen.  Same elsewhere.
>>>
 +error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : 
 "null",
 +   "string");
 +return;
 +}
>>>
>>> So far, this is basically qobject_input_type_str() less the g_strdup().
>>> Worth factoring out?
>>>
>>> Now we're entering out parsing swamp:
>>>
 +
 +if (qemu_strtoll(qstr->string, NULL, 0, ) < 0) {
 +error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name, "a number");
>>
>> "integer", actually.
>
> Wait!  You're using QERR_INVALID_PARAMETER_VALUE here, so it's "an
> integer".
>
> To be pedantically correct, we'd have to complain about the type when
> qemu_strtoll() fails to parse, and about the value when it parses okay,
> but the value is out of range.

Nevermind, I got confused.  The type is actually always string here, so
your use of QERR_INVALID_PARAMETER_VALUE is appropriate.

[...]

[Qemu-devel] [PATCH] hw/block/nvme: Simplify if-statements a little bit

[Thomas Huth]

The condition  '!A || (A && B)' is equivalent to '!A || B'.

Buglink: https://bugs.launchpad.net/qemu/+bug/1464611
Signed-off-by: Thomas Huth 
---
 hw/block/nvme.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index cef3bb4..53d9d2e 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -373,7 +373,7 @@ static uint16_t nvme_create_sq(NvmeCtrl *n, NvmeCmd *cmd)
 if (!cqid || nvme_check_cqid(n, cqid)) {
 return NVME_INVALID_CQID | NVME_DNR;
 }
-if (!sqid || (sqid && !nvme_check_sqid(n, sqid))) {
+if (!sqid || !nvme_check_sqid(n, sqid)) {
 return NVME_INVALID_QID | NVME_DNR;
 }
 if (!qsize || qsize > NVME_CAP_MQES(n->bar.cap)) {
@@ -447,7 +447,7 @@ static uint16_t nvme_create_cq(NvmeCtrl *n, NvmeCmd *cmd)
 uint16_t qflags = le16_to_cpu(c->cq_flags);
 uint64_t prp1 = le64_to_cpu(c->prp1);
 
-if (!cqid || (cqid && !nvme_check_cqid(n, cqid))) {
+if (!cqid || !nvme_check_cqid(n, cqid)) {
 return NVME_INVALID_CQID | NVME_DNR;
 }
 if (!qsize || qsize > NVME_CAP_MQES(n->bar.cap)) {
-- 
1.8.3.1

Re: [Qemu-devel] MTTCG memory ordering

[Laszlo Ersek]

On 10/12/16 10:58, Stefan Hajnoczi wrote:
> Hi Pranith,
> I was curious about the status of your MTTCG GSoC work:
> 
> I saw your fence series which implements the noop memory barrier/fence
> instructions on various architectures, but I wasn't sure if that also
> covers the case where a strong target is emulated on a weak host.
> 
> Did you make TCG automatically emit barriers so stronger targets (x86)
> run correctly on weaker targets (ARM)?

(Testing that might run into other problems:
)

Re: [Qemu-devel] [PATCH] socket: add atomic QEMU_SOCK_NONBLOCK flag

[Stefan Hajnoczi]

On Fri, Oct 07, 2016 at 10:55:55AM -0500, Eric Blake wrote:
> On 10/07/2016 08:54 AM, Stefan Hajnoczi wrote:
> > The socket(2) and accept(2) syscalls have been extended to take flags
> > that affect the socket atomically at creation time.  This not only
> > avoids the overhead of additional system calls but also closes the race
> > condition with forking threads.
> > 
> > This patch adds support for the SOCK_NONBLOCK flag.  QEMU already
> > implements the SOCK_CLOEXEC flag.
> 
> Atomic where supported by the OS, racy fallback on older systems, but
> not the end of the world (and our already-existing fallback is already
> racy, where the SOCK_CLOEXEC race is more likely to affect a
> multithreaded forking app, while SOCK_NONBLOCK is just there for
> convenience).
> 
> > 
> > All qemu_socket() and qemu_accept() callers are updated to pass the new
> > flags argument.  Callers that later use qemu_set_nonblock() are
> > refactored as follows:
> > 
> >   fd = qemu_socket(...) or qemu_accept(...);
> >   ...
> >   qemu_set_nonblock(fd);
> > 
> > Becomes:
> > 
> >   fd = qemu_socket(..., QEMU_SOCK_NONBLOCK) or
> >qemu_accept(..., QEMU_SOCK_NONBLOCK);
> > 
> > For full details on SOCK_NONBLOCK in the POSIX spec see:
> > http://austingroupbugs.net/view.php?id=411
> 
> /me that looks strangely familiar... :)
> 
> > 
> > Note that slirp code violates the coding style with a mix of tabs and
> > space indentation.  This patch passes checkpatch.pl but the diff may
> > appear odd due to the mixed indentation in slirp code.
> > 
> > Suggested-by: Eric Blake 
> > Signed-off-by: Stefan Hajnoczi 
> > ---
> 
> > +++ b/include/qemu/sockets.h
> > @@ -11,9 +11,14 @@ int inet_aton(const char *cp, struct in_addr *ia);
> >  
> >  #include "qapi-types.h"
> >  
> > +typedef enum {
> > +QEMU_SOCK_NONBLOCK = 1,
> > +} QemuSockFlags;
> 
> Good, since we can't rely on SOCK_NONBLOCK being defined everywhere yet.
> 
> > --- a/slirp/misc.c
> > +++ b/slirp/misc.c
> 
> > @@ -184,13 +185,13 @@ fork_exec(struct socket *so, const char *ex, int 
> > do_pty)
> >   * of connect() fail in the child process
> >   */
> >  do {
> > -so->s = accept(s, (struct sockaddr *), );
> > +so->s = qemu_accept(s, (struct sockaddr *), 
> > ,
> > +QEMU_SOCK_NONBLOCK);
> 
> Silent bug fix here and elsewhere that we now set CLOEXEC where we
> previously didn't.  Probably worth mentioning in the commit message that
> you fixed a couple of places that used accept() instead of the proper
> qemu_accept().

Ok

> > @@ -288,12 +300,17 @@ int qemu_socket(int domain, int type, int protocol)
> >  /*
> >   * Accept a connection and set FD_CLOEXEC
> >   */
> > -int qemu_accept(int s, struct sockaddr *addr, socklen_t *addrlen)
> > +int qemu_accept(int s, struct sockaddr *addr, socklen_t *addrlen,
> > +QemuSockFlags flags)
> >  {
> >  int ret;
> >  
> >  #ifdef CONFIG_ACCEPT4
> > -ret = accept4(s, addr, addrlen, SOCK_CLOEXEC);
> > +int accept_flags = SOCK_CLOEXEC;
> > +if (flags & QEMU_SOCK_NONBLOCK) {
> > +accept_flags |= SOCK_NONBLOCK;
> > +}
> 
> You're also (implicitly) assuming that CONFIG_ACCEPT4 implies both
> SOCK_CLOEXEC and SOCK_NONBLOCK; again likely to be true.

The code already assumed CONFIG_ACCEPT4 implies SOCK_CLOEXEC so I
checked linux.git and found it is true in mainline Linux.  Of course
distros could do a crazy backport where only some of the feature is
backported...

> > @@ -317,18 +318,20 @@ static int inet_connect_addr(struct addrinfo *addr, 
> > bool *in_progress,
> >   ConnectState *connect_state, Error **errp)
> >  {
> >  int sock, rc;
> > +QemuSockFlags flags = 0;
> >  
> >  *in_progress = false;
> >  
> > -sock = qemu_socket(addr->ai_family, addr->ai_socktype, 
> > addr->ai_protocol);
> > -if (sock < 0) {
> > -error_setg_errno(errp, errno, "Failed to create socket");
> > -return -1;
> > -}
> > -socket_set_fast_reuse(sock);
> >  if (connect_state != NULL) {
> > -qemu_set_nonblock(sock);
> > +flags = QEMU_SOCK_NONBLOCK;
> 
> Use |= here? ...

Sure.


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v8 4/6] docs: Add Documentation for Mediated devices

[Kirti Wankhede]

On 10/12/2016 7:22 AM, Tian, Kevin wrote:
>> From: Kirti Wankhede [mailto:kwankh...@nvidia.com]
>> Sent: Wednesday, October 12, 2016 4:45 AM
 +* mdev_supported_types:
 +List of current supported mediated device types and its details are 
 added
 +in this directory in following format:
 +
 +|- 
 +|--- Vendor-specific-attributes [optional]
 +|--- mdev_supported_types
 +| |--- 
 +| |   |--- create
 +| |   |--- name
 +| |   |--- available_instances
 +| |   |--- description /class
 +| |   |--- [devices]
 +| |--- 
 +| |   |--- create
 +| |   |--- name
 +| |   |--- available_instances
 +| |   |--- description /class
 +| |   |--- [devices]
 +| |--- 
 +|  |--- create
 +|  |--- name
 +|  |--- available_instances
 +|  |--- description /class
 +|  |--- [devices]
 +
 +[TBD : description or class is yet to be decided. This will change.]
>>>
>>> I thought that in previous discussions we had agreed to drop
>>> the  concept and use the name as the unique identifier.
>>> When reporting these types in libvirt we won't want to report
>>> the type id values - we'll want the name strings to be unique.
>>>
>>
>> The 'name' might not be unique but type_id will be. For example that Neo
>> pointed out in earlier discussion, virtual devices can come from two
>> different physical devices, end user would be presented with what they
>> had selected but there will be internal implementation differences. In
>> that case 'type_id' will be unique.
>>
> 
> Hi, Kirti, my understanding is that Neo agreed to use an unique type
> string (if you still called it ), and then no need of additional
> 'name' field which can be put inside 'description' field. See below quote:
> 

We had internal discussions about this within NVIDIA and found that
'name' might not be unique where as 'type_id' would be unique. I'm
refering to Neo's mail after that, where Neo do pointed that out.

https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg07714.html

Thanks,
Kirti

Re: [Qemu-devel] PSA: wiki cleaned up

[Peter Maydell]

On 12 October 2016 at 11:48, Paolo Bonzini  wrote:
> I don't have the power to delete pages on the wiki

You do now :-)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 4/4] qga: add vsock-listen method

[Stefan Hajnoczi]

On Fri, Oct 07, 2016 at 12:07:41PM -0500, Michael Roth wrote:
> Quoting Stefan Hajnoczi (2016-10-06 11:40:18)
> > Add AF_VSOCK (virtio-vsock) support as an alternative to virtio-serial.
> > 
> >   $ qemu-system-x86_64 -device vhost-vsock-pci,guest-cid=3 ...
> >   (guest)# qemu-ga -m vsock-listen -p 3:1234
> > 
> > Signed-off-by: Stefan Hajnoczi 
> 
> Reviewed-by: Michael Roth 
> 
> I still need to get a vsock environment set up to test with, but looks
> good other than minor comments in patch 3.

Linux 4.8 has the guest and vhost drivers:

CONFIG_VSOCKETS=m
CONFIG_VIRTIO_VSOCKETS=m
CONFIG_VIRTIO_VSOCKETS_COMMON=m
CONFIG_VHOST_VSOCK=m

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 3/4] sockets: add AF_VSOCK support

[Stefan Hajnoczi]

On Fri, Oct 07, 2016 at 11:42:35AM -0500, Michael Roth wrote:
> Quoting Stefan Hajnoczi (2016-10-06 11:40:17)
> > Add the AF_VSOCK address family so that qemu-ga will be able to use
> > virtio-vsock.
> > 
> > The AF_VSOCK address family uses  address tuples.  The cid is
> > the unique identifier comparable to an IP address.  AF_VSOCK does not
> > use name resolution so it's seasy to convert between struct sockaddr_vm
> > and strings.
> > 
> > This patch defines a VsockSocketAddress instead of trying to piggy-back
> > on InetSocketAddress.  This is cleaner in the long run since it avoids
> > lots of IPv4 vs IPv6 vs vsock special casing.
> > 
> > Signed-off-by: Stefan Hajnoczi 
> > ---
> >  qapi-schema.json|  23 +-
> >  util/qemu-sockets.c | 222 
> > 
> >  2 files changed, 244 insertions(+), 1 deletion(-)
> > 
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index c3dcf11..8864a96 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -987,12 +987,14 @@
> >  #
> >  # @unix: unix socket
> >  #
> > +# @vsock: vsock family (since 2.8)
> > +#
> >  # @unknown: otherwise
> >  #
> >  # Since: 2.1
> >  ##
> >  { 'enum': 'NetworkAddressFamily',
> > -  'data': [ 'ipv4', 'ipv6', 'unix', 'unknown' ] }
> > +  'data': [ 'ipv4', 'ipv6', 'unix', 'vsock', 'unknown' ] }
> > 
> >  ##
> >  # @VncBasicInfo
> > @@ -3017,6 +3019,24 @@
> >  'path': 'str' } }
> > 
> >  ##
> > +# @VsockSocketAddress
> > +#
> > +# Captures a socket address in the vsock namespace.
> > +#
> > +# @cid: unique host identifier
> > +# @port: port
> > +#
> > +# Note that string types are used to allow for possible future hostname or
> > +# service resolution support.
> > +#
> > +# Since 2.8
> > +##
> > +{ 'struct': 'VsockSocketAddress',
> > +  'data': {
> > +'cid': 'str',
> > +'port': 'str' } }
> 
> Is there any reason to not define these as uint32_t? Not sure if there
> are other reasons for this, but if it's just for consistency with how
> Inet is handled, the code seems to do straight atoi()<->printf("%d") to
> covert between numerical and string representation so it doesn't seem
> like we need to account for any differences between command-line and
> internal representation in sockaddr_vm.

Just in case AF_VSOCK ever supports name and service resolution like
TCP/IP.  In that case cid could be a host name and port could be a
service name.

(I mentioned this in the doc comment.)

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v14 07/21] qapi: permit scalar type conversions in QObjectInputVisitor

[Markus Armbruster]

Markus Armbruster  writes:

> Markus Armbruster  writes:
>
>> "Daniel P. Berrange"  writes:
>>
>>> Currently the QObjectInputVisitor assumes that all scalar
>>> values are directly represented as the final types declared
>>> by the thing being visited. ie it assumes an 'int' is using
>>
>> i.e.
>>
>>> QInt, and a 'bool' is using QBool, etc.  This is good when
>>> QObjectInputVisitor is fed a QObject that came from a JSON
>>> document on the QMP monitor, as it will strictly validate
>>> correctness.
>>>
>>> To allow QObjectInputVisitor to be reused for visiting
>>> a QObject originating from QemuOpts, an alternative mode
>>> is needed where all the scalars types are represented as
>>> QString and converted on the fly to the final desired
>>> type.
>>>
>>> Reviewed-by: Kevin Wolf 
>>> Reviewed-by: Marc-André Lureau 
>>> Signed-off-by: Daniel P. Berrange 
> [...]
>>> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
>>> index 5ff3db3..cf41df6 100644
>>> --- a/qapi/qobject-input-visitor.c
>>> +++ b/qapi/qobject-input-visitor.c
>>> @@ -20,6 +20,7 @@
>>>  #include "qemu-common.h"
>>>  #include "qapi/qmp/types.h"
>>>  #include "qapi/qmp/qerror.h"
>>> +#include "qemu/cutils.h"
>>>  
>>>  #define QIV_STACK_SIZE 1024
>>>  
>>> @@ -263,6 +264,28 @@ static void qobject_input_type_int64(Visitor *v, const 
>>> char *name, int64_t *obj,
>>>  *obj = qint_get_int(qint);
>>>  }
>>>  
>>> +
>>> +static void qobject_input_type_int64_autocast(Visitor *v, const char *name,
>>> +  int64_t *obj, Error **errp)
>>> +{
>>> +QObjectInputVisitor *qiv = to_qiv(v);
>>> +QString *qstr = qobject_to_qstring(qobject_input_get_object(qiv, name,
>>> +true));
>>
>> Needs a rebase for commit 1382d4a.  Same elsewhere.
>>
>>> +int64_t ret;
>>> +
>>> +if (!qstr || !qstr->string) {
>>
>> I don't think !qstr->string can happen.  Same elsewhere.
>>
>>> +error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>>> +   "string");
>>> +return;
>>> +}
>>
>> So far, this is basically qobject_input_type_str() less the g_strdup().
>> Worth factoring out?
>>
>> Now we're entering out parsing swamp:
>>
>>> +
>>> +if (qemu_strtoll(qstr->string, NULL, 0, ) < 0) {
>>> +error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name, "a number");
>
> "integer", actually.

Wait!  You're using QERR_INVALID_PARAMETER_VALUE here, so it's "an
integer".

To be pedantically correct, we'd have to complain about the type when
qemu_strtoll() fails to parse, and about the value when it parses okay,
but the value is out of range.

>>> +return;
>>> +}
>>
>> To serve as replacement for the options visitor, this needs to parse
>> exactly like opts_type_int64().
>>
>> It should also match the JSON parser as far as possible, to minimize
>> difference between the two QObject input visitor variants, and the
>> QemuOpts parser, for command line consistency.
>>
>> opts_type_int64() uses strtoll() directly.  It carefully checks for no
>> conversion (both ways, EINVAL and endptr == str), range, and string not
>> fully consumed.
>>
>> Your code uses qemu_strtoll().  Bug#1: qemu_strtoll() assumes long long
>> is exactly 64 bits.  If it's wider, we fail to diagnose overflow.  If
>> it's narrower, we can't parse all values.  Bug#2: your code fails to
>> check the string is fully consumed.
>>
>> The JSON parser also uses strtoll() directly, in parse_literal().  When
>> we get there, we know that the string consists only of decimal digits,
>> possibly prefixed by a minus sign.  Therefore, strtoll() can only fail
>> with ERANGE, and parse_literal() handles that correctly.
>>
>> QemuOpts doesn't do signed integers.
>>
>>> +*obj = ret;
>>> +}
>>> +
>>>  static void qobject_input_type_uint64(Visitor *v, const char *name,
>>>uint64_t *obj, Error **errp)
>>>  {
>>> @@ -279,6 +302,27 @@ static void qobject_input_type_uint64(Visitor *v, 
>>> const char *name,
>>>  *obj = qint_get_int(qint);
>>>  }
>>>  
>>> +static void qobject_input_type_uint64_autocast(Visitor *v, const char 
>>> *name,
>>> +   uint64_t *obj, Error **errp)
>>> +{
>>> +QObjectInputVisitor *qiv = to_qiv(v);
>>> +QString *qstr = qobject_to_qstring(qobject_input_get_object(qiv, name,
>>> +true));
>>> +unsigned long long ret;
>>> +
>>> +if (!qstr || !qstr->string) {
>>> +error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>>> +   "string");
>>> +return;
>>> +}
>>> +
>>> +if (parse_uint_full(qstr->string, , 0) < 0) {
>>> +error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name, 

[Qemu-devel] [Bug 1630527] Re: qemu/hw/i386/amd_iommu.c:188: possible bad shift ?

[T. Huth]

Thanks for reporting this bug! Looks like this has already been fixed by this 
commit here:
http://git.qemu.org/?p=qemu.git;a=commitdiff;h=1d5b128cbeeab638f772e


** Changed in: qemu
   Status: New => Fix Committed

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

Title:
  qemu/hw/i386/amd_iommu.c:188: possible bad shift ?

Status in QEMU:
  Fix Committed

Bug description:
  qemu/hw/i386/amd_iommu.c:188]: (error) Shifting 32-bit value by 64
  bits is undefined behaviour

  Source code is

  uint64_t mask = ((1 << length) - 1) << bitpos;

  Maybe better code

  uint64_t mask = ((1ULL << length) - 1) << bitpos;

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

Re: [Qemu-devel] [QEMU PATCH v5 3/6] migration: extend VMStateInfo

[Dr. David Alan Gilbert]

* Halil Pasic (pa...@linux.vnet.ibm.com) wrote:
> 
> 
> On 10/12/2016 02:07 PM, Paolo Bonzini wrote:
> > 
> > On 12/10/2016 13:59, Halil Pasic wrote:
> >> > IMHO this would:
> >> > * allow us to keep the good old MVStateInfo objects unmodified and
> >> >   the semantic of VMStateInfo unchanged
> >> > * make clear that VMStateLinked does not care about the calculated size
> >> >   and provide a new anchor for documentation
> >> > * if overloading the semantic of VMStateField.start is not desired we
> >> >   could put it into  VMStateLinked, if needed we could also put more
> >> >   stuff in there
> >> > * have clearer separation between special handling for (linked/certain)
> >> >   data structures and the usual scenario with the DAG.
> > No, I disagree.  We shouldn't be worried about making intrusive changes
> > to all invocations or declarations, if that leads to a simpler API.
> 
> Paolo I agree on a theoretical level. It's just I do not see why this
> particular change makes the API simpler. In my opinion this complicates
> things because now all VMStateInfo's can do funky stuff. Having additional
> state you can poke is rarely a simplification. Same goes for lots
> of arguments especially if some of them are barely ever used. These
> additional parameters contribute nothing for the large majority
> of the cases (except maybe some head scratching when reading
> the code).

I think it might depend how many VMStateInfo's we have.
My ideal rule would be there are no .get/.put implementations outside
of migration/ and we can trust that they would never do anything silly (right?);
so the extra parameters aren't going to be misused too badly.

However, we're probably quite a way from pulling all of the weirder
.get/.put implementations back in.

Dave

> No strong opinion here, it's just that I don't understand. I think one
> trait of a simple API is that it is easy to document. Unfortunately
> the documentation is quite sparse and in the patch the signature
> change goes undocumented.
> 
> Well as I said, just an idea, motivated by how I understood he role of
> VMStateInfo form reading the code.
> 
> Cheers,
> Halil
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v14 07/21] qapi: permit scalar type conversions in QObjectInputVisitor

[Markus Armbruster]

Markus Armbruster  writes:

> "Daniel P. Berrange"  writes:
>
>> Currently the QObjectInputVisitor assumes that all scalar
>> values are directly represented as the final types declared
>> by the thing being visited. ie it assumes an 'int' is using
>
> i.e.
>
>> QInt, and a 'bool' is using QBool, etc.  This is good when
>> QObjectInputVisitor is fed a QObject that came from a JSON
>> document on the QMP monitor, as it will strictly validate
>> correctness.
>>
>> To allow QObjectInputVisitor to be reused for visiting
>> a QObject originating from QemuOpts, an alternative mode
>> is needed where all the scalars types are represented as
>> QString and converted on the fly to the final desired
>> type.
>>
>> Reviewed-by: Kevin Wolf 
>> Reviewed-by: Marc-André Lureau 
>> Signed-off-by: Daniel P. Berrange 
[...]
>> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
>> index 5ff3db3..cf41df6 100644
>> --- a/qapi/qobject-input-visitor.c
>> +++ b/qapi/qobject-input-visitor.c
>> @@ -20,6 +20,7 @@
>>  #include "qemu-common.h"
>>  #include "qapi/qmp/types.h"
>>  #include "qapi/qmp/qerror.h"
>> +#include "qemu/cutils.h"
>>  
>>  #define QIV_STACK_SIZE 1024
>>  
>> @@ -263,6 +264,28 @@ static void qobject_input_type_int64(Visitor *v, const 
>> char *name, int64_t *obj,
>>  *obj = qint_get_int(qint);
>>  }
>>  
>> +
>> +static void qobject_input_type_int64_autocast(Visitor *v, const char *name,
>> +  int64_t *obj, Error **errp)
>> +{
>> +QObjectInputVisitor *qiv = to_qiv(v);
>> +QString *qstr = qobject_to_qstring(qobject_input_get_object(qiv, name,
>> +true));
>
> Needs a rebase for commit 1382d4a.  Same elsewhere.
>
>> +int64_t ret;
>> +
>> +if (!qstr || !qstr->string) {
>
> I don't think !qstr->string can happen.  Same elsewhere.
>
>> +error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>> +   "string");
>> +return;
>> +}
>
> So far, this is basically qobject_input_type_str() less the g_strdup().
> Worth factoring out?
>
> Now we're entering out parsing swamp:
>
>> +
>> +if (qemu_strtoll(qstr->string, NULL, 0, ) < 0) {
>> +error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name, "a number");

"integer", actually.

>> +return;
>> +}
>
> To serve as replacement for the options visitor, this needs to parse
> exactly like opts_type_int64().
>
> It should also match the JSON parser as far as possible, to minimize
> difference between the two QObject input visitor variants, and the
> QemuOpts parser, for command line consistency.
>
> opts_type_int64() uses strtoll() directly.  It carefully checks for no
> conversion (both ways, EINVAL and endptr == str), range, and string not
> fully consumed.
>
> Your code uses qemu_strtoll().  Bug#1: qemu_strtoll() assumes long long
> is exactly 64 bits.  If it's wider, we fail to diagnose overflow.  If
> it's narrower, we can't parse all values.  Bug#2: your code fails to
> check the string is fully consumed.
>
> The JSON parser also uses strtoll() directly, in parse_literal().  When
> we get there, we know that the string consists only of decimal digits,
> possibly prefixed by a minus sign.  Therefore, strtoll() can only fail
> with ERANGE, and parse_literal() handles that correctly.
>
> QemuOpts doesn't do signed integers.
>
>> +*obj = ret;
>> +}
>> +
>>  static void qobject_input_type_uint64(Visitor *v, const char *name,
>>uint64_t *obj, Error **errp)
>>  {
>> @@ -279,6 +302,27 @@ static void qobject_input_type_uint64(Visitor *v, const 
>> char *name,
>>  *obj = qint_get_int(qint);
>>  }
>>  
>> +static void qobject_input_type_uint64_autocast(Visitor *v, const char *name,
>> +   uint64_t *obj, Error **errp)
>> +{
>> +QObjectInputVisitor *qiv = to_qiv(v);
>> +QString *qstr = qobject_to_qstring(qobject_input_get_object(qiv, name,
>> +true));
>> +unsigned long long ret;
>> +
>> +if (!qstr || !qstr->string) {
>> +error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>> +   "string");
>> +return;
>> +}
>> +
>> +if (parse_uint_full(qstr->string, , 0) < 0) {
>> +error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name, "a number");

"integer".

>> +return;
>> +}
>> +*obj = ret;
>
> Differently wrong :)
>
> To serve as replacement for the options visitor, this needs to parse
> exactly like opts_type_uint64().
>
> Again, this should also match the JSON parser and the QemuOpts parser as
> far as possible.
>
> opts_type_uint64() uses parse_uint().  It carefully checks for no
> conversion (EINVAL; parse_uint() normalizes), range, 

Re: [Qemu-devel] [PATCH v10 09/16] block: Add QMP support for streaming to an intermediate layer

[Alberto Garcia]

On Wed 12 Oct 2016 04:30:27 PM CEST, Kevin Wolf  wrote:
>>  if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_STREAM, errp)) {
>>  goto out;
>>  }
>
> Added a bit more context.
>
> This check is redundant now...
>
>>  if (has_base) {
>>  base_bs = bdrv_find_backing_image(bs, base);
>>  if (base_bs == NULL) {
>>  error_setg(errp, QERR_BASE_NOT_FOUND, base);
>>  goto out;
>>  }
>>  assert(bdrv_get_aio_context(base_bs) == aio_context);
>>  base_name = base;
>>  }
>>  
>> +/* Check for op blockers in the whole chain between bs and base */
>> +for (iter = bs; iter && iter != base_bs; iter = backing_bs(iter)) {
>> +if (bdrv_op_is_blocked(iter, BLOCK_OP_TYPE_STREAM, errp)) {
>> +goto out;
>> +}
>> +}
>
> ...because you start with iter = bs here.

You're right, I'll fix it.

> Another thought I had while looking at the previous few patches is
> whether all the op blocker checks wouldn't be better moved to the
> actual block job code (i.e. stream_start in this case).

I thought about that too. In some cases I don't know if it's a good idea
because the qmp_foo_bar() functions do a bit more than simply checking
blockers (e.g. blockdev-mirror creates the target image), so you would
want to have the checks before that.

However doing it in the actual block job code could allow us to do other
things. For example: at the moment when we call block-stream we check
whether a number of BDSs are blocked (in qmp_block_stream()), and if
they're not we proceed to block them (in stream_start()). We could make
block_job_add_bdrv() do both things. On the other hand, since the plan
is to move to a new block job API maybe it's better not to overdo things
now.

It's worth considering for the future anyway.

Berto

Re: [Qemu-devel] [PATCH v10 08/16] block: Support streaming to an intermediate layer

[Kevin Wolf]

Am 12.10.2016 um 16:33 hat Alberto Garcia geschrieben:
> On Wed 12 Oct 2016 04:23:05 PM CEST, Kevin Wolf  wrote:
> >> +/* Block all intermediate nodes between bs and base, because they
> >> + * will disappear from the chain after this operation */
> >> +for (iter = backing_bs(bs); iter && iter != base; iter = 
> >> backing_bs(iter)) {
> >> +block_job_add_bdrv(>common, iter);
> >> +}
> >
> > In patch 6, you had a comment that the top node must be blocked as
> > well because its backing file string will be updated. Isn't it the
> > same for streaming?
> 
> In the block-stream case, 'device' is always the top node, and creating
> the block job there already blocks all operations in it.
> 
> In the block-commit case, 'device' and 'top' are different parameters,
> that's why 'top' must be explicitly blocked. I actually don't think that
> we need to block 'device' at all, and we could even make the parameter
> optional. That would be for a future patch, though. Also, the backing
> file string is not updated in 'top', but in its overlay (unlike in
> block-stream, 'top' disappears after a block-commit job).

I see. So the block job for commit is owned by a node that is
potentially completely unrelated to the operation except that it holds
op blockers and because we use it to finde the overlay to change the
backing file pointers. This is _so_ broken. :-)

With your series (for the op blockers) and BdrvChild (for the operations
on the overlay) we should actually be much closer to finally remove
this. Good news.

Kevin

Re: [Qemu-devel] [PATCH v10 10/16] docs: Document how to stream to an intermediate layer

[Kevin Wolf]

Am 06.10.2016 um 15:02 hat Alberto Garcia geschrieben:
> Signed-off-by: Alberto Garcia 
> ---
>  docs/live-block-ops.txt | 31 ---
>  1 file changed, 20 insertions(+), 11 deletions(-)
> 
> diff --git a/docs/live-block-ops.txt b/docs/live-block-ops.txt
> index a257087..014c8c9 100644
> --- a/docs/live-block-ops.txt
> +++ b/docs/live-block-ops.txt
> @@ -10,9 +10,9 @@ Snapshot live merge
>  Given a snapshot chain, described in this document in the following
>  format:
>  
> -[A] -> [B] -> [C] -> [D]
> +[A] <- [B] <- [C] <- [D] <- [E]
>  
> -Where the rightmost object ([D] in the example) described is the current
> +Where the rightmost object ([E] in the example) described is the current
>  image which the guest OS has write access to. To the left of it is its base
>  image, and so on accordingly until the leftmost image, which has no
>  base.
> @@ -21,11 +21,14 @@ The snapshot live merge operation transforms such a chain 
> into a
>  smaller one with fewer elements, such as this transformation relative
>  to the first example:
>  
> -[A] -> [D]
> +[A] <- [E]
>  
> -Currently only forward merge with target being the active image is
> -supported, that is, data copy is performed in the right direction with
> -destination being the rightmost image.
> +Data is copied in the right direction with destination being the
> +rightmost image, but any other intermediate image can be specified
> +instead. In this example data is copied from [C] into [D], so [D] can
> +be backed by [B]:
> +
> +[A] <- [B] <- [D] <- [E]
>  
>  The operation is implemented in QEMU through image streaming facilities.

This whole document is hopelessly outdated. At least, we need to clarify
here that streaming isn't the only operation that exists and that the
explanation refers to streaming only.

Ideally we would also add a section on commit. And likeweise, the next
section about "live block copy" should be extended to cover mirror and
backup.

Kevin

Re: [Qemu-devel] [PATCH v10 08/16] block: Support streaming to an intermediate layer

[Alberto Garcia]

On Wed 12 Oct 2016 04:23:05 PM CEST, Kevin Wolf  wrote:
>> +/* Block all intermediate nodes between bs and base, because they
>> + * will disappear from the chain after this operation */
>> +for (iter = backing_bs(bs); iter && iter != base; iter = 
>> backing_bs(iter)) {
>> +block_job_add_bdrv(>common, iter);
>> +}
>
> In patch 6, you had a comment that the top node must be blocked as
> well because its backing file string will be updated. Isn't it the
> same for streaming?

In the block-stream case, 'device' is always the top node, and creating
the block job there already blocks all operations in it.

In the block-commit case, 'device' and 'top' are different parameters,
that's why 'top' must be explicitly blocked. I actually don't think that
we need to block 'device' at all, and we could even make the parameter
optional. That would be for a future patch, though. Also, the backing
file string is not updated in 'top', but in its overlay (unlike in
block-stream, 'top' disappears after a block-commit job).

Berto

Re: [Qemu-devel] [PATCH v10 09/16] block: Add QMP support for streaming to an intermediate layer

[Kevin Wolf]

Am 06.10.2016 um 15:02 hat Alberto Garcia geschrieben:
> This patch makes the 'device' parameter of the 'block-stream' command
> accept a node name that is not a root node.
> 
> In addition to that, operation blockers will be checked in all
> intermediate nodes between the top and the base node.
> 
> Signed-off-by: Alberto Garcia 
> ---
>  blockdev.c   | 11 +--
>  qapi/block-core.json | 10 +++---
>  2 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index f13fc69..57c8329 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2937,7 +2937,7 @@ void qmp_block_stream(bool has_job_id, const char 
> *job_id, const char *device,
>bool has_on_error, BlockdevOnError on_error,
>Error **errp)
>  {
> -BlockDriverState *bs;
> +BlockDriverState *bs, *iter;
>  BlockDriverState *base_bs = NULL;
>  AioContext *aio_context;
>  Error *local_err = NULL;
> @@ -2947,7 +2947,7 @@ void qmp_block_stream(bool has_job_id, const char 
> *job_id, const char *device,
>  on_error = BLOCKDEV_ON_ERROR_REPORT;
>  }
>  
> -bs = qmp_get_root_bs(device, errp);
> +bs = bdrv_lookup_bs(device, device, errp);
>  if (!bs) {
>  return;
>  }
>  aio_context = bdrv_get_aio_context(bs);
>  aio_context_acquire(aio_context);
>  
>  if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_STREAM, errp)) {
>  goto out;
>  }

Added a bit more context.

This check is redundant now...

>  if (has_base) {
>  base_bs = bdrv_find_backing_image(bs, base);
>  if (base_bs == NULL) {
>  error_setg(errp, QERR_BASE_NOT_FOUND, base);
>  goto out;
>  }
>  assert(bdrv_get_aio_context(base_bs) == aio_context);
>  base_name = base;
>  }
>  
> +/* Check for op blockers in the whole chain between bs and base */
> +for (iter = bs; iter && iter != base_bs; iter = backing_bs(iter)) {
> +if (bdrv_op_is_blocked(iter, BLOCK_OP_TYPE_STREAM, errp)) {
> +goto out;
> +}
> +}

...because you start with iter = bs here.

Another thought I had while looking at the previous few patches is
whether all the op blocker checks wouldn't be better moved to the actual
block job code (i.e. stream_start in this case). In this case it doesn't
matter much because this is the only caller of stream_start(), but for
other jobs the situation is more complicated and it would be easy for a
caller to forget the checks.

Probably also matter for another series, but I wanted to mention it.

> +
>  /* if we are streaming the entire chain, the result will have no backing
>   * file, and specifying one is therefore an error */
>  if (base_bs == NULL && has_backing_file) {

Kevin

Re: [Qemu-devel] [PATCH v10 08/16] block: Support streaming to an intermediate layer

[Kevin Wolf]

Am 06.10.2016 um 15:02 hat Alberto Garcia geschrieben:
> This makes sure that the image we are streaming into is open in
> read-write mode during the operation.
> 
> Operation blockers are also set in all intermediate nodes, since they
> will be removed from the chain afterwards.
> 
> Finally, this also unblocks the stream operation in backing files.
> 
> Signed-off-by: Alberto Garcia 
> ---
>  block.c|  4 +++-
>  block/stream.c | 24 
>  2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index c80b528..8255d75 100644
> --- a/block.c
> +++ b/block.c
> @@ -1428,9 +1428,11 @@ void bdrv_set_backing_hd(BlockDriverState *bs, 
> BlockDriverState *backing_hd)
>  backing_hd->drv ? backing_hd->drv->format_name : "");
>  
>  bdrv_op_block_all(backing_hd, bs->backing_blocker);
> -/* Otherwise we won't be able to commit due to check in bdrv_commit */
> +/* Otherwise we won't be able to commit or stream */
>  bdrv_op_unblock(backing_hd, BLOCK_OP_TYPE_COMMIT_TARGET,
>  bs->backing_blocker);
> +bdrv_op_unblock(backing_hd, BLOCK_OP_TYPE_STREAM,
> +bs->backing_blocker);
>  /*
>   * We do backup in 3 ways:
>   * 1. drive backup
> diff --git a/block/stream.c b/block/stream.c
> index 3187481..b8ab89a 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -37,6 +37,7 @@ typedef struct StreamBlockJob {
>  BlockDriverState *base;
>  BlockdevOnError on_error;
>  char *backing_file_str;
> +int bs_flags;
>  } StreamBlockJob;
>  
>  static int coroutine_fn stream_populate(BlockBackend *blk,
> @@ -81,6 +82,11 @@ static void stream_complete(BlockJob *job, void *opaque)
>  bdrv_set_backing_hd(bs, base);
>  }
>  
> +/* Reopen the image back in read-only mode if necessary */
> +if (s->bs_flags != bdrv_get_flags(bs)) {
> +bdrv_reopen(bs, s->bs_flags, NULL);
> +}
> +
>  g_free(s->backing_file_str);
>  block_job_completed(>common, data->ret);
>  g_free(data);
> @@ -220,6 +226,8 @@ void stream_start(const char *job_id, BlockDriverState 
> *bs,
>BlockCompletionFunc *cb, void *opaque, Error **errp)
>  {
>  StreamBlockJob *s;
> +BlockDriverState *iter;
> +int orig_bs_flags;
>  
>  s = block_job_create(job_id, _job_driver, bs, speed,
>   cb, opaque, errp);
> @@ -227,8 +235,24 @@ void stream_start(const char *job_id, BlockDriverState 
> *bs,
>  return;
>  }
>  
> +/* Make sure that the image is opened in read-write mode */
> +orig_bs_flags = bdrv_get_flags(bs);
> +if (!(orig_bs_flags & BDRV_O_RDWR)) {
> +if (bdrv_reopen(bs, orig_bs_flags | BDRV_O_RDWR, errp) != 0) {
> +block_job_unref(>common);
> +return;
> +}
> +}
> +
> +/* Block all intermediate nodes between bs and base, because they
> + * will disappear from the chain after this operation */
> +for (iter = backing_bs(bs); iter && iter != base; iter = 
> backing_bs(iter)) {
> +block_job_add_bdrv(>common, iter);
> +}

In patch 6, you had a comment that the top node must be blocked as well
because its backing file string will be updated. Isn't it the same for
streaming?

>  s->base = base;
>  s->backing_file_str = g_strdup(backing_file_str);
> +s->bs_flags = orig_bs_flags;
>  
>  s->on_error = on_error;
>  s->common.co = qemu_coroutine_create(stream_run, s);

Kevin

Re: [Qemu-devel] Async savevm using userfaultfd(2)

[Dr. David Alan Gilbert]

* Stefan Hajnoczi (stefa...@gmail.com) wrote:
> John and I recently discussed asynchronous savevm and I wanted to post
> the ideas so they aren't forgotten.  (We're not actively working on this
> feature.)
> 
> Asynchronous savevm has the same effect as the 'savevm' monitor command:
> it saves RAM, device state, and a snapshot of all disks at the point in
> time the command was issued.
> 
> The current 'savevm' monitor command is synchronous so the guest and
> QEMU monitor are blocked while the operation runs (it can take a
> while!).  Asynchronous savevm has the advantage of allowing the guest
> and QEMU monitor to continue while the operation is running.
> 
> This sounds similar to live migration to file but remember that live
> migration's consistency point is when the guest is paused at the end of
> the iteration phase.  The user has no control over *when* live migration
> captures the guest state.  Therefore it's not a useful command for
> taking snapshots of guest state at a specific point in time - we need
> asynchronous savevm for that.
> 
> Async savevm must copy-on-write guest RAM so the guest can continue
> writing to memory while the snapshot is being saved.  Rik van Riel
> suggested using userfaultfd(2) to do this on Linux.
> 
> Unlike post-copy live migration, we want to track memory writes (instead
> of missing page faults).  The userfaultfd(2) flag
> UFFDIO_REGISTER_MODE_WP provides these semantics.  Unfortunately I think
> UFFDIO_REGISTER_MODE_WP is not yet implemented?

A prototype of this has already been written by Hailiang Zhang;
see https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg03441.html

> Once UFFDIO_REGISTER_MODE_WP is available QEMU can catch writes to guest
> RAM and copy the original pages to a buffer.  If memory is dirtied too
> quickly then it's necessary to throttle the guest or fail the savevm
> operation.

The only limit there is the size of the buffer, waiting for space will
do the throttling.

Dave

> 
> Perhaps this approach can be prototyped with mprotect and a SIGSEGV
> handler if anyone wants to get async savevm going.  I don't know if
> there are any disadvantages to mprotecting guest RAM that the kvm kernel
> module is using.  Hopefully in-kernel devices and vhost will continue to
> work.
> 
> Stefan


--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] Async savevm using userfaultfd(2)

[Eric Blake]

On 10/12/2016 09:04 AM, Stefan Hajnoczi wrote:
> John and I recently discussed asynchronous savevm and I wanted to post
> the ideas so they aren't forgotten.  (We're not actively working on this
> feature.)
> 
> Asynchronous savevm has the same effect as the 'savevm' monitor command:
> it saves RAM, device state, and a snapshot of all disks at the point in
> time the command was issued.
> 

Interesting idea.

I suspect this would have benefits over using fork()'s copy-on-write
semantics, even if we could come up with a way to safely fork where the
child permits no state modification, but merely starts scraping off the
memory state of the guest at the time of the fork.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PULL 00/11] usb patch queue

[Peter Maydell]

On 12 October 2016 at 13:58, Gerd Hoffmann <kra...@redhat.com> wrote:
>   Hi,
>
> This is the usb patch queue, again in bugfixing mode.  xhci transfers
> are allocated dynamically now, the static allocations (and limit) didn't
> play nicely with stream endpoints.  Some xhci cleanups along the way.
> Some bugfixes picked up from the list.
>
> please pull,
>   Gerd
>
> The following changes since commit 6b39b06339ee59559b31f860d4af635b046322df:
>
>   build: Work around SIZE_MAX bug in OSX headers (2016-10-11 19:22:20 +0100)
>
> are available in the git repository at:
>
>   git://git.kraxel.org/qemu tags/pull-usb-20161012-1
>
> for you to fetch changes up to d5c42857d6b0c35028897df8dfc3749eba6f6de3:
>
>   usb-redir: allocate buffers before waking up the host adapter (2016-10-12 
> 14:37:24 +0200)
>
> 
> various usb bugfixes
> some xhci cleanups

Applied, thanks.

-- PMM

Re: [Qemu-devel] [PATCH v14 10/21] qapi: permit auto-creating nested structs

[Markus Armbruster]

"Daniel P. Berrange"  writes:

> Some of the historical command line opts that had their
> keys in in a completely flat namespace are now represented
> by QAPI schemas that use a nested structs. When converting
> the QemuOpts to QObject, there is no information about
> compound types available, so the QObject will be completely
> flat, even after the qdict_crumple() call. So when starting
> a struct, we may not have a QDict available in the input
> data, so we auto-create a QDict containing all the currently
> unvisited input data keys. Not all historical command line
> opts require this, so the behaviour is opt-in, by specifying
> how many levels of structs are permitted to be auto-created.
>
> Note that this only works if the child struct is the last
> field to the visited in the parent struct. This is always
> the case for currently existing legacy command line options.
>
> The example is the NetLegacy type which has 3 levels of
> structs. The modern way to represent this in QemuOpts would
> be the dot-separated component approach
>
>   -net vlan=1,id=foo,name=bar,opts.type=tap,\
>opts.data.fd=3,opts.data.script=ifup
>
> The legacy syntax will just be presenting
>
>   -net vlan=1,id=foo,name=bar,type=tap,fd=3,script=ifup
>
> So we need to auto-create 3 levels of struct when visiting.
>
> The implementation here will enable visiting in both the
> modern and legacy syntax, compared to OptsVisitor which
> only allows the legacy syntax.
>
> Signed-off-by: Daniel P. Berrange 
> ---
>  include/qapi/qobject-input-visitor.h |  22 +-
>  qapi/qobject-input-visitor.c |  59 ++--
>  tests/test-qobject-input-visitor.c   | 130 
> ---
>  3 files changed, 194 insertions(+), 17 deletions(-)
>
> diff --git a/include/qapi/qobject-input-visitor.h 
> b/include/qapi/qobject-input-visitor.h
> index 1809f48..94051f3 100644
> --- a/include/qapi/qobject-input-visitor.h
> +++ b/include/qapi/qobject-input-visitor.h
> @@ -45,7 +45,7 @@ Visitor *qobject_input_visitor_new(QObject *obj, bool 
> strict);
>   * If @autocreate_list is true, then as an alternative to a normal QList,
>   * list values can be stored as a QString or QDict instead, which will
>   * be interpreted as representing single element lists. This should only
> - * by used if compatibility is required with the OptsVisitor which allowed
> + * be used if compatibility is required with the OptsVisitor which allowed
>   * repeated keys, without list indexes, to represent lists. e.g. set this
>   * to true if you have compatibility requirements for
>   *

Typo introduced in the previous patch, please fix it there.

Skipping the rest of this patch until it's clear we need it.

[...]

Re: [Qemu-devel] [PATCH v2] 9pfs: fix memory leak in v9fs_write

[Greg Kurz]

On Wed, 12 Oct 2016 06:10:56 -0700
Li Qiang  wrote:
> From: Li Qiang 
> 
> If an error occurs when marshal the transfer length to the guest, the
> v9fs_write doesn't free an IO vector, thus leading a memory leak.
> This patch fix this.
> 
> Signed-off-by: Li Qiang 
> ---

Good catch again!

Reviewed-by: Greg Kurz 

>  hw/9pfs/9p.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 8b50bfb..c8cf8c2 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -2090,7 +2090,7 @@ static void v9fs_write(void *opaque)
>  offset = 7;
>  err = pdu_marshal(pdu, offset, "d", total);
>  if (err < 0) {
> -goto out;
> +goto out_qiov;
>  }
>  err += offset;
>  trace_v9fs_write_return(pdu->tag, pdu->id, total, err);

Re: [Qemu-devel] [Qemu-block] [PATCH v14 10/21] qapi: permit auto-creating nested structs

[Markus Armbruster]

"Daniel P. Berrange"  writes:

> On Thu, Oct 06, 2016 at 05:51:57PM +0200, Kevin Wolf wrote:
>> Am 06.10.2016 um 17:39 hat Daniel P. Berrange geschrieben:
>> > On Thu, Oct 06, 2016 at 05:30:05PM +0200, Kevin Wolf wrote:
>> > > Am 06.10.2016 um 17:18 hat Daniel P. Berrange geschrieben:
>> > > > On Thu, Oct 06, 2016 at 05:10:42PM +0200, Kevin Wolf wrote:
>> > > > > Am 30.09.2016 um 16:45 hat Daniel P. Berrange geschrieben:
>> > > > > > The example is the NetLegacy type which has 3 levels of
>> > > > > > structs. The modern way to represent this in QemuOpts would
>> > > > > > be the dot-separated component approach
>> > > > > > 
>> > > > > >   -net vlan=1,id=foo,name=bar,opts.type=tap,\
>> > > > > >opts.data.fd=3,opts.data.script=ifup
>> > > > > > 
>> > > > > > The legacy syntax will just be presenting
>> > > > > > 
>> > > > > >   -net vlan=1,id=foo,name=bar,type=tap,fd=3,script=ifup
>> > > > > > 
>> > > > > > So we need to auto-create 3 levels of struct when visiting.
>> > > > > > 
>> > > > > > The implementation here will enable visiting in both the
>> > > > > > modern and legacy syntax, compared to OptsVisitor which
>> > > > > > only allows the legacy syntax.
>> > > > > 
>> > > > > So you're actually introducing the modern syntax only now?
>> > > > 
>> > > > No, the modern syntax is fully implemented by patch 8.
>> > > 
>> > > "now" in the sense of "in this series" (because this means that there is
>> > > no external API to preserve yet).
>> > 
>> > Well the syntax implemented in patch 8 is designed to
>> > 100% mirror the QAPI schema structure nesting. I don't
>> > think we want to change that behaviour.
>> 
>> I think patch 8 is fine as it is.
>> 
>> > If there are certain QAPI schemas structs can still
>> > have freedom to change though, its fine to reconsider
>> > them
>> > 
>> > > 
>> > > > This patch is about adding hacks for the legacy syntax used
>> > > > by the OptsVisitor. The OptsVisitor didn't interpret struct
>> > > > nesting at all, so everything looked flat - this only works
>> > > > as long as you don't have the same key used in multiple
>> > > > structs at different levels, so is not useful as a general
>> > > > approach - it only works by luck really.
>> > > > 
>> > > > > I don't think an "opts.data." prefix makes a lot of sense. I suspect 
>> > > > > the
>> > > > > schema looks this way only because we didn't have flat unions in 1.2.
>> > > > >
>> > > > > So, considering that it is a purely internally used type not visible 
>> > > > > in
>> > > > > QMP, would it make sense to change NetLegacy to be a flat union 
>> > > > > instead,
>> > > > > with NetLegacyOptions as the common base? Then you get the same flat
>> > > > > namespace that we always had and that makes much more sense as an 
>> > > > > API.
>> > > > 
>> > > > Changing that will impact on the QMP data structure, so I don't think
>> > > > we can do that.
>> > > 
>> > > I don't see this type used in QMP at all. It's only used for command
>> > > line parsing, and only with the OptsVisitor, so I think we're fine if we
>> > > flatten it now.
>> > 
>> > Ok, yes, it seems "NetLegacy" is only used in CLI arg parsing, so we
>> > do have some freedom there.
>> > 
>> > This patch was also needed for -numa handling too - again we might have
>> > some flexibility to flatten that.
>> 
>> NumaOptions is also unused in QMP, so it's the same thing.

For better or worse, the QAPI schema doesn't separate externally visible
stuff from purely internal stuff.  Useful trick to see what's external:

$ python -B scripts/qapi-introspect.py -cu -p scratch- qapi-schema.json

This generates scratch-qmp-introspect.c describing the external QMP
interface with unmasked type names (-u).  Anything not mentioned there
can be changed freely.

I'd like to see patches flattening simple unions that aren't externally
visible.  Flat carries a bit of notational overhead in the schema, but I
hope to address that once the QAPI queue is under control.

>> If these two are the only options that need the behaviour, I would
>> prefer if we changed the QAPI schema to flatten them, and then we could
>> save ourselves some complexity by dropping this patch.
>
> Agreed, the fewer hacks like this that we need the better.

Indeed.  Please find out which hacks we actually need.

I believe the best way to keep the warts in check is getting rid of the
options visitor (done in this series) and the string visitor (hopefully
enabled by this series).

Re: [Qemu-devel] [PATCH v10 07/16] block: Block all intermediate nodes in commit_active_start()

[Kevin Wolf]

Am 06.10.2016 um 15:02 hat Alberto Garcia geschrieben:
> When block-commit is launched without the top parameter, it uses
> internally a mirror block job. In that case all intermediate nodes
> between the active and base nodes must be blocked as well.
> 
> Signed-off-by: Alberto Garcia 

Same as the patch before, dataplane is okay because AioContexts are
switched together with the source image.

On second thought, however, maybe both places should set a blocker that
prevents attaching intermediate nodes to a new block device because you
can't read consistent data there. That's a preexisting problem, though,
and needs its own patch.

Reviewed-by: Kevin Wolf 

[Qemu-devel] Async savevm using userfaultfd(2)

[Stefan Hajnoczi]

John and I recently discussed asynchronous savevm and I wanted to post
the ideas so they aren't forgotten.  (We're not actively working on this
feature.)

Asynchronous savevm has the same effect as the 'savevm' monitor command:
it saves RAM, device state, and a snapshot of all disks at the point in
time the command was issued.

The current 'savevm' monitor command is synchronous so the guest and
QEMU monitor are blocked while the operation runs (it can take a
while!).  Asynchronous savevm has the advantage of allowing the guest
and QEMU monitor to continue while the operation is running.

This sounds similar to live migration to file but remember that live
migration's consistency point is when the guest is paused at the end of
the iteration phase.  The user has no control over *when* live migration
captures the guest state.  Therefore it's not a useful command for
taking snapshots of guest state at a specific point in time - we need
asynchronous savevm for that.

Async savevm must copy-on-write guest RAM so the guest can continue
writing to memory while the snapshot is being saved.  Rik van Riel
suggested using userfaultfd(2) to do this on Linux.

Unlike post-copy live migration, we want to track memory writes (instead
of missing page faults).  The userfaultfd(2) flag
UFFDIO_REGISTER_MODE_WP provides these semantics.  Unfortunately I think
UFFDIO_REGISTER_MODE_WP is not yet implemented?

Once UFFDIO_REGISTER_MODE_WP is available QEMU can catch writes to guest
RAM and copy the original pages to a buffer.  If memory is dirtied too
quickly then it's necessary to throttle the guest or fail the savevm
operation.

Perhaps this approach can be prototyped with mprotect and a SIGSEGV
handler if anyone wants to get async savevm going.  I don't know if
there are any disadvantages to mprotecting guest RAM that the kvm kernel
module is using.  Hopefully in-kernel devices and vhost will continue to
work.

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v10 04/16] block: Use block_job_add_bdrv() in backup_start()

[Alberto Garcia]

On Wed 12 Oct 2016 03:47:34 PM CEST, Kevin Wolf  wrote:
> Am 06.10.2016 um 15:02 hat Alberto Garcia geschrieben:
>> Use block_job_add_bdrv() instead of blocking all operations in
>> backup_start() and unblocking them in backup_run().
>> 
>> Signed-off-by: Alberto Garcia 
>
> This has the same problem as mirror (dataplane must be blocked).

Yeah, I think I'll keep dataplane blocked in all cases except in
block_job_create(). BLOCK_OP_TYPE_DATAPLANE is only checked in root
nodes anyway.

Berto

Re: [Qemu-devel] [PATCH v1 2/2] gdbstub: Fix vCont behaviour

[Claudio Imbrenda]

On 12/10/16 15:15, David Hildenbrand wrote:
>>> +for (cx = 0; ccpus && ccpus[cx]; cx++) {
>>> +cpu_single_step(cpu, 0);
> 
> This looks suspicious

why? we set all cpus to single step, since that is the default, and then
we clear the single-step property from all CPUs that should be restarted
in normal mode, then we restart all CPUs. Those in single-step will
indeed only perform one single step, the others will run freely (at
least until the first single-step CPU stops again).

>>> +}
>>> +CPU_FOREACH(cpu) {
>>> +cpu_resume(cpu);
>>> +}
> 
> Claudio, did you have a look at how s->c_cpu is used later on? I remember 
> that we
> have to take care of some query reply packages.

yes, that's set by the H packet and used by the c,s,m,etc packets. vCont
ignores it and doesn't change it
(see here https://sourceware.org/gdb/onlinedocs/gdb/Packets.html )

Re: [Qemu-devel] [PATCH v10 05/16] block: Check blockers in all nodes involved in a block-commit job

[Kevin Wolf]

Am 06.10.2016 um 15:02 hat Alberto Garcia geschrieben:
> qmp_block_commit() checks for op blockers in the active and
> destination (base) images. However all nodes between top_bs and base
> are also involved, and they are removed from the chain afterwards.
> 
> In addition to that, if top_bs is not the active layer then top_bs's
> overlay also needs to be checked because it's involved in the job (its
> backing image string needs to be updated to point to 'base').
> 
> This patch checks that none of those nodes are blocked.
> 
> Signed-off-by: Alberto Garcia 

Reviewed-by: Kevin Wolf 

Re: [Qemu-devel] [PATCH v10 06/16] block: Block all nodes involved in the block-commit operation

[Kevin Wolf]

Am 06.10.2016 um 15:02 hat Alberto Garcia geschrieben:
> After a successful block-commit operation all nodes between top and
> base are removed from the backing chain, and top's overlay needs to
> be updated to point to base. Because of that we should prevent other
> block jobs from messing with them.
> 
> This patch blocks all operations in these nodes in commit_start().

Again, except for dataplane. But this time, I think it's actually okay
because backing files of the source automatically stay in the same
AioContext as the source. Just mention it in the commit message.

> Signed-off-by: Alberto Garcia 

Reviewed-by: Kevin Wolf 

Re: [Qemu-devel] [PATCH v10 04/16] block: Use block_job_add_bdrv() in backup_start()

[Kevin Wolf]

Am 06.10.2016 um 15:02 hat Alberto Garcia geschrieben:
> Use block_job_add_bdrv() instead of blocking all operations in
> backup_start() and unblocking them in backup_run().
> 
> Signed-off-by: Alberto Garcia 

This has the same problem as mirror (dataplane must be blocked).

Kevin

Re: [Qemu-devel] [PATCH 12/15] xen: Rename xen_be_evtchn_event

[Anthony PERARD]

On Tue, Oct 04, 2016 at 09:43:41AM +0300, Emil Condrea wrote:
> Prepare xen_be_evtchn_event to be shared with frontends:
>  * xen_be_evtchn_event -> xen_pv_evtchn_event
> 
> Signed-off-by: Emil Condrea 

Acked-by: Anthony PERARD 

-- 
Anthony PERARD

Re: [Qemu-devel] [PATCH 14/15] xen: Rename xen_be_del_xendev

[Anthony PERARD]

On Tue, Oct 04, 2016 at 09:43:43AM +0300, Emil Condrea wrote:
> Prepare xen_be_del_xendev to be shared with frontends:
>  * xen_be_del_xendev -> xen_pv_del_xendev
> 
> Signed-off-by: Emil Condrea 

Acked-by: Anthony PERARD 

-- 
Anthony PERARD

Re: [Qemu-devel] [PATCHv2 6/7] spapr_pci: Add a 64-bit MMIO window

[Laurent Vivier]

On 12/10/2016 06:44, David Gibson wrote:
> On real hardware, and under pHyp, the PCI host bridges on Power machines
> typically advertise two outbound MMIO windows from the guest's physical
> memory space to PCI memory space:
>   - A 32-bit window which maps onto 2GiB..4GiB in the PCI address space
>   - A 64-bit window which maps onto a large region somewhere high in PCI
> address space (traditionally this used an identity mapping from guest
> physical address to PCI address, but that's not always the case)
> 
> The qemu implementation in spapr-pci-host-bridge, however, only supports a
> single outbound MMIO window, however.  At least some Linux versions expect
> the two windows however, so we arranged this window to map onto the PCI
> memory space from 2 GiB..~64 GiB, then advertised it as two contiguous
> windows, the "32-bit" window from 2G..4G and the "64-bit" window from
> 4G..~64G.
> 
> This approach means, however, that the 64G window is not naturally aligned.
> In turn this limits the size of the largest BAR we can map (which does have
> to be naturally aligned) to roughly half of the total window.  With some
> large nVidia GPGPU cards which have huge memory BARs, this is starting to
> be a problem.
> 
> This patch adds true support for separate 32-bit and 64-bit outbound MMIO
> windows to the spapr-pci-host-bridge implementation, each of which can
> be independently configured.  The 32-bit window always maps to 2G.. in PCI
> space, but the PCI address of the 64-bit window can be configured (it
> defaults to the same as the guest physical address).
> 
> So as not to break possible existing configurations, as long as a 64-bit
> window is not specified, a large single window can be specified.  This
> will appear the same way to the guest as the old approach, although it's
> now implemented by two contiguous memory regions rather than a single one.
> 
> For now, this only adds the possibility of 64-bit windows.  The default
> configuration still uses the legacy mode.
> 
> Signed-off-by: David Gibson 

Reviewed-by: Laurent Vivier 

> ---
>  hw/ppc/spapr.c  | 12 ++---
>  hw/ppc/spapr_pci.c  | 66 
> -
>  include/hw/pci-host/spapr.h |  8 --
>  include/hw/ppc/spapr.h  |  3 ++-
>  4 files changed, 71 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 7cb167c..764a871 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2372,7 +2372,8 @@ static HotpluggableCPUList 
> *spapr_query_hotpluggable_cpus(MachineState *machine)
>  
>  static void spapr_phb_placement(sPAPRMachineState *spapr, uint32_t index,
>  uint64_t *buid, hwaddr *pio, hwaddr 
> *pio_size,
> -hwaddr *mmio, hwaddr *mmio_size,
> +hwaddr *mmio32, hwaddr *mmio32_size,
> +hwaddr *mmio64, hwaddr *mmio64_size,
>  unsigned n_dma, uint32_t *liobns, Error 
> **errp)
>  {
>  const uint64_t base_buid = 0x8002000ULL;
> @@ -2407,8 +2408,13 @@ static void spapr_phb_placement(sPAPRMachineState 
> *spapr, uint32_t index,
>  phb_base = phb0_base + index * phb_spacing;
>  *pio = phb_base + pio_offset;
>  *pio_size = SPAPR_PCI_IO_WIN_SIZE;
> -*mmio = phb_base + mmio_offset;
> -*mmio_size = SPAPR_PCI_MMIO_WIN_SIZE;
> +*mmio32 = phb_base + mmio_offset;
> +*mmio32_size = SPAPR_PCI_MMIO_WIN_SIZE;
> +/*
> + * We don't set the 64-bit MMIO window, relying on the PHB's
> + * fallback behaviour of automatically splitting a large "32-bit"
> + * window into contiguous 32-bit and 64-bit windows
> + */
>  }
>  
>  static void spapr_machine_class_init(ObjectClass *oc, void *data)
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index c0fc964..442185b 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1317,6 +1317,7 @@ static void spapr_phb_realize(DeviceState *dev, Error 
> **errp)
>  if ((sphb->buid != (uint64_t)-1) || (sphb->dma_liobn[0] != 
> (uint32_t)-1)
>  || (sphb->dma_liobn[1] != (uint32_t)-1 && windows_supported == 2)
>  || (sphb->mem_win_addr != (hwaddr)-1)
> +|| (sphb->mem64_win_addr != (hwaddr)-1)
>  || (sphb->io_win_addr != (hwaddr)-1)) {
>  error_setg(errp, "Either \"index\" or other parameters must"
> " be specified for PAPR PHB, not both");
> @@ -1326,6 +1327,7 @@ static void spapr_phb_realize(DeviceState *dev, Error 
> **errp)
>  smc->phb_placement(spapr, sphb->index,
> >buid, >io_win_addr, 
> >io_win_size,
> >mem_win_addr, >mem_win_size,
> +   >mem64_win_addr, >mem64_win_size,
> windows_supported, sphb->dma_liobn, _err);
>  if 

Re: [Qemu-devel] [PATCH 13/15] xen: Rename xen_be_find_xendev

[Anthony PERARD]

On Tue, Oct 04, 2016 at 09:43:42AM +0300, Emil Condrea wrote:
> Prepare xen_be_find_xendev to be shared with frontends:
>  * xen_be_find_xendev -> xen_pv_find_xendev
> 
> Signed-off-by: Emil Condrea 

Acked-by: Anthony PERARD 

-- 
Anthony PERARD

Re: [Qemu-devel] [PATCH 10/15] xen: Rename xen_be_unbind_evtchn

[Anthony PERARD]

On Tue, Oct 04, 2016 at 09:43:39AM +0300, Emil Condrea wrote:
> Prepare xen_be_unbind_evtchn to be shared with frontends:
>  * xen_be_unbind_evtchn -> xen_pv_unbind_evtchn
> 
> Signed-off-by: Emil Condrea 

Acked-by: Anthony PERARD 

-- 
Anthony PERARD

Re: [Qemu-devel] [PATCH v3 6/7] target-arm: Make page size a runtime setting

[Andrew Jones]

On Tue, Oct 11, 2016 at 06:08:18PM +0100, Peter Maydell wrote:
> Rather than defining TARGET_PAGE_BITS to always be 10,
> switch to using a value picked at runtime. This allows us
> to use 4K pages for modern ARM CPUs (and in particular all
> 64-bit CPUs) without having to drop support for the old
> ARMv5 CPUs which had 1K pages.
> 
> Signed-off-by: Peter Maydell 
> ---
>  target-arm/cpu.c | 24 
>  target-arm/cpu.h |  9 +
>  2 files changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 1b9540e..c94a324 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -576,6 +576,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>  ARMCPU *cpu = ARM_CPU(dev);
>  ARMCPUClass *acc = ARM_CPU_GET_CLASS(dev);
>  CPUARMState *env = >env;
> +int pagebits;
>  
>  /* Some features automatically imply others: */
>  if (arm_feature(env, ARM_FEATURE_V8)) {
> @@ -631,6 +632,29 @@ static void arm_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>  set_feature(env, ARM_FEATURE_THUMB_DSP);
>  }
>  
> +if (arm_feature(env, ARM_FEATURE_V7) &&
> +!arm_feature(env, ARM_FEATURE_M) &&
> +!arm_feature(env, ARM_FEATURE_MPU)) {
> +/* v7VMSA drops support for the old ARMv5 tiny pages, so we
> + * can use 4K pages.
> + */
> +pagebits = 12;
> +} else {
> +/* For CPUs which might have tiny 1K pages, or which have an
> + * MPU and might have small region sizes, stick with 1K pages.
> + */
> +pagebits = 10;
> +}
> +if (!set_preferred_target_page_bits(pagebits)) {
> +/* This can only ever happen for hotplugging a CPU, or if
> + * the board code incorrectly creates a CPU which it has
> + * promised via minimum_page_size that it will not.
> + */
> +error_setg(errp, "This CPU requires a smaller page size than the "
> +   "system is using");

I'm not sure about this. IIUC, then with this it won't be possible to
create a board that sets up its cpus with the preferred target page bits
greater than the cpu's default set above, even when the cpu supports it.
For example, I may want a board that will only use AArch64 cpus with 64K
pages. In that case I'd like to set the minimum to 16 bits, but then that
would result in this error. I think we should only set the default when a
preference hasn't already been given. And, maybe we should also provide
a maximum to sanity check against? (side note: if we provide a maximum,
then we could use it in arch_dump.c for the dump info's block size,
which must be the maximum page size the cpu supports.)

Thanks,
drew

> +return;
> +}
> +
>  if (cpu->reset_hivecs) {
>  cpu->reset_sctlr |= (1 << 13);
>  }
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 76d824d..37d6eb3 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -1766,10 +1766,11 @@ bool write_cpustate_to_list(ARMCPU *cpu);
>  #if defined(CONFIG_USER_ONLY)
>  #define TARGET_PAGE_BITS 12
>  #else
> -/* The ARM MMU allows 1k pages.  */
> -/* ??? Linux doesn't actually use these, and they're deprecated in recent
> -   architecture revisions.  Maybe a configure option to disable them.  */
> -#define TARGET_PAGE_BITS 10
> +/* ARMv7 and later CPUs have 4K pages minimum, but ARMv5 and v6
> + * have to support 1K tiny pages.
> + */
> +#define TARGET_PAGE_BITS_VARY
> +#define TARGET_PAGE_BITS_MIN 10
>  #endif
>  
>  #if defined(TARGET_AARCH64)
> -- 
> 2.7.4
> 
> 

Re: [Qemu-devel] [PATCH v3 6/7] target-arm: Make page size a runtime setting

[Peter Maydell]

On 12 October 2016 at 14:33, Andrew Jones  wrote:
> On Tue, Oct 11, 2016 at 06:08:18PM +0100, Peter Maydell wrote:
>> Rather than defining TARGET_PAGE_BITS to always be 10,
>> switch to using a value picked at runtime. This allows us
>> to use 4K pages for modern ARM CPUs (and in particular all
>> 64-bit CPUs) without having to drop support for the old
>> ARMv5 CPUs which had 1K pages.
>>
>> Signed-off-by: Peter Maydell 

>> +if (!set_preferred_target_page_bits(pagebits)) {
>> +/* This can only ever happen for hotplugging a CPU, or if
>> + * the board code incorrectly creates a CPU which it has
>> + * promised via minimum_page_size that it will not.
>> + */
>> +error_setg(errp, "This CPU requires a smaller page size than the "
>> +   "system is using");
>
> I'm not sure about this. IIUC, then with this it won't be possible to
> create a board that sets up its cpus with the preferred target page bits
> greater than the cpu's default set above, even when the cpu supports it.
> For example, I may want a board that will only use AArch64 cpus with 64K
> pages. In that case I'd like to set the minimum to 16 bits, but then that
> would result in this error. I think we should only set the default when a
> preference hasn't already been given. And, maybe we should also provide
> a maximum to sanity check against? (side note: if we provide a maximum,
> then we could use it in arch_dump.c for the dump info's block size,
> which must be the maximum page size the cpu supports.)

If we had a CPU which supported only 64K pages then we would
make this if-ladder set pagebits appropriately. But we don't
have any of those, so it's a bit moot. If the CPU can be
configured by the guest to use 4K pages then the board
must not have set the preferred-page-size to something
larger, or the guest will fall over when it tries it.
That's what this check is protecting against.

The CPU's maximum page size isn't very interesting for this
patchset, because we only care about the minimum (which is
what the TLB needs to use). If the board code was somehow
buggy and requested a page size greater than the CPU's
maximum, then it will also be greater than the CPU's
minimum, and be caught by this error message.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] 9pfs: fix memory leak in v9fs_link

[Greg Kurz]

On Wed, 12 Oct 2016 00:12:48 -0700
Li Qiang  wrote:

> From: Li Qiang 
> 
> In v9fs_link dispatch function, it doesn't put the 'oldfidp'
> fid object, this will make the 'oldfidp->ref' never reach to 0,
> thus leading a memory leak issue. This patch fix this.
> 
> Signed-off-by: Li Qiang 
> ---

Good catch!

Reviewed-by: Greg Kurz 

>  hw/9pfs/9p.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 8b50bfb..29f8b7a 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -2413,6 +2413,7 @@ static void v9fs_link(void *opaque)
>  if (!err) {
>  err = offset;
>  }
> +put_fid(pdu, oldfidp);
>  out:
>  put_fid(pdu, dfidp);
>  out_nofid:

Re: [Qemu-devel] [PATCH 11/15] xen: Rename xen_be_send_notify

[Anthony PERARD]

On Tue, Oct 04, 2016 at 09:43:40AM +0300, Emil Condrea wrote:
> Prepare xen_be_send_notify to be shared with frontends:
>  * xen_be_send_notify -> xen_pv_send_notify
> 
> Signed-off-by: Emil Condrea 

Acked-by: Anthony PERARD 

-- 
Anthony PERARD

Re: [Qemu-devel] [PATCH v2] MAINTAINERS: Add some ARM related files to the corresponding sections

[Alistair Francis]

On Wed, Oct 12, 2016 at 11:57 AM, Thomas Huth  wrote:
> The files w/cpu/a*mpcore.c are already assigned to the ARM CPU
> section, but the corresponding headers include/hw/cpu/a*mpcore.h
> are still missing.
>
> The file hw/*/imx* are already assigned to the i.MX31 machine, but
> the corresponding header files include/hw/*/imx* are still missing.
>
> The file hw/misc/arm_integrator_debug.c seems to belong to Integrator
> CP, hw/cpu/realview_mpcore.c seems to belong to Real View, and
> hw/misc/mst_fpga.c seems to belong to PXA2XX.
>
> And the files hw/misc/zynq* and include/hw/misc/zynq* seem to belong
> to the Xilinx Zynq machine.
>
> Signed-off-by: Thomas Huth 
> ---
>  v2:
>  - Added some more file
>  - Dropped hw/misc/arm_sysctl.c since it's unclear right now how to
>represent a file that belong to multiple machines
>
>  MAINTAINERS | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8c1c0a1..7fc5c9d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -107,6 +107,7 @@ S: Maintained
>  F: target-arm/
>  F: hw/arm/
>  F: hw/cpu/a*mpcore.c
> +F: include/hw/cpu/a*mpcore.h
>  F: disas/arm.c
>  F: disas/arm-a64.cc
>  F: disas/libvixl/
> @@ -408,6 +409,7 @@ M: Peter Chubb 
>  L: qemu-...@nongnu.org
>  S: Odd fixes
>  F: hw/*/imx*
> +F: include/hw/*/imx*
>  F: hw/arm/kzm.c
>  F: include/hw/arm/fsl-imx31.h
>
> @@ -416,6 +418,7 @@ M: Peter Maydell 
>  L: qemu-...@nongnu.org
>  S: Maintained
>  F: hw/arm/integratorcp.c
> +F: hw/misc/arm_integrator_debug.c
>
>  Musicpal
>  M: Jan Kiszka 
> @@ -440,6 +443,7 @@ M: Peter Maydell 
>  L: qemu-...@nongnu.org
>  S: Maintained
>  F: hw/arm/realview*
> +F: hw/cpu/realview_mpcore.c
>  F: hw/intc/realview_gic.c
>  F: include/hw/intc/realview_gic.h
>
> @@ -452,6 +456,7 @@ F: hw/arm/spitz.c
>  F: hw/arm/tosa.c
>  F: hw/arm/z2.c
>  F: hw/*/pxa2xx*
> +F: hw/misc/mst_fpga.c
>  F: include/hw/arm/pxa.h
>
>  Stellaris
> @@ -473,7 +478,8 @@ L: qemu-...@nongnu.org
>  S: Maintained
>  F: hw/*/xilinx_*
>  F: hw/*/cadence_*
> -F: hw/misc/zynq_slcr.c
> +F: hw/misc/zynq*
> +F: include/hw/misc/zynq*
>  X: hw/ssi/xilinx_*

For the Zynq/Xilinx part:

Reviewed-by: Alistair Francis 

Thanks,

Alistair

Thanks,

Alistair


>
>  Xilinx ZynqMP
> --
> 1.8.3.1
>
>

Re: [Qemu-devel] [PATCH] 9pfs: fix integer overflow issue in xattr read/write

[Greg Kurz]

Hi Li,

I agree with the idea behind this patch but I have the impression that some
more work is needed. See below.

On Tue, 11 Oct 2016 21:29:19 -0700
Li Qiang  wrote:

> From: Li Qiang 
> 
> In 9pfs xattr read/write function, it mix to use unsigned/signed
> ,32/64 bits integers. This will causes oob read/write issues. This
> patch fix this.
> 

The root cause for OOB to happen is that the off argument is an unint64_t
coming from the guest: if it exceeds xattr_len more than INT_MAX+2, then
write_count will be equal to INT_MAX and pass the write_count < 0 check.
The use of proper types is needed to detect that.

What about this:

The v9fs_xattr_read() and v9fs_xattr_write() are passed a guest originated
offset: they must ensure this offset does not go beyond the size of the
extended attribute that was set in v9fs_xattrcreate(). Unfortunately, the
current code implement these checks with unsafe calculations on 32 and 64
bit values, which may allow a malicious guest to cause OOB access anyway.

Let's fix this by comparing the offset and the xattr size, which are both
uint64_t, before trying to compute the effective number of bytes to read
or write.

> Signed-off-by: Li Qiang 
> ---
>  hw/9pfs/9p.c | 29 +
>  1 file changed, 13 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index e4040dc..8b50bfb 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -1642,21 +1642,21 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU 
> *pdu, V9fsFidState *fidp,
>  {
>  ssize_t err;
>  size_t offset = 7;
> -int read_count;
> -int64_t xattr_len;
> +uint64_t read_count;
> +uint64_t xattr_len;

I don't think xattr_len is needed. See below.

>  V9fsVirtioState *v = container_of(s, V9fsVirtioState, state);
>  VirtQueueElement *elem = v->elems[pdu->idx];
>  
>  xattr_len = fidp->fs.xattr.len;

typedef struct V9fsXattr
{
int64_t copied_len;
int64_t len;

I believe len should be uint64_t since it comes from the size argument to
setxattr() in the guest:

int setxattr(const char *path, const char *name,
 const void *value, size_t size, int flags);

and it is treated as u64 in the linux kernel client code:

int p9_client_xattrcreate(struct p9_fid *fid, const char *name,
  u64 attr_size, int flags)

I guess copied_len should also be turned to uint64_t as well since its main
use is to account for copied bytes. And introduce a xattrwalk_fid bool
instead of setting copied_len to -1.

I suggest you fix the types in some prelimary patches and then build
this patch on top.

> +if (xattr_len < off) {
> +read_count = 0;
> +goto over_read_count;

goto should only be used when doing rollback on error paths, which is not
the case here. Please use a regular if...else construct instead.

> +}
>  read_count = xattr_len - off;
>  if (read_count > max_count) {
>  read_count = max_count;
> -} else if (read_count < 0) {
> -/*
> - * read beyond XATTR value
> - */
> -read_count = 0;
>  }
> +over_read_count:
>  err = pdu_marshal(pdu, offset, "d", read_count);
>  if (err < 0) {
>  return err;
> @@ -1982,22 +1982,19 @@ static int v9fs_xattr_write(V9fsState *s, V9fsPDU 
> *pdu, V9fsFidState *fidp,
>  {
>  int i, to_copy;
>  ssize_t err = 0;
> -int write_count;
> -int64_t xattr_len;
> +uint64_t write_count;
> +uint64_t xattr_len;

Same remark: xattr_len not needed.

>  size_t offset = 7;
>  
>  
>  xattr_len = fidp->fs.xattr.len;
> +if (xattr_len < off) {
> +err = -ENOSPC;
> +goto out;
> +}
>  write_count = xattr_len - off;
>  if (write_count > count) {
>  write_count = count;
> -} else if (write_count < 0) {
> -/*
> - * write beyond XATTR value len specified in
> - * xattrcreate
> - */
> -err = -ENOSPC;
> -goto out;
>  }
>  err = pdu_marshal(pdu, offset, "d", write_count);
>  if (err < 0) {

Cheers.

--
Greg

Re: [Qemu-devel] [PATCH 08/15] xen: Move xenstore cleanup and mkdir functions

[Anthony PERARD]

On Tue, Oct 04, 2016 at 09:43:37AM +0300, Emil Condrea wrote:
> The name of the functions moved to xen_pvdev.c:
>  * xenstore_cleanup_dir
>  * xen_config_cleanup
>  * xenstore_mkdir
> 
> Signed-off-by: Emil Condrea 

Acked-by: Anthony PERARD 

-- 
Anthony PERARD

Re: [Qemu-devel] [PATCH 09/15] xen: Rename xen_be_printf to xen_pv_printf

[Anthony PERARD]

On Tue, Oct 04, 2016 at 09:43:38AM +0300, Emil Condrea wrote:
> Prepare xen_be_printf to be used by both backend and frontends:
>  * xen_be_printf -> xen_pv_printf
> 
> Signed-off-by: Emil Condrea 

Acked-by: Anthony PERARD 

-- 
Anthony PERARD

Re: [Qemu-devel] [PATCH v1 2/2] gdbstub: Fix vCont behaviour

[David Hildenbrand]

> > +if (def == 0) {
> > +for (cx = 0; scpus && scpus[cx]; cx++) {
> > +cpu_single_step(scpus[cx], sstep_flags);
> > +cpu_resume(scpus[cx]);
> > +}
> > +for (cx = 0; ccpus && ccpus[cx]; cx++) {
> > +cpu_resume(ccpus[cx]);
> > +}
> > +} else if (def == 'c' || def == 'C') {
> > +for (cx = 0; scpus && scpus[cx]; cx++) {
> > +cpu_single_step(scpus[cx], sstep_flags);
> > +}
> > +CPU_FOREACH(cpu) {
> > +cpu_resume(cpu);
> > +}
> > +} else if (def == 's' || def == 'S') {
> > +CPU_FOREACH(cpu) {
> > +cpu_single_step(cpu, sstep_flags);
> > +}
> > +for (cx = 0; ccpus && ccpus[cx]; cx++) {
> > +cpu_single_step(cpu, 0);

This looks suspicious

> > +}
> > +CPU_FOREACH(cpu) {
> > +cpu_resume(cpu);
> > +}
 
Claudio, did you have a look at how s->c_cpu is used later on? I remember that 
we
have to take care of some query reply packages.

David

Re: [Qemu-devel] [PATCH 04/15] xen: Create a new file xen_frontend.c

[Anthony PERARD]

On Tue, Oct 04, 2016 at 09:43:33AM +0300, Emil Condrea wrote:
> Its purpose is to store frontend related functions.
> 
> Signed-off-by: Quan Xu 
> Signed-off-by: Emil Condrea 

Looks good, once the comments on the previous patches are addressed,
same comments for patch 5, 6 and 7.

-- 
Anthony PERARD

Re: [Qemu-devel] [PATCH 09/29] target-sparc: hypervisor mode takes over nucleus mode

[Richard Henderson]

On 10/12/2016 06:33 AM, Artyom Tarasenko wrote:

On Mon, Oct 10, 2016 at 11:41 PM, Richard Henderson  wrote:

On 10/01/2016 05:05 AM, Artyom Tarasenko wrote:


Signed-off-by: Artyom Tarasenko 
---
 target-sparc/cpu.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/target-sparc/cpu.h b/target-sparc/cpu.h
index 0b5c79f..fbeb8d7 100644
--- a/target-sparc/cpu.h
+++ b/target-sparc/cpu.h
@@ -699,10 +699,10 @@ static inline int cpu_mmu_index(CPUSPARCState *env1,
bool ifetch)
 #elif !defined(TARGET_SPARC64)
 return env1->psrs;
 #else
-if (env1->tl > 0) {
-return MMU_NUCLEUS_IDX;
-} else if (cpu_hypervisor_mode(env1)) {
+if (cpu_hypervisor_mode(env1)) {
 return MMU_HYPV_IDX;
+} else if (env1->tl > 0) {
+return MMU_NUCLEUS_IDX;
 } else if (cpu_supervisor_mode(env1)) {
 return MMU_KERNEL_IDX;
 } else {



While playing with your patch set, I discovered that we also need a patch to
get_asi for ASI_N et al to retain MMU_HYPV_IDX, and not decrease privilege.
This happens *very* early in the prom boot, with the first casx (when casx
is implemented inline).


Why is the bug not visible with the current master? I wonder if we
have a symmetrical bug somewhere.


Hmm, I dunno.  I assume it has something to do with casx being implemented out 
of line, and using helper_ld_asi instead of tcg_gen_qemu_ld_tl directly.



r~

Re: [Qemu-devel] [PATCH 03/29] target-sparc: add UA2005 TTE bit #defines

[Richard Henderson]

On 10/12/2016 06:18 AM, Artyom Tarasenko wrote:

What I would most like to see, for QEMU, is an artificial sun4v compatible
machine that implements a "hardware" page table walk.  I.e. no use of
SparcTLBEntry, but walking the page tables directly.

Because QEMU can then satisfy a page lookup internally, without having to
longjmp out of a memory reference in progress in order to restart the cpu
for the software TLB miss handler, the emulation runs about 30-50% faster.
At least that has been my experience emulating Alpha vs MIPS.

It would require custom roms, but those should be fairly easy to modify from
the existing source.



Maybe it's even possible without the modifications. For instance,
implement the table walk compatible with the current hypervisor, and
then just add possibility to overlay hypervisor call using some CPU
feature flag.


Maybe so.  What we lack is being given direct access to the page table base. 
But we know that the CPU structure is in the hypervisor shadow register 0, and 
that offset CPU_ROOT is the page table base.


As long as we're willing to hard-code these two facts concerning any rom we 
care to load, we could in fact implement the tlb miss success path inside QEMU. 
 We would let the rom re-do the work for the tlb miss failure path, on the way 
to raising the exception with the supervisor.



r~

Re: [Qemu-devel] [PATCH 01/29] target-sparc: don't trap on MMU-fault if MMU is disabled

[Artyom Tarasenko]

On Tue, Oct 11, 2016 at 4:50 PM, Richard Henderson  wrote:
> On 10/11/2016 09:00 AM, Artyom Tarasenko wrote:
>>
>> On Mon, Oct 10, 2016 at 11:14 PM, Richard Henderson 
>> wrote:
>>>
>>> On 10/01/2016 05:05 AM, Artyom Tarasenko wrote:


  if (is_exec) {
 -helper_raise_exception(env, TT_CODE_ACCESS);
 +if (env->lsu & (IMMU_E)) {
 +helper_raise_exception(env, TT_CODE_ACCESS);
 +}
  } else {
 -helper_raise_exception(env, TT_DATA_ACCESS);
 +if (env->lsu & (DMMU_E)) {
 +helper_raise_exception(env, TT_DATA_ACCESS);
 +}
>>>
>>>
>>>
>>> The cpu really does no kind of machine check for a hypervisor write to
>>> 0x1122334455667788?
>>
>>
>> A bare metal machine would raise Real Translation Exception. But since
>> we don't do real addresses...
>
>
> I was asking about an errant access from the hypervisor itself.  I would
> have thought the guest running without an mmu would be running with real
> addresses, but the hypervisor itself would be running in physical addresses.
>
> But that said, we can't just let the access go completely unreported,
> surely. We can think as if we do real addresses, but with a 1-1 mapping to
> physical. So at least for !cpu_hypervisor_mode(env), a Real Translation
> Exception would seem to be totally justified.

Good point. data_access_error/instruction_access_error shall happen
in the hypervisor mode, otherwise it shall be data_real_translation_miss /
instruction_real_translation_miss.

Will change it, thanks.

-- 
Regards,
Artyom Tarasenko

SPARC and PPC PReP under qemu blog: http://tyom.blogspot.com/search/label/qemu

Re: [Qemu-devel] [PATCH 1/2] 9pfs: fix information leak in xattr read

[Greg Kurz]

On Mon, 10 Oct 2016 10:56:03 +0200
Greg Kurz  wrote:

> On Sat,  8 Oct 2016 22:26:51 -0700
> Li Qiang  wrote:
> 
> > From: Li Qiang 
> > 
> > 9pfs uses g_malloc() to allocate the xattr memory space, if the guest
> > reads this memory before writing to it, this will leak host heap memory
> > to the guest. This patch avoid this.
> > 
> > Signed-off-by: Li Qiang 
> > ---  
> 
> I've looked again and we could theorically defer allocation until
> v9fs_xattr_write() is called, and only allow v9fs_xattr_read() to
> return bytes previously written by the client. But this would
> result in rather complex code to handle partial writes and reads.
> 
> So this patch looks like the way to go.
> 
> Reviewed-by: Greg Kurz 
> 

But in fact, I'm afraid we have a more serious problem here... size
comes from the guest and could cause g_malloc() to abort if QEMU has
reached some RLIMIT... we need to call g_try_malloc0() and return
ENOMEM if the allocation fails.

Since this is yet another issue, I suggest you send another patch
on top of this one... and maybe check other locations in the code
where this could happen.

Cheers.

--
Greg

> >  hw/9pfs/9p.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index 119ee58..8751c19 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -3282,7 +3282,7 @@ static void v9fs_xattrcreate(void *opaque)
> >  xattr_fidp->fs.xattr.flags = flags;
> >  v9fs_string_init(_fidp->fs.xattr.name);
> >  v9fs_string_copy(_fidp->fs.xattr.name, );
> > -xattr_fidp->fs.xattr.value = g_malloc(size);
> > +xattr_fidp->fs.xattr.value = g_malloc0(size);
> >  err = offset;
> >  put_fid(pdu, file_fidp);
> >  out_nofid:  
> 
> 

[Qemu-devel] [PATCH v2] 9pfs: fix memory leak in v9fs_write

[Li Qiang]

From: Li Qiang 

If an error occurs when marshal the transfer length to the guest, the
v9fs_write doesn't free an IO vector, thus leading a memory leak.
This patch fix this.

Signed-off-by: Li Qiang 
---
 hw/9pfs/9p.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 8b50bfb..c8cf8c2 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -2090,7 +2090,7 @@ static void v9fs_write(void *opaque)
 offset = 7;
 err = pdu_marshal(pdu, offset, "d", total);
 if (err < 0) {
-goto out;
+goto out_qiov;
 }
 err += offset;
 trace_v9fs_write_return(pdu->tag, pdu->id, total, err);
-- 
1.8.3.1

[Qemu-devel] [Bug 1381642] Re: ecovec.c:66: buffer too small by one.

[Peter Maydell]

...and we don't build u-boot for the renesas ecovec, so we don't need to
worry about updating our copy of u-boot to something with the fix in it.

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

Title:
  ecovec.c:66: buffer too small by one.

Status in QEMU:
  Fix Released

Bug description:
  [qemu-2.1.2/roms/u-boot/board/renesas/ecovec/ecovec.c:66]: (error)
  Buffer is accessed out of bounds.

  sprintf(env_mac, "%02X:%02X:%02X:%02X:%02X:%02X",
  mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);

  but

  char env_mac[17];

  and 18 into 17 won't go. Suggest increase size of env_mac.

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

[Qemu-devel] [PULL 06/11] xhci: add & use xhci_kick_epctx()

[Gerd Hoffmann]

xhci_kick_epctx is a xhci_kick_ep variant which takes an XHCIEPContext
as input instead of slotid and epid.  So in case we have a XHCIEPContext
at hand at the callsite we can just pass it directly.

Signed-off-by: Gerd Hoffmann 
Message-id: 1474965172-30321-7-git-send-email-kra...@redhat.com
---
 hw/usb/hcd-xhci.c | 43 ++-
 1 file changed, 26 insertions(+), 17 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 20f46c4..c82fe09 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -509,6 +509,7 @@ enum xhci_flags {
 
 static void xhci_kick_ep(XHCIState *xhci, unsigned int slotid,
  unsigned int epid, unsigned int streamid);
+static void xhci_kick_epctx(XHCIEPContext *epctx, unsigned int streamid);
 static TRBCCode xhci_disable_ep(XHCIState *xhci, unsigned int slotid,
 unsigned int epid);
 static void xhci_xfer_report(XHCITransfer *xfer);
@@ -1362,7 +1363,7 @@ static void xhci_set_ep_state(XHCIState *xhci, 
XHCIEPContext *epctx,
 static void xhci_ep_kick_timer(void *opaque)
 {
 XHCIEPContext *epctx = opaque;
-xhci_kick_ep(epctx->xhci, epctx->slotid, epctx->epid, 0);
+xhci_kick_epctx(epctx, 0);
 }
 
 static XHCIEPContext *xhci_alloc_epctx(XHCIState *xhci,
@@ -2008,7 +2009,7 @@ static int xhci_fire_ctl_transfer(XHCIState *xhci, 
XHCITransfer *xfer)
 
 xhci_complete_packet(xfer);
 if (!xfer->running_async && !xfer->running_retry) {
-xhci_kick_ep(xhci, xfer->slotid, xfer->epid, 0);
+xhci_kick_epctx(xfer->epctx, 0);
 }
 return 0;
 }
@@ -2112,7 +2113,7 @@ static int xhci_submit(XHCIState *xhci, XHCITransfer 
*xfer, XHCIEPContext *epctx
 
 xhci_complete_packet(xfer);
 if (!xfer->running_async && !xfer->running_retry) {
-xhci_kick_ep(xhci, xfer->slotid, xfer->epid, xfer->streamid);
+xhci_kick_epctx(xfer->epctx, xfer->streamid);
 }
 return 0;
 }
@@ -2126,16 +2127,8 @@ static int xhci_fire_transfer(XHCIState *xhci, 
XHCITransfer *xfer, XHCIEPContext
 static void xhci_kick_ep(XHCIState *xhci, unsigned int slotid,
  unsigned int epid, unsigned int streamid)
 {
-XHCIStreamContext *stctx;
 XHCIEPContext *epctx;
-XHCITransfer *xfer;
-XHCIRing *ring;
-USBEndpoint *ep = NULL;
-uint64_t mfindex;
-int length;
-int i;
 
-trace_usb_xhci_ep_kick(slotid, epid, streamid);
 assert(slotid >= 1 && slotid <= xhci->numslots);
 assert(epid >= 1 && epid <= 31);
 
@@ -2150,11 +2143,27 @@ static void xhci_kick_ep(XHCIState *xhci, unsigned int 
slotid,
 return;
 }
 
+xhci_kick_epctx(epctx, streamid);
+}
+
+static void xhci_kick_epctx(XHCIEPContext *epctx, unsigned int streamid)
+{
+XHCIState *xhci = epctx->xhci;
+XHCIStreamContext *stctx;
+XHCITransfer *xfer;
+XHCIRing *ring;
+USBEndpoint *ep = NULL;
+uint64_t mfindex;
+int length;
+int i;
+
+trace_usb_xhci_ep_kick(epctx->slotid, epctx->epid, streamid);
+
 /* If the device has been detached, but the guest has not noticed this
yet the 2 above checks will succeed, but we must NOT continue */
-if (!xhci->slots[slotid - 1].uport ||
-!xhci->slots[slotid - 1].uport->dev ||
-!xhci->slots[slotid - 1].uport->dev->attached) {
+if (!xhci->slots[epctx->slotid - 1].uport ||
+!xhci->slots[epctx->slotid - 1].uport->dev ||
+!xhci->slots[epctx->slotid - 1].uport->dev->attached) {
 return;
 }
 
@@ -2235,7 +2244,7 @@ static void xhci_kick_ep(XHCIState *xhci, unsigned int 
slotid,
 }
 xfer->streamid = streamid;
 
-if (epid == 1) {
+if (epctx->epid == 1) {
 xhci_fire_ctl_transfer(xhci, xfer);
 } else {
 xhci_fire_transfer(xhci, xfer, epctx);
@@ -2255,7 +2264,7 @@ static void xhci_kick_ep(XHCIState *xhci, unsigned int 
slotid,
 }
 }
 
-ep = xhci_epid_to_usbep(xhci, slotid, epid);
+ep = xhci_epid_to_usbep(xhci, epctx->slotid, epctx->epid);
 if (ep) {
 usb_device_flush_ep_queue(ep->dev, ep);
 }
@@ -3486,7 +3495,7 @@ static void xhci_complete(USBPort *port, USBPacket 
*packet)
 return;
 }
 xhci_complete_packet(xfer);
-xhci_kick_ep(xfer->epctx->xhci, xfer->slotid, xfer->epid, xfer->streamid);
+xhci_kick_epctx(xfer->epctx, xfer->streamid);
 if (xfer->complete) {
 xhci_ep_free_xfer(xfer);
 }
-- 
1.8.3.1

[Qemu-devel] [PATCH] input-linux: initialize key state

[Gerd Hoffmann]

Query input device keys, initialize state accordingly, so the correct
state is reflected in case any key is pressed at initialization time.
There is a high chance for this to actually happen for the 'enter' key
in case you start qemu with a terminal command (directly or virsh).

When finding any pressed keys the input grab is delayed until all keys
are lifted, to avoid confusing guest and host with appearently stuck
keys.

Reported-by: Muted Bytes 
Signed-off-by: Gerd Hoffmann 
---
 ui/input-linux.c | 15 +--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/ui/input-linux.c b/ui/input-linux.c
index 0e230ce..f345317 100644
--- a/ui/input-linux.c
+++ b/ui/input-linux.c
@@ -347,7 +347,8 @@ static void input_linux_event(void *opaque)
 static void input_linux_complete(UserCreatable *uc, Error **errp)
 {
 InputLinux *il = INPUT_LINUX(uc);
-uint8_t evtmap, relmap, absmap, keymap[KEY_CNT / 8];
+uint8_t evtmap, relmap, absmap;
+uint8_t keymap[KEY_CNT / 8], keystate[KEY_CNT / 8];
 unsigned int i;
 int rc, ver;
 
@@ -394,6 +395,7 @@ static void input_linux_complete(UserCreatable *uc, Error 
**errp)
 if (evtmap & (1 << EV_KEY)) {
 memset(keymap, 0, sizeof(keymap));
 rc = ioctl(il->fd, EVIOCGBIT(EV_KEY, sizeof(keymap)), keymap);
+rc = ioctl(il->fd, EVIOCGKEY(sizeof(keystate)), keystate);
 for (i = 0; i < KEY_CNT; i++) {
 if (keymap[i / 8] & (1 << (i % 8))) {
 if (linux_is_button(i)) {
@@ -401,12 +403,21 @@ static void input_linux_complete(UserCreatable *uc, Error 
**errp)
 } else {
 il->num_keys++;
 }
+if (keystate[i / 8] & (1 << (i % 8))) {
+il->keydown[i] = true;
+il->keycount++;
+}
 }
 }
 }
 
 qemu_set_fd_handler(il->fd, input_linux_event, NULL, il);
-input_linux_toggle_grab(il);
+if (il->keycount) {
+/* delay grab until all keys are released */
+il->grab_request = true;
+} else {
+input_linux_toggle_grab(il);
+}
 QTAILQ_INSERT_TAIL(, il, next);
 il->initialized = true;
 return;
-- 
1.8.3.1

Re: [Qemu-devel] Live migration without bdrv_drain_all()

[Stefan Hajnoczi]

On Tue, Sep 27, 2016 at 10:48:48AM +0100, Daniel P. Berrange wrote:
> On Mon, Aug 29, 2016 at 11:06:48AM -0400, Stefan Hajnoczi wrote:
> > At KVM Forum an interesting idea was proposed to avoid
> > bdrv_drain_all() during live migration.  Mike Cui and Felipe Franciosi
> > mentioned running at queue depth 1.  It needs more thought to make it
> > workable but I want to capture it here for discussion and to archive
> > it.
> > 
> > bdrv_drain_all() is synchronous and can cause VM downtime if I/O
> > requests hang.  We should find a better way of quiescing I/O that is
> > not synchronous.  Up until now I thought we should simply add a
> > timeout to bdrv_drain_all() so it can at least fail (and live
> > migration would fail) if I/O is stuck instead of hanging the VM.  But
> > the following approach is also interesting...
> 
> How would you decide what an acceptable timeout is for the drain
> operation ?

Same as most timeouts: an arbitrary number :(.

> At what point does a stuck drain op cause the VM
> to stall ?

The drain call has acquired the QEMU global mutex.  Any vmexit that
requires taking the QEMU global mutex will hang that thread (i.e. vcpu
thread).

Stefan


signature.asc
Description: PGP signature

[Qemu-devel] [PULL 04/11] xhci: use linked list for transfers

[Gerd Hoffmann]

xhci has a fixed number of 24 (TD_QUEUE) XHCITransfer structs per
endpoint, which turns out to be a problem for usb3 devices with 32 (or
more) bulk streams.  xhci re-checks the trb rings on every finished
transfer to make sure it'll pick up any pending work.  But that scheme
breaks in case the first transfer of a ring can't be started because we
ran out of XHCITransfer structs already.

So remove static XHCITransfer array from XHCIEPContext.  Use a linked
list instead, and allocate/free XHCITransfer as needed.  Add helper
functions to allocate & initialize and to cleanup & release
XHCITransfer structs.  That also simplifies trb management, we never
have to realloc XHCITransfer->trbs because we don't reuse XHCITransfer
structs any more.

New dynamic limit for in-flight xhci transfers per endpoint is
number-of-streams + 16.

Signed-off-by: Gerd Hoffmann 
Message-id: 1474965172-30321-5-git-send-email-kra...@redhat.com
---
 hw/usb/hcd-xhci.c | 122 ++
 1 file changed, 68 insertions(+), 54 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 3a035c8..e14c2a8 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -21,6 +21,7 @@
 #include "qemu/osdep.h"
 #include "hw/hw.h"
 #include "qemu/timer.h"
+#include "qemu/queue.h"
 #include "hw/usb.h"
 #include "hw/pci/pci.h"
 #include "hw/pci/msi.h"
@@ -46,8 +47,6 @@
 #define MAXSLOTS 64
 #define MAXINTRS 16
 
-#define TD_QUEUE 24
-
 /* Very pessimistic, let's hope it's enough for all cases */
 #define EV_QUEUE (((3 * 24) + 16) * MAXSLOTS)
 /* Do not deliver ER Full events. NEC's driver does some things not bound
@@ -346,6 +345,7 @@ typedef struct XHCIPort {
 
 typedef struct XHCITransfer {
 XHCIState *xhci;
+XHCIEPContext *epctx;
 USBPacket packet;
 QEMUSGList sgl;
 bool running_async;
@@ -361,7 +361,6 @@ typedef struct XHCITransfer {
 bool timed_xfer;
 
 unsigned int trb_count;
-unsigned int trb_alloced;
 XHCITRB *trbs;
 
 TRBCCode status;
@@ -371,6 +370,8 @@ typedef struct XHCITransfer {
 unsigned int cur_pkt;
 
 uint64_t mfindex_kick;
+
+QTAILQ_ENTRY(XHCITransfer) next;
 } XHCITransfer;
 
 struct XHCIStreamContext {
@@ -385,8 +386,8 @@ struct XHCIEPContext {
 unsigned int epid;
 
 XHCIRing ring;
-unsigned int next_xfer;
-XHCITransfer transfers[TD_QUEUE];
+uint32_t xfer_count;
+QTAILQ_HEAD(, XHCITransfer) transfers;
 XHCITransfer *retry;
 EPType type;
 dma_addr_t pctx;
@@ -1370,19 +1371,13 @@ static XHCIEPContext *xhci_alloc_epctx(XHCIState *xhci,
unsigned int epid)
 {
 XHCIEPContext *epctx;
-int i;
 
 epctx = g_new0(XHCIEPContext, 1);
 epctx->xhci = xhci;
 epctx->slotid = slotid;
 epctx->epid = epid;
 
-for (i = 0; i < ARRAY_SIZE(epctx->transfers); i++) {
-epctx->transfers[i].xhci = xhci;
-epctx->transfers[i].slotid = slotid;
-epctx->transfers[i].epid = epid;
-usb_packet_init(>transfers[i].packet);
-}
+QTAILQ_INIT(>transfers);
 epctx->kick_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, xhci_ep_kick_timer, 
epctx);
 
 return epctx;
@@ -1443,6 +1438,41 @@ static TRBCCode xhci_enable_ep(XHCIState *xhci, unsigned 
int slotid,
 return CC_SUCCESS;
 }
 
+static XHCITransfer *xhci_ep_alloc_xfer(XHCIEPContext *epctx,
+uint32_t length)
+{
+uint32_t limit = epctx->nr_pstreams + 16;
+XHCITransfer *xfer;
+
+if (epctx->xfer_count >= limit) {
+return NULL;
+}
+
+xfer = g_new0(XHCITransfer, 1);
+xfer->xhci = epctx->xhci;
+xfer->epctx = epctx;
+xfer->slotid = epctx->slotid;
+xfer->epid = epctx->epid;
+xfer->trbs = g_new(XHCITRB, length);
+xfer->trb_count = length;
+usb_packet_init(>packet);
+
+QTAILQ_INSERT_TAIL(>transfers, xfer, next);
+epctx->xfer_count++;
+
+return xfer;
+}
+
+static void xhci_ep_free_xfer(XHCITransfer *xfer)
+{
+QTAILQ_REMOVE(>epctx->transfers, xfer, next);
+xfer->epctx->xfer_count--;
+
+usb_packet_cleanup(>packet);
+g_free(xfer->trbs);
+g_free(xfer);
+}
+
 static int xhci_ep_nuke_one_xfer(XHCITransfer *t, TRBCCode report)
 {
 int killed = 0;
@@ -1469,7 +1499,7 @@ static int xhci_ep_nuke_one_xfer(XHCITransfer *t, 
TRBCCode report)
 g_free(t->trbs);
 
 t->trbs = NULL;
-t->trb_count = t->trb_alloced = 0;
+t->trb_count = 0;
 
 return killed;
 }
@@ -1479,7 +1509,8 @@ static int xhci_ep_nuke_xfers(XHCIState *xhci, unsigned 
int slotid,
 {
 XHCISlot *slot;
 XHCIEPContext *epctx;
-int i, xferi, killed = 0;
+XHCITransfer *xfer;
+int killed = 0;
 USBEndpoint *ep = NULL;
 assert(slotid >= 1 && slotid <= xhci->numslots);
 assert(epid >= 1 && epid <= 31);
@@ -1494,14 +1525,16 @@ static int xhci_ep_nuke_xfers(XHCIState *xhci, unsigned 
int slotid,
 
 epctx = slot->eps[epid-1];
 
-xferi = 

[Qemu-devel] [PULL 10/11] usb: Fix incorrect default DMA offset.

[Gerd Hoffmann]

From: Vijay Kumar B 

The default DMA offset is set to 3. When the property is not set by
the consumer, the default causes DMA access to be shifted by 3
bytes. In PXA, this results in incorrect DMA access, leading to error
notification in the USB controller driver. A better default would be
0, so that there is no offset, when the consumer does not specify one.

Signed-off-by: Vijay Kumar B. 
Reviewed-by: Deepak S. 
Message-id: 1475060958-7760-1-git-send-email-vijayku...@zilogic.com
Signed-off-by: Gerd Hoffmann 
---
 hw/usb/hcd-ohci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index fa57038..c82a92f 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -2139,7 +2139,7 @@ static const TypeInfo ohci_pci_info = {
 
 static Property ohci_sysbus_properties[] = {
 DEFINE_PROP_UINT32("num-ports", OHCISysBusState, num_ports, 3),
-DEFINE_PROP_DMAADDR("dma-offset", OHCISysBusState, dma_offset, 3),
+DEFINE_PROP_DMAADDR("dma-offset", OHCISysBusState, dma_offset, 0),
 DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
1.8.3.1

[Qemu-devel] [PULL 03/11] xhci: drop unused comp_xfer field

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann 
Message-id: 1474965172-30321-4-git-send-email-kra...@redhat.com
---
 hw/usb/hcd-xhci.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index d9ac1b4..3a035c8 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -386,7 +386,6 @@ struct XHCIEPContext {
 
 XHCIRing ring;
 unsigned int next_xfer;
-unsigned int comp_xfer;
 XHCITransfer transfers[TD_QUEUE];
 XHCITransfer *retry;
 EPType type;
-- 
1.8.3.1

[Qemu-devel] [PULL 09/11] usb: fix serial generator

[Gerd Hoffmann]

snprintf return value is *not* the number of chars written into the
buffer, but the number of chars needed.  So in case the buffer is too
small you can go alloc a bigger one and try again.  But that also means
you can't simply use the return value for the next snprintf call
without checking beforehand that things did actually fit.

Problem is that usb_desc_create_serial didn't perform that check, so a
lng path string (can happen with deep pci-bridge nesting) results in
the third snprintf call smashing the stack.

Fix this by throwing out all the snpintf calls and use g_strdup_printf
instead.

https://bugzilla.redhat.com/show_bug.cgi?id=1381630

Reported-by: Thomas Huth 
Signed-off-by: Gerd Hoffmann 
Reviewed-by: Thomas Huth 
Reviewed-by: Eric Blake 
Message-id: 1475659998-22045-1-git-send-email-kra...@redhat.com
---
 hw/usb/desc.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/usb/desc.c b/hw/usb/desc.c
index 5e0e1d1..7828e52 100644
--- a/hw/usb/desc.c
+++ b/hw/usb/desc.c
@@ -556,9 +556,7 @@ void usb_desc_create_serial(USBDevice *dev)
 DeviceState *hcd = dev->qdev.parent_bus->parent;
 const USBDesc *desc = usb_device_get_usb_desc(dev);
 int index = desc->id.iSerialNumber;
-char serial[64];
-char *path;
-int dst;
+char *path, *serial;
 
 if (dev->serial) {
 /* 'serial' usb bus property has priority if present */
@@ -567,14 +565,16 @@ void usb_desc_create_serial(USBDevice *dev)
 }
 
 assert(index != 0 && desc->str[index] != NULL);
-dst = snprintf(serial, sizeof(serial), "%s", desc->str[index]);
 path = qdev_get_dev_path(hcd);
 if (path) {
-dst += snprintf(serial+dst, sizeof(serial)-dst, "-%s", path);
+serial = g_strdup_printf("%s-%s-%s", desc->str[index],
+ path, dev->port->path);
+} else {
+serial = g_strdup_printf("%s-%s", desc->str[index], dev->port->path);
 }
-dst += snprintf(serial+dst, sizeof(serial)-dst, "-%s", dev->port->path);
 usb_desc_set_string(dev, index, serial);
 g_free(path);
+g_free(serial);
 }
 
 const char *usb_desc_get_string(USBDevice *dev, uint8_t index)
-- 
1.8.3.1

[Qemu-devel] [PULL 05/11] xhci: drop XHCITransfer->xhci

[Gerd Hoffmann]

Use XHCITransfer->epctx->xhci instead.

Signed-off-by: Gerd Hoffmann 
Message-id: 1474965172-30321-6-git-send-email-kra...@redhat.com
---
 hw/usb/hcd-xhci.c | 22 +-
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index e14c2a8..20f46c4 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -344,7 +344,6 @@ typedef struct XHCIPort {
 } XHCIPort;
 
 typedef struct XHCITransfer {
-XHCIState *xhci;
 XHCIEPContext *epctx;
 USBPacket packet;
 QEMUSGList sgl;
@@ -1449,7 +1448,6 @@ static XHCITransfer *xhci_ep_alloc_xfer(XHCIEPContext 
*epctx,
 }
 
 xfer = g_new0(XHCITransfer, 1);
-xfer->xhci = epctx->xhci;
 xfer->epctx = epctx;
 xfer->slotid = epctx->slotid;
 xfer->epid = epctx->epid;
@@ -1488,10 +1486,9 @@ static int xhci_ep_nuke_one_xfer(XHCITransfer *t, 
TRBCCode report)
 killed = 1;
 }
 if (t->running_retry) {
-XHCIEPContext *epctx = t->xhci->slots[t->slotid-1].eps[t->epid-1];
-if (epctx) {
-epctx->retry = NULL;
-timer_del(epctx->kick_timer);
+if (t->epctx) {
+t->epctx->retry = NULL;
+timer_del(t->epctx->kick_timer);
 }
 t->running_retry = 0;
 killed = 1;
@@ -1721,7 +1718,7 @@ static TRBCCode xhci_set_ep_dequeue(XHCIState *xhci, 
unsigned int slotid,
 
 static int xhci_xfer_create_sgl(XHCITransfer *xfer, int in_xfer)
 {
-XHCIState *xhci = xfer->xhci;
+XHCIState *xhci = xfer->epctx->xhci;
 int i;
 
 xfer->int_req = false;
@@ -1780,7 +1777,7 @@ static void xhci_xfer_report(XHCITransfer *xfer)
 bool reported = 0;
 bool shortpkt = 0;
 XHCIEvent event = {ER_TRANSFER, CC_SUCCESS};
-XHCIState *xhci = xfer->xhci;
+XHCIState *xhci = xfer->epctx->xhci;
 int i;
 
 left = xfer->packet.actual_length;
@@ -1854,9 +1851,8 @@ static void xhci_xfer_report(XHCITransfer *xfer)
 
 static void xhci_stall_ep(XHCITransfer *xfer)
 {
-XHCIState *xhci = xfer->xhci;
-XHCISlot *slot = >slots[xfer->slotid-1];
-XHCIEPContext *epctx = slot->eps[xfer->epid-1];
+XHCIEPContext *epctx = xfer->epctx;
+XHCIState *xhci = epctx->xhci;
 uint32_t err;
 XHCIStreamContext *sctx;
 
@@ -1880,7 +1876,7 @@ static int xhci_submit(XHCIState *xhci, XHCITransfer 
*xfer,
 
 static int xhci_setup_packet(XHCITransfer *xfer)
 {
-XHCIState *xhci = xfer->xhci;
+XHCIState *xhci = xfer->epctx->xhci;
 USBEndpoint *ep;
 int dir;
 
@@ -3490,7 +3486,7 @@ static void xhci_complete(USBPort *port, USBPacket 
*packet)
 return;
 }
 xhci_complete_packet(xfer);
-xhci_kick_ep(xfer->xhci, xfer->slotid, xfer->epid, xfer->streamid);
+xhci_kick_ep(xfer->epctx->xhci, xfer->slotid, xfer->epid, xfer->streamid);
 if (xfer->complete) {
 xhci_ep_free_xfer(xfer);
 }
-- 
1.8.3.1

[Qemu-devel] [PULL 01/11] xhci: limit the number of link trbs we are willing to process

[Gerd Hoffmann]

Needed to avoid we run in circles forever in case the guest builds
an endless loop with link trbs.

Reported-by: Li Qiang 
Tested-by: P J P 
Signed-off-by: Gerd Hoffmann 
Message-id: 1476096382-7981-1-git-send-email-kra...@redhat.com
---
 hw/usb/hcd-xhci.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 726435c..ee4fa48 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -54,6 +54,8 @@
  * to the specs when it gets them */
 #define ER_FULL_HACK
 
+#define TRB_LINK_LIMIT  4
+
 #define LEN_CAP 0x40
 #define LEN_OPER(0x400 + 0x10 * MAXPORTS)
 #define LEN_RUNTIME ((MAXINTRS + 1) * 0x20)
@@ -1000,6 +1002,7 @@ static TRBType xhci_ring_fetch(XHCIState *xhci, XHCIRing 
*ring, XHCITRB *trb,
dma_addr_t *addr)
 {
 PCIDevice *pci_dev = PCI_DEVICE(xhci);
+uint32_t link_cnt = 0;
 
 while (1) {
 TRBType type;
@@ -1026,6 +1029,9 @@ static TRBType xhci_ring_fetch(XHCIState *xhci, XHCIRing 
*ring, XHCITRB *trb,
 ring->dequeue += TRB_SIZE;
 return type;
 } else {
+if (++link_cnt > TRB_LINK_LIMIT) {
+return 0;
+}
 ring->dequeue = xhci_mask64(trb->parameter);
 if (trb->control & TRB_LK_TC) {
 ring->ccs = !ring->ccs;
@@ -1043,6 +1049,7 @@ static int xhci_ring_chain_length(XHCIState *xhci, const 
XHCIRing *ring)
 bool ccs = ring->ccs;
 /* hack to bundle together the two/three TDs that make a setup transfer */
 bool control_td_set = 0;
+uint32_t link_cnt = 0;
 
 while (1) {
 TRBType type;
@@ -1058,6 +1065,9 @@ static int xhci_ring_chain_length(XHCIState *xhci, const 
XHCIRing *ring)
 type = TRB_TYPE(trb);
 
 if (type == TR_LINK) {
+if (++link_cnt > TRB_LINK_LIMIT) {
+return -length;
+}
 dequeue = xhci_mask64(trb.parameter);
 if (trb.control & TRB_LK_TC) {
 ccs = !ccs;
-- 
1.8.3.1

[Qemu-devel] [PULL 11/11] usb-redir: allocate buffers before waking up the host adapter

[Gerd Hoffmann]

From: Hans de Goede 

Needed to make sure usb redirection is prepared to actually handle the
callback from the usb host adapter.  Without this interrupt endpoints
don't work on xhci.

Note: On ehci the usb_wakeup() call only schedules a BH for the actual
work, which hides this bug because the allocation happens before ehci
calls back even without this patch.

Signed-off-by: Hans de Goede 
Message-id: 1476096313-7730-1-git-send-email-kra...@redhat.com
Signed-off-by: Gerd Hoffmann 
---
 hw/usb/redirect.c | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 444672a..d4ca026 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -2036,18 +2036,22 @@ static void usbredir_interrupt_packet(void *priv, 
uint64_t id,
 }
 
 if (ep & USB_DIR_IN) {
+bool q_was_empty;
+
 if (dev->endpoint[EP2I(ep)].interrupt_started == 0) {
 DPRINTF("received int packet while not started ep %02X\n", ep);
 free(data);
 return;
 }
 
-if (QTAILQ_EMPTY(>endpoint[EP2I(ep)].bufpq)) {
-usb_wakeup(usb_ep_get(>dev, USB_TOKEN_IN, ep & 0x0f), 0);
-}
+q_was_empty = QTAILQ_EMPTY(>endpoint[EP2I(ep)].bufpq);
 
 /* bufp_alloc also adds the packet to the ep queue */
 bufp_alloc(dev, data, data_len, interrupt_packet->status, ep, data);
+
+if (q_was_empty) {
+usb_wakeup(usb_ep_get(>dev, USB_TOKEN_IN, ep & 0x0f), 0);
+}
 } else {
 /*
  * We report output interrupt packets as completed directly upon
-- 
1.8.3.1

[Qemu-devel] [PULL 00/11] usb patch queue

[Gerd Hoffmann]

  Hi,

This is the usb patch queue, again in bugfixing mode.  xhci transfers
are allocated dynamically now, the static allocations (and limit) didn't
play nicely with stream endpoints.  Some xhci cleanups along the way.
Some bugfixes picked up from the list.

please pull,
  Gerd

The following changes since commit 6b39b06339ee59559b31f860d4af635b046322df:

  build: Work around SIZE_MAX bug in OSX headers (2016-10-11 19:22:20 +0100)

are available in the git repository at:

  git://git.kraxel.org/qemu tags/pull-usb-20161012-1

for you to fetch changes up to d5c42857d6b0c35028897df8dfc3749eba6f6de3:

  usb-redir: allocate buffers before waking up the host adapter (2016-10-12 
14:37:24 +0200)


various usb bugfixes
some xhci cleanups


Gerd Hoffmann (9):
  xhci: limit the number of link trbs we are willing to process
  xhci: decouple EV_QUEUE from TD_QUEUE
  xhci: drop unused comp_xfer field
  xhci: use linked list for transfers
  xhci: drop XHCITransfer->xhci
  xhci: add & use xhci_kick_epctx()
  xhci: drop XHCITransfer->{slotid,epid}
  xhci: make xhci_epid_to_usbep accept XHCIEPContext
  usb: fix serial generator

Hans de Goede (1):
  usb-redir: allocate buffers before waking up the host adapter

Vijay Kumar B (1):
  usb: Fix incorrect default DMA offset.

 hw/usb/desc.c |  12 +--
 hw/usb/hcd-ohci.c |   2 +-
 hw/usb/hcd-xhci.c | 229 ++
 hw/usb/redirect.c |  10 ++-
 4 files changed, 142 insertions(+), 111 deletions(-)