Re: [Qemu-devel] [PATCH v11 2/6] ACPI: Add APEI GHES Table Generation support

[gengdongjiu]

Hi Shannon,

On 2017/8/26 9:08, Shannon Zhao wrote:
> 
> 
> On 2017/8/25 19:20, gengdongjiu wrote:
 diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 3d78ff6..def1ec1 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -45,6 +45,7 @@
>>  #include "hw/arm/virt.h"
>>  #include "sysemu/numa.h"
>>  #include "kvm_arm.h"
>> +#include "hw/acpi/hest_ghes.h"
>>  
>>  #define ARM_SPI_BASE 32
>>  #define ACPI_POWER_BUTTON_DEVICE "PWRB"
>> @@ -771,6 +772,9 @@ void virt_acpi_build(VirtMachineState *vms, 
>> AcpiBuildTables *tables)
>>  acpi_add_table(table_offsets, tables_blob);
>>  build_spcr(tables_blob, tables->linker, vms);
>>  
>> +acpi_add_table(table_offsets, tables_blob);
>> +ghes_build_acpi(tables_blob, tables->hardware_errors, 
>> tables->linker);
>> +
 So we add this table unconditionally. Is there any bad impact if QEMU
 runs on old kvm? Does it need to check whether KVM supports RAS?
>> this table is added before guest OS boot. so can not use KVM to check it.
> No, we can check the RAS capability when we create vcpus like you done
> in another patch ans can use that in table generation.
understand your meaning.

ARM James ever have below comments about the table generation.
--
But you can use APEI in a guest on CPUs without the RAS extensions: the host may
signal memory errors to Qemu for any number of reasons, user-space shouldn't
care how it knows. Examples are PCI-AER, any APEI event notified by polling or
one of the flavours of irq.

I would expect Qemu to generate a HEST based on its abilities, i.e. if it
supports any mechanism of notifying the guest about errors. Choosing the
mechanism then depends on the type of error.

Ideally the Qemu code for HEST/GHES/CPER generation code using some of the irqs
and polling could be shared with x86, as these should be possible using common
KVM APIs.
---

He means we can use APEI on CPUs without RAS and may be share this code with 
x86,
if Qemu can support any mechanism of notifying the guest about errors, it 
should be
generate the table.
Now we depend on the macro KVM_HAVE_MCE_INJECTION to decide whether Qemu can 
support
notifying the guest.

what do you think which we should be dependent on to generate the table?

> 
>> if the old kvm does not support RAS, it does not have bad impact. only waste 
>> table memory.
>> May be we can make it as device? if this device is enabled in the qemu
>> boot parameters, then we will add this table?
>>
> 
> And you need to add a option to virt machine for (migration)
> compatibility. On new virt machine it's on by default while off for old
> ones.
ok.

> 
> Thanks,
> 

Re: [Qemu-devel] [PATCH v11 1/6] ACPI: add APEI/HEST/CPER structures and macros

[gengdongjiu]

On 2017/8/26 9:00, Shannon Zhao wrote:
> 
> 
> On 2017/8/25 18:37, gengdongjiu wrote:
 +
>> +/* From the ACPI 6.1 spec, "18.3.2.9 Hardware Error Notification" */
>> +
 It's better to refer to the first spec version of this structure and
 same with others you define.
>>  do you mean which spec version? the definition is aligned with the linux 
>> kernel.
> What I mean here is that it's better to refer to the ACPI spec version
> which introduces Hardware Error Notification first time.
Ok, I basically understand your meaning. I will clear that. thanks.

> 

>> +enum AcpiHestNotifyType {
>> +ACPI_HEST_NOTIFY_POLLED = 0,
>> +ACPI_HEST_NOTIFY_EXTERNAL = 1,
>> +ACPI_HEST_NOTIFY_LOCAL = 2,
>> +ACPI_HEST_NOTIFY_SCI = 3,
>> +ACPI_HEST_NOTIFY_NMI = 4,
>> +ACPI_HEST_NOTIFY_CMCI = 5,  /* ACPI 5.0 */
>> +ACPI_HEST_NOTIFY_MCE = 6,   /* ACPI 5.0 */
>> +ACPI_HEST_NOTIFY_GPIO = 7,  /* ACPI 6.0 */
>> +ACPI_HEST_NOTIFY_SEA = 8,   /* ACPI 6.1 */
>> +ACPI_HEST_NOTIFY_SEI = 9,   /* ACPI 6.1 */
>> +ACPI_HEST_NOTIFY_GSIV = 10, /* ACPI 6.1 */
>> +ACPI_HEST_NOTIFY_RESERVED = 11  /* 11 and greater are reserved */
 In ACPI 6.2, 11 is for Software Delegated Exception, is this useful for
 your patchset?
>>   it is usefull, for all the error source, I reserved the space for them.
>> Because the space is allocated one time, is not dynamically allocated.
>> so I use the ACPI_HEST_NOTIFY_RESERVED to specify that there is 11 error 
>> source.
>>
> I mean whether the new type Software Delegated Exception is useful for
> RAS. If so, we could add this new type here.
Just now I check the ACPI 6.2 spec, it indeed introduced the new type SDEI.

currently we do not use the type Software Delegated Exception which introduced 
by ACPI 6.2,
so may not need to add a new type.

> 
> Thanks,
> 

Re: [Qemu-devel] [PATCH v11 2/6] ACPI: Add APEI GHES Table Generation support

[Shannon Zhao]

On 2017/8/25 19:20, gengdongjiu wrote:
>>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>>> >> index 3d78ff6..def1ec1 100644
>>> >> --- a/hw/arm/virt-acpi-build.c
>>> >> +++ b/hw/arm/virt-acpi-build.c
>>> >> @@ -45,6 +45,7 @@
>>> >>  #include "hw/arm/virt.h"
>>> >>  #include "sysemu/numa.h"
>>> >>  #include "kvm_arm.h"
>>> >> +#include "hw/acpi/hest_ghes.h"
>>> >>  
>>> >>  #define ARM_SPI_BASE 32
>>> >>  #define ACPI_POWER_BUTTON_DEVICE "PWRB"
>>> >> @@ -771,6 +772,9 @@ void virt_acpi_build(VirtMachineState *vms, 
>>> >> AcpiBuildTables *tables)
>>> >>  acpi_add_table(table_offsets, tables_blob);
>>> >>  build_spcr(tables_blob, tables->linker, vms);
>>> >>  
>>> >> +acpi_add_table(table_offsets, tables_blob);
>>> >> +ghes_build_acpi(tables_blob, tables->hardware_errors, 
>>> >> tables->linker);
>>> >> +
>> > So we add this table unconditionally. Is there any bad impact if QEMU
>> > runs on old kvm? Does it need to check whether KVM supports RAS?
> this table is added before guest OS boot. so can not use KVM to check it.
No, we can check the RAS capability when we create vcpus like you done
in another patch ans can use that in table generation.

> if the old kvm does not support RAS, it does not have bad impact. only waste 
> table memory.
> May be we can make it as device? if this device is enabled in the qemu
> boot parameters, then we will add this table?
> 

And you need to add a option to virt machine for (migration)
compatibility. On new virt machine it's on by default while off for old
ones.

Thanks,
-- 
Shannon

Re: [Qemu-devel] [PATCH v11 1/6] ACPI: add APEI/HEST/CPER structures and macros

[Shannon Zhao]

On 2017/8/25 18:37, gengdongjiu wrote:
>>> +
>>> >> +/* From the ACPI 6.1 spec, "18.3.2.9 Hardware Error Notification" */
>>> >> +
>> > It's better to refer to the first spec version of this structure and
>> > same with others you define.
>  do you mean which spec version? the definition is aligned with the linux 
> kernel.
What I mean here is that it's better to refer to the ACPI spec version
which introduces Hardware Error Notification first time.

>> > 
>>> >> +enum AcpiHestNotifyType {
>>> >> +ACPI_HEST_NOTIFY_POLLED = 0,
>>> >> +ACPI_HEST_NOTIFY_EXTERNAL = 1,
>>> >> +ACPI_HEST_NOTIFY_LOCAL = 2,
>>> >> +ACPI_HEST_NOTIFY_SCI = 3,
>>> >> +ACPI_HEST_NOTIFY_NMI = 4,
>>> >> +ACPI_HEST_NOTIFY_CMCI = 5,  /* ACPI 5.0 */
>>> >> +ACPI_HEST_NOTIFY_MCE = 6,   /* ACPI 5.0 */
>>> >> +ACPI_HEST_NOTIFY_GPIO = 7,  /* ACPI 6.0 */
>>> >> +ACPI_HEST_NOTIFY_SEA = 8,   /* ACPI 6.1 */
>>> >> +ACPI_HEST_NOTIFY_SEI = 9,   /* ACPI 6.1 */
>>> >> +ACPI_HEST_NOTIFY_GSIV = 10, /* ACPI 6.1 */
>>> >> +ACPI_HEST_NOTIFY_RESERVED = 11  /* 11 and greater are reserved */
>> > In ACPI 6.2, 11 is for Software Delegated Exception, is this useful for
>> > your patchset?
>   it is usefull, for all the error source, I reserved the space for them.
> Because the space is allocated one time, is not dynamically allocated.
> so I use the ACPI_HEST_NOTIFY_RESERVED to specify that there is 11 error 
> source.
> 
I mean whether the new type Software Delegated Exception is useful for
RAS. If so, we could add this new type here.

Thanks,
-- 
Shannon

Re: [Qemu-devel] [PATCH v8 0/5] hypertrace: Lightweight guest-to-QEMU trace channel

[Emilio G. Cota]

On Sun, Jul 30, 2017 at 17:08:18 +0300, Lluís Vilanova wrote:
> The hypertrace channel allows guest code to emit events in QEMU (the host) 
> using
> its tracing infrastructure (see "docs/trace.txt"). This works in both 'system'
> and 'user' modes, is architecture-agnostic and introduces minimal noise on the
> guest.
> 
> See first commit for a full description, use-cases and an example.
> 
> Signed-off-by: Lluís Vilanova 

This would be indeed very useful once TCG instrumentation is in place.

However, I'm not very excited about this being PCI-only and Linux-only for
system mode
I wonder how we could make this work on all hosts -- did you consider using
"magic" instructions? We'd need a different magic instruction for each
guest ISA, but the library would hide that anyway (and the library code
would be the same for user and system modes).

Thanks,

Emilio

Re: [Qemu-devel] [PATCH 2/3] docker.py: Python 2.6 argparse compatibility

[Fam Zheng]

On Fri, 08/25 16:57, Stefan Hajnoczi wrote:
> Add the scripts/ directory to sys.path so Python 2.6 will be able to
> import argparse.
> 
> Cc: Fam Zheng 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  tests/docker/docker.py | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/docker/docker.py b/tests/docker/docker.py
> index ee40ca04d9..81c87ee329 100755
> --- a/tests/docker/docker.py
> +++ b/tests/docker/docker.py
> @@ -13,12 +13,14 @@
>  
>  import os
>  import sys
> +sys.path.append(os.path.join(os.path.dirname(__file__),
> + '..', '..', 'scripts'))
> +import argparse
>  import subprocess
>  import json
>  import hashlib
>  import atexit
>  import uuid
> -import argparse
>  import tempfile
>  import re
>  import signal
> -- 
> 2.13.5
> 

Acked-by: Fam Zheng 

Re: [Qemu-devel] [PATCH 00/13] instrument: Add basic event instrumentation

[Emilio G. Cota]

On Thu, Aug 03, 2017 at 12:54:57 +0100, Stefan Hajnoczi wrote:
> > > Please post an example of the API you'd like.
> > 
> > In my opinion, the instrumentation support in this series provides an API 
> > that
> > works in the opposite way you're suggesting (let's ignore the fact that it's
> > built on tracing events).
> > 
> > When QEMU loads an instrumentation library (which can happen at any time), 
> > some
> > initialization function is called on the library so that it can establish 
> > what
> > events to instrument. This also has the advantage that a user can hook into 
> > a
> > running QEMU instance at any time to perform some instrumentation.
> > 
> > I think this is the bare minimum necessary to make it work, and has the 
> > upside
> > of being completely orthogonal to the libqemu approach. We could reuse most 
> > of
> > the stable instrumentation API there too, except for the instrumentation 
> > code
> > initialization.
> > 
> > That being said, the libqemu approach *might* make it a bit easier to 
> > provide an
> > API for things such as "run for this many instructions and return control to
> > instrumentor", but I don't think that's mandatory for a first prototype 
> > (and can
> > definitely be implemented using both approaches).
> 
> The main concern I have is that a feature for loading shared libraries
> and hooking QEMU will be abused.  This can be mostly solved by offering
> only a stable API without the ability to hook trace events.  There are
> still ways to abuse this and that's why I prefer the libqemu approach.
> 
> The libqemu approach also avoids the "how do I enable my
> instrumentation?" step for new users because they just need to run their
> program after compiling it.  The workflow is simpler.

Widely used instrumentation tools, such as DynamoRIO and Pin, follow the
same approach Lluis took here, i.e. your plugin is loaded at run-time by the 
tool.
So from a usability viewpoint I don't think this approach would be 
controversial.

> As a next step I suggest defining the stable instrumentation API and
> dropping the tracing hooks.  We can still work out whether shared
> library loading or libqemu is okay later but the stable API is most
> important.

I agree with dropping the tracing hooks. However, I wonder whether we'd
want to add another field to struct TranslationBlock to keep track of
the dynamic instrumentation state, or just reuse trace_vcpu_dstate.

I gravitate towards the former approach--would be nice to have the
instrumentation code completely separate from the tracing code, and
hopefully not rely on python nor trace-events files :-)

Emilio

Re: [Qemu-devel] [PATCH 00/13] instrument: Add basic event instrumentation

[Emilio G. Cota]

On Fri, Jul 28, 2017 at 19:05:43 +0300, Lluís Vilanova wrote:
> As for the (minimum) requirements I've collected:
> 
> * Peek at register values and guest memory.
> * Enumerate guest cpus.
> * Control when events are raised to minimize overheads (e.g., avoid generating
>   TCG code to trace a guest code event we don't care about just now).
> * When a guest code event is raised at translation time, let instrumentor 
> decide
>   if it needs to be raised at execution time too (e.g., might be decided on
>   instruction opcode)
> * Minimum overhead when instrumenting (I think the proposed fifo/socket 
> approach
>   does not meet this; point 2 helps a lot here, which is what the current
>   tracing code does)
> * Let instrumentor know when TBs are being freed (i.e., to free internal 
> per-TB
>   structures; I have a patch queued adding this event).
> 
> Nice to have:
> 
> * Avoid recompilation for each new instrumentation logic.
> * Poke at register values and guest memory.
> * [synchronous for sure] Let user annotate guest programs to raise additional
>   events with guest-specified information (the hypertrace series I sent).
> * [synchronous for sure] Make guest breakpoints/watchpoints raise an event 
> (for
>   when we cannot annotate the guest code; I have some patches).
> * Trigger QEMU's event tracing routine when the instrumentation code
>   decides. This is what this series does now, as then lazy instrumentors don't
>   need to write their own tracing-to-disk code. Otherwise, per-event tracing
>   states would need to be independent of instrumentation states (but the logic
>   is exactly the same).
> * Let instrumentor define additional arguments to execution-time events during
>   translation-time (e.g., to quickly index into an instrumentor struct when 
> the
>   execution-time event comes that references info collected during 
> translation).
> * Attach an instrumentor-specified value to each guest CPU (e.g., pointing to
>   the instrumentor's cpu state). Might be less necessary if the point above is
>   met (if we only look at overall performance).

Agreed.

An additional "nice to have" would be:

* Allow inlining of TCG code by the instrumenter. Example use case:
  the instrumenter wants to increment a counter every time a
  basic block is executed. Instead of calling a callback function on every 
block's
  execution, we could just have a translation-time callback to emit at the 
beginning
  of the translated block the counter increment. This would be much faster, and
  is something that all other tools (e.g. DynamoRIO/Pin) implement.

E.

Re: [Qemu-devel] [PATCH 00/22] tcg: tb_lock removal

[Emilio G. Cota]

On Mon, Aug 07, 2017 at 19:52:16 -0400, Emilio G. Cota wrote:
> This series applies on top of the "multiple TCG contexts" series, v4:
>   https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg06769.html
> 
> Highlights:
> 
> - First, fix a few typos I encountered while working on this (patches 1-3).
>   I could send them separately to qemu-trivial if you prefer.
> - QHT: use a proper cmp function, instead of just checking pointer values
>   to determine equality of matches.
> - Use a binary search tree for each TCG region.
> - Make l1_map lockless by using cmpxchg
> - Introduce page locks (for !user-mode), so that tb_lock is not
>   needed when operating on a page
>   - Introduce page_collection, to lock a range of pages
> - Introduce tb->jmp_lock to protect TB jump lists.
> - Remove tb_lock. User-mode uses just mmap_lock and tb->jmp_lock's;
>   !user-mode uses the same jump locks as well as page locks.
> 
> Performance numbers are in patch 22. We get nice speedups, but I still
> see a lot of idling when booting many cores. I suspect it comes from
> cross-CPU events (e.g. TLB invalidations), but I need to profile it
> better (perf is not good for this; mutrace doesn't quite work). But
> anyway that's for another patchset.

The idling is due to BQL contention related to interrupt handling. In the
case of ARM, this boils down to the GICv3 code being single-threaded.
I don't have time right now to make it multi-threaded, but at least we
know where the scalability bottleneck is.

BTW if there's interest I can submit the lock profiler to the list. The code
is in this branch:
  https://github.com/cota/qemu/tree/lock-profiler
The first commit has sample output: https://github.com/cota/qemu/commit/c5bda634

Also, any feedback on the parent (tb_lock removal) patchset would be 
appreciated.

To make the 2.11 merge easier, I rebased this patchset (as well as the
multi-tcg-v4 set it is based on) on top of rth's tcg-generic-15, fixing a good
bunch of annoying conflicts. The resulting branch is available at:
  https://github.com/cota/qemu/tree/tcg-generic-15%2Bmulti-tcg-v4-parallel

Thanks,

Emilio

Re: [Qemu-devel] [PATCH] slirp: fix clearing ifq_so from pending packets

[Samuel Thibault]

Samuel Thibault, on sam. 26 août 2017 01:05:04 +0200, wrote:
> So Wjjzhang and PJP, can you confirm that this fixes your uses?

PJP, can you forward it to Wjjzhang?  I keep getting

: host cloudmx.qq.com[113.108.11.188] said: 550 Mail
content denied.
http://ascloud.qq.com/cgi-bin/readtemplate?t=anti_spam_errors#n1000726 (in
reply to end of DATA command)

Samuel

Re: [Qemu-devel] [PATCH] slirp: fix clearing ifq_so from pending packets

[Samuel Thibault]

Hello,

So Wjjzhang and PJP, can you confirm that this fixes your uses?

Samuel

Samuel Thibault, on sam. 26 août 2017 00:37:21 +0200, wrote:
> The if_fastq and if_batchq contain not only packets, but queues of packets
> for the same socket. When sofree frees a socket, it thus has to clear ifq_so
> from all the packets from the queues, not only the first.
> 
> Signed-off-by: Samuel Thibault 
> Acked-by: Philippe Mathieu-Daudé 
> ---
>  slirp/socket.c | 39 +++
>  1 file changed, 23 insertions(+), 16 deletions(-)
> 
> diff --git a/slirp/socket.c b/slirp/socket.c
> index ecec0295a9..cb7b5b608d 100644
> --- a/slirp/socket.c
> +++ b/slirp/socket.c
> @@ -59,6 +59,27 @@ socreate(Slirp *slirp)
>return(so);
>  }
>  
> +/*
> + * Remove references to so from the given message queue.
> + */
> +static void
> +soqfree(struct socket *so, struct quehead *qh)
> +{
> +struct mbuf *ifq;
> +
> +for (ifq = (struct mbuf *) qh->qh_link;
> + (struct quehead *) ifq != qh;
> + ifq = ifq->ifq_next) {
> +if (ifq->ifq_so == so) {
> +struct mbuf *ifm;
> +ifq->ifq_so = NULL;
> +for (ifm = ifq->ifs_next; ifm != ifq; ifm = ifm->ifs_next) {
> +ifm->ifq_so = NULL;
> +}
> +}
> +}
> +}
> +
>  /*
>   * remque and free a socket, clobber cache
>   */
> @@ -66,23 +87,9 @@ void
>  sofree(struct socket *so)
>  {
>Slirp *slirp = so->slirp;
> -  struct mbuf *ifm;
>  
> -  for (ifm = (struct mbuf *) slirp->if_fastq.qh_link;
> -   (struct quehead *) ifm != >if_fastq;
> -   ifm = ifm->ifq_next) {
> -if (ifm->ifq_so == so) {
> -  ifm->ifq_so = NULL;
> -}
> -  }
> -
> -  for (ifm = (struct mbuf *) slirp->if_batchq.qh_link;
> -   (struct quehead *) ifm != >if_batchq;
> -   ifm = ifm->ifq_next) {
> -if (ifm->ifq_so == so) {
> -  ifm->ifq_so = NULL;
> -}
> -  }
> +  soqfree(so, >if_fastq);
> +  soqfree(so, >if_batchq);
>  
>if (so->so_emu==EMU_RSH && so->extra) {
>   sofree(so->extra);
> -- 
> 2.14.1
> 

-- 
Samuel
...
 et Ctrl alt F2 pour aller sous console
 mais c koi pour passer d'un bureau a un autre !
 au fait c koi le raccourci pour passer d'un bureau a un autre 'question 
stupide"
 ça dépend du window manager et de ta conf
 ce qui fonctionne toujours c'est CTRL-ALT-BCKSP
-:- SignOff rv_: #linuxfr (Read error: EOF from client)
-:- rv_ [~rv@217.11.166.169] has joined #linuxfr
 Firebird: MEURT...

[Qemu-devel] [PATCH] target/arm: Fix aa64 ldp register writeback

[Richard Henderson]

For "ldp x0, x1, [x0]", if the second load is on a second page and
the second page is unmapped, the exception would be raised with x0
already modified.  This means the instruction couldn't be restarted.

Cc: qemu-...@nongnu.org
Cc: qemu-sta...@nongnu.org
Reported-by: Andrew 
Fixes: https://bugs.launchpad.net/qemu/+bug/1713066
Signed-off-by: Richard Henderson 
---
 target/arm/translate-a64.c | 28 
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index c596025b04..bfba816a77 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -2217,29 +2217,33 @@ static void disas_ldst_pair(DisasContext *s, uint32_t 
insn)
 } else {
 do_fp_st(s, rt, tcg_addr, size);
 }
-} else {
-TCGv_i64 tcg_rt = cpu_reg(s, rt);
-if (is_load) {
-do_gpr_ld(s, tcg_rt, tcg_addr, size, is_signed, false,
-  false, 0, false, false);
-} else {
-do_gpr_st(s, tcg_rt, tcg_addr, size,
-  false, 0, false, false);
-}
-}
-tcg_gen_addi_i64(tcg_addr, tcg_addr, 1 << size);
-if (is_vector) {
+tcg_gen_addi_i64(tcg_addr, tcg_addr, 1 << size);
 if (is_load) {
 do_fp_ld(s, rt2, tcg_addr, size);
 } else {
 do_fp_st(s, rt2, tcg_addr, size);
 }
 } else {
+TCGv_i64 tcg_rt = cpu_reg(s, rt);
 TCGv_i64 tcg_rt2 = cpu_reg(s, rt2);
+
 if (is_load) {
+TCGv_i64 tmp = tcg_temp_new_i64();
+
+/* Do not modify tcg_rt before recognizing any exception
+   from the second load.  */
+do_gpr_ld(s, tmp, tcg_addr, size, is_signed, false,
+  false, 0, false, false);
+tcg_gen_addi_i64(tcg_addr, tcg_addr, 1 << size);
 do_gpr_ld(s, tcg_rt2, tcg_addr, size, is_signed, false,
   false, 0, false, false);
+
+tcg_gen_mov_i64(tcg_rt, tmp);
+tcg_temp_free_i64(tmp);
 } else {
+do_gpr_st(s, tcg_rt, tcg_addr, size,
+  false, 0, false, false);
+tcg_gen_addi_i64(tcg_addr, tcg_addr, 1 << size);
 do_gpr_st(s, tcg_rt2, tcg_addr, size,
   false, 0, false, false);
 }
-- 
2.13.5

Re: [Qemu-devel] [PATCH] slirp: fix clearing ifq_so from pending packets

[Philippe Mathieu-Daudé]

Hi Sam, thanks for this patch :)

On 08/25/2017 07:37 PM, Samuel Thibault wrote:

The if_fastq and if_batchq contain not only packets, but queues of packets
for the same socket. When sofree frees a socket, it thus has to clear ifq_so
from all the packets from the queues, not only the first.

Signed-off-by: Samuel Thibault 
Acked-by: Philippe Mathieu-Daudé 


so let's raise this to:
Reviewed-by: Philippe Mathieu-Daudé 


---
  slirp/socket.c | 39 +++
  1 file changed, 23 insertions(+), 16 deletions(-)

diff --git a/slirp/socket.c b/slirp/socket.c
index ecec0295a9..cb7b5b608d 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -59,6 +59,27 @@ socreate(Slirp *slirp)
return(so);
  }
  
+/*

+ * Remove references to so from the given message queue.
+ */
+static void
+soqfree(struct socket *so, struct quehead *qh)
+{
+struct mbuf *ifq;
+
+for (ifq = (struct mbuf *) qh->qh_link;
+ (struct quehead *) ifq != qh;
+ ifq = ifq->ifq_next) {
+if (ifq->ifq_so == so) {
+struct mbuf *ifm;
+ifq->ifq_so = NULL;
+for (ifm = ifq->ifs_next; ifm != ifq; ifm = ifm->ifs_next) {
+ifm->ifq_so = NULL;
+}
+}
+}
+}
+
  /*
   * remque and free a socket, clobber cache
   */
@@ -66,23 +87,9 @@ void
  sofree(struct socket *so)
  {
Slirp *slirp = so->slirp;
-  struct mbuf *ifm;
  
-  for (ifm = (struct mbuf *) slirp->if_fastq.qh_link;

-   (struct quehead *) ifm != >if_fastq;
-   ifm = ifm->ifq_next) {
-if (ifm->ifq_so == so) {
-  ifm->ifq_so = NULL;
-}
-  }
-
-  for (ifm = (struct mbuf *) slirp->if_batchq.qh_link;
-   (struct quehead *) ifm != >if_batchq;
-   ifm = ifm->ifq_next) {
-if (ifm->ifq_so == so) {
-  ifm->ifq_so = NULL;
-}
-  }
+  soqfree(so, >if_fastq);
+  soqfree(so, >if_batchq);
  
if (so->so_emu==EMU_RSH && so->extra) {

sofree(so->extra);

Re: [Qemu-devel] [PATCH 1/5] pci: INTERFACE_LEGACY_PCI_DEVICE and INTERFACE_PCIE_DEVICE interfaces

[Eduardo Habkost]

On Fri, Aug 25, 2017 at 02:19:00PM -0600, Alex Williamson wrote:
> On Wed, 23 Aug 2017 19:14:41 -0300
> Eduardo Habkost  wrote:
> 
> > Those two interfaces will be used to indicate which device types
> > support legacy PCI or PCI-express buses.  Management software
> > will be able to use the qom-list-types QMP command to query that
> > information.
> 
> Nit, while "legacy PCI" and "conventional PCI" have about the same
> number of google hits, I believe the latter is the more correct term.
> The quality of hits is certainly a lot better with "conventional".
> Calling something "legacy" also spurs an immediate negative reaction
> for some folks, "conventional" is more neutral.  Thanks,

Oh, Conventional PCI even has a Wikipedia article.  I agree.  I
will change it in v2.  Thanks!

-- 
Eduardo

[Qemu-devel] [PATCH] slirp: fix clearing ifq_so from pending packets

[Samuel Thibault]

The if_fastq and if_batchq contain not only packets, but queues of packets
for the same socket. When sofree frees a socket, it thus has to clear ifq_so
from all the packets from the queues, not only the first.

Signed-off-by: Samuel Thibault 
Acked-by: Philippe Mathieu-Daudé 
---
 slirp/socket.c | 39 +++
 1 file changed, 23 insertions(+), 16 deletions(-)

diff --git a/slirp/socket.c b/slirp/socket.c
index ecec0295a9..cb7b5b608d 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -59,6 +59,27 @@ socreate(Slirp *slirp)
   return(so);
 }
 
+/*
+ * Remove references to so from the given message queue.
+ */
+static void
+soqfree(struct socket *so, struct quehead *qh)
+{
+struct mbuf *ifq;
+
+for (ifq = (struct mbuf *) qh->qh_link;
+ (struct quehead *) ifq != qh;
+ ifq = ifq->ifq_next) {
+if (ifq->ifq_so == so) {
+struct mbuf *ifm;
+ifq->ifq_so = NULL;
+for (ifm = ifq->ifs_next; ifm != ifq; ifm = ifm->ifs_next) {
+ifm->ifq_so = NULL;
+}
+}
+}
+}
+
 /*
  * remque and free a socket, clobber cache
  */
@@ -66,23 +87,9 @@ void
 sofree(struct socket *so)
 {
   Slirp *slirp = so->slirp;
-  struct mbuf *ifm;
 
-  for (ifm = (struct mbuf *) slirp->if_fastq.qh_link;
-   (struct quehead *) ifm != >if_fastq;
-   ifm = ifm->ifq_next) {
-if (ifm->ifq_so == so) {
-  ifm->ifq_so = NULL;
-}
-  }
-
-  for (ifm = (struct mbuf *) slirp->if_batchq.qh_link;
-   (struct quehead *) ifm != >if_batchq;
-   ifm = ifm->ifq_next) {
-if (ifm->ifq_so == so) {
-  ifm->ifq_so = NULL;
-}
-  }
+  soqfree(so, >if_fastq);
+  soqfree(so, >if_batchq);
 
   if (so->so_emu==EMU_RSH && so->extra) {
sofree(so->extra);
-- 
2.14.1

Re: [Qemu-devel] [PATCH 00/17] nbd client refactoring and fixing

[Eric Blake]

On 08/04/2017 10:14 AM, Vladimir Sementsov-Ogievskiy wrote:
> A bit more refactoring and fixing before BLOCK_STATUS series.
> I've tried to make individual patches simple enough, so there are
> a lot of them.
> 
> Vladimir Sementsov-Ogievskiy (17):
>   nbd/client: fix nbd_opt_go
>   nbd/client: refactor nbd_read_eof
>   nbd/client: refactor nbd_receive_reply
>   nbd/client: fix nbd_send_request to return int
>   block/nbd-client: get rid of ssize_t
>   block/nbd-client: fix nbd_read_reply_entry
>   block/nbd-client: refactor request send/receive
>   block/nbd-client: rename nbd_recv_coroutines_enter_all
>   block/nbd-client: move nbd_co_receive_reply content into
> nbd_co_request
>   block/nbd-client: move nbd_coroutine_end content into nbd_co_request
>   block/nbd-client: fix nbd_co_request: set s->reply.handle to 0 on
> error
>   block/nbd-client: refactor nbd_co_request
>   block/nbd-client: refactor NBDClientSession.recv_coroutine
>   block/nbd-client: exit reply-reading coroutine on incorrect handle
>   block/nbd-client: refactor reading reply
>   block/nbd-client: drop reply field from NBDClientSession
>   block/nbd-client: always return EIO on and after the first io channel
> error

I've pushed 1-5 and 7-10 onto my NBD staging branch for 2.11:

  git://repo.or.cz/qemu/ericb.git nbd

with a couple of changes squashed in as mentioned in individual patches;
please double-check that it looks okay.  If so, then I will use that
branch as the starting point for all NBD commits destined for 2.11,
sending a pull request once the tree opens.

Patches 6 and 11 are somewhat subsumed by the work that went into 2.10,
and the remaining patches are starting to cause enough conflicts that
I'd prefer you complete the rebase of patches 12-17 and post a v2 on top
of my staging branch.

I'm also hoping that Stefan will rebase a v2 of his "nbd-client: enter
read_reply_co during init to avoid crash" on top of your preliminary
work, since Paolo had a good suggestion on improving the semantics in
the review of v1.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 10/17] block/nbd-client: move nbd_coroutine_end content into nbd_co_request

[Eric Blake]

On 08/04/2017 10:14 AM, Vladimir Sementsov-Ogievskiy wrote:
> Move code from nbd_coroutine_end into nbd_co_request. The function
> nbd_coroutine_end is not needed separately, also it is better to
> have in_flight-- in nbd_co_request as in_flight++ lives in nbd_co_request
> too.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/nbd-client.c | 14 +-
>  1 file changed, 1 insertion(+), 13 deletions(-)
> 

Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 08/17] block/nbd-client: rename nbd_recv_coroutines_enter_all

[Eric Blake]

On 08/25/2017 01:43 PM, Eric Blake wrote:
> On 08/04/2017 10:14 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Rename nbd_recv_coroutines_enter_all to nbd_recv_coroutines_wake_all,
>> as it most probably just add all recv coroutines into co_queue_wakeup,

s/adds/

>> not directly enter them.

s/not/rather than/

>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>> ---
>>  block/nbd-client.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Reviewed-by: Eric Blake 
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 05/17] block/nbd-client: get rid of ssize_t

