Re: [Qemu-devel] [PATCH v2 1/1] parallels: add format spec

[Stefan Hajnoczi]

On Mon, Nov 23, 2015 at 04:32:37PM +0300, Denis V. Lunev wrote:
> From: Vladimir Sementsov-Ogievskiy 
> 
> This specifies Parallels image format as implemented in Parallels Cloud
> Server 6.10
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> Signed-off-by: Denis V. Lunev 
> CC: Eric Blake 
> CC: John Snow 
> CC: Stefan Hajnoczi 
> ---
> v2: add license info
> switch to offsets from types in field descriptions

Cool!  Thanks for publishing a specification.  This will help everyone
understand the code.

> +=== Dirty bitmaps feature ===
> +
> +This feature provides a way of storing dirty bitmaps in the image. The fields
> +of its data area are:
> +
> +   0 -  7:size
> +  The bitmap size, should be equal to disk size in sectors.
> +
> +   8 - 23:id
> +  An identifier for backup consistency checking.
> +
> +  24 - 27:granularity
> +  Bitmap granularity, in sectors. I.e., the number of sectors
> +  corresponding to one bit of the bitmap.

Does this need to be a power of 2?

> +  28 - 31:l1_size
> +  The number of entries in the L1 table of the bitmap.
> +
> +  variable:   l1 (64 * l1_size bytes)
> +  L1 offset table (in bytes)
> +
> +A dirty bitmap is stored using a one-level structure for the mapping to host
> +clusters - an L1 table.
> +
> +Given an offset into the bitmap, the offset in bytes into the image file can 
> be

What are the units of the offset into the bitmap?  Is it a bit number
(i.e. sector number / granularity)?


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v3 for-2.5 05/12] qjson: Give each of the six structural chars its own token type

[Markus Armbruster]

Eric Blake  writes:

