[Qemu-devel] [PATCH] pl190: Fix off-by-one error in priority handling when reading VECTADDR

[Marc Bommert]

The "current" priority bit (1 << i) should also be set in s->prio_mask[i], if 
the interrupt is enabled. This will in turn cause the read operation of 
VECTADDR to return the correct vector of the pending interrupt.

---
hw/intc/pl190.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/intc/pl190.c b/hw/intc/pl190.c
index 55ea15d..0369da8 100644
--- a/hw/intc/pl190.c
+++ b/hw/intc/pl190.c
@@ -80,12 +80,12 @@ static void pl190_update_vectors(PL190State *s)
mask = 0;
for (i = 0; i < 16; i++)
{
- s->prio_mask[i] = mask;
if (s->vect_control[i] & 0x20)
{
n = s->vect_control[i] & 0x1f;
mask |= 1 << n;
}
+ s->prio_mask[i] = mask;
}
s->prio_mask[16] = mask;
pl190_update(s);
--
2.5.0

Re: [Qemu-devel] [PATCH 2/3] COLO-compare: Optimize colo_packet_compare_common

[Hailiang Zhang]

On 2017/2/27 15:34, Zhang Chen wrote:



On 02/27/2017 03:28 PM, Hailiang Zhang wrote:

On 2017/2/27 15:03, Zhang Chen wrote:



On 02/25/2017 02:58 PM, Hailiang Zhang wrote:

On 2017/2/25 11:32, Zhang Chen wrote:

Add offset args for colo_packet_compare_common, optimize
colo_packet_compare_icmp() and colo_packet_compare_udp()
just compare the IP payload.

Signed-off-by: Zhang Chen 
---
net/colo-compare.c | 28 +---
1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index e75f0ae..9853232 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -180,7 +180,7 @@ static int packet_enqueue(CompareState *s, int
mode)
 * return:0  means packet same
 *> 0 || < 0 means packet different
 */
-static int colo_packet_compare_common(Packet *ppkt, Packet *spkt)
+static int colo_packet_compare_common(Packet *ppkt, Packet *spkt,
int offset)
{
trace_colo_compare_ip_info(ppkt->size,
inet_ntoa(ppkt->ip->ip_src),
inet_ntoa(ppkt->ip->ip_dst), spkt->size,
@@ -188,7 +188,8 @@ static int colo_packet_compare_common(Packet
*ppkt, Packet *spkt)
inet_ntoa(spkt->ip->ip_dst));

if (ppkt->size == spkt->size) {
-return memcmp(ppkt->data, spkt->data, spkt->size);
+return memcmp(ppkt->data + offset, spkt->data + offset,
+  spkt->size - offset);
} else {
trace_colo_compare_main("Net packet size are not the
same");
return -1;
@@ -237,8 +238,7 @@ static int colo_packet_compare_tcp(Packet *spkt,
Packet *ppkt)
spkt->ip->ip_sum = ppkt->ip->ip_sum;
}

-res = memcmp(ppkt->data + ETH_HLEN, spkt->data + ETH_HLEN,
-(spkt->size - ETH_HLEN));
+res = colo_packet_compare_common(ppkt, spkt, ETH_HLEN);



For tcp packets check, why not ignore the ip headers, just like udp
packets check ?
Besides, here, can we compare the checksum stored in headers of tcp
and udp
before call colo_packet_compare_common(), which i think will improve
the comparing
performance.


That's another way to compare the packet suggested by Dr. David Alan
Gilbert,
It makes two packets IP header be same firstly, then compare all IP
packet.
This way can tell people why we ignore IP header explicitly in other
packets check like udp.
For performance, If we ignore the IP header that will reduce at least 20
byte not to be compared.
So, ignore ip header have better comparing performance.



OK, here, i think we can re-use the checksum value stored in headers
of tcp
or udp, comparing it first before comparing the complete payload of
tcp or udp
packets is another way to improve the performance, it is only 2 bytes.


No, The IP header's checksum are always different, Because the IP
header's ID field
are always different.



Not checksum in ip header， i mean checksum in tcp header or udp header.



Thanks
Zhang Chen



Thanks.


Thanks
Zhang Chen




Thanks.
Hailiang


if (res != 0 &&
trace_event_get_state(TRACE_COLO_COMPARE_MISCOMPARE)) {
trace_colo_compare_pkt_info_src(inet_ntoa(ppkt->ip->ip_src),
@@ -277,7 +277,14 @@ static int colo_packet_compare_udp(Packet *spkt,
Packet *ppkt)
return -1;
}

-ret = colo_packet_compare_common(ppkt, spkt);
+/*
+ * Because of ppkt and spkt are both in the same connection,
+ * The ppkt's src ip, dst ip, src port, dst port, ip_proto all
are
+ * same with spkt. In addition, IP header's Identification is a
random
+ * field, we can handle it in IP fragmentation function later.
+ * So we just compare the ip payload here.
+ */
+ret = colo_packet_compare_common(ppkt, spkt, network_length +
ETH_HLEN);

if (ret) {
trace_colo_compare_udp_miscompare("primary pkt size",
ppkt->size);
@@ -304,7 +311,14 @@ static int colo_packet_compare_icmp(Packet
*spkt, Packet *ppkt)
return -1;
}

-if (colo_packet_compare_common(ppkt, spkt)) {
+/*
+ * Because of ppkt and spkt are both in the same connection,
+ * The ppkt's src ip, dst ip, src port, dst port, ip_proto all
are
+ * same with spkt. In addition, IP header's Identification is a
random
+ * field, we can handle it in IP fragmentation function later.
+ * So we just compare the ip payload here.
+ */
+if (colo_packet_compare_common(ppkt, spkt, network_length +
ETH_HLEN)) {
trace_colo_compare_icmp_miscompare("primary pkt size",
ppkt->size);
qemu_hexdump((char *)ppkt->data, stderr, "colo-compare",
@@ -330,7 +344,7 @@ static int colo_packet_compare_other(Packet
*spkt, Packet *ppkt)
inet_ntoa(ppkt->ip->ip_dst), spkt->size,
inet_ntoa(spkt->ip->ip_src),
inet_ntoa(spkt->ip->ip_dst));
-return colo_packet_compare_common(ppkt, spkt);
+return colo_packet_compare_common(ppkt, spkt, 0);
}

static int colo_old_packet_check_one(Packet *pkt, int64_t
*check_time)






.







.

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 08/22] ppc/xics: use the QOM interface to resend irqs

[Cédric Le Goater]

  void icp_set_cppr(ICPState *ss, uint8_t cppr)
 @@ -262,7 +261,7 @@ void icp_set_cppr(ICPState *ss, uint8_t cppr)
  }
  } else {
  if (!XISR(ss)) {
 -icp_resend(ss);
 +icp_resend(XICS_INTERFACE(qdev_get_machine()), ss);
>>>
>>> Here you're assuming that the machine is the implementor of the xics
>>> interface, which is kinda ugly.  The ICP should have a pointer to the
>>> xics interface, which will eventually replace the pointer to the
>>> overall xics object it has now.
>>
>> yes. I will try improve that. I don't like those calls to 
>> qdev_get_machine()either. 
>>
>> There are done in a couple of places though, under spapr_cpu_core
>> to get XICS for instance.
> 
> Right, but I'm happier with it there, in code that's definitely
> associated with a particular machine, rather than in the xics code
> which is at least somewhat reusable.

I fixed that with the backlink on the XICSFabric. 

C.

Re: [Qemu-devel] [PULL 00/24] MTTCG Base enabling patches with ARM enablement

[Christian Borntraeger]

On 02/25/2017 10:14 PM, Peter Maydell wrote:
> On 24 February 2017 at 11:20, Alex Bennée  wrote:
>> The following changes since commit 2d896b454a0e19ec4c1ddbb0e0b65b7e54fcedf3:
>>
>>   Revert "hw/mips: MIPS Boston board support" (2017-02-23 18:04:45 +)
>>
>> are available in the git repository at:
>>
>>   https://github.com/stsquad/qemu.git tags/pull-mttcg-240217-1
>>
>> for you to fetch changes up to ca759f9e387db87e1719911f019bc60c74be9ed8:
>>
>>   tcg: enable MTTCG by default for ARM on x86 hosts (2017-02-24 10:32:46 
>> +)
>>
>> 
>> This is the MTTCG pull-request as posted yesterday.
>>
>> 
> 
> Applied, thanks.
> 
> -- PMM
> 

This seems to trigger 

/home/cborntra/REPOS/qemu/vl.c: In function ‘main’:
/home/cborntra/REPOS/qemu/vl.c:3700:18: error: ‘QEMU_OPTION_accel’ undeclared 
(first use in this function)
 case QEMU_OPTION_accel:
  ^
/home/cborntra/REPOS/qemu/vl.c:3700:18: note: each undeclared identifier is 
reported only once for each function it appears in


on s390.

Christian

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 04/22] ppc/xics: add an InterruptStatsProvider interface to ICS and ICP objects

[Cédric Le Goater]

On 02/27/2017 12:43 AM, David Gibson wrote:
> On Fri, Feb 24, 2017 at 11:52:01AM +0100, Cédric Le Goater wrote:
>> On 02/23/2017 03:15 AM, David Gibson wrote:
>>> On Thu, Feb 16, 2017 at 02:47:27PM +0100, Cédric Le Goater wrote:
 This is, again, to reduce the use of the list of ICS objects. Let's
 make each individual ICS and ICP object an InterruptStatsProvider and
 remove this same interface from XICSState.

 Signed-off-by: Cédric Le Goater 
>>>
>>> I'm a little hesitant about this, because it means that getting the
>>> interrupt stats information is now spread out over the qom tree,
>>> whereas previously there was a single location to get a good summary
>>> of the systems overall interrupt status.  The previous behaviour seems
>>> like it would be more convenient for debugging.
>>>
>>> That said, I see the structural advantages of this split.  Hmm.. still
>>> thinking..
>>
>> This is true. Another argument in favour of what you are saying 
>> is the order in which these are printed. See below.  
>>
>> What we could do after the cleanup is to make the machine an 
>> InterruptStatsProvider to clarify things.
> 
> Right.  So "info pic" does at least iterate through all the providers,
> but the semi-random order is pretty icky.  I think putting the stats
> provider on the machine would be a better idea - I guess it should be
> easy enough if the xics code provides a helper.

OK. This is on my TODO list for the next version of the patchset but as 
a followup patch. I don't want to change too much the initial cleanups
as he took me a while to find a working path to redo XICS. 

Thanks,

C.

Re: [Qemu-devel] [PATCH] target-s390x: Implement lpp instruction

[Miroslav Benes]

On Sat, 25 Feb 2017, Richard Henderson wrote:

> On 02/25/2017 12:50 AM, Miroslav Benes wrote:
> > Linux arch/s390/kernel/head(64).S uses lpp instruction if it is
> > available in facilities list provided by stfl/stfle instruction. This is
> > the case of newer z/System generations and their qemu definition.
> > 
> > Signed-off-by: Miroslav Benes 
> > ---
> 
> I can't find LPP in my PoO 11th edition...
> 
> > +static ExitStatus op_lpp(DisasContext *s, DisasOps *o)
> > +{
> > +check_privileged(s);
> > +potential_page_fault(s);
> > +
> > +tcg_gen_st_i64(o->in2, cpu_env, offsetof(CPUS390XState, pp));
> 
> But you don't need the potential_page_fault, since this is not a store to
> guest memory.

Right. I got confused by Access Program Exception in the spec. Sending v2.

Thanks for the review!

Miroslav

[Qemu-devel] [PATCH v2] target-s390x: Implement lpp instruction

[Miroslav Benes]

Linux arch/s390/kernel/head(64).S uses lpp instruction if it is
available in facilities list provided by stfl/stfle instruction. This is
the case of newer z/System generations and their qemu definition.

Signed-off-by: Miroslav Benes 
---
v2
- remove potential_page_fault()

 target/s390x/insn-data.def | 2 ++
 target/s390x/translate.c   | 9 +
 2 files changed, 11 insertions(+)

diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
index 075ff597c3de..880b0403cb5e 100644
--- a/target/s390x/insn-data.def
+++ b/target/s390x/insn-data.def
@@ -843,6 +843,8 @@
 /* LOAD CONTROL */
 C(0xb700, LCTL,RS_a,  Z,   0, a2, 0, 0, lctl, 0)
 C(0xeb2f, LCTLG,   RSY_a, Z,   0, a2, 0, 0, lctlg, 0)
+/* LOAD PROGRAM PARAMETER */
+C(0xb280, LPP, S,   LPP,   0, m2_64, 0, 0, lpp, 0)
 /* LOAD PSW */
 C(0x8200, LPSW,S, Z,   0, a2, 0, 0, lpsw, 0)
 /* LOAD PSW EXTENDED */
diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 01c62176bf70..3032d495dcf8 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -1194,6 +1194,7 @@ typedef enum DisasFacility {
 FAC_SCF,/* store clock fast */
 FAC_SFLE,   /* store facility list extended */
 FAC_ILA,/* interlocked access facility 1 */
+FAC_LPP,/* load-program-parameter */
 } DisasFacility;
 
 struct DisasInsn {
@@ -2567,6 +2568,14 @@ static ExitStatus op_lra(DisasContext *s, DisasOps *o)
 return NO_EXIT;
 }
 
+static ExitStatus op_lpp(DisasContext *s, DisasOps *o)
+{
+check_privileged(s);
+
+tcg_gen_st_i64(o->in2, cpu_env, offsetof(CPUS390XState, pp));
+return NO_EXIT;
+}
+
 static ExitStatus op_lpsw(DisasContext *s, DisasOps *o)
 {
 TCGv_i64 t1, t2;
-- 
2.11.0

Re: [Qemu-devel] [PATCH 2/3] COLO-compare: Optimize colo_packet_compare_common

[Zhang Chen]

On 02/27/2017 04:43 PM, Hailiang Zhang wrote:

On 2017/2/27 15:34, Zhang Chen wrote:



On 02/27/2017 03:28 PM, Hailiang Zhang wrote:

On 2017/2/27 15:03, Zhang Chen wrote:



On 02/25/2017 02:58 PM, Hailiang Zhang wrote:

On 2017/2/25 11:32, Zhang Chen wrote:

Add offset args for colo_packet_compare_common, optimize
colo_packet_compare_icmp() and colo_packet_compare_udp()
just compare the IP payload.

Signed-off-by: Zhang Chen 
---
net/colo-compare.c | 28 +---
1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index e75f0ae..9853232 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -180,7 +180,7 @@ static int packet_enqueue(CompareState *s, int
mode)
 * return:0  means packet same
 *> 0 || < 0 means packet different
 */
-static int colo_packet_compare_common(Packet *ppkt, Packet *spkt)
+static int colo_packet_compare_common(Packet *ppkt, Packet *spkt,
int offset)
{
trace_colo_compare_ip_info(ppkt->size,
inet_ntoa(ppkt->ip->ip_src),
inet_ntoa(ppkt->ip->ip_dst), spkt->size,
@@ -188,7 +188,8 @@ static int colo_packet_compare_common(Packet
*ppkt, Packet *spkt)
inet_ntoa(spkt->ip->ip_dst));

if (ppkt->size == spkt->size) {
-return memcmp(ppkt->data, spkt->data, spkt->size);
+return memcmp(ppkt->data + offset, spkt->data + offset,
+  spkt->size - offset);
} else {
trace_colo_compare_main("Net packet size are not the
same");
return -1;
@@ -237,8 +238,7 @@ static int colo_packet_compare_tcp(Packet *spkt,
Packet *ppkt)
spkt->ip->ip_sum = ppkt->ip->ip_sum;
}

-res = memcmp(ppkt->data + ETH_HLEN, spkt->data + ETH_HLEN,
-(spkt->size - ETH_HLEN));
+res = colo_packet_compare_common(ppkt, spkt, ETH_HLEN);



For tcp packets check, why not ignore the ip headers, just like udp
packets check ?
Besides, here, can we compare the checksum stored in headers of tcp
and udp
before call colo_packet_compare_common(), which i think will improve
the comparing
performance.


That's another way to compare the packet suggested by Dr. David Alan
Gilbert,
It makes two packets IP header be same firstly, then compare all IP
packet.
This way can tell people why we ignore IP header explicitly in other
packets check like udp.
For performance, If we ignore the IP header that will reduce at 
least 20

byte not to be compared.
So, ignore ip header have better comparing performance.



OK, here, i think we can re-use the checksum value stored in headers
of tcp
or udp, comparing it first before comparing the complete payload of
tcp or udp
packets is another way to improve the performance, it is only 2 bytes.


No, The IP header's checksum are always different, Because the IP
header's ID field
are always different.



Not checksum in ip header， i mean checksum in tcp header or udp header.


TCP header can do it, but UDP header's checksum is optional.

Thanks
Zhang Chen





Thanks
Zhang Chen



Thanks.


Thanks
Zhang Chen




Thanks.
Hailiang


if (res != 0 &&
trace_event_get_state(TRACE_COLO_COMPARE_MISCOMPARE)) {
trace_colo_compare_pkt_info_src(inet_ntoa(ppkt->ip->ip_src),
@@ -277,7 +277,14 @@ static int colo_packet_compare_udp(Packet 
*spkt,

Packet *ppkt)
return -1;
}

-ret = colo_packet_compare_common(ppkt, spkt);
+/*
+ * Because of ppkt and spkt are both in the same connection,
+ * The ppkt's src ip, dst ip, src port, dst port, ip_proto all
are
+ * same with spkt. In addition, IP header's Identification is a
random
+ * field, we can handle it in IP fragmentation function later.
+ * So we just compare the ip payload here.
+ */
+ret = colo_packet_compare_common(ppkt, spkt, network_length +
ETH_HLEN);

if (ret) {
trace_colo_compare_udp_miscompare("primary pkt size",
ppkt->size);
@@ -304,7 +311,14 @@ static int colo_packet_compare_icmp(Packet
*spkt, Packet *ppkt)
return -1;
}

-if (colo_packet_compare_common(ppkt, spkt)) {
+/*
+ * Because of ppkt and spkt are both in the same connection,
+ * The ppkt's src ip, dst ip, src port, dst port, ip_proto all
are
+ * same with spkt. In addition, IP header's Identification is a
random
+ * field, we can handle it in IP fragmentation function later.
+ * So we just compare the ip payload here.
+ */
+if (colo_packet_compare_common(ppkt, spkt, network_length +
ETH_HLEN)) {
trace_colo_compare_icmp_miscompare("primary pkt size",
ppkt->size);
qemu_hexdump((char *)ppkt->data, stderr, "colo-compare",
@@ -330,7 +344,7 @@ static int colo_packet_compare_other(Packet
*spkt, Packet *ppkt)
inet_ntoa(ppkt->ip->ip_dst), spkt->size,
inet_ntoa(spkt->ip->ip_src),
inet_ntoa(spkt->ip->ip_dst));
-return colo_packet_compare_common(ppkt, spkt);
+return colo_packet_compare_common(ppkt, spkt, 0);
}

stati

Re: [Qemu-devel] [PATCH v2 2/3] filter-rewriter: fix memory leak for connection in connection_track_table

[Jason Wang]

On 2017年02月27日 14:53, Hailiang Zhang wrote:

I think the issue is that your code can not differ A from B.



We have a parameter 'fin_ack_seq' recording the sequence of
'FIN=1,ACK=1,seq=w,ack=u+1' and if the ack value from the opposite
side is is 'w+1', we can consider this connection is closed, no ? 


Let's see what happens, consider VM is doing active close (reuse the 
figure above):


(VM)
Client:Server:

ESTABLISHED|   |
   | -> FIN=1,seq=u   ->   |

handle_secondary():
fin_ack_seq = u
tcp_state = TCPS_LAST_ACK

FIN_WAIT_1 |   |
   | <- ACK=1,seq=v,ack=u+1 <- |

handle_primary():
fin_ack_seq = ack + 1
g_hash_table_remove()

But we probably want it to be removed in TIME_WAIT_CLOSED.

Thanks

Re: [Qemu-devel] [PULL 0/5] slirp updates

[Dr. David Alan Gilbert]

* Peter Maydell (peter.mayd...@linaro.org) wrote:
> On 26 February 2017 at 20:27, Samuel Thibault
>  wrote:
> > The following changes since commit 685783c5b69c83c942d1fc21679311eeb8f79ab9:
> >
> >   Merge remote-tracking branch 'remotes/thibault/tags/samuel-thibault' into 
> > staging (2017-02-26 16:38:40 +)
> >
> > are available in the git repository at:
> >
> >   http://people.debian.org/~sthibault/qemu.git tags/samuel-thibault
> >
> > for you to fetch changes up to c363a5b7f9ca9e802665587900b7ea1aefcf26ea:
> >
> >   slirp: VMStatify remaining except for loop (2017-02-26 21:16:38 +0100)
> >
> > 
> > slirp updates
> >
> > 
> > Dr. David Alan Gilbert (5):
> >   slirp: VMState conversion; tcpcb
> >   slirp: VMStatify sbuf
> >   slirp: Common lhost/fhost union
> >   slirp: VMStatify socket level
> >   slirp: VMStatify remaining except for loop
> 
> I'm afraid this doesn't build on OSX:
> 
> 
> /Users/pm215/src/qemu-for-merges/slirp/slirp.c:1291:9: error:
> 'uint16_t *' (aka 'unsigned short *') and 'typeof (((union
> slirp_sockaddr *)0)->ss.ss_family) *' (aka 'unsigned char *') are not
> pointers to compatible types
> VMSTATE_SS_FAMILY(ss.ss_family, union slirp_sockaddr),
> ^

Yes, we need to drop 4 and 5;  as per the separate thread the problem
is the BSDs just have a char for their ss_family.

Dave

> /Users/pm215/src/qemu-for-merges/slirp/slirp.c:1277:33: note: expanded
> from macro 'VMSTATE_SS_FAMILY'
> #define VMSTATE_SS_FAMILY(f, s) VMSTATE_UINT16(f, s)
> ^~~~
> /Users/pm215/src/qemu-for-merges/include/migration/vmstate.h:785:5:
> note: expanded from macro 'VMSTATE_UINT16'
> VMSTATE_UINT16_V(_f, _s, 0)
> ^~~
> /Users/pm215/src/qemu-for-merges/include/migration/vmstate.h:764:5:
> note: expanded from macro 'VMSTATE_UINT16_V'
> VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint16, uint16_t)
> ^
> note: (skipping 1 expansions in backtrace; use
> -fmacro-backtrace-limit=0 to see all)
> /Users/pm215/src/qemu-for-merges/include/migration/vmstate.h:300:21:
> note: expanded from macro 'VMSTATE_SINGLE_TEST'
> .offset   = vmstate_offset_value(_state, _field, _type), \
> ^~~
> /Users/pm215/src/qemu-for-merges/include/migration/vmstate.h:272:6:
> note: expanded from macro 'vmstate_offset_value'
>  type_check(_type, typeof_field(_state, _field)))
>  ^~~
> /Users/pm215/src/qemu-for-merges/include/qemu/compiler.h:86:35: note:
> expanded from macro 'type_check'
> #define type_check(t1,t2) ((t1*)0 - (t2*)0)
>~~ ^ ~~
> 1 error generated.
> 
> In the OSX headers sockaddr_storage is:
> 
> struct sockaddr_storage {
> __uint8_t   ss_len; /* address length */
> sa_family_t ss_family;  /* [XSI] address family */
> char__ss_pad1[_SS_PAD1SIZE];
> __int64_t   __ss_align; /* force structure storage alignment 
> */
> char__ss_pad2[_SS_PAD2SIZE];
> };
> 
> and sa_family_t is
> 
> typedef __uint8_t   sa_family_t;
> 
> (NetBSD also defines sa_family_t as an 8 bit type, and
> perhaps so do the other BSDs.)
> 
> I think we can't get away with having the on-the-wire
> type for this field be the same as the in-memory
> representation, since the on-the-wire rep. should
> be host-OS-independent...
> 
> thanks
> -- PMM
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PULL 00/24] MTTCG Base enabling patches with ARM enablement

[Alex Bennée]

Christian Borntraeger  writes:

> On 02/25/2017 10:14 PM, Peter Maydell wrote:
>> On 24 February 2017 at 11:20, Alex Bennée  wrote:
>>> The following changes since commit 2d896b454a0e19ec4c1ddbb0e0b65b7e54fcedf3:
>>>
>>>   Revert "hw/mips: MIPS Boston board support" (2017-02-23 18:04:45 +)
>>>
>>> are available in the git repository at:
>>>
>>>   https://github.com/stsquad/qemu.git tags/pull-mttcg-240217-1
>>>
>>> for you to fetch changes up to ca759f9e387db87e1719911f019bc60c74be9ed8:
>>>
>>>   tcg: enable MTTCG by default for ARM on x86 hosts (2017-02-24 10:32:46 
>>> +)
>>>
>>> 
>>> This is the MTTCG pull-request as posted yesterday.
>>>
>>> 
>>
>> Applied, thanks.
>>
>> -- PMM
>>
>
> This seems to trigger
>
> /home/cborntra/REPOS/qemu/vl.c: In function ‘main’:
> /home/cborntra/REPOS/qemu/vl.c:3700:18: error: ‘QEMU_OPTION_accel’ undeclared 
> (first use in this function)
>  case QEMU_OPTION_accel:
>   ^
> /home/cborntra/REPOS/qemu/vl.c:3700:18: note: each undeclared identifier is 
> reported only once for each function it appears in
>
>
> on s390.

Is this for softmmu compilation? I'll have a look but I'll have to set
up some s390 images to test so you might beat me to it if you have real
hardware around.

--
Alex Bennée

Re: [Qemu-devel] [PATCH v8 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"

[Daniel P. Berrange]

On Fri, Feb 24, 2017 at 03:30:21PM -0800, ashish mittal wrote:
> Thanks!
> 
> I hope the following is in line with what you suggested -

Yes, that looks suitable for password auth

> 
> We will error out in case either of username, secret-id, or password
> are missing.
> 
> Good case, passing password via a file -
> $ ./qemu-io --trace enable=vxhs* --object
> secret,id=xvxhspasswd,file=/tmp/some/file/path  -c 'read 66000 128k'
> 'json:{"server.host": "127.0.0.1", "server.port": "", "vdisk-id":
> "/test.raw", "driver": "vxhs", "user": "ashish",  "password-secret":
> "xvxhspasswd"}'
> 1132@1487977829.151064:vxhs_open_vdiskid Opening vdisk-id /test.raw
> 
> 1132@1487977829.151141:vxhs_get_creds User ashish, SecretID
> xvxhspasswd, Password Str0ngP@ssw0rd   <===   NOTE WILL NOT PRINT
> PASSWORD IN FINAL CODE 
> 
> 1132@1487977829.151168:vxhs_open_hostinfo Adding host 127.0.0.1:
> to BDRVVXHSState
> 1132@1487977829.173062:vxhs_get_vdisk_stat vDisk /test.raw stat ioctl
> returned size 196616
> read 131072/131072 bytes at offset 66000
> 128 KiB, 1 ops; 0.0012 sec (99.049 MiB/sec and 792.3930 ops/sec)
> 1132@1487977829.175141:vxhs_close Closing vdisk /test.raw
> 
> 
> Bad case, missing user -
> $ ./qemu-io --trace enable=vxhs* --object
> secret,id=xvxhspasswd,data=/tmp/some/file/path  -c 'read 66000 128k'
> 'json:{"server.host": "127.0.0.1", "server.port": "", "vdisk-id":
> "/test.raw", "driver": "vxhs"}'
> 1310@1487978547.771234:vxhs_open_vdiskid Opening vdisk-id /test.raw
> can't open device json:{"server.host": "127.0.0.1", "server.port":
> "", "vdisk-id": "/test.raw", "driver": "vxhs"}: please specify the
> user for authenticating to target
> 
> diff --git a/block/vxhs.c b/block/vxhs.c
> index 4f0633e..9b60ddf 100644
> --- a/block/vxhs.c
> +++ b/block/vxhs.c
> @@ -17,12 +17,16 @@
>  #include "qemu/uri.h"
>  #include "qapi/error.h"
>  #include "qemu/uuid.h"
> +#include "crypto/secret.h"
> 
>  #define VXHS_OPT_FILENAME   "filename"
>  #define VXHS_OPT_VDISK_ID   "vdisk-id"
>  #define VXHS_OPT_SERVER "server"
>  #define VXHS_OPT_HOST   "host"
>  #define VXHS_OPT_PORT   "port"
> +#define VXHS_OPT_USER   "user"
> +#define VXHS_OPT_PASSWORD   "password"
> +#define VXHS_OPT_SECRETID   "password-secret"
>  #define VXHS_UUID_DEF "12345678-1234-1234-1234-123456789012"
> 
>  QemuUUID qemu_uuid __attribute__ ((weak));
> @@ -136,6 +140,22 @@ static QemuOptsList runtime_opts = {
>  .type = QEMU_OPT_STRING,
>  .help = "UUID of the VxHS vdisk",
>  },
> +{
> +.name = VXHS_OPT_USER,
> +.type = QEMU_OPT_STRING,
> +.help = "username for authentication to target",
> +},
> +{
> +.name = VXHS_OPT_PASSWORD,
> +.type = QEMU_OPT_STRING,
> +.help = "password for authentication to target",
> +},
> +{
> +.name = VXHS_OPT_SECRETID,
> +.type = QEMU_OPT_STRING,
> +.help = "ID of the secret providing password for"
> +"authentication to target",
> +},
>  { /* end of list */ }
>  },
>  };
> @@ -257,6 +277,9 @@ static int vxhs_open(BlockDriverState *bs, QDict *options,
>  const char *server_host_opt;
>  char *str = NULL;
>  int ret = 0;
> +const char *user = NULL;
> +const char *secretid = NULL;
> +const char *password = NULL;
> 
>  ret = vxhs_init_and_ref();
>  if (ret < 0) {
> @@ -320,6 +343,35 @@ static int vxhs_open(BlockDriverState *bs, QDict 
> *options,
>  goto out;
>  }
> 
> +/* check if we got username and secretid via the options */
> +user = qemu_opt_get(opts, VXHS_OPT_USER);
> +if (!user) {
> +error_setg(&local_err, "please specify the user for authenticating 
> to "
> +   "target");
> +qdict_del(backing_options, str);

Not sure why you're deleting this ? Likewise the 2 cases below too

> +ret = -EINVAL;
> +goto out;
> +}
> +
> +secretid = qemu_opt_get(opts, VXHS_OPT_SECRETID);
> +if (!secretid) {
> +error_setg(&local_err, "please specify the ID of the secret to be "
> +   "used for authenticating to target");
> +qdict_del(backing_options, str);
> +ret = -EINVAL;
> +goto out;
> +}
> +
> +/* check if we got password via the --object argument */
> +password = qcrypto_secret_lookup_as_utf8(secretid, &local_err);
> +if (local_err != NULL) {
> +trace_vxhs_get_creds(user, secretid, password);
> +qdict_del(backing_options, str);
> +ret = -EINVAL;
> +goto out;
> +}
> +trace_vxhs_get_creds(user, secretid, password);
> +
>  s->vdisk_hostinfo.host = g_strdup(server_host_opt);
> 
>  s->vdisk_hostinfo.port = g_ascii_strtoll(qemu_opt_get(tcp_opts,

Regards,
Daniel
-- 
|: http://berrange.com  -o-htt

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 16/22] ppc/xics: register the reset handler of ICP objects

[Cédric Le Goater]

[ adding Peter for some insights ] 

On 02/27/2017 02:00 AM, David Gibson wrote:
> On Fri, Feb 24, 2017 at 12:27:35PM +0100, Cédric Le Goater wrote:
>> On 02/23/2017 03:42 AM, David Gibson wrote:
>>> On Thu, Feb 16, 2017 at 02:47:39PM +0100, Cédric Le Goater wrote:
 The reset of the ICP objects is currently handled by XICS but this can
 be done for each individual ICP.

 Signed-off-by: Cédric Le Goater 
>>>
>>> Hrm.  I think whether device_reset() gets called automatically depends
>>> on how the device is wired into the composition tree, and I'm not sure
>>> the icps are in the right place for it to work.
>>
>> reset gets called only if it under sysbus or if you have registered 
>> a reset_handler. previously, XICS was a sysbus object so 
>> xics_common_reset() was getting called automatically.
> 
> Right.  Hmm.  So I think artificially placing the ics under the sysbus
> is not the right way to get the reset called - I think explicitly
> registering a reset handler is better.
> 
> The only thing that concerns me about that is that the MMIO ICS
> variants we'll want for powernv really _do_ have a bus presence,
> either on sysbus or some descendent, so that will get its reset called
> automatically.  I'm not sure what the best way to ensure the reset
> gets called exactly once in all cases.

Well, I am not sure either. I have reproduced this pattern as it is 
frequently used under ARM which has quite a few QOM'ified machines.

Thanks,

C. 

>>> This doesn't replace the code in xics_common_reset() so if it does
>>> work it means we must have previously been resetting the ICPs twice.
>>> Is that right?
>>
>> no. but there has been some confusion with the recent changes
>> on XICS.
>>
>> What replace the code in xics_common_reset() is :
>>
>>  qdev_set_parent_bus(DEVICE(icp), sysbus_get_default());
>>
>> That's how the reset handlers get called from QOM objects.
> 
> Right, I saw that later on.
> 
>>
>> C.
>>
 ---
  hw/intc/xics.c | 18 --
  hw/ppc/spapr.c |  1 +
  2 files changed, 1 insertion(+), 18 deletions(-)

 diff --git a/hw/intc/xics.c b/hw/intc/xics.c
 index dd41340d41a5..3ad7e8cf8ec4 100644
 --- a/hw/intc/xics.c
 +++ b/hw/intc/xics.c
 @@ -137,29 +137,11 @@ static void 
 ics_simple_pic_print_info(InterruptStatsProvider *obj,
  /*
   * XICS Common class - parent for emulated XICS and KVM-XICS
   */
 -static void xics_common_reset(DeviceState *d)
 -{
 -XICSState *xics = XICS_COMMON(d);
 -int i;
 -
 -for (i = 0; i < xics->nr_servers; i++) {
 -device_reset(DEVICE(&xics->ss[i]));
 -}
 -}
 -
 -static void xics_common_class_init(ObjectClass *oc, void *data)
 -{
 -DeviceClass *dc = DEVICE_CLASS(oc);
 -
 -dc->reset = xics_common_reset;
 -}
 -
  static const TypeInfo xics_common_info = {
  .name  = TYPE_XICS_COMMON,
  .parent= TYPE_DEVICE,
  .instance_size = sizeof(XICSState),
  .class_size= sizeof(XICSStateClass),
 -.class_init= xics_common_class_init,
  };
  
  /*
 diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
 index 9c1772f93155..445d9a6ddad4 100644
 --- a/hw/ppc/spapr.c
 +++ b/hw/ppc/spapr.c
 @@ -130,6 +130,7 @@ static XICSState *try_create_xics(sPAPRMachineState 
 *spapr,
  ICPState *icp = &xics->ss[i];
  
  object_initialize(icp, sizeof(*icp), type_icp);
 +qdev_set_parent_bus(DEVICE(icp), sysbus_get_default());
  object_property_add_child(OBJECT(xics), "icp[*]", OBJECT(icp), 
 NULL);
  object_property_add_const_link(OBJECT(icp), "xics", OBJECT(xics), 
 NULL);
  object_property_set_bool(OBJECT(icp), true, "realized", &err);
>>>
>>
> 

Re: [Qemu-devel] [PULL 00/24] MTTCG Base enabling patches with ARM enablement

[Christian Borntraeger]

On 02/27/2017 10:11 AM, Alex Bennée wrote:
> 
> Christian Borntraeger  writes:
> 
>> On 02/25/2017 10:14 PM, Peter Maydell wrote:
>>> On 24 February 2017 at 11:20, Alex Bennée  wrote:
 The following changes since commit 
 2d896b454a0e19ec4c1ddbb0e0b65b7e54fcedf3:

   Revert "hw/mips: MIPS Boston board support" (2017-02-23 18:04:45 +)

 are available in the git repository at:

   https://github.com/stsquad/qemu.git tags/pull-mttcg-240217-1

 for you to fetch changes up to ca759f9e387db87e1719911f019bc60c74be9ed8:

   tcg: enable MTTCG by default for ARM on x86 hosts (2017-02-24 10:32:46 
 +)

 
 This is the MTTCG pull-request as posted yesterday.

 
>>>
>>> Applied, thanks.
>>>
>>> -- PMM
>>>
>>
>> This seems to trigger
>>
>> /home/cborntra/REPOS/qemu/vl.c: In function ‘main’:
>> /home/cborntra/REPOS/qemu/vl.c:3700:18: error: ‘QEMU_OPTION_accel’ 
>> undeclared (first use in this function)
>>  case QEMU_OPTION_accel:
>>   ^
>> /home/cborntra/REPOS/qemu/vl.c:3700:18: note: each undeclared identifier is 
>> reported only once for each function it appears in
>>
>>
>> on s390.
> 
> Is this for softmmu compilation? I'll have a look but I'll have to set
> up some s390 images to test so you might beat me to it if you have real
> hardware around.

Yes, softmmu. I do not yet understand why it is failing Still looking 8-| 

Re: [Qemu-devel] [PATCH] os: don't corrupt pre-existing memory-backend data with prealloc

[Daniel P. Berrange]

On Fri, Feb 24, 2017 at 09:33:05AM -0800, no-re...@patchew.org wrote:
> === OUTPUT BEGIN ===
> Checking PATCH 1/1: os: don't corrupt pre-existing memory-backend data with 
> prealloc...
> ERROR: Use of volatile is usually wrong: see 
> Documentation/volatile-considered-harmful.txt

ERROR: checkpatch.pl is usually wrong ;-P

Heh, it is refering to a doc in the kernel source tree, which does not even
exist at that path location anymore :-)


> #42: FILE: util/oslib-posix.c:370:
> +volatile char val = *(area + (hpagesize * i));
> 
> total: 1 errors, 0 warnings, 21 lines checked
> 
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> === OUTPUT END ===

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

Re: [Qemu-devel] [PATCH 2/4] block/rbd: code movement

[Daniel P. Berrange]

Describing this as "code movement" when the added & removed chunks are not
identical is a bit misleading.

Can you expand the commit message to explain why the extra options are
being added

On Mon, Feb 27, 2017 at 02:30:39AM -0500, Jeff Cody wrote:
> Signed-off-by: Jeff Cody 
> ---
>  block/rbd.c | 64 
> +++--
>  1 file changed, 45 insertions(+), 19 deletions(-)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index 3f1a9de..c8d4eb1 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -357,6 +357,51 @@ static void qemu_rbd_memset(RADOSCB *rcb, int64_t offs)
>  }
>  }
>  
> +static QemuOptsList runtime_opts = {
> +.name = "rbd",
> +.head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
> +.desc = {
> +{
> +.name = "filename",
> +.type = QEMU_OPT_STRING,
> +.help = "Specification of the rbd image",
> +},
> +{
> +.name = "password-secret",
> +.type = QEMU_OPT_STRING,
> +.help = "ID of secret providing the password",
> +},
> +{
> +.name = "conf",
> +.type = QEMU_OPT_STRING,
> +},
> +{
> +.name = "pool",
> +.type = QEMU_OPT_STRING,
> +},
> +{
> +.name = "image",
> +.type = QEMU_OPT_STRING,
> +},
> +{
> +.name = "snapshot",
> +.type = QEMU_OPT_STRING,
> +},
> +{
> +/* you might be tempted to call this 'id' to match
> + * the ceph documentation, but then it'll get gobbled
> + * up in the block layer before it gets to the image driver */
> +.name = "rbd-id",
> +.type = QEMU_OPT_STRING,
> +},
> +{
> +.name = "keyvalue-pairs",
> +.type = QEMU_OPT_STRING,
> +},
> +{ /* end of list */ }
> +},
> +};
> +
>  static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error 
> **errp)
>  {
>  Error *local_err = NULL;
> @@ -500,25 +545,6 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
>  qemu_aio_unref(acb);
>  }
>  
> -/* TODO Convert to fine grained options */
> -static QemuOptsList runtime_opts = {
> -.name = "rbd",
> -.head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
> -.desc = {
> -{
> -.name = "filename",
> -.type = QEMU_OPT_STRING,
> -.help = "Specification of the rbd image",
> -},
> -{
> -.name = "password-secret",
> -.type = QEMU_OPT_STRING,
> -.help = "ID of secret providing the password",
> -},
> -{ /* end of list */ }
> -},
> -};
> -
>  static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>   Error **errp)
>  {
> -- 
> 2.9.3
> 

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

Re: [Qemu-devel] exec: Respect as_tranlsate_internal length clamp

[Paolo Bonzini]

On 27/02/2017 06:53, Alexey Kardashevskiy wrote:
> There is a link to this one [1]:
> http://git.qemu-project.org/?p=qemu.git;a=commitdiff;h=c3c1bb99d1c11978d9ce94d1bdcf0705378c1459
> 
> Which was reverted with a sensible explanation [2]:
> http://git.qemu-project.org/?p=qemu.git;a=commitdiff;h=4025446f0ac6213335c22ec43f3c3d8362ce7286
> 
> However it is still in the tree as [3]:
> http://git.qemu-project.org/?p=qemu.git;a=commitdiff;h=23820dbfc79d1c9dce090b4c555994f2bb6a69b3
> 
> The only difference between [1] and [3] is a fixed typo in the subject,
> other than that they are identical.
> 
> Is not the explanation from [2] correct any more and [3] is a correct final
> fix? Or [3] should not be in the tree at all?

The explanation from [2] is correct, it caused "problems with boards
that declare memory regions shorter than the registers they contain" and
those were fixed.  Unfortunately I remember that the fixes were complex,
IIRC covering mostly ioport.c and Xen.  It might be fine for pSeries though.

Paolo

Re: [Qemu-devel] [PATCH 4/4] block/rbd: Add blockdev-add support

[Daniel P. Berrange]

On Mon, Feb 27, 2017 at 02:36:13AM -0500, Jeff Cody wrote:
> On Mon, Feb 27, 2017 at 02:30:41AM -0500, Jeff Cody wrote:
> > Signed-off-by: Jeff Cody 
> > ---
> >  qapi/block-core.json | 47 ---
> >  1 file changed, 44 insertions(+), 3 deletions(-)
> > 
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 5f82d35..08a1419 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -2111,6 +2111,7 @@
> >  # @replication: Since 2.8
> >  # @ssh: Since 2.8
> >  # @iscsi: Since 2.9
> > +# @rbd: Since 2.9
> >  #
> >  # Since: 2.0
> >  ##
> > @@ -2120,7 +2121,7 @@
> >  'host_device', 'http', 'https', 'iscsi', 'luks', 'nbd', 'nfs',
> >  'null-aio', 'null-co', 'parallels', 'qcow', 'qcow2', 'qed',
> >  'quorum', 'raw', 'replication', 'ssh', 'vdi', 'vhdx', 'vmdk',
> > -'vpc', 'vvfat' ] }
> > +'vpc', 'vvfat', 'rbd' ] }
> >  
> >  ##
> >  # @BlockdevOptionsFile:
> > @@ -2376,7 +2377,6 @@
> >  'path': 'str',
> >  '*user': 'str' } }
> >  
> > -
> >  ##
> >  # @BlkdebugEvent:
> >  #
> > @@ -2666,6 +2666,47 @@
> >  '*timeout': 'int' } }
> >  
> >  ##
> > +# @BlockdevOptionsRbd:
> > +#
> > +# @pool:   Ceph pool name
> > +#
> > +# @image:  Image name in the Ceph pool
> > +#
> > +# @conf:   # optional path to Ceph configuration file.  Values
> > +#  in the configuration file will be overridden by
> > +#  options specified via QAPI.
> > +#
> > +# @snapshot:   #optional Ceph snapshot name
> > +#
> > +# @rbd-id: #optional Ceph id name
> > +#
> > +# @password-secret:#optional The ID of a QCryptoSecret object providing
> > +#   the password for the login.
> > +#
> > +# @keyvalue-pairs: #optional  string containing key/value pairs for
> > +#  additional Ceph configuration, not including "id" 
> > or "conf"
> > +#  options. This can be used to specify any of the 
> > options
> > +#  that Ceph supports.  The format is of the form:
> > +#   key1=value1:key2=value2:[...]
> > +#
> > +#  Special characters such as ":" and "=" can be 
> > escaped
> > +#  with a '\' character, which means the QAPI needs an
> > +#  extra '\' character to pass the needed escape 
> > character.
> > +#  For example:
> > +#"keyvalue-pairs": "mon_host=127.0.0.1\\:6321"
> > +#
> 
> This is the key / value pair issue mentioned in the cover letter.  Encoding
> all the options as a string like this is ugly.  What is the preference on
> how to handle these via QAPI, when the actual key and value pairs could be
> anything?   Talking with Markus on IRC, one option he mentioned was an array
> of a generic struct of 'key' and 'value' pairs.
> 
> Do the libvirt folks have any interface preferences here?