[Eric Blake]

On 08/04/2017 10:14 AM, Vladimir Sementsov-Ogievskiy wrote:
> Use int variable for nbd_co_send_request return value (as
> nbd_co_send_request returns int).
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/nbd-client.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 04/17] nbd/client: fix nbd_send_request to return int

[Eric Blake]

On 08/04/2017 10:14 AM, Vladimir Sementsov-Ogievskiy wrote:
> Fix nbd_send_request to return int, as it returns a return value
> of nbd_write (which is int), and the only user of nbd_send_request's
> return value (nbd_co_send_request) consider it as int too.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  include/block/nbd.h | 2 +-
>  nbd/client.c| 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 03/17] nbd/client: refactor nbd_receive_reply

[Eric Blake]

On 08/04/2017 10:14 AM, Vladimir Sementsov-Ogievskiy wrote:
> Refactor nbd_receive_reply to return 1 on success, 0 on eof, when no
> data was read and <0 for other cases, because returned size of read
> data is not actually used.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  include/block/nbd.h |  2 +-
>  nbd/client.c| 12 +---
>  2 files changed, 10 insertions(+), 4 deletions(-)
> 

> +++ b/nbd/client.c
> @@ -914,11 +914,16 @@ ssize_t nbd_send_request(QIOChannel *ioc, NBDRequest 
> *request)
>  return nbd_write(ioc, buf, sizeof(buf), NULL);
>  }
>  
> -ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp)
> +/* nbd_receive_reply
> + * Returns 1 on success
> + * 0 on eof, when no data was read from @ioc (errp is not set)
> + * < 0 on fail

Similar to the previous patch, I'd like to squash in:

diff --git i/nbd/client.c w/nbd/client.c
index a1758a1931..f8c213bc96 100644
--- i/nbd/client.c
+++ w/nbd/client.c
@@ -916,8 +916,8 @@ ssize_t nbd_send_request(QIOChannel *ioc, NBDRequest
*request)

 /* nbd_receive_reply
  * Returns 1 on success
- * 0 on eof, when no data was read from @ioc (errp is not set)
- * < 0 on fail
+ * 0 on eof, when no data was read (errp is not set)
+ * negative errno on failure (errp is set)
  */
 int nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp)
 {

With that,
Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH for-2.11 v2] hw/ppc: CAS reset on early device hotplug

[Daniel Henrique Barboza]

This patch is a follow up on the discussions made in patch
"hw/ppc: disable hotplug before CAS is completed" that can be
found at [1].

At this moment, we do not support CPU/memory hotplug in early
boot stages, before CAS. When a hotplug occurs, the event is logged
in an internal RTAS event log queue and an IRQ pulse is fired. In
regular conditions, the guest handles the interrupt by executing
check_exception, fetching the generated hotplug event and enabling
the device for use.

In early boot, this IRQ isn't caught (SLOF does not handle hotplug
events), leaving the event in the rtas event log queue. If the guest
executes check_exception due to another hotplug event, the re-assertion
of the IRQ ends up de-queuing the first hotplug event as well. In short,
a device hotplugged before CAS is considered coldplugged by SLOF.
This leads to device misbehavior and, in some cases, guest kernel
Ooops when trying to unplug the device.

A proper fix would be to turn every device hotplugged before CAS
as a colplugged device. This is not trivial to do with the current
code base though - the FDT is written in the guest memory at
ppc_spapr_reset and can't be retrieved without adding extra state
(fdt_size for example) that will need to managed and migrated. Adding
the hotplugged DT in the middle of CAS negotiation via the updated DT
tree works with CPU devs, but panics the guest kernel at boot. Additional
analysis would be necessary for LMBs and PCI devices. There are
questions to be made in QEMU/SLOF/kernel level about how we can make
this change in a sustainable way.

Until we go all the way with the proper fix, this patch works around
the situation by issuing a CAS reset if a hotplugged device is detected
during CAS:

- the DRC conditions that warrant a CAS reset is the same as those that
triggers a DRC migration - the DRC must have a device attached and
the DRC state is not equal to its ready_state. With that in mind, this
patch makes use of 'spapr_drc_needed' to determine if a CAS reset
is needed.

- In the middle of CAS negotiations, the function
'spapr_hotplugged_dev_before_cas' goes through all the DRCs to see
if there are any DRC that requires a reset, using spapr_drc_needed. If
that happens, returns '1' in 'spapr_h_cas_compose_response' which will set
spapr->cas_reboot to true, causing the machine to reboot.

- a small fix was made in 'spapr_drc_needed' to change how we detect
a DRC device. Using dr_entity_sense worked for physical DRCs but,
for logical DRCs, it didn't cover the case where a logical DRC has
a drc->dev but the state is LOGICAL_UNUSABLE (e.g. a hotplugged CPU before
CAS). In this case, the dr_entity_sense of this DRC returns UNUSABLE and
spapr_drc_needed was return 'false' for a scenario what we would like
to migrate the DRC (or issue a CAS reset). Changing it to check for
drc->dev instead works for all DRC types.

- a new function called 'spapr_clear_pending_events' was created
and is being called inside ppc_spapr_reset. This function clears
the pending_events QTAILQ that holds the RTAS event logs. This prevents
old/deprecated events from persisting after a reset.

No changes are made for coldplug devices.

[1] http://lists.nongnu.org/archive/html/qemu-devel/2017-08/msg02855.html

Signed-off-by: Daniel Henrique Barboza 
---
 hw/ppc/spapr.c | 34 ++
 hw/ppc/spapr_drc.c |  5 ++---
 include/hw/ppc/spapr_drc.h |  1 +
 3 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index fb1e5e0..4b23ad3 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -790,6 +790,26 @@ out:
 return ret;
 }
 
+static bool spapr_hotplugged_dev_before_cas(void)
+{
+Object *drc_container, *obj;
+ObjectProperty *prop;
+ObjectPropertyIterator iter;
+
+drc_container = container_get(object_get_root(), "/dr-connector");
+object_property_iter_init(, drc_container);
+while ((prop = object_property_iter_next())) {
+if (!strstart(prop->type, "link<", NULL)) {
+continue;
+}
+obj = object_property_get_link(drc_container, prop->name, NULL);
+if (spapr_drc_needed(obj)) {
+return true;
+}
+}
+return false;
+}
+
 int spapr_h_cas_compose_response(sPAPRMachineState *spapr,
  target_ulong addr, target_ulong size,
  sPAPROptionVector *ov5_updates)
@@ -797,6 +817,10 @@ int spapr_h_cas_compose_response(sPAPRMachineState *spapr,
 void *fdt, *fdt_skel;
 sPAPRDeviceTreeUpdateHeader hdr = { .version_id = 1 };
 
+if (spapr_hotplugged_dev_before_cas()) {
+return 1;
+}
+
 size -= sizeof(hdr);
 
 /* Create sceleton */
@@ -1369,6 +1393,15 @@ static void find_unknown_sysbus_device(SysBusDevice 
*sbdev, void *opaque)
 }
 }
 
+static void spapr_clear_pending_events(sPAPRMachineState *spapr)
+{
+sPAPREventLogEntry *entry = NULL;
+
+

[Qemu-devel] [PATCH for-2.11 v2] hw/ppc: CAS reset on early device hotplug

[Daniel Henrique Barboza]

v2:
- rebased with ppc-for-2.11
- function 'spapr_cas_completed' dropped
- function 'spapr_drc_needed' made public and it's now used inside
  'spapr_hotplugged_dev_before_cas'
- 'spapr_drc_needed' was changed to support the migration of logical
  DRCs with devs attached in UNUSED state
- new function: 'spapr_clear_pending_events'. This function is used
  inside ppc_spapr_reset to reset the pending_events QTAILQ

Daniel Henrique Barboza (1):
  hw/ppc: CAS reset on early device hotplug

 hw/ppc/spapr.c | 34 ++
 hw/ppc/spapr_drc.c |  5 ++---
 include/hw/ppc/spapr_drc.h |  1 +
 3 files changed, 37 insertions(+), 3 deletions(-)

-- 
2.9.4

[Qemu-devel] [Bug 1711316] Re: fbsd strip(1) segfaults on aarch64

[Gergely Czuczy]

*** This bug is a duplicate of bug 1713066 ***
https://bugs.launchpad.net/bugs/1713066

** This bug has been marked a duplicate of bug 1713066
   Incorrect handling of aarch64 ldp in some cases

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

Title:
  fbsd strip(1) segfaults on aarch64

Status in QEMU:
  New

Bug description:
  Hello,

  During port builds ld.lld(devel/boost-libs, www/node), and strip
  (textproc/libxml2 on xmlcatalog) are segfaulting. On physical boxes
  these things do work, as they should:

  root@build-aarch64:~ # strip xmlcatalog
  Segmentation fault (core dumped)

  All the cores fail at the same assembly instruction, here's strip's backtrace:
  # lldb -c strip.core strip
  (lldb) target create "strip" --core "strip.core"
  Core file '/root/strip.core' (aarch64) was loaded.
  (lldb) t 1
  * thread #1, name = 'strip', stop reason = signal SIGSEGV
  frame #0: 0x40312f40 libc.so.7`memcpy + 192
  libc.so.7`memcpy:
  ->  0x40312f40 <+192>: ldpx4, x3, [x4, #-0x10]
  0x40312f44 <+196>: stpx6, x7, [x0]
  0x40312f48 <+200>: stpx8, x9, [x0, #0x10]
  0x40312f4c <+204>: stpx10, x11, [x0, #0x20]
  (lldb) bt
  * thread #1, name = 'strip', stop reason = signal SIGSEGV
* frame #0: 0x40312f40 libc.so.7`memcpy + 192
  frame #1: 0x4017ac70 
libelf.so.2`_libelf_cvt_HALF_tom(dst=, dsz=, 
src=, count=, byteswap=) at 
libelf_convert.c:794
  frame #2: 0x40177b34 
libelf.so.2`elf_getdata(s=0x40f355a0, ed=) at 
elf_data.c:155
  frame #3: 0x000283d4 strip`copy_data(s=0x40f36a40) at 
sections.c:1176
  frame #4: 0x00027ea4 strip`copy_content(ecp=) at 
sections.c:594
  frame #5: 0x00023ff4 strip`create_elf(ecp=0x40ed6000) at 
main.c:381
  frame #6: 0x0002558c strip`create_file(ecp=, 
src=, dst=) at main.c:705
  frame #7: 0x00024e20 strip`main [inlined] 
strip_main(argc=, argv=) at main.c:1192
  frame #8: 0x00024cc8 strip`main(argc=, 
argv=) at main.c:1590
  frame #9: 0x00020190 strip`__start + 376
  frame #10: 0x40050018 ld-elf.so.1`.rtld_start at rtld_start.S:41

  Host system information:
  # uname -a
  FreeBSD marvin.harmless.hu 11.0-STABLE FreeBSD 11.0-STABLE #0 r311927: Wed 
Jan 11 14:53:55 CET 2017 
t...@marvin.harmless.hu:/usr/obj/usr/src/sys/MARVIN  amd64

  # qemu-system-aarch64 --version
  QEMU emulator version 2.9.0
  Copyright (c) 2003-2017 Fabrice Bellard and the QEMU Project developers
  # pkg info | grep qemu
  qemu-2.9.0 QEMU CPU Emulator
  I also had this happening with 2.8.0 and 2.8.1

  Guest information:
  # uname -a
  FreeBSD build-aarch64.marvin.harmless.hu 12.0-CURRENT FreeBSD 12.0-CURRENT #0 
r322578: Wed Aug 16 18:08:43 CEST 2017 
t...@marvin.harmless.hu:/tank/rpi3/obj/arm64.aarch64/tank/rpi3/src/sys/GENERIC-NODEBUG
  arm64

  Startup:
  zdev=/dev/zvol/tank/rpi3/qemu-image
  qemu-system-aarch64 -m 4096M -cpu cortex-a57 -M virt  \
  -accel tcg,thread=single \
  -bios QEMU_EFI.fd -serial telnet::,server -nographic \
  -drive if=none,file=${image},id=hd0,format=raw \
  -device virtio-blk-device,drive=hd0 \
  -device e1000,netdev=net0 \
  -netdev 
tap,id=net0,ifname=tap0,script=/tank/rpi3/build/qemu-ifup.sh

  Tested with cortex A53 as well, does the same.

  I've attached a test image to reproduce with (340MB), if it's too big for an 
attachment, it can be downloaded from here:
  http://czg.harmless.hu/qemu-bug-stripsigsegv/image.gz

  Reproduction steps:
  1) log in as root, no password
  2) strip xmlcatalog, it will segfault

  For a full reproduction with ld.lld as well, you need a ports tree, it's 
suggested to attach a bigger volume to /usr/ports over NFS first (it might need 
more space than the image has). Steps for it:
  1) portsnap fetch extract
  2) make -C /usr/ports/devel/boost-libs package-recursive
  3) make -C /usr/ports/textproc/libxml2 package-recursive
  4) make -C /usr/ports/www/node package-recursive

  Boost and node can take a day or more in a qemu VM. The build-time
  options I've be set already, /var/db/ports is populated, so you
  shouldn't have questions asked during the builds.

  I've saved the core dumps, those can be found at /root/cores/ in the
  image.

  I hope I've submitted all the required information. If anything else
  is needed, please let me know.

  Best regards,
  Gergely

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

[Qemu-devel] [Bug 1713066] Re: Incorrect handling of aarch64 ldp in some cases

[Gergely Czuczy]

This might be the cause for my bugreport:
https://bugs.launchpad.net/qemu/+bug/1711316

Marked mine as a duplicate of this, please correct me if I'm wrong.

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

Title:
  Incorrect handling of aarch64 ldp in some cases

Status in QEMU:
  New

Bug description:
  In some cases the ldp instruction (and presumably other multi-register
  loads and stores) can behave incorrectly.

  Given the following instruction:
  ldp x0, x1, [x0]

  This will load two 64 bit values from memory, however if each location
  to load is on a different page and the second page is unmapped this
  will raise an exception. When this happens x0 has already been updated
  so after the exception handler has run the operating system will try
  to rerun the instruction. QEMU will now try to perform an invalid load
  and raise a new exception.

  I believe this is incorrect as section D.1.14.5 of the ARMv8 reference
  manual B.a states that, on taking an exception, registers used in the
  generation of addresses are restored to their initial value, so x0
  shouldn't be changed, where x1 can be un an unknown state.

  I found the issue running FreeBSD with the cortex-strings
  implementation of memcpy. This uses a similar instruction when copying
  between 64 and 96 bytes.

  I've observed this on:
  QEMU emulator version 2.5.0 (Debian 1:2.5+dfsg-5ubuntu10.14), Copyright (c) 
2003-2008 Fabrice Bellard

  And checked I still get the same behaviour on:
  QEMU emulator version 2.9.94 (v2.10.0-rc4-dirty)
  Git revision: 248b23735645f7cbb503d9be6f5bf825f2a603ab

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

Re: [Qemu-devel] [Qemu-block] [PATCH 0/3] nbd-client: enter read_reply_co during init to avoid crash

[Paolo Bonzini]

On 25/08/2017 17:08, Eric Blake wrote:
> On 08/24/2017 11:05 AM, Stefan Hajnoczi wrote:
>> On Thu, Aug 24, 2017 at 4:52 PM, Eric Blake  wrote:
>>> On 08/24/2017 10:33 AM, Stefan Hajnoczi wrote:
 See Patch 1 for the segfault fix.  Patches 2 & 3 add qemu-iotests coverage.

 This is a rare crash that we'll probably only see in testing.  It only 
 seems to
 happen with UNIX domain sockets.
>>>
>>> Rare enough that I don't think it is 2.10-rc4 material.  I'll review,
>>> and if I don't find anything needing a respin, I'll add it to my NBD
>>> queue for 2.11.
>>
>> Great.  This is 2.11 material.
> 
> Hmm - I wonder if https://bugs.launchpad.net/qemu/+bug/1712818 is
> related to this.  I really don't want to delay 2.10 further (downstream
> distros can backport fixes) - but comment 3 stating that crashes are
> less frequent but still possible with -rc4 is not good.

Well, I won't be able to review or write the patch, so I think it's
better to wait.

Paolo



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 1/5] pci: INTERFACE_LEGACY_PCI_DEVICE and INTERFACE_PCIE_DEVICE interfaces

[Alex Williamson]

On Wed, 23 Aug 2017 19:14:41 -0300
Eduardo Habkost  wrote:

> Those two interfaces will be used to indicate which device types
> support legacy PCI or PCI-express buses.  Management software
> will be able to use the qom-list-types QMP command to query that
> information.

Nit, while "legacy PCI" and "conventional PCI" have about the same
number of google hits, I believe the latter is the more correct term.
The quality of hits is certainly a lot better with "conventional".
Calling something "legacy" also spurs an immediate negative reaction
for some folks, "conventional" is more neutral.  Thanks,

Alex