> On 11/25/2015 02:23 PM, Markus Armbruster wrote:
>> Simplifies things, because we always check for a specific one.
>> 
>> Signed-off-by: Markus Armbruster 
>> ---
>>  include/qapi/qmp/json-lexer.h |  7 ++-
>>  qobject/json-lexer.c  | 19 ---
>>  qobject/json-parser.c | 31 +--
>>  qobject/json-streamer.c   | 32 +++-
>>  4 files changed, 42 insertions(+), 47 deletions(-)
>
> Diffstat shows that it is already a win, even if slight; the real win is
> that later patches are easier :)
>
>> 
>> diff --git a/include/qapi/qmp/json-lexer.h b/include/qapi/qmp/json-lexer.h
>> index 61a143f..f3e8dc7 100644
>> --- a/include/qapi/qmp/json-lexer.h
>> +++ b/include/qapi/qmp/json-lexer.h
>> @@ -19,7 +19,12 @@
>>  
>>  typedef enum json_token_type {
>>  JSON_MIN = 100,
>> -JSON_OPERATOR = JSON_MIN,
>> +JSON_LCURLY = JSON_MIN,
>> +JSON_RCURLY,
>> +JSON_LSQUARE,
>> +JSON_RSQUARE,
>
> I might have used LBRACE and LBRACKET - but I also acknowledge that UK
> spellers think of '()' for 'bracket'.  Your naming is fine (unless you
> really want that bikeshed to be chartreuse).

I normally use (parenthesis), [bracket] and {brace} myself, but here I
decided to stick to RFC 7159's wording:

  begin-array = ws %x5B ws  ; [ left square bracket

  begin-object= ws %x7B ws  ; { left curly bracket

  end-array   = ws %x5D ws  ; ] right square bracket

  end-object  = ws %x7D ws  ; } right curly bracket

  name-separator  = ws %x3A ws  ; : colon

  value-separator = ws %x2C ws  ; , comma

> Reviewed-by: Eric Blake 

Thanks!

Re: [Qemu-devel] [PATCH v3 for-2.5 06/12] qjson: Inline token_is_keyword() and simplify

[Markus Armbruster]

Eric Blake  writes:

> On 11/25/2015 02:23 PM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster 
>> ---
>>  qobject/json-parser.c | 20 +++-
>>  1 file changed, 7 insertions(+), 13 deletions(-)
>> 
>
>>  
>> -if (token_is_keyword(token, "true")) {
>> +val = token_get_value(token);
>> +
>> +if (!strcmp(val, "true")) {
>>  ret = QOBJECT(qbool_from_bool(true));
>> -} else if (token_is_keyword(token, "false")) {
>> +} else if (!strcmp(val, "false")) {
>>  ret = QOBJECT(qbool_from_bool(false));
>> -} else if (token_is_keyword(token, "null")) {
>> +} else if (!strcmp(val, "null")) {
>>  ret = qnull();
>>  } else {
>> -parse_error(ctxt, token, "invalid keyword `%s'", 
>> token_get_value(token));
>> +parse_error(ctxt, token, "invalid keyword '%s'", val);
>
> Yay - fewer `' in error messages.  (Great back in the day when fonts
> rendered them symmetrically, and still useful in m4; but lousy for
> pasting into shell code and in modern fonts)

Perhaps we can some day use the proper U+2018 (LEFT SINGLE QUOTATION
MARK) and U+2019 (RIGHT SINGLE QUOTATION MARK).

> Reviewed-by: Eric Blake 

Thanks!

Re: [Qemu-devel] [PATCH v3 for-2.5 07/12] qjson: Inline token_is_escape() and simplify

[Markus Armbruster]

Eric Blake  writes:

> On 11/25/2015 02:23 PM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster 
>> ---
>>  qobject/json-parser.c | 32 +++-
>>  1 file changed, 15 insertions(+), 17 deletions(-)
>> 
>
>> +if (!strcmp(val, "%p")) {
>>  obj = va_arg(*ap, QObject *);
>> -} else if (token_is_escape(token, "%i")) {
>> +} else if (!strcmp(val, "%i")) {
>>  obj = QOBJECT(qbool_from_bool(va_arg(*ap, int)));
>> -} else if (token_is_escape(token, "%d")) {
>> +} else if (!strcmp(val, "%d")) {
>>  obj = QOBJECT(qint_from_int(va_arg(*ap, int)));
>> -} else if (token_is_escape(token, "%ld")) {
>> +} else if (!strcmp(val, "%ld")) {
>
> Not for this patch, but I'd love to kill our support for "%ld" - it has
> behavior that differs between 32-bit and 64-bit platforms, and is
> therefore useless for our goal of using fixed-width integer types.
>
>>  obj = QOBJECT(qint_from_int(va_arg(*ap, long)));
>> -} else if (token_is_escape(token, "%lld") ||
>> -   token_is_escape(token, "%I64d")) {
>> +} else if (!strcmp(val, "%lld") ||
>> +   !strcmp(val, "%I64d")) {
>
> Not for this patch, but I'd love to kill our support for "%I64d". Isn't
> modern mingw friendlier to using POSIX escape sequences in printf()?
> I'm assuming mingw is the only reason we have this hold-out?

These escapes are chosen so we can get type checking from gcc via format
attribute.  The code here is somewhat unclean; it should arguably
compare to "%" PRId64, whatever that may be.  Instead, we accept
anything we anticipate it could be.

> Reviewed-by: Eric Blake 

Thanks!

Re: [Qemu-devel] [PATCH v1 0/7] KVM: Hyper-V SynIC timers

[Andrey Smetanin]

On 11/26/2015 08:28 AM, Wanpeng Li wrote:

2015-11-25 23:20 GMT+08:00 Andrey Smetanin :

Per Hyper-V specification (and as required by Hyper-V-aware guests),
SynIC provides 4 per-vCPU timers.  Each timer is programmed via a pair
of MSRs, and signals expiration by delivering a special format message
to the configured SynIC message slot and triggering the corresponding
synthetic interrupt.


Could you post a link for this specification?


Official link:

http://download.microsoft.com/download/A/B/4/AB43A34E-BDD0-4FA6-BDEF-79EEF16E880B/Hypervisor%20Top%20Level%20Functional%20Specification%20v4.0.docx

and there is a pdf variant(my own docx -> pdf conversion):

https://www.dropbox.com/s/ehxictr5wgnedq7/Hypervisor%20Top%20Level%20Functional%20Specification%20v4.0.pdf?dl=0



Regards,
Wanpeng Li

Re: [Qemu-devel] [PATCH v3 for-2.5 09/12] qjson: Convert to parser to recursive descent

[Markus Armbruster]

Eric Blake  writes:

> On 11/25/2015 02:23 PM, Markus Armbruster wrote:
>> We backtrack in parse_value(), even though JSON is LL(1) and thus can
>> be parsed by straightforward recursive descent.  Do exactly that.
>> 
>> Based on an almost-correct patch from Paolo Bonzini.
>> 
>> Signed-off-by: Markus Armbruster 
>> ---
>>  qobject/json-parser.c | 165 
>> ++
>>  1 file changed, 47 insertions(+), 118 deletions(-)
>> 
>
>>  static QObject *parse_value(JSONParserContext *ctxt, va_list *ap)
>>  {
>> -QObject *obj;
>> +QObject *token;
>>  
>> -obj = parse_object(ctxt, ap);
>> -if (obj == NULL) {
>> -obj = parse_array(ctxt, ap);
>> -}
>> -if (obj == NULL) {
>> -obj = parse_escape(ctxt, ap);
>> -}
>> -if (obj == NULL) {
>> -obj = parse_keyword(ctxt);
>> -} 
>> -if (obj == NULL) {
>> -obj = parse_literal(ctxt);
>> +token = parser_context_peek_token(ctxt);
>> +if (token == NULL) {
>> +parse_error(ctxt, NULL, "premature EOI");
>
> Should we spell that out as 'end of input'?
>
> But that's cosmetic, and doesn't affect correctness of the conversion.

Doesn't matter, because we generally throw away these error messages,
then make up a useless one *boggle*.  Once that's fixed, the
parse_error() could use some love.

> Reviewed-by: Eric Blake 

Thanks!

Re: [Qemu-devel] [PATCH for-2.5] Avoid memory leak

[stefano]

On 2015/11/26 11:43, Eric Blake wrote:
> On 11/25/2015 06:30 PM, dongxingshui wrote:
>> monitor.c: Avoid memory leak
>>
>> When send a wrong qmp command, a memory leak occurs. Fix it.
> 
> Looks like the leak was introduced in 710aec9; would be worth amending
> the commit message to mention that.

Sorry! I didn't see the mailing list before. It has been fixed by Markus.

> 
> Reviewed-by: Eric Blake 
> 
>>
>> Signed-off-by: dongxingshui 
> 
> This looks like your first patch to qemu (at least I wasn't able to find
> anything from you via 'git log') - welcome to the community.  I suspect
> that 'dongxingshui' is probably your login name; it doesn't give a
> Western reader like me an idea where to break into family and personal
> names.  It's a bit nicer to use a 'Full Name' notation, or even to list
> your name in two forms (Latin and native UTF-8 characters); see this
> recent conversation:

I'm pleased to join the community.
Thank you for your suggestion, i'll change my signed-off message and use the 
following one.
Signed-off-by: Stefano Dong (董兴水) 


> 
> https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg05208.html
> 
>> ---
>>  monitor.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/monitor.c b/monitor.c
>> index e4cf34e..af6cfc5 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -3906,6 +3906,7 @@ static void handle_qmp_command(JSONMessageParser 
>> *parser, QList *tokens)
>>  
>>  err_out:
>>  monitor_protocol_emitter(mon, data, local_err);
>> +error_free(local_err);
>>  qobject_decref(data);
>>  QDECREF(input);
>>  QDECREF(args);
>>
> 

Re: [Qemu-devel] [PATCH for-2.5] Avoid memory leak

[Markus Armbruster]

stefano  writes:

> On 2015/11/26 11:43, Eric Blake wrote:
>> On 11/25/2015 06:30 PM, dongxingshui wrote:
>>> monitor.c: Avoid memory leak
>>>
>>> When send a wrong qmp command, a memory leak occurs. Fix it.
>> 
>> Looks like the leak was introduced in 710aec9; would be worth amending
>> the commit message to mention that.
>
> Sorry! I didn't see the mailing list before. It has been fixed by Markus.
>
>> 
>> Reviewed-by: Eric Blake 
>> 
>>>
>>> Signed-off-by: dongxingshui 
>> 
>> This looks like your first patch to qemu (at least I wasn't able to find
>> anything from you via 'git log') - welcome to the community.  I suspect
>> that 'dongxingshui' is probably your login name; it doesn't give a
>> Western reader like me an idea where to break into family and personal
>> names.  It's a bit nicer to use a 'Full Name' notation, or even to list
>> your name in two forms (Latin and native UTF-8 characters); see this
>> recent conversation:
>
> I'm pleased to join the community.
> Thank you for your suggestion, i'll change my signed-off message and
> use the following one.
> Signed-off-by: Stefano Dong (董兴水) 

Looks good.  Welcome!

Re: [Qemu-devel] [PATCH v1 0/7] KVM: Hyper-V SynIC timers

[Wanpeng Li]

2015-11-26 16:34 GMT+08:00 Andrey Smetanin :
>
>
> On 11/26/2015 08:28 AM, Wanpeng Li wrote:
>>
>> 2015-11-25 23:20 GMT+08:00 Andrey Smetanin :
>>>
>>> Per Hyper-V specification (and as required by Hyper-V-aware guests),
>>> SynIC provides 4 per-vCPU timers.  Each timer is programmed via a pair
>>> of MSRs, and signals expiration by delivering a special format message
>>> to the configured SynIC message slot and triggering the corresponding
>>> synthetic interrupt.
>>
>>
>> Could you post a link for this specification?
>
>
> Official link:
>
> http://download.microsoft.com/download/A/B/4/AB43A34E-BDD0-4FA6-BDEF-79EEF16E880B/Hypervisor%20Top%20Level%20Functional%20Specification%20v4.0.docx
>
> and there is a pdf variant(my own docx -> pdf conversion):
>
> https://www.dropbox.com/s/ehxictr5wgnedq7/Hypervisor%20Top%20Level%20Functional%20Specification%20v4.0.pdf?dl=0

Cool, thanks.

Regards,
Wanpeng Li

Re: [Qemu-devel] [PATCH v1 6/7] kvm/x86: Hyper-V SynIC message slot pending clearing at SINT ack

[Andrey Smetanin]

On 11/25/2015 08:14 PM, Paolo Bonzini wrote:



On 25/11/2015 17:55, Andrey Smetanin wrote:


+gpa = synic->msg_page & PAGE_MASK;
+page = kvm_vcpu_gfn_to_page(vcpu, gpa >> PAGE_SHIFT);
+if (is_error_page(page)) {
+vcpu_err(vcpu, "Hyper-V SynIC can't get msg page, gpa 0x%llx\n",
+ gpa);
+return;
+}
+msg_page = kmap_atomic(page);


But the message page is not being pinned, is it?

Actually I don't know anything about pinning.
Is it pinning against page swapping ?


Yes.  Unless the page is pinned, kmap_atomic can fail.

kmap_atomic() can't fail for a valid page struct. Does
kvm_vcpu_gfn_to_page() can provide invalid page(swapped page) struct 
which may pass is_error_page(page) check but can leads to incorrect

behavior inside kmap_atomic()?


However, I don't think that kvm_hv_notify_acked_sint is called from
atomic context.  It is only called from apic_set_eoi.  Could you just
use kvm_vcpu_write_guest_page?
In this case I can use kvm_vcpu_write_guest_page(), but in the 'PATCH v1 
7/7' I do the same page mapping method to sync_cmpxchg() at guest 
message page address to exclusively acquire message page slot(see 
synic_deliver_msg()). So we need some method to map and access 
atomically memory of guest page in KVM. Does any method to pin and map 
guest page in kernel exists? Or should we use mlock() for this page in 
QEMU part ?


By the way, do you need to do this also in kvm_get_apic_interrupt, for
auto EOI interrupts?
No we don't need this because in case of auto EOI interrupts, if 
->msg_pending was set, host will receive HV_X64_MSR_EOM write request 
which calls kvm_hv_notify_acked_sint().


Thanks,

Paolo


Could you please clarify and provide an API to use in this case ?

Re: [Qemu-devel] [PATCH v3 for-2.5 09/12] qjson: Convert to parser to recursive descent

[Markus Armbruster]

Markus Armbruster  writes:

> We backtrack in parse_value(), even though JSON is LL(1) and thus can
> be parsed by straightforward recursive descent.  Do exactly that.
>
> Based on an almost-correct patch from Paolo Bonzini.
>
> Signed-off-by: Markus Armbruster 

Missing a
Signed-off-by: Paolo Bonzini 

Fixed in my tree.

Re: [Qemu-devel] [PATCH v2] qemu_mutex_iothread_locked not correctly synchronized

[David Engraf]

Am 25.11.2015 um 17:16 schrieb Paolo Bonzini:



On 25/11/2015 16:48, David Engraf wrote:


Indeed, TLS handling is broken. The address of iothread_locked is always
the same between threads and I can see that a different thread sets
iothread_locked to false, thus my current thread uses an invalid state.
I will have to check why my compiler produces invalid TLS code.


That rings a bell, I think there are different CRT libraries or
something like that.  Stefan, what would break TLS under Windows?


"--extra-cflags=-mthreads" is the solution. iothread_locked is unique 
for each thread now. I saw that this is already mentioned in the 
Documentation [1], but why isn't that enabled by default if QEMU 
requires this option on Windows? Without this option, even the latest 
version of gcc doesn't produce working TLS code.


Many thanks for your support!

David


[1] http://wiki.qemu.org/Hosts/W32

Re: [Qemu-devel] [RFC PATCH 00/40] Sneak peek of virtio and dataplane changes for 2.6

[Christian Borntraeger]

On 11/24/2015 07:00 PM, Paolo Bonzini wrote:
> This large series is basically all that I would like to get into 2.6.
> It is a combination of several pieces of work on dataplane and
> multithreaded block layer.
> 
> It's also a large part of why I would like someone else to look at
> miscellaneous patches for a while (in case you've missed that).  I
> can foresee that following the reviews is going to be a huge time drain.
> 
> With it I can get ~1300 Kiops on 8 disks (which I achieve with 2 iothreads
> and 5 VCPUs).  The bulk of the improvement actually comes from the first
> 8 patches, but the rest of the series is what prepares for what's next
> to come in QEMU 2.7 and later, such as a multiqueue block layer.
> 
> It's tedious to review, with some pretty large patches (3, 32, 33, 35).
On 11/24/2015 07:00 PM, Paolo Bonzini wrote:
> This large series is basically all that I would like to get into 2.6.
> It is a combination of several pieces of work on dataplane and
> multithreaded block layer.
> 
> It's also a large part of why I would like someone else to look at
> miscellaneous patches for a while (in case you've missed that).  I
> can foresee that following the reviews is going to be a huge time drain.
> 
> With it I can get ~1300 Kiops on 8 disks (which I achieve with 2 iothreads
> and 5 VCPUs).  The bulk of the improvement actually comes from the first
> 8 patches, but the rest of the series is what prepares for what's next
> to come in QEMU 2.7 and later, such as a multiqueue block layer.

For some unknown reason, this seems to be slightly slower than 2.5-rc1 on my
old z196. (have not net tested the z13)

your branch is certainly better regarding malloc, but worse regarding others.

your branch:

# Overhead Command Shared Object Symbol
#  ... ... 
...
#
3.99% qemu-system-s39 libc-2.18.so  [.] __memcpy_z196
2.66% qemu-system-s39 qemu-system-s390x [.] address_space_lduw_le
2.51% qemu-system-s39 qemu-system-s390x [.] address_space_map
2.51% qemu-system-s39 qemu-system-s390x [.] phys_page_find
2.24% qemu-system-s39 qemu-system-s390x [.] qemu_get_ram_ptr
2.18% qemu-system-s39 libc-2.18.so  [.] _int_malloc
2.18% qemu-system-s39 qemu-system-s390x [.] address_space_translate_internal
2.03% qemu-system-s39 libc-2.18.so  [.] malloc
1.91% qemu-system-s39 qemu-system-s390x [.] qemu_coroutine_switch
1.66% qemu-system-s39 qemu-system-s390x [.] address_space_rw
1.63% qemu-system-s39 qemu-system-s390x [.] address_space_stw_le
1.57% qemu-system-s39 qemu-system-s390x [.] address_space_stl_le
1.57% qemu-system-s39 qemu-system-s390x [.] address_space_translate
1.45% qemu-system-s39 qemu-system-s390x [.] virtqueue_pop
1.33% qemu-system-s39 libc-2.18.so  [.] _int_free
1.27% qemu-system-s39 [kernel.vmlinux]  [k] system_call
1.00% qemu-system-s39 qemu-system-s390x [.] qemu_coroutine_enter
0.94% qemu-system-s39 libc-2.18.so  [.] __sigsetjmp
0.94% qemu-system-s39 libc-2.18.so  [.] free
0.91% qemu-system-s39 qemu-system-s390x [.] qemu_ram_block_from_host
0.88% qemu-system-s39 qemu-system-s390x [.] virtio_blk_handle_request
0.82% qemu-system-s39 qemu-system-s390x [.] object_unref
0.79% qemu-system-s39 qemu-system-s390x [.] vring_desc_read
0.76% qemu-system-s39 qemu-system-s390x [.] qemu_get_ram_block
0.73% qemu-system-s39 libglib-2.0.so.0.3800.2 [.] g_free

2.5.0-rc1:


# Overhead Command Shared Object Symbol
#  ... ... 
...
#
5.10% qemu-system-s39 libc-2.18.so  [.] _int_malloc
3.30% qemu-system-s39 libc-2.18.so  [.] __memcpy_z196
2.83% qemu-system-s39 qemu-system-s390x [.] memory_region_find_rcu
2.72% qemu-system-s39 qemu-system-s390x [.] vring_pop
2.27% qemu-system-s39 qemu-system-s390x [.] qemu_coroutine_switch
2.18% qemu-system-s39 libc-2.18.so  [.] _int_free
1.99% qemu-system-s39 libc-2.18.so  [.] malloc
1.37% qemu-system-s39 qemu-system-s390x [.] address_space_rw
1.37% qemu-system-s39 qemu-system-s390x [.] aio_bh_poll
1.37% qemu-system-s39 qemu-system-s390x [.] qemu_get_ram_ptr
1.34% qemu-system-s39 libc-2.18.so  [.] free
1.29% qemu-system-s39 libc-2.18.so  [.] malloc_consolidate
1.18% qemu-system-s39 qemu-system-s390x [.] memory_region_find
1.09% qemu-system-s39 libglib-2.0.so.0.3800.2 [.] g_malloc
1.06% qemu-system-s39 [kernel.vmlinux] [k] system_call
0.95% qemu-system-s39 [kernel.vmlinux] [k] __schedule
0.95% qemu-system-s39 qemu-system-s390x [.] qemu_coroutine_enter
0.92% qemu-system-s39 qemu-system-s390x [.] get_desc.isra.11
0.92% qemu-system-s39 qemu-system-s390x [.] qemu_ram_block_from_host
0.87% qemu-system-s39 [kernel.vmlinux] [k] enqueue_entity
0.84% qemu-system-s39 qemu-system-s390x [.] vring_push
0.78% qemu-system-s39 [kernel.vmlinux] [k] set_next_entity
0.73% qemu-system-s39 [kernel.vmlinux] [k] kvm_arch_vcpu_ioctl_run
0.73% qemu-system-s39 [vdso] [.] __kernel_clock_gettime
0.73% qemu-system-s39 qemu-system-s390

Re: [Qemu-devel] [RFC PATCH 00/40] Sneak peek of virtio and dataplane changes for 2.6

[Christian Borntraeger]

On 11/26/2015 10:36 AM, Christian Borntraeger wrote:
> On 11/24/2015 07:00 PM, Paolo Bonzini wrote:
>> This large series is basically all that I would like to get into 2.6.
>> It is a combination of several pieces of work on dataplane and
>> multithreaded block layer.
>>
>> It's also a large part of why I would like someone else to look at
>> miscellaneous patches for a while (in case you've missed that).  I
>> can foresee that following the reviews is going to be a huge time drain.
>>
>> With it I can get ~1300 Kiops on 8 disks (which I achieve with 2 iothreads
>> and 5 VCPUs).  The bulk of the improvement actually comes from the first
>> 8 patches, but the rest of the series is what prepares for what's next
>> to come in QEMU 2.7 and later, such as a multiqueue block layer.
>>
>> It's tedious to review, with some pretty large patches (3, 32, 33, 35).
> On 11/24/2015 07:00 PM, Paolo Bonzini wrote:
>> This large series is basically all that I would like to get into 2.6.
>> It is a combination of several pieces of work on dataplane and
>> multithreaded block layer.
>>
>> It's also a large part of why I would like someone else to look at
>> miscellaneous patches for a while (in case you've missed that).  I
>> can foresee that following the reviews is going to be a huge time drain.
>>
>> With it I can get ~1300 Kiops on 8 disks (which I achieve with 2 iothreads
>> and 5 VCPUs).  The bulk of the improvement actually comes from the first
>> 8 patches, but the rest of the series is what prepares for what's next
>> to come in QEMU 2.7 and later, such as a multiqueue block layer.
> 
> For some unknown reason, this seems to be slightly slower than 2.5-rc1 on my
> old z196. (have not net tested the z13)
> 
> your branch is certainly better regarding malloc, but worse regarding others.

Using the first 8 patches or so (commit be2f6b163e2b2a604f52a258fd932142c5974ffe
vring: slim down allocation of VirtQueueElements)
is slightly faster than 2.5.0-rc1, so the regression seems to come from some
of the later patches.

Re: [Qemu-devel] [PULL 0/2] Migration pull request

[Peter Maydell]

On 25 November 2015 at 14:32, Juan Quintela  wrote:
> Hi
>
> This series:
> - Ignore madvise return value (david)
>
>   As there is no way to diferentiate an error because the kernel don't
>   understand HUGE_PAGES and anything else
>
> - Limit memory usage for block migraiton (wen)
>
> Please, aply.
>
> Later, Juan.
>
> The following changes since commit e85dda8070b20dd8765d52daf64de70a9ccf395f:
>
>   Merge remote-tracking branch 'remotes/sstabellini/tags/xen-20151125' into 
> staging (2015-11-25 12:09:34 +)
>
> are available in the git repository at:
>
>   git://github.com/juanquintela/qemu.git tags/migration/20151125
>
> for you to fetch changes up to f77dcdbc76dbf9bade9739e85e1013639e535835:
>
>   block-migration: limit the memory usage (2015-11-25 15:27:28 +0100)
>
> 
> migration/next for 20151125
>
> 
> Dr. David Alan Gilbert (1):
>   Assume madvise for (no)hugepage works
>
> Wen Congyang (1):
>   block-migration: limit the memory usage
>
>  migration/block.c|  7 ++-
>  migration/postcopy-ram.c | 10 ++
>  2 files changed, 8 insertions(+), 9 deletions(-)
>



Applied, thanks.

-- PMM

Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25)

[Peter Maydell]

On 25 November 2015 at 17:19, Paolo Bonzini  wrote:
> The following changes since commit 4b6eda626fdb8bf90472c6868d502a2ac09abeeb:
>
>   Merge remote-tracking branch 'remotes/lalrae/tags/mips-20151124' into 
> staging (2015-11-24 17:05:06 +)
>
> are available in the git repository at:
>
>
>   git://github.com/bonzini/qemu.git tags/for-upstream
>
> for you to fetch changes up to 6ac504d20a0967da06caadf595388f753908328a:
>
>   target-i386: kvm: Print warning when clearing mcg_cap bits (2015-11-25 
> 17:48:46 +0100)
>
> 
> Small patches, most notably the introduction of -fwrapv.
>
> 
> Daniel P. Berrange (2):
>   Revert "exec: silence hugetlbfs warning under qtest"
>   exec: remove warning about mempath and hugetlbfs
>
> Eduardo Habkost (3):
>   target-i386: kvm: Abort if MCE bank count is not supported by host
>   target-i386: kvm: Use env->mcg_cap when setting up MCE
>   target-i386: kvm: Print warning when clearing mcg_cap bits
>
> Paolo Bonzini (3):
>   MAINTAINERS: Update TCG CPU cores section
>   QEMU does not care about left shifts of signed negative values
>   target-sparc: fix 32-bit truncation in fpackfix
>
> Wen Congyang (1):
>   call bdrv_drain_all() even if the vm is stopped

I definitely don't think we should apply the -fwrapv patch yet;
would you mind respinning this pullrequest without it?

thanks
-- PMM

[Qemu-devel] [PATCH v3 0/3] Update tests/qemu-iotests failing cases for the s390 platform

[Bo Tu]

From: Bo Tu  

v3:
1. Remove patch for test 120 because Fam Zheng upstreamed same fix for 
test 119 and 120
2. Rename 051.out to 051.s390.out, add rule in Makefile to generate 
051.s390-ccw-virtio.out
3. Remove superfluous quotation marks in common.config
4. Add "Acked-by: Max Reitz " for test 068, 
add "Reviewed-by: Max Reitz " for test 051 and common.config

v2:
1. Refine common.config via changing the definition of default_alias_machine
and default_machine
2. Add Reviewed-by of Eric Blake for common.config

v1:
1. Refine common.config
2. Update the output file for test 051 based on its current
output for s390 platform, add a pc specific output file for test 051
3. checkpatch.pl reports invaid UTF-8 error for 051 patch,
because its output files contain some non-text data
4. Add the parameter of "-no-shutdown -machine accel=kvm" for
s390-ccw-virtio for test 068
5. Disable VNC server for test 120

Bo Tu (3):
  qemu-iotests: refine common.config
  qemu-iotests: s390x: fix test 051
  qemu-iotests: s390x: fix test 068

 tests/qemu-iotests/051   |  99 +
 tests/qemu-iotests/051.out   | 422 ---
 tests/qemu-iotests/051.pc.out| 422 +++
 tests/qemu-iotests/051.s390.out  | 335 +++
 tests/qemu-iotests/068   |  14 +-
 tests/qemu-iotests/Makefile  |   7 +-
 tests/qemu-iotests/common.config |   9 +-
 7 files changed, 844 insertions(+), 464 deletions(-)
 delete mode 100644 tests/qemu-iotests/051.out
 create mode 100644 tests/qemu-iotests/051.pc.out
 create mode 100644 tests/qemu-iotests/051.s390.out

-- 
2.4.3

[Qemu-devel] [PATCH v3 3/3] qemu-iotests: s390x: fix test 068

[Bo Tu]

From: Bo Tu  

Now, s390-virtio-ccw is default machine and s390-ccw.img is default boot
loader. If the s390-virtio-ccw machine finds no device to load from and
errors out, then emits a panic and exits the vm. This breaks test cases
068 for s390x.
Adding the parameter of "-no-shutdown" for s390-ccw-virtio will pause VM
before shutdown.

Acked-by: Max Reitz 
Reviewed-by: Sascha Silbe 
Signed-off-by: Bo Tu 
---
 tests/qemu-iotests/068 | 14 --
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/068 b/tests/qemu-iotests/068
index b72e555..58d1d80 100755
--- a/tests/qemu-iotests/068
+++ b/tests/qemu-iotests/068
@@ -50,13 +50,23 @@ echo
 echo "=== Saving and reloading a VM state to/from a qcow2 image ==="
 echo
 _make_test_img $IMG_SIZE
+
+case "$QEMU_DEFAULT_MACHINE" in
+  s390-ccw-virtio)
+  platform_parm="-no-shutdown -machine accel=kvm"
+  ;;
+  *)
+  platform_parm=""
+  ;;
+esac
+
 # Give qemu some time to boot before saving the VM state
 bash -c 'sleep 1; echo -e "savevm 0\nquit"' |\
-$QEMU -nographic -monitor stdio -serial none -hda "$TEST_IMG" |\
+$QEMU $platform_parm -nographic -monitor stdio -serial none -hda 
"$TEST_IMG" |\
 _filter_qemu
 # Now try to continue from that VM state (this should just work)
 echo quit |\
-$QEMU -nographic -monitor stdio -serial none -hda "$TEST_IMG" -loadvm 0 |\
+$QEMU $platform_parm -nographic -monitor stdio -serial none -hda 
"$TEST_IMG" -loadvm 0 |\
 _filter_qemu
 
 # success, all done
-- 
2.4.3

[Qemu-devel] [PATCH v3 1/3] qemu-iotests: refine common.config

[Bo Tu]

From: Bo Tu  

Replacing awk with sed, then it's easier to read.
Replacing "[ ! -z "$default_alias_machine" ]" with
"[[ $default_alias_machine ]]", then it's slightly shorter.

Reviewed-by: Max Reitz 
Suggested-By: Sascha Silbe 
Reviewed-by: Sascha Silbe 
Reviewed-by: Eric Blake   
Signed-off-by: Bo Tu 
---
 tests/qemu-iotests/common.config | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/common.config b/tests/qemu-iotests/common.config
index 3ed51b8..60bfabf 100644
--- a/tests/qemu-iotests/common.config
+++ b/tests/qemu-iotests/common.config
@@ -154,11 +154,10 @@ export QEMU_IMG=_qemu_img_wrapper
 export QEMU_IO=_qemu_io_wrapper
 export QEMU_NBD=_qemu_nbd_wrapper
 
-default_machine=$($QEMU -machine \? | awk '/(default)/{print $1}')
-default_alias_machine=$($QEMU -machine \? |\
-awk -v var_default_machine="$default_machine"\)\
-'{if ($(NF-2)=="(alias"&&$(NF-1)=="of"&&$(NF)==var_default_machine){print 
$1}}')
-if [ ! -z "$default_alias_machine" ]; then
+default_machine=$($QEMU -machine help | sed -n '/(default)/ s/ .*//p')
+default_alias_machine=$($QEMU -machine help | \
+   sed -n "/(alias of $default_machine)/ { s/ .*//p; q; }")
+if [[ "$default_alias_machine" ]]; then
 default_machine="$default_alias_machine"
 fi
 
-- 
2.4.3

[Qemu-devel] [PULL 1/1] vnc: fix segfault

[Gerd Hoffmann]

Commit "c7628bf vnc: only alloc server surface with clients connected"
missed one rarely used codepath (cirrus with guest drivers using 2d
accel) where we have to check for the server surface being present,
to avoid qemu crashing with a NULL pointer dereference.  Add the check.

Reported-by: Anthony PERARD 
Tested-by: Anthony PERARD 
Signed-off-by: Gerd Hoffmann 
---
 ui/vnc.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/ui/vnc.c b/ui/vnc.c
index c9f2fed..7538405 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -931,6 +931,11 @@ static void vnc_dpy_copy(DisplayChangeListener *dcl,
 int i, x, y, pitch, inc, w_lim, s;
 int cmp_bytes;
 
+if (!vd->server) {
+/* no client connected */
+return;
+}
+
 vnc_refresh_server_surface(vd);
 QTAILQ_FOREACH_SAFE(vs, &vd->clients, next, vn) {
 if (vnc_has_feature(vs, VNC_FEATURE_COPYRECT)) {
-- 
1.8.3.1

[Qemu-devel] [PULL for-2.5 0/1] vnc: fix segfault

[Gerd Hoffmann]

  Hi,

Here is the vnc patch queue with a single fix for 2.5

please pull,
  Gerd

The following changes since commit 4b6eda626fdb8bf90472c6868d502a2ac09abeeb:

  Merge remote-tracking branch 'remotes/lalrae/tags/mips-20151124' into staging 
(2015-11-24 17:05:06 +)

are available in the git repository at:


  git://git.kraxel.org/qemu tags/pull-vnc-20151126-1

for you to fetch changes up to 7fe4a41c262e2529dc79f77f6fe63c5309fa2fd9:

  vnc: fix segfault (2015-11-26 08:32:11 +0100)


vnc: fix segfault


Gerd Hoffmann (1):
  vnc: fix segfault

 ui/vnc.c | 5 +
 1 file changed, 5 insertions(+)

Re: [Qemu-devel] [PATCH v2 1/1] parallels: add format spec

[Vladimir Sementsov-Ogievskiy]

On 26.11.2015 11:17, Stefan Hajnoczi wrote:

On Mon, Nov 23, 2015 at 04:32:37PM +0300, Denis V. Lunev wrote:

From: Vladimir Sementsov-Ogievskiy 

This specifies Parallels image format as implemented in Parallels Cloud
Server 6.10

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Denis V. Lunev 
CC: Eric Blake 
CC: John Snow 
CC: Stefan Hajnoczi 
---
v2: add license info
 switch to offsets from types in field descriptions

Cool!  Thanks for publishing a specification.  This will help everyone
understand the code.


+=== Dirty bitmaps feature ===
+
+This feature provides a way of storing dirty bitmaps in the image. The fields
+of its data area are:
+
+   0 -  7:size
+  The bitmap size, should be equal to disk size in sectors.
+
+   8 - 23:id
+  An identifier for backup consistency checking.
+
+  24 - 27:granularity
+  Bitmap granularity, in sectors. I.e., the number of sectors
+  corresponding to one bit of the bitmap.

Does this need to be a power of 2?


Good point




+  28 - 31:l1_size
+  The number of entries in the L1 table of the bitmap.
+
+  variable:   l1 (64 * l1_size bytes)
+  L1 offset table (in bytes)
+
+A dirty bitmap is stored using a one-level structure for the mapping to host
+clusters - an L1 table.
+
+Given an offset into the bitmap, the offset in bytes into the image file can be

What are the units of the offset into the bitmap?  Is it a bit number
(i.e. sector number / granularity)?


No, here bitmap is considered as raw data, stored using L1, so offset is 
in bytes. Bytes of the bitmap itself, as raw binary data.


--
Best regards,
Vladimir
* now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience.

Re: [Qemu-devel] [PATCH v2 2/4] qemu-iotests: s390x: fix test 051

[tu bo]

Hi Max:

On 11/25/2015 11:41 PM, Max Reitz wrote:

On 24.11.2015 22:17, Sascha Silbe wrote:

This PC/s390x-only hunk looks like an oversight to me.


Not really, see
http://lists.nongnu.org/archive/html/qemu-devel/2015-02/msg01906.html
and
http://lists.nongnu.org/archive/html/qemu-devel/2015-04/msg02851.html

I noticed, but I am fine with it since the tests probably won't run on
anything but x86/pc and s390 anyway (without modifications; most of the
changes this series is making to make the iotests work on s390 are
necessary for other non-pc platforms as well, and that shows to me that
apparently nobody tried to run the iotests on non-pc platforms before
s390, or didn't care enough about them to fix them).


We should make
one of the options the default. I'd prefer defaulting to virtio (see
below), but since the test previously hard-coded IDE that would be fine,
too.


In my first reply above, I noted that virtio0 may not be available on
all platforms either. Therefore, I'd rather have an explicit list of
platforms there than an asterisk where it does not belong.

However, my second reply above spawned a bit of a discussion, where
Kevin simply proposed to change the ID of the drive to something known,
i.e. just set the ID by adding an id=drive0 or something to the -drive
parameter.

Thanks for reminding me of the above, I had already forgotten. Indeed,
we should just add id=drive0 to the -drive parameter and use drive0. A
similar solution may be possible in most other places as well where PC
and s390 differ due to the names of the default devices available.


thanks for the reminder :-)

Yes, Kevin mentioned that we can use "id=testdisk" because it's the same 
on all platforms. Please refer this link:

http://lists.nongnu.org/archive/html/qemu-devel/2015-04/msg03715.html

For test 130, I used "qemu -drive id=testdisk" for both pc and s390x.
For test 051, I didn't find a way to do the same thing for qemu-io.

Re: [Qemu-devel] [PULL 0/4] Ide patches

[Peter Maydell]

On 25 November 2015 at 20:25, John Snow  wrote:
> The following changes since commit 4b6eda626fdb8bf90472c6868d502a2ac09abeeb:
>
>   Merge remote-tracking branch 'remotes/lalrae/tags/mips-20151124' into 
> staging (2015-11-24 17:05:06 +)
>
> are available in the git repository at:
>
>   https://github.com/jnsnow/qemu.git tags/ide-pull-request
>
> for you to fetch changes up to 9c73517ca56d6611371376bd298b4b20f3ad6140:
>
>   ide-test: fix timeouts (2015-11-25 11:37:34 -0500)
>
> 
>
> 
>
> Alberto Garcia (2):
>   atapi: Account for failed and invalid operations in cd_read_sector()
>   atapi: Fix code indentation
>
> John Snow (2):
>   ide-test: cdrom_pio_impl fixup
>   ide-test: fix timeouts
>
>  hw/ide/atapi.c   |  8 +---
>  tests/ide-test.c | 32 +++-
>  2 files changed, 28 insertions(+), 12 deletions(-)

Applied, thanks.

-- PMM

Re: [Qemu-devel] [PATCH RFC 00/14] vhost-user: shutdown and reconnection

[Michael S. Tsirkin]

On Wed, Sep 09, 2015 at 01:09:52AM +0200, marcandre.lur...@redhat.com wrote:
> From: Marc-André Lureau 
> 
> In a previous series "Add feature to start QEMU without vhost-user
> backend", Tetsuya Mukawa proposed to allow the vhost-user backend to
> disconnect and reconnect. However, Michael Tsirkin pointed out that
> you can't do that without extra care, because the guest and hypervisor
> don't know the slave ring manipulation state, there might be pending
> replies for example that could be lost, and suggested to reset the
> guest queues, but this requires kernel changes, and it may have to
> clear the ring and lose queued packets.
> 
> The following series starts from the idea that the slave can request a
> "managed" shutdown instead and later recover (I guess the use case for
> this is to allow for example to update static dispatching/filter rules
> etc)

I'm still not sure users actually need this.  I am inclined to think we
should teach guests to respond to NEED_RESET status. Then to handle
disconnect, we would
- deactivate the disconnected backend
- stop VM, and wait for a reconnect
- set NEED_RESET status, and re-activate the backend
  after guest reset

-- 
MST

Re: [Qemu-devel] [RFC PATCH 00/40] Sneak peek of virtio and dataplane changes for 2.6

[Paolo Bonzini]

On 26/11/2015 10:36, Christian Borntraeger wrote:
> For some unknown reason, this seems to be slightly slower than 2.5-rc1 on my
> old z196. (have not net tested the z13)
> 
> your branch is certainly better regarding malloc, but worse regarding others.

Thanks for taking the time to test this!

This is correct, see the cover letter:

"[Patches 14 to 16 remove] the duplicate dataplane-specific
implementation of virtio in favor of the regular one that is already
used for non-dataplane. While the dataplane implementation is slightly
more optimized, I chose to keep the other one to avoid another "touch
all virtio devices" series.

Patch 10 alone mostly brings performance in par between the two.
The remaining 7-8% can be recovered by mostly getting rid of tiny
address_space_* operations, keeping the rings always mapped. Note that
the rest of this big series does bring a little performance improvement,
and already makes up for the lost performance."

The profile shows that the culprit is the repeated access
to the virtio ring:

3.99% qemu-system-s39 libc-2.18.so [.] __memcpy_z196
2.66% qemu-system-s39 qemu-system-s390x [.] address_space_lduw_le
2.51% qemu-system-s39 qemu-system-s390x [.] address_space_map
2.51% qemu-system-s39 qemu-system-s390x [.] phys_page_find
2.24% qemu-system-s39 qemu-system-s390x [.] qemu_get_ram_ptr
2.18% qemu-system-s39 qemu-system-s390x [.] address_space_translate_internal
1.91% qemu-system-s39 qemu-system-s390x [.] qemu_coroutine_switch
1.66% qemu-system-s39 qemu-system-s390x [.] address_space_rw
1.63% qemu-system-s39 qemu-system-s390x [.] address_space_stw_le
1.57% qemu-system-s39 qemu-system-s390x [.] address_space_stl_le
1.57% qemu-system-s39 qemu-system-s390x [.] address_space_translate
1.45% qemu-system-s39 qemu-system-s390x [.] virtqueue_pop
0.91% qemu-system-s39 qemu-system-s390x [.] qemu_ram_block_from_host
0.79% qemu-system-s39 qemu-system-s390x [.] vring_desc_read
0.76% qemu-system-s39 qemu-system-s390x [.] qemu_get_ram_block
---
28.33%

3.30% qemu-system-s39 libc-2.18.so [.] __memcpy_z196
2.83% qemu-system-s39 qemu-system-s390x [.] memory_region_find_rcu
2.72% qemu-system-s39 qemu-system-s390x [.] vring_pop
1.37% qemu-system-s39 qemu-system-s390x [.] address_space_rw
1.37% qemu-system-s39 qemu-system-s390x [.] qemu_get_ram_ptr
1.18% qemu-system-s39 qemu-system-s390x [.] memory_region_find
0.92% qemu-system-s39 qemu-system-s390x [.] get_desc.isra.11
0.92% qemu-system-s39 qemu-system-s390x [.] qemu_ram_block_from_host
0.84% qemu-system-s39 qemu-system-s390x [.] vring_push
---
15.45%

I would really prefer to get rid of vring.c as soon as the infrastructure
makes it possible---even if it's faster. We know what makes virtio.c
slower, and it's simpler to fix virtio.c than to convert all the other
models to vring.c _plus_ make vring.c safe for migration.

Paolo

Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25)

[Paolo Bonzini]

On 26/11/2015 10:46, Peter Maydell wrote:
> On 25 November 2015 at 17:19, Paolo Bonzini  wrote:
>> The following changes since commit 4b6eda626fdb8bf90472c6868d502a2ac09abeeb:
>>
>>   Merge remote-tracking branch 'remotes/lalrae/tags/mips-20151124' into 
>> staging (2015-11-24 17:05:06 +)
>>
>> are available in the git repository at:
>>
>>
>>   git://github.com/bonzini/qemu.git tags/for-upstream
>>
>> for you to fetch changes up to 6ac504d20a0967da06caadf595388f753908328a:
>>
>>   target-i386: kvm: Print warning when clearing mcg_cap bits (2015-11-25 
>> 17:48:46 +0100)
>>
>> 
>> Small patches, most notably the introduction of -fwrapv.
>>
>> 
>> Daniel P. Berrange (2):
>>   Revert "exec: silence hugetlbfs warning under qtest"
>>   exec: remove warning about mempath and hugetlbfs
>>
>> Eduardo Habkost (3):
>>   target-i386: kvm: Abort if MCE bank count is not supported by host
>>   target-i386: kvm: Use env->mcg_cap when setting up MCE
>>   target-i386: kvm: Print warning when clearing mcg_cap bits
>>
>> Paolo Bonzini (3):
>>   MAINTAINERS: Update TCG CPU cores section
>>   QEMU does not care about left shifts of signed negative values
>>   target-sparc: fix 32-bit truncation in fpackfix
>>
>> Wen Congyang (1):
>>   call bdrv_drain_all() even if the vm is stopped
> 
> I definitely don't think we should apply the -fwrapv patch yet;
> would you mind respinning this pullrequest without it?

In what way does that patch make that thing worse?

Paolo

Re: [Qemu-devel] [PATCH v2 1/2] utils: Add warning messages

[Markus Armbruster]

Lluís Vilanova  writes:

> Adds a special error object that transforms error messages into
> immediately reported warnings.

Before I dive into details: my fundamental objection is that &error_warn
is new infrastructure without a use.  The new "this is how you should do
warnings" paragraph in error.h's big comment does not count as use :)

Without actual use, we can't be sure it's useful.  And being useful
should be mandatory for new infrastructure, even when it's as small as
this one.

Finally, note that

err = NULL;
foo(arg, &err);
if (err) {
error_report("warning: %s", error_get_pretty(err));
error_free(err);
}

and

foo(arg, &error_warn)

are subtly different.

One, the former throws away the hint.  That may or may not be
appropriate.  It also happens in other places that amend an error's
message.  Perhaps we could use a function to amend an error's message.
To find out, we'd have to track down the places that do that now.

Two, the latter doesn't let the caller see whether foo() warned.  I
believe this makes it unsuitable for some cases, or in other words, it's
insufficiently general.

Three, the latter can't catch misuse the former catches, namely multiple
error_set().  The latter happily reports all of them, the former fails
error_setv()'s assertion for the second one.

If warnings are common enough to justify infrastructure, then I figure a
convenience function to report an Error as a warning similar to
error_report_err() would be more general and less prone to misuse.

> Signed-off-by: Lluís Vilanova 
> ---
>  include/qapi/error.h |   20 
>  util/error.c |   37 +++--
>  2 files changed, 47 insertions(+), 10 deletions(-)
>
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index 4d42cdc..9b7600c 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -57,6 +57,9 @@
>   * Call a function treating errors as fatal:
>   * foo(arg, &error_fatal);
>   *
> + * Call a function immediately showing all errors as warnings:
> + * foo(arg, &error_warn);
> + *
>   * Receive an error and pass it on to the caller:
>   * Error *err = NULL;
>   * foo(arg, &err);
> @@ -108,6 +111,7 @@ ErrorClass error_get_class(const Error *err);
>   * then.
>   * If @errp is &error_abort, print a suitable message and abort().
>   * If @errp is &error_fatal, print a suitable message and exit(1).
> + * If @errp is &error_warn, print a suitable message.
>   * If @errp is anything else, *@errp must be NULL.
>   * The new error's class is ERROR_CLASS_GENERIC_ERROR, and its
>   * human-readable error message is made from printf-style @fmt, ...
> @@ -158,6 +162,7 @@ void error_setg_win32_internal(Error **errp,
>   * abort().
>   * Else, if @dst_errp is &error_fatal, print a suitable message and
>   * exit(1).
> + * Else, if @dst_errp is &error_warn, print a suitable message.
>   * Else, if @dst_errp already contains an error, ignore this one: free
>   * the error object.
>   * Else, move the error object from @local_err to *@dst_errp.
> @@ -218,12 +223,27 @@ void error_set_internal(Error **errp,
>  
>  /*
>   * Pass to error_setg() & friends to abort() on error.
> + *
> + * WARNING: Do _not_ use for errors that are (or can be) triggered by guest 
> code
> + *  (e.g., some unimplimented corner case in guest code translation 
> or
> + *  device code). Otherwise that can be abused by guest code to
> + *  terminate QEMU.
>   */
>  extern Error *error_abort;
>  
>  /*
>   * Pass to error_setg() & friends to exit(1) on error.
> + *
> + * WARNING: Do _not_ use for errors that are (or can be) triggered by guest 
> code
> + *  (e.g., some unimplimented corner case in guest code translation 
> or
> + *  device code). Otherwise that can be abused by guest code to
> + *  terminate QEMU.
>   */
>  extern Error *error_fatal;

This hunk isn't covered by the commit message.

Similar admonitions elsewhere in this file are less ornately decorated.
Plain

 * Do *not* use for errors that are (or can be) triggered by guest code
 * (e.g., some unimplemented corner case in guest code translation or
 * device code). Otherwise that can be abused by guest code to terminate
 * QEMU.

would do, and avoid the long lines.

Except this wording is too narrow.  There are many more places where
"terminate on error" is wrong, such as monitor commands.

&error_exit is okay exactly when and where exit(1) is okay: on
configuration error during startup, and on certain unrecoverable errors
where it's unsafe to continue.

Likewise, &error_abort is okay exactly when and where abort() is okay:
mostly on programming errors.  Possibly also on "unexpected"
unrecoverable errors where it's unsafe to continue; we did that for most
memory allocation failures at some time, not sure we still do.

Perhaps we need to advise on proper use of abort() and exit(1), but I
doubt error.h is the right place to do it.

P

Re: [Qemu-devel] [PATCH v7 12/24] virtio-blk: Functions for op blocker management

[Kevin Wolf]

Am 26.11.2015 um 08:48 hat Stefan Hajnoczi geschrieben:
> On Wed, Nov 25, 2015 at 05:26:02PM +0100, Max Reitz wrote:
> > On 25.11.2015 17:18, Kevin Wolf wrote:
> > > Am 25.11.2015 um 17:03 hat Max Reitz geschrieben:
> > >> On 25.11.2015 16:57, Kevin Wolf wrote:
> > >>> Am 09.11.2015 um 23:39 hat Max Reitz geschrieben:
> > >>> This makes me wonder: What do we even block here any more? If I didn't
> > >>> miss anything, it's only BLOCK_OP_TYPE_BACKUP_TARGET, and I'm not sure
> > >>> why this needs to be blocked, or if we simply forgot to enable it.
> > >>
> > >> Well, even though in practice this wall of code doesn't make much sense,
> > >> of course it will be safe for potential additions of new op blockers.
> > >>
> > >> And of course we actually don't want these blockers at all anymore...
> > > 
> > > Yes, but dataplane shouldn't really be special enough any more that we
> > > want to disable features for it initially. By now it sounds more like an
> > > easy way to forget unblocking a new feature even though it would work.
> > > 
> > > So perhaps we should really just remove the blockers from dataplane.
> > > Then we don't have to answer the question above...
> > 
> > Well, maybe. I guess this is up to Stefan.
> 
> At this point blockdev.c and block jobs acquire/release AioContext,
> hence all these op blockers are being unblocked.  I think we can switch
> from whitelisting (unblocking) nearly everything to blacklisting
> (blocking) only things that aren't supported yet.

As I said, there is only one operation that isn't whitelisted today,
BLOCK_OP_TYPE_BACKUP_TARGET, and I would be surprised if it weren't just
a bug that it's not whitelisted yet.

Kevin


pgpcr_nzOA90f.pgp
Description: PGP signature

Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25)

[Peter Maydell]

On 26 November 2015 at 10:40, Paolo Bonzini  wrote:
>
>
> On 26/11/2015 10:46, Peter Maydell wrote:
>> I definitely don't think we should apply the -fwrapv patch yet;
>> would you mind respinning this pullrequest without it?
>
> In what way does that patch make that thing worse?

It makes a claim about the semantics that the compiler
guarantees us which isn't currently valid. (Or
alternatively, it's implicitly claiming that clang isn't
a supported compiler, which isn't true.) I don't think
we should document or rely on signed-shift semantics
until we have the relevant documented promises from the
compiler developers that that is what they are providing.
(I'm happy that the gcc folks have provided those promises, they
just need to actually document them in the -fwrapv option
docs. The clang folks haven't replied yet so we don't know.)

thanks
-- PMM

Re: [Qemu-devel] [RESEND RFC 2/6] device_tree: introduce load_device_tree_from_sysfs

[Thomas Huth]

On 19/11/15 16:22, Eric Auger wrote:
> This function returns the host device tree blob from sysfs
> (/sys/firmware/devicetree/base).
> 
> This has a runtime dependency on the dtc binary. This functionality
> is useful for platform device passthrough where the host device tree
> needs to be parsed to feed information into the guest device tree.
> 
> Signed-off-by: Eric Auger 
> ---
>  device_tree.c| 40 
>  include/sysemu/device_tree.h |  1 +
>  2 files changed, 41 insertions(+)
> 
> diff --git a/device_tree.c b/device_tree.c
> index a9f5f8e..58a5329 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -117,6 +117,46 @@ fail:
>  return NULL;
>  }
>  
> +/**
> + * load_device_tree_from_sysfs
> + *
> + * extract the dt blob from host sysfs
> + * this has a runtime dependency on the dtc binary
> + */
> +void *load_device_tree_from_sysfs(void)
> +{
> +char cmd[] = "dtc -I fs -O dtb /sys/firmware/devicetree/base";
> +FILE *pipe;
> +void *fdt;
> +int ret, actual_dt_size;
> +
> +pipe = popen(cmd, "r");
> +if (!pipe) {
> +error_report("%s: Error when executing dtc", __func__);
> +return NULL;
> +}

The Device Tree Compiler binary is normally only installed on
developer's machines, so I somewhat doubt that it is a good idea to rely
on the availability of that binary in QEMU? Maybe you should rather
extend libfdt to support such a feature?

 Thomas

Re: [Qemu-devel] [PULL v2 for-2.5 0/6] qemu-ga patch queue for 2.5

[Peter Maydell]

On 26 November 2015 at 00:01, Michael Roth  wrote:
> The following changes since commit 1a4dab849d5d06191ab5e5850f6b8bfcad8ceb47:
>
>   Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into staging 
> (2015-11-25 14:47:06 +)
>
> are available in the git repository at:
>
>
>   git://github.com/mdroth/qemu.git tags/qga-pull-2015-11-25-v2-tag
>
> for you to fetch changes up to 44c6e00c3fd85b9c496bd3e74108ace126813a59:
>
>   qga: added another non-interactive gspawn() helper file. (2015-11-25 
> 17:56:45 -0600)
>
> 
> qemu-ga patch queue for 2.5
>
> * include additional w32 MSI install components needed for
>   guest-exec
> * fix 'make install' when compiling with --disable-tools
> * fix potential data corruption/loss when accessing files
>   bi-directionally via guest-file-{read,write}
> * explicitly document how integer args for guest-file-seek map to
>   SEEK_SET/SEEK_CUR/etc to avoid platform-specific differences
>
> v2:
> * fixed missing SoB
>
> 
> Eric Blake (1):
>   qga: Better mapping of SEEK_* in guest-file-seek
>
> Marc-André Lureau (2):
>   qga: flush explicitly when needed
>   tests: add file-write-read test
>
> Michael Roth (1):
>   makefile: fix qemu-ga make install for --disable-tools
>
> Yuri Pudgorodskiy (2):
>   qga: gspawn() console helper to Windows guest agent msi build
>   qga: added another non-interactive gspawn() helper file.
>

Applied, thanks.

-- PMM

Re: [Qemu-devel] [Qemu-ppc] [PATCH qemu] spapr: Add /system-id

[Greg Kurz]

On Thu, 26 Nov 2015 15:29:07 +1100
Alexey Kardashevskiy  wrote:

> On 11/26/2015 11:49 AM, David Gibson wrote:
> > On Wed, Nov 25, 2015 at 04:15:01PM +0100, Alexander Graf wrote:
> >>
> >>
> >> On 18.11.15 11:49, David Gibson wrote:
> >>> On Wed, Nov 18, 2015 at 06:45:39PM +1100, Alexey Kardashevskiy wrote:
>  On 11/09/2015 07:47 PM, David Gibson wrote:
> > On Mon, Nov 09, 2015 at 05:47:17PM +1100, Alexey Kardashevskiy wrote:
> >> Section B.6.2.1 Root Node Properties of PAPR specification defines
> >> a set of properties which shall be present in the device tree root,
> >> one of these properties is "system-id" which "should be unique across
> >> all systems and all manufacturers". Since UUID is meant to be unique,
> >> it makes sense to use it as "system-id".
> >>
> >> This adds "system-id" property to the device tree root when not empty.
> >>
> >> Signed-off-by: Alexey Kardashevskiy 
> >> ---
> >>
> >> This might be expected by AIX so here is the patch.
> >> I am really not sure if it makes sense to initialize property when
> >> UUID is all zeroes as the requirement is "unique" and zero-uuid is
> >> not.
> >
> > Yeah, I think it would be better to omit system-id entirely when a
> > UUID hasn't been supplied.
> 
> 
>  so this did not go anywhere yet, did it?
> >>>
> >>> No.
> >>
> >> So where is it stuck?
> >
> > I was waiting for a respin which didn't set the property when a UUID
> > hadn't been given.
> >
> 
> This is the original patch:
> 
> 
> +if (qemu_uuid_set) {
> +_FDT((fdt_property_string(fdt, "system-id", buf)));
> +}
> 
> I does not set property if qemu_uuid_set==false already. What did I miss?
> 

It looks like the confusion lies in David's answer to your question
about nil UUID... BTW does it make sense to assign nil UUID to a system ?

--
Greg

[Qemu-devel] [PATCH for-2.5] w32: Use gcc option -mthreads

[Stefan Weil]

QEMU uses threads / coroutines, therefore support for thread local storage
and thread safe libraries (-D_MT) must be enabled by using -mthreads.

Reported-by: David Engraf 
Signed-off-by: Stefan Weil 
---
 configure | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/configure b/configure
index 979bc55..67801b0 100755
--- a/configure
+++ b/configure
@@ -727,6 +727,8 @@ if test "$mingw32" = "yes" ; then
   QEMU_CFLAGS="-DWIN32_LEAN_AND_MEAN -DWINVER=0x501 $QEMU_CFLAGS"
   # enable C99/POSIX format strings (needs mingw32-runtime 3.15 or later)
   QEMU_CFLAGS="-D__USE_MINGW_ANSI_STDIO=1 $QEMU_CFLAGS"
+  # MinGW needs -mthreads for TLS and macro _MT.
+  QEMU_CFLAGS="-mthreads $QEMU_CFLAGS"
   LIBS="-lwinmm -lws2_32 -liphlpapi $LIBS"
   write_c_skeleton;
   if compile_prog "" "-liberty" ; then
-- 
2.1.4

Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25)

[Paolo Bonzini]

On 26/11/2015 11:56, Peter Maydell wrote:
> On 26 November 2015 at 10:40, Paolo Bonzini  wrote:
>>
>>
>> On 26/11/2015 10:46, Peter Maydell wrote:
>>> I definitely don't think we should apply the -fwrapv patch yet;
>>> would you mind respinning this pullrequest without it?
>>
>> In what way does that patch make that thing worse?
> 
> It makes a claim about the semantics that the compiler
> guarantees us which isn't currently valid. (Or
> alternatively, it's implicitly claiming that clang isn't
> a supported compiler, which isn't true.) I don't think
> we should document or rely on signed-shift semantics

But we are relying on them, and thus we should document them.  Witness
the number of patches fixing so called "undefined" behavior.  And those
patches are _dangerous_.

I can certainly remove the "as documented by the GCC manual" part and
the -fwrapv setting, but silencing -Wshift-negative-value and
documenting what we rely on should go in.

Paolo

> until we have the relevant documented promises from the
> compiler developers that that is what they are providing.
> (I'm happy that the gcc folks have provided those promises, they
> just need to actually document them in the -fwrapv option
> docs. The clang folks haven't replied yet so we don't know.)

Re: [Qemu-devel] [PATCH v2] qemu_mutex_iothread_locked not correctly synchronized

[Stefan Weil]

Am 26.11.2015 um 10:12 schrieb David Engraf:
> Am 25.11.2015 um 17:16 schrieb Paolo Bonzini:
>>
>>
>> On 25/11/2015 16:48, David Engraf wrote:
>>>
>>> Indeed, TLS handling is broken. The address of iothread_locked is
>>> always
>>> the same between threads and I can see that a different thread sets
>>> iothread_locked to false, thus my current thread uses an invalid state.
>>> I will have to check why my compiler produces invalid TLS code.
>>
>> That rings a bell, I think there are different CRT libraries or
>> something like that.  Stefan, what would break TLS under Windows?
>
> "--extra-cflags=-mthreads" is the solution. iothread_locked is unique
> for each thread now. I saw that this is already mentioned in the
> Documentation [1], but why isn't that enabled by default if QEMU
> requires this option on Windows? Without this option, even the latest
> version of gcc doesn't produce working TLS code.
>
> Many thanks for your support!
>
> David
>
> [1] http://wiki.qemu.org/Hosts/W32

Hi David,

I just prepared a patch which will enable that option by default
for all compilations targeting Windows.

Thanks for your report!

Regards,
Stefan

Re: [Qemu-devel] [PULL 00/15] vhost, pc: fixes for 2.5

[Peter Maydell]

On 19 November 2015 at 13:35, Michael S. Tsirkin  wrote:
> The following changes since commit 8337c6cbc37c6b2184f41bab3eaff47d5e68012a:
>
>   Update version for v2.5.0-rc0 release (2015-11-13 17:10:36 +)
>
> are available in the git repository at:
>
>   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
>
> for you to fetch changes up to 1c7ba94a184df1eddd589d5400d879568d3e5d08:
>
>   exec: silence hugetlbfs warning under qtest (2015-11-19 15:26:05 +0200)
>
> 
> vhost, pc: fixes for 2.5
>
> Fixes all over the place.
>
> This also re-enables a test we disabled in 2.5 cycle
> now that there's a way not to get a warning from it.
>
> Signed-off-by: Michael S. Tsirkin 

Hi; I've just noticed that since this pull was applied the Travis
builds have been failing:
https://travis-ci.org/qemu/qemu/builds

The log messages are rather odd but suggest a virtio-user problem:

GTESTER check-qtest-i386
blkdebug: Suspended request 'A'
blkdebug: Resuming request 'A'
main-loop: WARNING: I/O thread spun for 1000 iterations
main-loop: WARNING: I/O thread spun for 1000 iterations
main-loop: WARNING: I/O thread spun for 1000 iterations
main-loop: WARNING: I/O thread spun for 1000 iterations
[vmxnet3][WR][vmxnet3_peer_has_vnet_hdr]: Peer has no virtio
extension. Task offloads will be emulated.
GTester: last random seed: R02S8538d05b7c04668517e6c9985845363e
qemu-system-i386: Device '�P1��*' not found
qemu-system-i386: Failed to read msg header. Read 0 instead of 12.
Original request 11.
qemu-system-i386: Failed to read msg header. Read 0 instead of 12.
Original request 11.
qemu-system-i386: Device 'sd0' not found
qemu-system-i386: Failed to read msg header. Read 0 instead of 12.
Original request 11.
qemu-system-i386: Failed to read msg header. Read 0 instead of 12.
Original request 11.
qemu-system-i386: Failed to read msg header. Read 0 instead of 12.
Original request 11.
qemu-system-i386: Failed to read msg header. Read 0 instead of 12.
Original request 11.
make: *** [check-qtest-i386] Error 1

(note the corrupted string in some of the "device not found" messages --
if you look at different build logs this varies from run to run)

Can you look into this, please?

thanks
-- PMM

Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25)

[Peter Maydell]

On 26 November 2015 at 11:23, Paolo Bonzini  wrote:
> On 26/11/2015 11:56, Peter Maydell wrote:
>> It makes a claim about the semantics that the compiler
>> guarantees us which isn't currently valid. (Or
>> alternatively, it's implicitly claiming that clang isn't
>> a supported compiler, which isn't true.) I don't think
>> we should document or rely on signed-shift semantics
>
> But we are relying on them, and thus we should document them.  Witness
> the number of patches fixing so called "undefined" behavior.  And those
> patches are _dangerous_.

Until and unless the compiler guarantees us the semantics that
we want, then silently hoping we get something we're not getting
is even more dangerous, and avoiding the UB is better than
just crossing our fingers and hoping.

> I can certainly remove the "as documented by the GCC manual" part and
> the -fwrapv setting, but silencing -Wshift-negative-value and
> documenting what we rely on should go in.

I don't see much point in documenting what we rely on
if we can't rely on it and need to stop relying on it.

thanks
-- PMM

Re: [Qemu-devel] [PULL for-2.5 0/1] vnc: fix segfault

[Peter Maydell]

On 26 November 2015 at 09:59, Gerd Hoffmann  wrote:
>   Hi,
>
> Here is the vnc patch queue with a single fix for 2.5
>
> please pull,
>   Gerd
>
> The following changes since commit 4b6eda626fdb8bf90472c6868d502a2ac09abeeb:
>
>   Merge remote-tracking branch 'remotes/lalrae/tags/mips-20151124' into 
> staging (2015-11-24 17:05:06 +)
>
> are available in the git repository at:
>
>
>   git://git.kraxel.org/qemu tags/pull-vnc-20151126-1
>
> for you to fetch changes up to 7fe4a41c262e2529dc79f77f6fe63c5309fa2fd9:
>
>   vnc: fix segfault (2015-11-26 08:32:11 +0100)
>
> 
> vnc: fix segfault
>
> 
> Gerd Hoffmann (1):
>   vnc: fix segfault
>
>  ui/vnc.c | 5 +
>  1 file changed, 5 insertions(+)
>

Applied, thanks.

-- PMM

Re: [Qemu-devel] [PATCH v2] ui/cocoa.m: Prevent activation clicks from going to guest

[Peter Maydell]

On 26 November 2015 at 01:14, Programmingkid  wrote:
> When QEMU is brought to the foreground, the click event that activates QEMU
> should not go to the guest. Accidents happen when they do go to the guest
> without giving the user a change to handle them. Buttons are clicked 
> accidently.
> Windows are closed accidently. Volumes are unmounted accidently. This patch
> prevents these accidents from happening.
>
> Signed-off-by: John Arbuckle 
>
> ---
> Added code that handles the right mouse button and the other mouse button.

This seems like a fair bit of repeated code. Does the change
below do the right thing? I think it ought to work but I don't have
any guests handy which use the mouse to check with.

diff --git a/ui/cocoa.m b/ui/cocoa.m
index 1554331..d76b942 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -724,7 +724,15 @@ QemuCocoaView *cocoaView;
 }

 if (mouse_event) {
-if (last_buttons != buttons) {
+/* Don't send button events to the guest unless we've got a
+ * mouse grab or window focus. If we have neither then this event
+ * is the user clicking on the background window to activate and
+ * bring us to the front, which will be done by the sendEvent
+ * call below. We definitely don't want to pass that click through
+ * to the guest.
+ */
+if ((isMouseGrabbed || [[self window] isKeyWindow]) &&
+(last_buttons != buttons)) {
 static uint32_t bmap[INPUT_BUTTON_MAX] = {
 [INPUT_BUTTON_LEFT]   = MOUSE_EVENT_LBUTTON,
 [INPUT_BUTTON_MIDDLE] = MOUSE_EVENT_MBUTTON,



(if this is the activation click then we will do the mousegrab
on mouse-button-up so it's not necessary to do it on button-down,
I think.)

thanks
-- PMM

[Qemu-devel] [PATCH] Fix memory leak on error

[董兴水]

hw/ppc/spapr.c: Fix memory leak on error, it was introduced in bc09e0611
hw/acpi/memory_hotplug.c: Fix memory leak on error, it was introduced in 
34f2af3d

Signed-off-by: Stefano Dong (董兴水) 
---
 hw/acpi/memory_hotplug.c | 1 +
 hw/ppc/spapr.c   | 1 +
 2 files changed, 2 insertions(+)

diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index ce428df..e4b9a01 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -155,6 +155,7 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr 
addr, uint64_t data,
 qapi_event_send_mem_unplug_error(dev->id,
  error_get_pretty(local_err),
  &error_abort);
+error_free(local_err);
 break;
 }
 trace_mhp_acpi_pc_dimm_deleted(mem_st->selector);
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 030ee35..3bb8bcd 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -125,6 +125,7 @@ static XICSState *xics_system_init(MachineState *machine,
 error_report("kernel_irqchip requested but unavailable: %s",
  error_get_pretty(err));
 }
+error_free(err);
 }
 
 if (!icp) {
-- 
2.5.1.windows.1

Re: [Qemu-devel] [RESEND RFC 3/6] device_tree: introduce qemu_fdt_node_path

[Alex Bennée]

Eric Auger  writes:

> This new helper routine returns the node path of a device
> referred to by its name and compat string.
>
> Signed-off-by: Eric Auger 
> ---
>  device_tree.c| 40 
>  include/sysemu/device_tree.h |  3 +++
>  2 files changed, 43 insertions(+)
>
> diff --git a/device_tree.c b/device_tree.c
> index 58a5329..f184e3c 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -171,6 +171,46 @@ static int findnode_nofail(void *fdt, const char 
> *node_path)
>  return offset;
>  }
>
> +/**
> + * qemu_fdt_node_path
> + *
> + * return the node path of a device, referred to by its node name
> + * and its compat string
> + * fdt: pointer to the dt blob
> + * name: name of the device
> + * compat: compatibility string of the device
> + *
> + * returns the node path
> + */
> +int qemu_fdt_node_path(void *fdt, const char *name, char *compat,
> +   char **node_path)
> +{
> +int offset = 0, len;
> +const char *iter_name;
> +char path[256];
> +int ret;
> +
> +*node_path = NULL;
> +while (1) {
> +offset = fdt_node_offset_by_compatible(fdt, offset, compat);
> +if (offset == -FDT_ERR_NOTFOUND) {

Is this not the only error code fdt_node_offset_by_compatible() won't
return?

> +break;
> +}
> +iter_name = fdt_get_name(fdt, offset, &len);
> +if (!strncmp(iter_name, name, len)) {

is it possible for fdt_get_name to fail here and give you NULL and -len?

> +goto found;
> +}
> +}
> +return offset;
> +
> +found:
> +ret = fdt_get_path(fdt, offset, path, 256);
> +if (!ret) {
> +*node_path = g_strdup(path);
> +}
> +return ret;
> +}
> +
>  int qemu_fdt_setprop(void *fdt, const char *node_path,
>   const char *property, const void *val, int size)
>  {
> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
> index 307e53d..f9e6e6e 100644
> --- a/include/sysemu/device_tree.h
> +++ b/include/sysemu/device_tree.h
> @@ -18,6 +18,9 @@ void *create_device_tree(int *sizep);
>  void *load_device_tree(const char *filename_path, int *sizep);
>  void *load_device_tree_from_sysfs(void);
>
> +int qemu_fdt_node_path(void *fdt, const char *name, char *compat,
> +   char **node_path);
> +
>  int qemu_fdt_setprop(void *fdt, const char *node_path,
>   const char *property, const void *val, int size);
>  int qemu_fdt_setprop_cell(void *fdt, const char *node_path,


--
Alex Bennée

Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25)

[Markus Armbruster]

Peter Maydell  writes:

> On 26 November 2015 at 11:23, Paolo Bonzini  wrote:
>> On 26/11/2015 11:56, Peter Maydell wrote:
>>> It makes a claim about the semantics that the compiler
>>> guarantees us which isn't currently valid. (Or
>>> alternatively, it's implicitly claiming that clang isn't
>>> a supported compiler, which isn't true.) I don't think
>>> we should document or rely on signed-shift semantics
>>
>> But we are relying on them, and thus we should document them.  Witness
>> the number of patches fixing so called "undefined" behavior.  And those
>> patches are _dangerous_.

I'm pretty sure us screwing some of them up is a much larger risk than
gcc suddenly starting to screw up our signed shifts without anybody
noticing.

If gcc ever started to mess up signed shifts, I'd fully expect the
kernel and many applications to go down in flames.  Hopefully quickly
enough to avoid real damage.  I don't think gcc has become *that*
reckless.  Clang neither.

> Until and unless the compiler guarantees us the semantics that
> we want, then silently hoping we get something we're not getting
> is even more dangerous, and avoiding the UB is better than
> just crossing our fingers and hoping.

The cost and risk of proactively fixing a hypothetical^Wlatent problem
needs to be weighed against the probability of it ever becoming real.

>> I can certainly remove the "as documented by the GCC manual" part and
>> the -fwrapv setting, but silencing -Wshift-negative-value and
>> documenting what we rely on should go in.
>
> I don't see much point in documenting what we rely on
> if we can't rely on it and need to stop relying on it.

"Can't" and "need" are too strong.  The kernel can, and I fail to see
what makes us so special that we absolutely cannot.

For what it's worth, I'm sick and tired of patches "fixing" signed
shifts, and the unnecessary risk that comes with them.

Re: [Qemu-devel] [PATCH] Fix memory leak on error

[Markus Armbruster]

Stefano Dong (董兴水)  writes:

> hw/ppc/spapr.c: Fix memory leak on error, it was introduced in bc09e0611
> hw/acpi/memory_hotplug.c: Fix memory leak on error, it was introduced in 
> 34f2af3d
>
> Signed-off-by: Stefano Dong (董兴水) 
> ---
>  hw/acpi/memory_hotplug.c | 1 +
>  hw/ppc/spapr.c   | 1 +
>  2 files changed, 2 insertions(+)
>
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index ce428df..e4b9a01 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -155,6 +155,7 @@ static void acpi_memory_hotplug_write(void *opaque, 
> hwaddr addr, uint64_t data,
>  qapi_event_send_mem_unplug_error(dev->id,
>   error_get_pretty(local_err),
>   &error_abort);
> +error_free(local_err);
>  break;
>  }
>  trace_mhp_acpi_pc_dimm_deleted(mem_st->selector);
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 030ee35..3bb8bcd 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -125,6 +125,7 @@ static XICSState *xics_system_init(MachineState *machine,
>  error_report("kernel_irqchip requested but unavailable: %s",
>   error_get_pretty(err));
>  }
> +error_free(err);
>  }
>  
>  if (!icp) {

Two independent instances of the same kind of bug (failure to free an
Error after handling it).  Collecting multiple fixes of the same kind in
one patch can be fine.  Note, however, that the combined patch spans
separately maintained areas of the code:

$ scripts/get_maintainer.pl -f hw/acpi/memory_hotplug.c 
"Michael S. Tsirkin"  (supporter:ACPI/SMBIOS)
Igor Mammedov  (supporter:ACPI/SMBIOS)
$ scripts/get_maintainer.pl -f hw/ppc/spapr.c 
David Gibson  (supporter:sPAPR (pseries))
Alexander Graf  (supporter:sPAPR (pseries))
Eduardo Habkost  (maintainer:NUMA)
qemu-...@nongnu.org (open list:sPAPR (pseries))

If the combined patch is trivial, cc: qemu-triv...@nongnu.org.  I figure
this one could qualify.

If it isn't, split it up along mainenance boundaries, and cc: the
maintainer(s) for each part.

When you fix something with serious impact, the commit message should
give a clue when it was broken.  For minor bugs like these, it's still
nice to do.  The acpi_memory_hotplug_write() leak was introduced in
bc09e06 (v2.4.0), and the xics_system_init() leak in commit 34f2af3
(v2.3.0).  Thanks for fixing the mess I made there, by the way.

Since this is a straightforward bug fix, proposing it for inclusion into
2.5 makes sense.  You can do that by putting [PATCH for-2.5] into the
subject.

Re: [Qemu-devel] [PATCH] target-arm: Fix and improve AA32 singlestep translation completion code

[Peter Maydell]

On 25 November 2015 at 18:02, Sergey Fedorov  wrote:
> The AArch32 translation completion code for singlestep enabled/active
> case was a way more confusing and too repetitive then it needs to be.
> Probably that was the cause for a bug to be introduced into it at some
> point. The bug was that SWI/HVC/SMC exception would be generated in
> condition-failed instruction code path whereas it shouldn't.

So I did some testing, and I think this is a bug that's not actually
really visible to Linux guests. For both QEMU's gdbstub and for gdb
running within a system emulation, gdb for 32-bit ARM will prefer to
do singlestep via setting breakpoints rather than trying to use the
gdbstub's singlestep command. So while we should definitely fix it
(and the code cleanup is nice) I think we don't need to do this for 2.5,
and I'm going to put this on my review-for-2.6 list. Do you agree?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target-arm: Fix and improve AA32 singlestep translation completion code

[Sergey Fedorov]

On 26.11.2015 15:33, Peter Maydell wrote:
> On 25 November 2015 at 18:02, Sergey Fedorov  wrote:
>> The AArch32 translation completion code for singlestep enabled/active
>> case was a way more confusing and too repetitive then it needs to be.
>> Probably that was the cause for a bug to be introduced into it at some
>> point. The bug was that SWI/HVC/SMC exception would be generated in
>> condition-failed instruction code path whereas it shouldn't.
> So I did some testing, and I think this is a bug that's not actually
> really visible to Linux guests. For both QEMU's gdbstub and for gdb
> running within a system emulation, gdb for 32-bit ARM will prefer to
> do singlestep via setting breakpoints rather than trying to use the
> gdbstub's singlestep command. So while we should definitely fix it
> (and the code cleanup is nice) I think we don't need to do this for 2.5,
> and I'm going to put this on my review-for-2.6 list. Do you agree?

Sure, that's okay. I just wanted to finish this before I move on to
something else.

BTW, I used the following quick-and-dirty Perl script to do testing (it
was helpful to detect some bugs in my first attempts):

#!/usr/bin/perl

use strict;
use warnings;

use IO::Socket::INET;

our $addr = 'localhost:1234';

sub recv_pack {
my $sock = shift;
my $c = $sock->getc() || die();
if ($c eq '+') {
return $c;
}
if ($c eq '-') {
die;
}
if ($c eq '$') {
my $packet = $c;
while (($c = $sock->getc()) ne '#') {
defined($c) || die();
$packet .= $c;
}
$sock->getc();
$sock->getc();
$sock->print('+') || die();
return $packet;
}
return "";
}

sub wait_ack {
my $sock = shift;
my $pack = recv_pack($sock);
while ($pack ne "+") {
$pack = recv_pack($sock);
}
}

sub send_pack {
my $sock = shift;
my $packet = shift;
my $sum = unpack("%8C*", $packet);
$packet = '$' . $packet . '#' . sprintf("%hhx", $sum);
$sock->print($packet) || die();
wait_ack($sock);
}

our $sock = IO::Socket::INET->new($addr) || die();

our $quit = 0;

$SIG{INT} = sub { $quit = 1; };

my $nr_packets = 0;

while (!$quit) {
send_pack($sock, 's');
recv_pack($sock);
printf("\r%d packets sent", ++$nr_packets);
STDOUT->flush();
}

print("\n");

send_pack($sock, 'c');

Best regards,
Sergey

[Qemu-devel] [PULL for-2.5 04/13] check-qjson: Add test for JSON nesting depth limit

[Markus Armbruster]

This would have prevented the regression mentioned in the previous
commit.

Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
Message-Id: <1448486613-17634-4-git-send-email-arm...@redhat.com>
---
 tests/check-qjson.c | 25 +
 1 file changed, 25 insertions(+)

diff --git a/tests/check-qjson.c b/tests/check-qjson.c
index 1cfffa5..61e9bfb 100644
--- a/tests/check-qjson.c
+++ b/tests/check-qjson.c
@@ -1484,6 +1484,30 @@ static void unterminated_literal(void)
 g_assert(obj == NULL);
 }
 
+static char *make_nest(char *buf, size_t cnt)
+{
+memset(buf, '[', cnt - 1);
+buf[cnt - 1] = '{';
+buf[cnt] = '}';
+memset(buf + cnt + 1, ']', cnt - 1);
+buf[2 * cnt] = 0;
+return buf;
+}
+
+static void limits_nesting(void)
+{
+enum { max_nesting = 1024 }; /* see qobject/json-streamer.c */
+char buf[2 * (max_nesting + 1) + 1];
+QObject *obj;
+
+obj = qobject_from_json(make_nest(buf, max_nesting));
+g_assert(obj != NULL);
+qobject_decref(obj);
+
+obj = qobject_from_json(make_nest(buf, max_nesting + 1));
+g_assert(obj == NULL);
+}
+
 int main(int argc, char **argv)
 {
 g_test_init(&argc, &argv, NULL);
@@ -1519,6 +1543,7 @@ int main(int argc, char **argv)
 g_test_add_func("/errors/invalid_array_comma", invalid_array_comma);
 g_test_add_func("/errors/invalid_dict_comma", invalid_dict_comma);
 g_test_add_func("/errors/unterminated/literal", unterminated_literal);
+g_test_add_func("/errors/limits/nesting", limits_nesting);
 
 return g_test_run();
 }
-- 
2.4.3

[Qemu-devel] [PULL for-2.5 01/13] monitor: Plug memory leak on QMP error

[Markus Armbruster]

Leak introduced in commit 8a4f501..710aec9, v2.4.0.

Signed-off-by: Markus Armbruster 
Message-Id: <1446117309-15322-1-git-send-email-arm...@redhat.com>
Reviewed-by: Eric Blake 
Reviewed-by: Luiz Capitulino 
---
 monitor.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/monitor.c b/monitor.c
index e4cf34e..49073ac 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3907,6 +3907,7 @@ static void handle_qmp_command(JSONMessageParser *parser, 
QList *tokens)
 err_out:
 monitor_protocol_emitter(mon, data, local_err);
 qobject_decref(data);
+error_free(local_err);
 QDECREF(input);
 QDECREF(args);
 }
-- 
2.4.3

[Qemu-devel] [PULL for-2.5 10/13] qjson: Convert to parser to recursive descent

[Markus Armbruster]

We backtrack in parse_value(), even though JSON is LL(1) and thus can
be parsed by straightforward recursive descent.  Do exactly that.

Based on an almost-correct patch from Paolo Bonzini.

Signed-off-by: Paolo Bonzini 
Signed-off-by: Markus Armbruster 
Message-Id: <1448486613-17634-10-git-send-email-arm...@redhat.com>
Reviewed-by: Eric Blake 
---
 qobject/json-parser.c | 165 ++
 1 file changed, 47 insertions(+), 118 deletions(-)

diff --git a/qobject/json-parser.c b/qobject/json-parser.c
index b57cac7..60dd624 100644
--- a/qobject/json-parser.c
+++ b/qobject/json-parser.c
@@ -265,23 +265,6 @@ static QObject 
*parser_context_peek_token(JSONParserContext *ctxt)
 return token;
 }
 
-static JSONParserContext parser_context_save(JSONParserContext *ctxt)
-{
-JSONParserContext saved_ctxt = {0};
-saved_ctxt.tokens.pos = ctxt->tokens.pos;
-saved_ctxt.tokens.count = ctxt->tokens.count;
-saved_ctxt.tokens.buf = ctxt->tokens.buf;
-return saved_ctxt;
-}
-
-static void parser_context_restore(JSONParserContext *ctxt,
-   JSONParserContext saved_ctxt)
-{
-ctxt->tokens.pos = saved_ctxt.tokens.pos;
-ctxt->tokens.count = saved_ctxt.tokens.count;
-ctxt->tokens.buf = saved_ctxt.tokens.buf;
-}
-
 static void tokens_append_from_iter(QObject *obj, void *opaque)
 {
 JSONParserContext *ctxt = opaque;
@@ -333,7 +316,6 @@ static void parser_context_free(JSONParserContext *ctxt)
 static int parse_pair(JSONParserContext *ctxt, QDict *dict, va_list *ap)
 {
 QObject *key = NULL, *token = NULL, *value, *peek;
-JSONParserContext saved_ctxt = parser_context_save(ctxt);
 
 peek = parser_context_peek_token(ctxt);
 if (peek == NULL) {
@@ -371,7 +353,6 @@ static int parse_pair(JSONParserContext *ctxt, QDict *dict, 
va_list *ap)
 return 0;
 
 out:
-parser_context_restore(ctxt, saved_ctxt);
 qobject_decref(key);
 
 return -1;
@@ -381,16 +362,9 @@ static QObject *parse_object(JSONParserContext *ctxt, 
va_list *ap)
 {
 QDict *dict = NULL;
 QObject *token, *peek;
-JSONParserContext saved_ctxt = parser_context_save(ctxt);
 
 token = parser_context_pop_token(ctxt);
-if (token == NULL) {
-goto out;
-}
-
-if (token_get_type(token) != JSON_LCURLY) {
-goto out;
-}
+assert(token && token_get_type(token) == JSON_LCURLY);
 
 dict = qdict_new();
 
@@ -434,7 +408,6 @@ static QObject *parse_object(JSONParserContext *ctxt, 
va_list *ap)
 return QOBJECT(dict);
 
 out:
-parser_context_restore(ctxt, saved_ctxt);
 QDECREF(dict);
 return NULL;
 }
@@ -443,16 +416,9 @@ static QObject *parse_array(JSONParserContext *ctxt, 
va_list *ap)
 {
 QList *list = NULL;
 QObject *token, *peek;
-JSONParserContext saved_ctxt = parser_context_save(ctxt);
 
 token = parser_context_pop_token(ctxt);
-if (token == NULL) {
-goto out;
-}
-
-if (token_get_type(token) != JSON_LSQUARE) {
-goto out;
-}
+assert(token && token_get_type(token) == JSON_LSQUARE);
 
 list = qlist_new();
 
@@ -506,109 +472,72 @@ static QObject *parse_array(JSONParserContext *ctxt, 
va_list *ap)
 return QOBJECT(list);
 
 out:
-parser_context_restore(ctxt, saved_ctxt);
 QDECREF(list);
 return NULL;
 }
 
 static QObject *parse_keyword(JSONParserContext *ctxt)
 {
-QObject *token, *ret;
-JSONParserContext saved_ctxt = parser_context_save(ctxt);
+QObject *token;
 const char *val;
 
 token = parser_context_pop_token(ctxt);
-if (token == NULL) {
-goto out;
-}
-
-if (token_get_type(token) != JSON_KEYWORD) {
-goto out;
-}
-
+assert(token && token_get_type(token) == JSON_KEYWORD);
 val = token_get_value(token);
 
 if (!strcmp(val, "true")) {
-ret = QOBJECT(qbool_from_bool(true));
+return QOBJECT(qbool_from_bool(true));
 } else if (!strcmp(val, "false")) {
-ret = QOBJECT(qbool_from_bool(false));
+return QOBJECT(qbool_from_bool(false));
 } else if (!strcmp(val, "null")) {
-ret = qnull();
-} else {
-parse_error(ctxt, token, "invalid keyword '%s'", val);
-goto out;
+return qnull();
 }
-
-return ret;
-
-out: 
-parser_context_restore(ctxt, saved_ctxt);
-
+parse_error(ctxt, token, "invalid keyword '%s'", val);
 return NULL;
 }
 
 static QObject *parse_escape(JSONParserContext *ctxt, va_list *ap)
 {
-QObject *token = NULL, *obj;
-JSONParserContext saved_ctxt = parser_context_save(ctxt);
+QObject *token;
 const char *val;
 
 if (ap == NULL) {
-goto out;
+return NULL;
 }
 
 token = parser_context_pop_token(ctxt);
-if (token == NULL) {
-goto out;
-}
-
-if (token_get_type(token) != JSON_ESCAPE) {
-goto out;
-}
-
+assert(token && token_get_type(token) == JSON_ESCAPE);
 val = token_get_value(token);
 
 if 

[Qemu-devel] [PULL for-2.5 08/13] qjson: Inline token_is_escape() and simplify

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
Message-Id: <1448486613-17634-8-git-send-email-arm...@redhat.com>
Reviewed-by: Eric Blake 
---
 qobject/json-parser.c | 32 +++-
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/qobject/json-parser.c b/qobject/json-parser.c
index df76cc3..b57cac7 100644
--- a/qobject/json-parser.c
+++ b/qobject/json-parser.c
@@ -63,15 +63,6 @@ static JSONTokenType token_get_type(QObject *obj)
 return qdict_get_int(qobject_to_qdict(obj), "type");
 }
 
-static int token_is_escape(QObject *obj, const char *value)
-{
-if (token_get_type(obj) != JSON_ESCAPE) {
-return 0;
-}
-
-return (strcmp(token_get_value(obj), value) == 0);
-}
-
 /**
  * Error handler
  */
@@ -560,6 +551,7 @@ static QObject *parse_escape(JSONParserContext *ctxt, 
va_list *ap)
 {
 QObject *token = NULL, *obj;
 JSONParserContext saved_ctxt = parser_context_save(ctxt);
+const char *val;
 
 if (ap == NULL) {
 goto out;
@@ -570,20 +562,26 @@ static QObject *parse_escape(JSONParserContext *ctxt, 
va_list *ap)
 goto out;
 }
 
-if (token_is_escape(token, "%p")) {
+if (token_get_type(token) != JSON_ESCAPE) {
+goto out;
+}
+
+val = token_get_value(token);
+
+if (!strcmp(val, "%p")) {
 obj = va_arg(*ap, QObject *);
-} else if (token_is_escape(token, "%i")) {
+} else if (!strcmp(val, "%i")) {
 obj = QOBJECT(qbool_from_bool(va_arg(*ap, int)));
-} else if (token_is_escape(token, "%d")) {
+} else if (!strcmp(val, "%d")) {
 obj = QOBJECT(qint_from_int(va_arg(*ap, int)));
-} else if (token_is_escape(token, "%ld")) {
+} else if (!strcmp(val, "%ld")) {
 obj = QOBJECT(qint_from_int(va_arg(*ap, long)));
-} else if (token_is_escape(token, "%lld") ||
-   token_is_escape(token, "%I64d")) {
+} else if (!strcmp(val, "%lld") ||
+   !strcmp(val, "%I64d")) {
 obj = QOBJECT(qint_from_int(va_arg(*ap, long long)));
-} else if (token_is_escape(token, "%s")) {
+} else if (!strcmp(val, "%s")) {
 obj = QOBJECT(qstring_from_str(va_arg(*ap, const char *)));
-} else if (token_is_escape(token, "%f")) {
+} else if (!strcmp(val, "%f")) {
 obj = QOBJECT(qfloat_from_double(va_arg(*ap, double)));
 } else {
 goto out;
-- 
2.4.3

[Qemu-devel] [PULL for-2.5 07/13] qjson: Inline token_is_keyword() and simplify

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
Message-Id: <1448486613-17634-7-git-send-email-arm...@redhat.com>
Reviewed-by: Eric Blake 
---
 qobject/json-parser.c | 20 +++-
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/qobject/json-parser.c b/qobject/json-parser.c
index 020c6e1..df76cc3 100644
--- a/qobject/json-parser.c
+++ b/qobject/json-parser.c
@@ -63,15 +63,6 @@ static JSONTokenType token_get_type(QObject *obj)
 return qdict_get_int(qobject_to_qdict(obj), "type");
 }
 
-static int token_is_keyword(QObject *obj, const char *value)
-{
-if (token_get_type(obj) != JSON_KEYWORD) {
-return 0;
-}
-
-return strcmp(token_get_value(obj), value) == 0;
-}
-
 static int token_is_escape(QObject *obj, const char *value)
 {
 if (token_get_type(obj) != JSON_ESCAPE) {
@@ -533,6 +524,7 @@ static QObject *parse_keyword(JSONParserContext *ctxt)
 {
 QObject *token, *ret;
 JSONParserContext saved_ctxt = parser_context_save(ctxt);
+const char *val;
 
 token = parser_context_pop_token(ctxt);
 if (token == NULL) {
@@ -543,14 +535,16 @@ static QObject *parse_keyword(JSONParserContext *ctxt)
 goto out;
 }
 
-if (token_is_keyword(token, "true")) {
+val = token_get_value(token);
+
+if (!strcmp(val, "true")) {
 ret = QOBJECT(qbool_from_bool(true));
-} else if (token_is_keyword(token, "false")) {
+} else if (!strcmp(val, "false")) {
 ret = QOBJECT(qbool_from_bool(false));
-} else if (token_is_keyword(token, "null")) {
+} else if (!strcmp(val, "null")) {
 ret = qnull();
 } else {
-parse_error(ctxt, token, "invalid keyword `%s'", 
token_get_value(token));
+parse_error(ctxt, token, "invalid keyword '%s'", val);
 goto out;
 }
 
-- 
2.4.3

[Qemu-devel] [PULL for-2.5 00/13] QMP and QObject patches

[Markus Armbruster]

The following changes since commit 1a4dab849d5d06191ab5e5850f6b8bfcad8ceb47:

  Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into staging 
(2015-11-25 14:47:06 +)

are available in the git repository at:

  git://repo.or.cz/qemu/armbru.git tags/pull-monitor-2015-11-26

for you to fetch changes up to df649835fe48f635a93316fdefe96ced7189316e:

  qjson: Limit number of tokens in addition to total size (2015-11-26 10:07:07 
+0100)


QMP and QObject patches


Markus Armbruster (10):
  monitor: Plug memory leak on QMP error
  qjson: Apply nesting limit more sanely
  qjson: Don't crash when input exceeds nesting limit
  check-qjson: Add test for JSON nesting depth limit
  qjson: Spell out some silent assumptions
  qjson: Give each of the six structural chars its own token type
  qjson: Inline token_is_keyword() and simplify
  qjson: Inline token_is_escape() and simplify
  qjson: Convert to parser to recursive descent
  qjson: Limit number of tokens in addition to total size

Paolo Bonzini (3):
  qjson: replace QString in JSONLexer with GString
  qjson: store tokens in a GQueue
  qjson: surprise, allocating 6 QObjects per token is expensive

 include/qapi/qmp/json-lexer.h|  16 +-
 include/qapi/qmp/json-parser.h   |   4 +-
 include/qapi/qmp/json-streamer.h |  16 +-
 monitor.c|   3 +-
 qga/main.c   |   2 +-
 qobject/json-lexer.c |  48 +++---
 qobject/json-parser.c| 330 ---
 qobject/json-streamer.c  |  89 ++-
 qobject/qjson.c  |   2 +-
 tests/check-qjson.c  |  25 +++
 tests/libqtest.c |   2 +-
 11 files changed, 224 insertions(+), 313 deletions(-)

-- 
2.4.3

[Qemu-devel] [PULL for-2.5 02/13] qjson: Apply nesting limit more sanely

[Markus Armbruster]

The nesting limit from commit 29c75dd "json-streamer: limit the
maximum recursion depth and maximum token count" applies separately to
braces and brackets.  This makes no sense.  Apply it to their sum,
because that's actually a measure of recursion depth.

Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
Message-Id: <1448486613-17634-2-git-send-email-arm...@redhat.com>
---
 qobject/json-streamer.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/qobject/json-streamer.c b/qobject/json-streamer.c
index 1b2f9b1..dced2c7 100644
--- a/qobject/json-streamer.c
+++ b/qobject/json-streamer.c
@@ -64,8 +64,7 @@ static void json_message_process_token(JSONLexer *lexer, 
QString *token, JSONTok
  parser->bracket_count == 0)) {
 goto out_emit;
 } else if (parser->token_size > MAX_TOKEN_SIZE ||
-   parser->bracket_count > MAX_NESTING ||
-   parser->brace_count > MAX_NESTING) {
+   parser->bracket_count + parser->brace_count > MAX_NESTING) {
 /* Security consideration, we limit total memory allocated per object
  * and the maximum recursion depth that a message can force.
  */
-- 
2.4.3

[Qemu-devel] [PULL for-2.5 11/13] qjson: store tokens in a GQueue

[Markus Armbruster]

From: Paolo Bonzini 

Even though we still have the "streamer" concept, the tokens can now
be deleted as they are read.  While doing so convert from QList to
GQueue, since the next step will make tokens not a QObject and we
will have to do the conversion anyway.

Signed-off-by: Paolo Bonzini 
Message-Id: <1448300659-23559-4-git-send-email-pbonz...@redhat.com>
Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
---
 include/qapi/qmp/json-parser.h   |  4 +--
 include/qapi/qmp/json-streamer.h |  8 ++---
 monitor.c|  2 +-
 qga/main.c   |  2 +-
 qobject/json-parser.c| 65 +---
 qobject/json-streamer.c  | 25 +---
 qobject/qjson.c  |  2 +-
 tests/libqtest.c |  2 +-
 8 files changed, 45 insertions(+), 65 deletions(-)

diff --git a/include/qapi/qmp/json-parser.h b/include/qapi/qmp/json-parser.h
index 44d88f3..fea89f8 100644
--- a/include/qapi/qmp/json-parser.h
+++ b/include/qapi/qmp/json-parser.h
@@ -18,7 +18,7 @@
 #include "qapi/qmp/qlist.h"
 #include "qapi/error.h"
 
-QObject *json_parser_parse(QList *tokens, va_list *ap);
-QObject *json_parser_parse_err(QList *tokens, va_list *ap, Error **errp);
+QObject *json_parser_parse(GQueue *tokens, va_list *ap);
+QObject *json_parser_parse_err(GQueue *tokens, va_list *ap, Error **errp);
 
 #endif
diff --git a/include/qapi/qmp/json-streamer.h b/include/qapi/qmp/json-streamer.h
index e901144..e9f2937 100644
--- a/include/qapi/qmp/json-streamer.h
+++ b/include/qapi/qmp/json-streamer.h
@@ -15,21 +15,21 @@
 #define QEMU_JSON_STREAMER_H
 
 #include 
-#include "qapi/qmp/qlist.h"
+#include "glib-compat.h"
 #include "qapi/qmp/json-lexer.h"
 
 typedef struct JSONMessageParser
 {
-void (*emit)(struct JSONMessageParser *parser, QList *tokens);
+void (*emit)(struct JSONMessageParser *parser, GQueue *tokens);
 JSONLexer lexer;
 int brace_count;
 int bracket_count;
-QList *tokens;
+GQueue *tokens;
 uint64_t token_size;
 } JSONMessageParser;
 
 void json_message_parser_init(JSONMessageParser *parser,
-  void (*func)(JSONMessageParser *, QList *));
+  void (*func)(JSONMessageParser *, GQueue *));
 
 int json_message_parser_feed(JSONMessageParser *parser,
  const char *buffer, size_t size);