IMHO, we should formally model each option that we need to be able to provide
and *not* provide any generic passthrough feature in QAPI.

Particularly for the server hostname/port, we should have the same QAPI
modelling approach that we did for other network protocols.


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

Re: [Qemu-devel] [PULL 00/24] MTTCG Base enabling patches with ARM enablement

[Christian Borntraeger]

On 02/27/2017 10:11 AM, Alex Bennée wrote:
> 
> Christian Borntraeger  writes:
> 
>> On 02/25/2017 10:14 PM, Peter Maydell wrote:
>>> On 24 February 2017 at 11:20, Alex Bennée  wrote:
 The following changes since commit 
 2d896b454a0e19ec4c1ddbb0e0b65b7e54fcedf3:

   Revert "hw/mips: MIPS Boston board support" (2017-02-23 18:04:45 +)

 are available in the git repository at:

   https://github.com/stsquad/qemu.git tags/pull-mttcg-240217-1

 for you to fetch changes up to ca759f9e387db87e1719911f019bc60c74be9ed8:

   tcg: enable MTTCG by default for ARM on x86 hosts (2017-02-24 10:32:46 
 +)

 
 This is the MTTCG pull-request as posted yesterday.

 
>>>
>>> Applied, thanks.
>>>
>>> -- PMM
>>>
>>
>> This seems to trigger
>>
>> /home/cborntra/REPOS/qemu/vl.c: In function ‘main’:
>> /home/cborntra/REPOS/qemu/vl.c:3700:18: error: ‘QEMU_OPTION_accel’ 
>> undeclared (first use in this function)
>>  case QEMU_OPTION_accel:
>>   ^
>> /home/cborntra/REPOS/qemu/vl.c:3700:18: note: each undeclared identifier is 
>> reported only once for each function it appears in
>>
>>
>> on s390.
> 
> Is this for softmmu compilation? I'll have a look but I'll have to set
> up some s390 images to test so you might beat me to it if you have real
> hardware around.

Ok, my fault. I seem to have run make in the code folder somewhen in the past,
which created an qemu-options.def file in the source folder. When doing the 
rebuild in my build folder, it used qemu-options.def from the source folder
and not from the build folder.

With a clean restart everything seems fine

Christian

[Qemu-devel] [PATCH] Removed support for depth!=32

[Suramya Shah]

Signed-off-by: Suramya Shah 
---
 hw/display/sm501_template.h | 8 +---
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/hw/display/sm501_template.h b/hw/display/sm501_template.h
index f33e499..4e5801e 100644
--- a/hw/display/sm501_template.h
+++ b/hw/display/sm501_template.h
@@ -22,13 +22,7 @@
  * THE SOFTWARE.
  */
 
-#if DEPTH == 8
-#define BPP 1
-#define PIXEL_TYPE uint8_t
-#elif DEPTH == 15 || DEPTH == 16
-#define BPP 2
-#define PIXEL_TYPE uint16_t
-#elif DEPTH == 32
+#if DEPTH == 32
 #define BPP 4
 #define PIXEL_TYPE uint32_t
 #else
-- 
2.9.3

[Qemu-devel] Fail to start 2nd guest

[Xiong Zhou]

Hi,

It worked fine on Linus tree commit:
  7bb0338 Merge tag 'rodata-v4.11-rc1' of git://git.kernel.org/pub/scm/..