> 
> Signed-off-by: Eduardo Habkost 
> ---
>  include/hw/pci/pci.h |  6 ++
>  hw/pci/pci.c | 12 
>  2 files changed, 18 insertions(+)
> 
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index e598b09..f5e8ab9 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -195,6 +195,12 @@ enum {
>  #define PCI_DEVICE_GET_CLASS(obj) \
>   OBJECT_GET_CLASS(PCIDeviceClass, (obj), TYPE_PCI_DEVICE)
>  
> +/* Interface implemented by devices that can be plugged on PCIe buses */
> +#define INTERFACE_PCIE_DEVICE "pci-express-device"
> +
> +/* Interface implemented by devices that can be plugged on legacy PCI buses 
> */
> +#define INTERFACE_LEGACY_PCI_DEVICE "legacy-pci-device"
> +
>  typedef struct PCIINTxRoute {
>  enum {
>  PCI_INTX_ENABLED,
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 258fbe5..baa3429 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -168,6 +168,16 @@ static const TypeInfo pci_bus_info = {
>  .class_init = pci_bus_class_init,
>  };
>  
> +static const TypeInfo pcie_interface_info = {
> +.name  = INTERFACE_PCIE_DEVICE,
> +.parent= TYPE_INTERFACE,
> +};
> +
> +static const TypeInfo legacy_pci_interface_info = {
> +.name  = INTERFACE_LEGACY_PCI_DEVICE,
> +.parent= TYPE_INTERFACE,
> +};
> +
>  static const TypeInfo pcie_bus_info = {
>  .name = TYPE_PCIE_BUS,
>  .parent = TYPE_PCI_BUS,
> @@ -2645,6 +2655,8 @@ static void pci_register_types(void)
>  {
>  type_register_static(_bus_info);
>  type_register_static(_bus_info);
> +type_register_static(_pci_interface_info);
> +type_register_static(_interface_info);
>  type_register_static(_device_type_info);
>  }
>  

[Qemu-devel] [PATCH] xio3130_downstream: Report error if pcie_chassis_add_slot() failed

[Eduardo Habkost]

On commit f8cd1b02 ("pci: Convert to realize"), no error_set*()
call was added for the pcie_chassis_add_slot() error case.
pcie_chassis_add_slot() errors get ignored, making QEMU crash
later.  e.g.:

  $ qemu-system-x86_64 -device ioh3420 -device xio3130-downstream
  qemu-system-x86_64: memory.c:2166: memory_region_del_subregion: Assertion 
`subregion->container == mr' failed.
  Aborted (core dumped)

Fix it by reporting the error using error_setg().

Fixes: f8cd1b0201c41d88bb97dcafb80348a0e88d8805
Signed-off-by: Eduardo Habkost 
---
 hw/pci-bridge/xio3130_downstream.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/pci-bridge/xio3130_downstream.c 
b/hw/pci-bridge/xio3130_downstream.c
index e706f36..5a882b0 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -94,6 +94,7 @@ static void xio3130_downstream_realize(PCIDevice *d, Error 
**errp)
 pcie_chassis_create(s->chassis);
 rc = pcie_chassis_add_slot(s);
 if (rc < 0) {
+error_setg(errp, "Can't add chassis slot, error %d", rc);
 goto err_pcie_cap;
 }
 
-- 
2.9.4

Re: [Qemu-devel] [PATCH 4/5] pci: Add INTERFACE_LEGACY_PCI_DEVICE to legacy PCI devices

[Eduardo Habkost]

CCing maintainers of affected devices (sorry for not CCing you
before).

On Wed, Aug 23, 2017 at 07:14:44PM -0300, Eduardo Habkost wrote:
> Add INTERFACE_LEGACY_PCI_DEVICE to all direct subtypes of
> TYPE_PCI_DEVICE, except:
> 
> 1) The ones that already have INTERFACE_PCIE_DEVICE set:
> 
> * base-xhci
> * e1000e
> * nvme
> * pvscsi
> * vfio-pci
> * virtio-pci
> * vmxnet3
> 
> 2) base-pci-bridge
> 
> Not all PCI bridges are legacy PCI devices, so
> INTERFACE_LEGACY_PCI_DEVICE is added only to the subtypes that
> are actually legacy PCI devices:
> 
> * dec-21154-p2p-bridge
> * i82801b11-bridge
> * pbm-bridge
> * pci-bridge
> 
> The direct subtypes of base-pci-bridge not touched by this patch
> are:
> 
> * xilinx-pcie-root: Already marked as PCIe-only device.
> * pcie-port: all non-abstract subtypes of pcie-port are already
>   marked as PCIe-only devices.
> 
> 3) megasas-base
> 
> Not all megasas devices are legacy PCI devices, so the interface
> names are added to the subclasses registered by
> megasas_register_types(), according to information in the
> megasas_devices[] array.
> 
> "megasas-gen2" already implements INTERFACE_PCIE_DEVICE, so add
> INTERFACE_LEGACY_PCI_DEVICE only to "megasas".
> 
> Signed-off-by: Eduardo Habkost 
> ---
>  hw/acpi/piix4.c |  1 +
>  hw/audio/ac97.c |  4 
>  hw/audio/es1370.c   |  4 
>  hw/audio/intel-hda.c|  4 
>  hw/char/serial-pci.c| 12 
>  hw/display/cirrus_vga.c |  4 
>  hw/display/qxl.c|  4 
>  hw/display/sm501.c  |  4 
>  hw/display/vga-pci.c|  4 
>  hw/display/vmware_vga.c |  4 
>  hw/i2c/smbus_ich9.c |  4 
>  hw/i386/amd_iommu.c |  4 
>  hw/i386/kvm/pci-assign.c|  4 
>  hw/i386/pc_piix.c   |  4 
>  hw/i386/xen/xen_platform.c  |  4 
>  hw/i386/xen/xen_pvdevice.c  |  4 
>  hw/ide/ich.c|  4 
>  hw/ide/pci.c|  4 
>  hw/ipack/tpci200.c  |  4 
>  hw/isa/i82378.c |  4 
>  hw/isa/lpc_ich9.c   |  1 +
>  hw/isa/piix4.c  |  4 
>  hw/isa/vt82c686.c   | 16 
>  hw/mips/gt64xxx_pci.c   |  4 
>  hw/misc/edu.c   |  5 +
>  hw/misc/ivshmem.c   |  4 
>  hw/misc/macio/macio.c   |  4 
>  hw/misc/pci-testdev.c   |  4 
>  hw/net/e1000.c  |  4 
>  hw/net/eepro100.c   |  4 
>  hw/net/ne2000.c |  4 
>  hw/net/pcnet-pci.c  |  4 
>  hw/net/rocker/rocker.c  |  4 
>  hw/net/rtl8139.c|  4 
>  hw/pci-bridge/dec.c |  8 
>  hw/pci-bridge/i82801b11.c   |  4 
>  hw/pci-bridge/pci_bridge_dev.c  |  1 +
>  hw/pci-bridge/pci_expander_bridge.c |  8 
>  hw/pci-host/apb.c   |  8 
>  hw/pci-host/bonito.c|  4 
>  hw/pci-host/gpex.c  |  4 
>  hw/pci-host/grackle.c   |  4 
>  hw/pci-host/piix.c  |  8 
>  hw/pci-host/ppce500.c   |  4 
>  hw/pci-host/prep.c  |  4 
>  hw/pci-host/q35.c   |  4 
>  hw/pci-host/uninorth.c  | 16 
>  hw/pci-host/versatile.c |  4 
>  hw/ppc/ppc4xx_pci.c |  4 
>  hw/scsi/esp-pci.c   |  4 
>  hw/scsi/lsi53c895a.c|  4 
>  hw/scsi/megasas.c   |  4 
>  hw/scsi/mptsas.c|  4 
>  hw/sd/sdhci.c   |  4 
>  hw/sh4/sh_pci.c |  4 
>  hw/sparc64/sun4u.c  |  4 
>  hw/usb/hcd-ehci-pci.c   |  4 
>  hw/usb/hcd-ohci.c   |  4 
>  hw/usb/hcd-uhci.c   |  4 
>  hw/vfio/pci-quirks.c|  4 
>  hw/watchdog/wdt_i6300esb.c  |  4 
>  hw/xen/xen_pt.c |  4 
>  62 files changed, 288 insertions(+)
> 
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index f276967..defe98a 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -723,6 +723,7 @@ static const TypeInfo piix4_pm_info = {
>  .interfaces = (InterfaceInfo[]) {
>  { TYPE_HOTPLUG_HANDLER },
>  { TYPE_ACPI_DEVICE_IF },
> +{ INTERFACE_LEGACY_PCI_DEVICE },
>  { }
>  }
>  };
> diff --git a/hw/audio/ac97.c b/hw/audio/ac97.c
> index 959c786..6027e15 100644
> --- a/hw/audio/ac97.c
> +++ b/hw/audio/ac97.c
> @@ -1431,6 +1431,10 @@ static const TypeInfo ac97_info = {
>  .parent= 

Re: [Qemu-devel] [PATCH 3/5] pci: Add INTERFACE_PCIE_DEVICE to all PCIe devices

[Eduardo Habkost]

CCing maintainers of affected devices (sorry for not CCing you
before).

On Wed, Aug 23, 2017 at 07:14:43PM -0300, Eduardo Habkost wrote:
> Change all devices that set is_express=1 to implement
> INTERFACE_PCIE_DEVICE.
> 
> Signed-off-by: Eduardo Habkost 
> ---
>  hw/block/nvme.c| 4 
>  hw/net/e1000e.c| 4 
>  hw/pci-bridge/pcie_root_port.c | 4 
>  hw/pci-bridge/xio3130_downstream.c | 4 
>  hw/pci-bridge/xio3130_upstream.c   | 4 
>  hw/pci-host/xilinx-pcie.c  | 4 
>  hw/scsi/megasas.c  | 6 ++
>  hw/usb/hcd-xhci.c  | 4 
>  8 files changed, 34 insertions(+)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 6071dc1..26d58b6 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1110,6 +1110,10 @@ static const TypeInfo nvme_info = {
>  .instance_size = sizeof(NvmeCtrl),
>  .class_init= nvme_class_init,
>  .instance_init = nvme_instance_init,
> +.interfaces = (InterfaceInfo[]) {
> +{ INTERFACE_PCIE_DEVICE },
> +{ }
> +},
>  };
>  
>  static void nvme_register_types(void)
> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
> index 6c42b44..81f7934 100644
> --- a/hw/net/e1000e.c
> +++ b/hw/net/e1000e.c
> @@ -708,6 +708,10 @@ static const TypeInfo e1000e_info = {
>  .instance_size = sizeof(E1000EState),
>  .class_init = e1000e_class_init,
>  .instance_init = e1000e_instance_init,
> +.interfaces = (InterfaceInfo[]) {
> +{ INTERFACE_PCIE_DEVICE },
> +{ }
> +},
>  };
>  
>  static void e1000e_register_types(void)
> diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
> index 4d588cb..9b6e4ce 100644
> --- a/hw/pci-bridge/pcie_root_port.c
> +++ b/hw/pci-bridge/pcie_root_port.c
> @@ -161,6 +161,10 @@ static const TypeInfo rp_info = {
>  .class_init= rp_class_init,
>  .abstract  = true,
>  .class_size = sizeof(PCIERootPortClass),
> +.interfaces = (InterfaceInfo[]) {
> +{ INTERFACE_PCIE_DEVICE },
> +{ }
> +},
>  };
>  
>  static void rp_register_types(void)
> diff --git a/hw/pci-bridge/xio3130_downstream.c 
> b/hw/pci-bridge/xio3130_downstream.c
> index e706f36..7d2f762 100644
> --- a/hw/pci-bridge/xio3130_downstream.c
> +++ b/hw/pci-bridge/xio3130_downstream.c
> @@ -195,6 +195,10 @@ static const TypeInfo xio3130_downstream_info = {
>  .name  = "xio3130-downstream",
>  .parent= TYPE_PCIE_SLOT,
>  .class_init= xio3130_downstream_class_init,
> +.interfaces = (InterfaceInfo[]) {
> +{ INTERFACE_PCIE_DEVICE },
> +{ }
> +},
>  };
>  
>  static void xio3130_downstream_register_types(void)
> diff --git a/hw/pci-bridge/xio3130_upstream.c 
> b/hw/pci-bridge/xio3130_upstream.c
> index a052224..227997c 100644
> --- a/hw/pci-bridge/xio3130_upstream.c
> +++ b/hw/pci-bridge/xio3130_upstream.c
> @@ -166,6 +166,10 @@ static const TypeInfo xio3130_upstream_info = {
>  .name  = "x3130-upstream",
>  .parent= TYPE_PCIE_PORT,
>  .class_init= xio3130_upstream_class_init,
> +.interfaces = (InterfaceInfo[]) {
> +{ INTERFACE_PCIE_DEVICE },
> +{ }
> +},
>  };
>  
>  static void xio3130_upstream_register_types(void)
> diff --git a/hw/pci-host/xilinx-pcie.c b/hw/pci-host/xilinx-pcie.c
> index 4613dda..7659253 100644
> --- a/hw/pci-host/xilinx-pcie.c
> +++ b/hw/pci-host/xilinx-pcie.c
> @@ -317,6 +317,10 @@ static const TypeInfo xilinx_pcie_root_info = {
>  .parent = TYPE_PCI_BRIDGE,
>  .instance_size = sizeof(XilinxPCIERoot),
>  .class_init = xilinx_pcie_root_class_init,
> +.interfaces = (InterfaceInfo[]) {
> +{ INTERFACE_PCIE_DEVICE },
> +{ }
> +},
>  };
>  
>  static void xilinx_pcie_register(void)
> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index 734fdae..3641c30 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -2451,6 +2451,7 @@ typedef struct MegasasInfo {
>  int osts;
>  const VMStateDescription *vmsd;
>  Property *props;
> +InterfaceInfo *interfaces;
>  } MegasasInfo;
>  
>  static struct MegasasInfo megasas_devices[] = {
> @@ -2480,6 +2481,10 @@ static struct MegasasInfo megasas_devices[] = {
>  .is_express = true,
>  .vmsd = _megasas_gen2,
>  .props = megasas_properties_gen2,
> +.interfaces = (InterfaceInfo[]) {
> +{ INTERFACE_PCIE_DEVICE },
> +{ }
> +},
>  }
>  };
>  
> @@ -2531,6 +2536,7 @@ static void megasas_register_types(void)
>  type_info.parent = TYPE_MEGASAS_BASE;
>  type_info.class_data = (void *)info;
>  type_info.class_init = megasas_class_init;
> +type_info.interfaces = info->interfaces;
>  
>  type_register(_info);
>  }
> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> index 204ea69..d95ed4f 100644
> --- a/hw/usb/hcd-xhci.c
> +++ 

Re: [Qemu-devel] [PATCH 2/5] pci: Add interface names to hybrid PCI devices

[Eduardo Habkost]

CCing maintainers of affected devices (sorry for not CCing you
before)

On Wed, Aug 23, 2017 at 07:14:42PM -0300, Eduardo Habkost wrote:
> The following devices support both PCIe and legacy PCI, by
> including special code to handle the QEMU_PCI_CAP_EXPRESS flag:
> 
> * vfio-pci (is_express=1, but legacy PCI handled by
>   vfio_populate_device())
> * vmxnet3 (is_express=0, but PCIe handled by vmxnet3_realize())
> * pvscsi (is_express=0, but PCIe handled by pvscsi_realize())
> * virtio-pci (is_express=0, but PCIe handled by
>   virtio_pci_dc_realize(), and additional legacy PCI code at
>   virtio_pci_realize())
> 
> Signed-off-by: Eduardo Habkost 
> ---
>  hw/net/vmxnet3.c   | 5 +
>  hw/scsi/vmw_pvscsi.c   | 2 ++
>  hw/vfio/pci.c  | 5 +
>  hw/virtio/virtio-pci.c | 5 +
>  4 files changed, 17 insertions(+)
> 
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index a19a7a3..61feacf 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -2651,6 +2651,11 @@ static const TypeInfo vmxnet3_info = {
>  .instance_size = sizeof(VMXNET3State),
>  .class_init= vmxnet3_class_init,
>  .instance_init = vmxnet3_instance_init,
> +.interfaces = (InterfaceInfo[]) {
> +{ INTERFACE_PCIE_DEVICE },
> +{ INTERFACE_LEGACY_PCI_DEVICE },
> +{ }
> +},
>  };
>  
>  static void vmxnet3_register_types(void)
> diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
> index 77d8b6f..40dfffa 100644
> --- a/hw/scsi/vmw_pvscsi.c
> +++ b/hw/scsi/vmw_pvscsi.c
> @@ -1300,6 +1300,8 @@ static const TypeInfo pvscsi_info = {
>  .class_init= pvscsi_class_init,
>  .interfaces = (InterfaceInfo[]) {
>  { TYPE_HOTPLUG_HANDLER },
> +{ INTERFACE_PCIE_DEVICE },
> +{ INTERFACE_LEGACY_PCI_DEVICE },
>  { }
>  }
>  };
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 31e1edf..2b21391 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3023,6 +3023,11 @@ static const TypeInfo vfio_pci_dev_info = {
>  .class_init = vfio_pci_dev_class_init,
>  .instance_init = vfio_instance_init,
>  .instance_finalize = vfio_instance_finalize,
> +.interfaces = (InterfaceInfo[]) {
> +{ INTERFACE_PCIE_DEVICE },
> +{ INTERFACE_LEGACY_PCI_DEVICE },
> +{ }
> +},
>  };
>  
>  static void register_vfio_pci_dev_type(void)
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 8b0d6b6..8c0b6bf 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1958,6 +1958,11 @@ static const TypeInfo virtio_pci_info = {
>  .class_init= virtio_pci_class_init,
>  .class_size= sizeof(VirtioPCIClass),
>  .abstract  = true,
> +.interfaces = (InterfaceInfo[]) {
> +{ INTERFACE_PCIE_DEVICE },
> +{ INTERFACE_LEGACY_PCI_DEVICE },
> +{ }
> +},
>  };
>  
>  /* virtio-blk-pci */
> -- 
> 2.9.4
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 02/17] nbd/client: refactor nbd_read_eof

[Eric Blake]

On 08/07/2017 07:05 AM, Vladimir Sementsov-Ogievskiy wrote:
> 07.08.2017 14:42, Eric Blake wrote:
>> On 08/04/2017 10:14 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Refactor nbd_read_eof to return 1 on success, 0 on eof, when no
>>> data was read and <0 for other cases, because returned size of
>>> read data is not actually used.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>>> ---

>>> + * Returns 1 on success
>>> + * 0 on eof, when no data was read (errp is not set)
>>> + * -EINVAL on eof, when some data < @size was read until eof
>>> + * < 0 on read fail
>> In general, mixing negative errno value and generic < 0 in the same
>> function is most likely ambiguous.
> 
> Hmm, but this is entirely what we do so often:
> 
> if (,,) return -EINVAL;
> 
> return some_other_func().
> 
> last two lines may be rewritten like this:
> + * < 0 on fail

Or even better as 'negative errno on failure'.

Here's what I'm proposing to squash in, at which point you have:

Reviewed-by: Eric Blake 

diff --git i/nbd/nbd-internal.h w/nbd/nbd-internal.h
index 3fb0b6098a..03549e3f39 100644
--- i/nbd/nbd-internal.h
+++ w/nbd/nbd-internal.h
@@ -80,8 +80,7 @@
  * Tries to read @size bytes from @ioc.
  * Returns 1 on success
  * 0 on eof, when no data was read (errp is not set)
- * -EINVAL on eof, when some data < @size was read until eof
- * < 0 on read fail
+ * negative errno on failure (errp is set)
  */
 static inline int nbd_read_eof(QIOChannel *ioc, void *buffer, size_t size,
Error **errp)
@@ -95,7 +94,7 @@ static inline int nbd_read_eof(QIOChannel *ioc, void
*buffer, size_t size,
  * that this is coroutine-safe.
  */

-assert(size > 0);
+assert(size);

 ret = nbd_rwv(ioc, , 1, size, true, errp);
 if (ret <= 0) {


>>> +
>>> +if (ret != size) {
>>> +error_setg(errp, "End of file");
>>> +return -EINVAL;
>> and so is this. Which makes the function documentation not quite
>> accurate; you aren't mixing a generic < 0.
> 
> hmm.. my wordings are weird sometimes, sorry for that :(. and thank you
> for your patience.

Not a problem - I understand that English is not your native language,
so you are already a leg up on me (I'm nowhere near as competent in a
second language as you already are on English, even if review still
gives you grammar hints and other improvements).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 2/5] pci: Add interface names to hybrid PCI devices

[Eduardo Habkost]

On Wed, Aug 23, 2017 at 07:14:42PM -0300, Eduardo Habkost wrote:
> The following devices support both PCIe and legacy PCI, by
> including special code to handle the QEMU_PCI_CAP_EXPRESS flag:
> 
> * vfio-pci (is_express=1, but legacy PCI handled by
>   vfio_populate_device())
> * vmxnet3 (is_express=0, but PCIe handled by vmxnet3_realize())
> * pvscsi (is_express=0, but PCIe handled by pvscsi_realize())
> * virtio-pci (is_express=0, but PCIe handled by
>   virtio_pci_dc_realize(), and additional legacy PCI code at
>   virtio_pci_realize())

Oh, the rules are even messier than that: QEMU_PCI_CAP_EXPRESS
controls _some_ of the code that makes a device become a PCI
Express device, but not every case.

In addition to vmxnet3, pvscsi and virtio-pci, PCIe caps
initialization is conditional on hcd-xhci (see below).

This means xhci is also a hybrid device.  But it doesn't seem to
clear QEMU_PCI_CAP_EXPRESS.  Doesn't it mean pci_config_size() is
broken if xhci is plugged to a legacy PCI bus?  How does it
affect migration?

Full list of pci_endpoint_cap*_init() calls:

hw/usb/hcd-xhci.c-if (pci_bus_is_express(dev->bus) ||
hw/usb/hcd-xhci.c-xhci_get_flag(xhci, XHCI_FLAG_FORCE_PCIE_ENDCAP)) {
hw/usb/hcd-xhci.c:ret = pcie_endpoint_cap_init(dev, 0xa0);

hw/net/vmxnet3.c-if (pci_is_express(pci_dev)) {
hw/net/vmxnet3.c-if (pci_bus_is_express(pci_dev->bus)) {
hw/net/vmxnet3.c:pcie_endpoint_cap_init(pci_dev, 
VMXNET3_EXP_EP_OFFSET);
hw/net/vmxnet3.c-}

hw/scsi/megasas.c-if (pci_is_express(dev)) {
hw/scsi/megasas.c:pcie_endpoint_cap_init(dev, 0xa0);
hw/scsi/megasas.c-}

hw/scsi/vmw_pvscsi.c-if (pci_is_express(pci_dev) && 
pci_bus_is_express(pci_dev->bus)) {
hw/scsi/vmw_pvscsi.c:pcie_endpoint_cap_init(pci_dev, 
PVSCSI_EXP_EP_OFFSET);
hw/scsi/vmw_pvscsi.c-}

hw/virtio/virtio-pci.c-if (pcie_port && pci_is_express(pci_dev)) {
hw/virtio/virtio-pci.c-int pos;
hw/virtio/virtio-pci.c-
hw/virtio/virtio-pci.c:pos = pcie_endpoint_cap_init(pci_dev, 0);


Note: on megasas, pci_endpoitn_cap_init() is conditional on
pci_is_express(), but pci_is_express() will never be false on
megasas-gen2 (because QEMU_PCI_CAP_EXPRESS is always set).


Full list of unconditional pci_endpoint_cap*init() calls:

hw/block/nvme.c:pcie_endpoint_cap_init(>parent_obj, 0x80);

hw/net/e1000e.c:if (pcie_endpoint_cap_v1_init(pci_dev, e1000e_pcie_offset) 
< 0) {

hw/pci-bridge/pcie_root_port.c:rc = pcie_cap_init(d, rpc->exp_offset, 
PCI_EXP_TYPE_ROOT_PORT,
hw/pci-bridge/pcie_root_port.c-   p->port, errp);

hw/pci-bridge/xio3130_downstream.c:rc = pcie_cap_init(d, 
XIO3130_EXP_OFFSET, PCI_EXP_TYPE_DOWNSTREAM,
hw/pci-bridge/xio3130_downstream.c-   p->port, errp);

hw/pci-bridge/xio3130_upstream.c:rc = pcie_cap_init(d, XIO3130_EXP_OFFSET, 
PCI_EXP_TYPE_UPSTREAM,
hw/pci-bridge/xio3130_upstream.c-   p->port, errp);

hw/pci-host/xilinx-pcie.c:if (pcie_endpoint_cap_v1_init(dev, 0x80) < 0) {



> 
> Signed-off-by: Eduardo Habkost 
> ---
>  hw/net/vmxnet3.c   | 5 +
>  hw/scsi/vmw_pvscsi.c   | 2 ++
>  hw/vfio/pci.c  | 5 +
>  hw/virtio/virtio-pci.c | 5 +
>  4 files changed, 17 insertions(+)
> 
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index a19a7a3..61feacf 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -2651,6 +2651,11 @@ static const TypeInfo vmxnet3_info = {
>  .instance_size = sizeof(VMXNET3State),
>  .class_init= vmxnet3_class_init,
>  .instance_init = vmxnet3_instance_init,
> +.interfaces = (InterfaceInfo[]) {
> +{ INTERFACE_PCIE_DEVICE },
> +{ INTERFACE_LEGACY_PCI_DEVICE },
> +{ }
> +},
>  };
>  
>  static void vmxnet3_register_types(void)
> diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
> index 77d8b6f..40dfffa 100644
> --- a/hw/scsi/vmw_pvscsi.c
> +++ b/hw/scsi/vmw_pvscsi.c
> @@ -1300,6 +1300,8 @@ static const TypeInfo pvscsi_info = {
>  .class_init= pvscsi_class_init,
>  .interfaces = (InterfaceInfo[]) {
>  { TYPE_HOTPLUG_HANDLER },
> +{ INTERFACE_PCIE_DEVICE },
> +{ INTERFACE_LEGACY_PCI_DEVICE },
>  { }
>  }
>  };
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 31e1edf..2b21391 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3023,6 +3023,11 @@ static const TypeInfo vfio_pci_dev_info = {
>  .class_init = vfio_pci_dev_class_init,
>  .instance_init = vfio_instance_init,
>  .instance_finalize = vfio_instance_finalize,
> +.interfaces = (InterfaceInfo[]) {
> +{ INTERFACE_PCIE_DEVICE },
> +{ INTERFACE_LEGACY_PCI_DEVICE },
> +{ }
> +},
>  };
>  
>  static void register_vfio_pci_dev_type(void)
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 8b0d6b6..8c0b6bf 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1958,6 

Re: [Qemu-devel] [PATCH 07/17] block/nbd-client: refactor request send/receive

[Eric Blake]

On 08/25/2017 01:49 PM, Eric Blake wrote:
> On 08/04/2017 10:14 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Move nbd_co_receive_reply and nbd_coroutine_end calls into
>> nbd_co_send_request and rename the latter to just nbd_co_request.
>>
>> This removes code duplications in nbd_client_co_{pwrite,pread,...}
>> functions. Also this is needed for further refactoring.
>>

>> -static int nbd_co_send_request(BlockDriverState *bs,
>> -   NBDRequest *request,
>> -   QEMUIOVector *qiov)
>> +static void nbd_co_receive_reply(NBDClientSession *s,
>> + NBDRequest *request,
>> + NBDReply *reply,
>> + QEMUIOVector *qiov);
>> +static void nbd_coroutine_end(BlockDriverState *bs,
>> +  NBDRequest *request);
> 
> Is it possible to organize the functions in topological order so that we
> don't need forward declarations of static functions?  (If there is
> mutual recursion, you need the forward declaration; but other than that,
> I like reading the building blocks first rather than skipping around)

Answering myself: patch 9 inlines nbd_co_receive_reply into its lone
caller, eliminating the need for a forward reference, and it is less
code churn to have a temporary forward declaration than it is to move
the function body up and then back down.  (Maybe the commit message
could hint that nbd_co_receive_reply will later be inlined)

> 
> Otherwise,
> Reviewed-by: Eric Blake 
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 09/17] block/nbd-client: move nbd_co_receive_reply content into nbd_co_request

[Eric Blake]

On 08/04/2017 10:14 AM, Vladimir Sementsov-Ogievskiy wrote:
> Move code from nbd_co_receive_reply into nbd_co_request. This simplify

s/simplify/simplifies/

> things, makes further refactoring possible. Also, a function starting

s/makes/and makes/

> with qemu_coroutine_yield is weird.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/nbd-client.c | 33 ++---
>  1 file changed, 10 insertions(+), 23 deletions(-)
> 

Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 07/17] block/nbd-client: refactor request send/receive

[Eric Blake]

On 08/04/2017 10:14 AM, Vladimir Sementsov-Ogievskiy wrote:
> Move nbd_co_receive_reply and nbd_coroutine_end calls into
> nbd_co_send_request and rename the latter to just nbd_co_request.
> 
> This removes code duplications in nbd_client_co_{pwrite,pread,...}
> functions. Also this is needed for further refactoring.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/nbd-client.c | 89 
> --
>  1 file changed, 33 insertions(+), 56 deletions(-)

The diffstat shows this is a nice improvement.
> +++ b/block/nbd-client.c
> @@ -112,12 +112,20 @@ static coroutine_fn void nbd_read_reply_entry(void 
> *opaque)
>  s->read_reply_co = NULL;
>  }
>  
> -static int nbd_co_send_request(BlockDriverState *bs,
> -   NBDRequest *request,
> -   QEMUIOVector *qiov)
> +static void nbd_co_receive_reply(NBDClientSession *s,
> + NBDRequest *request,
> + NBDReply *reply,
> + QEMUIOVector *qiov);
> +static void nbd_coroutine_end(BlockDriverState *bs,
> +  NBDRequest *request);

Is it possible to organize the functions in topological order so that we
don't need forward declarations of static functions?  (If there is
mutual recursion, you need the forward declaration; but other than that,
I like reading the building blocks first rather than skipping around)

Otherwise,
Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 08/17] block/nbd-client: rename nbd_recv_coroutines_enter_all

[Eric Blake]

On 08/04/2017 10:14 AM, Vladimir Sementsov-Ogievskiy wrote:
> Rename nbd_recv_coroutines_enter_all to nbd_recv_coroutines_wake_all,
> as it most probably just add all recv coroutines into co_queue_wakeup,
> not directly enter them.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/nbd-client.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake 

> 
> diff --git a/block/nbd-client.c b/block/nbd-client.c
> index c9ade9b517..8ad2264a40 100644
> --- a/block/nbd-client.c
> +++ b/block/nbd-client.c
> @@ -34,7 +34,7 @@
>  #define HANDLE_TO_INDEX(bs, handle) ((handle) ^ ((uint64_t)(intptr_t)bs))
>  #define INDEX_TO_HANDLE(bs, index)  ((index)  ^ ((uint64_t)(intptr_t)bs))
>  
> -static void nbd_recv_coroutines_enter_all(NBDClientSession *s)
> +static void nbd_recv_coroutines_wake_all(NBDClientSession *s)
>  {
>  int i;
>  
> @@ -108,7 +108,7 @@ static coroutine_fn void nbd_read_reply_entry(void 
> *opaque)
>  }
>  
>  s->reply.handle = 0;
> -nbd_recv_coroutines_enter_all(s);
> +nbd_recv_coroutines_wake_all(s);
>  s->read_reply_co = NULL;
>  }
>  
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 5/5] pci: Validate interfaces on base_class_init

[Alistair Francis]

On Wed, Aug 23, 2017 at 3:14 PM, Eduardo Habkost  wrote:
> Make sure we don't forget to add the legacy-PCI or PCIe interface
> names on PCI device classes in the future.
>
> Signed-off-by: Eduardo Habkost 

Reviewed-by: Alistair Francis 

Thanks,
Alistair

> ---
>  hw/pci/pci.c | 12 
>  1 file changed, 12 insertions(+)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index baa3429..7ac5cc6 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2525,6 +2525,17 @@ static void pci_device_class_init(ObjectClass *klass, 
> void *data)
>  pc->realize = pci_default_realize;
>  }
>
> +static void pci_device_class_base_init(ObjectClass *klass, void *data)
> +{
> +if (!object_class_is_abstract(klass)) {
> +ObjectClass *legacy =
> +object_class_dynamic_cast(klass, INTERFACE_LEGACY_PCI_DEVICE);
> +ObjectClass *pcie =
> +object_class_dynamic_cast(klass, INTERFACE_PCIE_DEVICE);
> +assert(legacy || pcie);
> +}
> +}
> +
>  AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
>  {
>  PCIBus *bus = PCI_BUS(dev->bus);
> @@ -2649,6 +2660,7 @@ static const TypeInfo pci_device_type_info = {
>  .abstract = true,
>  .class_size = sizeof(PCIDeviceClass),
>  .class_init = pci_device_class_init,
> +.class_base_init = pci_device_class_base_init,
>  };
>
>  static void pci_register_types(void)
> --
> 2.9.4
>
>

Re: [Qemu-devel] [PATCH 3/5] pci: Add INTERFACE_PCIE_DEVICE to all PCIe devices

[Alistair Francis]

On Wed, Aug 23, 2017 at 3:14 PM, Eduardo Habkost  wrote:
> Change all devices that set is_express=1 to implement
> INTERFACE_PCIE_DEVICE.
>
> Signed-off-by: Eduardo Habkost 
> ---
>  hw/block/nvme.c| 4 
>  hw/net/e1000e.c| 4 
>  hw/pci-bridge/pcie_root_port.c | 4 
>  hw/pci-bridge/xio3130_downstream.c | 4 
>  hw/pci-bridge/xio3130_upstream.c   | 4 
>  hw/pci-host/xilinx-pcie.c  | 4 
>  hw/scsi/megasas.c  | 6 ++
>  hw/usb/hcd-xhci.c  | 4 
>  8 files changed, 34 insertions(+)

For the Xilinx devices.

Reviewed-by: Alistair Francis 

Thanks,
Alistair

>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 6071dc1..26d58b6 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1110,6 +1110,10 @@ static const TypeInfo nvme_info = {
>  .instance_size = sizeof(NvmeCtrl),
>  .class_init= nvme_class_init,
>  .instance_init = nvme_instance_init,
> +.interfaces = (InterfaceInfo[]) {
> +{ INTERFACE_PCIE_DEVICE },
> +{ }
> +},
>  };
>
>  static void nvme_register_types(void)
> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
> index 6c42b44..81f7934 100644
> --- a/hw/net/e1000e.c
> +++ b/hw/net/e1000e.c
> @@ -708,6 +708,10 @@ static const TypeInfo e1000e_info = {
>  .instance_size = sizeof(E1000EState),
>  .class_init = e1000e_class_init,
>  .instance_init = e1000e_instance_init,
> +.interfaces = (InterfaceInfo[]) {
> +{ INTERFACE_PCIE_DEVICE },
> +{ }
> +},
>  };
>
>  static void e1000e_register_types(void)
> diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
> index 4d588cb..9b6e4ce 100644
> --- a/hw/pci-bridge/pcie_root_port.c
> +++ b/hw/pci-bridge/pcie_root_port.c
> @@ -161,6 +161,10 @@ static const TypeInfo rp_info = {
>  .class_init= rp_class_init,
>  .abstract  = true,
>  .class_size = sizeof(PCIERootPortClass),
> +.interfaces = (InterfaceInfo[]) {
> +{ INTERFACE_PCIE_DEVICE },
> +{ }
> +},
>  };
>
>  static void rp_register_types(void)
> diff --git a/hw/pci-bridge/xio3130_downstream.c 
> b/hw/pci-bridge/xio3130_downstream.c
> index e706f36..7d2f762 100644
> --- a/hw/pci-bridge/xio3130_downstream.c
> +++ b/hw/pci-bridge/xio3130_downstream.c
> @@ -195,6 +195,10 @@ static const TypeInfo xio3130_downstream_info = {
>  .name  = "xio3130-downstream",
>  .parent= TYPE_PCIE_SLOT,
>  .class_init= xio3130_downstream_class_init,
> +.interfaces = (InterfaceInfo[]) {
> +{ INTERFACE_PCIE_DEVICE },
> +{ }
> +},
>  };
>
>  static void xio3130_downstream_register_types(void)
> diff --git a/hw/pci-bridge/xio3130_upstream.c 
> b/hw/pci-bridge/xio3130_upstream.c
> index a052224..227997c 100644
> --- a/hw/pci-bridge/xio3130_upstream.c
> +++ b/hw/pci-bridge/xio3130_upstream.c
> @@ -166,6 +166,10 @@ static const TypeInfo xio3130_upstream_info = {
>  .name  = "x3130-upstream",
>  .parent= TYPE_PCIE_PORT,
>  .class_init= xio3130_upstream_class_init,
> +.interfaces = (InterfaceInfo[]) {
> +{ INTERFACE_PCIE_DEVICE },
> +{ }
> +},
>  };
>
>  static void xio3130_upstream_register_types(void)
> diff --git a/hw/pci-host/xilinx-pcie.c b/hw/pci-host/xilinx-pcie.c
> index 4613dda..7659253 100644
> --- a/hw/pci-host/xilinx-pcie.c
> +++ b/hw/pci-host/xilinx-pcie.c
> @@ -317,6 +317,10 @@ static const TypeInfo xilinx_pcie_root_info = {
>  .parent = TYPE_PCI_BRIDGE,
>  .instance_size = sizeof(XilinxPCIERoot),
>  .class_init = xilinx_pcie_root_class_init,
> +.interfaces = (InterfaceInfo[]) {
> +{ INTERFACE_PCIE_DEVICE },
> +{ }
> +},
>  };
>
>  static void xilinx_pcie_register(void)
> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index 734fdae..3641c30 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -2451,6 +2451,7 @@ typedef struct MegasasInfo {
>  int osts;
>  const VMStateDescription *vmsd;
>  Property *props;
> +InterfaceInfo *interfaces;
>  } MegasasInfo;
>
>  static struct MegasasInfo megasas_devices[] = {
> @@ -2480,6 +2481,10 @@ static struct MegasasInfo megasas_devices[] = {
>  .is_express = true,
>  .vmsd = _megasas_gen2,
>  .props = megasas_properties_gen2,
> +.interfaces = (InterfaceInfo[]) {
> +{ INTERFACE_PCIE_DEVICE },
> +{ }
> +},
>  }
>  };
>
> @@ -2531,6 +2536,7 @@ static void megasas_register_types(void)
>  type_info.parent = TYPE_MEGASAS_BASE;
>  type_info.class_data = (void *)info;
>  type_info.class_init = megasas_class_init;
> +type_info.interfaces = info->interfaces;
>
>  type_register(_info);
>  }
> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> index 204ea69..d95ed4f 100644
> --- 

Re: [Qemu-devel] [PATCH 1/5] pci: INTERFACE_LEGACY_PCI_DEVICE and INTERFACE_PCIE_DEVICE interfaces

[Alistair Francis]

On Wed, Aug 23, 2017 at 3:14 PM, Eduardo Habkost  wrote:
> Those two interfaces will be used to indicate which device types
> support legacy PCI or PCI-express buses.  Management software
> will be able to use the qom-list-types QMP command to query that
> information.
>
> Signed-off-by: Eduardo Habkost 

Reviewed-by: Alistair Francis 

Thanks,
Alistair

> ---
>  include/hw/pci/pci.h |  6 ++
>  hw/pci/pci.c | 12 
>  2 files changed, 18 insertions(+)
>
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index e598b09..f5e8ab9 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -195,6 +195,12 @@ enum {
>  #define PCI_DEVICE_GET_CLASS(obj) \
>   OBJECT_GET_CLASS(PCIDeviceClass, (obj), TYPE_PCI_DEVICE)
>
> +/* Interface implemented by devices that can be plugged on PCIe buses */
> +#define INTERFACE_PCIE_DEVICE "pci-express-device"
> +
> +/* Interface implemented by devices that can be plugged on legacy PCI buses 
> */
> +#define INTERFACE_LEGACY_PCI_DEVICE "legacy-pci-device"
> +
>  typedef struct PCIINTxRoute {
>  enum {
>  PCI_INTX_ENABLED,
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 258fbe5..baa3429 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -168,6 +168,16 @@ static const TypeInfo pci_bus_info = {
>  .class_init = pci_bus_class_init,
>  };
>
> +static const TypeInfo pcie_interface_info = {
> +.name  = INTERFACE_PCIE_DEVICE,
> +.parent= TYPE_INTERFACE,
> +};
> +
> +static const TypeInfo legacy_pci_interface_info = {
> +.name  = INTERFACE_LEGACY_PCI_DEVICE,
> +.parent= TYPE_INTERFACE,
> +};
> +
>  static const TypeInfo pcie_bus_info = {
>  .name = TYPE_PCIE_BUS,
>  .parent = TYPE_PCI_BUS,
> @@ -2645,6 +2655,8 @@ static void pci_register_types(void)
>  {
>  type_register_static(_bus_info);
>  type_register_static(_bus_info);
> +type_register_static(_pci_interface_info);
> +type_register_static(_interface_info);
>  type_register_static(_device_type_info);
>  }
>
> --
> 2.9.4
>
>

Re: [Qemu-devel] [PATCH for-2.11 v4 06/25] sparc: make cpu feature parsing property based

[Eduardo Habkost]

On Fri, Aug 25, 2017 at 04:47:40PM +0200, Igor Mammedov wrote:
> with features converted to properties we can use the same
> approach as x86 for features parsing and drop legacy
> approach that manipulated CPU instance directly.
> New sparc_cpu_parse_features() will allow only +-feat
> and explicitly disable feat=on|off syntax for now.
> 
> With that in place and sparc_cpu_parse_features() providing
> generic CPUClass::parse_features callback, the cpu_sparc_init()
> will do the same job as cpu_generic_init() so replace content
> of cpu_sparc_init() with it.
> 
> Signed-off-by: Igor Mammedov 
> Acked-by: Philippe Mathieu-Daudé 
> Tested-by: Philippe Mathieu-Daudé 

Reviewed-by: Eduardo Habkost 

-- 
Eduardo

Re: [Qemu-devel] [PULL 09/21] pci: Convert to realize

[Eduardo Habkost]

On Fri, Aug 25, 2017 at 07:57:57PM +0300, Michael S. Tsirkin wrote:
> On Fri, Aug 25, 2017 at 12:17:42PM -0300, Eduardo Habkost wrote:
> > On Mon, Jul 03, 2017 at 10:45:16PM +0300, Michael S. Tsirkin wrote:
> > > From: Mao Zhongyi 
> > > 
> > > Convert i82801b11, io3130_upstream, io3130_downstream and
> > > pcie_root_port devices to realize.
> > > 
> > > Cc: m...@redhat.com
> > > Cc: mar...@redhat.com
> > > Cc: arm...@redhat.com
> > > Signed-off-by: Mao Zhongyi 
> > > Reviewed-by: Marcel Apfelbaum 
> > > Reviewed-by: Michael S. Tsirkin 
> > > Signed-off-by: Michael S. Tsirkin 
> > > ---
> > [...]
> > > diff --git a/hw/pci-bridge/xio3130_downstream.c 
> > > b/hw/pci-bridge/xio3130_downstream.c
> > > index cfe8a36..e706f36 100644
> > > --- a/hw/pci-bridge/xio3130_downstream.c
> > > +++ b/hw/pci-bridge/xio3130_downstream.c
> > > @@ -56,33 +56,33 @@ static void xio3130_downstream_reset(DeviceState 
> > > *qdev)
> > >  pci_bridge_reset(qdev);
> > >  }
> > >  
> > > -static int xio3130_downstream_initfn(PCIDevice *d)
> > > +static void xio3130_downstream_realize(PCIDevice *d, Error **errp)
> > >  {
> > [...]
> > >  pcie_chassis_create(s->chassis);
> > >  rc = pcie_chassis_add_slot(s);
> > >  if (rc < 0) {
> > >  goto err_pcie_cap;
> > 
> > Missing error_setg() call here.  If pcie_chassis_add_slot() fails, realize
> > won't report an error properly.  Causes crash with:
> > 
> > $ ./x86_64-softmmu/qemu-system-x86_64 -device ioh3420 -device 
> > xio3130-downstream
> > qemu-system-x86_64: /home/ehabkost/rh/proj/virt/qemu/memory.c:2166: 
> > memory_region_del_subregion: Assertion `subregion->container == mr' failed.
> > Aborted (core dumped)
> > 
> > 
> > >  }
> > >  
> > >  rc = pcie_aer_init(d, PCI_ERR_VER, XIO3130_AER_OFFSET,
> > > -   PCI_ERR_SIZEOF, );
> > > +   PCI_ERR_SIZEOF, errp);
> > >  if (rc < 0) {
> > > -error_report_err(err);
> > >  goto err;
> > >  }
> > >  
> > > -return 0;
> > > +return;
> > >  
> > >  err:
> > >  pcie_chassis_del_slot(s);
> > > @@ -114,7 +113,6 @@ err_msi:
> > >  msi_uninit(d);
> > >  err_bridge:
> > >  pci_bridge_exitfn(d);
> > > -return rc;
> > >  }
> > >  
> > [...]
> 
> Good catch!  Patch?

I will send one later today.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 0/3] scripts: add argparse module for Python 2.6 compatibility