diff --git a/monitor.c b/monitor.c
index 49073ac..9a35d72 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3849,7 +3849,7 @@ static QDict *qmp_check_input_obj(QObject *input_obj, 
Error **errp)
 return input_dict;
 }
 
-static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
+static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
 {
 Error *local_err = NULL;
 QObject *obj, *data;
diff --git a/qga/main.c b/qga/main.c
index d2a0ffc..f83a97d 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -570,7 +570,7 @@ static void process_command(GAState *s, QDict *req)
 }
 
 /* handle requests/control events coming in over the channel */
-static void process_event(JSONMessageParser *parser, QList *tokens)
+static void process_event(JSONMessageParser *parser, GQueue *tokens)
 {
 GAState *s = container_of(parser, GAState, parser);
 QDict *qdict;
diff --git a/qobject/json-parser.c b/qobject/json-parser.c
index 60dd624..5a84951 100644
--- a/qobject/json-parser.c
+++ b/qobject/json-parser.c
@@ -26,11 +26,8 @@
 typedef struct JSONParserContext
 {
 Error *err;
-struct {
-QObject **buf;
-size_t pos;
-size_t count;
-} tokens;
+QObject *current;
+GQueue *buf;
 } JSONParserContext;
 
 #define BUG_ON(cond) assert(!(cond))
@@ -243,56 +240,34 @@ out:
 return NULL;
 }
 