failed to start 2nd domain on this commit:
37c8596 Merge tag 'tty-4.11-rc1' of git://git.kernel.org/pub/scm/..
(this commit probably is not the first bad, i didn't do the bisecting)


sh-4.2# uname -r
4.10.0-master-37c8596+
sh-4.2# rpm -qv qemu
qemu-2.0.0-1.el7.6.x86_64
sh-4.2# ps ajxf | grep qemu
11441 11462 11461 10972 pts/011461 S+   0   0:00  \_ grep 
qemu
1 11065 11064 11064 ?   -1 Sl 107   2:03 /usr/libexec/qemu-kvm 
-name 73h -S -machine pc-i440fx-rhel7.0.0,accel=kvm,usb=off,dump-guest-core=off 
-cpu Haswell,-hle,-rtm -m 8000 -realtime mlock=off -smp 
8,sockets=8,cores=1,threads=1 -uuid aab09e0f-d48a-4223-9b81-35d4718d59fc 
-no-user-config -nodefaults -chardev 
socket,id=charmonitor,path=/var/lib/libvirt/qemu/domain-1-73h/monitor.sock,server,nowait
 -mon chardev=charmonitor,id=monitor,mode=control -rtc base=utc,driftfix=slew 
-global kvm-pit.lost_tick_policy=delay -no-hpet -no-shutdown -global 
PIIX4_PM.disable_s3=1 -global PIIX4_PM.disable_s4=1 -boot strict=on -device 
ich9-usb-ehci1,id=usb,bus=pci.0,addr=0x6.0x7 -device 
ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,addr=0x6 
-device ich9-usb-uhci2,masterbus=usb.0,firstport=2,bus=pci.0,addr=0x6.0x1 
-device ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pci.0,addr=0x6.0x2 
-device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x5 -drive 
file=/var/lib/libvirt/images/rhel73.qcow2,format=qcow2,if=none,id=drive-ide0-0-0
 -device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 
-drive 
file=/var/lib/libvirt/images/73h.qcow2,format=qcow2,if=none,id=drive-ide0-0-1 
-device ide-hd,bus=ide.0,unit=1,drive=drive-ide0-0-1,id=ide0-0-1 -drive 
file=/root/RHEL-7.2-20151030.0-Server-x86_64-dvd1.iso,format=raw,if=none,id=drive-ide0-1-0,readonly=on
 -device ide-cd,bus=ide.1,unit=0,drive=drive-ide0-1-0,id=ide0-1-0,bootindex=2 
-netdev tap,fd=25,id=hostnet0 -device 
rtl8139,netdev=hostnet0,id=net0,mac=52:54:00:83:12:7b,bus=pci.0,addr=0x3 
-chardev pty,id=charserial0 -device isa-serial,chardev=charserial0,id=serial0 
-chardev spicevmc,id=charchannel0,name=vdagent -device 
virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,id=channel0,name=com.redhat.spice.0
 -spice 
port=5900,addr=127.0.0.1,disable-ticketing,image-compression=off,seamless-migration=on
 -vga qxl -global qxl-vga.ram_size=67108864 -global qxl-vga.vram_size=67108864 
-global qxl-vga.vgamem_mb=16 -device intel-hda,id=sound0,bus=pci.0,addr=0x4 
-device hda-duplex,id=sound0-codec0,bus=sound0.0,cad=0 -chardev 
spicevmc,id=charredir0,name=usbredir -device 
usb-redir,chardev=charredir0,id=redir0,bus=usb.0,port=1 -chardev 
spicevmc,id=charredir1,name=usbredir -device 
usb-redir,chardev=charredir1,id=redir1,bus=usb.0,port=2 -device 
virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x7 -msg timestamp=on
sh-4.2# virsh list
 IdName   State

 1 73hrunning

sh-4.2# virsh start 73us
error: Failed to start domain 73us
error: internal error: qemu unexpectedly closed the monitor: ((null):11497): 
Spice-Warning **: reds.c:2499:reds_init_socket: listen: Address already in use
2017-02-27T09:33:42.335708Z qemu-kvm: failed to initialize spice server

sh-4.2# 

Re: [Qemu-devel] [PATCH 3/4] savevm: fix savevm after migration

[Denis V. Lunev]

On 02/25/2017 10:31 PM, Vladimir Sementsov-Ogievskiy wrote:
> After migration all drives are inactive and savevm will fail with
>
> qemu-kvm: block/io.c:1406: bdrv_co_do_pwritev:
>Assertion `!(bs->open_flags & 0x0800)' failed.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/snapshot.c   |  3 ++-
>  migration/savevm.c | 11 +++
>  2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/block/snapshot.c b/block/snapshot.c
> index bf5c2ca5e1..256d06ac9f 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -145,7 +145,8 @@ bool bdrv_snapshot_find_by_id_and_name(BlockDriverState 
> *bs,
>  int bdrv_can_snapshot(BlockDriverState *bs)
>  {
>  BlockDriver *drv = bs->drv;
> -if (!drv || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
> +if (!drv || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs) ||
> +(bs->open_flags & BDRV_O_INACTIVE)) {
>  return 0;
>  }
>  
at my opinion we do not need this hunk. It will result in a wrong
thing.


> diff --git a/migration/savevm.c b/migration/savevm.c
> index 5ecd264134..75e56d2d07 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2068,6 +2068,17 @@ int save_vmstate(Monitor *mon, const char *name)
>  Error *local_err = NULL;
>  AioContext *aio_context;
>  
> +if (runstate_check(RUN_STATE_FINISH_MIGRATE) ||
> +runstate_check(RUN_STATE_POSTMIGRATE) ||
> +runstate_check(RUN_STATE_PRELAUNCH))
> +{
> +bdrv_invalidate_cache_all(&local_err);
> +if (local_err) {
> +error_report_err(local_err);
> +return -EINVAL;
> +}
> +}
> +
>  if (!bdrv_all_can_snapshot(&bs)) {
>  monitor_printf(mon, "Device '%s' is writable but does not "
> "support snapshots.\n", bdrv_get_device_name(bs));

Re: [Qemu-devel] [PULL 0/2] sun4v queue

[Peter Maydell]

On 26 February 2017 at 22:29, Artyom Tarasenko  wrote:
> The following changes since commit a951316b8a5c3c63254f20a826afeed940dd4cba:
>
>   Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request' 
> into staging (2017-02-03 14:41:49 +)
>
> are available in the git repository at:
>
>   https://github.com/artyom-tarasenko/qemu tags/pull-sun4v-20170226
>
> for you to fetch changes up to a5a08302d44a8b1a8c5819b1411002f85bb5f847:
>
>   niagara: check if a serial port is available (2017-02-26 22:46:08 +0100)
>
> 
> Pull request for Niagara patches 2017 02 26
>
> 
> Artyom Tarasenko (2):
>   niagara: fail if a firmware file is missing
>   niagara: check if a serial port is available
>
>  hw/sparc64/niagara.c | 33 +++--
>  1 file changed, 23 insertions(+), 10 deletions(-)

Applied, thanks.

-- PMM

Re: [Qemu-devel] [PATCH] vhost-user: delay vhost_user_stop

[Dr. David Alan Gilbert]

* Marc-André Lureau (marcandre.lur...@redhat.com) wrote:
> Since commit b0a335e351103bf92f3f9d0bd5759311be8156ac, a socket write
> may trigger a disconnect events, calling vhost_user_stop() and clearing
> all the vhost_dev strutures holding data that vhost.c functions expect
> to remain valid. Delay the cleanup to keep the vhost_dev structure
> valid during the vhost.c functions.
> 
> Signed-off-by: Marc-André Lureau 

This does get me through a 'make check' succesfully.

Dave

> ---
>  net/vhost-user.c | 49 +++--
>  1 file changed, 43 insertions(+), 6 deletions(-)
> 
> diff --git a/net/vhost-user.c b/net/vhost-user.c
> index 77b8110f8c..00573c8ac8 100644
> --- a/net/vhost-user.c
> +++ b/net/vhost-user.c
> @@ -25,6 +25,7 @@ typedef struct VhostUserState {
>  guint watch;
>  uint64_t acked_features;
>  bool started;
> +QEMUBH *chr_closed_bh;
>  } VhostUserState;
>  
>  VHostNetState *vhost_user_get_vhost_net(NetClientState *nc)
> @@ -190,9 +191,40 @@ static gboolean net_vhost_user_watch(GIOChannel *chan, 
> GIOCondition cond,
>  
>  qemu_chr_fe_disconnect(&s->chr);
>  
> +s->watch = 0;
>  return FALSE;
>  }
>  
> +static void chr_closed_bh(void *opaque)
> +{
> +const char *name = opaque;
> +NetClientState *ncs[MAX_QUEUE_NUM];
> +VhostUserState *s;
> +Error *err = NULL;
> +int queues;
> +
> +queues = qemu_find_net_clients_except(name, ncs,
> +  NET_CLIENT_DRIVER_NIC,
> +  MAX_QUEUE_NUM);
> +assert(queues < MAX_QUEUE_NUM);
> +
> +s = DO_UPCAST(VhostUserState, nc, ncs[0]);
> +
> +qmp_set_link(name, false, &err);
> +vhost_user_stop(queues, ncs);
> +if (s->watch) {
> +g_source_remove(s->watch);
> +}
> +s->watch = 0;
> +
> +qemu_bh_delete(s->chr_closed_bh);
> +s->chr_closed_bh = NULL;
> +
> +if (err) {
> +error_report_err(err);
> +}
> +}
> +
>  static void net_vhost_user_event(void *opaque, int event)
>  {
>  const char *name = opaque;
> @@ -212,20 +244,25 @@ static void net_vhost_user_event(void *opaque, int 
> event)
>  trace_vhost_user_event(chr->label, event);
>  switch (event) {
>  case CHR_EVENT_OPENED:
> -s->watch = qemu_chr_fe_add_watch(&s->chr, G_IO_HUP,
> - net_vhost_user_watch, s);
>  if (vhost_user_start(queues, ncs, &s->chr) < 0) {
>  qemu_chr_fe_disconnect(&s->chr);
>  return;
>  }
> +s->watch = qemu_chr_fe_add_watch(&s->chr, G_IO_HUP,
> + net_vhost_user_watch, s);
>  qmp_set_link(name, true, &err);
> +s->chr_closed_bh = qemu_bh_new(chr_closed_bh, opaque);
>  s->started = true;
>  break;
>  case CHR_EVENT_CLOSED:
> -qmp_set_link(name, false, &err);
> -vhost_user_stop(queues, ncs);
> -g_source_remove(s->watch);
> -s->watch = 0;
> +/* a close event may happen during a read/write, but vhost
> + * code assumes the vhost_dev remains setup, so delay the
> + * stop & clear to idle.
> + * FIXME: better handle failure in vhost code, remove bh
> + */
> +if (s->chr_closed_bh) {
> +qemu_bh_schedule(s->chr_closed_bh);
> +}
>  break;
>  }
>  
> -- 
> 2.12.0.rc2.3.gc93709801
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] Fail to start 2nd guest

[Stefan Hajnoczi]

On Mon, Feb 27, 2017 at 05:40:50PM +0800, Xiong Zhou wrote:
> It worked fine on Linus tree commit:
>   7bb0338 Merge tag 'rodata-v4.11-rc1' of git://git.kernel.org/pub/scm/..
> 
> failed to start 2nd domain on this commit:
> 37c8596 Merge tag 'tty-4.11-rc1' of git://git.kernel.org/pub/scm/..
> (this commit probably is not the first bad, i didn't do the bisecting)
> 
> 
> sh-4.2# uname -r
> 4.10.0-master-37c8596+
> sh-4.2# rpm -qv qemu
> qemu-2.0.0-1.el7.6.x86_64
> sh-4.2# ps ajxf | grep qemu
> 11441 11462 11461 10972 pts/011461 S+   0   0:00  \_ grep 
> qemu
> 1 11065 11064 11064 ?   -1 Sl 107   2:03 
> /usr/libexec/qemu-kvm -name 73h -S -machine 
> pc-i440fx-rhel7.0.0,accel=kvm,usb=off,dump-guest-core=off -cpu 
> Haswell,-hle,-rtm -m 8000 -realtime mlock=off -smp 
> 8,sockets=8,cores=1,threads=1 -uuid aab09e0f-d48a-4223-9b81-35d4718d59fc 
> -no-user-config -nodefaults -chardev 
> socket,id=charmonitor,path=/var/lib/libvirt/qemu/domain-1-73h/monitor.sock,server,nowait
>  -mon chardev=charmonitor,id=monitor,mode=control -rtc base=utc,driftfix=slew 
> -global kvm-pit.lost_tick_policy=delay -no-hpet -no-shutdown -global 
> PIIX4_PM.disable_s3=1 -global PIIX4_PM.disable_s4=1 -boot strict=on -device 
> ich9-usb-ehci1,id=usb,bus=pci.0,addr=0x6.0x7 -device 
> ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,addr=0x6
>  -device ich9-usb-uhci2,masterbus=usb.0,firstport=2,bus=pci.0,addr=0x6.0x1 
> -device ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pci.0,addr=0x6.0x2 
> -device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x5 -drive 
> file=/var/lib/libvirt/images/rhel73.qcow2,format=qcow2,if=none,id=drive-ide0-0-0
>  -device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 
> -drive 
> file=/var/lib/libvirt/images/73h.qcow2,format=qcow2,if=none,id=drive-ide0-0-1 
> -device ide-hd,bus=ide.0,unit=1,drive=drive-ide0-0-1,id=ide0-0-1 -drive 
> file=/root/RHEL-7.2-20151030.0-Server-x86_64-dvd1.iso,format=raw,if=none,id=drive-ide0-1-0,readonly=on
>  -device ide-cd,bus=ide.1,unit=0,drive=drive-ide0-1-0,id=ide0-1-0,bootindex=2 
> -netdev tap,fd=25,id=hostnet0 -device 
> rtl8139,netdev=hostnet0,id=net0,mac=52:54:00:83:12:7b,bus=pci.0,addr=0x3 
> -chardev pty,id=charserial0 -device isa-serial,chardev=charserial0,id=serial0 
> -chardev spicevmc,id=charchannel0,name=vdagent -device 
> virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,id=channel0,name=com.redhat.spice.0
>  -spice 
> port=5900,addr=127.0.0.1,disable-ticketing,image-compression=off,seamless-migration=on
>  -vga qxl -global qxl-vga.ram_size=67108864 -global 
> qxl-vga.vram_size=67108864 -global qxl-vga.vgamem_mb=16 -device 
> intel-hda,id=sound0,bus=pci.0,addr=0x4 -device 
> hda-duplex,id=sound0-codec0,bus=sound0.0,cad=0 -chardev 
> spicevmc,id=charredir0,name=usbredir -device 
> usb-redir,chardev=charredir0,id=redir0,bus=usb.0,port=1 -chardev 
> spicevmc,id=charredir1,name=usbredir -device 
> usb-redir,chardev=charredir1,id=redir1,bus=usb.0,port=2 -device 
> virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x7 -msg timestamp=on
> sh-4.2# virsh list
>  IdName   State
> 
>  1 73hrunning
> 
> sh-4.2# virsh start 73us
> error: Failed to start domain 73us
> error: internal error: qemu unexpectedly closed the monitor: ((null):11497): 
> Spice-Warning **: reds.c:2499:reds_init_socket: listen: Address already in use
> 2017-02-27T09:33:42.335708Z qemu-kvm: failed to initialize spice server

The error message says that the spice remote desktop cannot listen on
-spice port=5900,addr=127.0.0.1.

Did you hardcode port 5900 in the domain XML?  That could explain why
the second guest fails to launch - you need to use unique port numbers
or let libvirt automatically assign them.  Check the domain XML:

  

Another possibility is that a process running on the host is already
using port 5900.  Perhaps a guest or VNC server that was launched
outside of libvirt?  You can check this with:

  netstat -alpn | grep 5900

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] using fdt_setprop() to set properties to empty values

[Peter Maydell]

On 27 February 2017 at 01:05, David Gibson  wrote:
> On Fri, Feb 24, 2017 at 10:35:35AM +, Peter Maydell wrote:
>> On 24 February 2017 at 00:16, David Gibson  
>> wrote:
>> > Ok, I've pushed libfdt upstream patches to (a) make passing NULL to
>> > setprop() with zero length explicitly safe and (b) add an
>> > fdt_setprop_empty() helper macro.  Do you want me to make a pullreq to
>> > update the qemu submodule?
>>
>> Yes, please. Are we OK with using a random libfdt commit or do
>> we update only to proper release tags?
>
> I'm find with a random SHA, but that's not really my department - I'm
> upstream libfdt maintainer, but update policy in the qemu tree seems
> like a qemu side decision.
>
>> There's no real rush with
>> this so if you have a release due shortly it might be better
>> to wait for that.
>
> dtc/libfdt releases are a rather haphazard affair.  Usually they
> happen when somebody complains that there hasn't been a release with
> some feature they want.  Our tests are both fast to run and have
> reasonaably good coverage, so random commits are usually good.  So a
> "release" is usually just slapping a new version number onto whatever
> is in master and making a tag and tarball.

>From my end I think we'd rather use a proper release version
(if only because it's then easier to refer to and to state
as a dependency for packaged versions if required). It looks
like we've done that for our previous updates (starting
with 1.3.0 and then moving to 1.4.0 and 1.4.2) so I think
we should continue using released versions.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/2] risu_ppc64le: distinguish real illegal instruction

[Peter Maydell]

On 27 February 2017 at 05:33, Nikunj A Dadhania
 wrote:
> Peter Maydell  writes:
>
>> On 13 February 2017 at 08:59, Nikunj A Dadhania
>>  wrote:
>>> While executing qemu_ppc64le, found an issue that the real illegal
>>> instructions are handled as risu_op which results in wrong info at the
>>> master end. Even the master needs to distinguish real illegal
>>> instructions versus risu_op.
>>>
>>> Signed-off-by: Nikunj A Dadhania 
>>
>> No, this is deliberate. Otherwise you can't test illegal
>> instructions. What should happen is that both master and
>> apprentice ends end up in the default case, which does
>> a register info compare and continues having stepped the
>> PC past the illegal insn.
>
> One of the issue that I had was some of the instruction are implemented
> in the master and not in apprentice. I think we could then disable them
> in the ppc64.risu. And enable them only when we have that implemented it
> in qemu tcg.

Yes; if you haven't yet implemented an instruction the best
approach is just to not try to test it.

>> (If only one end thinks the insn is illegal then there will
>> be a register mismatch on the PC.)
>
> Yeah, the issue here was it does not come out obviously that there was a
> real illegal instruction. Maybe a error print at both the ends would
> help in debugging.

It should print "faulting insn mismatch" if the instructions which
fault aren't the same thing. This is what the arm and aarch64
implementations of reginfo_dump_mismatch() do, anyway. It looks
like the ppc and m68k versions don't do that, though.

thanks
-- PMM

[Qemu-devel] [Bug 1668103] Re: Possible off-by-one error in priority handling of hw/PL190.c

[Peter Maydell]

For a one-off one-liner bugfix patch it's easier for me to grab it from
the bug tracker than require the submitter to resend, though... I'll
have a look at it later today.

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

Title:
  Possible off-by-one error in priority handling of hw/PL190.c

Status in QEMU:
  In Progress

Bug description:
  I have a problem when reading back VECTADDR in my proprietary OS's
  interrupt handler.

  Example client code:

   1) Write INTENCLEAR to clear all interrupt enable bits
   2) Set all 16 vector control registers to zero
   3) Set vector address #2 to value 2
   4) Set vector control #2 to 0x21 (vector_interrupt_enable(0x20) | 
vector_interrupt_source(0x1) )
   5) Enable interrupt 1 by writing 0x2 to INTENABLE
   6) In interrupt handler: read VECTADDR [should read 0x2 (active IRQs vector 
address as set in step 3), reads 0x0 (active vector address index 3 instead of 
index 2)]

  Problem:

  So, for me, the block commented with /* Read vector address at the
  start of an ISR...  */ in hw/pl190.c has an off by-one error and does
  not return the vector address of the pending interrupt, but of the
  next one in the list of priorities (i.e. vector address 3).

  Solution:

  In pl190_update_vectors(), also set the priority bit for the current
  priority (1

[Qemu-devel] [PATCH v2] vhost-user: delay vhost_user_stop

[Marc-André Lureau]

Since commit b0a335e351103bf92f3f9d0bd5759311be8156ac, a socket write
may trigger a disconnect events, calling vhost_user_stop() and clearing
all the vhost_dev strutures holding data that vhost.c functions expect
to remain valid. Delay the cleanup to keep the vhost_dev structure
valid during the vhost.c functions.

Signed-off-by: Marc-André Lureau 
---
 net/vhost-user.c | 56 ++--
 1 file changed, 50 insertions(+), 6 deletions(-)

diff --git a/net/vhost-user.c b/net/vhost-user.c
index 77b8110f8c..028bf0cf5d 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -25,6 +25,7 @@ typedef struct VhostUserState {
 guint watch;
 uint64_t acked_features;
 bool started;
+QEMUBH *chr_closed_bh;
 } VhostUserState;
 
 VHostNetState *vhost_user_get_vhost_net(NetClientState *nc)
@@ -190,9 +191,45 @@ static gboolean net_vhost_user_watch(GIOChannel *chan, 
GIOCondition cond,
 
 qemu_chr_fe_disconnect(&s->chr);
 
+s->watch = 0;
 return FALSE;
 }
 
+static void net_vhost_user_event(void *opaque, int event);
+
+static void chr_closed_bh(void *opaque)
+{
+const char *name = opaque;
+NetClientState *ncs[MAX_QUEUE_NUM];
+VhostUserState *s;
+Error *err = NULL;
+int queues;
+
+queues = qemu_find_net_clients_except(name, ncs,
+  NET_CLIENT_DRIVER_NIC,
+  MAX_QUEUE_NUM);
+assert(queues < MAX_QUEUE_NUM);
+
+s = DO_UPCAST(VhostUserState, nc, ncs[0]);
+
+qmp_set_link(name, false, &err);
+vhost_user_stop(queues, ncs);
+if (s->watch) {
+g_source_remove(s->watch);
+}
+s->watch = 0;
+
+qemu_bh_delete(s->chr_closed_bh);
+s->chr_closed_bh = NULL;
+
+qemu_chr_fe_set_handlers(&s->chr, NULL, NULL, net_vhost_user_event,
+ opaque, NULL, true);
+
+if (err) {
+error_report_err(err);
+}
+}
+
 static void net_vhost_user_event(void *opaque, int event)
 {
 const char *name = opaque;
@@ -212,20 +249,27 @@ static void net_vhost_user_event(void *opaque, int event)
 trace_vhost_user_event(chr->label, event);
 switch (event) {
 case CHR_EVENT_OPENED:
-s->watch = qemu_chr_fe_add_watch(&s->chr, G_IO_HUP,
- net_vhost_user_watch, s);
 if (vhost_user_start(queues, ncs, &s->chr) < 0) {
 qemu_chr_fe_disconnect(&s->chr);
 return;
 }
+s->watch = qemu_chr_fe_add_watch(&s->chr, G_IO_HUP,
+ net_vhost_user_watch, s);
 qmp_set_link(name, true, &err);
+s->chr_closed_bh = qemu_bh_new(chr_closed_bh, opaque);
 s->started = true;
 break;
 case CHR_EVENT_CLOSED:
-qmp_set_link(name, false, &err);
-vhost_user_stop(queues, ncs);
-g_source_remove(s->watch);
-s->watch = 0;
+/* a close event may happen during a read/write, but vhost
+ * code assumes the vhost_dev remains setup, so delay the
+ * stop & clear to idle.
+ * FIXME: better handle failure in vhost code, remove bh
+ */
+if (s->chr_closed_bh) {
+qemu_chr_fe_set_handlers(&s->chr, NULL, NULL, NULL,
+ NULL, NULL, false);
+qemu_bh_schedule(s->chr_closed_bh);
+}
 break;
 }
 
-- 
2.12.0.rc2.3.gc93709801

Re: [Qemu-devel] [PATCH] vhost-user: delay vhost_user_stop

[Marc-André Lureau]

Hi

- Original Message -
> * Marc-André Lureau (marcandre.lur...@redhat.com) wrote:
> > Since commit b0a335e351103bf92f3f9d0bd5759311be8156ac, a socket write
> > may trigger a disconnect events, calling vhost_user_stop() and clearing
> > all the vhost_dev strutures holding data that vhost.c functions expect
> > to remain valid. Delay the cleanup to keep the vhost_dev structure
> > valid during the vhost.c functions.
> > 
> > Signed-off-by: Marc-André Lureau 
> 
> This does get me through a 'make check' succesfully.
> 

Yes, it's not optimal, but there is so much cleanup to deal with otherwise, 
than I am inclined to go with this approach for now, and keep the FIXME for 2.10

I'll update the patch to avoid the race on reconnect.

> Dave
> 
> > ---
> >  net/vhost-user.c | 49 +++--
> >  1 file changed, 43 insertions(+), 6 deletions(-)
> > 
> > diff --git a/net/vhost-user.c b/net/vhost-user.c
> > index 77b8110f8c..00573c8ac8 100644
> > --- a/net/vhost-user.c
> > +++ b/net/vhost-user.c
> > @@ -25,6 +25,7 @@ typedef struct VhostUserState {
> >  guint watch;
> >  uint64_t acked_features;
> >  bool started;
> > +QEMUBH *chr_closed_bh;
> >  } VhostUserState;
> >  
> >  VHostNetState *vhost_user_get_vhost_net(NetClientState *nc)
> > @@ -190,9 +191,40 @@ static gboolean net_vhost_user_watch(GIOChannel *chan,
> > GIOCondition cond,
> >  
> >  qemu_chr_fe_disconnect(&s->chr);
> >  
> > +s->watch = 0;
> >  return FALSE;
> >  }
> >  
> > +static void chr_closed_bh(void *opaque)
> > +{
> > +const char *name = opaque;
> > +NetClientState *ncs[MAX_QUEUE_NUM];
> > +VhostUserState *s;
> > +Error *err = NULL;
> > +int queues;
> > +
> > +queues = qemu_find_net_clients_except(name, ncs,
> > +  NET_CLIENT_DRIVER_NIC,
> > +  MAX_QUEUE_NUM);
> > +assert(queues < MAX_QUEUE_NUM);
> > +
> > +s = DO_UPCAST(VhostUserState, nc, ncs[0]);
> > +
> > +qmp_set_link(name, false, &err);
> > +vhost_user_stop(queues, ncs);
> > +if (s->watch) {
> > +g_source_remove(s->watch);
> > +}
> > +s->watch = 0;
> > +
> > +qemu_bh_delete(s->chr_closed_bh);
> > +s->chr_closed_bh = NULL;
> > +
> > +if (err) {
> > +error_report_err(err);
> > +}
> > +}
> > +
> >  static void net_vhost_user_event(void *opaque, int event)
> >  {
> >  const char *name = opaque;
> > @@ -212,20 +244,25 @@ static void net_vhost_user_event(void *opaque, int
> > event)
> >  trace_vhost_user_event(chr->label, event);
> >  switch (event) {
> >  case CHR_EVENT_OPENED:
> > -s->watch = qemu_chr_fe_add_watch(&s->chr, G_IO_HUP,
> > - net_vhost_user_watch, s);
> >  if (vhost_user_start(queues, ncs, &s->chr) < 0) {
> >  qemu_chr_fe_disconnect(&s->chr);
> >  return;
> >  }
> > +s->watch = qemu_chr_fe_add_watch(&s->chr, G_IO_HUP,
> > + net_vhost_user_watch, s);
> >  qmp_set_link(name, true, &err);
> > +s->chr_closed_bh = qemu_bh_new(chr_closed_bh, opaque);
> >  s->started = true;
> >  break;
> >  case CHR_EVENT_CLOSED:
> > -qmp_set_link(name, false, &err);
> > -vhost_user_stop(queues, ncs);
> > -g_source_remove(s->watch);
> > -s->watch = 0;
> > +/* a close event may happen during a read/write, but vhost
> > + * code assumes the vhost_dev remains setup, so delay the
> > + * stop & clear to idle.
> > + * FIXME: better handle failure in vhost code, remove bh
> > + */
> > +if (s->chr_closed_bh) {
> > +qemu_bh_schedule(s->chr_closed_bh);
> > +}
> >  break;
> >  }
> >  
> > --
> > 2.12.0.rc2.3.gc93709801
> > 
> --
> Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
> 

[Qemu-devel] [PATCH v4] target-s390x: Implement stfl and stfle

[Michal Marek]

Indicate the actual features in the STFL implementation and implement
STFLE.

Signed-off-by: Michal Marek 
---
v4:
 - Remove redundant buffer clearing in do_stfle()
 - Always store whole doublewords in STFLE
 - Use s390_cpu_virt_mem_write() to store the result
 - Raise a specification exception if the STFLE address is not aligned
 - Use the LowCore offset instead of hardcoding the STFL store address
v3:
 - Initialize the buffer in do_stfle()
v2:
 - STFLE is not a privileged instruction, go through the MMU to store the
   result
 - annotate the stfl helper with TCG_CALL_NO_RWG
 - Use a large enough buffer to hold the feature bitmap
 - Fix coding style of the stfle helper
---
 target/s390x/cpu_features.c |  6 --
 target/s390x/cpu_features.h |  2 +-
 target/s390x/helper.h   |  2 ++
 target/s390x/insn-data.def  |  2 ++
 target/s390x/misc_helper.c  | 38 ++
 target/s390x/translate.c| 18 ++
 6 files changed, 57 insertions(+), 11 deletions(-)

diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
index 42fd9d792bc8..d77c560380c4 100644
--- a/target/s390x/cpu_features.c
+++ b/target/s390x/cpu_features.c
@@ -286,11 +286,11 @@ void s390_init_feat_bitmap(const S390FeatInit init, 
S390FeatBitmap bitmap)
 }
 }
 
-void s390_fill_feat_block(const S390FeatBitmap features, S390FeatType type,
+int s390_fill_feat_block(const S390FeatBitmap features, S390FeatType type,
   uint8_t *data)
 {
 S390Feat feat;
-int bit_nr;
+int bit_nr, res = 0;
 
 if (type == S390_FEAT_TYPE_STFL && test_bit(S390_FEAT_ZARCH, features)) {
 /* z/Architecture is always active if around */
@@ -303,9 +303,11 @@ void s390_fill_feat_block(const S390FeatBitmap features, 
S390FeatType type,
 bit_nr = s390_features[feat].bit;
 /* big endian on uint8_t array */
 data[bit_nr / 8] |= 0x80 >> (bit_nr % 8);
+res = MAX(res, bit_nr / 8 + 1);
 }
 feat = find_next_bit(features, S390_FEAT_MAX, feat + 1);
 }
+return res;
 }
 
 void s390_add_from_feat_block(S390FeatBitmap features, S390FeatType type,
diff --git a/target/s390x/cpu_features.h b/target/s390x/cpu_features.h
index d66912178680..e3c41be08060 100644
--- a/target/s390x/cpu_features.h
+++ b/target/s390x/cpu_features.h
@@ -56,7 +56,7 @@ typedef uint64_t S390FeatInit[S390_FEAT_MAX / 64 + 1];
 const S390FeatDef *s390_feat_def(S390Feat feat);
 S390Feat s390_feat_by_type_and_bit(S390FeatType type, int bit);
 void s390_init_feat_bitmap(const S390FeatInit init, S390FeatBitmap bitmap);
-void s390_fill_feat_block(const S390FeatBitmap features, S390FeatType type,
+int s390_fill_feat_block(const S390FeatBitmap features, S390FeatType type,
   uint8_t *data);
 void s390_add_from_feat_block(S390FeatBitmap features, S390FeatType type,
   uint8_t *data);
diff --git a/target/s390x/helper.h b/target/s390x/helper.h
index 9102071d0aa4..f24b50ea48ab 100644
--- a/target/s390x/helper.h
+++ b/target/s390x/helper.h
@@ -95,6 +95,8 @@ DEF_HELPER_FLAGS_1(stckc, TCG_CALL_NO_RWG, i64, env)
 DEF_HELPER_FLAGS_2(spt, TCG_CALL_NO_RWG, void, env, i64)
 DEF_HELPER_FLAGS_1(stpt, TCG_CALL_NO_RWG, i64, env)
 DEF_HELPER_4(stsi, i32, env, i64, i64, i64)
+DEF_HELPER_FLAGS_1(stfl, TCG_CALL_NO_RWG, void, env)
+DEF_HELPER_4(stfle, i64, env, i64, i32, i64)
 DEF_HELPER_FLAGS_4(lctl, TCG_CALL_NO_WG, void, env, i32, i64, i32)
 DEF_HELPER_FLAGS_4(lctlg, TCG_CALL_NO_WG, void, env, i32, i64, i32)
 DEF_HELPER_FLAGS_4(stctl, TCG_CALL_NO_WG, void, env, i32, i64, i32)
diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
index 075ff597c3de..be830a42ed8d 100644
--- a/target/s390x/insn-data.def
+++ b/target/s390x/insn-data.def
@@ -899,6 +899,8 @@
 C(0xb202, STIDP,   S, Z,   la2, 0, new, m1_64, stidp, 0)
 /* STORE CPU TIMER */
 C(0xb209, STPT,S, Z,   la2, 0, new, m1_64, stpt, 0)
+/* STORE FACILITY LIST EXTENDED */
+C(0xb2b0, STFLE,   S,  SFLE,   0, a2, 0, 0, stfle, 0)
 /* STORE FACILITY LIST */
 C(0xb2b1, STFL,S, Z,   0, 0, 0, 0, stfl, 0)
 /* STORE PREFIX */
diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
index c9604ea9c728..2645ff8b1840 100644
--- a/target/s390x/misc_helper.c
+++ b/target/s390x/misc_helper.c
@@ -500,6 +500,44 @@ uint32_t HELPER(stsi)(CPUS390XState *env, uint64_t a0,
 return cc;
 }
 
+static int do_stfle(CPUS390XState *env, uint64_t addr, uint32_t ar, int len)
+{
+S390CPU *cpu = s390_env_get_cpu(env);
+/* 256 doublewords as per STFLE documentation */
+uint8_t data[256 * 8] = { 0 };
+int res;
+
+res = s390_fill_feat_block(cpu->model->features, S390_FEAT_TYPE_STFL, 
data);
+res = ROUND_UP(res, 8);
+s390_cpu_virt_mem_write(cpu, addr, ar, data, MIN(res, len));
+
+return res;
+}
+
+uint64_t HELPER(stfle)(CPUS390XState *env, uint64_t addr, uint32_t ar,
+   uint64_t r0)
+{
+int need,

Re: [Qemu-devel] [PATCH 04/29] 9pfs: introduce openat_nofollow() helper

[Stefan Hajnoczi]

On Fri, Feb 24, 2017 at 11:17:44PM +0100, Greg Kurz wrote:
> On Fri, 24 Feb 2017 17:17:30 +
> Stefan Hajnoczi  wrote:
> [...]
> > > > This function doesn't handle absolute paths?  It ignores leading '/' and
> > > > therefore treats all paths as relative paths.
> > > >   
> > > 
> > > Yes because any path coming from the client is supposed (*) to be 
> > > relative to the
> > > shared directory and openat(2) says:  
> > 
> > Please change the function name since this isn't openat with nofollow
> > behavior, it's a subset of openat that only takes relative paths with
> > nofollow behavior.
> 
> In the v2, this function is only called by local_open_nofollow() actually.
> Maybe I should move the stripping of leading '/' characters there ?

As long as the function name is clear then I'm happy.  If it has
different semantics from openat() then it should have a different name
(e.g. relative_openat_nofollow()).

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v2] vhost-user: delay vhost_user_stop

[Paolo Bonzini]

On 27/02/2017 11:18, Marc-André Lureau wrote:
> Since commit b0a335e351103bf92f3f9d0bd5759311be8156ac, a socket write
> may trigger a disconnect events, calling vhost_user_stop() and clearing
> all the vhost_dev strutures holding data that vhost.c functions expect
> to remain valid. Delay the cleanup to keep the vhost_dev structure
> valid during the vhost.c functions.
> 
> Signed-off-by: Marc-André Lureau 
> ---
>  net/vhost-user.c | 56 
> ++--
>  1 file changed, 50 insertions(+), 6 deletions(-)
> 
> diff --git a/net/vhost-user.c b/net/vhost-user.c
> index 77b8110f8c..028bf0cf5d 100644
> --- a/net/vhost-user.c
> +++ b/net/vhost-user.c
> @@ -25,6 +25,7 @@ typedef struct VhostUserState {
>  guint watch;
>  uint64_t acked_features;
>  bool started;
> +QEMUBH *chr_closed_bh;
>  } VhostUserState;
>  
>  VHostNetState *vhost_user_get_vhost_net(NetClientState *nc)
> @@ -190,9 +191,45 @@ static gboolean net_vhost_user_watch(GIOChannel *chan, 
> GIOCondition cond,
>  
>  qemu_chr_fe_disconnect(&s->chr);
>  
> +s->watch = 0;
>  return FALSE;
>  }
>  
> +static void net_vhost_user_event(void *opaque, int event);
> +
> +static void chr_closed_bh(void *opaque)
> +{
> +const char *name = opaque;
> +NetClientState *ncs[MAX_QUEUE_NUM];
> +VhostUserState *s;
> +Error *err = NULL;
> +int queues;
> +
> +queues = qemu_find_net_clients_except(name, ncs,
> +  NET_CLIENT_DRIVER_NIC,
> +  MAX_QUEUE_NUM);
> +assert(queues < MAX_QUEUE_NUM);
> +
> +s = DO_UPCAST(VhostUserState, nc, ncs[0]);
> +
> +qmp_set_link(name, false, &err);
> +vhost_user_stop(queues, ncs);
> +if (s->watch) {
> +g_source_remove(s->watch);
> +}
> +s->watch = 0;
> +
> +qemu_bh_delete(s->chr_closed_bh);
> +s->chr_closed_bh = NULL;
> +
> +qemu_chr_fe_set_handlers(&s->chr, NULL, NULL, net_vhost_user_event,
> + opaque, NULL, true);
> +
> +if (err) {
> +error_report_err(err);
> +}
> +}
> +
>  static void net_vhost_user_event(void *opaque, int event)
>  {
>  const char *name = opaque;
> @@ -212,20 +249,27 @@ static void net_vhost_user_event(void *opaque, int 
> event)
>  trace_vhost_user_event(chr->label, event);
>  switch (event) {
>  case CHR_EVENT_OPENED:
> -s->watch = qemu_chr_fe_add_watch(&s->chr, G_IO_HUP,
> - net_vhost_user_watch, s);
>  if (vhost_user_start(queues, ncs, &s->chr) < 0) {
>  qemu_chr_fe_disconnect(&s->chr);
>  return;
>  }
> +s->watch = qemu_chr_fe_add_watch(&s->chr, G_IO_HUP,
> + net_vhost_user_watch, s);
>  qmp_set_link(name, true, &err);
> +s->chr_closed_bh = qemu_bh_new(chr_closed_bh, opaque);
>  s->started = true;
>  break;
>  case CHR_EVENT_CLOSED:
> -qmp_set_link(name, false, &err);
> -vhost_user_stop(queues, ncs);
> -g_source_remove(s->watch);
> -s->watch = 0;
> +/* a close event may happen during a read/write, but vhost
> + * code assumes the vhost_dev remains setup, so delay the
> + * stop & clear to idle.
> + * FIXME: better handle failure in vhost code, remove bh
> + */
> +if (s->chr_closed_bh) {
> +qemu_chr_fe_set_handlers(&s->chr, NULL, NULL, NULL,
> + NULL, NULL, false);
> +qemu_bh_schedule(s->chr_closed_bh);
> +}

The bottom half adds a small overhead to the event loop all the time,
even if it is not scheduled.  Would it be possible to create it here
instead?  You can have a s->state enum (OPENED, CLOSING, CLOSED for
example).  You can even use aio_bh_schedule_oneshot to avoid having to
store the QEMUBH pointer.

Paolo

>  break;
>  }
>  
> 

Re: [Qemu-devel] Non-flat command line option argument syntax

[Markus Armbruster]

Markus Armbruster  writes:

[...]
> === Dotted keys ===
>
> One sufficiently powerful syntax extension already exists: the dotted
> key convention.  It's syntactically unambiguous only when none of the
> KEYs involved contains '.'  To adopt it across the board, we'd have to
> outlaw '.' in KEYs.  QAPI outlaws '.' already, but we have a bunch of
> QOM properties names with '.'.  We'd have to rename at least the ones
> that need to be accessible in -object.
>
> Dotted keys can't express member names that look like integers.  We'd
> have to outlaw them at least for the objects that are accessible on the
> command line.  Once again, QAPI outlaws such names already.  QOM is
> anarchy when it comes to names, however.
>
> The way dotted keys do arrays is inconsistent with how QOM's automatic
> arrayification (commit 3396590) do them: foo.0 vs. foo[0].  Backward
> compatibility makes changing the dotted key convention awkward.  Perhaps
> we can still change QOM.

Design flaw: there is no good way to denote an empty array or object
other than the root object.

Empty KEY=VALUE,... is valid and results in an empty root object.

Presence of a KEY that contains periods results in additional non-root
objects or arrays.  For instance, KEY a.b.c results in root object
member "a" that has member "b" that has (scalar) member "c".

These additional objects and arrays all have at least one member, by
construction.

Begs the question how to denote an empty object or array other than the
root.

A natural idea is to interpret "absent in KEY=VALUE,..." as empty.
After all, removing one key from it removes one member when there are
more, so why not when there aren't.

Sadly, it doesn't work: "absent in KEY=VALUE,..." already means
"optional object/array absent", which isn't the same as "empty
object/array present".

Without additional syntax, all we can do is choose what exactly to make
impossible:

* Absent key means absent, period.  No way to do empty array or object.
  This is what I implemented.

* Absent key means absent, except when the member is visited it means
  empty.  No way to do absent optional array or object.

* Likewise, but if the visit is preceeded by a test for presence with
  visit_optional(), it means absent again.  No way to do present
  optional empty array or object.  This requires keeping additional
  state.

Any bright ideas on how to avoid making things impossible?


[...]

Re: [Qemu-devel] [PATCH v2 2/3] filter-rewriter: fix memory leak for connection in connection_track_table

[Hailiang Zhang]

On 2017/2/27 17:05, Jason Wang wrote:



On 2017年02月27日 14:53, Hailiang Zhang wrote:

I think the issue is that your code can not differ A from B.



We have a parameter 'fin_ack_seq' recording the sequence of
'FIN=1,ACK=1,seq=w,ack=u+1' and if the ack value from the opposite
side is is 'w+1', we can consider this connection is closed, no ?




Hi Jason,

Thanks very much for your patience.


Let's see what happens, consider VM is doing active close (reuse the
figure above):



(We didn't support tracking the connection start
by the VM in current rewriter codes.
I mean the Client side is VM).

Your figure is not quite correct, the process should be:
  (VM)
Client:Server:

ESTABLISHED|   |
   | -> FIN=1,seq=u   ->   |
FIN_WAIT_1 |   |
   | <- ACK=1,seq=v,ack=u+1 <- |
FINA_WAIT_2|   |CLOSE_WAIT
   | <- FIN=1,ACK=1,seq=w,ack=u+1<-|
handle_secondary():
fin_ack_seq = w
tcp_state = TCPS_LAST_ACK

   |   |LAST+ACK
   |   -> ACK=1,seq=u+1,ack=w+1|
TIME_WAIT  |   |CLOSED
CLOSED |   |

handle_primary():
if (ack = fin_ack_seq + 1)
   g_hash_table_remove()


(VM)
Client:Server:

ESTABLISHED|   |
 | -> FIN=1,seq=u   ->   |

handle_secondary():
fin_ack_seq = u
tcp_state = TCPS_LAST_ACK

FIN_WAIT_1 |   |
 | <- ACK=1,seq=v,ack=u+1 <- |

handle_primary():
fin_ack_seq = ack + 1
g_hash_table_remove()

But we probably want it to be removed in TIME_WAIT_CLOSED.



Yes, we should removed it after 2MSL, because the last
the sever side may not get the 'ACK=1,seq=v,ack=u+1' packet,
and it will resend the 'FIN=1,ACK=1,seq=w,ack=u+1'.

Thanks.



Thanks

.

Re: [Qemu-devel] [PATCH] Removed support for depth!=32

[Peter Maydell]

On 27 February 2017 at 08:14, Suramya Shah  wrote:
> Signed-off-by: Suramya Shah 
> ---
>  hw/display/sm501_template.h | 8 +---
>  1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/hw/display/sm501_template.h b/hw/display/sm501_template.h
> index f33e499..4e5801e 100644
> --- a/hw/display/sm501_template.h
> +++ b/hw/display/sm501_template.h
> @@ -22,13 +22,7 @@
>   * THE SOFTWARE.
>   */
>
> -#if DEPTH == 8
> -#define BPP 1
> -#define PIXEL_TYPE uint8_t
> -#elif DEPTH == 15 || DEPTH == 16
> -#define BPP 2
> -#define PIXEL_TYPE uint16_t
> -#elif DEPTH == 32
> +#if DEPTH == 32
>  #define BPP 4
>  #define PIXEL_TYPE uint32_t
>  #else

Hi there; thanks for this patch. How did you test it? It doesn't
compile for me:

In file included from
/home/petmay01/linaro/qemu-from-laptop/qemu/hw/display/sm501.c:1177:0:
/home/petmay01/linaro/qemu-from-laptop/qemu/hw/display/sm501_template.h:29:2:
error: #error unsupport depth
 #error unsupport depth
  ^

This is because you've removed the code from this #ifdef
but you haven't also removed the code in sm501.c which is
currently trying to use it. (You also can expand out the
uses of PIXEL_NAME and PIXEL_TYPE used later in sm501_template.h
since they are now only ever defined to one thing, but not
doing that doesn't cause compilation failure.)


(Gerd: can you remind me of the reason why we can assume that
depth is always 32 here? IIRC it's because the UI layer always
uses 32 bit depth now but I couldn't convince myself of that
with a quick look through the ui code...)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 04/29] 9pfs: introduce openat_nofollow() helper

[Greg Kurz]

On Mon, 27 Feb 2017 10:20:58 +
Stefan Hajnoczi  wrote:

> On Fri, Feb 24, 2017 at 11:17:44PM +0100, Greg Kurz wrote:
> > On Fri, 24 Feb 2017 17:17:30 +
> > Stefan Hajnoczi  wrote:
> > [...]  
> > > > > This function doesn't handle absolute paths?  It ignores leading '/' 
> > > > > and
> > > > > therefore treats all paths as relative paths.
> > > > > 
> > > > 
> > > > Yes because any path coming from the client is supposed (*) to be 
> > > > relative to the
> > > > shared directory and openat(2) says:
> > > 
> > > Please change the function name since this isn't openat with nofollow
> > > behavior, it's a subset of openat that only takes relative paths with
> > > nofollow behavior.  
> > 
> > In the v2, this function is only called by local_open_nofollow() actually.
> > Maybe I should move the stripping of leading '/' characters there ?  
> 
> As long as the function name is clear then I'm happy.  If it has
> different semantics from openat() then it should have a different name
> (e.g. relative_openat_nofollow()).
> 

I've moved the stripping to the caller. This makes the code simpler.

> Stefan



pgppeyBEBD7cv.pgp
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 4/4] migration: fix use-after-free of to_dst_file

[Dr. David Alan Gilbert]

* Vladimir Sementsov-Ogievskiy (vsement...@virtuozzo.com) wrote:
> hmp_savevm calls qemu_savevm_state(f), which sets to_dst_file=f in
> global migration state. Then hmp_savevm closes f (g_free called).
> 
> Next access to to_dst_file in migration state (for example,
> qmp_migrate_set_speed) will use it after it was freed.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 

Reviewed-by: Dr. David Alan Gilbert 

> ---
>  migration/savevm.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 75e56d2d07..fcb8fd8acd 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1276,6 +1276,11 @@ done:
>  status = MIGRATION_STATUS_COMPLETED;
>  }
>  migrate_set_state(&ms->state, MIGRATION_STATUS_SETUP, status);
> +
> +/* f is outer parameter, it should not stay in global migration state 
> after
> + * this function finished */
> +ms->to_dst_file = NULL;
> +
>  return ret;
>  }
>  
> -- 
> 2.11.1
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] Adding support for LPD and LPDG instructions

[Éric Bischoff]

Le samedi 25 février 2017, 10:42:43 CET Richard Henderson a écrit :
> On 02/23/2017 10:58 PM, Eric Bischoff wrote:
> > +/* LOAD PAIR DISJOINT */
> > +C(0xc804, LPD, SSF,   ILA, m1_32s, m2_32s, 0, r3_P32, movx, 0)
> > +C(0xc805, LPDG,SSF,   ILA, m1_64, m2_64, 0, r3_P64, movx, 0)
> 
> Missing is the update to the condition codes.
> I think just setting CC = 3 (not loaded interlocked) is probably fine.

I tested and 0 at the end of C macro means "do nothing with CC".

I indeed need to set CC (but to 0, not to 3). New patch coming.


-- 
Eric Bischoff - SUSE Manager QA Engineer
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip 
Upmanyu, Graham Norton, HRB 21284 (AG Nürnberg)Le dimanche 26 février 2017, 

[Qemu-devel] [PATCH v3] vhost-user: delay vhost_user_stop

[Marc-André Lureau]

Since commit b0a335e351103bf92f3f9d0bd5759311be8156ac, a socket write
may trigger a disconnect events, calling vhost_user_stop() and clearing
all the vhost_dev strutures holding data that vhost.c functions expect
to remain valid. Delay the cleanup to keep the vhost_dev structure
valid during the vhost.c functions.

Signed-off-by: Marc-André Lureau 
---
v3:
 - use aio_bh_schedule_oneshot(), as suggest by Paolo
v2:
 - fix reconnect race

net/vhost-user.c | 53 ++---
 1 file changed, 46 insertions(+), 7 deletions(-)

diff --git a/net/vhost-user.c b/net/vhost-user.c
index 77b8110f8c..e7e63408a1 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -190,7 +190,35 @@ static gboolean net_vhost_user_watch(GIOChannel *chan, 
GIOCondition cond,
 
 qemu_chr_fe_disconnect(&s->chr);
 
-return FALSE;
+return TRUE;
+}
+
+static void net_vhost_user_event(void *opaque, int event);
+
+static void chr_closed_bh(void *opaque)
+{
+const char *name = opaque;
+NetClientState *ncs[MAX_QUEUE_NUM];
+VhostUserState *s;
+Error *err = NULL;
+int queues;
+
+queues = qemu_find_net_clients_except(name, ncs,
+  NET_CLIENT_DRIVER_NIC,
+  MAX_QUEUE_NUM);
+assert(queues < MAX_QUEUE_NUM);
+
+s = DO_UPCAST(VhostUserState, nc, ncs[0]);
+
+qmp_set_link(name, false, &err);
+vhost_user_stop(queues, ncs);
+
+qemu_chr_fe_set_handlers(&s->chr, NULL, NULL, net_vhost_user_event,
+ opaque, NULL, true);
+
+if (err) {
+error_report_err(err);
+}
 }
 
 static void net_vhost_user_event(void *opaque, int event)
@@ -212,20 +240,31 @@ static void net_vhost_user_event(void *opaque, int event)
 trace_vhost_user_event(chr->label, event);
 switch (event) {
 case CHR_EVENT_OPENED:
-s->watch = qemu_chr_fe_add_watch(&s->chr, G_IO_HUP,
- net_vhost_user_watch, s);
 if (vhost_user_start(queues, ncs, &s->chr) < 0) {
 qemu_chr_fe_disconnect(&s->chr);
 return;
 }
+s->watch = qemu_chr_fe_add_watch(&s->chr, G_IO_HUP,
+ net_vhost_user_watch, s);
 qmp_set_link(name, true, &err);
 s->started = true;
 break;
 case CHR_EVENT_CLOSED:
-qmp_set_link(name, false, &err);
-vhost_user_stop(queues, ncs);
-g_source_remove(s->watch);
-s->watch = 0;
+/* a close event may happen during a read/write, but vhost
+ * code assumes the vhost_dev remains setup, so delay the
+ * stop & clear to idle.
+ * FIXME: better handle failure in vhost code, remove bh
+ */
+if (s->watch) {
+AioContext *ctx = qemu_get_current_aio_context();
+
+g_source_remove(s->watch);
+s->watch = 0;
+qemu_chr_fe_set_handlers(&s->chr, NULL, NULL, NULL,
+ NULL, NULL, false);
+
+aio_bh_schedule_oneshot(ctx, chr_closed_bh, opaque);
+}
 break;
 }
 
-- 
2.12.0.rc2.3.gc93709801

Re: [Qemu-devel] [PATCH v3] vhost-user: delay vhost_user_stop

[Paolo Bonzini]

On 27/02/2017 11:49, Marc-André Lureau wrote:
> Since commit b0a335e351103bf92f3f9d0bd5759311be8156ac, a socket write
> may trigger a disconnect events, calling vhost_user_stop() and clearing
> all the vhost_dev strutures holding data that vhost.c functions expect
> to remain valid. Delay the cleanup to keep the vhost_dev structure
> valid during the vhost.c functions.
> 
> Signed-off-by: Marc-André Lureau 
> ---
> v3:
>  - use aio_bh_schedule_oneshot(), as suggest by Paolo
> v2:
>  - fix reconnect race
> 
> net/vhost-user.c | 53 ++---
>  1 file changed, 46 insertions(+), 7 deletions(-)
> 
> diff --git a/net/vhost-user.c b/net/vhost-user.c
> index 77b8110f8c..e7e63408a1 100644
> --- a/net/vhost-user.c
> +++ b/net/vhost-user.c
> @@ -190,7 +190,35 @@ static gboolean net_vhost_user_watch(GIOChannel *chan, 
> GIOCondition cond,
>  
>  qemu_chr_fe_disconnect(&s->chr);
>  
> -return FALSE;
> +return TRUE;
> +}
> +
> +static void net_vhost_user_event(void *opaque, int event);
> +
> +static void chr_closed_bh(void *opaque)
> +{
> +const char *name = opaque;
> +NetClientState *ncs[MAX_QUEUE_NUM];
> +VhostUserState *s;
> +Error *err = NULL;
> +int queues;
> +
> +queues = qemu_find_net_clients_except(name, ncs,
> +  NET_CLIENT_DRIVER_NIC,
> +  MAX_QUEUE_NUM);
> +assert(queues < MAX_QUEUE_NUM);
> +
> +s = DO_UPCAST(VhostUserState, nc, ncs[0]);
> +
> +qmp_set_link(name, false, &err);
> +vhost_user_stop(queues, ncs);
> +
> +qemu_chr_fe_set_handlers(&s->chr, NULL, NULL, net_vhost_user_event,
> + opaque, NULL, true);
> +
> +if (err) {
> +error_report_err(err);
> +}
>  }
>  
>  static void net_vhost_user_event(void *opaque, int event)
> @@ -212,20 +240,31 @@ static void net_vhost_user_event(void *opaque, int 
> event)
>  trace_vhost_user_event(chr->label, event);
>  switch (event) {
>  case CHR_EVENT_OPENED:
> -s->watch = qemu_chr_fe_add_watch(&s->chr, G_IO_HUP,
> - net_vhost_user_watch, s);
>  if (vhost_user_start(queues, ncs, &s->chr) < 0) {
>  qemu_chr_fe_disconnect(&s->chr);
>  return;
>  }
> +s->watch = qemu_chr_fe_add_watch(&s->chr, G_IO_HUP,
> + net_vhost_user_watch, s);
>  qmp_set_link(name, true, &err);
>  s->started = true;
>  break;
>  case CHR_EVENT_CLOSED:
> -qmp_set_link(name, false, &err);
> -vhost_user_stop(queues, ncs);
> -g_source_remove(s->watch);
> -s->watch = 0;
> +/* a close event may happen during a read/write, but vhost
> + * code assumes the vhost_dev remains setup, so delay the
> + * stop & clear to idle.
> + * FIXME: better handle failure in vhost code, remove bh
> + */
> +if (s->watch) {
> +AioContext *ctx = qemu_get_current_aio_context();
> +
> +g_source_remove(s->watch);
> +s->watch = 0;

Removing the watch here makes sense too!  Thanks,

Paolo
> +qemu_chr_fe_set_handlers(&s->chr, NULL, NULL, NULL,
> + NULL, NULL, false);
> +
> +aio_bh_schedule_oneshot(ctx, chr_closed_bh, opaque);
> +}
>  break;
>  }
>  
> 

Re: [Qemu-devel] Non-flat command line option argument syntax

[Kevin Wolf]

Am 27.02.2017 um 11:27 hat Markus Armbruster geschrieben:
> Markus Armbruster  writes:
> 
> [...]
> > === Dotted keys ===
> >
> > One sufficiently powerful syntax extension already exists: the dotted
> > key convention.  It's syntactically unambiguous only when none of the
> > KEYs involved contains '.'  To adopt it across the board, we'd have to
> > outlaw '.' in KEYs.  QAPI outlaws '.' already, but we have a bunch of
> > QOM properties names with '.'.  We'd have to rename at least the ones
> > that need to be accessible in -object.
> >
> > Dotted keys can't express member names that look like integers.  We'd
> > have to outlaw them at least for the objects that are accessible on the
> > command line.  Once again, QAPI outlaws such names already.  QOM is
> > anarchy when it comes to names, however.
> >
> > The way dotted keys do arrays is inconsistent with how QOM's automatic
> > arrayification (commit 3396590) do them: foo.0 vs. foo[0].  Backward
> > compatibility makes changing the dotted key convention awkward.  Perhaps
> > we can still change QOM.
> 
> Design flaw: there is no good way to denote an empty array or object
> other than the root object.
> 
> Empty KEY=VALUE,... is valid and results in an empty root object.
> 
> Presence of a KEY that contains periods results in additional non-root
> objects or arrays.  For instance, KEY a.b.c results in root object
> member "a" that has member "b" that has (scalar) member "c".
> 
> These additional objects and arrays all have at least one member, by
> construction.
> 
> Begs the question how to denote an empty object or array other than the
> root.
> 
> A natural idea is to interpret "absent in KEY=VALUE,..." as empty.
> After all, removing one key from it removes one member when there are
> more, so why not when there aren't.
> 
> Sadly, it doesn't work: "absent in KEY=VALUE,..." already means
> "optional object/array absent", which isn't the same as "empty
> object/array present".
> 
> Without additional syntax, all we can do is choose what exactly to make
> impossible:
> 
> * Absent key means absent, period.  No way to do empty array or object.
>   This is what I implemented.

I'm not currently aware of any places where the difference between a
present, but empty array and an absent array is actually significant, so
this is probably the most consistent and useful way to interpret things.

In other words, I agree with your implementation.

> * Absent key means absent, except when the member is visited it means
>   empty.  No way to do absent optional array or object.
> 
> * Likewise, but if the visit is preceeded by a test for presence with
>   visit_optional(), it means absent again.  No way to do present
>   optional empty array or object.  This requires keeping additional
>   state.
> 
> Any bright ideas on how to avoid making things impossible?

I can't see any other option than extending the syntax if we need this.
We can't tell the difference between a string and any other object
description after =, so we would need to make use of reserved characters
in the key name. Maybe just 'foo.array[]' (without any =) for an empty
array or something like that.

Before we introduce anything like this, do we actually need it?

Kevin

Re: [Qemu-devel] Question regarding Snapshots

[Stefan Hajnoczi]

On Wed, Feb 22, 2017 at 03:06:41PM -0600, Tim Cusack wrote:
> I have a scenario that perhaps not many have attempted, but I still hope
> that it is possible.
> 
> Scenario:
> 
> I would like to have a Base Windows 7 VM in KVM/QEMU.
> 
> I would follow the following process (confirmed it works)
> 
> 1. Shutdown running domain
> - virsh shutdown 
> 2. Remove existing disk from Domain XML:
> - virt-xml BASE --remove-device --disk target=hda
> 3. Add Correct Disk image for Snapshot to Domain XML:
> - virt-xml BASE --add-device --disk
> /var/lib/libvirt/images/BASE.qcow2,format=qcow2,target=hda,bus=ide
> 4. Create the snapshot with description
> - virsh snapshot-create-as BASE .qcow2 "OEM, Model" --disk-only
> --atomic
> 5. Start Snapshot
> - virsh start FA2BASE

This process uses libvirt instead of QEMU commands.  Adding libvirt
mailing list.

> 
> All that works, but my question and issue is this:
> 
> Can you make a snapshot, then go back to the base and ignore that snapshot
> like a closed branch, make another snapshot, and then go back and forth
> from each to each?
> 
> My reasons for needing this are that I have tools and software that doesnt
> play nice together, and would like to keep them separated on different
> snapshots, but never really need to have more than one up at a time.
> 
> This would allow me to have only 1 windows license per computer, and the
> ability to just change from tool to tool with simple front end to virsh to
> remove the disk from the .xml and add the other one.
> 
> So like this:
> Tool A is on snapshot A
> Tool B is on snapshot B
> go through steps 1-5 above to go back and forth.
> 
> I know that I can not merge things back into the base, but I really don't
> need to do that, in fact, I can throw away the tool snapshots when new
> tools come out, by making new snapshots from the base when new tools come
> out.
> 
> We have done this already, but we had an issue where the ability to write
> to the snapshots seemed to stop.
> 
> Reason unknown, everything worked one day and next could not write to any
> snapshot, but could still load the base and work on it fine.
> 
> This might be the wrong mailing list to post to, if so, could anyone point
> out a more appropriate one?
> 
> Tim


signature.asc
Description: PGP signature

[Qemu-devel] [PATCH V2] qemu-img: make convert async

[Peter Lieven]

the convert process is currently completely implemented with sync operations.
That means it reads one buffer and then writes it. No parallelism and each sync
request takes as long as it takes until it is completed.

This can be a big performance hit when the convert process reads and writes
to devices which do not benefit from kernel readahead or pagecache.
In our environment we heavily have the following two use cases when using
qemu-img convert.

a) reading from NFS and writing to iSCSI for deploying templates
b) reading from iSCSI and writing to NFS for backups

In both processes we use libiscsi and libnfs so we have no kernel cache.

This patch changes the convert process to work with parallel running coroutines
which can significantly improve performance for network storage devices:

qemu-img (master)
 nfs -> iscsi 22.8 secs
 nfs -> ram   11.7 secs
 ram -> iscsi 12.3 secs

qemu-img-async (8 coroutines, in-order write disabled)
 nfs -> iscsi 11.0 secs
 nfs -> ram   10.4 secs
 ram -> iscsi  9.0 secs

This patches introduces 2 new cmdline parameters. The -m parameter to specify
the number of coroutines running in parallel (defaults to 8). And the -W 
paremeter to
allow qemu-img to write to the target out of order rather than sequential. This 
improves
performance as the writes do not have to wait for each other to complete.

Signed-off-by: Peter Lieven 
---
V1->V2: - do not calculate source partition globally [Kevin]
- don't use s->status outside the global lock [Kevin]
- remove accidently left bracket in qemu-img.texi [Kevin]
- reworkd -W parageaph in documentation [Stefan]

   RFC->V1: - add documentation
- add missing coroutine_fn annotation [Stefan]
- add a comment why it is safe to call coroutine_enter [Stefan]
- check -m paramater for values < 1 [Stefan]
- disallow -W parameter with compression [Stefan]

RFC V3->V4: - avoid to prepare a request queue upfront [Kevin]
- do not ignore the BLK_BACKING_FILE status [Kevin]
- redesign the interface to the read and write routines [Kevin]

RFC V2->V3: - updated stats in the commit msg from a host with a better network 
card
- only wake up the coroutine that is acutally waiting for a write 
to complete.
  this was not only overhead, but also breaking at least linux AIO.
- fix coding style complaints
- rename some variables and structs

RFC V1->V2: - using coroutine as worker "threads". [Max]
- keeping the request queue as otherwise it happens
  that we wait on BLK_ZERO chunks while keeping the write order.
  it also avoids redundant calls to get_block_status and helps
  to skip some conditions for fully allocated imaged 
(!s->min_sparse)

---
 qemu-img-cmds.hx |   4 +-
 qemu-img.c   | 322 ++-
 qemu-img.texi|  16 ++-
 3 files changed, 243 insertions(+), 99 deletions(-)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index f054599..9c9702c 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -40,9 +40,9 @@ STEXI
 ETEXI
 
 DEF("convert", img_convert,
-"convert [--object objectdef] [--image-opts] [-c] [-p] [-q] [-n] [-f fmt] 
[-t cache] [-T src_cache] [-O output_fmt] [-o options] [-s snapshot_id_or_name] 
[-l snapshot_param] [-S sparse_size] filename [filename2 [...]] 
output_filename")
+"convert [--object objectdef] [--image-opts] [-c] [-p] [-q] [-n] [-f fmt] 
[-t cache] [-T src_cache] [-O output_fmt] [-o options] [-s snapshot_id_or_name] 
[-l snapshot_param] [-S sparse_size] [-m num_coroutines] [-W] filename 
[filename2 [...]] output_filename")
 STEXI
-@item convert [--object @var{objectdef}] [--image-opts] [-c] [-p] [-q] [-n] 
[-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-o 
@var{options}] [-s @var{snapshot_id_or_name}] [-l @var{snapshot_param}] [-S 
@var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
+@item convert [--object @var{objectdef}] [--image-opts] [-c] [-p] [-q] [-n] 
[-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-o 
@var{options}] [-s @var{snapshot_id_or_name}] [-l @var{snapshot_param}] [-S 
@var{sparse_size}] [-m @var{num_coroutines}] [-W] @var{filename} 
[@var{filename2} [...]] @var{output_filename}
 ETEXI
 
 DEF("dd", img_dd,
diff --git a/qemu-img.c b/qemu-img.c
index cff22e3..1826202 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -156,6 +156,11 @@ static void QEMU_NORETURN help(void)
"   kinds of errors, with a higher risk of choosing the wrong 
fix or\n"
"   hiding corruption that has already occurred.\n"
"\n"
+   "Parameters to convert subcommand:\n"
+   "  '-m' specifies how many coroutines work in parallel during the 
convert\n"
+   "   process (defaults to 8)\n"
+   "  '-W' allow to write to the targe

Re: [Qemu-devel] [PATCH v2 00/30] Various memory leak fixes

[Marc-André Lureau]

Hi

On Tue, Feb 21, 2017 at 6:27 PM Marc-André Lureau <
marcandre.lur...@redhat.com> wrote:

> After this series removing a few memory leaks, make check with ASAN
> enabled gives a "clean" run with x86_64-softmmu target (there are
> warnings related to coroutine makecontext/swapcontext).
>
> This should help spot memory related regressions when introducing one.
>
> Note: some of these patches were sent and reviewed previously but not
> yet applied, I rebased them and included them here for completeness.
>

The series is missing reviews of the following patches: 8, 9, 11, 12, 17,
19, 20, 28, 29

Any volunteer? Thanks!


>
> v2:
> - add r-b tags
> - fix gtest compatibility with old glib
> - add "migration: fix id leak regression" patch
>
> Igor Mammedov (1):
>   pc: pcihp: avoid adding ACPI_PCIHP_PROP_BSEL twice
>
> Marc-André Lureau (29):
>   qtest: fix a memory leak
>   tests: fix qmp response leak
>   tests: fix leaks in test-io-channel-command
>   timer: use an inline function for free
>   glib-compat: add g_test_add_data_func_full fallback
>   tests: fix ptimer leaks
>   tests: fix endianness-test leaks
>   tests: fix q35-test leaks
>   tests: fix vhost-user-test leaks
>   tests: fix ide-test leaks
>   tests: fix hd-geo-test leaks
>   tests: fix bios-tables-test leak
>   tests: fix ipmi-kcs-test leak
>   tests: fix ipmi-bt-test leak
>   tests: fix eepro100-test leak
>   tests: fix tco-test leaks
>   tests: fix e1000-test leak
>   tests: fix i440fx-test leaks
>   tests: fix e1000e leaks
>   tests: fix virtio-scsi-test leak
>   tests: fix virtio-9p-test leaks
>   bus: do not unref hotplug handler
>   usb: replace handle_destroy with unrealize
>   usb: release the created buses
>   tests: allows to run single test in usb-hcd-ehci-test
>   tests: fix usb-test leaks
>   tests: add specialized device_find function
>   tests: fix virtio-blk-test leaks
>   migration: fix id leak regression
>
>  include/glib-compat.h   |  21 +++
>  include/hw/ptimer.h |   1 +
>  include/hw/usb.h|   5 --
>  include/qemu/timer.h|   5 +-
>  tests/libqos/usb.h  |   1 +
>  tests/libqos/virtio-pci.h   |   6 +-
>  hw/acpi/pcihp.c |  11 
>  hw/core/bus.c   |   2 +-
>  hw/core/ptimer.c|   8 +++
>  hw/i386/acpi-build.c|   4 +-
>  hw/usb/bus.c|   9 +--
>  hw/usb/dev-audio.c  |   4 +-
>  hw/usb/dev-bluetooth.c  |   4 +-
>  hw/usb/dev-hid.c|   4 +-
>  hw/usb/dev-hub.c|   4 +-
>  hw/usb/dev-network.c|   4 +-
>  hw/usb/dev-smartcard-reader.c   |   4 +-
>  hw/usb/dev-storage.c|  16 ++
>  hw/usb/dev-uas.c|   6 +-
>  hw/usb/dev-wacom.c  |   4 +-
>  hw/usb/host-libusb.c|   4 +-
>  hw/usb/redirect.c   |   4 +-
>  migration/savevm.c  |   1 +
>  qemu-timer.c|   5 --
>  qtest.c |   1 +
>  tests/bios-tables-test.c|   2 +-
>  tests/e1000-test.c  |   1 +
>  tests/e1000e-test.c |   6 +-
>  tests/eepro100-test.c   |   1 +
>  tests/endianness-test.c |   3 +
>  tests/hd-geo-test.c |  47 ++--
>  tests/i440fx-test.c |   5 ++
>  tests/ide-test.c|  12 
>  tests/ipmi-bt-test.c|   1 +
>  tests/ipmi-kcs-test.c   |   1 +
>  tests/libqos/usb.c  |   6 ++
>  tests/libqos/virtio-pci.c   |  38 +++--
>  tests/libqtest.c|  10 
>  tests/postcopy-test.c   |   2 +-
>  tests/ptimer-test-stubs.c   |   5 ++
>  tests/ptimer-test.c | 122
> 
>  tests/pvpanic-test.c|   1 +
>  tests/q35-test.c|   3 +
>  tests/tco-test.c|  35 +++-
>  tests/test-filter-mirror.c  |   2 +-
>  tests/test-filter-redirector.c  |   4 +-
>  tests/test-io-channel-command.c |   6 +-
>  tests/usb-hcd-ehci-test.c   |  19 +--
>  tests/usb-hcd-uhci-test.c   |   1 +
>  tests/vhost-user-test.c |  11 ++--
>  tests/virtio-9p-test.c  |   2 +-
>  tests/virtio-blk-test.c |  29 +-
>  tests/virtio-scsi-test.c|   2 +-
>  53 files changed, 338 insertions(+), 177 deletions(-)
>
> --
> 2.11.0.295.gd7dffce1c.dirty
>
>
> --
Marc-André Lureau

Re: [Qemu-devel] [PATCH v2 00/16] Postcopy: Hugepage support

[Alexey Perevalov]

Hi David,


On Tue, Feb 21, 2017 at 10:03:14AM +, Dr. David Alan Gilbert wrote:
> * Alexey Perevalov (a.pereva...@samsung.com) wrote:
> > 
> > Hello David,
> 
> Hi Alexey,
> 
> > On Tue, Feb 14, 2017 at 07:34:26PM +, Dr. David Alan Gilbert wrote:
> > > * Alexey Perevalov (a.pereva...@samsung.com) wrote:
> > > > Hi David,
> > > > 
> > > > Thank your, now it's clear.
> > > > 
> > > > On Mon, Feb 13, 2017 at 06:16:02PM +, Dr. David Alan Gilbert wrote:
> > > > > * Alexey Perevalov (a.pereva...@samsung.com) wrote:
> > > > > >  Hello David!
> > > > > 
> > > > > Hi Alexey,
> > > > > 
> > > > > > I have checked you series with 1G hugepage, but only in 1 Gbit/sec 
> > > > > > network
> > > > > > environment.
> > > > > 
> > > > > Can you show the qemu command line you're using?  I'm just trying
> > > > > to make sure I understand where your hugepages are; running 1G 
> > > > > hostpages
> > > > > across a 1Gbit/sec network for postcopy would be pretty poor - it 
> > > > > would take
> > > > > ~10 seconds to transfer the page.
> > > > 
> > > > sure
> > > > -hda ./Ubuntu.img -name PAU,debug-threads=on -boot d -net nic -net user
> > > > -m 1024 -localtime -nographic -enable-kvm -incoming tcp:0: -object
> > > > memory-backend-file,id=mem,size=1G,mem-path=/dev/hugepages -mem-prealloc
> > > > -numa node,memdev=mem -trace events=/tmp/events -chardev
> > > > socket,id=charmonitor,path=/var/lib/migrate-vm-monitor.sock,server,nowait
> > > > -mon chardev=charmonitor,id=monitor,mode=control
> > > 
> > > OK, it's a pretty unusual setup - a 1G page guest with 1G of guest RAM.
> > > 
> > > > > 
> > > > > > I started Ubuntu just with console interface and gave to it only 1G 
> > > > > > of
> > > > > > RAM, inside Ubuntu I started stress command
> > > > > 
> > > > > > (stress --cpu 4 --io 4 --vm 4 --vm-bytes 25600 &)
> > > > > > in such environment precopy live migration was impossible, it never
> > > > > > being finished, in this case it infinitely sends pages (it looks 
> > > > > > like
> > > > > > dpkg scenario).
> > > > > > 
> > > > > > Also I modified stress utility
> > > > > > http://people.seas.harvard.edu/~apw/stress/stress-1.0.4.tar.gz
> > > > > > due to it wrote into memory every time the same value `Z`. My
> > > > > > modified version writes every allocation new incremented value.
> > > > > 
> > > > > I use google's stressapptest normally; although remember to turn
> > > > > off the bit where it pauses.
> > > > 
> > > > I decided to use it too
> > > > stressapptest -s 300 -M 256 -m 8 -W
> > > > 
> > > > > 
> > > > > > I'm using Arcangeli's kernel only at the destination.
> > > > > > 
> > > > > > I got controversial results. Downtime for 1G hugepage is close to 
> > > > > > 2Mb
> > > > > > hugepage and it took around 7 ms (in 2Mb hugepage scenario downtime 
> > > > > > was
> > > > > > around 8 ms).
> > > > > > I made that opinion by query-migrate.
> > > > > > {"return": {"status": "completed", "setup-time": 6, "downtime": 6, 
> > > > > > "total-time": 9668, "ram": {"total": 1091379200, 
> > > > > > "postcopy-requests": 1, "dirty-sync-count": 2, "remaining": 0, 
> > > > > > "mbps": 879.786851, "transferred": 1063007296, "duplicate": 7449, 
> > > > > > "dirty-pages-rate": 0, "skipped": 0, "normal-bytes": 1060868096, 
> > > > > > "normal": 259001}}}
> > > > > > 
> > > > > > Documentation says about downtime field - measurement unit is ms.
> > > > > 
> > > > > The downtime measurement field is pretty meaningless for postcopy; 
> > > > > it's only
> > > > > the time from stopping the VM until the point where we tell the 
> > > > > destination it
> > > > > can start running.  Meaningful measurements are only from inside the 
> > > > > guest
> > > > > really, or the place latencys.
> > > > >
> > > > 
> > > > Maybe improve it by receiving such information from destination?
> > > > I wish to do that.
> > > > > > So I traced it (I added additional trace into postcopy_place_page
> > > > > > trace_postcopy_place_page_start(host, from, pagesize); )
> > > > > > 
> > > > > > postcopy_ram_fault_thread_request Request for HVA=7f6dc000 
> > > > > > rb=/objects/mem offset=0
> > > > > > postcopy_place_page_start host=0x7f6dc000 from=0x7f6d7000, 
> > > > > > pagesize=4000
> > > > > > postcopy_place_page_start host=0x7f6e0e80 from=0x55b665969619, 
> > > > > > pagesize=1000
> > > > > > postcopy_place_page_start host=0x7f6e0e801000 from=0x55b6659684e8, 
> > > > > > pagesize=1000
> > > > > > several pages with 4Kb step ...
> > > > > > postcopy_place_page_start host=0x7f6e0e817000 from=0x55b6659694f0, 
> > > > > > pagesize=1000
> > > > > > 
> > > > > > 4K pages, started from 0x7f6e0e80 address it's
> > > > > > vga.ram, /rom@etc/acpi/tables etc.
> > > > > > 
> > > > > > Frankly saying, right now, I don't have any ideas why hugepage 
> > > > > > wasn't
> > > > > > resent. Maybe my expectation of it is wrong as well as 
> > > > > > understanding )
> > > > > 
> > > > > That's pretty much what I expect to see 

Re: [Qemu-devel] [PATCH] os: don't corrupt pre-existing memory-backend data with prealloc

[Stefan Hajnoczi]

On Thu, Feb 23, 2017 at 10:59:22AM +, Daniel P. Berrange wrote:
> When using a memory-backend object with prealloc turned on, QEMU
> will memset() the first byte in every memory page to zero. While
> this might have been acceptable for memory backends associated
> with RAM, this corrupts application data for NVDIMMs.
> 
> Instead of setting every page to zero, read the current byte
> value and then just write that same value back, so we are not
> corrupting the original data.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
> 
> I'm unclear if this is actually still safe in practice ? Is the
> compiler permitted to optimize away the read+write since it doesn't
> change the memory value. I'd hope not, but I've been surprised
> before...
> 
> IMHO this is another factor in favour of requesting an API from
> the kernel to provide the prealloc behaviour we want.
> 
>  util/oslib-posix.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> index 35012b9..8f5b656 100644
> --- a/util/oslib-posix.c
> +++ b/util/oslib-posix.c
> @@ -355,7 +355,8 @@ void os_mem_prealloc(int fd, char *area, size_t memory, 
> Error **errp)
>  
>  /* MAP_POPULATE silently ignores failures */
>  for (i = 0; i < numpages; i++) {
> -memset(area + (hpagesize * i), 0, 1);
> +char val = *(area + (hpagesize * i));
> +memset(area + (hpagesize * i), 0, val);

Please include a comment in the final patch explaining why we want to
preserve memory contents.

In the case of NVDIMM I'm not sure if the memset is needed at all.  The
memory already exists - no new pages need to be allocated by the kernel.
We just want the page table entries to be populated for the NVDIMM when
-mem-prealloc is used.

Perhaps Andrea or Rik have ideas on improving the kernel interface and
whether mmap(MAP_POPULATE) should be used with NVDIMM instead of this
userspace "touch every page" workaround?

Stefan


signature.asc
Description: PGP signature

[Qemu-devel] [PATCH] tests-aio-multithread: use atomic_read properly

[Paolo Bonzini]

nodes[id].next is written by other threads.  If atomic_read is not used
(matching atomic_set in mcs_mutex_lock!) the compiler can optimize the
whole "if" away!

Reported-by: Alex Bennée 
Signed-off-by: Paolo Bonzini 
---
 tests/test-aio-multithread.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/test-aio-multithread.c b/tests/test-aio-multithread.c
index f11e990..8b0b40e 100644
--- a/tests/test-aio-multithread.c
+++ b/tests/test-aio-multithread.c
@@ -309,7 +309,7 @@ static void mcs_mutex_lock(void)
 static void mcs_mutex_unlock(void)
 {
 int next;
-if (nodes[id].next == -1) {
+if (atomic_read(&nodes[id].next) == -1) {
 if (atomic_read(&mutex_head) == id &&
 atomic_cmpxchg(&mutex_head, id, -1) == id) {
 /* Last item in the list, exit.  */
@@ -323,7 +323,7 @@ static void mcs_mutex_unlock(void)
 }
 
 /* Wake up the next in line.  */
-next = nodes[id].next;
+next = atomic_read(&nodes[id].next);
 nodes[next].locked = 0;
 qemu_futex_wake(&nodes[next].locked, 1);
 }
-- 
2.9.3

[Qemu-devel] [PATCH 11/24] test-qobject-input-visitor: Abort earlier on bad test input

[Markus Armbruster]

visitor_input_test_init_internal() parses test input with
qobject_from_jsonv(), and asserts it succeeds.  Pass &error_abort for
good measure.

Signed-off-by: Markus Armbruster 
---
 tests/test-qobject-input-visitor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/test-qobject-input-visitor.c 
b/tests/test-qobject-input-visitor.c
index 44885ee..746dfa3 100644
--- a/tests/test-qobject-input-visitor.c
+++ b/tests/test-qobject-input-visitor.c
@@ -51,7 +51,7 @@ static Visitor 
*visitor_input_test_init_internal(TestInputVisitorData *data,
 {
 visitor_input_teardown(data, NULL);
 
-data->obj = qobject_from_jsonv(json_string, ap, NULL);
+data->obj = qobject_from_jsonv(json_string, ap, &error_abort);
 g_assert(data->obj);
 
 if (keyval) {
-- 
2.7.4

[Qemu-devel] [PATCH 10/24] qjson: Abort earlier on qobject_from_jsonf() misuse

[Markus Armbruster]

Ignoring errors first, then asserting success is suboptimal.  Pass
&error_abort instead, so we abort earlier, and hopefully get more
useful clues on what's wrong.

Signed-off-by: Markus Armbruster 
---
 qobject/qjson.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qobject/qjson.c b/qobject/qjson.c
index 339c9f7..c98d6a7 100644
--- a/qobject/qjson.c
+++ b/qobject/qjson.c
@@ -65,7 +65,7 @@ QObject *qobject_from_jsonf(const char *string, ...)
 va_list ap;
 
 va_start(ap, string);
-obj = qobject_from_jsonv(string, &ap, NULL);
+obj = qobject_from_jsonv(string, &ap, &error_abort);
 va_end(ap);
 
 assert(obj != NULL);
-- 
2.7.4

[Qemu-devel] [PATCH 02/24] tests: Fix gcov-files-test-qemu-opts-y, gcov-files-test-logging-y

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
---
 tests/Makefile.include | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/Makefile.include b/tests/Makefile.include
index cb97473..fdf528c 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -93,7 +93,7 @@ gcov-files-check-qom-interface-y = qom/object.c
 check-unit-y += tests/check-qom-proplist$(EXESUF)
 gcov-files-check-qom-proplist-y = qom/object.c
 check-unit-y += tests/test-qemu-opts$(EXESUF)
-gcov-files-test-qemu-opts-y = qom/test-qemu-opts.c
+gcov-files-test-qemu-opts-y = util/qemu-option.c
 check-unit-y += tests/test-write-threshold$(EXESUF)
 gcov-files-test-write-threshold-y = block/write-threshold.c
 check-unit-y += tests/test-crypto-hash$(EXESUF)
@@ -118,8 +118,8 @@ check-unit-y += tests/test-crypto-ivgen$(EXESUF)
 check-unit-y += tests/test-crypto-afsplit$(EXESUF)
 check-unit-y += tests/test-crypto-xts$(EXESUF)
 check-unit-y += tests/test-crypto-block$(EXESUF)
-gcov-files-test-logging-y = tests/test-logging.c
 check-unit-y += tests/test-logging$(EXESUF)
+gcov-files-test-logging-y = util/log.c
 check-unit-$(CONFIG_REPLICATION) += tests/test-replication$(EXESUF)
 check-unit-y += tests/test-bufferiszero$(EXESUF)
 gcov-files-check-bufferiszero-y = util/bufferiszero.c
-- 
2.7.4

[Qemu-devel] [PATCH 01/24] test-qemu-opts: Cover qemu_opts_parse() of "no"

[Markus Armbruster]

qemu_opts_parse() interprets "no" as negated empty key.  Consistent
with its acceptance of empty keys elsewhere, whatever that's worth.

Signed-off-by: Markus Armbruster 
---
 tests/test-qemu-opts.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
index c46ef31..f6310b3 100644
--- a/tests/test-qemu-opts.c
+++ b/tests/test-qemu-opts.c
@@ -532,6 +532,11 @@ static void test_opts_parse(void)
 g_assert_cmpstr(qemu_opt_get(opts, "aus"), ==, "off");
 g_assert_cmpstr(qemu_opt_get(opts, "noaus"), ==, "");
 
+/* Implied value, negated empty key */
+opts = qemu_opts_parse(&opts_list_03, "no", false, &error_abort);
+g_assert_cmpuint(opts_count(opts), ==, 1);
+g_assert_cmpstr(qemu_opt_get(opts, ""), ==, "off");
+
 /* Implied key */
 opts = qemu_opts_parse(&opts_list_03, "an,noaus,noaus=", true,
&error_abort);
-- 
2.7.4

[Qemu-devel] [PATCH 15/24] test-visitor-serialization: Pass &error_abort to qobject_from_json()

[Markus Armbruster]

qmp_deserialize() calls qobject_from_json() ignoring errors.  It
passes the result to qobject_input_visitor_new(), which asserts it's
not null.  Therefore, we can just as well pass &error_abort to
qobject_from_json().

Signed-off-by: Markus Armbruster 
---
 tests/test-visitor-serialization.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/test-visitor-serialization.c 
b/tests/test-visitor-serialization.c
index 37dff41..4d47cee 100644
--- a/tests/test-visitor-serialization.c
+++ b/tests/test-visitor-serialization.c
@@ -1037,7 +1037,7 @@ static void qmp_deserialize(void **native_out, void 
*datap,
 visit_complete(d->qov, &d->obj);
 obj_orig = d->obj;
 output_json = qobject_to_json(obj_orig);
-obj = qobject_from_json(qstring_get_str(output_json), NULL);
+obj = qobject_from_json(qstring_get_str(output_json), &error_abort);
 
 QDECREF(output_json);
 d->qiv = qobject_input_visitor_new(obj);
-- 
2.7.4

[Qemu-devel] [PATCH 16/24] monitor: Assert qmp_schema_json[] is sane

[Markus Armbruster]

qmp_query_qmp_schema() parses qmp_schema_json[] with
qobject_from_json().  This must not fail, so pass &error_abort.

Signed-off-by: Markus Armbruster 
---
 monitor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/monitor.c b/monitor.c
index 858bcda..13f6133 100644
--- a/monitor.c
+++ b/monitor.c
@@ -950,7 +950,7 @@ EventInfoList *qmp_query_events(Error **errp)
 static void qmp_query_qmp_schema(QDict *qdict, QObject **ret_data,
  Error **errp)
 {
-*ret_data = qobject_from_json(qmp_schema_json, NULL);
+*ret_data = qobject_from_json(qmp_schema_json, &error_abort);
 }
 
 /*
-- 
2.7.4

[Qemu-devel] [PATCH 07/24] qapi: Factor out common qobject_input_get_keyval()

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
---
 qapi/qobject-input-visitor.c | 87 ++--
 1 file changed, 35 insertions(+), 52 deletions(-)

diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index e47615e..3db5850 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -151,6 +151,28 @@ static QObject 
*qobject_input_get_object(QObjectInputVisitor *qiv,
 return obj;
 }
 
+static const char *qobject_input_get_keyval(QObjectInputVisitor *qiv,
+const char *name,
+Error **errp)
+{
+QObject *qobj;
+QString *qstr;
+
+qobj = qobject_input_get_object(qiv, name, true, errp);
+if (!qobj) {
+return NULL;
+}
+
+qstr = qobject_to_qstring(qobj);
+if (!qstr) {
+error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
+   full_name(qiv, name), "string");
+return NULL;
+}
+
+return qstring_get_str(qstr);
+}
+
 static void qdict_add_key(const char *key, QObject *obj, void *opaque)
 {
 GHashTable *h = opaque;
@@ -342,20 +364,13 @@ static void qobject_input_type_int64_keyval(Visitor *v, 
const char *name,
 int64_t *obj, Error **errp)
 {
 QObjectInputVisitor *qiv = to_qiv(v);
-QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
-QString *qstr;
+const char *str = qobject_input_get_keyval(qiv, name, errp);
 
-if (!qobj) {
-return;
-}
-qstr = qobject_to_qstring(qobj);
-if (!qstr) {
-error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
-   full_name(qiv, name), "string");
+if (!str) {
 return;
 }
 
-if (qemu_strtoi64(qstring_get_str(qstr), NULL, 0, obj) < 0) {
+if (qemu_strtoi64(str, NULL, 0, obj) < 0) {
 /* TODO report -ERANGE more nicely */
 error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
full_name(qiv, name), "integer");
@@ -387,20 +402,13 @@ static void qobject_input_type_uint64_keyval(Visitor *v, 
const char *name,
  uint64_t *obj, Error **errp)
 {
 QObjectInputVisitor *qiv = to_qiv(v);
-QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
-QString *qstr;
+const char *str = qobject_input_get_keyval(qiv, name, errp);
 
-if (!qobj) {
-return;
-}
-qstr = qobject_to_qstring(qobj);
-if (!qstr) {
-error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
-   full_name(qiv, name), "string");
+if (!str) {
 return;
 }
 
-if (qemu_strtou64(qstring_get_str(qstr), NULL, 0, obj) < 0) {
+if (qemu_strtou64(str, NULL, 0, obj) < 0) {
 /* TODO report -ERANGE more nicely */
 error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
full_name(qiv, name), "integer");
@@ -431,21 +439,12 @@ static void qobject_input_type_bool_keyval(Visitor *v, 
const char *name,
bool *obj, Error **errp)
 {
 QObjectInputVisitor *qiv = to_qiv(v);
-QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
-QString *qstr;
-const char *str;
+const char *str = qobject_input_get_keyval(qiv, name, errp);
 
-if (!qobj) {
-return;
-}
-qstr = qobject_to_qstring(qobj);
-if (!qstr) {
-error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
-   full_name(qiv, name), "string");
+if (!str) {
 return;
 }
 
-str = qstring_get_str(qstr);
 if (!strcmp(str, "on")) {
 *obj = true;
 } else if (!strcmp(str, "off")) {
@@ -508,22 +507,13 @@ static void qobject_input_type_number_keyval(Visitor *v, 
const char *name,
  double *obj, Error **errp)
 {
 QObjectInputVisitor *qiv = to_qiv(v);
-QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
-QString *qstr;
-const char *str;
+const char *str = qobject_input_get_keyval(qiv, name, errp);
 char *endp;
 
-if (!qobj) {
-return;
-}
-qstr = qobject_to_qstring(qobj);
-if (!qstr) {
-error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
-   full_name(qiv, name), "string");
+if (!str) {
 return;
 }
 
-str = qstring_get_str(qstr);
 errno = 0;
 *obj = strtod(str, &endp);
 if (errno || endp == str || *endp) {
@@ -567,20 +557,13 @@ static void qobject_input_type_size_keyval(Visitor *v, 
const char *name,
uint64_t *obj, Error **errp)
 {
 QObjectInputVisitor *qiv = to_qiv(v);
-QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
-QString *qstr;
+const char *str = qobject_input_get_keyval(qiv, name, errp);
 
-if (!qobj) {
-return;
-}
-qstr = qobject_to_qstring(qobj);
-if (!qstr) {
-error

[Qemu-devel] [PATCH 08/24] qobject: Propagate parse errors through qobject_from_jsonv()

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
---
 include/qapi/qmp/qjson.h   |  3 ++-
 qobject/qjson.c| 12 
 tests/libqtest.c   |  2 +-
 tests/test-qobject-input-visitor.c |  2 +-
 4 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/include/qapi/qmp/qjson.h b/include/qapi/qmp/qjson.h
index 02b1f2c..6fe42d0 100644
--- a/include/qapi/qmp/qjson.h
+++ b/include/qapi/qmp/qjson.h
@@ -19,7 +19,8 @@
 
 QObject *qobject_from_json(const char *string);
 QObject *qobject_from_jsonf(const char *string, ...) GCC_FMT_ATTR(1, 2);
-QObject *qobject_from_jsonv(const char *string, va_list *ap) GCC_FMT_ATTR(1, 
0);
+QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
+GCC_FMT_ATTR(1, 0);
 
 QString *qobject_to_json(const QObject *obj);
 QString *qobject_to_json_pretty(const QObject *obj);
diff --git a/qobject/qjson.c b/qobject/qjson.c
index 9a0de89..339c9f7 100644
--- a/qobject/qjson.c
+++ b/qobject/qjson.c
@@ -12,6 +12,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qapi/error.h"
 #include "qapi/qmp/json-lexer.h"
 #include "qapi/qmp/json-parser.h"
 #include "qapi/qmp/json-streamer.h"
@@ -24,15 +25,17 @@ typedef struct JSONParsingState
 JSONMessageParser parser;
 va_list *ap;
 QObject *result;
+Error *err;
 } JSONParsingState;
 
 static void parse_json(JSONMessageParser *parser, GQueue *tokens)
 {
 JSONParsingState *s = container_of(parser, JSONParsingState, parser);
-s->result = json_parser_parse(tokens, s->ap);
+
+s->result = json_parser_parse_err(tokens, s->ap, &s->err);
 }
 
-QObject *qobject_from_jsonv(const char *string, va_list *ap)
+QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
 {
 JSONParsingState state = {};
 
@@ -43,12 +46,13 @@ QObject *qobject_from_jsonv(const char *string, va_list *ap)
 json_message_parser_flush(&state.parser);
 json_message_parser_destroy(&state.parser);
 
+error_propagate(errp, state.err);
 return state.result;
 }
 
 QObject *qobject_from_json(const char *string)
 {
-return qobject_from_jsonv(string, NULL);
+return qobject_from_jsonv(string, NULL, NULL);
 }
 
 /*
@@ -61,7 +65,7 @@ QObject *qobject_from_jsonf(const char *string, ...)
 va_list ap;
 
 va_start(ap, string);
-obj = qobject_from_jsonv(string, &ap);
+obj = qobject_from_jsonv(string, &ap, NULL);
 va_end(ap);
 
 assert(obj != NULL);
diff --git a/tests/libqtest.c b/tests/libqtest.c
index cf27afc..683d5e3 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -442,7 +442,7 @@ void qmp_fd_sendv(int fd, const char *fmt, va_list ap)
  * is an array type.
  */
 va_copy(ap_copy, ap);
-qobj = qobject_from_jsonv(fmt, &ap_copy);
+qobj = qobject_from_jsonv(fmt, &ap_copy, NULL);
 va_end(ap_copy);
 
 /* No need to send anything for an empty QObject.  */
diff --git a/tests/test-qobject-input-visitor.c 
b/tests/test-qobject-input-visitor.c
index 650637e..44885ee 100644
--- a/tests/test-qobject-input-visitor.c
+++ b/tests/test-qobject-input-visitor.c
@@ -51,7 +51,7 @@ static Visitor 
*visitor_input_test_init_internal(TestInputVisitorData *data,
 {
 visitor_input_teardown(data, NULL);
 
-data->obj = qobject_from_jsonv(json_string, ap);
+data->obj = qobject_from_jsonv(json_string, ap, NULL);
 g_assert(data->obj);
 
 if (keyval) {
-- 
2.7.4

[Qemu-devel] [PATCH 21/24] test-qapi-util: New, covering qapi/qapi-util.c

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
---
 tests/.gitignore   |  1 +
 tests/Makefile.include |  3 +++
 tests/test-qapi-util.c | 51 ++
 3 files changed, 55 insertions(+)
 create mode 100644 tests/test-qapi-util.c

diff --git a/tests/.gitignore b/tests/.gitignore
index 30b7740..a966740 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -53,6 +53,7 @@ test-mul64
 test-opts-visitor
 test-qapi-event.[ch]
 test-qapi-types.[ch]
+test-qapi-util
 test-qapi-visit.[ch]
 test-qdev-global-props
 test-qemu-opts
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 2171e4a..2b8301e 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -128,6 +128,8 @@ gcov-files-check-bufferiszero-y = util/bufferiszero.c
 check-unit-y += tests/test-uuid$(EXESUF)
 check-unit-y += tests/ptimer-test$(EXESUF)
 gcov-files-ptimer-test-y = hw/core/ptimer.c
+check-unit-y += tests/test-qapi-util$(EXESUF)
+gcov-files-test-qapi-util-y = qapi/qapi-util.c
 
 check-block-$(CONFIG_POSIX) += tests/qemu-iotests-quick.sh
 
@@ -733,6 +735,7 @@ tests/ivshmem-test$(EXESUF): tests/ivshmem-test.o 
contrib/ivshmem-server/ivshmem
 tests/vhost-user-bridge$(EXESUF): tests/vhost-user-bridge.o 
contrib/libvhost-user/libvhost-user.o $(test-util-obj-y)
 tests/test-uuid$(EXESUF): tests/test-uuid.o $(test-util-obj-y)
 tests/test-arm-mptimer$(EXESUF): tests/test-arm-mptimer.o
+tests/test-qapi-util$(EXESUF): tests/test-qapi-util.o $(test-util-obj-y)
 
 tests/migration/stress$(EXESUF): tests/migration/stress.o
$(call quiet-command, $(LINKPROG) -static -O3 $(PTHREAD_LIB) -o $@ $< 
,"LINK","$(TARGET_DIR)$@")
diff --git a/tests/test-qapi-util.c b/tests/test-qapi-util.c
new file mode 100644
index 000..39db8bf
--- /dev/null
+++ b/tests/test-qapi-util.c
@@ -0,0 +1,51 @@
+/*
+ * Unit tests for QAPI utility functions
+ *
+ * Copyright (C) 2017 Red Hat Inc.
+ *
+ * Authors:
+ *  Markus Armbruster ,
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qapi/util.h"
+#include "test-qapi-types.h"
+
+static void test_qapi_enum_parse(void)
+{
+Error *err = NULL;
+int ret;
+
+ret = qapi_enum_parse(QType_lookup, NULL, QTYPE__MAX, QTYPE_NONE,
+  &error_abort);
+g_assert_cmpint(ret, ==, QTYPE_NONE);
+
+ret = qapi_enum_parse(QType_lookup, "junk", QTYPE__MAX, -1,
+  NULL);
+g_assert_cmpint(ret, ==, -1);
+
+ret = qapi_enum_parse(QType_lookup, "junk", QTYPE__MAX, -1,
+  &err);
+error_free_or_abort(&err);
+
+ret = qapi_enum_parse(QType_lookup, "none", QTYPE__MAX, -1,
+  &error_abort);
+g_assert_cmpint(ret, ==, QTYPE_NONE);
+
+ret = qapi_enum_parse(QType_lookup, QType_lookup[QTYPE__MAX - 1],
+  QTYPE__MAX, QTYPE__MAX - 1,
+  &error_abort);
+g_assert_cmpint(ret, ==, QTYPE__MAX - 1);
+}
+
+int main(int argc, char *argv[])
+{
+g_test_init(&argc, &argv, NULL);
+g_test_add_func("/qapi/util/qapi_enum_parse", test_qapi_enum_parse);
+g_test_run();
+return 0;
+}
-- 
2.7.4

[Qemu-devel] [PATCH 12/24] qobject: Propagate parse errors through qobject_from_json()

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
---
 block.c|  2 +-
 include/qapi/qmp/qjson.h   |  5 +--
 monitor.c  |  2 +-
 qobject/qjson.c|  4 +--
 tests/check-qjson.c| 62 +++---
 tests/test-visitor-serialization.c |  2 +-
 6 files changed, 39 insertions(+), 38 deletions(-)

diff --git a/block.c b/block.c
index 3c36af5..aa6790c 100644
--- a/block.c
+++ b/block.c
@@ -1163,7 +1163,7 @@ static QDict *parse_json_filename(const char *filename, 
Error **errp)
 ret = strstart(filename, "json:", &filename);
 assert(ret);
 
-options_obj = qobject_from_json(filename);
+options_obj = qobject_from_json(filename, NULL);
 if (!options_obj) {
 error_setg(errp, "Could not parse the JSON options");
 return NULL;
diff --git a/include/qapi/qmp/qjson.h b/include/qapi/qmp/qjson.h
index 6fe42d0..8568f2d 100644
--- a/include/qapi/qmp/qjson.h
+++ b/include/qapi/qmp/qjson.h
@@ -17,8 +17,9 @@
 #include "qapi/qmp/qobject.h"
 #include "qapi/qmp/qstring.h"
 
-QObject *qobject_from_json(const char *string);
-QObject *qobject_from_jsonf(const char *string, ...) GCC_FMT_ATTR(1, 2);
+QObject *qobject_from_json(const char *string, Error **errp);
+QObject *qobject_from_jsonf(const char *string, ...)
+GCC_FMT_ATTR(1, 2);
 QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
 GCC_FMT_ATTR(1, 0);
 
diff --git a/monitor.c b/monitor.c
index 97b73ab..858bcda 100644
--- a/monitor.c
+++ b/monitor.c
@@ -950,7 +950,7 @@ EventInfoList *qmp_query_events(Error **errp)
 static void qmp_query_qmp_schema(QDict *qdict, QObject **ret_data,
  Error **errp)
 {
-*ret_data = qobject_from_json(qmp_schema_json);
+*ret_data = qobject_from_json(qmp_schema_json, NULL);
 }
 
 /*
diff --git a/qobject/qjson.c b/qobject/qjson.c
index c98d6a7..b2f3bfe 100644
--- a/qobject/qjson.c
+++ b/qobject/qjson.c
@@ -50,9 +50,9 @@ QObject *qobject_from_jsonv(const char *string, va_list *ap, 
Error **errp)
 return state.result;
 }
 
-QObject *qobject_from_json(const char *string)
+QObject *qobject_from_json(const char *string, Error **errp)
 {
-return qobject_from_jsonv(string, NULL, NULL);
+return qobject_from_jsonv(string, NULL, errp);
 }
 
 /*
diff --git a/tests/check-qjson.c b/tests/check-qjson.c
index e6d6935..aa63758 100644
--- a/tests/check-qjson.c
+++ b/tests/check-qjson.c
@@ -53,7 +53,7 @@ static void escaped_string(void)
 QObject *obj;
 QString *str;
 
-obj = qobject_from_json(test_cases[i].encoded);
+obj = qobject_from_json(test_cases[i].encoded, NULL);
 str = qobject_to_qstring(obj);
 g_assert(str);
 g_assert_cmpstr(qstring_get_str(str), ==, test_cases[i].decoded);
@@ -85,7 +85,7 @@ static void simple_string(void)
 QObject *obj;
 QString *str;
 
-obj = qobject_from_json(test_cases[i].encoded);
+obj = qobject_from_json(test_cases[i].encoded, NULL);
 str = qobject_to_qstring(obj);
 g_assert(str);
 g_assert(strcmp(qstring_get_str(str), test_cases[i].decoded) == 0);
@@ -116,7 +116,7 @@ static void single_quote_string(void)
 QObject *obj;
 QString *str;
 
-obj = qobject_from_json(test_cases[i].encoded);
+obj = qobject_from_json(test_cases[i].encoded, NULL);
 str = qobject_to_qstring(obj);
 g_assert(str);
 g_assert(strcmp(qstring_get_str(str), test_cases[i].decoded) == 0);
@@ -809,7 +809,7 @@ static void utf8_string(void)
 utf8_in = test_cases[i].utf8_in ?: test_cases[i].utf8_out;
 json_out = test_cases[i].json_out ?: test_cases[i].json_in;
 
-obj = qobject_from_json(json_in);
+obj = qobject_from_json(json_in, NULL);
 if (utf8_out) {
 str = qobject_to_qstring(obj);
 g_assert(str);
@@ -836,7 +836,7 @@ static void utf8_string(void)
  * FIXME Enable once these bugs have been fixed.
  */
 if (0 && json_out != json_in) {
-obj = qobject_from_json(json_out);
+obj = qobject_from_json(json_out, NULL);
 str = qobject_to_qstring(obj);
 g_assert(str);
 g_assert_cmpstr(qstring_get_str(str), ==, utf8_out);
@@ -886,7 +886,7 @@ static void simple_number(void)
 for (i = 0; test_cases[i].encoded; i++) {
 QInt *qint;
 
-qint = qobject_to_qint(qobject_from_json(test_cases[i].encoded));
+qint = qobject_to_qint(qobject_from_json(test_cases[i].encoded, NULL));
 g_assert(qint);
 g_assert(qint_get_int(qint) == test_cases[i].decoded);
 if (test_cases[i].skip == 0) {
@@ -920,7 +920,7 @@ static void float_number(void)
 QObject *obj;
 QFloat *qfloat;
 
-obj = qobject_from_json(test_cases[i].encoded);
+obj = qobject_from_json(test_cases[i].encoded, NULL);
 qfloat = qobject_to_q

[Qemu-devel] [PATCH 14/24] check-qjson: Test errors from qobject_from_json()

[Markus Armbruster]

Pass &error_abort with known-good input.  Else pass &err and check
what comes back.  This demonstrates that the parser fails silently for
many errors.

Signed-off-by: Markus Armbruster 
---
 tests/check-qjson.c | 88 ++---
 1 file changed, 56 insertions(+), 32 deletions(-)

diff --git a/tests/check-qjson.c b/tests/check-qjson.c
index aa63758..963dd46 100644
--- a/tests/check-qjson.c
+++ b/tests/check-qjson.c
@@ -10,8 +10,10 @@
  * See the COPYING.LIB file in the top-level directory.
  *
  */
+
 #include "qemu/osdep.h"
 
+#include "qapi/error.h"
 #include "qapi/qmp/types.h"
 #include "qapi/qmp/qjson.h"
 #include "qemu-common.h"
@@ -53,7 +55,7 @@ static void escaped_string(void)
 QObject *obj;
 QString *str;
 
-obj = qobject_from_json(test_cases[i].encoded, NULL);
+obj = qobject_from_json(test_cases[i].encoded, &error_abort);
 str = qobject_to_qstring(obj);
 g_assert(str);
 g_assert_cmpstr(qstring_get_str(str), ==, test_cases[i].decoded);
@@ -85,7 +87,7 @@ static void simple_string(void)
 QObject *obj;
 QString *str;
 
-obj = qobject_from_json(test_cases[i].encoded, NULL);
+obj = qobject_from_json(test_cases[i].encoded, &error_abort);
 str = qobject_to_qstring(obj);
 g_assert(str);
 g_assert(strcmp(qstring_get_str(str), test_cases[i].decoded) == 0);
@@ -116,7 +118,7 @@ static void single_quote_string(void)
 QObject *obj;
 QString *str;
 
-obj = qobject_from_json(test_cases[i].encoded, NULL);
+obj = qobject_from_json(test_cases[i].encoded, &error_abort);
 str = qobject_to_qstring(obj);
 g_assert(str);
 g_assert(strcmp(qstring_get_str(str), test_cases[i].decoded) == 0);
@@ -809,7 +811,7 @@ static void utf8_string(void)
 utf8_in = test_cases[i].utf8_in ?: test_cases[i].utf8_out;
 json_out = test_cases[i].json_out ?: test_cases[i].json_in;
 
-obj = qobject_from_json(json_in, NULL);
+obj = qobject_from_json(json_in, utf8_out ? &error_abort : NULL);
 if (utf8_out) {
 str = qobject_to_qstring(obj);
 g_assert(str);
@@ -836,7 +838,7 @@ static void utf8_string(void)
  * FIXME Enable once these bugs have been fixed.
  */
 if (0 && json_out != json_in) {
-obj = qobject_from_json(json_out, NULL);
+obj = qobject_from_json(json_out, &error_abort);
 str = qobject_to_qstring(obj);
 g_assert(str);
 g_assert_cmpstr(qstring_get_str(str), ==, utf8_out);
@@ -886,7 +888,8 @@ static void simple_number(void)
 for (i = 0; test_cases[i].encoded; i++) {
 QInt *qint;
 
-qint = qobject_to_qint(qobject_from_json(test_cases[i].encoded, NULL));
+qint = qobject_to_qint(qobject_from_json(test_cases[i].encoded,
+ &error_abort));
 g_assert(qint);
 g_assert(qint_get_int(qint) == test_cases[i].decoded);
 if (test_cases[i].skip == 0) {
@@ -920,7 +923,7 @@ static void float_number(void)
 QObject *obj;
 QFloat *qfloat;
 
-obj = qobject_from_json(test_cases[i].encoded, NULL);
+obj = qobject_from_json(test_cases[i].encoded, &error_abort);
 qfloat = qobject_to_qfloat(obj);
 g_assert(qfloat);
 g_assert(qfloat_get_double(qfloat) == test_cases[i].decoded);
@@ -965,7 +968,7 @@ static void keyword_literal(void)
 QObject *null;
 QString *str;
 
-obj = qobject_from_json("true", NULL);
+obj = qobject_from_json("true", &error_abort);
 qbool = qobject_to_qbool(obj);
 g_assert(qbool);
 g_assert(qbool_get_bool(qbool) == true);
@@ -976,7 +979,7 @@ static void keyword_literal(void)
 
 QDECREF(qbool);
 
-obj = qobject_from_json("false", NULL);
+obj = qobject_from_json("false", &error_abort);
 qbool = qobject_to_qbool(obj);
 g_assert(qbool);
 g_assert(qbool_get_bool(qbool) == false);
@@ -998,7 +1001,7 @@ static void keyword_literal(void)
 g_assert(qbool_get_bool(qbool) == true);
 QDECREF(qbool);
 
-obj = qobject_from_json("null", NULL);
+obj = qobject_from_json("null", &error_abort);
 g_assert(obj != NULL);
 g_assert(qobject_type(obj) == QTYPE_QNULL);
 
@@ -1134,13 +1137,13 @@ static void simple_dict(void)
 QObject *obj;
 QString *str;
 
-obj = qobject_from_json(test_cases[i].encoded, NULL);
+obj = qobject_from_json(test_cases[i].encoded, &error_abort);
 g_assert(compare_litqobj_to_qobj(&test_cases[i].decoded, obj) == 1);
 
 str = qobject_to_json(obj);
 qobject_decref(obj);
 
-obj = qobject_from_json(qstring_get_str(str), NULL);
+obj = qobject_from_json(qstring_get_str(str), &error_abort);
 g_assert(compare_litqobj_to_qobj(&test_cases[i].decoded, obj) == 1);
 qobject_decref(obj);
 QDECREF(s

[Qemu-devel] [PATCH 00/24] block: Command line option -blockdev

[Markus Armbruster]

Actually, the command line option is the least part of this series.
Its bulk is about building infrastructure and getting errors out of
the JSON parser[*].  The latter part could be spun out in its own
series, if that helps.  We'll see.

The design of the command line interface was discussed here:
Subject: Non-flat command line option argument syntax
Message-ID: <87bmukmlau@dusky.pond.sub.org>
https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg00555.html

This series is based on "[PATCH v2 00/26] qapi: QMP dispatch and input
visitor work".  Available as branch blockdev-cmdline in my public repo
, except that one still has a
patch to add implied value sugar, followed by its revert.

It supersedes "[PATCH RFC v3 0/5] block: Crude initial implementation
of -blockdev".

[*] Who'd have thunk anyone actually wants to know what's wrong with
his JSON!

Daniel P. Berrange (1):
  qapi: qobject input visitor variant for use with keyval_parse()

Markus Armbruster (23):
  test-qemu-opts: Cover qemu_opts_parse() of "no"
  tests: Fix gcov-files-test-qemu-opts-y, gcov-files-test-logging-y
  keyval: New keyval_parse()
  test-keyval: Cover use with qobject input visitor
  qapi: Factor out common part of qobject input visitor creation
  qapi: Factor out common qobject_input_get_keyval()
  qobject: Propagate parse errors through qobject_from_jsonv()
  libqtest: Fix qmp() & friends to abort on JSON parse errors
  qjson: Abort earlier on qobject_from_jsonf() misuse
  test-qobject-input-visitor: Abort earlier on bad test input
  qobject: Propagate parse errors through qobject_from_json()
  block: More detailed syntax error reporting for JSON filenames
  check-qjson: Test errors from qobject_from_json()
  test-visitor-serialization: Pass &error_abort to qobject_from_json()
  monitor: Assert qmp_schema_json[] is sane
  qapi: New qobject_input_visitor_new_str() for convenience
  block: Initial implementation of -blockdev
  qapi: Improve how keyval input visitor reports unexpected dicts
  docs/qapi-code-gen.txt: Clarify naming rules
  test-qapi-util: New, covering qapi/qapi-util.c
  qapi: New parse_qapi_name()
  keyval: Restrict key components to valid QAPI names
  keyval: Support lists

 block.c  |   9 +-
 docs/qapi-code-gen.txt   |  61 ++--
 include/qapi/qmp/qjson.h |   8 +-
 include/qapi/qobject-input-visitor.h |  21 ++
 include/qapi/util.h  |   2 +
 include/qemu/option.h|   3 +
 monitor.c|   2 +-
 qapi/qapi-util.c |  47 +++
 qapi/qobject-input-visitor.c | 210 +++-
 qemu-options.hx  |   7 +
 qobject/qjson.c  |  14 +-
 tests/.gitignore |   2 +
 tests/Makefile.include   |  10 +-
 tests/check-qjson.c  |  88 +++--
 tests/libqtest.c |   3 +-
 tests/test-keyval.c  | 619 +++
 tests/test-qapi-util.c   |  85 +
 tests/test-qemu-opts.c   |   5 +
 tests/test-qobject-input-visitor.c   | 190 ++-
 tests/test-visitor-serialization.c   |   2 +-
 util/Makefile.objs   |   1 +
 util/keyval.c| 385 ++
 vl.c |  39 +++
 23 files changed, 1730 insertions(+), 83 deletions(-)
 create mode 100644 tests/test-keyval.c
 create mode 100644 tests/test-qapi-util.c
 create mode 100644 util/keyval.c

-- 
2.7.4

[Qemu-devel] [PATCH 17/24] qapi: New qobject_input_visitor_new_str() for convenience

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
---
 include/qapi/qobject-input-visitor.h | 12 
 qapi/qobject-input-visitor.c | 33 +
 2 files changed, 45 insertions(+)

diff --git a/include/qapi/qobject-input-visitor.h 
b/include/qapi/qobject-input-visitor.h
index 282f9d2..b399285 100644
--- a/include/qapi/qobject-input-visitor.h
+++ b/include/qapi/qobject-input-visitor.h
@@ -68,4 +68,16 @@ Visitor *qobject_input_visitor_new(QObject *obj);
  */
 Visitor *qobject_input_visitor_new_keyval(QObject *obj);
 
+/*
+ * Create a QObject input visitor for parsing @str.
+ *
+ * If @str looks like JSON, parse it as JSON, else as KEY=VALUE,...
+ * @implied_key applies to KEY=VALUE, and works as in keyval_parse().
+ * On failure, store an error through @errp and return NULL.
+ * On success, return a new QObject input visitor for the parse.
+ */
+Visitor *qobject_input_visitor_new_str(const char *str,
+   const char *implied_key,
+   Error **errp);
+
 #endif
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index 3db5850..64a08d3 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -18,9 +18,11 @@
 #include "qapi/visitor-impl.h"
 #include "qemu/queue.h"
 #include "qemu-common.h"
+#include "qapi/qmp/qjson.h"
 #include "qapi/qmp/types.h"
 #include "qapi/qmp/qerror.h"
 #include "qemu/cutils.h"
+#include "qemu/option.h"
 
 typedef struct StackObject {
 const char *name;/* Name of @obj in its parent, if any */
@@ -655,3 +657,34 @@ Visitor *qobject_input_visitor_new_keyval(QObject *obj)
 
 return &v->visitor;
 }
+
+Visitor *qobject_input_visitor_new_str(const char *str,
+   const char *implied_key,
+   Error **errp)
+{
+bool is_json = str[0] == '{';
+QObject *obj;
+QDict *args;
+Visitor *v;
+
+if (is_json) {
+obj = qobject_from_json(str, errp);
+if (!obj) {
+/* Work around qobject_from_json() lossage TODO fix that */
+if (errp && !*errp) {
+error_setg(errp, "JSON parse error");
+return NULL;
+}
+return NULL;
+}
+args = qobject_to_qdict(obj);
+assert(args);
+v = qobject_input_visitor_new(QOBJECT(args));
+} else {
+args = keyval_parse(optarg, implied_key, errp);
+v = qobject_input_visitor_new_keyval(QOBJECT(args));
+}
+QDECREF(args);
+
+return v;
+}
-- 
2.7.4

[Qemu-devel] [PATCH 23/24] keyval: Restrict key components to valid QAPI names

[Markus Armbruster]

Restricting the key components to something sane leaves us room for
evolving key syntax.  Since they will be commonly used as QAPI member
names by the QObject input visitor, we can just as well borrow the
QAPI naming rules here.

Signed-off-by: Markus Armbruster 
---
 tests/test-keyval.c | 10 ++
 util/keyval.c   | 12 
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/tests/test-keyval.c b/tests/test-keyval.c
index f6496d7..6eceafb 100644
--- a/tests/test-keyval.c
+++ b/tests/test-keyval.c
@@ -41,6 +41,11 @@ static void test_keyval_parse(void)
 error_free_or_abort(&err);
 g_assert(!qdict);
 
+/* Invalid non-empty key (qemu_opts_parse() doesn't care) */
+qdict = keyval_parse("7up=val", NULL, &err);
+error_free_or_abort(&err);
+g_assert(!qdict);
+
 /* Overlong key */
 memset(long_key, 'a', 127);
 long_key[127] = 'z';
@@ -73,6 +78,11 @@ static void test_keyval_parse(void)
 QDECREF(qdict);
 g_free(params);
 
+/* Crap after valid key */
+qdict = keyval_parse("key[0]=val", NULL, &err);
+error_free_or_abort(&err);
+g_assert(!qdict);
+
 /* Multiple keys, last one wins */
 qdict = keyval_parse("a=1,b=2,,x,a=3", NULL, &error_abort);
 g_assert_cmpuint(qdict_size(qdict), ==, 2);
diff --git a/util/keyval.c b/util/keyval.c
index 3904c39..1170dad 100644
--- a/util/keyval.c
+++ b/util/keyval.c
@@ -34,6 +34,8 @@
  *   doesn't have one, because R.a must be an object to satisfy a.b=1
  *   and a string to satisfy a=2.
  *
+ * Key-fragments must be valid QAPI names.
+ *
  * The length of any key-fragment must be between 1 and 127.
  *
  * Design flaw: there is no way to denote an empty non-root object.
@@ -51,12 +53,12 @@
  * where no-key is syntactic sugar for implied-key=val-no-key.
  *
  * TODO support lists
- * TODO support key-fragment with __RFQDN_ prefix (downstream extensions)
  */
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qapi/qmp/qstring.h"
+#include "qapi/util.h"
 #include "qemu/option.h"
 
 /*
@@ -115,6 +117,7 @@ static const char *keyval_parse_one(QDict *qdict, const 
char *params,
 size_t len;
 char key_in_cur[128];
 QDict *cur;
+int ret;
 QObject *next;
 QString *val;
 
@@ -134,9 +137,10 @@ static const char *keyval_parse_one(QDict *qdict, const 
char *params,
 cur = qdict;
 s = key;
 for (;;) {
-for (len = 0; s + len < key_end && s[len] != '.'; len++) {
-}
-if (!len) {
+ret = parse_qapi_name(s, false);
+len = ret < 0 ? 0 : ret;
+assert(s + len <= key_end);
+if (!len || (s + len < key_end && s[len] != '.')) {
 assert(key != implied_key);
 error_setg(errp, "Invalid parameter '%.*s'",
(int)(key_end - key), key);
-- 
2.7.4

[Qemu-devel] [PATCH 13/24] block: More detailed syntax error reporting for JSON filenames

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
---
 block.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index aa6790c..de7d530 100644
--- a/block.c
+++ b/block.c
@@ -1163,9 +1163,14 @@ static QDict *parse_json_filename(const char *filename, 
Error **errp)
 ret = strstart(filename, "json:", &filename);
 assert(ret);
 
-options_obj = qobject_from_json(filename, NULL);
+options_obj = qobject_from_json(filename, errp);
 if (!options_obj) {
-error_setg(errp, "Could not parse the JSON options");
+/* Work around qobject_from_json() lossage TODO fix that */
+if (errp && !*errp) {
+error_setg(errp, "Could not parse the JSON options");
+return NULL;
+}
+error_prepend(errp, "Could not parse the JSON options: ");
 return NULL;
 }
 
-- 
2.7.4

[Qemu-devel] [PATCH 05/24] test-keyval: Cover use with qobject input visitor

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
---
 tests/test-keyval.c | 312 
 1 file changed, 312 insertions(+)

diff --git a/tests/test-keyval.c b/tests/test-keyval.c
index 27f6625..f6496d7 100644
--- a/tests/test-keyval.c
+++ b/tests/test-keyval.c
@@ -12,6 +12,8 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qemu/cutils.h"
 #include "qemu/option.h"
 
 static void test_keyval_parse(void)
@@ -171,10 +173,320 @@ static void test_keyval_parse(void)
 g_assert(!qdict);
 }
 
+static void test_keyval_visit_bool(void)
+{
+Error *err = NULL;
+Visitor *v;
+QDict *qdict;
+bool b;
+
+qdict = keyval_parse("bool1=on,bool2=off", NULL, &error_abort);
+v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+QDECREF(qdict);
+visit_start_struct(v, NULL, NULL, 0, &error_abort);
+visit_type_bool(v, "bool1", &b, &error_abort);
+g_assert(b);
+visit_type_bool(v, "bool2", &b, &error_abort);
+g_assert(!b);
+visit_check_struct(v, &error_abort);
+visit_end_struct(v, NULL);
+visit_free(v);
+
+qdict = keyval_parse("bool1=offer", NULL, &error_abort);
+v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+QDECREF(qdict);
+visit_start_struct(v, NULL, NULL, 0, &error_abort);
+visit_type_bool(v, "bool1", &b, &err);
+error_free_or_abort(&err);
+visit_end_struct(v, NULL);
+visit_free(v);
+}
+
+static void test_keyval_visit_number(void)
+{
+Error *err = NULL;
+Visitor *v;
+QDict *qdict;
+uint64_t u;
+
+/* Lower limit zero */
+qdict = keyval_parse("number1=0", NULL, &error_abort);
+v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+QDECREF(qdict);
+visit_start_struct(v, NULL, NULL, 0, &error_abort);
+visit_type_uint64(v, "number1", &u, &error_abort);
+g_assert_cmpuint(u, ==, 0);
+visit_check_struct(v, &error_abort);
+visit_end_struct(v, NULL);
+visit_free(v);
+
+/* Upper limit 2^64-1 */
+qdict = keyval_parse("number1=18446744073709551615,number2=-1",
+ NULL, &error_abort);
+v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+QDECREF(qdict);
+visit_start_struct(v, NULL, NULL, 0, &error_abort);
+visit_type_uint64(v, "number1", &u, &error_abort);
+g_assert_cmphex(u, ==, UINT64_MAX);
+visit_type_uint64(v, "number2", &u, &error_abort);
+g_assert_cmphex(u, ==, UINT64_MAX);
+visit_check_struct(v, &error_abort);
+visit_end_struct(v, NULL);
+visit_free(v);
+
+/* Above upper limit */
+qdict = keyval_parse("number1=18446744073709551616",
+ NULL, &error_abort);
+v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+QDECREF(qdict);
+visit_start_struct(v, NULL, NULL, 0, &error_abort);
+visit_type_uint64(v, "number1", &u, &err);
+error_free_or_abort(&err);
+visit_end_struct(v, NULL);
+visit_free(v);
+
+/* Below lower limit */
+qdict = keyval_parse("number1=-18446744073709551616",
+ NULL, &error_abort);
+v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+QDECREF(qdict);
+visit_start_struct(v, NULL, NULL, 0, &error_abort);
+visit_type_uint64(v, "number1", &u, &err);
+error_free_or_abort(&err);
+visit_end_struct(v, NULL);
+visit_free(v);
+
+/* Hex and octal */
+qdict = keyval_parse("number1=0x2a,number2=052",
+ NULL, &error_abort);
+v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+QDECREF(qdict);
+visit_start_struct(v, NULL, NULL, 0, &error_abort);
+visit_type_uint64(v, "number1", &u, &error_abort);
+g_assert_cmpuint(u, ==, 42);
+visit_type_uint64(v, "number2", &u, &error_abort);
+g_assert_cmpuint(u, ==, 42);
+visit_check_struct(v, &error_abort);
+visit_end_struct(v, NULL);
+visit_free(v);
+
+/* Trailing crap */
+qdict = keyval_parse("number1=3.14,number2=08",
+ NULL, &error_abort);
+v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+QDECREF(qdict);
+visit_start_struct(v, NULL, NULL, 0, &error_abort);
+visit_type_uint64(v, "number1", &u, &err);
+error_free_or_abort(&err);
+visit_type_uint64(v, "number2", &u, &err);
+error_free_or_abort(&err);
+visit_end_struct(v, NULL);
+visit_free(v);
+}
+
+static void test_keyval_visit_size(void)
+{
+Error *err = NULL;
+Visitor *v;
+QDict *qdict;
+uint64_t sz;
+
+/* Lower limit zero */
+qdict = keyval_parse("sz1=0", NULL, &error_abort);
+v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+QDECREF(qdict);
+visit_start_struct(v, NULL, NULL, 0, &error_abort);
+visit_type_size(v, "sz1", &sz, &error_abort);
+g_assert_cmpuint(sz, ==, 0);
+visit_check_struct(v, &error_abort);
+visit_end_struct(v, NULL);
+visit_free(v);
+
+/* Note: precision is 53 bits since we're parsing with

[Qemu-devel] [PATCH v2] Adding support for LPD and LPDG instructions

[Eric Bischoff]

Second version of the patch, setting CC to zero.
I am not too satisfied, I had to create a cout_zero() helper, but
I could not find a better solution.

[Qemu-devel] [PATCH 09/24] libqtest: Fix qmp() & friends to abort on JSON parse errors

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
---
 tests/libqtest.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index 683d5e3..bb444d5 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -21,6 +21,7 @@
 #include 
 #include 
 
+#include "qapi/error.h"
 #include "qapi/qmp/json-parser.h"
 #include "qapi/qmp/json-streamer.h"
 #include "qapi/qmp/qjson.h"
@@ -442,7 +443,7 @@ void qmp_fd_sendv(int fd, const char *fmt, va_list ap)
  * is an array type.
  */
 va_copy(ap_copy, ap);
-qobj = qobject_from_jsonv(fmt, &ap_copy, NULL);
+qobj = qobject_from_jsonv(fmt, &ap_copy, &error_abort);
 va_end(ap_copy);
 
 /* No need to send anything for an empty QObject.  */
-- 
2.7.4

[Qemu-devel] [PATCH 19/24] qapi: Improve how keyval input visitor reports unexpected dicts

[Markus Armbruster]

Incorrect option

-blockdev node-name=foo,driver=file,filename=foo.img,aio.unmap

is rejected with "Invalid parameter type for 'aio', expected: string".
To make sense of this, you almost have to translate it into the
equivalent QMP command

{ "execute": "blockdev-add", "arguments": { "node-name": "foo", "driver": 
"file", "filename": "foo.img", "aio": { "unmap": true } } }

Improve the error message to "Parameters 'aio.*' are unexpected".
Take care not to confuse the case "unexpected nested parameters"
(i.e. the object is a QDict or QList) with the case "non-string scalar
parameter".  The latter is a misuse of the visitor, and should perhaps
be an assertion.  Note that test-qobject-input-visitor exercises this
misuse in test_visitor_in_int_keyval(), test_visitor_in_bool_keyval()
and test_visitor_in_number_keyval().

Signed-off-by: Markus Armbruster 
---
 qapi/qobject-input-visitor.c | 26 ++
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index 64a08d3..1d7b420 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -167,9 +167,18 @@ static const char 
*qobject_input_get_keyval(QObjectInputVisitor *qiv,
 
 qstr = qobject_to_qstring(qobj);
 if (!qstr) {
-error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
-   full_name(qiv, name), "string");
-return NULL;
+switch (qobject_type(qobj)) {
+case QTYPE_QDICT:
+case QTYPE_QLIST:
+error_setg(errp, "Parameters '%s.*' are unexpected",
+   full_name(qiv, name));
+return NULL;
+default:
+/* Non-string scalar (should this be an assertion?) */
+error_setg(errp, "Internal error: parameter %s invalid",
+   full_name(qiv, name));
+return NULL;
+}
 }
 
 return qstring_get_str(qstr);
@@ -478,6 +487,15 @@ static void qobject_input_type_str(Visitor *v, const char 
*name, char **obj,
 *obj = g_strdup(qstring_get_str(qstr));
 }
 
+static void qobject_input_type_str_keyval(Visitor *v, const char *name,
+  char **obj, Error **errp)
+{
+QObjectInputVisitor *qiv = to_qiv(v);
+const char *str = qobject_input_get_keyval(qiv, name, errp);
+
+*obj = g_strdup(str);
+}
+
 static void qobject_input_type_number(Visitor *v, const char *name, double 
*obj,
   Error **errp)
 {
@@ -649,7 +667,7 @@ Visitor *qobject_input_visitor_new_keyval(QObject *obj)
 v->visitor.type_int64 = qobject_input_type_int64_keyval;
 v->visitor.type_uint64 = qobject_input_type_uint64_keyval;
 v->visitor.type_bool = qobject_input_type_bool_keyval;
-v->visitor.type_str = qobject_input_type_str;
+v->visitor.type_str = qobject_input_type_str_keyval;
 v->visitor.type_number = qobject_input_type_number_keyval;
 v->visitor.type_any = qobject_input_type_any;
 v->visitor.type_null = qobject_input_type_null;
-- 
2.7.4

[Qemu-devel] [PATCH 20/24] docs/qapi-code-gen.txt: Clarify naming rules

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
---
 docs/qapi-code-gen.txt | 61 --
 1 file changed, 39 insertions(+), 22 deletions(-)

diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index 6746c10..9514d93 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -216,33 +216,38 @@ single-dimension array of that type; multi-dimension 
arrays are not
 directly supported (although an array of a complex struct that
 contains an array member is possible).
 
+All names must begin with a letter, and contain only ASCII letters,
+digits, hyphen, and underscore.  There are two exceptions: enum values
+may start with a digit, and names that are downstream extensions (see
+section Downstream extensions) start with underscore.
+
+Names beginning with 'q_' are reserved for the generator, which uses
+them for munging QMP names that resemble C keywords or other
+problematic strings.  For example, a member named "default" in qapi
+becomes "q_default" in the generated C code.
+
 Types, commands, and events share a common namespace.  Therefore,
 generally speaking, type definitions should always use CamelCase for
-user-defined type names, while built-in types are lowercase. Type
-definitions should not end in 'Kind', as this namespace is used for
-creating implicit C enums for visiting union types, or in 'List', as
-this namespace is used for creating array types.  Command names,
-and member names within a type, should be all lower case with words
-separated by a hyphen.  However, some existing older commands and
-complex types use underscore; when extending such expressions,
-consistency is preferred over blindly avoiding underscore.  Event
-names should be ALL_CAPS with words separated by underscore.  Member
-names cannot start with 'has-' or 'has_', as this is reserved for
-tracking optional members.
+user-defined type names, while built-in types are lowercase.
+
+Type names ending with 'Kind' or 'List' are reserved for the
+generator, which uses them for implicit union enums and array types,
+respectively.
+
+Command names, and member names within a type, should be all lower
+case with words separated by a hyphen.  However, some existing older
+commands and complex types use underscore; when extending such
+expressions, consistency is preferred over blindly avoiding
+underscore.
+
+Event names should be ALL_CAPS with words separated by underscore.
+
+Member names starting with 'has-' or 'has_' are reserved for the
+generator, which uses them for tracking optional members.
 
 Any name (command, event, type, member, or enum value) beginning with
 "x-" is marked experimental, and may be withdrawn or changed
-incompatibly in a future release.  All names must begin with a letter,
-and contain only ASCII letters, digits, dash, and underscore.  There
-are two exceptions: enum values may start with a digit, and any
-extensions added by downstream vendors should start with a prefix
-matching "__RFQDN_" (for the reverse-fully-qualified-domain-name of
-the vendor), even if the rest of the name uses dash (example:
-__com.redhat_drive-mirror).  Names beginning with 'q_' are reserved
-for the generator: QMP names that resemble C keywords or other
-problematic strings will be munged in C to use this prefix.  For
-example, a member named "default" in qapi becomes "q_default" in the
-generated C code.
+incompatibly in a future release.
 
 In the rest of this document, usage lines are given for each
 expression type, with literal strings written in lower case and
@@ -643,6 +648,18 @@ any non-empty complex type (struct, union, or alternate), 
and a
 pointer to that QAPI type is passed as a single argument.
 
 
+=== Downstream extensions ===
+
+QAPI schema names that are externally visible, say in the Client JSON
+Protocol, need to be managed with care.  Names starting with a
+downstream prefix of the form __RFQDN_ are reserved for the downstream
+who controls the valid, reverse fully qualified domain name RFQDN.
+RFQDN may only contain ASCII letters, digits, hyphen and period.
+
+Example: Red Hat, Inc. controls redhat.com, and may therefore add a
+downstream command __com.redhat_drive-mirror.
+
+
 == Client JSON Protocol introspection ==
 
 Clients of a Client JSON Protocol commonly need to figure out what
-- 
2.7.4

[Qemu-devel] [PATCH] Adding support for LPD and LPDG instructions

[Eric Bischoff]

From: Eric Bischoff 

LPD = LOAD PAIR DISJOINT
---
 target/s390x/insn-data.def |  4 +++-
 target/s390x/translate.c   | 21 +
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
index 075ff59..e427988 100644
--- a/target/s390x/insn-data.def
+++ b/target/s390x/insn-data.def
@@ -504,7 +504,9 @@
 C(0xb9e2, LOCGR,   RRF_c, LOC, r1, r2, r1, 0, loc, 0)
 C(0xebf2, LOC, RSY_b, LOC, r1, m2_32u, new, r1_32, loc, 0)
 C(0xebe2, LOCG,RSY_b, LOC, r1, m2_64, r1, 0, loc, 0)
-/* LOAD PAIR DISJOINT TODO */
+/* LOAD PAIR DISJOINT */
+C(0xc804, LPD, SSF,   ILA, m1_32s, m2_32s, 0, r3_P32, movx, zero)
+C(0xc805, LPDG,SSF,   ILA, m1_64, m2_64, 0, r3_P64, movx, zero)
 /* LOAD POSITIVE */
 C(0x1000, LPR, RR_a,  Z,   0, r2_32s, new, r1_32, abs, abs32)
 C(0xb900, LPGR,RRE,   Z,   0, r2, r1, 0, abs, abs64)
diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 01c6217..a363efb 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -4158,6 +4158,11 @@ static ExitStatus op_zero2(DisasContext *s, DisasOps *o)
the original inputs), update the various cc data structures in order to
be able to compute the new condition code.  */
 
+static void cout_zero(DisasContext *s, DisasOps *o)
+{
+gen_op_movi_cc(s, 0);
+}
+
 static void cout_abs32(DisasContext *s, DisasOps *o)
 {
 gen_op_update1_cc_i64(s, CC_OP_ABS_32, o->out);
@@ -4420,6 +4425,22 @@ static void wout_r1_D32(DisasContext *s, DisasFields *f, 
DisasOps *o)
 }
 #define SPEC_wout_r1_D32 SPEC_r1_even
 
+static void wout_r3_P32(DisasContext *s, DisasFields *f, DisasOps *o)
+{
+int r3 = get_field(f, r3);
+store_reg32_i64(r3, o->out);
+store_reg32_i64(r3 + 1, o->out2);
+}
+#define SPEC_wout_r3_P32 SPEC_r3_even
+
+static void wout_r3_P64(DisasContext *s, DisasFields *f, DisasOps *o)
+{
+int r3 = get_field(f, r3);
+store_reg(r3, o->out);
+store_reg(r3 + 1, o->out2);
+}
+#define SPEC_wout_r3_P64 SPEC_r3_even
+
 static void wout_e1(DisasContext *s, DisasFields *f, DisasOps *o)
 {
 store_freg32_i64(get_field(f, r1), o->out);
-- 
2.10.2

[Qemu-devel] [PATCH 06/24] qapi: Factor out common part of qobject input visitor creation

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
---
 qapi/qobject-input-visitor.c | 61 +++-
 1 file changed, 26 insertions(+), 35 deletions(-)

diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index d53bad3..e47615e 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -618,22 +618,34 @@ static void qobject_input_free(Visitor *v)
 g_free(qiv);
 }
 
+static QObjectInputVisitor *qobject_input_visitor_base_new(QObject *obj)
+{
+QObjectInputVisitor *v = g_malloc0(sizeof(*v));
+
+assert(obj);
+
+v->visitor.type = VISITOR_INPUT;
+v->visitor.start_struct = qobject_input_start_struct;
+v->visitor.check_struct = qobject_input_check_struct;
+v->visitor.end_struct = qobject_input_pop;
+v->visitor.start_list = qobject_input_start_list;
+v->visitor.next_list = qobject_input_next_list;
+v->visitor.check_list = qobject_input_check_list;
+v->visitor.end_list = qobject_input_pop;
+v->visitor.start_alternate = qobject_input_start_alternate;
+v->visitor.optional = qobject_input_optional;
+v->visitor.free = qobject_input_free;
+
+v->root = obj;
+qobject_incref(obj);
+
+return v;
+}
+
 Visitor *qobject_input_visitor_new(QObject *obj)
 {
-QObjectInputVisitor *v;
+QObjectInputVisitor *v = qobject_input_visitor_base_new(obj);
 
-assert(obj);
-v = g_malloc0(sizeof(*v));
-
-v->visitor.type = VISITOR_INPUT;
-v->visitor.start_struct = qobject_input_start_struct;
-v->visitor.check_struct = qobject_input_check_struct;
-v->visitor.end_struct = qobject_input_pop;
-v->visitor.start_list = qobject_input_start_list;
-v->visitor.next_list = qobject_input_next_list;
-v->visitor.check_list = qobject_input_check_list;
-v->visitor.end_list = qobject_input_pop;
-v->visitor.start_alternate = qobject_input_start_alternate;
 v->visitor.type_int64 = qobject_input_type_int64;
 v->visitor.type_uint64 = qobject_input_type_uint64;
 v->visitor.type_bool = qobject_input_type_bool;
@@ -641,30 +653,14 @@ Visitor *qobject_input_visitor_new(QObject *obj)
 v->visitor.type_number = qobject_input_type_number;
 v->visitor.type_any = qobject_input_type_any;
 v->visitor.type_null = qobject_input_type_null;
-v->visitor.optional = qobject_input_optional;
-v->visitor.free = qobject_input_free;
-
-v->root = obj;
-qobject_incref(obj);
 
 return &v->visitor;
 }
 
 Visitor *qobject_input_visitor_new_keyval(QObject *obj)
 {
-QObjectInputVisitor *v;
+QObjectInputVisitor *v = qobject_input_visitor_base_new(obj);
 
-v = g_malloc0(sizeof(*v));
-
-v->visitor.type = VISITOR_INPUT;
-v->visitor.start_struct = qobject_input_start_struct;
-v->visitor.check_struct = qobject_input_check_struct;
-v->visitor.end_struct = qobject_input_pop;
-v->visitor.start_list = qobject_input_start_list;
-v->visitor.next_list = qobject_input_next_list;
-v->visitor.check_list = qobject_input_check_list;
-v->visitor.end_list = qobject_input_pop;
-v->visitor.start_alternate = qobject_input_start_alternate;
 v->visitor.type_int64 = qobject_input_type_int64_keyval;
 v->visitor.type_uint64 = qobject_input_type_uint64_keyval;
 v->visitor.type_bool = qobject_input_type_bool_keyval;
@@ -673,11 +669,6 @@ Visitor *qobject_input_visitor_new_keyval(QObject *obj)
 v->visitor.type_any = qobject_input_type_any;
 v->visitor.type_null = qobject_input_type_null;
 v->visitor.type_size = qobject_input_type_size_keyval;
-v->visitor.optional = qobject_input_optional;
-v->visitor.free = qobject_input_free;
-
-v->root = obj;
-qobject_incref(obj);
 
 return &v->visitor;
 }
-- 
2.7.4

[Qemu-devel] [PATCH 22/24] qapi: New parse_qapi_name()

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
---
 include/qapi/util.h|  2 ++
 qapi/qapi-util.c   | 47 +++
 tests/test-qapi-util.c | 34 ++
 3 files changed, 83 insertions(+)

diff --git a/include/qapi/util.h b/include/qapi/util.h
index 7ad26c0..7436ed8 100644
--- a/include/qapi/util.h
+++ b/include/qapi/util.h
@@ -14,4 +14,6 @@
 int qapi_enum_parse(const char * const lookup[], const char *buf,
 int max, int def, Error **errp);
 
+int parse_qapi_name(const char *name, bool complete);
+
 #endif
diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
index 818730a..53b27e2 100644
--- a/qapi/qapi-util.c
+++ b/qapi/qapi-util.c
@@ -33,3 +33,50 @@ int qapi_enum_parse(const char * const lookup[], const char 
*buf,
 error_setg(errp, "invalid parameter value: %s", buf);
 return def;
 }
+
+/*
+ * Parse a valid QAPI name from @str.
+ * A valid name consists of letters, digits, hyphen and underscore.
+ * It may be prefixed by __RFQDN_ (downstream extension), where RFQDN
+ * may contain only letters, digits and hyphen.
+ * The special exception for enumeration names is not implemented.
+ * See docs/qapi-code-gen.txt for more on QAPI naming rules.
+ * Keep this consistent with scripts/qapi.py!
+ * If @complete, the parse fails unless it consumes @str completely.
+ * Return its length on success, -1 on failure.
+ */
+int parse_qapi_name(const char *str, bool complete)
+{
+const char *p = str;
+
+if (*p == '_') {/* Downstream __RFQDN_ */
+p++;
+if (*p != '_') {
+return -1;
+}
+while (*++p) {
+if (!qemu_isalnum(*p) && *p != '-' && *p != '.') {
+break;
+}
+}
+
+if (*p != '_') {
+return -1;
+}
+p++;
+}
+
+if (!qemu_isalpha(*p)) {
+return -1;
+}
+while (*++p) {
+if (!qemu_isalnum(*p) && *p != '-' && *p != '_') {
+break;
+}
+}
+
+if (complete && *p) {
+return -1;
+}
+return p - str;
+}
diff --git a/tests/test-qapi-util.c b/tests/test-qapi-util.c
index 39db8bf..e869757 100644
--- a/tests/test-qapi-util.c
+++ b/tests/test-qapi-util.c
@@ -42,10 +42,44 @@ static void test_qapi_enum_parse(void)
 g_assert_cmpint(ret, ==, QTYPE__MAX - 1);
 }
 
+static void test_parse_qapi_name(void)
+{
+int ret;
+
+/* Must start with a letter */
+ret = parse_qapi_name("a", true);
+g_assert(ret == 1);
+ret = parse_qapi_name("a$", false);
+g_assert(ret == 1);
+ret = parse_qapi_name("", false);
+g_assert(ret == -1);
+ret = parse_qapi_name("1", false);
+g_assert(ret == -1);
+
+/* Only letters, digits, hyphen, underscore */
+ret = parse_qapi_name("A-Za-z0-9_", true);
+g_assert(ret == 10);
+ret = parse_qapi_name("A-Za-z0-9_$", false);
+g_assert(ret == 10);
+ret = parse_qapi_name("A-Za-z0-9_$", true);
+g_assert(ret == -1);
+
+/* __RFQDN_ */
+ret = parse_qapi_name("__com.redhat_supports", true);
+g_assert(ret == 21);
+ret = parse_qapi_name("_com.example_", false);
+g_assert(ret == -1);
+ret = parse_qapi_name("__com.example", false);
+g_assert(ret == -1);
+ret = parse_qapi_name("__com.example_", false);
+g_assert(ret == -1);
+}
+
 int main(int argc, char *argv[])
 {
 g_test_init(&argc, &argv, NULL);
 g_test_add_func("/qapi/util/qapi_enum_parse", test_qapi_enum_parse);
+g_test_add_func("/qapi/util/parse_qapi_name", test_parse_qapi_name);
 g_test_run();
 return 0;
 }
-- 
2.7.4

[Qemu-devel] [PATCH 03/24] keyval: New keyval_parse()

[Markus Armbruster]

keyval_parse() parses KEY=VALUE,... into a QDict.  Works like
qemu_opts_parse(), except:

* Returns a QDict instead of a QemuOpts (d'oh).

* Supports nesting, unlike QemuOpts: a KEY is split into key
  fragments at '.' (dotted key convention; the block layer does
  something similar on top of QemuOpts).  The key fragments are QDict
  keys, and the last one's value is updated to VALUE.

* Each key fragment may be up to 127 bytes long.  qemu_opts_parse()
  limits the entire key to 127 bytes.

* Overlong key fragments are rejected.  qemu_opts_parse() silently
  truncates them.

* Empty key fragments are rejected.  qemu_opts_parse() happily
  accepts empty keys.

* It does not store the returned value.  qemu_opts_parse() stores it
  in the QemuOptsList.

* It does not treat parameter "id" specially.  qemu_opts_parse()
  ignores all but the first "id", and fails when its value isn't
  id_wellformed(), or duplicate (a QemuOpts with the same ID is
  already stored).  It also screws up when a value contains ",id=".

* Implied value is not supported.  qemu_opts_parse() desugars "foo" to
  "foo=on", and "nofoo" to "foo=off".

* An implied key's value can't be empty, and can't contain ','.

I intend to grow this into a saner replacement for QemuOpts.  It'll
take time, though.

Note: keyval_parse() provides no way to do lists, and its key syntax
is incompatible with the __RFQDN_ prefix convention for downstream
extensions, because it blindly splits at '.', even in __RFQDN_.  Both
issues will be addressed later in the series.

Signed-off-by: Markus Armbruster 
---
 include/qemu/option.h  |   3 +
 tests/.gitignore   |   1 +
 tests/Makefile.include |   3 +
 tests/test-keyval.c| 180 ++
 util/Makefile.objs |   1 +
 util/keyval.c  | 228 +
 6 files changed, 416 insertions(+)
 create mode 100644 tests/test-keyval.c
 create mode 100644 util/keyval.c

diff --git a/include/qemu/option.h b/include/qemu/option.h
index e786df0..f7338db 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -141,4 +141,7 @@ void qemu_opts_print_help(QemuOptsList *list);
 void qemu_opts_free(QemuOptsList *list);
 QemuOptsList *qemu_opts_append(QemuOptsList *dst, QemuOptsList *list);
 
+QDict *keyval_parse(const char *params, const char *implied_key,
+Error **errp);
+
 #endif
diff --git a/tests/.gitignore b/tests/.gitignore
index dc37519..30b7740 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -47,6 +47,7 @@ test-io-channel-file.txt
 test-io-channel-socket
 test-io-channel-tls
 test-io-task
+test-keyval
 test-logging
 test-mul64
 test-opts-visitor
diff --git a/tests/Makefile.include b/tests/Makefile.include
index fdf528c..2171e4a 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -94,6 +94,8 @@ check-unit-y += tests/check-qom-proplist$(EXESUF)
 gcov-files-check-qom-proplist-y = qom/object.c
 check-unit-y += tests/test-qemu-opts$(EXESUF)
 gcov-files-test-qemu-opts-y = util/qemu-option.c
+check-unit-y += tests/test-keyval$(EXESUF)
+gcov-files-test-keyval-y = util/keyval.c
 check-unit-y += tests/test-write-threshold$(EXESUF)
 gcov-files-test-write-threshold-y = block/write-threshold.c
 check-unit-y += tests/test-crypto-hash$(EXESUF)
@@ -721,6 +723,7 @@ tests/vhost-user-test$(EXESUF): tests/vhost-user-test.o 
$(test-util-obj-y) \
$(chardev-obj-y)
 tests/qemu-iotests/socket_scm_helper$(EXESUF): 
tests/qemu-iotests/socket_scm_helper.o
 tests/test-qemu-opts$(EXESUF): tests/test-qemu-opts.o $(test-util-obj-y)
+tests/test-keyval$(EXESUF): tests/test-keyval.o $(test-util-obj-y)
 tests/test-write-threshold$(EXESUF): tests/test-write-threshold.o 
$(test-block-obj-y)
 tests/test-netfilter$(EXESUF): tests/test-netfilter.o $(qtest-obj-y)
 tests/test-filter-mirror$(EXESUF): tests/test-filter-mirror.o $(qtest-obj-y)
diff --git a/tests/test-keyval.c b/tests/test-keyval.c
new file mode 100644
index 000..27f6625
--- /dev/null
+++ b/tests/test-keyval.c
@@ -0,0 +1,180 @@
+/*
+ * Unit tests for parsing of KEY=VALUE,... strings
+ *
+ * Copyright (C) 2017 Red Hat Inc.
+ *
+ * Authors:
+ *  Markus Armbruster ,
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qemu/option.h"
+
+static void test_keyval_parse(void)
+{
+Error *err = NULL;
+QDict *qdict, *sub_qdict;
+char long_key[129];
+char *params;
+
+/* Nothing */
+qdict = keyval_parse("", NULL, &error_abort);
+g_assert_cmpuint(qdict_size(qdict), ==, 0);
+QDECREF(qdict);
+
+/* Empty key (qemu_opts_parse() accepts this) */
+qdict = keyval_parse("=val", NULL, &err);
+error_free_or_abort(&err);
+g_assert(!qdict);
+
+/* Empty key fragment */
+qdict = keyval_parse(".", NULL, &err);
+error_free_or_abort(&err);
+g_assert(!qdict);
+qdict = keyval_par

Re: [Qemu-devel] [PATCH v8 0/2] docs: Improve sample configuration files

[Andrea Bolognani]

On Fri, 2017-02-17 at 11:14 +0100, Andrea Bolognani wrote:
[...]
> Andrea Bolognani (2):
>   q35: Improve sample configuration files
>   mach-virt: Provide sample configuration files
> 
>  docs/mach-virt-graphical.cfg  | 281 +
>  docs/mach-virt-serial.cfg | 243 +++
>  docs/q35-chipset.cfg  | 152 --
>  docs/q35-emulated.cfg | 288 
>++
>  docs/q35-virtio-graphical.cfg | 248 
>  docs/q35-virtio-serial.cfg| 193 
>  6 files changed, 1253 insertions(+), 152 deletions(-)
>  create mode 100644 docs/mach-virt-graphical.cfg
>  create mode 100644 docs/mach-virt-serial.cfg
>  delete mode 100644 docs/q35-chipset.cfg
>  create mode 100644 docs/q35-emulated.cfg
>  create mode 100644 docs/q35-virtio-graphical.cfg
>  create mode 100644 docs/q35-virtio-serial.cfg

Ping?

The series is fully ACKed as of two weeks ago, and it would
be great if someone could pick it up.

-- 
Andrea Bolognani / Red Hat / Virtualization

[Qemu-devel] [PATCH 18/24] block: Initial implementation of -blockdev

[Markus Armbruster]

The new command line option -blockdev works like QMP command
blockdev-add.

The option argument may be given in JSON syntax, exactly as in QMP.
Example usage:

-blockdev '{"node-name": "foo", "driver": "raw", "file": {"driver": "file", 
"filename": "foo.img"} }'

The JSON argument doesn't exactly blend into the existing option
syntax, so the traditional KEY=VALUE,... syntax is also supported,
using dotted keys to do the nesting:

-blockdev node-name=foo,driver=raw,file.driver=file,file.filename=foo.img

This does not yet support lists or downstream extensions, i.e. keys
with __RFQDN_ prefix, but the next few patches will take care of that.

Note that calling qmp_blockdev_add() (say via qmp_marshal_block_add())
right away would crash.  We need to stash the configuration for later
instead.  This is crudely done, and bypasses QemuOpts, even though
storing configuration is what QemuOpts is for.  Need to revamp option
infrastructure to support QAPI types like BlockdevOptions.

Signed-off-by: Markus Armbruster 
---
 qemu-options.hx |  7 +++
 vl.c| 39 +++
 2 files changed, 46 insertions(+)

diff --git a/qemu-options.hx b/qemu-options.hx
index bf458f8..8f02264 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -532,6 +532,13 @@ Use @var{file} as CD-ROM image (you cannot use 
@option{-hdc} and
 using @file{/dev/cdrom} as filename (@pxref{host_drives}).
 ETEXI
 
+DEF("blockdev", HAS_ARG, QEMU_OPTION_blockdev,
+"-blockdev driver[,node-name=N][,discard=ignore|unmap]\n"
+"  [,cache.direct=on|off][,cache.no-flush=on|off]\n"
+"  [,read-only=on|off][,detect-zeroes=on|off|unmap]\n"
+"  [,driver specific parameters...]\n"
+"configure a block backend\n", QEMU_ARCH_ALL)
+
 DEF("drive", HAS_ARG, QEMU_OPTION_drive,
 "-drive [file=file][,if=type][,bus=n][,unit=m][,media=d][,index=i]\n"
 "   [,cyls=c,heads=h,secs=s[,trans=t]][,snapshot=on|off]\n"
diff --git a/vl.c b/vl.c
index c6020b9..0684d52 100644
--- a/vl.c
+++ b/vl.c
@@ -95,6 +95,9 @@ int main(int argc, char **argv)
 #include "migration/colo.h"
 #include "sysemu/kvm.h"
 #include "sysemu/hax.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qapi-visit.h"
 #include "qapi/qmp/qjson.h"
 #include "qemu/option.h"
 #include "qemu/config-file.h"
@@ -2975,6 +2978,13 @@ int main(int argc, char **argv, char **envp)
 Error *main_loop_err = NULL;
 Error *err = NULL;
 bool list_data_dirs = false;
+typedef struct BlockdevOptions_queue {
+BlockdevOptions *bdo;
+Location loc;
+QSIMPLEQ_ENTRY(BlockdevOptions_queue) entry;
+} BlockdevOptions_queue;
+QSIMPLEQ_HEAD(, BlockdevOptions_queue) bdo_queue
+= QSIMPLEQ_HEAD_INITIALIZER(bdo_queue);
 
 module_call_init(MODULE_INIT_TRACE);
 
@@ -3117,6 +3127,25 @@ int main(int argc, char **argv, char **envp)
 drive_add(IF_DEFAULT, popt->index - QEMU_OPTION_hda, optarg,
   HD_OPTS);
 break;
+case QEMU_OPTION_blockdev:
+{
+Visitor *v;
+BlockdevOptions_queue *bdo;
+
+v = qobject_input_visitor_new_str(optarg, "driver", &err);
+if (!v) {
+error_report_err(err);
+exit(1);
+}
+
+bdo = g_new(BlockdevOptions_queue, 1);
+visit_type_BlockdevOptions(v, NULL, &bdo->bdo,
+   &error_fatal);
+visit_free(v);
+loc_save(&bdo->loc);
+QSIMPLEQ_INSERT_TAIL(&bdo_queue, bdo, entry);
+break;
+}
 case QEMU_OPTION_drive:
 if (drive_def(optarg) == NULL) {
 exit(1);
@@ -4454,6 +4483,16 @@ int main(int argc, char **argv, char **envp)
 qemu_opts_foreach(qemu_find_opts("drive"), drive_enable_snapshot,
   NULL, NULL);
 }
+while (!QSIMPLEQ_EMPTY(&bdo_queue)) {
+BlockdevOptions_queue *bdo = QSIMPLEQ_FIRST(&bdo_queue);
+
+QSIMPLEQ_REMOVE_HEAD(&bdo_queue, entry);
+loc_push_restore(&bdo->loc);
+qmp_blockdev_add(bdo->bdo, &error_fatal);
+loc_pop(&bdo->loc);
+qapi_free_BlockdevOptions(bdo->bdo);
+g_free(bdo);
+}
 if (qemu_opts_foreach(qemu_find_opts("drive"), drive_init_func,
   &machine_class->block_default_type, NULL)) {
 exit(1);
-- 
2.7.4

[Qemu-devel] [PATCH 04/24] qapi: qobject input visitor variant for use with keyval_parse()

[Markus Armbruster]

From: "Daniel P. Berrange" 

Currently the QObjectInputVisitor assumes that all scalar values are
directly represented as the final types declared by the thing being
visited. i.e. it assumes an 'int' is using 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 keyval_parse(), an alternative mode is needed where
all the scalars types are represented as QString and converted on the
fly to the final desired type.

Signed-off-by: Daniel P. Berrange 
Message-Id: <1475246744-29302-8-git-send-email-berra...@redhat.com>

Rebased, conflicts resolved, commit message updated to refer to
keyval_parse().  autocast replaced by keyval in identifiers,
noautocast replaced by fail in tests.

Fix qobject_input_type_uint64_keyval() not to reject '-', for QemuOpts
compatibility: replace parse_uint_full() by open-coded
parse_option_number().  The next commit will add suitable tests.
Leave out the fancy ERANGE error reporting for now, but add a TODO
comment.  Add it qobject_input_type_int64_keyval() and
qobject_input_type_number_keyval(), too.

Open code parse_option_bool() and parse_option_size() so we have to
call qobject_input_get_name() only when actually needed.  Again, leave
out ERANGE error reporting for now.

QAPI/QMP downstream extension prefixes __RFQDN_ don't work, because
keyval_parse() splits them at '.'.  Add a TODO comment there.

qobject_input_type_int64_keyval(), qobject_input_type_uint64_keyval(),
qobject_input_type_number_keyval() tweaked for style.

Signed-off-by: Markus Armbruster 
---
 include/qapi/qobject-input-visitor.h |   9 ++
 qapi/qobject-input-visitor.c | 166 ++-
 tests/test-qobject-input-visitor.c   | 188 ++-
 3 files changed, 358 insertions(+), 5 deletions(-)

diff --git a/include/qapi/qobject-input-visitor.h 
b/include/qapi/qobject-input-visitor.h
index 0b7633a..282f9d2 100644
--- a/include/qapi/qobject-input-visitor.h
+++ b/include/qapi/qobject-input-visitor.h
@@ -59,4 +59,13 @@ typedef struct QObjectInputVisitor QObjectInputVisitor;
  */
 Visitor *qobject_input_visitor_new(QObject *obj);
 
+/*
+ * Create a QObject input visitor for @obj for use with keyval_parse()
+ *
+ * This is like qobject_input_visitor_new(), except scalars are all
+ * QString, and error messages refer to parts of @obj in the syntax
+ * keyval_parse() uses for KEYs.
+ */
+Visitor *qobject_input_visitor_new_keyval(QObject *obj);
+
 #endif
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index 6bb5a80..d53bad3 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -1,7 +1,7 @@
 /*
  * Input Visitor
  *
- * Copyright (C) 2012-2016 Red Hat, Inc.
+ * Copyright (C) 2012-2017 Red Hat, Inc.
  * Copyright IBM, Corp. 2011
  *
  * Authors:
@@ -20,6 +20,7 @@
 #include "qemu-common.h"
 #include "qapi/qmp/types.h"
 #include "qapi/qmp/qerror.h"
+#include "qemu/cutils.h"
 
 typedef struct StackObject {
 const char *name;/* Name of @obj in its parent, if any */
@@ -336,6 +337,31 @@ 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_keyval(Visitor *v, const char *name,
+int64_t *obj, Error **errp)
+{
+QObjectInputVisitor *qiv = to_qiv(v);
+QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
+QString *qstr;
+
+if (!qobj) {
+return;
+}
+qstr = qobject_to_qstring(qobj);
+if (!qstr) {
+error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
+   full_name(qiv, name), "string");
+return;
+}
+
+if (qemu_strtoi64(qstring_get_str(qstr), NULL, 0, obj) < 0) {
+/* TODO report -ERANGE more nicely */
+error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+   full_name(qiv, name), "integer");
+}
+}
+
 static void qobject_input_type_uint64(Visitor *v, const char *name,
   uint64_t *obj, Error **errp)
 {
@@ -357,6 +383,30 @@ static void qobject_input_type_uint64(Visitor *v, const 
char *name,
 *obj = qint_get_int(qint);
 }
 
+static void qobject_input_type_uint64_keyval(Visitor *v, const char *name,
+ uint64_t *obj, Error **errp)
+{
+QObjectInputVisitor *qiv = to_qiv(v);
+QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
+QString *qstr;
+
+if (!qobj) {
+return;
+}
+qstr = qobject_to_qstring(qobj);
+if (!qstr) {
+error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
+   full_name(qiv, name), "string");
+return;
+}
+
+if (qemu_strtou64(qstring_get_str(qstr), NULL, 0, obj) < 0) {
+/* TODO 

Re: [Qemu-devel] [PATCH v2 00/16] Postcopy: Hugepage support

[Dr. David Alan Gilbert]

* Alexey Perevalov (a.pereva...@samsung.com) wrote:
> Hi David,
> 
> 
> On Tue, Feb 21, 2017 at 10:03:14AM +, Dr. David Alan Gilbert wrote:
> > * Alexey Perevalov (a.pereva...@samsung.com) wrote:
> > > 
> > > Hello David,
> > 
> > Hi Alexey,
> > 
> > > On Tue, Feb 14, 2017 at 07:34:26PM +, Dr. David Alan Gilbert wrote:
> > > > * Alexey Perevalov (a.pereva...@samsung.com) wrote:
> > > > > Hi David,
> > > > > 
> > > > > Thank your, now it's clear.
> > > > > 
> > > > > On Mon, Feb 13, 2017 at 06:16:02PM +, Dr. David Alan Gilbert 
> > > > > wrote:
> > > > > > * Alexey Perevalov (a.pereva...@samsung.com) wrote:
> > > > > > >  Hello David!
> > > > > > 
> > > > > > Hi Alexey,
> > > > > > 
> > > > > > > I have checked you series with 1G hugepage, but only in 1 
> > > > > > > Gbit/sec network
> > > > > > > environment.
> > > > > > 
> > > > > > Can you show the qemu command line you're using?  I'm just trying
> > > > > > to make sure I understand where your hugepages are; running 1G 
> > > > > > hostpages
> > > > > > across a 1Gbit/sec network for postcopy would be pretty poor - it 
> > > > > > would take
> > > > > > ~10 seconds to transfer the page.
> > > > > 
> > > > > sure
> > > > > -hda ./Ubuntu.img -name PAU,debug-threads=on -boot d -net nic -net 
> > > > > user
> > > > > -m 1024 -localtime -nographic -enable-kvm -incoming tcp:0: -object
> > > > > memory-backend-file,id=mem,size=1G,mem-path=/dev/hugepages 
> > > > > -mem-prealloc
> > > > > -numa node,memdev=mem -trace events=/tmp/events -chardev
> > > > > socket,id=charmonitor,path=/var/lib/migrate-vm-monitor.sock,server,nowait
> > > > > -mon chardev=charmonitor,id=monitor,mode=control
> > > > 
> > > > OK, it's a pretty unusual setup - a 1G page guest with 1G of guest RAM.
> > > > 
> > > > > > 
> > > > > > > I started Ubuntu just with console interface and gave to it only 
> > > > > > > 1G of
> > > > > > > RAM, inside Ubuntu I started stress command
> > > > > > 
> > > > > > > (stress --cpu 4 --io 4 --vm 4 --vm-bytes 25600 &)
> > > > > > > in such environment precopy live migration was impossible, it 
> > > > > > > never
> > > > > > > being finished, in this case it infinitely sends pages (it looks 
> > > > > > > like
> > > > > > > dpkg scenario).
> > > > > > > 
> > > > > > > Also I modified stress utility
> > > > > > > http://people.seas.harvard.edu/~apw/stress/stress-1.0.4.tar.gz
> > > > > > > due to it wrote into memory every time the same value `Z`. My
> > > > > > > modified version writes every allocation new incremented value.
> > > > > > 
> > > > > > I use google's stressapptest normally; although remember to turn
> > > > > > off the bit where it pauses.
> > > > > 
> > > > > I decided to use it too
> > > > > stressapptest -s 300 -M 256 -m 8 -W
> > > > > 
> > > > > > 
> > > > > > > I'm using Arcangeli's kernel only at the destination.
> > > > > > > 
> > > > > > > I got controversial results. Downtime for 1G hugepage is close to 
> > > > > > > 2Mb
> > > > > > > hugepage and it took around 7 ms (in 2Mb hugepage scenario 
> > > > > > > downtime was
> > > > > > > around 8 ms).
> > > > > > > I made that opinion by query-migrate.
> > > > > > > {"return": {"status": "completed", "setup-time": 6, "downtime": 
> > > > > > > 6, "total-time": 9668, "ram": {"total": 1091379200, 
> > > > > > > "postcopy-requests": 1, "dirty-sync-count": 2, "remaining": 0, 
> > > > > > > "mbps": 879.786851, "transferred": 1063007296, "duplicate": 7449, 
> > > > > > > "dirty-pages-rate": 0, "skipped": 0, "normal-bytes": 1060868096, 
> > > > > > > "normal": 259001}}}
> > > > > > > 
> > > > > > > Documentation says about downtime field - measurement unit is ms.
> > > > > > 
> > > > > > The downtime measurement field is pretty meaningless for postcopy; 
> > > > > > it's only
> > > > > > the time from stopping the VM until the point where we tell the 
> > > > > > destination it
> > > > > > can start running.  Meaningful measurements are only from inside 
> > > > > > the guest
> > > > > > really, or the place latencys.
> > > > > >
> > > > > 
> > > > > Maybe improve it by receiving such information from destination?
> > > > > I wish to do that.
> > > > > > > So I traced it (I added additional trace into postcopy_place_page
> > > > > > > trace_postcopy_place_page_start(host, from, pagesize); )
> > > > > > > 
> > > > > > > postcopy_ram_fault_thread_request Request for HVA=7f6dc000 
> > > > > > > rb=/objects/mem offset=0
> > > > > > > postcopy_place_page_start host=0x7f6dc000 
> > > > > > > from=0x7f6d7000, pagesize=4000
> > > > > > > postcopy_place_page_start host=0x7f6e0e80 
> > > > > > > from=0x55b665969619, pagesize=1000
> > > > > > > postcopy_place_page_start host=0x7f6e0e801000 
> > > > > > > from=0x55b6659684e8, pagesize=1000
> > > > > > > several pages with 4Kb step ...
> > > > > > > postcopy_place_page_start host=0x7f6e0e817000 
> > > > > > > from=0x55b6659694f0, pagesize=1000
> > > > > > > 
> > > > > > > 4K pages, started from 0x7f6e0e80 addre

Re: [Qemu-devel] [PATCH] null-machine: Add support for the "-kernel" parameter

[Thomas Huth]

On 25.01.2017 09:40, Thomas Huth wrote:
> We can have basic support for the "-kernel" parameter quite easily
> by using the generic loader device. This should be enough for most
> boards which do not need special machine-specific magic for loading
> a kernel (and for those that need special magic, the generic "none"
> machine is likely not suitable for using it as an instruction set
> simulator board anyway).
> 
> Signed-off-by: Thomas Huth 
> ---
>  PS: If we can't agree on using the generic loader here, I can also
>  prepare a patch instead that simply prints out an error message
>  if the user tried to use the "-kernel" parameter.
> 
>  hw/core/null-machine.c | 14 ++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/hw/core/null-machine.c b/hw/core/null-machine.c
> index 27c8369..866e699 100644
> --- a/hw/core/null-machine.c
> +++ b/hw/core/null-machine.c
> @@ -5,6 +5,7 @@
>   *
>   * Authors:
>   *  Anthony Liguori   
> + *  Thomas Huth   
>   *
>   * This work is licensed under the terms of the GNU GPL, version 2 or later.
>   * See the COPYING file in the top-level directory.
> @@ -16,6 +17,7 @@
>  #include "qemu/error-report.h"
>  #include "hw/hw.h"
>  #include "hw/boards.h"
> +#include "hw/core/generic-loader.h"
>  #include "sysemu/sysemu.h"
>  #include "exec/address-spaces.h"
>  #include "cpu.h"
> @@ -40,6 +42,18 @@ static void machine_none_init(MachineState *mch)
>  memory_region_allocate_system_memory(ram, NULL, "ram", 
> mch->ram_size);
>  memory_region_add_subregion(get_system_memory(), 0, ram);
>  }
> +
> +/* Load kernel */
> +if (mch->kernel_filename) {
> +DeviceState *loader;
> +
> +loader = qdev_create(sysbus_get_default(), TYPE_GENERIC_LOADER);
> +qdev_prop_set_string(loader, "file", mch->kernel_filename);
> +if (cpu) {
> +qdev_prop_set_uint32(loader, "cpu-num", cpu->cpu_index);
> +}
> +qdev_init_nofail(loader);
> +}
>  }
>  
>  static void machine_none_machine_init(MachineClass *mc)

*ping*

Apparently the discussion has ceased ... can we get a consensus whether
we want to support the "-kernel" parameter for the "none" machine or not?

 Thomas

[Qemu-devel] [PATCH 24/24] keyval: Support lists

[Markus Armbruster]

Additionally permit non-negative integers as key components.  A
dictionary's keys must either be all integers or none.  If all keys
are integers, convert the dictionary to a list.  The set of keys must
be [0,N].

Examples:

* list.1=goner,list.0=null,list.1=eins,list.2=zwei
  is equivalent to JSON [ "null", "eins", "zwei" ]

* a.b.c=1,a.b.0=2
  is inconsistent: a.b.c clashes with a.b.0

* list.0=null,list.2=eins,list.2=zwei
  has a hole: list.1 is missing

Similar design flaw as for objects: there is now way to denote an
empty list.  While interpreting "key absent" as empty list seems
natural (removing a list member from the input string works when there
are multiple ones, so why not when there's just one), it doesn't work:
"key absent" already means "optional list absent", which isn't the
same as "empty list present".

Update the keyval object visitor to use this a.0 syntax in error
messages rather than the usual a[0].

Signed-off-by: Markus Armbruster 
---
 qapi/qobject-input-visitor.c |   5 +-
 tests/test-keyval.c  | 117 
 util/keyval.c| 177 ---
 3 files changed, 286 insertions(+), 13 deletions(-)

diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index 1d7b420..4c159e0 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -41,6 +41,7 @@ struct QObjectInputVisitor {
 
 /* Root of visit at visitor creation. */
 QObject *root;
+bool keyval;/* Assume @root made with keyval_parse() */
 
 /* Stack of objects being visited (all entries will be either
  * QDict or QList). */
@@ -73,7 +74,9 @@ static const char *full_name_nth(QObjectInputVisitor *qiv, 
const char *name,
 g_string_prepend(qiv->errname, name ?: "");
 g_string_prepend_c(qiv->errname, '.');
 } else {
-snprintf(buf, sizeof(buf), "[%u]", so->index);
+snprintf(buf, sizeof(buf),
+ qiv->keyval ? ".%u" : "[%u]",
+ so->index);
 g_string_prepend(qiv->errname, buf);
 }
 name = so->name;
diff --git a/tests/test-keyval.c b/tests/test-keyval.c
index 6eceafb..1ff6035 100644
--- a/tests/test-keyval.c
+++ b/tests/test-keyval.c
@@ -12,6 +12,7 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "qapi/qmp/qstring.h"
 #include "qapi/qobject-input-visitor.h"
 #include "qemu/cutils.h"
 #include "qemu/option.h"
@@ -183,6 +184,71 @@ static void test_keyval_parse(void)
 g_assert(!qdict);
 }
 
+static void check_list012(QList *qlist)
+{
+static const char *expected[] = { "null", "eins", "zwei" };
+int i;
+QString *qstr;
+
+g_assert(qlist);
+for (i = 0; i < ARRAY_SIZE(expected); i++) {
+qstr = qobject_to_qstring(qlist_pop(qlist));
+g_assert(qstr);
+g_assert_cmpstr(qstring_get_str(qstr), ==, expected[i]);
+}
+g_assert(qlist_empty(qlist));
+}
+
+static void test_keyval_parse_list(void)
+{
+Error *err = NULL;
+QDict *qdict, *sub_qdict;
+
+/* Root can't be a list */
+qdict = keyval_parse("0=1", NULL, &err);
+error_free_or_abort(&err);
+g_assert(!qdict);
+
+/* List elements need not be in order */
+qdict = keyval_parse("list.0=null,list.2=zwei,list.1=eins",
+ NULL, &error_abort);
+g_assert_cmpint(qdict_size(qdict), ==, 1);
+check_list012(qdict_get_qlist(qdict, "list"));
+QDECREF(qdict);
+
+/* Multiple indexes, last one wins */
+qdict = keyval_parse("list.1=goner,list.0=null,list.1=eins,list.2=zwei",
+ NULL, &error_abort);
+g_assert_cmpint(qdict_size(qdict), ==, 1);
+check_list012(qdict_get_qlist(qdict, "list"));
+QDECREF(qdict);
+
+/* List at deeper nesting */
+qdict = keyval_parse("a.list.1=eins,a.list.0=null,a.list.2=zwei",
+ NULL, &error_abort);
+g_assert_cmpint(qdict_size(qdict), ==, 1);
+sub_qdict = qdict_get_qdict(qdict, "a");
+g_assert_cmpint(qdict_size(sub_qdict), ==, 1);
+check_list012(qdict_get_qlist(sub_qdict, "list"));
+QDECREF(qdict);
+
+/* Inconsistent dotted keys: both list and dictionary */
+qdict = keyval_parse("a.b.c=1,a.b.0=2", NULL, &err);
+error_free_or_abort(&err);
+g_assert(!qdict);
+qdict = keyval_parse("a.0.c=1,a.b.c=2", NULL, &err);
+error_free_or_abort(&err);
+g_assert(!qdict);
+
+/* Missing list indexes */
+qdict = keyval_parse("list.2=lonely", NULL, &err);
+error_free_or_abort(&err);
+g_assert(!qdict);
+qdict = keyval_parse("list.0=null,list.2=eins,list.2=zwei", NULL, &err);
+error_free_or_abort(&err);
+g_assert(!qdict);
+}
+
 static void test_keyval_visit_bool(void)
 {
 Error *err = NULL;
@@ -459,6 +525,55 @@ static void test_keyval_visit_dict(void)
 visit_free(v);
 }
 
+static void test_keyval_visit_list(void)
+{
+Error *err = NULL;
+Visitor *v;
+QDi

Re: [Qemu-devel] [PATCH v2 3/5] s390x/ipl: Load network boot image

[Cornelia Huck]

On Sat, 25 Feb 2017 07:18:29 +0100
Thomas Huth  wrote:

> On 23.02.2017 13:20, Cornelia Huck wrote:
> > From: Farhan Ali 
> > 
> > Load the network boot image into guest RAM when the boot
> > device selected is a network device. Use some of the reserved
> > space in IplBlockCcw to store the start address of the netboot
> > image.
> > 
> > A user could also use 'chreipl'(diag 308/5) to change the boot device.
> > So every time we update the IPLB, we need to verify if the selected
> > boot device is a network device so we can appropriately load the
> > network boot image.
> > 
> > Signed-off-by: Farhan Ali 
> > Signed-off-by: Cornelia Huck 
> > ---
> >  hw/s390x/ipl.c | 89 
> > ++
> >  hw/s390x/ipl.h |  4 ++-
> >  2 files changed, 92 insertions(+), 1 deletion(-)

> > +static bool is_virtio_net_device(IplParameterBlock *iplb)
> > +{
> > +uint8_t cssid;
> > +uint8_t ssid;
> > +uint16_t devno;
> > +uint16_t schid;
> > +SubchDev *sch = NULL;
> > +
> > +if (iplb->pbt != S390_IPL_TYPE_CCW) {
> > +return false;
> > +}
> > +
> > +devno = be16_to_cpu(iplb->ccw.devno);
> > +ssid = iplb->ccw.ssid & 3;
> > +
> > +for (schid = 0; schid < MAX_SCHID; schid++) {
> > +for (cssid = 0; cssid < MAX_CSSID; cssid++) {
> > +sch = css_find_subch(1, cssid, ssid, schid);
> > +
> > +if (sch && sch->devno == devno) {
> > +return sch->id.cu_model == VIRTIO_ID_NET;
> > +}
> > +}
> > +}
> > +   return false;
> 
> The above line has only 3 instead of 4 spaces. I wonder why checkpatch
> does not complain here...?

Odd. Will fixup.

> 
> > +}

> > +if (ipl->netboot) {
> > +if (load_netboot_image(&err) < 0) {
> > +error_report_err(err);
> > +vm_stop(RUN_STATE_INTERNAL_ERROR);
> > +}
> > +ipl->iplb.ccw.netboot_start_addr = ipl->start_addr;
> 
> Not sure whether it matters, but in case of early errors during
> load_netboot_image(), ipl->start_addr could be used uninitialized here.
> Maybe you should move the "ipl->start_addr = KERN_IMAGE_START;" there at
> the beginning of the function, to make it also the default value for the
> other error cases?

ipl->start_addr has already been set to some value in the realize
function (either the kernel entry address, or the bios address).

But that should not matter with the vm_stop() on error anyway, no?

Re: [Qemu-devel] [PATCH] Removed support for depth!=32

[Gerd Hoffmann]

  Hi,

> (Gerd: can you remind me of the reason why we can assume that
> depth is always 32 here? IIRC it's because the UI layer always
> uses 32 bit depth now but I couldn't convince myself of that
> with a quick look through the ui code...)

qemu-allocated display surfaces (backed by host memory) are always
32bpp.  Display emulation will get such surfaces in case it uses
qemu_create_displaysurface() or qemu_console_resize().  So the display
emulation only needs to be able to generate 32bpp output
(PIXMAN_x8r8g8b8 to be exact) from whatever the guest creates.

There is also the option to create display surfaces which are backed by
guest ram or vga device memory, using qemu_create_displaysurface_from(),
those can have a different format (any pixman supported format is
allowed here, because the ui code will use pixman to convert those
surfaces if needed).

sm501 uses qemu_console_resize (see line 1320), so yes, the code will
never see a display surface with a depth != 32 and a whole bunch of code
can be dropped.  The need for the include template goes away too.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2 3/5] s390x/ipl: Load network boot image

[Thomas Huth]

On 27.02.2017 12:51, Cornelia Huck wrote:
> On Sat, 25 Feb 2017 07:18:29 +0100
> Thomas Huth  wrote:
> 
>> On 23.02.2017 13:20, Cornelia Huck wrote:
>>> From: Farhan Ali 
>>>
>>> Load the network boot image into guest RAM when the boot
>>> device selected is a network device. Use some of the reserved
>>> space in IplBlockCcw to store the start address of the netboot
>>> image.
>>>
>>> A user could also use 'chreipl'(diag 308/5) to change the boot device.
>>> So every time we update the IPLB, we need to verify if the selected
>>> boot device is a network device so we can appropriately load the
>>> network boot image.
>>>
>>> Signed-off-by: Farhan Ali 
>>> Signed-off-by: Cornelia Huck 
>>> ---
>>>  hw/s390x/ipl.c | 89 
>>> ++
>>>  hw/s390x/ipl.h |  4 ++-
>>>  2 files changed, 92 insertions(+), 1 deletion(-)
[...]
>>> +if (ipl->netboot) {
>>> +if (load_netboot_image(&err) < 0) {
>>> +error_report_err(err);
>>> +vm_stop(RUN_STATE_INTERNAL_ERROR);
>>> +}
>>> +ipl->iplb.ccw.netboot_start_addr = ipl->start_addr;
>>
>> Not sure whether it matters, but in case of early errors during
>> load_netboot_image(), ipl->start_addr could be used uninitialized here.
>> Maybe you should move the "ipl->start_addr = KERN_IMAGE_START;" there at
>> the beginning of the function, to make it also the default value for the
>> other error cases?
> 
> ipl->start_addr has already been set to some value in the realize
> function (either the kernel entry address, or the bios address).
> 
> But that should not matter with the vm_stop() on error anyway, no?

Likely not. It's just a little bit strange to see that the program flow
continues after the error in this function here ... maybe it would have
been clearer to put a "return" right after the "vm_stop()"? Anyway, it
likely does not really matter here, so never mind.

 Thomas

Re: [Qemu-devel] [PATCH v3 0/4] bcm2835: add sdhost and gpio controllers

[Peter Maydell]

On 24 February 2017 at 16:40, Clement Deschamps
 wrote:
> This patches add the Arasan SDHost controller and the GPIO controller to the 
> BCM2835/36 platforms.
>
> This patches have been tested with raspbian 2015-09-24 (which uses the SDHCI 
> controller),
> and raspbian 2017-01-11 (which dynamically switches to the SDHost controller).

Thanks; applied to target-arm.next; I'll put out a pullreq either today
or tomorrow with these in.

> I spotted an issue with the raspi2 platform, but it is not related to this 
> patch.
> The CP15 timer frequency is set to 19.20MHz in the raspbian kernel DTB.
> There is a constant (GTIMER_SCALE) in target/arm/internals.h which seems to 
> tell that the timer is at 62.5MHz.
> This causes problems:
> - The sleep command entered in guest lasts around 1/3 of the time it is 
> supposed to last.
> - The cursor is blinking faster that it should
> - The systemd timeouts expire too early.
> - ...
>
> Did you already have this issue on another platform ?
> Should the frequency in the DTB be dynamically replaced depending on the 
> GTIMER_SCALE value ?
> Or should the constant GTIMER_SCALE be platform dependent ?

We haven't noticed that on other platforms -- I think typically the kernel
trusts the value in the cp15 "what frequency are the timers" register,
at least on the "virt" board which gets the most usage.

We probably should have a mechanism for allowing the SoC or
board to set the clock frequency for the timer registers, though.
Ideally this would be done with the clocktree patchset that
Fred Konrad has been working on, which provides a generic
mechanism for wiring up "clocks" to devices which tell them how
fast things should tick.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] tests-aio-multithread: use atomic_read properly

[Greg Kurz]

On Mon, 27 Feb 2017 12:17:26 +0100
Paolo Bonzini  wrote:

> nodes[id].next is written by other threads.  If atomic_read is not used
> (matching atomic_set in mcs_mutex_lock!) the compiler can optimize the
> whole "if" away!
> 
> Reported-by: Alex Bennée 
> Signed-off-by: Paolo Bonzini 
> ---

Cool ! I can use travis again :)

Tested-by: Greg Kurz 

>  tests/test-aio-multithread.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/test-aio-multithread.c b/tests/test-aio-multithread.c
> index f11e990..8b0b40e 100644
> --- a/tests/test-aio-multithread.c
> +++ b/tests/test-aio-multithread.c
> @@ -309,7 +309,7 @@ static void mcs_mutex_lock(void)
>  static void mcs_mutex_unlock(void)
>  {
>  int next;
> -if (nodes[id].next == -1) {
> +if (atomic_read(&nodes[id].next) == -1) {
>  if (atomic_read(&mutex_head) == id &&
>  atomic_cmpxchg(&mutex_head, id, -1) == id) {
>  /* Last item in the list, exit.  */
> @@ -323,7 +323,7 @@ static void mcs_mutex_unlock(void)
>  }
>  
>  /* Wake up the next in line.  */
> -next = nodes[id].next;
> +next = atomic_read(&nodes[id].next);
>  nodes[next].locked = 0;
>  qemu_futex_wake(&nodes[next].locked, 1);
>  }



pgpYhH598o4yh.pgp
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 1/3] block: implement bdrv_snapshot_goto for blkreplay

[Pavel Dovgalyuk]

> From: Kevin Wolf [mailto:kw...@redhat.com]
> Am 31.01.2017 um 12:57 hat Pavel Dovgalyuk geschrieben:
> > This patch enables making snapshots with blkreplay used in
> > block devices.
> >
> > Signed-off-by: Pavel Dovgalyuk 
> 
> Specifically, I think it avoids the blkreplay_open/close sequence. Is
> this what is needed to make it work?

Then I'll need to implement bdrv_open, because there is only bdrv_file_open
for blkreplay now.

Which way is better?

> We should probably mention in the commit message the exact reason why
> implementing .bdrv_snapshot_goto, but not the other snapshot related
> callbacks, fixes things. If you confirm my assumption, I can add that
> while applying.

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH v2 1/5] elf-loader: Allow late loading of elf

[Cornelia Huck]

On Sat, 25 Feb 2017 07:05:27 +0100
Thomas Huth  wrote:

> On 23.02.2017 13:20, Cornelia Huck wrote:
> > From: Farhan Ali 
> > 
> > The current QEMU ROM infrastructure rejects late loading of ROMs.
> > And ELFs are currently loaded as ROM, this prevents delayed loading
> > of ELFs. So when loading ELF, allow the user to specify if ELF should
> > be loaded as ROM or not.
> > 
> > If an ELF is not loaded as ROM, then they are not restored on a
> > guest reboot/reset and so its upto the user to handle the reloading.
> > 
> > Signed-off-by: Farhan Ali 
> > Reviewed-by: Christian Borntraeger 
> > Cc: Peter Maydell 
> > Signed-off-by: Cornelia Huck 
> > ---
> >  hw/core/loader.c | 17 +++--
> >  include/hw/elf_ops.h | 13 +
> >  include/hw/loader.h  | 13 -
> >  3 files changed, 36 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/core/loader.c b/hw/core/loader.c
> > index ee5abd6eb7..9d1af1f6f3 100644
> > --- a/hw/core/loader.c
> > +++ b/hw/core/loader.c
> > @@ -435,6 +435,19 @@ int load_elf_as(const char *filename,
> >  uint64_t *highaddr, int big_endian, int elf_machine,
> >  int clear_lsb, int data_swab, AddressSpace *as)
> >  {
> > +return load_elf_ram(filename, translate_fn, translate_opaque,
> > +pentry, lowaddr, highaddr, big_endian, elf_machine,
> > +clear_lsb, data_swab, as, true);
> > +}
> > +
> > +/* return < 0 if error, otherwise the number of bytes loaded in memory */
> > +int load_elf_ram(const char *filename,
> > + uint64_t (*translate_fn)(void *, uint64_t),
> > + void *translate_opaque, uint64_t *pentry, uint64_t 
> > *lowaddr,
> > + uint64_t *highaddr, int big_endian, int elf_machine,
> > + int clear_lsb, int data_swab, AddressSpace *as,
> > + bool load_rom)
> > +{
> >  int fd, data_order, target_data_order, must_swab, ret = 
> > ELF_LOAD_FAILED;
> >  uint8_t e_ident[EI_NIDENT];
> 
> 
> 
> The patch looks basically fine to me, but I think it's a little bit
> confusing to have a function called load_elf_ram() which can also be
> used to load ROMs with a load_rom=1 parameter. If I read
> "load_elf_ram()", I'd expect a function that can only read ELFs to RAM.
> So what about adding the "load_rom" parameter to load_elf_as() instead
> and then making load_elf_ram() a wrapper function to that one with
> load_rom=0 ? AFAICS there is only one additional caller to load_elf_as
> (in the generic-loader), so the additional effort here should be OK, I
> think.
> 
> 
> 
>  Thomas
> 

I think both approaches are fine, but I'd prefer to keep the bikeshed
colour to avoid a re-spin :)

Re: [Qemu-devel] [PATCH v16 08/22] qcow2: add bitmaps extension

[Max Reitz]

On 25.02.2017 18:07, Vladimir Sementsov-Ogievskiy wrote:
> Add bitmap extension as specified in docs/specs/qcow2.txt.
> For now, just mirror extension header into Qcow2 state and check
> constraints. Also, calculate refcounts for qcow2 bitmaps, to not break
> qemu-img check.
> 
> For now, disable image resize if it has bitmaps. It will be fixed later.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/Makefile.objs|   2 +-
>  block/qcow2-bitmap.c   | 439 
> +
>  block/qcow2-refcount.c |   6 +
>  block/qcow2.c  | 124 +-
>  block/qcow2.h  |  27 +++
>  5 files changed, 592 insertions(+), 6 deletions(-)
>  create mode 100644 block/qcow2-bitmap.c

Somehow I have the feeling Kevin will find bad things in this patch, but
since I have already approved of all of the previous patches this one is
composed of and the changes on top of that look OK to me:

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 15/54] block: Involve block drivers in permission granting

[Kevin Wolf]

Am 22.02.2017 um 15:04 hat Max Reitz geschrieben:
> > @@ -1390,6 +1565,7 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState 
> > *child_bs,
> >  
> >  ret = bdrv_check_update_perm(child_bs, perm, shared_perm, NULL, errp);
> >  if (ret < 0) {
> > +bdrv_abort_perm_update(child_bs);
> >  return NULL;
> >  }
> >  
> 
> This function doesn't call bdrv_set_perm(). Intentional?

Yes, intentional. It calls it indirectly via bdrv_replace_child(). I'll
add a comment.

Kevin


pgpq5h0FtErg8.pgp
Description: PGP signature

Re: [Qemu-devel] [PATCH 15/54] block: Involve block drivers in permission granting

[Max Reitz]

On 27.02.2017 13:28, Kevin Wolf wrote:
> Am 22.02.2017 um 15:04 hat Max Reitz geschrieben:
>>> @@ -1390,6 +1565,7 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState 
>>> *child_bs,
>>>  
>>>  ret = bdrv_check_update_perm(child_bs, perm, shared_perm, NULL, errp);
>>>  if (ret < 0) {
>>> +bdrv_abort_perm_update(child_bs);
>>>  return NULL;
>>>  }
>>>  
>>
>> This function doesn't call bdrv_set_perm(). Intentional?
> 
> Yes, intentional. It calls it indirectly via bdrv_replace_child(). I'll
> add a comment.

Right, bdrv_replace_child() invokes bdrv_update_perm() which invokes
bdrv_set_perm(). However, that means that there shouldn't be any caller
of bdrv_replace_child() which doesn't call bdrv_check_perm() before;
some don't do that, though, e.g. change_parent_backing_link().

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 18/54] block: Default .bdrv_child_perm() for format drivers

[Kevin Wolf]

Am 25.02.2017 um 12:57 hat Max Reitz geschrieben:
> On 21.02.2017 15:58, Kevin Wolf wrote:
> > Almost all format drivers have the same characteristics as far as
> > permissions are concerned: They have one or more children for storing
> > their own data and, more importantly, metadata (can be written to and
> > grow even without external write requests, must be protected against
> > other writers and present consistent data) and optionally a backing file
> > (this is just data, so like for a filter, it only depends on what the
> > parent nodes need).
> > 
> > This provides a default implementation that can be shared by most of
> > our format drivers.
> > 
> > Signed-off-by: Kevin Wolf 
> > ---
> >  block.c   | 42 ++
> >  include/block/block_int.h |  8 
> >  2 files changed, 50 insertions(+)
> > 
> > diff --git a/block.c b/block.c
> > index 523cbd3..f2e7178 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -1554,6 +1554,48 @@ void bdrv_filter_default_perms(BlockDriverState *bs, 
> > BdrvChild *c,
> > (c->shared_perm & DEFAULT_PERM_UNCHANGED);
> >  }
> >  
> > +void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c,
> > +   const BdrvChildRole *role,
> > +   uint64_t perm, uint64_t shared,
> > +   uint64_t *nperm, uint64_t *nshared)
> > +{
> > +bool backing = (role == &child_backing);
> > +assert(role == &child_backing || role == &child_file);
> > +
> > +if (!backing) {
> > +/* Apart from the modifications below, the same permissions are
> > + * forwarded and left alone as for filters */
> > +bdrv_filter_default_perms(bs, c, role, perm, shared, &perm, 
> > &shared);
> > +
> > +/* Format drivers may touch metadata even if the guest doesn't 
> > write */
> > +if (!bdrv_is_read_only(bs)) {
> > +perm |= BLK_PERM_WRITE | BLK_PERM_RESIZE;
> > +}
> > +
> > +/* bs->file always needs to be consistent because of the metadata. 
> > We
> > + * can never allow other users to resize or write to it. */
> > +perm |= BLK_PERM_CONSISTENT_READ;
> > +shared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE);
> > +} else {
> > +/* We want consistent read from backing files if the parent needs 
> > it.
> > + * No other operations are performed on backing files. */
> > +perm &= BLK_PERM_CONSISTENT_READ;
> > +
> > +/* If the parent can deal with changing data, we're okay with a
> > + * writable and resizable backing file. */
> > +if (shared & BLK_PERM_WRITE) {
> > +shared = BLK_PERM_WRITE | BLK_PERM_RESIZE;
> 
> Wouldn't this break CONSISTENT_READ?

WRITE (even for multiple users) and CONSISTENT_READ aren't mutually
exclusive. I was afraid that I didn't define CONSISTENT_READ right, but
it appears that the definition is fine:

 * A user that has the "permission" of consistent reads is guaranteed that
 * their view of the contents of the block device is complete and
 * self-consistent, representing the contents of a disk at a specific
 * point.

Kevin


pgpMUuIsTl9kq.pgp
Description: PGP signature

Re: [Qemu-devel] Question regarding Snapshots

[Kashyap Chamarthy]

On Mon, Feb 27, 2017 at 11:01:06AM +, Stefan Hajnoczi wrote:
> On Wed, Feb 22, 2017 at 03:06:41PM -0600, Tim Cusack wrote:
> > I have a scenario that perhaps not many have attempted, but I still hope
> > that it is possible.
> > 
> > Scenario:
> > 
> > I would like to have a Base Windows 7 VM in KVM/QEMU.
> > 
> > I would follow the following process (confirmed it works)
> > 
> > 1. Shutdown running domain
> > - virsh shutdown 
> > 2. Remove existing disk from Domain XML:
> > - virt-xml BASE --remove-device --disk target=hda
> > 3. Add Correct Disk image for Snapshot to Domain XML:
> > - virt-xml BASE --add-device --disk
> > /var/lib/libvirt/images/BASE.qcow2,format=qcow2,target=hda,bus=ide
> > 4. Create the snapshot with description
> > - virsh snapshot-create-as BASE .qcow2 "OEM, Model" --disk-only
> > --atomic
> > 5. Start Snapshot
> > - virsh start FA2BASE
> 
> This process uses libvirt instead of QEMU commands.  Adding libvirt
> mailing list.
> 
> > 
> > All that works, but my question and issue is this:
> > 
> > Can you make a snapshot, then go back to the base and ignore that snapshot
> > like a closed branch, make another snapshot, and then go back and forth
> > from each to each?

If you edit the '--disk' element as above and point to the right
'snapshot', you should be able to switch between several of them.


The correct term is 'overlay', let me quote Eric Blake,
from his 2015 KVM Forum presentation ("Backing Chain Management" --
recommend it), where Eric warns about points in time vs. file names:

Given the chain “A <- B <- C”, we have 2 points in time and an
active layer:

  - Point 1: Guest state when B was created, contained in file A
  - Point 2: Guest state when C was created, contained in A+B
  - Active layer: Current guest state, contained in A+B+C

Be careful with naming choices: Naming a file after the time it
is created is misleading -- the guest data for that point in
time is NOT contained in that file.  Rather, think of files as a
delta from the backing file.) 


And conveniently, you can tell libvirt to not track the metadata:

  $ virsh snapshot-create-as --domain vm1 guest-state1 \
  --diskspec vda,file=/export/overlay1.qcow2 \
  --disk-only --atomic --no-metadata

This way, libvirt will not track 'overlay1.qcow2'.  But if you do need
it, as you know, just update the 


For the long answer to the complications involved in reverting to
external snapshots, refer this (long read):

https://wiki.libvirt.org/page/I_created_an_external_snapshot,_but_libvirt_will_not_let_me_delete_or_revert_to_it

> > My reasons for needing this are that I have tools and software that doesnt
> > play nice together, and would like to keep them separated on different
> > snapshots, but never really need to have more than one up at a time.
> > 
> > This would allow me to have only 1 windows license per computer, and the
> > ability to just change from tool to tool with simple front end to virsh to
> > remove the disk from the .xml and add the other one.
> > 
> > So like this:
> > Tool A is on snapshot A
> > Tool B is on snapshot B
> > go through steps 1-5 above to go back and forth.
> > 
> > I know that I can not merge things back into the base, 

How did you arrive at that conclusion?  I realize you tell further below
that you don't need it, but I should point out that it _is_ possible, if
need be, to merge things into base:


http://wiki.libvirt.org/page/Live-merge-an-entire-disk-image-chain-including-current-active-disk

> > but I really don't
> > need to do that, in fact, I can throw away the tool snapshots when new
> > tools come out, by making new snapshots from the base when new tools come
> > out.
> > 
> > We have done this already, but we had an issue where the ability to write
> > to the snapshots seemed to stop.

Without more details like libvirt logs (with QEMU log filters), hard to
tell what's going on.

> > Reason unknown, everything worked one day and next could not write to any
> > snapshot, but could still load the base and work on it fine.
> > 
> > This might be the wrong mailing list to post to, if so, could anyone point
> > out a more appropriate one?
> > 
> > Tim



-- 
/kashyap

Re: [Qemu-devel] [PATCH] nios2: iic: Convert CPU prop to qom link

[Igor Mammedov]

On Sun, 26 Feb 2017 17:48:15 +0100
Marek Vasut  wrote:

> Add a const qom link between the CPU and the IIC instead
> of passing the CPU link through a qom property.
> 
> Signed-off-by: Marek Vasut 
> Cc: Alexander Graf 
> Cc: Chris Wulff 
> Cc: Jeff Da Silva 
> Cc: Ley Foon Tan 
> Cc: Markus Armbruster 
> Cc: Richard Henderson 
> Cc: Sandra Loosemore 
> Cc: Yves Vandervennet 
> ---
>  hw/intc/nios2_iic.c   | 13 -
>  hw/nios2/10m50_devboard.c |  3 ++-
>  2 files changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/intc/nios2_iic.c b/hw/intc/nios2_iic.c
> index 818ab1b315..70550ef8c1 100644
> --- a/hw/intc/nios2_iic.c
> +++ b/hw/intc/nios2_iic.c
> @@ -62,17 +62,15 @@ static void altera_iic_init(Object *obj)
>  sysbus_init_irq(SYS_BUS_DEVICE(obj), &pv->parent_irq);
>  }
>  
> -static Property altera_iic_properties[] = {
> -DEFINE_PROP_PTR("cpu", AlteraIIC, cpu),
> -DEFINE_PROP_END_OF_LIST(),
> -};
> -
>  static void altera_iic_realize(DeviceState *dev, Error **errp)
>  {
>  struct AlteraIIC *pv = ALTERA_IIC(dev);
> +Error *err = NULL;
>  
> +pv->cpu = object_property_get_link(OBJECT(dev), "cpu", &err);
>  if (!pv->cpu) {
> -error_setg(errp, "altera,iic: CPU not connected");
> +error_setg(errp, "altera,iic: CPU link not found: %s",
> +   error_get_pretty(err));
>  return;
>  }
>  }
> @@ -81,9 +79,6 @@ static void altera_iic_class_init(ObjectClass *klass, void 
> *data)
>  {
>  DeviceClass *dc = DEVICE_CLASS(klass);
>  
> -dc->props = altera_iic_properties;
> -/* Reason: pointer property "cpu" */

> -dc->cannot_instantiate_with_device_add_yet = true;
this device is still not device_add creatable
since object_property_add_const_link() is basically the same
as DEFINE_PROP_PTR("cpu", AlteraIIC, cpu), so drop this hunk.

otherwise patch is still improvement as it removes an instance
of obsoleted DEFINE_PROP_PTR

>  dc->realize = altera_iic_realize;
>  }
>  
> diff --git a/hw/nios2/10m50_devboard.c b/hw/nios2/10m50_devboard.c
> index 0d8b9aa58f..c18e0b2a17 100644
> --- a/hw/nios2/10m50_devboard.c
> +++ b/hw/nios2/10m50_devboard.c
> @@ -83,7 +83,8 @@ static void nios2_10m50_ghrd_init(MachineState *machine)
>  
>  /* Register: Internal Interrupt Controller (IIC) */
>  dev = qdev_create(NULL, "altera,iic");
> -qdev_prop_set_ptr(dev, "cpu", cpu);
> +object_property_add_const_link(OBJECT(dev), "cpu", OBJECT(cpu),
> +   &error_abort);
>  qdev_init_nofail(dev);
>  sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, cpu_irq[0]);
>  for (i = 0; i < 32; i++) {

Re: [Qemu-devel] [PATCH 18/54] block: Default .bdrv_child_perm() for format drivers

[Max Reitz]

On 27.02.2017 13:33, Kevin Wolf wrote:
> Am 25.02.2017 um 12:57 hat Max Reitz geschrieben:
>> On 21.02.2017 15:58, Kevin Wolf wrote:
>>> Almost all format drivers have the same characteristics as far as
>>> permissions are concerned: They have one or more children for storing
>>> their own data and, more importantly, metadata (can be written to and
>>> grow even without external write requests, must be protected against
>>> other writers and present consistent data) and optionally a backing file
>>> (this is just data, so like for a filter, it only depends on what the
>>> parent nodes need).
>>>
>>> This provides a default implementation that can be shared by most of
>>> our format drivers.
>>>
>>> Signed-off-by: Kevin Wolf 
>>> ---
>>>  block.c   | 42 ++
>>>  include/block/block_int.h |  8 
>>>  2 files changed, 50 insertions(+)
>>>
>>> diff --git a/block.c b/block.c
>>> index 523cbd3..f2e7178 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -1554,6 +1554,48 @@ void bdrv_filter_default_perms(BlockDriverState *bs, 
>>> BdrvChild *c,
>>> (c->shared_perm & DEFAULT_PERM_UNCHANGED);
>>>  }
>>>  
>>> +void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c,
>>> +   const BdrvChildRole *role,
>>> +   uint64_t perm, uint64_t shared,
>>> +   uint64_t *nperm, uint64_t *nshared)
>>> +{
>>> +bool backing = (role == &child_backing);
>>> +assert(role == &child_backing || role == &child_file);
>>> +
>>> +if (!backing) {
>>> +/* Apart from the modifications below, the same permissions are
>>> + * forwarded and left alone as for filters */
>>> +bdrv_filter_default_perms(bs, c, role, perm, shared, &perm, 
>>> &shared);
>>> +
>>> +/* Format drivers may touch metadata even if the guest doesn't 
>>> write */
>>> +if (!bdrv_is_read_only(bs)) {
>>> +perm |= BLK_PERM_WRITE | BLK_PERM_RESIZE;
>>> +}
>>> +
>>> +/* bs->file always needs to be consistent because of the metadata. 
>>> We
>>> + * can never allow other users to resize or write to it. */
>>> +perm |= BLK_PERM_CONSISTENT_READ;
>>> +shared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE);
>>> +} else {
>>> +/* We want consistent read from backing files if the parent needs 
>>> it.
>>> + * No other operations are performed on backing files. */
>>> +perm &= BLK_PERM_CONSISTENT_READ;
>>> +
>>> +/* If the parent can deal with changing data, we're okay with a
>>> + * writable and resizable backing file. */
>>> +if (shared & BLK_PERM_WRITE) {
>>> +shared = BLK_PERM_WRITE | BLK_PERM_RESIZE;
>>
>> Wouldn't this break CONSISTENT_READ?
> 
> WRITE (even for multiple users) and CONSISTENT_READ aren't mutually
> exclusive. I was afraid that I didn't define CONSISTENT_READ right, but
> it appears that the definition is fine:
> 
>  * A user that has the "permission" of consistent reads is guaranteed that
>  * their view of the contents of the block device is complete and
>  * self-consistent, representing the contents of a disk at a specific
>  * point.

Right, but writes to the backing file at least to me appear to be a
different matter. If those don't break CONSISTENT_READ, then I don't see
how commit breaks CONSISTENT_READ for the intermediate nodes.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PULL 00/24] MTTCG Base enabling patches with ARM enablement

[Paolo Bonzini]

On 24/02/2017 12:20, Alex Bennée wrote:
> The following changes since commit 2d896b454a0e19ec4c1ddbb0e0b65b7e54fcedf3:
> 
>   Revert "hw/mips: MIPS Boston board support" (2017-02-23 18:04:45 +)
> 
> are available in the git repository at:
> 
>   https://github.com/stsquad/qemu.git tags/pull-mttcg-240217-1
> 
> for you to fetch changes up to ca759f9e387db87e1719911f019bc60c74be9ed8:
> 
>   tcg: enable MTTCG by default for ARM on x86 hosts (2017-02-24 10:32:46 
> +)
> 
> 
> This is the MTTCG pull-request as posted yesterday.

This breaks "-icount auto" on qemu-system-aarch64 with "-M virt" and
AAVMF firmware, in two ways:

1) "-icount auto" doesn't work;

2) "-icount auto -accel tcg,thread=single" hangs fairly early, printing
this on the serial console.  It's okay if it hangs at

[Bds]=End Load Options Dumping=
[Bds]BdsWait ...Zzzz...
[Bds]BdsWait(3)..Zzzz...

(pressing Enter a few times then seems to unhang it), but it now hangs
much earlier than that.


Also, x86 "-accel tcg,thread=multi" prints the scary message on memory
ordering.

Paolo

> 
> Alex Bennée (18):
>   docs: new design document multi-thread-tcg.txt
>   tcg: move TCG_MO/BAR types into own file
>   tcg: add kick timer for single-threaded vCPU emulation
>   tcg: rename tcg_current_cpu to tcg_current_rr_cpu
>   tcg: remove global exit_request
>   tcg: enable tb_lock() for SoftMMU
>   tcg: enable thread-per-vCPU
>   cputlb: add assert_cpu_is_self checks
>   cputlb: tweak qemu_ram_addr_from_host_nofail reporting
>   cputlb and arm/sparc targets: convert mmuidx flushes from varg to bitmap
>   cputlb: add tlb_flush_by_mmuidx async routines
>   cputlb: atomically update tlb fields used by tlb_reset_dirty
>   cputlb: introduce tlb_flush_*_all_cpus[_synced]
>   target-arm/powerctl: defer cpu reset work to CPU context
>   target-arm: don't generate WFE/YIELD calls for MTTCG
>   target-arm: ensure all cross vCPUs TLB flushes complete
>   hw/misc/imx6_src: defer clearing of SRC_SCR reset bits
>   tcg: enable MTTCG by default for ARM on x86 hosts
> 
> Jan Kiszka (1):
>   tcg: drop global lock during TCG code execution
> 
> KONRAD Frederic (2):
>   tcg: add options for enabling MTTCG
>   cputlb: introduce tlb_flush_* async work.
> 
> Pranith Kumar (3):
>   mttcg: translate-all: Enable locking debug in a debug build
>   mttcg: Add missing tb_lock/unlock() in cpu_exec_step()
>   tcg: handle EXCP_ATOMIC exception for system emulation
> 
>  configure  |   6 +
>  cpu-exec-common.c  |   3 -
>  cpu-exec.c |  89 ++---
>  cpus.c | 345 ++---
>  cputlb.c   | 463 
> +
>  docs/multi-thread-tcg.txt  | 350 ++
>  exec.c |  12 +-
>  hw/core/irq.c  |   1 +
>  hw/i386/kvmvapic.c |   4 +-
>  hw/intc/arm_gicv3_cpuif.c  |   3 +
>  hw/misc/imx6_src.c |  58 +-
>  hw/ppc/ppc.c   |  16 +-
>  hw/ppc/spapr.c |   3 +
>  include/exec/cputlb.h  |   2 -
>  include/exec/exec-all.h| 132 +++--
>  include/qom/cpu.h  |  16 ++
>  include/sysemu/cpus.h  |   2 +
>  memory.c   |   2 +
>  qemu-options.hx|  20 ++
>  qom/cpu.c  |  10 +
>  target/arm/arm-powerctl.c  | 202 +---
>  target/arm/arm-powerctl.h  |   2 +
>  target/arm/cpu.c   |   4 +-
>  target/arm/cpu.h   |  18 +-
>  target/arm/helper.c| 219 ++---
>  target/arm/kvm.c   |   7 +-
>  target/arm/machine.c   |  41 +++-
>  target/arm/op_helper.c |  50 -
>  target/arm/psci.c  |   4 +-
>  target/arm/translate-a64.c |   8 +-
>  target/arm/translate.c |  20 +-
>  target/i386/smm_helper.c   |   7 +
>  target/s390x/misc_helper.c |   5 +-
>  target/sparc/ldst_helper.c |   8 +-
>  tcg/i386/tcg-target.h  |  11 ++
>  tcg/tcg-mo.h   |  48 +
>  tcg/tcg.h  |  27 +--
>  translate-all.c|  66 ++-
>  translate-common.c |  21 +-
>  vl.c   |  49 -
>  40 files changed, 1878 insertions(+), 476 deletions(-)
>  create mode 100644 docs/multi-thread-tcg.txt
>  create mode 100644 tcg/tcg-mo.h
> 
> 

Re: [Qemu-devel] [PATCH 37/43] spapr: reuse machine->possible_cpus instead of cores[]

[Igor Mammedov]

On Mon, 27 Feb 2017 09:46:43 +1100
David Gibson  wrote:

> On Fri, Feb 24, 2017 at 02:29:21PM +0100, Igor Mammedov wrote:
> > On Sat, 25 Feb 2017 00:03:57 +1100
> > David Gibson  wrote:
> >   
> > > On Wed, Feb 22, 2017 at 12:05:55PM +0100, Igor Mammedov wrote:  
> > > > Replace SPAPR specific cores[] array with generic
> > > > machine->possible_cpus and store core objects there.
> > > > It makes cores bookkeeping similar to x86 cpus and
> > > > will allow to unify similar code.
> > > > It would allow to replace cpu_index based NUMA node
> > > > mapping with property based one (for -device created
> > > > cores) since possible_cpus carries board defined
> > > > topology/layout.
> > > > 
> > > > Signed-off-by: Igor Mammedov 
> > > > Acked-by: David Gibson 
> > > > Signed-off-by: David Gibson 
> > > 
> > > Sorry Igor, Peter has applied the original version.  Can you resend
> > > these updates as deltas against the applied versions of the patches.
> > >   
> > > > ---
> > > > v3:
> > > >   - drop "// TODO" comment as todo is completed in next patch anyway
> > > > fixes checkpatch error wrt // comment  
> > there is no need for patches on top as the next applied patch
> > 38/43 removed c++ style comment as part of removed
> > spapr_query_hotpluggable_cpus().  
> 
> Ok, good to hear.  What about the other update patch you posted?
that was posted as addition to this patch to fix merge conflict
due to removal of TODO comment here, so ignore it as well.

Re: [Qemu-devel] [PATCH v2 04/28] 9pfs: introduce openat_nofollow() helper

[Stefan Hajnoczi]

On Sun, Feb 26, 2017 at 11:42:03PM +0100, Greg Kurz wrote:
> +int openat_nofollow(int dirfd, const char *path, int flags, mode_t mode)
> +{
> +int fd;
> +
> +fd = dup(dirfd);
> +if (fd == -1) {
> +return -1;
> +}
> +
> +while (*path) {
> +const char *c;
> +int next_fd;
> +char *head;
> +
> +head = g_strdup(path);
> +c = strchr(path, '/');
> +if (c) {
> +head[c - path] = 0;
> +next_fd = openat_dir(fd, head);
> +} else {
> +next_fd = openat_file(fd, head, flags, mode);
> +}
> +g_free(head);
> +if (next_fd == -1) {
> +close_preserve_errno(fd);
> +return -1;
> +}
> +close(fd);
> +fd = next_fd;
> +
> +if (!c) {
> +break;
> +}
> +path = c + 1;
> +}
> +
> +return fd;
> +}

If I understand the Linux openat(2) implementation correctly this
function fails with ENOENT if:

1. An absolute path is given
2. A path contains consecutive slashes ("a///b")

Both of these behaviors are problematic.  If the function doesn't
support absolute paths it should be called relative_openat_nofollow()
and have an error if path[0] == '/'.

I believe guests can pass in paths with consecutive slashes, so the
function must cope with them.


signature.asc
Description: PGP signature

[Qemu-devel] [PATCH 03/17] cpu-exec: remove unnecessary check of cpu->exit_request

[Paolo Bonzini]

The cpu->exit_request check in cpu_loop_exec_tb is unnecessary,
because cpu->tcg_exit_req is always set after cpu->exit_request.
So let the TB exit and we will pick up the exit request later
in cpu_handle_interrupt.

Signed-off-by: Paolo Bonzini 
---
 cpu-exec.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index 2a0dfb0..ea1e155 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -571,10 +571,6 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, 
TranslationBlock *tb,
 uintptr_t ret;
 int32_t insns_left;
 
-if (unlikely(atomic_read(&cpu->exit_request))) {
-return;
-}
-
 trace_exec_tb(tb, tb->pc);
 ret = cpu_tb_exec(cpu, tb);
 tb = (TranslationBlock *)(ret & ~TB_EXIT_MASK);
-- 
2.9.3

[Qemu-devel] [PATCH 01/17] cpu-exec: unify icount_decr and tcg_exit_req

[Paolo Bonzini]

The icount interrupt flag and tcg_exit_req serve almost the same
purpose, let's make them completely the same.

The former TB_EXIT_REQUESTED and TB_EXIT_ICOUNT_EXPIRED cases are
unified, since we can distinguish them from the value of the
interrupt flag.

Signed-off-by: Paolo Bonzini 
---
 cpu-exec.c| 80 ++-
 include/exec/gen-icount.h | 53 +++
 include/qom/cpu.h | 15 +
 qom/cpu.c |  2 +-
 tcg/tcg.h |  1 -
 translate-all.c   |  2 +-
 translate-common.c| 13 +++-
 7 files changed, 76 insertions(+), 90 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index 1a5ad48..6fd3f47 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -186,12 +186,6 @@ static inline tcg_target_ulong cpu_tb_exec(CPUState *cpu, 
TranslationBlock *itb)
 cc->set_pc(cpu, last_tb->pc);
 }
 }
-if (tb_exit == TB_EXIT_REQUESTED) {
-/* We were asked to stop executing TBs (probably a pending
- * interrupt. We've now stopped, so clear the flag.
- */
-atomic_set(&cpu->tcg_exit_req, 0);
-}
 return ret;
 }
 
@@ -575,6 +569,7 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, 
TranslationBlock *tb,
 SyncClocks *sc)
 {
 uintptr_t ret;
+int32_t insns_left;
 
 if (unlikely(atomic_read(&cpu->exit_request))) {
 return;
@@ -584,49 +579,48 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, 
TranslationBlock *tb,
 ret = cpu_tb_exec(cpu, tb);
 tb = (TranslationBlock *)(ret & ~TB_EXIT_MASK);
 *tb_exit = ret & TB_EXIT_MASK;
-switch (*tb_exit) {
-case TB_EXIT_REQUESTED:
+if (*tb_exit != TB_EXIT_REQUESTED) {
+*last_tb = tb;
+return;
+}
+
+*last_tb = NULL;
+insns_left = atomic_read(&cpu->icount_decr.u32);
+atomic_set(&cpu->icount_decr.u16.high, 0);
+if (insns_left < 0) {
 /* Something asked us to stop executing chained TBs; just
  * continue round the main loop. Whatever requested the exit
- * will also have set something else (eg interrupt_request)
- * which we will handle next time around the loop.  But we
- * need to ensure the tcg_exit_req read in generated code
- * comes before the next read of cpu->exit_request or
- * cpu->interrupt_request.
+ * will also have set something else (eg exit_request or
+ * interrupt_request) which we will handle next time around
+ * the loop.  But we need to ensure the zeroing of icount_decr
+ * comes before the next read of cpu->exit_request
+ * or cpu->interrupt_request.
  */
 smp_mb();
-*last_tb = NULL;
-break;
-case TB_EXIT_ICOUNT_EXPIRED:
-{
-/* Instruction counter expired.  */
-#ifdef CONFIG_USER_ONLY
-abort();
-#else
-int insns_left = cpu->icount_decr.u32;
-*last_tb = NULL;
-if (cpu->icount_extra && insns_left >= 0) {
-/* Refill decrementer and continue execution.  */
-cpu->icount_extra += insns_left;
-insns_left = MIN(0x, cpu->icount_extra);
-cpu->icount_extra -= insns_left;
-cpu->icount_decr.u16.low = insns_left;
-} else {
-if (insns_left > 0) {
-/* Execute remaining instructions.  */
-cpu_exec_nocache(cpu, insns_left, tb, false);
-align_clocks(sc, cpu);
-}
-cpu->exception_index = EXCP_INTERRUPT;
-cpu_loop_exit(cpu);
-}
-break;
-#endif
+return;
 }
-default:
-*last_tb = tb;
-break;
+
+/* Instruction counter expired.  */
+assert(use_icount);
+#ifndef CONFIG_USER_ONLY
+if (cpu->icount_extra) {
+/* Refill decrementer and continue execution.  */
+cpu->icount_extra += insns_left;
+insns_left = MIN(0x, cpu->icount_extra);
+cpu->icount_extra -= insns_left;
+cpu->icount_decr.u16.low = insns_left;
+} else {
+/* Execute any remaining instructions, then let the main loop
+ * handle the next event.
+ */
+if (insns_left > 0) {
+cpu_exec_nocache(cpu, insns_left, tb, false);
+align_clocks(sc, cpu);
+}
+cpu->exception_index = EXCP_INTERRUPT;
+cpu_loop_exit(cpu);
 }
+#endif
 }
 
 /* main execution loop */
diff --git a/include/exec/gen-icount.h b/include/exec/gen-icount.h
index 050de59..62d462e 100644
--- a/include/exec/gen-icount.h
+++ b/include/exec/gen-icount.h
@@ -6,58 +6,55 @@
 /* Helpers for instruction counting code generation.  */
 
 static int icount_start_insn_idx;
-static TCGLabel *icount_label;
 static TCGLabel *exitreq_label;
 
 static inline void gen_tb_start(TranslationBlock *tb)
 {
-TCGv_i32 count, flag, imm;
+TCGv_i32 count, imm;
 
 exitreq_label = gen_n

[Qemu-devel] [PATCH 02/17] replay: check icount in cpu exec loop

[Paolo Bonzini]

From: Pavel Dovgalyuk 

This patch adds check to break cpu loop when icount expires without
setting the TB_EXIT_ICOUNT_EXPIRED flag. It happens when there is no
available translated blocks and all instructions were executed.
In icount replay mode unnecessary tb_find will be called (which may
cause an exception) and execution will be non-deterministic.
Because cpu_loop_exec_tb cannot longjmp anymore, we can remove
the anticipated call to align_clocks in cpu_loop_exec_tb, as
well as the SyncClocks *sc argument.

Signed-off-by: Pavel Dovgalyuk 
Message-Id: <002801d2810f$18809c20$4981d460$@ru>
Signed-off-by: Paolo Bonzini 
---
 cpu-exec.c | 13 +
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index 6fd3f47..2a0dfb0 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -554,8 +554,9 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
 qemu_mutex_unlock_iothread();
 }
 
-
-if (unlikely(atomic_read(&cpu->exit_request) || replay_has_interrupt())) {
+/* Finally, check if we need to exit to the main loop.  */
+if (unlikely(atomic_read(&cpu->exit_request)
+|| (use_icount && cpu->icount_decr.u16.low + cpu->icount_extra == 0))) 
{
 atomic_set(&cpu->exit_request, 0);
 cpu->exception_index = EXCP_INTERRUPT;
 return true;
@@ -565,8 +566,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
 }
 
 static inline void cpu_loop_exec_tb(CPUState *cpu, TranslationBlock *tb,
-TranslationBlock **last_tb, int *tb_exit,
-SyncClocks *sc)
+TranslationBlock **last_tb, int *tb_exit)
 {
 uintptr_t ret;
 int32_t insns_left;
@@ -615,10 +615,7 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, 
TranslationBlock *tb,
  */
 if (insns_left > 0) {
 cpu_exec_nocache(cpu, insns_left, tb, false);
-align_clocks(sc, cpu);
 }
-cpu->exception_index = EXCP_INTERRUPT;
-cpu_loop_exit(cpu);
 }
 #endif
 }
@@ -593,7 +592,7 @@ int cpu_exec(CPUState *cpu)
 {
 CPUClass *cc = CPU_GET_CLASS(cpu);
 int ret;
-SyncClocks sc;
+SyncClocks sc = { 0 };
 
 /* replay_interrupt may need current_cpu */
 current_cpu = cpu;
@@ -677,7 +674,7 @@ int cpu_exec(CPUState *cpu)
 
 while (!cpu_handle_interrupt(cpu, &last_tb)) {
 TranslationBlock *tb = tb_find(cpu, last_tb, tb_exit);
-cpu_loop_exec_tb(cpu, tb, &last_tb, &tb_exit, &sc);
+cpu_loop_exec_tb(cpu, tb, &last_tb, &tb_exit);
 /* Try to align the host and virtual clocks
if the guest is in advance */
 align_clocks(&sc, cpu);
-- 
2.9.3

[Qemu-devel] [PULL v2 00/17] KVM and cpu-exec patches for 2.9 soft freeze

[Paolo Bonzini]

The following changes since commit d992f2f1368ceb92e6bfd8efece174110f4236ff:

  Merge remote-tracking branch 'remotes/artyom/tags/pull-sun4v-20170226' into 
staging (2017-02-26 22:40:23 +)

are available in the git repository at:

  git://github.com/bonzini/qemu.git tags/for-upstream

for you to fetch changes up to f601681feff62113b526d4ad491af4a03aca825a:

  qmp-events: fix GUEST_PANICKED description formatting (2017-02-27 13:31:25 
+0100)

v1->v2: add missing #ifdef KVM_HAVE_MCE_INJECTION

rebase over MTTCG pull request.  -icount is currently broken but
I'd like to preserve bisectability, so this pull request brings
in the icount patches through a merge commit.


* kernel header update (requested by David and Vijay)
* GuestPanicInformation fixups (Anton)
* record/replay icount fixes (Pavel)
* cpu-exec cleanup, unification of icount_decr with tcg_exit_req (me)
* KVM_CAP_IMMEDIATE_EXIT support (me)
* vmxcap update (me)


Anton Nefedov (2):
  qapi: flatten GuestPanicInformation union
  qmp-events: fix GUEST_PANICKED description formatting

Paolo Bonzini (14):
  cpu-exec: unify icount_decr and tcg_exit_req
  cpu-exec: remove unnecessary check of cpu->exit_request
  update-linux-headers: update for 4.11
  update Linux headers to 4.11
  cpus: remove ugly cast on sigbus_handler
  KVM: x86: cleanup SIGBUS handlers
  cpus: reorganize signal handling code
  KVM: remove kvm_arch_on_sigbus
  KVM: do not use sigtimedwait to catch SIGBUS
  KVM: move SIG_IPI handling to kvm-all.c
  kvm: use atomic_read/atomic_set to access cpu->exit_request
  KVM: use KVM_CAP_IMMEDIATE_EXIT
  vmxcap: port to Python 3
  vmxcap: update for September 2016 SDM

Pavel Dovgalyuk (1):
  replay: check icount in cpu exec loop

 cpu-exec.c |  91 ++---
 cpus.c | 102 +
 include/exec/gen-icount.h  |  53 ++-
 include/qemu/compatfd.h|  42 ---
 include/qemu/osdep.h   |  35 ++
 include/qom/cpu.h  |  15 +-
 include/standard-headers/asm-x86/hyperv.h  |   8 +
 include/standard-headers/linux/input-event-codes.h |   2 +-
 include/standard-headers/linux/pci_regs.h  |  25 ++
 include/standard-headers/linux/virtio_ids.h|   1 +
 include/sysemu/kvm.h   |  11 +-
 kvm-all.c  | 150 +++-
 kvm-stub.c |  12 +-
 linux-headers/asm-arm/kvm.h|  15 +
 linux-headers/asm-arm/unistd-common.h  | 357 ++
 linux-headers/asm-arm/unistd-eabi.h|   5 +
 linux-headers/asm-arm/unistd-oabi.h|  17 +
 linux-headers/asm-arm/unistd.h | 419 +
 linux-headers/asm-arm64/kvm.h  |  13 +
 linux-headers/asm-powerpc/kvm.h|  27 ++
 linux-headers/asm-powerpc/unistd.h |   1 +
 linux-headers/asm-x86/kvm_para.h   |  13 +-
 linux-headers/linux/kvm.h  |  24 +-
 linux-headers/linux/kvm_para.h |   2 +
 linux-headers/linux/userfaultfd.h  |  67 +++-
 linux-headers/linux/vfio.h |  10 +
 os-win32.c |   7 +
 qapi-schema.json   |  12 +
 qapi/event.json|   4 +-
 qom/cpu.c  |   2 +-
 scripts/kvm/vmxcap |  23 +-
 scripts/update-linux-headers.sh|  13 +-
 target/arm/kvm.c   |  10 -
 target/i386/cpu.c  |  15 +-
 target/i386/kvm.c  |  81 ++--
 target/mips/kvm.c  |  12 -
 target/ppc/kvm.c   |  10 -
 target/s390x/kvm.c |  10 -
 tcg/tcg.h  |   1 -
 translate-all.c|   2 +-
 translate-common.c |  13 +-
 util/compatfd.c|   1 -
 util/main-loop.c   |   5 +-
 util/oslib-posix.c |  33 ++
 vl.c   |  12 +-
 45 files changed, 972 insertions(+), 811 deletions(-)
 delete mode 100644 include/qemu/compatfd.h
 create mode 100644 linux-headers/asm-arm/unistd-common.h
 create mode 100644 linux-headers/asm-arm/unistd-eabi.h
 create mode 100644 linux-headers/asm-arm/unistd-oabi.h

-- 
2.

[Qemu-devel] [PATCH 09/17] KVM: remove kvm_arch_on_sigbus

[Paolo Bonzini]

Build it on kvm_arch_on_sigbus_vcpu instead.  They do the same
for "action optional" SIGBUSes, and the main thread should never get
"action required" SIGBUSes because it blocks the signal.

Signed-off-by: Paolo Bonzini 
---
 include/sysemu/kvm.h |  1 -
 kvm-all.c|  9 -
 target/arm/kvm.c |  5 -
 target/i386/kvm.c| 40 +---
 target/mips/kvm.c|  6 --
 target/ppc/kvm.c |  5 -
 target/s390x/kvm.c   |  5 -
 7 files changed, 13 insertions(+), 58 deletions(-)

diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 3045ee7..6ecb61c 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -358,7 +358,6 @@ bool kvm_vcpu_id_is_valid(int vcpu_id);
 unsigned long kvm_arch_vcpu_id(CPUState *cpu);
 
 int kvm_arch_on_sigbus_vcpu(CPUState *cpu, int code, void *addr);
-int kvm_arch_on_sigbus(int code, void *addr);
 
 void kvm_arch_init_irq_routing(KVMState *s);
 
diff --git a/kvm-all.c b/kvm-all.c
index 0c94637..a433ad3 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -2391,6 +2391,7 @@ int kvm_set_signal_mask(CPUState *cpu, const sigset_t 
*sigset)
 
 return r;
 }
+
 int kvm_on_sigbus_vcpu(CPUState *cpu, int code, void *addr)
 {
 return kvm_arch_on_sigbus_vcpu(cpu, code, addr);
@@ -2398,7 +2399,13 @@ int kvm_on_sigbus_vcpu(CPUState *cpu, int code, void 
*addr)
 
 int kvm_on_sigbus(int code, void *addr)
 {
-return kvm_arch_on_sigbus(code, addr);
+/* Action required MCE kills the process if SIGBUS is blocked.  Because
+ * that's what happens in the I/O thread, where we handle MCE via signalfd,
+ * we can only get action optional here.
+ */
+assert(code != BUS_MCEERR_AR);
+kvm_arch_on_sigbus_vcpu(first_cpu, code, addr);
+return 0;
 }
 
 int kvm_create_device(KVMState *s, uint64_t type, bool test)
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index 395e986..e5218f6 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -565,11 +565,6 @@ int kvm_arch_on_sigbus_vcpu(CPUState *cs, int code, void 
*addr)
 return 1;
 }
 
-int kvm_arch_on_sigbus(int code, void *addr)
-{
-return 1;
-}
-
 /* The #ifdef protections are until 32bit headers are imported and can
  * be removed once both 32 and 64 bit reach feature parity.
  */
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index f49a786..2adf992 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -462,14 +462,13 @@ int kvm_arch_on_sigbus_vcpu(CPUState *c, int code, void 
*addr)
 ram_addr_t ram_addr;
 hwaddr paddr;
 
+/* If we get an action required MCE, it has been injected by KVM
+ * while the VM was running.  An action optional MCE instead should
+ * be coming from the main thread, which qemu_init_sigbus identifies
+ * as the "early kill" thread.
+ */
 assert(code == BUS_MCEERR_AR || code == BUS_MCEERR_AO);
 
-/* Because the MCE happened while running the VCPU, KVM could have
- * injected action required MCEs too.  Action optional MCEs should
- * be delivered to the main thread, which qemu_init_sigbus identifies
- * as the "early kill" thread, but if we get one for whatever reason
- * we just handle it just like the main thread would.
- */
 if ((env->mcg_cap & MCG_SER_P) && addr) {
 ram_addr = qemu_ram_addr_from_host(addr);
 if (ram_addr != RAM_ADDR_INVALID &&
@@ -491,35 +490,6 @@ int kvm_arch_on_sigbus_vcpu(CPUState *c, int code, void 
*addr)
 return 0;
 }
 
-int kvm_arch_on_sigbus(int code, void *addr)
-{
-X86CPU *cpu = X86_CPU(first_cpu);
-
-assert(code == BUS_MCEERR_AR || code == BUS_MCEERR_AO);
-
-if (code == BUS_MCEERR_AR) {
-hardware_memory_error();
-}
-
-/* Hope we are lucky for AO MCE */
-if ((cpu->env.mcg_cap & MCG_SER_P) && addr) {
-ram_addr_t ram_addr;
-hwaddr paddr;
-
-ram_addr = qemu_ram_addr_from_host(addr);
-if (ram_addr != RAM_ADDR_INVALID &&
-kvm_physical_memory_addr_from_host(first_cpu->kvm_state,
-   addr, &paddr)) {
-kvm_hwpoison_page_add(ram_addr);
-kvm_mce_inject(X86_CPU(first_cpu), paddr, code);
-}
-
-fprintf(stderr, "Hardware memory error for memory used by "
-"QEMU itself instead of guest system!: %p\n", addr);
-}
-return 0;
-}
-
 static int kvm_inject_mce_oldstyle(X86CPU *cpu)
 {
 CPUX86State *env = &cpu->env;
diff --git a/target/mips/kvm.c b/target/mips/kvm.c
index 998c341..3e686e7 100644
--- a/target/mips/kvm.c
+++ b/target/mips/kvm.c
@@ -186,12 +186,6 @@ int kvm_arch_on_sigbus_vcpu(CPUState *cs, int code, void 
*addr)
 return 1;
 }
 
-int kvm_arch_on_sigbus(int code, void *addr)
-{
-DPRINTF("%s\n", __func__);
-return 1;
-}
-
 void kvm_arch_init_irq_routing(KVMState *s)
 {
 }
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 52bbea5..bc011c6 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -2587,11 +2587,6 @@ int kvm_a