[Eric Blake]

On 08/25/2017 11:40 AM, Peter Maydell wrote:
> Our choices about our dependencies are generally driven by "what
> are the versions available on the oldest distros which we wish
> to support building QEMU on", which typically is whatever the
> long-term-support versions of Ubuntu, SUSE, Redhat, etc are.
> 
> Has somebody checked what that means for our Python version
> requirements?

At least this one:

RHEL/CentOS 6: Python-2.6.6

> (It would certainly be nicer if we could get away
> with bumping the version-req rather than including a big lump
> of code with yet-another-software-license, but maybe we can't.)

Based on the above, no sooner than when Red Hat is done supporting RHEL
6 (well, upstream can always decide to bump the minimum requirements it
and make Red Hat do the downstream back-compat work in isolation, but
there's enough Red Hat contributors that it shouldn't surprise anyone
that back-compat work patches will still be posted upstream...)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2] xen: Emit RTC_CHANGE upon TIMEOFFSET ioreq

[Stefano Stabellini]

On Wed, 23 Aug 2017, Ross Lagerwall wrote:
> When the guest writes to the RTC, Xen emulates it and broadcasts a
> TIMEOFFSET ioreq. Emit an RTC_CHANGE QMP event to all QMP monitors when
> this happens rather than ignoring it so that something useful can be
> done with the information. This is the same event that QEMU generates
> when it emulates the RTC.
> 
> This patch by itself doesn't affect any of the toolstacks that I
> checked; the libxl toolstack doesn't currently handle this event nor
> does the XAPI toolstack. If nothing handles the event, it is simply
> ignored. We plan on modifying XAPI to handle it.
> 
> Signed-off-by: Ross Lagerwall 

Reviewed-by: Stefano Stabellini 

I queued it up.

> ---
> 
> Changed in v2:
> * Expanded commit message.
> 
>  hw/i386/xen/xen-hvm.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
> index d9ccd5d..ffd20dc 100644
> --- a/hw/i386/xen/xen-hvm.c
> +++ b/hw/i386/xen/xen-hvm.c
> @@ -16,6 +16,7 @@
>  #include "hw/i386/apic-msidef.h"
>  #include "hw/xen/xen_common.h"
>  #include "hw/xen/xen_backend.h"
> +#include "qapi-event.h"
>  #include "qmp-commands.h"
>  
>  #include "qemu/error-report.h"
> @@ -967,6 +968,7 @@ static void handle_ioreq(XenIOState *state, ioreq_t *req)
>  handle_vmport_ioreq(state, req);
>  break;
>  case IOREQ_TYPE_TIMEOFFSET:
> +qapi_event_send_rtc_change((int64_t)req->data, _abort);
>  break;
>  case IOREQ_TYPE_INVALIDATE:
>  xen_invalidate_map_cache();
> -- 
> 2.9.5
> 

Re: [Qemu-devel] [Qemu-arm] [RFC v6 8/9] hw/arm/smmuv3: VFIO integration

[Michael S. Tsirkin]

On Wed, Aug 23, 2017 at 08:39:18AM +0200, Auger Eric wrote:
> Hi Linu,
> 
> On 23/08/2017 06:24, Linu Cherian wrote:
> > Hi Eric,
> > 
> > 
> > On Fri Aug 11, 2017 at 04:22:33PM +0200, Eric Auger wrote:
> >> This patch allows doing PCIe passthrough with a guest exposed
> >> with a vSMMUv3. It implements the replay and notify_flag_changed
> >> iommu ops. Also on TLB and data structure invalidation commands,
> >> we replay the mappings so that the physical IOMMU implements
> >> updated stage 1 settings (Guest IOVA -> Guest PA) + stage 2 settings.
> >>
> >> This works only if the guest smmuv3 driver implements the
> >> "tlbi-on-map" option.
> >>
> >> Signed-off-by: Eric Auger 
> > 
> > Tried out launching a guest with Qemu option "-machine virt-2.10,smmu"
> > and a 1G Ethernet controller as vfio-pci device. It works fine for me.
> 
> Hum sorry, I forgot to update the cover letter. You need to use -machine
> virt-2.11,smmu for the instantiation as the 2.10 machine was released as
> part of 2.10 and those changes only apply on 2.11 mach virt introduced
> in this series. Please apologize for the pain.
> 
> I am going to release a new version this week fixing my last DPDK bug
> (at least I am aware of ). In this new version, the instantiation method
> will change to -device smmuv3 which is closer to what is done on Intel.
>
> By the way I will also take time to provide some more info about the
> VFIO integration as it is implemented in this series although this
> latter may evolve due to NAK of kernel FW quirk.

Yes I think changing compat string to avoid pretending this is smmuv3
was a hard requirement. So you might need to do -device qemu-smmuv3
if you want VFIO support with this PV quirk.

For now, how about a subset of the functionality with VFIO disabled?
People can use it with virtio for now.