+/* Note: unless the token object returned by parser_context_peek_token
+ * or parser_context_pop_token is explicitly incref'd, it will be
+ * deleted as soon as parser_context_pop_token is called again.
+ */
 static QObject *parser_context_pop_token(JSONParserContext *ctxt)
 {
-QObject *token;
-g_assert(ctxt->tokens.pos < ctxt->tokens.count);
-token = ctxt->tokens.buf[ctxt->tokens.pos];
-ctxt->tokens.pos++;
-return token;
+qobject_decref(ctxt->current);
+assert(!g_queue_is_empty(ctxt->buf));
+ctxt->current = g_queue_pop_head(ctxt->buf);
+return ctxt->current;
 }
 
-/* Note: parser_context_{peek|pop}_token do not increment the
- * token object's refcount. In both cases the references will continue
- * to be tracked and cleaned up in parser_context_free(), so do not
- * attempt to free the token object.
- */
 static QObject *parser_context_peek_token(JSONParserContext *ctxt)
 {
-QObject *token;
-g_assert(ctxt->tokens.pos < ctxt->tokens.count);
-token = ctxt->tokens.buf[ctxt->tokens.pos];
-return token;
+assert(!g_queue_is_empty(ctxt->buf));
+return g_queue_peek_head(ctxt->buf);
 }
 
-static void tokens_append_from_iter(QObject *obj, void *opaque)
-{
-JS

[Qemu-devel] [PULL for-2.5 03/13] qjson: Don't crash when input exceeds nesting limit

[Markus Armbruster]

We limit nesting depth and input size to defend against input
triggering excessive heap or stack memory use (commit 29c75dd
json-streamer: limit the maximum recursion depth and maximum token
count).  However, when the nesting limit is exceeded,
parser_context_peek_token()'s assertion fails.

Broken in commit 65c0f1e "json-parser: don't replicate tokens at each
level of recursion".

To reproduce stuff 1025 open braces or brackets into QMP.

Fix by taking the error exit instead of the normal one.

Reported-by: Eric Blake 
Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
Message-Id: <1448486613-17634-3-git-send-email-arm...@redhat.com>
---
 qobject/json-streamer.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/qobject/json-streamer.c b/qobject/json-streamer.c
index dced2c7..2bd22a7 100644
--- a/qobject/json-streamer.c
+++ b/qobject/json-streamer.c
@@ -68,13 +68,14 @@ static void json_message_process_token(JSONLexer *lexer, 
QString *token, JSONTok
 /* Security consideration, we limit total memory allocated per object
  * and the maximum recursion depth that a message can force.
  */
-goto out_emit;
+goto out_emit_bad;
 }
 
 return;
 
 out_emit_bad:
-/* clear out token list and tell the parser to emit and error
+/*
+ * Clear out token list and tell the parser to emit an error
  * indication by passing it a NULL list
  */
 QDECREF(parser->tokens);
-- 
2.4.3

[Qemu-devel] [PULL for-2.5 06/13] qjson: Give each of the six structural chars its own token type

[Markus Armbruster]

Simplifies things, because we always check for a specific one.

Signed-off-by: Markus Armbruster 
Message-Id: <1448486613-17634-6-git-send-email-arm...@redhat.com>
Reviewed-by: Eric Blake 
---
 include/qapi/qmp/json-lexer.h |  7 ++-
 qobject/json-lexer.c  | 19 ---
 qobject/json-parser.c | 31 +--
 qobject/json-streamer.c   | 32 +++-
 4 files changed, 42 insertions(+), 47 deletions(-)

diff --git a/include/qapi/qmp/json-lexer.h b/include/qapi/qmp/json-lexer.h
index 61a143f..f3e8dc7 100644
--- a/include/qapi/qmp/json-lexer.h
+++ b/include/qapi/qmp/json-lexer.h
@@ -19,7 +19,12 @@
 
 typedef enum json_token_type {
 JSON_MIN = 100,
-JSON_OPERATOR = JSON_MIN,
+JSON_LCURLY = JSON_MIN,
+JSON_RCURLY,
+JSON_LSQUARE,
+JSON_RSQUARE,
+JSON_COLON,
+JSON_COMMA,
 JSON_INTEGER,
 JSON_FLOAT,
 JSON_KEYWORD,
diff --git a/qobject/json-lexer.c b/qobject/json-lexer.c
index 5735c1e..1df7d5e 100644
--- a/qobject/json-lexer.c
+++ b/qobject/json-lexer.c
@@ -257,12 +257,12 @@ static const uint8_t json_lexer[][256] =  {
 ['0'] = IN_ZERO,
 ['1' ... '9'] = IN_NONZERO_NUMBER,
 ['-'] = IN_NEG_NONZERO_NUMBER,
-['{'] = JSON_OPERATOR,
-['}'] = JSON_OPERATOR,
-['['] = JSON_OPERATOR,
-[']'] = JSON_OPERATOR,
-[','] = JSON_OPERATOR,
-[':'] = JSON_OPERATOR,
+['{'] = JSON_LCURLY,
+['}'] = JSON_RCURLY,
+['['] = JSON_LSQUARE,
+[']'] = JSON_RSQUARE,
+[','] = JSON_COMMA,
+[':'] = JSON_COLON,
 ['a' ... 'z'] = IN_KEYWORD,
 ['%'] = IN_ESCAPE,
 [' '] = IN_WHITESPACE,
@@ -299,7 +299,12 @@ static int json_lexer_feed_char(JSONLexer *lexer, char ch, 
bool flush)
 }
 
 switch (new_state) {
-case JSON_OPERATOR:
+case JSON_LCURLY:
+case JSON_RCURLY:
+case JSON_LSQUARE:
+case JSON_RSQUARE:
+case JSON_COLON:
+case JSON_COMMA:
 case JSON_ESCAPE:
 case JSON_INTEGER:
 case JSON_FLOAT:
diff --git a/qobject/json-parser.c b/qobject/json-parser.c
index ac991ba..020c6e1 100644
--- a/qobject/json-parser.c
+++ b/qobject/json-parser.c
@@ -63,19 +63,6 @@ static JSONTokenType token_get_type(QObject *obj)
 return qdict_get_int(qobject_to_qdict(obj), "type");
 }
 
-static int token_is_operator(QObject *obj, char op)
-{
-const char *val;
-
-if (token_get_type(obj) != JSON_OPERATOR) {
-return 0;
-}
-
-val = token_get_value(obj);
-
-return (val[0] == op) && (val[1] == 0);
-}
-
 static int token_is_keyword(QObject *obj, const char *value)
 {
 if (token_get_type(obj) != JSON_KEYWORD) {
@@ -384,7 +371,7 @@ static int parse_pair(JSONParserContext *ctxt, QDict *dict, 
va_list *ap)
 goto out;
 }
 
-if (!token_is_operator(token, ':')) {
+if (token_get_type(token) != JSON_COLON) {
 parse_error(ctxt, token, "missing : in object pair");
 goto out;
 }
@@ -419,7 +406,7 @@ static QObject *parse_object(JSONParserContext *ctxt, 
va_list *ap)
 goto out;
 }
 
-if (!token_is_operator(token, '{')) {
+if (token_get_type(token) != JSON_LCURLY) {
 goto out;
 }
 
@@ -431,7 +418,7 @@ static QObject *parse_object(JSONParserContext *ctxt, 
va_list *ap)
 goto out;
 }
 
-if (!token_is_operator(peek, '}')) {
+if (token_get_type(peek) != JSON_RCURLY) {
 if (parse_pair(ctxt, dict, ap) == -1) {
 goto out;
 }
@@ -442,8 +429,8 @@ static QObject *parse_object(JSONParserContext *ctxt, 
va_list *ap)
 goto out;
 }
 
-while (!token_is_operator(token, '}')) {
-if (!token_is_operator(token, ',')) {
+while (token_get_type(token) != JSON_RCURLY) {
+if (token_get_type(token) != JSON_COMMA) {
 parse_error(ctxt, token, "expected separator in dict");
 goto out;
 }
@@ -481,7 +468,7 @@ static QObject *parse_array(JSONParserContext *ctxt, 
va_list *ap)
 goto out;
 }
 
-if (!token_is_operator(token, '[')) {
+if (token_get_type(token) != JSON_LSQUARE) {
 goto out;
 }
 
@@ -493,7 +480,7 @@ static QObject *parse_array(JSONParserContext *ctxt, 
va_list *ap)
 goto out;
 }
 
-if (!token_is_operator(peek, ']')) {
+if (token_get_type(peek) != JSON_RSQUARE) {
 QObject *obj;
 
 obj = parse_value(ctxt, ap);
@@ -510,8 +497,8 @@ static QObject *parse_array(JSONParserContext *ctxt, 
va_list *ap)
 goto out;
 }
 
-while (!token_is_operator(token, ']')) {
-if (!token_is_operator(token, ',')) {
+while (token_get_type(token) != JSON_RSQUARE) {
+if (token_get_type(token) != JSON_COMMA) {
 parse_error(ctxt, token, "expected separator in list");
 goto out;
 

[Qemu-devel] [PULL for-2.5 05/13] qjson: Spell out some silent assumptions

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
Message-Id: <1448486613-17634-5-git-send-email-arm...@redhat.com>
Reviewed-by: Eric Blake 
---
 include/qapi/qmp/json-lexer.h | 3 ++-
 qobject/json-lexer.c  | 7 ++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/include/qapi/qmp/json-lexer.h b/include/qapi/qmp/json-lexer.h
index cdff046..61a143f 100644
--- a/include/qapi/qmp/json-lexer.h
+++ b/include/qapi/qmp/json-lexer.h
@@ -18,7 +18,8 @@
 #include "qapi/qmp/qlist.h"
 
 typedef enum json_token_type {
-JSON_OPERATOR = 100,
+JSON_MIN = 100,
+JSON_OPERATOR = JSON_MIN,
 JSON_INTEGER,
 JSON_FLOAT,
 JSON_KEYWORD,
diff --git a/qobject/json-lexer.c b/qobject/json-lexer.c
index b19623e..5735c1e 100644
--- a/qobject/json-lexer.c
+++ b/qobject/json-lexer.c
@@ -30,7 +30,7 @@
  */
 
 enum json_lexer_state {
-IN_ERROR = 0,
+IN_ERROR = 0,   /* must really be 0, see json_lexer[] */
 IN_DQ_UCODE3,
 IN_DQ_UCODE2,
 IN_DQ_UCODE1,
@@ -62,6 +62,8 @@ enum json_lexer_state {
 IN_START,
 };
 
+QEMU_BUILD_BUG_ON((int)JSON_MIN <= (int)IN_START);
+
 #define TERMINAL(state) [0 ... 0x7F] = (state)
 
 /* Return whether TERMINAL is a terminal state and the transition to it
@@ -71,6 +73,8 @@ enum json_lexer_state {
 (json_lexer[(old_state)][0] == (terminal))
 
 static const uint8_t json_lexer[][256] =  {
+/* Relies on default initialization to IN_ERROR! */
+
 /* double quote string */
 [IN_DQ_UCODE3] = {
 ['0' ... '9'] = IN_DQ_STRING,
@@ -287,6 +291,7 @@ static int json_lexer_feed_char(JSONLexer *lexer, char ch, 
bool flush)
 }
 
 do {
+assert(lexer->state <= ARRAY_SIZE(json_lexer));
 new_state = json_lexer[lexer->state][(uint8_t)ch];
 char_consumed = !TERMINAL_NEEDED_LOOKAHEAD(lexer->state, new_state);
 if (char_consumed) {
-- 
2.4.3

[Qemu-devel] [PULL for-2.5 12/13] qjson: surprise, allocating 6 QObjects per token is expensive

[Markus Armbruster]

From: Paolo Bonzini 

Replace the contents of the tokens GQueue with a simple struct.  This cuts
the amount of memory allocated by tests/check-qjson from ~500MB to ~20MB,
and the execution time from 600ms to 80ms on my laptop.  Still a lot (some
could be saved by using an intrusive list, such as QSIMPLEQ, instead of
the GQueue), but the savings are already massive and the right thing to
do would probably be to get rid of json-streamer completely.

Signed-off-by: Paolo Bonzini 
Message-Id: <1448300659-23559-5-git-send-email-pbonz...@redhat.com>
[Straightforwardly rebased on my patches]
Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
---
 include/qapi/qmp/json-streamer.h |   7 +++
 qobject/json-parser.c| 115 ---
 qobject/json-streamer.c  |  19 +++
 3 files changed, 63 insertions(+), 78 deletions(-)

diff --git a/include/qapi/qmp/json-streamer.h b/include/qapi/qmp/json-streamer.h
index e9f2937..09b3d3e 100644
--- a/include/qapi/qmp/json-streamer.h
+++ b/include/qapi/qmp/json-streamer.h
@@ -18,6 +18,13 @@
 #include "glib-compat.h"
 #include "qapi/qmp/json-lexer.h"
 
+typedef struct JSONToken {
+int type;
+int x;
+int y;
+char str[];
+} JSONToken;
+
 typedef struct JSONMessageParser
 {
 void (*emit)(struct JSONMessageParser *parser, GQueue *tokens);
diff --git a/qobject/json-parser.c b/qobject/json-parser.c
index 5a84951..3c5d35d 100644
--- a/qobject/json-parser.c
+++ b/qobject/json-parser.c
@@ -22,11 +22,12 @@
 #include "qapi/qmp/qbool.h"
 #include "qapi/qmp/json-parser.h"
 #include "qapi/qmp/json-lexer.h"
+#include "qapi/qmp/json-streamer.h"
 
 typedef struct JSONParserContext
 {
 Error *err;
-QObject *current;
+JSONToken *current;
 GQueue *buf;
 } JSONParserContext;
 
@@ -44,27 +45,10 @@ typedef struct JSONParserContext
 static QObject *parse_value(JSONParserContext *ctxt, va_list *ap);
 
 /**
- * Token manipulators
- *
- * tokens are dictionaries that contain a type, a string value, and geometry 
information
- * about a token identified by the lexer.  These are routines that make 
working with
- * these objects a bit easier.
- */
-static const char *token_get_value(QObject *obj)
-{
-return qdict_get_str(qobject_to_qdict(obj), "token");
-}
-
-static JSONTokenType token_get_type(QObject *obj)
-{
-return qdict_get_int(qobject_to_qdict(obj), "type");
-}
-
-/**
  * Error handler
  */
 static void GCC_FMT_ATTR(3, 4) parse_error(JSONParserContext *ctxt,
-   QObject *token, const char *msg, 
...)
+   JSONToken *token, const char *msg, 
...)
 {
 va_list ap;
 char message[1024];
@@ -142,9 +126,10 @@ static int hex2decimal(char ch)
  *  \t
  *  \u four-hex-digits 
  */
-static QString *qstring_from_escaped_str(JSONParserContext *ctxt, QObject 
*token)
+static QString *qstring_from_escaped_str(JSONParserContext *ctxt,
+ JSONToken *token)
 {
-const char *ptr = token_get_value(token);
+const char *ptr = token->str;
 QString *str;
 int double_quote = 1;
 
@@ -240,19 +225,19 @@ out:
 return NULL;
 }
 
-/* Note: unless the token object returned by parser_context_peek_token
- * or parser_context_pop_token is explicitly incref'd, it will be
- * deleted as soon as parser_context_pop_token is called again.
+/* Note: the token object returned by parser_context_peek_token or
+ * parser_context_pop_token is deleted as soon as parser_context_pop_token
+ * is called again.
  */
-static QObject *parser_context_pop_token(JSONParserContext *ctxt)
+static JSONToken *parser_context_pop_token(JSONParserContext *ctxt)
 {
-qobject_decref(ctxt->current);
+g_free(ctxt->current);
 assert(!g_queue_is_empty(ctxt->buf));
 ctxt->current = g_queue_pop_head(ctxt->buf);
 return ctxt->current;
 }
 
-static QObject *parser_context_peek_token(JSONParserContext *ctxt)
+static JSONToken *parser_context_peek_token(JSONParserContext *ctxt)
 {
 assert(!g_queue_is_empty(ctxt->buf));
 return g_queue_peek_head(ctxt->buf);
@@ -279,7 +264,7 @@ static void parser_context_free(JSONParserContext *ctxt)
 while (!g_queue_is_empty(ctxt->buf)) {
 parser_context_pop_token(ctxt);
 }
-qobject_decref(ctxt->current);
+g_free(ctxt->current);
 g_queue_free(ctxt->buf);
 g_free(ctxt);
 }
@@ -290,7 +275,8 @@ static void parser_context_free(JSONParserContext *ctxt)
  */
 static int parse_pair(JSONParserContext *ctxt, QDict *dict, va_list *ap)
 {
-QObject *key = NULL, *token = NULL, *value, *peek;
+QObject *key = NULL, *value;
+JSONToken *peek, *token;
 
 peek = parser_context_peek_token(ctxt);
 if (peek == NULL) {
@@ -310,7 +296,7 @@ static int parse_pair(JSONParserContext *ctxt, QDict *dict, 
va_list *ap)
 goto out;
 }
 
-if (token_get_type(token) != JSON_COLON) {
+if (token->type != 

[Qemu-devel] [PULL for-2.5 13/13] qjson: Limit number of tokens in addition to total size

[Markus Armbruster]

Commit 29c75dd "json-streamer: limit the maximum recursion depth and
maximum token count" attempts to guard against excessive heap usage by
limiting total token size (it says "token count", but that's a lie).

Total token size is a rather imprecise predictor of heap usage: many
small tokens use more space than few large tokens with the same input
size, because there's a constant per-token overhead: 37 bytes on my
system.

Tighten this up: limit the token count to 2Mi.  Chosen to roughly
match the 64MiB total token size limit.

Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
Message-Id: <1448486613-17634-13-git-send-email-arm...@redhat.com>
---
 qobject/json-streamer.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/qobject/json-streamer.c b/qobject/json-streamer.c
index e87230d..a4db4b8 100644
--- a/qobject/json-streamer.c
+++ b/qobject/json-streamer.c
@@ -16,6 +16,7 @@
 #include "qapi/qmp/json-streamer.h"
 
 #define MAX_TOKEN_SIZE (64ULL << 20)
+#define MAX_TOKEN_COUNT (2ULL << 20)
 #define MAX_NESTING (1ULL << 10)
 
 static void json_message_free_tokens(JSONMessageParser *parser)
@@ -68,6 +69,7 @@ static void json_message_process_token(JSONLexer *lexer, 
GString *input,
  parser->bracket_count == 0)) {
 goto out_emit;
 } else if (parser->token_size > MAX_TOKEN_SIZE ||
+   g_queue_get_length(parser->tokens) > MAX_TOKEN_COUNT ||
parser->bracket_count + parser->brace_count > MAX_NESTING) {
 /* Security consideration, we limit total memory allocated per object
  * and the maximum recursion depth that a message can force.
-- 
2.4.3

[Qemu-devel] [PULL for-2.5 09/13] qjson: replace QString in JSONLexer with GString

[Markus Armbruster]

From: Paolo Bonzini 

JSONLexer only needs a simple resizable buffer.  json-streamer.c
can allocate memory for each token instead of relying on reference
counting of QStrings.

Signed-off-by: Paolo Bonzini 
Message-Id: <1448300659-23559-2-git-send-email-pbonz...@redhat.com>
[Straightforwardly rebased on my patches, checkpatch made happy]
Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
---
 include/qapi/qmp/json-lexer.h|  8 
 include/qapi/qmp/json-streamer.h |  1 +
 qobject/json-lexer.c | 22 --
 qobject/json-streamer.c  |  9 +
 4 files changed, 18 insertions(+), 22 deletions(-)

diff --git a/include/qapi/qmp/json-lexer.h b/include/qapi/qmp/json-lexer.h
index f3e8dc7..cb456d5 100644
--- a/include/qapi/qmp/json-lexer.h
+++ b/include/qapi/qmp/json-lexer.h
@@ -14,8 +14,7 @@
 #ifndef QEMU_JSON_LEXER_H
 #define QEMU_JSON_LEXER_H
 
-#include "qapi/qmp/qstring.h"
-#include "qapi/qmp/qlist.h"
+#include "glib-compat.h"
 
 typedef enum json_token_type {
 JSON_MIN = 100,
@@ -36,13 +35,14 @@ typedef enum json_token_type {
 
 typedef struct JSONLexer JSONLexer;
 
-typedef void (JSONLexerEmitter)(JSONLexer *, QString *, JSONTokenType, int x, 
int y);
+typedef void (JSONLexerEmitter)(JSONLexer *, GString *,
+JSONTokenType, int x, int y);
 
 struct JSONLexer
 {
 JSONLexerEmitter *emit;
 int state;
-QString *token;
+GString *token;
 int x, y;
 };
 
diff --git a/include/qapi/qmp/json-streamer.h b/include/qapi/qmp/json-streamer.h
index 823f7d7..e901144 100644
--- a/include/qapi/qmp/json-streamer.h
+++ b/include/qapi/qmp/json-streamer.h
@@ -14,6 +14,7 @@
 #ifndef QEMU_JSON_STREAMER_H
 #define QEMU_JSON_STREAMER_H
 
+#include 
 #include "qapi/qmp/qlist.h"
 #include "qapi/qmp/json-lexer.h"
 
diff --git a/qobject/json-lexer.c b/qobject/json-lexer.c
index 1df7d5e..92798ae 100644
--- a/qobject/json-lexer.c
+++ b/qobject/json-lexer.c
@@ -11,12 +11,9 @@
  *
  */
 
-#include "qapi/qmp/qstring.h"
-#include "qapi/qmp/qlist.h"
-#include "qapi/qmp/qdict.h"
-#include "qapi/qmp/qint.h"
 #include "qemu-common.h"
 #include "qapi/qmp/json-lexer.h"
+#include 
 
 #define MAX_TOKEN_SIZE (64ULL << 20)
 
@@ -276,7 +273,7 @@ void json_lexer_init(JSONLexer *lexer, JSONLexerEmitter 
func)
 {
 lexer->emit = func;
 lexer->state = IN_START;
-lexer->token = qstring_new();
+lexer->token = g_string_sized_new(3);
 lexer->x = lexer->y = 0;
 }
 
@@ -295,7 +292,7 @@ static int json_lexer_feed_char(JSONLexer *lexer, char ch, 
bool flush)
 new_state = json_lexer[lexer->state][(uint8_t)ch];
 char_consumed = !TERMINAL_NEEDED_LOOKAHEAD(lexer->state, new_state);
 if (char_consumed) {
-qstring_append_chr(lexer->token, ch);
+g_string_append_c(lexer->token, ch);
 }
 
 switch (new_state) {
@@ -313,8 +310,7 @@ static int json_lexer_feed_char(JSONLexer *lexer, char ch, 
bool flush)
 lexer->emit(lexer, lexer->token, new_state, lexer->x, lexer->y);
 /* fall through */
 case JSON_SKIP:
-QDECREF(lexer->token);
-lexer->token = qstring_new();
+g_string_truncate(lexer->token, 0);
 new_state = IN_START;
 break;
 case IN_ERROR:
@@ -332,8 +328,7 @@ static int json_lexer_feed_char(JSONLexer *lexer, char ch, 
bool flush)
  * induce an error/flush state.
  */
 lexer->emit(lexer, lexer->token, JSON_ERROR, lexer->x, lexer->y);
-QDECREF(lexer->token);
-lexer->token = qstring_new();
+g_string_truncate(lexer->token, 0);
 new_state = IN_START;
 lexer->state = new_state;
 return 0;
@@ -346,10 +341,9 @@ static int json_lexer_feed_char(JSONLexer *lexer, char ch, 
bool flush)
 /* Do not let a single token grow to an arbitrarily large size,
  * this is a security consideration.
  */
-if (lexer->token->length > MAX_TOKEN_SIZE) {
+if (lexer->token->len > MAX_TOKEN_SIZE) {
 lexer->emit(lexer, lexer->token, lexer->state, lexer->x, lexer->y);
-QDECREF(lexer->token);
-lexer->token = qstring_new();
+g_string_truncate(lexer->token, 0);
 lexer->state = IN_START;
 }
 
@@ -379,5 +373,5 @@ int json_lexer_flush(JSONLexer *lexer)
 
 void json_lexer_destroy(JSONLexer *lexer)
 {
-QDECREF(lexer->token);
+g_string_free(lexer->token, true);
 }
diff --git a/qobject/json-streamer.c b/qobject/json-streamer.c
index 4a161a1..7292f3a 100644
--- a/qobject/json-streamer.c
+++ b/qobject/json-streamer.c
@@ -12,6 +12,7 @@
  */
 
 #include "qapi/qmp/qlist.h"
+#include "qapi/qmp/qstring.h"
 #include "qapi/qmp/qint.h"
 #include "qapi/qmp/qdict.h"
 #include "qemu-common.h"
@@ -21,7 +22,8 @@
 #define MAX_TOKEN_SIZE (64ULL << 20)
 #define MAX_NESTING (1ULL << 10)
 
-static void json_message_process_token(JSONLexer *lexer, QString *token, 
JSONTokenTyp

Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25)

[Paolo Bonzini]

On 26/11/2015 12:28, Peter Maydell wrote:
>> But we are relying on them, and thus we should document them.  Witness
>> the number of patches fixing so called "undefined" behavior.  And those
>> patches are _dangerous_.
> 
> Until and unless the compiler guarantees us the semantics that
> we want, then silently hoping we get something we're not getting
> is even more dangerous, and avoiding the UB is better than
> just crossing our fingers and hoping.
> 
>> I can certainly remove the "as documented by the GCC manual" part and
>> the -fwrapv setting, but silencing -Wshift-negative-value and
>> documenting what we rely on should go in.
> 
> I don't see much point in documenting what we rely on
> if we can't rely on it and need to stop relying on it.

I'm having a hard, hard time believing that we can't rely on it.  And if
we can rely on it, we don't need to stop relying on it.

To sum up:

- GCC promises that signed shift of << is implementation-defined except
for overflow in constant expressions, and defines it as operating on bit
values.  This was approved.  For GCC, -fwrapv does not even apply except
again for constant expressions.

- signed shift of negative values in constant expressions _are_ covered
by GCC's promise.  The implementation-defined behavior of signed <<
gives a meaning to all signed shifts, and all that the C standard
requires is "Each constant expression shall evaluate to a constant that
is in the range of representable values for its type" (6.6p4).
Therefore we should at least disable -Wshift-negative-value, because it
doesn't buy us anything.

- regarding overflow, in addition to the weird -Wpedantic warning, GCC 6
adds a new -Wshift-overflow flag which is enabled by default in C99 and
C11 modes, and which only applies to constant expressions.  So the
remaining case where the compiler may change its behavior on overflow,
i.e. constant expressions, will thus be caught by our usage of -Werror
(because -Wshift-overflow is enabled by default).  So, independent of
the limited scope of GCC's promise, with GCC 6 we will never have
constant expressions that overflow because of left shifts.

- if a compiler actually treated signed << overflow undefined behavior,
a possible fix would be to use C89 mode (-std=gnu89), since signed << is
unspecified there rather than undefined.  With C89, GCC's promise is
complete.  We do use C89 with GCC <= 4.9 anyway, it makes sense to be
homogeneous across all supported compilers.

Now, -fwrapv was really included in my patch only to silence ubsan in
the future.  I think it should, and I will work on patches to fix that.
 However, an advantage of -std=gnu89 is that it silences ubsan _now_ at
least for GCC.  So let's just drop -fwrapv and use -std=gnu89 instead.
This buys us time, and in the meanwhile I'll gett -fwrapv complete in GCC.

If this is okay, I'll remove the patch, respin the pull request, and
post a new configure change to use -std=gnu89.

Yes, we haven't heard anything from clang developers.  But clang does
not even document implementation-defined behavior
(https://llvm.org/bugs/show_bug.cgi?id=11272).  The bug says:

> The clang documentation should specify how clang behaves in cases
> specified to be implementation-defined in the relevant standards.
> Perhaps simply saying that our behavior is the same as GCC's would suffice?

This is about implementation-defined rather than undefined behavior, but
I think it's enough to not care about clang developer's silence.

Paolo

Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25)

[Paolo Bonzini]

On 26/11/2015 13:19, Peter Maydell wrote:
> On 26 November 2015 at 12:15, Markus Armbruster  wrote:
>> Peter Maydell  writes:
>>> I don't see much point in documenting what we rely on
>>> if we can't rely on it and need to stop relying on it.
>>
>> "Can't" and "need" are too strong.  The kernel can, and I fail to see
>> what makes us so special that we absolutely cannot.
> 
> The kernel has the luxury of being able to say "we only compile
> with gcc".

Actually no, there are people interested in compiling it with clang
(mostly because of GPL FUD and LLVM koolaid, but that's secondary in
this context).

>> For what it's worth, I'm sick and tired of patches "fixing" signed
>> shifts, and the unnecessary risk that comes with them.
> 
> Me too.

Great, that's a start.  And I'm totally not sarcastic about this.

Paolo

> I just want us to fix this by getting the compiler authors
> to document that we can rely on this stuff, not just by silencing
> warnings in QEMU's makefiles.

Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25)

[Peter Maydell]

On 26 November 2015 at 12:15, Markus Armbruster  wrote:
> Peter Maydell  writes:
>> I don't see much point in documenting what we rely on
>> if we can't rely on it and need to stop relying on it.
>
> "Can't" and "need" are too strong.  The kernel can, and I fail to see
> what makes us so special that we absolutely cannot.

The kernel has the luxury of being able to say "we only compile
with gcc".

> For what it's worth, I'm sick and tired of patches "fixing" signed
> shifts, and the unnecessary risk that comes with them.

Me too. I just want us to fix this by getting the compiler authors
to document that we can rely on this stuff, not just by silencing
warnings in QEMU's makefiles.

thanks
-- PMM

Re: [Qemu-devel] [RESEND RFC 4/6] device_tree: introduce qemu_fdt_getprop_optional

[Alex Bennée]

Eric Auger  writes:

> Current qemu_fdt_getprop exits if the property is not found. It is
> sometimes needed to read an optional property, in which case we do
> not wish to exit but simply returns a null value.
>
> This is what this new qemu_fdt_getprop_optional function does.
>
> Signed-off-by: Eric Auger 

Reviewed-by: Alex Bennée 

> ---
>  device_tree.c| 17 +
>  include/sysemu/device_tree.h |  2 ++
>  2 files changed, 19 insertions(+)
>
> diff --git a/device_tree.c b/device_tree.c
> index f184e3c..a318683 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -280,6 +280,23 @@ const void *qemu_fdt_getprop(void *fdt, const char 
> *node_path,
>  return r;
>  }
>
> +const void *qemu_fdt_getprop_optional(void *fdt, const char *node_path,
> + const char *property, bool optional, int *lenp)
> +{
> +int len;
> +const void *r;
> +if (!lenp) {
> +lenp = &len;
> +}
> +r = fdt_getprop(fdt, findnode_nofail(fdt, node_path), property, lenp);
> +if (!r && !optional) {
> +error_report("%s: Couldn't get %s/%s: %s", __func__,
> + node_path, property, fdt_strerror(*lenp));
> +exit(1);
> +}
> +return r;
> +}
> +
>  uint32_t qemu_fdt_getprop_cell(void *fdt, const char *node_path,
> const char *property)
>  {
> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
> index f9e6e6e..10cbe8e 100644
> --- a/include/sysemu/device_tree.h
> +++ b/include/sysemu/device_tree.h
> @@ -34,6 +34,8 @@ int qemu_fdt_setprop_phandle(void *fdt, const char 
> *node_path,
>   const char *target_node_path);
>  const void *qemu_fdt_getprop(void *fdt, const char *node_path,
>   const char *property, int *lenp);
> +const void *qemu_fdt_getprop_optional(void *fdt, const char *node_path,
> + const char *property, bool optional, int *lenp);
>  uint32_t qemu_fdt_getprop_cell(void *fdt, const char *node_path,
> const char *property);
>  uint32_t qemu_fdt_get_phandle(void *fdt, const char *path);


--
Alex Bennée

[Qemu-devel] [PATCH] vhost-user-test: fix migration overlap test

[Michael S. Tsirkin]

During migration, source does GET_BASE, destination does SET_BASE.
Use that as opposed to fds being configured to detect
vhost user running on both source and destination.

Signed-off-by: Michael S. Tsirkin 
---
 tests/vhost-user-test.c | 13 -
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index 78aac92..4975b9b 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -131,6 +131,7 @@ typedef struct TestServer {
 GMutex data_mutex;
 GCond data_cond;
 int log_fd;
+uint64_t rings;
 } TestServer;
 
 #if !GLIB_CHECK_VERSION(2, 32, 0)
@@ -279,6 +280,9 @@ static void chr_read(void *opaque, const uint8_t *buf, int 
size)
 msg.payload.state.num = 0;
 p = (uint8_t *) &msg;
 qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size);
+
+assert(msg.payload.state.index < 2);
+s->rings &= ~(0x1ULL << msg.payload.state.index);
 break;
 
 case VHOST_USER_SET_MEM_TABLE:
@@ -316,10 +320,9 @@ static void chr_read(void *opaque, const uint8_t *buf, int 
size)
 g_cond_signal(&s->data_cond);
 break;
 
-case VHOST_USER_SET_VRING_ENABLE:
-if (!msg.payload.state.num) {
-s->fds_num = 0;
-}
+case VHOST_USER_SET_VRING_BASE:
+assert(msg.payload.state.index < 2);
+s->rings |= 0x1ULL << msg.payload.state.index;
 break;
 
 default:
@@ -486,7 +489,7 @@ static gboolean
 test_migrate_source_check(GSource *source)
 {
 TestMigrateSource *t = (TestMigrateSource *)source;
-gboolean overlap = t->src->fds_num > 0 && t->dest->fds_num > 0;
+gboolean overlap = t->src->rings && t->dest->rings;
 
 g_assert(!overlap);
 
-- 
MST

[Qemu-devel] [PATCH] vhost-user-test: fix migration overlap test

[Michael S. Tsirkin]

During migration, source does GET_BASE, destination does SET_BASE.
Use that as opposed to fds being configured to detect
vhost user running on both source and destination.

Signed-off-by: Michael S. Tsirkin 
---
 tests/vhost-user-test.c | 13 -
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index 78aac92..4975b9b 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -131,6 +131,7 @@ typedef struct TestServer {
 GMutex data_mutex;
 GCond data_cond;
 int log_fd;
+uint64_t rings;
 } TestServer;
 
 #if !GLIB_CHECK_VERSION(2, 32, 0)
@@ -279,6 +280,9 @@ static void chr_read(void *opaque, const uint8_t *buf, int 
size)
 msg.payload.state.num = 0;
 p = (uint8_t *) &msg;
 qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size);
+
+assert(msg.payload.state.index < 2);
+s->rings &= ~(0x1ULL << msg.payload.state.index);
 break;
 
 case VHOST_USER_SET_MEM_TABLE:
@@ -316,10 +320,9 @@ static void chr_read(void *opaque, const uint8_t *buf, int 
size)
 g_cond_signal(&s->data_cond);
 break;
 
-case VHOST_USER_SET_VRING_ENABLE:
-if (!msg.payload.state.num) {
-s->fds_num = 0;
-}
+case VHOST_USER_SET_VRING_BASE:
+assert(msg.payload.state.index < 2);
+s->rings |= 0x1ULL << msg.payload.state.index;
 break;
 
 default:
@@ -486,7 +489,7 @@ static gboolean
 test_migrate_source_check(GSource *source)
 {
 TestMigrateSource *t = (TestMigrateSource *)source;
-gboolean overlap = t->src->fds_num > 0 && t->dest->fds_num > 0;
+gboolean overlap = t->src->rings && t->dest->rings;
 
 g_assert(!overlap);
 
-- 
MST

Re: [Qemu-devel] [PATCH v6 1/3] target-i386: fallback vcpu's TSC rate to value returned by KVM

[Eduardo Habkost]

On Tue, Nov 24, 2015 at 11:33:55AM +0800, Haozhong Zhang wrote:
> If no user-specified TSC rate is present, we will try to set
> env->tsc_khz to the value returned by KVM_GET_TSC_KHZ. This patch does
> not change the current functionality of QEMU and just prepares for later
> patches to enable migrating vcpu's TSC rate.
> 
> Signed-off-by: Haozhong Zhang 

Reviewed-by: Eduardo Habkost 

> ---
>  target-i386/kvm.c | 14 ++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 2a9953b..a0fe9d4 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -832,6 +832,20 @@ int kvm_arch_init_vcpu(CPUState *cs)
>  }
>  }
>  
> +/* vcpu's TSC frequency is either specified by user, or following
> + * the value used by KVM if the former is not present. In the
> + * latter case, we query it from KVM and record in env->tsc_khz,
> + * so that vcpu's TSC frequency can be migrated later via this field.
> + */
> +if (!env->tsc_khz) {
> +r = kvm_check_extension(cs->kvm_state, KVM_CAP_GET_TSC_KHZ) ?
> +kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ) :
> +-ENOTSUP;
> +if (r > 0) {
> +env->tsc_khz = r;
> +}
> +}
> +
>  if (has_xsave) {
>  env->kvm_xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave));
>  }
> -- 
> 2.4.8
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v6 2/3] target-i386: reorganize TSC rate setting code

[Eduardo Habkost]

On Tue, Nov 24, 2015 at 11:33:56AM +0800, Haozhong Zhang wrote:
> Following changes are made to the TSC rate setting code in
> kvm_arch_init_vcpu():
>  * The code is moved to a new function kvm_arch_set_tsc_khz().
>  * If kvm_arch_set_tsc_khz() fails, i.e. following two conditions are
>both satisfied:
>* KVM does not support the TSC scaling or it fails to set vcpu's
>  TSC rate by KVM_SET_TSC_KHZ,
>* the TSC rate to be set is different than the value currently used
>  by KVM,
>then kvm_arch_init_vcpu() will fail. Prevously,
>* the lack of TSC scaling never failed kvm_arch_init_vcpu(),
>* the failure of KVM_SET_TSC_KHZ failed kvm_arch_init_vcpu()
>  unconditionally, even though the TSC rate to be set is identical
>  to the value currently used by KVM.
> 
> Signed-off-by: Haozhong Zhang 

Reviewed-by: Eduardo Habkost 

> ---
>  target-i386/kvm.c | 40 +---
>  1 file changed, 33 insertions(+), 7 deletions(-)
> 
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index a0fe9d4..1e811ee 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -524,6 +524,36 @@ static bool hyperv_enabled(X86CPU *cpu)
>  cpu->hyperv_runtime);
>  }
>  
> +static int kvm_arch_set_tsc_khz(CPUState *cs)
> +{
> +X86CPU *cpu = X86_CPU(cs);
> +CPUX86State *env = &cpu->env;
> +int r;
> +
> +if (!env->tsc_khz) {
> +return 0;
> +}
> +
> +r = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL) ?
> +kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz) :
> +-ENOTSUP;
> +if (r < 0) {
> +/* When KVM_SET_TSC_KHZ fails, it's an error only if the current
> + * TSC frequency doesn't match the one we want.
> + */
> +int cur_freq = kvm_check_extension(cs->kvm_state, 
> KVM_CAP_GET_TSC_KHZ) ?
> +   kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ) :
> +   -ENOTSUP;
> +if (cur_freq <= 0 || cur_freq != env->tsc_khz) {
> +error_report("warning: TSC frequency mismatch between "
> + "VM and host, and TSC scaling unavailable");
> +return r;
> +}
> +}
> +
> +return 0;
> +}
> +
>  static Error *invtsc_mig_blocker;
>  
>  #define KVM_MAX_CPUID_ENTRIES  100
> @@ -823,13 +853,9 @@ int kvm_arch_init_vcpu(CPUState *cs)
>  return r;
>  }
>  
> -r = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL);
> -if (r && env->tsc_khz) {
> -r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz);
> -if (r < 0) {
> -fprintf(stderr, "KVM_SET_TSC_KHZ failed\n");
> -return r;
> -}
> +r = kvm_arch_set_tsc_khz(cs);
> +if (r < 0) {
> +return r;
>  }
>  
>  /* vcpu's TSC frequency is either specified by user, or following
> -- 
> 2.4.8
> 

-- 
Eduardo

Re: [Qemu-devel] poor virtio-scsi performance (fio testing)

[Alexandre DERUMIER]

>>May be my default cfq slowdown?

Yes ! (in your first mail you said that you use deadline scheduler ?)

cfq don't play well with a lof of current job.

cfq + numjobs=10 : 1 iops
cfq + numjobs=1 : 25000 iops
deadline + numjobs=1 : 25000 iops
deadline + numjobs=10 : 25000 iops


- Mail original -
De: "Vasiliy Tolstov" 
À: "aderumier" 
Cc: "qemu-devel" 
Envoyé: Mercredi 25 Novembre 2015 11:48:11
Objet: Re: [Qemu-devel] poor virtio-scsi performance (fio testing)

2015-11-25 13:27 GMT+03:00 Alexandre DERUMIER : 
> I have tested with a raw file, qemu 2.4 + virtio-scsi (without iothread), I'm 
> around 25k iops 
> with an intel ssd 3500. (host cpu are xeon v3 3,1ghz) 


What scheduler you have on host system? May be my default cfq slowdown? 

-- 
Vasiliy Tolstov, 
e-mail: v.tols...@selfip.ru 

Re: [Qemu-devel] [PATCH] Fix memory leak on error

[Igor Mammedov]

On Thu, 26 Nov 2015 12:00:12 +
Stefano Dong (董兴水)  wrote:

> hw/ppc/spapr.c: Fix memory leak on error, it was introduced in bc09e0611
> hw/acpi/memory_hotplug.c: Fix memory leak on error, it was introduced in 
> 34f2af3d
> 
> Signed-off-by: Stefano Dong (董兴水) 
Reviewed-by: Igor Mammedov 


> ---
>  hw/acpi/memory_hotplug.c | 1 +
>  hw/ppc/spapr.c   | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index ce428df..e4b9a01 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -155,6 +155,7 @@ static void acpi_memory_hotplug_write(void *opaque, 
> hwaddr addr, uint64_t data,
>  qapi_event_send_mem_unplug_error(dev->id,
>   error_get_pretty(local_err),
>   &error_abort);
> +error_free(local_err);
>  break;
>  }
>  trace_mhp_acpi_pc_dimm_deleted(mem_st->selector);
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 030ee35..3bb8bcd 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -125,6 +125,7 @@ static XICSState *xics_system_init(MachineState *machine,
>  error_report("kernel_irqchip requested but unavailable: %s",
>   error_get_pretty(err));
>  }
> +error_free(err);
>  }
>  
>  if (!icp) {

Re: [Qemu-devel] [PATCH v6 3/3] target-i386: add support to migrate vcpu's TSC rate

[Eduardo Habkost]

On Tue, Nov 24, 2015 at 11:33:57AM +0800, Haozhong Zhang wrote:
> This patch enables migrating vcpu's TSC rate. If KVM on the destination
> machine supports TSC scaling, guest programs will observe a consistent
> TSC rate across the migration.
> 
> If TSC scaling is not supported on the destination machine, the
> migration will not be aborted and QEMU on the destination will not set
> vcpu's TSC rate to the migrated value.
> 
> If vcpu's TSC rate specified by CPU option 'tsc-freq' on the destination
> machine is inconsistent with the migrated TSC rate, the migration will
> be aborted.
> 
> For backwards compatibility, the migration of vcpu's TSC rate is
> disabled on pc-*-2.4 and older machine types.
> 
> Signed-off-by: Haozhong Zhang 

Assuming the PC compat code will be moved to
pc_*_2_5_machine_options(), because the patch will be included
after QEMU 2.5.0:

Reviewed-by: Eduardo Habkost 

One comment below:

> ---
[...]
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 1e811ee..2a0fd54 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -2381,6 +2381,28 @@ int kvm_arch_put_registers(CPUState *cpu, int level)
>  }
>  }
>  
> +if (level == KVM_PUT_FULL_STATE) {
> +/* kvm_arch_set_tsc_khz() below can be called in two control flows 
> and
> + * we don't need to handle its errors in both of them.
> + *
> + * One is the control flow that creates a vcpu, where
> + * kvm_arch_set_tsc_khz() has already been called once before by
> + * kvm_arch_init_vcpu(). The latter will abort the control flow if 
> there
> + * are any errors of kvm_arch_set_tsc_khz(). Thus, in this control 
> flow,
> + * kvm_arch_set_tsc_khz() below never fails and we can safely ignore 
> its
> + * return values here.
> + *
> + * Another is the control flow of migration that sets vcpu's TSC
> + * frequency on the destination. The only error that can fail the
> + * migration is the mismatch between the migrated and the 
> user-specified
> + * TSC frequencies, which has been handled by cpu_post_load(). Other
> + * errors, i.e. those from kvm_arch_set_tsc_khz(), never fail the
> + * migration, so we also safely ignore its return values in this 
> control
> + * flow.
> + */

This could be more succint. Something like:

/* We don't check for kvm_arch_set_tsc_khz() errors here, because
 * TSC frequency mismatch shouldn't abort migration, unless the
 * user explicitly asked for a more strict TSC setting (e.g.
 * using an explicit "tsc-freq" option).
 */

No need to resubmit because of that, though. The comment can be
changed when applying the patch.

> +kvm_arch_set_tsc_khz(cpu);
> +}
> +
>  ret = kvm_getput_regs(x86_cpu, 1);
>  if (ret < 0) {
>  return ret;
> diff --git a/target-i386/machine.c b/target-i386/machine.c
> index a18e16e..e560ca3 100644
> --- a/target-i386/machine.c
> +++ b/target-i386/machine.c
> @@ -6,6 +6,8 @@
>  #include "cpu.h"
>  #include "sysemu/kvm.h"
>  
> +#include "qemu/error-report.h"
> +
>  static const VMStateDescription vmstate_segment = {
>  .name = "segment",
>  .version_id = 1,
> @@ -331,6 +333,13 @@ static int cpu_post_load(void *opaque, int version_id)
>  CPUX86State *env = &cpu->env;
>  int i;
>  
> +if (env->tsc_khz && env->user_tsc_khz &&
> +env->tsc_khz != env->user_tsc_khz) {
> +error_report("Mismatch between user-specified TSC frequency and "
> + "migrated TSC frequency");
> +return -EINVAL;
> +}
> +
>  /*
>   * Real mode guest segments register DPL should be zero.
>   * Older KVM version were setting it wrongly.
> @@ -775,6 +784,26 @@ static const VMStateDescription vmstate_xss = {
>  }
>  };
>  
> +static bool tsc_khz_needed(void *opaque)
> +{
> +X86CPU *cpu = opaque;
> +CPUX86State *env = &cpu->env;
> +MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> +PCMachineClass *pcmc = PC_MACHINE_CLASS(mc);
> +return env->tsc_khz && pcmc->save_tsc_khz;
> +}
> +
> +static const VMStateDescription vmstate_tsc_khz = {
> +.name = "cpu/tsc_khz",
> +.version_id = 1,
> +.minimum_version_id = 1,
> +.needed = tsc_khz_needed,
> +.fields = (VMStateField[]) {
> +VMSTATE_INT64(env.tsc_khz, X86CPU),
> +VMSTATE_END_OF_LIST()
> +}
> +};
> +
>  VMStateDescription vmstate_x86_cpu = {
>  .name = "cpu",
>  .version_id = 12,
> @@ -895,6 +924,7 @@ VMStateDescription vmstate_x86_cpu = {
>  &vmstate_msr_hyperv_runtime,
>  &vmstate_avx512,
>  &vmstate_xss,
> +&vmstate_tsc_khz,
>  NULL
>  }
>  };
> -- 
> 2.4.8
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v13 01/14] qobject: Simplify QObject

[Markus Armbruster]

Eric Blake  writes:

> The QObject hierarchy is small enough, and unlikely to grow further
> (since we only use it to map to JSON and already cover all JSON
> types), that we can simplify things by not tracking a separate
> vtable, but just inline the refcnt element of the vtable QType
> directly into QObject, and track a separate array of destroy
> functions.  We can drop qnull_destroy_obj() in the process.
>
> The remaining QObject subclasses must export their destructor.
>
> This also has the nice benefit of moving the typename 'QType'
> out of the way, so that the next patch can repurpose it for a
> nicer name for 'qtype_code'.
>
> The various objects are still the same size (so no change in cache
> line pressure), but now have less indirection (although I didn't
> bother benchmarking to see if there is a noticeable speedup, as
> we don't have hard evidence that this was in a performance hotspot
> in the first place).
>
> Suggested-by: Markus Armbruster 
> Signed-off-by: Eric Blake 
[...]
> diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
> index 4b96ed5..7e5b9b1 100644
> --- a/include/qapi/qmp/qobject.h
> +++ b/include/qapi/qmp/qobject.h
> @@ -47,21 +47,20 @@ typedef enum {
>  QTYPE_MAX,
>  } qtype_code;
>
> -struct QObject;
> -
> -typedef struct QType {
> -qtype_code code;
> -void (*destroy)(struct QObject *);
> -} QType;
> -
>  typedef struct QObject {
> -const QType *type;
> +qtype_code type;
>  size_t refcnt;
>  } QObject;

Future opportunity: uint32_t refcnt should be more than enough.  Halves
the size to eight bytes.

> +typedef void (*QDestroy)(QObject *);
> +

Typedef used exactly once, in qobject.c.  Let's drop it.

>  /* Get the 'base' part of an object */
>  #define QOBJECT(obj) (&(obj)->base)
>
> +/* High-level interface for qobject_init() */
> +#define QOBJECT_INIT(obj, type) \
> +qobject_init(QOBJECT(obj), type)
> +

Not really high-level; it's used only internally, and its users are the
high-level constructors.

I wouldn't mind dropping the macro.

>  /* High-level interface for qobject_incref() */
>  #define QINCREF(obj)  \
>  qobject_incref(QOBJECT(obj))
> @@ -71,9 +70,12 @@ typedef struct QObject {
>  qobject_decref(obj ? QOBJECT(obj) : NULL)
>
>  /* Initialize an object to default values */
> -#define QOBJECT_INIT(obj, qtype_type)   \
> -obj->base.refcnt = 1;   \
> -obj->base.type   = qtype_type
> +static inline void qobject_init(QObject *obj, qtype_code type)
> +{
> +assert(type);

QTYPE_NONE < type && type < QTYPE_MAX would be tighter.

Aside: I dislike our use of _MAX as the last enum.  _MAX should be the
largest *valid* value (see: INT_MAX), not one more.

> +obj->refcnt = 1;
> +obj->type = type;
> +}
>
>  /**
>   * qobject_incref(): Increment QObject's reference count
> @@ -85,6 +87,11 @@ static inline void qobject_incref(QObject *obj)
>  }
>
>  /**
> + * qobject_destroy(): Free resources used by the object
> + */
> +void qobject_destroy(QObject *obj);
> +
> +/**
>   * qobject_decref(): Decrement QObject's reference count, deallocate
>   * when it reaches zero
>   */
> @@ -92,9 +99,7 @@ static inline void qobject_decref(QObject *obj)
>  {
>  assert(!obj || obj->refcnt);
>  if (obj && --obj->refcnt == 0) {
> -assert(obj->type != NULL);
> -assert(obj->type->destroy != NULL);
> -obj->type->destroy(obj);
> +qobject_destroy(obj);
>  }
>  }
>
> @@ -103,8 +108,8 @@ static inline void qobject_decref(QObject *obj)
>   */
>  static inline qtype_code qobject_type(const QObject *obj)
>  {
> -assert(obj->type != NULL);
> -return obj->type->code;
> +assert(obj->type);

Likewise.

> +return obj->type;
>  }
>
>  extern QObject qnull_;
> diff --git a/include/qapi/qmp/qstring.h b/include/qapi/qmp/qstring.h
> index 34675a7..df7df55 100644
> --- a/include/qapi/qmp/qstring.h
> +++ b/include/qapi/qmp/qstring.h
> @@ -32,5 +32,6 @@ void qstring_append_int(QString *qstring, int64_t value);
>  void qstring_append(QString *qstring, const char *str);
>  void qstring_append_chr(QString *qstring, int c);
>  QString *qobject_to_qstring(const QObject *obj);
> +void qstring_destroy_obj(QObject *obj);
>
>  #endif /* QSTRING_H */
> diff --git a/qobject/Makefile.objs b/qobject/Makefile.objs
> index 0031e8b..bed5508 100644
> --- a/qobject/Makefile.objs
> +++ b/qobject/Makefile.objs
> @@ -1,2 +1,2 @@
>  util-obj-y = qnull.o qint.o qstring.o qdict.o qlist.o qfloat.o qbool.o
> -util-obj-y += qjson.o json-lexer.o json-streamer.o json-parser.o
> +util-obj-y += qjson.o qobject.o json-lexer.o json-streamer.o json-parser.o
> diff --git a/qobject/qbool.c b/qobject/qbool.c
> index bc6535f..895fd4d 100644
> --- a/qobject/qbool.c
> +++ b/qobject/qbool.c
> @@ -15,13 +15,6 @@
>  #include "qapi/qmp/qobject.h"
>  #include "qemu-common.h"
>
> -static void qbool_destroy_obj(QObject *obj);
> -
> -static const QType qbool_type = {
> -.code = QTYPE_QBOOL,
> -.destroy = qbool_de

Re: [Qemu-devel] [PATCH v2] qemu_mutex_iothread_locked not correctly synchronized

[David Engraf]

Am 26.11.2015 um 12:25 schrieb Stefan Weil:

Am 26.11.2015 um 10:12 schrieb David Engraf:

Am 25.11.2015 um 17:16 schrieb Paolo Bonzini:



On 25/11/2015 16:48, David Engraf wrote:


Indeed, TLS handling is broken. The address of iothread_locked is
always
the same between threads and I can see that a different thread sets
iothread_locked to false, thus my current thread uses an invalid state.
I will have to check why my compiler produces invalid TLS code.


That rings a bell, I think there are different CRT libraries or
something like that.  Stefan, what would break TLS under Windows?


"--extra-cflags=-mthreads" is the solution. iothread_locked is unique
for each thread now. I saw that this is already mentioned in the
Documentation [1], but why isn't that enabled by default if QEMU
requires this option on Windows? Without this option, even the latest
version of gcc doesn't produce working TLS code.

Many thanks for your support!

David

[1] http://wiki.qemu.org/Hosts/W32


Hi David,

I just prepared a patch which will enable that option by default
for all compilations targeting Windows.

Thanks for your report!


Thank you.

David

Re: [Qemu-devel] [PATCH v2] ui/cocoa.m: Prevent activation clicks from going to guest

[Programmingkid]

On Nov 26, 2015, at 6:45 AM, Peter Maydell wrote:

> On 26 November 2015 at 01:14, Programmingkid  
> wrote:
>> When QEMU is brought to the foreground, the click event that activates QEMU
>> should not go to the guest. Accidents happen when they do go to the guest
>> without giving the user a change to handle them. Buttons are clicked 
>> accidently.
>> Windows are closed accidently. Volumes are unmounted accidently. This patch
>> prevents these accidents from happening.
>> 
>> Signed-off-by: John Arbuckle 
>> 
>> ---
>> Added code that handles the right mouse button and the other mouse button.
> 
> This seems like a fair bit of repeated code. Does the change
> below do the right thing?
YES! Excellent job with this one. It's very compact. 

> I think it ought to work but I don't have
> any guests handy which use the mouse to check with.

Really? This Debian distro should fix that: 
http://cdimage.debian.org/cdimage/archive/5.0.10/powerpc/iso-cd/

> 
> diff --git a/ui/cocoa.m b/ui/cocoa.m
> index 1554331..d76b942 100644
> --- a/ui/cocoa.m
> +++ b/ui/cocoa.m
> @@ -724,7 +724,15 @@ QemuCocoaView *cocoaView;
> }
> 
> if (mouse_event) {
> -if (last_buttons != buttons) {
> +/* Don't send button events to the guest unless we've got a
> + * mouse grab or window focus. If we have neither then this event
> + * is the user clicking on the background window to activate and
> + * bring us to the front, which will be done by the sendEvent
> + * call below. We definitely don't want to pass that click through
> + * to the guest.
> + */
> +if ((isMouseGrabbed || [[self window] isKeyWindow]) &&
> +(last_buttons != buttons)) {
> static uint32_t bmap[INPUT_BUTTON_MAX] = {
> [INPUT_BUTTON_LEFT]   = MOUSE_EVENT_LBUTTON,
> [INPUT_BUTTON_MIDDLE] = MOUSE_EVENT_MBUTTON,
> 
> 
> 
> (if this is the activation click then we will do the mousegrab
> on mouse-button-up so it's not necessary to do it on button-down,
> I think.)
> 
> thanks
> -- PMM

Reviewed-by: John Arbuckle 

Re: [Qemu-devel] [PATCH v1 6/7] kvm/x86: Hyper-V SynIC message slot pending clearing at SINT ack

[Paolo Bonzini]

On 26/11/2015 10:06, Andrey Smetanin wrote:
> 
> 
> On 11/25/2015 08:14 PM, Paolo Bonzini wrote:
>>
>>
>> On 25/11/2015 17:55, Andrey Smetanin wrote:

 +gpa = synic->msg_page & PAGE_MASK;
 +page = kvm_vcpu_gfn_to_page(vcpu, gpa >> PAGE_SHIFT);
 +if (is_error_page(page)) {
 +vcpu_err(vcpu, "Hyper-V SynIC can't get msg page, gpa
 0x%llx\n",
 + gpa);
 +return;
 +}
 +msg_page = kmap_atomic(page);
>>>
>>> But the message page is not being pinned, is it?
>>>
>>> Actually I don't know anything about pinning.
>>> Is it pinning against page swapping ?
>>
>> Yes.  Unless the page is pinned, kmap_atomic can fail.
> kmap_atomic() can't fail for a valid page struct. Does
> kvm_vcpu_gfn_to_page() can provide invalid page(swapped page) struct
> which may pass is_error_page(page) check but can leads to incorrect
> behavior inside kmap_atomic()?

No, you're right.  Nevermind, I was confused because I thought you
needed kmap_atomic rather than kmap.  Here using kmap_atomic is just an
optimization, so it's okay.  (If you needed kmap_atomic, the problem
would have been that kvm_vcpu_gfn_to_page() can sleep).

In patch 7/7 you're also not in atomic context, so kvm_vcpu_gfn_to_page
is okay.

Shouldn't have reviewed the patch when tired. :)

Then the patches look good, I think.  With a testcase I can try them out
and hopefully merge them for Linux 4.5 / QEMU 2.6.

Paolo

Re: [Qemu-devel] [PATCH v13 03/14] qapi: Convert QType into QAPI built-in enum type

[Markus Armbruster]

Eric Blake  writes:

> What's more meta than using qapi to define qapi? :)
>
> Convert QType into a full-fledged[*] builtin qapi enum type, so
> that a subsequent patch can then use it as the discriminator
> type of qapi alternate types.  Fortunately, the judicious use of
> 'prefix' in the qapi definition avoids churn to the spelling of
> the enum constants.
>
> To avoid circular definitions, we have to flip the order of
> inclusion between "qobject.h" vs. "qapi-types.h".  Back in commit
> 28770e0, we had the latter include the former, so that we could
> use 'QObject *' for our implementation of 'any'.  But that usage
> also works with only a forward declaration, whereas the
> definition of QObject requires QType to be a complete type.
>
> [*] The type has to be builtin, rather than declared in
> qapi/common.json, because we want to use it for alternates even
> when common.json is not included. But since it is the first
> builtin enum type, we have to add special cases to qapi-types
> and qapi-visit to only emit definitions once, even when two
> qapi files are being compiled into the same binary (the way we
> already handled builtin list types like 'intList').  We may
> need to revisit how multiple qapi files share common types,
> but that's a project for another day.
>
> Signed-off-by: Eric Blake 
>
> ---
> v13: change title, use typedefs.h, rebase to cleaner QObject,
> rebase to Markus' typedefs.h re-sort
> v12: split out preparatory renames, retitle patch, use info rather
> than name comparison
> v11: new patch
> ---
>  docs/qapi-code-gen.txt   |  1 +
>  include/qapi/qmp/qobject.h   | 17 +++--
>  include/qemu/typedefs.h  |  1 +
>  qobject/qobject.c|  4 ++--
>  scripts/qapi-types.py| 15 +++
>  scripts/qapi-visit.py| 11 +--
>  scripts/qapi.py  |  6 ++
>  tests/qapi-schema/alternate-empty.out|  2 ++
>  tests/qapi-schema/comments.out   |  2 ++
>  tests/qapi-schema/empty.out  |  2 ++
>  tests/qapi-schema/event-case.out |  2 ++
>  tests/qapi-schema/flat-union-empty.out   |  2 ++
>  tests/qapi-schema/ident-with-escape.out  |  2 ++
>  tests/qapi-schema/include-relpath.out|  2 ++
>  tests/qapi-schema/include-repetition.out |  2 ++
>  tests/qapi-schema/include-simple.out |  2 ++
>  tests/qapi-schema/indented-expr.out  |  2 ++
>  tests/qapi-schema/qapi-schema-test.out   |  2 ++
>  tests/qapi-schema/union-clash-data.out   |  2 ++
>  tests/qapi-schema/union-empty.out|  2 ++
>  20 files changed, 59 insertions(+), 22 deletions(-)
>
> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
> index 2becba9..79bf072 100644
> --- a/docs/qapi-code-gen.txt
> +++ b/docs/qapi-code-gen.txt
> @@ -160,6 +160,7 @@ The following types are predefined, and map to C as 
> follows:
> accepts size suffixes
>bool  bool   JSON true or false
>any   QObject *  any JSON value
> +  QType QType  JSON string matching enum QType values
>
>
>  === Includes ===
> diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
> index 525fb39..295aa76 100644
> --- a/include/qapi/qmp/qobject.h
> +++ b/include/qapi/qmp/qobject.h
> @@ -34,23 +34,12 @@
>
>  #include 
>  #include 
> +#include "qapi-types.h"

Needed for QType.  Risk for circular inclusion.  We're currently fine,
because generated qapi-types.h includes only "qemu/typedefs.h" (visible
below).  Should we add a comment to qapi-types.py?

>
> -typedef enum {
> -QTYPE_NONE,/* sentinel value, no QObject has this type code */
> -QTYPE_QNULL,
> -QTYPE_QINT,
> -QTYPE_QSTRING,
> -QTYPE_QDICT,
> -QTYPE_QLIST,
> -QTYPE_QFLOAT,
> -QTYPE_QBOOL,
> -QTYPE_MAX,
> -} QType;
> -
> -typedef struct QObject {
> +struct QObject {
>  QType type;
>  size_t refcnt;
> -} QObject;
> +};
>
>  typedef void (*QDestroy)(QObject *);
>
> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> index 3eedcf4..78fe6e8 100644
> --- a/include/qemu/typedefs.h
> +++ b/include/qemu/typedefs.h
> @@ -80,6 +80,7 @@ typedef struct QEMUSGList QEMUSGList;
>  typedef struct QEMUSizedBuffer QEMUSizedBuffer;
>  typedef struct QEMUTimer QEMUTimer;
>  typedef struct QEMUTimerListGroup QEMUTimerListGroup;
> +typedef struct QObject QObject;
>  typedef struct RAMBlock RAMBlock;
>  typedef struct Range Range;
>  typedef struct SerialState SerialState;
> diff --git a/qobject/qobject.c b/qobject/qobject.c
> index 5325447..d0eb7f1 100644
> --- a/qobject/qobject.c
> +++ b/qobject/qobject.c
> @@ -15,7 +15,7 @@
>  #include "qapi/qmp/qlist.h"
>  #include "qapi/qmp/qstring.h"
>
> -static QDestroy qdestroy[QTYPE_MAX] = {
> +static QDestroy qdestroy[QTYPE__MAX] = {
>  [QTYPE_NONE] = NULL, /* No such object exists */
>  [QTYPE_QNULL] = NULL, /* qnull_ is indestructible */
>  [QTYPE_QINT] = qint_destroy_obj,
> 

Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25)

[Peter Maydell]

On 26 November 2015 at 13:04, Paolo Bonzini  wrote:
>
>
> On 26/11/2015 12:28, Peter Maydell wrote:
>>> But we are relying on them, and thus we should document them.  Witness
>>> the number of patches fixing so called "undefined" behavior.  And those
>>> patches are _dangerous_.
>>
>> Until and unless the compiler guarantees us the semantics that
>> we want, then silently hoping we get something we're not getting
>> is even more dangerous, and avoiding the UB is better than
>> just crossing our fingers and hoping.
>>
>>> I can certainly remove the "as documented by the GCC manual" part and
>>> the -fwrapv setting, but silencing -Wshift-negative-value and
>>> documenting what we rely on should go in.
>>
>> I don't see much point in documenting what we rely on
>> if we can't rely on it and need to stop relying on it.
>
> I'm having a hard, hard time believing that we can't rely on it.  And if
> we can rely on it, we don't need to stop relying on it.

Really my concern here is simply an ordering one: we should
(1) confirm with the clang and gcc developers that what we think
-fwrapv means is what they agree (and will document) as what it means
(2) update our makefiles/docs/etc appropriately

and also I think that (1) is the significant part of the exercise,
whereas (2) is just a cleanup/tidyup that we can do afterwards.
This patch is doing (2) before we've done (1).

> To sum up:
>
> - GCC promises that signed shift of << is implementation-defined except
> for overflow in constant expressions, and defines it as operating on bit
> values.  This was approved.  For GCC, -fwrapv does not even apply except
> again for constant expressions.
>
> - signed shift of negative values in constant expressions _are_ covered
> by GCC's promise.  The implementation-defined behavior of signed <<
> gives a meaning to all signed shifts, and all that the C standard
> requires is "Each constant expression shall evaluate to a constant that
> is in the range of representable values for its type" (6.6p4).
> Therefore we should at least disable -Wshift-negative-value, because it
> doesn't buy us anything.
>
> - regarding overflow, in addition to the weird -Wpedantic warning, GCC 6
> adds a new -Wshift-overflow flag which is enabled by default in C99 and
> C11 modes, and which only applies to constant expressions.  So the
> remaining case where the compiler may change its behavior on overflow,
> i.e. constant expressions, will thus be caught by our usage of -Werror
> (because -Wshift-overflow is enabled by default).  So, independent of
> the limited scope of GCC's promise, with GCC 6 we will never have
> constant expressions that overflow because of left shifts.

I'm confused by all this text about constant expressions. Does
-fwrapv guarantee that signed shift of << behaves as we want
in all situations (including constant expressions) or doesn't it?
If it doesn't do that for constant expressions I don't think we should
rely on it, because it's way too confusing to have "this is OK except
when it isn't OK". (And a lot of our uses of "-1 << 8" are indeed
in constant expressions.)

> - if a compiler actually treated signed << overflow undefined behavior,
> a possible fix would be to use C89 mode (-std=gnu89), since signed << is
> unspecified there rather than undefined.  With C89, GCC's promise is
> complete.  We do use C89 with GCC <= 4.9 anyway, it makes sense to be
> homogeneous across all supported compilers.

"unspecified" isn't a great deal better than "undefined" really.

> Now, -fwrapv was really included in my patch only to silence ubsan in
> the future.  I think it should, and I will work on patches to fix that.
>  However, an advantage of -std=gnu89 is that it silences ubsan _now_ at
> least for GCC.  So let's just drop -fwrapv and use -std=gnu89 instead.
> This buys us time, and in the meanwhile I'll gett -fwrapv complete in GCC.
>
> If this is okay, I'll remove the patch, respin the pull request, and
> post a new configure change to use -std=gnu89.

I don't think this gains us much as a different approach, and it's
still trying to do cleanup (2) before we have dealt with the main
issue (1).

> Yes, we haven't heard anything from clang developers.  But clang does
> not even document implementation-defined behavior
> (https://llvm.org/bugs/show_bug.cgi?id=11272).  The bug says:
>
>> The clang documentation should specify how clang behaves in cases
>> specified to be implementation-defined in the relevant standards.
>> Perhaps simply saying that our behavior is the same as GCC's would suffice?
>
> This is about implementation-defined rather than undefined behavior, but
> I think it's enough to not care about clang developer's silence.

I disagree here. I think it's important to get the clang developers
to tell us what they mean by -fwrapv and what they want to guarantee
about signed shifts. That's because if they decide to say "no, we
don't guarantee behaviour here because the C spec says it's UB" then
we need to change how we de

Re: [Qemu-devel] [PATCH v2] ui/cocoa.m: Prevent activation clicks from going to guest

[Peter Maydell]

On 26 November 2015 at 14:40, Programmingkid  wrote:
>
> On Nov 26, 2015, at 6:45 AM, Peter Maydell wrote:
>
>> On 26 November 2015 at 01:14, Programmingkid  
>> wrote:
>>> When QEMU is brought to the foreground, the click event that activates QEMU
>>> should not go to the guest. Accidents happen when they do go to the guest
>>> without giving the user a change to handle them. Buttons are clicked 
>>> accidently.
>>> Windows are closed accidently. Volumes are unmounted accidently. This patch
>>> prevents these accidents from happening.
>>>
>>> Signed-off-by: John Arbuckle 
>>>
>>> ---
>>> Added code that handles the right mouse button and the other mouse button.
>>
>> This seems like a fair bit of repeated code. Does the change
>> below do the right thing?
> YES! Excellent job with this one. It's very compact.
>
>> I think it ought to work but I don't have
>> any guests handy which use the mouse to check with.
>
> Really?

All of my test guests on the OSX box are serial-console only
(they're faster to boot, and I don't do much development on
OSX anyway.)

> Reviewed-by: John Arbuckle 

Great, thanks for testing this. I'll send it out as a proper
patch with my sign-off and your reviewed-by tag this afternoon.

-- PMM

Re: [Qemu-devel] [PATCH v13 01/14] qobject: Simplify QObject

[Markus Armbruster]

Eric Blake  writes:

> The QObject hierarchy is small enough, and unlikely to grow further
> (since we only use it to map to JSON and already cover all JSON
> types), that we can simplify things by not tracking a separate
> vtable, but just inline the refcnt element of the vtable QType
> directly into QObject, and track a separate array of destroy
> functions.  We can drop qnull_destroy_obj() in the process.

Make that something like "inline the code element of the vtable QType
directly into QObject (renamed to type), and".

>
> The remaining QObject subclasses must export their destructor.
>
> This also has the nice benefit of moving the typename 'QType'
> out of the way, so that the next patch can repurpose it for a
> nicer name for 'qtype_code'.
>
> The various objects are still the same size (so no change in cache
> line pressure), but now have less indirection (although I didn't
> bother benchmarking to see if there is a noticeable speedup, as
> we don't have hard evidence that this was in a performance hotspot
> in the first place).
>
> Suggested-by: Markus Armbruster 
> Signed-off-by: Eric Blake 

Re: [Qemu-devel] [PATCH qemu 2/2] ppc/spapr: Add "ibm, pa-features" property to the device-tree

[Alexander Graf]

On 22.10.15 09:30, Alexey Kardashevskiy wrote:
> From: Benjamin Herrenschmidt 
> 
> LoPAPR defines a "ibm,pa-features" per-CPU device tree property which
> describes extended features of the Processor Architecture.
> 
> This adds the property to the device tree. At the moment this is the
> copy of what pHyp advertises except "I=1 (cache inhibited) Large Pages"
> which is enabled for TCG and disabled when running under HV KVM host
> with 4K system page size.
> 
> Signed-off-by: Benjamin Herrenschmidt 
> [aik: rebased, changed commit log, moved ci_large_pages initialization,
> renamed pa_features arrays]
> Signed-off-by: Alexey Kardashevskiy 
> ---
>  hw/ppc/spapr.c  | 31 +++
>  target-ppc/cpu.h|  1 +
>  target-ppc/kvm.c|  7 +++
>  target-ppc/translate_init.c |  1 +
>  4 files changed, 40 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 3852ad1..21c1312 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -597,6 +597,24 @@ static void spapr_populate_cpu_dt(CPUState *cs, void 
> *fdt, int offset,
>  uint32_t vcpus_per_socket = smp_threads * smp_cores;
>  uint32_t pft_size_prop[] = {0, cpu_to_be32(spapr->htab_shift)};
>  
> +/* Note: we keep CI large pages off for now because a 64K capable guest
> + * provisioned with large pages might otherwise try to map a qemu
> + * framebuffer (or other kind of memory mapped PCI BAR) using 64K pages
> + * even if that qemu runs on a 4k host.
> + *
> + * We can later add this bit back when we are confident this is not
> + * an issue (!HV KVM or 64K host)
> + */
> +uint8_t pa_features_206[] = { 6, 0,
> +0xf6, 0x1f, 0xc7, 0x00, 0x80, 0xc0 };
> +uint8_t pa_features_207[] = { 24, 0,
> +0xf6, 0x1f, 0xc7, 0xc0, 0x80, 0xf0,
> +0x80, 0x00, 0x00, 0x00, 0x00, 0x00,
> +0x00, 0x00, 0x00, 0x00, 0x80, 0x00,
> +0x80, 0x00, 0x80, 0x00, 0x80, 0x00 };

That's a lot of magic numbers. Do you think you could convert them into
something slightly more readable?

> +uint8_t *pa_features;
> +size_t pa_size;
> +
>  _FDT((fdt_setprop_cell(fdt, offset, "reg", index)));
>  _FDT((fdt_setprop_string(fdt, offset, "device_type", "cpu")));
>  
> @@ -662,6 +680,19 @@ static void spapr_populate_cpu_dt(CPUState *cs, void 
> *fdt, int offset,
>page_sizes_prop, page_sizes_prop_size)));
>  }
>  
> +/* Do the ibm,pa-features property, adjust it for ci-large-pages */
> +if (env->mmu_model == POWERPC_MMU_2_06) {
> +pa_features = pa_features_206;
> +pa_size = sizeof(pa_features_206);
> +} else /* env->mmu_model == POWERPC_MMU_2_07 */ {
> +pa_features = pa_features_207;
> +pa_size = sizeof(pa_features_207);
> +}
> +if (env->ci_large_pages) {
> +pa_features[3] |= 0x20;
> +}
> +_FDT((fdt_setprop(fdt, offset, "ibm,pa-features", pa_features, 
> pa_size)));
> +
>  _FDT((fdt_setprop_cell(fdt, offset, "ibm,chip-id",
> cs->cpu_index / vcpus_per_socket)));
>  
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index 69d8cf6..b34aed6 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -1073,6 +1073,7 @@ struct CPUPPCState {
>  uint64_t insns_flags2;
>  #if defined(TARGET_PPC64)
>  struct ppc_segment_page_sizes sps;
> +bool ci_large_pages;
>  #endif
>  
>  #if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 7671ae7..0c59f7f 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -414,6 +414,13 @@ static void kvm_fixup_page_sizes(PowerPCCPU *cpu)
>  /* Convert to QEMU form */
>  memset(&env->sps, 0, sizeof(env->sps));
>  
> +/* If we have HV KVM, we need to forbid CI large pages if our
> + * host page size is smaller than 64K.
> + */
> +if (smmu_info.flags & KVM_PPC_PAGE_SIZES_REAL) {
> +env->ci_large_pages = getpagesize() >= 0x1;

There's a global variable for the page size, no?


Alex

[Qemu-devel] [PATCH for-2.5] ui/cocoa.m: Prevent activation clicks from going to guest

[Peter Maydell]

When QEMU is brought to the foreground, the click event that activates QEMU
should not go to the guest. Accidents happen when they do go to the guest
without giving the user a chance to handle them. In particular, if the
guest input device is not an absolute-position one then the location of
the guest cursor (and thus the click) will likely not be the location of
the host cursor when it is clicked, and could be completely obscured
below another window. Don't send mouse clicks to QEMU unless the
window either has focus or has grabbed mouse events.

Reported-by: John Arbuckle 
Signed-off-by: Peter Maydell 
Reviewed-by: John Arbuckle 
---
 ui/cocoa.m | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/ui/cocoa.m b/ui/cocoa.m
index 1554331..d76b942 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -724,7 +724,15 @@ QemuCocoaView *cocoaView;
 }
 
 if (mouse_event) {
-if (last_buttons != buttons) {
+/* Don't send button events to the guest unless we've got a
+ * mouse grab or window focus. If we have neither then this event
+ * is the user clicking on the background window to activate and
+ * bring us to the front, which will be done by the sendEvent
+ * call below. We definitely don't want to pass that click through
+ * to the guest.
+ */
+if ((isMouseGrabbed || [[self window] isKeyWindow]) &&
+(last_buttons != buttons)) {
 static uint32_t bmap[INPUT_BUTTON_MAX] = {
 [INPUT_BUTTON_LEFT]   = MOUSE_EVENT_LBUTTON,
 [INPUT_BUTTON_MIDDLE] = MOUSE_EVENT_MBUTTON,
-- 
2.6.2

Re: [Qemu-devel] [PATCH] migration: fix analyze-migration.py script

[Alexander Graf]

On 30.10.15 17:50, Mark Cave-Ayland wrote:
> On 26/10/15 09:48, Mark Cave-Ayland wrote:
> 
>> On 06/09/15 12:54, Mark Cave-Ayland wrote:
>>
>>> On 06/09/15 09:36, Alexander Graf wrote:
>>>
 On 05.09.15 21:51, Mark Cave-Ayland wrote:
> Commit 61964 "Add configuration section" broke the analyze-migration.py 
> script
> which terminates due to the unrecognised section. Fix the script by 
> parsing
> the contents of the configuration section directly into a new
> ConfigurationSection object (although nothing is done with it yet).
>
> Signed-off-by: Mark Cave-Ayland 
> ---
>  scripts/analyze-migration.py |   13 +
>  1 file changed, 13 insertions(+)
>
> diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
> index f6894be..1455387 100755
> --- a/scripts/analyze-migration.py
> +++ b/scripts/analyze-migration.py
> @@ -252,6 +252,15 @@ class HTABSection(object):
>  def getDict(self):
>  return ""
>  
> +
> +class ConfigurationSection(object):
> +def __init__(self, file):
> +self.file = file
> +
> +def read(self):
> +name_len = self.file.read32()
> +name = self.file.readstr(len = name_len)
> +
>  class VMSDFieldGeneric(object):
>  def __init__(self, desc, file):
>  self.file = file
> @@ -474,6 +483,7 @@ class MigrationDump(object):
>  QEMU_VM_SECTION_FULL  = 0x04
>  QEMU_VM_SUBSECTION= 0x05
>  QEMU_VM_VMDESCRIPTION = 0x06
> +QEMU_VM_CONFIGURATION = 0x07
>  QEMU_VM_SECTION_FOOTER= 0x7e
>  
>  def __init__(self, filename):
> @@ -514,6 +524,9 @@ class MigrationDump(object):
>  section_type = file.read8()
>  if section_type == self.QEMU_VM_EOF:
>  break
> +elif section_type == self.QEMU_VM_CONFIGURATION:
> +section = ConfigurationSection(file)
> +section.read()

 So since we don't have a normal section header, there is no version
 field either. That in turn means that the format is determined by the
 machine version only - bleks.
>>>
>>> Yes :(  I double-checked the output of a migration file with hexdump and
>>> confirmed that just the raw fields are included without any additional
>>> metadata, even though the state is held in a VMStateDescription.
>>>
 So if there ever has to be more in the configuration section than the
 machine name, please move to a more detectable scheme. Ideally something
 that contains

   * version
   * length of dynamically sized fields
   * lenght of full blob

 would be ideal, so that we have a chance to at least put code into the
 analyze script to examine it.

 For now, I think the hard coded solution in the analyze script is
 reasonable.

 However, I think we should print out the name if we find it. It should
 be as simple as adding a special case for the configuration section in
 MigrationDump.getDict().
>>>
>>> I did play with adding a separate JSON object for configuration but was
>>> torn between whether configuration should have its own JSON object
>>> (better if we include extra fields and metadata as above) or to just
>>> embed it as a simple "machine" property similar to "page_size". I'll
>>> wait until we hear back from David/Juan and submit a v2 accordingly.
>>
>> Ping again from Juan/David? The analyze-migration.py script is currently
>> broken without this patch (or another equivalent) applied.
> 
> Ping^2? FWIW I've added this to the wiki at
> http://wiki.qemu.org/Planning/2.5 since without this patch or something
> similar applied, this feature is completely broken.

Juan, David?


Alex

Re: [Qemu-devel] [PATCH] migration: fix analyze-migration.py script

[Dr. David Alan Gilbert]

* Alexander Graf (ag...@suse.de) wrote:
> 
> 
> On 30.10.15 17:50, Mark Cave-Ayland wrote:
> > On 26/10/15 09:48, Mark Cave-Ayland wrote:
> > 
> >> On 06/09/15 12:54, Mark Cave-Ayland wrote:
> >>
> >>> On 06/09/15 09:36, Alexander Graf wrote:
> >>>
>  On 05.09.15 21:51, Mark Cave-Ayland wrote:
> > Commit 61964 "Add configuration section" broke the analyze-migration.py 
> > script
> > which terminates due to the unrecognised section. Fix the script by 
> > parsing
> > the contents of the configuration section directly into a new
> > ConfigurationSection object (although nothing is done with it yet).
> >
> > Signed-off-by: Mark Cave-Ayland 
> > ---
> >  scripts/analyze-migration.py |   13 +
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
> > index f6894be..1455387 100755
> > --- a/scripts/analyze-migration.py
> > +++ b/scripts/analyze-migration.py
> > @@ -252,6 +252,15 @@ class HTABSection(object):
> >  def getDict(self):
> >  return ""
> >  
> > +
> > +class ConfigurationSection(object):
> > +def __init__(self, file):
> > +self.file = file
> > +
> > +def read(self):
> > +name_len = self.file.read32()
> > +name = self.file.readstr(len = name_len)
> > +
> >  class VMSDFieldGeneric(object):
> >  def __init__(self, desc, file):
> >  self.file = file
> > @@ -474,6 +483,7 @@ class MigrationDump(object):
> >  QEMU_VM_SECTION_FULL  = 0x04
> >  QEMU_VM_SUBSECTION= 0x05
> >  QEMU_VM_VMDESCRIPTION = 0x06
> > +QEMU_VM_CONFIGURATION = 0x07
> >  QEMU_VM_SECTION_FOOTER= 0x7e
> >  
> >  def __init__(self, filename):
> > @@ -514,6 +524,9 @@ class MigrationDump(object):
> >  section_type = file.read8()
> >  if section_type == self.QEMU_VM_EOF:
> >  break
> > +elif section_type == self.QEMU_VM_CONFIGURATION:
> > +section = ConfigurationSection(file)
> > +section.read()
> 
>  So since we don't have a normal section header, there is no version
>  field either. That in turn means that the format is determined by the
>  machine version only - bleks.
> >>>
> >>> Yes :(  I double-checked the output of a migration file with hexdump and
> >>> confirmed that just the raw fields are included without any additional
> >>> metadata, even though the state is held in a VMStateDescription.
> >>>
>  So if there ever has to be more in the configuration section than the
>  machine name, please move to a more detectable scheme. Ideally something
>  that contains
> 
>    * version
>    * length of dynamically sized fields
>    * lenght of full blob
> 
>  would be ideal, so that we have a chance to at least put code into the
>  analyze script to examine it.
> 
>  For now, I think the hard coded solution in the analyze script is
>  reasonable.
> 
>  However, I think we should print out the name if we find it. It should
>  be as simple as adding a special case for the configuration section in
>  MigrationDump.getDict().
> >>>
> >>> I did play with adding a separate JSON object for configuration but was
> >>> torn between whether configuration should have its own JSON object
> >>> (better if we include extra fields and metadata as above) or to just
> >>> embed it as a simple "machine" property similar to "page_size". I'll
> >>> wait until we hear back from David/Juan and submit a v2 accordingly.
> >>
> >> Ping again from Juan/David? The analyze-migration.py script is currently
> >> broken without this patch (or another equivalent) applied.
> > 
> > Ping^2? FWIW I've added this to the wiki at
> > http://wiki.qemu.org/Planning/2.5 since without this patch or something
> > similar applied, this feature is completely broken.
> 
> Juan, David?

Isn't this already in ( 96e5c9bc77acef8b7b56cbe23a8a2611feff9e34 ) - or is
this a different breakage?

Dave

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

[Qemu-devel] [PATCH] qom-test: fix qmp() leaks

[marcandre . lureau]

From: Marc-André Lureau 

Before this patch ASAN reported:
SUMMARY: AddressSanitizer: 677165875 byte(s) leaked in 1272437 allocation(s)

After this patch:
SUMMARY: AddressSanitizer: 465 byte(s) leaked in 32 allocation(s)

Signed-off-by: Marc-André Lureau 
---
 tests/qom-test.c | 25 -
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/tests/qom-test.c b/tests/qom-test.c
index fde04e7..8a08fd7 100644
--- a/tests/qom-test.c
+++ b/tests/qom-test.c
@@ -47,7 +47,7 @@ static bool is_blacklisted(const char *arch, const char *mach)
 static void test_properties(const char *path, bool recurse)
 {
 char *child_path;
-QDict *response, *tuple;
+QDict *response, *tuple, *tmp;
 QList *list;
 QListEntry *entry;
 
@@ -57,6 +57,7 @@ static void test_properties(const char *path, bool recurse)
 g_assert(response);
 
 if (!recurse) {
+QDECREF(response);
 return;
 }
 
@@ -75,19 +76,21 @@ static void test_properties(const char *path, bool recurse)
 } else {
 const char *prop = qdict_get_str(tuple, "name");
 g_test_message("Testing property %s.%s", path, prop);
-response = qmp("{ 'execute': 'qom-get',"
-   "  'arguments': { 'path': %s,"
-   " 'property': %s } }",
-   path, prop);
+tmp = qmp("{ 'execute': 'qom-get',"
+  "  'arguments': { 'path': %s,"
+  " 'property': %s } }",
+  path, prop);
 /* qom-get may fail but should not, e.g., segfault. */
-g_assert(response);
+g_assert(tmp);
+QDECREF(tmp);
 }
 }
+QDECREF(response);
 }
 
-static void test_machine(gconstpointer data)
+static void test_machine(gpointer data)
 {
-const char *machine = data;
+char *machine = data;
 char *args;
 QDict *response;
 
@@ -98,9 +101,11 @@ static void test_machine(gconstpointer data)
 
 response = qmp("{ 'execute': 'quit' }");
 g_assert(qdict_haskey(response, "return"));
+QDECREF(response);
 
 qtest_end();
 g_free(args);
+g_free(machine);
 }
 
 static void add_machine_test_cases(void)
@@ -129,10 +134,12 @@ static void add_machine_test_cases(void)
 mname = qstring_get_str(qstr);
 if (!is_blacklisted(arch, mname)) {
 path = g_strdup_printf("qom/%s", mname);
-qtest_add_data_func(path, mname, test_machine);
+qtest_add_data_func(path, g_strdup(mname), test_machine);
 }
 }
+
 qtest_end();
+QDECREF(response);
 }
 
 int main(int argc, char **argv)
-- 
2.5.0

Re: [Qemu-devel] [PATCH] migration: fix analyze-migration.py script

[Alexander Graf]

On 26.11.15 16:31, Dr. David Alan Gilbert wrote:
> * Alexander Graf (ag...@suse.de) wrote:
>>
>>
>> On 30.10.15 17:50, Mark Cave-Ayland wrote:
>>> On 26/10/15 09:48, Mark Cave-Ayland wrote:
>>>
 On 06/09/15 12:54, Mark Cave-Ayland wrote:

> On 06/09/15 09:36, Alexander Graf wrote:
>
>> On 05.09.15 21:51, Mark Cave-Ayland wrote:
>>> Commit 61964 "Add configuration section" broke the analyze-migration.py 
>>> script
>>> which terminates due to the unrecognised section. Fix the script by 
>>> parsing
>>> the contents of the configuration section directly into a new
>>> ConfigurationSection object (although nothing is done with it yet).
>>>
>>> Signed-off-by: Mark Cave-Ayland 
>>> ---
>>>  scripts/analyze-migration.py |   13 +
>>>  1 file changed, 13 insertions(+)
>>>
>>> diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
>>> index f6894be..1455387 100755
>>> --- a/scripts/analyze-migration.py
>>> +++ b/scripts/analyze-migration.py
>>> @@ -252,6 +252,15 @@ class HTABSection(object):
>>>  def getDict(self):
>>>  return ""
>>>  
>>> +
>>> +class ConfigurationSection(object):
>>> +def __init__(self, file):
>>> +self.file = file
>>> +
>>> +def read(self):
>>> +name_len = self.file.read32()
>>> +name = self.file.readstr(len = name_len)
>>> +
>>>  class VMSDFieldGeneric(object):
>>>  def __init__(self, desc, file):
>>>  self.file = file
>>> @@ -474,6 +483,7 @@ class MigrationDump(object):
>>>  QEMU_VM_SECTION_FULL  = 0x04
>>>  QEMU_VM_SUBSECTION= 0x05
>>>  QEMU_VM_VMDESCRIPTION = 0x06
>>> +QEMU_VM_CONFIGURATION = 0x07
>>>  QEMU_VM_SECTION_FOOTER= 0x7e
>>>  
>>>  def __init__(self, filename):
>>> @@ -514,6 +524,9 @@ class MigrationDump(object):
>>>  section_type = file.read8()
>>>  if section_type == self.QEMU_VM_EOF:
>>>  break
>>> +elif section_type == self.QEMU_VM_CONFIGURATION:
>>> +section = ConfigurationSection(file)
>>> +section.read()
>>
>> So since we don't have a normal section header, there is no version
>> field either. That in turn means that the format is determined by the
>> machine version only - bleks.
>
> Yes :(  I double-checked the output of a migration file with hexdump and
> confirmed that just the raw fields are included without any additional
> metadata, even though the state is held in a VMStateDescription.
>
>> So if there ever has to be more in the configuration section than the
>> machine name, please move to a more detectable scheme. Ideally something
>> that contains
>>
>>   * version
>>   * length of dynamically sized fields
>>   * lenght of full blob
>>
>> would be ideal, so that we have a chance to at least put code into the
>> analyze script to examine it.
>>
>> For now, I think the hard coded solution in the analyze script is
>> reasonable.
>>
>> However, I think we should print out the name if we find it. It should
>> be as simple as adding a special case for the configuration section in
>> MigrationDump.getDict().
>
> I did play with adding a separate JSON object for configuration but was
> torn between whether configuration should have its own JSON object
> (better if we include extra fields and metadata as above) or to just
> embed it as a simple "machine" property similar to "page_size". I'll
> wait until we hear back from David/Juan and submit a v2 accordingly.

 Ping again from Juan/David? The analyze-migration.py script is currently
 broken without this patch (or another equivalent) applied.
>>>
>>> Ping^2? FWIW I've added this to the wiki at
>>> http://wiki.qemu.org/Planning/2.5 since without this patch or something
>>> similar applied, this feature is completely broken.
>>
>> Juan, David?
> 
> Isn't this already in ( 96e5c9bc77acef8b7b56cbe23a8a2611feff9e34 ) - or is
> this a different breakage?

Awesome, I didn't see an accepted email :). Sorry for the fuss.


Alex

Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25)

[Paolo Bonzini]

On 26/11/2015 16:01, Peter Maydell wrote:
> > - regarding overflow, in addition to the weird -Wpedantic warning, GCC 6
> > adds a new -Wshift-overflow flag which is enabled by default in C99 and
> > C11 modes, and which only applies to constant expressions.  So the
> > remaining case where the compiler may change its behavior on overflow,
> > i.e. constant expressions, will thus be caught by our usage of -Werror
> > (because -Wshift-overflow is enabled by default).  So, independent of
> > the limited scope of GCC's promise, with GCC 6 we will never have
> > constant expressions that overflow because of left shifts.
> 
> I'm confused by all this text about constant expressions. Does
> -fwrapv guarantee that signed shift of << behaves as we want
> in all situations (including constant expressions) or doesn't it?

Until GCC 5, it does even without -fwrapv.

Until GCC 6, the generated code is OK but you get warnings.  For (-1 <<
8) you have to enable them manually, for (0xFF << 28) you always get
them.  I am working on a patch to make GCC 6 shut up if you specify
-fwrapv, but my point is that -fwrapv is _not_ necessary because:

- GCC very sensibly does not make -Wall enable -Wshift-negative-value

- warnings such as 0xFF << 28 are much less contentious, and even shifts
into the sign bit (e.g. 1 << 31 or 0xFF << 24) are not enabled by
default either.

> (And a lot of our uses of "-1 << 8" are indeed in constant expressions.)

Those are okay as long as you do not use -Wextra.

Again, the _value_ is perfectly specified by the GCC documentation (and
as of this morning, it's promised to remain that way).  GCC leaves the
door open for warning in constant expressions, and indeed GCC 6 warns
more than GCC 5 in this regard.

> "unspecified" isn't a great deal better than "undefined" really.

It is better inasmuch as it shuts up ubsan.

>> If this is okay, I'll remove the patch, respin the pull request, and
>> post a new configure change to use -std=gnu89.
> 
> I don't think this gains us much as a different approach, and it's
> still trying to do cleanup (2) before we have dealt with the main
> issue (1).

What I'm saying is that:

- you were (rightly) worried about the compiler's behavior for constant
expressions

- but since we use -Werror, we do not have to worry about them.  With
-Werror, anything that GCC 6 can compile is perfectly specified by the
GCC documentation, including left shifts of negative values.

So GCC does not even need -fwrapv, and -std=gnu89 is a better way to
shut up ubsan because it already works.

Regarding clang, we cannot be hostage of their silence.  And recalling
that they were the ones who f***ed up their warning levels in the first
place, and are making us waste so much time, I couldn't care less about
their opinions.

> > This is about implementation-defined rather than undefined behavior, but
> > I think it's enough to not care about clang developer's silence.
>
> I disagree here. I think it's important to get the clang developers
> to tell us what they mean by -fwrapv and what they want to guarantee
> about signed shifts. That's because if they decide to say "no, we
> don't guarantee behaviour here because the C spec says it's UB" then
> we need to change how we deal with these in QEMU.

No, we need to change our list of supported compilers (if that happens).

Paolo

[Qemu-devel] [PULL 6/9] virtio-scsi: don't crash without a valid device

[Paolo Bonzini]

From: "Eugene (jno) Dvurechenski" 

Make sure that we actually have a device when checking the aio
context. Otherwise guests could trigger QEMU crashes.

Signed-off-by: "Eugene (jno) Dvurechenski" 
Reviewed-by: David Hildenbrand 
Message-Id: <1448549135-6582-2-git-send-email-...@linux.vnet.ibm.com>
Signed-off-by: Paolo Bonzini 
---
 hw/scsi/virtio-scsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 7655401..3a4f520 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -250,7 +250,7 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq 
*req)
 int target;
 int ret = 0;
 
-if (s->dataplane_started) {
+if (s->dataplane_started && d) {
 assert(blk_get_aio_context(d->conf.blk) == s->ctx);
 }
 /* Here VIRTIO_SCSI_S_OK means "FUNCTION COMPLETE".  */
-- 
2.5.0

[Qemu-devel] [PULL 2/9] call bdrv_drain_all() even if the vm is stopped

[Paolo Bonzini]

From: Wen Congyang 

There are still I/O operations when the vm is stopped. For example,
stop the vm, and do block migration. In this case, we don't drain all
I/O operation, and may meet the following problem:

qemu-system-x86_64: migration/block.c:731: block_save_complete: Assertion 
`block_mig_state.submitted == 0' failed.

Signed-off-by: Wen Congyang 
Message-Id: <564ee92e.4070...@cn.fujitsu.com>
Cc: qemu-sta...@nongnu.org
Signed-off-by: Paolo Bonzini 
---
 cpus.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/cpus.c b/cpus.c
index 877bd70..43676fa 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1415,6 +1415,8 @@ int vm_stop_force_state(RunState state)
 return vm_stop(state);
 } else {
 runstate_set(state);
+
+bdrv_drain_all();
 /* Make sure to return an error if the flush in a previous vm_stop()
  * failed. */
 return bdrv_flush_all();
-- 
2.5.0

[Qemu-devel] [PULL 4/9] exec: remove warning about mempath and hugetlbfs

[Paolo Bonzini]

From: "Daniel P. Berrange" 

The gethugepagesize() method in exec.c printed a warning if
the file path for "-mem-path" or "-object memory-backend-file"
was not on a hugetlbfs filesystem. This warning is bogus, because
QEMU functions perfectly well with the path on a regular tmpfs
filesystem. Use of hugetlbfs vs tmpfs is a choice for the management
application or end user to make as best fits their needs. As such it
is inappropriate for QEMU to have an opinion on whether the user's
choice is right or wrong in this case.

Signed-off-by: Daniel P. Berrange 
Message-Id: <1448448749-1332-3-git-send-email-berra...@redhat.com>
Signed-off-by: Paolo Bonzini 
---
 exec.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/exec.c b/exec.c
index b09f18b..de1cf19 100644
--- a/exec.c
+++ b/exec.c
@@ -1196,9 +1196,6 @@ static long gethugepagesize(const char *path, Error 
**errp)
 return 0;
 }
 
-if (fs.f_type != HUGETLBFS_MAGIC)
-fprintf(stderr, "Warning: path not on HugeTLBFS: %s\n", path);
-
 return fs.f_bsize;
 }
 
-- 
2.5.0

[Qemu-devel] [PULL v2 0/9] Misc patches for QEMU 2.5-rc2

[Paolo Bonzini]

The following changes since commit 4b6eda626fdb8bf90472c6868d502a2ac09abeeb:

  Merge remote-tracking branch 'remotes/lalrae/tags/mips-20151124' into staging 
(2015-11-24 17:05:06 +)

are available in the git repository at:

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

for you to fetch changes up to 5120901a378501403d5454b69cf43e666fc29d5b:

  target-i386: kvm: Print warning when clearing mcg_cap bits (2015-11-26 
16:48:16 +0100)


Small patches, without the one that introduces -fwrapv.


Daniel P. Berrange (2):
  Revert "exec: silence hugetlbfs warning under qtest"
  exec: remove warning about mempath and hugetlbfs

Eduardo Habkost (3):
  target-i386: kvm: Abort if MCE bank count is not supported by host
  target-i386: kvm: Use env->mcg_cap when setting up MCE
  target-i386: kvm: Print warning when clearing mcg_cap bits

Eugene (jno) Dvurechenski (1):
  virtio-scsi: don't crash without a valid device

Paolo Bonzini (2):
  MAINTAINERS: Update TCG CPU cores section
  target-sparc: fix 32-bit truncation in fpackfix

Wen Congyang (1):
  call bdrv_drain_all() even if the vm is stopped

 MAINTAINERS   | 17 +
 cpus.c|  2 ++
 exec.c|  6 --
 hw/scsi/virtio-scsi.c |  2 +-
 target-i386/cpu.h |  2 ++
 target-i386/kvm.c | 22 ++
 target-sparc/vis_helper.c |  2 +-
 vl.c  | 28 ++--
 8 files changed, 47 insertions(+), 34 deletions(-)
-- 
2.5.0

[Qemu-devel] [PULL 1/9] MAINTAINERS: Update TCG CPU cores section

[Paolo Bonzini]

These are the people that I think have been touching it lately
or reviewing patches.

Signed-off-by: Paolo Bonzini 
---
 MAINTAINERS | 17 +
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 28f0139..bb1f3e4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -62,14 +62,22 @@ Guest CPU cores (TCG):
 --
 Overall
 L: qemu-devel@nongnu.org
-S: Odd fixes
+M: Paolo Bonzini 
+M: Peter Crosthwaite 
+M: Richard Henderson 
+S: Maintained
 F: cpu-exec.c
+F: cpu-exec-common.c
+F: cpus.c
 F: cputlb.c
+F: exec.c
 F: softmmu_template.h
-F: translate-all.c
-F: include/exec/cpu_ldst.h
-F: include/exec/cpu_ldst_template.h
+F: translate-all.*
+F: translate-common.c
+F: include/exec/cpu*.h
+F: include/exec/exec-all.h
 F: include/exec/helper*.h
+F: include/exec/tb-hash.h
 
 Alpha
 M: Richard Henderson 
@@ -1042,6 +1050,7 @@ S: Supported
 F: include/exec/ioport.h
 F: ioport.c
 F: include/exec/memory.h
+F: include/exec/ram_addr.h
 F: memory.c
 F: include/exec/memory-internal.h
 F: exec.c
-- 
2.5.0

[Qemu-devel] [PULL 8/9] target-i386: kvm: Use env->mcg_cap when setting up MCE

[Paolo Bonzini]

From: Eduardo Habkost 

When setting up MCE, instead of using the MCE_*_DEF macros
directly, just filter the existing env->mcg_cap value.

As env->mcg_cap is already initialized as
MCE_CAP_DEF|MCE_BANKS_DEF at target-i386/cpu.c:mce_init(), this
doesn't change any behavior. But it will allow us to change
mce_init() in the future, to implement different defaults
depending on CPU model, machine-type or command-line parameters.

Signed-off-by: Eduardo Habkost 
Signed-off-by: Paolo Bonzini 
Message-Id: <1448471956-66873-9-git-send-email-pbonz...@redhat.com>
---
 target-i386/cpu.h |  2 ++
 target-i386/kvm.c | 11 ---
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index fc4a605..84edfd0 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -286,6 +286,8 @@
 #define MCE_CAP_DEF (MCG_CTL_P|MCG_SER_P)
 #define MCE_BANKS_DEF   10
 
+#define MCG_CAP_BANKS_MASK 0xff
+
 #define MCG_STATUS_RIPV (1ULL<<0)   /* restart ip valid */
 #define MCG_STATUS_EIPV (1ULL<<1)   /* ip points to correct instruction */
 #define MCG_STATUS_MCIP (1ULL<<2)   /* machine check in progress */
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 93d1f5e..90bd447 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -784,21 +784,18 @@ int kvm_arch_init_vcpu(CPUState *cs)
 return ret;
 }
 
-if (banks < MCE_BANKS_DEF) {
+if (banks < (env->mcg_cap & MCG_CAP_BANKS_MASK)) {
 error_report("kvm: Unsupported MCE bank count (QEMU = %d, KVM = 
%d)",
- MCE_BANKS_DEF, banks);
+ (int)(env->mcg_cap & MCG_CAP_BANKS_MASK), banks);
 return -ENOTSUP;
 }
 
-mcg_cap &= MCE_CAP_DEF;
-mcg_cap |= MCE_BANKS_DEF;
-ret = kvm_vcpu_ioctl(cs, KVM_X86_SETUP_MCE, &mcg_cap);
+env->mcg_cap &= mcg_cap | MCG_CAP_BANKS_MASK;
+ret = kvm_vcpu_ioctl(cs, KVM_X86_SETUP_MCE, &env->mcg_cap);
 if (ret < 0) {
 fprintf(stderr, "KVM_X86_SETUP_MCE: %s", strerror(-ret));
 return ret;
 }
-
-env->mcg_cap = mcg_cap;
 }
 
 qemu_add_vm_change_state_handler(cpu_update_state, env);
-- 
2.5.0

[Qemu-devel] [PULL 7/9] target-i386: kvm: Abort if MCE bank count is not supported by host

[Paolo Bonzini]

From: Eduardo Habkost 

Instead of silently changing the number of banks in mcg_cap based
on kvm_get_mce_cap_supported(), abort initialization if the host
doesn't support MCE_BANKS_DEF banks.

Note that MCE_BANKS_DEF was always 10 since it was introduced in
QEMU, and Linux always returned 32 at KVM_CAP_MCE since
KVM_CAP_MCE was introduced, so no behavior is being changed and
the error can't be triggered by any Linux version. The point of
the new check is to ensure we won't silently change the bank
count if we change MCE_BANKS_DEF or make the bank count
configurable in the future.

Signed-off-by: Eduardo Habkost 
[Avoid Yoda condition and \n at end of error_report. - Paolo]
Signed-off-by: Paolo Bonzini 
Message-Id: <1448471956-66873-8-git-send-email-pbonz...@redhat.com>
---
 target-i386/kvm.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 2a9953b..93d1f5e 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -784,11 +784,14 @@ int kvm_arch_init_vcpu(CPUState *cs)
 return ret;
 }
 
-if (banks > MCE_BANKS_DEF) {
-banks = MCE_BANKS_DEF;
+if (banks < MCE_BANKS_DEF) {
+error_report("kvm: Unsupported MCE bank count (QEMU = %d, KVM = 
%d)",
+ MCE_BANKS_DEF, banks);
+return -ENOTSUP;
 }
+
 mcg_cap &= MCE_CAP_DEF;
-mcg_cap |= banks;
+mcg_cap |= MCE_BANKS_DEF;
 ret = kvm_vcpu_ioctl(cs, KVM_X86_SETUP_MCE, &mcg_cap);
 if (ret < 0) {
 fprintf(stderr, "KVM_X86_SETUP_MCE: %s", strerror(-ret));
-- 
2.5.0

[Qemu-devel] [PULL 5/9] target-sparc: fix 32-bit truncation in fpackfix

[Paolo Bonzini]

This is reported by Coverity.  The algorithm description at
ftp://ftp.icm.edu.pl/packages/ggi/doc/hw/sparc/Sparc.pdf suggests
that the 32-bit parts of rs2, after the left shift, is treated
as a 64-bit integer.  Bits 32 and above are used to do the
saturating truncation.

Message-Id: <1446473134-4330-1-git-send-email-pbonz...@redhat.com>
Signed-off-by: Paolo Bonzini 
---
 target-sparc/vis_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-sparc/vis_helper.c b/target-sparc/vis_helper.c
index 383cc8b..45fc7db 100644
--- a/target-sparc/vis_helper.c
+++ b/target-sparc/vis_helper.c
@@ -447,7 +447,7 @@ uint32_t helper_fpackfix(uint64_t gsr, uint64_t rs2)
 for (word = 0; word < 2; word++) {
 uint32_t val;
 int32_t src = rs2 >> (word * 32);
-int64_t scaled = src << scale;
+int64_t scaled = (int64_t)src << scale;
 int64_t from_fixed = scaled >> 16;
 
 val = (from_fixed < -32768 ? -32768 :
-- 
2.5.0

[Qemu-devel] [PULL 3/9] Revert "exec: silence hugetlbfs warning under qtest"

[Paolo Bonzini]

From: "Daniel P. Berrange" 

This reverts commit 1c7ba94a184df1eddd589d5400d879568d3e5d08.

That commit changed QEMU initialization order from

 - object-initial, chardev, qtest, object-late

to

 - chardev, qtest, object-initial, object-late

This breaks chardev setups which need to rely on objects
having been created. For example, when chardevs use TLS
encryption in the future, they need to have tls credential
objects created first.

This revert, restores the ordering introduced in

  commit f08f9271bfe3f19a5eb3d7a2f48532065304d5c8
  Author: Daniel P. Berrange 
  Date:   Wed May 13 17:14:04 2015 +0100

vl: Create (most) objects before creating chardev backends

Signed-off-by: Daniel P. Berrange 
Message-Id: <1448448749-1332-2-git-send-email-berra...@redhat.com>
Signed-off-by: Paolo Bonzini 
---
 exec.c |  5 +
 vl.c   | 28 ++--
 2 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/exec.c b/exec.c
index acbd4a2..b09f18b 100644
--- a/exec.c
+++ b/exec.c
@@ -51,7 +51,6 @@
 #include "qemu/main-loop.h"
 #include "translate-all.h"
 #include "sysemu/replay.h"
-#include "sysemu/qtest.h"
 
 #include "exec/memory-internal.h"
 #include "exec/ram_addr.h"
@@ -1197,10 +1196,8 @@ static long gethugepagesize(const char *path, Error 
**errp)
 return 0;
 }
 
-if (!qtest_driver() &&
-fs.f_type != HUGETLBFS_MAGIC) {
+if (fs.f_type != HUGETLBFS_MAGIC)
 fprintf(stderr, "Warning: path not on HugeTLBFS: %s\n", path);
-}
 
 return fs.f_bsize;
 }
diff --git a/vl.c b/vl.c
index 525929b..4211ff1 100644
--- a/vl.c
+++ b/vl.c
@@ -4291,26 +4291,17 @@ int main(int argc, char **argv, char **envp)
 page_size_init();
 socket_init();
 
-if (qemu_opts_foreach(qemu_find_opts("chardev"),
-  chardev_init_func, NULL, NULL)) {
-exit(1);
-}
-
-if (qtest_chrdev) {
-Error *local_err = NULL;
-qtest_init(qtest_chrdev, qtest_log, &local_err);
-if (local_err) {
-error_report_err(local_err);
-exit(1);
-}
-}
-
 if (qemu_opts_foreach(qemu_find_opts("object"),
   object_create,
   object_create_initial, NULL)) {
 exit(1);
 }
 
+if (qemu_opts_foreach(qemu_find_opts("chardev"),
+  chardev_init_func, NULL, NULL)) {
+exit(1);
+}
+
 #ifdef CONFIG_VIRTFS
 if (qemu_opts_foreach(qemu_find_opts("fsdev"),
   fsdev_init_func, NULL, NULL)) {
@@ -4337,6 +4328,15 @@ int main(int argc, char **argv, char **envp)
 
 configure_accelerator(current_machine);
 
+if (qtest_chrdev) {
+Error *local_err = NULL;
+qtest_init(qtest_chrdev, qtest_log, &local_err);
+if (local_err) {
+error_report_err(local_err);
+exit(1);
+}
+}
+
 machine_opts = qemu_get_machine_opts();
 kernel_filename = qemu_opt_get(machine_opts, "kernel");
 initrd_filename = qemu_opt_get(machine_opts, "initrd");
-- 
2.5.0

[Qemu-devel] [PULL 9/9] target-i386: kvm: Print warning when clearing mcg_cap bits

[Paolo Bonzini]

From: Eduardo Habkost 

Instead of silently clearing mcg_cap bits when the host doesn't
support them, print a warning when doing that.

Signed-off-by: Eduardo Habkost 
[Avoid \n at end of error_report. - Paolo]
Signed-off-by: Paolo Bonzini 
Message-Id: <1448471956-66873-10-git-send-email-pbonz...@redhat.com>
---
 target-i386/kvm.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 90bd447..6dc9846 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -774,7 +774,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
 && (env->features[FEAT_1_EDX] & (CPUID_MCE | CPUID_MCA)) ==
(CPUID_MCE | CPUID_MCA)
 && kvm_check_extension(cs->kvm_state, KVM_CAP_MCE) > 0) {
-uint64_t mcg_cap;
+uint64_t mcg_cap, unsupported_caps;
 int banks;
 int ret;
 
@@ -790,6 +790,12 @@ int kvm_arch_init_vcpu(CPUState *cs)
 return -ENOTSUP;
 }
 
+unsupported_caps = env->mcg_cap & ~(mcg_cap | MCG_CAP_BANKS_MASK);
+if (unsupported_caps) {
+error_report("warning: Unsupported MCG_CAP bits: 0x%" PRIx64,
+ unsupported_caps);
+}
+
 env->mcg_cap &= mcg_cap | MCG_CAP_BANKS_MASK;
 ret = kvm_vcpu_ioctl(cs, KVM_X86_SETUP_MCE, &env->mcg_cap);
 if (ret < 0) {
-- 
2.5.0

Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25)

[Peter Maydell]

On 26 November 2015 at 15:40, Paolo Bonzini  wrote:
> On 26/11/2015 16:01, Peter Maydell wrote:
>> I'm confused by all this text about constant expressions. Does
>> -fwrapv guarantee that signed shift of << behaves as we want
>> in all situations (including constant expressions) or doesn't it?
>
> Until GCC 5, it does even without -fwrapv.
>
> Until GCC 6, the generated code is OK but you get warnings.  For (-1 <<
> 8) you have to enable them manually, for (0xFF << 28) you always get
> them.  I am working on a patch to make GCC 6 shut up if you specify
> -fwrapv, but my point is that -fwrapv is _not_ necessary because:
>
> - GCC very sensibly does not make -Wall enable -Wshift-negative-value
>
> - warnings such as 0xFF << 28 are much less contentious, and even shifts
> into the sign bit (e.g. 1 << 31 or 0xFF << 24) are not enabled by
> default either.
>
>> (And a lot of our uses of "-1 << 8" are indeed in constant expressions.)
>
> Those are okay as long as you do not use -Wextra.

This is all talking about in-practice behaviour of the compiler.
What I'm interested in is what the documented guarantees are.

> Again, the _value_ is perfectly specified by the GCC documentation (and
> as of this morning, it's promised to remain that way).  GCC leaves the
> door open for warning in constant expressions, and indeed GCC 6 warns
> more than GCC 5 in this regard.

I still don't understand why the GCC documentation can't straightforwardly
say "-fwrapv means you get 2s complement behaviour for all operations
including shifts and arithmetic, in all contexts, and we will not warn
or otherwise complain about it". If that's what they're in practice
agreeing to then why not just say so in a comprehensible way, rather
than splitting the information across two different sections of the
documentation and including a confusing bit of text about constant
expressions?

>> I don't think this gains us much as a different approach, and it's
>> still trying to do cleanup (2) before we have dealt with the main
>> issue (1).
>
> What I'm saying is that:
>
> - you were (rightly) worried about the compiler's behavior for constant
> expressions
>
> - but since we use -Werror, we do not have to worry about them.  With
> -Werror, anything that GCC 6 can compile is perfectly specified by the
> GCC documentation, including left shifts of negative values.
>
> So GCC does not even need -fwrapv, and -std=gnu89 is a better way to
> shut up ubsan because it already works.
>
> Regarding clang, we cannot be hostage of their silence.  And recalling
> that they were the ones who f***ed up their warning levels in the first
> place, and are making us waste so much time, I couldn't care less about
> their opinions.

It's been less than 10 days since you filed a bug report with them.
Why are we in such a hurry? I would much rather we took the time to
hear from the clang developers as well as the gcc developers, and then
made our decision about what the right compiler flags are. There doesn't
seem to be any sudden change here that means we need to adjust our
compiler flags for the 2.5 release.

>> > This is about implementation-defined rather than undefined behavior, but
>> > I think it's enough to not care about clang developer's silence.
>>
>> I disagree here. I think it's important to get the clang developers
>> to tell us what they mean by -fwrapv and what they want to guarantee
>> about signed shifts. That's because if they decide to say "no, we
>> don't guarantee behaviour here because the C spec says it's UB" then
>> we need to change how we deal with these in QEMU.
>
> No, we need to change our list of supported compilers (if that happens).

I would strongly favour avoiding this UB rather than dropping clang
as a supported compiler,and implicitly dropping OSX as a supported
platform. (But it doesn't seem to me very likely we'd end up having
to make that awkward choice.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 6/7] kvm/x86: Hyper-V SynIC message slot pending clearing at SINT ack

[Andrey Smetanin]

On 11/26/2015 05:43 PM, Paolo Bonzini wrote:



On 26/11/2015 10:06, Andrey Smetanin wrote:



On 11/25/2015 08:14 PM, Paolo Bonzini wrote:



On 25/11/2015 17:55, Andrey Smetanin wrote:


+gpa = synic->msg_page & PAGE_MASK;
+page = kvm_vcpu_gfn_to_page(vcpu, gpa >> PAGE_SHIFT);
+if (is_error_page(page)) {
+vcpu_err(vcpu, "Hyper-V SynIC can't get msg page, gpa
0x%llx\n",
+ gpa);
+return;
+}
+msg_page = kmap_atomic(page);


But the message page is not being pinned, is it?

Actually I don't know anything about pinning.
Is it pinning against page swapping ?


Yes.  Unless the page is pinned, kmap_atomic can fail.

kmap_atomic() can't fail for a valid page struct. Does
kvm_vcpu_gfn_to_page() can provide invalid page(swapped page) struct
which may pass is_error_page(page) check but can leads to incorrect
behavior inside kmap_atomic()?


No, you're right.  Nevermind, I was confused because I thought you
needed kmap_atomic rather than kmap.  Here using kmap_atomic is just an
optimization, so it's okay.  (If you needed kmap_atomic, the problem
would have been that kvm_vcpu_gfn_to_page() can sleep).

In patch 7/7 you're also not in atomic context, so kvm_vcpu_gfn_to_page
is okay.

Shouldn't have reviewed the patch when tired. :)

Then the patches look good, I think.  With a testcase I can try them out
and hopefully merge them for Linux 4.5 / QEMU 2.6.

Thank you!

We already have a working Hyper-V SynIC timers kvm-unit-tests test case.
We are going to send appropriate patches seria into kvm-unit-tests git.

But  kvm-unit-tests master now broken:
> make
objcopy -O elf32-i386 x86/memory.elf x86/memory.flat
make: *** No rule to make target 'x86/pku.o', needed by 'x86/pku.elf'. 
Stop.


The problem is in latest commit 3da70799dd3cf1169c4668b4a3fd6f598528b8b9.

The commit adds 'pku' test case building, but not added any pku.c 
implementation file.


[root@asm-pc kvm-unit-tests]# ls -al x86/pku.c
ls: cannot access x86/pku.c: No such file or directory


Could you please fix it ?


Paolo

Re: [Qemu-devel] [PATCH v1 6/7] kvm/x86: Hyper-V SynIC message slot pending clearing at SINT ack

[Paolo Bonzini]

On 26/11/2015 16:53, Andrey Smetanin wrote:
>> Then the patches look good, I think.  With a testcase I can try them out
>> and hopefully merge them for Linux 4.5 / QEMU 2.6.
> 
> Thank you!
> 
> We already have a working Hyper-V SynIC timers kvm-unit-tests test case.
> We are going to send appropriate patches seria into kvm-unit-tests git.
> 
> But  kvm-unit-tests master now broken:
>> make
> objcopy -O elf32-i386 x86/memory.elf x86/memory.flat
> make: *** No rule to make target 'x86/pku.o', needed by 'x86/pku.elf'.
> Stop.
> 
> The problem is in latest commit 3da70799dd3cf1169c4668b4a3fd6f598528b8b9.
> 
> The commit adds 'pku' test case building, but not added any pku.c
> implementation file.
> 
> [root@asm-pc kvm-unit-tests]# ls -al x86/pku.c
> ls: cannot access x86/pku.c: No such file or directory
> 
> 
> Could you please fix it ?

Done, sorry.

Paolo

[Qemu-devel] [PATCH V3 1/3] hw/acpi: merge pxb adjacent memory/IO ranges

[Marcel Apfelbaum]

A generic PCI Bus Expander doesn't necessary have a built-in PCI bridge.
Int this case the ACPI will include IO/MEM ranges per device. Try to merge
adjacent resources to reduce the ACPI tables length.

Signed-off-by: Marcel Apfelbaum 
---
 hw/i386/acpi-build.c | 123 +++
 1 file changed, 74 insertions(+), 49 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 95e0c65..736b252 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -762,16 +762,59 @@ static void crs_replace_with_free_ranges(GPtrArray 
*ranges,
 g_ptr_array_free(free_ranges, false);
 }
 
+/*
+ * crs_range_merge - merges adjacent ranges in the given array.
+ * Array elements are deleted and replaced with the merged ranges.
+ */
+static void crs_range_merge(GPtrArray *range)
+{
+GPtrArray *tmp =  g_ptr_array_new_with_free_func(crs_range_free);
+CrsRangeEntry *entry;
+uint64_t range_base, range_limit;
+int i;
+
+if (!range->len) {
+return;
+}
+
+g_ptr_array_sort(range, crs_range_compare);
+
+entry = g_ptr_array_index(range, 0);
+range_base = entry->base;
+range_limit = entry->limit;
+for (i = 1; i < range->len; i++) {
+entry = g_ptr_array_index(range, i);
+if (entry->base - 1 == range_limit) {
+range_limit = entry->limit;
+} else {
+crs_range_insert(tmp, range_base, range_limit);
+range_base = entry->base;
+range_limit = entry->limit;
+}
+}
+crs_range_insert(tmp, range_base, range_limit);
+
+g_ptr_array_set_size(range, 0);
+for (i = 0; i < tmp->len; i++) {
+entry = g_ptr_array_index(tmp, i);
+crs_range_insert(range, entry->base, entry->limit);
+}
+g_ptr_array_free(tmp, true);
+}
+
 static Aml *build_crs(PCIHostState *host,
   GPtrArray *io_ranges, GPtrArray *mem_ranges)
 {
 Aml *crs = aml_resource_template();
+GPtrArray *host_io_ranges = g_ptr_array_new_with_free_func(crs_range_free);
+GPtrArray *host_mem_ranges = 
g_ptr_array_new_with_free_func(crs_range_free);
+CrsRangeEntry *entry;
 uint8_t max_bus = pci_bus_num(host->bus);
 uint8_t type;
 int devfn;
+int i;
 
 for (devfn = 0; devfn < ARRAY_SIZE(host->bus->devices); devfn++) {
-int i;
 uint64_t range_base, range_limit;
 PCIDevice *dev = host->bus->devices[devfn];
 
@@ -794,26 +837,9 @@ static Aml *build_crs(PCIHostState *host,
 }
 
 if (r->type & PCI_BASE_ADDRESS_SPACE_IO) {
-aml_append(crs,
-aml_word_io(AML_MIN_FIXED, AML_MAX_FIXED,
-AML_POS_DECODE, AML_ENTIRE_RANGE,
-0,
-range_base,
-range_limit,
-0,
-range_limit - range_base + 1));
-crs_range_insert(io_ranges, range_base, range_limit);
+crs_range_insert(host_io_ranges, range_base, range_limit);
 } else { /* "memory" */
-aml_append(crs,
-aml_dword_memory(AML_POS_DECODE, AML_MIN_FIXED,
- AML_MAX_FIXED, AML_NON_CACHEABLE,
- AML_READ_WRITE,
- 0,
- range_base,
- range_limit,
- 0,
- range_limit - range_base + 1));
-crs_range_insert(mem_ranges, range_base, range_limit);
+crs_range_insert(host_mem_ranges, range_base, range_limit);
 }
 }
 
@@ -832,15 +858,7 @@ static Aml *build_crs(PCIHostState *host,
  * that do not support multiple root buses
  */
 if (range_base && range_base <= range_limit) {
-aml_append(crs,
-   aml_word_io(AML_MIN_FIXED, AML_MAX_FIXED,
-   AML_POS_DECODE, AML_ENTIRE_RANGE,
-   0,
-   range_base,
-   range_limit,
-   0,
-   range_limit - range_base + 1));
-crs_range_insert(io_ranges, range_base, range_limit);
+crs_range_insert(host_io_ranges, range_base, range_limit);
 }
 
 range_base =
@@ -853,16 +871,7 @@ static Aml *build_crs(PCIHostState *host,
  * that do not support multiple root buses
  */
 if (range_base && range_base <= range_limit) {
-aml_append(crs,
-   aml_dword_memory(AML_POS_DECODE, AML_MIN_FIXED,
-A