> Thank you for testing!
> 
> Best Regards
> 
> Eric
> > 
> > Qemu source: https://github.com/eauger/qemu.git Branch: v2.10.0-rc2-SMMU-v6 
> >  
> > 
> > But had to make this change, 
> > 
> > --- a/hw/arm/virt.c 
> >
> > +++ b/hw/arm/virt.c 
> >
> > @@ -1806,7 +1806,7 @@ static void virt_machine_2_10_options(MachineClass 
> > *mc)  
> >  virt_machine_2_11_options(mc); 
> >
> >  SET_MACHINE_COMPAT(mc, VIRT_COMPAT_2_10);  
> >
> > 
> >
> > -vmc->no_smmu = true;   
> >
> > +vmc->no_smmu = false;  
> >
> >  }  
> >
> >  DEFINE_VIRT_MACHINE(2, 10) 
> > 
> > so that qemu doesnt complain about "Property .smmu not found"
> > 
> > Will let you know if i have updates on further testing.
> > 
> > Thanks.
> > 
> >>
> >> ---
> >>
> >> v5 -> v6:
> >> - use IOMMUMemoryRegion
> >> - handle implementation defined SMMU_CMD_TLBI_NH_VA_AM cmd
> >>   (goes along with TLBI_ON_MAP FW quirk)
> >> - replay systematically unmap the whole range first
> >> - smmuv3_map_hook does not unmap anymore and the unmap is done
> >>   before the replay
> >> - add and use smmuv3_context_device_invalidate instead of
> >>   blindly replaying everything
> >> ---
> >>  hw/arm/smmuv3-internal.h |   1 +
> >>  hw/arm/smmuv3.c  | 265 
> >> ++-
> >>  hw/arm/trace-events  |  14 +++
> >>  3 files changed, 277 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
> >> index e255df1..ac4628f 100644
> >> --- a/hw/arm/smmuv3-internal.h
> >> +++ b/hw/arm/smmuv3-internal.h
> >> @@ -344,6 +344,7 @@ enum {
> >>  SMMU_CMD_RESUME  = 0x44,
> >>  SMMU_CMD_STALL_TERM,
> >>  SMMU_CMD_SYNC,  /* 0x46 */
> >> +SMMU_CMD_TLBI_NH_VA_AM   = 0x8F, /* VIOMMU Impl Defined */
> >>  };
> >>  
> >>  static const char *cmd_stringify[] = {
> >> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> >> index e195a0e..89fb116 100644
> >> --- a/hw/arm/smmuv3.c
> >> +++ b/hw/arm/smmuv3.c
> >> @@ -25,6 +25,7 @@
> >>  #include "exec/address-spaces.h"
> >>  #include "trace.h"
> >>  #include "qemu/error-report.h"
> >> +#include "exec/target_page.h"
> >>  
> >>  #include "hw/arm/smmuv3.h"
> >>  #include "smmuv3-internal.h"
> >> @@ -143,6 +144,71 @@ static MemTxResult smmu_read_cmdq(SMMUV3State *s, Cmd 
> >> *cmd)
> >>  return ret;
> >>  }
> >>  
> >> +static void smmuv3_replay_all(SMMUState *s)
> >> +{
> >> +SMMUNotifierNode *node;
> >> +
> >> +QLIST_FOREACH(node, >notifiers_list, next) {
> >> +trace_smmuv3_replay_all(node->sdev->iommu.parent_obj.name);
> >> +

Re: [Qemu-devel] [PULL 09/21] pci: Convert to realize

[Michael S. Tsirkin]

On Fri, Aug 25, 2017 at 12:17:42PM -0300, Eduardo Habkost wrote:
> On Mon, Jul 03, 2017 at 10:45:16PM +0300, Michael S. Tsirkin wrote:
> > From: Mao Zhongyi 
> > 
> > Convert i82801b11, io3130_upstream, io3130_downstream and
> > pcie_root_port devices to realize.
> > 
> > Cc: m...@redhat.com
> > Cc: mar...@redhat.com
> > Cc: arm...@redhat.com
> > Signed-off-by: Mao Zhongyi 
> > Reviewed-by: Marcel Apfelbaum 
> > Reviewed-by: Michael S. Tsirkin 
> > Signed-off-by: Michael S. Tsirkin 
> > ---
> [...]
> > diff --git a/hw/pci-bridge/xio3130_downstream.c 
> > b/hw/pci-bridge/xio3130_downstream.c
> > index cfe8a36..e706f36 100644
> > --- a/hw/pci-bridge/xio3130_downstream.c
> > +++ b/hw/pci-bridge/xio3130_downstream.c
> > @@ -56,33 +56,33 @@ static void xio3130_downstream_reset(DeviceState *qdev)
> >  pci_bridge_reset(qdev);
> >  }
> >  
> > -static int xio3130_downstream_initfn(PCIDevice *d)
> > +static void xio3130_downstream_realize(PCIDevice *d, Error **errp)
> >  {
> [...]
> >  pcie_chassis_create(s->chassis);
> >  rc = pcie_chassis_add_slot(s);
> >  if (rc < 0) {
> >  goto err_pcie_cap;
> 
> Missing error_setg() call here.  If pcie_chassis_add_slot() fails, realize
> won't report an error properly.  Causes crash with:
> 
> $ ./x86_64-softmmu/qemu-system-x86_64 -device ioh3420 -device 
> xio3130-downstream
> qemu-system-x86_64: /home/ehabkost/rh/proj/virt/qemu/memory.c:2166: 
> memory_region_del_subregion: Assertion `subregion->container == mr' failed.
> Aborted (core dumped)
> 
> 
> >  }
> >  
> >  rc = pcie_aer_init(d, PCI_ERR_VER, XIO3130_AER_OFFSET,
> > -   PCI_ERR_SIZEOF, );
> > +   PCI_ERR_SIZEOF, errp);
> >  if (rc < 0) {
> > -error_report_err(err);
> >  goto err;
> >  }
> >  
> > -return 0;
> > +return;
> >  
> >  err:
> >  pcie_chassis_del_slot(s);
> > @@ -114,7 +113,6 @@ err_msi:
> >  msi_uninit(d);
> >  err_bridge:
> >  pci_bridge_exitfn(d);
> > -return rc;
> >  }
> >  
> [...]

Good catch!  Patch?

> -- 
> Eduardo

Re: [Qemu-devel] [PATCH for-2.11] tests/test-hmp: Remove puv3 and tricore_testboard from the blacklist

[Dr. David Alan Gilbert]

* Thomas Huth (th...@redhat.com) wrote:
> The problem with puv3 has been fixed with 0ac241bcf9f9d99a252a352a162f
> ('unicore32: abort when entering "x 0" on the monitor') and the problem
> with tricore_testboard has been fixed with b190f477e29c7cd03a8fee49c96d
> ('qemu-system-tricore: segfault when entering "x 0" on the monitor').
> 
> Signed-off-by: Thomas Huth 

Reviewed-by: Dr. David Alan Gilbert 

> ---
>  tests/test-hmp.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/tests/test-hmp.c b/tests/test-hmp.c
> index 4382df0..9f286bf 100644
> --- a/tests/test-hmp.c
> +++ b/tests/test-hmp.c
> @@ -192,8 +192,7 @@ static void add_machine_test_case(const char *mname)
>  char *path;
>  
>  /* Ignore blacklisted machines that have known problems */
> -if (!strcmp("puv3", mname) || !strcmp("tricore_testboard", mname) ||
> -!strcmp("xenfv", mname) || !strcmp("xenpv", mname)) {
> +if (!strcmp("xenfv", mname) || !strcmp("xenpv", mname)) {
>  return;
>  }
>  
> -- 
> 1.8.3.1
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 0/3] scripts: add argparse module for Python 2.6 compatibility

[Peter Maydell]

On 25 August 2017 at 17:35, Philippe Mathieu-Daudé  wrote:
> Hi Stefan,
>
> On 08/25/2017 12:57 PM, Stefan Hajnoczi wrote:
>>
>> Many scripts can benefit from the standard library argparse module, which
>> has
>> improvements over the older optparse module.  Unfortunately argparse was
>> only
>> shipped in Python 2.7 so we need a fallback for Python 2.6.
>
>
> I probably missed some discussion about it, but what are the reasons to stay
> 2.6 compatible?
>
> Python 2.6 support ended during October 2013, 4 years ago... [1] Why don't
> kill it, start deprecating 2.7 which support will end in less than 3 years
> from now [2], and move efforts to version 3...?

Our choices about our dependencies are generally driven by "what
are the versions available on the oldest distros which we wish
to support building QEMU on", which typically is whatever the
long-term-support versions of Ubuntu, SUSE, Redhat, etc are.

Has somebody checked what that means for our Python version
requirements? (It would certainly be nicer if we could get away
with bumping the version-req rather than including a big lump
of code with yet-another-software-license, but maybe we can't.)

> Apparently we expect a C compiler compatible with GCC >= 4.1 which was
> released on 2006, before Python 2.5 :S
> Then QEMU_BUILD_BUG_ON() try to use C11 feature...

That's protected by a configure check for exactly this reason.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/3] scripts: add argparse module for Python 2.6 compatibility

[Philippe Mathieu-Daudé]

Hi Stefan,

On 08/25/2017 12:57 PM, Stefan Hajnoczi wrote:

Many scripts can benefit from the standard library argparse module, which has
improvements over the older optparse module.  Unfortunately argparse was only
shipped in Python 2.7 so we need a fallback for Python 2.6.


I probably missed some discussion about it, but what are the reasons to 
stay 2.6 compatible?


Python 2.6 support ended during October 2013, 4 years ago... [1] Why 
don't kill it, start deprecating 2.7 which support will end in less than 
3 years from now [2], and move efforts to version 3...?


Apparently we expect a C compiler compatible with GCC >= 4.1 which was 
released on 2006, before Python 2.5 :S

Then QEMU_BUILD_BUG_ON() try to use C11 feature...

[1] https://www.python.org/dev/peps/pep-0361/ and
https://mail.python.org/pipermail/python-dev/2013-September/128287.html
[2] https://www.python.org/dev/peps/pep-0373/ and
https://pythonclock.org/

Regards,

Phil.



This patch series adds a copy of argparse.py and updates scripts as necessary
to import it.

Stefan Hajnoczi (3):
   scripts: add argparse module for Python 2.6 compatibility
   docker.py: Python 2.6 argparse compatibility
   tests: migration/guestperf Python 2.6 argparse compatibility

  COPYING.PYTHON |  270 
  scripts/argparse.py| 2406 
  tests/docker/docker.py |4 +-
  tests/migration/guestperf/shell.py |8 +-
  4 files changed, 2684 insertions(+), 4 deletions(-)
  create mode 100644 COPYING.PYTHON
  create mode 100644 scripts/argparse.py

Re: [Qemu-devel] [RFC v2 2/8] monitor: allow monitor to create thread to poll

[Dr. David Alan Gilbert]

* Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> Hi
> 
> On Fri, Aug 25, 2017 at 6:12 PM Dr. David Alan Gilbert 
> wrote:
> 
> > * Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> > > On Fri, Aug 25, 2017 at 5:33 PM Dr. David Alan Gilbert <
> > dgilb...@redhat.com>
> > > wrote:
> > >
> > > > * Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> > > > > Hi
> > > > >
> > > > > On Wed, Aug 23, 2017 at 8:52 AM Peter Xu  wrote:
> > > > >
> > > > > > Firstly, introduce Monitor.use_thread, and set it for monitors
> > that are
> > > > > > using non-mux typed backend chardev.  We only do this for
> > monitors, so
> > > > > > mux-typed chardevs are not suitable (when it connects to, e.g.,
> > serials
> > > > > > and the monitor together).
> > > > > >
> > > > > > When use_thread is set, we create standalone thread to poll the
> > monitor
> > > > > > events, isolated from the main loop thread.  Here we still need to
> > take
> > > > > > the BQL before dispatching the tasks since some of the monitor
> > commands
> > > > > > are not allowed to execute without the protection of BQL.  Then
> > this
> > > > > > gives us the chance to avoid taking the BQL for some monitor
> > commands
> > > > in
> > > > > > the future.
> > > > > >
> > > > > > * Why this change?
> > > > > >
> > > > > > We need these per-monitor threads to make sure we can have at
> > least one
> > > > > > monitor that will never stuck (that can receive further monitor
> > > > > > commands).
> > > > > >
> > > > > > * So when will monitors stuck?  And, how do they stuck?
> > > > > >
> > > > > > After we have postcopy and remote page faults, it's simple to
> > achieve a
> > > > > > stuck in the monitor (which is also a stuck in main loop thread):
> > > > > >
> > > > > > (1) Monitor deadlock on BQL
> > > > > >
> > > > > > As we may know, when postcopy is running on destination VM, the
> > vcpu
> > > > > > threads can stuck merely any time as long as it tries to access an
> > > > > > uncopied guest page.  Meanwhile, when the stuck happens, it is
> > possible
> > > > > > that the vcpu thread is holding the BQL.  If the page fault is not
> > > > > > handled quickly, you'll find that monitors stop working, which is
> > > > trying
> > > > > > to take the BQL.
> > > > > >
> > > > > > If the page fault cannot be handled correctly (one case is a paused
> > > > > > postcopy, when network is temporarily down), monitors will hang
> > > > > > forever.  Without current patch, that means the main loop hanged.
> > > > We'll
> > > > > > never find a way to talk to VM again.
> > > > > >
> > > > >
> > > > > Could the BQL be pushed down to the monitor commands level instead?
> > That
> > > > > way we wouldn't need a seperate thread to solve the hang on commands
> > that
> > > > > do not need BQL.
> > > >
> > > > If the main thread is stuck though I don't see how that helps you; you
> > > > have to be able to run these commands on another thread.
> > > >
> > >
> > > Why would the main thread be stuck? In (1) If the vcpu thread takes the
> > BQL
> > > and the command doesn't need it, it would work.
> >
> > True; assuming nothing else in the main loop is blocked;  which is a big
> > if - making sure no bh's etc could block on guest memory or the bql.
> >
> >
> > In (2),  info cpus
> > > shouldn't keep the BQL (my qapi-async series would probably help here)
> >
> > How does that work?
> >
> >
> https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg03626.html
> 
> With the series, a command can be broken up in receive/start &
> finish/reply. This allows to reenter the loop, potentially freeing the BQL,
> and process other events. This allowed me to fix a screendump glitch bug (
> http://lists.gnu.org/archive/html/qemu-devel/2017-01/msg03650.html). This
> also open the door to concurrent QMP commands (if the client turns on the
> capability option).

Interesting.
I can see how that would work well for commands that know they're long
lived and do that work to split themselves into
receive/start/finish/reply.  However I'm worried that it means that
it's fragile in that if something accesses guest memory when they din't
realise they were doing, or code that forgot it's taking the lock, then
we've got a command that can occasionally block.  That's going to be a
lot of analysis and design on each command and if we were to do it
widely then we'd certainly miss some cases.  Having the monitors in
spearate threads means you only have to worry about the commands
you want to be lock-free.

Dave

> -- 
> Marc-André Lureau
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 0/3] scripts: add argparse module for Python 2.6 compatibility

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20170825155732.15665-1-stefa...@redhat.com
Subject: [Qemu-devel] [PATCH 0/3] scripts: add argparse module for Python 2.6 
compatibility

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
b0f75bbee1 tests: migration/guestperf Python 2.6 argparse compatibility
a0976d88ae docker.py: Python 2.6 argparse compatibility
3f9dfc445a scripts: add argparse module for Python 2.6 compatibility

=== OUTPUT BEGIN ===
Checking PATCH 1/3: scripts: add argparse module for Python 2.6 compatibility...
ERROR: trailing whitespace
#115: FILE: COPYING.PYTHON:93:
+Reserved" are retained in Python alone or in any derivative version $

total: 1 errors, 0 warnings, 2676 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 2/3: docker.py: Python 2.6 argparse compatibility...
Checking PATCH 3/3: tests: migration/guestperf Python 2.6 argparse 
compatibility...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@freelists.org

Re: [Qemu-devel] [RFC v2 2/8] monitor: allow monitor to create thread to poll

[Marc-André Lureau]

Hi

On Fri, Aug 25, 2017 at 6:12 PM Dr. David Alan Gilbert 
wrote:

> * Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> > On Fri, Aug 25, 2017 at 5:33 PM Dr. David Alan Gilbert <
> dgilb...@redhat.com>
> > wrote:
> >
> > > * Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> > > > Hi
> > > >
> > > > On Wed, Aug 23, 2017 at 8:52 AM Peter Xu  wrote:
> > > >
> > > > > Firstly, introduce Monitor.use_thread, and set it for monitors
> that are
> > > > > using non-mux typed backend chardev.  We only do this for
> monitors, so
> > > > > mux-typed chardevs are not suitable (when it connects to, e.g.,
> serials
> > > > > and the monitor together).
> > > > >
> > > > > When use_thread is set, we create standalone thread to poll the
> monitor
> > > > > events, isolated from the main loop thread.  Here we still need to
> take
> > > > > the BQL before dispatching the tasks since some of the monitor
> commands
> > > > > are not allowed to execute without the protection of BQL.  Then
> this
> > > > > gives us the chance to avoid taking the BQL for some monitor
> commands
> > > in
> > > > > the future.
> > > > >
> > > > > * Why this change?
> > > > >
> > > > > We need these per-monitor threads to make sure we can have at
> least one
> > > > > monitor that will never stuck (that can receive further monitor
> > > > > commands).
> > > > >
> > > > > * So when will monitors stuck?  And, how do they stuck?
> > > > >
> > > > > After we have postcopy and remote page faults, it's simple to
> achieve a
> > > > > stuck in the monitor (which is also a stuck in main loop thread):
> > > > >
> > > > > (1) Monitor deadlock on BQL
> > > > >
> > > > > As we may know, when postcopy is running on destination VM, the
> vcpu
> > > > > threads can stuck merely any time as long as it tries to access an
> > > > > uncopied guest page.  Meanwhile, when the stuck happens, it is
> possible
> > > > > that the vcpu thread is holding the BQL.  If the page fault is not
> > > > > handled quickly, you'll find that monitors stop working, which is
> > > trying
> > > > > to take the BQL.
> > > > >
> > > > > If the page fault cannot be handled correctly (one case is a paused
> > > > > postcopy, when network is temporarily down), monitors will hang
> > > > > forever.  Without current patch, that means the main loop hanged.
> > > We'll
> > > > > never find a way to talk to VM again.
> > > > >
> > > >
> > > > Could the BQL be pushed down to the monitor commands level instead?
> That
> > > > way we wouldn't need a seperate thread to solve the hang on commands
> that
> > > > do not need BQL.
> > >
> > > If the main thread is stuck though I don't see how that helps you; you
> > > have to be able to run these commands on another thread.
> > >
> >
> > Why would the main thread be stuck? In (1) If the vcpu thread takes the
> BQL
> > and the command doesn't need it, it would work.
>
> True; assuming nothing else in the main loop is blocked;  which is a big
> if - making sure no bh's etc could block on guest memory or the bql.
>
>
> In (2),  info cpus
> > shouldn't keep the BQL (my qapi-async series would probably help here)
>
> How does that work?
>
>
https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg03626.html

With the series, a command can be broken up in receive/start &
finish/reply. This allows to reenter the loop, potentially freeing the BQL,
and process other events. This allowed me to fix a screendump glitch bug (
http://lists.gnu.org/archive/html/qemu-devel/2017-01/msg03650.html). This
also open the door to concurrent QMP commands (if the client turns on the
capability option).

-- 
Marc-André Lureau

Re: [Qemu-devel] [RFC v2 2/8] monitor: allow monitor to create thread to poll

[Dr. David Alan Gilbert]

* Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> On Fri, Aug 25, 2017 at 5:33 PM Dr. David Alan Gilbert 
> wrote:
> 
> > * Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> > > Hi
> > >
> > > On Wed, Aug 23, 2017 at 8:52 AM Peter Xu  wrote:
> > >
> > > > Firstly, introduce Monitor.use_thread, and set it for monitors that are
> > > > using non-mux typed backend chardev.  We only do this for monitors, so
> > > > mux-typed chardevs are not suitable (when it connects to, e.g., serials
> > > > and the monitor together).
> > > >
> > > > When use_thread is set, we create standalone thread to poll the monitor
> > > > events, isolated from the main loop thread.  Here we still need to take
> > > > the BQL before dispatching the tasks since some of the monitor commands
> > > > are not allowed to execute without the protection of BQL.  Then this
> > > > gives us the chance to avoid taking the BQL for some monitor commands
> > in
> > > > the future.
> > > >
> > > > * Why this change?
> > > >
> > > > We need these per-monitor threads to make sure we can have at least one
> > > > monitor that will never stuck (that can receive further monitor
> > > > commands).
> > > >
> > > > * So when will monitors stuck?  And, how do they stuck?
> > > >
> > > > After we have postcopy and remote page faults, it's simple to achieve a
> > > > stuck in the monitor (which is also a stuck in main loop thread):
> > > >
> > > > (1) Monitor deadlock on BQL
> > > >
> > > > As we may know, when postcopy is running on destination VM, the vcpu
> > > > threads can stuck merely any time as long as it tries to access an
> > > > uncopied guest page.  Meanwhile, when the stuck happens, it is possible
> > > > that the vcpu thread is holding the BQL.  If the page fault is not
> > > > handled quickly, you'll find that monitors stop working, which is
> > trying
> > > > to take the BQL.
> > > >
> > > > If the page fault cannot be handled correctly (one case is a paused
> > > > postcopy, when network is temporarily down), monitors will hang
> > > > forever.  Without current patch, that means the main loop hanged.
> > We'll
> > > > never find a way to talk to VM again.
> > > >
> > >
> > > Could the BQL be pushed down to the monitor commands level instead? That
> > > way we wouldn't need a seperate thread to solve the hang on commands that
> > > do not need BQL.
> >
> > If the main thread is stuck though I don't see how that helps you; you
> > have to be able to run these commands on another thread.
> >
> 
> Why would the main thread be stuck? In (1) If the vcpu thread takes the BQL
> and the command doesn't need it, it would work.

True; assuming nothing else in the main loop is blocked;  which is a big
if - making sure no bh's etc could block on guest memory or the bql.

> In (2),  info cpus
> shouldn't keep the BQL (my qapi-async series would probably help here)

How does that work?

Dave

> 
> > Dave
> >
> > > We could also optionnally make command that need the BQL to fail if lock
> > is
> > > held (after a timeout)?
> > >
> > >
> > >
> > > > (2) Monitor tries to run codes page-faulted vcpus
> > > >
> > > > The HMP command "info cpus" is one of the good example - it tries to
> > > > kick all the vcpus and sync status from them.  However, if there is any
> > > > vcpu that stuck at an unhandled page fault, it can never achieve the
> > > > sync, then the HMP hangs.  Again, it hangs the main loop thread as
> > well.
> > > >
> > > > After either (1) or (2), we can see the deadlock problem:
> > > >
> > > > - On one hand, if monitor hangs, we cannot do the postcopy recovery,
> > > >   because postcopy recovery needs user to specify new listening port on
> > > >   destination monitor.
> > > >
> > > > - On the other hand, if we cannot recover the paused postcopy, then
> > page
> > > >   faults cannot be serviced, and the monitors will possibly hang
> > > >   forever then.
> > > >
> > > > * How this patch helps?
> > > >
> > > > - Firstly, we'll have our own thread for each dedicated monitor (or
> > say,
> > > >   the backend chardev is only used for monitor), so even main loop
> > > >   thread hangs (it is always possible), this monitor thread may still
> > > >   survive.
> > > >
> > > > - Not all monitor commands need the BQL.  We can selectively take the
> > > >   BQL (depends on which command we are running) to avoid waiting on a
> > > >   page-faulted vcpu thread that has taken the BQL (this will be done in
> > > >   following up patches).
> > > >
> > > > Signed-off-by: Peter Xu 
> > > > ---
> > > >  monitor.c   | 75
> > > > +
> > > >  qapi/qmp-dispatch.c | 15 +++
> > > >  2 files changed, 85 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/monitor.c b/monitor.c
> > > > index 7c90df7..3d4ecff 100644
> > > > --- a/monitor.c
> > > > +++ b/monitor.c
> > > > @@ -36,6 +36,8 @@
> > > >  #include "net/net.h"
> > > >  

Re: [Qemu-devel] [PATCH v2 09/16] qapi-schema: Collect migration stuff in qapi/migration.json

[Dr. David Alan Gilbert]

* Markus Armbruster (arm...@redhat.com) wrote:
> Cc: Juan Quintela 
> Cc: Dr. David Alan Gilbert 
> Signed-off-by: Markus Armbruster 


Two thoughts:
  a) Do you actually want that as migration/migration.json?
  b) I'd prefer StrOrNull to be somewhere more central; Migration may be
  the only user, but it's not logically migration specific.


Reviewed-by: Dr. David Alan Gilbert 

Dave

> ---
>  MAINTAINERS |1 +
>  Makefile|1 +
>  qapi-schema.json| 1056 +
>  qapi/common.json|   16 +
>  qapi/event.json |   38 --
>  qapi/migration.json | 1085 
> +++
>  6 files changed, 1104 insertions(+), 1093 deletions(-)
>  create mode 100644 qapi/migration.json
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 24c5105..baa9859 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1498,6 +1498,7 @@ F: migration/
>  F: scripts/vmstate-static-checker.py
>  F: tests/vmstate-static-checker-data/
>  F: docs/migration.txt
> +F: qapi/migration.json
>  
>  Seccomp
>  M: Eduardo Otubo 
> diff --git a/Makefile b/Makefile
> index c7b6fd1..18cf670 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -413,6 +413,7 @@ qapi-modules = $(SRC_PATH)/qapi-schema.json 
> $(SRC_PATH)/qapi/common.json \
> $(SRC_PATH)/qapi/char.json \
> $(SRC_PATH)/qapi/crypto.json \
> $(SRC_PATH)/qapi/event.json $(SRC_PATH)/qapi/introspect.json \
> +   $(SRC_PATH)/qapi/migration.json \
> $(SRC_PATH)/qapi/net.json \
> $(SRC_PATH)/qapi/rocker.json \
> $(SRC_PATH)/qapi/run-state.json \
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 6a23f59..21f54ea 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -87,6 +87,7 @@
>  { 'include': 'qapi/net.json' }
>  { 'include': 'qapi/rocker.json' }
>  { 'include': 'qapi/ui.json' }
> +{ 'include': 'qapi/migration.json' }
>  { 'include': 'qapi/event.json' }
>  { 'include': 'qapi/trace.json' }
>  { 'include': 'qapi/introspect.json' }
> @@ -117,22 +118,6 @@
>  { 'command': 'qmp_capabilities' }
>  
>  ##
> -# @StrOrNull:
> -#
> -# This is a string value or the explicit lack of a string (null
> -# pointer in C).  Intended for cases when 'optional absent' already
> -# has a different meaning.
> -#
> -# @s: the string value
> -# @n: no string value
> -#
> -# Since: 2.10
> -##
> -{ 'alternate': 'StrOrNull',
> -  'data': { 's': 'str',
> -'n': 'null' } }
> -
> -##
>  # @LostTickPolicy:
>  #
>  # Policy for handling lost ticks in timer devices.
> @@ -316,778 +301,6 @@
>  { 'command': 'query-events', 'returns': ['EventInfo'] }
>  
>  ##
> -# @MigrationStats:
> -#
> -# Detailed migration status.
> -#
> -# @transferred: amount of bytes already transferred to the target VM
> -#
> -# @remaining: amount of bytes remaining to be transferred to the target VM
> -#
> -# @total: total amount of bytes involved in the migration process
> -#
> -# @duplicate: number of duplicate (zero) pages (since 1.2)
> -#
> -# @skipped: number of skipped zero pages (since 1.5)
> -#
> -# @normal: number of normal pages (since 1.2)
> -#
> -# @normal-bytes: number of normal bytes sent (since 1.2)
> -#
> -# @dirty-pages-rate: number of pages dirtied by second by the
> -#guest (since 1.3)
> -#
> -# @mbps: throughput in megabits/sec. (since 1.6)
> -#
> -# @dirty-sync-count: number of times that dirty ram was synchronized (since 
> 2.1)
> -#
> -# @postcopy-requests: The number of page requests received from the 
> destination
> -#(since 2.7)
> -#
> -# @page-size: The number of bytes per page for the various page-based
> -#statistics (since 2.10)
> -#
> -# Since: 0.14.0
> -##
> -{ 'struct': 'MigrationStats',
> -  'data': {'transferred': 'int', 'remaining': 'int', 'total': 'int' ,
> -   'duplicate': 'int', 'skipped': 'int', 'normal': 'int',
> -   'normal-bytes': 'int', 'dirty-pages-rate' : 'int',
> -   'mbps' : 'number', 'dirty-sync-count' : 'int',
> -   'postcopy-requests' : 'int', 'page-size' : 'int' } }
> -
> -##
> -# @XBZRLECacheStats:
> -#
> -# Detailed XBZRLE migration cache statistics
> -#
> -# @cache-size: XBZRLE cache size
> -#
> -# @bytes: amount of bytes already transferred to the target VM
> -#
> -# @pages: amount of pages transferred to the target VM
> -#
> -# @cache-miss: number of cache miss
> -#
> -# @cache-miss-rate: rate of cache miss (since 2.1)
> -#
> -# @overflow: number of overflows
> -#
> -# Since: 1.2
> -##
> -{ 'struct': 'XBZRLECacheStats',
> -  'data': {'cache-size': 'int', 'bytes': 'int', 'pages': 'int',
> -   'cache-miss': 'int', 'cache-miss-rate': 'number',
> -   'overflow': 'int' } }
> -
> -##
> -# @MigrationStatus:
> -#
> -# An enumeration of migration status.
> -#
> -# @none: no migration has ever happened.
> -#

Re: [Qemu-devel] [RFC v2 2/8] monitor: allow monitor to create thread to poll

[Marc-André Lureau]

On Fri, Aug 25, 2017 at 5:33 PM Dr. David Alan Gilbert 
wrote:

> * Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> > Hi
> >
> > On Wed, Aug 23, 2017 at 8:52 AM Peter Xu  wrote:
> >
> > > Firstly, introduce Monitor.use_thread, and set it for monitors that are
> > > using non-mux typed backend chardev.  We only do this for monitors, so
> > > mux-typed chardevs are not suitable (when it connects to, e.g., serials
> > > and the monitor together).
> > >
> > > When use_thread is set, we create standalone thread to poll the monitor
> > > events, isolated from the main loop thread.  Here we still need to take
> > > the BQL before dispatching the tasks since some of the monitor commands
> > > are not allowed to execute without the protection of BQL.  Then this
> > > gives us the chance to avoid taking the BQL for some monitor commands
> in
> > > the future.
> > >
> > > * Why this change?
> > >
> > > We need these per-monitor threads to make sure we can have at least one
> > > monitor that will never stuck (that can receive further monitor
> > > commands).
> > >
> > > * So when will monitors stuck?  And, how do they stuck?
> > >
> > > After we have postcopy and remote page faults, it's simple to achieve a
> > > stuck in the monitor (which is also a stuck in main loop thread):
> > >
> > > (1) Monitor deadlock on BQL
> > >
> > > As we may know, when postcopy is running on destination VM, the vcpu
> > > threads can stuck merely any time as long as it tries to access an
> > > uncopied guest page.  Meanwhile, when the stuck happens, it is possible
> > > that the vcpu thread is holding the BQL.  If the page fault is not
> > > handled quickly, you'll find that monitors stop working, which is
> trying
> > > to take the BQL.
> > >
> > > If the page fault cannot be handled correctly (one case is a paused
> > > postcopy, when network is temporarily down), monitors will hang
> > > forever.  Without current patch, that means the main loop hanged.
> We'll
> > > never find a way to talk to VM again.
> > >
> >
> > Could the BQL be pushed down to the monitor commands level instead? That
> > way we wouldn't need a seperate thread to solve the hang on commands that
> > do not need BQL.
>
> If the main thread is stuck though I don't see how that helps you; you
> have to be able to run these commands on another thread.
>

Why would the main thread be stuck? In (1) If the vcpu thread takes the BQL
and the command doesn't need it, it would work.  In (2),  info cpus
shouldn't keep the BQL (my qapi-async series would probably help here)


> Dave
>
> > We could also optionnally make command that need the BQL to fail if lock
> is
> > held (after a timeout)?
> >
> >
> >
> > > (2) Monitor tries to run codes page-faulted vcpus
> > >
> > > The HMP command "info cpus" is one of the good example - it tries to
> > > kick all the vcpus and sync status from them.  However, if there is any
> > > vcpu that stuck at an unhandled page fault, it can never achieve the
> > > sync, then the HMP hangs.  Again, it hangs the main loop thread as
> well.
> > >
> > > After either (1) or (2), we can see the deadlock problem:
> > >
> > > - On one hand, if monitor hangs, we cannot do the postcopy recovery,
> > >   because postcopy recovery needs user to specify new listening port on
> > >   destination monitor.
> > >
> > > - On the other hand, if we cannot recover the paused postcopy, then
> page
> > >   faults cannot be serviced, and the monitors will possibly hang
> > >   forever then.
> > >
> > > * How this patch helps?
> > >
> > > - Firstly, we'll have our own thread for each dedicated monitor (or
> say,
> > >   the backend chardev is only used for monitor), so even main loop
> > >   thread hangs (it is always possible), this monitor thread may still
> > >   survive.
> > >
> > > - Not all monitor commands need the BQL.  We can selectively take the
> > >   BQL (depends on which command we are running) to avoid waiting on a
> > >   page-faulted vcpu thread that has taken the BQL (this will be done in
> > >   following up patches).
> > >
> > > Signed-off-by: Peter Xu 
> > > ---
> > >  monitor.c   | 75
> > > +
> > >  qapi/qmp-dispatch.c | 15 +++
> > >  2 files changed, 85 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/monitor.c b/monitor.c
> > > index 7c90df7..3d4ecff 100644
> > > --- a/monitor.c
> > > +++ b/monitor.c
> > > @@ -36,6 +36,8 @@
> > >  #include "net/net.h"
> > >  #include "net/slirp.h"
> > >  #include "chardev/char-fe.h"
> > > +#include "chardev/char-mux.h"
> > > +#include "chardev/char-io.h"
> > >  #include "ui/qemu-spice.h"
> > >  #include "sysemu/numa.h"
> > >  #include "monitor/monitor.h"
> > > @@ -190,6 +192,8 @@ struct Monitor {
> > >  int flags;
> > >  int suspend_cnt;
> > >  bool skip_flush;
> > > +/* Whether the monitor wants to be polled in standalone thread */
> > > +bool 

[Qemu-devel] [PATCH 2/3] docker.py: Python 2.6 argparse compatibility

[Stefan Hajnoczi]

Add the scripts/ directory to sys.path so Python 2.6 will be able to
import argparse.

Cc: Fam Zheng 
Signed-off-by: Stefan Hajnoczi 
---
 tests/docker/docker.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/docker/docker.py b/tests/docker/docker.py
index ee40ca04d9..81c87ee329 100755
--- a/tests/docker/docker.py
+++ b/tests/docker/docker.py
@@ -13,12 +13,14 @@
 
 import os
 import sys
+sys.path.append(os.path.join(os.path.dirname(__file__),
+ '..', '..', 'scripts'))
+import argparse
 import subprocess
 import json
 import hashlib
 import atexit
 import uuid
-import argparse
 import tempfile
 import re
 import signal
-- 
2.13.5

[Qemu-devel] [PATCH 0/3] scripts: add argparse module for Python 2.6 compatibility

[Stefan Hajnoczi]

Many scripts can benefit from the standard library argparse module, which has
improvements over the older optparse module.  Unfortunately argparse was only
shipped in Python 2.7 so we need a fallback for Python 2.6.

This patch series adds a copy of argparse.py and updates scripts as necessary
to import it.

Stefan Hajnoczi (3):
  scripts: add argparse module for Python 2.6 compatibility
  docker.py: Python 2.6 argparse compatibility
  tests: migration/guestperf Python 2.6 argparse compatibility

 COPYING.PYTHON |  270 
 scripts/argparse.py| 2406 
 tests/docker/docker.py |4 +-
 tests/migration/guestperf/shell.py |8 +-
 4 files changed, 2684 insertions(+), 4 deletions(-)
 create mode 100644 COPYING.PYTHON
 create mode 100644 scripts/argparse.py

-- 
2.13.5

[Qemu-devel] [PATCH 1/3] scripts: add argparse module for Python 2.6 compatibility

[Stefan Hajnoczi]

The minimum Python version supported by QEMU is 2.6.  The argparse
standard library module was only added in Python 2.7.  Many scripts
would like to use argparse because it supports command-line
sub-commands.

This patch adds argparse.  See the top of argparse.py for details.

Suggested-by: Daniel P. Berrange 
Signed-off-by: Stefan Hajnoczi 
---
 COPYING.PYTHON  |  270 ++
 scripts/argparse.py | 2406 +++
 2 files changed, 2676 insertions(+)
 create mode 100644 COPYING.PYTHON
 create mode 100644 scripts/argparse.py

diff --git a/COPYING.PYTHON b/COPYING.PYTHON
new file mode 100644
index 00..4d3f1ef276
--- /dev/null
+++ b/COPYING.PYTHON
@@ -0,0 +1,270 @@
+A. HISTORY OF THE SOFTWARE
+==
+
+Python was created in the early 1990s by Guido van Rossum at Stichting
+Mathematisch Centrum (CWI, see http://www.cwi.nl) in the Netherlands
+as a successor of a language called ABC.  Guido remains Python's
+principal author, although it includes many contributions from others.
+
+In 1995, Guido continued his work on Python at the Corporation for
+National Research Initiatives (CNRI, see http://www.cnri.reston.va.us)
+in Reston, Virginia where he released several versions of the
+software.
+
+In May 2000, Guido and the Python core development team moved to
+BeOpen.com to form the BeOpen PythonLabs team.  In October of the same
+year, the PythonLabs team moved to Digital Creations (now Zope
+Corporation, see http://www.zope.com).  In 2001, the Python Software
+Foundation (PSF, see http://www.python.org/psf/) was formed, a
+non-profit organization created specifically to own Python-related
+Intellectual Property.  Zope Corporation is a sponsoring member of
+the PSF.
+
+All Python releases are Open Source (see http://www.opensource.org for
+the Open Source Definition).  Historically, most, but not all, Python
+releases have also been GPL-compatible; the table below summarizes
+the various releases.
+
+Release Derived YearOwner   GPL-
+fromcompatible? (1)
+
+0.9.0 thru 1.2  1991-1995   CWI yes
+1.3 thru 1.5.2  1.2 1995-1999   CNRIyes
+1.6 1.5.2   2000CNRIno
+2.0 1.6 2000BeOpen.com  no
+1.6.1   1.6 2001CNRIyes (2)
+2.1 2.0+1.6.1   2001PSF no
+2.0.1   2.0+1.6.1   2001PSF yes
+2.1.1   2.1+2.0.1   2001PSF yes
+2.2 2.1.1   2001PSF yes
+2.1.2   2.1.1   2002PSF yes
+2.1.3   2.1.2   2002PSF yes
+2.2.1   2.2 2002PSF yes
+2.2.2   2.2.1   2002PSF yes
+2.2.3   2.2.2   2003PSF yes
+2.3 2.2.2   2002-2003   PSF yes
+2.3.1   2.3 2002-2003   PSF yes
+2.3.2   2.3.1   2002-2003   PSF yes
+2.3.3   2.3.2   2002-2003   PSF yes
+2.3.4   2.3.3   2004PSF yes
+2.3.5   2.3.4   2005PSF yes
+2.4 2.3 2004PSF yes
+2.4.1   2.4 2005PSF yes
+2.4.2   2.4.1   2005PSF yes
+2.4.3   2.4.2   2006PSF yes
+2.5 2.4 2006PSF yes
+2.7 2.6 2010PSF yes
+
+Footnotes:
+
+(1) GPL-compatible doesn't mean that we're distributing Python under
+the GPL.  All Python licenses, unlike the GPL, let you distribute
+a modified version without making your changes open source.  The
+GPL-compatible licenses make it possible to combine Python with
+other software that is released under the GPL; the others don't.
+
+(2) According to Richard Stallman, 1.6.1 is not GPL-compatible,
+because its license has a choice of law clause.  According to
+CNRI, however, Stallman's lawyer has told CNRI's lawyer that 1.6.1
+is "not incompatible" with the GPL.
+
+Thanks to the many outside volunteers who have worked under Guido's
+direction to make these releases possible.
+
+
+B. TERMS AND CONDITIONS FOR ACCESSING OR OTHERWISE USING PYTHON
+===
+
+PYTHON SOFTWARE FOUNDATION LICENSE VERSION 2
+
+
+1. This LICENSE AGREEMENT is between the Python Software Foundation
+("PSF"), and the Individual or Organization ("Licensee") accessing and
+otherwise using this software ("Python") in source or binary form and
+its associated documentation.
+
+2. Subject to the terms and 

[Qemu-devel] [PATCH 3/3] tests: migration/guestperf Python 2.6 argparse compatibility

[Stefan Hajnoczi]

Add the scripts/ directory to sys.path so Python 2.6 will be able to
import argparse.

Cc: Daniel P. Berrange 
Signed-off-by: Stefan Hajnoczi 
---
 tests/migration/guestperf/shell.py | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/tests/migration/guestperf/shell.py 
b/tests/migration/guestperf/shell.py
index 185c5697a6..7992459a97 100644
--- a/tests/migration/guestperf/shell.py
+++ b/tests/migration/guestperf/shell.py
@@ -18,12 +18,14 @@
 #
 
 
-import argparse
-import fnmatch
 import os
 import os.path
-import platform
 import sys
+sys.path.append(os.path.join(os.path.dirname(__file__),
+ '..', '..', '..', 'scripts'))
+import argparse
+import fnmatch
+import platform
 
 from guestperf.hardware import Hardware
 from guestperf.engine import Engine
-- 
2.13.5

Re: [Qemu-devel] [PATCH 1/3] nbd-client: enter read_reply_co during init to avoid crash

[Vladimir Sementsov-Ogievskiy]

24.08.2017 20:42, Paolo Bonzini wrote:

On 24/08/2017 19:37, Eric Blake wrote:

On 08/24/2017 11:21 AM, Paolo Bonzini wrote:

On 24/08/2017 17:33, Stefan Hajnoczi wrote:

This patch enters read_reply_co directly in
nbd_client_attach_aio_context().  This is safe because new_context is
acquired by the caller.  This ensures that read_reply_co reaches its
first yield point and its ctx is set up.

I'm not very confident with this patch.  aio_context_acquire/release is
going to go away, and this then becomes possible

main contextnew_context
qemu_aio_coroutine_enter
send request
wait for reply
read first reply
wake coroutine

where the "wake coroutine" part thinks it's running in new_context, and
thus simply enters the coroutine instead of using the bottom half.

But blk_co_preadv() should need the read_reply_co itself, in order to be
woken up after reading the reply header.  The core issue here is that
nbd_co_receive_reply was never called, I suspect.  And if it was never
called, read_reply_co should not be woken up by nbd_coroutine_end.

So the fix is:

1) assign NULL to s->recv_coroutine[i] when nbd_co_send_request fails

2) move this to nbd_co_receive_reply:

 s->recv_coroutine[i] = NULL;

 /* Kick the read_reply_co to get the next reply.  */
 if (s->read_reply_co) {
 aio_co_wake(s->read_reply_co);
 }

Does this make sense?  (Note that the read_reply_co idea actually came
from you, or from my recollections of your proposed design :)).

How much of this overlaps with Vladimir's proposal?
https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg00846.html

The above should be about 15 lines added, 10 removed. :)

Paolo




I'll be on vocation for the next two weeks and will continue this work 
after it.


I think we have a lot of conflicts already with my series after 
different fixes,


so I'll rebase on them all, no problem.


PS: I think recent problems and bugs in nbd are related to not very good 
separation between


block/nbd-client.c and nbd/client.c. Ideally, I think, block/nbd-client 
should not


directly access the ioc, it should just call one or two high level 
functions of


nbd/client. The big problem of this separation is CMD_READ reply - only 
real client


knows, should we read a payload or not. I have two ideas on it:

1. We can add this information to request handle, then nbd/client will 
now by handle,


  that it should read the payload.. The length of payload should be in 
handle too. It looks


 possible, as handle is 64bit when request len is 32bit.

2. Move part of NBDClientSession to nbd/client, with NBDClientRequest 
requests[MAX_NBD_REQUESTS];


  to nbd/client, to implement one public function nbd_request(state, 
*request, *reply), which will do all the


 work for block/nbd-client..


--
Best regards,
Vladimir

[Qemu-devel] [PATCH] block/curl: wake read completion coroutine only if necessary

[Evgeny Yakovlev]

When curl_co_preadv is called it sets up an ACB block which points to
current coroutine. It will then call curl_setup_preadv and wait until
request is completed by polling return status and yeilding:

curl_setup_preadv(bs, );
while (acb.ret == -EINPROGRESS) {
qemu_coroutine_yield();
}

curl_setup_preadv will ask libcurl to handle read request and to use
curl_read_cb as completion callback for each completed chunk.

When curl_read_cb sees request as completed it will attempt to wake up
issuing coroutine assuming that yield was called previously:

qemu_mutex_unlock(>s->mutex);
aio_co_wake(acb->co);
qemu_mutex_lock(>s->mutex);

However if request is short enough (< 16K in our test) curl_read_cb will
be called right away before returning from libcurl to curl_setup_preadv.
Request will be completed before yield was called from the same
coroutine, which asserts in aio_co_enter.

This change attempts to fix this by waking completion coroutine only if
it is not the current one.

Signed-off-by: Evgeny Yakovlev 
---
 block/curl.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index 2a244e2..b1106d6 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -271,9 +271,12 @@ static size_t curl_read_cb(void *ptr, size_t size, size_t 
nmemb, void *opaque)
 
 acb->ret = 0;
 s->acb[i] = NULL;
-qemu_mutex_unlock(>s->mutex);
-aio_co_wake(acb->co);
-qemu_mutex_lock(>s->mutex);
+
+if (qemu_coroutine_self() != acb->co) {
+qemu_mutex_unlock(>s->mutex);
+aio_co_wake(acb->co);
+qemu_mutex_lock(>s->mutex);
+}
 }
 }
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/2] migration: Reset rather than destroy main_thread_load_event

[Dr. David Alan Gilbert]

* Peter Maydell (peter.mayd...@linaro.org) wrote:
> On 25 August 2017 at 15:19, Dr. David Alan Gilbert (git)
>  wrote:
> > From: "Dr. David Alan Gilbert" 
> >
> > migration_incoming_state_destroy doesn't really destroy, it cleans up.
> > After a loadvm it's called, but the loadvm command can be run twice,
> > and so destroying an init-once mutex breaks on the second loadvm.
> >
> > Reported-by: Stafford Horne 
> > Signed-off-by: Dr. David Alan Gilbert 
> > ---
> >  migration/migration.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/migration/migration.c b/migration/migration.c
> > index c3fe0ed9ca..a625551ce5 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -167,7 +167,7 @@ void migration_incoming_state_destroy(void)
> >  mis->from_src_file = NULL;
> >  }
> >
> > -qemu_event_destroy(>main_thread_load_event);
> > +qemu_event_reset(>main_thread_load_event);
> >  }
> 
> Is it worth doing more here?

In the longer-term, yes;  it seemed right just to get
this bug fixed first though.

> For instance:
>  * rename the function to something that better reflects
>what it's doing
>  * make sure it actually goes back to the state it's in
>after you first call migration_incoming_get_current()
>(eg setting mis_current.state, zeroing various fields)
>  * maybe instead of a "get current state" that does a
>reset if it's not been called before and a "destroy"
>that does cleanup stuff (like telling the source we've
>stopped) and also resetting back to clean state, we
>could structure this with a function that does
>"give me a clean completely reset state" which you
>must call first, then use get_current() purely to
>get the current state with no 'reset on first call'
>semantics, and finally a "complete" function that just
>does the "tell source we've stopped" and close
>resources we no longer need  ?

Yes; an init/current/clean does make sense;  one of my comments
on one of Peter's patchsets was to point out the
migration_incoming_get_current isn't thread safe if you're
not sure you've already called it, so it could do with fixing.

There has also been a few things that have wanted to gather stats
that are available after the end of an incoming migration;
so we don't really want to destroy that state, we just want
to get rid of anything temporary.

You could argue that this thread_load_event is best init'd
at the start of the incoming migration and then destroyed at the
end.

> PS, in migration_incoming_get_current() we do
> mis_current.state = MIGRATION_STATUS_NONE;
> memset(_current, 0, sizeof(MigrationIncomingState));
> 
> and the first line there is pointless because the memset
> blasts zeroes over it anyway.

Hmm yes.

Dave

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

Re: [Qemu-devel] [PATCH v2 02/54] qdict: add qdict_put_null() helper

[Eric Blake]

On 08/22/2017 10:09 AM, Markus Armbruster wrote:

>#define qdict_put_bool(qdict, key, value) \
>>  qdict_put(qdict, key, qbool_from_bool(value))
>>  #define qdict_put_str(qdict, key, value) \
>>  qdict_put(qdict, key, qstring_from_str(value))
>> +#define qdict_put_null(qdict, key) \
>> +qdict_put(qdict, key, qnull())
>>  
>>  /* High level helpers */
>>  double qdict_get_double(const QDict *qdict, const char *key);
> 
> Marginal.  I can accept it for completeness's sake, or rather a step
> towards completeness.  But please update the "Helpers for ..." comment,
> and convert existing qdict_put(QD, K, qnull()) to use qdict_put_null().
> A quick grep finds some in target/i386/cpu.c.  There might be more.

And we already used Coccinelle to find candidates for qdict_put_bool and
friends, so that should be the reasonable approach to use here as well.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [Bug 1713066] [NEW] Incorrect handling of aarch64 ldp in some cases

[Peter Maydell]

On 25 August 2017 at 14:50, Andrew  wrote:
> Given the following instruction:
> ldp x0, x1, [x0]
>
> This will load two 64 bit values from memory, however if each location
> to load is on a different page and the second page is unmapped this will
> raise an exception. When this happens x0 has already been updated

Yes, this is a QEMU bug. disas_ldst_pair() should not let the
first load go directly to the target integer register but instead
postpone updating the register until after the second load.
We can safely do this only for the integer load case because
float/vector registers can't be used in address generation so
they're OK to become UNKNOWN.
(D1.14.5 is about interrupts and exceptions that happen during
a multiple-register load or store; for straightforward synchronous
data aborts D1.13.4 is what you want, but the requirements are the
same in any case.)

We got this right for the load/store exclusive pair, so it's only
the plain load pair that needs fixing I think.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 13/14] qlit: add qobject_form_qlit()

[Eric Blake]

On 08/25/2017 05:59 AM, Marc-André Lureau wrote:

s/form/from/ in the subject

> Signed-off-by: Marc-André Lureau 
> ---
>  include/qapi/qmp/qlit.h |  2 ++
>  qobject/qlit.c  | 37 +
>  tests/check-qlit.c  | 26 ++
>  3 files changed, 65 insertions(+)
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [RFC v2 2/8] monitor: allow monitor to create thread to poll

[Dr. David Alan Gilbert]

* Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> Hi
> 
> On Wed, Aug 23, 2017 at 8:52 AM Peter Xu  wrote:
> 
> > Firstly, introduce Monitor.use_thread, and set it for monitors that are
> > using non-mux typed backend chardev.  We only do this for monitors, so
> > mux-typed chardevs are not suitable (when it connects to, e.g., serials
> > and the monitor together).
> >
> > When use_thread is set, we create standalone thread to poll the monitor
> > events, isolated from the main loop thread.  Here we still need to take
> > the BQL before dispatching the tasks since some of the monitor commands
> > are not allowed to execute without the protection of BQL.  Then this
> > gives us the chance to avoid taking the BQL for some monitor commands in
> > the future.
> >
> > * Why this change?
> >
> > We need these per-monitor threads to make sure we can have at least one
> > monitor that will never stuck (that can receive further monitor
> > commands).
> >
> > * So when will monitors stuck?  And, how do they stuck?
> >
> > After we have postcopy and remote page faults, it's simple to achieve a
> > stuck in the monitor (which is also a stuck in main loop thread):
> >
> > (1) Monitor deadlock on BQL
> >
> > As we may know, when postcopy is running on destination VM, the vcpu
> > threads can stuck merely any time as long as it tries to access an
> > uncopied guest page.  Meanwhile, when the stuck happens, it is possible
> > that the vcpu thread is holding the BQL.  If the page fault is not
> > handled quickly, you'll find that monitors stop working, which is trying
> > to take the BQL.
> >
> > If the page fault cannot be handled correctly (one case is a paused
> > postcopy, when network is temporarily down), monitors will hang
> > forever.  Without current patch, that means the main loop hanged.  We'll
> > never find a way to talk to VM again.
> >
> 
> Could the BQL be pushed down to the monitor commands level instead? That
> way we wouldn't need a seperate thread to solve the hang on commands that
> do not need BQL.

If the main thread is stuck though I don't see how that helps you; you
have to be able to run these commands on another thread.

Dave

> We could also optionnally make command that need the BQL to fail if lock is
> held (after a timeout)?
> 
> 
> 
> > (2) Monitor tries to run codes page-faulted vcpus
> >
> > The HMP command "info cpus" is one of the good example - it tries to
> > kick all the vcpus and sync status from them.  However, if there is any
> > vcpu that stuck at an unhandled page fault, it can never achieve the
> > sync, then the HMP hangs.  Again, it hangs the main loop thread as well.
> >
> > After either (1) or (2), we can see the deadlock problem:
> >
> > - On one hand, if monitor hangs, we cannot do the postcopy recovery,
> >   because postcopy recovery needs user to specify new listening port on
> >   destination monitor.
> >
> > - On the other hand, if we cannot recover the paused postcopy, then page
> >   faults cannot be serviced, and the monitors will possibly hang
> >   forever then.
> >
> > * How this patch helps?
> >
> > - Firstly, we'll have our own thread for each dedicated monitor (or say,
> >   the backend chardev is only used for monitor), so even main loop
> >   thread hangs (it is always possible), this monitor thread may still
> >   survive.
> >
> > - Not all monitor commands need the BQL.  We can selectively take the
> >   BQL (depends on which command we are running) to avoid waiting on a
> >   page-faulted vcpu thread that has taken the BQL (this will be done in
> >   following up patches).
> >
> > Signed-off-by: Peter Xu 
> > ---
> >  monitor.c   | 75
> > +
> >  qapi/qmp-dispatch.c | 15 +++
> >  2 files changed, 85 insertions(+), 5 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index 7c90df7..3d4ecff 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -36,6 +36,8 @@
> >  #include "net/net.h"
> >  #include "net/slirp.h"
> >  #include "chardev/char-fe.h"
> > +#include "chardev/char-mux.h"
> > +#include "chardev/char-io.h"
> >  #include "ui/qemu-spice.h"
> >  #include "sysemu/numa.h"
> >  #include "monitor/monitor.h"
> > @@ -190,6 +192,8 @@ struct Monitor {
> >  int flags;
> >  int suspend_cnt;
> >  bool skip_flush;
> > +/* Whether the monitor wants to be polled in standalone thread */
> > +bool use_thread;
> >
> >  QemuMutex out_lock;
> >  QString *outbuf;
> > @@ -206,6 +210,11 @@ struct Monitor {
> >  mon_cmd_t *cmd_table;
> >  QLIST_HEAD(,mon_fd_t) fds;
> >  QLIST_ENTRY(Monitor) entry;
> > +
> > +/* Only used when "use_thread" is used */
> > +QemuThread mon_thread;
> > +GMainContext *mon_context;
> > +GMainLoop *mon_loop;
> >  };
> >
> >  /* QMP checker flags */
> > @@ -568,7 +577,7 @@ static void monitor_qapi_event_init(void)
> >
> >  static void handle_hmp_command(Monitor *mon, const 

Re: [Qemu-devel] [PATCH v2 01/14] qdict: add qdict_put_null() helper

[Eric Blake]

On 08/25/2017 05:59 AM, Marc-André Lureau wrote:
> A step towards completeness.
> 
> Signed-off-by: Marc-André Lureau 
> Reviewed-by: Markus Armbruster 
> ---
>  include/qapi/qmp/qdict.h | 4 +++-
>  target/i386/cpu.c| 4 ++--
>  2 files changed, 5 insertions(+), 3 deletions(-)

Is it worth touching up scripts/coccinelle/qobject.cocci at the same time?

I guess we don't care about a qlist_append_null() variant?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 1/2] migration: Reset rather than destroy main_thread_load_event

[Peter Maydell]

On 25 August 2017 at 15:19, Dr. David Alan Gilbert (git)
 wrote:
> From: "Dr. David Alan Gilbert" 
>
> migration_incoming_state_destroy doesn't really destroy, it cleans up.
> After a loadvm it's called, but the loadvm command can be run twice,
> and so destroying an init-once mutex breaks on the second loadvm.
>
> Reported-by: Stafford Horne 
> Signed-off-by: Dr. David Alan Gilbert 
> ---
>  migration/migration.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index c3fe0ed9ca..a625551ce5 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -167,7 +167,7 @@ void migration_incoming_state_destroy(void)
>  mis->from_src_file = NULL;
>  }
>
> -qemu_event_destroy(>main_thread_load_event);
> +qemu_event_reset(>main_thread_load_event);
>  }

Is it worth doing more here? For instance:
 * rename the function to something that better reflects
   what it's doing
 * make sure it actually goes back to the state it's in
   after you first call migration_incoming_get_current()
   (eg setting mis_current.state, zeroing various fields)
 * maybe instead of a "get current state" that does a
   reset if it's not been called before and a "destroy"
   that does cleanup stuff (like telling the source we've
   stopped) and also resetting back to clean state, we
   could structure this with a function that does
   "give me a clean completely reset state" which you
   must call first, then use get_current() purely to
   get the current state with no 'reset on first call'
   semantics, and finally a "complete" function that just
   does the "tell source we've stopped" and close
   resources we no longer need  ?

PS, in migration_incoming_get_current() we do
mis_current.state = MIGRATION_STATUS_NONE;
memset(_current, 0, sizeof(MigrationIncomingState));

and the first line there is pointless because the memset
blasts zeroes over it anyway.

thanks
-- PMM

Re: [Qemu-devel] [RFC v2 04/32] qemu_ram_block_host_offset

[Dr. David Alan Gilbert]

* Philippe Mathieu-Daudé (f4...@amsat.org) wrote:
> Hi David,
> 
> On 08/24/2017 04:27 PM, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" 
> > 
> > Utility to give the offset of a host pointer within a RAMBlock
> > (assuming we already know it's in that RAMBlock)
> > 
> > Signed-off-by: Dr. David Alan Gilbert 
> > ---
> >   exec.c| 10 ++
> >   include/exec/cpu-common.h |  1 +
> >   2 files changed, 11 insertions(+)
> > 
> > diff --git a/exec.c b/exec.c
> > index 67df2909ce..35b4cea2ed 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -2231,6 +2231,16 @@ static void *qemu_ram_ptr_length(RAMBlock 
> > *ram_block, ram_addr_t addr,
> >   return ramblock_ptr(block, addr);
> >   }
> > +/* Return the offset of a hostpointer within a ramblock */
> > +ram_addr_t qemu_ram_block_host_offset(RAMBlock *rb, void *host)
> > +{
> > +ram_addr_t res = (uint8_t *)host - (uint8_t *)rb->host;
> 
> What about using ptrdiff_t here,

We tend to use ram_addr_t for offsets in RAM, and so that's
the return type of the function, and we're also comparing this
value to rb->max_length below which is also a ram_addr_t.

> > +assert((uint8_t *)host >= (uint8_t *)rb->host);
> 
> and uintptr_t here?

Done.

Thanks,

Dave

> > +assert(res < rb->max_length);
> > +
> > +return res;
> > +}
> > +
> >   /*
> >* Translates a host ptr back to a RAMBlock, a ram_addr and an offset
> >* in that RAMBlock.
> > diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> > index 74341b19d2..0d861a6289 100644
> > --- a/include/exec/cpu-common.h
> > +++ b/include/exec/cpu-common.h
> > @@ -68,6 +68,7 @@ ram_addr_t qemu_ram_addr_from_host(void *ptr);
> >   RAMBlock *qemu_ram_block_by_name(const char *name);
> >   RAMBlock *qemu_ram_block_from_host(void *ptr, bool round_offset,
> >  ram_addr_t *offset);
> > +ram_addr_t qemu_ram_block_host_offset(RAMBlock *rb, void *host);
> >   void qemu_ram_set_idstr(RAMBlock *block, const char *name, DeviceState 
> > *dev);
> >   void qemu_ram_unset_idstr(RAMBlock *block);
> >   const char *qemu_ram_get_idstr(RAMBlock *rb);
> > 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [RFC v2 2/8] monitor: allow monitor to create thread to poll

[Marc-André Lureau]

Hi

On Wed, Aug 23, 2017 at 8:52 AM Peter Xu  wrote:

> Firstly, introduce Monitor.use_thread, and set it for monitors that are
> using non-mux typed backend chardev.  We only do this for monitors, so
> mux-typed chardevs are not suitable (when it connects to, e.g., serials
> and the monitor together).
>
> When use_thread is set, we create standalone thread to poll the monitor
> events, isolated from the main loop thread.  Here we still need to take
> the BQL before dispatching the tasks since some of the monitor commands
> are not allowed to execute without the protection of BQL.  Then this
> gives us the chance to avoid taking the BQL for some monitor commands in
> the future.
>
> * Why this change?
>
> We need these per-monitor threads to make sure we can have at least one
> monitor that will never stuck (that can receive further monitor
> commands).
>
> * So when will monitors stuck?  And, how do they stuck?
>
> After we have postcopy and remote page faults, it's simple to achieve a
> stuck in the monitor (which is also a stuck in main loop thread):
>
> (1) Monitor deadlock on BQL
>
> As we may know, when postcopy is running on destination VM, the vcpu
> threads can stuck merely any time as long as it tries to access an
> uncopied guest page.  Meanwhile, when the stuck happens, it is possible
> that the vcpu thread is holding the BQL.  If the page fault is not
> handled quickly, you'll find that monitors stop working, which is trying
> to take the BQL.
>
> If the page fault cannot be handled correctly (one case is a paused
> postcopy, when network is temporarily down), monitors will hang
> forever.  Without current patch, that means the main loop hanged.  We'll
> never find a way to talk to VM again.
>

Could the BQL be pushed down to the monitor commands level instead? That
way we wouldn't need a seperate thread to solve the hang on commands that
do not need BQL.

We could also optionnally make command that need the BQL to fail if lock is
held (after a timeout)?



> (2) Monitor tries to run codes page-faulted vcpus
>
> The HMP command "info cpus" is one of the good example - it tries to
> kick all the vcpus and sync status from them.  However, if there is any
> vcpu that stuck at an unhandled page fault, it can never achieve the
> sync, then the HMP hangs.  Again, it hangs the main loop thread as well.
>
> After either (1) or (2), we can see the deadlock problem:
>
> - On one hand, if monitor hangs, we cannot do the postcopy recovery,
>   because postcopy recovery needs user to specify new listening port on
>   destination monitor.
>
> - On the other hand, if we cannot recover the paused postcopy, then page
>   faults cannot be serviced, and the monitors will possibly hang
>   forever then.
>
> * How this patch helps?
>
> - Firstly, we'll have our own thread for each dedicated monitor (or say,
>   the backend chardev is only used for monitor), so even main loop
>   thread hangs (it is always possible), this monitor thread may still
>   survive.
>
> - Not all monitor commands need the BQL.  We can selectively take the
>   BQL (depends on which command we are running) to avoid waiting on a
>   page-faulted vcpu thread that has taken the BQL (this will be done in
>   following up patches).
>
> Signed-off-by: Peter Xu 
> ---
>  monitor.c   | 75
> +
>  qapi/qmp-dispatch.c | 15 +++
>  2 files changed, 85 insertions(+), 5 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 7c90df7..3d4ecff 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -36,6 +36,8 @@
>  #include "net/net.h"
>  #include "net/slirp.h"
>  #include "chardev/char-fe.h"
> +#include "chardev/char-mux.h"
> +#include "chardev/char-io.h"
>  #include "ui/qemu-spice.h"
>  #include "sysemu/numa.h"
>  #include "monitor/monitor.h"
> @@ -190,6 +192,8 @@ struct Monitor {
>  int flags;
>  int suspend_cnt;
>  bool skip_flush;
> +/* Whether the monitor wants to be polled in standalone thread */
> +bool use_thread;
>
>  QemuMutex out_lock;
>  QString *outbuf;
> @@ -206,6 +210,11 @@ struct Monitor {
>  mon_cmd_t *cmd_table;
>  QLIST_HEAD(,mon_fd_t) fds;
>  QLIST_ENTRY(Monitor) entry;
> +
> +/* Only used when "use_thread" is used */
> +QemuThread mon_thread;
> +GMainContext *mon_context;
> +GMainLoop *mon_loop;
>  };
>
>  /* QMP checker flags */
> @@ -568,7 +577,7 @@ static void monitor_qapi_event_init(void)
>
>  static void handle_hmp_command(Monitor *mon, const char *cmdline);
>
> -static void monitor_data_init(Monitor *mon, bool skip_flush)
> +static void monitor_data_init(Monitor *mon, bool skip_flush, bool
> use_thread)
>  {
>  memset(mon, 0, sizeof(Monitor));
>  qemu_mutex_init(>out_lock);
> @@ -576,10 +585,34 @@ static void monitor_data_init(Monitor *mon, bool
> skip_flush)
>  /* Use *mon_cmds by default. */
>  mon->cmd_table = mon_cmds;
>  mon->skip_flush = 

[Qemu-devel] [Bug 1713066] [NEW] Incorrect handling of aarch64 ldp in some cases

[Andrew]

Public bug reported:

In some cases the ldp instruction (and presumably other multi-register
loads and stores) can behave incorrectly.

Given the following instruction:
ldp x0, x1, [x0]

This will load two 64 bit values from memory, however if each location
to load is on a different page and the second page is unmapped this will
raise an exception. When this happens x0 has already been updated so
after the exception handler has run the operating system will try to
rerun the instruction. QEMU will now try to perform an invalid load and
raise a new exception.

I believe this is incorrect as section D.1.14.5 of the ARMv8 reference
manual B.a states that, on taking an exception, registers used in the
generation of addresses are restored to their initial value, so x0
shouldn't be changed, where x1 can be un an unknown state.

I found the issue running FreeBSD with the cortex-strings implementation
of memcpy. This uses a similar instruction when copying between 64 and
96 bytes.

I've observed this on:
QEMU emulator version 2.5.0 (Debian 1:2.5+dfsg-5ubuntu10.14), Copyright (c) 
2003-2008 Fabrice Bellard

And checked I still get the same behaviour on:
QEMU emulator version 2.9.94 (v2.10.0-rc4-dirty)
Git revision: 248b23735645f7cbb503d9be6f5bf825f2a603ab

** Affects: qemu
 Importance: Undecided
 Status: New

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

Title:
  Incorrect handling of aarch64 ldp in some cases

Status in QEMU:
  New

Bug description:
  In some cases the ldp instruction (and presumably other multi-register
  loads and stores) can behave incorrectly.

  Given the following instruction:
  ldp x0, x1, [x0]

  This will load two 64 bit values from memory, however if each location
  to load is on a different page and the second page is unmapped this
  will raise an exception. When this happens x0 has already been updated
  so after the exception handler has run the operating system will try
  to rerun the instruction. QEMU will now try to perform an invalid load
  and raise a new exception.

  I believe this is incorrect as section D.1.14.5 of the ARMv8 reference
  manual B.a states that, on taking an exception, registers used in the
  generation of addresses are restored to their initial value, so x0
  shouldn't be changed, where x1 can be un an unknown state.

  I found the issue running FreeBSD with the cortex-strings
  implementation of memcpy. This uses a similar instruction when copying
  between 64 and 96 bytes.

  I've observed this on:
  QEMU emulator version 2.5.0 (Debian 1:2.5+dfsg-5ubuntu10.14), Copyright (c) 
2003-2008 Fabrice Bellard

  And checked I still get the same behaviour on:
  QEMU emulator version 2.9.94 (v2.10.0-rc4-dirty)
  Git revision: 248b23735645f7cbb503d9be6f5bf825f2a603ab

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

Re: [Qemu-devel] [PATCH qemu] pci: Initialize pci_dev->name before use

[Eric Blake]

On 08/24/2017 10:21 PM, Alexey Kardashevskiy wrote:
> This moves pci_dev->name initialization earlier so
> pci_dev->bus_master_as could get a name instead of empry string.

s/empry/an empty/

> 
> Signed-off-by: Alexey Kardashevskiy 
> ---

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PULL 09/21] pci: Convert to realize

[Eduardo Habkost]

On Mon, Jul 03, 2017 at 10:45:16PM +0300, Michael S. Tsirkin wrote:
> From: Mao Zhongyi 
> 
> Convert i82801b11, io3130_upstream, io3130_downstream and
> pcie_root_port devices to realize.
> 
> Cc: m...@redhat.com
> Cc: mar...@redhat.com
> Cc: arm...@redhat.com
> Signed-off-by: Mao Zhongyi 
> Reviewed-by: Marcel Apfelbaum 
> Reviewed-by: Michael S. Tsirkin 
> Signed-off-by: Michael S. Tsirkin 
> ---
[...]
> diff --git a/hw/pci-bridge/xio3130_downstream.c 
> b/hw/pci-bridge/xio3130_downstream.c
> index cfe8a36..e706f36 100644
> --- a/hw/pci-bridge/xio3130_downstream.c
> +++ b/hw/pci-bridge/xio3130_downstream.c
> @@ -56,33 +56,33 @@ static void xio3130_downstream_reset(DeviceState *qdev)
>  pci_bridge_reset(qdev);
>  }
>  
> -static int xio3130_downstream_initfn(PCIDevice *d)
> +static void xio3130_downstream_realize(PCIDevice *d, Error **errp)
>  {
[...]
>  pcie_chassis_create(s->chassis);
>  rc = pcie_chassis_add_slot(s);
>  if (rc < 0) {
>  goto err_pcie_cap;

Missing error_setg() call here.  If pcie_chassis_add_slot() fails, realize
won't report an error properly.  Causes crash with:

$ ./x86_64-softmmu/qemu-system-x86_64 -device ioh3420 -device xio3130-downstream
qemu-system-x86_64: /home/ehabkost/rh/proj/virt/qemu/memory.c:2166: 
memory_region_del_subregion: Assertion `subregion->container == mr' failed.
Aborted (core dumped)


>  }
>  
>  rc = pcie_aer_init(d, PCI_ERR_VER, XIO3130_AER_OFFSET,
> -   PCI_ERR_SIZEOF, );
> +   PCI_ERR_SIZEOF, errp);
>  if (rc < 0) {
> -error_report_err(err);
>  goto err;
>  }
>  
> -return 0;
> +return;
>  
>  err:
>  pcie_chassis_del_slot(s);
> @@ -114,7 +113,6 @@ err_msi:
>  msi_uninit(d);
>  err_bridge:
>  pci_bridge_exitfn(d);
> -return rc;
>  }
>  
[...]

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/3] Test for full Backup

[John Snow]

On 08/25/2017 10:36 AM, Peter Maydell wrote:
> On 23 August 2017 at 14:04, Ishani Chugh
>  wrote:
>> This patch is the test for full backup implementation in Backup tool.
>> The test employs two basic substests:
>> 1) Backing up an empty guest and comparing it with base image.
>> 2) Writing a pattern to the guest, creating backup and comparing
>>with the base image.
>>
>> Signed-off-by: Ishani Chugh 
>> ---
>>  tests/qemu-iotests/191 | 78 
>> ++
>>  tests/qemu-iotests/191.out | 33 
>>  tests/qemu-iotests/group   |  1 +
>>  3 files changed, 112 insertions(+)
>>  create mode 100755 tests/qemu-iotests/191
>>  create mode 100644 tests/qemu-iotests/191.out
>>
>> diff --git a/tests/qemu-iotests/191 b/tests/qemu-iotests/191
>> new file mode 100755
>> index 000..fb4cde9
>> --- /dev/null
>> +++ b/tests/qemu-iotests/191
>> @@ -0,0 +1,78 @@
>> +#!/bin/bash
>> +#
>> +# Test full backup functionality of qemu-backup tool
>> +#
>> +# Copyright (C) 2009 Red Hat, Inc.
> 
> Should this perhaps have a different copyright owner and date?
> 
> thanks
> -- PMM
> 
Yes!

Ishani, please take credit for your own work! :)

Re: [Qemu-devel] [Qemu-block] [PATCH 0/3] nbd-client: enter read_reply_co during init to avoid crash

[Eric Blake]

On 08/24/2017 11:05 AM, Stefan Hajnoczi wrote:
> On Thu, Aug 24, 2017 at 4:52 PM, Eric Blake  wrote:
>> On 08/24/2017 10:33 AM, Stefan Hajnoczi wrote:
>>> See Patch 1 for the segfault fix.  Patches 2 & 3 add qemu-iotests coverage.
>>>
>>> This is a rare crash that we'll probably only see in testing.  It only 
>>> seems to
>>> happen with UNIX domain sockets.
>>
>> Rare enough that I don't think it is 2.10-rc4 material.  I'll review,
>> and if I don't find anything needing a respin, I'll add it to my NBD
>> queue for 2.11.
> 
> Great.  This is 2.11 material.

Hmm - I wonder if https://bugs.launchpad.net/qemu/+bug/1712818 is
related to this.  I really don't want to delay 2.10 further (downstream
distros can backport fixes) - but comment 3 stating that crashes are
less frequent but still possible with -rc4 is not good.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] s390-ccw: Fix alignment for CCW1

[Farhan Ali]

On 08/25/2017 10:04 AM, Cornelia Huck wrote:

On Fri, 25 Aug 2017 09:24:46 -0400
Farhan Ali  wrote:


The commit 198c0d1f9df8c4 s390x/css: check ccw address validity
exposes an alignment issue in ccw bios.

According to PoP the CCW must be doubleword aligned. Let's fix
this in the bios.

Cc: qemu-sta...@nongnu.org
Signed-off-by: Farhan Ali 
Reviewed-by: Halil Pasic 
Reviewed-by: Eric Farman 
Acked-by: Christian Borntraeger 
---
 pc-bios/s390-ccw/cio.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pc-bios/s390-ccw/cio.h b/pc-bios/s390-ccw/cio.h
index f5b4549..55eaeee 100644
--- a/pc-bios/s390-ccw/cio.h
+++ b/pc-bios/s390-ccw/cio.h
@@ -133,7 +133,7 @@ struct ccw1 {
 __u8 flags;
 __u16 count;
 __u32 cda;
-} __attribute__ ((packed));
+} __attribute__ ((packed, aligned(8)));

 #define CCW_FLAG_DC  0x80
 #define CCW_FLAG_CC  0x40


Currently testing.

This looks obviously right, but did you figure out what the (probably
unrelated) other failure was?



That is still under investigation, for some reason it only fails for an 
LDL DASD and it works for SCSIs and CDL DASD.

Re: [Qemu-devel] [PATCH v2 2/3] osdep: Define QEMU_MADV_REMOVE

[Dr. David Alan Gilbert]

* Eduardo Habkost (ehabk...@redhat.com) wrote:
> Define QEMU_MADV_REMOVE, so we can use it with qemu_madvise().
> 
> Signed-off-by: Eduardo Habkost 

Reviewed-by: Dr. David Alan Gilbert 

> ---
> Changes v1 -> v2:
> * New patch added to series
> ---
>  include/qemu/osdep.h | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 6855b94..e9fa217 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -257,6 +257,11 @@ void qemu_anon_ram_free(void *ptr, size_t size);
>  #else
>  #define QEMU_MADV_NOHUGEPAGE QEMU_MADV_INVALID
>  #endif
> +#ifdef MADV_REMOVE
> +#define QEMU_MADV_REMOVE MADV_REMOVE
> +#else
> +#define QEMU_MADV_REMOVE QEMU_MADV_INVALID
> +#endif
>  
>  #elif defined(CONFIG_POSIX_MADVISE)
>  
> @@ -269,6 +274,7 @@ void qemu_anon_ram_free(void *ptr, size_t size);
>  #define QEMU_MADV_DONTDUMP QEMU_MADV_INVALID
>  #define QEMU_MADV_HUGEPAGE  QEMU_MADV_INVALID
>  #define QEMU_MADV_NOHUGEPAGE  QEMU_MADV_INVALID
> +#define QEMU_MADV_REMOVE QEMU_MADV_INVALID
>  
>  #else /* no-op */
>  
> @@ -281,6 +287,7 @@ void qemu_anon_ram_free(void *ptr, size_t size);
>  #define QEMU_MADV_DONTDUMP QEMU_MADV_INVALID
>  #define QEMU_MADV_HUGEPAGE  QEMU_MADV_INVALID
>  #define QEMU_MADV_NOHUGEPAGE  QEMU_MADV_INVALID
> +#define QEMU_MADV_REMOVE QEMU_MADV_INVALID
>  
>  #endif
>  
> -- 
> 2.9.4
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [RFC v2 01/32] vhu: vu_queue_started

[Dr. David Alan Gilbert]

* Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> Hi
> 
> On Thu, Aug 24, 2017 at 9:39 PM Dr. David Alan Gilbert (git) <
> dgilb...@redhat.com> wrote:
> 
> > From: "Dr. David Alan Gilbert" 
> >
> > Add a vu_queue_started method to complement vu_queue_enabled.
> >
> > Signed-off-by: Dr. David Alan Gilbert 
> >
> 
> Reviewed-by: Marc-André Lureau 

Thanks.

> 
> > ---
> >  contrib/libvhost-user/libvhost-user.c | 6 ++
> >  contrib/libvhost-user/libvhost-user.h | 9 +
> >  2 files changed, 15 insertions(+)
> >
> > diff --git a/contrib/libvhost-user/libvhost-user.c
> > b/contrib/libvhost-user/libvhost-user.c
> > index 35fa0c5e56..201b9846e9 100644
> > --- a/contrib/libvhost-user/libvhost-user.c
> > +++ b/contrib/libvhost-user/libvhost-user.c
> > @@ -930,6 +930,12 @@ vu_queue_enabled(VuDev *dev, VuVirtq *vq)
> >  return vq->enable;
> >  }
> >
> > +bool
> > +vu_queue_started(VuDev *dev, VuVirtq *vq)
> >
> 
> I guess we could make it const, but this is true for many other functions.
> Could be done later in one go.

Thanks; I've added the consts.

Dave

> > +{
> > +return vq->started;
> > +}
> > +
> >  static inline uint16_t
> >  vring_avail_flags(VuVirtq *vq)
> >  {
> > diff --git a/contrib/libvhost-user/libvhost-user.h
> > b/contrib/libvhost-user/libvhost-user.h
> > index 53ef222c0b..acd019876d 100644
> > --- a/contrib/libvhost-user/libvhost-user.h
> > +++ b/contrib/libvhost-user/libvhost-user.h
> > @@ -328,6 +328,15 @@ void vu_queue_set_notification(VuDev *dev, VuVirtq
> > *vq, int enable);
> >  bool vu_queue_enabled(VuDev *dev, VuVirtq *vq);
> >
> >  /**
> > + * vu_queue_started:
> > + * @dev: a VuDev context
> > + * @vq: a VuVirtq queue
> > + *
> > + * Returns: whether the queue is started.
> > + */
> > +bool vu_queue_started(VuDev *dev, VuVirtq *vq);
> > +
> > +/**
> >   * vu_queue_empty:
> >   * @dev: a VuDev context
> >   * @vq: a VuVirtq queue
> > --
> > 2.13.5
> >
> >
> > --
> Marc-André Lureau
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

[Qemu-devel] [PATCH for-2.11 v4 06/25] sparc: make cpu feature parsing property based

[Igor Mammedov]

with features converted to properties we can use the same
approach as x86 for features parsing and drop legacy
approach that manipulated CPU instance directly.
New sparc_cpu_parse_features() will allow only +-feat
and explicitly disable feat=on|off syntax for now.

With that in place and sparc_cpu_parse_features() providing
generic CPUClass::parse_features callback, the cpu_sparc_init()
will do the same job as cpu_generic_init() so replace content
of cpu_sparc_init() with it.

Signed-off-by: Igor Mammedov 
Acked-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 
---
CC: Riku Voipio 
CC: Laurent Vivier 
CC: Mark Cave-Ayland 
CC: Artyom Tarasenko 
CC: Philippe Mathieu-Daudé 
CC: Eduardo Habkost 

v4:
  * add comment why feat=on|off isn't supported
  * drop un-needed NULL check before freeing GList
v3:
  * drop cpu_legacy_parse_featurestr() and reimplement
sparc_cpu_parse_features() similar to x86 parser
but without x86 baggage.
v2:
  * use new cpu_legacy_parse_featurestr() without
plus_features/minus_features
  * drop cpu_legacy_apply_features() as it's been removed in
previuos patch and new cpu_legacy_parse_featurestr()
does its job
---
 target/sparc/cpu.c | 215 -
 1 file changed, 82 insertions(+), 133 deletions(-)

diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
index 9dfc150..29296b2 100644
--- a/target/sparc/cpu.c
+++ b/target/sparc/cpu.c
@@ -104,50 +104,99 @@ static void cpu_sparc_disas_set_info(CPUState *cpu, 
disassemble_info *info)
 #endif
 }
 
-static void sparc_cpu_parse_features(CPUState *cs, char *features,
- Error **errp);
+static void
+cpu_add_feat_as_prop(const char *typename, const char *name, const char *val)
+{
+GlobalProperty *prop = g_new0(typeof(*prop), 1);
+prop->driver = typename;
+prop->property = g_strdup(name);
+prop->value = g_strdup(val);
+prop->errp = _fatal;
+qdev_prop_register_global(prop);
+}
 
-static int cpu_sparc_register(SPARCCPU *cpu, const char *cpu_model)
+/* Parse "+feature,-feature,feature=foo" CPU feature string */
+static void sparc_cpu_parse_features(const char *typename, char *features,
+ Error **errp)
 {
-char *s = g_strdup(cpu_model);
-char *featurestr = strtok(s, ",");
-Error *err = NULL;
+GList *l, *plus_features = NULL, *minus_features = NULL;
+char *featurestr; /* Single 'key=value" string being parsed */
+static bool cpu_globals_initialized;
 
-featurestr = strtok(NULL, ",");
-sparc_cpu_parse_features(CPU(cpu), featurestr, );
-g_free(s);
-if (err) {
-error_report_err(err);
-return -1;
+if (cpu_globals_initialized) {
+return;
 }
+cpu_globals_initialized = true;
 
-return 0;
-}
+if (!features) {
+return;
+}
 
-SPARCCPU *cpu_sparc_init(const char *cpu_model)
-{
-SPARCCPU *cpu;
-ObjectClass *oc;
-char *str, *name;
+for (featurestr = strtok(features, ",");
+ featurestr;
+ featurestr = strtok(NULL, ",")) {
+const char *name;
+const char *val = NULL;
+char *eq = NULL;
 
-str = g_strdup(cpu_model);
-name = strtok(str, ",");
-oc = cpu_class_by_name(TYPE_SPARC_CPU, name);
-g_free(str);
-if (oc == NULL) {
-return NULL;
-}
+/* Compatibility syntax: */
+if (featurestr[0] == '+') {
+plus_features = g_list_append(plus_features,
+  g_strdup(featurestr + 1));
+continue;
+} else if (featurestr[0] == '-') {
+minus_features = g_list_append(minus_features,
+   g_strdup(featurestr + 1));
+continue;
+}
 
-cpu = SPARC_CPU(object_new(object_class_get_name(oc)));
+eq = strchr(featurestr, '=');
+name = featurestr;
+if (eq) {
+*eq++ = 0;
+val = eq;
+
+/*
+ * Temporarily, only +feat/-feat will be supported
+ * for boolean properties until we remove the
+ * minus-overrides-plus semantics and just follow
+ * the order options appear on the command-line.
+ *
+ * TODO: warn if user is relying on minus-override-plus semantics
+ * TODO: remove minus-override-plus semantics after
+ *   warning for a few releases
+ */
+if (!strcasecmp(val, "on") ||
+!strcasecmp(val, "off") ||
+!strcasecmp(val, "true") ||
+!strcasecmp(val, "false")) {
+error_setg(errp, "Boolean properties in format %s=%s"
+ " are not supported", 

Re: [Qemu-devel] [RFC v2 3/8] char-io: fix possible risk on IOWatchPoll

[Marc-André Lureau]

On Wed, Aug 23, 2017 at 8:54 AM Peter Xu  wrote:

> This is not a problem if we are only having one single loop thread like
> before.  However, after per-monitor thread is introduced, this is not
> true any more, and the risk can happen.
>
> The risk can be triggered with "make check -j8" sometimes:
>
>   qemu-system-x86_64: /root/git/qemu/chardev/char-io.c:91:
>   io_watch_poll_finalize: Assertion `iwp->src == NULL' failed.
>
> This patch keeps the reference for the watch object when creating in
> io_add_watch_poll(), so that the object will never be released in the
> context main loop, especially when the context loop is running in
> another standalone thread.  Meanwhile, when we want to remove the watch
> object, we always first detach the watch object from its owner context,
> then we continue with the cleanup.
>
> Without this patch, calling io_remove_watch_poll() in main loop thread
> is not thread-safe, since the other per-monitor thread may be modifying
> the watch object at the same time.
>
> Signed-off-by: Peter Xu 
>

Reviewed-by: Marc-André Lureau 



> ---
>  chardev/char-io.c | 15 +--
>  1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/chardev/char-io.c b/chardev/char-io.c
> index f810524..5c52c40 100644
> --- a/chardev/char-io.c
> +++ b/chardev/char-io.c
> @@ -122,7 +122,6 @@ GSource *io_add_watch_poll(Chardev *chr,
>  g_free(name);
>
>  g_source_attach(>parent, context);
> -g_source_unref(>parent);
>  return (GSource *)iwp;
>  }
>
> @@ -131,12 +130,24 @@ static void io_remove_watch_poll(GSource *source)
>  IOWatchPoll *iwp;
>
>  iwp = io_watch_poll_from_source(source);
> +
> +/*
> + * Here the order of destruction really matters.  We need to first
> + * detach the IOWatchPoll object from the context (which may still
> + * be running in another loop thread), only after that could we
> + * continue to operate on iwp->src, or there may be risk condition
> + * between current thread and the context loop thread.
> + *
> + * Let's blame the glib bug mentioned in commit 2b3167 (again) for
> + * this extra complexity.
> + */
> +g_source_destroy(>parent);
>  if (iwp->src) {
>  g_source_destroy(iwp->src);
>  g_source_unref(iwp->src);
>  iwp->src = NULL;
>  }
> -g_source_destroy(>parent);
> +g_source_unref(>parent);
>  }
>
>  void remove_fd_in_watch(Chardev *chr)
> --
> 2.7.4
>
>
> --
Marc-André Lureau

[Qemu-devel] [Bug 1712564] Re: loadvm fails twice in sequence

[Dr. David Alan Gilbert]

Posted:
migration: Reset rather than destroy main_thread_load_event
snapshot/tests: Try loadvm twice

** Changed in: qemu
   Status: New => In Progress

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

Title:
  loadvm fails twice in sequence

Status in QEMU:
  In Progress

Bug description:
  13:38:23) shorne_: Hello, I was doing some testing with migrations for my 
OpenRISC SMP patch set, I noticed something that looks like a bug, wondering if 
someone else wants to confirm
  (13:38:47) shorne_: Basically, calling loadvm 2 times causes crash
  (13:38:54) shorne_:migration/savevm.c:
qemu_event_set(>main_thread_load_event)
  (13:38:54) stefanha: fam: Here is my take at this change: 
https://paste.debian.net/982690/
  (13:38:56) shorne_: assert(ev->initialized)  - 
fails inside
  (13:39:32) stefanha: quintela davidgiluk: ^
  (13:41:23) ***davidgiluk looks
  (13:41:40) shorne_: c096358e747 util/qemu-thread-posix.c (Fam Zheng   
   2017-07-04 20:23:25 +0800 397) assert(ev->initialized);
  (13:41:51) davidgiluk: shorne_: So you're doing a loadvm to load a snapshot 
and then again?
  (13:42:02) shorne_: Looks like adding that assert() was done really recently
  (13:42:41) shorne_: yes, just loadvm 'a' ... then wait a bit longer, loadvm 
'a' again (confirm clocks go back etc)
  (13:42:50) stefanha: fam: While you're having dinner I'll work on turning my 
script into a qemu-iotests test case that we can merge.
  (13:44:03) gpiccoli [~gpicc...@0002093a.user.oftc.net] entered the room.
  (13:44:21) davidgiluk: shorne_: Well, it looks like the c09635 assert is a 
sanity check to make sure we didn't do anything stupid, and well.
  (13:44:57) pm215: migration_incoming_get_current() and 
migration_incoming_state_destroy() seem a bit mismatched
  (13:45:13) davidgiluk: pm215: Yep
  (13:45:46) davidgiluk: pm215: Generally we've thought that an incoming 
migration normally only happens once - shorne_'s case is the exception
  (13:46:03) shorne_: pm215: yeah, it looked something like that I just had a 
few seconds to look at today
  (13:46:03) HariharanTS left the room (quit: Ping timeout: 480 seconds).
  (13:46:03) shorne_ is now known as shorne
  (13:48:05) shorne: davidgiluk: pm215: thanks for having a look.  
Unfortunately I need to head off to bed and put kids to sleep
  (13:49:11) davidgiluk: shorne: Sleep well, no nightmares about event 
destroyers
  (13:49:30) davidgiluk: pm215: Yeh this is fall out from b4b076daf32

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

Re: [Qemu-devel] S390 bios breaks in qemu 2.10.rc3

[Philippe Mathieu-Daudé]

Hi Farhan,

On 08/25/2017 04:20 AM, Cornelia Huck wrote:

On Thu, 24 Aug 2017 20:14:06 +0200
Christian Borntraeger  wrote:


On 08/24/2017 07:38 PM, Farhan Ali wrote:



On 08/24/2017 12:07 PM, Peter Maydell wrote:

On 24 August 2017 at 16:53, Farhan Ali  wrote:



On 08/24/2017 11:50 AM, Thomas Huth wrote:

True, so that could still be an issue. Looking at the cio.h in the
kernel, they define the struct like this:

struct ccw1 {
 __u8  cmd_code;
 __u8  flags;
 __u16 count;
 __u32 cda;
} __attribute__ ((packed,aligned(8)));

So I guess adding the aligned(8) is the right way to go?
  

This was my initial fix and it works on my system. But for some reason this
fix does not work on my colleague's system. So I am hesitant about
submitting this fix


It seems like it ought to be the obvious fix, so I would double
check that on your colleague's system the change really did
get recompiled and it's actually using the new version (that
sort of mistake can be easy to make and very confusing...)




thanks
-- PMM
  


So after trying again with the fix, it seems to work on my colleague's system 
for most cases. It fails for LDL DASD boot case we are still investigating 
it.


no-packed-stack is GCC default but you can test forcing it with:

  make QEMU_CFLAGS=-mno-packed-stack

Also, can you add LDFLAGS+=-Wl,-verbose in pc-bios/s390-ccw/Makefile and 
send your and your colleague bios build output?


I'm interested in comparing the internal linker scripts.



So chances are that this is an independent problem, I guess?



OK, to recap:

- the current pre-built bios seems fine
- rebuilding the bios may yield a version that fails on some systems
   (different compiler?)
- adding aligned(8) looks like the right thing to do
- it seems to fix the problem, but on at least one system something
   still seems off (under investigation)

Re: [Qemu-devel] [PATCH 2/3] Test for full Backup

[Peter Maydell]

On 23 August 2017 at 14:04, Ishani Chugh
 wrote:
> This patch is the test for full backup implementation in Backup tool.
> The test employs two basic substests:
> 1) Backing up an empty guest and comparing it with base image.
> 2) Writing a pattern to the guest, creating backup and comparing
>with the base image.
>
> Signed-off-by: Ishani Chugh 
> ---
>  tests/qemu-iotests/191 | 78 
> ++
>  tests/qemu-iotests/191.out | 33 
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 112 insertions(+)
>  create mode 100755 tests/qemu-iotests/191
>  create mode 100644 tests/qemu-iotests/191.out
>
> diff --git a/tests/qemu-iotests/191 b/tests/qemu-iotests/191
> new file mode 100755
> index 000..fb4cde9
> --- /dev/null
> +++ b/tests/qemu-iotests/191
> @@ -0,0 +1,78 @@
> +#!/bin/bash
> +#
> +# Test full backup functionality of qemu-backup tool
> +#
> +# Copyright (C) 2009 Red Hat, Inc.

Should this perhaps have a different copyright owner and date?

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-2.11 2/6] ppc: make cpu_model translation to type consistent

[Igor Mammedov]

On Fri, 25 Aug 2017 23:28:00 +1000
David Gibson  wrote:

> On Fri, Aug 25, 2017 at 01:40:07PM +0200, Igor Mammedov wrote:
> > On Fri, 25 Aug 2017 19:45:38 +1000
> > David Gibson  wrote:
> >   
> > > On Fri, Aug 25, 2017 at 09:27:40AM +0200, Igor Mammedov wrote:  
> > > > On Fri, 25 Aug 2017 11:38:19 +1000
> > > > David Gibson  wrote:
> > > > 
> > > > > On Thu, Aug 24, 2017 at 10:21:47AM +0200, Igor Mammedov wrote:
> > > > > > PPC handles -cpu FOO rather incosistently,
> > > > > > i.e. it does case-insensitive matching of FOO to
> > > > > > a CPU type (see: ppc_cpu_compare_class_name) but
> > > > > > handles alias names as case-sensitive, as result:
> > > > > > 
> > > > > >  # qemu-system-ppc64 -M mac99 -cpu g3
> > > > > >  qemu-system-ppc64: unable to find CPU model ' kN�U'
> > > > > > 
> > > > > >  # qemu-system-ppc64 -cpu 970MP_V1.1
> > > > > >  qemu-system-ppc64: Unable to find sPAPR CPU Core definition
> > > > > > 
> > > > > > while
> > > > > > 
> > > > > >  # qemu-system-ppc64 -M mac99 -cpu G3
> > > > > >  # qemu-system-ppc64 -cpu 970MP_v1.1
> > > > > > 
> > > > > > start up just fine.
> > > > > > 
> > > > > > Considering we can't take case-insensitive matching away,
> > > > > > make it case-insensitive for  all alias/type/core_type
> > > > > > lookups.
> > > > > > 
> > > > > > As side effect it allows to remove duplicate core types
> > > > > > which are the same but use lower-case letters in name.
> > > > > > 
> > > > > > Signed-off-by: Igor Mammedov   
> > > > > 
> > > > > Hmm.  So, I'm certainly good with making the matching more consistent,
> > > > > and removing extra entries differing only in case.
> > > > > 
> > > > > However, I don't like capitalizing everything in the model table.  The
> > > > > "canonical" capitalization - as in how it appears in manuals and
> > > > > marketing info - is quite frequently mixed case.  So I think making
> > > > > everything caps in the table will make it harder to read in the
> > > > > context of looking at the corresponding documentation.
> > > > only cpu models => typenames are made caps, the rest
> > > 
> > > Yes, I know.  A number of the CPU model names themselves have mixed
> > > case.  Really.
> > >   
> > > > incl. description is kept as is in mixed case.
> > > > It looks like _desc is the thing one would use for
> > > > 1:1 lookup in spec, while _name isn't direct match consistently
> > > > (i.e. sometimes it skips spaces and sometimes spaces are replaced by 
> > > > underscore).
> > > > So keeping _name in mixed case unnecessarily complicates name->type 
> > > > lookup.
> > > > 
> > > > These mixed case + case-insensitive is what made lookup
> > > > code over-engineered and overly complex.
> > > 
> > > I'm fine with making all the lookups case insensitive.  I just don't
> > > want to drop the case on the names in the actual code.  
> > I've tried to find a way but considering that names are statically
> > 'converted' into typenames and lack of ability to magically
> > uppercase them with preprocessor, I've gave up on this approach.  
> 
> Ok.
> 
> > Providing that there is _desc with model name that should be
> > greppable in spec and complexity of lookup code that mixed-cased
> > typenames incur, I've opted in favor of simplifying lookup
> > code at expense uniform typenames (which is inter QEMU thingy).
> > It is easier to maintain and less chances to make mistake.  
> 
> Yeah, I guess.  It just looks weird to have non-standard capsing in
> the table field.
> 
> Any chance we could standardize on lowercase rather than upper - for
> some reason that would seem less odd to me (I guess because C-ish
> names and identifiers are usually lowercased).
The only reason I've picked up uppercase is that diffstat is smaller
then with lowercase.
So if you prefer lowercase, I'll switch case in v2.
 
[...]

Re: [Qemu-devel] [PATCH 2/3] Test for full Backup

[Stefan Hajnoczi]

On Wed, Aug 23, 2017 at 06:34:39PM +0530, Ishani Chugh wrote:
> +CONFIG_FILE=$TEST_DIR/backup-config
> +SOCKET=unix:$TEST_DIR/backup_socket
> +size=128M
> +
> +_make_test_img $size
> +export QEMU_BACKUP_CONFIG=$CONFIG_FILE
> +qemu_comm_method="monitor"
> +echo
> +_launch_qemu -drive if=virtio,file=$TEST_IMG -qmp $SOCKET,server,nowait

I tend to put doublequotes around any variable expansion unless the
variable is trivially guaranteed to contain no spaces.  Imagine
TEST_DIR="a b", then your command expands to:

  '-qmp' 'unix:a' 'b/backup_socket,server,nowait'

QEMU will think the option is -qmp unix:a only.

If you use doublequotes then the space will not split the argument into
multiple arguments:

  -qmp "$SOCKET",server,nowait

expands to:

  '-qmp' 'unix:a b/backup_socket,server,nowait'

Please use doublequotes around all variable expansion.

> +$PYTHON ../../contrib/backup/qemu-backup.py guest add --guest adad --qmp 
> $SOCKET
> +$PYTHON ../../contrib/backup/qemu-backup.py drive add --id virtio0 --guest 
> adad --target $TEST_DIR/virtio0
> +echo
> +echo "== Creating backup =="
> +$PYTHON ../../contrib/backup/qemu-backup.py backup --guest adad
> +_send_qemu_cmd $QEMU_HANDLE 'quit' ''
> +wait=1 _cleanup_qemu
> +echo
> +echo "== Comparing images =="
> +$QEMU_IMG compare $TEST_DIR/virtio0 $TEST_IMG
> +rm $TEST_DIR/virtio0
> +
> +_launch_qemu -drive if=virtio,id=virtio0,file=$TEST_IMG -qmp 
> $SOCKET,server,nowait
> +echo
> +echo "== Writing Pattern =="
> +_send_qemu_cmd $QEMU_HANDLE 'qemu-io virtio0 "write -P 0x22 0 1M"' "(qemu)" 
> | _filter_qemu_io
> +echo
> +echo "== Creating backup =="
> +$PYTHON ../../contrib/backup/qemu-backup.py backup --guest adad
> +_send_qemu_cmd $QEMU_HANDLE 'quit' ''
> +wait=1 _cleanup_qemu
> +echo
> +echo "== Comparing images =="
> +$QEMU_IMG compare $TEST_DIR/virtio0 $TEST_IMG
> +rm $TEST_DIR/virtio0
> +rm $CONFIG_FILE

All cleanup should be done from a trap handler function.  This way the
cleanup happens even if the process terminates early:

_cleanup()
{
rm -f "$TEST_DIR"/virtio0
rm -f "$CONFIG_FILE"
_cleanup_test_img
}
trap "_cleanup; exit \$status" 0 1 2 3 15

[Qemu-devel] [PATCH 0/2] Fix double loadvm failure

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" 

lp: https://bugs.launchpad.net/qemu/+bug/1712564

Reported-by: Stafford Horne 

Dr. David Alan Gilbert (2):
  migration: Reset rather than destroy main_thread_load_event
  snapshot/tests: Try loadvm twice

 migration/migration.c  | 2 +-
 tests/qemu-iotests/068 | 2 +-
 tests/qemu-iotests/068.out | 4 
 3 files changed, 6 insertions(+), 2 deletions(-)

-- 
2.13.5

[Qemu-devel] [PATCH 2/2] snapshot/tests: Try loadvm twice

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" 

It's legal to loadvm twice, modify the existing save/loadvm test
to do it twice.

Signed-off-by: Dr. David Alan Gilbert 
---
 tests/qemu-iotests/068 | 2 +-
 tests/qemu-iotests/068.out | 4 
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/068 b/tests/qemu-iotests/068
index cfa0f2aed5..e7fca6a494 100755
--- a/tests/qemu-iotests/068
+++ b/tests/qemu-iotests/068
@@ -78,7 +78,7 @@ for extra_args in \
 # Give qemu some time to boot before saving the VM state
 { sleep 1; printf "savevm 0\nquit\n"; } | _qemu $extra_args
 # Now try to continue from that VM state (this should just work)
-echo quit | _qemu $extra_args -loadvm 0
+{ sleep 1; printf "loadvm 0\nloadvm 0\nquit\n"; } | _qemu $extra_args -S
 done
 
 # success, all done
diff --git a/tests/qemu-iotests/068.out b/tests/qemu-iotests/068.out
index aa063cf711..f07a938a38 100644
--- a/tests/qemu-iotests/068.out
+++ b/tests/qemu-iotests/068.out
@@ -7,6 +7,8 @@ QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) savevm 0
 (qemu) quit
 QEMU X.Y.Z monitor - type 'help' for more information
+(qemu) loadvm 0
+(qemu) loadvm 0
 (qemu) quit
 
 === Saving and reloading a VM state to/from a qcow2 image (-object 
iothread,id=iothread0 -set device.hba0.iothread=iothread0) ===
@@ -16,5 +18,7 @@ QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) savevm 0
 (qemu) quit
 QEMU X.Y.Z monitor - type 'help' for more information
+(qemu) loadvm 0
+(qemu) loadvm 0
 (qemu) quit
 *** done
-- 
2.13.5

[Qemu-devel] [PATCH 1/2] migration: Reset rather than destroy main_thread_load_event

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" 

migration_incoming_state_destroy doesn't really destroy, it cleans up.
After a loadvm it's called, but the loadvm command can be run twice,
and so destroying an init-once mutex breaks on the second loadvm.

Reported-by: Stafford Horne 
Signed-off-by: Dr. David Alan Gilbert 
---
 migration/migration.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/migration.c b/migration/migration.c
index c3fe0ed9ca..a625551ce5 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -167,7 +167,7 @@ void migration_incoming_state_destroy(void)
 mis->from_src_file = NULL;
 }
 
-qemu_event_destroy(>main_thread_load_event);
+qemu_event_reset(>main_thread_load_event);
 }
 
 static void migrate_generate_event(int new_state)
-- 
2.13.5

Re: [Qemu-devel] [PATCH v2 01/54] qapi: fix type_seen key error

[Marc-André Lureau]

On Fri, Aug 25, 2017 at 2:57 PM Eduardo Habkost  wrote:

> On Fri, Aug 25, 2017 at 08:02:26AM +0200, Markus Armbruster wrote:
> > Conflicts with Eduardo's "[PATCH v2] qapi: Fix error handling code on
> > alternate conflict".
> > Message-Id: <20170717180926.14924-1-ehabk...@redhat.com>
> >
> > Marc-André, could you have a look?  You can rebase your fix on top of
> > Eduardo's, or merge the two into one commit.
>
> Both patches seem to be fixing the same bug.
>

Yes, it is solved differently. But take Eduardo's patch, it has more
extensive tests .

>
> >
> > Marc-André Lureau  writes:
> >
> > > The type_seen member can be of a different type than the 'qtype' being
> > > checked, since a string create several conflicts. Lookup the real
> > > conflicting type in the conflict set, that one must be present in
> > > type_seen.
> > >
> > > This fixes the following error, reproducible with the modified test:
> > >
> > > Traceback (most recent call last):
> > >   File "/home/elmarco/src/qq/tests/qapi-schema/test-qapi.py", line 56,
> in 
> > > schema = QAPISchema(sys.argv[1])
> > >   File "/home/elmarco/src/qq/scripts/qapi.py", line 1470, in __init__
> > > self.exprs = check_exprs(parser.exprs)
> > >   File "/home/elmarco/src/qq/scripts/qapi.py", line 959, in check_exprs
> > > check_alternate(expr, info)
> > >   File "/home/elmarco/src/qq/scripts/qapi.py", line 831, in
> check_alternate
> > > % (name, key, types_seen[qtype]))
> > > KeyError: 'QTYPE_QSTRING'
> > >
> > > Signed-off-by: Marc-André Lureau 
> > > ---
> > >  scripts/qapi.py  | 6 --
> > >  tests/qapi-schema/alternate-conflict-string.json | 4 ++--
> > >  2 files changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/scripts/qapi.py b/scripts/qapi.py
> > > index 8aa2775f12..a3ac799535 100644
> > > --- a/scripts/qapi.py
> > > +++ b/scripts/qapi.py
> > > @@ -825,10 +825,12 @@ def check_alternate(expr, info):
> > >  else:
> > >  conflicting.add('QTYPE_QNUM')
> > >  conflicting.add('QTYPE_QBOOL')
> > > -if conflicting & set(types_seen):
> > > +conflict = conflicting & set(types_seen)
> > > +if conflict:
> > > +conflict_qtype = list(conflict)[0]
> > >  raise QAPISemError(info, "Alternate '%s' member '%s'
> can't "
> > > "be distinguished from member '%s'"
> > > -   % (name, key, types_seen[qtype]))
> > > +   % (name, key,
> types_seen[conflict_qtype]))
> > >  for qt in conflicting:
> > >  types_seen[qt] = key
> > >
> > > diff --git a/tests/qapi-schema/alternate-conflict-string.json
> b/tests/qapi-schema/alternate-conflict-string.json
> > > index 85adbd4adc..bb2702978e 100644
> > > --- a/tests/qapi-schema/alternate-conflict-string.json
> > > +++ b/tests/qapi-schema/alternate-conflict-string.json
> > > @@ -1,4 +1,4 @@
> > >  # alternate branches of 'str' type conflict with all scalar types
> > >  { 'alternate': 'Alt',
> > > -  'data': { 'one': 'str',
> > > -'two': 'int' } }
> > > +  'data': { 'one': 'int',
> > > +'two': 'str' } }
>
> --
> Eduardo
>
> --
Marc-André Lureau

Re: [Qemu-devel] [PATCH 1/3] backup: QEMU Backup Tool

[Stefan Hajnoczi]

On Wed, Aug 23, 2017 at 06:34:38PM +0530, Ishani Chugh wrote:
> +def write_config(self):
> +"""
> +Writes configuration to ini file.
> +"""
> +config_file = open(self.config_file+".tmp", 'w')

Please use whitespace around arithmetic operators:

  self.config_file + ".tmp"

> +self.config.write(config_file)
> +config_file.flush()
> +os.fsync(config_file.fileno())
> +config_file.close()
> +os.rename(self.config_file+".tmp", self.config_file)

self.config_file + ".tmp"

> +
> +def get_socket_address(self, socket_address):
> +"""
> +Return Socket address in form of string or tuple
> +"""
> +if socket_address.startswith('tcp'):
> +return (socket_address.split(':')[1],
> +int(socket_address.split(':')[2]))
> +return socket_address.split(':', 2)[1]
> +
> +def _full_backup(self, guest_name):
> +"""
> +Performs full backup of guest
> +"""
> +if guest_name not in self.config.sections():
> +print("Cannot find specified guest", file=sys.stderr)
> +sys.exit(1)
> +
> +self.verify_guest_running(guest_name)
> +connection = QEMUMonitorProtocol(
> + self.get_socket_address(
> + self.config[guest_name]['qmp']))
> +connection.connect()
> +cmd = {"execute": "transaction", "arguments": {"actions": []}}
> +drive_list = []
> +for key in self.config[guest_name]:
> +if key.startswith("drive_"):
> +drive = key[len('drive_'):]
> +drive_list.append(drive)
> +target = self.config[guest_name][key]
> +sub_cmd = {"type": "drive-backup", "data": {"device": drive,
> +"target": target,
> +"sync": "full"}}
> +cmd['arguments']['actions'].append(sub_cmd)
> +qmp_return = connection.cmd_obj(cmd)
> +if 'error' in qmp_return:
> +print(qmp_return['error']['desc'], file=sys.stderr)
> +sys.exit(1)
> +print("Backup Started")
> +while len(drive_list) != 0:

PEP8 says:

  For sequences, (strings, lists, tuples), use the fact that empty sequences 
are false.

  Yes: if not seq:
   if seq:

  No: if len(seq):
  if not len(seq):

This statement should be:

  while drive_list:

> +event = connection.pull_event(wait=True)
> +print(event)

Please remove debugging or add a --verbose command-line option that
makes the print statement conditional:

  if verbose:
  print(event)

> +if event['event'] == 'SHUTDOWN':
> +print("The guest was SHUT DOWN", file=sys.stderr)
> +sys.exit(1)
> +
> +if event['event'] == 'RESET':
> +print("The guest was Rebooted", file=sys.stderr)
> +sys.exit(1)

I think you found this is a soft reset and the blockjob is still
running.  There is no need to exit.  You can ignore this event.

> +
> +if event['event'] == 'BLOCK_JOB_COMPLETED':
> +if event['data']['device'] in drive_list and \
> +event['data']['type'] == 'backup':
> +print("*"+event['data']['device'])

Please use whitespace around arithmetic operators:

  print("*" + event['data']['device'])

> +drive_list.remove(event['data']['device'])
> +
> +if event['event'] == 'BLOCK_JOB_ERROR':
> +if event['data']['device'] in drive_list and \
> +event['data']['type'] == 'backup':
> +print(event['data']['device']+" Backup Failed", 
> file=sys.stderr)

The drive was not removed from drive_list so the loop never terminates.

sys.exit(1)?

Re: [Qemu-devel] [RFC PATCH qemu] exec: Destroy dispatch immediately

[Paolo Bonzini]

On 25/08/2017 15:19, David Gibson wrote:
> On Fri, Aug 25, 2017 at 11:57:26AM +0200, Paolo Bonzini wrote:
>> On 25/08/2017 11:22, Peter Maydell wrote:
>>> On 25 August 2017 at 09:53, Paolo Bonzini  wrote:
 The solution is to: 1) share the FlatView structures if they refer to
 the same root memory region; 2) have one AddressSpaceDispatch per
 FlatView instead of one per AddressSpace, so that FlatView reference
 counting takes care of clearing the AddressSpaceDispatch too.  Neither
 is particularly hard.
>>> If we did this we could get rid of address_space_init_shareable(),
>>> right? (It's a bit of a cheesy hack aimed at avoiding having duplicate
>>> address space structures for the same root memory region, but if
>>> the underlying code is better at not duplicating all the data
>>> structures unless necessary then the benefit of having the
>>> separate API goes away I think.)
>>
>> Yes, indeed.
> 
> Hm.  Why do we need to construct full ASes for virtio-blk, rather than
> just MRs?

It's an artifact of how virtio_address_space_read and
virtio_address_space_write are implemented.

Paolo



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] s390-ccw: Fix alignment for CCW1

[Cornelia Huck]

On Fri, 25 Aug 2017 09:24:46 -0400
Farhan Ali  wrote:

> The commit 198c0d1f9df8c4 s390x/css: check ccw address validity
> exposes an alignment issue in ccw bios.
> 
> According to PoP the CCW must be doubleword aligned. Let's fix
> this in the bios.
> 
> Cc: qemu-sta...@nongnu.org
> Signed-off-by: Farhan Ali 
> Reviewed-by: Halil Pasic 
> Reviewed-by: Eric Farman 
> Acked-by: Christian Borntraeger 
> ---
>  pc-bios/s390-ccw/cio.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/pc-bios/s390-ccw/cio.h b/pc-bios/s390-ccw/cio.h
> index f5b4549..55eaeee 100644
> --- a/pc-bios/s390-ccw/cio.h
> +++ b/pc-bios/s390-ccw/cio.h
> @@ -133,7 +133,7 @@ struct ccw1 {
>  __u8 flags;
>  __u16 count;
>  __u32 cda;
> -} __attribute__ ((packed));
> +} __attribute__ ((packed, aligned(8)));
>  
>  #define CCW_FLAG_DC  0x80
>  #define CCW_FLAG_CC  0x40

Currently testing.

This looks obviously right, but did you figure out what the (probably
unrelated) other failure was?

Re: [Qemu-devel] [PATCH 7/9] AHCI: Rework IRQ constants

[Philippe Mathieu-Daudé]

Hi John,

On 08/08/2017 03:33 PM, John Snow wrote:

Create a new enum so that we can name the IRQ bits, which will make debugging
them a little nicer if we can print them out. Not handled in this patch, but
this will make it possible to get a nice debug printf detailing exactly which
status bits are set, as it can be multiple at any given time.

As a consequence of this patch, it is no longer possible to set multiple IRQ
codes at once, but nothing was utilizing this ability anyway.

Signed-off-by: John Snow 
---
  hw/ide/ahci.c  | 49 ++---
  hw/ide/ahci_internal.h | 44 +++-
  hw/ide/trace-events|  2 +-
  3 files changed, 74 insertions(+), 21 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index d5acceb..c5a9b69 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -56,6 +56,27 @@ static bool ahci_map_fis_address(AHCIDevice *ad);
  static void ahci_unmap_clb_address(AHCIDevice *ad);
  static void ahci_unmap_fis_address(AHCIDevice *ad);
  
+static const char *AHCIPortIRQ_lookup[AHCI_PORT_IRQ__END] = {

+[AHCI_PORT_IRQ_BIT_DHRS] = "DHRS",
+[AHCI_PORT_IRQ_BIT_PSS]  = "PSS",
+[AHCI_PORT_IRQ_BIT_DSS]  = "DSS",
+[AHCI_PORT_IRQ_BIT_SDBS] = "SDBS",
+[AHCI_PORT_IRQ_BIT_UFS]  = "UFS",
+[AHCI_PORT_IRQ_BIT_DPS]  = "DPS",
+[AHCI_PORT_IRQ_BIT_PCS]  = "PCS",
+[AHCI_PORT_IRQ_BIT_DMPS] = "DMPS",
+[8 ... 21]   = "RESERVED",
+[AHCI_PORT_IRQ_BIT_PRCS] = "PRCS",
+[AHCI_PORT_IRQ_BIT_IPMS] = "IPMS",
+[AHCI_PORT_IRQ_BIT_OFS]  = "OFS",
+[25] = "RESERVED",
+[AHCI_PORT_IRQ_BIT_INFS] = "INFS",
+[AHCI_PORT_IRQ_BIT_IFS]  = "IFS",
+[AHCI_PORT_IRQ_BIT_HBDS] = "HBDS",
+[AHCI_PORT_IRQ_BIT_HBFS] = "HBFS",
+[AHCI_PORT_IRQ_BIT_TFES] = "TFES",
+[AHCI_PORT_IRQ_BIT_CPDS] = "CPDS"
+};
  
  static uint32_t  ahci_port_read(AHCIState *s, int port, int offset)

  {
@@ -170,12 +191,18 @@ static void ahci_check_irq(AHCIState *s)
  }
  
  static void ahci_trigger_irq(AHCIState *s, AHCIDevice *d,

- int irq_type)
+ enum AHCIPortIRQ irqbit)
  {
-DPRINTF(d->port_no, "trigger irq %#x -> %x\n",
-irq_type, d->port_regs.irq_mask & irq_type);
+g_assert(irqbit >= 0 && irqbit < 32);


Since you now use an enum this check is no more necessary.

However ...


+uint32_t irq = 1U << irqbit;
+uint32_t irqstat = d->port_regs.irq_stat | irq;
  
-d->port_regs.irq_stat |= irq_type;

+trace_ahci_trigger_irq(s, d->port_no,
+   AHCIPortIRQ_lookup[irqbit], irq,
+   d->port_regs.irq_stat, irqstat,
+   irqstat & d->port_regs.irq_mask);
+


Why not keep using masked irqs, and iterate over AHCI_PORT_IRQ__END 
bits? Something like:


if (TRACE_AHCI_IRQ_ENABLED) {
int irqbit;
for (irqbit = 0; irqbit < AHCI_PORT_IRQ__END; irqbit++) {
if (irqmask & BIT(irqbit)) {
trace_ahci_trigger_irq(s, d->port_no, ...


+d->port_regs.irq_stat = irqstat;
  ahci_check_irq(s);
  }
  
@@ -718,7 +745,7 @@ static void ahci_write_fis_sdb(AHCIState *s, NCQTransferState *ncq_tfs)
  
  /* Trigger IRQ if interrupt bit is set (which currently, it always is) */

  if (sdb_fis->flags & 0x40) {
-ahci_trigger_irq(s, ad, PORT_IRQ_SDB_FIS);
+ahci_trigger_irq(s, ad, AHCI_PORT_IRQ_BIT_SDBS);
  }
  }
  
@@ -761,10 +788,10 @@ static void ahci_write_fis_pio(AHCIDevice *ad, uint16_t len)

  ad->port.ifs[0].status;
  
  if (pio_fis[2] & ERR_STAT) {

-ahci_trigger_irq(ad->hba, ad, PORT_IRQ_TF_ERR);
+ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_TFES);
  }
  
-ahci_trigger_irq(ad->hba, ad, PORT_IRQ_PIOS_FIS);

+ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_PSS);
  }
  
  static bool ahci_write_fis_d2h(AHCIDevice *ad)

@@ -804,10 +831,10 @@ static bool ahci_write_fis_d2h(AHCIDevice *ad)
  ad->port.ifs[0].status;
  
  if (d2h_fis[2] & ERR_STAT) {

-ahci_trigger_irq(ad->hba, ad, PORT_IRQ_TF_ERR);
+ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_TFES);
  }
  
-ahci_trigger_irq(ad->hba, ad, PORT_IRQ_D2H_REG_FIS);

+ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_DHRS);
  return true;
  }
  
@@ -1082,7 +1109,7 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,

   "is smaller than the requested size (0x%zx)",
   ncq_tfs->sglist.size, size);
  ncq_err(ncq_tfs);
-ahci_trigger_irq(ad->hba, ad, PORT_IRQ_OVERFLOW);
+ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_OFS);
  return;
  } else if (ncq_tfs->sglist.size != size) {
  trace_process_ncq_command_large(s, port, tag,
@@ -1225,7 +1252,7 @@ static int handle_cmd(AHCIState *s, int port, uint8_t 
slot)
  

Re: [Qemu-devel] [PATCH] s390-ccw: Fix alignment for CCW1

[no-reply]

Hi,

This series failed automatic build test. Please find the testing commands and
their output below. If you have docker installed, you can probably reproduce it
locally.

Type: series
Message-id: 
3ed8b810b6592daee6a775037ce21f850e40647d.1503667215.git.al...@linux.vnet.ibm.com
Subject: [Qemu-devel] [PATCH] s390-ccw: Fix alignment for CCW1

=== TEST SCRIPT BEGIN ===
#!/bin/bash
set -e
git submodule update --init dtc
# Let docker tests dump environment info
export SHOW_ENV=1
export J=8
time make docker-test-quick@centos6
time make docker-test-build@min-glib
time make docker-test-mingw@fedora
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
2c85119b84 s390-ccw: Fix alignment for CCW1

=== OUTPUT BEGIN ===
Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc'
Cloning into '/var/tmp/patchew-tester-tmp-8eozvhi_/src/dtc'...
Submodule path 'dtc': checked out '558cd81bdd432769b59bff01240c44f82cfb1a9d'
  BUILD   centos6
make[1]: Entering directory '/var/tmp/patchew-tester-tmp-8eozvhi_/src'
  ARCHIVE qemu.tgz
  ARCHIVE dtc.tgz
  COPYRUNNER
RUN test-quick in qemu:centos6 
Packages installed:
SDL-devel-1.2.14-7.el6_7.1.x86_64
bison-2.4.1-5.el6.x86_64
bzip2-devel-1.0.5-7.el6_0.x86_64
ccache-3.1.6-2.el6.x86_64
csnappy-devel-0-6.20150729gitd7bc683.el6.x86_64
flex-2.5.35-9.el6.x86_64
gcc-4.4.7-18.el6.x86_64
git-1.7.1-8.el6.x86_64
glib2-devel-2.28.8-9.el6.x86_64
libepoxy-devel-1.2-3.el6.x86_64
libfdt-devel-1.4.0-1.el6.x86_64
librdmacm-devel-1.0.21-0.el6.x86_64
lzo-devel-2.03-3.1.el6_5.1.x86_64
make-3.81-23.el6.x86_64
mesa-libEGL-devel-11.0.7-4.el6.x86_64
mesa-libgbm-devel-11.0.7-4.el6.x86_64
package g++ is not installed
pixman-devel-0.32.8-1.el6.x86_64
spice-glib-devel-0.26-8.el6.x86_64
spice-server-devel-0.12.4-16.el6.x86_64
tar-1.23-15.el6_8.x86_64
vte-devel-0.25.1-9.el6.x86_64
xen-devel-4.6.3-15.el6.x86_64
zlib-devel-1.2.3-29.el6.x86_64

Environment variables:
PACKAGES=bison bzip2-devel ccache csnappy-devel flex g++
 gcc git glib2-devel libepoxy-devel libfdt-devel 
librdmacm-devel lzo-devel make mesa-libEGL-devel 
mesa-libgbm-devel pixman-devel SDL-devel spice-glib-devel 
spice-server-devel tar vte-devel xen-devel zlib-devel
HOSTNAME=91fb69074736
TERM=xterm
MAKEFLAGS= -j8
HISTSIZE=1000
J=8
USER=root
LS_COLORS=rs=0:di=01;34:ln=01;36:mh=00:pi=40;33:so=01;35:do=01;35:bd=40;33;01:cd=40;33;01:or=40;31;01:mi=01;05;37;41:su=37;41:sg=30;43:ca=30;41:tw=30;42:ow=34;42:st=37;44:ex=01;32:*.tar=01;31:*.tgz=01;31:*.arj=01;31:*.taz=01;31:*.lzh=01;31:*.lzma=01;31:*.tlz=01;31:*.txz=01;31:*.zip=01;31:*.z=01;31:*.Z=01;31:*.dz=01;31:*.gz=01;31:*.lz=01;31:*.xz=01;31:*.bz2=01;31:*.tbz=01;31:*.tbz2=01;31:*.bz=01;31:*.tz=01;31:*.deb=01;31:*.rpm=01;31:*.jar=01;31:*.rar=01;31:*.ace=01;31:*.zoo=01;31:*.cpio=01;31:*.7z=01;31:*.rz=01;31:*.jpg=01;35:*.jpeg=01;35:*.gif=01;35:*.bmp=01;35:*.pbm=01;35:*.pgm=01;35:*.ppm=01;35:*.tga=01;35:*.xbm=01;35:*.xpm=01;35:*.tif=01;35:*.tiff=01;35:*.png=01;35:*.svg=01;35:*.svgz=01;35:*.mng=01;35:*.pcx=01;35:*.mov=01;35:*.mpg=01;35:*.mpeg=01;35:*.m2v=01;35:*.mkv=01;35:*.ogm=01;35:*.mp4=01;35:*.m4v=01;35:*.mp4v=01;35:*.vob=01;35:*.qt=01;35:*.nuv=01;35:*.wmv=01;35:*.asf=01;35:*.rm=01;35:*.rmvb=01;35:*.flc=01;35:*.avi=01;35:*.fli=01;35:*.flv=01;35:*.gl=01;35:*.dl=01;35:*.xcf=01;35:*.xwd=01;35:*.yuv=01;35:*.cgm=01;35:*.emf=01;35:*.axv=01;35:*.anx=01;35:*.ogv=01;35:*.ogx=01;35:*.aac=01;36:*.au=01;36:*.flac=01;36:*.mid=01;36:*.midi=01;36:*.mka=01;36:*.mp3=01;36:*.mpc=01;36:*.ogg=01;36:*.ra=01;36:*.wav=01;36:*.axa=01;36:*.oga=01;36:*.spx=01;36:*.xspf=01;36:
CCACHE_DIR=/var/tmp/ccache
EXTRA_CONFIGURE_OPTS=
V=
SHOW_ENV=1
MAIL=/var/spool/mail/root
PATH=/usr/lib/ccache:/usr/lib64/ccache:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
PWD=/
LANG=en_US.UTF-8
TARGET_LIST=
HISTCONTROL=ignoredups
SHLVL=1
HOME=/root
TEST_DIR=/tmp/qemu-test
LOGNAME=root
LESSOPEN=||/usr/bin/lesspipe.sh %s
FEATURES= dtc
DEBUG=
G_BROKEN_FILENAMES=1
CCACHE_HASHDIR=
_=/usr/bin/env

Configure options:
--enable-werror --target-list=x86_64-softmmu,aarch64-softmmu 
--prefix=/var/tmp/qemu-build/install
No C++ compiler available; disabling C++ specific optional code
Install prefix/var/tmp/qemu-build/install
BIOS directory/var/tmp/qemu-build/install/share/qemu
binary directory  /var/tmp/qemu-build/install/bin
library directory /var/tmp/qemu-build/install/lib
module directory  /var/tmp/qemu-build/install/lib/qemu
libexec directory /var/tmp/qemu-build/install/libexec
include directory /var/tmp/qemu-build/install/include
config directory  /var/tmp/qemu-build/install/etc
local state directory   /var/tmp/qemu-build/install/var
Manual directory  /var/tmp/qemu-build/install/share/man
ELF interp prefix /usr/gnemul/qemu-%M
Source path   /tmp/qemu-test/src
C compilercc
Host C compiler   cc
C++ compiler  
Objective-C compiler cc
ARFLAGS   rv
CFLAGS 

Re: [Qemu-devel] [PATCH 9/9] AHCI: remove DPRINTF macro

[Philippe Mathieu-Daudé]

On 08/08/2017 03:33 PM, John Snow wrote:

Signed-off-by: John Snow 


Reviewed-by: Philippe Mathieu-Daudé 


---
  hw/ide/ahci.c | 9 -
  1 file changed, 9 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 1a4d4db..ce2010c 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -34,17 +34,8 @@
  #include "hw/ide/pci.h"
  #include "hw/ide/ahci_internal.h"
  
-#define DEBUG_AHCI 0

  #include "trace.h"
  
-#define DPRINTF(port, fmt, ...) \

-do { \
-if (DEBUG_AHCI) { \
-fprintf(stderr, "ahci: %s: [%d] ", __func__, port); \
-fprintf(stderr, fmt, ## __VA_ARGS__); \
-} \
-} while (0)
-
  static void check_cmd(AHCIState *s, int port);
  static int handle_cmd(AHCIState *s, int port, uint8_t slot);
  static void ahci_reset_port(AHCIState *s, int port);

Re: [Qemu-devel] [PATCH 5/9] IDE: replace DEBUG_AIO with trace events

[Philippe Mathieu-Daudé]

Hi John,

On 08/08/2017 03:33 PM, John Snow wrote:

Signed-off-by: John Snow 
---
  hw/ide/atapi.c|  5 +
  hw/ide/core.c | 17 ++---
  hw/ide/trace-events   |  3 +++
  include/hw/ide/internal.h |  7 +--
  4 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 37fa699..b8fc51e 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -416,10 +416,7 @@ static void ide_atapi_cmd_read_dma_cb(void *opaque, int 
ret)
  s->io_buffer_size = n * 2048;
  data_offset = 0;
  }
-#ifdef DEBUG_AIO
-printf("aio_read_cd: lba=%u n=%d\n", s->lba, n);
-#endif
-
+trace_ide_atapi_cmd_read_dma_cb_aio(s, s->lba, n);
  s->bus->dma->iov.iov_base = (void *)(s->io_buffer + data_offset);
  s->bus->dma->iov.iov_len = n * ATAPI_SECTOR_SIZE;
  qemu_iovec_init_external(>bus->dma->qiov, >bus->dma->iov, 1);
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 29848ff..1358029 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -58,6 +58,13 @@ static const int smart_attributes[][12] = {
  { 190,  0x03, 0x00, 0x45, 0x45, 0x1f, 0x00, 0x1f, 0x1f, 0x00, 0x00, 0x32},
  };
  
+const char *IDE_DMA_CMD_lookup[IDE_DMA__END] = {

+[IDE_DMA_READ] = "DMA_READ",
+[IDE_DMA_WRITE] = "DMA_WRITE",
+[IDE_DMA_TRIM] = "DMA TRIM",
+[IDE_DMA_ATAPI] = "DMA ATAPI"
+};
+
  static void ide_dummy_transfer_stop(IDEState *s);
  
  static void padstr(char *str, const char *src, int len)

@@ -860,10 +867,8 @@ static void ide_dma_cb(void *opaque, int ret)
  goto eot;
  }
  
-#ifdef DEBUG_AIO

-printf("ide_dma_cb: sector_num=%" PRId64 " n=%d, cmd_cmd=%d\n",
-   sector_num, n, s->dma_cmd);
-#endif
+trace_ide_dma_cb(s, sector_num, n,
+ IDE_DMA_CMD_lookup[s->dma_cmd]);
  
  if ((s->dma_cmd == IDE_DMA_READ || s->dma_cmd == IDE_DMA_WRITE) &&

  !ide_sect_range_ok(s, sector_num, n)) {
@@ -2383,9 +2388,7 @@ void ide_bus_reset(IDEBus *bus)
  
  /* pending async DMA */

  if (bus->dma->aiocb) {
-#ifdef DEBUG_AIO
-printf("aio_cancel\n");
-#endif
+trace_ide_bus_reset_aio();
  blk_aio_cancel(bus->dma->aiocb);
  bus->dma->aiocb = NULL;
  }
diff --git a/hw/ide/trace-events b/hw/ide/trace-events
index ade1e76..6e33cb3 100644
--- a/hw/ide/trace-events
+++ b/hw/ide/trace-events
@@ -17,6 +17,8 @@ ide_cancel_dma_sync_remaining(void) "draining all remaining 
requests"
  ide_sector_read(int64_t sector_num, int nsectors) "sector=%"PRId64" 
nsectors=%d"
  ide_sector_write(int64_t sector_num, int nsectors) "sector=%"PRId64" 
nsectors=%d"
  ide_reset(void *s) "IDEstate %p"
+ide_bus_reset_aio(void) "aio_cancel"
+ide_dma_cb(void *s, int64_t sector_num, int n, const char *dma) "IDEState %p; 
sector_num=%"PRId64" n=%d cmd=%s"
  
  # hw/ide/pci.c

  bmdma_reset(void) ""
@@ -48,5 +50,6 @@ ide_atapi_cmd_reply_end_new(void *s, int status) "IDEState: 
%p; new transfer sta
  ide_atapi_cmd_check_status(void *s) "IDEState: %p"
  ide_atapi_cmd_read(void *s, const char *method, int lba, int nb_sectors) 
"IDEState: %p; read %s: LBA=%d nb_sectors=%d"
  ide_atapi_cmd(void *s, uint8_t cmd) "IDEState: %p; cmd: 0x%02x"
+ide_atapi_cmd_read_dma_cb_aio(void *s, int lba, int n) "IDEState: %p; aio read: 
lba=%d n=%d"
  # Warning: Verbose
  ide_atapi_cmd_packet(void *s, uint16_t limit, const char *packet) "IDEState: %p; 
limit=0x%x packet: %s"
\ No newline at end of file
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 74efe8a..97e97a2 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -14,7 +14,6 @@
  #include "block/scsi.h"
  
  /* debug IDE devices */

-//#define DEBUG_AIO
  #define USE_DMA_CDROM
  
  typedef struct IDEBus IDEBus;

@@ -333,12 +332,16 @@ struct unreported_events {
  };
  
  enum ide_dma_cmd {

-IDE_DMA_READ,
+IDE_DMA__BEGIN = 0,
+IDE_DMA_READ = IDE_DMA__BEGIN,


not that useful, you can use directly:

IDE_DMA_READ = 0,


  IDE_DMA_WRITE,
  IDE_DMA_TRIM,
  IDE_DMA_ATAPI,
+IDE_DMA__END


IDE_DMA__COUNT sounds better.


  };
  
+extern const char *IDE_DMA_CMD_lookup[IDE_DMA__END];

+
  #define ide_cmd_is_read(s) \
((s)->dma_cmd == IDE_DMA_READ)
  



removing IDE_DMA__BEGIN and using IDE_DMA__COUNT:
Reviewed-by: Philippe Mathieu-Daudé 

Re: [Qemu-devel] [RFC PATCH qemu] exec: Destroy dispatch immediately

[Peter Maydell]

On 25 August 2017 at 14:19, David Gibson  wrote:
> Hm.  Why do we need to construct full ASes for virtio-blk, rather than
> just MRs?

It's the PCI layer that's doing it, but the overall reason is because
virtio-blk needs to make memory transactions (DMA reads and writes).
The AddressSpace is our construct for "thing you use to be able
to efficiently make memory transactions on a MemoryRegion".
(This is what the FlatView and AddressSpaceDispatch data structures
are for -- they let you efficiently go from "write to address 0x4000
in this thing" to the actual destination RAM or device read/write
pointers.)

thanks
-- PMM

Re: [Qemu-devel] Persistent bitmaps for non-qcow2 formats

[Max Reitz]

On 2017-08-25 02:55, John Snow wrote:
> Sorry in advance for :words: ...
> 
> On 08/23/2017 02:04 PM, Vladimir Sementsov-Ogievskiy wrote:
>> 23.08.2017 11:59, Vladimir Sementsov-Ogievskiy wrote:
>>> 22.08.2017 22:07, John Snow wrote:
 Well, we knew we'd want this sooner or later. I've got some pings
 downstream over whether or not we support persistent bitmaps for
 non-qcow2 formats.

 Currently: no, we don't.

 We tried two different ideas for storage agnostic bitmap persistence:


 (1) Using qcow2 as a storage container format (similar to VM save
 states) for information not associated with that particular drive.

 This didn't go over so well; we decided it was too messy logistically to
 manage data in any meaningful sense in a file that didn't share any
 semantic relationship to the qcow2 in question.

 Well, "We" decided is more like "Kevin and Max" decided. They are right,
 though.


 (2) Using a new proto-wrapper format that Fam Zheng designed that serves
 as a simple top-layer redirect for metadata that allows us to associate
 raw bitmap data with an arbitrary backing image.

 This also didn't go over so well, the desire for a "new format" was
 pretty thin, even if it was only a pseudo-format.


 We'd still like to be able to use bitmaps with non-qcow2 formats
 however, and people are going to keep asking. So here's a third thought:


 (3) Add either a new flag that turns qcow2's backing file into a full
 R/W backing file, or add a new extension to qcow2 entirely (bypassing
 the traditional backing file mechanism to avoid confusion for older
 tooling) that adds a new read-write backing file field.

 This RW backing file field will be used for all reads AND writes; the
 qcow2 in question becomes a metadata container on top of the BDS chain.
 We can re-use Vladimir's bitmap persistence extension to save bitmaps in
 a qcow2 shell.

 The qcow2 becomes effectively a metadata cache for a new (essentially)
 filter node that handles features such as bitmaps. This could also be
 used to provide allocation map data for RAW files and other goodies down
 the road.

 Hopefully this achieves our desire to not create new formats AND our
 desire to concentrate features (and debugging, testing, etc) into qcow2,
 while allowing users to "have bitmaps with raw files."

 Of course, in this scenario, users now have two files: a qcow2 wrapper
 and the actual raw file in question; but regardless of how we were going
 to solve this, a raw file necessitates an external file of some sort,
 else we give up the idea that it was a raw file.


 Sound good?
>>>
>>> Looks interesting. It is a clean reusing of qcow2-bitmaps without any
>>> modifications to them.
>>>
>>> Should there be some problems with internal snapshots and other things?

I'd suspect you get exactly the same problems when using internal
snapshots together with backing files.  Imagine a newly created overlay
file and taking a snapshot.  This should give you exactly the same
issue, doesn't it?

>>>
>>>
>>>
>> Hm. looks like that this backing file should not only receive all reads
>> and writes, but almost everything ->bdrv_ handlers, except bitmap
>> related of course.

How so?  Shouldn't it just work like a backing file, except it also
receives writes instead of just reads?

>>This doesn't seems simple to implement. Especially if
>> imaging some not-raw feature-full format under this thin qcow2 layer..
>> Or we can restrict this RW backing file to be raw-only?
>>
>>
> 
> The idea would really be to support any arbitrary data store, so I
> wouldn't want to restrict it to just raw.
> 
> You're right though, this might be a kind of messy approach.
> 
> [From your other mail:]
> 
>>
>> So, anyway, I see only two differences (from the outside) between this 
>> approach and just a separate bitmap-only qcow2 without a data:
>>
> 
> It's very delicately similar, yes.
> 
>> 1. in RW-backing approach qcow2-bitmap file has a link to data file (as a 
>> backing). It looks good.

And this is rather important to me.

> Right. The information necessary to establish a link between the bitmap
> data and the data being described is fully contained within a file fully
> specified by the QCOW2 spec.
> 
>> 2. in RW-backing approach qcow2-bitmap file is a top of the virtual disk, in 
>> separate-file approach it is an option of the real data drive. In my opinion 
>> the second is more clean for users ("to add this feature you should use 
>> other file as your disk" vs "to add this feature you should add an option to 
>> your disk description")

I'd argue it's rather: "You cannot use this feature unless your format
supports it.  The only format supporting persistent bitmaps currently is
qcow2.  To use persistent bitmaps with other 

Re: [Qemu-devel] [PATCH 2/9] IDE: Add register hints to tracing

[Philippe Mathieu-Daudé]

On 08/08/2017 03:32 PM, John Snow wrote:

Name the registers for tracing purposes.

Signed-off-by: John Snow 
---
  hw/ide/core.c   | 88 +
  hw/ide/trace-events |  4 +--
  2 files changed, 70 insertions(+), 22 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 53fa084..6235bdf 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1177,13 +1177,37 @@ static void ide_clear_hob(IDEBus *bus)
  bus->ifs[1].select &= ~(1 << 7);
  }
  
+/* IOport [W]rite [R]egisters */

+enum ATA_IOPORT_WR {
+ATA_IOPORT_WR_DATA = 0,
+ATA_IOPORT_WR_FEATURES = 1,
+ATA_IOPORT_WR_SECTOR_COUNT = 2,
+ATA_IOPORT_WR_SECTOR_NUMBER = 3,
+ATA_IOPORT_WR_CYLINDER_LOW = 4,
+ATA_IOPORT_WR_CYLINDER_HIGH = 5,
+ATA_IOPORT_WR_DEVICE_HEAD = 6,
+ATA_IOPORT_WR_COMMAND = 7,
+ATA_IOPORT_WR_NUM_REGISTERS,
+};
+
+const char *ATA_IOPORT_WR_lookup[ATA_IOPORT_WR_NUM_REGISTERS] = {
+[ATA_IOPORT_WR_DATA] = "Data",
+[ATA_IOPORT_WR_FEATURES] = "Features",
+[ATA_IOPORT_WR_SECTOR_COUNT] = "Sector Count",
+[ATA_IOPORT_WR_SECTOR_NUMBER] = "Sector Number",
+[ATA_IOPORT_WR_CYLINDER_LOW] = "Cylinder Low",
+[ATA_IOPORT_WR_CYLINDER_HIGH] = "Cylinder High",
+[ATA_IOPORT_WR_DEVICE_HEAD] = "Device/Head",
+[ATA_IOPORT_WR_COMMAND] = "Command"
+};
+
  void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val)
  {
  IDEBus *bus = opaque;
  IDEState *s = idebus_active_if(bus);
  int reg_num = addr & 7;
  
-trace_ide_ioport_write(addr, val, bus, s);

+trace_ide_ioport_write(addr, ATA_IOPORT_WR_lookup[reg_num], val, bus, s);
  
  /* ignore writes to command block while busy with previous command */

  if (reg_num != 7 && (s->status & (BUSY_STAT|DRQ_STAT))) {
@@ -1193,43 +1217,43 @@ void ide_ioport_write(void *opaque, uint32_t addr, 
uint32_t val)
  switch(reg_num) {
  case 0:
  break;
-case 1:
-   ide_clear_hob(bus);
+case ATA_IOPORT_WR_FEATURES:
+ide_clear_hob(bus);
  /* NOTE: data is written to the two drives */
-   bus->ifs[0].hob_feature = bus->ifs[0].feature;
-   bus->ifs[1].hob_feature = bus->ifs[1].feature;
+bus->ifs[0].hob_feature = bus->ifs[0].feature;
+bus->ifs[1].hob_feature = bus->ifs[1].feature;
  bus->ifs[0].feature = val;
  bus->ifs[1].feature = val;
  break;
-case 2:
+case ATA_IOPORT_WR_SECTOR_COUNT:
ide_clear_hob(bus);
bus->ifs[0].hob_nsector = bus->ifs[0].nsector;
bus->ifs[1].hob_nsector = bus->ifs[1].nsector;
  bus->ifs[0].nsector = val;
  bus->ifs[1].nsector = val;
  break;
-case 3:
+case ATA_IOPORT_WR_SECTOR_NUMBER:
ide_clear_hob(bus);
bus->ifs[0].hob_sector = bus->ifs[0].sector;
bus->ifs[1].hob_sector = bus->ifs[1].sector;
  bus->ifs[0].sector = val;
  bus->ifs[1].sector = val;
  break;
-case 4:
+case ATA_IOPORT_WR_CYLINDER_LOW:
ide_clear_hob(bus);
bus->ifs[0].hob_lcyl = bus->ifs[0].lcyl;
bus->ifs[1].hob_lcyl = bus->ifs[1].lcyl;
  bus->ifs[0].lcyl = val;
  bus->ifs[1].lcyl = val;
  break;
-case 5:
+case ATA_IOPORT_WR_CYLINDER_HIGH:
ide_clear_hob(bus);
bus->ifs[0].hob_hcyl = bus->ifs[0].hcyl;
bus->ifs[1].hob_hcyl = bus->ifs[1].hcyl;
  bus->ifs[0].hcyl = val;
  bus->ifs[1].hcyl = val;
  break;
-case 6:
+case ATA_IOPORT_WR_DEVICE_HEAD:
/* FIXME: HOB readback uses bit 7 */
  bus->ifs[0].select = (val & ~0x10) | 0xa0;
  bus->ifs[1].select = (val | 0x10) | 0xa0;
@@ -1237,7 +1261,7 @@ void ide_ioport_write(void *opaque, uint32_t addr, 
uint32_t val)
  bus->unit = (val >> 4) & 1;
  break;
  default:
-case 7:
+case ATA_IOPORT_WR_COMMAND:
  /* command */
  ide_exec_cmd(bus, val);
  break;
@@ -2044,6 +2068,30 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
  }
  }
  
+/* IOport [R]ead [R]egisters */

+enum ATA_IOPORT_RR {
+ATA_IOPORT_RR_DATA = 0,
+ATA_IOPORT_RR_ERROR = 1,
+ATA_IOPORT_RR_SECTOR_COUNT = 2,
+ATA_IOPORT_RR_SECTOR_NUMBER = 3,
+ATA_IOPORT_RR_CYLINDER_LOW = 4,
+ATA_IOPORT_RR_CYLINDER_HIGH = 5,
+ATA_IOPORT_RR_DEVICE_HEAD = 6,
+ATA_IOPORT_RR_STATUS = 7,
+ATA_IOPORT_RR_NUM_REGISTERS,
+};
+
+const char *ATA_IOPORT_RR_lookup[ATA_IOPORT_RR_NUM_REGISTERS] = {
+[ATA_IOPORT_RR_DATA] = "Data",
+[ATA_IOPORT_RR_ERROR] = "Error",
+[ATA_IOPORT_RR_SECTOR_COUNT] = "Sector Count",
+[ATA_IOPORT_RR_SECTOR_NUMBER] = "Sector Number",
+[ATA_IOPORT_RR_CYLINDER_LOW] = "Cylinder Low",
+[ATA_IOPORT_RR_CYLINDER_HIGH] = "Cylinder High",
+[ATA_IOPORT_RR_DEVICE_HEAD] = "Device/Head",
+[ATA_IOPORT_RR_STATUS] = "Status"
+};
+
  uint32_t ide_ioport_read(void *opaque, uint32_t addr)
  {
  IDEBus *bus = opaque;
@@ -2056,10 +2104,10 @@ 

Re: [Qemu-devel] [RFC PATCH qemu] exec: Destroy dispatch immediately

[David Gibson]

On Fri, Aug 25, 2017 at 11:57:26AM +0200, Paolo Bonzini wrote:
> On 25/08/2017 11:22, Peter Maydell wrote:
> > On 25 August 2017 at 09:53, Paolo Bonzini  wrote:
> >> The solution is to: 1) share the FlatView structures if they refer to
> >> the same root memory region; 2) have one AddressSpaceDispatch per
> >> FlatView instead of one per AddressSpace, so that FlatView reference
> >> counting takes care of clearing the AddressSpaceDispatch too.  Neither
> >> is particularly hard.
> > If we did this we could get rid of address_space_init_shareable(),
> > right? (It's a bit of a cheesy hack aimed at avoiding having duplicate
> > address space structures for the same root memory region, but if
> > the underlying code is better at not duplicating all the data
> > structures unless necessary then the benefit of having the
> > separate API goes away I think.)
> 
> Yes, indeed.

Hm.  Why do we need to construct full ASes for virtio-blk, rather than
just MRs?

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature